ci(ai-sdlc): run the spec guards on PRs, not only inside the agent - #319
Merged
Conversation
`verify-spec-ownership.mjs` and `check-spec-scope.mjs` ran only inside spec-agent.yml. `ci.yml` runs `node --test scripts/ai-sdlc/*.test.mjs` - it tests the guards, it never invokes them. So they guarded the machine path and not the change: - the spec ratification PR (Gate 1) never re-ran ownership on the version a human edits and merges; - PR #283, the delivery for issue #269, was written by hand and passed through none of them. So did 13bf6b3 and c0af5ec. The guardrails were attached to the machine path while the delivery went down the human path. This is the second time that inversion has shown up - the first was impl-agent.yml hardcoding `Refs #%s`, which makes agent PRs structurally incapable of skipping Gate 4 while hand-written ones can. Runs on `docs/specs/**` PRs only, so it costs nothing on the rest. Ownership is hard, scope stays advisory - and moving scope here is most of its value, since its warning previously landed in the spec-agent run's step summary rather than on the PR the reviewer is actually reading. Verified against real specs rather than assumed: spec 0237 gives "ownership lint passed - 4 declared path(s) checked" and "4 file(s) declared, within cap (6)". Spec 0226 has no `Files touched:` line and both guards skip with exit 0 - that fail-open path is pre-existing and is noted in the follow-up issue rather than silently relied on here. Deliberately spec-side only. The impl equivalent needs a PR-diff-level comparison that check-impl-scope.mjs explicitly declines to attempt (its own header: "does not re-check later commits a human pushes to the same PR before merge - that's a different, PR-diff-level check this script does not attempt"), plus spec resolution from the linked issue. That is new logic rather than a relocation, so it is filed separately. actionlint clean. Assisted-by: claude-opus-5 (agent) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a GitHub Actions workflow for pull requests that change ChangesSpecification guard automation
Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant GitHubActions
participant GitHistory
participant OwnershipValidator
participant ScopeValidator
PullRequest->>GitHubActions: Trigger on docs/specs/** changes
GitHubActions->>GitHistory: Compare pull request base and head
GitHistory-->>GitHubActions: Return changed Markdown specs
GitHubActions->>OwnershipValidator: Validate each changed spec
OwnershipValidator-->>GitHubActions: Return aggregated failures
GitHubActions->>ScopeValidator: Check each changed spec
ScopeValidator-->>GitHubActions: Emit advisory warnings
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.github/workflows/ai-sdlc-guards.yml (1)
103-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep advisory findings non-blocking without hiding execution failures.
check-spec-scope.mjsalready exits 0 for advisory findings.|| truealso hides syntax errors and launch failures. The step can appear successful when the scope guard did not run.Emit a warning when the command fails while keeping the step non-blocking.
This uses the supplied
check-spec-scope.mjscontract that advisory findings exit successfully.Proposed diagnostic handling
- SPEC_FILE="$spec" node scripts/ai-sdlc/check-spec-scope.mjs || true + if ! SPEC_FILE="$spec" node scripts/ai-sdlc/check-spec-scope.mjs; then + echo "::warning::check-spec-scope failed to run for a changed specification" + fi🤖 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 @.github/workflows/ai-sdlc-guards.yml at line 103, Update the check-spec-scope command in the workflow so advisory findings remain non-blocking while execution failures are detected and emit a GitHub Actions warning. Remove the unconditional `|| true` from the `SPEC_FILE` invocation and use failure handling that distinguishes the script’s successful advisory result from nonzero launch or runtime failures.
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/ai-sdlc-guards.yml:
- Line 55: Update the changed-file discovery command in the workflow to remove
the trailing “|| true” from the git diff invocation. Allow git diff failures to
propagate and fail the job, while preserving successful empty results as an
empty file list.
- Around line 91-92: Update the “Check spec scope (advisory)” step condition to
use !cancelled() instead of relying on the implicit success() condition,
ensuring it runs even when ownership validation fails while still skipping
cancelled workflow runs.
- Around line 52-54: Update the ownership gate in the workflow around
verify-spec-ownership.mjs so unreadable spec files cause a nonzero failure
instead of leaving failed=0 and passing; either make the script’s readFileSync
failure exit nonzero or validate and reject unreadable paths before invocation.
Revise the nearby comment to describe this failure behavior accurately while
preserving the deleted-spec handling.
- Around line 40-43: Update the “Checkout PR merge result” step and the
validator invocations in the workflow so guard scripts and policy data are
loaded from the trusted base revision rather than PR-controlled content. Ensure
verify-spec-ownership.mjs and docs/adr/README.md resolve from that base
revision, while preserving the workflow’s ability to validate the PR changes.
- Around line 40-43: Update the “Checkout PR merge result” actions/checkout@v4
step to set persist-credentials to false, preventing the GITHUB_TOKEN from being
stored in the repository configuration before guard scripts run.
---
Nitpick comments:
In @.github/workflows/ai-sdlc-guards.yml:
- Line 103: Update the check-spec-scope command in the workflow so advisory
findings remain non-blocking while execution failures are detected and emit a
GitHub Actions warning. Remove the unconditional `|| true` from the `SPEC_FILE`
invocation and use failure handling that distinguishes the script’s successful
advisory result from nonzero launch or runtime failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56fe4816-4551-417e-a16f-6509955db034
📒 Files selected for processing (1)
.github/workflows/ai-sdlc-guards.yml
Member
|
@claude review |
|
Reviewed 1 file ( Verified: |
There was a problem hiding this comment.
Pull request overview
Adds a dedicated GitHub Actions workflow to run the AI-SDLC spec guard scripts directly on pull requests that modify docs/specs/**, ensuring ownership and scope checks apply to human-authored edits before merge (not only inside agent workflows).
Changes:
- Introduces
.github/workflows/ai-sdlc-guards.ymltriggered only for PRs touchingdocs/specs/**. - Runs
verify-spec-ownership.mjsas a hard gate andcheck-spec-scope.mjsas advisory, iterating over all changed specs in the PR.
Suppressed comments (3)
.github/workflows/ai-sdlc-guards.yml:44
- Pin the Node.js version (and disable persisted checkout credentials) so these guard scripts run under the same Node major as the rest of CI, and to reduce risk when running on PR code.
- name: Checkout PR merge result
uses: actions/checkout@v4
with:
fetch-depth: 0
.github/workflows/ai-sdlc-guards.yml:55
- This
git diffis currently fail-open because of|| true(any diff error becomes an empty file list and the guard silently skips). Also,--diff-filter=AMmisses renames, and the pathspec only matches top-leveldocs/specs/*.mdeven though the workflow triggers ondocs/specs/**.
files=$(git diff --name-only --diff-filter=AM "$BASE_SHA" "$HEAD_SHA" -- 'docs/specs/*.md' || true)
.github/workflows/ai-sdlc-guards.yml:88
- The workflow-level failure annotation is a bit misleading:
verify-spec-ownership.mjsdoesn’t check whether a spec cites an ADR; it checks whether declared paths collide with components attributed to a different repo in docs/adr/README.md (unless explicitly out-of-scope). Consider aligning the message with what the script actually enforces.
echo "::error::Spec path ownership check failed. A spec declares files owned by an ADR it does not cite."
exit 1
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fixes real gaps found in review: catch renamed-and-modified specs by adding R to the ownership/scope file-discovery diff-filter (AM alone silently skipped them), disable credential persistence on the PR checkout, fail closed instead of fail open when file discovery errors, run the advisory scope check even when the hard ownership gate fails, correct the ownership-failure error text to match what the script actually checks, surface (rather than silently swallow) a scope-check launch failure, and fix a transposed commit SHA in a header comment. Assisted-by: claude-opus-5 (agent)
Every other workflow that runs scripts/ai-sdlc/*.mjs (spec-agent.yml, impl-agent.yml, ci.yml) pins Node 22 via actions/setup-node. This workflow ran the same scripts on whatever Node ubuntu-latest happens to ship, an unpinned dependency the rest of CI already avoids. Assisted-by: claude-opus-5 (agent)
Collaborator
Author
|
Addressing findings that weren't posted as their own inline threads:
Pushed as two commits; also replied to all 5 inline threads individually. |
Member
|
@claude review |
|
Reviewed 1 changed file (the new AI-SDLC guards workflow); no real-impact issues found. Security posture is clean (pull_request not pull_request_target, minimal contents:read perms, persist-credentials:false, all shell interpolation routed through env: blocks), and the diff/heredoc/per-file-iteration pipeline matches the stated intent. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/ai-sdlc-guards.yml:64
git diff "$BASE_SHA" "$HEAD_SHA"compares the two commits directly and can include unrelated changes if the PR branch is behind the base branch (it will show base-only changes as deletions). For a PR-only file list you generally want the merge-base diff (base...head) so only the PR’s changes are checked.
# R is included because a rename that also edits content (e.g. a
# spec renumbered alongside a wording fix) would otherwise be
# invisible to AM and skip both guards silently.
files=$(git diff --name-only --diff-filter=AMR "$BASE_SHA" "$HEAD_SHA" -- 'docs/specs/*.md')
Like2Read
approved these changes
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
verify-spec-ownership.mjsandcheck-spec-scope.mjsran only insidespec-agent.yml.ci.ymlrunsnode --test scripts/ai-sdlc/*.test.mjs- it tests the guards, it never invokes them.So they guarded the machine path and not the change:
13bf6b3andc0af5ecThe guardrails were attached to the machine path while the delivery went down the human path. This is the second appearance of that inversion - the first is
impl-agent.ymlhardcodingRefs #%s, which makes agent PRs structurally incapable of skipping Gate 4 while hand-written ones can.What this does
Runs on
docs/specs/**PRs only, so it costs nothing on the rest. Ownership is hard; scope stays advisory. Moving scope here is most of its value on its own: its warning previously landed in the spec-agent run's step summary, a surface the person ratifying the spec never opens.Both guards run against every changed spec before failing, so a PR touching two specs reports both rather than only the first.
Verified, not assumed
0237-...0226-...Files touched:line, both guards skip, exit 0That second row is a pre-existing fail-open path, not something this PR introduces -
verify-spec-ownership.mjsexits 0 on four separate paths (unreadable spec, missingdocs/adr/README.md, zero foreign tokens parsed, noFiles touched:line). It is named in the follow-up rather than quietly relied on here.Deliberately spec-side only
The impl equivalent is not a relocation.
check-impl-scope.mjssays so itself:That needs new logic plus spec resolution from the linked issue, so it is filed separately.
actionlintclean. Additive - no existing workflow changes behaviour.Refs #320
Assisted-by: claude-opus-5 (agent)
Co-authored-by: Claude Opus 5 (1M context) noreply@anthropic.com