fix(#484): terminal completion persistence + child-contract invalidation - #519
Merged
richard-devbot merged 3 commits intoJul 31, 2026
Merged
Conversation
…ion for the Business Hub
completed_at was never written anywhere in the codebase — only ever read by
deriveRunStatus/statusFromEntry, which key their "done" classification on it,
not on manifest.status. The only writer of status: "DONE" was sdlc_status's
handler, fired opportunistically, so a finished run that nobody polled
afterward stayed IN_PROGRESS forever and eventually classified "stalled".
markRunCompleted() now stamps status: "DONE" and completed_at atomically
(one read-modify-write under the manifest's own lock) at the actual moment
of completion — the end of sdlc_validate's success path — and is idempotent,
so sdlc_status's original opportunistic check remains safe as a defensive
catch-up without ever moving an already-terminal run's timestamp.
runSignature() covered manifest/events/tasks/approvals/evidence + the run
dir's own mtime, but never tasks/<taskId>/{builder,validation,prompt} files —
a retry overwrites builder.json in place, which doesn't bump the parent
directory's mtime on POSIX filesystems, so a cached "unchanged" entry could
keep serving a stale verdict. childContractsSignature() closes this.
Investigated but deliberately NOT changed: attempted to also make the
rollup-index's general pipeline rollup rebuild on pipelineStateEventsBehind
(matching attachPipelineRollups' repair branch) — this regressed an existing,
deliberate test pinning the opposite contract (the general rollup must show
honest staleness, not rebuild on every 3s poll for every active run). Caught
by this PR's own verification pass and reverted; reasoning recorded in
computeRunPipelineRollup's doc comment and docs/HARNESS.md.
Deferred (documented in docs/HARNESS.md): the full state_generation/outbox
subsystem, multi-instance WebSocket delivery and reconnect-cursor
reconciliation, and a degraded/error surface at the generation layer — none
of these primitives exist anywhere in the codebase today; building them is a
separate, much larger subsystem than this slice.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
Contributor
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix terminal completion persistence + child-contract invalidation (#484)
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+3605
to
+3613
| if (status === "PASS" && !manifest.completed_at) { | ||
| try { | ||
| const freshTasks = JSON.parse(await readFile(tasksPath, "utf8")).tasks; | ||
| if (await isRunEligibleForCompletion(projectRoot, manifest, freshTasks)) { | ||
| await markRunCompleted(projectRoot, manifest.run_id); | ||
| } | ||
| } catch (completionError) { | ||
| console.error("Failed to mark run completed:", completionError); | ||
| } |
There was a problem hiding this comment.
5. Completion-error path untested 📜 Skill insight ▣ Testability
A new try/catch swallows failures when stamping run completion, but there is no test that forces this error branch to run and asserts the intended behavior.
Agent Prompt
## Issue description
The new error-handling path for completion stamping (`catch (completionError) { console.error(...) }`) has no test that triggers the failure path and verifies behavior.
## Issue Context
If `readFile(tasksPath)` / JSON parsing / `markRunCompleted()` fails, the code intentionally logs and continues. The compliance requirement asks for tests that trigger newly-added error-handling branches.
## Fix Focus Areas
- src/integrations/pi/rstack-sdlc.ts[3605-3613]
- tests/extension-terminal-completion-484.test.js[68-110]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
… nits Real correctness bug found by Qodo review of PR #519, confirmed against the actual sdlc_plan writer: childContractsSignature() joined a task's output_dir under runDir, but sdlc_plan writes output_dir as PROJECT-ROOT- relative (.rstack/runs/<runId>/tasks/<taskId>), not run-dir-relative — so in production every stat() targeted a nonexistent doubly-nested path and the whole signature was a silent no-op. The fix builds paths from the canonical <runDir>/tasks/<taskId>/ location using each task's id directly, with containment checking (defense in depth against a corrupted/tampered tasks.json carrying a traversal segment in id) — closes both the correctness bug and the related path-injection finding Qodo flagged separately. The regression test's fixture previously used a run-dir- relative fake output_dir that accidentally matched the buggy join and masked the bug entirely; now matches the real production shape, plus a new dedicated traversal-rejection test. Also addressed from the same review: removed a duplicated comment block in sdlc_status, replaced `any` with a narrow structural type in isRunEligibleForCompletion, cleaned up temp test directories + a magic- number test delay, and renamed a non-descriptive loop variable. Consciously not addressed: a test for the completion-stamp catch block's error-swallowing path. Triggering that exact mid-function race deterministically would require either timing-dependent hacks or exporting internal functions solely for test access — not proportionate for a three-line best-effort catch, and the finding was rated "Review recommended," not "Action required." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…w CI CI failed on Node 18.x only: "/api/state carries a strong ETag and revalidates 304 once settled" got 200 instead of 304. Not reproducible locally across 5 runs. The settle loop above this assertion breaks as soon as it observes two consecutive identical ETags, but nothing guarantees state stays stable in the gap between that check and the very next fetch — an async index-write from the next background poll cycle can still land in that window (the same class of race the loop's own comment documents for a prior Node-22 CI flake this test already survived once). #484's childContractsSignature adds more per-poll disk I/O (stat calls per task's child contract files), plausibly widening that window just enough to reproduce on the slowest CI runner. Retries the revalidation itself with the same pacing as the settle loop, re-observing the new ETag on a miss, instead of assuming post-loop stability holds instantaneously. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
Author
|
@strix-security please re-run |
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
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
1 participant
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
Closes the two confirmed, verifiable bugs from #484's audit finding; investigates and deliberately does NOT implement a third change that would have regressed an existing, deliberate test.
completed_atwas written nowhere in the codebase — only read byderiveRunStatus/statusFromEntry, which key their"done"classification on it. The only writer ofstatus: "DONE"wassdlc_status's handler, fired opportunistically. A finished run nobody polled afterward stayedIN_PROGRESSforever, eventually classified"stalled".markRunCompleted()now stamps both atomically at the actual moment of completion (end ofsdlc_validate's success path), idempotently.runSignature()never coveredtasks/<taskId>/{builder,validation,prompt}files. A retry overwritesbuilder.jsonin place, which doesn't bump the parent directory's mtime on POSIX — a cached "unchanged" index entry could keep serving a stale verdict.childContractsSignature()closes this.pipelineStateEventsBehind(matchingattachPipelineRollups' repair branch, per a literal reading of "the rollup index can preattach a pipeline rollup and bypass that fallback"). This regressedtests/dashboard-command-pages.test.js's pre-existing, deliberate test pinning the OPPOSITE contract — the general rollup must show honest staleness, not silently rebuild on every 3s poll for every active run. Caught by this PR's own verification pass; reverted before merge. Full reasoning incomputeRunPipelineRollup's doc comment anddocs/HARNESS.md.Deferred (documented in
docs/HARNESS.md): the fullstate_generation/outbox subsystem, multi-instance WebSocket delivery + reconnect-cursor reconciliation, and a degraded/error surface at the generation layer. None of these primitives exist anywhere in the codebase today (confirmed by direct inspection before scoping) — building them is a separate, much larger subsystem than this slice.Test plan
npm run typecheck— cleannpm test— 1822/1822 passing, including 3 new mutation-checked tests intests/extension-terminal-completion-484.test.js(proven to fail without the fix) and 1 new mutation-checked test intests/dashboard-rollup-index.test.jsnpm run lint— 0 errors (4 pre-existing unrelated warnings)node scripts/security-audit.mjs— clean (1 pre-existing tolerated bundled advisory)npm run validate— 196/196 agents passgit diff --check— clean, nopackage-lock.jsondrift🤖 Generated with Claude Code