Skip to content

ci(security): make the security-fixes routine observable and stagger its schedules - #1794

Open
PMerlet wants to merge 5 commits into
mainfrom
ci/security-fixes-visibility
Open

ci(security): make the security-fixes routine observable and stagger its schedules#1794
PMerlet wants to merge 5 commits into
mainfrom
ci/security-fixes-visibility

Conversation

@PMerlet

@PMerlet PMerlet commented Jul 30, 2026

Copy link
Copy Markdown
Member

Why

First real runs of the new routine (2026-07-30) gave mixed results:

Repo Result
toolbelt #809 — 1 alert fixed, 4 stale resolutions removed
agent-ruby #337 — 1 alert fixed
forestadmin-server ❌ agent step failed: 1 turn, $0.00, is_error: true after ~180 s
forestadmin ❌ same signature
agent-nodejs ⏹ cancelled manually after 28 min of zero visible output

Two problems to address, neither in the agent instructions themselves:

1. No observability. claude-code-action hides all agent output unless show_full_output: true is set. A run that is working normally and a run that is stuck look identical in the logs — which is exactly why the agent-nodejs run was cancelled by hand, and why the two failures can't be diagnosed beyond "the first API request never completed" (total_cost_usd: 0 = nothing was ever billed).

2. Five concurrent Opus sessions on one API key. All five workflows shared the 0 11 * * 4 slot, so five sessions started within ~3 minutes. The two repos with the largest initial context (forestadmin-server, forestadmin — the biggest monorepos, both with a sizeable CLAUDE.md) started 53 s apart and both burned ~3 minutes of retries before giving up without a single billed token; the smaller repos, running with less overlap, succeeded. That is the signature of an input-token rate limit, not of a bug in the routine.

What

  • show_full_output: true — agent progress becomes visible in the workflow log (GitHub still masks secrets).
  • Staggered schedules, 30 min apart, so the five repos never run concurrently:
    forestadmin-server 11:00 · forestadmin 11:30 · agent-nodejs 12:00 · toolbelt 12:30 · agent-ruby 13:00 UTC.

workflow_dispatch stays available; when triggering several repos by hand, space them out.

Next

Re-run the three non-green repos manually one at a time after merging. With full output on, a repeat failure will name the actual API error — if it is a rate limit, the options are a higher API tier or --model claude-sonnet-5 for the two big monorepos.

🤖 Generated with Claude Code

Note

Make the security-fixes CI workflow observable on failure and stagger its schedule

  • Adds a failure-only step to security-fixes.yml that reads the agent's execution output and emits a compact error summary (subtype, terminal reason, cost, etc.) without exposing the full transcript.
  • Shifts the weekly schedule from 11:00 to 12:00 UTC on Thursdays to stagger runs and avoid rate limits.
  • Bumps actions/checkout and actions/setup-node from v4 to v6.
  • Updates security-fixes-prompt.md to use yarn install --ignore-scripts, scope pre-push linting to changed files only, and replace checkbox columns in PR tables with a separate 'Review checklist' task list.

Macroscope summarized 4c7d4b4.

First real runs: toolbelt and agent-ruby produced valid security PRs;
forestadmin-server and forestadmin failed with '1 turn, $0.00,
is_error: true' after ~180s (no token ever billed → the first API request
never completed), and agent-nodejs was cancelled by hand after 28 minutes
of zero visible output.

- show_full_output: true — the action hides all agent output by default,
  making a working run indistinguishable from a hung one and the failures
  impossible to diagnose (secrets stay masked by GitHub)
- stagger the five schedules 30 min apart: five concurrent Opus sessions
  on one API key, the two largest-context repos starting 53s apart, is
  the likely trigger of the unbilled retry loop

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@qltysh

qltysh Bot commented Jul 30, 2026

Copy link
Copy Markdown

Qlty


Coverage Impact

This PR will not change total coverage.

🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

PMerlet and others added 2 commits July 30, 2026 16:17
…epos

The agent-nodejs run reached 'git add' in 15 min but spent most of that
on checks that cannot fail because of what the routine changes, and the
runner was terminated (SIGTERM) 3 min later.

- pre-push checks now only cover files this run modified: prettier on the
  changed files, eslint only if a source file changed. The root lint
  script (lerna exec --parallel eslint over 25 packages) is dropped: it
  dominated the runtime, pressured the runner, and only surfaced
  pre-existing issues the routine must not touch.
- verification installs use --ignore-scripts: node-gyp native rebuilds
  cost minutes here and resolution is identical without them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
actions/checkout@v4 and actions/setup-node@v4 ship the Node 20 runtime,
which runners now warn about and force onto Node 24. v6 targets Node 24
natively and is already used elsewhere in these repos.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread .github/workflows/security-fixes.yml Outdated
PMerlet and others added 2 commits July 30, 2026 16:53
GitHub renders '- [ ]' as an interactive checkbox only at the start of a
list item. Inside a table cell it stays literal text ('<td>- [ ]</td>',
verified against GitHub's own /markdown API), and a raw
<input type="checkbox"> is stripped by the sanitizer — so the Done and
Dismissed columns rendered as '- []' text in every row.

The tables are now purely informational and the PR body ends with a
'Review checklist' section holding real task lists: fixes to verify, and
alerts to dismiss.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ilure

Macroscope is right that show_full_output publishes the whole agent
transcript — tool results, file reads, command output — to logs any repo
member can download, and GitHub only masks registered secret values. On
toolbelt it would also copy the full Dependabot alert JSON into logs
readable by people without security_events access.

Its diagnostic value was real (it is what revealed the 401 on the two
repos with a stale repo-level ANTHROPIC_API_KEY) but it is far wider than
needed. Replaced by a failure-only step that extracts just the terminal
error fields (subtype, is_error, terminal_reason, api_error_status, turns,
cost, and the first 400 chars of the result string) from the execution
log. Enough to tell an auth/API failure from an application one, without
publishing the transcript.

Also: the workflow-file push fallback now rewrites the commit
(reset --soft + restore + re-commit) instead of merely unstaging — the
files were already inside the single commit, so unstaging changed nothing
and the retry was rejected identically.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines +73 to +77
jq -r '[.. | objects | select(.type == "result")] | last
| {subtype, is_error, terminal_reason, api_error_status,
num_turns, total_cost_usd,
result: ((.result // "") | tostring | .[0:400])}' "$F" \
|| echo "Could not parse the execution log"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium workflows/security-fixes.yml:73

The failure-reporter step prints terminal_reason, api_error_status, and result as null/empty on the failures it is meant to diagnose, so it still cannot distinguish an API/auth failure from another terminal failure. The pinned claude-code-action result message does not expose those fields — it reports failures through its errors array. The jq filter selects .type == "result" but reads fields that are absent from that schema. Consider surfacing the result message's errors field (with truncation/redaction) instead.

Suggested change
jq -r '[.. | objects | select(.type == "result")] | last
| {subtype, is_error, terminal_reason, api_error_status,
num_turns, total_cost_usd,
result: ((.result // "") | tostring | .[0:400])}' "$F" \
|| echo "Could not parse the execution log"
jq -r '[.. | objects | select(.type == "result")] | last
| {subtype, is_error, num_turns, total_cost_usd,
errors: ((.errors // []) | map(.message // . | tostring) | join("; ") | .[0:400])}' "$F" \
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/security-fixes.yml around lines 73-77:

The failure-reporter step prints `terminal_reason`, `api_error_status`, and `result` as `null`/empty on the failures it is meant to diagnose, so it still cannot distinguish an API/auth failure from another terminal failure. The pinned `claude-code-action` result message does not expose those fields — it reports failures through its `errors` array. The `jq` filter selects `.type == "result"` but reads fields that are absent from that schema. Consider surfacing the result message's `errors` field (with truncation/redaction) instead.

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.

1 participant