feat: make E003 file-count error actionable and document max-patch-files#32541
Conversation
- Make E003 error actionable: tell users the exact file count and the `max-patch-files` frontmatter field they can set to raise it - Document `max-patch-files` and `max-patch-size` options in safe-outputs-pull-requests.md with usage examples - Add `max-patch-files` and `max-patch-size` to the config YAML snippet - Update tests to assert the new actionable hint is present in E003 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
When the patch exceeds `max-patch-files` and `fallback-as-issue` is enabled (the default), create a fallback issue that surfaces the E003 error and the actionable `max-patch-files` fix — instead of silently returning a bare failure result that only appears in step logs. When `fallback-as-issue: false`, behaviour is unchanged (return error). Add three integration tests covering: - E003 + fallback_as_issue=true → fallback issue created - E003 + fallback_as_issue=false → error returned, no issue - max_patch_files raised above count → PR created normally Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 031a69e. When E003 fires and |
| // issue/comment thread and the workflow operator knows exactly how to fix it. | ||
| const fallbackTitle = pullRequestItem.title?.trim() || "Agent Output"; | ||
| const fallbackLabels = mergeFallbackIssueLabels(configFallbackLabels.length > 0 ? configFallbackLabels : envLabels); | ||
| const fallbackBody = [ |
There was a problem hiding this comment.
@copilot refactor text to template file in actions/setup/md
There was a problem hiding this comment.
Done in the latest commits. Extracted the E003 fallback body into actions/setup/md/e003_file_limit_fallback.md with {error_message} and {suggested_limit} placeholders. The code now calls renderTemplateFromFile using getPromptPath("e003_file_limit_fallback.md"), matching the pattern used for other fallback messages. The E003 test was updated to copy the template into a temp prompts directory and set GH_AW_PROMPTS_DIR accordingly.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR makes the create-pull-request file-count limit more actionable and documents the related configuration knobs.
Changes:
- Updates E003 to include the received file count and
max-patch-filesguidance. - Adds a fallback issue template for file-limit failures.
- Documents
max-patch-filesandmax-patch-size, and adds tests for the E003/fallback behavior.
Show a summary per file
| File | Description |
|---|---|
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Documents patch file-count and patch-size configuration. |
actions/setup/md/e003_file_limit_fallback.md |
Adds the fallback issue body template for E003 failures. |
actions/setup/js/create_pull_request.test.cjs |
Adds assertions and fallback behavior tests for E003. |
actions/setup/js/create_pull_request.cjs |
Updates E003 messaging and adds fallback issue creation on file-limit failures. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
| if (!fallbackAsIssue) { | ||
| return { success: false, error: errorMessage }; | ||
| } | ||
|
|
| const fallbackBody = renderTemplateFromFile(fallbackTemplatePath, { | ||
| error_message: errorMessage, | ||
| suggested_limit: maxFiles * 2, | ||
| }); |
|
|
||
| // Surface the limit error in a fallback issue so it appears in the agent failure | ||
| // issue/comment thread and the workflow operator knows exactly how to fix it. | ||
| const fallbackTitle = pullRequestItem.title?.trim() || "Agent Output"; |
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification DetailsView per-test classification
Analysis NotesAll 3 new tests are in the
|
![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 for this feature addition — the PR introduces new fallback behaviour and has meaningful test coverage to evaluate.
Key Themes
- Suggested limit inconsistency: the inline error message hints
max-patch-files: <exact-count>, but the fallback issue template usesmaxFiles * 2— two different values for the same setting in the same failure event (see inline comment on line 1101). - Missing error-path test: the
catch (issueError)branch (fallback issue creation fails) is untested, leaving a silent regression risk (see inline comment on line 3168).
Positive Highlights
- ✅ Great UX improvement — the new error message is immediately actionable and tells the operator exactly what field to set.
- ✅ Three distinct scenarios tested (fallback enabled, fallback disabled, limit raised) — good behavioural coverage for the happy paths.
- ✅ Template-driven fallback issue body is a clean separation of concerns.
- ✅ Documentation additions are clear and include worked examples, which matches the project's documentation style.
Verdict
Two small issues worth addressing before merge: align the suggested limit value across both surfaces, and add a test for the fallback-issue-creation-failure path. Neither is blocking, but fixing them keeps the error story internally consistent and closes the one untested branch.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 8.3M
| const fallbackTemplatePath = getPromptPath("e003_file_limit_fallback.md"); | ||
| const fallbackBody = renderTemplateFromFile(fallbackTemplatePath, { | ||
| error_message: errorMessage, | ||
| suggested_limit: maxFiles * 2, |
There was a problem hiding this comment.
[/tdd] The suggested_limit passed to the fallback template is maxFiles * 2 (e.g. 200 when the default 100-file limit is in effect), but the actionable error message in enforcePullRequestLimits suggests exactly fileCount (e.g. 101). A user who reads the fallback issue body will see "set max-patch-files: 200" while the inline error says "set max-patch-files: 101" — two different values for the same setting in the same failure event.
Consider aligning both surfaces to use fileCount:
suggested_limit: fileCount, // match the error message hintIf a deliberate buffer is intended, documenting the reasoning would help avoid future confusion.
| expect(global.github.rest.pulls.create).toHaveBeenCalledTimes(1); | ||
| expect(global.github.rest.issues.create).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The happy-path and fallback_as_issue: false cases are well-covered, but the catch (issueError) branch in the new fallback block has no test. If github.rest.issues.create rejects, the combined error should be returned as { success: false, error: '...' } and core.error should be called. Without a test, a regression in that branch would be invisible.
Suggested addition:
it('should return success:false when fallback issue creation itself fails', async () => {
global.github.rest.issues.create = vi.fn().mockRejectedValue(new Error('API rate limited'));
const patchPath = path.join(tempDir, 'aw-test.patch');
fs.writeFileSync(patchPath, buildOversizedPatch(101));
const { main } = require('./create_pull_request.cjs');
const handler = await main({});
const result = await handler({ title: 'T', body: 'B', branch: 'b', patch_path: patchPath }, {});
expect(result.success).toBe(false);
expect(result.error).toContain('E003');
expect(result.error).toContain('API rate limited');
expect(global.core.error).toHaveBeenCalledTimes(1);
});
|
@copilot review all comments |
…llback Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all three reviewer comments:
Also added regression assertions ( |
The hard-coded 100-file cap in
create_pull_requestsilently blocks generated-content workflows (e.g. daily API-reference regeneration) with no recovery path — the error message gave no hint that the limit is configurable.Changes
Actionable error message — E003 now tells the user the exact count received and the exact field to set:
Documentation (
safe-outputs-pull-requests.md) —max-patch-filesandmax-patch-sizewere undocumented; added both to the config YAML example and added a prose section with a worked example for generated-content workflows:Test — added assertion that the E003 message always contains
"max-patch-files"to prevent regression.