feat: implement issue #1151 — [Phase 3] Pilot scope + per-run cap + drift-closed verification gate - #1272
Conversation
…rift-closed verification gate
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe remediation script adds pilot-only live gating, per-run repository caps, and per-repository drift verification summaries. CLI flags expose pilot and cap settings, while Bats tests cover scope, deferral, verification, mismatch, skip, and validation behavior. ChangesRemediation controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant run_remediation
participant _remediate_repo
participant _emit_verification_summary
CLI->>run_remediation: provide pilot and max-repos settings
run_remediation->>_remediate_repo: process permitted repositories within cap
_remediate_repo-->>run_remediation: return remediation outcome
run_remediation->>_emit_verification_summary: emit per-repository verification results
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Phase 3 pilot-first rollout controls to the fleet stub remediation script, adding support for pilot repository gating, per-run caps, and drift-closed verification summaries. The feedback highlights portability issues with mktemp templates on macOS/BSD within the test suite, suggests optimizing the pilot helper functions to avoid spawning external processes and subshells, and recommends safer null-value handling in jq when generating the verification summary.
There was a problem hiding this comment.
Pull request overview
Implements Phase 3 of the fleet stub remediation driver (issue #1151) by adding a pilot-first live remediation gate, a per-run remediation cap, and a drift-closed verification summary, with corresponding Bats coverage to validate the new behavior.
Changes:
- Adds
REMEDIATE_PILOT_REPO/--pilotgating so live writes require an explicit pilot repo allowlist (otherwise fail-closed to dry-run). - Adds
REMEDIATE_MAX_REPOS/--max-reposto cap live remediations per run and log deferred repos. - Emits a per-repo drift-closed verification summary to logs and optionally a JSON artifact (
REMEDIATE_SUMMARY_FILE), and extends Bats tests accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/fleet_stub_remediate.sh | Adds pilot allowlist gating, per-run repo cap, and a drift-closed verification summary artifact/logging. |
| tests/fleet_stub_remediate.bats | Updates existing driver tests to use pilot scope, and adds Phase 3 tests for pilot gating, cap deferral logging, and verification summary output. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Superseded by automated re-review at
|
…0260715-1211 # Conflicts: # scripts/lib/shadow-compare.sh # scripts/shadow-run.sh
… portable mktemp, builtins Rebased onto main (resolved the stale shadow-mode file conflict — #1272 never modified those; took main's canonical versions) and addressed the 6 review findings on Phase 3: - [copilot] Validate REMEDIATE_MAX_REPOS is a non-negative integer before the numeric -ge cap comparison (empty/non-numeric would error under set -e); also handle mktemp failure explicitly. - [copilot] Idempotent skip (existing PR) now returns a distinct code and is recorded as status=skipped (drift_closed=false), NOT verified — the summary no longer reports drift-closed=yes for a run that verified nothing. Summary JSON now carries {repo, status: verified|skipped|failed, drift_closed} (true only for verified). - [gemini] _scope_list/_pilot_list/_in_scope/_in_pilot rewritten with pure-Bash parameter expansion — no tr/grep/subshells in run_remediation's per-repo loop. - [gemini] jq null-safe (.[0] // "" | tostring) in the summary artifact. - [gemini] Portable mktemp templates in the bats (trailing X, macOS/BSD-safe). Adds regression tests: idempotent-skip→status=skipped (not drift-closed=yes), and non-numeric cap fails loud. All 30 bats pass; shellcheck clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1272 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
|
@donpetry-bot re-review please — the last review was on the stale head All prior findings are resolved on the current head:
|
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 06a95f09b256439db7377e53a6e48dc647983479
Review mode: triage-approved (single reviewer)
Summary
Phase 3 pilot-first rollout controls for fleet stub remediation (#1151): a fail-closed pilot allowlist (REMEDIATE_PILOT_REPO/--pilot) gating live writes, a validated per-run cap (REMEDIATE_MAX_REPOS/--max-repos) with loud deferral logging, and a per-repo drift-closed verification summary (log + optional JSON artifact). Triage cleared this as low-risk; this confirmation review verified the triage assessment and that every finding from the prior cascade review (on stale head 1862bc1) is resolved on the current head. All CI green, all review threads resolved, all 5 acceptance criteria covered by new bats tests.
Linked issue analysis
Closes #1151 — [Phase 3] Pilot scope + per-run cap + drift-closed verification gate. All 5 acceptance criteria substantively addressed: (1) pilot allowlist restricts LIVE remediation to named repo(s), out-of-scope DRIFTED repos logged 'would remediate (out of pilot scope)' and skipped without writes; (2) REMEDIATE_MAX_REPOS (default 1) defers repos beyond the cap with explicit per-repo logging — no silent truncation, and the value is validated as a non-negative integer before the -ge comparison; (3) per-repo drift-closed verdicts (pushed blob SHA == canon) surfaced in the run log and an optional REMEDIATE_SUMMARY_FILE JSON artifact, with an idempotent skip correctly reported as status=skipped / drift_closed=false (never a false verified close); (4) with no pilot repo set the driver forces DRY_RUN=true and warns (fail-closed); (5) 8 new bats tests cover pilot include/exclude, cap enforcement + deferral, verification summary, mismatch, idempotent skip, and cap validation — shellcheck and bats CI checks pass on head. Change is additive to scripts/fleet_stub_remediate.sh + its bats suite, exactly the target surface named in the story.
Findings
All findings from the prior cascade review (sha 1862bc1) are resolved on head 06a95f0:
- RESOLVED (MAJOR): branch was CONFLICTING/DIRTY — main has been merged in; PR is now MERGEABLE (mergeStateStatus=BEHIND, which auto-rebase/merge handles).
- RESOLVED (MAJOR): shellcheck/bats CI missing from the rollup — shellcheck, ShellCheck, bats, unit-tests, and Lint all report SUCCESS on the current head.
- RESOLVED (MINOR ×2): non-portable mktemp template 'summary.XXXXXX.json' — all three test call sites now use mktemp "${BATS_TEST_TMPDIR}/summary.XXXXXX" (trailing X's, portable).
- RESOLVED (INFO): jq null-guard — summary builder now uses (.[0] // "" | tostring) / (.[1] // "" | tostring).
- Copilot findings also fixed: REMEDIATE_MAX_REPOS is validated fail-loud before the numeric comparison, and an already-open PR returns a distinct code (3) recorded as 'skipped', never drift_closed=true.
- All 6 review threads (gemini ×4, copilot ×2) are resolved.
- No new issues found in the diff since 1862bc1 (merge of main + one fix commit addressing the review). Blast-radius-reducing change: no secrets, auth, crypto, migrations, or injection surfaces. run_secret_scanning MCP tool not available this session; diff reviewed manually — no secrets found, and the gitleaks CI check is green.
CI status
All checks green on head 06a95f0: shellcheck, ShellCheck, bats, unit-tests, Lint, actionlint, CodeQL (actions + python), SonarCloud (quality gate passed), CodeRabbit, agent-shield, Agent Security Scan, Secret scan (gitleaks), holdout-guard, guard, caller-stub-freeze, template-drift, gh-aw-compile, validate-* — SUCCESS. CANCELLED entries in the rollup are superseded runs of dev-lead dispatch/relay and review checks from earlier pushes; each context has a SUCCESS conclusion for the current head. mergeStateStatus=BEHIND (not blocking).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/fleet_stub_remediate.sh`:
- Around line 171-177: Restore the single-pilot contract across
scripts/fleet_stub_remediate.sh lines 171-177, 211-225, and 486-488: define
REMEDIATE_PILOT_REPO as one repository, validate and compare it directly, and
reject repeated, comma-separated, or whitespace-separated values before any
writes. Update tests/fleet_stub_remediate.bats lines 340-354 to replace the
multi-pilot scenario with an assertion that invalid multi-pilot input fails
before writes.
- Around line 441-457: Move the remediated increment in the main remediation
loop so it occurs only after _remediate_repo returns a live-remediation result
(rc=0), not before the call. Ensure rc=3 idempotent skips do not consume
REMEDIATE_MAX_REPOS or defer subsequent eligible repositories, while preserving
failed-result handling and add coverage for a skipped repository followed by an
eligible pilot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c897b3f-4d03-4ec8-b62b-5fc668c6820e
📒 Files selected for processing (2)
scripts/fleet_stub_remediate.shtests/fleet_stub_remediate.bats
| # Pilot scope (AC #1/#4): a space/comma-separated pilot allowlist — the ONLY | ||
| # consumer repos eligible for LIVE remediation. Live remediation REQUIRES a named | ||
| # pilot repo; with none set the driver stays fully dry-run (fail-closed). Every | ||
| # DRIFTED repo outside the pilot scope is logged 'would remediate (out of pilot | ||
| # scope)' and skipped without writes. Typically ONE repo (the per-run cap below | ||
| # is the count bound); modeled as an allowlist so the two controls stay orthogonal. | ||
| REMEDIATE_PILOT_REPO="${REMEDIATE_PILOT_REPO:-}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the single-pilot contract.
The configuration, matcher, CLI, and tests collectively allow multiple live pilot repositories, contrary to the explicitly single-repository rollout requirement.
scripts/fleet_stub_remediate.sh#L171-L177: defineREMEDIATE_PILOT_REPOas one repository, not an allowlist.scripts/fleet_stub_remediate.sh#L211-L225: validate and compare one pilot value directly.scripts/fleet_stub_remediate.sh#L486-L488: reject repeated, comma-separated, or whitespace-separated pilot values.tests/fleet_stub_remediate.bats#L340-L354: replace the multi-pilot scenario and assert invalid multi-pilot input fails before writes.
📍 Affects 2 files
scripts/fleet_stub_remediate.sh#L171-L177(this comment)scripts/fleet_stub_remediate.sh#L211-L225scripts/fleet_stub_remediate.sh#L486-L488tests/fleet_stub_remediate.bats#L340-L354
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/fleet_stub_remediate.sh` around lines 171 - 177, Restore the
single-pilot contract across scripts/fleet_stub_remediate.sh lines 171-177,
211-225, and 486-488: define REMEDIATE_PILOT_REPO as one repository, validate
and compare it directly, and reject repeated, comma-separated, or
whitespace-separated values before any writes. Update
tests/fleet_stub_remediate.bats lines 340-354 to replace the multi-pilot
scenario with an assertion that invalid multi-pilot input fails before writes.
| if [ "$remediated" -ge "$max" ]; then | ||
| echo "[remediate] ${repo} deferred — per-run cap reached (REMEDIATE_MAX_REPOS=${max}); will be picked up on a later run" | ||
| continue | ||
| fi | ||
| remediated=$((remediated + 1)) | ||
| fi | ||
|
|
||
| entries="$(printf '%s' "$plan" | jq -r --arg r "$repo" \ | ||
| '.[] | select(.repo == $r) | [.stub_file, .canonical_repo, .canonical_path, .stub] | @tsv')" | ||
| _remediate_repo "$repo" "$entries" || failed=1 | ||
| # Capture the exact exit code without tripping set -e: 0 = opened+verified, | ||
| # 3 = idempotent skip (PR already open, nothing verified), other = failed. | ||
| if _remediate_repo "$repo" "$entries"; then rc=0; else rc=$?; fi | ||
| if ! _is_dry; then | ||
| case "$rc" in | ||
| 0) printf '%s\t%s\n' "$repo" "verified" >> "$summary_tmp" ;; | ||
| 3) printf '%s\t%s\n' "$repo" "skipped" >> "$summary_tmp" ;; | ||
| *) printf '%s\t%s\n' "$repo" "failed" >> "$summary_tmp"; failed=1 ;; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not let idempotent skips consume the remediation cap.
Line 445 increments remediated before _remediate_repo returns. Thus an existing-PR result (rc=3) can consume the entire cap and defer repositories despite performing no live remediation.
Proposed fix
- remediated=$((remediated + 1))
fi
entries="$(printf '%s' "$plan" | jq -r --arg r "$repo" \
'.[] | select(.repo == $r) | [.stub_file, .canonical_repo, .canonical_path, .stub] | `@tsv`')"
if _remediate_repo "$repo" "$entries"; then rc=0; else rc=$?; fi
if ! _is_dry; then
case "$rc" in
- 0) printf '%s\t%s\n' "$repo" "verified" >> "$summary_tmp" ;;
+ 0) remediated=$((remediated + 1))
+ printf '%s\t%s\n' "$repo" "verified" >> "$summary_tmp" ;;
3) printf '%s\t%s\n' "$repo" "skipped" >> "$summary_tmp" ;;
- *) printf '%s\t%s\n' "$repo" "failed" >> "$summary_tmp"; failed=1 ;;
+ *) remediated=$((remediated + 1))
+ printf '%s\t%s\n' "$repo" "failed" >> "$summary_tmp"; failed=1 ;;
esacAdd coverage for a skipped first repository followed by another eligible pilot.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$remediated" -ge "$max" ]; then | |
| echo "[remediate] ${repo} deferred — per-run cap reached (REMEDIATE_MAX_REPOS=${max}); will be picked up on a later run" | |
| continue | |
| fi | |
| remediated=$((remediated + 1)) | |
| fi | |
| entries="$(printf '%s' "$plan" | jq -r --arg r "$repo" \ | |
| '.[] | select(.repo == $r) | [.stub_file, .canonical_repo, .canonical_path, .stub] | @tsv')" | |
| _remediate_repo "$repo" "$entries" || failed=1 | |
| # Capture the exact exit code without tripping set -e: 0 = opened+verified, | |
| # 3 = idempotent skip (PR already open, nothing verified), other = failed. | |
| if _remediate_repo "$repo" "$entries"; then rc=0; else rc=$?; fi | |
| if ! _is_dry; then | |
| case "$rc" in | |
| 0) printf '%s\t%s\n' "$repo" "verified" >> "$summary_tmp" ;; | |
| 3) printf '%s\t%s\n' "$repo" "skipped" >> "$summary_tmp" ;; | |
| *) printf '%s\t%s\n' "$repo" "failed" >> "$summary_tmp"; failed=1 ;; | |
| if [ "$remediated" -ge "$max" ]; then | |
| echo "[remediate] ${repo} deferred — per-run cap reached (REMEDIATE_MAX_REPOS=${max}); will be picked up on a later run" | |
| continue | |
| fi | |
| fi | |
| entries="$(printf '%s' "$plan" | jq -r --arg r "$repo" \ | |
| '.[] | select(.repo == $r) | [.stub_file, .canonical_repo, .canonical_path, .stub] | `@tsv`')" | |
| # Capture the exact exit code without tripping set -e: 0 = opened+verified, | |
| # 3 = idempotent skip (PR already open, nothing verified), other = failed. | |
| if _remediate_repo "$repo" "$entries"; then rc=0; else rc=$?; fi | |
| if ! _is_dry; then | |
| case "$rc" in | |
| 0) remediated=$((remediated + 1)) | |
| printf '%s\t%s\n' "$repo" "verified" >> "$summary_tmp" ;; | |
| 3) printf '%s\t%s\n' "$repo" "skipped" >> "$summary_tmp" ;; | |
| *) remediated=$((remediated + 1)) | |
| printf '%s\t%s\n' "$repo" "failed" >> "$summary_tmp"; failed=1 ;; | |
| esac | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/fleet_stub_remediate.sh` around lines 441 - 457, Move the remediated
increment in the main remediation loop so it occurs only after _remediate_repo
returns a live-remediation result (rc=0), not before the call. Ensure rc=3
idempotent skips do not consume REMEDIATE_MAX_REPOS or defer subsequent eligible
repositories, while preserving failed-result handling and add coverage for a
skipped repository followed by an eligible pilot.
Closes #1151
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
--pilotand--max-reposcommand-line options.Bug Fixes