[ronak-guliani]

Problem

An Official 0.0.42 client connected to a Dev 0.0.23 server repeatedly opens only subscribeServerConfig, then disconnects before readiness. The first real config chunk contains a string-valued settings.sourceControlWritingStyle; the newer client requires an object. Its decoder dies with SchemaError, and the supervisor treats that as a transient transport failure, indefinitely retrying.

The installed production factory reproduced this failure against an isolated copy of the installed Dev server: one config subscription, a received chunk, failed readiness, and a clean client close with code 1000. A matching Official server succeeds. No live applications, authentication database, or running user server were changed.

Change

Accept legacy writing-style strings in the shared contract: empty/whitespace values become repository conventions, and non-empty instructions become custom mode. Keep modern object encoding and structured settings patches unchanged.

Convert config schema defects into ConnectionBlockedError(reason="unsupported"). Let the always-on config subscription own readiness and termination, so the disconnect hook cannot mask that failure as transient. The existing registry then disables an incompatible environment rather than automatically retrying. Ordinary transport failures, opening timeouts, heartbeat failures, and unrelated defects retain their existing classifications.

This is read compatibility for the identified bootstrap field, not a promise that every RPC or mutation across a nineteen-release gap is compatible. The separate Dev client's brief-disconnect toast and the server's session-inactive logging gap are not claimed fixed.

Scope and approval

Focused obvious-bug exception: a demonstrated config decode regression prevents an otherwise authenticated session from becoming ready. The contract repair and terminal-error classification address that same bootstrap reconnect loop; no new setting, workflow, or server wire rewrite is introduced. Changes are upstream-based because the incompatible production decoder is not in the older fork.

Verification

Tested code: dc6534f5bc (the commit contains the same code exercised in the browser; the real-socket probe was repeated after committing).

Command	Observed result
cd packages/client-runtime && pnpm test src/rpc/session.test.ts before production edits	Five new regressions failed for the intended reasons; 21 existing tests passed.
cd packages/client-runtime && pnpm test src/rpc/session.test.ts src/connection/supervisor.test.ts src/state/server.test.ts	Passed: 94 tests, including normal closures, opening failures, heartbeat timeouts, durable consumers, and retry policy.
cd packages/contracts && pnpm test src/settings.test.ts	Passed: 161 tests, including unchanged modern encoding and settings patches.
pnpm fmt:check	Passed, exit 0.
pnpm lint	Passed, exit 0; existing warnings remain.
pnpm typecheck	Passed, exit 0.

Real legacy server: run the production source RpcSessionFactory against an isolated installed Dev 0.0.23 server, without rewriting its response. The string config decoded successfully; readiness and probes before/after a 35-second hold passed, heartbeats exchanged ping/pong, exactly one config subscription was opened, and no active-period socket closed. The reproducible probe and revision-bound report are retained as session artifacts, not committed.

Real web client: authenticated a worktree-isolated web app and replaced only external config fields as a controlled wire fixture. A legacy instruction string established a connected production session, normalized to custom mode, and rendered unchanged instructions in Source Control settings. A separate malformed cwd: 42 config was recorded as unsupported, disabled the environment through the real registry, and produced only one socket open over a 71-second observation. Removed the fixture, restored compatibility, and verified a normal connected state.

The browser fixture does not claim to be a full old-server UI integration; the independent socket experiment uses the actual unmodified legacy server. Captures illustrate current modern-control and legacy-normalization states, not a reconstructed pre-fix Electron screenshot. Pre-authentication socket rejections and an initial IPv4 navigation failure were observed before pairing. There were no application-console errors during authenticated checks, but four environment-descriptor requests were aborted during the scripted reconnect/cleanup phase. The browser pass establishes the asserted production runtime states and rendering, not an entirely error-free network smoke check. A provider-authentication warning was present but unrelated; no provider turn was started.

pnpm test:self and pnpm pr:media are not scripts in this upstream checkout. Attempted media publication using the fork's existing tool:

pnpm pr:media -- https://github.com/pingdotgg/t3code/pull/15402 <modern-object-control.png> <legacy-instructions.png> <incompatible-config.webm>

Publication blocked: capture decoding passed, but GitHub attachment upload returned HTTP 404 (exit 1). Verified that no attachment URLs were recorded and no media section was published. Inspected, privacy-cropped screenshots and a 20-second recording remain in local session artifacts; they are not committed. No full server test suite was run, and no unrelated pre-existing fork failures were investigated.

Model: GPT-6.1 Sol. Harness: GitHub Copilot in T3 Code.

[macroscopeapp]

Approvability

Verdict: Approved at dc6534f

Macroscope's review found this PR approvable — This is a focused compatibility fix that accepts the legacy settings representation and turns incompatible server-config decoding into a stable unsupported state instead of a reconnect loop. Modern settings encoding, ordinary transport handling, and existing defaults remain unchanged, with targeted tests covering the new paths.

Notes:

• Diff unchanged. Approvability was decided on eligibility alone.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

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 configuration

• Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 35f16d17-97ea-4b4e-84b8-2b5148be9239

📥 Commits

Reviewing files that changed from the base of the PR and between 2d81e4c and dc6534f.

📒 Files selected for processing (5)

• docs/internals/connection-runtime.md
• packages/client-runtime/src/rpc/session.test.ts
• packages/client-runtime/src/rpc/session.ts
• packages/contracts/src/settings.test.ts
• packages/contracts/src/settings.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The changes add legacy string decoding for source-control writing-style settings. RPC sessions now classify incompatible server configuration as unsupported and distinguish connection-establishment failures from later socket disconnects.

Changes

Settings and connection handling

Layer / File(s)	Summary
Legacy writing-style settings packages/contracts/src/settings.ts, packages/contracts/src/settings.test.ts, packages/client-runtime/src/rpc/session.test.ts	The settings schema accepts structured values and legacy strings. Tests cover round-trip encoding and normalization of empty, whitespace-only, and custom legacy styles.
RPC session connection and config handling packages/client-runtime/src/rpc/session.ts, packages/client-runtime/src/rpc/session.test.ts, docs/internals/connection-runtime.md	RPC sessions distinguish failed connection establishment from later disconnects. Config schema failures become unsupported connection errors, and readiness and closure follow config-subscription state. Tests cover incompatible config before and after readiness. The documentation describes error propagation and registry behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ConfigSubscription
  participant RpcSession
  participant Supervisor
  participant Registry
  ConfigSubscription->>RpcSession: Sends initial snapshot or config update
  RpcSession->>Supervisor: Reports decode failure as unsupported
  Supervisor->>Registry: Reports unsupported environment
  Registry->>Registry: Disables environment until compatibility is restored

Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to dc653

The changes are mergeable after normal checks. The available evidence identifies no actionable compatibility or connection-handling failure.

Security Architecture Review

Security architecture risk: 🔵 Low · up to dc653

The compatibility change maps legacy values into an existing settings representation without demonstrating new privileges. Incompatible configurations now stop reconnecting. Risk remains low rather than minimal because early-disconnect handling and complete recovery and configuration-provenance paths remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — A party able to supply the selected environment's configuration can affect that connection's compatibility outcome and writing-style instructions. The shared decoder also admits legacy values in project overrides. These paths do not establish cross-environment authority; configuration-producer authentication and broader downstream exposure remain incompletely traced.

Trust Boundaries and Controls

• observed — Initial configuration remains subject to expected environment-ID validation, and environment authorization errors remain permission-blocked errors. Legacy and modern custom instructions use the same trimming schema, while settings patches continue to require structured fields. These controls establish identity comparison and input shape, not proof of producer authenticity or safe interpretation by every downstream provider.

Resilience and Maintainability Implications

• inferred — Preserving configuration incompatibility as a blocked error contains repeated attempts against an unchanged incompatible server. This benefit depends on the configuration source delivering its terminal cause to the supervisor; complete transport-close propagation and compatibility recovery remain evidence gaps rather than established defects.

Hardening Proposals

• proposed — Validate that transport closure before the first configuration snapshot reaches a terminal session outcome, and that interrupt-only exits occur only during owner-driven cancellation. This would directly substantiate failure containment after removing the independent disconnect fallback.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: accepting legacy configuration to prevent reconnect loops.
Description check	✅ Passed	The description covers the problem, fix, scope rationale, and focused verification results. It also states the limits of the compatibility claim and notes checks that were not completed.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

Dismissing prior approval to re-evaluate dc6534f

[juliusmarminge]

Taking over this one as maintainer for batch ssh-relay-remote-08. The connection runtime has moved since this branch was written, so I will rebuild from current main as two reviewable stack layers: legacy settings decoding, then terminal incompatible-config classification, preserving heartbeat timeout diagnostics. I will credit your original work and close this PR only after the replacement stack is published and compared against your diff.

[juliusmarminge]

Thanks @ronak-guliani — this correctly identified the legacy string-valued writing style and the need to let the config subscription retain incompatible-config errors. I rebuilt it against current main because the shared runtime and heartbeat handling have moved, and split the two concerns into the published stack:

• #21665: legacy settings reads, normalized object encoding, strict global and project patch writes.
• #21667: incompatible initial/later config terminates the session and disables the environment without reconnecting, retaining heartbeat timeout diagnostics.

I compared your complete original diff against both layers. All intended behavior is retained. The original schema also broadened project writes accidentally; the lower layer preserves that strict boundary. The config-stream approach needed the current heartbeat timeout hook to preserve “stopped responding”; that is in the upper layer. The internal documentation paragraphs were replaced by two short comments at the actual ordering constraints. The original eight cases were replaced by six behavior cases in the existing test files, with existing heartbeat/legacy tests strengthened: 296 passing plus six regressions on main → 302/302 here, scoped lint/format/typechecks passing. Both independent full-stack reviews (Claude Opus 5.5 and GPT-6 Sol) found no actionable issues.

Live Chromium verification used the actual client/server with disposable wire frames: legacy bootstrap shows normalized instructions, malformed initial and later configs close their sockets and disable the environment, and modern frames recover after reload. Proof and platform limits are in both replacement descriptions; actual old binaries and native platforms remain unverified. CI is running on the published stack; neither layer is merged.

Existing recovery policy still refuses re-enable while unsupported is recorded; recovery uses rediscovery/restart, re-onboarding or the host-update flow. Non-default config consumers and the legacy probe retain pre-existing defect behavior outside this always-on subscription. Those do not block this fix and are recorded on the upper layer.

Closing this superseded PR in favor of the linked stack, with credit preserved in both descriptions.