chore(security): clean up 21 pre-existing silent-success-masking findings on main

[ClintEastman02]

Problem

mise run security:sast:masking (the py/ts-silent-success-masking rule, AI004 / #257) currently reports 21 un-suppressed findings that already exist on main. Because the check is wired into the pre-push git hook and scans the whole repo, every contributor hits this failure on git push regardless of what their branch touches — forcing --no-verify bypasses (e.g. PR #755).

These are pre-existing: none were introduced by recent feature branches. The full scan reports 79 findings, but 58 carry valid inline nosemgrep suppressions and are correctly ignored — only these 21 are genuinely unannotated.

This is separate from the PR-level security gate: security-pr.yml only runs secrets/deps/gh-actions, so the masking scan does not block PRs — it runs in security.yml on push-to-main / schedule. That job is effectively red on main for this rule today.

The 21 findings (file — line)

• agent/src/clarification_tool.py — 42
• agent/src/hooks.py — 1602
• agent/src/observability.py — 86
• cdk/src/handlers/github-webhook-processor.ts — 448
• cdk/src/handlers/jira-webhook-processor.ts — 1530
• cdk/src/handlers/linear-webhook-processor.ts — 532, 2757, 3057, 3108
• cdk/src/handlers/orchestration-reconciler.ts — 471, 1455, 1476
• cdk/src/handlers/shared/jira-feedback.ts — 372, 395
• cdk/src/handlers/shared/linear-feedback.ts — 280
• cdk/src/handlers/shared/linear-subissue-fetch.ts — 333
• cdk/src/handlers/shared/orchestration-rollup.ts — 490
• cdk/src/handlers/shared/orchestration-store.ts — 344
• cdk/src/handlers/shared/slack-api.ts — 112
• cli/src/commands/linear.ts — 1721
• cli/src/linear-oauth.ts — 382

Fix

For each line, do one of (per the rule's own guidance):

1. Add a justified inline # nosemgrep: <rule> -- <why the fallback is safe degraded-mode> on the return line, or
2. Re-raise / return a result shape that encodes the failure, if the swallow is actually a bug.

Most look like intentional best-effort/feature-detect fallbacks (e.g. except ImportError: return None for optional SDK; optional GitHub/Jira/Linear fetches returning null) → option 1. Each should be reviewed on its merits, not blanket-annotated.

Optional follow-up

Consider scoping the pre-push masking hook to the push diff rather than the whole repo, so contributors stop being blocked by untouched debt. That is a gate-semantics change and should be discussed separately.

Surfaced while addressing the PR #755 review (issue #641).

[scottschreckengaust]

Triage: re-scanned on main — 25 active findings, not 21, and 6 of them are not debt at all

Re-ran mise run security:sast:masking against origin/main (2cee8800). Counts from the emitted SARIF (test-reports/semgrep-silent-success-masking.sarif), splitting suppressions[] from active results:

	issue body	actual on main
total findings	79	83
suppressed (valid inline nosemgrep)	58	58
active / blocking	21	25

The 4 new ones all arrived with the registry stack in 4712e061 (#664), merged the same day this issue was filed: agent/src/registry/agentcore_client.py:84, cdk/src/handlers/registry-publish.ts:116, cdk/src/handlers/shared/registry/agentcore-client.ts:158,432.

That drift is the actual finding here. The rule is --error blocking but there is no ratchet and no PR-level gate, so the count grows on main while this issue sits unapproved. Fixing 25 today just resets a counter.

Also correcting the gate description in the body: security.yml triggers are workflow_dispatch + weekly Monday cron only — not push-to-main. So the scan blocks nothing in CI on a PR (security-pr.yml runs only security:secrets:range, security:deps, security:gh-actions), while hooks:pre-push:security → mise run security scans the whole repo on every contributor's git push. The local gate is strictly stricter than the merge gate — the same asymmetry class #730 called out, and the reason #755 needed --no-verify.

Framing: suppression is not the default remedy

Per the #730 precedent, blanket-annotating is the wrong shape of fix. #730 explicitly withdrew its own earlier suppressions ("that was the wrong call, and those suppressions are gone"), fixed both findings at source, and recorded the trap: build_opener(HTTPSHandler) would have satisfied the scanner while still carrying FileHandler/FTPHandler/DataHandler — a dodge that reads like a fix. A nosemgrep that quiets the scanner without removing the capability is worse than the finding, because it also removes the reminder.

So the categories below are ordered by what work is required, and the third bucket is deliberately small.

---

Category 0 — Not debt: rule false positives (4 sites) → fix the rule, add fixtures

These do not mask anything. Each returns an empty value from a guarded, classified branch and re-throws everything else. The rule's patterns match try { ... } catch ($E) { ... return $RET; ... } with no awareness of a sibling throw, so a correct fail-closed classifier trips it.

• cli/src/linear-oauth.ts:382 — if (isNotFound(err)) return undefined; immediately followed by throw err;. The surrounding 20-line docblock exists because of the fix(cli): linear setup clobbers a second workspace's webhook signing secret with the stack-wide one → 401 Invalid signature #611 secret-clobber incident and specifies "ANY OTHER error → THROW". This is the exemplar of correct handling and the scanner flags it.
• cdk/src/handlers/shared/registry/agentcore-client.ts:432 — if (err instanceof ResourceNotFoundException) return null; throw err;. null means absent, which is the function's documented domain (Promise<RegistryRecord | null>), not failed.
• cdk/src/handlers/registry-publish.ts:116 — parseBody returning null is the failure encoding; the caller converts it to 400 VALIDATION_ERROR at line 56-58. Malformed client JSON is not an exception, it is an expected input class.
• agent/src/observability.py:86 — already has a justified nosemgrep, on the wrong line. It sits on the except line above the return, and the rule uses focus-metavariable: $RET, so the suppression never binds. Verified: moving the identical comment onto the return None line drops the file from 1 active finding to 0.

Suppressing these teaches contributors that correct code needs annotations. Preferred fix: tighten the rule with a pattern-not for a catch block containing throw/raise, add the four shapes as fixtures under .semgrep/, and fix the comment-placement convention (the placement footgun is worth a line in the rule header, since 58 existing suppressions depend on getting it right).

---

Category 1 — Security-relevant: refactor, do not annotate (3 sites)

Registry frontmatter parsing, all from #664. These sit on the path the #664 review already flagged for SKILL.md frontmatter injection, and they convert a parse failure into a valid-but-empty trust decision.

• cdk/src/handlers/shared/registry/agentcore-client.ts:158 — parseSkillFrontmatter returns {} on yaml.load throw. Consumed by parseSkillPublisher (line 163), which recovers the publisher Cognito sub. A malformed-YAML record therefore yields publisher: undefined — indistinguishable from a record with no publisher. An attacker-supplied unparseable frontmatter is a way to erase attribution, and the docblock's injection argument ("a duplicate key is a YAML error") depends on that YAML error being surfaced, not swallowed into {}.
• agent/src/registry/agentcore_client.py:84 — the Python mirror, return {} on yaml.YAMLError, feeding _extract_runtime → the runtime payload actually loaded into tasks. A record whose runtime silently resolves to {} gets default runtime treatment rather than being rejected.
• cdk/src/handlers/shared/orchestration-store.ts:344 — parsePreScreenedAttachments returns [] when pre_screened_attachments_json is unparseable. [] here means "no attachments were pre-screened" and "screening state is unknown" — and children are released either way. The [] is load-bearing for a content-screening decision.

Fix shape: return a discriminated result ({ ok: true, value } | { ok: false, reason }) or raise a typed RegistryRecordMalformedError, and reject the record. These three are the ones I would not merge behind an annotation.

---

Category 2 — Tech debt (bad practice): refactor, cheap-to-medium (12 sites)

Best-effort network/DDB reads whose null/[] collapses "not found" into "lookup broke". Individually defensible, collectively the pattern that makes orchestration state hard to reason about. All already log, so the failure is observable — it just isn't actionable by the caller.

Cheap — widen the return type to encode failure:

• cdk/src/handlers/shared/slack-api.ts:112 — null conflates "no ts returned" with "Slack unreachable"
• cdk/src/handlers/shared/linear-feedback.ts:280 — GraphQL transport failure ≡ empty data
• cdk/src/handlers/shared/linear-subissue-fetch.ts:333 — "no parent" ≡ "parent lookup failed"; parent-vs-sub routing depends on this
• cdk/src/handlers/shared/jira-feedback.ts:372 and :395 — transition lookup; :395 masks invalid JSON from Jira, a different class from a network timeout
• cli/src/commands/linear.ts:1721 — queryLinearTeamKeys returns []; caller can't tell "no teams" from "auth failed", and the docblock admits the lookup silently degrades to scanning

Medium — these feed orchestration state, where a swallowed read changes control flow, not just presentation:

• cdk/src/handlers/orchestration-reconciler.ts:1476 and linear-webhook-processor.ts:2757 — duplicated resolvePrNumber/resolveChildPrNumber; identical body, both swallow. Worth extracting to one shared helper while fixing, so the fix lands once.
• cdk/src/handlers/orchestration-reconciler.ts:471 — combined screenshot read
• cdk/src/handlers/shared/orchestration-rollup.ts:490 — epic panel upsert; null becomes "no panel id" and the next edit silently posts fresh
• cdk/src/handlers/github-webhook-processor.ts:448 — findIterationReplyId; a masked failure causes a duplicate top-level comment instead of a threaded edit, so the user-visible symptom is real
• cdk/src/handlers/linear-webhook-processor.ts:532 — iteration ack reply

Note orchestration-reconciler.ts:1455 is a bare catch { return null; } with no logging at all (the pr_url read directly above resolvePrNumber) — strictly worse than its neighbours and the one I would fix first in this bucket.

---

Category 3 — Exceptional suppression defensible (6 sites)

Only where the empty value is genuinely the specified contract and a refactor buys nothing. Each still needs an inline justification on the return line naming why the caller may treat failure as empty success.

Guardrail fail-open (4) — jira-webhook-processor.ts:1530, linear-webhook-processor.ts:3057, :3108, and the already-suppressed sibling pattern. [] means "these advisory comments/docs were dropped"; the task proceeds by explicit design, the drop is logged, and encoding failure would only give callers a decision they have no policy for. Caveat worth a follow-up: fail-open on a content-screening outage is a deliberate security posture choice. It's defensible for advisory context, but it should be an ADR line rather than four independent inline comments — right now the same decision is re-litigated per call site.

Optional-dependency feature detect (1) — agent/src/clarification_tool.py:42, except ImportError: return None for claude_agent_sdk. Already # pragma: no cover with "SDK always present in the container". None is the documented "not registered" signal and the marker-based fallback covers it.

Advisory guard, fail-open by design (1) — agent/src/hooks.py:1602. The docblock states "ADVISORY ONLY … a guard bug can never wedge a healthy agent" and a false positive costs one extra comment. max_turns is the real backstop, so there is no caller decision to encode.

---

Suggested split

Current shape mixes a rule bug, a security fix, and a gate-semantics change into one issue. Proposal:

1. This issue — Category 0 (rule precision + 4 fixtures + fix the mis-placed observability.py suppression). Highest leverage: removes 4 findings without touching product code and stops the rule from punishing correct patterns.
2. New issue, Category 1 — registry frontmatter parse failures surface as typed rejections. Security-relevant, references the feat(registry): agent asset catalog on AgentCore — provisioning, port/adapter, API, CLI (#246) #664 review thread.
3. New issue, Category 2 — encode failure in best-effort lookups; dedupe the two resolvePrNumber copies.
4. New issue, gate semantics — add a security:sast:masking:range equivalent (mirroring the existing security:secrets:range) and wire the masking scan into security-pr.yml. Diff-scoping the pre-push hook without adding the PR gate would just make new violations invisible — the feat(registry): agent asset catalog on AgentCore — provisioning, port/adapter, API, CLI (#246) #664 drift above is the proof. Category 3's annotations should land only after this, so the count stops moving.

Happy to take #1 and #2. Neither is approved/assigned yet, so flagging for approved before anyone starts.

_{Verified against origin/main 2cee8800; all line numbers confirmed identical between local HEAD and origin/main for the 18 affected files.}