BitSquare/Codeman
2
0
Fork 0
mirror of https://github.com/Ark0N/Codeman.git synced 2026-10-04 22:49:41 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(cli-registry): address maintainer review on #380

Browse Source 
Rebased onto current master (the one real conflict was the import line
in docker-hosts.ts Ark0N flagged; kept both), then addressed every
point from the review:

**1. Rebase.** Done — this branch now sits on current upstream/master.

**2. Agent-image special cases are data now, not an id-keyed table
outside stock.ts.** `AGENT_IMAGE_SPECIAL_CASE_IDS`/`AGENT_IMAGE_SPECIAL_CASES`
are gone. `CliDiscovery.install.agentImageLayer?: { kind: 'dedicated';
reason: string }` is a field on the registry entry itself (pi,
deepseek), `reason` is required by schema.ts, both producers
(docker-hosts.ts and cli-catalog.mjs) filter on its presence instead
of an id, and the coverage test reads it from the generated catalogue.
Also added the npm-package-name validation to the TS producer, which
only the .mjs one had — same SAFE_PACKAGE regex, duplicated
(necessarily, one side can't import the other) and now pinned
byte-identical by a new parity test.

**3. Changeset said five, it's eight.** (Not nine — see the DeepSeek
point below, which changes the true count.) Reworded to state it
structurally rather than pin a number that will go stale again.

Then the four behavior-changing findings:

- **DeepSeek was offered as a normal install option but can't actually
  drive a pane.** `npm install -g @deepseek-ai/dsh` installs the
  launcher only; DeepSeek ships no profile that can run standalone.
  The generator now emits an empty install command for any
  `launcherProfile` entry, so install.sh's menu (which requires a
  non-empty command) skips it and falls through to its docs URL hint
  instead — matching what the old hand-written code did before this
  PR replaced it.
- **wget-only hosts lost every automatic install, including the npm
  ones that never needed curl.** The menu-building loop now filters
  PER ENTRY (only a command starting with `curl ` is held back) rather
  than wiping the whole menu when DOWNLOADER != curl.
- **The DISPLAY/TRUSTED split and the catalogue refresh didn't hold up
  under review** (refresh's only real write was the label; it ran
  before the Node existence check; its own eval-detection test was
  tripped by the word "eval'd" in a comment). Dropped entirely per
  your own recommendation — embedded catalogue only, no network
  fetch, no second array. install-sh-invariants.test.ts now asserts
  the refresh/DISPLAY machinery does not exist rather than testing its
  internals.

The three take-or-leave items, applied:

- `dsh_banner_probe`'s bash 3.2 empty-array bug: `${runner[@]}` →
  `${runner[@]+"${runner[@]}"}`. Verified live in a real `bash:3.2.57`
  container with `timeout` removed from PATH — crashed before, clean
  now, full `detect_all_clis` path exercised end to end.
- `docker-agent-image-coverage.test.ts` now anchors on each layer's
  `<binary> --version` proof line instead of `Dockerfile.includes(binary)`,
  which stayed true if a layer were deleted but its comment survived.
- Doc drift: docs/docker-cases.md (four → five, and now describes the
  data field), docker/agent.Dockerfile's "other four CLIs" comment (no
  longer a magic number — CLI_NPM_PACKAGES is generated and can grow),
  CLAUDE.md's install.sh size (104KB → ~112KB) and its stale mention of
  the now-dropped refresh.

Verified: tsc clean, prettier clean, the full targeted suite (142
tests across the 8 affected files) green, and the full `npm test` gate
diffed BY TEST NAME against a clean upstream/master baseline run on
this same machine — identical 201-name failure set both sides (168
tests / 67 files, all pre-existing Windows-environment noise: symlinks,
PTY spawning, POSIX permission bits — none of it touching anything
this PR changes), zero new failures either side of the diff.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R9ZSTEenc8soSu9bTi8Xru
This commit is contained in:
Devvyn
2026-09-13 17:43:14 +08:00
co-authored by Claude Sonnet 5
parent c5c015d648
commit a0628a40e8
16 changed files with 257 additions and 218 deletions
Download Patch File Download Diff File Expand all files Collapse all files
test/agent-image-build-args-parity.test.ts
+18
View File
@@ -77,4 +77,22 @@ describe('agent-image build args: the .mjs and the TS mirror agree', () => {
expect(declared, 'the Dockerfile no longer declares CLI_NPM_PACKAGES').toBeDefined();
expect(declared).toBe(tsPackages().join(' '));
});
it('validates an unsafe package name with the SAME regex on both sides', () => {
// Equal OUTPUT on today's catalogue (asserted above) does not prove equal VALIDATION — a
// looser regex on one side would only show up the day someone ships a hostile package name.
// The regex is duplicated rather than shared (the .mjs side cannot import the .ts side, the
// whole reason this file exists), so pin the literal PATTERN text is identical between the
// two source files rather than trusting the comment that says so.
const tsSource = readFileSync(fileURLToPath(new URL('../src/docker-hosts.ts', import.meta.url)), 'utf-8');
const mjsSource = readFileSync(fileURLToPath(new URL('../scripts/lib/cli-catalog.mjs', import.meta.url)), 'utf-8');
const extract = (source: string, file: string): string => {
// Non-greedy to `/;` deliberately: the pattern itself contains a `/` (inside the
// character class), so a naive `[^/]+` stops at the wrong slash.
const m = /const SAFE_PACKAGE = (\/.+?\/);/.exec(source);
expect(m, `could not find the SAFE_PACKAGE regex literal in ${file}`).toBeDefined();
return m![1];
};
expect(extract(tsSource, 'docker-hosts.ts')).toBe(extract(mjsSource, 'cli-catalog.mjs'));
});
});
test/docker-agent-image-coverage.test.ts
+31 -18
View File
@@ -16,7 +16,7 @@
import { describe, expect, it } from 'vitest';
import { readFileSync } from 'node:fs';
import { fileURLToPath } from 'node:url';
import { AGENT_IMAGE_SPECIAL_CASES, agentImageNpmPackages } from '../scripts/lib/cli-catalog.mjs';
import { agentImageNpmPackages } from '../scripts/lib/cli-catalog.mjs';
import { STOCK_CLIS } from '../src/config/cli-registry/stock.js';
const read = (rel: string): string => readFileSync(fileURLToPath(new URL(`../${rel}`, import.meta.url)), 'utf-8');
@@ -27,40 +27,53 @@ const INDEX_HTML = read('src/web/public/index.html');
const CATALOG = JSON.parse(read('config/clis.stock.json')) as Array<{
id: string;
enabled: boolean;
discovery: { binaries: string[]; install: { npmPackage?: string } };
discovery: {
binaries: string[];
install: { npmPackage?: string; agentImageLayer?: { kind: 'dedicated'; reason: string } };
};
}>;
const enabledAgents = CATALOG.filter((e) => e.enabled && e.discovery.binaries.length > 0);
/**
* A layer's PROOF it installed the right thing, not merely a substring anywhere in the file.
* Every dedicated layer in agent.Dockerfile ends by running `<binary> --version`, so anchoring
* on that (rather than `Dockerfile.includes(binary)`) survives a layer being deleted while its
* COMMENT — which also names the binary — is left behind. That gap is why this replaced the
* looser check.
*/
const hasVersionProof = (binary: string): boolean => AGENT_DOCKERFILE.includes(`${binary} --version`);
describe('docker agent image covers the catalogue', () => {
it('installs every enabled npm CLI, via the build arg or a documented special case', () => {
it('installs every enabled npm CLI, via the build arg or a documented dedicated layer', () => {
const inBuildArg = new Set(agentImageNpmPackages(CATALOG));
const missing: string[] = [];
for (const entry of enabledAgents) {
const pkg = entry.discovery.install.npmPackage;
if (!pkg) continue; // standalone installer, checked below
if (inBuildArg.has(pkg)) continue;
if (entry.id in AGENT_IMAGE_SPECIAL_CASES) continue;
if (entry.discovery.install.agentImageLayer) continue;
missing.push(`${entry.id} (${pkg})`);
}
expect(
missing,
`npm CLI reaches neither the build arg nor a special case:\n ${missing.join('\n ')}\n` +
'Add it to the arg (it is automatic) or give it a Dockerfile layer AND a reason in AGENT_IMAGE_SPECIAL_CASES.'
`npm CLI reaches neither the build arg nor a dedicated layer:\n ${missing.join('\n ')}\n` +
'Add it to the arg (it is automatic) or give it a Dockerfile layer AND an agentImageLayer.reason in stock.ts.'
).toEqual([]);
});
it('gives every special case a reason and a real layer', () => {
for (const [id, reason] of Object.entries(AGENT_IMAGE_SPECIAL_CASES)) {
expect(reason.length, `${id} has an empty reason`).toBeGreaterThan(20);
const entry = CATALOG.find((e) => e.id === id);
expect(entry, `${id} is a special case but not in the catalogue`).toBeDefined();
const pkg = entry?.discovery.install.npmPackage;
// Excluded from the shared arg, so it MUST appear in a hand-written layer, or it is
// simply not installed at all — an exclusion silently becoming an omission.
it('gives every dedicated-layer entry a reason and a real, provable layer', () => {
for (const entry of CATALOG) {
const layer = entry.discovery.install.agentImageLayer;
if (!layer) continue;
expect(layer.reason.length, `${entry.id} has an empty agentImageLayer.reason`).toBeGreaterThan(20);
const binary = entry.discovery.binaries[0];
// Excluded from the shared arg, so it MUST appear in a hand-written layer that actually
// ran the binary, or it is simply not installed at all — an exclusion silently becoming
// an omission.
expect(
AGENT_DOCKERFILE.includes(pkg ?? id),
`${id} is excluded from the arg but absent from the Dockerfile`
hasVersionProof(binary),
`${entry.id} is excluded from the arg but has no "${binary} --version" proof line in the Dockerfile`
).toBe(true);
}
});
@@ -70,8 +83,8 @@ describe('docker agent image covers the catalogue', () => {
if (entry.discovery.install.npmPackage) continue;
const binary = entry.discovery.binaries[0];
expect(
AGENT_DOCKERFILE.includes(binary),
`${entry.id} ships no npm package and no Dockerfile layer mentions "${binary}"`
hasVersionProof(binary),
`${entry.id} ships no npm package and no Dockerfile layer proves it ran "${binary} --version"`
).toBe(true);
}
});
test/install-sh-invariants.test.ts
+30 -27
View File
@@ -93,9 +93,13 @@ describe('install.sh generated-catalogue block', () => {
});
describe('install.sh trust boundary', () => {
// The whole point of splitting TRUSTED from DISPLAY: a command the installer EXECUTES must
// have arrived embedded in this file, over the same TLS fetch and in the same commit as the
// script itself. Anything pulled from the network at install time is display-only.
// A command the installer EXECUTES must have arrived embedded in this file, over the same
// TLS fetch and in the same commit as the script itself — there is no second, network-derived
// copy of these commands anywhere in the script (an earlier draft that added one, and split
// a TRUSTED/DISPLAY pair to keep the fetched copy display-only, was dropped before merge:
// see docs/cli-registry.md). These three assertions are what is left to guard now that the
// fetch path itself does not exist: everything the installer runs or shows still comes only
// from the generated block, and nothing in the file eval()s.
it('writes CLI_INSTALL_CMD_TRUSTED only from the generated per-platform arrays', () => {
const writes = CODE_LINES.filter((line) => /CLI_INSTALL_CMD_TRUSTED\s*\[[^\]]*\]\s*=/.test(line));
expect(writes.length, 'expected exactly the two platform assignments').toBe(2);
@@ -106,23 +110,31 @@ describe('install.sh trust boundary', () => {
}
});
it('never lets the refresh touch a *_TRUSTED array', () => {
const refresh = CODE.slice(CODE.indexOf('cli_catalog_refresh() {'));
const body = refresh.slice(0, refresh.indexOf('\n}\n'));
expect(body.length, 'could not isolate cli_catalog_refresh').toBeGreaterThan(0);
expect(/_TRUSTED\s*\[[^\]]*\]\s*=/.test(body), 'the refresh assigns into a TRUSTED array').toBe(false);
it('fetches no CLI catalogue over the network at install time', () => {
// The exact shape of the earlier, dropped design: a URL built from the repo/branch this
// script came from, an opt-in env var to enable it, and a `download()` call feeding
// straight into the trusted arrays. None of that exists in this file any more; this pins
// the absence so it cannot quietly come back without a reviewer noticing.
for (const needle of [
'cli_catalog_refresh',
'cli_catalog_default_url',
'CODEMAN_CLI_CATALOGUE_URL',
'CODEMAN_REFRESH_CLI_CATALOGUE',
'CLI_INSTALL_CMD_DISPLAY',
]) {
expect(SOURCE.includes(needle), `${needle} should not exist — the catalogue refresh was dropped`).toBe(false);
}
});
it('never eval()s network-derived catalogue data', () => {
// Scoped to the refresh deliberately. install.sh has two long-standing, legitimate evals
// elsewhere (`eval "$(brew shellenv)"`, Homebrew's documented idiom, and one inside a
// node -e that reads `tailscale serve status`), and banning the word outright would flag
// those while saying nothing about the line that matters: `eval` on a fetched file would
// hand the shell to whatever answered the request.
const refresh = CODE.slice(CODE.indexOf('cli_catalog_refresh() {'));
const body = refresh.slice(0, refresh.indexOf('\n}\n'));
expect(body.length, 'could not isolate cli_catalog_refresh').toBeGreaterThan(0);
expect(/\beval\b/.test(body), 'the catalogue refresh eval()s something').toBe(false);
it('never eval()s anything', () => {
// install.sh has two long-standing, legitimate evals (`eval "$(brew shellenv)"`, Homebrew's
// documented idiom, and one inside a node -e that reads `tailscale serve status`), both of
// which operate on output this script itself produced, never on fetched content. With no
// network-derived catalogue left to eval, the word should not appear at all outside those.
const offenders = CODE_LINES.filter(
(line) => /\beval\b/.test(line) && !/eval "\$\(.*shellenv\)"/.test(line) && !line.includes('eval(process.argv')
);
expect(offenders, `unexpected eval:\n ${offenders.join('\n ')}`).toEqual([]);
});
it("redirects stdin for every command it executes on the user's behalf", () => {
@@ -137,15 +149,6 @@ describe('install.sh trust boundary', () => {
});
describe('install.sh runtime safety', () => {
it('guards the catalogue refresh on DOWNLOADER being set', () => {
// DOWNLOADER is assigned only by check_curl_or_wget, which only main() calls. Any path
// that reaches the refresh without it (the `tailscale` subcommand is one) would abort on
// an unbound variable under `set -u` rather than simply skipping the refresh.
const refresh = CODE.slice(CODE.indexOf('cli_catalog_refresh() {'));
const body = refresh.slice(0, refresh.indexOf('\n}\n'));
expect(body).toMatch(/\[\[\s*-n\s*"\$\{DOWNLOADER:-\}"\s*\]\]\s*\|\|\s*return 0/);
});
it('can be sourced without installing anything', () => {
// The bash 3.2 CI step sources this file to exercise detect_all_clis. Without the guard
// the dispatch `case` at the tail would run a real install inside the container.