test(forge): pin the one-shared-Linear-TokenSource invariant (RIG-3135) - #1073
Merged
Merged
Conversation
The forge Linear notify lane and write coordinate MUST ride exactly one linearagent.TokenSource (DEC-4). Linear revokes a client-credentials app's tokens when its scope set changes, and the mint singleflight coalesces only WITHIN an instance, so a second source is two mints racing one credential -- each revoking the other's live token. That invariant was held by code inspection only: a refactor building a source per site passed CI. Unlike the GitHub lanes, which share a whole *forge.GitHub and are pinned by TestForgeLanesShareOneBudgetGate, the two Linear sinks each build their OWN *forge.Linear. Only the source inside is shared, so client identity proves nothing and forge.Linear.token is unexported. Adds a read-only TokenSourceForTest accessor -- the smallest seam that makes it observable, matching the existing forgeNotifyLane.reader recorded-for-tests pattern, and no injectable OAuth endpoint or pgtest e2e as the issue had assumed. The test reads the source each BUILDER threaded into the client it produced, never a handle the test holds, and asserts the two clients are distinct objects so the pair is not trivially true. Mutation-proved against the exact regression, twice: minting a separate source at the write coordinate fails the write arm, and at the notify lane fails the notify arm. Refs RIG-3090. Co-authored-by: Matt Wilkinson <matt@rigel.build>
|
😎 Merged successfully - details. |
|
Compass engineering docs preview: https://compass-forge-rig-3135-share.compass-eng-docs.pages.dev Deployed from |
…uthor hop (RIG-3135) Review found the comments asserting a mechanism the frozen record marks unverified, which is the same defect I shipped one PR ago. I wrote that two TokenSource instances each revoke the other's live token. The documented trigger is a scope-set CHANGE (tokenScope const comment in internal/linearagent/client.go), and tokenScope is pinned, so both sources mint IDENTICAL scope and that trigger cannot fire here. What the record actually says is weaker and is the real reason: 'whether concurrent same-scope mints from independent instances coexist is unverified -- one shared instance removes the question entirely' (docs/designs/server/compass-forge-app-credentials/design.md, T4). Stating an unverified hazard as certain invites a future reader to 'fix' the sharing once they discover same-scope mints do not in fact cross-revoke. Also corrects the citation: the one-instance directive is T4's task text, not DEC-4 (which is the clean-cutover/no-PAT-fallback decision). Drops a dead guard -- NewTokenSource cannot return nil -- and records why the nil store/hub/board args are safe, mirroring the sibling budget test's note. Closes RIG-3135's optional secondary bullet with a fourth arm: the GitHub author role IS the primaryClient the builder was passed. Mutation-proved like the others -- registering a freshly-built client as author fails that arm alone. Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
marked this pull request as ready for review
September 11, 2026 15:41
mattwilkinsonn
approved these changes
Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes RIG-3135. Test-only; no production behaviour changes.
What was unguarded
The forge Linear notify lane and the Linear write coordinate must ride exactly
ONE
linearagent.TokenSource(DEC-4's one-instance rule). Linear revokes aclient-credentials app's tokens when its scope set changes, and the mint
singleflight coalesces only WITHIN an instance — so two sources are two mints
racing one credential, each revoking the other's live token.
That was held by code inspection only. The gating branches (nil / error /
partial-warn) and each consumer's nil-or-non-nil acceptance were tested, but
nothing asserted the same instance reaches both sinks, so a refactor building
a source per call site passed CI.
Why this needed a seam at all
The sibling GitHub invariant is easy: both lanes share a whole
*forge.GitHub,so
TestForgeLanesShareOneBudgetGatecompares client pointers. Linear is notthat shape —
buildLinearNotifyLaneandbuildForgeWriteServiceeach buildtheir OWN
*forge.Linear, so client identity proves nothing and the onlyshared object is the
tokenfield, which is unexported.Adds
Linear.TokenSourceForTest, a read-only accessor. It mirrors the existingforgeNotifyLane.readerrecorded-for-tests pattern (same comment shape, same"production reads it never" note). RIG-3135 assumed this would need an
injectable OAuth endpoint or a pgtest e2e with an httptest OAuth server;
neither turned out to be necessary, because
forge.TokenSourceis already aninterface and both builders accept an already-built source.
The test
TestForgeLinearLanesShareOneTokenSourcereads the source each BUILDERthreaded into the client it produced —
notifyLane.reader, and the registry'sresolved
author— never a handle the test holds. A builder that accepted theshared source and then minted its own would satisfy every existing test and
fail only this one. It also asserts the two clients are distinct objects, so
the two arms cannot be one object read twice.
Mutation-proved, both arms independently:
Verification
go build -tags unix ./...OK ·go vet -tags unix ./internal/forge/... ./server/...OK ·go test -tags unix ./internal/forge/... ./server/...both ok ·gofmtclean ·moon ci32 actions, 0 failed.Review
0 high, 1 medium, 4 low. The medium was mine and it is the same defect I
shipped one PR ago: I asserted that two TokenSource instances each revoke the
other's live token. The documented trigger is a scope-set CHANGE, and
tokenScopeis a pinned const, so both sources mint identical scope and thattrigger cannot fire. The frozen record says something weaker and truer —
"whether concurrent same-scope mints from independent instances coexist is
unverified — one shared instance removes the question entirely"
(
compass-forge-app-credentials, T4). Reworded to match, because a reader whodiscovers same-scope mints do not cross-revoke would conclude the sharing is
unnecessary and "fix" it.
Also corrected the citation (the directive is T4's text, not DEC-4), dropped a
dead nil-guard, and documented why the nil store/hub/board args are safe.
Took the optional acceptance bullet too. A fourth arm now asserts the GitHub
author role IS the
primaryClientthe builder was passed, mutation-proved likethe others: registering a freshly-built client as author fails that arm alone.
All four arms are individually mutation-proved.
Verified after the fixes:
go buildOK,go vetOK, both packagesok,gofmtclean.