fix(agent): harden model execution continuity - #262
Conversation
|
Independent final-head audit found blockers after the initial review slices passed. Holding this PR in draft while fixing:
I will update the branch with focused regressions, rerun autoreview, and restore ready status only after the exact head is green. |
|
Codex review: found issues before merge. Reviewed July 10, 2026, 11:17 PM ET / July 11, 2026, 03:17 UTC. Summary Reproducibility: yes. The exact source paths provide high-confidence reproductions for unterminated tool streams, narrated terminal tools, URL userinfo retention, and swallowed resumed-turn failures, and the collaborator author independently confirmed the combined-head blockers. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Finish the claimed exact-head repair with terminal-event gating, explicit terminal-result preservation, balanced lifecycle events, fully credential-free identities and errors, timestamp-safe persistence, nonzero resume failures, isolated tests, aligned docs, and paired Tachikoma validation. Do we have a high-confidence way to reproduce the issue? Yes. The exact source paths provide high-confidence reproductions for unterminated tool streams, narrated terminal tools, URL userinfo retention, and swallowed resumed-turn failures, and the collaborator author independently confirmed the combined-head blockers. Is this the best way to solve the issue? No for the current head. The continuity architecture and test investment are directionally strong, but the implementation is not the safest solution until the confirmed ordering, content-preservation, redaction, exit-status, lifecycle, timestamp, documentation, and isolation defects are repaired. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9be5918dac92. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb0b0fdb1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| provider: configuration.provider, | ||
| messages: state.messages, |
There was a problem hiding this comment.
This streaming path sends the persisted transcript verbatim. When a session containing provider-specific reasoning/thinking messages is resumed with an explicit different streaming model or provider, those .thinking entries are replayed instead of being filtered by sanitizedForProviderContext(...) as in runGenerationLoop, so providers can receive incompatible signed/native reasoning and reject or mis-handle the continuation. Pass the sanitized message list here using configuration.provider before calling streamText.
Useful? React with 👍 / 👎.
|
|
||
| components.scheme = scheme | ||
| components.host = host | ||
| components.password = nil |
There was a problem hiding this comment.
canonicalEndpointIdentity removes the password but leaves components.user in the URL that is hashed. For custom endpoints configured with basic-auth/userinfo or a token in the username part, the session identity still depends on credential material, so rotating that username breaks otherwise-valid session resume and persists a credential-derived fingerprint. Clear the user field before hashing the canonical endpoint.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: sufficient
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb0b0fdb1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| components.scheme = scheme | ||
| components.host = host | ||
| components.password = nil | ||
| components.fragment = nil |
There was a problem hiding this comment.
When a custom endpoint is configured with credentials in the URL query (for example ?token=... or ?api-key=...), components.string below still includes that query in the SHA-256 input. Since this value is persisted as modelEndpointIdentity and later compared on resume, rotating the token makes an otherwise unchanged session fail verification and leaves a credential-derived fingerprint in session storage. Clear components.query before hashing the canonical endpoint.
Useful? React with 👍 / 👎.
1 participant
Summary
--max-stepscount model turns, validate1...100, finish pending tool work correctly, and report cancellation, usage, tool, and terminal events exactly onceProof
pnpm run build:clipnpm run test:safe— 721 tests in 82 suitesgit diff --checkLive Ollama validation
Ran the debug CLI against native
ollama/qwen3.5:9bwith a task requiring exactly one shell call topwd,--max-steps 4, local-only routing, and verbose streaming. The run usedOllamaProvider, streamed reasoning, invoked the shell exactly once, completed a second model turn, emitted the terminal event, reported two model turns, and returned the exact working directory.Tachikoma prerequisites are already landed in openclaw/Tachikoma#37 and openclaw/Tachikoma#38; their exact-head CI matrices are green.