Ensure Copilot proxy-auth template resolves for PR Sous Chef#40623
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey
If you'd like a hand completing the remaining work, here's a prompt you can assign to your coding agent:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot remove fallback and ensure that the template file exists and is properly resolved. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40623 does not have the 'implementation' label and has 0 new lines of code (≤100) in business logic directories. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Copilot harness’s gh-aw API proxy HTTP 403 + S2STOKENS diagnostic to load the checked-in copilot_requests_proxy_auth_403.md via the standard runtime prompt lookup (getPromptPath(...)) rather than relying on an inline fallback, and adds regression tests to validate prompt resolution via both GH_AW_PROMPTS_DIR and RUNNER_TEMP.
Changes:
- Switch the proxy 403 copilot-requests diagnostic from a hardcoded on-disk path to
getPromptPath("copilot_requests_proxy_auth_403.md"). - Add focused harness tests to verify the 403 template resolves correctly from an explicit prompts dir and via the runtime
${RUNNER_TEMP}/gh-aw/promptsfallback. - Introduce test helpers to temporarily override
GH_AW_PROMPTS_DIR/RUNNER_TEMPand to stage a minimal prompts directory.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/copilot_harness.cjs | Uses getPromptPath(...) for the S2STOKENS + proxy HTTP 403 diagnostic template resolution. |
| actions/setup/js/copilot_harness.test.cjs | Adds regression coverage and helper utilities to validate prompt template resolution paths. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
| if (authFailure.statusCode === "403" && envFlagEnabled(env.S2STOKENS)) { | ||
| return renderTemplateFromFile(COPILOT_REQUESTS_PROXY_AUTH_403_TEMPLATE_PATH, { | ||
| const render = options.renderTemplateFromFile || renderTemplateFromFile; | ||
| return render(getPromptPath(COPILOT_REQUESTS_PROXY_AUTH_403_TEMPLATE_NAME), { | ||
| selected_model: selectedModel, | ||
| stage, |
| }); | ||
|
|
||
| describe("gh-aw API proxy auth diagnostics", () => { | ||
| const promptsSourceDir = path.resolve("../md"); |
| function withRunnerTemp(runnerTempDir, callback) { | ||
| const originalRunnerTemp = process.env.RUNNER_TEMP; | ||
| process.env.RUNNER_TEMP = runnerTempDir; | ||
| try { | ||
| return callback(); | ||
| } finally { | ||
| if (typeof originalRunnerTemp === "string") { | ||
| process.env.RUNNER_TEMP = originalRunnerTemp; | ||
| } else { | ||
| delete process.env.RUNNER_TEMP; | ||
| } | ||
| } | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Go: 0; JavaScript: 4 ( Note on inflation: The 16.7:1 line-addition ratio is partly explained by the production file being simplified (−25 lines removed, +7 added) while the test file added three reusable environment-isolation helpers ( Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 4 tests verify observable behavioral contracts: diagnostic output content, template path resolution from GH_AW_PROMPTS_DIR, and fallback resolution from RUNNER_TEMP.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with suggestions. The fix correctly removes the compile-time __dirname path and routes through getPromptPath(). The test additions directly validate both runtime lookup branches.
📋 Key Themes & Highlights
Key Themes
- Correctness gap:
getPromptPath()now throws when neitherGH_AW_PROMPTS_DIRnorRUNNER_TEMPis set — the old constant never threw. Worth adding a try/catch fallback for defensive robustness. - Test fragility:
promptsSourceDir = path.resolve("../md")is CWD-relative;path.resolve(__dirname, "../md")is safer. - Reusability:
withTemporaryPromptTemplatehardcodes the template filename; a small generalisation would let future tests reuse it. - Mock over-specification: the
vi.fnmock reimplements the template engine unnecessarily — a simple sentinel return would keep the test focused on path-resolution.
Positive Highlights
- ✅ Root cause correctly identified and addressed — compile-time path → runtime
getPromptPath()lookup. - ✅
options.renderTemplateFromFileinjection is clean DI — no test-framework magic required. - ✅ Two new tests cover both runtime lookup branches (
GH_AW_PROMPTS_DIRandRUNNER_TEMP/gh-aw/prompts) with isolated temp dirs and proper cleanup. - ✅
withTestPromptsDir/withRunnerTemphelpers are clean bracket-style env fixtures with reliable restore-on-throw behaviour. - ✅ Net production delta is tiny (6 additions, 5 deletions), regression test surface is proportionally large (+121/-18).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 85.4 AIC · ⌖ 9.29 AIC · ⊞ 6.9K
| if (authFailure.statusCode === "403" && envFlagEnabled(env.S2STOKENS)) { | ||
| return renderTemplateFromFile(COPILOT_REQUESTS_PROXY_AUTH_403_TEMPLATE_PATH, { | ||
| const render = options.renderTemplateFromFile || renderTemplateFromFile; | ||
| return render(getPromptPath(COPILOT_REQUESTS_PROXY_AUTH_403_TEMPLATE_NAME), { |
There was a problem hiding this comment.
[/diagnose] getPromptPath() throws if neither GH_AW_PROMPTS_DIR nor RUNNER_TEMP is set — the old hardcoded __dirname constant never threw. In normal GitHub Actions runs RUNNER_TEMP is always present, but if either variable is missing for any reason the harness will crash here instead of returning a graceful empty diagnostic.
💡 Suggested guard
let templatePath;
try {
templatePath = getPromptPath(COPILOT_REQUESTS_PROXY_AUTH_403_TEMPLATE_NAME);
} catch {
// RUNNER_TEMP not set — skip the 403 template diagnostic
return "";
}
return render(templatePath, { selected_model: selectedModel, stage });Alternatively, define a safeGetPromptPath helper that returns null on failure so the guard is reusable across branches.
| }); | ||
|
|
||
| describe("gh-aw API proxy auth diagnostics", () => { | ||
| const promptsSourceDir = path.resolve("../md"); |
There was a problem hiding this comment.
[/tdd] path.resolve("../md") is CWD-relative — it resolves correctly when vitest runs from actions/setup/js/ (the package.json root), but breaks if tests are ever invoked from a different working directory. Anchoring to __dirname is more robust.
💡 Suggested fix
const promptsSourceDir = path.resolve(__dirname, "../md");__dirname is always the directory of this .cjs file (actions/setup/js/), so the path resolves correctly regardless of where vitest is invoked.
| try { | ||
| const promptsDir = promptDirResolver(tempDir); | ||
| fs.mkdirSync(promptsDir, { recursive: true }); | ||
| fs.copyFileSync(path.join(sourceTemplateDir, "copilot_requests_proxy_auth_403.md"), path.join(promptsDir, "copilot_requests_proxy_auth_403.md")); |
There was a problem hiding this comment.
[/tdd] withTemporaryPromptTemplate hardcodes copilot_requests_proxy_auth_403.md, making this utility unusable for future template tests. A templateName parameter would cost nothing and allow reuse.
💡 Suggested signature
function withTemporaryPromptTemplate(prefix, sourceTemplateDir, templateName, promptDirResolver, callback) {
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), prefix));
try {
const promptsDir = promptDirResolver(tempDir);
fs.mkdirSync(promptsDir, { recursive: true });
fs.copyFileSync(
path.join(sourceTemplateDir, templateName),
path.join(promptsDir, templateName)
);
return callback(tempDir, promptsDir);
} finally {
fs.rmSync(tempDir, { recursive: true, force: true });
}
}Call sites would pass "copilot_requests_proxy_auth_403.md" as the third argument.
| tempDir => tempDir, | ||
| (_tempDir, runtimePromptsDir) => { | ||
| withTestPromptsDir(runtimePromptsDir, () => { | ||
| const renderTemplateFromFile = vi.fn((templatePath, context) => { |
There was a problem hiding this comment.
[/tdd] The vi.fn mock reimplements a simplified template engine using .replace("{selected_model}", ...). Since renderTemplate does use {key} syntax this happens to be correct, but it creates an implicit contract between the mock and the real implementation. The key assertion here is toHaveBeenCalledWith (verifying path resolution) — the content checks are already covered by the non-mock 403 tests above.
💡 Simpler alternative
Return a predictable sentinel string so the mock remains honest about its scope:
const renderTemplateFromFile = vi.fn(() => "mock-render-result");
// ...
expect(renderTemplateFromFile).toHaveBeenCalledWith(
path.join(runtimePromptsDir, "copilot_requests_proxy_auth_403.md"),
{ selected_model: "claude-sonnet-4.5", stage: "starting the Copilot CLI request" }
);
// content correctness is already asserted in the earlier non-mock 403 tests
|
@copilot |
PR Sous Chef was completing its PR nudging work but still failing because the Copilot harness could not resolve the
copilot_requests_proxy_auth_403.mddiagnostic template at runtime. This change makes the 403 diagnostic path use the checked-in template via the standard runtime prompt lookup so scheduled runs keep the existing billing guidance without relying on an inline fallback.Copilot harness template resolution
copilot_requests_proxy_auth_403.mddiagnostic throughgetPromptPath(...)Failure-path hardening
S2STOKENS+ proxyHTTP 403diagnostic branchFocused regression coverage
GH_AW_PROMPTS_DIRandRUNNER_TEMP