Repository navigation
Make PR Sous Chef comments friendlier and flag remaining CI failures - #76080
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
❌ PR Specification Reviewer failed to deliver outputs. Please review the logs for details.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
What this change does: PR Sous Chef now uses collapsed structured data for assignment comments and reports remaining CI failures after a completed task.
Should fix: The running-comment renderer is used only by tests; production emits the assignment form and Copilot writes the acknowledgement. Remove that renderer and its unused report branch, and derive the test fixture from the assignment. This leaves one production comment format to maintain.
Verdict: Ship.
Lean: -4 lines possible.
Generated by ✂️ Ponytail Reviewer for #76080 · codex · routed: gpt61 medium · ~8 AIC · ⌖ ~1 AIC · ⊞ 181
Comment /ponytail to run again
There was a problem hiding this comment.
I found one blocking correctness issue: the new agent instructions tell Copilot to put the structured envelope in the comment body while the workflow also submits it through the separate data field, causing the validator to append a second copy and breaking the parser’s single-match deduplication. The inline comment explains the failure mode and fix. Static review only; tests were not run per review instructions. The grumpy-coder specialist was unavailable because its file access was denied.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~5 AIC · ⌖ ~2 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Reviewed PR head 4d0b987298f214aa7b5e427b5f57cf0aed75f042 against Safe Outputs specification v1.29.20, especially GP6 (data) and §7.1 add_comment. The implementation and tests intentionally add presentation-only add-comment.collapse-data behavior and backward-compatible structured-data recovery; no blocking contract violation found.
Synchronized docs/src/content/docs/specs/safe-outputs-specification.md to v1.30.0 (2026-11-09), adding GP6-5 publication/parsing requirements, §7.1 rendering/configuration/security requirements, and changelog coverage. The specification commit is queued for publication to this same-repository PR branch.
Coverage: static review only; no PR code, tests, or conformance scripts were executed, per review contract. Generated lock content was inspected only for the relevant collapse_data wiring.
Generated by PR Specification Reviewer for #76080 · copilot · gpt50 · ~8 AIC · ⌖ ~2 AIC · ⊞ 1.7K
Comment /review-specs to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd and /codebase-design. Requesting changes on one reachable crash and one failing test.
📋 Key Themes & Highlights
Key Themes
- Crash on untrusted input:
readSousChefDatathrows on deeply nested comment bodies (reproduced at depth 17).buildQueuehas no per-PR catch, so one comment aborts the queue. - Failing test:
workflow polls every five minutes but never starts an agent...fails because the prompt omitsnudges[0].data. - Dead legacy path: the
Sous-chef ci-follow-up:marker reader has no writer in this repo.
Positive Highlights
- ✅ Structured data envelope with
additionalProperties: falseand strict hex/integer checks - ✅ Fail-closed handling for rate limits and incomplete API reads
- ✅ 106 of 107 PR Sous Chef tests pass
Limits
- I did not verify the
event=dynamicandrun.pathfilters against live API data.
@copilot please address the review comments above.
Comments that could not be inline-anchored
scripts/pr-sous-chef.mjs:62
**[/diagnosing-bugs]** `readSousChefData` can throw on untrusted comment text. `parseReportMarkdown` raises `TypeError` once nesting exceeds its limit, and I reproduced it at depth 17 in a `"pr-sous-chef"` comment body. `isNudge` calls `commentData` before any author check, so one malformed comment aborts `buildQueue` and the whole queue run. <details> <summary>💡 Suggested fix</summary> Wrap the parse in a try/catch that returns `null` for `TypeError`, or check author trust before calling `co...
.github/workflows/pr-sous-chef.md:277
**[/tdd]** The failing test `workflow polls every five minutes but never starts an agent...` expects this prompt to tell the agent to post `nudges[0].data` as well as `nudges[0].body`. The prompt only mentions `.body`, so the structured envelope is never posted and the dedup markers are lost. <details> <summary>💡 Suggested fix</summary> Change the instruction to post both `nudges[0].body` and `nudges[0].data` via `add_comment` on `nudges[0].pr_number`, then re-run `node --test scripts/pr-sous...
scripts/pr-sous-chef.mjs:337
**[/codebase-design]** The `Sous-chef ci-follow-up: <id>` marker branch has no writer in the repo. Only readers at lines 129 and 337 reference it, and the new follow-up path writes structured data. This adds a second parallel dedup path that is hard to reason about. <details> <summary>💡 Suggested fix</summary> If there is no legacy writer, remove the marker reader. If it is kept for compatibility, add a comment naming the legacy writer and a test that exercises it. </details> @copilot pleas...
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~8 AIC · ⌖ ~2 AIC · ⊞ 6.3K
Comment /matt to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Untrusted comments can trigger uncaught parsing errors that halt the scheduled prefilter and block processing of other PRs.
3 open issues
What changed in this PR
This PR makes PR Sous Chef’s comments easier to read and adds completion-aware CI status updates, supported by opt-in collapsed structured data in gh-aw’s safe-output pipeline.
Changes:
- Build friendly prep lists with typed reports, descriptive links, and collapsed bookkeeping.
- Add status-only follow-ups for completed Copilot tasks with remaining CI failures.
- Extend configuration, runtime handling, tests, and documentation for
add-comment.collapse-data.
| File | Description |
|---|---|
scripts/pr-sous-chef.test.mjs |
Covers structured state, comment rendering, and completion follow-ups. |
scripts/pr-sous-chef.mjs |
Adds typed reports, structured bookkeeping, and CI follow-up selection. |
pkg/workflow/safe_outputs_handler_registry_comments.go |
Passes collapse-data configuration to runtime handlers. |
pkg/workflow/add_comment.go |
Adds the optional collapse-data setting. |
pkg/workflow/add_comment_max_links_test.go |
Tests configuration parsing and serialization. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the new boolean configuration option. |
docs/src/content/docs/reference/safe-outputs.md |
Documents collapsed comment data and state recovery. |
actions/setup/js/safe_output_type_validator.test.cjs |
Tests trusted control of structured-data layout. |
actions/setup/js/safe_output_type_validator.cjs |
Supports runtime collapse-data overrides. |
actions/setup/js/collect_ndjson_output.test.cjs |
Tests handler-configured comment data rendering. |
actions/setup/js/collect_ndjson_output.cjs |
Forwards collapse-data settings to validation. |
.github/workflows/pr-sous-chef.md |
Configures structured data and completion-follow-up instructions. |
.github/workflows/pr-sous-chef.lock.yml |
Regenerates executable workflow configuration. |
.github/workflows/agentic_commands.yml |
Updates the Sous Chef command description. |
.github/aw/safe-outputs-runtime.md |
Adds runtime documentation for collapsed data. |
.changeset/minor-collapse-comment-structured-data.md |
Records the new opt-in feature for release. |
Review effort: Max (auto)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Regarding this skills-review notice, the parser crash is fixed in 1f6b6da and legacy completion-marker recovery is now explicitly tested and documented. The body/data instruction was already corrected; all 114 sous-chef tests pass, so the older workflow-contract failure is no longer reproducible. |
|
Regarding this test-quality notice, no concrete test change was requested, and reported_no_action alone does not certify review coverage. This batch adds parser, completion, legacy-marker, and URL regressions; all 114 sous-chef tests and the repository publication gates pass. |
|
Regarding this failed-output notice, the earlier investigation traced publication failure to baseline branch-protection verification returning HTTP 403 because the token lacks Administration:read. This needs a maintainer credential follow-up, not a PR-code change; specification publication has not been established, and no rerun was started. |
|
Regarding this code-quality completion notice, 8f374ad clarified the distinct publication paths: native comments contain one envelope, while safe outputs pass data separately without inline JSON. Single-envelope recovery and deduplication are tested, and the underlying thread was already answered and resolved. |
|
Regarding this design-gate notice, reported_no_action contains no concrete architectural change request or attached ADR draft. No additional ADR is needed in response to this notice; the status alone does not establish that every gate requirement was verified. |
|
Regarding this Ponytail completion notice, the unused production running-comment renderer and report branch were removed in 8f374ad. The underlying review thread was answered and resolved; this status notice needs no additional code change. |
Why
PR Sous Chef's long SHA entries clutter its comments, and maintainers need a clear status update when an assigned Copilot task finishes but CI is still failing. This addresses the feedback on #75978 (comment).
Approach
datafield, rendered as collapsed JSON instead of visible bookkeeping.Compatibility and safeguards
Adds opt-in
safe-outputs.add-comment.collapse-data; existing comment data rendering remains unchanged by default. Readers support both structured data and legacy markers, preserving deduplication and author checks. Active-agent, idle, stale, and approval-only guards remain in place. Completion lookup is bounded to 100 recent dynamic runs on the PR branch; missing evidence is not assumed. No merging or CI dispatch is added.Includes the regenerated workflow, documentation, and a minor changeset.
Validation
make agent-report-progresspassed, including formatting, linting, impacted tests, and drift verification for all 365 workflows.