Repository navigation
Conversation
- Replace core.error() + process.exit(1) with throw new Error() in requireEnvVar() and loadGatewayContext() for better testability - Add /// <reference types="@actions/github-script" /> for type safety - Create convert_gateway_config_shared.test.cjs with 23 tests covering: rewriteUrl, normalizeGatewayEntry, filterAndTransformServers, writeSecureOutput, loadGatewayContext, logCLIFilters, logServerStats All validation checks passed: format, lint, typecheck, tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47192 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
Great work on the jsweep cleanup! 🧹 This PR does an excellent job: ✅ Clean refactoring — Replacing ✅ Comprehensive test coverage — The new test suite adds 23 tests covering all exported functions with thoughtful scenarios (immutability checks, edge cases, error paths). ✅ Well documented — The PR body clearly explains what changed, why (testability), and provides validation evidence. ✅ Type safety — The This looks ready for review and merge!
|
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Refactors gateway configuration error handling for testability and adds comprehensive shared-helper tests.
Changes:
- Replaces process termination with thrown errors.
- Adds GitHub Script type reference.
- Adds tests for all exported shared helpers.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/convert_gateway_config_shared.cjs |
Updates error handling and type declarations. |
actions/setup/js/convert_gateway_config_shared.test.cjs |
Adds shared-helper unit tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
| it("throws on write failure", () => { | ||
| expect(() => writeSecureOutput("/nonexistent/deep/path/file.json", "{}")).toThrow(); | ||
| }); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look correct and well-tested.
throw new Error()replacingcore.error() + process.exit(1)is a clear improvement: errors become testable without process-exit side effects, and callers already propagate exceptions naturally.- The triple-slash
/// <reference types="@actions/github-script" />directive correctly scopes thecoreglobal type. - 23 new tests cover all exported functions with meaningful edge cases.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 17.4 AIC · ⌖ 5.09 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review
Applied /tdd — two minor issues worth addressing, no blocking concerns.
Key Themes & Highlights
Issues Found
- ESM/CJS mismatch (line 2): Test file uses
importsyntax but has a.cjsextension. Works at runtime via Vitest transform but is semantically inconsistent and can trip up tools. - Temp dir cleanup (writeSecureOutput tests): Inline
fs.rmSynconly runs on assertion success; a failed assertion leaks the temp directory. TheloadGatewayContexttests already show the correctbeforeEach/afterEachpattern.
Positive Highlights
- The
core.error + process.exit(1)→throw new Error()change is exactly the right move for testability. - 23 tests with solid arrange/act/assert structure and good edge case coverage across all exported functions.
loadGatewayContexttests correctly save/restoreprocess.envinbeforeEach/afterEach.- The
/// <reference types>triple-slash directive cleanly resolves thecoreglobal typing.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 22.4 AIC · ⌖ 4.71 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -0,0 +1,224 @@ | |||
| // @ts-check | |||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | |||
There was a problem hiding this comment.
[/tdd] ESM import syntax in a .cjs file — the .cjs extension signals CommonJS to Node, but import/export is ES-module syntax. Tests pass via Vitest's transform, but this mismatch can confuse editors, type-checkers, and CI configs that inspect the extension.
💡 Options to resolve
- Rename the file to
.test.mjs(or.test.jsif"type":"module"is set), or - Switch to
require()/module.exportsto stay consistently CommonJS.
@copilot please address this.
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "gateway-test-")); | ||
| const outputPath = path.join(dir, "output.json"); | ||
| writeSecureOutput(outputPath, '{"key":"value"}'); | ||
| expect(fs.readFileSync(outputPath, "utf8")).toBe('{"key":"value"}'); |
There was a problem hiding this comment.
[/tdd] The writeSecureOutput tests use fs.mkdtempSync with inline cleanup via fs.rmSync, but cleanup is only reached on success. If the assertion throws, the temp directory leaks.
💡 Use afterEach for cleanup
describe('writeSecureOutput', () => {
let dir;
beforeEach(() => { dir = fs.mkdtempSync(...); });
afterEach(() => { fs.rmSync(dir, { recursive: true, force: true }); });
// tests no longer need inline rmSync
});This mirrors the pattern already used in loadGatewayContext.
@copilot please address this.
There was a problem hiding this comment.
❌ Request Changes — test file is broken as written
The core logic change (replacing core.error + process.exit(1) with throw new Error) is correct and makes the module testable. The test coverage intent is good. However, the test file itself has a critical defect that prevents it from running at all, plus two secondary issues.
Findings summary
Critical — test file cannot run
- Line 2:
import(ESM) syntax in a.cjsfile. Node.js will throwSyntaxError: Cannot use import statement in a modulebefore any test runs. This means the PR's claimed 31/31 test pass is suspicious — either the test runner is somehow transpiling, or the results were not from this file. Needs investigation and fix (rename or switch torequire).
Medium — test pollution
- Line 161:
global.coreis set without being restored, leaking into subsequent tests in the same worker.
Low — inconsistent resource cleanup
- Lines 126–131:
writeSecureOutputtests don't usetry/finallyfor temp-dir cleanup, unlike theloadGatewayContexttests directly below.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 37.5 AIC · ⌖ 4.86 AIC · ⊞ 5.6K
Comment /review to run again
| @@ -0,0 +1,224 @@ | |||
| // @ts-check | |||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | |||
There was a problem hiding this comment.
ESM import syntax in a .cjs file will throw a SyntaxError at runtime. Node.js unconditionally treats .cjs files as CommonJS; every import statement here makes these tests un-runnable.
💡 Suggested fix
Rename to convert_gateway_config_shared.test.mjs (or .test.js if the project's vitest config already enables ESM), then update any runner glob patterns. The existing adapter test files nearby may show the established convention. If CJS is required, switch all top-level imports to require() — vitest CJS usage would need to pull from require('vitest') or via a createRequire shim.
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "gateway-test-")); | ||
| const gatewayFile = path.join(dir, "gateway.json"); | ||
| const servers = { github: { url: "http://old/mcp/github", type: "http" } }; | ||
| fs.writeFileSync(gatewayFile, JSON.stringify({ mcpServers: servers })); |
There was a problem hiding this comment.
global.core is set in beforeEach but never cleared in afterEach, leaking the mock into any subsequent test suite or file sharing the same vitest worker.
💡 Suggested fix
Restore the original value (or delete the key) in afterEach:
let mockCore;
let _originalCore;
beforeEach(() => {
_originalCore = global.core;
mockCore = { info: vi.fn(), warning: vi.fn() };
// `@ts-ignore`
global.core = mockCore;
});
afterEach(() => {
global.core = _originalCore;
});Without this, if any later test in the same worker touches global.core it receives the vitest spy object instead of the real or undefined value, producing non-deterministic failures.
| beforeEach(() => { | ||
| savedEnv = { ...process.env }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Temp directories created by writeSecureOutput tests are not cleaned up if an assertion fails mid-test — unlike the loadGatewayContext tests which use try/finally.
💡 Suggested fix
Wrap cleanup in try/finally consistently:
it("writes file with mode 0o600", () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "gateway-test-"));
try {
const outputPath = path.join(dir, "output.json");
writeSecureOutput(outputPath, '{"key":"value"}');
expect(fs.readFileSync(outputPath, "utf8")).toBe('{"key":"value"}');
expect(fs.statSync(outputPath).mode & 0o777).toBe(0o600);
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
});Without this, temp dirs accumulate on the runner when tests fail, which can affect later runs or disk space.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (19 tests)
|
…hared-6b804901cb528abd
|
@copilot please run the Unresolved review threads (newest first):
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
All review feedback has been addressed in commit copilot-pull-request-reviewer threads:
github-actions threads:
All 23 tests pass; pre-existing |
|
@copilot please run the Unresolved review threads
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…ore global.core Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All four remaining threads addressed in the latest commit:
All 23 tests pass locally. |
|
🎉 This pull request is included in a new release. Release: |
Summary
Cleaned
actions/setup/js/convert_gateway_config_shared.cjsas part of the jsweep daily unbloat rotation.Context Type
github-script (uses
coreglobal from github-script/shim)Changes Made
convert_gateway_config_shared.cjscore.error() + process.exit(1)withthrow new Error()inrequireEnvVar()andloadGatewayContext()— makes errors testable without process termination side effects/// <reference types="@actions/github-script" />for proper type declarations of thecoreglobalconvert_gateway_config_shared.test.cjs(new file)rewriteUrl(4 tests): URL rewriting, path preservation, non-mcp URLsnormalizeGatewayEntry(4 tests): mutation, immutability, no-callback, missing urlfilterAndTransformServers(3 tests): CLI server exclusion, transforms, empty resultwriteSecureOutput(4 tests): mode 0o600, nested dirs, overwrite, failureloadGatewayContext(5 tests): missing env vars, missing file, full parsing, extraRequiredEnvlogCLIFilters+logServerStats(3 tests): core.info calls, empty set noop, stats mathValidation
✅ Formatting:
npm run format:cjs✓✅ Linting:
npm run lint:cjs✓✅ Type checking:
npm run typecheck✓✅ Tests: 31/31 passed (
convert_gateway_config_shared.test.cjs+convert_gateway_config_adapters.test.cjs) ✓Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/29895667664
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
run: https://github.com/github/gh-aw/actions/runs/29898890921
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.