feat: implement issue #1144 — [Phase 1] Spec-drift detector script + pure-classifier unit tests - #1224
Conversation
…pure-classifier unit tests
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 57 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 scripts/spec-drift.sh, a deterministic spec-drift detector that compares merged PR diffs against a story's acceptance criteria to output a structured verdict (DRIFT, ALIGNED, or INDETERMINATE), along with a comprehensive BATS test suite in tests/test_spec_drift.bats. The review feedback recommends separating the local declaration from the command substitution for the now variable in spec-drift.sh to prevent masking exit codes (SC2155). Additionally, it is advised to create temporary directories within $BATS_TEST_TMPDIR with proper error handling in the test suite to ensure automatic cleanup and robust error recovery.
There was a problem hiding this comment.
Pull request overview
Adds the first-phase “spec drift” detector to the org CI tooling: a deterministic scripts/spec-drift.sh that compares a merged initiative PR diff against the source story’s acceptance criteria, emits a JSON verdict envelope, and includes bats coverage for the pure classifier path.
Changes:
- Introduces
scripts/spec-drift.shwith pure helpers (classifier + AC parsing + envelope) and an I/Omain()that fetches story/PR data and callsrun_triagefromscripts/engine.sh. - Adds
tests/test_spec_drift.batsto unit-test the pure helpers and exercisemain()with mockedgh/run_triage. - Registers the new bats test in
.github/workflows/lint.yml.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/spec-drift.sh | New spec-drift detector script with pure classifier + I/O main() that fetches story ACs and PR diff and emits a JSON verdict. |
| tests/test_spec_drift.bats | New bats tests covering the pure helpers and mocked-IO main() behavior (including inert no-ACs path). |
| .github/workflows/lint.yml | Adds the new bats file to the lint workflow’s test list. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #1224 |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 103eb0bc2054b54cb7077004cab03e176560aae8
Review mode: triage-approved (single reviewer)
Summary
Adds scripts/spec-drift.sh (a spec-drift detector with a pure, unit-testable classifier and an I/O-only main()), 22 bats tests in tests/test_spec_drift.bats, and registers the test file in the lint.yml bats list. Implementation cleanly follows the pure-helper + main() split modeled on scripts/mcp_connectivity_check.sh. All 6 acceptance criteria of issue #1144 are satisfied. All 5 prior bot-review threads (2 gemini, 3 copilot) were addressed by follow-up commits and are resolved. CodeRabbit approved the head commit.
Linked issue analysis
Issue #1144 lists 6 ACs — all substantively addressed:
- Pure classifier (classify_drift) maps analysis to DRIFT / ALIGNED / INDETERMINATE with no network/LLM in the pure path ✓
- main() gathers the PR diff + story ACs and invokes the cheap tier via run_triage from scripts/engine.sh (lazy-sourced so pure helpers never load the engine) ✓
- Inert paths (no story, fetch failure, non-initiative story, no ACs, no PR, empty diff) all emit INDETERMINATE and exit 0 — never fabricates a verdict ✓
- Structured JSON verdict envelope via jq, with non-numeric pr/story safely rendered as null (try tonumber catch null) ✓
- bats tests cover DRIFT / ALIGNED / INDETERMINATE including the inert no-ACs path and main() with mocked gh + run_triage; test file registered in the lint.yml bats list per AGENTS.md ✓
- shellcheck passes (ShellCheck CI check green) ✓
Findings
No blocking findings.
- Secret scan: run_secret_scanning MCP tool unavailable in this run; the gitleaks CI check passed (SUCCESS).
- lint.yml change is a single-line bats-list registration — exactly the AGENTS.md-sanctioned edit; lint.yml is not one of the protected thin-caller stubs.
- Non-blocking observation: classify_drift greps for the verdict token anywhere in the analysis (case-insensitive), and the PR diff is embedded in the LLM prompt, so a crafted diff could in principle steer the model toward ALIGNED. Impact is bounded: this is a post-merge detector, DRIFT wins when both tokens appear, absent/garbled output is INDETERMINATE, and per the story the detector is validated against a frozen eval before any workflow wiring. Worth revisiting in the Phase-2 eval story.
- Prior review threads (local-declare masking, BATS_TEST_TMPDIR usage, tonumber hardening, run_triage stderr passthrough, empty-body fetch-failure messaging) are all fixed in the current head and resolved.
CI status
All checks green on head 103eb0b: ShellCheck, shellcheck, bats, unit-tests, Lint, CodeQL (actions + python), Secret scan (gitleaks), SonarCloud quality gate, agent-shield, Agent Security Scan, holdout-guard, template-drift, guard, validate-agent-profiles, validate-personas, gh-aw-compile — SUCCESS. Cancelled/skipped entries are dev-lead dispatch/ci-relay and conditional audit jobs from earlier pushes, not failures. Branch is BEHIND main but MERGEABLE.
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: 505a7422b5a39deafa824d4f6632c651ad58b83a
Review mode: triage-approved (single reviewer)
Summary
Adds scripts/spec-drift.sh, a deterministic spec-drift detector for the initiative pipeline (issue #1144, epic #1142), with a clean pure-classifier/main() I/O split mirroring scripts/mcp_connectivity_check.sh, plus 18 bats tests registered in lint.yml. The pure helpers (classify_drift, parse_acceptance_criteria, story_is_initiative, drift_verdict_envelope) have no network/LLM dependency; main() resolves the story ACs and PR diff via gh, invokes the cheap triage tier through engine.sh, and emits a JSON verdict envelope. Fail-loud design: all unresolvable paths emit inert INDETERMINATE with exit 0 and distinct reasons, and DRIFT wins over ALIGNED when both tokens appear.
Linked issue analysis
Linked issue #1144 (closed) is substantively addressed on all 6 acceptance criteria: (1) pure unit-testable classifier with no network/LLM in the pure path; (2) main() gathers PR diff + story ACs and calls run_triage from engine.sh; (3) inert INDETERMINATE with exit 0 when ACs cannot be resolved, including a distinct reason for fetch failures vs non-initiative stories; (4) compact JSON verdict envelope via jq; (5) bats tests cover DRIFT/ALIGNED/INDETERMINATE plus the inert paths and are registered in the lint.yml bats list per AGENTS.md; (6) shellcheck passes in CI.
Findings
No blocking findings. Security: no secrets or credentials introduced (gitleaks CI green; run_secret_scanning MCP tool unavailable in this run — noted, not blocking); expansions are quoted, set -euo pipefail, mktemp used correctly, no eval/dynamic execution; the lint.yml change is a one-line additive test registration with no security-relevant workflow changes. Correctness: classify_drift fails toward DRIFT on ambiguous dual tokens; drift_verdict_envelope degrades non-numeric pr/story to JSON null (try tonumber catch null) preserving the inert contract; run_triage stderr is preserved to the script's own stderr. All 5 prior bot review threads (gemini SC2155 + BATS_TEST_TMPDIR, Copilot tonumber crash + stderr discard + misleading empty-body reason) were fixed, verified in follow-up replies, and are resolved. Risk is MEDIUM rather than triage's low call due to non-trivial new script logic, but this does not change the outcome.
CI status
All 30 checks green: shellcheck, bats (unit-tests), Lint, CodeQL (actions + python), SonarCloud, gitleaks secret scan, AgentShield, agent-profile/persona validation, template-drift, holdout-guard all SUCCESS; dependency-audit per-ecosystem jobs SKIPPED (no matching ecosystems).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Closes #1144
Implemented by dev-lead agent. Please review.