Conversation
The ward_audit_require_proposal_approval_detail_insert trigger had two
mutually exclusive clauses for any proposal_approved row: the human-path
clause required window_close IS null for human_review/human_required,
while the window-exists clause required window_close to be an object
whenever a proposal_window_opened row existed for that proposal_id. A
human-labelled proposal carrying a window row could never satisfy both,
making it unapprovable by any detail shape (terminal and silent).
Gate the window-exists clause on approval_path_label NOT IN
('human_review','human_required') so it no longer conflicts with the
human-path null requirement. Applied identically to all three
byte-identical copies of the trigger body (the escaped-quote fingerprint
macro, the migration-SQL macro, and the fresh-schema macro) so the
schema fingerprints stay in sync.
Adds a regression test that reproduces the exact ABORT against the
pre-fix trigger and passes against the fix.
Fixes threads-76z.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness gap in the SQLite ward_audit_require_proposal_approval_detail_insert trigger that could make certain human-path proposals permanently unapprovable when a proposal_window_opened row exists. It strengthens the SQL “store boundary” validation so human-labelled approvals can legally have window_close = null even if a window row is present, while preserving the stricter requirement for non-human approval paths.
Changes:
- Gate the trigger’s “window row exists ⇒ window_close must be object” clause to apply only to non-human approval paths (
approval_path_label NOT IN ('human_review','human_required')), across all three byte-identical SQL copies. - Add a regression test that reproduces the prior ABORT and confirms the fixed behavior against the real
WARD_AUDIT_SCHEMA_SQLschema initialization.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Requesting Nova's review on this — she's the daemon-integration lane owner and the one Echo's Nova doesn't have a GitHub account to add as a formal requested reviewer, so flagging here for visibility alongside the file-based handoff. |
|
Checked No embedded duplicate found. One real follow-up though: |
Implementation review — revision required before mergeThis patch resolves the SQL contradiction by weakening the stronger Phase-5 audit invariant, so it should not merge in its current form.
Please revise the fix so it does one of the following, with the first preferred:
Required regression evidence:
This is directly relevant to the open I could not submit a formal “request changes” review because this PR is authored by the authenticated account, so I am converting it back to draft to make the merge state unambiguous. |
|
Superseded by the stricter terminal-close invariant tracked in OpenCoven/coven#886 and implemented as a draft checkpoint in OpenCoven/coven#932. Human-labelled authority carrying an existing |
Summary
Fixes threads-76z.
ward_audit_require_proposal_approval_detail_inserthad two mutually exclusive clauses for anyproposal_approvedrow:window_close IS nullforhuman_review/human_requiredwindow_closeto be an object whenever aproposal_window_openedrow existed for thatproposal_idA human-labelled proposal carrying a window row could never satisfy both — unapprovable by any detail shape, terminal and silent. Not reachable via today's Rust types (
ApprovalPath::HumanApproval/HumanApprovalWithRationaleare unit variants with no veto field, soveto_window()returnsNoneand no window row is ever written for a human path in practice), but a live gap in the SQL's own defenses — the last line behind the daemon.Fix
Gated the window-exists clause on
approval_path_label NOT IN ('human_review','human_required'), applied identically to all three byte-identical copies of the trigger body so the schema fingerprints stay in sync:ward_audit_exact_trigger_fp_sql!()macro (escaped-quote fingerprint copy)ward_audit_authority_triggers_sql!()macro (used byWARD_AUDIT_MIGRATION_V020_SQL)ward_audit_current_objects_sql!()macro (used byWARD_AUDIT_SCHEMA_SQL)Testing
Added
human_review_approval_permits_null_window_close_when_window_row_exists, which inserts aproposal_window_openedrow then ahuman_reviewproposal_approvedrow withwindow_close=nullagainst the realWARD_AUDIT_SCHEMA_SQLfresh schema. Verified it reproduces the exact ABORT against the pre-fix trigger and passes against the fix.cargo fmt --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --locked --workspace— 215 lib tests (incl. new regression test) + allc7_roundtrip/phase5_retired_ward_corpus/rfc0001_s5_conformanceintegration suites green, including the fingerprint-sensitive tests that confirm all three copies remain byte-identical.🦄 Generated with Copilot CLI
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com