From d371a8a3e5df032c8d874b1a8b5cf7311addd13d Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 27 Sep 2026 00:20:35 +0200 Subject: [PATCH 1/3] :bug: test(release): disable tag signing in the changelog gate sandboxes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate is meant to be hermetic — its sandbox git helpers already pass -c commit.gpgsign=false so a global signing config cannot red it — but they missed tag.gpgsign. With tag.gpgsign=true in a developer global config, git tag becomes a signed annotated tag that needs a message, so dgit tag v99.0.0 died with "no tag message?" (and, with an interactive editor configured, an editor prompt). Add -c tag.gpgsign=false beside commit.gpgsign=false in every sandbox helper (sgit/ssgit/dgit/ogit/tgit). --- hack/release/changelog_gate_test.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hack/release/changelog_gate_test.sh b/hack/release/changelog_gate_test.sh index afb250f..d1bfef3 100755 --- a/hack/release/changelog_gate_test.sh +++ b/hack/release/changelog_gate_test.sh @@ -573,7 +573,7 @@ sgit() { -u GIT_ALTERNATE_OBJECT_DIRECTORIES -u GIT_COMMON_DIR \ git -C "$SANDBOX" \ -c user.name='changelog gate' -c user.email='gate@example.invalid' \ - -c commit.gpgsign=false -c core.hooksPath=/dev/null \ + -c commit.gpgsign=false -c tag.gpgsign=false -c core.hooksPath=/dev/null \ -c init.defaultBranch=main -c advice.detachedHead=false "$@" } env -u GIT_DIR -u GIT_WORK_TREE -u GIT_INDEX_FILE -u GIT_OBJECT_DIRECTORY \ @@ -1001,7 +1001,7 @@ ssgit() { -u GIT_ALTERNATE_OBJECT_DIRECTORIES -u GIT_COMMON_DIR \ git -C "$SUBJ_SANDBOX" \ -c user.name='commit subject gate' -c user.email='gate@example.invalid' \ - -c commit.gpgsign=false -c core.hooksPath=/dev/null \ + -c commit.gpgsign=false -c tag.gpgsign=false -c core.hooksPath=/dev/null \ -c init.defaultBranch=main -c advice.detachedHead=false "$@" } env -u GIT_DIR -u GIT_WORK_TREE -u GIT_INDEX_FILE -u GIT_OBJECT_DIRECTORY \ @@ -1151,7 +1151,7 @@ dgit() { -u GIT_ALTERNATE_OBJECT_DIRECTORIES -u GIT_COMMON_DIR \ git -C "$DSB" \ -c user.name='changelog gate' -c user.email='gate@example.invalid' \ - -c commit.gpgsign=false -c core.hooksPath=/dev/null \ + -c commit.gpgsign=false -c tag.gpgsign=false -c core.hooksPath=/dev/null \ -c advice.detachedHead=false "$@" } head_sha="$(git -C "$ROOT" rev-parse HEAD)" @@ -1400,7 +1400,7 @@ if [[ -z "${ASSENT_CHANGELOG_GATE_NESTED:-}" ]]; then -u GIT_ALTERNATE_OBJECT_DIRECTORIES -u GIT_COMMON_DIR \ git -C "$TSB" \ -c user.name='changelog gate' -c user.email='gate@example.invalid' \ - -c commit.gpgsign=false -c core.hooksPath=/dev/null \ + -c commit.gpgsign=false -c tag.gpgsign=false -c core.hooksPath=/dev/null \ -c advice.detachedHead=false "$@" } env -u GIT_DIR -u GIT_WORK_TREE -u GIT_INDEX_FILE -u GIT_OBJECT_DIRECTORY \ @@ -1438,7 +1438,7 @@ if [[ -z "${ASSENT_CHANGELOG_GATE_NESTED:-}" ]]; then env -u GIT_DIR -u GIT_WORK_TREE -u GIT_INDEX_FILE -u GIT_OBJECT_DIRECTORY \ -u GIT_ALTERNATE_OBJECT_DIRECTORIES -u GIT_COMMON_DIR \ git -c user.name='changelog gate' -c user.email='gate@example.invalid' \ - -c commit.gpgsign=false -c core.hooksPath=/dev/null "$@" + -c commit.gpgsign=false -c tag.gpgsign=false -c core.hooksPath=/dev/null "$@" } mkdir -p "$OSRC/sub" echo kept >"$OSRC/kept" && echo unstaged >"$OSRC/unstaged-rm" && echo staged >"$OSRC/sub/staged-rm" From 5adf5d818753bb2ead573e0e6cf17af349b944c4 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 27 Sep 2026 00:20:45 +0200 Subject: [PATCH 2/3] :bug: fix(aggregate): an unmatched edit never arms APPROVE (RVW-S02, D-185) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adversarial review (finding C-1, reproduced) showed cover() marks a required obligation covered as soon as an enforce rule NAMES it in prove.obligation — before matchChanges runs — so a rule whose match selects none of the governed changes still marks it covered, contributes no finding, and the decision falls through to APPROVE with an empty finding set. A policy whose only rule matches valueChanges.pointers ["/partitions"] and proves non-destructive, under require [non-destructive], APPROVED a change at /replicas with zero findings. GUIDELINES Safety-1 and D-142 REQ-DEM-S10-02 both intend REVIEW, and the intended runtime backstop (classify.ValidateRouting) has no production caller. Add an additive, fail-safe escalation in cover(), mirroring the D-063/D-064 unmatched-whole-file-DELETE guard: for each value-level change (path != "") that no enforce-effective prove rule selects, under a binding that requires an obligation some rule proves, worse(decision, REVIEW) and emit a synthetic aggregate.unmatchedEdit finding (code change.unmatchedEdit). Scope is value-level edits only (whole-file deletes are D-064, adds are non-destructive); observe-phase rules do not govern; one finding per unvouched subject. The comparison corpus baseline (promotion-gates) gains a permissive retention-permissive rule: the challenge-intervention-added case had depended on the fail-open (an ungoverned /retentionMs edit auto-approving), so the baseline now genuinely governs and permits the edit and the candidate retention challenge is a real delta again. Spec: openspec/specs/p5-rvw-review-remediation/spec.md RVW-S02. --- docs/decisions/decisions.md | 1 + .../comparison/promotion-gates/baseline.yaml | 20 ++ internal/core/aggregate/aggregate.go | 5 + internal/core/aggregate/coverage.go | 99 ++++++++ internal/core/aggregate/coverage_test.go | 36 ++- .../core/aggregate/unmatched_edit_test.go | 230 ++++++++++++++++++ openspec/specs/backlog.md | 7 +- .../specs/p5-rvw-review-remediation/spec.md | 98 ++++++-- 8 files changed, 473 insertions(+), 23 deletions(-) create mode 100644 internal/core/aggregate/unmatched_edit_test.go diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index 614c7b7..f6329b2 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -188,3 +188,4 @@ project/process decisions. | D-182 | 2026-09-25 | **`mr.labels` is WIRED through the live `assent run` path rather than retired by a lint hard error. The frozen predicate-scope contract keeps its label field, and the fail-open is closed by making the forge fetch honest.** The six-leg review found `mr.labels` schema-carried (`evaluation-input.schema.json`), engine-bound (`aggregate/evaluate.go` binds `labels`) and harness-tested (`internal/adoptertest/mr_test.go`) but never fetched on the live path: `cmd/assent/run.go:581` `mrFrom` built `aggregate.MR` with no `Labels`, and the GitLab adapter's two MR decoders (`GetMR` in `internal/forge/gitlab/gitlab.go` and `mrWithAuthor` in `snapshot.go`) ignored the API's `labels` array. **Consequence, and why it is a safety defect not a missing feature:** every label predicate in production evaluated against an empty list, so a NEGATIVE guard (`!('security-hold' in mr.labels)`) was vacuously true and a `security-hold` MR could APPROVE and merge. **Decision — the alternative is recorded and rejected:** a lint hard error rejecting any `mr.labels` reference plus a `predicate-scope.md` amendment would have made the contract truthful by deleting a capability adopters were already sold; wiring keeps the contract and costs two JSON decodes. **Implementation:** `forge.MRInfo` gains `Labels`; `GetMR` and `mrWithAuthor` decode the string array (absent field → nil, never an error); `Snapshot` surfaces them on `forge.MRHeads.Labels`; `mrFrom` copies `info.Labels` into `aggregate.MR.Labels`. **Proven, not asserted:** a run-path test through the real GitLab adapter over `httptest` (`fakeGitLab`) shows a labelled MR reaching the engine (a label-proven obligation APPROVEs with the label, REVIEWs without it) and the negative-guard polarity failing CLOSED (the guarded label yields BLOCK with zero approve/merge writes). Spec: `openspec/specs/p5-e4-gitlab-forge/spec.md` REQ-E4-S02-05 + REQ-E4-S06-08. Revert: drop `Labels` from `MRInfo`/`MRHeads`, the two decodes and the `mrFrom` copy — reopens the empty-label fail-open for every negative guard. | | D-183 | 2026-09-25 | **The six-review round's first fix slice lands: `assent run` reads every judged byte at the pinned commit SHA, not the mutable branch name (U-04 item 1).** Origin: the 2026-09-25 six-leg review (`data/assent-unify/report.md` §4.1 U-04, §6 lane B1; `data/assent-diff-qfn/report.md` QFN-01) at HEAD `b8123cc`. **The defect, stated precisely:** `orchestrate` pins `info.SourceSHA`/`info.TargetSHA` from `GetMR` (`run.go:177`), and the approval and `merge?sha=` CAS are pinned to those commits — but the six content reads on the live path resolved a mutable branch name (`run.go:203,211,230,249,270` = `info.TargetBranch`; `:274` = `info.SourceBranch`). The invariant the merge leans on, *final head == pin ⇒ final head == what was judged*, is therefore false: a contributor with push rights on the MR branch pushes benign X2 (judged), then force-pushes back to pinned X1 before `Reconcile`'s re-read, the SHA-guard passes (head == pin), and the forge merges X1 — bytes never evaluated. The `DecisionRecord` pins `policySha/sourceSha/targetSha` but no content digest, so the record cannot distinguish the states either; ADR-0015 §2's "Merge is SHA-guarded (no TOCTOU)" heading describes the *branch-moved* guard, not a bytes-equal-to-pin guarantee. **The change:** pass `info.TargetSHA` at `:203`, `:211`, `:230`, `:249`, `:270` and `info.SourceSHA` at `:274`. The GitLab adapter already accepts a commit SHA as `?ref=` (`gitlab.go:474`, the same URL-escaped parameter), so this is one argument at six sites — no adapter API change, no new discriminator, no reordering. Target-side reads are pinned too (judgment call (a)): not strictly required for the source-side attack, but it removes the maintainer-push-window ambiguity on the policy/config/pack loads and leaves one uniform rule rather than an asymmetry a later reader must re-justify. **The tests are FLIPPED, not merely updated (judgment call (b)):** the run-path fake's file router now serves the pinned SHAs and errors on any other ref, and `TestFileAtRef` asserts the SHA it sends — so a regression to `info.TargetBranch`/`info.SourceBranch` reddens the existing suite rather than passing against a fake that still accepts branch names. `run_self_vouch_test.go:70`'s policy-load ref assertion moves from `f.target` to `f.targetTip` for the same reason. **The move-and-restore case is in two places, deliberately (judgment call (c)):** the `internal/forge/conformance` case `sha-guard-source-moved-and-restored` (with a new `Fixture.MoveSourceHead` seam and a `catalog.yaml` row) drives `forge.Reconcile` only, so it proves the CAS **merges once the head is back at the pin** — i.e. the CAS alone cannot catch a move-and-restore, which is exactly why the read must be pinned; the run-path case `TestRunJudgesPinnedSHAWhenBranchMoves` proves the read itself follows the pin (branch points at benign bytes, pin at violating bytes, decision follows the pin). **Strictly stronger pinning, no behaviour change beyond it:** the same bytes are read when the branch has not moved. **Explicitly NOT in this lane:** U-04 items 2–4 (SHA-bound `/repository/compare` enumeration re-fold — shares its mechanism with the D-139 trio, lane B5; the `CI_MERGE_REQUEST_SOURCE_BRANCH_SHA` belt; the additive `headContentSha` pin, lane B9), the `--checkout` tree↔SHA binding (U-08/QFN-04, lane B5), the `forgePort` shape (E10), and the `docs/usage/cli.md` "runs without `--checkout` are not exposed" claim (its enumeration half is item 2). Spec: `openspec/specs/p5-rev1-pinned-sha/spec.md`. **Revert:** restore `info.TargetBranch`/`info.SourceBranch` at the six sites and un-flip the fake routers — which re-opens the move-and-restore merge of un-evaluated bytes on the checkout-less path the docs call safe. | | D-184 | 2026-09-25 | **RVW-S01 — an empty/absent binding `require:` must never arm APPROVE: the run path refuses it, `assent lint` hard-errors `binding-require-empty`, and this row supersedes D-021's "vacuously covered" clause. The frozen schema's `minItems: 1` is deferred.** **Finding (reproduced on the run path):** a schema-valid `RulesetBinding` binding with `require: []` (or no `require:` key) decides **APPROVE with zero findings** for any governed change its block rules do not fire on. The obligation layer iterates `bind.Require`, so an empty list makes it vacuous; with the forge-probed arming preconditions met the run then approves and merges. The schema accepts it (`require` is not in the binding's `required` list and carries no `minItems`; `schemas/policy/v1alpha1/ruleset-binding.schema.json:52-57`) and lint passed it (`checkObligationCoverage` iterates the same empty list), while two in-repo authorities say the opposite: `GUIDELINES.md` §Safety-1 ("Every change must be positively vouched") and `internal/core/decision/record.go`'s own seam note (S03 review F3: the CLI wiring "must guarantee require is non-empty before an APPROVE is armed"). D-021 added `require` as optional and recorded "absent/empty ⇒ no required obligations (vacuously covered)" — a true statement about the schema's shape that was read as a safety property, and was never one. **Decision:** the invariant wins. (a) **Run path** — `decide`'s decidable, non-reserved arm returns a hard error when the covering binding's `require` is empty; the run exits `1` with a contributor-readable message naming the binding `(class, environment)`, and **zero forge writes** happen (the guard sits before `buildEvaluationInput`, so no record is emitted either). It is deliberately NOT placed in `selectBinding`: that function is shared with `assent compare`, whose whole purpose is to compare permissive candidates, and refusing to compare an unsafe candidate would defeat it. (b) **Lint** — new hard error `binding-require-empty`, located to the binding, with a `good`/`bad` fixture pair registered in the E3-S08 `hardErrorCorpus`. (c) **Reserved-class exemption, stated because it is a real carve-out:** the reserved `assent-policy` meta-class is block-by-default (ADR-0015 §1) and its safety comes from GUARD 1 (any `.assent/**` edit BLOCKs), not from obligation coverage — and it *cannot* carry a non-empty `require` at all, because any `prove` rule in a reserved-bound pack routes the policy class to a vouch/APPROVE-arming disposition the reserved-class check forbids. The lint check therefore skips `class == assent-policy`; the run-path guard still refuses an empty require whenever the SUBJECT is not the reserved class. (d) **Reconciliation** — the schema description's "Absent or empty ⇒ no required obligations (vacuously covered)" and D-021's identical clause are superseded as *safety* statements by this row; D-021 is left byte-unchanged (supersede, don't edit); D-134's ARM-08 run-path consequence ("with no `require:` on the binding it turns a BLOCK into APPROVE+merge") is superseded the same way — the engine still decides APPROVE for an empty `require`, but `decide` (a) refuses before it, so that approve+merge path is gone. The schema's `minItems: 1` is **deferred to the next schema change window**: `schemas/**/*.json` is under the ref-relative freeze guard (D-132) against the released `v0.1.0` tag, which permits exactly one diff (the D-120 `toolDigest` description) and no other, so editing the frozen file in this lane would red `hack/audit/exitgate_test.sh`. Until that window, (a)+(b) are the enforcement. **Not in scope, deliberately:** the unmatched-edit fail-open (a rule that names an obligation but whose `match` selects nothing still marks it covered) shares the `cover()` invariant but is its own register row and is gated on a held captain decision; `assent test` does not arm APPROVE and is unchanged; `assent compare` is unchanged per (a). **Scope note:** `examples/lint-fixtures/{fail-open,reserved-class}/` were touched — `fail-open`'s bindings gained `require: [reviewed]` (the rule already proved it), and `reserved-class/good` is covered by the exemption; both were previously "clean" only because the check did not exist. **Revert:** delete `checkBindingRequireEmpty` and its corpus row, drop the `decide` guard, restore the two `fail-open` bindings, and the vacuous APPROVE returns. | +| D-185 | 2026-09-26 | **RVW-S02 — an unmatched EDIT never arms APPROVE: an additive fail-safe escalation in `cover()` mirrors the D-063/D-064 unmatched-whole-file-DELETE guard, and the `promotion-gates` comparison baseline is corrected because one of its cases had accidentally depended on the fail-open.** **Finding (report C-1, reproduced):** `cover()` marks a required obligation *covered* as soon as any enforce-phase rule names it in `prove.obligation` — BEFORE it computes `matchChanges` (`internal/core/aggregate/coverage.go:150-152` vs `:154-160`). So a rule that names the obligation but whose `match` selects none of the governed changes still marks it covered, contributes no finding, and the decision falls through to APPROVE with an EMPTY finding set: a policy whose only rule matches `valueChanges.pointers: ["/partitions"]` and proves `non-destructive`, under `require: [non-destructive]`, APPROVED a change at `/replicas` with zero findings. GUIDELINES §Safety-1 and D-142's REQ-DEM-S10-02 both intend REVIEW. The intended runtime backstop, `classify.ValidateRouting`, has no production caller and `assent run` never runs lint, so nothing else catches it. **Decision (captain: option (a), "add an unmatched-edit escalation to REVIEW, mirroring the D-064 unmatched-delete guard"):** after the rule loop, for each VALUE-LEVEL change (`ch.Path != ""`), if no enforce-effective prove rule selects it, `worse(decision, REVIEW)` and emit a synthetic `aggregate.unmatchedEdit` finding (`effect: require-review`, `code: change.unmatchedEdit`, subject the change's subject). **Scope — value-level edits only:** whole-file (`path==""`) lifecycle events are the fileEvents domain — a delete is the D-064 guard's subject, an add is non-destructive (D-063) — so neither escalates here; this also leaves the D-016 golden's whole-file rename event untouched. **Gate:** the escalation runs only when `requiredObligationCovered` (some obligation the binding requires has an enforce proving rule, i.e. `covered[obl]`). An empty/absent `require` is D-184's run-path seam and the engine's vacuous-APPROVE reading is deliberately unchanged; a `require` with no proving rule already trips the uncovered guard. Without the gate a policy with no obligation layer would escalate every value change — outside C-1's scope. **"Governed" (`editGoverned`) mirrors `fileDeleteGoverned` exactly:** some enforce-effective prove rule selects the change, obligation-agnostic (a non-required signal rule that matches is a real evaluation, and the points model depends on it being able to leave the decision at APPROVE). Observe-phase (and pack-ceiling-observe) rules do NOT govern — their findings are decision-excluded, so treating them as governing would suppress escalation into APPROVE, the same D-063 fail-open the delete guard names. One finding per unvouched SUBJECT (deduped; a subject can carry many value changes and duplicate identical findings would be noise). **Additive and deterministic:** it only ever raises toward REVIEW via `worse()`, never relaxes a BLOCK, and the findings are canonically sorted; a fully governed changeset is byte-identical. **Corpus correction, stated because it is a real behaviour change to a shipped example:** `examples/comparison/promotion-gates/cases/challenge-intervention-added` changed `/retentionMs` under a baseline (`prod-strict@6`) with NO rule matching that pointer, so it APPROVED only because of this fail-open; with the guard the baseline REVIEWs and the case's `stricter-intervention-added` delta disappears. The baseline gains a permissive `retention-permissive` rule (proves a required obligation cleanly true), so the baseline genuinely governs and permits the edit and the candidate's `retention-ack` challenge is a real delta again — the record is byte-identical. **Reproduced then fixed:** the pre-fix `Cover` returned `APPROVE findings=[]`; the regression test `TestUnmatchedEditFailsSafeReview` pins the REVIEW + `aggregate.unmatchedEdit` finding, both polarities, the gate, the scope and the observe cases. Spec: `openspec/specs/p5-rvw-review-remediation/spec.md` RVW-S02. **Revert:** delete the escalation block + `editGoverned`/`requiredObligationCovered` + `ruleUnmatchedEdit`, restore `baseline.yaml`, and the unmatched edit silently APPROVEs again. | diff --git a/examples/comparison/promotion-gates/baseline.yaml b/examples/comparison/promotion-gates/baseline.yaml index a5a5d69..f060524 100644 --- a/examples/comparison/promotion-gates/baseline.yaml +++ b/examples/comparison/promotion-gates/baseline.yaml @@ -45,3 +45,23 @@ spec: onFailure: effect: comment code: value-bumped + # RVW-S02: the baseline must GOVERN a /retentionMs edit for it to be a valid + # APPROVE side. Before the unmatched-edit escalation it approved /retentionMs + # only because no rule matched it — the fail-open this corpus case accidentally + # depended on. This permissive rule proves a required obligation cleanly true, + # so the baseline genuinely permits retention changes; the candidate's + # retention-ack challenge is then a real stricter-intervention-added delta. + - name: retention-permissive + phase: enforce + match: + valueChanges: + pointers: ["/retentionMs"] + kinds: [modify] + prove: + obligation: non-destructive + when: + cel: "true" + message: "baseline permits retention changes" + onFailure: + effect: block + code: retention.shrunk diff --git a/internal/core/aggregate/aggregate.go b/internal/core/aggregate/aggregate.go index cd88ad8..c27c9b5 100644 --- a/internal/core/aggregate/aggregate.go +++ b/internal/core/aggregate/aggregate.go @@ -220,6 +220,11 @@ const ( // event earns (EFE-S02, Judgment call (a) / D-063): a delete no evaluated // fileEvents rule covers must never silently APPROVE. ruleUnmatchedDelete = "aggregate.unmatchedDelete" + // ruleUnmatchedEdit labels the fail-safe REVIEW a value-level change earns when + // NO enforce-effective prove rule selects it, under a binding that requires an + // obligation some rule proves (RVW-S02 / C-1): an edit outside every rule's + // match scope is not positively vouched, so it must never silently APPROVE. + ruleUnmatchedEdit = "aggregate.unmatchedEdit" ) // ReservedPolicyClass is the built-in meta-class (ADR-0008/ADR-0015 §1) that an diff --git a/internal/core/aggregate/coverage.go b/internal/core/aggregate/coverage.go index b3d9d4d..24e8e0d 100644 --- a/internal/core/aggregate/coverage.go +++ b/internal/core/aggregate/coverage.go @@ -265,6 +265,62 @@ func cover(pol *policy.MergePolicy, bind *policy.Binding, in *EvaluationInput, a }) } + // Unmatched-EDIT fail-safe escalation (RVW-S02 / report finding C-1). A + // value-level change (path!="") that NO enforce-effective prove rule selects, + // under a binding that requires an obligation some rule proves, is not + // positively vouched: the rule that names the required obligation marks it + // covered even when its `match` selects none of the governed changes (the + // covered[] bug at :150-152), so without this guard the change APPROVEs with an + // empty finding set. GUIDELINES §Safety-1 ("an empty, broken, or non-matching + // policy set never auto-merges anything; every change must be positively + // vouched") and D-142's REQ-DEM-S10-02 both intend REVIEW. This MIRRORS the + // D-063/D-064 unmatched-whole-file-DELETE escalation above and is ADDITIVE: it + // only ever RAISES the decision toward REVIEW via worse(), never relaxes it. + // + // Scope — value-level edits only. Whole-file (path=="") lifecycle events are + // the fileEvents domain: a delete is owned by the D-064 guard above, and an add + // is non-destructive (D-063), so neither escalates here. This keeps the D-016 + // golden's whole-file rename event (path=="") untouched. + // + // Gate — a required obligation must be "covered" by SOME enforce rule + // (covered[obl] is true). An empty/absent require is D-184's seam (the run path + // refuses it; the engine's vacuous-APPROVE reading is deliberately unchanged + // here), and a require with no proving rule already trips the uncovered guard + // above. Without this gate a policy with no obligation layer would escalate + // every value change — a behaviour change outside C-1's scope. + // + // "Governed" — some enforce-effective prove rule selects the change + // (editGoverned), exactly as the delete guard's fileDeleteGoverned does. + // Observe-phase (and pack-ceiling-observe) rules do NOT govern — their findings + // are decision-excluded, so treating them as governing would suppress escalation + // into APPROVE (the same D-063 fail-open the delete guard documents). One finding + // is emitted per governed SUBJECT that has at least one unvouched change + // (deduped, since a subject can carry many value changes and duplicate identical + // findings would be noise). + if requiredObligationCovered(bind, covered) { + escalated := map[string]bool{} + for _, ch := range in.ChangeSet.Changes { + if ch.Path == "" { + continue // whole-file lifecycle event: the fileEvents domain, not an edit + } + if escalated[ch.Subject] { + continue + } + if editGoverned(pol, ch, ceiling) { + continue // an enforce prove rule selects it -> already governed + } + escalated[ch.Subject] = true + decision = worse(decision, DecisionReview) + findings = append(findings, Finding{ + Rule: ruleUnmatchedEdit, + Effect: EffectRequireReview, + Subject: ch.Subject, + Points: 0, + Code: "change.unmatchedEdit", + }) + } + } + // Aggregation order #4 (ADR-0007, the LAST check): with block (#1), unresolved // challenge (#2), and uncovered/unproven obligations (#3) already reduced, a // points sum over the binding threshold escalates an otherwise-APPROVE decision @@ -593,6 +649,49 @@ func fileDeleteGoverned(pol *policy.MergePolicy, ch EvalChange, ceiling policy.P return false } +// requiredObligationCovered reports whether at least one obligation the binding +// REQUIRES is "covered" — i.e. some enforce-phase rule names it in prove.obligation +// (the covered[] map). It is the gate for the unmatched-EDIT escalation: with no +// required obligation proven-by-a-rule there is no obligation layer to vouch for, +// so the escalation stays silent (an empty/absent require is D-184's run-path +// seam; an uncovered require already trips the uncovered guard). Mirror of the +// fileDeleteGoverned enforce-only gate, at the binding level. +func requiredObligationCovered(bind *policy.Binding, covered map[string]bool) bool { + for _, obl := range bind.Require { + if covered[obl] { + return true + } + } + return false +} + +// editGoverned reports whether some ENFORCE-effective prove rule selects the +// value-level change ch — i.e. the edit is decision-governing. Mirrors +// fileDeleteGoverned exactly: ONLY effective PhaseEnforce may suppress the +// escalation (an observe rule's findings are structurally excluded from the +// decision, so treating observe as governing would suppress escalation → APPROVE, +// a D-063 fail-open). Like the delete guard it is obligation-agnostic: any +// enforce-phase prove rule that selects the change governs it (a non-required +// signal rule matching the change is still a real evaluation of it, and the points +// model depends on such a rule being able to leave the decision at APPROVE). Off / +// unknown / prove-nil all count as NOT governing. +func editGoverned(pol *policy.MergePolicy, ch EvalChange, ceiling policy.Phase) bool { + for i := range pol.Spec.Rules { + r := pol.Spec.Rules[i] + if r.Prove == nil { + continue + } + // Enforce-only — same gate as covered[] for obligation satisfaction. + if effectivePhase(r.Phase, ceiling) != policy.PhaseEnforce { + continue + } + if m, err := matchChanges(r.Match, []EvalChange{ch}); err == nil && len(m) > 0 { + return true + } + } + return false +} + // selectMatched returns, IN INPUT ORDER, the changes for which keep is true. func selectMatched(changes []EvalChange, keep func(EvalChange) bool) []EvalChange { var out []EvalChange diff --git a/internal/core/aggregate/coverage_test.go b/internal/core/aggregate/coverage_test.go index b9d9663..621e3f0 100644 --- a/internal/core/aggregate/coverage_test.go +++ b/internal/core/aggregate/coverage_test.go @@ -359,8 +359,22 @@ func TestCoverValuesMatch(t *testing.T) { if err != nil { t.Fatalf("Cover: %v", err) } - if len(got.Findings) != 1 || got.Findings[0].Code != "retention-shrunk" { - t.Fatalf("want one retention-shrunk block finding, got %+v", got.Findings) + // The Values domain selects /retentionMs (shrinking -> BLOCK) but NOT /other, + // so /other is governed by no enforce prove rule and earns the unmatched-edit + // escalation (RVW-S02 / C-1) in addition to the block. Assert both findings + // rather than a bare count: the point of this test is which pointer the domain + // selected, and the escalation is a separate, expected outcome for the other. + var blocked, escalated bool + for _, f := range got.Findings { + switch f.Code { + case "retention-shrunk": + blocked = f.Rule == "retention-monotonic" && f.Subject == "s:1" + case "change.unmatchedEdit": + escalated = f.Rule == ruleUnmatchedEdit && f.Effect == EffectRequireReview && f.Subject == "s:1" + } + } + if len(got.Findings) != 2 || !blocked || !escalated { + t.Fatalf("want the retention-shrunk block on the matched pointer plus the unmatched-edit escalation on /other, got %+v", got.Findings) } if got.Decision != DecisionBlock { t.Errorf("decision = %q, want BLOCK", got.Decision) @@ -429,11 +443,21 @@ func TestCoverValuesPathsFileGlob(t *testing.T) { if err != nil { t.Fatalf("Cover: %v", err) } - if len(got.Findings) != 1 { - t.Fatalf("want exactly one finding (staging file excluded by paths glob), got %+v", got.Findings) + // The prod file is selected (shrinking -> BLOCK). The staging file is excluded + // by the paths glob, so it is governed by no enforce prove rule and earns the + // unmatched-edit escalation (RVW-S02 / C-1) — the paths glob's exclusion is + // exactly why it is unvouched, not a licence to APPROVE it. + var blocked, escalated bool + for _, f := range got.Findings { + switch f.Code { + case "retention-shrunk": + blocked = f.Subject == "s:prod" && f.Rule == "prod-retention-monotonic" + case "change.unmatchedEdit": + escalated = f.Subject == "s:stg" && f.Rule == ruleUnmatchedEdit && f.Effect == EffectRequireReview + } } - if got.Findings[0].Subject != "s:prod" || got.Findings[0].Code != "retention-shrunk" { - t.Errorf("finding must be the prod subject only, got %+v", got.Findings[0]) + if len(got.Findings) != 2 || !blocked || !escalated { + t.Fatalf("want the prod block plus the unmatched-edit escalation on the excluded staging file, got %+v", got.Findings) } } diff --git a/internal/core/aggregate/unmatched_edit_test.go b/internal/core/aggregate/unmatched_edit_test.go new file mode 100644 index 0000000..245112d --- /dev/null +++ b/internal/core/aggregate/unmatched_edit_test.go @@ -0,0 +1,230 @@ +package aggregate + +import ( + "testing" + + "github.com/PlatformRelay/assent/internal/core/policy" +) + +// TestUnmatchedEditFailsSafeReview (RVW-S02, report finding C-1) pins the +// load-bearing fail-safe default: a value-level EDIT (path!="") that NO +// enforce-phase prove rule selects, under a binding that requires an obligation +// some rule proves, escalates the decision to at-least-REVIEW — NEVER APPROVE — +// so a change no rule vouches for can never silently ship. Before this guard the +// reproduction below returned APPROVE with an EMPTY finding set: the required +// obligation was marked covered by a rule whose `match` selected none of the +// governed changes, so the uncovered-obligation guard never fired. +// +// Mirrors TestUnmatchedFileDeleteFailsSafeReview (D-063/D-064). The escalation is +// value-level-EDIT only (whole-file deletes are the D-064 guard's subject, adds +// are non-destructive), and GOVERNED-aware (a change an enforce prove rule +// actually selects is NOT escalated). +func TestUnmatchedEditFailsSafeReview(t *testing.T) { + // The report's exact reproduction: the only rule proves non-destructive but + // matches valueChanges pointers ["/partitions"]; the governed change is at + // /replicas, so no rule selects it. + partitionRule := func(phase policy.Phase) *policy.MergePolicy { + return &policy.MergePolicy{ + Spec: policy.MergePolicySpec{ + Rules: []policy.Rule{{ + Name: "partitions-must-not-shrink", + Phase: phase, + Match: policy.Match{ValueChanges: &policy.ValueChangesMatch{Pointers: []string{"/partitions"}, Kinds: []string{"modify"}}}, + Prove: &policy.Prove{Obligation: "non-destructive", When: policy.AssertTree{Leaf: &policy.Leaf{CEL: "new >= old"}}}, + OnFailure: &policy.OnFailure{Effect: policy.EffectBlock, Code: "partition-count-shrunk"}, + }}, + }, + } + } + replicas := EvalChange{Subject: "topic-registry:orders.events.v1", File: "topics/prod/orders-events.yaml", Path: "/replicas", Kind: "modify", Old: intNum(3), New: intNum(6)} + replicasIn := &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{replicas}}} + // require non-destructive: some rule proves it, so it is "covered" and the + // uncovered guard does NOT fire — a passing REVIEW can only come from the new + // unmatched-edit escalation. + bind := &policy.Binding{Require: []string{"non-destructive"}, Environment: "prod"} + + hasUnmatchedEdit := func(fs []Finding) bool { + for _, f := range fs { + if f.Rule == ruleUnmatchedEdit { + return true + } + } + return false + } + + t.Run("REPRODUCTION: an unmatched value-level edit -> REVIEW, not APPROVE", func(t *testing.T) { + got, err := Cover(partitionRule(policy.PhaseEnforce), bind, replicasIn) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionReview { + t.Fatalf("an unmatched edit must fail safe -> REVIEW, got %q (%+v)", got.Decision, got.Findings) + } + if len(got.Findings) != 1 { + t.Fatalf("want exactly one escalation finding, got %+v", got.Findings) + } + f := got.Findings[0] + if f.Rule != ruleUnmatchedEdit || f.Effect != EffectRequireReview || f.Subject != replicas.Subject || f.Code != "change.unmatchedEdit" { + t.Fatalf("escalation finding shape wrong: %+v", f) + } + }) + + t.Run("an unmatched edit outside every rule's FILE glob -> REVIEW", func(t *testing.T) { + // A file-glob rule over topics/** never selects services/x.yaml. + pol := &policy.MergePolicy{ + Spec: policy.MergePolicySpec{ + Rules: []policy.Rule{{ + Name: "topics-only", + Phase: policy.PhaseEnforce, + Match: policy.Match{Files: &policy.FilesMatch{Paths: []string{"topics/**"}}}, + Prove: &policy.Prove{Obligation: "non-destructive", When: policy.AssertTree{Leaf: &policy.Leaf{CEL: "new >= old"}}}, + OnFailure: &policy.OnFailure{Effect: policy.EffectBlock, Code: "shrunk"}, + }}, + }, + } + svc := EvalChange{Subject: "service:orders", File: "services/x.yaml", Path: "/replicas", Kind: "modify", Old: intNum(1), New: intNum(2)} + got, err := Cover(pol, bind, &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{svc}}}) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionReview || !hasUnmatchedEdit(got.Findings) { + t.Fatalf("an edit outside every file glob must escalate to REVIEW, got %q (%+v)", got.Decision, got.Findings) + } + }) + + t.Run("GOVERNED: a matched clean-true edit -> APPROVE, no escalation", func(t *testing.T) { + matched := EvalChange{Subject: "topic-registry:orders.events.v1", File: "topics/prod/orders-events.yaml", Path: "/partitions", Kind: "modify", Old: intNum(3), New: intNum(6)} + got, err := Cover(partitionRule(policy.PhaseEnforce), bind, &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{matched}}}) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionApprove || hasUnmatchedEdit(got.Findings) { + t.Fatalf("a matched, clean-true edit must prove the obligation -> APPROVE with no escalation, got %q (%+v)", got.Decision, got.Findings) + } + }) + + t.Run("GOVERNED: a matched clean-FALSE edit fires the rule, not the escalation", func(t *testing.T) { + matched := EvalChange{Subject: "topic-registry:orders.events.v1", File: "topics/prod/orders-events.yaml", Path: "/partitions", Kind: "modify", Old: intNum(6), New: intNum(3)} + got, err := Cover(partitionRule(policy.PhaseEnforce), bind, &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{matched}}}) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionBlock || hasUnmatchedEdit(got.Findings) { + t.Fatalf("a governed edit must earn its OWN rule effect (BLOCK), not the escalation, got %q (%+v)", got.Decision, got.Findings) + } + if len(got.Findings) != 1 || got.Findings[0].Code != "partition-count-shrunk" { + t.Fatalf("want exactly the authored block finding, got %+v", got.Findings) + } + }) + + // The gate: with NO required obligation proven by any rule there is no + // obligation layer to vouch for, so the escalation stays silent. This preserves + // the engine's documented vacuous-APPROVE reading for an empty require (D-184 + // owns the run-path refusal) and keeps the D-016 golden's semantics intact. + t.Run("no require (no obligation layer) -> no escalation, APPROVE", func(t *testing.T) { + got, err := Cover(partitionRule(policy.PhaseEnforce), &policy.Binding{Environment: "prod"}, replicasIn) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionApprove || hasUnmatchedEdit(got.Findings) { + t.Fatalf("with no require the escalation must not fire, got %q (%+v)", got.Decision, got.Findings) + } + }) + + t.Run("require with no proving rule -> uncovered guard, not the edit escalation", func(t *testing.T) { + // The rule proves a NON-required obligation, so nothing the binding requires + // is "covered" -> the escalation is gated off; the uncovered guard REVIEWs. + pol := &policy.MergePolicy{ + Spec: policy.MergePolicySpec{ + Rules: []policy.Rule{{ + Name: "unrelated", + Phase: policy.PhaseEnforce, + Match: policy.Match{Files: &policy.FilesMatch{Paths: []string{"topics/**"}}}, + Prove: &policy.Prove{Obligation: "unrelated", When: policy.AssertTree{Leaf: &policy.Leaf{CEL: "true"}}}, + OnFailure: &policy.OnFailure{Effect: policy.EffectBlock, Code: "c"}, + }}, + }, + } + got, err := Cover(pol, bind, replicasIn) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionReview || hasUnmatchedEdit(got.Findings) { + t.Fatalf("an uncovered require must REVIEW via the uncovered guard only, got %q (%+v)", got.Decision, got.Findings) + } + if len(got.Findings) != 1 || got.Findings[0].Rule != ruleUncovered { + t.Fatalf("want exactly one uncovered finding, got %+v", got.Findings) + } + }) + + // Scope: whole-file lifecycle events belong to the fileEvents domain. A delete + // is the D-064 guard's subject (its own finding), an add is non-destructive. + // An empty binding isolates the scope check from the obligation layer. + noRequire := &policy.Binding{Environment: "prod"} + t.Run("whole-file DELETE uses the D-064 guard, not the edit escalation", func(t *testing.T) { + del := EvalChange{Subject: "file:topics/orders.yaml", File: "topics/orders.yaml", Path: "", Kind: "delete"} + got, err := Cover(&policy.MergePolicy{}, noRequire, &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{del}}}) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionReview || hasUnmatchedEdit(got.Findings) { + t.Fatalf("a whole-file delete must escalate via D-064 only, got %q (%+v)", got.Decision, got.Findings) + } + if len(got.Findings) != 1 || got.Findings[0].Rule != ruleUnmatchedDelete { + t.Fatalf("want exactly the D-064 delete finding, got %+v", got.Findings) + } + }) + + t.Run("whole-file ADD is not an edit -> no escalation", func(t *testing.T) { + add := EvalChange{Subject: "file:topics/orders.yaml", File: "topics/orders.yaml", Path: "", Kind: "add"} + got, err := Cover(&policy.MergePolicy{}, noRequire, &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{add}}}) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionApprove || hasUnmatchedEdit(got.Findings) { + t.Fatalf("a whole-file add must not escalate (non-destructive, D-063), got %q (%+v)", got.Decision, got.Findings) + } + }) + + // OBSERVE must NOT suppress the escalation: observe findings are structurally + // excluded from the decision, so treating observe as "governed" would suppress + // escalation → APPROVE — the same D-063 fail-open the delete guard documents. + // The enforce rule covers the required obligation (so the gate opens) but does + // not select the change; the observe rule selects it and must NOT govern. + t.Run("an OBSERVE-phase rule does NOT govern -> REVIEW", func(t *testing.T) { + pol := partitionRule(policy.PhaseEnforce) + obs := partitionRule(policy.PhaseObserve).Spec.Rules[0] + obs.Name = "observe-replicas" + obs.Match = policy.Match{ValueChanges: &policy.ValueChangesMatch{Pointers: []string{"/replicas"}, Kinds: []string{"modify"}}} + pol.Spec.Rules = append(pol.Spec.Rules, obs) + got, err := Cover(pol, bind, replicasIn) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if got.Decision != DecisionReview || !hasUnmatchedEdit(got.Findings) { + t.Fatalf("an observe-phase rule must not suppress the unmatched-edit escalation, got %q findings=%+v observed=%+v", got.Decision, got.Findings, got.Observed) + } + }) + + // One finding per unvouched SUBJECT: a subject carrying several unmatched + // changes must not emit duplicate identical findings. + t.Run("several unmatched changes on one subject -> one finding", func(t *testing.T) { + in := &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{ + {Subject: "s:1", File: "a.yaml", Path: "/replicas", Kind: "modify", Old: intNum(1), New: intNum(2)}, + {Subject: "s:1", File: "a.yaml", Path: "/shards", Kind: "modify", Old: intNum(1), New: intNum(2)}, + }}} + got, err := Cover(partitionRule(policy.PhaseEnforce), bind, in) + if err != nil { + t.Fatalf("Cover: %v", err) + } + n := 0 + for _, f := range got.Findings { + if f.Rule == ruleUnmatchedEdit { + n++ + } + } + if n != 1 { + t.Fatalf("want one escalation finding per subject, got %d in %+v", n, got.Findings) + } + }) +} diff --git a/openspec/specs/backlog.md b/openspec/specs/backlog.md index 387ecb1..9ab95a7 100644 --- a/openspec/specs/backlog.md +++ b/openspec/specs/backlog.md @@ -726,12 +726,15 @@ A six-leg review (adversarial + differential, all at the same HEAD) found a sche obligation layer is vacuous — while `GUIDELINES.md` §Safety-1 and the repo's own `record.go` seam note promise the opposite. RVW-S01 closes that contradiction: the CLI refuses to arm on an empty `require`, `assent lint` hard-errors it, and D-184 supersedes the schema/D-021 "vacuously -covered" wording. Sibling review rows are decomposed as they are claimed; the unmatched-edit -fail-open is a separate row gated on a held captain decision. +covered" wording. The adversarial leg's C-1 is the sibling fail-open: a rule that names a +required obligation but whose `match` selects none of the governed changes still marks it +covered, so an unmatched edit APPROVEs with zero findings. RVW-S02 closes it with an additive +escalation mirroring D-063/D-064. | ID | Story | Execution | Depends on | Gate contribution | | --- | --- | --- | --- | --- | | RVW-S01 | ⚠️ empty `require:` never arms APPROVE: run-path guard + lint `binding-require-empty` + polarity test + D-184 reconciliation | **[autonomous · engine-adjacent]** | none | **do first** — closes a reproduced APPROVE-not-proven fail-open on the shipped run path | +| RVW-S02 | ⚠️ unmatched edit never arms APPROVE: additive REVIEW escalation mirroring D-063/D-064 + regression test + corpus correction + D-185 | **[autonomous · engine-adjacent]** | none | closes the adversarial review's C-1 fail-open (`data/assent-adv-dsk/report.md`) | ## Phase 5 — WG `writes: false` runtime gate (D-145) diff --git a/openspec/specs/p5-rvw-review-remediation/spec.md b/openspec/specs/p5-rvw-review-remediation/spec.md index 14fad2f..da59419 100644 --- a/openspec/specs/p5-rvw-review-remediation/spec.md +++ b/openspec/specs/p5-rvw-review-remediation/spec.md @@ -1,4 +1,4 @@ -# P5-RVW — six-review remediation: empty `require` must never arm APPROVE +# P5-RVW — six-review remediation: empty `require` and unmatched edits never arm APPROVE **Epic ID / REQ prefix:** `RVW` / `REQ-RVW-Snn-nn`. Cross-cutting remediation epic (same naming class as `AUD` / `AUD2` / `PCS` / `SEC-SC`). Does **not** consume E10–E14. @@ -22,23 +22,32 @@ list, so an empty list checks nothing). `cmd/assent/run.go`'s `selectBinding` ch binding *count* only. The result is a gate that stays clean on a document the schema accepts and lint passes, while silently converting to a rubber stamp. -**Decision (this epic, D-184):** the invariant wins. An empty `require` is a policy-authoring -defect, and the CLI refuses to arm on it (fail closed, no forge write). The schema description -and D-021's "vacuously covered" clause are reconciled to the invariant by superseding decision -row; the schema's `minItems: 1` is deferred to its next change window, because the frozen -`schemas/**/*.json` artifacts are under a ref-relative freeze guard (D-132) that permits no -edit here. - -**Not in scope:** the unmatched-edit fail-open (a rule that names an obligation but whose -`match` selects nothing still marks it covered) — that is a separate register row and is gated -on a held captain decision; it shares the `cover()` invariant but not this lane's fix. The -`minItems: 1` schema change. Making `assent compare` refuse an empty-require candidate: the -comparison tool exists to compare permissive candidates, so it is deliberately left able to -read one. `assent test` (the adopter harness) does not arm APPROVE and is unchanged. +**Second problem (S02, the adversarial review's C-1)**: an **unmatched edit** fails open. +`cover()` marks a required obligation *covered* as soon as any enforce-phase rule names it in +`prove.obligation` — before it computes `matchChanges` — so a rule that names the obligation +but whose `match` selects none of the governed changes still marks it covered, contributes no +finding, and the decision falls through to APPROVE with an empty finding set. A value-level +edit outside every proving rule's match scope is not positively vouched, which GUIDELINES §1 +and D-142's REQ-DEM-S10-02 both intend to be REVIEW. Reproduced in +`data/assent-adv-dsk/report.md` finding C-1. + +**Decision (this epic):** the invariant wins. D-184 closes the empty `require` (S01). D-185 +closes the unmatched edit (S02): an additive fail-safe escalation mirroring the D-063/D-064 +unmatched-whole-file-DELETE guard. The frozen schema's `minItems: 1` is deferred to its next +change window, because the frozen `schemas/**/*.json` artifacts are under a ref-relative freeze +guard (D-132) that permits no edit here. + +**Not in scope:** the `minItems: 1` schema change. Making `assent compare` refuse an +empty-require candidate: the comparison tool exists to compare permissive candidates, so it is +deliberately left able to read one. `assent test` (the adopter harness) does not arm APPROVE +and is unchanged. For S02, whole-file lifecycle events: a whole-file DELETE is the D-063/D-064 +guard's subject and a whole-file ADD is non-destructive (D-063), so neither is an "edit". **Lanes:** **A** run-path guard + polarity test (`cmd/assent/run.go`, `cmd/assent/run_test.go`) · **B** lint hard error + fixture pair (`internal/lint`, `examples/lint-fixtures`) · -**C** reconciliation docs/decision row. +**C** reconciliation docs/decision row. **S02** engine escalation + regression test +(`internal/core/aggregate`), with the comparison corpus baseline corrected to govern the edit +it previously auto-approved. --- @@ -93,3 +102,62 @@ Requirements: APPROVE path on the run path. Test: those files; Verify: `rg 'binding-require-empty' docs/planning/lint-hard-errors.md && rg 'D-184' docs/decisions/decisions.md`; Level: doc + +--- + +## RVW-S02 — an unmatched edit never arms APPROVE `[autonomous · engine-adjacent]` + +As an adopter, I want a value-level change that no enforcing rule selects, under a binding +that requires an obligation some rule proves, to escalate to REVIEW rather than silently +APPROVE, so that a rule whose `match` is narrower than the binding's `require` cannot turn my +merge gate into an auto-approver for every change outside its scope. + +**Depends on:** none. Engine lane; decision-path adjacent, independently reviewed. + +Acceptance criteria: + +- Given a binding that requires an obligation some enforce-phase rule proves, and a value-level + change (`path != ""`) that no enforce-phase rule's `match` selects, when `Cover` evaluates, + then the decision is **at least REVIEW** and exactly one synthetic `aggregate.unmatchedEdit` + finding (`code: change.unmatchedEdit`, effect `require-review`) is emitted for the unvouched + subject — it is never APPROVE with zero findings. +- Given the same binding and a change an enforce-phase rule **does** select, then the + escalation does **not** fire: a clean-true match proves the obligation (APPROVE) and a + clean-false match earns the rule's own effect. +- **Gate** — an empty/absent `require` (D-184's run-path seam) and a `require` with no proving + rule (the uncovered-obligation guard's subject) do not trigger this escalation. +- **Scope** — whole-file lifecycle events are excluded: an ungoverned whole-file DELETE keeps + the D-063/D-064 `aggregate.unmatchedDelete` escalation and an ungoverned whole-file ADD is + non-destructive, so neither emits `aggregate.unmatchedEdit`. +- **Observe does not govern** — an observe-phase rule (or an enforce rule capped to observe by + the pack ceiling) selecting the change does NOT suppress the escalation, because its findings + are structurally excluded from the decision (the same D-063 fail-open the delete guard names). +- **Additive** — the escalation only ever raises the decision toward REVIEW via `worse()`; it + never relaxes a BLOCK and never changes the decision of a fully governed changeset. + +**Definition of done:** the escalation lands in `internal/core/aggregate/coverage.go` with +`editGoverned` and `requiredObligationCovered`; `unmatched_edit_test.go` reproduces the +zero-finding APPROVE and proves it now REVIEWs, with both polarities and the scope/observe +cases; D-185 records the mechanism; `task check` green. + +**Not in scope:** as the epic's *Not in scope* above. + +Requirements: + +- **REQ-RVW-S02-01** *(engine escalation)* — an unmatched value-level edit under a binding that + requires a rule-proven obligation escalates to REVIEW with one `aggregate.unmatchedEdit` + finding. Test: `internal/core/aggregate/unmatched_edit_test.go`; Verify: + `go test ./internal/core/aggregate -run TestUnmatchedEditFailsSafeReview -count=1`; Level: L0 +- **REQ-RVW-S02-02** *(governed polarity)* — a matched edit is untouched (clean-true → APPROVE, + clean-false → the rule's effect). Test: `internal/core/aggregate/unmatched_edit_test.go`; + Verify: same; Level: L0 +- **REQ-RVW-S02-03** *(gate + scope + observe)* — no-require, uncovered-require, whole-file + delete/add and observe-phase cases do not emit `aggregate.unmatchedEdit`. Test: + `internal/core/aggregate/unmatched_edit_test.go`; Verify: same; Level: L0 +- **REQ-RVW-S02-04** *(corpus correction · doc)* — the `promotion-gates` baseline profile + governs `/retentionMs` with a permissive rule, because the `challenge-intervention-added` + case previously depended on the fail-open (an ungoverned edit auto-approving); D-185 records + the mechanism. Test: `examples/comparison/promotion-gates/baseline.yaml`, + `docs/decisions/decisions.md`; Verify: + `go test ./examples/comparison/... -run TestCompareCorpusRunsGreen -count=1`; Level: L1 + From f9abaa34e9b12682f0de22bbd04f7ea7fbe23508 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 27 Sep 2026 00:34:53 +0200 Subject: [PATCH 3/3] no-mistakes(document): docs: record D-185 superseding D-184's unmatched-edit clause --- docs/decisions/decisions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index f6329b2..7e36721 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -188,4 +188,4 @@ project/process decisions. | D-182 | 2026-09-25 | **`mr.labels` is WIRED through the live `assent run` path rather than retired by a lint hard error. The frozen predicate-scope contract keeps its label field, and the fail-open is closed by making the forge fetch honest.** The six-leg review found `mr.labels` schema-carried (`evaluation-input.schema.json`), engine-bound (`aggregate/evaluate.go` binds `labels`) and harness-tested (`internal/adoptertest/mr_test.go`) but never fetched on the live path: `cmd/assent/run.go:581` `mrFrom` built `aggregate.MR` with no `Labels`, and the GitLab adapter's two MR decoders (`GetMR` in `internal/forge/gitlab/gitlab.go` and `mrWithAuthor` in `snapshot.go`) ignored the API's `labels` array. **Consequence, and why it is a safety defect not a missing feature:** every label predicate in production evaluated against an empty list, so a NEGATIVE guard (`!('security-hold' in mr.labels)`) was vacuously true and a `security-hold` MR could APPROVE and merge. **Decision — the alternative is recorded and rejected:** a lint hard error rejecting any `mr.labels` reference plus a `predicate-scope.md` amendment would have made the contract truthful by deleting a capability adopters were already sold; wiring keeps the contract and costs two JSON decodes. **Implementation:** `forge.MRInfo` gains `Labels`; `GetMR` and `mrWithAuthor` decode the string array (absent field → nil, never an error); `Snapshot` surfaces them on `forge.MRHeads.Labels`; `mrFrom` copies `info.Labels` into `aggregate.MR.Labels`. **Proven, not asserted:** a run-path test through the real GitLab adapter over `httptest` (`fakeGitLab`) shows a labelled MR reaching the engine (a label-proven obligation APPROVEs with the label, REVIEWs without it) and the negative-guard polarity failing CLOSED (the guarded label yields BLOCK with zero approve/merge writes). Spec: `openspec/specs/p5-e4-gitlab-forge/spec.md` REQ-E4-S02-05 + REQ-E4-S06-08. Revert: drop `Labels` from `MRInfo`/`MRHeads`, the two decodes and the `mrFrom` copy — reopens the empty-label fail-open for every negative guard. | | D-183 | 2026-09-25 | **The six-review round's first fix slice lands: `assent run` reads every judged byte at the pinned commit SHA, not the mutable branch name (U-04 item 1).** Origin: the 2026-09-25 six-leg review (`data/assent-unify/report.md` §4.1 U-04, §6 lane B1; `data/assent-diff-qfn/report.md` QFN-01) at HEAD `b8123cc`. **The defect, stated precisely:** `orchestrate` pins `info.SourceSHA`/`info.TargetSHA` from `GetMR` (`run.go:177`), and the approval and `merge?sha=` CAS are pinned to those commits — but the six content reads on the live path resolved a mutable branch name (`run.go:203,211,230,249,270` = `info.TargetBranch`; `:274` = `info.SourceBranch`). The invariant the merge leans on, *final head == pin ⇒ final head == what was judged*, is therefore false: a contributor with push rights on the MR branch pushes benign X2 (judged), then force-pushes back to pinned X1 before `Reconcile`'s re-read, the SHA-guard passes (head == pin), and the forge merges X1 — bytes never evaluated. The `DecisionRecord` pins `policySha/sourceSha/targetSha` but no content digest, so the record cannot distinguish the states either; ADR-0015 §2's "Merge is SHA-guarded (no TOCTOU)" heading describes the *branch-moved* guard, not a bytes-equal-to-pin guarantee. **The change:** pass `info.TargetSHA` at `:203`, `:211`, `:230`, `:249`, `:270` and `info.SourceSHA` at `:274`. The GitLab adapter already accepts a commit SHA as `?ref=` (`gitlab.go:474`, the same URL-escaped parameter), so this is one argument at six sites — no adapter API change, no new discriminator, no reordering. Target-side reads are pinned too (judgment call (a)): not strictly required for the source-side attack, but it removes the maintainer-push-window ambiguity on the policy/config/pack loads and leaves one uniform rule rather than an asymmetry a later reader must re-justify. **The tests are FLIPPED, not merely updated (judgment call (b)):** the run-path fake's file router now serves the pinned SHAs and errors on any other ref, and `TestFileAtRef` asserts the SHA it sends — so a regression to `info.TargetBranch`/`info.SourceBranch` reddens the existing suite rather than passing against a fake that still accepts branch names. `run_self_vouch_test.go:70`'s policy-load ref assertion moves from `f.target` to `f.targetTip` for the same reason. **The move-and-restore case is in two places, deliberately (judgment call (c)):** the `internal/forge/conformance` case `sha-guard-source-moved-and-restored` (with a new `Fixture.MoveSourceHead` seam and a `catalog.yaml` row) drives `forge.Reconcile` only, so it proves the CAS **merges once the head is back at the pin** — i.e. the CAS alone cannot catch a move-and-restore, which is exactly why the read must be pinned; the run-path case `TestRunJudgesPinnedSHAWhenBranchMoves` proves the read itself follows the pin (branch points at benign bytes, pin at violating bytes, decision follows the pin). **Strictly stronger pinning, no behaviour change beyond it:** the same bytes are read when the branch has not moved. **Explicitly NOT in this lane:** U-04 items 2–4 (SHA-bound `/repository/compare` enumeration re-fold — shares its mechanism with the D-139 trio, lane B5; the `CI_MERGE_REQUEST_SOURCE_BRANCH_SHA` belt; the additive `headContentSha` pin, lane B9), the `--checkout` tree↔SHA binding (U-08/QFN-04, lane B5), the `forgePort` shape (E10), and the `docs/usage/cli.md` "runs without `--checkout` are not exposed" claim (its enumeration half is item 2). Spec: `openspec/specs/p5-rev1-pinned-sha/spec.md`. **Revert:** restore `info.TargetBranch`/`info.SourceBranch` at the six sites and un-flip the fake routers — which re-opens the move-and-restore merge of un-evaluated bytes on the checkout-less path the docs call safe. | | D-184 | 2026-09-25 | **RVW-S01 — an empty/absent binding `require:` must never arm APPROVE: the run path refuses it, `assent lint` hard-errors `binding-require-empty`, and this row supersedes D-021's "vacuously covered" clause. The frozen schema's `minItems: 1` is deferred.** **Finding (reproduced on the run path):** a schema-valid `RulesetBinding` binding with `require: []` (or no `require:` key) decides **APPROVE with zero findings** for any governed change its block rules do not fire on. The obligation layer iterates `bind.Require`, so an empty list makes it vacuous; with the forge-probed arming preconditions met the run then approves and merges. The schema accepts it (`require` is not in the binding's `required` list and carries no `minItems`; `schemas/policy/v1alpha1/ruleset-binding.schema.json:52-57`) and lint passed it (`checkObligationCoverage` iterates the same empty list), while two in-repo authorities say the opposite: `GUIDELINES.md` §Safety-1 ("Every change must be positively vouched") and `internal/core/decision/record.go`'s own seam note (S03 review F3: the CLI wiring "must guarantee require is non-empty before an APPROVE is armed"). D-021 added `require` as optional and recorded "absent/empty ⇒ no required obligations (vacuously covered)" — a true statement about the schema's shape that was read as a safety property, and was never one. **Decision:** the invariant wins. (a) **Run path** — `decide`'s decidable, non-reserved arm returns a hard error when the covering binding's `require` is empty; the run exits `1` with a contributor-readable message naming the binding `(class, environment)`, and **zero forge writes** happen (the guard sits before `buildEvaluationInput`, so no record is emitted either). It is deliberately NOT placed in `selectBinding`: that function is shared with `assent compare`, whose whole purpose is to compare permissive candidates, and refusing to compare an unsafe candidate would defeat it. (b) **Lint** — new hard error `binding-require-empty`, located to the binding, with a `good`/`bad` fixture pair registered in the E3-S08 `hardErrorCorpus`. (c) **Reserved-class exemption, stated because it is a real carve-out:** the reserved `assent-policy` meta-class is block-by-default (ADR-0015 §1) and its safety comes from GUARD 1 (any `.assent/**` edit BLOCKs), not from obligation coverage — and it *cannot* carry a non-empty `require` at all, because any `prove` rule in a reserved-bound pack routes the policy class to a vouch/APPROVE-arming disposition the reserved-class check forbids. The lint check therefore skips `class == assent-policy`; the run-path guard still refuses an empty require whenever the SUBJECT is not the reserved class. (d) **Reconciliation** — the schema description's "Absent or empty ⇒ no required obligations (vacuously covered)" and D-021's identical clause are superseded as *safety* statements by this row; D-021 is left byte-unchanged (supersede, don't edit); D-134's ARM-08 run-path consequence ("with no `require:` on the binding it turns a BLOCK into APPROVE+merge") is superseded the same way — the engine still decides APPROVE for an empty `require`, but `decide` (a) refuses before it, so that approve+merge path is gone. The schema's `minItems: 1` is **deferred to the next schema change window**: `schemas/**/*.json` is under the ref-relative freeze guard (D-132) against the released `v0.1.0` tag, which permits exactly one diff (the D-120 `toolDigest` description) and no other, so editing the frozen file in this lane would red `hack/audit/exitgate_test.sh`. Until that window, (a)+(b) are the enforcement. **Not in scope, deliberately:** the unmatched-edit fail-open (a rule that names an obligation but whose `match` selects nothing still marks it covered) shares the `cover()` invariant but is its own register row and is gated on a held captain decision; `assent test` does not arm APPROVE and is unchanged; `assent compare` is unchanged per (a). **Scope note:** `examples/lint-fixtures/{fail-open,reserved-class}/` were touched — `fail-open`'s bindings gained `require: [reviewed]` (the rule already proved it), and `reserved-class/good` is covered by the exemption; both were previously "clean" only because the check did not exist. **Revert:** delete `checkBindingRequireEmpty` and its corpus row, drop the `decide` guard, restore the two `fail-open` bindings, and the vacuous APPROVE returns. | -| D-185 | 2026-09-26 | **RVW-S02 — an unmatched EDIT never arms APPROVE: an additive fail-safe escalation in `cover()` mirrors the D-063/D-064 unmatched-whole-file-DELETE guard, and the `promotion-gates` comparison baseline is corrected because one of its cases had accidentally depended on the fail-open.** **Finding (report C-1, reproduced):** `cover()` marks a required obligation *covered* as soon as any enforce-phase rule names it in `prove.obligation` — BEFORE it computes `matchChanges` (`internal/core/aggregate/coverage.go:150-152` vs `:154-160`). So a rule that names the obligation but whose `match` selects none of the governed changes still marks it covered, contributes no finding, and the decision falls through to APPROVE with an EMPTY finding set: a policy whose only rule matches `valueChanges.pointers: ["/partitions"]` and proves `non-destructive`, under `require: [non-destructive]`, APPROVED a change at `/replicas` with zero findings. GUIDELINES §Safety-1 and D-142's REQ-DEM-S10-02 both intend REVIEW. The intended runtime backstop, `classify.ValidateRouting`, has no production caller and `assent run` never runs lint, so nothing else catches it. **Decision (captain: option (a), "add an unmatched-edit escalation to REVIEW, mirroring the D-064 unmatched-delete guard"):** after the rule loop, for each VALUE-LEVEL change (`ch.Path != ""`), if no enforce-effective prove rule selects it, `worse(decision, REVIEW)` and emit a synthetic `aggregate.unmatchedEdit` finding (`effect: require-review`, `code: change.unmatchedEdit`, subject the change's subject). **Scope — value-level edits only:** whole-file (`path==""`) lifecycle events are the fileEvents domain — a delete is the D-064 guard's subject, an add is non-destructive (D-063) — so neither escalates here; this also leaves the D-016 golden's whole-file rename event untouched. **Gate:** the escalation runs only when `requiredObligationCovered` (some obligation the binding requires has an enforce proving rule, i.e. `covered[obl]`). An empty/absent `require` is D-184's run-path seam and the engine's vacuous-APPROVE reading is deliberately unchanged; a `require` with no proving rule already trips the uncovered guard. Without the gate a policy with no obligation layer would escalate every value change — outside C-1's scope. **"Governed" (`editGoverned`) mirrors `fileDeleteGoverned` exactly:** some enforce-effective prove rule selects the change, obligation-agnostic (a non-required signal rule that matches is a real evaluation, and the points model depends on it being able to leave the decision at APPROVE). Observe-phase (and pack-ceiling-observe) rules do NOT govern — their findings are decision-excluded, so treating them as governing would suppress escalation into APPROVE, the same D-063 fail-open the delete guard names. One finding per unvouched SUBJECT (deduped; a subject can carry many value changes and duplicate identical findings would be noise). **Additive and deterministic:** it only ever raises toward REVIEW via `worse()`, never relaxes a BLOCK, and the findings are canonically sorted; a fully governed changeset is byte-identical. **Corpus correction, stated because it is a real behaviour change to a shipped example:** `examples/comparison/promotion-gates/cases/challenge-intervention-added` changed `/retentionMs` under a baseline (`prod-strict@6`) with NO rule matching that pointer, so it APPROVED only because of this fail-open; with the guard the baseline REVIEWs and the case's `stricter-intervention-added` delta disappears. The baseline gains a permissive `retention-permissive` rule (proves a required obligation cleanly true), so the baseline genuinely governs and permits the edit and the candidate's `retention-ack` challenge is a real delta again — the record is byte-identical. **Reproduced then fixed:** the pre-fix `Cover` returned `APPROVE findings=[]`; the regression test `TestUnmatchedEditFailsSafeReview` pins the REVIEW + `aggregate.unmatchedEdit` finding, both polarities, the gate, the scope and the observe cases. Spec: `openspec/specs/p5-rvw-review-remediation/spec.md` RVW-S02. **Revert:** delete the escalation block + `editGoverned`/`requiredObligationCovered` + `ruleUnmatchedEdit`, restore `baseline.yaml`, and the unmatched edit silently APPROVEs again. | +| D-185 | 2026-09-26 | **RVW-S02 — an unmatched EDIT never arms APPROVE: an additive fail-safe escalation in `cover()` mirrors the D-063/D-064 unmatched-whole-file-DELETE guard, and the `promotion-gates` comparison baseline is corrected because one of its cases had accidentally depended on the fail-open.** **Finding (report C-1, reproduced):** `cover()` marks a required obligation *covered* as soon as any enforce-phase rule names it in `prove.obligation` — BEFORE it computes `matchChanges` (`internal/core/aggregate/coverage.go:150-152` vs `:154-160`). So a rule that names the obligation but whose `match` selects none of the governed changes still marks it covered, contributes no finding, and the decision falls through to APPROVE with an EMPTY finding set: a policy whose only rule matches `valueChanges.pointers: ["/partitions"]` and proves `non-destructive`, under `require: [non-destructive]`, APPROVED a change at `/replicas` with zero findings. GUIDELINES §Safety-1 and D-142's REQ-DEM-S10-02 both intend REVIEW. The intended runtime backstop, `classify.ValidateRouting`, has no production caller and `assent run` never runs lint, so nothing else catches it. **Decision (captain: option (a), "add an unmatched-edit escalation to REVIEW, mirroring the D-064 unmatched-delete guard"):** after the rule loop, for each VALUE-LEVEL change (`ch.Path != ""`), if no enforce-effective prove rule selects it, `worse(decision, REVIEW)` and emit a synthetic `aggregate.unmatchedEdit` finding (`effect: require-review`, `code: change.unmatchedEdit`, subject the change's subject). **Scope — value-level edits only:** whole-file (`path==""`) lifecycle events are the fileEvents domain — a delete is the D-064 guard's subject, an add is non-destructive (D-063) — so neither escalates here; this also leaves the D-016 golden's whole-file rename event untouched. **Gate:** the escalation runs only when `requiredObligationCovered` (some obligation the binding requires has an enforce proving rule, i.e. `covered[obl]`). An empty/absent `require` is D-184's run-path seam and the engine's vacuous-APPROVE reading is deliberately unchanged; a `require` with no proving rule already trips the uncovered guard. Without the gate a policy with no obligation layer would escalate every value change — outside C-1's scope. **"Governed" (`editGoverned`) mirrors `fileDeleteGoverned` exactly:** some enforce-effective prove rule selects the change, obligation-agnostic (a non-required signal rule that matches is a real evaluation, and the points model depends on it being able to leave the decision at APPROVE). Observe-phase (and pack-ceiling-observe) rules do NOT govern — their findings are decision-excluded, so treating them as governing would suppress escalation into APPROVE, the same D-063 fail-open the delete guard names. One finding per unvouched SUBJECT (deduped; a subject can carry many value changes and duplicate identical findings would be noise). **Additive and deterministic:** it only ever raises toward REVIEW via `worse()`, never relaxes a BLOCK, and the findings are canonically sorted; a fully governed changeset is byte-identical. **Corpus correction, stated because it is a real behaviour change to a shipped example:** `examples/comparison/promotion-gates/cases/challenge-intervention-added` changed `/retentionMs` under a baseline (`prod-strict@6`) with NO rule matching that pointer, so it APPROVED only because of this fail-open; with the guard the baseline REVIEWs and the case's `stricter-intervention-added` delta disappears. The baseline gains a permissive `retention-permissive` rule (proves a required obligation cleanly true), so the baseline genuinely governs and permits the edit and the candidate's `retention-ack` challenge is a real delta again — the record is byte-identical. **Reproduced then fixed:** the pre-fix `Cover` returned `APPROVE findings=[]`; the regression test `TestUnmatchedEditFailsSafeReview` pins the REVIEW + `aggregate.unmatchedEdit` finding, both polarities, the gate, the scope and the observe cases. **Supersession:** this row closes the clause D-184's *Not in scope* left open ("the unmatched-edit fail-open … is its own register row and is gated on a held captain decision"); D-184 is left byte-unchanged (supersede, don't edit). Spec: `openspec/specs/p5-rvw-review-remediation/spec.md` RVW-S02. **Revert:** delete the escalation block + `editGoverned`/`requiredObligationCovered` + `ruleUnmatchedEdit`, restore `baseline.yaml`, and the unmatched edit silently APPROVEs again. |