From a072933e82b4d912ff2c4c8ead6d4f5de2ae85c0 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 23 Aug 2026 17:48:05 +0200 Subject: [PATCH 1/3] :memo: docs(e10): GitHub addressing & representation model (E10-S00) Executes ADR-0021 items 5-8 unamended, on paper, before the port signature freezes. Answers all four REQ-E10-S00 questions and names the conformance case that will prove each. Q1 fork-head addressing (P0): the governed subject moves to MR-relative FileAtBase/FileAtHead; FileAtRef survives for ref-addressed decision inputs. Verified against the tree, not transcribed: exactly two call sites migrate (run.go:270,:274) and six must not - and three of ADR-0021's anchors have drifted, including the D-130 who-may-approve registry read (provider_host.go:292, not :275). Q2 capability predicates: two kinds - probed (endpoint + field + comparison) or contract-proven (a constant licensed by a NAMED conformance case; no case means unknown). GitLab probes 3 of 11 honestly, 1 by heuristic, hardcodes 1 and implicitly assumes 7. eligible-approval-evidence is reachable as `full` on GitHub via codeowners (improving on ADR-0021's prediction), while the aggregate route satisfies nothing. protected-pipeline-source is unknown on BOTH forges - GitLab's strings.Contains(ci_config_path, "@") is a heuristic, not a predicate - which is OQ-33. Q3 record surface (P0): ADR-0021 item 8 option (ii) confirmed. No schema change; git diff schemas/ == 0 holds. Comma-joining gaps into pins.capabilityGap is not a loophole (the allOf forbids the field whenever mergeResultDigest is pinned), so there is no gap-selection rule to invent. The audit-trail cost is pre-existing: aggregate.Result.CapabilityGaps already never reaches the record. Q4 status-to-sentinel: forge.ErrUnauthorized lifts to the port at S02; a GitHub 404 is absence only inside a repo the token can read; a 404 on protection/ruleset reads without permissions.admin is unknown, never absent. Extends the AUD2-S02/REL-03 discrimination into the adapter that mints the sentinel. Design story - no Go, no schema, no catalog row (S01 owns catalog.yaml). Refs: D-155, OQ-33, ADR-0021 items 5-8 --- CHANGELOG.md | 1 + docs/decisions/decisions.md | 1 + docs/planning/github-addressing-model.md | 430 +++++++++++++++++++++++ docs/planning/open-questions.md | 1 + openspec/specs/backlog.md | 2 +- 5 files changed, 434 insertions(+), 1 deletion(-) create mode 100644 docs/planning/github-addressing-model.md diff --git a/CHANGELOG.md b/CHANGELOG.md index d70c8ea..2c0cf42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,7 @@ repository still gets a decision, never by following the link; no release carrie - :memo: docs(openspec): amend AUD2-S03 to the shipped pin and log D-153 (SEC-03) - :memo: docs(audit): document the AUD2 gate and correct stale line counts (F4, F6) - :memo: docs(backlog): mark P5-AUD2 complete (S01-S05 landed) +- :memo: docs(e10): GitHub addressing & representation model (E10-S00) ### Features - :sparkles: feat(audit): measure REL-03/REL-07 by real mutation run, not by name diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index b1d7e71..4bcb4d8 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -158,3 +158,4 @@ project/process decisions. | D-151 | 2026-08-16 | **OpenSSF Best Practices project creation is RESOLVED: (a) — the operator will create the `bestpractices.dev` project for `PlatformRelay/assent`.** Unblocks SEC-SC-S02 (evidence page + README badge, once *passing*); SEC-SC-S01 (fuzzing) is not blocked by this and can proceed independently. Operator action only — requires a personal account at bestpractices.dev, not automatable in-tree. Current CII score is 0, which drags the OpenSSF Scorecard (~7); the repo already satisfies most criteria (pinned CodeQL, Dependabot, secret scanning, signed releases, SLSA provenance). Revert: leave CII at 0 indefinitely — SEC-SC-S01 is unaffected either way. | | D-152 | 2026-08-18 | **The 2026-08-18 audit's "Next (risk reduction)" wave is decomposed as its own epic P5-AUD2 (`openspec/specs/p5-aud2-audit-remediation/spec.md`), five stories, spec-first.** Context: `agent-context/PROJECT-AUDIT-2026-08-18.md` closed both P1 conditions the same day and v0.3.0 shipped, leaving a named but untracked wave — exec-transport trio (REL-01/02/07), REL-03 `ErrNotFound` discrimination, SEC-03 cosign identity pin, TEST-02 mutant. Options considered: **(a)** hand-fix them as loose commits (rejected — AGENTS.md rule 4 is spec-first, and REL-01 is now byte-identical across *three* audits precisely because nothing ever tracked it); **(b)** append them to the existing P5-AUD epic (rejected — that epic's exit gate is CLOSED and reopening it would make "AUD complete" a moving claim); **(c)** a new AUD2 epic keyed to this audit **(chosen)**. Three consequences recorded here so they are not re-litigated: **(1)** AUD2 has **no release-condition story** — the audit had exactly two P1s and both are already closed; **(2)** **WG-S01 is deliberately excluded** — it carries the LGTM governance marker, and GOVERNANCE says such stories are surfaced to the maintainer, which an autonomous loop's decide-and-log does not override; **(3)** AUD2's exit gate is a **`task check` stage**, not a `release-exitgate` step, because that job is `pull_request`-skipped (RELSE-08) and wiring the gate there is how AUD-S18's own stale `CHECK_STAGES` pin survived four merges. A 2026-08-10-keyed AUD2 draft exists only in the local stash `leave-aud2-not-this-epic`, was never committed, and is superseded; its still-open items (F3/F5/F7) stay Later-wave. Revert: delete the spec + backlog section; the four findings return to the audit report untracked. | | D-153 | 2026-08-19 | **The cosign signer-identity pin published in `SECURITY.md` was WRONG, not merely missing from `hack/install.sh`: the identity regexp is widened to `^https://github\.com/PlatformRelay/[Aa]ssent/` in both files (AUD2-S03 / SEC-03).** Substance first: `SECURITY.md`'s "Verify a tagged release" instructions pinned `--certificate-identity-regexp '^https://github.com/PlatformRelay/assent/'`, and an adopter who followed them on **v0.2.0 or v0.3.0 got a verification FAILURE on a genuine, correctly signed artifact** — the published recipe has been broken for every release since v0.2.0, and the natural reading of that failure is "this release was tampered with". Cause: the repository was renamed `PlatformRelay/assent` → `PlatformRelay/Assent` between v0.1.0 and v0.2.0; the keyless signing certificate's Subject Alternative Name carries GitHub's canonical casing; cosign compiles `--certificate-identity-regexp` as a Go RE2 pattern and matches it **case-sensitively**. Decoded from the published bundles: v0.3.0 and v0.2.0 sign as `https://github.com/PlatformRelay/Assent/.github/workflows/release.yaml@refs/tags/vX.Y.Z`, v0.1.0 as `https://github.com/PlatformRelay/assent/.github/workflows/release.yaml@refs/heads/main`. All three verify under the new value; independently re-confirmed with real cosign against the real v0.3.0 artifact (`Verified OK` under the new pin, exit 1 "expected SAN value to match regex" under the old). Options considered: **(a)** ship the pin byte-identical to the published-but-broken value and file the breakage as a follow-up (rejected — it would land a green gate asserting a guarantee that fails closed on this project's own releases, D-124's defect one level up); **(b)** case-insensitive `(?i)` (rejected — wider than the defect and it silently accepts casings GitHub never issues); **(c)** an explicit `[Aa]` class in both files, dots escaped (chosen). Not widened otherwise: the `^` anchor and the owner/repo scope stand, so another owner, an `assent-mirror` typosquat, another forge and an unescaped-dot host all still fail. Enforcement, per D-128: **one** published truth — `hack/release/install_cosign_pin_test.sh` extracts the pair from `hack/install.sh` and `SECURITY.md`, requires exactly one distinct value per file, reddens on drift, and (this is the assertion that would have caught the defect) matches the pin against the three **real** SANs above, committed as offline fixtures, plus six negatives. Extends D-110 (bundles beside archives) and D-109; the maintainer-path twin at `hack/release/verify-artifacts.sh:124` is still unpinned and is tracked in the backlog, not fixed here. Revert: restore `'^https://github.com/PlatformRelay/assent/'` in both files — which re-breaks verification of v0.2.0 and every later release, so revert only together with a repo rename back to lowercase. | +| D-155 | 2026-08-23 | **E10-S00 — the GitHub addressing & representation model is written ([docs/planning/github-addressing-model.md](../planning/github-addressing-model.md)) and ADR-0021 items 5–8 are executed UNAMENDED**, because nothing in the four answers contradicts them. **(1) Fork-head addressing (P0):** the governed subject moves to MR-relative `FileAtBase(mr, path)` / `FileAtHead(mr, path)`; `FileAtRef(project, path, targetRef)` survives for ref-addressed decision inputs. Verified against the tree rather than transcribed: **exactly two call sites migrate** (`cmd/assent/run.go:270`, `:274`) and **six must not** (`run.go:203/211/230/249`, `provider_host.go:82/292`) — and **three of ADR-0021's anchors have drifted**: the resource-owner registry read (D-130, "who may approve") is at `provider_host.go:292` not `:275`, `refFilePort` is at `:263` not `:246`, and the port method is `GetMR` not the proposed `Describe`. **(2) Capability predicates:** every dossier §4 flag gets one of two kinds — `P` probed (named endpoint + field + comparison) or `C` contract-proven (a constant licensed by a NAMED conformance case; no case ⇒ `unknown`) — which makes "we assume the API supports it" legal exactly once. Counted against the tree, GitLab probes 3 of 11 honestly, 1 by heuristic, hardcodes 1, and leaves **7 implicitly assumed**; `unknown ⇒ never arm` binds per capability at its consultation point (`internal/forge/precondition.go` consults 3 today), not "all eleven". **A finding that improves on ADR-0021's own prediction:** `eligible-approval-evidence` is reachable as `full` on GitHub via `codeowners` (already a first-class `VerifyingCapability` at `internal/core/aggregate/approval.go:126-133`) — base-ref CODEOWNERS + team expansion yields the named eligible-id set the engine requires — whereas the `aggregate` route (GraphQL `reviewDecision`) is **not a satisfying sub-value at all**, exactly like GitLab Free. **A finding that is worse than expected:** `protected-pipeline-source` is `unknown` on **both** forges — GitLab's `strings.Contains(ci_config_path, "@")` is a heuristic, not a predicate → **OQ-33**, and retiring it is a user-visible GitLab behaviour change owed its own D-row at S04/S09 (judgment call (e)), not minted here. **(3) Record surface (P0):** ADR-0021 item 8 option (ii) confirmed — the multi-capability report is `doctor` output and arming-refusal reasons only, never the DecisionRecord; **`git diff schemas/ == 0` holds and NO schema change is proposed.** Sharpened: comma-joining gaps into `pins.capabilityGap` is not a loophole (the `allOf` FORBIDS the field whenever `mergeResultDigest` is pinned, so the channel closes exactly when the merge succeeds), and there is therefore **no gap-selection rule to invent** — the field is reserved for one capability. The audit-trail cost is real but **pre-existing**: `aggregate.Result.CapabilityGaps` already never reaches the record, and `run.go:368` writes one hardcoded merge-result string. **(4) Status → sentinel:** `forge.ErrUnauthorized` lifts to the port at S02; a GitHub **404 is absence only when a CONTENT-SCOPE probe (a sibling `/contents` read at the same ref) returned 200** — deliberately NOT a repo-metadata probe, since a fine-grained PAT with `metadata: read` and no `contents: read` gets 200 on `GET /repos/{o}/{r}` and 404 on every file, which would re-open the P0 through the front door; a 404 on branch-protection/ruleset reads without `permissions.admin` is `unknown` (never `absent`), and rate-limited 403s are transport errors — extending the AUD2-S02/REL-03 discrimination one layer down into the adapter that mints the sentinel. Ten conformance case IDs are minted (each fail-closed case paired with a positive control, per E10-S12's rule applied early); `internal/forge/conformance/catalog.yaml` is S01's to edit, not this story's. ADR-0021 stays **Proposed** — accepting it is the maintainer's call, and E10-S00 is `[LGTM]`. | diff --git a/docs/planning/github-addressing-model.md b/docs/planning/github-addressing-model.md new file mode 100644 index 0000000..3ae2679 --- /dev/null +++ b/docs/planning/github-addressing-model.md @@ -0,0 +1,430 @@ +# GitHub addressing & representation model (E10-S00) + +> **Status**: written, awaiting maintainer LGTM (E10-S00 is `[autonomous · design · LGTM]`). +> **Governing ADR**: [ADR-0021](../adr/0021-multi-adapter-forge-seam.md) items 5–8 — this +> document *executes* them. Nothing here contradicts items 5–8, so ADR-0021 is not amended. +> **Inputs**: [forge-dossier-github.md](forge-dossier-github.md) (the spec input, not +> guesswork), [forge-dossier-gitlab.md](forge-dossier-gitlab.md), ADR-0015 §1/§2/§4/§8, +> ADR-0017 §1/§3, ADR-0020, GUIDELINES "Safety invariants". +> **Raises**: [OQ-33](open-questions.md) — `protected-pipeline-source` has no decidable +> predicate on either forge today. + +This is E10 story zero: four questions answered on paper before the port signature freezes. +Each answer names the conformance case that will prove it. Case IDs are *minted here and +implemented in S01/S07/S14* — `internal/` is not this story's to touch, so no row is added to +`internal/forge/conformance/catalog.yaml` by this change. + +**Evidence classes** used throughout, so a reader can tell a fact from a plan: + +| Tag | Meaning | +| --- | --- | +| `code` | read from this tree at the cited line, this change's HEAD | +| `dossier` | cited endpoint/field in `forge-dossier-github.md` (docs snapshot 2026-07-21) | +| `unverified` | plausible but not confirmed against a live forge — resolves to `unknown` | + +--- + +## Q1 — How does the port name the head content of a fork PR? + +**Decision: confirm ADR-0021 item 5.** The governed subject is addressed **relative to the +merge request**, never by `(project, branch-name)`. `forge.RunPort` gains + +``` +FileAtBase(mr, path string) ([]byte, error) +FileAtHead(mr, path string) ([]byte, error) +``` + +and `FileAtRef(project, path, ref string)` **survives unchanged** for ref-addressed decision +inputs. The two accessors are not alternatives: item 5 decides which is legal where, and the +split *is* the trust boundary of GUIDELINES Safety 3 / ADR-0015 §1. + +### The concrete failure this prevents + +`code`: `cmd/assent/run.go:270` reads the governed base at `info.TargetBranch` and `:274` +reads the governed head at `info.SourceBranch`, both through `fileAtRefOrAbsent` +(`run.go:465`, call at `:466`), which maps `forge.ErrNotFound` to `nil` bytes. `forge.MRInfo` +(`internal/forge/port.go:40`) carries `SourceBranch` but **no source-repository identifier**. +On GitHub a fork PR's head branch does not exist in the base repository, so that read 404s → +`nil` → `change.OneSidedLifecycle(base, nil)` → `(KindDelete, true)` → **a fabricated +whole-file DELETE the contributor never made**: a spurious BLOCK, or an APPROVE reached on +invented change semantics. + +### Verified call-site inventory (this is the load-bearing part) + +ADR-0021 warns "verify against the tree before relying on either list", and the check was +worth running — two of its line anchors have drifted. As of this change's HEAD: + +| Call site | Reads | Ref | Disposition | +| --- | --- | --- | --- | +| `run.go:270` | governed subject, base side | `info.TargetBranch` | **migrates → `FileAtBase(mr, path)`** | +| `run.go:274` | governed subject, head side | `info.SourceBranch` | **migrates → `FileAtHead(mr, path)`** | +| `run.go:203` | MergePolicy | `info.TargetBranch` | stays `FileAtRef` | +| `run.go:211` | RulesetBinding | `info.TargetBranch` | stays `FileAtRef` | +| `run.go:230` | `.assent/config.yaml` (provider-host declarations) | `info.TargetBranch` | stays `FileAtRef` | +| `run.go:249` | policy pack | `info.TargetBranch` | stays `FileAtRef` | +| `provider_host.go:82` | provider host declaration | `targetRef` | stays `FileAtRef` | +| `provider_host.go:292` | **resource-owner registry** | `targetRef` | stays `FileAtRef` — **D-130** | + +Exactly **two** call sites migrate; **six** must not. Corrections to ADR-0021's anchors: +the resource-owner registry read is at `provider_host.go:292` (ADR-0021 cites `:275`, which +now falls inside that function's doc comment), and `refFilePort` is declared at +`provider_host.go:263` (ADR-0021 cites `:246`). `forgePort` at `run.go:64` is correct. A +third drift for S02 to absorb: ADR-0021 item 1 proposes `Describe(project, mr)`; the tree's +method is `GetMR(project, mr)` (`run.go:68`). + +`run.go:230` and `provider_host.go:292` deserve the specific mention ADR-0021 gives them: +the first carries provider-host declarations, so migrating it would let a fork's head +redefine its own fact semantics; the second **decides who may approve**, and moving it onto +an MR-relative accessor re-opens D-130 verbatim. Migrating any of the six is a trust-boundary +regression, not a cleanup. + +Out of scope for the migration, noted so S02 does not trip over it: when `--checkout` is set, +`run.go:282-287` overrides both sides from the local tree (EFE-S03 / ADR-0008 §4). That path +never touches the forge and is unaffected. + +### How each adapter reaches the head (adapter-internal freedom) + +Both forges expose an **MR-relative head ref inside the target project**, which is why this +shape is neutral rather than GitHub-shaped: + +- **GitHub** — `GET /repos/{o}/{r}/contents/{path}?ref={pr.head.sha}`; the fork's head commit + is reachable in the base repo as `refs/pull/{n}/head` (`dossier` C13). Addressing by the + head **SHA** rather than the ref name is preferred: it is the same value pinned into + `pins.sourceSha`, so the record and the read cannot disagree. +- **GitLab** — the source project id from the MR object, or the target project's + `refs/merge-requests/{iid}/head`; either way the adapter already holds the MR object. + +Two implementations are explicitly forbidden, because both re-mint the defect: +`FileAtBase`/`FileAtHead` delegating to `FileAtRef(project, path, info.SourceBranch)`, and +smuggling `refs/pull/N/head` into `MRInfo.SourceBranch` (it corrupts a documented field and +leaks into rendering — ADR-0021 item 5). + +### Conformance cases + +| Case id | Test name | Proves | +| --- | --- | --- | +| `fork-head-unchanged-file-no-lifecycle` | `TestConformanceForkHeadUnchangedFileNoLifecycle` | a fork MR whose governed file is **unchanged** yields *no* lifecycle event — the fixture's head branch name must not exist in the base repo, or the case cannot fail | +| `fork-head-genuine-delete-detected` | `TestConformanceForkHeadGenuineDeleteDetected` | **positive control**: a fork MR that really deletes the governed file still mints `KindDelete` | + +The positive control is not optional. Without it, "never mint a DELETE from a fork" is +satisfiable by never minting a DELETE at all — the unfailable-assertion shape this repo's +review gate finds more often than any other defect, and the same pairing E10-S12 already +mandates for capability gaps. + +--- + +## Q2 — What operationally decidable predicate makes each capability `supported`, per forge? + +**Decision.** Every one of the dossier §4 flags gets a predicate of exactly one of two kinds. +Anything that is neither is `unknown`, and `unknown` is treated as `absent` for arming +(ADR-0021 §3). + +- **`P` — probed.** A named read-only endpoint, a named response field, and a comparison, + evaluated per run. A read that fails for permission reasons yields `unknown`, never + `absent` (see Q4 — this is where Q4's mapping earns its keep). +- **`C` — contract-proven.** The adapter returns a constant, and that constant is licensed by + a **named passing conformance case** for that adapter. No case, no constant: the flag is + `unknown`. This is the rule that makes "we assume the API supports it" legal exactly once — + when someone wrote the case — and it is what today's adapter is missing (below). + +**Scope of `unknown ⇒ never arm`.** It binds *per capability, at its consultation point*, not +"all eleven must be `supported`". `code`: `internal/forge/precondition.go:47-82` consults +exactly three today — `protected-pipeline-source`, `threads-block-merge` and +`eligible-approval-evidence`. S04 must state the arming set explicitly; each capability +promoted into it that is currently assumed-true becomes an honest gap and owes a decision row +(epic judgment call (e)). + +### The eleven flags + +| # | Capability | GitLab predicate | GitHub predicate | Kind | +| --- | --- | --- | --- | --- | +| 1 | `resolvable-threads` | constant `supported`, licensed by the existing `p3e5-*` reconciliation cases | constant `supported`, licensed by a GraphQL case (`reviewThreads.isResolved` / `resolveReviewThread`, `dossier` C1/C2) | `C` | +| 2 | `threads-block-merge` | `GET /projects/{id}` → `only_allow_merge_if_all_discussions_are_resolved == true` (`code`: `snapshot.go:290`) | `GET /repos/{o}/{r}/branches/{base}/protection` → `required_conversation_resolution.enabled == true`, **or** the equivalent ruleset row from `GET /repos/{o}/{r}/rules/branches/{base}`; both must be consulted (endpoints `dossier` C3 + §2 step (a); **field name `unverified`**) | `P` | +| 3 | `blocking-review` | `absent` — GitLab has no `REQUEST_CHANGES` primitive; ADR-0017 §3 uses threads (`dossier` C4) | `required_pull_request_reviews.required_approving_review_count >= 1` on the base branch; without required reviews a `REQUEST_CHANGES` review does not block (`dossier` C4) | `P` | +| 4 | `review-dismissal-restrictions` | `absent` — no analogue | `required_pull_request_reviews.dismissal_restrictions` non-empty **and** `users[].id ∪ expand(teams[]) ∌ pr.user.id`; team expansion via `GET /orgs/{org}/teams/{slug}/members` (`dossier` C8′) | `P` | +| 5 | `sha-guarded-merge` | constant `supported`, licensed by `sha-guard-source-moved` / `sha-guard-target-advanced` (already in `catalog.yaml`) | constant `supported`, licensed by a GitHub-factory run of the same two cases — `PUT /pulls/{n}/merge` with `sha`, 409 on mismatch (`dossier` C10) | `C` | +| 6 | `deferred-merge-arming` | constant `supported` (MWPS is tier-independent, `dossier` C11) | `GET /repos/{o}/{r}` → `allow_auto_merge == true`; `enablePullRequestAutoMerge` fails without the repo setting (setting `dossier` C11; **field name `unverified`**) | `C` / `P` | +| 7 | `arming-revoked-on-push` | constant `supported` — any new commit cancels MWPS (`dossier` C11) | `supported` **only** when the substitute is configured — stale-approval dismissal **and** at least one required status check; a write-access push does **not** auto-disarm GitHub auto-merge (`dossier` §3 delta 2). Otherwise `absent`. (**Field names `unverified`** — C19 names the setting, not the API field) | `P` | +| 8 | `merge-result-pinning` | `GET /projects/{id}` → `merge_trains_enabled == true` (`code`: `snapshot.go:291`) | base-branch ruleset from `GET /repos/{o}/{r}/rules/branches/{base}` contains the merge-queue rule (endpoint `dossier` C14; **rule-type string `unverified`**) | `P` | +| 9 | `eligible-approval-evidence{full\|aggregate}` | `full` iff `GET /projects/{id}/merge_requests/{iid}/approval_rules` returns rules with `eligible_approvers[]` (`code`: `gitlab/snapshot.go` `hasApprovalRulesAPI`); else `absent` | `full` iff `required_pull_request_reviews.require_code_owner_reviews == true` **and** CODEOWNERS is readable **at the base ref** **and** every referenced team expands (`GET /orgs/{org}/teams/{slug}/members` → 200). Team expansion 403 → `unknown`. See below | `P` | +| 10 | `approval-reset-on-push{default\|opt-in}` | `GET /projects/{id}/approvals` → `reset_approvals_on_push == true` — **currently unprobed** (audit RELI-03; **`unverified`** — the field is named by RELI-03, not read from this tree) | stale-approval dismissal enabled on the base branch; the stronger variant is "require approval of the most recent reviewable push" (`dossier` C19/C8; **field names `unverified`**) | `P` | +| 11 | `protected-pipeline-source` | **no predicate today** — see below | **no predicate today** — see below | — | + +**On the `unverified` tags.** The dossier is an *endpoint* study: for several rows it cites the +GitHub **UI setting** and the endpoint that carries it, but not the JSON field name. Those +field names are marked `unverified` rather than dressed up as `dossier`, and under this +document's own rule they resolve to **`unknown`** — hence non-arming — until S09 confirms each +against a live response. That is the fail-closed direction and it costs nothing: a cell that +turns out to be right is promoted at S09; a cell that turns out to be wrong never armed +anything. Rows 3, 4 and 9's GitHub field names are genuinely enumerated in dossier §2 and are +not tagged. + +### Row 9 is the one that decides whether GitHub can ever gate — and it is decidable + +The dossier's "no API returns the computed per-PR eligible code owners" reads like a dead end. +It is not, because assent never needed the *forge* to compute the set — it needs a +**forge-proven named principal set**, and `code` shows the engine already accepts two ways to +get one: `internal/core/aggregate/approval.go:126-133` admits +`VerifyingCapability ∈ {"approval-rules-api", "codeowners"}` and fails closed on anything +else, and `approvalSatisfies` counts an approver only if its id is in `ev.Eligibility`. + +So: + +- **`aggregate` is not a satisfying sub-value.** GraphQL `reviewDecision: APPROVED` proves the + forge enforced *its* rule but names no principal; fed to the engine it is + `VerifyingCapability: "none"` → a recorded capability gap that **never satisfies** + (`approval.go:130`). Recording `aggregate` as `supported` would be exactly the + paper-over REQ-E10-S00-02 forbids. +- **`full` is reachable on GitHub via `codeowners`**, which is already a first-class verifying + capability: CODEOWNERS read **from the base ref** (fork-safe by GitHub's own definition, + `dossier` §2 step (b)), matched client-side, teams expanded to ids, owners filtered to + write-holders. The predicate above is what makes that honest rather than optimistic — and + it is symmetric with GitLab Free, which is `absent` for the same reason (no named eligible + set) and already refuses to arm (`precondition.go:70-78`). + +This **refines**, and does not contradict, ADR-0021's Consequences prediction that +`eligible-approval-evidence` would plausibly be `unknown` on GitHub forever. The prediction was +about the *aggregate* route; the `codeowners` route was already in the engine. It also needs no +schema change: `code`, +`schemas/approval/v1alpha1/approval-evidence.schema.json:40-42` enumerates +`verifyingCapability` as exactly `["approval-rules-api", "codeowners", "none"]` — the value is +already frozen-legal. + +**The asymmetry a reviewer will find, owned here rather than left to be discovered.** Two +things are true and both should be said. First, **no adapter produces `"codeowners"` today** — +`code`: the only writer is `internal/forge/gitlab/resolve.go:85`, which emits +`"approval-rules-api"`; `"codeowners"` appears in the tree solely as an accepted *input* value +(`approval.go:41`, `:127`) and in the frozen enum. GitHub would be its first producer. Second, +GitLab's `eligible_approvers[]` is **forge-computed** while GitHub's CODEOWNERS set is +**adapter-computed from forge-supplied data**, and ADR-0017 §3 says *forge-proven*. The +defence is that the frozen contract already ruled on this: it admits `codeowners` as a real +verifying capability, distinct from `none`, precisely because a CODEOWNERS file served by the +forge **at the base ref** is forge-supplied evidence — what the adapter contributes is +pattern-matching, not authority. The trust-boundary half is what makes it hold, and it is why +the predicate says *base ref*: a head-ref CODEOWNERS would let an author name themselves owner, +which is D-130's shape on a different file. If the maintainer reads ADR-0017 §3 more strictly +than the schema does, the consequence is stated and survivable — GitHub falls to `absent` for +require-review, exactly like GitLab Free, and gates nothing rather than gating wrongly. + +### Row 11 is the honest failure, and it is worse than "GitHub lacks it" + +`code`: `internal/forge/gitlab/snapshot.go:292` computes GitLab's arming prerequisite as + +```go +caps.ProtectedPipelineExternal = strings.Contains(proj.CIConfigPath, "@") +``` + +A substring test for `@` is a *heuristic*, not a predicate: it shows the CI config names +another project, and proves neither that the referenced file sits on a protected branch nor +that the MR author cannot push to it. That is the SEC-04 shape ADR-0021 forbids by name — and +it is currently the single load-bearing arming prerequisite (`precondition.go:54-62`, +`RefusalInsecureTopology`). **Under this model's rules, `protected-pipeline-source` is +`unknown` on GitLab today**, not `supported`. + +A real GitLab predicate exists but costs several reads on a second project (`ci_config_path` +resolves to project *X* → `GET /projects/X/protected_branches` covers the file's branch → +the MR author lacks push access there), and degrades to `unknown` whenever the token is not +scoped to *X*. + +On GitHub there is no single readable analogue at all. The plausible predicate is a +**composite of environment and forge** (`unverified`): the run's trigger event is one whose +workflow definition GitHub loads from the base repository's default branch +(`pull_request_target` / `workflow_run` / `merge_group`), **and** that default branch requires +reviews, so the definition is not author-editable. Under `pull_request` from a fork the token +is read-only and secretless (`dossier` C17) — safe, and advisory-only per ADR-0015 §8, which +is a *non-arming* state, not a gap. + +Consequences, stated rather than discovered: + +1. Under `unknown ⇒ never arm`, **v1 GitHub comments but does not arm** unless OQ-33 is + answered. ADR-0021 named this as a plausible shipped outcome; this document confirms it. +2. Retiring the `@` heuristic makes **GitLab** arming paths that pass today stop passing. + That is the intended surfacing (judgment call (e)) and it owes its own decision row at + S04/S09 — **not** here, because S00 changes no behaviour. +3. The composite predicate spans `cmd/assent`'s CI-env adapter and `internal/forge`, which + ADR-0015 §4 did not anticipate. That is why it is a question, not a decision → **OQ-33**. + +### What GitLab actually probes today (judgment call (e), made concrete) + +`code`: `forge.CapabilityFlags` (`internal/forge/snapshot.go:83-92`) is five booleans plus a +tier. Mapped onto the eleven: + +| Probed honestly (3) | Heuristic (1) | Hardcoded `true` (1) | Unprobed / implicitly assumed (7) | +| --- | --- | --- | --- | +| 2, 8, 9 | 11 | `MergeResultDigestRecordable` (the *record-only* axis of 8, `snapshot.go:268`) | 1, 3, 4, 5, 6, 7, 10 | + +Seven flags become explicit `unknown` at S04 unless a conformance case licenses a `C` +constant. Rows 1 and 5 already have licensing cases; rows 3 and 4 are honestly `absent` on +GitLab; rows 6, 7 and 10 need either a case or a probe. This is the "surfacing is the point" +consequence ADR-0021 promised, with a count attached. + +### Conformance cases + +| Case id | Test name | Proves | +| --- | --- | --- | +| `capability-report-exhaustive` | `TestConformanceCapabilityReportExhaustive` | every enum member has an entry; a new member without one fails to compile or fails the case | +| `capability-unknown-never-arms` | `TestConformanceUnknownCapabilityNeverArms` | `unknown` at a consultation point refuses arming, `merges == 0` | +| `capability-supported-does-arm` | `TestConformanceSupportedCapabilityArms` | **positive control**, `merges == 1` — without it the previous case is vacuous (E10-S12's rule, applied one story early) | + +--- + +## Q3 — Where does an eleven-capability report live in the record? + +**Decision: confirm ADR-0021 item 8, option (ii)** — the multi-capability report is `doctor` +output and arming-refusal reasons only, **never the DecisionRecord**. `git diff schemas/ == 0` +holds; **no schema change is required or proposed by this story.** + +### The schema, read rather than quoted + +`code`: `schemas/decision/v1alpha1/decision-record.schema.json` `$defs.pins` (lines 65-101) is +`additionalProperties: false`; `capabilityGap` is `{"type": "string", "minLength": 1}` — no +enum, no pattern — and the `allOf` at 94-100 requires it iff `mergeResultDigest` is `null` and +**forbids it otherwise**. The top-level object is `additionalProperties: true`. + +Three consequences follow directly, and the second is the one a reviewer will reach for: + +1. `capabilityGap` models **one** capability, merge-result pinning. (A prior draft's claim that + it "already models the absent-capability case" was **False** and stays retired.) +2. **Comma-joining eleven gaps into that string is not a loophole.** It passes validation, but + it is a semantic lie against the field's own description, and it is *unavailable in the + common case*: when `mergeResultDigest` **is** pinned, the field is forbidden outright. A + channel that closes precisely when the merge succeeds is not an audit trail. +3. **There is therefore no gap-selection problem to solve.** The field is not a general gap + channel that eleven candidates compete for; it is reserved for one capability, and the + remaining ten have a different home. No priority order is needed, which is the desirable + answer under AGENTS.md rule 7 — a total, deterministic selection over eleven contenders + would have been one more thing to get wrong. + +Recording the report in the open top-level object is **rejected**, per item 8: a safety-bearing +field that no schema validates and no consumer must read is a fail-closed guarantee in name +only. + +### The audit-trail cost, stated plainly — and already visible + +The cost is real: *a capability gap that blocks a merge leaves no trace in the DecisionRecord +beyond the existing single `capabilityGap` string.* It is also **pre-existing, not introduced +by E10**, and the tree shows exactly where: + +- `code`: `internal/core/aggregate/aggregate.go:193` already computes + `Result.CapabilityGaps map[string]string` (per governed subject, + `capabilityGapNone = "approval-capability-none"`), and its own comment says S10 would thread + it into `pins.capabilityGap`. It never did — `cmd/assent` reads `CapabilityGaps` in + `doctor.go`/`doctor_forge.go` only, never in the record path. +- `code`: `cmd/assent/run.go:368` sets the record's `capabilityGap` to one **hardcoded + merge-result string**, unconditionally. + +So an approval-capability gap already fails closed and already leaves no record trace. E10 +does not create that hole; it inherits it, bounds it, and names where it lives. Revisiting +option (i) — widening `pins` — is a `v1alpha2` conversation, unchanged. + +### Where the report does live + +`forge.CapabilityReport` (S04) → `doctor`'s typed report (ADR-0017 §9, the existing +`PreconditionProbe.CapabilityGaps` / `Refusals` shape in `internal/forge/precondition.go`) and +the arming-refusal reason surfaced to the operator. Both are unfrozen internal surfaces. + +### Conformance case + +| Case id | Test name | Proves | +| --- | --- | --- | +| `record-schema-unchanged-under-multi-gap` | `TestConformanceMultiGapRecordStillValidates` | a run with several `absent`/`unknown` capabilities still emits a record that validates against the frozen schema, and the doctor report carries all of them | + +--- + +## Q4 — What is each adapter's HTTP-status → port-sentinel mapping? + +**Decision: confirm ADR-0021 item 6, with the disambiguation made concrete.** `ErrNotFound` +means *absent*, never *forbidden*. `forge.ErrUnauthorized` is lifted to the port at S02 +(`code`: it exists today only as `internal/forge/gitlab/gitlab.go:238`, adapter-private, while +`forge.ErrNotFound` is already neutral at `internal/forge/port.go:34`). + +The precedent to stay consistent with is **AUD2-S02 / REL-03**: `code`: +`cmd/assent/provider_host.go:84` and `:276-283` — the fallback is gated on +`errors.Is(err, forge.ErrNotFound)` **alone**, so a 503, a throttle or a token scoped away +from the repo can no longer masquerade as an absent file. This story extends the same +discrimination one layer down, into the adapter that mints the sentinel. + +### GitLab (verified against the tree) + +| Status | Sentinel | Note | +| --- | --- | --- | +| 200 | content | `code`: `gitlab.go:486` region | +| 401 / 403 | `ErrUnauthorized` | `code`: `gitlab.go:238`, `:792` | +| 404 | `forge.ErrNotFound` (**absent**) | safe: GitLab distinguishes 403 from 404 for files | +| 429 / 5xx | transport error | retry/backoff per AUD-S11; never a sentinel | +| other | error | never absence | + +### GitHub (the trap) + +GitHub returns **404 for permission-denied resources** to avoid leaking existence, so *a bare +404 is not evidence of absence*. The adapter must disambiguate before it may return +`ErrNotFound`: + +| Status | Context | Sentinel | +| --- | --- | --- | +| 404 on `/contents/{path}` | the **content-scope probe** below returned 200 for this token at this ref | `forge.ErrNotFound` — **absent** | +| 404 on `/contents/{path}` | the content-scope probe did **not** return 200 | `ErrUnauthorized`. **Never** `ErrNotFound` | +| 404 / 403 on `GET /repos/{o}/{r}` | — | `ErrUnauthorized`. No read below it may claim absence | +| 404 on `/branches/{b}/protection` or `/rules/**` | token lacks admin read (`GET /repos/{o}/{r}` → `permissions.admin != true`) | **not a sentinel** — the capability probe yields `unknown` (Q2). GitHub 404s an unprotected branch *and* an unauthorized read with the same status | +| 403 with `x-ratelimit-remaining: 0` or `Retry-After` | primary/secondary rate limit | transport error, retryable; never a sentinel (`unverified` on exact header set) | +| 403 otherwise | — | `ErrUnauthorized` | +| 401 | — | `ErrUnauthorized` | +| 409 on `PUT /pulls/{n}/merge` | `sha` mismatch | typed SHA-guard precondition failure (`dossier` C10), not absence | +| 405 on `PUT /pulls/{n}/merge` | not mergeable | typed precondition failure, not absence | +| 422 | validation | error, never absence | +| 429 / 5xx | — | transport error | + +**The content-scope probe, and why it is not a repo-metadata probe.** The obvious +disambiguation — "`GET /repos/{o}/{r}` returned 200, so the token can read this repo, so a +content 404 is path absence" — is **wrong for a token shape E10-S06 explicitly supports**. A +fine-grained PAT with `metadata: read` and **without** `contents: read` gets 200 on the repo +object and 404 on every content read: under that rule, *forbidden* renders as *absent* and the +P0 this question exists to close re-opens through the front door. + +The probe must therefore exercise the **same permission as the read it is licensing**: a +sibling content read in the same repo at the same ref — the root listing, +`GET /repos/{o}/{r}/contents?ref={sha}` — returning 200 proves `contents: read` is granted +*there*, and only then is a 404 on the specific path genuine absence. The probe is cacheable +per `(repo, ref)` for the run, so it costs one request, not one per governed read. It also +fails in the right direction on an unresolvable ref: a bad `ref` 404s the root listing too, and +"the ref does not exist" is correctly not path absence. (`unverified` — the exact Contents API +behaviour for the root-listing form is an E10-S06 live check; until confirmed, the residual +rule below applies and the adapter errors.) + +**Residual rule (ADR-0021 item 6, restated as the fallback):** for any endpoint where the +adapter cannot distinguish absent from forbidden, it returns an **error**, not absence. The +content-scope probe is what converts "cannot distinguish" into "can" for governed-file reads, +and the `permissions.admin` probe does the same for the protection/ruleset reads; anything not +covered by a row in this table takes the residual rule. + +### Conformance cases + +| Case id | Test name | Proves | +| --- | --- | --- | +| `forbidden-never-renders-as-absent` | `TestConformanceForbiddenNotAbsent` | a governed-file read the forge refuses on permission grounds aborts the run with **zero forge writes** — never `nil` content, never a lifecycle event | +| `absent-file-still-renders-as-absent` | `TestConformanceAbsentFileIsAbsent` | **positive control**: a genuine 404 inside a readable repo still yields `forge.ErrNotFound`, so a real whole-file ADD/DELETE is still detected (EFE-S03 preserved). Without this, "always error" would pass the case above | +| `ratelimit-403-is-transport-error` | `TestConformanceRateLimit403NotAbsent` | a rate-limited 403 is retried/errored, never mapped to a sentinel | +| `metadata-only-token-is-not-absence` | `TestConformanceMetadataOnlyTokenNotAbsent` | the fixture answers `GET /repos/{o}/{r}` **200** and every `/contents/**` read **404** (the fine-grained-PAT shape). The run must abort, not mint a lifecycle event — this is the case that refuses the repo-metadata probe | + +--- + +## Forward obligations this document creates + +| # | Owed by | Obligation | +| --- | --- | --- | +| 1 | S01 | the ten case ids above appear in `catalog.yaml` with an adapter disposition (S01-03's strict-decode adapter list) | +| 2 | S02 | two call sites migrate, six do not; `forge.ErrUnauthorized` lifted to the port; ADR-0021's stale anchors (`provider_host.go:275→:292`, `:246→:263`, `Describe→GetMR`) absorbed | +| 2b | S02 | **resolve `FileAtBase(mr, path)`'s missing project binding before freezing the signature.** `FileAtRef` takes `project` as a parameter and today's client binds none, so ADR-0021's two-argument shape is under-specified: either the client binds the *target* project at construction (and the accessor is genuinely MR-relative) or `mr` becomes composite. Flagged here because a signature that freezes wrong is precisely what S00 exists to prevent; the choice is S02's, the ambiguity is not S02's to discover | +| 3 | S04 | the arming-relevant capability subset is stated explicitly; every `C` constant names its licensing case | +| 4 | S04 / S09 | retiring the `@` heuristic (and any other capability promoted into the arming set) is a **user-visible GitLab behaviour change** → its own `D-nnn` row + changelog entry, per epic judgment call (e). Not minted by S00, which changes no behaviour | +| 5 | operator | **OQ-33** — `protected-pipeline-source` predicate; until answered, v1 GitHub does not arm | + +## Claims deliberately not resurrected + +Both were found **False** by the 2026-08-10 adversarial review and are recorded here so a +later reader does not re-derive them: + +- Extracting the conformance suite does **not** unblock `catalog.yaml`'s `github-deferred` + rows — both are `level: L3, package: test/e2e`, gated on live GitHub infrastructure (S18). +- `capabilityGap` does **not** already model the absent-capability case generally; it models + merge-result pinning, which is why it is singular and coupled to `mergeResultDigest`. diff --git a/docs/planning/open-questions.md b/docs/planning/open-questions.md index b0f47f1..4d63485 100644 --- a/docs/planning/open-questions.md +++ b/docs/planning/open-questions.md @@ -34,3 +34,4 @@ | OQ-30 | **RESOLVED (D-148, 2026-08-16): (b) — keep the guard skipped on `pull_request`; the real mechanism (merge-direction-dependent ordering hazard) is now recorded in D-125/D-136.** Analysis retained below for the record. Original text: Is a `pull_request`-scoped CHANGELOG drift gate viable now that D-136 skips merge commits? The guard is retained with NO demonstrated reason — its original one is dead and its proposed successor measures false.** D-125 skipped the gate on `pull_request` because `refs/pull/N/merge`'s synthetic merge subject rendered into the generated changelog, so no committed `CHANGELOG.md` could match. **D-136 killed that reason** — that commit is a merge commit and is now skipped. The successor reason drafted in D-136's first version — "the merge ref also carries every commit landed on `main` since the branch forked, so the render is a union the branch's file cannot match, red by construction" — was then measured four ways and **could not be made true**: (1) PR #41's **live** `refs/pull/41/merge` (`491bb2a`, head `49eebb3` into base `7513d79`) rendered with the new `cliff.toml` → `verify-changelog: ok`, **0 diff lines**; (2) the direct counterexample — the same head merged into a `main` that **had** moved (`1d8aa60`, containing PR #40) → `verify-changelog: ok`, **0 diff lines**, i.e. not red with the base moved; (3) a synthetic sandbox where base and lane each add a commit to the **same** cliff group and each regenerate → `CONFLICT (content): Merge conflict in CHANGELOG.md`, so the PR is unmergeable, GitHub mints no merge ref, and the gate never runs. (4) **The strongest one, taken last and re-run rather than transcribed:** GitHub RE-MINTED `refs/pull/41/merge` against the moved base after all of the above. Re-fetched live — `7715bf7`, head `ee5e527` into base `1d8aa60` — and put through the real gate script: **`verify-changelog: ok`, 0 diff lines, 0 merge subjects rendered.** That is not a simulation: it is the exact artifact a `pull_request`-scoped gate would evaluate, with the base moved past the fork point AND after the lane had merged `main` in — the direction the finding below shows is hazardous — and it is green. Measurement (1)'s `491bb2a` at base `7513d79` is its stale predecessor, kept only to show the result did not depend on the base standing still. **Mechanism the dead premise overlooked:** the merge ref's `CHANGELOG.md` is not "the branch's committed file" — it is the three-way MERGE RESULT, which already contains the base's lines, because the file is merged like any other. So base movement ends in clean-and-matching or conflict-and-no-merge-ref. **The third outcome EXISTS, and merge DIRECTION decides it — measured while writing this row.** A clean textual auto-merge whose line order differs from git-cliff's topological order is red with no author error, and it reproduced immediately: merging `origin/main` **into the lane** (lane as first parent) auto-merged `CHANGELOG.md` without conflict and then failed `verify-changelog` on pure ordering — one `docs(compare)` line moved and PR #40's lines landed in a different position. The SAME two commits merged in the **merge-ref direction** (base `1d8aa60` as first parent, measurement (2) above) matched exactly. git-cliff's traversal follows parent order, so first-parent choice changes the render. This does not revive the retired premise — GitHub always mints the merge ref base-first, which is the direction that matched — but it means the clean-and-matching outcome is a property of that direction, measured on two merges, not a proof. It also re-confirms D-125's surviving rule: regenerate after any `git merge origin/main`. **Still untested:** behaviour on `pull_request_target`, on a PR from a fork, and after a force-push that re-mints the merge ref. **Counter-evidence for enabling it:** the only red reproduced on any merge ref was a branch that had not run `task changelog-write` for its own commits — a **true positive the gate exists to catch**, which argues the PR placement may now be correct rather than merely harmless. **Correction, folded in from the PR #41 review because it belongs in the row and not only in a review thread:** that review first read these greens as "the evidence points toward the PR gate being viable", and then took it back as **one measurement short**. The direction finding above supplies a false-positive mechanism it had not considered — a clean textual auto-merge whose line order differs from git-cliff's topological order reds with no author error and no author fix available. Four green measurements are therefore NOT a green light; on today's evidence the gate would not be enabled. **Ruling needed (deliberately not taken here, operator's call):** (a) enable the step on `pull_request` and delete the guard; (b) keep the guard and record the real reason once someone finds one; or (c) keep the guard permanently on cost/noise grounds and say so, rather than on a mechanism. Not to be resolved by deleting the guard on the strength of these three measurements alone — they show the claimed failure did not reproduce, not that no failure exists. | Before any change to the `pull_request` guard on the changelog step in `.github/workflows/verify.yaml`; not a release blocker — the guard is fail-safe (the gate runs locally in `task check` and on push-to-main) | Raised by the PR #41 review (finding CL-02) against D-136's first draft; measurements reproduced independently before recording. Sites now pointing here: `Taskfile.yml` `check:`, `.github/workflows/verify.yaml`, `hack/release/README.md`, `hack/release/changelog_gate_test.sh` §3. See [D-125](../decisions/decisions.md) and [D-136](../decisions/decisions.md) | | OQ-31 | **RESOLVED (D-146, 2026-08-16): (a) — "zero forge writes" stays absolute; the BLOCK is surfaced via a required CI job status reading the already-emitted `DecisionRecord`, not via a forge write.** Analysis retained below for the record. Original text: May the GUARD-1 self-edit BLOCK path write a summary or supersession note, or is "zero forge writes on a self-modifying MR" absolute? If it is absolute, what channel carries the BLOCK to the human reviewer — given that no thread is posted and the exit code is `0`?** Raised by RELI-01 ([D-138](../decisions/decisions.md)) and deliberately left **UNDECIDED**. The tension is real in both directions. **For absolute:** `openspec/specs/p5-aud-audit-remediation/spec.md` pins *"the decision is BLOCK with zero forge writes (GUARD-1 dominance over the gap-degrade)"* as a frozen acceptance criterion, and the guard exists so that an MR editing `.assent/**` cannot make assent vouch for its own policy — any write is a write the MR's own content influenced. **Against absolute:** the only human-visible surface then keeps whatever the previous run said, which today can be `✅ Decision: APPROVE`, so the guard's output is *invisible* to the reviewer it protects, and D-130's compensating control (a REVIEW rerun upserts the summary and adds an unresolved discussion) does not reach this path because no thread is posted. Zero *authority* writes need not mean zero *communication*. **Options, none taken here:** (a) keep it absolute and carry BLOCK on a non-forge channel — a non-zero exit code, or a required CI job status; (b) permit exactly one write, a fixed-text supersession/BLOCK note with no policy-derived content, which cannot be steered by the MR; (c) permit the summary upsert but not the thread. (b) and (c) both **reopen the frozen criterion above and need an openspec change proposal first** — spec before code. Note that (a) changes an exit-code contract wrapper scripts rely on ([`docs/usage/cli.md`](../usage/cli.md)), so it is not the free option it looks like. | Before the RELI-01 fix lands (v0.2.1) | Found by the 2026-08-09 audit's reliability lens; recorded in [D-138](../decisions/decisions.md). Evidence: `cmd/assent/run.go` step-9 GUARD switch, `openspec/specs/p5-aud-audit-remediation/spec.md`, `openspec/specs/p5-e5-provider-host/spec.md` REQ-E5-S08-03 | | OQ-32 | **RESOLVED (D-147, 2026-08-16): (b) — add a host-side secret resolver (process env / file path / hosted store); repo-side config gains only an opaque, host-allowlisted reference name, never a literal credential or URL pairing. ADR amending ADR-0015 §7 required before code.** Analysis retained below for the record. Original text: No provider transport can carry a credential, so NO provider can call Entra ID, Keycloak, or any token-authenticated IdP directly — and nothing says so. Found while designing P5-DEM (D-142). Verified across three surfaces that agree: `CallHTTP` (`internal/provider/transport.go`) sets **only** `Content-Type: application/json` — no header map, no bearer token, no client certificate; the repo-side provider schema (`schemas/policy/v1alpha1/config.schema.json` `$defs/provider`) is `additionalProperties:false` over exactly `{type, url, failure}`, so there is nowhere to put one; and `ScrubEnv`/`ScrubArgv` build the exec child's environment **from scratch** and refuse any name matching `(?i)(TOKEN\|SECRET)` **even when explicitly configured**, so the exec tier cannot carry one either. **This is not a bug — it is ADR-0015 §7 working exactly as designed**, and Spike C's `TestIsolation` proves it against a deliberately hostile provider that exfiltrates its whole environment and stdin. **What has never been written down is the consequence:** Entra ID and Keycloak both require a bearer token on every call, so the only shape that works today is a **broker** — a service holding the IdP credential *itself*, reachable by assent without one (loopback/sidecar, or mTLS terminated outside assent's transport). That is arguably the *correct* architecture: the credential never enters the decision path and a compromised provider's blast radius stays bounded. But it is undocumented, and it **narrows what `docs/vision.md:67` promises** ("pluggable providers: Keycloak, LDAP, GitLab/GitHub groups, ownership files, custom plugins") and what ADR-0004 §1 planned ("OIDC/Keycloak group lookup, LDAP" as builtins — never shipped). `docs/architecture/c4-context.md:19` is currently the **only** place stating the truth: *"Keycloak / LDAP: no builtin — reachable only via the generic HTTP/exec provider transport."* **Ruling needed (deliberately not taken):** **(a)** bless the broker pattern, document it in the provider-author guide, and amend `docs/vision.md:67` + ADR-0004 §1 to stop implying direct IdP calls — **RECOMMENDED**: costs nothing, keeps ADR-0015 §7 and the isolation proof intact, and is what DEM-S02/DEM-S03 are already written against; **(b)** add a narrow repo-side credential channel (header or secret-ref) to the HTTP transport — **reopens a frozen schema AND the trust boundary the hostile-provider isolation proof rests on**, and would need its own ADR; **(c)** state the limitation and add nothing. Note this is **not** merely a docs question under (a): a reader of the vision page today would reasonably budget a Keycloak integration as "configure a builtin" and discover mid-implementation that they must also deploy and operate a broker. **Not to be resolved by quietly adding a header field** — that is option (b) and it is a trust-boundary change. | Before DEM-S02 publishes the provider-author guide (the guide must state one of these answers); **not** a release blocker — the current behaviour is fail-safe, just undocumented | Found designing [P5-DEM](https://github.com/PlatformRelay/assent/blob/main/openspec/specs/p5-dem-demo-repos/spec.md) (judgment call (e)); recorded in [D-142](../decisions/decisions.md). Evidence: `internal/provider/transport.go` `CallHTTP`/`ScrubEnv`/`ScrubArgv`, `schemas/policy/v1alpha1/config.schema.json` `$defs/provider`, `docs/planning/spikes/spike-c-provider.md` § Isolation evidence, ADR-0015 §7, ADR-0004 §1, `docs/vision.md:67`, `docs/architecture/c4-context.md:19` | +| OQ-33 | **`protected-pipeline-source` (ADR-0015 §4's arming prerequisite) has no operationally decidable predicate on EITHER forge.** On GitLab it is a substring heuristic — `strings.Contains(proj.CIConfigPath, "@")` (`internal/forge/gitlab/snapshot.go:292`), the SEC-04 shape ADR-0021 forbids by name — which proves neither that the referenced CI config sits on a protected branch nor that the MR author cannot push to it; on GitHub there is no single readable analogue. E10-S00 proposes a **composite** predicate spanning the CI-env adapter (the run's trigger event is one whose workflow definition GitHub loads from the base repo's default branch — `pull_request_target` / `workflow_run` / `merge_group`) **and** a forge read (that default branch requires reviews, so the definition is not author-editable). Two sub-questions: **(a)** does ADR-0015 §4 accept an env+forge composite as proof of protected config, or must the predicate be forge-readable only? **(b)** if not, does v1 ship with arming unavailable — comment-only on GitHub, and on GitLab too once the heuristic is retired? | E10-S04 / S09 / S11 (arming); v1 GitHub gating | [github-addressing-model.md](github-addressing-model.md) Q2 row 11; ADR-0021 items 3+5–8 and Consequences; audit 2026-08-09 SEC-04. **Leading answer: (a)** — accept the composite, gated by a conformance case and reported as doctor evidence; the alternatives are a heuristic (rejected) or no gate at all. Retiring the `@` heuristic is a user-visible GitLab behaviour change owed its own D-row at S04/S09 (E10 judgment call (e)) | diff --git a/openspec/specs/backlog.md b/openspec/specs/backlog.md index 77c5c5e..42cd265 100644 --- a/openspec/specs/backlog.md +++ b/openspec/specs/backlog.md @@ -559,7 +559,7 @@ them rather than auto-merge. | ID | Story | Execution | Depends on | Gate contribution | | --- | --- | --- | --- | --- | -| E10-S00 | ⚠️ GitHub addressing & representation model (4 questions, ~1 page) | **[autonomous · design · LGTM]** | none | **do first** — kills both P0s before the port freezes | +| E10-S00 | **DONE** (D-155) — ⚠️ GitHub addressing & representation model: [github-addressing-model.md](../../docs/planning/github-addressing-model.md) answers all four questions, mints 10 conformance case IDs (each paired with a positive control), corrects **3 stale ADR-0021 anchors** (`provider_host.go:275→:292` — the D-130 registry — `:246→:263`, `Describe→GetMR`), and raises **OQ-33**: `protected-pipeline-source` has no decidable predicate on **either** forge, so `unknown ⇒ never arm` leaves v1 GitHub comment-only until it is answered. `git diff schemas/ == 0` holds — no schema change proposed. **Awaiting maintainer LGTM** | **[autonomous · design · LGTM]** | none | **do first** — kills both P0s before the port freezes | | E10-S01 | Extract the conformance suite into an importable package + observation surface | **[autonomous]** | S00 | first **code** story; no assertion may be weakened | | E10-S02 | ⚠️ `forge.RunPort` + **neutral factory** + MR-relative addressing + identity | **[autonomous · engine-grade · LGTM]** | S00, S01 | one neutral seam; ARCH-02 cannot recur | | E10-S03 | ⚠️ Collapse `SyntheticDigest` onto `Snapshot.Heads.MergeResultDigest` | **[autonomous · engine-grade]** | S02 | digest scheme adapter-owned; allowlist emptied | From d385f32606c021d9ba7b6f93842a0217a7beab7d Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 23 Aug 2026 18:19:07 +0200 Subject: [PATCH 2/3] :memo: docs(e10): demote GitHub eligible-approval-evidence to unknown (review fixes) Independent review of E10-S00 returned REQUEST CHANGES. Fixes F1-F11. F1 (P1): Q2 row 9 declared GitHub eligible-approval-evidence `supported{full}` on three conjuncts that establish a CODEOWNERS file exists, but none of which decides what `full` DENOTES - that the adapter-computed eligible set EQUALS the forge's. That property has no forge-readable predicate, exactly the situation row 11 handled correctly; two identical gaps had opposite treatments. It also contradicted the named spec input: forge-dossier-github.md section 2 grades the "which typed principal" property `partial`. Demoted to `unknown` and filed as OQ-34. The CODEOWNERS route stays legitimate - ADR-0017 section 3 names it - so the objection is section 3's OTHER clause, "typed eligible principals": an over-permissive matcher would put an ineligible principal into ev.Eligibility and approvalSatisfies would record the obligation satisfied, harm needing no arming. Consequence stated up front: with rows 9 and 11 both unknown, v1 GitHub comments and does not gate. F8: that CONFIRMS ADR-0021's unknown-forever prediction (whose stated reason was dossier section 2 step (b)) rather than refining it. D-155 reframed. F2: the anchor drift was misattributed. `246` appears nowhere in ADR-0021; `provider_host.go:246 refFilePort` is E10-S02's own DoD at spec.md:257 - fixed there. ADR-0021's genuine drifts are two. F3: the twelve minted case IDs had no enforcement path. Written into S01/S07/S14's DoD in the epic spec. F4: `record-schema-unchanged-under-multi-gap` could not fail - run.go:394 already validates every record. Replaced with a byte-identical capabilityGap case plus a positive control. Also corrected the loophole-closure claim: run.go:368 sets the gap unconditionally, so mergeResultDigest is null in 100% of runs and the schema's forbid-clause is latent until S03/S07. F6: OQ-33 now quotes ADR-0015 section 4 and carries its same-repo route (branch protection over .github/workflows/**, largely forge-readable) as a third candidate; "no readable analogue at all" was overstated. F5: refs/pull/N/head is ADR-0021 item 5, not dossier C13 (which names refs/pull/N/MERGE). F7: row 1's GitHub C constant now names its case. F9: gitlab.go:484 is the 200 branch. F10: the REL-03 gate is provider_host.go:299. F11: unresolvable-ref diagnostic wording noted. Refs: D-155, OQ-33, OQ-34, ADR-0021 items 5-8, ADR-0017 3, ADR-0015 4 --- CHANGELOG.md | 1 + docs/decisions/decisions.md | 2 +- docs/planning/github-addressing-model.md | 245 ++++++++++++++------- docs/planning/open-questions.md | 3 +- openspec/specs/backlog.md | 2 +- openspec/specs/p5-e10-github-forge/spec.md | 20 +- 6 files changed, 191 insertions(+), 82 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c0cf42..88f39ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,7 @@ repository still gets a decision, never by following the link; no release carrie - :memo: docs(audit): document the AUD2 gate and correct stale line counts (F4, F6) - :memo: docs(backlog): mark P5-AUD2 complete (S01-S05 landed) - :memo: docs(e10): GitHub addressing & representation model (E10-S00) +- :memo: docs(e10): demote GitHub eligible-approval-evidence to unknown (review fixes) ### Features - :sparkles: feat(audit): measure REL-03/REL-07 by real mutation run, not by name diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index 4bcb4d8..595fd8e 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -158,4 +158,4 @@ project/process decisions. | D-151 | 2026-08-16 | **OpenSSF Best Practices project creation is RESOLVED: (a) — the operator will create the `bestpractices.dev` project for `PlatformRelay/assent`.** Unblocks SEC-SC-S02 (evidence page + README badge, once *passing*); SEC-SC-S01 (fuzzing) is not blocked by this and can proceed independently. Operator action only — requires a personal account at bestpractices.dev, not automatable in-tree. Current CII score is 0, which drags the OpenSSF Scorecard (~7); the repo already satisfies most criteria (pinned CodeQL, Dependabot, secret scanning, signed releases, SLSA provenance). Revert: leave CII at 0 indefinitely — SEC-SC-S01 is unaffected either way. | | D-152 | 2026-08-18 | **The 2026-08-18 audit's "Next (risk reduction)" wave is decomposed as its own epic P5-AUD2 (`openspec/specs/p5-aud2-audit-remediation/spec.md`), five stories, spec-first.** Context: `agent-context/PROJECT-AUDIT-2026-08-18.md` closed both P1 conditions the same day and v0.3.0 shipped, leaving a named but untracked wave — exec-transport trio (REL-01/02/07), REL-03 `ErrNotFound` discrimination, SEC-03 cosign identity pin, TEST-02 mutant. Options considered: **(a)** hand-fix them as loose commits (rejected — AGENTS.md rule 4 is spec-first, and REL-01 is now byte-identical across *three* audits precisely because nothing ever tracked it); **(b)** append them to the existing P5-AUD epic (rejected — that epic's exit gate is CLOSED and reopening it would make "AUD complete" a moving claim); **(c)** a new AUD2 epic keyed to this audit **(chosen)**. Three consequences recorded here so they are not re-litigated: **(1)** AUD2 has **no release-condition story** — the audit had exactly two P1s and both are already closed; **(2)** **WG-S01 is deliberately excluded** — it carries the LGTM governance marker, and GOVERNANCE says such stories are surfaced to the maintainer, which an autonomous loop's decide-and-log does not override; **(3)** AUD2's exit gate is a **`task check` stage**, not a `release-exitgate` step, because that job is `pull_request`-skipped (RELSE-08) and wiring the gate there is how AUD-S18's own stale `CHECK_STAGES` pin survived four merges. A 2026-08-10-keyed AUD2 draft exists only in the local stash `leave-aud2-not-this-epic`, was never committed, and is superseded; its still-open items (F3/F5/F7) stay Later-wave. Revert: delete the spec + backlog section; the four findings return to the audit report untracked. | | D-153 | 2026-08-19 | **The cosign signer-identity pin published in `SECURITY.md` was WRONG, not merely missing from `hack/install.sh`: the identity regexp is widened to `^https://github\.com/PlatformRelay/[Aa]ssent/` in both files (AUD2-S03 / SEC-03).** Substance first: `SECURITY.md`'s "Verify a tagged release" instructions pinned `--certificate-identity-regexp '^https://github.com/PlatformRelay/assent/'`, and an adopter who followed them on **v0.2.0 or v0.3.0 got a verification FAILURE on a genuine, correctly signed artifact** — the published recipe has been broken for every release since v0.2.0, and the natural reading of that failure is "this release was tampered with". Cause: the repository was renamed `PlatformRelay/assent` → `PlatformRelay/Assent` between v0.1.0 and v0.2.0; the keyless signing certificate's Subject Alternative Name carries GitHub's canonical casing; cosign compiles `--certificate-identity-regexp` as a Go RE2 pattern and matches it **case-sensitively**. Decoded from the published bundles: v0.3.0 and v0.2.0 sign as `https://github.com/PlatformRelay/Assent/.github/workflows/release.yaml@refs/tags/vX.Y.Z`, v0.1.0 as `https://github.com/PlatformRelay/assent/.github/workflows/release.yaml@refs/heads/main`. All three verify under the new value; independently re-confirmed with real cosign against the real v0.3.0 artifact (`Verified OK` under the new pin, exit 1 "expected SAN value to match regex" under the old). Options considered: **(a)** ship the pin byte-identical to the published-but-broken value and file the breakage as a follow-up (rejected — it would land a green gate asserting a guarantee that fails closed on this project's own releases, D-124's defect one level up); **(b)** case-insensitive `(?i)` (rejected — wider than the defect and it silently accepts casings GitHub never issues); **(c)** an explicit `[Aa]` class in both files, dots escaped (chosen). Not widened otherwise: the `^` anchor and the owner/repo scope stand, so another owner, an `assent-mirror` typosquat, another forge and an unescaped-dot host all still fail. Enforcement, per D-128: **one** published truth — `hack/release/install_cosign_pin_test.sh` extracts the pair from `hack/install.sh` and `SECURITY.md`, requires exactly one distinct value per file, reddens on drift, and (this is the assertion that would have caught the defect) matches the pin against the three **real** SANs above, committed as offline fixtures, plus six negatives. Extends D-110 (bundles beside archives) and D-109; the maintainer-path twin at `hack/release/verify-artifacts.sh:124` is still unpinned and is tracked in the backlog, not fixed here. Revert: restore `'^https://github.com/PlatformRelay/assent/'` in both files — which re-breaks verification of v0.2.0 and every later release, so revert only together with a repo rename back to lowercase. | -| D-155 | 2026-08-23 | **E10-S00 — the GitHub addressing & representation model is written ([docs/planning/github-addressing-model.md](../planning/github-addressing-model.md)) and ADR-0021 items 5–8 are executed UNAMENDED**, because nothing in the four answers contradicts them. **(1) Fork-head addressing (P0):** the governed subject moves to MR-relative `FileAtBase(mr, path)` / `FileAtHead(mr, path)`; `FileAtRef(project, path, targetRef)` survives for ref-addressed decision inputs. Verified against the tree rather than transcribed: **exactly two call sites migrate** (`cmd/assent/run.go:270`, `:274`) and **six must not** (`run.go:203/211/230/249`, `provider_host.go:82/292`) — and **three of ADR-0021's anchors have drifted**: the resource-owner registry read (D-130, "who may approve") is at `provider_host.go:292` not `:275`, `refFilePort` is at `:263` not `:246`, and the port method is `GetMR` not the proposed `Describe`. **(2) Capability predicates:** every dossier §4 flag gets one of two kinds — `P` probed (named endpoint + field + comparison) or `C` contract-proven (a constant licensed by a NAMED conformance case; no case ⇒ `unknown`) — which makes "we assume the API supports it" legal exactly once. Counted against the tree, GitLab probes 3 of 11 honestly, 1 by heuristic, hardcodes 1, and leaves **7 implicitly assumed**; `unknown ⇒ never arm` binds per capability at its consultation point (`internal/forge/precondition.go` consults 3 today), not "all eleven". **A finding that improves on ADR-0021's own prediction:** `eligible-approval-evidence` is reachable as `full` on GitHub via `codeowners` (already a first-class `VerifyingCapability` at `internal/core/aggregate/approval.go:126-133`) — base-ref CODEOWNERS + team expansion yields the named eligible-id set the engine requires — whereas the `aggregate` route (GraphQL `reviewDecision`) is **not a satisfying sub-value at all**, exactly like GitLab Free. **A finding that is worse than expected:** `protected-pipeline-source` is `unknown` on **both** forges — GitLab's `strings.Contains(ci_config_path, "@")` is a heuristic, not a predicate → **OQ-33**, and retiring it is a user-visible GitLab behaviour change owed its own D-row at S04/S09 (judgment call (e)), not minted here. **(3) Record surface (P0):** ADR-0021 item 8 option (ii) confirmed — the multi-capability report is `doctor` output and arming-refusal reasons only, never the DecisionRecord; **`git diff schemas/ == 0` holds and NO schema change is proposed.** Sharpened: comma-joining gaps into `pins.capabilityGap` is not a loophole (the `allOf` FORBIDS the field whenever `mergeResultDigest` is pinned, so the channel closes exactly when the merge succeeds), and there is therefore **no gap-selection rule to invent** — the field is reserved for one capability. The audit-trail cost is real but **pre-existing**: `aggregate.Result.CapabilityGaps` already never reaches the record, and `run.go:368` writes one hardcoded merge-result string. **(4) Status → sentinel:** `forge.ErrUnauthorized` lifts to the port at S02; a GitHub **404 is absence only when a CONTENT-SCOPE probe (a sibling `/contents` read at the same ref) returned 200** — deliberately NOT a repo-metadata probe, since a fine-grained PAT with `metadata: read` and no `contents: read` gets 200 on `GET /repos/{o}/{r}` and 404 on every file, which would re-open the P0 through the front door; a 404 on branch-protection/ruleset reads without `permissions.admin` is `unknown` (never `absent`), and rate-limited 403s are transport errors — extending the AUD2-S02/REL-03 discrimination one layer down into the adapter that mints the sentinel. Ten conformance case IDs are minted (each fail-closed case paired with a positive control, per E10-S12's rule applied early); `internal/forge/conformance/catalog.yaml` is S01's to edit, not this story's. ADR-0021 stays **Proposed** — accepting it is the maintainer's call, and E10-S00 is `[LGTM]`. | +| D-155 | 2026-08-23 | **E10-S00 — the GitHub addressing & representation model is written ([docs/planning/github-addressing-model.md](../planning/github-addressing-model.md)) and ADR-0021 items 5–8 are executed UNAMENDED.** **(1) Fork-head addressing (P0):** the governed subject moves to MR-relative `FileAtBase(mr, path)` / `FileAtHead(mr, path)`; `FileAtRef(project, path, targetRef)` survives for ref-addressed decision inputs. Verified against the tree rather than transcribed: **exactly two call sites migrate** (`cmd/assent/run.go:270`, `:274`) and **six must not** (`run.go:203/211/230/249`, `provider_host.go:82/292`). **Anchor drift, attributed to the document that carries it:** ADR-0021's genuine drifts are **two** — `provider_host.go:275` (the D-130 who-may-approve registry read; actual `:292`) and the proposed `Describe(project, mr)` (the tree's method is `GetMR`). A third stale anchor, `provider_host.go:246 refFilePort` (actual `:263`), lives in `openspec/specs/p5-e10-github-forge/spec.md:257` — E10-S02's own DoD, where an implementer would have tripped on it — and is **fixed in this change**. **(2) Capability predicates:** every dossier §4 flag gets one of two kinds — `P` probed (named endpoint + field + comparison) or `C` contract-proven (a constant licensed by a NAMED conformance case; no case ⇒ `unknown`). Counted against the tree, GitLab probes 3 of 11 honestly, 1 by heuristic, hardcodes 1, and leaves **7 implicitly assumed**; `unknown ⇒ never arm` binds per capability at its consultation point (`internal/forge/precondition.go` consults 3 today), not "all eleven". **BOTH arming-relevant GitHub capabilities are `unknown`, so v1 GitHub COMMENTS AND DOES NOT GATE** — the fail-closed outcome, stated up front. `protected-pipeline-source` is `unknown` on **both** forges (GitLab's `strings.Contains(ci_config_path, "@")` is a heuristic, not a predicate) → **OQ-33**, which now poses ADR-0015 §4's own three routes including the largely forge-readable same-repo one; retiring the heuristic is a user-visible GitLab behaviour change owed its own D-row at S04/S09 (judgment call (e)), not minted here. `eligible-approval-evidence` is **`unknown`** → **OQ-34**: the CODEOWNERS route is legitimate (ADR-0017 §3 names it; the frozen schema enumerates `codeowners`), but no probeable conjunct decides what `full` DENOTES — that the adapter-computed eligible set EQUALS the forge's — and an over-permissive matcher would make the DecisionRecord assert an obligation satisfied by an ineligible principal, harm needing no arming. This **confirms** ADR-0021's Consequences prediction (whose stated reason was dossier §2 step (b), the CODEOWNERS step) and **agrees with** the dossier's own `partial` verdict; an earlier draft of this model graded it `full`, contradicting that spec input, and the wrong answer is kept visible in the doc rather than edited away. **(3) Record surface (P0):** ADR-0021 item 8 option (ii) confirmed — the multi-capability report is `doctor` output and arming-refusal reasons only, never the DecisionRecord; **`git diff schemas/ == 0` holds and NO schema change is proposed.** There is **no gap-selection rule to invent** — the field is reserved for one capability. Stated honestly rather than over-claimed: the `allOf` forbids `capabilityGap` only when `mergeResultDigest` is pinned, and `run.go:368` sets the gap **unconditionally**, so the channel is open in 100% of runs today and the schema clause is a **latent** guard until an adapter pins a real digest (S03/S07); what closes the comma-join loophole meanwhile is a conformance case, not the schema. The audit-trail cost is real but **pre-existing**: `aggregate.Result.CapabilityGaps` already never reaches the record. **(4) Status → sentinel:** `forge.ErrUnauthorized` lifts to the port at S02; a GitHub **404 is absence only when a CONTENT-SCOPE probe (a sibling `/contents` read at the same ref) returned 200** — deliberately NOT a repo-metadata probe, since a fine-grained PAT with `metadata: read` and no `contents: read` gets 200 on `GET /repos/{o}/{r}` and 404 on every file, which would re-open the P0 through the front door; a 404 on branch-protection/ruleset reads without `permissions.admin` is `unknown` (never `absent`), and rate-limited 403s are transport errors — extending the AUD2-S02/REL-03 discrimination one layer down into the adapter that mints the sentinel. **Twelve** conformance case IDs are minted, each fail-closed case paired with a positive control (E10-S12's rule applied early), and they are **wired into S01/S07/S14's DoD in the epic spec** rather than left in a planning doc nothing reads; `internal/forge/conformance/catalog.yaml` remains S01's to edit. ADR-0021 stays **Proposed** — accepting it is the maintainer's call, and E10-S00 is `[LGTM]`. | diff --git a/docs/planning/github-addressing-model.md b/docs/planning/github-addressing-model.md index 3ae2679..cae72ef 100644 --- a/docs/planning/github-addressing-model.md +++ b/docs/planning/github-addressing-model.md @@ -7,7 +7,14 @@ > guesswork), [forge-dossier-gitlab.md](forge-dossier-gitlab.md), ADR-0015 §1/§2/§4/§8, > ADR-0017 §1/§3, ADR-0020, GUIDELINES "Safety invariants". > **Raises**: [OQ-33](open-questions.md) — `protected-pipeline-source` has no decidable -> predicate on either forge today. +> predicate on either forge today; **OQ-34** — whether an adapter-computed CODEOWNERS eligible +> set can ever be `full` under ADR-0017 §3's "typed eligible principals". +> +> **Revision (post-review)**: the first version graded GitHub `eligible-approval-evidence` as +> `supported{full}`. That contradicted this document's own named spec input (dossier §2 grades +> the property `partial`) and was the exact "papered over with a heuristic" outcome +> `REQ-E10-S00-02` forbids. It is now **`unknown`** → OQ-34. Q2's row-9 section keeps the wrong +> answer visible rather than editing it away, because *how* it was wrong is the reusable part. This is E10 story zero: four questions answered on paper before the port signature freezes. Each answer names the conformance case that will prove it. Case IDs are *minted here and @@ -65,12 +72,20 @@ worth running — two of its line anchors have drifted. As of this change's HEAD | `provider_host.go:82` | provider host declaration | `targetRef` | stays `FileAtRef` | | `provider_host.go:292` | **resource-owner registry** | `targetRef` | stays `FileAtRef` — **D-130** | -Exactly **two** call sites migrate; **six** must not. Corrections to ADR-0021's anchors: -the resource-owner registry read is at `provider_host.go:292` (ADR-0021 cites `:275`, which -now falls inside that function's doc comment), and `refFilePort` is declared at -`provider_host.go:263` (ADR-0021 cites `:246`). `forgePort` at `run.go:64` is correct. A -third drift for S02 to absorb: ADR-0021 item 1 proposes `Describe(project, mr)`; the tree's -method is `GetMR(project, mr)` (`run.go:68`). +Exactly **two** call sites migrate; **six** must not. + +**Anchor drift, attributed to the document that actually carries it** — this matters because +S02 reads the epic spec as its own DoD, so a correction filed against the wrong document +leaves the stale anchor exactly where an implementer will trip on it: + +| Stale anchor | Carried by | Correct value | +| --- | --- | --- | +| `provider_host.go:275` (the D-130 resource-owner registry read) | **ADR-0021** — Decision items 1 and 5 | `provider_host.go:292`; `:275` now falls inside that function's doc comment | +| `Describe(project, mr string)` | **ADR-0021** — Decision item 1 | the tree's method is `GetMR(project, mr)` (`run.go:68`) | +| `provider_host.go:246 refFilePort` | **`openspec/specs/p5-e10-github-forge/spec.md:257`** (E10-S02's DoD) — the string `246` appears nowhere in ADR-0021 | `provider_host.go:263`; **fixed in this change**, since the epic spec is in this story's fence | + +So ADR-0021's genuine drifts are **two**, not three, and the third belongs to the epic spec. +`run.go:64 forgePort` (spec.md:256) is **correct** and needs no change. `run.go:230` and `provider_host.go:292` deserve the specific mention ADR-0021 gives them: the first carries provider-host declarations, so migrating it would let a fork's head @@ -88,7 +103,9 @@ Both forges expose an **MR-relative head ref inside the target project**, which shape is neutral rather than GitHub-shaped: - **GitHub** — `GET /repos/{o}/{r}/contents/{path}?ref={pr.head.sha}`; the fork's head commit - is reachable in the base repo as `refs/pull/{n}/head` (`dossier` C13). Addressing by the + is reachable in the base repo as `refs/pull/{n}/head` (source: **ADR-0021 item 5**, not the + dossier — C13 names `refs/pull/N/`**`merge`**, a semantically different ref carrying *merged* + content, not head content; `unverified` until S06 confirms the head ref live). Addressing by the head **SHA** rather than the ref name is preferred: it is the same value pinned into `pins.sourceSha`, so the record and the read cannot disagree. - **GitLab** — the source project id from the MR object, or the target project's @@ -138,7 +155,7 @@ promoted into it that is currently assumed-true becomes an honest gap and owes a | # | Capability | GitLab predicate | GitHub predicate | Kind | | --- | --- | --- | --- | --- | -| 1 | `resolvable-threads` | constant `supported`, licensed by the existing `p3e5-*` reconciliation cases | constant `supported`, licensed by a GraphQL case (`reviewThreads.isResolved` / `resolveReviewThread`, `dossier` C1/C2) | `C` | +| 1 | `resolvable-threads` | constant `supported`, licensed by the existing `p3e5-*` reconciliation cases | constant `supported`, licensed by `threads-resolvable-graphql` / `TestConformanceThreadResolveRoundTrip` (`reviewThreads.isResolved` / `resolveReviewThread`, `dossier` C1/C2) | `C` | | 2 | `threads-block-merge` | `GET /projects/{id}` → `only_allow_merge_if_all_discussions_are_resolved == true` (`code`: `snapshot.go:290`) | `GET /repos/{o}/{r}/branches/{base}/protection` → `required_conversation_resolution.enabled == true`, **or** the equivalent ruleset row from `GET /repos/{o}/{r}/rules/branches/{base}`; both must be consulted (endpoints `dossier` C3 + §2 step (a); **field name `unverified`**) | `P` | | 3 | `blocking-review` | `absent` — GitLab has no `REQUEST_CHANGES` primitive; ADR-0017 §3 uses threads (`dossier` C4) | `required_pull_request_reviews.required_approving_review_count >= 1` on the base branch; without required reviews a `REQUEST_CHANGES` review does not block (`dossier` C4) | `P` | | 4 | `review-dismissal-restrictions` | `absent` — no analogue | `required_pull_request_reviews.dismissal_restrictions` non-empty **and** `users[].id ∪ expand(teams[]) ∌ pr.user.id`; team expansion via `GET /orgs/{org}/teams/{slug}/members` (`dossier` C8′) | `P` | @@ -146,7 +163,7 @@ promoted into it that is currently assumed-true becomes an honest gap and owes a | 6 | `deferred-merge-arming` | constant `supported` (MWPS is tier-independent, `dossier` C11) | `GET /repos/{o}/{r}` → `allow_auto_merge == true`; `enablePullRequestAutoMerge` fails without the repo setting (setting `dossier` C11; **field name `unverified`**) | `C` / `P` | | 7 | `arming-revoked-on-push` | constant `supported` — any new commit cancels MWPS (`dossier` C11) | `supported` **only** when the substitute is configured — stale-approval dismissal **and** at least one required status check; a write-access push does **not** auto-disarm GitHub auto-merge (`dossier` §3 delta 2). Otherwise `absent`. (**Field names `unverified`** — C19 names the setting, not the API field) | `P` | | 8 | `merge-result-pinning` | `GET /projects/{id}` → `merge_trains_enabled == true` (`code`: `snapshot.go:291`) | base-branch ruleset from `GET /repos/{o}/{r}/rules/branches/{base}` contains the merge-queue rule (endpoint `dossier` C14; **rule-type string `unverified`**) | `P` | -| 9 | `eligible-approval-evidence{full\|aggregate}` | `full` iff `GET /projects/{id}/merge_requests/{iid}/approval_rules` returns rules with `eligible_approvers[]` (`code`: `gitlab/snapshot.go` `hasApprovalRulesAPI`); else `absent` | `full` iff `required_pull_request_reviews.require_code_owner_reviews == true` **and** CODEOWNERS is readable **at the base ref** **and** every referenced team expands (`GET /orgs/{org}/teams/{slug}/members` → 200). Team expansion 403 → `unknown`. See below | `P` | +| 9 | `eligible-approval-evidence{full\|aggregate}` | `full` iff `GET /projects/{id}/merge_requests/{iid}/approval_rules` returns rules with `eligible_approvers[]` (`code`: `gitlab/snapshot.go` `hasApprovalRulesAPI`); else `absent` | **`unknown`** — see below. The CODEOWNERS route is legitimate (ADR-0017 §3 names it) and its preconditions are probeable (`require_pull_request_reviews.require_code_owner_reviews == true`, CODEOWNERS readable **at the base ref**, teams expandable via `GET /orgs/{org}/teams/{slug}/members`), but **none of them decides the property `full` denotes** — that the adapter-computed eligible set equals the forge's. No forge-readable predicate exists → `unknown`, promoted only by the fidelity case named below (**OQ-34**) | `P` → `unknown` | | 10 | `approval-reset-on-push{default\|opt-in}` | `GET /projects/{id}/approvals` → `reset_approvals_on_push == true` — **currently unprobed** (audit RELI-03; **`unverified`** — the field is named by RELI-03, not read from this tree) | stale-approval dismissal enabled on the base branch; the stronger variant is "require approval of the most recent reviewable push" (`dossier` C19/C8; **field names `unverified`**) | `P` | | 11 | `protected-pipeline-source` | **no predicate today** — see below | **no predicate today** — see below | — | @@ -159,52 +176,95 @@ turns out to be right is promoted at S09; a cell that turns out to be wrong neve anything. Rows 3, 4 and 9's GitHub field names are genuinely enumerated in dossier §2 and are not tagged. -### Row 9 is the one that decides whether GitHub can ever gate — and it is decidable - -The dossier's "no API returns the computed per-PR eligible code owners" reads like a dead end. -It is not, because assent never needed the *forge* to compute the set — it needs a -**forge-proven named principal set**, and `code` shows the engine already accepts two ways to -get one: `internal/core/aggregate/approval.go:126-133` admits -`VerifyingCapability ∈ {"approval-rules-api", "codeowners"}` and fails closed on anything -else, and `approvalSatisfies` counts an approver only if its id is in `ev.Eligibility`. - -So: - -- **`aggregate` is not a satisfying sub-value.** GraphQL `reviewDecision: APPROVED` proves the - forge enforced *its* rule but names no principal; fed to the engine it is - `VerifyingCapability: "none"` → a recorded capability gap that **never satisfies** - (`approval.go:130`). Recording `aggregate` as `supported` would be exactly the - paper-over REQ-E10-S00-02 forbids. -- **`full` is reachable on GitHub via `codeowners`**, which is already a first-class verifying - capability: CODEOWNERS read **from the base ref** (fork-safe by GitHub's own definition, - `dossier` §2 step (b)), matched client-side, teams expanded to ids, owners filtered to - write-holders. The predicate above is what makes that honest rather than optimistic — and - it is symmetric with GitLab Free, which is `absent` for the same reason (no named eligible - set) and already refuses to arm (`precondition.go:70-78`). - -This **refines**, and does not contradict, ADR-0021's Consequences prediction that -`eligible-approval-evidence` would plausibly be `unknown` on GitHub forever. The prediction was -about the *aggregate* route; the `codeowners` route was already in the engine. It also needs no -schema change: `code`, +### Row 9 decides whether GitHub can ever gate — and the honest answer is `unknown` + +A first draft of this document declared GitHub row 9 `supported{full}` on three conjuncts: +`require_code_owner_reviews == true`, CODEOWNERS readable at the base ref, and every referenced +team expandable. **That was wrong, and the way it was wrong is worth recording**, because it is +the failure mode `REQ-E10-S00-02` names: each conjunct establishes that a CODEOWNERS file +exists and that GitHub *cares* about code owners. **None of them establishes what `full` +denotes** — that the adapter's computed eligible set **equals** the set GitHub would accept. +That property has no forge-readable predicate, which is exactly the situation row 11 handles +correctly. Two identical gaps had opposite treatments; the asymmetry, not the CODEOWNERS route, +was the defect. + +**Decision: GitHub row 9 is `unknown` for v1**, promoted only by a named fidelity case (below). + +### What is and is not in dispute + +**CODEOWNERS is a legitimate route — this is not the objection.** ADR-0017 §3 names it +explicitly: `require-review` is "satisfied only by forge-proven eligible approval (**approval +rules / CODEOWNERS evidence**, typed eligible principals)". `code`: the frozen schema agrees — `schemas/approval/v1alpha1/approval-evidence.schema.json:40-42` enumerates -`verifyingCapability` as exactly `["approval-rules-api", "codeowners", "none"]` — the value is -already frozen-legal. - -**The asymmetry a reviewer will find, owned here rather than left to be discovered.** Two -things are true and both should be said. First, **no adapter produces `"codeowners"` today** — -`code`: the only writer is `internal/forge/gitlab/resolve.go:85`, which emits -`"approval-rules-api"`; `"codeowners"` appears in the tree solely as an accepted *input* value -(`approval.go:41`, `:127`) and in the frozen enum. GitHub would be its first producer. Second, -GitLab's `eligible_approvers[]` is **forge-computed** while GitHub's CODEOWNERS set is -**adapter-computed from forge-supplied data**, and ADR-0017 §3 says *forge-proven*. The -defence is that the frozen contract already ruled on this: it admits `codeowners` as a real -verifying capability, distinct from `none`, precisely because a CODEOWNERS file served by the -forge **at the base ref** is forge-supplied evidence — what the adapter contributes is -pattern-matching, not authority. The trust-boundary half is what makes it hold, and it is why -the predicate says *base ref*: a head-ref CODEOWNERS would let an author name themselves owner, -which is D-130's shape on a different file. If the maintainer reads ADR-0017 §3 more strictly -than the schema does, the consequence is stated and survivable — GitHub falls to `absent` for -require-review, exactly like GitLab Free, and gates nothing rather than gating wrongly. +`verifyingCapability` as exactly `["approval-rules-api", "codeowners", "none"]`, and +`internal/core/aggregate/approval.go:126-133` admits `"codeowners"` as a real capability, +distinct from `none`. So no schema change and no ADR amendment is implied by the route itself. + +**The objection is §3's *other* clause — "typed eligible principals".** GitLab's +`eligible_approvers[]` is **forge-computed**; a GitHub CODEOWNERS set is **adapter-computed** +from forge-supplied bytes. What the adapter contributes is a matcher, and a matcher can be +wrong in a direction that is not fail-safe. + +**The concrete slip-through, spelled out because a vaguer statement would not have caught the +first draft.** An over-permissive matcher — first-match-wins instead of last-match-wins, an +unhandled section or negation syntax, case-folding, or a team-membership read returning a +superset of actual write-holders — puts a principal into `ev.Eligibility` that GitHub would not +accept. `approvalSatisfies` then finds the approver's id in the eligible set +(`approval.go:127`, `:150-158`) and records the obligation **satisfied**. *No arming is needed +for the harm*: the DecisionRecord — the product's whole thesis — asserts that a governance +obligation was met by a principal the forge does not recognise as eligible. GitHub's own +`require_code_owner_reviews` is not a backstop, because it enforces over the **PR's changed +files** while assent's eligible set is scoped to the **governed subject**; the two sets need +not coincide. + +### Reconciling the spec input, which graded this `partial` and was right + +`docs/planning/forge-dossier-github.md` §2 closes: *"Verdict for ADR-0017 §3: GitHub can prove +**that** eligible approval exists (aggregate `reviewDecision` + protection config) — `partial` +on proving **which** typed principal satisfied **which** rule."* The capability's sub-values +are `{full|aggregate}`, and `full` is precisely the "which typed principal" property the +dossier grades `partial`. **The first draft's `full` contradicted its own named spec input.** +`unknown` is the grading that agrees with it: not `absent` (the route exists and the evidence is +partly forge-supplied), not `full` (unproven), and non-arming under `unknown ⇒ never arm`. + +This also **confirms rather than refines ADR-0021's Consequences prediction.** The ADR predicted +`eligible-approval-evidence` would plausibly report `unknown` on GitHub forever, and its stated +reason was dossier §2 step (b) — the CODEOWNERS step, not the aggregate route. That reason +stands. + +### The `aggregate` sub-value satisfies nothing, on either forge + +GraphQL `reviewDecision: APPROVED` proves the forge enforced *its* rule but names no principal. +Fed to the engine it is `VerifyingCapability: "none"` → a recorded capability gap that **never +satisfies** (`approval.go:129-130`). Recording `aggregate` as `supported` would be the +paper-over `REQ-E10-S00-02` forbids by name. This is symmetric with GitLab Free, which is +`absent` for the same reason — no named eligible set — and already refuses to arm +(`precondition.go:70-78`). + +### What would promote row 9 to `full` — named, not hand-waved + +`unknown` is a state with an exit, and S08 owns it. Two candidate routes, neither specifiable +as a *proof* today, which is why this is **OQ-34** rather than a decision: + +1. **Fixture-corpus fidelity case** — `codeowners-eligible-set-matches-forge` / + `TestConformanceCodeownersEligibleSetMatchesForge`, over a corpus covering last-match-wins, + sections, negation, case sensitivity and team expansion, **paired with a positive control** + proving the case reddens on a deliberately over-permissive matcher (this document's own + E10-S12 pairing rule). *Limit, stated:* it proves the matcher against fixtures, not against + GitHub's live computation — fidelity to the spec, not equality with the forge. +2. **Live cross-check** against GitHub's own code-owner determination for the PR + (`unverified`). *Limit, stated:* any such signal is scoped to the PR's changed files and is + mutable after PR open, so it is corroboration, not authority. + +Until one is accepted and discharged, row 9 stays `unknown` and **GitHub does not satisfy +`require-review`** — the same standing as GitLab Free, which ships today. Consequence: with +rows 9 and 11 both `unknown`, **v1 GitHub comments and does not gate.** That is the fail-closed +outcome the project's thesis prefers over gating on an unproven set. + +**One asymmetry owned rather than left to be found:** `"codeowners"` is accepted by the engine +and legal in the frozen enum, but **no adapter produces it today** — `code`: the only writer is +`internal/forge/gitlab/resolve.go:85`, which emits `"approval-rules-api"`. GitHub would be its +first producer, which is part of why the fidelity case is owed before, not after. ### Row 11 is the honest failure, and it is worse than "GitHub lacks it" @@ -226,13 +286,28 @@ resolves to project *X* → `GET /projects/X/protected_branches` covers the file the MR author lacks push access there), and degrades to `unknown` whenever the token is not scoped to *X*. -On GitHub there is no single readable analogue at all. The plausible predicate is a -**composite of environment and forge** (`unverified`): the run's trigger event is one whose -workflow definition GitHub loads from the base repository's default branch -(`pull_request_target` / `workflow_run` / `merge_group`), **and** that default branch requires -reviews, so the definition is not author-editable. Under `pull_request` from a fork the token -is read-only and secretless (`dossier` C17) — safe, and advisory-only per ADR-0015 §8, which -is a *non-arming* state, not a gap. +On GitHub there is no *single* readable field — but "no readable analogue at all" would +overstate it against this document's own cited input. **ADR-0015 §4 already frames the GitHub +condition, and in composite terms**: the assent job must come from a protected source — +*"GitHub: workflows from the target branch (`pull_request` runs the base-ref workflow for +forks; same-repo branches need branch protection on workflow paths)"* — and §4 states it is +"verified by `assent doctor`". Three candidate predicates follow, and OQ-33 asks which the +operator accepts: + +1. **Same-repo route (§4's own second clause, largely forge-readable).** Branch protection or a + path-restricted ruleset covering `.github/workflows/**` on the base branch, so a same-repo + PR author cannot alter the definition that gates them. This is the closest thing to a + GitLab-style forge-only predicate and §4 already names it; its readability limits + (path-scoped push rulesets are org/Enterprise-shaped) are `unverified`. +2. **Fork route (§4's first clause).** Under `pull_request` from a fork GitHub runs the + **base-ref** workflow with a read-only, secretless token (`dossier` C17). Safe, but + advisory-only per ADR-0015 §8 — a *non-arming* state, not a gap, and therefore not a route + to `supported`. +3. **Composite env+forge route** (`unverified`): the trigger event is one whose workflow + definition GitHub loads from the base repository's default branch (`pull_request_target` / + `workflow_run` / `merge_group`) **and** that branch requires reviews. This spans + `cmd/assent`'s CI-env adapter and `internal/forge`, which is the part §4 did not anticipate + being asked to *probe* rather than document. Consequences, stated rather than discovered: @@ -241,8 +316,9 @@ Consequences, stated rather than discovered: 2. Retiring the `@` heuristic makes **GitLab** arming paths that pass today stop passing. That is the intended surfacing (judgment call (e)) and it owes its own decision row at S04/S09 — **not** here, because S00 changes no behaviour. -3. The composite predicate spans `cmd/assent`'s CI-env adapter and `internal/forge`, which - ADR-0015 §4 did not anticipate. That is why it is a question, not a decision → **OQ-33**. +3. Route 1 may be forge-readable enough to stand alone; routes 3 spans `cmd/assent`'s CI-env + adapter and `internal/forge`. Which of these ADR-0015 §4 accepts as a *probe* — as opposed + to documentation verified by hand — is not this story's to decide → **OQ-33**. ### What GitLab actually probes today (judgment call (e), made concrete) @@ -262,6 +338,7 @@ consequence ADR-0021 promised, with a count attached. | Case id | Test name | Proves | | --- | --- | --- | +| `threads-resolvable-graphql` | `TestConformanceThreadResolveRoundTrip` | licenses row 1's GitHub `C` constant — post a thread, resolve it via GraphQL, read `isResolved`. No case, no constant: the flag reports `unknown` | | `capability-report-exhaustive` | `TestConformanceCapabilityReportExhaustive` | every enum member has an entry; a new member without one fails to compile or fails the case | | `capability-unknown-never-arms` | `TestConformanceUnknownCapabilityNeverArms` | `unknown` at a consultation point refuses arming, `merges == 0` | | `capability-supported-does-arm` | `TestConformanceSupportedCapabilityArms` | **positive control**, `merges == 1` — without it the previous case is vacuous (E10-S12's rule, applied one story early) | @@ -285,10 +362,16 @@ Three consequences follow directly, and the second is the one a reviewer will re 1. `capabilityGap` models **one** capability, merge-result pinning. (A prior draft's claim that it "already models the absent-capability case" was **False** and stays retired.) -2. **Comma-joining eleven gaps into that string is not a loophole.** It passes validation, but - it is a semantic lie against the field's own description, and it is *unavailable in the - common case*: when `mergeResultDigest` **is** pinned, the field is forbidden outright. A - channel that closes precisely when the merge succeeds is not an audit trail. +2. **Comma-joining eleven gaps into that string must be refused — but the schema does not + refuse it today, and saying otherwise would be the same over-claim this document is trying + to avoid.** Such a string passes validation; it is a semantic lie against the field's own + description; and the `allOf` at `:94-100` forbids the field only *when `mergeResultDigest` + is pinned*. `code`: `cmd/assent/run.go:368` calls `decision.MergeResultGap(...)` + **unconditionally**, so `mergeResultDigest` is null and the channel is **open in 100% of + runs today**. The schema clause is therefore a *latent* guard that goes live only once an + adapter pins a real merge-result digest (E10-S03/S07 on a merge train or merge queue). What + closes the loophole in the meantime is not the schema — it is the conformance case named + below, which is why that case had to be rewritten. 3. **There is therefore no gap-selection problem to solve.** The field is not a general gap channel that eleven candidates compete for; it is reserved for one capability, and the remaining ten have a different home. No priority order is needed, which is the desirable @@ -327,7 +410,13 @@ the arming-refusal reason surfaced to the operator. Both are unfrozen internal s | Case id | Test name | Proves | | --- | --- | --- | -| `record-schema-unchanged-under-multi-gap` | `TestConformanceMultiGapRecordStillValidates` | a run with several `absent`/`unknown` capabilities still emits a record that validates against the frozen schema, and the doctor report carries all of them | +| `capability-gap-string-is-merge-result-only` | `TestConformanceCapabilityGapCarriesOnlyMergeResult` | a run with **N** additional `absent`/`unknown` capabilities emits a `pins.capabilityGap` **byte-identical** to the same run with none of them, while the doctor report carries all N. This is the case that reddens on the comma-join adapter | +| `capability-gap-positive-control` | `TestConformanceCapabilityGapStillRecordsMergeResult` | **positive control**: the merge-result gap itself *is* still recorded, so the case above is not satisfiable by emitting an empty string | + +The case this replaces — "the record still validates against the frozen schema" — was +**unfailable**, and the reason is worth keeping: `code`, `cmd/assent/run.go:394` already +validates every record against the schema before any write, so that clause is enforced +unconditionally by production code, and a comma-joining adapter would have *passed* it. --- @@ -339,7 +428,8 @@ means *absent*, never *forbidden*. `forge.ErrUnauthorized` is lifted to the port `forge.ErrNotFound` is already neutral at `internal/forge/port.go:34`). The precedent to stay consistent with is **AUD2-S02 / REL-03**: `code`: -`cmd/assent/provider_host.go:84` and `:276-283` — the fallback is gated on +`cmd/assent/provider_host.go:84` and `:299` (`:276-283` is that function's doc comment, which +explains the gate; `:299` is the gate) — the fallback is gated on `errors.Is(err, forge.ErrNotFound)` **alone**, so a 503, a throttle or a token scoped away from the repo can no longer masquerade as an absent file. This story extends the same discrimination one layer down, into the adapter that mints the sentinel. @@ -348,7 +438,7 @@ discrimination one layer down, into the adapter that mints the sentinel. | Status | Sentinel | Note | | --- | --- | --- | -| 200 | content | `code`: `gitlab.go:486` region | +| 200 | content | `code`: `gitlab.go:484` (the 404 → `ErrNotFound` branch is `:486`) | | 401 / 403 | `ErrUnauthorized` | `code`: `gitlab.go:238`, `:792` | | 404 | `forge.ErrNotFound` (**absent**) | safe: GitLab distinguishes 403 from 404 for files | | 429 / 5xx | transport error | retry/backoff per AUD-S11; never a sentinel | @@ -387,7 +477,11 @@ sibling content read in the same repo at the same ref — the root listing, *there*, and only then is a 404 on the specific path genuine absence. The probe is cacheable per `(repo, ref)` for the run, so it costs one request, not one per governed read. It also fails in the right direction on an unresolvable ref: a bad `ref` 404s the root listing too, and -"the ref does not exist" is correctly not path absence. (`unverified` — the exact Contents API +"the ref does not exist" is correctly not path absence. **Diagnostic caveat for S06:** that +case is *safe* but its operator-facing message would be wrong — an unresolvable ref is not a +permission failure, so the adapter must word the error as "content unreadable at ref X: +absent and forbidden could not be distinguished", never as a bare authorization message. The +sentinel choice is fail-closed either way; only the wording is at stake. (`unverified` — the exact Contents API behaviour for the root-listing form is an E10-S06 live check; until confirmed, the residual rule below applies and the adapter errors.) @@ -412,12 +506,13 @@ covered by a row in this table takes the residual rule. | # | Owed by | Obligation | | --- | --- | --- | -| 1 | S01 | the ten case ids above appear in `catalog.yaml` with an adapter disposition (S01-03's strict-decode adapter list) | -| 2 | S02 | two call sites migrate, six do not; `forge.ErrUnauthorized` lifted to the port; ADR-0021's stale anchors (`provider_host.go:275→:292`, `:246→:263`, `Describe→GetMR`) absorbed | +| 1 | S01 | the **twelve** case ids above appear in `catalog.yaml` with an adapter disposition (S01-03's strict-decode adapter list). **Now enforced, not merely requested**: this obligation is written into E10-S01's DoD in `spec.md`, and S07/S14's DoDs name the cases they must run — the `Verify:` lines otherwise depend on an implementer voluntarily reading a planning doc. `codeowners-eligible-set-matches-forge` is a **candidate**, not minted: it is OQ-34's promotion route | +| 2 | S02 | two call sites migrate, six do not; `forge.ErrUnauthorized` lifted to the port; **ADR-0021's two** stale anchors absorbed (`provider_host.go:275→:292`, `Describe→GetMR`). The third, `provider_host.go:246→:263`, lived in E10-S02's own DoD (`spec.md:257`) and is **already fixed** by this change | | 2b | S02 | **resolve `FileAtBase(mr, path)`'s missing project binding before freezing the signature.** `FileAtRef` takes `project` as a parameter and today's client binds none, so ADR-0021's two-argument shape is under-specified: either the client binds the *target* project at construction (and the accessor is genuinely MR-relative) or `mr` becomes composite. Flagged here because a signature that freezes wrong is precisely what S00 exists to prevent; the choice is S02's, the ambiguity is not S02's to discover | | 3 | S04 | the arming-relevant capability subset is stated explicitly; every `C` constant names its licensing case | | 4 | S04 / S09 | retiring the `@` heuristic (and any other capability promoted into the arming set) is a **user-visible GitLab behaviour change** → its own `D-nnn` row + changelog entry, per epic judgment call (e). Not minted by S00, which changes no behaviour | -| 5 | operator | **OQ-33** — `protected-pipeline-source` predicate; until answered, v1 GitHub does not arm | +| 5 | operator | **OQ-33** — `protected-pipeline-source` predicate (three candidate routes, one of them §4's own largely-forge-readable same-repo route); until answered, v1 GitHub does not arm | +| 6 | operator / S08 | **OQ-34** — whether an adapter-computed CODEOWNERS eligible set can be `full`. Until answered, GitHub row 9 is `unknown` and **GitHub does not satisfy `require-review`**, the same standing as GitLab Free. With rows 9 and 11 both `unknown`, **v1 GitHub comments and does not gate** | ## Claims deliberately not resurrected diff --git a/docs/planning/open-questions.md b/docs/planning/open-questions.md index 4d63485..ae0ed93 100644 --- a/docs/planning/open-questions.md +++ b/docs/planning/open-questions.md @@ -34,4 +34,5 @@ | OQ-30 | **RESOLVED (D-148, 2026-08-16): (b) — keep the guard skipped on `pull_request`; the real mechanism (merge-direction-dependent ordering hazard) is now recorded in D-125/D-136.** Analysis retained below for the record. Original text: Is a `pull_request`-scoped CHANGELOG drift gate viable now that D-136 skips merge commits? The guard is retained with NO demonstrated reason — its original one is dead and its proposed successor measures false.** D-125 skipped the gate on `pull_request` because `refs/pull/N/merge`'s synthetic merge subject rendered into the generated changelog, so no committed `CHANGELOG.md` could match. **D-136 killed that reason** — that commit is a merge commit and is now skipped. The successor reason drafted in D-136's first version — "the merge ref also carries every commit landed on `main` since the branch forked, so the render is a union the branch's file cannot match, red by construction" — was then measured four ways and **could not be made true**: (1) PR #41's **live** `refs/pull/41/merge` (`491bb2a`, head `49eebb3` into base `7513d79`) rendered with the new `cliff.toml` → `verify-changelog: ok`, **0 diff lines**; (2) the direct counterexample — the same head merged into a `main` that **had** moved (`1d8aa60`, containing PR #40) → `verify-changelog: ok`, **0 diff lines**, i.e. not red with the base moved; (3) a synthetic sandbox where base and lane each add a commit to the **same** cliff group and each regenerate → `CONFLICT (content): Merge conflict in CHANGELOG.md`, so the PR is unmergeable, GitHub mints no merge ref, and the gate never runs. (4) **The strongest one, taken last and re-run rather than transcribed:** GitHub RE-MINTED `refs/pull/41/merge` against the moved base after all of the above. Re-fetched live — `7715bf7`, head `ee5e527` into base `1d8aa60` — and put through the real gate script: **`verify-changelog: ok`, 0 diff lines, 0 merge subjects rendered.** That is not a simulation: it is the exact artifact a `pull_request`-scoped gate would evaluate, with the base moved past the fork point AND after the lane had merged `main` in — the direction the finding below shows is hazardous — and it is green. Measurement (1)'s `491bb2a` at base `7513d79` is its stale predecessor, kept only to show the result did not depend on the base standing still. **Mechanism the dead premise overlooked:** the merge ref's `CHANGELOG.md` is not "the branch's committed file" — it is the three-way MERGE RESULT, which already contains the base's lines, because the file is merged like any other. So base movement ends in clean-and-matching or conflict-and-no-merge-ref. **The third outcome EXISTS, and merge DIRECTION decides it — measured while writing this row.** A clean textual auto-merge whose line order differs from git-cliff's topological order is red with no author error, and it reproduced immediately: merging `origin/main` **into the lane** (lane as first parent) auto-merged `CHANGELOG.md` without conflict and then failed `verify-changelog` on pure ordering — one `docs(compare)` line moved and PR #40's lines landed in a different position. The SAME two commits merged in the **merge-ref direction** (base `1d8aa60` as first parent, measurement (2) above) matched exactly. git-cliff's traversal follows parent order, so first-parent choice changes the render. This does not revive the retired premise — GitHub always mints the merge ref base-first, which is the direction that matched — but it means the clean-and-matching outcome is a property of that direction, measured on two merges, not a proof. It also re-confirms D-125's surviving rule: regenerate after any `git merge origin/main`. **Still untested:** behaviour on `pull_request_target`, on a PR from a fork, and after a force-push that re-mints the merge ref. **Counter-evidence for enabling it:** the only red reproduced on any merge ref was a branch that had not run `task changelog-write` for its own commits — a **true positive the gate exists to catch**, which argues the PR placement may now be correct rather than merely harmless. **Correction, folded in from the PR #41 review because it belongs in the row and not only in a review thread:** that review first read these greens as "the evidence points toward the PR gate being viable", and then took it back as **one measurement short**. The direction finding above supplies a false-positive mechanism it had not considered — a clean textual auto-merge whose line order differs from git-cliff's topological order reds with no author error and no author fix available. Four green measurements are therefore NOT a green light; on today's evidence the gate would not be enabled. **Ruling needed (deliberately not taken here, operator's call):** (a) enable the step on `pull_request` and delete the guard; (b) keep the guard and record the real reason once someone finds one; or (c) keep the guard permanently on cost/noise grounds and say so, rather than on a mechanism. Not to be resolved by deleting the guard on the strength of these three measurements alone — they show the claimed failure did not reproduce, not that no failure exists. | Before any change to the `pull_request` guard on the changelog step in `.github/workflows/verify.yaml`; not a release blocker — the guard is fail-safe (the gate runs locally in `task check` and on push-to-main) | Raised by the PR #41 review (finding CL-02) against D-136's first draft; measurements reproduced independently before recording. Sites now pointing here: `Taskfile.yml` `check:`, `.github/workflows/verify.yaml`, `hack/release/README.md`, `hack/release/changelog_gate_test.sh` §3. See [D-125](../decisions/decisions.md) and [D-136](../decisions/decisions.md) | | OQ-31 | **RESOLVED (D-146, 2026-08-16): (a) — "zero forge writes" stays absolute; the BLOCK is surfaced via a required CI job status reading the already-emitted `DecisionRecord`, not via a forge write.** Analysis retained below for the record. Original text: May the GUARD-1 self-edit BLOCK path write a summary or supersession note, or is "zero forge writes on a self-modifying MR" absolute? If it is absolute, what channel carries the BLOCK to the human reviewer — given that no thread is posted and the exit code is `0`?** Raised by RELI-01 ([D-138](../decisions/decisions.md)) and deliberately left **UNDECIDED**. The tension is real in both directions. **For absolute:** `openspec/specs/p5-aud-audit-remediation/spec.md` pins *"the decision is BLOCK with zero forge writes (GUARD-1 dominance over the gap-degrade)"* as a frozen acceptance criterion, and the guard exists so that an MR editing `.assent/**` cannot make assent vouch for its own policy — any write is a write the MR's own content influenced. **Against absolute:** the only human-visible surface then keeps whatever the previous run said, which today can be `✅ Decision: APPROVE`, so the guard's output is *invisible* to the reviewer it protects, and D-130's compensating control (a REVIEW rerun upserts the summary and adds an unresolved discussion) does not reach this path because no thread is posted. Zero *authority* writes need not mean zero *communication*. **Options, none taken here:** (a) keep it absolute and carry BLOCK on a non-forge channel — a non-zero exit code, or a required CI job status; (b) permit exactly one write, a fixed-text supersession/BLOCK note with no policy-derived content, which cannot be steered by the MR; (c) permit the summary upsert but not the thread. (b) and (c) both **reopen the frozen criterion above and need an openspec change proposal first** — spec before code. Note that (a) changes an exit-code contract wrapper scripts rely on ([`docs/usage/cli.md`](../usage/cli.md)), so it is not the free option it looks like. | Before the RELI-01 fix lands (v0.2.1) | Found by the 2026-08-09 audit's reliability lens; recorded in [D-138](../decisions/decisions.md). Evidence: `cmd/assent/run.go` step-9 GUARD switch, `openspec/specs/p5-aud-audit-remediation/spec.md`, `openspec/specs/p5-e5-provider-host/spec.md` REQ-E5-S08-03 | | OQ-32 | **RESOLVED (D-147, 2026-08-16): (b) — add a host-side secret resolver (process env / file path / hosted store); repo-side config gains only an opaque, host-allowlisted reference name, never a literal credential or URL pairing. ADR amending ADR-0015 §7 required before code.** Analysis retained below for the record. Original text: No provider transport can carry a credential, so NO provider can call Entra ID, Keycloak, or any token-authenticated IdP directly — and nothing says so. Found while designing P5-DEM (D-142). Verified across three surfaces that agree: `CallHTTP` (`internal/provider/transport.go`) sets **only** `Content-Type: application/json` — no header map, no bearer token, no client certificate; the repo-side provider schema (`schemas/policy/v1alpha1/config.schema.json` `$defs/provider`) is `additionalProperties:false` over exactly `{type, url, failure}`, so there is nowhere to put one; and `ScrubEnv`/`ScrubArgv` build the exec child's environment **from scratch** and refuse any name matching `(?i)(TOKEN\|SECRET)` **even when explicitly configured**, so the exec tier cannot carry one either. **This is not a bug — it is ADR-0015 §7 working exactly as designed**, and Spike C's `TestIsolation` proves it against a deliberately hostile provider that exfiltrates its whole environment and stdin. **What has never been written down is the consequence:** Entra ID and Keycloak both require a bearer token on every call, so the only shape that works today is a **broker** — a service holding the IdP credential *itself*, reachable by assent without one (loopback/sidecar, or mTLS terminated outside assent's transport). That is arguably the *correct* architecture: the credential never enters the decision path and a compromised provider's blast radius stays bounded. But it is undocumented, and it **narrows what `docs/vision.md:67` promises** ("pluggable providers: Keycloak, LDAP, GitLab/GitHub groups, ownership files, custom plugins") and what ADR-0004 §1 planned ("OIDC/Keycloak group lookup, LDAP" as builtins — never shipped). `docs/architecture/c4-context.md:19` is currently the **only** place stating the truth: *"Keycloak / LDAP: no builtin — reachable only via the generic HTTP/exec provider transport."* **Ruling needed (deliberately not taken):** **(a)** bless the broker pattern, document it in the provider-author guide, and amend `docs/vision.md:67` + ADR-0004 §1 to stop implying direct IdP calls — **RECOMMENDED**: costs nothing, keeps ADR-0015 §7 and the isolation proof intact, and is what DEM-S02/DEM-S03 are already written against; **(b)** add a narrow repo-side credential channel (header or secret-ref) to the HTTP transport — **reopens a frozen schema AND the trust boundary the hostile-provider isolation proof rests on**, and would need its own ADR; **(c)** state the limitation and add nothing. Note this is **not** merely a docs question under (a): a reader of the vision page today would reasonably budget a Keycloak integration as "configure a builtin" and discover mid-implementation that they must also deploy and operate a broker. **Not to be resolved by quietly adding a header field** — that is option (b) and it is a trust-boundary change. | Before DEM-S02 publishes the provider-author guide (the guide must state one of these answers); **not** a release blocker — the current behaviour is fail-safe, just undocumented | Found designing [P5-DEM](https://github.com/PlatformRelay/assent/blob/main/openspec/specs/p5-dem-demo-repos/spec.md) (judgment call (e)); recorded in [D-142](../decisions/decisions.md). Evidence: `internal/provider/transport.go` `CallHTTP`/`ScrubEnv`/`ScrubArgv`, `schemas/policy/v1alpha1/config.schema.json` `$defs/provider`, `docs/planning/spikes/spike-c-provider.md` § Isolation evidence, ADR-0015 §7, ADR-0004 §1, `docs/vision.md:67`, `docs/architecture/c4-context.md:19` | -| OQ-33 | **`protected-pipeline-source` (ADR-0015 §4's arming prerequisite) has no operationally decidable predicate on EITHER forge.** On GitLab it is a substring heuristic — `strings.Contains(proj.CIConfigPath, "@")` (`internal/forge/gitlab/snapshot.go:292`), the SEC-04 shape ADR-0021 forbids by name — which proves neither that the referenced CI config sits on a protected branch nor that the MR author cannot push to it; on GitHub there is no single readable analogue. E10-S00 proposes a **composite** predicate spanning the CI-env adapter (the run's trigger event is one whose workflow definition GitHub loads from the base repo's default branch — `pull_request_target` / `workflow_run` / `merge_group`) **and** a forge read (that default branch requires reviews, so the definition is not author-editable). Two sub-questions: **(a)** does ADR-0015 §4 accept an env+forge composite as proof of protected config, or must the predicate be forge-readable only? **(b)** if not, does v1 ship with arming unavailable — comment-only on GitHub, and on GitLab too once the heuristic is retired? | E10-S04 / S09 / S11 (arming); v1 GitHub gating | [github-addressing-model.md](github-addressing-model.md) Q2 row 11; ADR-0021 items 3+5–8 and Consequences; audit 2026-08-09 SEC-04. **Leading answer: (a)** — accept the composite, gated by a conformance case and reported as doctor evidence; the alternatives are a heuristic (rejected) or no gate at all. Retiring the `@` heuristic is a user-visible GitLab behaviour change owed its own D-row at S04/S09 (E10 judgment call (e)) | +| OQ-33 | **`protected-pipeline-source` (ADR-0015 §4's arming prerequisite) has no operationally decidable predicate on EITHER forge — which of three candidate routes does §4 accept as a PROBE?** Today's GitLab value is a substring heuristic — `strings.Contains(proj.CIConfigPath, "@")` (`internal/forge/gitlab/snapshot.go:292`), the SEC-04 shape ADR-0021 forbids by name — which proves neither that the referenced CI config sits on a protected branch nor that the MR author cannot push to it. **ADR-0015 §4 already frames the GitHub condition in composite terms** and says it is "verified by `assent doctor`": *"GitHub: workflows from the target branch (`pull_request` runs the base-ref workflow for forks; same-repo branches need branch protection on workflow paths)"*. Three candidates: **(1) same-repo route** — §4's own second clause: branch protection or a path-restricted ruleset over `.github/workflows/**` on the base branch; largely **forge-readable**, so the closest analogue to a GitLab-style forge-only predicate (its path-scoped-ruleset availability is unverified). **(2) fork route** — §4's first clause: `pull_request` from a fork runs the base-ref workflow with a read-only, secretless token (dossier C17); safe but **advisory-only** per ADR-0015 §8, so a non-arming state rather than a route to `supported`. **(3) composite env+forge route** — the trigger event is one whose definition GitHub loads from the base repo's default branch (`pull_request_target` / `workflow_run` / `merge_group`) **and** that branch requires reviews; spans `cmd/assent`'s CI-env adapter and `internal/forge`, which §4 did not anticipate being asked to probe. Sub-questions: **(a)** does §4 accept route 1 alone as the GitHub predicate? **(b)** if not, does it accept the env+forge composite (route 3), or must the predicate be forge-readable only? **(c)** if neither, does v1 ship with arming unavailable — comment-only on GitHub, and on GitLab too once the heuristic is retired? | E10-S04 / S09 / S11 (arming); v1 GitHub gating | [github-addressing-model.md](github-addressing-model.md) Q2 row 11; ADR-0015 §4/§8; ADR-0021 item 3 and Consequences; audit 2026-08-09 SEC-04. **Leading answer: (a) if route 1 is readable on ordinary repos, else (b)** — the alternatives are a heuristic (rejected) or no gate at all. Retiring the `@` heuristic is a user-visible GitLab behaviour change owed its own D-row at S04/S09 (E10 judgment call (e)) | +| OQ-34 | **Can an ADAPTER-COMPUTED CODEOWNERS eligible set ever be `full` under ADR-0017 §3's "typed eligible principals", and what proof would license it?** §3 admits "CODEOWNERS evidence" explicitly and the frozen schema enumerates `codeowners` (`schemas/approval/v1alpha1/approval-evidence.schema.json:40-42`), so the ROUTE is legitimate — the open question is the **set-equality** property. GitLab's `eligible_approvers[]` is forge-computed; a GitHub CODEOWNERS set is adapter-computed from forge-supplied bytes, and no GitHub API returns the computed per-PR eligible code owners (dossier §2 step (b), which grades this `partial`). An over-permissive matcher (first- vs last-match-wins, sections, negation, case-folding, a team read returning a superset of write-holders) puts a principal into `ev.Eligibility` that GitHub would not accept; `approvalSatisfies` then records the obligation **satisfied** (`internal/core/aggregate/approval.go:127`) — harm that needs no arming, because the DecisionRecord itself asserts a governance obligation was met by an ineligible principal. GitHub's own `require_code_owner_reviews` is not a backstop: it enforces over the PR's changed files, while assent's eligible set is scoped to the governed subject. Candidate licences: **(1)** a fixture-corpus fidelity case `codeowners-eligible-set-matches-forge` + a positive control that reddens on a deliberately over-permissive matcher — proves fidelity to the spec, not equality with the forge; **(2)** a live cross-check against GitHub's own code-owner determination — scoped to changed files and mutable after PR open, so corroboration rather than authority; **(3)** accept `unknown` permanently and ship GitHub comment-only for require-review. | E10-S08 / S14; whether GitHub can ever satisfy `require-review` | [github-addressing-model.md](github-addressing-model.md) Q2 row 9; ADR-0017 §3; [forge-dossier-github.md](forge-dossier-github.md) §2; ADR-0021 Consequences (which predicted exactly this `unknown`). **Leading answer: (1)**, with row 9 staying `unknown` until the case and its positive control are green — fail-closed meanwhile, per this project's thesis | diff --git a/openspec/specs/backlog.md b/openspec/specs/backlog.md index 42cd265..942aea2 100644 --- a/openspec/specs/backlog.md +++ b/openspec/specs/backlog.md @@ -559,7 +559,7 @@ them rather than auto-merge. | ID | Story | Execution | Depends on | Gate contribution | | --- | --- | --- | --- | --- | -| E10-S00 | **DONE** (D-155) — ⚠️ GitHub addressing & representation model: [github-addressing-model.md](../../docs/planning/github-addressing-model.md) answers all four questions, mints 10 conformance case IDs (each paired with a positive control), corrects **3 stale ADR-0021 anchors** (`provider_host.go:275→:292` — the D-130 registry — `:246→:263`, `Describe→GetMR`), and raises **OQ-33**: `protected-pipeline-source` has no decidable predicate on **either** forge, so `unknown ⇒ never arm` leaves v1 GitHub comment-only until it is answered. `git diff schemas/ == 0` holds — no schema change proposed. **Awaiting maintainer LGTM** | **[autonomous · design · LGTM]** | none | **do first** — kills both P0s before the port freezes | +| E10-S00 | **DONE** (D-155) — ⚠️ GitHub addressing & representation model: [github-addressing-model.md](../../docs/planning/github-addressing-model.md) answers all four questions, mints **12** conformance case IDs (each fail-closed case paired with a positive control) and **wires them into S01/S07/S14's DoD** rather than leaving them in a doc nothing reads, corrects **2 stale ADR-0021 anchors** (`provider_host.go:275→:292` — the D-130 registry — and `Describe→GetMR`) plus **1 in this epic's own spec** (`spec.md:257`, `provider_host.go:246→:263`, fixed here), and raises **OQ-33** (`protected-pipeline-source`) + **OQ-34** (adapter-computed CODEOWNERS eligibility). Both arming-relevant GitHub capabilities land `unknown`, so **v1 GitHub comments and does not gate** — fail-closed and stated up front. `git diff schemas/ == 0` holds — no schema change proposed. **Awaiting maintainer LGTM** | **[autonomous · design · LGTM]** | none | **do first** — kills both P0s before the port freezes | | E10-S01 | Extract the conformance suite into an importable package + observation surface | **[autonomous]** | S00 | first **code** story; no assertion may be weakened | | E10-S02 | ⚠️ `forge.RunPort` + **neutral factory** + MR-relative addressing + identity | **[autonomous · engine-grade · LGTM]** | S00, S01 | one neutral seam; ARCH-02 cannot recur | | E10-S03 | ⚠️ Collapse `SyntheticDigest` onto `Snapshot.Heads.MergeResultDigest` | **[autonomous · engine-grade]** | S02 | digest scheme adapter-owned; allowlist emptied | diff --git a/openspec/specs/p5-e10-github-forge/spec.md b/openspec/specs/p5-e10-github-forge/spec.md index a539c12..643d955 100644 --- a/openspec/specs/p5-e10-github-forge/spec.md +++ b/openspec/specs/p5-e10-github-forge/spec.md @@ -209,7 +209,11 @@ runs it. - **Dependencies**: S00. - **Definition of done**: case bodies live in importable Go; `go test ./internal/forge/...` passes with **no case deleted, renamed, or weakened**; the GitLab entry point is a thin - `_test.go` calling the shared runner. + `_test.go` calling the shared runner; **and every case id minted by S00's model + (`docs/planning/github-addressing-model.md`, "Conformance cases" tables) exists as a + `catalog.yaml` row with an explicit adapter disposition** — S00's `Verify:` lines ("the named + conformance case appears in S01's catalog") are otherwise satisfied only by an implementer + voluntarily reading a planning doc, which this repo has already learned does not happen. - **The tension to resolve deliberately, not cheaply**: the existing cases assert on `*fake.Forge` internals — `sha_guard_test.go:49` takes `*fake.Forge`, and `reconciliation_test.go:220` type-asserts to it — reading recorded writes (`Merges`, @@ -254,7 +258,7 @@ runs it. - **Dependencies**: S00, S01 (so the port change is proven by an executable suite). - **Definition of done**: `forge.RunPort` declared in `internal/forge`; **both** of `cmd/assent`'s port declarations retired — `run.go:64 forgePort` (the anonymous literal at - the call site) **and** `provider_host.go:246 refFilePort`, a second, hand-rolled + the call site) **and** `provider_host.go:263 refFilePort`, a second, hand-rolled `FileAtRef`-only interface. Naming only the first is how this story closes while `cmd/assent` still depends on a private port: replacing `forgePort` alone leaves `refFilePort` standing, the DoD reads satisfied, and `go build` + `task lint` stay green. @@ -491,7 +495,12 @@ runs it. - **Dependencies**: S06. - **Definition of done**: PR metadata → `forge.MRInfo` (head/base SHAs, fork detection), changed-file enumeration satisfying **ADR-0020 completeness** (truncation is an opaque - enumeration failure, never a short list), and merge-result pinning via `refs/pull/N/merge`. + enumeration failure, never a short list), and merge-result pinning via `refs/pull/N/merge`; + **plus S00's addressing and sentinel cases green against the GitHub factory** — + `fork-head-unchanged-file-no-lifecycle` + its positive control + `fork-head-genuine-delete-detected`, and `forbidden-never-renders-as-absent`, + `absent-file-still-renders-as-absent`, `metadata-only-token-is-not-absence`, + `ratelimit-403-is-transport-error` (see `docs/planning/github-addressing-model.md` Q1/Q4). - **REQ-E10-S07-01** — Given `forge.MRInfo`'s contract, when a PR is described, then `SourceSHA` is the PR head, `TargetSHA` is the **base branch tip** (not the merge base), @@ -686,7 +695,10 @@ runs it. - **Dependencies**: S13. - **Definition of done**: the S01 suite runs against the GitHub factory in CI; every `github-deferred` row in `catalog.yaml` is either flipped to `both` or **retains the - deferral with a named, cited reason**; D-084 is dispositioned. + deferral with a named, cited reason**; D-084 is dispositioned; **and every S00-minted case id + is dispositioned like any other row** — executed on both adapters, or deferred with a cited + reason. A capability whose `C` (contract-proven) constant has no passing case here must be + reported `unknown`, per S00's Q2 rule. - **REQ-E10-S14-01** — Given **every one of the 14 non-deferred catalog rows is `forge: gitlab`**, when the catalog is updated, then **every row** — not only From d9c2b6f4f1dec33bfd292dcfe5879dd2cfee4b7d Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Mon, 24 Aug 2026 14:24:02 +0200 Subject: [PATCH 3/3] :memo: docs(e10): record the operator LGTM that satisfies E10-S00's GOVERNANCE marker (D-161) --- CHANGELOG.md | 1 + docs/decisions/decisions.md | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a13d3dc..f0bff03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,7 @@ repository still gets a decision, never by following the link; no release carrie - :memo: docs(release): correct the residual the gate documents (R3-01) - :memo: docs(ci): withdraw three falsifiable claims from the PR-reach reader (D-157) - :memo: docs(ci): withdraw the third copy of the retracted claim (D-157) +- :memo: docs(e10): record the operator LGTM that satisfies E10-S00's GOVERNANCE marker (D-161) ### Features - :sparkles: feat(audit): measure REL-03/REL-07 by real mutation run, not by name diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index 8c439db..83732db 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -165,3 +165,4 @@ project/process decisions. | D-158 | 2026-08-23 | **CI-TOOLCHAIN — `verify` was red on every PR from a golangci-lint/Go skew; the remedy is to bump `GOLANGCI_LINT_VERSION` v2.12.2 → v2.13.1 and KEEP `go-version: stable`.** Symptom: PR #85, a six-file Markdown diff with **zero Go bytes**, failed `verify` with `/opt/hostedtoolcache/go/1.27.0/x64/src/crypto/internal/randutil/randutil.go:11:2: could not import math/rand/v2 (… method must have no type parameters) (typecheck)` — the failing path is inside the **Go toolchain's own source tree**, so no author change can fix it and re-running cannot clear it. `Analyze (go)`, `Analyze (actions)` and `CodeQL` all passed on the same run: the tree compiles clean under Go 1.27, only the linter chokes. **Mechanism**: golangci-lint typechecks the standard library with the `go/types` it was **compiled with**, not the `go` on `PATH`. GitHub's `stable` rolled 1.26 → 1.27.0 between this repo's last green `main` run (2026-08-19) and 2026-08-23; the v2.12.2 release binary is built with go1.26.x and cannot read the 1.27 stdlib. **Reproduced locally, not reasoned** (Go 1.27.0 SDK on `PATH`, `GOTOOLCHAIN=local`, whole tree): v2.12.2 → **panic** in `pkg/goanalysis/runner_loadingpackage.go:335` inside `go/types.(*Checker).Files`, exit 2; official v2.13.1 asset (`built with go1.27.0`) → **0 issues**, exit 0; `golangci-lint config verify` → exit 0, so the v2 config schema in `.golangci.yml` needs no migration; v2.13.1 also → **0 issues** against local Go 1.26.6, so the bump is safe for contributors who have not moved; and `go install …@v2.13.1` under Go 1.27.0 → 0 issues, which is the **`release-exitgate`** path (that job builds the binary rather than using the action, and `if: github.event_name != 'pull_request'` / RELSE-08 means no PR ever exercises it). **Diagnosis heuristic worth keeping**: golangci-lint's `go.mod` carries the comment *"the minimum Go version must always be latest-1"*, so its `go` directive is a machine-readable "supports Go 1.N+1" signal — v2.12.2 says `go 1.25.0` (supports ≤1.26), v2.13.1 says `go 1.26.0` (supports ≤1.27), and upstream commit `42a0530` in v2.13.0 is literally "go1.27 support". **Options considered.** (A) *bump the linter* — CHOSEN: treats the root cause, is a **one-line** diff because the version is single-sourced at workflow scope (D-128), fixes BOTH exposed sites (the `golangci-lint-action` in `verify` and the `go install` in `release-exitgate`) at once, and the risk it carried — new findings from a newer linter blocking every merge, since `golangci-lint` runs inside `task lint` inside `task check` — was **measured at zero** on the whole tree under both Go versions. (B) *pin `go-version: 1.26.x`* — REJECTED as the primary: it needs two pins (`verify.yaml:56` and `:171`), leaves the class of bug live, undercuts the stated reason `stable` is there (`# govulncheck needs it`, with `setup-go` exporting `GOTOOLCHAIN=local` so a pinned toolchain can fail to `go run` a tool whose module requires a newer Go — unverified, and moot under (A)), and trades a loud recurrence for **silent indefinite drift onto an unsupported Go** in a repo whose posture is govulncheck + cosign + SLSA + Scorecard. (C) *both* — REJECTED as gold-plating: (A) alone is measured green, and a second simultaneous remedy would make a still-red PR ambiguous. **Per-file sweep of `go-version: stable`, stated rather than silently skipped:** `verify.yaml:56` and `:171` were the only exposed sites and are fixed by the env bump without touching either line; `schemas.yml:57` (runs `go test` + ajv, no linter), `vulncheck.yaml:30` (govulncheck only — `stable` is actively *desirable* there) and `codeql.yaml:45` (CodeQL, which passed on 1.27.0) carry no golangci-lint and are NOT exposed; `release.yaml` uses `go-version-file: go.mod` and is unaffected. `hack/lint/workflow_pins_test.sh` asserts `TASK_VERSION`, action SHA pins and step wiring but **not** `go-version` or `GOLANGCI_LINT_VERSION`; it passes on this change. **Revert is (B), not bare v2.12.2** — reverting the env line alone restores a known-red state: revert = `GOLANGCI_LINT_VERSION` back to `v2.12.2` **and** pin `verify.yaml:56` + `:171` to `go-version: 1.26.x`. **Residual risk, stated plainly:** when `stable` rolls to Go 1.28, v2.13.1 breaks identically. The repo finds out because `verify` reds on **every** PR including zero-Go diffs — loud and immediate, with the signature recorded in `verify.yaml`'s env comment and the fix being one env line. Nothing detects it *earlier*: dependabot's `github-actions` ecosystem updates `uses:` refs, not `env:` literals (backlog `CI-TOOLCHAIN-F01`). Accepted on purpose over (B)'s failure mode. Note also the pre-existing local/CI linter skew — `Taskfile.yml`'s `lint` task is bare `golangci-lint run ./...` against whatever binary a contributor has installed, so this pin has never governed local `task check`; deliberately not changed here (that file was owned by a concurrent lane). | | D-159 | 2026-08-23 | **ORPHAN — `release-verify-test` was a gate invoked by nothing (D-124 again), and it is wired into `task check` in its three OFFLINE modes, not its default `all`.** Finding: `Taskfile.yml` defined `release-verify-test` — the **only** test of `hack/release/verify-artifacts.sh` — and no caller existed anywhere: not `check:`, not `CHECK_STAGES` in `hack/audit/exitgate_test.sh`, not `.github/workflows/**`, not the push-only `release-exitgate`. That script is not incidental: **`SECURITY.md:61` publishes it as the procedure release consumers run** to check artifact checksums, the stamped `assent version` and cosign signatures, and D-153/AUD2-F01 had just proved that published verification instructions rot silently. Measured before fixing, as the lane required: the gate was **green**, so this is a wiring defect only — nothing was being papered over. **Why not the `all` default.** `verify_test.sh` with no argument adds `test_snapshot_pass`, which shells out to `task release-snapshot`: `go install github.com/goreleaser/goreleaser/v2@…` on any machine without it (**network**, in a gate that runs before every commit), a `before: hooks: go mod tidy` that **rewrites `go.mod`/`go.sum` in the middle of `task check`**, `--clean` on the developer's `dist/`, and a five-target cross-compile (10s here with warm caches and goreleaser already installed; minutes cold). A flaky or network-bound stage is worse than a missing one, so `check:` runs `negative`, `cosign-skip-when-absent` and `readme` — REQ-E9-S12-02/03/04, all offline, together well under a second. **Consequence, stated precisely: REQ-E9-S12-01 (the real snapshot→verify round trip) is not gated per-commit in `task check`; it runs in push-only CI via `hack/release/exitgate_test.sh:33-34` (the `release-exitgate` job, `if: github.event_name != 'pull_request'`) — automated but not PR-visible, i.e. the RELSE-08 blind spot.** Those two lines are literally `task release-snapshot` then `task release-verify`, which is the `Verify:` string the spec gives the requirement (`openspec/specs/p5-e9-distribution/spec.md:441-442`), so nothing here needs a new gate built — an earlier draft of this row claimed the requirement was covered by no automation at all, which was false, and acting on it would have meant a future lane duplicating `exitgate_test.sh:33-34`. What this lane declines to do is move that L1 round trip into a per-commit gate: it cannot be, for the network/`go mod tidy`/`--clean` reasons above. Narrowing the RELSE-08 exposure for this requirement is a separate question from D-159. **Pinning.** `CHECK_STAGES` goes 20 → 21 in the same commit as the `check:` line (the AUD-S18/RELSE-08 lockstep), and `STAGE_BODY_PINS` pins the body **per mode** rather than by the bare script path — a single `bash hack/release/verify_test.sh` pin would stay green if two modes were dropped *and* if the stage were reverted to the unrunnable-in-check default. Three mutation controls prove it: deleting the `check:` line reds `check_check_wiring` by stage name, gutting one of the three modes reds `check_stage_bodies` while the other two still run, and the same deletion applied to the real tree reds the real gate at rc=1. **Also fixed, minimally, because wiring made it matter:** `verify_test.sh` wrote its captured stdout/stderr to fixed `/tmp/verify-{tamper,nosig}.{out,err}` and then **grepped** them; harmless while nothing ran it, but this repo runs several lane worktrees whose `task check` invocations overlap, and a concurrent run truncating `/tmp/verify-nosig.err` at open would fail the other run's `skip.*cosign` grep spuriously. Scratch files now live in each run's own `mktemp -d` (invisible to `verify-artifacts.sh`, which globs only `*.tar.gz`/`*.zip`) and are removed by the existing `RETURN` trap. | | D-160 | 2026-08-23 | **UNIV-COSIGN — the cosign signer-pin gate asserted an EXISTENTIAL property where the guarantee it publishes is UNIVERSAL, so it certified nothing about a file's second `cosign verify-blob`; `hack/release/install_cosign_pin_test.sh` now grades EVERY invocation in ALL THREE published files, statically and at runtime.** Substance first, and measured before anything was written: a scratch copy of each graded file with a **second, unpinned** `cosign verify-blob` spliced IN-BODY next to its real invocation (the splice shape matters — see UC-04 below) ran the gate to **exit 0** — `hack/install.sh` rc=0, `SECURITY.md` rc=0, `hack/release/verify-artifacts.sh` rc=0, the last of these being the shape a reviewer had already demonstrated on an earlier lane. **Every individual assertion in that gate was sound.** `has_flag` grepped the whole folded extraction, so a pinned sibling satisfied it; `one_value`'s `sort -u` collapsed two agreeing values to one, so the D-153 drift comparison saw a consistent file; section 0 **printed** the extracted invocation count and asserted nothing about it; and 4b/5d's stub-log checks were positive-only greps, i.e. the same existential shape at runtime, where a second unpinned call leaves the pinned call's log line intact. The conjunction of sound existential assertions is still an existential assertion. **This is the AUD2-S05 quorum defect one layer out: not a wrong assertion, a wrong quantifier**, and it predates both AUD2-S03 and AUD2-F01. **`SECURITY.md` was worse than the other two** — it fed only the drift comparison and had **no invocation-level grading at all**, so an unpinned recipe published to adopters was invisible to the gate that exists to police the recipe. **Property chosen: "every `cosign verify-blob` invocation in every graded file carries `--certificate-oidc-issuer`, `--certificate-identity-regexp` and `--bundle`, EACH WITH SECURITY.md's published value".** The value half is not decoration: `--certificate-identity-regexp ''` carries the flag and matches every Fulcio identity ever issued, and `one_value`'s `sort -u` does not catch it either, because an empty capture is not a line and the file still reports exactly one distinct value — a presence-only universal check would be the same defect one level down, so each invocation's own extracted value is compared against the published pair (reported `WRONG-VALUE[...]`, distinct from `UNPINNED[...]`). Options considered: **(a)** the remedy recorded when the finding was filed — assert the extracted invocation count is **exactly 1** per file (**REJECTED AS FACTUALLY WRONG — do not implement it in a later lane; it would red `main` on the day it lands.** `SECURITY.md` legitimately publishes **two** — one over the archive, one over `checksums.txt`, which is what covers the SBOMs listed inside it — so that assertion reds `main` on the day it lands, and the natural repair is to weaken it again); **(b)** universal quantification over invocations **(chosen)** — strictly stronger than (a) where (a) is true, and it keeps holding when a file legitimately grows a third correctly-pinned call. **Enforcement, per D-128, is one predicate**: `pin_violations ` prints one line per problem and nothing when the file is clean, so callers grade its OUTPUT and the identical function is asserted empty against the tree and non-empty against a mutant with no `set -e` games; `assert_all_pinned` is the thin failing wrapper; `log_unpinned_lines` is its runtime twin over the stub-cosign argv log. **Classification is fail-CLOSED, which is the part that carries the guarantee.** The candidate extractor is deliberately left un-narrowed (every non-comment line mentioning `cosign verify-blob`, continuations folded), because anything the *denominator* drops is a hole. A candidate is an invocation when the folded text begins with `cosign verify-blob`; the ONE waved-through non-invocation shape is a markdown inline-code mention (`` `cosign verify-blob` ``, which is how `SECURITY.md`'s capability table names it); **on a line carrying NO backticked mention, anything else — `foo && cosign verify-blob …`, a here-doc line, an un-backticked prose sentence — is reported UNCLASSIFIABLE rather than skipped**, because "this gate cannot tell whether that call is pinned" must never read as "that call is fine". **That scoping is not cosmetic, and the unscoped version of this sentence was FALSE (R3-01, caught by review on the round that added the honest banner).** The exemption tests whether the substring `` `cosign verify-blob` `` appears **anywhere on the line**, not whether the line is prose, and it is tested BEFORE the command-position and multi-token checks. So a line carrying a backticked mention AND one or more LIVE calls is exempted, neither graded nor refused: measured by splicing ``See `cosign verify-blob`: cosign verify-blob --bundle A.json a && cosign verify-blob --bundle EVIL.json evil`` into `SECURITY.md` — `occurrence_count` = 2 by the definition the banner itself states, `pin_violations` EMPTY, **whole gate rc=0**, while the banner reported that line among those "exempted as backticked prose". The **static** defect is not markdown-specific — splicing a live unpinned call with a trailing backticked mention into `hack/install.sh` gives candidates=2, invocations=1, `pin_violations` EMPTY — but the **whole-gate green is** markdown-specific, and the distinction is stated because a looser sentence here would itself be falsifiable: in that shell script the run reds at §4b for an unrelated RUNTIME reason (the spliced call executes and its bundle is absent), whereas `SECURITY.md` is a document with no runtime twin, which is why rc=0 survives there. Measured both ways rather than inferred from either. **Narrowing the exemption needs quote- and markdown-aware parsing — the machinery that produced UC-01/02/03 — so it is deliberately NOT patched here.** It is instead disclosed as residual (1) in the PASS banner, ahead of the variable/`eval`/fragment spellings, because it is the residual that is actually REACHABLE in these three files while those are not present in any of them. The banner also no longer labels an exempted line "prose": the branch it took is a fact, "prose" was an inference, and R3-01 is the case where that inference is wrong. **This is UC-07 doing its job on its own author:** the claim was falsifiable, it was checked, and the gate is now incomplete-and-honest rather than complete-and-wrong. Accepted cost, stated rather than discovered later: a *correctly pinned* invocation written off command position also reds. That is the intended direction of error for a security gate over three files carrying four invocations in one canonical style, and the message names the fix. **Anti-vacuity, because a lane whose entire deliverable is "make a gate stricter" is maximally exposed** (this repo's #1 review finding is an assertion that cannot fail — D-124, AUD-S18): section 2b runs **eight** mutants against **each** of the three files and 2c runs a ninth, and they were chosen for **branch** coverage of the new predicate rather than for variety of shape — (a) a second **unpinned** invocation must red, name the file, quote the offending command, and produce **exactly two** violations so the pinned siblings are provably not flagged; (b) a second **correctly pinned** invocation must stay green **and** must raise the extractor's invocation count by one, without which (b)'s green could mean "the extractor never saw it"; (c) an occurrence off command position, appended **fully pinned on purpose** so the only thing that can redden it is the classifier itself; (d) every invocation deleted → `NO-CANDIDATES`; (e) the continuation fold broken → `FOLD-BROKEN`, asserted **not** to also report `UNPINNED`, because conflating "the extractor truncated this" with "the pin is missing" sends the next maintainer to the wrong file; (f) every invocation displaced off command position → `NO-INVOCATIONS`; (g) a second invocation carrying both flags with an **empty** identity regexp → `WRONG-VALUE`, the hostile shape a presence-only check waves through; (h) a second invocation carrying both flags with a **foreign OIDC issuer** → `WRONG-VALUE` on the issuer half; and 2c strips the backticks from `SECURITY.md`'s prose mention and requires the same row to fail closed, proving the exemption keys on the backticks and is not a blanket bypass. The runtime twin has its own vacuity control (an unpinned line appended to a copy of the stub log must be seen). **One real defect was caught by this section during the lane and is recorded because it is a trap for the next author**: `log_unpinned_lines` was first written with `awk -v id="identity_re=${sec_identity}"`, and **awk processes escape sequences in a `-v` assignment**, so the pin's `github\.com` arrived as `github.com` and matched nothing; the values now reach awk through `ENVIRON`. **Stated limit, same posture as D-154's**: the denominator is the literal string `cosign verify-blob`, so an invocation spelled through a variable (`"$COSIGN" verify-blob …`), built by `eval`, or assembled from fragments is invisible to this gate. Widening the trigger to bare `verify-blob` was considered and **rejected** — it buys one hypothetical no file uses, and turns any future `SECURITY.md` sentence saying "verify-blob" outside backticks into a red gate, whose repair would be to loosen the classifier. **Nothing outside the gate changed**: `hack/install.sh`, `SECURITY.md` and `hack/release/verify-artifacts.sh` are byte-unchanged, and all four of their invocations were already pinned — this row closes a hole in the *checker*, not in the checked. The stage `release-install-cosign-pin-test` was already wired into `task check` and pinned in `hack/audit/exitgate_test.sh`'s `CHECK_STAGES`, so no wiring changed either. **SECOND ROUND — the first version of this fix REPRODUCED the very defect it closes, one level down, and independent review caught it before merge.** `has_flag` grepped the whole FILE; the replacement grepped the whole LINE. Both are existential; only the scope shrank. **UC-01**: one folded line was graded as at most ONE invocation, so `cosign verify-blob --bundle a.json a && cosign verify-blob --bundle EVIL.json evil` began with `cosign verify-blob` (not UNCLASSIFIABLE), carried `--bundle` (not FOLD-BROKEN), carried both flag strings (not UNPINNED) and yielded the published values (not WRONG-VALUE) — **green**, in same-line, `;`-separated and backslash-folded forms, on all three graded files. Worse, the **full gate exited 0** for `SECURITY.md`: the two shell scripts were incidentally caught by the runtime twin (`log_unpinned_lines` at §5d), but **SECURITY.md is a document and has no runtime twin**, so the hole was unmitigated precisely on the file this row calls "the copy-paste recipe adopters run by hand" — D-153's exact incident class — while the gate printed "EVERY cosign verify-blob invocation ... is pinned", false as printed. The stated-limit paragraph did not cover it: it enumerated `$COSIGN`, `eval` and fragments, not `&&` chaining, and **leading** `&&` was explicitly handled (mutant (c) → UNCLASSIFIABLE), so the operator had been considered and only one side handled. Closed by grading per **OCCURRENCE** (`occurrence_count` + a new `MULTI-OCCURRENCE` violation that fails closed on any line carrying two calls) rather than per line; counting inside an already-triggered line uses the shorter `verify-blob`, so a second call spelled `cosign verify-blob` or `"$COSIGN" verify-blob` and chained onto a pinned one is caught too, which the literal file-level trigger would miss. **UC-02**: `extract_issuer`/`extract_identity` anchor on a greedy `.*`, so the LAST value on a line wins — a hostile `--certificate-identity-regexp ''` placed FIRST was masked by a correct value placed second, **the extraction direction favoured the attacker**. Closed by `flag_value_tokens`, which returns EVERY value of every flag on the occurrence; each must equal the published one, so a repeated flag with an empty value first now reds. **UC-03 (P2, the erosion path)**: a CORRECTLY pinned call written with **double quotes** was refused as `WRONG-VALUE ... identity= ... (an empty regexp matches every Fulcio identity)` — a true refusal with a **false reason**, and the repair a maintainer reaches for on that message is widening the extractor. Both quote styles are now accepted (double quotes are correct shell for this value: it contains no `$` and no backtick), and a **bare** value is diagnosed as `UNQUOTED-VALUE`, a QUOTING defect, never as a wrong or empty one — a gate that misdiagnoses erodes faster than one that refuses. **UC-04**: this row's "rc=0, rc=0, rc=0" reproduction figure is **splice-shape-sensitive** and is stated as measured: it holds for a second unpinned call spliced **in-body** next to the real one. An EOF-appended mutant reds the two shell scripts for an unrelated runtime reason, so the figure should not be read as holding for any second unpinned call. The same sensitivity is why the UC-03 double-quoted mutant reds the two scripts through the full gate (the spliced call actually RUNS and its fixture bundle is absent) while `SECURITY.md`, having no runtime, is the clean end-to-end proof at rc=0. The classifier's fail-closed posture on off-command-position occurrences was independently adjudicated KEEP, since §2c proves the prose exemption keys on the backticks and cannot be widened silently. **The lesson, recorded because it is the whole point of the lane:** a fix for an existential-where-universal defect is itself maximally exposed to the same defect at the next scope down — file, then line, then occurrence — and the only thing that caught it was a reviewer building the chained-call mutant that neither the 20-row external matrix nor the 25 in-gate mutants contained. **THIRD ROUND (UC-05..UC-09), and the reason this row now reads the way it does.** **UC-05**: `occurrence_count` compared whitespace-delimited fields to `verify-blob`, found one, and concluded **singularity** — when all it had established is that it did not find a second. `cosign "verify-blob"`, `cosign 'verify-blob'` and `cosign verify-blob""` are the same command to the shell, and each hid a chained unpinned call from the count. Measured against the round-2 gate on **all three** files at whole-gate level: **rc=0, rc=0, rc=0** — worse than first reported, because `A && B` short-circuits and `set -e` does **not** fire on a non-final member of an AND-list, so even the two shell scripts' runtime twin missed it. Closed by stripping shell quotes before the field compare, which is **a spelling patch, not a terminator, and is labelled as one in the code**. A structural terminator was sought and **ruled out with evidence**: refusing any graded line carrying a command separator would red the REAL files, since `hack/install.sh` and `hack/release/verify-artifacts.sh` both legitimately end their genuine invocation with an or-else `die` tail. **No structural terminator exists for this gate**, so completeness is not available and must not be claimed. **UC-07 is therefore the durable fix, and it is a change of posture rather than of logic.** The `PASS:` banner was printed unconditionally and asserted "EVERY cosign verify-blob OCCURRENCE ... is pinned" plus "a second unpinned call reddens whether it sits on its own line or is chained onto a pinned one" — a **universal claim bound to no assertion**. Under UC-05 that banner did not merely overstate: on a green run it printed something **false**, which is strictly worse than printing nothing, and it is this repo's #1 defect class (an assertion that cannot fail) wearing a different hat. The banner now reports **OBSERVED** counts per file (occurrences found, classified-and-graded, prose-exempt, refused), lists **ASSERTED** properties each of which is shown to fail on a mutant, and states the **NOT ASSERTED** residual in the same breath: this gate finds calls by a literal string and counts quote-stripped words, so a call spelled through a variable, `eval`, or fragments is not seen and not graded, and the counts are what was classified — **not a proof that nothing else exists**. When a fifth spelling turns up, the gate is then **incomplete rather than wrong**, and incomplete is recoverable. **UC-06**: a TRAILING comment mentioning `verify-blob` fired `MULTI-OCCURRENCE: … has 2 verify-blob calls chained on ONE line` — fail-closed but **false as printed**, since `cosign_candidates` only drops lines whose FIRST non-space character is `#`. That is the UC-03 misdiagnosis shape recurring **inside the UC-01 fix**. The message now says TOKENS, names both possible causes, and names the correct repair; excluding trailing comments was rejected as it needs quote-aware parsing, which is the machinery that produced UC-01/02/03. **UC-08**: `shellcheck` SC1087 ×2 (error severity) on `flag_value_tokens` — `"$flag[[:space:]]…"` parses as an array subscript; `${flag}` silences it. Cosmetic: no shellcheck gate exists in this repo. **UC-09**: two stage-count figures reported during this lane ("25", then "28") were **both wrong** and neither came from a committed file — they counted `task: [` lines in a run LOG, which includes nested task invocations. Measured: the Taskfile `check:` block has **21** `- task:` entries and `CHECK_STAGES` has **21** stage names; they agree, and both files are byte-identical to `origin/main`. There is no stage-count mismatch and this lane could not have caused one. **THE PATTERN, recorded because it is the lane's most transferable output:** four P1s, three of them the same scope error at successively finer scopes — file (`has_flag`), line (round 1), then occurrence-spelling (round 2) — each found by review and none by the author's own mutant matrix, because each matrix was built from the shape the previous round had just learned. The general lesson is not "add another spelling": it is that **when a gate's denominator cannot be made provably complete, the gate must report what it classified instead of asserting what exists.** That is now enforced where it is hardest to ignore, in the banner the gate prints on every green run. Revert: restore `has_flag`, drop the per-occurrence grading and the per-invocation value comparison, and restore the two positive-only stub-log greps — which returns the gate to certifying that *some* cosign call in each file is pinned, a property that is compatible with an unpinned one sitting beside it. | +| D-161 | 2026-08-24 | **E10-S00's `backlog.md:557` maintainer-LGTM (GOVERNANCE) marker is SATISFIED: the operator granted LGTM explicitly and authorized the integration of PR #85.** Recorded here because the approval was given in an agent session, and **an approval that exists only in a transcript is invisible to everyone who later asks whether the marker was honoured** — the same defect this lane's own handoff named as a standing lesson. GitHub's `reviewDecision` on #85 is empty (the maintainer is also the PR author, so GitHub refuses a self-review), which is why this D-row plus a PR comment — not a GitHub approval — is the durable record. **Integration mechanism:** #85 was 2 ahead / 24 behind with three genuine append-adjacency conflicts (`CHANGELOG.md`, `decisions.md`, `open-questions.md`). Force-push is forbidden (hard rule 2), so the branch was brought up to date by **merging `main` down into the lane** and merged with `gh pr merge --merge`, NOT by a rebase and NOT by a replacement PR — this preserves #85 and, with it, the full review history posted as its comment. Conflicts were resolved as **ordered unions** and verified **by content, not by count** (D-152..D-160 each present exactly once; OQ-31..OQ-36 each exactly once; all 11 CHANGELOG entries present); `task changelog-write` was then a byte no-op, independently corroborating the CHANGELOG union. **What the LGTM covers:** a docs/spec-only model (zero Go, zero schema bytes) whose one P1 was fixed fail-closed, so v1 GitHub comments and does not gate. **What it does NOT cover:** the same marker still guards **E10-S02/S04, E11-S02/S04/S06/S07, DEM-S00, WG-S01** — each needs its own LGTM; this row is not a blanket waiver. Revert: `git revert` the merge commit. |