refactor(machine): Terminal→Machine naming coherence sweep (part 2) - #2012
Merged
Conversation
#1992 renamed the substrate (PageType.MACHINE, machineId, machine_* tables) but stopped there to stay atomic. This finishes the job: a MACHINE page is no longer rendered by TerminalView.tsx from a directory named page-views/terminal/, and TerminalSession is no longer an exact type-name collision between the DB substrate row and the realtime PTY session. One vocabulary: Machine = the persistent page/Sprite substrate. Terminal = an interactive PTY pane inside a Machine (kept, deliberately — see PR body). - Page-view surface: page-views/terminal/ → machine/; TerminalView → MachineView; TerminalWorkspace → MachineWorkspace; TerminalKeepAliveHost → MachineKeepAliveHost. - Store: stores/terminal-workspace/ → machine-workspace/; useTerminalWorkspaceStore → useMachineWorkspaceStore. Pane-level types (TerminalPaneState, OpenTerminalScope) kept — they are genuine PTY concepts. - DB: terminal_sessions → machine_sessions, pages."terminalAccess" → "machineAccess", global_assistant_config.terminal_access → machine_access. All ALTER ... RENAME; no table rebuild, no data rewrite. - Substrate services: terminal-session-manager → machine-session-manager (low-level acquire renamed acquireMachineSession to avoid colliding with machine-session.ts's pre-existing acquireMachineSandbox wrapper), terminal-storage-* → machine-storage-*, terminal-payer → machine-payer, terminal-pricing → machine-pricing, cron route reconcile-terminal-storage → reconcile-machine-storage. - User-facing: page type displays as "Machine"; "Terminal Access" → "Machine Access"; billing card "Terminals" → "Machines". Verified: db:generate emits a pure RENAME migration and then reports no drift; the full chain applies clean to a fresh Postgres with machine_sessions + renamed indexes, constraints and pkey; typecheck, build and lint green monorepo-wide; 1483 lib + 641 realtime + 2058 web tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2dVpQa811gvgVtZuY1866
Contributor
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (135)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b761a20097
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * overdraw is bounded by TERMINAL_MAX_INFLIGHT instead. Tune via env. | ||
| */ | ||
| export const TERMINAL_HOLD_ESTIMATE_CENTS = envInt('TERMINAL_HOLD_ESTIMATE_CENTS', 2); | ||
| export const MACHINE_HOLD_ESTIMATE_CENTS = envInt('MACHINE_HOLD_ESTIMATE_CENTS', 2); |
There was a problem hiding this comment.
On deployments that already customized terminal billing via TERMINAL_HOLD_ESTIMATE_CENTS or TERMINAL_MARKUP_BPS, this naming sweep silently ignores those variables and falls back to the new defaults (MACHINE_MARKUP_BPS below has the same issue). That can change credit holds or reduce a deliberately higher markup without any config error; please either keep reading the old env names as fallbacks or coordinate a config migration.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch, and it was worse than the symptom you saw — thank you.
The codemod renamed the TS constants, and because the env keys are string literals matching those constant names, it silently rewrote the keys inside envInt(...) too. Env names are an operator-facing contract (same category as the terminal-session:v1 HMAC namespace, which I deliberately left alone for exactly this reason), so that change should never have been implicit. It also left the file half-renamed: MACHINE_MARKUP_BPS sitting next to TERMINAL_MAX_INFLIGHT, TERMINAL_RATES, TERMINAL_ASSUMED_*.
What I did (d0c42b1): rather than patch the two, I finished the family, so the rename is intentional and complete instead of accidental and partial. All nine Machine-billing env vars are now MACHINE_* (MACHINE_MAX_INFLIGHT, MACHINE_MARKUP_FLOOR_BPS, MACHINE_RATES, MACHINE_USD_PER_CPU_HOUR, MACHINE_USD_PER_MEM_GB_HOUR, MACHINE_ASSUMED_CPUS, MACHINE_ASSUMED_MEMORY_GB, MACHINE_STORAGE_USD_PER_GB_MONTH, MACHINE_STORAGE_MEASURE_THROTTLE_MS).
On the fallback specifically — I went with a hard cutover, and here's the evidence that nothing can break:
None of these variables is set anywhere. Not in any repo config (.env*, fly.toml, docker, infra — zero hits), and not as a Fly secret on any deployed app:
pagespace-web: no TERMINAL_*/MACHINE_* secrets
pagespace-realtime: no TERMINAL_*/MACHINE_* secrets
pagespace-cron: no TERMINAL_*/MACHINE_* secrets
pagespace-processor: no TERMINAL_*/MACHINE_* secrets
pagespace-admin: no TERMINAL_*/MACHINE_* secrets
(fly secrets list reports names even though values are digest-only, so this is a definitive check.) Every deployment is therefore already running on the documented defaults, which means renaming the key changes no computed hold, markup, or rate — the concrete risk you identified (a deliberately higher markup being silently lowered) has no instance. Combined with the repo's hard-cutover policy for unreleased features (Machine is gated behind CODE_EXECUTION_ENABLED), a permanent envInt('MACHINE_X', envInt('TERMINAL_X', default)) shim would be dead code on every host that exists. If a deployment had set one, I'd have taken your fallback suggestion instead.
Generalizing your finding: I re-audited the whole diff for the same class of bug — codemod rewriting a string that's actually an external contract. These two env keys were the only ones. No socket event names, no storage keys, no other process.env reads changed. The persisted billing values (source: 'terminal', metadata.type) and the HMAC namespace were already explicitly excluded and are documented in the PR body.
Verified: build + typecheck green monorepo-wide, 1318 packages/lib billing/monitoring/sandbox tests pass (including the MACHINE_MARKUP_BPS override tests, which exercise the renamed key end to end).
Leaving this thread open for your verification.
…w: codex P2) The part-2 codemod rewrote two env-var STRING keys as a side effect of renaming their TS constants — `TERMINAL_HOLD_ESTIMATE_CENTS` and `TERMINAL_MARKUP_BPS` became `MACHINE_*` inside their `envInt(...)` calls. Env names are an operator-facing contract (same category as the `terminal-session:v1` HMAC namespace), so that was an accidental, silent, and half-applied change: it left credit-pricing.ts with MACHINE_MARKUP_BPS sitting next to TERMINAL_MAX_INFLIGHT, TERMINAL_RATES and TERMINAL_ASSUMED_*. Rather than patch the two, finish the family so the rename is intentional and complete. Every Machine-billing env var is now MACHINE_*: TERMINAL_MAX_INFLIGHT -> MACHINE_MAX_INFLIGHT TERMINAL_MARKUP_FLOOR_BPS -> MACHINE_MARKUP_FLOOR_BPS TERMINAL_RATES -> MACHINE_RATES TERMINAL_USD_PER_CPU_HOUR -> MACHINE_USD_PER_CPU_HOUR TERMINAL_USD_PER_MEM_GB_HOUR -> MACHINE_USD_PER_MEM_GB_HOUR TERMINAL_ASSUMED_CPUS -> MACHINE_ASSUMED_CPUS TERMINAL_ASSUMED_MEMORY_GB -> MACHINE_ASSUMED_MEMORY_GB TERMINAL_STORAGE_USD_PER_GB_MONTH -> MACHINE_STORAGE_USD_PER_GB_MONTH TERMINAL_STORAGE_MEASURE_THROTTLE_MS -> MACHINE_STORAGE_MEASURE_THROTTLE_MS No compat fallback, because there is nothing to fall back to: none of these vars is set in any repo config (.env*, fly.toml, docker, infra) or as a Fly secret on pagespace-web/realtime/cron/processor/admin — verified against every deployed app. Every deployment therefore runs on the documented defaults, so renaming the key changes no computed hold, markup or rate. Repo policy is a hard cutover for unreleased features (Machine is gated behind CODE_EXECUTION_ENABLED), so a permanent fallback shim would be dead code. Audited the rest of the diff for the same class of bug: these two were the ONLY external-contract strings the codemod touched. No socket event names, no storage keys, no other process.env reads changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2dVpQa811gvgVtZuY1866
Self-review leftover: machine-access.test.ts (which tests canViewMachine /
canEditMachine — pure Machine substrate) still seeded its page id from a
TERMINAL_ID = 'terminal-1' fixture. Now MACHINE_ID = 'machine-1'.
Also confirmed the remaining `terminal` hits under services/{machines,sandbox},
billing and monitoring are all intentional: they track the persisted
`source: 'terminal'` usage-log value (the markup-override map is keyed BY that
source, so it must match) and the unrelated "terminal status" sense.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J2dVpQa811gvgVtZuY1866
The rename left one silent-failure coupling untested: `uiComponent: 'MachineView'` (page-types.config.ts, packages/lib) has to match the hard-coded string CenterPanel.tsx dispatches on (`componentName === 'MachineView'`, apps/web). Two literals, two packages, nothing tying them together — rename one side alone and a Machine page renders NOTHING rather than failing to compile. A Terminal->Machine sweep is exactly the change that drifts them apart. Also locks the user-facing rebrand: QuickCreatePalette and PageTypeIcon are config-driven, so `displayName: 'Machine'` is the single value that makes the create palette and page tree say Machine instead of Terminal — which is what the epic's manual-verification checklist asks a human to eyeball. Mutation-checked: flipping uiComponent back to 'TerminalView' fails the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2dVpQa811gvgVtZuY1866
Owner
Author
Additional verification: the migration was tested as an upgrade with live data, not just on a fresh DBThe PR body originally claimed only "applies clean to a fresh Postgres". That's the easy path — it exercises
Result: Every value survived. And the constraint that mattered most still works: Final object state — zero residual Practical upshot: a live Machine with a warm Sprite keeps its |
…id-deploy
Self-review caught a production regression I introduced by exceeding the epic's
stated scope. The epic scoped the DB migration to `terminal_sessions` ONLY; I
had also renamed two columns on LIVE tables:
pages."terminalAccess" -> "machineAccess"
global_assistant_config.terminal_access -> machine_access
Deploys run migrations in a separate Fly machine BEFORE the new app image takes
traffic (.github/workflows/docker-images.yml — "Run migrations" at line 155,
"Deploy web" at line 326). `pages."terminalAccess"` is read AND written by
api/pages/[pageId]/agent-config, and global_assistant_config by
api/user/assistant-config — neither is behind CODE_EXECUTION_ENABLED, both serve
live traffic today. So the migration would drop the column out from under the
still-running old image and 500 every agent-config request until the new image
went live several steps later. A column rename has no safe deploy order; only an
expand/contract across two releases does.
Fix: keep the physical columns, keep the code coherent. Drizzle decouples the
field name from the column name, so the TS field stays `machineAccess`
everywhere while mapping to the legacy column:
machineAccess: boolean('terminalAccess') // core.ts
machineAccess: boolean('terminal_access') // integrations.ts
Regenerated 0200: it now contains ZERO live-table DDL — only the
`terminal_sessions` -> `machine_sessions` rename, which is safe because that
table is read exclusively behind CODE_EXECUTION_ENABLED (off). 0201 (pkey
rename) unchanged. `db:generate` reports no drift.
Verified against a real Postgres: reading `pages.machineAccess` returns the
legacy column's value, and writing `machineAccess: false` lands in
pages."terminalAccess" — the mapping round-trips both directions.
The physical column names are now the only residual `terminal_*` in the schema;
that's a deliberate, documented trade (zero-downtime beats cosmetic column
naming) and is a clean expand/contract follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J2dVpQa811gvgVtZuY1866
Owner
Author
|
#2008 landed on master and modified terminal-session-manager.ts — the exact file this branch renames to machine-session-manager.ts — plus realtime/index.ts, agent-terminal-access.ts and agent-terminals-runtime.ts. Git followed the rename, leaving four content conflicts. Resolved by taking MASTER'S content wholesale for every conflicted file and re-applying this branch's renames on top. This branch's edits to those files were rename-only, so nothing of ours is lost, and none of #2008's semantics can be silently dropped: its deletion of `ensureSpriteAwake` and its rewritten reconnect/wake comments are preserved verbatim (grep-verified: ensureSpriteAwake appears nowhere in the merged tree). #2008's three NEW test files came in clean (no conflict) but still referenced the pre-rename symbols (acquireTerminalSandbox, deriveTerminalSessionKey, TerminalSessionStore, ...terminal-session-manager). Applied the same rename map to them, so they now exercise acquireMachineSession / deriveMachineSessionKey / MachineSessionStore against machine-session-manager. Verified: build + typecheck green monorepo-wide; realtime 647/647 pass INCLUDING #2008's 3 new sprite test files (50 tests); packages/lib 7570/7570 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2dVpQa811gvgVtZuY1866
2witstudios
added a commit
that referenced
this pull request
Jul 11, 2026
Addresses the Codex P2 on #2014: `applyEgressLockdown` was also the only thing that recreated SANDBOX_ROOT, so applying it on fresh create only left a warm reconnect exposed if a sandbox command had deleted /workspace — the PTY passes cwd to createSession, the server chdirs into it before spawning, and the session open fails outright. Fixed where the directory is actually consumed rather than by buying the mkdir back on every hand-back (the exec this PR exists to remove): `spawnWithSelfHealingCwd` (pure, in the driver) wraps command+args in an `sh` that recreates and enters the cwd, then execs the real command — preserving the PTY, signals and exit code, with cwd/command/args as positional data args so the no-injection invariant of the arg-array form holds. This is the shape `runCommand` already used; it is now the single definition, shared by the batch path, MachineHost.stream, and the realtime PTY. Also rebased onto master's Terminal→Machine rename (#2012): machine-session-manager, MachineSessionStore/Record, surface 'machine', and the migration renumbered to 0202 on machine_sessions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
2witstudios
added a commit
that referenced
this pull request
Jul 11, 2026
Keeps the branch current with the Terminal→Machine naming sweep (#2012) and the iOS/mobile fixes. No conflicts: the sweep did not touch sprites-shell.ts, replay-dedupe.ts, or their tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CTUvWzi9AkWgDQcKA8DwRH
2witstudios
added a commit
that referenced
this pull request
Jul 11, 2026
* perf(sandbox): apply egress lockdown on fresh create only
The Sprite network policy lives at /.sprite/policy/network.json — a persistent
file that survives pause/hibernation (docs.sprites.dev/concepts/networking).
getOrCreate nonetheless re-pushed it, plus a SANDBOX_ROOT mkdir exec, on EVERY
hand-back: every terminal connect, every tab-back, and every 60s re-auth tick
paid a control-plane round-trip plus an exec for a policy that was already in
place.
The lockdown is now applied exactly when it is not already known-good: a fresh
create (a new Sprite starts on the platform's open outbound), a hash mismatch
(the desired policy changed), or an unknown recorded state (fail closed). The
decision is a pure function — shouldApplyPolicy({fresh, appliedPolicyHash,
desiredPolicyHash}) over a canonical hashPolicy — and the shell records the
confirmed hash on the terminal_sessions row.
The crash window the old unconditional re-apply defended (a crash between
createSprite and its lockdown leaving an open-egress Sprite reachable) is closed
by ORDERING instead: the session row is written only after getOrCreate resolves,
so an unlocked Sprite is never linked to a session. A lockdown failure on fresh
create still destroys the Sprite and rejects the hand-back.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
* fix(sandbox): self-healing cwd on PTY open; rebase onto Machine rename
Addresses the Codex P2 on #2014: `applyEgressLockdown` was also the only thing
that recreated SANDBOX_ROOT, so applying it on fresh create only left a warm
reconnect exposed if a sandbox command had deleted /workspace — the PTY passes
cwd to createSession, the server chdirs into it before spawning, and the session
open fails outright.
Fixed where the directory is actually consumed rather than by buying the mkdir
back on every hand-back (the exec this PR exists to remove): `spawnWithSelfHealingCwd`
(pure, in the driver) wraps command+args in an `sh` that recreates and enters the
cwd, then execs the real command — preserving the PTY, signals and exit code, with
cwd/command/args as positional data args so the no-injection invariant of the
arg-array form holds. This is the shape `runCommand` already used; it is now the
single definition, shared by the batch path, MachineHost.stream, and the realtime
PTY.
Also rebased onto master's Terminal→Machine rename (#2012): machine-session-manager,
MachineSessionStore/Record, surface 'machine', and the migration renumbered to
0202 on machine_sessions.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
* fix(sandbox): self-heal a deleted workspace on the file-write path too
Second half of the same gap the PTY fix closed. The Sprite fs API does not create
parent directories, so `writeFiles` also depended on the egress lockdown's
per-hand-back `mkdir` to guarantee SANDBOX_ROOT existed. With the lockdown now
fresh-create-only, an agent that had `rm -rf`'d /workspace would fail every
subsequent file write.
Folded into the exec the fs path was already paying for: `fsWithWakeRetry`'s
recovery exec now takes the directories the op needs, so the single exec between
a failed op and its retry BOTH wakes the VM (the reason it exists — the fs API is
a bare fetch that cannot wake a hibernated Sprite) and `mkdir -p`s the parents.
Zero extra round-trips on the happy path; the read path is unchanged (`sh -c :`).
New pure functions + tests: `parentDir` (POSIX by construction — these are paths
inside the Linux VM, so node:path would apply Win32 semantics on a Windows host)
and `fsRecoveryExec`. Directories are positional data args, never interpolated
into the script.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
* fix(sandbox): key the egress record on the Sprite instance, not just the policy
Addresses the Codex P1 on #2014: a policy-hash-only record could not tell one VM
from its replacement, which opened a containment hole on the vanish-and-recreate
path. Given a session row whose hash matches the desired policy but whose Sprite
has vanished, two concurrent getOrCreate calls race: A creates the replacement
(which starts on the platform's default OPEN egress) and has not yet reached its
lockdown; B then finds that new Sprite by name, sees fresh === false, and — since
the recorded hash still describes the DESTROYED Sprite's policy — skipped the
push and handed back an unlocked VM.
The record is now a LOCKDOWN TOKEN over (Sprite instance id, policy hash): proof
that a specific policy was applied to a specific VM. The SDK hydrates `id` from
the API response on both getSprite and createSprite, so reading it costs nothing.
A replacement VM has a different id, so B's token does not match and B locks it
down itself (both callers pushing the same policy is harmless; one skipping it is
not). An SDK that reports no id yields NO token — unprovable, so never recorded
as proof, and the next hand-back re-applies. Fail closed throughout.
The driver now returns the token it confirmed and the shell persists that, rather
than the shell computing what it hoped for: machine_sessions.egressPolicyToken
(migration 0202), threaded through SandboxHandle/MachineHandle.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 12, 2026
2witstudios
added a commit
that referenced
this pull request
Jul 13, 2026
* fix(cron): schedule machine-storage billing reconcile hourly The /api/cron/reconcile-machine-storage route (PR #2012) existed with GET and POST handlers but had no crontab entry, so machine persistent-storage billing never reconciled. Add an hourly GET entry alongside the other reconcile/sweep jobs; the route is idempotent via per-machine last-billed watermarks, so hourly cadence is safe and missed runs self-correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWBkQUgKqCBphueo738YPo * fix(cron): serialize machine-storage reconciles with flock Review flagged that charge + watermark-advance in reconcileMachineStorage are two un-transactioned writes, so two CONCURRENT invocations that both snapshot the same storageLastBilledAt could bill the same elapsed window twice. Wrap the crontab entry in flock -n: an overlapping tick is skipped rather than queued, and the skipped window is caught up by the watermark on the next hour. Install util-linux flock explicitly in the cron image rather than relying on a busybox applet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWBkQUgKqCBphueo738YPo * fix(cron): bound cron requests, make skips visible, honest overlap docs Review-pass hardening of the machine-storage reconcile schedule: - cron-curl now runs curl with -sS --connect-timeout 10 --max-time 3300: a wedged request could previously hold the reconcile flock forever, silently starving every future tick (and any entry could hang/pile up); now every request is bounded below the hourly tick and transport errors print into the entry's log instead of vanishing with -s. - The reconcile entry logs skipped/failed ticks via || echo, so a held lock is distinguishable from a healthy quiet run. - Lock moved /tmp -> /run (never bulk-cleaned; unlinking a held lock file would silently break mutual exclusion). - Crontab comment no longer over-promises: flock is a best-effort guard for this container's own ticks; in-service serialization for ALL callers (manual POST, out-of-band second machine, client-disconnect with server-side run continuing) is tracked follow-up work. - route.ts docblock corrected (comment-only): it claimed "overlapping or repeated runs never double-bill", but the charge and watermark advance are two un-transactioned writes — concurrent runs CAN double-bill. Sequential-rerun idempotency claim kept, concurrency caveat added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWBkQUgKqCBphueo738YPo --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finishes the rename #1992 deliberately stopped halfway through. Before this PR a
PageType.MACHINEpage was rendered byTerminalView.tsxout of a directory literally namedpage-views/terminal/,useTerminalWorkspaceStorewas keyed bymachineId,terminal_sessionsstored Machine substrate state, andTerminalSessionwas an exact type-name collision — the DB substrate row and the realtime PTY session shared one name.One vocabulary now: Machine = the persistent page/Sprite substrate. Terminal = an interactive PTY pane inside a Machine.
Ships as one atomic PR: a half-done rename breaks the build, which is exactly what #1993 had to hotfix.
What changed (the 6 groups)
git mv page-views/terminal/ → machine/(history preserved);TerminalView → MachineView;TerminalWorkspace → MachineWorkspace;TerminalKeepAliveHost → MachineKeepAliveHost+terminal-keepalive.ts → machine-keepalive.ts.uiComponentstring and thecomponentName === 'TerminalView'check inCenterPanel.tsxupdated together.stores/terminal-workspace/ → machine-workspace/;useTerminalWorkspaceStore → useMachineWorkspaceStore.terminal_sessions → machine_sessions(table + indexes + pkey). This resolves theTerminalSessioncollision. The two*Accesscolumns on live tables are deliberately NOT renamed — see the deploy-safety note below.terminal-session-manager → machine-session-manager,terminal-storage-{reconcile,billing,measure} → machine-storage-*,terminal-payer → machine-payer,terminal-pricing → machine-pricing, cron routereconcile-terminal-storage → reconcile-machine-storage.QuickCreatePaletteandPageTypeIconare config-driven, so they rebrand frompage-types.config.tswith no code change.terminalId— gone from all code; only immutable historical migrations still contain it.One name I had to choose
machine-session-manager.ts's low-levelacquireTerminalSandboxcould not becomeacquireMachineSandbox—machine-session.tsalready exports anacquireMachineSandbox(the authorize-then-acquire wrapper, Bucket A). The low-level one is nowacquireMachineSession, which also matches its module.machine-projects.ts's pre-existingacquireMachineSandboxdeps property is untouched.Bucket B — same-word hits deliberately LEFT ALONE
These are genuine terminal/PTY concepts. Renaming them would be a bug, so please confirm the boundary holds:
XtermTerminal,TerminalPanes,TerminalPaneState,TerminalColumnState,OpenTerminalScope,openTerminalTerminalTab.tsx(andTerminalList,useNodeTerminals)apps/realtime/src/terminal/dir,terminal-session-map.tsand itsTerminalSessiontypeagent-terminals.ts/-store/-types,agent-terminal-handler.ts,agent-terminal:*socket events,buildAgentTerminalSessionKey,agent-terminals-runtime.ts,terminal-activity.tsmachine_agent_terminalstableSquareTerminal/TerminalSquareTERMINALstatus,isTerminalStatus, AITerminalReason/terminalReason,GIT_TERMINAL_PROMPTBehaviour-bearing strings I did NOT rename (deliberate)
NAMESPACE_VERSION = 'terminal-session:v1'(machine-session-manager.ts) — HMAC input for every session key. Renaming it re-derives every key and orphans the warm Sprite behind each livemachine_sessionsrow. Annotated in place so nobody "tidies" it later.source: 'terminal',metadata.type: 'terminal_machine' | 'terminal_storage',model: 'terminal-machine-storage'. These are persisted billing-attribution values; rewriting them without a data backfill would silently split historical usage across two labels. Out of scope for a rename PR.(
SandboxSurface's'terminal'member was renamed to'machine'— it's a pure in-memory discriminator that never reaches the DB or the emitted egress tag.)Every Machine-billing env var is now
MACHINE_*(wasTERMINAL_*):TERMINAL_HOLD_ESTIMATE_CENTSMACHINE_HOLD_ESTIMATE_CENTSTERMINAL_MARKUP_BPSMACHINE_MARKUP_BPSTERMINAL_MAX_INFLIGHTMACHINE_MAX_INFLIGHTTERMINAL_MARKUP_FLOOR_BPSMACHINE_MARKUP_FLOOR_BPSTERMINAL_USD_PER_CPU_HOURMACHINE_USD_PER_CPU_HOURTERMINAL_USD_PER_MEM_GB_HOURMACHINE_USD_PER_MEM_GB_HOURTERMINAL_ASSUMED_CPUSMACHINE_ASSUMED_CPUSTERMINAL_ASSUMED_MEMORY_GBMACHINE_ASSUMED_MEMORY_GBTERMINAL_STORAGE_USD_PER_GB_MONTHMACHINE_STORAGE_USD_PER_GB_MONTHTERMINAL_STORAGE_MEASURE_THROTTLE_MSMACHINE_STORAGE_MEASURE_THROTTLE_MSNo action required, and no compat fallback, because there is nothing to fall back to: none of these is set in any repo config (
.env*,fly.toml, docker, infra) or as a Fly secret on any deployed app (pagespace-web/realtime/cron/processor/admin— all verified viafly secrets list, which reports names even though values are digest-only). Every deployment already runs on the documented defaults, so renaming the keys changes no computed hold, markup or rate. Machine is also still gated behindCODE_EXECUTION_ENABLED, and repo policy is a hard cutover for unreleased features. If you ever set one of these, use the new name.Caught by codex review (P2): the codemod had silently rewritten two of these keys as a side effect of renaming their TS constants, leaving the family half-renamed. The rest of the diff was re-audited for the same class of bug — those two were the only external-contract strings touched (no socket event names, no storage keys, no other
process.envreads).Deploy safety: why no live column is renamed
An earlier revision of this PR also renamed two columns on live tables
(
pages."terminalAccess",global_assistant_config.terminal_access). That was a production regression and has been reverted.Deploys run migrations in a separate Fly machine before the new app image takes traffic (
docker-images.yml: "Run migrations" line 155, "Deploy web" line 326). Both of those columns are read and written by endpoints that are not behindCODE_EXECUTION_ENABLEDand serve live traffic today (api/pages/[pageId]/agent-config,api/user/assistant-config). Renaming them would drop the column out from under the still-running old image and 500 every one of those requests until the new image went live several steps later. A column rename has no safe deploy order — only an expand/contract across two releases does.So the columns stay put and the code stays coherent, using drizzle's field/column decoupling:
The TS field is
machineAccesseverywhere; only the physical column names remain legacy. Verified against a real Postgres that the mapping round-trips: readingpages.machineAccessreturns the legacy column's value, and writingmachineAccess: falselands inpages."terminalAccess".machine_sessionsis renamed, because — unlike those two — it is read exclusively behindCODE_EXECUTION_ENABLED(off), so no running code touches it during the window.Those physical column names are the only residual
terminal_*left in the schema: a deliberate trade (zero-downtime beats cosmetic column naming) and a clean expand/contract follow-up.Verification
db:generateproduced a pureALTER TABLE ... RENAMEmigration (0200) — no rebuild, noDROP TABLE/DROP COLUMN, no data rewrite — and now reports "No schema changes" (snapshot chain intact).0201renames the leftoverterminal_sessions_pkeyconstraint. Postgres doesn't rename a pkey with its table and drizzle doesn't track pkey names, sodb:generatewill never emit it. Hand-authored, following the0195_migrate_machine_refs_*precedent from refactor(machines): rename TERMINAL page type → MACHINE, terminalId FK → machineId #1992; guarded with an existence check and metadata-only.machine_sessionsexists withmachine_sessions_page_id_idx,machine_sessions_last_active_at_idx,machine_sessions_pkey, both FKs and the unique constraint — zero residualterminal_session*objects.terminal_sessionsrow + both boolean columns, then applied only0200+0201. Every value survived (sessionKey/sandboxId/storageMeasuredBytes,machineAccess=true,machine_access=true), andON DELETE CASCADEstill fires after the FK drop/recreate — so a warm Sprite keeps itssessionKey → sandboxIdmapping and reattaches instead of being orphaned. Full output in this comment.bun run typecheckgreen monorepo-wide (16/16);bun run buildgreen (incl. apps/web + apps/realtime);bun run lintclean.packages/lib+ 641apps/realtime+ 2058apps/webpassing.TerminalSessionnow resolves to exactly two meanings by file —MachineSessioninpackages/db/packages/lib, PTYTerminalSessiononly inapps/realtime/src/terminal/— and no file references both.Notes for the reviewer
/api/cron/reconcile-machine-storage) is not registered indocker/cron/crontab— it never was. So renaming its path is zero-risk, but the route is still unscheduled; that gap predates this PR.scripts/fix-wrong-imports.mjsstill listedisTerminalPage, an export refactor(machines): rename TERMINAL page type → MACHINE, terminalId FK → machineId #1992 deleted. Fixed toisMachinePage.🤖 Generated with Claude Code
https://claude.ai/code/session_01J2dVpQa811gvgVtZuY1866