Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Collaborator
Author
|
CodeQL status after 6c8ea70: the medium alert, the mock echoing error text, is fixed. 11 alerts remain (1 critical, 10 high), all in
The threads give the path traced for each. They stay open until the author decides between two options:
Every other check on this head is for the author to judge once it completes. Generated by Claude Code |
![mergestorm-vortex[bot]](https://avatars.githubusercontent.com/in/3924858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review progress ████░░░░░░ 54/145 files
Comment — found 6 issue(s) at 6c8ea70.
Actionable comments posted: 6
Coverage is incomplete: reviewed 54/145 files; 2 skipped; 89 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 257, Important):
`node ... 2>&1 </dev/null | tee <file>` has no `set -o pipefail`, so the command's exit status is `tee`'s (0). If the agent exits non-zero (node missing from PATH, uncaught exception, bundle load failure), Harbor sees a successful run, so no `NonZeroAgentExitCodeError` is recorded. The trial then goes to the verifier and, if no summary was written, `classifyTrial` labels it `wrong-answer` instead of `agent-crash`. Fix by prefixing the command with `set -o pipefail;` (or `set -euo pipefail;`, as `install` does), so the agent's status propagates.
2. In `@examples/agent-eval/src/agent/tools/read.ts` (line 81, Important):
`const buffer = await readFile(path);` reads the whole file, then sniffs for binary and splits it into lines, even when the model asked for a 2000-line window. Reading a multi-hundred-MB log or dataset in a benchmark container costs that much RSS and time for a response capped at 50KB. Files over the Node buffer limit fail with a generic "read failed" error instead of a size message. A `stat` size check, or reading only the first few KB for the binary sniff and the requested line range, would bound this. Non-blocking.
3. In `@examples/agent-eval/src/run/runHarbor.ts` (line 46, Important):
The precheck tests `existsSync(bundle)` where `bundle` is always `<root>/dist/workglow-agent.mjs`. But the env it passes to Harbor uses `process.env.WORKGLOW_AGENT_BUNDLE ?? bundle` (line 60), and the Python `_bundle()` honours that env var too. If a user points WORKGLOW_AGENT_BUNDLE at a bundle elsewhere and has no dist build, this throws "run `bun run build-agent` first" even though the run would succeed. Check the same resolved path that is later exported (`process.env.WORKGLOW_AGENT_BUNDLE ?? bundle`).
4. In `@examples/cli/src/run-events/projectRunEvents.ts` (line 125, Important):
Previously every non-empty text-delta was emitted as a `text` event regardless of port. classifyTextDelta (and foldTextDelta in TaskRunApp/ChatTaskRow/StreamingTextRow, and the `port === "text"` check in web TaskNode) now returns nothing for any port other than `text` or `reasoning`. StreamProcessor accumulates append-mode output per port, so a task whose streamed output lives on a port such as `code` or `summary` now shows an empty panel and no scrollback in the CLI and web console, with no error. The unit test `ignores ports other than text and reasoning` pins this on purpose, so it is a deliberate choice, but it is a visible regression for non-chat streaming tasks. Either fall back to text for any port other than `reasoning`, or confirm such tasks are out of scope.
5. In `@packages/ai/src/task/ChatHistory.ts` (line 74, Important):
`messageChars` drops `payload` before measuring, so `trimHistoryForModel` never counts provider-native reasoning. But `normalizeHistoryForModel(trimHistoryForModel(...))` is what gets sent, and the converters replay each earlier turn's payload verbatim (Anthropic `parseOwnTurn`, OpenAI Responses `native_items`) whenever the provider and call ids still match. The request therefore grows by the payload size on every earlier turn without being counted, so `maxHistoryChars` no longer caps what reaches the model. The docstring says it caps history at `max` characters. A long multi-round turn with large encrypted reasoning can exceed the window the budget is meant to protect. If the exclusion is intentional, the trimming docs should say payloads are not capped.
6. In `@providers/openai/src/ai/common/OpenAI_ToolCalling.ts` (line 71, Important):
`replay ? { nativeTurnProvider: nativeKey } : {}` gates replay only on `replay_reasoning`. The request only gets `include: ["reasoning.encrypted_content"]` when `wantsEncryptedReasoning` is true (line ~89: `replay && reasoning !== undefined && reasoning.effort !== "none"`). So when a run's config has reasoning off (`effort: "none"`, or a non-reasoning model) but the history carries an earlier `openai:<model>` native turn whose reasoning items hold `encrypted_content`, `toOpenAIMessages`/`buildResponsesInput` replays those reasoning items verbatim into a request that did not ask for `reasoning.encrypted_content`. The header comment of this function and the comment in `accumulateOpenAIResponsesStream` say a replayed reasoning item without that include is looked up by id and fails where the response is not stored. Example: a persisted chat history written with effort high is resumed with effort none under the same model_name. Fix: use the same `wantsEncryptedReasoning` condition for the replay option as for emit and include. The test "asks for nothing extra when reasoning is off" only checks that `include` is absent and does not assert on `input`, so it does not catch this.
There was a problem hiding this comment.
Review progress ████████░░ 109/145 files
Request changes — found 6 issue(s) at 5ab86f4.
Actionable comments posted: 6
Coverage is incomplete: reviewed 109/145 files; 0 skipped; 36 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/bin/workglow-agent.ts` (line 97, Blocker):
`Number` converts an invalid value such as `--max-round-retries nope` to `NaN` without rejecting it. AgentTask then evaluates `attempts > maxRoundRetries`; every comparison with `NaN` is false, so retryable provider failures keep retrying indefinitely (notably in direct runs without a duration budget). Validate this as a finite, non-negative integer before passing it to the task.
2. In `@examples/agent-eval/package.json` (line 25, Important):
`"workglow-agent-eval": "./dist/bin/workglow-agent-eval.js"`
The workspace entry in `bun.lock` still declares this bin as `./dist/workglow-agent-eval.js`, while `build-cli` emits it under `dist/bin/`. After installing the workspace, the generated `workglow-agent-eval` executable therefore targets a nonexistent file; update the lockfile bin path to match this manifest/build output.
3. In `@examples/agent-eval/src/agent/atif.ts` (line 118, Important):
This per-round total omits `usage.imageInput` and `usage.imageCached`, and the `cached_tokens` field below omits `imageCached` too. `summarizeUsage` includes both image-token counters in the trajectory's `final_metrics`, so image-using rounds (supported by this harness) report per-step metrics that do not reconcile with the totals.
4. In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 49, Important):
`AgentTask` appends `reviewPrompt` as a user message, then can start another model round if the model requests a tool. In that sequence this returns the review prompt as the entire query; because Needle has no conversation state, the next round no longer gets the original user request (and this query also omits the tool result). The agent can therefore attempt the review without knowing what task it was reviewing. Preserve the original request/context when forming subsequent-round queries.
5. In `@providers/huggingface-transformers/src/ai/common/HFT_ToolCalling.ts` (line 216, Important):
`reasoning_content` is only added by `buildHFTMessages`, but `buildPromptAndPrefix` uses that builder only when the history contains a `tool` message; otherwise it uses `toTextFlatMessages`, which drops assistant reasoning blocks. For example, when `AgentTask` gets a no-tool answer and appends `reviewPrompt`, the next HFT round receives the prior answer and review request but not the reasoning this code captured, so the advertised reasoning replay does not work for that follow-up path. Preserve reasoning in the no-tool history path as well.
6. In `@providers/ollama/src/ai/common/Ollama_JobRunFns.ts` (line 35, Important):
`toOllamaMessages` attaches result images to the `role: "tool"` message. When a tool returns a screenshot or other image, Ollama's next round does not receive it as vision input; tool-result images need to follow the tool message in a user message. This converter is also wired into the browser run-fn, so the same case fails there.
Collaborator
Author
|
The analysis itself ran. GitHub then refused the upload: "CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled." The new To fix (needs a repo admin): Settings → Code security → CodeQL analysis → switch to Advanced, or disable default setup. Then re-run this workflow; the No code change in this PR can clear it, so I haven't re-run the job. Generated by Claude Code |
There was a problem hiding this comment.
Review progress ██████████ 148/148 files
Comment — found 3 issue(s) at 64cb980.
Actionable comments posted: 3
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@.github/codeql/codeql-config.yml` (line 8, Important):
`examples/agent-eval` is matched against source-file paths as a glob, so this literal directory path does not match files beneath it. CodeQL will continue scanning the shell-capable harness and report the alerts this config is intended to suppress; use a recursive pattern such as `examples/agent-eval/**`.
2. In `@examples/agent-eval/src/bin/workglow-agent.ts` (line 212, Important):
`process.exit()` terminates immediately, without waiting for pending `process.stdout.write()` / `process.stderr.write()` data. Harbor runs this agent with output piped through `tee`; when the final answer or accumulated log output exceeds the pipe's writable buffer, the write is queued and this line can truncate it. Set `process.exitCode` and allow the event loop to drain (or explicitly wait for the streams) before exiting.
3. In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 53, Important):
`users[0]` is the first user message in the entire `input.messages` history, not the first message of this AgentTask turn. In `runAgentChat`, a second independent request is passed alongside the prior transcript, so Cactus receives `old request\n\nnew request` and can answer or call tools for the stale request. Use the current `input.prompt` as the active turn’s first question while retaining a later review/reminder user message when present.
There was a problem hiding this comment.
Review progress ██████████ 148/148 files
Comment — found 1 issue(s) at 82ea1c4.
Actionable comment posted: 1
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/cactus/src/ai/common/Cactus_ToolCallingCore.ts` (line 60, Important):
`AgentTask` appends the model's assistant answer before adding `reviewPrompt`, but this query includes only the original prompt and latest user message. Since Cactus passes only this string to stateless Needle, the review round never receives the answer (or tool results) it is supposed to check and instead generates a fresh response to the question. Include the prior assistant/tool context needed for the review in the query.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 22:35
82ea1c4 to
ec97452
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/148 files
Request changes — found 7 issue(s) at ec97452.
Actionable comments posted: 7
Coverage is incomplete: reviewed 59/148 files; 1 skipped; 88 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
3. In `@examples/agent-eval/harbor/workglow_agent.py` (line 165, Important):
`return min(base, config.agent.max_timeout_sec or float("inf")) * multiplier`
This applies `agent.max_timeout_sec` before scaling, so the computed allowance can exceed Harbor's maximum. For example, with `base=600`, `max_timeout_sec=300`, and `multiplier=2`, this reports 600 seconds; `_max_duration_sec()` then gives the agent 480 seconds, although Harbor will cancel at 300 seconds. That can terminate a still-running trial before the agent writes its summary. Apply the multiplier before capping the resolved timeout.
4. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 117, Important):
`Number` accepts values such as `nope` as `NaN`. `buildHarborJob` includes that value, and `runHarbor`'s `JSON.stringify` serializes it as `null`, so Harbor receives a changed timeout setting instead of a clear CLI error. Validate that the value is finite and in the supported range before building the job.
5. In `@examples/agent-eval/src/compare/report.ts` (line 210, Important):
With multiple attempts per task (for example, `-k 2`), equal nonzero pass fractions reach this branch. If both arms pass 1 of 2 attempts, the report counts the task as “Both ✓,” even though neither arm passed every attempt. Distinguish partial-rate ties from tasks both arms fully passed.
6. In `@packages/ai/src/task/MessageConversion.ts` (line 208, Important):
`OpenAI_ToolCalling_Stream` calls `toOpenAIMessages` with only `nativeTurnProvider`, so this option stays false for OpenAI. Tool-result images are then left inline in the `tool` message, and `buildResponsesInput` converts that content with `contentToString`, which drops every image part. When an OpenAI tool returns an image (for example, a screenshot), the next model round never receives it; enable the user-message image path for the Responses caller.
7. In `@providers/openrouter/src/ai/common/OpenRouter_ToolCalling.ts` (line 93, Important):
`filterValidToolCalls` can remove an unknown-name call while retaining another call from the same assistant response. This set then records only the surviving IDs, so `encodeReasoningTurn` makes the response's reasoning details look bound to that shortened call list; on the next round `withReasoningDetails` accepts and replays them beside the filtered transcript. For signed reasoning (for example, Anthropic details through OpenRouter), the details were produced with the original call set and may be rejected or replayed against the wrong turn. Suppress native-turn replay whenever any response call was filtered, rather than treating only forwarded IDs as the original turn's IDs.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 22:46
ec97452 to
5315c52
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/148 files
Request changes — found 9 issue(s) at 5315c52.
Actionable comments posted: 9
Coverage is incomplete: reviewed 60/148 files; 0 skipped; 88 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/runCodingAgent.ts` (line 198, Important):
`finalText = steps.at(-1)?.text ?? ""`
When the last allowed round (for example, `--max-rounds 1`) returns text alongside a tool call, AgentTask can stop as `max-rounds` after running that tool without ever receiving a final answer. This still records the narration from the tool-call round as `finalText`, and the CLI prints it on stdout as if it were the answer. Only expose text from a terminal no-tool round, or otherwise avoid presenting an incomplete run's narration as the final answer.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 45, Important):
`spawn()` runs before the abort listener is installed below. If the task signal is already aborted when `runCommand` is entered, `addEventListener` will not replay the abort event, so the shell still runs until its timeout and can perform side effects after cancellation. This can occur when cancellation happens while AgentTask is awaiting progress before invoking the tool; reject immediately or handle the already-aborted state before starting the process.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 55, Important):
Once captured output reaches 16 MiB, this drops all later chunks while retaining only the prefix. `createBashTool` then calls `truncateTail` on that prefix, so for a command producing more than 16 MiB the model sees the end of the first 16 MiB rather than the command’s final output (where test failures and summaries commonly appear), despite the tool promising to show the tail.
4. In `@examples/agent-eval/src/agent/tools/read.ts` (line 98, Important):
`split("\n")` leaves a trailing empty element for the ordinary one-line file `hello\n`. Consequently `offset: 2` returns an empty result instead of reporting that the requested line is past EOF, and page counts/continuation offsets are off by one for newline-terminated files.
5. In `@examples/agent-eval/src/agent/tools/read.ts` (line 109, Important):
This slices by UTF-16 characters rather than UTF-8 bytes. A 50 KiB line of four-byte characters can therefore return about 200 KiB here, bypassing `MAX_OUTPUT_BYTES` and sending several times the intended tool output to the model. Bound this fallback by encoded byte length while preserving character boundaries.
6. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 209, Important):
`buildReport(loadTrials([jobDir]))`
If Harbor creates `jobDir` but produces no parseable trial results (for example, it fails before scheduling trials), this builds and writes an empty comparison and the CLI exits successfully. The `compare` command already rejects this case; apply the same check here so a failed/empty run is not reported as a successful benchmark.
7. In `@packages/ai/src/task/AgentTask.ts` (line 838, Important):
`AgentToolRecord.chars` is calculated in `runCalls` before this notice is appended, while the updated result is what the model receives. With `announceTimeLeft`, each round's final tool record therefore undercounts the visible result by the notice length, making `steps[].tools[].chars` disagree with the transcript.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 22:55
5315c52 to
abd0f4f
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 61/148 files
Request changes — found 6 issue(s) at abd0f4f.
Actionable comments posted: 6
Coverage is incomplete: reviewed 60/148 files; 1 skipped; 87 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/read.ts` (line 91, Important):
`return `${path} is a directory:\n${names.join("\n")}``
Directory results bypass `truncateHead`, so a directory with thousands of entries returns more than the documented 2000 lines/50 KB. The later AgentTask clamp is character-based and configurable, so it does not enforce either limit; large listings can consume excessive context. Apply the same output bounds to directory listings.
2. In `@packages/ai/src/task/AgentToolExecution.ts` (line 273, Important):
`approveToolCall` is awaited before this invocation, but the deadline is not checked again afterward. If a human approves a gated call only after `maxDurationMs` has elapsed, the tool still starts; task-backed tools also receive no deadline in `invokeTool`, so they cannot honor the expired deadline themselves. This lets a budget-expired turn perform the approved side effect after the deadline.
3. In `@providers/llamacpp-server/src/ai/common/LlamaCppServer_ToolCalling.ts` (line 72, Important):
`toOpenAIMessages(input, ...)` represents text from `input.messages` as content-part arrays, but the estimator below only counts messages whose `content` is a string. `AgentTask` supplies its transcript in `input.messages`, so the user prompt is omitted from the in-flight input-token snapshots (often showing 0 until terminal server usage arrives, or remaining wrong if the server omits usage). Flatten text parts when building the prompt estimate.
4. In `@providers/ollama/src/ai/common/Ollama_TextGeneration.ts` (line 84, Important):
`toOllamaChatMessages(...)` is used by the text-generation run function, but its subsequent `client.chat` request does not include `ollamaThinkParam(model)` (unlike the tool-calling request). A model configured with `effort: "high"` can therefore be used by `TextGenerationTask` or `AiChatTask` while Ollama receives no `think` setting, silently ignoring the effort policy exposed by the provider.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:03
abd0f4f to
7995382
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/149 files
Request changes — found 6 issue(s) at 7995382.
Actionable comments posted: 6
Coverage is incomplete: reviewed 60/149 files; 0 skipped; 89 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 257, Important):
`writeFile(path, out, "utf8")`
When `toolConcurrency` is greater than 1, two `edit` calls targeting different regions of the same file can both read the original contents, compute separate results, and then overwrite the whole file; whichever write lands last silently discards the other edit, even though both calls report success. This is reachable through `runCodingAgent`'s `toolConcurrency` setting and `AgentTask` runs calls concurrently at that width.
2. In `@packages/ai/src/task/AgentTask.ts` (line 959, Important):
`normalizeHistoryForModel` merges adjacent user messages by extracting only their text blocks. Since AgentTask appends `prompt` after `input.messages`, a resumed conversation whose history ends in a user message will merge that message with the new prompt; if either contains an image, the image is dropped before the provider sees it. For example, a trailing user image in `messages` followed by a new text prompt becomes text-only, so the next model round cannot inspect the image.
3. In `@packages/ai/src/task/ToolCallingTask.ts` (line 346, Important):
When `emitCheckpoint` is set, `executeStream` captures only `text` and `toolCalls` before calling `finalizeCheckpoint`; that finalizer stores an assistant message without this `nativeTurn` payload (or reasoning). A later request consuming that checkpoint therefore replays an incomplete assistant turn. For providers that require signed/encrypted reasoning to be echoed back, such as Anthropic, a follow-up after a checkpointed tool call can be rejected because the required native turn was discarded.
4. In `@providers/huggingface-inference/src/ai/HfInferenceProvider.ts` (line 33, Important):
`effortPolicy` exposes the effort dial for this provider, but only `HFI_ToolCalling_Stream` currently adds `hfiReasoningParams(model)` to its request. The registered `HFI_TextGeneration_Stream` also handles `TextGenerationTask` and `AiChatTask` and omits that field, so selecting an effort on either of those common paths silently has no effect.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:12
7995382 to
c2882f8
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/152 files
Request changes — found 7 issue(s) at c2882f8.
Actionable comments posted: 7
Coverage is incomplete: reviewed 60/152 files; 0 skipped; 92 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 258, Important):
`crlf ? result.content.replace(/\n/g, "\r\n") : result.content`
If a file contains both CRLF and LF separators, any successful edit converts every LF separator to CRLF, including untouched sections. This contradicts the adjacent claim that the tool keeps the file's own line endings and can create a whole-file diff from a small edit; preserve the original separators rather than choosing one ending for the entire file.
2. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 207, Important):
`runHarbor` returns Harbor's nonzero exit code, but when at least one trial result exists this branch only prints a warning; the action then completes and the outer handler leaves `process.exitCode` at 0. Thus a failed or incomplete Harbor run can produce a partial comparison while shell scripts and CI treat the benchmark as successful. Keep writing the partial report if desired, but preserve the nonzero status.
3. In `@examples/agent-eval/src/compare/report.ts` (line 166, Important):
When a run has an unpriced passing trial, `costUsd` sums only priced trials while `passed` includes every pass. For example, one priced pass costing $1 plus one unpriced pass reports $0.50/pass, understating the cost; the Markdown table also presents this value without the asterisk used for the partial cost total. Avoid reporting this as an exact cost per pass when pass costs are incomplete.
4. In `@packages/ai/src/task/MessageConversion.ts` (line 243, Important):
`deferredImages` is flushed at the end of each individual `role: "tool"` message. If history represents parallel calls as separate tool messages and an earlier result contains an image, this inserts a user turn between the assistant's function calls and the later call's result (for example, `assistant(c1,c2) → tool(c1,image) → user(image) → tool(c2)`). OpenAI's Responses conversion preserves that order, so the model sees a new user turn before the outstanding tool result and the replayed tool-call exchange is malformed. Defer these images until the contiguous tool-result messages for that assistant turn have all been emitted.
5. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_EffortPolicy.ts` (line 16, Important):
`llamaCppEffortPolicy` advertises these effort levels for the whole provider, but `llamaCppThoughtBudget` is only consumed by `LlamaCpp_ToolCalling_Stream`. The registered `LlamaCpp_TextGeneration_Stream` and `LlamaCpp_Chat_Stream` both call `session.prompt` without a `budgets.thoughtTokens` option, so selecting an effort on those common paths is silently ignored despite the provider reporting it as supported.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:23
c2882f8 to
c3dec79
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 57/154 files
Request changes — found 4 issue(s) at c3dec79.
Actionable comments posted: 4
Coverage is incomplete: reviewed 57/154 files; 0 skipped; 97 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/anthropic/src/ai/common/Anthropic_CacheCheckpoint.ts` (line 143, Important):
Passing `undefined` makes `buildAnthropicMessages` discard every reasoning block, including Anthropic native payloads. A checkpoint produced by `ToolCallingTask.finalizeCheckpoint` can contain an assistant turn with signed thinking; when a consumer prepends this prefix, the signature is omitted and the prior turn is not replayed. Use the matching Anthropic model key for checkpoint-prefix conversion (the warm-up conversion at line 93 has the same omission).
2. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 64, Important):
`parseOwnTurn` accepts a payload whenever the tool-call IDs match, even if the current message has a different tool name or input. If a host edits an assistant history entry while preserving its call ID, this replays the old `tool_use` from `payload` and silently ignores the current transcript. Compare the complete tool-use blocks before replaying; otherwise rebuild the assistant message.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:30
c3dec79 to
1ca5533
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 59/155 files
Request changes — found 9 issue(s) at 1ca5533.
Actionable comments posted: 9
Coverage is incomplete: reviewed 58/155 files; 1 skipped; 96 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
2. In `@examples/agent-eval/harbor/workglow_agent.py` (line 203, Important):
`command -v node` skips installation whenever any Node binary is present, and the later `node --version` only prints its version. On a musl/Alpine task image with a preinstalled Node below the bundle's stated 22+ requirement, setup proceeds and the agent is then run on an unsupported runtime, causing those trials to fail. Verify the installed major version and install a supported runtime or fail setup clearly.
3. In `@examples/agent-eval/src/agent/tools/read.ts` (line 81, Important):
`optionalInteger` only checks that the value is finite and then truncates it; it does not enforce the schema’s `minimum: 1`, and AgentTask passes function-tool arguments directly to `execute`. A call with `offset: 0` therefore reaches `slice(-1)` and returns the file’s last line as though it were the requested page; `limit: 0` can return an empty page with a misleading continuation notice. Validate these bounds at runtime and return a tool error for invalid values.
4. In `@examples/agent-eval/src/compare/report.ts` (line 161, Important):
`ran.length > 0 ? passed / ran.length : 0`
When every trial for an arm is blocked, this reports a 0% pass rate; `wilsonInterval(0, 0)` also renders a 0–0% interval, and `passAt1` falls back to 0. These are missing measurements, not zero-success measurements, so an entirely blocked arm is presented and ranked like an arm that ran and failed every task.
5. In `@examples/agent-eval/src/compare/report.ts` (line 173, Important):
`sum(ran.map((trial) => trial.inputTokens))`
`sum` turns each missing token count into zero, but the report displays these values as complete totals with no indication that some trials lacked usage data. For example, if one trial reports 100 input tokens and another has `inputTokens === undefined`, the table reports 100 as the arm total, understating usage. Distinguish unavailable counts from measured zero or mark these totals as partial.
6. In `@examples/agent-eval/src/run/jobConfig.ts` (line 74, Important):
`spec.slice(colon + 1).split(",")`
This splits before parsing each value as JSON, so valid object/array kwargs such as `opencode_config={"agent":{"build":{"steps":20}}}` fail at the embedded commas; string kwargs such as `append_system_prompt=Use X, then Y` fail too. The parser documents JSON-valued kwargs, but this delimiter handling makes those values impossible to pass through `--arm`.
7. In `@providers/openrouter/src/ai/common/OpenRouter_ToolCalling.ts` (line 39, Important):
`mergeReasoningDetailDelta` only stores these details for the native-turn payload; `accumulateOpenAIChatStream` forwards only `delta.reasoning` and `delta.reasoning_content`. When an OpenRouter response provides readable thoughts only as `reasoning_details` (for example, a `reasoning.text` detail without either legacy field), the reasoning is saved for replay but never reaches `AgentTask`'s reasoning output port or UI. Forward the text-bearing details as reasoning as well, avoiding duplication when the legacy field is also present.
Additional findings outside changed lines:
1. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
2. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (2) — the lines they point at have not changed:
- 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:39
1ca5533 to
63a04e1
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/155 files
Request changes — found 14 issue(s) at 63a04e1.
Actionable comments posted: 14
Coverage is incomplete: reviewed 60/155 files; 0 skipped; 95 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 140, Important):
The timeout stays armed while `finish` waits up to `DRAIN_MS` for pipe output. If the shell exits just before the timeout while a background child holds the pipes (for example, `sleep 0.9; sleep 30 & echo done` with a 1-second timeout), the timer fires during that drain window, marks the successful command as timed out, and SIGTERMs its process group. Clear the timeout when the shell emits `exit`, while retaining the drain delay.
2. In `@examples/agent-eval/src/agent/tools/read.ts` (line 104, Important):
`readFile(path)` loads the entire path before any size or file-type check. Because `resolvePath` accepts absolute paths, a call on `/dev/zero` never reaches EOF and can grow the agent process until it runs out of memory; even an ordinary huge file bypasses the advertised 50 KB output bound during reading. Reject non-regular files and cap or stream the read before allocating the full contents.
3. In `@examples/agent-eval/src/agent/tools/truncate.ts` (line 79, Important):
`text.split("\n")` leaves an empty final element for ordinary newline-terminated output. For example, `seq 1 5000` is reported by `createBashTool` as 5001 lines, and the 2000-line tail includes that empty element, so it retains only 1999 actual output lines. This also makes the new `seq` test assert the inflated count; treat a final newline as a terminator, not another line.
4. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 117, Important):
`--effort low` (or `off`, etc.) still leaves this option at its default `high`; `parityKwargs` then sends OpenCode `variant: "high"` while Workglow and pi receive the requested level. The arms are no longer running at the same reasoning level, so the comparison is not measuring the advertised parity.
5. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 172, Important):
This replaces any `task_names` already set by the suite. For example, `run -s polyglot-lite --tasks polyglot_go_*` drops the suite's Python/JavaScript restriction and runs Go tasks instead, broadening a command described as narrowing the suite. Preserve the suite's allowed task set when applying this filter.
6. In `@examples/agent-eval/src/bin/workglow-agent-eval.ts` (line 173, Important):
This overwrites a dataset's built-in `n_tasks` cap even when the CLI value is larger. For example, `-s swebench-50 -l 100` changes the suite from its documented first 50 tasks to 100, despite `--n-tasks` being described as an upper bound.
7. In `@examples/agent-eval/src/compare/report.ts` (line 163, Important):
When every trial is blocked, this still stores `passRate: 0` (and the summary also stores a zero Wilson interval and `passAt1: 0`). `renderMarkdown` hides these values via `ranAny`, but the CLI also writes the `ArmSummary` directly to `comparison.json`, where consumers see an all-blocked arm as measured zero success rather than no measurement.
8. In `@examples/agent-eval/src/compare/report.ts` (line 176, Important):
A trial with input and output counts but `cacheTokens === undefined` is not counted as missing usage. Its cache count is then added as zero at line 179, and the Markdown table shows the cache total without the `†` marker. This presents an incomplete cache-token total as complete.
9. In `@packages/ai/src/provider-utils/OpenAIShapedResponses.ts` (line 152, Important):
`sameCallIds` only checks `call_id`, so if a host changes a tool call's name or arguments but preserves its ID, this branch still sends the old native `function_call` and skips the edited assistant transcript. The following `function_call_output` may then contain a result for the edited call while Responses sees the old call; compare the names and arguments too before replaying these items.
10. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 123, Important):
`parseOwnTurn` checks only the tool-use blocks, so a host can edit or remove an assistant text block while keeping the calls unchanged and this still replays the original payload. The request then silently sends the stale text from `payload` instead of the transcript in `msg.content` (also occurs for text-only turns, where both call lists are empty); validate the current text blocks against the payload before replaying, or rebuild the assistant message.
11. In `@providers/ollama/src/ai/common/Ollama_Messages.ts` (line 88, Important):
`for (const msg of history)`
When a `ToolCallingTask` caller puts its system instructions in `messages` and leaves `input.systemPrompt` unset, this loop has no `system` branch, so those instructions are omitted from the Ollama request. `ToolCallingTaskInput.messages` accepts full `ChatMessage` history (including `role: "system"`), and the text-generation converter already lifts such entries; preserve them here as well, while retaining the explicit system prompt behavior.
Additional findings outside changed lines:
1. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
2. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
3. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (3) — the lines they point at have not changed:
- 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:51
63a04e1 to
1f15283
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/155 files
Request changes — found 7 issue(s) at 1f15283.
Actionable comments posted: 7
Coverage is incomplete: reviewed 60/155 files; 0 skipped; 95 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/run/suites.ts` (line 146, Important):
`globWithin(wanted, allowed)` only recognizes containment for literals and simple trailing-prefix globs. For example, `--suite polyglot-lite --tasks '*_001'` overlaps `polyglot_python_*` on Python tasks, but neither pattern contains the other, so `kept` stays empty and the CLI throws “matches none” instead of running the matching tasks. Handle valid overlaps (or only reject when disjointness is established).
2. In `@packages/ai/src/provider-utils/OpenAIShapedResponses.ts` (line 152, Important):
`if (native && sameCalls(native, message.tool_calls ?? [])) {`
This checks only tool calls, then replays every native output item and skips the current assistant content. If a caller edits or removes the assistant text while leaving its calls unchanged (including a text-only turn, where both call lists are empty), the old message item is still sent to the next Responses request and the edited transcript is ignored. Include the current assistant text in the replay check, or rebuild the turn when it differs.
3. In `@packages/ai/src/task/MessageConversion.ts` (line 199, Important):
`toOpenAIMessages` attaches the complete native assistant output whenever this provider key matches. Downstream, `buildResponsesInput` accepts those items based only on `sameCalls`; if a host edits or removes the assistant text while leaving its tool calls unchanged (and for a text-only turn, where both call lists are empty), it replays the old native message and ignores the current transcript. Check the text blocks against the native output too, or rebuild the assistant message when they differ.
4. In `@providers/ollama/src/ai/common/Ollama_Messages.ts` (line 134, Important):
`toolImages` is flushed at the end of each individual `role: "tool"` message. For a valid history with parallel calls represented as separate tool messages, if the first result contains an image, this inserts a synthetic user turn before the next call's result (assistant calls → tool result → user image → tool result). The later result is then no longer adjacent to the assistant's outstanding calls, so Ollama can treat the image as a new turn before receiving all results. Defer images across consecutive tool messages and flush them before the next non-tool message.
Additional findings outside changed lines:
1. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
2. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
3. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (3) — the lines they point at have not changed:
- 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 11, 2026 23:59
1f15283 to
4197146
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 68/155 files
Request changes — found 8 issue(s) at 4197146.
Actionable comments posted: 8
Coverage is incomplete: reviewed 60/155 files; 8 skipped; 87 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 188, Important):
`optionalInteger` truncates but does not enforce the schema's `minimum: 1`, and `AgentTask` passes function-tool arguments directly to `execute` without validating that schema. With `timeout: 0` (or a negative value), this starts `runCommand` with an immediate timer; the shell can still run a short command such as `touch marker` before the timer kills it, so the invalid timeout does not reliably prevent the command from executing. Reject values below 1 before spawning.
2. In `@examples/agent-eval/src/run/jobConfig.ts` (line 76, Important):
`split(/,(?=\s*[A-Za-z_][\w-]*=)/)` also splits inside a kwarg value whenever text after a comma looks like `key=`. For example, `workglow:append_system_prompt="When x=a, set y=b"` is split into separate pairs, corrupting the prompt; JSON-valued kwargs with such a string are affected too. Only recognize separators outside the value (or otherwise provide an escaping/quoting rule), so valid values containing this pattern survive parsing.
3. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 453, Important):
`thinking_delta` is only appended to the native replay payload here; unlike text deltas, it is never emitted as a `text-delta` on the `reasoning` port or accumulated into `ToolCallingTaskOutput.reasoning`. Consequently, Anthropic turns retain thoughts for replay but `AgentTask`'s `onReasoningDelta` and reasoning output/UI never receive them. Emit the thinking delta on the reasoning port as well as retaining it for replay.
4. In `@providers/google-gemini/src/ai/common/Gemini_ToolCalling.ts` (line 140, Important):
`if (separateImageParts.length > 0) {`
This flushes the image turn after each individual `role: "tool"` message. With an assistant turn containing parallel calls represented by separate tool messages, an image in the first result produces `assistant(c1,c2) → functionResponse(c1) → user(image) → functionResponse(c2)`. Gemini receives the image before the response for the other outstanding call, so it may treat the image as the next user turn before all call results are supplied. Accumulate these images across consecutive tool messages and emit them after their function responses.
5. In `@providers/llamacpp-server/src/ai/common/LlamaCppServer_ToolCalling.ts` (line 83, Important):
`toOpenAIMessages` only emits the separate `input.systemPrompt` and skips `role: "system"` entries while iterating `input.messages`. When a caller supplies its instructions as a system `ChatMessage` and leaves `systemPrompt` unset, this request omits those instructions entirely, changing the model's behavior. Lift system messages from the history (as the text-generation path does) before conversion.
Additional findings outside changed lines:
1. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
2. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
3. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (3) — the lines they point at have not changed:
- 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 00:08
4197146 to
03d69da
Compare
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 02:23
05549d9 to
c37dfca
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/159 files
Request changes — found 12 issue(s) at c37dfca.
Actionable comments posted: 12
Coverage is incomplete: reviewed 60/159 files; 0 skipped; 99 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 187, Important):
`max(1, int(limit - reserve))` rounds the computed budget back up to the full Harbor limit when `limit == 1`: `reserve` is 0.5, but the resulting `max_duration_sec` is 1. If the model call uses that budget, Harbor cancels `run()` at the same deadline before the adapter can write its summary and trajectory. Reject this boundary or keep the internal budget strictly below the resolved limit.
2. In `@examples/agent-eval/src/agent/tools/write.ts` (line 34, Important):
`writeFile(path, content, "utf8")` also opens named pipes. If the task contains a FIFO (or the model creates one with `mkfifo`) and passes it here, opening it for writing blocks until a reader appears. `runAgentTool` awaits this promise without enforcing the deadline itself, so the agent never returns its budget result and the trial hangs until its outer timeout. Reject non-regular targets before writing, as `read` and `edit` do.
3. In `@examples/agent-eval/src/run/suites.ts` (line 148, Important):
`return [];`
For a restricted suite, a valid glob such as `polyglot_[pj]ython_*` intersects `polyglot_python_*`, but neither head starts with the other because `[...]` is treated literally. This returns an empty intersection, so `run --suite polyglot-lite --tasks 'polyglot_[pj]ython_*'` reports that no tasks match instead of running the matching Python tasks. Preserve character-class semantics when intersecting suite filters, or reject unsupported glob syntax rather than silently concluding there is no overlap.
4. In `@packages/ai/src/task/AgentToolExecution.ts` (line 321, Important):
`output.content` is an ordered sequence, but this extraction gathers every text block before the separate image list. A tool returning text, image, then more text is presented to the model as both text blocks followed by the image, so surrounding text can become detached from the image it describes. Preserve the original block order when constructing the tool result.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
7. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (8) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
Collaborator
Author
|
Both failed because xAI's API answered "The model is currently at capacity due to high demand". The client reported this correctly as a Nothing in this PR changes what is sent on those two paths, so this is xAI's capacity, not this change. There is no fix to port. I've re-run the job once. Generated by Claude Code |
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 02:30
c37dfca to
0a9a5fb
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/159 files
Request changes — found 13 issue(s) at 0a9a5fb.
Actionable comments posted: 13
Coverage is incomplete: reviewed 60/159 files; 0 skipped; 99 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
2. In `@examples/agent-eval/src/agent/runCodingAgent.ts` (line 219, Important):
`steps` contains only settled rounds, so when `task.run()` throws after exhausting retries, none of that round’s failed attempts is represented here. For example, a retryable provider error on every attempt produces `retries: 0` (and the catch also sets `rounds` to `steps.length`, so it reports zero rounds). The benchmark summary therefore hides the failed calls; preserve the in-flight round/attempt counts when handling the rejection.
3. In `@packages/ai/src/task/AgentTask.ts` (line 656, Important):
`deadline = ...` is checked at the top of the loop, but `await context.updateProgress(undefined, "Thinking")` runs before `streamRound` without another check. A synchronous progress listener or slow progress sink can block past the deadline (the retry path already handles this exact overrun), after which the code still starts a billable model call beyond `maxDurationMs`.
4. In `@packages/ai/src/task/AgentToolExecution.ts` (line 270, Important):
`deadline - Date.now() > MAX_TIMER_MS` bypasses the expiry signal altogether. Since `maxDurationMs` accepts larger values (for example, 30 days), a pending human approval can remain unresolved past that deadline and keep the turn hanging indefinitely instead of returning its budget result.
5. In `@providers/ollama/src/ai/common/Ollama_Messages.ts` (line 150, Important):
`flattenBlocks` collects all text into this tool message while returning images separately; those images are later appended to a user turn after the tool results. For a result ordered as text-before, image, text-after, Ollama receives `text-before\ntext-after` as the tool result and only then the image, so surrounding text is no longer associated with the image it describes. Preserve positional markers in the tool result when deferring images.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
7. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (8) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 02:41
0a9a5fb to
d82d202
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 59/160 files
Request changes — found 10 issue(s) at d82d202.
Actionable comments posted: 10
Coverage is incomplete: reviewed 59/160 files; 0 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/openrouter/src/ai/common/OpenRouter_ToolCalling.ts` (line 65, Important):
`!delta.reasoning` is false when a chunk has non-empty `reasoning` but also `reasoning_content: ""`, so this branch does not synthesize the detail text or clear the empty alias. Downstream, `accumulateOpenAIChatStream` chooses `reasoning_content ?? reasoning`, and the empty string wins; that chunk's legacy reasoning (and any text available only in `reasoning_details`) is therefore omitted from the reasoning output. Treat an empty alias as absent when deciding which reasoning field to preserve.
Additional findings outside changed lines:
1. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
4. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
5. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
6. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 02:46
d82d202 to
578221b
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 59/160 files
Request changes — found 9 issue(s) outside changed lines at 578221b.
Actionable comments posted: 9
Coverage is incomplete: reviewed 59/160 files; 0 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 04:46
578221b to
0dd2a1e
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 59/161 files
Request changes — found 10 issue(s) at 0dd2a1e.
Actionable comments posted: 10
Coverage is incomplete: reviewed 59/161 files; 0 skipped; 102 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 59, Important):
`if (!Array.isArray(blocks)) return undefined;`
An array is not enough to establish that the stored native payload is replayable. For example, a schema-valid reasoning block with this provider key and `payload: "[null]"` passes the later empty call/text comparisons, so `parseOwnTurn` returns `[null]` and sends it as assistant content; Anthropic rejects the request rather than the turn being rebuilt. Validate each block's supported type and required fields before returning the payload.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 04:51
0dd2a1e to
e61c550
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 61/161 files
Request changes — found 13 issue(s) at e61c550.
Actionable comments posted: 13
Coverage is incomplete: reviewed 60/161 files; 1 skipped; 100 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 78, Important):
`round_timeout_sec` has no upper bound, and `workglow-agent.ts` multiplies it by 1000 before passing it to `AgentTask`, which uses that value in `setTimeout`. At 2,147,484 seconds and above the delay exceeds Node's 2,147,483,647 ms timer limit and is coerced to 1 ms, so each model round is abandoned almost immediately instead of honoring the configured timeout. Reject out-of-range values or schedule longer waits in bounded intervals.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 161, Important):
`setTimeout(() => finish(code), DRAIN_MS)` can resolve a timed-out command before the `KILL_GRACE_MS` escalation runs. For example, a command can start a background child that ignores `SIGTERM` and keep the foreground shell alive with `sleep`; on timeout the foreground shell exits, but the child keeps mutating files until the later `SIGKILL`. The tool returns its timeout error after the 250 ms drain, so a subsequent tool call can observe or modify files while the timed-out command is still running.
3. In `@examples/agent-eval/src/agent/tools/read.ts` (line 104, Important):
`readdir` loads every entry before `truncateHead` is applied to the joined listing below. Reading a directory with millions of entries therefore allocates the full Dirent array, names array, sort, and joined string before the output limit helps; in the memory-constrained agent container, that can exhaust memory and terminate the trial. The listing needs to be bounded while it is collected, rather than only after the complete listing is built.
4. In `@packages/ai/src/task/AgentTask.ts` (line 459, Important):
`left > 0` skips this and all later text blocks once an earlier block has consumed the budget exactly, without adding the truncation marker used by the overflow branch. For example, with `maxToolResultChars` equal to the first text block's length and another text block after an image, the later text disappears and the model is not told that the result was truncated; mark that omission as truncated too.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 04:59
e61c550 to
eb22b4b
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/161 files
Request changes — found 12 issue(s) at eb22b4b.
Actionable comments posted: 12
Coverage is incomplete: reviewed 60/161 files; 0 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 273, Important):
`--max-duration-sec` only makes `AgentTask` stop between rounds; it does not abort a model request already in progress, and this adapter does not derive a round timeout from the remaining Harbor time. For example, with a 60-second Harbor limit the computed budget is 30 seconds, but a round started just before then can remain blocked past the outer deadline, so Harbor cancels `run()` before it can write the summary and trajectory. Bound the in-flight request against the remaining external deadline if `time_budget` is meant to guarantee clean completion.
2. In `@packages/ai/src/task/AgentTask.ts` (line 1018, Important):
`turnStart` is the index of the newly appended prompt, but when `input.messages` already ends with a user message, that message and the prompt form the same pending user turn (as `normalizeHistoryForModel` handles). If the history exceeds `maxHistoryChars`, `trimHistoryForModel` may cut at the prompt's index and drop the earlier user message before normalization. The model then receives the new prompt without the prior question/context; use the start of the trailing user turn as `keepFrom` in this case.
3. In `@packages/ai/src/task/MessageConversion.ts` (line 220, Important):
`if (Array.isArray(items)) entry.native_items = items;`
An array alone does not show that the payload contains replayable Responses items. For example, a valid reasoning block with `provider: "openai"` and `payload: "[null]"` becomes `native_items: [null]`; `buildResponsesInput` then accepts it when the assistant message has no text or calls and sends `null` as an input item. The API rejects the request instead of rebuilding the turn. Validate the item shapes before attaching the payload (or before replay).
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 05:08
eb22b4b to
acad604
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 59/161 files
Request changes — found 14 issue(s) at acad604.
Actionable comments posted: 14
Coverage is incomplete: reviewed 59/161 files; 0 skipped; 102 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/runCodingAgent.ts` (line 138, Important):
`AgentTask` emits `Retrying in ...` before it sleeps, and `sleepUnlessAborted` can then reject if the hard stop or external signal fires during that backoff. This increments `retriesSeen` even though no retry request starts; the later `Math.max(retriesSeen, ...)` records that as a retry in the run summary. For interrupted runs, the benchmark retry count is therefore overstated.
2. In `@examples/agent-eval/src/agent/runSummary.ts` (line 139, Important):
`steps.length > 0 ? total : undefined`
`AgentTask` records a step only after a round settles, so a run with one priced round followed by a provider request that errors or is aborted has no step for that in-flight round. If that request was processed and billed before its response was lost, this returns the earlier-round subtotal as the full `costUsd`; the Harbor adapter then records it as priced and the comparison can understate trial cost and `$ / pass`. Invalidate the total when `runCodingAgent` observed a started round that has no settled step.
3. In `@examples/agent-eval/src/compare/harborJobs.ts` (line 271, Important):
`existsSync(join(path, "agent"))` makes an `agent/` directory a prerequisite for recognizing a trial. If Harbor writes `result.json` for an infrastructure/setup failure before creating that directory, recursion skips the trial result entirely, so the blocked trial disappears from comparisons; if every trial fails this way, the CLI reports that Harbor wrote no trial results. Identify trial results from their JSON fields rather than requiring agent logs.
4. In `@packages/ai/src/task/MessageConversion.ts` (line 333, Important):
`liftSystemMessages` lifts every system message here, but `AgentTask.streamRound` first calls `trimHistoryForModel`, which preserves only a *leading* system run. If an oversized `AgentTask` history contains an older user/assistant exchange followed by a system message and the new prompt, trimming at the prompt drops that intervening system message with the old exchange; the provider then receives no instruction that this conversion otherwise promises to retain. Preserve system-role messages across trimming or otherwise ensure they are lifted before trimming.
5. In `@providers/openrouter/src/ai/common/OpenRouter_ReasoningDetails.ts` (line 130, Important):
`if (!Array.isArray(details)) return undefined;`
This accepts arrays without checking their entries. A caller can supply a schema-valid reasoning block for this model with payload `{"calls":[],"details":[null]}`; `withReasoningDetails` then attaches `[null]` to the assistant request, and OpenRouter rejects the malformed `reasoning_details` instead of sending a rebuilt turn. Validate the detail shapes before returning the decoded turn.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 05:15
acad604 to
22f7425
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/161 files
Request changes — found 15 issue(s) at 22f7425.
Actionable comments posted: 15
Coverage is incomplete: reviewed 60/161 files; 0 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 254, Important):
`const tail = truncateTail(result.output)`
The capture cap limits `result.output` to 16 MiB, but `truncateTail` splits the entire captured string into an array before keeping only 2,000 lines. A command that emits millions of short lines can therefore multiply the capped output into a very large array and exhaust the agent; the byte cap does not bound this allocation.
2. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 72, Important):
`const lines = content.split("\n")`
When an exact match is absent, `fuzzyRanges` splits the entire file and then `normalized` maps every line into a second array. A newline-dense file near the allowed 64 MiB limit can create millions of entries and exhaust agent memory even though it passes the size check. Bound the fuzzy search's line materialization.
3. In `@examples/agent-eval/src/agent/tools/read.ts` (line 154, Important):
`(text.endsWith("\n") ? text.slice(0, -1) : text).split("\n")`
`MAX_READ_BYTES` limits the file to 64 MiB, but this still allocates one array entry per line before the output cap is applied; `window` then copies the array, and `truncateHead` splits the joined text again. A newline-dense file within the allowed size can therefore expand to hundreds of megabytes and OOM the agent. Keep only the requested/output-bounded lines while scanning.
4. In `@examples/agent-eval/src/compare/harborJobs.ts` (line 161, Important):
`models` includes `undefined`, so when one trial lacks `agent_info.model_info` while another has it, the known trial gets a model suffix but the unknown one does not. For example, a blocked setup trial for `workglow` is labeled `workglow` while successful trials are `workglow@anthropic/m`; `buildReport` then treats them as separate arms, leaving the blocked trial out of the actual arm’s totals. Resolve the model from available configuration or keep unknown-model trials grouped with their arm.
5. In `@examples/agent-eval/src/compare/harborJobs.ts` (line 204, Important):
This API-error branch returns before the later `OVERFLOW.test(text)` fallback. A non-Workglow trial with `exceptionType: "ApiError"` and a message such as `maximum context length exceeded` is therefore reported as `provider-error`, not `context-overflow`, making the failure-mode table misleading for API-reported context limits. Check the overflow message before classifying the error as a generic provider failure.
6. In `@packages/ai/src/task/AgentTask.ts` (line 450, Important):
`orderedToolBody` inserts a newline when it merges adjacent text blocks, but `runCalls` later computes `AgentToolRecord.chars` with `toolResultText`, which concatenates text blocks without separators. For a `ToolResultContent` containing `"a"` and `"b"`, the model receives `"a\nb"` while the step records 2 characters instead of 3, undercounting the tool-result metric.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 05:25
22f7425 to
95d996b
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/161 files
Request changes — found 13 issue(s) at 95d996b.
Actionable comments posted: 13
Coverage is incomplete: reviewed 59/161 files; 1 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 290, Important):
`stop` is passed as a relative `--stop-after-sec` only after `run()` has uploaded the instruction and credentials; the Node timer does not start until the agent process launches. Harbor's timeout, however, covers `run()` from its entry (as `_agent_timeout_sec` documents), so if those uploads take longer than the reserved margin (10 seconds for typical limits), the hard-stop fires after Harbor has already cancelled the run, and the summary/trajectory are lost. Calculate this delay from a timestamp taken at `run()` entry, or otherwise account for setup time when setting the relative stop.
2. In `@examples/agent-eval/src/agent/runSummary.ts` (line 99, Important):
`Usage.cached`/`imageCached` being `undefined` means the provider did not report those counts, not that they were zero. A round with reported input/output but no cache count passes `hasTokenCounts`, so this adds zero here and leaves `unreportedRounds` at zero; `workglow_agent.py` then publishes `n_cache_tokens: 0`, and the comparison marks the trial fully priced in tokens instead of flagging cache usage as unknown. Preserve per-category completeness (or otherwise avoid publishing an invented zero) so providers that omit cache accounting are not reported as having no cache tokens.
3. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 66, Important):
`fuzzyRanges` retries the entire `wanted` sequence from every line start. On a permitted newline-dense file with repeated `a` lines, an `oldText` containing hundreds or thousands of `a` lines followed by a non-matching final line makes each candidate scan nearly the whole needle, yielding O(file lines × needle lines) normalization work and effectively hanging the agent. Use a search that does not restart the full comparison at every line (or otherwise bound this work).
4. In `@examples/agent-eval/src/agent/tools/truncate.ts` (line 126, Important):
This adds one byte for a newline even when `text` has no final newline. For example, `truncateTail("abc", 100, 3)` treats the three-byte input as too large and returns only `"bc"` with `truncated: true`; at the default limit, an unterminated final line exactly 50 KiB loses its first character. Count the separator only when it is actually present.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
…ive-turn storage
- Each tool's ToolExecuteContext carries the turn's deadline. announceTimeLeft
appends the time left to each round's last tool result. A retry wait that
would pass the deadline ends the turn "budget".
- reviewPrompt asks the model for one check of its answer before the turn ends.
- Assistant turns keep their reasoning, and a provider's own turn
({ provider, payload }) rides on the reasoning block so the provider can
replay it; history trimming keeps the current turn whole and measures earlier
ones without their payloads.
- Tool results may carry images. Empty tool results and reasoning-only turns
are sent as strings, and Ollama and llama.cpp server histories are converted
instead of forwarded as content blocks.
- Each round's reasoning streams on the task's own reasoning port, held with
the text until the attempt settles; AgentRoundContext gains onReasoningDelta.
OpenAI replays the Responses items with their encrypted reasoning, and Anthropic its content blocks with their signed thinking, keyed by provider and model so another model gets the rebuilt turn. A turn is replayed only while its call ids still match, and emitted only when it is truly replayable: OpenAI requests encrypted reasoning only for a reasoning model and rebuilds earlier turns when it does not ask for it; Anthropic retries once with thinking stripped when a request is rejected as edited. Both send tool-result images on a user turn after the results (OpenAI) or beside them (Anthropic).
…loud APIs - DeepSeek sends its reasoning effort and replays reasoning_content. - OpenRouter replays reasoning_details beside the same tool-call ids, never thinking truncated before its signature, and never a turn with a call it filtered out. - xAI, OpenRouter and HF Inference send tool-result images in a user message; HF Inference gains an effort setting. - Gemini sends tool results as objects with errors under error, emits thought summaries as reasoning on Gemini 2.5 and later, and puts tool-result images in functionResponse.parts on Gemini 3+, or in a labelled turn of their own on older models, which reject that field.
…ocal runtimes - Ollama and llama.cpp server send the whole conversation with their own tool calls and replay thinking / reasoning_content; Ollama gains a think effort setting and sends tool-result images on a user turn after the results. - node-llama-cpp emits and replays thought segments, maps effort to a thought budget and never inlines image data. - HF Transformers routes <think> to reasoning (ThinkTagFilter), hands it back to the chat template whenever the history carries it, and names tool results. - Cactus pairs the latest user turn with the turn's prompt and emits its reasoning; Chrome AI's finish has the same shape as the other providers.
Ink rows, the web console and examples/web's TaskNode show the reasoning port separately and drop it once the answer starts; every other port is still the task's text. workglow agent chat and the run-event projection no longer print reasoning as answer text, and StreamingTextRow also draws while a task is streaming.
Runs AgentTask, opencode and pi on Terminal-Bench 2.0, Aider Polyglot and SWE-bench Verified through Harbor, with the same model, turn cap and reasoning level for every arm, and compares them task by task. The workglow arm is AgentTask plus read, bash, edit and write tools matching pi's: bash defaults to 120 s and is capped at the trial deadline, edit applies the edits that match and names the rest, and the system prompt lists the commands installed and asks for one verification once the change is complete. Includes ATIF trajectories, retries for trials the infrastructure lost, --tasks-dir for local task copies and a scripted mock model for offline runs.
The harness's tools read, write and run what a model hands them inside a throwaway container by design, which scanning reports as injection; default setup cannot exclude a path.
sroussey
force-pushed
the
claude/nifty-albattani-rbmooa
branch
from
October 12, 2026 05:34
95d996b to
259dff5
Compare
There was a problem hiding this comment.
Review progress ████░░░░░░ 60/161 files
Request changes — found 13 issue(s) at 259dff5.
Actionable comments posted: 13
Coverage is incomplete: reviewed 60/161 files; 0 skipped; 101 deferred and still unreviewed.
Comment @mergestorm-vortex review on this PR to review the remaining files.
🤖 Prompt for AI agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
Findings to address:
1. In `@examples/agent-eval/harbor/workglow_agent.py` (line 300, Important):
`max(1, stop - spent)` can move the stop past Harbor's deadline when setup has consumed nearly all of a short trial. For a 2-second limit, `_stop_after_sec()` returns 1 second; if the uploads take 1.1 seconds, `spent` is 1 and this still gives the agent a 1-second timer, so it stops at about 2.1 seconds—after Harbor cancels `run()` at 2 seconds, losing the summary and trajectory. The same minimum clamp on the derived loop budget can likewise leave it running beyond the outer deadline.
2. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 45, Important):
`found.push(at)`
`locate()` only needs to distinguish zero, one, or multiple matches, but `exactRanges` retains every hit (and `fuzzyRanges` similarly accumulates every matching range). On an allowed 64 MiB file containing repeated `a` characters, editing `oldText: "a"` builds tens of millions of indices before reporting ambiguity; a fuzzy match repeated across many lines can build millions of range objects. This can exhaust the benchmark agent instead of returning the ambiguity error; stop collecting once uniqueness is disproven.
3. In `@examples/agent-eval/src/run/suites.ts` (line 113, Important):
`splitStar` rejects every pattern containing `?`, so `narrowDataset` throws for valid, directly expressible filters such as `--suite polyglot-lite --tasks 'polyglot_python_?ank'`: that filter is already a subset of the suite's `polyglot_python_*` pattern, but `globIntersection` returns `undefined` instead of retaining it. This makes the advertised glob filter unusable with `?` on restricted suites.
4. In `@examples/agent-eval/src/test/tools.test.ts` (line 580, Important):
`loadCredentialsFile` adds `WORKGLOW_TEST_API_KEY` after the Vitest process has started, but Linux `/proc/<pid>/environ` exposes the process's exec-time environment; variables added later are not reflected there. This assertion can therefore pass even if a provider key inherited when the agent starts remains readable to model-controlled commands via `/proc/$PPID/environ`. Exercise the check in a subprocess launched with the credential already in its environment.
Additional findings outside changed lines:
1. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 34, Blocker):
Filter the Hugging Face credential
`HF_TOKEN` is the credential used by this repository's Hugging Face Inference provider (`providers/huggingface-inference/src/ai/common/HFI_Client.ts`), but its name does not match either suffix here. When that provider runs the agent, model-controlled `bash` can print `$HF_TOKEN` into tool output and the persisted transcript. Include this credential name in the shell environment filter.
2. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 54, Blocker):
Do not pass provider credentials into the model-controlled shell
`env: process.env` includes the credential used by this agent (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, etc.; Harbor passes `model_connection.env` to the agent, and `registerAgentProvider` reads credentials from the environment). Since the model controls `command`, it can run `printenv ANTHROPIC_API_KEY`; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment.
3. In `@examples/agent-eval/src/agent/tools/bash.ts` (line 69, Blocker):
Filtered shell can still read provider credentials from `/proc`
`env: shellEnv()` only removes credentials from the child environment; Harbor starts the Node agent with `model_connection.env`, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g. `tr '\0' '\n' </proc/$PPID/environ`, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop.
4. In `@examples/agent-eval/src/agent/tools/edit.ts` (line 249, Blocker):
Reject non-regular files before reading
`stat('/dev/zero')` succeeds with size 0, so this size check passes and the following `readFile` reads until EOF—which `/dev/zero` never supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain the `Stats` result and reject non-regular files before reading, as `read` does.
5. In `@packages/ai/src/task/ToolCallingTask.ts` (line 514, Blocker):
Preserve Gemini signatures in emitted checkpoints
`replyContent` rebuilds each `tool_use` from `id`, `name`, and `input`, but omits `tc.providerSignature`. For Gemini 2.5+/3, `buildGeminiContents` requires that thought signature on replay; after a `ToolCallingTask` emits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block.
6. In `@providers/anthropic/src/ai/common/Anthropic_ToolCalling.ts` (line 334, Blocker):
Avoid replaying signed thinking when forced choice disables it
`buildAnthropicMessages` replays the prior signed thinking blocks unconditionally for this model key, but on a legacy model with `toolChoice: "required"` (or a named forced choice), `applyAnthropicThinkingParams` deliberately omits `thinking` because Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run.
7. In `@providers/google-gemini/src/ai/common/Gemini_Client.ts` (line 179, Blocker):
Do not request thought summaries from older Gemini models
`const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;`
`geminiEffortPolicy` treats every `gemini-*` text model as supporting effort, including `gemini-1.0-pro` and `gemini-1.5-*` (both still declared tool-use-capable in `Gemini_Capabilities.ts`). `Gemini_ToolCalling_Stream` now always passes `includeThoughts: true`, so those models receive `thinkingConfig: { includeThoughts: true }`, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them.
8. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route SDK text chunks to the answer port
`if (chunk.type === undefined)`
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, so this branch drops every answer delta; `routeLlamaResponseChunk` then only preserves thought segments, leaving `accumulatedText` empty and the tool-calling run's `text` output blank. Accept the SDK's text chunk type as well as the untyped form.
9. In `@providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.ts` (line 303, Blocker):
Route ordinary response chunks to the text port
`onResponseChunk` emits ordinary answer chunks with `type: "text"`, but this branch only accepts chunks whose `type` is `undefined`; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped, `accumulatedText` stays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to the `text` port as well.
Still open from earlier reviews (9) — the lines they point at have not changed:
- 🔴 Blocker · Filter the Hugging Face credential ·
examples/agent-eval/src/agent/tools/bash.tsline 34 · frome7b5acb
HF_TOKENis the credential used by this repository's Hugging Face Inference provider (providers/huggingface-inference/src/ai/common/HFI_Client.ts), but its name does not match either suffix here. When that provider runs the agent, model-controlledbashcan print$HF_TOKENinto tool output and the persisted transcript. Include this credential name in the shell environment filter. - 🔴 Blocker · Do not pass provider credentials into the model-controlled shell ·
examples/agent-eval/src/agent/tools/bash.tsline 54 · from8792152
env: process.envincludes the credential used by this agent (ANTHROPIC_API_KEY,OPENAI_API_KEY, etc.; Harbor passesmodel_connection.envto the agent, andregisterAgentProviderreads credentials from the environment). Since the model controlscommand, it can runprintenv ANTHROPIC_API_KEY; that value is returned as tool output and forwarded into the conversation and event log. This exposes live provider credentials to model-generated commands and persisted logs. Filter credential variables from the shell environment. - 🔴 Blocker · Filtered shell can still read provider credentials from
/proc·examples/agent-eval/src/agent/tools/bash.tsline 69 · from99d5569
env: shellEnv()only removes credentials from the child environment; Harbor starts the Node agent withmodel_connection.env, so the provider keys remain in the parent process. In the Linux task container, a model-controlled command can read the same-UID agent environment with e.g.tr '\0' '\n' </proc/$PPID/environ, then return the key in tool output and the persisted transcript. Thus this filter does not prevent the credential exposure it is intended to stop. - 🔴 Blocker · Reject non-regular files before reading ·
examples/agent-eval/src/agent/tools/edit.tsline 249 · frome7b5acb
stat('/dev/zero')succeeds with size 0, so this size check passes and the followingreadFilereads until EOF—which/dev/zeronever supplies—growing memory until the agent is OOM-killed. The edit tool accepts absolute paths; retain theStatsresult and reject non-regular files before reading, asreaddoes. - 🔴 Blocker · Preserve Gemini signatures in emitted checkpoints ·
packages/ai/src/task/ToolCallingTask.tsline 514 · from1ca5533
replyContentrebuilds eachtool_usefromid,name, andinput, but omitstc.providerSignature. For Gemini 2.5+/3,buildGeminiContentsrequires that thought signature on replay; after aToolCallingTaskemits a checkpoint containing a tool call, a consumer replays the prefix without it and Gemini rejects the next request with “Function call is missing a thought_signature.” Include the signature when recording each checkpointed tool-use block. - 🔴 Blocker · Avoid replaying signed thinking when forced choice disables it ·
providers/anthropic/src/ai/common/Anthropic_ToolCalling.tsline 334 · from0a9a5fb
buildAnthropicMessagesreplays the prior signed thinking blocks unconditionally for this model key, but on a legacy model withtoolChoice: "required"(or a named forced choice),applyAnthropicThinkingParamsdeliberately omitsthinkingbecause Anthropic disallows extended thinking with forced tool choice. If the supplied history contains a thinking turn from an earlier auto-choice request, the next request sends those thinking blocks without thinking enabled and Anthropic rejects it with HTTP 400; the retry only handles invalid-signature errors, so this path fails the run. - 🔴 Blocker · Do not request thought summaries from older Gemini models ·
providers/google-gemini/src/ai/common/Gemini_Client.tsline 179 · fromec97452
const thinks = budget !== 0 && geminiEffortPolicy(model).supported.length > 0;
geminiEffortPolicytreats everygemini-*text model as supporting effort, includinggemini-1.0-proandgemini-1.5-*(both still declared tool-use-capable inGemini_Capabilities.ts).Gemini_ToolCalling_Streamnow always passesincludeThoughts: true, so those models receivethinkingConfig: { includeThoughts: true }, a feature unsupported before Gemini 2.5, and reject otherwise-valid tool-calling requests. Restrict thought summaries to model families that support them. - 🔴 Blocker · Route SDK text chunks to the answer port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · from89e1154
if (chunk.type === undefined)
onResponseChunkemits ordinary answer chunks withtype: "text", so this branch drops every answer delta;routeLlamaResponseChunkthen only preserves thought segments, leavingaccumulatedTextempty and the tool-calling run'stextoutput blank. Accept the SDK's text chunk type as well as the untyped form. - 🔴 Blocker · Route ordinary response chunks to the text port ·
providers/node-llama-cpp/src/ai/common/LlamaCpp_ToolCalling.tsline 303 · fromec97452
onResponseChunkemits ordinary answer chunks withtype: "text", but this branch only accepts chunks whosetypeisundefined; the subsequent checks only retain thought segments. As a result, every normal answer chunk is dropped,accumulatedTextstays empty, and this run-fn emits no answer text for node-llama-cpp tool-calling runs. Route the SDK's text chunk type to thetextport as well.
| flags = f"{flags} --max-duration-sec {max(1, budget - spent)}".strip() | ||
| stop = self._stop_after_sec() | ||
| if stop is not None: | ||
| flags = f"{flags} --stop-after-sec {max(1, stop - spent)}".strip() |
There was a problem hiding this comment.
Keep the hard stop inside the remaining trial budget
max(1, stop - spent) can move the stop past Harbor's deadline when setup has consumed nearly all of a short trial. For a 2-second limit, _stop_after_sec() returns 1 second; if the uploads take 1.1 seconds, spent is 1 and this still gives the agent a 1-second timer, so it stops at about 2.1 seconds—after Harbor cancels run() at 2 seconds, losing the summary and trajectory. The same minimum clamp on the derived loop budget can likewise leave it running beyond the outer deadline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/harbor/workglow_agent.py` (line 300, Important):
`max(1, stop - spent)` can move the stop past Harbor's deadline when setup has consumed nearly all of a short trial. For a 2-second limit, `_stop_after_sec()` returns 1 second; if the uploads take 1.1 seconds, `spent` is 1 and this still gives the agent a 1-second timer, so it stops at about 2.1 seconds—after Harbor cancels `run()` at 2 seconds, losing the summary and trajectory. The same minimum clamp on the derived loop budget can likewise leave it running beyond the outer deadline.
| function exactRanges(content: string, needle: string): number[] { | ||
| const found: number[] = []; | ||
| let at = content.indexOf(needle); | ||
| while (at !== -1) { |
There was a problem hiding this comment.
Bound match collection to the uniqueness check
found.push(at)
locate() only needs to distinguish zero, one, or multiple matches, but exactRanges retains every hit (and fuzzyRanges similarly accumulates every matching range). On an allowed 64 MiB file containing repeated a characters, editing oldText: "a" builds tens of millions of indices before reporting ambiguity; a fuzzy match repeated across many lines can build millions of range objects. This can exhaust the benchmark agent instead of returning the ambiguity error; stop collecting once uniqueness is disproven.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/agent/tools/edit.ts` (line 45, Important):
`found.push(at)`
`locate()` only needs to distinguish zero, one, or multiple matches, but `exactRanges` retains every hit (and `fuzzyRanges` similarly accumulates every matching range). On an allowed 64 MiB file containing repeated `a` characters, editing `oldText: "a"` builds tens of millions of indices before reporting ambiguity; a fuzzy match repeated across many lines can build millions of range objects. This can exhaust the benchmark agent instead of returning the ambiguity error; stop collecting once uniqueness is disproven.
| /** A glob with exactly one `*` and no `?`, split at the star. */ | ||
| function splitStar(glob: string): { readonly head: string; readonly tail: string } | undefined { | ||
| const star = glob.indexOf("*"); | ||
| if (star < 0 || glob.indexOf("*", star + 1) >= 0 || glob.includes("?")) return undefined; |
There was a problem hiding this comment.
Allow expressible ? glob intersections
splitStar rejects every pattern containing ?, so narrowDataset throws for valid, directly expressible filters such as --suite polyglot-lite --tasks 'polyglot_python_?ank': that filter is already a subset of the suite's polyglot_python_* pattern, but globIntersection returns undefined instead of retaining it. This makes the advertised glob filter unusable with ? on restricted suites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/run/suites.ts` (line 113, Important):
`splitStar` rejects every pattern containing `?`, so `narrowDataset` throws for valid, directly expressible filters such as `--suite polyglot-lite --tasks 'polyglot_python_?ank'`: that filter is already a subset of the suite's `polyglot_python_*` pattern, but `globIntersection` returns `undefined` instead of retaining it. This makes the advertised glob filter unusable with `?` on restricted suites.
| // a filter on the shell's environment cannot reach. | ||
| const out = String( | ||
| await createBashTool(context).execute!( | ||
| { command: "env; tr '\\0' '\\n' < /proc/$PPID/environ" }, |
There was a problem hiding this comment.
Test an exec-time parent credential
loadCredentialsFile adds WORKGLOW_TEST_API_KEY after the Vitest process has started, but Linux /proc/<pid>/environ exposes the process's exec-time environment; variables added later are not reflected there. This assertion can therefore pass even if a provider key inherited when the agent starts remains readable to model-controlled commands via /proc/$PPID/environ. Exercise the check in a subprocess launched with the credential already in its environment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.
In `@examples/agent-eval/src/test/tools.test.ts` (line 580, Important):
`loadCredentialsFile` adds `WORKGLOW_TEST_API_KEY` after the Vitest process has started, but Linux `/proc/<pid>/environ` exposes the process's exec-time environment; variables added later are not reflected there. This assertion can therefore pass even if a provider key inherited when the agent starts remains readable to model-controlled commands via `/proc/$PPID/environ`. Exercise the check in a subprocess launched with the credential already in its environment.
This branch has not been deployed
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
2 participants
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.
Summary
This branch takes
AgentTaskfrom a basic loop to one that competes with opencode and pi on public agent benchmarks. It also brings every tool-calling provider up to what that loop needs.Agent-eval harness (
examples/agent-eval)AgentTask, opencode and pi on Terminal-Bench 2.0, Aider Polyglot and SWE-bench Verified through Harbor. All three arms use the same model, turn cap and reasoning level, and the harness compares them task by task.mock-modelfor offline end-to-end runs, retries for trials the infrastructure lost, and a--tasks-diroption for local task copies.AgentTaskplus four coding tools (read,bash,edit,write):bashdefaults to a 120 s timeout and is capped at the trial deadline.editapplies the edits that match and names the ones that don't.AgentTask loop
ToolExecuteContext.announceTimeLeftshows the model how much time is left, and a retry wait that would pass the deadline ends the turn with"budget".reviewPromptasks the model for one check of its answer before the turn ends.reasoningoutput port. It is held back with the text until each attempt settles, so a failed attempt contributes neither.AgentRoundContextgainsonReasoningDelta.Provider parity
thinking/reasoning_content). Ollama's browser build now reads the conversation, and Ollama also gets athinkeffort setting.error.functionResponse.parts. Older models get them in a separate labelled turn: they reject that field with HTTP 400, and an image placed beside the response leaked their thinking into the answer.reasoning_detailsis replayed between rounds, guarded by call ids. Thinking cut off before its signature is never replayed.<think>text goes to reasoning instead of the answer, and is sent back to the chat template. Tool-result messages carry the tool's name.CLI and web examples
examples/web'sTaskNodeshow reasoning apart from the answer.workglow agent chatand the run-event projection no longer print reasoning as answer text.StreamingTextRowwas only drawn while a task was processing, but the first streamed chunk moves a task into the streaming state, so the panel rarely appeared. It now also draws while streaming, so streamed answers now show in Ink rows.Results
docs/superpowers/evals/andplans/.Downstream
Builder and embarc-data each have a PR on the same branch name that needs this one released first: workglow-dev/builder and sroussey/embarc-data. Both pin
@workglow/*at 0.6.12, so the pins need bumping to the release that carries this.Test plan
ai,task,ai-provider*directories pass: 3,545 tests in the last whole-branch run.tscis clean onpackages/aiand every touched provider; oxlint and oxfmt are clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Gkqic45a2wtQiXKsKD96x7
Generated by Claude Code