Skip to content

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

Merged
isadeks merged 9 commits into
mainfrom
fix/611-linear-setup-webhook-secret
Jul 22, 2026
Merged

fix(cli): linear setup must not clobber a workspace's webhook signing secret (#611)#612
isadeks merged 9 commits into
mainfrom
fix/611-linear-setup-webhook-secret

Conversation

@isadeks

@isadeks isadeks commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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).

…611)

Re-running `bgagent linear setup <slug>` on an already-installed workspace
overwrote that workspace's per-workspace webhook signing secret with the
STACK-WIDE fallback — which belongs to whichever workspace installed first. In a
multi-workspace deployment this silently breaks HMAC signature verification for
every workspace after the first: the receiver rejects Linear's deliveries with
401 {"error":"Invalid signature"} (live-hit on demo-abca after a re-auth).

Root cause (linear.ts setup action): the mirror was guarded only on
`stackWideAlreadyConfigured`, and the OAuth bundle is re-upserted fresh before
that block, so the existing per-workspace `webhook_signing_secret` was already
dropped and then replaced with the wrong (stack-wide) value.

Fix:
- New pure helper `resolveWebhookSecretAction(existingPerWorkspaceSecret,
  stackWideConfigured)` → preserve | mirror-stackwide | prompt. An existing valid
  per-workspace secret always wins (rotation stays `update-webhook-secret`'s job);
  else mirror the stack-wide secret (with a warning to verify for an additional
  workspace); else prompt. 5 unit tests incl. the multi-workspace re-run case.
- `setup` reads the existing per-workspace secret BEFORE the OAuth overwrite and
  preserves it; the mirror path now warns that an additional workspace's Linear
  secret differs.

cli build green (610 tests, compile + eslint clean).
@isadeks
isadeks requested review from a team as code owners July 15, 2026 16:26

@scottschreckengaust scottschreckengaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread cli/src/commands/linear.ts Outdated
Comment thread cli/src/commands/linear.ts
…+ 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).
@isadeks

isadeks commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in e26085d:

B1 (fail-open re-armed the clobber) — fixed, fail closed. Extracted the pre-read into readExistingWebhookSecret() (in linear-oauth.ts, next to resolveWebhookSecretAction). Only ResourceNotFoundException → "nothing to preserve"; a corrupt-but-present bundle (JSON.parse) or ANY other SM error (AccessDenied / KMSAccessDenied / Throttling / network) now propagates, and setup re-throws a CliError with an IAM/KMS hint — mirroring the sibling isWebhookSecretConfigured you pointed to. No more silent mirror-stackwide-over-a-working-secret behind a green checkmark.

B2 (fix untested end-to-end) — added 7 tests. Rather than mock the full OAuth dance (browser + callback server + code exchange + prompts) to reach the seam, I extracted the load-bearing logic into readExistingWebhookSecret and unit-tested it directly: preserve-not-clobber (returns lin_wh_thisWorkspaceresolveWebhookSecretActionpreserve), ResourceNotFound→undefined, malformed/absent secret, and the fail-closed THROW on AccessDenied / KMS / Throttling / corrupt-JSON (the exact B1 path). That guards both AC#1 and the B1 regression at the function boundary.

N1 (stale comment) — fixed. Rewrote the Step-6 header to describe the preserve → mirror-stackwide → prompt decision (it still described the removed "stored value is this workspace's secret — lift it in" behavior).

N2 (double-write + failure window) — fixed. Folded existingWebhookSecret into the INITIAL stored bundle so the OAuth-secret write lands it in one PutSecretValue; the preserve path skips the re-write (guarded by !== stored.webhook_signing_secret), closing the window where a throw between the two writes left the bundle persisted without the secret.

N3 — leaving the lin_wh_ prefix heuristic as-is per your note (consistent with every sibling). Full cli build green (616 tests).

@scottschreckengaust scottschreckengaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Approve with nits

The re-review clears the prior Request changes. The two blocking items from my earlier review are genuinely resolved in the current head (c9a5fc7), verified against the worktree — not just asserted in the commit message — and the one residual gap (an end-to-end setup-action regression test) is a should-fix follow-up, not a merge blocker, because the load-bearing logic on both sides of the untested seam is now independently unit-tested, cli typecheck is clean, and all 8 CI checks are green.

Re-review of prior blocking claims (independently verified against head, not taken on faith)

B1 (fail-open pre-read re-armed the #611 clobber) — RESOLVED. The bare catch {} is gone. The pre-read is extracted to readExistingWebhookSecret() (cli/src/linear-oauth.ts:364-379), which fails closed: only the caller-supplied isNotFound predicate (ResourceNotFoundException) yields undefined (genuine first install); every other GetSecretValue error — and a corrupt-but-present bundle (JSON.parse throws) — propagates. The setup caller (cli/src/commands/linear.ts:629-641) re-wraps a propagated error in a CliError with an actionable secretsmanager:GetSecretValue / kms:Decrypt hint, mirroring the sibling isWebhookSecretConfigured. I confirmed the fail-closed path with tests: readExistingWebhookSecret throws on AccessDenied, KMSAccessDeniedException, ThrottlingException, InternalServiceError, and malformed JSON (cli/test/linear-oauth.test.ts:307-329). Ran the suite: 70/70 pass; linear-oauth.ts at 100% statement coverage.

N1 (stale Step-6 header describing removed behavior) — RESOLVED. The header (linear.ts:726-739) now describes the preserve → mirror-stackwide → prompt decision. A maintainer can no longer follow the comment back into the bug.

N2 (double-write + mid-setup failure window) — RESOLVED. The preserved secret is folded into the initial stored bundle (linear.ts:663) so the first upsertOauthSecret lands the correct payload in one write, and the mirror-back is guarded by webhookSigningSecret !== stored.webhook_signing_secret (linear.ts:799) so the preserve path skips the redundant re-write. I traced all three branches: preserve → guard equal → skip (correct); mirror-stackwide/prompt → stored.webhook_signing_secret undefined → guard true → write (correct). Window closed.

Remaining nit (should-fix follow-up, non-blocking)

B2 (partially addressed) — the setup-action wiring is still asserted only by inspection. The fail-closed seam is now well-tested (7 new readExistingWebhookSecret cases) and the pure decision is fully tested (resolveWebhookSecretAction), which covers the safety half of the original B2. But no test drives the setup action end-to-end to prove issue #611's AC#3 ("regression test for the second-workspace re-run case") against the final PutSecretValue payload. Coverage confirms the gap: linear.ts:741-842 (the entire secretAction branch and the mirror-back guard) is uncovered. The scaffolding to close it already exists — cli/test/commands/linear-helpers.test.ts module-mocks SecretsManagerClient.send and drives setup via parseAsync. Recommended follow-up: a setup-action test where the prior per-workspace bundle holds lin_wh_thisWorkspace, the stack-wide holds a different lin_wh_other, and the final per-workspace PutSecretValue payload is asserted to still contain lin_wh_thisWorkspace. I am not blocking on it because a refactor that re-broke the wiring would have to defeat both independently-tested halves, and the guard logic is simple and verified by inspection — but it is the one thing that would make the fix self-defending. File it as a P2 test-debt follow-up.

N3 (lin_wh_ prefix as a validity heuristic) — unchanged, consistent with every sibling in the file. Leave as-is.

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 (recovery stays scoped to the dedicated update-webhook-secret command). No tenet traded; no ADR needed.

Governance (ADR-003) — PASS

Backing issue #611 carries approved (+ bug); the implementation matches its acceptance criteria (AC#1 and AC#2 satisfied; AC#3 covered in substance by the fail-closed + pure-helper tests, with the literal end-to-end regression test outstanding — see B2). Branch fix/611-linear-setup-webhook-secret conforms. Head c9a5fc7 merges current main.

Security — PASS

Secrets-handling change, reviewed for leakage and fail-closed behavior. No secret value reaches stdout/stderr on any path: error messages print only the SDK error name/message (linear.ts:630-640, 765), never the secret. The change is strictly fail-closed (an IAM/KMS/throttle fault now aborts setup with a clear error rather than silently mirroring the wrong secret). No IAM/Cedar/network/CDK surface touched.

Documentation — PASS (nothing required)

No user-facing command, flag, env var, or contract changed — internal idempotency/safety fix to existing setup behavior. docs/guides/LINEAR_SETUP_GUIDE.md already documents setup and update-webhook-secret. Starlight mirror untouched (no mutation-check risk). No types.ts drift: webhook_signing_secret? is optional and identical on StoredLinearOauthToken (cli/src/linear-oauth.ts:101) and the CDK resolver (cdk/src/handlers/shared/linear-oauth-resolver.ts:93).

Tests & CI

  • Ran test/linear-oauth.test.ts, test/commands/linear.test.ts, test/commands/linear-helpers.test.ts at head: 70/70 pass. cli tsc --noEmit: clean.
  • Pure-helper + fail-closed-seam coverage: strong. End-to-end setup-action coverage: still missing (B2 nit; linear.ts:741-842 uncovered).
  • No CDK construct/stack changes → bootstrap synth-coverage: not applicable. No Cedar engine pin moved.
  • All 8 CI checks green on head c9a5fc7.

Review agents run

  • silent-failure-hunter (scope: the pre-read catch + readExistingWebhookSecret) — covered. Independently confirmed the catch now fails closed and re-throws with an IAM/KMS hint; no AI004 masking-default remains; JSON.parse failures surface. Verified by the fail-closed THROW tests actually running green.
  • pr-test-analyzer (scope: test coverage) — covered. Confirmed via a real coverage run that linear.ts:741-842 is uncovered → the residual B2 nit; the pure helper and fail-closed seam are fully covered.
  • code-reviewer (scope: guidelines/quality) — covered. No secret leakage, no types.ts drift, routing correct (CLI-only change lands in cli/), preserve/mirror/prompt branch logic verified by inspection.
  • comment-analyzer (scope: comment accuracy) — covered. The fold comment (linear.ts:656-662) and the mirror-back guard comment (linear.ts:795-798) accurately describe current behavior; the previously-stale N1 header is fixed.
  • type-design-analyzer (scope: new WebhookSecretAction discriminated union) — covered. The preserve | mirror-stackwide | prompt union is small, idiomatic, readonly, and makes the invalid state (a preserve without a secret) unrepresentable. No concern.
  • The pr-review-toolkit subagents are not exposed as callable tools in this headless environment; each agent's scope was executed by hand with the actual test/typecheck/coverage runs above (not a substitute hand-wave — the analyses were driven by executing the suite and reading the current worktree). None omitted for scope; /security-review covered inline (secrets-handling change) since no dedicated sub-agent is available here.

Human heuristics

  • Proportionality — PASS. A pure decision function plus a fail-closed read-before-write is proportional to the bug; no over-abstraction.
  • Coherence — PASS (was CONCERN). The pre-read now mirrors the sibling isWebhookSecretConfigured error contract (fail closed + CliError with IAM hint) — the earlier divergence is gone.
  • Clarity — PASS (was CONCERN). The stale header is rewritten; the empty catch is replaced with a fail-closed, actionable error.
  • Appropriateness — CONCERN (narrowed). Tests now assert what the fix does at both seams (fail-closed pre-read + pure decision), but not yet the full setup-action payload end-to-end (B2, AI005). Acceptable to merge; file the regression test as follow-up.

Re-review at head c9a5fc7. Prior blocking items B1/N1/N2 verified resolved against the worktree; B2 downgraded to a should-fix follow-up.

Comment thread cli/src/commands/linear.ts
@isadeks
isadeks removed the request for review from scottschreckengaust July 21, 2026 19:47
…ok-secret clobber (#612 review B2 / #611 AC#3)

Scott's re-review approved with one should-fix follow-up (B2): the safety seam
(readExistingWebhookSecret fail-closed) and the pure decision
(resolveWebhookSecretAction) were unit-tested, but nothing drove the `setup`
ACTION end-to-end to prove issue #611's AC#3 against the final PutSecretValue
payload — linear.ts's secretAction branch + mirror-back guard were uncovered.

New test drives `bgagent linear setup demo --no-browser` through parseAsync with
the OAuth flow mocked (awaitOauthCallback echoes the wizard's generated state,
scraped from the printed auth URL; exchangeAuthorizationCode + fetch +
SecretsManager + CFN + DynamoDB stubbed), for the second-workspace re-run:
  - prior per-workspace bundle carries lin_wh_thisWorkspace,
  - stack-wide holds a DIFFERENT lin_wh_otherWorkspace (the #611 clobber source).
Asserts the final per-workspace PutSecretValue payload keeps lin_wh_thisWorkspace
(preserve, folded into the initial bundle — never the stack-wide other), that
the stack-wide ARN is never overwritten by setup, and that the registry row is
still written (setup ran to completion). A refactor that re-broke the wiring now
fails a test, not just inspection.

Partial-mocks linear-oauth (keeps the real readExistingWebhookSecret /
resolveWebhookSecretAction under test; stubs only exchangeAuthorizationCode).
cli gate green: 621 tests, compile + eslint clean.
@isadeks

isadeks commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Closed the one remaining nit — B2 (issue #611 AC#3), the end-to-end setup-action regression test — in bc27dc48.

cli/test/commands/linear-setup-webhook.test.ts drives bgagent linear setup demo --no-browser through parseAsync with the OAuth flow mocked, for the second-workspace re-run case:

It then asserts the final per-workspace PutSecretValue payload still contains lin_wh_thisWorkspace (the preserve path, folded into the initial bundle — never the stack-wide other), that no PutSecretValue ever targets the stack-wide ARN (rotation stays update-webhook-secret's job), and that the registry row is still written (setup ran to completion). This covers the previously-uncovered linear.ts:741-842 (secretAction branch + mirror-back guard) — a refactor that re-broke the wiring now fails a test, not just inspection.

Implementation notes matching your sketch:

  • Follows the linear-helpers.test.ts / jira.test.ts precedent (module-mock SecretsManagerClient.send, CFN, DDB, awaitOauthCallback, swap global.fetch).
  • Partial-mocks ../linear-oauth (requireActual + override only exchangeAuthorizationCode) so the real readExistingWebhookSecret + resolveWebhookSecretAction under test still run.
  • awaitOauthCallback echoes the wizard's internally-generated state (scraped from the auth URL printed under --no-browser), so the state check passes without a real socket.

cli gate green: 621 tests, compile + eslint clean. N3 (lin_wh_ prefix heuristic) left as-is per your note.

@isadeks

isadeks commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

⚠️ Heads-up: the failing "Secrets, deps, and workflow scan" check is not from this PR — same pre-existing, repo-wide pattern as before, just a different dep.

  • osv-scanner now flags svgo@4.0.1 (GHSA-2p49-hgcm-8545, CVSS 8.2, fix 4.0.2) — a newly-published advisory on a pre-existing transitive dep.
  • This PR's diff is one test file (cli/test/commands/linear-setup-webhook.test.ts) — it touches zero deps. svgo@4.0.1 is on main too (via the ^4.0.1 range), so every open PR fails this scan until yarn.lock re-resolves svgo to 4.0.2.
  • Astro is already fine on this branch (7.1.3, from the fix(deps): bump astro to 7.1.3 + re-resolve brace-expansion to clear osv-scanner (#636) #637 merge).

Everything else is green (CodeQL ×4, build, dead-code, PR-title). The fix belongs in its own small yarn.lock re-resolve against main (like #637 did for astro), not in this test-only PR.

isadeks added a commit that referenced this pull request Jul 22, 2026
…scanner (#638)

osv-scanner (required 'Secrets, deps, and workflow scan') fails on main's
yarn.lock, ejecting every PR from the merge queue via the merge_group re-scan
(observed on #612, #616). Newly-published advisories on pre-existing deps — same
class as #636 (astro/brace-expansion), different packages:

  GHSA-2p49-hgcm-8545  svgo             4.0.1  -> 4.0.2   (CVSS 8.2)
  GHSA-8r6m-32jq-jx6q  fast-xml-parser  5.9.3  -> 5.10.1  (CVSS 8.7)
  GHSA-v2hh-gcrm-f6hx  fast-uri         3.1.3  -> 3.1.4   (CVSS 7.5)

All three re-resolve WITHIN their existing ranges (svgo/fast-uri transitive;
fast-xml-parser is ^5.7.0 direct, 5.10.1 is in-range), so this is yarn.lock-only
— no package.json / version-string / source change. All fix versions clean in OSV.

Verified: mise run security:deps -> 'No issues found' (exit 0);
yarn install --frozen-lockfile consistent; mise //docs:build green (68 pages).

Closes #638.

@scottschreckengaust scottschreckengaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Approve

This re-review (head bc27dc48) clears the final should-fix nit from the prior Approve with nits (dismissed) review. I independently re-verified all prior blocking claims (B1/N1/N2) against the current worktree — not the commit messages — and each is genuinely resolved. The one residual gap (B2: the end-to-end setup-action regression test for issue #611 AC#3) is now closed by cli/test/commands/linear-setup-webhook.test.ts. The fix is correct, fails closed, preserves an existing per-workspace secret, and is now self-defending against a future refactor of the load-bearing branch. Ship it.

Vision alignment — PASS

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

Governance (ADR-003) — PASS

Backing issue #611 carries approved (+ bug); AC#1/#2/#3 now all satisfied. Branch fix/611-linear-setup-webhook-secret conforms to (fix)/<issue>-desc. merge-base == current origin/main (238eb30) — base is fresh.

Re-verification of prior blocking claims (checked against head bc27dc4, not taken on faith)

B1 (fail-open pre-read re-armed the #611 clobber) — RESOLVED. readExistingWebhookSecret (cli/src/linear-oauth.ts:362-380) fails closed: only the caller-supplied ResourceNotFoundException predicate yields undefined (genuine first install); every other GetSecretValue error AND a corrupt-but-present bundle (JSON.parse throws) propagate. The setup caller (cli/src/commands/linear.ts:629-641) re-wraps the propagated error in a CliError with an actionable secretsmanager:GetSecretValue / kms:Decrypt hint, mirroring the sibling isWebhookSecretConfigured (linear-oauth.ts:170-195). Six fail-closed tests cover AccessDenied / KMSAccessDenied / Throttling / InternalServiceError / corrupt-JSON (cli/test/linear-oauth.test.ts:344-366). The AI004 masking-default is gone.

B2 (fix untested end-to-end / #611 AC#3) — RESOLVED. cli/test/commands/linear-setup-webhook.test.ts drives the real setup action via parseAsync (mocked OAuth flow) for the second-workspace re-run: the prior per-workspace bundle holds lin_wh_thisWorkspace, the stack-wide holds a different lin_wh_otherWorkspace (the exact #611 clobber source), and the test asserts every per-workspace PutSecretValue payload still carries lin_wh_thisWorkspace and never the other, that no PutSecretValue ever targets the stack-wide ARN (rotation stays update-webhook-secret's job), and that the registry row is written (setup ran to completion). This covers the previously-uncovered linear.ts:741-842. It requireActuals ../linear-oauth so the real readExistingWebhookSecret + resolveWebhookSecretAction run under test, and routes SM commands against real @aws-sdk/client-secrets-manager command classes (instanceof on GetSecretValueCommand / PutSecretValueCommand) — verified against real API shapes, not self-written mocks (no AI001/AI005 concern).

N1 (stale Step-6 header) — RESOLVED. The header (linear.ts:710-739) now describes the preserve → mirror-stackwide → prompt decision; a maintainer can no longer follow the comment back into the bug.

N2 (double-write + mid-setup failure window) — RESOLVED. The preserved secret is folded into the initial stored bundle (linear.ts:663) so the first upsertOauthSecret lands the correct payload in one write; the mirror-back is guarded by webhookSigningSecret !== stored.webhook_signing_secret (linear.ts:799) so the preserve path skips the redundant re-write. Traced all three branches: preserve → guard equal → skip; mirror-stackwide/prompt → stored.webhook_signing_secret undefined → guard true → write. Window closed.

Non-blocking

N3 (lin_wh_ prefix as a validity heuristic) — unchanged, consistent with every sibling in the file. Leave as-is.

Security — PASS

Secrets-handling change, reviewed for leakage and fail-closed behavior. No secret value reaches stdout/stderr on any path — error messages print only the SDK error name/message (linear.ts:630-640; the mirror-stackwide catch at 765), never the secret. The change is strictly fail-closed: an IAM/KMS/throttle fault now aborts setup with a clear error rather than silently mirroring the wrong secret. No IAM/Cedar/network/CDK surface touched.

Documentation — PASS (nothing required)

Internal idempotency/safety fix to existing setup behavior — no user-facing command, flag, env var, or contract changed. docs/guides/LINEAR_SETUP_GUIDE.md already documents setup and update-webhook-secret. Starlight mirror untouched (no mutation-check risk). No types.ts drift: webhook_signing_secret? is optional and identical on StoredLinearOauthToken (cli/src/linear-oauth.ts) and the CDK resolver (cdk/src/handlers/shared/linear-oauth-resolver.ts). No CDK construct/stack change → bootstrap synth-coverage not applicable.

Tests & CI

  • Pure-helper (resolveWebhookSecretAction), fail-closed seam (readExistingWebhookSecret), and now the end-to-end setup-action wiring are all covered. The previously-uncovered linear.ts:741-842 is now exercised.
  • I could not run the suite locally (the pre-staged worktree has no node_modules), but the build (agentcore) check is GREEN on head bc27dc48 — that job runs mise run build, which includes mise //cli:build (tsc compile + jest test + eslint). So the full CLI suite, including the new regression test, compiled, ran green, and passed eslint in CI. The prior reviewer also ran 70/70 green at c9a5fc7.
  • CI secrets-scan (RED) — NOT a blocker on this PR. I inspected the failing job log directly. The scoped gitleaks run (security:secrets:range, range 238eb30..bc27dc48 — exactly this PR's diff) reports "no leaks found" (3 commits scanned). The sole failure is security:deps (osv-scanner): svgo@4.0.1 in yarn.lock (GHSA-2p49-hgcm-8545, CVSS 8.2, fix 4.0.2). svgo is a transitive dep present on main too; this PR's diff touches zero dependencies (CLI source + tests only). This is the known repo-wide, newly-published-advisory pattern (same class as #637's astro re-resolve) — it will red-fail every open PR until a small yarn.lock re-resolve lands on main. Determination: sibling/full-tree artifact, NOT a real secret in this diff. No literal high-entropy secret appears in the diff — the only lin_wh_* strings are obvious test placeholders (lin_wh_thisWorkspace, lin_wh_otherWorkspace, lin_wh_existing, lin_wh_other). Recommend a rebase once svgo is bumped on main so the merge-queue re-scan stays green.

Review agents run

The pr-review-toolkit subagents are not exposed as callable tools in this headless environment; each agent's scope was executed by hand against the current worktree, driven by reading the actual code and the CI job logs (not a hand-wave):

  • silent-failure-hunter (scope: the pre-read catch + readExistingWebhookSecret) — covered. Confirmed the pre-read fails closed and the caller re-throws with an IAM/KMS hint; no AI004 masking-default remains; JSON.parse failures surface. Backed by the six fail-closed THROW tests.
  • pr-test-analyzer (scope: test coverage) — covered. Confirmed the new end-to-end test drives secretAction + mirror-back guard (linear.ts:741-842) and asserts the final per-workspace PutSecretValue payload; closes AC#3.
  • code-reviewer (scope: guidelines/quality) — covered. No secret leakage, no types.ts drift, routing correct (CLI-only change in cli/), preserve/mirror/prompt branch logic verified.
  • comment-analyzer (scope: comment accuracy) — covered. The Step-6 header, the fold comment (linear.ts:656-663), and the mirror-back guard comment (linear.ts:795-798) accurately describe current behavior; the previously-stale N1 header is fixed.
  • type-design-analyzer (scope: WebhookSecretAction discriminated union) — covered. preserve | mirror-stackwide | prompt is small, idiomatic, readonly, and makes preserve-without-a-secret unrepresentable. No concern.
  • /security-review — covered inline (secrets-handling change): fail-closed analysis, no secret leakage into stdout/errors. None omitted for scope.

Human heuristics

  • Proportionality — PASS. A pure decision function plus a fail-closed read-before-write is proportional to the bug; no over-abstraction.
  • Coherence — PASS. The pre-read now mirrors the sibling isWebhookSecretConfigured error contract (fail closed + CliError with IAM hint); the earlier divergence is gone.
  • Clarity — PASS. The stale header is rewritten; the empty catch is replaced with a fail-closed, actionable error.
  • Appropriateness — PASS (was CONCERN). Tests now assert what the fix should do at all three seams — the pure decision, the fail-closed pre-read, AND the full setup-action PutSecretValue payload end-to-end. The integration test verifies against real SDK command classes, not only self-written mocks.

Re-review at head bc27dc4. Prior B1/N1/N2 verified resolved against the worktree; B2 (issue #611 AC#3) now closed by cli/test/commands/linear-setup-webhook.test.ts. RED CI = osv-scanner svgo@4.0.1 (transitive, on main; zero deps touched here) — scoped gitleaks range is clean. Approve; recommend a rebase once svgo is bumped on main.

@isadeks
isadeks enabled auto-merge July 22, 2026 12:00
isadeks added a commit to Kalindi-Dev/sample-autonomous-cloud-coding-agents that referenced this pull request Jul 22, 2026
…scanner (aws-samples#638) (aws-samples#639)

osv-scanner (required 'Secrets, deps, and workflow scan') fails on main's
yarn.lock, ejecting every PR from the merge queue via the merge_group re-scan
(observed on aws-samples#612, aws-samples#616). Newly-published advisories on pre-existing deps — same
class as aws-samples#636 (astro/brace-expansion), different packages:

  GHSA-2p49-hgcm-8545  svgo             4.0.1  -> 4.0.2   (CVSS 8.2)
  GHSA-8r6m-32jq-jx6q  fast-xml-parser  5.9.3  -> 5.10.1  (CVSS 8.7)
  GHSA-v2hh-gcrm-f6hx  fast-uri         3.1.3  -> 3.1.4   (CVSS 7.5)

All three re-resolve WITHIN their existing ranges (svgo/fast-uri transitive;
fast-xml-parser is ^5.7.0 direct, 5.10.1 is in-range), so this is yarn.lock-only
— no package.json / version-string / source change. All fix versions clean in OSV.

Verified: mise run security:deps -> 'No issues found' (exit 0);
yarn install --frozen-lockfile consistent; mise //docs:build green (68 pages).

Closes aws-samples#638.
@isadeks
isadeks added this pull request to the merge queue Jul 22, 2026
Merged via the queue into main with commit 7ef21dc Jul 22, 2026
8 checks passed
@isadeks
isadeks deleted the fix/611-linear-setup-webhook-secret branch July 22, 2026 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): linear setup clobbers a second workspace's webhook signing secret with the stack-wide one → 401 Invalid signature

2 participants