feat(ci): AI-first CI workflows — review, interact, health monitor, smoke test - #2459
serrrfirat wants to merge 12 commits into
Conversation
12a4ece to
8aaddf4
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces an 'AI-First CI' strategy by updating the engine's system prompts to prioritize direct tool calls over CodeAct orchestration and adding a comprehensive design and implementation plan for new GitHub Actions workflows. The plan outlines the addition of per-PR AI reviews, interactive Claude responses in issues, and a daily CI health monitor. Feedback focuses on improving the robustness of shell scripts within the proposed workflows, specifically regarding file filtering logic and ensuring sufficient data retrieval limits for CI health reporting.
Three-tier model strategy: Haiku for per-PR early feedback, Sonnet for the staging promotion gate (deeper reasoning), Opus reserved for the CI health monitor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Lightweight 2-agent review (quality+bugs, security) runs on every non-draft PR targeting staging. Non-blocking informational feedback. Skips docs-only and JSON-only changes.
Engineers can mention @claude in any issue or PR comment to get AI-powered code analysis, bug investigation, or explanations. Uses Sonnet for investigation depth, bounded to 30 turns.
Scheduled daily at 8 AM UTC. Collects CI run outcomes, detects flaky tests, checks dependabot alerts and open bugs. Opus analyzes patterns and maintains a rolling health report issue with action items.
Pulls the just-pushed image, starts it with minimal config, and verifies the health endpoint responds. Creates a GitHub issue on failure with container logs and workflow link.
After building and pushing the Docker image, the smoke test workflow runs against the sha-tagged image. Failure creates a GitHub issue; success is silent.
bb8b0de to
049ee9f
Compare
Code ReviewOverviewSolid PR — well-structured tiered model strategy, good security posture (read-only code access, bot-loop prevention), smart filtering (draft/docs-only skip, IssuesHIGH — Unnecessary
|
henrypark133
left a comment
There was a problem hiding this comment.
Review: AI-first CI workflows (Risk: High)
Good operational additions — health monitor, deploy verification, and interactive Claude are valuable CI capabilities. However, two security issues need resolution before merge.
Positives:
- All actions properly SHA-pinned to commit hashes
- deploy-verify.yml correctly handles container crashes (docker ps check in retry loop)
- Concurrency groups prevent duplicate runs per issue/PR
persist-credentials: falseon all checkouts
Critical: claude-interact.yml lacks author restriction — prompt injection risk [Security]
File: .github/workflows/claude-interact.yml:22-25
The if: guard only blocks bot accounts (claude[bot], github-actions[bot]), not external contributors. Any GitHub user who can comment on issues/PRs can trigger Claude with arbitrary instructions. The workflow has issues: write and pull-requests: write — a crafted @claude comment could post misleading content under the bot identity.
Suggested fix: Add author association check:
if: >
contains(github.event.comment.body, '@claude') &&
contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) &&
github.event.comment.user.login != 'claude[bot]'Critical: Unnecessary id-token: write permission [Security]
Files: .github/workflows/claude-interact.yml:13, .github/workflows/claude-review-pr.yml:11
Neither workflow uses OIDC authentication. This permission enables GitHub OIDC token minting — a force-multiplier if combined with prompt injection (finding above).
Suggested fix: Remove id-token: write from both workflows.
Concerning: claude-review-pr.yml overlaps with claude-review.yml [Architecture]
Files: .github/workflows/claude-review-pr.yml, .github/workflows/claude-review.yml
Both fire on PRs to staging (different triggers: open/synchronize vs labeled). On promotion PRs, both run simultaneously. The staging gate in staging-ci.yml reads the last claude[bot] comment — a race could cause the wrong review to be evaluated.
Suggested fix: Either remove claude-review-pr.yml (the existing workflow already covers promotion PRs) or use distinct comment signatures so the gate can differentiate.
Concerning: Model upgrade haiku→sonnet with unchanged 50-turn/4-agent budget [Architecture]
File: .github/workflows/claude-review.yml:33
5-8x cost increase per review. With hourly staging promotions, significant monthly cost delta. Consider reducing --max-turns to 15-20 if upgrading to Sonnet.
Convention notes:
cancel-in-progress: falseon claude-interact is acceptable (concurrency group is per-issue) buttruewould be safer against comment spam- Consider extracting inline Claude prompts to
.github/prompts/*.mdper the project's "prompt templates live in files" convention
zmanian
left a comment
There was a problem hiding this comment.
Review: AI-first CI workflows
+511/-1, 6 files. Adds Claude-powered PR review, issue interaction, health monitoring, and deploy verification workflows.
Security (must fix)
-
claude-interact.ymlis open to any GitHub commenter. Theif:guard only blocks two bot accounts. Any external user who can comment on a public issue can trigger Claude withissues: write+pull-requests: writepermissions — prompt injection vector. Add anauthor_associationcheck restricting toOWNER,MEMBER,COLLABORATOR. -
id-token: writeis unnecessary in bothclaude-interact.ymlandclaude-review-pr.yml. Neither uses OIDC. Remove to reduce blast radius.
Architecture (should fix)
-
Duplicate review triggers on staging PRs.
claude-review-pr.ymlfires onpull_request: [opened, synchronize]tostaging, while the existingclaude-review.ymlfires on labeled PRs tostaging. Promotion PRs trigger both, risking a race where the staging gate evaluates the wrong (lighter) review. Scope one to exclude the other. -
Sonnet upgrade + unchanged 50-turn budget. The Haiku→Sonnet change in
claude-review.ymlis 5-8x cost increase. Consider reducing to 15-20 turns.
Minor
-
ci-health-monitor.yml--limit 200may silently truncate busy weeks. -
deploy-verify.ymlinterpolates container logs into issue body without sanitization — potential markdown injection from compromised images.
Agree with Henry's existing CHANGES_REQUESTED. Items 1-2 are security blockers.
|
all of these are fair comments. I will attend to these tomorrow and ask for re-reviews. |
…conventions (#2459) Security: - Add author_association guard (OWNER/MEMBER/COLLABORATOR) to claude-interact - Remove unnecessary id-token: write from claude-interact and claude-review-pr - Sanitize container logs in deploy-verify to prevent markdown injection Architecture: - Exclude staging-promotion PRs from claude-review-pr (prevents race with promotion gate) - Reduce claude-review max-turns from 50 to 20 (Sonnet cost control) - Increase ci-health-monitor run limit from 200 to 1000 Conventions: - Extract all inline Claude prompts to .github/prompts/*.md - Set cancel-in-progress: true on claude-interact Bug fix: - Consolidate grep filters and add empty-line guard in file change detection
Addressed review feedback (a3db6f0)Security (henrypark133 + zmanian)
Architecture (henrypark133 + zmanian)
Convention / minor
|
zmanian
left a comment
There was a problem hiding this comment.
Review: AI-First CI Workflows
Critical
1. claude-interact.yml -- COLLABORATOR author_association is too broad.
The guard allows anyone who has ever had a PR merged. Combined with pull-requests: write, issues: write, and Bash(cargo check:*, cargo clippy:*) tool access, a former contributor could craft a comment that triggers cargo commands against a malicious branch. The prompt says "Do NOT attempt to build" but the tool allowlist contradicts this.
- Restrict to
OWNER+MEMBERonly, or use an explicit username allowlist - Remove
cargo check/cargo clippyfrom allowed tools
2. docker.yml -- secrets: inherit passes ALL repo secrets to smoke test.
deploy-verify.yml only needs github.token, but secrets: inherit exposes ANTHROPIC_API_KEY, Docker Hub credentials, and everything else. If the tested container is compromised, secrets could leak.
- Replace with explicit secret passing, or remove
secrets: inheritentirely (defaultGITHUB_TOKENis passed automatically)
High
3. claude-review-pr.yml uses pull_request trigger -- silent no-op on fork PRs since the ANTHROPIC_API_KEY secret isn't available in fork context. Should be documented as intentional or switched to pull_request_target with safeguards.
4. claude-review.yml -- Sonnet upgrade with 4 parallel sub-agents could be costly per promotion PR. No cost ceiling. Acceptable if monitored, but worth documenting expected per-run cost.
Medium
5. ci-health-monitor.yml -- issue dedup is fragile (keyword search only). Could create duplicate issues on repeated runs. Also ~$15-30/run with Opus -- consider whether Sonnet suffices for this structured task.
6. deploy-verify.yml -- log sanitization only escapes backticks. Container logs could contain markdown injection or sensitive env vars that get embedded in auto-created issues.
Positives
- All GitHub Actions are SHA-pinned (good supply-chain hygiene)
- Prompts extracted to
.github/prompts/*.md - Tool allowlists are restrictive (no
Bash(*)wildcard) persist-credentials: falseused consistently- Concurrency groups prevent parallel runaway
…2459) Security: - Restrict claude-interact author_association to OWNER+MEMBER (drop COLLABORATOR) - Remove cargo check/clippy from claude-interact allowed tools - Remove secrets: inherit from docker.yml verify job (only needs github.token) - Broaden deploy-verify log sanitization: strip ANSI codes, redact secret-like env vars Architecture: - Downgrade ci-health-monitor from Opus to Sonnet (~$1-2/run vs ~$15-30) - Document fork PR no-op behavior in claude-review-pr.yml - Document expected per-run cost in claude-review.yml - Document issue dedup trade-off in ci-health-monitor.yml
Addressed zmanian review round 2 (625935a)Security
Architecture / cost
|
zmanian
left a comment
There was a problem hiding this comment.
Re-review: All critical/high findings addressed
| Finding | Status |
|---|---|
claude-interact.yml author_association + cargo tools |
FIXED -- restricted to OWNER+MEMBER, cargo tools removed |
docker.yml secrets: inherit |
FIXED -- removed, only github.token used |
| Fork PR silent skip undocumented | FIXED -- comment block added |
| Sonnet + 4 agents cost ceiling | FIXED -- max-turns reduced to 20, cost documented |
| Health monitor dedup + Opus cost | FIXED -- switched to Sonnet (~$1-2/run) |
| Log sanitization | PARTIALLY FIXED -- ANSI stripping + keyword redaction added, acceptable risk since container gets no real secrets |
No new issues introduced by the fix commits. LGTM.
Three-tier model strategy: - Haiku: per-PR early feedback (lightweight, high volume) - Sonnet: interactive @claude + CI health monitor (balanced) - Opus: staging promotion gate (deep reasoning for 4-agent consolidation) Promotion PRs are low-frequency, high-stakes — Opus's superior reasoning justifies the cost (~$10-20/review) for the final gate.
) CI fix: - Restore id-token: write to claude-review-pr.yml (required by claude-code-action for OIDC auth — removal was a false positive) - Remove redundant always() wrapper in docker.yml verify job Model upgrades (use latest stable IDs): - Haiku: claude-haiku-4-5-20251001 → claude-haiku-4-5 - Sonnet: claude-sonnet-4-5-20250929 → claude-sonnet-4-6 - Opus: already on claude-opus-4-6 (unchanged)
- Add RUSTSEC-2026-0098 and RUSTSEC-2026-0099 (rustls-webpki URI/wildcard name constraint validation) — 0.102.8 pinned by libsql transitive dep, 0.103.x awaiting upstream patch - Remove 4 stale wasmtime advisories (RUSTSEC-2025-0046, RUSTSEC-2025-0118, RUSTSEC-2026-0020, RUSTSEC-2026-0021) that no longer match any crate after wasmtime upgrade to v43 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
There was a problem hiding this comment.
Verified finding:
.github/workflows/ci-health-monitor.yml:63-69 serializes flaky runs as "<workflow> <sha>" and then parses them with while read -r workflow sha. That breaks as soon as the workflow name contains spaces. For example, "Docker Image abc123" is parsed as workflow=Docker and sha="Image abc123", so the follow-up jq lookup never matches the failed run and no flaky log is collected. Several of this repo's workflow names are multi-word, so the health monitor silently drops the failure-log enrichment for the common case.
Suggested fix: emit structured data (JSON, tab-separated, or NUL-separated fields) and parse that instead of splitting on spaces.
Multi-word workflow names (e.g. "Docker Image") broke the space-delimited `while read -r workflow sha` loop — the name was split across both variables so the jq lookup never matched. Switch to tab-separated jq output with IFS=$'\t'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed @henrypark133's flaky-run parsing feedback in c032e7e:
No other unresolved review items remain. |
henrypark133
left a comment
There was a problem hiding this comment.
Review: interactive workflow still reads the wrong tree on PR comments
The permission tightening and log-parsing fixes look good, but one correctness issue remains in the interactive workflow.
Concerning: PR comment investigations still run against the default branch checkout
File: .github/workflows/claude-interact.yml:28
issue_comment and pull_request_review_comment both trigger on PR discussions, and the prompt explicitly tells the agent to use Read/Glob plus file:line references when analyzing code. This checkout step never switches to the PR head (refs/pull/<n>/head), so for PR comments the workspace contains the repository default branch, not the commented change. The result is that @claude can read stale files, cite wrong line numbers, and answer review threads against code that is not actually under discussion.
Suggested fix: When the comment is attached to a PR, check out the PR head (or merge ref) before invoking Claude. For issue-only comments, keep the default-branch checkout.
|
Context: this is review feedback informed by a broader 2-week velocity/quality audit of the repo. Flagging upfront so the suggestions land as a strategic read, not drive-by nits. What's genuinely good
Strategic concernThis PR adds a 3rd AI reviewer without retiring the first two. Copilot and Gemini still fire on every PR. Large PRs already show the cost of that firehose — #2515 drew ~7 bot reviews on top of 33 human rounds. Adding a Haiku reviewer is the right call, but the velocity win only materializes if Copilot + Gemini are demoted to summary-only (or off) in the same change. Otherwise net reviewer count goes up, not down. Specific issues worth addressing before merge
Suggested splitThis is
Ship A and B this week. Hold C until spend caps land. Happy to help draft any of these if useful. |
|
Closing this PR based on team discussion around pushing more local testing on pre hooks rather than making noise on the CI. Will come up with a redesign asap. |
Summary
Adds four GitHub Actions workflows (and modifies two existing ones) to move IronClaw's CI from AI-assisted to AI-first, inspired by CREAO's harness engineering approach.
@claudein any issue or PR comment to get Sonnet-powered investigation and analysisThree-Tier Model Strategy
Estimated monthly cost at 10 PRs/day: $100-250/month.
Files Changed
.github/workflows/claude-review-pr.yml.github/workflows/claude-interact.yml.github/workflows/ci-health-monitor.yml.github/workflows/deploy-verify.yml.github/workflows/claude-review.yml.github/workflows/docker.ymldocs/superpowers/specs/docs/superpowers/plans/Test plan
python3 -c "import yaml; yaml.safe_load(open(f))"for each workflowclaude-review-pr.yml@claude what does this PR do?to triggerclaude-interact.ymlci-health-monitor.ymlvia Actions UIdeploy-verify.ymlwith a known-good image tagdocker.ymlbuild triggers the smoke test verify job🤖 Generated with Claude Code