mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-02 13:39:41 +02:00
fix(pi): align the doctor with the pi resolver, correct the strip rationale, update the skill
Second review pass on #282, the three items left open after f4dcfbe.
1. `codeman doctor` and the run mode disagreed about pi. The registry entry
accepted a bare `which pi` hit while pi-cli-resolver demanded semver-shaped
`--version` output, so the Dependencies panel could report an installed Pi CLI
on a box where Run Pi stays hidden, which reads as a broken mode rather than a
missing install. Both sides now share one exported PI_VERSION_REGEX, and
PathResolver gains an opt-in `requireVersionMatch` so a binary that fails the
shape check is reported MISSING instead of installed-with-unknown-version.
Only pi sets it; every other tool keeps its current behaviour.
2. The isAltScreenStripMode comment justified excluding pi with "the alt screen
is load-bearing for its fullscreen TUI". That is not what exclusion does: pi
is tmux-backed, so it falls through to isMuxAltScreenOnlyStripMode, which
strips the alt-screen toggles anyway. What exclusion actually preserves is
`\x1b[3J` and the mouse DECSETs, which is the real reason (pi renders into the
main screen and is mouse-aware). Comment and changeset now say that, and state
the consequence: fullscreen pi paints into the main buffer, like vim in a tmux
shell session.
3. skills/codeman still enumerated the five pre-pi modes in nine places, telling
agents a backend does not exist and understating class-wide caveats by one
mode. All updated, plus stale session.ts line references refreshed.
Tests: a new static guard derives the mode set from the Zod schema (not a copy)
and fails when a skill enumeration lists a partial set of external CLIs, verified
by mutation. It also documents the one legitimate exception it found: the "writes
no transcript" lists drop codex, which does write a rollout Codeman reads back.
Plus doctor cases for an unrelated `pi` on PATH and registry/resolver regex parity.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -7,6 +7,8 @@
|
||||
* @module config/dependency-registry
|
||||
*/
|
||||
|
||||
import { PI_VERSION_REGEX } from '../utils/pi-cli-resolver.js';
|
||||
|
||||
export type ProbeEnvironment = 'linux' | 'darwin' | 'win32' | 'wsl';
|
||||
|
||||
/** The valid `--category` filter values; single source of truth for the type, the CLI
|
||||
@@ -20,6 +22,13 @@ export interface PathResolver {
|
||||
bins: string[];
|
||||
versionArg?: string; // default '--version'
|
||||
versionRegex?: RegExp; // default matches first \d+.\d+(.\d+)?
|
||||
/**
|
||||
* Treat a binary whose version output does not match as NOT INSTALLED, instead of
|
||||
* reporting it with an unknown version. Only for tools with a short, generic binary
|
||||
* name (`pi`), where a `which` hit is not by itself evidence the right program is
|
||||
* there and a false "installed" contradicts the run mode's own resolver.
|
||||
*/
|
||||
requireVersionMatch?: boolean;
|
||||
}
|
||||
|
||||
/** Resolve a Windows-installed app reachable from win32 or WSL. */
|
||||
@@ -112,7 +121,23 @@ export const DEPENDENCY_REGISTRY: ToolDependency[] = [
|
||||
category: 'core',
|
||||
required: false,
|
||||
usedBy: ['Pi sessions'],
|
||||
resolvers: [{ match: ALL, resolver: { kind: 'path', bins: ['pi'], versionArg: '--version' } }],
|
||||
// The only entry that requires a version match, for the same reason
|
||||
// pi-cli-resolver.ts probes: `pi` is a short generic name (Raspberry Pi tooling,
|
||||
// personal scripts), so a `which pi` hit alone is not the coding agent. Both sides
|
||||
// share PI_VERSION_REGEX, so the doctor and the run mode cannot drift into telling
|
||||
// the user opposite things about the same binary.
|
||||
resolvers: [
|
||||
{
|
||||
match: ALL,
|
||||
resolver: {
|
||||
kind: 'path',
|
||||
bins: ['pi'],
|
||||
versionArg: '--version',
|
||||
versionRegex: PI_VERSION_REGEX,
|
||||
requireVersionMatch: true,
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{
|
||||
id: 'libreoffice',
|
||||
|
||||
+14
-5
@@ -194,11 +194,20 @@ function getModeLabel(mode: SessionMode): string {
|
||||
* repaint via cursor positioning, so dropping the alt-screen switch is safe —
|
||||
* content stays in the normal buffer. Excluded: `shell` (arbitrary programs like
|
||||
* vim/less/htop legitimately need the alt screen), `opencode` (renders its own
|
||||
* TUI that may rely on it) and `pi` (its default TUI already renders into the
|
||||
* MAIN screen with terminal-owned scrollback, so there is nothing to strip — and
|
||||
* since pi 0.84.0 the user can switch to a fullscreen TUI at runtime via
|
||||
* `/settings`, where the alt screen is load-bearing). Keep parity with the
|
||||
* replay-side strip in session-routes.ts.
|
||||
* TUI that may rely on it) and `pi` (below). Keep parity with the replay-side
|
||||
* strip in session-routes.ts.
|
||||
*
|
||||
* ⚠️ Being excluded here does NOT preserve the alt screen. Every excluded mode
|
||||
* falls through to isMuxAltScreenOnlyStripMode(), which strips the alt-screen
|
||||
* toggles too whenever the session is tmux-backed, and pi/opencode ALWAYS are
|
||||
* (both refuse the direct-PTY fallback). What exclusion actually buys is the rest
|
||||
* of the full strip: `\x1b[3J` and the mouse-tracking DECSETs survive. That is the
|
||||
* real reason pi is out: its default TUI renders into the MAIN screen with
|
||||
* terminal-owned scrollback and is mouse-aware, so it is a `3J`/mouse consumer in
|
||||
* a way an Ink TUI repainting in place is not. Consequence to know before
|
||||
* debugging it: pi's runtime-switchable fullscreen TUI (`/settings`, 0.84.0+)
|
||||
* still gets its `?1049h` stripped and paints into the main buffer, exactly like
|
||||
* vim inside a tmux `shell` session.
|
||||
*/
|
||||
export function isAltScreenStripMode(mode: SessionMode): boolean {
|
||||
return mode === 'codex' || mode === 'claude' || mode === 'gemini';
|
||||
|
||||
@@ -94,12 +94,17 @@ export function checkTool(tool: ToolDependency, host: ProbeHost): ToolResult {
|
||||
if (!spec) return { ...base, status: 'skipped', reason: `not applicable on ${host.environment}` };
|
||||
|
||||
if (spec.resolver.kind === 'path') {
|
||||
const { bins, versionArg, versionRegex } = spec.resolver;
|
||||
const { bins, versionArg, versionRegex, requireVersionMatch } = spec.resolver;
|
||||
for (const bin of bins) {
|
||||
const resolved = host.which(bin);
|
||||
if (resolved) {
|
||||
const out = host.runVersion(bin, [versionArg ?? '--version']);
|
||||
const version = out ? extractVersion(out, versionRegex) : undefined;
|
||||
// A generic binary name that prints the wrong thing is some OTHER program (see
|
||||
// PathResolver.requireVersionMatch). Keep looking, then report MISSING; the
|
||||
// alternative is claiming a tool is installed that the feature's own resolver
|
||||
// rejects, which reads as "the mode is broken" rather than "install it".
|
||||
if (requireVersionMatch && !version) continue;
|
||||
return finalize(base, tool, resolved, version);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -30,8 +30,20 @@ const PI_SEARCH_DIRS = [
|
||||
join(homedir(), 'bin'),
|
||||
];
|
||||
|
||||
/** A real `pi --version` prints a semver-shaped string (e.g. `0.84.1`). */
|
||||
const PI_VERSION_PATTERN = /^\d+\.\d+\.\d+/;
|
||||
/**
|
||||
* A real `pi --version` prints a semver-shaped string (e.g. `0.84.1`).
|
||||
*
|
||||
* Exported and SHARED with the `pi` entry in `config/dependency-registry.ts`, so
|
||||
* `codeman doctor` and the run mode cannot disagree about what counts as an installed
|
||||
* pi: two copies of this rule would let the Dependencies panel report "Pi CLI ✓" on a
|
||||
* box where `resolvePiDir()` rejects the same binary and Run Pi stays hidden.
|
||||
*
|
||||
* Shape is dictated by the doctor's `extractVersion()`, which returns the first CAPTURE
|
||||
* GROUP and scans the whole output: hence a capturing group, and a leading boundary
|
||||
* instead of `^` so `pi 0.84.1` matches while `v0.84.1` (some other program) does not.
|
||||
* No `g` flag, so there is no shared `lastIndex` to reset.
|
||||
*/
|
||||
export const PI_VERSION_REGEX = /(?:^|\s)(\d+\.\d+\.\d+)/;
|
||||
|
||||
/** Cached directory containing the pi binary (empty string = searched but not found) */
|
||||
let _piDir: string | null = null;
|
||||
@@ -56,7 +68,7 @@ function probePiVersion(binPath: string): string | null {
|
||||
stdio: ['ignore', 'pipe', 'ignore'],
|
||||
}).trim();
|
||||
// Upstream prints a bare version today; tolerate a `pi 0.84.1` style prefix too.
|
||||
const candidate = out.split(/\s+/).find((token) => PI_VERSION_PATTERN.test(token));
|
||||
const candidate = PI_VERSION_REGEX.exec(out)?.[1];
|
||||
if (candidate) return candidate;
|
||||
console.warn(`[PiResolver] Ignoring ${binPath}: "pi --version" printed ${JSON.stringify(out.slice(0, 80))}`);
|
||||
} catch (err) {
|
||||
|
||||
Reference in New Issue
Block a user