feat(notifications): preview-deploy screenshot pipeline (provider-agnostic)

[isadeks]

Closes #240.

Summary

• New deploy-preview screenshot pipeline. Listens for GitHub deployment_status: success events from any provider (Vercel, Amplify Hosting, Netlify, GitHub Actions custom CD); captures the preview URL via AgentCore Browser; posts a markdown image comment on the open PR.
• Optional Linear-side comment: if a Linear identifier is in the PR title/body and Linear is configured (per feat(linear): OAuth migration with per-workspace token storage (Phase 2.0b) #160), the same screenshot lands on the Linear issue.
• New bgagent github CLI subcommand: webhook-info + set-webhook-secret.
• Provider-agnostic. The repo's worked example uses Vercel because that's what was smoke-tested.

Architecture

• Lambda-only post-PR (no LLM). Receiver verifies HMAC, dedupes, async-invokes processor.
• AgentCore Browser via SigV4-presigned WSS + Chrome DevTools Protocol — no Playwright in the bundle.
• Private S3 + CloudFront with Origin Access Control. Bucket fully private; CloudFront serves images anonymously over HTTPS so GitHub markdown image embeds render without auth.
• WAF exemption on /v1/github/webhook for GenericRFI_BODY (deployment_status payloads embed absolute URLs).

Test plan

• Full CDK suite green locally: 1810 tests, 101 suites passing
• Smoke-tested end-to-end on dev stack: Vercel-connected repo → push commit → preview builds → screenshot lands on PR ~12s after Vercel reports success → also lands on linked Linear issue
• Provider-agnostic verified: no Vercel-specific code paths, only deployment_status filtering by configurable SCREENSHOT_TARGET_ENVIRONMENT (default Preview)
• CI: build + tests pass on PR

Out of scope (followups)

• IAM scoping — currently bedrock-agentcore:* for the processor; tighter action set is followup.
• Tests for the screenshot pipeline (mock for AgentCore Browser WSS, GitHub PR-lookup, S3, comment posting).
• Deploy-protection bypass tokens for providers that gate previews behind auth.
• Multi-workspace Linear lookup prefix-routing.

[krokoko]

Review — Preview-deploy screenshot pipeline

Thanks for this — it's a really well-built feature. The receiver/processor topology faithfully mirrors LinearIntegration, the verify module follows linear-verify.ts, the private-S3 + CloudFront-OAC design is the correct answer to account-level Block Public Access, and the SigV4-presigned-WSS handshake is nicely reasoned (the commit trail documenting why you landed there is genuinely helpful). The docs are thorough and the Starlight mirrors are regenerated and committed. A few things I'd like to see addressed before merge, then some non-blocking notes.

Requesting changes on

1. Tests on the new security boundary. POST /v1/github/webhook is authorizationType: NONE, so the in-Lambda HMAC is the entire authentication path — and it currently ships with no unit tests. The repo already has copy-pasteable templates for exactly this shape (slack-verify.test.ts, linear-webhook.test.ts). The load-bearing branches I'd want pinned:

• verifyGitHubSignature — the timingSafeEqual unequal-length throw guard, the sha256= prefix check, and the rotation re-fetch in verifyGitHubRequest.
• github-webhook.ts receiver — state/environment filter gates, and the dedup ConditionalCheckFailedException + rollback-DeleteCommand path.
• extractLinearIdentifier — pure function, trivial to test, and the g-flag lastIndex reset is the kind of thing that breaks silently on back-to-back calls.

The browser/CDP plumbing in agentcore-browser.ts is a fair followup (it's smoke-tested), but the HMAC + receiver routing + regex are ~2–3h with the existing templates. Worth noting the coverage gate added in b277cc6 may reject this as-is.

2. Unrelated default-label change (bgagent → abca). linear-webhook-processor.ts:34 flips DEFAULT_LABEL_FILTER, but cli/src/commands/linear.ts:50, the mapping-table doc, and LINEAR_SETUP_GUIDE.md:89 ("Default trigger label is bgagent") still say bgagent. Any onboarded project whose row lacks an explicit label_filter would silently switch trigger label on deploy. Could we either split this out of the screenshot PR, or align all four sites and add a migration note? (The label-gate reorder itself looks correct and the two new regression tests are great — it's just the default-value change + drift.)

3. Processor failures are invisible. WebhookProcessorFn is invoked InvocationType: 'Event' with no DLQ / onFailure / Errors alarm, and every failure path logs-and-returns. Since the receiver already 200'd, GitHub won't redeliver — so a systemic break (IAM, AgentCore quota, token rotation) stops 100% of screenshots with no signal anywhere. Could we add an SQS DLQ + a CloudWatch alarm on processor Errors?

Non-blocking suggestions

• WAF rationale vs. code. /v1/github/webhook is added to the priority-1 scope-down, which excludes only SizeRestrictions_BODY — so GenericRFI_BODY is still evaluated on the path, which doesn't match the commit message ("trips GenericRFI_BODY"). Since the smoke test passed, the real blocker may have been body-size rather than RFI. Worth reconciling the comment/commit with what's exempted (and the troubleshooting doc, which tells operators the path is RFI-exempt).
• Error/login-page screenshots. Page.navigate only checks errorText; an HTTP 4xx/5xx or auth wall navigates "successfully" and the 404/login PNG gets posted as if it were the app (the guide acknowledges this). Enabling the Network domain and skipping the post when the main-document status isn't 2xx would avoid posting confidently-wrong output.
• WebSocket leak on open-timeout (agentcore-browser.ts:1469-1494): the open promise rejects without ws.close()/terminate(), and the throw escapes before the try/finally, leaving a dangling socket per failed attempt. Wrapping the open in the same finally would tidy this up.
• Unguarded res.json() (github-webhook-processor.ts:854): a 2xx non-array/HTML body throws, and since findPullRequestForShaWithRetry has no try/catch, it crashes the (un-DLQ'd) processor. An Array.isArray guard returning null keeps the function's existing contract.
• SSRF note: environment_url flows from the payload straight into Page.navigate. It's HMAC-gated and runs in the managed AgentCore session (not the Lambda VPC), so blast radius is bounded — but a scheme/host allowlist before navigating would be a cheap hardening follow-up.
• bedrock-agentcore:* on * is broader than the project precedent (task-orchestrator.ts scopes specific actions). Acknowledged in the guide as a known gap — worth a tracked follow-up issue.
• Stale strings: a couple of CfnOutput descriptions still say "Vercel-preview", and the construct doc-comment still calls it a "Public-read screenshot S3 bucket" though it's now private + OAC.

Really nice work overall — the bulk of this is the test coverage on the auth boundary and making failures observable; the rest is polish. Happy to pair on the test scaffolding if useful.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 90.99284% with 88 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@14820c8). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
cdk/src/handlers/shared/linear-issue-lookup.ts	53.47%	87 Missing ⚠️
...dk/src/constructs/github-screenshot-integration.ts	99.65%	1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #241   +/-   ##
=======================================
  Coverage        ?   86.47%           
=======================================
  Files           ?      181           
  Lines           ?    41764           
  Branches        ?     4157           
=======================================
  Hits            ?    36116           
  Misses          ?     5648           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[isadeks]

@krokoko thanks for the review. Status by item.

Shipped on this branch

• bedrock-agentcore:* IAM scope-down (e791e62) — narrowed to StartBrowserSession, StopBrowserSession, ConnectBrowserAutomationStream
• Cosmetic Vercel CfnOutput (e791e62)
• WebSocket leak on open-timeout (be7b527) — error, unexpected-response, and timeout paths in the open-promise now call ws.terminate() before reject
• findPullRequestForSha JSON parse + Array.isArray guard (be7b527) — non-array body no longer crashes the un-DLQ'd processor
• WAF rationale reconciled (873ecea) — CloudWatch BlockedRequests metric for TaskApiWebAcl shows SizeRestrictions_BODY fired 2x on 2026-05-21 (matching commit 36e8d14's smoke-test window 1:1); GenericRFI_BODY has never fired on this WebACL. Code was correct (only excludes SizeRestrictions_BODY); just the inline comment + DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md rationale were wrong. Also fixed the doc wording that overstated the scope (was "excluded from the CommonRuleSet" → now "exempted from SizeRestrictions_BODY; LFI/RFI/XSS/SQLi still evaluate"). Starlight mirror re-synced.
• DEFAULT_LABEL_FILTER reverted to bgagent (8a6fc86) — the bgagent → abca rename is unrelated to this PR; fix(linear): split bgagent → abca default-label change out of PR #241 + align CLI/docs #285 owns landing it across all four sites (processor / CLI / setup guide / mapping doc) in its own PR. Test fixture updated to match the restored default.

Shipped in stacked PRs

• Tests on the security boundary → test(screenshot): cover the screenshot pipeline #275 (53 tests). Covers verifyGitHubSignature (timingSafeEqual unequal-length guard, sha256= prefix check, rotation re-fetch in verifyGitHubRequest), receiver state/environment filters and dedup ConditionalCheckFailedException + rollback DeleteCommand path, regex extract including the g-flag lastIndex reset on back-to-back calls, processor happy/failure paths with CloudFront-host URL assertion, and the AgentCore Browser CDP exchange (Stop invoked in finally even on error, 403 unexpected-response surfaced, navigate errorText raised). 385/385 across 16 suites.
• Bonus → feat(linear): prefix-route multi-workspace issue lookup by team key #273 (multi-workspace prefix-routing for the findLinearIssueByIdentifier scan). Smoke-validated on dev with two real workspaces — ABCA-78 from a PR title routed directly to the matching workspace on the first try (single GraphQL call, not N).

Tracked as follow-up issues (won't hold this PR)

• DLQ + alarm on processor → feat(infra): add SQS DLQ + CloudWatch alarm to GitHub webhook processor #284 (blocking item Docs: user guide hard-codes us-east-1 #3 — observability gap; better as its own PR with cdk-nag scrubs and a smoke test of the alarm firing)
• Default-label split (bgagent → abca) + four-site alignment → fix(linear): split bgagent → abca default-label change out of PR #241 + align CLI/docs #285 (blocking item Docs: specify that you can't use the agent with the canned repo #2 — needs a migration note for projects relying on the default; cleaner as its own PR)
• SSRF allowlist on environment_url → feat(security): scheme + host allowlist on screenshot processor's environment_url #286 (non-blocking — defense-in-depth; HMAC-gated path means it's not load-bearing on this PR)
• Skip screenshot when preview URL returns non-2xx → feat(screenshot): skip screenshot when preview URL returns non-2xx #287 (non-blocking — Network domain status check; out of scope for the receiver/processor wiring under review here)

Re-requesting review.

[theagenticguy]

🏛️ Principal-architect review — preview-deploy screenshot pipeline

Reviewed with the ABCA review_pr workflow: vision and tenet alignment, the automated agent passes, principal judgment, then docs and tests. Thank you for a well-built PR. The provider-agnostic framing, the Lambda-only post-PR path, the CloudFront-plus-OAC pattern, and the care from prior review rounds all show through. Most findings below are hygiene. Two are worth fixing before merge.

1. Verdict

Request changes, on a small and well-scoped set. The architecture is sound and vision-aligned. The blockers are a thin Lambda-timeout budget and a fail-open edge in the HMAC verifier, plus a few stale comments that mislead in a security direction. None is an active exploit on the default configuration.

2. Vision alignment

Strongly aligned. It keeps the LLM out of a deterministic post-PR job (tenet 3) and preserves fire-and-forget for the submitter (tenet 1). It stays provider-agnostic through config rather than hardcoded provider names (tenet 8). It updates ROADMAP and COST_MODEL so the change is attributable (tenets 7 and 10). No tenet is traded away, so no ADR is needed.

3. Blocking issues

B1. The processor Lambda timeout budget can be exceeded on the worst-case path, and the budget comment is inaccurate.
cdk/src/constructs/github-screenshot-integration.ts:143-145, and the lookup-then-capture order in cdk/src/handlers/github-webhook-processor.ts.
The processor runs findPullRequestForShaWithRetry first. Its delay schedule is [0, 5s, 10s, 20s], so it can spend up to about 35s when a PR is found only on the final attempt. Only then does it call captureScreenshot, which has its own independent 60s default budget, followed by the S3 PUT and the comment POST. The inline comment accounts for "60s screenshot + … = 95s" but never counts the 35s retry that precedes it. The realistic worst case (PR found on the last retry, plus a slow render) is roughly 95 to 100s against a 120s timeout. Slow GitHub round-trips during the retry can push it over. A hard timeout then dies mid-capture or mid-comment. There is no DLQ (InvocationType: 'Event', no onFailure), and the dedup row stays written, so GitHub's redelivery is deduped and the work is silently lost.
Suggested fix: thread one shared wall-clock deadline. Compute deadline = now + budget at handler start, then pass the remaining budget into both the retry helper and captureScreenshot(url, { timeoutMs: remaining - reserve }). Alternatively, shorten the retry ladder and raise the Lambda timeout. Either way, correct the budget comment to count the retry that runs first.

B2. HMAC verification has no empty-or-blank-secret guard, which is a fail-open edge.
cdk/src/handlers/shared/github-webhook-verify.ts, in verifyGitHubSignature.
crypto.createHmac('sha256', webhookSecret) runs with no check that the secret is non-empty. If webhookSecret is '', an attacker who computes HMAC-SHA256('', body) produces a signature that passes. That is the only authentication on a public, unauthenticated endpoint. It is not reachable on the default path: a fresh CDK Secret gets a random value and so fails closed, and set-webhook-secret trims and rejects empty input. It only bites if an operator writes a blank secret out of band. That is still the fail-closed-on-risk case (tenet 4), and the fix is about three lines.
Suggested fix: in getGitHubWebhookSecret, treat an empty or whitespace SecretString as null so the receiver returns 401. Optionally also early-return false in verifyGitHubSignature when the secret is empty.

The rest of this path fails closed correctly. A missing signature returns 401. A malformed or length-mismatched signature is rejected. A null or unfetchable secret yields false. Transient Secrets Manager errors re-throw and return 500, so GitHub retries. The comparison uses timingSafeEqual, so there is no timing oracle. There is no way to reach the processor without a valid signature on the default configuration.

B3. Three "public-read" comments contradict the implementation, in a security-misleading direction.
cdk/src/constructs/github-screenshot-integration.ts:86 (topology JSDoc), :93 (field doc), and :117 (inline).
The bucket is fully private. It sets BlockPublicAccess.BLOCK_ALL and is served only via CloudFront OAC. screenshot-bucket.ts gets this right and explains why. Only these three comments still say "Public-read." On a security-sensitive construct, that could lead a future maintainer to "simplify" by weakening Block Public Access. Reword to "Private bucket; served anonymously via CloudFront OAC."

4. Non-blocking suggestions and nits

The primary artifact can fail quietly. The PR-comment catch in github-webhook-processor.ts logs at warn with no metric, after the browser session and S3 PUT are already spent. A systematic break such as a token-scope regression or a comment-size limit produces no operator signal. Suggest an error-level log plus a ScreenshotCommentPostFailed CloudWatch metric and alarm. The same idea applies to "no PR after retries": a …PrLookupExhausted metric would let you tune the ladder.

SSRF is present but bounded. The processor navigates the browser to deployment_status.environment_url with no validation. AgentCore Browser is AWS-managed and sits outside the customer VPC, so IMDS and private-subnet pivots are neutralized. The residual risk is that whatever renders is published to a public URL, which amplifies any read. Suggest validating the URL (require https:, reject literal-IP, localhost, and link-local hosts) and adding an SSRF bullet to the guide's hardening section, which currently omits it.

Screenshot URLs are public and enumerable. The bucket is private, but CloudFront serves anonymously and the key screenshots/<org_repo>/<sha>.png is guessable, since the repo is public and the SHA appears in the PR. Preview deploys can render PII. The guide's "Sensitive content" note understates this. It suggests WAF and logs, which do not stop a correctly-formed anonymous GET. The cheapest hardening is a high-entropy suffix on the key, which makes the URL unguessable while staying anonymously cacheable.

A few smaller items:

• GitHubDeploymentStatusEnvelope and GitHubDeploymentStatusPayload describe the same wire payload in two files and already disagree on target_url (declared in the processor, never read). Hoist one shared interface, ideally a single validate(payload) narrowing function. That also closes the gap where the receiver admits a payload missing deployment.sha that the processor then drops.
• The bedrock-agentcore IAM PutSecretValue grant is correct but reads as read-only. I traced the chain: resolveLinearOauthToken rotates Linear's refresh token via PutSecretValueCommand, so the write grant is necessary and matches the orchestrator. Add a one-line note on the construct comment (:168-171) and the IAM5 suppression so the write intent is explicit. This is not an over-grant.
• extractLinearIdentifier uses a g-flagged module-level regex. It is handled correctly today via the manual lastIndex = 0 reset, but it deserves a back-to-back-call test (see section 6).
• CdpMessage.result is Record<string, unknown>, which forces four unchecked as casts in agentcore-browser.ts, for example targetInfos as Array<…>. linear-issue-lookup.ts casts DynamoDB attributes as string. Prefer per-command result interfaces, or a typed unmarshal plus an Array.isArray guard.
• buildScreenshotKey uses repo.replace('/', '_'), which replaces only the first slash. It is fine for owner/repo, but replaceAll is more honest. SCREENSHOT_KEY_PREFIX is exported but unused, since the processor hardcodes 'screenshots/'. nextCdpId is a needless module global; scope it to runCdpScreenshot. delays.indexOf(delay) in the retry log breaks if a delay value repeats, so use an indexed loop. The step-6 header comment in agentcore-browser.ts says Page.frameStoppedLoading, but the code waits on Page.loadEventFired. The GitHub fetch calls lack the AbortController timeout the Linear path uses.

5. Documentation

Coverage is solid. There is a new operator guide, and USER_GUIDE, COST_MODEL, ROADMAP, and Task-lifecycle are all updated. The Starlight mirror is in sync: re-running docs/scripts/sync-starlight.mjs produced zero diff, so CI's "Fail build on mutation" gate will pass. Three spots to fix:

• DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md near line 203 still says the processor IAM grants bedrock-agentcore:*. The code already scoped it to the three Browser actions, so the doc describes a worse posture than what ships.
• Step 3c references "the same secret you used in 4b." The secret is generated in step 3b. There is no 4b.
• In the PR description only, the WAF exemption is described as GenericRFI_BODY. The code and all in-tree comments and docs correctly exempt SizeRestrictions_BODY. Please correct the description so the merge commit is accurate.

6. Tests and CI

This is the most consistent theme across the automated passes. The two new linear-webhook-processor.test.ts regression tests are good, because they assert the absence of side effects rather than just "doesn't throw." The 13→14 table-count bump is correct. But the new pipeline ships with zero dedicated tests, and several deferred units are pure, cheap, and security-relevant. The issue fairly scopes out the expensive AgentCore-WSS mock, but these are not expensive:

• verifyGitHubSignature and verifyGitHubRequest are the only auth on a public endpoint. A table-driven test (valid, wrong-secret, no-prefix, length-mismatch, empty-secret, post-rotation) is the cheapest possible and would have caught B2 directly.
• extractLinearIdentifier needs a back-to-back-call test to pin the lastIndex reset.
• buildScreenshotKey is a pure string builder that determines the public URL.
• CDK synth assertions for ScreenshotBucket (BlockPublicAccess all-true) and the IAM scope-down both have copy-adaptable sibling precedents in trace-artifacts-bucket.test.ts and linear-integration.test.ts. Nothing currently locks in the scope-down that a prior review round explicitly asked for.

On CI: Validate PR title passed. build (agentcore) was still pending at review time, so please confirm it is green before merge.

7. Review agents run

The named pr-review-toolkit plugin agents are not installed as agent definitions in this environment. To honor the skill's mandatory-plugin step, I ran a dedicated subagent for each toolkit role against a checkout of the PR head, and I am noting that here for transparency:

• code-reviewer: surfaced B1 and the DRY and cast nits.
• silent-failure-hunter: confirmed the HMAC path fails closed; flagged the warn-level comment-delivery gap.
• type-design-analyzer: the duplicated payload interface and the unsafe casts.
• comment-analyzer: the public-read, IAM, timeout, and step-4b stale comments.
• pr-test-analyzer: the coverage assessment in section 6.
• /security-review: the empty-secret fail-open, SSRF, enumerable-URL, and IAM findings.

None in scope was omitted. I did not run build, lint, or cdk-nag locally, because the review checkout has no node_modules. I am flagging that as an explicit skip, deferred to the PR's own CI.

8. Human heuristics

• Proportionality. Pass. Complexity matches the problem. The hand-rolled CDP loop is justified, since it avoids roughly 150 MB of Playwright in the bundle, and it stays minimal.
• Coherence. Concern. The duplicated deployment_status interface and the three "public-read" comments describe one concept two ways.
• Clarity. Concern. The timeout-budget comment (B1) and the stale IAM doc would mislead the next maintainer. Names are otherwise clear and intent-revealing.
• Appropriateness. Concern. The code is maintainable and idiomatic, but the absence of tests on the pure security-critical units is below this repo's bar. The empty-secret case is exactly the "assert what the code should do" test that is missing.

---

Automated review by Bonk via the ABCA review_pr workflow. Happy to dig into any of these or open follow-up issues for the deferred items.

[isadeks]

@theagenticguy thanks for the principal-architect pass — addressed everything end-to-end. Smoke-tested live on dev: 8.1s end-to-end, narrowed IAM holds, screenshots landed on the PR and the linked Linear issue. Re-requesting review.

Blockers (all in 2892673)

B1 — Shared wall-clock deadline. github-webhook-processor.ts now threads one TOTAL_BUDGET_MS=110_000ms deadline across findPullRequestForShaWithRetry + captureScreenshot + S3 PUT + comment POST. PR-lookup is capped so the screenshot half always gets at least MIN_CAPTURE_BUDGET_MS=15s; if PR-lookup eats the budget on a slow-GitHub day we fail-fast with screenshot.budget_exhausted instead of starting an AgentCore session that's already doomed. Lambda timeout stays 120s; the 10s headroom covers SDK retries + shutdown grace. Construct comment updated to count the retry that runs first.

B2 — Empty-secret HMAC fail-open closed. getGitHubWebhookSecret now returns null when SecretString is empty or whitespace-only (logs error, drops cache entry). verifyGitHubSignature adds defense-in-depth early-return when the secret is empty. New table-driven test covers the exact attacker shape: HMAC('', body) against an empty stored secret.

B3 — Stale public-read comments reworded. github-screenshot-integration.ts topology JSDoc + field doc + inline all said "Public-read screenshot S3 bucket"; the bucket has been BlockPublicAccess.BLOCK_ALL the whole time. Reworded to "Private bucket; served via CloudFront OAC."

IAM action-name verification

You called out ConnectBrowserAutomationStream as a possible SDK type-name guess. I had a subagent verify against the AWS Service Authorization Reference for bedrock-agentcore: it is a real published action. The reference also notes it has no resource types or condition keys — Resource:"*" is mandatory. Construct comment + screenshots-guide hardening section now cite the reference. Smoke-tested on dev with the narrowed {StartBrowserSession, StopBrowserSession, ConnectBrowserAutomationStream} set: full pipeline succeeds in 8.1s.

Non-blocking nits

• SSRF allowlist on environment_url — new cdk/src/handlers/shared/screenshot-url.ts with isAllowedScreenshotUrl: https-only, rejects literal-IP / localhost / link-local / loopback. Unit tests cover Vercel/Amplify/Netlify/GitHub-Pages happy paths plus IPv4 / IPv6 / RFC1918 / IMDS rejections.
• High-entropy 64-bit key suffix — screenshots/<owner>_<repo>/<sha>-<deploymentId>-<16hex>.png. Public CloudFront URL is now unguessable from the public PR; suffix entropy verified at 100/100 distinct over 100 calls.
• Single shared GitHubDeploymentStatusPayload + validateDeploymentStatusPayload in cdk/src/handlers/shared/github-deployment-status.ts. Receiver and processor share one validation contract. Closes the gap where the receiver admitted payloads missing deployment.sha that the processor would drop.
• AbortController on the GitHub commit-pulls fetch (5s per-attempt cap; caller still owns the wall-clock). Mirrors the Linear path.
• Type-narrowed CDP result accessors — replaced the four as casts in agentcore-browser.ts with narrowTargetInfos / narrowSessionId / narrowNavigateError / narrowScreenshotData checked accessors.
• replaceAll('/', '_') for the repo slug.
• Scoped nextCdpId to runCdpScreenshot (was module-global).
• Stale Page.frameStoppedLoading docstring fixed (the code waits on Page.loadEventFired).
• PutSecretValue IAM intent comment — explicit note that the write grant is for Linear refresh-token rotation, not a typo.
• Promoted warn → error with tagged event_id on screenshot.* paths (pr_lookup_exhausted, capture_failed, s3_put_failed, pr_comment_post_failed). CloudWatch metric filters / alarms can now fire on what was previously invisible.
• Indexed retry loop replaces delays.indexOf(delay) (broken if delays repeats).

Doc fixes

• DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md hardening section: was "IAM grants bedrock-agentcore:*" (stale); now describes the scope-down with the auth-reference citation, plus SSRF and screenshot-URL enumerability mitigations.
• Step 3c reference from "4b" → "3b" (the secret is generated in 3b; there is no 4b).

Tests (4 new files, 58 tests)

• github-webhook-verify.test.ts — table-driven verifier, including the exact empty-secret attack shape that B2 fixes.
• screenshot-url.test.ts — buildScreenshotKey shape + 100/100 entropy + replaceAll; isAllowedScreenshotUrl covers schemes, IPv4 / IPv6 literals, localhost, RFC1918, IMDS.
• linear-issue-lookup.test.ts — extractLinearIdentifier including the back-to-back-call test that pins the g-flag lastIndex reset.
• screenshot-bucket.test.ts — synth-time assertion that BlockPublicAccess is all-true so a future "simplify" can't drop public-access protection.

Full handler suite still green: 1387/1387 across 70 suites.

Smoke verification on dev

Cherry-picked onto linear-vercel, deployed, direct-invoked the processor with a real PR SHA (PR #52 on isadeks/vercel-abca-linear). Logs:

{"message":"Screenshot pipeline starting","budget_ms":110000}
{"message":"AgentCore Browser session started","session_id":"01KTS0GDGJ8204G22P97EHSQCY"}
{"message":"Posted screenshot comment to PR","pr_number":52,"public_url":".../000106996c4c5315de7fa43cf2ba0ab475cf2670-42-aa913ff858180ede.png"}
{"message":"Posted screenshot comment to Linear issue","identifier":"ABCA-108","workspace_slug":"maguireb"}
Duration: 8110.56 ms

Confirms the narrowed IAM (3 actions) is sufficient for the SigV4-presigned WSS handshake on real traffic. Earlier dev hang turned out to be transient and is not reproducing.

[krokoko]

🏛️ Principal-architect review — round 3

Thank you @isadeks for the thorough follow-through across both prior review rounds — this is exemplary review responsiveness. I verified every claimed fix against the code at head e553b2c rather than taking the response comments on faith, and everything checks out.

Verdict

Approve with nits. All blocking items from both prior rounds (@krokoko's and @theagenticguy's) are verifiably fixed in code, CI is fully green, and the follow-up issues (#284, #285, #286, #287) and stacked PRs (#275, #273) all exist and are correctly scoped. What remains is a small set of new, non-blocking findings.

Prior feedback — verification status

Item	Status
B1 shared wall-clock budget	✅ github-webhook-processor.ts:55-70,165,185 — arithmetic verified consistent with the 120s Lambda timeout
B2 empty-secret HMAC fail-open	✅ Fixed at both fetch (github-webhook-verify.ts:59) and verify (:107) layers, with the exact attack-shape test
B3 stale "public-read" comments	✅ All three reworded
IAM scope-down to 3 bedrock-agentcore actions	✅ With Service Authorization Reference citation
bgagent→abca label revert	✅ Reverted; split tracked in #285
WS leak, Array.isArray guard, WAF rationale, SSRF allowlist, key entropy, shared payload type, warn→error promotion, CDP cast removal, doc fixes	✅ All verified
Security-boundary tests	✅ 58 in this PR (all pass; verifier at 100% statements), rest in stacked #275

Starlight mirror verified in sync (re-ran sync-starlight.mjs — zero diff). Governance ✅ (issue #240 approved, branch naming correct).

New non-blocking findings

1. Markdown comment injection via environment_url — github-webhook-processor.ts:457-483. previewUrl is interpolated raw into [![preview](…)](${previewUrl}). Node's URL parser preserves ) in the path, so a URL like https://preview.vercel.app/x)](https://evil/a.png) passes isAllowedScreenshotUrl (hostname is clean) yet breaks out of the markdown link syntax, injecting attacker content into a comment posted under ABCA's token. Reachable without the webhook secret in configs where a fork-PR author influences the preview path. Cheap fix: percent-encode (/) before interpolation. Fits feat(security): scheme + host allowlist on screenshot processor's environment_url #286's scope, or a one-liner here.
2. SSRF allowlist: IPv6 unique-local passes — screenshot-url.ts:60-68. Good news: decimal/octal/hex IPv4 literals (https://2130706433) are normalized by WHATWG URL and correctly blocked (verified empirically). But https://[fc00::1] (RFC 4193) and NAT64 literals pass — only ::1, fe80:, ::ffff: are checked. Since preview URLs are always DNS names, rejecting any IPv6 literal (hostname.includes(':')) is the simplest closure. Suggest folding into feat(security): scheme + host allowlist on screenshot processor's environment_url #286.
3. Stale key-layout comments — screenshot-bucket.ts:31-35,67-69 still describe screenshots/<repo>/<sha>.png; the actual key now has the slug, deployment id, and 16-hex suffix. Same drift at DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md:189 (line 205 of the same guide is correct). Also SCREENSHOT_KEY_PREFIX is exported but unused. Two-minute fix + mirror sync.
4. validateDeploymentStatusPayload has zero direct or indirect coverage — it's the receiver's 400-gate and pure/synchronous; a ~15-line table-driven test needs no mocks. Worth pulling forward from test(screenshot): cover the screenshot pipeline #275, along with the uncovered ::ffff: branch at screenshot-url.ts:68.
5. Smaller items: (a) linear-issue-lookup.ts:102-103 casts DynamoDB attributes as string unchecked — the one spot not meeting the narrowing standard this PR itself set in agentcore-browser.ts; (b) if (!resolved) continue; at :118 skips a workspace with no log — if every token breaks, Linear comments stop silently; (c) the null-secret path surfaces as "Invalid signature" 401s, sending an operator to GitHub config instead of Secrets Manager — a distinct tagged log would help; (d) cli/src/commands/github.ts:126 placeholder heuristic is inverted (CDK's default Secret is a random password, not JSON), so the overwrite warning fires on a fresh deploy — UX-only; (e) the review-citation comments ("theagenticguy PR-241 review item B1") are rot risks — keep the rationale, drop the attribution.

Human heuristics

All four dimensions now pass (coherence/clarity/appropriateness were concerns in round 2 — the shared payload type, corrected budget comments, and attack-shape tests resolved them).

Bottom line

Mergeable. My suggestion: fix the two cheapest items (1's paren-encoding, 3's comment sync) before merge, fold 2 into #286 and 4 into #275 — or merge as-is and track 1 in #286 too, since its prerequisite is configuration-dependent. Either way, really nice work — three rounds in, this pipeline is in great shape.

---

Review agents run: code-reviewer, silent-failure-hunter, type-design-analyzer, comment-analyzer, pr-test-analyzer, security-review (subagent against a checkout of the PR head). None in scope omitted. All 58 new tests executed locally — green.

[isadeks]

@krokoko thanks for the round-3 pass — and for verifying against the code at head rather than the response comments. Addressed the two you flagged for "before merge," routed the other two to where they belong, and noted the finding-5 items below. New head: f284e04.

Fixed on this branch (f284e04)

• Finding 1 — markdown injection via environment_url ✅ New encodeMarkdownUrl helper in screenshot-url.ts percent-encodes (→%28 / )→%29 before previewUrl is interpolated into either comment renderer (renderCommentBody, renderLinearCommentBody). The browser decodes the escapes so the link still resolves to the real preview, but it can't close the ](…) early. publicUrl is our own CloudFront key (no parens by construction) so it's left as-is. Covered by 3 new unit tests including your exact …/x)](https://evil/a.png) attack shape — asserts no ]( delimiter survives encoding.
• Finding 3 — stale key-layout comments + unused export ✅ screenshot-bucket.ts and DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md:189 now describe the real screenshots/<owner>_<repo>/<sha>-<deploymentId>-<16hex>.png layout (with a pointer to buildScreenshotKey and a note that the random suffix can't be hand-constructed). Removed the unused SCREENSHOT_KEY_PREFIX export. Starlight mirror re-synced.

Routed to the stacked work, per your suggestion

• Finding 2 — IPv6 unique-local (fc00::/7) / NAT64 pass → folded into feat(security): scheme + host allowlist on screenshot processor's environment_url #286 (it owns the environment_url allowlist). Proposed closure there: reject any IPv6 literal (hostname.includes(':')) since preview URLs are always DNS names — also closes the uncovered ::ffff: branch.
• Finding 4 — validateDeploymentStatusPayload has no coverage → folded into test(screenshot): cover the screenshot pipeline #275 (it owns the pipeline test gap). Table-driven, no mocks; happy path + each missing-field rejection, plus the ::ffff: branch.

Finding 5 (smaller items) — disposition

• (a) unchecked as string casts at linear-issue-lookup.ts:102-103 — agreed it's the one spot below the narrowing bar this PR set; folding into test(screenshot): cover the screenshot pipeline #275's scope alongside the other lookup work.
• (b) silent continue on unresolved workspace token (:118) — real silent-failure risk; tracking in feat(security): scheme + host allowlist on screenshot processor's environment_url #286's neighborhood (observability on the resolve path). Will add a tagged warn so a fully-broken workspace isn't invisible.
• (c) null-secret surfaces as "Invalid signature" 401 — fair; a distinct tagged log ("secret empty/missing" vs "signature mismatch") points operators at Secrets Manager instead of GitHub config. Small observability follow-up, not blocking.
• (d) inverted CLI placeholder heuristic (github.ts:126) — UX-only, fires the overwrite warning on a fresh deploy. Tracking as a CLI nit.
• (e) review-citation comments ("theagenticguy PR-241 review item B1") — agreed these are rot risks. I kept the rationale and dropped the attribution on the new finding-1 comments; happy to sweep the older ones in a docs/comment pass rather than churn this diff again.

CI re-running on f284e04. Thanks again — this was a genuinely sharpening review.

[isadeks]

Follow-up — findings 2 and 4 are now also fixed on this branch (335eea4), not deferred after all.

When I went to fold them into #275/#286 as suggested, the routing didn't hold up: #275 (feat/97-screenshot-tests) predates the #240 refactor that extracted screenshot-url.ts and the shared github-deployment-status validator — neither module exists on #275's branch, and #240 isn't an ancestor of it. #286 is an issue with no code branch. So both findings' code lives only here; fixing them here was the only place they'd actually land against the right code.

• Finding 2 — IPv6 unique-local / NAT64 SSRF gap ✅ isAllowedScreenshotUrl now rejects any IPv6 literal (bracketed host, or a : in the host) instead of enumerating ranges — covers fc00::/7 ([fc00::1]), NAT64, IPv4-mapped, loopback, and link-local in one rule, since preview URLs are always DNS names. Verified empirically that the WHATWG parser normalizes integer-form IPv4 (2130706433, 0x7f000001 → 127.0.0.1) so the dotted-quad regex catches those too. screenshot-url.ts now at 100% branch coverage — closes the old uncovered ::ffff: branch you flagged.
• Finding 4 — validateDeploymentStatusPayload coverage ✅ New github-deployment-status.test.ts: happy path, each missing/empty required field (state, statusId, environmentUrl, deploymentId, sha, environment, repoFullName), absent nested objects, and the id: 0 type-vs-truthiness edge. Validator at 100%.

That leaves only the finding-5 items (small observability/UX nits) as genuine follow-ups; everything blocking-or-cheap from rounds 1–3 is now on 335eea4. CI re-running.