fix(proof): enforce EvidenceRule.test_id/must_pass in proof runner (#729) - #802
Conversation
) A requirement is no longer SATISFIED by any green pytest run — each must_pass evidence rule is enforced by a pytest run scoped to its test_id (-k), and a named test that was never written is a FAILED obligation. - EvidenceRule gains an owning gate field, stamped at capture, persisted in the ledger; legacy rows derive it from the test_id prefix convention - gates.run/_run_pytest accept a test_selector; with a selector, "no tests matched" (exit 5) is a failure, not an empty suite - _run_gate enforces pytest-style rules on every runnable gate: SEC runs ruff AND its test_sec_* rules (codex review finding) - UNIT stub now defines test_unit_<slug> to match its rule, and stub slugs share obligations.slugify with rules (was [:50] vs [:60]) - drop unused pytest import in tests/test_new_feature.py (pre-existing ruff failure)
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. 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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR implements enforcement of ChangesEvidence Rule Enforcement
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant run_proof
participant _run_gate
participant pytest_runner as gates.run (pytest)
participant GateRunner
run_proof->>run_proof: compute gate_rules for obligation gate
run_proof->>_run_gate: _run_gate(workspace, gate, gate_rules)
loop each enforced must_pass pytest rule
_run_gate->>pytest_runner: run(test_selector=rule.test_id)
pytest_runner-->>_run_gate: exit_code, output
alt exit_code == 5
_run_gate->>_run_gate: mark rule FAILED (missing test)
end
end
alt no pytest-backed rules enforced for this gate
_run_gate->>GateRunner: run whole gate suite
GateRunner-->>_run_gate: result
end
_run_gate-->>run_proof: GateOutcome (PASSED/FAILED)
run_proof->>run_proof: update requirement status
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Demo verification (Phase 11 — outcome evidence)Throwaway project with a green suite ( 1. Named test missing → obligation FAILS despite green suite (the acceptance criterion): Evidence artifact ( 2. Named tests written → obligations PASS, requirement SATISFIED: Artifact now reads Before this PR, step 1 reported PASS (whole-suite green satisfied the requirement). |
Code Review: fix(proof) enforce EvidenceRule.test_id/must_pass (#729)This is a clean, well-targeted fix that addresses the core soundness hole: a green whole-suite pytest run was satisfying any PROOF9 obligation, even one whose named regression test was never written. The design (one scoped pytest invocation per rule, exit-code-5 = FAILED) avoids external JSON-report tooling and maps directly onto pytest semantics. What is working well
Issues and suggestions1. Silent unenforcement of In unresolved = [r for r in req.evidence_rules if r.gate is None]
if unresolved:
logger.warning(
"REQ %s: %d evidence rule(s) with unresolvable gate prefix skipped: %s",
req.id, len(unresolved), [r.test_id for r in unresolved]
)2. The check = result.checks[0] if result.checks else None
if check is not None and check.exit_code == 5:
...
elif result.passed: # reached when check is None
lines.append(f"{rule.test_id}: passed")If elif check is None:
lines.append(f"{rule.test_id}: FAILED - no gate check returned")
all_passed = False3. Pre-PR stub/rule slug length mismatch is not mitigated The PR correctly unifies slug truncation at 60 chars going forward. But requirements captured before this PR have evidence rules with 60-char slugs (from 4. Sequential subprocesses per rule One 5. Missing test:
Nits (non-blocking)
SummaryThe fix is correct and the test coverage is solid. Most actionable before merge: #1 (log unresolved rules) and #2 (guard the |
…e=None no-op Addresses claude-review findings on #802: explicit FAILED when gates.run returns no checks, per-requirement warning for rules with no resolvable gate, docstring note on per-rule subprocesses, and a test pinning that gate=None rules fall back to whole-suite behavior.
Follow-up ReviewThe second commit ( Issue 1 -- Silent unenforcement of gate=None rules -- addressed The warning block is now present in Issue 2 -- Latent false-positive on empty checks list -- addressed The Issue 4 -- Sequential subprocess rationale -- addressed The Issue 5 -- Missing test for gate=None in full run_proof path -- addressed
Still open from previous reviewIssue 3 -- Slug migration gap for pre-PR workspaces Acknowledged in the PR description's "Known limitations" but no runtime signal. Requirements captured before this PR with stubs generated by the old Remaining nits (non-blocking)
OverallThe core fix is sound and the follow-up commit closes all three high-priority items from my previous review. The test suite is comprehensive -- missing/passing/failing named test, must_pass=False, SEC dual path, multi-rule mixed outcome, legacy hydration, gate=None no-op. Ready to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/core/gates.py (1)
482-505: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
-kcan pass the wrong test here.pytest -kis substring-based, sorule.test_idcan match a different test and make the gate pass even when the exact named test is missing. Pass a full node ID or do an exact collect check instead.🤖 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 `@codeframe/core/gates.py` around lines 482 - 505, The _run_pytest gate currently uses pytest -k via test_selector, which can match the wrong test by substring and falsely pass the gate. Update _run_pytest to avoid -k for exact test validation: either pass a full pytest node id when selecting a specific test, or perform an exact collect check before running pytest. Keep the change localized around the test_selector handling in _run_pytest so rule.test_id cannot match a different test.
🧹 Nitpick comments (2)
tests/core/test_proof_evidence_rules.py (1)
149-269: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLGTM! Enforcement scenarios (missing/passing/failing named tests, must_pass=False, mixed rules, SEC+ruff coexistence, unmapped gate) thoroughly exercise
_run_gate. Note: these mocks returnGateStatus.FAILED/PASSEDexplicitly and never exercise theSKIPPEDpath flagged inrunner.py's_run_gatereview — consider adding a case mocking pytest-unavailable (SKIPPED,exit_code=None) to lock in the fix suggested there.🤖 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 `@tests/core/test_proof_evidence_rules.py` around lines 149 - 269, Add a test that exercises the `_run_gate` SKIPPED branch in `codeframe.core.proof.runner` by mocking `codeframe.core.gates.run` to return a pytest-unavailable result with `GateStatus.SKIPPED` and `exit_code=None`. Update `TestRunGateEnforcement` with a case similar to the existing `_rule`-driven tests, and assert that `_run_gate` handles the skipped result as expected rather than treating it like a normal pass/fail outcome. This will lock in the behavior for the `SKIPPED` path referenced in `_run_gate`.codeframe/core/proof/runner.py (1)
123-129: 🚀 Performance & Scalability | 🔵 TrivialEach enforced rule triggers a full
core_gates.run()preflight (dependency install check) plus its ownGATES_STARTED/GATES_COMPLETEDevents.For a requirement with several
must_passrules on the same gate, this multiplies dependency-install checks and event-log writes N times per proof run (on top of N per requirement across the whole run). The per-rule-subprocess design is explicitly justified in the docstring for exit-code precision, but the dependency preflight and event emission overhead look like an unintended side effect of reusing the fullcore_gates.run()entry point rather than a lower-level runner. Consider hoisting a single dependency check perrun_proofinvocation (or passingauto_install_deps=Falsefor the scoped per-rule calls once dependencies are known to be installed) to avoid repeated subprocess overhead and event-log noise.🤖 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 `@codeframe/core/proof/runner.py` around lines 123 - 129, Each enforced rule is currently invoking the full core_gates.run() path, which repeats dependency preflight and emits GATES_STARTED/GATES_COMPLETED events for every rule. In run_proof, update the per-rule loop over enforced to avoid redoing that shared setup: either perform one dependency-install check once before the loop and reuse it, or call core_gates.run() with auto_install_deps disabled for the scoped rule runs after dependencies are known to be installed. Keep the existing per-rule subprocess behavior for exit-code precision, but reduce the repeated overhead and event noise.
🤖 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 `@codeframe/core/proof/ledger.py`:
- Around line 199-211: The persisted gate parsing in _evidence_rules_from_json
is too strict and can crash requirement loading when a stored gate string is
invalid. Update the EvidenceRule construction to defensively handle bad
d["gate"] values by catching invalid Gate(...) conversion and falling back to
gate_from_test_id(d["test_id"]) just like the legacy path, so
_row_to_requirement and get_requirement can continue loading other rows.
In `@codeframe/core/proof/runner.py`:
- Line 118: The runner logic in runner.py drops must_pass rules whose test_id
does not start with "test_", so they are neither enforced nor reported. Update
the enforcement path around enforced/all_passed to detect any must_pass rule
that is not eligible for execution, and treat it as a failure or explicit
unenforceable condition instead of silently skipping it. Make sure the status is
reflected in the artifact/log output alongside the existing rule handling in
run_proof and the informational loop so the rule is visible even when its
test_id prefix is nonstandard.
- Around line 123-141: The proof enforcement loop in runner.py is using
result.passed for each enforced rule, which can incorrectly treat SKIPPED checks
as satisfied; update the logic in the rule-processing block inside the
enforcement loop to require an explicit GateStatus.PASSED on the selected check
instead of relying on core_gates.run()’s aggregate passed flag, and make any
non-PASSED status (including SKIPPED) fail the rule while preserving the
existing missing-check and exit_code handling.
---
Outside diff comments:
In `@codeframe/core/gates.py`:
- Around line 482-505: The _run_pytest gate currently uses pytest -k via
test_selector, which can match the wrong test by substring and falsely pass the
gate. Update _run_pytest to avoid -k for exact test validation: either pass a
full pytest node id when selecting a specific test, or perform an exact collect
check before running pytest. Keep the change localized around the test_selector
handling in _run_pytest so rule.test_id cannot match a different test.
---
Nitpick comments:
In `@codeframe/core/proof/runner.py`:
- Around line 123-129: Each enforced rule is currently invoking the full
core_gates.run() path, which repeats dependency preflight and emits
GATES_STARTED/GATES_COMPLETED events for every rule. In run_proof, update the
per-rule loop over enforced to avoid redoing that shared setup: either perform
one dependency-install check once before the loop and reuse it, or call
core_gates.run() with auto_install_deps disabled for the scoped rule runs after
dependencies are known to be installed. Keep the existing per-rule subprocess
behavior for exit-code precision, but reduce the repeated overhead and event
noise.
In `@tests/core/test_proof_evidence_rules.py`:
- Around line 149-269: Add a test that exercises the `_run_gate` SKIPPED branch
in `codeframe.core.proof.runner` by mocking `codeframe.core.gates.run` to return
a pytest-unavailable result with `GateStatus.SKIPPED` and `exit_code=None`.
Update `TestRunGateEnforcement` with a case similar to the existing
`_rule`-driven tests, and assert that `_run_gate` handles the skipped result as
expected rather than treating it like a normal pass/fail outcome. This will lock
in the behavior for the `SKIPPED` path referenced in `_run_gate`.
🪄 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: CHILL
Plan: Pro
Run ID: 93d500b4-1ab5-43bc-b6d7-c2cf26104941
📒 Files selected for processing (9)
codeframe/core/gates.pycodeframe/core/proof/ledger.pycodeframe/core/proof/models.pycodeframe/core/proof/obligations.pycodeframe/core/proof/runner.pycodeframe/core/proof/stubs.pytests/cli/test_proof_commands.pytests/core/test_proof_evidence_rules.pytests/core/test_proof_runner_outcomes.py
- invalid persisted gate values fall back to test_id prefix derivation instead of crashing requirement load - SKIPPED/ERROR pytest checks fail enforcement (pytest unavailable is not proof) - must_pass rules without a pytest-style test_id fail loudly instead of vanishing silently
Follow-up Review (Round 3)Previous rounds closed Issues 1, 2, 4, 5 — the warning on gate=None rules, the explicit empty-checks guard, the subprocess rationale docstring, and the gate=None integration test. Good work closing those quickly. Three items remain open from the earlier reviews and CodeRabbit's formal pass. 1. SKIPPED path still has no test (CodeRabbit nitpick, still open)
# In TestRunGateEnforcement
def test_must_pass_rule_fails_when_gate_is_skipped(self, tmp_path):
ws = _make_workspace(tmp_path)
rule = EvidenceRule(test_id="test_unit_login", must_pass=True, gate=Gate.UNIT)
skipped_result = MagicMock()
skipped_result.checks = [MagicMock(status=GateStatus.SKIPPED, exit_code=None, name="pytest")]
skipped_result.passed = False
with patch("codeframe.core.proof.runner.core_gates.run", return_value=skipped_result):
outcome, output = _run_gate(ws, Gate.UNIT, [rule])
assert outcome == GateOutcome.FAILEDThis should be a quick addition before merge. 2. Repeated preflight overhead (CodeRabbit nitpick)For a requirement with N The targeted fix is to pass 3. Pre-PR slug migration gap (from Round 1, still open)Requirements captured before this PR that have titles longer than 50 chars will silently remain unenforced — the old stubs used a 50-char slug, the new The PR description notes this limitation. If there's no plan to add a migration or CLI hint, a tracking issue would keep it visible for users who hit this after upgrading. SummaryThe core enforcement logic is solid and the fix for #729 is correct. The SKIPPED path test (Item 1) is the only remaining change I'd want to see before merge — it covers a real code branch added in this PR. Items 2 and 3 are non-blocking but worth filing as follow-ups. |
1 participant
Closes #729
Problem
Capture generates per-requirement evidence rules and the ledger serializes them, but
_run_gatejust ran the whole gate suite — a requirement was SATISFIED by any green pytest run even if the named regression test was never written. The "enforced forever" loop enforced nothing specific.Changes
EvidenceRule.gate: each rule now carries its owning gate (stamped insuggest_evidence_rules, persisted in the ledger; legacy rows derive it from thetest_idprefix convention, unknown prefixes stayNone/unenforced).gates.run/_run_pytesttest_selector: optional pytest-kexpression; with a selector, exit 5 ("no tests matched") is FAILED, not an empty-suite pass._run_gate(workspace, gate, rules): everymust_passrule with a pytest-styletest_idruns as its own scoped pytest invocation — missing →FAILED — named test missing, failing → FAILED, and only passing named tests satisfy. The gate's own runner still runs when it isn't pytest (SEC = ruff and itstest_sec_*rules).must_pass=Falserules are informational. Gates with no rules keep prior behavior; unmapped gates stay UNVERIFIABLE ([P1.1] PROOF9: give the 6 unrunnable gates real runners or an explicit UNVERIFIABLE state (not perpetual fail) #728).test_unit_<slug>(wastest_<slug>, which could never satisfy its own rule), and stubs shareobligations.slugifywith rules (slug truncation was [:50] vs [:60]).Design notes (divergence from the CodeRabbit issue plan)
No
pytest-json-report: one scoped pytest run per rule gives the full signal from exit codes alone (5=missing, 1=failing, 0=passing), avoiding the json-report flakiness the plan itself warned about.Review
Pre-PR cross-family review (
codex) found 2 issues, both fixed and covered by tests:must_passpytest rulestest_idTests
19 new tests in
tests/core/test_proof_evidence_rules.py(missing/passing/failing named test, must_pass=False, SEC dual enforcement, stub/rule alignment, legacy hydration,-kthreading). Existing_run_gatestubs updated for the new signature. Full non-e2e suite: 3980+ passed; ruff clean.Known limitations
-kmatching is substring-based: a rule namedtest_unit_loginalso matchestest_unit_login_extra. Acceptable for presence-detection; exact node IDs would require tests to exist at capture time.test_idhas no recognizable gate prefix remain unenforced (safe fallback, logged in output as absent).test_id. A per-requirement warning also lists any rules whose gate could not be resolved.Summary by CodeRabbit
New Features
Bug Fixes