Skip to content

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

Merged
isadeks merged 19 commits into
carve/s1-foundation-contractsfrom
carve/s4-linear-surface
Jul 29, 2026
Merged

feat(carve S4): Linear issue-context surface — attachments, PDF/image screening, auth health#655
isadeks merged 19 commits into
carve/s1-foundation-contractsfrom
carve/s4-linear-surface

Conversation

@isadeks

@isadeks isadeks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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). Deliberately not Closes — no single slice closes it; it closes when S8 activates the arc. The auto-decomposition issue (#299) is no longer part of this carve — that feature stays on the development branch as experimental.

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

@isadeks

isadeks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

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 fileslinear-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
isadeks force-pushed the carve/s3-linear-nomcp-attachments branch from 77677a0 to fcc18a3 Compare July 27, 2026 17:11
@isadeks
isadeks force-pushed the carve/s4-linear-surface branch from c58d51b to efd1765 Compare July 27, 2026 17:28
@isadeks
isadeks force-pushed the carve/s3-linear-nomcp-attachments branch from fcc18a3 to 4b4a3d6 Compare July 27, 2026 17:51
@isadeks
isadeks force-pushed the carve/s4-linear-surface branch from efd1765 to 430576f Compare July 27, 2026 17:51
@isadeks
isadeks force-pushed the carve/s3-linear-nomcp-attachments branch from 4b4a3d6 to 868197c Compare July 27, 2026 18:40
@isadeks
isadeks force-pushed the carve/s4-linear-surface branch from 430576f to 227058d Compare July 27, 2026 18:40
@isadeks

isadeks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

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 #3markWorkspaceRevoked 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
isadeks force-pushed the carve/s3-linear-nomcp-attachments branch from 63ae670 to fd0834a Compare July 28, 2026 01:42
@isadeks
isadeks force-pushed the carve/s4-linear-surface branch from 2a8e7b2 to dadf00f Compare July 28, 2026 01:42
@isadeks

isadeks commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

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.

@isadeks
isadeks force-pushed the carve/s3-linear-nomcp-attachments branch from fd0834a to 4367dc5 Compare July 28, 2026 12:17
@isadeks
isadeks force-pushed the carve/s4-linear-surface branch from dadf00f to 1a3f6bf Compare July 28, 2026 12:17
@isadeks
isadeks marked this pull request as ready for review July 28, 2026 12:53
@isadeks
isadeks requested review from a team as code owners July 28, 2026 12:53

@scottschreckengaust scottschreckengaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@isadeks
isadeks force-pushed the carve/s3-linear-nomcp-attachments branch from 4367dc5 to 0398ec2 Compare July 28, 2026 18:52
@isadeks
isadeks force-pushed the carve/s4-linear-surface branch from 1a3f6bf to 0a2610a Compare July 28, 2026 18:52
isadeks added 2 commits July 28, 2026 22:47
Stress-tested the modest default on the workload the old 120 GB default existed
for: a full parallel `mise run build` of this monorepo (agent + CDK + CLI + docs)
on ECS, with the build gate actually running.

    memory  3132 MiB peak of 16384   → ~5x headroom
    disk    14.68 GiB peak of ~21    → ~1.4x headroom
    result  build_passed, ECS exit 0, no OOM, no ENOSPC

So CPU and memory are comfortably right, and for a reason worth recording:
`MISE_JOBS=1` serialises the packages, so peak memory is max-single-package rather
than sum-of-all. The old ceiling was never the binding constraint.

Disk is the outlier. 70% of the requested figure consumed by a single build with
a warm clone means a heavier dependency cache, or a second build sharing the task,
plausibly runs it out of space — and running out of space surfaces as a spurious
build FAILURE rather than an obvious resource error, which is the worst way for
this to present. 50 GiB restores real margin, and ephemeral storage is a small
fraction of the per-task cost next to vCPU and RAM, so the cost win that motivated
the modest default is intact.

Sized on measurement rather than on the assumption that the old numbers were all
equally over-provisioned. The docstring records the figures, and a test pins the
default so it cannot drift back to the floor unnoticed.
… adds it

Review found the planning task def inert in this PR: nothing sets
ECS_PLANNING_TASK_DEFINITION_ARN, so the strategy's
`readOnly && ECS_PLANNING_TASK_DEFINITION_ARN` guard is always falsy and
read-only workflows keep running on the build def.

The three wiring lines existed, but in the LAST slice of this series rather than
this one, so the feature works at the series tip and on the branch this was carved
from — which is why it live-tested fine — while this slice ships a task def that
is billed for and never used. Merges land one at a time, so there is a real window
where the def is deployed and unreachable, and anyone testing rightsized planning
in that window would conclude the feature is broken. Moved the wiring here, to the
slice that introduces the def.

`planningTaskDefinitionArn` is required rather than optional, matching its
siblings, so an ecsConfig that omits it fails to compile instead of silently
disabling the routing. No IAM change: the def shares the build def's task and
execution roles, and the ecs:RunTask grant is scoped by ecs:cluster rather than by
task-definition ARN.

Adds a stack-level assertion that the orchestrator's environment carries BOTH
ARNs and that they differ. This has to be at the stack level: the strategy's unit
tests set the env var by hand to exercise the routing branch, so they pass whether
or not the stack supplies it. Verified by deleting the wiring line and watching
both the test and tsc fail.

Also corrects four docstrings that still described a 64 GB / 16 vCPU build default
after it was lowered to 4 vCPU / 16 GB, one of which claimed a 64 GB task had been
OOM-killed when the measured figure was 32 GB.

Routes the ECS MAX_TURNS fallback through DEFAULT_MAX_TURNS instead of a literal
200. main uses 100, so the literal was an unflagged behavioral change, and it
disagreed with the hydrate path whenever a payload omitted max_turns — a turn
ceiling that depends on which code path filled it in.
@isadeks
isadeks force-pushed the carve/s3-linear-nomcp-attachments branch from ad34d22 to e011e0e Compare July 28, 2026 21:55
@isadeks
isadeks force-pushed the carve/s4-linear-surface branch 2 times, most recently from ff085fb to b2e4ac2 Compare July 28, 2026 23:54
isadeks added 8 commits July 29, 2026 02:49
… one workflow

These comments used the decomposition planner as the example for the read-only
planning task def and the artifacts bucket. With decomposition staying on the
development branch, that names a workflow main will not have — so a reader looking
for it finds nothing, and the comment reads as describing absent code rather than
the general rule.

The rule is what matters and is what the code keys on: a repo-ful workflow whose
terminal outcome is an ARTIFACT clones for context and delivers a document instead
of opening a PR, and a read-only workflow runs on the smaller task def. Reworded
around that, using coding/pr-review-v1 where a concrete read-only example helps.

Two tests used the planner purely as a convenient read-only example; repointed to
pr-review-v1. The properties they pin are unchanged and still real: a read-only
task inherits the repo's compute substrate rather than being downgraded to a
different substrate family, and it runs on the planning def rather than the build def.
…lint gate, plan/restack)

Slice 3 of the carve — the full autonomous-agent runtime, landed as ONE unit.
Stacked on S2. Unlike the CDK subsystems, the agent Python evolved as a tightly-
coupled whole: the changes to pipeline.py / repo.py / post_hooks.py / shell.py /
models.py interleave WITHIN shared files (a single pipeline.py carries the no-MCP,
decompose-plan, restack, and clarify-resume hunks). Splitting it along the CDK
subsystem lines produces non-compiling intermediate states, so it lands atomically:
all 27 agent/src modules + their tests + the two workflow definitions.

What it brings:
- Linear no-MCP: strip the Linear MCP server; the platform pre-hydrates comments +
  attachments deterministically instead of the agent fetching them at runtime.
- Per-repo build/lint verification gate before opening a PR (build_command /
  lint_command), with a structured verify outcome (passed / timed-out / infra-failed).
- Stacked-child branch handling + predecessor re-merge (for the sub-issue DAG).
- Agent-native decompose + restack workflows (clone, plan/re-stack, emit artifact).
- Attachment download + screening + version-pinned integrity read.

Behavioral note for reviewers: this also moves the default agent model and a couple
of runtime defaults to their current values — intrinsic to the runtime as it runs
today; called out here rather than split out, to keep the runtime internally consistent.

Gates: agent ruff + ty + full pytest (1421 tests) green. Files are verbatim from
the source branch (combination-verified: 0 diffs vs source). Comment cleanup for
public readability follows as a separate commit on this branch.
…ble-workflow set

The agent-vs-CDK cross-check for the writeable-workflow constant matched
only a single-line `frozenset((...))`. The formatter renders the same
literal across several lines once it grows past the line limit, and the
pattern then found nothing — so the check failed on a purely cosmetic
reformat instead of on a real drift between the two lists.

Allow whitespace between the frozenset call and its inner tuple so the
assertion tests the set's contents, which is what it is for.
…t-side changes

The agent-side files in this slice were taken wholesale from the branch
that carries the orchestration work. That branch predates the Jira
app-identity feature already on the target, so copying its versions over
silently dropped that feature from all 13 agent files — the app-actor
env plumbing, the per-task credential scrub, and their tests — while the
CDK, CLI and Forge halves of the same feature stayed. The result would
have merged as a half-reverted feature.

Reapply the agent-side changes as a three-way merge against the common
ancestor instead of a wholesale copy, so both survive: the app-identity
path and this slice's own agent changes. Verified per file that the
result is exactly "branch version + app-identity delta" with no other
drift, and that a symbol removed on purpose here (the Linear MCP
endpoint) stays removed.

One docstring conflicted, describing how Jira comments get posted; kept
the target's wording since it names the app actor and is now correct.
…e model the agent defaults to

Three fixes from the automated review of this slice, plus its comment cleanup.

**The self-reclone guard let `git clone -b` through.** `-b` is `gh`'s `--body`
short form, so it sits in the free-text-argument list, but it is ALSO
`git clone`'s own `--branch`. The guard truncated the command at the first
free-text flag BEFORE scanning for the clone verb, so the cut landed ahead of
the very verb it was looking for and an ordinary
`git clone -b main <own repo>` was allowed — reopening the stranded-work
failure the guard exists to prevent. Two more shapes slipped through for the
same reason: `gh repo clone -b main <own repo>`, and a clone chained after an
unrelated `-m` (`git commit -m wip && gh repo clone <own repo>`), where the
`-m` belongs to the commit, not the clone.

Now each shell segment is checked on its own, and within a segment the verb is
located first — a verb only counts as prose if a free-text argument opens
before it in that same segment. Prose in a `--body` value is still ignored.

**The agent's fallback model was not in the Bedrock grant list.** This slice
flips the fallback to Opus 4.8 for a repo that pins no model, but the IAM grant
is derived from `DEFAULT_BEDROCK_MODEL_IDS`, and that entry was landing several
changes later. Merging this slice on its own would have produced AccessDenied at
turn 0 on every task with no per-repo model. The grant now travels with the
default, and a drift guard reads the agent's own fallback and asserts the grant
covers it, so the two cannot diverge again.

**Two workflow descriptors arrived here rather than in the base slice**, matching
where their `agent/workflows/**` definitions live. `DESCRIPTORS` is the live
admission table, so an entry without its file means a submission is accepted
with a 201 and then dies when the agent cannot load it.

Also removes private tracker ids and work-item shorthand used as the load-bearing
explanation in comments, docstrings, log messages and test names. The reasoning
is preserved in plain language — a comment that recorded a real defect still
records it, just without an id only this team can resolve.
…to protect

A second review pass on my own fix found that it closed the `-b` bypass while
opening a new one and introducing a false positive. Both are worse than the
original bug in a security guard, so both are fixed and pinned.

**New bypass: a wrapped clone walked straight through.** Splitting on a bare
newline separated the verb from the repo, so `git clone \` + newline + url never
had the slug in the verb's segment. That command was BLOCKED before my rewrite
and ALLOWED after it — including via the `-b` form the rewrite existed to catch.
Backslash-continuations are now joined before scanning, which also covers a
continuation splitting the verb itself (`git \` + newline + `clone`) — the case
that genuinely needs the joining, since no segment handling reunites those.

**New false positive: a multi-line PR body was denied.** A `--body` or `-m` value
spanning lines is how an agent normally writes a PR or commit body, and that text
routinely documents a clone command. Treating `\n` as a command separator put the
quoted line in its own segment with no preceding free-text flag, so a legitimate
`gh pr create` was refused — exactly the false positive the free-text list exists
to prevent. A bare newline is no longer a separator; the real separators still are,
so a clone chained after an unrelated `-m` is still caught.

Also removes `_CLONE_VERB_RE`, which was never load-bearing: across every segment
shape it was the sole matcher zero times, and it actually MISSES a subshell
(`( git clone …`) that the anchored pattern catches. Dead alternatives in a
security path are worse than none.

Both regressions are mutation-verified: restoring `\n` as a separator fails the
multi-line-body test, and dropping the continuation join fails the wrapped-clone
test.
Review found the streamed path returning Popen's raw return code, which for a
signal death is the NEGATIVE signal number (SIGKILL -> -9). The OOM classifier
keys on the shell's 128+signal convention (137), so the two never met.

This is reachable, not theoretical. A verify command only goes through a shell
when it contains shell operators, and `mise run build` has none — so it is
exec'd directly and its signal deaths arrive as -9. Reproduced: is_infra_failure
returns False for -9 and True for 137, on identical input otherwise.

The consequence is the exact mislabel that function exists to prevent. The
container/cgroup OOM-killer writes "Killed process" to the KERNEL log, not the
build's stderr, so an OOM'd build frequently has no stderr signature at all and
the exit status is the only evidence. Left raw, that build is reported as a
genuine failure — telling the user their code is broken when the box ran out of
memory. Given the build tier's default was just lowered, this is the wrong
moment to have OOM detection silently not fire.

Normalized at the boundary where the signal is still visible rather than
teaching each consumer both encodings, so SIGTERM (-15 -> 143) is covered too.

Also replaces `proc.returncode or 0`, which mapped an unreaped None to 0. That
cannot happen after wait(), but success is the one unsafe default for an unknown
outcome, since it would let a build that never ran report as passing. Now -1.

Tests assert 137 reaches is_infra_failure as infra, and that None maps to
non-zero. Verified by restoring the old expression and watching the first fail.
…nd slices

Decomposition is being kept on the development branch as experimental rather than
landed on main. It presupposes one way of working — an issue split into a
sub-issue graph with a human approval gate on the proposed plan — and main should
not presuppose that. The rest of this arc is workstyle-neutral: iterating on a PR,
heartbeats, clarify-resume, and the compute substrate work regardless.

Removes from this slice the workflow YAML, its prompt, its registration in the
prompt map, its DESCRIPTORS admission entry, and the two prompt-injection blocks
that only a planning task used (the warm repo digest and the revise-round
edit-in-place directive, both keyed on channel_metadata nothing else writes).

Two things deliberately KEPT, because they are contracts rather than features:

- The repo-ful artifact branch in the pipeline. It is selected by the workflow
  contract (terminal_outcomes.primary == artifact AND requires_repo), not by a
  workflow id, and no shipped workflow currently takes it. Deleting it would
  remove a declared capability and the failure mode is bad — an empty PR opened
  for a document task, or a build run that was never wanted. Its test now
  synthesizes the workflow by copying the real coding workflow and flipping only
  those two fields, so the test cannot drift from the production contract the way
  a hand-built stub would. Verified by forcing the gate false and watching it fail.
- The trigger-label and :help helpers. Those are label plumbing, not decomposition.

Tests that used the planning workflow purely as a convenient read-only or
non-PR example now use coding/pr-review-v1, which is the surviving read-only
workflow — the properties they assert (a read-only task inherits the repo's
compute substrate; a non-PR workflow still takes the default clone path) are real
and still worth pinning.

Comments across the stack, cluster, orchestrator, and fan-out handlers described
this branch in terms of the planning workflow; they now describe artifact
workflows generally, since that is what the code actually keys on. No decompose
reference survives in this slice's source or tests.
@isadeks
isadeks force-pushed the carve/s3-linear-nomcp-attachments branch from 8058296 to b34b07f Compare July 29, 2026 01:51
isadeks added 4 commits July 29, 2026 02:52
… screening, auth health

Bring the Linear surface's issue-context and operator tooling onto main. All of
this is reachable today through the existing Linear webhook path; none of it
depends on the sub-issue orchestration arc, so it stands alone.

- linear-attachments.ts / 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 would.
- attachment-screening.ts: screen fetched attachments (images and PDFs) before
  they reach the model. This carries the pdf-parse v2 upgrade as one unit — the
  source uses the v2 `PDFParse` class, which requires pinning the dependency and
  dropping the hand-written module declaration that described the v1 function
  export. Splitting any of the three breaks the build, so they travel together.
- error-classifier.ts: distinguish transient compute faults from user-visible
  build failures, and report whether work was preserved when a run does not
  produce a pull request.
- screenshot-url.ts + github-screenshot-integration.ts: resolve screenshot URLs
  for the review path.
- CLI: `linear-auth-health.ts` + `platform-doctor.ts` surface an expired or
  revoked Linear authorization as a diagnosable condition instead of silently
  dropped events; `linear.ts` / `platform.ts` expose it.

The comments and test names in these files are rewritten to explain the what and
why directly rather than pointing at internal tracker ids, and two CLI strings
that printed an internal issue number to user-facing output are reworded.

Stacked on the agent-runtime slice. Full CDK suite 2598 passing, CLI 675
passing, both type-checks clean.
…pping an inert marker

**A quadratic regex could blow the webhook timeout.** Making the leading `!`
optional in the markdown-link pattern stopped the engine anchoring each attempt
on a literal `!`, so the unbounded label quantifier retried from every `[`.
Measured on a description of unmatched brackets: 4 KB ≈ 7 ms, 12 KB ≈ 56 ms,
50 KB ≈ 940 ms — and around 150 KB exceeds the processor's 30 s timeout. No
crafting is needed; a large pasted table does it, and the existing scan cap does
not bound it because the backtracking happens inside a single `exec()` before any
match is produced. The label quantifier is now bounded, which caps the work per
start position while still matching every real markdown label.

**Two attachments whose names differ only by `.` versus `-` silently became
one.** The upload id collapsed `.`, `-` and `/` onto the same character and then
squashed runs, so `design.v1.png` and `design-v1.png` produced an identical id —
and both de-dupe sites resolve a collision by discarding the second file. A user
who attached both got one, with nothing logged and no warning, and the agent
planned against incomplete material. The id now carries a short digest of the
full pathname, keeping the readable stem for logs.

Both fixes are pinned by tests that were mutation-verified: reverting either
change fails its own test and nothing else.

**The revoked-authorization marker was on by default and could never succeed.**
Every Lambda that resolves a token holds read-only registry access, and no slice
in this arc grants write, so the marker's write failed AccessDenied on every
revoked refresh and the failure was swallowed. The feature read as working while
being permanently inert, which is worse than being visibly absent. It is now
opt-in: a caller that holds the grant passes a recorder explicitly, and the
default is to attempt nothing. When the grant lands, flip the default in that
same change.
…ound lost attachments

A second review pass found my markdown-regex fix was wrong in both directions.

**It silently dropped attachments.** A 300-char label bound makes a link with
longer alt text stop matching entirely, so the file vanishes with nothing logged
and the agent plans against incomplete material. That is the failure this slice
argues elsewhere is the worst kind, and I introduced it while fixing a slow scan.
The bound is now 4096 — far past any real label — and a test pins 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, a 100 KB description takes ~820 ms with or without a label bound, and
larger inputs still exceed the webhook timeout. Bounding the label only fixed the
one shape the new test happened to assert.

The real bound is on the input: descriptions longer than 64 KB have their tail
left unscanned, and the truncation is LOGGED rather than passed over in silence.
That fixes every hostile shape at once.

Both are mutation-verified — restoring the tight bound fails the long-label test,
removing the cap fails the URL-shape test.

Two timing assertions are replaced with assertions on the observable work (the
truncation warning). A wall-clock budget inside a parallel suite measures CPU
contention as much as the code: both passed alone and failed under full-suite
load, which would have been a flaky gate rather than a guard.
…le docs

The project-mapping table's field list documented decompose_allowed,
max_sub_issues, and max_parent_budget_usd. Nothing in the main-bound slices reads
them, since decomposition stays on the development branch as experimental, and a
doc describing configuration the code does not honour is worse than no doc — an
operator would set those fields and wait for behaviour that never comes.

The table is schemaless apart from its partition key, so this removes only prose;
any row carrying those fields is still read back untouched.
@isadeks
isadeks force-pushed the carve/s4-linear-surface branch from b2e4ac2 to 5d3612a Compare July 29, 2026 01:54
`linear onboard-project` offered --decompose-allowed, --max-sub-issues, and
--max-parent-budget-usd. With decomposition staying on the development branch,
those wrote project-mapping fields nothing reads.

Worse than inert: the command printed "Auto-decomposition: ENABLED" and told the
operator which labels to apply, so the only feedback they got said it had worked.
They would then label an issue and get an ordinary single task, with nothing
anywhere explaining why.

Found by widening the search past cdk/ and agent/, where I had been looking. The
table itself is schemaless, so a project row that already carries these fields is
still read back unharmed — they are simply ignored.
@isadeks

isadeks commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Scope change: auto-decomposition is no longer part of this carve

Heads-up before you re-read this — every slice was force-pushed, so a review in progress is against a stale diff. Sorry for the churn; the reason is a scope decision, not a fix.

Auto-decomposition is staying on the development branch as experimental and is not landing on main. It presupposes one way of working: an issue split into a sub-issue graph, with a human approval gate on a proposed plan. main should not presuppose that. Everything else in this arc is workstyle-neutral — running a graph a human already authored, iterating on a PR from a comment, heartbeats, clarify-resume, the Channel abstraction, the compute plane — and those all stay.

What moved

~7,900 lines removed. 23 dedicated files, the agent workflow and its prompt, the admission-table entry, the CLI flags, and about 314 lines embedded in files that survive.

slice effect
#647 S1 unchanged
#653 S2 comments now describe artifact workflows by their contract, not by one workflow
#654 S3 workflow YAML, prompt, and admission entry removed
#655 S4 --decompose-allowed / --max-sub-issues / --max-parent-budget-usd removed from linear onboard-project
#656 S5 note-sweep renamed for what it does
#657 S6 8538 → ~2000 lines. Now purely the iteration slice; retitled
#659 S7 graph CREATION removed from the reconciler; graph EXECUTION kept
#662 S8 the plan-proposal surface removed; webhook 4502 → 3156 lines

Three things worth knowing as a reviewer

Some of the review fixes from the last round are gone with the feature. The verdict-marker fix (B1 on #662) and the retry-hint trigger_label fix (#659) were both decomposition-only. They were real fixes and they live on the development branch — they are not being dropped as wrong.

Two things looked decomposition-only and were not. DEFAULT_LABEL_FILTER and hasHelpLabel lived in a decompose-named module but are plain label plumbing, so they moved to trigger-label.ts rather than being deleted. And the note sweep is prefix-based and general, so it was renamed, not removed. Both would have been silent breakage.

Removing the feature exposed dead IAM. The reconciler held an s3:GetObject on the artifacts prefix plus put/delete on the attachments bucket, all for plan seeding. They had no caller left. Removed — a net least-privilege gain, and there is now a test asserting the reconciler has no object grant at all, which is the stronger claim since S3 does not normalize keys.

Verification

  • All 8 slices CI-green, chain integrity confirmed (each slice's base is its parent's tip)
  • 3436 CDK / 681 CLI / 1436 agent tests at the tip
  • Zero decompose references in every slice individually, not just at the tip — an earlier pass looked clean at the tip while intermediate slices still carried them, which would have landed decompose text on main temporarily during the serial merge
  • Deployed to dev and live-verified by artifact: the deployed webhook bundle contains no plan surface; coding/decompose-v1 is now rejected at admission (Unknown workflow_ref, 400); a normal coding task still runs end to end (PR opened with the requested file, exact contents); coding/pr-review-v1 still admits; the reconciler's IAM shows no S3 statements

…load the PDF

Every PDF attachment was being refused with "Attachment '<name>' could not be
stored." The S3 upload threw "Cannot perform Construct on a detached ArrayBuffer",
and because attachment handling is fail-closed the whole task was rejected.

pdf-parse transfers ownership of whatever it is handed to its worker, which
DETACHES the underlying ArrayBuffer. It was handed
`new Uint8Array(content.buffer, content.byteOffset, content.byteLength)` — a VIEW,
which shares its backing store with the caller's Buffer. The caller uploads those
same bytes to S3 after screening returns, so by then its buffer was dead.

The existing comment reasoned about this and still got it wrong: it said "slice to
the exact PDF bytes so a pooled Buffer's backing ArrayBuffer isn't handed over
wholesale", which correctly avoids donating a whole pooled buffer but still hands
over the caller's. A view narrows WHAT is transferred, not WHOSE it is.

Now `Uint8Array.from(content)` — a copy. That costs one allocation of an
already-size-capped payload, against silently rejecting every PDF upload.

The unit tests could not catch this: they mock the PDFParse class, so no transfer
ever happens and a view behaves like a copy. The new test asserts the OWNERSHIP
boundary instead of the outcome — same bytes reach pdf-parse, different backing
store — which is checkable without a real worker. Verified by restoring the view
and watching it fail.
Base automatically changed from carve/s3-linear-nomcp-attachments to carve/s1-foundation-contracts July 29, 2026 17:56
@isadeks
isadeks merged commit 57f6105 into carve/s1-foundation-contracts Jul 29, 2026
5 checks passed
@isadeks
isadeks deleted the carve/s4-linear-surface branch July 29, 2026 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants