mirror of
https://github.com/nestriness/nestri.git
synced 2026-09-26 04:22:26 +03:00
4a2a4412c231507c6dbbdb0723ba49c4ea3afd90
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Wanjohi
|
4a2a4412c2 |
style: apply the formatter across the tree
The formatter had never been run over these files, so `oxfmt` on a couple of touched files rewrote two dozen others and buried the real change. Running it everywhere once makes the next diff mean something. No behaviour changes: import ordering, line joining, and reindented JSON in the generated migration snapshots. Both test suites and both typechecks give the same answers as before, including the two type errors this does not fix. |
||
|
Wanjohi
|
40b4270161 |
refactor(api)!: remove the shared operator secret, and let hosts sync their own
A single secret that turned any request into an operator was the only credential several routes accepted, and it had no caller left: the device pairing it existed for is on hold, and nothing in this tree or any client sent it. What remained was a key that bypassed authentication entirely, required to boot, and checked by nobody. Every route behind it had a better answer available: - Library and game sync move to host credentials. Both took a `userId` in the body, which meant one secret could write into anybody's library. A host now says which of its enrolled users a batch is for, and that claim is checked against the Steam sign-ins it actually holds — one box carries several people's accounts, so the pair is the unit. - Download-state reporting narrows to hosts alone, and the body that could name a different host is gone. Which host is reporting comes from its own credentials, and a body that still names one is refused rather than ignored. - Linking a Steam account is always for the caller. - Creating a game by hand is deleted; syncing already upserts the catalogue. - Reading the waitlist is deleted. Every address on it belongs to someone who has not agreed to anything, and answering it over HTTP made that list something a leaked key could drain. - The pairing-code routes are deleted with the flow they served. The domain module and its table stay, so returning to it is a route file rather than a migration. Nothing in the API now accepts a credential that stands for more than one caller: every request resolves to a specific user or a specific host, which is what lets a route say "the caller's own library" and mean it. BREAKING CHANGE: the `x-nestri-admin-token` header is no longer accepted and `ADMIN_SHARED_SECRET` is no longer read. `POST /games`, `GET /waitlist` and the `/pairing-code` routes are gone; `POST /games/sync` and `POST /library/sync` now require host credentials and take `userId` in the body; `POST /steam/link` no longer accepts `userId`; `POST /games/download-state` no longer accepts `hostId`. |
||
|
Wanjohi
|
cfb8ec26a0 |
feat(machine): mint a public name, and stop routing on the id
A host's hostname was its primary key. That worked and disclosed three things it should not have: ids here are monotonic, so an id in a hostname tells anyone who reads a URL roughly when that machine was registered and where it falls among its owner's others; an id is the primary key, so a name that had to change could only change by re-registering the machine, which is changing its identity to fix its name; and the hostname is also the OAuth audience and the cookie scope, so the id travelled into redirect URLs and browser history. Machines now carry a minted name -- two words and four digits, unique across the fleet, DNS-safe by construction, which an id was not. The words are a curated list rather than a dictionary, because every pair is shown to strangers. Names the fleet's own infrastructure answers on are refused at mint time: one minted onto the edge's own label would take the published key path away from every host at once. Minting retries on the unique index rather than checking first, because two registrations in the same instant both read "free" and both write. Only a name collision retries; a duplicate id or secret means something a new name cannot fix. The migration adds the column in three steps. Generated as a NOT NULL column it fails outright against a populated table, and a default would be worse: every row would share one value on a routing key. |
||
|
Wanjohi
|
aecae0c69e |
style(auth): tighten the sign-in field, and stop calling the runtime a worker
The email field sat a little taller than the action below it. Two comments also described this page as rendering inside a worker, which is not how the control plane runs; what they were reaching for is that nothing preprocesses this file, so the design tokens are written out longhand. |
||
|
Wanjohi
|
74391714d0 |
feat(auth): draw the sign-in screen in the product's design language
The screen was still the upstream template's: its font, its accent, its logo, and a theme that tried to serve a light and a dark scheme from one set of colours by deriving each one from the background's lightness. That derivation is replaced with stated values, and the page is dark only. Black, a neutral grey ramp, one brand accent, Mona Sans for the display line and Geist for anything read or typed; two dashed bands closing into a box on a wide screen, a dashed vertical either side of the column, the wordmark, and the line of copy the rest of the product opens with. The email field keeps the fix from the previous commit and takes the brand colour on focus, which is the only place it appears besides the wordmark. Measured against a render of the same design built from its own source: the band rules, the column rules, the wordmark box and the button height land on identical pixels. |
||
|
Wanjohi
|
4ed36e0d38 |
fix(auth): give the sign-in input a text colour, and restore the theme
The email field computed its own background one step lighter than the page and never set `color`. Form controls do not inherit it, so the text someone typed was the UA default — black, over a near-black field. There was no `color-scheme` either, so the browser rendered the control in light appearance to begin with. The theme was a second, separate loss: `issuer()` still takes one and calls `setTheme`, but nothing had passed one since `packages/auth` became a vendored fork, so every sign-in rendered as OpenAuth — its font, its periwinkle, its logo. Restored from the pre-fork config, with the logo and favicon repointed because both URLs it carried now 404 and a broken `logo` renders a broken image rather than falling back. |
||
|
Wanjohi
|
7859c8e13a |
ci: restart the issuer before the API
Units come back in the order their artefacts appear in the manifest, and the API reaches the issuer over AUTH_INTERNAL_URL -- so api-then-auth means the API spends a moment talking to a service that is restarting. |
||
|
Wanjohi
|
4668c98785 |
ci: a merge to prod releases the control plane, migrator included
`release-prod.yml` for api and auth, mirroring the edge's. The gate is not "you may not merge" — it is that merging does not deploy: tests run against a real Postgres, both binaries are executed, and a failure anywhere means no release exists, so the machine keeps serving what it has. The new piece is `nestri-migrate`, because the deploy runs migrations *before* it swaps a release into place and had nothing to run. `drizzle-kit migrate` reads the migrations folder at runtime, which is right on a laptop and wrong on a server: the deploy ships flat, checksummed files into bin/, and a migrator that needs a directory beside it can be pointed at the wrong directory. So the folder is baked into the binary — generated on every build rather than committed, so it cannot drift — and the artefact's checksum then covers every statement it will run. It reimplements drizzle's bookkeeping in thirty lines of SQL rather than calling into `db.dialect.migrate`: same table, same schema, same sha256 over the whole file, same high-water-mark comparison. That equivalence is the one thing that must not rot, because the production database was first migrated by drizzle-kit and a disagreement means a migration applied twice. Two checks hold it: CI applies the migrations with drizzle-kit and then asserts the embedded set reports nothing pending, and the hashes were verified by hand against the live database — all fourteen match to the byte. Proven before shipping, against the real database: nothing pending on the deployed schema, 14 unchanged rows, and a scratch database migrated from empty to the same 22 tables and the same high-water mark, idempotent on a second run. Refuses with exit 2 when DATABASE_URL is absent rather than defaulting to localhost, which would be a migrator reporting success having migrated nothing. setup-bun is pinned to a commit and not to `v2`. A moving major tag is fine everywhere else in this repository; this workflow is the only thing between a merge and a process serving users, and there is no first-party bun action to prefer instead. |
||
|
Wanjohi
|
b00f1064ae |
fix(api,auth)!: bind loopback by default, and let a container ask for more
Both servers bound `0.0.0.0`. That was harmless while the only deployment was docker-compose.yml, which publishes these ports on 127.0.0.1 and makes the container's own bind irrelevant. As ordinary processes on a rented machine there is no such wrapper, and `0.0.0.0` is a listener on the internet — in front of an issuer that sets cookies without `Secure` and mints sign-in codes, because it expects something else to be terminating TLS. So the default is `127.0.0.1` and `HOST` is there for the deployment that genuinely needs every interface. compose now sets `HOST: 0.0.0.0` explicitly, which is not a workaround: inside a container, binding loopback is what would make the published port unreachable. The right answer differs by deployment, which is why it is a variable rather than a constant. Both now log the address they bound, not the one they hoped for. BREAKING CHANGE: api and auth no longer listen on every interface by default. A deployment that relied on that must set HOST=0.0.0.0. |
||
|
Wanjohi
|
e3c9416170 |
docs(dns): the origin is a tunnel, and there is nothing to cut over from
`After the move off Workers` described proxied `A` records to the host and an ordering to avoid a gap while Worker routes were removed. Both are wrong. There is no `A` record because there is no address to publish: each name is a proxied CNAME to the tunnel, and the machine's address appears nowhere. And there is no ordering problem because `api.nestri.io` and `auth.nestri.io` have no DNS records and do not resolve — only the sandbox pair was ever deployed, so these names are created for the first time with no window to protect. The `nestri.link` section promised an origin certificate as the one key on our host. There is no key on our host at all. Measured 2026-09-16: a public HTTPS request served correctly while nothing listened on 443 or 8443 and both origins spoke plain HTTP on loopback; the certificate was Cloudflare's, covering the apex and one level, and a two-label name was refused at the handshake with no certificate presented. |
||
|
Wanjohi
|
ec8b13d0c9 |
fix: A host-only session cookie, and a column for where a host actually is (#331)
Closes #330. Two gaps that block a reverse proxy sitting in front of user-owned hardware and authenticating browsers on its behalf, plus one thing found on the way that is worse than either. ## 1. `machine` records where a host is `machine` said who owns a host, which team it belongs to and when it was last seen, and nothing about how to reach it — so a request arriving for a machine could be authorised perfectly and then have nowhere to go. `endpoint_id` is **reported, never assigned**: a host holds the secret half of that identity and is the only thing that can know the public half first, so it rides on the beat it already sends as itself. Migration `0013`. - **Nullable**, because "has never reported one" is a real state that every host registered before today is in. Null reads as *not reachable yet*; a default would read as an address and route somewhere wrong. - **Unique**, because an endpoint id belongs to one host. Two rows claiming the same one would send a request addressed to one machine to another machine's agent, which is the one mistake here the authorisation in front of it cannot catch. - **Omitting the field leaves the stored value alone.** An agent that does not say where it is has not moved, and an absent field must never read as "nowhere" — that would take every host shipped before this field off the map on its next beat. There is a test for exactly that. ## 2. The session cookie, and why it is not shaped the way the issue asked The issue asked for a `__Host-nestri-session` set on sign-in carrying the access token unwrapped. Building it turned up two reasons that cannot work, and item 3 of the issue is the reason why: 1. **There is nowhere to set it.** The sign-in UI is served by the issuer on its own hostname, and the web client has no auth code at all. A `__Host-` cookie set at sign-in is host-only *to the issuer* — a host that does not need one. 2. **The value would be a control-plane credential** sitting on hardware the control plane does not run. Item 3 said the hand-off had to be settled from both ends and did not pick a shape. It is settled now, in the way the issue's own two rules point at: **the proxy is an ordinary public OAuth client, one per hostname**, and what crosses in the URL is an **authorization code** — single-use, sixty seconds long, redeemed exactly once by the store that already exists here, and exchanged over a back channel. A code in an access log is worthless by the time anybody reads it, which is the hazard the issue named. No credential in a URL, and no caller-supplied return address. What this repo owes that flow is one rule, and it is the whole of the change to the issuer: **a client id that is a single hostname under the host zone, whose `redirect_uri` is `https` and that same hostname at one reserved path, is an allowed client.** Everything else keeps the behaviour it had. Making the client id the hostname is load-bearing rather than tidy. A token's `aud` is its client id, so the session that comes back is **bound to the host it will live on**, with no change to how tokens are minted — and it is not a credential on any other host, nor a control-plane credential at all. That is a better answer than "the access token, exactly", and it costs nothing. ## 3. `/authorize` was an open redirector Found while testing the above. A refused client's `redirect_uri` was still used to deliver the refusal — and the check that approves that URI is the one that just failed: ``` GET /authorize?client_id=web&redirect_uri=https://somewhere.example/callback -> 302 https://somewhere.example/callback?error=unauthorized_client ``` No sign-in required, on the hostname people are asked to type a password into. A refusal is now a page here. This is in `packages/auth` and is independent of everything above; it is in this PR because the flow above is built on that path and shipping one without the other would have been odd. ## Tests Every case below fails against the unmodified code and passes after. `379 pass, 0 fail` across `packages` and `apps`. - `apps/auth/test/allow.test.ts` — the allowed case; a code is never sent anywhere but the client id; only the reserved path; `https` only; one label, because `a.b.zone` is not a host id; another zone does not get in by using the path; the three existing rules unchanged; and the refusal is a page rather than a redirect. - `packages/core/src/machine/machine.test.ts` — reported and read back, a silent beat leaves it alone, four malformed ids refused, two machines cannot claim one id. - `apps/api/test/heartbeat.test.ts` — a beat with no body is still a beat, a host cannot report where *another* host is, a malformed id is a `400`. ## What this does not verify - **No end-to-end run against a real browser.** The flow is exercised from the proxy's side and from this side separately; nothing has driven a browser through sign-in and out the other end. - **The token's shape is asserted to the written contract, not captured from a running issuer.** If minting changes, these tests pass and a signed-in person is redirected to sign in again. - **`machine_endpoint_id_unique` is not exercised under concurrency.** Two hosts reporting the same id in the same instant is a database-level race that the tests assert the *outcome* of, sequentially. - **Nothing here makes a hand-off silent.** The issuer keeps no session of its own, so a second hostname asks for an email code again. That is not a regression — nothing anywhere is silent today — but it is the next piece of work, and it is what the issue's item 1 will eventually be, on the issuer's own hostname. - Pre-existing `tsc` errors remain in `apps/api/app/utils/{hook,validator}.ts` and `packages/core/src/session/session.test.ts`; none is touched here. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR adds a durable, host-reported endpoint identity and permits tightly constrained OAuth callbacks for individual hostnames. It also prevents rejected OAuth clients from controlling the error redirect. - Adds a nullable, unique machine endpoint ID with migration and heartbeat reporting. - Preserves endpoint IDs when legacy agents send bodyless heartbeats. - Converts duplicate endpoint claims into the API’s stable HTTP 409 conflict response. - Allows HTTPS authorization callbacks only at the reserved path on the matching single-label host. - Returns unauthorized-client failures locally instead of redirecting to an unapproved URI. <h3>Confidence Score: 5/5</h3> The PR appears safe to merge; the previously reported endpoint-conflict failure is now translated and tested as a stable HTTP 409 response. No actionable new failure remains. The prior endpoint-conflict finding is fully fixed by translating PostgreSQL uniqueness failures into `already_exists`, which the API maps to 409, with both domain-level and route-level coverage. <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | apps/auth/src/index.ts | Adds strict host callback authorization while retaining the existing same-domain and local-development rules. | | packages/auth/src/issuer.ts | Prevents unauthorized clients from using their rejected redirect URI as an open-redirect destination. | | packages/core/src/machine/index.ts | Persists optional endpoint reports and translates duplicate endpoint claims into a stable domain conflict. | | apps/api/app/routes/machine.ts | Extends machine heartbeats with optional endpoint reporting and documents the 409 response. | | packages/core/migrations/0013_machine_endpoint_id.sql | Adds the nullable endpoint column and database-enforced uniqueness invariant. | <h3>Sequence Diagram</h3> ```mermaid sequenceDiagram participant B as Browser participant A as Auth issuer participant P as Host proxy participant H as Machine participant D as Database H->>A: Authenticated heartbeat with optional endpointId A->>D: Update lastSeen and, when supplied, endpointId D-->>A: Stored or unique conflict A-->>H: 200 or typed 409 B->>A: /authorize for host.nestri.link A->>A: Validate matching HTTPS reserved callback A-->>B: Provider flow B->>A: Complete authentication A-->>P: Short-lived authorization code P->>A: Exchange code A-->>P: Host-audience session tokens ``` <sub>Reviews (2): Last reviewed commit: ["fix(machine): a taken endpoint id is a c..."](https://github.com/nestrilabs/nestri/commit/51d25f3e8ce48d4eadef868736334d5dfbd5c05c) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=61026693)</sub> <details><summary><h4>Context used (5)</h4></summary> - Knowledge Base — [API HTTP composition and authorization](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/api-http-and-auth.md) - Knowledge Base — [Core domain and persistence](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-domain-data.md) - Knowledge Base — [Machines, pairing, and access tokens](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-machines-and-pairing.md) - Knowledge Base — [Authentication platform](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/auth-platform.md) - Knowledge Base — [Auth providers, sessions, and storage](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/auth-providers-and-storage.md) </details> <!-- /greptile_comment --> |
||
|
Wanjohi
|
51d25f3e8c |
fix(machine): a taken endpoint id is a conflict, not a server fault
A host reporting an endpoint id another machine already holds hit the unique index, and the raw refusal reached the global handler as a 500 -- telling a host its beat broke the server rather than that the id is taken. It is now the 409 every other conflict here gives, and the route documents it. Checked-then-written would be worse rather than better: two hosts reporting the same id in the same instant both read "nobody holds it" and both write, which is precisely what the index is for. The read would add a query and remove nothing. Before: expect(res.status).toBe(409) Received: 500 |
||
|
Wanjohi
|
49ae45624e |
fix(auth): a host may receive a code at its own name, and a refusal is not a redirect
Two changes to who may start a flow here, and where a refusal is delivered. A host reached at its own hostname sits on a different registrable domain from this issuer, deliberately: that is what stops a cookie set there from ever reaching this one. The default rule allows a redirect back to whatever hostname the request arrived on, so it refused exactly the case the separation created. Which is a real problem rather than a theoretical one, because a session cookie without a Domain attribute is host-only, so a browser arriving at one of those hostnames for the first time carries no cookie whether or not it is signed in, and sending it here to sign in again changes nothing. So a client id that is a single hostname under that zone, whose redirect_uri is https and that same hostname at one reserved path, is allowed. Making the client id the hostname is the load-bearing part: a token's audience is its client id, so the session that comes back is bound to the host it will live on and is not a credential anywhere else. Separately, and worth its own paragraph: a refused client's redirect_uri was still used to report the refusal. The check that approves that URI is the one that just failed, so /authorize was an open redirector to anywhere at all -- no sign-in required, on the hostname people are asked to type a password into. It is now a page here. Before: GET /authorize?client_id=web&redirect_uri=https://somewhere.example/callback -> 302 https://somewhere.example/callback?error=unauthorized_client |
||
|
Wanjohi
|
6603383ad1 |
feat(machine): record where a host can be reached, as the host reports it
The machine table said who owns a host, which team it belongs to and when it was last seen, and nothing about how to reach it. Anything standing in front of a host and authenticating browsers on its behalf could therefore authorise a request perfectly and then have nowhere to send it. Reported, never assigned. A host holds the secret half of this identity and is the only thing that can know the public half first, so it rides on the beat it already sends as itself. Omitting the field leaves the stored value alone -- an agent that does not mention where it is has not moved, and an absent field must never read as "nowhere", which would take every host shipped before this field off the map on its next beat. Nullable, because "has never reported one" is a real state that every host registered before today is in. Unique, because an endpoint id belongs to one host: two rows claiming the same one would send a request addressed to one machine to another machine's agent, which is the one mistake here that the authorisation in front of it cannot catch. |
||
|
Wanjohi
|
0e94620808 |
feat(nesinit): carry the session's address out of the guest (#328)
## What was missing
`neshub` serves the session's address on a socket, and its own flag has
always
said how that address gets out:
> *"neshub listens; nesinit dials and carries the ticket to the host,
because
> the person who needs it is outside this VM and stdout here is a log
file
> inside one."*
Nothing dialled it. `grep -rn ticket apps/nesinit/src` returned nothing
at all,
so the address never left the guest — and the one lifecycle message for
it,
`Ticket`, had no sender.
This adds the carrier.
## Three decisions worth reading
**Polled, not read once.** An address is not a value, it is the best
answer so
far: an endpoint discovers more ways to reach it after it binds — a
local one
immediately, a relayed one seconds later. Reading once means whoever
asked first
decides, and the first answer is the one that works on a local network
and fails
from anywhere else. Only a *changed* answer is forwarded, so an
unchanged one
costs nothing.
**Dials, does not listen.** The opposite of the payload relay next door,
and
deliberately so. There the guest listens because the workload starts
later; here
the server is the long-lived one. Dialling also makes "not bound yet" an
error to
retry rather than a connection to wait for without knowing whether it is
coming
— which is the ordinary case at boot, since this starts before the
server does.
**The address is never logged.** It is a capability to reach the
session, and a
log inside the guest is the one place it has no reason to be. The log
line says
whether it is the first one and nothing else.
## Failing first
The carrier's tests fail against unmodified code by not compiling:
`ticket.rs`
does not exist and `session::run` takes three arguments. Said plainly
rather
than manufactured. The behavioural gap is better shown as the `grep`
above —
nothing in the guest ever sent a `Ticket`, so the message had one end.
`nesinit`: **35 tests passing**, up from 27. Four on the carrier itself
(an address arrives; a better one replaces it; a socket that is not
there yet is
waited out rather than failed; an empty answer is not an address) and
two on the
session (an address reaches the caller as `Ticket`; a carrier that stops
does
not end the session).
## Verified in a real guest
Built static for musl, run as PID 1 in a real microVM under a real VMM
with a
real vsock. It dialled out, completed the handshake at version 2, took a
boot
descriptor, started its workload, read the address that workload
published and
sent it up the channel — twice, the second time because a better one
appeared.
The caller saw `nestri:local-only` and then `nestri:with-relays`.
Guest boot to init was **310 ms**.
## Review round (
|
||
|
Wanjohi
|
f74de9beb8 |
fix(nesinit): do not mount over the share tree, and check who serves an address
Four findings from review, all of them real. The relay's directory was mounted on the tree a session's shares live in. A fresh tmpfs there hides every directory the image prepared underneath it: the install, the user state, the work directory, and the mount point the log share is attached to from fstab. A box would have come up with a socket and without any of the places its workload looks for its files, and the exact-path check could not notice, because what fstab mounts is a directory inside that tree rather than the tree itself. It moves to /run, which is where a runtime socket belongs, is a tmpfs already, and has nothing else mounted inside it. It was also owned by this process and closed to everyone else, which stopped the workload traversing it to reach the relay at all. The directory is now readable and searchable, and still writable by nothing but this process, which is what makes the socket in it unreplaceable; the socket itself is what the workload is allowed to connect to. The permission belongs on the socket rather than on the path. The address served to a reader was built once at startup and served forever, so a reader that polls for a better one could only ever get the first. An endpoint does not know all of its own addresses when it binds: the first is the one that works on the same network and fails from anywhere else. It is now rebuilt per read, which is what makes polling for it worth doing. And the address was taken from whoever held a path in a directory the workload can write. Workload code could unlink the socket a service was listening on, bind its own, and every read afterwards would hand the client an address of its choosing -- a session given to somebody else rather than a session that fails. The peer's credentials are now checked before a byte is read, from the kernel rather than from anything the peer says about itself, and an address served by the workload's own user is refused and said loudly. That check is only worth something while the workload has a user of its own, so the image grows one. Two users, and they must stay two: one runs the services that ship in the image, the other is who a workload runs as. Sharing one does not weaken the check, it makes every session fail it. A workload running as root is every user at once and cannot be told apart from anything; the check stands down there and says so at boot instead, because refusing root would refuse whatever legitimately serves the address as well. Also bumps tinyvec by a patch release. It does not build on this toolchain -- `vec` resolves to the module and not the macro -- which made every crate that depends on an endpoint, including this one, unbuildable. Pre-existing and nothing to do with this change; the lockfile said the same version before it. |
||
|
Wanjohi
|
2a7be92a41 |
ci: run each half only when that half changes (#329)
## Why
Both jobs ran on every pull request. A change to a Rust binary waited on
a
Postgres service and a full TypeScript test run; a change to a
TypeScript route
spent a runner compiling Rust. Neither result told anyone anything.
## What changed
A `paths:` filter belongs to a **workflow**, not to a job — so the two
jobs
become two workflows. That is the entire cost of the change:
| | |
|---|---|
| `.github/workflows/web.yml` | the TypeScript half — `bun test` over
the control-plane apps and shared packages |
| `.github/workflows/nesdoctor.yml` | the Rust half — `fmt`, `clippy`,
`test`, and a no-network run |
| `.github/workflows/ci.yml` | deleted; it was the two of them together
|
**Both job bodies are carried over unchanged.** Parsed and compared
rather than
eyeballed:
```
web job body identical to ci.yml: True
nesdoctor job body identical to ci.yml: True
```
Only the triggers differ. `push` is untouched (see the first note
below).
## The filters, and where they come from
**`web`** — every TypeScript workspace member, plus the things that
reach all of
them. `packages/` is entirely TypeScript so it is taken whole; `apps/`
is mostly
Rust, so its two TypeScript members are named.
```
apps/api/** apps/auth/** packages/**
package.json bun.lock tsconfig.json oxlintrc.json
.github/workflows/web.yml
```
**`nesdoctor`** — its own directory, plus the workspace root and
lockfile, which
pin every version it builds against. No other member is listed because
it
depends on no other member; its dependency tree is four external crates
deep and
that is deliberate.
```
apps/nesdoctor/** Cargo.toml Cargo.lock
.github/workflows/nesdoctor.yml
```
## Verified by simulating the filters, not by reading them
The failure mode of a path filter is *silence* — a wrong pattern means
the job
never runs and the pull request goes green. So the globs were
implemented in
GitHub's dialect (`*` stops at a slash, `**` crosses them) and run
against real
change sets, including the actual file list of the last merged PR:
```
the enrolment PR, actual file list → web
a TS route only → web
a core module only → web
a migration only → web
the auth worker → web
the shared auth package → web
the bun lockfile → web
lint config → web
nesdoctor source → nesdoctor
nesdoctor README → nesdoctor
the Rust workspace root → nesdoctor
the Cargo lockfile → nesdoctor
another Rust app → (nothing)
a shared Rust crate → (nothing)
the web workflow itself → web
the nesdoctor workflow itself → nesdoctor
docs only → (nothing)
the root README → (nothing)
a stray root artefact → (nothing)
both halves at once → web, nesdoctor
```
`another Rust app` and `a shared Rust crate` firing nothing is correct
**today**
— CI covers `nesdoctor` alone, and the rest of the Rust half has never
been
under it. It stops being correct the moment a second member is added to
CI, and
each new member wants its own filter alongside its own job.
Both jobs were also run locally with the exact commands the workflows
use:
`nesdoctor` — fmt clean, clippy clean under `-D warnings`, 18 tests
pass, and
the binary runs; `web` — migrations apply and 363 tests pass, 0 fail.
## Three things found on the way, none of them fixed here
1. **`push: branches: [main]` is inert.** There is no `main` branch —
the
default is `dev` — so the push half of this trigger has never fired and
does
not fire now. I carried it over verbatim rather than "fixing" it to
`dev`,
because that would *add* CI runs and this PR exists to remove them. One
word
either way; your call.
2. **A new TypeScript app will silently not be tested** until someone
adds it to
`web.yml`'s list. Nothing detects this. It is written as a comment in
the
file, in the place someone editing that list will be looking.
3. **`nesdoctor.json` is committed at the repo root** and appears to be
a report
generated on someone's machine — it carries a specific CPU, kernel and
disk
layout. It is an output rather than an input, so no filter references
it.
Probably wants deleting and gitignoring, separately.
## Before turning on required status checks
Path-filtered workflows do not report at all when they do not match,
which
branch protection reads as *expected but missing* — a pull request that
touches
only docs would never become mergeable. There are no required checks on
`dev`
today (`required_status_checks: null`, checked), so nothing is broken by
this.
If you enable them later, the usual answer is a companion job that
always runs
and reports success under the same name.
## What this does not verify
- **The simulation implements GitHub's glob dialect; it is not GitHub.**
This
pull request is the first real exercise of it: it changes both workflow
files, each of which lists itself, so **both jobs should run here** —
which
is the intended behaviour, since a change to how the tests run is a
change
worth running. Anything else on the checks tab means a filter is wrong.
(An earlier draft of this section predicted *neither* would run. That
was
wrong, and the simulator says so: `this PR itself → web, nesdoctor`.
Left
visible because it is exactly the mistake path filters invite —
reasoning
about which files a change touches without checking.)
**Confirmed on the runner**, which is no longer a prediction:
```
nesdoctor / nesdoctor → success (pull_request)
web / web → success (pull_request)
```
- **`actionlint` was not available**, so the workflow files are
validated as
YAML and by parsing their trigger and job structure, not by a
schema-aware
linter.
- **Nothing is measured.** No before/after timings — the saving is "a
job that
had no reason to run does not run", not a number I benchmarked.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR splits the combined CI workflow into independently filtered web
and nesdoctor workflows while preserving their existing job bodies.
- Web tests now run for changes to current TypeScript workspace members
and their shared configuration.
- Nesdoctor checks now run for changes to its crate, Cargo workspace
inputs, or its workflow.
- Unrelated pull requests no longer start both test stacks.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge; the new filters cover the current inputs
of both preserved CI jobs.
No actionable failure remains: current workspace members and build
inputs are covered, no in-repository consumer relies on the old workflow
identity, and the split does not increase permissions or action
exposure.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| .github/workflows/web.yml | Extracts the unchanged TypeScript test job
into a workflow filtered to all current web workspace members and
relevant shared inputs. |
| .github/workflows/nesdoctor.yml | Extracts the unchanged nesdoctor
checks into a workflow filtered to the crate and its Cargo workspace
inputs. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart TD
PR[Pull request changes] --> W{Matches web paths?}
PR --> N{Matches nesdoctor paths?}
W -->|Yes| WT[Run Bun install, migrations, and tests]
W -->|No| WS[Skip web workflow]
N -->|Yes| NT[Run fmt, clippy, tests, and no-network smoke run]
N -->|No| NS[Skip nesdoctor workflow]
```
<sub>Reviews (1): Last reviewed commit: ["ci: run each half only when
that half
ch..."](https://github.com/nestrilabs/nestri/commit/f27ea3a132880ee2aba640329caa542b12cc5e24)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=60935535)</sub>
<!-- /greptile_comment -->
|
||
|
Wanjohi
|
f27ea3a132 |
ci: run each half only when that half changes
Both jobs ran on every pull request, so a change to a Rust binary waited on a Postgres service and a full TypeScript test run, and a change to a TypeScript route spent a runner compiling Rust. Neither told anyone anything. A `paths` filter belongs to a workflow rather than to a job, so the two jobs become two workflows. That is the whole cost of the change: both job bodies are carried over unchanged, and only the triggers differ. The filters are written from what each job actually reads. `packages/` is entirely TypeScript so it is taken whole, and the two TypeScript apps are named because the rest of `apps/` is Rust. The Rust job takes its own directory plus the workspace root and lockfile, which pin every version it builds against, and nothing else — it depends on no other member of the workspace. The failure mode of a path filter is silence: a job that does not run leaves a green pull request. So the one thing a future change has to remember is written where it will be read — adding a TypeScript app means adding it to that list. |
||
|
Wanjohi
|
b95d939aeb |
feat(api): record which host holds a Steam token for whom (#327)
## What this is
A host that signs someone into Steam ends up holding a refresh token.
The
control plane needs to know that happened; it must not know the
credential.
This adds the table, the domain module and the three
machine-authenticated
routes that record the outcome, and a test that the surface refuses a
token.
| | |
|---|---|
| `steam_enrolment` | `(machine_id, user_id)` primary key, `steam_id`,
`state`, `enrolled_at`, `last_ok_at`, `revoked_at`. **No token column.**
|
| `Enrolment.record` | upsert to `enrolled` — keeps the original
`enrolled_at`, clears a previous refusal |
| `Enrolment.markStale` | machine-scoped update; `null` when there is
nothing to mark |
| `Enrolment.listByMachine` | what this host is believed to hold, oldest
first |
| `POST /machine/enrolment` | `{userId, steamId}` → the enrolment |
| `POST /machine/enrolment/stale` | `{userId}` → the enrolment, now
stale; `404` if absent |
| `GET /machine/enrolment` | the list |
All three take the host from its own credentials, never from a body, so
a box
can neither report onto nor read another box's hardware. `data` is the
object
itself — never `{"data": {"enrolment": …}}`.
## The test failing first
Both new files, against unmodified code:
```
✗ POST /machine/enrolment > the outcome is recorded and `data` is the enrolment itself
✗ POST /machine/enrolment > the machine is taken from the credentials, never the body
✗ POST /machine/enrolment > re-enrolling keeps the first `enrolledAt` and adopts the new Steam account
✗ POST /machine/enrolment > one Steam account on two hosts is two enrolments
✗ POST /machine/enrolment > a user nobody has heard of is refused rather than crashing
✗ POST /machine/enrolment > a Steam id has to look like one
✗ POST /machine/enrolment > machine credentials are required
✗ POST /machine/enrolment/stale > a refused token moves the enrolment to stale
✗ POST /machine/enrolment/stale > re-enrolling after a refusal returns the row to enrolled
✗ POST /machine/enrolment/stale > an enrolment this host does not have is a 404
✗ POST /machine/enrolment/stale > a host cannot mark another host’s enrolment stale
✗ POST /machine/enrolment/stale > machine credentials are required
✗ GET /machine/enrolment > a host with no enrolments gets an empty list, not a 404
✗ GET /machine/enrolment > every enrolment this host is expected to hold, and no other host’s
✗ GET /machine/enrolment > machine credentials are required
✗ The enrolment surface refuses a token > POST /machine/enrolment rejects every credential-shaped field
✗ The enrolment surface refuses a token > POST /machine/enrolment/stale rejects every credential-shaped field
✗ The enrolment surface refuses a token > the published surface has exactly three enrolment routes and no field for a credential
error: Cannot find module './enrolment.js' from 'packages/core/src/steam/enrolment.test.ts'
0 pass
19 fail
1 error
Ran 19 tests across 2 files.
```
And after, against a database migrated from zero:
```
363 pass
0 fail
1047 expect() calls
Ran 363 tests across 29 files. [8.39s]
```
`bunx oxlint` clean; `oxfmt --check` clean on every file in the diff.
`tsc
--noEmit` on `apps/api` and `packages/core` reports exactly the same 5
and 2
pre-existing errors as `dev` does — none in files this touches.
## How the token is kept out, in three places rather than one
1. **The column list.** A core test asserts `information_schema.columns`
for
the table is exactly the seven contract columns. Adding `refresh_token`,
or
an `encrypted_token`, or a `secret`, fails it.
2. **The request bodies are `.strict()`**, derived from the domain
schema with
`Info.pick(...)` so they cannot drift from it. `refreshToken`, `token`,
`accessToken`, `challengeUrl` and `clientId` are each rejected with a
`400`.
3. **The published surface.** A test walks `/doc`, collects every
request-body
property and parameter under `/machine/enrolment*`, and asserts the set
is
exactly `{userId, steamId}` — so any new field on this surface has to be
argued for in that test, not only a token-shaped one. It also asserts
the
route list is exactly the two paths.
Checked by hand against a live server: the rejected key's **name**
reaches the
log, its **value** does not (`grep -c eyJsecret` over the server log →
0).
## Driven over real HTTP, not only `app.request`
A real `bun run apps/api/app/server.ts`, a real registered machine,
curl:
```
GET /machine/enrolment → {"data":[]} [200]
POST /machine/enrolment → {"data":{…,"state":"enrolled","lastOkAt":null}} [200]
POST again, new steam account → same enrolledAt, new steamId [200]
POST + refreshToken → Unrecognized key: "refreshToken" [400]
POST + challengeUrl+clientId → Unrecognized keys: "challengeUrl", "clientId" [400]
POST /machine/enrolment/stale → {"data":{…,"state":"stale"}} [200]
GET /machine/enrolment → the one row, stale [200]
no credentials / wrong secret → Machine credentials required [403]
stale for an absent enrolment → This machine has no enrolment for that user [404]
POST naming another machine → Unrecognized key: "machineId" [400]
```
## Two places the contract was read rather than followed literally, both
worth a look
- **`state` is a Postgres enum, not `text`.** The three values are the
whole
state machine, so the database refuses a fourth rather than storing it.
If
you would rather have `text`, say so and I will change it — but a typo'd
state is otherwise a silent write.
- **The row has no `id` and no `time_deleted`**, which departs from the
every-table convention in `packages/core/CLAUDE.md`. The pair *is* the
identity, and `enrolled_at` would make `time_created` a second answer to
the
same question. The row's life is bounded by the machine's and the
user's, and
both foreign keys cascade. Flagging it because it is the sort of thing a
reviewer should agree to on purpose.
## Review rounds: three findings, all real, all fixed
- **An overlong `userId` returned a 500.** Ids live in a `char(30)`
column, so
an overlong one is refused by Postgres with `22001` — not the `23503`
the
handler catches — and fell through to the global error boundary.
Measured
before fixing: 44 characters → `500`, absent-but-well-formed → `404`.
`Identifier.schema` had no callers anywhere in the tree, so it now
asserts
the exact width and the separator as well as the prefix, and
`Enrolment.Info`
uses it for both foreign keys. The route picks up the constraint through
its
existing `Info.pick(...)`, so the answer is a `400` naming `userId`.
Verified
against a live server: zero 500s across the run.
- **`user_id` was unindexed.** The key begins with `machine_id`, so
neither the
cascade behind deleting a user nor "which hosts hold a token for me" can
use
it. `steam_enrolment_user_idx` added. The migration has not been
released, so
it is folded in and regenerated through `drizzle-kit` rather than
followed by
a corrective `0013`.
- **Every documented id was one character short.** `Examples.Id` emitted
25
payload characters where an id has 26, so the published examples were 29
characters — invalid against the width the previous fix started
enforcing.
Never broken at runtime, since an example is not parsed; wrong in the
documentation people copy from. The width now comes from
`Identifier.LENGTH` rather than being typed out, in both places that had
counted it by hand, and the third hand-written copy of the same literal
in
`apps/api/app/routes/steam.ts` now calls the generator instead.
Each is covered by a test, including one that walks four differently
misshapen
ids. The existing "a user nobody has heard of" test now uses a
well-formed
absent id, so it exercises the foreign-key path it was written for
rather than
passing for the wrong reason.
## Files outside this lane's ownership
Four, each the smallest possible diff:
- `apps/api/app/index.ts` — one import, one `.route('/machine', …)`
line.
- `packages/core/src/examples.ts` — one `Examples.SteamEnrolment` block.
- `packages/core/src/id.ts` — `Identifier.schema` gains the width and
separator
checks described above, and `LENGTH` is exported so the examples can
derive
from it. `schema` had **no callers in the tree** before this branch, so
nothing else can be affected by the tightening; this lane is its first.
- `packages/core/src/id.test.ts` — new. Pins a generated id, the schema
for
one, and the documented example together, for every prefix.
- `apps/api/app/routes/steam.ts` — one line: a hand-written `usr_XXX…`
example
literal, wrong by the same character, replaced with
`Examples.Id('user')`.
Owned by no lane this week. Flagging it because it is the only change
here
outside enrolment's own surface.
- `packages/core/CLAUDE.md` — one row in the sub-module table, and the
sentence
saying `steam/` owns no table is now false, so it is reworded.
`packages/core/src/steam/index.ts` is this lane's, and the change there
is one
word: `STEAM_ID_RE` is exported so the enrolment schema uses the same
rule
rather than a second copy of the same regex.
## What this does not verify
- **No host has ever called these routes.** The other half of this seam
was
written from the same document without either side reading the other's
code.
A disagreement, if there is one, surfaces on first contact — not here.
- **Nothing enforces that a token never arrives.** The three guards
above fail
when somebody adds a field *to this surface*. They say nothing about a
route
added elsewhere, and no test can.
- **`revoked` has no writer.** The value exists in the enum and nothing
sets
it. Revocation is not built here.
- **`last_ok_at` has no writer**, so it is null in every row this
creates. It
has never been exercised with a value.
- **Authorisation is not tested here and is not this lane's.** Whether a
person
may reach a given box is decided before a request arrives; these routes
authenticate a *machine*, which is a different question. Nothing here
would
catch a mistake in the other one.
- **The `404` for an unknown user comes from catching a foreign-key
violation**
(`23503`), not from a lookup. It is exercised for a user id that does
not
exist. It has not been exercised against a user deleted concurrently
with the
insert, which is the same code path but a race I did not reproduce.
- **The index is not measured.** It is added because two readers exist
that
cannot use the primary key, not because a plan was compared. On a table
this
size neither would be slow yet.
- **Nothing else is measured.** No timing, no throughput, no load. The
only
numbers above are test counts and HTTP status codes.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR adds machine-authenticated Steam enrolment reporting without
transmitting or storing Steam credentials.
- Adds record, stale-state, and machine-scoped listing operations in the
core domain.
- Adds corresponding `/machine/enrolment` API routes with strict request
validation.
- Adds the enrolment table, state enum, foreign keys, user index, and
migration metadata.
- Aligns identifier validation and OpenAPI examples with the fixed
30-character identifier format.
- Adds domain, API, schema, authorization, and credential-exclusion
tests.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge; the prior identifier-example issue is
fixed and no blocking correctness, security, or repository-rule
violations remain.
The current code fully addresses all previous findings: malformed
identifiers are rejected before database access, the user foreign key
has its own index, and shared examples now satisfy the identifier
schema. The changes since the previous review introduce no new
actionable failures.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| apps/api/app/routes/enrolment.ts | Adds three strictly validated,
machine-authenticated enrolment routes scoped through the authenticated
machine actor. |
| packages/core/src/steam/enrolment.ts | Adds validated enrolment
recording, stale-state updates, machine-scoped listing, and
serialization. |
| packages/core/src/steam/enrolment.sql.ts | Defines credential-free
enrolment persistence with a composite key, cascading foreign keys, and
a user lookup index. |
| packages/core/migrations/0012_steam_enrolment_without_a_token.sql |
Creates the enrolment enum, table, constraints, and user index
consistently with the Drizzle model. |
| packages/core/src/id.ts | Tightens identifier validation to the exact
prefixed 30-character storage format. |
| packages/core/src/examples.ts | Corrects shared identifier examples to
produce schema-valid 30-character values. |
| apps/api/test/enrolment.test.ts | Covers route shape, authentication,
machine isolation, validation, lifecycle behavior, and rejection of
credential fields. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant H as Authenticated host
participant A as API
participant C as Core Enrolment domain
participant D as PostgreSQL
H->>A: "POST /machine/enrolment {userId, steamId}"
A->>A: Derive machineId from actor credentials
A->>C: record(machineId, userId, steamId)
C->>D: Upsert enrolment metadata
D-->>C: Enrolment row
C-->>A: Serialized enrolment
A-->>H: "{data: enrolment}"
H->>A: "POST /machine/enrolment/stale {userId}"
A->>C: markStale(authenticated machineId, userId)
C->>D: Machine-and-user-scoped update
D-->>H: Updated enrolment or 404
H->>A: GET /machine/enrolment
A->>C: listByMachine(authenticated machineId)
C->>D: Select this machine's rows
D-->>H: "{data: enrolments[]}"
```
<sub>Reviews (3): Last reviewed commit: ["fix(core): document an id that
is
actual..."](https://github.com/nestrilabs/nestri/commit/fe5297acbdbc9e2ce63c87f1e720b1ade426a920)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=60932045)</sub>
<details><summary><h4>Context used (3)</h4></summary>
- Knowledge Base — [API HTTP composition and
authorization](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/api-http-and-auth.md)
- Knowledge Base — [API catalog, machine, and account
endpoints](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/api-catalog-and-machine-endpoints.md)
- Knowledge Base — [Core domain and
persistence](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-domain-data.md)
</details>
<!-- /greptile_comment -->
|
||
|
Wanjohi
|
fe5297acbd |
fix(core): document an id that is actually a valid id
The example generator emitted twenty-five payload characters where an id has twenty-six, so every documented id was twenty-nine characters — one short of the width the column holds and, since last commit, one short of what the schema publishing it will accept. Nothing caught it because an example is never parsed: it is copied into documentation and read by people. The width now comes from the generator's own constant instead of being typed out, in the two places that had counted it by hand. Counting twenty-six of anything by eye is a thing people get right once and never re-check. A test pins the three together — a generated id, the schema for one, and the documented example must all agree, for every prefix. It fails on the off-by-one that prompted this, and on a prefix without its separator, which would otherwise read as an id of that type because it starts with the same three letters. |
||
|
Wanjohi
|
3d24a8e130 |
feat(nesinit): carry the session's address out of the guest
Whatever serves media in a box knows how it can be reached, and the person who needs to know is not in the box. Standard output here is a log file inside a VM, so the control channel is the delivery path rather than a convenience -- which makes this init's job and not a detail of whichever component happens to bind the port. The socket it reads was already documented as being read this way; nothing read it. Polled rather than read once, because an address is not a value but the best answer so far. An endpoint discovers more ways to reach it after it binds, so the first answer is the one that works on a local network and fails from anywhere else. Only a changed answer is forwarded. It dials rather than listens, which is the opposite of the relay next door and deliberate: there the guest listens because the workload starts later, and here the server is the long-lived one. Dialling also makes a server that has not bound yet something to retry rather than something to wait for without knowing whether it is coming. The address itself is never logged. It is a capability to reach the session, and a log inside the guest is the one place it has no reason to be. A carrier that stops does not end a session: whatever was already reported is still correct, and the workload's exit still has to be. |
||
|
Wanjohi
|
64a90abf75 |
fix(api): a misshapen id is bad input, not a server fault
Ids are stored in a fixed-width column, so an overlong one is refused by Postgres rather than simply matching nothing. That refusal is not a foreign-key violation, so it fell through to the global error boundary and reached the caller as a 500 — telling a host to retry something that can never succeed. Measured: a 44-character user id returned 500, where an absent but well-formed one correctly returned 404. `Identifier.schema` is the natural place for the check and had no callers yet, so it now asserts the exact width an id has as well as its prefix — including the separator, without which `usrsomething` reads as a user id. The enrolment schema uses it for both foreign keys, so the refusal happens where the input arrives and names the field. Also index `steam_enrolment.user_id`. The primary key begins with the machine, which answers what one host holds and nothing else, so neither of the two things that read by user alone can use it: the cascade behind deleting a user, and asking which hosts hold a token for one person. The table's migration has not been released, so this is folded into it rather than following it with a correction. |
||
|
Wanjohi
|
6429ec4ff7 |
feat(api): record which host holds a Steam token for whom
A host that signs a person into Steam ends up holding a refresh token. The control plane needs to know that happened — to show it, and so a host that lost its disk can find out what it is expected to hold — but it must not know the credential, because the token is bound to the address that obtained it and a copy anywhere else is the account-theft signal Steam watches for. So `steam_enrolment` stores the outcome and has no token column, no encrypted token column, and no column that could hold one later. The safeguard is that the credential is never sent here at all; a nullable column would be the first step in undoing it, so a test asserts the column list exactly and fails if one appears. Three machine-authenticated routes go with it: report a completed sign-in, report that Steam refused the token, and list what this host should have. All three take the host from its own credentials, so a box can neither report onto nor read another box's hardware. Their bodies are strict, so a host that sends a token is told it is wrong rather than quietly believed — which also keeps the value out of the request log. The Steam id is deliberately not unique. One account signed in on two hosts is two rows and two tokens, and a unique index there would look like hygiene while refusing somebody their second box. There is no `pending` state: a sign-in challenge lives about two minutes inside one process, and nothing outside it needs to know it exists. Nothing revokes yet, and `last_ok_at` has no writer — a successful logon happens where there is no credential to report it with — so the column exists with the shape it will need and stays null rather than being filled with the nearest event that was easy to observe. |
||
|
Wanjohi
|
7f7e39de60 |
docs: point the test database setting at the right database
Same correction as the one on the helper itself: "isolated" here means isolated from anything you care about, not isolated from DATABASE_URL. Filling this in with a second database name is a plausible reading that fails the suite in a way that does not point back here. |
||
|
Wanjohi
|
dae2990cbe |
docs(core): say which database the tests actually need
The helper told you to use "an isolated database for tests", which reads as a database of its own and is not what the suite wants. Route tests reach the database through the app and core tests reach it directly, so two different values put the fixtures in one database and the assertions in the other — around forty failures, none of them in the code that caused it, and nothing in the output naming the setting. Also drops a type import nothing uses. |
||
|
Wanjohi
|
b296918ab4 |
feat(api): record what a host says it is running
A host agent already sends a full inventory snapshot on a cadence, and nothing served the endpoint it sends it to — so every one of those calls answered 404. It fails quietly by design, because a dropped snapshot is meant to be corrected by the next one, which is exactly why nobody noticed: the only symptom is a line in the agent's own log. Kept separate from the heartbeat because the two have different loss tolerance. A dropped beat moves a host towards offline and unplaces it; a dropped snapshot costs nothing until the next one arrives. Folding them together would let a malformed inventory field make a healthy host look dead. Three rules decide what a snapshot may do, and the last two are why this is one core function rather than a loop in the route: - a box we know, that the snapshot names, takes the reported state - a box we know that was running, and that the snapshot omits, is stopped and says so — absence inside a snapshot is information - a box the snapshot names that is not placed on the calling host is never created, only reported back as a divergence The scope is in the `where` clause and not in the agent asking politely about its own boxes: a machine credential is a long-lived secret sitting on hardware in somebody's living room. `pid` and `uptimeS` are accepted and deliberately dropped. A pid is a number in another machine's namespace, and uptime is derivable from a run's start time, which is already stored and already trustworthy. |
||
|
Wanjohi
|
b6aae5c2ab |
fix(deploy): make bun dev actually start, and sign-in actually work (#326)
Follow-up to #325. Six defects, all found by running the thing rather than reading it — #325 was verified by bundling, by tests, and by the container images, and none of those start a Worker. | | | |---|---| | `bun dev` never started | one multi-config process does not connect a service binding between the workers it loads — the API reported `AUTH [not connected]`. Two processes now, which is what the dev registry connects | | Neither server could bind | wrangler resolves `localhost` and takes `::1` first; a host with no IPv6 on its loopback dies with a bind error naming neither app nor port. `dev.ip` pinned, and `inspector_port` made distinct — it is not derived from the port, so the second server died on an address already in use | | The API worker failed to evaluate | a specifier ending in `.sql` is claimed by the bundler as its own module, so the schema file was emitted verbatim beside the bundle and the runtime threw on a missing export | | Sign-in failed on the second DB request | a pooled socket created while handling one request may not be touched while handling another on a Worker. The *first* request always succeeded, which is why nobody saw it | | The images named a base podman will not resolve | a short name needs a registry; the database service alongside them already spelled one | | Compose pinned container names | not scoped to the project, so `down` in one checkout stops another's containers — it stopped a running development database while this was being tested | Two of these are worth a second look because they are not confined to local development. **The database pool one is a live bug on Workers**, and it predates #325 — it arrived with the pool cache in `6c1d407`. On a Worker an I/O object created during one request cannot be used during another, so the cached socket throws *"Cannot perform I/O on behalf of a different request"* on the second request that reuses it. The cache is now kept only where a process outlives its requests, which is the case it was added for; a Worker goes back to a pool per invocation, which is what it did before. **The `.sql` import was also a layering break.** A route was reaching past the domain module into the schema to spell a download status. It asks the domain module now, which is the rule everywhere else here and happens to be what removes the bundler hazard. ## Verified A real sign-in, end to end, against both dev servers: a code requested over HTTP, read out of the issuer's log the way a person reads it out of their mail, redeemed, exchanged for tokens, and presented to the API — which resolved it to the account the sign-in had just created, and refused the same request without it. ``` 1. asked for a code -> 200 2. code from the log -> 811548 3. redeemed the code -> 302 4. exchanged for tokens-> access eyJhbGciOiJFUzI1NiIsImtp… 5. GET /user with it -> 200 {"data":{"id":"usr_071b56380001F9L3Wf8OQj8ogS", …}} 6. GET /user without -> 401 ``` Also: 316 tests pass, both images build, and compose substitution resolves with the required-variable guards refusing correctly when `.env` is incomplete. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved game download status validation for more consistent API behavior. - Improved database connection handling in Cloudflare Workers by using request-local connections. - **Developer Experience** - Added explicit local development and debugger ports for the API and authentication services. - Improved development process handling when running services together. - **Chores** - Updated container configuration to support multiple project checkouts without naming conflicts. - Standardized container image references for more reliable builds. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR repairs local development startup and sign-in behavior while making containerized development safer across environments. - Runs the authentication and API Workers as separate, jointly supervised development processes. - Assigns explicit IPv4 listener and distinct inspector ports. - Prevents Cloudflare Workers from reusing database I/O objects across requests. - Exposes download statuses through the domain module rather than importing a schema module from the route. - Corrects the authentication app’s Hono JSX transform configuration. - Uses fully qualified Bun image names and Compose-managed container names. <h3>Confidence Score: 5/5</h3> The PR appears safe to merge, with the previous process-supervision issue resolved and no new actionable regressions identified. The current development scripts stop the sibling server when either process exits, and the changes since the previous review preserve server entrypoint behavior while correcting the Hono JSX runtime selection. The previous thread was manually resolved after the supervision fix. <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | package.json | Starts and supervises the two development servers independently; the follow-up direct entrypoint invocation preserves their intended behavior. | | apps/auth/tsconfig.json | Aligns authentication-server JSX transformation with Hono and the repository’s existing TypeScript configuration. | | packages/core/src/db/index.ts | Limits database pool caching to long-lived process environments so Worker requests do not reuse request-bound I/O. | | packages/core/src/game/download.ts | Exposes valid download statuses through the domain namespace for API consumers. | | apps/api/app/routes/game.ts | Uses the domain-level download status export, avoiding a direct runtime import of the SQL schema module. | | docker-compose.yml | Removes globally fixed container names so Compose projects remain isolated between checkouts. | <h3>Flowchart</h3> ```mermaid %%{init: {'theme': 'neutral'}}%% flowchart LR Dev["bun dev"] --> Auth["Auth Worker<br/>127.0.0.1:1337<br/>Inspector 9229"] Dev --> API["API Worker<br/>127.0.0.1:3000<br/>Inspector 9230"] API -->|AUTH service binding| Auth Auth --> DB["Request-local DB pool<br/>on Cloudflare Workers"] API --> DB Supervisor["Process supervisor"] --> Dev Auth -->|Either process exits| Supervisor API -->|Either process exits| Supervisor Supervisor -->|Stops sibling process| Dev ``` <sub>Reviews (3): Last reviewed commit: ["fix(deploy): make \`bun run dev:server\` s..."](https://github.com/nestrilabs/nestri/commit/46c3a67d4dd24544288b4bfeed5c6685dd4e60e8) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=60753832)</sub> <!-- /greptile_comment --> |
||
|
Wanjohi
|
46c3a67d4d |
fix(deploy): make bun run dev:server start and reach its settings
Two defects, both found by running it. Neither affects the container path, which is why the images passed: they run the same entrypoints from the repository root, and that turns out to be the load-bearing detail. **The issuer would not start at all.** `apps/auth/tsconfig.json` named React as the JSX runtime — left over from an earlier scaffold; there is no React anywhere in this repository. It only bit when something transpiled from that directory, and then the process died resolving `react/jsx-dev-runtime` from a sign-in screen in `packages/auth` before it bound a port. The package holding those components already said `hono/jsx`, and so did the root; this is the third place agreeing with them. **Neither process could see `.env`.** They ran with the working directory set to their own app, so the environment file the repository documents — the one at the root — was not the one they were offered. The issuer then correctly refused to send a sign-in code rather than logging one, which is the right behaviour and an opaque way to discover a path problem. Both now run from the root, which is also exactly what the images run. |
||
|
Wanjohi
|
b9b090a620 |
fix(deploy): stop bun dev when either half dies, not just the API
The issuer ran in the background and nothing watched it. If it failed to bind, or exited an hour later, the API kept serving and kept reporting itself ready — while every authenticated request failed, because there was no issuer to verify a token against. That is the worst shape for a development failure: the thing you are looking at looks fine. Both run in the background now, and the script waits for either to stop before killing the other. The previous form only handled the direction where the API was the one that exited. |
||
|
Wanjohi
|
9258c8dfef |
fix(deploy): make bun dev actually start, and sign-in actually work
Six defects found by running the thing rather than reading it. The previous change was verified by bundling, by tests, and by the container images — none of which start a Worker, so every one of these was invisible. **`bun dev` did not start.** It ran one multi-config process, which does not connect a service binding between the workers it loads; the API reported `AUTH [not connected]` and could not verify a token. It is two processes now, which is what the dev registry connects, and the second is backgrounded with the first killed on exit so stopping the pair stops both. **Neither server could bind.** Wrangler resolves `localhost` and takes `::1` first; a host with no IPv6 address on its loopback dies with a bind error from inside the runtime that names neither the app nor the port. `dev.ip` is pinned to `127.0.0.1`, and `inspector_port` is now distinct per app — it is not derived from the port above, so the second server to start died on an address already in use. **The API worker failed to evaluate.** A specifier ending in `.sql` is claimed by the bundler as a module of its own, so the schema file was emitted verbatim beside the bundle and the runtime threw on an export it could not find. The route was reaching past the domain module into the schema to spell a status; it now asks the domain module, which is the rule everywhere else here and happens to also avoid the hazard. **Signing in failed on the second request that touched the database.** A pool is cached per connection string, and on a Worker an I/O object created while handling one request may not be touched while handling another. The first request always succeeded, which is why it went unnoticed — a sign-in is several. The cache is now kept only where a process outlives its requests, which is the case it was added for. **The images named a base that podman will not resolve.** A short name needs a registry; the database service alongside them already spelled one. **Compose pinned container names.** The name is not scoped to the project, so a second checkout got the same three, and `down` in one stopped the other's containers. This is not hypothetical — it stopped a running development database while this was being tested. Verified by signing in end to end against both dev servers: a code requested over HTTP, read from the issuer's log, redeemed, exchanged for tokens, and presented to the API, which resolved it to the account the sign-in had just created. |
||
|
Wanjohi
|
8c80c025be |
feat(deploy): drop the IaC layer, and make both apps runnable as containers (#325)
Moving the issuer's state into Postgres (#324) removed the last thing tying either app to one hosting provider. What was left was a deployment tool describing resources that no longer existed — so this drops it in favour of `wrangler`, which is what actually deploys a Worker, and adds a second way to run each app that involves no provider at all. ## What changes **Gone:** `alchemy.run.ts`, `docs/alchemy.md`, the `alchemy` and `effect` root dependencies, and the two type imports that reached out of `apps/api` into the infrastructure file. **In its place**, per app: | | | |---|---| | `wrangler.jsonc` | one environment per stage, custom-domain routes, Hyperdrive, the `AUTH` service binding | | `Dockerfile` + `server.ts` | the same handler behind a listening socket | The handler is the same one either way. What differs is only where its settings come from, and two of them gained a second spelling so that nothing has to branch on the runtime: Postgres arrives as a pooled binding or as `DATABASE_URL`, and the route to the issuer is a service binding or `AUTH_INTERNAL_URL`. `docker-compose.yml` now brings up Postgres and both apps together, which is both what a self-hoster runs and the shape this takes when it stops being a set of Workers. ## `AUTH_INTERNAL_URL`, which is new A service binding was quietly doing two jobs: routing to the issuer, and letting the `iss` claim stay the issuer's public name. Nothing else can do both with one setting — the public name is often not routable from inside a deployment — so the name and the route are two settings now. `AUTH_ISSUER_URL` is still compared literally against every token, and is unchanged. ## DNS Moves out of code and into [`docs/dns.md`](docs/dns.md): every hostname, what it is for, and what answers it today. Six records that change roughly never did not need a tool, and a table outlives whatever is serving the names — which is the point, because some of them will stop being Workers. `wrangler` keeps owning only the part that must stay in step with a deploy, since a route and its hostname are one fact. The one rule the table enforces is **one label deep on `nestri.io`**. A certificate for `*.nestri.io` covers one level and not two, so the sandbox names are hyphenated rather than nested — `api-sandbox.nestri.io` can become an ordinary proxied origin later without a certificate having to be ordered for it first. Production hostnames are unchanged. ## Also `EMAIL_DEV_LOG` moves from committed configuration into `apps/auth/.dev.vars`, which `wrangler deploy` cannot upload. Printing a live sign-in code to a log should not be one forgotten override away from a stage somebody else can reach. ## Before this deploys 1. The Hyperdrive ids in both `wrangler.jsonc` files are placeholders. `wrangler hyperdrive list` has the real ones. Local development works without them. 2. The Worker names change, so the first deploy creates new Workers. Confirm the custom domains answer, *then* remove the old Workers and routes — that order, or the names resolve to nothing in between. 3. Set the secrets listed in [`docs/deploy.md`](docs/deploy.md). The issuer refuses to sign anyone in without its three mail settings. ## Checks - 316 tests pass, 0 fail, against a freshly migrated database. - All four wrangler environments bundle with no warnings. - Both images build, run, and report `healthy`; the API container reaches the issuer container and rejects a bad token as 401 rather than 500. - `AUTH_INTERNAL_URL` verified end to end: discovery and JWKS resolve through the internal route while `iss` stays the public name. - `oxlint` clean apart from one pre-existing unused import in `packages/auth`. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR replaces the Alchemy deployment layer with direct Wrangler configuration and container-based execution for both control-plane applications. - Adds Bun HTTP servers, production Dockerfiles, health checks, and a Compose deployment for Postgres, auth, and API. - Supports database and issuer routing through either Cloudflare bindings or ordinary environment variables. - Adds stage-specific Worker routes, service bindings, Hyperdrive configuration, and deployment documentation. - Moves DNS ownership and deployment guidance into dedicated documentation. - Removes unused Alchemy, Effect, and Steam API-key configuration. <h3>Confidence Score: 5/5</h3> The current changes appear safe to merge, with no established new defects or outstanding previous findings. The container and Worker configurations are internally consistent, required privileged credentials no longer have Compose defaults, and plaintext service ports are loopback-bound. The three previous threads were manually resolved without explanation and therefore are not outstanding. <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | docker-compose.yml | Defines the self-hosted stack with required credentials, loopback-bound ports, mail pass-through, health dependencies, and internal issuer routing. | | apps/api/app/middleware/auth.ts | Adds issuer access through either a Worker service binding or AUTH_INTERNAL_URL while preserving the public issuer used for token validation. | | apps/api/app/server.ts | Exposes the existing API handler through Bun’s HTTP server with a process-compatible execution context. | | apps/auth/src/server.ts | Exposes the existing authentication handler through Bun’s HTTP server. | | apps/api/wrangler.jsonc | Configures API development, sandbox, and production Workers with routes, Hyperdrive, issuer settings, and auth service bindings. | | apps/auth/wrangler.jsonc | Configures auth development, sandbox, and production Workers with custom domains and Hyperdrive. | | packages/core/src/env.ts | Supports database and issuer routing through environment variables and removes an unused Steam API-key setting. | <h3>Flowchart</h3> ```mermaid %%{init: {'theme': 'neutral'}}%% flowchart LR Client[Clients] --> Proxy[Custom domain or TLS proxy] Proxy --> API[API handler] Proxy --> Auth[Auth issuer handler] API -->|AUTH service binding| Auth API -->|AUTH_INTERNAL_URL in containers| Auth API --> DB[(Postgres)] Auth --> DB Wrangler[Cloudflare Wrangler runtime] --> API Wrangler --> Auth Compose[Docker Compose runtime] --> API Compose --> Auth Compose --> DB ``` <sub>Reviews (2): Last reviewed commit: ["fix(deploy): require every credential, a..."](https://github.com/nestrilabs/nestri/commit/f30a1432f84d9a4301d3cda4d628d69a1758e5f5) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=60746897)</sub> <!-- /greptile_comment --> |
||
|
Wanjohi
|
f30a1432f8 |
fix(deploy): require every credential, and give sandbox its own domain
Three things review caught, and one shape correction. **No credential has a default any more.** The compose file shipped `ADMIN_SHARED_SECRET` falling back to a value written in this repository — and that header bypasses token verification entirely, so anyone reading the file could act as an operator against any deployment that had not overridden it. A default is worth less than it looks here: the deployment that never set the variable is exactly the one where the default is public. Every credential now comes from `.env`, and compose refuses to start naming the variable it wanted. That also takes the last literal password out of a tracked file. **The origin ports are on loopback.** Both services speak plain HTTP and mark no cookie `Secure`, because both expect to sit behind something that terminates TLS. Published on every interface they were a way to reach the issuer around that proxy, with sign-in codes and tokens in clear text. **Mail settings are passed through rather than fixed.** The issuer was pinned to printing sign-in codes to its log, and the three delivery settings never reached it — so the documented way to configure mail could not work, and every code and recipient went to the container log instead. Printing codes is now asked for in `.env` like everything else, and with nothing configured the issuer refuses to send rather than logging. **Sandbox becomes a domain rather than a prefix.** `api.sandbox.nestri.io` and `auth.sandbox.nestri.io`, because sandbox holds whatever is not production and that set grows. One certificate for `*.sandbox.nestri.io` then covers all of it, including unpredictable per-pull-request names, and cannot be presented for production's own domain — which the zone-wide wildcard the previous shape leaned on could. Also drops `STEAM_API_KEY`. It was declared in two type definitions and read by nothing: linking an account makes no outbound call that needs it. |
||
|
Wanjohi
|
51ababc900 |
feat(deploy): drop the IaC layer, and make both apps runnable as containers
Moving the issuer's state into Postgres removed the last thing that tied either app to one hosting provider. What was left was a deployment tool describing resources that no longer existed — so this replaces it with `wrangler`, which is what actually deploys a Worker, and adds a second way to run each app that involves no provider at all. Each app now has a `wrangler.jsonc` with an environment per stage, and a `Dockerfile` beside it. The handler is the same one in both cases; what differs is only where its settings come from. Two of them gained a second spelling so that nothing has to branch on the runtime: Postgres arrives as a pooled binding or as `DATABASE_URL`, and the route to the issuer is a service binding or `AUTH_INTERNAL_URL`. That last one is new, and it is a split the binding was already making without saying so. `AUTH_ISSUER_URL` has to be the issuer's public name, because it is compared literally against every token's `iss` claim — but the public name is often not routable from inside a deployment. So the name and the route are two settings now rather than one that cannot be both. DNS moves out of code and into `docs/dns.md`, which lists every hostname and what it is for. Six records that change roughly never did not need a tool, and the table outlives whatever is answering the names — which is the point, since some of them will stop being Workers. The sandbox hostnames are hyphenated rather than nested for the same reason: a certificate covering `*.nestri.io` covers one label and not two, so `api-sandbox.nestri.io` can become an ordinary origin later without a certificate having to be ordered for it first. Also drops `EMAIL_DEV_LOG` from committed configuration into `.dev.vars`, which `wrangler deploy` cannot upload. Printing a live sign-in code to a log should not be one forgotten override away from production. |
||
|
Wanjohi
|
3d0dcf3e46 |
feat(auth): move issuer state to Postgres (#324)
Moves the issuer's state out of Cloudflare KV and into Postgres, and splits it by whether a record actually needs the guarantees a key-value store cannot give. ## Why The issuer kept everything behind one `get`/`set`/`remove`/`scan` interface — which is what a library that has to run on any provider's cache can offer. Three of the things kept there could not be served by it: - **Authorization codes** must be redeemable once. `get` → validate → `remove` are separate steps, so two exchanges of one code arriving together both pass every check, and each answer is a complete session. - **Refresh tokens** must be spendable once. Reuse detection works by recording *when* a token was first spent, so the check and the record have to be the same operation. Split apart, two refreshes both look like the first — and the reuse that reveals a stolen token is never recorded, because recording it is the write the second caller overwrites. - **Signing keys** don't race, but they're the one record whose loss ends every session at once, and a cache is a place things may be evicted from. This is the same argument `device_grant` already made, applied to the records that had it too. ## What's here | table | why it exists | |---|---| | `authorization_code` | redeeming is one `delete … returning` | | `refresh_token` | spending is one `update … where time_used is null returning *`; `subject` indexed, so signing out everywhere is a query rather than a prefix scan | | `auth_key` | retired by setting `expired_at`, never deleted, so tokens stay verifiable through a rotation | | `auth_kv` | everything left: the rate-limit counters | `auth_kv` stays deliberately generic. Those counters are written far more often than read, meaningless within the hour, and allowed to be approximate — a lost increment costs one guess out of ten. It's the one place an unmigrated `jsonb` blob is the right answer rather than a shortcut. Both credential tables store a **hash and never the credential**, as `device_grant` does. An authorization code travels in a query string, so it passes through browser history, referrer headers and any log along the redirect; a refresh token resumes a session outright. `packages/auth` gains `keyStore`, `codeStore` and `refreshStore` as optional issuer inputs, each defaulting to a storage-backed shim so the library behaves exactly as before when they aren't passed — same shape as the existing `deviceStore`. ## Portability `AuthStorage` was the only stateful Cloudflare-proprietary primitive in the control plane. It's gone, so the remaining CF surface is Hyperdrive (a pooler over a plain `DATABASE_URL`), Workers and DNS. The self-host story stays one service. ## Two behaviour changes worth reviewing 1. **A code that fails a check is now spent.** `consume` happens before the redirect-URI, client and PKCE checks, because the operation that decides which caller gets the code has to be the one that removes it. RFC 6749 §4.1.2 asks for this, but it is a change: a code presented wrongly no longer gets a second try. 2. **`legacySigningKeys` is removed.** It read a pre-ES256 `oauth:key` prefix and stamped every key it returned with a hardcoded expiry of 2025-01-02 — so everything it produced has been expired for over a year, and it only ever read from the store being left behind. ## ⚠️ Deploying this signs everyone out The signing keys and refresh tokens live in the KV namespace this removes. The issuer will start with a fresh key set, so every existing access token stops verifying and every refresh token is gone. Worth timing deliberately, or copying the key rows across first if that isn't acceptable. ## Testing - `packages/auth` — 66 pass (the issuer flows run through the storage-backed shims, so the rewrite is behaviour-preserving on the default path) - `packages/core` — 130 pass, including 24 new ones The concurrency claims are tested rather than asserted: five overlapping `claim`s of one refresh token yield exactly one `fresh` and four `reused`; five overlapping `consume`s of one code yield exactly one non-null. Also covered: `LIKE` wildcard escaping in `scan` (keys are built from email addresses and caller addresses, so `%` and `_` are not hypothetical), and that `scan(['a'])` no longer reaches into `['ab']`. Migration `0011` applies cleanly from empty. ## Not in scope Pre-existing `tsc` errors — JSX config for `packages/auth/src/ui/*.tsx`, `subject.ts`, `oauth2.ts`, `session.test.ts` — are untouched and left for their own PR. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR moves issuer state from Cloudflare KV into dedicated PostgreSQL stores and adds atomic operations for authorization-code redemption, refresh-token claims, and signing-key bootstrap. - Authorization codes are hashed and consumed with `DELETE ... RETURNING`. - Refresh tokens are hashed and claimed with a conditional atomic update. - Signing keys are persisted in PostgreSQL with one live key permitted per kind. - Remaining approximate rate-limit state uses a generic PostgreSQL-backed adapter. - The changes since the previous review preserve each stored key’s algorithm and make concurrent key bootstrap converge on one live key. <h3>Confidence Score: 5/5</h3> The PR appears safe to merge; both findings from the previous review are resolved and no new actionable failure remains. Concurrent key bootstrap now converges through a partial unique index and a post-insert reread, while imported keys retain their stored algorithms. Both previous threads were manually resolved, and the current code confirms their underlying issues are fixed. <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | packages/auth/src/issuer.ts | Routes authorization codes, refresh tokens, signing keys, and invalidation through specialized stores. | | packages/auth/src/keys.ts | Preserves stored key algorithms and rereads the store after bootstrap so concurrent issuers converge. | | packages/core/src/auth/signing-key.ts | Persists key material and safely ignores a concurrent insertion that already established the live key. | | packages/core/src/auth/authorization-code.ts | Implements atomic, single-use authorization-code consumption. | | packages/core/src/auth/refresh-token.ts | Implements atomic refresh-token claims and subject-wide revocation. | | packages/core/migrations/0011_auth_state_in_postgres.sql | Adds PostgreSQL tables and constraints for issuer state, including one live key per kind. | <h3>Flowchart</h3> ```mermaid %%{init: {'theme': 'neutral'}}%% flowchart LR Issuer[OAuth issuer] --> Codes[Authorization code store] Issuer --> Refresh[Refresh token store] Issuer --> Keys[Signing key store] Issuer --> KV[Generic auth state] Codes -->|DELETE RETURNING| PG[(PostgreSQL)] Refresh -->|Conditional claim| PG Keys -->|Partial unique live-kind index| PG KV -->|Counters and rate limits| PG ``` <sub>Reviews (2): Last reviewed commit: ["fix(auth): keep one live key per kind, a..."](https://github.com/nestrilabs/nestri/commit/f64f037574c87acf40f0acf061297297169c6fb0) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=60727758)</sub> <!-- /greptile_comment --> |
||
|
Wanjohi
|
f64f037574 |
fix(auth): keep one live key per kind, and report a key's own algorithm
Two problems found in review, both in the key store. Nothing stopped a kind from having two live keys, and the bootstrap path walks straight into it: two workers starting against an empty table both find no key and both insert one. From then on each signs and encrypts with its own. That is not the harmless split the comment here claimed — the issuer reaches for a single key rather than the published set when it decrypts a session cookie and when it verifies an access token, so a cookie written by one worker is unreadable to the other and a token minted by one is rejected by the other. It stays silent until someone cannot sign in. A partial unique index over the kind, where the key has not been retired, makes the second insert a dropped write instead. Both workers then read the table again and use the key that won, which is all that matters. The conflict clause stops naming a target: both indexes on the table mean the same thing at this call site, that the row already exists in some form. Creating a key is now attempted once rather than retried, because a store declining the write is an expected answer and spinning on it would hang the request instead of failing it. Separately, a key pair reported the algorithm the issuer currently uses rather than the one stored on the key it was built from, so a retained key would advertise the wrong algorithm in a token header and in the JWKS after a rotation — which defeats keeping it. The material was already being imported with the stored value; only what was handed back disagreed. Retiring a key and creating its replacement now have to happen together, so that a kind never has two live keys and never has none. |
||
|
Wanjohi
|
f25c9af545 |
feat(auth): keep issuer state in Postgres
The issuer kept everything behind one get/set/remove/scan interface, which is what a library that must run on any provider's cache can offer. Three of the things kept there could not actually be served by it. An authorization code must be redeemable once and a refresh token spendable once, and through get and set the check and the write are separate steps — so two requests arriving together both read an unspent record, and both mint a session. In the refresh case that also means the reuse which reveals a stolen token is never recorded, because recording it is the write that the second caller overwrites. Each now has a table and an interface of its own: redeeming is one `delete ... returning`, spending is one `update ... where time_used is null returning *`, so exactly one caller is ever told it went first. This is the same argument the device grant already made, applied to the two records that had it too. Signing keys move for a different reason. Nothing races for them; they are the one record whose loss ends every session at once, and a cache is a place things may be evicted from. They are retired by setting a column rather than deleted, so the tokens they signed stay verifiable until they expire. Both credential tables store a hash and never the credential, as the device grant does. An authorization code travels in a query string and so passes through history, referrer headers and any log along the redirect; a refresh token resumes a session outright. What is left in the generic store is the rate-limit counters — written far more often than read, meaningless within the hour, and allowed to be approximate, since a lost increment costs one guess out of ten. Those move to Postgres too, so the only key-value binding this deploys with is gone and the control plane's state is one database. That was the point: nothing here now depends on a primitive a self-hoster cannot run. The generic scan also gained the separator on its prefix, so scanning `a` cannot return what is under `ab` — subjects and email addresses are both prefixes of longer subjects and email addresses. Deploying this signs everyone out. The signing keys and refresh tokens are in a store that is being left behind, so the issuer starts with a fresh key set and every existing token stops verifying. |
||
|
Wanjohi
|
349305d0cc |
feat(api): hold a run to the attempt that claimed it (#323)
Closes the seam week 2 merged without. The agent sends a claim token on
every
write and this side rejected the field, so **every state report and
every ticket
publish answered 400** — neither end's tests could see it, because each
was
written against the document rather than against the other end.
## What lands
**Both agent bodies take `claimToken`.** That alone is what unblocks the
wire.
**The row remembers which attempt holds it.** Taking a claim requires
there to be
no holder; every write after it requires the caller to *be* the holder.
The guard
is in the `where` clause and not only in the read above it, so two
attempts that
both read an unheld row still leave with one winner.
**A report from an attempt that does not hold the run is 409 whatever
the state
is** — including the state the run is already in. That row is the whole
point: an
agent retrying after a lost response holds the token and is told nothing
broke;
an agent that lost the race does not and is told to stop. Both answers
are
decided by the request rather than by when it arrived.
**The ticket is held to the claim too**, for a worse reason than a
double start.
The client re-reads the address rather than caching it, so a ticket
published by
a losing attempt produces a client that connects, successfully, to a
machine
running nothing. A box started twice is waste and it is visible.
**The holder is never cleared**, including on terminal states, so a
settled claim
cannot be replayed and a finished run still records which attempt ran
it. It is
**not** in what goes out — holding one permits writing to a run, and the
owner
reading their own session is not the holder. There is a test asserting
the whole
response shape, so a column added later has to be added there before it
ships.
## One thing the contract asked for that cannot exist
The spec distinguishes *"claim, row already has a holder → 409"* from
*"report,
token is not the holder → 409"*. **Those are the same case.** Taking the
claim
and leaving `requested` are one write, so a rival never observes a
`requested`
row with a holder — it observes a `starting` row it does not hold. The
answer is
409 either way and nothing is lost, but the branch is unreachable and I
have not
written code pretending otherwise. Worth folding into the document.
## Failing first
The new tests against unmodified source:
```
Expected: 200 / Received: 400 the claim moves the row
Expected: 409 / Received: 400 the same state from a second attempt
Expected: 403 / Received: 400 a different host reporting anything
51 pass, 22 fail
```
Every 400 is the strict validator refusing `claimToken` — the live
break,
reproduced. After:
```
284 pass, 0 fail, 777 expect() calls
```
against a `dev` baseline of **271 pass, 0 fail, 747 expect()** that I
measured
before starting. 13 new tests.
## What this does not verify
- **No agent has ever sent one of these requests.** Both ends are still
held by
tests written against a document. This PR makes the shapes agree by
reading
both, which is the thing rule 1 exists to avoid needing — it is a
repair, not
evidence that the wire works. Only a live round trip settles it.
- **Two attempts racing now happens in the tests, but only in one
process.**
Two tests claim concurrently: one races whole `transition` calls, the
other
fires the guarded updates directly so nothing but the `where` clause can
refuse the second. Both fail with two winners if the check is moved out
of
the write. What they do not reach is two *processes* against a shared
database, which is the real shape — and with one host it cannot happen
in
the field either.
- **Neither guard in that `where` clause is pinned on its own.** The
state
predicate and the holder predicate each cover the other, so removing
either
one alone leaves every test passing. That redundancy is deliberate, but
it
means these tests hold the pair and not the parts.
- **An agent that restarts loses its token**, and is then locked out of
a run it
is still hosting. The host side persists it to disk, which narrows this
a lot,
but nothing on this side can recover from a lost holder and nothing
reaps the
run that results.
- **`min(22)` is not an entropy check.** A caller can present 22
identical
characters and be believed. Nothing on this side can verify randomness.
- The `notHolder` branch of `publishTicket` is reachable only when a run
is past
`requested`; the invariant guard on the claim path is unreachable by
design and
is marked as such rather than tested.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR binds each session run to the agent attempt that claimed it.
- Accepts `claimToken` in state-report and ticket-publish API payloads.
- Atomically records the token during the `requested → starting`
transition.
- Rejects later state or ticket writes from attempts that do not hold
the claim.
- Keeps the token out of serialized session responses.
- Adds route, core, and concurrent PostgreSQL coverage for claim
ownership and guarded updates.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge; the prior concern about concurrent claims
is addressed by tests that execute competing guarded updates against
PostgreSQL.
No actionable new failures or repository-rule violations remain, and the
added concurrent coverage verifies that exactly one attempt can claim a
run while only its token can perform subsequent writes.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| packages/core/src/session/index.ts | Adds atomic claim ownership and
enforces it on subsequent state transitions and ticket publication. |
| apps/api/app/routes/session.ts | Extends strict request schemas with
claim tokens and maps holder conflicts to HTTP 409. |
| packages/core/src/session/session.test.ts | Covers claim persistence,
competing attempts, guarded concurrent updates, ticket ownership, and
response non-disclosure. |
| apps/api/test/session.test.ts | Verifies the claim-token wire contract
and route-level conflict behavior. |
| packages/core/src/examples.ts | Adds a correctly shaped claim-token
example for generated API documentation. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant A as Attempt A
participant B as Attempt B
participant API
participant DB as Session row
par Competing claims
A->>API: report starting + token A
API->>DB: UPDATE WHERE requested AND token IS NULL
and
B->>API: report starting + token B
API->>DB: UPDATE WHERE requested AND token IS NULL
end
DB-->>API: Exactly one guarded update succeeds
API-->>A: moved or conflict
API-->>B: moved or conflict
Note over DB: Winning token remains the holder
A->>API: later state/ticket write + token A
API->>DB: "UPDATE WHERE claimToken = token A"
B->>API: later state/ticket write + token B
API-->>B: 409 Another attempt holds this run
```
<sub>Reviews (2): Last reviewed commit: ["test(core): claim two attempts
at once,
..."](https://github.com/nestrilabs/nestri/commit/647e5c5264b390af3a10c5f21d8aa79ae9b807ea)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=60722179)</sub>
<!-- /greptile_comment -->
|
||
|
Wanjohi
|
647e5c5264 |
test(core): claim two attempts at once, not one after the other
The mutual exclusion was asserted only through sequential calls, where the winner had already committed before the rival began. That never reaches the case the design is for: both attempts reading the run as unclaimed before either writes. Two tests, because the first can pass for the wrong reason. The concurrent transitions depend on how the transactions interleave; the paired updates skip the read entirely, so nothing but the predicate in the where clause can refuse the second. Both fail with two winners if the check is moved out of the write and left in the read above it. |
||
|
Wanjohi
|
54d5c81edb |
feat(api): hold a run to the attempt that claimed it
The agent side sends a claim token on every write; this side rejected the field outright, so every state report and every ticket publish answered 400. Both bodies now take it. Underneath that, nothing compared a holder. A run was reachable by any caller on the right machine, and a box names exactly one machine — so two attempts polling the same job presented identical credentials and were told apart only by which one's select landed first. That is timing, not a rule, and no caller could be told which case it was in. The row now remembers which attempt holds it. Taking a claim requires there to be no holder; every write after it requires the caller to be the holder. The same state reported by a different attempt is a lost race and not a retry, and is refused whatever the state is - which is the only thing that separates the two 200s from the 409s. The ticket is held to the claim too, for a worse reason than a double start: the client re-reads the address rather than keeping the first, so a ticket written by a losing attempt produces a client that connects, successfully, to a machine running nothing. The holder is never cleared, including on a terminal state, so a settled claim cannot be replayed and a finished run still records which attempt ran it. It is not in what goes out - holding one permits writing to a run, and the owner reading their own session is not the holder. |
||
|
KAAL1 (Bingus)
|
2faf7d77db |
feat(nesinit): mount what the descriptor names, and relay the layer it cannot read (#320)
Stacked on #319, which has PID 1, the channel and the trait but mounts nothing. Review that one first; this PR is the descriptor half. - **The shares are mounted.** A tag names an export, the descriptor names where it lands, and every share goes on `nosuid` and `nodev` whether or not it is writable — a share is data handed to the guest, and no descriptor has a way to ask for a setuid binary or a device node in one. Mounting needs privileges a test does not have, so the arguments and flags are derived by a function the tests assert; that is where the read-only decision lives. - **Progress is two messages, not one.** `mounted` / `mount_failed` stay apart from `started` / `start_failed`, because a share that did not appear and a command that did not run want different things looked at. A failure carries the reason the operating system gave, verbatim, and the path it happened on. - **The second layer is relayed and never read.** Envelopes cross a unix socket to the workload and come back the same way. `body` is a string rather than nested JSON on purpose: a document this component can index into is a document it can grow to depend on, and then the layer is not opaque any more and the boundary it exists to draw is gone. - **An envelope is never logged** — not the body, not truncated, not at debug level. The channel name and a byte count are the whole of what may be said about one. `Payload`'s `Debug` is written by hand for the same reason. - **A write to a channel nobody reads now ends the session** the same way a closed read does, and stops the workload. A caller that stopped listening has also stopped being able to say stop; that was two outcomes and is one. ## The tests, failing first Progress reporting, with the mount result dropped (the state this branch started from): ``` running 11 tests test session::tests::an_unreadable_line_does_not_end_a_session ... ok test session::tests::a_stop_is_idempotent_and_does_not_end_the_session ... ok test session::tests::the_guest_speaks_first_and_says_its_version ... ok test session::tests::a_closed_channel_stops_the_workload ... ok test session::tests::the_descriptor_mounts_and_starts_what_it_names ... ok test session::tests::an_envelope_crosses_the_session_in_both_directions_unread ... ok test session::tests::a_share_that_will_not_mount_is_refused_before_anything_starts ... ok test session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ... FAILED test session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount ... FAILED test session::tests::a_signalled_workload_is_reported_as_signalled ... FAILED test session::tests::a_relay_nothing_is_on_does_not_end_a_session ... FAILED failures: ---- session::tests::an_exit_is_reported_and_the_workload_is_not_started_again stdout ---- thread 'session::tests::an_exit_is_reported_and_the_workload_is_not_started_again' (312969) panicked at apps/nesinit/src/session.rs:248:13: assertion `left == right` failed left: Started right: Mounted ---- session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount stdout ---- thread 'session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount' (312963) panicked at apps/nesinit/src/session.rs:460:9: assertion `left == right` failed left: StartFailed { reason: "ENOENT: /usr/bin/workload" } right: Mounted note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- session::tests::a_signalled_workload_is_reported_as_signalled stdout ---- thread 'session::tests::a_signalled_workload_is_reported_as_signalled' (312966) panicked at apps/nesinit/src/session.rs:248:13: assertion `left == right` failed left: Started right: Mounted ---- session::tests::a_relay_nothing_is_on_does_not_end_a_session stdout ---- thread 'session::tests::a_relay_nothing_is_on_does_not_end_a_session' (312964) panicked at apps/nesinit/src/session.rs:248:13: assertion `left == right` failed left: Started right: Mounted failures: session::tests::a_command_that_will_not_run_is_reported_apart_from_a_share_that_will_not_mount session::tests::a_relay_nothing_is_on_does_not_end_a_session session::tests::a_signalled_workload_is_reported_as_signalled session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ``` The read-only flag, ignored: ``` running 9 tests test shutdown::tests::a_workload_that_leaves_in_time_is_not_killed ... ok test shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power ... ok test shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes ... ok test workload::tests::a_failure_names_the_path_it_happened_on ... ok test workload::tests::a_writable_share_is_still_mounted_without_devices_or_setuid ... ok test workload::tests::a_read_only_share_is_mounted_read_only ... FAILED test session::tests::a_closed_channel_stops_the_workload ... ok test session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ... ok test session::tests::a_signalled_workload_is_reported_as_signalled ... ok failures: ---- workload::tests::a_read_only_share_is_mounted_read_only stdout ---- thread 'workload::tests::a_read_only_share_is_mounted_read_only' (311963) panicked at apps/nesinit/src/workload.rs:212:9: assertion `left == right` failed left: 0 right: 1 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: workload::tests::a_read_only_share_is_mounted_read_only ``` The relay quoting a line it could not decode — which is the second way a body reaches a log line, and the reason the failure path logs a length and nothing else: ``` running 2 tests test payload::tests::an_envelope_crosses_in_both_directions_untouched ... ok test payload::tests::nothing_the_relay_logs_contains_a_body ... FAILED failures: ---- payload::tests::nothing_the_relay_logs_contains_a_body stdout ---- thread 'payload::tests::nothing_the_relay_logs_contains_a_body' (312716) panicked at apps/nesinit/src/payload.rs:202:9: a body reached a log line: 2026-09-04T21:11:56.619025Z WARN nesinit::payload: ignoring an envelope that would not decode bytes=62 line={"channel":"identity","body":"a-credential-nobody-should-read" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: payload::tests::nothing_the_relay_logs_contains_a_body test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 17 filtered out; finished in 0.06s ``` And the first way: a derived `Debug` instead of the hand-written one. ``` running 6 tests test lifecycle::tests::a_mount_failure_keeps_its_reason_verbatim ... ok test lifecycle::tests::a_signalled_exit_is_not_a_zero_exit ... ok test lifecycle::tests::a_line_round_trips ... ok test lifecycle::tests::defaults_cover_what_a_caller_may_leave_out ... ok test lifecycle::tests::an_envelope_does_not_print_its_body ... FAILED test lifecycle::tests::an_envelope_body_stays_a_string_in_both_directions ... ok failures: ---- lifecycle::tests::an_envelope_does_not_print_its_body stdout ---- thread 'lifecycle::tests::an_envelope_does_not_print_its_body' (313791) panicked at crates/nesprotocol/src/lifecycle.rs:290:9: the body reached a log line: Payload { channel: "identity", body: "a-credential-nobody-should-read" } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: lifecycle::tests::an_envelope_does_not_print_its_body test result: FAILED. 5 passed; 1 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.00s error: test failed, to rerun pass `-p nesprotocol --lib` ``` All green after: 19 unit tests, 3 against real forked children, 15 in `nesprotocol`. ## What this does not verify - **Nothing has been mounted.** No `virtiofs` share has been mounted by this code, in a VM or anywhere else. What is tested is the source, target and flag word handed to the mount call; that the call succeeds against a real virtio transport, that the mount point is where a workload then finds its files, and that a `uid` mismatch surfaces as the permission error this is written to produce, are all unverified. - **The relay has never carried a real workload's traffic.** Two processes, a real unix socket and bytes that come back unchanged is what the test shows. Whether the socket path is the right mechanism is openly a guess, and it is meant to be replaceable without anything above it moving. - **"Never logged" is enforced more narrowly than it reads.** The hand-written `Debug` and the failure path's length-only line are both tested. The capture test cannot reliably see debug-level lines: callsite interest is cached process-wide, so a line another test in the same binary reached first never arrives in the capture. A future `{:?}` on a whole envelope at debug level would not necessarily be caught by these tests, only by the `Debug` impl keeping its shape. - **`geometry` is parsed and carried, and nothing consumes it.** This component does not start the guest's own services yet. `ticket` exists as a message with no producer wired to it. - **Still no VM, still no vsock, still no `uid` drop**, as in #319, and no number in this PR is measured. - **No third-party workload has gone through any of this.** The claim that a descriptor plus a set of shares is enough to run something we did not write is untested, and our own workload is the weakest possible witness for it. ## Since review `d4d473f` — the relay may not stall the session and may not buffer without end, plus a descriptor with a nul byte in it is refused by name. Failing first, in order: The session held still behind a workload that was not reading: ``` running 1 test test session::tests::a_relay_that_is_not_draining_does_not_stall_the_session ... FAILED failures: ---- session::tests::a_relay_that_is_not_draining_does_not_stall_the_session stdout ---- thread 'session::tests::a_relay_that_is_not_draining_does_not_stall_the_session' (326881) panicked at apps/nesinit/src/session.rs:670:10: the session stalled on the relay: Elapsed(()) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: session::tests::a_relay_that_is_not_draining_does_not_stall_the_session test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 5.00s error: test failed, to rerun pass `-p nesinit --lib` ``` A frame with no end to it: ``` running 1 test test payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest ... FAILED failures: ---- payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest stdout ---- thread 'payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest' (327888) panicked at apps/nesinit/src/payload.rs:375:10: the relay is still assembling a frame that never ends: Elapsed(()) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: payload::tests::a_frame_that_never_ends_costs_the_connection_and_not_the_guest test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 5.01s error: test failed, to rerun pass `-p nesinit --lib` ``` And a tag that was quietly emptied instead of refused: ``` running 1 test test workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name ... FAILED failures: ---- workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name stdout ---- thread 'workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name' (327380) panicked at apps/nesinit/src/workload.rs:274:40: an empty source would have been mounted: ("", "/mnt/user", 6) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: workload::tests::a_descriptor_with_a_nul_byte_in_it_is_refused_by_name test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 22 filtered out; finished in 0.00s error: test failed, to rerun pass `-p nesinit --lib` ``` One behaviour changed rather than only hardened, and it is worth a reviewer's eye: **nothing is queued for a workload that is not on the relay.** An envelope that arrives with nobody connected is dropped, as is one that arrives faster than the workload reads. That follows the layer's own rule — what crosses it is re-sent when it changes, so a held copy is a stale copy — but it does mean a sender that assumes delivery is wrong to. Nothing here retries, and nothing tells the far end that a particular envelope was dropped. 23 unit tests, 5 against real forked children, 15 in `nesprotocol`. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR mounts descriptor-defined virtiofs shares, reports mount and process-start progress independently, and relays opaque payload envelopes between the host channel and workload Unix socket. Changes since the previous review also bound relay frames, prevent relay backpressure from stalling lifecycle handling, reject descriptor strings containing NUL bytes, and track whether a reaped PID remains valid. - Mounts shares at descriptor-selected targets with `nosuid`, `nodev`, and optional read-only flags. - Adds bidirectional opaque payload forwarding with bounded, non-blocking queues and body-safe logging. - Adds distinct mounted/start lifecycle responses and failure reporting. - Adds capped newline-delimited relay frames and drops stale or backpressured envelopes. - Reworks workload tracking to avoid signaling a PID after its exit has been delivered. <h3>Confidence Score: 5/5</h3> The reviewed changes appear safe to merge, with no accepted new findings or outstanding previous root-thread findings. The resolved relay-stall, unbounded-frame, and invalid-NUL findings are addressed by non-blocking delivery, capped frame assembly, and explicit descriptor validation. The protocol-version concern was correctly withdrawn under the coordinated version-2 deployment model. The remaining PID check-to-signal race duplicates an existing prior comment and therefore is not reposted or counted as a new finding. <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | apps/nesinit/src/payload.rs | Adds the bounded, bidirectional Unix-socket payload relay with non-blocking delivery and body-safe logging. | | apps/nesinit/src/session.rs | Integrates payload events with lifecycle handling and separately reports mount and process-start outcomes. | | apps/nesinit/src/workload.rs | Implements descriptor-driven virtiofs mounts and switches process signaling to tracked reaper state. | | apps/nesinit/src/reap.rs | Adds shared reaped-state tracking so callers stop treating a delivered PID as the workload. | | crates/nesprotocol/src/lifecycle.rs | Extends lifecycle messages with mount progress and opaque payload envelopes while redacting payload bodies from Debug output. | | apps/nesinit/src/main.rs | Starts the payload relay before the workload session and wires bounded relay ports into session handling. | | apps/nesinit/README.md | Documents mount behavior, payload opacity, delivery semantics, frame limits, and progress reporting. | <h3>Sequence Diagram</h3> ```mermaid sequenceDiagram participant H as Host participant N as nesinit participant M as virtiofs mounts participant W as Workload H->>N: Boot descriptor N->>M: Mount descriptor shares M-->>N: Success or failure N-->>H: mounted / mount_failed N->>W: Start command N-->>H: started / start_failed H->>N: Payload envelope N-->>W: Non-blocking Unix-socket relay W->>N: Payload envelope N-->>H: Payload envelope W-->>N: Exit N-->>H: workload_exited ``` <sub>Reviews (3): Last reviewed commit: ["fix(nesinit): the relay may not stall th..."](https://github.com/nestrilabs/nestri/commit/e94ea005938dea48baf554815dcb489dc666666f) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=60585116)</sub> **Context used:** - Knowledge Base — [Streaming appliance build](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/appliance-build.md) <!-- /greptile_comment --> |
||
|
Wanjohi
|
217e82a9a8 |
feat(auth): email is the root of an account, and Steam is a connection (#318)
## What landed
**Email is the root of an account.** A `user` is created by verifying an
email
address and nothing else. Every Steam account is now a connection
hanging off a
user that already exists, capped at four.
**And it is the only thing that creates one.** Signing in with a gaming
account
or with an SSH key are both unwired from the issuer. Each could mint a
user,
which makes an account only as recoverable as the thing that made it and
gives
one person as many accounts as they have gaming logins. The providers
still
exist under `packages/auth/src/provider/` and can be wired back;
connecting a
Steam account is unaffected, because that runs through `POST
/steam/link` in
`apps/api` against a user who already exists. A test asserts the two
routes are
not served, so they cannot come back quietly.
**The pin-code provider is wired**, and email delivery refuses rather
than
guesses. **The device authorization grant is served**, and it now ends
at a
question somebody has to answer.
## The review found five real defects. All five are fixed, and so are
six more
The first pass of this branch shipped a device flow that handed out
tokens
without asking anybody, an email path whose pin could be guessed
outright, and
three read-then-write races. Each fix below has a test that fails
without it —
verified by reverting the fix and watching the test go red, not by
assertion.
### Signing in was mistaken for saying yes
`GET /device?user_code=…` started a provider flow and provider success
approved
the grant. So the whole attack was: ask for a device code, mail somebody
the
pre-filled link, keep the device code, poll. They see an ordinary
sign-in
prompt, complete it correctly, and you hold their access **and refresh**
tokens.
They were never asked a question, because there wasn't one.
There is now. Signing in establishes who the browser belongs to; it does
not
establish that the person meant to hand an account to a program running
somewhere else. The flow ends at a page that names the client, shows the
user
code back so it can be compared against what the device is displaying,
and
offers Approve and Deny. Approving is a POST carrying a value placed in
the
cookie alongside it, so another site cannot submit it on their behalf.
Denial moved onto that page too. It was a `GET` anybody could fire with
no
authentication: a link scanner or a chat unfurler would cancel real
sign-ins,
and anyone who learned a user code could grief one.
### A six-digit pin with unlimited guesses and a day to use them
The code travelled in an encrypted cookie held by the caller,
verification
compared against that cookie, and a wrong answer re-rendered the form.
Nobody
has to be the person the code was mailed to — type someone else's
address into
the first screen and the code goes to their mailbox while the cookie
stays with
you. At that point the only thing between a stranger and an account was
a
million requests. The constant-time comparison was guarding a door you
could
keep knocking on.
Guesses are counted on the server now, under a name that rotates with
every
code. The placement is the point: a counter kept beside the code, in the
cookie,
is one the guesser winds back by replaying an older copy. Starting over
is still
allowed and still costs a fresh code sent to the mailbox being aimed at,
where
somebody notices. The cookie's twenty-four hour life is ten minutes.
Resend is
spaced and bounded, because it was otherwise a way to mail a stranger as
fast as
requests go out. Both refusals say the same thing, since which one it
was is a
fact about someone else's mailbox.
The user codes on the other side of the flow are rate limited too —
eight
characters over a twenty-five character alphabet is a large space but a
fixed
one, and the endpoint had no opinion about how often you asked.
### Three read-then-write races
- **A poll could erase an approval.** The grant was read, modified and
written
back whole, so a poll that read a pending record and then wrote its
bookkeeping undid an approval that landed in between, leaving the client
polling a dead grant until it aged out. The same window let an approval
overwrite a denial.
- **The connection cap counted nothing.** `select … for update` over the
connections a user already had locks the rows it finds, and finding none
locks
nothing — there are no gap locks under read committed. Six concurrent
links
against a cap of four produced six; the test asserts that.
- **Concurrent email sign-ins returned a driver error.** Two tabs
finishing the
same sign-in both found no user, and the loser got a raw constraint
violation
instead of the account the winner had just made.
The cap now counts under a lock on the account's own row, which is the
one thing
every caller for that account contends on. The email paths let the
unique index
arbitrate and read back what the winner wrote. Device grants moved out
of the
key-value store into a table, where approving is one conditional update
and
redeeming is one delete that returns what it deleted.
### Three more the review did not raise
- **`client_id` was never checked at either end.** Anyone could mint a
grant
naming any client, and any holder of a leaked device code could redeem
it. It
is validated at issue and has to match at redemption.
- **Tokens were minted at approval** and left in storage until
collected, so the
lifetime reported to the client overstated what was left, and a grant
nobody
collected still left a usable refresh token lying around. They are
minted at
redemption.
- **The device code was stored as written.** It is the credential the
tokens are
handed to, so what is kept is now its hash: enough to recognise it, not
enough
to present it.
### And the environment check that decided none of this mattered
Mail delivery threw only when the environment said `production`, and
logged the
recipient and the live code otherwise. The deployment sets no such
marker — see
`alchemy.run.ts` before this change — so production took the developer
branch,
printed every code to a retained log, and reported success while nobody
received
anything.
That is the cost of a fail-open default: the deployment that forgets its
mail
settings is exactly the one with no marker saying it is real, so it gets
the
lenient branch precisely when it should not. Turned around. Printing a
code is
asked for by name; absence of configuration is a refusal; two settings
out of
three is an error rather than a fallback. Stages anyone else can reach
are
checked at deploy time, so a missing setting stops the deploy with the
name of
the variable it wanted.
## Where device grants live, and why it is a table
Short-lived state that would sit happily in a cache, in Postgres anyway.
The
reason is not durability. Every transition has to happen exactly once
while two
parties touch the same record — a browser somebody is clicking through
and a
program polling every few seconds — and a store that can only read and
write
whole records cannot promise that. The key-value store behind the rest
of the
issuer has no compare-and-swap, so on it the poll/approval race and
single
redemption can be narrowed and never closed.
The issuer cannot reach the database, so the store is an interface
(`packages/auth/src/device.ts`) with two implementations: one in memory
for
tests, one in `packages/core/src/auth/device-grant.ts`. Every method is
a single
operation and no caller reads a grant, decides, and writes it back.
Migration
`0010` adds the table. Rows are swept when a grant is created rather
than on a
schedule, since a grant lives ten minutes and that is the only statement
that
adds one.
## `session.claim_token` is here on another lane's behalf
Migration `0009` adds `session.claim_token`, nullable `text`, no default
and no
backfill. **It is not identity work and carries no identity reason** —
do not go
looking for one. It records which attempt holds a session run; the
endpoint that
reads and writes it arrives separately. It is in this migration only
because a
schema change has one owner at a time. The column is declared in
`session.sql.ts` and the snapshot, so the next `drizzle-kit generate`
will not
try to drop it — `0010` was generated clean, which is the proof.
## The tests
```
$ bun test
268 pass
0 fail
738 expect() calls
Ran 268 tests across 21 files.
```
Baseline on `dev` before any of this, measured on the same database:
**198 pass,
0 fail, 531 expect() calls.**
The tests that matter are the ones that would have caught the defects,
so each
was checked by putting the defect back:
| Reverted | What goes red |
|---|---|
| the confirmation step | signing in through the link leaves the grant
pending |
| the guess counter | four tests, including one that replays an older
cookie |
| the lock on the account row | six connections against a cap of four |
| the unique-violation handling | a driver error where a sentence should
be |
| the field-level poll write | an approval erased by a poll behind it |
| the verification rate limit | four tests |
Concurrency is exercised by running the same call several times at once
against
a real database, because run one at a time all of it passes whether or
not any
of the protection exists.
## The migration, against a database built to be awkward
`packages/core/script/verify-migration-0009.sh` builds a database
containing the
rows that make the statements do work — an account with no address, two
accounts
holding one address in different cases, an account already over the cap,
a
soft-deleted row holding a live row's address — applies everything
before `0009`,
applies it, and checks each case. All nineteen checks still pass. The
negative
control still dies where it should: with the de-duplication statement
neutered,
`create unique index` fails on the first duplicate pair.
## What this still does not verify
**1. No real email has ever been sent.** The mailer is tested against a
stubbed
`fetch`: the URL, the bearer header, the body it builds, and that it
refuses
when unconfigured. The body shape follows the common `{from, to,
subject, text}`
convention and may need a field the first real provider wants. This now
fails
closed and the deploy refuses without settings, so the failure mode is
loud
rather than silent — but it is still untested against a provider.
**2. The migration is verified against a database I built, not the one
that
matters.** I do not know whether production has duplicate addresses, how
many
rows carry case or whitespace, or whether any account is already over
the cap.
The fixtures cover those because they are *possible*. Worth three
`select`s
against production before this is applied.
**3. The verification rate limit is approximate.** The counter is in the
key-value store, so a caller spread across a distributed edge can exceed
the
budget somewhat. The number that decides the question is whether
somebody is
working through the code space, and a handful either way does not change
it.
**4. The single-redemption and conditional-transition properties are
held
against Postgres, and the in-memory store is trusted rather than
proved.** The
memory implementation gets its atomicity from nothing suspending inside
a
method, which is true of it and is not a promise the interface makes. It
is for
tests and local runs.
**5. A person who signed up by email carries an empty connected-account
id in
their token.** The subject schema requires the field and an account with
no
connection has nothing to put there, so it gets `''` — the same value a
server-to-server caller has always carried, and the one consumer already
falls
back to `''`. The honest shape is an optional field, and making it
optional
touches `subjects.ts`, the actor model and the API middleware. Flagging
rather
than doing.
**6. The desktop client cannot actually complete this flow yet.** Its
`DeviceCode` struct has no field for the device code, so it has nothing
to poll
with. That is a different lane's file and nothing here touches it, but
the grant
is not reachable end to end until it does.
**7. The four-account cap and the user-code alphabet are asserted, not
measured.** Four comes from a household's size and a screen's width. The
alphabet excludes look-alikes on the same reasoning. No measurement
decides
either, and no test here pretends one does.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR makes verified email the root account identity, turns Steam into
an attached account connection, adds provider-neutral email-code
delivery, and implements an RFC 8628 device authorization flow backed by
atomic PostgreSQL grant transitions. It also aligns the related
identity, session, and device-grant migrations and adds concurrency and
flow tests.
- Removes Steam and SSH as direct auth-worker sign-in providers.
- Adds email-code account creation and deployment-time mail
configuration checks.
- Adds explicit device approval, denial, polling throttling, client
binding, and one-time redemption.
- Serializes Steam-link cap enforcement and handles concurrent email
uniqueness conflicts.
- Adds and aligns migrations, snapshots, durable device-grant storage,
and `session.claim_token`.
- One non-blocking resend-limit replay issue remains.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge, with one non-blocking email-delivery abuse
limitation that should be hardened.
The prior device-token theft, polling race, Steam-link concurrency,
email uniqueness, and session-schema findings are fixed in the current
code; the five corresponding threads were manually resolved without
explanatory replies. The remaining new issue permits bypassing the
intended email send cap through replay of an older provider cookie, but
the resend interval still bounds its rate and it does not compromise
account authentication.
**Files Needing Attention:** packages/auth/src/provider/code.ts
<details open><summary><h3>Security Review</h3></summary>
The device flow now requires an explicit, CSRF-protected confirmation
and uses atomic, client-bound redemption. One lower-impact abuse issue
remains: replaying an older code-provider cookie can bypass the intended
email send cap.
</details>
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| packages/auth/src/issuer.ts | Adds an explicitly confirmed RFC 8628
device flow with client-bound, one-time token redemption. |
| packages/auth/src/provider/code.ts | Adds server-side attempt and send
accounting, but replacement flows leave earlier cookie-referenced
counters replayable. |
| packages/core/src/auth/device-grant.ts | Implements durable device
grants using atomic conditional approval, denial, polling updates, and
consumption. |
| packages/core/src/user/identity.ts | Makes email identity creation
conflict-aware and serializes Steam-link cap enforcement on the user
row. |
| apps/auth/src/index.ts | Reconfigures the deployed issuer around email
sign-in and PostgreSQL-backed desktop device authorization. |
| alchemy.run.ts | Requires complete mail configuration for permanent
stages and explicitly enables code logging only for ephemeral
development stages. |
| packages/core/migrations/0010_device_authorization_grant.sql | Adds
the device-grant enum, table, and unique indexes in alignment with the
Drizzle model and snapshot. |
<h3>Sequence Diagram</h3>
```mermaid
sequenceDiagram
participant D as Desktop client
participant A as Auth issuer
participant B as Browser
participant E as Email provider
participant DB as PostgreSQL
D->>A: POST /device/authorize
A->>DB: Create pending grant
A-->>D: device_code, user_code, interval
B->>A: Enter user_code
A->>E: Send email verification code
B->>A: Verify email code
A-->>B: Display client and user-code confirmation
B->>A: Approve or deny
A->>DB: Atomic terminal transition
loop Until terminal
D->>A: Poll /token
end
A->>DB: Delete-and-return approved grant
A-->>D: Access and refresh tokens
```
<details><summary>Prompt To Fix All With AI</summary>
`````markdown
### Issue 1
packages/auth/src/provider/code.ts:291-297
**Cookie replay bypasses send cap**
Replaying an earlier encrypted provider cookie bypasses `maxSends`. A resend creates a new flow with an incremented counter but leaves the old flow and its lower counter valid, so the old cookie can call `sendCode` again after each resend interval. This permits repeated unsolicited sign-in emails to an attacker-selected address, although the resend interval still limits their rate.
**How this was verified:** The resend check reads only the flow named by the presented cookie, while creating its replacement neither updates nor removes that old flow.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
`````
</details>
<sub>Reviews (3): Last reviewed commit: ["fix(auth): stop a caller
working
through..."](https://github.com/nestrilabs/nestri/commit/fc825f5219db6144ea8b026f3fa52965bb2ded3b)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=60581000)</sub>
> Greptile also left **1 inline comment** on this PR.
<details><summary><h4>Context used (4)</h4></summary>
- Knowledge Base — [Authentication
platform](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/auth-platform.md)
- Knowledge Base — [Auth providers, sessions, and
storage](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/auth-providers-and-storage.md)
- Knowledge Base — [Core domain and
persistence](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-domain-data.md)
- Knowledge Base — [Users, identity, and game
libraries](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-identity-and-library.md)
</details>
<!-- /greptile_comment -->
|
||
|
KAAL1 (Bingus)
|
736c0013e9 |
feat(nesinit): PID 1 for a box — reaping, ordered shutdown, and one channel out (#319)
PID 1 inside a box. A microVM has no init unless something is it, and
three of
the jobs belong to nothing else in the guest: reaping whatever the
workload
orphans, turning a signal into an ordered shutdown, and holding the
guest end
of the one channel out.
- `reap.rs` — reaping, plus the subreaper bit and taking init out of the
OOM
killer's reach.
- `shutdown.rs` — the order, behind a trait so it can be asserted
without a VM:
workload first and alone, then everything else, then flush, then power
off.
- `session.rs` — the exchange. The guest speaks first with its protocol
version, is handed one boot descriptor, reports, and stops. Generic over
the
byte stream, so the whole protocol is testable over an in-memory pipe.
- `workload.rs` — one trait the descriptor drops into, a real process
behind
it, and a double.
- `nesprotocol::lifecycle` — the types, behind a feature that is off by
default so the media components keep building without serde.
It reports and does not supervise: when the workload ends, the exit goes
up the
channel and the session is over. Nothing here restarts anything.
**Mounting is not implemented in this PR.** The descriptor's `mounts`
are
refused rather than ignored, and the next PR in the stack implements
them.
### On the OOM killer
`refuse_oom_kill()` writes `-1000` to this process's own
`oom_score_adj`, and
that part is init's job: everything else in the guest exiting is a
message up
the channel, whereas init exiting takes the channel with it, and the far
end
sees a box that stopped answering for no stated reason.
The rest is the image's job and cannot be done from here. Making the
workload
the *preferred* victim means scoring processes this component did not
start, so
the image has to leave the workload's score at or above the default and
must
not lower it for the guest's own services either.
## The tests, failing first
Reaping, with `reap_exited()` returning nothing:
```
running 3 tests
test a_child_that_exits_is_reaped_with_its_code ... FAILED
test a_child_that_is_killed_is_reaped_as_signalled ... FAILED
test an_orphan_is_reaped_by_whoever_inherits_it ... FAILED
failures:
---- a_child_that_exits_is_reaped_with_its_code stdout ----
thread 'a_child_that_exits_is_reaped_with_its_code' (304365) panicked at apps/nesinit/tests/reaping.rs:52:32:
the child was left a zombie
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- a_child_that_is_killed_is_reaped_as_signalled stdout ----
thread 'a_child_that_is_killed_is_reaped_as_signalled' (304366) panicked at apps/nesinit/tests/reaping.rs:66:32:
the child was left a zombie
---- an_orphan_is_reaped_by_whoever_inherits_it stdout ----
thread 'an_orphan_is_reaped_by_whoever_inherits_it' (304367) panicked at apps/nesinit/tests/reaping.rs:98:5:
the child was left a zombie
failures:
a_child_that_exits_is_reaped_with_its_code
a_child_that_is_killed_is_reaped_as_signalled
an_orphan_is_reaped_by_whoever_inherits_it
test result: FAILED. 0 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 15.02s
```
Shutdown, with one signal to everything instead of an order:
```
running 3 tests
test shutdown::tests::a_workload_that_leaves_in_time_is_not_killed ... ok
test shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes ... FAILED
test shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power ... FAILED
failures:
---- shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes stdout ----
thread 'shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes' (304568) panicked at apps/nesinit/src/shutdown.rs:105:79:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power stdout ----
thread 'shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power' (304569) panicked at apps/nesinit/src/shutdown.rs:80:9:
assertion `left == right` failed
left: ["signal_rest", "kill_rest", "flush_disks", "power_off"]
right: ["signal_workload", "await_workload", "signal_rest", "kill_rest", "flush_disks", "power_off"]
failures:
shutdown::tests::a_workload_that_overstays_its_grace_is_killed_and_shutdown_still_finishes
shutdown::tests::the_workload_stops_before_anything_else_and_the_disks_flush_before_power
test result: FAILED. 1 passed; 2 failed; 0 ignored; 0 measured; 8 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
The handshake, with the version left where it was before the bump:
```
running 1 test
test session::tests::the_guest_speaks_first_and_says_its_version ... FAILED
failures:
---- session::tests::the_guest_speaks_first_and_says_its_version stdout ----
thread 'session::tests::the_guest_speaks_first_and_says_its_version' (304772) panicked at apps/nesinit/src/session.rs:180:9:
assertion `left == right` failed
left: Ready { protocol_version: 1 }
right: Ready { protocol_version: 2 }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
session::tests::the_guest_speaks_first_and_says_its_version
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 10 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
And the property most likely to be quietly regressed later — a
supervisor loop
that restarts what it started, instead of reporting:
```
running 1 test
test session::tests::an_exit_is_reported_and_the_workload_is_not_started_again ... FAILED
failures:
---- session::tests::an_exit_is_reported_and_the_workload_is_not_started_again stdout ----
thread 'session::tests::an_exit_is_reported_and_the_workload_is_not_started_again' (304968) panicked at apps/nesinit/src/session.rs:237:9:
assertion `left == right` failed: an exit is reported, never restarted
left: 2
right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
session::tests::an_exit_is_reported_and_the_workload_is_not_started_again
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 10 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p nesinit --lib`
```
All green after: 11 unit tests, 3 against real forked children, plus 3
new in
`nesprotocol`.
## What this does not verify
- **Nothing has run in a VM, and nothing has run as PID 1.** Orphan
reparenting is exercised with `PR_SET_CHILD_SUBREAPER` in a test
process,
which is the same kernel mechanism but not the same privilege.
- **No real vsock connection has been made.** The protocol is tested
over an
in-memory pipe; the dial itself — the address, the port, a listener that
is
not there — is unexercised, and the deliberate no-retry behaviour has
never
met a refused connection.
- **The real shutdown is untested.** The order is asserted through a
double;
`kill(-1)`, `sync` and the power-off call themselves need a guest, and a
test
process must not make them.
- **The OOM write is unchecked.** It warns and continues where there is
no
procfs, and nothing here confirms the kernel honoured the score.
- **Dropping to `uid`/`gid` before exec is unexercised** — it needs
privileges
a test does not have, so the `pre_exec` path has run in no test.
- **No number here is measured.** Nothing in this PR claims a timing, a
rate
or a count from hardware.
- **A version mismatch is not refused by this end.** The guest announces
its
version in its first line and the far end compares; as the layer stands
there
is nothing for the guest to compare against, so "both ends refuse on
mismatch" is only half-implementable. Worth settling in the channel's
specification before either end grows a second version, and I would
rather
raise it than invent a message for it here.
## Since review
`cb8f37a` — three fixes, all from the review, described in its message.
The one
worth naming here is that the reaper is now the only thing in the
component
that calls `wait`: it hands each exit to whoever asked for that pid, and
registering interest holds the same lock the delivery takes, so an exit
that
happens before its caller is registered is delivered rather than
dropped.
There is a test for exactly that, and it fails without the lock:
```
running 1 test
test an_exit_that_happens_before_the_caller_is_registered_is_not_lost ... FAILED
failures:
---- an_exit_that_happens_before_the_caller_is_registered_is_not_lost stdout ----
thread 'an_exit_that_happens_before_the_caller_is_registered_is_not_lost' (321463) panicked at apps/nesinit/tests/reaping.rs:171:9:
the exit was dropped on the way through
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
an_exit_that_happens_before_the_caller_is_registered_is_not_lost
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 4 filtered out; finished in 5.20s
error: test failed, to rerun pass `-p nesinit --test reaping`
```
Also, and still not verified: the ordered shutdown's real calls now
include
signalling and waiting for one pid rather than every child, and none of
that
has run in a guest either.
`071241f` — a pid stops being the workload's the moment it is reaped, so
a stop
or a kill during shutdown can no longer land on whatever the kernel gave
that
number to next. One window is left, between the reap and the delivery,
and
closing it needs a handle the kernel keeps rather than a number — noted
below
rather than papered over.
- **A pid is still a number here.** Between a reap and the exit being
handed
on, a freed pid is briefly treated as the workload's. Nothing has hit
that
window, and nothing can until a guest recycles pids under load; a
`pidfd`
would remove the class of bug rather than narrow it.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR adds `nesinit`, a guest PID 1 implementation that reaps orphaned
children, exchanges lifecycle messages over vsock, launches one
workload, and performs ordered shutdown.
- Adds a centralized child reaper and waiter registry.
- Adds workload launch, identity changes, exit reporting, and explicit
rejection of unsupported mounts.
- Adds the lifecycle protocol behind an optional `nesprotocol` feature.
- Adds ordered workload-first shutdown and associated tests.
- Since the previous review, tracks whether the watched workload PID is
still live to reduce stale-PID signaling risk.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge based on the accepted findings and the
resolved state of all previous review threads.
No new actionable finding remains after excluding the stale-PID race as
a duplicate of a manually resolved previous thread and confirming that
the stale state left by the shutdown-only wait path is not subsequently
used to signal a workload PID. Previous thread PRRC_kwDOLnCyk87q1Eew was
manually resolved without explanation.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| apps/nesinit/src/reap.rs | Adds centralized child reaping, waiter
registration, exit delivery, and shared workload-running state. |
| apps/nesinit/src/workload.rs | Implements workload launch and
signaling through a watched PID whose delivered exit marks it inactive.
|
| apps/nesinit/src/main.rs | Wires the reaper, vsock session, workload
handle, and real ordered-shutdown operations together. |
| apps/nesinit/src/session.rs | Implements the versioned one-descriptor
lifecycle exchange and reports one workload exit without restarting it.
|
| apps/nesinit/src/shutdown.rs | Encodes and tests workload-first
shutdown followed by guest services, disk flush, and power-off. |
| crates/nesprotocol/src/lifecycle.rs | Defines the feature-gated
lifecycle protocol types shared by the guest and host. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[nesinit starts] --> B[Become subreaper and adjust OOM score]
B --> C[Connect to host over vsock]
C --> D[Send protocol version]
D --> E[Receive boot descriptor]
E --> F[Start workload and register PID]
F --> G{Session event}
G -->|Workload exits| H[Reaper delivers exit]
H --> I[Report workload exit]
G -->|Stop or channel closes| J[Signal workload]
G -->|Shutdown or session ends| K[Ordered shutdown]
I --> K
J --> K
K --> L[Stop workload]
L --> M[Stop remaining processes]
M --> N[Sync disks]
N --> O[Power off]
```
<sub>Reviews (3): Last reviewed commit: ["fix(nesinit): a pid stops
being the
work..."](https://github.com/nestrilabs/nestri/commit/071241f9440351be0149d421cba320b15f137236)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=60585100)</sub>
<!-- /greptile_comment -->
|
||
|
Wanjohi
|
304bb1f2ef |
fix(auth): count sign-in codes against the mailbox, not the browser
The cap on how many codes a sign-in could ask for was held per attempt, keyed by a value in the caller's own cookie. That bounds nothing. The caller decides how many attempts to start, and starting a fresh one costs them a discarded cookie — so either replaying an older cookie or simply beginning again walked straight around it, and the only thing left spacing the mail out was the interval between sends. The count now sits against the claim, over a window. That is the thing being protected: the mailbox belongs to somebody who did not ask to hear from us, and whoever is pointing at it is not the party to trust with the tally. A resend also left the previous code live, with a budget of guesses of its own. Several resends therefore meant several working codes and several times the chances at them, which made asking for a new code the cheapest way to buy more tries at the old one. A new code now retires the one before it. Reported against the replay path. The replay was real and the same hole was wider than that: starting a new attempt needed no replay at all. |
||
|
Wanjohi
|
fc825f5219 |
fix(auth): stop a caller working through the user code space
A user code is eight characters from a twenty-five character alphabet, which is a large space but a fixed one, and the endpoint that checked them had no opinion about how often you asked. That is the guessing attack RFC 8628 section 5.2 asks implementations to limit, and nothing here did. Wrong codes are now counted per caller address over a rolling window, and the endpoint stops answering once the budget is gone. Getting a code right is not charged for, so somebody who mistypes once and then succeeds is not walking towards a lockout. A caller whose address cannot be established shares one bucket with every other such caller, which makes stripping the headers that say where you are buy a smaller budget rather than an unlimited one. The counter lives in the general-purpose store and is approximate. The number that decides this is whether somebody is working through the code space, and a handful either way does not change that answer. |
||
|
Wanjohi
|
355d1492d9 |
fix(auth): give a sign-in code a budget of guesses and a short life
A six-digit code has a million values, and nothing was counting how many of them a caller tried. The code travelled in an encrypted cookie the caller held, verification compared against that cookie, and a wrong answer simply re-rendered the form. Nobody has to be the person the code was mailed to: type somebody else's address into the first screen and the code goes to their mailbox while the cookie stays with you. At that point the only thing between a stranger and an account is a million requests, and the constant-time comparison protecting the code was guarding a door you could just keep knocking on. Guesses are now counted on the server, under a name that changes with every code. That placement is the point: a counter kept beside the code, in the cookie, is a counter the guesser can wind back by replaying an older copy. Starting over is still allowed and still costs a fresh code sent to the mailbox being aimed at, which is where somebody notices. A correct code spends its record too, so its remaining guesses do not carry into the next one. The cookie also lived for twenty-four hours, which made the pin a password with a million possible values and a day to try them. Ten minutes now, and the code stops being accepted when the clock says so rather than when the cookie happens to go away. Resend had no limit either, so the button was a way to mail a stranger as fast as requests go out. Codes to one address are spaced, and one attempt at signing in can only ask for so many. Both refusals say the same thing on purpose. Which of the two it was is a fact about somebody else's mailbox. |
||
|
Wanjohi
|
36179150a1 |
fix(auth): make a device sign-in an answer somebody gave
Anybody could ask for a device code and be handed a link with the user code already in it. Following that link started a sign-in, and finishing the sign-in approved the grant. So sending somebody the link was enough: they saw an ordinary sign-in prompt, completed it, and whoever kept the device code polled and collected their access and refresh tokens. The victim never saw a question, because there was not one. There is now. Signing in says who the browser belongs to; it does not say the person meant to hand an account to a program somewhere else. Those are two questions and only the second authorizes anything, so the flow ends at a page that names the program, shows the code back so it can be compared with what the device is displaying, and offers Approve and Deny. Approving is a POST carrying a value from the cookie, so another site cannot submit it on somebody's behalf. Denial moved onto the same page: it used to be a GET anyone could fire, which meant a link scanner could cancel a real sign-in and a stranger with a user code could grief one. Three more things that were wrong underneath. The grant was read, modified and written back as a whole record. A poll that read a pending grant and then wrote its bookkeeping erased an approval that landed in between, and the client polled a dead grant until it expired. Grants moved to a table, where approving is one conditional update and redeeming is one delete that returns what it deleted, so neither party can undo the other and two polls cannot both be served. Tokens were minted when the person clicked and left sitting in storage until collected. They are minted at redemption now, so the lifetime the client is told about starts when it receives them, and a grant nobody collects leaves no usable refresh token behind. The client identifier was never checked, at either end. It is validated when the grant is created and has to match when the code is redeemed — without that, a leaked code is redeemable by anyone, and the identifier the token carries is whatever the last caller claimed. The device code is also stored as a hash now, since it is the credential the tokens are handed to. The store is an interface because the issuer cannot reach the database, and because the guarantees are the point: every method is one operation, and no caller reads a grant, decides, and writes it back. |
||
|
Wanjohi
|
15f8d3eb34 |
fix(core): hold the account rules when two requests arrive together
Three rules here are enforced across a lookup and then a write, and each was only as good as whatever stopped the two from interleaving. Nothing did. The connection cap counted with `select ... for update` over the connections a user already had. That locks the rows it finds, and when it finds none it locks nothing — there are no gap locks under read committed — so several first-time links all counted zero and all inserted. Six concurrent links against a cap of four produced six. The count now happens under a lock on the account's own row, which is the one thing every caller for that account is guaranteed to contend on. Creating an account from a verified address looked the address up and then inserted. Two tabs finishing the same sign-in both found nothing, and the loser got the driver's constraint violation instead of the account the winner had just made. The unique index is the thing that actually arbitrates, so the loser now reads back what the winner wrote. Claiming an address on an older account had the same shape and now gives the same sentence a screen would have shown a moment earlier. The tests run each call several times at once against a real database, because run one at a time all three pass whether or not any of this exists. |
||
|
Wanjohi
|
2c4e9d9b0b |
fix(auth): refuse to send a sign-in code rather than log one
The rule was "throw when the environment says production, otherwise log the code and carry on". The deployment sets no such marker, so the branch that ran was the developer one: every recipient and every usable sign-in code printed to a retained log, the screen reporting success, and nobody receiving anything. That is what a fail-open default costs. The deployment that forgets its mail settings is exactly the deployment with no marker saying it is a real one, so it takes the lenient branch precisely when it should not. Turned around: printing a live code is asked for by name and anything else is an error, so absence of configuration is a refusal instead of an assumption. Two settings out of three is also an error now, because it means somebody is halfway through wiring a provider up and a quiet fallback would hide the missing half. Stages anyone else can reach are checked at deploy time, so a missing setting stops the deploy with the name of the variable it wanted rather than surfacing later as a person waiting for mail that never comes. |
||
|
Wanjohi
|
affe1e3c73 |
refactor(auth): serve one provider, and make it the email one
Signing in with a gaming account or with an SSH key could both bring a user into existence. That makes an account only as recoverable as the thing that created it, and gives one person as many accounts as they have gaming logins — neither of which is what an account is supposed to be now that verifying an address is what creates one. Both are unwired rather than deleted. The provider implementations stay where they are, because connecting a gaming account is still something this product does; it just does it from the API, against a user who already exists, which is a connection hanging off an identity rather than an identity of its own. The worker test followed: it exercised the two flows that are gone, and now covers the one that is left plus an assertion that the other two are not routed, so they cannot come back quietly. |
||
|
Wanjohi
|
bd163392ca |
fix(core): declare the claim column the migration adds
The migration adds session.claim_token, but neither the schema nor the
snapshot knew about it. Nothing breaks today because the two agree with
each other; it breaks the moment someone declares the field, because
generate then diffs against a snapshot without it and emits
ALTER TABLE "session" ADD COLUMN "claim_token" text;
which fails on every database the migration has already run against.
Declared with no writer yet, so the schema, the snapshot and the
database say the same thing.
|
||
|
Wanjohi
|
1b61d2251e |
fix(core): hold the connection cap on the path a settings screen uses
Connecting a Steam account wrote the row itself, so the limit on how many one person may connect was enforced on the sign-in path and nowhere else — and this is the path the settings screen calls, which makes it the one that would have gone over. It now resolves who is asking and hands over to the single place the rule lives. Two things fall out of that. A Steam account already connected to somebody else is a conflict rather than a silent success returning the other person's row id, and a Steam id of the wrong shape is refused before a lookup. |
||
|
Wanjohi
|
96b0cf8111 |
feat(auth): sign in with an email address
Wires the pin-code provider, which existed and was never reachable, and makes it the only branch that can create an account. Steam now resolves an existing connection instead of minting a user from a persona, and refuses when there is no account behind it — which is an answer the interface renders rather than an implicit signup. Delivery is a small provider-neutral POST rather than a vendor SDK: configure an endpoint, a key and a from address. With none of them set it logs the code outside production so a local sign-in works, and throws in production, because a screen that says "check your email" when nothing was sent leaves someone waiting instead of telling anybody. A person who has only ever signed in by email has no connected account, and the token says so with an empty value — the same one a server-to-server caller has always carried. |
||
|
Wanjohi
|
2a1b7abe9a |
feat(auth): serve the device authorization grant
A program with no browser — the desktop app — had a client for RFC 8628 and nothing to point it at. This serves the other half: a device authorization request that hands back a code, a page a person enters that code on, and a token endpoint that answers the poll. Both of the paths the client already implements are now reachable. Polling faster than the advertised interval gets slow_down, and each warning widens the interval so ignoring one costs more than the last; refusing gets access_denied, so a request nobody started stops instead of being polled until it ages out. The interval is capped, because it only ever grows and a code has to stay pollable for the whole of its life. The codes live in the same storage as the other short-lived grants rather than in a table, since that is what they are. User codes are drawn from an alphabet with no vowels and no look-alike pairs, and are accepted back in whatever case and spacing a person retyped them in. |
||
|
Wanjohi
|
1e81a8f92d |
feat(core): make one address one account, on rows that never had one
Runs against a database where every user was created by a gaming sign-in, so most rows have no email at all and nothing has ever stopped two rows from sharing one. The address is normalized first, duplicates are separated before the unique index exists — the older row keeps the address, the newer one is asked for a new one and loses nothing else — and the index is partial so that accounts with no address do not collide with each other. Verified against a database built to contain the awkward rows rather than against an empty schema, by the script alongside it: an account with no address, one with both, one with two connections, a duplicated address in two different cases, an account already over the connection cap, and a deleted row holding an address a live row also holds. Removing the de-duplication makes the index creation fail, which is how we know the fixtures are load-bearing. Also adds a nullable column recording which attempt holds a session run. It is not part of the change above and carries no reason of its own; the endpoint that reads and writes it arrives separately, and it is here because a schema change has one owner at a time. |
||
|
Wanjohi
|
da65cca4f2 |
feat(core): an account is an email address, and Steam is a connection
Signing in with Steam used to create the account. That made a second Steam account a second person, and it made losing a Steam account lose everything attached to it — the boxes, the team, the billing history. Invert it. A user comes into existence by verifying an email address and nothing else; a Steam account hangs off a user that already exists, capped at four. Signing in with Steam resolves an account and refuses when there is none, so the accounts made before this keep working — they already have the connection this looks for — while nothing new is created behind a persona. The cap lives here rather than in the schema because a unique index cannot count the rows sharing a foreign key. The email column gains a partial unique index instead, which is the constraint that can be expressed, and the address is trimmed and lower-cased at the edge so two spellings are not two accounts. |
||
|
Wanjohi
|
4eff67a11a | fix(core): point the placement marker at the decision that covers placement | ||
|
Wanjohi
|
ecebc528ae |
feat(api): the session endpoint, and a claim that only one caller can win (#317)
## What this is `POST /session`, `GET /session/:id`, and the three endpoints the host agent calls: `GET /machine/jobs`, `POST /session/:id/state`, `POST /session/:id/ticket`. Core had `Session` and `Box` and no HTTP surface at all; this adds the surface and the two things core was missing to support it safely. ### The access rule An agent may only see or touch a run whose box is placed on its own hardware, and that is a `where` clause on all three agent endpoints — not a check sitting next to the query, and not the agent asking politely for its own work. Host credentials are long-lived secrets on hardware in somebody's living room, so the blast radius of one leaking is decided by the join and nowhere else. `Session.listJobsForMachine`, `forMachine`, `compareAndSetState` and `publishTicket` all scope to the machine inside the statement. The compare-and-set is scoped there too, and not only by the read above it: a caller checking first and the write being scoped are different properties, and only one of them survives somebody adding a second caller. "No such run" and "not your run" are the same 403 with the same body, so reporting states at ids cannot be used to discover which ids exist. ### The claim `Session.setState` updated on the id alone. Two agents polling the same run both succeeded and both started the same box. There is one host today, which is exactly why that would have been built wrong and stayed wrong. `Session.compareAndSetState` puts the state being moved *out of* into the `where` clause, so the database picks the winner and the loser matches zero rows. `Session.transition` layers the classification on top and returns one of five outcomes, which the route maps: | Case | Answer | |---|---| | same host re-reports a state it already reported | `200`, nothing changes | | a different host reports anything | `403` | | a transition not in the table | `409`, row does not move | | a legal transition something else won first | `409` | | it happened | `200` | `setState` is left exactly as it was — it is the unscoped primitive the core tests already pin, and the new path is additive. ### Placement `POST /session` makes no placement decision. A box names its hardware, so a run inherits it by join, and the request body is `.strict()` so that naming a machine there is a validation error rather than a field quietly ignored. The interface went where boxes are actually placed: `Placement.Placer` in `packages/core/src/box/placement.ts`, with `Placement.onlyHost` as the implementation and `Box.createPlaced` as the one caller. A real scheduler replaces that file and nothing else. ## The test failing first Tests were written and run against unmodified code. Verbatim, trimmed to the result lines (the full log is stack traces for the same failures): ``` (fail) POST /session > a request creates the job, in the envelope both ends read [199.00ms] (fail) POST /session > creating a session makes no placement decision [55.00ms] (fail) POST /session > a box somebody else owns is not there to run [106.00ms] (fail) POST /session > a box already running refuses a second run rather than picking one [51.00ms] (fail) POST /session > you can only play as an account you have linked [85.00ms] (fail) POST /session > a host cannot ask for a session on its owner’s behalf [54.00ms] (fail) POST /session > requesting a session requires a signed-in person [1.00ms] (fail) GET /session/:id > the owner reads their own run, ticket and all [47.00ms] (fail) GET /session/:id > somebody else’s run is not visible, and neither is its absence [97.00ms] (fail) GET /session/:id > reading a run requires a signed-in person [1.00ms] (fail) GET /machine/jobs > a host is handed the work for its own boxes, with the kind on the wire [50.00ms] (fail) GET /machine/jobs > a host never sees work for a box on other hardware [84.00ms] (fail) GET /machine/jobs > bad credentials are indistinguishable from none [41.00ms] (fail) GET /machine/jobs > a person cannot poll for jobs [44.00ms] (fail) POST /session/:id/state > the claim moves the row, and the job stops being offered [41.00ms] (fail) POST /session/:id/state > the same host re-reporting a state it already reported is fine [54.00ms] (fail) POST /session/:id/state > a different host reporting anything is refused, and learns nothing [99.00ms] (fail) POST /session/:id/state > a transition that is not allowed is a conflict, and the row stays put [47.00ms] (fail) POST /session/:id/state > a stopped run cannot be started again [47.00ms] (fail) POST /session/:id/state > a duplicate live report does not extend a run somebody is billed for [44.00ms] (fail) POST /session/:id/state > a state nobody defined is a validation error, not a conflict [49.00ms] (fail) POST /session/:id/state > a person cannot report a state on their own session [48.00ms] (fail) POST /session/:id/ticket > a later ticket replaces the first, because it is a better address [44.00ms] (fail) POST /session/:id/ticket > a different host cannot publish an address for someone else’s run [75.00ms] (fail) POST /session/:id/ticket > a stopped run has no address to publish [48.00ms] (fail) POST /session/:id/ticket > a ticket has to say something [47.00ms] (fail) Session routes in the spec > every path a caller needs is documented [48.00ms] error: Cannot find module './placement.js' from '/home/…/packages/core/src/box/placement.test.ts' (fail) Session jobs > a requested session is the job, and it carries its kind [37.00ms] (fail) Session jobs > a job belongs to the machine its box is placed on and to no other [71.00ms] (fail) Session jobs > only a requested session is work; a claimed one is not offered again [34.00ms] (fail) Session claim > the claim is a compare-and-set, so the second attempt finds nothing to move [37.00ms] (fail) Session claim > a machine that is not the box’s host cannot move the row [72.00ms] (fail) Session claim > a session that does not exist is refused the same way as one that is not yours [31.00ms] (fail) Session claim > re-reporting the state you already reported changes nothing [37.00ms] (fail) Session claim > a transition off the table is refused and the row does not move [36.00ms] (fail) Session claim > the timestamps survive a duplicate report, which is what billing rests on [33.00ms] (fail) Session claim > publishing a ticket is scoped to the host too [73.00ms] (fail) Session claim > a stopped session has no address to publish [36.00ms] 7 pass 39 fail 1 error Ran 46 tests across 3 files. [2.88s] ``` (46 rather than 49 because the three `Placement` tests never ran — the module they import did not exist.) ## And passing after ``` 49 pass 0 fail 126 expect() calls Ran 49 tests across 3 files. [3.72s] ``` Full suite, against a fresh migrated database on `DATABASE_URL` and `TEST_DATABASE_URL`: ``` 181 pass 0 fail 479 expect() calls Ran 181 tests across 16 files. [7.89s] ``` Baseline before this branch was `138 pass, 0 fail, 371 expect() calls` across 14 files, so 43 tests and 108 assertions are new and nothing regressed. `tsc --noEmit -p apps/api` reports the same five pre-existing errors in `app/utils/hook.ts` and `app/utils/validator.ts` as it does on `dev`, and none in the files this branch adds. ## Shared files touched - `apps/api/app/index.ts` — three lines: one import, and two mounts (`/session`, plus `SessionApi.machineRoute` at `/machine`). - `packages/core/src/box/index.ts` — one import and `Box.createPlaced` added. Nothing existing changed. - `packages/core/src/session/session.test.ts` — `scene()` now also returns `machineId`; two new `describe` blocks appended. - `packages/core/src/examples.ts` was **not** touched; the job schema's examples reuse the existing `Examples.Session`, `Examples.Box` and `Examples.Game` values. No migration was created and none was needed. ## Judgement calls 1. **`GET /machine/jobs` lives in `session.ts`, mounted at `/machine`.** The path belongs under the prefix a host already uses for everything it asks about itself, but the handler is this lane's code, so it is a second Hono instance (`SessionApi.machineRoute`) rather than an edit to `machine.ts`. 2. **A job's payload beyond `kind` was not specified anywhere**, so it is: `kind`, `sessionId`, `boxId`, `boxTier`, `gameId`, `steamAppId`, `linkedAccountId`. `steamAppId` is in there because the agent launches by store id and not by our internal one, and `boxTier` because the tier also sets output geometry. **This is the most likely place for the two implementations to disagree** — see the disagreement note below. 3. **A run's terminal states refuse a ticket** (`409`). Nothing said what publishing an address for a stopped run should do; writing one can only mislead a client that is still polling. 4. **A state report accepts only `starting`, `live`, `ended`, `failed`.** `requested` is written once, at creation, so reporting it is a `400` rather than a no-op. 5. **A box already running refuses a second run** with `409`. Not stated in so many words, but `Session.activeForBox` documents itself as the query that should start refusing rather than picking a winner silently, and this is the caller that would otherwise have made it pick. 6. **`Placement.onlyHost` refuses when there is more than one candidate** rather than taking the first row. Picking would be a scheduling policy invented by accident and impossible to find later. It also refuses when there are none, because `box.machineId` is not nullable and a placer that cannot answer must say so. 7. **`POST /session` returns `201`**, and its body is `.strict()`. 8. **Person-facing 404s, agent-facing 403s.** Both are the settled shape for their realm and both are asserted identical between "does not exist" and "not yours". 9. **`POST /session` accepts an explicit `linkedAccountId`**, defaulting to the one the caller's session carries. A personal access token carries none, so requiring the session to supply it would make that credential unable to start a run; the account is verified to belong to the caller either way. ## Where the specification was ambiguous or came out wrong - **The lost-claim 409 is not reachable through the HTTP endpoint as specified.** A box names exactly one machine, so both racers for a given run authenticate as *the same* machine — and the same machine re-reporting a state it already reported is specified as `200`. Which of the two answers a caller gets therefore depends only on whether its read happened before or after the winner's write: concurrent gets `409`, a sequential retry gets `200`. That is a defensible reading and it is what is implemented, but it means the two rules are distinguished by timing and not by anything a caller can see. If a second agent process per host is ever real, this needs a claim token in the request rather than a timing accident. - **The job payload is unspecified**, which is the one part of the wire the agent parses and the one thing a document written before two implementations exist was supposed to pin. If the other end's test expects different field names, that is this gap and not a bug in either implementation, and it should be settled in the specification before either side moves. - **Nothing said whether a person may report a state.** Implemented as `403`: terminal states are the agent's to write, and a person closing their app is a different fact from a run that stopped. That reading is also why cancellation, listed below, has nowhere to live yet. ## What this does not verify - **No live round trip.** Every assertion here goes through `app.request` against a real Postgres. No real host agent has ever called any of these five endpoints, and no client has ever read a ticket out of one. - **The claim is not verified under concurrency.** The compare-and-set is pinned by stepping two attempts in sequence and showing the second matches zero rows. Two agents actually racing, on two connections, is not tested and cannot be from a single-process test. - **The wire shape is asserted against this lane's reading of the specification, not against the other end.** That is deliberate — each end writes its own test — but it means a shape agreement has *not* been demonstrated. Both tests passing separately is the evidence, and it does not exist yet. - **No ticket in this repository has ever been a real iroh ticket.** They are opaque strings to every assertion here; that the value survives a round trip says nothing about whether anything could connect to it. - **`steamAppId` reaching the agent is untested end to end.** It is asserted present and correct in the poll response and nothing consumes it. - **Nothing reaps a stuck `requested`.** A run whose host never polls sits there forever, and no screen distinguishes that from "starting soon". Left open deliberately: a timeout here would hide the gap rather than close it. - **Cancellation has no path.** A person who closes their app between `requested` and `starting` leaves work that will be claimed and started into an empty room. There is no endpoint for it and this branch did not invent one. - **Nobody writes `ended` when the agent itself dies.** The only writer of terminal states is the agent, so a host that loses power leaves a `live` run that keeps billing. The fix belongs with whatever decides host liveness has authority over a run's state, and that does not exist. `Machine.isOnline` exists but nothing joins it to a session. - **Placement is verified only for the single-host case and the two refusals.** No test exercises a real alternative placer beyond an inline stub, and nothing calls `Box.createPlaced` from an endpoint, because there is no box creation endpoint yet. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR adds the HTTP and core-domain session lifecycle, including user session requests, machine job polling, host-scoped state transitions and ticket publication, placement support, box-state synchronization, and a database constraint ensuring one active session per box. - Adds authenticated user and machine session endpoints with resource-level ownership and host scoping. - Uses compare-and-set state transitions so only one caller can claim a requested session. - Adds a partial unique index and duplicate-data repair migration to enforce one active run per box. - Clears connection tickets when sessions become terminal. - Adds placement abstractions and extensive API, domain, migration, and concurrency-oriented tests. - The latest changes document and test that ownership is checked per user rather than per selected linked account; they do not resolve the previously reported multi-account launch failure. <h3>Confidence Score: 4/5</h3> The PR is not yet safe to merge because selecting a different linked account can still launch a game that only another account owns. The previously reported ownership issue remains unresolved: the current request path checks the game against the user-wide library and separately verifies only that the selected linked account belongs to that user. The new test explicitly accepts this behavior, so a user with multiple Steam links can request a game through an account that does not own it, causing the host launch to fail. The three resolved previous findings are fully addressed or manually resolved and do not remain outstanding. **Files Needing Attention:** apps/api/app/routes/session.ts <h3>Important Files Changed</h3> | Filename | Overview | |----------|----------| | apps/api/app/routes/session.ts | Adds user and host session endpoints with scoped authorization and validation, but the previously reported selected-account ownership mismatch remains. | | packages/core/src/session/index.ts | Adds job queries, host-scoped compare-and-set transitions, ticket lifecycle handling, box-state synchronization, and active-session conflict translation. | | packages/core/src/session/session.sql.ts | Declares the partial unique index enforcing at most one undeleted, unstopped session per box. | | packages/core/migrations/0008_session_one_active_run_per_box.sql | Repairs pre-existing duplicate active sessions, clears their stale tickets, and creates the active-session unique index. | | packages/core/src/box/placement.ts | Introduces a placement seam that selects the sole owner host and refuses ambiguous or unavailable placement. | | apps/api/test/session.test.ts | Adds broad endpoint coverage and now explicitly demonstrates the unresolved per-user rather than per-linked-account ownership behavior. | <h3>Sequence Diagram</h3> ```mermaid sequenceDiagram participant U as User client participant A as Session API participant D as Core domain participant DB as PostgreSQL participant H as Host agent U->>A: POST /session A->>D: Validate box, game, library, and linked account D->>DB: Insert requested session DB-->>D: Enforce one active session per box H->>A: GET /machine/jobs A->>D: List requested sessions scoped to host D->>DB: Join session through box.machineId H->>A: POST /session/:id/state (starting) A->>D: Compare-and-set state for host D->>DB: Atomic scoped transition H->>A: POST /session/:id/ticket A->>D: Publish ticket while addressable U->>A: GET /session/:id A-->>U: Current state and ticket H->>A: POST /session/:id/state (ended/failed) D->>DB: Set terminal state and clear ticket ``` <sub>Reviews (4): Last reviewed commit: ["fix(api): say what the library check act..."](https://github.com/nestrilabs/nestri/commit/7bdca1240f3e3644e87686a4be3546374c461800) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=60463182)</sub> <details><summary><h4>Context used (3)</h4></summary> - Knowledge Base — [API HTTP composition and authorization](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/api-http-and-auth.md) - Knowledge Base — [Core domain and persistence](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-domain-data.md) - Knowledge Base — [Users, identity, and game libraries](https://app.greptile.com/nestri/-/custom-context/knowledge-base/nestrilabs/nestri/-/docs/core-identity-and-library.md) </details> <!-- /greptile_comment --> |
||
|
Wanjohi
|
7bdca1240f |
fix(api): say what the library check actually proves
A library entry records the person, not the account the games were synced from, and `POST /library/sync` is not even told which account a list came from. So the ownership check added for session requests asks "has somebody this person linked got this game?" and not "does the account about to play own it?" — for the one Steam account most people have those are the same sentence, and for two they are not. Confirmed rather than reasoned about: a person with two Steam links, a game synced at person level, and a request naming the second account is accepted today. The check stays, because it still turns a box that boots, tries to launch and fails minutes later into an immediate refusal, and it never refuses on account grounds that the data cannot support. What changes is the comment, which claimed the stronger property, and a test that pins the gap so it is found deliberately rather than by surprise. Closing it properly means recording the linked account on a library entry: a column, a sync contract that says which account a list belongs to, a uniqueness rule per account rather than per person, and a backfill with no correct answer for rows already written. That is a decision about what a library is, and inferring it here would be the kind of modelling taken by accident that this branch refuses elsewhere. |
||
|
Wanjohi
|
51dabddbd8 |
fix(api): a run drives the box under it, and needs a game and a claim
Four things the session endpoints did not do, or did wrongly. The box had three states and nothing wrote them. A box read `created` while a run on it was `live`, so every screen showing a person what their hardware is doing was reading a column no code had ever moved. A run reaching `live` now makes its box `running`, and a terminal run stops it: `ended` cleanly, `failed` not, carrying the reason the agent gave. Not every run state maps — a box has no `starting` on purpose, because that transition is synchronous from the agent's side and a state nobody sets is a state that lies. Both writes are one transaction, since "this run is live" and "the box under it is running" are one fact in two tables, and a box stuck `running` with nothing on it has nothing to correct it. `POST /session` accepted any game in the catalog. A run launches as a Steam account that has to own the game, so one outside the caller's library is a box that starts, tries to launch and fails minutes later with nothing to point at; it is now refused up front. Told apart from a game that does not exist rather than hidden, because the catalog is public and "you do not own this" is a sentence a person can act on. The library is a synced copy, so this refuses a game bought since the last sync — that is a staleness bug in the sync, not a reason to start runs that cannot work. Publishing a ticket only refused terminal runs, so a host could publish an address for a run it had never claimed. A ticket is the address of something being brought up, so only `starting` and `live` accept one, and the state is in the write rather than only in the check above it. The two refusals stay separate answers because they are different mistakes: one agent skipped a step, the other has nothing left to reach. The migration that adds the one-active-run index stopped older duplicate runs without clearing the ticket they had published, which is the invariant that same migration exists to establish. It clears it now, verified against a box carrying two unstopped runs. Nine tests, each checked against the unfixed code first. |
||
|
Wanjohi
|
0d8630379b |
fix(api): a box gets one run, and a stopped run keeps no address
Two invariants the session endpoint stated but did not hold. A box runs one thing at a time. `POST /session` read `activeForBox` and refused when something was already running, but the read and the insert are two statements with nothing between them: two requests that both saw "nothing is running" each got a row, and the job poll then handed the host the same box to start twice. Demonstrated at 2 rows and 2 jobs from one box. That is the failure the state claim exists to prevent, one step earlier, and it takes the same answer — a partial unique index on the predicate the read asks about, so the database refuses the second insert. `Session.request` turns that refusal into the same 409 in the same words, so a caller cannot tell which of the two caught it. The migration resolves any existing duplicates before creating the index, keeping each box's newest unstopped run because that is the one a person is waiting on, and stopping the rest rather than deleting them. Separately, a run that reached `ended` or `failed` kept the last ticket it published. Publishing a new one is already refused, so the stale address was both the only ticket a client could read for a dead run and the one nothing was allowed to replace — and a client that polls would dial it. Terminal transitions now clear it, in `setState` as well as in the compare-and-set, so the invariant does not depend on which writer stopped the run. Seven tests, each checked against the unfixed code first. The published descriptions for the ticket field and the read endpoint now say that a stopped run has no address. |
||
|
Wanjohi
|
bbe729e5c7 |
feat(api): the session endpoint, and a claim that only one caller can win
A run of a box had core support and no HTTP surface. This adds both halves of it: a person asks for a run and reads it back, and the host agent the box is placed on is handed the work and reports what happened. The access rule is the point. An agent may only see or touch a run whose box is placed on its own hardware, and that is a `where` clause on every one of the three agent endpoints rather than a check next to them — host credentials are long-lived secrets sitting on hardware in somebody's home, so what one leaking can reach has to be decided by the query. "No such run" and "not your run" are the same refusal, so ids cannot be discovered by reporting states at them. `Session.setState` updated on the id alone, which means two agents polling the same work both succeed and both start the same box. There is one host today, which is exactly why that would have been built wrong and stayed wrong. The state a run is moving out of is now part of the `where` clause, so the database picks the winner; the loser gets a conflict rather than a silent no-op. Three cases that look alike are kept apart: re-reporting a state you already reported changes nothing and is not an error, a transition that does not exist is refused with the run left where it was, and another host reporting anything is forbidden. Asking for a run makes no decision about where it happens — a box already names its hardware, so the run inherits it by join. Placement therefore gets an interface at box creation, where the decision actually is, with the single-host case as its implementation and a deliberate refusal when there is more than one candidate and no policy to choose with. Tests cover the wire shape from both sides, the query scoping, the claim, and the timestamp idempotence a run's billing rests on. |
||
|
Wanjohi
|
aaa1bbd0f4 |
fix(nesdoctor): the closing prose still said "launch times"
Missed when the histogram was corrected: the tool spent a paragraph asking people to send the JSON and described it as holding "installed titles with sizes and launch times", which is the same claim the histogram itself no longer makes. Steam stores when a title was last played, not each time it was launched. Found by running the installer end to end after the release — the shipped 0.3.0 binary prints it. Fixed here for the next one; it is prose in the closing paragraph, not a number anybody acted on. Same wording in the --json flag's doc comment, in the submit URL's comment, and in the consent prompt that asks to read the library at all. That last one matters most of the four: it is what somebody reads before saying yes. |
||
|
Wanjohi
|
d8e5c19181 |
release(nesdoctor): point the installers at v0.3.0
Flipped after publishing, so there was never a window where the live installer named a tag that did not exist. Verified before committing: all five assets under the new tag return 200, and the installer was run end to end — it fetched the published binary, checked it against SHA256SUMS, and the thing it ran reported 0.3.0. |
||
|
Wanjohi
|
29a826d9f9 |
release(nesdoctor): 0.3.0
Minor rather than patch, because the output changed meaning and not just its numbers. `up=` was overstated by about a fifth and is now measured over the window the bytes were actually counted in, so a figure from 0.2.x and one from this release are not comparable. And the hour-of-day histogram was labelled launches when Steam only stores one timestamp per title, so the summary line now carries which sample a peak came from — `n=28/all` where it used to say `n=28`, which is a shape people paste. New wire keys hours30, n30, nspan, playh and peaksrc. hours, n and peak keep their names and meaning so the corpus stays continuous; a submission without peaksrc is whole-library by construction. |
||
|
Wanjohi
|
cf56aaf04c |
docs: this repo is public, so say what things are, not who decided them
Comments and served API descriptions here had grown references that only make sense to someone with our internal notes: relative paths that escape this tree, filenames and titles of documents nobody outside can open, quoted prose from them, and the name of a component that has no public surface — once in an OpenAPI description, which is published output rather than source. None of it was load-bearing. Every case restates as what the code actually requires, and every rewrite came out shorter: "in the words the host agent reports" for a component name, "republished as addresses are discovered" for a quoted phrase, "a size tier sets vCPU, RAM and the output geometry" for a sentence that had been carrying a path. Internal reasoning is now cited exactly one way, ref(d-NNNN) in a source comment, with the rule that the sentence must still stand if the marker is deleted. CLAUDE.md leads with it, because the previous version of this mistake was made by people who knew the repo was public and it still took ten occurrences to notice, so "be careful" is not a mechanism. Commit messages get the stricter rule and carry no references at all: a comment can be fixed by the next commit and a published message cannot be fixed at all. Git hooks now enforce both halves. The check caught a real one while being written: the CLAUDE.md table spelled out the paths it was prohibiting, which discloses them to exactly the reader it protects against. 138 tests, 0 fail. |
||
|
Wanjohi
|
c3682136f1 |
test(api): hold the heartbeat wire contract from the host's side
`lastSeen` and `intervalSeconds` are the two field names neslet's plane.rs reads out of the reply, and a rename on either side yields a host that beats, parses nothing and reports success. These tests are what make that a contract rather than a coincidence. Also asserts the property the middleware comment claims and nothing checked: wrong machine credentials and no credentials produce *identical* responses, because bad credentials fall through to `public` rather than erroring so that probing cannot reveal which machine ids exist. Comparing the two bodies is the only way that stays true. Two of these tests started out asserting 401 and were wrong, not the code — `machineOnly` sees a public actor either way and forbids. 138 tests, 0 fail. |
||
|
Wanjohi
|
4315510de8 |
feat(api): a host can say it is alive, and is told how often to
Second half of G1's "neslet registers against api.nestri.io and heartbeats".
Registration already worked; there was no heartbeat endpoint at all — grep for
it across apps/api and packages/core returned nothing, and neslet's own
main.rs says the same from its side.
POST /machine/heartbeat, machine credentials only. Two decisions worth stating
because neither is obvious from the diff:
**It returns the interval.** The auth middleware already touches lastSeen on
every authenticated machine request, so an endpoint that only did that would
add an endpoint and no capability. What a host cannot know on its own is how
often the control plane wants to hear from it, so the response carries the
cadence. A fleet whose interval can only change by shipping a new agent is a
fleet whose interval never changes.
**It takes no body.** neslet has a HostSummary ready to send, and week 2 owns
box state reporting. Accepting fields nothing acts on yet would mean a wire
shape we would have to keep, chosen before the thing that consumes it exists.
Online-ness is derived from lastSeen rather than stored: a host that stops
beating goes offline through the passage of time, which is the one mechanism
that cannot itself fail. Three missed beats, not one — a single missed beat is
a lost packet, and treating that as offline would make placement flap.
Also: the machine actor's teamID stops being optional. It was `...(teamId ? {}
: {})` in the middleware, a branch for a state that cannot exist now that
machine.team_id is notNull.
134 tests, 0 fail.
|
||
|
Wanjohi
|
6c1d407985 |
feat(core): a box is a row, a session is the billing unit
Migration 1 of 0048, and the first of the seven weeks — nothing about a live
feed works without these two tables, so it is not a cleanup during them.
box a VM someone owns: an id that is also its DNS label, an editable
label, an owning user, the machine it sits on, a tier and a state.
Owned by a person and placed on a team's hardware, which are two
different relationships, hence both userId and machineId.
session one run of one box by one linked Steam account, and what costs
money. Separate from box because the ticket changes after bind as
addresses are discovered — the vsock contract calls it "a stream,
not one value" — so it is a column a client polls, not a value it
is handed once.
Box states are neslet's own three and no more. `starting` and `stopping` are
the obvious additions and both are omitted because nothing would ever write
them; a failed box is `stopped` with stopClean false, which is how neslet
models it too.
The generated migration would have failed on live rows in three ways, so it
is hand-written and tested against a database seeded at the old schema:
- machine.team_id becomes notNull, and *every existing row is null* because
the old registration path passed null. Personal teams are backfilled for
machine owners first, reusing a team they already own rather than minting
a second, with the owner membership row repaired where missing.
- game_download.host_id becomes a foreign key. It held free-form strings,
so unattributable rows are deleted before the cast — the only destructive
statement here, and a considered loss: it is a progress report neslet
re-derives from disk.
- Team.createPersonal was written and documented in packages/core/CLAUDE.md
as part of the login flow and never actually called, so no user has a
team. ensurePersonal is idempotent and now runs on every login, which is
what backfills accounts the migration does not reach.
Verified on a seeded legacy database: three null-team machines backfilled, an
existing team reused rather than duplicated, a blank display name handled, and
both unattributable download rows dropped while the attributable one survived.
Also fixes two things this work ran into rather than caused:
- Database.client() built a new postgres pool on every call, and use()
called it twice per invocation — pools of ten connections held for a 30s
idle timeout. Invisible in a Worker where requests are short; the suite
crossed 100 connections and Postgres said "sorry, too many clients
already" in whichever file ran last, which reads as a flaky test rather
than a leak. Now one pool per connection string.
- download.test.ts asserted against `hst_…` host ids, which is exactly the
unattributable row the new foreign key exists to refuse.
There is no "no team" any more: PATCH /machine/:id took teamId null to mean
"mine alone" and now requires a team, because the personal team is the one to
name. Its test is updated to the new contract rather than deleted.
113 → 128 tests, 0 fail.
|
||
|
Wanjohi
|
1bfdfcf3cf |
fix(nesdoctor): "launch records" were never launches
Steam keeps one LastPlayed per title, so the hour-of-day histogram holds one sample per *title* — at the hour it was last closed, over the whole life of the library. It was labelled and reported as a launch histogram, and the module header claimed a library of eighty games is "eighty samples of what hour this person launches a game at — a real distribution". It is not. The bias has a direction, and everything pushes the same way: a title played once years ago weighs exactly as much as a daily driver, a daily driver contributes one sample ever, and an afternoon spent installing and trying a dozen games stamps a dozen titles with that afternoon's hour. So the metric over-weights trying and under-weights playing. On the production host it reads n=331 with 329 titles no longer installed; on this dev machine, 28 titles spanning 570 days. Corrected rather than disclaimed, because the direction is knowable: - Fields say what they hold — last_played_hours, titles_sampled, and no "launch" anywhere. The display says "when you last played each game — 28 titles, local time, reaching back 19 months". - A second histogram over titles played in the last 30 days, which is one sample per title still in use, and the peak window prefers it when it has the samples to claim a shape. - Which histogram the peak came from is stated in the output and on the wire (peaksrc=30d|all), and the summary line carries the sample count the window was actually computed from, so a narrow peak drawn from nine titles cannot borrow the authority of three hundred. - New keys hours30, n30, nspan, playh. hours/n/peak keep their names and meaning so the corpus stays continuous; submissions without peaksrc are whole-library by construction. Playtime is read and reported but deliberately not used as a weight: it is a lifetime total against a single timestamp, so weighting by it would multiply one arbitrary hour by five hundred. Five tests, including the wrapping midnight window, which is the case a non-wrapping scan gets wrong and exactly the evening peak 0017 is about. |
||
|
Wanjohi
|
3dba825f17 |
fix(nesdoctor): up= was overstated by about a fifth
The throughput window and the byte count disagreed. Bytes were counted from the moment the upload threads started, the 1.5 s queue-fill ramp included; the divisor was that same span with 1.5 s subtracted from it. So a numerator covering ~8.3 s was divided by ~6.8 s, and every up= figure nesdoctor has ever published is high by ~22%. Snapshot the counter and the clock together after the ramp, and measure both from there. Excluding the ramp is also the better measurement: TCP slow-start lives in it, so it is not the steady state a session gets. Found by running speedtest on the same line in the same afternoon — 284 Mbps against our 502 — which is the only way it could have been found. The code was self-consistent and the number it printed was plausible, so no amount of re-reading would have shown it. A boundary effect remains and is documented in the code rather than papered over: bytes arrive one completed 8 MiB POST at a time, so up= keeps a few per cent of upward slack. Submissions collected to date stay useful as a floor and as a bufferbloat corpus. They are not usable as throughput. |
||
|
Wanjohi
|
90bd93f47f |
docs(nesdoctor): Windows will block it, and here is why and what to press
Reported from a real machine on release day. The README asks strangers to run a binary, so it should say what actually happens when they try. The why matters more than the workaround: SmartScreen objects to the file being unsigned and having no download history, not to anything the program does. And history attaches to the file hash, so a project releasing four times in an afternoon never accumulates any -- waiting is not a strategy. Offers the source build as the version that requires no trust, and says that stopping is a reasonable choice. Reproducible CI builds and published checksums prove provenance without moving SmartScreen an inch, and pretending otherwise would be the kind of overclaim this tool cannot afford. |
||
|
Wanjohi
|
f0227201db |
release(nesdoctor): point the installers at v0.2.2
Flipped after publishing, so there was never a window where the live installer named a tag that did not exist. |
||
|
Wanjohi
|
dc99bd2743 |
fix(nesdoctor): the Apple Silicon GPU name had doubled parentheses
The fallback worked -- the macOS runner now reports a GPU instead of `unknown`, and the raw probe dump settled which of the two candidate causes it was: a headless virtual Mac with no display adapter to enumerate, so `system_profiler` had nothing and the parser was never at fault. It read `Apple M1 (Virtual) (integrated)`, because the SoC name already carries a parenthetical on a VM. Em-dash instead. The suffix stays: it records that the name came from the chip rather than from a display adapter, which is the difference between a machine with no GPU and a machine with no display. |
||
|
Wanjohi
|
730739a5c0 |
fix(nesdoctor): fall back to the SoC name on Apple Silicon, and print raw probes
The macOS arm added in the previous commit did not change anything -- the runner still reported `gpu=unknown`. Checked rather than assumed, which is the only reason it is known. Two possible causes and no way to choose between them from here: either the `system_profiler SPDisplaysDataType` parsing is wrong, or that machine is a headless virtual Mac with no display adapter to enumerate at all, in which case `unknown` was the correct answer and there is nothing to fix. The second is likely and the first is not ruled out. So, rather than guessing again: on an arm64 Mac the GPU *is* the SoC, so the chip name is a true and useful answer even with no display attached. `sysctl -n machdep.cpu.brand_string` works headless and yields "Apple M1 (integrated)". Intel Macs get no fallback, because there the GPU may be integrated or discrete and a guess would be wrong rather than coarse. And the CI step now dumps the **raw** output of each platform's probes -- `system_profiler`, `Get-CimInstance Win32_VideoController`, `Get-PSDrive`, `df -Pk`, `/sys/class/drm` -- into its own log group. A field that comes back empty can then be told apart from a parser that is wrong, which is exactly the distinction that cost this round trip. All of it is `|| true`: the step exists for looking, and a probe that misbehaves on a runner must never fail a release. |
||
|
Wanjohi
|
0f26df5c02 |
fix(nesdoctor): every Mac reported gpu=unknown, because the probe had no macOS arm
Seen in the macOS CI log: nesdoctor 0.2.2 | macos/aarch64 | gpu=unknown | ... `gpus()` had a Linux arm, a Windows arm, and `Vec::new()` for everything else. Macs are clients rather than hosts, so it went unnoticed -- but 0041 wants a client vendor matrix and an unlabelled row is no use in one. An M-series integrated GPU and a discrete Radeon in an Intel Mac decode very differently, and "unknown" cannot tell them apart. `system_profiler SPDisplaysDataType` is the only place the chipset name lives. Parsed loosely: the format has changed between macOS releases, so a name we cannot find costs a field rather than the run. Vendor is matched over Apple, AMD, Radeon, NVIDIA and Intel; `render_node` stays `None` because macOS has none and a Mac cannot host regardless. Still missing on macOS and stated rather than papered over: filesystem types and the display probe. The EDID path is sysfs on Linux and WMI on Windows, and macOS exposes neither -- so Mac respondents report no colour depth or HDR capability. That is a real gap for the video work, since Mac panels are exactly the P3 and high-refresh cases worth knowing about, and it needs `CoreDisplay`/`system_profiler` parsing rather than a one-line fix. |
||
|
Wanjohi
|
53d69ca289 |
fix(nesdoctor): parse df from the right; a device name can contain a space
Three more things the macOS CI log showed, none of which had ever been visible
from this laptop.
`df -P` fixes the column order but not that the filesystem name is one word.
macOS emits
map auto_home 0 0 0 100% /System/Volumes/Data/home
which shifts every field by one, so indexing from the left read the capacity
percentage as part of the mount point and a device name as the size. The row
appeared in the log as `100% /System/Volumes/Data/home`, which is what gave it
away. Columns are now counted from the right, where `df` actually guarantees
them: size, used, avail, capacity, mount. A test covers the plain row, the
two-word `map auto_home` row, and an SMB share whose device name contains a
space -- the case that makes left-indexing wrong in principle rather than just
on Macs.
The `/System` filter I claimed to have added in the previous commit was not in
the file. The assertion that was supposed to catch that passed against the
wrong block, so it went in silently and `/System/Volumes/xarts` kept appearing
in the very output I had just quoted as fixed. It is there now, along with
`/private/var/vm` and `/Volumes/Recovery`, and verified by grep rather than by
belief.
And a filesystem reporting no capacity is not storage: `map auto_home`, devfs
and macOS signed asset bundles all report zero and were padding the filesystem
count in the summary line.
Net effect on the runner, across this commit and the last: 11 filesystems and
"483 GiB free of 1600 GiB" on a 320 GiB machine, down to the one real volume.
|
||
|
Wanjohi
|
f32db5393b |
fix(nesdoctor): APFS volumes share one pool, and were counted eleven times
Found by reading what the macOS CI runner prints, which is the whole reason that step was added an hour ago. First time anyone had looked at what these probes return on a platform that is not this laptop: / 96 GiB free of 320 GiB /System/Volumes/VM 96 GiB free of 320 GiB /System/Volumes/Preboot 96 GiB free of 320 GiB /System/Volumes/Update 96 GiB free of 320 GiB /System/Volumes/Data 96 GiB free of 320 GiB 11 filesystems · 483 GiB free of 1600 GiB total On a machine with 320 GiB. An APFS container presents each volume as its own filesystem with its own `/dev/diskNsM`, so the device-name dedupe -- which correctly collapses btrfs subvolumes -- cannot see that the space is shared. Two changes. `/System/Volumes` and `/private/var/vm` are skipped: they are not user storage, and on a Mac they are most of the rows. And filesystems are deduped by *pool* as well as by device. Two filesystems reporting byte-identical capacity and byte-identical free space are one store, whatever their device names say -- which also covers bind mounts and thin-provisioned LVM, neither of which the device check catches either. Two genuinely separate disks agreeing to the byte on both figures would cost one row; a storage total inflated fivefold is a number a capacity plan gets built on. Simulated against the exact runner output: eight rows and 2560 GiB become one row and 320 GiB. The Windows runner, by contrast, was correct first time -- `C:` and `D:` are genuinely separate and totalled 179 GiB free of 299 GiB. Worth recording that the reason we know is that we looked, rather than that we reasoned about it. |
||
|
Wanjohi
|
ac4bf666e6 |
docs(claude): nesdoctor is not guest-side, and cannot be tested from here
CLAUDE.md said "Rust components are guest-side: they run inside a virtual machine". That was true of every one of them until yesterday and is now false, which makes it worse than a gap -- it is instruction that would send someone looking for nesdoctor in the wrong half of the system. Records the two things about it that hold nowhere else in this tree. Its dependency list is part of its interface, because it is handed to strangers and asked to be trusted. And it cannot be verified on the development machine alone: both bugs it has shipped were Windows-only, found by users, in code paths Linux never executes -- so prefer a property test that runs everywhere over a platform check that runs nowhere, and read what the Windows and macOS runners print. |
||
|
Wanjohi
|
786267f30a |
fix(nesdoctor): report storage properly, and stop being blind on Windows
A submission from a team machine with four drives and 22 TiB reported
`disk=8880`, and the field was not wrong so much as meaningless: it was the
free space on the single largest mount, with no capacity anywhere and no total.
A content store is sized against capacity.
Storage now reports four things, because they answer different questions and
one number could not:
diskfree total free across every real filesystem
disksize total capacity
diskmax the largest single filesystem, which is the real ceiling for any
one store -- a dataset cannot be spread across drives
disks how many there are
The ambiguous `disk` key is gone rather than silently redefined, so old rows
stay readable as what they were. `Get-PSDrive` reports Free *and* Used and we
were reading only Free, hence no capacity on Windows at all.
Pseudo-filesystems are now excluded by *type* rather than by mount path. Path
filtering missed `/tmp` on a tmpfs, whose free space is RAM -- so 7 GiB of
memory was being added to a storage total, which is exactly the sort of number
a capacity plan gets built on.
## The real finding, which was not about disks
"We are working blind on Windows" is correct, and both Windows bugs this tool
has had prove it: a virtual display adapter reported as the GPU, and a URL
truncated at its first `&`. Both were in code that only runs on Windows, both
were found by a person reading the results channel, and neither could have been
found here -- the development machine is Linux and `xdg-open` never sees a
shell.
Two things about that, and the first is the one that generalises.
`OPENERS` is now a const with a test asserting the property that actually
matters: **never hand a URL to anything that will re-parse it.** No `cmd`, no
`sh`, no `powershell`, no `start` builtin, and no argument that looks like it
wants the URL interpolated into it. Unlike the bug, that is checkable on every
platform in a millisecond. Verified by reintroducing `cmd /C start "" <url>`
and confirming the test fails with the right message, then reverting.
And CI already runs a real Windows machine and a real macOS one -- we simply
were not looking at them. Each smoke-tested target now prints its full report
and JSON into a collapsed log group. Deliberately not `set -e`: this step is
for looking, and a probe that misbehaves on a runner must not fail a release.
It turns "working blind" into "looking at it once per release", which would
have shown the Parsec adapter problem the first time a Windows binary was ever
built.
Version to 0.2.2.
|
||
|
Wanjohi
|
a84886861c |
release(nesdoctor): point the installers at v0.2.1
v0.2.0 and earlier lose Windows submissions entirely, so nothing should be installing them. |
||
|
Wanjohi
|
a7eeb14de5 |
fix(nesdoctor): cmd re-parsed the submit URL and destroyed every Windows result
Two submissions arrived carrying `v=0.2.0` and nothing else. Flagged from the
channel, not caught by us.
The Windows arm of `open_in_browser` was `cmd /C start "" <url>`. `cmd.exe`
re-parses its own command line and treats `&` as a command separator; Rust's
`Command` quotes arguments for the MSVC C runtime convention, which `cmd` does
not honour. So the URL was cut at its first `&` -- which in ours falls
immediately after `v=` -- and the browser opened
https://doctor.nestri.io/?v=0.2.0
carrying nothing whatsoever. Reproduced exactly with the same mechanism in a
POSIX shell: `sh -c 'echo <url>'` unquoted prints precisely that prefix.
Every Windows user who pressed Enter lost their entire report, and lost it
silently -- the page returned 200 and thanked them. Windows is most of this
audience, so most of the data we would ever have collected was going to
disappear this way.
Now `rundll32 url.dll,FileProtocolHandler`, which hands the URL to the shell's
protocol handler with no command interpreter anywhere in the path, so nothing
re-parses it. `explorer.exe` also opens URLs and was rejected: it returns a
non-zero exit status even on success, which would make the caller believe it
had failed and fall through.
The relay now also refuses to thank anyone for a version-only arrival, since an
older binary keeps producing them and a URL pasted into a shell unquoted does
the same thing.
Version to 0.2.1.
|
||
|
Wanjohi
|
19e1bf4152 |
docs: put nesdoctor in the README, near the top
The README opens by saying the repository is mid-rewrite, nothing is stable, and the documentation is behind the code -- all true, and it leaves a visitor with nothing to do. There is now one thing here that is finished and runs on its own machine, so it goes immediately after that note rather than buried in a component table. The section leads with the two install commands and the number worth having: added latency under load, which decides whether a stream feels right and which almost nobody has ever seen for their own connection. Then the display probe, because "it tells you what your monitor can actually accept" is a better hook for this audience than anything else in the file. It also says plainly that this does not stream a game and that most machines come back CLIENT, "a real answer, not a failure". A README that oversells the one runnable thing would undo the reason it is worth running. Three smaller corrections that follow: `nesdoctor` gets its own heading rather than a row under the guest components. It is neither control plane nor guest -- it runs on the reader's own machine, which is the whole point of it. Getting started said the Rust components "are not much use on their own yet". That was true of every one of them yesterday and is now wrong; scoped to the guest half, with `cargo run --release -p nesdoctor` called out as the exception. And Contributing now asks for the thing we actually need. "Tell us where the documentation failed you" was the most useful contribution when nothing could be run; running nesdoctor and sending the result is worth more, because we have almost no idea what the machines on the other end look like. Checked: every relative link in the file resolves, and the scripts doctor.nestri.io serves are byte-identical to the files the README points at -- which is the claim the section makes about them. |
||
|
Wanjohi
|
a244c9213c |
release(nesdoctor): point the installers at v0.2.0
Flipped after publishing, so there was never a window where the live installer named a tag that did not exist. |
||
|
Wanjohi
|
3544f857ff |
feat(nesdoctor): read the display, and offer early access
Two things, both of which every response collected without them is a response we cannot go back for -- since a submission carries nothing that identifies anyone, there is no second chance to ask. ## The display and decode probe This is the readable half of the client capability probe our build order already specifies -- GPU, decoder, display -- and its stated purpose is attribution: told only that a stream "looks bad", the cheapest available explanation is that our reconstruction ratio was too aggressive, so without this we would lower the ratio and pay density for somebody else's window manager. presentation path x11 · bspwm eDP-1 1920x1200 @ 60 Hz, 8-bit Vulkan decode h264, h265 VA-API decode h264, h265, vp9 Session type, compositor, and whether we are under XWayland -- which is exactly the objection raised against our own A/B rounds, now recorded automatically rather than argued about. A bare window manager sets none of the XDG variables, so bspwm and thirteen others are matched from the process list; a report that cannot name bspwm cannot answer the challenge that named it. From EDID, parsed here rather than shelled out to: native mode, refresh, colour bit depth, which HDR transfer functions the panel accepts, BT.2020 colorimetry, and 4:2:0 chroma. The CTA-861 extension blocks are where all the colour capability lives -- base EDID says nothing about any of it. That decides real choices. Whether 10-bit is worth sending, whether BT.2020 is worth encoding, which codec to reach for. Every one of those has so far been decided against the one panel in this room -- which this now reports as 8-bit, meaning the 10-bit work cannot be validated on it at all. EDID is untrusted binary from a device node. Every read is bounds-checked and every field optional: monitors ship broken EDIDs and docks synthesise worse ones, so a bad panel costs one field rather than the run. Three tests, one of which truncates the block mid-extension and asserts that no colour capability is invented. The colorimetry byte offset was wrong the first time and the test caught it, which is the argument for the test. Present mode, tearing and fractional scaling need a real window and swapchain, so they are absent and said to be absent rather than guessed. ## Early access An optional email, asked last, after the verdict has printed -- so nobody types an address before seeing what this said about their machine. Blank skips it. The offer branches on the verdict, because telling someone with no KVM and a grade-F uplink that we liked what their machine can do is a lie, and this program's only real asset is that it does not flatter anyone. A host-capable machine gets the host offer; everyone else gets early access as a player, which is a true offer too. It is the one identifying thing collected here, so: it appears in the pre-submit disclosure with everything else, and the promise elsewhere had to be reworded -- "no username, no identifiers" stopped being true the moment this field existed, and leaving the old line standing would have been the dishonest option. Validation is deliberately loose; arguing with somebody about their own address over a regex loses the response outright. Version to 0.2.0. |
||
|
Wanjohi
|
c2c3bb5ba0 |
feat(nesdoctor): ask about the other Linux box while we still can
Our first respondent answered `otherlinux=yes` -- they have a Linux machine -- and their Windows desktop came back CLIENT, which is a dead end. The Linux box is the result we have none of. There is no way to ask them. A submission carries no hostname, no address and no name, by design, so the moment the program exits whoever ran it is anonymous and unreachable. That is the correct trade and it is not being changed. What was wrong is that the program knew about the other machine *while they were still reading* and said nothing. So when someone answers that they have, or could set up, a Linux machine and this one cannot host, the run now ends by asking for it -- with the command, and with the reason stated plainly: nearly every result is a client, a host has to be Linux with KVM, and one run over there is worth more than a hundred of these. Including why we cannot follow up, since that is the honest argument for doing it now. The nudge is suppressed when the machine already qualifies as a host, because then it is noise. |
||
|
Wanjohi
|
b055e40546 |
fix(nesdoctor): the first Windows submission recorded a virtual display adapter
Our first response, and the GPU field is wrong: gpu=Parsec%20Virtual%20Display%20Adapter&gpus=2 Parsec installs an indirect display driver, it enumerated first out of `Win32_VideoController`, and the primary was taken as the first entry -- so the real card on that machine is gone. `gpus=2` is the only reason we can tell anything was lost, and it cannot tell us what. This is not an edge case for this audience. Parsec, Sunshine, Moonlight, TeamViewer and Splashtop all install one, and a cloud-gaming community is precisely the population that has one already. A recorded gpu_model is a hard requirement for a host; a virtual display driver satisfies it in name only. Three changes. Adapters are now sorted so real hardware is first, by two keys: whether the name matches a known software adapter, then whether a vendor could be identified at all. Order is the only signal the rest of the program has for which GPU is "the" GPU. The vendor comes from `AdapterCompatibility` rather than from pattern-matching the marketing name. An "AMD Radeon" string is easy; an OEM-rebadged one is not. And every adapter name is now sent, not only the count. `gpus=2` told us something had been lost and not what, which is the kind of field that wastes a response we cannot ask again. The known-software-adapter list has unit tests, on all platforms -- it is a list of strings and it will need extending, so it should fail loudly rather than quietly stop matching. Version to 0.1.2. For the record, what that submission got right, because none of it needed asking: 50% of 165 Steam launch records fall in five hours of twenty-four (21:00-01:59) against five records across the whole of 07:00-13:59. That is 0017's evening peak, measured, from one person's own files. 140 of the 165 records are titles no longer installed -- restricting the histogram to installed titles, as review suggested, would have left 25 samples and lost the shape entirely. |
||
|
Wanjohi
|
d6c5eafe9e |
release(nesdoctor): point the installers at v0.1.1
v0.1.1 is published, so the pin moves. Flipped after publishing rather than with the version bump, so there was never a moment where the live installer pointed at a tag that did not exist yet. v0.1.0's added-latency grade cannot be trusted -- it measured bloat against a median that goes unstable on a bimodally routed link -- so nothing should be installing it. |
||
|
Wanjohi
|
f0e65b9738 |
fix(nesdoctor): bloat was measured against the median, and could grade a bad line A
Found by running the published one-liner, which is the only reason it was
found: `up=34Mbps rtt=188ms rttload=181ms bloat=+0ms grade=A` on a connection
that measured +115 ms and grade F three hours earlier.
The idle baseline was the median of twelve handshakes to one anycast address.
On the development connection those twelve came back **bimodal**:
[56, 56, 57, 59, 60, 176, 177, 179, 179, 179, 182, 368]
min 56 p50 177 max 368 spread 312 ms on an *idle* link
Two points of presence answering. The median therefore lands wherever the split
happens to fall, and when it lands high the loaded median comes in *below* it,
the difference goes negative, `.max(0.0)` clamps it to zero, and the headline
number reports grade A.
That is the one error direction that cannot be tolerated here. A tool whose
whole pitch is a number nobody else shows you has no business saying "your line
is fine" about a line that is not.
Bloat is now measured against the **minimum**. Queueing is delay above the
floor the path can achieve, so the floor is the baseline -- which is also how
every bufferbloat test does it. Twenty samples rather than twelve.
The distance verdict deliberately keeps the **median**, because it asks a
different question. Bloat asks how much queueing is added, so its baseline is
the best case. HOST-READY-LOCAL asks what a player will actually see, so it
takes the typical case: on a link that is bimodal between 56 ms and 180 ms, the
floor would call it near when half of all connections are not.
Both are now reported, and the gap between them is itself the finding -- a
floor of 55 ms against a typical of 180 ms says the route is the problem, which
no single number could have said.
Verified on the same connection: floor 55, typical 180, loaded 95, **+39 ms,
grade C**, verdict HOST-NET. Defensible, and no longer flattering.
Version to 0.1.1. Submissions carry it, so any row with `v=0.1.0` has a grade
that cannot be trusted.
|
||
|
Wanjohi
|
166c1e9c24 |
fix(nesdoctor): pin the release tag; releases/latest is a time bomb here
Both installers fetched from `releases/latest/download`, which is wrong in this repository specifically: it ships product releases as well as this tool, so `latest` is whichever release went out most recently regardless of what it contains. Measured before publishing anything: `releases/latest` resolved to `v0.2.0`, from May 2024, and the asset URL 404'd. Publishing nesdoctor-v0.1.0 would have papered over it by becoming the newest release -- and then the first product release after it would have moved `latest` again and broken every `curl | sh` in the announcement, silently, for everyone, with the tool itself untouched and nothing to point at. Now pinned to a tag that is bumped when a nesdoctor release is cut, with `NESDOCTOR_TAG` still overriding for testing. The download failure message also now names the tag and says outright that an unpublished tag is the likely cause, since that is the one mistake this arrangement invites. |
||
|
Wanjohi
|
09b9472134 |
ci(nesdoctor): cross-compile the Intel Mac target, and tag to a draft
Two things a dispatch on 2026-09-02 exposed. `macos-13` is being retired and the x86_64-apple-darwin job sat queued indefinitely waiting for a runner, while the other three targets built and smoke-tested in under three minutes. A release should not have that as a dependency, so Intel macOS is now cross-compiled from the arm64 runner. The cost is real and is stated rather than hidden: an x86_64 binary cannot be executed on an arm64 runner without Rosetta, which these images do not carry, so it is the one target whose smoke test cannot run. The matrix carries an explicit `smoke` flag, the step is gated on it, and the generated release notes say which binary is unexercised. Fabricating a pass for it would have been easy and worse. And a tag now produces a **draft** release rather than a published one. The binaries get attached and the notes get written, then a person reads both and presses publish -- which is the only step in this pipeline that cannot be undone in public. For the record, from the successful three: the musl smoke test measured the network from inside the static binary -- `up=1635Mbps rtt=2ms bloat=+0ms grade=A` -- so `ring` and the platform verifier do resolve root certificates in a fully static build. That was the one thing about this release nobody could have known without running it. |
||
|
Wanjohi
|
d01e4a180e |
fix: nesdoctor.json was committed to a public repository (#312)
My mistake, in
|
||
|
Wanjohi
|
79f1732a14 |
feat(nesdoctor): a host readiness checker that measures instead of asking (#310)
The first executable form of our host requirements. Until now a machine
was qualified by a human reading a table of hard requirements — and a
requirement that nothing can check is one that is silently optional.
It also replaces a form. Everything we wanted from a prospective host is
measurable, and most of it **cannot be answered honestly by a human
anyway**: almost nobody knows their real upstream, and essentially
nobody has ever seen their own bufferbloat figure. What's left for the
questions is only what a machine cannot know — intent, and what someone
already pays.
## What it does
```
nesdoctor
```
- **Checks every hard requirement**: `/dev/kvm`, an AMD or Intel GPU
with a DRM render node, `VK_KHR_video_encode_queue` plus a codec,
`virglrenderer`, the two stores, the `io` cgroup controller,
`virtiofsd`. Pass / fail / **unknown**, and unknown is never collapsed
into fail — a machine we could not ask is not a machine that failed, and
losing a capable host to a missing `lspci` is the failure mode that
matters.
- **Measures upstream and, the point of the whole thing, added latency
under load.** Grade bands come from the frame budget rather than
convention: the network allowance is ~40 ms because render, encode,
decode, display and jitter buffer have already spent ~58 ms.
- **Reads Steam, only with an explicit yes**, for library size and shape
plus an hour-of-day histogram of launches — one sample per title, which
is a real distribution obtained without asking anybody anything.
- **Asks at most five questions**, branched on what was found, all
skippable.
## No server
Nothing is uploaded and no telemetry endpoint exists. The network test
talks to Cloudflare's public speed-test sink and to `1.1.1.1`, neither
of which is ours. The output is a line on the terminal that the person
may choose to paste.
The shareable line carries **no hostname, IP, username, game title or
path** — a size band rather than a size, hours rather than dates. The
long version, which does include titles and paths, stays in a local JSON
file the person is told the path of.
That is a property of the design and not a promise about our intentions:
there is nothing to switch on later.
```
nesdoctor 0.1.0 | linux/x86_64 | gpu=AMD Barcelo | cpu=12t ram=13G |
kvm=y venc=y zfs=n boxfs=n io=y | up=28Mbps rtt=179ms bloat=+19ms grade=B |
disk=91G | edge=KE/JNB | steam=1 titles/<100G | plays=20-03h n=74 |
role=- share=- pays=- | HOST-READY-LOCAL
```
## Five bugs found by running it, every one of which would have produced
wrong data
- **`vulkaninfo --summary` lists ZERO `VK_KHR_video` entries** where
full `vulkaninfo` lists five on the same machine. Preferring the summary
reported "not advertised" on a card that advertises it — a false
negative on the check most likely to disqualify a host.
- **btrfs subvolumes counted as separate disks**: `/`, `/home` and
`/srv` each reporting 91 GiB of one 91 GiB device. Now deduped by
backing device, which the two-stores check needs anyway since it wants
*separate devices*.
- **Proton and the Steam Linux Runtimes are installed like games and are
not games.** Five of eight entries on the test machine, so the title
count was 5× too high and the library-shape question was corrupted.
- **`--quiet` printed the whole questionnaire** before its summary line,
breaking the one thing `--quiet` promises. Prompts are now skipped when
output is quiet or stdin is not a terminal — and a pipe is explicitly
*not* treated as consent to read a Steam library, unlike `--yes`.
- Boot history was reporting `13.2 h/day` off **two days** of history.
Under a three-day span it now reports the span and no rate.
## One finding, now encoded as a verdict
The development connection measures **179 ms idle RTT, served from
Johannesburg**. That machine passes every other check and cannot host
for a European player, because it is distance and no upgrade shortens
it.
`HOST-READY-LOCAL` exists for exactly that case, and the wording is
deliberate:
> Every requirement passes and your uplink queues cleanly. But the idle
round trip to the nearest major network is already most of the latency
budget, and that is distance rather than a fault: no upgrade shortens
it. So this machine is a good host for people on your side of the world
and cannot be one for anybody else. **If you are somewhere without a
cloud gaming edge, that is not a consolation prize — it is the only way
anyone there gets a playable stream.**
## CI
- **`ci.yml` gains a `nesdoctor` job** — fmt, `clippy -D warnings`,
test, one real run. Scoped to this member deliberately: the rest of the
Rust half has never been under CI, so `--workspace` would turn every PR
red for unrelated reasons. Widen it one member at a time.
- **`release-nesdoctor.yml`** builds four targets on tag `nesdoctor-v*`
— x86_64 linux-musl, x86_64 windows-msvc, aarch64 and x86_64 macOS —
with `SHA256SUMS`. musl rather than glibc so one Linux binary runs on
every distro.
The step that justifies the workflow **runs the binary it just built,
network included**. `ring` under rustls resolves root certificates
through the host trust store, so a static musl build can compile cleanly
and then fail TLS on the machine it ships to — breaking the network
test, silently, and only for other people. The step fails the build if
the summary line comes back `net=unmeasured`.
## Dependencies
Four: `anyhow`, `clap`, `serde`, `ureq`. The VDF parser, every platform
probe and the text wrapping are in-tree. A binary handed to strangers
has a dependency tree that is part of its interface, so anything that
could be done with `std` is.
4 MB release binary.
## What it deliberately does not claim
- **A pass is not a promise.** Every check is a *necessary* condition,
and nothing here runs under load — a machine that passes can still fail
on block I/O.
- **The encode extension being advertised is not proof the path works.**
We have had a correct extension list over a broken path before, so that
row says so.
- **Whether `libvirglrenderer` carries the native-context patches cannot
be determined from outside**, so that row reports presence only and
stays `unknown` rather than `pass`.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
The PR adds the nesdoctor host-readiness executable, local Steam
analysis, network measurement, installers, CI validation, and
multi-platform release packaging. Two attempted correctness fixes remain
incomplete:
- physical disk deduplication does not resolve common device-mapper
source names before comparing backing devices
- unknown historical Steam appids can still be counted as game launches
without passing runtime filtering
<h3>Confidence Score: 3/5</h3>
The PR is not yet safe to merge because shared LVM-backed stores can be
reported as physically independent and unknown Steam tools can still be
reported as game launches.
The new disk resolver fails open for common device-mapper names,
preserving a false host-readiness verdict, while Steam history still
counts absent appids without determining whether they are games or
runtime tools.
**Files Needing Attention:** apps/nesdoctor/src/sys.rs,
apps/nesdoctor/src/hostreq.rs, apps/nesdoctor/src/steam.rs
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| apps/nesdoctor/src/sys.rs | Adds system and disk discovery, but
unresolved device-mapper names undermine physical-backing comparisons. |
| apps/nesdoctor/src/hostreq.rs | Implements host requirement verdicts
and uses physical-device sets that can falsely classify shared LVM
backing as independent. |
| apps/nesdoctor/src/steam.rs | Adds manifest and launch-history
analysis, but unknown appids bypass runtime classification and
contaminate launch metrics. |
| apps/nesdoctor/src/net.rs | Adds bounded upload-based upstream and
bufferbloat measurement; the previously reported unbounded request path
is addressed. |
| .github/workflows/release-nesdoctor.yml | Builds, smoke-tests,
packages, checksums, and publishes the four release targets. |
| .github/workflows/ci.yml | Adds focused formatting, linting, testing,
and offline execution checks for nesdoctor. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Run[nesdoctor] --> Host[Host requirement probes]
Run --> Net[Upload and latency measurement]
Run --> Consent{Steam consent}
Consent -->|yes| Steam[Installed manifests and LastPlayed records]
Host --> Physical[Resolve filesystem sources to physical devices]
Physical --> Verdict[Host readiness verdict]
Net --> Report[Detailed JSON and shareable summary]
Steam --> Report
Verdict --> Report
```
<details><summary>Prompt To Fix All With AI</summary>
`````markdown
### Issue 1
apps/nesdoctor/src/sys.rs:369-374
**Mapper devices remain unresolved**
When root and box-store filesystems are separate LVM or dm-crypt mappings on the same physical disk, `df` supplies `/dev/mapper/...` names that do not exist under `/sys/class/block`. This branch returns those unrelated logical names unchanged, so the overlap check passes stores that still share one physical I/O queue.
### Issue 2
apps/nesdoctor/src/steam.rs:247-250
**Unknown appids bypass runtime filtering**
If `localconfig.vdf` retains `LastPlayed` data for an uninstalled Proton build, Steam runtime, or other non-game tool, its appid is absent from the installed-manifest map and this branch treats it as an uninstalled game. The tool activity then changes the launch histogram, peak window, and shareable `n` value.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
`````
</details>
<sub>Reviews (5): Last reviewed commit: ["fix(nesdoctor): three valid P1
findings
..."](https://github.com/nestrilabs/nestri/commit/7afc8929a6a1f8f6c1f0737efa82857df5aae500)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=59231233)</sub>
> Greptile also left **2 inline comments** on this PR.
<!-- /greptile_comment -->
|
||
|
Wanjohi
|
1c24a6e684 |
fix(ci): the web job has been failing on two separate bugs (#311)
`bun run db:push` has been failing on **every pull request** with
`error: Script not found "db:push"`. A red check has meant nothing for
as long as that's been true.
There are **two independent causes**, and fixing only the reported one
leaves the job red.
## 1. The script isn't at the root
`db:push` lives in `packages/core/package.json`; CI runs from the root.
Added root passthroughs for `db:migrate` and `db:push`, so the command
CI runs is also the one a human can run.
## 2. `drizzle.config.ts` enabled TLS for any `DATABASE_URL`
```ts
ssl: !!process.env.DATABASE_URL ? { rejectUnauthorized: false } : false
```
That's true for *any* URL — so it failed against every plain Postgres,
**including CI's own `postgres:18-alpine` service container**. And
`drizzle-kit` reports that failure as a spinner and a non-zero exit with
no message attached, which is why it would have been maddening to find
from a log.
Measured against a local container:
| | result |
|---|---|
| `DATABASE_URL` set (TLS on) | migrations fail, no error text |
| `DATABASE_URL` unset, same database | all seven apply |
TLS is now decided by the connection string: an explicit `sslmode` wins,
otherwise a local host gets none (it doesn't speak TLS at all) and any
other host gets TLS without chain verification, which is what a hosted
Postgres usually needs. The URL is parsed once rather than eight times.
## CI now applies migrations instead of `push`
`drizzle-kit push` diffs the schema against whatever is already in the
database and is a development tool — CI wants exactly what's committed
in `packages/core/migrations`. And `push` under `strict: true` asks for
confirmation, which on a runner is a **hang**, not a failure.
## Verified
Locally against `postgres:18-alpine` from an empty database, running
exactly what the workflow runs:
```
bun install --frozen-lockfile ✓
bun run db:migrate ✓ 7 migrations applied
bun test ✓ 113 pass, 0 fail, 297 expect() calls
```
Worth landing ahead of #310 so that a red check starts meaning something
again.
<!-- greptile_comment -->
<h3>Greptile Summary</h3>
This PR repairs the database-backed web CI job by exposing core database
commands at the workspace root, applying committed migrations instead of
schema push, and selecting PostgreSQL TLS behavior from the connection
URL.
- Adds root passthrough scripts for database migration and schema push
commands.
- Adds the core `drizzle-kit migrate` command and runs it in CI.
- Disables TLS for local PostgreSQL while honoring explicit `sslmode`
settings.
- Keeps migration and test steps pointed at the same temporary CI
database.
<h3>Confidence Score: 5/5</h3>
The PR appears safe to merge, with the migration command, working
directory, connection settings, and test database remaining aligned.
The changed CI path reaches the committed migration history through the
intended core package configuration, uses plaintext for the local
PostgreSQL service, and then tests against the same migrated database;
no changed-code defect remains.
<h3>Important Files Changed</h3>
| Filename | Overview |
|----------|----------|
| .github/workflows/ci.yml | Replaces schema push with committed
migration execution while preserving the shared CI database URL. |
| package.json | Adds root-level passthroughs to the core package's
database commands. |
| packages/core/drizzle.config.ts | Parses the database URL once and
selects TLS based on explicit mode or local-versus-remote host
inference. |
| packages/core/package.json | Adds the `drizzle-kit migrate` script
consumed by the root command and CI workflow. |
<h3>Flowchart</h3>
```mermaid
%%{init: {'theme': 'neutral'}}%%
flowchart LR
PR[Pull request or main push] --> CI[Web CI job]
CI --> PG[PostgreSQL 18 service]
CI --> Install[Bun frozen install]
Install --> Root[Root db:migrate script]
Root --> Core[packages/core db:migrate]
Core --> Config[drizzle.config.ts]
Config --> Migrations[Committed migrations]
Migrations --> PG
PG --> Tests[Bun tests]
```
<sub>Reviews (1): Last reviewed commit: ["fix(ci): the web job has been
failing
on..."](https://github.com/nestrilabs/nestri/commit/203e882fbd7ab1578922f63e2c8986dce0b78c6f)
| [Re-trigger
Greptile](https://app.greptile.com/api/retrigger?id=59430932)</sub>
<!-- /greptile_comment -->
|
||
 WanjohiandClaude Opus 5
|
bf2c9632f5 |
fix(build): nescapture's release profile was being ignored
`apps/nescapture/Cargo.toml` carried `[profile.release]` with `opt-level = 3` and `lto = "thin"`. Cargo only reads `[profile.*]` from the workspace root and warns about a member that writes one, so `cargo build --release --workspace` — which is what build/Dockerfile runs — ignored it. The Vulkan capture layer that ends up in the guest image was built at the default release profile, and the warning saying so scrolled past on every build. `opt-level = 3` is already the release default, so `lto = "thin"` is the only part that was actually lost. It moves to the workspace root, where Cargo reads it, and now applies to all five members — the same rule as `[workspace.dependencies]` right above it: what must not differ between members is stated once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
WanjohiandClaude Opus 5
|
a76ca9ae81 |
fix(build): the root really is read-only, and two comments that overclaimed
Three places where build/ said something the tree does not do. - fstab mounted `/` as `rw`. A box is started with `ro` on the kernel command line and `is_read_only: true` on the root device (every config in nesbox's tree agrees: examples/vm.json, test.json, run.local.json), so the virtio-blk device rejects writes whatever fstab asks for. `rw` here only made OpenRC's `root` service attempt a remount that has to fail. The Dockerfile already depended on the truth — it pre-creates /nestri/* at build time precisely because a runtime mkdir gets EROFS — so this makes fstab agree with the comment that was already right. - build/README.md said nesbox's jail image "extracts Mesa and virglrenderer" from this base. It extracts only Mesa. virglrenderer is the host half of the native-context protocol and nesbox builds its own, patched, from nesbox/patches/; nothing in this image carries it at all. - conf.d/nestri-user-env called the zink driver-forcing block "load-bearing" directly above three exports that are commented out, here and in the profile.d copy. Whether they should come back is a separate question; a comment insisting disabled lines are load-bearing tells the next reader the opposite of what the file does. The reasoning is kept, because it is still the reason to re-enable them, along with why both copies have to move together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Wanjohi
|
6ea241c910 |
docs: the guest READMEs described code that is gone
All three came across with their standalone repos and none had been touched since the transport changed. nescapture claimed to packetize with Reed-Solomon FEC and stream RTP/UDP to a Moonlight client. It sends encoded frames to neshub over a Unix socket. packetizer.rs, control.rs and shard_batch.rs do not exist, and neither does ARCHITECTURE.md. Nine documented environment variables are not read by anything -- NESCAPTURE_RTP_HOST was listed as *required* -- and five that are read were undocumented, including the one that says where frames go. Someone following that quick start would have set a required variable that does nothing and got no output. Documented the three sockets, since nescapture binds one and connects to two and that was written down nowhere. neswire documented --rtp-addr and --channels against a gstreamer pipeline. It has --ipc-path, --channels, --packet-duration-ms and --bitrate-per-channel, and hub-stub exists precisely so it can be tested without a hub. Kept the reason hub-stub decodes rather than counts bytes: Opus codes silence at ~3 kbps, so a dead sink looks alive on a meter. nescope was mostly right. It called the capture layer "vkcapture", listed seven of fifteen modules and five of nine flags, and its TODO list was three items that neshub and nescapture now do. Added compositor mode, which is the shape a real session uses and was undocumented. All three licence lines were "TBD" or "See project repository". |
||
|
Wanjohi
|
c103e1257f |
docs: list neshub, and say the tree does not know what it runs
The payload-independence rule is the one thing about this repo that is easy to violate by being helpful. Stating it where it will be read before the first edit is cheaper than catching it in review. |
||
|
Wanjohi
|
40b80d4b14 |
refactor: the hub is neshub everywhere
Six comments across nescope, neswire and nescapture still named nestri-guest-hub, plus one still naming nestri-protocol. Deferred from the import commits so the rename would read as one change rather than six unexplained edits inside otherwise-verbatim trees. Comments only. Realigned an ASCII box in encode.rs that the shorter name knocked crooked. |
||
|
Wanjohi
|
3c574af2ea |
feat(neshub): open the media hub
The component nescapture, neswire and nescope all talk to, and the only thing in the guest that speaks to the client. It muxes their frames into one iroh QUIC endpoint and fans input back. Renamed from nestri-guest-hub, which named a location rather than a job. Four files came across unchanged -- session.rs, ipc_listener.rs, ticket.rs, screenshot.rs. Between them they mention Steam zero times, and they import only nesprotocol's open modules; the control feature carrying LaunchIntent and SteamIdentity is used exclusively by the three files that are staying closed. The two clusters shared a main.rs and nothing else, so there was no untangling to do -- only a cut. main.rs loses --proton, --steamclient-so, --root and the game uid/gid, and no longer ends by handing the process to a controller. It runs until it is stopped. Deciding when the box is finished belongs to nesinit. The ticket used to leave via that controller, so it needed a new way out: neshub now serves it on a socket and nesinit dials for it. Listening rather than dialling matches every other socket here and means no startup ordering to get wrong. Three tests, where there were none -- the ticket crosses a process boundary as text now, so a round trip that drops a field would otherwise be found by whoever cannot connect. |
||
|
WanjohiandClaude Opus 5
|
77d4782c86 |
docs: split CLAUDE.md by scope, and say what this repo is
CLAUDE.md was 1,324 lines and all of it was about the TypeScript half, written before there was another half. Every line of it loaded on every turn regardless of what was being worked on, which is a real cost paid constantly for context that is usually irrelevant. Split by where it applies, so each guide loads when you are in the directory it describes: packages/core/CLAUDE.md 694 domain modules, fn(), actor, errors, auth apps/api/CLAUDE.md 284 routes, registration, error flow docs/alchemy.md 345 stages, bindings, secrets, the CLI CLAUDE.md 72 the repo, both toolchains, two hard rules Nothing was rewritten or dropped — the three files are the original text, verified identical after the split. What the root file now carries is only what is true repo-wide: the layout, the commands, where the detail lives, and the two rules that are not style preferences. One of those is that nothing closed may enter this repo, which is here because it has already been caught once. The README described a streaming platform in four bullets and did not mention that half the repository is Rust that runs inside a virtual machine. It now says what each component does, why a micro-VM rather than a container, what is deliberately absent, and what decides whether a thing is open — data is, capacity is not. It also says plainly that this is mid-rewrite and the docs are behind. Someone arriving at a repo whose documentation does not match its tree should be told that by the README rather than discover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |