-
Notifications
You must be signed in to change notification settings - Fork 283
fix(ui): prepare history reviews before terminal handoff #989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "hunkdiff": patch | ||
| --- | ||
|
|
||
| Keep the themed history loading screen visible until a selected commit review is ready to claim the terminal. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { | ||
| hasTerminalHandoff, | ||
| parseTerminalHandoffMessage, | ||
| terminalHandoffEnv, | ||
| terminalHandoffMessage, | ||
| terminalHandoffThemeMode, | ||
| } from "./terminalHandoff"; | ||
|
|
||
| describe("terminal handoff protocol", () => { | ||
| test("uses a private marker and inherits only a valid terminal mode", () => { | ||
| const env = terminalHandoffEnv({ PATH: "/bin" }, "dark"); | ||
| expect(hasTerminalHandoff(env)).toBe(true); | ||
| expect(terminalHandoffThemeMode(env)).toBe("dark"); | ||
| expect( | ||
| terminalHandoffThemeMode({ | ||
| HUNK_TERMINAL_HANDOFF: "1", | ||
| HUNK_TERMINAL_HANDOFF_THEME_MODE: "blue", | ||
| }), | ||
| ).toBeUndefined(); | ||
| expect(terminalHandoffThemeMode({ HUNK_TERMINAL_HANDOFF_THEME_MODE: "light" })).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("accepts only versioned bounded messages", () => { | ||
| expect(parseTerminalHandoffMessage(terminalHandoffMessage("ready"))).toEqual({ | ||
| protocol: "hunk-terminal-handoff-v1", | ||
| kind: "ready", | ||
| }); | ||
| expect(parseTerminalHandoffMessage({ protocol: "wrong", kind: "ready" })).toBeUndefined(); | ||
| expect( | ||
| parseTerminalHandoffMessage({ protocol: "hunk-terminal-handoff-v1", kind: "other" }), | ||
| ).toBeUndefined(); | ||
| expect( | ||
| parseTerminalHandoffMessage({ | ||
| protocol: "hunk-terminal-handoff-v1", | ||
| kind: "failed", | ||
| message: "x".repeat(3_000), | ||
| }), | ||
| ).toEqual({ | ||
| protocol: "hunk-terminal-handoff-v1", | ||
| kind: "failed", | ||
| message: "x".repeat(2_000), | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| const HANDOFF_ENV = "HUNK_TERMINAL_HANDOFF"; | ||
| const HANDOFF_THEME_MODE_ENV = "HUNK_TERMINAL_HANDOFF_THEME_MODE"; | ||
| const PROTOCOL = "hunk-terminal-handoff-v1"; | ||
|
|
||
| export type TerminalHandoffMessage = | ||
| | { protocol: typeof PROTOCOL; kind: "ready" } | ||
| | { protocol: typeof PROTOCOL; kind: "release" } | ||
| | { protocol: typeof PROTOCOL; kind: "failed"; message: string }; | ||
|
|
||
| /** Return whether this process was launched for a coordinated terminal handoff. */ | ||
| export function hasTerminalHandoff(env: NodeJS.ProcessEnv = process.env) { | ||
| return env[HANDOFF_ENV] === "1"; | ||
| } | ||
|
|
||
| /** Read the parent's already-detected terminal mode without querying the owned terminal again. */ | ||
| export function terminalHandoffThemeMode( | ||
| env: NodeJS.ProcessEnv = process.env, | ||
| ): "dark" | "light" | undefined { | ||
| if (!hasTerminalHandoff(env)) return undefined; | ||
| const value = env[HANDOFF_THEME_MODE_ENV]; | ||
|
Comment on lines
+13
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new handoff helpers default directly to Context Used: guidelines.mdc Cursor rule (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: src/core/process/terminalHandoff.ts
Line: 13-20
Comment:
**Environment access bypasses Varlock**
The new handoff helpers default directly to `process.env`, and the same direct access appears in the helpers at lines 70 and 100 and in `reviewLaunch.ts` at line 89. This violates the repository directive requiring type-safe, validated environment access through Varlock. Route these values through the prescribed environment boundary before merging.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No change. The cited Varlock directive belongs to a different repository and is not part of Hunk’s project guidance or environment architecture. The entire terminal-handoff module and its private environment protocol are also removed by #992. Responded by Pi using gpt-5.6-sol. This comment was generated by Pi using gpt-5.6-sol
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. The Varlock directive isn't applicable to Hunk, and #992 removes |
||
| return value === "dark" || value === "light" ? value : undefined; | ||
| } | ||
|
|
||
| /** Add the private one-shot handoff marker and terminal mode to a child environment. */ | ||
| export function terminalHandoffEnv( | ||
| env: NodeJS.ProcessEnv, | ||
| themeMode: "dark" | "light" | undefined, | ||
| ): NodeJS.ProcessEnv { | ||
| return { | ||
| ...env, | ||
| [HANDOFF_ENV]: "1", | ||
| ...(themeMode ? { [HANDOFF_THEME_MODE_ENV]: themeMode } : {}), | ||
| }; | ||
| } | ||
|
|
||
| /** Narrow an IPC payload to one bounded handoff protocol message. */ | ||
| export function parseTerminalHandoffMessage(value: unknown): TerminalHandoffMessage | undefined { | ||
| if (!value || typeof value !== "object") return undefined; | ||
| const message = value as Record<string, unknown>; | ||
| if (message.protocol !== PROTOCOL) return undefined; | ||
| if (message.kind === "ready" || message.kind === "release") { | ||
| return { protocol: PROTOCOL, kind: message.kind }; | ||
| } | ||
| if (message.kind === "failed" && typeof message.message === "string") { | ||
| return { protocol: PROTOCOL, kind: "failed", message: message.message.slice(0, 2_000) }; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| /** Build one authenticated-by-inheritance IPC message for the handoff peer. */ | ||
| export function terminalHandoffMessage(kind: "ready" | "release"): TerminalHandoffMessage { | ||
| return { protocol: PROTOCOL, kind }; | ||
| } | ||
|
|
||
| /** Tell the parent startup succeeded, then wait boundedly for exclusive terminal ownership. */ | ||
| export async function awaitTerminalHandoffRelease({ | ||
| env = process.env, | ||
| timeoutMs = 10_000, | ||
| }: { | ||
| env?: NodeJS.ProcessEnv; | ||
| timeoutMs?: number; | ||
| } = {}) { | ||
| if (!hasTerminalHandoff(env)) return; | ||
| if (typeof process.send !== "function" || !process.connected) { | ||
| throw new Error("The terminal handoff channel is unavailable."); | ||
| } | ||
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| let settled = false; | ||
| const finish = (error?: Error) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timeout); | ||
| process.off("message", onMessage); | ||
| process.off("disconnect", onDisconnect); | ||
| if (error) reject(error); | ||
| else resolve(); | ||
| }; | ||
| const onMessage = (value: unknown) => { | ||
| const message = parseTerminalHandoffMessage(value); | ||
| if (message?.kind === "release") finish(); | ||
| }; | ||
| const onDisconnect = () => finish(new Error("The terminal handoff parent disconnected.")); | ||
| const timeout = setTimeout( | ||
| () => finish(new Error("Timed out waiting for terminal ownership.")), | ||
| timeoutMs, | ||
| ); | ||
| timeout.unref?.(); | ||
| process.on("message", onMessage); | ||
| process.once("disconnect", onDisconnect); | ||
| process.send!(terminalHandoffMessage("ready"), (error) => { | ||
| if (error) finish(error); | ||
| }); | ||
| }); | ||
| process.disconnect?.(); | ||
| delete env[HANDOFF_ENV]; | ||
| delete env[HANDOFF_THEME_MODE_ENV]; | ||
| } | ||
|
|
||
| /** Report a bounded pre-render startup failure to a waiting parent. */ | ||
| export async function reportTerminalHandoffFailure( | ||
| error: unknown, | ||
| env: NodeJS.ProcessEnv = process.env, | ||
| ) { | ||
| if (!hasTerminalHandoff(env) || typeof process.send !== "function" || !process.connected) { | ||
| return false; | ||
| } | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| await new Promise<void>((resolve) => { | ||
| process.send!({ protocol: PROTOCOL, kind: "failed", message: message.slice(0, 2_000) }, () => | ||
| resolve(), | ||
| ); | ||
| }); | ||
| process.disconnect?.(); | ||
| return true; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
terminalHandoff.tsfile uses camelCase, but the repository requires dash-case names for.tsand.tsxfiles. Rename it and its imports toterminal-handoff.ts; this repository requirement must be satisfied before merging.Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change. This repository does not have a dash-case filename rule; the active project guidance and surrounding source use camelCase/PascalCase filenames extensively. In addition,
terminalHandoff.tsis deleted by #992 when the cross-process handoff is replaced with the persistent-renderer architecture.Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right — I incorrectly inferred a dash-case filename rule. The repository's active guidance and surrounding files support
terminalHandoff.ts, and since this file is removed by #992 as part of the persistent-renderer follow-up, there is no change needed for this PR. I'm retracting this comment.