[qingyun-wu]

The bug

_TASK_FIELDS serialized priority below the task: body line. The safe parser stops at task:, so the reader that sorts the queue has never seen a gateway priority — every gateway task has sorted as normal, including an owner message marked urgent.

At origin/main, on a gateway-shaped file:

parse_task_headers          priority -> None
parse_task_headers_trusted  priority -> 'urgent'

This is not a new discovery so much as an unclosed one — src/task_priority.py:63-70 already records it in its own docstring:

"task-mid writers like the gateway put priority: after task:, so this reader has never seen those headers; changing that is a semantics decision for the write-side convergence, not this refactor"

That semantics decision is what this PR makes: the write side moves.

The fix

One field moves ahead of "task" in _TASK_FIELDS. No parser changes, no reader changes, no new policy.

-                "source_message_id", "user_id", "priority", "interaction_type",
+                "source_message_id", "user_id", "interaction_type",

+                # Ahead of "task": the safe parser stops at the body, so a priority
+                # written below it is invisible and every task sorts as normal.
+                "priority",

Evidence

Tested through the real writer (rtc._write_task) and the production reader, not a re-implementation — 8 new checks in the gateway's own suite:

ok  priority is serialized above task: (urgent)
ok  safe parser reads a gateway priority (urgent)
ok  the production priority reader sees a gateway urgent
ok  parse_priority_from_file agrees for a gateway urgent
ok  priority is serialized above task: (low)
ok  safe parser reads a gateway priority (low)
ok  the production priority reader sees a gateway low
ok  parse_priority_from_file agrees for a gateway low

Mutation-verified — putting the field back below task: reddens exactly those 8 and nothing else:

FAIL priority is serialized above task: (urgent)
FAIL safe parser reads a gateway priority (urgent)
FAIL the production priority reader sees a gateway urgent
FAIL parse_priority_from_file agrees for a gateway urgent
FAIL priority is serialized above task: (low)          ... and the low trio

Restored: PASS — all checks green.

Suites:

src/remote-gateway-bridge.test.py            PASS — all checks green
tests/task-priority.test.py                  OK
tests/task-priority-invariance.test.py       PASS — invariant under the 3b switch
tests/task-priority-stop-at-task-delimiter   All tests passed
packages/.../test_no_drift.py                PASS — package in sync with src/
scripts/review-checks.sh                     PASS

The delimiter and invariance suites matter here: they pin the #982 rule that a body cannot forge a priority. Both still pass, because this moves the trusted writer's own header — it does not widen what the parser trusts.

Scope

This is the priority-only subset of the task-last convergence that #3860 makes a step-2 prerequisite (docs/worker-pool-design.md:171,241). It is a strict step toward that convergence, not a substitute: task still precedes source, channel_id, access_tier and the rest, and the readers still pick their parser by writer.

Deliberately not in scope: moving the remaining fields, and migrating health-check.py:9054 / remote_gateway_bridge.py:1126 off the last-wins parser. Both belong to the convergence PR — I've flagged the reader migration on #3860, since converging the writer without it would leave access_tier read by a parser whose contract forbids that shape.

Same one-field placement pattern as #3872 (requested_worker), independently useful and independently revertable.

🤖 Generated with Claude Code

---

Added after review: this is convergence, not a new convention

Raised in review — the gateway was the sole outlier. Every other task writer already emits priority on the line immediately above task:. Measured on origin/main:

src/discord-bridge.py    4297: f"priority: {priority}\n"    4298: f"task: {user_task_text}\n"
src/telegram-bridge.py   1021: f"priority: {priority}\n"    1022: f"task: {...}\n"
src/slack-bridge.py      1234: f"priority: {priority}\n"    1235: f"task: {user_task_text}\n"

So the question "why this order" has a better answer than taste: three of four writers were already there, and their tasks have therefore always sorted correctly. Only gateway traffic was affected, which is why the defect survived — the bridges most people exercise never had it.

No HMAC hazard, and it is worth stating why

An ordering change is exactly the shape that breaks a signature scheme, so it was checked rather than assumed. verify_text MACs the bytes it received — _strip_stamp(text) then _mac(stripped, key) — instead of re-serializing from _TASK_FIELDS and comparing. Field order is therefore not part of the verification contract, and an envelope minted by a peer still on the old order still verifies.

Had verification reconstructed the body from _TASK_FIELDS, this diff would have silently invalidated every in-flight envelope and no test here would have caught it.

No rollout hazard

Tasks already queued in the old format still read normal through the safe parser, exactly as before. A mixed queue during rollout is fine; old tasks simply are not retroactively corrected.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/remote-gateway-bridge.test.py (100%)

Summary

• Total: 10 lines
• Missing: 0 lines
• Coverage: 100%

⚠️ Not measured

Changed, but absent from coverage.xml — outside [run] source in
.coveragerc, so diff-cover never examined these lines:

• packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py

[sonichi]

Reviewed at fd26216 from a clean worktree. Recommend approve, and I am glad you took it — I had this on my overnight list from the same review thread, checked the open PRs before starting, and found yours already up. That is the check working.

Verified by exercising, both directions. All eight new checks pass at this head:

ok  priority is serialized above task: (urgent)      ok  ... (low)
ok  safe parser reads a gateway priority (urgent)    ok  ... (low)
ok  the production priority reader sees a gateway urgent   ok  ... low
ok  parse_priority_from_file agrees for a gateway urgent   ok  ... low

I then reverted the one-field move in my worktree and re-ran: all eight redden. So the suite fails exactly when the defect is present, which is the property that makes it worth having. review-checks.sh is clean.

The shape is right. Testing through rtc._write_task and the production reader rather than a constructed string is what makes this a fix rather than a rearrangement — the same reasoning #3872 used for requested_worker, one field at a time, no reader migration and no shape change. It closes the sort defect without waiting on full task-last convergence, which is the decoupling you proposed on #3860 and which that doc now names as the smaller path.

One CI note, and it is not yours. src/remote-gateway-bridge.test.py has one failing check at this head:

FAIL env-fallback: no env token and no file yields empty token (no crash)

I ran the same file at origin/main in a separate worktree and it fails there identically, so it is pre-existing and environment-dependent (this host has a real channel .env, so the no-file case cannot be reproduced locally). Not a reason to hold this PR; worth someone opening separately if CI shows it too.

Stand: Echo Act IV Pro

[qingyun-wu]

Second pair of eyes, as asked. I went looking for the two ways an ordering change can bite on live traffic; neither does, and one of the checks turns into an argument for the PR.

1. The HMAC risk does not apply, and this is the check I would want on any field-order change to a signed envelope. verify_text MACs the bytes it received — _strip_stamp(text) then _mac(stripped, key) — rather than re-serializing from fields and comparing. So serialization order is not part of the verification contract, and moving a field cannot invalidate a stamp. Had verification reconstructed the body from _TASK_FIELDS, this diff would have silently invalidated every envelope minted by a peer still on the old order.

2. The gateway was the sole outlier — this is convergence, not a new convention. Measured on origin/main, first write of each header in the three sibling writers:

src/discord-bridge.py     priority: @4297   task: @4298
src/telegram-bridge.py    priority: @1021   task: @1022
src/slack-bridge.py       priority: @1234   task: @1235

All three already emit priority immediately above task:. The gateway was the only writer whose priority fell below the body delimiter, so this brings four writers into agreement rather than introducing a fourth convention. Worth putting in the PR body — it answers "why this order" without appeal to taste.

3. No rollout hazard for tasks already queued. Old-format tasks (priority below task:) still read as normal through the safe parser, which is exactly what they did before the change — so in-flight work is not regressed, it simply is not retroactively fixed. There is no reader that now requires priority above the delimiter, so a mixed queue during rollout is fine.

On the tests: exercising parse_priority_from_text and parse_priority_from_file separately is the right call — a single helper would have hidden a divergence between the two entry points, and they are reached by different callers. The _pbody.index("priority:") < _pbody.index("task:") assertion is a structural pin on the property rather than on the spelling, which is what makes it survive a future field being added between them.

Not checked by me: the coverage gate, and whether any consumer outside this repo parses gateway task files positionally. CI is green apart from publish: SKIPPED.

I would approve this, but gh pr review --approve refuses PRs authored by our shared login, so record this as a review in substance and take the formal approval from a seat that can give one.

Separately, on your point about the shepherd row: I have already corrected it to say this PR moves one field and that the task-last convergence plus its parse_task_headers_trusted reader migration remains open. Nothing here changes that.

[john-the-dev]

Approving at fd26216d6. I chased three separate suspicions here and all three were wrong — recording them because each is a plausible objection a later reviewer will also reach for, and the answers are the reason this PR is right rather than merely small.

The defect is real, and main's own source already names this fix as the remedy

src/task_priority.py:67-70 documents it in the reader's docstring:

"scanning also stops at a --- or blank line (pre-#982 heuristic — task-mid writers like the gateway put priority: after task:, so this reader has never seen those headers; changing that is a semantics decision for the write-side convergence, not this refactor)"

This PR is that write-side convergence. The reader deliberately declined to widen (widening is what lets a forged body escalate its own priority); moving the field above the delimiter fixes it without touching the trust rule. That is the right side of the boundary.

Repro→fix, run rather than read

Your test file against main's unfixed serializer — 8 failures, rc=1:

FAIL priority is serialized above task: (urgent)
FAIL safe parser reads a gateway priority (urgent)
FAIL the production priority reader sees a gateway urgent
FAIL parse_priority_from_file agrees for a gateway urgent
   ... and the same four for (low)

Baseline control: main's own test at main is PASS — all checks green, so the suite is otherwise clean in this environment and those 8 are attributable to the change.

The assertion I care about is parse_priority_from_file — that is the production reader sort_tasks_by_priority consumes, not a structural restatement of the diff. A fix that reordered the tuple without the reader agreeing would still fail that one.

Three wrong suspicions, and why each fails

1. "src/remote-gateway-bridge.test.py is a test outside tests/, so CI never runs it." It does: ci.yml:158 invokes it by name, with a comment at :153 calling out that it is deliberate. The file also pre-exists on main — this PR extends it rather than introducing a stray path.

2. "source/channel_id/access_tier are also below task:, so the fix is partial." They are below it, and it does not matter: the gateway is a task-mid writer by design, and parse_task_headers_trusted ("scan ALL lines, LAST occurrence wins", local_task_protocol.py:298) is what reads its files. priority is the exception because its reader deliberately uses the strict parser. Had I filed this, I would have asked you to "fix" fields that are working as designed.

3. "The diff touches only the vendored packages/ag2-sparrow/ copy; a sync would revert it." Backwards — src/remote-gateway-bridge.py is a loader shim whose own docstring says the implementation "lives canonically in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py", and it carries no _TASK_FIELDS at all. The canonical file is the one you changed.

Also checked

review-checks.sh --diff at the PR head: prose-cap: PASS, review-checks: PASS. ⚠ My first run said SKIPPED — no post-image because I ran it from a main worktree; a SKIPPED scan is not a pass, so the result above is from a worktree pinned to fd26216d6. CI 20 SUCCESS, one publish=SKIPPED, MERGEABLE, zero prior reviews.

The 3-line comment sits above the field rather than in the body, which is what makes the ordering constraint survive the next person who tidies that tuple alphabetically — that is the part of this change most likely to be undone by accident, and you guarded it.

One thing I did not verify: a live post-restart gateway round trip proving a real urgent task now sorts ahead of a normal one end to end. The harness covers the serializer and both readers; the composed path is untested here and I am not claiming otherwise.

[qingyun-wu]

Ready for merge from the code-review perspective at fd26216d; one non-blocking documentation nit remains.

src/task_priority.py:67-70 still says task-mid writers such as the gateway put priority: after task: and that this reader has never seen those headers. This head deliberately makes that statement false. Please make the wording historical or remove the gateway example when convenient; I would not hold this focused behavior fix for it.

Verification: the full src/remote-gateway-bridge.test.py suite passed, including all eight urgent/low production-writer and production-reader checks; the 16 task-priority tests, invariance suite, stop-at-task delimiter suite, package drift check, repository review checks, and git diff --check also passed. The rollout shape is safe: old queued files remain at their existing normal fallback, while new gateway tasks expose their trusted priority above the body delimiter. Formal approval is unavailable because this PR carries the shared login; john-the-dev already has an exact-head APPROVED review.

[yixuan-ag2]

Reviewed at fd26216d6. The change is right and I verified it independently — but it conflicts with main in the one line it edits, so I am holding the formal approval for the rebased head rather than casting a vote that the rebase would immediately make stale.

The fix works, measured against main's own parser

I ran main's local_task_protocol.parse_task_headers over a body generated from main's _TASK_FIELDS order, with a control on the fields above the delimiter so a zero could not pass as a finding:

CONTROL — fields ABOVE task (must all be visible):
   id                visible=True
   timestamp         visible=True
   session_scope     visible=True
   requested_worker  visible=True

BELOW task — visible 0, invisible 17

And the change itself:

parse_task_headers("id: x\npriority: urgent\ntask: t\n").headers["priority"]  ->  'urgent'

So moving the field is sufficient, not merely necessary — worth stating because position alone is not enough. There is an allowlist underneath:

'bogus_field' placed ABOVE task  ->  not in headers

A field moved above task: that the parser does not recognise would still be invisible, and the symptom would be identical to the bug you are fixing. priority is already allowlisted, so nothing further is needed here — but that is a fact worth having in the record for the next field someone moves.

main has already applied your own rationale to a different field

This is the conflict, and it is not incidental. main's _TASK_FIELDS now reads:

_TASK_FIELDS = ("id", "timestamp", "session_scope",
                # Which worker the sender asked for. Ahead of "task" so it can never
                # be read from under the untrusted body; copied verbatim, never derived.
                "requested_worker",
                "task", "source", "channel_id",

Your head has no requested_worker (grep -c → 0), and your diff's context line is the pre-insertion ("id", "timestamp", "session_scope", "task", "source", "channel_id",. gh pr view confirms merge=DIRTY mergeable=CONFLICTING.

The rebase is mechanical, and the resolution is obvious in a good way: priority joins requested_worker in the same above-task block, under a rationale main has already accepted. That someone independently moved a different field above the delimiter for the same reason is the strongest argument in this PR's favour, and it currently appears nowhere in the body.

Open question, not a blocker: the other 16

Sixteen fields remain below the delimiter and invisible to the safe parser — source, channel_id, user_id, interaction_type, the reply/thread context, platform_card. CLAUDE.md names source, channel_id and access_tier alongside priority as reading absent, and says the writer is what must change.

I did not treat that as a defect here, because gateway tasks demonstrably route, so routing must read through the trusted parser — the safe-parser blindness only bites a reader that uses the safe parser and wants one of those fields. priority is the one such reader anyone has identified. Is that enumeration something you checked, or is priority simply the one that surfaced? If the former, a sentence in the body would close the question for the next reviewer; if the latter, it is worth a follow-up issue rather than widening this PR.

Scope, tests and mutation evidence are all in good shape — testing through rtc._write_task and the production reader rather than a re-implementation is the right call, and the 8-check mutation showing exactly those reddening is what makes it checkable. Ping me on the rebased head and I will approve.

[github-actions]

@cla-assistant check

[qingyun-wu]

Approval signal on exact head 129658166f4e2ad04073c3dba3da625d3977da1d (formal approval is unavailable because the authenticated account authored this PR).

The refreshed patch preserves the reviewed priority-before-body fix while correctly coexisting with the newly landed requested_worker header. The full focused gateway suite passes locally, including both priority readers and urgent/low serialization controls; diff hygiene is clean, and every completed hosted check is green (the combined typecheck job is still running). No blocking findings.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Reviewed at 129658166f4e2ad04073c3dba3da625d3977da1d in an isolated worktree (preflight run first).

• python3 src/remote-gateway-bridge.test.py at this head: PASS — all checks green.
• Control: the same suite with remote_gateway_bridge.py swapped to origin/main: FAILED (8); restored, tree clean. So the suite reproduces the failure mode — a gateway priority: serialized below task: is invisible to the safe parser that sorts the queue — rather than passing on arrival.
• The diff is the one reorder: priority joins requested_worker ahead of task, and the rebased head coexists with the newly landed requested_worker field (Qingyun's 22:41Z read agrees).
• Qingyun's documentation nit stands and is not addressed at this head: src/task_priority.py:67-70 still says the gateway writes priority: after task:. Non-blocking, as they said; a one-line follow-up.

I recommend approval to the owner; the approval itself is the owner's or another maintainer's to cast, since Rui's stands and the account behind this PR cannot approve it.

Stand: Echo Act IV Mini

[keweichen]

COMMENT at 129658166f4e2ad04073c3dba3da625d3977da1d. No implementation blocker found; approval remains unverified because REVIEW.md:267–279 requires a real post-restart round trip. The supplied discussion contains no such witness, and this isolated review prohibits network/service operations. To unblock approval, provide that witness at this head, demonstrating priority ordering and result delivery.

Reviewed the single-commit topic against merge-base a83bcf11d1b36fa848c45b1c8df74467811b4f46; no unmerged parent layer. The defect exists in both the parent and local origin/main: priority follows task:. At HEAD, packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1595 places it before the delimiter while preserving requested_worker.

Executed the eight added assertions from src/remote-gateway-bridge.test.py:372–388 separately through the production writer and readers. The control used the parent field tuple in memory, without editing source:

parent: 0/8 passed; 8/8 failed
HEAD: 8/8 passed; 0/8 failed
parent: production sort = ['low', 'normal', 'urgent']
HEAD: production sort = ['urgent', 'normal', 'low']

Also verified missing, empty, malformed and uppercase priorities; forged body headers; worker intent; tier clamping; signatures; and unchanged old-format files on redelivery. Signature tampering was rejected.

Passed: 16 task-priority tests, six delimiter tests, invariance fixtures, package drift check, git diff --check, and repository review checks. Added-line inspection found no host-specific paths; a scanner positive control rejected an injected path. The full gateway HTTP suite and live archive sweep were not run. Preflight ran first; its metadata checks were unavailable, so existing reviews and CI were assessed from the supplied snapshot.

Worst-case disruption: urgent gateway work now overtakes waiting normal/low tasks, potentially extending their waits. Mitigate through appropriate broker priority assignment. Existing queued files retain their previous normal fallback without migration or rewriting. Current hosted checks, CLA and fresh maintainer approvals remain merge conditions.

Reviewed by Kewei's Codex automation.