Repository navigation
Add declarative sorting to work queue recommendations - #65495
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The MCP schema permits inputs that the runtime rejects, creating an inconsistent public tool contract.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds configurable ordering to work queue recommendations while preserving oldest-first defaults and claim semantics.
Changes:
- Implements bounded, multi-key sorting by ID, enqueue time, or ID length.
- Adds sorting tests and documents read-only behavior.
| File | Description |
|---|---|
actions/setup/js/work_queue_mcp_server.cjs |
Implements sorting and exposes its MCP schema. |
actions/setup/js/work_queue_mcp_server.test.cjs |
Tests sorting and runtime validation. |
specs/work-queue/README.md |
Defines sorting semantics. |
.github/aw/work-queue.md |
Documents dispatcher usage. |
| properties: { | ||
| work: { type: "string", minLength: 1, description: "Optional Work identifier to read." }, | ||
| sort: { | ||
| type: "array", |
There was a problem hiding this comment.
Added minItems: 1 and maxItems: 4 to the sort array schema, matching runtime validation. The schema and runtime now also use the key enum. Fixed in f454431.
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review. Review found a blocking schema/runtime mismatch in work_queue_read sort handling, but safeoutputs review comment/review submission was denied by the environment, so no PR write was emitted.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One simplification opportunity: keep sort keys declarative without introducing an expression AST.
net: -28 lines possible.
Generated by ✂️ Ponytail Reviewer for #65495 · codex · gpt56 · 9.61 AIC · ⌖ 6.4 AIC · ⊞ 13.4K
Comment /ponytail to run again
| }); | ||
| } | ||
|
|
||
| function validateSort(sort) { |
There was a problem hiding this comment.
L38-63: yagni: mini expression language for three fixed sort keys. Use a key enum (id, enqueued, id_length) plus direction, no evaluator.
There was a problem hiding this comment.
Replaced the expression objects with a key enum (id, enqueued, id_length) plus direction; the evaluator is removed. Fixed in f454431.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design on the new work_queue_read sort feature. The implementation is solid and well-tested (multi-operator sort, tie-breaking, validation edge cases); leaving two non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Duplicated enqueued lookup:
readWorkQueueStatecomputes the Work→enqueuedmapping twice via two separate scans ofsnapshot.projection.transactions(once for sorting, once per-item for the response) — a maintainability/seam risk worth consolidating into one Map. - Unaddressed prior thread: the existing Copilot review comment about the
sortJSON Schema lackingminItems/maxItems(so schema advertises unbounded arraysvalidateSortwould reject) is still open — reinforced with a concrete fix suggestion. - Untested Unicode edge case: the
lengthsort op deliberately uses[...value].lengthfor code-point counting, but no test proves this over a plain.length, so a future refactor could silently regress astral-character handling.
Positive Highlights
- ✅ Thorough validation (
validateSort) with a strict allow-list of operators/fields, rejecting malformed shapes with clearTypeErrors. - ✅ Good regression coverage for multi-operator sort, tie-breaking to oldest-first, and the full matrix of invalid
sortinputs. - ✅ Documentation (
work-queue.md,specs/work-queue/README.md) clearly states sorting is read-only and doesn't affect Claim authority — consistent with the rest of the spec's vocabulary.
Note: triage used the pr-triage agent successfully (change_type: new_feature).
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 53.5 AIC · ⌖ 13.6 AIC · ⊞ 10K
Comment /matt to run again
| const workIds = args.work === undefined ? [...snapshot.projection.available, ...Object.keys(snapshot.projection.work).filter(work => !snapshot.projection.available.includes(work))] : [args.work]; | ||
| const available = [...snapshot.projection.available]; | ||
| if (args.sort) { | ||
| const metadata = new Map(snapshot.projection.transactions.filter(transaction => transaction.kind === "Work").map(transaction => [transaction.work, transaction.enqueued ?? 0])); |
There was a problem hiding this comment.
[/codebase-design] The metadata Map rebuilds the Work→enqueued lookup from snapshot.projection.transactions, duplicating the same .find(transaction => transaction.kind === "Work" && ...) logic already used per-item at line 101 to populate works[].enqueued. Two independent code paths computing the same value from the same source is a seam risk: if one is changed (e.g. to handle duplicate Work facts or a future metadata shape) without the other, sort order and displayed enqueued can silently diverge.
💡 Suggested consolidation
Build a single enqueuedByWork Map once per readWorkQueueState call (not just when args.sort is set) and reuse it both for sorting and for populating works[].enqueued, e.g.:
const enqueuedByWork = new Map(
snapshot.projection.transactions
.filter(t => t.kind === "Work")
.map(t => [t.work, t.enqueued ?? 0])
);
// ...
enqueued: enqueuedByWork.get(work) ?? 0,This removes the duplicated .find() scans (O(n) per item) and guarantees the sort and the reported value can never disagree.
@copilot please address this.
There was a problem hiding this comment.
A single first-seen enqueuedByWork map now supplies both sort values and each Work response's enqueued field, preserving the existing first-match behavior. Fixed in f454431.
| } | ||
| } | ||
|
|
||
| function evaluateSortExpression(work, expression) { |
There was a problem hiding this comment.
[/tdd] evaluateSortExpression's length op uses [...value].length (correctly counting Unicode code points instead of UTF-16 code units), which is a deliberate and non-obvious choice — but no test exercises an id containing a surrogate pair (e.g. an emoji) to prove the spread actually matters here versus a plain .length.
💡 Suggested test
it("length expression counts code points, not UTF-16 units", () => {
const snapshot = loadWorkQueueSnapshot(
writeSnapshot({
version: 2, sha: "head",
transactionLog: serializeTransactionLog([
createWorkTransaction("\u{1F600}", 10), // 1 code point, 2 UTF-16 units
createWorkTransaction("ab", 10),
]),
worker: null,
})
);
const byLength = [{ expression: { op: "length", field: "id" }, direction: "asc" }];
expect(readWorkQueueState(snapshot, { sort: byLength }).works.map(i => i.id)).toEqual(["\u{1F600}", "ab"]);
});Without this, a future refactor to value.length would silently break astral-character identities and nothing would catch it.
@copilot please address this.
There was a problem hiding this comment.
Added a regression test sorting an emoji ID against a two-character ASCII ID by id_length; it verifies counting Unicode code points rather than UTF-16 units. Fixed in f454431.
|
@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: f1bb797
|
…uery-language 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>
Merged the latest |
…automation.md dispatch-workflow gained a work_queue selector (actions: write dispatch of a Claim to a tools.work-queue worker) across #65443/#65494/#65495, but its own doc entry never mentioned it - only work-queue.md did. Add a one-line cross-reference so readers of the dispatch-workflow section discover it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎉 This pull request is included in a new release. Release: |
work_queue_readrecommends the oldest available Work, limiting dispatchers that need a different queue order. This change adds optional sort operators while preserving the existing Work selector and oldest-first default.id,enqueued, or ID length. Multiple operators break ties in order; remaining ties retain oldest-first order.next_workrecommendation only. It does not change Claim selection or dispatch authority.For example, to recommend the newest available Work:
{"sort":[{"expression":{"op":"field","field":"enqueued"},"direction":"desc"}]}