Follow-up from #157: 1 item(s) (security)

[mvillmow]

Consolidated follow-up from implementation of #157.

Each section below lists scope-checked follow-up items discovered during implementation. Items are restricted to core library defects, security, safety hazards, or critical functional bugs.

Security

Add security/sast-scan to branch protection required checks

The security-sast-scan CI job was added in PR #264 but is not listed in the repo's branch ruleset (id 15556487, homeric-main-baseline). Confirmed via gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 — required_status_checks contains security/dependency-scan and security/secrets-scan but NOT security/sast-scan. Without this, PRs with SAST findings can merge unblocked, defeating the purpose of the required check. Fix: PATCH the ruleset to add {"context": "security/sast-scan"} to required_status_checks. Admin command: gh api -X PUT repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 --input <updated-ruleset.json> where the updated JSON adds {"context": "security/sast-scan", "integration_id": <gha-app-id>} to the existing checks array.

---

The implementer also considered the items below and rejected them as out of scope; they are recorded in the PR body, not filed as separate issues.

[mvillmow]

Implementation Plan

I have everything I need. Key facts:

• Job security-sast-scan with name: security/sast-scan exists in PR feat(security): add Bandit SAST to CI/CD pipeline #264 branch (39a509a), not yet merged to main (PR feat(security): add Bandit SAST to CI/CD pipeline #264 is OPEN).
• The check context to add is security/sast-scan with integration_id: 15368 (GitHub Actions app, same as all other checks).
• This is a repo-config change (ruleset PATCH), not a code change. There are no acceptance criteria provable by pytest — verification is via gh api.

One decision to surface: the ruleset PATCH requires admin/owner privileges and an admin command, which the issue explicitly flags. The plan must sequence the ruleset change after PR #264 merges (otherwise a required check that never reports would block all PRs).

Implementation Plan

Objective

Branch-protection ruleset 15556487 (homeric-main-baseline) requires security/dependency-scan and security/secrets-scan but not security/sast-scan. As a result, the Bandit SAST job can fail (or be skipped) without blocking a PR merge, defeating the purpose of the gate. This issue adds security/sast-scan to the ruleset's required_status_checks so PRs with SAST findings cannot merge unblocked. The change is a GitHub ruleset PATCH (admin-only); no application source code is touched.

Approach

• The change is a ruleset edit, not a code change. Verified the live ruleset via gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 — the required_status_checks.required_status_checks array contains 8 contexts (lint, unit-tests, integration-tests, security/dependency-scan, security/secrets-scan, build, schema-validation, deps/version-sync), each with integration_id: 15368. security/sast-scan is absent. Confirmed.
• The new context must use integration_id: 15368. Every existing check in the ruleset uses 15368 (the GitHub Actions app id). The SAST job (security-sast-scan → name: security/sast-scan in _required.yml:295-296 of commit 39a509a) runs on GitHub Actions, so its check reports under the same app id. The issue body's suggestion to use <gha-app-id> resolves to 15368.
• CRITICAL ordering decision — the ruleset change must NOT land before PR feat(security): add Bandit SAST to CI/CD pipeline #264 merges. gh pr view 264 shows PR feat(security): add Bandit SAST to CI/CD pipeline #264 (feat(security): add Bandit SAST to CI/CD pipeline) is OPEN, not merged. grep over .github/workflows/ on main finds no sast-scan job (Grep sast → No matches); the job exists only on branch 157-auto-impl (git log --all -S 'sast-scan' → single commit 39a509a). GitHub required status checks block merges until the named context reports; if security/sast-scan is added to the ruleset while the job does not yet exist on main, every open PR would be permanently blocked waiting for a check that never runs. Therefore the ruleset PATCH is gated on PR feat(security): add Bandit SAST to CI/CD pipeline #264 being merged first. The issue body assumes the job already exists ("added in PR feat(security): add Bandit SAST to CI/CD pipeline #264") — that premise is not yet true on main, and the plan corrects for it.
• Method: full-PUT update via gh api. GitHub's ruleset update endpoint replaces the required_status_checks rule wholesale. The safe pattern is: GET the current ruleset → append the new context to the checks array → PUT the modified payload. A blind PATCH that omits the existing 8 checks would drop them. Use a captured-and-modified JSON file, never a hand-written one.
• No PR/branch in this repo. A ruleset lives in GitHub repo settings, not in the working tree. There is no file to edit, no test to add, and just validate/pytest do not apply. The deliverable is the executed admin command plus a verification query. This plan documents the exact command for the admin to run (the planning/implementer agent cannot self-grant admin scope).

Files to Create

None. (Ruleset configuration is GitHub-server-side state, not a tracked file. A scratch JSON payload is generated transiently and not committed.)

Files to Modify

None in the repository.

The single change is to GitHub ruleset 15556487, applied via API. The transient payload built from the live ruleset is:

// required_status_checks rule AFTER the change — note the appended last entry
{
  "type": "required_status_checks",
  "parameters": {
    "strict_required_status_checks_policy": false,
    "do_not_enforce_on_create": false,
    "required_status_checks": [
      { "context": "lint", "integration_id": 15368 },
      { "context": "unit-tests", "integration_id": 15368 },
      { "context": "integration-tests", "integration_id": 15368 },
      { "context": "security/dependency-scan", "integration_id": 15368 },
      { "context": "security/secrets-scan", "integration_id": 15368 },
      { "context": "build", "integration_id": 15368 },
      { "context": "schema-validation", "integration_id": 15368 },
      { "context": "deps/version-sync", "integration_id": 15368 },
      { "context": "security/sast-scan", "integration_id": 15368 }   // <-- ADDED
    ]
  }
}

Implementation Order

1. Gate on PR feat(security): add Bandit SAST to CI/CD pipeline #264. Confirm PR feat(security): add Bandit SAST to CI/CD pipeline #264 is merged into main and that security/sast-scan appears as a check on a main-targeted PR run:

   gh pr view 264 --repo HomericIntelligence/ProjectTelemachy --json state,mergedAt

   Do not proceed to step 2 until state is MERGED. (If feat(security): add Bandit SAST to CI/CD pipeline #264 is closed unmerged, the job context name must be re-confirmed against whatever PR actually lands the SAST job before adding it to the ruleset.)
2. Capture the live ruleset to a scratch file:

   gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 > /tmp/ruleset-15556487.json

3. Append the new check to the required_status_checks array with jq, writing the API-shaped update payload (only the fields the PUT endpoint accepts: name, enforcement, conditions, rules, bypass_actors):

   jq '{
     name, enforcement, conditions, bypass_actors,
     rules: (.rules | map(
       if .type == "required_status_checks"
       then .parameters.required_status_checks +=
            [{"context":"security/sast-scan","integration_id":15368}]
       else . end))
   }' /tmp/ruleset-15556487.json > /tmp/ruleset-15556487-updated.json

4. Apply the update (ADMIN-ONLY). Run as a repo admin/owner — the implementer agent lacks ruleset-write scope:

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

5. Verify (see Verification). Confirm exactly 9 checks and that security/sast-scan is present.
6. Clean up the scratch files: rm -f /tmp/ruleset-15556487.json /tmp/ruleset-15556487-updated.json.

Verification

The issue's single acceptance criterion is "security/sast-scan is in the ruleset's required status checks." There is no pytest target (no repo code changes); verification is an API read:

# Acceptance criterion: security/sast-scan is a required status check on ruleset 15556487
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 "PASS: security/sast-scan is required" || echo "FAIL: not present"

# Regression guard: the original 8 checks are still present (the PUT did not drop them)
gh api repos/HomericIntelligence/ProjectTelemachy/rulesets/15556487 \
  --jq '[.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context] | sort' 
# Expected (9 entries): build, deps/version-sync, integration-tests, lint,
# security/dependency-scan, security/sast-scan, security/secrets-scan, schema-validation, unit-tests

# Pre-flight gate: confirm the job exists on main before requiring it (prevents permanently blocking PRs)
gh api repos/HomericIntelligence/ProjectTelemachy/contents/.github/workflows/_required.yml \
  --jq '.content' | base64 -d | grep -q 'name: security/sast-scan' \
  && echo "PASS: SAST job on main" || echo "BLOCK: job not on main yet — do not add to ruleset"

Skills Used

• gha-security-scanning-supply-chain (team knowledge base, Prior Learnings) — applied its guidance on promoting a SAST scan to a required PR gate, the security/sast-scan job/context naming, and the integration_id convention for GitHub Actions check contexts. The skill's reference confirms the Bandit security-sast-scan job shape that produces the security/sast-scan context being required here.
• No repo-local Skill tool invocations were needed (this is a GitHub-settings change, not a code edit).

Changes from Review

N/A — initial plan.

---

Final review verdict (from strict review loop)

Every load-bearing claim verified:

• PR feat(security): add Bandit SAST to CI/CD pipeline #264 is OPEN/unmerged — job exists only on branch 157-auto-impl, not main ✓
• Job key security-sast-scan → name: security/sast-scan at _required.yml:295-296 ✓
• Ruleset 15556487 has exactly the 8 checks listed, all integration_id=15368, security/sast-scan absent ✓
• The jq payload keys (name, enforcement, conditions, bypass_actors, rules) are all valid top-level ruleset fields ✓

Assessment against rubric

Requirements alignment — The issue's single criterion ("add security/sast-scan to ruleset 15556487 required checks") is named and addressed by a concrete PATCH step (Implementation Order step 4). No criteria dropped.

Plan completeness — Strong. The plan does NOT blindly follow the issue. It catches that the issue's premise ("added in PR #264") is false on main, and inserts a hard gate (step 1) preventing the documented hazard: adding a required check whose job doesn't yet report would permanently block every open PR. This is the single most important risk and the plan owns it.

Concreteness — Specific ruleset id, integration_id, file:line (_required.yml:295-296), commit 39a509a, exact gh api/jq commands. No vague verbs.

Risk surface — Excellent. (a) Ordering hazard called out and gated. (b) Full-PUT-drops-existing-checks hazard called out with GET→append→PUT mitigation and a regression guard verifying all 9 survive. (c) Admin-only scope flagged; implementer cannot self-grant.

Verification plan — Three concrete, runnable checks: acceptance (grep for context), regression (all 9 sorted), and a pre-flight gate (job present on main). Correctly notes no pytest applies.

Stage handoff — Implementer can act without re-deriving: integration_id resolved (15368, not <gha-app-id>), context-vs-YAML-key distinction surfaced, payload shape pre-built.

Principles

• P1/KISS, P2/YAGNI: Minimal change, no scope creep. ✓
• P3/TDD: No code → no unit tests appropriate; verification is API reads, correctly. The acceptance check is essentially the test contract. ✓
• P7/POLA: Correctly resists the issue's surprising/wrong premise rather than executing it blindly. ✓

Minor findings (≤2)

1. The PUT-payload-field-stripping and the jq mutation were not executed (admin-only) — they're verified-local at best. I confirmed the field names are valid keys, but whether GitHub's PUT endpoint accepts exactly this subset (e.g. whether conditions/bypass_actors round-trip cleanly from GET shape) is unverified until run. The plan honestly labels this; not a defect, but a residual risk the implementer must watch (regression guard covers the worst case).
2. Step 1's fallback ("if feat(security): add Bandit SAST to CI/CD pipeline #264 closed unmerged, re-confirm the job context name against whatever PR lands it") is the right instinct but slightly hand-wavy — acceptable since it's a contingency branch, not the main path.

Zero critical/major findings. The plan's defining strength is that it caught and corrected a false issue premise that would have caused a repo-wide outage if followed literally.

Grade: A
Verdict: GO

---

Generated by Claude Code Planner (strict review loop)

[mvillmow]

🔍 Plan Review

Every load-bearing claim verified:

• PR feat(security): add Bandit SAST to CI/CD pipeline #264 is OPEN/unmerged — job exists only on branch 157-auto-impl, not main ✓
• Job key security-sast-scan → name: security/sast-scan at _required.yml:295-296 ✓
• Ruleset 15556487 has exactly the 8 checks listed, all integration_id=15368, security/sast-scan absent ✓
• The jq payload keys (name, enforcement, conditions, bypass_actors, rules) are all valid top-level ruleset fields ✓

Assessment against rubric

Requirements alignment — The issue's single criterion ("add security/sast-scan to ruleset 15556487 required checks") is named and addressed by a concrete PATCH step (Implementation Order step 4). No criteria dropped.

Plan completeness — Strong. The plan does NOT blindly follow the issue. It catches that the issue's premise ("added in PR #264") is false on main, and inserts a hard gate (step 1) preventing the documented hazard: adding a required check whose job doesn't yet report would permanently block every open PR. This is the single most important risk and the plan owns it.

Concreteness — Specific ruleset id, integration_id, file:line (_required.yml:295-296), commit 39a509a, exact gh api/jq commands. No vague verbs.

Risk surface — Excellent. (a) Ordering hazard called out and gated. (b) Full-PUT-drops-existing-checks hazard called out with GET→append→PUT mitigation and a regression guard verifying all 9 survive. (c) Admin-only scope flagged; implementer cannot self-grant.

Verification plan — Three concrete, runnable checks: acceptance (grep for context), regression (all 9 sorted), and a pre-flight gate (job present on main). Correctly notes no pytest applies.

Stage handoff — Implementer can act without re-deriving: integration_id resolved (15368, not <gha-app-id>), context-vs-YAML-key distinction surfaced, payload shape pre-built.

Principles

• P1/KISS, P2/YAGNI: Minimal change, no scope creep. ✓
• P3/TDD: No code → no unit tests appropriate; verification is API reads, correctly. The acceptance check is essentially the test contract. ✓
• P7/POLA: Correctly resists the issue's surprising/wrong premise rather than executing it blindly. ✓

Minor findings (≤2)

1. The PUT-payload-field-stripping and the jq mutation were not executed (admin-only) — they're verified-local at best. I confirmed the field names are valid keys, but whether GitHub's PUT endpoint accepts exactly this subset (e.g. whether conditions/bypass_actors round-trip cleanly from GET shape) is unverified until run. The plan honestly labels this; not a defect, but a residual risk the implementer must watch (regression guard covers the worst case).
2. Step 1's fallback ("if feat(security): add Bandit SAST to CI/CD pipeline #264 closed unmerged, re-confirm the job context name against whatever PR lands it") is the right instinct but slightly hand-wavy — acceptable since it's a contingency branch, not the main path.

Zero critical/major findings. The plan's defining strength is that it caught and corrected a false issue premise that would have caused a repo-wide outage if followed literally.

Grade: A
Verdict: GO