diff --git a/CHANGELOG.md b/CHANGELOG.md index a6f0f5b..3de0048 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,9 @@ repository still gets a decision, never by following the link; no release carrie - build(deps): bump astral-sh/setup-uv from 6.1.0 to 9.0.0 - build(deps): bump github/codeql-action/upload-sarif from 4.37.4 to 4.37.6 - build(deps): bump github.com/santhosh-tekuri/jsonschema/v6 from 6.0.2 to 6.0.3 + +### Documentation +- :memo: docs(decisions): record D-126 (D-031 domain deferred) and D-127 (SEC-05 PAT rotation scheduled) ## [0.2.0] - 2026-08-09 ### Chores diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index cb9f7cf..244fc1f 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -130,6 +130,8 @@ 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-126 | 2026-08-08 | **D-031 domain string — DEFERRED (operator ruling).** Operator explicitly deferred picking the owned domain to replace `assent.dev` in apiVersion/`$id` fields across schemas, tests, and `API_STABILITY.md`; `assent.dev` stays in place for now. Does not block future releases — v0.1.0 (D-111) already shipped with this open, so it's a contract-honesty residual, not a release gate. Revert: n/a — revisit whenever the operator picks a domain. | +| D-127 | 2026-08-08 | **SEC-05 PAT rotation — SCHEDULED, not immediate (operator ruling).** Operator chose to schedule the `HOMEBREW_TAP_GITHUB_TOKEN` classic→fine-grained PAT rotation for later rather than doing it this session (requires a browser step in the GitHub UI). The broad-scope classic PAT remains live in the interim — accepted as a bounded, known risk. Revert: n/a — stays a standing operator task until done. | | 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. |