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.}

[ClintEastman02]

Split into follow-ups per the triage — #756 stays open as the umbrella

Acting on the triage split. PR #788 (the gate + Category 3 suppressions) is now scoped as Part of #756 rather than closing it, and the remaining categories are tracked as children:

• chore(security): masking rule precision — stop flagging correct fail-closed code + fix mis-placed suppression (#756 Cat 0) #790 — Category 0: masking-rule precision (stop flagging correct fail-closed code) + fix the mis-placed observability.py suppression. P2
• fix(security): registry frontmatter parse failure erases publisher attribution — surface as typed rejection, don't suppress (#756 Cat 1) #791 — Category 1: security — registry frontmatter parse failure erases publisher attribution; surface as a typed rejection instead of {}. Note the two registry sites were newly nosemgrep-suppressed during the feat(registry): migrate to standalone Agent Registry #779 migration rather than fixed (the fix(security): clear the two security:sast findings blocking pre-push (#729) #730 trap). P1
• refactor(handlers): encode failure in best-effort lookups — 13 silent-success-masking sites (#756 Cat 2) #792 — Category 2: encode failure in the 13 best-effort lookups; dedupe the two resolvePrNumber copies; add logging to the bare catch at orchestration-reconciler.ts:1455. P2

#756 remains open until #790/#791/#792 land, since the full security:sast:masking scan (weekly security.yml + pre-push hook) still reports the remaining findings on main. PR #788's gate stops new drift but does not clear the existing debt.

Line numbers in the children were re-verified against the current PR #788 branch (the counts shifted from the original triage because #779 migrated the agentcore-client registry files). Credit to @scottschreckengaust for the original triage.

[scottschreckengaust]

Fresh inventory, since the counts in this thread (21) and the sibling threads (#766: 25, earlier readings: 46 → 19) have all drifted as fixes landed. Measured twice today with mise 2026.9.9 / semgrep 1.176.0, and the two runs agree exactly:

• main @ 25330c72 — mise run security:sast:masking → rc=1, 6 findings
• fix/879-insecure-object-assign @ a382fca2 (a merge of that same main) — rc=1, the same 6, byte-identical files and line numbers

#863 (Cat 2) took this from 19 sites to 6. What remains:

#	file:line	code	likely owner
1	agent/src/observability.py:86	return None	this issue
2	agent/src/server.py:1447	return ""	this issue
3	cdk/src/handlers/registry-publish.ts:116	return null;	this issue
4	cdk/src/handlers/shared/linear-oauth-resolver.ts:496	return null;	this issue
5	cdk/src/handlers/shared/orchestration-store.ts:350	return [];	this issue
6	cli/src/linear-oauth.ts:428	if (isNotFound(err)) return undefined; // genuine first install	#790 — false positive

Two things worth recording while the numbers are fresh:

#6 is why "fix the last one" can never make this gate green as the rule stands today. cli/src/linear-oauth.ts:428 re-throws on the very next line (throw err;), so the empty return is a correct fail-closed guard, not masking. The rule's pattern matches an empty return anywhere in the catch body, including inside an if guard — so adding a guard does not clear it; the return has to leave the catch block entirely. That is exactly the precision work #790 (Cat 0) describes, and until it lands, item 6 is unfixable-by-cleanup.

#542 looks closeable. Its six named findings (agent/src/config.py:392/406/428, cdk/src/handlers/shared/jira-feedback.ts:152, cli/src/commands/jira.ts:299, cli/src/commands/linear.ts:1629) appear in none of today's runs — all six are fixed. The symptom it reports ("security:sast:masking fails on main with 6 findings") is still true, but with an entirely different six, so leaving it open reads as unfinished work that is actually done. Not closing it myself; flagging for triage.

One thing this gate is now hiding. mise run security stops at the first failing leg and this one is 4th of 8, so //agent:security never runs. Measured leg-by-leg, it is also red — 14 fixable HIGH/CRITICAL trivy findings in image-bundled gh / uv / npm binaries, filed as #897. Clearing the 6 findings above will therefore not make mise run security (and so the pre-push hook) pass; #897 is the next wall.

[laithalsaadoon]

Three findings remain on main at 97152ae3, and two of them aren't in the list above. Measured
with the repo's own task (--config .semgrep/silent-success-masking.yaml --exclude '.semgrep/*',
semgrep 1.176.1 via mise):

site	in the 21-item list?	introduced
cdk/src/handlers/shared/orchestration-store.ts:350	yes, listed as :344	predates this issue
agent/src/server.py:1447	no	4d53a73b (#733, 2026-08-28)
cdk/src/handlers/shared/linear-oauth-resolver.ts:525	no	12c9b63f (#831, 2026-09-04)

All three read as Category 3: deliberate degraded-mode fallbacks whose reasoning already sits in a
comment or docstring beside the return, without a nosemgrep token. The gate is genuinely red and
none of the three is a vulnerability.

The part worth your time is why the two newer ones landed. #831 merged 2026-09-04, four days
after the ratchet (#788, c33befa0, 2026-08-31) became a required check. That check ran on its
head c772e4f0 and reported success, printing nothing in 3.6s.

Reproduced at those exact commits:

• whole-tree scan at the PR base 38ff380c: 18 findings
• whole-tree scan at the PR head c772e4f0: 19, the extra one being linear-oauth-resolver.ts:496
• --baseline-commit 38ff380c at that same head: 0

--baseline-commit subtraction pairs findings by rule, path, the returned literal, and the
occurrence index among identical matches in that file, and it applies nosemgrep after subtraction
rather than before. At the base, linear-oauth-resolver.ts carried three return null matches
(:363, :442, :475), all suppressed. At the head it carried five, with the new unsuppressed one
first. So the new finding took index 0 and paired with the base's suppressed :363, the four
suppressed ones shifted down a slot, and the two left without a base counterpart were filtered by
their own tokens. Nothing survived to report.

Five controls, each planting one masking violation at HEAD and running the ratchet with
--baseline-commit 97152ae3:

plant	position	literal	pre-existing matches in that file	ratchet
appended to validation.ts	after both	null	null:64 sup, undefined:168 sup	reports it
inserted above imports, validation.ts	before both	null	same	silent
inserted above imports, validation.ts	before both	{}	same	reports it
appended to logger.ts	n/a	null	none	reports it
inserted above, orchestration-store.ts	before	null	[]:350	reports it

With the pre-existing match unsuppressed, the substitution is visible directly. Unsuppress
validation.ts:64, commit that as the baseline, then insert a null-returning violation above it:
the ratchet reports one finding, at the shifted old line :74, and says nothing about the new
one at :38.

So the gate is blind in exactly the files most likely to gain a masking finding. At HEAD, 35 files
carry 64 raw matches and 61 of those are suppressed; a new same-literal finding at or above an
existing match in any of those files is invisible.

Nothing else catches it either. The whole-tree blocking scan is step 4 of [tasks.security], behind
security:sast, which has failed every weekly security.yml run from 2026-07-27 through
2026-09-14. Run 34842090188 stops at [//:security:sast] ERROR task failed, so masking, grype,
retire and gh-actions never execute.

Suggested landing: annotate the three, then the tree is at zero and security:sast:masking
(whole-tree) can replace security:sast:masking:range at
.github/workflows/security-pr.yml:105-:108. That removes the ratchet instead of working around a
semgrep matching detail, and on a clean tree the whole-tree scan takes about 19s here across 380
files. #766 asked for the ratchet as a stopgap while a full cleanup couldn't land; the cleanup is
now three annotations away.

drafted by Bonk

[scottschreckengaust]

Implementation notice (ADR-003) + fresh inventory

Picking up the remaining tail of this issue; draft PR is up. Flagging scope explicitly since @ClintEastman02 is assigned here: this touches only the three sites still live on main plus a reconciliation with #862, and nothing else — no overlap with work in flight elsewhere.

(The notice is late rather than skipped — my GH_TOKEN had expired and the replacement was a fine-grained PAT, which cannot write to an org repo no matter which permissions are added to it. Reads kept working because this repo is public, which is what made it look like a permission gap.)

The count is now 3, not 6 — #862 (#790) cleared half of them

Re-measured today against origin/main @ 9d4beda8, exported clean and scanned with the rule as it exists on main (61 total matches, 58 suppressed):

#	file:line	code	status
1	agent/src/server.py:1447	return ""	live → this PR
2	cdk/src/handlers/shared/linear-oauth-resolver.ts:525	return null;	live → this PR
3	cdk/src/handlers/shared/orchestration-store.ts:350	return [];	live → this PR
—	agent/src/observability.py	return None	cleared by #862 (marker placement)
—	cdk/src/handlers/registry-publish.ts	return null;	cleared by #862 (annotation)
—	cli/src/linear-oauth.ts:428	return undefined	cleared by #862 (rule predicate)

origin/main → rc=1. The PR branch → rc=0.

Item 6 resolved the way this thread predicted — via the rule, not an annotation

My 2026-09-15 comment said cli/src/linear-oauth.ts:428 was unfixable-by-cleanup until #790's precision work landed, because a guarded empty return inside the catch body cannot be annotated away without lying about the shape. #862 delivered exactly that: a pattern-not keyed on the order (a classified empty return before the fallthrough rethrow), which distinguishes it from the masked_conditional_reraise / maskedConditionalRethrow fixtures that must stay flagged. Both suppressions on that pair are now gone.

Worth recording that #862's own PR body initially argued this rule change was impossible and dropped it; @isadeks pushed back and it turned out to be deliverable. That review is the reason this issue can reach 0 without a permanent in-source blind spot.

I had a marker staged for that site on my branch. It is removed in the merge — a reviewable rule predicate beats a suppression, and the justification I had written for it ("the guard cannot be restructured to clear it") is now simply false.

Two deltas against #862 the PR proposes, both easy to reject

1. registry-publish.ts — structural instead of annotated. chore(security): masking rule precision — annotate correct fail-closed sites + fix nosemgrep placement (#790) #862 annotated the file's local parseBody. That function is a duplicate of shared/validation.parseBody, which seven sibling handlers already use and which carries an identical invalid JSON ⇒ null justification. The PR deletes the duplicate and imports the shared helper, so the repo ends with one suppression instead of two and one 400 message across all eight handlers. chore(security): masking rule precision — annotate correct fail-closed sites + fix nosemgrep placement (#790) #862's new malformed-JSON→400 VALIDATION_ERROR test passes unchanged — the contract is identical, which is the point. If you'd rather not churn a just-merged file, dropping this hunk still leaves the gate at rc=0.
2. observability.py — the degrade is made observable. chore(security): masking rule precision — annotate correct fail-closed sites + fix nosemgrep placement (#790) #862 fixed the marker so it binds (same root cause, found independently). What a correct marker cannot do is make the failure visible: a tracer fault and "no recording span" both return None and callers cannot tell them apart. The PR adds a log("WARN", …) breadcrumb plus two tests (fault → logged; no span → silent). Raising is not an option — callers read this inside DDB-write try blocks, where it would be misclassified as a DDB failure and trip the shared progress circuit breaker (feat(observability): end-to-end task attribution and cross-plane trace correlation #245 review).

Still true from before

Two of the live three (agent/src/server.py, and registry-publish.ts before #862 took it) are absent from this issue's original list of 21 — they landed after it was filed, behind an already-failing whole-repo leg. A red gate ratchets nothing; reaching rc=0 is what makes it start working.

And clearing this leg still does not make mise run security pass: //agent:security is red with 14 fixable HIGH/CRITICAL in the image-bundled gh / uv / npm binaries (#897, now approved), and the local pre-push security:sast leg is red on main from #879 (#880, unmerged).