Skip to content

fix(audit): permit human-path window close in proposal approval trigger - #27

Closed
BunsDev wants to merge 1 commit into
mainfrom
fix/threads-76z-window-close-human-path
Closed

BunsDev wants to merge 1 commit into
mainfrom
fix/threads-76z-window-close-human-path

Conversation

@BunsDev

@BunsDev BunsDev commented Aug 9, 2026

Copy link
Copy Markdown
Member

Summary

Fixes threads-76z. ward_audit_require_proposal_approval_detail_insert had two mutually exclusive clauses for any proposal_approved row:

  • the human-path clause required window_close IS null for human_review/human_required
  • 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 — unapprovable by any detail shape, terminal and silent. Not reachable via today's Rust types (ApprovalPath::HumanApproval/HumanApprovalWithRationale are unit variants with no veto field, so veto_window() returns None and 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 by WARD_AUDIT_MIGRATION_V020_SQL)
  • ward_audit_current_objects_sql!() macro (used by WARD_AUDIT_SCHEMA_SQL)

Testing

Added human_review_approval_permits_null_window_close_when_window_row_exists, which inserts a proposal_window_opened row then a human_review proposal_approved row with window_close=null against the real WARD_AUDIT_SCHEMA_SQL fresh schema. Verified it reproduces the exact ABORT against the pre-fix trigger and passes against the fix.

  • cargo fmt --check — clean
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo test --locked --workspace — 215 lib tests (incl. new regression test) + all c7_roundtrip/phase5_retired_ward_corpus/rfc0001_s5_conformance integration 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

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>
Copilot AI lite review requested due to automatic review settings August 9, 2026 22:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_SQL schema initialization.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@BunsDev

BunsDev commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

Requesting Nova's review on this — she's the daemon-integration lane owner and the one Echo's threads-76z ruling was addressed to (nova/handoffs/threads-76z-sql-invariant-ruling-from-echo-2026-08-09.md). Val directed the fix applied at the file level; a corresponding handoff (threads-76z-fixed-review-requested-from-cody-2026-08-09.md) is in her workspace with the same context, asking her to spot-check the gating clause and confirm there isn't a fourth copy of this trigger text mirrored on the coven daemon side that also needs the fix — I only checked coven-threads.

Nova doesn't have a GitHub account to add as a formal requested reviewer, so flagging here for visibility alongside the file-based handoff.

@BunsDev

BunsDev commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

Checked coven (daemon repo) for a mirrored copy of this trigger, per the open question above.

No embedded duplicate found. coven-cli consumes coven-threads-core purely as a compiled git dependency — it calls coven_threads_core::WARD_AUDIT_SCHEMA_SQL / WARD_AUDIT_MIGRATION_V020_SQL directly (see crates/coven-cli/src/store.rs), it does not vendor or hand-copy the DDL/trigger text anywhere. The two other audit.rs files in that repo (coven-afs, mobile_memory) are unrelated modules with no approval_path_label/window_close logic.

One real follow-up though: coven's Cargo.toml currently pins coven-threads-core at rev = c102844, which predates this fix — I confirmed the same 3-copy bug exists at that exact pinned commit. So coven inherits nothing dangerous today (unreachable via current Rust code, same as here), but it will keep carrying the bug until someone bumps that pin to a rev at/after this PR merges. That's a version-bump follow-up in coven, not a code change — flagging separately so it doesn't get lost.

BunsDev commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Implementation review — revision required before merge

This patch resolves the SQL contradiction by weakening the stronger Phase-5 audit invariant, so it should not merge in its current form.

PHASE-5-APPROVAL-SEMANTICS.md requires every proposal_window_opened interval to end in exactly one typed terminal close. Human approval paths are unit variants and do not own veto windows. A human-labelled proposal that already has a window row is therefore inconsistent state that should fail closed—not become approvable with window_close = null.

Please revise the fix so it does one of the following, with the first preferred:

  1. Reject creation/import/migration of proposal_window_opened for human_review / human_required, preserving the invariant that any opened window always closes with typed detail; or
  2. Define an explicit normative repair/migration path for legacy inconsistent rows and still emit a typed terminal close.

Required regression evidence:

  • inconsistent human-path + opened-window history is rejected before terminal approval;
  • every opened-window terminal event requires a typed close reason;
  • ordinary human approval with no window remains valid;
  • schema fingerprint and migration tests remain green.

This is directly relevant to the open threads-980 audit-completeness blocker. Bypassing close detail would move the implementation away from Phase-5 sign-off rather than toward it.

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.

@BunsDev

BunsDev commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

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 proposal_window_opened row must fail closed and receive an explicit typed terminal rejection; permitting window_close = null would preserve the unexplained audit interval that threads-980 is intended to eliminate. Closing this bypass-oriented change rather than weakening the schema contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants