feat(runners): buildExecutionPlan emits snapshot carry-over (#878 A2-3-runners)#933
Merged
Conversation
…3-runners)
Per docs/development/a2-3-replay-execution-design.md A2-3-runners slice:
populate the carry-over snapshot in buildExecutionPlan output so a future
A2-3-impl slice can attach it to artifact.debug.execution.snapshot for
--plan replay execution.
Additive shape:
- New top-level `snapshot: { fileTypes, relatedADRs, reviewMode, riskAssessment }`
- New top-level `riskAssessment` (was previously computed but never returned,
silently breaking `plan.riskAssessment` consumers; #877 silent-skip
taxonomy applies)
The empty-selection early return now also carries the snapshot (with
relatedADRs: [], reviewMode: null) so consumers can record the audit trail
even when no skills run.
Action bundle rebuilt (npm run build:action on Node 22) since cli.mjs
transitively imports review-runner.
Next slice: A2-3-impl wires src/lib/review-plan.mjs to copy plan.snapshot
into artifact.debug.execution.snapshot AND the replay path (runReviewExecReplay)
to read it back. That requires src/lib/ change; AGENTS.md "Ask before editing"
gate applies.
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
River Reviewer
選択されたスキル (0)
スキップされたスキル (88)
優先度サマリー
スコア (参考値)結果(スコア): 96/100 内訳:
指摘
|
Contributor
PlanGate Review
PlanGate review decision: pass
ポリシー: critical=fail / major=fail-if-required (warn) / minor=comment-only / info=skipped — spec: |
5 tasks
s977043
added a commit
that referenced
this pull request
May 28, 2026
…pl) (#935) Final functional slice of #878. runReviewExecReplay now executes the source plan's selectedSkills against the current diff instead of echoing findings: []. Per docs/development/a2-3-replay-execution-design.md: - Replay does NOT re-plan: selectedSkills come verbatim from the source plan. - Diff is resolved from the current working tree / --artifact, NOT snapshotted in the plan (intentional — avoids artifact bloat). - Carry-over context (fileTypes / relatedADRs / reviewMode / riskAssessment) is read from the source plan's debug.execution.snapshot (#933 A2-3-runners). When the source predates the snapshot, generateReview uses engine defaults (graceful degradation, not silent skip). - debug.execution.replaySnapshotUsed records whether the carry-over was present. Behavior matrix: - exec --plan <p> --artifact diff=... → executes, findings populated - exec --plan <p> (no diff) → echo contract (findings: []) - exec --plan <p> --dry-run → echo only, no execution #927's todo-marked failing test is now a passing integration test (3 cases: execution reach, no-diff echo fallback, dry-run echo). Action dist rebuilt (Node 22) — cli.mjs transitively bundled. drift detection (debug.replay.drift) is deferred to a follow-up per Codex slice judgment; this PR delivers execution + carry-over.
This was referenced May 28, 2026
s977043
added a commit
that referenced
this pull request
May 28, 2026
…on (#939) The 2026-05-25..28 retrospective was written mid-session (#930). This addendum captures the remaining arc (#878 epic, #868/#921/#929 closeout) and the one durable lesson worth persisting: "completed" report ≠ wired-through. #877 claimed to fix the riskAssessment silent-skip but only wired the consumer side; the producer never emitted the value, so plan.riskAssessment was always undefined. Found during A2-3-runners (#933). New P1 rule: a silent-skip fix needs a test asserting the value is RETURNED by the producer AND consumed — not just "forwarded if present". A "forwards X" test passes whether X is real or undefined. Also records: the 4-slice design-doc-first delivery shape (#878) and the design-only discipline held twice (#910, #938). Full session totals: 38 PRs / 13 releases / 4 issues closed / 5 opened.
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
Third slice of #878 (after A2-3-pre #922 schema and A2-3-test #927 todo-marked failing test). Populates the carry-over snapshot in
buildExecutionPlanoutput. The artifact-side wiring (readplan.snapshot, writeartifact.debug.execution.snapshot) is the next slice (A2-3-impl); this PR only adds the producer.Per the design doc (#910): A2-3-impl flips the
{ todo: true }test #927 to a regulartest()when it lands.Changes —
runners/core/review-runner.mjssnapshot: { fileTypes, relatedADRs, reviewMode, riskAssessment }in both planner and no-planner return branches.riskAssessmentfield — previously computed in line 213 but never returned (silent-skip per fix(cli): propagate riskAssessment on review exec path (#802 Phase 3 A2-fix-4) #877 taxonomy;plan.riskAssessmentconsumers always sawundefined).snapshotandfileTypes/riskAssessmentso audit-trail consumers can record context even when no skill matched.Dist rebuild
runners/github-action/dist/index.mjsrebuilt on Node 22 becauserunners/github-action/src/index.mjstransitively importsrunners/core/review-runner.mjsviasrc/cli.mjs. Avoids theAction dist freshnessCI failure pattern from #917.Test plan
node --test tests/review-runner-snapshot-field.test.mjs→ 4/4 passnpm test→ 1110/1110 pass (4 new tests, no regression)npm run skills:validate:promptfoocleannode scripts/fix-dashes.mjs --checkcleannpm run build:actionon Node 22 to rebuild distCodify-then-validate (per AGENTS.md from #904)
3 failure scenarios:
plan.{fileTypes,relatedADRs,reviewMode}at top level → unchanged, additive only.riskAssessmentandsnapshotare added; Object.freeze / type-strict consumers would need updating. Searched — none today.snapshotobject holds references to the same arrays as the top-level fields, not copies. Mutation by a consumer would change both. Documented inline; consumers should treat output as read-only (existing convention).Reopen / exception: if A2-3-impl decides the snapshot needs to be deep-cloned, that's a one-line change in
src/lib/review-plan.mjsat copy time.Gray zone: when no skills selected, the snapshot is partially synthetic (
relatedADRs: [],reviewMode: null). Documented inline; consumers treating these asunknownrather thanemptyshould branch onplan.selected.length === 0instead.Out of scope
src/lib/review-plan.mjschange which isAsk before editing.