From a072933e82b4d912ff2c4c8ead6d4f5de2ae85c0 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 23 Aug 2026 17:48:05 +0200 Subject: [PATCH 1/2] :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/2] :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