diff --git a/.github/workflows/verify.yaml b/.github/workflows/verify.yaml index 5df552f..ee440cc 100644 --- a/.github/workflows/verify.yaml +++ b/.github/workflows/verify.yaml @@ -57,16 +57,17 @@ jobs: go test -count=2 ./internal/forge/conformance/... -run 'TestConformanceTargetAdvancedRejected|TestConformanceRerunIdempotence|TestConformanceDuplicateRepair' go test -count=2 ./internal/forge/... -run TestMergeFailsClosedDoubleRun go test -count=2 ./internal/render/... -run 'TestRenderGoldens|TestRenderSummaryGolden' - - name: coverage gate (>=90% on internal/, D-010) - run: | - if ls internal/*/*.go >/dev/null 2>&1; then - go test -coverprofile=cov.out ./internal/... - pct=$(go tool cover -func=cov.out | awk '/^total:/ {gsub("%","",$3); print $3}') - echo "coverage: ${pct}% (required: 90%)" - awk -v p="$pct" 'BEGIN { exit (p+0 >= 90 ? 0 : 1) }' - else - echo "no internal packages yet — gate armed, not applicable" - fi + - name: install Task (coverage + exit gate scripts) + run: go install github.com/go-task/task/v3/cmd/task@"${TASK_VERSION}" + # AUD-S13 / D-128 — the coverage floor is SINGLE-SOURCED in Taskfile.yml's + # `coverage:` task (var COVERAGE_MIN). This step used to re-implement the gate + # inline with its own copy of the threshold, so raising the local floor left CI + # silently enforcing the previous one — the skew that actually shipped. + # Delegating to `task coverage` means the threshold exists in exactly ONE place + # and the two halves cannot drift again. Do not re-inline the shell here; + # change COVERAGE_MIN in Taskfile.yml instead. + - name: coverage gate (D-010 — threshold single-sourced from Taskfile.yml) + run: task coverage - run: CGO_ENABLED=0 go build ./... - name: dogfood examples (P5-E6-S08 — every non-locked pack gates itself green under the real `assent test` CLI) run: | @@ -77,8 +78,6 @@ jobs: done - name: comparison corpus dogfood (PCS-S08 — examples/comparison validates and runs green) run: go test ./examples/comparison/... - - name: install Task (exit gate scripts) - run: go install github.com/go-task/task/v3/cmd/task@"${TASK_VERSION}" - name: PCS compare exit gate (RELSE-03 — REQ-PCS-S09) run: bash hack/compare/exitgate_test.sh # AUD-S02 (REQ-AUD-S02-02) — CHANGELOG.md drift gate, main + schedule only. diff --git a/CHANGELOG.md b/CHANGELOG.md index 1087528..4dfcd84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,8 @@ repository still gets a decision, never by following the link; no release carrie ### Chores - :wrench: chore(changelog): render version headings in Keep-a-Changelog bracket form +- :wrench: chore(gate): raise the D-010 coverage floor 90% -> 91% (D-128) +- :wrench: chore(gate): single-source the coverage floor so CI and local cannot skew (D-128) ### Documentation - :memo: docs(decisions): close D-111 E9 exit gate after v0.1.0 @@ -116,30 +118,35 @@ repository still gets a decision, never by following the link; no release carrie - :memo: docs(decisions): record D-125 — CHANGELOG drift gate placement and its cost - :memo: docs(release): the changelog drift gate is in task check now, not outside it - :memo: docs(changelog): name the second toolDigest fallback branch -- :memo: docs(compare): state the D-121 digest change where consumers will read it - :memo: docs(architecture): redraw the C4 diagrams from the real go list graph (AUD-S17) - :memo: docs: mark the rego backend and GitHub adapter as planned outside the C4 pages - :memo: docs: narrow the composition-root claim and hedge the planned modes in vision.md +- :memo: docs(compare): state the D-121 digest change where consumers will read it - :memo: docs(adr): record the AUD-S12 malformed-marker behaviour change in ADR-0019 (review F3) - :memo: docs(adr): correct the convergence mechanism for a skipped bot marker (review F8) - :memo: docs(planning): mark E10 design-note steps 1-2 shipped by AUD-S15 -- :memo: docs: truth the D-129 checkout residuals, D-130's shadow claim, OQ-28 -- :memo: docs: point the OQ-28 decision-log link at docs/decisions/ -- :memo: docs: truth D-130 — a non-absence registry error aborts the run, not REVIEW -- :memo: docs: record the sibling host-declaration skip in D-130 +- :memo: docs(decisions): correct D-128's margin arithmetic and its stale ownership line +- :memo: docs(open-questions): widen OQ-27 to the full string-operand class, deepen OQ-28 (review F4/F5) - :memo: docs: renumber this lane's decision row D-129 -> D-131 (D-129/D-130 claimed by the containment lane) - :memo: docs(engine): the text-order refusal sentence reaches no adopter surface - :memo: docs: record the review close-out for the three non-blocking notes in D-131 - :memo: docs(engine): narrow the bytes-binding claim to what was verified -- :memo: docs: correct the D-130 abort/REVIEW comparison and the resource-owner fixture -- :memo: docs: narrow D-130's arming-posture and approval-evidence claims to what was measured +- :memo: docs(aud-s13): retarget the TEST-02 spec arm at the D-131 refusal and close OQ-27 - :memo: docs: record D-133 and ADR-0008 Amendment 2 — the checkout is content under judgment - :memo: docs(checkout): state the truncation mechanism precisely — WalkDir is happy, the read is not - :memo: docs(cli): document the --checkout symlink limitation where adopters hit it - :memo: docs(decisions): correct D-133's falsifiability claim, and record the doc carriers - :memo: docs(release): add compatibility notes for D-133 symlinks and D-131 ordering +- :memo: docs(aud-s13): re-measure D-128 against the current denominator and de-contradict its headline +- :memo: docs: truth the D-129 checkout residuals, D-130's shadow claim, OQ-28 +- :memo: docs: point the OQ-28 decision-log link at docs/decisions/ +- :memo: docs: truth D-130 — a non-absence registry error aborts the run, not REVIEW +- :memo: docs: record the sibling host-declaration skip in D-130 +- :memo: docs: correct the D-130 abort/REVIEW comparison and the resource-owner fixture +- :memo: docs: narrow D-130's arming-posture and approval-evidence claims to what was measured - :memo: docs(decisions): amend D-129 with the D-133 subsumption and the re-arming condition - :memo: docs(oq): close OQ-28's residual and relocate its proof pointer after D-133 +- :memo: docs(aud-s13): re-measure D-128 again after main moved (7513d79) ### Features - :sparkles: feat(cli): dispatch-table help listing the real subcommands (REQ-AUD-S05-01) @@ -153,9 +160,9 @@ repository still gets a decision, never by following the link; no release carrie - :bug: docs(readme): point the ADR-0014 link at the file that exists (DOC-05) - :bug: fix(docs-gates): the scripts claimed a wiring that does not exist - :bug: docs(examples): starter packs advertised a subcommand that does not exist +- :bug: fix(release): stop stripping the changelog header from the GitHub Release body - :bug: fix(forge): treat an over-limit body as deterministic, not retryable (AUD-S10 x S11) - :bug: fix(forge): carry reconcile warnings on refusal paths too (review F1) -- :bug: fix(release): stop stripping the changelog header from the GitHub Release body - :bug: fix(engine): relational compare over string-bound operands must fail safe, not lexically - :bug: fix(engine): ordering a bytes operand is text ordering too — refuse it (D-131) - :bug: fix(checkout): keep a rooted --subject working, and unshadow the path package @@ -186,21 +193,21 @@ repository still gets a decision, never by following the link; no release carrie - :lock: fix(release): reject a tag whose only verify run is a pull_request run - :lock: fix(run): derive pins.toolDigest from Go build info (D-120) - :lock: fix(run): emit the DecisionRecord before forge reconcile (D-122) -- :lock: fix(compare): domain-separate the replay-bundle digest per D-121 (ARCH-04) -- :lock: fix(test): report leaked credential names, never their values (review F1/F2) -- :lock: fix(forge): bound response reads and cap pagination loops (AUD-S10, REL-03/SEC-08) -- :lock: fix(forge): make retry-body safety structural, not conventional (review F5) - :lock: security(ci): pin the Task version in verify.yaml via a single workflow env (AUD-S09 / SEC-04) - :lock: security(ci): lockfile-pin the ajv validator and scrub checkout credentials (AUD-S14 / SEC-01 + SEC-03) - :lock: fix(lint): close the comment-blind fail-opens in the workflow-pin gate (review F1/F3/F4/F5/F6) - :lock: security(deps): override fast-json-patch to ^3.1.1, clearing GHSA-8gh8-hqwg-xf34 (review F2) - :lock: fix(lint): match what EXECUTES, not what the line mentions (review N1/N2/N4) +- :lock: fix(compare): domain-separate the replay-bundle digest per D-121 (ARCH-04) +- :lock: fix(forge): bound response reads and cap pagination loops (AUD-S10, REL-03/SEC-08) +- :lock: fix(forge): make retry-body safety structural, not conventional (review F5) - :lock: fix(lint): isolate the real CI step, and enforce command_view's scalar precondition (review N5/N6) +- :lock: fix(test): report leaked credential names, never their values (review F1/F2) +- :lock: fix(checkout): never return a truncated tree with a nil error +- :lock: fix(checkout): contain checkout reads to the checkout root - :lock: fix(provider): contain repo-file reads to a symlink-safe root - :lock: fix(provider): load the resource-owner registry from the target ref - :lock: fix(provider): fall back to the checkout registry on absence alone -- :lock: fix(checkout): never return a truncated tree with a nil error -- :lock: fix(checkout): contain checkout reads to the checkout root ### Testing - :white_check_mark: test(forge): model truncation and diff-endpoint failure in the fake @@ -219,15 +226,18 @@ repository still gets a decision, never by following the link; no release carrie - :white_check_mark: test(run): pin the emit-before-reconcile invariant on stdout too (D-122) - :white_check_mark: test(docs): pin the retired truth-lag claims so they cannot come back (DOC-05/06/09/10/11) - :white_check_mark: test(release): pin the CHANGELOG drift gate content, wiring and polarity -- :white_check_mark: fix(test): make the exec-timeout tests deterministic under load - :white_check_mark: test(cmd): pin policySha to raw policy bytes (D-121 byte-vs-document split) -- :white_check_mark: test(provider): pin the hard error on an unopenable --checkout - :white_check_mark: test(lint): close the aliased-import evasion in the ARCH-02 gate -- :white_check_mark: test(provider): point the leak scan at the refusal reason -- :white_check_mark: test(provider): pin the registry forge error out of resolveRunFacts +- :white_check_mark: fix(test): make the exec-timeout tests deterministic under load +- :white_check_mark: test(aud-s13): TEST-02/05/06 — toCEL overflow, reconcileClearSlot branches, repo_file containment + expiry +- :white_check_mark: test(aud-s13): drop an unreachable assertion in the containment table (review F6) - :white_check_mark: test(engine): pin how an unrepresentable numeric renders in {{ }} +- :white_check_mark: test(aud-s13): realign TEST-02 with the D-131 refusal contract - :white_check_mark: test(checkout): make every containment guard falsifiable - :white_check_mark: test(checkout): unmask the two collectFS guards and pin the Lstat side check +- :white_check_mark: test(provider): pin the hard error on an unopenable --checkout +- :white_check_mark: test(provider): point the leak scan at the refusal reason +- :white_check_mark: test(provider): pin the registry forge error out of resolveRunFacts - :white_check_mark: test: deduplicate the requireSymlinks helper across the merged lanes - :white_check_mark: test(provider): reshape the D-129 live-path proof around D-133's enumeration refusal - :white_check_mark: test(provider): guard the containment subtests at the parent, not per case diff --git a/GUIDELINES.md b/GUIDELINES.md index fbdf3b0..5f07515 100644 --- a/GUIDELINES.md +++ b/GUIDELINES.md @@ -23,7 +23,9 @@ holds the *how*. Conflicts: ADRs win; update this file via PR when they do. ## Testing (D-010) - TDD: failing test first, always. One logical change per commit; `task check` green before - every commit (includes the ≥90% coverage gate on `internal/…`). + every commit (includes the D-010 coverage gate on `internal/…`; the floor is the + `COVERAGE_MIN` var in `Taskfile.yml`'s `coverage:` task — currently 91%, raised by D-128 — + and CI runs that same task, so there is only ever one number to change). - Test at the level that gives the proof: golden decision tests (L0) for engine semantics; the adopter harness (L1) for policy behavior; cassettes (L2) for adapters; real GitLab (L3, `//go:build e2e`) for forge semantics — never mocks for thread/approval/merge flows. diff --git a/Taskfile.yml b/Taskfile.yml index 16c1b15..435e066 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -32,7 +32,16 @@ tasks: - go test -race ./... coverage: - desc: "Coverage gate: >=90% on internal/ (D-010) — arms itself once internal/ has code" + # D-010 coverage gate. COVERAGE_MIN below is the SINGLE SOURCE of the floor: + # .github/workflows/verify.yaml runs this very task rather than re-implementing + # the threshold, so the local and CI halves cannot skew (D-128). Raise the floor + # by editing COVERAGE_MIN only — no other GATE may carry its own copy of the + # number (GUIDELINES.md quotes it as prose and names this var as the source). + # NOTE: the comparison is against the ONE-DECIMAL total `go tool cover -func` + # prints, so a floor of 91 really admits anything rounding to 91.0, i.e. ~90.95%. + desc: "Coverage gate on internal/ (D-010; floor = COVERAGE_MIN, raised to 91 by D-128) — arms itself once internal/ has code" + vars: + COVERAGE_MIN: 91 cmds: - | if ! ls internal/*/*.go >/dev/null 2>&1; then @@ -40,8 +49,8 @@ tasks: fi go test -coverprofile=cov.out ./internal/... pct=$(go tool cover -func=cov.out | awk '/^total:/ {gsub("%","",$3); print $3}') - echo "coverage: ${pct}% (required: 90%)" - awk -v p="$pct" 'BEGIN { exit (p+0 >= 90 ? 0 : 1) }' || { echo "FAIL: coverage below 90% (D-010)"; exit 1; } + echo "coverage: ${pct}% (required: {{.COVERAGE_MIN}}%)" + awk -v p="$pct" -v min="{{.COVERAGE_MIN}}" 'BEGIN { exit (p+0 >= min+0 ? 0 : 1) }' || { echo "FAIL: coverage below {{.COVERAGE_MIN}}% (D-010, floor raised by D-128)"; exit 1; } build: desc: Build the CLI diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index 0fab4c4..5fc78a6 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -130,6 +130,7 @@ project/process decisions. | D-123 | 2026-08-06 | **ARCH-01 — boundary enforcement automated (depguard + extended purity walk); ADR-0011 Amendment 3 truths the "arch-lint enforced" claim.** Two layers: (1) golangci `depguard` deny-rules — `internal/core/**`, `internal/change/**`, `internal/glob`, `internal/lint`, `internal/catalogue`, `internal/evaldecode`, `internal/compare`, `schemas/**` may import none of `internal/forge/**`, `internal/render/**`, `cmd/**`, `net/**`; (2) `TestCorePurity` walk extends to `../evaldecode`, `../compare`, and `../../schemas` (call-level: `time.Now`/`os.Getenv`/`os.Environ`/rand/net, adversarial self-test retained). Scope note: this EXTENDS the AGENTS.md rule-7 pure tree — `internal/evaldecode` (engine input decode) and `internal/compare` (D-116/D-117 gate determinism) join the determinism guard; `schemas` is embedded compile-time authority. Acceptance: a synthetic violating import/call fails CI both ways. Revert: drop depguard rules + walk dirs and re-amend ADR-0011. | | D-124 | 2026-08-07 | **AUD-S06 residual — the two docs truth-lag gates exist but are UNWIRED; wiring is assigned to Lane B.** `hack/docs/readme_smoke_test.sh` (REQ-AUD-S06-01, executes every README quick-start command) and `hack/docs/truthlag_pins_test.sh` (REQ-AUD-S06-02, 18 grep/diff pins over DOC-05/06/09/10/11 + the ADR status index) are green and mutation-proven, but nothing invokes them: `Taskfile.yml` and `.github/workflows/**` belong to **Lane B** (AUD-S02/S03/S09/S14), so AUD-S06 could not add them. **Consequence, stated plainly: until they are wired, a future README or docs edit reopens DOC-07 (a quick-start that exits 2) or any pinned claim with NOTHING going red** — the mechanism is a manual gate, which is the same class of defect AUD-S06 exists to close. Assignment: Lane B adds a `docs-gates` task (`bash hack/docs/readme_smoke_test.sh && bash hack/docs/truthlag_pins_test.sh`) as a `check:` dependency, following the `hack/compare/exitgate_test.sh` precedent (D-118), alongside its AUD-S09/S14 workflow work. **Extend when wiring** (known pin gaps, both accepted for now): (a) the DOC-09 walkthrough check asserts banner PRESENCE, not polarity — flipping a step's `Planned` to `Shipped` stays green; (b) the DOC-05 link-resolution loop is scoped to `README.md`, so the relative links in `examples/README.md` are unpinned; (c) AUD-S05's `TestNoStaleProductClaims` (`cmd/assent/main_help_test.go`, Lane A5's file) walks only `cmd/`, `internal/` and `docs/` — markdown under `hack/`, `.github/` and `test/` is grepped by no pin at all. **Known unfixed truth-lag, deliberately not corrected in AUD-S06 to keep the reviewed diff narrow — fix when next editing these files:** (i) `docs/planning/meta-plan.md` closes the Phase-5 epic table with "Ordering constraint: E7 starts early (alongside E1)", directly under the new heading asserting the table is the numbering that actually executed — E7 in fact landed after E6 (E6 tip `ec91226` is an ancestor of E7-S08 `f27457d`, both 2026-08-04); (ii) `docs/usage/install.md` credits a stamped version to "the Homebrew **bottle**", but `.goreleaser.yaml`'s `brews:` block publishes a **Formula** whose `url_template` points at the release archive — no bottle is built or hosted; the version claim is true, only the term is wrong. Revert: delete the two scripts and reopen DOC-05/06/07/09/10/11 as live findings. | | D-125 | 2026-08-07 | **AUD-S02 judgment call (b) — the CHANGELOG drift gate runs in `task check` on every local commit and in CI on push-to-main + schedule, NOT on `pull_request`.** The spec asked for both placements; the PR placement is not merely noisy, it is red by construction. Evidence (reproduced locally, not reasoned): `hack/release/verify-changelog.sh` diffs the WHOLE generated changelog against the committed file, and `cliff.toml`'s parser list ends in a catch-all `{ message = ".*", group = "Other" }`, so merge commits are rendered — merging a probe branch put `- Merge 1234567 into 89abcde` in the generated output. On `pull_request`, `actions/checkout` checks out `refs/pull/N/merge`: a merge commit minted at CI time whose subject is exactly that shape. No committed `CHANGELOG.md` can contain a line naming a SHA that did not exist when it was written, so a PR-scoped step fails on every PR with no author fix (and would push commit SHAs into the changelog, contra **D-101**). Walk-back taken under the spec's decide-and-log sanction, using the `release-exitgate` precedent already in the file: `if: github.event_name != 'pull_request'`. **Consequence, stated plainly:** PR CI does not catch changelog drift. The `check:` entry does — one commit later, by construction: `task check` is green at HEAD, the next commit makes `CHANGELOG.md` stale, and the following `task check` is red until `task changelog-write` is committed. Push-to-main is the backstop, so a lane that lands without regenerating reds main until a regeneration commit follows. **Working rule:** a regeneration commit must be subject-prefixed `:memo: chore(release):` or `:wrench: chore(release):` — the two forms `cliff.toml` skips — or it creates fresh drift itself; and a lane must regenerate AFTER its last content commit and after any `git merge origin/main`. Two companion changes ship with it: version headings render Keep-a-Changelog style (`## [0.1.0] - 2026-08-05`, matching the hand-written `[0.0.0]` stub), and the **D-120** `pins.toolDigest` record-consumer warning lives in `cliff.toml`'s `[changelog] header` — CHANGELOG.md is regenerated in full, so a hand-edit there would be wiped by the next `changelog-write` with the drift gate still green. **Closes the D-124 assignment** in the same `check:` list: `docs-gates` (`hack/docs/readme_smoke_test.sh` + `truthlag_pins_test.sh`) and `lint-depguard-test` (`hack/lint/depguard_test.sh`) are now sequential `check:` commands — sequential, not `deps:`, because go-task runs deps in parallel and the smoke test builds a binary while `fmt` rewrites the tree. The depguard proof is local-only: CI lints via `golangci-lint-action`, which leaves no binary on PATH for a later step, and that gate refuses to skip when `golangci-lint` is absent. D-124's known pin gaps (a)/(b)/(c) and truth-lag items (i)/(ii) are NOT addressed here and remain open. Revert: drop the `check:` entries and the verify.yaml step — reopens RELSE-01 and D-124. | +| D-128 | 2026-08-08 | **AUD-S13 — the D-010 coverage floor is raised 90% → 91%, SINGLE-SOURCED in `Taskfile.yml`'s `COVERAGE_MIN` so the CI half follows automatically.** (Headline corrected 2026-08-09: it previously read "local gate only; the CI half is NOT raised here" and then reversed itself in its own "Split-gate CLOSED" paragraph below. The split-gate half of this row is the current state; the "local only" phrasing described a fence that had already lifted when the row was written.) Operator ruling, taken in the same change AUD-S13 lands. Rationale: the aggregate `./internal/...` total sat at **90.28%** against a 90% floor, so the gate had stopped MEASURING and started STEERING — any lane that added a branch without a test reddened `task check` for a reason unrelated to that lane, and the cheapest green was a filler test. **Supersedes the AUD-S13 spec line "Not in scope: raising the D-010 floor"** (`openspec/specs/p5-aud-audit-remediation/spec.md`, story AUD-S13) and its goal-(4) parenthetical "gate floor stays 90% per judgment call (e) unless the operator raises it" — the operator raised it; read the two together, not as a contradiction. **Measured — re-measured 2026-08-09, and the figures this row shipped with were WRONG.** The row originally said "90.28% → 91.010% (4479→4515 of 4961 statements)". Those were true against the `main` this lane first branched from; the D-131 lane then landed and **the denominator moved**, and the row was left behind — a docs-truth defect of exactly the class this epic exists to close, shipping inside the epic. **Direction, stated so a reader knows which way the error cut: the stale figures UNDERSTATED the margin, so anyone acting on them was over-conservative, never under.** Re-measured against `origin/main` at **`7513d79`** and at the lane head: **90.4041% → 91.0810%**, i.e. **4541 → 4575 of 5023 statements** (+34 covered) from the three TEST-02/05/06 behavior families, no filler. The **denominator is identical on both sides** (5023) — AUD-S13 adds `_test.go` files and no production statements, and that unchanged denominator is the proof. **This is the SECOND re-measurement inside this one lane** — `main` moved once for D-131 and again for the provider containment lane (PR #36, `7513d79`), and both times the previously-written figures went stale within a day. That is the rule below earning its place: what is durable is the FORMULA, not the pair of integers. **The real margin, stated as a RULE this time so it cannot rot again with the next denominator move.** "91%" is nominal. The gate does not compare the raw ratio: it compares the **one-decimal total** `go tool cover -func` prints (`pct=$(… awk '/^total:/ …')` yields the string `91.1`, not `91.0810`). Anything that rounds to `91.0` therefore passes, so the effective floor is **~90.95%** and the true minimum is **`ceil(0.9095 × N)` covered statements for whatever the current denominator N is** — recompute it from `cov.out`, never transcribe it from here. Dated instance, 2026-08-09, N = 5023: the true minimum is **4569** (4569 → 90.9616% → prints `91.0` → PASS; 4568 → 90.9417% → prints `90.9` → FAIL). AUD-S13 leaves 4575, so the slack is **6 statements**, not the 0.08pp the raw ratio suggests. The rounding behaviour is inherited from the pre-existing gate design (D-010), not introduced here, and is left as-is. Net: the gate is now genuinely load-bearing — the next lane that adds more untested statements than the current slack (six, at N = 5023) reds `task check`, which is the intent. If that proves disruptive the answer is more behavior tests, not a lower floor. **Split-gate CLOSED, and closed by SINGLE-SOURCING rather than by editing two numbers.** The floor briefly existed twice: `Taskfile.yml` said 91% while `.github/workflows/verify.yaml` re-implemented the whole gate inline and still said 90% — AUD-S13 was fenced out of `.github/workflows/**` while PR #25 (AUD-S09/S14) owned it. #25 merged 2026-08-08, the fence lifted, and rather than raise a second literal (which would only reset the drift clock) the CI step now **delegates to `task coverage`**: the `install Task` step moves above it, and the step body is just `task coverage`. The threshold therefore lives in exactly ONE place — `Taskfile.yml`'s `coverage:` task var **`COVERAGE_MIN: 91`**, interpolated into the echo and the awk comparison, with the `desc` naming the var instead of the number. `verify.yaml` now carries no coverage threshold at all, so the two halves are structurally incapable of skewing. Proven both polarities: `COVERAGE_MIN: 99` reds the gate with the right message (so the templated var is really load-bearing — an unrendered `{{.COVERAGE_MIN}}` would make `min+0 == 0` and pass everything), and `hack/lint/workflow_pins_test.sh` stays green across all 39 controls, including "both Task installs interpolate a single workflow-level TASK_VERSION" after the step move. **Consequence:** raising or lowering the floor is now a one-line edit and CI follows automatically; re-inlining the shell into `verify.yaml` would silently reopen the skew, so the step carries a comment saying not to. Revert: restore the inline coverage shell in `verify.yaml` and the literal thresholds in `Taskfile.yml` / `GUIDELINES.md`. | | D-129 | 2026-08-08 | **OQ-28 (P1) CLOSED — repo-file / resource-owner reads are contained to a symlink-safe root, and a symlinked candidate is REFUSED, not followed.** `builtin/repo-file` held only PATH containment (`cleanRel`/`underAnyRoot`, pure string guards); `os.DirFS` is documented as not a security boundary and `fs.Stat` follows links, so the invariant "never a fact from outside the declared roots" was false in production. Reproduced two ways on the live `assent run --checkout` route (the tree is the MR HEAD — content authored by the contributor): (1) directory symlink `topics/evil -> ` with `Roots:["topics"]` → `resolved` value from off-tree; (2) file symlink at a wholly legitimate in-root path `topics/prod/quota.yaml -> /abs/host/cluster-secrets.yaml` → `resolved` 31337, and end-to-end an `APPROVE` for a change no in-repo quota allows. Reach was arbitrary absolute host paths (narrowed only by "must be a YAML/JSON mapping carrying a declared output name"), with `render.displayFactValue` as an exfiltration seam for non-`sensitive` values. **Two layers, different guarantees.** (a) INJECTION SITE — `cmd/assent/provider_host.go` `checkoutFS` now returns `builtin.OpenRepoRoot(dir)` = `os.OpenRoot` + `(*os.Root).FS()`: a syscall-level boundary, no read outside the checkout root for ANY consumer of that FS, closed for resource-owner for free; the `*os.Root` is closed by `resolveRunFacts`, and a `--checkout` that cannot be opened as a root is now a HARD error (loud, no decision emitted) instead of a silent degrade. (b) BUILTIN — `classifyCandidate` Lstats EVERY path component of a candidate and refuses on any symlink: this is the only layer that can protect the declared `Roots` CLIP, which `os.Root` cannot see (`topics/prod/quota.yaml -> ../../secrets/quota.yaml` never leaves the FS root yet leaves the roots), and it also catches the directory-symlink form under a bare `os.DirFS`, where an Lstat of the leaf alone reports an ordinary regular file. `RepoFileOpts.FS` now carries the documented contract "must be a symlink-safe root"; layer (b) is defence in depth, NOT a substitute — it can only observe what the injected FS reports. **In-root symlinks are refused too** (not followed): one rule, no per-link reachability reasoning, and it is the rule that keeps the roots clip honest; no fixture, example or corpus file in the tree is a symlink, so nothing legitimate regresses. **Refusal shape:** `unavailable` with a contributor-readable reason naming the refused candidate — same fail direction and same class as the existing "anchor outside declared roots" (the operator's config is well-formed; the repo content is hostile), and never the raw OS text (`path escapes from parent`) which would leak a Go internal into an MR comment (ADR-0012). The refusal STOPS the walk-up rather than skipping to a less-specific file: skipping would silently mask an escape behind an ordinary-looking resolution. Sibling assessment: `LoadResourceOwnerMap` reads the same FS with NO roots at all and decides who may approve — it refuses a symlinked registry with an error (no client → the owner fact never resolves). **Checkout residuals — TWO of them, both LIVE, and both worse than this row first said.** As first written this row claimed `cmd/assent/checkout.go` shares the assumption but that "`filepath.WalkDir` does not follow directory symlinks, so **only** a final-component file symlink escapes there". That sentence is **materially false** and is corrected here rather than left as the thing a maintainer reads when deciding whether to tag: it understated a **P0**. `collectTree`/`readIfPresent` are NOT fixed in this lane — they are E1-S08 fold semantics — and are being fixed in a sibling containment lane. **(P0) `collectTree` (`checkout.go:121`) returns a SILENTLY TRUNCATED map.** The `errors.Is(err, fs.ErrNotExist)` tolerance wraps the **entire** `WalkDir`, not just a missing root, and `WalkDir` aborts the whole walk on the first callback error. A dangling symlink is visited as a non-directory entry (`WalkDir` Lstats), the callback's `os.ReadFile` follows it and fails `ENOENT`, the walk stops, and the **partial** map is returned **with no error**. `WalkDir` visits in lexical order, so a dangling symlink whose name sorts before `.assent` (any leading byte below `0x2E` — `!evil`, `-evil`) erases `.assent/**` from the changed-file set entirely: the `.assent/**` dominance class **and** the opacity flag both disappear. The reviewer drove this end-to-end through `runRun --checkout` and turned a **BLOCK into an APPROVE**, defeating the **D-042 self-vouch guard** — the same starvation class as D-119/REL-07, reached by a different mechanism. **(P1) `readIfPresent` (`checkout.go:129`) is a bare `os.ReadFile`, which follows symlinks**, and `run.go:279` overrides the **governed subject's** base and head bytes from it whenever `--checkout` is set. A head-side file symlink therefore makes an arbitrary off-tree host file the document under judgment; the reviewer confirmed both APPROVE-manufacture and verbatim exfiltration of the host file's contents into the posted thread. Neither residual is mitigated by anything in this row: the `os.Root` containment landed here is on `checkoutFS`, a different read path. **Amendment (2026-08-09, cross-lane collision with D-133) — the live-path REPRODUCTION in this row is SUBSUMED; the guard is not. It is now defence in depth, and this amendment names the condition that makes it load-bearing again.** D-133 landed on `main` (PR #39, `71b573e`) AFTER both reviewers had approved this lane, so no reviewer saw the interaction. It refuses ANY symlink anywhere under `base/` or `head/` at changed-file ENUMERATION — `run.go` step 5b (`foldCheckout` → `collectFS`) — which runs BEFORE providers resolve (step 5c) and returns a hard error. So the sentence above, “Reproduced two ways on the live `assent run --checkout` route”, **no longer re-runs on that route**: the tree is refused at enumeration with `exit 1`, zero forge writes, and `refusing "topics/prod/quota.yaml" … reached through a symlink`. That sentence stands as HISTORY — it was true when written and it is why this fix exists — but a maintainer re-deriving it today meets D-133's refusal and must NOT conclude that the D-129 fix is what stopped it. **No alternative live route exists, and that was MEASURED, not reasoned.** The tree providers read and the tree the enumerator walks can diverge only where `root/head` is not a directory, in which case `checkoutFS` falls back to `builtin.OpenRepoRoot(root)` — and in every such shape `dirCheckout` cannot read a `head/` side, so the governed subject degenerates and the decision stops discriminating. Measured on both sub-shapes with an off-tree `31337` planted where only the provider can see it: with NEITHER side present, a poisoned tree and a clean tree emit BYTE-IDENTICAL records (`"decision":"REVIEW"`, `code: changeset.undecidable`); with `base/` only, likewise byte-identical (`code: fileEvent.unmatchedDelete`). A test on that route would pass whether or not containment works — precisely the wrong-reason pass this lane's guard comment exists to forbid. Option (b) — exercise provider containment without passing through the enumerator — therefore does not exist. **The relationship is defence in depth, not redundancy, and it is DIRECTIONAL.** D-133 is the outer layer and fires first today; ADR-0008 Amendment 2 names its own revisit direction as folding the refusal OPAQUE (fail-safe REVIEW with a resolvable thread) rather than following the link. The moment that fold lands, a run CONTINUES with a symlink present, providers resolve, and this row's guard is the only barrier left between the merge request and an off-tree fact. The coverage is therefore not dropped, and not documented-away: it is **reshaped and armed**. `TestRunCheckoutSymlinkFactEscapeRefused` becomes `TestRunCheckoutSymlinkRefusedAtEnumeration`, asserting the subsumption on the live path — exit ≠ 0, the `reached through a symlink` MESSAGE CONTRACT plus the refused repo-relative path (never the side directory: which side is walked first is behaviourally irrelevant), no `31337` on stdout or in the posted thread, zero forge writes — and carrying the TRIPWIRE: when the exit code becomes 0 its failure message states that the fold has landed and hands the engineer the successor test by name. Simulated by folding `foldCheckout`'s error opaque instead of returning it: the case reds with exactly that message over a `REVIEW`/`changeset.undecidable` record while the successor stays green, so the hand-over demonstrably works. **The successor, `TestResolveRunFactsRefusesSymlinkedQuotaCandidate`, keeps the DISTINGUISHING assertion intact at the deepest seam the live path still reaches.** It drives `resolveRunFacts` — the exact function `runRun` step 5c calls — over the same planted tree, traversing `checkoutFS` → `providerCallFor` → the SOLE production `builtin.RepoFileOpts` construction site (`cmd/assent/provider_host.go:213`) → `CallRepoFile`'s walk-up → `ResolveFactsChecked` → `ToAggregateFact`; the FS, the roots, the anchor and the declaration all come from production code, none of it hand-wired. That is strictly deeper than the two pre-existing cmd-edge pins: `TestProviderHostInjectsSymlinkSafeRoot` is a single `fs.ReadFile` through `checkoutFS` (layer (a) only, no provider in the picture), and `TestResourceOwnerRegistrySymlinkInCheckoutRefused` documents itself as a both-layers-together outcome on a different builtin. The distinguishing logic survives verbatim: a legitimate `topics/quota.yaml` (24) sits on the walk-up path, so a fix that merely SKIPPED the symlinked candidate resolves 24 — an ordinary-looking success that silently masks the attack — and only a refusal that STOPS the walk-up leaves the fact unresolved. **The two layers are pinned SEPARATELY, not as a conjunction gate** — the blur D-133 was itself corrected for. An ABSOLUTE off-tree link is refused by EITHER layer, and pins the combined outcome plus the no-`31337`-in-the-fact-reason property; a RELATIVE in-root link (`topics/prod/quota.yaml -> ../quota.yaml`) is FOLLOWED by `(*os.Root).FS()` because it never leaves the root, so `classifyCandidate`'s refusal is the only barrier — layer (b) in isolation. **Mutation-proven, each alone:** deleting `classifyCandidate`'s symlink arm reds BOTH poisoned subtests with `FACT ESCAPE: … resolved to 24` (the skip-and-fallback outcome) while the control subtest stays green, so this is not “refuse everything”; deleting `collectFS`'s symlink arm alone leaves the tree refused by the non-regular-file arm (`refusing non-regular file …`) and reds the enumeration case on the MESSAGE contract, which is the honest reading of what that arm pins. **No new decision row was minted.** Nothing about the guard, its shape or its fail direction changed — only which layer fires first, and where the proof now lives — so this belongs to the row that owns the guard, not to a D-135. **Cross-reference owed elsewhere, deliberately NOT taken here:** ADR-0008 Amendment 2's “loosening this means folding the refusal opaque” should add that the fold RE-ARMS this row's provider guard, and the D-133 row should record the subsumption from its side. Both are already merged on `main`, so that edit belongs to the next lane that touches them rather than to a rewrite of published rows. Spec: `REQ-E5-S07-03` / `REQ-E5-S08-03`. Revert: restore `os.DirFS` in `checkoutFS` and drop `classifyCandidate` — reopens OQ-28 as a live P1. | | D-130 | 2026-08-08 | **The resource→owner registry loads from the TARGET ref first; the checkout is a fallback ON ABSENCE ALONE, never a shadow (GUIDELINES §Safety 2+3 / ADR-0015 §1).** Found while assessing the D-129 sibling: `loadResourceOwnerRegistry` tried `repoFS` BEFORE `client.FileAtRef(targetRef)`, and under `--checkout` `repoFS` is the merge request's own head tree — so an MR could ship `governance/owners.yaml` naming its author as owner of the resource it is changing and satisfy an `ownership` obligation with a registry it wrote itself. That registry is a decision input (it decides WHO MAY APPROVE), so it must come from the target ref like every other deciding document; only the material under judgment comes from the MR branch. Order is now target ref → (on **absence only**) checkout → error. Reproduced by mutation: with the old order the loaded owner is `attacker`, with the new order `team-payments`, same inputs (`TestResourceOwnerRegistryLoadsFromTargetRef`). Compat: repos/hermetic runs whose target ref carries no registry still load the checkout copy (`TestResourceOwnerRegistryFallsBackToCheckout`); both sides missing stays an error, never an empty map that would make every resource unowned. No test, example or dogfood fixture depended on the old precedence (the live resource-owner path had no cmd-level test at all before this row). `loadResourceOwnerRegistry` now takes a one-method `refFilePort` instead of the whole `forgePort`, which is what made the precedence unit-testable. **Amendment (independent security review, same lane) — the fallback is gated on ABSENCE, not on "the forge answered badly", and this row's own "never a shadow" claim was false as first landed.** The code fell back to the checkout on **every** `FileAtRef` error, so a forge that merely failed — 401, 429, 500, 502, 503, an expired token, a proxy hiccup — handed the who-may-approve document to the merge request's own head tree, with **no error surfaced** and nothing in the decision to show it happened. Reproduced: a 503 on the **modify-plus-transient-error** shape (a plain value diff on an existing registry) resolves `owner = "attacker"`. The 404 shape is separately mitigated — a whole-file registry *add* folds opaque → REVIEW — which is precisely why the modify shape was the live one. That is a fail-OPEN under GUIDELINES §Safety 2 and the exact shadow this row claimed was impossible. The gate is now `errors.Is(err, forge.ErrNotFound)`, the **neutral port sentinel** (`internal/forge/port.go`, already wrapped with `%w` by the GitLab adapter) — deliberately **not** `gitlab.ErrNotFound`, which would red the ARCH-02 / depguard boundary gate (D-123). **Outcome, corrected — this amendment first said "any non-absence error → … → fail-safe REVIEW", and that is NOT what ships.** The error is not swallowed into an unresolved fact: it propagates `loadResourceOwnerRegistry` → `providerCallFor` → `resolveRunFacts` → `orchestrate`, so **the run ABORTS**. Measured on the live `runRun` path with a forge that 503s the registry read only — one run against a fresh MR: exit code **1**, nothing on stdout, and on stderr `assent run: resolve providers: provider "owner": resource-owner registry "governance/owners.yaml" at ref "main": gitlab: get file "governance/owners.yaml" at ref "main": unexpected status 503`; `approvals=0 merges=0 discussionsPosted=0 notesPosted=0 notesUpdated=0`. That is more conservative in what assent WRITES — no APPROVE, no arming, nothing written — but **NOT strictly more conservative in effect**, and "strictly" was itself an overclaim minted by the previous correction of this sentence. It matters because this row exists to inform a future safety choice, so a false premise here is worse than the error it replaced. **Measured** — two `runRun` calls against ONE `fakeGitLab`, which models the forge and the MR rather than the run, so the carry-over is genuine MR state and not test leakage: run 1 serves the registry, decides `APPROVE`, and posts one summary note the MR displays as `**✅ Policy evaluation** … **Decision:** APPROVE`; run 2 then 503s on the registry read and leaves the MR EXACTLY as run 1 left it — `notes=1` still carrying that `✅ … APPROVE` body **unrevised**, `notesUpdated=0`, `discussionsPosted=0`, no thread — on top of the red job above. The same two runs with ONLY the registry content changed, so the resource is unknown and run 2 decides `REVIEW`, instead **upsert that same note** to `**📋 Policy evaluation** … **Decision:** REVIEW` (`notesUpdated=1`, note count still 1) **and post one unresolved discussion** (`discussionsPosted=1`). The ABORT shape was measured in both arming postures — arming eligible (run 1 also writes `approvals=1 merges=1`) and arming refused/advisory (`approvals=0 merges=0`, so the stale `✅` summary is the ONLY thing the MR carries) — with the identical stale-summary result; the REVIEW comparison above was run in the arming-eligible posture only. **Read, not measured** (a fake forge cannot demonstrate merge behaviour): that unresolved discussion is what HOLDS the merge wherever assent can arm at all, because `PreconditionFromCapabilities` refuses arming unless `only_allow_merge_if_all_discussions_are_resolved` is enabled (`internal/forge/precondition.go:63-68` → `ArmEligible` → `reconcileApproveMerge`'s `ErrArmingRefused`, `internal/forge/forge.go:780`; polarity pinned by `TestRunApproveUnarmedNoWrite`) — and the abort supplies none. So the gap is not the UX gap this row previously described ("there is nothing in the MR explaining why"): the artifact a human reads still says APPROVE, and nothing assent wrote holds the merge. It is **not** an auto-merge risk — an aborted run arms nothing and writes nothing — it is a **stale-green risk on the human path**, and it bites precisely on the rerun-after-a-green-run shape. The rest of the earlier wording stands: a contributor hitting a flaky forge gets a red CI job with **no DecisionRecord and no thread at all**, not a reviewable REVIEW thread. Anyone changing this must choose between the two deliberately; today it is the abort. Test: `TestResourceOwnerRegistryTransientForgeErrorNeverFallsBackToCheckout` (500/502/503/429/401), mutation-proven red at `owner = "attacker"` before the gate landed, plus `TestResourceOwnerRegistryForgeErrorAbortsResolveRunFacts`, which pins the propagation one level up — `providerCallFor` → `loadResourceOwnerRegistry` → `resolveRunFacts`, wiring that was correct but that NO test in the tree entered (`builtin/resource-owner` appeared in no config, example, exit gate or run test), mutation-proven red on both links (`return nil, err` → `return nil, nil` in `providerCallFor`'s resource-owner arm; the `provider %q` wrap → `continue` in `resolveRunFacts`), and red for no other test in the repo. The three pre-existing registry stubs moved from `errors.New("404 file not found")` to the adapter's real wrapped shape: "any error opens the fallback" **was** the defect, so a stub asserting it would have stayed green over the fix. **Sibling checked and deliberately NOT changed, recorded so the next reviewer need not re-derive it:** the host-declaration load at the head of `resolveRunFacts`'s provider loop `continue`s on ANY `FileAtRef` error — the same absence/error conflation this amendment closed for the registry — but it is fail-safe by a DIFFERENT mechanism, re-confirmed empirically through `runRun` on an ownership policy proving `facts.owner.owner.state == 'resolved'` (declaration present + registry served from the target ref → `APPROVE` on a genuinely **resolved** owner fact, `approvals=1 merges=1`; declaration 404 → `REVIEW` with `effect: require-review`, `code: predicate.error`, one thread posted, `approvals=0 merges=0`), because a skipped provider leaves the fact ABSENT, the CEL bound reference errors, and the engine routes a predicate error to require-review — a decision and a thread, not an abort. **The earlier reading of that control was DEGENERATE, and it is corrected rather than restated.** It ran against `cmd/assent/provider_host_registry_test.go`'s host-declaration fixture, which declared output `team` at `maxAge 1h` while `builtin/resource-owner` emits `owner` at 24h (`OutputOwner` / `OwnerDeclaration()`), so `ResolveFactsChecked`'s `DeclarationsEqual` check synthesized `state:"invalid"` / "provider echoed declaration does not match host config" on every run — the APPROVE arm therefore said nothing about a healthy resolve. Measured on that fixture: the run emits `APPROVE` with ZERO findings — the same output a run with no provider configured at all would produce — because the probe rule referenced `facts.owner.team` on an MR that already carried a human approval. The mechanism (the FALSE predicate's `require-review` onFailure being satisfied by that standing approval evidence) is INFERRED, not separately measured here; its opposite polarity is pinned by `TestRunNilEvidenceRequiresReview`. Re-measured with the rule pointed at the fact the builtin actually emits: the broken fixture yields `REVIEW`/`predicate.error` with a thread and `facts used: owner.team=invalid` — indistinguishable from the 404 arm — while the corrected fixture yields `APPROVE` on a resolved fact. The fixture is corrected in this lane and the healthy resolve is pinned by `TestResourceOwnerDeclarationResolvesOwnerFact` (mutation-proven red on both the output name and the `maxAge`), so the control now discriminates. The skip, fail-safe though it is, remains **SILENT** (`pins.factsResolvedAt` is simply `{}` and no reason is emitted), so an operator cannot distinguish "the provider is not declared for this repo" from "the forge failed to serve the declaration"; noted, not fixed here. Spec: `REQ-E5-S08-03`. Revert: swap the order back, or widen the gate to any error — either reinstates the shadowing fail-open. | | D-131 | 2026-08-08 | **Engine P1 fail-open — an ordering operator over a TEXT operand is an evaluation error, not a lexical answer (ADR-0013 Amendment 1).** CEL defines `<` `<=` `>` `>=` over strings as a lexical compare, so a `when: new >= old` leaf answered a boolean whenever its operands bound as text — and both answers are wrong. Reproduced end-to-end through the production entry point (`change.Diff` → `evaldecode.DecodeCanonical` → `aggregate.Cover`) on a routine adopter shape, a QUOTED numeric: `partitions: "12"` → `"6"` is a `!!str` the differ tag-discriminates and the decoder keeps a Go string BY DESIGN, so the D-016 `partitions-must-not-shrink` rule evaluated the lexical `"6" >= "12"` = **true**, proved `non-destructive`, fired nothing, and returned **APPROVE with zero findings** — a BLOCK→APPROVE flip. The mirror case is equally wrong: a legitimate grow `"6"` → `"12"` evaluated false and BLOCKed. Second, narrower instance: a numeric literal representable as neither int64 nor float64 fell back to its STRING form in `toCEL`, so `9e399 > 1e400` was lexically true (numerically false). **Fix, at the `evalLeaf`/`toCEL` seam only:** (1) `evalLeaf` plants a watcher (a cel-go `CustomDecorator`) on every operand of every relational operator; if an operand ACTUALLY evaluates to text — a CEL `string` OR `bytes` value — the leaf returns an error → `predicate.error` → REVIEW. Value-based, not syntax-based, and deliberately so: whether `new >= old` is sound depends on the adopter's DATA, not the policy text, so **lint cannot catch this class** (`checkLeafScope`/`checkPredicateScope`/`checkFactsShape` were checked; none could) — the information exists only at evaluation. Watching (not reading post-eval `EvalState`) is what makes EVERY comprehension iteration visible, since state keeps one value per node id. Short-circuited compares never run their watcher, so a policy cannot flip to REVIEW for a compare that did not happen. The guard is applied to **both** evaluation seams: `evalLeaf` (the E2 production path) and `evalRule` (the walking-skeleton `Aggregate` path, exported and test-only today, whose env declares `old`/`new` as `StringType` and binds the RAW canonical strings — so every bare relational there was lexical by construction, with only convention mandating `int()`). One unguarded evaluator is how this class returns; same drift argument as D-055(c). Its cost-budget test doubles as the interaction proof that cel-go still charges a decorated operand (`costBombWhen` is the left operand of `> 0`; cel-go v0.30.0). (2) `toCEL` binds an unrepresentable numeric literal as a CEL error value instead of its string form; an over-int64 but float64-representable literal still binds lossily — ADR-0013 residual #1's lossy half is UNCHANGED and still live. **Behaviour change, stated plainly: a policy that previously evaluated (wrongly) now ERRORS.** Any leaf that ordered text — in either direction, at any decision — becomes `predicate.error` → REVIEW. Nothing in `examples/`, the comparison corpus, either dogfood pack or the test corpus ordered text (all relational leaves are numeric: partitions, memory_mb, replicas, `size()`), so **no golden, fixture or gate output changed**; the only changed test is `internal/evaldecode`'s mutation proof `TestStringOldNewFailsOpen`, renamed `TestUndecodedStringOldNewFailsSafe` — it asserted APPROVE to document the fail-open the decoder closes, and now asserts the discriminating pair (decoded → BLOCK + `partition-count-shrunk`; un-decoded → REVIEW + `predicate.error`), so neither layer can rot silently. **Consequence for authors:** ordering quoted numerics means coercing first — `int(new) >= int(old)` (already the repo idiom) or `double(...)`; dates use `timestamp(...)`. **Ordering raw text is no longer expressible in tier-1 `assert`, in ANY spelling** — `string(a) < string(b)` and the byte-wise `bytes(a) < bytes(b)` are the same lexical sort and are refused with the bare form — and graduates to Rego (ADR-0002 escape hatch), consistent with ADR-0013's "don't grow a programming language in YAML". Equality, `in` and the string functions are untouched. Scope call: the guard is on `evalLeaf` only, NOT on `EvalScalar` — `{{ }}` message interpolation renders text and decides nothing, so guarding it would widen the blast radius for no safety gain. Records: **no `openspec/` change entry** — `openspec/changes/` holds only its README (this repo has never used change proposals in practice), and a fail-safe correction of an existing frozen semantic is proportionate to a D-row + ADR amendment + failing-test-first. Revert: drop `textOrderGuard` and restore `toCEL`'s `x.String()` fallback — reopens the APPROVE flip. **Review close-out (three non-blocking notes, decided by the author):** (F-1) the first cut watched only `types.String`, so `bytes(a) < bytes(b)` — the IDENTICAL byte-wise compare — slipped through and `bytes(new) >= bytes(old)` over the quoted `"12"` → `"6"` shrink still answered `true`. Not accident-reachable (the defect class is a quoted numeric in adopter YAML, not a deliberate `bytes()` call), so the choice was real: exempt it as a sanctioned explicit escape hatch, or refuse it. **Refused** — the lane had already rejected exempting `string(...)` because it "re-admits the identical defect", and that reasoning applies verbatim; a documented exemption would also advertise a one-token workaround for the guard's own error message, converting "not accident-reachable" into "the documented way round the guard". The refusal set is now `string` + `bytes`, and it is provably COMPLETE: the `_<_` overloads reachable from `newEvalEnv` were enumerated empirically — `int`/`uint`/`double`/`bool`/`duration`/`timestamp` are genuine orderings and pass through, `list`/`map`/`type`/mixed-numeric have no overload at all — so ADR-0013 Amendment 1's universal claim is now true as written rather than merely intended. `TestOnlyTextShapedOperandsAreRefused` pins all three buckets and reds if the env gains a comparable type (e.g. `CrossTypeNumericComparisons`). (F-2) the guard's refusal sentence was commented "adopter-facing"; it is not — both consumers use the error only for its non-nil-ness (`coverSubject` renders the AUTHORED leaf message, `evalRule`'s caller renders none), so no surface prints it today. Comment corrected to say what the consumers do; the sentence stays plain-worded so it is fit to surface if an engine error channel is ever added. (F-3) `{{ old }}`/`{{ new }}` over an unrepresentable literal interpolate the CEL error VALUE, rendering toCEL's sentence where a number belongs. Left as-is deliberately and GOLDENED: it is authored prose, not a Go-internals leak (GUIDELINES §Contracts), it decides nothing (the leaf already errored → REVIEW), it says WHY the rule could not decide where a bare `{{ old }}` placeholder would not, and re-opening this row's explicit "guard `evalLeaf`, not `EvalScalar`" scope call for a cosmetic P3 buys no safety. The golden also pins the DIVERGENCE between the two interpolators (`expandMessage` renders the sentence; `internal/render`'s `EvalScalar` path rejects the error value), so a future unification cannot change either silently. | diff --git a/docs/planning/open-questions.md b/docs/planning/open-questions.md index 2be4b09..d57e002 100644 --- a/docs/planning/open-questions.md +++ b/docs/planning/open-questions.md @@ -29,3 +29,5 @@ | OQ-26 | **`assent test` `score.total` faithfulness (P5-E6-S03).** The S03 matcher computes `score.total` as Σ `finding.Points` over the enforcing `Result.Findings`, but a finding carries the AUTHORED per-firing weight `r.Points`, not `firings*r.Points` (the engine's real `pointsSum`, an intentional aggregate asymmetry, ADR-0007 Amendment 2). So for a rule that fires K>1 times the matcher UNDERcounts — safe (it can only mismatch/FAIL, never spuriously pass on higher real risk) but not faithful. A faithful total needs the engine to expose the summed `pointsSum` on `Result` (a decision-path change, its OWN fail-safety-reviewed lane — parallel to the `findings[].path` field-add, D-054(b)). Until then S03 fixtures are single-firing so `total` is exact. | E6 fast-follow / engine lane | logged by S03; leading answer: add `Result.PointsTotal` in the path/score engine lane, then Match reads it | | OQ-28 | ~~**Filesystem containment for provider reads: is PATH containment enough, or must the injected FS itself be a security boundary?** (raised P5-E5-S07/S08 while implementing `builtin/repo-file` and `builtin/resource-owner`.) The builtins clip candidates to declared `roots` with pure string guards (`cleanRel`/`underAnyRoot`) over an `os.DirFS`. Under `--checkout` that FS is the merge request's own HEAD tree — contributor-authored content — and Go documents `os.DirFS` as **not** a security boundary while `fs.Stat` follows links. Question: does the invariant "never a fact from outside the declared roots" need a syscall-level root, a per-component symlink refusal, or both?~~ **Resolved (D-129): BOTH, and they are not substitutes.** (a) `cmd/assent` injects `builtin.OpenRepoRoot` = `os.OpenRoot` + `(*os.Root).FS()`, a syscall-level boundary for every consumer of that FS; (b) `classifyCandidate` Lstats every path component and refuses any symlinked candidate — the only layer that can protect the `roots` clip, which `os.Root` cannot see. In-root symlinks are refused too; refusal is `unavailable` with a contributor-readable reason and STOPS the walk-up. Retroactive row: D-129 and `REQ-E5-S07-03` cited "OQ-28" before this table carried it (AGENTS.md rule 6 — no dangling references). | — (closed) | [decisions.md](../decisions/decisions.md) D-129/D-130; `REQ-E5-S07-03`/`REQ-E5-S08-03`. **Residual CLOSED (D-133):** `collectTree`'s silent truncation (P0) and `readIfPresent`'s governed-subject symlink (P1) are both fixed in `cmd/assent/checkout.go`. **Proof relocated — stated here so nobody re-derives it wrongly:** D-133 refuses ANY symlink under `base/`/`head/` at changed-file ENUMERATION, before providers resolve, so this row's escape is no longer reproducible end-to-end through `assent run --checkout`. The provider guard is now defence in depth, proven at `cmd/assent`'s production fact-resolution seam (`TestResolveRunFactsRefusesSymlinkedQuotaCandidate`, which pins the two layers separately) plus `internal/provider/builtin/{repo_file,resource_owner}_symlink_test.go`; it becomes the live barrier again if ADR-0008 Amendment 2's fold-the-refusal-opaque direction lands — see D-129's 2026-08-09 amendment | | OQ-16 | ~~Which **open-source repos** join the demo/test corpus?~~ **Resolved (P2-E5):** kafka/org + JulieOps descriptors + octoDNS zones, pinned by SHA with vendored excerpts — see [examples/repos/corpus.md](https://github.com/PlatformRelay/assent/blob/main/examples/repos/corpus.md) | — | [adr-acceptance-review.md](adr-acceptance-review.md); D-008/D-029 extra private shapes deferred but kept in corpus plan | +| OQ-27 | ~~A relational CEL leaf over STRING-bound operands returns a silently WRONG boolean instead of erroring — a verified BLOCK→APPROVE flip (found by AUD-S13 / TEST-02, widened by review F4).~~ **RESOLVED (D-131 / ADR-0013 Amendment 1, merged on `main`):** an ordering operator over a text-shaped operand (`string` or `bytes`) now ERRORS — `internal/core/aggregate`'s `textOrderGuard` watches every relational operand as it evaluates and refuses text in either position, on both seams (`evalLeaf` and the walking-skeleton `evalRule`) — and `toCEL` no longer demotes an unrepresentable numeric literal to its string form, it binds a CEL error value. Ordering raw text graduates to Rego; `int()`/`double()`/`timestamp()` stay the tier-1 migration path. **The analysis below is retained as the record of how the defect was found and how far it reached — it describes the PRE-FIX engine.** **The class is any string-bound operand, not just numeric overflow.** cel-go's relational operators are DEFINED over two strings (lexical compare), so they return a clean boolean where the engine's fail-safe design assumes an error. **P1 instance — quoted YAML scalars, no overflow anywhere:** `internal/evaldecode` maps a `!!str` literal to a Go `string` BY DESIGN (the differ deliberately keeps the string `"12"` distinct from the number `12`) — but that design assumed a numeric rule over a string would fail safe, and it does not. Reproduced end-to-end through the production `aggregate.Cover` entry point with the D-016-shaped `partitions-must-not-shrink` rule (`new >= old`, onFailure block): `partitions: 12 → 6` (numeric) yields **BLOCK, 1 finding `partition-count-shrunk`**; the identical policy and subject with `partitions: "12" → "6"` (quoted) yields **APPROVE, ZERO findings** — `evalLeaf` returns `(true, nil)`, the obligation is recorded as PROVEN, and the destructive change auto-merges. **Second instance — numeric overflow (the original finding):** a `json.Number` fitting neither int64 nor float64 falls back to its string form (`evaluate.go:191`), so `9e399 > 1e400` evaluates `true` (arithmetically false). **No lint guard exists:** `checkLeafScope` and `checkPredicateScope` (`internal/lint/scope.go`) validate identifier SCOPE and `checkFactsShape` (`facts_ref.go:249`) validates facts-path shape — none type-checks relational operands, so an author gets no warning. This is the exact failure `internal/evaldecode`'s package doc warns about ("lexically `\"6\" >= \"12\"` is TRUE, so a partition shrink 12->6 would be judged non-destructive and APPROVE. That is the exact forbidden outcome") — the doc believed it had closed it; it closed only the *canonical-render* path, not the authored `!!str` path. **Also a docs-truth defect:** `evaldecode.go:61` names this "the ADR-0013 residual #1 the S02 evaluator owns" but describes it as "float64 (a lossy compare) or its string form", never saying the string form yields a silently wrong boolean rather than an error — so the residual reads as benign precision loss. **Severity split, kept explicit because conflating these is how a real finding gets dismissed:** the MECHANISM is **P1**; the over-range instance ALONE is **P2** on reachability (it needs BOTH operands to exceed ~1.8e308); the P1 rests on the quoted-string case, which needs only ordinary authored YAML. **Hard rule 7 (determinism) is NOT violated** — a lexical compare is perfectly deterministic and reproducible. What is violated is the **fail-safe direction** (GUIDELINES §2 / ADR-0013: undecidable or type-mismatched must error → REVIEW, never a permissive boolean) and `evaldecode`'s own written claim. AUD-S13 deliberately wrote NO test asserting `9e399 > 1e400 == true` or the quoted-string `true`; blessing either would enshrine the fail-open. Candidate fixes: make a relational leaf over string-bound operands ERROR (fail-safe, preferred — a lexical compare is almost never what a policy author meant); and/or a lint hard-error when a relational operator can bind a string; and/or reject over-range numerals at the loader boundary. Rejected: big.Float (reintroduces a decision-path numeric tower). | ~~release tag BLOCKED on this~~ **unblocked**; severity ruling done (P1), fixed in its own decision-path lane | found by AUD-S13 (PR #35), widened by independent review F4; NOT fixed there (tests-only lane). Fixed on `main` by **D-131** in a dedicated decision-path lane, as required; AUD-S13's TEST-02 was realigned to the refusal contract when this lane merged `main` | +| OQ-28 | **`builtin/repo-file` enforces PATH containment but not FILESYSTEM containment — a symlink reads arbitrary ABSOLUTE host paths, live in production (found by AUD-S13 / TEST-06, widened by review F5).** `cleanRel`/`cleanRoots`/`underAnyRoot` (`internal/provider/builtin/repo_file.go`) are pure STRING guards: they reject `../`, absolute anchors, root-escaping and sibling-prefix paths, and `TestRepoFileContainment` pins all of that. They cannot see the filesystem, and **`os.DirFS` is explicitly documented in Go as NOT a security boundary**. Path-level and filesystem-level containment are DIFFERENT guarantees; only the former is held. **Two forms, the second worse:** (1) a DIRECTORY symlink under a declared root — `topics/evil -> `, `Roots: ["topics"]`, `Anchor: "topics/evil/orders.yaml"` → a `resolved` fact valued from outside the roots; (2) review-reproduced and stronger, a **FILE symlink at a wholly legitimate in-root path** — `topics/prod/quota.yaml -> /abs/outside/cluster-secrets.yaml` — where every path string the guards inspect is impeccable and the target is an **arbitrary absolute host path**, not merely repo-relative. Either way this contradicts the AUD-S13 story's own criterion ("never a fact from outside the roots"). **This is LIVE IN PRODUCTION, not a test-only shape:** `cmd/assent/provider_host.go`'s `checkoutFS()` builds `os.DirFS(/head)` — the **MR head tree**, i.e. attacker-authored content — reachable via `assent run --checkout`. So the symlink arrives IN THE VERY MR UNDER EVALUATION. **Exfiltration seam:** a resolved, non-sensitive fact value reaches the forge-facing comment body through `render.formatMessageScalar` → `displayFactValue` (`internal/render/message.go` / `redact.go:18`), so a rule whose `message` interpolates a fact value **echoes the file's contents into the MR comment**; `sensitive: true` renders `[redacted]`, which is the only thing limiting it. **Constraint that keeps it narrow (do not overstate the finding):** the symlink target must be a YAML/JSON **mapping** carrying a top-level key equal to a declared output name — so it is not arbitrary file read of any format, it is arbitrary read of attacker-chosen *mapping-shaped* files, plus influence over any fact derived from them. **DISCRIMINATING QUESTION for the severity call — must be answered before ruling:** does `internal/change`'s differ/classifier see a symlink SWAP of a governed path as a change to that path? Git records the mode change 100644→120000, so forge-snapshot changed-file enumeration SHOULD surface it (and governance would then apply) — but this is asserted, not yet verified, and it decides whether the finding is "attacker can read arbitrary mapping-shaped files" or the strictly worse "attacker can **silently** influence a decision with no governed change visible". **Sibling with the same defect and a worse blast radius:** `builtin/resource-owner`'s `LoadResourceOwnerMap` (`internal/provider/builtin/resource_owner.go:93`) does a bare `fs.ReadFile(fsys, file)` over the same repo FS with **NO roots and no containment at all** — and that registry decides **who may approve**, so a symlinked ownership file is an authorization-boundary issue, not just a fact-integrity one. Candidate fix: `os.OpenRoot()` + `(*os.Root).FS()` (compile-verified available on this module's `go 1.25.0`), which refuses symlink traversal out of the root; alternatively reject symlinked candidates explicitly before read. `RepoFileOpts.FS` should also gain a documented contract ("must be a symlink-safe root") so the guarantee cannot be lost again by the caller. | **release tag BLOCKED on this**; severity ruling pending the differ question → dedicated provider lane | found by AUD-S13 (PR #35), widened by independent review F5; NOT fixed there (tests-only lane). Audit the checkout-mode change reader and every builtin that takes an `fs.FS` for the same assumption | diff --git a/internal/core/aggregate/evaluate_tocel_test.go b/internal/core/aggregate/evaluate_tocel_test.go new file mode 100644 index 0000000..1bde527 --- /dev/null +++ b/internal/core/aggregate/evaluate_tocel_test.go @@ -0,0 +1,279 @@ +package aggregate + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/google/cel-go/common/types" + "github.com/google/cel-go/common/types/ref" + + "github.com/PlatformRelay/assent/internal/core/policy" +) + +// overRange numbers fit NEITHER int64 NOR float64, so toCEL's third branch — the +// one that REFUSES to bind them at all (D-131 / ADR-0013 Amendment 1) — is what +// handles them. +// +// - "1e400" — grammatically valid JSON, ParseInt rejects the exponent form and +// ParseFloat reports value-out-of-range (+Inf), so both typed branches fail. +// - a 400-digit integer — ParseInt out of range; ParseFloat out of range too. +// +// A 60-digit integer is deliberately NOT here: it overflows int64 but Float64 +// succeeds (1.23e+59), so it takes the float64 branch and still compares (lossily) +// — the ADR-0013 residual #1, which D-131 deliberately left alone. +const ( + overRangeExp = "1e400" + overRangeDigits = "1" + // a 400-digit integer: beyond float64's ~1.8e308 too. + "0000000000000000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000000000000000000000000000000000000000000000000000000000000" + + "0000000000000000000" +) + +// TestToCELOverflowFailsSafe — REQ-AUD-S13-01 (TEST-02). +// +// toCEL binds an integral json.Number as int64 and a decimal as float64. A number +// that fits NEITHER is REFUSED: it binds a CEL ERROR value. It used to fall back +// to the literal's STRING form, which was a silent demotion to text — `9e399 > +// 1e400` became the lexical "9e399" > "1e400" = true, the numerically wrong answer +// with no error at all. D-131 closed that; this test pins the replacement contract +// with the rigour the fallback version had, and pins the arms that D-131's own +// suite (relational_string_test.go) does not: +// +// - the 400-digit NON-exponent literal. That suite only exercises 1e400/9e399/ +// -1e400, and "does ParseFloat reject it" is a different question for a plain +// digit string than for an exponent form. +// - WHY these literals reach the refusing branch — Int64 AND Float64 must BOTH +// fail. Nothing over there asserts that, so a future change binding 1e400 as +// +Inf would leave that suite green while this test quietly stopped testing +// the branch it names. +// - operators that are NOT relational: `type(new)`, `int(new)`, `string(new)`, +// `new - 1`. Every assertion over there reaches the refusal through a +// relational or `==` operator, where D-131's textOrderGuard is a second line +// of defence; here nothing but toCEL's own return value can produce the error. +// `int(...)` matters most: it is the BLESSED escape hatch for ordering quoted +// numerics (TestLegitimateComparesStillEvaluate depends on `int(new) >= +// int(old)` still working), so this pins that the hatch does not launder an +// unrepresentable literal back into something comparable. +// +// MUTATION that reds this test and leaves relational_string_test.go's whole +// suite green (verified): refuse only the exponent forms — `types.NewErr(...)` +// when the literal contains e/E, `return x.String()` otherwise. Every +// unrepresentable literal that suite uses is an exponent form, so the 400-digit +// arm below is the only thing that catches a half-closed fix. +// MUTATIONS that red this test too (not differential, but it must still catch +// them): restore `return x.String()` outright, or return a coercible sentinel +// (0, +Inf, math.MaxInt64) — `int(new) > 0` would then answer cleanly again. +func TestToCELOverflowFailsSafe(t *testing.T) { + env, err := newEvalEnv() + if err != nil { + t.Fatalf("env: %v", err) + } + in := EvaluationInput{} + + // First: both over-range literals really do reach the refusing branch. If a + // future toCEL change made either one typed, every case below would stop + // testing that branch and would start passing for the wrong reason. + for _, lit := range []string{overRangeExp, overRangeDigits} { + n := json.Number(lit) + if _, err := n.Int64(); err == nil { + t.Fatalf("%s: Int64 must fail for an over-range literal (else the int64 branch binds it)", lit) + } + if _, err := n.Float64(); err == nil { + t.Fatalf("%s: Float64 must fail for an over-range literal (else the float64 branch binds it)", lit) + } + bound := toCEL(n) + if s, isString := bound.(string); isString { + t.Fatalf("%s: toCEL returned the STRING form %q — the D-131 lexical demotion is back", lit, s) + } + rv, ok := bound.(ref.Val) + if !ok { + t.Fatalf("%s: toCEL bound %T (%v), want a CEL error value", lit, bound, bound) + } + if !types.IsError(rv) { + t.Fatalf("%s: toCEL bound a %s value (%v), want a CEL error", lit, rv.Type().TypeName(), rv.Value()) + } + // The equivalent of the old "the fallback kept the literal" assertion: a + // refusal an adopter cannot act on is not much better than a wrong answer, + // so the error must NAME the number it refused. (The exact sentence is + // goldened once, at the Cover/message seam, in relational_string_test.go's + // TestUnrepresentableNumericInterpolatesItsRefusal — not duplicated here.) + cause, isErr := rv.Value().(error) + if !isErr { + t.Fatalf("%s: the CEL error value carries %T, not an error", lit, rv.Value()) + } + if !strings.Contains(cause.Error(), lit) { + t.Fatalf("%s: the refusal does not name the number it refused: %q", lit, cause.Error()) + } + } + + // The refusal is observable in the ACTIVATION, not merely in toCEL's return + // value. This is the assertion that isolates toCEL's contract from D-131's + // textOrderGuard: `type(new) == string` contains no relational operator, so + // the guard provably is not what makes it fail — the bound value itself is. + // Under the string fallback this predicate was a clean `true`; it must now be + // unanswerable, because a value the engine refused to bind must not be + // inspectable as if it had been bound. + overCh := EvalChange{File: "topics/x.yaml", Path: "/partitions", Kind: "modify", + Old: json.Number("100"), New: json.Number(overRangeExp)} + isStr, err := evalLeaf(env, in, overCh, "prod", "type(new) == string") + if err == nil { + t.Fatalf("`type(new) == string` returned %v with NO error — an unrepresentable literal is answering predicates again", isStr) + } + if isStr { + t.Fatal("`type(new) == string` errored but still reported satisfied") + } + + // Every predicate shape a real policy uses over that binding must ERROR — and + // the error must be attributable to the refused literal, not to some unrelated + // overload failure that would happen to look the same. + cases := []struct { + name string + ch EvalChange + expr string + lit string // the over-range literal whose refusal must be the cause + }{ + { + name: "relational_against_in_range_old", + ch: EvalChange{File: "a.yaml", Path: "/n", Kind: "modify", Old: json.Number("100"), New: json.Number(overRangeExp)}, + expr: "new >= old", + lit: overRangeExp, + }, + { + name: "relational_against_int_literal", + ch: EvalChange{File: "a.yaml", Path: "/n", Kind: "modify", Old: json.Number("100"), New: json.Number(overRangeExp)}, + expr: "new > 100", + lit: overRangeExp, + }, + { + name: "over_range_on_the_old_side", + ch: EvalChange{File: "a.yaml", Path: "/n", Kind: "modify", Old: json.Number(overRangeExp), New: json.Number("6")}, + expr: "new >= old", + lit: overRangeExp, + }, + { + name: "digit_overflow_relational", + ch: EvalChange{File: "a.yaml", Path: "/n", Kind: "modify", Old: json.Number("12"), New: json.Number(overRangeDigits)}, + expr: "new >= old", + lit: overRangeDigits, + }, + { + // int() is the documented escape hatch for ordering a quoted numeric. + // It must NOT double as a way to make an unrepresentable literal + // comparable again. + name: "explicit_int_conversion", + ch: EvalChange{File: "a.yaml", Path: "/n", Kind: "modify", Old: json.Number("1"), New: json.Number(overRangeExp)}, + expr: "int(new) > 0", + lit: overRangeExp, + }, + { + // Asking explicitly for the very value the old fallback handed over + // must not resurrect it. + name: "explicit_string_conversion", + ch: EvalChange{File: "a.yaml", Path: "/n", Kind: "modify", Old: json.Number("1"), New: json.Number(overRangeExp)}, + expr: `string(new) == "` + overRangeExp + `"`, + lit: overRangeExp, + }, + { + // Arithmetic, not comparison: the refusal propagates through operators + // the D-131 guard never watches. + name: "arithmetic_against_in_range", + ch: EvalChange{File: "a.yaml", Path: "/n", Kind: "modify", Old: json.Number("1"), New: json.Number(overRangeExp)}, + expr: "new - 1 > 0", + lit: overRangeExp, + }, + } + // Positive control: a table that silently iterated nothing would pass vacuously. + if len(cases) != 7 { + t.Fatalf("table lost cases: have %d, want 7", len(cases)) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := evalLeaf(env, in, tc.ch, "prod", tc.expr) + if err == nil { + t.Fatalf("%q over an over-range value returned %v with NO error — "+ + "a value assent refused to bind silently produced a boolean", tc.expr, got) + } + // evalLeaf's contract: an erroring predicate is never reported satisfied. + if got { + t.Fatalf("%q errored (%v) but still reported satisfied", tc.expr, err) + } + if !strings.Contains(err.Error(), tc.lit) { + t.Fatalf("%q errored with %q, which does not name the refused literal — "+ + "the failure is not attributable to toCEL's refusal", tc.expr, err) + } + }) + } +} + +// TestToCELOverflowNeverApprovesThroughCover — REQ-AUD-S13-01, production entry +// point. The unit assertions above prove evalLeaf errors; this proves the error +// actually reaches the decision as a fail-safe REVIEW through Cover, the real +// coverage loop. A `partitions-must-not-shrink`-shaped rule whose subject carries +// an over-range new value must surface predicate.error/require-review — it must +// not prove the obligation and it must never APPROVE. +// +// relational_string_test.go also drives an unrepresentable literal through Cover +// (TestUnrepresentableNumericInterpolatesItsRefusal, which goldens the rendered +// message). What that test cannot show, because every input it hands Cover +// errors, is that this policy DECIDES anything at all: the control below is the +// difference between "the refusal produced REVIEW" and "this fixture produces +// REVIEW no matter what you feed it". +func TestToCELOverflowNeverApprovesThroughCover(t *testing.T) { + pol := &policy.MergePolicy{ + Spec: policy.MergePolicySpec{ + Rules: []policy.Rule{{ + Name: "partitions-must-not-shrink", + Phase: policy.PhaseEnforce, + 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"}, + }}, + }, + } + bind := &policy.Binding{Require: []string{"non-destructive"}, Environment: "prod"} + + // Control: an ordinary in-range shrink proves the rule FAILS normally (BLOCK + // via the rule's own onFailure code) — so the over-range case below is + // distinguishable from "this policy blocks everything". + inRange := &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{ + {Subject: "topic:orders", File: "topics/orders.yaml", Path: "/partitions", Kind: "modify", + Old: json.Number("12"), New: json.Number("6")}, + }}} + ctrl, err := Cover(pol, bind, inRange) + if err != nil { + t.Fatalf("Cover (control): %v", err) + } + if len(ctrl.Findings) != 1 || ctrl.Findings[0].Code != "partition-count-shrunk" { + t.Fatalf("control must produce the rule's own onFailure finding, got %+v", ctrl.Findings) + } + + // The over-range case: the value was refused at binding time, so the compare + // cannot be made and the obligation is UNPROVEN via predicate.error — not + // proven, and not the rule's own effect. + overRange := &EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{ + {Subject: "topic:orders", File: "topics/orders.yaml", Path: "/partitions", Kind: "modify", + Old: json.Number("12"), New: json.Number(overRangeExp)}, + }}} + got, err := Cover(pol, bind, overRange) + if err != nil { + t.Fatalf("Cover (over-range): %v", err) + } + if got.Decision == DecisionApprove { + t.Fatal("an over-range value assent refused to bind must never APPROVE") + } + if len(got.Findings) != 1 { + t.Fatalf("want exactly one finding, got %+v", got.Findings) + } + f := got.Findings[0] + if f.Code != "predicate.error" || f.Effect != EffectRequireReview { + t.Fatalf("want predicate.error/require-review (fail-safe), got code=%q effect=%q", f.Code, f.Effect) + } + if !strings.Contains(string(got.Decision), "REVIEW") { + t.Fatalf("decision = %q, want the fail-safe REVIEW", got.Decision) + } +} diff --git a/internal/forge/clearslot_test.go b/internal/forge/clearslot_test.go new file mode 100644 index 0000000..183157e --- /dev/null +++ b/internal/forge/clearslot_test.go @@ -0,0 +1,290 @@ +package forge_test + +import ( + "encoding/json" + "errors" + "strings" + "testing" + + "github.com/PlatformRelay/assent/internal/forge" + "github.com/PlatformRelay/assent/internal/forge/fake" +) + +// scriptedForge wraps the in-memory fake to inject the forge-side FAILURES the +// clear-slot path must fail closed on. The fake is a happy-path substrate: it +// never errors from ListBotThreads and its ResolveThread always succeeds, so the +// error arms of reconcileClearSlot are unreachable through it alone. +// +// It EMBEDS *fake.Forge rather than reimplementing the port, so a port change +// (new method) does not silently drop this stub out of forge.Forge — and so the +// non-scripted behaviour stays the real fake's, not a second hand-rolled model +// that could drift from it. +type scriptedForge struct { + *fake.Forge + + // listErrAt is the 1-based ListBotThreads call ordinal that fails; 0 = never. + // Ordinal 1 is reconcileClearSlot's own listing, ordinal 2 the post-write + // rescan (P3-E5 step 9) — the two distinct list sites in this path. + listErrAt int + listCalls int + + // resolveErr, when set, is returned by ResolveThread (the forge refuses the + // write). resolveSilentNoop instead returns SUCCESS while leaving the thread + // OPEN — a forge that lies about having resolved, which is exactly the + // partial clear the rescan exists to catch. + resolveErr error + resolveSilentNoop bool +} + +var errForgeUnavailable = errors.New("forge unavailable (503)") + +func (s *scriptedForge) ListBotThreads(project, mr string) ([]forge.Thread, error) { + s.listCalls++ + if s.listErrAt != 0 && s.listCalls == s.listErrAt { + return nil, errForgeUnavailable + } + return s.Forge.ListBotThreads(project, mr) +} + +func (s *scriptedForge) ResolveThread(project, mr, id string) error { + if s.resolveErr != nil { + return s.resolveErr + } + if s.resolveSilentNoop { + return nil + } + return s.Forge.ResolveThread(project, mr, id) +} + +// static assertion that the scripted stub is still a full Forge. +var _ forge.Forge = (*scriptedForge)(nil) + +// otherSlotMarker is a marker for a DIFFERENT, healthy slot on the same MR. It is +// seeded in every case so each assertion also proves the clear is slot-scoped: +// clearing one slot must never resolve another slot's open thread. +func otherSlotMarker() forge.Marker { + m := reviewMarker() + m.Slot.Rule = "placement/region-allowlist" + return m +} + +// openForSlot counts UNRESOLVED bot threads for a slot, reading through the +// embedded fake directly so a scripted list error cannot distort the assertion. +func openForSlot(t *testing.T, f *scriptedForge, slot forge.Slot) int { + t.Helper() + threads, err := f.Forge.ListBotThreads(proj, mrIID) + if err != nil { + t.Fatalf("assertion listing failed: %v", err) + } + n := 0 + for _, th := range threads { + if th.Marker.Slot == slot && !th.Resolved { + n++ + } + } + return n +} + +// TestReconcileClearSlotBranches — REQ-AUD-S13-02 (TEST-05). +// +// Drives the PRODUCTION entry point (forge.Reconcile) down the clear-slot path +// and walks every branch of reconcileClearSlot. Each ERROR branch is asserted at +// BOTH polarities: the refusal AND the neighbouring success that proves the +// refusal was caused by the injected failure and not by the fixture being inert. +// +// The fail-closed teeth, asserted on every erroring case: +// - ZERO operations on the returned receipt (an empty-ops receipt is not even +// representable in the frozen schema, so a refusal must never fabricate one); +// - the slot's open thread is STILL OPEN (no half-write left behind); +// - the unrelated healthy slot is untouched. +func TestReconcileClearSlotBranches(t *testing.T) { + slot := reviewMarker().Slot + otherSlot := otherSlotMarker().Slot + + desired := forge.DesiredReviewState{ + Project: proj, + MR: mrIID, + ClearSlot: &slot, + } + + cases := []struct { + name string + // seed prepares the forge state and the failure script. + seed func(f *scriptedForge) + // wantErrIs is a sentinel the error must match (nil when none expected). + wantErrIs error + // wantErrSub is a substring the error message must carry ("" when none). + wantErrSub string + // wantOpID is the receipt operation's targetId on the success polarity. + wantOpID string + // wantOpenAfter is how many threads for the cleared slot remain OPEN. + wantOpenAfter int + }{ + { + // Branch: ListBotThreads error. The slot cannot be inspected, so the + // clear cannot be proven — refuse rather than assume nothing is open. + name: "list_error_fails_closed", + seed: func(f *scriptedForge) { + f.SeedThread("note/9001", botID, reviewMarker(), false) + f.listErrAt = 1 + }, + wantErrSub: "list bot threads", + wantOpenAfter: 1, + }, + { + // Polarity for the branch above AND for the resolve/rescan branches + // below: the ordinary single-open-thread clear succeeds. + name: "single_open_thread_resolved", + seed: func(f *scriptedForge) { + f.SeedThread("note/9001", botID, reviewMarker(), false) + }, + wantOpID: "note/9001", + wantOpenAfter: 0, + }, + { + // Branch: no bot thread for the slot at all. There is nothing to + // reference, so no schema-valid receipt exists — fail closed with the + // typed sentinel rather than invent an operation that did not happen. + name: "no_thread_for_slot_unsupported", + seed: func(_ *scriptedForge) { + // Only the OTHER slot has a thread; the cleared slot has none. + }, + wantErrIs: forge.ErrUnsupportedDecision, + wantOpenAfter: 0, + }, + { + // Branch: idempotent clear. The slot's thread is ALREADY resolved, so + // zero writes occur, yet the receipt still references the thread so it + // validates against the frozen schema (operations minItems:1). + name: "already_clear_is_idempotent", + seed: func(f *scriptedForge) { + f.SeedThread("note/9001", botID, reviewMarker(), true) + }, + wantOpID: "note/9001", + wantOpenAfter: 0, + }, + { + // Branch: the idempotent arm ALSO rescans. If that rescan listing + // fails, the already-clear state is unconfirmed — refuse. + name: "already_clear_rescan_list_error", + seed: func(f *scriptedForge) { + f.SeedThread("note/9001", botID, reviewMarker(), true) + f.listErrAt = 2 // the post-write rescan listing + }, + wantErrSub: "rescan list bot threads", + wantOpenAfter: 0, + }, + { + // Branch: ResolveThread refuses. No receipt, and the thread stays open. + name: "resolve_error_fails_closed", + seed: func(f *scriptedForge) { + f.SeedThread("note/9001", botID, reviewMarker(), false) + f.resolveErr = errForgeUnavailable + }, + wantErrSub: "resolve no-longer-desired note/9001", + wantOpenAfter: 1, + }, + { + // Branch: PARTIAL CLEAR. ResolveThread reports success but the thread + // is still open on the forge. Success must never be reported without + // forge confirmation — the rescan catches the lie. + name: "partial_clear_rescan_mismatch", + seed: func(f *scriptedForge) { + f.SeedThread("note/9001", botID, reviewMarker(), false) + f.resolveSilentNoop = true + }, + wantErrIs: forge.ErrRescanFailed, + wantOpenAfter: 1, + }, + { + // Branch: two open threads occupy the cleared slot. Which to resolve is + // undecidable here, so refuse with ZERO writes rather than guess. + name: "duplicate_open_threads_fail_closed", + seed: func(f *scriptedForge) { + f.SeedThread("note/9001", botID, reviewMarker(), false) + f.SeedThread("note/9003", botID, reviewMarker(), false) + }, + wantErrSub: "duplicate open threads", + wantOpenAfter: 2, + }, + } + // Positive control: a table that lost its cases would sweep an empty set and + // pass vacuously. + if len(cases) != 8 { + t.Fatalf("table lost cases: have %d, want 8", len(cases)) + } + // Both polarities must actually be present: at least one refusal and at least + // one success, else "both-polarity" would be a claim the table cannot back. + var errCases, okCases int + for _, tc := range cases { + if tc.wantErrIs != nil || tc.wantErrSub != "" { + errCases++ + } else { + okCases++ + } + } + if errCases < 5 || okCases < 2 { + t.Fatalf("table must keep both polarities: %d error cases, %d success cases", errCases, okCases) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := &scriptedForge{Forge: fake.New(botID, "src", "tgt", "sha256:merge")} + // A healthy, unrelated slot on the same MR — must survive every case. + f.SeedThread("note/9500", botID, otherSlotMarker(), false) + tc.seed(f) + + receipt, err := forge.Reconcile(f, testClock(), desired, forge.Preconditions{}) + + switch { + case tc.wantErrIs != nil: + if !errors.Is(err, tc.wantErrIs) { + t.Fatalf("err = %v, want errors.Is %v", err, tc.wantErrIs) + } + case tc.wantErrSub != "": + if err == nil { + t.Fatalf("want an error containing %q, got success with receipt %+v", tc.wantErrSub, receipt) + } + if !strings.Contains(err.Error(), tc.wantErrSub) { + t.Fatalf("err = %v, want it to contain %q", err, tc.wantErrSub) + } + default: + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + } + + if tc.wantErrIs != nil || tc.wantErrSub != "" { + // Fail-closed: a refusal fabricates no operation. + if len(receipt.Operations) != 0 { + t.Fatalf("a refusal must return ZERO operations, got %+v", receipt.Operations) + } + } else { + if len(receipt.Operations) != 1 { + t.Fatalf("want exactly one operation, got %+v", receipt.Operations) + } + op := receipt.Operations[0] + if op.TargetID != tc.wantOpID { + t.Fatalf("operation targetId = %q, want %q", op.TargetID, tc.wantOpID) + } + // The receipt must satisfy the frozen schema — this is the reason + // the idempotent arm references a thread instead of returning empty. + raw, mErr := json.Marshal(receipt) + if mErr != nil { + t.Fatalf("marshal receipt: %v", mErr) + } + if vErr := validateReceipt(t, raw); vErr != nil { + t.Fatalf("receipt does not validate against the frozen schema: %v", vErr) + } + } + + if got := openForSlot(t, f, slot); got != tc.wantOpenAfter { + t.Fatalf("open threads for the cleared slot = %d, want %d", got, tc.wantOpenAfter) + } + // Slot scoping: the unrelated healthy slot is never touched. + if got := openForSlot(t, f, otherSlot); got != 1 { + t.Fatalf("the unrelated slot's open thread must survive, open = %d want 1", got) + } + }) + } +} diff --git a/internal/provider/builtin/repo_file_test.go b/internal/provider/builtin/repo_file_test.go index b5e67a9..37df9e9 100644 --- a/internal/provider/builtin/repo_file_test.go +++ b/internal/provider/builtin/repo_file_test.go @@ -3,9 +3,12 @@ package builtin_test import ( "context" "encoding/json" + "io/fs" "os" "path/filepath" + "strings" "testing" + "time" "github.com/PlatformRelay/assent/internal/provider" "github.com/PlatformRelay/assent/internal/provider/builtin" @@ -227,3 +230,468 @@ func TestBuiltinRepoFileAbsentUnavailable(t *testing.T) { } }) } + +// containmentFixture builds a checkout-shaped tree whose contents are chosen so +// that ANY containment failure is observable as a WRONG VALUE, not merely as a +// different state: +// +// quota.yaml 12 repo root (above a "topics" clip) +// topics/quota.yaml 24 inside the clip +// topics/prod/quota.yaml 6 most specific +// topics-archive/quota.yaml 99 SIBLING of the clip — sharing its name prefix +// rootonly.yaml exists ONLY at the repo root +// +// 99 is the sibling-prefix tell: a containment check written as +// strings.HasPrefix(p, root) instead of HasPrefix(p, root+"/") treats +// "topics-archive/..." as inside "topics". The assertion that BITES is the state +// one (the case expects unavailable and the leak makes it resolved); the distinct +// per-directory values exist so the failure output names the source unambiguously +// rather than leaving "resolved, but from where?". +func containmentFixture(t *testing.T) fs.FS { + t.Helper() + dir := t.TempDir() + write := func(rel, body string) { + t.Helper() + full := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(full), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + write("quota.yaml", "max_partitions: 12\n") + write("topics/quota.yaml", "max_partitions: 24\n") + write("topics/prod/quota.yaml", "max_partitions: 6\n") + write("topics-archive/quota.yaml", "max_partitions: 99\n") + write("rootonly.yaml", "max_partitions: 77\n") + return os.DirFS(dir) +} + +// TestRepoFileContainment — REQ-AUD-S13-03 (TEST-06), security-adjacent. +// +// Pins the PATH-CONTAINMENT surface (cleanRel / cleanRoots / underAnyRoot): a +// traversal, absolute, or root-escaping path must never yield a fact, and the +// declared roots must actually clip the walk-up. The load-bearing assertion on +// every rejecting case is that `fact.Value` is nil — the story's invariant is +// "never a fact from outside the roots", so a rejection that still carried a +// value would be the failure, whatever state label it wore. +// +// SCOPE (see the PR body): these are STRING-level guards. `os.DirFS` is +// explicitly not a security boundary in Go, and a real on-disk symlink under a +// declared root is NOT rejected by this code. The "symlink-shaped" inputs below +// are traversal-shaped path strings, which is what cleanRel/underAnyRoot can +// actually decide. +func TestRepoFileContainment(t *testing.T) { + fsys := containmentFixture(t) + + cases := []struct { + name string + nilFS bool + file string + anchor string + roots []string + wantState string + // wantValue is asserted only when wantState is resolved. + wantValue any + // wantReason is a substring the non-resolved fact's reason must carry, so + // a case cannot pass by being rejected for an unrelated reason. + wantReason string + }{ + { + // Control: the fixture CAN produce a fact. Without this, every + // rejection below could be an inert harness rather than a guard. + name: "control_under_declared_root_resolves", file: "quota.yaml", + anchor: "topics/prod/orders.yaml", roots: []string{"topics"}, + wantState: provider.StateResolved, wantValue: float64(6), + }, + { + name: "anchor_absolute_rejected", file: "quota.yaml", + anchor: "/etc/passwd", + wantState: provider.StateInvalid, wantReason: "anchor must be relative", + }, + { + name: "anchor_traversal_rejected", file: "quota.yaml", + anchor: "../../etc/passwd", + wantState: provider.StateInvalid, wantReason: "anchor escapes filesystem root", + }, + { + // Traversal that only escapes AFTER normalization — the shape a naive + // strings.Contains("..") check would catch but a naive one would not. + name: "anchor_traversal_after_normalization_rejected", file: "quota.yaml", + anchor: "topics/prod/../../../outside/x.yaml", + wantState: provider.StateInvalid, wantReason: "anchor escapes filesystem root", + }, + { + // Backslash-separated traversal is normalized to "/" first, so a + // Windows-shaped escape cannot slip past the "../" check. + name: "anchor_backslash_traversal_rejected", file: "quota.yaml", + anchor: `..\..\etc\passwd`, + wantState: provider.StateInvalid, wantReason: "anchor escapes filesystem root", + }, + { + name: "root_absolute_rejected", file: "quota.yaml", + anchor: "topics/prod/orders.yaml", roots: []string{"/etc"}, + wantState: provider.StateInvalid, wantReason: "root", + }, + { + name: "root_traversal_rejected", file: "quota.yaml", + anchor: "topics/prod/orders.yaml", roots: []string{"../outside"}, + wantState: provider.StateInvalid, wantReason: "root", + }, + { + // A "." root is the whole FS, i.e. NO clip — so the walk-up reaches the + // repo root. Polarity partner for the two root rejections above. + name: "root_dot_is_whole_fs_not_a_clip", file: "quota.yaml", + anchor: "services/billing/app.yaml", roots: []string{"."}, + wantState: provider.StateResolved, wantValue: float64(12), + }, + { + // THE SIBLING-PREFIX CASE. "topics-archive" shares "topics"' prefix but + // is a different directory: it is NOT under the declared root, so no + // fact — and in particular never the 99 that lives there. + name: "sibling_prefix_directory_is_not_under_root", file: "quota.yaml", + anchor: "topics-archive/orders.yaml", roots: []string{"topics"}, + wantState: provider.StateUnavailable, wantReason: "anchor outside declared roots", + }, + { + // The clip must also stop the WALK-UP, not just the anchor check: the + // file exists only above the root, so it must stay invisible. + name: "walkup_above_root_is_clipped", file: "rootonly.yaml", + anchor: "topics/prod/orders.yaml", roots: []string{"topics"}, + wantState: provider.StateUnavailable, wantReason: "no matching file", + }, + { + // An empty anchor normalizes to "." (the repo root). The anchor guard + // deliberately exempts "." (`&& anchor != "."`), so the clip is enforced + // one step later, by the walk-up: every candidate is outside the root, so + // the root-level quota.yaml (12) stays invisible and NO fact is produced. + // The reason is therefore the walk-up's, not the anchor guard's. + name: "empty_anchor_with_roots_yields_no_fact", file: "quota.yaml", + anchor: "", roots: []string{"topics"}, + wantState: provider.StateUnavailable, wantReason: "no matching file", + }, + { + // File is a BASENAME, never a path: a traversal in it is basenamed away, + // so it resolves from inside the roots (6) — it cannot reach ../../etc. + name: "file_traversal_is_basenamed_not_followed", file: "../../etc/quota.yaml", + anchor: "topics/prod/orders.yaml", roots: []string{"topics"}, + wantState: provider.StateResolved, wantValue: float64(6), + }, + { + name: "file_dot_rejected", file: ".", + anchor: "topics/prod/orders.yaml", + wantState: provider.StateInvalid, wantReason: "File must be a basename", + }, + { + name: "file_dotdot_rejected", file: "..", + anchor: "topics/prod/orders.yaml", + wantState: provider.StateInvalid, wantReason: "File must be a basename", + }, + { + name: "file_empty_rejected", file: " ", + anchor: "topics/prod/orders.yaml", + wantState: provider.StateInvalid, wantReason: "FS and File are required", + }, + { + name: "nil_fs_rejected", nilFS: true, file: "quota.yaml", + anchor: "topics/prod/orders.yaml", + wantState: provider.StateInvalid, wantReason: "FS and File are required", + }, + } + // Positive control: a table that lost its cases would sweep an empty set. + if len(cases) != 16 { + t.Fatalf("table lost cases: have %d, want 16", len(cases)) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + q := repoFileQuery("q-aud-s13-"+tc.name, []string{"max_partitions"}) + opts := builtin.RepoFileOpts{ + FS: fsys, + File: tc.file, + Anchor: tc.anchor, + Roots: tc.roots, + Declarations: map[string]provider.Declaration{"max_partitions": quotaDecl()}, + } + if tc.nilFS { + opts.FS = nil + } + + result := provider.ResolveFacts(context.Background(), callRepoFile(t, opts, q), q, fixedAsOf) + fact, ok := result.Facts["max_partitions"] + if !ok { + t.Fatal("requested output omitted from the result — fail-open via absence") + } + if fact.State != tc.wantState { + t.Fatalf("state = %q (reason %q), want %q", fact.State, fact.Reason, tc.wantState) + } + + if tc.wantState == provider.StateResolved { + if fact.Value != tc.wantValue { + t.Fatalf("value = %#v, want %#v", fact.Value, tc.wantValue) + } + return + } + + // The invariant: a rejected path NEVER carries a value, whatever the + // state label. This is what "never a fact from outside the roots" means. + if fact.Value != nil { + t.Fatalf("a rejected path must carry NO value; got %#v (reason %q)", fact.Value, fact.Reason) + } + if !strings.Contains(fact.Reason, tc.wantReason) { + t.Fatalf("reason = %q, want it to contain %q (rejected for the wrong cause?)", fact.Reason, tc.wantReason) + } + }) + } +} + +// TestRepoFileUndecodableDocument — REQ-AUD-S13-03 (TEST-06), the third +// rejection axis in answerRepoFile's fail-closed list ("undecodable body → +// invalid"), alongside containment and expiry. +// +// A repo file the provider cannot parse, and a file that parses to nothing, are +// both states where the provider knows nothing. Neither may become a fact: an +// undecodable document must be invalid, and an empty/null one must NOT become a +// resolved-empty fact pretending the key was present. +func TestRepoFileUndecodableDocument(t *testing.T) { + cases := []struct { + name string + body string + wantState string + wantValue any + wantReason string + }{ + { + // Control: the same harness DOES produce a fact from a good document. + name: "control_valid_document_resolves", + body: "max_partitions: 6\n", + wantState: provider.StateResolved, wantValue: float64(6), + }, + { + name: "unparseable_yaml_is_invalid", + body: "max_partitions: [unclosed\n", + wantState: provider.StateInvalid, wantReason: "decode", + }, + { + // A top-level sequence is well-formed YAML but not a mapping, so it + // cannot be keyed by output name — undecodable for this provider. + name: "top_level_sequence_is_invalid", + body: "- a\n- b\n", + wantState: provider.StateInvalid, wantReason: "decode", + }, + { + // Empty document: present-but-empty mapping, so the requested key is + // ABSENT (invalid) — never a resolved fact carrying a nil/zero value. + name: "empty_document_is_key_absent_not_resolved_empty", + body: "", + wantState: provider.StateInvalid, wantReason: "key absent", + }, + { + // Explicit YAML null decodes to a nil document — same treatment. + name: "null_document_is_key_absent_not_resolved_empty", + body: "null\n", + wantState: provider.StateInvalid, wantReason: "key absent", + }, + } + // Positive control: a table that lost its cases would sweep an empty set. + if len(cases) != 5 { + t.Fatalf("table lost cases: have %d, want 5", len(cases)) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "quota.yaml"), []byte(tc.body), 0o600); err != nil { + t.Fatal(err) + } + q := repoFileQuery("q-aud-s13-doc-"+tc.name, []string{"max_partitions"}) + opts := builtin.RepoFileOpts{ + FS: os.DirFS(dir), + File: "quota.yaml", + Anchor: "topics/prod/orders.yaml", + Declarations: map[string]provider.Declaration{"max_partitions": quotaDecl()}, + } + + result := provider.ResolveFacts(context.Background(), callRepoFile(t, opts, q), q, fixedAsOf) + fact, ok := result.Facts["max_partitions"] + if !ok { + t.Fatal("requested output omitted from the result — fail-open via absence") + } + if fact.State != tc.wantState { + t.Fatalf("state = %q (reason %q), want %q", fact.State, fact.Reason, tc.wantState) + } + if tc.wantState == provider.StateResolved { + if fact.Value != tc.wantValue { + t.Fatalf("value = %#v, want %#v", fact.Value, tc.wantValue) + } + return + } + if fact.Value != nil { + t.Fatalf("an undecodable/empty document must yield NO value; got %#v", fact.Value) + } + if !strings.Contains(fact.Reason, tc.wantReason) { + t.Fatalf("reason = %q, want it to contain %q", fact.Reason, tc.wantReason) + } + }) + } +} + +// TestRepoFileExpiry — REQ-AUD-S13-03 (TEST-06), the expiry half. +// +// `expiresAt` is the only thing standing between a stale repo file and a fact +// the engine treats as current. An undeclared, unparseable, zero or negative +// maxAge is UNDECIDABLE freshness, so it must synthesize a non-resolved state — +// never a fact with no expiry, and never a fact that outlives its window. +// +// The boundary is asserted at both polarities one nanosecond apart: expiry is +// EXCLUSIVE (a fact is already expired AT its expiresAt instant). +func TestRepoFileExpiry(t *testing.T) { + fsys := containmentFixture(t) + const day = 24 * time.Hour + + cases := []struct { + name string + maxAge string + // now is the evaluation instant handed to ResolveFacts; the query's AsOf + // stays fixedAsOf, so expiresAt is always fixedAsOf+maxAge. + now time.Time + + // wantProviderState/Reason are asserted on the builtin's OWN response + // bytes. They pin `expiresAt` itself: for a malformed maxAge the host's + // schema gate rejects the echoed declaration first and overwrites the + // reason, so asserting only the host outcome would never see which + // expiresAt branch actually fired. + wantProviderState string + wantProviderReason string + + // wantState/wantReason are the host outcome after ResolveFacts. + wantState string + wantReason string + // wantExpiry is the expiresAt a resolved fact must carry. + wantExpiry time.Time + }{ + { + // Polarity partner for every rejection below. + name: "valid_maxAge_resolves_with_expiry", maxAge: "24h", now: fixedAsOf, + wantProviderState: provider.StateResolved, + wantState: provider.StateResolved, wantExpiry: fixedAsOf.Add(day), + }, + { + name: "missing_maxAge_is_invalid", maxAge: "", now: fixedAsOf, + wantProviderState: provider.StateInvalid, wantProviderReason: "maxAge is required", + wantState: provider.StateInvalid, + }, + { + name: "unparseable_maxAge_is_invalid", maxAge: "24", now: fixedAsOf, + wantProviderState: provider.StateInvalid, wantProviderReason: `maxAge "24"`, + wantState: provider.StateInvalid, + }, + { + name: "zero_maxAge_is_invalid", maxAge: "0s", now: fixedAsOf, + wantProviderState: provider.StateInvalid, wantProviderReason: "must be positive", + wantState: provider.StateInvalid, wantReason: "must be positive", + }, + { + name: "negative_maxAge_is_invalid", maxAge: "-1h", now: fixedAsOf, + wantProviderState: provider.StateInvalid, wantProviderReason: "must be positive", + wantState: provider.StateInvalid, + }, + { + // Boundary, fresh side: one nanosecond before expiry it still resolves. + name: "one_ns_before_expiry_still_resolves", maxAge: "24h", + now: fixedAsOf.Add(day - time.Nanosecond), + wantProviderState: provider.StateResolved, + wantState: provider.StateResolved, wantExpiry: fixedAsOf.Add(day), + }, + { + // Boundary, stale side: AT the expiry instant it is ALREADY expired + // (the host's check is `!expiresAt.After(now)` — exclusive). The + // provider still answers resolved; the host is what ages it out. + name: "exactly_at_expiry_instant_is_expired", maxAge: "24h", + now: fixedAsOf.Add(day), + wantProviderState: provider.StateResolved, + wantState: provider.StateExpired, wantReason: "not after the evaluation instant", + }, + { + name: "past_expiry_is_expired", maxAge: "24h", + now: fixedAsOf.Add(day + time.Hour), + wantProviderState: provider.StateResolved, + wantState: provider.StateExpired, wantReason: "not after the evaluation instant", + }, + } + // Positive control: a table that lost its cases would sweep an empty set. + if len(cases) != 8 { + t.Fatalf("table lost cases: have %d, want 8", len(cases)) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + decl := quotaDecl() + decl.MaxAge = tc.maxAge + + q := repoFileQuery("q-aud-s13-exp-"+tc.name, []string{"max_partitions"}) + opts := builtin.RepoFileOpts{ + FS: fsys, + File: "quota.yaml", + Anchor: "topics/prod/orders.yaml", + Declarations: map[string]provider.Declaration{"max_partitions": decl}, + } + + // Layer 1 — the builtin's own answer, which is where expiresAt decides. + raw, err := builtin.CallRepoFile(context.Background(), opts, q) + if err != nil { + t.Fatalf("CallRepoFile: %v", err) + } + var resp provider.FactResponse + if err := json.Unmarshal(raw, &resp); err != nil { + t.Fatalf("unmarshal provider response: %v", err) + } + if len(resp.Facts) != 1 { + t.Fatalf("provider returned %d facts, want 1", len(resp.Facts)) + } + pf := resp.Facts[0] + if pf.State != tc.wantProviderState { + t.Fatalf("provider state = %q (reason %q), want %q", pf.State, pf.Reason, tc.wantProviderState) + } + if tc.wantProviderReason != "" && !strings.Contains(pf.Reason, tc.wantProviderReason) { + t.Fatalf("provider reason = %q, want it to contain %q "+ + "(a different expiresAt branch fired)", pf.Reason, tc.wantProviderReason) + } + if pf.State != provider.StateResolved && pf.Value != nil { + t.Fatalf("a non-resolved provider fact must drop its value; got %#v", pf.Value) + } + + // Layer 2 — the host outcome the engine actually consumes. + result := provider.ResolveFacts(context.Background(), callRepoFile(t, opts, q), q, tc.now) + fact, ok := result.Facts["max_partitions"] + if !ok { + t.Fatal("requested output omitted from the result — fail-open via absence") + } + if fact.State != tc.wantState { + t.Fatalf("host state = %q (reason %q), want %q", fact.State, fact.Reason, tc.wantState) + } + + if tc.wantState == provider.StateResolved { + if fact.Value != float64(6) { + t.Fatalf("value = %#v, want 6", fact.Value) + } + if fact.ExpiresAt == nil { + t.Fatal("a resolved fact must carry expiresAt — an unbounded fact never goes stale") + } + if !fact.ExpiresAt.Equal(tc.wantExpiry) { + t.Fatalf("expiresAt = %s, want %s", fact.ExpiresAt, tc.wantExpiry) + } + return + } + + // Undecidable or elapsed freshness must never leave a usable value behind. + if fact.Value != nil { + t.Fatalf("a non-resolved fact must drop its value; got %#v", fact.Value) + } + if tc.wantReason != "" && !strings.Contains(fact.Reason, tc.wantReason) { + t.Fatalf("host reason = %q, want it to contain %q", fact.Reason, tc.wantReason) + } + }) + } +} diff --git a/openspec/specs/p5-aud-audit-remediation/spec.md b/openspec/specs/p5-aud-audit-remediation/spec.md index 6f941d5..aff2695 100644 --- a/openspec/specs/p5-aud-audit-remediation/spec.md +++ b/openspec/specs/p5-aud-audit-remediation/spec.md @@ -657,9 +657,15 @@ Requirements: **so that** the D-010 gate stops sitting at exactly 90.0% (steering, not measuring) and the operator's "bump coverage a bit" lands as verified behavior, not filler. -**Goal**: (1) **TEST-02** — `toCEL` overflow semantics (`evaluate.go:191`): a `json.Number` that -fits neither int64 nor float64 falls back to its string form; pin what a predicate comparing it -does (errors → fail-safe, never a silent numeric coercion). (2) **TEST-05** — +**Goal**: (1) **TEST-02** — `toCEL` overflow semantics: a `json.Number` that fits neither int64 +nor float64 is **refused** — it binds a CEL error value; pin what a predicate over it does +(errors → fail-safe, never a silent coercion, and never the lexical string form). *Amended after +**D-131** / ADR-0013 Amendment 1 landed on `main`: as specified, this arm pinned the old +**string-fallback** contract, which the D-131 lane replaced precisely because the fallback was a +silent demotion to text. TEST-02 was written to red if that branch ever changed, and it did; the +requirement now pins the refusal with the same rigour (the branch really is reached, the refusal +names the number, and non-relational operators — `type()`, `int()`, `string()`, arithmetic — +cannot launder it).* (2) **TEST-05** — `reconcileClearSlot` (`internal/forge/forge.go:568-…`, 56.5%): table over its fail-closed branches (resolve failure, partial clear, already-clear idempotence) — each error branch both-polarity. (3) **TEST-06** — `repo_file` builtin (`internal/provider/builtin/repo_file.go`): path-containment @@ -673,9 +679,10 @@ floor stays 90% per judgment call (e) unless the operator raises it). **Dependencies**: sequence AFTER Lane A lands S10–S12 (shared `internal/forge` test dirs). Lane E. **Acceptance criteria (G-W-T)**: -- Given an over-range `json.Number`, when bound through `toCEL` into a comparison predicate, then - the evaluation errors → fail-safe (never APPROVE via string/number confusion); the string - fallback line's hit count is non-zero in the profile. +- Given an over-range `json.Number`, when bound through `toCEL` into any predicate — relational, + equality, explicit `int()`/`string()` coercion, or arithmetic — then the evaluation errors → + fail-safe (never APPROVE, and never the lexical string form); the refusing branch's hit count is + non-zero in the profile. - Given each `reconcileClearSlot` error branch, when driven by a fake forge, then the fail-closed outcome is asserted (and the happy idempotent clear too). - Given traversal/absolute/escape paths and expired/undeclared maxAge, when `repo_file` answers, @@ -689,7 +696,7 @@ production-code changes. **Not in scope**: TEST-04 (`cmd/assent` gate widening — fenced); raising the D-010 floor. Requirements: -- **REQ-AUD-S13-01** *(TEST-02)* — toCEL overflow string-fallback covered incl. predicate fail-safe polarity. Test: `internal/core/aggregate/evaluate_tocel_test.go` (new); Verify: `go test ./internal/core/aggregate/... -run TestToCELOverflowFailsSafe`; Level: L0 +- **REQ-AUD-S13-01** *(TEST-02)* — toCEL overflow refusal (D-131) covered incl. predicate fail-safe polarity, the non-exponent literal, and the non-relational operators. Test: `internal/core/aggregate/evaluate_tocel_test.go` (new); Verify: `go test ./internal/core/aggregate/... -run TestToCELOverflowFailsSafe`; Level: L0 - **REQ-AUD-S13-02** *(TEST-05)* — reconcileClearSlot branch table, error branches both-polarity. Test: `internal/forge/clearslot_test.go` (new); Verify: `go test ./internal/forge/... -run TestReconcileClearSlotBranches`; Level: L0 - **REQ-AUD-S13-03** *(TEST-06)* — repo_file containment + expiry tables (security-adjacent). Test: `internal/provider/builtin/repo_file_test.go` (extend); Verify: `go test ./internal/provider/builtin/... -run 'TestRepoFileContainment|TestRepoFileExpiry'`; Level: L0 - **REQ-AUD-S13-04** *(headroom)* — aggregate `./internal/...` coverage ≥ 91.0% via the D-010 recipe. Test: the coverage gate itself; Verify: `task coverage` (printed pct ≥ 91.0); Level: L1