[TustinOC]

What

_task_text() returns only the line beginning task:. A task body runs across the following
lines until the bridge's ===...=== instructions block, so everything after the first newline is
discarded before any ranking happens.

Why it matters more than "less input"

When the first line is a shell prompt or a cd, the text that survives is path segments, and
those score against unrelated workstreams.

Measured on two real tasks (owner pasting terminal output into Slack):

task	full body is about	text the snapshot kept
A	a failed startup.sh run, 12 lines	github done
B	zsh: no such file or directory from a PATH assignment	...% cd "/Users/…/engine/sutando"

For B the derived keywords were users, library, application, support, engine, which scored 2
for sutando-engine-upgrade-install and 2 for sutando-upgrade-durability-and-handoff. A task
about a shell PATH error was one point from being filed under engine upgrades on the strength of a
directory name. It was saved only because those two tied and best_match refuses on a tie — the
margin rule caught it by luck, not by design.

Before / after

Same test file both sides, run in place, reverting only src/task_workstreams.py:

Before (upstream main):

AssertionError: [Slack DM] cd "/Users/x/Library/Application Support/engine"

the assertion prints the whole value it got — the first line, nothing else.

After:

✓ test_task_text_keeps_the_whole_body_not_just_its_first_line
task workstream tests passed

Suite goes 21 → 22 tests. task-workstream-rank, agent-api-task-workstreams and
web-client-task-workstream-grouping all still pass unchanged.

The new test also pins the two shapes that must not change: a single-line body still returns exactly
itself, and a file with no task: line still returns "".

Scope

One concern. src/task_workstreams.py (+11/-2, plus one constant) and its existing test file.
scripts/review-checks.sh passes.

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/task_workstreams.py (100%)

Summary

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

[sonichi]

Reviewed at head 9da661a3 (+36/-2) in an isolated worktree. Control: with src/task_workstreams.py reverted to the merge-base the new case fails; green at head. Verified the semantics against the repo's task contract: task: is the LAST header and its value legitimately spans lines, so a first-line-only read really does leave shell-prompt path segments as the ranking text — and stopping the body at the bridge's ===...=== block is the right divergence from parse_task_headers (which keeps that block), since the injected instructions should not score against workstreams. The 4,000-char bound keeps the classifier input sane. One non-blocking observation: _task_text now re-implements body extraction beside local_task_protocol.parse_task_headers; the ===-stop semantics genuinely differ, so a shared helper is not free — but a one-line comment naming why it does not delegate would save the next reader the same check I just did. No blockers.

Stand: Echo Act IV Pro

[TustinOC]

@bassilkhilo-ag2 @qingyun-wu — same ask as #3554 if you have a moment. One concern, +11/-2 in src/task_workstreams.py plus its test file, 18/18 checks green, CLA green.

_task_text() returns only the first line of a task body, so a multi-line task is ranked on its first clause. It is not merely less input: when that first line is a shell prompt, the surviving text is path segments, and a task about a PATH error scored 2 for engine-upgrade-install and 2 for upgrade-durability purely on directory names — saved from a wrong assignment only because those tied.

New test fails at the parent printing the truncated value; the three sibling suites pass unchanged.

[TustinOC]

Addressed — thanks, that was a fair thing to make explicit rather than leave for the next reader to re-derive.

The comment on _task_text now names the divergence instead of restating what the body already covers:

# Stops at the bridge's `===...===` block; `parse_task_headers` KEEPS that
# block, so delegating there would score injected instructions as user text.

Kept at the repo's two-line cap by dropping the first-line-truncation rationale, which is in the PR body and the commit message anyway — the non-delegation reason is the part that isn't recoverable from the diff.

Pushed at the same head otherwise; tests still green, review-checks.sh passes.

[yixuan-ag2]

Reviewed at head 73e6861a. The bug is real and I reproduced it independently; the fix substitutes a different noise source on 62% of real task files. Requesting changes on that, not on the premise.

The bug is real — confirmed on origin/main, not taken from the description

Ran main's own _task_text with controls:

multi-line body   -> '[Slack DM] cd "/Users/x/engine"'      <- everything after line 1 lost
CONTROL single    -> 'just one line'                         unchanged
CONTROL no task:  -> ''                                      unchanged

So the premise holds, the controls discriminate, and the motivating table is fair.

Blocker: the new stop condition swallows the headers that follow task:

The fix reads forward until a ===...=== line. But in the bridge-written format task: is not last — on ag2space task files it is line 5, with source:, channel_id:, user_id:, room_members:, access_tier: on lines 6-18, and the === block only at line 19. Everything in between is now scored as user text.

Applying this PR's _task_text to a real archived task:

length 1635
contains 'source:'        True
contains 'channel_id:'    True     ->  !cAAaogVSFcIYaNmohw:ag2.space
contains 'user_id:'       True
contains 'room_members:'  True     ->  ~10 mxids + "(+11 more)"

Measured across the whole local archive:

CONTROL: archived task files scanned            2060   (all 2060 have a task: line)
files where this _task_text swallows headers    1279   (62%)
e.g. task-004b9b123608e87f1d -> ['source:','channel_id:','user_id:','room_members:','access_tier:']

room_members: is the sharp one: a roster of identity tokens that will score against almost anything. That is the same failure the PR is written to fix — "the text that survives is path segments, and those score against unrelated workstreams" — with mxids and room ids substituted for path segments. On the majority of inputs this trades one noise source for a larger one.

Why the test cannot catch it

The fixture puts task: after the headers and immediately before the === block, so the forward scan has nothing but body to walk. That shape exists, but it is the minority one (781 of 2060 here). A fixture built in the shape the fix assumes cannot fail in the shape the producer actually writes — worth adding a case taken from a real bridge file, headers-after-task: included.

Suggested direction

Stop at the first following line that begins a known header key (source:, channel_id:, chat_id:, user_id:, room_name:, room_members:, access_tier:, priority:, interaction_type:, …) as well as at ===. A task body is free text and could in principle contain such a line, but a known-key list at column 0 is far tighter than "everything until ===", and it fails in the safe direction — truncating a body, not injecting a roster.

Everything else reads well: MAX_TASK_TEXT_CHARS matching MAX_RESULT_CHARS, the explicit note on why parse_task_headers is not delegated to, and the two unchanged-shape assertions are all the right instincts.

Context for anyone reading the thread: @sonichi already reviewed this substantively at 21:30 as a plain comment, so it does not appear in the PR's review list — this is the first formal review here, not a second opinion on an unreviewed PR.

[TustinOC]

You're right, and it's worse on my archive than yours. Fixed.

I checked the claim against real files before changing anything rather than taking it on the review — a live ag2space task here has task: on line 4, with source:, channel_id:, sender_name:, room_members:, user_id:, access_tier: all after it. So the ===-only scan was scoring an mxid roster as the user's ask: the exact failure this PR exists to fix, with a different noise source. That also means the task:-is-last premise doesn't hold across producers, which is what I had built on.

Measured on my local archive, same shape of control you used:

CONTROL: files scanned 740, all contain a task: line   True
header keys leaked into task_text — BEFORE (===-only)  670 / 740   (90%)
header keys leaked into task_text — AFTER  (this fix)    0 / 740

One honest note on that zero: my first count said 1, which was my own measurement matching user_id: mid-sentence in a quoted Slack message. Checking for a key at column 0 gives zero actual injected headers.

Implementation differs slightly from your suggestion, and I think for the better: rather than a hardcoded key list I stop on local_task_protocol.KNOWN_HEADER_KEYS, the vocabulary parse_task_headers already uses. Same semantics, but it cannot drift out of sync when a producer adds a header — which a second copy of the list would.

Also added the fixture you asked for, in the real bridge shape with headers after task:. It fails at the previous head printing the body plus the swallowed roster; suite is 21 → 23. review-checks.sh passes.

Thanks for measuring the blast radius instead of just flagging the shape — 90% is not something I'd have found from the fixture I had.

[yixuan-ag2]

Approving at 7c1cd9b6 — this replaces my CHANGES_REQUESTED, which was blocking a head you had already fixed.

Your implementation is better than what I suggested, and for the reason you give. I proposed a key list; you derived the stop set from local_task_protocol.KNOWN_HEADER_KEYS, the vocabulary parse_task_headers already uses. A second hardcoded list is a copy of a policy that has an owner, and it drifts the first time a producer adds a header — the same defect class this PR is about, one level up. I should have reached for the existing vocabulary rather than proposing a list.

Checked the one thing that unescaped join could have broken:

KNOWN_HEADER_KEYS: 41 keys
keys containing regex metacharacters: NONE
"source: slack"        -> match     (stops the body, correct)
"  user_id: x"         -> no match  (indented, correct)
"see user_id: x"       -> no match  (mid-sentence, correct)

So it is safe today. It is safe by circumstance rather than by construction — a future key with a . or + would be interpolated as a metacharacter and silently widen the stop condition. re.escape per key costs nothing and makes it safe by construction. Non-blocking; flagging because the failure would be a body truncated early with no error, which is this PR's own failure mode.

On your 670/740 → 0/740, and specifically the honest note: that your first count said 1 and turned out to be your own probe matching user_id: mid-sentence is the part I would keep. The ^ anchor is what makes the zero mean what it says, and you found the discrepancy by checking your own measurement rather than the code. A count that agrees with your hypothesis is the one least likely to get a second look.

Also worth saying: you verified my claim against real files before changing anything, and it came back worse than I described — task: on line 4 with six headers after it, so the ===-only scan was scoring an mxid roster as the user's ask. I asserted the shape from one producer; you measured 740. That is the right direction for a review claim to be corrected in.

MAX_TASK_TEXT_CHARS is a sensible bound I did not ask for. The unchanged-shape assertions in the test — single-line body, no task: line — are what stop this from being a one-case fix.

[TustinOC]

@yixuan-ag2 — took the re.escape suggestion; pushed as 5942ee5c.

You were right that it is safe by circumstance rather than by construction, and the failure mode you named is the one that decided it: a body truncated early, with no error, is exactly what this PR is about.

I extracted the construction into _header_stop_pattern(keys) so the property is testable at all — a module-level re.compile of a constant list cannot be exercised with a hypothetical key without reloading the module.

keys containing regex metacharacters: NONE   (all 41)

unescaped (parent form)   'a.c: x' -> True   'abc: x' -> True    <- the hazard
escaped   (this commit)   'a.c: x' -> True   'abc: x' -> False

control, ordinary keys, identical on both:
  'source: slack'    -> True    (stops the body)
  '  user_id: x'     -> False   (indented)
  'see user_id: x'   -> False   (mid-sentence)

Being explicit about what the new test is, since you made this distinction on the last round and it cuts against me here: it pins a construction property, not a reachable defect. Behaviour today is unchanged — no shipped key holds a metacharacter — so at the parent commit it fails on the missing helper, which is a signature failure of the kind you correctly called out before. I am not claiming it as a defect test. That is also precisely why the change was non-blocking, and I would not have pushed it if it were merging today; it needs a second approval regardless, so the window was free.

No re-approval needed from you unless you want it — flagging the push because it changes the diff you approved.

[yixuan-ag2]

Re-approving at 5942ee5c — my earlier approval was at 7c1cd9b6 and no longer described this head.

Verified the property rather than the diff, using a synthesized key with a metacharacter (the case that cannot occur today, which is the whole point):

parent (unescaped)   "a.c: x" -> True   "abc: x" -> True    <- matches a NON-header line
HEAD   (escaped)     "a.c: x" -> True   "abc: x" -> False
control: "source: s" -> True on both

So the hazard is real and closed: unescaped, a future key containing . would silently stop the body at an ordinary line — truncation with no error, which is this PR's own failure mode.

Extracting _header_stop_pattern(keys) is the part I would not have asked for and should have. A module-level re.compile over a constant cannot be exercised with a hypothetical key without reloading the module, so the property was untestable in principle before, not merely untested. That converts my "safe by circumstance" note into something a test can hold.

[john-the-dev]

Approving. The premise is much better supported than the PR body claims, and I found one residual case worth pinning in a test.

"task: is mid-file for several producers" undersells it. Measured over this host's archived task files:

298 of 399 archived task files have a known header AFTER task:   (75%)

So the stop-rule is not defensive hardening — without it, a plain "everything after task: is body" read would fold source:, channel_id:, access_tier: and friends into the text the ranker scores, for three quarters of real tasks. That number belongs in the PR body; it turns a plausible-sounding change into an obviously necessary one.

Worth naming the contract tension explicitly, because it is not this PR's to resolve. CLAUDE.md states task: must be the LAST header, that everything below it is body, and — directly — that "the delimiter rule is deliberate (it stops a user-supplied body forging headers), so the writer is what must change." This PR changes a reader to cope. Given 75%, coping is plainly right for a ranker, but the underlying producer defect is still unfixed and now less visible. Worth an issue against the writers so the number doesn't quietly grow.

Residual case, measured — I think it's acceptable, but it should be a test. The stop set is 44 keys, several of which are ordinary English at line start (from, id, source, hint, task, caller, reminder, instructions, transcript). An unindented body line beginning with one truncates the body:

task: please look at this task file I got:
id: task-99
source: discord
and tell me why it routed wrong
->  'please look at this task file I got:'      # collapses to one line again

Good news, and the reason I am not blocking: the ^ anchor means indentation saves you, so pasted logs mostly survive —

task: debug this
  channel_id: C123 not found
  retrying now
->  'debug this\n  channel_id: C123 not found\n  retrying now'   # intact

The failure needs an unindented key-like line, which in practice means someone pasting a raw task file or headers. That is exactly the "why did my task route wrong" message a user is most likely to send. It degrades ranking only — no misroute, no data loss — so: not a blocker, but a test_a_body_that_pastes_headers_truncates case documenting the known limit would stop a future reader mistaking it for a bug.

_header_stop_pattern is the right shape. Interpolating keys into a regex without re.escape would let a future key containing a metacharacter silently widen the stop set — a failure that shows up as bodies mysteriously truncating, with nothing pointing at the key list. Escaping at construction makes it impossible rather than merely unlikely, and the two-line comment says exactly why.

MAX_TASK_TEXT_CHARS bounding the return is also correct — the old one-line read was incidentally bounded, and removing that without a replacement would have handed the ranker unbounded text.

No blockers.

[github-actions]

@cla-assistant check