fix(cli): address #612 review — fail-closed webhook pre-read + tests + nits

B1 (fail-open re-armed the clobber): the pre-read catch swallowed EVERY
GetSecretValue error and treated it as 'first install — nothing to preserve'.
That only holds for ResourceNotFoundException; for AccessDenied / KMSAccessDenied
/ Throttling / network / a corrupt bundle — all on a secret that EXISTS —
existingWebhookSecret stayed undefined → resolveWebhookSecretAction mirrors the
stack-wide (workspace #1's) secret over this workspace's real one, re-arming the
#611 401 clobber silently behind a green 'Setup complete'. Extracted the pre-read
into readExistingWebhookSecret() which FAILS CLOSED: only ResourceNotFound → 'none
to preserve'; a corrupt bundle (JSON.parse) or any other SM error propagates, and
setup re-throws an actionable CliError with an IAM/KMS hint (mirroring the sibling
isWebhookSecretConfigured).

B2 (fix was untested end-to-end): the 5 prior tests only exercised the pure
resolveWebhookSecretAction. Added 7 tests for readExistingWebhookSecret covering
the load-bearing seam — preserve-not-clobber, ResourceNotFound→undefined,
malformed/absent secret, and the fail-closed THROW on AccessDenied / KMS /
Throttling / corrupt JSON (the exact B1 path).

N1 (stale comment): the Step-6 header still described the REMOVED behavior ('the
stored value is this workspace's secret — lift it in'). Rewrote it to describe the
preserve → mirror-stackwide → prompt decision so a maintainer can't revert the fix.

N2 (double-write + failure window): fold the preserved secret into the INITIAL
 bundle so the OAuth-secret write lands it in ONE PutSecretValue; the
preserve path skips the later re-write (guarded by !== stored.webhook_signing_secret).
Closes the window where a throw between the two writes left the bundle persisted
without the secret (401 until update-webhook-secret).

Full cli build green (616 tests).

Author: isadeks

cli/src/commands/linear.ts

@@ -43,6 +43,7 @@ import {
   generatePkce,
   LINEAR_OAUTH_SECRET_PREFIX,
   linearOauthSecretName,
+  readExistingWebhookSecret,
   resolveWebhookSecretAction,
   StoredLinearOauthToken,
 } from '../linear-oauth';
@@ -609,17 +610,34 @@ export function makeLinearCommand(): Command {
         // first) — that silently breaks signature verification (401 "Invalid
         // signature") for every workspace after the first in a multi-workspace
         // deployment. Rotation stays the job of `update-webhook-secret`.
+        // Fail CLOSED (#612 review B1): read this workspace's existing signing
+        // secret before the overwrite. Only ResourceNotFoundException is a clean
+        // first-install; any other SM error (AccessDenied, KMSAccessDenied,
+        // Throttling, network) — or a corrupt bundle — must surface, NOT default
+        // to undefined (which would mirror the stack-wide secret over a working
+        // per-workspace one → the #611 401 clobber, silently, behind a green
+        // "Setup complete"). Extracted to readExistingWebhookSecret + unit-tested.
         let existingWebhookSecret: string | undefined;
         try {
-          const prior = await sm.send(new GetSecretValueCommand({ SecretId: linearOauthSecretName(slug) }));
-          if (prior.SecretString) {
-            const priorBundle = JSON.parse(prior.SecretString) as Partial<StoredLinearOauthToken>;
-            if (priorBundle.webhook_signing_secret?.startsWith('lin_wh_')) {
-              existingWebhookSecret = priorBundle.webhook_signing_secret;
-            }
-          }
-        } catch {
-          // No prior bundle (first install) — nothing to preserve.
+          existingWebhookSecret = await readExistingWebhookSecret(
+            async () => {
+              const prior = await sm.send(new GetSecretValueCommand({ SecretId: linearOauthSecretName(slug) }));
+              return prior.SecretString;
+            },
+            (err) => (err as { name?: string }).name === 'ResourceNotFoundException',
+          );
+        } catch (err) {
+          const errorName = (err as { name?: string }).name;
+          const message = err instanceof Error ? err.message : String(err);
+          throw new CliError(
+            `Failed to read the existing Linear webhook secret for '${slug}' before re-write: `
+            + `${errorName ?? 'Error'}: ${message}. Refusing to proceed — continuing could clobber `
+            + 'this workspace\'s webhook signing secret with the stack-wide value (a 401 on every '
+            + 'delivery). Likely an IAM/KMS gap: confirm your CLI principal has '
+            + '`secretsmanager:GetSecretValue` (and kms:Decrypt if a CMK) on '
+            + `${linearOauthSecretName(slug)}, or run \`bgagent linear update-webhook-secret ${slug}\` `
+            + 'after fixing access.',
+          );
         }
         const stored: StoredLinearOauthToken = {
           access_token: tokenResponse.access_token,
@@ -635,6 +653,14 @@ export function makeLinearCommand(): Command {
           installed_at: now,
           updated_at: now,
           installed_by_platform_user_id: cognitoSub,
+          // Fold the preserved secret into the INITIAL bundle (#612 review N2):
+          // the OAuth-secret write below then lands the correct bundle in ONE
+          // PutSecretValue, and the preserve path skips the later re-write. This
+          // also closes a narrow window — if any fallible step between the two
+          // writes threw, the bundle was left persisted WITHOUT the secret (401
+          // until update-webhook-secret). Only set for a real `lin_wh_` value;
+          // mirror-stackwide / prompt cases add it in the mirror-back below.
+          ...(existingWebhookSecret ? { webhook_signing_secret: existingWebhookSecret } : {}),
         };
         if (!stored.refresh_token) {
           throw new CliError(
@@ -696,14 +722,21 @@ export function makeLinearCommand(): Command {
         // without re-onboarding. Multi-workspace installs need each
         // workspace to own its own per-workspace signing secret — only
         // the FIRST install can populate the stack-wide one usefully.
-        // If stack-wide is already populated, this is either a re-run
-        // of setup on the SAME workspace or the FIRST workspace of a
-        // future multi-workspace install. Either way the stored value
-        // is this workspace's signing secret — lift it into the
-        // per-workspace bundle without prompting (auto-migration to
-        // the new shape). Rotation is not setup's job: use
-        // `bgagent linear update-webhook-secret <slug>` to rotate the
-        // signing secret without re-running OAuth.
+        //
+        // resolveWebhookSecretAction picks one of three outcomes from
+        // (existingWebhookSecret, stackWideAlreadyConfigured):
+        //   • preserve      — this workspace ALREADY has its own `lin_wh_`
+        //                     secret (a re-run). Keep it; do NOT overwrite from
+        //                     the stack-wide fallback, which is a DIFFERENT
+        //                     workspace's secret once >1 is installed. This is
+        //                     the #611 clobber fix — the stack-wide value is NOT
+        //                     necessarily this workspace's.
+        //   • mirror-stackwide — no per-workspace secret yet but stack-wide is
+        //                     set: mirror it (correct for the first/only
+        //                     workspace; warn for an additional one).
+        //   • prompt        — nothing to go on: ask for the signing secret.
+        // Rotation is not setup's job: use
+        // `bgagent linear update-webhook-secret <slug>` to rotate.
         const stackWideAlreadyConfigured = await isWebhookSecretConfigured(sm, webhookSecretArn!);
         let webhookSigningSecret: string | undefined;
 
@@ -759,9 +792,11 @@ export function makeLinearCommand(): Command {
           webhookSigningSecret = webhookSecret;
         }
 
-        // Mirror into the per-workspace OAuth secret so the receiver can
-        // look it up by orgId. Re-upsert with the merged payload.
-        if (webhookSigningSecret) {
+        // Mirror into the per-workspace OAuth secret so the receiver can look it
+        // up by orgId. In the PRESERVE case the secret is already in `stored`
+        // (folded above) and was written by the OAuth-secret upsert — no re-write
+        // needed. Only mirror-stackwide / prompt produce a NEW secret to persist.
+        if (webhookSigningSecret && webhookSigningSecret !== stored.webhook_signing_secret) {
           const merged: StoredLinearOauthToken = {
             ...stored,
             webhook_signing_secret: webhookSigningSecret,

cli/src/linear-oauth.ts

@@ -335,3 +335,46 @@ export function resolveWebhookSecretAction(
   }
   return { kind: 'prompt' };
 }
+
+/**
+ * Read this workspace's EXISTING `webhook_signing_secret` before `setup`
+ * overwrites the OAuth bundle — FAIL CLOSED (#612 review B1).
+ *
+ * `fetchSecretString` fetches the prior per-workspace OAuth `SecretString`
+ * (typically a `SecretsManagerClient.send(GetSecretValueCommand)` wrapper). The
+ * value fed to {@link resolveWebhookSecretAction} decides whether the existing
+ * secret is preserved or the stack-wide one is mirrored over it — so a wrong
+ * `undefined` here re-arms the #611 clobber. Therefore:
+ *
+ *   • secret missing (`ResourceNotFoundException`) → genuine first install,
+ *     return undefined (nothing to preserve).
+ *   • secret present with a valid `lin_wh_…` `webhook_signing_secret` → return it.
+ *   • secret present but no/malformed secret → return undefined (nothing valid
+ *     to preserve; the caller mirrors/prompts).
+ *   • ANY OTHER error (AccessDenied, KMSAccessDenied, Throttling, network, or a
+ *     corrupt-JSON bundle) → THROW. Swallowing it would leave undefined and
+ *     silently clobber a working secret behind a green "Setup complete".
+ *
+ * `isNotFound` classifies the fetch error; callers pass an
+ * `name === 'ResourceNotFoundException'` check. On a throw the caller re-raises
+ * a CliError with an IAM/KMS hint (mirroring `isWebhookSecretConfigured`).
+ */
+export async function readExistingWebhookSecret(
+  fetchSecretString: () => Promise<string | undefined>,
+  isNotFound: (err: unknown) => boolean,
+): Promise<string | undefined> {
+  let raw: string | undefined;
+  try {
+    raw = await fetchSecretString();
+  } catch (err) {
+    if (isNotFound(err)) return undefined; // genuine first install
+    throw err; // fail closed — caller wraps with an actionable CliError
+  }
+  if (!raw) return undefined;
+  // A corrupt-but-present bundle must surface (JSON.parse throws → propagates),
+  // NOT silently become "nothing to preserve" → mirror-stackwide.
+  const bundle = JSON.parse(raw) as Partial<StoredLinearOauthToken>;
+  return bundle.webhook_signing_secret?.startsWith('lin_wh_')
+    ? bundle.webhook_signing_secret
+    : undefined;
+}

cli/test/linear-oauth.test.ts

@@ -28,6 +28,7 @@ import {
   LINEAR_OAUTH_SCOPES,
   LINEAR_TOKEN_ENDPOINT,
   linearOauthSecretName,
+  readExistingWebhookSecret,
   refreshAccessToken,
   resolveWebhookSecretAction,
 } from '../src/linear-oauth';
@@ -314,3 +315,53 @@ describe('resolveWebhookSecretAction', () => {
     expect(resolveWebhookSecretAction('', false)).toEqual({ kind: 'prompt' });
   });
 });
+
+describe('readExistingWebhookSecret — fail-closed pre-read (#612 review B1/B2)', () => {
+  const notFound = (err: unknown) => (err as { name?: string }).name === 'ResourceNotFoundException';
+  const bundle = (secret?: string) =>
+    JSON.stringify({ access_token: 'a', webhook_signing_secret: secret });
+
+  it('returns the existing lin_wh_ secret when the bundle has one (→ preserve, not clobber)', async () => {
+    const got = await readExistingWebhookSecret(async () => bundle('lin_wh_thisWorkspace'), notFound);
+    // This is the value that makes resolveWebhookSecretAction PRESERVE — the fix.
+    expect(got).toBe('lin_wh_thisWorkspace');
+    expect(resolveWebhookSecretAction(got, true)).toEqual({ kind: 'preserve', secret: 'lin_wh_thisWorkspace' });
+  });
+
+  it('returns undefined on ResourceNotFoundException (genuine first install)', async () => {
+    const got = await readExistingWebhookSecret(async () => {
+      throw Object.assign(new Error('no'), { name: 'ResourceNotFoundException' });
+    }, notFound);
+    expect(got).toBeUndefined();
+  });
+
+  it('returns undefined when the bundle exists but has no/malformed secret', async () => {
+    expect(await readExistingWebhookSecret(async () => bundle(undefined), notFound)).toBeUndefined();
+    expect(await readExistingWebhookSecret(async () => bundle('not-a-wh'), notFound)).toBeUndefined();
+    expect(await readExistingWebhookSecret(async () => undefined, notFound)).toBeUndefined();
+  });
+
+  it('THROWS (fails closed) on AccessDenied — must NOT default to undefined and clobber', async () => {
+    // The B1 bug: a bare catch here would return undefined → mirror-stackwide →
+    // the #611 clobber, silently. The pre-read must surface the error instead.
+    const accessDenied = Object.assign(new Error('denied'), { name: 'AccessDeniedException' });
+    await expect(
+      readExistingWebhookSecret(async () => { throw accessDenied; }, notFound),
+    ).rejects.toBe(accessDenied);
+  });
+
+  it('THROWS on KMSAccessDeniedException / Throttling / network (any non-not-found)', async () => {
+    for (const name of ['KMSAccessDeniedException', 'ThrottlingException', 'InternalServiceError']) {
+      const err = Object.assign(new Error(name), { name });
+      await expect(
+        readExistingWebhookSecret(async () => { throw err; }, notFound),
+      ).rejects.toBe(err);
+    }
+  });
+
+  it('THROWS on a corrupt-but-present bundle (JSON.parse) — not silently "nothing to preserve"', async () => {
+    await expect(
+      readExistingWebhookSecret(async () => '{not valid json', notFound),
+    ).rejects.toThrow();
+  });
+});