From a6e6826e6a710ab4fd3f0c259d01236008522724 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 31 Aug 2026 21:41:06 -0400 Subject: [PATCH 1/2] refactor(forge): cut forge credentials over to 2 GitHub Apps + Linear OAuth, drop all PATs (RIG-3090) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves the whole forge credential model onto App/OAuth identities and deletes every personal-access-token path, in one cutover. End state: two GitHub App identities plus one Linear OAuth app, zero PAT secret names anywhere in the write or Linear path. This is the combined code of design tasks T2 (author writes onto the primary App), T3 (reviewer App), and T4 (drop the PATs; Linear actor=app) from the frozen record `docs/designs/server/compass-forge-app-credentials/design.md`. Deployment, IaC, live-oracle, and webhook-runbook work stay in their own issues (RIG-3094 T3.5, RIG-3095 T4-IaC, RIG-3096 T5, RIG-3098 T6). ### Two GitHub Apps - **Primary App** (`ForgeConfig.App`) serves everything but reviews: board reads, notify reads, author writes, board, and webhooks. - **Reviewer App** (new `ForgeConfig.ReviewerApp`) serves only the reviewer write client — a distinct GitHub identity so an agent approving a PR it authored dispatches `submit_review` on a different account, dissolving the author-cannot-approve-own-PR 422 at the credential layer (F1). - Author writes **reuse the shared primary App `*forge.GitHub` client** built for board/notify reads (RIG-2991), not a fresh client over the same token source: the client-side rate-budget/`resetAt` gate is per-client, so one client keeps reads and author writes on one budget gate against the single installation. The reviewer leg builds its own App token source + client. ### Linear OAuth (actor=app) - Linear write and notify lanes ride **one shared `linearagent.TokenSource`** (client-credentials, the production "Compass" app), built once in `Serve` and passed to both build sites — the one-instance rule (Linear revokes a client-credentials app's tokens on a scope-set change; the mint singleflight coalesces only within an instance). - A boot-time `Token(ctx)` mint check fails `Serve` fast on a bad pair or a disabled client-credentials toggle, rather than on the first write. ### Gate re-key - `forgeWritesEnabled` now keys on "both Apps configured" (each `AppID != 0` and its key secret declared), replacing the two-PAT-names predicate. Enabling writes therefore requires the primary App, which force-enables board ingestion — the unified shape (DEC-1/DEC-3, DL-305), amending the earlier independent-gates ruling. - `warnPartialForgeWriteSecrets` re-keyed to the App-based partial (exactly one App configured warns once). ### PATs deleted - Removed `GITHUB_FORGE_TOKEN`, `GITHUB_FORGE_REVIEWER_TOKEN`, `LINEAR_FORGE_TOKEN` secret names — fields, flags, env, defaults — and the now-dead `forgeTokenSource` type. New flags: `--forge-reviewer-app-id`/`-installation-id`/`-key-secret`, `--forge-linear-client-id`/`-client-secret` (with `$COMPASS_FORGE_*` env precedence). ### Tests - `TestForgeWriteAppsGate` (App-based enablement, incl. the force-couples-board-ingestion case), `TestBuildLinearNotifyLaneGate` and `TestBuildLinearTokenSourceGate` (token-source gating + the partial-config Warn), `TestWarnPartialForgeWriteSecrets` (App roles), `main_forge_test.go` reviewer + Linear flag mapping. - `TestForgeLanesShareOneBudgetGate` extended: the author write leg rides the armed shared client and fast-fails `ErrBudgetExhausted` with zero extra HTTP calls — the regression guard proving author writes and reads share one budget gate (not a separate one a fresh client would carry). No `panic`; every credential validated at boot (fail-fast). No fallback PAT path. Refs RIG-3090 Co-authored-by: Matt Wilkinson --- go/cmd/compass-server/main.go | 145 ++++-- go/cmd/compass-server/main_forge_test.go | 100 ++-- go/internal/forge/linear.go | 2 +- go/server/forge_e2e_pgtest_test.go | 3 +- go/server/forge_notify_pgtest_test.go | 6 +- go/server/serve.go | 620 +++++++++++++---------- go/server/serve_forge_budget_test.go | 29 ++ go/server/serve_forge_pgtest_test.go | 22 +- go/server/serve_forge_test.go | 353 ++++++------- go/server/sinks.go | 6 +- 10 files changed, 719 insertions(+), 567 deletions(-) diff --git a/go/cmd/compass-server/main.go b/go/cmd/compass-server/main.go index a98798b1f..5486a3f13 100644 --- a/go/cmd/compass-server/main.go +++ b/go/cmd/compass-server/main.go @@ -375,14 +375,18 @@ func resolveNetworkDoor(listen, tlsCert, tlsKey string) (string, *server.TLSConf // forgeFlags holds the RIG-1810/RIG-2883 forge CLI flag pointers, registered as // a group so run() stays short (they mirror the S3 flag set's precedence). type forgeFlags struct { - repos *string - secret *string - host *string - appID *string - installationID *string - appKeySecret *string - appWebhook *string - linearWebhook *string + repos *string + host *string + appID *string + installationID *string + appKeySecret *string + appWebhook *string + reviewerAppID *string + reviewerInstallationID *string + reviewerAppKeySecret *string + linearClientID *string + linearClientSecret *string + linearWebhook *string } // registerForgeFlags declares the forge flags on the given FlagSet and returns @@ -395,26 +399,49 @@ func registerForgeFlags(fs *flag.FlagSet) forgeFlags { "(RIG-2883 board ingestion). Defaults to $COMPASS_FORGE_REPOS. A declarative "+ "seed reconciled at boot (bootstrap-only insert), NOT the live target "+ "set — the table is authoritative after the first insert."), - secret: fs.String("forge-secret", "", - "Declared server_only secret NAME holding the forge token (the VALUE never "+ - "crosses a flag). Defaults to $COMPASS_FORGE_SECRET, then GITHUB_FORGE_TOKEN."), host: fs.String("forge-host", "", "Forge host the board lane binds (github.com or a GHES host; the API base "+ "derives from it). Defaults to $COMPASS_FORGE_HOST, then github.com."), appID: fs.String("forge-app-id", "", - "GitHub App id (numeric) the board webhook lane runs on (RIG-2883, App-only). "+ - "Defaults to $COMPASS_FORGE_APP_ID. Board ingestion runs iff this is set "+ - "AND both App secrets are declared."), + "PRIMARY GitHub App id (numeric): serves board reads, notify reads, author "+ + "writes, board, and webhooks (2-App topology). Defaults to "+ + "$COMPASS_FORGE_APP_ID. Board ingestion runs iff this is set AND both App "+ + "secrets are declared; the forge-WRITE path additionally requires the "+ + "reviewer App."), installationID: fs.String("forge-installation-id", "", - "GitHub App installation id the token is minted for. Defaults to "+ + "PRIMARY GitHub App installation id the token is minted for. Defaults to "+ "$COMPASS_FORGE_INSTALLATION_ID."), appKeySecret: fs.String("forge-app-key-secret", "", - "Declared server_only secret NAME holding the App PEM private key (the VALUE "+ - "never crosses a flag). Defaults to $COMPASS_FORGE_APP_KEY_SECRET."), + "Declared server_only secret NAME holding the PRIMARY App PEM private key "+ + "(the VALUE never crosses a flag). Defaults to "+ + "$COMPASS_FORGE_APP_KEY_SECRET."), appWebhook: fs.String("forge-app-webhook-secret", "", "Declared server_only secret NAME holding the webhook signing secret the "+ "ingress verifies deliveries against. Defaults to "+ "$COMPASS_FORGE_APP_WEBHOOK_SECRET."), + reviewerAppID: fs.String("forge-reviewer-app-id", "", + "REVIEWER GitHub App id (numeric): a distinct App identity serving ONLY the "+ + "reviewer write client (F1 author-cannot-approve-own-PR). Defaults to "+ + "$COMPASS_FORGE_REVIEWER_APP_ID. The forge-WRITE path runs iff this AND the "+ + "primary App are both configured. The reviewer App registers no webhook."), + reviewerInstallationID: fs.String("forge-reviewer-app-installation-id", "", + "REVIEWER GitHub App installation id the token is minted for. Defaults to "+ + "$COMPASS_FORGE_REVIEWER_APP_INSTALLATION_ID."), + reviewerAppKeySecret: fs.String("forge-reviewer-app-key-secret", "", + "Declared server_only secret NAME holding the REVIEWER App PEM private key "+ + "(the VALUE never crosses a flag; conventionally "+ + "FORGE_REVIEWER_APP_PRIVATE_KEY). Defaults to "+ + "$COMPASS_FORGE_REVIEWER_APP_KEY_SECRET."), + linearClientID: fs.String("forge-linear-client-id", "", + "Declared server_only secret NAME holding the Linear OAuth client id (the "+ + "client-credentials actor=app pair, the VALUE never crosses a flag). "+ + "Defaults to $COMPASS_FORGE_LINEAR_CLIENT_ID, then LINEAR_FORGE_CLIENT_ID. "+ + "The Linear write + notify lanes run iff BOTH this and the client secret "+ + "are declared."), + linearClientSecret: fs.String("forge-linear-client-secret", "", + "Declared server_only secret NAME holding the Linear OAuth client secret "+ + "(the VALUE never crosses a flag). Defaults to "+ + "$COMPASS_FORGE_LINEAR_CLIENT_SECRET, then LINEAR_FORGE_CLIENT_SECRET."), linearWebhook: fs.String("forge-linear-webhook-secret", "", "Declared server_only secret NAME holding the Linear webhook signing "+ "secret the shared POST /webhooks/linear ingress verifies deliveries "+ @@ -427,48 +454,84 @@ func registerForgeFlags(fs *flag.FlagSet) forgeFlags { // resolve applies the flag-then-env precedence to each forge flag and delegates // to resolveForge (the pure input->output core, unit-tested directly). func (f forgeFlags) resolve() (server.ForgeConfig, error) { - return resolveForge( - firstNonEmpty(*f.repos, os.Getenv("COMPASS_FORGE_REPOS")), - firstNonEmpty(*f.secret, os.Getenv("COMPASS_FORGE_SECRET")), - firstNonEmpty(*f.host, os.Getenv("COMPASS_FORGE_HOST")), - firstNonEmpty(*f.appID, os.Getenv("COMPASS_FORGE_APP_ID")), - firstNonEmpty(*f.installationID, os.Getenv("COMPASS_FORGE_INSTALLATION_ID")), - firstNonEmpty(*f.appKeySecret, os.Getenv("COMPASS_FORGE_APP_KEY_SECRET")), - firstNonEmpty(*f.appWebhook, os.Getenv("COMPASS_FORGE_APP_WEBHOOK_SECRET")), - firstNonEmpty(*f.linearWebhook, os.Getenv("COMPASS_FORGE_LINEAR_WEBHOOK_SECRET")), - ) + return resolveForge(forgeInputs{ + repos: firstNonEmpty(*f.repos, os.Getenv("COMPASS_FORGE_REPOS")), + host: firstNonEmpty(*f.host, os.Getenv("COMPASS_FORGE_HOST")), + appID: firstNonEmpty(*f.appID, os.Getenv("COMPASS_FORGE_APP_ID")), + installationID: firstNonEmpty(*f.installationID, os.Getenv("COMPASS_FORGE_INSTALLATION_ID")), + appKeySecret: firstNonEmpty(*f.appKeySecret, os.Getenv("COMPASS_FORGE_APP_KEY_SECRET")), + appWebhook: firstNonEmpty(*f.appWebhook, os.Getenv("COMPASS_FORGE_APP_WEBHOOK_SECRET")), + reviewerAppID: firstNonEmpty(*f.reviewerAppID, os.Getenv("COMPASS_FORGE_REVIEWER_APP_ID")), + reviewerInstallationID: firstNonEmpty(*f.reviewerInstallationID, os.Getenv("COMPASS_FORGE_REVIEWER_APP_INSTALLATION_ID")), + reviewerAppKeySecret: firstNonEmpty(*f.reviewerAppKeySecret, os.Getenv("COMPASS_FORGE_REVIEWER_APP_KEY_SECRET")), + linearClientID: firstNonEmpty(*f.linearClientID, os.Getenv("COMPASS_FORGE_LINEAR_CLIENT_ID")), + linearClientSecret: firstNonEmpty(*f.linearClientSecret, os.Getenv("COMPASS_FORGE_LINEAR_CLIENT_SECRET")), + linearWebhook: firstNonEmpty(*f.linearWebhook, os.Getenv("COMPASS_FORGE_LINEAR_WEBHOOK_SECRET")), + }) +} + +// forgeInputs is the already-resolved (flag-then-env) forge input set the pure +// resolveForge core maps onto server.ForgeConfig. A struct rather than a long +// positional list so a new knob is a named field, not another unlabeled arg. +type forgeInputs struct { + repos string + host string + appID string + installationID string + appKeySecret string + appWebhook string + reviewerAppID string + reviewerInstallationID string + reviewerAppKeySecret string + linearClientID string + linearClientSecret string + linearWebhook string } -// resolveForge turns the forge flags (already flag-then-env resolved) into the +// resolveForge turns the forge inputs (already flag-then-env resolved) into the // ServeConfig.Forge surface, mirroring resolveNetworkDoor's shape: pure // input->output, no I/O. The repos string is a comma-separated owner/name list; // each entry is validated (garbage is a startup error) and lowercased for GITHUB -// so Owner/Name and owner/name collapse to one target. appID/installationID are -// parsed as int64 when set (garbage is a startup error); empty leaves them zero. -// Empty host/secret/App-secret NAMEs default server-side. -func resolveForge(repos, secret, host, appID, installationID, appKeySecret, appWebhook, linearWebhook string) (server.ForgeConfig, error) { - seed, err := parseForgeRepos(repos) +// so Owner/Name and owner/name collapse to one target. App ids are parsed as +// int64 when set (garbage is a startup error); empty leaves them zero. Empty +// host / secret NAMEs are left zero for server-side defaulting. +func resolveForge(in forgeInputs) (server.ForgeConfig, error) { + seed, err := parseForgeRepos(in.repos) + if err != nil { + return server.ForgeConfig{}, err + } + id, err := parseForgeInt(in.appID, "--forge-app-id") if err != nil { return server.ForgeConfig{}, err } - id, err := parseForgeInt(appID, "--forge-app-id") + instID, err := parseForgeInt(in.installationID, "--forge-installation-id") if err != nil { return server.ForgeConfig{}, err } - instID, err := parseForgeInt(installationID, "--forge-installation-id") + reviewerID, err := parseForgeInt(in.reviewerAppID, "--forge-reviewer-app-id") + if err != nil { + return server.ForgeConfig{}, err + } + reviewerInstID, err := parseForgeInt(in.reviewerInstallationID, "--forge-reviewer-app-installation-id") if err != nil { return server.ForgeConfig{}, err } return server.ForgeConfig{ - Host: host, - SeedRepos: seed, - SecretName: secret, - LinearWebhookSecretName: linearWebhook, + Host: in.host, + SeedRepos: seed, + LinearClientIDSecretName: in.linearClientID, + LinearClientSecretName: in.linearClientSecret, + LinearWebhookSecretName: in.linearWebhook, App: server.ForgeAppConfig{ AppID: id, InstallationID: instID, - AppPrivateKeySecret: appKeySecret, - AppWebhookSecretName: appWebhook, + AppPrivateKeySecret: in.appKeySecret, + AppWebhookSecretName: in.appWebhook, + }, + ReviewerApp: server.ForgeAppConfig{ + AppID: reviewerID, + InstallationID: reviewerInstID, + AppPrivateKeySecret: in.reviewerAppKeySecret, }, }, nil } diff --git a/go/cmd/compass-server/main_forge_test.go b/go/cmd/compass-server/main_forge_test.go index 9b90ef934..abfd3fe74 100644 --- a/go/cmd/compass-server/main_forge_test.go +++ b/go/cmd/compass-server/main_forge_test.go @@ -13,66 +13,73 @@ package main // unique to forge. import ( + "reflect" "strings" "testing" + + "github.com/RigelBuild/compass/go/server" ) func TestResolveForgeMapping(t *testing.T) { - t.Run("disabled default: no repos, no App", func(t *testing.T) { - fc, err := resolveForge("", "", "", "", "", "", "", "") + t.Run("disabled default: no repos, no Apps", func(t *testing.T) { + fc, err := resolveForge(forgeInputs{}) if err != nil { t.Fatalf("resolveForge: %v", err) } - if len(fc.SeedRepos) != 0 { - t.Fatalf("SeedRepos = %v, want empty", fc.SeedRepos) - } - if fc.App.AppID != 0 || fc.App.InstallationID != 0 { - t.Fatalf("App ids = %d/%d, want 0/0", fc.App.AppID, fc.App.InstallationID) - } - // Empty host/secret/App-secret NAMEs are left zero for server-side - // defaulting — resolveForge must not bake defaults the ServeConfig owns. - if fc.Host != "" || fc.SecretName != "" || - fc.App.AppPrivateKeySecret != "" || fc.App.AppWebhookSecretName != "" { - t.Fatalf("empty inputs should stay zero, got host=%q secret=%q key=%q webhook=%q", - fc.Host, fc.SecretName, fc.App.AppPrivateKeySecret, fc.App.AppWebhookSecretName) + // Everything stays zero: no seed, no Apps, and — crucially — empty host / + // secret NAMEs are NOT defaulted here (server-side ServeConfig owns those + // defaults). A zero-value ForgeConfig is exactly that contract. + if want := (server.ForgeConfig{}); !reflect.DeepEqual(fc, want) { + t.Fatalf("empty inputs should map to a zero ForgeConfig\n got %+v\nwant %+v", fc, want) } }) t.Run("full flag mapping", func(t *testing.T) { - fc, err := resolveForge("owner/repo, foo/bar", "MY_TOKEN", "ghe.example.com", - "12345", "678", "APP_KEY", "WEBHOOK_SECRET", "LINEAR_WEBHOOK_SECRET") + fc, err := resolveForge(forgeInputs{ + repos: "owner/repo, foo/bar", + host: "ghe.example.com", + appID: "12345", + installationID: "678", + appKeySecret: "APP_KEY", + appWebhook: "WEBHOOK_SECRET", + reviewerAppID: "222", + reviewerInstallationID: "333", + reviewerAppKeySecret: "REVIEWER_APP_KEY", + linearClientID: "LINEAR_CID", + linearClientSecret: "LINEAR_CSECRET", + linearWebhook: "LINEAR_WEBHOOK_SECRET", + }) if err != nil { t.Fatalf("resolveForge: %v", err) } - if fc.Host != "ghe.example.com" { - t.Fatalf("Host = %q, want ghe.example.com", fc.Host) - } - if fc.SecretName != "MY_TOKEN" { - t.Fatalf("SecretName = %q, want MY_TOKEN", fc.SecretName) + // The whole mapping in one comparison: ids parsed to int64, secret NAMEs + // threaded verbatim, repos split + lowercased, and the reviewer App's + // webhook NAME left empty (it registers no webhook). + want := server.ForgeConfig{ + Host: "ghe.example.com", + SeedRepos: []string{"owner/repo", "foo/bar"}, + LinearClientIDSecretName: "LINEAR_CID", + LinearClientSecretName: "LINEAR_CSECRET", + LinearWebhookSecretName: "LINEAR_WEBHOOK_SECRET", + App: server.ForgeAppConfig{ + AppID: 12345, + InstallationID: 678, + AppPrivateKeySecret: "APP_KEY", + AppWebhookSecretName: "WEBHOOK_SECRET", + }, + ReviewerApp: server.ForgeAppConfig{ + AppID: 222, + InstallationID: 333, + AppPrivateKeySecret: "REVIEWER_APP_KEY", + }, } - if fc.App.AppID != 12345 || fc.App.InstallationID != 678 { - t.Fatalf("App ids = %d/%d, want 12345/678", fc.App.AppID, fc.App.InstallationID) - } - if fc.App.AppPrivateKeySecret != "APP_KEY" || fc.App.AppWebhookSecretName != "WEBHOOK_SECRET" { - t.Fatalf("App secrets = %q/%q, want APP_KEY/WEBHOOK_SECRET", - fc.App.AppPrivateKeySecret, fc.App.AppWebhookSecretName) - } - if fc.LinearWebhookSecretName != "LINEAR_WEBHOOK_SECRET" { - t.Fatalf("LinearWebhookSecretName = %q, want LINEAR_WEBHOOK_SECRET", fc.LinearWebhookSecretName) - } - want := []string{"owner/repo", "foo/bar"} - if len(fc.SeedRepos) != len(want) { - t.Fatalf("SeedRepos = %v, want %v", fc.SeedRepos, want) - } - for i, w := range want { - if fc.SeedRepos[i] != w { - t.Fatalf("SeedRepos[%d] = %q, want %q", i, fc.SeedRepos[i], w) - } + if !reflect.DeepEqual(fc, want) { + t.Fatalf("full flag mapping mismatch\n got %+v\nwant %+v", fc, want) } }) t.Run("case normalization: Owner/Name lowercases to one target", func(t *testing.T) { - fc, err := resolveForge("Owner/Name", "", "", "", "", "", "", "") + fc, err := resolveForge(forgeInputs{repos: "Owner/Name"}) if err != nil { t.Fatalf("resolveForge: %v", err) } @@ -94,7 +101,7 @@ func TestResolveForgeRejectsGarbage(t *testing.T) { } for _, tc := range garbage { t.Run(tc.name, func(t *testing.T) { - _, err := resolveForge(tc.repos, "", "", "", "", "", "", "") + _, err := resolveForge(forgeInputs{repos: tc.repos}) if err == nil { t.Fatalf("resolveForge(%q) = nil error, want a startup error", tc.repos) } @@ -107,13 +114,16 @@ func TestResolveForgeRejectsGarbage(t *testing.T) { func TestResolveForgeRejectsBadAppID(t *testing.T) { for _, tc := range []struct { - name, appID, installID, wantFlag string + name, wantFlag string + in forgeInputs }{ - {"non-numeric app id", "notanumber", "", "--forge-app-id"}, - {"non-numeric installation id", "123", "nope", "--forge-installation-id"}, + {"non-numeric app id", "--forge-app-id", forgeInputs{repos: "owner/repo", appID: "notanumber"}}, + {"non-numeric installation id", "--forge-installation-id", forgeInputs{repos: "owner/repo", appID: "123", installationID: "nope"}}, + {"non-numeric reviewer app id", "--forge-reviewer-app-id", forgeInputs{repos: "owner/repo", reviewerAppID: "nope"}}, + {"non-numeric reviewer installation id", "--forge-reviewer-app-installation-id", forgeInputs{repos: "owner/repo", reviewerAppID: "123", reviewerInstallationID: "nope"}}, } { t.Run(tc.name, func(t *testing.T) { - _, err := resolveForge("owner/repo", "", "", tc.appID, tc.installID, "", "", "") + _, err := resolveForge(tc.in) if err == nil { t.Fatalf("resolveForge = nil error, want a startup error") } diff --git a/go/internal/forge/linear.go b/go/internal/forge/linear.go index 4810d3ae5..5a3b22363 100644 --- a/go/internal/forge/linear.go +++ b/go/internal/forge/linear.go @@ -79,7 +79,7 @@ var linearClosedStateTypes = []string{"completed", "canceled"} // LinearConfig configures a Linear client. type LinearConfig struct { Host string // GraphQL endpoint URL; "" -> linearDefaultEndpoint - Token TokenSource // required (its own LINEAR_FORGE_TOKEN, DL-052) + Token TokenSource // required (the shared Linear OAuth client-credentials source, DL-052) Client *http.Client // nil -> a default client with a sane timeout Log *slog.Logger // nil -> slog.Default(); carries the degrade log line } diff --git a/go/server/forge_e2e_pgtest_test.go b/go/server/forge_e2e_pgtest_test.go index e4371af66..f01c1885a 100644 --- a/go/server/forge_e2e_pgtest_test.go +++ b/go/server/forge_e2e_pgtest_test.go @@ -418,7 +418,8 @@ func TestForgeNoLiveSessionFailsClosedOverTheWire(t *testing.T) { // provider=LINEAR) comes back as an in-band `unimplemented` ForgeCallError — // Linear is issues-only (DL-051), its PR half returns ErrUnsupported, which the // chokepoint flattens to unimplemented. Present because serve.go registers a -// Linear coordinate when LINEAR_FORGE_TOKEN is declared (not left as a TODO), and +// Linear coordinate when the Linear OAuth client-credentials pair is declared +// (not left as a TODO), and // Linear satisfies forge.Provider. func TestForgeLinearUnimplementedOverTheWire(t *testing.T) { w := newForgeE2EWire(t) diff --git a/go/server/forge_notify_pgtest_test.go b/go/server/forge_notify_pgtest_test.go index 2193459f9..a5ab9ed50 100644 --- a/go/server/forge_notify_pgtest_test.go +++ b/go/server/forge_notify_pgtest_test.go @@ -145,15 +145,15 @@ func TestForgeNotifyLaneDisabledWithoutApp(t *testing.T) { // issueBrd is nil-safe here: the App-absent gate short-circuits before any // lane assembly, so the projection is never dereferenced. - lane, notifyLane, sink, secret, err := buildBoardWebhookWiring(ctx, ServeConfig{Forge: ForgeConfig{Host: forgeTestHost}}, st, nil, nil, &fakeResolver{}, slog.Default()) + lane, notifyLane, sink, secret, client, err := buildBoardWebhookWiring(ctx, ServeConfig{Forge: ForgeConfig{Host: forgeTestHost}}, st, nil, nil, &fakeResolver{}, slog.Default()) if err != nil { t.Fatalf("buildBoardWebhookWiring (App absent): %v", err) } if notifyLane != nil { t.Fatal("notifyLane != nil with no App configured, want nil (notify lane hard-off)") } - if lane != nil || sink != nil || secret != nil { - t.Fatalf("wiring not all-nil with no App configured: lane==nil? %t sink==nil? %t secret==nil? %t", lane == nil, sink == nil, secret == nil) + if lane != nil || sink != nil || secret != nil || client != nil { + t.Fatalf("wiring not all-nil with no App configured: lane==nil? %t sink==nil? %t secret==nil? %t client==nil? %t", lane == nil, sink == nil, secret == nil, client == nil) } } diff --git a/go/server/serve.go b/go/server/serve.go index bc0312eda..2c44bf9fd 100644 --- a/go/server/serve.go +++ b/go/server/serve.go @@ -42,6 +42,7 @@ import ( "github.com/RigelBuild/compass/go/internal/forge" compassv1internal "github.com/RigelBuild/compass/go/internal/gen/compass/v1" "github.com/RigelBuild/compass/go/internal/ingest" + "github.com/RigelBuild/compass/go/internal/linearagent" "github.com/RigelBuild/compass/go/internal/otel" "github.com/RigelBuild/compass/go/internal/runnerhub" "github.com/RigelBuild/compass/go/internal/secrets" @@ -140,16 +141,25 @@ type ForgeConfig struct { // App secrets are declared; otherwise board ingestion is hard-off with a // boot Warn. No PAT fallback on the read path (Constraint #3). App ForgeAppConfig - // SecretName is the declared server_only secret NAME holding the forge token - // (default "GITHUB_FORGE_TOKEN"; the VALUE never crosses config or a flag). - SecretName string - // ReviewerSecretName is the declared server_only secret NAME holding the - // REVIEWER forge token (default "GITHUB_FORGE_REVIEWER_TOKEN"; the VALUE - // never crosses config or a flag). A distinct GitHub identity from the - // author token so an agent approving a PR it authored is a different account - // (F1). The agent forge-WRITE path is enabled iff BOTH this and SecretName - // resolve to a declared secret (Matt's 2026-08-19 ruling). - ReviewerSecretName string + // ReviewerApp is the SECOND GitHub App credential — a distinct App + // definition (own AppID + private key + one installation) serving ONLY the + // reviewer write client (the submit_review arm). A distinct GitHub identity + // from the primary App so an agent approving a PR it authored dispatches + // submit_review on a different account than it authored with, dissolving the + // author-approving-own-PR 422 at the credential layer (F1, DEC-1). The + // reviewer App registers NO webhook and no read lane, so its + // AppWebhookSecretName is unused; reads/webhooks/board/author-writes all ride + // the primary App (2-App topology, DEC-3). + ReviewerApp ForgeAppConfig + // LinearClientIDSecretName / LinearClientSecretName are the declared + // server_only secret NAMEs holding the Linear OAuth client-credentials pair + // (actor=app, the RIG-2682 "Compass" app). The Linear write + notify lanes + // mint one shared client-credentials token from this pair (never a member + // PAT); a Linear coordinate + notify lane are wired iff BOTH names resolve to + // a declared secret (the VALUEs never cross config or a flag). Default to + // LINEAR_FORGE_CLIENT_ID / LINEAR_FORGE_CLIENT_SECRET. + LinearClientIDSecretName string + LinearClientSecretName string // LinearWebhookSecretName is the declared server_only secret NAME holding // the Linear webhook signing secret the shared POST /webhooks ingress // verifies deliveries against (the VALUE never crosses config or a flag). @@ -182,17 +192,15 @@ type ForgeAppConfig struct { // Forge config defaults, applied by resolveForge when a field is zero. const ( - defaultForgeHost = "github.com" - defaultForgeSecretName = "GITHUB_FORGE_TOKEN" //nolint:gosec // G101: this is the default declared-secret NAME (an env-var identifier), not a credential value — the value is resolved from the secrets provider, never hardcoded - // defaultForgeReviewerSecretName is the default declared-secret NAME holding - // the REVIEWER forge token (F1) — a distinct identity from the author token. - // A secret NAME, not a value (see defaultForgeSecretName's gosec note). - defaultForgeReviewerSecretName = "GITHUB_FORGE_REVIEWER_TOKEN" //nolint:gosec // G101: the default declared-secret NAME (an env-var identifier), not a credential value — resolved from the secrets provider, never hardcoded - // defaultForgeLinearSecretName is the declared-secret NAME holding the Linear - // write token (DL-051/DL-052). A Linear write coordinate is registered ONLY - // when this secret is declared; otherwise the write path is GitHub-only. A - // secret NAME, not a value. - defaultForgeLinearSecretName = "LINEAR_FORGE_TOKEN" //nolint:gosec // G101: the default declared-secret NAME (an env-var identifier), not a credential value — resolved from the secrets provider, never hardcoded + defaultForgeHost = "github.com" + // defaultForgeLinearClientIDSecretName / defaultForgeLinearClientSecretName + // are the default declared-secret NAMEs holding the Linear OAuth + // client-credentials pair (actor=app, the RIG-2682 "Compass" app). The Linear + // write coordinate + notify lane are wired iff BOTH resolve to a declared + // secret; otherwise the write path is GitHub-only and the notify lane is off. + // Secret NAMEs, not values (see the gosec note below). + defaultForgeLinearClientIDSecretName = "LINEAR_FORGE_CLIENT_ID" //nolint:gosec // G101: the default declared-secret NAME (an env-var identifier), not a credential value — resolved from the secrets provider, never hardcoded + defaultForgeLinearClientSecretName = "LINEAR_FORGE_CLIENT_SECRET" //nolint:gosec // G101: the default declared-secret NAME (an env-var identifier), not a credential value — resolved from the secrets provider, never hardcoded // defaultReconcileBackstop is the board reconciler's default sweep cadence // (OQ-5): startup sweep + a 30-min ticker (a 304 page-1 GET per enabled repo // is ≈ free, notify_reader.go:12-13; a cold-start zero watermark walks once). @@ -216,18 +224,19 @@ func (c ForgeConfig) boardIngestionEnabled() bool { return c.App.AppID != 0 } -// resolved returns the config with its zero fields defaulted (Host, SecretName, -// ReviewerSecretName, App.ReconcileBackstop). SeedRepos and App ids are taken -// verbatim. +// resolved returns the config with its zero fields defaulted (Host, the Linear +// client-credentials secret NAMEs, App.ReconcileBackstop). SeedRepos and App ids +// are taken verbatim; App private-key/webhook secret NAMEs are operator-set with +// no default (a configured App names them explicitly). func (c ForgeConfig) resolved() ForgeConfig { if c.Host == "" { c.Host = defaultForgeHost } - if c.SecretName == "" { - c.SecretName = defaultForgeSecretName + if c.LinearClientIDSecretName == "" { + c.LinearClientIDSecretName = defaultForgeLinearClientIDSecretName } - if c.ReviewerSecretName == "" { - c.ReviewerSecretName = defaultForgeReviewerSecretName + if c.LinearClientSecretName == "" { + c.LinearClientSecretName = defaultForgeLinearClientSecretName } if c.App.ReconcileBackstop <= 0 { c.App.ReconcileBackstop = defaultReconcileBackstop @@ -236,35 +245,47 @@ func (c ForgeConfig) resolved() ForgeConfig { } // forgeWritesEnabled reports whether the agent forge-WRITE path is enabled: iff -// BOTH the author secret (SecretName) and the reviewer secret -// (ReviewerSecretName) resolve to a name present in declared (Matt's 2026-08-19 -// ruling — independent of boardIngestionEnabled, both secrets required). It is a -// pure predicate over the resolved declared-secret set so the "enabled = both -// declared" rule is unit-testable without a running Serve; buildForgeWriteService -// re-validates each name through validateForgeSecret to fail fast with the two -// distinct texts. Called on the resolved() config so the defaulted names apply. +// BOTH the primary App and the reviewer App are configured — each with AppID != 0 +// AND its private-key secret present in declared (the 2-App cutover, DEC-1/DEC-3, +// re-keying the retired two-PAT-names predicate). Requiring the primary App to +// enable writes force-enables board ingestion (boardIngestionEnabled keys on the +// same App.AppID) — the unified shape Matt explicitly wants (DL-305). It is a +// pure predicate over the resolved declared-secret set so the rule is +// unit-testable without a running Serve; buildForgeWriteService re-validates each +// App key through validateForgeSecret to fail fast. Called on the resolved() +// config so the defaulted names apply. func (c ForgeConfig) forgeWritesEnabled(declared []secrets.ResolvedSecret) bool { - haveAuthor, haveReviewer := c.forgeWriteSecretsDeclared(declared) - return haveAuthor && haveReviewer + havePrimary, haveReviewer := c.forgeWriteAppsConfigured(declared) + return havePrimary && haveReviewer +} + +// forgeWriteAppsConfigured reports which of the two required write Apps — the +// primary (c.App) and the reviewer (c.ReviewerApp) — are configured: AppID != 0 +// AND the App's private-key secret NAME present in the resolved declared set. +// Both true is the writes-enabled state (forgeWritesEnabled); exactly one true is +// a partial misconfiguration warnPartialForgeWriteSecrets surfaces. Resolves the +// config internally so any defaulted names apply — the caller need not +// pre-resolve. +func (c ForgeConfig) forgeWriteAppsConfigured(declared []secrets.ResolvedSecret) (havePrimary, haveReviewer bool) { + fc := c.resolved() + havePrimary = fc.App.AppID != 0 && secretDeclared(declared, fc.App.AppPrivateKeySecret) + haveReviewer = fc.ReviewerApp.AppID != 0 && secretDeclared(declared, fc.ReviewerApp.AppPrivateKeySecret) + return havePrimary, haveReviewer } -// forgeWriteSecretsDeclared reports which of the two required write secrets — -// the author (SecretName) and the reviewer (ReviewerSecretName) — are present -// in the resolved declared set. Both true is the writes-enabled state -// (forgeWritesEnabled); exactly one true is a partial misconfiguration -// warnPartialForgeWriteSecrets surfaces. Resolves the config internally so the -// defaulted names apply — the caller need not pre-resolve. -func (c ForgeConfig) forgeWriteSecretsDeclared(declared []secrets.ResolvedSecret) (haveAuthor, haveReviewer bool) { - fc := c.resolved() +// secretDeclared reports whether a non-empty name is present in the resolved +// declared set. An empty name is never declared (an App with a zero key-secret +// name is not configured). +func secretDeclared(declared []secrets.ResolvedSecret, name string) bool { + if name == "" { + return false + } for _, s := range declared { - switch s.Name { - case fc.SecretName: - haveAuthor = true - case fc.ReviewerSecretName: - haveReviewer = true + if s.Name == name { + return true } } - return haveAuthor, haveReviewer + return false } // The bootstrap-admin identity the local-socket door attributes callers to until @@ -414,9 +435,7 @@ func Serve(ctx context.Context, cfg ServeConfig) error { // Owner-only: the socket is the server's whole trust boundary on the local // machine, so no other user may connect. if err := os.Chmod(cfg.SocketPath, 0o600); err != nil { - udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing chmod path — nothing actionable remains (errcheck + its gosec G104 twin) - listeners.close() - return fmt.Errorf("chmod 0600 %s: %w", cfg.SocketPath, err) + return failStartup(udsListener, listeners, fmt.Errorf("chmod 0600 %s: %w", cfg.SocketPath, err)) } // Pin the inode we bound so shutdown cleanup can tell our socket apart from @@ -436,9 +455,7 @@ func Serve(ctx context.Context, cfg ServeConfig) error { // mid-request. st, err := openStore(ctx, cfg) if err != nil { - udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing startup path — nothing actionable remains (errcheck + its gosec G104 twin) - listeners.close() - return err + return failStartup(udsListener, listeners, err) } defer st.Close() @@ -452,9 +469,7 @@ func Serve(ctx context.Context, cfg ServeConfig) error { // observable at boot. admin, systemAccount, err := seedBootstrapAccounts(ctx, st, cfg) if err != nil { - udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing startup path — nothing actionable remains (errcheck + its gosec G104 twin) - listeners.close() - return err + return failStartup(udsListener, listeners, err) } // Log the resolved public base URL so an operator can see which host the @@ -486,9 +501,7 @@ func Serve(ctx context.Context, cfg ServeConfig) error { // subscribed yet at boot, so it does not publish. issueBrd := board.NewIssueProjection(bus, st) if err := issueBrd.Rehydrate(ctx); err != nil { - udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing startup path — nothing actionable remains (errcheck + its gosec G104 twin) - listeners.close() - return fmt.Errorf("rehydrating issue board: %w", err) + return failStartup(udsListener, listeners, fmt.Errorf("rehydrating issue board: %w", err)) } // The comms event stream rides a second bus instance — its own seq space and @@ -544,18 +557,15 @@ func Serve(ctx context.Context, cfg ServeConfig) error { // notifies live sessions to re-fetch); it shares the one resolver with FetchSecrets. secretsSvc := newSecretsService(st, resolver, hub) - // The board webhook-ingestion lane (RIG-2883) and its webhook ingress wiring, - // built BEFORE the doors because the network door mounts the lane's webhook - // ingress (sink + secret resolver, threaded into buildDoors). Fails fast HERE - // on the same udsListener.Close()+listeners.close() cleanup path the Rehydrate - // fault above uses. All returns are nil when the App is absent. The notify - // lane (T7) rides the SAME ingress via webhookSink; Serve starts its arm + - // reconciler alongside the board lane's below. - lane, notifyLane, webhookSink, webhookSecret, err := buildBoardWebhookWiring(ctx, cfg, st, issueBrd, hub, resolver, hubLog) + // The forge read-side credentials, built BEFORE the doors because the network + // door mounts the board lane's webhook ingress (sink + secret resolver) and + // the Linear notify lane, both threaded into buildDoors. Fails fast HERE on + // the shared cleanup path. Board fields are nil when the GitHub App is absent; + // forge.linearTokens is nil when Linear is not configured. Serve starts each + // lane's arm + reconciler below. + forgeWiring, err := buildForgeReadWiring(ctx, cfg, st, issueBrd, hub, resolver, hubLog) if err != nil { - udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing startup path — nothing actionable remains (errcheck + its gosec G104 twin) - listeners.close() - return err + return failStartup(udsListener, listeners, err) } // Assemble the three compass.v1 doors (shipped Unix socket, optional dev @@ -563,16 +573,17 @@ func Serve(ctx context.Context, cfg ServeConfig) error { // net door's /webhooks/linear handler feeds. On a net-door build error the // listeners this Serve bound are still ours to close. doors, err := buildDoors(ctx, cfg, svc, commsSvc, secretsSvc, hub, st, admin.ID, resolver, - devListener, netListener, netTLS, webhookSink, webhookSecret) + devListener, netListener, netTLS, forgeWiring.webhookSink, forgeWiring.webhookSecret, forgeWiring.linearTokens) if err != nil { - udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing startup path — nothing actionable remains (errcheck + its gosec G104 twin) - listeners.close() - return err + return failStartup(udsListener, listeners, err) } - // The forge-WRITE caller is independent of the board ingest lane (Matt's - // 2026-08-19 ruling): enabled iff BOTH write secrets are declared. - if err := wireForgeWriteCaller(ctx, cfg, st, issueBrd, resolver, hub, hubLog, udsListener, listeners); err != nil { + // The forge-WRITE caller: enabled iff BOTH the primary and reviewer Apps are + // configured (the 2-App cutover). The author write leg REUSES the shared + // primary App client (forgeWiring.primaryClient) so it rides the one budget + // gate; the reviewer leg gets its own App client; the Linear coordinate rides + // the shared forgeWiring.linearTokens instance. + if err := wireForgeWriteCaller(ctx, cfg, st, issueBrd, resolver, hub, hubLog, forgeWiring.primaryClient, forgeWiring.linearTokens, udsListener, listeners); err != nil { return err } @@ -601,10 +612,10 @@ func Serve(ctx context.Context, cfg ServeConfig) error { // group so they inherit the doors' lifecycle exactly — cancelled on // SIGINT/SIGTERM via gctx, first-error-wins, drained with everything else. // The board + GitHub notify lanes are nil when the GitHub App is absent (they - // share the App gate); the Linear notify lane is nil when LINEAR_FORGE_TOKEN - // is undeclared (its independent gate). A nil lane starts nothing. Every Run - // returns nil on ctx-cancel. - startForgeIngestLanes(gctx, g, lane, notifyLane, doors.linearNotify) + // share the App gate); the Linear notify lane is nil when Linear is not + // configured (its client-credentials pair undeclared). A nil lane starts + // nothing. Every Run returns nil on ctx-cancel. + startForgeIngestLanes(gctx, g, forgeWiring.boardLane, forgeWiring.notifyLane, doors.linearNotify) // The comms-bus consumers (RIG-1569): the T3 delivery fan-out consumer and // the T8 presence projection, both tailing the comms bus with their bus-tail // goroutines on the serve group rooted on gctx (cancels at shutdown; each also @@ -641,8 +652,9 @@ type serveDoors struct { dev *http.Server net *http.Server // linearNotify is the Linear agent-notification lane (RIG-2732 T7), built - // beside the webhook handler it feeds; nil when LINEAR_FORGE_TOKEN is - // undeclared. Serve starts its arm + reconciler on the serve group. + // beside the webhook handler it feeds; nil when Linear is not configured (its + // client-credentials pair undeclared). Serve starts its arm + reconciler on + // the serve group. linearNotify *forgeNotifyLane } @@ -667,6 +679,7 @@ func buildDoors( netTLS *tls.Config, webhookSink ForgeEventSink, webhookSecret func(ctx context.Context) ([]byte, error), + linearTokens *linearagent.TokenSource, ) (serveDoors, error) { // otelconnect produces the server RPC span (and, once a MeterProvider is // installed, RPC duration/count metrics); NewTraceResponseInterceptor stamps @@ -745,18 +758,15 @@ func buildDoors( } // The Linear agent-notification lane (RIG-2732 T7): App-INDEPENDENT, gated on - // LINEAR_FORGE_TOKEN. Built here — beside the webhook handler it feeds — so a - // resolve fault fail-fasts door assembly, and its data-change sink threads - // straight into buildLinearWebhookWiring below, replacing the - // injected-and-nil-for-now sink so a verified /webhooks/linear Issue/Comment - // event routes to subscribers instead of ack-and-drop. Nil when the secret is - // undeclared (the handler's data branch then acks-and-drops). The lane is - // returned in serveDoors so Serve can start its arm + reconciler on the serve - // group. - linearNotifyLane, err := buildLinearNotifyLane(ctx, st, hub, resolver, slog.Default()) - if err != nil { - return serveDoors{}, err - } + // the shared Linear client-credentials token source (nil when Linear is not + // configured). Built here — beside the webhook handler it feeds — so its + // data-change sink threads straight into buildLinearWebhookWiring below, + // replacing the injected-and-nil-for-now sink so a verified /webhooks/linear + // Issue/Comment event routes to subscribers instead of ack-and-drop. Nil when + // linearTokens is nil (the handler's data branch then acks-and-drops). The + // lane is returned in serveDoors so Serve can start its arm + reconciler on + // the serve group. + linearNotifyLane := buildLinearNotifyLane(st, hub, linearTokens, slog.Default()) var linearDataSink ForgeEventSink if linearNotifyLane != nil { linearDataSink = linearNotifyLane.sink @@ -768,10 +778,10 @@ func buildDoors( // notifications without a GitHub App). Its data-change arm's sink is the // Linear-provider-bound notify lane's sink (linearDataSink), so a verified // Issue/Comment event routes to subscribers at the LINEAR/linear.app - // coordinate; nil when the notify lane is off (LINEAR_FORGE_TOKEN undeclared), - // and the handler's data branch then acks-and-drops. The two gates are - // independent: the webhook secret gates the handler; LINEAR_FORGE_TOKEN gates - // the sink. Its session arm is left unwired (nil sessionSink -> logged-drop) + // coordinate; nil when the notify lane is off (Linear not configured), and the + // handler's data branch then acks-and-drops. The two gates are independent: + // the webhook secret gates the handler; the Linear client-credentials pair + // gates the sink. Its session arm is left unwired (nil sessionSink -> logged-drop) // until the RIG-2717 responder assembly wires a *linearagent.Dispatcher here. // The handler is mounted only on the net door below, when one exists. linearWebhookHandler, err := buildLinearWebhookWiring(ctx, cfg, resolver, linearDataSink, slog.Default()) @@ -911,6 +921,55 @@ type boardIngestLane struct { client *forge.GitHub } +// forgeReadWiring bundles the forge read-side credentials Serve builds once and +// threads into the doors + the write path: the board + notify lanes over the +// shared primary App client (the board arm's webhook ingress sink + secret), the +// shared primary App client the author write leg reuses, and the ONE Linear +// OAuth token source both the notify lane and the Linear write coordinate ride. +// Board fields are nil when the GitHub App is absent; linearTokens is nil when +// Linear is not configured. +type forgeReadWiring struct { + boardLane *boardIngestLane + notifyLane *forgeNotifyLane + webhookSink ForgeEventSink + webhookSecret func(ctx context.Context) ([]byte, error) + primaryClient *forge.GitHub + linearTokens *linearagent.TokenSource +} + +// buildForgeReadWiring assembles the forge read-side credentials in one call: +// the board webhook wiring (both App lanes + the shared primary client) and the +// shared Linear OAuth token source. Either half is independently off (App absent +// -> nil board fields; Linear unconfigured -> nil linearTokens); a resolve or +// boot-time-mint fault from either is returned so Serve fails fast on its +// cleanup path. +func buildForgeReadWiring( + ctx context.Context, + cfg ServeConfig, + st *store.Store, + issueBrd *board.IssueProjection, + hub *runnerhub.Hub, + resolver secrets.Resolver, + log *slog.Logger, +) (forgeReadWiring, error) { + boardLane, notifyLane, webhookSink, webhookSecret, primaryClient, err := buildBoardWebhookWiring(ctx, cfg, st, issueBrd, hub, resolver, log) + if err != nil { + return forgeReadWiring{}, err + } + linearTokens, err := buildLinearTokenSource(ctx, cfg, resolver, log) + if err != nil { + return forgeReadWiring{}, err + } + return forgeReadWiring{ + boardLane: boardLane, + notifyLane: notifyLane, + webhookSink: webhookSink, + webhookSecret: webhookSecret, + primaryClient: primaryClient, + linearTokens: linearTokens, + }, nil +} + // buildBoardWebhookWiring builds BOTH forge lanes (board ingestion + agent // notification) over ONE shared GitHub App client and derives the network // door's webhook ingress wiring: the lanes themselves (whose arms + reconcilers @@ -938,23 +997,23 @@ func buildBoardWebhookWiring( hub *runnerhub.Hub, resolver secrets.Resolver, log *slog.Logger, -) (*boardIngestLane, *forgeNotifyLane, ForgeEventSink, func(ctx context.Context) ([]byte, error), error) { +) (*boardIngestLane, *forgeNotifyLane, ForgeEventSink, func(ctx context.Context) ([]byte, error), *forge.GitHub, error) { rc := cfg.Forge.resolved() // App absent -> both forge lanes hard-off. Warn once here (the single // diagnostic site) when enabled subscription rows exist, and mount nothing. if !cfg.Forge.boardIngestionEnabled() { warnDisabledBoardIngestion(ctx, st, store.ForgeProviderGitHub, rc.Host, log) - return nil, nil, nil, nil, nil + return nil, nil, nil, nil, nil, nil } // Validate both App secrets ONCE at this shared site (distinct fail-fast // texts via validateForgeSecret) so a configured App with a missing secret // fails startup and BOTH lanes inherit a validated App — the notify lane no // longer relies on the board lane validating first. if err := validateForgeSecret(ctx, resolver, "board webhook app key", rc.App.AppPrivateKeySecret); err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, nil, err } if err := validateForgeSecret(ctx, resolver, "board webhook secret", rc.App.AppWebhookSecretName); err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, nil, err } // Build the ONE shared App token source + GitHub client both lanes ride. // appTokenSource is safe for concurrent use (mint singleflighted), so the @@ -966,18 +1025,22 @@ func buildBoardWebhookWiring( Host: rc.Host, }) if err != nil { - return nil, nil, nil, nil, fmt.Errorf("board webhook app token source: %w", err) + return nil, nil, nil, nil, nil, fmt.Errorf("board webhook app token source: %w", err) } client := forge.NewGitHub(forge.GitHubConfig{Host: rc.Host, Token: tok}) lane, err := buildBoardIngestLane(ctx, cfg, st, issueBrd, client, log) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, nil, err } notifyLane := buildForgeNotifyLane(cfg, st, hub, client, log) sink := &fanoutSink{sinks: []ForgeEventSink{lane.sink, notifyLane.sink}} secret := newCachedWebhookSecret(resolver, rc.App.AppWebhookSecretName) - return lane, notifyLane, sink, secret, nil + // client is returned as the shared primary App client the author write leg + // REUSES (RIG-2991 + the App cutover): the SAME *forge.GitHub object board + // reads, notify reads, AND author writes ride, so one client-side + // rate-budget/resetAt gate spans all three against the single installation. + return lane, notifyLane, sink, secret, client, nil } // buildLinearWebhookWiring builds the shared Linear POST /webhooks/linear @@ -1350,13 +1413,13 @@ func buildForgeNotifyLane( } // buildLinearNotifyLane assembles the Linear agent-notification lane (RIG-2732 -// T7), the Linear sibling of buildForgeNotifyLane. It gates App-INDEPENDENTLY on -// the LINEAR_FORGE_TOKEN read credential (forgeSecretDeclared) — the same secret -// the write path's Linear coordinate gates on (serve.go:1513-1517) — matching the -// house pattern that every forge lane gates as a unit on its own credential. An -// undeclared/absent secret returns (nil, nil), the off-state the caller reads as -// "mount no Linear notify sink"; a resolve FAULT returns the error (fail-fast, -// like forgeSecretDeclared elsewhere). +// T7), the Linear sibling of buildForgeNotifyLane. It is gated by the caller on +// the shared Linear OAuth client-credentials token source: a nil tokens means +// Linear is not configured, so it returns nil — the off-state the caller reads +// as "mount no Linear notify sink". A non-nil tokens builds the lane over a +// Linear client riding the SAME token-source instance the write coordinate rides +// (the one-instance rule, DEC-4), so it cannot fail (no resolve, no error +// return). // // Linear is issues-only and check-less (DL-051): its event alphabet is // Issue/Comment, so no CHECKS/REVIEW arms ever fire. The checks roller is still @@ -1365,24 +1428,19 @@ func buildForgeNotifyLane( // ChecksConditional returns ErrUnsupported, but the router invokes RollUp only on // a CHECKS event Linear never produces (correct and never-called). func buildLinearNotifyLane( - ctx context.Context, st *store.Store, hub *runnerhub.Hub, - resolver secrets.Resolver, + tokens *linearagent.TokenSource, log *slog.Logger, -) (*forgeNotifyLane, error) { - declared, err := forgeSecretDeclared(ctx, resolver, defaultForgeLinearSecretName) - if err != nil { - return nil, err - } - if !declared { - return nil, nil //nolint:nilnil // an undeclared LINEAR_FORGE_TOKEN is a valid off-state: a nil lane is the signal (the caller guards `if lane != nil`), not an ambiguous nil-nil — a sentinel error would force the caller to distinguish it from a real fault. +) *forgeNotifyLane { + if tokens == nil { + return nil // Linear not configured (client-credentials pair undeclared): lane off. } const ( provider = store.ForgeProviderLinear host = "linear.app" ) - client := forge.NewLinear(forge.LinearConfig{Token: newForgeTokenSource(resolver, defaultForgeLinearSecretName), Log: log}) + client := forge.NewLinear(forge.LinearConfig{Token: tokens, Log: log}) notifyStore := &forgeNotifyStore{st: st, provider: provider, host: host} dispatcher := &forgeNotifyDispatcher{hub: hub} @@ -1398,7 +1456,7 @@ func buildLinearNotifyLane( Backstop: 0, // no App config carries a Linear backstop; 0 -> ingest's defaultBackstop. Log: log, }) - return &forgeNotifyLane{arm: arm, reconciler: reconciler, sink: arm, reader: client}, nil + return &forgeNotifyLane{arm: arm, reconciler: reconciler, sink: arm, reader: client} } // newDeclaredSecretResolver returns a func that resolves the declared server_only @@ -1429,9 +1487,8 @@ func newDeclaredSecretResolver(resolver secrets.Resolver, name string) func(ctx // uncached resolve there lets an attacker force one full secretspec provider // Load (registry read + manifest temp-file write + provider Load) per cheap // garbage POST — an asymmetric-cost amplification ahead of authentication. The -// cache (same forgeTokenTTL and rotation semantics as forgeTokenSource) bounds -// the per-request cost to a memcmp while a rotated signing secret still takes -// effect within the TTL. A resolve fault is surfaced to the caller (a 503), +// cache (forgeTokenTTL) bounds the per-request cost to a memcmp while a rotated +// signing secret still takes effect within the TTL. A resolve fault is surfaced to the caller (a 503), // never cached. Unlike the App-key resolver (also newDeclaredSecretResolver but // cold — NewAppTokenSource caches the minted token and only reads the key on // mint), this one is on the request hot path, so it needs the cache. @@ -1491,16 +1548,20 @@ func validateForgeSecret(ctx context.Context, resolver secrets.Resolver, reason, return fmt.Errorf("forge secret %q not declared", name) } -// wireForgeWriteCaller resolves the declared secrets, and — when the forge-WRITE -// path is enabled (both write secrets declared) — builds the write chokepoint -// and mounts it on the hub via SetForgeCaller. A resolve FAULT (not an absent -// name) fails startup regardless of whether writes are on; when writes are off, -// the caller is left unwired and Hub.RelayForgeCall fail-closes to an in-band -// CodeUnavailable (relay_forge.go), the clean degrade — but a PARTIAL misconfig -// (only one of the two write secrets declared) logs a Warn first, since that is -// a likely operator typo rather than an intentional off. On any startup fault it -// unwinds the caller-bound listeners (udsListener + listeners) before returning, -// the same teardown path the poll driver and Rehydrate faults use. +// wireForgeWriteCaller builds the write chokepoint and mounts it on the hub via +// SetForgeCaller when the forge-WRITE path is enabled (BOTH the primary and +// reviewer Apps configured). A resolve FAULT (not an absent name) fails startup +// regardless of whether writes are on; when writes are off, the caller is left +// unwired and Hub.RelayForgeCall fail-closes to an in-band CodeUnavailable +// (relay_forge.go), the clean degrade — but a PARTIAL misconfig (exactly one App +// configured) logs a Warn first, since that is a likely operator typo rather than +// an intentional off. primaryClient is the shared primary App client the author +// write leg reuses (nil when the App is absent — which, since writes now require +// the primary App, only coincides with writes being off); linearTokens is the +// shared Linear token source the Linear coordinate rides (nil when Linear is not +// configured). On any startup fault it unwinds the caller-bound listeners +// (udsListener + listeners) before returning, the same teardown path the poll +// driver and Rehydrate faults use. func wireForgeWriteCaller( ctx context.Context, cfg ServeConfig, @@ -1509,86 +1570,104 @@ func wireForgeWriteCaller( resolver secrets.Resolver, hub *runnerhub.Hub, log *slog.Logger, + primaryClient *forge.GitHub, + linearTokens *linearagent.TokenSource, udsListener net.Listener, listeners boundListeners, ) error { declaredSecrets, err := resolver.Resolve(ctx, "forge write") if err != nil { - udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing startup path — nothing actionable remains (errcheck + its gosec G104 twin) - listeners.close() - return fmt.Errorf("forge secret resolve failed at startup: %w", err) + return failStartup(udsListener, listeners, fmt.Errorf("forge secret resolve failed at startup: %w", err)) } if !cfg.Forge.forgeWritesEnabled(declaredSecrets) { warnPartialForgeWriteSecrets(cfg.Forge, declaredSecrets, log) return nil } - forgeSvc, err := buildForgeWriteService(ctx, cfg, st, issueBrd, resolver, log) + forgeSvc, err := buildForgeWriteService(ctx, cfg, st, issueBrd, resolver, primaryClient, linearTokens, log) if err != nil { - udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing startup path — nothing actionable remains (errcheck + its gosec G104 twin) - listeners.close() - return err + return failStartup(udsListener, listeners, err) } hub.SetForgeCaller(forgeSvc) return nil } // buildForgeWriteService assembles the agent forge-WRITE chokepoint (the -// runnerhub.ForgeCaller) per Matt's 2026-08-19 ruling: independent of the board -// ingest lane, enabled iff BOTH forge write secrets are declared. In order it: (1) -// validates BOTH the author and reviewer secrets ONCE at startup so a -// misconfiguration fails fast with the two distinct texts (undeclared name vs a -// resolve that errors, via validateForgeSecret — the same fail-fast the board -// lane's App-secret validation uses), (2) builds the provider registry, registering the GitHub -// coordinate (author+reviewer, F1) and — when its secret is declared — a Linear -// coordinate, and (3) returns the forgeService the caller mounts with -// hub.SetForgeCaller. The caller gates the whole call on forgeWritesEnabled, so -// this only runs when both write secrets are present; the validateForgeSecret -// calls are the fail-fast on a resolve fault. +// runnerhub.ForgeCaller) for the 2-App cutover: enabled iff BOTH the primary and +// reviewer Apps are configured. In order it: (1) validates the reviewer App key +// secret ONCE at startup (the primary App key was already validated when the +// board wiring built primaryClient) via validateForgeSecret so a misconfig fails +// fast; (2) builds the reviewer App token source + client; (3) builds the +// provider registry, registering the GitHub coordinate (author = the shared +// primary App client, reviewer = the reviewer App client, F1) and — when the +// shared Linear token source is present — a Linear coordinate; and (4) returns +// the forgeService the caller mounts with hub.SetForgeCaller. // -// The registry always holds the GitHub coordinate (its host defaults to -// github.com, so registerGitHubForgeCoordinate registers it whenever writes are -// enabled) plus a Linear coordinate when LINEAR_FORGE_TOKEN is declared, so the -// returned service is always non-nil on success. +// The author write leg REUSES primaryClient (NOT a fresh client over the same +// token source): the client-side rate-budget/resetAt gate is per-*forge.GitHub +// client (mu-guarded), so threading only the token source into a new client would +// give the author writes a SEPARATE budget gate from the board/notify reads. +// Reusing the one client keeps author writes and reads on ONE shared budget gate +// against the single installation (RIG-2991). primaryClient is non-nil here: the +// caller only reaches this when writes are enabled, which requires the primary +// App configured, which is exactly when buildBoardWebhookWiring built the client. func buildForgeWriteService( ctx context.Context, cfg ServeConfig, st *store.Store, issueBrd *board.IssueProjection, resolver secrets.Resolver, + primaryClient *forge.GitHub, + linearTokens *linearagent.TokenSource, log *slog.Logger, ) (*forgeService, error) { fc := cfg.Forge.resolved() - // (1) Startup secret resolve for BOTH roles: fail fast with the distinct - // texts so a permanent misconfig (an undeclared name) is not confused with a - // transient outage (a resolve that errors). The TokenSources re-resolve later - // on TTL/Invalidate. - if err := validateForgeSecret(ctx, resolver, "forge write", fc.SecretName); err != nil { - return nil, err + // (1) The author write leg rides the shared primary App client. A defensive + // guard: the caller only reaches here when writes are enabled (primary App + // configured), so primaryClient is built — but a nil here would otherwise + // register a nil author client, so fail fast with a clear error rather than + // panic on first write. + if primaryClient == nil { + return nil, errors.New("forge write: primary App client is nil (writes enabled without a configured primary App)") } - if err := validateForgeSecret(ctx, resolver, "forge write", fc.ReviewerSecretName); err != nil { + + // (2) The reviewer App client: validate its key secret (distinct fail-fast + // text), then build its own installation-token source + client — a distinct + // GitHub identity from the primary App so F1's author-cannot-approve holds. + if err := validateForgeSecret(ctx, resolver, "forge reviewer app key", fc.ReviewerApp.AppPrivateKeySecret); err != nil { return nil, err } + reviewerTok, err := forge.NewAppTokenSource(forge.GitHubAppConfig{ + AppID: fc.ReviewerApp.AppID, + InstallationID: fc.ReviewerApp.InstallationID, + PrivateKey: newDeclaredSecretResolver(resolver, fc.ReviewerApp.AppPrivateKeySecret), + Host: fc.Host, + }) + if err != nil { + return nil, fmt.Errorf("forge reviewer app token source: %w", err) + } + reviewerClient := forge.NewGitHub(forge.GitHubConfig{Host: fc.Host, Token: reviewerTok}) - // (2) The provider registry: the GitHub coordinate (author+reviewer, F1) - // plus a Linear coordinate when its secret is declared. + // (3) The provider registry: the GitHub coordinate (author = shared primary + // client, reviewer = reviewer App client, F1) plus a Linear coordinate when + // the shared Linear token source is configured. registry := newForgeProviderRegistry() - registerGitHubForgeCoordinate(registry, fc, resolver) - - // Linear write coordinate — registered ONLY when LINEAR_FORGE_TOKEN is - // declared (else GitHub-only). Linear is issues-only (DL-051): its PR/review - // ops return ErrUnsupported, which the chokepoint flattens to in-band - // unimplemented. One client serves both roles — Linear has no author/reviewer - // split (no review concept), so the same client is the author and the reviewer - // entry. Its coordinate host is left empty so a Linear-provider ForgeRef with - // no host resolves it via the registry's per-provider default; the GraphQL - // endpoint default lives inside NewLinear. isDefault=false: the GitHub - // coordinate is the default a nil/unset ForgeRef resolves to, so Linear is the - // additive coordinate a LINEAR-addressed ForgeRef selects explicitly. - if linearDeclared, err := forgeSecretDeclared(ctx, resolver, defaultForgeLinearSecretName); err != nil { - return nil, err - } else if linearDeclared { - linear := forge.NewLinear(forge.LinearConfig{Token: newForgeTokenSource(resolver, defaultForgeLinearSecretName), Log: log}) + registerGitHubForgeCoordinate(registry, fc, primaryClient, reviewerClient) + + // Linear write coordinate — registered ONLY when Linear is configured (the + // shared client-credentials token source is non-nil, else GitHub-only). Linear + // is issues-only (DL-051): its PR/review ops return ErrUnsupported, which the + // chokepoint flattens to in-band unimplemented. One client serves both roles — + // Linear has no author/reviewer split (no review concept), so the same client + // is the author and the reviewer entry. It rides the SAME linearTokens + // instance the notify lane rides (the one-instance rule, DEC-4). Its + // coordinate host is left empty so a Linear-provider ForgeRef with no host + // resolves it via the registry's per-provider default; the GraphQL endpoint + // default lives inside NewLinear. isDefault=false: the GitHub coordinate is the + // default a nil/unset ForgeRef resolves to, so Linear is the additive + // coordinate a LINEAR-addressed ForgeRef selects explicitly. + if linearTokens != nil { + linear := forge.NewLinear(forge.LinearConfig{Token: linearTokens, Log: log}) registry.register(forgeCoordinate{provider: compassv1.ForgeProvider_FORGE_PROVIDER_LINEAR}, linear, linear, false) } @@ -1596,18 +1675,64 @@ func buildForgeWriteService( } // registerGitHubForgeCoordinate registers the production GitHub write coordinate -// — the AUTHOR client (over fc.SecretName) and the REVIEWER client (over -// fc.ReviewerSecretName, F1), each on its own TTL-caching TokenSource — as the -// default coordinate a nil/unset ForgeRef resolves to. The two roles are -// distinct GitHub identities so an agent approving a PR it authored dispatches -// submit_review on a different account than it authored with, dissolving the -// author-approving-own-PR rejection at the credential layer. -func registerGitHubForgeCoordinate(reg *forgeProviderRegistry, fc ForgeConfig, resolver secrets.Resolver) { - author := forge.NewGitHub(forge.GitHubConfig{Host: fc.Host, Token: newForgeTokenSource(resolver, fc.SecretName)}) - reviewer := forge.NewGitHub(forge.GitHubConfig{Host: fc.Host, Token: newForgeTokenSource(resolver, fc.ReviewerSecretName)}) +// — the AUTHOR client (the shared primary App client) and the REVIEWER client +// (the reviewer App client, F1) — as the default coordinate a nil/unset ForgeRef +// resolves to. The two roles are distinct GitHub App identities so an agent +// approving a PR it authored dispatches submit_review on a different account than +// it authored with, dissolving the author-approving-own-PR rejection at the +// credential layer. +func registerGitHubForgeCoordinate(reg *forgeProviderRegistry, fc ForgeConfig, author, reviewer *forge.GitHub) { reg.register(forgeCoordinate{provider: compassv1.ForgeProvider_FORGE_PROVIDER_GITHUB, host: fc.Host}, author, reviewer, true) } +// buildLinearTokenSource builds the ONE shared Linear OAuth client-credentials +// token source (actor=app) from the declared client-id/secret pair, or returns +// nil when Linear is not configured (neither name declared — the clean off-state +// for a deployment that runs no Linear lane). Exactly ONE of the two names +// declared is a likely operator typo: it Warns and treats Linear as off (mirrors +// warnPartialForgeWriteSecrets). When BOTH are declared it builds the source and +// runs a boot-time mint check — one Token(ctx) call — so a bad pair or a disabled +// client_credentials toggle fails Serve at startup (fail-fast like +// validateForgeSecret), not on the first write. The returned instance is passed +// to BOTH the notify lane and the write coordinate (the one-instance rule, DEC-4). +func buildLinearTokenSource(ctx context.Context, cfg ServeConfig, resolver secrets.Resolver, log *slog.Logger) (*linearagent.TokenSource, error) { + fc := cfg.Forge.resolved() + declared, err := resolver.Resolve(ctx, "forge linear") + if err != nil { + return nil, fmt.Errorf("forge secret resolve failed at startup: %w", err) + } + var clientID, clientSecret string + for _, s := range declared { + switch s.Name { + case fc.LinearClientIDSecretName: + clientID = s.Value + case fc.LinearClientSecretName: + clientSecret = s.Value + } + } + haveID, haveSecret := clientID != "", clientSecret != "" + if !haveID && !haveSecret { + return nil, nil //nolint:nilnil // Linear not configured is a valid off-state: a nil source is the signal (the callers guard `if tokens != nil`), not an ambiguous nil-nil. + } + if haveID != haveSecret { + declaredName, missingName := fc.LinearClientIDSecretName, fc.LinearClientSecretName + if haveSecret { + declaredName, missingName = fc.LinearClientSecretName, fc.LinearClientIDSecretName + } + log.Warn("forge Linear lanes disabled: only one of the two Linear client-credential secrets is declared; both are required", + "declared", declaredName, "missing", missingName) + return nil, nil //nolint:nilnil // a partial Linear config is an operator typo, surfaced by the Warn; treated as off (a nil source), never a fatal. + } + tokens := linearagent.NewTokenSource(clientID, clientSecret, nil, "") + // Boot-time mint check: a Token(ctx) call proves the pair mints (the DL-204 + // degrade probe guards only attribution, not the mint path), so a bad secret + // or a disabled client_credentials toggle fails Serve here, not on first write. + if _, err := tokens.Token(ctx); err != nil { + return nil, fmt.Errorf("forge Linear boot-time mint check failed (verify the client-credentials pair and the app's client-credentials toggle): %w", err) + } + return tokens, nil +} + // forgeSecretDeclared reports whether name is present in the resolved // declared-secret set — the additive Linear gate (register a Linear coordinate // iff its secret is declared). A resolve fault fails fast the same way @@ -1684,24 +1809,24 @@ func (f *fanoutSink) Enqueue(ctx context.Context, ev forge.ForgeEvent) { } // warnPartialForgeWriteSecrets emits exactly one slog.Warn when the forge-WRITE -// path is disabled because only ONE of the two required write secrets is -// declared — a likely operator typo in one of the two env-var NAMES, which -// otherwise silently fails every agent forge write closed (CodeUnavailable) -// with nothing in the startup log to explain it. The intentional both-absent -// OFF state stays silent. Mirrors warnDisabledBoardIngestion: diagnostic only, -// never fail-fast, and logs secret NAMES (env-var identifiers) never values. +// path is disabled because exactly ONE of the two required Apps (the primary and +// the reviewer) is configured — a likely operator typo (a missing App id or an +// undeclared App key secret), which otherwise silently fails every agent forge +// write closed (CodeUnavailable) with nothing in the startup log to explain it. +// The intentional both-absent OFF state stays silent. Mirrors +// warnDisabledBoardIngestion: diagnostic only, never fail-fast, and logs a role +// name never a secret value. func warnPartialForgeWriteSecrets(fc ForgeConfig, declared []secrets.ResolvedSecret, log *slog.Logger) { - haveAuthor, haveReviewer := fc.forgeWriteSecretsDeclared(declared) - if haveAuthor == haveReviewer { - return // both present (enabled path, not here) or both absent (intentional off) + havePrimary, haveReviewer := fc.forgeWriteAppsConfigured(declared) + if havePrimary == haveReviewer { + return // both configured (enabled path, not here) or both absent (intentional off) } - rc := fc.resolved() - declaredName, missingName := rc.SecretName, rc.ReviewerSecretName + configured, missing := "primary App", "reviewer App" if haveReviewer { - declaredName, missingName = rc.ReviewerSecretName, rc.SecretName + configured, missing = "reviewer App", "primary App" } - log.Warn("forge write path disabled: only one of the two required forge write secrets is declared; both are required", - "declared", declaredName, "missing", missingName) + log.Warn("forge write path disabled: only one of the two required GitHub Apps is configured; both are required", + "configured", configured, "missing", missing) } // normalizeGitHubRepo validates an "owner/name" repo string and lowercases it @@ -1717,71 +1842,14 @@ func normalizeGitHubRepo(raw string) (string, error) { return strings.ToLower(trimmed), nil } -// forgeTokenSource is the driver's forge.TokenSource: a TTL cache over the one -// SpecResolver, selecting the configured secret name from the resolved set. A -// resolve is not cheap (reads the whole registry, writes a manifest, drives a -// provider Load — resolver.go:135-165), so the value is cached for forgeTokenTTL -// and re-resolved only on TTL expiry or Invalidate() (the client calls the -// latter on a 401/bad-creds-403). This is the design's stated reason for a -// TTL-cache with an invalidation seam rather than a captured token or bare func. -type forgeTokenSource struct { - resolver secrets.Resolver - name string - ttl time.Duration - now func() time.Time - - mu sync.Mutex - token string - expires time.Time - valid bool -} - -// newForgeTokenSource returns a TokenSource resolving name through resolver, -// caching each resolved value for forgeTokenTTL. -func newForgeTokenSource(resolver secrets.Resolver, name string) *forgeTokenSource { - return &forgeTokenSource{resolver: resolver, name: name, ttl: forgeTokenTTL, now: time.Now} -} - -// Token returns the cached token while it is valid and unexpired, else -// re-resolves the declared set and selects the configured name. A missing name -// at Token time (declaration deleted post-boot) is an error the driver surfaces -// per-pass as an auth failure + retry next tick (idempotent). -// -// Single-caller by the driver's contract: the poll driver calls Token -// sequentially per fetch batch on one goroutine, and Invalidate runs on that -// same goroutine (never re-entrantly from inside Token), so holding t.mu across -// the resolve I/O never contends. Were a second concurrent caller ever added, -// the lock would simply serialize resolves (singleflight-like) — benign, but the -// single-caller assumption is the reason the resolve is inside the lock. -func (t *forgeTokenSource) Token(ctx context.Context) (string, error) { - t.mu.Lock() - defer t.mu.Unlock() - if t.valid && t.now().Before(t.expires) { - return t.token, nil - } - resolved, err := t.resolver.Resolve(ctx, "forge poll") - if err != nil { - return "", fmt.Errorf("forge token resolve: %w", err) - } - for _, s := range resolved { - if s.Name == t.name { - t.token = s.Value - t.expires = t.now().Add(t.ttl) - t.valid = true - return t.token, nil - } - } - t.valid = false - return "", fmt.Errorf("forge secret %q not declared", t.name) -} - -// Invalidate drops the cached value so the next Token re-resolves — the client -// calls it when it observes an auth failure, so a rotated token takes effect -// immediately rather than after the TTL. -func (t *forgeTokenSource) Invalidate() { - t.mu.Lock() - defer t.mu.Unlock() - t.valid = false +// failStartup tears down the UDS socket + eager-bound listeners on any post-bind +// startup fault, then returns err unchanged — the one cleanup path every Serve +// fail-fast (and wireForgeWriteCaller) shares, so a new fault site is one call, +// not a repeated three-line teardown. +func failStartup(udsListener net.Listener, listeners boundListeners, err error) error { + udsListener.Close() //nolint:errcheck,gosec // teardown on an already-failing startup path — nothing actionable remains (errcheck + its gosec G104 twin) + listeners.close() + return err } func closeListener(l net.Listener) { diff --git a/go/server/serve_forge_budget_test.go b/go/server/serve_forge_budget_test.go index 3c5f6fb1c..12322f77d 100644 --- a/go/server/serve_forge_budget_test.go +++ b/go/server/serve_forge_budget_test.go @@ -150,4 +150,33 @@ func TestForgeLanesShareOneBudgetGate(t *testing.T) { t.Fatalf("notify lane's reader issued a request through an armed shared gate: transport calls = %d, want 1 "+ "(two independent gates would let it through as call 2)", rt.calls) } + + // (4) The AUTHOR WRITE leg rides the SAME shared primary client (the App + // cutover reuses buildBoardWebhookWiring's client as the author write client, + // NOT a fresh client over its token source — a fresh client would carry a + // SEPARATE resetAt gate). registerGitHubForgeCoordinate wires that exact + // client as the author role, so an author write now fast-fails on the armed + // gate and issues NO request. A regression that threaded only the token source + // into a new author client would let this write through as call 2. + reg := newForgeProviderRegistry() + reviewerClient := forge.NewGitHub(forge.GitHubConfig{Host: host, Token: staticTokenSource{}}) + registerGitHubForgeCoordinate(reg, cfg.Forge.resolved(), client, reviewerClient) + resolved, ok := reg.resolve(nil) + if !ok { + t.Fatal("registry did not resolve the default GitHub coordinate") + } + if resolved.author != forge.Provider(client) { + t.Fatal("author role is not the shared primary client (a fresh client would carry a separate budget gate)") + } + _, err = resolved.author.CommentOnIssue(ctx, "owner/repo", 1, "body") + if err == nil { + t.Fatal("author write after arming: err = nil, want ErrBudgetExhausted (the shared gate is armed)") + } + if !errors.Is(err, forge.ErrBudgetExhausted) { + t.Fatalf("author write err = %v, want ErrBudgetExhausted", err) + } + if rt.calls != 1 { + t.Fatalf("author write issued a request through an armed shared gate: transport calls = %d, want 1 "+ + "(a separate author-client gate would let it through as call 2)", rt.calls) + } } diff --git a/go/server/serve_forge_pgtest_test.go b/go/server/serve_forge_pgtest_test.go index de9776028..9a860a65f 100644 --- a/go/server/serve_forge_pgtest_test.go +++ b/go/server/serve_forge_pgtest_test.go @@ -225,20 +225,20 @@ func TestBoardIngestionDisabledWarnsOnEnabledRows(t *testing.T) { const host = forgeTestHost cfg := ServeConfig{Forge: ForgeConfig{Host: host}} - assertAllNil := func(t *testing.T, lane *boardIngestLane, notify *forgeNotifyLane, sink ForgeEventSink, secret func(context.Context) ([]byte, error), err error) { + assertAllNil := func(t *testing.T, lane *boardIngestLane, notify *forgeNotifyLane, sink ForgeEventSink, secret func(context.Context) ([]byte, error), client *forge.GitHub, err error) { t.Helper() if err != nil { t.Fatalf("buildBoardWebhookWiring (App absent): %v", err) } - if lane != nil || notify != nil || sink != nil || secret != nil { - t.Fatalf("wiring not all-nil with no App configured: lane==nil? %t notify==nil? %t sink==nil? %t secret==nil? %t", lane == nil, notify == nil, sink == nil, secret == nil) + if lane != nil || notify != nil || sink != nil || secret != nil || client != nil { + t.Fatalf("wiring not all-nil with no App configured: lane==nil? %t notify==nil? %t sink==nil? %t secret==nil? %t client==nil? %t", lane == nil, notify == nil, sink == nil, secret == nil, client == nil) } } t.Run("no Warn when no enabled rows exist", func(t *testing.T) { h := &capHandler{} - lane, notify, sink, secret, err := buildBoardWebhookWiring(ctx, cfg, st, nil, nil, &fakeResolver{}, slog.New(h)) - assertAllNil(t, lane, notify, sink, secret, err) + lane, notify, sink, secret, client, err := buildBoardWebhookWiring(ctx, cfg, st, nil, nil, &fakeResolver{}, slog.New(h)) + assertAllNil(t, lane, notify, sink, secret, client, err) if n := warnCount(h.recs); n != 0 { t.Fatalf("Warn count with no enabled rows = %d, want 0", n) } @@ -252,8 +252,8 @@ func TestBoardIngestionDisabledWarnsOnEnabledRows(t *testing.T) { t.Run("exactly one Warn when enabled rows exist for the bound coordinate", func(t *testing.T) { h := &capHandler{} - lane, notify, sink, secret, err := buildBoardWebhookWiring(ctx, cfg, st, nil, nil, &fakeResolver{}, slog.New(h)) - assertAllNil(t, lane, notify, sink, secret, err) + lane, notify, sink, secret, client, err := buildBoardWebhookWiring(ctx, cfg, st, nil, nil, &fakeResolver{}, slog.New(h)) + assertAllNil(t, lane, notify, sink, secret, client, err) if n := warnCount(h.recs); n != 1 { t.Fatalf("Warn count with an enabled row = %d, want exactly 1", n) } @@ -262,8 +262,8 @@ func TestBoardIngestionDisabledWarnsOnEnabledRows(t *testing.T) { t.Run("no Warn for a different bound host (abandoned rows give no false comfort)", func(t *testing.T) { h := &capHandler{} otherCfg := ServeConfig{Forge: ForgeConfig{Host: "other.example.com"}} - lane, notify, sink, secret, err := buildBoardWebhookWiring(ctx, otherCfg, st, nil, nil, &fakeResolver{}, slog.New(h)) - assertAllNil(t, lane, notify, sink, secret, err) + lane, notify, sink, secret, client, err := buildBoardWebhookWiring(ctx, otherCfg, st, nil, nil, &fakeResolver{}, slog.New(h)) + assertAllNil(t, lane, notify, sink, secret, client, err) if n := warnCount(h.recs); n != 0 { t.Fatalf("Warn count for a different host = %d, want 0 (count is bound-coordinate only)", n) } @@ -296,7 +296,7 @@ func TestBoardIngestLaneFailsFastOnMissingAppSecret(t *testing.T) { // Neither App secret declared -> the FIRST validateForgeSecret (app key) fails. t.Run("both undeclared fails on the app key", func(t *testing.T) { res := &fakeResolver{resolved: nil} - _, _, _, _, err := buildBoardWebhookWiring(ctx, cfg, st, nil, nil, res, slog.Default()) + _, _, _, _, _, err := buildBoardWebhookWiring(ctx, cfg, st, nil, nil, res, slog.Default()) if err == nil { t.Fatal("buildBoardWebhookWiring with no App secrets = nil, want a startup error") } @@ -309,7 +309,7 @@ func TestBoardIngestLaneFailsFastOnMissingAppSecret(t *testing.T) { // validateForgeSecret must fail (both secrets required, checked separately). t.Run("app key present but webhook secret undeclared fails on the webhook secret", func(t *testing.T) { res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "APP_KEY", Value: "pem"}}} - _, _, _, _, err := buildBoardWebhookWiring(ctx, cfg, st, nil, nil, res, slog.Default()) + _, _, _, _, _, err := buildBoardWebhookWiring(ctx, cfg, st, nil, nil, res, slog.Default()) if err == nil { t.Fatal("buildBoardWebhookWiring with the webhook secret undeclared = nil, want a startup error") } diff --git a/go/server/serve_forge_test.go b/go/server/serve_forge_test.go index 2c2ad77ee..b96179e22 100644 --- a/go/server/serve_forge_test.go +++ b/go/server/serve_forge_test.go @@ -21,6 +21,7 @@ import ( "testing" "time" + "github.com/RigelBuild/compass/go/internal/linearagent" "github.com/RigelBuild/compass/go/internal/secrets" ) @@ -65,205 +66,182 @@ func TestForgeConfigEnableAndDefaults(t *testing.T) { if got.Host != defaultForgeHost { t.Fatalf("Host = %q, want %q", got.Host, defaultForgeHost) } - if got.SecretName != defaultForgeSecretName { - t.Fatalf("SecretName = %q, want %q", got.SecretName, defaultForgeSecretName) + if got.LinearClientIDSecretName != defaultForgeLinearClientIDSecretName { + t.Fatalf("LinearClientIDSecretName = %q, want %q", got.LinearClientIDSecretName, defaultForgeLinearClientIDSecretName) + } + if got.LinearClientSecretName != defaultForgeLinearClientSecretName { + t.Fatalf("LinearClientSecretName = %q, want %q", got.LinearClientSecretName, defaultForgeLinearClientSecretName) } if got.App.ReconcileBackstop != defaultReconcileBackstop { t.Fatalf("ReconcileBackstop = %v, want %v", got.App.ReconcileBackstop, defaultReconcileBackstop) } }) t.Run("explicit fields survive defaulting", func(t *testing.T) { - in := ForgeConfig{Host: "ghe.example.com", SecretName: "TOK", App: ForgeAppConfig{ReconcileBackstop: 3 * time.Minute}} + in := ForgeConfig{Host: "ghe.example.com", LinearClientIDSecretName: "LID", App: ForgeAppConfig{ReconcileBackstop: 3 * time.Minute}} got := in.resolved() - if got.Host != "ghe.example.com" || got.SecretName != "TOK" || got.App.ReconcileBackstop != 3*time.Minute { + if got.Host != "ghe.example.com" || got.LinearClientIDSecretName != "LID" || got.App.ReconcileBackstop != 3*time.Minute { t.Fatalf("explicit fields clobbered by defaulting: %+v", got) } }) } -// TestForgeReviewerSecretDefaultingAndWritesEnabled pins the T8 write-path -// enablement contract (Matt's 2026-08-19 ruling): the reviewer secret name -// defaults to defaultForgeReviewerSecretName, an explicit one survives, and the -// write path is enabled iff BOTH the author and reviewer secrets are declared — -// independent of the board lane's boardIngestionEnabled gate. -func TestForgeReviewerSecretDefaultingAndWritesEnabled(t *testing.T) { - t.Run("reviewer secret defaulted to the F1 default name", func(t *testing.T) { - if got := (ForgeConfig{}).resolved().ReviewerSecretName; got != defaultForgeReviewerSecretName { - t.Fatalf("ReviewerSecretName = %q, want %q", got, defaultForgeReviewerSecretName) - } - }) - t.Run("explicit reviewer secret survives defaulting", func(t *testing.T) { - if got := (ForgeConfig{ReviewerSecretName: "REV_TOK"}).resolved().ReviewerSecretName; got != "REV_TOK" { - t.Fatalf("ReviewerSecretName = %q, want the explicit REV_TOK", got) +// TestForgeWriteAppsGate pins the 2-App write-path enablement contract +// (DEC-1/DEC-3): the forge-WRITE path is enabled iff BOTH the primary App and +// the reviewer App are configured — each AppID != 0 AND its private-key secret +// declared. Requiring the primary App force-couples writes to board ingestion +// (both key on App.AppID) — the unified shape Matt wants. +func TestForgeWriteAppsGate(t *testing.T) { + // bothApps is a config with the primary + reviewer Apps configured; declared + // carries both App key secrets. + bothApps := ForgeConfig{ + App: ForgeAppConfig{AppID: 1, InstallationID: 2, AppPrivateKeySecret: "PRIMARY_KEY", AppWebhookSecretName: "WH"}, + ReviewerApp: ForgeAppConfig{AppID: 3, InstallationID: 4, AppPrivateKeySecret: "REVIEWER_KEY"}, + } + bothDeclared := []secrets.ResolvedSecret{{Name: "PRIMARY_KEY"}, {Name: "REVIEWER_KEY"}} + + t.Run("both Apps configured + both keys declared -> writes enabled", func(t *testing.T) { + if !bothApps.forgeWritesEnabled(bothDeclared) { + t.Fatal("both Apps configured with both key secrets declared should enable the write path") } }) - t.Run("both defaulted secrets declared -> writes enabled", func(t *testing.T) { - declared := []secrets.ResolvedSecret{ - {Name: defaultForgeSecretName}, {Name: defaultForgeReviewerSecretName}, - } - if !(ForgeConfig{}).forgeWritesEnabled(declared) { - t.Fatal("both secrets declared should enable the write path") + t.Run("primary App only -> writes disabled", func(t *testing.T) { + cfg := ForgeConfig{App: bothApps.App} + if cfg.forgeWritesEnabled(bothDeclared) { + t.Fatal("primary-App-only should NOT enable the write path (both Apps required)") } }) - t.Run("only the author secret declared -> writes disabled", func(t *testing.T) { - declared := []secrets.ResolvedSecret{{Name: defaultForgeSecretName}} - if (ForgeConfig{}).forgeWritesEnabled(declared) { - t.Fatal("author-only should NOT enable the write path (both required)") + t.Run("reviewer App only -> writes disabled", func(t *testing.T) { + cfg := ForgeConfig{ReviewerApp: bothApps.ReviewerApp} + if cfg.forgeWritesEnabled(bothDeclared) { + t.Fatal("reviewer-App-only should NOT enable the write path (both Apps required)") } }) - t.Run("only the reviewer secret declared -> writes disabled", func(t *testing.T) { - declared := []secrets.ResolvedSecret{{Name: defaultForgeReviewerSecretName}} - if (ForgeConfig{}).forgeWritesEnabled(declared) { - t.Fatal("reviewer-only should NOT enable the write path (both required)") + t.Run("both App ids set but a key secret undeclared -> writes disabled", func(t *testing.T) { + // The reviewer key is missing from the declared set: configured means + // AppID != 0 AND key declared, so this is a partial (disabled) state. + onlyPrimaryKey := []secrets.ResolvedSecret{{Name: "PRIMARY_KEY"}} + if bothApps.forgeWritesEnabled(onlyPrimaryKey) { + t.Fatal("a configured reviewer App with its key undeclared must NOT enable writes") } }) - t.Run("neither declared -> writes disabled", func(t *testing.T) { + t.Run("neither App configured -> writes disabled", func(t *testing.T) { if (ForgeConfig{}).forgeWritesEnabled(nil) { - t.Fatal("no declared secrets should leave the write path disabled") - } - }) - t.Run("enablement honours explicit secret names, not just defaults", func(t *testing.T) { - cfg := ForgeConfig{SecretName: "AUTHOR_TOK", ReviewerSecretName: "REVIEWER_TOK"} - both := []secrets.ResolvedSecret{{Name: "AUTHOR_TOK"}, {Name: "REVIEWER_TOK"}} - if !cfg.forgeWritesEnabled(both) { - t.Fatal("explicit names both declared should enable the write path") - } - // The DEFAULT names being present must NOT enable a config that named - // custom secrets — the predicate keys on the resolved config's names. - defaults := []secrets.ResolvedSecret{{Name: defaultForgeSecretName}, {Name: defaultForgeReviewerSecretName}} - if cfg.forgeWritesEnabled(defaults) { - t.Fatal("default names must not satisfy a config that declared custom secret names") + t.Fatal("no Apps configured should leave the write path disabled") } }) - t.Run("write enablement is independent of the board ingestion gate", func(t *testing.T) { - // boardIngestionEnabled is false (no App) yet writes are enabled on both - // secrets — the two gates are orthogonal (Matt's ruling). - cfg := ForgeConfig{} - if cfg.boardIngestionEnabled() { - t.Fatal("fixture precondition: board ingestion should be disabled") + t.Run("enabling writes force-enables board ingestion (unified shape)", func(t *testing.T) { + // Both Apps configured -> writes enabled AND boardIngestionEnabled true + // (both key on App.AppID). The 2026-08-19 independent-gates ruling is + // amended (DL-305): writes now require the primary App. + if !bothApps.forgeWritesEnabled(bothDeclared) { + t.Fatal("fixture precondition: writes should be enabled") } - declared := []secrets.ResolvedSecret{{Name: defaultForgeSecretName}, {Name: defaultForgeReviewerSecretName}} - if !cfg.forgeWritesEnabled(declared) { - t.Fatal("writes must enable on both secrets even with board ingestion disabled") + if !bothApps.boardIngestionEnabled() { + t.Fatal("enabling writes must force board ingestion on (primary App configured)") } }) } -// TestBuildLinearNotifyLaneGate pins the RIG-2732 T7 Linear notify lane's -// App-INDEPENDENT gate: buildLinearNotifyLane runs iff LINEAR_FORGE_TOKEN is -// declared (the read credential the reconciler needs), NOT the GitHub App gate. -// The gate short-circuits before any store/hub touch, so a nil store + nil hub -// suffice; the declared path binds the Linear coordinate -// (store.ForgeProviderLinear / "linear.app"). A resolve fault fails fast. +// TestBuildLinearNotifyLaneGate pins the Linear notify lane's gate: it is built +// iff the caller passes a non-nil shared Linear token source (Linear configured); +// a nil source is the off-state (lane nil). The gate short-circuits before any +// store/hub touch, so a nil store + nil hub suffice; the built lane binds the +// Linear coordinate (store.ForgeProviderLinear / "linear.app"). func TestBuildLinearNotifyLaneGate(t *testing.T) { - ctx := context.Background() // test root - t.Run("undeclared LINEAR_FORGE_TOKEN -> nil lane (off-state)", func(t *testing.T) { - lane, err := buildLinearNotifyLane(ctx, nil, nil, &fakeResolver{}, nil) - if err != nil { - t.Fatalf("buildLinearNotifyLane (undeclared): %v", err) - } - if lane != nil { - t.Fatal("lane != nil with LINEAR_FORGE_TOKEN undeclared, want nil (lane off)") + t.Run("nil token source -> nil lane (off-state)", func(t *testing.T) { + if lane := buildLinearNotifyLane(nil, nil, nil, nil); lane != nil { + t.Fatal("lane != nil with a nil Linear token source, want nil (lane off)") } }) - t.Run("declared LINEAR_FORGE_TOKEN -> non-nil lane with a sink", func(t *testing.T) { - res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: defaultForgeLinearSecretName, Value: "lin-tok"}}} - lane, err := buildLinearNotifyLane(ctx, nil, nil, res, nil) - if err != nil { - t.Fatalf("buildLinearNotifyLane (declared): %v", err) - } + t.Run("non-nil token source -> non-nil lane with a sink", func(t *testing.T) { + tokens := linearagent.NewTokenSource("cid", "csecret", nil, "") + lane := buildLinearNotifyLane(nil, nil, tokens, nil) if lane == nil { - t.Fatal("lane == nil with LINEAR_FORGE_TOKEN declared, want a non-nil lane") + t.Fatal("lane == nil with a configured Linear token source, want a non-nil lane") } if lane.arm == nil || lane.reconciler == nil || lane.sink == nil { t.Fatalf("assembled lane has a nil member: %+v", lane) } }) - t.Run("resolve fault -> error (fail-fast)", func(t *testing.T) { - res := &fakeResolver{err: errors.New("boom")} - lane, err := buildLinearNotifyLane(ctx, nil, nil, res, nil) - if err == nil { - t.Fatal("buildLinearNotifyLane returned nil error on a resolve fault, want fail-fast") - } - if lane != nil { - t.Fatalf("lane = %+v on a resolve fault, want nil", lane) - } - }) } -func TestForgeTokenSourceCachesUntilTTL(t *testing.T) { - res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "GITHUB_FORGE_TOKEN", Value: "tok-1"}}} - ts := newForgeTokenSource(res, "GITHUB_FORGE_TOKEN") - - // A fixed clock the test advances by hand — no real sleeps. - now := time.Unix(0, 0) - ts.now = func() time.Time { return now } - ts.ttl = time.Minute - +// TestBuildLinearTokenSourceGate pins buildLinearTokenSource's pre-mint gating — +// the branches that decide WHETHER a Linear token source is built, before any +// network mint (the mint itself is covered by linearagent's TokenSource tests). +// Neither client-cred secret declared is the clean off-state (nil, nil); a +// resolve fault fails fast; exactly one of the pair declared is a likely +// operator typo, surfaced by ONE Warn naming the declared + missing secret and +// treated as off (nil, nil) — never a fatal. +func TestBuildLinearTokenSourceGate(t *testing.T) { ctx := context.Background() // test root - tok, err := ts.Token(ctx) - if err != nil || tok != "tok-1" { - t.Fatalf("first Token = %q, %v; want tok-1, nil", tok, err) - } - if res.calls != 1 { - t.Fatalf("resolve calls = %d after first Token, want 1", res.calls) - } - - // Rotate the resolver's value; within the TTL the cache still serves the OLD - // value and does NOT re-resolve — the whole point of the TTL cache. - res.resolved[0].Value = "tok-2" - now = now.Add(30 * time.Second) - tok, err = ts.Token(ctx) - if err != nil || tok != "tok-1" { - t.Fatalf("within-TTL Token = %q, %v; want cached tok-1, nil", tok, err) - } - if res.calls != 1 { - t.Fatalf("resolve calls = %d within TTL, want still 1 (cache hit)", res.calls) - } + cfg := ServeConfig{} // Forge zero -> resolved() defaults the two client-cred names. + idName, secretName := defaultForgeLinearClientIDSecretName, defaultForgeLinearClientSecretName - // Cross the TTL: the next Token re-resolves and the rotated value takes over. - now = now.Add(time.Minute) - tok, err = ts.Token(ctx) - if err != nil || tok != "tok-2" { - t.Fatalf("post-TTL Token = %q, %v; want re-resolved tok-2, nil", tok, err) - } - if res.calls != 2 { - t.Fatalf("resolve calls = %d post-TTL, want 2 (re-resolve)", res.calls) - } -} - -func TestForgeTokenSourceInvalidateDropsCache(t *testing.T) { - res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "GITHUB_FORGE_TOKEN", Value: "tok-1"}}} - ts := newForgeTokenSource(res, "GITHUB_FORGE_TOKEN") - now := time.Unix(0, 0) - ts.now = func() time.Time { return now } - ts.ttl = time.Hour // long TTL, so only Invalidate can force a re-resolve + t.Run("neither secret declared -> nil source (off-state), no Warn", func(t *testing.T) { + h := &capWarnHandler{} + tokens, err := buildLinearTokenSource(ctx, cfg, &fakeResolver{}, slog.New(h)) + if err != nil { + t.Fatalf("buildLinearTokenSource (neither declared): %v", err) + } + if tokens != nil { + t.Fatal("token source != nil with neither Linear secret declared, want nil (off)") + } + if h.warns != 0 { + t.Fatalf("Warn count = %d, want 0 for the intentional both-absent off state", h.warns) + } + }) - ctx := context.Background() // test root - if tok, err := ts.Token(ctx); err != nil || tok != "tok-1" { - t.Fatalf("first Token = %q, %v; want tok-1, nil", tok, err) - } + t.Run("resolve fault -> error (fail-fast)", func(t *testing.T) { + tokens, err := buildLinearTokenSource(ctx, cfg, &fakeResolver{err: errors.New("boom")}, slog.Default()) + if err == nil { + t.Fatal("buildLinearTokenSource on a resolve fault = nil error, want fail-fast") + } + if tokens != nil { + t.Fatalf("token source = %v on a resolve fault, want nil", tokens) + } + }) - // A rotation the driver learns about via an auth failure (client calls - // Invalidate). Within the TTL, only Invalidate drops the cache so the next - // Token re-resolves and picks up the changed value. - res.resolved[0].Value = "tok-2" - ts.Invalidate() - tok, err := ts.Token(ctx) - if err != nil || tok != "tok-2" { - t.Fatalf("post-Invalidate Token = %q, %v; want re-resolved tok-2, nil", tok, err) - } - if res.calls != 2 { - t.Fatalf("resolve calls = %d, want 2 (initial + post-Invalidate)", res.calls) - } -} + t.Run("only the client id declared -> nil source + one Warn naming declared+missing", func(t *testing.T) { + h := &capWarnHandler{} + res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: idName, Value: "cid"}}} + tokens, err := buildLinearTokenSource(ctx, cfg, res, slog.New(h)) + if err != nil { + t.Fatalf("buildLinearTokenSource (partial): %v", err) + } + if tokens != nil { + t.Fatal("token source != nil with only the client id declared, want nil (partial -> off)") + } + if h.warns != 1 { + t.Fatalf("Warn count = %d, want exactly 1 on a partial (id-only) misconfig", h.warns) + } + if h.lastAttr["declared"] != idName { + t.Fatalf("declared attr = %q, want %q", h.lastAttr["declared"], idName) + } + if h.lastAttr["missing"] != secretName { + t.Fatalf("missing attr = %q, want %q", h.lastAttr["missing"], secretName) + } + }) -func TestForgeTokenSourceMissingNameErrors(t *testing.T) { - res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "OTHER", Value: "x"}}} - ts := newForgeTokenSource(res, "GITHUB_FORGE_TOKEN") - if _, err := ts.Token(context.Background()); err == nil { - t.Fatal("Token with the configured name absent = nil error, want a not-declared error") - } + t.Run("only the client secret declared -> nil source + one Warn naming declared+missing", func(t *testing.T) { + h := &capWarnHandler{} + res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: secretName, Value: "csecret"}}} + tokens, err := buildLinearTokenSource(ctx, cfg, res, slog.New(h)) + if err != nil { + t.Fatalf("buildLinearTokenSource (partial): %v", err) + } + if tokens != nil { + t.Fatal("token source != nil with only the client secret declared, want nil (partial -> off)") + } + if h.warns != 1 { + t.Fatalf("Warn count = %d, want exactly 1 on a partial (secret-only) misconfig", h.warns) + } + if h.lastAttr["declared"] != secretName { + t.Fatalf("declared attr = %q, want %q", h.lastAttr["declared"], secretName) + } + if h.lastAttr["missing"] != idName { + t.Fatalf("missing attr = %q, want %q", h.lastAttr["missing"], idName) + } + }) } // TestValidateForgeSecretDistinctErrors pins record test 7's core property: the @@ -276,7 +254,7 @@ func TestValidateForgeSecretDistinctErrors(t *testing.T) { t.Run("name absent -> not declared", func(t *testing.T) { res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "OTHER", Value: "x"}}} - err := validateForgeSecret(ctx, res, "forge write", "GITHUB_FORGE_TOKEN") + err := validateForgeSecret(ctx, res, "forge write", "APP_KEY") if err == nil { t.Fatal("validateForgeSecret with the name absent = nil, want an error") } @@ -288,7 +266,7 @@ func TestValidateForgeSecretDistinctErrors(t *testing.T) { t.Run("resolve errors -> resolve failed at startup", func(t *testing.T) { sentinel := errors.New("provider unreachable") res := &fakeResolver{err: sentinel} - err := validateForgeSecret(ctx, res, "forge write", "GITHUB_FORGE_TOKEN") + err := validateForgeSecret(ctx, res, "forge write", "APP_KEY") if err == nil { t.Fatal("validateForgeSecret with a resolve error = nil, want an error") } @@ -302,16 +280,16 @@ func TestValidateForgeSecretDistinctErrors(t *testing.T) { t.Run("the two texts are distinguishable", func(t *testing.T) { absent := validateForgeSecret(ctx, - &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "OTHER"}}}, "forge write", "GITHUB_FORGE_TOKEN") - failed := validateForgeSecret(ctx, &fakeResolver{err: errors.New("boom")}, "forge write", "GITHUB_FORGE_TOKEN") + &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "OTHER"}}}, "forge write", "APP_KEY") + failed := validateForgeSecret(ctx, &fakeResolver{err: errors.New("boom")}, "forge write", "APP_KEY") if absent.Error() == failed.Error() { t.Fatal("the not-declared and resolve-failed texts must differ so a crash-loop is diagnosable") } }) t.Run("name present -> nil", func(t *testing.T) { - res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "GITHUB_FORGE_TOKEN", Value: "x"}}} - if err := validateForgeSecret(ctx, res, "forge write", "GITHUB_FORGE_TOKEN"); err != nil { + res := &fakeResolver{resolved: []secrets.ResolvedSecret{{Name: "APP_KEY", Value: "x"}}} + if err := validateForgeSecret(ctx, res, "forge write", "APP_KEY"); err != nil { t.Fatalf("validateForgeSecret with the name present = %v, want nil", err) } }) @@ -342,53 +320,56 @@ func (h *capWarnHandler) WithAttrs([]slog.Attr) slog.Handler { return h } func (h *capWarnHandler) WithGroup(string) slog.Handler { return h } // TestWarnPartialForgeWriteSecrets pins the observability contract for a partial -// write-secret misconfiguration: exactly one of the two required secrets -// declared emits ONE Warn naming the declared and the missing secret, while the +// write-App misconfiguration: exactly ONE of the two required Apps configured +// emits ONE Warn naming the configured and the missing role, while the // intentional both-absent OFF state and the both-present ENABLED state stay -// silent. Guards against a silent hard-outage on an operator typo in one of the -// two env-var names. +// silent. Guards against a silent hard-outage on an operator typo (a missing App +// id or an undeclared App key). func TestWarnPartialForgeWriteSecrets(t *testing.T) { - t.Run("author-only declared -> one Warn naming declared+missing", func(t *testing.T) { + primary := ForgeAppConfig{AppID: 1, InstallationID: 2, AppPrivateKeySecret: "PRIMARY_KEY"} + reviewer := ForgeAppConfig{AppID: 3, InstallationID: 4, AppPrivateKeySecret: "REVIEWER_KEY"} + primaryDeclared := []secrets.ResolvedSecret{{Name: "PRIMARY_KEY"}} + reviewerDeclared := []secrets.ResolvedSecret{{Name: "REVIEWER_KEY"}} + bothDeclared := []secrets.ResolvedSecret{{Name: "PRIMARY_KEY"}, {Name: "REVIEWER_KEY"}} + + t.Run("primary-App-only -> one Warn naming configured+missing", func(t *testing.T) { h := &capWarnHandler{} - declared := []secrets.ResolvedSecret{{Name: defaultForgeSecretName}} - warnPartialForgeWriteSecrets(ForgeConfig{}, declared, slog.New(h)) + warnPartialForgeWriteSecrets(ForgeConfig{App: primary}, primaryDeclared, slog.New(h)) if h.warns != 1 { - t.Fatalf("Warn count = %d, want exactly 1 on a partial (author-only) misconfig", h.warns) + t.Fatalf("Warn count = %d, want exactly 1 on a partial (primary-App-only) misconfig", h.warns) } - if h.lastAttr["declared"] != defaultForgeSecretName { - t.Fatalf("declared attr = %q, want %q", h.lastAttr["declared"], defaultForgeSecretName) + if h.lastAttr["configured"] != "primary App" { + t.Fatalf("configured attr = %q, want %q", h.lastAttr["configured"], "primary App") } - if h.lastAttr["missing"] != defaultForgeReviewerSecretName { - t.Fatalf("missing attr = %q, want %q", h.lastAttr["missing"], defaultForgeReviewerSecretName) + if h.lastAttr["missing"] != "reviewer App" { + t.Fatalf("missing attr = %q, want %q", h.lastAttr["missing"], "reviewer App") } }) - t.Run("reviewer-only declared -> one Warn naming declared+missing", func(t *testing.T) { + t.Run("reviewer-App-only -> one Warn naming configured+missing", func(t *testing.T) { h := &capWarnHandler{} - declared := []secrets.ResolvedSecret{{Name: defaultForgeReviewerSecretName}} - warnPartialForgeWriteSecrets(ForgeConfig{}, declared, slog.New(h)) + warnPartialForgeWriteSecrets(ForgeConfig{ReviewerApp: reviewer}, reviewerDeclared, slog.New(h)) if h.warns != 1 { - t.Fatalf("Warn count = %d, want exactly 1 on a partial (reviewer-only) misconfig", h.warns) + t.Fatalf("Warn count = %d, want exactly 1 on a partial (reviewer-App-only) misconfig", h.warns) } - if h.lastAttr["declared"] != defaultForgeReviewerSecretName { - t.Fatalf("declared attr = %q, want %q", h.lastAttr["declared"], defaultForgeReviewerSecretName) + if h.lastAttr["configured"] != "reviewer App" { + t.Fatalf("configured attr = %q, want %q", h.lastAttr["configured"], "reviewer App") } - if h.lastAttr["missing"] != defaultForgeSecretName { - t.Fatalf("missing attr = %q, want %q", h.lastAttr["missing"], defaultForgeSecretName) + if h.lastAttr["missing"] != "primary App" { + t.Fatalf("missing attr = %q, want %q", h.lastAttr["missing"], "primary App") } }) - t.Run("neither declared -> silent (intentional off)", func(t *testing.T) { + t.Run("neither configured -> silent (intentional off)", func(t *testing.T) { h := &capWarnHandler{} warnPartialForgeWriteSecrets(ForgeConfig{}, nil, slog.New(h)) if h.warns != 0 { t.Fatalf("Warn count = %d, want 0 for the intentional both-absent off state", h.warns) } }) - t.Run("both declared -> silent (enabled path warns nothing)", func(t *testing.T) { + t.Run("both configured -> silent (enabled path warns nothing)", func(t *testing.T) { h := &capWarnHandler{} - declared := []secrets.ResolvedSecret{{Name: defaultForgeSecretName}, {Name: defaultForgeReviewerSecretName}} - warnPartialForgeWriteSecrets(ForgeConfig{}, declared, slog.New(h)) + warnPartialForgeWriteSecrets(ForgeConfig{App: primary, ReviewerApp: reviewer}, bothDeclared, slog.New(h)) if h.warns != 0 { - t.Fatalf("Warn count = %d, want 0 when both secrets are declared", h.warns) + t.Fatalf("Warn count = %d, want 0 when both Apps are configured", h.warns) } }) } diff --git a/go/server/sinks.go b/go/server/sinks.go index fa279ea5b..8df7fbad2 100644 --- a/go/server/sinks.go +++ b/go/server/sinks.go @@ -179,9 +179,9 @@ func startCommsBusConsumers(gctx context.Context, g *errgroup.Group, commsBus *e // notification lanes (RIG-2732 T7) — the GitHub notify lane and the Linear notify // lane — each contributing its webhook-arm drain and its reconciler sweep. The // board and GitHub notify lanes share the App gate (nil-or-set together); the -// Linear notify lane gates INDEPENDENTLY on LINEAR_FORGE_TOKEN, so it is nil-or- -// set on its own. Every lane is nil-checked; a nil lane starts nothing. The -// notify lanes are the same *forgeNotifyLane type, taken variadically so a new +// Linear notify lane gates INDEPENDENTLY on the Linear client-credentials pair, +// so it is nil-or-set on its own. Every lane is nil-checked; a nil lane starts +// nothing. The notify lanes are the same *forgeNotifyLane type, taken variadically so a new // notify lane is one more argument, not a new param. Serve calls this one helper // so the Run starts, which share the serve group + gctx, stay one statement at // the call site (mirroring startCommsBusConsumers). Every Run returns nil on From 25aa03a497803d0f2980e06131b6e698527ea502 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 31 Aug 2026 22:53:21 -0400 Subject: [PATCH 2/2] refactor(forge): bound Linear token client + refresh forgeTokenTTL doc (RIG-3090) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-fix commit on the App-credential super-cutover (PR #827). - Bound the shared Linear `linearagent.TokenSource` HTTP client to a 30s timeout, matching NewGitHub/appTokenSource. The boot-time mint check ran an unbounded `Token(ctx)` over `http.DefaultClient` (no timeout) with the process-root ctx (no deadline), so a half-open TCP to api.linear.app could wedge Serve's whole boot — defeating the fail-fast the check exists for. The bound also caps the same instance the notify lane + write coordinate reuse. - Rewrite the `forgeTokenTTL` doc comment: it still described the deleted `forgeTokenSource` (a driver TokenSource with poll-pass re-resolve and Invalidate-on-401), none of which applies to its sole remaining consumer, the cachedWebhookSecret hot-path cache. Documents its real use. Refs RIG-3090 Co-authored-by: Matt Wilkinson --- go/server/serve.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/go/server/serve.go b/go/server/serve.go index 2c44bf9fd..ab90837d8 100644 --- a/go/server/serve.go +++ b/go/server/serve.go @@ -205,13 +205,14 @@ const ( // (OQ-5): startup sweep + a 30-min ticker (a 304 page-1 GET per enabled repo // is ≈ free, notify_reader.go:12-13; a cold-start zero watermark walks once). defaultReconcileBackstop = 30 * time.Minute - // forgeTokenTTL is the TTL the driver's TokenSource caches a resolved token - // for: a resolve reads the whole declared-secret registry, writes a manifest - // temp file, and drives a full secretspec provider Load (resolver.go:135-165), - // so re-resolving every poll pass would tax the store and provider. The - // cache drops its value on TTL expiry or on Invalidate() (the client calls - // it on a 401/bad-creds-403), so a rotated token still takes effect within - // the TTL or immediately on the next auth failure. + // forgeTokenTTL is the TTL the cachedWebhookSecret hot-path cache holds a + // resolved webhook signing secret for: /webhooks/{github,linear} resolve the + // secret on every request before the HMAC check, and a resolve reads the whole + // declared-secret registry, writes a manifest temp file, and drives a full + // secretspec provider Load (resolver.go:135-165), so an uncached resolve would + // let a garbage POST force that whole Load ahead of authentication. The cache + // bounds the per-request cost to a memcmp; a rotated secret still takes effect + // within the TTL. forgeTokenTTL = 5 * time.Minute ) @@ -1723,7 +1724,11 @@ func buildLinearTokenSource(ctx context.Context, cfg ServeConfig, resolver secre "declared", declaredName, "missing", missingName) return nil, nil //nolint:nilnil // a partial Linear config is an operator typo, surfaced by the Warn; treated as off (a nil source), never a fatal. } - tokens := linearagent.NewTokenSource(clientID, clientSecret, nil, "") + // The 30s-bounded client matches NewGitHub/appTokenSource: it caps the + // boot-time mint below (an unbounded doer would let a half-open TCP to + // api.linear.app wedge Serve's whole boot, defeating this fail-fast check) + // and the same instance the notify lane + write coordinate later reuse. + tokens := linearagent.NewTokenSource(clientID, clientSecret, &http.Client{Timeout: 30 * time.Second}, "") // Boot-time mint check: a Token(ctx) call proves the pair mints (the DL-204 // degrade probe guards only attribution, not the mint path), so a bad secret // or a disabled client_credentials toggle fails Serve here, not on first write.