Repository navigation
Conversation
Ahamed-1G
left a comment
Ahamed-1G
left a comment
There was a problem hiding this comment.
Submitting PR review on #: event=COMMENT, comments=0, bodyLength=...
Created PR review #...: https://github.com//pull/#pullrequestreview-...
✓ PR review submitted for #
=== Processing Summary === Status: partial_success Successful: 1 Failed: 1
##[error]1 safe output(s) failed:
- create_check_run: Safe-output prerequisite did not complete successfully (E099)
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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.
|
|
✅ PR Specification Reviewer completed successfully!
|
|
✅ 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.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed head cbe5c63024ac5caec6f3c61c0f74e0a7e8d63d7b against base 6ba136231dc894becdc0dd00fe22c0f4332e02eb.
Applicable contracts: docs/src/content/docs/specs/safe-outputs-specification.md v1.29.18 (§7 review handlers; dependency semantics absent) and docs/src/content/docs/reference/safe-outputs.md (changed dependency behavior). The implementation appears aligned with the stated intentional batching behavior and preserves fail-closed dependency handling; I found no demonstrated conformance violation requiring REQUEST_CHANGES.
Proposed synchronization: add a normative §7.0.4 covering eligible review prerequisites, buffer draining before dependent release, publication-based success, deferred-dependent retry without replay, and one explicit review per PR across drained batches; update the two handler Notes, bump the patch version/date, and add document history. See the inline proposal.
Coverage: static review only; no PR code, tests, or conformance scripts were executed, per review policy.
Generated by PR Specification Reviewer for #75387 · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review-specs to run again
| success. Missing, ambiguous, or cyclic dependencies also fail closed. Independent | ||
| outputs retain their existing behavior and policy checks. | ||
|
|
||
| `submit_pull_request_review` and `create_pull_request_review_comment` can be |
There was a problem hiding this comment.
The implementation and this reference text intentionally add a new dependency/batching contract, but the authoritative formal contract remains silent: docs/src/content/docs/specs/safe-outputs-specification.md (v1.29.18), §7. Please synchronize it rather than leaving the reference as the only normative-looking description. Proposed insertion after the handler field-construction section:
### 7.0.4 Dependency and Batch Processing Semantics
**Requirement DSB1 (Eligible Review Prerequisites):** Implementations MUST permit `submit_pull_request_review` and `create_pull_request_review_comment` outputs to serve as `depends-on` prerequisites.
**Requirement DSB2 (Review Buffer Drain):** Implementations MUST drain and finalize the applicable buffered review batch before releasing outputs that depend on it.
**Requirement DSB3 (Publication-Based Success):** Buffering MUST NOT establish prerequisite success; success requires publication.
**Requirement DSB4 (Deferred-Dependent Retry):** Newly eligible deferred dependents MUST be retried without replaying completed handlers.
**Requirement DSB5 (Single Explicit Review Submission):** Implementations MUST preserve at most one explicit `submit_pull_request_review` per pull request across drained batches.Update the review/comment handler Notes to reference §7.0.4, and bump the spec patch version/date with a document-history entry. Keep the existing unsupported/delegated-output and fail-closed obligations unchanged.
There was a problem hiding this comment.
Addressed in 57ffa16: added normative §7.0.4 (DSB1–DSB5), linked both review handlers’ Notes, and bumped the spec to v1.29.19 with a history entry. Publication date remains today, 2026-11-06. Unsupported/delegated prerequisites and fail-closed behavior remain prohibited.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Review verdict
Request changes: the new finalization loop can exit while a fresh buffered review remains pending when retries update existing result records in place.
Highlights
- Ensure loop progress tracks pending buffered work, not only
results.length. - Add a regression test for an in-place deferred retry that creates a subsequent review batch.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| do { | ||
| await finalizeBufferedOutputs(results); | ||
| previousResultCount = results.length; | ||
| await drainWaitingMessages(); |
There was a problem hiding this comment.
The finalization pass can stop after an in-place retry even when that retry has created a new buffered review, leaving the review unpublished and its dependents unresolved.
💡 Continue until buffered work is drained
previousResultCount only detects appended result records. retryDeferredMessages() updates existing records in place, so a retry that returns deferred_manifest and adds a fresh registry buffer can leave results.length unchanged; the loop exits without another finalizeBufferedOutputs call. Track actual pending registry/deferred-manifest work (with a bounded pass count) rather than using result-array growth as the progress condition.
There was a problem hiding this comment.
Fixed in 57ffa16. Bounded finalization passes now detect newly buffered result objects, including in-place retries, instead of relying on result-array growth. Retries require resolution progress. Added regressions for in-place buffering and no-progress finalization; targeted and impacted tests pass.
There was a problem hiding this comment.
What this change does:
It moves buffered PR-review publication into the dependency loop so checks and chained outputs wait for the real review result. It also tracks per-run review targets and documents the new prerequisite behavior.
Should fix:
-
Buffered reviews have two status classifiers (
actions/setup/js/safe_output_dependencies.cjs:L153)- What this is: This line adds a second place that knows
deferred_manifestmeans “deferred”. - Problem:
classifySafeOutputResultstill returnssuccessfor{success: true, result: {deferred_manifest: true}}, whilebuildSafeOutputOutcomespatches the same record todeferred; every new consumer now needs another exception. - Fix: Fold
deferred_manifestintoclassifySafeOutputResult, then remove this condition and thebuildSafeOutputOutcomesspecial case. - If we skip it: A consumer that uses the shared classifier can report or release a buffered review as successful even though it has not been published.
- What this is: This line adds a second place that knows
Nice to have:
-
Two guards enforce the same review limit (
actions/setup/js/submit_pr_review.cjs:L215)- What this is:
reviewTargetsis now the per-run guard. - Problem: In registry mode this handler is the only production writer of review metadata; it adds the target immediately after writing it. Before the first call the buffer is empty, and after
drainEntriesthe new buffer is empty but the set still contains the target, sobuffer.hasReviewMetadata()never changes the result. - Fix: Keep only
reviewTargets.has(reviewTarget)here. - If we skip it: The one-review rule has two sources of truth that can drift.
- What this is:
Verdict: fix 1 first.
Lean: -1 lines possible.
Generated by ✂️ Ponytail Reviewer for #75387 · codex · routed: gpt56 max · ~6 AIC · ⌖ ~1 AIC · ⊞ 118
Comment /ponytail to run again
| const record = records[0]; | ||
| if (isConfirmedDependencySuccess(record)) continue; | ||
| if (allowWaiting && (!record || classifySafeOutputResult(record) === "deferred")) { | ||
| if (allowWaiting && (!record || classifySafeOutputResult(record) === "deferred" || (record.success === true && pickOutcomeField(record, "deferred_manifest")))) { |
There was a problem hiding this comment.
[Should fix] This adds a second deferred-state classifier: classifySafeOutputResult returns success for {success: true, result: {deferred_manifest: true}}, while buildSafeOutputOutcomes already patches it to deferred. Fold deferred_manifest into the shared classifier and remove these call-site exceptions so a buffered review cannot get different statuses in different consumers.
There was a problem hiding this comment.
Fixed in 57ffa16: deferred_manifest classification now lives in classifySafeOutputResult, and the dependency/outcome call-site exceptions are removed. Regression coverage verifies consistent classifier, summary, and persisted-outcome statuses while retaining failed/skipped precedence.
|
|
||
| if (buffer.hasReviewMetadata()) { | ||
| const reviewTarget = `${repo}#${prNum}`; | ||
| if (buffer.hasReviewMetadata() || reviewTargets.has(reviewTarget)) { |
There was a problem hiding this comment.
[Nice to have] In registry mode the only production writer of review metadata is the setReviewMetadata call immediately below, which also adds reviewTarget; before that call the buffer is empty, and after drainEntries the new buffer is empty but the set still contains the target. buffer.hasReviewMetadata() therefore adds no behavior—keep only the per-run target guard.
There was a problem hiding this comment.
Simplified in 57ffa16 to the per-run reviewTargets guard alone. Existing duplicate-review coverage, including rejection after draining a batch, passes.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. The fix is sound and fails closed, with one diagnostic gap and one test gap. No blocking issues.
📋 Key Themes & Highlights
Key Themes
- Skipped review prerequisites: the skip path leaves
deferred_manifest: true, so the dependent waits and then fails with a generic error. The skip reason is lost (inline comment onsafe_output_dependencies.cjs). - Test gap: the
skippedscenario does not assert the reason (inline comment onsafe_output_outcomes.test.cjs).
Positive Highlights
- ✅
drainEntries()gives each batch a single drain and a fresh buffer, which enables chained reviews. - ✅ The
reviewTargetsguard enforces one explicit review per PR, even after draining. - ✅ The
allowWaitingrule fordeferred_manifestis narrow and rejects staged, queued, and buffered outcomes. - ✅ The
it.eachmatrix covers success, chained, failed, skipped, and staged outcomes.
Limits of this review
- Vitest suites were not run:
npm cifailed on a TLS certificate error in this environment, so the test claims are from static reading only. - Skill selection used the pr-triage output.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~13 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
| const record = records[0]; | ||
| if (isConfirmedDependencySuccess(record)) continue; | ||
| if (allowWaiting && (!record || classifySafeOutputResult(record) === "deferred")) { | ||
| if (allowWaiting && (!record || classifySafeOutputResult(record) === "deferred" || (record.success === true && pickOutcomeField(record, "deferred_manifest")))) { |
There was a problem hiding this comment.
[/diagnosing-bugs] A skipped review prerequisite keeps deferred_manifest: true. skipReviewResultsForPR (safe_output_handler_manager.cjs, about L859-869) sets only skipped and skipReason, so this branch reports the dependent as waiting. The final pass then fails it with a generic error ("did not complete successfully") and the skip reason is lost. It fails closed, but the diagnosis is lost.
💡 Suggested fix
When a review is skipped, clear deferred_manifest on the record result so the gate settles immediately, and carry skipReason into the dependent error. Reproduced with a standalone checkSafeOutputDependencies call: with allowWaiting=true, a skipped review returns {state: "waiting"}.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 57ffa16. Skipping buffered reviews clears deferred_manifest; dependency checks classify skips immediately and preserve skipReason (or the handler’s reason) in the dependent failure. Added coverage for closure before processing and after buffering.
| } else { | ||
| expect(createCheck).not.toHaveBeenCalled(); | ||
| expect(global.core.setFailed).toHaveBeenCalled(); | ||
| expect(outcomes.find(outcome => outcome.type === "create_check_run").status).toBe("failed"); |
There was a problem hiding this comment.
[/tdd] The skipped case asserts only that the check fails and setFailed is called. Nothing checks why, so the dropped skip reason from the comment on safe_output_dependencies.cjs is not caught.
💡 Suggested assertion
Add an expectation that the check outcome error, or the review outcome skipReason, contains the stale-PR reason. That turns the fix above into a regression test.
@copilot please address this.
There was a problem hiding this comment.
Added assertions in 57ffa16 that skipped reviews retain their reason and that dependent failure diagnostics contain the stale-PR explanation. Both the original skipped scenario and a new skipped-after-buffering scenario pass.
There was a problem hiding this comment.
🟡 Changes recommended
Review supersession can dismiss earlier blocking reviews before dependent comments succeed, while unconditional retries can exhaust unresolved handlers’ quotas.
2 open issues
What changed in this PR
This PR integrates buffered PR-review publication with gh-aw’s safe-output scheduler so dependent operations can wait for confirmed publication, addressing #75293.
Changes:
- Finalizes successive review batches before releasing dependent outputs.
- Confirms published inline-comment outcomes and preserves the one-explicit-review-per-PR guard across batches.
- Adds regression coverage and documents batching and prerequisite restrictions.
| File | Description |
|---|---|
docs/src/content/docs/reference/safe-outputs.md |
Documents review batching and dependency restrictions. |
actions/setup/js/submit_pr_review.test.cjs |
Tests duplicate-review rejection after draining. |
actions/setup/js/submit_pr_review.cjs |
Preserves the per-PR review guard across batches. |
actions/setup/js/safe_output_outcomes.test.cjs |
Covers review-dependent checks and submission outcomes. |
actions/setup/js/safe_output_outcomes.cjs |
Clears buffered status after inline-comment publication. |
actions/setup/js/safe_output_handler_manager.cjs |
Finalizes review batches before resuming dependents. |
actions/setup/js/safe_output_dependencies.test.cjs |
Tests buffered prerequisites and retry sequencing. |
actions/setup/js/safe_output_dependencies.cjs |
Allows dependents to wait for buffered publication. |
actions/setup/js/pr_review_buffer.test.cjs |
Tests draining and fresh buffers for subsequent batches. |
actions/setup/js/pr_review_buffer.cjs |
Adds review-buffer batch draining. |
Review effort: Max (auto)
| skipReviewResultsForPR(reviewResults, reviewRepo, reviewPrNum, stalePRReason); | ||
| continue; | ||
| } | ||
| const reviewResult = await reviewBuffer.submitReview(); |
| await drainWaitingMessages(); | ||
| await retryDeferredMessages(); |
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: cbe5c63
|
Assignment running:
I’ll validate the changes, reply to the listed threads, and stop without merging the PR or dispatching CI. Sous-chef requested-head: cbe5c63 |
…-dependency-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Outputs depending on
submit_pull_request_reviewfailed even after the review was posted because dependency checks ran before buffered submission completed.