feat: implement issue #683 — canary #668 increment 4: decision telemetry (Layer 2) — decision-step emission + decide_decision_shift#684
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds decision outputs and telemetry steps to the reusable review workflow, introduces opt-in decision-mix comparison for canary correctness, and reports decision-mix shifts as ChangesDecision telemetry
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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 |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
![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 decision telemetry sampling (Layer 2, increment 4) to detect gross decision-mix shifts between candidate and baseline versions, opting the pr-auto-review agent into correctness gating. It implements helper functions to extract and compare decision classes, updates the orchestrator to handle correctness blocks, and adds extensive unit tests. The feedback recommends optimizing performance by constructing JSON directly in Bash instead of calling jq in a loop, and improving robustness by using shell parameter expansion to provide fallback defaults for jq --argjson inputs.
Superseded by automated re-review at
|
…try (Layer 2) — decision-step emission + decide_decision_shift
…on guard, drop out-of-scope lockfiles - _sample_decision_counts: build the counts object in ONE jq pass instead of spawning jq per key (gemini @488) — jq handles escaping so class names stay valid. - _decision_mix_table: guard cand_counts/base_counts before --argjson (gemini @545). NOTE the suggested `${var:-{}}` inline default is unusable — the inner `}` closes the parameter expansion and appends a stray brace → invalid JSON; use explicit guards. - Revert accidental package-lock.json / node_modules "name" churn (out of scope for the decision-telemetry change; also the source of the DIRTY merge conflict). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4z5uYVjwdyQjh2wFZxkut
don-petry
force-pushed
the
dev-lead/issue-683-20260712-1729
branch
from
e58ca96 to
d45c763
Compare
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — fix-reviews (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: d45c763e99d1139a4767e7d3171777008f7fc297
Review mode: triage-approved (single reviewer)
Summary
Re-review after cycle-1 fix-request. Both prior blocking findings are resolved: (1) the merge conflict is gone — the branch is MERGEABLE after the out-of-scope lockfile churn was reverted; (2) both unresolved gemini-code-assist threads on scripts/canary-rollout.sh are now resolved, with the fix commit addressing both substantively (single-pass jq counts build in _sample_decision_counts; explicit empty-string guards before --argjson in _decision_mix_table). No new issues introduced. The implementation is spec-faithful to issue #683 and the full CI suite — previously absent — is now green, including ShellCheck, bats, gitleaks, CodeQL (actions), AgentShield, and SonarCloud.
Linked issue analysis
Closes #683 (canary #668 increment 4, Layer 2 decision telemetry). Substantively addressed: Part A emits one static decision: <class> no-op step per outcome branch in pr-auto-review-reusable.yml (dispatched / skip-draft / skip-checks-pending / skip-changes-requested / skip-unresolved-threads), mirroring every early-exit + the dispatch path. Part B adds the pure decision_class/decide_decision_shift cores (jq-only, no gh/git — lib stays pure) returning OK|SHIFT|INSUFFICIENT with the exact specified knobs (min_candidate_sample=10, min_baseline_sample=20, max_shift_permille=400), sampling K=15 candidate / K=25 baseline off the memoized gh run view --json jobs surface. SHIFT holds an otherwise-PROMOTE verdict as BLOCKED/SUSPECT with a decision-mix table in the blocker issue; INSUFFICIENT is a no-op (degrades to reliability-only); default-off — a test asserts pr-auto-review is the only opted-in agent. 237 lines of bats cover all acceptance cases.
Findings
Prior findings (cycle 1) — all resolved:
- ✅ Merge conflict — resolved.
mergeable: MERGEABLE; the fix commit reverted the accidental package-lock.json/node_modules churn that caused the DIRTY state (also correctly dropped as out-of-scope). - ✅ 2 unresolved gemini threads — both resolved. (a) jq-per-key loop replaced with a single-pass
jq -Rnbuild in_sample_decision_counts; (b) explicit[ -n "$var" ] || var='{}'guards added before--argjsonin_decision_mix_table(with a correct note that the suggested${var:-{}}inline default is broken bash — the inner}closes the expansion). - ✅ Unverified bats/shellcheck — the "PR Auto-Review Tests / Lint and bats" check now runs on this PR and is green.
No new issues. The workflow delta is security-clean: static step names, run: 'true' no-ops gated on step outputs, static literals to $GITHUB_OUTPUT, no new permissions, no untrusted interpolation. The engine overlay only fires from an otherwise-PROMOTE verdict and never masks a reliability BLOCK. One accepted deviation from the issue text: the baseline samples the source tier pre-cut (prior version on identical traffic) rather than the stable tier — a sound, documented adaptation since the stable ring is the unenumerable *.
Secret scan: run_secret_scanning MCP tool unavailable in this environment; gitleaks CI check is green and manual diff inspection found no secrets.
CI status
All checks green: CI (Lint, ShellCheck, Agent Security Scan, gitleaks) ✓, PR Auto-Review Tests (Lint and bats) ✓, CodeQL (actions) ✓, AgentShield ✓, SonarCloud (quality gate passed) ✓, Dependency audit ✓ (language audits skipped, n/a), dependabot-automerge skipped (n/a). Merge state: MERGEABLE/BLOCKED (blocked only on the required review this verdict supplies). CodeRabbit: APPROVED.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: d45c763e99d1139a4767e7d3171777008f7fc297
Review mode: triage-approved (single reviewer)
Summary
Implements #668 increment 4 (Layer 2 decision telemetry): pr-auto-review-reusable.yml emits one no-op 'decision: ' step per readiness-gate outcome branch, and the canary-rollout engine gains an opt-in gate.correctness overlay that tallies the candidate's decision mix vs a prior-version baseline and holds an otherwise-PROMOTE frontier as BLOCKED/SUSPECT on a gross shift. Pure cores (decision_class, decide_decision_shift) live in lib/ with no I/O; sampling/verdict/mix-table live in the orchestrator; registry opt-in is pr-auto-review only. 23 new bats tests cover the pure cores, engine overlay, structural workflow guard, and sync-issues rendering. Triage's low-risk assessment is confirmed: the change is additive and default-off — agents without gate.correctness are byte-identical.
Linked issue analysis
Closes #683 (canary #668 increment 4). All acceptance criteria are met: (1) lib/ additions are pure jq transforms, schema addition is default-off, and a test asserts pr-auto-review is the only agent carrying gate.correctness; (2) the reusable emits decision steps on every outcome branch (structural bats guard checks all 5 classes + outputs) and the engine gates SHIFT→hold(SUSPECT) with a decision-mix table in the blocker, INSUFFICIENT→no-op; (3) bats, Lint, and ShellCheck are green. The stated out-of-scope items (other agents' telemetry, statistical rigor, increments 5/6) are correctly untouched.
Findings
No blocking findings.
Non-blocking observations:
- _RUN_DECISION_CACHE memoization does not survive the subshell boundaries at the call sites (
read ... < <(_frontier_state ...)andmix_table="$(_decision_mix_table ...)"), so the 'cache-warm second pass' comment in _decision_mix_table overstates — a SHIFT recomputes up to ~40 gh run view calls. Performance nit only; correctness is unaffected. - If gate.correctness were configured without max_shift_permille it defaults to 0, making every comparison SHIFT (fail-closed). The registry sets 400 and a test pins it, so this is theoretical.
- The decision no-op steps run after 'Dispatch review agent', so a failed dispatch emits no decision step — consistent with the documented fail-open design (the run contributes nothing to the tally).
- Workflow changes are additive no-op steps (run: 'true') and GITHUB_OUTPUT writes of fixed literals; no untrusted-input interpolation, no new permissions, no new injection surface.
- Secret-scanning MCP tool unavailable in this environment; the gitleaks CI check is green. Both prior gemini-code-assist review threads (jq-in-a-loop; empty --argjson guard) were addressed in the code and are resolved.
CI status
All checks green: Lint, Lint and bats, ShellCheck, CodeQL, Analyze (actions), Agent Security Scan, agent-shield, Secret scan (gitleaks), npm audit, SonarCloud, CodeRabbit — all SUCCESS; ecosystem-specific audits (pip/cargo/govulncheck/pnpm) SKIPPED as not applicable. Reviews: coderabbitai APPROVED, donpetry-bot APPROVED; no unresolved review threads, no unanswered human questions.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
…I gap from #613/#624) (#685) * ci: add canary-rollout-tests.yml — run the engine's bats suite (closes CI gap from #613/#624 relocation) The canary-rollout engine + tests/canary_rollout.bats were relocated into .github by #613/#624 but no CI runner came with them, so the 149+ test suite has been unenforced ever since — every #668 increment (#672/#676/#678/#684) merged without its tests actually running in CI. This workflow runs shellcheck + the full bats suite on any PR touching the engine, its pure core, the suite, or this workflow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4z5uYVjwdyQjh2wFZxkut * chore: dev-lead update (review-changes) [skip ci-relay] * chore: dev-lead update (review-changes) [skip ci-relay] --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
2 participants
Closes #683
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
SUSPECTandBLOCKEDoutcomes when significant decision shifts are detected.Bug Fixes