fix: downgrade 4 TODOs to XXX per repo standard

Merging after clean merge from master + clean Codex CLI review. Reclassifies 4 non-vendor TODOs that were tagged too aggressively:

- XXX(stateful-shell): design reference, current spawn-per-call works fine
- XXX(tool-bash-owner-hmr) (3x): HMR-only dev issue, acceptable today

All other TODOs kept as-is. No vendor changes.
This commit is contained in:
Tianyi Cui
2026-06-18 23:20:21 +08:00
committed by GitHub
5 changed files with 6 additions and 6 deletions
+1 -1
View File
@@ -18,7 +18,7 @@ Local-subprocess implementation of the `@deepseek-ai/dsh-bash` executor seam: `L
Design surveyed against the bash tools of Claude Code, OpenCode, Codex, and pi; the notable choices:
- **Spawn per call, no shell state** — every call is a fresh non-login `bash -c` (deterministic; no rc files). All four surveyed tools spawn per call. `TODO(stateful-shell)` in `src/run.ts` records the two proven stateful designs (Claude Code's cwd-only persistence; Codex's PTY exec sessions) for when real workflows demand them.
- **Spawn per call, no shell state** — every call is a fresh non-login `bash -c` (deterministic; no rc files). All four surveyed tools spawn per call. `XXX(stateful-shell)` in `src/run.ts` records the two proven stateful designs (Claude Code's cwd-only persistence; Codex's PTY exec sessions) for when real workflows demand them.
- **Process-group kills with escalation** — children are spawned `detached` (own process group); kills send SIGTERM to the group, then SIGKILL after a 3s grace (OpenCode's escalation; pipelines and subshells die with the parent). ESRCH is tolerated; daemons that re-parent away from the group can still survive — same caveat as the surveyed tools.
- **Tail-keep truncation + spill files** — output beyond `maxOutputBytes` keeps the in-memory TAIL (errors/results cluster at the end — pi/OpenCode rationale) while the FULL stream is appended to a temp file whose path is reported. The model can `grep`/`tail` the spill file with bash itself.
- **Model-friendly env** — `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results.
+1 -1
View File
@@ -245,7 +245,7 @@ export interface RunningBash {
* RESOLVES with a {@link SpawnOutcome} describing what happened, so callers
* shape one consistent report for the model.
*
* TODO(stateful-shell): per the agent-tool survey there are two proven
* XXX(stateful-shell): per the agent-tool survey there are two proven
* stateful designs worth revisiting — Claude Code persists ONLY cwd between
* calls (captures `pwd -P` after each command), and Codex keeps whole PTY
* exec sessions addressable via session ids + stdin writes. We deliberately
+1 -1
View File
@@ -30,7 +30,7 @@ Result text: stdout, then a `[stderr]` section, then status markers — `[timed
### Task ownership (cross-session isolation)
The owning agent is recorded per task id at spawn and kept for the lifetime of the loaded plugin instance (it is **not** cleared on completion). `bash_output`/`bash_kill` reject a task owned by a *different* agent with `task <id> belongs to another session` (a task started with no agent — a non-loop caller — has no owner and is open to anyone; a call with no `exec.agent` cannot access an owned task). Task ids are global and predictable, so under multi-session ACP this ownership check is the fence that stops one session's agent from reading or killing another session's background task. (`TODO(tool-bash-owner-hmr)`: an independent HMR reload of this plugin starts a fresh map, so a task spawned before the reload becomes un-owned — acceptable as HMR is dev-only and the session boundary is one user's cooperative editor; a durable fix attaches ownership to the executor/task lifetime.)
The owning agent is recorded per task id at spawn and kept for the lifetime of the loaded plugin instance (it is **not** cleared on completion). `bash_output`/`bash_kill` reject a task owned by a *different* agent with `task <id> belongs to another session` (a task started with no agent — a non-loop caller — has no owner and is open to anyone; a call with no `exec.agent` cannot access an owned task). Task ids are global and predictable, so under multi-session ACP this ownership check is the fence that stops one session's agent from reading or killing another session's background task. (`XXX(tool-bash-owner-hmr)`: an independent HMR reload of this plugin starts a fresh map, so a task spawned before the reload becomes un-owned — acceptable as HMR is dev-only and the session boundary is one user's cooperative editor; a durable fix attaches ownership to the executor/task lifetime.)
## UI presentation
+2 -2
View File
@@ -20,7 +20,7 @@
* ownership check is the fence that stops one session's agent from reading or
* killing another session's background task.
*
* TODO(tool-bash-owner-hmr): the ownership map is per-plugin-instance, so an
* XXX(tool-bash-owner-hmr): the ownership map is per-plugin-instance, so an
* independent HMR reload of `tool-bash` (without reloading `dsh-bash`) starts a
* fresh map and a task spawned before the reload becomes un-owned (open to any
* caller). This is acceptable today — HMR is dev-only, the ACP session boundary
@@ -276,7 +276,7 @@ export function apply(ctx: Context): void {
// another session's background task. A task with no recorded owner (started by
// a non-loop caller, `exec.agent` absent) is unowned and accessible to anyone.
// An independent `tool-bash` HMR reload resets this map — see the
// TODO(tool-bash-owner-hmr) note in the module doc.
// XXX(tool-bash-owner-hmr) note in the module doc.
const taskOwner = new Map<string, Agent>()
/**
+1 -1
View File
@@ -410,7 +410,7 @@ describe('background task ownership (cross-session isolation)', () => {
})
it('documents the HMR caveat: an independent tool-bash reload resets ownership', async () => {
// The ownership map is per-plugin-instance (TODO(tool-bash-owner-hmr)). When
// The ownership map is per-plugin-instance (XXX(tool-bash-owner-hmr)). When
// ONLY tool-bash is reloaded (bash/executor + task survive), the new instance
// has an empty map, so the previously-owned task becomes unowned (open). This
// test pins that documented behavior — a regression here (e.g. an accidental