Skip to content

chore(security): ratchet silent-success-masking gate + clear 5 Category 3 findings (#756) - #788

Open
ClintEastman02 wants to merge 2 commits into
aws-samples:mainfrom
ClintEastman02:chore/756-masking-gate-cat3
Open

chore(security): ratchet silent-success-masking gate + clear 5 Category 3 findings (#756)#788
ClintEastman02 wants to merge 2 commits into
aws-samples:mainfrom
ClintEastman02:chore/756-masking-gate-cat3

Conversation

@ClintEastman02

@ClintEastman02 ClintEastman02 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Scopes issue #756 to the two pieces from @scottschreckengaust's triage that weren't claimed for follow-up issues: the missing PR-level gate (the actual cause of the drift) and the 5 genuinely-defensible Category 3 suppressions. Categories 0–2 (rule false positives, registry parse failures, best-effort lookups) are left for the separate issues proposed in the triage.

Area

  • agent — Python runtime / Docker image
  • cdk — infrastructure, handlers, constructs
  • tooling — root mise.toml, scripts, CI workflows

Related

Changes

The gate — a ratchet, not a full scan (fixes the drift). The blocking full-repo masking scan (security:sast:masking) only runs in security.yml (workflow_dispatch + weekly Monday cron only — not push-to-main) and the pre-push hook. security-pr.yml never ran it, so new findings land on main unchecked while every contributor is blocked on untouched debt on git push. That asymmetry is why the count grew 21 → 25 (the #664 registry stack) and why #755 needed --no-verify.

  • Add security:sast:masking:range to mise.tomlsemgrep --baseline-commit, so it reports only findings a branch ADDS relative to the base. Mirrors the existing security:secrets:range. Defaults baseline to origin/main so it's safe to run anywhere.
  • Wire it into security-pr.yml: the existing range-resolution step now also emits a baseline SHA (PR base / merge-group base / HEAD on dispatch), and a new step runs the ratchet. A PR that adds a masking finding now fails at PR time; the 25 pre-existing findings on main are not re-flagged.

The 5 Category 3 suppressions — genuinely-contract fail-open / feature-detect only, justified inline on the return line (repo convention; a comment above doesn't bind under the rule's focus-metavariable: $RET). Not blanket-annotated, per #730.

Site Why fail-open is the contract
cdk/.../jira-webhook-processor.ts (comments) Third-party comments are advisory enrichment; a screening outage drops them (logged), the reporter's task proceeds.
cdk/.../linear-webhook-processor.ts (project docs + comments) Same posture, mirrors the Jira processor.
agent/src/clarification_tool.py Optional-dependency feature detect for claude_agent_sdk; None is the documented "server not registered" signal, marker fallback covers it.
agent/src/hooks.py The stuck guard is advisory-only (see docstring); max_turns is the real backstop, so [] is "no steer this turn", not a masked failure.

Masking scan drops 25 → 20 active findings (semgrep test .semgrep/ still 2/2).

Scope note — this does not green main. The gate stops new masking findings at PR time; it does not clear the pre-existing debt. The full security:sast:masking scan (weekly security.yml + the pre-push hook) still reports the remaining findings, so main's weekly security suite stays red and the pre-push hook keeps forcing --no-verify until the follow-ups land. #790 (Cat 0), #791 (Cat 1 — registry attribution-erasure, security), and #792 (Cat 2) carry that remainder.

Verification

semgrep (scan + fixtures), ruff check/format, eslint (cdk), zizmor, and python -m py_compile all pass on the changed files. The security:sast:masking:range task was confirmed to exit non-zero on a newly-introduced finding and zero when only pre-existing debt remains.

⚠️ Committed/pushed with --no-verify: the agent-quality pre-push suite has an environment-specific test-isolation bug — test_registry_loader.py::TestMcpJsonNotCommittable::test_untracked_mcp_json_cannot_be_staged corrupts the real repo HEAD (commits a tree-deleting "init" commit and rewrites user.name/user.email) when run from the repo root, though it passes in isolation. Flagging separately; not touched by this PR.

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

…tes (aws-samples#756)

Adds a per-PR gate for the py/ts-silent-success-masking rule (AI004, aws-samples#257)
and clears the genuinely-defensible fail-open findings, per the triage on
aws-samples#756.

Gate (the actual fix for the drift): the blocking full-repo masking scan
only runs in security.yml (push-to-main/weekly) and the pre-push hook, so
new findings land on main unchecked while every contributor is blocked on
untouched debt (the aws-samples#664 drift, and why aws-samples#755 needed --no-verify). Add
`security:sast:masking:range` (semgrep --baseline-commit, mirrors
`security:secrets:range`) and wire it into security-pr.yml so a PR that
ADDS a masking finding fails at PR time without re-flagging the 25
pre-existing findings on main.

Suppressions (Category 3 only -- genuinely-contract fail-open/feature-detect;
justified inline on the return line per repo convention, not blanket-
annotated):
- jira/linear webhook processors: guardrail screening fail-open -- third-party
  comments/docs are advisory enrichment; a screening outage drops them
  (logged) and the reporter's task still proceeds.
- clarification_tool.py: optional-dependency feature detect for
  claude_agent_sdk; None is the documented "not registered" signal.
- hooks.py: the stuck guard is advisory-only; max_turns is the real backstop.

Masking scan drops 25 -> 20 active findings. The remaining 20 (rule false
positives, registry parse failures, best-effort lookups) are Categories 0-2
and tracked separately.

Note: committed with --no-verify because the agent-quality pre-push suite has
an environment-specific test-isolation bug (test_untracked_mcp_json_cannot_be_
staged corrupts repo HEAD when run from the root); all relevant checks
(semgrep, ruff, eslint, yaml, zizmor) were verified manually.
@ClintEastman02
ClintEastman02 requested review from a team as code owners August 25, 2026 14:23
@codecov-commenter

codecov-commenter commented Aug 25, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@9340b18). Learn more about missing BASE report.

Files with missing lines Patch % Lines
cdk/src/handlers/linear-webhook-processor.ts 0.00% 2 Missing ⚠️
agent/src/hooks.py 0.00% 1 Missing ⚠️
cdk/src/handlers/jira-webhook-processor.ts 0.00% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #788   +/-   ##
=======================================
  Coverage        ?   92.09%           
=======================================
  Files           ?      318           
  Lines           ?    87726           
  Branches        ?     9811           
=======================================
  Hits            ?    80787           
  Misses          ?     6939           
  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.

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