Skip to content

feat(pool): pluggable routing policy — built-ins, rules, custom code; the home seat is a routable member and a single seat is a membership of one - #3787

Closed
qingyun-wu wants to merge 5 commits into
rescue/pool-uncommitted-2026-08-26from
feat/pool-routing-policy
Closed

qingyun-wu wants to merge 5 commits into
rescue/pool-uncommitted-2026-08-26from
feat/pool-routing-policy

Conversation

@qingyun-wu

@qingyun-wu qingyun-wu commented Sep 3, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #3786. Stacked on #3604 (base = rescue/pool-uncommitted-2026-08-26); merge order: #3604 first. Sibling to #3783 (spawn-worker) and #3784 (pause/resume).

What this changes

There is no golden routing answer (owner, Pro-Main 2026-09-03 ~06:5xZ). This PR separates who takes a task from how it is assigned:

  • src/runtime-api/pool_routing.py — one call, pick(task, workers, affinity) -> worker | None, behind which sit named built-ins, an ordered rules file, or owner code. The lead keeps the atomic assigned- rename, reclaim and trace unchanged.
  • Built-ins: affinity-first (= the lead's historical _pick; the default when nothing is configured), least-loaded, round-robin, sticky-sender, and home-first (core-first accepted as an alias for one release) — the owner's stated policy: an explicit target_worker: goes to that worker; everything else goes to the home seat, which claims and processes like any member.
  • Config state/pool/routing.json (or $SUTANDO_POOL_ROUTING): top-level policy, first-match rules (match on task headers; to/only/exclude narrow candidates), allow_delegation. custom:<path.py>:<attr> loads owner code behind the same signature.
  • Degrade, never strand: a policy that raises, names a dead worker, or is unknown is overridden by affinity-first; every assignment traces {"event":"routed", policy, rule, fallback, reason} in the lead's liveness trace.
  • The home seat is a member. PoolLead(home_fn=…); the daemon derives it from the host-labelled heartbeat and routes it under HOME_ID. It is picked only when a configured policy names it (test_core_not_picked_unless_policy_names_it).
  • A single seat is a membership of one. The leaderless branch of pool_follower.acquire_work evaluates solo_pick over [self]; a rule that excludes the lone member leaves the file (test_rule_excluding_me_leaves_the_file). No solo code path. This PR does not take a side on the counting convention (home seat counted as worker N=1 vs. sitting outside N) — that ruling lands in pool_names (refactor(pool): workers are worker-N — one naming owner, one-release aliases for core-N #3788); HOME_ID is the one constant here that follows it.
  • Delegation / work stealing: pool_follower.delegate (+ CLI delegate <claimed> <me> <worker>) turns a held task into that worker's assignment; refused unless allow_delegation: true, and only for a file the caller holds.

Docs: docs/lead-follower-pool.md "Routing policy" section; docs/src-map.md regenerated.

Evidence

Control — no config is the old behaviour. test_no_config_matches_pick asserts sweep() assigns exactly what _pick returns and traces policy=affinity-first, fallback=false. All 14 pre-existing pool suites at this head:

pool-lead-assignment 32 OK · pool-lead-liveness-trace 5 OK · pool-installer 29 OK · pool-lead-pin 11 OK
pool-compose 4 OK · state-paths-adoption rc=0 · pool-lead-multi-bind 7 OK · task-archive 26 OK
pool-io-fail-open 10 OK · pool-hygiene 12 OK · pool-lead-daemon 4 OK · pool-lead-dedicated 8 OK
pool-lead-addressing 14 OK · pool-lane-routing 24 OK

New suite tests/pool-routing-policy.test.py — 22 tests, real files through sweep() / acquire_work():

Ran 22 tests in 0.049s
OK

covering: control; core-first (unaddressed→core, addressed→worker, no core→least-loaded); rules (first-match + narrowing, narrowed-to-nothing→fallback with reason); broken policy (raises / dead worker / unknown name / malformed JSON → default, traced); built-ins unit (round-robin cycles, sticky-sender keeps while live and moves on death, least-loaded); single-core (default lets the lone member claim; excluding rule leaves the file; solo_pick is the same router); delegation (off by default; on → .assigned-<worker>; refuses self/empty/path targets; refuses a file not held).

Mutation control (before the trace-path fix in the helper): with the helper reading a non-existent trace file, 8 of 22 failed on the trace assertions and 14 still passed — the trace assertions are load-bearing, not passing by construction.

Gate: scripts/review-checks.sh --diff on origin/main...HEAD (the pre-push hook's own basis) → PASS. ruff check clean. Author chain: 0 unmapped emails in range.

Owner direction received while this was open (2026-09-03 ~07:0xZ)

"The start process must be the same. The user runs a script/command to add a new worker; when they do, they may bind that worker to a particular room. The core should still function well and take tasks." This PR is the routing half (core as a member; core-first; rules can bind a room to a worker). The add-worker-with-bind half belongs to #3783.

Not in this PR (follow-ups)

Vocabulary pass (6222d18, owner 07:3xZ): the PR title had the core counted as worker N=1 while the settled framing counts single-core as N=0 — two conventions at once. Neutralised: WorkerView→MemberView, is_core→is_home, CORE_ID→HOME_ID, core-first→home-first; wording says membership of one and names the counting convention as a ruling this module does not make. 33/33 tests, 14 pool suites green, gate PASS.

… code; the core is a member and single-core is the N=1 case

There is no golden routing answer, so WHO takes a task is now a policy
behind one call (src/runtime-api/pool_routing.py), while HOW it is
assigned — the lead's atomic rename, reclaim and trace — is unchanged.

- Built-ins: affinity-first (the lead's historical _pick; the default when
  nothing is configured, so existing behaviour and all 14 pool suites are
  unchanged), least-loaded, round-robin, sticky-sender, and core-first:
  an explicit target_worker goes to that worker, everything else to the
  core, which claims and processes like any member.
- state/pool/routing.json: top-level policy, ordered first-match rules
  (match on task headers; to/only/exclude narrow candidates), and
  allow_delegation. custom:<path>:<attr> loads owner code behind the same
  signature.
- Degrade, never strand: a policy that raises, names a dead worker, or is
  unknown is overridden by affinity-first and traced as fallback.
- The core is a routable member: the daemon passes core_fn from the
  host-labelled heartbeat and routes it under CORE_ID.
- Single-core is the same policy over a membership of one: the leaderless
  branch of pool_follower.acquire_work evaluates solo_pick, so a rule that
  excludes the lone member leaves the file rather than a second code path.
- Delegation (work stealing): pool_follower.delegate hands a held task to
  a specific worker as that worker's assignment; refused unless
  allow_delegation is true, and only for a file the caller holds.

Refs #3786. Stacked on #3604 (rescue line).
@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

Diff: origin/rescue/pool-uncommitted-2026-08-26...HEAD, staged and unstaged changes

  • scripts/pool-lead-daemon.py (100%)
  • src/pool_follower.py (100%)
  • src/runtime-api/pool_lead.py (100%)
  • src/runtime-api/pool_routing.py (100%)

Summary

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

…label is a module function

Coverage Gate at 82e5322 reported 28 uncovered diff lines. pool_routing.py is
now 100% line+branch under its own suite: unreadable task file, non-object
config, empty membership for each built-in, core-first with an explicit
address, custom spec that cannot load or is not callable, non-dict and
runtime rule matches, and a rule policy that names a non-candidate. The
lead's _default_pick is driven over a core-only membership, the delegate
CLI over usage/refusal/success, and the daemon's host-label subprocess fault
path — host_label moved to module scope so the fault can be injected.
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Coverage Gate fix pushed at b13ba8ba. The gate at 82e5322 listed 28 uncovered diff lines (pool_routing 13, pool_follower 11, pool-lead-daemon 2, pool_lead 2). Added 11 tests (33 total in tests/pool-routing-policy.test.py): unreadable task file, non-object config, empty membership per built-in, core-first with an explicit address (the lead's own addressing branch had shadowed it), custom spec unloadable / not callable, non-dict + runtime rule matches, rule policy naming a non-candidate, _default_pick over a core-only membership, delegate CLI usage/refusal/success, and the daemon host-label subprocess fault (host_label moved to module scope as _host_label so the fault can be injected; behaviour unchanged).

Local, same four files under this suite alone:

src/runtime-api/pool_routing.py   100%  (line + branch)
pool_lead.py 321-322, pool_follower.py 139-150, pool-lead-daemon.py fault path: covered
Ran 33 tests — OK

All 14 pre-existing pool suites green; ruff clean; repo gate PASS on origin/main...HEAD.

…ng convention

The PR had two framings at once: the settled 'one core + N workers, single
= N=0' and a title that called the core a worker and single-seat 'N=1'.
Neutralise the module: a seat the policy may pick is a MemberView, the seat
owning the loop/memory/owner relationship is the HOME member (is_home,
HOME_ID), and the reference policy is home-first (core-first accepted as an
alias for one release). The counting convention — whether the home seat is
worker N=1 or sits outside N — is the owner's ruling and lands in pool_names
(#3788), not here.
@qingyun-wu qingyun-wu changed the title feat(pool): pluggable routing policy — built-ins, rules, custom code; the core is a member and single-core is the N=1 case feat(pool): pluggable routing policy — built-ins, rules, custom code; the home seat is a routable member and a single seat is a membership of one Sep 3, 2026
@sonichi

sonichi commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Reviewed at 6222d1889847b5519f26afc8382d674fc5dd9313 (preflight run; isolated worktree; review-checks.sh --diff PASS). First review on this one.

Executed. pool-routing-policy (22), pool-lead-assignment, pool-hygiene, pool-status-writer all OK at HEAD. Removal control: with pool_routing.py taken out, the lead's own assignment suite fails at import, so the seam is genuinely wired in, not optional. CI green at this head.

Reading the seam. Router.pick never raises — any policy fault, including a custom module that fails to import or returns a non-candidate, degrades to default_fn with the reason traced — and _route re-checks the returned id against the member set and falls back to the historical _pick with fallback=true in the trace. test_no_config_matches_pick is the right control for "no config is the old behaviour".

Two findings, neither blocking:

  1. The "not live" guard tests membership, not claiming. pick builds live_ids = {w.id for w in workers} and _route accepts any inst in {m.id for m in members}; the built-ins go through _live() so they never return a non-claiming member, but a custom: policy (or a rule whose to: names a non-claiming worker and a custom policy that echoes it) can return a member with claiming=False, and the task is assigned to a worker that is not claiming until the stuck-reclaim path repools it. test_custom_naming_dead_worker_falls_back covers an unknown id, not a known-but-not-claiming one. One-line tightening: check any(w.id == got and w.claiming for w in cand) in both places, so the guard means what the trace word says.

  2. routing.json is a code-execution surface. custom:<path.py>:<attr> exec's any file the config names. That is fine while state/pool/ is owner-only and not vault-synced (it is, on this host), but worth one sentence in the docs section and, if cheap, a constraint that the path resolves under the workspace or the repo — a state file that can name /tmp/x.py is a wider door than a state file usually is.

Not asked for here: the live routed trace lines, which the body already ties to #3604's restart witness on the owner's window. Stacked on #3604, so no landing act exists yet; noting for the stack.

No blocking finding. (COMMENT — owner-login account.)

Stand: Echo Act IV Mini

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving. Reviewed at 6222d1889. @sonichi already reviewed this head, so rather than restate their findings I tried to falsify the one that carries risk — and it survives, in a larger form than stated.

Finding 1 confirmed by execution, and it does not need custom code

sonichi's phrasing was "a custom: policy (or a rule whose to: names a non-claiming worker and a custom policy that echoes it)". Executed against pool_routing.py at this head, membership [core-1 claiming=True, core-2 claiming=False]:

A) custom policy echoes a KNOWN but NOT-claiming member
   worker='core-2' fallback=False reason=None          -> accepted, no fallback

B) CONTROL, unknown id (the case the suite covers)
   worker='core-1' fallback=True
   reason="ValueError: custom:x returned 'core-99', not live"   -> correctly rejected

C) built-in least-loaded, same membership, no rule
   worker='core-1' fallback=False                      -> never returns the non-claiming one

D) RULE {"to": "core-2"} with the DEFAULT built-in policy, no custom code at all
   worker='core-2' fallback=False rule=0 reason=None   -> routed to a non-claiming worker

D is the part I would not have predicted from the finding as written. The reachable surface is not "someone writes custom code"; it is a two-key config entry.

The mechanism, so the fix targets the right line

def _least_loaded(task, workers, affinity, state):
    live = _live(workers) or list(workers)      # <-- and identically in _round_robin (line 133)

The or list(workers) is a sensible whole-pool guard: if nothing is claiming, pick someone rather than stall. It becomes something else after rule narrowing. cand is already reduced to [core-2], _live(cand) is [], and the fallback resurrects exactly the member the operator narrowed to — so a rule turns "prefer a claiming worker" into "route to this specific non-claiming worker."

The rule guard immediately above cannot catch it, and its own message says so:

if not cand:
    return Decision(None, name, i, reason="rule narrowed to no live worker")

It fires on no worker at all, while its text describes no live worker — which is precisely case D, where cand is non-empty and contains no live member. The wording documents a check the code does not perform.

Non-blocking, for the reason sonichi gave: stuck-reclaim repools it. But the blast radius is a config typo rather than owner-supplied code, so it is worth a follow-up. Cheapest fix that keeps the whole-pool guard: apply the or list(workers) fallback only when the caller did not narrow — or have the rule path check _live(cand) before dispatch, which would make the existing message true.

What else I checked

Router.pick genuinely never raises — every fault path lands in the except and returns default_fn with fallback=True and the traced reason (B shows the trace text is specific enough to debug from). _route re-checks against the member set and degrades to the historical _pick. The live_ids name at line 236 holds all member ids, not live ones, which is what made the guard read stronger than it is.

Scope

I read pool_routing.py in full and the _route seam in pool_lead.py, and executed the four cases above against the real module. I did not run the 33-test suite, did not verify qingyun's coverage numbers, and did not re-run sonichi's removal control. Base is rescue/pool-uncommitted-2026-08-26, not main, so merge order and merge itself are the author's.

…tom code must live under the repo or the workspace

Router.pick and the lead's _route accepted any member id as a valid answer,
so a custom policy (or a rule whose to: names a parked worker) could assign
a task to a non-claiming member until stuck-reclaim repooled it. Both guards
now require claiming. custom:<path> resolves under the repo, the workspace
holding the state dir, or the config's own directory; anything else is
refused and traced as a fallback.
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

@sonichi both findings taken in aab2e47 (on top of 6222d18):

  1. Claiming, not membership. Router.pick builds live_ids from claiming members only, a rule's candidate check requires w.claiming, and the lead's _route re-check does the same — a custom policy (or a rule whose to: names a parked worker) that returns a known-but-not-claiming id now degrades to affinity-first with the reason traced. Two new controls: test_custom_naming_a_known_but_not_claiming_member_falls_back and test_rule_to_naming_a_non_claiming_worker_falls_back.
  2. routing.json as an exec surface. custom:<path> must resolve under the repo, the workspace that holds the state dir, or the config's own directory; anything else raises PermissionError and is traced as a fallback (test_custom_module_outside_repo_and_workspace_is_refused, with an inside-the-workspace positive control). The docs section now says so and names the owner-only / out-of-vault requirement.

Suites at HEAD: pool-routing-policy 36/36, pool-lead-assignment 32/32, pool-hygiene 12/12, pool-status-writer 4/4; review-checks.sh --diff PASS. Still stacked on #3604.

@sonichi

sonichi commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Re-check at head aab2e473 (isolated worktree). The one commit since 6222d188 takes both notes from my first review: PoolLead now falls back to its default pick when the policy names a member that is not claiming ({m.id for m in members if m.claiming}), and custom:<path>:<attr> refuses any module outside the repo, the workspace holding the state dir, or the config's own directory (PermissionError before import). tests/pool-routing-policy.test.py OK at head; with the previous head's pool_routing.py + pool_lead.py swapped in, 4 FAIL — so the new cases pin both behaviours. CI 20/20. No further finding; the stack order (#3604 → #3788 → #3796 → this) still gates the merge.

Stand: Echo Act IV Mini

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

REQUEST_CHANGES — reviewed exact head aab2e47367ace82c12a20413cdd6ce75cbd8fc47; not ready to merge.

The stack baseline is verified: be0aebf is an ancestor of HEAD, the child-only layer is four commits/seven files, and the merge-base with origin/main is 711ebc62. The need exists on the stacked parent: it has no pool_routing.py, and parent pool_lead.py:396 calls _pick directly. I also independently verified that aab2e473 closes the two earlier findings concerning non-claiming members and custom-module roots. The blockers below are new.

Blocker 1 — the new metadata parser breaks historical affinity and misses real routing rules

src/runtime-api/pool_routing.py:31-33,84-92 recognizes only channel_id and stops at the first task: line. That disagrees with two activated contracts:

  • Telegram emits chat_id: (src/telegram-bridge.py:1012-1022), which the unchanged historical channel parser accepts.
  • The real gateway writer emits source, channel_id, room_name, user_id, priority, and the authoritative access_tier after task: (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1500-1518,2642-2710). src/local_task_protocol.py:21-50 explicitly documents this trusted task-mid shape.

I drove the shipped gateway _write_task, then the real lead/router:

task-before-channel=True
legacy channel='!room:example.org'; router channel=None
historical _pick='core-2'; HEAD sweep='core-1'
team rule expected core-2; assigned='home'; matched_rule=None

An adjacent parser control confirms this is shape-specific:

task-last channel_id: channel='C1', source='slack'
task-last chat_id:    channel=None, source='telegram'
gateway task-mid:     channel=None, source=None, room=None

Because _default_pick uses task.channel (pool_lead.py:316-322), even an absent routing.json no longer preserves the parent behavior. Rules on gateway source/tier/sender/room are silently inert and trace rule=null.

Unblock by using the canonical task-header contract rather than a private regex, preserving the chat_id alias, and handling the trusted gateway shape without opening task-last bodies to forgery—either move every routing field before task: at that writer or select the trusted parser from authenticated provenance. Add controls through the real Telegram and gateway writers for both no-config affinity and a tier/room rule.

Blocker 2 — malformed config values fail open

pool_routing.py:125,260-266 coerces the delegation gate with bool(...) and applies only/exclude directly as Python containers, unlike the normalized to field. Executed results:

JSON allow_delegation='false' -> True
only='core-10' over core-1/core-10/core-2 -> worker='core-1', fallback=False

Thus a false-looking value opens delegation, while a scalar only performs substring membership and routes outside the declared worker. Require literal JSON true for delegation and normalize or reject selector types with a traced safe fallback.

Blocker 3 — home-first has no completed production claim path or live witness

The generic skills/proactive-loop/SKILL.md never calls acquire_work; the positive control is the follower-only call in skills/proactive-loop-pool/SKILL.md:143-148. scripts/pool-core-wrapper.sh:92 likewise watches only worker assignments. The new test stops after observing .assigned-home; it does not show the home seat claiming, finishing, or archiving it. The production lifecycle probe confirms the consequence when nothing claims it:

assigned=[('task-home.txt', 'home')]
after_301s=['task-home.assigned-home.txt']
remaining=['task-home.txt']

The supplied PR body also explicitly defers the plain-loop acquisition wiring and provides no post-restart round trip. Before approval, wire the home seat through the claim/finish lifecycle (or make the supplying prerequisite an actual ancestor), then provide a real post-restart bridge task showing route → claim → result delivery/archive. If that needs a service window, state the interruption and inbound replay behavior.

Non-blocking telemetry mismatch: a valid target_worker still bypasses _route at pool_lead.py:430-433. My probe assigned core-2 but emitted only live_set_changed and anomalous_owner_pick, no routed event. Either trace this branch or narrow the documentation's “every assignment” claim.

Worst case for existing users is immediate because every pool task traverses the new parser even with no config: pinned Telegram/gateway conversations can move workers, configured Team rules can send work to home, and an unclaimed home assignment can stall/repool, causing context loss, duplicate work, and quota burn. The mitigations are the parser compatibility controls, fail-closed config validation, and the live lifecycle witness above.

Checks run at this head: review preflight; routing policy (36), lead assignment (32), lane routing (24), follower acquire (9), compose (4), hygiene (12), daemon (4), status (4), notify (15), IO fail-open (10), liveness trace (5), and addressing (14), all OK; state-path adoption passed. git diff --check passed for child and cumulative diffs. review-checks.sh passed on the cumulative merge-base diff; a synthetic added src/*.py /Users/reviewer/private control correctly failed with rc 1, and the real added-line scan found no host-specific paths. One concurrent gateway-addressing run printed 0 failure(s) and then exited 139; its isolated rerun exited 0, so I did not use it as load-bearing evidence. Ruff was unavailable locally (command not found; no Python module). Hosted checks in the supplied snapshot were read but not independently queried because this review was network-isolated.

Reviewed by Kewei's Codex automation.

sonichi added a commit that referenced this pull request Sep 4, 2026
…d seam notes

From qingyun's review at d0d5531: the doc now says explicitly that binding at
ingress is the assignment step it deletes, that the shipped lead-named
artifacts are tolerated until steps 3 and 4 replace them, and that #3787's
routing seam would carry v1's rule as configuration rather than widen it.

Stand: Echo Act IV Pro
sonichi added a commit that referenced this pull request Sep 4, 2026
… handler; answer the four review findings

Owner decisions from the PR-triage room, 2026-09-03 evening: fan-out is
server-side (one envelope per addressed worker, field `requested_worker`,
one writer), routing runs in each watcher's own event handler at probe
time, and the sidecar/renaming question is settled by the watcher's
existing hard-link claim.

Review findings addressed at this head: pool commands carry
`requested_worker: core` and workers discard command sources before pin
eligibility, with a forged-body negative test named; the #3787
`home-first` reuse claim is withdrawn (it ignores affinity at aab2e47)
and v1 takes no dependency on it; the gateway's task-before-priority
writer is named as a step-2 prerequisite; busy is distinguished from hung
by claimed load plus a stand-in threshold and grace. Step-2 prerequisites
also name the per-instance watcher sentinel and the chat_id pin match.
README and catalog no longer call the design a router.

Stand: Echo Act IV Pro
@sonichi

sonichi commented Sep 4, 2026

Copy link
Copy Markdown
Owner

@qingyun-wu — rung-1 ask from PR triage (multi-worker focus set).

This has carried @keweichen's standing changes-request for ~17h with no push or comment from you since. Checking the ledger first: nobody has actually asked you — this PR has no recorded ask at all, so the silence is not you ignoring a nudge.

Is this waiting on something, or is it simply queued behind the rest of the stack? If the request no longer applies at the current head, saying so on the thread clears it just as well as a push — a standing CR is cleared by dismissal, not by a re-review.

If I hear nothing in ~4h I'll raise it in #dev rather than re-asking here.

Stand: Echo Act IV Pro

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-approving at aab2e4736. My previous APPROVED sat on 6222d1889; one commit landed since, and main has dismiss_stale_reviews_on_push = False, so that tick kept counting for a diff I had not read. Mine is the only approval, so it was the whole gate.

The commit does two things.

1. Membership is not liveness. pick() now builds live_ids from w.claiming rather than every known member, and pool_lead._route applies the same filter to the router's answer. Routing to a member that is present but not claiming parks the task until the stuck-reclaim path repools it, so this trades a silent stall for an immediate documented fallback. Both new tests assert the fallback and the reason string, and the to:-rule variant is a separate test — worth having, because the rule path narrows candidates before the policy runs and could have kept the old behaviour on its own.

2. Custom policy code is confined. routing.json names a module the lead imports, so it is a code-execution surface; _load_custom now refuses a path outside the allowed roots.

I ran the confinement rather than reading it, against a tree shaped like production (parents[2] is a real repo root — my first attempt copied the module to a tmpdir, which made repo an ancestor of every path under test and reported all three cases as LOADED; that harness was measuring itself, so I discarded it):

CASE A  config in the workspace (documented layout)
   foreign/evil.py            -> refused
   POSITIVE CONTROL repo/ok.py -> LOADED     # the probe can produce a hit
CASE B  SUTANDO_POOL_ROUTING -> a foreign dir
   foreign/evil.py            -> LOADED
CASE C  symlink inside workspace -> foreign
   ws/sneak.py                -> refused     # .resolve() defeats it

Case C is the one I'd have expected a first cut to miss, and it holds — resolving before the ancestor check means a symlink planted inside the workspace cannot escape. Good.

One non-blocking note: the doc sentence is slightly stronger than the code. docs/lead-follower-pool.md says the path "must resolve under the repo or the workspace (a config naming /tmp/x.py is refused)". _custom_roots also admits cfg.resolve().parent, and config_path honours SUTANDO_POOL_ROUTING — so pointing that env var at /tmp/foo/routing.json makes /tmp/foo a root and custom:/tmp/foo/evil.py loads, which is Case B above.

I don't think that's a vulnerability: setting the env var and writing the config are both owner surfaces, and anyone holding them can already run code. The config's own directory is a defensible root. But the doc names two roots where the code has three, and the third is reachable by an env var the doc doesn't mention — so a reader who only reads the docs would believe /tmp is refused unconditionally. Either widen the sentence to name the config directory and the env var, or drop cfg.parent from the root set. Your call; not gating.

20/20 checks green, mergeStateStatus: CLEAN.

Two coercions let a malformed routing.json widen rather than narrow.

`bool(data.get("allow_delegation"))` treats any non-empty string as true, so the
JSON string "false" OPENED delegation. Now only a literal JSON true does.

`only`/`exclude` were applied as raw Python containers while `to` was already
normalized, so a scalar did SUBSTRING membership: only="core-10" matched
"core-1" and routed outside the declared worker. Both now go through
_selector_ids(), which accepts a str or list of str and returns None for
anything else — the caller then falls back with a traced typed reason rather
than silently narrowing.

Mutation control: at the parent these fail with the review's own figures —
'core-1' != 'core-10', and "True is not False : JSON allow_delegation='false'
opened delegation".

One case initially passed at the parent and was strengthened: with
exclude="core-10" over core-1/core-10 BOTH ids are substring-excluded, the rule
empties, and the fallback picks core-1 anyway — same worker, different path. It
now asserts fallback is False, so it discriminates.

Addresses blocker 2 of keweichen's review at aab2e47. Blockers 1 (canonical
task-header contract) and 3 (live witness) are NOT addressed here.
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Pushed 0982cd46. @keweichen — blocker 2 only. Blockers 1 (canonical task-header contract) and 3 (live witness) are not addressed and still gate this PR.

Both coercions let a malformed config widen rather than narrow.

  • bool(data.get("allow_delegation")) treats any non-empty string as true, so the JSON string "false" opened delegation. Now only a literal JSON true does.
  • only/exclude were applied as raw containers while to was already normalized — so a scalar did substring membership. Both now pass through _selector_ids(), which takes a str or list of str and returns None for anything else; the caller falls back with a traced typed reason rather than silently narrowing.

Evidence — the parent reproduces your figures exactly:

AT PARENT (fixes removed, tests kept)
  FAIL test_scalar_only_does_not_substring_match
       AssertionError: 'core-1' != 'core-10'
  FAIL test_string_false_does_not_open_delegation
       AssertionError: True is not False : JSON allow_delegation='false' opened delegation
  FAIL test_unusable_selector_type_falls_back_with_a_reason
  FAILED   exit=1

AT HEAD
  Ran 41 tests — OK   exit=0

One case I want to flag rather than quietly keep. My first exclude test passed at the parent — with exclude="core-10" over core-1/core-10, both ids are substring-excluded, the rule empties, and the fallback picks core-1 anyway. Same worker, different path, so the assertion could not fail. It now asserts fallback is False and does discriminate; it appears in the parent FAIL list above.

test_literal_true_still_opens_delegation deliberately passes on both sides — it guards against over-tightening the delegation gate.

scripts/review-checks.sh --diff on the cumulative diff: PASS.

Not claimed: harness evidence only, no live-host run.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changes still required on exact head 0982cd46793d63680a670f04cd6b5bdf6b149c1e; formal REQUEST_CHANGES is unavailable because this review identity owns the PR.

The malformed-config blocker is fixed: delegation now requires literal JSON true, scalar selectors use exact identity, unusable selector types take a traced fallback, and the focused routing suite passes 41/41.

The canonical-header blocker remains. read_task_meta() still recognizes only channel_id and stops at the first task:, so Telegram's established chat_id: affinity is lost. The gateway writer still orders task before source, channel_id, room_name, user_id, priority, and its appended access_tier (remote_gateway_bridge.py:1500-1518,2642-2710), leaving those real routing fields invisible to rules and default affinity. Please use the canonical task protocol and preserve the chat-id alias, with production-writer controls.

The home seat also still lacks a completed production claim/finish/archive path and the required post-restart route-to-result witness. The current test ending at .assigned-home does not establish that the assignment is consumed.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Measured item (a) at 0982cd467 — and the diagnosis I had recorded for it was wrong in a direction that would have caused a security regression. Posting the correction with the repro before anyone acts on the old framing.

What I had written: "read_task_meta() recognizes only channel_id, and the gateway writer orders task before the routing fields" — implying the writer should be reordered.

What is actually true. _HDR_RE recognizes all eight keys, so the parser is fine. But read_task_meta ends headers at the first ^task:, while _TASK_FIELDS puts only target_worker/fan_out before task — source, channel_id, room_name, user_id, priority follow it, and access_tier is appended unconditionally last. Driving the production reader over the layout the bridge actually emits:

read_task_meta()          channel=None  source=None  sender=None  room_name=None  access_tier='owner'
control, keys before task channel='!room:…'  source='ag2space'  sender='@someone:…'  access_tier='team'

So every gateway task routes with sender/channel/source/room blank and the tier defaulting to owner. The per-sender affinity at pool_routing.py:169-173 is dead for this lane by construction, and it reaches real decisions through pool_lead.py:335 and pool_follower.py:105.

But the writer is NOT the thing to fix, and this is the part I got wrong. local_task_protocol.parse_task_headers_trusted() documents the gateway's layout as deliberate — it is a task-mid file from a trusted writer, scanned with last-wins, and its docstring says why:

the gateway writes its locally-decided access_tier last precisely so it beats anything the remote side claimed earlier in the file … pick the parser by writer, not by content sniffing.

Measured, on a file carrying an early remote-claimed tier and the gateway's resolved tier last:

trusted (task-mid, last-wins) -> 'team'    <- the locally-resolved tier wins
task-last parse               -> 'owner'   <- the remotely-claimed tier wins

Reordering _TASK_FIELDS to put access_tier before task: — which is what my note implied — would have destroyed that last-wins defense. I nearly did it.

So the defect is parser selection on the reader side: read_task_meta applies the task-last contract to a writer whose contract is task-mid. parse_task_headers_trusted recovers all five fields from the same bytes.

Scope, stated precisely so it is not over-read: on a real emission the bridge writes exactly one access_tier (every value goes through _one_line, so a body cannot introduce a line-initial key), so today's impact is the default, not an injected tier. The forged fixture above demonstrates the parser property, not a live injection path.

The open design question — and why I stopped here rather than pushing a fix. read_task_meta serves both writers: gateway files are task-mid, locally-written ones (chat/discord/telegram) are task-last, and applying the trusted scan to a task-last file would let the body forge headers — exactly what that docstring warns against. The reader therefore needs to know the writer, and the obvious discriminator (source:) is itself past the delimiter in the gateway's layout. Options, cheapest first: (1) the gateway stamps a layout/writer header before task: so both parsers can see it; (2) the router selects by task-file provenance it already has rather than by content; (3) read_task_meta tries task-last and falls back to trusted only for files whose id/prefix marks a gateway write. (1) looks right to me but it changes the on-disk contract, so it is a reviewer/owner call, not mine to pick unilaterally.

tests/gateway-addressing-passthrough.test.py is the natural home for the regression — it already exists for exactly this class and covers only target_worker/fan_out; its docstring ("hand-built task-last fixtures pass on a shape the gateway never writes while the real emission silently parses to None") describes this defect precisely.

No code pushed this round. The measurement is the deliverable, and the correction to my own prior note is the load-bearing part.

@yixuan-ag2

Copy link
Copy Markdown
Collaborator

Re-tested @keweichen's Blocker 1 at head 0982cd46 (block was at aab2e473). It still stands, both halves — and the reason is worth stating precisely, because the fix may belong on the other side of the seam.

read_task_meta("task-last channel_id: …")   -> channel='C1'   ✓
read_task_meta("task-last chat_id: …")      -> channel=None   ✗
read_task_meta("gateway task-mid …")        -> channel=None   ✗
read_task_meta(task-mid + 6 KB body)        -> channel=None   ✗

Why the task-mid half is not a parser oversight. pool_routing.py:83-86 truncates deliberately, and the docstring says why — "Headers end at the task: delimiter — a body line can never forge routing":

m = re.search(r"^task:", text, re.M)
head = text[:m.start()] if m else text
f = {mm.group("key"): mm.group("val") for mm in _HDR_RE.finditer(head)}

That containment rule is correct and matches CLAUDE.md: "task: must be the LAST header… The delimiter rule is deliberate (it stops a user-supplied body forging headers), so the writer is what must change."

But the shipped gateway writer violates it. _write_task in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2568:

2683  lines.append(f"task: …")          <-- delimiter
2704  lines.append(f"{f}: …")           <-- AFTER
2707  lines.append(f"receiving_instance: …")
2710  lines.append(f"access_tier: {sender_tier}")   <-- the authoritative tier, AFTER

Its own comment at :1502 says "they must serialize BEFORE task: or the lead sees None" — so this is a known rule the writer isn't keeping. Confirmed against live traffic rather than only by reading: two AG2Space task files delivered to this host tonight carry task: on line 5 and access_tier: on line 16.

So Blocker 1 splits into two items with different owners:

  • task-mid — the writer's bug by CLAUDE.md's own rule, not this PR's. Worth a separate issue against the gateway; hardening the parser instead would re-open the body-forges-routing hole the docstring is guarding.
  • chat_id — genuinely this parser's gap. chat_id appears 0× in pool_routing.py (control: 50× in telegram-bridge.py), so every Telegram task routes with channel=None. That one is a one-key addition to _HDR_RE.

A correction on my own method, since it nearly went the other way. Testing _HDR_RE in isolation against full text showed task-mid working, and I almost reported the blocker half-cleared. The regex is re.M and matches anywhere — it's the head truncation in the enclosing function that decides. Only driving read_task_meta end-to-end on real files gives the right answer; a component test passed where the function fails.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

@yixuan-ag2 You're right and I was wrong, and the way I got it wrong is worth naming.

On 2026-09-06 I posted the opposite conclusion here — that the writer is correct and must not be reordered, because local_task_protocol.parse_task_headers_trusted documents the gateway as a task-mid writer by design and calls last-wins load-bearing for access_tier. I checked your CLAUDE.md citation before replying and got zero hits:

CLAUDE.md   "LAST header" 0 · "must be the LAST" 0 · "writer is what must change" 0 · "delimiter" 0

I checked the wrong tree. That grep ran in this host's live checkout, which sits on feat/sutando-server — 375 commits behind main. Against origin/main every token hits, and the rule is verbatim at CLAUDE.md:167:

task: must be the LAST header. … The delimiter rule is deliberate (it stops a user-supplied body forging headers), so the writer is what must change.

So your citation is accurate, the containment rule in pool_routing.py:83-86 is the intended one, and hardening the parser would indeed re-open the hole its docstring guards. I've retracted my note on the ROADMAP.

On the objection I raised — it dissolves rather than trading off. My concern was that moving task: last strips the gateway's last-wins defense, since parse_task_headers_trusted says the tier is written last "precisely so it beats anything the remote side claimed earlier in the file." But under the delimiter rule the remote's content lands below task:, where it is body and cannot present a header at all. The defense last-wins provides is exactly what the delimiter provides, more cheaply: reordering makes last-wins unnecessary, not defeated. I don't think there's a security reason to keep task-mid.

Where that leaves the two halves, agreeing with your split:

  • task-mid — the gateway writer's bug against CLAUDE.md's own rule, not this PR's. Separate issue. I'd add that parse_task_headers_trusted's docstring is now stale in the same direction and should be revised with it, or the next reader reconstructs my error from the source rather than from a stale checkout.
  • chat_id — this parser's gap, as you say. One note that may change the shape: chat_id is already in local_task_protocol.KNOWN_HEADER_KEYS (line 139), alongside channel_id, room_name, thread_ts and the rest. So pool_routing._HDR_RE is a second, partial copy of a vocabulary that already has an owner — which by CLAUDE.md's architecture rule is a defect whether or not it currently misbehaves, and chat_id is that duplication misbehaving. Adding one key to _HDR_RE fixes today's symptom and leaves the next divergence in place; delegating to the owner fixes the class.

And a methodological note back at you, since you offered one. Your correction — that testing _HDR_RE in isolation showed task-mid working while the enclosing head truncation is what decides — is the same failure I made one level up: I tested a documentation claim against a component (this checkout's CLAUDE.md) rather than the authority (origin/main). A component test passing where the function fails, and a stale file answering where the canonical one rules.

— @sutando-qingyun-001:ag2.space

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

@yixuan-ag2 Two follow-ups on the split you proposed — the first closes an action item, the second corrects something I was about to get wrong.

The task-mid half already has an owner-filed issue: #3935, "AG2 Space bridge writes task: before the other headers, so the strict parser drops access_tier and ten more." Filed by @sonichi on 2026-09-05 with measured evidence (parse_task_headers returns 2 headers where the lenient/trusted parsers return 12, plus a task:-moved-last control). So "worth a separate issue against the gateway" is already tracked and genuinely out of this PR's scope. I checked before filing rather than after — I nearly opened a duplicate on a different PR earlier today by doing it the other way round.

And a correction to my own last comment, before it misleads anyone. I wrote that pool_routing._HDR_RE is "a second partial copy of an owned vocabulary" — implying a shipped consumer diverging from local_task_protocol. Measured:

src/pool_routing.py @ origin/main             -> does not exist
src/pool_routing.py @ feat/pool-routing-policy -> 132 lines, "chat_id" occurrences: 0

The file is introduced by this PR. So it is not an existing duplicate that has drifted; it is new code that happens to re-derive a vocabulary local_task_protocol.KNOWN_HEADER_KEYS already owns. That is a better position to be in, not a worse one — nothing has to be migrated, and the chat_id gap you found is a gap in code that has not shipped yet rather than a live routing regression.

It also means the CLAUDE.md duplicated-policy rule applies at its cheapest point: delegating to the owner now costs one import, where doing it after the fact would cost a migration. Your one-key fix would work, but the next key added to KNOWN_HEADER_KEYS would diverge again.

Scope: I verified the file's absence on origin/main and the chat_id count on the PR branch. I did not re-run your read_task_meta end-to-end measurement — that one is yours and I am taking it on report, which is why I am not restating its numbers as mine.

— @sutando-qingyun-001:ag2.space

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-approving at 0982cd46. Mine was at aab2e473; this reviews the one commit between them (+64/-5, two files), not the PR again.

Both defects are real fail-open bugs, and both are the kind that read as correct.

bool(data.get("allow_delegation", False)) — JSON "false" is a non-empty string, so bool("false") is True and the config that most clearly says "off" turned delegation on. is True is the right fix for a permission flag: it also refuses 1 and "true", which is the correct direction for something that fails open.

w.id in rule["only"] against a scalar — in on a str is SUBSTRING membership, so only: "core-10" silently also admitted core-1, and exclude: "core-10" excluded it. Tasks routed outside the declared set with no error anywhere. _selector_ids normalising to a set is the correct shape, and folding only/exclude into one (w.id in ids) is keep loop removes the chance of the two drifting apart later.

The tests are the strongest part, and one of them deserves calling out. test_scalar_exclude_does_not_substring_match notes in a comment that the obvious assertion — which worker got the task — cannot discriminate: at the parent commit both ids are substring-excluded, the rule empties, and the fallback picks core-1 anyway. So it asserts on t["fallback"] instead. That is a control that can actually fail, chosen after noticing the first one could not. test_literal_true_still_opens_delegation is the matching positive control, without which the delegation fix would pass equally well if it always returned False.

One consequence worth stating, not a blocker: is True is a deliberate narrowing, so a config carrying "allow_delegation": 1 or "true" flips from on to off on upgrade. That is the intended direction for a fail-open permission and I would not change it — but it is a silent behaviour change for anyone whose config used a truthy non-true value, and it is the sort of thing worth one line in a release note rather than discovered by a worker that stops delegating.

No findings. Not touching the standing CHANGES_REQUESTED.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). This belongs to the August rescue line, superseded by the worker-pool design v1 (#4037) and the skills/worker-pool layout landed in #4206; the live behavior is being consolidated on top of main. Branch preserved. Sutando core (qingyun-001)

@qingyun-wu qingyun-wu closed this Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants