Repository navigation
fix(hermes): launch plugin and status hook through the installed CLI - #1367
Conversation
|
Moved the two POSIX bash/Python runtime probes to hermes-launcher.test.ts, which the normal Linux/macOS suite runs, so the Windows-safe ownership/config test file remains portable. The probes still run and both pass; no assertion was skipped or weakened. Ran the complete Windows test selection locally on macOS: 29 files, 2,908 passed and one existing platform-specific skip. Type checking and lint passed. This is a local selection run, not a claim of native Windows execution; the new CI run checks that host. |
|
No findings in the specified PR diff. The PR description includes sufficient testing: a post-build real-CLI verification covers the generated instructions plugin and status launcher without Review was read-only; I did not run, build, or install PR code. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc The fix works. With teamai off PATH, a build of main (f3cd042) gives Hermes empty instructions and a status hook that never dispatches, while this head delivers the context and dispatches through Node and the CLI entry, also from a home path with a space and an apostrophe. It reuses resolveCliEntry() like #1307 and #1336 and adds no new home resolver. Build, tsc, lint and the PR's own tests pass, and the new tests fail on main. One thing blocks.
[P1 blocking] The Lint & Test jobs are red because of this PR. All six fail on src/__tests__/hermes-hook-command.test.ts (runs a hook in a home containing spaces and apostrophes) with ENOENT on .../receipt. The test puts a stand-in teamai on PATH, and the status command now runs node <entry>, so the stand-in never runs. The test passes on main, and it is the only failure in the full unit suite at this head. Keeping the apostrophe-in-home check while pointing it at a stub entry fixes it: spy on resolveCliEntry to return a small script that writes its argv and stdin to $RECEIPT, and drop the PATH stand-in.
- [P2 non-blocking]
skill-data/core/references/troubleshooting.mdhas the "moved Node or TeamAI, rerun pull" paragraph for OpenCode (from #1336) but none for Hermes, and AGENTS.md asksskill-data/to follow behavior changes. - [P2 non-blocking] The Hermes readiness check (
src/instruction-targets.ts:218) rebuilds the expected plugin from the running CLI's entry, so a plugin installed by one build shows✖indoctorrun from another path. OpenCode has the same design since #1336, and #1365 adds it for Pi; one fix in the readiness check would cover all three. - [P3 nit] Two commit subjects are in Chinese. The squash title is English, so the merged history is unaffected.
#1365 (the Pi half) also appends to the end of both diagnostics.md files, so whichever lands second needs a rebase.
The CI review reports no findings. We disagree: it read only the diff and did not see the red Lint & Test jobs. CI at this head: the six Lint & Test jobs fail as above, so Build is skipped; the GitHub-provider job is skipped by the fork policy.
93b08f7 to
5b79f28
Compare
|
No findings in the specified PR diff. The PR description documents sufficient testing: a post-build real-CLI run exercises the generated Hermes instructions plugin and status launcher without The earlier review reported no findings; there are no prior findings to mark resolved. Review was read-only. I did not run, build, or install PR code. |
|
Updated and rebased onto main Verification: 273 focused unit tests passed; types, lint and build passed. Post-build real-CLI Full unit run: 16,372 passed, 36 skipped, 3 failed. The same three failures reproduce on unchanged main fc8a229: two submodule-checkout empty legacy-file / Git hook-config cases and the workflow-pr-providers child-pipe deadline case (holder.pid missing). This is not a fully green full-suite claim; the focused regression suites above pass. The separate cross-install doctor-readiness fix is #1409. Updated head: |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Approving at 5b79f28. The red Lint & Test is fixed: the test now stubs resolveCliEntry, the PR's unit files pass (273 tests), and all six Lint & Test jobs are green. Troubleshooting now has a Hermes section. The PR's unit and e2e tests fail on current main for the intended reason and pass here. In the e2e, the built CLI runs with an isolated HOME and teamai absent from PATH.
It conflicts with main in skill-data/core/references/troubleshooting.md: both sides append a section at the end. Please rebase; we will merge as soon as CI is green on the rebased head.
Non-blocking notes:
- [P2 non-blocking] doctor's readiness check still rebuilds the plugin from the running CLI, so another install or Node version reports it out of date. You moved this to #1409, which merges cleanly with this branch; it is reviewed there.
- [P3 nit]
buildInstructionsPlugin()now throws when no CLI entry resolves, and the doctor path calls it without a guard. We could not reach that case from a built bundle. - [P3 nit] The warn-and-keep branch in
injectHermesHookshas no test. The three commit subjects are still in Chinese; a squash merge lets the PR title stand instead.
The CI review reports no blocking findings. We agree. All CI jobs pass on this head; the GitHub-provider e2e job is skipped by the fork policy.
- 在 Python 指令插件和状态报告脚本中记录绝对 Node 与 TeamAI 入口，不再依赖 Hermes 服务进程的 PATH 查找命令。 - 保留 shell 引号、Python 参数数组、超时与错误放行；入口不可解析时给出刷新提示，更新中英文排查说明。 - 状态与指令两个回归在修改前失败，并覆盖含空格和引号的入口；23 项相关单测、受限 PATH 的真实 CLI E2E、构建、类型检查和 lint 通过。
- 将真实 bash 与 Python 启动回归集中到独立运行测试文件，继续由 Linux 和 macOS 常规测试执行。 - 保留 Windows 白名单中 Hermes 配置与所有权测试的跨平台契约，不跳过或弱化两项启动断言。 - 本机 Windows 测试集合 2908 项通过、1 项原有平台用例跳过；两项运行探针、类型检查和 lint 通过。
- 使用真实 Node 测试入口代替 PATH 上的 teamai，验证实际生成的绝对路径启动器。 - 保留空格、单引号目录及受限 PATH，补充重新安装后的刷新指引。 - 273 项定向单测及 Hermes 安装入口真实 CLI 场景通过，类型、lint 与构建通过。
- 在已安装 hook 与插件后模拟 CLI 入口不可用，验证脚本、配置、审批和插件内容均保留。 - 断言输出可操作的安装入口警告，补齐评审指出的 warn-and-keep 分支。 - 主干合并后 298 项定向单测、4 项真实 CLI 场景及类型检查、lint、构建通过。
5b79f28 to
46349db
Compare
|
Rebased onto main Types, lint, build and 298 focused unit tests pass. The three post-build real-CLI files (Hermes installed launcher, blocked skills lifecycle, and cross-entry readiness) pass all 4 cases. The body now records this rebased branch. New-head CI is running. |
The PR description documents sufficient testing: a post-build real-CLI verification passes. Disclosed native Windows/vendor/provider coverage limitations are not blocking. Earlier reviews raised no findings, so none require resolution. Review was read-only; I did not run, build, or install PR code. |
- 为依赖系统 Bash 和 Python 的单测与真实 CLI 场景添加平台条件，避免 Windows 上出现 ENOENT。 - 保留 Windows 通用配置与所有权测试，macOS 上三项启动器单测及真实 CLI 场景通过。 - 类型检查和 lint 通过；生产代码与已验证的构建产物未改变。
|
Addressed the new platform-guard note in |
|
No findings in the specified PR diff.
Review was read-only; I did not run, build, or install PR code. |
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 the rebase. Re-checked at this head:
- The conflict in
troubleshooting.mdkeeps main's sections and adds the Hermes one after them. - Outside tests, nothing changed since our approval at 5b79f28.
- The new warn-and-keep test closes our earlier nit.
- Since #1409,
doctoraccepts the generated[node, entry, …]launcher.
With the built CLI and no teamai on PATH, the generated plugin delivered the team instructions, and the status hook ran through the installed entry.
CI review: no findings at this head; we agree. The Windows guards only matter for local runs, since neither file is on the Windows CI allow-list.
Verified at 582aa74:
- Build,
tscand lint pass. - The PR's unit files pass (298 tests), also under a symlinked TMPDIR.
- All 10 e2e files that mention Hermes pass.
- CI is green.
Hermes's instructions plugin used
shutil.which("teamai") or "teamai", and its status script invokedteamaidirectly. A service without the interactive shell's PATH silently lost project context and lifecycle reporting.Generate both launchers with the installed Node executable and CLI entry. Python keeps an argv array and the shell script quotes each path. If the CLI entry cannot be resolved, injection warns and keeps the installed script, configuration, approvals and plugin. Bilingual diagnostics and troubleshooting explain refreshing after Node/TeamAI moves.
Rebased onto
e612a5ae, retaining all main troubleshooting sections and the registration-only/approval-only Hermes tests merged through #1335. The cross-install doctor-readiness change in #1409 is already on main.Validation on this branch:
npx tsc --noEmit,npm run lint,npm run build,git diff --check origin/main...HEAD: passed.npx vitest run src/__tests__/hermes-hooks.test.ts src/__tests__/hermes-hook-command.test.ts src/__tests__/hermes-launcher.test.ts src/__tests__/hermes-config.test.ts src/__tests__/instruction-targets.test.ts src/__tests__/installed-launcher-readiness.test.ts: 298 passed. Includes the new warn-and-keep test, installed-entry shell/Python probes with spaces and an apostrophe in paths, and legacy command/approval cleanup.npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/hermes-installed-launcher.test.ts src/__tests__/e2e/blocked-skills-directory.test.ts src/__tests__/e2e/installed-launcher-readiness.test.ts: 4 passed. The built CLI exercises Hermes instructions and status dispatch without teamai on PATH, blocked-directory repair/uninstall, and doctor from another valid entry.Local verification used macOS, Node 22.22.1 and isolated local Git/HOME fixtures. Native Windows, live Hermes and additional providers remain CI or live-environment coverage.
Refs #1221 (Hermes installed launcher only).
Follow-up at
582aa74c: the two Bash/Python-only probes now use the sameprocess.platform !== "win32"condition ashermes-hook-command.test.ts. Typecheck/lint, the 3 POSIX launcher unit cases and the built-CLI Hermes case pass again on macOS. The production build is unchanged.