Repository navigation
fix(doctor): reject invalid saved hook launcher paths - #1431
Conversation
|
Full local validation is complete at |
|
No findings in the specified PR diff. The PR description satisfies the testing requirement: it records a post-build real-CLI E2E run covering valid cross-installation launchers, invalid saved paths, and recovery. Additional platform/provider coverage is explicitly disclosed as untested locally and is not blocking. Review was read-only; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks for following up on the #1409 note. With the built CLI, installing through one entry and running doctor through another:
- A directory as the CLI entry,
/bin/shas the runtime, and swapped runtime and entry paths are now rejected. All three passed on main. - A valid launcher with spaces in its entry path still passes, and so does a symlink named
node. doctorleaves the hook file byte-identical.- The new tests fail on main and pass here.
One regression to fix:
[P1 blocking] Healthy launchers on a Node binary named node-22 (Fedora) are now rejected. teamai saves the runtime as process.execPath, which is the resolved binary. Fedora's nodejs22 package installs the binary as /usr/bin/node-22, with node as a link to it, so the saved launcher holds NODE = "/usr/bin/node-22". The new check (src/utils/hook-source.ts:44) accepts only node or bun (optionally with .exe).
With the built CLI, hooks installed through a Node copy named node-22 pass the check on main and fail it here: doctor reports the hook check as not ready. Pull and the local agent then treat the hook channel as not ready too, which undoes #1409 on that layout. A copy named nodejs fails the same way.
A pattern such as /^(node(js)?(-\d+)?|bun)(\.exe)?$/i would cover these. Add a node-22 unit case, and widen the en/zh diagnostics line to match.
[P3 nit] The new unit file is not on the Windows CI allow-list (src/__tests__/helpers/windows-tests.ts), so accepting node.exe is never tested on Windows.
CI review: it reported no findings. We agree on the testing record, and disagree only on the regression above, which we reproduced with the built CLI.
Verified at 8930dcd:
tscand lint pass.- 394 unit tests and 56 e2e tests across the 6 related files pass, and the new file also passes under a symlinked TMPDIR.
- CI is green.
Not run here: a real Fedora host, native Windows, and real Bun, Volta or asdf binaries.
- 对另一安装记录的启动路径检查实际文件类型，目录不再被判定为有效入口。 - 保留运行时与 CLI 入口的角色，要求运行时名称为 node 或 bun，拒绝路径互换和 shell 冒充。 - 四项新增回归先失败后通过；333 项相关单测与构建后的真实 CLI 验证通过。 - 同步中英文诊断说明，明确跨安装兼容依据为处理逻辑一致。
- 接受 nodejs、node-22 等有效运行时名称及 .exe 形式，继续拒绝目录、路径互换和 shell 运行时。 - 新增名称兼容回归并纳入 Windows CI 清单，适配主干统一的 Pi hook 路径定位接口。 - 四项新增名称断言及两项真实 CLI 用例先失败后通过；382 项相关测试、类型检查、lint 和构建通过。 - 同步中英文诊断文档，说明合法运行时名称与版本后缀。
8930dcd to
69555c4
Compare
|
@SaulMoro Fixed the blocking runtime-name regression at
382 related unit tests, types, lint, build and both post-build real-CLI cases pass. Native Windows CI passed with the newly listed test file. The full local suite has completed: 17,328 passed, 36 skipped, 4 failed. Three failures reproduce on untouched main (50 passed / the same 3 failed); the fourth is a runner-start timing failure whose focused case and complete 31-test file pass on both main and this branch when rerun. The full run is not claimed as green. The PR body now records all results and platform limits. |
|
I couldn’t complete the exact-diff review: this checkout is shallow, The supplied PR description satisfies the testing requirement: it records a post-build real-CLI E2E run and discloses untested platforms/providers. The older validation commit is not blocking under the review rules. Please provide the exact three-dot diff or the missing base history. I did not run, build, install, or modify anything from the PR. |
|
The automated review could not find the merge base in its shallow checkout. From a complete local clone, the exact requested comparison is Exact three-dot diff for 69555c4diff --git a/docs/guide/diagnostics.md b/docs/guide/diagnostics.md
index aab89e7a..ebfe4269 100644
--- a/docs/guide/diagnostics.md
+++ b/docs/guide/diagnostics.md
@@ -17,7 +17,7 @@ teamai remove rules <name> --force
`doctor` exits 1 when any check fails. `stats` adds a recall section when that scope has recall runs. See [Recall adoption and upvotes](./knowledge.md#recall-adoption-and-upvotes).
-Generated-hook checks compare the handler logic and accept an existing absolute launcher recorded by another installation of the same build. Running `doctor` from a different CLI path alone does not require reinstalling hooks. A missing saved launcher or changed handler still requires `teamai hooks inject` or `teamai pull`.
+Generated-hook checks accept a launcher from another installation when the handler logic matches. Running `doctor` from a different CLI path alone does not require reinstalling hooks. The saved paths must be files, and the runtime must be named `node`, `nodejs`, or `bun`. Node names may have a numeric version suffix such as `node-22` or `nodejs-22`; `.exe` names are also accepted. An invalid saved launcher or changed handler still requires `teamai hooks inject` or `teamai pull`.
If a file occupies a tool's skills directory, `pull` keeps it, names the blocked path, and continues delivering to other tools. `doctor` fails the skills-directory check instead of treating the skipped tool as delivered. Move the file aside, then run `teamai pull` again to deliver the missing skills. `uninstall` preserves the blocking file and continues cleaning the tool's other managed resources.
diff --git a/docs/guide/zh-CN/diagnostics.md b/docs/guide/zh-CN/diagnostics.md
index 94bd0d60..7f7ff4de 100644
--- a/docs/guide/zh-CN/diagnostics.md
+++ b/docs/guide/zh-CN/diagnostics.md
@@ -17,7 +17,7 @@ teamai remove rules <name> --force
`doctor` 有一项没过就以状态码 1 退出。`stats` 在有 recall 记录时会多一节，见 [Recall 采纳与 upvote](./knowledge.md#recall-采纳与-upvote)。
-生成 hook 的检查会比较处理逻辑，并接受同一构建的另一份安装记录的有效绝对启动路径。仅换一个 CLI 路径运行 `doctor` 不需要重装 hooks；记录的启动入口不存在或处理逻辑发生变化时，仍需运行 `teamai hooks inject` 或 `teamai pull`。
+生成 hook 的检查会比较处理逻辑，并接受处理逻辑相同的另一份安装记录的启动路径。仅换一个 CLI 路径运行 `doctor` 不需要重装 hooks；记录的路径必须指向文件，运行时名称必须为 `node`、`nodejs` 或 `bun`。Node 名称可带数字版本后缀，例如 `node-22` 或 `nodejs-22`；也接受带 `.exe` 的名称。启动入口无效或处理逻辑发生变化时，仍需运行 `teamai hooks inject` 或 `teamai pull`。
如果工具的 skills 目录被普通文件占用，`pull` 会保留该文件、指出阻塞路径，并继续向其他工具分发。`doctor` 会将该目录检查标为失败，避免把跳过的投递误报为成功。移开该文件后，再运行 `teamai pull` 即可补齐缺失的技能。`uninstall` 会保留该文件，并继续清理该工具的其他托管资源。
diff --git a/skill-data/core/references/troubleshooting.md b/skill-data/core/references/troubleshooting.md
index 229adcab..8fe394ff 100644
--- a/skill-data/core/references/troubleshooting.md
+++ b/skill-data/core/references/troubleshooting.md
@@ -448,7 +448,7 @@ Session archive writers (`session push`, `migrate --push`, and `session pull`) w
## OpenCode hooks cannot start the CLI
-If OpenCode hooks stopped after moving or reinstalling Node or TeamAI, run `teamai hooks inject` or `teamai pull` and restart OpenCode. The plugin records the installed Node and CLI paths so it also works without `teamai` on the service PATH. Doctor accepts those live paths when another installation of the same build inspects the plugin; missing saved paths or a changed handler still fail the check.
+If OpenCode hooks stopped after moving or reinstalling Node or TeamAI, run `teamai hooks inject` or `teamai pull` and restart OpenCode. The plugin records the installed Node and CLI paths so it also works without `teamai` on the service PATH. Doctor accepts those live paths when another installation with the same handler inspects the plugin; invalid saved launcher files or a changed handler still fail the check.
## ZCode registered hooks but missing project rules
diff --git a/src/__tests__/e2e/installed-launcher-readiness.test.ts b/src/__tests__/e2e/installed-launcher-readiness.test.ts
index da1a0281..198e9914 100644
--- a/src/__tests__/e2e/installed-launcher-readiness.test.ts
+++ b/src/__tests__/e2e/installed-launcher-readiness.test.ts
@@ -6,7 +6,7 @@ import { fileURLToPath, pathToFileURL } from 'node:url';
import YAML from 'yaml';
import { expect, it } from 'vitest';
const CLI = fileURLToPath(new URL('../../../dist/index.js', import.meta.url));
-it('doctor accepts the live launcher saved by another entry to the same built CLI', () => {
+it.each(['node-22', 'nodejs'])('doctor accepts a live launcher installed through %s and inspected through another CLI entry', name => {
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-readiness-cli-'));
try {
const home = path.join(root, 'home');
@@ -27,11 +27,15 @@ it('doctor accepts the live launcher saved by another entry to the same built CL
execFileSync('git', ['add', '.'], { cwd: team, env });
execFileSync('git', ['commit', '-qm', 'fixture'], { cwd: team, env });
const cli = (entry: string, ...args: string[]) => spawnSync(process.execPath, [entry, ...args], { cwd: project, env, encoding: 'utf8', timeout: 30000 });
- const installed = cli(first, 'hooks', 'inject');
+ const runtime = path.join(root, process.platform === 'win32' ? `${name}.exe` : name);
+ fs.copyFileSync(process.execPath, runtime);
+ const installed = spawnSync(runtime, [first, 'hooks', 'inject'], { cwd: project, env, encoding: 'utf8', timeout: 30000 });
expect(installed.status, installed.stdout + installed.stderr).toBe(0);
const file = path.join(home, '.pi/agent/extensions/teamai-hooks.ts');
const before = fs.readFileSync(file, 'utf8');
expect(before).toContain(first);
+ const runtimeLine = `const NODE = ${JSON.stringify(fs.realpathSync(runtime))};`;
+ expect(before).toContain(runtimeLine);
const ready = () => {
const run = cli(second, 'doctor', '--json');
const report = JSON.parse(run.stdout) as { checks: Array<{ name: string; ok: boolean }> };
@@ -39,7 +43,19 @@ it('doctor accepts the live launcher saved by another entry to the same built CL
};
expect(ready()).toBe(true);
expect(fs.readFileSync(file, 'utf8')).toBe(before);
+ const shell = path.join(root, 'sh');
+ fs.writeFileSync(shell, '#!/bin/sh\n');
+ fs.writeFileSync(file, before.replace(runtimeLine, `const NODE = ${JSON.stringify(shell)};`));
+ expect(ready()).toBe(false);
+ fs.writeFileSync(file, before.replace(runtimeLine, `const NODE = ${JSON.stringify(first)};`)
+ .replace(`const ENTRY = ${JSON.stringify(first)};`, `const ENTRY = ${JSON.stringify(runtime)};`));
+ expect(ready()).toBe(false);
+ fs.writeFileSync(file, before);
+ expect(ready()).toBe(true);
fs.unlinkSync(first);
+ fs.mkdirSync(first);
+ expect(ready()).toBe(false);
+ fs.rmdirSync(first);
expect(ready()).toBe(false);
} finally { fs.rmSync(root, { recursive: true, force: true }); }
}, 60000);
diff --git a/src/__tests__/helpers/windows-tests.ts b/src/__tests__/helpers/windows-tests.ts
index 18226fb6..902b5b72 100644
--- a/src/__tests__/helpers/windows-tests.ts
+++ b/src/__tests__/helpers/windows-tests.ts
@@ -17,6 +17,7 @@ export const WINDOWS_TESTS = [
'src/__tests__/hooks-shell-check.test.ts',
'src/__tests__/hooks-skill-retire.test.ts',
'src/__tests__/hooks-windows-bash.test.ts',
+ 'src/__tests__/installed-launcher-readiness.test.ts',
'src/__tests__/joycode.test.ts',
'src/__tests__/locate-agents.test.ts',
'src/__tests__/locate-blocks.test.ts',
diff --git a/src/__tests__/installed-launcher-readiness.test.ts b/src/__tests__/installed-launcher-readiness.test.ts
index e9367ddd..cadb8aff 100644
--- a/src/__tests__/installed-launcher-readiness.test.ts
+++ b/src/__tests__/installed-launcher-readiness.test.ts
@@ -12,6 +12,7 @@ let entry: string;
beforeEach(() => {
home = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-launcher-check-'));
vi.stubEnv('HOME', home); vi.stubEnv('USERPROFILE', home);
+ vi.stubEnv('PI_CODING_AGENT_DIR', '');
vi.stubEnv('OPENCODE_CONFIG_DIR', ''); vi.stubEnv('XDG_CONFIG_HOME', '');
entry = path.join(home, 'other install.mjs');
fs.writeFileSync(entry, '// CLI entry in another installation');
@@ -46,6 +47,45 @@ it.each(['bun', 'hermes'])('recognizes %s launcher metadata without ignoring han
: `${value.slice(0, -1)}, "hook-dispatch", "instructions", "--tool", "hermes"]`;
expect(await hookSourceMatches(source(launcher), source(current))).toBe(true);
expect(await hookSourceMatches(source(launcher) + '\nchanged();', source(current))).toBe(false);
+ expect(await hookSourceMatches(source(JSON.stringify([entry, process.execPath])), source(current))).toBe(false);
fs.unlinkSync(entry);
+ fs.mkdirSync(entry);
expect(await hookSourceMatches(source(launcher), source(current))).toBe(false);
+ fs.rmdirSync(entry);
+ expect(await hookSourceMatches(source(launcher), source(current))).toBe(false);
+});
+
+it.each(['node', 'node-22', 'nodejs', 'nodejs-22', 'node.exe', 'node-22.exe', 'bun', 'bun.exe'])('accepts a Pi launcher saved with the %s runtime name', async name => {
+ const runtime = path.join(home, name);
+ fs.writeFileSync(runtime, 'runtime fixture');
+ const file = path.join(hookAdapterDir('pi-extension'), PI_HOOK_FILE);
+ fs.mkdirSync(path.dirname(file), { recursive: true });
+ const source = buildPiExtensionSource(entry)
+ .replace(`const NODE = ${JSON.stringify(process.execPath)};`, `const NODE = ${JSON.stringify(runtime)};`);
+ fs.writeFileSync(file, source);
+ expect((await channelReadiness('pi', 'pi-extension')).ready).toBe(true);
+ expect(fs.readFileSync(file, 'utf8')).toBe(source);
+});
+
+it.each(['entry directory', 'runtime directory', 'swapped paths', 'shell runtime'])('rejects a Pi launcher with %s', async kind => {
+ const file = path.join(hookAdapterDir('pi-extension'), PI_HOOK_FILE);
+ fs.mkdirSync(path.dirname(file), { recursive: true });
+ let source = buildPiExtensionSource(entry);
+ if (kind === 'entry directory') {
+ fs.unlinkSync(entry);
+ fs.mkdirSync(entry);
+ } else if (kind === 'runtime directory') {
+ const runtime = path.join(home, 'node');
+ fs.mkdirSync(runtime);
+ source = source.replace(`const NODE = ${JSON.stringify(process.execPath)};`, `const NODE = ${JSON.stringify(runtime)};`);
+ } else if (kind === 'swapped paths') {
+ source = source.replace(`const NODE = ${JSON.stringify(process.execPath)};`, `const NODE = ${JSON.stringify(entry)};`)
+ .replace(`const ENTRY = ${JSON.stringify(entry)};`, `const ENTRY = ${JSON.stringify(process.execPath)};`);
+ } else {
+ const runtime = path.join(home, 'sh');
+ fs.writeFileSync(runtime, 'shell runtime fixture');
+ source = source.replace(`const NODE = ${JSON.stringify(process.execPath)};`, `const NODE = ${JSON.stringify(runtime)};`);
+ }
+ fs.writeFileSync(file, source);
+ expect((await channelReadiness('pi', 'pi-extension')).ready).toBe(false);
});
diff --git a/src/utils/hook-source.ts b/src/utils/hook-source.ts
index 9033182e..0a54bf73 100644
--- a/src/utils/hook-source.ts
+++ b/src/utils/hook-source.ts
@@ -1,26 +1,30 @@
import path from 'node:path';
-import { pathExists } from './fs.js';
+import { stat } from 'node:fs/promises';
/** Compare generated handlers without requiring the inspecting CLI's launcher. */
export async function hookSourceMatches(actual: string | null, expected: string): Promise<boolean> {
if (actual === null) return false;
if (actual === expected) return true;
- const normalize = (source: string): { body: string; paths: string[] } | null => {
+ const normalize = (source: string): { body: string; paths: string[]; runtimes: string[] } | null => {
const paths: string[] = [];
+ const runtimes: string[] = [];
let valid = true;
- const launcher = (encoded: string): void => {
+ const launcher = (encoded: string, runtime = true): void => {
try {
const parsed: unknown = JSON.parse(encoded);
const values: unknown = typeof parsed === 'string' ? [parsed] : parsed;
if (!Array.isArray(values)
|| !values.every((value): value is string => typeof value === 'string' && path.isAbsolute(value))) {
valid = false;
- } else paths.push(...values);
+ } else {
+ paths.push(...values);
+ if (runtime && values[0]) runtimes.push(values[0]);
+ }
} catch { valid = false; }
};
const jsonString = '"(?:[^"\\\\]|\\\\.)*"';
let body = source.replace(new RegExp(`^const (NODE|ENTRY) = (${jsonString});$`, 'gm'), (_line, name: string, value: string) => {
- launcher(value);
+ launcher(value, name === 'NODE');
return `const ${name} = "<installed-launcher>";`;
});
body = body.replace(/^const launcher = (\[[^\n]+\]);$/gm, (_line, values: string) => {
@@ -31,11 +35,12 @@ export async function hookSourceMatches(actual: string | null, expected: string)
launcher(`[${node},${entry}]`);
return '["<installed-launcher>", "hook-dispatch", "instructions", "--tool", "hermes"]';
});
- return valid ? { body, paths } : null;
+ return valid ? { body, paths, runtimes } : null;
};
const installed = normalize(actual);
const current = normalize(expected);
return installed !== null && current !== null && installed.paths.length > 0
&& installed.paths.length === current.paths.length && installed.body === current.body
- && (await Promise.all(installed.paths.map(pathExists))).every(Boolean);
+ && installed.runtimes.every(file => /^(node(js)?(-\d+)?|bun)(\.exe)?$/i.test(path.basename(file)))
+ && (await Promise.all(installed.paths.map(file => stat(file).then(info => info.isFile(), () => false)))).every(Boolean);
}The native Windows CI job for this head has now passed, including the new readiness file. I am also preparing a separate fix for the review workflow's shallow-history setup. |
|
The shallow-history workflow fix is now isolated in #1449 (two lines). A real Git reproduction confirms the old checkout fails with |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, both items are resolved. With the built CLI, installing through copies of Node named nodejs and node-22 and running doctor through a second entry, the hook check now reports ready for both; the previous head failed both. A directory as the entry, /bin/sh as the runtime and swapped paths are still rejected, and doctor leaves the hook file byte-identical. The unit file is now on the Windows CI list, and that job ran it.
CI review: it could not diff this head because its checkout is shallow, which #1449 addresses. We agree with its point that the testing record is sufficient.
Verified at 69555c4: build, tsc and lint pass; the 6 related unit files (435 tests, the new file also under a symlinked TMPDIR) and 6 related e2e files pass; CI is green. Not run here: a real Fedora host, native Windows, and real Bun, Volta or asdf installs.
Doctor accepted a hook launcher from another installation whenever its saved absolute paths existed. A directory at the CLI entry, a directory named
node, swapped runtime/entry paths, or a shell used as the runtime could therefore appear healthy. Recall toggles and local-agent fallback decisions also use this readiness result.Require the saved paths to be files and identify the runtime separately from the entry. Accept
node,nodejs, numeric Node suffixes such asnode-22andnodejs-22, andbun, including.exeforms. This preserves working distribution-specific Node installations while rejecting invalid launcher roles. Handler bodies must still match, and inspection leaves installed files unchanged. Bilingual diagnostics describe compatibility in terms of the same handler rather than the same build.Follow-up to the P2 review note on #1409. Refs #1221.
Validation at
69555c4e, rebased onto main8ee3ca5a:node-22,nodejs,nodejs-22,node-22.exe) fail. The two real-CLI cases also fail their readiness assertion when installing through actual Node binaries copied under the namesnode-22andnodejs.npx vitest run --maxWorkers=2 src/__tests__/installed-launcher-readiness.test.ts src/__tests__/instruction-targets.test.ts src/__tests__/opencode-hooks.test.ts src/__tests__/hermes-hooks.test.ts src/__tests__/windows-test-list.test.ts: 382 passed. The readiness tests are now included in the Windows CI allow-list, and the native Windows job passed at this head; shell rejection uses a plain file fixture and requires no POSIX shell.npx tsc --noEmit,npm run lint,npm run build,git diff --check: passed.npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/installed-launcher-readiness.test.ts: 2 passed. The real CLI installs through each renamed Node binary and entry A, then runs doctor through the normal Node binary and entry B. Valid paths pass without rewriting; shell runtime, swapped paths, a directory entry and a missing entry fail; restoring valid paths passes again. The installation path contains spaces and an apostrophe.npx vitest run --maxWorkers=4finished in 950.21s: 17,328 passed, 36 skipped, 4 failed across 583 files (579 passed, 1 skipped, 3 failed). Three failures reproduce on unchanged main8ee3ca5a: the twosubmodule-checkoutempty legacy-file/Git hook-config assertions and theworkflow-pr-providerschild-pipe deadline fixture (holder.pidabsent); that baseline run gives 50 passed / the same 3 failed. The fourth was aworkflow-skill-driverrunner-start timing failure (sleep.pidabsent). Its focused case and the complete 31-test file both pass when rerun on unchanged main and on this branch. No test was changed to obtain those rerun results; the complete run itself was not fully green.Local verification: macOS, Node 22.22.1, isolated Git/HOME fixtures. Native Fedora, Windows, live vendor applications, actual Bun/Volta/asdf distributions and additional providers were not run locally; Windows CI now exercises the new unit file. The filename/type checks validate readiness, not executable contents.