feat(pi): add Pi Coding Agent integration - #692
Conversation
|
@codex plz review |
|
|
|
Findings
The PR description includes a substantive test plan and real-CLI/e2e records, so no testing-description finding is needed. |
The PR description includes a detailed test plan and real-CLI/end-to-end verification record, so it satisfies the testing-documentation requirement. |
|
|
Findings
The PR description includes a detailed test plan and multiple real-CLI/Pi end-to-end records, so it satisfies the testing-documentation requirement. I did not run any code or tests, as requested. |
|
|
Findings
The PR description includes a test plan and real-CLI/Pi end-to-end records, so no testing-description finding is needed. |
The PR description otherwise contains a detailed test plan and real-CLI/e2e records, so it does not lack testing documentation. |
|
Responding to the latest finding:
This is intentional, not a regression, and it's the direct reversal of the previous review pass on this same PR (comment above this one), which said:
Those two findings ask for opposite things against the same constraint: Pi ships one global extension file, and
There's no third option without adding real per-project runtime gating to We went with (2) deliberately, because it's not a novel choice: it's exactly what the OMP adapter already does for the identical single-global-file constraint. On "contradicts the PR description's claimed project-scope preservation": the current PR description doesn't claim that as present-tense behavior — the only place "preserve" appears is a "why removal changed mid-review" paragraph explicitly describing the old, now-replaced behavior and why it was dropped. The committed test at Not changing this for now — happy to revisit if there's a concrete case for building real per-project dispatch gating instead. |
Squash-rebased onto the latest upstream/main to resolve the PR's merge conflict (main gained Tencent#693/Tencent#685/Tencent#694/Tencent#691/Tencent#681/Tencent#680/Tencent#666 since this branch forked). This combines all commits from the PR into one, applied cleanly on top of the new base — no functional changes from the previously reviewed state. The only real conflict was in src/__tests__/uninstall.test.ts, where diff3 split a test mid-body because of the repeated `});` boilerplate around it; resolved by keeping both sides' new tests intact, in full.
9f971c6 to
44bd402
Compare
|
Findings
The PR description includes a detailed test plan and multiple real-CLI/e2e records, so its testing documentation is sufficient. |
…check applyPiAgentHook()/removePiAgentHook() wrote and deleted teamai-agent-<slug>.ts purely by path, with no ownership check — the same class of bug already fixed for the main teamai-hooks.ts file, but never extended to the per-slug HTTP agent-hook files. A user-authored file at that conventional path could be silently overwritten on sync or deleted on uninstall. Adds hasPiAgentHook(slug), mirroring hasPiHooks: injection now skips (with a warning) instead of overwriting a same-named file without the `[teamai] agent hook [<slug>]` marker, and removal skips instead of deleting one. uninstall.ts's discovery scan now derives each file's slug and checks the same marker before scheduling it for removal, instead of matching by filename prefix alone.
|
|
This PR currently has merge conflicts with |
…Pi agent hook applyPiAgentHook warned and returned normally when the requested event has no Pi equivalent or a same-named extension file exists without the TeamAI marker. The caller in local-agent.ts wrote the manifest entry and acked success regardless, so the server and local state believed the hook was installed even though the file was never touched. Throw in both cases so the existing install_hook_rule error path acks failure instead. Also document the known limitation (shared with the OMP adapter) that a scoped Pi uninstall is not durable across multiple projects on the same machine, since the extension is one machine-wide file and hook dispatch has no per-project exclusion check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Conflicts: - README.* (5 languages): upstream relocated the agent-capability table and "Git providers" line out of the READMEs into the new docs/product-overview.md; took upstream's side and added the Pi row (with its footnote) to product-overview.md / .zh-CN.md instead, since the "Learn More" link to Product Overview was already in place. - src/hooks-cmd.ts (hooksList): combined our tool === 'pi' branch with upstream's new isAgentExcluded() skip — excluded agents are now filtered before the Pi-specific branch runs. - src/hooks-cmd.ts (hooksRemove): combined upstream's fix (resolve scopedToolPaths at the hooks' effective scope, not the raw unscoped toolPaths) with our scope/installedBaseDir options, which Pi's self-mode reconciliation needs. Also registered `pi` in ADAPTER_BUILTIN_HOOKS (src/builtin-hooks.ts), mirroring OMP's four keys — a gap the merge surfaced: upstream's new `teamai hooks list` "Built-in hooks (A)" section (Tencent#717) made HookListRow.builtinDefs required, and the Pi row (added before that field existed) had no entry, which manifested as a type error rather than a textual conflict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…idge) upstream/main added a DeepSeek Harness (dsh) adapter alongside Pi in the same reconcile/discovery/removal code paths, causing conflicts in src/hooks.ts and src/uninstall.ts. Kept both adapters' blocks side by side everywhere (reconcileHooksToAllTools's tool switch, ToolResources/ RemovalPlan fields, discovery, isPlanEmpty, plan printing, execution). One spot needed care rather than a mechanical union: the removal-plan merge loop's claudeMdFiles handling. Our branch already carries a fix (from an earlier review round) that skips a shared instruction file still owned by another active, enabled tool — upstream's version of that line predates the fix and is a plain push. Kept our retainedInstructionFiles-aware version, not upstream's, to avoid reintroducing that bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
upstream/main's Tencent#732 fix (probe Copilot hooks via hookToolPaths instead of toolPaths) landed right where our Pi hook-check branch is: both add code before the shared hookPath resolution in buildHookChecks. Pi's branch already returns via `continue` before reaching that point, so kept both blocks in sequence with no logic overlap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Findings
Earlier marker-ownership, timeout propagation, argument correlation, matcher cleanup, EPIPE handling, warning gating, and shared- |
Summary
Part of #617.
This PR adds first-class Pi Coding Agent support for the Phase 1 surface requested in Tencent/teamai-cli#617:
teamai hook-dispatch;Pi MCP and sub-agent formats are outside the Phase 1 surface described in #617.
Pi hook support boundary
Pi supports TeamAI's built-in lifecycle bridge only in this PR. Custom team hooks and built-in hook overrides from
hooks/hooks.yamlare not applied to Pi; reconciliation prints an explicit warning when either is present. README capability tables and both usage guides document this boundary.Full Pi support for
hooks/hooks.yaml, including per-project ownership and cross-scope enable/disable behavior, requires a shared adapter design and will be handled in a follow-up PR.Pi hook mapping
session-startsession_startprompt-submitbefore_agent_startpre-tool-usepayload capturetool_execution_startpost-tool-usetool_execution_endstopagent_settledThe generated extension invokes
teamai hook-dispatch <event> --tool piand forwards the active cwd and event payload. Hook failures are swallowed so a missing or failing TeamAI process does not block Pi.Scope and extension ownership — single-copy policy
Pi auto-loads both
~/.pi/agent/extensionsand<project>/.pi/extensions, and dedups by absolute path — a copy in each root would dispatch every lifecycle event twice. TeamAI writes exactly one copy, ever, in the user agent dir, and never in a project — the same single-copy policy the OMP and OpenCode adapters already use:~/.pi/agent/extensions/teamai-hooks.tsonly, for every scope (user, project, and self mode);init/self mode to commit, so a fresh clone still needs oneteamai init/pullrun on that machine before Pi hooks are active there;hooks remove,uninstall) is gated on the[teamai]marker, so a same-named file without it is never touched;teamai hooks remove, or a scopedteamai uninstall --agent pifrom any scope — deletes the single global extension outright, the same single-file removal semantics as OMP.hook-dispatchhas no per-project exclusion check for any tool (thecwdforwarded on STDIN only lets downstream handlers resolve project context, it doesn't gate whether Pi is excluded there), so a single shared file genuinely has no way to be scoped to one project — see "Why removal changed mid-review" below.teamai hooks listandteamai doctoralways inspect the global lifecycle extension.Why installation changed mid-review: an earlier revision had self mode write and commit its own project copy so a fresh clone could self-bootstrap. That reintroduced the double-dispatch problem this section opens with whenever a global copy also existed on the same machine, and made scoped
uninstall/hooks removeownership ambiguous — two different review passes flagged opposite sides of that same trade-off. The OMP adapter (src/omp-hooks.ts) already made the other choice for the identical dual-root problem: never write a project copy, and accept that self mode doesn't get zero-touch bootstrap on a fresh clone. That design shipped and passed review cleanly, so Pi now follows it instead of carrying its own resolution to the same problem.Why removal changed mid-review (a separate axis from the above): the previous revision had scoped
uninstall/hooks removepreserve the global extension so other projects wouldn't lose their hooks. That preservation was well-intentioned but not actually load-bearing: sincehook-dispatchnever checks per-project exclusion for any tool, the "preserved" extension kept firing session-start/stop/post-tool-use hooks for the very project that had just uninstalled Pi — the scoping it was trying to provide was never enforceable in the first place. OMP's uninstall already makes the honest choice for the identical single-global-file constraint: any targeted removal deletes the one global copy outright, accepting that it affects every project on the machine rather than pretending to scope something unscopable. Pi's removal logic now matches.Cross-cutting fixes (uninstall.ts / hooks.ts, shared by every agent)
These three landed together because they're all instances of the same class of bug: code that decided whether a different tool still "owns" a shared resource by checking whether some file merely existed, instead of checking whether that tool was actually enabled and installed.
retainedInstructionFiles(and the sibling "other tools still have resources" check) now require the other tool to be enabled and actually installed, viaisAgentExcluded+isToolInstalledForConfigagainst a tool-specific probe path (skills/rules/settings — neverclaudemd, since that's exactly the ambiguous shared path in question).toolPathsalways carries every built-in tool's default entry regardless of what a project actually selected, and Hermes/WorkBuddy default to the same projectAGENTS.mdas Pi, so a Pi-only project could never getuninstall --agent pito clean itsAGENTS.mdblock — those never-installed tools always "claimed" it.teamai-agent-*.tsfiles, mirroring OpenCode's existing fallback. Previously it only checked for the mainteamai-hooks.ts; a Pi-only server-pushed agent hook with no lifecycle extension installed was invisible touninstalland left on disk forever.reconcilePiExtensionalready used before writing anything), instead of firing whenever a team defines a Pi-targeted hook regardless of whether the teammate running the command uses Pi at all.Other review fixes
stdinerrorevent before writing, so a missing or short-livedteamaibinary raises an asyncEPIPEthat is swallowed instead of crashing Pi (fixed in both the lifecycle extension and HTTP-source agent hooks).injectPiHooks()now applies the same[teamai]-marker ownership checkremovePiHooks()already had: it skips with a warning instead of overwriting a same-named file that isn't TeamAI-managed, so a user-authored extension at the conventional path is never clobbered byinit/pull.teamai-agent-<slug>.ts):applyPiAgentHook()/removePiAgentHook()previously wrote/deleted these purely by path with no marker check at all. NewhasPiAgentHook(slug)mirrorshasPiHooks();uninstall.ts's discovery scan now derives each file's slug and checks its marker before scheduling removal, instead of matching by filename prefix alone.PI_CODING_AGENT_DIR/PI_CONFIG_DIR), which relocate the agent directory, are out of scope for this adapter — the same limitation the OMP adapter already documents and ships with.tool_execution_startonly caches the tool's input;post-tool-useattool_execution_endforwards that cached input astool_input— there's no separate result/output field, matching OMP's post-tool-use payload shape (nothing inhook-dispatch's handler registry reads a result field for any adapter).PostToolUseagent hooks correlate tool input bytoolCallIdand delete cached input on both matching and non-matching matcher paths..cmdshims can run.Validation
npm run buildnpx tsc --noEmitnpx vitest run— 270 test files / 3756 tests passed (branch rebased onto the latestmain, which added ~100 unrelated tests of its own)uninstall --agent pi, orhooks removefrom any scope) deletes the single global extension outright, mirroring OMP; a scoped pass separately cleans up a legacy project copy first.removePiHooks, and nowinjectPiHooksare all gated on the[teamai]marker — a same-named file without it (global or a legacy project copy) is left untouched on removal, and never overwritten on injection.applyPiAgentHook/removePiAgentHookanduninstall's discovery scan all skip a same-namedteamai-agent-<slug>.tsthat lacks its own[teamai] agent hook [<slug>]marker, both on write and on removal.AGENTS.mdwhen another tool sharing that path is genuinely installed, and now separately removes it when that other tool is only present as atoolPathsdefault and was never actually installed.uninstall --agent piat user scope now discovers and removes a server-pushedteamai-agent-*.tsfile even when the main lifecycle extension is absent.EPIPEfrom the lifecycle extension's or an HTTP agent hook's childstdinresolves cleanly instead of throwing.PostToolUsecleanup deletes cached tool input before returning on a mismatch.45000ms; the default renders as10000ms.git@gitlab.surbanajurong.com:ai-hub/sjg-teamai.git): user scope and project scope pulled skills/rules, injected Pi hooks, anddoctor --jsonpassed.node dist/index.js hooks injectwith one Pi-targeted custom hook plus one built-in override emitted both skip warnings; the custom command was absent from the generated extension and the built-in stop bridge remained present.injection removed a seeded TeamAI-marked legacy project copy; project
teamai hooks removedeleted the global extension; after reinjection, projectteamai uninstall --force --agent piremoved both the legacy project copy and the global extension (updated from an earlier record of this same run, back when scoped removal still preserved the global copy).teamaishim onPATHthat exits immediately without reading stdin (the exactEPIPEtrigger),pi --offline --approve --no-session --no-tools --model openai/nonexistent -p global-extension-smokeran to completion with the expectedNo API key found for openairesult; the shim's log recorded exactly onehook-dispatch session-start --tool picall — confirming single dispatch (no project copy present to double-fire) and that the EPIPE fix holds against a real Pi runtime, not just the unit harness.initSelfRepo's commit skeleton andreconcilePiExtensionno longer contain any code path that writes to<project>/.pi/extensions/for any scope — the only project-path operation left isremovePiProjectHooks(delete-only, legacy cleanup).gitprovider, covering the cross-cutting fixes across Claude/Codex/CodeBuddy/OpenCode alongside Pi:node dist/index.js init . --self --agent claude --agent codex --agent codebuddy --agent opencode --agent pi --forceagainst a real localgit-provider remote (an actualgit push/git-upload-packround trip, not a mock) installed all five cleanly;teamai doctorpassed for all five. Seeding a sharedAGENTS.mdwith a TeamAI block (simulating Hermes/WorkBuddy's defaulttoolPathsentry pointing at the same file, with neither actually installed) and runningteamai uninstall --agent pi --forceremovedAGENTS.mdentirely and left Claude/Codex/CodeBuddy/OpenCode'ssettings.json/hooks.json/skills completely untouched; a follow-updoctorshowed the remaining four agents still fully healthy. This is the scenario the retention-check fix targets, exercised end-to-end through the compiled CLI.claude+pi(same localgit-provider remote) created the global extension; a project-scopedteamai uninstall --agent pi --forcethen removed it entirely (confirmed by direct filesystem check, not just command output), while.claude/settings.jsonand.claude/skillswere untouched.The self-hosted GitLab resource pipeline was verified through the actual repository clone and local provider configuration. Real GitHub- or GitLab-hosted network verification (live API/auth against
github.com/an actual GitLab instance) was not run because no credentials were available in this sandboxed environment — the multi-agent run above used a real localgit-provider remote instead, which exercises the same generic-provider code path and the same shared uninstall/hooks logic this round's fixes changed.