-
Notifications
You must be signed in to change notification settings - Fork 444
fix(harness): exit 0 when expected safe-outputs already produced despite numerous permission-denied #41675
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(harness): exit 0 when expected safe-outputs already produced despite numerous permission-denied #41675
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ const { | |
| extractDeniedCommands, | ||
| hasNumerousPermissionDeniedIssues, | ||
| hasNoopInSafeOutputs, | ||
| hasExpectedSafeOutputs, | ||
| INFERENCE_ACCESS_ERROR_PATTERN, | ||
| AGENTIC_ENGINE_TIMEOUT_PATTERN, | ||
| isDetectionPhase, | ||
|
|
@@ -1906,4 +1907,69 @@ process.exit(1);`, | |
| expect(result.stderr).toContain("noop message found in safe-outputs — not retrying"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("permission-denied suppression when expected safe-outputs already produced", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Integration coverage exists only for 💡 SuggestionAdd parallel integration tests in each of those files (same stub pattern as lines 1912–1945 here): describe("permission-denied suppression when expected safe-outputs already produced", () => {
it("exits 0 and suppresses verdict when safe-output already written", () => { ... });
it("exits 1 when no expected safe-outputs present", () => { ... });
});All three harnesses use identical guard logic, so divergence from a future refactor or merge conflict would only be caught in the copilot harness today. |
||
| it("exits 0 and suppresses terminal verdict when numerous permission-denied occurs after expected safe-output was written", () => { | ||
| const tempDir = makeHarnessTempDir("copilot-perm-denied-suppression-"); | ||
| const safeOutputsPath = path.join(tempDir, "safe-outputs.jsonl"); | ||
| const stubPath = path.join(tempDir, "stub.cjs"); | ||
| const promptPath = path.join(tempDir, "prompt.txt"); | ||
| const callsPath = path.join(tempDir, "calls.jsonl"); | ||
| // Stub writes an expected safe-output then fails with numerous permission-denied output. | ||
| fs.writeFileSync( | ||
| stubPath, | ||
| `const fs = require("fs"); | ||
| const callsPath = process.env.COPILOT_HARNESS_STUB_CALLS; | ||
| const safeOutputsPath = process.env.GH_AW_SAFE_OUTPUTS; | ||
| fs.appendFileSync(callsPath, JSON.stringify({args: process.argv.slice(2)}) + "\\n"); | ||
| fs.appendFileSync(safeOutputsPath, JSON.stringify({type:"add_comment",body:"Report posted"}) + "\\n"); | ||
| process.stdout.write("permission denied\\npermission denied\\npermission denied\\nEACCES: permission denied\\nEPERM operation not permitted\\n"); | ||
| process.exit(1);`, | ||
| "utf8" | ||
| ); | ||
| fs.writeFileSync(promptPath, "fix the bug", "utf8"); | ||
|
|
||
| const result = spawnSync(process.execPath, ["copilot_harness.cjs", process.execPath, stubPath, "--prompt-file", promptPath], { | ||
| cwd: path.dirname(require.resolve("./copilot_harness.cjs")), | ||
| env: { ...process.env, COPILOT_HARNESS_STUB_CALLS: callsPath, GH_AW_SAFE_OUTPUTS: safeOutputsPath }, | ||
| encoding: "utf8", | ||
| timeout: 15000, | ||
| }); | ||
| const callCount = fs.readFileSync(callsPath, "utf8").trim().split("\n").filter(Boolean).length; | ||
| // Only one attempt — no retries when permission-denied is suppressed | ||
| expect(callCount).toBe(1); | ||
| // Harness exits 0 because the core work (add_comment) already succeeded | ||
| expect(result.status).toBe(0); | ||
| expect(result.stderr).toContain("suppressing terminal verdict (false-red: core work succeeded)"); | ||
| }); | ||
|
|
||
| it("exits 1 and emits missing_tool when numerous permission-denied occurs with no expected safe-outputs", () => { | ||
| const tempDir = makeHarnessTempDir("copilot-perm-denied-no-outputs-"); | ||
| const safeOutputsPath = path.join(tempDir, "safe-outputs.jsonl"); | ||
| const stubPath = path.join(tempDir, "stub.cjs"); | ||
| const promptPath = path.join(tempDir, "prompt.txt"); | ||
| const callsPath = path.join(tempDir, "calls.jsonl"); | ||
| // Stub fails with numerous permission-denied but writes no expected safe-output. | ||
| fs.writeFileSync( | ||
| stubPath, | ||
| `const fs = require("fs"); | ||
| const callsPath = process.env.COPILOT_HARNESS_STUB_CALLS; | ||
| fs.appendFileSync(callsPath, JSON.stringify({args: process.argv.slice(2)}) + "\\n"); | ||
| process.stdout.write("permission denied\\npermission denied\\npermission denied\\nEACCES: permission denied\\nEPERM operation not permitted\\n"); | ||
| process.exit(1);`, | ||
| "utf8" | ||
| ); | ||
| fs.writeFileSync(promptPath, "fix the bug", "utf8"); | ||
|
|
||
| const result = spawnSync(process.execPath, ["copilot_harness.cjs", process.execPath, stubPath, "--prompt-file", promptPath], { | ||
| cwd: path.dirname(require.resolve("./copilot_harness.cjs")), | ||
| env: { ...process.env, COPILOT_HARNESS_STUB_CALLS: callsPath, GH_AW_SAFE_OUTPUTS: safeOutputsPath, GH_AW_SAFEOUTPUTS_CLI: "true" }, | ||
| encoding: "utf8", | ||
| timeout: 15000, | ||
| }); | ||
| // Harness exits 1 because no expected output was produced | ||
| expect(result.status).toBe(1); | ||
| expect(result.stderr).toContain("detected numerous permission-denied issues — not retrying"); | ||
|
Comment on lines
+1964
to
+1972
|
||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,6 +152,58 @@ function emitInfrastructureIncomplete(details, options) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Diagnostic safe-output types that represent infrastructure signals, not task-level work. | ||
| * Entries with these types are excluded when checking whether expected outputs were produced. | ||
| */ | ||
| const DIAGNOSTIC_SAFE_OUTPUT_TYPES = new Set(["noop", "missing_tool", "report_incomplete"]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/zoom-out] 💡 SuggestionExport the constant so external code can enumerate diagnostic types without hardcoding the same strings: // safeoutputs_cli.cjs
module.exports = {
// ...
DIAGNOSTIC_SAFE_OUTPUT_TYPES,
hasExpectedSafeOutputs,
};Tests can then import it to drive parametric cases (e.g.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Negative allowlist will silently misclassify any new diagnostic safe-output type added in the future, causing false-green suppression when no real task work was done. 💡 Details
A secondary robustness gap: Option A — minimal fix (typeof guard): if (parsed && typeof parsed.type === "string" && !DIAGNOSTIC_SAFE_OUTPUT_TYPES.has(parsed.type)) {Option B — flip to a positive allowlist (explicit, but must be updated as new tools are added): const REAL_SAFE_OUTPUT_TYPES = new Set([
"add_comment", "create_pull_request", "submit_pull_request_review",
"create_pull_request_review_comment", "create_check_run",
// extend here when new task-output tools are added
]);
if (parsed && typeof parsed.type === "string" && REAL_SAFE_OUTPUT_TYPES.has(parsed.type)) {Option A is the minimal safe fix regardless of which approach is preferred long-term. |
||
|
|
||
| /** | ||
| * Read the safe-outputs JSONL file and check whether it contains at least one | ||
| * non-diagnostic entry (i.e. an output that represents real, task-level work). | ||
| * Returns true when such an entry exists; false otherwise. | ||
| * Used by harnesses to suppress the terminal "numerous permission-denied" verdict | ||
| * when the agent already produced the expected output — preventing false-red runs. | ||
|
Comment on lines
+161
to
+166
|
||
| * @param {string} safeOutputsPath - Path to the safe-outputs JSONL file | ||
| * @param {{ | ||
| * logger?: (msg: string) => void, | ||
| * readFileSync?: (path: string, encoding: BufferEncoding) => string | ||
| * }=} options | ||
| * @returns {boolean} | ||
| */ | ||
| function hasExpectedSafeOutputs(safeOutputsPath, options) { | ||
| const logger = options && options.logger ? options.logger : defaultLog; | ||
| const readFile = options && options.readFileSync ? options.readFileSync : fs.readFileSync; | ||
|
|
||
| if (!safeOutputsPath) { | ||
| return false; | ||
| } | ||
|
|
||
| let content; | ||
| try { | ||
| content = readFile(safeOutputsPath, "utf8"); | ||
| } catch { | ||
| // File does not exist or is not readable — no expected entries present | ||
| return false; | ||
| } | ||
|
|
||
| const lines = content.split("\n"); | ||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) continue; | ||
| try { | ||
| const parsed = JSON.parse(trimmed); | ||
| if (parsed && parsed.type && !DIAGNOSTIC_SAFE_OUTPUT_TYPES.has(parsed.type)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnose] The heuristic "any single non-diagnostic entry = core work succeeded" is correct for the primary false-red scenario, but it also fires when an agent produced only a partial set of required outputs before hitting permission denials. 💡 ContextFor example, an agent that wrote This is a known trade-off (the PR description acknowledges "at least one"), but it's worth documenting explicitly in the JSDoc: /**
* ...
* Note: returns true on the *first* non-diagnostic entry found — it does not
* verify that all required outputs for the task were written. A partial output
* set will still suppress the permission-denied verdict.
*/No code change needed; just a documentation nudge so future readers understand the intentional looseness of this check. |
||
| logger(`hasExpectedSafeOutputs: non-diagnostic entry found in ${safeOutputsPath}: type=${parsed.type}`); | ||
| return true; | ||
| } | ||
| } catch { | ||
| // Skip malformed lines — they do not represent valid entries | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Read the safe-outputs JSONL file and check whether any noop entry has been written. | ||
| * Returns true when at least one {"type":"noop"} line is present; false otherwise. | ||
|
|
@@ -211,6 +263,7 @@ if (typeof module !== "undefined" && module.exports) { | |
| buildMissingToolAlternatives, | ||
| emitMissingToolPermissionIssue, | ||
| emitInfrastructureIncomplete, | ||
| hasExpectedSafeOutputs, | ||
| hasNoopInSafeOutputs, | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ import os from "os"; | |
| import path from "path"; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
| const { runSafeOutputsCLI, buildMissingToolAlternatives, emitMissingToolPermissionIssue, emitInfrastructureIncomplete, hasNoopInSafeOutputs } = require("./safeoutputs_cli.cjs"); | ||
| const { runSafeOutputsCLI, buildMissingToolAlternatives, emitMissingToolPermissionIssue, emitInfrastructureIncomplete, hasExpectedSafeOutputs, hasNoopInSafeOutputs } = require("./safeoutputs_cli.cjs"); | ||
|
|
||
| describe("safeoutputs_cli.cjs", () => { | ||
| describe("runSafeOutputsCLI", () => { | ||
|
|
@@ -155,6 +155,138 @@ describe("safeoutputs_cli.cjs", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("hasExpectedSafeOutputs", () => { | ||
| function makeTempFile(content) { | ||
| const p = path.join(os.tmpdir(), `safeoutputs-expected-test-${Date.now()}-${Math.random().toString(36).slice(2)}.jsonl`); | ||
| fs.writeFileSync(p, content, "utf8"); | ||
| return p; | ||
| } | ||
|
|
||
| it("returns true when the file contains a non-diagnostic entry", () => { | ||
| const filePath = makeTempFile('{"type":"add_comment","body":"done"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(true); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("returns true for a submit_pull_request_review entry", () => { | ||
| const filePath = makeTempFile('{"type":"submit_pull_request_review","event":"APPROVE"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(true); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("returns false when all entries are diagnostic types", () => { | ||
| const filePath = makeTempFile('{"type":"noop","message":"nothing to do"}\n{"type":"missing_tool","tool":"x"}\n{"type":"report_incomplete","reason":"infra"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(false); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("returns false when the file contains only a noop entry", () => { | ||
| const filePath = makeTempFile('{"type":"noop","message":"nothing to do"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(false); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("returns false when the file contains only a missing_tool entry", () => { | ||
| const filePath = makeTempFile('{"type":"missing_tool","tool":"tool/permission"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(false); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("returns false when the file contains only a report_incomplete entry", () => { | ||
| const filePath = makeTempFile('{"type":"report_incomplete","reason":"infrastructure_error"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(false); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("returns true when an expected entry is mixed with diagnostic entries", () => { | ||
| const filePath = makeTempFile('{"type":"missing_tool","tool":"x"}\n{"type":"add_comment","body":"done"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(true); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("returns false when the file does not exist", () => { | ||
| expect(hasExpectedSafeOutputs("/tmp/nonexistent-safe-outputs-expected.jsonl")).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when safeOutputsPath is empty", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test covers the empty-string path but not 💡 Suggestionit("returns false when safeOutputsPath is null", () => {
expect(hasExpectedSafeOutputs(null)).toBe(false);
});
it("returns false when safeOutputsPath is undefined", () => {
expect(hasExpectedSafeOutputs(undefined)).toBe(false);
});The internal |
||
| expect(hasExpectedSafeOutputs("")).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false for an empty file", () => { | ||
| const filePath = makeTempFile(""); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(false); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("skips malformed lines and returns false when no valid expected entry exists", () => { | ||
| const filePath = makeTempFile('not-json\n{"type":"noop"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(false); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("skips malformed lines and returns true when a valid non-diagnostic entry follows them", () => { | ||
| const filePath = makeTempFile('not-json\n{"type":"add_comment","body":"done"}\n'); | ||
| try { | ||
| expect(hasExpectedSafeOutputs(filePath)).toBe(true); | ||
| } finally { | ||
| fs.rmSync(filePath); | ||
| } | ||
| }); | ||
|
|
||
| it("uses injected readFileSync for testability", () => { | ||
| const logs = []; | ||
| const result = hasExpectedSafeOutputs("/fake/path.jsonl", { | ||
| readFileSync: () => '{"type":"update_pull_request","title":"fix"}\n', | ||
| logger: m => logs.push(m), | ||
| }); | ||
| expect(result).toBe(true); | ||
| expect(logs.some(m => m.includes("non-diagnostic entry found"))).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false when injected readFileSync returns only diagnostic entries", () => { | ||
| const result = hasExpectedSafeOutputs("/fake/path.jsonl", { | ||
| readFileSync: () => '{"type":"noop","message":"nothing"}\n', | ||
| }); | ||
| expect(result).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when injected readFileSync throws", () => { | ||
| const result = hasExpectedSafeOutputs("/fake/path.jsonl", { | ||
| readFileSync: () => { | ||
| throw new Error("ENOENT"); | ||
| }, | ||
| }); | ||
| expect(result).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("hasNoopInSafeOutputs", () => { | ||
| function makeTempFile(content) { | ||
| const p = path.join(os.tmpdir(), `safeoutputs-noop-test-${Date.now()}-${Math.random().toString(36).slice(2)}.jsonl`); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suppression path added here (and in
claude_harness.cjs) has no integration tests, so a regression in either harness would ship silently.💡 Details
copilot_harness.test.cjsgrows two integration tests that validate the suppression and the non-suppression paths end-to-end. The identical guard block was copy-pasted intocodex_harness.cjsandclaude_harness.cjs, but neither harness has a corresponding test. The unit tests insafeoutputs_cli.test.cjscoverhasExpectedSafeOutputsin isolation but do not exercise the harness integration glue (thelastExitCode = 0; break;path, thesafeOutputsPath &&guard, the log message). A typo, wrong variable, or subtle difference in how the other harnesses handlelastExitCodeafter the loop would go undetected.The quickest fix is to add the same two describe-block tests to the test suites for
codex_harnessandclaude_harness, or factor them into a shared helper both suites call.