Sign up
View stargazersSign up

fix(server): t3 service uninstall leaves another T3 home's service alone#29872

merged1
main
GitHub fork: fix/service-uninstall-other-home
opened8d agobySsaphid
2 files changed
View on GitHubTwo-way sync enabledPR last imported 7d ago

Problem

The background service is one per-user unit, and it serves the T3 home in its T3CODE_HOME. Running t3 service uninstall --base-dir /some/test-home (or running it with a different T3CODE_HOME) stopped and removed that unit even when it served another home, such as the real ~/.t3. Cleaning up a test home this way took the user's live service down and removed it from startup. Their remote clients disconnected until they reinstalled the service.

t3 service restart, t3 update, and t3 uninstall already leave another home's service alone. Only t3 service uninstall had no check.

Change

t3 service uninstall now reads the service status first. When the installed unit names a different T3 home than this CLI's base dir, it prints "The installed service runs another T3 home, so it was not removed. Pass its --base-dir." and removes nothing. This is the same path.resolve comparison that t3 uninstall makes (servesThisHome in cli/uninstall.ts), and it matches the restart wording in #29848.

A unit is still removed if it names no home, or if its status cannot be read. That keeps a damaged unit removable, and only a unit known to serve another home is kept. BootService.uninstall itself is unchanged.

Scope and approval

This is a very small, focused fix for an obvious bug, so there is no prior issue. One sibling command lacked the other-home guard that every other service-mutating path already has. The change is 16 non-test lines in one handler, plus one extended test.

Overlap search: the local PR index and GitHub searches found no PR for service uninstall. Open #23878 adds the same kind of check to install and status only. Open #29848 fixes the restart message only. Neither touches the uninstall handler, and this PR does not touch their lines.

Verification

  • Red on main: the new uninstall removes only another home's service row fails with AssertionError: expected [ true ] to deeply equal [], because the other home's unit is uninstalled.
  • Green: vp test run src/cli/service.test.ts passes 11/11. The rows cover this home's service (removed), another home's service (kept, with the message), and a status read failure (still removed).
  • vp run check:changed passes: format, lint, knip, graphql, typecheck, and tests.
  • Not checked: a real systemd or launchd unit. The tests stub BootService, like the existing CLI tests.

Reviewed with sol-loop: 2 rounds with GPT-6.1 Sol on high. Sol verdict: approved. Round 1 found that a failing status read blocked uninstalling an unreadable unit; that was fixed and covered by a test row.

Created with Claude Opus 5.5 in Claude Code (T3 Code).

🤖 Generated with Claude Code

Checks

24 successful · 14 skipped
All checks passed
727a5da

Added 1 commit

  • fix(server): t3 service uninstall leaves another T3 home's service alone727a5da
MmacroscopeappBot8d ago
Synced from GitHub

Approvability

Verdict: Approved at 727a5da

Macroscope's review found this PR approvable — Uninstall now leaves a service alone when it is known to belong to another T3 home and explains how to target it explicitly. This narrowly scoped safeguard reduces accidental service disruption without changing installation defaults.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot8d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The service uninstall command now checks the installed service’s home against the configured base directory. It skips removal when the homes differ. Tests cover matching homes, a different home, and a status lookup failure.

Changes: Service uninstall

Layer / File(s)Summary
Check installed home before uninstall
apps/server/src/cli/service.ts, apps/server/src/cli/service.test.ts		The command checks the installed home before uninstalling. If the known home differs from the configured base directory, it logs that the service was retained and returns. Tests record uninstall calls and cover matching homes, a different home, and a status lookup failure.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

  • pingdotgg/t3code#23858 describes cross-home checks in service commands. This change adds that check to uninstall; it does not add the issue’s stated guard for install.

Possibly related PRs

  • pingdotgg/t3code#19652: Keeps remote service-unit runtime state aligned and preserves a unit assigned to another T3 home.
  • pingdotgg/t3code#23878: Refuses to install a service when the existing unit serves another T3 home.

Suggested labels: bugfix, size:S

Suggested reviewers: juliusmarminge, macroscopeapp

Merge Risk: External image Minimal · up to 727a5

This change stops t3 service uninstall from removing a service that belongs to another T3 home. The remaining gap is a very narrow race with a concurrent install, which can be recovered by reinstalling. The change is ready to merge.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title names the main change: a fix so that t3 service uninstall no longer removes or stops another T3 home's service. The raw summary and objectives both describe a guard in the uninstall handler …
Description checkPassedThe description follows the template. It has Problem, Change, Scope and approval, and Verification sections. The Problem section states the bug, its expected behavior, and reproduction steps (running …
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: a fix so that t3 service uninstall no longer removes or stops another T3 home's service. The raw summary and objectives both describe a guard in the uninstall handler that keeps a service serving another home. The title is specific, follows the conventional commit form, and is about 72 characters.

Full details: Description check

Explanation

The description follows the template. It has Problem, Change, Scope and approval, and Verification sections. The Problem section states the bug, its expected behavior, and reproduction steps (running uninstall with a different --base-dir or T3CODE_HOME). The Change section explains the status-first check and the path.resolve comparison, and it notes the unchanged BootService.uninstall. The Scope section explains why no prior issue is needed and links overlapping PRs. The Verification section gives a red-then-green test result, the check:changed result, and what was not tested. The template's HTML comments were removed, and the author's model attribution is included at the end as the template asks.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

External image ast-grep (0.51.1)

ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/rust/security/weak-cipher-des-rc4-ecb-rust.yml

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between c0ec87a55bbffa437673f037d7b4ca68c6a300f1 and 727a5dabeff8959a2552b955368d9c265cd396e8.

External image Files selected for processing: 2
  • apps/server/src/cli/service.test.ts
  • apps/server/src/cli/service.ts

Limit details: You’ve used all 10 included reviews currently available.

Mmaria8d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified unchanged head 727a5dabeff8959a2552b955368d9c265cd396e8 on a disposable linux vm.

  • reproduced on github/main (61c3794c32): service uninstall --base-dir /home/user/service-home-b removed the real systemd user unit serving home a, stopped pid 35155, and disconnected its endpoint.
  • on the pr, both --base-dir and T3CODE_HOME targeting home b printed the preservation message. pid 39506, the unit hash, and startup enablement stayed unchanged. discovery returned http 200.
  • created a real git project and thread through the running server's cli/MCP path. the systemd-hosted server completed real codex/gpt-6.1-sol turns before and after the wrong-home uninstall.
  • same-home uninstall with a normalized a/../a path removed the unit. repeating it reported no service installed. wrong-home restart also left the service running.
  • started main and pr dev environments sequentially with vp run dev. drove the pr web client with repository Playwright and /usr/bin/google-chrome, typed and submitted a message, and saw the real codex reply.
  • blacksmith: focused service tests passed 11/11; vp run check:changed c0ec87a55bbffa437673f037d7b4ca68c6a300f1 passed format, lint, knip, graphql, affected typechecks, and tests. all 31 reported pr checks passed.

the real systemd units used the production renderBootServiceUnit renderer and ran each checkout's source server. this avoids downloading a released runtime in place of the code under review. no service, provider, or database mocks were used. no code changes were needed.

command transcript

real pr web client completing a codex turn

real pr web client submitting and receiving a codex turn

Not verified

macos launchd, windows, native desktop/mobile clients, and remote/tunnel connections were not exercised on this linux vm. the released-runtime download/install path was not exercised. the unreadable-unit fallback passed its existing test but was not forced against a real unit.