Skip to content

Open swe/a9cd0eb9 3172 4fe8 a64c 5ef191d4b266#36

Open
fernando-plank wants to merge 16 commits into
mainfrom
open-swe/a9cd0eb9-3172-4fe8-a64c-5ef191d4b266
Open

Open swe/a9cd0eb9 3172 4fe8 a64c 5ef191d4b266#36
fernando-plank wants to merge 16 commits into
mainfrom
open-swe/a9cd0eb9-3172-4fe8-a64c-5ef191d4b266

Conversation

@fernando-plank

@fernando-plank fernando-plank commented Feb 11, 2026

Copy link
Copy Markdown
Owner

EntelligenceAI PR Summary

This PR simplifies the getThreadAndRunIds() utility function by removing thread_id extraction and return value.

  • Removed thread_id from the return type, changing from { thread_id?: string; run_id?: string } to { run_id?: string }
  • Updated function implementation to only extract and return run_id from LangGraph config
  • Streamlines logging context information by removing unused thread_id field

Summary by CodeRabbit

Release Notes

  • Refactor

    • Streamlined concurrent tool execution for improved performance
    • Migrated URL content caching to external persistence layer for better data durability
    • Updated session management and execution tracking identifiers
  • Bug Fixes

    • Enhanced error handling in tool execution with clearer error messages
  • Documentation

    • Added comprehensive database schema documentation

@coderabbitai

coderabbitai Bot commented Feb 11, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This pull request introduces an HTTP-based database client for persisting graph state, refactors tool execution flow to support concurrent processing with distributed caching, migrates from in-memory document caching to external cache via HTTP, and restructures public TypeScript types with reorganized graph state and configuration schemas.

Changes

Cohort / File(s) Summary
HTTP-Based Infrastructure
apps/open-swe/src/utils/http-db-client.ts
New module providing HTTP client for CRUD operations on GraphState, GraphConfig, and document cache. Exposes 10 async functions (createThread, createRun, updateRunStatus, saveGraphState, getGraphState, saveGraphConfig, getGraphConfig, getDocumentFromCache, saveDocumentToCache, clearDocumentCache) with configurable API_BASE_URL endpoint.
Caching and Tool Refactoring
apps/open-swe/src/tools/url-content.ts
Updated createGetURLContentTool signature from state parameter to runId: string. Replaced in-memory graph-based caching with distributed cache via getDocumentFromCache/saveDocumentToCache. Removed stateUpdates return pathway from tool result.
Tool Execution Pipeline
apps/open-swe/src/graphs/planner/nodes/take-action.ts
Refactored takeActions to use concurrent map-based execution over tool_calls, single sandbox retrieval, and uniform error handling. Added extraction logic for codebaseTree and dependenciesInstalled from shell tool outputs. Updated GetURLContentTool invocation to use config.run_id. Removed prior state merging and allStateUpdates logic.
Configuration and Logging
apps/open-swe/src/graphs/programmer/nodes/request-help.ts, apps/open-swe/src/utils/logger.ts
Replaced thread_id with run_id for obtaining Open SWE run URL. Removed thread_id handling from getThreadAndRunIds; now returns only run_id (optional).
Type System Restructuring
packages/shared/src/open-swe/types.ts
Major reorganization of public types: Task simplified (removed summary, planRevisions, etc.); GraphAnnotation restructured with new LangGraph-wrapped fields (tasks, planRevisions, modelTokenData, initialPrompt, problemStatement, uiMessages, codebaseTree, dependenciesInstalled); GraphConfigurationMetadata converted from verbose object to Record-based structure; GraphConfig adds run_id, removes thread_id.
Documentation
docs/code_changes.md, docs/database_schema.md
Added architecture documentation describing HTTP client integration and URL content tool migration. Introduced proposed PostgreSQL relational schema with five tables (threads, runs, graph_state, graph_config, document_cache) using JSONB for complex objects and composite/foreign keys for referential integrity.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Node (take-action)
    participant HttpClient as HTTP Client
    participant API as API Server
    participant DB as Database

    Client->>HttpClient: getDocumentFromCache(runId, url)
    HttpClient->>API: GET /document-cache?runId=...&url=...
    API->>DB: Query document_cache table
    DB-->>API: Return cached content or null
    API-->>HttpClient: Response
    
    alt Cache Hit
        HttpClient-->>Client: Document content
    else Cache Miss
        Client->>Client: Fetch via FireCrawl
        Client->>HttpClient: saveDocumentToCache(runId, url, content)
        HttpClient->>API: POST /document-cache
        API->>DB: Insert/update document_cache
        DB-->>API: Success
        API-->>HttpClient: Acknowledgment
        HttpClient-->>Client: Done
    end
Loading
sequenceDiagram
    participant Planner as Planner Node
    participant ToolExec as Tool Executor
    participant Sandbox as Sandbox
    participant HttpClient as HTTP Client
    participant Shell as Shell Tool

    Planner->>ToolExec: invoke all tool_calls concurrently
    ToolExec->>HttpClient: getSandboxWithErrorHandling(config)
    HttpClient-->>ToolExec: sandbox instance
    
    par Parallel Tool Invocations
        ToolExec->>Shell: invoke(args, config + runId + sandbox)
        Shell->>Sandbox: execute command
        Sandbox-->>Shell: output
        Shell-->>ToolExec: tool result
        ToolExec->>ToolExec: processToolCallContent(output)
        ToolExec->>ToolExec: extract codebaseTree/dependenciesInstalled if applicable
    end
    
    ToolExec->>ToolExec: construct commandUpdate with codebaseTree & dependenciesInstalled
    ToolExec-->>Planner: ToolMessage[] with results and stateUpdates
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 Code hops through the database door,
Where caches dance with HTTP lore,
Types restructured, clean and bright,
Tool execution takes its flight—
From memory's nest to storage's might!

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 43.75% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The PR title is a UUID-like string that provides no meaningful information about the changes, which include significant refactoring of tool execution, migration to distributed caching, and restructuring of public TypeScript types. Replace the UUID-based title with a descriptive summary of the main changes, such as 'Migrate to distributed caching and refactor graph state management' or 'Add HTTP database client and restructure GraphState types'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch open-swe/a9cd0eb9-3172-4fe8-a64c-5ef191d4b266

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Comment @coderabbitai help to get the list of available commands and usage tips.

@entelligence-ai-pr-reviews

Copy link
Copy Markdown

Walkthrough

This PR simplifies the logging utility by removing the thread_id field from the getThreadAndRunIds() helper function. The function previously extracted and returned both thread_id and run_id from the LangGraph config, but now only returns run_id. This change streamlines the returned object structure and indicates that thread_id is no longer required by the consumers of this utility function, reducing unnecessary data in the logging context.

Changes

File(s) Summary
apps/open-swe/src/utils/logger.ts Removed thread_id from the return type and implementation of getThreadAndRunIds() helper function; function now only extracts and returns run_id from LangGraph config.

Sequence Diagram

This diagram shows the interactions between components:

sequenceDiagram
    participant Caller
    participant getThreadAndRunIds
    participant getConfig
    participant Config

    Caller->>getThreadAndRunIds: Call function
    activate getThreadAndRunIds
    
    getThreadAndRunIds->>getConfig: getConfig()
    activate getConfig
    getConfig->>Config: Access configuration
    Config-->>getConfig: Return config object
    getConfig-->>getThreadAndRunIds: config
    deactivate getConfig
    
    Note over getThreadAndRunIds: Extract run_id from<br/>config.configurable?.run_id
    
    Note over getThreadAndRunIds: ❌ REMOVED: thread_id extraction<br/>✅ KEPT: run_id extraction
    
    getThreadAndRunIds-->>Caller: Return { run_id }
    deactivate getThreadAndRunIds
    
    Note over Caller: Previously returned:<br/>{ thread_id, run_id }<br/>Now returns:<br/>{ run_id }
Loading

🔗 Cross-Repository Impact Analysis

Enable automatic detection of breaking changes across your dependent repositories. → Set up now

Learn more about Cross-Repository Analysis

What It Does

  • Automatically identifies repositories that depend on this code
  • Analyzes potential breaking changes across your entire codebase
  • Provides risk assessment before merging to prevent cross-repo issues

How to Enable

  1. Visit Settings → Code Management
  2. Configure repository dependencies
  3. Future PRs will automatically include cross-repo impact analysis!

Benefits

  • 🛡️ Prevent breaking changes across repositories
  • 🔍 Catch integration issues before they reach production
  • 📊 Better visibility into your multi-repo architecture

▶️AI Code Reviews for VS Code, Cursor, Windsurf
Install the extension

Note for Windsurf Please change the default marketplace provider to the following in the windsurf settings:

Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery

Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items

Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below

Emoji Descriptions:

  • ⚠️ Potential Issue - May require further investigation.
  • 🔒 Security Vulnerability - Fix to ensure system safety.
  • 💻 Code Improvement - Suggestions to enhance code quality.
  • 🔨 Refactor Suggestion - Recommendations for restructuring code.
  • ℹ️ Others - General comments and information.

Interact with the Bot:

  • Send a message or request using the format:
    @entelligenceai + *your message*
Example: @entelligenceai Can you suggest improvements for this code?
  • Help the Bot learn by providing feedback on its responses.
    @entelligenceai + *feedback*
Example: @entelligenceai Do not comment on `save_auth` function !

Also you can trigger various commands with the bot by doing
@entelligenceai command

The current supported commands are

  1. config - shows the current config
  2. retrigger_review - retriggers the review

More commands to be added soon.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/shared/src/open-swe/types.ts (1)

376-377: ⚠️ Potential issue | 🟡 Minor

Typo in JSDoc comment: "maxium" should be "maximum".

📝 Proposed fix
   /**
-   * The maxium number of times the reviewer subgraph can be executed.
+   * The maximum number of times the reviewer subgraph can be executed.
    */
🤖 Fix all issues with AI agents
In `@apps/open-swe/src/graphs/planner/nodes/take-action.ts`:
- Around line 91-98: The object passed to tool.invoke contains a duplicate runId
property (uuidv4() and config.run_id) so remove the dead/duplicate assignment
and ensure the tool receives the intended consistent run identifier; update the
call site in the tool.invoke invocation (the object built around toolCall.args,
config, sandbox) to only set runId using config.run_id (or, if you prefer to
generate when missing, set runId: config.run_id ?? uuidv4()), leaving sandbox
spread and other config intact.

In `@apps/open-swe/src/tools/url-content.ts`:
- Around line 27-30: The code uses parsedUrl = urlParseResult.url?.href which
can be undefined even when urlParseResult.success is true; ensure parsedUrl is a
string before calling getDocumentFromCache and saveDocumentToCache by adding a
type guard or assertion immediately after verifying urlParseResult.success
(e.g., check that urlParseResult.url and urlParseResult.url.href are defined),
and if not present return or throw a descriptive error; update places that call
getDocumentFromCache(runId, parsedUrl) and saveDocumentToCache(runId, parsedUrl,
...) to only run when parsedUrl is guaranteed to be a string.

In `@apps/open-swe/src/utils/http-db-client.ts`:
- Around line 14-22: The makeRequest helper currently always calls
response.json(), which throws on empty bodies (e.g., 204 No Content) and breaks
callers like updateRunStatus, saveGraphState, saveGraphConfig,
saveDocumentToCache, and clearDocumentCache that expect Promise<void>; modify
makeRequest so it first checks for an empty response (response.status === 204,
content-length === '0', or missing/empty body) or non-JSON responses (check
response.headers.get('content-type') for 'application/json') and only call
response.json() when a JSON body is present, otherwise return undefined (or
void) so callers that expect no body do not fail. Ensure the check happens right
before the current return response.json() in makeRequest.
- Around line 81-84: The catch block in the try/catch in http-db-client.ts
currently swallows all errors and returns null; update it to only treat a 404 as
"not found" and surface other failures: inside the catch, test the error for an
HTTP response status (e.g. (error as any).response?.status === 404) and return
null only in that case, otherwise rethrow or return a rejected promise so
callers can see real network/5xx errors; ensure this change is applied in the
function that contains that try/catch (the HTTP DB client fetch/get helper) and
preserve existing typing and behavior for the 404 path.

In `@docs/code_changes.md`:
- Line 91: The docs incorrectly say createGetURLContentTool is called with
config.thread_id whereas the implementation in take-action.ts passes
config.run_id; update the documentation to match the code by changing the
mention to config.run_id (or, alternatively, change the code to use
config.thread_id if that is the intended identifier), ensuring consistency
between the doc and the createGetURLContentTool call and referencing the
createGetURLContentTool invocation and the config.run_id/config.thread_id
symbols to locate and verify the fix.
- Around line 101-102: Remove the trailing incomplete fenced code block at the
end of the file by either deleting the lone triple-backtick fence or completing
it with the intended code and an optional language specifier; locate the
dangling "```" near the end of the document (after the line about
`documentCache` deletion) and remove or replace it so the markdown ends cleanly.

In `@docs/database_schema.md`:
- Around line 117-132: The docs state `url` is a PRIMARY KEY but the SQL for
table `document_cache` defines a composite primary key `(run_id, url)`; update
the column list under "Columns" to reflect that `url` is part of a composite
primary key (remove "PRIMARY KEY" from the `url` line and document that the
table-level constraint `PRIMARY KEY (run_id, url)` applies), and ensure `run_id`
is described as part of the composite key for `document_cache`.
- Line 13: The schema uses MySQL's `ON UPDATE CURRENT_TIMESTAMP` on the
`updated_at` column which is invalid for PostgreSQL; remove the `ON UPDATE
CURRENT_TIMESTAMP` clause from all `updated_at` column definitions (e.g., the
`updated_at` column mentioned in the diff and the other occurrences referenced
in the comment) and instead add a single trigger function (e.g.,
`update_updated_at_column`) that sets `NEW.updated_at = CURRENT_TIMESTAMP`, then
create per-table triggers (e.g., `update_threads_updated_at`) that call that
function BEFORE UPDATE FOR EACH ROW on each table needing automatic updates.
🧹 Nitpick comments (4)
apps/open-swe/src/utils/http-db-client.ts (1)

5-12: Consider adding request timeouts to prevent indefinite hangs.

The fetch calls have no timeout configured. If the API server is slow or unresponsive, requests could hang indefinitely, blocking the agent.

♻️ Proposed enhancement with AbortController timeout
-async function makeRequest(url: string, method: string, body?: unknown) {
+async function makeRequest(url: string, method: string, body?: unknown, timeoutMs = 30000) {
+  const controller = new AbortController();
+  const timeoutId = setTimeout(() => controller.abort(), timeoutMs);
+
-  const response = await fetch(url, {
+  try {
+    const response = await fetch(url, {
       method,
       headers: {
         "Content-Type": "application/json",
       },
       body: body ? JSON.stringify(body) : undefined,
+      signal: controller.signal,
     });
+    // ... rest of the function
+  } finally {
+    clearTimeout(timeoutId);
+  }
apps/open-swe/src/graphs/planner/nodes/take-action.ts (2)

139-150: Fragile command detection heuristics.

The checks command.includes("ls -R") and command.includes("yarn install") are brittle:

  • ls -R could appear in various forms (ls -laR, ls -R ., etc.)
  • yarn install detection doesn't account for npm install, pnpm install, or partial matches in unrelated commands

Consider using more robust pattern matching or explicit tool metadata to track these state changes.

♻️ Example of more robust detection
-    if (command.includes("ls -R")) {
+    // More flexible pattern for recursive listing
+    if (/\bls\b.*-[^\s]*R/.test(command) || /\btree\b/.test(command)) {
       const toolCallResult = toolCallResults.find(
         (tc) => tc.tool_call_id === shellToolCall.id,
       );
       if (toolCallResult) {
         codebaseTree = toolCallResult.content;
       }
-    } else if (command.includes("yarn install")) {
+    // More flexible pattern for package installation
+    } else if (/\b(yarn|npm|pnpm)\s+(install|i|add)\b/.test(command)) {
       dependenciesInstalled = true;
     }

128-130: Destructuring stateUpdates from tools that may not return it.

The URL content tool was updated to no longer return stateUpdates, but the code here still attempts to destructure it. While the spread operator handles missing properties gracefully, this could cause confusion and the variable is immediately discarded.

If no tools return stateUpdates anymore, consider removing this destructuring pattern entirely.

packages/shared/src/open-swe/types.ts (1)

214-241: Update model defaults to latest Claude versions.

The hardcoded model defaults reference outdated versions: claude-3.5-sonnet-20240620 (June 2024) and claude-3-haiku-20240307 (March 2024). As of February 2026, newer versions are available: claude-sonnet-4-20250514, claude-3-7-sonnet-20250219 for Sonnet, and claude-3-5-haiku-20241022 for Haiku. Update defaults to use the latest available models or implement a mechanism to pull the current versions dynamically.

Comment on lines +91 to +98
const toolOutput = await tool.invoke(toolCall.args, {
...config,
runId: uuidv4(),
runId: config.run_id,
...(sandbox && {
sandbox,
}),
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Duplicate runId property in object literal.

Lines 93-94 both define runId. The second assignment (runId: config.run_id) overwrites the first (runId: uuidv4()), making line 93 dead code. This appears to be a merge conflict or copy-paste error.

Based on the context (the tool needs a consistent run identifier for caching), config.run_id is likely the intended value.

🐛 Proposed fix
         const toolOutput = await tool.invoke(toolCall.args, {
           ...config,
-          runId: uuidv4(),
           runId: config.run_id,
           ...(sandbox && {
             sandbox,
           }),
         });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const toolOutput = await tool.invoke(toolCall.args, {
...config,
runId: uuidv4(),
runId: config.run_id,
...(sandbox && {
sandbox,
}),
});
const toolOutput = await tool.invoke(toolCall.args, {
...config,
runId: config.run_id,
...(sandbox && {
sandbox,
}),
});
🧰 Tools
🪛 Biome (2.3.14)

[error] 93-93: This property is later overwritten by an object member with the same name.

Overwritten with this property.

If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.

(lint/suspicious/noDuplicateObjectKeys)

🤖 Prompt for AI Agents
In `@apps/open-swe/src/graphs/planner/nodes/take-action.ts` around lines 91 - 98,
The object passed to tool.invoke contains a duplicate runId property (uuidv4()
and config.run_id) so remove the dead/duplicate assignment and ensure the tool
receives the intended consistent run identifier; update the call site in the
tool.invoke invocation (the object built around toolCall.args, config, sandbox)
to only set runId using config.run_id (or, if you prefer to generate when
missing, set runId: config.run_id ?? uuidv4()), leaving sandbox spread and other
config intact.

Comment on lines 27 to +30
const parsedUrl = urlParseResult.url?.href;

try {
let documentContent = state.documentCache[parsedUrl];
let documentContent = await getDocumentFromCache(runId, parsedUrl);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

parsedUrl could be undefined when passed to cache functions.

When urlParseResult.success is true, urlParseResult.url should exist, but the optional chaining on line 27 (urlParseResult.url?.href) can still result in parsedUrl being undefined. This undefined value is then passed to getDocumentFromCache and saveDocumentToCache, which expect a string.

Consider adding a type guard or asserting the value after the success check.

🛡️ Proposed fix
       const parsedUrl = urlParseResult.url?.href;
+      if (!parsedUrl) {
+        return { result: "Failed to parse URL", status: "error" };
+      }

       try {
         let documentContent = await getDocumentFromCache(runId, parsedUrl);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const parsedUrl = urlParseResult.url?.href;
try {
let documentContent = state.documentCache[parsedUrl];
let documentContent = await getDocumentFromCache(runId, parsedUrl);
const parsedUrl = urlParseResult.url?.href;
if (!parsedUrl) {
return { result: "Failed to parse URL", status: "error" };
}
try {
let documentContent = await getDocumentFromCache(runId, parsedUrl);
🤖 Prompt for AI Agents
In `@apps/open-swe/src/tools/url-content.ts` around lines 27 - 30, The code uses
parsedUrl = urlParseResult.url?.href which can be undefined even when
urlParseResult.success is true; ensure parsedUrl is a string before calling
getDocumentFromCache and saveDocumentToCache by adding a type guard or assertion
immediately after verifying urlParseResult.success (e.g., check that
urlParseResult.url and urlParseResult.url.href are defined), and if not present
return or throw a descriptive error; update places that call
getDocumentFromCache(runId, parsedUrl) and saveDocumentToCache(runId, parsedUrl,
...) to only run when parsedUrl is guaranteed to be a string.

Comment on lines +14 to +22
if (!response.ok) {
const errorText = await response.text();
throw new Error(
`HTTP error! status: ${response.status}, message: ${errorText}`,
);
}

return response.json();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

response.json() may fail on empty response bodies.

makeRequest unconditionally calls response.json() on line 21, but HTTP responses like 204 No Content have no body and will throw. Functions like updateRunStatus, saveGraphState, saveGraphConfig, saveDocumentToCache, and clearDocumentCache return Promise<void> but still attempt to parse JSON.

🐛 Proposed fix
   if (!response.ok) {
     const errorText = await response.text();
     throw new Error(
       `HTTP error! status: ${response.status}, message: ${errorText}`,
     );
   }

-  return response.json();
+  const contentType = response.headers.get("content-type");
+  if (contentType && contentType.includes("application/json")) {
+    return response.json();
+  }
+  return null;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!response.ok) {
const errorText = await response.text();
throw new Error(
`HTTP error! status: ${response.status}, message: ${errorText}`,
);
}
return response.json();
}
if (!response.ok) {
const errorText = await response.text();
throw new Error(
`HTTP error! status: ${response.status}, message: ${errorText}`,
);
}
const contentType = response.headers.get("content-type");
if (contentType && contentType.includes("application/json")) {
return response.json();
}
return null;
}
🤖 Prompt for AI Agents
In `@apps/open-swe/src/utils/http-db-client.ts` around lines 14 - 22, The
makeRequest helper currently always calls response.json(), which throws on empty
bodies (e.g., 204 No Content) and breaks callers like updateRunStatus,
saveGraphState, saveGraphConfig, saveDocumentToCache, and clearDocumentCache
that expect Promise<void>; modify makeRequest so it first checks for an empty
response (response.status === 204, content-length === '0', or missing/empty
body) or non-JSON responses (check response.headers.get('content-type') for
'application/json') and only call response.json() when a JSON body is present,
otherwise return undefined (or void) so callers that expect no body do not fail.
Ensure the check happens right before the current return response.json() in
makeRequest.

Comment on lines +81 to +84
} catch (error) {
// Assuming a 404 or other error means not found
return null;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Overly broad error handling masks real failures.

The catch block swallows all errors, treating network failures, 500 server errors, and actual 404s the same way. This could hide serious issues and make debugging difficult. Consider distinguishing between "not found" (expected) and other errors (unexpected).

🛡️ Proposed fix to differentiate error types
 export async function getDocumentFromCache(
   runId: string,
   url: string,
 ): Promise<string | null> {
   try {
     const result = await makeRequest(
       `${API_BASE_URL}/document-cache?runId=${runId}&url=${encodeURIComponent(
         url,
       )}`,
       "GET",
     );
     return result.content;
   } catch (error) {
-    // Assuming a 404 or other error means not found
-    return null;
+    // Only treat as "not found" if it's a 404-like error
+    if (error instanceof Error && error.message.includes("status: 404")) {
+      return null;
+    }
+    // Re-throw unexpected errors (network failures, 500s, etc.)
+    throw error;
   }
 }
🤖 Prompt for AI Agents
In `@apps/open-swe/src/utils/http-db-client.ts` around lines 81 - 84, The catch
block in the try/catch in http-db-client.ts currently swallows all errors and
returns null; update it to only treat a 404 as "not found" and surface other
failures: inside the catch, test the error for an HTTP response status (e.g.
(error as any).response?.status === 404) and return null only in that case,
otherwise rethrow or return a rejected promise so callers can see real
network/5xx errors; ensure this change is applied in the function that contains
that try/catch (the HTTP DB client fetch/get helper) and preserve existing
typing and behavior for the 404 path.

Comment thread docs/code_changes.md
**File:** `apps/open-swe/src/graphs/planner/nodes/take-action.ts`

**Summary:**
- The `createGetURLContentTool` is now called with `config.thread_id`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Documentation inconsistency: config.thread_id vs config.run_id.

The documentation states createGetURLContentTool is called with config.thread_id, but the actual implementation in take-action.ts (line 60) uses config.run_id. This should be corrected to match the implementation.

📝 Proposed fix
 **Summary:**
--   The `createGetURLContentTool` is now called with `config.thread_id`.
+-   The `createGetURLContentTool` is now called with `config.run_id`.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- The `createGetURLContentTool` is now called with `config.thread_id`.
- The `createGetURLContentTool` is now called with `config.run_id`.
🤖 Prompt for AI Agents
In `@docs/code_changes.md` at line 91, The docs incorrectly say
createGetURLContentTool is called with config.thread_id whereas the
implementation in take-action.ts passes config.run_id; update the documentation
to match the code by changing the mention to config.run_id (or, alternatively,
change the code to use config.thread_id if that is the intended identifier),
ensuring consistency between the doc and the createGetURLContentTool call and
referencing the createGetURLContentTool invocation and the
config.run_id/config.thread_id symbols to locate and verify the fix.

Comment thread docs/code_changes.md
Comment on lines +101 to +102
- The `documentCache` property and its associated reducer were deleted from the `GraphAnnotation` schema.
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Incomplete code block at end of file.

The file ends with an empty/incomplete fenced code block that has no language specifier and appears to be a leftover from editing.

📝 Proposed fix

Remove the trailing empty code fence or complete the code example:

 -   The `documentCache` property and its associated reducer were deleted from the `GraphAnnotation` schema.
-```
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- The `documentCache` property and its associated reducer were deleted from the `GraphAnnotation` schema.
```
- The `documentCache` property and its associated reducer were deleted from the `GraphAnnotation` schema.
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)

[warning] 102-102: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
In `@docs/code_changes.md` around lines 101 - 102, Remove the trailing incomplete
fenced code block at the end of the file by either deleting the lone
triple-backtick fence or completing it with the intended code and an optional
language specifier; locate the dangling "```" near the end of the document
(after the line about `documentCache` deletion) and remove or replace it so the
markdown ends cleanly.

Comment thread docs/database_schema.md

- `thread_id` (VARCHAR(255), PRIMARY KEY): Unique identifier for the thread.
- `created_at` (TIMESTAMP, DEFAULT CURRENT_TIMESTAMP): Timestamp of when the thread was created.
- `updated_at` (TIMESTAMP, DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP): Timestamp of the last update.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

PostgreSQL does not support ON UPDATE CURRENT_TIMESTAMP syntax.

The schema claims to be PostgreSQL (line 3), but ON UPDATE CURRENT_TIMESTAMP is MySQL syntax. In PostgreSQL, you need a trigger function to automatically update updated_at.

This same issue appears on lines 35, 68, and 100.

📝 Proposed fix for PostgreSQL compatibility

Add a trigger function and apply it to tables:

-- Create the trigger function once
CREATE OR REPLACE FUNCTION update_updated_at_column()
RETURNS TRIGGER AS $$
BEGIN
    NEW.updated_at = CURRENT_TIMESTAMP;
    RETURN NEW;
END;
$$ language 'plpgsql';

-- Apply to each table, e.g.:
CREATE TRIGGER update_threads_updated_at
    BEFORE UPDATE ON threads
    FOR EACH ROW
    EXECUTE FUNCTION update_updated_at_column();

And update the column definitions to remove the invalid clause:

-    updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
+    updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP
🤖 Prompt for AI Agents
In `@docs/database_schema.md` at line 13, The schema uses MySQL's `ON UPDATE
CURRENT_TIMESTAMP` on the `updated_at` column which is invalid for PostgreSQL;
remove the `ON UPDATE CURRENT_TIMESTAMP` clause from all `updated_at` column
definitions (e.g., the `updated_at` column mentioned in the diff and the other
occurrences referenced in the comment) and instead add a single trigger function
(e.g., `update_updated_at_column`) that sets `NEW.updated_at =
CURRENT_TIMESTAMP`, then create per-table triggers (e.g.,
`update_threads_updated_at`) that call that function BEFORE UPDATE FOR EACH ROW
on each table needing automatic updates.

Comment thread docs/database_schema.md
Comment on lines +117 to +132
**Columns:**

- `run_id` (VARCHAR(255), FOREIGN KEY REFERENCES runs(run_id)): The ID of the run this cache entry belongs to.
- `url` (TEXT, PRIMARY KEY): The URL of the cached document.
- `content` (TEXT): The content of the document.
- `created_at` (TIMESTAMP, DEFAULT CURRENT_TIMESTAMP)

**SQL Definition:**

```sql
CREATE TABLE document_cache (
run_id VARCHAR(255) REFERENCES runs(run_id),
url TEXT,
content TEXT,
created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP,
PRIMARY KEY (run_id, url)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Documentation inconsistency: url described as PRIMARY KEY but schema uses composite key.

Line 120 states url (TEXT, PRIMARY KEY) but the SQL definition on line 132 correctly uses a composite primary key PRIMARY KEY (run_id, url). The column description should be updated for accuracy.

📝 Proposed fix
 **Columns:**

 -   `run_id` (VARCHAR(255), FOREIGN KEY REFERENCES runs(run_id)): The ID of the run this cache entry belongs to.
--   `url` (TEXT, PRIMARY KEY): The URL of the cached document.
+-   `url` (TEXT): The URL of the cached document.
 -   `content` (TEXT): The content of the document.
 -   `created_at` (TIMESTAMP, DEFAULT CURRENT_TIMESTAMP)
+
+**Primary Key:** Composite key on (`run_id`, `url`)
🤖 Prompt for AI Agents
In `@docs/database_schema.md` around lines 117 - 132, The docs state `url` is a
PRIMARY KEY but the SQL for table `document_cache` defines a composite primary
key `(run_id, url)`; update the column list under "Columns" to reflect that
`url` is part of a composite primary key (remove "PRIMARY KEY" from the `url`
line and document that the table-level constraint `PRIMARY KEY (run_id, url)`
applies), and ensure `run_id` is described as part of the composite key for
`document_cache`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant