Skip to content

Improve tool-denial failure report formatting for last denied request #38101

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.

Sign up for GitHub

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

Merged
pelikhan merged 2 commits into from
Jun 9, 2026
+25 −21
Merged

Improve tool-denial failure report formatting for last denied request #38101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec9440c
Improve last denied request rendering in failure report
Copilot Jun 9, 2026
08e48d9
Remove unnecessary template-rendering try/catch
Copilot Jun 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 11 additions & 19 deletions actions/setup/js/handle_agent_failure.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,25 +1253,17 @@ function buildToolDenialsExceededContext(events, workflowId) {
// collapsed to a single-line summary so the issue body renders cleanly.
const normalizedReason = normalizeDeniedPermissionCommand(reason);

try {
const templatePath = getPromptPath("tool_denials_exceeded_context.md");
const template = fs.readFileSync(templatePath, "utf8");
return (
"\n" +
renderTemplate(template, {
denial_count: denialCount,
threshold,
reason: `\`${normalizedReason}\``,
workflow_id: workflowId || "the workflow",
})
);
} catch {
return (
buildWarningAlertLine("Excessive Tool Denials", `The Copilot SDK stopped the session after ${denialCount}/${threshold} permission denials.`) +
`**Last denied request:** \`${normalizedReason}\`\n\n` +
"This is a guardrail stop (`guard.tool_denials_exceeded`) and indicates the workflow's allowed tool set does not match the prompt's requested actions.\n"
);
}
const templatePath = getPromptPath("tool_denials_exceeded_context.md");
const template = fs.readFileSync(templatePath, "utf8");

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the fallback silently kills the entire failure report when this template is missing.

The old try/catch ensured that if tool_denials_exceeded_context.md was unreadable, buildToolDenialsExceededContext still returned a useful inline string and the caller could proceed to create the failure issue/comment. Now, if this template is absent or getPromptPath throws (e.g. neither GH_AW_PROMPTS_DIR nor RUNNER_TEMP is set), the exception propagates out of this helper and is caught by the outer try/catch at line 2625 — which only emits a core.warning and returns, so no failure issue or comment is created at all.

This is strictly worse than the previous behaviour: a single missing optional template now suppresses the entire guard.tool_denials_exceeded failure report, leaving users with no feedback.

💡 Suggested fix 
  let templateResult;
  try {
    const templatePath = getPromptPath("tool_denials_exceeded_context.md");
    const template = fs.readFileSync(templatePath, "utf8");
    templateResult = "\n" + renderTemplate(template, {
      denial_count: denialCount,
      threshold,
      reason: normalizedReason,
      workflow_id: workflowId || "the workflow",
    });
  } catch {
    templateResult =
      buildWarningAlertLine("Excessive Tool Denials",
        `The Copilot SDK stopped the session after ${denialCount}/${threshold} permission denials.`) +
      `**Last denied request:** \`${normalizedReason}\`\n\n` +
      "This is a guardrail stop (`guard.tool_denials_exceeded`) and indicates the workflow's allowed tool set does not match the prompt's requested actions.\n";
  }
  return templateResult;

The main issue/comment templates (agent_failure_comment.md, agent_failure_issue.md) are read inside the same outer try and have no local fallback — if those fail the report is unavoidably lost. But this helper was specifically architected to degrade gracefully; restoring that property is cheap and meaningfully improves resilience.

return (
"\n" +
renderTemplate(template, {
denial_count: denialCount,
threshold,
reason: normalizedReason,
workflow_id: workflowId || "the workflow",
})
);
Comment on lines +1256 to +1266
}

/**
Expand Down
5 changes: 5 additions & 0 deletions actions/setup/js/handle_agent_failure.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2482,6 +2482,10 @@ describe("handle_agent_failure", () => {
expect(result).toContain("guard.tool_denials_exceeded");
expect(result).toContain("daily-spdd-spec-planner");
expect(result).toContain("> [!WARNING]");
expect(result).toContain("<details>");
expect(result).toContain("<summary><strong>Last denied request</strong></summary>");
expect(result).toContain("```text");
expect(result).toContain("\nread\n");

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The primary behavioral change (fallback removal) has zero test coverage.

These new assertions verify the happy-path template rendering, but there is no test for what happens when getPromptPath or fs.readFileSync fails. That is the key semantic change in this patch: previously the function returned a hardcoded fallback string on any template error; now it throws. A missing test means the regression described in the handle_agent_failure.cjs comment can re-emerge silently.

💡 Suggested fix

Add a test that stubs the file system to throw and asserts the function still produces output rather than propagating the exception — or, if the new intent is to let it throw, document that explicitly with a test asserting the exception type so the caller contract is locked:

it("throws when the template file is missing", () => {
  // If the design intent is fail-fast, this test documents the contract.
  const tmpEnv = process.env.GH_AW_PROMPTS_DIR;
  delete process.env.GH_AW_PROMPTS_DIR;
  delete process.env.RUNNER_TEMP;
  try {
    expect(() =>
      buildToolDenialsExceededContext([{ denialCount: 1, threshold: 5, reason: "permission denied: read" }], "wf")
    ).toThrow();
  } finally {
    if (tmpEnv !== undefined) process.env.GH_AW_PROMPTS_DIR = tmpEnv;
  }
});

Without a test in either direction, this edge case is invisible to future refactors.

});

it("normalizes Python 3 heredoc reason to a single-line summary", () => {
Expand All @@ -2492,6 +2496,7 @@ describe("handle_agent_failure", () => {
const python3Reason = 'permission denied: shell(python3 << \'EOF\'\nimport re\n\nfiles = [("foo.go", "/path/foo.go")]\nfor f, p in files:\n print(f)\nEOF)';
const result = buildToolDenialsExceededContext([{ denialCount: 5, threshold: 5, reason: python3Reason }], "daily-compiler-quality");
expect(result).toContain("shell(python3 ...)");
expect(result).not.toContain("`shell(python3 ...)`");
// The full multi-line program body should not appear in the output
expect(result).not.toContain("import re");
expect(result).not.toContain("for f, p in files");
Expand Down
11 changes: 9 additions & 2 deletions actions/setup/md/tool_denials_exceeded_context.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@
> [!WARNING]
> **Excessive Tool Denials**: The Copilot SDK hit the max tool denial guardrail and stopped the session early (`{denial_count}/{threshold}`).

**Last denied request:**
<details>
<summary><strong>Last denied request</strong></summary>

```text
{reason}

@github-actions github-actions Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raw substitution of {reason} into a fenced code block: triple-backtick content in the denied command can break fence rendering.

renderTemplate does a single-pass regex replacement with no escaping. If normalizedReason contains ``` (possible for the fallthrough case in normalizeDeniedPermissionCommand — any command not matching the read(...) or heredoc patterns is returned verbatim), the fence opened on the line above closes early and the remaining reason text renders as raw markdown outside the block.

💡 Suggested fix

Sanitize the reason before substitution in the caller:

reason: normalizedReason.replace(/`{3,}/g, "``"),

Or add a dedicated escaping step in renderTemplate / document that callers must pre-escape fence characters. The case is unlikely today given normalizeDeniedPermissionCommand's normalisation rules, but the assumption is implicit and fragile if new tool types are added later.

```

</details>

This is a structured guardrail event (`guard.tool_denials_exceeded`) captured in `events.jsonl`.

Expand All @@ -16,7 +22,8 @@ Update the workflow prompt and/or permissions so required actions are permitted:

```
The workflow {workflow_id} stopped because the Copilot SDK exceeded its tool denial threshold ({denial_count}/{threshold}).
Last denied request: {reason}
Last denied request:
{reason}

Please update the workflow so the prompt only uses tools permitted by the workflow tool policy.
```
Expand Down
Loading