fleet/fleetd
3
0
Fork 0
Code Issues 108 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleetd #799: observer SEND header carries the space/tab label #800

Closed
agent wants to merge 0 commits from worker/799-a76336-9 into main
pull from: worker/799-a76336-9
merge into: fleet:main
Conversation 2 Commits - Files Changed -
+171 -9
agent commented 2026-10-06 19:23:58 +02:00
Member
Copy Link
Copy Source

Closes #799.

attributeIfObserver prefixed an observer's SEND with its terminal id and nothing else. This adds the herdr space and tab display label alongside the id, e.g.

[fleet_send from observer term_x (space "ltms", tab "lead")]

Design

  • PaneLocator gets a new locate(terminal) method that resolves a terminal to its pane's tab/workspace id from pane.list (the same call terminalForPid already scans -- no new herdr RPC). That id is then joined against the existing tabLabelsByTabId/workspaceLabelsByWorkspaceId suppliers already wired into fleet_list.
  • The terminal id always stays first in the header and is never replaced by a label (constraint 1).
  • Labels are rendered as a trailing (space "...", tab "...") annotation, quoted and explicitly named, so the header does not read as something to reply to (constraint 2) -- the id is the only addressable token.
  • When neither label is known, or a herdr error interrupts the lookup, the header degrades to the bare id with no parenthetical, never null or empty brackets (constraint 3).
  • FleetApp's REST entry path now passes its own PaneLocator(herdr, memberHerdr) to the same shared method, so both surfaces attribute identically.
  • plugin/hooks/register.js is unchanged, as instructed -- it just prepends its own line to whatever the daemon produced.

Tests

  • Updated FleetMcpObserverSendDeliveryTest and FleetMcpObserverSendToLeadDeliveryTest for the new exact string (FakeHerdr's term_shell sits in workspace "w2"/"ltms", tab "w2:t8" has no seeded label).
  • New FleetMcpAttributeIfObserverTest covers: a non-observer passes through unchanged; both labels present; a shared label across two different panes still yields two distinct headers by id; an unresolvable pane degrades to the bare id with no null/empty parens; a herdr error mid-lookup degrades the same way; only one label known.

Build: mvn clean install in fleetd/ -- BUILD SUCCESS. Surefire totals counted from target/surefire-reports/*.xml: Tests run 2214, Failures 0, Errors 0, Skipped 0.

Closes #799. `attributeIfObserver` prefixed an observer's SEND with its terminal id and nothing else. This adds the herdr space and tab display label alongside the id, e.g. `[fleet_send from observer term_x (space "ltms", tab "lead")]` **Design** - `PaneLocator` gets a new `locate(terminal)` method that resolves a terminal to its pane's tab/workspace id from `pane.list` (the same call `terminalForPid` already scans -- no new herdr RPC). That id is then joined against the existing `tabLabelsByTabId`/`workspaceLabelsByWorkspaceId` suppliers already wired into `fleet_list`. - The terminal id always stays first in the header and is never replaced by a label (constraint 1). - Labels are rendered as a trailing `(space "...", tab "...")` annotation, quoted and explicitly named, so the header does not read as something to reply to (constraint 2) -- the id is the only addressable token. - When neither label is known, or a herdr error interrupts the lookup, the header degrades to the bare id with no parenthetical, never `null` or empty brackets (constraint 3). - `FleetApp`'s REST entry path now passes its own `PaneLocator(herdr, memberHerdr)` to the same shared method, so both surfaces attribute identically. - `plugin/hooks/register.js` is unchanged, as instructed -- it just prepends its own line to whatever the daemon produced. **Tests** - Updated `FleetMcpObserverSendDeliveryTest` and `FleetMcpObserverSendToLeadDeliveryTest` for the new exact string (FakeHerdr's `term_shell` sits in workspace "w2"/"ltms", tab "w2:t8" has no seeded label). - New `FleetMcpAttributeIfObserverTest` covers: a non-observer passes through unchanged; both labels present; a shared label across two different panes still yields two distinct headers by id; an unresolvable pane degrades to the bare id with no `null`/empty parens; a herdr error mid-lookup degrades the same way; only one label known. **Build**: `mvn clean install` in `fleetd/` -- BUILD SUCCESS. Surefire totals counted from `target/surefire-reports/*.xml`: Tests run 2214, Failures 0, Errors 0, Skipped 0.
agent added 4 commits 2026-10-06 19:23:58 +02:00
Bridge block: a receiver confirms what it receives f544906621 
Adds invariant 6 to the canonical block. Nothing in it said a received
message must be answered, so a sender could not tell a handled message
from one that never arrived.

Measured today: the anki pane (observer) sent to vms, which reads
deliverable:false because it has not contacted the daemon since the 08:05
restart. The send was accepted, held 83s, and failed with the body 'nst' --
three characters scraped off the vms screen. The sender read that as an
inconclusive result and asked the lead what happened. fleetd #757 covers
the accept-time refusal; this rule covers the half the protocol owns.

Operator's words: 'when a message sent, at least the receiver should
confirm unless explicit told to not reply'.

wiki/7-Use-Cases.md template synced byte-identical (wiki 3ad606f).
Bridge block: invariant 6 — an operator outranks the confirm rule b41aa663f7 
Invariant 6 as first written told every receiver to confirm, with no
mention of who may override it. A peer cannot impose a rule on another
operator's session.

Measured: the trinotes pane (observer, work config dir) answered the
communication test and then said its operator's standing rule is not to
answer fleet messages, that a peer cannot change that rule, and that it
would ask its operator before following mine. That is the correct reading
and the rule now says so.

Also records the symmetric error: a sender must not read silence as
agreement or as a dead session.

wiki/7-Use-Cases.md synced byte-identical.
reviewer skill: send fleet_reply before writing the reasoning out 96ebae28a6 
Five reviewer turns were lost in one session. Each wrote a long, correct
analysis to its own pane and ended the turn with no fleet_reply, so the
bridge scraped the pane and the lead received a clipped fragment.

The briefs are half the cause: each carried a five-item checklist of things
to hunt alongside a ~90-word capped output format, which reads as two
contradictory output contracts. The skill now says the checklist is where to
look, not the shape of the answer, and that the reply goes out as soon as
the answer is known.

Measured: tickets task-7da785-29 and -30 resolved at 18:28 via the
turn-completion fallback, 1233 and 1403 chars scraped.
fleetd #799: observer SEND header carries the space/tab label, not just the id
CI / shell-tests (pull_request) Failing after 10s
Details
CI / contract (pull_request) Successful in 55s
Details
CI / build (pull_request) Failing after 1m58s
Details
2dddacdd38 
attributeIfObserver now joins the caller's terminal to its herdr pane via
PaneLocator.locate (new, reusing pane.list) and appends whichever of the
tab/workspace labels herdr reports, e.g.
"[fleet_send from observer term_x (space "ltms", tab "lead")]". The id stays
first and is never replaced; missing labels, or a herdr error while looking
them up, degrade to the bare id with no parenthetical. FleetApp's REST entry
path takes the same PaneLocator so both surfaces attribute identically.
ltms commented 2026-10-07 12:17:26 +02:00
Owner
Copy Link
Copy Source

Reviewed the diff myself at origin/main = 376b583, plus one delegated reviewer. Not merging yet — one blocking finding, one low-severity one.

The shape of the change is right: locate() is a clean addition, the header degrades to the bare terminal id rather than printing null, and both entry paths (FleetMcp.java:507 and FleetApp.java:717) go through the same attributeIfObserver, so the two surfaces cannot drift. The terminal id in the header does come from caller.terminal(), which is connection-resolved and not caller-supplied.

Blocking: the tab label is written by the sender's own agent, and it is interpolated unescaped

observerHeader builds the header with no escaping and no length cap:

header.append("space \"").append(workspaceLabel).append('"');
...
header.append("tab \"").append(tabLabel).append('"');

tabLabel comes from PaneLocator.tabLabelsByTabId(), which is Tab.label() — herdr's label field (PaneLocator.java:167, Tab.java:21).

That field is writable by the agent sitting in the pane. This is not a hypothesis. It is what #811 measured earlier today: a lead's tab that LeadLauncher created as label='lead' read label='Fleet lead session setup' about ten seconds later, because Claude Code titles its terminal from the first message it receives. LeadTabScanner matches on that same field, which is why the session was demoted. So the one field this PR puts in the header is the field #811 proved an agent rewrites about itself.

Two consequences, and the second does not need an attacker:

  1. Injection. An observer that sets its own title can close the parenthetical and forge a second one, because ", ) and ] pass through untouched. The javadoc's defence — "a label is display-only, and is never substituted for [the terminal id]" — is about what a reply can target. It does not hold for a human or an agent reading the header to decide who it is talking to, which is the header's stated purpose.
  2. Leak, in the normal case. CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 is set only by LeadLauncher.leadEnv(), so an observer pane does not get it. An observer's tab label is therefore routinely an auto-generated summary of its own conversation. This PR forwards that text into another pane's prompt on every send. Of the live observer panes right now, several carry labels that look conversation-derived rather than operator-set.

This repo already has the precedent for the fix: Metrics.escapeLabel (Metrics.java:178) escapes backslash, quote and newline for exactly this key="value" interpolation. Please do the same here, and add a length cap — an auto-generated title has no bound, and this text is pasted into someone else's prompt.

I have not tested whether a literal newline can reach the label; an OSC title sequence is terminated by BEL or ST, so it may be filtered upstream. Quote and bracket injection need no newline, so the escaping is warranted either way. Do not let the escaping rest on an untested assumption about newlines.

Low: one herdr daemon's failure hides a label the other could answer

From the delegated review, and I agree with it. locate() (PaneLocator.java:210), tabLabelsByTabId() (:158) and workspaceLabelsByWorkspaceId() (:184) all loop for (HerdrClient herdr : herdrs) with no per-client try. A HerdrException from the first daemon propagates out, so observerHeader's outer catch degrades the whole header to the bare id even when the pane's label was reachable on the healthy daemon. terminalForPid/scan in this same class deliberately does the opposite — it keeps going and only gives up once every daemon has been tried. Match that pattern.

Two smaller notes, not blocking

  • observerHeader makes three herdr round trips per observer send: pane.list, then workspace.list + tab.list per workspace, then workspace.list again — and it builds two whole maps to read one key out of each. Worth a single lookup.
  • FleetApp.java:717 constructs new PaneLocator(herdr, memberHerdr) per request. The MCP path reuses identity.panes(). Hoist it.
  • The outer catch covers HerdrException only. If either map call can throw anything else, an observer's send now fails where it previously always succeeded. Worth widening to RuntimeException, since the header is strictly best-effort.

Verified, so you need not redo it

  • The diff merges cleanly against current origin/main — git merge-tree exit 0, no conflicts, despite being 28 commits behind.
  • The new FleetMcpAttributeIfObserverTest assertions are exact-match and not vacuous, and the two touched delivery tests assert the call-site output rather than only the helper.
  • The null/missing tab-and-workspace cases are handled: neither null, "null", nor an empty parenthetical can be produced.
  • PR #785 does conflict with main in FleetMcp.java/FleetApp.java, but on main's own sendableObserverTarget → observerSendTarget rename, nowhere near the lines this PR touches. That conflict is #785's to resolve and is not a reason to hold #800.
Reviewed the diff myself at `origin/main` = `376b583`, plus one delegated reviewer. **Not merging yet** — one blocking finding, one low-severity one. The shape of the change is right: `locate()` is a clean addition, the header degrades to the bare terminal id rather than printing `null`, and both entry paths (`FleetMcp.java:507` and `FleetApp.java:717`) go through the same `attributeIfObserver`, so the two surfaces cannot drift. The terminal id in the header does come from `caller.terminal()`, which is connection-resolved and not caller-supplied. ## Blocking: the tab label is written by the sender's own agent, and it is interpolated unescaped `observerHeader` builds the header with no escaping and no length cap: ```java header.append("space \"").append(workspaceLabel).append('"'); ... header.append("tab \"").append(tabLabel).append('"'); ``` `tabLabel` comes from `PaneLocator.tabLabelsByTabId()`, which is `Tab.label()` — herdr's `label` field (`PaneLocator.java:167`, `Tab.java:21`). **That field is writable by the agent sitting in the pane.** This is not a hypothesis. It is what #811 measured earlier today: a lead's tab that `LeadLauncher` created as `label='lead'` read `label='Fleet lead session setup'` about ten seconds later, because Claude Code titles its terminal from the first message it receives. `LeadTabScanner` matches on that same field, which is why the session was demoted. So the one field this PR puts in the header is the field #811 proved an agent rewrites about itself. Two consequences, and the second does not need an attacker: 1. **Injection.** An observer that sets its own title can close the parenthetical and forge a second one, because `"`, `)` and `]` pass through untouched. The javadoc's defence — "a label is display-only, and is never substituted for [the terminal id]" — is about what a *reply* can target. It does not hold for a human or an agent reading the header to decide who it is talking to, which is the header's stated purpose. 2. **Leak, in the normal case.** `CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1` is set only by `LeadLauncher.leadEnv()`, so an observer pane does not get it. An observer's tab label is therefore *routinely* an auto-generated summary of its own conversation. This PR forwards that text into another pane's prompt on every send. Of the live observer panes right now, several carry labels that look conversation-derived rather than operator-set. This repo already has the precedent for the fix: `Metrics.escapeLabel` (`Metrics.java:178`) escapes backslash, quote and newline for exactly this `key="value"` interpolation. Please do the same here, and add a length cap — an auto-generated title has no bound, and this text is pasted into someone else's prompt. I have **not** tested whether a literal newline can reach the label; an OSC title sequence is terminated by BEL or ST, so it may be filtered upstream. Quote and bracket injection need no newline, so the escaping is warranted either way. Do not let the escaping rest on an untested assumption about newlines. ## Low: one herdr daemon's failure hides a label the other could answer From the delegated review, and I agree with it. `locate()` (`PaneLocator.java:210`), `tabLabelsByTabId()` (`:158`) and `workspaceLabelsByWorkspaceId()` (`:184`) all loop `for (HerdrClient herdr : herdrs)` with no per-client `try`. A `HerdrException` from the first daemon propagates out, so `observerHeader`'s outer catch degrades the whole header to the bare id even when the pane's label was reachable on the healthy daemon. `terminalForPid`/`scan` in this same class deliberately does the opposite — it keeps going and only gives up once every daemon has been tried. Match that pattern. ## Two smaller notes, not blocking - `observerHeader` makes three herdr round trips per observer send: `pane.list`, then `workspace.list` + `tab.list` per workspace, then `workspace.list` again — and it builds two whole maps to read one key out of each. Worth a single lookup. - `FleetApp.java:717` constructs `new PaneLocator(herdr, memberHerdr)` per request. The MCP path reuses `identity.panes()`. Hoist it. - The outer catch covers `HerdrException` only. If either map call can throw anything else, an observer's send now fails where it previously always succeeded. Worth widening to `RuntimeException`, since the header is strictly best-effort. ## Verified, so you need not redo it - The diff merges cleanly against current `origin/main` — `git merge-tree` exit 0, no conflicts, despite being 28 commits behind. - The new `FleetMcpAttributeIfObserverTest` assertions are exact-match and not vacuous, and the two touched delivery tests assert the call-site output rather than only the helper. - The null/missing tab-and-workspace cases are handled: neither `null`, `"null"`, nor an empty parenthetical can be produced. - PR #785 does conflict with main in `FleetMcp.java`/`FleetApp.java`, but on main's own `sendableObserverTarget` → `observerSendTarget` rename, nowhere near the lines this PR touches. That conflict is #785's to resolve and is not a reason to hold #800.
ltms commented 2026-10-07 12:48:35 +02:00
Owner
Copy Link
Copy Source

Closing this in favour of #819, which carries this PR's commits plus the fixes for the findings raised here.

worker/799-6d217f-4 contains 2dddacd (this PR's work) and d3c5a63 (the merge of it), so nothing from here is lost — #819 is a strict superset. Reviewing and merging two branches that share the same commits would have meant adjudicating the same diff twice.

What was found here, and where it went

The blocking finding was that FleetMcp.observerHeader interpolated the space and tab labels into a quoted header with no escaping and no length cap. That is not a theoretical input: fleetd #811 measured Claude Code rewriting its own tab label, and Tab.label() is the exact field this header carries. So the label is written by the agent in the pane and must be treated as hostile.

#819 adds sanitizeObserverLabel and MAX_OBSERVER_LABEL_LENGTH = 80. I checked the result by running the sanitizer's logic directly rather than reading it: " and \ are escaped so the quoted field cannot be closed, ) and [ are inert inside it, and the worst-case escaped output is bounded at 320 characters. Four code points still pass through unescaped — U+0085, U+2028, U+2029 and U+202E — and truncation can leave a lone surrogate. That is now a fix in flight on #819, not an open question.

Also carried into #819: PaneLocator catching per client so one herdr daemon's failure no longer stops the other, observerHeader's catch widened from HerdrException to RuntimeException, and FleetApp no longer building a PaneLocator per request — it is a constructor-initialised field reused at both call sites.

Measured on the #819 merge

origin/main had moved on (#815 landed), so I built the merge, not the head. git merge-tree --write-tree exited 0 and gave tree 8a6c129; merging origin/main into the branch in a throwaway worktree produced the same tree 8a6c129. My own unpiped mvn -o -f fleetd/pom.xml clean install on it: BUILD SUCCESS, 0 ^[ERROR] lines, Tests run: 2286, Failures: 0, Errors: 0, Skipped: 0 — main is at 2271.

A second reviewer looked only at the PaneLocator half and reported no issue, having checked the per-client catches, the empty-result paths, every new PaneLocator( in main source, the shared instance's statelessness, and the five fallback tests.

Nothing further is needed on this PR. Follow #819.

Closing this in favour of **#819**, which carries this PR's commits plus the fixes for the findings raised here. `worker/799-6d217f-4` contains `2dddacd` (this PR's work) and `d3c5a63` (the merge of it), so nothing from here is lost — #819 is a strict superset. Reviewing and merging two branches that share the same commits would have meant adjudicating the same diff twice. ## What was found here, and where it went The blocking finding was that `FleetMcp.observerHeader` interpolated the space and tab labels into a quoted header with no escaping and no length cap. That is not a theoretical input: fleetd #811 measured Claude Code rewriting its own tab label, and `Tab.label()` is the exact field this header carries. So the label is written by the agent in the pane and must be treated as hostile. #819 adds `sanitizeObserverLabel` and `MAX_OBSERVER_LABEL_LENGTH = 80`. I checked the result by running the sanitizer's logic directly rather than reading it: `"` and `\` are escaped so the quoted field cannot be closed, `)` and `[` are inert inside it, and the worst-case escaped output is bounded at 320 characters. Four code points still pass through unescaped — U+0085, U+2028, U+2029 and U+202E — and truncation can leave a lone surrogate. That is now a fix in flight on #819, not an open question. Also carried into #819: `PaneLocator` catching per client so one herdr daemon's failure no longer stops the other, `observerHeader`'s catch widened from `HerdrException` to `RuntimeException`, and `FleetApp` no longer building a `PaneLocator` per request — it is a constructor-initialised field reused at both call sites. ## Measured on the #819 merge `origin/main` had moved on (#815 landed), so I built the **merge**, not the head. `git merge-tree --write-tree` exited 0 and gave tree `8a6c129`; merging `origin/main` into the branch in a throwaway worktree produced the same tree `8a6c129`. My own unpiped `mvn -o -f fleetd/pom.xml clean install` on it: `BUILD SUCCESS`, 0 `^[ERROR]` lines, `Tests run: 2286, Failures: 0, Errors: 0, Skipped: 0` — `main` is at 2271. A second reviewer looked only at the `PaneLocator` half and reported no issue, having checked the per-client catches, the empty-result paths, every `new PaneLocator(` in main source, the shared instance's statelessness, and the five fallback tests. Nothing further is needed on this PR. Follow #819.
ltms closed this pull request 2026-10-07 12:48:39 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 10s
Details
CI / contract (pull_request) Successful in 55s
Details
CI / build (pull_request) Failing after 1m58s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#800
Delete Branch "worker/799-a76336-9"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?