Refactor harness permission-denied handling into shared helper module#36415
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes “permission denied” detection and escalation logic used by the claude, copilot, and codex harness entrypoints into a shared helper module, reducing duplication and drift risk across engines.
Changes:
- Added
permission_denied_helpers.cjsproviding shared counting, threshold detection, denied-command extraction, and issue-payload construction utilities. - Updated
claude_harness.cjs,copilot_harness.cjs, andcodex_harness.cjsto import the shared helpers and removed the duplicated in-file implementations. - Added
permission_denied_helpers.test.cjsto cover the extracted helper behavior independently of any single harness.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/permission_denied_helpers.cjs | Introduces shared helper functions for permission-denied classification, extraction, and payload creation. |
| actions/setup/js/permission_denied_helpers.test.cjs | Adds focused unit tests validating the shared helper behaviors. |
| actions/setup/js/claude_harness.cjs | Replaces duplicated permission-denied logic with imports from the shared helpers. |
| actions/setup/js/copilot_harness.cjs | Replaces duplicated permission-denied logic with imports from the shared helpers. |
| actions/setup/js/codex_harness.cjs | Replaces duplicated permission-denied logic with imports from the shared helpers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36415 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold 100, no custom config). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One blocker: test file uses ESM syntax in a .cjs module
The refactor itself is clean — the extraction is correct and the shared helper logic is identical to what was in all three harnesses. The only issue blocking merge is the test file.
permission_denied_helpers.test.cjs uses ESM import statements (import { describe, it, expect } from "vitest", import { createRequire } from "module"). Node.js treats .cjs files as CommonJS unconditionally, so this file will throw a SyntaxError before any test runs. The test coverage this PR adds effectively does not exist.
Fix: replace the ESM imports with require("vitest") and require("./permission_denied_helpers.cjs") directly.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.5M
| @@ -0,0 +1,33 @@ | |||
| import { describe, it, expect } from "vitest"; | |||
There was a problem hiding this comment.
Tests will never run: ESM import syntax in a .cjs file causes a SyntaxError at load time — Node.js unconditionally treats .cjs as CommonJS regardless of Vitest transforms.
💡 Suggested fix
Replace the ESM imports with CJS equivalents:
// `@ts-check`
"use strict";
const { describe, it, expect } = require("vitest");
const {
countPermissionDeniedIssues,
hasNumerousPermissionDeniedIssues,
extractDeniedCommands,
buildMissingToolPermissionIssuePayload,
} = require("./permission_denied_helpers.cjs");Drop createRequire(import.meta.url) (also ESM) and use require() directly — consistent with every other *.cjs test file in the project. The current file will produce zero test results in any runner that honours the .cjs extension contract.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /improve-codebase-architecture — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: Two small but meaningful test cases are missing — a below-threshold false-case for
hasNumerousPermissionDeniedIssues, and a non-emptydenied_commandscase forbuildMissingToolPermissionIssuePayload. - Stateful regex footgun: The module-level
/giregex is safe with the currentString.match()usage, but becomes a subtle trap for any future caller using.test()in a loop.
Positive Highlights
- ✅ Excellent scoping — only the duplicated logic moved, harness-specific retry policy untouched
- ✅ Clean, focused module with JSDoc on every function
- ✅ Test file follows established ESM-over-CJS pattern correctly
- ✅ The refactor eliminates ~198 lines of triplicated code with zero behaviour change — a textbook
/improve-codebase-architecturewin
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.6M
| it("detects numerous permission-denied issues at threshold", () => { | ||
| const output = "permission denied\npermission denied\npermission denied"; | ||
| expect(hasNumerousPermissionDeniedIssues(output)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Only the at-threshold (true) case is tested — missing a below-threshold assertion to guard against an accidental off-by-one if the threshold ever changes.
💡 Suggested addition
it("returns false when below the threshold", () => {
const output = "permission denied\npermission denied"; // 2 < 3
expect(hasNumerousPermissionDeniedIssues(output)).toBe(false);
});Without a false-case test, a regression that lowers the threshold or removes the check would still pass the suite.
| alternatives: "Verify token scopes, repository permissions, and MCP/tool access configuration.", | ||
| denied_commands: [], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] buildMissingToolPermissionIssuePayload is only tested with no arguments (empty denied_commands). The non-empty path — the one that fires in production — is untested.
💡 Suggested addition
it("builds missing_tool payload with denied commands list", () => {
const payload = JSON.parse(buildMissingToolPermissionIssuePayload(["git push", "npm publish"]));
expect(payload.denied_commands).toEqual(["git push", "npm publish"]);
});This guards the deniedCommands && deniedCommands.length > 0 conditional, which is the production path when extractDeniedCommands finds matches.
| "use strict"; | ||
|
|
||
| const PERMISSION_DENIED_PATTERN = /\b(?:permission denied|permissions denied|EACCES|EPERM)\b/gi; | ||
| const NUMEROUS_PERMISSION_DENIED_THRESHOLD = 3; |
There was a problem hiding this comment.
[/improve-codebase-architecture] The module-level /gi regex is stateful — its lastIndex advances on every .test() or .exec() call. It's safe today because countPermissionDeniedIssues only calls String.match() (which resets lastIndex), but any future caller using .test() in a loop will get intermittent misses.
💡 Suggestion: use a factory function instead
// Instead of a module-level regex constant:
const PERMISSION_DENIED_PATTERN = () => /\b(?:permission denied|permissions denied|EACCES|EPERM)\b/gi;
// Usage:
const matches = output.match(PERMISSION_DENIED_PATTERN());Or, since String.match() is the only consumer and it resets lastIndex, at minimum add a comment noting the statefulness risk to warn future callers.
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in I investigated the referenced |
The three harness entrypoints (
claude,copilot,codex) each carried near-identical permission-denied classification and payload code, creating parallel maintenance and drift risk. This change centralizes that pipeline so all engines use the same detection and escalation behavior.Shared permission-denied module
actions/setup/js/permission_denied_helpers.cjswith:countPermissionDeniedIssues(output)hasNumerousPermissionDeniedIssues(output)extractDeniedCommands(output)buildMissingToolPermissionIssuePayload(deniedCommands)Harness deduplication
actions/setup/js/claude_harness.cjsactions/setup/js/copilot_harness.cjsactions/setup/js/codex_harness.cjsFocused coverage for extracted logic
actions/setup/js/permission_denied_helpers.test.cjsto validate shared behavior independently of any single harness.