Skip to content

docs(sast): fleet SAST gate canary results #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
escote-tatari wants to merge 1 commit into
base: main
Choose a base branch
from
Open

docs(sast): fleet SAST gate canary results #42

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 96 additions & 0 deletions docs/design/2026-07-03-fleet-sast-canary-results.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
# SAST gate canary -- results

Pre-registration evidence for adding `sast.yaml` to org ruleset #126206.
Companion to `2026-07-03-fleet-sast.md`. Question under test: which failure
modes let the required-workflow run conclude `success` (non-blocking, as
designed) vs. which fail/cancel the run and would HARD-BLOCK merges fleet-wide.

## Test bed A -- run conclusion by failure mode

Run as repo-local `on: push` workflows on branch `sast-canary-tests` in
`tatari-tv/github-required-workflows` (excluded from #126206, so zero fleet
risk). Measures the RUN conclusion only; the real required-check gate effect is
test bed C.

| # | Mode | Workflow | Run conclusion | continue-on-error absorbed? | Notes |
|---|------|----------|----------------|------------------------------|-------|
| 1 | scanner finding (guarded, exit 1) | canary-findings | success | yes | intended non-blocking behavior; control |
| 2 | YAML syntax error | canary-yaml-error | failure | n/a (file never parsed) | repo-local still produced a queryable failing run |
| 3 | unresolvable `uses:` (guarded) | canary-bad-uses | success | YES (job conclusion=failure, run=success) | OVERTURNS handoff hypothesis that bad uses fails the run |
| 4 | forced timeout (guarded) | canary-timeout | cancelled | NO | timeout-minutes:1 + sleep 120; step + job + run all cancelled |
| 5 | cancellation | (= mode 4) | cancelled | NO | not directly exercised (gh run cancel blocked by perms); timeout proves a cancel is not absorbed |
| 6 | un-guarded job fails | canary-unguarded | failure | n/a (no guard) | proves the guard is per-job; any future job without the line re-arms blocking |

### Corrected model of `continue-on-error: true`
- Converts a failed step/job into run conclusion `success`. Covers findings (1)
AND unresolvable-action failures (3).
- Does NOT cover: timeout/cancellation (-> `cancelled`), unparseable YAML
(-> startup `failure`), or a job that lacks the guard (-> `failure`).

## Open question for test bed C (the decisive one)
Does the required-workflow gate pass/fail on the RUN conclusion, or on
per-job/per-check-run status? Mode 3 concluded run=`success` while its job
went red -- if the gate keys off the run conclusion it is SAFE; if it keys off
the failed job's check run it BLOCKS. Test bed C (real gate on tatari-tv/
sast-canary) settles this, plus confirms modes 2/4/6 actually block.

## Test bed C -- real required-workflow gate

Setup: private throwaway repo `tatari-tv/sast-canary` + org ruleset 18502427
(scoped to `sast-canary` ONLY, no bypass actors, #126206 untouched) requiring
`sast.yaml` from `github-required-workflows`.

### C1 -- findings PR (PR #1), gate confirmed on a real required check
- vuln.py trips Semgrep: log shows `Findings: 3 (3 blocking). Ran 320 rules`.
- BUT the canonical `semgrep scan --sarif --output ...` invocation EXITS 0 on
findings -> the Scan step + semgrep job conclude `success` (green). Findings
never even turn the job red. continue-on-error is not exercised by findings.
- All three jobs green, run conclusion = `success`.
- PR: `mergeable=MERGEABLE mergeStateStatus=CLEAN`.
- RESULT: on a real required gate, a success run does NOT block; findings are
non-blocking (and stronger than assumed -- they don't redden the job at all).

### Check-run vs run conclusion (from test bed A commit)
A `continue-on-error` job that FAILS reports check-run conclusion = `failure`
(observed for both the guarded findings job and the guarded bad-uses job) while
the RUN concludes `success`. So run conclusion and per-job check conclusion
DISAGREE for a guarded crash. Which one the required-workflow gate honors is the
open crux below.

### C2 -- the crux (guarded job CRASHES: run=success, job check=failure) -- RESOLVED
Gate repointed to branch `canary-infra-fail` (guarded semgrep job with an
injected `exit 1`). PR #2 on `sast-canary`:
- Semgrep required check-run = `fail` (the injected crash), Checkov/Trivy = pass.
- Workflow RUN conclusion = `success` (continue-on-error absorbed the crash).
- PR: `mergeable=MERGEABLE mergeStateStatus=CLEAN`.
- VERDICT: the required-workflow gate honors the workflow RUN conclusion, NOT
the per-job check-run. A guarded job that CRASHES (red job check, run=success)
does NOT block the merge. continue-on-error protects the gate even on infra
crashes inside a guarded job. The non-blocking premise HOLDS for anything the
guard can absorb.

## Decision table (failure mode -> run conclusion -> blocks merge? -> mitigation)
| Failure mode | Run conclusion | Blocks merge? | Mitigation before fleet registration |
|---|---|---|---|
| scanner finding (semgrep exits 0) | success | NO | none -- job stays green |
| step/job crash inside a guarded job (pipx/network/bad-pin) | success | NO (proven C2) | none -- continue-on-error absorbs it |
| YAML/syntax error in sast.yaml | failure (startup) | YES | pre-merge actionlint/yamllint gate on github-required-workflows |
| unresolvable `uses:` in an UN-guarded job | failure | YES | actionlint pin check; keep uses inside guarded jobs |
| timeout | cancelled | YES (run != success) | set an acceptable timeout-minutes; treat cancel as don't-block if feasible |
| cancellation | cancelled | YES (run != success) | operational; rare for a required run |
| job WITHOUT continue-on-error fails | failure | YES | lint asserting every job in sast.yaml carries continue-on-error: true |

Bottom line: blocking is driven purely by RUN conclusion. continue-on-error
makes the run conclude success for any step/job failure it can reach (findings
AND crashes). The residual blockers are the modes the guard cannot cover:
unparseable YAML, timeout/cancel, and any job that lacks the guard. Those are
the fleet-registration MUST-FIXes above.
Comment on lines +72 to +87

@coderabbitai coderabbitai Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Add the source-repo accessibility blocker to the must-fix list.

This checklist is not exhaustive yet: the workflow header already treats an inaccessible required-workflow source repo as a fleet-wide merge blocker. Since this section is the pre-registration blocker set, please add that failure mode and recovery action so operators do not miss it.

Proposed doc update 
 | timeout | cancelled | YES | set an acceptable timeout-minutes; treat cancel as don't-block if feasible |
 | cancellation | cancelled | YES | operational; rare for a required run |
 | job WITHOUT continue-on-error fails | failure | YES | lint asserting every job in sast.yaml carries continue-on-error: true |
+| inaccessible required-workflow source repo | failure | YES | restore source availability / re-point the required workflow before retrying |
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## Decision table (failure mode -> run conclusion -> blocks merge? -> mitigation)
| Failure mode | Run conclusion | Blocks merge? | Mitigation before fleet registration |
|---|---|---|---|
| scanner finding (semgrep exits 0) | success | NO | none -- job stays green |
| step/job crash inside a guarded job (pipx/network/bad-pin) | success | NO (proven C2) | none -- continue-on-error absorbs it |
| YAML/syntax error in sast.yaml | failure (startup) | YES | pre-merge actionlint/yamllint gate on github-required-workflows |
| unresolvable `uses:` in an UN-guarded job | failure | YES | actionlint pin check; keep uses inside guarded jobs |
| timeout | cancelled | YES (run != success) | set an acceptable timeout-minutes; treat cancel as don't-block if feasible |
| cancellation | cancelled | YES (run != success) | operational; rare for a required run |
| job WITHOUT continue-on-error fails | failure | YES | lint asserting every job in sast.yaml carries continue-on-error: true |
Bottom line: blocking is driven purely by RUN conclusion. continue-on-error
makes the run conclude success for any step/job failure it can reach (findings
AND crashes). The residual blockers are the modes the guard cannot cover:
unparseable YAML, timeout/cancel, and any job that lacks the guard. Those are
the fleet-registration MUST-FIXes above.
## Decision table (failure mode -> run conclusion -> blocks merge? -> mitigation)
| Failure mode | Run conclusion | Blocks merge? | Mitigation before fleet registration |
|---|---|---|---|
| scanner finding (semgrep exits 0) | success | NO | none -- job stays green |
| step/job crash inside a guarded job (pipx/network/bad-pin) | success | NO (proven C2) | none -- continue-on-error absorbs it |
| YAML/syntax error in sast.yaml | failure (startup) | YES | pre-merge actionlint/yamllint gate on github-required-workflows |
| unresolvable `uses:` in an UN-guarded job | failure | YES | actionlint pin check; keep uses inside guarded jobs |
| timeout | cancelled | YES (run != success) | set an acceptable timeout-minutes; treat cancel as don't-block if feasible |
| cancellation | cancelled | YES (run != success) | operational; rare for a required run |
| job WITHOUT continue-on-error fails | failure | YES | lint asserting every job in sast.yaml carries continue-on-error: true |
| inaccessible required-workflow source repo | failure | YES | restore source availability / re-point the required workflow before retrying |
Bottom line: blocking is driven purely by RUN conclusion. continue-on-error
makes the run conclude success for any step/job failure it can reach (findings
AND crashes). The residual blockers are the modes the guard cannot cover:
unparseable YAML, timeout/cancel, and any job that lacks the guard. Those are
the fleet-registration MUST-FIXes above.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 73-73: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/design/2026-07-03-fleet-sast-canary-results.md` around lines 72 - 87,
Add the source-repo accessibility failure mode to the decision table and
must-fix summary. In the markdown section containing the decision table and
“Bottom line,” update the fleet-registration blocker list to include an
inaccessible required-workflow source repo as a merge blocker, and note the
recovery action for the workflow header/required-workflow setup so operators
know how to remediate it. Use the existing decision table wording and the
bottom-line “MUST-FIXes” list as the place to insert this new entry.


## Break-glass owner (decided)
Anyone on `@tatari-tv/security` or `@tatari-tv/sre` may pull `sast.yaml` from
ruleset #126206 if it jams merges.
Comment on lines +89 to +91

@coderabbitai coderabbitai Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Make the break-glass owner specific.

Anyone on @tatari-tv/securityor@tatari-tv/sre`` is too broad for the documented rollback contract. The upstream design calls for a named owner plus a rollback trigger and speed target; please record those explicitly so the unblock path is unambiguous.

Proposed doc update 
-Anyone on `@tatari-tv/security` or `@tatari-tv/sre` may pull `sast.yaml` from
-ruleset `#126206` if it jams merges.
+Break-glass owner: <named primary>, <named backup>.
+Trigger: fleet-wide merge block attributable to SAST.
+Target: rollback in <minutes>, not hours.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## Break-glass owner (decided)
Anyone on `@tatari-tv/security` or `@tatari-tv/sre` may pull `sast.yaml` from
ruleset #126206 if it jams merges.
## Break-glass owner (decided)
Break-glass owner: <named primary>, <named backup>.
Trigger: fleet-wide merge block attributable to SAST.
Target: rollback in <minutes>, not hours.
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/design/2026-07-03-fleet-sast-canary-results.md` around lines 89 - 91,
The break-glass owner wording is too broad and should be replaced with a
specific named owner plus explicit rollback trigger and speed target. Update the
“Break-glass owner” section in the fleet-sast canary design doc to identify the
accountable person or team contact, describe the exact condition that allows
pulling `sast.yaml` from ruleset `#126206`, and state the expected time-to-unblock
so the rollback path is unambiguous.


## Cleanup (throwaway artifacts to remove when done)
- org ruleset 18502427 (org change)
- repo `tatari-tv/sast-canary`
- branches `sast-canary-tests`, `canary-infra-fail` in github-required-workflows
Loading