fix(jira): repair webhook admission and secret rotation - #710
fix(jira): repair webhook admission and secret rotation#710ayushtr-aws wants to merge 11 commits into
Conversation
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
1. Verdict
Request changes — the functional fix is sound and well-tested, but the PR silently trades away a documented ADR-015 tenet (per-tenant signature binding / multi-tenant support for Jira) without updating that ADR, and it ships a documentation/comment set that now contradicts itself in three places. Neither is a large amount of work; the code changes themselves are close to mergeable.
2. Vision alignment
Mostly aligned, with one undocumented tenet trade.
Aligned:
- Bounded blast radius — making unmapped/removed Jira projects a true no-op for every event type (
cdk/src/handlers/jira-webhook-processor.ts:303-317,getActiveProjectMappingat:1047) is exactly right. A site-wide admin-console webhook that posts ABCA comments into projects that never opted in is unbounded blast radius by any reading, and the oldsafeReportIssueFailureon the unmapped path was that bug. Good fix, and the tests pin it (cdk/test/handlers/jira-webhook-processor.test.ts:385-407,:864-873). - Fire-and-forget — no change to the async path;
update-webhook-secretis an operator-plane command, not a task-plane one. - Reviewable outcomes — the added attribution diagnostics (
jira_account_source,jira_identity_lookup_key) plus the new troubleshooting section are a real observability win over the previous "isn't linked to a platform user, runbgagent jira link" dead-end, which did not tell the operator which account to link.
Trade-off that needs an ADR update (see Blocking #1): ADR-015 §Multi-tenant signature binding states the design intent explicitly: the stack-wide secret "is not copied into later tenants' bundles", and the per-tenant secret "proves which tenant signed a delivery". This PR inverts that: bgagent jira setup now refuses outright to onboard a second active Jira tenant (cli/src/commands/jira.ts:701-703). That is a defensible decision — one webhook URL genuinely cannot select among N secrets when the payload omits cloudId — but it converts "multi-tenant with per-tenant binding" into "single-tenant by hard constraint," and ADR-015 (lines 44, 49-51, 63) still asserts the opposite. Per the review process, an undocumented tenet trade is a blocking concern.
3. Blocking issues
B1 — ADR-015 not updated for the single-active-tenant constraint (cli/src/commands/jira.ts:701)
setup now throws multiTenantWebhookError whenever any other active tenant exists in the registry. That is a hard product constraint on the Jira channel, and it directly contradicts three ADR-015 statements that remain unedited on this branch:
- line 44: "The stack-wide secret is seeded only once (from the first tenant) for single-tenant back-compat — it is not copied into later tenants' bundles"
- lines 49-51 (§Multi-tenant signature binding): describes the receiver as preserving "the fail-closed multi-tenant guarantee"
- line 63: "(+) Per-tenant credential isolation, signature binding, and the changelog-diff trigger keep the trust and re-trigger semantics correct for multi-tenant installs"
Also stale on line 68: "(!) rotating it in Jira without re-running bgagent jira setup causes silent 401s" — the remedy is now update-webhook-secret.
Risk: the next contributor reads ADR-015, believes multi-tenant Jira is supported and intentionally per-tenant-bound, and either re-adds the second-tenant path or builds on the assumption. The guide note added at docs/guides/JIRA_SETUP_GUIDE.md:150 is good but a guide does not override an ADR.
Fix: amend ADR-015 in this PR (a Superseded by #709 / Revision section is fine, no new ADR needed) to state (a) admin-console webhooks omit cloudId, (b) the Jira channel therefore supports exactly one active tenant, (c) the stack-wide secret is now a synchronized copy of that tenant's secret rather than a first-tenant-only seed, and (d) the rotation command. Then mise //docs:sync.
B2 — The multi-tenant guard in setup is completely untested (cli/src/commands/jira.ts:699-704)
The rotation command's identical guard has four dedicated tests (cli/test/commands/jira.test.ts:471, :493, :539, :561). The setup guard — which is the security-relevant one, because it is what prevents an operator from onboarding tenant B and thereby silently overwriting tenant A's stack-wide verifier — has none. The only new setup test (cli/test/commands/jira.test.ts:1044) mocks the Scan to return exactly [cloud-123], i.e. the happy path.
This matters more than a normal coverage gap because of the guard's placement: it runs after the OAuth dance and before the secret/registry writes. A future refactor that moves the makeDocClient/Scan below the upsertOauthSecret call would leave tenant B's OAuth bundle and registry row written while the command throws — a half-onboarded tenant with an active registry row. Nothing in the suite would catch that reordering.
Fix: add two setup tests — (a) a Scan returning a different active tenant rejects with /multiple tenant secrets/ and asserts smSend was never called (so the ordering invariant is pinned, matching the expect(smSend).not.toHaveBeenCalled() assertion already used at cli/test/commands/jira.test.ts:490); (b) a Scan returning [] for a first-ever install still proceeds (the otherActiveTenantIds filter makes this pass today — worth pinning, since the rotation path treats empty as fatal and the asymmetry is easy to "fix" wrongly later).
B3 — Comments and type docs now contradict the implemented behavior (3 sites)
The PR carefully rewrote the CDK-side comments but missed the mirror copies, leaving the codebase asserting both the old and new models:
cli/src/jira-oauth.ts:114-124—StoredJiraOauthToken.webhook_signing_secretstill reads "Webhook subscriptions are tenant-scoped, so a single stack-wide signing secret cannot verify events from multiple tenants" and "the receiver falls back to the stack-wideJIRA_WEBHOOK_SECRET_ARN". The CDK twin atcdk/src/handlers/shared/jira-oauth-resolver.ts:93-102was updated in this diff to say the opposite. These two type docs describe the same wire field and are now in direct conflict. (The repo already treats CLI/CDK shared-shape drift as a first-class hazard — see the AGENTS.md routing table entry forshared/types.ts↔cli/src/types.ts.)cdk/src/constructs/jira-integration.ts:52-56— the new comment claims "The explicit JSON placeholder can never accidentally match a valid operator HMAC secret." WithisWebhookSecretPlaceholderdeleted, nothing in the codebase reads the marker key any more; the placeholder's recognizability is now dead information. The comment describes a property no consumer depends on, and a reader will hunt for the (now nonexistent) recognizer. Say instead that the JSON shape is a non-verifiable initial value thatsetupunconditionally overwrites.cdk/test/constructs/jira-integration.test.ts:61-70— untouched by this PR, and its rationale is now false: "the webhook secret MUST seed an explicit JSON placeholder so the CLI can distinguish 'never configured' from an operator-set value." The CLI no longer distinguishes anything —setupalways overwrites. The test still asserts a real property (the seeded value isn't a bare random string an attacker could brute-force-shape), but the stated reason is the deleted #368 heuristic. A test whose comment justifies it by a deleted mechanism is how the assertion gets deleted next.
Fix: update all three to the synchronize-always model. Keep the assertion in (3); rewrite only its why.
4. Non-blocking suggestions / nits
cdk/src/handlers/jira-webhook-processor.ts:391—jira_actor_display_nameis new PII in CloudWatch. Every other identity field added in this hunk is an opaque Atlassian accountId;displayNameis a human name. It is the only PII-shaped field logged by any Jira handler (verified:grepfordisplay_name:acrosscdk/src/handlers/returns exactly this one line). The repo has an explicit PII-redaction posture elsewhere (docs/design/SECURITY.md:170,deny-reason-scanner.ts). The accountId +jira_identity_lookup_keyalready give the operator everything needed to runinvite-user, so the display name buys little. Suggest dropping it, or at minimum note in the PR why it is needed.cli/src/commands/jira.ts:311-336— the rollback is best-effort by construction and worth saying so. If the process is killed between the tenantPutSecretValueand the stack-wide one, the two copies diverge with no rollback. That is acceptable (the remedy is re-runningupdate-webhook-secret, and the guide already says "Keep the Jira webhook disabled until the command succeeds"), but the function's docstring reads as if two-phase durability is guaranteed. One sentence — "not atomic; a crash between writes leaves the copies divergent, re-run to converge" — would prevent a future reader trusting it too far.cli/src/commands/jira.ts:998+:776—GetSecretValue→PutSecretValueread-modify-write on the shared OAuth bundle. BothsynchronizeJiraWebhookSecretscall sites re-read the bundle and write it back whole. This races the Lambda-side token refresher (cdk/src/handlers/shared/jira-oauth-resolver.ts, which holdsPutSecretValueand rotatesrefresh_tokenon every use — ADR-015 line 55 warns that losing a rotated refresh token bricks the tenant). The window is a few hundred ms of an interactive operator command, and the pre-existinginvite-userpath (:1067-1096) has the identical pattern, so this is not newly introduced and not blocking. Butsetupnow performs an extra read-back at:776that did not exist before, widening the window slightly. If you want to close it,PutSecretValueaccepts aClientRequestToken, or aVersionId-conditioned write would make it CAS-like.ConsistentRead: trueblanket application. Correct for the read-your-writes bugs this fixes (jira-link.ts:70,lookupPlatformUser,getActiveProjectMapping, the registry rows) and cheap on PAY_PER_REQUEST at this volume. Note it does double the RCU cost of the registryScaninresolveSoleTenantCloudId(cdk/src/handlers/jira-webhook-processor.ts:145) on every admin-console delivery. Given the table is one row per tenant that is genuinely negligible; flagging only so it is a known cost rather than an accident.lookupPlatformUsertightened fromstatus !== 'pending'tostatus !== 'active'(cdk/src/handlers/jira-webhook-processor.ts:1036-1043). Fail-closed and correct — the only writer of a non-pending row isjira-link.ts:105, which always setsstatus: 'active'(confirmed via history: that field has been written since the original #302 landing), so there is no legacy row shape this silently un-links. Good change; calling it out because a reader might worry about back-compat.cli/src/commands/jira.ts:1115—invite-user's "already linked" pre-checkGetdid not getConsistentRead: truewhile every otherGetin the file did. It is only an advisory warning so a stale read is harmless, but the inconsistency will read as an oversight.- Branch name
fix/709-jira-webhook-admissionmatches the convention. PR body is genuinely good — it explains why, and the live-acceptance evidence (401 on bad HMAC, matching SHA-256 hashes, silent unmapped-project behavior, idempotent replay) is the kind of verification this repo should ask for more often.
5. Documentation
Updated: docs/guides/JIRA_SETUP_GUIDE.md (single-active-tenant callout, rotation command, admission-silence rule, new "Linking succeeds but a trigger says the Jira user is unlinked" section), cli/README.md (subcommand list + usage block), and the Starlight mirror docs/src/content/docs/using/Jira-setup-guide.md.
Mirror sync: verified clean. I ran node scripts/sync-starlight.mjs from docs/ at the head SHA and git status --porcelain came back empty — the mirror is regenerated and byte-consistent, with only the expected link-rewrite deltas vs. the source. This will not trip CI's "Fail build on mutation".
Missing: ADR-015 (blocking, B1). The new troubleshooting section is well-targeted at the actual customer failure — it correctly calls out that "the name shown above an ABCA comment is not proof that the same account triggered the event," which is the non-obvious part.
Issue tracking: #709 exists, carries the approved label, and its six acceptance criteria map 1:1 onto the diff. Governance satisfied.
6. Tests & CI
CI: all 8 checks green at 4dfe3e1 (CodeQL ×3 + aggregate, dead-code advisory, secrets/deps/workflow scan, PR-title lint, build (agentcore) 11m46s). Notably the "Secrets, deps, and workflow scan" is passing here — the PR body's caveat about dependency advisories is stale, since #711/#718 landed those fixes on main and this branch merged main three times (last at 9bd1bc25). Base is current: df1ebac6 is an ancestor of HEAD, so nothing is being reasoned about against a stale base. mergeStateStatus: BLOCKED reflects the missing approval, not a check failure.
Coverage — strong on the paths that were broken, one hole:
- Silent-admission behavior: thoroughly pinned, including the negative assertions that matter (
expect(reportIssueFailureMock).not.toHaveBeenCalled()) across unmapped, removed, and no-project-key states for both the label path and the comment path. The twosafeReportIssueFailureresilience tests were correctly re-pointed at the surviving report path (:949-970) rather than deleted when their old trigger became silent — that is the right instinct. ConsistentRead: asserted at each of the ~8 call sites rather than assumed.synchronizeJiraWebhookSecrets: all three branches covered — success, rollback-on-stack-wide-failure, and rollback-also-fails (cli/test/commands/jira.test.ts:1544-1620). The rollback test asserting the restored payload.toEqual(stored)is exactly the right assertion.- Hole: the
setupmulti-tenant guard (B2).
Bootstrap synth-coverage: not applicable. No new CFN resource types — the AWS::SecretsManager::Secret in jira-integration.ts is pre-existing and only its comments changed; cdk/src/bootstrap/** and BOOTSTRAP_VERSION are correctly untouched.
Test performance: no CDK synth changes; no test re-enables aws:cdk:bundling-stacks, and no new new App() + Template.fromStack() per-test patterns (#366 clean).
7. Review agents run
I must be transparent about a process gap here.
/security-review— RUN. In scope (secrets handling, webhook input gateway, HMAC admission). Findings folded in above: no HIGH/MEDIUM exploitable vulnerability introduced. Specifically checked and cleared: (a) the stack-wide secret is now equal to the sole tenant's secret rather than an independent credential, which does not weaken the receiver —jira-webhook-processor.ts:285-297still ignores a body-suppliedcloudIdon stack-wide-verified deliveries and binds to the sole active tenant, so the ADR-015 fail-closed property survives the change in substance even as its wording goes stale; (b)verifyJiraSignatureretainsisUsableHmacSecret(empty/whitespace rejected) andtimingSafeEqual; (c) the.trim()added to the prompted secret atcli/src/commands/jira.ts:768is a correctness fix, not a weakening — an untrimmed trailing newline was a plausible root cause of the reported 401s; (d)getActiveProjectMappingfails closed on a missing/non-active row. Items surfaced as non-blocking: PII in logs (nit 1), non-atomic dual-secret write (nit 2), read-modify-write race on the OAuth bundle (nit 3).pr-review-toolkit:code-reviewer— NOT RUN.pr-review-toolkit:silent-failure-hunter— NOT RUN. Clearly in scope (this PR is largely about converting loud failures to silent skips, plus a new try/catch rollback).pr-review-toolkit:type-design-analyzer— NOT RUN. In scope (getActiveProjectMappingreturnsRecord<string, unknown> | null;synchronizeJiraWebhookSecretsis a new exported contract).pr-review-toolkit:comment-analyzer— NOT RUN. In scope, and would very likely have found B3 independently.pr-review-toolkit:pr-test-analyzer— NOT RUN. In scope, and would very likely have found B2 independently.
Reason for the omissions (not a judgment that the diff was too simple): no subagent-dispatch tool was exposed in my execution context — I searched the available tool surface for Task/Agent/dispatch entry points and none resolved, so the five toolkit agents were not invocable. To compensate I hand-verified their specific scopes: I traced every error path this PR made silent against its callers, checked the new exported type contracts against both the CLI and CDK twins, grepped for stale references to every deleted symbol (isWebhookSecretConfigured, JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY, #368) across .ts/.md, and diffed the test file's negative assertions against the new control flow. B2 and B3 are the findings that surfaced. Treat this section as a known gap in this review rather than a clean bill from those five agents, and re-run them if your environment has them wired.
8. Human heuristics
- Proportionality — pass. The diff deletes more mechanism than it adds: the
isWebhookSecretPlaceholder/isWebhookSecretConfiguredheuristic pair (~70 lines plus 9 tests) goes away, replaced by "always overwrite," which is the simpler and more correct rule.getActiveProjectMappingis a genuine two-call-site extraction, not a speculative abstraction.update-webhook-secretis a real operator need (rotation without re-running OAuth), not a knob. No new factory/engine/indirection. The Scan inlistActiveJiraTenantIdsduplicatesresolveSoleTenantCloudId's logic across the CLI/CDK boundary, which is unavoidable given the package split. - Coherence — concern. The code is coherent; the prose layer is not, and it is spread across four files that now disagree about whether the stack-wide secret is a "fallback," a "synchronized copy," or a back-compat seed (B3, plus ADR-015 in B1). The repo's own term for this is drift, and it is the specific thing AGENTS.md flags for CLI/CDK shared shapes. Also:
cli/src/commands/linear.ts:176still exports the realisWebhookSecretConfiguredfor Linear, so the name now means something in one integration and nothing in the other — fine, but worth a sentence in the Jira construct comment so a reader does not go looking for the Jira twin. - Clarity — pass, with nit 1.
accountSourceas an explicit discriminator beats re-deriving the precedence at the log site. Error messages are actionable and name the real command with real positionals (bgagent jira invite-user <cloudId> <accountId>), and the test at:874-878asserts on that exact string — good, because the previous message pointed atbgagent jira link <code>which the operator could not run without a code.multiTenantWebhookErrorhandles the zero-tenant case with a distinct message rather than a confusing plural. The one clarity regression isjira_actor_display_name(nit 1) and the over-strong "can never accidentally match" claim (B3.2). - Appropriateness — pass. This is the dimension the PR is strongest on. The author verified against real Atlassian behavior on a live deployment (
bgagent-linear-vercel, us-east-1) rather than only against self-written mocks — 200 on a correctly-signed delivery, 401 on a bad HMAC, matching SHA-256 hashes across both secret locations, a real task ID and a real PR, a real unmapped-project silence check, and the other two webhooks explicitly disabled to isolate the test. That directly answers AI001, which is exactly the failure mode webhook-integration code is prone to. The tests assert what the code should do (the negativenot.toHaveBeenCalled()assertions are the load-bearing ones) rather than merely recording current behavior.
To unblock: amend ADR-015 (B1) + mise //docs:sync, add the two setup-guard tests (B2), and fix the three contradictory comments (B3). No implementation changes required — the runtime behavior in this PR is, as far as I can determine, correct.
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict: Request changes — one regression (B1); everything else is a nit
Strong PR overall, and the live acceptance evidence is unusually good. Governance is clean (#709 approved, self-assigned, branch name conforms), all four of my earlier threads are resolved, and I found no blocking security findings — the HMAC path is fail-closed and the stack-wide-verified path correctly refuses to route to a body-supplied cloudId. One genuine logic regression holds this up, and the fix is ~5 lines.
Verified locally on the PR head (27873c99): cdk 128/128 Jira tests across 4 suites, cli 740/740 across 56 suites.
Vision alignment — fits
Directly serves bounded blast radius: the unsolicited-comment bug had ABCA commenting on projects it was never onboarded to. My earlier ADR-015 tenet-trade blocker is discharged — ADR-015:44-68 now documents the single-active-tenant constraint and names the downside ("(-) The Jira channel supports exactly one active tenant because admin-console webhook payloads provide no tenant-routing key").
Blocking
B1 — cdk/src/handlers/jira-webhook-processor.ts:306-313 silently drops @bgagent comments on onboarded projects
The new comment gate returns early when issue.fields?.project?.key is absent. Three facts make this a regression rather than hardening:
- Main's comment path never needed
project.key. Onorigin/mainthe block isif (!cloudId) {...} await handleCommentTrigger(...)— no project lookup at all. handleCommentTriggerstill doesn't use it for routing. It callsresolveTaskByJiraIssue, which keys onjira_issue_identityalone (jira-task-by-issue.ts:59-60) — noprojectKeyparameter.- The PR contradicts its own downstream code. Line 771, inside this same path, already handles the absence:
const projectKey = issue.fields?.project?.key ?? previous.jira_project_key;. That fallback exists precisely because comment payloads don't always hydrateproject— and the new gate returns before it can ever run.
Impact: a reviewer's @bgagent follow-up on a fully-onboarded issue is discarded at logger.info with zero user-visible feedback. That is the same class of defect this PR set out to fix (silent mishandling of legitimate Jira activity), just inverted — and unlike an unsolicited comment, a dropped request is invisible to the user.
The PR's own test at cdk/test/handlers/jira-webhook-processor.test.ts:398 (keeps @bgagent comments without a project key silent) pins this as intended, so it would survive future refactoring. That test should be inverted along with the fix.
Suggested fix — let the prior task prove onboarding, which is what the pre-existing :771 fallback already assumes:
const commentProjectKey = issue.fields?.project?.key;
if (!commentProjectKey) {
// A prior task IS proof of onboarding, and :771 already falls back to
// previous.jira_project_key — so a missing project.key must not be fatal here.
logger.warn('Jira comment issue has no project.key — routing via prior task', {
issue_key: issue.key,
});
} else if (!await getActiveProjectMapping(cloudId, commentProjectKey, issue.key)) {
return;
}If instead you believe Jira always sends project.key on comment_created and the gate is unreachable defensive code, then :771's fallback is dead and one of the two should go — but they can't both be right.
Non-blocking
-
cdk/src/handlers/shared/jira-verify.ts:193-196— the new sentence says'no-per-tenant-secret'"is also the normal path for admin-console payloads, which omit cloudId". Unreachable:jira-webhook.ts:138only enters this function underWORKSPACE_REGISTRY_TABLE && payload.cloudId, so a cloudId-less payload never gets here. It also contradictsjira-webhook.ts:122-124, which correctly lists "no cloudId in body" as a caller-side skip. Misleading in a trust-boundary file. -
cdk/src/handlers/jira-webhook.ts:146-159— the four-variant verify union has no exhaustiveness check, andverifiedinitializes tofalse. A fifth variant added later would compile clean and fall through to stack-wide verification — fail-open on webhook admission. Pre-existing (mirrored inlinear-webhook.ts:137), and the repo already has the idiom atcompute-strategy.ts:110:default: { const _exhaustive: never = result; throw new Error(`Unhandled: ${String(_exhaustive)}`); }
-
cdk/src/handlers/jira-webhook-processor.ts:1028-1044—lookupPlatformUsernow requiresstatus === 'active'where main rejected only'pending'. I checked every writer (jira-link.ts:105→'active',cli/src/commands/jira.ts:1144→'pending'), so no live row breaks. But a row lackingstatusnow yields "isn't linked to a platform user — runbgagent jira invite-user", which is wrong advice for a row that exists. Log the observed status so the two cases are distinguishable. -
cli/src/commands/jira.ts:1005-1013—update-webhook-secrettrusts the registry row'soauth_secret_arnwithout verifyingstored.cloud_id === cloudId.app-setup:878-882has exactly this guard. Two lines, already idiomatic here. -
cli/src/commands/jira.ts:301—synchronizeJiraWebhookSecrets(sm, oauthSecretArn, stackWideSecretArn, stored, webhookSigningSecret)takes three interchangeablestrings. Transposing the two ARNs compiles cleanly and would write the tenant JSON bundle into the stack-wide verifier. Both call sites are correct today; an options object would make that a compile error. -
cli/src/commands/jira.ts:341— "Both secrets remain consistent at the previous value" overstates on a firstsetup, where the two were never equal (the OAuth bundle had nowebhook_signing_secret; stack-wide held the CDK placeholder). Suggest "restored to their prior values; re-run to retry." -
cli/src/commands/jira.ts:308-318— read-modify-write on the OAuth bundle with no optimistic concurrency. A concurrent Lambda token refresh between the read and thePutSecretValue(or the rollback) is clobbered with a stalerefresh_token.app-setup:893-907deliberately re-reads to dodge this; consider the same, or gate onVersionId. -
cdk/test/constructs/jira-integration.test.ts:64— asserts the placeholder marker whose only consumer this PR deleted. Its own comment concedes "no runtime code interprets" it, so the test can no longer fail for any user-visible reason.
Documentation — complete
ADR-015 and JIRA_SETUP_GUIDE.md updated, and I verified the Starlight mirrors are byte-for-byte in sync (ADR 10/5 ↔ 10/5; guide 27/3 ↔ 27/3) — no stale-mirror CI failure. New operator IAM (dynamodb:Scan + the two PutSecretValue targets) is documented.
Tests & CI
Coverage is genuinely good, not just voluminous. The unmapped-project silence uses real negative assertions (not.toHaveBeenCalled() on resolveTaskByJiraIssue, createTaskCore, and reportIssueFailure) with a live @bgagent update the README too mention in the fixture — so they'd fail if the guard were removed. ConsistentRead: true is asserted at all ~8 call sites rather than trusted. The rotation rollback branches are covered (cli/test/commands/jira.test.ts:1667 and :1692); I ran both and they pass.
Bootstrap synth-coverage: not applicable. The jira-integration.ts diff is comment-only — no new CFN resource types and no IAM grant changes. Rotation runs under the operator's CLI credentials against a CLI-owned secret, so ADR-002 / #350 correctly doesn't trigger and leaving BOOTSTRAP_VERSION at 1.3.0 is right.
CI: only the three CodeQL Analyze jobs pending at review time.
Review agents run
code-reviewer, silent-failure-hunter, pr-test-analyzer, type-design-analyzer, comment-analyzer, plus a security review (IAM / HMAC / secret-handling all in scope). None omitted.
Two agent findings I verified as FALSE and am deliberately not passing on:
- "No rollback test coverage" — the agent grepped the working tree (checked out on a different branch) instead of the PR blob. The tests exist and pass.
- "Registry-drop path and missing processor DLQ introduced here" — both are pre-existing on
main(git show origin/main:shows the samedropping eventwarn and no DLQ onWebhookProcessorFn). Real, but a separate issue rather than a blocker on this PR — happy to file it.
Human heuristics
- Proportionality — pass. 292 lines in
jira.tsfor one command is earned: rotation, rollback, scan pagination, five validation branches. - Coherence — mostly pass; nit 1 is where the "fallback → primary" vocabulary rename didn't fully land.
- Clarity — concern at
:306(B1): the silence is indistinguishable from a genuine drop. - Appropriateness — pass, and notably strong. Live acceptance on a real deployment (signed → 200, invalid HMAC → 401, matching secret SHA-256 hashes, replay idempotency, zero comments from unmapped project
TG) is verification against real API behavior, not self-written mocks.
|
Filed the two pre-existing findings my review deliberately excluded as #734 — webhook processors silently discard admitted events (no DLQ, and a transient DynamoDB failure is misread as "tenant not onboarded"). To be explicit: this is not a blocker on your PR. I verified both halves on It does touch the same file you're editing, so worth knowing it exists. It also affects Linear and Slack identically, and #284 already set the precedent by fixing exactly this for the GitHub processor. B1 in my review stands on its own and is unrelated to #734. |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
1. Verdict
Approve — all five prior blocking items are resolved in the current head (c2723095), the B1 comment-admission regression is fixed with the tests correctly inverted, and I found no blocking security findings on the HMAC/admission/rotation paths. Remaining items are nits I am explicitly not holding the PR for.
This is a re-review against origin/main tip c927a209 (#544 admission queue). The PR is up to date with that tip.
2. Prior blocking claims — RESOLVED / STILL OPEN
B1 (2026-08-07) — comment gate silently dropped @bgagent on onboarded projects → RESOLVED
cdk/src/handlers/jira-webhook-processor.ts:306-313 is now conditional, exactly the shape I asked for and then some:
const commentProjectKey = issue.fields?.project?.key;
if (commentProjectKey && !await getActiveProjectMapping(cloudId, commentProjectKey, issue.key)) {
return;
}
await handleCommentTrigger(payload, issue, cloudId, commentProjectKey);A missing project.key is no longer fatal. The author went stricter than my suggestion, which I verified is the better call: handleCommentTrigger (:594, new verifiedProjectKey? param) resolves the prior task, recovers channel_metadata.jira_project_key, and performs a fresh getActiveProjectMapping consistent read before any feedback or task creation (:628-651). That closes the hole my own suggestion left open — under my version, a comment on an offboarded project with no project.key would have been admitted on the strength of a stale prior task. The #709 site-wide admission guarantee holds in both branches.
The contradiction I flagged with :788 (issue.fields?.project?.key ?? previous.jira_project_key) is gone: that fallback is now reachable and load-bearing on exactly the path it was written for.
Tests inverted as requested — the old keeps @bgagent comments without a project key silent at :398 is replaced by three cases that pin the tri-state:
:398routes via the active prior-task project (asserts thecloud-1#ENGconsistent read,createTaskCorecalled once, andchannelMetadata.jira_project_key === 'ENG'):426no prior task → silent,expect(ddbSend).not.toHaveBeenCalled():438prior project removed → silent, mapping read still asserted
B1 (2026-08-05) — undocumented ADR-015 tenet trade → RESOLVED
docs/decisions/ADR-015-jira-integration.md carries a **Revised:** 2026-08-05 by #709 header, §Multi-tenant signature binding is retitled Admin-console webhook tenant binding and rewritten, all three stale multi-tenant assertions (old :44, :49-51, :63) are gone, and the downside is named explicitly rather than buried: (-) The Jira channel supports exactly one active tenant because admin-console webhook payloads provide no tenant-routing key. The rotation consequence now points at update-webhook-secret instead of setup.
B2 — setup multi-tenant guard had no test → RESOLVED
Both tests I asked for exist. cli/test/commands/jira.test.ts:1158 (refuses a second active tenant before writing OAuth or registry state) asserts the ordering invariant, not just the throw:
expect(ddbSend).toHaveBeenCalledTimes(1);
expect(ddbSend.mock.calls[0][0]).toBeInstanceOf(ScanCommand);
expect(ddbSend.mock.calls.some(([c]) => c instanceof UpdateCommand)).toBe(false);
expect(smSend).not.toHaveBeenCalled();
expect(promptSecretMock).not.toHaveBeenCalled();That is the half-onboarded-tenant regression pinned. The empty-scan first-install case is covered too (:493 for rotation's fatal treatment, and the otherActiveTenantIds filter path in setup), so the asymmetry I worried about is now documented by tests on both sides.
B3.1/B3.2/B3.3 — three contradicting comment sites → RESOLVED
cli/src/jira-oauth.ts:113-124now matches its CDK twin (cdk/src/handlers/shared/jira-oauth-resolver.ts:93-102) — both describe the synchronized-copy model. The CLI/CDK shared-shape drift is closed.cdk/src/constructs/jira-integration.ts:181-195reframed to "non-operational initial value... Setup unconditionally replaces" + "No runtime code interprets the marker key." No reader will go hunting for the deleted recognizer.cdk/test/constructs/jira-integration.test.ts:62-64keeps the assertion but itswhyis rewritten; the #368 recognizer rationale is gone.
Non-atomicity docstring nit → RESOLVED
cli/src/commands/jira.ts:298-300: "The writes are not atomic. If the process exits between them, re-run the command to converge both copies." Read-modify-write race correctly deferred to #724 (open, with concurrency-safe acceptance criteria) rather than scope-creeping this PR — and I agree with the author that ClientRequestToken gives idempotency, not CAS.
jira_actor_display_name PII nit → RESOLVED
Gone. grep display_name: cdk/src/handlers/ is clean of Jira handler hits; only opaque accountIds, jira_account_source, and jira_identity_lookup_key remain (:384-392).
3. Security review — no blocking findings
I ran the security review manually against the PR worktree diff (the security-review skill harness resolved against the wrong checkout and no sub-task tool was available in this workflow context — see §7). Domain-specific verification:
- Constant-time compare —
jira-verify.ts:130usescrypto.timingSafeEqual; thesha256=prefix is stripped first, and a length-mismatch throw is caught and returnsfalse(not an exception leak). - Verify before parse / side effect —
jira-webhook.ts:110-122rejects a missing signature 401 beforeJSON.parse; the parse is needed only to select the tenant secret, and HMAC is computed over the rawevent.body, never a re-stringified object.isBase64Encodedis rejected 400 rather than silently HMAC'ing the wrong bytes (:105). No DDB write or Lambda invoke happens before verification — the dedupPutItem(:225) and processorInvoke(:242) are both downstream of the 401 gates. - Fail-closed on empty/malformed secret —
isUsableHmacSecretguards in two places (getJiraSecret:79,verifyJiraSignature:121), soHMAC('', body)is never forgeable.getRegistryRowStrict/getOauthSecretStrictthrow on infra error so a DDB throttle cannot silently downgrade a per-tenant-secured tenant to the stack-wide verifier — this PR addsConsistentRead: trueto those reads, which strengthens it further (a rotation is no longer visible-eventually). mismatch/revokedare terminal —jira-webhook.ts:146-157returns 401 with no alternate verification. Onlyno-per-tenant-secretreaches the stack-wide verifier, andverifiedstartsfalse.- Rotation window is bounded and does NOT accept both secrets —
verifyJiraRequest:166-180tries the cached value, then force-refreshes once and retries only iffresh !== cached. So the old secret is accepted only until the first delivery after the write, and never alongside the new one. That is the right trade for a webhook (Atlassian sends one secret at a time); the guide correctly tells the operator to keep the webhook disabled untilupdate-webhook-secretsucceeds. - Replay — dedup
PutItemwithattribute_not_exists(comment id forcomment_created, delivery timestamp for issue events) plus the advisory 1h freshness window with one-sided skew tolerance (isWebhookTimestampFresh:151). A timestamp-less delivery is logged, not silently accepted-as-fresh. - Tenant-steering — unchanged and still correct: a stack-wide-verified delivery ignores
payload.cloudIdand binds toresolveSoleTenantCloudId(), which returnsundefined(→ drop) on zero or >1 active tenants (:284-296,:131-161). - IAM — no grant changes.
cdk/src/constructs/jira-integration.tsis comment-only in this diff. Rotation runs under the operator's CLI credentials against CLI-owned secrets, so no Lambda role widens. Bootstrap synth-coverage: not applicable — no new CFN resource types; leavingBOOTSTRAP_VERSIONat1.3.0is correct per ADR-002/#350. The new operator-side IAM (dynamodb:Scan, the twoPutSecretValuetargets) is documented atJIRA_SETUP_GUIDE.md:160. - Composition with #544 — verified.
handleCommentTriggertreatscreateTaskCore200 as idempotent replay and 201 as accepted; #544 queues inside the orchestrator, after the 201, and reports back throughnotifyJiraOnConcurrencyCap(orchestrate-task.ts:545-570) which readschannel_metadata.jira_cloud_id/jira_issue_key. Both are stamped bybuildIterationChannelMetadataon the comment path, so a queued comment-triggered iteration gets its⏸️ queuedfeedback. No conflict.
4. Non-blocking nits (carried forward, unchanged from my last review — none are merge blockers)
cdk/src/handlers/shared/jira-verify.ts:195— the "also the normal path for admin-console payloads" sentence is still unreachable prose:jira-webhook.ts:138only calls this function underWORKSPACE_REGISTRY_TABLE && payload.cloudId, so a cloudId-less payload never arrives here. Minor, but it is a trust-boundary file.cli/src/commands/jira.ts:341— "Both secrets remain consistent at the previous value" still overstates on a firstsetup, where they were never equal.cdk/src/handlers/jira-webhook.ts:146-159— the four-variant verify union still has noneverexhaustiveness gate; a fifth variant would compile clean and fall through to stack-wide. Pre-existing and mirrored inlinear-webhook.ts; the idiom exists atcompute-strategy.ts:110.cli/src/commands/jira.ts:1005-1011—update-webhook-secretstill doesn't assertstored.cloud_id === cloudIdthe wayapp-setup:878and:903do. Two lines. Low risk (the sole-active-tenant scan already pins the tenant), but the guard is idiomatic here.cli/src/commands/jira.ts:301-307—synchronizeJiraWebhookSecretsstill takes three positional interchangeablestrings; transposing the two ARNs would write the tenant JSON bundle into the stack-wide verifier and compile cleanly. Both call sites are correct today.cdk/src/handlers/jira-webhook-processor.ts:1053—lookupPlatformUserrequiresstatus === 'active'; a row lackingstatusyields "isn't linked" advice for a row that exists. Logging the observed status would separate the two cases. (I re-verified every writer:jira-link.ts:105→active,cli/.../jira.tsinvite →pending, so no live row breaks.)cdk/test/constructs/jira-integration.test.ts:64— the placeholder-marker assertion now admits in its own comment that nothing interprets the key, so it can't fail for a user-visible reason. Harmless.
5. Documentation — complete
ADR-015 (revision header, retitled section, named downside, #709 reference) and JIRA_SETUP_GUIDE.md (one-active-tenant callout, rotation command + operator IAM, offboarding-silence bullet, new "Linking succeeds but a trigger says the Jira user is unlinked" troubleshooting section) both updated. cli/README.md documents the new command.
Mirror sync verified empirically, not just by line counts: I re-ran node docs/scripts/sync-starlight.mjs in the worktree and git status --porcelain docs/ came back empty. No stale-mirror "Fail build on mutation" risk.
Backing issue #709 carries approved. Branch fix/709-jira-webhook-admission conforms. Follow-up #724 filed for the deferred OAuth-bundle CAS work.
6. Tests & CI
CI green on c2723095 — CodeQL (3 analyzers), secrets/deps/workflow scan, dead-code advisory, PR-title validation, and build (agentcore) all pass. Note the three CodeQL jobs that were merely pending at my last review have now passed.
I could not re-run the suites locally: this review worktree has no node_modules, and the sibling checkout I linked carries TypeScript 5.9.3 against the repo's ^6.0.3 pin, which fails with TS5103: Invalid value for '--ignoreDeprecations'. That is a local toolchain artifact, not a PR defect — the same suites passed for me at 27873c99 (cdk 128/128 Jira across 4 suites, cli 740/740 across 56), and CI's build (agentcore) covers the head. I verified the new/changed tests by reading them, and the assertions are substantive (real negative assertions on resolveTaskByJiraIssue/createTaskCore/reportIssueFailure, ConsistentRead: true asserted at each call site rather than trusted, rotation rollback branches at :1668 and :1694).
Bootstrap synth-coverage: not applicable (see §3).
7. Review agents run
Process disclosure: the pr-review-toolkit agents (code-reviewer, silent-failure-hunter, type-design-analyzer, comment-analyzer, pr-test-analyzer) could not be invoked — no Agent/sub-task dispatch tool is exposed in this review context (only the Task* list tools resolved; a ToolSearch for Agent returned no match). The security-review skill launched but its harness resolved against a different, dirty checkout and reported that working tree instead of PR #710's diff, and it too needs sub-tasks it cannot spawn here.
So the Stage 3 automated sweep is omitted for tooling reasons, not by choice — stating it plainly rather than implying coverage I don't have. To compensate I hand-applied each agent's rubric to the diff: error-handling/fail-open paths (the fail-closed HMAC audit and the getActiveProjectMapping silence semantics in §3), the new verifiedProjectKey? parameter and the three-positional-string signature (nit 5), comment-vs-code accuracy across all six prior contradiction sites (§2 B3, nit 1), and test-coverage gaps (§2 B1/B2, §6). The prior review round's full agent output — which I ran at 27873c99 — remains valid for everything except the ~30 lines c2723095 changed, and those 30 lines are the subject of §2 B1.
8. Human heuristics
- Proportionality — pass. The B1 fix is ~20 lines plus a revalidation branch; it did not grow an abstraction.
update-webhook-secret's ~290 lines injira.tsremain earned (rotation, rollback, scan pagination, five validation branches). - Coherence — pass. The "fallback → verifier/primary" vocabulary rename now lands consistently across CDK handlers, the CLI twin, ADR-015, and the guide. Only residue is nit 1.
- Clarity — pass, and materially better than last round. My
:306concern is discharged: the three comment outcomes (verified project / recovered-and-revalidated project / silent) are now distinguishable in both code and logs, and the recovery path logs atwarnwith the recoveredproject_key. - Appropriateness — pass, notably strong. Live acceptance on a real deployment (signed → 200, invalid HMAC → 401, matching secret SHA-256 hashes across both copies, replay idempotency, zero comments from unmapped project
TG) is verification against real Atlassian behavior, not self-written mocks — the AI001 failure mode this repo cares about.
Nice work on the B1 fix specifically: going stricter than the suggested patch, and catching that a prior task alone shouldn't re-admit an offboarded project, is the right instinct.
| * has no `webhook_signing_secret`. Caller should fall back to the | ||
| * stack-wide secret for back-compat with single-tenant installs. | ||
| * has no `webhook_signing_secret`. Caller should use the stack-wide | ||
| * verifier. This is also the normal path for admin-console payloads, which |
There was a problem hiding this comment.
Nit (non-blocking, carried from my last review). This sentence is still unreachable prose.
jira-webhook.ts:138 only calls verifyJiraRequestForTenant under WORKSPACE_REGISTRY_TABLE && payload.cloudId, so a payload that omits cloudId never reaches this function — it is skipped at the caller, which jira-webhook.ts:122-124 already documents correctly ("(b) no cloudId in body").
Worth a small edit because this is a trust-boundary file: a reader reasoning about which deliveries can return 'no-per-tenant-secret' will conclude admin-console payloads do, and they don't. Suggest dropping the sentence, or reattributing it:
Admin-console payloads omit
cloudIdand are skipped by the caller before reaching this function; they go straight to the stack-wide verifier.
| + '`secretsmanager:GetSecretValue` on this ARN.', | ||
| 'Failed to update the stack-wide Jira webhook secret, but restored the tenant bundle ' | ||
| + `to its previous value: ${err instanceof Error ? err.message : String(err)}. ` | ||
| + 'Both secrets remain consistent at the previous value; rotation can be safely retried.', |
There was a problem hiding this comment.
Nit (non-blocking, carried from my last review). "Both secrets remain consistent at the previous value" is true for a rotation but overstates on a first setup, where the two copies were never equal: the OAuth bundle had no webhook_signing_secret at all and the stack-wide secret still held the CDK-generated placeholder JSON.
After a rollback in that case the copies are restored-but-divergent, which is fine (the guide says keep the webhook disabled until setup succeeds) — the message just shouldn't promise consistency.
| + 'Both secrets remain consistent at the previous value; rotation can be safely retried.', | |
| + 'Both secrets were restored to their prior values; rotation can be safely retried.', |
|
|
||
| const sm = makeClient(SecretsManagerClient, { region }); | ||
| const oauthSecret = await sm.send(new GetSecretValueCommand({ SecretId: oauthSecretArn })); | ||
| const stored = parseStoredJiraOauthToken(oauthSecret.SecretString, oauthSecretArn); |
There was a problem hiding this comment.
Nit (non-blocking, carried from my last review). update-webhook-secret still trusts the registry row's oauth_secret_arn without confirming the bundle it points at actually belongs to cloudId.
app-setup guards exactly this at :878 (and again at :903 after its network round trip):
if (stored.cloud_id !== cloudId) {
throw new CliError(`Jira OAuth secret cloud_id '${stored.cloud_id}' does not match requested tenant '${cloudId}'.`);
}Risk is low here — the sole-active-tenant scan at :995 already pins the tenant, so a mismatch implies a corrupted registry row rather than an attack. But this write puts a signing secret into that bundle, so a stale/mis-pointed ARN would seed the wrong tenant's verifier. Two lines, already idiomatic in this file.
2 participants
Fix Jira admin-console webhook verification, admission, and user-link diagnostics reported by a customer deployment.
Area
cdk— infrastructure, handlers, constructsagent— Python runtime / Docker imagecli—bgagentclientdocs— guides or design sources (docs/guides/,docs/design/)tooling— rootmise.toml, scripts, CI workflowsRelated
Closes #709
Changes
bgagent jira update-webhook-secret <cloud-id>for rotation without repeating OAuth, including rollback if the second secret write fails.cloudId.comment_created, from unmapped or removed Jira projects completely silent.Verification
Local automated checks:
mise run build: passed, including 173 CDK suites / 3,477 tests.linear-vercelfull build: passed (3,626 CDK, 670 CLI, and 1,421 agent tests, plus docs, synth, compile, and lint).linear-vercelAgentStack tests: 57 passed.Live acceptance checks on
bgagent-linear-vercelin account336879875486,us-east-1:01KZ6ZGMHP9GPY92SZA8KEAZVMand chore(validation): add Jira integration e2e test validation note ayushtr-aws/abca-testing#21.10122with success, cost, turn count, duration, and PR link.TGproduced zero Jira comments and zero tasks; logs recorded a silent onboarding skip.SCRUMissue without trigger criteria produced zero Jira comments and zero tasks.backgroundagent-devandbackgroundagent-dev2) were disabled during isolation testing and received neither test event.All current CI checks pass. This PR changes no dependency or lockfile.
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.