Repository navigation
fix(work-queue): verify discussion cleanup effects during Claim settlement - #71361
Conversation
…ement Capture private cleanup receipts and independently verify comments and outdated discussion closure. Add bounded readback retries, redacted transport diagnostics, and local Claim settlement regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Match the new response and request-ID presence diagnostics while preserving strict HTTP status, failure classification, error propagation, and privacy assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![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 /diagnosing-bugs, /tdd, and /codebase-design (triage was unavailable; fallback heuristic used against pr-meta.json/diff) — submitting as COMMENT with three minor, non-blocking findings.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap:
close_older_discussions.cjsreceipt-push logic lacks direct unit tests; coverage is only indirect via the larger settlement-replay fixture inwork_queue_native_creation_delivery.test.cjs. - Defensive bound missing:
verifyDiscussion's cleanup-readback loop has no explicit cap, unlike theMAX_CLOSE_COUNT(10) enforced upstream when generating receipts. - Logging inconsistency: cleanup mismatch uses always-on
log.diagnosticwhile the adjacent primary-discussion mismatch uses DEBUG-gatedlog.debug— possibly intentional, but undocumented.
Positive Highlights
- ✅ Fail-closed design is consistently applied: missing/changed/foreign/ambiguous cleanup evidence correctly blocks settlement (
{ verified: false }), matching the PR's stated goal. - ✅ Strong negative-case test matrix (
missing_receipts,comment_changed,reopened,reason_changed,foreign_repository,unavailable) inwork_queue_native_creation_delivery.test.cjsdirectly exercises the new verification branches. - ✅ Privacy-preserving logging additions in
work_queue_logging.cjs(transport codes, HTTP method, request/response presence flags) are carefully bounded and avoid leaking URLs/payloads/credentials, backed by dedicated tests asserting no "private-" strings leak. - ✅ Retry/readback reuse via
readQueueMetadatafor the new cleanup query keeps the three-attempt bound consistent with existing discussion reads.
Note: given the diff size (458 lines across 9 files), review focused on the highest-impact verification/delivery logic; exhaustive tracing of every downstream authority_resources consumer was out of scope for this pass.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~135 AIC · ⌖ ~17 AIC · ⊞ 10.2K
Comment /matt to run again
Cover cleanup receipt contracts directly, cap independent readbacks at two receipts per allowed discussion, and explain always-visible cleanup mismatch diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Regarding /matt: The requested review completed, and its three findings are addressed in c7a52c6. No additional code change is needed for the command itself. |
|
Regarding the review completion notice: Acknowledged; all three inline findings are addressed in c7a52c6, with the combined validation gate passing 304 tests. This status notice requires no separate code change. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: 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.
|
|
✅ 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.
|
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.
Request changes
The cleanup receipt loop can throw on malformed entries instead of producing the intended fail-closed unknown settlement. Validate each cleanup item before dereferencing it and add malformed-entry regression coverage.
Details
One inline comment was posted on work_queue_native_creation_delivery.cjs:153. The specialist pass was unavailable (it returned no parseable findings), so this review is based on the independent diff analysis.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
Bring in the work-queue output and Bash 3-compatible validation fixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage: bug_fix). This is a careful fix: the Claim settlement cleanup-verification path (comment body, parent discussion, repository identity, OUTDATED closure) is well-covered by negative-case tests (missing receipts, reopened, reason-changed, foreign repo, unavailable readback), and the two prior review threads (unbounded receipt array, inconsistent log level) were already addressed in commit c7a52c6.
One minor efficiency note left inline: resolveRepositoryTarget re-resolves the repository identity via a fresh REST call for every cleanup receipt (up to 20), even though the repository match against the already-resolved authority.repository_id was just verified a few lines above. Not a correctness bug — bounded and fail-closed — but worth simplifying to avoid redundant API calls at scale.
📋 Key Themes & Highlights
Key Themes
- Redundant
rest.repos.getcalls per cleanup item inwork_queue_native_creation_delivery.cjs(inline comment)
Positive Highlights
- ✅ Fail-closed design consistently applied across all new mismatch branches
- ✅ Strong regression coverage for the exact bug class (missing/changed/foreign/ambiguous cleanup evidence)
- ✅ Privacy-preserving diagnostics in
work_queue_logging.cjsverified by dedicated tests asserting no leakage of URLs/bodies/credentials - ✅ Retry bound (3 attempts) correctly scoped to reads only, never mutations
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~91 AIC · ⌖ ~14 AIC · ⊞ 10.2K
Comment /matt to run again
Include the transitive dependencies introduced by Claim discussion cleanup verification in isolated MCP and safe-output deployments, and load the delivery module in both deployed-bundle tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The Claim settlement trust boundary warrants human review, and a confirmed deployment dependency failure remains unresolved.
2 open issues
What changed in this PR
This PR extends gh-aw’s Claim settlement verification to cover comments and closures on older discussions, so independently verified cleanup effects can contribute to settlement.
Changes:
- Capture private cleanup receipts and verify content, discussion identity, repository, and closure state.
- Retry transient discussion readbacks without replaying mutations.
- Add privacy-preserving diagnostics and regression coverage for settlement and repeated finalization.
| File | Description |
|---|---|
| actions/setup/js/work_queue_read_retry.test.cjs | Tests bounded retry diagnostics without DEBUG. |
| actions/setup/js/work_queue_read_retry.cjs | Reports attempt counts and retry delays. |
| actions/setup/js/work_queue_native_creation_delivery.test.cjs | Covers cleanup evidence, receipt limits, and settlement replay. |
| actions/setup/js/work_queue_native_creation_delivery.cjs | Verifies cleanup receipts and retries discussion reads. |
| actions/setup/js/work_queue_logging.test.cjs | Tests privacy-safe transport metadata and getter handling. |
| actions/setup/js/work_queue_logging.cjs | Adds restricted request and transport diagnostics. |
| actions/setup/js/work_queue_delivery.cjs | Makes uncovered-effect diagnostics visible without DEBUG. |
| actions/setup/js/reconcile_work_queue_run.test.cjs | Updates expected failure diagnostic fields. |
| actions/setup/js/create_discussion.cjs | Passes cleanup receipts into delivery capture. |
| actions/setup/js/close_older_discussions.test.cjs | Tests Claim-scoped cleanup receipt capture. |
| actions/setup/js/close_older_discussions.cjs | Collects and forwards cleanup receipts. |
Review effort: Max
Reject malformed cleanup receipts explicitly and reuse independently resolved repository authority while retaining canonical target validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Regarding the code-quality review-start notice: Findings are handled separately in their inline threads. This status notice requires no code change. |
|
Regarding the Design Decision Gate notice: No actionable change is specified here, so no code change is needed. The reported no-action outcome does not independently establish correctness or completion. |
|
Regarding the prior response: This records the earlier review resolution and requires no additional code change. |
|
Regarding the validation acknowledgment: This records earlier validation, not validation of subsequent changes. No code change is needed for the acknowledgment. |
|
Regarding the completion notice: The associated findings were addressed in c7a52c6. This notification requires no separate code change. |
|
Regarding the review-start notice: This is informational and requires no code change or reviewer rerun. |
|
Regarding Comment Memory: The cleanup-receipt finding is addressed separately in its inline thread. No edit to the bot-managed memory is needed. |
|
Regarding the Ponytail Reviewer notice: This notice specifies no actionable change, so no code change is needed. Its no-action outcome is not independent validation. |
|
Regarding /matt: The requested review completed; its three findings were addressed in c7a52c6. No additional command or code change is needed. |
|
Regarding the Test Quality Sentinel notice: This notice specifies no actionable change, so no code change is needed. Its no-action outcome does not replace testing. |
…e targets Share the cleanup limit through dependency-free constants, test embedded-only verifier loading, and retain one authorization target per distinct cleanup resource without skipping effect readbacks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
ESLint Refiner successfully published its outputs but could not settle its Claim because the comment and closure on an older discussion were not covered by independently verified delivery receipts. This adds private cleanup receipts and verifies the comment body, parent discussion, repository identity, and OUTDATED closure before accepting their recorded effects.
Validation
make fmt-cjs && make lint-cjsnpm --prefix actions/setup/js run typecheckPATH="/opt/homebrew/bin:$PATH" make agent-report-progressScope and limitations
This is a mocked replay of the cleanup/settlement path, not the entire captured Refiner workflow. No live Claim was repaired, production mutations replayed, or workflow dispatched. The origin of the observed HTTP 500s remains unproven.
The initial static security review found no confirmed vulnerabilities. Subsequent formatting and test-only corrections did not receive a refreshed review because same-reviewer follow-up was unavailable.