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

Five places trust a command's exit code as proof of its effect and never read the value back — the #400 shape outside the scrub #408

New Issue
Open
opened 2026-09-10 04:03:53 +02:00 by ltms · 0 comments
ltms commented 2026-09-10 04:03:53 +02:00
Owner
Copy Link
Copy Source

One ticket for the shape, not five tickets. Surfaced by the #400 worker's survey and filtered by me; the confidence ordering below is theirs and I agree with it.

The shape

#400 was: eval "export NAME=" returns 0, so the code recorded the name as blanked — but zsh had coerced the assignment on an integer parameter and the value was unchanged. An attempt's exit status is not a measurement of its effect. Measured there: 7 false receipts out of 10 names. Fixed in #403 by reading the value back with [[ -z "${(P)name}" ]].

The same reasoning error appears in five other places. Each runs a command that changes state, checks only that it did not throw, and never reads the state back.

The instances, most serious first

  1. session/GitWorktrees.java:440-441 — blanks credential.helper with git config --worktree --replace-all / --add "" and treats no-exception as proof it is now empty. Never reads it back with --get-all. Credential-adjacent, so this is the one I would take first: a helper that silently survives is a credential channel left open, and nothing would say so.

  2. session/GitWorktrees.java:621,982 — git update-index --skip-worktree <file> trusts exit 0 and never checks git ls-files -v for the S prefix. This matters more than it looks. The project addendum's whole neutralized-config mechanism depends on that bit being set, and the failure mode is a worker that edits .mcp.json or opencode.json, cannot commit it, and is told nothing — which is already documented as a known trap for workers.

  3. session/GitWorktrees.java:1055-1058 — the WIP-commit path (add -A → write-tree → commit-tree → update-ref) trusts each step's exit code and never reads back refs/wip/<branch> to confirm it points at a commit carrying the intended tree. This is the handover path: a snapshot is reported as saved with no proof it exists. The cost of being wrong here is a live worker's uncommitted work.

  4. power/CaffeinateSleepAssertionMechanism.java:50-84 — treats the caffeinate -i process still being alive as proof sleep is blocked. Never checks actual system state with pmset -g assertions. Same shape one layer out: a live process is not a held assertion.

  5. session/GitWorktrees.java:599,922 — git config --worktree --add fleet.neutralizedConfig <file> / fleet.seededSkills <name> trusts the add and never reads back --get-all to confirm the value is in the list. Lower stakes than (2) because this is the record of what was neutralized rather than the neutralization itself, but a wrong record is what a worker or a debugging lead reads.

Low confidence, probably benign, listed for completeness: session/GitWorktrees.java:298,489 — origin-URL checks where the value is read back immediately afterwards anyway.

What a fix should and should not do

Do not add a read-back everywhere reflexively. The question for each site is whether a silent no-op is reachable and whether it matters. A defect on paper is not a reachable defect. For each instance, name the state the command can leave behind that the exit code hides, before writing the read-back.

Where a read-back is warranted, it must go somewhere the result is actually used — a read-back whose result is discarded is the same defect with extra code.

Instances (1), (2) and (3) look genuinely reachable to me and are worth doing. (4) and (5) are worth a decision recorded in a comment, which may well be "the exit code is enough here, because X".

Acceptance

  • For each of the five, either a read-back that a test pins, or a comment naming why the exit code is sufficient. Not silence.
  • Each read-back is proven by a mutation: break the underlying operation so it silently no-ops, and show a test fails. A test that only exercises the happy path proves nothing here — the whole bug class is the unhappy path returning 0.
  • Never print the value of a credential or a credential channel. For (1) the assertion is that the helper is empty; check emptiness, do not log what was there. Redact any URL at the pipe before it reaches a log or a test message.
  • Report each instance separately with its own result. If one turns out unreachable, say so — that is a real finding and closes it properly.

Related

#400 / #403 (the original, in EnvAllowListScrub), and this repo's standing note that a receipt which overstates is worse than no receipt.

One ticket for the shape, not five tickets. Surfaced by the #400 worker's survey and filtered by me; the confidence ordering below is theirs and I agree with it. ## The shape #400 was: `eval "export NAME="` returns 0, so the code recorded the name as blanked — but zsh had coerced the assignment on an integer parameter and the value was unchanged. **An attempt's exit status is not a measurement of its effect.** Measured there: 7 false receipts out of 10 names. Fixed in #403 by reading the value back with `[[ -z "${(P)name}" ]]`. The same reasoning error appears in five other places. Each runs a command that changes state, checks only that it did not throw, and never reads the state back. ## The instances, most serious first 1. **`session/GitWorktrees.java:440-441`** — blanks `credential.helper` with `git config --worktree --replace-all` / `--add ""` and treats no-exception as proof it is now empty. Never reads it back with `--get-all`. Credential-adjacent, so this is the one I would take first: a helper that silently survives is a credential channel left open, and nothing would say so. 2. **`session/GitWorktrees.java:621,982`** — `git update-index --skip-worktree <file>` trusts exit 0 and never checks `git ls-files -v` for the `S` prefix. This matters more than it looks. The project addendum's whole neutralized-config mechanism depends on that bit being set, and the failure mode is a worker that edits `.mcp.json` or `opencode.json`, cannot commit it, **and is told nothing** — which is already documented as a known trap for workers. 3. **`session/GitWorktrees.java:1055-1058`** — the WIP-commit path (`add -A` → `write-tree` → `commit-tree` → `update-ref`) trusts each step's exit code and never reads back `refs/wip/<branch>` to confirm it points at a commit carrying the intended tree. This is the handover path: a snapshot is reported as saved with no proof it exists. The cost of being wrong here is a live worker's uncommitted work. 4. **`power/CaffeinateSleepAssertionMechanism.java:50-84`** — treats the `caffeinate -i` process still being alive as proof sleep is blocked. Never checks actual system state with `pmset -g assertions`. Same shape one layer out: a live process is not a held assertion. 5. **`session/GitWorktrees.java:599,922`** — `git config --worktree --add fleet.neutralizedConfig <file>` / `fleet.seededSkills <name>` trusts the add and never reads back `--get-all` to confirm the value is in the list. Lower stakes than (2) because this is the *record* of what was neutralized rather than the neutralization itself, but a wrong record is what a worker or a debugging lead reads. **Low confidence, probably benign, listed for completeness:** `session/GitWorktrees.java:298,489` — origin-URL checks where the value is read back immediately afterwards anyway. ## What a fix should and should not do **Do not add a read-back everywhere reflexively.** The question for each site is whether a silent no-op is *reachable* and whether it *matters*. A defect on paper is not a reachable defect. For each instance, name the state the command can leave behind that the exit code hides, before writing the read-back. Where a read-back is warranted, it must go somewhere the result is actually used — a read-back whose result is discarded is the same defect with extra code. Instances (1), (2) and (3) look genuinely reachable to me and are worth doing. (4) and (5) are worth a decision recorded in a comment, which may well be "the exit code is enough here, because X". ## Acceptance - For each of the five, either a read-back that a test pins, or a comment naming why the exit code is sufficient. Not silence. - Each read-back is proven by a mutation: break the underlying operation so it silently no-ops, and show a test fails. A test that only exercises the happy path proves nothing here — the whole bug class is the unhappy path returning 0. - **Never print the value of a credential or a credential channel.** For (1) the assertion is that the helper is empty; check emptiness, do not log what was there. Redact any URL at the pipe before it reaches a log or a test message. - Report each instance separately with its own result. If one turns out unreachable, say so — that is a real finding and closes it properly. ## Related #400 / #403 (the original, in `EnvAllowListScrub`), and this repo's standing note that a receipt which overstates is worse than no receipt.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#408
Delete Branch "%!s()"

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?