diff --git a/CHANGELOG.md b/CHANGELOG.md index a392ce30..1f350836 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -104,6 +104,7 @@ repository still gets a decision, never by following the link; no release carrie - :memo: docs(ci): withdraw the third copy of the retracted claim (D-157) - :memo: docs(e10): record the operator LGTM that satisfies E10-S00's GOVERNANCE marker (D-161) - :memo: docs(planning): record the 2026-08-25 operator rulings and withdraw the stale rule-7 block (D-162..D-164) +- :memo: docs(forge): name the catalog's overstated-coverage defect and log the coverage-floor headroom (D-165) ### Features - :sparkles: feat(audit): measure REL-03/REL-07 by real mutation run, not by name @@ -126,6 +127,7 @@ repository still gets a decision, never by following the link; no release carrie ### Refactoring - :recycle: refactor(audit): pin REL-03 and REL-07 by test name, demote source shape +- :recycle: refactor(forge): extract the conformance suite into an importable package with an explicit observation surface (E10-S01) ### Security - :lock: fix(cmd): discriminate absent provider declaration from forge failure (REL-03) diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index af23855e..3bc27750 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -169,3 +169,4 @@ project/process decisions. | D-162 | 2026-08-25 | **P5-DEM is re-decomposed spec-first before any DEM story is implemented: all 15 stories (DEM-S00–S14) carry ZERO `Verify:` annotations, so not one of them states how it would be proven.** Operator-ruled 2026-08-25. This is a spec defect, not a per-lane gap: hard rule 4 puts specs before code, and DEM-S14 is the epic exit gate — it has nothing to aggregate if each lane invents its own gate ad hoc when it is picked up. **Rejected: annotate lane-by-lane** — cheaper per lane, but it defers the question of what the epic proves to the last story, which is precisely where this project has repeatedly found gates asserting a universal property their denominator could not establish (D-160, D-157). **Rejected: leave parked** — DEM is the adoption/demo epic and DEM-S11 is also E10-S18's live adoption target, so it is on the critical path for E10's own evidence, not optional. Scope: re-decomposition is a spec change under `openspec/` only; no DEM code lands until every DEM story has a `Verify:` annotation. Revert: restore the current 15 rows from git history; the annotations are additive and can be kept regardless. | | D-163 | 2026-08-25 | **The recurring "dependabot reddens `main` on the CHANGELOG drift gate" failure is fixed narrowly, at the bot, NOT by enabling the drift gate on `pull_request`: OQ-30's guard STAYS and D-148 STANDS.** Operator-ruled 2026-08-25 after an initial ruling to the contrary was withdrawn on corrected premises. Incident: `cd4dac1` + `34e94cf` (codeql-action 4.37.7→4.37.8) merged green and reddened `main` in BOTH `verify` (step *changelog drift gate*) and `release-exitgate` (`task check` stage 21) — run 32817829705/32817735039; healed by PR #96. **Why not enable the gate on PRs, even though D-148's stated mechanism does not survive:** OQ-30 measured the merge-direction ordering hazard only in the lane-first-parent direction, while GitHub mints the merge ref base-first — so D-148 is defensible as PRECAUTION, not as mechanism, and this row records that explicitly rather than leaving a dead premise standing. The decisive objection is different and was missed when the question was first put: **dependabot cannot run `task changelog-write` on its own branch**, and `verify` is a REQUIRED status check with `strict=true` and `enforce_admins=true` (measured via the branch-protection API). Enabling the gate would therefore convert "main red for ~1h, healed by one regen commit" into "every dependabot PR red on a required check until a human pushes a regen commit to the bot's branch" — worse for the very incident that prompted it — while additionally exposing OQ-30's three still-untested cases (fork PRs, `pull_request_target`, force-push re-minting the merge ref) on a required check, where a false positive is unfixable by the author. **Mechanism sub-choice is deliberately NOT taken here** and is parked for the operator in `agent-context/INBOX.md`: (i) auto-regenerate on dependabot branches needs a write token on a bot-authored PR branch (`pull_request_target` or a PAT) and races dependabot's own rebase force-pushes; (ii) making the push-to-main gate tolerate bot-only `build(deps)` drift is a FAIL-OPEN in a gate, the exact species this repo has spent two waves removing, and would need both polarities plus a mutation control before it could be trusted. Until one is chosen, the standing remedy is the status quo: heal `main` with a `:memo: chore(release):` regen commit, whose subject `cliff.toml:119` skips so it does not add itself. Revert: none needed — this row changes no code; it records why the guard was not deleted. | | D-164 | 2026-08-25 | **DOC-TRUTH — `backlog.md` rows E11-S03 and E11-S04 still said "blocked on the operator's rule-7 answer (d1/d2)" nine days after D-144 answered it, and that staleness actively misled a session.** D-144 (2026-08-16) resolves D-141's sub-question (d) to **(d1)** and ends with the words "**Unblocks E11-S03 and E11-S04**"; the two backlog rows kept their 🔴 marker and their "blocked on the operator" text regardless. Consequence, observed rather than hypothesised: the 2026-08-25 session handoff and the operator's own session-opening message both carried "the rule-7 (d1/d2) question is still parked on you" forward as fact, and the question was nearly put to the operator a second time. Same species as D-157/D-160 — a claim retained after the thing that justified it was withdrawn. Fix: both rows now cite D-144 and carry their real remaining dependency. **Generalisation deliberately not made:** no gate is added here asserting "every 🔴 row has a live blocker", because that predicate would match row TEXT, and matching text where the property is structural is the defect this project keeps re-introducing (14 P1s across two gate lanes). Revert: restore the two rows from git history. | +| D-165 | 2026-08-25 | **E10-S01 extracts the forge conformance suite into importable Go, and the extraction found that the two SHA-guard cases had NEVER run against GitLab despite their catalog rows claiming `forge: gitlab`.** **That second half is a DOC-TRUTH defect in its own right, same species as [D-157], [D-160] and [D-164]:** `catalog.yaml` asserted adapter coverage that did not exist, for two L1 cases on the SHA-guard — the single most safety-relevant property in the suite — and nothing detected it for the entire life of the file, because the only thing checking the catalog compared it against test-function NAMES, which were all present and correct. A row claiming coverage is not evidence of coverage. Scope decisions taken deliberately: **(a)** `conformance.Factory` returns a `forge.Forge`, NOT the composite `forge.RunPort` that REQ-E10-S01-01 names — `RunPort` is E10-S02's deliverable and S02 is **LGTM-gated**, so declaring it in this `[autonomous]` lane would smuggle an LGTM-gated core-contract change past the `backlog.md:557` GOVERNANCE marker; S02 depends on S01 exactly so the port change lands against an executable suite, and the substitution is then a one-line change. **(b)** Merge/approve/thread write counts are taken by a PORT-LEVEL decorator (`portcount.go`), not per adapter, because they are not observable at the transport layer for GitLab — its `MergeCAS` re-reads heads and refuses client-side, so a moved head produces ZERO merge HTTP requests and an httptest harness would report 0 attempts where the port was called once. **(c)** Cases take pins from `Fixture.Pins()` instead of literals, because GitLab synthesises its merge-result digest (`gitlab.SyntheticDigest`) — a literal digest is why those cases could only ever run on the fake. Collapsing that digest is E10-S03. **Assertions were STRENGTHENED, never downgraded (REQ-E10-S01-04):** the summary-updated-in-place check was guarded behind `if ff, ok := f.(*fake.Forge); ok` and therefore unfailable on every non-fake backend; merge ATTEMPTS are now distinguished from merges PERFORMED, so "the pre-check refused before any mutation" (0 attempts) is separable from "the atomic CAS refused" (1 attempt), which `len(Merges) == 0` alone could not express; and a `NotesUpdated() > 0` check was replaced with an exact count after a gate proved no corruption of that value could flip it. Coverage of the catalogued cases went from partial-and-asymmetric to **all five on both adapters**; the package's passing assertions went from 9 test functions to 103 subtests. **The three self-checks are the deliverable as much as the extraction is** — observed-execution catalog matching, can-fail-against-sabotage on every adapter, and per-observation load-bearing corruption; the last of these found two real weaknesses in code that had already passed review, which is the recorded lesson that reviewers must EXECUTE rather than read. **Foreseeable consequence of THIS commit, recorded here so E10-S02's author does not discover it:** the extraction moved ~700 statements from `_test.go` (uncounted) into production-classified Go whose only exerciser is the three self-check gates, and global coverage now sits at **91.1% against a 91.0% floor**. Every future E10 story that adds a conformance case also adds uncovered `t.Fatalf` assertion branches and will red the D-010/D-128 floor. The fix when it happens is NOT to delete assertions — it is either to extend the load-bearing gate to the new case (which covers those branches as a side effect, and is the reason the floor is met today) or to rule on whether a test-support package belongs in the coverage denominator at all. Logged in `agent-context/INBOX.md`. Revert: the pre-extraction `_test.go` bodies are one commit back; the gates are additive and worth keeping regardless. | diff --git a/internal/forge/conformance/backends_test.go b/internal/forge/conformance/backends_test.go new file mode 100644 index 00000000..5172eaf4 --- /dev/null +++ b/internal/forge/conformance/backends_test.go @@ -0,0 +1,219 @@ +package conformance + +import ( + "encoding/json" + "fmt" + "net/http" + "strconv" + "strings" + + "github.com/PlatformRelay/assent/internal/forge" + "github.com/PlatformRelay/assent/internal/forge/fake" + gitlab "github.com/PlatformRelay/assent/internal/forge/gitlab" +) + +// backends_test.go wires the two built-in backends into the shared suite. Each is +// a Factory and nothing more — the case bodies live in importable non-test Go, so +// a third adapter (GitHub, E10-S06+) needs a file this size and no copied cases. + +// ---- fake backend ---- + +type fakeBackend struct { + f *fake.Forge + cp *countingPort +} + +func (b fakeBackend) MergeAttempts() int { return b.cp.mergeAttempts } +func (b fakeBackend) MergesPerformed() int { return b.cp.mergesPerformed } +func (b fakeBackend) Approvals() int { return b.cp.approvals } +func (b fakeBackend) ThreadsCreated() int { return b.cp.threadsCreated } +func (b fakeBackend) ThreadsResolved() int { return b.cp.threadsResolved } +func (b fakeBackend) NotesCreated() int { return b.f.NoteCreateCalls } +func (b fakeBackend) NotesUpdated() int { return b.f.NoteUpdateCalls } +func (b fakeBackend) NoteBody(id string) string { return b.f.NoteBody(id) } +func (b fakeBackend) IsResolved(id string) bool { return b.f.IsResolved(id) } +func (b fakeBackend) ThreadCount() int { return b.f.ThreadCount() } +func (b fakeBackend) BotThreadCount() int { return b.f.BotThreadCount() } +func (b fakeBackend) OpenBotThreadCount() int { return b.f.OpenBotThreadCount() } + +func (b fakeBackend) SeedThread(id, author string, m forge.Marker, resolved bool) error { + b.f.SeedThread(id, author, m, resolved) + return nil +} + +func (b fakeBackend) SeedNote(id, author string, m forge.Marker, body string) error { + b.f.SeedNote(id, author, m, body) + return nil +} + +func (b fakeBackend) MoveTargetHead(sha string) { b.f.CurrentTargetSha = sha } + +func (b fakeBackend) Pins() forge.DesiredMerge { + return forge.DesiredMerge{ + SourceSha: b.f.CurrentSourceSha, + TargetSha: b.f.CurrentTargetSha, + MergeResultDigest: b.f.CurrentMergeResultDigest, + } +} + +func (b fakeBackend) DriftSourceHeadAfterRead(sha string) { + b.f.AfterCurrentHeads = func(fk *fake.Forge) { fk.CurrentSourceSha = sha } +} + +func fakeFactory(t TB, cfg Config) Backend { + t.Helper() + f := fake.New(cfg.BotAuthor, cfg.CurrentSourceSHA, cfg.CurrentTargetSHA, cfg.CurrentMergeResultDigest) + cp := newCountingPort(f) + b := fakeBackend{f: f, cp: cp} + return Backend{Port: cp, Fixture: b, Observer: b} +} + +// ---- gitlab backend (httptest) ---- + +type gitlabBackend struct { + h *gitlabHarness + cp *countingPort +} + +func (b gitlabBackend) MergeAttempts() int { return b.cp.mergeAttempts } +func (b gitlabBackend) MergesPerformed() int { return b.cp.mergesPerformed } +func (b gitlabBackend) Approvals() int { return b.cp.approvals } +func (b gitlabBackend) ThreadsCreated() int { return b.cp.threadsCreated } +func (b gitlabBackend) ThreadsResolved() int { return b.cp.threadsResolved } +func (b gitlabBackend) NotesCreated() int { return b.h.noteCreateCalls } +func (b gitlabBackend) NotesUpdated() int { return b.h.noteUpdateCalls } + +func (b gitlabBackend) NoteBody(id string) string { + n, err := strconv.Atoi(strings.TrimPrefix(id, "note/")) + if err != nil { + return "" + } + for _, note := range b.h.notes { + if note.id == n { + return note.body + } + } + return "" +} + +func (b gitlabBackend) IsResolved(id string) bool { + for _, d := range b.h.discussions { + if d.id == id { + return d.resolved + } + } + return false +} + +func (b gitlabBackend) ThreadCount() int { return len(b.h.discussions) } + +func (b gitlabBackend) BotThreadCount() int { + n := 0 + for _, d := range b.h.discussions { + if d.author == b.h.botAuthor { + n++ + } + } + return n +} + +func (b gitlabBackend) OpenBotThreadCount() int { + n := 0 + for _, d := range b.h.discussions { + if d.author == b.h.botAuthor && !d.resolved { + n++ + } + } + return n +} + +func (b gitlabBackend) SeedThread(id, author string, m forge.Marker, resolved bool) error { + return b.h.seed(id, author, m, resolved) +} + +func (b gitlabBackend) SeedNote(id, author string, m forge.Marker, body string) error { + n, err := strconv.Atoi(strings.TrimPrefix(id, "note/")) + if err != nil { + return fmt.Errorf("gitlab backend: note id %q is not note/: %w", id, err) + } + return b.h.seedNote(n, author, m, body) +} + +func (b gitlabBackend) MoveTargetHead(sha string) { b.h.targetSHA = sha } + +// Pins reports the pins an evaluation would record against THIS backend. The +// digest is synthesised by the adapter (`gitlab.SyntheticDigest`) because GitLab +// exposes no merge-result digest — which is exactly why a case carrying a literal +// digest could only ever run against the fake. Collapsing the synthetic digest +// onto a real one is E10-S03. +func (b gitlabBackend) Pins() forge.DesiredMerge { + return forge.DesiredMerge{ + SourceSha: b.h.sourceSHA, + TargetSha: b.h.targetSHA, + MergeResultDigest: gitlab.SyntheticDigest(b.h.sourceSHA, b.h.targetSHA), + } +} + +// DriftSourceHeadAfterRead moves the source head once the pre-check's MR read has +// been served — the TOCTOU window. Reconcile reads the heads, then MergeCAS reads +// them again; arming on the FIRST read means the second sees the moved head, so +// the atomic CAS is the thing that refuses. +func (b gitlabBackend) DriftSourceHeadAfterRead(sha string) { + b.h.afterMRRead = func(h *gitlabHarness) { h.sourceSHA = sha } +} + +func gitlabFactory(t TB, cfg Config) Backend { + t.Helper() + h := newGitLabHarness(cfg.Project, cfg.MR) + h.botAuthor = cfg.BotAuthor + h.sourceSHA = cfg.CurrentSourceSHA + h.targetSHA = cfg.CurrentTargetSHA + cp := newCountingPort(h.client(t)) + b := gitlabBackend{h: h, cp: cp} + return Backend{Port: cp, Fixture: b, Observer: b} +} + +// ---- MR / approve / merge endpoints (E10-S01) ---- +// +// The pre-extraction harness served discussions and notes only, so the two +// SHA-guard cases could never run against GitLab — they were fake-only, while +// their catalog rows said `forge: gitlab`. These three routes close that gap. + +func (h *gitlabHarness) serveMR(w http.ResponseWriter, _ *http.Request) { + h.mrReads++ + _ = json.NewEncoder(w).Encode(map[string]any{ + "iid": 1, + "project_id": 42, + "sha": h.sourceSHA, + "source_branch": "feature", + "target_branch": "main", + }) + // Fire AFTER the response is written, so this read returns the PRE-move value + // and only the NEXT one sees the drift. + if h.afterMRRead != nil { + h.afterMRRead(h) + } +} + +func (h *gitlabHarness) serveBranch(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "commit": map[string]any{"id": h.targetSHA}, + }) +} + +func (h *gitlabHarness) approve(w http.ResponseWriter, _ *http.Request) { + h.approveCalls++ + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{"id": 1}) +} + +func (h *gitlabHarness) merge(w http.ResponseWriter, r *http.Request) { + h.mergePUTs++ + // ?sha= is GitLab's compare-and-swap on the SOURCE head: a moved source is + // 409, no merge. Modelled faithfully so the case proves the guard, not the fake. + if got := r.URL.Query().Get("sha"); got != h.sourceSHA { + http.Error(w, "sha mismatch", http.StatusConflict) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"merge_commit_sha": "deadbeef"}) +} diff --git a/internal/forge/conformance/canfail_test.go b/internal/forge/conformance/canfail_test.go new file mode 100644 index 00000000..f1b75bd8 --- /dev/null +++ b/internal/forge/conformance/canfail_test.go @@ -0,0 +1,107 @@ +package conformance + +import ( + "testing" + + "github.com/PlatformRelay/assent/internal/forge" +) + +// canfail_test.go answers the question a green conformance suite cannot answer +// about itself: would any of these cases NOTICE if the property it claims to +// prove were violated? +// +// This repo's most frequently found defect is an assertion that cannot fail, and +// three of the cases here shipped with one — the summary-updated-in-place check +// was guarded behind a `*fake.Forge` type assertion that was false on GitLab, so +// it proved nothing there while still reporting PASS. Reading the code did not +// catch that; running it against a violating backend does. + +// sabotagedObserver reports zero for every observation and truthfully answers +// nothing. A case that still passes against it is asserting on nothing. +type sabotagedObserver struct{} + +func (sabotagedObserver) MergeAttempts() int { return 0 } +func (sabotagedObserver) MergesPerformed() int { return 0 } +func (sabotagedObserver) Approvals() int { return 0 } +func (sabotagedObserver) ThreadsCreated() int { return 0 } +func (sabotagedObserver) ThreadsResolved() int { return 0 } +func (sabotagedObserver) NotesCreated() int { return 0 } +func (sabotagedObserver) NotesUpdated() int { return 0 } +func (sabotagedObserver) NoteBody(string) string { return "" } +func (sabotagedObserver) IsResolved(string) bool { return false } +func (sabotagedObserver) ThreadCount() int { return 0 } +func (sabotagedObserver) BotThreadCount() int { return 0 } +func (sabotagedObserver) OpenBotThreadCount() int { return 0 } + +// inertFixture accepts every arrangement and performs none of it: the SHA never +// moves, so a guard that should fire never gets the chance. +type inertFixture struct{ real Fixture } + +func (f inertFixture) SeedThread(id, author string, m forge.Marker, resolved bool) error { + return f.real.SeedThread(id, author, m, resolved) +} +func (f inertFixture) SeedNote(id, author string, m forge.Marker, body string) error { + return f.real.SeedNote(id, author, m, body) +} +func (f inertFixture) Pins() forge.DesiredMerge { return f.real.Pins() } +func (inertFixture) MoveTargetHead(string) {} +func (inertFixture) DriftSourceHeadAfterRead(string) {} + +// sabotage wraps ANY factory, so the gate runs against every backend rather than +// only the fake. That matters: the defect this file exists to catch — an +// assertion guarded behind a `*fake.Forge` type assertion — was invisible on the +// fake and dead on GitLab, so a fake-only gate would have missed the very bug +// that motivated it. +func sabotage(f Factory) Factory { + return func(t TB, cfg Config) Backend { + b := f(t, cfg) + return Backend{ + Port: b.Port, + Fixture: inertFixture{real: b.Fixture}, + Observer: sabotagedObserver{}, + } + } +} + +// TestEveryCaseCanFail requires EVERY catalogued case to go red against a backend +// that violates what it claims to prove. A case that passes here has stopped +// asserting: either its checks were weakened to what any backend can satisfy, or +// they were guarded behind a condition that is false in general. +// +// This is the gate REQ-E10-S01-04 needs in order to mean anything. Without it, +// "no assertion is downgraded to accommodate a backend" is a promise in a comment. +func TestEveryCaseCanFail(t *testing.T) { + cases := Cases() + if len(cases) == 0 { + t.Fatal("no cases — this gate would be vacuously satisfied") + } + for _, be := range backends() { + for _, c := range cases { + t.Run(be.name+"/"+c.ID, func(t *testing.T) { + failed, msg := runAndRecord(c, sabotage(be.f)) + if !failed { + t.Fatalf("case %q PASSED against a sabotaged %s backend — it is asserting "+ + "nothing that distinguishes a conforming forge from a broken one "+ + "(REQ-E10-S01-04)", c.ID, be.name) + } + t.Logf("case %q correctly failed: %s", c.ID, msg) + }) + } + } +} + +// TestSabotageIsDetectableNotUniversal is the POSITIVE CONTROL. Without it, +// TestEveryCaseCanFail would also be satisfied by cases that fail against +// EVERYTHING — including a conforming backend — which proves nothing about the +// assertions and everything about a broken harness. +func TestSabotageIsDetectableNotUniversal(t *testing.T) { + for _, be := range backends() { + for _, c := range Cases() { + t.Run(be.name+"/"+c.ID, func(t *testing.T) { + if failed, msg := runAndRecord(c, be.f); failed { + t.Fatalf("case %q failed against a CONFORMING %s backend: %s", c.ID, be.name, msg) + } + }) + } + } +} diff --git a/internal/forge/conformance/cases_replay.go b/internal/forge/conformance/cases_replay.go new file mode 100644 index 00000000..e62cf13b --- /dev/null +++ b/internal/forge/conformance/cases_replay.go @@ -0,0 +1,223 @@ +package conformance + +import ( + "github.com/PlatformRelay/assent/internal/forge" +) + +// cases_replay.go holds the three ADR-0019 publication-protocol replay cases. +// +// Each one previously existed TWICE — once as a `fake/...` subtest and once as a +// `gitlab/...` subtest — with assertions that did not match: the fake subtests +// asserted on thread COUNTS, the GitLab subtests on write-CALL counts +// (`createCalls`, `noteCreateCalls`, `noteUpdateCalls`, `resolveCalls`), because +// each backend only exposed one of the two. Neither backend was checked against +// the other's assertions. +// +// Extraction merges them into ONE body per case asserting the UNION, which every +// backend must now satisfy. No assertion was dropped to make that work +// (REQ-E10-S01-04); the counters the fake lacked were added to it instead. + +func replayConfig(project, mr string) Config { + return Config{ + Project: project, + MR: mr, + BotAuthor: botID, + // The replay cases never merge, so the heads only have to be internally + // consistent — but they are still stated explicitly rather than defaulted, + // because a Factory that invents SHAs makes the SHA-guard cases unreadable. + CurrentSourceSHA: "src", + CurrentTargetSHA: "tgt", + CurrentMergeResultDigest: "sha256:merge", + } +} + +// caseRerunIdempotence is REQ-E4-S09-01, replaying rerun-idempotence.yaml and +// crash-then-rerun.yaml: a plain rerun creates zero new bot threads and updates +// the summary IN PLACE, while a crash-then-rerun fills exactly the one gap slot +// without duplicating the partial work that already landed. +func caseRerunIdempotence(t TB, f Factory) { + t.Helper() + + t.Run("rerun-idempotence", func(t TB) { + // Pre-state = rerun-idempotence.yaml run2.step2ExistingArtifacts (lines 58-66). + b := f(t, replayConfig(proj, mrIID)) + mustSeedNote(t, b, "note/9000", botID, rerunSummaryMarker(), "old summary") + mustSeedThread(t, b, "note/9001", botID, rerunChallengeMarker(), true) // reviewer-resolved + mustSeedThread(t, b, "note/9002", botID, rerunCommentMarker(), false) + + created, err := replayRerunIdempotence(b.Port, b.Observer) + if err != nil { + t.Fatal(err) + } + // expected.newArtifactsCreated: 0 (rerun-idempotence.yaml line 82). + if created != 0 { + t.Fatalf("rerun must create zero new artifacts, created %d", created) + } + if got := b.Observer.BotThreadCount(); got != 2 { + t.Fatalf("rerun must leave exactly 2 bot threads, got %d", got) + } + if !b.Observer.IsResolved("note/9001") { + t.Fatal("rerun must preserve reviewer resolution of note/9001") + } + // Was GitLab-only before extraction; now required of every backend. + if got := b.Observer.ThreadsCreated(); got != 0 { + t.Fatalf("rerun must not create discussions, ThreadsCreated=%d", got) + } + if got := b.Observer.NotesCreated(); got != 0 { + t.Fatalf("rerun must not create summary notes, NotesCreated=%d", got) + } + // EXACT, not ">0". The replay reconciles two markers, each carrying the same + // summary, so the summary must be edited in place once per reconcile and never + // re-created. The ">0" form here was DEAD WEIGHT: any positive count satisfied + // it, including a runaway one, so no corruption of the value could flip the + // verdict. TestEveryObservationIsLoadBearing caught that, not review. + if got := b.Observer.NotesUpdated(); got != 2 { + t.Fatalf("rerun must update the summary in place once per reconcile (want 2), got %d", got) + } + }) + + t.Run("crash-then-rerun", func(t TB) { + // Pre-state = crash-then-rerun.yaml step2ExistingArtifacts (lines 55-61). + m := crashChallengeMarker() + b := f(t, replayConfig(m.Slot.Project, m.Slot.MR)) + mustSeedNote(t, b, "note/7000", botID, crashSummaryMarker(), "crash summary") + mustSeedThread(t, b, "note/7001", botID, m, false) + + created, err := replayCrashThenRerun(b.Port) + if err != nil { + t.Fatal(err) + } + // expected.newArtifactsCreated: 1 (crash-then-rerun.yaml line 81). + if created != 1 { + t.Fatalf("crash-then-rerun must create exactly one gap artifact, created %d", created) + } + if got := b.Observer.BotThreadCount(); got != 2 { + t.Fatalf("expected 2 bot threads after gap-fill, got %d", got) + } + if got := b.Observer.ThreadsCreated(); got != 1 { + t.Fatalf("crash-then-rerun must create exactly one discussion, ThreadsCreated=%d", got) + } + }) +} + +// caseDuplicateRepair is REQ-E4-S09-02, replaying duplicate-repair.yaml: the +// lowest forge id is canonical, non-canonical duplicates are RESOLVED rather than +// deleted or re-created, and PublicationReceipt.Repairs records each repair +// deterministically. +// +// Both seed orders run against every backend. The reversed order is not +// decoration: it is what proves canonical selection is by forge ID and not by +// scan order, and before extraction it ran on the fake only, while the GitLab +// side ran the fixture order alone under the name `gitlab/fixture-pagination-order`. +func caseDuplicateRepair(t TB, f Factory) { + t.Helper() + + wantRepairs := []forge.Repair{ + {RepairedForgeID: "note/8003", CanonicalForgeID: "note/8001", Action: "resolve"}, + {RepairedForgeID: "note/8005", CanonicalForgeID: "note/8001", Action: "resolve"}, + } + + for _, tc := range []struct { + name string + seed []string + }{ + {"fixture-pagination-order", []string{"note/8005", "note/8001", "note/8003"}}, + {"reversed-scan-order", []string{"note/8003", "note/8001", "note/8005"}}, + } { + t.Run(tc.name, func(t TB) { + b := f(t, replayConfig("platform/orders-service", "612")) + for _, id := range tc.seed { + mustSeedThread(t, b, id, botID, dupMarker(), false) + } + + receipt, err := replayDuplicateRepair(b.Port) + if err != nil { + t.Fatal(err) + } + + // expected.canonicalForgeId: note/8001 (duplicate-repair.yaml line 102). + if len(receipt.Operations) != 1 || receipt.Operations[0].TargetID != "note/8001" { + t.Fatalf("canonical must be note/8001, got %+v", receipt.Operations) + } + if len(receipt.Repairs) != len(wantRepairs) { + t.Fatalf("expected %d repairs, got %+v", len(wantRepairs), receipt.Repairs) + } + for i, want := range wantRepairs { + if receipt.Repairs[i] != want { + t.Fatalf("repair[%d]: got %+v want %+v", i, receipt.Repairs[i], want) + } + } + if got := b.Observer.OpenBotThreadCount(); got != 1 { + t.Fatalf("after repair exactly one open bot thread must remain, got %d", got) + } + // Both were GitLab-only before extraction. + if got := b.Observer.ThreadsCreated(); got != 0 { + t.Fatalf("duplicate repair must not create threads, ThreadsCreated=%d", got) + } + if got := b.Observer.ThreadsResolved(); got != 2 { + t.Fatalf("duplicate repair must resolve 2 duplicates, ThreadsResolved=%d", got) + } + }) + } +} + +// caseSpoofedMarkerIgnored is REQ-E4-S09-03 / P3-E5-S01-04: a contributor posting +// a WELL-FORMED marker has zero reconciliation effect, because the +// author-identity filter on ListBotThreads excludes it. +// +// The two sub-cases assert different things and both are load-bearing. The first +// proves the spoofed thread does not SATISFY an occupied slot; the second proves +// it does not satisfy an EMPTY one either — without it, a backend that ignored +// contributor threads by deleting them would pass. +func caseSpoofedMarkerIgnored(t TB, f Factory) { + t.Helper() + m := rerunCommentMarker() + + t.Run("contributor-marker-invisible-on-rerun", func(t TB) { + b := f(t, replayConfig(m.Slot.Project, m.Slot.MR)) + mustSeedThread(t, b, "note/9002", botID, m, false) + mustSeedThread(t, b, "note/6660", "contributor-mallory", m, false) + + before := b.Observer.ThreadCount() + receipt, err := forge.Reconcile(b.Port, testClock(), desiredThreadFor(m, rerunSummary()), forge.Preconditions{}) + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + // ThreadCount is deliberately the UNFILTERED count: "the contributor thread + // still exists but is invisible to the bot filter" is a different claim from + // "no contributor thread exists", and only the unfiltered count can tell a + // backend that IGNORED the spoof from one that DELETED it. + if after := b.Observer.ThreadCount(); after != before { + t.Fatalf("spoofed marker must not create threads on rerun: before=%d after=%d", before, after) + } + if got := threadOpTarget(receipt, "note/9002"); got != "note/9002" { + t.Fatalf("receipt must target bot thread note/9002, got %q (contributor would be note/6660)", got) + } + }) + + t.Run("contributor-only-creates-bot-thread", func(t TB) { + b := f(t, replayConfig(m.Slot.Project, m.Slot.MR)) + mustSeedThread(t, b, "note/6660", "contributor-mallory", m, false) + + if _, err := forge.Reconcile(b.Port, testClock(), desiredThreadFor(m, nil), forge.Preconditions{}); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if got := b.Observer.BotThreadCount(); got != 1 { + t.Fatalf("contributor marker alone must not satisfy slot; expected 1 bot thread, got %d", got) + } + }) +} + +func mustSeedThread(t TB, b Backend, id, author string, m forge.Marker, resolved bool) { + t.Helper() + if err := b.Fixture.SeedThread(id, author, m, resolved); err != nil { + t.Fatalf("seed thread %s: %v", id, err) + } +} + +func mustSeedNote(t TB, b Backend, id, author string, m forge.Marker, body string) { + t.Helper() + if err := b.Fixture.SeedNote(id, author, m, body); err != nil { + t.Fatalf("seed note %s: %v", id, err) + } +} diff --git a/internal/forge/conformance/catalog.go b/internal/forge/conformance/catalog.go new file mode 100644 index 00000000..7aac1174 --- /dev/null +++ b/internal/forge/conformance/catalog.go @@ -0,0 +1,142 @@ +package conformance + +import ( + "bytes" + "fmt" + "os" + "sort" + + "gopkg.in/yaml.v3" +) + +// catalog.go loads the forge-neutral conformance catalog (E10-S01, +// REQ-E10-S01-03). +// +// It is STRICT (P3-E2): an unknown field or an unknown adapter name is an error, +// never a silently ignored row. That matters specifically here — the catalog is +// the artifact that claims which backends a case is proven against, so a typo'd +// adapter name that decoded to "nothing" would make the catalog overstate +// coverage while staying green. Silent tolerance in a coverage index is +// indistinguishable from lying about coverage. + +// ConformancePackage is the package whose rows RunSuite executes. Rows in other +// packages (cmd/assent, test/e2e) are catalogued conformance cases too, but they +// are driven by their own tests, so they are outside the suite's denominator. +const ConformancePackage = "internal/forge/conformance" + +// DeferredForge marks a row nothing executes yet. +const DeferredForge = "github-deferred" + +// KnownAdapters is the closed set of adapter names a row may list. It is closed +// on purpose: adding "github" here is a deliberate act in E10-S06+, not something +// a catalog row can do on its own by naming a backend that does not exist. +var KnownAdapters = map[string]bool{ + "fake": true, + "gitlab": true, + "github": true, +} + +// CatalogCase is one row. +type CatalogCase struct { + ID string `yaml:"id"` + ADR string `yaml:"adr"` + Level string `yaml:"level"` + Req string `yaml:"req"` + Test string `yaml:"test"` + Package string `yaml:"package"` + Forge string `yaml:"forge"` + Adapters []string `yaml:"adapters"` + Note string `yaml:"note"` +} + +// Catalog is the decoded catalog file. +type Catalog struct { + Cases []CatalogCase `yaml:"cases"` +} + +// DecodeCatalog strict-decodes the catalog from raw YAML. +func DecodeCatalog(raw []byte) (Catalog, error) { + dec := yaml.NewDecoder(bytes.NewReader(raw)) + dec.KnownFields(true) + var c Catalog + if err := dec.Decode(&c); err != nil { + return Catalog{}, fmt.Errorf("conformance catalog: %w", err) + } + for _, row := range c.Cases { + for _, a := range row.Adapters { + if !KnownAdapters[a] { + return Catalog{}, fmt.Errorf( + "conformance catalog: case %q lists unknown adapter %q (known: %s)", + row.ID, a, knownAdapterList()) + } + } + } + return c, nil +} + +// LoadCatalog reads and strict-decodes the catalog at path. +func LoadCatalog(path string) (Catalog, error) { + raw, err := os.ReadFile(path) //nolint:gosec // co-located catalog fixture. + if err != nil { + return Catalog{}, fmt.Errorf("conformance catalog: %w", err) + } + return DecodeCatalog(raw) +} + +// SuiteCaseIDs is the catalog's claim about what RunSuite executes: every row in +// the conformance package that is not deferred. This is one half of +// REQ-E10-S01-02; the other half is what running the suite ACTUALLY dispatches, +// and the gate fails when they differ in either direction. +func (c Catalog) SuiteCaseIDs() []string { + var ids []string + for _, row := range c.Cases { + if row.Package != ConformancePackage || row.Forge == DeferredForge { + continue + } + ids = append(ids, row.ID) + } + sort.Strings(ids) + return ids +} + +func knownAdapterList() string { + var names []string + for a := range KnownAdapters { + names = append(names, a) + } + sort.Strings(names) + return fmt.Sprint(names) +} + +// DiffCaseSets reports catalog rows with no executed case (missing) and executed +// cases with no catalog row (extra). Both directions matter: `missing` catches a +// case that stopped running while its row still claims coverage, and `extra` +// catches a case running with no catalogued disposition at all. +// +// It is a pure function on two slices precisely so it can be MUTATION-TESTED — +// the gate that uses it feeds it real data, and TestDiffCaseSetsDetectsDrift +// feeds it deliberately broken data and requires it to complain. A comparison +// that only ever sees matching inputs has never been shown to be able to fail. +func DiffCaseSets(catalog, executed []string) (missing, extra []string) { + inExec := map[string]bool{} + for _, id := range executed { + inExec[id] = true + } + inCat := map[string]bool{} + for _, id := range catalog { + inCat[id] = true + } + for _, id := range catalog { + if !inExec[id] { + missing = append(missing, id) + } + } + for _, id := range executed { + if !inCat[id] { + extra = append(extra, id) + } + } + sort.Strings(missing) + sort.Strings(extra) + return missing, extra +} diff --git a/internal/forge/conformance/catalog.yaml b/internal/forge/conformance/catalog.yaml index b00b5262..273a57f5 100644 --- a/internal/forge/conformance/catalog.yaml +++ b/internal/forge/conformance/catalog.yaml @@ -13,6 +13,7 @@ cases: test: TestConformanceTargetAdvancedRejected package: internal/forge/conformance forge: gitlab + adapters: [fake, gitlab] - id: sha-guard-source-moved adr: ADR-0015 §2 @@ -21,6 +22,7 @@ cases: test: TestConformanceSourceMovedRejected package: internal/forge/conformance forge: gitlab + adapters: [fake, gitlab] # --- E4-S09 P3-E5 reconciliation replay (ADR-0019) --- - id: p3e5-rerun-idempotence @@ -30,6 +32,7 @@ cases: test: TestConformanceRerunIdempotence package: internal/forge/conformance forge: gitlab + adapters: [fake, gitlab] - id: p3e5-duplicate-repair adr: ADR-0019 @@ -38,6 +41,7 @@ cases: test: TestConformanceDuplicateRepair package: internal/forge/conformance forge: gitlab + adapters: [fake, gitlab] - id: p3e5-spoofed-marker-ignored adr: ADR-0019 @@ -46,6 +50,7 @@ cases: test: TestConformanceSpoofedMarkerIgnored package: internal/forge/conformance forge: gitlab + adapters: [fake, gitlab] # --- E4-S08 assent-policy self-vouch (ADR-0015 §1) --- - id: assent-policy-self-edit-block @@ -55,6 +60,7 @@ cases: test: TestRunAssentPolicySelfModificationBlocks package: cmd/assent forge: gitlab + adapters: [gitlab] - id: policy-from-target-ref-only adr: ADR-0015 §1 @@ -63,6 +69,7 @@ cases: test: TestRunPolicyFromTargetRefOnly package: cmd/assent forge: gitlab + adapters: [gitlab] # --- AUD-S01 changed-file enumeration completeness (ADR-0020, D-119) --- # REQUIRED, not optional: in checkout-less runs the snapshot changed-file list @@ -75,6 +82,7 @@ cases: test: TestRunEnumerationIncompleteNeverApproves package: cmd/assent forge: gitlab + adapters: [gitlab] note: truncated enumeration → REVIEW changeset.undecidable, one thread, zero approve/merge - id: enumeration-incomplete-visible-policy-block @@ -84,6 +92,7 @@ cases: test: TestRunEnumerationIncompleteNeverApproves package: cmd/assent forge: gitlab + adapters: [gitlab] note: subtest visible_policy_path_in_partial_list_still_blocks — partial list carrying `.assent/**` still BLOCKs (GUARD 1 over the gap-degrade), zero writes - id: enumeration-diff-endpoint-404-hard-error @@ -93,6 +102,7 @@ cases: test: TestRunEnumerationIncompleteNeverApproves package: cmd/assent forge: gitlab + adapters: [gitlab] note: subtest diff_endpoint_404_is_hard_error_with_no_record — run exits non-zero, no DecisionRecord emitted, zero forge writes - id: enumeration-diff-endpoint-non200-adapter @@ -102,6 +112,7 @@ cases: test: TestChangedFiles404IsError package: internal/forge/gitlab forge: gitlab + adapters: [gitlab] note: adapter level — 404/403/429/500 on the diffs endpoint all raise a hard error; the 404→empty-list mapping is removed # --- E4-S05 doctor forge probe (ADR-0015 §4) — catalog-index only --- @@ -112,6 +123,7 @@ cases: test: TestDoctorForgeInsecureCITopology package: cmd/assent forge: gitlab + adapters: [gitlab] # --- E7-S03 new hermetic adversarial cases --- - id: fork-context-advisory-only @@ -121,6 +133,7 @@ cases: test: TestRunForkContextAdvisoryOnly package: cmd/assent forge: gitlab + adapters: [gitlab] - id: expired-fact-blocks-arming adr: ADR-0017 §4 @@ -129,6 +142,7 @@ cases: test: TestRunExpiredFactBlocksArming package: cmd/assent forge: gitlab + adapters: [gitlab] # --- GitHub-deferred (E10) — catalog stubs only --- - id: github-pull-request-fork-advisory @@ -138,6 +152,7 @@ cases: test: TestLiveGitHubForkAdvisoryOnly package: test/e2e forge: github-deferred + adapters: [] note: pull_request from fork → read-only token, advisory-only (forge-dossier-github C17) - id: github-merge-queue-sha-guard @@ -147,4 +162,5 @@ cases: test: TestLiveGitHubMergeQueueSHAGuard package: test/e2e forge: github-deferred + adapters: [] note: merge-queue merge-result digest pinning — live proof deferred to E10 diff --git a/internal/forge/conformance/catalog_test.go b/internal/forge/conformance/catalog_test.go new file mode 100644 index 00000000..ffc6c69e --- /dev/null +++ b/internal/forge/conformance/catalog_test.go @@ -0,0 +1,132 @@ +package conformance + +import ( + "sort" + "strings" + "testing" +) + +// TestCatalogMatchesExecutedCases is REQ-E10-S01-02: extraction is a refactor, so +// the set of executed case IDs must equal the catalog's claim, with no case +// missing and none extra. +// +// The denominator is OBSERVED EXECUTION — ExecutedCaseIDs actually runs the suite +// and records each case as it is dispatched — not a hand-maintained list and not +// a scan for test-function names in source. That distinction is the whole point. +// A gate keyed on names would stay green for a case that still exists as a +// function but has been unhooked from the runner, which is a predicate over TEXT +// standing in for a property that is STRUCTURAL. This repo has paid for that +// substitution repeatedly; see D-164. +func TestCatalogMatchesExecutedCases(t *testing.T) { + cat, err := LoadCatalog(catalogPath) + if err != nil { + t.Fatalf("load catalog: %v", err) + } + executed := RunSuite(tbT{t}, fakeFactory) + sort.Strings(executed) + + missing, extra := DiffCaseSets(cat.SuiteCaseIDs(), executed) + if len(missing) > 0 { + t.Errorf("catalog rows never executed by RunSuite: %v", missing) + } + if len(extra) > 0 { + t.Errorf("RunSuite executed cases with no catalog row: %v", extra) + } + if len(executed) == 0 { + t.Fatal("RunSuite executed nothing — the denominator is empty, so this gate proves nothing") + } +} + +// TestDiffCaseSetsDetectsDrift is the MUTATION CONTROL for the gate above. It +// feeds DiffCaseSets the two failure shapes and requires each to be reported. If +// someone reduces the comparison to a length check, or to a one-directional +// containment test, this reds. +func TestDiffCaseSetsDetectsDrift(t *testing.T) { + full := []string{"a", "b", "c"} + + // A case unhooked from the runner while its catalog row remains. + missing, extra := DiffCaseSets(full, []string{"a", "c"}) + if len(missing) != 1 || missing[0] != "b" { + t.Fatalf("unhooked case must be reported missing, got missing=%v extra=%v", missing, extra) + } + + // A case running with no catalogued disposition. + missing, extra = DiffCaseSets([]string{"a", "b"}, full) + if len(extra) != 1 || extra[0] != "c" { + t.Fatalf("uncatalogued case must be reported extra, got missing=%v extra=%v", missing, extra) + } + + // Equal-length but disjoint: the shape a naive count comparison misses entirely. + missing, extra = DiffCaseSets([]string{"a", "b"}, []string{"a", "z"}) + if len(missing) != 1 || len(extra) != 1 { + t.Fatalf("same-size divergence must be reported on both axes, got missing=%v extra=%v", missing, extra) + } +} + +// TestCatalogStrictDecode is REQ-E10-S01-03: the loader rejects an unknown +// adapter name rather than silently ignoring it, and rejects unknown fields. +func TestCatalogStrictDecode(t *testing.T) { + if _, err := LoadCatalog(catalogPath); err != nil { + t.Fatalf("the shipped catalog must decode strictly: %v", err) + } + + for _, tc := range []struct { + name string + yaml string + wantSub string + }{ + { + name: "unknown adapter name", + yaml: `cases: + - id: bogus + level: L1 + req: REQ-X + test: TestX + package: internal/forge/conformance + forge: gitlab + adapters: [gitlub] +`, + wantSub: "unknown adapter", + }, + { + name: "unknown field", + yaml: `cases: + - id: bogus + level: L1 + req: REQ-X + test: TestX + package: internal/forge/conformance + forge: gitlab + adapters: [gitlab] + frge: typo +`, + wantSub: "field frge not found", + }, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := DecodeCatalog([]byte(tc.yaml)) + if err == nil { + t.Fatalf("strict decode must reject %s, got nil error", tc.name) + } + if !strings.Contains(err.Error(), tc.wantSub) { + t.Fatalf("error must name the cause; want substring %q, got %v", tc.wantSub, err) + } + }) + } + + // POSITIVE CONTROL: an otherwise identical row with a KNOWN adapter decodes. + // Without this, every rejection above would also be satisfied by a loader that + // rejects everything. + ok := `cases: + - id: fine + level: L1 + req: REQ-X + test: TestX + package: internal/forge/conformance + forge: gitlab + adapters: [gitlab, github] +` + if _, err := DecodeCatalog([]byte(ok)); err != nil { + t.Fatalf("a well-formed row must decode, got %v", err) + } +} diff --git a/internal/forge/conformance/doc.go b/internal/forge/conformance/doc.go index c43fd6d6..89bbccf3 100644 --- a/internal/forge/conformance/doc.go +++ b/internal/forge/conformance/doc.go @@ -1,4 +1,11 @@ -// Package conformance holds L2 conformance goldens for the forge port (ADR-0005). +// Package conformance holds the forge-neutral L2 conformance suite (ADR-0005). +// +// The suite is IMPORTABLE (E10-S01): case bodies live in ordinary Go, not in +// `_test.go`, so a second adapter conformance-tests itself by supplying a +// Factory and calling RunSuite — no case is copied, so none can drift. Before +// this, duplication was the only option, which is why D-084's `github-deferred` +// rows were unflippable. +// // Cases exercise forge.Reconcile through the hermetic fake and gitlab httptest, // proving executable contract for: // @@ -8,4 +15,14 @@ // crash-then-rerun gap-fill without duplication, deterministic duplicate // repair, and contributor marker spoofing ignored by the author-identity // filter on ListBotThreads. +// +// Three gates keep the suite honest, because a conformance suite that has +// stopped proving its property still reports PASS: +// +// - TestCatalogMatchesExecutedCases — the catalog is checked against OBSERVED +// EXECUTION, so a case unhooked from the runner cannot stay accounted for. +// - TestEveryCaseCanFail — every case must go red against a sabotaged backend, +// on every adapter. +// - TestEveryObservationIsLoadBearing — corrupting any single value a case +// reads must flip its verdict, so no observation is dead weight. package conformance diff --git a/internal/forge/conformance/fixtures.go b/internal/forge/conformance/fixtures.go new file mode 100644 index 00000000..80dbbc8c --- /dev/null +++ b/internal/forge/conformance/fixtures.go @@ -0,0 +1,187 @@ +package conformance + +import ( + "github.com/PlatformRelay/assent/internal/core/decision" + "github.com/PlatformRelay/assent/internal/forge" + "github.com/PlatformRelay/assent/internal/render" +) + +// fixtures.go holds the conformance suite's FIXTURE VOCABULARY — the markers, +// summaries and desired-states replayed from the P3-E5 publication-protocol +// contract fixtures. Moved out of `_test.go` by E10-S01 (REQ-E10-S01-01) for one +// reason only: Go cannot import a `_test.go` file, so while these lived there the +// only way to conformance-test a second adapter was to COPY them — which +// guarantees drift and is why D-084's `github-deferred` rows were unflippable. +// +// Nothing here is new and nothing is changed: each helper is the byte-identical +// body it had in `reconciliation_test.go`, fixture line references included, so a +// reviewer can diff rather than re-derive. + +const decHex = "sha256:1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaaa" + +// ---- rerun-idempotence.yaml (docs/contracts/p3-e5-publication-protocol/fixtures/rerun-idempotence.yaml) ---- + +const occChallenge = "sha256:c6957a516c95532386bed08f56441dfbb8d18efda24f5abdab1e48437aa3357d" // line 23 +const occComment = "sha256:1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaaa" // line 25 + +func rerunChallengeMarker() forge.Marker { + return forge.Marker{ + Slot: forge.Slot{ + Project: proj, + MR: mrIID, + Rule: "topic-safety/retention-shrink-challenge", + Effect: "challenge", + EntryRef: "topic-registry:orders.events.v1", + }, + Occurrence: occChallenge, + Decision: decHex, + Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, + } +} + +func rerunCommentMarker() forge.Marker { + return forge.Marker{ + Slot: forge.Slot{ + Project: proj, + MR: mrIID, + Rule: "ownership/entry-owner-required", + Effect: "comment", + EntryRef: "topic-registry:orders.events.v1", + }, + Occurrence: occComment, + Decision: decHex, + Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, + } +} + +// ---- crash-then-rerun.yaml ---- + +const occCrashChallenge = "sha256:aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111" // line 23 +const occCrashComment = "sha256:bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222" // line 25 + +func crashChallengeMarker() forge.Marker { + return forge.Marker{ + Slot: forge.Slot{ + Project: "platform/orders-service", + MR: "551", + Rule: "topic-safety/retention-shrink-challenge", + Effect: "challenge", + EntryRef: "topic-registry:payments.events.v1", + }, + Occurrence: occCrashChallenge, + Decision: decHex, + Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, + } +} + +func crashCommentMarker() forge.Marker { + return forge.Marker{ + Slot: forge.Slot{ + Project: "platform/orders-service", + MR: "551", + Rule: "ownership/entry-owner-required", + Effect: "comment", + EntryRef: "topic-registry:payments.events.v1", + }, + Occurrence: occCrashComment, + Decision: decHex, + Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, + } +} + +// ---- duplicate-repair.yaml ---- + +const occDup = "sha256:dddd4444dddd4444dddd4444dddd4444dddd4444dddd4444dddd4444dddd4444" // line 38 + +func dupMarker() forge.Marker { + return forge.Marker{ + Slot: forge.Slot{ + Project: "platform/orders-service", + MR: "612", + Rule: "topic-safety/retention-shrink-challenge", + Effect: "challenge", + EntryRef: "topic-registry:orders.events.v1", + }, + Occurrence: occDup, + Decision: decHex, + Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, + } +} + +func rerunSummaryMarker() forge.Marker { + return forge.Marker{ + Slot: forge.Slot{ + Project: proj, + MR: mrIID, + Rule: "assent/summary", + Effect: "comment", + }, + Occurrence: decHex, + Decision: decHex, + Artifact: forge.Artifact{Kind: "summary-comment", SchemaVersion: "v1alpha1"}, + } +} + +func crashSummaryMarker() forge.Marker { + return forge.Marker{ + Slot: forge.Slot{ + Project: "platform/orders-service", + MR: "551", + Rule: "assent/summary", + Effect: "comment", + }, + Occurrence: decHex, + Decision: decHex, + Artifact: forge.Artifact{Kind: "summary-comment", SchemaVersion: "v1alpha1"}, + } +} + +func fixtureSummaryBody() string { + pm := decision.PresentationModel{ + APIVersion: "assent.dev/v1alpha1", + Kind: "PresentationModel", + Decision: "REVIEW", + Findings: []decision.Finding{{ + Rule: "topic-safety/retention-shrink-challenge", + Effect: "challenge", + Subject: "topic-registry:orders.events.v1", + Code: "retention-shrink", + Points: 10, + }}, + } + body, err := render.RenderSummary(pm, render.Context{ + Options: render.DefaultOptions(), + RiskThreshold: 10, + }) + if err != nil { + panic(err) + } + return body +} + +func rerunSummary() *forge.DesiredSummary { + return &forge.DesiredSummary{ + Marker: rerunSummaryMarker(), + Body: fixtureSummaryBody(), + } +} + +func crashSummary() *forge.DesiredSummary { + return &forge.DesiredSummary{ + Marker: crashSummaryMarker(), + Body: fixtureSummaryBody(), + } +} + +func desiredThreadFor(m forge.Marker, summary *forge.DesiredSummary) forge.DesiredReviewState { + return forge.DesiredReviewState{ + Project: m.Slot.Project, + MR: m.Slot.MR, + Thread: &forge.DesiredThread{Marker: m, Body: "obligation not proven"}, + Summary: summary, + } +} + +func dupDesired() forge.DesiredReviewState { + return desiredThreadFor(dupMarker(), nil) +} diff --git a/internal/forge/conformance/helpers_test.go b/internal/forge/conformance/helpers_test.go deleted file mode 100644 index 533437af..00000000 --- a/internal/forge/conformance/helpers_test.go +++ /dev/null @@ -1,17 +0,0 @@ -package conformance - -import "time" - -const ( - botID = "assent-bot" - proj = "platform/orders-service" - mrIID = "482" -) - -type fixedClock struct{ t time.Time } - -func (c fixedClock) Now() time.Time { return c.t } - -func testClock() fixedClock { - return fixedClock{t: time.Date(2026, 7, 26, 10, 0, 0, 0, time.UTC)} -} diff --git a/internal/forge/conformance/helpers_unit_test.go b/internal/forge/conformance/helpers_unit_test.go new file mode 100644 index 00000000..f9b347d3 --- /dev/null +++ b/internal/forge/conformance/helpers_unit_test.go @@ -0,0 +1,71 @@ +package conformance + +import ( + "strings" + "testing" + + "github.com/PlatformRelay/assent/internal/forge" +) + +// helpers_unit_test.go covers the pure helpers the extraction moved into +// importable Go. They were previously `_test.go` bodies exercised only as a side +// effect of a case running, so their fallback branches had never been executed by +// anything — moving them into production code made that visible rather than +// creating it. + +func TestLoadCatalogMissingFileIsAnError(t *testing.T) { + _, err := LoadCatalog("does-not-exist.yaml") + if err == nil { + t.Fatal("a missing catalog must be an error, not an empty catalog — an empty " + + "catalog would make the coverage gate vacuously pass") + } + if !strings.Contains(err.Error(), "conformance catalog") { + t.Fatalf("error must name the subsystem, got %v", err) + } +} + +func TestDecodeCatalogRejectsMalformedYAML(t *testing.T) { + if _, err := DecodeCatalog([]byte("cases: [oh: no: yes")); err == nil { + t.Fatal("malformed YAML must be rejected") + } +} + +// TestThreadOpTargetFallbacks pins the selection order: the operation matching the +// wanted id wins; failing that the FIRST operation is reported; failing that the +// empty string. The middle branch is what makes a receipt with operations but no +// match distinguishable from an empty receipt. +func TestThreadOpTargetFallbacks(t *testing.T) { + want := forge.PublicationReceipt{Operations: []forge.Operation{ + {TargetID: "note/1"}, {TargetID: "note/2"}, + }} + if got := threadOpTarget(want, "note/2"); got != "note/2" { + t.Fatalf("exact match must win, got %q", got) + } + if got := threadOpTarget(want, "note/absent"); got != "note/1" { + t.Fatalf("no match must fall back to the first operation, got %q", got) + } + if got := threadOpTarget(forge.PublicationReceipt{}, "note/1"); got != "" { + t.Fatalf("an empty receipt must yield the empty string, got %q", got) + } +} + +func TestSuiteCaseIDsExcludesOtherPackagesAndDeferred(t *testing.T) { + c := Catalog{Cases: []CatalogCase{ + {ID: "in-suite", Package: ConformancePackage, Forge: "gitlab"}, + {ID: "other-package", Package: "cmd/assent", Forge: "gitlab"}, + {ID: "deferred", Package: ConformancePackage, Forge: DeferredForge}, + }} + got := c.SuiteCaseIDs() + if len(got) != 1 || got[0] != "in-suite" { + t.Fatalf("the denominator must be conformance-package, non-deferred rows only; got %v", got) + } +} + +// TestKnownAdapterListIsSorted keeps the error message deterministic — an +// unordered map render would make the strict-decode failure text flaky. +func TestKnownAdapterListIsSorted(t *testing.T) { + got := knownAdapterList() + if got != "[fake github gitlab]" { + t.Fatalf("adapter list must render sorted and complete, got %q", got) + } +} diff --git a/internal/forge/conformance/ids.go b/internal/forge/conformance/ids.go new file mode 100644 index 00000000..a4443d70 --- /dev/null +++ b/internal/forge/conformance/ids.go @@ -0,0 +1,26 @@ +package conformance + +import "time" + +// ids.go holds the fixed identities and the frozen clock the conformance cases +// run under. Moved verbatim out of `helpers_test.go` by E10-S01: the fixtures and +// replay bodies that reference these are now importable non-test Go, so their +// constants must be too. +// +// The clock is FIXED, not injected from the wall: hard rule 7 forbids wall-clock +// dependence in the decision path, and a conformance suite that drifted with the +// clock could not be a golden. + +const ( + botID = "assent-bot" + proj = "platform/orders-service" + mrIID = "482" +) + +type fixedClock struct{ t time.Time } + +func (c fixedClock) Now() time.Time { return c.t } + +func testClock() fixedClock { + return fixedClock{t: time.Date(2026, 7, 26, 10, 0, 0, 0, time.UTC)} +} diff --git a/internal/forge/conformance/loadbearing_test.go b/internal/forge/conformance/loadbearing_test.go new file mode 100644 index 00000000..c96ac1c3 --- /dev/null +++ b/internal/forge/conformance/loadbearing_test.go @@ -0,0 +1,200 @@ +package conformance + +import ( + "fmt" + "sort" + "testing" +) + +// loadbearing_test.go strengthens the can-fail gate from "the case notices SOME +// violation" to "EVERY observation the case makes is load-bearing". +// +// TestEveryCaseCanFail sabotages everything at once, so a case passes it by +// noticing a single violation while its remaining assertions are dead. That is +// not a hypothetical failure mode — it is what a case looks like after someone +// weakens one assertion to accommodate a backend and leaves the rest in place. +// +// The gate below needs no hand-maintained expectations, which matters because a +// pinned list of "assertions case X makes" is a predicate over text that drifts +// from the code the moment either changes. Instead it RECORDS which observations +// a case actually calls, then corrupts them ONE AT A TIME and requires each to +// change the verdict. What is asserted is a property of the run, not a +// description of the source. + +type observation string + +// recordingObserver notes which observations a case actually consults. +type recordingObserver struct { + real Observer + called map[observation]bool +} + +func (r *recordingObserver) note(name observation) { r.called[name] = true } + +func (r *recordingObserver) MergeAttempts() int { + r.note("MergeAttempts") + return r.real.MergeAttempts() +} +func (r *recordingObserver) MergesPerformed() int { + r.note("MergesPerformed") + return r.real.MergesPerformed() +} +func (r *recordingObserver) Approvals() int { r.note("Approvals"); return r.real.Approvals() } +func (r *recordingObserver) ThreadsCreated() int { + r.note("ThreadsCreated") + return r.real.ThreadsCreated() +} +func (r *recordingObserver) ThreadsResolved() int { + r.note("ThreadsResolved") + return r.real.ThreadsResolved() +} +func (r *recordingObserver) NotesCreated() int { r.note("NotesCreated"); return r.real.NotesCreated() } +func (r *recordingObserver) NotesUpdated() int { r.note("NotesUpdated"); return r.real.NotesUpdated() } +func (r *recordingObserver) ThreadCount() int { r.note("ThreadCount"); return r.real.ThreadCount() } +func (r *recordingObserver) BotThreadCount() int { + r.note("BotThreadCount") + return r.real.BotThreadCount() +} +func (r *recordingObserver) OpenBotThreadCount() int { + r.note("OpenBotThreadCount") + return r.real.OpenBotThreadCount() +} +func (r *recordingObserver) NoteBody(id string) string { + r.note("NoteBody") + return r.real.NoteBody(id) +} +func (r *recordingObserver) IsResolved(id string) bool { + r.note("IsResolved") + return r.real.IsResolved(id) +} + +// corruptObserver returns a DIFFERENT value for exactly one observation and the +// truth for every other. Ints are offset by one and bools inverted, so the +// corruption is guaranteed to differ from the real answer whatever it is — a +// fixed sentinel like 0 or "" could coincide with the true value and produce a +// false green. +// It corrupts only the FIRST read of the target. A uniform offset is invisible to +// an assertion comparing a BEFORE and an AFTER of the same observation — both +// shift by the same amount and the delta is unchanged — which would report a +// perfectly good delta assertion as dead weight. Corrupting one read makes the +// delta move. +type corruptObserver struct { + real Observer + target observation + seen *int +} + +func (c corruptObserver) hit(name observation) bool { + if c.target != name { + return false + } + *c.seen++ + return *c.seen == 1 +} + +func (c corruptObserver) MergeAttempts() int { + return offsetIf(c.hit("MergeAttempts"), c.real.MergeAttempts()) +} +func (c corruptObserver) MergesPerformed() int { + return offsetIf(c.hit("MergesPerformed"), c.real.MergesPerformed()) +} +func (c corruptObserver) Approvals() int { + return offsetIf(c.hit("Approvals"), c.real.Approvals()) +} +func (c corruptObserver) ThreadsCreated() int { + return offsetIf(c.hit("ThreadsCreated"), c.real.ThreadsCreated()) +} +func (c corruptObserver) ThreadsResolved() int { + return offsetIf(c.hit("ThreadsResolved"), c.real.ThreadsResolved()) +} +func (c corruptObserver) NotesCreated() int { + return offsetIf(c.hit("NotesCreated"), c.real.NotesCreated()) +} +func (c corruptObserver) NotesUpdated() int { + return offsetIf(c.hit("NotesUpdated"), c.real.NotesUpdated()) +} +func (c corruptObserver) ThreadCount() int { + return offsetIf(c.hit("ThreadCount"), c.real.ThreadCount()) +} +func (c corruptObserver) BotThreadCount() int { + return offsetIf(c.hit("BotThreadCount"), c.real.BotThreadCount()) +} +func (c corruptObserver) OpenBotThreadCount() int { + return offsetIf(c.hit("OpenBotThreadCount"), c.real.OpenBotThreadCount()) +} + +func (c corruptObserver) NoteBody(id string) string { + body := c.real.NoteBody(id) + if c.hit("NoteBody") { + return body + "-corrupted" + } + return body +} + +func (c corruptObserver) IsResolved(id string) bool { + got := c.real.IsResolved(id) + if c.hit("IsResolved") { + return !got + } + return got +} + +func offsetIf(corrupt bool, v int) int { + if corrupt { + return v + 1 + } + return v +} + +// TestEveryObservationIsLoadBearing is the per-assertion mutation control. +func TestEveryObservationIsLoadBearing(t *testing.T) { + for _, c := range Cases() { + t.Run(c.ID, func(t *testing.T) { + // 1. Discover what this case actually observes, by running it. + called := map[observation]bool{} + recording := func(tb TB, cfg Config) Backend { + b := fakeFactory(tb, cfg) + return Backend{ + Port: b.Port, + Fixture: b.Fixture, + Observer: &recordingObserver{real: b.Observer, called: called}, + } + } + if failed, msg := runAndRecord(c, recording); failed { + t.Fatalf("case must pass against a conforming backend, got: %s", msg) + } + if len(called) == 0 { + t.Fatal("case consulted NO observation — it cannot be proving anything " + + "about what the forge actually did") + } + + // 2. Corrupt each observed value ALONE; each must flip the verdict. + var names []observation + for n := range called { + names = append(names, n) + } + sort.Slice(names, func(i, j int) bool { return names[i] < names[j] }) + + for _, name := range names { + t.Run(string(name), func(t *testing.T) { + seen := new(int) + corrupting := func(tb TB, cfg Config) Backend { + b := fakeFactory(tb, cfg) + return Backend{ + Port: b.Port, + Fixture: b.Fixture, + Observer: corruptObserver{real: b.Observer, target: name, seen: seen}, + } + } + failed, _ := runAndRecord(c, corrupting) + if !failed { + t.Fatalf("case %q read %s but does not ASSERT on it — corrupting the "+ + "value alone left the case green, so that observation is dead "+ + "weight and the property it implies is unproven", c.ID, name) + } + }) + } + t.Logf("%d load-bearing observation(s): %s", len(names), fmt.Sprint(names)) + }) + } +} diff --git a/internal/forge/conformance/observe.go b/internal/forge/conformance/observe.go new file mode 100644 index 00000000..c72772d6 --- /dev/null +++ b/internal/forge/conformance/observe.go @@ -0,0 +1,169 @@ +package conformance + +import "github.com/PlatformRelay/assent/internal/forge" + +// observe.go declares the PORT-LEVEL OBSERVATION SURFACE for the forge +// conformance suite (E10-S01, REQ-E10-S01-04). +// +// Why this file exists at all. Before extraction, the cases asserted directly on +// `*fake.Forge` internals — `sha_guard_test.go` read `f.Merges` / `f.Approvals`, +// and `reconciliation_test.go:220` type-asserted to `*fake.Forge` to read +// `NoteBody`. A `Factory` returning only a `forge.Forge` is therefore half a +// contract: the port says what a backend can be ASKED to do, and says nothing +// about what a case may OBSERVE afterwards. +// +// The cheap way to close that gap is to delete the assertions the port cannot +// express — keep only "Reconcile returned ErrSHAMoved" and drop "and it recorded +// zero merge attempts". That is exactly how a conformance suite silently stops +// proving the SHA-guard: a backend that returns the right error while merging +// anyway would pass. REQ-E10-S01-04 forbids that resolution, so the surface below +// is derived from what the cases ALREADY read, not from what is convenient to +// implement: +// +// f.Merges -> MergeAttempts() f.Approvals -> Approvals() +// ff.NoteBody -> NoteBody() f.IsResolved -> IsResolved() +// f.ThreadCount -> ThreadCount() +// +// Everything else the cases read (ListBotThreads, ListBotNotes) is already on +// `forge.Forge` and is deliberately NOT duplicated here. +// +// One assertion is STRENGTHENED rather than preserved, and that is intentional. +// `reconciliation_test.go:220` guarded its "summary must be updated in place" +// check behind `if ff, ok := f.(*fake.Forge); ok` — on the GitLab backend `ok` is +// false, the body never ran, and the check silently proved nothing. It was an +// assertion that could not fail for half the suite. `NoteBody` is on this +// interface so both adapters must answer it and the check becomes universal. +// (The GitLab harness already stores note bodies, so this costs it nothing.) + +// Observer is the recorded-writes view of a backend, for assertions the +// `forge.Forge` port cannot express. Both adapters implement it in full; there is +// no optional method and no "unsupported" return, because an adapter allowed to +// opt out of an observation is an adapter allowed to opt out of the proof. +type Observer interface { + // MergeAttempts is the number of times MergeCAS was INVOKED, whether or not + // it merged. MergesPerformed is the number that actually merged. Both are on + // this interface because the two SHA-guard cases need DIFFERENT answers from + // them, and a single counter cannot express either honestly: + // + // target advanced -> the CurrentHeads pre-check fails closed BEFORE any + // forge mutation, so MergeCAS is never called at all: + // attempts 0, performed 0. + // source moved -> the pre-check passes, the head moves inside the TOCTOU + // window, and the ATOMIC CAS guard refuses: + // attempts 1, performed 0. + // + // Collapsing these to "no merge happened" is precisely the weakening + // REQ-E10-S01-04 exists to prevent: it would pass a backend that skipped the + // pre-check entirely and leaned on the CAS, and it would equally pass one + // that never reached the CAS guard at all. The pre-existing suite asserted + // only `len(f.Merges) == 0` — the performed count — so which of the two + // guards actually fired was never established on either case. + MergeAttempts() int + + // MergesPerformed is the number of merges that actually completed. It must + // be zero in every fail-closed case. + MergesPerformed() int + + // Approvals is the number of approvals recorded on the MR. + Approvals() int + + // ThreadsCreated, ThreadsResolved, NotesCreated and NotesUpdated are WRITE-CALL + // counters. They exist because the pre-extraction GitLab subtests asserted on + // exactly these (`h.createCalls`, `h.resolveCalls`, `h.noteCreateCalls`, + // `h.noteUpdateCalls`) while the fake subtests could not, so "a rerun must not + // POST a new discussion" was proven on ONE backend only. Putting them on the + // shared surface makes those assertions universal instead of adapter-local — + // a strengthening, and the direction REQ-E10-S01-04 requires. + ThreadsCreated() int + ThreadsResolved() int + NotesCreated() int + NotesUpdated() int + + // BotThreadCount is the number of bot-authored threads, and + // OpenBotThreadCount the number of those still unresolved. Both are derivable + // from `forge.Forge.ListBotThreads`, but duplicate-repair asserts on the OPEN + // count specifically — "exactly one open occupant remains per slot" — which + // the port's list cannot answer without re-implementing the filter in every + // case body. + BotThreadCount() int + OpenBotThreadCount() int + + // NoteBody returns the full rendered body of the bot note with this id, or + // "" if there is no such note. Used to prove a summary was updated IN PLACE + // rather than re-created. + NoteBody(id string) string + + // IsResolved reports whether the thread with this id is resolved. + IsResolved(id string) bool + + // ThreadCount is the total number of threads on the MR, bot-authored or + // not. Contrast `forge.Forge.ListBotThreads`, which is author-filtered: the + // spoofing cases need BOTH, because "the contributor thread still exists but + // is invisible to the bot filter" is a different claim from "no thread + // exists". + ThreadCount() int +} + +// Fixture is the backend-neutral ARRANGE surface: how a case puts a backend into +// the state it wants to act on. Seeding is not part of `forge.Forge` because a +// production adapter has no business creating a contributor-authored thread. +type Fixture interface { + // SeedThread pre-creates a thread authored by `author` carrying `marker`. + // `author` is deliberately a parameter and not fixed to the bot: the + // marker-spoofing cases turn on a CONTRIBUTOR posting a well-formed marker. + SeedThread(id, author string, marker forge.Marker, resolved bool) error + + // SeedNote pre-creates a non-resolvable MR note authored by `author`. + SeedNote(id, author string, marker forge.Marker, body string) error + + // MoveTargetHead moves the target-branch tip to `sha` IMMEDIATELY. Distinct + // from DriftSourceHeadAfterRead below, which fires inside the TOCTOU window: + // the target-advanced case needs the move to have ALREADY happened when + // Reconcile takes its pre-check read, so that the pre-check is what refuses. + MoveTargetHead(sha string) + + // Pins reports the merge pins matching the backend's CURRENT state — what an + // evaluation would have recorded if it ran right now. Cases take their pins + // from here rather than hardcoding them, because the pin VALUES are + // adapter-owned: GitLab has no merge-result digest and synthesises one from + // source+target (`gitlab.SyntheticDigest`), so a case with a literal digest + // can only ever run against the fake. That is not a hypothetical — it is why + // the two SHA-guard cases were fake-only before this story, despite their + // catalog rows claiming `forge: gitlab`. Collapsing the synthetic digest onto + // a real one is E10-S03's job; making the cases indifferent to it is this + // story's. + Pins() forge.DesiredMerge + + // DriftSourceHeadAfterRead makes the MR's source head change to `sha` AFTER + // Reconcile's CurrentHeads pre-check read and BEFORE MergeCAS — the TOCTOU + // window the source-moved case exists to exercise. A backend that cannot + // model the window cannot host that case honestly, so this is a required + // method rather than an optional hook. + DriftSourceHeadAfterRead(sha string) +} + +// Backend is one constructed backend under test: the port a case drives, plus +// the two surfaces it arranges and observes through. +type Backend struct { + Port forge.Forge + Fixture Fixture + Observer Observer +} + +// Config is the initial state a case asks a Factory to construct. Every field is +// explicit: a Factory must never default a SHA, because "the pins the case +// intended" versus "whatever the backend happened to start with" is precisely +// what the SHA-guard cases discriminate. +type Config struct { + Project string + MR string + + // BotAuthor is the identity ListBotThreads/ListBotNotes filter on. + BotAuthor string + + // The backend's CURRENT heads, which may deliberately differ from the pins a + // case passes to Reconcile. + CurrentSourceSHA string + CurrentTargetSHA string + CurrentMergeResultDigest string +} diff --git a/internal/forge/conformance/portcount.go b/internal/forge/conformance/portcount.go new file mode 100644 index 00000000..546e3bbb --- /dev/null +++ b/internal/forge/conformance/portcount.go @@ -0,0 +1,70 @@ +package conformance + +import "github.com/PlatformRelay/assent/internal/forge" + +// portcount.go counts PORT-LEVEL calls by decorating any `forge.Forge`. +// +// Counting at the port rather than inside each adapter is what makes the +// observation surface adapter-independent: a new backend gets merge/approve/ +// thread counters for free, and cannot report them dishonestly, because it never +// implements them. That matters more than it first appears — the two counts the +// SHA-guard cases turn on are NOT observable at the transport layer for GitLab: +// its `MergeCAS` re-reads the heads and refuses client-side, so a moved head +// produces ZERO merge HTTP requests. An httptest harness counting PUTs would +// report 0 attempts where the port was called once, and the case asserting "the +// CAS guard was reached and refused" would be measuring the wrong thing. +// +// UpsertComment is deliberately NOT counted here: create-vs-update is one port +// method with two outcomes, and only the backend knows which occurred. Those two +// stay adapter-reported. +type countingPort struct { + forge.Forge + + mergeAttempts int + mergesPerformed int + approvals int + threadsCreated int + threadsResolved int +} + +func newCountingPort(inner forge.Forge) *countingPort { + return &countingPort{Forge: inner} +} + +func (c *countingPort) MergeCAS(project, mr string, m forge.DesiredMerge) (string, error) { + // Incremented BEFORE delegating: a refused CAS is still an attempt. Counting + // after a successful return would make the two indistinguishable. + c.mergeAttempts++ + id, err := c.Forge.MergeCAS(project, mr, m) + if err == nil { + c.mergesPerformed++ + } + return id, err +} + +func (c *countingPort) Approve(project, mr string) (string, error) { + id, err := c.Forge.Approve(project, mr) + if err == nil { + c.approvals++ + } + return id, err +} + +func (c *countingPort) CreateThread(project, mr string, marker forge.Marker, body string) (forge.Thread, error) { + th, err := c.Forge.CreateThread(project, mr, marker, body) + if err == nil { + c.threadsCreated++ + } + return th, err +} + +func (c *countingPort) ResolveThread(project, mr, id string) error { + err := c.Forge.ResolveThread(project, mr, id) + if err == nil { + c.threadsResolved++ + } + return err +} + +// static assertion that the decorator still satisfies the port it wraps. +var _ forge.Forge = (*countingPort)(nil) diff --git a/internal/forge/conformance/reconciliation_test.go b/internal/forge/conformance/reconciliation_test.go index e0563527..aa308918 100644 --- a/internal/forge/conformance/reconciliation_test.go +++ b/internal/forge/conformance/reconciliation_test.go @@ -12,303 +12,11 @@ import ( "testing" "time" - "github.com/PlatformRelay/assent/internal/core/decision" "github.com/PlatformRelay/assent/internal/forge" - "github.com/PlatformRelay/assent/internal/forge/fake" gitlab "github.com/PlatformRelay/assent/internal/forge/gitlab" "github.com/PlatformRelay/assent/internal/render" ) -const decHex = "sha256:1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaaa" - -// ---- rerun-idempotence.yaml (docs/contracts/p3-e5-publication-protocol/fixtures/rerun-idempotence.yaml) ---- - -const occChallenge = "sha256:c6957a516c95532386bed08f56441dfbb8d18efda24f5abdab1e48437aa3357d" // line 23 -const occComment = "sha256:1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaa1111aaaa" // line 25 - -func rerunChallengeMarker() forge.Marker { - return forge.Marker{ - Slot: forge.Slot{ - Project: proj, - MR: mrIID, - Rule: "topic-safety/retention-shrink-challenge", - Effect: "challenge", - EntryRef: "topic-registry:orders.events.v1", - }, - Occurrence: occChallenge, - Decision: decHex, - Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, - } -} - -func rerunCommentMarker() forge.Marker { - return forge.Marker{ - Slot: forge.Slot{ - Project: proj, - MR: mrIID, - Rule: "ownership/entry-owner-required", - Effect: "comment", - EntryRef: "topic-registry:orders.events.v1", - }, - Occurrence: occComment, - Decision: decHex, - Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, - } -} - -// ---- crash-then-rerun.yaml ---- - -const occCrashChallenge = "sha256:aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111" // line 23 -const occCrashComment = "sha256:bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222" // line 25 - -func crashChallengeMarker() forge.Marker { - return forge.Marker{ - Slot: forge.Slot{ - Project: "platform/orders-service", - MR: "551", - Rule: "topic-safety/retention-shrink-challenge", - Effect: "challenge", - EntryRef: "topic-registry:payments.events.v1", - }, - Occurrence: occCrashChallenge, - Decision: decHex, - Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, - } -} - -func crashCommentMarker() forge.Marker { - return forge.Marker{ - Slot: forge.Slot{ - Project: "platform/orders-service", - MR: "551", - Rule: "ownership/entry-owner-required", - Effect: "comment", - EntryRef: "topic-registry:payments.events.v1", - }, - Occurrence: occCrashComment, - Decision: decHex, - Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, - } -} - -// ---- duplicate-repair.yaml ---- - -const occDup = "sha256:dddd4444dddd4444dddd4444dddd4444dddd4444dddd4444dddd4444dddd4444" // line 38 - -func dupMarker() forge.Marker { - return forge.Marker{ - Slot: forge.Slot{ - Project: "platform/orders-service", - MR: "612", - Rule: "topic-safety/retention-shrink-challenge", - Effect: "challenge", - EntryRef: "topic-registry:orders.events.v1", - }, - Occurrence: occDup, - Decision: decHex, - Artifact: forge.Artifact{Kind: "finding-thread", SchemaVersion: "v1alpha1"}, - } -} - -func rerunSummaryMarker() forge.Marker { - return forge.Marker{ - Slot: forge.Slot{ - Project: proj, - MR: mrIID, - Rule: "assent/summary", - Effect: "comment", - }, - Occurrence: decHex, - Decision: decHex, - Artifact: forge.Artifact{Kind: "summary-comment", SchemaVersion: "v1alpha1"}, - } -} - -func crashSummaryMarker() forge.Marker { - return forge.Marker{ - Slot: forge.Slot{ - Project: "platform/orders-service", - MR: "551", - Rule: "assent/summary", - Effect: "comment", - }, - Occurrence: decHex, - Decision: decHex, - Artifact: forge.Artifact{Kind: "summary-comment", SchemaVersion: "v1alpha1"}, - } -} - -func fixtureSummaryBody() string { - pm := decision.PresentationModel{ - APIVersion: "assent.dev/v1alpha1", - Kind: "PresentationModel", - Decision: "REVIEW", - Findings: []decision.Finding{{ - Rule: "topic-safety/retention-shrink-challenge", - Effect: "challenge", - Subject: "topic-registry:orders.events.v1", - Code: "retention-shrink", - Points: 10, - }}, - } - body, err := render.RenderSummary(pm, render.Context{ - Options: render.DefaultOptions(), - RiskThreshold: 10, - }) - if err != nil { - panic(err) - } - return body -} - -func rerunSummary() *forge.DesiredSummary { - return &forge.DesiredSummary{ - Marker: rerunSummaryMarker(), - Body: fixtureSummaryBody(), - } -} - -func crashSummary() *forge.DesiredSummary { - return &forge.DesiredSummary{ - Marker: crashSummaryMarker(), - Body: fixtureSummaryBody(), - } -} - -func desiredThreadFor(m forge.Marker, summary *forge.DesiredSummary) forge.DesiredReviewState { - return forge.DesiredReviewState{ - Project: m.Slot.Project, - MR: m.Slot.MR, - Thread: &forge.DesiredThread{Marker: m, Body: "obligation not proven"}, - Summary: summary, - } -} - -func dupDesired() forge.DesiredReviewState { - return desiredThreadFor(dupMarker(), nil) -} - -func replayRerunIdempotence(f forge.Forge) (newArtifacts int, err error) { - before, err := botThreadCount(f, proj, mrIID) - if err != nil { - return 0, err - } - beforeSummaries, err := botSummaryCount(f, proj, mrIID) - if err != nil { - return 0, err - } - for _, m := range []forge.Marker{rerunChallengeMarker(), rerunCommentMarker()} { - r, err := forge.Reconcile(f, testClock(), desiredThreadFor(m, rerunSummary()), forge.Preconditions{}) - if err != nil { - return 0, fmt.Errorf("Reconcile(%s): %w", m.Slot.Rule, err) - } - if len(r.Repairs) != 0 { - return 0, fmt.Errorf("rerun must record no repairs, got %+v", r.Repairs) - } - } - after, err := botThreadCount(f, proj, mrIID) - if err != nil { - return 0, err - } - if got := botSummaryCountMust(f, proj, mrIID) - beforeSummaries; got != 0 { - return 0, fmt.Errorf("rerun must not create a new summary note, created %d", got) - } - wantSummary, err := render.Envelope(rerunSummaryMarker(), fixtureSummaryBody()) - if err != nil { - return 0, err - } - if ff, ok := f.(*fake.Forge); ok { - if got := ff.NoteBody("note/9000"); got != wantSummary { - return 0, fmt.Errorf("summary must be updated in place (summaryUpdated: true), got %q", got) - } - } - return after - before, nil -} - -func replayCrashThenRerun(f forge.Forge) (newArtifacts int, err error) { - project := crashChallengeMarker().Slot.Project - mr := crashChallengeMarker().Slot.MR - before, err := botThreadCount(f, project, mr) - if err != nil { - return 0, err - } - for _, m := range []forge.Marker{crashChallengeMarker(), crashCommentMarker()} { - r, err := forge.Reconcile(f, testClock(), desiredThreadFor(m, crashSummary()), forge.Preconditions{}) - if err != nil { - return 0, fmt.Errorf("Reconcile(%s): %w", m.Slot.Rule, err) - } - if len(r.Repairs) != 0 { - return 0, fmt.Errorf("crash-then-rerun must record no repairs, got %+v", r.Repairs) - } - } - after, err := botThreadCount(f, project, mr) - if err != nil { - return 0, err - } - return after - before, nil -} - -func replayDuplicateRepair(f forge.Forge) (forge.PublicationReceipt, error) { - before, err := botThreadCount(f, "platform/orders-service", "612") - if err != nil { - return forge.PublicationReceipt{}, err - } - receipt, err := forge.Reconcile(f, testClock(), dupDesired(), forge.Preconditions{}) - if err != nil { - return forge.PublicationReceipt{}, err - } - after, err := botThreadCount(f, "platform/orders-service", "612") - if err != nil { - return forge.PublicationReceipt{}, err - } - if newArtifacts := after - before; newArtifacts != 0 { - return forge.PublicationReceipt{}, fmt.Errorf("repair must create zero artifacts, created %d", newArtifacts) - } - return receipt, nil -} - -func botThreadCount(f forge.Forge, project, mr string) (int, error) { - threads, err := f.ListBotThreads(project, mr) - if err != nil { - return 0, err - } - return len(threads), nil -} - -func botSummaryCount(f forge.Forge, project, mr string) (int, error) { - notes, err := f.ListBotNotes(project, mr) - if err != nil { - return 0, err - } - n := 0 - for _, note := range notes { - if note.Marker.Artifact.Kind == "summary-comment" { - n++ - } - } - return n, nil -} - -func botSummaryCountMust(f forge.Forge, project, mr string) int { - n, err := botSummaryCount(f, project, mr) - if err != nil { - panic(err) - } - return n -} - -func threadOpTarget(receipt forge.PublicationReceipt, wantID string) string { - for _, op := range receipt.Operations { - if op.TargetID == wantID { - return op.TargetID - } - } - if len(receipt.Operations) > 0 { - return receipt.Operations[0].TargetID - } - return "" -} - // ---- GitLab httptest harness (list/create/resolve path for conformance) ---- type gitlabDiscussion struct { @@ -335,6 +43,17 @@ type gitlabHarness struct { resolveCalls int noteCreateCalls int noteUpdateCalls int + + // E10-S01: MR heads + merge/approve state, so the SHA-guard cases can run + // against GitLab instead of being fake-only. + sourceSHA string + targetSHA string + mrReads int + approveCalls int + mergePUTs int + + // afterMRRead fires once an MR read has been SERVED — the TOCTOU seam. + afterMRRead func(h *gitlabHarness) } func newGitLabHarness(project, mr string) *gitlabHarness { @@ -375,6 +94,10 @@ func (h *gitlabHarness) handle(w http.ResponseWriter, r *http.Request) { http.Error(w, "missing token", http.StatusUnauthorized) return } + mrBase := fmt.Sprintf("/api/v4/projects/%s/merge_requests/%s", + url.PathEscape(h.project), url.PathEscape(h.mr)) + branchBase := fmt.Sprintf("/api/v4/projects/%s/repository/branches/", + url.PathEscape(h.project)) escapedBase := fmt.Sprintf("/api/v4/projects/%s/merge_requests/%s/discussions", url.PathEscape(h.project), url.PathEscape(h.mr)) notesBase := fmt.Sprintf("/api/v4/projects/%s/merge_requests/%s/notes", @@ -393,6 +116,14 @@ func (h *gitlabHarness) handle(w http.ResponseWriter, r *http.Request) { h.createNote(w, r) case r.Method == http.MethodPut && strings.HasPrefix(path, notesBase+"/"): h.updateNote(w, r, path, notesBase) + case r.Method == http.MethodGet && path == mrBase: + h.serveMR(w, r) + case r.Method == http.MethodGet && strings.HasPrefix(path, branchBase): + h.serveBranch(w, r) + case r.Method == http.MethodPost && path == mrBase+"/approve": + h.approve(w, r) + case r.Method == http.MethodPut && strings.HasPrefix(path, mrBase+"/merge"): + h.merge(w, r) default: http.Error(w, "unexpected "+r.Method+" "+path, http.StatusInternalServerError) } @@ -529,288 +260,6 @@ func (h *gitlabHarness) updateNote(w http.ResponseWriter, r *http.Request, path, // crash-then-rerun fixtures (REQ-E4-S09-01). A plain rerun creates zero new bot // threads; a crash-then-rerun fills exactly the one gap slot without duplicating // partial work. -func TestConformanceRerunIdempotence(t *testing.T) { - t.Run("fake/rerun-idempotence", func(t *testing.T) { - // Pre-state = rerun-idempotence.yaml run2.step2ExistingArtifacts (lines 58-66). - f := fake.New(botID, "src", "tgt", "sha256:merge") - f.SeedNote("note/9000", botID, rerunSummaryMarker(), "old summary") - f.SeedThread("note/9001", botID, rerunChallengeMarker(), true) // reviewer-resolved - f.SeedThread("note/9002", botID, rerunCommentMarker(), false) - - created, err := replayRerunIdempotence(f) - if err != nil { - t.Fatal(err) - } - // expected.newArtifactsCreated: 0 (rerun-idempotence.yaml line 82). - if created != 0 { - t.Fatalf("rerun must create zero new artifacts, created %d", created) - } - if got := f.BotThreadCount(); got != 2 { - t.Fatalf("rerun must leave exactly 2 bot threads, got %d", got) - } - if !f.IsResolved("note/9001") { - t.Fatal("rerun must preserve reviewer resolution of note/9001") - } - }) - - t.Run("fake/crash-then-rerun", func(t *testing.T) { - // Pre-state = crash-then-rerun.yaml step2ExistingArtifacts (lines 55-61). - f := fake.New(botID, "src", "tgt", "sha256:merge") - f.SeedNote("note/7000", botID, crashSummaryMarker(), "crash summary") - f.SeedThread("note/7001", botID, crashChallengeMarker(), false) - - created, err := replayCrashThenRerun(f) - if err != nil { - t.Fatal(err) - } - // expected.newArtifactsCreated: 1 (crash-then-rerun.yaml line 81). - if created != 1 { - t.Fatalf("crash-then-rerun must create exactly one gap artifact, created %d", created) - } - if got := f.BotThreadCount(); got != 2 { - t.Fatalf("expected 2 bot threads after gap-fill, got %d", got) - } - }) - - t.Run("gitlab/rerun-idempotence", func(t *testing.T) { - h := newGitLabHarness(proj, mrIID) - if err := h.seedNote(9000, botID, rerunSummaryMarker(), "old summary"); err != nil { - t.Fatal(err) - } - if err := h.seed("note/9001", botID, rerunChallengeMarker(), true); err != nil { - t.Fatal(err) - } - if err := h.seed("note/9002", botID, rerunCommentMarker(), false); err != nil { - t.Fatal(err) - } - c := h.client(t) - - created, err := replayRerunIdempotence(c) - if err != nil { - t.Fatal(err) - } - if created != 0 { - t.Fatalf("gitlab rerun must create zero new artifacts, created %d", created) - } - if h.createCalls != 0 { - t.Fatalf("gitlab rerun must not POST new discussions, createCalls=%d", h.createCalls) - } - if h.noteCreateCalls != 0 { - t.Fatalf("gitlab rerun must not POST new summary notes, noteCreateCalls=%d", h.noteCreateCalls) - } - if h.noteUpdateCalls == 0 { - t.Fatal("gitlab rerun must update summary in place (summaryUpdated: true)") - } - }) - - t.Run("gitlab/crash-then-rerun", func(t *testing.T) { - h := newGitLabHarness("platform/orders-service", "551") - if err := h.seedNote(7000, botID, crashSummaryMarker(), "crash summary"); err != nil { - t.Fatal(err) - } - if err := h.seed("note/7001", botID, crashChallengeMarker(), false); err != nil { - t.Fatal(err) - } - c := h.client(t) - - created, err := replayCrashThenRerun(c) - if err != nil { - t.Fatal(err) - } - if created != 1 { - t.Fatalf("gitlab crash-then-rerun must create one gap artifact, created %d", created) - } - if h.createCalls != 1 { - t.Fatalf("gitlab crash-then-rerun must POST exactly one discussion, createCalls=%d", h.createCalls) - } - }) -} - -// TestConformanceDuplicateRepair replays duplicate-repair.yaml (REQ-E4-S09-02): -// lowest forge id is canonical, non-canonical duplicates are resolved, and -// PublicationReceipt.repairs records each repair deterministically. -func TestConformanceDuplicateRepair(t *testing.T) { - fixtureOrder := []string{"note/8005", "note/8001", "note/8003"} - reversedOrder := []string{"note/8003", "note/8001", "note/8005"} - wantRepairs := []forge.Repair{ - {RepairedForgeID: "note/8003", CanonicalForgeID: "note/8001", Action: "resolve"}, - {RepairedForgeID: "note/8005", CanonicalForgeID: "note/8001", Action: "resolve"}, - } - - for _, tc := range []struct { - name string - seed []string - }{ - {"fake/fixture-pagination-order", fixtureOrder}, - {"fake/reversed-scan-order", reversedOrder}, - } { - t.Run(tc.name, func(t *testing.T) { - f := fake.New(botID, "src", "tgt", "sha256:merge") - for _, id := range tc.seed { - f.SeedThread(id, botID, dupMarker(), false) - } - - receipt, err := replayDuplicateRepair(f) - if err != nil { - t.Fatal(err) - } - - // expected.canonicalForgeId: note/8001 (duplicate-repair.yaml line 102). - if len(receipt.Operations) != 1 || receipt.Operations[0].TargetID != "note/8001" { - t.Fatalf("canonical must be note/8001, got %+v", receipt.Operations) - } - if len(receipt.Repairs) != len(wantRepairs) { - t.Fatalf("expected %d repairs, got %+v", len(wantRepairs), receipt.Repairs) - } - for i, want := range wantRepairs { - if receipt.Repairs[i] != want { - t.Fatalf("repair[%d]: got %+v want %+v", i, receipt.Repairs[i], want) - } - } - if got := f.OpenBotThreadCount(); got != 1 { - t.Fatalf("after repair exactly one open bot thread must remain, got %d", got) - } - }) - } - - t.Run("gitlab/fixture-pagination-order", func(t *testing.T) { - h := newGitLabHarness("platform/orders-service", "612") - for _, id := range fixtureOrder { - if err := h.seed(id, botID, dupMarker(), false); err != nil { - t.Fatal(err) - } - } - c := h.client(t) - - receipt, err := replayDuplicateRepair(c) - if err != nil { - t.Fatal(err) - } - if receipt.Operations[0].TargetID != "note/8001" { - t.Fatalf("gitlab canonical must be note/8001, got %q", receipt.Operations[0].TargetID) - } - if len(receipt.Repairs) != 2 { - t.Fatalf("gitlab repair must record 2 repairs, got %+v", receipt.Repairs) - } - if h.createCalls != 0 { - t.Fatalf("duplicate repair must not create threads, createCalls=%d", h.createCalls) - } - if h.resolveCalls != 2 { - t.Fatalf("duplicate repair must resolve 2 duplicates, resolveCalls=%d", h.resolveCalls) - } - open := 0 - for _, d := range h.discussions { - if d.author == botID && !d.resolved { - open++ - } - } - if open != 1 { - t.Fatalf("after repair exactly one open bot discussion must remain, got %d", open) - } - }) -} - -// TestConformanceSpoofedMarkerIgnored proves contributor marker spoofing has zero -// reconciliation effect (REQ-E4-S09-03 / P3-E5-S01-04): the author-identity -// filter on ListBotThreads excludes non-bot markers. -func TestConformanceSpoofedMarkerIgnored(t *testing.T) { - m := rerunCommentMarker() - - t.Run("fake/contributor-marker-invisible-on-rerun", func(t *testing.T) { - f := fake.New(botID, "src", "tgt", "sha256:merge") - f.SeedThread("note/9002", botID, m, false) - f.SeedThread("note/6660", "contributor-mallory", m, false) - - before := f.ThreadCount() - receipt, err := forge.Reconcile(f, testClock(), desiredThreadFor(m, rerunSummary()), forge.Preconditions{}) - if err != nil { - t.Fatalf("Reconcile: %v", err) - } - if after := f.ThreadCount(); after != before { - t.Fatalf("spoofed marker must not create threads on rerun: before=%d after=%d", before, after) - } - if got := threadOpTarget(receipt, "note/9002"); got != "note/9002" { - t.Fatalf("receipt must target bot thread note/9002, got %q (contributor would be note/6660)", got) - } - }) - - t.Run("fake/contributor-only-creates-bot-thread", func(t *testing.T) { - f := fake.New(botID, "src", "tgt", "sha256:merge") - f.SeedThread("note/6660", "contributor-mallory", m, false) - - _, err := forge.Reconcile(f, testClock(), desiredThreadFor(m, nil), forge.Preconditions{}) - if err != nil { - t.Fatalf("Reconcile: %v", err) - } - if got := f.BotThreadCount(); got != 1 { - t.Fatalf("contributor marker alone must not satisfy slot; expected 1 bot thread, got %d", got) - } - }) - - t.Run("gitlab/contributor-marker-excluded", func(t *testing.T) { - h := newGitLabHarness(m.Slot.Project, m.Slot.MR) - if err := h.seed("note/9002", botID, m, false); err != nil { - t.Fatal(err) - } - if err := h.seed("note/6660", "contributor-mallory", m, false); err != nil { - t.Fatal(err) - } - c := h.client(t) - - before, err := botThreadCount(c, m.Slot.Project, m.Slot.MR) - if err != nil { - t.Fatal(err) - } - receipt, err := forge.Reconcile(c, testClock(), desiredThreadFor(m, nil), forge.Preconditions{}) - if err != nil { - t.Fatal(err) - } - after, err := botThreadCount(c, m.Slot.Project, m.Slot.MR) - if err != nil { - t.Fatal(err) - } - if after-before != 0 { - t.Fatalf("gitlab spoof rerun must create zero artifacts, created %d", after-before) - } - if h.createCalls != 0 { - t.Fatalf("contributor marker must not prevent idempotent reuse; createCalls=%d", h.createCalls) - } - if got := threadOpTarget(receipt, "note/9002"); got != "note/9002" { - t.Fatalf("receipt must target bot thread note/9002, got %q", got) - } - threads, err := c.ListBotThreads(m.Slot.Project, m.Slot.MR) - if err != nil { - t.Fatal(err) - } - if len(threads) != 1 || threads[0].ID != "note/9002" { - t.Fatalf("ListBotThreads must return only bot thread, got %+v", threads) - } - }) - - t.Run("gitlab/contributor-only-creates-bot-thread", func(t *testing.T) { - h := newGitLabHarness(m.Slot.Project, m.Slot.MR) - if err := h.seed("note/6660", "contributor-mallory", m, false); err != nil { - t.Fatal(err) - } - c := h.client(t) - - _, err := forge.Reconcile(c, testClock(), desiredThreadFor(m, nil), forge.Preconditions{}) - if err != nil { - t.Fatalf("Reconcile: %v", err) - } - if h.createCalls != 1 { - t.Fatalf("contributor marker invisible — must create one bot thread, createCalls=%d", h.createCalls) - } - threads, err := c.ListBotThreads(m.Slot.Project, m.Slot.MR) - if err != nil { - t.Fatal(err) - } - if len(threads) != 1 || threads[0].Author != botID { - t.Fatalf("only bot-authored thread counts, got %+v", threads) - } - }) -} - // seedRaw seeds a discussion with a VERBATIM body — used to plant a corrupted // marker that h.seed (which renders a valid envelope) could never produce. func (h *gitlabHarness) seedRaw(id, author, body string) { diff --git a/internal/forge/conformance/replay.go b/internal/forge/conformance/replay.go new file mode 100644 index 00000000..8a8abf2d --- /dev/null +++ b/internal/forge/conformance/replay.go @@ -0,0 +1,139 @@ +package conformance + +import ( + "fmt" + + "github.com/PlatformRelay/assent/internal/forge" + "github.com/PlatformRelay/assent/internal/render" +) + +// replay.go holds the backend-neutral REPLAY BODIES of the P3-E5 publication +// protocol. These already took a `forge.Forge` before extraction — the case +// bodies were neutral; only their fixtures and their assertions were not — so +// this file is a move, not a rewrite, with ONE deliberate change called out at +// its site: the `if ff, ok := f.(*fake.Forge); ok` guard around the +// summary-updated-in-place check is gone, because it made that assertion +// unreachable, and therefore unfailable, on every non-fake backend. + +func replayRerunIdempotence(f forge.Forge, obs Observer) (newArtifacts int, err error) { + before, err := botThreadCount(f, proj, mrIID) + if err != nil { + return 0, err + } + beforeSummaries, err := botSummaryCount(f, proj, mrIID) + if err != nil { + return 0, err + } + for _, m := range []forge.Marker{rerunChallengeMarker(), rerunCommentMarker()} { + r, err := forge.Reconcile(f, testClock(), desiredThreadFor(m, rerunSummary()), forge.Preconditions{}) + if err != nil { + return 0, fmt.Errorf("Reconcile(%s): %w", m.Slot.Rule, err) + } + if len(r.Repairs) != 0 { + return 0, fmt.Errorf("rerun must record no repairs, got %+v", r.Repairs) + } + } + after, err := botThreadCount(f, proj, mrIID) + if err != nil { + return 0, err + } + if got := botSummaryCountMust(f, proj, mrIID) - beforeSummaries; got != 0 { + return 0, fmt.Errorf("rerun must not create a new summary note, created %d", got) + } + wantSummary, err := render.Envelope(rerunSummaryMarker(), fixtureSummaryBody()) + if err != nil { + return 0, err + } + // Pre-extraction this read `if ff, ok := f.(*fake.Forge); ok` and skipped + // silently on every other backend — an assertion that could not fail for half + // the suite, which is the exact species REQ-E10-S01-04 exists to stop. The + // Observer answers it on both adapters, so it is now unconditional. + if got := obs.NoteBody("note/9000"); got != wantSummary { + return 0, fmt.Errorf("summary must be updated in place (summaryUpdated: true), got %q", got) + } + return after - before, nil +} + +func replayCrashThenRerun(f forge.Forge) (newArtifacts int, err error) { + project := crashChallengeMarker().Slot.Project + mr := crashChallengeMarker().Slot.MR + before, err := botThreadCount(f, project, mr) + if err != nil { + return 0, err + } + for _, m := range []forge.Marker{crashChallengeMarker(), crashCommentMarker()} { + r, err := forge.Reconcile(f, testClock(), desiredThreadFor(m, crashSummary()), forge.Preconditions{}) + if err != nil { + return 0, fmt.Errorf("Reconcile(%s): %w", m.Slot.Rule, err) + } + if len(r.Repairs) != 0 { + return 0, fmt.Errorf("crash-then-rerun must record no repairs, got %+v", r.Repairs) + } + } + after, err := botThreadCount(f, project, mr) + if err != nil { + return 0, err + } + return after - before, nil +} + +func replayDuplicateRepair(f forge.Forge) (forge.PublicationReceipt, error) { + before, err := botThreadCount(f, "platform/orders-service", "612") + if err != nil { + return forge.PublicationReceipt{}, err + } + receipt, err := forge.Reconcile(f, testClock(), dupDesired(), forge.Preconditions{}) + if err != nil { + return forge.PublicationReceipt{}, err + } + after, err := botThreadCount(f, "platform/orders-service", "612") + if err != nil { + return forge.PublicationReceipt{}, err + } + if newArtifacts := after - before; newArtifacts != 0 { + return forge.PublicationReceipt{}, fmt.Errorf("repair must create zero artifacts, created %d", newArtifacts) + } + return receipt, nil +} + +func botThreadCount(f forge.Forge, project, mr string) (int, error) { + threads, err := f.ListBotThreads(project, mr) + if err != nil { + return 0, err + } + return len(threads), nil +} + +func botSummaryCount(f forge.Forge, project, mr string) (int, error) { + notes, err := f.ListBotNotes(project, mr) + if err != nil { + return 0, err + } + n := 0 + for _, note := range notes { + if note.Marker.Artifact.Kind == "summary-comment" { + n++ + } + } + return n, nil +} + +func botSummaryCountMust(f forge.Forge, project, mr string) int { + n, err := botSummaryCount(f, project, mr) + if err != nil { + panic(err) + } + return n +} + +func threadOpTarget(receipt forge.PublicationReceipt, wantID string) string { + for _, op := range receipt.Operations { + if op.TargetID == wantID { + return op.TargetID + } + } + if len(receipt.Operations) > 0 { + return receipt.Operations[0].TargetID + } + return "" +} diff --git a/internal/forge/conformance/sha_guard_test.go b/internal/forge/conformance/sha_guard_test.go deleted file mode 100644 index ee392e2c..00000000 --- a/internal/forge/conformance/sha_guard_test.go +++ /dev/null @@ -1,107 +0,0 @@ -package conformance - -import ( - "errors" - "fmt" - "testing" - - "github.com/PlatformRelay/assent/internal/forge" - "github.com/PlatformRelay/assent/internal/forge/fake" -) - -const ( - pinSource = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - pinTarget = "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" - pinDigest = "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc" - movedTarget = "sha256:0000000000000000000000000000000000000000000000000000000000000000" - movedSource = "sha256:1111111111111111111111111111111111111111111111111111111111111111" -) - -func approveState() forge.DesiredReviewState { - return forge.DesiredReviewState{ - Project: proj, - MR: mrIID, - Approve: true, - Merge: &forge.DesiredMerge{ - SourceSha: pinSource, - TargetSha: pinTarget, - MergeResultDigest: pinDigest, - }, - } -} - -func armedPre() forge.Preconditions { - return forge.Preconditions{ - ArmEligible: true, - SourceSha: pinSource, - TargetSha: pinTarget, - MergeResultDigest: pinDigest, - } -} - -// shaGuardOutcome captures the fail-closed result of a SHA-guarded reconcile. -type shaGuardOutcome struct { - Err error - Approvals int - Merges int -} - -func runSHAGuard(f *fake.Forge) shaGuardOutcome { - _, err := forge.Reconcile(f, testClock(), approveState(), armedPre()) - return shaGuardOutcome{ - Err: err, - Approvals: len(f.Approvals), - Merges: len(f.Merges), - } -} - -func assertSHAMoved(out shaGuardOutcome) error { - if !errors.Is(out.Err, forge.ErrSHAMoved) { - return fmt.Errorf("want ErrSHAMoved, got %v", out.Err) - } - if out.Merges != 0 { - return fmt.Errorf("zero merges expected, got %d", out.Merges) - } - return nil -} - -// TestConformanceTargetAdvancedRejected is REQ-E4-S07-01: the target branch tip -// advanced after evaluation pins were taken → Reconcile refuses with the typed -// ErrSHAMoved summary, records zero merges, and performs no approve write (the -// CurrentHeads pre-check fails closed before any forge mutation). -func TestConformanceTargetAdvancedRejected(t *testing.T) { - for run := 0; run < 2; run++ { - // Evaluated pins: source+target+digest. Forge CURRENT state: target moved. - f := fake.New(botID, pinSource, movedTarget, pinDigest) - - out := runSHAGuard(f) - if err := assertSHAMoved(out); err != nil { - t.Fatalf("run %d: %v", run, err) - } - if out.Approvals != 0 { - t.Fatalf("run %d: pre-check rejection must record zero approvals, got %d", run, out.Approvals) - } - } -} - -// TestConformanceSourceMovedRejected is REQ-E4-S07-02: the MR source head moved -// after evaluation → the atomic MergeCAS guard refuses (409/406 mapping on the -// GitLab adapter; ErrSHAMoved here), records zero merges, and leaves at most the -// dangling approval the TOCTOU window permits. The AfterCurrentHeads drift hook -// models the head advancing between the pre-check read and MergeCAS. -func TestConformanceSourceMovedRejected(t *testing.T) { - for run := 0; run < 2; run++ { - f := fake.New(botID, pinSource, pinTarget, pinDigest) - f.AfterCurrentHeads = func(fk *fake.Forge) { - fk.CurrentSourceSha = movedSource - } - - out := runSHAGuard(f) - if err := assertSHAMoved(out); err != nil { - t.Fatalf("run %d: %v", run, err) - } - if out.Approvals != 1 { - t.Fatalf("run %d: MergeCAS rejection may leave one dangling approval, got %d", run, out.Approvals) - } - } -} diff --git a/internal/forge/conformance/suite.go b/internal/forge/conformance/suite.go new file mode 100644 index 00000000..1252b637 --- /dev/null +++ b/internal/forge/conformance/suite.go @@ -0,0 +1,196 @@ +package conformance + +import ( + "errors" + "sync" + + "github.com/PlatformRelay/assent/internal/forge" +) + +// suite.go is the importable entry point for the forge conformance suite +// (E10-S01, REQ-E10-S01-01). Before it, the cases were `_test.go` bodies that no +// other package could reach, so a second adapter's only route to conformance was +// to copy them. +// +// A Factory constructs one backend in a known initial state. + +// Factory builds one backend under test from a Config. It returns a +// `forge.Forge` and NOT a `forge.RunPort`: the composite port is E10-S02's +// deliverable and S02 is LGTM-gated, so declaring it here would smuggle an +// LGTM-gated core-contract change into an `[autonomous]` lane. S02 depends on +// S01 precisely so the port change lands against an executable suite; the +// substitution is a one-line change to this type when it does. +type Factory func(t TB, cfg Config) Backend + +// Case is one catalogued conformance case. ID is not decorative — it is the join +// key to `catalog.yaml`, and TestCatalogMatchesExecutedCases fails when the two +// sets diverge in either direction. +type Case struct { + ID string + Run func(t TB, f Factory) +} + +// Cases returns the suite in a fixed order. This slice IS the dispatch list: the +// catalog gate compares the catalog against what running the suite actually +// executes, so deleting an entry here reds that gate. That is the point — a gate +// keyed on test-function NAMES would stay green for a case that had been +// unhooked from the runner, which is a predicate over text standing in for a +// property that is structural. +func Cases() []Case { + return []Case{ + {ID: "sha-guard-target-advanced", Run: caseSHAGuardTargetAdvanced}, + {ID: "sha-guard-source-moved", Run: caseSHAGuardSourceMoved}, + {ID: "p3e5-rerun-idempotence", Run: caseRerunIdempotence}, + {ID: "p3e5-duplicate-repair", Run: caseDuplicateRepair}, + {ID: "p3e5-spoofed-marker-ignored", Run: caseSpoofedMarkerIgnored}, + } +} + +// RunSuite runs every conformance case against the backend the Factory builds +// and returns the IDs it actually dispatched. This is the whole public surface an +// adapter needs: one call, no copying. +// +// The return value is not decoration — it is the DENOMINATOR for +// REQ-E10-S01-02. The catalog gate checks the catalog against OBSERVED +// EXECUTION, never against a hand-maintained list or a scan for test-function +// names, so a case that stops running cannot stay accounted for. Having one +// runner rather than a separate "and also tell me what ran" entry point is +// deliberate: two paths would let the reported set and the executed set drift, +// which is the failure this gate exists to catch. +func RunSuite(t TB, f Factory) []string { + t.Helper() + return runCases(t, f, Cases()) +} + +func runCases(t TB, f Factory, cases []Case) []string { + t.Helper() + var ( + mu sync.Mutex + executed []string + ) + for _, c := range cases { + t.Run(c.ID, func(t TB) { + // Recorded on ENTRY, not on pass. A case that runs and fails has still + // been executed; conflating "did not run" with "ran and failed" would + // let a red suite look like an incomplete one. + mu.Lock() + executed = append(executed, c.ID) + mu.Unlock() + c.Run(t, f) + }) + } + mu.Lock() + defer mu.Unlock() + return append([]string(nil), executed...) +} + +// ---- SHA-guard cases (ADR-0015 §2, REQ-E4-S07-01/02) ---- + +const ( + pinSource = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + pinTarget = "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + pinDigest = "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc" + movedTarget = "sha256:0000000000000000000000000000000000000000000000000000000000000000" + movedSource = "sha256:1111111111111111111111111111111111111111111111111111111111111111" +) + +func approveState(pins forge.DesiredMerge) forge.DesiredReviewState { + return forge.DesiredReviewState{ + Project: proj, + MR: mrIID, + Approve: true, + Merge: &pins, + } +} + +func armedPre(pins forge.DesiredMerge) forge.Preconditions { + return forge.Preconditions{ + ArmEligible: true, + SourceSha: pins.SourceSha, + TargetSha: pins.TargetSha, + MergeResultDigest: pins.MergeResultDigest, + } +} + +func shaGuardConfig() Config { + return Config{ + Project: proj, + MR: mrIID, + BotAuthor: botID, + CurrentSourceSHA: pinSource, + CurrentTargetSHA: pinTarget, + // Only the fake can honour a literal digest; GitLab synthesises its own + // from source+target. Factories are free to ignore this and report the + // truth through Fixture.Pins, which is what the cases actually read. + CurrentMergeResultDigest: pinDigest, + } +} + +// caseSHAGuardTargetAdvanced is REQ-E4-S07-01: the target branch tip advanced +// after the evaluation pins were taken, so Reconcile refuses with ErrSHAMoved, +// performs no approve write, and — the strengthened part — never reaches +// MergeCAS at all, because the CurrentHeads pre-check fails closed BEFORE any +// forge mutation. The pre-extraction case asserted only that zero merges were +// PERFORMED, which is equally true of a backend that skipped the pre-check and +// let the atomic CAS do the refusing. Those are different guarantees, and only +// the attempt count separates them. +func caseSHAGuardTargetAdvanced(t TB, f Factory) { + t.Helper() + // Twice: the guard must be deterministic, not first-run-only. + for run := 0; run < 2; run++ { + b := f(t, shaGuardConfig()) + pins := b.Fixture.Pins() // what an evaluation would have recorded + b.Fixture.MoveTargetHead(movedTarget) + + _, err := forge.Reconcile(b.Port, testClock(), approveState(pins), armedPre(pins)) + if !errors.Is(err, forge.ErrSHAMoved) { + t.Fatalf("run %d: want ErrSHAMoved, got %v", run, err) + } + if got := b.Observer.MergesPerformed(); got != 0 { + t.Fatalf("run %d: zero merges expected, got %d", run, got) + } + if got := b.Observer.MergeAttempts(); got != 0 { + t.Fatalf("run %d: pre-check must fail closed BEFORE MergeCAS is called, got %d attempt(s)", run, got) + } + if got := b.Observer.Approvals(); got != 0 { + t.Fatalf("run %d: pre-check rejection must record zero approvals, got %d", run, got) + } + } +} + +// caseSHAGuardSourceMoved is REQ-E4-S07-02: the MR source head moves inside the +// TOCTOU window between the pre-check read and MergeCAS, so the ATOMIC CAS guard +// refuses (409/406 on GitLab, ErrSHAMoved here), zero merges are performed, and +// at most the one dangling approval the window permits survives. MergeAttempts +// must be exactly 1 — this case is only meaningful if the CAS was actually +// REACHED, and "no merge happened" alone cannot distinguish that from a run that +// never got there, which is the same assertion the target-advanced case makes. +func caseSHAGuardSourceMoved(t TB, f Factory) { + t.Helper() + for run := 0; run < 2; run++ { + b := f(t, shaGuardConfig()) + pins := b.Fixture.Pins() + b.Fixture.DriftSourceHeadAfterRead(movedSource) + + _, err := forge.Reconcile(b.Port, testClock(), approveState(pins), armedPre(pins)) + if !errors.Is(err, forge.ErrSHAMoved) { + t.Fatalf("run %d: want ErrSHAMoved, got %v", run, err) + } + if got := b.Observer.MergesPerformed(); got != 0 { + t.Fatalf("run %d: zero merges expected, got %d", run, got) + } + if got := b.Observer.MergeAttempts(); got != 1 { + t.Fatalf("run %d: the CAS guard must be REACHED and refuse, want 1 attempt, got %d", run, got) + } + if got := b.Observer.Approvals(); got != 1 { + t.Fatalf("run %d: MergeCAS rejection may leave one dangling approval, got %d", run, got) + } + } +} + +// reconcileForObservation drives one SHA-guarded reconcile and discards the +// outcome. The SHA-guard cases already assert on the error; this exists for +// REQ-E10-S01-04's observation proof, which asserts on the surface instead. +func reconcileForObservation(b Backend, pins forge.DesiredMerge) (forge.PublicationReceipt, error) { + return forge.Reconcile(b.Port, testClock(), approveState(pins), armedPre(pins)) +} diff --git a/internal/forge/conformance/suite_test.go b/internal/forge/conformance/suite_test.go new file mode 100644 index 00000000..8a8d5ced --- /dev/null +++ b/internal/forge/conformance/suite_test.go @@ -0,0 +1,122 @@ +package conformance + +import "testing" + +// suite_test.go is the ENTRY POINT layer: it runs the shared suite against both +// built-in backends. +// +// The five exported test names are unchanged from before extraction, deliberately. +// E10-S01's definition of done forbids renaming a case, and `exitgate_test.go`'s +// `l1CatalogTests` pins these exact names against `catalog.yaml` — the pattern +// recorded in this repo's own lessons: pin named tests and let `go test` carry the +// semantics, rather than asserting a fix's shape from source text. Renaming them +// would have silently unhooked the E7 exit gate. +// +// What changed is what they DO: each now runs its case against every backend, +// where before the fake and GitLab variants were hand-written subtests with +// different assertions and no one comparing them. + +type namedBackend struct { + name string + f Factory +} + +// backends is the set every case runs against. Adding an adapter here runs the +// entire suite against it — that is the property E10-S01 exists to create. +func backends() []namedBackend { + return []namedBackend{ + {"fake", fakeFactory}, + {"gitlab", gitlabFactory}, + } +} + +func runCaseOnAllBackends(t *testing.T, id string) { + t.Helper() + var found *Case + for _, c := range Cases() { + if c.ID == id { + found = &c + break + } + } + if found == nil { + t.Fatalf("no conformance case with id %q — Cases() and the entry points have diverged", id) + } + for _, be := range backends() { + t.Run(be.name, func(sub *testing.T) { found.Run(tbT{sub}, be.f) }) + } +} + +// TestConformanceTargetAdvancedRejected is REQ-E4-S07-01. +func TestConformanceTargetAdvancedRejected(t *testing.T) { + runCaseOnAllBackends(t, "sha-guard-target-advanced") +} + +// TestConformanceSourceMovedRejected is REQ-E4-S07-02. +func TestConformanceSourceMovedRejected(t *testing.T) { + runCaseOnAllBackends(t, "sha-guard-source-moved") +} + +// TestConformanceRerunIdempotence is REQ-E4-S09-01. +func TestConformanceRerunIdempotence(t *testing.T) { + runCaseOnAllBackends(t, "p3e5-rerun-idempotence") +} + +// TestConformanceDuplicateRepair is REQ-E4-S09-02. +func TestConformanceDuplicateRepair(t *testing.T) { + runCaseOnAllBackends(t, "p3e5-duplicate-repair") +} + +// TestConformanceSpoofedMarkerIgnored is REQ-E4-S09-03. +func TestConformanceSpoofedMarkerIgnored(t *testing.T) { + runCaseOnAllBackends(t, "p3e5-spoofed-marker-ignored") +} + +// TestSHAGuardObservesMergeAttempts is REQ-E10-S01-04's named proof: the extracted +// SHA-guard cases still observe a merge ATTEMPT COUNT and not merely a returned +// error. +// +// It is written as a property over the observation surface rather than a re-run of +// the cases, because the weakening it guards against is not "the case fails" — it +// is "the case stops looking". The two SHA-guard scenarios must disagree about +// MergeAttempts (0 when the pre-check refuses, 1 when the CAS refuses) while +// agreeing that no merge was performed. If someone downgrades the surface to a +// single "did a merge happen" boolean to accommodate a backend, the two scenarios +// collapse to the same observation and this test reds. +func TestSHAGuardObservesMergeAttempts(t *testing.T) { + for _, be := range backends() { + t.Run(be.name, func(sub *testing.T) { + t := tbT{sub} + preCheck := be.f(t, shaGuardConfig()) + pins := preCheck.Fixture.Pins() + preCheck.Fixture.MoveTargetHead(movedTarget) + _, _ = reconcileForObservation(preCheck, pins) + + cas := be.f(t, shaGuardConfig()) + casPins := cas.Fixture.Pins() + cas.Fixture.DriftSourceHeadAfterRead(movedSource) + _, _ = reconcileForObservation(cas, casPins) + + if preCheck.Observer.MergeAttempts() != 0 { + t.Fatalf("target-advanced must not reach MergeCAS, got %d attempt(s)", + preCheck.Observer.MergeAttempts()) + } + if cas.Observer.MergeAttempts() != 1 { + t.Fatalf("source-moved must reach MergeCAS exactly once, got %d", + cas.Observer.MergeAttempts()) + } + if preCheck.Observer.MergeAttempts() == cas.Observer.MergeAttempts() { + t.Fatal("the two SHA-guard scenarios became indistinguishable — " + + "the observation surface has been downgraded (REQ-E10-S01-04)") + } + for _, c := range []struct { + name string + o Observer + }{{"target-advanced", preCheck.Observer}, {"source-moved", cas.Observer}} { + if got := c.o.MergesPerformed(); got != 0 { + t.Fatalf("%s: no merge may be performed, got %d", c.name, got) + } + } + }) + } +} diff --git a/internal/forge/conformance/tb.go b/internal/forge/conformance/tb.go new file mode 100644 index 00000000..2391e66a --- /dev/null +++ b/internal/forge/conformance/tb.go @@ -0,0 +1,104 @@ +package conformance + +import ( + "fmt" + "testing" +) + +// tb.go defines the testing surface the conformance case bodies assert through. +// +// Why an interface and not `*testing.T`. This repo's most frequently found defect +// is "this assertion cannot fail", and a conformance suite is the worst possible +// place for it: a case that silently stops proving its property still reports +// PASS, and the catalog keeps claiming coverage. The only way to know a case can +// fail is to RUN it against a backend that violates the property and observe it +// go red — and that is impossible with `*testing.T` hard-wired in, because +// `t.Fatalf` fails the very test doing the checking. The proof and the failure +// become indistinguishable. +// +// With TB, `TestEveryCaseCanFail` runs each case against a deliberately +// sabotaged backend and requires a failure. A case whose assertions were +// weakened into unfailability reds THAT test instead of passing quietly. + +// TB is the subset of *testing.T the case bodies use. +type TB interface { + Helper() + Cleanup(func()) + Fatal(args ...any) + Fatalf(format string, args ...any) + Run(name string, fn func(TB)) bool +} + +// tbT adapts *testing.T to TB for real runs. +type tbT struct{ *testing.T } + +func (t tbT) Run(name string, fn func(TB)) bool { + return t.T.Run(name, func(sub *testing.T) { fn(tbT{sub}) }) +} + +// abortFatal unwinds a recorder's Fatalf. Fatalf must STOP the case the way +// testing.T does; returning normally would let execution continue past a failed +// assertion and reach ones that assume it held. +type abortFatal struct{ msg string } + +// failRecorder is a TB that records failure instead of reporting it, so a case +// can be run for the purpose of proving it CAN fail. +type failRecorder struct { + failed bool + msg string + cleanups []func() +} + +func (r *failRecorder) Helper() {} +func (r *failRecorder) Cleanup(fn func()) { r.cleanups = append(r.cleanups, fn) } +func (r *failRecorder) Fatalf(f string, a ...any) { + r.failed = true + if r.msg == "" { + r.msg = fmt.Sprintf(f, a...) + } + panic(abortFatal{msg: r.msg}) +} + +// Run executes a subcase, absorbing its abort so sibling subcases still run — +// mirroring testing.T, where one failing subtest does not abandon the rest. +func (r *failRecorder) Run(_ string, fn func(TB)) bool { + before := r.failed + func() { + defer func() { + if rec := recover(); rec != nil { + if _, ok := rec.(abortFatal); !ok { + panic(rec) + } + } + }() + fn(r) + }() + return r.failed == before +} + +func (r *failRecorder) runCleanups() { + for i := len(r.cleanups) - 1; i >= 0; i-- { + r.cleanups[i]() + } + r.cleanups = nil +} + +// runAndRecord runs one case against a factory and reports whether it failed. +func runAndRecord(c Case, f Factory) (failed bool, msg string) { + r := &failRecorder{} + defer r.runCleanups() + func() { + defer func() { + if rec := recover(); rec != nil { + if _, ok := rec.(abortFatal); !ok { + panic(rec) + } + } + }() + c.Run(r, f) + }() + return r.failed, r.msg +} + +// Fatal mirrors testing.T.Fatal: record and abort. +func (r *failRecorder) Fatal(a ...any) { r.Fatalf("%s", fmt.Sprint(a...)) } diff --git a/internal/forge/fake/fake.go b/internal/forge/fake/fake.go index c87b989e..1fbbc4db 100644 --- a/internal/forge/fake/fake.go +++ b/internal/forge/fake/fake.go @@ -39,6 +39,14 @@ type Forge struct { Approvals []string Merges []string + // Note create-vs-update counters (E10-S01, REQ-E10-S01-04). UpsertComment is + // ONE port method with two outcomes, so the conformance suite's port-level + // decorator cannot tell them apart — only the backend can. Every other write + // counter the suite needs is counted at the port instead (see + // internal/forge/conformance/portcount.go), so nothing else was added here. + NoteCreateCalls int + NoteUpdateCalls int + seq int merged bool @@ -170,12 +178,14 @@ func (f *Forge) UpsertComment(_, _ string, marker forge.Marker, body string) (fo continue } if marker.Artifact.Kind == "summary-comment" { + f.NoteUpdateCalls++ f.notes[i].Marker = marker f.notes[i].Body = fullBody f.noteMutation() return f.notes[i], nil } } + f.NoteCreateCalls++ f.seq++ n := forge.Note{ ID: fmt.Sprintf("note/%d", 8000+f.seq), diff --git a/openspec/specs/backlog.md b/openspec/specs/backlog.md index 07a0dd7e..7ab1b1ef 100644 --- a/openspec/specs/backlog.md +++ b/openspec/specs/backlog.md @@ -560,7 +560,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 **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-S01 | **DONE** (D-165) — conformance suite extracted into importable Go: `RunSuite(t, Factory)`, explicit `Observer`/`Fixture` surfaces, and three self-checks (observed-execution catalog match, can-fail-against-sabotage per adapter, per-observation load-bearing). Found + closed: the two SHA-guard cases had never run against GitLab despite `forge: gitlab` | **[autonomous]** | S00 | no assertion weakened — several strengthened; all 5 cases now run on both adapters | | 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 | | E10-S04 | ⚠️ Neutral capability model — `unknown` never arms | **[autonomous · engine-grade · LGTM]** | S02 | one fail-closed guarantee, not two |