fix(cli): propagate availableContexts to plan layer (#802 Phase 3 A2-fix-1)#865
Merged
Merged
Conversation
…A2-fix-1) runReviewPlan never passed availableContexts to buildExecutionPlan, so all skills with inputContext: ['diff'] landed in skippedSkills silently. - Add resolveEffectiveContexts() helper: defaults to ['diff'], merges RIVER_AVAILABLE_CONTEXTS env var (mirrors local-runner.mjs pattern) - Thread availableContexts param through runReviewPlan signature with JSDoc - Pass effectiveAvailableContexts into buildExecutionPlanImpl call - Forward parsed.availableContexts from CLI --context flag to runReviewPlan Closes #802 (A2-fix-1 slice)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Comment on lines
+233
to
+237
| function resolveEffectiveContexts(inputContexts) { | ||
| const envContexts = parseList(process.env.RIVER_AVAILABLE_CONTEXTS); | ||
| const base = inputContexts?.length ? inputContexts : ['diff']; | ||
| return [...new Set([...base, ...envContexts])]; | ||
| } |
Contributor
There was a problem hiding this comment.
resolveEffectiveContexts のロジックが src/lib/local-runner.mjs 内の resolveAvailableContexts と重複しているようです（コメントにも記載があります）。将来的なロジックの乖離を防ぎメンテナンス性を向上させるため、この共通ロジックを src/lib/utils.mjs などの共通ユーティリティに抽出し、両方のエントリポイントから再利用することを検討してください。
| */ | ||
| function resolveEffectiveContexts(inputContexts) { | ||
| const envContexts = parseList(process.env.RIVER_AVAILABLE_CONTEXTS); | ||
| const base = inputContexts?.length ? inputContexts : ['diff']; |
Contributor
There was a problem hiding this comment.
この実装では、CLI引数（inputContexts）が提供されるとデフォルトの 'diff' が完全に上書きされます。runReviewPlan が実行される際は常に diff アーティファクトが解決されているため、ユーザーが --context tests のように他のコンテキストを追加指定した場合でも、'diff' コンテキストは自動的に維持される（または常に含まれる）方が、スキルのスキップを防ぐ観点では直感的かもしれません。現状の実装では、ユーザーは明示的に --context diff,tests と指定する必要があります。
Suggested change
| const base = inputContexts?.length ? inputContexts : ['diff']; | |
| const base = [...new Set(['diff', ...(inputContexts ?? [])])]; |
Contributor
River Reviewer
選択されたスキル (1)
スキップされたスキル (82)
優先度サマリー
スコア (参考値)結果(スコア): 100/100 内訳:
指摘No findings. |
Contributor
PlanGate Review
PlanGate review decision: pass
ポリシー: critical=fail / major=fail-if-required (warn) / minor=comment-only / info=skipped — spec: |
Picks up the new resolveEffectiveContexts helper and the additional availableContexts argument forwarded into buildExecutionPlan via the runReviewPlan path. No source/contract changes — bundle only. Built with Node 22.22.2 per .nvmrc. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
s977043
changed the title
- Move resolveAvailableContexts() to src/lib/utils.mjs as the single source of truth. local-runner.mjs now imports the shared helper. - Add alwaysInclude option to the helper. review-plan.mjs uses alwaysInclude: ['diff'] inside the diff-resolved branch so a narrow CLI override like `--context tests` no longer drops 'diff' from the effective set (would have re-introduced the A1 silent-skip failure Gemini flagged in its second comment). - New regression test: --context tests still ends up with both 'diff' and 'tests' in the buildExecutionPlan args. No behavior change for local-runner (it never passed alwaysInclude). 1076/1076 tests green. dist rebuilt with Node 22.22.2. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
s977043
added a commit
that referenced
this pull request
May 21, 2026
…Deferred D3: docs/review/troubleshooting.md — documents the silent-skip problem fixed in A2-fix-1 (v0.51.0): river review exec returned empty findings when availableContexts was not forwarded to buildExecutionPlan. Covers diagnosis with --debug, the fix, and RIVER_AVAILABLE_CONTEXTS usage. D2: docs/deprecated.md — marks artifact.executionDeferred as deprecated in v0.51.0 (planned removal v0.53.0) and points to plan.skippedSkills as the replacement. Closes documentation gap for PR #865 (A2-fix-1). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
s977043
added a commit
that referenced
this pull request
May 21, 2026
…cation (D2+D3) (#866) * docs: add skill-selection troubleshooting guide + deprecate executionDeferred D3: docs/review/troubleshooting.md — documents the silent-skip problem fixed in A2-fix-1 (v0.51.0): river review exec returned empty findings when availableContexts was not forwarded to buildExecutionPlan. Covers diagnosis with --debug, the fix, and RIVER_AVAILABLE_CONTEXTS usage. D2: docs/deprecated.md — marks artifact.executionDeferred as deprecated in v0.51.0 (planned removal v0.53.0) and points to plan.skippedSkills as the replacement. Closes documentation gap for PR #865 (A2-fix-1). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * docs: tighten executionDeferred deprecation + troubleshooting - Fix the deprecation narrative for `debug.executionDeferred`: the replacement is the new `debug.execution` trace (introduced in v0.51.0 / A2-1), not `plan.skippedSkills`. Migration snippet now shows both the execution trace check and the skippedSkills check side by side. - Fix the heading anchor: the deprecation header is now `output-artifact-field-debugexecutiondeferred-v0510`. - In troubleshooting.md, code-format every reference to skill metadata (`inputContext: ['diff']`, `--context`, schema field names) so the prose does not look like Markdown links. - Fix the relative path to the CLI spec (`../../pages/...` from `docs/review/`). - Note explicitly that the v0.51.0 `alwaysInclude: ['diff']` semantics protect the diff context from being stripped by `--context tests`. No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
4 tasks
s977043
added a commit
that referenced
this pull request
May 21, 2026
… A2-fix-2) (#869) Same silent-skip pattern as A2-fix-1: runReviewPlan never forwarded `availableDependencies` to buildExecutionPlan, so any skill that declared a dependency (e.g. `code_search`, `test_runner`) was always selectable regardless of whether the dependency was actually wired in the runtime. What changed - Move resolveAvailableDependencies + dependencyStubs to src/lib/utils.mjs so the helper is now shared between local-runner and review-plan (mirrors the A2-fix-1 split of resolveAvailableContexts). - runReviewPlan accepts a new availableDependencies option and forwards the resolved value (or null = disabled sentinel) to buildExecutionPlan. - CLI passes parsed.availableDependencies through so `--dependency` and RIVER_AVAILABLE_DEPENDENCIES / RIVER_DEPENDENCY_STUBS=1 work on the exec path. - 4 new unit tests cover null default, explicit arg, env var, and stubs. - runners/github-action/dist rebuilt with Node 22.22.2. Out of scope (Codex silent-skip list, follow-ups remaining): - fileTypes / relatedADRs / reviewMode propagation - --plan replay context-snapshot drift Follows: #864 (A2-1) and #865 (A2-fix-1). Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
4 tasks
s977043
added a commit
that referenced
this pull request
May 21, 2026
…802 Phase 3 A2-fix-3) (#871) Continue the Codex silent-skip cleanup after #865 (availableContexts) and #869 (availableDependencies). The plan layer's `buildExecutionPlan` already derives fileTypes, relatedADRs, and reviewMode (review-runner.mjs:212-217), but runReviewPlan never read them off the plan return and never forwarded them to generateReview. Impact (silent, no error) - fileTypes: generateReview's verifier file-phase coherence check fell back to "lenient when fileTypes not provided". On exec, phase mismatch checks were effectively disabled. - relatedADRs: ADR cross-references were never injected into the prompt on the exec path. Skill outputs lost the "ADR-001 says X" anchors that `river run` produced. - reviewMode: the diff-size-driven context budget preset was always the generateReview default rather than the calibrated 'tiny|medium|large' the plan layer determined. Fix - Pull `plan.fileTypes`, `plan.relatedADRs`, `plan.reviewMode` off the buildExecutionPlan return and pass them to generateReview alongside the existing diff/plan/phase/config. Optional chaining (`?? undefined`) keeps backward-compat: tests that mock buildExecutionPlan with a minimal shape continue to work. Tests - 2 new unit tests in tests/cli-review-plan.test.mjs cover the forward path and the undefined-fallback path. - runners/github-action/dist rebuilt with Node 22.22.2. Follows: #864 (A2-1), #865 (A2-fix-1), #869 (A2-fix-2). Remaining silent-skip risks - --plan replay context-snapshot drift (deferred to A2-3). Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
4 tasks
s977043
added a commit
that referenced
this pull request
May 22, 2026
…872) * docs(troubleshooting): close out silent-skip cleanup through v0.51.1 Codex-recommended docs follow-up after the v0.51.1 release. The existing troubleshooting page only documented the v0.51.0 availableContexts fix; v0.51.1 closed two more silent-skip failure modes (#869 availableDependencies, #871 fileTypes/relatedADRs/reviewMode) that were not yet reflected. What this adds - "Skill appears in skippedSkills with reason missing dependency": new section describing the v0.51.1 fix and the --dependency / RIVER_AVAILABLE_DEPENDENCIES / RIVER_DEPENDENCY_STUBS=1 knobs. - "Finding output looks generic / missing ADR references": new section explaining the three derived-context fields the exec path was silently dropping, and confirming the fix is automatic in v0.51.1+. - "Where the plan layer forwards each context": small reference table that maps each forwarded field to the PR / release that wired it. - "Residual gap: --plan replay does not yet run skills": explicit pointer to the remaining #802 A2-3 work so adopters know the boundary of v0.51.x. No code changes. Follows: #864 (A2-1), #865 (A2-fix-1), #869 (A2-fix-2), #871 (A2-fix-3). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * docs: link the #802 issue reference (Gemini PR #872 nit) Make the introductory mention of #802 a real GitHub link, matching the style of the residual-gap section. No content change. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
4 tasks
s977043
added a commit
that referenced
this pull request
May 22, 2026
…A2-fix-4) (#877) The final remaining silent-skip identified by the multi-perspective session review (Codex + Gemini): runReviewPlan was hard-coding riskMap: undefined when calling buildExecutionPlan, so the riskAssessment that buildExecutionPlan would have produced never appeared on the artifact, and never reached generateReview's prompt on the exec path. What changed - runReviewPlan loads the optional risk map via loadRiskMap (default path .river/risk-map.yaml). Missing file => null (no behaviour change). Malformed file => ReviewPlanError so it fails loudly instead of dropping the risk signal silently. - The loaded riskMap is forwarded to buildExecutionPlan, which already knew how to compute riskAssessment when given a non-null riskMap. - plan.riskAssessment is forwarded to generateReview alongside fileTypes / relatedADRs / reviewMode (the analysis-context bundle finalised in A2-fix-3). - loadRiskMapImpl is injectable so tests can drive the path without a fixture file on disk. Tests - 5 new unit tests cover happy path forwarding, null sentinel, load failure, generateReview hand-off, and the undefined-fallback when the plan omits riskAssessment. - runners/github-action/dist rebuilt with Node 22.22.2. Silent-skip cleanup status (post-A2-fix-4) - availableContexts (#865) - availableDependencies (#869) - fileTypes / relatedADRs / reviewMode (#871) - riskAssessment (this PR) - --plan replay context-snapshot drift is the only remaining item, deferred to A2-3. Multi-perspective review reference - Codex 86/100: "riskAssessment が runReviewPlan で riskMap: undefined 固定で実質ドロップ" 指摘 - Gemini 95/100: "riskAssessment の伝播漏れのみが唯一の明確な減点 対象" - 両者が独立に同じ点を 6 番目の silent-skip として指摘した。 Follows: #864 (A2-1), #865 (A2-fix-1), #869 (A2-fix-2), #871 (A2-fix-3). Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
This was referenced May 22, 2026
This was referenced May 27, 2026
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
Close the silent-skip gap discovered when dogfooding A2-1 (#864): every skill that declared
inputContext: ['diff']was being filtered out bybuildExecutionPlanbecauserunReviewPlannever forwardedavailableContexts. The plan layer now defaults to['diff']whenever a diff artifact is resolved and merges inRIVER_AVAILABLE_CONTEXTSplus an optional caller argument / CLI--contextflag.Why this slice
Codex flagged silent-skip as a higher-impact risk than the LLM call itself: failures look like success, demos go empty. The A2-1 dogfood reproduced this — 0 selected skills with a 290-line diff because
inputContext: diffskills were unconditionally skipped.After this fix, the same dogfood selects 5 midstream skills end-to-end:
{ "status": "ok", "selected_count": 5, "selected_ids": [ "rr-midstream-logging-observability-001", "rr-midstream-review-automation-boundary-001", "rr-midstream-gh-address-comments-001", "rr-midstream-hello-skill-001", "rr-midstream-review-comment-triage-001" ], "exec_trace": { "skillsExecuted": 5, "heuristicsUsed": true, "...": "..." } }Changes
Out of scope (follow-up silent-skip risks)
Codex flagged the same pattern in:
These should be handled the same way once the use case demands them.
Test plan
Follows: #864 (A2-1 generateReview adapter).
🤖 Generated with Claude Code