Repository navigation
fix(uninstall): remove the machine-wide hooks when nothing else uses them - #1826
Walternater wants to merge 0 commit into
Conversation
The description includes sufficient representative real-CLI verification for uninstall, covering both removal and preservation. Nothing from the PR was run, built, or installed. |
b314494 to
b27e94c
Compare
|
The
The gate is on the scope again: a user-scope removal leaves the other projects' data; a project removal on the last install has no other project to leave. The new case asserts the data home is gone as well — the first version only looked at the hook file, which is why it passed while the ledger failed. Locally, on this head: |
Both earlier findings remain unresolved. The description documents sufficient representative real-CLI verification for removal and preservation. Nothing from the PR was run, built, or installed. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Reviewed at b27e94c4, with the CLI built at this head and at main (107afae0) and run in verify worlds. The last-install case does what #1825 asks: after a workspace uninstall --force, ~/.codex/hooks.json and the project's data home are gone, and main kept the file. b27e94c4 also fixes the self-keeping partition from b3144947. One blocker remains, and a few things need to go in before I can approve.
[P1 blocking] An unreadable config in another workspace removes that workspace's hooks. The body says "A partition whose config cannot be read counts as another install, so the failure mode is keeping, not removing." The code does the opposite. installsSharingHome drops a partition whose readConfigFrom returns null, so lastInstallOnMachine answers true. Repro: two workspaces with Codex; corrupt the second one's config.yaml under ~/.teamai/projects/<slug>; run uninstall --force in the first. At this head ~/.codex/hooks.json is removed; main keeps it. Once the second workspace's config is fixed, it has no hooks. The automated review raised the same thing. A partition directory whose config can't be read should count as another install, and that needs a test row.
[P2 non-blocking] A pre-partition install isn't counted. A workspace whose config is still <project>/.teamai/config.yaml is read by detection (config.ts:1109-1112), but installsSharingHome only scans ~/.teamai/projects/*. With such a workspace on the machine, uninstalling another one removes ~/.codex/hooks.json while teamai status in the old one still reports it installed. Its next pull writes the hooks back, so I'm not blocking on it. Either count it, or say in the CHANGELOG that an old-layout install on the same machine needs a pull afterwards. (#1817's wrapper sweep has the same gap.)
[P2 non-blocking] The CHANGELOG lists only part of what is now removed. globalAdapters gates more than the six adapters the entry names. On this path a workspace uninstall now also removes:
- the Hermes status hook and plugin (
uninstall.ts:1633,:1775); - the bridged team hook file (
:776); - the OpenCode agent hooks (
:586-600); - the Pi agent extensions (
:667-674); - skills in earlier hermes/omp/opencode homes (
:1345).
They're all teamai's own files, but a fix that starts deleting member-visible files has to list them.
Merge main. The branch conflicts on CHANGELOG.md now that #1824 is merged as e30376d2. Merging main also drops the #1824 commit from this diff, which settles the automated review's second point.
Not covered here: Windows, self mode, submodules and workspace children. Hermes, Pi, OMP, Kiro and Antigravity were checked by reading the code. Only Codex was run.
b27e94c to
f39d0ec
Compare
Both earlier findings remain unresolved. The description documents sufficient representative real-CLI verification for removal and preservation. Nothing from the PR was run, built, or installed. |
|
CI did not start for So the fix for the regression CI found is pushed but unmeasured by CI:
A re-run at the new head would let those four jobs confirm it. I cannot start a |
CI isn't starting because the branch conflicts with main. GitHub runs When you do, the review above still applies: the unreadable-partition case (a partition directory whose config can't be read should count as another install), and the CHANGELOG list of what a workspace uninstall now removes. The self-keeping partition fix in |
f39d0ec to
fc61a5f
Compare
The earlier unrelated-change finding is resolved: the #1821 dashboard filtering, tests, marker, and changelog addition are absent from this diff. The description documents sufficient representative real-CLI verification for both removal and preservation. Nothing from the PR was run, built, or installed. |
|
You are right about the P1, and the E2E suite turned up something bigger, so I am withdrawing this rather than fixing the P1 and leaving the rest. On the P1. My body said "a partition whose config cannot be read counts as another install"; What makes me stop is the sibling checkout. Three shards fail on the same shape: A linked worktree, a submodule checkout and a sibling clone of the same project share the current data home, so Why I am not widening it once more. Making the predicate checkout-aware means enumerating checkouts of the current project, which What the right shape looks like, for whoever takes it: remove at the record level — strip only this install's dispatcher from the shared file and delete the file only when no other record still names it — instead of deciding the file wholesale from a machine-wide boolean. That is a bigger change than this PR, and it would also settle your P2 about the pre-partition install. I have put all of it — the sibling-checkout case, the unreadable-config rule, and the full list of what |
fc61a5f to
e30376d
Compare
|
No findings: the exact requested three-dot diff is empty.
The supplied description includes sufficient representative real-CLI verification. The empty diff does not establish whether the earlier uninstall finding is fixed. Please provide the correct base and head commits for #1826. Nothing was run, built, or installed from the PR. |
Summary
teamai uninstallin a project now removes the machine-wide hook files it wrote when no other teamai install on the machine runs them. It used to keep Codex's~/.codex/hooks.json, OpenCode's plugin, and the Pi, OMP, Kiro and Antigravity hook files for "other teamai installs (user scope, HTTP agent or other projects)" without checking that one existed — so on the last install those dispatchers stayed, fired on every session and found no config, andteamai hooks removewas the only way out.The decision is the one #1817 added for the wrapper:
installsSharingHome(dataHome)for the user scope and the other project partitions, plus~/.teamai/local-agent/manifest.jsonfor an HTTP agent. When any of them is set up, nothing changes: the files are kept and the message still names them.Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest run src/__tests__/uninstall.test.ts src/__tests__/mcp-uninstall.test.tspassesUnit
Three cases, in
uninstall.test.tsbeside the user-scope one:dataHomeset, no user config, no other partition~/.codex/hooks.jsonis gone, and so is the data home the uninstall ran inrun teamai hooks remove here firstscope: userThe data-home assertion is there because CI's
Agent wait budgetcaught the first version keeping it:keptForProjects()was gated onglobalAdapters, so a project uninstall that took the machine-wide adapters also asked which projects were still set up and kept its own. That gate is on the scope again.Negative control on the same head: with
globalAdapters = scope === 'user'put back, the first case fails —— and the second still passes, which is what "kept while another install exists is unchanged" means.
Real CLI, after
npm run buildVerify world, workspace scope, one agent, workspace
init+pull, thenuninstall --force:The kept side, same commands, with a second project's data home planted before the uninstall:
The same commands on
mainbefore this change are #1825's table: the file and all of its dispatchers were still there with nothing else installed.Related Issues
Fixes #1825.
Notes for Reviewers
globalAdaptersis what every adapter whose hooks live in a user-wide file already consults, so this adds a case to it rather than a branch per tool:scope === 'user' || await lastInstallOnMachine(localConfig.dataHome). No adapter's own code changed.loadLocalConfig), every other partition under~/.teamai/projectsbut this one, and an HTTP local agent. A partition whose config cannot be read counts as another install, so the failure mode is keeping, not removing.dataHomeis set on a project config; without it the helper answers false and the old rule stands, which is why the existing "keeps global delivery" cases in this file still pass untouched.hooks.stateapprovals for a removedhooks.jsonstay inconfig.toml;teamai hooks removeleaves them today as well, so this change does not introduce it. The empty tool directories (~/.codex,<project>/.codex) stay on purpose — fix(uninstall): remove what teamai created and left empty #1817 established that a tool's own directory is not ours to delete.