Repository navigation
[WIP] Restore classifier cost attribution in routed-run audits - #67230
SivaKesava1 with Copilot wants to merge 3 commits into
Conversation
Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused revert consistently removes the unrelated dispatcher no-op exception and updates its tests.
0 open findings
What changed in this PR
Reverts unrelated absent-queue dispatcher authorization changes so #67092 remains focused on classifier-cost attribution.
Changes:
- Restores trusted per-Claim enforcement for safe outputs.
- Updates claim-scope tests to require dispatcher and worker rejection.
| File | Description |
|---|---|
actions/setup/js/work_queue_claim_scope.cjs |
Removes the no-op authorization bypass. |
actions/setup/js/work_queue_claim_scope_checks.cjs |
Restores rejection expectations for unassigned roles. |
actions/setup/js/safe_outputs_handlers.cjs |
Removes direct handling outside Claim execution. |
actions/setup/js/safe_output_handler_manager.cjs |
Restores unconditional scoped-manager enforcement. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed for PR #67230: no "implementation" label and 0 new lines in default business logic directories (threshold 100, 4 files changed, no custom .design-gate.yml).
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking issue
The revert is only partial: absent-branch dispatcher noops are still treated as valid by normalizeRuntimeMessage(), but this PR now rejects them later in processMessages() and updates the scope check to expect the earlier normalization step to fail. That leaves the queue-scope contract internally inconsistent and should break the updated test.
Details
work_queue_claim_scope.cjs still has the explicit absentDispatcher && message?.type === "noop" allowlist, so the dispatcher path has not been fully reverted. The changed manager/handler assertions now disagree with that lower-level normalization behavior instead of removing it at the source.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 35.4 AIC · ⌖ 7.23 AIC · ⊞ 19.4K
Comment /review to run again
| messages.every(message => message?.type === "noop"); | ||
| if (claimScope && !currentClaimHandle() && !absentBranchDispatcherNoop) { | ||
| if (readClaimScopeContext() && !currentClaimHandle()) { | ||
| throw new Error("Queue safe outputs must execute in a trusted per-Claim handler context; use the scoped manager"); |
There was a problem hiding this comment.
This only blocks the absent-branch dispatcher noop at processMessages() time, but normalizeRuntimeMessage() still explicitly returns that message unchanged, so the revert is inconsistent and the new expectation in work_queue_claim_scope_checks.cjs:325 will fail.
💡 Why this is blocking and how to fix it
work_queue_claim_scope.cjs still allows an unassigned dispatcher noop in normalizeRuntimeMessage(). That means a dispatcher snapshot with no assignment still normalizes noop as valid, while this change now rejects the same message later in processMessages() and the updated test now expects normalizeRuntimeMessage() itself to throw. Please either remove the absentDispatcher && noop escape hatch from normalizeRuntimeMessage() as part of this revert, or keep the allowance end-to-end and stop asserting the earlier failure in the scope-check test.
There was a problem hiding this comment.
Fixed in e475ebb: normalizeRuntimeMessage() now rejects all unassigned dispatcher messages, including noop, matching the later processing-time authorization check. The previously failing JavaScript shard and final local validation pass.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — this is a clean, maintainer-requested mechanical revert of ac24247 ("Allow dispatcher no-op on absent queue"), and no actionable issues were found.
📋 Verification details
What I checked
- Diffed the four touched files against the exact inverse of
ac24247; the revert is byte-for-byte consistent with what was added in that commit. - Searched the repo for any remaining references to
absentBranchDispatcherNoop, "dispatcher no-op", or related identifiers — none found. No dangling helper functions, exports, or docs reference the removed bypass. - Reviewed
work_queue_claim_scope_checks.cjs: the updated test correctly tightens the assertion so that bothdispatcherandworkerroles now throw for an absent-branch/empty-ledger scenario (previously onlyworkerthrew;dispatcherhad a carve-out). This matches the removed authorization bypass and is a faithful regression test for the revert — satisfying/tdd's concern that reverts should be accompanied by a test that would catch the bypass being reintroduced. /codebase-design: the revert removes a special-cased branch that complicatedprocessMessages's authorization check; the simplifiedif (readClaimScopeContext() && !currentClaimHandle())is easier to reason about and consistent with the "trusted per-Claim handler context" invariant documented elsewhere in the file.
Scope note
Per the PR description, this change intentionally does not touch the classifier cost attribution logic that is the stated subject of the (still-WIP) PR title — only the work-queue safe-output authorization revert requested in the triggering review comment. That's correctly scoped as "one fix per PR."
Positive highlights
- ✅ Revert is complete and self-consistent — no orphaned code paths.
- ✅ Test change directly encodes the tightened authorization contract rather than just deleting coverage.
No blocking issues. I don't have network access in this sandbox to run npm run test:js for work_queue_claim_scope.test.cjs / work_queue_delivery.test.cjs, so please confirm CI passes those suites before merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 39.1 AIC · ⌖ 15.6 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
Review summary
This PR aims to revert the unrelated "Allow dispatcher no-op on absent queue" change (ac24247) from the classifier-cost-attribution PR, per maintainer feedback. The revert correctly removes the bypasses in safe_output_handler_manager.cjs, safe_outputs_handlers.cjs, and one bypass in work_queue_claim_scope.cjs, and updates the corresponding test.
Blocking issue: the revert is incomplete. A second, equivalent noop-on-absent-dispatcher bypass remains in normalizeRuntimeMessage within work_queue_claim_scope.cjs (not touched by this diff). Because normalizeRuntimeMessage runs before the authorization check this PR removes, the leftover branch still lets a dispatcher emit an unauthorized noop on an absent queue — the exact behavior being reverted.
I verified this locally: node --test work_queue_claim_scope_checks.cjs fails on the test this PR modified ("permits genuine absent-branch outputs only for a protected observer and rejects existing empty ledgers before handlers") with Missing expected exception, because the dispatcher-role case in the new for (const role of ["dispatcher", "worker"]) loop doesn't throw.
See inline comment for the exact lines to remove.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 56.1 AIC · ⌖ 13.4 AIC · ⊞ 8.1K
| @@ -182,7 +182,6 @@ async function assertClaimAuthorized(message, options = {}) { | |||
| const execution = claimExecution.getStore(); | |||
There was a problem hiding this comment.
Revert is incomplete — leftover dispatcher no-op bypass breaks this file's own test
This hunk removes the noop bypass from assertClaimAuthorized, but an equivalent bypass still exists a few lines above in normalizeRuntimeMessage (same file, around line 165-167):
if (!scope.assignment) {
const absentDispatcher = scope.snapshot?.sha === null && scope.snapshot.transactionLog === "" && scope.snapshot.worker === null;
if (absentDispatcher && message?.type === "noop") return message;
throw scopeError("unassigned dispatcher cannot emit worker safe outputs");
}Since normalizeRuntimeMessage is called first (including from inside assertClaimAuthorized), this leftover branch still allows a dispatcher-role noop on an absent queue to pass through unauthorized, exactly the behavior this PR is reverting.
This is not just theoretical — running node --test work_queue_claim_scope_checks.cjs locally fails on the test updated in this very PR ("permits genuine absent-branch outputs only for a protected observer and rejects existing empty ledgers before handlers") with Missing expected exception at the for (const role of ["dispatcher", "worker"]) loop, because the dispatcher role still doesn't throw for noop messages.
Please also remove the absentDispatcher branch (and the now-unused absentDispatcher local) from normalizeRuntimeMessage to complete the revert.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e475ebb: removed the absent-dispatcher noop bypass from normalizeRuntimeMessage(). The test now passes in the full previously failing JS shard.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 64089e2
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Thanks for the feedback on #67092. I've created this new PR, which merges into #67092, to address your comment. I will work on the changes and keep this PR's description up to date as I make progress.
Original PR: #67092
Triggering comment (#67092 (comment)):