fix: reject submit_pull_request_review max > 1 at compile time

[Copilot]

The runtime uses a single shared createReviewBuffer() for the entire run. With max > 1, the handler accepts all calls but submits only one review — silently collapsing to the first call's target and the last call's body. Intermediate calls are lost with no error.

Changes

pkg/workflow/safe_outputs_max_validation.go

• Adds checkMaxFieldAtMostOne(): wraps checkMaxField() (keeps standard range rejection for 0/-2/etc.), then additionally rejects -1 (unlimited) and any value > 1
• validateSafeOutputsMax: switches submit_pull_request_review from checkMaxField → checkMaxFieldAtMostOne

Error message is actionable — names the single-buffer limitation and directs authors to target: "*" with max: 1:

safe-outputs.submit-pull-request-review: max must be 1; the runtime uses a single
shared review buffer and cannot submit multiple independent reviews in one run.
To review multiple PRs, use target: "*" with max: 1 (or omit max entirely) so each
PR gets its own invocation and review buffer. Multi-buffer support will be added in
a future release

pkg/workflow/safe_outputs_max_validation_test.go

• TestCheckMaxFieldAtMostOne: unit tests for the new helper
• TestSubmitPullRequestReviewMaxValidation: end-to-end through validateSafeOutputsMax — covers max=2 rejected, max=-1 rejected, max=1 accepted, nil accepted, expression skipped

The existing TestValidateSafeOutputsMaxFieldCoverage (uses max=0) continues to pass because checkMaxFieldAtMostOne delegates to checkMaxField first, which produces the standard error for out-of-range values.

[pelikhan]

@copilot add support for multi review buffers

[Copilot]

@copilot add support for multi review buffers

On it — I'll implement multi-buffer support in the runtime so that each distinct (repo, pull_request_number) pair gets its own createReviewBuffer() instance, and all buffers are finalized in order. The compile-time max > 1 rejection will be reverted.

@copilot add support for multi review buffers

Done. The implementation adds createPrReviewBufferRegistry() to pr_review_buffer.cjs — a Map<"repo#prNum" → buffer> registry that creates an independent buffer per distinct PR on demand.

Key changes:

• submit_pr_review.cjs: When _prReviewBufferRegistry is provided (registry mode), resolves the target PR first, then calls registry.getOrCreate(repo, prNum) to obtain the per-PR buffer. Result includes pull_request_number + repo for per-PR rollback. The _prReviewBuffer legacy path is preserved unchanged for backward compat.
• create_pr_review_comment.cjs: Registry mode calls getOrCreate(repo, prNum) per message; footer context is set per-buffer (first-wins, no-op on repeat); cross-PR rejection is removed since each PR has its own buffer.
• safe_output_handler_manager.cjs: Creates prReviewBufferRegistry instead of a single buffer; passes it as _prReviewBufferRegistry; finalizes all registry entries in a loop with per-PR logging; adds rollbackReviewResultsForPR / skipReviewResultsForPR for accurate per-PR failure attribution.

Two calls targeting distinct PRs now produce two independently submitted reviews. See commits 9095e82 and ab7b7b0.

[github-actions]

🤖 PR Triage

Field	Value
Category	bug
Risk	high
Score	72/100 (impact:40, urgency:20, quality:12)
Action	fast_track

Adds compile-time rejection of submit_pull_request_review with max > 1 — prevents silent data loss. Large change (+575/-128), draft. High correctness impact. Prioritize review after undrafting.

Generated by 🔧 PR Triage Agent · 143.8 AIC · ⌖ 7.64 AIC · ⊞ 5.6K · ◷

[pelikhan]

/matt

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

Skills-Based Review 🧠

Applied /codebase-design and /tdd — requesting changes on three issues: a silent missing-footer regression in registry mode, a dangerous fallback in the new rollback helper, and an unclear max contract for multi-PR scenarios.

📋 Key Themes & Highlights

Issues

1. Footer context not propagated to registry buffers (correctness) — safe_output_handler_manager exports setDefaultFooterContext from the registry but never calls it. Body-only reviews via submit_pull_request_review in registry mode will silently omit the workflow footer. The fix is a one-liner in the manager.

2. rollbackReviewResultsForPR broad fallback is unsafe in multi-PR runs (correctness) — When no per-PR result match is found, the fallback rolls back all review results. In a multi-PR run this would corrupt success counts for unrelated PRs. The fallback should be scope-guarded by registry size.

3. processedCount semantics are ambiguous with target: "*" (contract gap) — The counter is global across all target PRs. max: 1 with two distinct PRs silently drops the second. The contract needs to be documented and tested.

Positive Highlights

• ✅ Clean registry abstraction in createPrReviewBufferRegistry with first-wins semantics for defaults
• ✅ Backward-compat legacy path retained without duplication
• ✅ Good test coverage for registry unit tests (insertion order, null guards, idempotency)
• ✅ Per-PR rollback/skip helpers are the right architectural direction
• ✅ setReviewContextOnBuffer extraction is a nice deduplication

done

[pelikhan]

@copilot run pr-finisher

[pelikhan]

@copilot add tests for multiple reviews

[Copilot]

@copilot please run the pr-finisher skill, address any unresolved review feedback, refresh the branch if needed, and rerun checks once updates are complete.

...

All review feedback has been addressed and changes are committed locally, but the push is blocked by a 403 permission error for gh-aw-bot. A maintainer will need to re-trigger or grant push access to flush these commits.

Changes made (commit 7e1414e on top of 792212d):

1. safe_output_handler_manager.cjs — Fixed rollbackReviewResultsForPR and skipReviewResultsForPR to read r.result?.repo / r.result?.pull_request_number instead of the top-level r.repo / r.pull_request_number. processMessages wraps each handler result under a result sub-object, so the old code never matched any results — a finalization failure for one PR would always fall through to the global fallback and roll back all PR reviews.

2. create_pr_review_comment.cjs — Now calls registry.setDefaultFooterContext(footerCtx) at init time for registry mode (alongside the existing legacyBuffer.setFooterContext() for legacy mode). This ensures runs that emit only submit_pull_request_review (no create_pull_request_review_comment) still get the footer applied to their buffer, since registry.getOrCreate() propagates the default on first creation.

3. submit_pr_review.cjs — Added a guard before setReviewMetadata: rejects a second submit call targeting the same PR buffer with an explicit error rather than silently overwriting the first call's body/event.

4. awf_reflect.cjs — Fixed JSDoc type models_url?: string → models_url?: string | null to match runtime behavior (test line 149 uses null).

5. safe_output_handler_manager.test.cjs — Added 12 new test cases for rollbackReviewResultsForPR and skipReviewResultsForPR covering nested-result shapes, per-PR/per-repo filtering, fallback to global rollback, and edge cases.

6. submit_pr_review.test.cjs — Added missing afterEach to vitest imports (was present in the test body at line 793 but missing from the import, causing the entire file to fail to load).

[Copilot]

@copilot add tests for multiple reviews

Done in commit fix: fix per-PR result matching in rollback/skip and add multi-review tests.

Added tests in two files:

safe_output_handler_manager.test.cjs — 12 new cases for rollbackReviewResultsForPR and skipReviewResultsForPR, covering:

• Nested result shape (r.result.repo/r.result.pull_request_number) as produced by processMessages — only the matching PR is rolled back/skipped
• Cross-PR isolation: a failure for PR 1 does not touch PR 2's results
• Fallback to global rollback when no results carry per-PR identifiers (legacy shape)
• Backward-compat: top-level repo/pull_request_number also matched

Also fixed the bug these tests exposed: rollbackReviewResultsForPR and skipReviewResultsForPR were reading r.repo/r.pull_request_number directly, but processMessages nests the handler result under r.result. The fix uses r.result?.repo ?? r.repo (with fallback for callers that pass raw handler results).

create_pr_review_comment.test.cjs — 4 new cases for registry mode (_prReviewBufferRegistry):

• Comments for two different PRs each land in their own buffer
• Multiple comments for the same PR accumulate in one buffer
• Footer context is propagated to the per-PR buffer
• Missing pull_request_number in target: "*" mode returns a clear error

[gh-aw-bot]

@copilot please run the pr-finisher skill, address any remaining review feedback, refresh the branch if needed, and rerun checks once updates are complete.

Generated by 👨🍳 PR Sous Chef

Generated by 👨‍🍳 PR Sous Chef · 11.2 AIC · ⌖ 5.7 AIC · ⊞ 7.5K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill, address any remaining review feedback, refresh the branch if needed, and rerun checks once updates are complete.

Generated by [👨🍳 PR Sous Chef](ht...

Done. Three remaining issues from the automated review are now fixed (commit fix: registry footer context at init, duplicate submit guard, and afterEach import):

1. create_pr_review_comment.cjs — Registry mode now calls registry.setDefaultFooterContext(footerCtx) at init time alongside the legacy-buffer path, so runs that emit only submit_pull_request_review (no review-comment messages) still get the footer propagated to each buffer on creation.

2. submit_pr_review.cjs — Added an explicit guard before setReviewMetadata: if the buffer for that repo#prNum already has review metadata, the second call is rejected with success: false and a clear error message instead of silently overwriting the first call's body/event.

3. submit_pr_review.test.cjs — Updated the "two calls to the same PR" test to assert success: false with the expected error message, and added the missing afterEach import that was preventing the file from loading.

Pre-existing test failure in safe_output_handler_manager.test.cjs (loadHandlers > should pass top-level mentions config) is unrelated to this PR — it fails due to a missing prompt file in the sandbox environment and was present before any of these changes.

[pelikhan]

@copilot merge main and recompile

Fix https://github.com/github/gh-aw/actions/runs/29408426569/job/87351079808#step:7:1

[Copilot]

@copilot merge main and recompile

Fix https://github.com/github/gh-aw/actions/runs/29408426569/job/87351079808#step:7:1

Done in commit fix: add result? to rollbackReviewResultsForPR JSDoc type to fix TS2339, merge main.

• Merged main (clean, no conflicts)
• Fixed the TS2339 error: rollbackReviewResultsForPR's JSDoc param type was missing result?: {repo?: string, pull_request_number?: number} — the skipReviewResultsForPR sibling already had it, but rollbackReviewResultsForPR did not
• Recompiled all workflow lock files

[github-actions]

🤖 PR Triage

Field	Value
Category	bug
Risk	🔴 High
Priority Score	78/100
Recommended Action	⚡ fast_track

Score breakdown: Impact 42 + Urgency 24 + Quality 12

Rationale: Fixes a silent data-loss bug in safe-outputs runtime — submit_pull_request_review with max > 1 silently drops intermediate calls. High correctness impact on critical-path agent tooling.

⚠️ CI: js-typecheck failing — must be resolved before merge.

Generated by 🔧 PR Triage Agent · 35 AIC · ⌖ 7.73 AIC · ⊞ 5.6K · ◷

[gh-aw-bot]

pr-sous-chef
@copilot please run the pr-finisher skill, address unresolved review comments, and rerun checks once finished.

Run: https://github.com/github/gh-aw/actions/runs/29420425558

Generated by 👨‍🍳 PR Sous Chef · 10.2 AIC · ⌖ 4.88 AIC · ⊞ 5.1K · ◷
_{Comment /souschef to run again}

[Copilot]

pr-sous-chef
@copilot please run the pr-finisher skill, address unresolved review comments, and rerun checks once finished.
...

All review feedback has been addressed in the commits on this branch. Here's the current state:

Reviews

• Outdated threads (rollbackReviewResultsForPR result nesting, skipReviewResultsForPR result nesting, footer context init, awf_reflect.cjs models_url type): all fixed in commits 8acf6e6 and 41f5224.

• submit_pr_review.cjs:192 (duplicate submit guard): fixed in commit 41f5224 — buffer.hasReviewMetadata() check now rejects a second submit_pull_request_review call for the same PR with an explicit error.

• safe_output_handler_manager.cjs:1488 (multi-PR rollback/skip tests): tests are in place covering nested-result shapes, per-PR isolation, and fallback — see rollbackReviewResultsForPR / skipReviewResultsForPR suites in safe_output_handler_manager.test.cjs.

• safe_output_handler_manager.cjs:1442 (PR title vs. implementation mismatch): the PR scope changed from compile-time rejection to multi-buffer runtime support per @pelikhan's request ("@copilot add support for multi review buffers"). The PR title and description need to be updated to reflect multi-buffer support rather than compile-time rejection — this requires a maintainer edit.

Checks (local): make fmt ✓, make lint ✓, JS unit tests (58 passing) ✓. Go pkg/cli bootstrap tests fail in the sandbox due to DNS proxy blocking external API calls — pre-existing and unrelated to this PR.

Mergeable: branch is up to date with main (merged in commit f6e9f41), no conflicts.

Hand-off: CI must be re-triggered by a maintainer (close/reopen or push). The PR title/description should be updated to reflect the multi-buffer implementation scope before merge.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.10