fix(proof): explicit UNVERIFIABLE state for gates without runners (#728)#801
Conversation
The 6 PROOF9 gates with no automated runner (e2e, visual, a11y, perf,
demo, manual) previously collapsed to hard FAIL, so most captured
glitches could never be SATISFIED and every run failed perpetually.
- core: new GateOutcome enum (passed/failed/unverifiable); _run_gate
returns the tri-state and its stale docstring is corrected; run_proof
maps obligations three-way, keeps requirements OPEN (waivable) on
unverifiable, and excludes unverifiable from overall_passed
- evidence/ledger: Evidence.status persists the outcome; nullable
status column added with a PRAGMA-guarded ALTER TABLE migration so
legacy DBs keep working (old rows read back status=None)
- api: run results carry {gate, satisfied, status}; EvidenceResponse
gains optional status; passed aggregation mirrors the runner rule
- cli: yellow UNVERIFIABLE cell, waive hint, exit 0 when nothing failed
- tui: distinct ❓ icon for unverifiable obligations
- web-ui: 'unverifiable' GateRunStatus + amber badge/banner/pills on
/proof and /proof/[req_id] (incl. evidence history filter + sort);
gate.run.failed notifications no longer fire for unverifiable gates
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 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 (4)
WalkthroughThis PR replaces boolean pass/fail gate results with a tri-state ChangesGateOutcome tri-state rollout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant proof_v2_router
participant run_proof
participant _run_gate
participant ledger
Client->>proof_v2_router: POST /api/v2/proof/run
proof_v2_router->>run_proof: execute requirement obligations
run_proof->>_run_gate: run gate for obligation
_run_gate-->>run_proof: GateOutcome (PASSED/FAILED/UNVERIFIABLE)
run_proof->>ledger: attach_evidence(outcome), save obligation status
run_proof-->>proof_v2_router: outcomes per gate
proof_v2_router-->>Client: results with satisfied and status
Client->>proof_v2_router: GET .../evidence
proof_v2_router->>ledger: fetch stored evidence
ledger-->>proof_v2_router: Evidence(status)
proof_v2_router-->>Client: EvidenceResponse(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 |
Code Review - fix(proof): explicit UNVERIFIABLE state (728)Overall: Clean, well-scoped implementation of the tri-state UNVERIFIABLE outcome. The design decision ("OR clause": honest state rather than 6 new runners) is correct for the current phase, the backward-compat story (keeping Medium - Connection leak in migrationconn = get_db_connection(workspace)
cursor = conn.cursor()
cursor.execute("PRAGMA table_info(proof_evidence)")
...
conn.close()If the PRAGMA or ALTER TABLE raises, conn = get_db_connection(workspace)
try:
cursor = conn.cursor()
cursor.execute("PRAGMA table_info(proof_evidence)")
columns = {row[1] for row in cursor.fetchall()}
if columns and "status" not in columns:
cursor.execute("ALTER TABLE proof_evidence ADD COLUMN status TEXT")
conn.commit()
finally:
conn.close()Medium - Migration PRAGMA runs on every DB access
_migrated_workspaces: set[str] = set()
def _migrate_evidence_status_column(workspace: Workspace) -> None:
if workspace.id in _migrated_workspaces:
return
...
_migrated_workspaces.add(workspace.id)Minor -
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web-ui/src/app/proof/[req_id]/page.tsx (1)
342-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated tri-state classification logic.
The
latestEv.status === 'unverifiable' ? ... : latestEv.satisfied ? ... : ...pattern is repeated here and again for the run-id link (Lines 371-375), separate from theevidenceResult()/RESULT_CLASShelper already introduced for the evidence table. Consider extracting a single helper (e.g.,classifyGateOutcome(ev)) shared across all three render sites to avoid drift if the tri-state rule changes again.♻️ Example consolidation
-const effectiveStatus = latestEv - ? latestEv.status === 'unverifiable' - ? 'unverifiable' - : latestEv.satisfied ? 'satisfied' : 'failed' - : ob.status; +function obligationOutcome(ev: ProofEvidence): 'satisfied' | 'failed' | 'unverifiable' { + if (ev.status === 'unverifiable') return 'unverifiable'; + return ev.satisfied ? 'satisfied' : 'failed'; +} +const effectiveStatus = latestEv ? obligationOutcome(latestEv) : ob.status;🤖 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 `@web-ui/src/app/proof/`[req_id]/page.tsx around lines 342 - 383, The tri-state status/color mapping is duplicated in the obligations table and the run-id link, which can drift from the existing evidence-table helpers. Extract a shared helper such as classifyGateOutcome(ev) (or reuse evidenceResult()/RESULT_CLASS if appropriate) in proof/[req_id]/page.tsx and use it for both the effectiveStatus calculation and the button className so all render sites follow one source of truth.web-ui/src/components/ui/badge.tsx (1)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
unverifiablebadge is visually identical toin-progress.Both variants use
bg-amber-100 text-amber-900, so if these ever appear together (e.g. in a status list or filter), users cannot visually distinguish "in progress" from "cannot verify." Consider a distinct hue to avoid ambiguity.♻️ Suggested distinct styling
- unverifiable: 'border-transparent bg-amber-100 text-amber-900', + unverifiable: 'border-transparent bg-yellow-100 text-yellow-900',🤖 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 `@web-ui/src/components/ui/badge.tsx` around lines 19 - 21, The `Badge` variant styles in `badge.tsx` make `unverifiable` identical to `in-progress`, so update the `badgeVariants` mapping to give `unverifiable` a distinct color treatment. Keep the `in-progress` amber styling, but change the `unverifiable` entry to a different hue so the two statuses are visually distinguishable wherever the `Badge` component is used.codeframe/cli/proof_commands.py (1)
342-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider missing test coverage for the new UNVERIFIABLE branch in
show.The logic itself is correct (comparing to
"unverifiable"works sinceGateOutcomeis astrEnum), buttests/cli/test_proof_commands.pyonly adds coverage forrun's tri-state output — no test exercisesshow's newev.status == "unverifiable"branch. Given this is a newly introduced display path, a regression here (e.g. a typo in the string, wrong precedence withelif ev.satisfied) would go unnoticed.🤖 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/cli/proof_commands.py` around lines 342 - 348, Add test coverage for the new UNVERIFIABLE display path in show within proof_commands/proof_commands.py. Extend tests/cli/test_proof_commands.py with a case that exercises show’s ev.status == "unverifiable" branch and verifies it renders [yellow]UNVERIFIABLE[/yellow], alongside existing PASS/FAIL behavior. Use the show command path and the GateOutcome/status handling in proof_commands.show so a typo or precedence regression in the new branch would be caught.
🤖 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 128-140: The _migrate_evidence_status_column migration is
vulnerable to a race during concurrent first-run because it checks
proof_evidence schema and then alters it in separate steps. Update this
migration to be safe when multiple workers call _ensure_tables at once by
handling the duplicate-column case around the ALTER TABLE in
_migrate_evidence_status_column (or otherwise serializing the schema change), so
a second worker can proceed cleanly if another already added status.
---
Nitpick comments:
In `@codeframe/cli/proof_commands.py`:
- Around line 342-348: Add test coverage for the new UNVERIFIABLE display path
in show within proof_commands/proof_commands.py. Extend
tests/cli/test_proof_commands.py with a case that exercises show’s ev.status ==
"unverifiable" branch and verifies it renders [yellow]UNVERIFIABLE[/yellow],
alongside existing PASS/FAIL behavior. Use the show command path and the
GateOutcome/status handling in proof_commands.show so a typo or precedence
regression in the new branch would be caught.
In `@web-ui/src/app/proof/`[req_id]/page.tsx:
- Around line 342-383: The tri-state status/color mapping is duplicated in the
obligations table and the run-id link, which can drift from the existing
evidence-table helpers. Extract a shared helper such as classifyGateOutcome(ev)
(or reuse evidenceResult()/RESULT_CLASS if appropriate) in
proof/[req_id]/page.tsx and use it for both the effectiveStatus calculation and
the button className so all render sites follow one source of truth.
In `@web-ui/src/components/ui/badge.tsx`:
- Around line 19-21: The `Badge` variant styles in `badge.tsx` make
`unverifiable` identical to `in-progress`, so update the `badgeVariants` mapping
to give `unverifiable` a distinct color treatment. Keep the `in-progress` amber
styling, but change the `unverifiable` entry to a different hue so the two
statuses are visually distinguishable wherever the `Badge` component is used.
🪄 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: f83b2254-4e09-4a34-aa0f-1139ec3911d0
📒 Files selected for processing (25)
codeframe/cli/proof_commands.pycodeframe/core/proof/evidence.pycodeframe/core/proof/ledger.pycodeframe/core/proof/models.pycodeframe/core/proof/runner.pycodeframe/tui/app.pycodeframe/ui/routers/proof_v2.pytests/cli/test_proof_commands.pytests/core/test_proof9.pytests/core/test_proof_runner_config.pytests/core/test_proof_runner_outcomes.pytests/ui/test_proof_config.pytests/ui/test_proof_v2.pyweb-ui/src/__tests__/components/proof/GateEvidencePanel.test.tsxweb-ui/src/__tests__/components/proof/GateRunBanner.test.tsxweb-ui/src/__tests__/components/proof/ProofDetailPage.test.tsxweb-ui/src/__tests__/hooks/useProofRun.test.tsweb-ui/src/app/proof/[req_id]/page.tsxweb-ui/src/app/proof/page.tsxweb-ui/src/components/proof/GateEvidencePanel.tsxweb-ui/src/components/proof/GateRunBanner.tsxweb-ui/src/components/proof/GateRunPanel.tsxweb-ui/src/components/ui/badge.tsxweb-ui/src/hooks/useProofRun.tsweb-ui/src/types/index.ts
Follow-up ReviewChecking the current code against my previous review. Fixed
Still outstanding1. Connection leak in migration (Medium) — conn = get_db_connection(workspace)
try:
cursor = conn.cursor()
cursor.execute("PRAGMA table_info(proof_evidence)")
columns = {row[1] for row in cursor.fetchall()}
if columns and "status" not in columns:
cursor.execute("ALTER TABLE proof_evidence ADD COLUMN status TEXT")
conn.commit()
finally:
conn.close()2. PRAGMA on every DB call (Medium) — _migrated_workspaces: set[str] = set()
def _migrate_evidence_status_column(workspace: Workspace) -> None:
if workspace.id in _migrated_workspaces:
return
# ... existing logic ...
_migrated_workspaces.add(workspace.id)3. const result = evidenceResult(ev);
<span className={RESULT_CLASS[result]}>
{result === 'unverifiable' ? 'cannot verify' : result}
</span>4. Items 1 and 2 are worth fixing before merge — the connection leak is a correctness issue and the PRAGMA-per-call will show up in profiling once workspaces accumulate evidence. Items 3 and 4 are cosmetic and can follow in a cleanup PR if preferred. |
- ledger: try/finally around the migration connection, tolerate the concurrent-first-run duplicate-column race, and memoize per workspace so the PRAGMA doesn't run on every DB access (CodeRabbit + claude-review) - web-ui: compute evidenceResult once per row; give the unverifiable badge distinct styling from in-progress so live runs visibly flip
|
Review triage (commit eaf4904):
Verified: |
|
Follow-up review: all prior concerns resolved. All five items I flagged in my previous review are addressed in the latest commit.\n\nConnection leak in migration - Fixed. try/finally wraps the connection; conn.close() now runs even if PRAGMA or ALTER raises.\n\nPRAGMA on every DB access - Fixed. _status_migrated_workspaces: set[str] caches at module level. Per-process, per-workspace is the correct tradeoff. Each worker process still runs one PRAGMA on first access per workspace, which is expected and acceptable.\n\nevidenceResult called 3x per row - Fixed. const result = evidenceResult(ev) computed once, reused for both the class and the label.\n\nBadge variants indistinguishable - Fixed. unverifiable now uses border-amber-300 bg-amber-50 text-amber-800 (visible border, lighter background) vs in-progress which uses bg-amber-100 text-amber-900. The inline comment explaining the visual distinction is a nice touch.\n\nHelp copy grammar - Fixed. New text ("the check could not run, so it never fails a run and stays waivable") is clear and accurate.\n\nOne minor new observation (non-blocking):\n\nThe amber GateRunBanner (shown when all verifiable gates pass but some are unverifiable) does not include the retry button that the green/red banners expose. That is arguably the right call since re-running will not un-unverify a gate, but if a deep-link to "cf proof waive" is ever added, this banner is the natural place for it.\n\nAlso, proof_v2.py lines 438-445 duplicate the runner all_passed calculation for the rare edge case where persisted_run is None. Both copies are correct; if the unverifiable exclusion rule ever changes there are two places to update.\n\nShip it. The tri-state design is clean, the backward-compat story (satisfied kept alongside status) is solid, test coverage is thorough (outcomes test module, migration test, filter regression), and all layers (core, CLI, API, web UI) are consistent. |
1 participant
Closes #728.
Problem
_GATE_TO_COREonly maps UNIT/CONTRACT→pytest and SEC→ruff; the other 6 gates (e2e, visual, a11y, perf, demo, manual) returned(False, "no automated runner — cannot verify"), so most captured glitches could never be SATISFIED and "cannot verify" was indistinguishable from "ran and failed".Approach
Per the issue's acceptance criteria "OR" clause: an explicit UNVERIFIABLE tri-state (CodeRabbit's plan, adapted), not 6 new runners.
GateOutcomeenum (passed/failed/unverifiable);_run_gatereturns the tri-state (stale docstring fixed);run_proofmaps obligation status three-way, keeps requirements OPEN (waivable) on unverifiable, and excludes unverifiable fromoverall_passed— a run never fails solely because a gate couldn't be verified.Evidence.statuspersists the outcome.proof_evidencegains a nullablestatus TEXTcolumn with aPRAGMA table_info-guardedALTER TABLEmigration (legacy rows read backstatus=None, all consumers fall back to thesatisfiedbool).proof_v2): run results carry{gate, satisfied, status}(satisfiedkept for compat);EvidenceResponsegains optionalstatus;passedaggregation mirrors the runner rule.UNVERIFIABLEcell; unverifiable-only run exits 0 with a summary naming the unverifiable gates and acf proof waivehint; real FAILs still exit 1.'unverifiable'added toGateRunStatus; amber badge variant; amber banner branch ("Gates passed — N gate(s) could not be verified"); amber "cannot verify" pills in evidence panels;/proof/[req_id]evidence history renders/filters/sorts the tri-state (incl. a new "Cannot verify" filter option);gate.run.failednotifications no longer fire for unverifiable gates; help section documents the amber state.ProofReqStatus(open/satisfied/waived) intentionally unchanged — this is a gate/obligation-level state.Acceptance criteria
/proofUI and run results distinguish "cannot verify" from "failed"_run_gatedocstring correctedTesting
uv run pytest tests/ --ignore=tests/e2e -m "not lifecycle"→ 3964 passed;ruff check .clean. Newtests/core/test_proof_runner_outcomes.py(+14) plus extensions in test_proof9 (+4), CLI (+2), proof_v2 (+3), incl. a legacy-DB column-migration test.npm test→ 95 suites / 1051+ tests green (new GateRunBanner suite, useProofRun tri-state cases, evidence-history filter regression test);npm run buildcompiles.codex review— its one P2 (evidence history treating unverifiable as fail) fixed with a regression test.Known limitations
pr_v2snapshots treat unverifiable as not passed (not proven ≠ proven); the raw status string flows intogate_breakdownfor display.Summary by CodeRabbit
New Features
Bug Fixes
Documentation