Skip to content

[WIP] Restore classifier cost attribution in routed-run audits #67230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Copilot wants to merge 3 commits into from
Closed

[WIP] Restore classifier cost attribution in routed-run audits #67230

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0de1695
Initial plan
Copilot Oct 9, 2026
64089e2
Revert unrelated dispatcher no-op change
Copilot Oct 9, 2026
e475ebb
Reject unassigned dispatcher noops during normalization
Copilot Oct 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions actions/setup/js/safe_output_handler_manager.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,16 +1001,7 @@ function sortMessagesByTemporaryIdDependencies(messages) {
* @returns {Promise<{success: boolean, results: Array<any>, temporaryIdMap: Object, artifactUrlMap: Map<string, string>, outputsWithUnresolvedIds: Array<any>, missings: Object, codePushFailures: Array<{type: string, error: string}>}>}
*/
async function processMessages(messageHandlers, messages, onItemCreated = null) {
const claimScope = readClaimScopeContext();
const absentBranchDispatcherNoop =
claimScope?.assignment === null &&
claimScope.snapshot?.sha === null &&
claimScope.snapshot.transactionLog === "" &&
claimScope.snapshot.worker === null &&
process.env.GH_AW_WORK_QUEUE_ROLE === "dispatcher" &&
messages.length > 0 &&
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");

@github-actions github-actions Bot Oct 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
messages = messages.map(message => {
Expand Down
1 change: 0 additions & 1 deletion actions/setup/js/safe_outputs_handlers.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3430,7 +3430,6 @@ function createHandlers(server, appendSafeOutput, config = {}) {
const message = normalizeRuntimeMessage(type ? { ...(args[0] || {}), type } : args[0] || {});
if (type && !Object.hasOwn(args[0] || {}, "type")) delete message.type;
if (executionHandle) return handler(message, ...args.slice(1));
if (scope && scope.assignment === null && message.type === "noop") return handler(message, ...args.slice(1));
return withClaimExecution({ ...scope, claim_handle: message.claim_handle }, () => handler(message, ...args.slice(1)));
};
return Object.fromEntries(
Expand Down
1 change: 0 additions & 1 deletion actions/setup/js/work_queue_claim_scope.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ async function assertClaimAuthorized(message, options = {}) {
const execution = claimExecution.getStore();

@github-actions github-actions Bot Oct 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e475ebb: removed the absent-dispatcher noop bypass from normalizeRuntimeMessage(). The test now passes in the full previously failing JS shard.

const scope = execution || readClaimScopeContext();
if (!scope) return normalized;
if (scope.assignment === null && normalized.type === "noop") return normalized;
if (options.effect === true) {
if (execution?.closedEffectChannel) throw scopeError("Claim write channel is closed for independent delivery verification");
if (resourceExecution.getStore()?.verification) throw scopeError("Claim resource verification is read-only");
Expand Down
24 changes: 8 additions & 16 deletions actions/setup/js/work_queue_claim_scope_checks.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@
}
});

it("permits protected observer outputs and dispatcher no-op only for a genuine absent branch", async () => {
it("permits genuine absent-branch outputs only for a protected observer and rejects existing empty ledgers before handlers", async () => {
const { queueFixture } = require("./work_queue_lifecycle.test_helpers.cjs");
const { serializeTransactionLog } = require("./work_queue_replay.cjs");
const fixture = queueFixture({ granted: false });
Expand Down Expand Up @@ -319,21 +319,13 @@
for (const message of messages) assert.throws(() => scope.normalizeRuntimeMessage(message), /protected compiler role/);
await assert.rejects(manager.processMessages(handlers, messages), /protected compiler role/);
assert.equal(writes, 3);
process.env.GH_AW_WORK_QUEUE_ROLE = "dispatcher";
write({ ...snapshot, role: "dispatcher" });
assert.deepEqual(scope.normalizeRuntimeMessage(messages[0]), messages[0]);
assert.throws(() => scope.normalizeRuntimeMessage(messages[1]), /unassigned dispatcher/);
const dispatcherNoop = await manager.processMessages(handlers, [messages[0]]);
assert.equal(dispatcherNoop.success, true);
assert.equal(dispatcherNoop.results[0].success, true);
await assert.rejects(manager.processMessages(handlers, [messages[1]]), /trusted per-Claim handler context/);
assert.equal(writes, 3);

process.env.GH_AW_WORK_QUEUE_ROLE = "worker";
write({ ...snapshot, role: "worker" });
for (const message of messages) assert.throws(() => scope.normalizeRuntimeMessage(message));
await assert.rejects(manager.processMessages(handlers, messages));
assert.equal(writes, 3);
for (const role of ["dispatcher", "worker"]) {
process.env.GH_AW_WORK_QUEUE_ROLE = role;
write({ ...snapshot, role });
for (const message of messages) assert.throws(() => scope.normalizeRuntimeMessage(message));

Check failure on line 325 in actions/setup/js/work_queue_claim_scope_checks.cjs

View workflow job for this annotation

GitHub Actions / JS Tests (shard 1/4)

work_queue_delivery.test.cjs > universal immutable Claim-scoped safe outputs > permits genuine absent-branch outputs only for a protected observer and rejects existing empty ledgers before handlers 

AssertionError: Missing expected exception.

simple

 ❯ work_queue_claim_scope_checks.cjs:325:50

Check failure on line 325 in actions/setup/js/work_queue_claim_scope_checks.cjs

View workflow job for this annotation

GitHub Actions / JS Tests (shard 1/4)

work_queue_delivery.test.cjs > universal immutable Claim-scoped safe outputs > permits genuine absent-branch outputs only for a protected observer and rejects existing empty ledgers before handlers 

AssertionError: Missing expected exception.

simple

 ❯ work_queue_claim_scope_checks.cjs:325:50
await assert.rejects(manager.processMessages(handlers, messages));
assert.equal(writes, 3);
}
} finally {
for (const key of keys) {
if (previous[key] === undefined) delete process.env[key];
Expand Down
Loading