Repository navigation
feat(update): record installations and update registered npm-global copies - #1810
Conversation
The PR description provides a sufficient representative real-CLI verification record. I did not run, build, or install anything from the PR. |
e4c2006 to
8086938
Compare
|
All three earlier findings remain unresolved in the current diff:
The PR description provides a sufficient representative real-CLI verification record. I only read the specified diff; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Reviewed at 8086938a. The main path works as the body describes. With updatePolicy: auto and updateOtherInstallations: true, an update of copy A updates the other registered npm-global copy under its own prefix, and the inventory and teamai doctor list both copies. prompt and skip never update another copy, and Volta, vendored and local copies are listed without being passed to npm. I ran the built CLI with an isolated HOME, separate npm prefixes and a local registry. Three things still need fixing, and I reproduced each one.
Blocking
-
[P1 blocking] A
~/.teamaithat cannot be written stops every command and hook before it runs (the CI review's P1).registerRunningInstallation()runs beforeprogram.parseAsync()insrc/index.ts, andwriteInstallations(src/installations.ts) does not catch itsmkdirSync/writeFileSync/renameSync. I made~/.teamaimode 555, andteamai statusexited 1 with an uncaughtEACCESstack frominstallations.json.<pid>.tmp. The merge-base build reaches the command's own check. The inventory is diagnostic, so registration should never fail a command: catch insideregisterRunningInstallationand log at debug. -
[P1 blocking] A copy that cannot be updated is picked on every run, so no other copy is reached (the CI review's first P2, reproduced). Candidates are sorted by
lastSeenAtnewest first (src/installations.ts:161), andsetInstallationUpdateadvanceslastSeenAton every outcome, failures included. With C (0.20.0) and B (0.21.0) registered and B'slib/node_modulesread-only, three runs in a row picked B and failed withnpm error code EACCES, and C was never updated. Once B was writable, B updated on the next run and C on the one after. The CI review's fix covers half of it: keeplastSeenAtas the last run time, but also sort bylastUpdateAtoldest first, with copies never tried before the rest. Removing the bump alone is not enough, because B was registered after C and stays first. -
[P1 blocking] The inventory reports
updatedafter a failed update (the CI review's second P2, plus one more case).src/update.tswritesupdatedbefore the hook refresh, and the install failure path writes nothing:- Latest moved to a new version and the install directory was read-only:
teamai updateexited 1 with "Permission denied", and the record still saidupdatedwith the previous message. - The install succeeded and the hook refresh then failed: the run exited 1 with "Hook refresh after update skipped", and the record said
updated.
Writing the outcome once, after the whole update is known, and recording
failedon every failure branch would fix both. - Latest moved to a new version and the install directory was read-only:
Non-blocking
- [P2 non-blocking] A Stop that updates the running copy can run two npm installs. One install is for the running copy and one for the other copy, each with the 60 s install timeout, inside the 90 s
UPDATE_BUDGET_MS. Two slow installs pass the budget during the second one. The body says the one-copy limit "keeps one Stop inside the existing update budget", and the issue's design is one install per Stop. Skipping the other copy when the running copy installed in this Stop, and leaving it for the next Stop, would make that true. I did not run slow installs. - [P2 non-blocking] A failed update of another copy makes
teamai updateexit 1, and makes the automatic update fail, even when the running copy is current (src/update.ts,src/hook-handlers.ts). I saw exit 1 in the run above with A already current. That may be what you want, but it is new behaviour of an existing command, and the body and CHANGELOG do not mention it. - [P2 non-blocking] Records of removed copies stay forever. After deleting a copy's directory,
teamai update --checkstill lists it as "automatic updates supported", anddoctorcounts it. The update path is safe, becausereadInstalledVersionreturns null and the copy is skipped. But everynpxor vendored path that ever ran becomes a permanent row. Marking a record whose package directory is gone as missing, or pruning it on registration, would cover it. - [P2 non-blocking] No CHANGELOG entry for the new setting, the inventory file and the new
updateanddoctoroutput. - [P3 nit]
RESEEN_AFTER_MSreads as a typo.compareVersionsinupdate.tsis a pass-through tocompareSemver, so a re-export would do.- Registration is an unlocked read-modify-write, so two processes registering at once can drop a row until that copy runs again. I found this by reading the code, not by running it.
CI review
It reports one P1 and two P2s. I agree with all three and reproduced each one. I grade both P2s as blocking because they reproduce and fit this PR. The first leaves the other copies outdated for good, and the second makes the inventory say the opposite of what happened.
CI
At this head the Windows job and the build passed. Lint & Test, the unit shards and the E2E shards were still running when I wrote this.
Merge risk
Merge risk: high.
- Alignment: decent. Part (a) of #1753 is there, but the three cases above contradict the body.
- Impact: medium. The PR writes a file on every command and hook, and runs npm installs in other prefixes.
- Verification: decent. The built CLI ran with real npm prefixes and a local registry, and the record in the body covers the main path. Slow installs and concurrent registration were not run.
- Rollback cost: low. The inventory file and the opt-in setting can be removed.
- Error detection: poor. The three blocking cases either fail loudly in the wrong place or record success after a failure.
@jeff-r2026 your call on this one. It adds the scope (a) inventory and opt-in from #1753, and the three blocking items need fixing whichever way you decide.
8086938 to
8d925e6
Compare
|
All three earlier findings remain unresolved:
Additional finding:
The PR description provides a sufficient representative real-CLI verification record. I only read the specified diff; I did not run, build, or install anything from the PR. |
8d925e6 to
4333dcc
Compare
|
Thanks for the reproduction. Fixed all three blocking items and the P2s in P1 — read-only home. P1 — a failing copy hiding the rest. Candidates are now ordered by P1 — P2 — two installs in one Stop. The other copy is only considered when the running copy is already current. A Stop that installs the running copy leaves the other copy for the next Stop; the PR body and docs now say so. P2 — exit 1 when the other copy fails. Kept, because the inventory should not report success for a failed requested update. Documented in the Limitations section and in P2 — removed copies. P2 — CHANGELOG. Added an Unreleased entry for the setting, the inventory file, the update behavior and the P3. Latest local checks: |
4d07216 to
016846c
Compare
|
The three findings in the earlier review were fixed in
|
016846c to
94d9bfb
Compare
|
No new findings I’m confident are real in the specified diff. All four earlier findings are resolved:
The PR description provides a sufficient representative real-CLI verification record. I only read the specified diff; I did not run, build, or install anything from the PR. |
b040c11 to
be0dbde
Compare
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Re-checked with the built CLI at 94d9bfbc. The later pushes (b040c11d changes tests, and be0dbdeb makes an automatic update fall back to the running scope when the member config cannot be read) do not touch what these runs cover, so the runs still apply to be0dbdeb. All three blocking items from my review are fixed:
- Read-only
~/.teamai. With the directory at mode 555, commands reach their handler. Registration catches the write error and logs it at debug. - A failing copy no longer hides the rest. Selection now goes by
lastUpdateAt, oldest first, with never-tried copies first, andlastSeenAtis no longer advanced by an attempt. With B failing, C is reached on the next run. - Failures are recorded as
failed. That covers a refused install and a failed hook refresh, and noupdatedrecord is left behind.
The P2s are covered too:
- a Stop that installs the running copy leaves the other copy for the next Stop;
- removed copies show as "no longer on disk";
- the CHANGELOG entry is in.
The CI review's newer-release point reproduced at d91293b3. Both installs are now pinned to the checked version, and the same race ends on that version. I accept keeping exit 1 when the opt-in copy fails, since it is documented. I also accept leaving the inventory write unlocked: the next run repairs its own row.
Three things are left. None of them blocks on its own:
- [P2 non-blocking]
--dry-runnow writes~/.teamai/installations.json. Registration runs for any command with arguments,--dry-runincluded, and also for one that refuses--dry-runand exits 1. The global option's help says "Preview mode, no changes made". The head relaxes real-CLI tests that checked a command leaves the data home untouched so that they ignore this file. Five of them are dry runs:dry-run-refusal,maintenance-checkouts-dry-run,recall-feedback-dry-run,stats-recall-dry-runandupdate-dry-run.contribute-namespaceis relaxed the same way.debug.logis already exempt, so there is a precedent for a diagnostic file. Even so, adding--dry-runto the skip list next to--helpand--versioninsrc/index.tsis a one-line change. It would keep the dry-run tests as strict as they were, and the same goes for thecodebase-deep-enrichandci extract-mrchecks. - [P2 non-blocking] A failed automatic update of another copy is not in
debug.log. It is logged withlog.warnonly, while the hook's message says to look indebug.log.doctorandupdatedo not print the recorded outcome either, so the cause is nowhere a member would look.log.persistfor that line, or printing the last outcome in the installation notes, would close it. - The branch needs a rebase. It is on
27b7f1e6, andCHANGELOG.mdconflicts with main, so CI will not run on what would be merged.
CI review at this head: no findings, and it calls all earlier findings resolved. I agree, with the two P2s above as additions.
Merge risk: medium. It writes a new file on every command, and runs npm in other prefixes, but only on opt-in.
@jeff-r2026 your call on this one. From our side the blocking items are fixed. What is left is whether you want --dry-run to skip the inventory, and the rebase.
|
No new findings I’m confident are real in the specified diff. All four earlier findings are resolved:
The PR description documents sufficient representative real-CLI verification. I only read the specified diff; I did not run, build, or install anything from the PR. |
c9eae68 to
48bf8ec
Compare
|
No new findings I’m confident are real in the specified diff. All four earlier findings are resolved:
The PR description documents sufficient representative real-CLI verification. I only read the specified diff; I did not run, build, or install anything from the PR. |
|
Rebased onto |
48bf8ec to
b58f8fb
Compare
|
Both P2s from the re-check are fixed in
The branch is rebased onto |
|
No new findings I’m confident are real in the specified diff. All four earlier findings are resolved:
The PR description documents sufficient representative real-CLI verification. I only read the specified diff; I did not run, build, or install anything from the PR. |
Checked
The branch is on current main ( @jeff-r2026 your call on this one. From our side nothing is left: the blocking items were fixed earlier, and these were the last two suggestions. |
|
A status question rather than a nudge: this has been green since
If the remaining decision is about that deferred scope rather than this diff, say so and I will leave the PR as it is; otherwise it is ready whenever you are. |
teamai uninstall no longer records the running copy, and its sweep of machine-wide leftovers removes ~/.teamai/installations.json under the same guards: kept while another install shares HOME.
To get this merged, I pushed on top of
How
The sweep cases in Real-CLI run, world built from I listed I'll merge once CI is green at |
|
No new findings I’m confident are real in the specified diff. All four earlier findings are resolved:
The PR description documents sufficient representative real-CLI verification. I only read the specified diff; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving at 1eb5672a. The blocking items and both suggestions were fixed earlier, and the commits on top close the inventory left behind by uninstall. The CI review at this head has no findings. I'll merge once CI is green.
Summary
~/.teamai/installations.json: entry, version, install method, npm prefix, whether automatic updates are supported, and the last update result. A copy registers on any command or hook, at most once an hour. Registration is diagnostic: a read-only home is logged at debug level and never stops the command or hook.--help,--versionand--dry-runskip it;TEAMAI_DRY_RUNcarries the dry-run skip into a workflow child.updateOtherInstallations(defaultfalse). WithupdatePolicy: auto, an update of the running copy can also update one other registered npm-global copy, under that copy's own--prefix, when the running copy is already current. A Stop that installs the running copy leaves the other copy for the next Stop.teamai-cli@<latest>), so a release landing between check and install cannot look like a failed update. Read the target copy'spackage.jsonafter npm exits; npm exiting 0 without changing that copy is reported asfailed, not success. Hooks are not refreshed for another copy, so the active session stays bound to the copy that runs it.teamai updateandteamai doctorlist the recorded copies with their version and method. No new command.prefixFromEntryPath/isVoltaPackageImagetosrc/install-prefix.tsandcompareVersionstosrc/version.ts, so the inventory does not import the updater.Refs #1753 (scope (a) from the maintainer decision). Scope (b), the team-visible summary and effective-policy reporting, stays a follow-up, so this PR does not close the issue.
Validation
npm run build— passesnpm run typecheck— passesnpm run lint— passes, including the documentation checksnpx vitest run src/__tests__/update.test.ts src/__tests__/installations.test.ts src/__tests__/update-policy.test.ts src/__tests__/member-settings-overlay.test.ts— 140/140 passnpx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/init-keeps-member-settings-1458.test.ts— 4/4 passReal-CLI run
Built CLI, two npm-global prefixes on one machine, and a local registry serving
teamai-cli@0.3.0:.../A/lib/node_modules/teamai-cli, version0.3.0(current).../B/lib/node_modules/teamai-cli, version0.1.0updatePolicy: auto,updateOtherInstallations: truepackage.jsonchanged from0.1.0to0.3.0.0.3.0; it was not installed twice."lastUpdateOutcome": "updated"with the new version.teamai doctorprinted the same two installation lines.Limitations
teamai updateexit 1 (and records the automatic update as failed), even when the running copy is current. That is the same failure signal the running copy's own update already uses.promptandskipnever update another copy; onlyautoplus the opt-in does.teamai doctorand are never picked for an update. The inventory is machine-local and contains absolute paths only; nothing from it is published.