Sign up
View stargazers13Sign up

fix(server): require the bootstrap envelope before startup#17568

open6
main
GitHub fork: t3/fix-issue-17367
opened14d agobyVvoltcrash
4 files changed
View on GitHubTwo-way sync enabledPR last imported 1d ago

Problem

A desktop-spawned backend can miss its bootstrap envelope and silently start in web mode without the desktop credential. On Linux, launching the real CLI with --bootstrap-fd 0 and either closing the pipe empty or delaying the envelope until 12 seconds reproduced a running backend that rejected the bootstrap credential with 401 invalid_credential.

Change

Require a decoded envelope whenever a bootstrap descriptor is explicitly supplied by flag or environment. Allow up to 30 seconds for input; propagate typed errors for unavailable input, EOF, read/decode failures and timeout before creating server state. Preserve ordinary web defaults when no bootstrap descriptor is supplied and existing platform-specific descriptor handling.

Scope and approval

Fixes #17367 GitCafe: https://git.cafe/pingdotgg/t3code/issues/17367 Triaged bug via the automated issue flow. Existing contributor commits and implementation are preserved.

Verification

  • Re-ran the existing flag/env configuration regressions against main at 49e97d48f9: both fail because server state is created despite the unavailable descriptor.
  • At PR head d52b1cc7fb, all 33 focused bootstrap/config tests pass. Scoped lint, formatting and server typecheck pass.
  • Real Linux CLI: empty input, invalid flag/env descriptors and malformed input exit 1 with the expected typed error and no userdata. Valid desktop input delivered at 12 seconds reaches readiness and exchanges its credential with HTTP 200. Ordinary no-fd web startup reaches readiness.
  • Withheld input logs the 30-second timeout error without creating userdata. The harness terminated that process after its observation deadline; natural timeout process exit was not established.
  • Visual fixture matrix is empty: no render paths changed. Runtime transcripts and red/green test output:

17367-runtime-evidence.txt 17367-red-test.txt 17367-green-test.txt

Native Windows/Electron post-update startup, WSL and native macOS were not verified. The Linux injection reproduces the missing-envelope failure mode, not the original nondeterministic Windows update trigger.

Implementation and fresh verification: GPT-6.1-Sol, Codex harness in T3 Code. Existing implementation and contributor commits preserved.

Checks

All checks passed

Added 1 commit

  • fix(server): require the bootstrap envelope before startup6c080b3
MmacroscopeappBot14d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — The server now blocks startup when an explicitly configured desktop bootstrap descriptor does not yield a valid envelope, and the default wait has increased from one second to 30 seconds. This changes production startup and credential-handling behavior, including a product default, so maintainer review is warranted.

No code changes detected at d52b1cc. Prior analysis still applies.

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

CcoderabbitaiBot14d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d4238575-ac0c-4c3c-aee9-db62fbfe1058
📥 Commits

Reviewing files that changed from the base of the PR and between 6c080b3ecc320b17d20507f31713e89eaafb42d8 and aa7dc91af45cd05c5cc818d6b54670242a297ac1.

📒 Files selected for processing: 4
  • apps/server/src/bootstrap.test.ts
  • apps/server/src/bootstrap.ts
  • apps/server/src/cli/config.test.ts
  • apps/server/src/cli/config.ts

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

📝 Walkthrough

Bootstrap envelope reads now return decoded values or typed errors instead of optional values. The default timeout is 30 seconds. Server configuration uses the decoded envelope directly when a descriptor is provided.

Changes: Bootstrap envelope startup handling

Layer / File(s)Summary
Envelope read contract and stream lifecycle
apps/server/src/bootstrap.ts, apps/server/src/bootstrap.test.ts		Reads fail with typed errors when the descriptor is unavailable, the stream closes without an envelope, or the read times out. The default timeout changes to 30 seconds. Tests cover successful reads, errors, timeout, and stream cleanup.
Configuration loading
apps/server/src/cli/config.ts, apps/server/src/cli/config.test.ts		Configuration assigns the decoded envelope directly when a descriptor is provided. Tests cover unavailable descriptors from the CLI flag and environment variable, and web defaults when mode is unset.

Priority: ➖ Normal

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

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to aa7dc

No actionable merge-blocking risk remains in the reviewed change; normal checks can proceed.

Security architecture risk: ⚪ Minimal · up to aa7dc

The change strengthens startup authentication guarantees: missing or invalid bootstrap input now stops startup instead of silently selecting web defaults. The reviewed changes introduce no material security concern.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The material boundary is a backend process consuming locally supplied bootstrap input and turning it into server configuration. Failure now prevents that process from proceeding without its requested envelope; the changed test helpers add no independently reachable production entrypoint.

Trust Boundaries and Controls

  • observed — A supplied descriptor must pass stat/open handling and schema decoding before configuration proceeds. Missing input, read failure, malformed input, and timeout produce failures rather than credential-free fallback. Configuration tests assert that unavailable flag and environment descriptors leave the isolated server home absent.

Resilience and Maintainability Implications

  • inferred — The one-shot line listener, terminal cleanup, and cancellation finalizer establish first-result ownership and prevent continued envelope consumption after completion. Delayed input is accepted within the bounded wait without making timeout a successful startup outcome. There is no dedicated repeated-delivery test, so that conclusion rests on the control flow.
Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title is concise, specific, and accurately describes the main change: requiring the bootstrap envelope before server startup.
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.
Description checkPassedThe description includes all required sections: Problem, Change, Scope and approval, and Verification. It identifies the linked issue, explains the behavior change, reports focused test and runtime re…
Full details: Description check

Explanation

The description includes all required sections: Problem, Change, Scope and approval, and Verification. It identifies the linked issue, explains the behavior change, reports focused test and runtime results, and states unverified platforms and limitations. It does not include the maintainer approval comment requested by the template, but the remaining information is sufficiently complete.

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

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

JJulius Marminge4d ago

Taking over this one as maintainer for batch server-persistence-02. I will preserve the contributor commits, merge current main, reduce the tests to focused startup behavior, and run independent implementation and adversarial reviews. No merge is planned.

Added 5 commits

  • Merge remote-tracking branch 'origin/main' into t3/fix-issue-173672344c77
  • fix(server): release bootstrap readers before completing startup inputaa7dc91
  • Merge remote-tracking branch 'origin/main' into pr-17568c349cd7
  • Merge remote-tracking branch 'origin/main' into pr-1756807133a4
  • Merge remote-tracking branch 'origin/main' into pr-17568d52b1cc
JJulius Marminge2d ago

Fresh review found no implementation defects, but current main conflicts in the two test files. Prepared a merge of main that preserves contributor commits and the bootstrap regressions in main’s trimmed test layout: 8c46a3d392, pushed to origin branch fix/issue-17367. The existing cross-fork head is unchanged: cafe checkout cannot resolve its source repository. The prepared branch needs to be applied to the original PR source before landing. Native Windows/Electron startup remains unverified.