diff --git a/.changeset/quiet-worktrees-explain.md b/.changeset/quiet-worktrees-explain.md new file mode 100644 index 000000000..472d9137b --- /dev/null +++ b/.changeset/quiet-worktrees-explain.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +`hunk session` commands that select a window with `--repo` or `--session-path` now say which path matched no live session, instead of failing with `protocol-validation-failed` (for example, when run from a different git worktree than the Hunk window). The session daemon produces this message, so it appears once a daemon from this release is running; an older daemon keeps answering with `protocol-validation-failed` until it restarts. diff --git a/packages/hunk/src/session/broker/brokerServer.test.ts b/packages/hunk/src/session/broker/brokerServer.test.ts index ec9f758a9..84d544f71 100644 --- a/packages/hunk/src/session/broker/brokerServer.test.ts +++ b/packages/hunk/src/session/broker/brokerServer.test.ts @@ -106,6 +106,7 @@ async function authenticatedFetch( port: number, path: string, init: SessionBrokerSignedRequestInit = {}, + options: { targetSpecific?: boolean } = {}, ) { const credentials = await loadOrCreateHunkSessionBrokerCredentials(); const caller = new SessionBrokerCallerClient({ @@ -120,7 +121,7 @@ async function authenticatedFetch( ? ((JSON.parse(init.body) as { action?: string }).action ?? "") : ""; return caller.request(path, init, { - targetSpecific: path === "/session-api" && action !== "list", + targetSpecific: options.targetSpecific ?? (path === "/session-api" && action !== "list"), }); } @@ -939,6 +940,82 @@ describe("Hunk session daemon server", () => { } }); + test("reports why a repo selector matched no session instead of a redacted failure", async () => { + const port = await reserveLoopbackPort(); + process.env.HUNK_MCP_HOST = "127.0.0.1"; + process.env.HUNK_MCP_PORT = String(port); + + const original = SessionBrokerState.prototype.dispatchCommand; + const dispatched: string[] = []; + SessionBrokerState.prototype.dispatchCommand = (({ command }: { command: string }) => { + dispatched.push(command); + return Promise.reject(new Error("A selector miss must not dispatch.")); + }) as SessionBrokerState["dispatchCommand"]; + + const server = await serveSessionBrokerDaemon(); + let socket: WebSocket | undefined; + + try { + // The live window is showing /repo; the caller is in a different checkout. + socket = await openRegisteredSession(port); + for (const request of [ + { action: "get", selector: { repoRoot: "/elsewhere" } }, + { + action: "reload", + selector: { repoRoot: "/elsewhere" }, + nextInput: { kind: "vcs", staged: false, options: {} }, + }, + ]) { + const response = await authenticatedFetch(port, "/session-api", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(request), + }); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: "No active session matches repoRoot /elsewhere.", + }); + } + expect(dispatched).toEqual([]); + } finally { + SessionBrokerState.prototype.dispatchCommand = original; + socket?.close(); + server.stop(true); + } + }); + + test("keeps malformed session API bodies redacted", async () => { + const port = await reserveLoopbackPort(); + process.env.HUNK_MCP_HOST = "127.0.0.1"; + process.env.HUNK_MCP_PORT = String(port); + + const server = await serveSessionBrokerDaemon(); + + try { + const response = await authenticatedFetch( + port, + "/session-api", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + action: "reload", + selector: { repoRoot: "/repo" }, + nextInput: { kind: "vcs", staged: false, options: { notAnOption: true } }, + }), + }, + // An unparseable body has no trustworthy target, so the daemon signs it untargeted. + { targetSpecific: false }, + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ error: "protocol-validation-failed" }); + } finally { + server.stop(true); + } + }); + test("serves review notes through the session API", async () => { const port = await reserveLoopbackPort(); process.env.HUNK_MCP_HOST = "127.0.0.1"; diff --git a/packages/hunk/src/session/broker/brokerServer.ts b/packages/hunk/src/session/broker/brokerServer.ts index 5766f063e..64fa73ded 100644 --- a/packages/hunk/src/session/broker/brokerServer.ts +++ b/packages/hunk/src/session/broker/brokerServer.ts @@ -1,6 +1,7 @@ import { SESSION_BROKER_ADMIN_SCOPE_VERSION, SessionBrokerAuthenticator, + SessionBrokerTargetResolutionError, createSessionBrokerDaemon, type SessionBrokerAuthenticatedControlFacts, type SessionBrokerController, @@ -357,14 +358,32 @@ function resolveNavigateCommandInput( }; } -/** Map each Hunk action to the generic operation and exact producer command scope it requires. */ +/** + * Map each Hunk action to the generic operation and exact producer command scope it requires. + * + * A `--repo` or `--session-path` selector names its session only after a lookup, so the lookup + * runs before authorization. When it matches no single session, the lookup error becomes a + * target-resolution failure that the daemon discloses only to callers allowed to `list`: the error + * names nothing a `list` would not, so the Hunk CLI learns why its selector missed, and a + * session-scoped caller still learns nothing. Malformed bodies keep throwing plain errors so the + * daemon answers with its redacted protocol failure. + */ function sessionApiAuthorizationFacts( state: HunkSessionBrokerState, bytes: Uint8Array, ): SessionBrokerAuthenticatedControlFacts { const input = parseJsonRequestBytes(bytes); if (input.action === "list") return { operation: "list", targetSpecific: false }; - const sessionId = input.selector.sessionId ?? state.getSession(input.selector).sessionId; + let sessionId = input.selector.sessionId; + if (sessionId === undefined) { + try { + sessionId = state.getSession(input.selector).sessionId; + } catch (error) { + throw new SessionBrokerTargetResolutionError( + error instanceof Error ? error.message : "No active session matches.", + ); + } + } if (["get", "context", "review", "comment-list"].includes(input.action)) { return { operation: "get", sessionId, targetSpecific: true }; } diff --git a/packages/session-broker/src/daemon.test.ts b/packages/session-broker/src/daemon.test.ts index 31e41aad9..fec8f365e 100644 --- a/packages/session-broker/src/daemon.test.ts +++ b/packages/session-broker/src/daemon.test.ts @@ -12,7 +12,7 @@ import { } from "@hunk/session-broker-core"; import { SessionBroker } from "./broker"; import { SESSION_BROKER_ADMIN_STOP_CLOSE_REASON } from "./admin"; -import { createSessionBrokerDaemon } from "./daemon"; +import { SessionBrokerTargetResolutionError, createSessionBrokerDaemon } from "./daemon"; import { createSessionBrokerProtocolParsers } from "./protocolParsers"; import type { AuthenticatedCallerRequest, @@ -1402,6 +1402,95 @@ describe("session broker daemon", () => { daemon.shutdown(); }); + test("discloses target resolution failures only to callers allowed to list, without handling", async () => { + const audited: { operation: string; decision: string; outcome: string }[] = []; + const caller = (principal: Partial) => + createSessionBrokerDaemon({ + broker: createBroker(), + exposeHttpApi: true, + appId: "session-broker", + appRevision: 1, + callerAuthenticator: { + authenticate: async () => + authenticatedRequest({ + kind: "caller", + appId: "session-broker", + principalId: "test-caller", + keyId: "test-key", + grantId: "test-grant", + operations: ["list", "get", "dispatch"], + commands: [], + ...principal, + }), + }, + authorizer: async () => true, + audit: ({ operation, decision, outcome }) => { + audited.push({ operation, decision, outcome }); + }, + }); + let handled = 0; + const control = { + resolve: () => { + throw new SessionBrokerTargetResolutionError("No active session matches repoRoot /x."); + }, + handle: () => { + handled += 1; + return { body: { ok: true } }; + }, + }; + const request = () => new Request("http://broker.test/custom", { method: "POST", body: "{}" }); + + const lister = caller({}); + const disclosed = await lister.handleAuthenticatedControl(request(), control); + expect(disclosed.status).toBe(400); + const disclosedEnvelope = (await disclosed.json()) as { + body: unknown; + authentication: { appContract?: unknown }; + }; + expect(disclosedEnvelope.body).toEqual({ error: "No active session matches repoRoot /x." }); + // The caller signed a targeted request, so the answer must carry the app contract. + expect(disclosedEnvelope.authentication.appContract).toBeDefined(); + lister.shutdown(); + + // A session-scoped caller can never list, so it learns nothing about other sessions. + const scoped = caller({ sessionId: "session-1" }); + const denied = await scoped.handleAuthenticatedControl(request(), control); + expect(denied.status).toBe(403); + expect(await authenticatedBody(denied)).toEqual({ error: "authorization-denied" }); + scoped.shutdown(); + + const unlisted = caller({ operations: ["get", "dispatch"] }); + expect((await unlisted.handleAuthenticatedControl(request(), control)).status).toBe(403); + unlisted.shutdown(); + + expect(handled).toBe(0); + expect(audited).toEqual([ + { operation: "list", decision: "allow", outcome: "target-resolution-failed" }, + { operation: "list", decision: "deny", outcome: "authorization-failed" }, + { operation: "list", decision: "deny", outcome: "authorization-failed" }, + ]); + }); + + test("keeps other resolve failures redacted", async () => { + const daemon = createSessionBrokerDaemon({ + broker: createBroker(), + exposeHttpApi: true, + ...authenticatedHttpApi, + }); + const response = await daemon.handleAuthenticatedControl( + new Request("http://broker.test/custom", { method: "POST", body: "{}" }), + { + resolve: () => { + throw new Error("private parser detail"); + }, + handle: () => ({ body: { ok: true } }), + }, + ); + expect(response.status).toBe(400); + expect(await authenticatedBody(response)).toEqual({ error: "protocol-validation-failed" }); + daemon.shutdown(); + }); + test("rejects daemon state limits that were not configured on its broker controller", () => { expect(() => createSessionBrokerDaemon({ diff --git a/packages/session-broker/src/daemon.ts b/packages/session-broker/src/daemon.ts index 411dd49d0..9fadedeb4 100644 --- a/packages/session-broker/src/daemon.ts +++ b/packages/session-broker/src/daemon.ts @@ -92,6 +92,31 @@ export interface SessionBrokerAuthenticatedControlResult { readonly status?: number; } +/** + * Reports that an authenticated control's selector resolved to no single session. + * + * Thrown from `handleAuthenticatedControl`'s `resolve` once the body has parsed. The daemon never + * runs `handle` for it; it answers with `message` only when the caller holds + * `disclosureOperation`, whose grant must already reveal everything the message names (with + * `list`, the matching session IDs and titles). Every other `resolve` error stays a redacted + * protocol failure. + */ +export class SessionBrokerTargetResolutionError extends Error { + readonly disclosureOperation: CallerOperation; + readonly targetSpecific: boolean; + + constructor( + message: string, + options: { disclosureOperation?: CallerOperation; targetSpecific?: boolean } = {}, + ) { + super(message); + this.name = "SessionBrokerTargetResolutionError"; + this.disclosureOperation = options.disclosureOperation ?? "list"; + // A selector that failed to resolve was aimed at a session, so the caller signed it targeted. + this.targetSpecific = options.targetSpecific ?? true; + } +} + /** * Configure the revision-tolerant admin scope (`status` and `stop`). * @@ -1016,7 +1041,10 @@ export class SessionBrokerDaemon< let facts: SessionBrokerAuthenticatedControlFacts; try { facts = options.resolve(body); - } catch { + } catch (error) { + if (error instanceof SessionBrokerTargetResolutionError) { + return this.answerTargetResolutionFailure(request, authenticated, error); + } let targetSpecific = false; try { targetSpecific = options.resolveFailureTargetSpecific?.(body) ?? false; @@ -1059,6 +1087,41 @@ export class SessionBrokerDaemon< }); } + /** + * Answer a control whose selector resolved to no single session, without running its action. + * + * Authorizes only the disclosure, audited under its own outcome so a missed `reload` never + * reads as a successful `list`. + */ + private async answerTargetResolutionFailure( + request: Request, + authenticated: AuthenticatedCallerRequest, + failure: SessionBrokerTargetResolutionError, + ): Promise { + const allowed = await this.authorize( + request, + authenticated, + { operation: failure.disclosureOperation }, + "target-resolution-failed", + ); + if (!allowed) { + return this.authenticatedResponse( + authenticated, + { error: "authorization-denied" }, + 403, + failure.targetSpecific, + ); + } + const inactive = this.rejectInactiveRequest(authenticated); + if (inactive) return inactive; + return this.authenticatedResponse( + authenticated, + { error: failure.message }, + 400, + failure.targetSpecific, + ); + } + private async authenticateRequest( request: Request, body: Uint8Array, @@ -1121,6 +1184,7 @@ export class SessionBrokerDaemon< command?: string; commandVersion?: number; }, + allowedOutcome: "authenticated" | "target-resolution-failed" = "authenticated", ): Promise { const principal: CallerPrincipal = authenticated.principal; const allowedByGrant = callerPrincipalAllows(principal, { @@ -1151,7 +1215,7 @@ export class SessionBrokerDaemon< ...(facts.commandVersion === undefined ? {} : { commandVersion: facts.commandVersion }), requestId: authenticated.requestId, decision: allowedByApp ? "allow" : "deny", - outcome: allowedByApp ? "authenticated" : "authorization-failed", + outcome: allowedByApp ? allowedOutcome : "authorization-failed", timestamp: Date.now(), }); return allowedByApp; diff --git a/packages/session-broker/src/types.ts b/packages/session-broker/src/types.ts index 700e613d2..d0f547ed1 100644 --- a/packages/session-broker/src/types.ts +++ b/packages/session-broker/src/types.ts @@ -133,7 +133,12 @@ export interface SessionBrokerAuditEvent { readonly commandVersion?: number; readonly requestId?: string; readonly decision: "allow" | "deny"; - readonly outcome: "authenticated" | "authentication-failed" | "authorization-failed"; + readonly outcome: + | "authenticated" + | "authentication-failed" + | "authorization-failed" + /** The caller may learn why its selector matched no single session; no action ran. */ + | "target-resolution-failed"; readonly timestamp: number; } diff --git a/test/session/cli.test.ts b/test/session/cli.test.ts index 85a1a9bc0..ee7efff5b 100644 --- a/test/session/cli.test.ts +++ b/test/session/cli.test.ts @@ -378,6 +378,34 @@ sessionDescribe("session CLI integration", () => { } }); + test("repo selectors that match no live session explain the miss", async () => { + const port = await reserveLoopbackPort(); + const fixture = createFixtureFiles( + "selector-miss", + ["export const a = 1;"], + ["export const a = 2;"], + ); + // Another checkout, such as a sibling worktree, that no live window is showing. + const elsewhere = createFixtureFiles("selector-elsewhere", [""], [""]); + const session = spawnHunkSession(fixture, port); + + try { + await waitForRegisteredSessions(port); + + for (const args of [ + ["get", "--repo", elsewhere.dir], + ["reload", "--repo", elsewhere.dir, "--", "diff"], + ]) { + const result = runSessionCli(args, port); + expect(result.proc.exitCode).not.toBe(0); + expect(result.stderr).toContain("No active session matches repoRoot"); + expect(result.stderr).not.toContain("protocol-validation-failed"); + } + } finally { + await cleanupHunkSession(session, fixture, port); + } + }); + test("reload replaces what a live session is showing", async () => { const port = await reserveLoopbackPort(); const fixture = createFixtureFiles(