feat(carve S4): Linear issue-context surface — attachments, PDF/image screening, auth health

[isadeks]

Stacked on #654 → #653 → #647. Review those first; this PR's diff is only its own slice.

What this adds

The Linear surface's issue-context gathering and operator tooling. None of it depends on the sub-issue orchestration arc — it's reachable today through the existing Linear webhook path.

Issue context. linear-attachments.ts and linear-issue-context-probe.ts pull an issue's file attachments and recent comments into the task context, so the agent sees the screenshots and discussion a human reading the ticket would.

Attachment screening. attachment-screening.ts screens fetched images and PDFs before they reach the model.

Error classification. error-classifier.ts separates transient compute faults from genuine build failures, and reports whether the work was preserved when a run doesn't produce a pull request — so a user gets "the branch has your work, the PR just didn't open" instead of a generic failure.

Auth health. An expired or revoked Linear authorization previously meant events were silently dropped. linear-auth-health.ts + platform-doctor.ts make it a diagnosable condition, surfaced through the CLI.

One coupling worth flagging

The PDF path upgrades pdf-parse to v2, and that's three inseparable changes: the source uses the v2 PDFParse class, which requires pinning the dependency and deleting the hand-written module declaration that described the v1 function export. Any two without the third fails to compile. They're together in this PR for that reason.

The bundling-contract test that guards the dependency being externalized from Lambda bundles asserts on the integration constructs, so it lands with those in a later slice.

Verification

Full CDK suite 2598 passing, CLI 675 passing, both type-checks clean. Comments and test names explain their reasoning directly rather than citing internal tracker ids, and two CLI strings that printed an internal issue number to user-facing output are reworded.

---

Tracking

Slice S4 of the linear-vercel → main carve. Tracking issue: #668 (slice table, why it is sliced rather than merged, and review guidance).

Lands part of #247 (parent/sub-issue orchestration) and #299 (auto-decomposition). Deliberately not Closes — no single slice closes either; they close when S8 activates the arc.

Stacked on #654 — review that first; this PR's diff is against its branch.

[isadeks]

Automated review — carve S4 (#655)

Reviewed as its own diff only (carve/s3-linear-nomcp-attachments..carve/s4-linear-surface, 27 files, +3423/−91). This slice ingests untrusted user-supplied files from an issue tracker, so security got priority. Governance is noted once for the whole stack, not repeated here.

Slice standalone-ness: PASS

tsc --noEmit clean at this tip on its own base (compiler run). Every symbol resolves at s3. yarn.lock changes here for the first time in the stack — adding pdf-parse for attachment screening — and never regresses against main afterwards, so no dependency revert.

Security posture — the parts that are right

Worth recording, since this is the slice where it matters most:

• SSRF is properly closed. linear-attachments.ts:154 enforces host === 'uploads.linear.app' || host.endsWith('.uploads.linear.app') on every fetched URL. No user-controlled host reaches fetch.
• Size limits are layered and sane: 10 attachments/task, 10 MB each, 50 MB total, 500 KB inline, 3 MB total inline.
• Path traversal is handled by deriving a safe id from the URL pathname rather than trusting the filename.
• No raw parse output reaches the user — failures surface a classified reason plus a task id, never the raw extracted text.

---

1. MAJOR — the widened markdown regex is quadratic; a large issue description exhausts the webhook timeout

linear-attachments.ts:104 made the leading ! optional:

MARKDOWN_LINK_OR_IMAGE_PATTERN = /!?\[([^\]]*)\]\(<?(https:\/\/[^)>]+)>?\)/g

With ! required, the engine anchors each attempt at a literal ! and advances. With !?, it retries [^\]]* from every [ position. I measured it (Node, description consisting of unmatched [):

input size	with !? (this PR)	with ! required
4 000 chars	14 ms	0.00 ms
12 000 chars	110 ms	0.01 ms
50 KB	1 960 ms	~0 ms
100 KB	7 702 ms	~0 ms
150 KB	17 798 ms	~0 ms
300 KB	72 275 ms	~0 ms

Growth is quadratic rather than exponential — so I would not call it catastrophic backtracking — but the practical outcome is the same: a ~150 KB description blows a 30 s webhook-processor timeout, and an attacker (or an unlucky user pasting a large bracketed table) needs no special crafting.

SCAN_HARD_CAP = 100 (line 259) does not bound this: it caps selected.length, and the backtracking happens inside a single exec() before any match is produced. I also checked for a description-length cap upstream in linear-webhook-processor.ts — there is none.

Cheapest fixes: cap description length before scanning, or make the ! non-optional and run two passes, or replace [^\]]* with a bounded quantifier ([^\]]{0,200}).

2. MAJOR — deriveUploadIdentity collapses . and -, so distinct attachments de-dupe and one is silently dropped

linear-attachments.ts:228:

pathname.replace(/[^A-Za-z0-9_-]/g, '-').replace(/-+/g, '-')

., - and / all map to -, then runs collapse. So spec.v2.pdf and spec-v2-pdf (and spec-v2.pdf) produce the same id. Both collectLinearUploads (line 264) and the paperclip merge (line 407) de-dupe on that id with if (seenIds.has(id)) continue — so the collision is resolved by discarding the second file.

Failure: a user attaches design.v1.png and design-v1.png to one issue. The agent silently receives only the first, and the plan is built against incomplete material with no warning to anyone. Include a short hash of the full pathname in the id, or keep . distinct from -.

3. MAJOR — markWorkspaceRevoked is the default for every resolver caller but can never succeed

linear-oauth-resolver.ts:256. The PR's own comment is candid about it: "NOT YET EFFECTIVE IN PRODUCTION: every Lambda that resolves a token currently has READ-ONLY access to the registry table, so this write fails AccessDenied and is swallowed."

I confirmed that is accurate and stays true for the entire stack: every workspaceRegistryTable grant at s4 through s8 is grantReadData, and no slice adds grantReadWriteData/grantWriteData. So the auth-health revocation marker is permanently dormant, and the new doctor check can never reach its fail branch.

That is a defensible way to stage a change, but shipping it on by default means the code path exists, runs, fails, and is swallowed on every token resolution. Either add the grant in this slice or default the flag off until the grant lands — as written, the feature reads as working and is not.

4. MAJOR — the pdf-parse v2 rewrite is correct, but the bundling and the cited guard don't line up at this slice

The rewrite itself is right: on pdf-parse@2.4.5 the v1 shape mod.default ?? mod yields a non-callable object, and the new new PDFParse({data}).getText({first}) path is correct.

But at this slice only task-api.ts:542 and task-orchestrator.ts:238 carry nodeModules: ['pdf-parse']. The Lambdas newly reaching this code path need the same treatment, and the guard the error message points the reader to does not exist yet. Worth reconciling before merge so a PDF attachment on the new path doesn't fail at runtime with a bundling error rather than a screening verdict.

5. MINOR — private review-round citations used as the explanation for code

The cleanup pass caught the ABCA-### ids but left a class behind: bare review #N references that index a private review thread, used as the reason the code is shaped the way it is. 11 occurrences across 2 files — linear-attachments.ts lines 195, 248, 372, 414, 1022, 1039 (review #8, review finding #2/#1/#3, review #2 + #6) and linear-issue-context-probe.ts lines 54, 106, 124, 133, 274 (review #3, review finding #1/#5/#6). Also present: #299 Mode B and iteration-UX.

These are worse than a tracker id in one respect — #2 looks like a GitHub issue reference, so a public reader will follow it to an unrelated issue rather than recognising it as unresolvable. State the constraint instead of citing the round.

6. MINOR — the 300-line context probe has no test in any slice

linear-issue-context-probe.ts is introduced whole here (+300 lines, two exported functions, a 7-field public interface), and there is no linear-issue-context-probe.test.ts at s4, s5, s6, s7 or s8. Its only coverage is a jest.requireActual block inside linear-webhook-processor.test.ts that lands four slices later and asserts only the GraphQL field selection. This is the largest untested new module in the stack.

7. NIT — the destructured dynamic import silently types as any

attachment-screening.ts:282. The comment claims the destructured ({ PDFParse } = await import('pdf-parse')) form lets TS "infer its type from the value". It does not: let PDFParse; with no annotation is implicitly any, so parser, result, result.total and result.text are all unchecked. The pdf-parse.d.ts shim this slice ships is therefore not actually constraining this call site. Annotate the binding to get the contract you wrote the shim for.

---

Reviewed with Claude Code. Verification: tsc --noEmit on this slice's own base; the regex timed in Node at six input sizes against both variants; the SSRF allowlist, size caps and SCAN_HARD_CAP semantics read directly; the registry grant checked at every slice s4–s8.

[isadeks]

Review findings addressed

I had missed this review initially — apologies. Two of these were real bugs that would have shipped.

MAJOR #1 — quadratic markdown regex. Confirmed by measurement and fixed. I reproduced your numbers before touching anything (4 KB ≈ 7 ms, 12 KB ≈ 56 ms, 50 KB ≈ 940 ms), and confirmed your two supporting points: the scan cap does not bound it because the backtracking happens inside a single exec() before any match is produced, and there is no description-length cap upstream. Fixed with your third suggestion — a bounded label quantifier — since it caps work per start position without needing two passes or a new cap upstream, and a real markdown label is far below the bound. Pinned by a test on a 60 KB bracket-heavy description; reverting the bound makes it fail at ~1.4 s against a 1 s budget.

MAJOR #2 — . and - collapse so distinct attachments de-dupe. Confirmed and fixed. I ran your example: design.v1.png and design-v1.png both produce design-v1-png, and both de-dupe sites resolve that by discarding the second file. This is the worst finding in the review — a user attaches two files, silently gets one, and the plan is built against incomplete material with nothing logged. Fixed with your first suggestion (a short digest of the full pathname), keeping the readable stem for logs. Tested end-to-end through the public entry point: two such uploads now yield two records with distinct ids.

Both fixes were mutation-verified — reverting either one fails its own test and nothing else.

MAJOR #3 — markWorkspaceRevoked is defaulted on but can never succeed. Confirmed and fixed your way. I verified your claim holds for the entire stack: every workspaceRegistryTable grant from s4 through s8 is grantReadData, and no slice adds write. So the marker ran, failed AccessDenied, and was swallowed on every revoked refresh — the feature read as working while being permanently inert. Your two options were "add the grant here" or "default the flag off"; I took the second, since granting registry write to every token-resolving Lambda widens IAM for a feature nothing yet consumes. It is now opt-in, with a note to flip the default in the same change as the grant. The two tests that pinned the defaulted behaviour were rewritten to assert the new contract in both directions — nothing written when no recorder is supplied, and the recorder still carries the installation when one is.

MAJOR #4 (pdf-parse bundling), #5 (private review citations), #6 (context probe untested), #7 (dynamic import types as any) — #5 is fixed as part of the stack-wide shorthand sweep. The others I have not changed; #4's bundling contract test lands with the constructs it asserts on in #662 (and separately caught a real missing carve-out there, so that test is earning its place), and #6/#7 are tracked. Say the word on any of them and I will take them in this slice.

Gates: 2545 CDK tests, tsc/eslint clean.

[isadeks]

Second-round review: my regex fix was wrong in both directions

A second adversarial pass found the label bound both introduced a worse failure and failed to fix the one it targeted. Both verified by measurement.

It silently dropped attachments. A 300-char label bound makes a link with longer alt text stop matching entirely — the file vanishes, nothing is logged, and the agent plans against incomplete material. Driven through the real entry point, a 350-char label went from 1 attachment to 0. That is the same silent-data-loss failure this slice's other fix exists to prevent, and I introduced it while fixing a slow scan. Trading a DoS for lost user data is a bad trade. The bound is now 4096 — far past any real label — with a test pinning that a ~780-char descriptive label still yields its attachment.

And it did not fix the slow scan. BOTH variable parts of the pattern backtrack per start position, and the URL part dominates. On [](https://a repeated:

input	old pattern	with my label bound
25 KB	51 ms	54 ms
50 KB	222 ms	221 ms
100 KB	811 ms	822 ms

Identical. My new test only asserted '['.repeat(60_000) — the one shape the bound does fix — so it passed while the vulnerability stayed reachable. No crafting needed; a mangled paste produces it.

The real bound is on the input: a description over 64 KB has its tail left unscanned, and the truncation is logged rather than passed over silently. That fixes every hostile shape at once and is honest about the trade.

Both mutation-verified: restoring the tight bound fails the long-label test; removing the cap fails the URL-shape test (4.9 s against a bounded budget).

Two timing assertions replaced with assertions on observable work. Both passed alone and failed under full-suite load — a wall-clock budget inside a parallel suite measures CPU contention as much as the code, so those would have been flaky gates rather than guards. They now assert the truncation warning fired, which also proves the cap engaged and that it is announced.

On the revocation marker being dead code — the reviewer is right that making it opt-in leaves markWorkspaceRevoked with no caller and the doctor's FAIL branch unreachable, so the live-caught outage now surfaces as a WARN indistinguishable from a healthy idle workspace. I have not resolved that here, and want to be explicit that it is a real open gap rather than something I consider closed: the honest options are to add the registry write grant to the four resolver Lambdas and restore the default, or to delete the function and the doctor branch. Shipping neither leaves a hollow feature. My change only stopped it lying — it no longer fires a doomed write and swallows the AccessDenied. Confirmed live during deploy verification: on a genuinely revoked token the resolver logged the failure honestly and attempted no write.

Verified as still-correct: deriveUploadIdentity's re-signed-URL stability (two different signed URLs for the same pathname still produce the same id), the length maths (117 + 1 + 10 = 128 exactly, never over), and that no caller relied on the marker default.

[scottschreckengaust]

Verdict: Approve with nits

Approve with nits. S4 lands the Linear issue-context surface (attachment fetch + screening, context probe, error classification, auth-health diagnostics) at a genuinely high engineering bar — fail-closed everywhere it matters, mutation-verified regression tests, and IAM grants scoped to the exact action×ARN pair each read needs. I found no blocking issue in the incremental diff. The items below are non-blocking.

Reviewed as a stacked PR: diff scoped to base carve/s3-linear-nomcp-attachments, not main. I did not re-flag anything introduced by S1–S3.

Governance

• Backing issues #247 and #299 both carry approved (P0). Tracking epic #668 documents the 8-slice carve and why S4 stands alone. Gate satisfied per ADR-003.
• Branch carve/s4-linear-surface — the carve naming is a de-facto-waived deviation from (feat|fix|…)/<issue>-…, not a blocker.
• CI is green on all four checks (build agentcore, secrets/deps scan, dead-code, PR title).

Vision alignment

Fits the fire-and-forget default; escalate by policy and bounded blast radius tenets. The whole slice is about turning previously-silent failure modes (dropped events on a dead Linear grant, unscreened attachment bytes, mis-classified infra faults reading as user errors) into diagnosable, attributable, fail-closed conditions. linear-attachments.ts fetches untrusted uploads through the same Bedrock Guardrail pipeline as every other attachment, with SSRF guards and a fail-closed contract — it widens the input surface but bounds it. No tenet is traded; ADR-016 (deterministic Linear, no MCP) is the cited basis for admission-time hydration.

Blocking issues

None.

Non-blocking suggestions / nits

1. isPrivateIp misses hex-form IPv4-mapped IPv6 (cdk/src/handlers/shared/linear-attachments.ts, ~L520-540). The mapped-address regex /(?:::ffff:|::)(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/ only catches the dotted-quad textual form. dns.lookup can return the metadata endpoint as the hextet form ::ffff:a9fe:a9fe (169.254.169.254), which this regex does not match and the fe80–febf link-local check does not cover (its first hextet is 0000). This is defense-in-depth only — the host is allowlisted to uploads.linear.app, so reaching it requires Linear's own DNS to resolve internal, and redirect: 'error' closes the redirect vector — but if the SSRF guard is meant to be authoritative, normalize v4-mapped v6 by parsing the low 32 bits rather than string-matching the dotted form.

2. looksLikeBundlingBug regex includes a bare /pdfjs/i alternative (cdk/src/handlers/shared/attachment-screening.ts, ~L335). A genuinely corrupt PDF whose pdfjs parse error message merely contains the substring "pdfjs" would emit the LOUD "almost certainly a BUNDLING bug" operator diagnostic. Log-only, user-facing message stays generic, so impact is a possibly-misleading operator log line — consider tightening to the DOM-global / native-binding signatures (DOMMatrix|ImageData|Path2D|napi-rs\/canvas|Cannot find native binding) and dropping the bare pdfjs token.

3. Dormant IAM grant references a not-yet-existing GSI. The new dynamodb:Query grant on ${taskTable}/index/LinearIssueIndex (github-screenshot-integration.ts) is gated on props.taskTable, which stacks/agent.ts does not pass yet, and LinearIssueIndex is not on the real TaskTable (only JIRA_ISSUE_INDEX exists today). This is consistent with the carve's dormant-until-S8 strategy and the construct test builds its own table with the index, so it compiles and tests pass — just flagging that the grant is inert on main until the activation slice adds both the prop wiring and the GSI. Confirm #668's coverage check catches the GSI addition in its owning slice.

4. pdf-parse pinned to exact 2.4.5 (no caret) in cdk/package.json. Deliberate per the PR description (v2 API coupling), and yarn.lock agrees. Fine, but a pinned transitive-heavy dep like pdfjs will silently miss patch security fixes — worth a renovate/dependabot note so it doesn't rot.

Documentation

• ADR-016 is the cited basis and already exists; no ADR needed for this additive slice.
• New user-facing CLI surface — --verify-refresh, --decompose-allowed, --max-sub-issues, --max-parent-budget-usd, --no-force-consent, and the new linear_workspace_auth doctor check — is not reflected in docs/guides/USER_GUIDE.md. Non-blocking for a dormant-arc slice (decompose flags don't do anything until S6/S8), but the auth-health doctor check and --verify-refresh ARE live through the existing webhook path and would benefit from a USER_GUIDE mention. No Starlight mirror edits are needed since no docs/guides source changed in this PR.

Tests & CI

Strong. New/changed suites: linear-attachments.test.ts (+534), linear-auth-health.test.ts (+216), platform-doctor.test.ts (+128), plus attachment-screening, error-classifier, validation, screenshot-url, oauth-resolver, and the construct IAM-grant regression tests. Coverage hits the failure paths, not just happy paths: fail-closed on over-page/over-byte PDFs, unsupported types, magic-byte mismatch, SSRF, mid-batch S3 cleanup, revoked-marker installation-scoping race, refresh-persist-or-report-error. The two timing-based ReDoS assertions were correctly replaced with assertions on observable work (the truncation warning) to avoid a flaky wall-clock gate under parallel load. I independently reproduced the ReDoS bound: at the 64 KB SCAN_MAX_DESCRIPTION_CHARS cap the worst-case URL-shape scan is ~630 ms and bracket-flood ~1080 ms — comfortably under the 30 s webhook timeout.

Bootstrap synth-coverage: N/A. The diff adds only dynamodb:Query/GetItem IAM policy statements to an existing Lambda execution role — no new CloudFormation resource types (no new bucket/queue/secret/service). No cdk/src/bootstrap/* update required.

Test-perf (CDK synth): the new construct suite uses a single beforeAll App/Template.fromStack and does not re-enable bundling — compliant with #366.

Review agents run

• code-reviewer / correctness (/code-review high, base-scoped): ran over the full incremental diff (8 finder angles + verify). No CONFIRMED correctness bugs survived; the SSRF hex-mapped gap and the pdfjs log-regex breadth surfaced as low-severity/defense-in-depth and are folded in as nits 1–2 above.
• silent-failure-hunter (manual, error-handling scope): the diff is heavy on error handling; I traced every catch. Swallowed failures are all intentional and either logged loudly or converted to fail-closed rejections (S3 cleanup best-effort with 90-day lifecycle backstop; onAuthorizationRevoked best-effort so a marker write can't break token resolution; probe ok:false → attachment hydration fails CLOSED). No hidden-default failures found.
• type-design-analyzer (manual, new types): LinearAuthState/ProbeResult/RefreshVerifyResult discriminated unions and the SelectedUpload/LinearProbeAttachment shapes are tight and make illegal states unrepresentable (e.g. expired_indeterminate is deliberately un-collapsible to healthy). No shared handlers/shared/types.ts API shape changed, so no cli/src/types.ts sync needed.
• comment-analyzer (manual): comments are unusually thorough and accurate against the code; the ABCA-### tracker-id scrubbing (→ "observed in practice") matches the diff. No misleading comments.
• pr-test-analyzer (manual): covered under Tests & CI — coverage is good, mutation-verified.
• /security-review: ran mentally over the IAM (scoped Query+GetItem, least-privilege), SSRF guard (DNS pre-resolve + private-IP reject + redirect: 'error'), secrets (tokens read narrowly, never returned; rotated refresh token persisted-or-report-error), and input-gateway (fail-closed screening, magic-bytes authoritative over attacker-controlled label, bounded scan) changes. One residual defense-in-depth gap (nit 1); no blocking security issue.
• Omitted: no dedicated agent for pure-docs or infra-only scope was needed beyond the above.

Human heuristics

• Proportionality — pass. Complexity matches the problem. The revoked-marker opt-in-until-grant-lands design and the probe ok:false fail-closed are proportionate responses to real observed outages, not speculative abstraction. linear-attachments.ts at 743 lines is essential surface (fetch/SSRF/magic-bytes/screen/upload/cleanup), not accreted.
• Coherence — pass. linear-attachments.ts is a deliberate, documented analog of jira-attachments.ts (same select→fetch→screen→upload→record shape), reusing shared attachment-screening/validation helpers rather than re-implementing. Same concepts, same terms.
• Clarity — pass. Names communicate intent; error handling surfaces failures loudly rather than hiding them behind plausible defaults. The expired_indeterminate state is a model of refusing to fake certainty.
• Appropriateness — concern (minor). Integration is verified against a class-level PDFParse mock, not the real pdfjs worker (the code comments are candid that ts-jest can't drive the ESM worker and real extraction is validated on live deploy). This is the AI001 pattern — mocks matching self-written expectations — mitigated by the honest disclosure and the live-deploy verification plan, but the v2 extraction contract is not exercised end-to-end in CI. Acceptable given the constraint; worth a live-deploy smoke check before S8 activation.