{{ message }}
Repository navigation
fix: fall back past gateway allowlist rejections, propagate abort into hanging reads - #4847
Closed
CerisonAutomation wants to merge 2 commits into
Closed
CerisonAutomation wants to merge 2 commits into
CerisonAutomation wants to merge 2 commits into
Conversation
Contributor
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 18 files
Confidence score: 4/5
fallback_ai_model.ts: Normal consumer cancellation is logged as a provider stream error. Only error the wrapper for an aborted signal; letcancel()close it normally.fallback_ai_model.ts: The broad 400-error match can treat unrelated request errors as model-specific failures. Match the OpenRouter wording more narrowly, such as “only available on agentic harnesses.”fallback_ai_model.test.ts: The test does not verify that abort cancels the provider’s pending read, so a hanging read could go unnoticed. Add an observable cancellation assertion.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/ipc/utils/fallback_ai_model.test.ts">
<violation number="1" location="src/ipc/utils/fallback_ai_model.test.ts:721">
P2: This test does not verify that abort cancels the provider’s pending read. A regression that errors only the wrapper stream while leaving the underlying provider read hanging would pass; add an observable `cancel` handler and assert it receives the abort reason.</violation>
</file>
<file name="src/ipc/utils/fallback_ai_model.ts">
<violation number="1" location="src/ipc/utils/fallback_ai_model.ts:103">
P3: The new pattern is broad enough to classify unrelated 400 errors as model-specific failures. Match the OpenRouter wording more narrowly, such as `"only available on agentic harnesses"`, so deterministic request errors do not fall through the model chain.</violation>
<violation number="2" location="src/ipc/utils/fallback_ai_model.ts:789">
P3: Normal consumer cancellation is now logged as a provider stream error because `stopReading` errors the wrapper even when no abort occurred. Only error the controller for an aborted signal; let `cancel()` close the already-canceled wrapper normally.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Re-trigger cubic
| async doStream() { | ||
| return { | ||
| stream: new ReadableStream<LanguageModelV3StreamPart>({ | ||
| start() {}, |
Contributor
There was a problem hiding this comment.
P2: This test does not verify that abort cancels the provider’s pending read. A regression that errors only the wrapper stream while leaving the underlying provider read hanging would pass; add an observable cancel handler and assert it receives the abort reason.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/ipc/utils/fallback_ai_model.test.ts, line 721:
<comment>This test does not verify that abort cancels the provider’s pending read. A regression that errors only the wrapper stream while leaving the underlying provider read hanging would pass; add an observable `cancel` handler and assert it receives the abort reason.</comment>
<file context>
@@ -695,6 +705,39 @@ describe("fallback failure policy", () => {
+ async doStream() {
+ return {
+ stream: new ReadableStream<LanguageModelV3StreamPart>({
+ start() {},
+ }),
+ };
</file context>
| }); | ||
| } | ||
| try { | ||
| controller.error(options.abortSignal?.reason); |
Contributor
There was a problem hiding this comment.
P3: Normal consumer cancellation is now logged as a provider stream error because stopReading errors the wrapper even when no abort occurred. Only error the controller for an aborted signal; let cancel() close the already-canceled wrapper normally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/ipc/utils/fallback_ai_model.ts, line 789:
<comment>Normal consumer cancellation is now logged as a provider stream error because `stopReading` errors the wrapper even when no abort occurred. Only error the controller for an aborted signal; let `cancel()` close the already-canceled wrapper normally.</comment>
<file context>
@@ -747,12 +766,36 @@ class FallbackModel implements LanguageModelV3 {
+ });
+ }
+ try {
+ controller.error(options.abortSignal?.reason);
+ } catch {
+ // Controller already closed or errored; nothing left to signal.
</file context>
| // A gateway allowlist rejection for one model (for example OpenRouter's | ||
| // "… is only available on agentic harnesses") says nothing about the next | ||
| // candidate, so move down the chain instead of failing the turn. | ||
| "only available on", |
Contributor
There was a problem hiding this comment.
P3: The new pattern is broad enough to classify unrelated 400 errors as model-specific failures. Match the OpenRouter wording more narrowly, such as "only available on agentic harnesses", so deterministic request errors do not fall through the model chain.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/ipc/utils/fallback_ai_model.ts, line 103:
<comment>The new pattern is broad enough to classify unrelated 400 errors as model-specific failures. Match the OpenRouter wording more narrowly, such as `"only available on agentic harnesses"`, so deterministic request errors do not fall through the model chain.</comment>
<file context>
@@ -97,6 +97,10 @@ const FALLBACK_NEXT_ERROR_PATTERNS = [
+ // A gateway allowlist rejection for one model (for example OpenRouter's
+ // "… is only available on agentic harnesses") says nothing about the next
+ // candidate, so move down the chain instead of failing the turn.
+ "only available on",
];
</file context>
Suggested change
| "only available on", | |
| "only available on agentic harnesses", |
…o hanging reads Map gateway harness-allowlist rejections to fallback-next so one restricted free model no longer ends the turn. Wire abortSignal into the wrapped provider read and cancel(). Tests for both.
CerisonAutomation
force-pushed
the
fix/fallback-harness-restriction-abort
branch
from
October 19, 2026 01:13
fc3dd9a to
3d360d6
Compare
wwwillchen
reviewed
Oct 20, 2026
| throw new DyadError( | ||
| "doGenerate is not supported for fallback model", | ||
| DyadErrorKind.External, | ||
| async doGenerate(options: LanguageModelV3CallOptions): Promise<any> { |
Collaborator
There was a problem hiding this comment.
can you explain why you need this change?
The AI SDK step loop re-sends the ENTIRE accumulated transcript on every step. The root agent had mid-turn compaction and tool-error truncation; sub-agents had NEITHER, so input tokens grew O(steps^2) - a 100-step implementer that reads 20 files re-sends the whole transcript 100 times. On top of that, sub-agent reports were injected back into the parent with no bound, the Explorer carried a 150k-char observation registry, ~30 tool schemas were re-sent on every request, and several tool results had no source-level cap at all. Add a shared context budget for the local agent: - new subagents/subagent_context_budget.ts prunes tool results (12k each, 60k total), assistant tool-call inputs (12k/40k) and assistant prose (20k) inside prepareStep, so the payload the provider actually receives is bounded on every step - new subagents/review_diff_budget.ts keeps whole files in the reviewer's diff and names the files that did not fit, instead of silently dropping the middle of the diff - new tools/tool_output_limits.ts caps SQL results, schema dumps, type check output, logs, file listings, project info and MCP type-def blocks at the source - bound the root agent's chat history, its retry replay, and the tool-call inputs it carries - per-persona sub-agent tool allowlists and persona-aware tool rules, so a persona is only told about the tools it actually has - bound the reports and summaries handed back to the parent agent
Contributor
There was a problem hiding this comment.
31 issues found across 25 files (changes from recent commits).
Confidence score: 2/5
- In
local_agent_handler.ts, a single user message over 200,000 characters can be dropped entirely, leaving the model without the latest request. Truncate the message instead of removing it. - In
local_agent_prompt.ts, truncating the serialized blueprint can cut off trailing fields likevisuals, and an update can overwrite the stored plan with that incomplete document. Preserve valid JSON and avoid saving an incomplete blueprint. - In
retry_replay_utils.ts, pruning the replay copy also prunes data persisted inaiMessagesJson, so later turns permanently lose tool output, inputs, or assistant text. Keep pruning limited to model context and leave the persisted messages intact. - In
subagent_manager.ts, pruning history without bounding assignment text still lets large implementer assignments exceed provider context. Apply the assignment bound and reserve budget for history.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/pro/main/ipc/handlers/local_agent/retry_replay_utils.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/retry_replay_utils.ts:268">
P2: Do not prune the copy appended to `accumulatedAiMessagesRef`; it is persisted to `aiMessagesJson` and reused on later turns, so large retry replays lose tool output, inputs, or assistant text permanently. Keep `pruneRootMessages` only on the live `currentMessageHistory` copy.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.test.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.test.ts:130">
P2: This test does not exercise the unknown-shape branch it claims to cover. Keep the `undefined` case and add an unsupported object such as `{ type: "unknown" }` so the switch default is verified.</violation>
<violation number="2" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.test.ts:487">
P2: The root umbrella test never verifies the assistant-prose budget despite creating oversized prose. Add an older assistant text message and assert that it is replaced with the omission marker, while retaining the newest message.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/subagents/subagent_manager.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_manager.ts:43">
P2: The new assignment-budget helpers are imported but never invoked, so the declared assignment limits do not affect sub-agent requests. Apply the helper when constructing the sub-agent history, or remove the unused budget API.</violation>
<violation number="2" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_manager.ts:84">
P2: This lower cap can remove the actual Explorer follow-up instruction: a prior report over 40k plus queued root messages is sliced from the front, before `currentAssignment` is reached. Bound each section or preserve the current assignment and queued messages explicitly.</violation>
<violation number="3" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_manager.ts:1723">
P1: Large reviews can omit changed files that this reviewer cannot re-read. Give the reviewer read/diff tools or force an explicit partial result whenever `boundedDiff.omittedFiles` is nonempty.</violation>
<violation number="4" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_manager.ts:1895">
P1: This new pruning call does not bound user/assignment text, so large implementer assignments can still exceed the provider context despite the added budget. Apply the imported assignment bound and reserve the history budget before sending these messages.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/list_files.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/list_files.ts:190">
P2: This character-level bound can split the final head entry in half, but `candidatesFromListFilesResult` treats that partial bullet as a valid file path. Large listings can therefore register a bogus candidate and cause the subagent to attempt reading a nonexistent path; truncate file listings on entry/newline boundaries or discard partial boundary lines.</violation>
</file>
<file name="src/prompts/local_agent_prompt.ts">
<violation number="1" location="src/prompts/local_agent_prompt.ts:558">
P1: This truncates the JSON document itself, so larger blueprints reach the model with fields—typically the trailing `visuals` array—missing or cut off. An existing-blueprint update then overwrites the stored plan through `write_app_blueprint`, which can discard those visuals and their card edits; preserve structured fields (or update only requested fields) instead of slicing the serialized JSON.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/subagents/review_diff_budget.test.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/subagents/review_diff_budget.test.ts:32">
P2: This test does not enforce the budget invariant, so it passes while the review assignment exceeds the requested limit. Assert `bounded.text.length` is at most `first.length + 10`; that exposes the omitted-note overflow for the implementation to fix.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/execute_sql.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/execute_sql.ts:257">
P2: The Neon branch still awaits an unabortable database read, so cancelling a turn can leave `execute_sql` pending indefinitely while the Neon request hangs; add signal support to `executeNeonSql` or race this call against `ctx.abortSignal` as the Supabase branch does.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/update_todos.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/update_todos.ts:10">
P2: `.max(500)` constrains only newly supplied content; existing persisted todos created before this limit can still carry arbitrarily long content through status-only merges and into model-facing prompts. Bound or sanitize loaded/current todo content as well if this limit is intended to prevent context bloat.</violation>
<violation number="2" location="src/pro/main/ipc/handlers/local_agent/tools/update_todos.ts:27">
P2: `.max(50)` limits one tool-call payload but not the merged todo state, so repeated valid `merge=true` calls can still grow `ctx.todos` and the model-facing summaries without bound. Enforce the maximum on the resulting list (and handle over-limit preexisting state) after merging.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/get_database_table_schema.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/get_database_table_schema.ts:70">
P2: This bounds only the tool result returned to the model, while the same unbounded schema is still committed to the assistant message. Apply the bound before `onXmlComplete` as well, or otherwise ensure the persisted presentation uses the bounded value, so oversized schema responses cannot bypass this limit.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/write_app_blueprint.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/write_app_blueprint.ts:74">
P2: These caps do not bound the approved blueprint sent back to the model: renderer edits and added visuals accept arbitrary-length strings, then `handleApprove` interpolates them into `followUpPrompt`. Apply the same limits at the shared IPC schemas/handlers so users cannot bypass the context bound by editing the blueprint.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/subagent_tools.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/subagent_tools.ts:431">
P2: `wait_agents` can still inject a large `latestActivity` into the parent context. `waitForSubagents` appends the latest activity after completion, but `boundSubagentSummariesForParent` only bounds the report, assignment, and error; bound or omit that activity before serializing.</violation>
</file>
<file name="src/ipc/utils/preview_browser_driver.ts">
<violation number="1" location="src/ipc/utils/preview_browser_driver.ts:67">
P2: Cached sessions bypass the running-preview check, so stopping an app leaves stale browser state and makes relative navigation throw on the null base URL. Revalidate the preview URL and close or discard the session when the app stops.</violation>
<violation number="2" location="src/ipc/utils/preview_browser_driver.ts:75">
P2: The browser-installation error is swallowed and reported as “the app is not currently running,” which sends users to debug the wrong problem. Preserve or return the launch error so a running app with an unavailable browser gets the actual remediation.</violation>
<violation number="3" location="src/ipc/utils/preview_browser_driver.ts:90">
P1: The session opens on `about:blank` because `launchSession` never navigates to its `baseUrl`. Navigate the page when creating the session so read, click, type, console, and screenshot work on the running preview without requiring a separate navigation call.</violation>
<violation number="4" location="src/ipc/utils/preview_browser_driver.ts:99">
P2: `pageerror` entries bypass the 200-entry bound, allowing repeated page errors to grow every session indefinitely. Apply the same trimming after recording page errors, preferably through one shared append helper.</violation>
<violation number="5" location="src/ipc/utils/preview_browser_driver.ts:132">
P1: This driver is never wired into an agent tool or IPC handler, so none of the added preview operations can run in the product. Register the operations and their lifecycle hooks, or remove this unused module.</violation>
<violation number="6" location="src/ipc/utils/preview_browser_driver.ts:210">
P1: `previewScreenshot` labels UTF-8-decoded PNG bytes as base64, producing an invalid data URL. Keep the screenshot as bytes and call `screenshot.toString("base64")` before adding the `data:image/png` prefix.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/tool_output_limits.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/tool_output_limits.ts:39">
P3: `MAX_ROOT_HISTORY_CHARS` is dead and duplicates the active setting in `subagent_context_budget.ts`. Remove the duplicate or make both consumers import one shared value so future limit changes cannot silently diverge.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts:460">
P1: `boundChatHistoryForModel` can delete the entire latest user request when that single message exceeds 200,000 characters. Preserve the latest user message by truncating it or applying the cap without dropping it.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/subagents/review_diff_budget.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/subagents/review_diff_budget.ts:138">
P2: `boundReviewDiff` can return more than `maxChars`: the selection counts only section lengths, while `kept.join("\n")` adds separators and `omittedNote` is appended afterward. Reserve space for separators and the omission note, or bound the final text, so the 30,000-character context limit actually holds.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.ts:182">
P2: Truncating or omitting an error tool result changes it into a successful text result, so the model can treat a failed operation as successful. Preserve the error/denial output variant while shortening its payload.</violation>
<violation number="2" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.ts:263">
P2: The total tool-result budget is not a hard cap because omitted results add unbudgeted markers. Include replacement-marker lengths in the allocation or otherwise account for them before claiming the aggregate limit.</violation>
<violation number="3" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.ts:354">
P2: The newest assistant text can exceed the entire 20,000-character budget because `isNewest` bypasses the size check. Truncate that part too, otherwise one large model response still dominates every subsequent request.</violation>
<violation number="4" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.ts:781">
P2: These assignment limits are never applied to a sub-agent request. Wire the helper into `buildModelHistory` before constructing the transcript, or large review diffs and assignments will still be resent on every step.</violation>
<violation number="5" location="src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.ts:868">
P2: The history cap drops messages independently and can split an assistant tool-call from its tool result. Trim complete tool exchanges together so the bounded transcript remains valid for providers and does not leave unmatched context.</violation>
</file>
<file name="src/pro/main/ipc/handlers/local_agent/tools/mcp_type_defs.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/mcp_type_defs.ts:399">
P1: The MCP size estimator now measures the already-capped block, so it can never reach the 8,000-token search threshold. Measure the unbounded declarations for mode selection, then apply the output cap only when rendering the inline block.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| // tool results so a single large read/grep/diff cannot dominate the request | ||
| // and so the accumulated transcript stays proportional to the budget instead | ||
| // of growing quadratically with the step count. | ||
| return pruneSubagentMessages(sanitized); |
Contributor
There was a problem hiding this comment.
P1: This new pruning call does not bound user/assignment text, so large implementer assignments can still exceed the provider context despite the added budget. Apply the imported assignment bound and reserve the history budget before sending these messages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/pro/main/ipc/handlers/local_agent/subagents/subagent_manager.ts, line 1895:
<comment>This new pruning call does not bound user/assignment text, so large implementer assignments can still exceed the provider context despite the added budget. Apply the imported assignment bound and reserve the history budget before sending these messages.</comment>
<file context>
@@ -1880,6 +1888,11 @@ export function prepareSubagentStepMessages(
+ // tool results so a single large read/grep/diff cannot dominate the request
+ // and so the accumulated transcript stays proportional to the budget instead
+ // of growing quadratically with the step count.
+ return pruneSubagentMessages(sanitized);
}
</file context>
| try { | ||
| if (!(await updateStatus(threadId, "running"))) return; | ||
| const boundedDiff = boundReviewDiff(target.diff); | ||
| const diffExcerpt = boundedDiff.text; |
Contributor
There was a problem hiding this comment.
P1: Large reviews can omit changed files that this reviewer cannot re-read. Give the reviewer read/diff tools or force an explicit partial result whenever boundedDiff.omittedFiles is nonempty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/pro/main/ipc/handlers/local_agent/subagents/subagent_manager.ts, line 1723:
<comment>Large reviews can omit changed files that this reviewer cannot re-read. Give the reviewer read/diff tools or force an explicit partial result whenever `boundedDiff.omittedFiles` is nonempty.</comment>
<file context>
@@ -1713,12 +1719,14 @@ async function runReview(
try {
if (!(await updateStatus(threadId, "running"))) return;
+ const boundedDiff = boundReviewDiff(target.diff);
+ const diffExcerpt = boundedDiff.text;
const reviewResult = await runModel({
threadId,
</file context>
| "[[SERVER_LAYER]]", | ||
| "\\u005b\\u005bSERVER_LAYER\\u005d\\u005d", | ||
| ) | ||
| ? boundBlueprintPromptText( |
Contributor
There was a problem hiding this comment.
P1: This truncates the JSON document itself, so larger blueprints reach the model with fields—typically the trailing visuals array—missing or cut off. An existing-blueprint update then overwrites the stored plan through write_app_blueprint, which can discard those visuals and their card edits; preserve structured fields (or update only requested fields) instead of slicing the serialized JSON.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/prompts/local_agent_prompt.ts, line 558:
<comment>This truncates the JSON document itself, so larger blueprints reach the model with fields—typically the trailing `visuals` array—missing or cut off. An existing-blueprint update then overwrites the stored plan through `write_app_blueprint`, which can discard those visuals and their card edits; preserve structured fields (or update only requested fields) instead of slicing the serialized JSON.</comment>
<file context>
@@ -539,14 +555,16 @@ Do not call \`write_app_blueprint\` and do not start implementation. Explain tha
- "[[SERVER_LAYER]]",
- "\\u005b\\u005bSERVER_LAYER\\u005d\\u005d",
- )
+ ? boundBlueprintPromptText(
+ JSON.stringify(appBlueprint, null, 2)
+ .replaceAll("<", "\\u003c")
</file context>
| return { session, baseUrl: getRunningTestBaseUrl(appId)! }; | ||
| } | ||
|
|
||
| export async function previewNavigate(appId: number, path: string) { |
Contributor
There was a problem hiding this comment.
P1: This driver is never wired into an agent tool or IPC handler, so none of the added preview operations can run in the product. Register the operations and their lifecycle hooks, or remove this unused module.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/ipc/utils/preview_browser_driver.ts, line 132:
<comment>This driver is never wired into an agent tool or IPC handler, so none of the added preview operations can run in the product. Register the operations and their lifecycle hooks, or remove this unused module.</comment>
<file context>
@@ -0,0 +1,234 @@
+ return { session, baseUrl: getRunningTestBaseUrl(appId)! };
+}
+
+export async function previewNavigate(appId: number, path: string) {
+ const resolved = await currentPage(appId);
+ if (!resolved) return NO_PREVIEW_MESSAGE;
</file context>
| } | ||
|
|
||
| return history; | ||
| return boundChatHistoryForModel(history); |
Contributor
There was a problem hiding this comment.
P1: boundChatHistoryForModel can delete the entire latest user request when that single message exceeds 200,000 characters. Preserve the latest user message by truncating it or applying the cap without dropping it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts, line 460:
<comment>`boundChatHistoryForModel` can delete the entire latest user request when that single message exceeds 200,000 characters. Preserve the latest user message by truncating it or applying the cap without dropping it.</comment>
<file context>
@@ -451,7 +457,7 @@ export function buildChatMessageHistory(
}
- return history;
+ return boundChatHistoryForModel(history);
}
</file context>
| total -= measureMessageChars(message); | ||
| } | ||
|
|
||
| return history.filter((_, index) => keep[index]); |
Contributor
There was a problem hiding this comment.
P2: The history cap drops messages independently and can split an assistant tool-call from its tool result. Trim complete tool exchanges together so the bounded transcript remains valid for providers and does not leave unmatched context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/pro/main/ipc/handlers/local_agent/subagents/subagent_context_budget.ts, line 868:
<comment>The history cap drops messages independently and can split an assistant tool-call from its tool result. Trim complete tool exchanges together so the bounded transcript remains valid for providers and does not leave unmatched context.</comment>
<file context>
@@ -0,0 +1,869 @@
+ total -= measureMessageChars(message);
+ }
+
+ return history.filter((_, index) => keep[index]);
+}
</file context>
| const baseUrl = getRunningTestBaseUrl(appId); | ||
| if (!baseUrl) { | ||
| logger.debug("No running preview proxy for app", appId); | ||
| return null; |
Contributor
There was a problem hiding this comment.
P2: The browser-installation error is swallowed and reported as “the app is not currently running,” which sends users to debug the wrong problem. Preserve or return the launch error so a running app with an unavailable browser gets the actual remediation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/ipc/utils/preview_browser_driver.ts, line 75:
<comment>The browser-installation error is swallowed and reported as “the app is not currently running,” which sends users to debug the wrong problem. Preserve or return the launch error so a running app with an unavailable browser gets the actual remediation.</comment>
<file context>
@@ -0,0 +1,234 @@
+ const baseUrl = getRunningTestBaseUrl(appId);
+ if (!baseUrl) {
+ logger.debug("No running preview proxy for app", appId);
+ return null;
+ }
+
</file context>
Suggested change
| return null; | |
| throw error; |
| if (consoleLog.length > 200) consoleLog.splice(0, consoleLog.length - 200); | ||
| }); | ||
| page.on("pageerror", (err: Error) => { | ||
| consoleLog.push({ type: "pageerror", text: err.message }); |
Contributor
There was a problem hiding this comment.
P2: pageerror entries bypass the 200-entry bound, allowing repeated page errors to grow every session indefinitely. Apply the same trimming after recording page errors, preferably through one shared append helper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/ipc/utils/preview_browser_driver.ts, line 99:
<comment>`pageerror` entries bypass the 200-entry bound, allowing repeated page errors to grow every session indefinitely. Apply the same trimming after recording page errors, preferably through one shared append helper.</comment>
<file context>
@@ -0,0 +1,234 @@
+ if (consoleLog.length > 200) consoleLog.splice(0, consoleLog.length - 200);
+ });
+ page.on("pageerror", (err: Error) => {
+ consoleLog.push({ type: "pageerror", text: err.message });
+ });
+
</file context>
Suggested change
| consoleLog.push({ type: "pageerror", text: err.message }); | |
| consoleLog.push({ type: "pageerror", text: err.message }); | |
| if (consoleLog.length > 200) consoleLog.splice(0, consoleLog.length - 200); |
|
|
||
| async function getSession(appId: number): Promise<Session | null> { | ||
| const existing = sessions.get(appId); | ||
| if (existing) return existing; |
Contributor
There was a problem hiding this comment.
P2: Cached sessions bypass the running-preview check, so stopping an app leaves stale browser state and makes relative navigation throw on the null base URL. Revalidate the preview URL and close or discard the session when the app stops.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/ipc/utils/preview_browser_driver.ts, line 67:
<comment>Cached sessions bypass the running-preview check, so stopping an app leaves stale browser state and makes relative navigation throw on the null base URL. Revalidate the preview URL and close or discard the session when the app stops.</comment>
<file context>
@@ -0,0 +1,234 @@
+
+async function getSession(appId: number): Promise<Session | null> {
+ const existing = sessions.get(appId);
+ if (existing) return existing;
+
+ const inFlight = launching.get(appId);
</file context>
| * applied to the history that is actually sent, so a long-running chat cannot | ||
| * ship an unbounded transcript on every step. | ||
| */ | ||
| export const MAX_ROOT_HISTORY_CHARS = 200_000; |
Contributor
There was a problem hiding this comment.
P3: MAX_ROOT_HISTORY_CHARS is dead and duplicates the active setting in subagent_context_budget.ts. Remove the duplicate or make both consumers import one shared value so future limit changes cannot silently diverge.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/pro/main/ipc/handlers/local_agent/tools/tool_output_limits.ts, line 39:
<comment>`MAX_ROOT_HISTORY_CHARS` is dead and duplicates the active setting in `subagent_context_budget.ts`. Remove the duplicate or make both consumers import one shared value so future limit changes cannot silently diverge.</comment>
<file context>
@@ -0,0 +1,75 @@
+ * applied to the history that is actually sent, so a long-running chat cannot
+ * ship an unbounded transcript on every step.
+ */
+export const MAX_ROOT_HISTORY_CHARS = 200_000;
+
+/**
</file context>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a76ef5f85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const entitlementWatcher = watchEntitlement(threadId, controller); | ||
| try { | ||
| if (!(await updateStatus(threadId, "running"))) return; | ||
| const boundedDiff = boundReviewDiff(target.diff); |
There was a problem hiding this comment.
When target.diff exceeds 30,000 characters, this replaces it with an excerpt that can omit entire files, but runModel still receives tools: {}, so the generated note's suggestion to use read_file or git_diff is impossible. The parsed result can still be marked completed with no findings and release auto-review even though those files were never inspected; either provide read tools/the full diff or force a partial outcome whenever omittedFiles is nonempty.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| return sections.join("\n"); | ||
| return boundToolOutput(sections.join("\n"), MAX_MCP_TYPE_DEFS_BLOCK_CHARS); |
There was a problem hiding this comment.
With the default constants, this cap makes buildMcpTypeDefsBlock at most 12,000 characters, so estimateMcpInlineTokens() can never exceed 3,000 while MCP_INLINE_TOKEN_THRESHOLD is 8,000. The search-mode predicates in tool_definitions.ts and local_agent_handler.ts are therefore always false under production defaults even for arbitrarily large catalogs; the model receives a truncated inline block and no discovery tools, making omitted MCP tools unusable. Estimate from the unbounded schemas and cap only after choosing inline versus search mode.
Useful? React with 👍 / 👎.
| if (message.role === "system") { | ||
| continue; | ||
| } | ||
| keep[index] = false; |
There was a problem hiding this comment.
ChatStreamParamsSchema accepts prompts up to 1 MiB, but once the active message itself exceeds 200,000 characters, this oldest-first loop eventually marks that final user message false, so buildChatMessageHistory invokes the model without the submitted request. The same loop can discard earlier user messages containing inline image or PDF parts as textual history grows; preserve the active turn and attachment-bearing messages, truncating or compacting content rather than deleting them. rules/local-agent-tools.mdL297-L297
Useful? React with 👍 / 👎.
| const MAX_ACTIVITY_XML_CHARS = 200_000; | ||
| const MAX_ACTIVITY_OUTPUT_CHARS = 200_000; | ||
| const MAX_MODEL_HISTORY_CHARS = 100_000; | ||
| const MAX_MODEL_HISTORY_CHARS = 40_000; |
There was a problem hiding this comment.
After this cap was reduced to 40,000 characters, buildExplorerFollowupAssignment still concatenates the original assignment and prior report before the queued root/current instruction and then head-slices the result. A valid prior report can be up to 100,000 characters, so a follow-up after a long report sends only stale context, and the successful run then marks the unseen queued message consumed; reserve space for the current/root tail or bound the older sections first. rules/local-agent-tools.mdL27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
An allowed blueprint can exceed 8,000 characters through the user prompt and visual entries, and this cuts its serialized JSON at an arbitrary character, removing later fields and visuals. On an edit turn the prompt tells the model to preserve every field, but write_app_blueprint replaces the stored object rather than merging it, so an update based on this excerpt can silently delete user-edited visuals or tail fields; provide a structured bounded representation or merge omitted fields server-side.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gateway allowlist rejections (e.g. OpenRouter 'is only available on agentic harnesses') now map to fallback-next so one restricted free model no longer ends the turn. abortSignal is wired into the wrapped provider read and cancel() so cancelled turns end promptly instead of stalling. Tests: policy case for the allowlist message, abort-ends-hanging-read case. Verified: 36/36 vitest in fallback_ai_model.test.ts, oxlint exit 0, oxfmt clean, npm run ts clean outside pre-existing e2e/playwright duplicate-install errors.