Improve tool-denial failure report formatting for last denied request

[Copilot]

The agent failure report now treats the last denied request as expandable detail instead of always-visible inline text. It also renders the request in a fenced code region and removes inline `shell(...)` wrapping for cleaner, more readable output.

• Context template update (tool_denials_exceeded_context.md)

  • Replaced always-visible Last denied request text with a <details> disclosure block.
  • Moved request content into a fenced text block.
  • Updated the remediation example to show the request on its own line (not inline).

• Runtime fallback parity (handle_agent_failure.cjs)

  • Updated fallback rendering (when template load fails) to the same structure:
    • <details> / <summary>
    • fenced text code block
  • Stopped injecting inline backticks around normalized denied request content.

• Behavioral coverage (handle_agent_failure.test.cjs)

  • Updated assertions to verify disclosure + code-block output shape.
  • Added assertion to ensure the normalized request is not rendered as inline `shell(...)` text.

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

```text
shell(python3 ...)

```

[pelikhan]

@copilot try/catch not needed, template rendering should not fail.

[Copilot]

Updated in 08e48d9: removed the try/catch from buildToolDenialsExceededContext so template rendering errors are no longer swallowed.

[Copilot]

Pull request overview

This PR improves the readability of the “tool denials exceeded” agent-failure report by moving the last denied request into an expandable disclosure section and rendering the request as a fenced text code block (instead of inline formatting), with matching assertion updates in tests.

Changes:

• Updated tool_denials_exceeded_context.md to render “Last denied request” inside a <details> block with a fenced text code region.
• Updated buildToolDenialsExceededContext to pass the normalized request string into the template without adding inline backticks.
• Updated unit tests to assert the new disclosure + code-fence output shape and to ensure no inline `shell(...)` wrapping appears.

Show a summary per file

File	Description
actions/setup/md/tool_denials_exceeded_context.md	Switches the last denied request rendering to an expandable <details> block with a fenced text snippet.
actions/setup/js/handle_agent_failure.cjs	Adjusts tool-denials-exceeded context rendering to avoid inline backtick wrapping around the normalized request.
actions/setup/js/handle_agent_failure.test.cjs	Updates assertions to verify the new details + fenced output and absence of inline `shell(...)` formatting.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 3/3 changed files
• Comments generated: 1

[github-actions]

🔍 Design Decision Gate 🏗️ is checking for design decision records on this pull request...

[github-actions]

🔬 Test Quality Sentinel is analyzing test quality on this pull request...

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills...

[github-actions]

REQUEST_CHANGES — one blocking issue + two non-blocking observations

The template rendering refactor and <details> collapsible block are clean improvements. The tests for the new output structure pass. One issue must be addressed before merging.

### Blocking: fallback removal silently kills the failure report on template error

The removed try/catch was not cosmetic — it was the resilience layer that ensured buildToolDenialsExceededContext never propagated an exception to its caller. The caller sits inside an outer try (line 2625) whose catch at line 3076 does core.warning(...) and returns, aborting the entire failure issue/comment creation. A missing tool_denials_exceeded_context.md (misconfigured env, first-boot race, etc.) now silently suppresses every guard.tool_denials_exceeded failure report — users see nothing. The previous behaviour was: degraded but complete failure report. See the inline comment for a suggested fix.

### Non-blocking: fence-break risk in template + missing error-path test

• Fence break (tool_denials_exceeded_context.md line 9): renderTemplate substitutes {reason} raw into the ```text block. Commands hitting the fallthrough path in normalizeDeniedPermissionCommand (anything other than read(...) or a shell heredoc) are returned verbatim; if such a command contains ``` the fence closes early. Low probability today but fragile as new tool types are added.
• Missing error-path test: the behavioral contract of this function after the fallback removal (does it throw or return?) is untested. Either add a test asserting the new throw behaviour, or restore the fallback and add a test asserting graceful degradation.

🔎 Code quality review by PR Code Quality Reviewer · 5.55 AIC · ⌖ 13.3 AIC

[github-actions]

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.

[github-actions]

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.

[github-actions]

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.

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Ran the pr-finisher pass. No additional code changes were needed for the @copilot feedback; the try/catch removal is already in 08e48d9. Local validation (make fmt, make lint, make test-unit, make test, make recompile) passed.

Done