chore(jira): non-blocking review nits deferred from #895 — preview claim-latch recovery, Amplify receiver observability, ADF/param typing

[isadeks]

Component

CDK Jira adapter and screenshot pipeline — cdk/src/handlers/shared/jira-deployment-preview.ts, jira-preview.ts, jira-feedback.ts, github-deployment-status.ts, cdk/src/handlers/iteration-heartbeat-sweep.ts, cdk/src/handlers/shared/types.ts

Describe the feature

Non-blocking findings deferred from the review of #895 (Jira deployment-preview feedback). The four review blockers were fixed in 1f2e37ab — bounded delivery with deadline cancellation, safe convergence outcomes, mutation-sensitive race coverage, and corrected routing docs — and the items below were deliberately left out of that round. All line numbers are as of 1f2e37ab; the symbol names are the anchors.

The first item is the one with a customer-visible wrong outcome; the rest are drift-prevention, observability, and type ergonomics.

• 1. The preview claim latch conflates definite failure with uncertain failure, and never releases. jira-deployment-preview.ts:124-160. jira_preview_claimed is a one-way latch — nothing in cdk/src removes it. On a Jira 429/503 the response was received, so nothing was created, yet the claim stays set and the preview is permanently lost for that task: every later delivery takes the ConditionalCheckFailedException branch, finds no comment id, logs one warn, and returns. writeComment already distinguishes "Jira answered non-2xx" (jira-feedback.ts:284-286) from its catch (genuinely uncertain), and JiraPostResult collapses both into { ok: false, retryable }. The documented trade — "Jira may have committed before a transport timeout" — is correct for the uncertain half only. Add a third outcome state and release the claim (REMOVE jira_preview_claimed, conditional on attribute_not_exists(jira_preview_comment_id)) on definite failure.
• 2. POST succeeds but the comment-id write fails → the issue shows a stale screenshot forever. jira-deployment-preview.ts:150-158. State becomes claimed ✓ / comment exists in Jira ✓ / id unrecorded ✗, so every subsequent deploy hits the !commentId branch at :141-146 and gives up. The Jira issue keeps the first commit's screenshot indefinitely — worse than showing nothing, because a stale screenshot is indistinguishable from a current one. Write the claim and the comment id in one conditional update where possible; failing that, this branch and item 1's should log at error with an error_id from constants/errorIds.ts rather than warn, since "a comment exists in Jira that we can never update again" is an operator-actionable inconsistency.
• 3. An empty-string comment id is persisted as if valid. jira-feedback.ts:718 returns { ok: true, commentId: '' } (pre-existing, deliberately, so a caller does not create a duplicate). jira-deployment-preview.ts:157 writes that verbatim, and the duplicate branch at :141 accepts it because typeof '' === 'string'. updateIssueCommentAdf then fails its /^\d+$/ guard forever, logging "Refusing to update Jira comment with an invalid id" — which misdescribes the cause: the id is not corrupt, it was never captured. Only persist a non-empty id, and route the empty case to item 2's "posted but unaddressable" path.
• 4. The heartbeat sweep drops the Jira result on the floor. iteration-heartbeat-sweep.ts:152-161 — if (result.ok) edited += 1; continue; with no else. A Jira tenant whose token has been revoked produces a sweep reporting edited: 0, indistinguishable from a sweep with nothing eligible, and with no per-task line correlating the failure (unlike the non-Jira path, whose failures land in the catch with task_id). Mirror that catch, and count attempts separately from successes.
• 5. jira_preview_claimed?: boolean contradicts its own DynamoDB predicate. types.ts:335. The condition is attribute_not_exists(jira_preview_claimed), so the invariant is presence, not truth — but the type admits false, which a future writer would read as "not claimed" while writing it permanently blocks delivery. One-character fix: readonly jira_preview_claimed?: true;. Better still, collapse it with jira_preview_comment_id into one field so {claimed: false, comment_id: 'x'} stops being representable.
• 6. The ADF document is the one structured outbound payload in the repo that is not typed. It is Record<string, unknown> | null at every boundary — buildAdfDocument, postIssueCommentAdf, updateIssueCommentAdf, TaskRecord.jira_iteration_status.body — while the repo already types inbound ADF (jira-adf.ts:41 AdfNode) and the closest outbound analogue in full (slack-blocks.ts:27-88 SlackBlock/SlackMessage). The cost is visible at jira-preview.ts:71, which needs two casts and a ?? [] to concatenate two documents; with an AdfDocument interface that line is cast-free, and a non-array content (from an older or partially-written jira_iteration_status) stops being representable — today it spreads characters into the posted body.
• 7. Positional string runs in the two new public functions. deliverJiraDeploymentPreview now takes 9 parameters and updateJiraIterationComment 8. (screenshotUrl, previewUrl) swapped compiles, passes the allowlist, and is caught by no test — the labels simply point at the wrong URLs. (tableName, registryTableName) swapped cross-wires the task table and the registry and fails silently (ValidationException, swallowed). updateJiraIterationComment gained six call sites in feat(jira): surface deployment preview feedback (#698) #895, so this gets more expensive with each one. An options object, or three one-line brands (TaskTableName, TaskId, JiraCommentId), makes every swap a compile error.
• 8. normalizeAmplifyPreviewCheck returns a bare null for ~19 distinct rejection reasons, with no log anywhere. github-deployment-status.ts:67-104; the receiver returns { skipped_check: true } at github-webhook.ts:106. Compare the neighbours: a dedup hit logs info with the key, an unrelated event type logs info with the type, a malformed deployment_status logs warn and returns 400. So a dropped Amplify preview is strictly less observable than a duplicate one. If AWS renames the check, changes the app-slug shape, or moves previews to a custom domain, the entire Amplify path dies permanently and silently behind a green webhook delivery page. Return a discriminated result carrying the reason, log it, and the nosemgrep: ts-silent-success-masking suppression at :88 should become removable.
• 9. Existing Amplify operators are silently broken by the environment filter. The normalizer hardcodes environment: 'Preview' (github-deployment-status.ts:101) while the receiver still applies SCREENSHOT_TARGET_ENVIRONMENT (github-webhook.ts:130). An operator following the previous guide has that set to a branch name; they subscribe to Check runs as the new guide instructs and every Amplify check drops with only skipped_environment in the HTTP response body — while setting it to Preview breaks their existing branch-deploy path, because the filter is a single fixed string, so the two Amplify modes are mutually exclusive. Either exempt the check_run path (check name + aws-amplify-console app owner + pr-N.<app-id>.amplifyapp.com hostname already prove it is a PR preview) or set the synthetic environment to targetEnv; either way add a migration note for operators currently on a branch-name value.
• 10. The validated PR number is discarded, then re-derived from the SHA. github-deployment-status.ts:90-103 proves pr-<n> matches a listed pull request whose head.sha equals check.head_sha, then drops preview[1]; github-webhook-processor.ts:195 asks GitHub again by SHA. When one SHA heads two open PRs (retargeted base, stacked series, duplicated PR), the screenshot can be posted onto a different PR — and persistScreenshotUrl then resolves the task from that PR's headRefName, so the whole Jira/Linear delivery chain follows the wrong task. Green, plausible, wrong. Carry the validated number on the normalized payload and prefer it; if the SHA lookup disagrees, log error and skip rather than guess.
• 11. Module naming. jira-preview.ts's dominant export is updateJiraIterationComment, imported by four handlers — it is the iteration-comment writer, not a preview module. Sitting beside jira-deployment-preview.ts (the delivery orchestrator) the two are near-indistinguishable at an import site; jira-iteration-comment.ts would read truer. Its module-private render() is also very generic for a shared module.
• 12. Two small consequences of the 1f2e37ab fix itself. (a) jira-deployment-preview.ts:48 computes document and throws at the top of the try, so a disallowed screenshot URL now aborts the iteration path too — where the document is never used, since updateJiraIterationComment re-renders from durable state. A misconfigured SCREENSHOT_PUBLIC_HOST would skip the entire status-comment update, including completion metrics and the PR link, rather than just omitting the preview block; moving the check into the standalone branch keeps it fail-closed without that side effect. (b) :104 tightened task.head_sha && task.head_sha !== sha to task.head_sha !== sha, which correctly closes a hole but means iteration records written before head_sha was persisted now receive no preview at all — worth confirming that is intended and, if so, noting it.
• 13. Residual test gaps. Jira 4xx vs 5xx classification is not pinned at the jira-preview layer (both collapse into one warn); screenshot.jira_missing_registry is unexercised; Number.isSafeInteger(check.id) has no non-safe-integer case; and the 400 Invalid webhook payload branch is unreachable in tests because the 'null'/'[]' bodies are sent as check_run and return skipped_check first — a deployment_status event with body null would exercise it.

Use case

Items 1-3 are the same failure class #698 set out to eliminate: a preview that cannot be delivered is accepted silently and resurfaces as either nothing at all or a confidently-wrong artifact on a customer's Jira issue. Items 8-10 are the Amplify receiver's observability and correctness edges — the path has no signal when it stops working. The rest keep the invariants #895 established enforceable rather than remembered.

Proposed solution

Items 4, 5, 11, 12a and 13 are small and can land as one cleanup PR. Items 1-3 belong together — they are one state machine (claim / posted / unaddressable) and are best fixed as a unit with the outcome type widened. Items 6 and 7 are mechanical but touch many call sites; worth doing before updateJiraIterationComment gains a ninth parameter. Items 8-10 concern the Amplify check_run receiver, which is tracked separately by #900 — they are recorded here to keep the review's findings in one place, and could equally be folded into that issue.

Other information

• Raised during review of feat(jira): surface deployment preview feedback (#698) #895, which is otherwise ready — the four review blockers were fixed in 1f2e37ab.
• Already fixed in 1f2e37ab, do not redo: deadline cancellation and bounded lookups (JiraPreviewBudget, MAX_CANDIDATE_READS, POST_CAPTURE_RESERVE_MS 8s→30s, screenshot.jira_budget_exhausted / screenshot.jira_lookup_limited); convergence returning ok: true after successful PUTs plus jira.preview.task_missing for an absent record; LookupResult on persistScreenshotUrl so a lookup failure skips both channel deliveries; jiraPreviewDocument returning null and the standalone path rejecting it before claiming; the page-limit throw becoming a break; mutation-sensitive race coverage (resolution-order assertions, PUT call counts, terminal: true at the fan-out and reconciler call sites, a literal ConditionExpression assertion); the PutSecretValue justification comment and the enumerated AwsSolutions-IAM5 reason; the routing sentence in JIRA_SETUP_GUIDE.md plus the full emitted-event list; and the stale preservePreview, parseMarkdownRuns subset, TASK_TABLE-unset, and ALL_OLD comments.
• Related: feat(screenshots): support Amplify PR preview check runs #900 (Amplify PR preview check runs), refactor(handlers): encode failure in best-effort lookups — 13 silent-success-masking sites (#756 Cat 2) #863 (the best-effort LookupResult convention these paths should follow), feat(jira): mature comment-triggered iteration feedback in place #697 (maturing Jira iteration status comments).