refactor(handlers): encode failure in best-effort lookups — 13 silent-success-masking sites (#756 Cat 2)

[ClintEastman02]

What & why

Closes #792. Widens 13 best-effort read paths so callers can distinguish a genuine empty result from a lookup failure, closing the silent-success-masking class (AI004, #756 Cat 2) — with no nosemgrep suppressions in this bucket, per the issue's constraint. Every site now carries the failure in its result shape; three of them also act differently on it (see Behavioural delta — the other ten are type-widening, with the failure logged at the source).

Approach

New shared discriminated union LookupResult<T> (cdk/src/handlers/shared/lookup-result.ts):

• { ok: true, value } — found
• { ok: false, absent: true } (LOOKUP_ABSENT) — genuinely nothing
• { ok: false, error } (lookupFailed) — the lookup itself broke

Helpers: lookupFound, LOOKUP_ABSENT, lookupFailed, isLookupFailure, isLookupAbsent, lookupValueOr. The two ok: false variants share their tag, so the two guards — not switch exhaustiveness — are the intended narrowing; both are unit-tested as exact complements so a future state can't quietly satisfy both.

Converted reads

• slack-api.slackFetchTs
• orchestration-rollup.upsertEpicPanel
• linear-feedback.graphqlData
• linear-subissue-fetch.fetchIssueParentId
• jira-feedback
• github-webhook.findIterationReplyId
• linear-webhook.postIterationAck
• task-pr-number (extracted helper)
• CLI linear.queryLinearTeamKeys (local TeamKeysResult union)

Behavioural delta

Being explicit about which sites change what, since it's where the risk sits:

Ten sites are observability-neutral type-widening. slackFetchTs, upsertEpicPanel, graphqlData, postIterationAck, findIterationReplyId, resolveCombinedScreenshotUrl, readTransitionSnapshot, and readTaskPrNumber at two of its four callers collapse with lookupValueOr(r, null) at the call site with identical downstream control flow. That is a deliberate choice at the call site rather than a swallowed catch, and the failure is still logged at the source. Two of these gained a log they were missing: resolvePrUrl (the issue's "fix first" item) and the Jira app-actor !result.ok path.

Three sites route differently on failure:

Site	On lookup failure
linear-webhook-processor issue-parent lookup	Throws. Refuses to downgrade an orchestration child to the standalone path on a transient error, and the throw is what actually re-drives the delivery — see below.
linear-webhook-processor parent-epic sub-issue PR read	Answers. Posts a threaded "couldn't read it — comment again" reply and flips 👀 → ❓ rather than reporting the distinct, misleading "no PR yet".
CLI queryLinearTeamKeys	Reports ok: false, so the caller warns instead of persisting a workspace row that claims "no teams".

Why those two webhook sites differ: the processor is async-invoked (InvocationType: 'Event'), so a plain return is a successful invocation and Lambda discards the event — and the receiver has already written the dedup row (8h TTL) and 200'd Linear, so Linear's own redelivery is deduped away too. Processor-level async retries are therefore the only replay available. The issue-parent site can use them: nothing user-visible has been posted yet and everything above it is a read, so a throw is safe and idempotent. The parent-epic PR-read site can't: it has already won claimCommentAck (never released) and posted the 👀, so a retry would land on !won and no-op, leaving a permanent 👀 with no reply — hence the visible answer instead.

Testing

• tsc --build clean (cdk, cli)
• Full suites: cdk 4566 passed, cli 932 passed
• eslint clean on both packages with no mutations
• New unit tests for lookup-result.ts and task-pr-number.ts; behaviour tests for both changed webhook paths (absent-vs-failed on the issue-parent site with a standalone fallback available in both cases, and the visible answer + 👀 → ❓ on the PR-read site)
• Masking gate: security:sast:masking:range against origin/main reports 0 newly-introduced findings; all target findings for this bucket cleared, zero suppressions added

[codecov-commenter]

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

Codecov Report

❌ Patch coverage is 77.45098% with 115 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@5e10038). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
cdk/src/handlers/orchestration-reconciler.ts	44.89%	27 Missing ⚠️
cdk/src/handlers/shared/jira-feedback.ts	38.23%	21 Missing ⚠️
cdk/src/handlers/linear-webhook-processor.ts	78.94%	20 Missing ⚠️
cdk/src/handlers/github-webhook-processor.ts	31.81%	15 Missing ⚠️
cli/src/commands/linear.ts	78.57%	15 Missing ⚠️
cdk/src/handlers/shared/slack-api.ts	58.82%	7 Missing ⚠️
cdk/src/handlers/shared/linear-subissue-fetch.ts	60.00%	6 Missing ⚠️
cdk/src/handlers/shared/linear-feedback.ts	87.87%	4 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #863   +/-   ##
=======================================
  Coverage        ?   92.49%           
=======================================
  Files           ?      338           
  Lines           ?    97739           
  Branches        ?    10800           
=======================================
  Hits            ?    90404           
  Misses          ?     7335           
  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]

1 Verdict

REQUEST_CHANGES — the type design is sound and 11 of the 13 sites are safe type-widening, but the two sites that actually change control flow both introduce a silent drop justified by a replay mechanism that does not exist on this path, and the CLI site named in #792 still cannot tell "auth failed" from "no teams".

Governance: backing issue #792 is approved + P2 and assigned to the author — gate passes (ADR-003). Branch chore/792-encode-failure-lookups conforms. Scope matches the issue's site list closely, including the "fix first" item (resolvePrUrl's bare catch { return null; } now logs — genuinely fixed).

2 Vision alignment

LookupResult<T> advances tenet 3 (reviewable outcomes / observability) and the AI004 class in .semgrep/silent-success-masking.yaml without a single nosemgrep — good, and I verified statically (semgrep over all 13 changed source files with the repo's own rule) that the masking gate reports 0 findings on this diff.

The two blockers below trade tenet 1 (fire-and-forget default: "submit, walk away, review the outcome") away undocumented: on a transient Linear/DynamoDB error a user's @bgagent comment now produces either no response at all or a permanent 👀 with no follow-up and no retry. Turning fail-open into fail-closed is a legitimate design choice, but a fail-closed path must be durable (retry/DLQ) or visible (a reply saying so) — otherwise it converts "degraded but answered" into "silently unanswered", which is strictly worse for the tenet than the masking it replaces. Not an ADR-worthy trade once fixed; as written it is a defect, not a documented trade.

3 Blocking issues

1. The two "defer" paths silently drop the user's trigger — the "stream replay re-drives it" justification does not hold for this Lambda

cdk/src/handlers/linear-webhook-processor.ts:1936-1944 and cdk/src/handlers/linear-webhook-processor.ts:2100-2110.

Both new branches return; on a lookup failure, justified by the comments "Defer; a stream replay re-drives once Linear recovers" and "Leave the 👀 in place so a stream replay can re-drive it". This processor is not stream-driven:

• cdk/src/handlers/linear-webhook.ts:246 invokes it with InvocationType: 'Event'. A normal return is a successful async invocation — Lambda discards the event, no retry, no DLQ. Only a thrown error triggers the async retry budget.
• The receiver has already written the dedup row (cdk/src/handlers/linear-webhook.ts:221-230, 8h TTL) and returned 200 to Linear, so Linear's own redelivery is deduped away too. It is rolled back only on invoke failure, not on processor failure.
• I grepped for any replay/reconcile path over Linear webhook events; the only reconciler (reconcile-admission-queue.ts) re-drives QUEUED tasks, not webhook deliveries.

Failure scenarios:

• Site A (:1936): Linear returns 429/5xx/GraphQL errors for the IssueParent query. Nothing has been posted to the user yet on this path (no 👀 — the first reaction happens further down). The comment is dropped permanently and invisibly. Previously it fell through to handleStandaloneCommentTrigger, which resolves the task by issue id and does iterate the PR (no dependency cascade) — degraded, but the user got an ack and an iteration.
• Site B (:2100): a DynamoDB GetItem blip on the child TaskRecord. The 👀 was already posted at :2046, whose comment reads "ACK immediately — a parent comment is never silently dropped again." The new branch reintroduces exactly that: 👀 forever, no reply, no retry. Previously the user got a reply (retry pointer / disambiguation) — misleading wording, but actionable.

Fix (either, per site): throw after logging so the async-invoke retry budget actually re-drives the delivery (and note in the comment that the receiver's dedup row means processor-level retries are the only replay); or keep the return and make it visible — post a threaded "hit a transient error reading Linear/DynamoDB, please re-comment" reply and flip the reaction to needs_input (as the neighbouring !prNumberResult.ok branch already does). Please also correct both comments — "a stream replay re-drives" is not true here, and it is the kind of plausible-sounding justification this whole issue exists to remove.

2. queryLinearTeamKeys still cannot distinguish "auth failed" from "no teams" — and a renamed test now certifies the masking

cli/src/commands/linear.ts:2461-2487, test at cli/test/commands/linear.test.ts:667.

The new TeamKeysResult only reports failure for a non-2xx response or a thrown fetch. Linear's GraphQL API returns HTTP 200 with {"errors": [...]} for auth/scope/validation failures — the repo knows this and checks for it in exactly the two sibling readers this PR touched (cdk/src/handlers/shared/linear-feedback.ts:283 and cdk/src/handlers/shared/linear-subissue-fetch.ts:329). Here body.errors is never inspected, so body.data?.teams?.nodes ?? [] yields { ok: true, keys: [] }.

Failure scenario: a token minted without read team scope during bgagent linear setup. Linear returns 200 {"errors":[{"message":"Authentication required"}]}; the CLI prints no warning, persists the workspace row with no team_keys, and every later issue lookup silently falls back to scanning every workspace — the precise symptom #792 lists for this site ("caller can't tell 'no teams' from 'auth failed'"). The site is reported as converted in the PR body while the dominant failure mode is still masked.

Worse, the test previously named "returns [] when GraphQL response shape is missing teams.nodes" is now "succeeds with no keys when GraphQL response shape is missing teams.nodes" and asserts { ok: true, keys: [] } — it locks in the masked shape as intended behaviour.

Fix: check body.errors first and return { ok: false, error: body.errors }; treat a missing data.teams as a failure too (a real workspace always returns a teams connection, so { data: {} } is a malformed body, not an empty workspace — same reasoning the PR applies to slackFetchTs's missing ts). Keep an ok: true, keys: [] test only for a genuine { data: { teams: { nodes: [] } } }.

4 Non-blocking suggestions

• Only 2 of 13 sites change behaviour; the rest are observability-neutral. I enumerated every site: slackFetchTs (6 callers), upsertEpicPanel (6), graphqlData (12), postIterationAck (4), findIterationReplyId, resolveCombinedScreenshotUrl, readTransitionSnapshot, and readTaskPrNumber at 2 of its 4 callers are all collapsed with lookupValueOr(..., null) at the call site with identical downstream control flow. That is defensible — the failure is still logged at the source and the issue asked for type-widening — but the PR body's framing ("Every site encodes failure") is stronger than the behavioural delta. Two genuine wins worth calling out: resolvePrUrl gained the missing log (orchestration-reconciler.ts:1467, the issue's "fix first" item), and the Jira app-actor !result.ok path gained one (jira-feedback.ts:355).
• No tests for the only behaviour changes. There is no test for either new defer branch, and no unit test file for lookup-result.ts or task-pr-number.ts (both new). Codecov reports 41.79% patch coverage on linear-webhook-processor.ts (39 lines missed) — the uncovered lines are exactly where blocker 1 lives. The updated tests are honest but mostly re-type existing assertions (toBe('cmt-1') → toEqual(lookupFound('cmt-1'))); only orchestration-rollup.test.ts:383 and orchestration-channel-slack.test.ts:203 assert a failure result, and none assert a failure is propagated into different caller behaviour. Please add: parent-lookup failure ⇒ standalone path NOT taken and the delivery is retried/answered; PR-read failure ⇒ user-visible outcome.
• LookupResult is not single-tag discriminated (lookup-result.ts:39-42): two variants share ok: false, so switch exhaustiveness is impossible and narrowing to absent needs !('error' in r). A kind: 'found' | 'absent' | 'failed' tag (or at least an isLookupAbsent companion to isLookupFailure) would make a future 4th state a compile error instead of silently falling into today's !r.ok branches. Nothing in the diff is wrong today.
• readTaskPrNumber no longer logs at the source (task-pr-number.ts:47-49) — a deliberate move to the callers. I checked all four callers and all four log on failure, so nothing is lost; but the module doc promises the caller "can tell", not that it must log, so the next caller can silently drop it. Consider a logger.warn in the helper (callers already add context) or a doc line stating the caller MUST log.
• LOOKUP_ABSENT is used for misconfiguration, e.g. github-webhook-processor.ts:417 (if (!TASK_TABLE) return LOOKUP_ABSENT). A missing env var is a failure, not "genuinely nothing" — behaviourally identical today, semantically the conflation the type exists to end.
• CLI/CDK shape divergence: TeamKeysResult (keys, no absent state) duplicates the concept under a different name and field. The package boundary makes literal reuse awkward, so this is acceptable, but a one-line comment pointing at cdk/src/handlers/shared/lookup-result.ts would keep the two from drifting further.

Cleared (negative results — things I suspected and dismissed)

• iterateOrchestrationChild switching from args.prNumber ?? … to args.prNumber !== undefined (linear-webhook-processor.ts:2202) — prNumber?: number so the only divergence is 0, not a valid PR number. Fine.
• resolvePrUrl dropping its optional-taskId guard — its sole caller passes evt.taskId: string. Fine.
• spawnRestackTask returning 'failed' for both absent and failure (orchestration-reconciler.ts:1392-1412) — identical to the previous behaviour, with the two cases now logged distinctly. Fine.
• graphqlData's body.data ? found : ABSENT vs the old ?? null — no reachable divergence.
• Contract parity: cdk/src/handlers/shared/types.ts is untouched, so no cli/src/types.ts sync is owed.
• Duplicate-work check: the resolvePrNumber/resolveChildPrNumber dedupe requested by #792 landed correctly as one shared helper; no leftover copies (grep confirms both originals deleted).

5 Documentation

No documentation was required and none shipped, which I believe is correct: the diff touches no docs/, CONTRIBUTING.md, contracts/, cdk/src/constructs/, or cdk/src/stacks/ path (verified with git diff --name-only against the merge-base). The new modules are internal handler helpers and are documented in-file with substantial, mostly accurate doc comments.

Starlight mirror sync: not applicable — no docs/guides/, docs/design/, or CONTRIBUTING.md edits, so docs/src/content/docs/ cannot be stale from this PR.

One documentation defect is inside the code and is part of blocker 1: two comments assert a replay mechanism that does not exist on this Lambda. The rest of the comment work is unusually good — the lookup-result.ts header and the per-site rationale on graphqlData/upsertEpicPanel/fetchIssueParentId accurately state why a collapse is safe rather than hand-waving it.

6 Tests & CI

• Coverage: see the second bullet in §4. Adequate for the type migration, absent for the behavioural change — which is the inverse of where the risk sits. Codecov reports 68.5% patch coverage overall with 129 lines missed.
• Actual check status observed at review time: all four checks were still pending — Dead-code detection (advisory), Secrets, deps, and workflow scan, Validate PR title, build (agentcore). mergeStateStatus: BLOCKED, mergeable: MERGEABLE. I am not asserting the suite passes.
• Not executed by me: this is a fork PR, so I did not run yarn/jest/tsc from the head. The PR body's "cdk 4471 passed / cli 903 passed / tsc clean" claims are unverified here; the one thing I could check without execution is the masking gate, which I ran with semgrep --config .semgrep/silent-success-masking.yaml over all 13 changed source files: 0 findings, consistent with the "zero suppressions, zero new findings" claim for these files.
• CDK test-perf rule #366: not applicable — no test in this diff synths a stack or touches bundling context.
• Bootstrap policy coverage (ADR-002): not applicable. The diff adds no construct or stack and therefore no new CloudFormation resource type — git diff --name-only shows nothing under cdk/src/constructs/, cdk/src/stacks/, or cdk/src/bootstrap/, so no resource-action-map.ts entry, BOOTSTRAP_VERSION bump, regenerated artifact, or DEPLOYMENT_ROLES.md baseline change is owed.

7 Human heuristics

• Proportionality — pass. A 63-line shared union for 13 sites across 5 subsystems is the right size; it is not a bespoke abstraction for a one-off, and the task-pr-number.ts extraction removes a real duplicate the issue asked to dedupe.
• Coherence — concern. The concept is expressed twice under different names/shapes: LookupResult<T> in cdk/src/handlers/shared/lookup-result.ts:39 vs TeamKeysResult in cli/src/commands/linear.ts:2449. Justified by the package split, but the divergence should be noted in-code so the two don't drift.
• Clarity — concern. Names are good (lookupFound / LOOKUP_ABSENT / lookupFailed read exactly right), but lookupValueOr is a one-keystroke way to discard the very fact the type exists to carry, and it is used at ~30 of ~34 call sites. More seriously, the two comments at linear-webhook-processor.ts:1938 and :2102 are precisely the "plausible default behind a confident justification" failure mode this PR is meant to eliminate — see blocker 1.
• Appropriateness — concern. Maintainable by this team and idiomatic for the codebase, but the CLI path is verified only against self-written mocks: the tests never exercise Linear's real 200-with-errors response, which is why blocker 2 survived, and the renamed test asserts what the code currently does rather than what it should do.

[ClintEastman02]

Thanks — both blockers were right, and digging into the second one turned up a wrinkle that changed the fix. Pushed as 89d3572.

Blocker 1 — the two linear-webhook-processor sites

You offered "throw or answer" interchangeably per site. They actually need different answers, because of claimCommentAck.

First, confirming the retry semantics you flagged: the processor is invoked with InvocationType: 'Event' from linear-webhook.ts, so a plain return is a successful invocation and Lambda discards the event. Worse, the processor's own async retries are the only replay available — the receiver has already written the dedup row (8h TTL) and 200'd Linear, and it rolls that row back only on invoke failure, so Linear's redelivery is deduped away, and no reconciler re-drives webhook deliveries. I also checked linear-integration.ts: no DLQ and no retry override, so it's the default 2 retries. A bare return on a lookup failure was a permanent, invisible drop either way.

Issue-parent lookup (handleCommentTrigger) → throws. Nothing user-visible has been posted on this path yet (the 👀 goes out downstream) and everything above the lookup is a read, so a retry is safe and idempotent. It also stops the specific bug: falling through to the standalone path on a transient error silently downgrades an orchestration child, losing the dependency cascade.

Parent-epic sub-issue PR read (handleParentEpicCommentTrigger) → answers instead. This is the wrinkle: by the time we reach that read, the handler has already won the one-time claimCommentAck and posted the 👀 — and claimCommentAck has no release function. So throwing to spend the retry budget would re-drive straight into !won and no-op, leaving a permanent 👀 with no reply. The only durable outcome is a visible one, so it posts a threaded "I couldn't read PR-N's pull request — nothing was started, please comment again" and flips 👀 → ❓, exactly as the neighbouring "no PR yet" branch does. That also keeps the two states distinct, which was your underlying point.

Blocker 2 — queryLinearTeamKeys and Linear's 200-with-errors

Correct, and this was the real hole: Linear's GraphQL API answers auth/scope/validation failures with HTTP 200 plus an errors array, so res.ok told us nothing and data?.teams?.nodes ?? [] turned an auth failure into a confident "this workspace has no teams" — which then got persisted. Now:

• body.errors present → { ok: false, error: body.errors }
• body.data.teams missing entirely → { ok: false, error } (distinguished from a present-but-empty nodes)
• only a genuinely empty nodes yields ok: true with []

Added a doc paragraph on the function spelling out the 200-with-errors behaviour and pointing at the sibling readers so the next person doesn't re-introduce it.

Non-blocking items picked up

• isLookupAbsent guard added. Since the two ok: false variants share their tag, switch exhaustiveness isn't available — the guards are the intended narrowing, so they're now unit-tested as exact complements over the two variants.
• LOOKUP_ABSENT doc hardened to say a precondition that never let the query run is a failure, not absence.
• findIterationReplyId unconfigured-TASK_TABLE case switched from LOOKUP_ABSENT to lookupFailed. Behaviourally identical for today's sole best-effort caller, but it was exactly the conflation this type exists to end. I audited the other return LOOKUP_ABSENT sites — orchestration-reconciler.ts:453 (if (!taskId)) is a genuine absence and left alone.
• task-pr-number doc now states the caller obligation explicitly: the helper deliberately doesn't log, because every caller holds orchestration/task context worth logging alongside the cause — so a caller that neither logs nor branches on isLookupFailure re-creates the drop the extraction removed.
• TeamKeysResult shape divergence — kept CLI-local (the CLI doesn't depend on cdk/), but its doc now cross-references cdk/src/handlers/shared/lookup-result.ts as its twin.
• PR body framing softened per your note: it now says every site carries the failure and three of them act differently, with a ### Behavioural delta section naming the ten type-widening sites and tabling the three that route differently.

New tests

• cdk/test/handlers/shared/lookup-result.test.ts — the module was untested; covers falsy lookupFound values, LOOKUP_ABSENT having no error key, non-Error and undefined causes, the guards as exact complements, and lookupValueOr collapsing both.
• cdk/test/handlers/shared/task-pr-number.test.ts — 10 tests including pr_url fallback parsing, preference order, and failure-not-absence when the DynamoDB read throws.
• Both changed webhook paths: an absent-vs-failed pair on the issue-parent site (with a standalone fallback available in both cases, so "no task created" can only mean the fall-through was refused), and the visible answer + 👀 → ❓ on the PR-read site.

Verification

tsc --build clean; eslint clean on both packages with no mutations; cdk 4566 passed (217 suites), cli 932 passed (62 suites); security:sast:masking:range against origin/main reports 0 newly-introduced findings with zero nosemgrep added. Rebased onto the current branch tip, which picked up #870 — osv-scanner across all three lockfiles now reports no issues.

another reviewer want to review

[scottschreckengaust]

1. Verdict

Approve (with minor nits). A disciplined, well-scoped fix for the silent-success-masking class (#756 Cat 2 / AI004): a shared LookupResult<T> discriminated union replaces 13 best-effort reads that collapsed "genuinely absent" and "the read broke" into the same bare null/[]. 10 sites are observability-neutral type-widening (failure still logged at source, collapsed at the call site with an explicit lookupValueOr), and 3 route differently on failure. The two prior blockers (DISMISSED review by @isadeks) are genuinely resolved in this head, not merely re-worded.

2. Vision alignment

Directly advances reviewable outcomes and bounded blast radius. Turning a swallowed catch into a typed, logged, sometimes-retried failure makes control-plane faults observable instead of masquerading as empty results — exactly the reliability posture VISION.md asks for. The linear-webhook-processor issue-parent site is the sharpest win: a transient Linear error no longer silently downgrades an orchestration child to the standalone path (losing the dependency cascade). No tenet is traded; no ADR required.

3. Blocking issues

None. I independently re-verified the two blockers from the earlier DISMISSED review against the current head:

• Prior blocker 1 (silent drop on the two linear-webhook-processor defer paths). Resolved correctly and — importantly — differently per site, which is the right call:
  • handleCommentTrigger issue-parent lookup now throws (linear-webhook-processor.ts:1946-1961). I traced the flow: the throw is reached only after the unmapped-commenter gate has returned and before any reaction/reply is posted (the 👀 is posted downstream), so nothing user-visible has happened and everything above it is a read — a throw spends the async-invoke retry budget idempotently. The justification comment now names the real mechanism (InvocationType: 'Event', dedup-row rollback on invoke failure) rather than a non-existent stream replay.
  • handleParentEpicCommentTrigger PR read now answers (posts a threaded "couldn't read it — comment again" reply + flips 👀 → ❓) because claimCommentAck is already won and never released, so a throw would re-drive into the !won no-op and strand a permanent 👀. Correct.
• Prior blocker 2 (queryLinearTeamKeys couldn't tell auth-failure from no-teams). Resolved: errors is now checked before data (Linear answers auth/scope failures with HTTP 200 + errors), a missing teams connection is treated as a malformed body, and only a present-but-empty nodes yields ok: true, keys: []. Tests cover the 200-with-errors, partial-data-with-errors, no-connection, and genuine-empty shapes against real response bodies.

4. Non-blocking suggestions / nits

1. Concept expressed twice (coherence). LookupResult<T> (cdk/src/handlers/shared/lookup-result.ts) and TeamKeysResult (cli/src/commands/linear.ts) are the same idea under two shapes. The package boundary (CLI must not import from cdk/) justifies the split, and both now carry cross-reference doc comments — good enough. If a third CLI reader ever needs this, consider a cli/src/-local shared twin rather than a per-function union.
2. lookupValueOr ubiquity. It appears at the large majority of call sites and is a one-token way to discard the exact distinction the type exists to carry. That is the documented, deliberate choice for purely best-effort callers (failure logged at source), so it is fine — but it does mean the type's value lives almost entirely in the 3 branching sites plus the source-level logging. No change requested.
3. orchestration-reconciler.ts:1011 calls upsertEpicPanel(...) fire-and-forget (return discarded). Widening its return to LookupResult<string> is harmless here; noting it only so a future reader doesn't expect a persisted comment id on that path.
4. readTaskPrNumber logging is a caller obligation (documented). All four current callers honor it (branch on isLookupFailure and/or log). Worth keeping an eye on in review of any future caller — a caller that neither logs nor branches re-creates the silent drop this extraction removed.

5. Documentation

No docs owed. This is an internal error-handling refactor: no user-facing contract, env var, or command changes except one new best-effort CLI warning line (team-keys read failure during linear setup/add-workspace), which is self-describing and does not rise to a guide/ADR update. No docs/guides/, docs/design/, or CONTRIBUTING.md edits → no Starlight mirror sync needed. Backing issue #792 is filed, approved, labelled P2/infra-cdk, and self-assigned to the author.

6. Tests & CI

Strong coverage. New lookup-result.test.ts locks the guards as exact complements over the two ok:false variants (so a future third state can't satisfy both) and asserts falsy found values aren't re-collapsed. New task-pr-number.test.ts covers pr_url fallback parsing, preference order, absent-vs-failure, and failure-not-absence on a throwing read. Behaviour tests pin both changed webhook paths: absent-vs-failed on the issue-parent site (throws to earn a retry) and the visible answer + 👀 → ❓ on the parent-epic PR-read site. All existing return-shape assertions updated. CI: all checks green (build (agentcore), secrets/deps/workflow scan, dead-code advisory, PR-title validation). PR body reports cdk 4566 / cli 932 passing, tsc + eslint clean, and security:sast:masking:range = 0 newly-introduced findings with zero nosemgrep added (consistent with #792's constraint) — unverified by me from the fork head but consistent with the passing CI.

Bootstrap policy coverage (ADR-002): not applicable. The diff touches only cdk/src/handlers/** and cli/src/**; nothing under cdk/src/constructs/, cdk/src/stacks/, or cdk/src/bootstrap/, so no new CloudFormation resource type is introduced — no resource-action-map.ts entry, BOOTSTRAP_VERSION bump, regenerated artifact, or DEPLOYMENT_ROLES.md baseline change is owed.

CDK test-perf (#366): not applicable — no test in this diff synthesizes a stack or re-enables bundling.

7. Review agents run

Execution-context limitation: this review runs in a subagent that cannot spawn nested pr-review-toolkit agents. I performed the equivalent analysis by hand and state that explicitly here, per the process requirement.

• silent-failure-hunter (by hand): primary lens for this PR. Verified every converted read now surfaces failure (typed + logged) and that the 3 branching sites route correctly; confirmed the 10 lookupValueOr collapses are deliberate and logged at source.
• type-design-analyzer (by hand): LookupResult<T> is a sound discriminated union; the two ok:false variants share a tag so switch exhaustiveness is unavailable — this is documented and covered by complementary, unit-tested guards.
• code-reviewer (by hand): style/guideline conformance, no orphaned references to the removed resolvePrNumber/resolveChildPrNumber, all callers of widened signatures updated.
• comment-analyzer (by hand): the two rewritten justification comments now match real Lambda async-invoke/dedup mechanics (verified against the described receiver behaviour).
• pr-test-analyzer (by hand): coverage assessed above — happy path, absent, and failure paths all exercised, including real Linear 200-with-errors bodies.
• /security-review: omitted — no IAM, Cedar, network-topology, secrets, or input-gateway change; this is error-typing over existing reads only.

8. Human heuristics

• Proportionality — pass. An 85-line shared union + a 59-line extracted helper for 13 sites across 5 subsystems is right-sized, not a bespoke abstraction for a one-off; task-pr-number.ts removes a genuine byte-for-byte duplicate.
• Coherence — pass (minor). Same concept, two names (LookupResult / TeamKeysResult) forced by the package boundary; both now cross-reference each other in-doc, mitigating drift.
• Clarity — pass. lookupFound / LOOKUP_ABSENT / lookupFailed read exactly right. The two previously-suspect webhook comments now state the real mechanism rather than a plausible-but-false one.
• Appropriateness — pass. Maintainable and idiomatic; the CLI path is now tested against Linear's real 200-with-errors shape, not just self-written happy-path mocks, closing the gap that let blocker 2 originally survive.