Skip to content

fix(shell): reconcile task updates by origin - #432

Open
zhuowp wants to merge 6 commits into
Pinvou:mainfrom
zhuowp:fix/shell-task-origin-reconciliation
Open

fix(shell): reconcile task updates by origin#432
zhuowp wants to merge 6 commits into
Pinvou:mainfrom
zhuowp:fix/shell-task-origin-reconciliation

Conversation

@zhuowp

@zhuowp zhuowp commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • carry stable origin tool-call and turn identities from CodeWhale shell dispatch into job snapshots and completion events
  • reconcile snapshots to the exact originating tool card in both app bridges
  • use origin identity before command text in the Rust shell monitor while retaining a safe legacy fallback
  • keep origin-identified running jobs visible after reload while preventing unmatched terminal root jobs from being appended to the current timeline
  • update the fork register, guard fingerprints, and public submodule verifier for the r14 candidate

Dependencies and stacking

This PR targets main as requested. Until #431 merges, its comparison also contains the phase-1 commit; after #431 merges, GitHub will automatically reduce this PR to the phase-2 delta.

Compatibility

The new origin fields are optional for existing serialized jobs. Older runtimes continue through the command-match fallback, but an explicit different origin is never rebound by matching command text. No revision counter or ambiguous generic tool-result metadata is introduced.

Verification

  • node --test tests/shell_task_projection.test.mjs (10 passed)
  • python -m unittest scripts.tests.test_ci_gate_policy (20 passed)
  • python scripts/architecture-guard.py
  • ./scripts/fork-guard.sh --fast
  • CodeWhale cargo check -p codewhale-tui --lib --locked
  • focused CodeWhale origin, completion-event, runtime-handoff, legacy-JSON, and evidence tests
  • git diff --check

@zhuowp
zhuowp force-pushed the fix/shell-task-origin-reconciliation branch 2 times, most recently from 7f7e1b7 to b5b6f41 Compare September 4, 2026 04:48
@asto18089
asto18089 force-pushed the fix/shell-task-origin-reconciliation branch from b5b6f41 to a192092 Compare September 4, 2026 06:04

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent re-review

Audited against latest main (6195d704a, which is exactly this branch's merge-base — no rebase needed), including the stacked phase-1 delta (a445e02b6) and the CodeWhale candidate 5a5bf363e (fork PR #40). Verified: the root cause on main (assign_unclaimed_tasks binding purely by job.command == command; worse, a completed foreground job lingers in list_jobs for its retention window, so a later same-command tool replays the entire historical buffer as one delta onto the wrong card); the four-layer identity chain (engine plan.id → forwarder Event::ToolCallStarted.id → host MonitorState key → JS card toolId); all three engine dispatch stamps incl. approval retry and parallel batch; the legacy fallback predicate being byte-identical to main for origin-less jobs (walked every origin-less producer: verifier gates, stale remember_job, serde-restored payloads — all behave exactly as main); serde default/skip_serializing_if compatibility; TUI display unchanged; zero unrelated changes in either phase. Local gates green: fork-guard.sh --fast, architecture-guard.py, node --test tests/shell_task_projection.test.mjs (7/7), python -m unittest scripts.tests.test_ci_gate_policy (20/20); cargo clippy --lib --tests -D warnings clean; targeted engine tests pass. The fast-gate failure here is the declared tag-publication dependency, not a defect.

The fix is real, meaningful, and elegant. Requesting changes on one behavioral decision and one register-accuracy fix:

1. MAJOR — the phase-2 tail-suppression guard also hides running root jobs (needs author decision)

pinvou3-app/src/platform/tauri/bridge/terminal.js:159, mirrored at pinvou3-app/src/platform/web/bridge.js:4435:

if (!item && job.origin_tool_call_id && !job.owner_agent_id) return;

Two problems:

(a) The comment's premise doesn't hold. "The job belongs elsewhere" is not possible: list_shell_tasks is queried per session (src-tauri/src/app/commands/runtime.rs:109-113) and applyShellSnapshots only ever receives this session's jobs. An unmatched job always belongs to this session; its origin card is missing because the card left the loaded transcript (compaction rewrote the persisted history, then reload), not because it lives elsewhere.

(b) Unlike the phase-1 guard at terminal.js:154, this one also suppresses running snapshots. Concrete loss: a root background job outlives a compaction that rewrote the persisted transcript; after a webview reload (Tauri keeps the Rust process, jobs and monitor, alive) or a page reload on the web bridge, re-hydration cannot restore the origin card, and the still-running job becomes invisible — the monitor emission handlers (chat:tool_deltaappendToolItemOutput, chat:shell_task_statusfinishBackgroundToolItem) only ever update existing cards and never append. On main it got a live-updating synthetic card. A running job is never a "stale" card.

If suppressing running identified jobs is intentional position-purity, the comment should state that tradeoff; otherwise exempting running — if (!item && !running && job.origin_tool_call_id && !job.owner_agent_id) return; — restores main's visibility at zero stale-card cost. Note this flips the harness test identified root jobs without a loaded origin card are never appended at the tail, which currently pins running suppression, so this is a genuine design decision rather than a mechanical fix. Either resolution works; please pick one.

2. MAJOR (register accuracy) — guard inventory undercounts the r14 candidate

Both docs/fork-modifications.md (守护行, and the "r14 候选守护" bullet at line 162) and docs/fork-modifications.en.md say the candidate adds "one Shell-origin behavior test". Measured fn forkguard_ count: 63 → 65, because (1) background_shell_job_carries_subagent_owner is promoted/renamed to forkguard_background_shell_job_preserves_origin_identity, and (2) a second new test forkguard_tool_context_for_call_preserves_turn_and_sets_call_origin lands in crates/tui/src/core/engine/turn_loop.rs — that one is absent from both docs' claims and from the T2 guard enumeration. The register is the declared single source of truth; please correct the count and enumerate the turn_loop test in both languages.

Non-blocking notes

  • scripts/fork-guard.sh layer 0 now dual-accepts the r13 head and the candidate, but the fingerprint layer unconditionally requires candidate-only content, so an r13 checkout passes layer 0 and fails layer 1. The #370 precedent replaced the accepted head outright (single accept). Harmless post-merge, but the script advertises a state it can never fully validate.
  • The r14-candidate bullet is filed under the "### r12 厂商原生搜索…" heading in both register docs, and the drift row records no candidate numbers (measured: 7 files, +133/−7).
  • Optional test gap: mixed precedence in assign_unclaimed_tasks (one tool with no origin match + snapshots containing both a foreign-origin and a None-origin job of the same command) is unpinned; two small tests would catch a future "relax the fallback to any job" regression.
  • Merge-order constraint is correctly declared and enforced by the gate: CodeWhale #40 → immutable tag pinjou-v0.9.5-r14 → this PR. Merging out of order would redden every open PR's fast-gate.

zhuowp and others added 2 commits September 4, 2026 16:41
Signed-off-by: zhuowp <zwp-1989@qq.com>
The guard re-evaluated the latest shell card for every snapshot, so a
running job's synthetic card created earlier in the same poll (the
manager lists running jobs first) disarmed it for the completed jobs
after it. Decide once from the pre-poll timeline instead, document the
accepted limits until origin identity lands, and pin the guard, the
web helper's scan predicate, and both documented limit scenarios in
tests.

Signed-off-by: asto <asto18089@126.com>
@zhuowp
zhuowp force-pushed the fix/shell-task-origin-reconciliation branch from a192092 to 4f7bbb7 Compare September 4, 2026 08:46
@zhuowp

zhuowp commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review findings and updated the stack on top of the current #431 head (62a3db2fc).

  • Origin-identified running root jobs remain visible through a synthetic card after compaction/reload; only unmatched terminal root jobs are suppressed from the current tail.
  • Added projection tests for the running/terminal split and a mixed foreign-origin/legacy-origin monitor regression.
  • Corrected the r14 fork inventory in both languages: 7 files, +133/-7, two new forkguard tests, 65 total, and a separate r14 candidate heading.
  • Made the fast fork guard require the r14 candidate consistently and fingerprint both CodeWhale origin-identity guards.

Validation:

  • node --test pinvou3-app/tests/shell_task_projection.test.mjs: 10/10 passed.
  • python -m unittest scripts.tests.test_ci_gate_policy: 20/20 passed.
  • python scripts/architecture-guard.py: passed.
  • cargo fmt --all -- --check: passed.
  • scripts/fork-guard.sh --fast: passed.
  • The targeted Rust test did not reach the test runner locally: Windows rustc hit STATUS_STACK_OVERFLOW while compiling codewhale-tui; a larger-stack retry passed that point but the local runner did not terminate cleanly. CI remains the authoritative Rust run.

Please re-review the updated head.

@zhuowp
zhuowp requested a review from asto18089 September 4, 2026 08:48
asto18089 and others added 4 commits September 4, 2026 17:16
The stale shell card guard recognized wait observers by tool name only.
Since engine v0.9.3 the exec_shell_wait/exec_wait names survive only in
replayed legacy sessions and the canonical wait observer is the Bash
tool with action="wait" (the engine's own timeout-recovery hint prefers
it), so the guard never engaged on the primary path and a retained
completed job was still appended below the newer wait result. Recognize
the action on the card args (present both live and after history
replay) in both bridges, cover it with regression and start-tool
boundary tests, and lock the web parity clause.

Signed-off-by: asto <asto18089@126.com>
Signed-off-by: zhuowp <zwp-1989@qq.com>
Signed-off-by: zhuowp <zwp-1989@qq.com>
- enumerate the r14 candidate commit in the English register's T2 list,
  matching the authoritative Chinese register
- make the subagent-named projection test actually set owner_agent_id and
  pin the owned-job visibility branch beside the root-job suppression test
- refresh the stale "until stable origin identity lands" limits comment in
  both bridges: origin identity lands in this same change

Signed-off-by: asto18089 <asto18089@126.com>
@asto18089
asto18089 force-pushed the fix/shell-task-origin-reconciliation branch from 4f7bbb7 to 2bcdfb7 Compare September 4, 2026 16:53
@asto18089

Copy link
Copy Markdown
Collaborator

Second-round re-review — prior findings resolved, branch re-stacked, small fixes pushed

Re-audited the updated head against latest main (546d8328), with re-verification split across three focused passes (Rust monitor, JS bridges + projection tests, submodule/register/scripts).

Prior MAJOR findings — both verified fixed

  1. Running-job suppression: both bridges now exempt running from the tail-suppression guard (terminal.js, bridge.js), the comment's premise is corrected, and the exhaustively-walked guard table confirms neither failure mode this PR targets is reachable: a running origin-identified root job keeps a live synthetic card after compaction/reload, and an unmatched identified terminal root job is never appended.
  2. Register inventory: both registers now state 63 → 65 with the two Shell-origin tests enumerated (including the turn_loop one); measured forkguard_ count is exactly 65, the r14 section has its own heading, and the drift row matches the measured 7 files, +133/-7 byte-for-byte.

Re-verified independently: the four-layer origin chain, the legacy fallback predicate remaining filter-for-filter identical to main for origin-less jobs (foreign origin is never command-text-rebound, pinned by two tests), serde compatibility in both directions, byte-identical applyShellSnapshots across the two bridges, zero unrelated changes in the 12-file diff, and no interaction with the newly merged #429/#426. Local gates green: fork-guard.sh --fast, architecture-guard.py, python -m unittest scripts.tests.test_ci_gate_policy (20/20), node --test tests/shell_task_projection.test.mjs, cargo test --lib shell_output / turn_shell_tasks.

Re-stack onto the current #431 head

This branch was still stacked on the old 62a3db2f, but #431 has since rewritten its phase-1 commits and added 22e5e9fd (arm the guard for Bash action=wait, the canonical observer). On the old stack the declared "GitHub will automatically reduce this PR to the phase-2 delta" no longer held, and the guard stayed disarmed on the primary wait path. Re-based the branch: phase-1 commits now shared with #431 (base 8102bf7e; main's extra #429 commit touches only platform/process.rs, disjoint from this PR), phase-2 cherry-picked on top — the only conflict was one 5-line assert.match hunk in the projection tests, both sides kept.

Fixes pushed in 2bcdfb7f

  • docs/fork-modifications.en.md: the T2 commit enumeration now includes the r14 candidate 5a5bf363e…, matching the authoritative Chinese register.
  • tests/shell_task_projection.test.mjs: the subagent-named test now actually sets owner_agent_id (it silently exercised the origin-less branch before), and a new test pins that an identified completed subagent job without its origin card stays visible while the root counterpart stays suppressed.
  • Both bridges: refreshed the stale "until stable origin identity lands" limits comment — origin identity lands in this same change; the comment now scopes the residual race to subagent-owned and legacy origin-less jobs.

Projection tests are 13/13 after these changes.

Non-blocking notes (no action required in this PR)

  • isShellExecutionTool's name set still diverges between the bridges (wait-tool-inclusive in Tauri only) — pre-existing on main, dormant now that origin lookup precedes, unpinned on the web side.
  • originToolCallId/originTurnId stored on cards currently have no consumers; the origin lookup adopts any tool card by id without a shell-name check (fine under the trusted local host).
  • Neither register mentions that forkguard_background_shell_job_preserves_origin_identity is a renamed+extended r13-era test; the 63→65 arithmetic stays exact either way.
  • Merge order is unchanged and enforced by the gate: CodeWhale #40 → tag pinjou-v0.9.5-r14#431 → this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants