feat: implement issue #433 — Compliance: copilot-setup-steps-invalid-job-name#489
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe compliance audit script's Changescopilot-setup-steps job-name detection fix and tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new BATS test suite to verify the job-name detection logic for the copilot-setup-steps workflow. The tests cover both compliant workflows and various near-miss scenarios. Feedback highlights a critical syntax error in the actual implementation script due to unescaped single quotes in a Python string, and suggests adding a file existence check in the tests to prevent abrupt failures if the workflow file is missing.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 3278ca89d61cc74e3cef123b7f37dbb3207af3c9
Review mode: triage-approved (single reviewer)
Summary
Test-only addition plus a one-line bug fix. The PR adds a BATS suite (test/scripts/compliance-audit/copilot-setup-steps-job-name.bats, 150 lines) pinning the indentation-aware parser in check_copilot_setup_steps, and fixes a real bug in scripts/compliance-audit.sh: the embedded python3 -c regex used a literal single quote (['] inside ["']?) within a single-quoted shell argument, which broke the shell string and made the compliance check error out. The fix replaces the literal quote with \x27 (regex hex escape for '), keeping the python valid inside the single-quoted block while still matching both quoted and unquoted YAML job keys. Triage's low-risk assessment is correct.
Linked issue analysis
Closes #433 (copilot-setup-steps-invalid-job-name compliance check). The issue is substantively addressed: the parser bug that made the check non-functional is fixed, and a near-miss test matrix (misnamed job, name only in a comment, name at wrong indent, no jobs section, quoted/unquoted/non-first job) guards against regressions.
Findings
No new issues. Both prior automated-review findings are resolved on the reviewed SHA: (1) gemini's CRITICAL — literal single quote in the script's python regex — fixed at compliance-audit.sh:1305 (now r"^["\x27]?..."), confirmed by 255 passing BATS tests; (2) gemini's LOW — missing file-existence guard in the first test — added (skip when copilot-setup-steps.yml is absent). No unresolved review threads or unanswered human questions. Secret scan: GitHub Secret Protection MCP tool was not grantable in this Action context; noted and skipped — changed content is a regex fix and test fixtures with no credentials, and the gitleaks CI check passed.
CI status
All required checks green: CI Lint, ShellCheck, Secret scan (gitleaks), Agent Security Scan, AgentShield, CodeQL, and SonarCloud (Quality Gate passed) all SUCCESS. The CANCELLED dev-lead/dispatch and ci-relay runs are the authoring agent's own superseded workflow runs, not gating checks. mergeStateStatus is BEHIND (branch behind main) but mergeable — handled by auto-rebase, not a review blocker.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/scripts/compliance-audit/copilot-setup-steps-job-name.bats`:
- Around line 88-95: Add a new test function after the existing "quoted job key
is recognised" test that validates single-quoted job keys in the _has_job
function. Create a test with a similar structure and assertions, but replace the
double-quoted job key "copilot-setup-steps" with a single-quoted equivalent
'copilot-setup-steps' to ensure the regex pattern in _has_job handles both quote
styles consistently.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b135e98-a00e-43c0-ad66-615816163cb4
📒 Files selected for processing (2)
scripts/compliance-audit.shtest/scripts/compliance-audit/copilot-setup-steps-job-name.bats
Dev-Lead — fix-bot-comment (applied)Changes committed and pushed. |
don-petry
dismissed stale reviews from coderabbitai[bot] and donpetry-bot
via
426f9e4
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #489 |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 426f9e4b2cd602eb6a9f6190d613b6c794b21b27
Review mode: triage-approved (single reviewer)
Summary
Test-only addition plus a one-line bug fix. scripts/compliance-audit.sh:1305 had a real shell-quoting bug: the embedded python3 -c '...' block (single-quoted at the shell level, line 1272) used a literal single quote inside the regex character class ["']?, which prematurely closed the shell single-quoted string and broke check_copilot_setup_steps. The fix replaces the literal ' with the regex hex escape \x27, which matches a single quote in Python without placing a literal quote in the shell argument, while still matching both quoted and unquoted YAML job keys. The new BATS suite (test/scripts/compliance-audit/copilot-setup-steps-job-name.bats, 159 lines) mirrors the parser and pins behaviour across compliant cases (repo file, minimal, double-quoted, single-quoted, non-first job) and near-misses (misnamed job, name only in a comment, wrong indent, no jobs section). Triage's low-risk assessment is confirmed correct.
Linked issue analysis
Closes #433 (copilot-setup-steps-invalid-job-name compliance check). Substantively addressed: the quoting bug that broke the detection logic is fixed, and a near-miss test matrix guards against regressions.
Findings
No new issues. Prior automated-review findings are resolved at the head SHA: gemini's CRITICAL (literal single quote in the script's python regex) is fixed at compliance-audit.sh:1305; CodeRabbit's CHANGES_REQUESTED at e6849d3 (add a single-quoted job-key test) is addressed — the 'single-quoted job key is recognised' test was added, and CodeRabbit re-reviewed at the head SHA and APPROVED. The prior single-reviewer APPROVED review was auto-dismissed only because a new commit was pushed; that commit is the CodeRabbit fix. No unresolved review threads or unanswered human questions. Secret scan: the GitHub secret-scanning MCP tool errored (GitHub Advanced Security not enabled for the repo) — noted and skipped; changed content is a regex fix and test fixtures with no credentials, and the gitleaks CI check passed.
CI status
All required checks green: CI Lint, ShellCheck, Secret scan (gitleaks), Agent Security Scan, AgentShield, CodeQL, SonarCloud (Quality Gate passed), and Dependency audit (Detect ecosystems) all SUCCESS. CANCELLED dev-lead/dispatch and ci-relay runs are the authoring agent's own superseded workflow runs, not gating checks. mergeStateStatus is BEHIND (branch behind main) but MERGEABLE — handled by auto-rebase, not a review blocker.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
2 participants
Closes #433
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
Bug Fixes
jobs.copilot-setup-stepskey across YAML-quoted variants, reducing false negatives during checks.Tests
jobs).