fix(cli): linear setup must not clobber a workspace's webhook signing secret (#611)

[isadeks]

Summary

Fixes #611. Re-running bgagent linear setup <slug> on an already-installed workspace overwrote its per-workspace webhook signing secret with the stack-wide fallback (a different workspace's secret once >1 is installed) → webhook 401 {"error":"Invalid signature"} for every workspace after the first. Live-hit on demo-abca after a token re-auth; recovery required update-webhook-secret.

Fix

• New pure helper resolveWebhookSecretAction() (cli/src/linear-oauth.ts): preserve | mirror-stackwide | prompt. An existing valid per-workspace secret always wins; else mirror stack-wide (warn for additional workspaces); else prompt.
• setup reads the existing per-workspace secret before the OAuth overwrite and preserves it.
• 5 unit tests incl. the multi-workspace re-run regression.

Governance

Approved issue #611, conforming branch fix/611-….

Test

cli build green — 610 tests (compile + eslint clean).

[scottschreckengaust]

Verdict: Request changes

The design is right and the diagnosis is excellent — extracting the three-way decision into the pure resolveWebhookSecretAction() is the correct move, the helper is well-documented, and its five unit tests cover every branch including the malformed-lin_wh_ fall-through. The receiver semantics confirm the bug is real and serious: verifyLinearRequestForWorkspace (cdk/src/handlers/shared/linear-verify.ts:224-239) treats a per-workspace signature mismatch as fatal with no stack-wide fallback (linear-webhook.ts:125-129), so clobbering workspace #2's secret with workspace #1's stack-wide value does 401 every delivery. Good catch, good structure.

But the fix is only as safe as the value fed into the pure helper, and the impure pre-read that produces that value fails open. Under any Secrets Manager error other than "not found" on a genuine re-run, the code silently re-triggers the exact clobber this PR exists to prevent — while printing green checkmarks. That, plus the absence of any end-to-end test proving acceptance criterion #1, is why this is Request changes rather than Approve-with-nits. The pure helper is tested; the fix is not.

Governance (ADR-003) — PASS

• Backing issue #611 carries the approved label (and bug); acceptance criteria match the implementation.
• Branch fix/611-linear-setup-webhook-secret conforms to (feat|fix|chore|docs)/<issue>-desc.
• Base is fresh: merge-base == origin/main == 22705e8. All 8 CI checks green (CodeQL, build agentcore, secrets/deps/workflow scan, PR-title, dead-code advisory).
• No prior reviews/comments to reconcile.

Vision alignment — PASS

Advances bounded blast radius (a re-run/re-auth on one workspace no longer silently breaks signature verification for the others) and reviewable outcomes (the failure mode was an opaque 401; the fix keeps recovery to the dedicated update-webhook-secret command). No tenet is traded; no ADR needed.

Blocking issues

B1 — The pre-read catch {} fails OPEN and re-introduces the clobber this PR fixes. (cli/src/commands/linear.ts:621-623)

The bare catch {} swallows every error from GetSecretValue and treats it as "first install — nothing to preserve." That assumption only holds for ResourceNotFoundException. For AccessDenied, KMSAccessDeniedException (CMK disabled / key-policy / grant revoked), ThrottlingException, InternalServiceError, or a network fault — all of which occur on a secret that does exist — existingWebhookSecret stays undefined. Traced forward: resolveWebhookSecretAction(undefined, true) → mirror-stackwide (linear-oauth.ts:333-334) → the stack-wide secret (workspace #1's) is read at line 727 and merged into this workspace's bundle at lines 764-771. That is the original bug, now re-armed by an IAM/KMS/throttle hiccup, and because the clobber is a successful PutSecretValue the operator sees only "Setup complete." It surfaces hours later as webhook 401s.

This is strictly worse than the anti-pattern the sibling reader isWebhookSecretConfigured (lines 169-194) was deliberately written to avoid — its own comment (178-182) warns that a bare catch { return false } masks IAM problems, and it re-throws a CliError with an IAM hint on any non-ResourceNotFoundException. The degraded path there merely re-prompts (harmless); here it destroys a working secret (harmful). It is also an AI004 silent-success-masking pattern (empty catch + masking default, no logging, no nosemgrep justification). Fix: fail closed — catch narrowly, treat only ResourceNotFoundException as "nothing to preserve," and re-throw an actionable CliError on everything else, mirroring isWebhookSecretConfigured. Note the JSON.parse at line 616 is inside the same try: a corrupt-but-present bundle should surface, not silently become mirror-stackwide. See inline suggestion.

B2 — No test proves acceptance criterion #1 end-to-end; the load-bearing wiring is uncovered.

Issue #611's AC #1 ("re-running setup does NOT change webhook_signing_secret") and AC #3 ("regression test for the second-workspace re-run case") are asserted only by proxy: the five new tests exercise the pure resolveWebhookSecretAction in isolation (cli/test/linear-oauth.test.ts:286-316). Nothing exercises the setup action itself — not the read-before-overwrite capture (linear.ts:612-623), not its ordering against the strip-and-write at line 646, not the mirror-back at 764-771, and not the B1 fail-open path. A future refactor that moved the read past line 646, or the B1 error path, would leave every current test green while re-breaking the fix. The scaffolding to close this already exists: cli/test/commands/linear-helpers.test.ts:45-62,197-208 module-mocks SecretsManagerClient.send and drives setup via parseAsync. Fix: add a setup-action test where the prior per-workspace bundle holds lin_wh_thisWorkspace, the stack-wide holds a different lin_wh_other, and assert the final per-workspace PutSecretValue payload still contains lin_wh_thisWorkspace. Add a companion test that a non-ResourceNotFoundException on the pre-read does not silently proceed to overwrite (guards B1).

Non-blocking suggestions / nits

N1 — Stale header comment contradicts the new code. (cli/src/commands/linear.ts:699-706) The block still reads "…Either way the stored value is this workspace's signing secret — lift it into the per-workspace bundle without prompting." That is the removed behavior; the whole point of the fix is that a populated stack-wide secret is not necessarily this workspace's. A maintainer following this comment would revert the fix. Rewrite it to describe the preserve → mirror-stackwide → prompt decision. (Not blocking, but high maintainability risk — please fix in this PR.)

N2 — preserve path double-writes and logs a misleading message. (cli/src/commands/linear.ts:646 + 764-772) In the preserve case, line 646 writes the bundle without the secret, then line 770 re-writes it with the same secret that was already stored — two secret versions to land where storage already was, and it prints "Mirrored signing secret to per-workspace OAuth bundle" when nothing was mirrored. Folding existingWebhookSecret into the initial stored object (...(existingWebhookSecret ? { webhook_signing_secret: existingWebhookSecret } : {})) would make line 646 write the correct bundle once, let the preserve path skip the re-write, and — importantly — close a narrow correctness window: today, if any fallible step between 646 and 770 throws (the DynamoDB PutCommand at 656, or isWebhookSecretConfigured at 707, which is designed to throw), the bundle is left persisted without webhook_signing_secret and the 401 returns until update-webhook-secret is run. Optional but recommended alongside B1.

N3 — lin_wh_ prefix as validity check is a heuristic, not authentication, but it is consistent with every sibling in this file — leave as-is.

Documentation — PASS (nothing required)

No user-facing command, flag, env var, or contract changed — this is an internal idempotency/safety fix to existing setup behavior. docs/guides/LINEAR_SETUP_GUIDE.md already documents setup and update-webhook-secret; no update needed, and the Starlight mirror is untouched (no mutation-check risk). Not a roadmap item.

Tests & CI

• Pure-helper coverage: strong. Integration/action coverage of the actual fix: missing (see B2).
• No CDK construct/stack changes → bootstrap synth-coverage: not applicable; no Cedar engine pins moved; no types.ts change (the CLI change is self-contained — webhook_signing_secret already exists and matches on both StoredLinearOauthToken (cli/src/linear-oauth.ts:101) and the CDK resolver (cdk/src/handlers/shared/linear-oauth-resolver.ts:93), no drift).
• All 8 CI checks green on head 45a770b.

Review agents run

• pr-review-toolkit:silent-failure-hunter — invoked. Independently identified B1 as fail-open/AI004; I verified the trace by hand (confirmed stored at lines 624-638 omits webhook_signing_secret, so line 646 strips it; confirmed resolveWebhookSecretAction(undefined, true) → mirror-stackwide).
• pr-review-toolkit:pr-test-analyzer — invoked. Confirmed B2: helper-only coverage, existing mock scaffolding in linear-helpers.test.ts makes the integration test affordable.
• pr-review-toolkit:code-reviewer — invoked. Surfaced N1 (stale comment), N2 (double-write + failure window), and confirmed no secret leakage into logs and no types.ts drift.
• /security-review — done by hand (secrets-handling change): fail-closed analysis (B1), no secret leakage into stdout/errors (the mirror-stackwide catch at line 732 prints only the SDK .message), prefix-validation robustness (N3). No sub-agent for this dimension was available; covered manually.
• Omitted: type-design-analyzer (the new WebhookSecretAction discriminated union is small and idiomatic; no design concern), comment-analyzer (folded into N1 by hand).

Human heuristics

• Proportionality — PASS. A pure decision function + a read-before-write is proportional to the bug; no over-abstraction.
• Coherence — CONCERN. The pre-read (linear.ts:613-623) diverges from the sibling reader isWebhookSecretConfigured (169-194) that solves the identical "read a SM secret in setup" problem — same concept, inconsistent error handling (B1).
• Clarity — CONCERN. The stale header comment (N1, lines 699-706) actively describes the removed behavior; the empty catch (B1) hides failures behind a plausible "first install" default (AI004).
• Appropriateness — CONCERN. Tests assert what the helper does, not what the fix should do end-to-end (B2, AI005); the integration path was verified against no test at all, only the pure function.

_{🤖 Generated with Claude Code}

[scottschreckengaust]

Blocking (B1): this fails OPEN and re-introduces the clobber this PR fixes.

The bare catch {} swallows every error, but only ResourceNotFoundException actually means "first install — nothing to preserve." On a genuine re-run, an AccessDenied / KMSAccessDeniedException / ThrottlingException / network fault leaves existingWebhookSecret === undefined → resolveWebhookSecretAction(undefined, true) returns mirror-stackwide → the stack-wide (workspace #1's) secret is mirrored over this workspace's bundle (lines 764-771). That is exactly the 401 "Invalid signature" regression, now re-armed by an IAM/KMS/throttle hiccup — and it prints "Setup complete." with no signal.

The sibling reader isWebhookSecretConfigured (lines 169-194) already solves this correctly: only ResourceNotFoundException is a clean negative; everything else re-throws a CliError with an IAM hint. Its own comment (178-182) warns against precisely this bare-catch pattern — and there the degraded path merely re-prompts, whereas here it destroys a working secret. This is also an AI004 silent-success-masking hit (empty catch + masking default, no logging, no nosemgrep).

Fail closed and keep JSON.parse failures visible (a corrupt-but-present bundle must not silently become mirror-stackwide):

Suggested change

	} catch {
	} catch (err) {
	const errorName = (err as { name?: string }).name;
	if (errorName !== 'ResourceNotFoundException') {
	// Fail CLOSED: if we cannot confirm the prior bundle, do NOT continue —
	// proceeding would mirror the stack-wide secret over a possibly-existing
	// per-workspace signing secret and break signature verification (the exact
	// bug this path prevents). Mirrors isWebhookSecretConfigured's contract.
	const message = err instanceof Error ? err.message : String(err);
	throw new CliError(
	`Failed to read this workspace's existing Linear OAuth bundle `
	+ `'${linearOauthSecretName(slug)}': ${errorName ?? 'Error'}: ${message}. `
	+ 'Refusing to continue: overwriting could clobber an existing per-workspace '
	+ 'webhook signing secret with the stack-wide fallback. Likely IAM/KMS gap — '
	+ 'confirm secretsmanager:GetSecretValue (and KMS decrypt) on this ARN, then re-run.',
	);
	}
	// ResourceNotFoundException only: genuine first install — nothing to preserve.
	}

[scottschreckengaust]

Nit (N2): the preserve path double-writes the bundle and logs a misleading "Mirrored" message.

When preserving, line 646 has already written the bundle without webhook_signing_secret, then lines 764-771 re-write it with the same secret that was already stored — two secret versions to end up where storage already was, and it then prints "Mirrored signing secret…" (line 771) when nothing was mirrored. It also leaves a narrow correctness window: if any fallible step between 646 and 770 throws (the DynamoDB PutCommand at 656, or isWebhookSecretConfigured at 707, which is designed to throw), the bundle is persisted without the field and the 401 returns until update-webhook-secret is run.

Fold the preserved secret into the initial stored object (line 624) so line 646 writes the correct bundle once and the mid-setup window closes:

const stored: StoredLinearOauthToken = {
  // ...existing fields...
  ...(existingWebhookSecret ? { webhook_signing_secret: existingWebhookSecret } : {}),
};

Then the preserve branch can skip the re-write entirely. Optional, but recommended alongside B1.