Skip to content

Fix ACP manual-spawn task tracking#97131

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
moeedahmed:fix/acp-manual-spawn-task-tracking
Jun 29, 2026
Merged

Fix ACP manual-spawn task tracking#97131
vincentkoc merged 1 commit into
openclaw:mainfrom
moeedahmed:fix/acp-manual-spawn-task-tracking

Conversation

@moeedahmed

@moeedahmed moeedahmed commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

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_runs rows for one ACP manual-spawn run, including a duplicate runtime = cli child-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 acp task 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:

  • keep plugin_subagent tracking precedence;
  • keep ordinary CLI tracking;
  • keep ACP-shaped turns without persisted ACP metadata tracked as CLI;
  • make ACP metadata confirmation best-effort, so metadata read/open/query failures fall back to normal CLI tracking instead of blocking the child run;
  • gate suppression to the trusted in-process backend ACP spawn caller (gateway-client + backend, excluding device-token clients), so public/operator-write callers cannot hide CLI rows by setting acpTurnSource;
  • suppress only confirmed ACP manual-spawn child turns with persisted ACP session metadata.

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.ts covering:

  • confirmed trusted-backend ACP manual-spawn child turn suppresses the Gateway CLI task row;
  • non-backend operator-write caller setting acpTurnSource: "manual_spawn" with persisted ACP metadata still keeps CLI tracking;
  • ACP-shaped manual-spawn without persisted ACP metadata keeps CLI tracking;
  • ACP metadata read failure keeps dispatch alive, creates/finalizes the CLI row, and logs the fallback;
  • ACP-shaped non-manual-spawn keeps CLI tracking;
  • plugin-subagent tracking still wins and does not emit a duplicate CLI row.

Local proof from this branch:

pnpm test src/gateway/server-methods/agent.test.ts -t "ACP manual-spawn child turn task tracking"

Test Files  2 passed (2)
Tests       12 passed | 330 skipped (342)

Type-check proof:

pnpm tsgo:test

passed

Formatting proof:

npx oxfmt --check --threads=1 src/gateway/server-methods/agent.ts src/gateway/server-methods/agent.test.ts

All matched files use the correct format.

Known Baseline Test Caveat

A wider local run of pnpm test src/gateway/server-methods/agent.test.ts currently fails in the existing test tracks plugin SDK subagent agent runs through the subagent registry only, where cleanupCompletedAt remains undefined.

I verified the same focused test fails on fresh origin/main before 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_spawn path with ACP runtime and Claude route. The child turn completed successfully.

Redacted task-ledger proof for the exact smoke label:

runtime   status     delivery_status  child           parent
--------  ---------  ---------------  --------------  ------
acp       succeeded  session_queued   redacted-child  none
subagent  succeeded  delivered        redacted-child  none
acp       succeeded  not_applicable   redacted-child  none

Duplicate CLI-row check for the same smoke label:

cli_rows
--------
0

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.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: M labels Jun 27, 2026
@harjothkhara

Copy link
Copy Markdown
Contributor

Source review note: I think this needs one more guard before it is safe to merge.

The new confirmation path in src/gateway/server-methods/agent.ts now calls readAcpSessionMeta({ sessionKey: resolvedSessionKey }) before task tracking is resolved and before the gateway dispatches agentCommandFromIngress. That metadata helper is not purely in-memory: it opens/queries the shared state SQLite DB through openOpenClawStateDatabase, and that opener rethrows schema/open/pragma failures. Task creation itself is intentionally best-effort in dispatchAgentRunFromGateway (the createRunningTaskRun call is caught and only logs), so this new pre-dispatch read can make an ACP manual-spawn child turn fail solely because the task-row de-duplication check could not read metadata.

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 false and logs on read/open/query failure, so the request falls back to normal CLI task tracking instead of blocking the child run. A focused regression would mock readAcpSessionMeta to throw for acpTurnSource: "manual_spawn" and assert the agent still dispatches and creates/finalizes the CLI row.

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 26, 2026, 11:06 PM ET / 03:06 UTC.

Summary
The branch changes Gateway agent task tracking so confirmed trusted-backend ACP manual-spawn child turns with persisted ACP metadata suppress the duplicate CLI task row, with focused regression tests for suppression and fallback cases.

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.

  • Pre-dispatch ACP confirmation: 1 added. This is the only new decision point that suppresses CLI task-row creation, so maintainers should notice that it is narrowly gated and covered by fallback tests.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Next step before merge

  • No automated repair-lane work remains; the PR is ready for ordinary maintainer merge review with normal gates.

Security
Cleared: No concrete security or supply-chain concern was found; the diff changes Gateway runtime logic and tests only, with no dependency, workflow, secret, or package-surface changes.

Review details

Best 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 changes

Label justifications:

  • P2: This is a bounded Gateway/ACP task-ledger bug with limited blast radius and no evidence of emergency data loss, security bypass, or core runtime outage.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes redacted after-fix installed-gateway ACP manual-spawn task-ledger output with ACP/subagent rows and cli_rows = 0 for the same smoke label.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted after-fix installed-gateway ACP manual-spawn task-ledger output with ACP/subagent rows and cli_rows = 0 for the same smoke label.
Evidence reviewed

PR surface:

Source +62, Tests +304. Total +366 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 65 3 +62
Tests 1 304 0 +304
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 369 3 +366

What I checked:

  • Repository policy read: Read the full root AGENTS.md plus scoped Gateway AGENTS.md files; the Gateway hot-path and review-depth guidance affected the review. (AGENTS.md:1, cee2aca40912)
  • Current-main CLI tracking path: Current main resolves accepted Gateway agent runs with a non-empty session key to CLI task tracking unless plugin_subagent or inter-session handling applies. (src/gateway/server-methods/agent.ts:580, cee2aca40912)
  • Current-main ACP replacement row path: ACP manual spawn calls the Gateway agent method with acpTurnSource="manual_spawn" and later creates runtime="acp" task rows with the child run id, which is the duplicate-row path this PR targets. (src/agents/acp-spawn.ts:1580, cee2aca40912)
  • PR implementation guard: The PR head gates CLI suppression to gateway-client/backend non-device callers, requires manual-spawn ACP metadata, catches metadata read failures, and passes the confirmed flag into task-tracking resolution. (src/gateway/server-methods/agent.ts:604, a4286389212e)
  • PR regression coverage: The added tests cover confirmed suppression, non-backend fallback, missing metadata fallback, metadata-read failure fallback, non-manual-spawn CLI tracking, and plugin-subagent precedence. (src/gateway/server-methods/agent.test.ts:4718, a4286389212e)
  • Real behavior proof supplied: The PR body includes redacted installed-gateway ACP manual-spawn task-ledger output showing ACP/subagent rows and cli_rows = 0 for the same smoke label. (a4286389212e)

Likely related people:

  • steipete: Recent history shows work on ACP metadata persistence and task requester attribution, both adjacent to the ACP task-row ownership this PR changes. (role: recent ACP/task tracking contributor; confidence: high; commits: db40fde88c6b, 1fef20c96bdc, 606e3d78669a; files: src/acp/runtime/session-meta.ts, src/agents/acp-spawn.ts, src/tasks/detached-task-runtime.ts)
  • jalehman: Recent ACP metadata accessor work is directly adjacent to the readAcpSessionMeta confirmation path added by this PR. (role: recent ACP metadata accessor contributor; confidence: medium; commits: 9512294e8f62; files: src/acp/runtime/session-meta.ts, src/agents/acp-spawn.ts)
  • vincentkoc: Recent commits touched Gateway agent handling and task/subagent recovery areas near the runtime path and task-ledger behavior under review. (role: adjacent Gateway and task contributor; confidence: medium; commits: 5571c786d3df, c2ee9b0be8ae, 273eed4c51cb; files: src/gateway/server-methods/agent.ts, src/tasks/task-registry.ts)
  • hansraj316: Recent Gateway work made background-task finalization failures observable on the same dispatch lifecycle this PR changes. (role: recent Gateway task lifecycle contributor; confidence: medium; commits: b9e1099f5e5b; files: src/gateway/server-methods/agent.ts)
  • mbelinky: Earlier detached task lifecycle runtime extraction and plugin-registration work owns the task runtime API that Gateway and ACP rows both use. (role: detached task runtime contributor; confidence: medium; commits: 0787266637fd, bd3ad3436efd; files: src/tasks/detached-task-runtime.ts, src/tasks/task-registry.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@moeedahmed
moeedahmed force-pushed the fix/acp-manual-spawn-task-tracking branch from 5b3bbb5 to 5cc1564 Compare June 27, 2026 01:11
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 27, 2026
@moeedahmed
moeedahmed force-pushed the fix/acp-manual-spawn-task-tracking branch from 5cc1564 to 3ded77d Compare June 27, 2026 01:17
@moeedahmed

Copy link
Copy Markdown
Contributor Author

Addressed the review note in the latest push.

The ACP metadata confirmation is now best-effort: isConfirmedAcpManualSpawnTaskOwner catches metadata read/open/query failures, logs a warning, and returns false, which falls back to normal CLI task tracking instead of blocking the manual-spawn child turn.

I also added the requested regression: keeps dispatch and CLI tracking when ACP metadata read fails. It mocks readAcpSessionMeta to throw for an ACP manual-spawn child turn, verifies dispatch still reaches agentCommand, verifies the CLI task row is created/finalized, and verifies the fallback warning is logged.

Local proof after the change:

pnpm test src/gateway/server-methods/agent.test.ts -t "ACP manual-spawn child turn task tracking"
# 2 files passed; 10 tests passed

pnpm tsgo:test
# passed

npx oxfmt --check --threads=1 src/gateway/server-methods/agent.ts src/gateway/server-methods/agent.test.ts
# passed

@moeedahmed

Copy link
Copy Markdown
Contributor Author

@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 readAcpSessionMeta path. The PR body has been updated with the focused test, type-check, formatting, and runtime smoke proof; the Real behavior proof check is passing on this head.

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jun 27, 2026
@moeedahmed
moeedahmed force-pushed the fix/acp-manual-spawn-task-tracking branch from 3ded77d to a428638 Compare June 27, 2026 02:19
@moeedahmed

Copy link
Copy Markdown
Contributor Author

Addressed the remaining source-review blocker in the latest push and requested re-review.

What changed:

  • Added isTrustedBackendAcpSpawnClient, gating CLI-row suppression to the in-process backend ACP spawn caller: gateway-client + backend, excluding device-token clients.
  • Kept the existing best-effort metadata-read fallback.
  • Added the requested non-backend/operator-write regression: an openclaw-control-ui operator-write caller that sets acpTurnSource: "manual_spawn" on an ACP session still dispatches and creates/finalizes the CLI row, because it does not own a replacement ACP task row.

Local proof:

pnpm test src/gateway/server-methods/agent.test.ts -t "ACP manual-spawn child turn task tracking"
# 2 files passed; 12 tests passed

pnpm tsgo:test
# passed

npx oxfmt --check --threads=1 src/gateway/server-methods/agent.ts src/gateway/server-methods/agent.test.ts
# passed

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@moeedahmed

Copy link
Copy Markdown
Contributor Author

@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 cli_rows = 0 for the same smoke label. CI is green, including the long scan.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jun 27, 2026
@vincentkoc
vincentkoc merged commit a9eb0d7 into openclaw:main Jun 29, 2026
134 of 139 checks passed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 29, 2026
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gateway Gateway runtime P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants