fix(deepseek): atomic shim write, honest attribution comment, name-fallback profile classifier

The three smaller review nits, plus the first real test coverage for the status
shim (it had none: it is emitted as a STRING, so tsc never sees it).

1. The shim was written with a plain writeFileSync. The TUI can be exec'ing that
   exact path while an upgraded Codeman refreshes it, and a reader catching a
   half-written file gets a syntax error, exits non-zero, and is retried four
   times per state change for a file that will never parse. Now temp + rename
   (atomic within the directory), with the temp chmod'ed before the rename since
   writeFileSync's mode only applies on create, and removed if the write throws.
   SHIM_VERSION bumped to 2, because SHIM_SOURCE changed and an existing v1 shim
   would otherwise keep matching the embedded marker and never be refreshed.

2. The pane-id comment claimed the ambient env "cannot be spoofed by an argument
   the agent itself could influence". The agent runs IN that pane and can invoke
   the shim with CODEMAN_SESSION_ID unset and any argv it likes. It buys nothing
   it did not already have (the hook-secret file is readable from the same pane,
   so it can POST /api/hook-event directly), but the comment read like a security
   boundary. Rewritten to say what the preference actually buys: correct
   attribution when a TUI mangles or re-uses the pane argument. Accidents, not
   adversaries.

3. classifyProfile() folded the directory name into the same haystack as the
   bundles, but only the TUI arm could match a bare name, so a stock profile
   whose package.json has no dsh.profile.bundles (hand-edited, older layout,
   mid-install) classified as `unknown` -> launchable -> eligible as the DEFAULT
   pick, which is exactly the pane-dies-on-arrival failure the two-part
   availability gate exists to prevent. The stock names are now a LAST-resort
   fallback consulted after the bundle patterns, so real bundle evidence still
   wins over a name the user chose. The loose `tui` arm gained word boundaries:
   it decides which profile boots by default, and matching the middle of
   `intuition` is not a rule anyone could predict.

New test/deepseek-status-shim.test.ts runs the generated script the way the
harness does -- real node process, real argv, real env, real listener -- and
covers the exit-code contract that makes the retry behaviour safe: mapped states
post and exit 0, an unknown verb or unmapped state exits 0 WITHOUT posting (a
non-zero there would be four HTTP requests per state change forever), a rejecting
server or an unreachable one exits non-zero so the caller retries, the hook secret
is read at execution time, and `node --check` parses the file (a template-literal
typo in SHIM_SOURCE is invisible to tsc).

Trap worth recording, hit while writing it: the tests must spawn the shim
ASYNCHRONOUSLY. The listener lives in the test process, so spawnSync blocks the
event loop that has to accept the connection, the shim waits out its own 1500ms
socket timeout and exits 1, and it reads exactly like a broken shim (measured:
Socket._onTimeout in its --trace-exit output, server logging nothing).

Verified: full gate green (6142 passed, +10), typecheck/lint/format clean.
This commit is contained in:
Codeman maintainer
2026-08-24 18:00:52 +02:00
parent 2034719d61
commit cdceede33d
5 changed files with 312 additions and 8 deletions
+29 -5
View File
@@ -44,7 +44,7 @@
* @module deepseek-status-shim
*/
import { chmodSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs';
import { chmodSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs';
import { dirname } from 'node:path';
import { dataPath } from './config/instance.js';
@@ -54,7 +54,7 @@ import { dataPath } from './config/instance.js';
* by an older Codeman and rewrite only when needed (rather than rewriting on
* every session create, or — worse — leaving a stale one in place forever).
*/
const SHIM_VERSION = 1;
const SHIM_VERSION = 2;
const SHIM_MARKER = `codeman-dsh-status-shim v${SHIM_VERSION}`;
/**
@@ -125,8 +125,13 @@ const event = STATE_TO_EVENT[String(flag('--state') ?? '')]
if (!event) process.exit(0)
// The pane id we hand the TUI IS the Codeman session id, but prefer the ambient
// env: it is set by the same code that set HERDR_PANE_ID and cannot be spoofed
// by an argument the agent itself could influence.
// env: it is set by the same code that set HERDR_PANE_ID, so a TUI that mangles,
// truncates or re-uses the pane argument still reports against the right session.
// NOT a security boundary, and do not read it as one: the agent runs IN this pane
// and can invoke the shim with CODEMAN_SESSION_ID unset and any argv it likes.
// That buys it nothing it did not already have, since the hook-secret file is
// readable from the same pane and any process there can POST /api/hook-event
// directly. Attribution here is about accidents, not adversaries.
const sessionId = process.env.CODEMAN_SESSION_ID || argv[2]
const apiUrl = process.env.CODEMAN_API_URL
if (!sessionId || !apiUrl) process.exit(1)
@@ -213,7 +218,26 @@ export function ensureDeepSeekStatusShim(): string | null {
}
if (!current.includes(SHIM_MARKER)) {
mkdirSync(dirname(path), { recursive: true });
writeFileSync(path, SHIM_SOURCE, { mode: 0o700 });
// Temp + rename, not a plain write: the TUI can be executing this exact
// path at the moment an upgraded Codeman refreshes it (every state change
// runs it, and session create is when the rewrite happens), and a reader
// that catches a half-written file gets a syntax error, exits non-zero,
// and is retried four times per state change for a file that will never
// parse. rename(2) is atomic within the directory, so a concurrent exec
// sees either the old shim or the new one, never a truncated one.
// Same reasoning as the state-store writes; pid-suffixed so two instances
// sharing a data dir cannot collide on the temp name.
const tempPath = `${path}.${process.pid}.tmp`;
try {
writeFileSync(tempPath, SHIM_SOURCE, { mode: 0o700 });
// The mode argument only applies when writeFileSync CREATES the file, so
// a leftover temp from a crashed run would keep its old permissions.
chmodSync(tempPath, 0o700);
renameSync(tempPath, path);
} catch (err) {
rmSync(tempPath, { force: true });
throw err;
}
}
// Re-assert the mode even when the content matched: a shim that lost its
// executable bit (a restored backup, a copied data dir) would make every
+30 -2
View File
@@ -120,8 +120,36 @@ const HEADLESS_BUNDLE_PATTERN = /dsh-headless/i;
* scoped `dsh-tui` packages from a dozen different authors compete. Anything
* matching is a TUI; anything unmatched is `unknown`, which still counts as
* launchable.
*
* `tui` carries word boundaries so the loose arm stays a TOKEN match: `-` and
* `/` are non-word characters, so `@someone/tui-app` and `dsh-tui` both match
* while `intuition` and `gratuitous` do not. Being wrong here is cheap (an
* unmatched profile is `unknown`, which is launchable too) but it decides which
* profile a session boots by DEFAULT, and "the one whose name happens to contain
* t-u-i" is not a rule anyone could predict.
*/
const TUI_BUNDLE_PATTERN = /dsh-tui|dsh-terminal-app|tui/i;
const TUI_BUNDLE_PATTERN = /dsh-tui|dsh-terminal-app|\btui\b/i;
/**
* The profile names DeepSeek itself ships for its non-interactive surfaces.
*
* Consulted only AFTER the bundle patterns have found nothing, and only against
* the directory name. `readProfile()` yields an empty bundle list for any
* `package.json` without a `dsh.profile.bundles` array — a hand-edited file, an
* older layout, a profile mid-install — and with no bundles to read, the stock
* `web` and `headless` profiles look exactly like an unrecognized third-party
* one and inherit its launchable-by-default treatment. That is the single
* "unknown" that is knowably wrong, and it produces precisely the
* pane-dies-on-arrival failure the two-part availability gate exists to prevent.
*
* Deliberately a fallback rather than a first check: a third-party profile that
* legitimately composes a terminal app is identified by its BUNDLES, and its
* directory name (which the user chose) must never override that evidence.
*/
const STOCK_NON_INTERACTIVE_PROFILES = new Map<string, DeepSeekProfileKind>([
['web', 'web'],
['headless', 'headless'],
]);
/** Profile directory names that are not profiles. */
const NON_PROFILE_DIRS = new Set(['node_modules', '.bin', '.pnpm']);
@@ -134,7 +162,7 @@ function classifyProfile(name: string, bundles: string[]): DeepSeekProfileKind {
if (WEB_BUNDLE_PATTERN.test(haystack)) return 'web';
if (HEADLESS_BUNDLE_PATTERN.test(haystack)) return 'headless';
if (TUI_BUNDLE_PATTERN.test(haystack)) return 'interactive';
return 'unknown';
return STOCK_NON_INTERACTIVE_PROFILES.get(name.toLowerCase()) ?? 'unknown';
}
/**