Repository navigation
Conversation
…s a launcher protocol `t3 update` installs the service from the outgoing CLI, so the state file carries that CLI's protocol next to the new version. The new launcher rejected the document and the service manager respawned it every few seconds while the update reported success. The launcher now adopts the two-field document an older install writes and rewrites it under its own protocol, so `t3 service status` reads it as well. The runtime layout the protocol stands for is still checked before a child starts. A document with an update record, a newer protocol, or a malformed protocol is rejected as before.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe service launcher adopts compatible older install-state protocols, rewrites them with the current protocol, and starts the active runtime. Tests cover legacy protocols, invalid states, malformed JSON, persistence, and runtime startup. ChangesService state compatibility
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The launcher migrates the compatible legacy install state before starting the service. No actionable merge blocker is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The compatibility change retains version validation and runtime checks, with no demonstrated expansion of remote authority. A remaining concurrency risk is that startup normalization could overwrite a newer installation state if installation and launcher startup overlap. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a focused compatibility fix for legacy service-state files: only known protocol 1/2 install records are migrated, while malformed, future, or update-in-progress records remain rejected. Existing state handling and runtime validation remain unchanged, with targeted tests covering migration and startup. You can add or adjust custom eligibility rules. Learn more. |
The Effect diagnostic rejects a bare JSON.stringify in the typecheck; the file already opts out per call site for fixtures, so the new table does too.
… need The helper takes an unknown value, so the JSON diagnostic never fires there and the unused directive is itself a typecheck warning that fails CI.
kvnloo
left a comment
There was a problem hiding this comment.
One protocol-compatibility invariant worth tightening.
Dismissing prior approval to re-evaluate d39aba2
Fixes #12627.
Problem
t3 updateruns the service switch in the outgoing CLI's process, soinstall()writesruntime/service-state.jsonwith that CLI's compile-time protocol next to the new version:{ "protocol": 2, "activeVersion": "0.0.43-nightly.20260919.1962" }The unit already points at the new launcher, which accepts only its own protocol, throws
Service state is invalid or unsupported., and gets respawned by launchd/systemd every 5 s while the update reports success. Hosts that updated from 0.0.42 stay down until someone runst3 service installon the machine.Fix
This is the first option from the triage: the launcher accepts and migrates compatible older state.
adoptOlderInstallStateinserviceProtocol.tsreads the two-field document an install writes when it is stamped with an older protocol, and returns it under the current one.readServiceStatein the launcher falls back to it and rewrites the file durably, sot3 service status(which uses the strict decoder) reads the state too. This also unsticks hosts that are crash-looping right now, as soon as they get a launcher with the fix.The protocol guards the runtime layout, and the launcher still verifies that itself (
runtimeExists) before starting a child, so adopting the document does not skip that check. Still rejected as before: a document with an update record (that belongs to an older launcher's update flow), a newer protocol, and a malformed protocol or version. The remote preflight stays strict.Not included: making
t3 updaterefuse or delegate when protocols differ. It cannot help the 0.0.42 CLIs already released, and it is a separate change to the update command.Tests
serviceLauncher.test.tsgains three groups, all on temp directories with the existing fake-runtime fixture:Launcherbuilt from the adopted state starts the active child for that version."2", 2.5, an invalid version, and malformed JSON still throw and leave the file byte-for-byte unchanged.With the adoption disabled the first two groups fail with
Service state is invalid or unsupported.. The launcher, preflight, and boot-service suites pass (62 tests); targeted lint and the server typecheck are clean. No installed service or real T3 home was touched.Implemented with Claude Code (Claude Fable 5.1); tests written with Codex (GPT-6 Astra).
Summary by CodeRabbit