Prevent Claude tool results from triggering failure classifiers - #60349
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the stated classifier scope and retry behavior with focused regression coverage.
Pull request overview
Scopes Claude failure classification to trusted output while ensuring signal exits retry as fresh runs.
Changes:
- Excludes stream-JSON
userevents from failure classifiers. - Prioritizes signal-termination retries over authentication and permission rules.
- Adds regression coverage for tool results and exit code 143.
File summaries
| File | Description |
|---|---|
actions/setup/js/claude_harness.cjs |
Filters classifier input and reorders signal handling. |
actions/setup/js/claude_harness.test.cjs |
Tests filtering and fresh signal retries. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Ponytail Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — this is a well-scoped fix (excluding stream-JSON user/tool-result events from failure classifiers, and prioritizing signal/crash-exit handling) with solid targeted regression tests. Two observations below, neither blocking.
📋 Key Themes & Highlights
Key Themes
- Reordering coverage gap: moving the signal/crash-exit check ahead of auth, invalid-model, and permission-denied terminal checks is tested for the auth interaction only; the invalid-model and permission-denied interactions aren't pinned down by a test.
- Intentional asymmetry:
extractDeniedCommandsdeliberately still reads rawresult.outputper the PR description, while the triggeringhasNumerousPermissionDeniedcheck now usesclassifiableOutput. This is a reasonable, documented choice, but worth a quick look since the extracted commands feed amissing_toolissue payload.
Positive Highlights
- ✅
classifiableOutputis small, well-commented, and defensively handles malformed JSON lines. - ✅ Good regression tests added for the exact bug scenario (tool-result text like "not logged in"/EACCES leaking into classifiers).
- ✅ Clear PR description mapping scope, prioritization, and coverage — easy to review.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 135.4 AIC · ⌖ 15 AIC · ⊞ 10.4K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/claude_harness.cjs:585
[/tdd] The signal/crash-exit short-circuit was moved ahead of the auth, invalid-model, and permission-denied terminal checks, but only the auth-failure interaction is regression-tested (uses a fresh retry after signal-style termination even when output is classified as an auth failure). The reordering changes real control flow for other classifiers too.
<details>
<summary>💡 Suggested regression tests</summary>
Add cases mirroring the existing auth test but for:
- `isInvalidModel === tr…
actions/setup/js/claude_harness.cjs:610
[/diagnosing-bugs] extractDeniedCommands still scans raw result.output (including untrusted tool-result content) even though the trigger condition hasNumerousPermissionDenied now uses classifiableOutput. The PR description calls this out as intentional ("continue using full output for...denied-command extraction"), and it's control-flow-safe since the retry/stop decision itself is driven by the filtered count. But the extracted denied_commands list feeds directly into the `missing…
Claude stream-JSON includes tool results as
userevents, allowing benign repository content such as “not logged in” orEACCESto suppress retries or misreport failures.Scope failure classification
userevents from auth, rate-limit, overload, permission-denied, and harness-guard classifiers.Prioritize process termination
--continuedisabled.Cover regressions
143retries even when classifiable authentication text is present.