[jsweep] Clean add_copilot_reviewer.cjs#21898
Conversation
- Replace verbose block comment with concise JSDoc on main() - Remove redundant inline comment - Add 3 new edge-case tests (11 → 14 tests): - ERR_VALIDATION code appears in validation error messages - core.error called before core.setFailed on API errors - summary.addRaw includes PR number on success Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot review changes |
Reviewed the changes in commit Changes are clean:
|
There was a problem hiding this comment.
Pull request overview
Cleans up the add_copilot_reviewer github-script (comment-only) and expands its test suite, while also updating several workflow lock files to include additional MCP guard policy fields.
Changes:
- Simplified header documentation in
add_copilot_reviewer.cjs(no functional logic change). - Added 3 new tests covering validation error codes, error-path call ordering, and summary contents.
- Updated multiple workflow
*.lock.ymlfiles to addrepos: "all"to GitHub MCPallow-onlyguard policies and a derivedsafeoutputswrite-sinkguard policy.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/add_copilot_reviewer.cjs | Replaces verbose block comment with concise JSDoc (documentation-only change). |
| actions/setup/js/add_copilot_reviewer.test.cjs | Adds edge-case tests for validation error codes, error handling call order, and summary content. |
| .github/workflows/workflow-generator.lock.yml | Updates MCP gateway config guard policies (repos: "all", adds safeoutputs write-sink). |
| .github/workflows/weekly-safe-outputs-spec-review.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/weekly-issue-summary.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/stale-repo-identifier.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/refiner.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/pr-triage-agent.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/org-health-report.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/issue-triage-agent.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/issue-monster.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/issue-arborist.lock.yml | Adds write-sink guard policy in TOML config + same JSON guard policy updates. |
| .github/workflows/grumpy-reviewer.lock.yml | Same guard policy updates in the locked workflow config. |
| .github/workflows/discussion-task-miner.lock.yml | Same guard policy updates in the locked workflow config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "guard-policies": { | ||
| "write-sink": { | ||
| "accept": [ | ||
| "*" |
There was a problem hiding this comment.
This introduces a write-sink guard policy with accept: ["*"], which (per docs/src/content/docs/reference/github-tools.md:151-157) allows all write operations for the non-GitHub MCP server. If this workflow doesn’t require unrestricted safe-outputs writes, consider tightening the accept list to the minimum needed scope to reduce blast radius.
| "*" | |
| "workflow-generator/*" |
| mockCore.error.mockImplementation(() => callOrder.push("error")); | ||
| mockCore.setFailed.mockImplementation(() => callOrder.push("setFailed")); |
There was a problem hiding this comment.
In this test, mockImplementation is set for core.error/core.setFailed but beforeEach only calls vi.clearAllMocks(), which does not reset mock implementations. This can leak the implementation (and the captured callOrder array) into subsequent tests and make the suite order-dependent. Prefer mockImplementationOnce(...) for both calls here, or switch the suite setup to vi.resetAllMocks() (or explicitly restore the original implementations) in beforeEach.
| mockCore.error.mockImplementation(() => callOrder.push("error")); | |
| mockCore.setFailed.mockImplementation(() => callOrder.push("setFailed")); | |
| mockCore.error.mockImplementationOnce(() => callOrder.push("error")); | |
| mockCore.setFailed.mockImplementationOnce(() => callOrder.push("setFailed")); |
| }, | ||
| "guard-policies": { | ||
| "allow-only": { | ||
| "min-integrity": "approved" | ||
| "min-integrity": "approved", | ||
| "repos": "all" | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR description only mentions changes to actions/setup/js/add_copilot_reviewer.cjs and its tests, but this PR also updates workflow lock files (e.g., guard policy config here). Please update the PR description to reflect these additional workflow configuration changes, or split them into a separate PR so reviewers can assess the policy impact independently.
Summary
Cleaned
actions/setup/js/add_copilot_reviewer.cjsas part of the daily jsweep unbloat pass.Context type: github-script
Changes to
add_copilot_reviewer.cjsmain()// Validate required environment variablesinline comment (obvious from code)The try/catch was preserved — it handles API failures with
core.error+core.setFailed(legitimate control flow).Test improvements
Added 3 new edge-case tests (11 → 14 tests):
should include ERR_VALIDATION in message for non-numeric PR_NUMBERshould call core.error before core.setFailed on API errorshould write summary with PR number on success✅ Validation checks
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js— 14 tests pass ✓