track: add security/sast-scan to branch protection required checks

[mvillmow]

Context

PR #264 added the security-sast-scan CI job to .github/workflows/_required.yml as part of issue #157. The job runs Bandit (medium+ severity) as a required check alongside security/dependency-scan and security/secrets-scan.

However, the repo's branch ruleset (homeric-main-baseline, id 15556487) does not yet list security/sast-scan as a required status check. Without this, PRs with SAST findings can merge unblocked.

Confirmed via:

gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
  --jq '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context]'
# Returns: ["lint","unit-tests","integration-tests","security/dependency-scan","security/secrets-scan","build","schema-validation","deps/version-sync"]
# Missing: "security/sast-scan"

Fix

An admin must add security/sast-scan to the ruleset's required status checks. The sibling security checks use integration_id: 15368 (GitHub Actions app).

# Fetch current ruleset to edit
gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 > /tmp/ruleset.json

# Add the new check to required_status_checks array in the rules[].parameters block,
# then PATCH:
gh api -X PUT repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
  --input /tmp/ruleset-updated.json

The updated required_status_checks array should include:

{"context": "security/sast-scan", "integration_id": 15368}

alongside the existing security/dependency-scan and security/secrets-scan entries.

Why now

Without this, security-sast-scan is advisory only — a PR author can ignore a red SAST gate and merge. The goal of issue #157 was to make SAST a merge gate, not an informational check.

Part of #92, closes the remaining enforcement gap from #157.

[mvillmow]

Implementation Plan

Implementation Plan

Objective

Add security/sast-scan to the homeric-main-baseline branch ruleset (id 15556487) as a required status check, making the Bandit SAST gate from issue #157 merge-blocking rather than advisory. The ruleset currently requires 8 contexts (lint, unit-tests, integration-tests, security/dependency-scan, security/secrets-scan, build, schema-validation, deps/version-sync) — security/sast-scan is absent, so a PR with SAST findings can merge unblocked. This closes the remaining enforcement gap from #157, part of epic #92.

Approach

This is a GitHub-API ruleset configuration change executed via an admin gh api runbook — no working-tree files change. The plan is an operational runbook plus read-back verification and an explicit rollback path.

Key decisions, each with evidence:

• Context name is security/sast-scan, not the job key security-sast-scan. Verified from PR feat(security): add Bandit SAST to CI/CD pipeline #264's diff (gh pr diff 264 | grep -iE "sast" → job key security-sast-scan: with name: security/sast-scan). Per the gha-required-checks-branch-protection skill (Section G), GitHub pins the job name: as the status-check context — the slash form. The sibling ruleset entries security/dependency-scan and security/secrets-scan confirm the slash convention. Because this was read from the unmerged PR, runbook step 1 re-confirms the name on main post-merge before the edit.

• BLOCKING PREREQUISITE: PR feat(security): add Bandit SAST to CI/CD pipeline #264 must be merged to main first. Verified this session: gh pr view 264 --json state,mergedAt → state: OPEN, mergedAt: null; grep -n "sast" .github/workflows/_required.yml on main → no match. The issue body's premise ("PR feat(security): add Bandit SAST to CI/CD pipeline #264 added the security-sast-scan CI job") is not yet true on main. Per the skill (Section A + Failed Attempts: "context never posts → BLOCKED forever"), adding a required context whose posting job is absent from main permanently bricks the merge queue — every PR waits forever for a context that never reports. This ordering is a hard gate (runbook step 1), not a preference.

• Use the ruleset API (/rulesets/15556487), not the branch-/protection API. Enforcement lives in the repo ruleset homeric-main-baseline; the issue's enumeration query against /rulesets/15556487 returns the 8 contexts. The skill (Section H) warns the branch-/protection PUT is a separate, full-replacement API that does NOT touch rulesets — do not confuse them.

• GET-before-PUT → surgical jq merge → PUT → read-back → rollback-on-mismatch. The ruleset PUT replaces the full ruleset object (skill Section H, destructive full-replacement). So: snapshot the live ruleset to /tmp/ruleset-before.json, add one entry to required_status_checks with jq (idempotent — only if absent, preserving every other field), PUT, then GET and assert the new context is present AND the prior 8 survived. If the read-back shows fewer than 9 contexts or any prior context missing, immediately restore by PUT-ing the /tmp/ruleset-before.json snapshot back (rollback step 7). A 200 response alone does not prove the field took.

• integration_id: 15368 (GitHub Actions app) on the new entry, matching the sibling security/dependency-scan / security/secrets-scan entries — from the issue body and re-derivable from the live ruleset JSON. The jq merge reuses the integration_id of an existing sibling entry rather than hardcoding, so a stale literal cannot silently diverge (runbook step 4).

Files to Create

None. This is a GitHub-API ruleset change; no repository files are added.

Files to Modify

None. No working-tree file changes. The mutation target is the GitHub ruleset object 15556487, edited via the gh api runbook below. The job that posts the security/sast-scan context lands separately via PR #264's edit to .github/workflows/_required.yml; this issue does not re-touch that file.

Implementation Order

1. Hard gate on PR feat(security): add Bandit SAST to CI/CD pipeline #264 merge. Confirm the SAST posting job exists on main before anything else:

   gh pr view 264 --json state,mergedAt --jq '{state,mergedAt}'
   git fetch origin main && git grep -n "security/sast-scan" origin/main -- .github/workflows/_required.yml

   Proceed only when state == "MERGED" AND the grep finds name: security/sast-scan on origin/main. If PR feat(security): add Bandit SAST to CI/CD pipeline #264 is still open, STOP — editing the ruleset now permanently bricks the merge queue.

2. (Recommended, non-gating) Confirm the context has actually posted at least once on a real post-feat(security): add Bandit SAST to CI/CD pipeline #264 run, so the ruleset will match a real context. Query the latest closed PR's head commit (a check-run only exists where the workflow ran):

   LATEST_PR_SHA=$(gh pr list --repo HomericIntelligence/ProjectTelemachy --state merged --limit 1 --json headRefOid --jq '.[0].headRefOid')
   gh api repos/HomericIntelligence/ProjectTelemachy/commits/$LATEST_PR_SHA/check-runs \
     --jq '[.check_runs[].name] | map(select(. == "security/sast-scan"))'
   # expect: ["security/sast-scan"]  (empty = job hasn't run on a PR yet; not fatal, but the gate won't be exercised until it does)

3. GET the current ruleset (snapshot for both the merge base and rollback):

   gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 > /tmp/ruleset-before.json
   # sanity: confirm the snapshot is the expected 8-context ruleset before mutating
   jq '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context] | length' /tmp/ruleset-before.json
   # expect: 8

4. Build the updated payload — append one entry to required_status_checks, reusing an existing sibling's integration_id (so no hardcoded literal can drift), idempotent (only adds if absent), every other field untouched:

   jq '
     (.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks) as $checks
     | ($checks | map(select(.context=="security/dependency-scan")) | .[0].integration_id) as $iid
     | (.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks)
       |= (if (map(.context) | index("security/sast-scan")) then .
           else . + [{"context":"security/sast-scan","integration_id":$iid}] end)
   ' /tmp/ruleset-before.json > /tmp/ruleset-updated.json
   # confirm the payload now has exactly 9 contexts incl. the new one
   jq '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context]' /tmp/ruleset-updated.json

5. PUT the updated ruleset:

   gh api -X PUT repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
     --input /tmp/ruleset-updated.json > /dev/null

6. Read-back and assert the new context is present AND the prior 8 survived (9 total):

   gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
     --jq '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context]' \
     | tee /tmp/ruleset-after-contexts.json
   # must contain "security/sast-scan" AND all 8 prior contexts (length 9)

7. ROLLBACK on mismatch (destructive-PUT safeguard). If step 6 shows length ≠ 9, or security/sast-scan is absent, or any of the prior 8 contexts is missing, restore the pre-edit ruleset immediately and abort:

   AFTER=$(jq 'length' /tmp/ruleset-after-contexts.json)
   if [ "$AFTER" != "9" ]; then
     echo "READ-BACK FAILED (got $AFTER contexts, expected 9) — restoring pre-edit ruleset"
     gh api -X PUT repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
       --input /tmp/ruleset-before.json > /dev/null
     # confirm restore returned to the original 8
     gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
       --jq '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context] | length'
     echo "Rolled back. Do NOT close #284; investigate the PUT failure."; exit 1
   fi

8. Close issue track: add security/sast-scan to branch protection required checks #284 referencing the step-6 read-back output (the 9-context list) as evidence.

Verification

# Acceptance criterion (the gap from the issue): security/sast-scan is now a REQUIRED context.
gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
  --jq '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context] | index("security/sast-scan")'
# expect: a non-null index (the context is present)

# Acceptance criterion (no collateral damage from the full-replacement PUT): all 8 prior contexts survive → 9 total.
gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
  --jq '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context] | length'
# expect: 9

# Acceptance criterion (the guard BLOCKS, not green-but-non-blocking — skill Section J):
# prove security/sast-scan is byte-for-byte one of the pinned required contexts.
gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
  --jq '.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context' \
  | grep -qx "security/sast-scan" \
  && echo "BLOCKING: security/sast-scan is a pinned required context" \
  || { echo "PLACEMENT BUG: security/sast-scan not pinned — gate blocks nothing"; exit 1; }

# Prerequisite gate (must pass BEFORE step 3, else the ruleset edit bricks the merge queue):
# the context-posting job must exist on main. Loose match — indentation-agnostic.
git fetch origin main >/dev/null 2>&1
git grep -q "name: security/sast-scan" origin/main -- .github/workflows/_required.yml \
  && echo "SAST job present on main — safe to edit ruleset" \
  || { echo "PR #264 not merged — DO NOT edit ruleset yet"; exit 1; }

# Rollback-path sanity (proves the snapshot is restorable): the pre-edit snapshot parses and carries the 8 contexts.
jq -e '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context] | length == 8' \
  /tmp/ruleset-before.json \
  && echo "Snapshot valid — rollback target intact" \
  || { echo "Snapshot missing/corrupt — do NOT PUT without a valid rollback target"; exit 1; }

Skills Used

• gha-required-checks-branch-protection (team knowledge base, Prior Learnings) — applied:
  • Section G (job-key vs context-name): the pinned context is the job name: security/sast-scan, not job key security-sast-scan.
  • Section A + Failed Attempts ("context never posts → BLOCKED forever"): drives the hard PR feat(security): add Bandit SAST to CI/CD pipeline #264-merge prerequisite gate — a never-posted required context permanently blocks the queue.
  • Section H (destructive full-replacement PUT mitigation): GET-before-PUT snapshot, surgical idempotent jq merge preserving all other fields, read-back assertion, and the explicit rollback (re-PUT the snapshot) on read-back mismatch.
  • Section J (place a merge-blocking guard in a pinned required context; enumerate required contexts and grep -qx the target): the blocking-vs-advisory verification command proves the gate blocks rather than being advisory.
  • Section K (verify the issue's prerequisite-PR premise before ordering the runbook): drove discovery that feat(security): add Bandit SAST to CI/CD pipeline #264 is unmerged, inverting the safety ordering.

Changes from review

• Finding 1 (Minor — no explicit rollback step on a destructive PUT): Added runbook step 7, a rollback block that re-PUTs /tmp/ruleset-before.json when the step-6 read-back shows ≠ 9 contexts or a missing context, then aborts without closing track: add security/sast-scan to branch protection required checks #284. Added a fifth verification command asserting the snapshot is a valid 8-context restore target before any PUT. This is the headline fix.
• Finding 2 (Minor — step 2 query/prose mismatch: commits/main/check-runs vs "a recent post-feat(security): add Bandit SAST to CI/CD pipeline #264 PR"): Rewrote step 2 to resolve the latest merged PR's head SHA (gh pr list … --json headRefOid) and query commits/$LATEST_PR_SHA/check-runs, so the command targets a commit where a PR workflow actually ran — matching the prose. Marked explicitly non-gating.
• Finding 3 (Trivial — brittle exact-indentation grep grep -qx " name: …"): Replaced with indentation-agnostic git grep "name: security/sast-scan" origin/main in both runbook step 1 and the verification block; removed the leading-whitespace dependency.
• Verified-correct items retained unchanged: the Section-G context-name claim, Section-J blocking proof, integration_id sourcing (now hardened further in step 4 by reading the sibling's integration_id via jq instead of a hardcoded 15368 literal, closing the "read from unmerged diff could drift" handoff soft-spot the reviewer flagged).

---

Final review verdict (from strict review loop)

The plan under review is a re-review (R1). The single new finding from the prior iteration was the missing explicit rollback step on a destructive PUT. I need to verify it was actually addressed, not just acknowledged.

Prior finding resolution check:

• Prior NOGO turned on: "No explicit rollback step" on the destructive full-replacement PUT.
• Current plan step 7 ("ROLLBACK on mismatch") adds a concrete block: re-PUTs /tmp/ruleset-before.json when read-back shows length ≠ 9 or a missing context, confirms restore to 8, aborts with exit 1, and explicitly does NOT close track: add security/sast-scan to branch protection required checks #284. This is a real plan-step change, not a hand-wave sentence. Resolved.
• Prior Finding 2 (step 2 query/prose mismatch commits/main/check-runs): now rewritten to resolve LATEST_PR_SHA via gh pr list … --json headRefOid and query commits/$LATEST_PR_SHA/check-runs, marked non-gating. Resolved.
• Prior Finding 3 (brittle exact-indentation grep): replaced with git grep "name: security/sast-scan" origin/main in step 1 and verification. Resolved (verification block still uses a name: security/sast-scan loose match, no leading-whitespace dependency).

Re-grading the dimensions:

1. Requirements alignment — single AC (add security/sast-scan to ruleset 15556487) named and addressed; the false-premise blocker (PR feat(security): add Bandit SAST to CI/CD pipeline #264 OPEN) is surfaced and hard-gated (step 1). Strong.
2. Plan completeness — design rationale, ordered runbook, read-back, and now rollback. The one genuine gap from R0 is closed. The TDD-analog (read-back assertions for a config-only change) is present.
3. Concreteness — exact ruleset id, jq surgical merge, integration_id sourced from a sibling entry (not hardcoded), every command named.
4. Risk surface — strongest dimension: destructive PUT (GET-before-PUT + surgical jq + read-back + rollback), never-posts-context brick, ordering gate. Nothing treated as routine.
5. Verification plan — five runnable checks incl. Section-J blocking-vs-advisory proof, the prerequisite gate, and a rollback-target sanity check.
6. Stage handoff — implementer can execute as-is; foreign keys (integration_id, ruleset id, context name) are re-derived/re-confirmed against the live GET (step 3/4) and post-merge grep (step 1). Loop closed.

Principles: P1/KISS — minimal, idempotent jq is justified not gold-plated. P2/YAGNI — explicit "no repo files change," no scope creep. P3/TDD — read-back assertions are the correct analog and present. P7/POLA — idempotent, predictable runbook.

Residual minors (all small):

• The runbook remains planning-only/unexecuted against the live API — but that is inherent to a plan artifact and disclosed in the Learnings; not a defect of the plan.
• integration_id/ruleset-id/sibling-name still originate from the issue body; step 3/4 re-derive from the live GET, and the Learnings flag re-confirmation. Acceptable.

No critical or major findings remain. The single major-ish operational gap from R0 (rollback on a destructive op) is now written down concretely. That clears the prior NOGO basis. Remaining items are ≤2 trivial/inherent. This meets A's "ZERO critical/major, ≤2 minor" and clears B's ceiling. The rollback being the headline R0 blocker and now genuinely fixed is the decisive change.

• Prior R0 NOGO basis (rollback) → resolved in step 7 (concrete re-PUT block, not acknowledgement).
• Findings 2 & 3 → resolved.
• No new major/critical findings; risk handling is exemplary for a destructive API op.

Grade: A
Verdict: GO

---

Generated by Claude Code Planner (strict review loop)

[mvillmow]

🔍 Plan Review

The plan under review is a re-review (R1). The single new finding from the prior iteration was the missing explicit rollback step on a destructive PUT. I need to verify it was actually addressed, not just acknowledged.

Prior finding resolution check:

• Prior NOGO turned on: "No explicit rollback step" on the destructive full-replacement PUT.
• Current plan step 7 ("ROLLBACK on mismatch") adds a concrete block: re-PUTs /tmp/ruleset-before.json when read-back shows length ≠ 9 or a missing context, confirms restore to 8, aborts with exit 1, and explicitly does NOT close track: add security/sast-scan to branch protection required checks #284. This is a real plan-step change, not a hand-wave sentence. Resolved.
• Prior Finding 2 (step 2 query/prose mismatch commits/main/check-runs): now rewritten to resolve LATEST_PR_SHA via gh pr list … --json headRefOid and query commits/$LATEST_PR_SHA/check-runs, marked non-gating. Resolved.
• Prior Finding 3 (brittle exact-indentation grep): replaced with git grep "name: security/sast-scan" origin/main in step 1 and verification. Resolved (verification block still uses a name: security/sast-scan loose match, no leading-whitespace dependency).

Re-grading the dimensions:

1. Requirements alignment — single AC (add security/sast-scan to ruleset 15556487) named and addressed; the false-premise blocker (PR feat(security): add Bandit SAST to CI/CD pipeline #264 OPEN) is surfaced and hard-gated (step 1). Strong.
2. Plan completeness — design rationale, ordered runbook, read-back, and now rollback. The one genuine gap from R0 is closed. The TDD-analog (read-back assertions for a config-only change) is present.
3. Concreteness — exact ruleset id, jq surgical merge, integration_id sourced from a sibling entry (not hardcoded), every command named.
4. Risk surface — strongest dimension: destructive PUT (GET-before-PUT + surgical jq + read-back + rollback), never-posts-context brick, ordering gate. Nothing treated as routine.
5. Verification plan — five runnable checks incl. Section-J blocking-vs-advisory proof, the prerequisite gate, and a rollback-target sanity check.
6. Stage handoff — implementer can execute as-is; foreign keys (integration_id, ruleset id, context name) are re-derived/re-confirmed against the live GET (step 3/4) and post-merge grep (step 1). Loop closed.

Principles: P1/KISS — minimal, idempotent jq is justified not gold-plated. P2/YAGNI — explicit "no repo files change," no scope creep. P3/TDD — read-back assertions are the correct analog and present. P7/POLA — idempotent, predictable runbook.

Residual minors (all small):

• The runbook remains planning-only/unexecuted against the live API — but that is inherent to a plan artifact and disclosed in the Learnings; not a defect of the plan.
• integration_id/ruleset-id/sibling-name still originate from the issue body; step 3/4 re-derive from the live GET, and the Learnings flag re-confirmation. Acceptable.

No critical or major findings remain. The single major-ish operational gap from R0 (rollback on a destructive op) is now written down concretely. That clears the prior NOGO basis. Remaining items are ≤2 trivial/inherent. This meets A's "ZERO critical/major, ≤2 minor" and clears B's ceiling. The rollback being the headline R0 blocker and now genuinely fixed is the decisive change.

• Prior R0 NOGO basis (rollback) → resolved in step 7 (concrete re-PUT block, not acknowledgement).
• Findings 2 & 3 → resolved.
• No new major/critical findings; risk handling is exemplary for a destructive API op.

Grade: A
Verdict: GO