feat(screenshots): preserve Amplify preview PR routing (#900) - #902
Conversation
Keep branch environment filters for deployment statuses, log Amplify rejection reasons, and carry validated PR identity through screenshot and Jira/Linear delivery. Fixes #900 Co-Authored-By: Codex <noreply@openai.com>
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict: request changes
Three mechanical items. The core design is right, and the headline claim is genuinely load-bearing rather than merely mirrored — I forced the fallback by stubbing validatedPrNumber = undefined at the top of findPullRequestForSha and 8 tests fail, including both the Jira and Linear routing cases, so the shared-SHA fix is proven. What needs another pass is the processor's new rejection path, which feeds terminal outcomes back into a retry loop built for a transient race, and one surviving mutation that happens to guard every non-Amplify provider.
Governance is clean: #900 carries approved + P1, the branch matches the convention, the diff stays inside the approved scope, and bootstrap policies are correctly untouched (no new CloudFormation resource types — the only constructs/ change is a doc comment, so BOOTSTRAP_VERSION rightly stays put).
Blocking
1. Terminal Amplify rejections are retried as if they were the "PR not created yet" race — cdk/src/handlers/github-webhook-processor.ts:655-673, with :567 and :212.
findPullRequestForSha returns OpenPr | null, so findPullRequestForShaWithRetry cannot distinguish pr_not_open (permanent) from "GitHub has not linked the PR yet" (retryable). The arithmetic makes this unconditional: PR_LOOKUP_RETRY_DELAYS_MS is [0, 5s, 10s, 20s] against a prLookupBudget of 110s - 30s - 15s = 65s, so all four attempts always run. Instrumenting the new rejects a changed or malformed PR case confirms 4 fetches to /pulls/42 and 4 identical screenshot.amplify_pr_rejected warns per event, for every one of the six fixtures including state: 'closed'.
The scenario is routine rather than exotic: an Amplify preview build takes minutes, so a PR that merges or receives another push mid-build is the common case, not the edge. That benign outcome now burns ~35s of billed idle Lambda, over-counts any reason-code metric filter 4x, and then terminates at logger.error('No open PR found for SHA after retries...', { error_id: 'SCREENSHOT_PR_LOOKUP_EXHAUSTED' }) — an ERROR whose own comment says it exists because it is "the shape of a systematic break (deploy-without-PR, token regression, GitHub outage)". A merged PR is none of those, and the specific reason already known one frame down is not carried into it. I checked and no CloudWatch metric filter is wired on error_id anywhere in cdk/src/ yet, so today's cost is error-log noise and latency rather than a page — but the comment states the alarm intent, so it will page once wired.
Related, and currently uncovered: GET /pulls/{n} 404s when the PR is deleted or the token loses access, where commit-pulls returned 200 []. That lands on !res.ok (:633-640), returns null, retries four times, and emits no screenshot.amplify_pr_rejected at all — so the reason code the new guide tells operators to look for is absent for exactly that case.
Suggested fix — the same shape this PR just introduced one file over:
type PrLookup =
| { ok: true; pr: OpenPr }
| { ok: false; reason: string; terminal: boolean };Break out of the retry wrapper on pr_number_mismatch, pr_not_open, head_sha_mismatch, missing_head_ref, and a 404; log those at warn with the reason instead of SCREENSHOT_PR_LOOKUP_EXHAUSTED. Retry only the transient shapes (fetch error, 5xx, non-JSON).
2. No negative assertion that the deployment_status path omits validated_pr_number — cdk/src/handlers/github-webhook.ts:200, test at cdk/test/handlers/github-webhook.test.ts:212-222.
The happy-path test asserts only expect(decoded.raw_body).toBeDefined(). This mutation leaves 85/85 green:
...{ validated_pr_number: normalized
? normalized.prNumber : 1 },That silently breaks every Vercel, GitHub Actions, and Netlify screenshot — the processor would fetch /pulls/1 and reject on pr_number_mismatch — and the suite would not notice. A range edit like this one needs a negative assertion, not a presence check:
expect(decoded).toEqual({ raw_body: expect.any(String) });Better still, one test that actually crosses the wire. cdk/test/handlers/github-webhook-processor.test.ts:500-519 hand-rebuilds { raw_body, validated_pr_number } as a second independent copy of the receiver's construction at github-webhook.ts:199-200, so the key name is asserted separately on each side and the contract holds by coincidence — which is precisely why the mutation above survives. Feeding the receiver's emitted Payload straight into the processor handler kills the whole class (it needs its own spec file, since the two suites have incompatible jest.mock sets):
await receiverHandler(event(amplifyBody(),
{ 'X-GitHub-Event': 'check_run' }));
const p = JSON.parse(new TextDecoder().decode(
lambdaSend.mock.calls[0][0].input.Payload));
await processorHandler(p);3. The migration paragraph does not state the behaviour change it causes for existing operators — docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md:28.
GitHubScreenshotIntegration is instantiated unconditionally in cdk/src/stacks/agent.ts:1975, with a comment that says so explicitly ("Default-on ... no opt-in flag"). The test this PR deletes — Amplify preview checks respect the configured environment filter — proves that SCREENSHOT_TARGET_ENVIRONMENT was the only in-band lever that suppressed Amplify PR-preview capture, because the normalizer forced environment: 'Preview'. So an operator who set it to a branch name in order to stop PR previews from being captured will, on the next deploy, start publishing them to the public-read CloudFront URL with no config change on their side. buildScreenshotKey's own docstring notes that previews can render customer data.
The paragraph currently frames this purely as a convenience ("You do not need to change it to Preview") and never says that capture starts for previews the operator was excluding. One sentence fixes it. An amplifyPreviewChecks?: boolean opt-out prop would be the fuller answer, but that is a new construct prop and therefore ask-first scope — worth parking rather than widening this PR.
Non-blocking
Trivial but worth doing in this pass: the reflowed comment at cdk/src/handlers/github-webhook.ts:123-134 lost its topic sentence. Replacing "Filter to a configured environment name." leaves // Defaults to \Preview`at:125with no subject, and the "because Vercel labels per-PR deploys that way" clause at:126` attaches to nothing. "legacy branch filters" is also an internal reference a reader cannot resolve from the file. This is the paragraph that explains the PR's central behaviour change, so it is the one most worth reading cleanly.
The rest, roughly by value:
- Case-sensitive SHA compare, now with no fallback —
:665.head_shais validated by/^[0-9a-f]{40}$/i, so uppercase hex survives intopayload.sha, while GitHub returnshead.shalowercase, givinghead_sha_mismatchand no capture. The old commit-pulls path tolerated this via?? openPulls[0]; the Amplify path deliberately has none, so the same input is now fatal rather than merely unpreferred. Low probability, one-line fix: lowercasehead_shaonce in the normalizer. - Reason codes should be one exported union —
:657.reject(reason: string)uses ad-hoc literals overlappingAmplifyPreviewRejectionReason;head_sha_mismatchmeans two different things across the two families (the check's ownpull_requests[].head.shavs. the live PR head), andinvalid_preview_pr_number/invalid_pr_numbername one concept twice. A typo in a plain-stringliteral breaks a metric filter with no compile error. Related: the new guide tabulates all 16 receiver codes but leaves the five processor codes in prose, so those are the ones an operator cannot look up. - Log volume regression on the receiver —
:106-111now emits an info line for every check run in the repo. The guide tells operators to subscribe Check runs repo-wide, so GitHub deliverscreatedandcompletedfor every app's every check; that is tens of lines per push, withaction_not_completed(expected, near-universal) at the same severity asuntrusted_preview_url(integration broken, or someone probing it). Tiering helps: silent or debug before the Amplify identity gate, info or warn after it. - The rejection logs dropped the correlators along with the secrets — removing the
JSON.parsemessage was the right call, since V8's "... is not valid JSON" text embeds a body fragment. ButX-GitHub-Deliveryis a GUID with no payload content andrepository.full_nameis already logged unredacted three frames later at:160, and without either an operator who seesreason=unexpected_app_slugcannot tell which repo or which delivery to open in GitHub's webhook UI. pr_number_mismatchcovers three unrelated causes —:663: a different PR number, anullbody, and an array body.mainlogged the array case distinctly, so an operator now reads "GitHub handed us the wrong PR" for what is really a malformed-response incident. Splitting outmalformed_pr_responsewould fix that; note also that theArray.isArray(pr)disjunct is dead (an array's.numberisundefined !== 42), which makes the[[pr41, pr42]]table row vacuous.!normalizedwould single-source the bypass —:136re-derives "is this an Amplify check" from the HTTP header when the value it already holds says so, leaving two sources of truth for the same distinction.validated_pr_numberis not a checked wire contract —ProcessorEventis a local, non-exported interface and the receiver builds an untyped literal, so a rename or misspelling on either side compiles green and silently degrades Amplify back to the commit-pulls fallback this PR exists to remove.raw_bodyhad the same exposure onmain, so this is house style being extended rather than a new sin, but it is the first field where a value rather than a body crosses the boundary. ExportingProcessorEventand annotating the literal closes it.- Two comments in the shared module expired with this change —
cdk/src/handlers/shared/github-deployment-status.ts:38-43still saysdeployment.environmentis "Filtered againstSCREENSHOT_TARGET_ENVIRONMENT" and thatdeployment.shamaps back to a PR via the commit-pulls API. Neither holds for the Amplify path now. That file's header presents itself as the anti-drift single source of truth, so it is worth the two-line touch-up. Separately,:642-643dropped the load-bearing why from the replaced comment:mainexplained that a non-array body "would crash an unguarded.findand throw out of the (un-DLQ'd) processor", which is the justification the surviving guard at:674-676still needs. - The processor still logs the full
preview_urlon rejection —:175-178, pre-existing rather than introduced here, but it sits against this PR's stated goal of not logging URLs on rejection. Only the hostname is validated, so path and query are unconstrained and a preview deep-link carrying a token in the query string lands in CloudWatch. Loggingnew URL(previewUrl).hostnameplus a reason code would match the receiver's new style. - Test hygiene —
fetchMock.mock.calls.every(...)atgithub-webhook-processor.test.ts:571is vacuously true on zero calls (add an exacttoHaveBeenCalledTimes); theConditionExpressionassertion inside theddbSend.mockImplementationatgithub-webhook.test.ts:348never runs if the mock is not invoked, so it belongs outside the mock body; andJIRA_WORKSPACE_REGISTRY_TABLE_NAMEis written inbeforeEachatgithub-webhook-processor.test.ts:521and never restored. Also worth noting thatamplifyBody's override spread lands insidecheck_run, so it cannot expressrepositoryoractionoverrides at all — which is whyinvalid_repositoryhas no test. - Cosmetic: the PR title is
fix(...)while the branch and the issue's label both say feature.
Documentation
The strongest part of this PR. Both docs/guides/ sources and their Starlight mirrors are updated, and re-running docs/scripts/sync-starlight.mjs produces a zero diff, so CI's mutation gate will pass. The reason-code diagnostic table and the explicit migration paragraph are the right artifacts to have added. Two gaps: item 3 above, and the five processor-side reason codes being prose-only while all 16 receiver codes get a table row.
Tests and CI
CI is green, and build (agentcore) is the full mise run build at 15m53s, so the whole suite did run. Locally the two changed specs pass 85/85 in 1.2s and eslint is clean on all four changed source files.
Coverage is good where it counts and thin at the edges. New branches with no test: invalid_repository (shared:127 — deleting the guard outright leaves the suite green, because validateDeploymentStatusPayload masks it downstream), the !res.ok 404 path (:633-640, the new real-world case from item 1), fetch throw and abort (:621-628), the non-JSON body (:648-654), and the title/body string coercions (:669-670). Surviving mutations beyond item 2: those coercions, and the dead Array.isArray disjunct. Bootstrap synth-coverage is not applicable here — no CloudFormation resource types were added.
What I verified, and what I checked and dropped
Beyond reading the diff: re-derived the retry-budget arithmetic; confirmed no metric filter exists on error_id anywhere in cdk/src/; confirmed the construct is default-on with no opt-in flag; regenerated the Starlight mirror to confirm zero drift; ran the two changed suites and eslint; and ran the fallback-forcing and payload-mutation experiments described above.
Candidates that did not survive checking, recorded so they do not resurface: the environment-filter bypass is not a fail-open — it is the approved acceptance criterion, and normalized.ok === false returns at :110, before both the dedup write and the invoke. The hostname regex /^pr-([1-9]\d*)\.[a-z0-9]+\.amplifyapp\.com$/ resists the usual tricks, since [a-z0-9]+ excludes . and the pattern is anchored, so suffix, extra-label, and trailing-dot forms all fail; userinfo, non-default ports, and non-https are rejected, and path or query cannot change origin. validated_pr_number cannot inject path segments (digits only, Number.isSafeInteger at both ends) and cannot reach another repo, since repository.full_name comes from the same signed payload and passes REPO_PATTERN. The reason codes in the 200 body are not an information leak: the signature is verified first, and every code is a static member of a closed union with no payload echo. The Amplify identity gates cannot be spoofed, because check_run.app.owner.login, app.slug, and check.name are set by GitHub from the app that created the check run, not by the sender. And the logger has no level filtering, so the guide's claim that reasons appear in the logs does hold. One layer is weaker than its docstring implies, though: isAllowedScreenshotUrl only rejects non-https, localhost, and IP literals — it accepts any DNS name — so the amplifyapp.com constraint lives in exactly one place. Not exploitable today, since grantInvoke from the receiver is the only path to the processor, but re-testing previewUrl against the exported preview-host regex when validated_pr_number is set would make the two-layer story true.
One last note on validation: the PR body states that no live Amplify or Jira smoke test was run for this revision, so the /pulls/{n} behaviour against real GitHub — the 404-on-deleted-PR path in particular — is currently exercised only against mocks.
ayushtr-aws
changed the title
|
Implemented the three blocking requests:
Also addressed SHA casing, the exported rejection-code union and processor diagnostic table, delivery-ID correlation, malformed-response classification, processor URL revalidation and hostname-only URL logging, the environment-filter condition/comment, stale shared-module comments, and the test hygiene issues. The PR title now matches the feature issue/branch. Two scope decisions:
Validation: full |
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict: request changes — narrowly, and much closer
All three blocking items from the last round are genuinely fixed, and I verified each one by mutation rather than by reading. What remains is one classification refactor, four test rows, and two stale doc blocks. I reviewed the redesigned code as it now stands rather than only the delta, because a fix is new reviewable surface — and two of the items below are direct consequences of the change I asked for, which I should own rather than hedge.
Round-1 items: verified fixed
Reverting each fix individually and re-running the suites, every one is caught, so none of these rest on a green suite alone. Restoring the retry of terminal rejections fails 7 tests. Leaking validated_pr_number: 1 onto the deployment path fails 2 — it was silent before. Undoing check.head_sha.toLowerCase() fails 3. Removing the processor's Amplify host/PR revalidation fails 4. The migration paragraph now states the consequence plainly, including public CloudFront readability, rendered customer data, and the deselect-Check runs opt-out.
The contract spec is better than I asked for: it feeds the receiver's real emitted payload into the real processor, uses an uppercase head_sha so the casing fix is exercised end to end, pins fetch to exactly one call, and proves check_run routes to PR 42 while deployment_status routes to 41. expect(Date.now()).toBe(start) as the no-backoff assertion is a nice touch, and expect(JSON.stringify(warn.mock.calls)).not.toContain('secret-response-fragment') is the right way to pin the log-hygiene claim. AMPLIFY_PREVIEW_HOST correctly carries no g flag, so sharing one module-level regex between the normalizer and the processor cannot accumulate lastIndex state across warm invocations.
Both scope decisions are reasonable and I accept them. Keeping info-level logs for unrelated checks is a fair reading of #900's diagnostics requirement now that delivery IDs make them traceable, and holding the construct opt-out for separately approved scope is the right call under ask-first.
Blocking
1. A revoked token on the Amplify path is now reported as a benign PR-state change, and the ERROR that existed to catch it is gone — cdk/src/handlers/github-webhook-processor.ts:658-668 (classification) and :211-220 (log).
This one is on me: it is a consequence of the terminal/retryable split I asked for. Before the split, every non-2xx retried and landed in logger.error('No open PR found for SHA after retries...', { error_id: 'SCREENSHOT_PR_LOOKUP_EXHAUSTED' }), whose own comment names "token regression" as one of the three things it exists to catch. Now, on the Amplify path — which is every check_run delivery — a 401 Bad credentials from a rotated or revoked token, a 403 from SAML or IP-allowlist enforcement or a lost scope (real GitHub 403s of that kind carry x-ratelimit-remaining: 4999, so rateLimited is false), and a 404 returned to conceal a repo the token can no longer see all produce a single logger.warn with no error_id and no retry. The level drops ERROR to WARN, the tag disappears, and the receiver has already 200'd and written a one-hour dedup row, so GitHub never redelivers. Screenshots stop repo-wide with nothing above WARN in the logs. The same token failure on the deployment_status path still produces SCREENSHOT_PR_LOOKUP_EXHAUSTED, so the two providers now have opposite observability for one cause.
The 404 case is the sharpest, because the receiver proved the PR existed seconds earlier: normalization already confirmed check_run.pull_requests contains that exact number with a matching head SHA, from GitHub's own signed payload. A 404 arriving right after that is far likelier to be visibility than absence, yet pr_not_found is the most benign-sounding code in the set — and it is the one terminal reason that returns no status, so nothing anywhere in the logs records that a 404 happened.
The message compounds it. 'Validated Amplify PR no longer matches preview' is true for pr_number_mismatch, pr_not_open, and live_pr_head_sha_mismatch. It is false for pr_request_rejected and pr_not_found, where we never saw the PR at all, and for malformed_pr_response, where GitHub returned a 200 with a non-object body. An operator who greps that string and reads "the PR changed under us, nothing to do" will close a live auth outage as expected behaviour.
The test at cdk/test/handlers/github-webhook-processor.test.ts:603-628 enshrines this rather than catching it: test.each([404, 400, 401, 403, 422]) asserts expect(error).not.toHaveBeenCalled(). That is a test pinning what the code does rather than what it should do.
Suggested fix — split terminal into two classes rather than adding a level check at the log site. Body-shape rejections (pr_number_mismatch, pr_not_open, live_pr_head_sha_mismatch, missing_head_ref, malformed_pr_response) keep the current WARN and wording. Transport rejections that imply operator action (401, 403 not rate-limited, 404) still stop the retry loop, but log at logger.error with an error_id and a message that says GitHub rejected the lookup. Add status: res.status to the pr_not_found return for symmetry.
2. A 403 carrying neither rate-limit header is a documented secondary-rate-limit shape, and it is classified terminal — :663-664.
GitHub's documented client contract for secondary rate limits is three-tiered: honour retry-after if present; else wait for x-ratelimit-reset if x-ratelimit-remaining is 0; else back off exponentially. The predicate covers the first two tiers and misses the third, so a 403 with no retry-after and a non-zero x-ratelimit-remaining is treated as permanent: no retry, screenshot dropped, and a WARN blaming the PR. The transient test.each at :631 covers 403 quota and 403 retry-after but not that third shape.
Fix: also treat a 403 carrying x-ratelimit-reset as retryable. The simpler and safer alternative is to make all 403s retryable — a genuinely revoked token then fails all four attempts and lands in the ERROR path from item 1, which is strictly better than a silent terminal.
While there: res.headers?. is mock accommodation, not defence, since Response.headers is non-optional per spec. It is not inert either — a header-less shim makes rateLimited false and produces the terminal verdict for a 403. Drop the ?. and give the doubles a new Headers().
3. The preview-URL rejection log has no event: key — :179-183.
Every other rejection in this handler carries event: 'screenshot.*', so any filter or dashboard keyed on that field cannot see this one — while the runbook at docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md:211 tells operators to look for untrusted_preview_url. One added key.
Related, same block, non-blocking: Number(amplifyHost[1]) !== validatedPrNumber is unreachable via the receiver, since normalization derives prNumber from that same hostname with that same regex and forwards both together. It fires only on a receiver bug or a forged direct invoke of the processor — the one condition here genuinely worth paging on — and it currently logs identically to "the URL didn't parse". Give it its own reason at error level. The empty catch {} is defensible for not logging attacker-controlled URL content, but it leaves preview_host: undefined, making a parse failure indistinguishable from a host-not-allowlisted rejection under the same code; a url_parsed: Boolean(preview) boolean costs nothing and leaks nothing. And !preview in the second clause is dead — isAllowedScreenshotUrl parses the same string and returns false on throw, and || short-circuits.
Test gaps
Distinct from the above: in each of these the code is correct and only the assertion is missing. Four rows and one assertion close them.
- The 5xx boundary is unpinned. Changing
res.status < 500tores.status <= 500leaves 115/115 green, so a GitHub 500 could be reclassified terminal and dropped without retry; the only 5xx test uses 503. Add 500 and 502 to the transienttest.each. - Nothing asserts a commit-pulls 404 stays retryable. Dropping the
validatedPrNumber !== undefinedguard on the terminal 404 survives. Add adeployment_status404 case asserting four fetches andSCREENSHOT_PR_LOOKUP_EXHAUSTED. - The PR-number comparison is tested in one direction only. Every revalidation row uses
pr-41against validated42, so!==weakened to<survives — a URL whose number is higher than the forwarded one would be accepted. Addinghttps://pr-99.app123.amplifyapp.comfixes it. (!==to>and dropping the comparison outright are both caught.) - The commit-pulls non-array guard is fully uncovered. I confirmed
if (false as boolean && !Array.isArray(parsed))leaves 115/115 green — so the guard that keeps a transient HTML or 502 body from reaching.filterand faulting the async processor has no test at all. This is the guard whose original comment explained the un-DLQ'd-processor risk; worth a test now that the comment is shorter. - The contract spec's negative assertion covers one arm.
toEqual({ raw_body })is gated ondeployment_status, so thecheck_runarm never pins the exact forwarded shape — adding a field (I tested re-leakingoriginal_body) passes. Give that arm the symmetrictoEqual({ raw_body, validated_pr_number: 42 }). - Three retryable reason codes are never asserted anywhere (
fetch_failed,non_json_response,pr_not_linked);http_erroris the only one that appears in a test file.budget_exhaustedis unreachable in tests, as nothing drivesprLookupBudgetto zero.
Comments and types
Two stale doc blocks, both in lines this PR rewrote, so they are fair game rather than pre-existing drift:
cdk/src/handlers/github-webhook.ts:57-58(and the inline comment at:167-170) still say dedup is on(repo, deployment_id, status_id). The key isamplify#-prefixed for check runs, which is the entire point of that line — a check run and a deployment status can share an id, and there is a test pinning the independent namespaces.cdk/src/handlers/github-webhook-processor.ts:606-615asserts unconditionally that the function "Uses the 'List pull requests associated with a commit' GitHub API" and returns "a retryable failure if none". The Amplify path callsGET /repos/{repo}/pulls/{number}and returns terminal failures on seven paths. Make both the endpoint and the failure kind conditional onvalidatedPrNumber.
The rewritten SCREENSHOT_TARGET_ENVIRONMENT block did regain a coherent topic sentence and every per-provider claim in it is now true — thanks. One nit: "including branch-name values that previously excluded those previews" is a changelog sentence in a comment; the docs are the right home for the before-and-after.
On types, AmplifyPreviewRejectionReason is now two types wearing one name — 23 members with two disjoint producers, the receiver's normalization codes and the processor's live-lookup codes, distinguished only by the event field at the log site. The practical cost is that PrLookup's terminal arm is typed to accept 23 values of which 7 are reachable, so the type constrains nothing there and reason: 'unexpected_app_slug' would compile inside findPullRequestForSha. Splitting into AmplifyCheckRejectionReason and PrLookupRejectionReason, with a union alias if a joint name is still wanted, would fix that and keep the runbook tables honest. Relatedly the terminal arm reuses an exported named type while the retryable arm inlines six anonymous literals — the retryable set being unexportable is part of why three of its codes went unasserted. And invalid_pr_number now means two things: the number parsed from the preview hostname isn't a safe integer (receiver), and the forwarded number isn't a positive safe integer (processor). The docs list it in both tables, honestly, but an operator grepping it gets two answers; invalid_forwarded_pr_number for the processor site would settle it. Minor: HTTP_REQUEST_TIMEOUT = 408 sits in the budget-constants block where every neighbour is a millisecond duration, and the name reads as one.
Tests, docs, CI
115 tests pass locally across the three suites, and eslint is clean on all six changed files. No test was removed or weakened — I diffed the names against main: both deletions are supersessions by stronger reason-bearing cases, the dropped test.each row is subsumed, and two assertions were strengthened. Test counts went up in both existing suites. Hygiene nits only: restoreAllMocks() runs last in one describe's beforeEach and first in the other two; SCREENSHOT_TARGET_ENVIRONMENT is deleted rather than saved and restored; the new contract spec never restores timers or mocks after its last test.
Docs are in good shape and the Starlight mirror regenerates to a zero diff, so the mutation gate will pass. All the new reason codes are documented, including the head_sha_mismatch versus live_pr_head_sha_mismatch distinction spelled out explicitly.
One note on CI: build (agentcore) — the job that actually runs the suites — is still pending on this head, so the local run above is currently the only evidence for the test claims. I could not run the masking gate locally (this worktree's mise predates the required version), so I am taking the reported clean baseline on trust there; worth a second look given that two nosemgrep: ts-silent-success-masking annotations were dropped when those bare return nulls became object returns.
…erage Refs #900 Co-Authored-By: OpenAI Codex <codex@openai.com>
|
Addressed the latest review:
Validation: |
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict: approve
Every item from both prior rounds is closed, and I verified each one by mutation rather than by reading the diff — a green suite over a redesign is camouflage otherwise. Nothing blocking remains. The nits below are genuinely optional and I am not gating on them.
Verified closed, by mutation
Each of these reverts the fix and confirms the suite notices:
- Restoring the terminal-rejection retry: 7 tests fail. Leaking
validated_pr_number: 1onto the deployment path: 2 fail (silent two rounds ago). Undoinghead_sha.toLowerCase(): 3 fail. Removing the processor's Amplify host revalidation: 4 fail. - Making 403 terminal again: 5 fail. Downgrading
request_rejectedback to a WARN: 3 fail. Widening the 5xx boundary to<= 500: 1 fails. Dropping thevalidatedPrNumberguard so a commit-pulls 404 turns terminal: 1 fails. Weakening the preview PR-number comparison from!==to<: 1 fails. Disabling the commit-pulls non-array guard: 1 fails.
The last five were the surviving mutations I reported last round; all five are now caught. 127 tests pass across the three suites, eslint is clean on all six changed files, the Starlight mirror regenerates to a zero diff, and the type rename left no stale references in cdk/src, cdk/test, or the docs.
The classification split reads well. pr_rejected / request_rejected / retryable as three kind arms is the right decomposition — a closed or moved PR keeps its single quiet WARN, while a 401 or a 404 now fails fast at ERROR with SCREENSHOT_PR_LOOKUP_REJECTED and the status attached, and a persistent 403 exhausts into SCREENSHOT_PR_LOOKUP_EXHAUSTED. Splitting the reason union by producer means PrLookup's arms now only accept codes the function can actually emit, which is what makes the runbook tables trustworthy. Taking the conservative all-403s-retryable reading was the right call.
Two consequences I traced and am satisfied with. The retry budget still holds: the loop is hard-capped at four attempts of 5s plus 35s of sleeps, and !lookup.ok returns before captureBudget is computed, so no exhaustion path can starve MIN_CAPTURE_BUDGET_MS. And budget_exhausted is now reachable only when zero attempts ran, which is exactly what the guide says it means. A revoked token returns 401 and still fails fast; only a missing-permission or secondary-limit 403 spends the backoff, which is the trade the comment states.
The contract spec now pins both arms' exact forwarded shape, so a receiver that quietly added a field cannot pass — I confirmed that by re-leaking original_body and watching it fail. preview_pr_number_mismatch getting its own tagged ERROR is better than what I suggested, since that branch is unreachable via the receiver and only fires on a forged direct invoke.
Nits — optional, not gating
cdk/src/handlers/github-webhook-processor.ts:688-689—const HTTP_STATUS_REQUEST_TIMEOUT = 408names one of three magic numbers in the same expression while403and429stay literals, and it is re-declared on every non-2xx. A module-levelRETRYABLE_LOOKUP_STATUSES = new Set([403, 408, 429])would read better, or just drop the lone constant.:699— the retryablehttp_errorarm discardsres.status, so the alarm-tagged exhausted line carries onlyreason: 'http_error'. For the case this PR changed — a persistent 403 exhausting rather than failing fast — an operator cannot tell 403-permission from 503-outage at the alarm without correlating the four preceding WARNs. The guide does prescribe exactly that, so it is a documented trade rather than a defect;status?: numberon the retryable arm would make the alarm self-contained.cdk/test/handlers/github-webhook-processor.test.ts:635-651— the four 403 fixtures build distinctHeaders(x-ratelimit-remaining: 0,retry-after: 5,4999, and none), but the code no longer reads any response header, so all four exercise one path. As a guard against reintroducing header-based branching that is worth keeping; as named it implies a branch that does not exist. A one-line comment saying headers are deliberately ignored would resolve it.:690—res.status >= 400is dead insideif (!res.ok), sincefetchfollows redirects by default and nothing sends a conditional header. Noting it only so nobody writes a test for an unreachable state.
On CI, and one thing outside this PR
CI is entirely pending on this head, so everything above rests on the local run rather than on the authoritative job. Worth a glance once build (agentcore) reports. I also could not run the masking gate locally — this worktree's mise predates the required version — so I am taking the reported clean baseline on trust; the reason it is worth a second look is that several bare return nulls carrying nosemgrep: ts-silent-success-masking annotations became object returns over these three rounds.
Separately, and explicitly not for this PR: deliverJiraDeploymentPreview is awaited without a try/catch, so a throw would fault the handler, trigger the async-invoke retries that bypass the receiver's dedup, and duplicate the S3 PUT and the PR comment. That is entirely pre-existing and well outside this diff — flagging it only so it is not lost, and it belongs in its own issue rather than expanding scope here.
Nice work across three rounds — the failure taxonomy is in better shape than what it replaced, and the tests now pin behaviour rather than trailing it.
2 participants
Amplify PR previews now retain their validated PR number through screenshot capture and Jira/Linear feedback. When two PRs share a head SHA, the preview for PR 42 fetches and posts to PR 42; a closed PR or changed SHA stops capture immediately instead of retrying or selecting another PR.
Area
cdk— infrastructure, handlers, constructsdocs— guides or design sourcesRelated
Fixes #900. Completes the Amplify follow-ups from merged #895, also recorded as items 8–10 in #901.
Changes
SCREENSHOT_TARGET_ENVIRONMENTonly to deployment statuses. Existing Amplify operators can retain branch-name filters while accepting validated PR preview checks.Validation
mise run build: 4,779 CDK tests, 990 CLI tests, 1,782 agent tests, and 11 Jira Forge tests passed, along with compilation, lint, synthesis, docs build, and drift checks. Synthesis ran without local AWS credentials, matching CI's environment-agnostic setup.origin/mainpassed. Fullmise run securitystopped at the existingObject.assignfinding in unchangedcdk/src/handlers/linear-webhook-processor.ts:928; later security stages were not reached. The separate masking scan againstorigin/mainreports no new findings.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.