diff --git a/packages/nikcli/src/server/httpapi/pty.ts b/packages/nikcli/src/server/httpapi/pty.ts index 6c9b2f72d..c3686cdf2 100644 --- a/packages/nikcli/src/server/httpapi/pty.ts +++ b/packages/nikcli/src/server/httpapi/pty.ts @@ -125,11 +125,13 @@ export namespace PtyHttpApi { export const ApiLive = HttpApiBuilder.layer(Api) /** - * Translate a `Pty.NotFoundError` to the declared 404 body. Anything - * else propagates as a defect — the service surfaces `never` on success - * channels for these handlers, so the only expected failure is "missing". + * Translate a `Pty.NotFoundError` to the declared 404 body. The parameter + * is the handler's declared failure type, so the `instanceof` guard is + * total today; it stays as the defect arm for the day the channel grows a + * second member, because mislabelling that member as a 404 is worse than + * a 500 that names it. */ - const asNotFound = (cause: unknown): Effect.Effect => { + const asNotFound = (cause: Pty.NotFoundError): Effect.Effect => { if (cause instanceof Pty.NotFoundError) { return Effect.fail({ name: "NotFoundError" as const, @@ -139,14 +141,20 @@ export namespace PtyHttpApi { return Effect.die(cause) } - const catchNotFound = (effect: Effect.Effect) => - effect.pipe(Effect.catch(asNotFound), Effect.catchDefect(asNotFound)) + /** + * E8.2: the 404 arrives on the typed channel, so there is no defect arm. + * `handlers.get` / `handlers.update` `Effect.fail` a `Pty.NotFoundError` + * rather than `throw`ing it inside `Effect.gen`, which is what used to + * make a declared error reach this boundary as a defect. + */ + const catchNotFound = (effect: Effect.Effect) => effect.pipe(Effect.catch(asNotFound)) /** - * Translate a `Pty.CreateError` to the declared 400 body. Anything else - * propagates as a defect. + * Translate a `Pty.CreateError` to the declared 400 body. `Pty.Error` is + * `CreateError` alone today, so the guard is total; it stays for the same + * reason `asNotFound`'s does. */ - const asCreateError = (cause: unknown): Effect.Effect => { + const asCreateError = (cause: Pty.Error): Effect.Effect => { if (cause instanceof Pty.CreateError) { return Effect.fail({ name: "PtyCreateError" as const, @@ -159,8 +167,13 @@ export namespace PtyHttpApi { return Effect.die(cause) } - const catchCreateError = (effect: Effect.Effect) => - effect.pipe(Effect.catch(asCreateError), Effect.catchDefect(asCreateError)) + /** + * E8.2: `Pty.Service.create` already declares `Pty.Error` (`CreateError`) + * on its failure channel — `src/pty/index.ts` builds it in the `catch` of + * the `Effect.try` around `spawnPty`. Nothing produces it as a defect, so + * the defect arm this used to carry was dead compensation. + */ + const catchCreateError = (effect: Effect.Effect) => effect.pipe(Effect.catch(asCreateError)) /** * Cast helpers — safe because `PtyCreateInput`/`PtyUpdateInput` are @@ -190,7 +203,7 @@ export namespace PtyHttpApi { const pty = yield* Pty.Service const info = yield* pty.get(params.ptyID) if (!info) { - throw new Pty.NotFoundError({ message: "Session not found" }) + return yield* Effect.fail(new Pty.NotFoundError({ message: "Session not found" })) } return info }).pipe(catchNotFound), @@ -200,7 +213,7 @@ export namespace PtyHttpApi { const pty = yield* Pty.Service const info = yield* pty.update(params.ptyID, toPtyUpdateInput(payload)) if (!info) { - throw new Pty.NotFoundError({ message: "Session not found" }) + return yield* Effect.fail(new Pty.NotFoundError({ message: "Session not found" })) } return info }).pipe(catchNotFound), diff --git a/packages/nikcli/src/session/prompt.ts b/packages/nikcli/src/session/prompt.ts index 4143a5a39..e9bb5996b 100644 --- a/packages/nikcli/src/session/prompt.ts +++ b/packages/nikcli/src/session/prompt.ts @@ -369,7 +369,13 @@ export namespace SessionPrompt { } export interface Interface { - assertNotBusy(sessionID: string): Effect.Effect + /** + * Fails with `Session.BusyError` when the session already has a running + * turn. Declared on the typed channel (E8.1): a busy session is an + * expected 409, not a defect, so every caller — Effect-side or through + * the Promise bridge — sees it without a `catchDefect` arm. + */ + assertNotBusy(sessionID: string): Effect.Effect /** * Persist the user message (and optional tool permissions) without starting * the model loop. Used by `prompt_async` so clients can observe the message @@ -2035,10 +2041,12 @@ NOTE: At any point in time through this workflow you should feel free to ask the Effect.gen(function* () { const match = (yield* PromptState.getServiceStateEffect())[sessionID] if (match) - throw new Session.BusyError({ - sessionID, - message: "Session is busy", - }) + return yield* Effect.fail( + new Session.BusyError({ + sessionID, + message: "Session is busy", + }), + ) }), admit: (input) => withInstanceContext(() => admit(input)), steerPending: (input) => withInstanceContext(() => steerPending(input)), diff --git a/packages/nikcli/test/server/httpapi-pty.test.ts b/packages/nikcli/test/server/httpapi-pty.test.ts index b9150218b..3e89a9b9c 100644 --- a/packages/nikcli/test/server/httpapi-pty.test.ts +++ b/packages/nikcli/test/server/httpapi-pty.test.ts @@ -1,6 +1,7 @@ import { preserveTestEnv } from "../helpers/env" import { removeTestDir } from "../helpers/fs" import { afterAll, afterEach, describe, expect, it } from "bun:test" +import { Cause, Effect } from "effect" import fs from "fs/promises" import os from "os" import path from "path" @@ -14,6 +15,9 @@ preserveTestEnv(["NIKCLI_TEST_HOME", "NIKCLI_DISABLE_PROJECT_CONFIG"]) const { Instance } = await import("@/project/instance") const { HttpApiBridge } = await import("@/server/httpapi/bridge") const { Server } = await import("@/server/server") +const { Pty } = await import("@/pty") +const { PtyHttpApi } = await import("@/server/httpapi/pty") +const { runPromiseExitWithLayer, withCurrentInstance } = await import("@/effect") const projectDirs: string[] = [] @@ -101,6 +105,73 @@ describe("Pty HttpApi (Wave 4 Path B)", () => { expect(body).toBe(true) }) + /** + * E8.2. `handlers.get` / `handlers.update` used to raise the declared + * `Pty.NotFoundError` with `throw` inside `Effect.gen`, so it reached the + * boundary as a *defect* and only became a 404 because `catchNotFound` + * carried an `Effect.catchDefect` arm alongside its typed one. The defect + * arm is gone; these assertions are what goes red if the `throw` returns, + * because a die is no longer recovered into the declared body. + */ + it("handlers.get maps a missing session on the typed channel, with no defect", async () => { + const directory = await makeProjectDir() + await Instance.provide({ + directory, + fn: async () => { + const exit = await runPromiseExitWithLayer( + Pty.defaultLayer, + withCurrentInstance(PtyHttpApi.handlers.get({ params: { ptyID: "pty_definitely_missing" } })), + ) + expect(exit._tag).toBe("Failure") + if (exit._tag !== "Failure") return + expect(Cause.hasDies(exit.cause)).toBe(false) + expect(Cause.squash(exit.cause)).toEqual({ + name: "NotFoundError", + data: { message: "Session not found" }, + }) + }, + }) + }) + + it("handlers.update maps a missing session on the typed channel, with no defect", async () => { + const directory = await makeProjectDir() + await Instance.provide({ + directory, + fn: async () => { + const exit = await runPromiseExitWithLayer( + Pty.defaultLayer, + withCurrentInstance( + PtyHttpApi.handlers.update({ + params: { ptyID: "pty_definitely_missing" }, + payload: { title: "renamed" }, + }), + ), + ) + expect(exit._tag).toBe("Failure") + if (exit._tag !== "Failure") return + expect(Cause.hasDies(exit.cause)).toBe(false) + expect(Cause.squash(exit.cause)).toEqual({ + name: "NotFoundError", + data: { message: "Session not found" }, + }) + }, + }) + }) + + it("handlers.list stays total — a success carries no failure channel to map", async () => { + const directory = await makeProjectDir() + await Instance.provide({ + directory, + fn: async () => { + const exit = await runPromiseExitWithLayer( + Pty.defaultLayer, + withCurrentInstance(Effect.map(PtyHttpApi.handlers.list(), (list) => list.length)), + ) + expect(exit._tag).toBe("Success") + }, + }) + }) + it("POST /pty rejects a malformed payload with 400 (schema layer)", async () => { const directory = await makeProjectDir() const response = await request("POST", "/pty", directory, { diff --git a/packages/nikcli/test/session/session-lifecycle.test.ts b/packages/nikcli/test/session/session-lifecycle.test.ts index a8ab42b69..c6eac965f 100644 --- a/packages/nikcli/test/session/session-lifecycle.test.ts +++ b/packages/nikcli/test/session/session-lifecycle.test.ts @@ -424,6 +424,47 @@ describe("session lifecycle", () => { }) }) + it("SessionPrompt.assertNotBusy fails with SessionBusyError on the typed channel", async () => { + await withProject(async () => { + const { PromptState } = await import("../../src/session/prompt-state") + const { SessionPrompt } = await import("../../src/session/prompt") + const session = await createSession() + + // Not reserved: the assertion is a plain success, not an absent + // failure that happens to be swallowed somewhere. + const idle = await runPromiseExitWithLayer( + SessionPrompt.defaultLayer, + withCurrentInstance( + Effect.gen(function* () { + const prompt = yield* SessionPrompt.Service + return yield* prompt.assertNotBusy(session.id) + }), + ), + ) + expect(idle._tag).toBe("Success") + + PromptState.reserve(session.id) + const exit = await runPromiseExitWithLayer( + SessionPrompt.defaultLayer, + withCurrentInstance( + Effect.gen(function* () { + const prompt = yield* SessionPrompt.Service + return yield* prompt.assertNotBusy(session.id) + }), + ), + ) + expect(exit._tag).toBe("Failure") + if (exit._tag !== "Failure") return + // E8.1. The busy assertion used to `throw` inside `Effect.gen`, which + // is a defect: it only reached callers typed because `SessionRevert` + // ran it through `runPromiseWithLayer` and re-mapped the rejection. + // `hasDies` is what separates the two, so this assertion is the one + // that goes red if the `throw` comes back. + expect(Cause.hasDies(exit.cause)).toBe(false) + expect(Cause.squash(exit.cause)).toBeInstanceOf(Session.BusyError) + }) + }) + it("SessionSummary.diff rejects with SessionNotFoundError on a typed failure channel", async () => { await withProject(async () => { const { SessionSummary } = await import("../../src/session/summary") diff --git a/specs/ROADMAP.md b/specs/ROADMAP.md index 3020d309b..e0dbcbdfd 100644 --- a/specs/ROADMAP.md +++ b/specs/ROADMAP.md @@ -45,6 +45,7 @@ The **E4 service-side slices landed** (2026-08-19): `Session.Info` and every `Me | **R2** | Next | Retire the instance ALS across its remaining boundary reads | | **H9** | Next | Declare response headers on the contract (`HttpApiSchema.WithHeaders`) | | **H10** | Gated | Measure `Schema.TaggedUnion.matchOrElse` against the open SessionV2 payloads | +| **E8** | Done | Declared failures raised with `Effect.fail`, not `throw` inside `Effect.gen` | ### Release integrity (C1) — landed 2026-08-23 @@ -154,7 +155,7 @@ These are evidenced leftovers, not product ideas. `Now` items are independent an ### Execute next -- **Order** — **E6, then E7; R2 and H9 follow.** E5, P2 and H8 landed on 2026-08-24 and R1 on 2026-08-26, which emptied the queue; the horizon below refills it from one measurement — [research-effect-4-rc.md](./research-effect-4-rc.md). E6 is first not because it is urgent but because every other Effect-facing item names an API that does not exist at the current pin, so none of them can be started or even measured until it lands. E7 is a paragraph of contributor guardrail that E6 makes true. R2 is independent of all of it and can run in a separate lane. H10 is a measurement that may reject itself. +- **Order** — **E6, then E7; R2 and H9 follow.** E5, P2 and H8 landed on 2026-08-24 and R1 on 2026-08-26, which emptied the queue; the horizon below refills it from one measurement — [research-effect-4-rc.md](./research-effect-4-rc.md). E6 is first not because it is urgent but because every other Effect-facing item names an API that does not exist at the current pin, so none of them can be started or even measured until it lands. E7 is a paragraph of contributor guardrail that E6 makes true. R2 is independent of all of it and can run in a separate lane. H10 is a measurement that may reject itself. E8 landed on 2026-08-26 and is independent of the pin: it closed E5's own recorded caveat with APIs `beta.83` already has. - **H8.1 — Put auth on the contract.** Landed 2026-08-24; see the dated log. - **P2.1 — Push list work into SQL.** Landed 2026-08-24, measured below. P2.2 is now unblocked, but it is a separate decision: read the measurement before scheduling it. - **P2.2 — Decided 2026-08-24 against the measurement.** The logging policy landed; the parsed-URL carry-through is **rejected** and the benches are **not scheduled**. Reasoning and numbers in the dated log below. @@ -207,13 +208,29 @@ These are evidenced leftovers, not product ideas. `Now` items are independent an - **Evidence** — `httpapi/session.ts` applied `Effect.catchDefect(asSessionError)` after mapping the typed failure channel, `SessionRevert.Interface` and the route-facing `SessionSummary` methods exposed `unknown`, and both modules used the untyped `Effect.tryPromise(() => ...)` form, so `Effect.tryPromise` wrapped `SessionNotFoundError` in `UnknownError` and missing-session revert and diff answered 500. `Session.BusyError` was already a `Schema.TaggedErrorClass`, so the contract vocabulary existed. - **Implementation** — E5.2 / E5.3 landed in `ff061973ec`: `Session.asSessionError` is exported, `SessionRevert.Interface` and `SessionSummary.summarize` / `diff` carry `Session.Error`, and the domain-rejecting handler bridges (`MessageV2.get` ×2, `SessionContext.breakdown` ×2, `SessionV2.entries`, `Monitor.get` / `readLog` / `cancel`) use `Effect.tryPromise({ catch: Session.asSessionError })`. `computeDiff` keeps `unknown` — it is real dependency I/O. E5.1 / E5.4 closed it: `declaredErrors` is a single `Effect.catch(asSessionError)`, and the `background` handler dropped its defect arm. The ten remaining `Effect.promise` sites are the audited unknown-I/O set — `Array.fromAsync`, the two session-delete cancels, the `collectSystemPaths` import and call, and the four `Delegation` job routes — and stay on `orDie`. The one `catchDefect` left in the file is the best-effort MCP toggle log, which swallows both channels on purpose and is not part of this boundary. - **Caveat for the next reader.** `SessionPrompt.assertNotBusy` is still declared `Effect.Effect` and raises by `throw` inside `Effect.gen`. It reaches callers typed only because `SessionRevert` runs it through `runPromiseWithLayer` and re-maps the rejection with `Session.asSessionError`; the busy assertion below pins that behavior. Narrowing that signature to `Session.BusyError` with an explicit `Effect.fail` is a separate cleanup, not a reopening of E5. + **Caveat for the next reader — closed 2026-08-26 as E8.1.** `SessionPrompt.assertNotBusy` was declared `Effect.Effect` and raised by `throw` inside `Effect.gen`. It reached callers typed only because `SessionRevert` ran it through `runPromiseWithLayer` and re-mapped the rejection with `Session.asSessionError`; the busy assertion below pins that behavior. Narrowing that signature to `Session.BusyError` with an explicit `Effect.fail` was called a separate cleanup rather than a reopening of E5, and it is one — see E8. **Corrected 2026-08-18 for `loop.ts` / `mission.ts`.** Both already carry the typed channel: declared 404/400 schemas plus `failNotFound` / `failValidation`, and their managers use the return-`undefined` convention the handlers already check. The `fromPromise` `orDie` wraps genuine I/O, not domain errors. There are no `Engine.LoopNotFoundError` / `MissionNotFoundError` / `MissionAlreadyExistsError` tags to fail with — an earlier draft of this item invented them. The one real gap there is fixed (see landed work). - **Depends on** — nothing. H4 landed, so there was already one boundary to fix. H8 waited for this typed vocabulary and no longer does. - **Done when** — Met. Domain methods map `Session.Error` on the typed channel; return-`undefined` plus an explicit `Effect.fail` remains valid for loop/mission. Session handlers map schema-declared errors without `catchDefect`, and `Effect.promise` / `orDie` remains only for genuinely unknown I/O. The service-level assertions in `test/session/session-lifecycle.test.ts` assert `Cause.hasDies === false` before squashing, so they separate `Effect.fail` from `Effect.die` instead of reading through both; route tests separately pin the unchanged 404/409 wire bodies. +### Declared failures on the typed channel (E8) — landed 2026-08-26 + +E5 closed the session HTTP boundary: declared errors are mapped from the typed channel and `catchDefect(asSessionError)` is gone. It recorded one remainder in its own text and did not claim it. E8 is that remainder plus the one other place in `src` with the same shape. It is a separate item because E5's acceptance gate is about the session boundary, and reopening a met gate to absorb new work is how a gate stops meaning anything. + +**On the id.** This was drafted as `E6` while the near plan was empty, and the queue was refilled with its own `E6` (the pin bump) in parallel. It is `E8` because the queued ids were published first; the numbering gap is that collision, not a missing item. E8 is independent of E6 — it uses only APIs `beta.83` already ships, so it neither blocks nor waits on the pin. + +- **Buys** — A declared domain error is a failure, not a defect, at the point it is raised. A handler that maps it needs one arm, not two, and a caller that forgets to map it gets a type error rather than a 500. +- **Evidence** — Two sites, both in the repository at the time of writing: + - `session/prompt.ts` declared `assertNotBusy(sessionID): Effect.Effect` and raised `Session.BusyError` with `throw` inside `Effect.gen`. `Session.BusyError` is already a `Schema.TaggedErrorClass` and already has a declared 409 body (`httpapi/session.ts`), so the vocabulary existed; only the signature and the raise did not use it. It reached `SessionRevert`'s callers typed purely by accident of the Promise bridge — `runPromiseWithLayer` rejects with the squashed cause and `Session.asSessionError` passes a `BusyError` straight through. + - `server/httpapi/pty.ts` raised the declared `Pty.NotFoundError` with `throw` inside `Effect.gen` in `handlers.get` and `handlers.update`, so the only reason `GET /pty/:id` answered 404 rather than 500 was that `catchNotFound` carried `Effect.catchDefect(asNotFound)` beside its typed arm. `catchCreateError` carried the same pair, and there its defect arm was dead: `Pty.Service.create` builds `CreateError` in the `catch` of an `Effect.try` around `spawnPty`, so it is always typed. +- **Implementation** — `assertNotBusy` is `Effect.Effect` and fails with `Effect.fail`. The pty handlers fail with `Effect.fail`, and `catchNotFound` / `catchCreateError` narrow their input to `Pty.NotFoundError` / `Pty.Error` and drop the `catchDefect` arm — so a re-introduced `throw` is a 500, visibly, instead of being silently absorbed. +- **Depends on** — E5, for the vocabulary and for the `Cause.hasDies` assertion style that separates a `fail` from a `die`. +- **Done when** — Met 2026-08-26. No HTTP wire change: `test/server/httpapi-pty.test.ts` still pins the 404 body byte-for-byte, and `test/session/session-lifecycle.test.ts` still pins the busy revert as `Session.BusyError`. Service-level assertions read `Cause.hasDies === false` on `SessionPrompt.assertNotBusy` and on both pty handlers, which is the check that goes red if the `throw` comes back — with the defect arms removed there is nothing left to recover it. + + **What E8 deliberately does not cover.** The `throw`s in `codemode/interpreter/` are not this: the interpreter unwinds by throwing `InterpreterRuntimeError` on purpose and catches it at its own boundary (`codemode/interpreter/errors.ts` matches on it twice), which is a design, not a leak. `session/prompt-commands.ts` throws `Session.BusyError` from an `async` function, which is the Promise side of the bridge and is the normal way to reject there. `httpapi/session.ts:908` throws a bare `Error` on a part-id mismatch; turning that into a declared 400 is a wire change (500 today) and needs its own item with a regenerated client, so it is not folded in here. + ### Request-path cuts (P2) - **Buys** — Encoded JSON requests stop paying for work the contract already did. Hot polls (`/event`, `/session/status`, TUI) stop dominating logs and extra SQL. @@ -714,6 +731,20 @@ P2.2 was queued behind "only after P2.1 records its result". It did, so this is **One cast worth not having.** The middleware layer was first written as `Layer.succeed(Middleware, { … } as never)`. A cast to `never` silences every check, so it came out before the commit, and the typecheck confirms it was never needed: `Layer.succeed` types its value from the service key, which instantiates `authorize`'s generic at `HttpServerResponse` and collapses its two return branches. With the cast in place that would have been unknowable — which is the argument against writing it in the first place. +### 2026-08-26 — E8 (declared failures on the typed channel) + +E5's own text recorded a remainder it did not claim, and a sweep for that shape found one more. Both are closed here. The sweep is worth describing because "grep for `throw`" does not answer the question: what matters is a `throw` whose _nearest enclosing_ function is an `Effect.gen` body, so the search brace-matched each `Effect.gen(function* () {` and reported the throws inside it. That found 57 occurrences across `src`, of which all but three are `codemode/interpreter/`, where throwing is the interpreter's own unwind mechanism and `codemode/interpreter/errors.ts` matches on `InterpreterRuntimeError` at the boundary. The three that were not that: `session/prompt.ts` (E8.1), `httpapi/pty.ts` ×2 (E8.2), and `httpapi/session.ts:908`, which is left alone because fixing it is a 500→400 wire change and needs its own item. + +**The pty finding is the one with a user-visible consequence.** `handlers.get` and `handlers.update` threw the declared `Pty.NotFoundError` inside `Effect.gen`, so it arrived at the boundary as a defect, and `GET /pty/:id` returned 404 only because `catchNotFound` paired `Effect.catchDefect(asNotFound)` with its typed arm. That is precisely the compensation E5 deleted from `httpapi/session.ts`. Removing the defect arm without moving the raise onto the typed channel would have turned every missing-pty lookup into a 500 — which is exactly what the mutation run below demonstrates, and why the two halves land together. + +**`catchCreateError`'s defect arm was dead.** `Pty.Service.create` builds `CreateError` in the `catch` of the `Effect.try` around `spawnPty`, so it is always typed; nothing in `src/pty/index.ts` throws at all. Both combinators now take the declared error type rather than `E`, so the compiler — not a second runtime arm — is what keeps the mapping total. + +**Verified by mutation, both ways.** Restoring the two `throw`s turns exactly three tests red and leaves the rest green: the two new typed-channel assertions, plus the **pre-existing** `GET /pty/:id ... returns 404 with the declared error body` route test, which is the one that proves the behavioural claim rather than describing it. Notably the busy-revert test stays green under mutation, and that is correct: it reaches its assertion through `runPromiseWithLayer`, which squashes a die and a fail alike — which is the whole reason E5 called the old signature typed only by accident. + +**Verified.** `bun run typecheck` clean on `packages/nikcli`. `bun test test/session/ test/server/` 780 pass / 1 fail across 90 files; the failure is `httpapi-top-level.test.ts` and is **not this change** — it fails identically with `session/prompt.ts` and `httpapi/pty.ts` reverted to their pre-E8 state. `bun run check:routes --strict` ok at 338 contracts / 315 handlers / 23 raw. `bunx oxlint` and `bunx prettier --check` clean on the four touched files. + +**A local-only regeneration difference, and why it is not drift.** Regenerating the HttpApi clients in the development sandbox reorders 29 `Event*` declarations in `packages/sdk/js/src/httpapi/generated/types.ts` — same content, different order. It was first recorded here as a pre-existing red gate, which was wrong: `validate` runs exactly that check (`generate:httpapi-clients` followed by `git diff --exit-code -- packages/sdk/js/src/httpapi/generated`) and it **passes** in CI on this commit. The committed file is correct. The reordering reproduced with the E8 source reverted, so it is not this change either — the likely cause is an interrupted `bun install` in that sandbox leaving a module graph in which a different set of event modules registered, and event declaration order follows registration. Worth knowing if you regenerate locally and see a diff you did not cause: check `validate` before opening an item for it. + ## Follow working rules - Commit at phase boundaries, not per file. H4 and H5 land together.