Fix ACP manual-spawn task tracking#97131
Conversation
|
Source review note: I think this needs one more guard before it is safe to merge. The new confirmation path in Best fix: keep the suppression, but make the ACP confirmation best-effort too. For example, wrap the metadata read in a small helper that returns |
|
Codex review: needs maintainer review before merge. Reviewed June 26, 2026, 11:06 PM ET / 03:06 UTC. Summary PR surface: Source +62, Tests +304. Total +366 across 2 files. Reproducibility: yes. at source level. Current main creates a CLI task row for accepted Gateway agent runs with a session key while ACP manual spawn creates ACP task rows for the same child run id. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land the backend-gated suppression with the tested metadata and caller fallback cases intact, preserving ordinary CLI tracking and plugin-subagent precedence while removing the duplicate ACP manual-spawn CLI row. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main creates a CLI task row for accepted Gateway agent runs with a session key while ACP manual spawn creates ACP task rows for the same child run id. Is this the best way to solve the issue? Yes. The latest PR shape is the narrow maintainable fix because Gateway has the trusted caller/acpTurnSource context, while the guard preserves plugin_subagent precedence and falls back to CLI tracking when confirmation fails. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cee2aca40912. Label changesLabel justifications:
Evidence reviewedPR surface: Source +62, Tests +304. Total +366 across 2 files. View PR surface stats
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
|
5b3bbb5 to
5cc1564
Compare
5cc1564 to
3ded77d
Compare
|
Addressed the review note in the latest push. The ACP metadata confirmation is now best-effort: I also added the requested regression: Local proof after the change: |
|
@clawsweeper re-review The maintainer review note is addressed on the latest head: ACP metadata confirmation now catches read/open/query failures and falls back to CLI task tracking, with a focused regression covering a throwing |
|
🦞🧹 I asked ClawSweeper to review this item again. |
3ded77d to
a428638
Compare
|
Addressed the remaining source-review blocker in the latest push and requested re-review. What changed:
Local proof: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Added the requested real ACP manual-spawn runtime proof to the PR body. The redacted task-ledger output shows the ACP/subagent rows for the manual-spawn smoke and |
Co-authored-by: Moeed Ahmed <[email protected]>
Co-authored-by: Moeed Ahmed <[email protected]>
Co-authored-by: Moeed Ahmed <[email protected]>
Co-authored-by: Moeed Ahmed <[email protected]>
What Problem This Solves
ACP manual-spawn child turns can create duplicate requester-visible task rows because the Gateway CLI task-tracking path runs alongside the ACP spawn control plane for the same child run.
This showed up in runtime smoke as multiple
task_runsrows for one ACP manual-spawn run, including a duplicateruntime = clichild-owned row in addition to ACP/subagent rows.Why This Change Was Made
A confirmed ACP manual-spawn child turn already owns its requester-visible
acptask row from the spawn control plane. The Gateway CLI path should not emit a second row for the same child run.The suppression is intentionally narrow:
plugin_subagenttracking precedence;gateway-client+backend, excluding device-token clients), so public/operator-write callers cannot hide CLI rows by settingacpTurnSource;User Impact
This reduces duplicate/noisy task-ledger rows for manual ACP spawns while preserving normal task visibility for plugin subagents, ordinary CLI agent runs, and non-backend operator-write callers.
Evidence
Focused regression test added in
src/gateway/server-methods/agent.test.tscovering:acpTurnSource: "manual_spawn"with persisted ACP metadata still keeps CLI tracking;Local proof from this branch:
Type-check proof:
Formatting proof:
Known Baseline Test Caveat
A wider local run of
pnpm test src/gateway/server-methods/agent.test.tscurrently fails in the existing testtracks plugin SDK subagent agent runs through the subagent registry only, wherecleanupCompletedAtremainsundefined.I verified the same focused test fails on fresh
origin/mainbefore this PR, so it is an existing adjacent baseline issue, not caused by this change.Real Behavior Proof
After-fix ACP manual-spawn runtime smoke was run against a real installed OpenClaw gateway using the normal parent
sessions_spawnpath with ACP runtime and Claude route. The child turn completed successfully.Redacted task-ledger proof for the exact smoke label:
Duplicate CLI-row check for the same smoke label:
Gateway health was checked after the smoke and reported healthy.
Secrets / Unrelated Changes
No secrets, config changes, generated files, runtime hotfixes, or unrelated files are included.