feat: Extension system, Remote Gateway, macOS Desktop App, and comprehensive documentation - #2
Conversation
|
WalkthroughAdds a control plane with an extensible extensions system (gateway, supervisor, cloudflare, tailscale, tickets), gateway HTTP/WS API with token auth and audit logging, supervisor job/shell services and persistence, CLI commands and daemon/launchd integration, extensive docs/examples, Next.js gateway shell UI, macOS app scaffold, and tests. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (hack)
participant Gateway as Gateway (hackd gateway)
participant Daemon as Daemon (hackd)
participant Supervisor as SupervisorService
participant JobStore as JobStore (file)
CLI->>Gateway: POST /v1/control-plane/projects/:id/jobs (Bearer token)
Gateway->>Gateway: authenticate token, append audit entry
Gateway->>Daemon: forward create job request
Daemon->>Supervisor: createJob(projectDir, command)
Supervisor->>JobStore: create meta.json + append job.created event
Supervisor->>Supervisor: spawn process, stream stdout/stderr
Supervisor->>JobStore: append job.started / stdout lines / job.completed
CLI->>Gateway: open WS /v1/control-plane/.../jobs/:jobId/stream
Gateway->>Daemon: attach to job stream
Daemon->>Gateway: relay events/logs
Gateway->>CLI: forward WS events to client
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI Agents
In @docs/cli.md:
- Around line 1-862: Several Markdown tables have mismatched column counts
causing lint failures; inspect and correct the tables so the header row and the
divider (---) define the same number of columns as every data row. Specifically,
fix the "Top-level commands" table and the option/argument tables in the
sections for "hack global logs", "hack logs", "hack remote qr" and the later
MCP/Options tables (these are the tables the linter flagged); ensure each
table's header cells, the separating --- row, and every subsequent row have
identical pipe-separated column counts, removing or merging extra columns or
expanding the header as appropriate so rendering and the linter pass.
In @docs/guides/remote-tailscale.md:
- Around line 34-39: Add a security note explaining that binding the gateway to
0.0.0.0 (the config key controlPlane.gateway.bind) exposes the gateway on all
network interfaces and is less secure than using tailscale serve (Option A);
instruct readers to prefer Option A for isolation or, if using Option B, to
restrict access via firewall rules and explicitly state the difference in
security posture between the two options.
In @docs/sdk.md:
- Line 10: Documentation examples use incorrect relative imports for
createGatewayClient and config; update the import statements in docs/sdk.md to
reference the actual SDK module locations (e.g., import createGatewayClient from
"src/control-plane/sdk/gateway-client.ts" and import config from
"src/control-plane/sdk/config.ts"), or alternatively add a short note explaining
that users should copy the SDK files into their project and then import from the
local paths (e.g., "./gateway-client.ts" after copying); change all examples
referencing "./gateway-client.ts" and "./config.ts" to one of these accurate
options so the docs run as-is for readers.
In @docs/supervisor.md:
- Around line 36-38: Update the Markdown code fence for the WebSocket URL so it
includes a language identifier (e.g., change the opening ``` to ```text) around
the block containing
"ws://127.0.0.1:7788/control-plane/projects/$PROJECT_ID/jobs/$JOB_ID/stream" to
match the project's code block conventions and silence markdownlint warnings.
In @examples/basic/gateway-demo.ts:
- Around line 232-243: safeJsonParse currently treats any non-null object as a
Record, which mistakenly includes arrays; update the function (safeJsonParse) to
explicitly reject arrays by adding a check such as Array.isArray(parsed) and
only return the parsed value when it's an object, not null, and not an array, so
the return stays Record<string, unknown> | null; keep the empty-string guard and
try/catch behavior unchanged.
- Around line 81-99: printStatus and printProjects currently call fetch without
handling network errors; wrap the fetch/response parsing in a try-catch inside
each function (printStatus and printProjects), catch any exceptions from fetch
or res.text(), and handle them by printing a clear error message (e.g., to
process.stderr or process.stdout) and returning early so the script doesn’t
crash; ensure you still use opts.headers and opts.baseUrl and preserve existing
behavior on success.
In @examples/next-app/app/api/gateway/route.ts:
- Around line 13-31: The POST proxy reads and forwards arbitrary baseUrl and
buffers the entire upstream response into memory (SSRF and memory risks); update
the POST handler to validate/whitelist/sanitize baseUrl before calling
buildUrl/fetch (add an isAllowedBaseUrl helper that enforces schemes like
https:, blocks private IPs and metadata addresses such as 169.254.169.254, or
checks against an explicit host whitelist), and replace the uncontrolled
response.text() buffering with a bounded/streaming approach (do not call
response.text(); instead stream response.body to the client or enforce a max
byte limit with an AbortController or byte-counter and return 413 if exceeded).
Ensure you reference and plug validation into the existing
parseBody/readJsonBody/buildUrl/fetch flow in POST.
- Around line 107-109: The isRecord type guard currently returns true for arrays
because arrays are typeof "object"; update isRecord to also exclude arrays by
checking !Array.isArray(value). Replace its body with: return typeof value ===
"object" && value !== null && !Array.isArray(value) so only plain objects
satisfy the guard.
In @examples/next-app/app/gateway/page.tsx:
- Around line 389-391: The local isRecord function incorrectly treats arrays as
records; update the isRecord implementation (function isRecord) to also exclude
arrays by adding a check like !Array.isArray(value) so it returns true only for
non-null plain objects, matching the canonical guard in src/lib/guards.ts.
In @examples/next-app/next.config.ts:
- Line 4: CI type-check is failing because the examples/next-app depends on
Next.js types that aren't installed; either ensure the example's deps are
installed in CI (add a step to run "cd examples/next-app && bun install" or the
equivalent npm/yarn install before running type-check), or exclude the
examples/next-app from root TypeScript checks by updating root tsconfig.json to
remove or add "examples/**" to "exclude", or add a local tsconfig.json in
examples/next-app to opt out; update CI config to implement one of these fixes
and verify by running a full type-check after the change.
In @README.md:
- Around line 147-156: The fenced code block containing the lines "Run `hack
help` (or `hack help <command>`) for full usage." and the following command list
should either have a language identifier added (change the opening ``` to
```bash) to enable syntax highlighting, or be converted to a plain bullet list
if intended as regular prose; locate the code fence that wraps the "hack help"
and "Common:" lines and update the opening fence to "```bash" or replace the
fenced block with standard Markdown list items to match the surrounding section.
In @src/commands/config.ts:
- Around line 331-406: In parseKeyPath rename the local boolean variable escape
to a non-shadowing name (e.g. isEscaped) and update every reference/assignment
(checks, setting true/false, and the final if (escape) branch) so behavior is
unchanged; ensure the helper pushBuffer and all conditional logic in the
inBracket and non-bracket branches use the new identifier so no functionality or
control flow is altered.
In @src/control-plane/extensions/manager.ts:
- Around line 168-223: sanitizeNamespace currently calls raw.trim() and will
throw when resolveNamespace passes undefined for override/preferred; make the
namespace sanitization robust by either changing sanitizeNamespace to accept
string | undefined and coerce with (raw ?? "").trim().toLowerCase() or update
the call site in resolveNamespace to pass a safe default
(sanitizeNamespace(override ?? preferred ?? "")). Update only sanitizeNamespace
or the call in resolveNamespace (referencing sanitizeNamespace and
resolveNamespace) so undefined cliNamespace values no longer cause runtime
errors and fallback logic can run.
In @src/control-plane/extensions/supervisor/service.ts:
- Around line 109-121: The catch block for runJob can append a duplicate
"job.failed" event if runJob already recorded failure then threw; modify the
catch handler (the Promise.catch that logs error, calls store.updateJobStatus
and store.appendEvent) to first read the current job status (e.g., via
store.getJob or store.getJobStatus) and only call store.appendEvent({ type:
"job.failed", ... }) if the status is not already "failed" (leave
updateJobStatus as-is or make it idempotent). Alternatively, have runJob return
a sentinel or set a flag when it has already recorded failure and check that
flag here before appending; ensure runningJobs.delete(jobId) remains in the
finally block.
In @src/control-plane/extensions/supervisor/shell-service.ts:
- Around line 260-266: The code currently overwrites any user-provided TERM
because env["TERM"] is always set after merging opts.env; update the logic in
the shell-service merging block (the env/opts.env handling) to only set TERM
when not already provided by the merged env — e.g., after merging opts.env into
env, check if env.TERM is undefined/null and then assign process.env.TERM ??
DEFAULT_TERM — referencing the env variable, opts.env, and DEFAULT_TERM in your
change.
In @src/lib/caddy-hosts.ts:
- Around line 82-99: extractExtraHostsIp currently returns raw values from the
record branch without IPv4 validation; update the record handling in
extractExtraHostsIp to validate each value (use existing helpers
parseExtraHostEntry or isIpv4) before returning, e.g., iterate
Object.values(extraHosts), coerce non-strings, run parseExtraHostEntry(entry) or
isIpv4(value) and return the first valid IPv4 string, otherwise continue and
finally return null; keep the Array.isArray branch behavior unchanged and reuse
parseExtraHostEntry/isIpv4 to ensure consistent validation.
🧹 Nitpick comments (41)
src/mcp/install.ts (1)
261-261: Unnecessary helper adds verbosity without benefit.The
createOkParseResulthelper is used only once and makes line 261 more verbose than the original inline object literal. The helper itself is 7 lines of type declarations wrapping a 1-line return statement, which adds ceremony without improving reusability or clarity.🔎 Simplify by removing the helper
- const parsed = text ? parseJsonObject(text) : createOkParseResult({ value: {} }) + const parsed = text ? parseJsonObject(text) : { ok: true, value: {} }And remove the helper function:
-function createOkParseResult(opts: { - readonly value: Record<string, unknown> -}): { - readonly ok: true - readonly value: Record<string, unknown> -} { - return { ok: true, value: opts.value } -} - function resolveHomeDir(): string | null {Also applies to: 416-423
src/templates.ts (1)
360-438: Add validation constraints for numeric and string fields.The new
controlPlaneschema lacks validation constraints that would catch invalid configurations at load time. Consider adding:
- Port range:
portshould be between 1-65535- Positive integers:
maxConcurrentJobs,logsMaxBytes,maxEntries,maxLines,historyTailStep,watchIntervalMs,historySizeshould haveminimum: 1orminimum: 0constraints- Bind address format:
bindcould use a pattern or format constraint to validate IP addresses or hostnames🔎 Example validation constraints
gateway: { type: "object", additionalProperties: true, properties: { enabled: { type: "boolean" }, - bind: { type: "string" }, + bind: { + type: "string", + pattern: "^([0-9]{1,3}\\.){3}[0-9]{1,3}$|^[a-zA-Z0-9.-]+$" + }, - port: { type: "number" }, + port: { type: "number", minimum: 1, maximum: 65535 }, allowWrites: { type: "boolean" } } }, supervisor: { type: "object", additionalProperties: true, properties: { enabled: { type: "boolean" }, - maxConcurrentJobs: { type: "number" }, + maxConcurrentJobs: { type: "number", minimum: 1 }, - logsMaxBytes: { type: "number" } + logsMaxBytes: { type: "number", minimum: 0 } } }, usage: { type: "object", additionalProperties: true, properties: { - watchIntervalMs: { type: "number" }, + watchIntervalMs: { type: "number", minimum: 100 }, - historySize: { type: "number" } + historySize: { type: "number", minimum: 1 } } }Apply similar constraints to
tui.logsfields (maxEntries,maxLines,historyTailStep).src/control-plane/extensions/supervisor/shell-service.ts (3)
104-108: Consider wrapping listener callbacks in try-catch.If a listener's
onDatacallback throws an exception, it will interrupt data delivery to subsequent listeners in the Set. This could cause some clients to miss data if another client's handler fails.🔎 Proposed fix
data: (_term, data) => { for (const listener of listeners) { - listener.onData(data) + try { + listener.onData(data) + } catch { + // Prevent one listener from breaking others + } } },
203-227: Operations on exited shells may fail silently or throw.The
write,resize,signal, andclosemethods don't check if the shell has already exited. Depending on Bun's Terminal/process behavior, these operations could throw or be silently ignored when the underlying process is no longer running.🔎 Proposed guard
return { meta: session.meta, write: data => { + if (session.meta.status === "exited") return session.terminal.write(data) touchShell(session) }, resize: (cols, rows) => { + if (session.meta.status === "exited") return session.terminal.resize(cols, rows) session.meta = { ...session.meta, cols, rows, updatedAt: new Date().toISOString() } }, signal: signal => { + if (session.meta.status === "exited") return session.proc.kill(signal) touchShell(session) }, close: () => { + if (session.meta.status === "exited") return session.proc.kill() touchShell(session) }, detach }
291-293: WraponExitcallbacks in try-catch for resilience.Same concern as with
onData: if one listener'sonExitcallback throws, remaining listeners won't receive the exit notification.🔎 Proposed fix
for (const listener of opts.session.listeners) { - listener.onExit(opts.exitCode, null) + try { + listener.onExit(opts.exitCode, null) + } catch { + // Prevent one listener from breaking others + } }src/tui/hack-tui.ts (1)
689-736: Large mutable state surface area.This segment declares ~50 mutable state variables. While acceptable for a TUI application, consider grouping related state into objects (e.g.,
searchState,statsState,uiState) to improve maintainability and reduce the risk of inconsistent updates.docs/architecture.md (1)
152-154: Fix spacing before heading (minor formatting).Line 154 has excess whitespace before the heading. This is a minor formatting issue.
🔎 Proposed fix
-Note: the daemon does not proxy logs yet; `hack logs` still talks directly to Docker Compose or Loki. ## Daemon (hackd) +Note: the daemon does not proxy logs yet; `hack logs` still talks directly to Docker Compose or Loki. +## Daemon (hackd)src/lib/projects-registry.ts (1)
138-164: LGTM! Consider API consistency.The implementation correctly mirrors the lookup logic of
resolveRegisteredProjectByName. However, note the return type difference:
resolveRegisteredProjectByNamereturnsProjectContext | nullresolveRegisteredProjectByIdreturns{ project: ProjectContext; registration: RegisteredProject } | nullIf this inconsistency is intentional (e.g., callers looking up by ID typically need the registration data), then this is fine. Otherwise, consider aligning the return types for API consistency.
examples/next-app/app/api/gateway/route.ts (2)
33-61: LGTM! Consider modern property check.The validation logic is thorough with appropriate error codes. Line 58 uses the verbose
Object.prototype.hasOwnProperty.call()pattern.🔎 Optional modernization
Consider using the more concise
Object.hasOwn()(ES2022+):-...(Object.prototype.hasOwnProperty.call(opts.value, "body") ? { body: opts.value.body } : {}) +...(Object.hasOwn(opts.value, "body") ? { body: opts.value.body } : {})Note: Ensure your target environment supports ES2022, or stick with the current approach for compatibility.
111-114: Consider aligning with the codebase's getString implementation.This
getStringimplementation differs fromsrc/lib/guards.ts:
- Returns
nullinstead ofundefinedfor missing/non-string values- Adds
.trim()which may be desired here but creates inconsistencyIf these differences are intentional for this specific use case, this is fine. Otherwise, consider using the shared utility from
src/lib/guards.tsto maintain consistency.tests/cloudflare-commands.test.ts (3)
9-37: Consider expanding test coverage for parseTunnelPrintArgs.The existing tests cover basic success and failure paths well. Consider adding tests for:
- Equals syntax:
--hostname=value- Missing values:
--hostnamewithout a value- Other supported flags:
--tunnel,--origin,--credentials-file- Multiple flag combinations
Example additional tests
test("parseTunnelPrintArgs parses equals syntax", () => { const result = parseTunnelPrintArgs({ args: ["--hostname=gateway.example.com", "--out=./config.yml"] }) expect(result.ok).toBe(true) if (!result.ok) return expect(result.value.hostname).toBe("gateway.example.com") expect(result.value.out).toBe("./config.yml") }) test("parseTunnelPrintArgs rejects missing value", () => { const result = parseTunnelPrintArgs({ args: ["--hostname"] }) expect(result.ok).toBe(false) })
39-52: Consider expanding test coverage for parseTunnelStartArgs.The tests cover the basic path. Consider adding:
- Equals syntax:
--config=value- Missing value handling
- The
--outflag (which maps toconfigbased on the parser implementation)Example additional tests
test("parseTunnelStartArgs parses out flag as config", () => { const result = parseTunnelStartArgs({ args: ["--out", "./my-config.yml"] }) expect(result.ok).toBe(true) if (!result.ok) return expect(result.value.config).toBe("./my-config.yml") }) test("parseTunnelStartArgs rejects missing value", () => { const result = parseTunnelStartArgs({ args: ["--config"] }) expect(result.ok).toBe(false) })
54-67: Consider expanding test coverage for parseAccessSetupArgs.The tests cover basic functionality. Consider adding:
- Equals syntax:
--ssh-hostname=value- Missing value handling
- Individual flag usage (only
--ssh-hostnameor only--user)Example additional tests
test("parseAccessSetupArgs parses equals syntax", () => { const result = parseAccessSetupArgs({ args: ["--ssh-hostname=ssh.example.com", "--user=dimitri"] }) expect(result.ok).toBe(true) if (!result.ok) return expect(result.value.sshHostname).toBe("ssh.example.com") expect(result.value.user).toBe("dimitri") }) test("parseAccessSetupArgs rejects missing value", () => { const result = parseAccessSetupArgs({ args: ["--ssh-hostname"] }) expect(result.ok).toBe(false) })src/control-plane/extensions/cloudflare/README.md (2)
162-166: Add language specifier to code block.The fenced code block is missing a language specifier, which improves syntax highlighting and documentation rendering.
🔎 Suggested fix
-``` +```text Host ssh.example.com User <user> ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h</details> --- `187-190`: **Add language specifier to code block.** The fenced code block is missing a language specifier for better documentation rendering. <details> <summary>🔎 Suggested fix</summary> ```diff -``` +```http CF-Access-Client-Id: <client-id> CF-Access-Client-Secret: <client-secret></details> </blockquote></details> <details> <summary>docs/guides/remote-cloudflare.md (1)</summary><blockquote> `56-60`: **Add language specifier to code fence.** The SSH config code block is missing a language identifier for proper syntax highlighting. <details> <summary>🔎 Suggested fix</summary> ```diff -``` +```ssh-config Host ssh.example.com User <user> ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h</details> </blockquote></details> <details> <summary>src/control-plane/extensions/builtins.ts (1)</summary><blockquote> `7-13`: **Consider adding an explicit type annotation.** While TypeScript inference works with `as const`, an explicit type annotation improves maintainability and IDE support: <details> <summary>🔎 Proposed enhancement</summary> ```diff +import type { ExtensionDefinition } from "./types.ts" + export const BUILTIN_EXTENSIONS = [ TICKETS_EXTENSION, SUPERVISOR_EXTENSION, GATEWAY_EXTENSION, CLOUDFLARE_EXTENSION, TAILSCALE_EXTENSION -] as const +] as const satisfies readonly ExtensionDefinition[]src/constants.ts (1)
38-41: Consider adding JSDoc comments for clarity.The purpose of
GLOBAL_ONLY_EXTENSION_IDSisn't immediately obvious. Adding documentation would help future maintainers understand why these specific extensions are restricted to global scope and how this list is used.🔎 Suggested documentation
+/** + * Extension IDs that can only be enabled in global scope. + * These extensions require machine-wide access and cannot be project-scoped. + */ export const GLOBAL_ONLY_EXTENSION_IDS = [ "dance.hack.cloudflare", "dance.hack.tailscale" ] as constdocs/extensions.md (2)
93-93: Add language identifier to fenced code block.For better syntax highlighting and documentation consistency, specify the language identifier for this code block.
🔎 Proposed fix
-``` +```bash hack x <namespace> <command> [args...]</details> --- `377-383`: **Add language identifier to JSON examples.** For better syntax highlighting and documentation consistency, specify `json` as the language identifier for these code blocks. <details> <summary>🔎 Proposed fix</summary> ```diff -``` +```json {"type":"start","jobId":"...","logsOffset":0,"eventsSeq":0} {"type":"log","stream":"combined","offset":128,"data":"..."} ...Apply the same fix to the code blocks at lines 391 and 401. </details> </blockquote></details> <details> <summary>docs/gateway-api.md (2)</summary><blockquote> `103-105`: **Add language identifier to fenced code block.** For better syntax highlighting, specify the language identifier for this code block. <details> <summary>🔎 Proposed fix</summary> ```diff -``` +```text https://gateway.dimitri.computerApply the same fix to the code block at line 147. </details> --- `319-322`: **Add blank lines around table for Markdown formatting.** Markdown tables should be surrounded by blank lines for proper rendering across different parsers. <details> <summary>🔎 Proposed fix</summary> ```diff
Response:
| Field | Type | Description |Apply the same fix to the table at line 385. </details> </blockquote></details> <details> <summary>src/control-plane/extensions/gateway/audit.ts (1)</summary><blockquote> `34-35`: **Silent error swallowing hides audit failures.** The empty `catch` block silently discards all errors. While audit logging shouldn't crash the gateway, completely hiding failures makes it impossible to diagnose issues (e.g., disk full, permission errors). Consider logging the error at debug/warn level or emitting a metric. <details> <summary>Proposed fix</summary> ```diff } catch { + // Audit writes are best-effort; log for observability but don't propagate. + // Consider: ctx.logger?.warn({ message: "Audit write failed", error }) }src/control-plane/extensions/tailscale/commands.ts (1)
62-68: Platform-specific install suggestion.The error message suggests
brew install tailscale, which is macOS-specific. On Linux, users typically install via their package manager or the official script. Consider a more generic message or detecting the platform.Proposed fix
async function ensureTailscale(): Promise<{ readonly ok: true } | { readonly ok: false; readonly error: string }> { const exitCode = await runTailscale({ args: ["--version"], inherit: false }) if (exitCode !== 0) { - return { ok: false, error: "tailscale not found. Install with: brew install tailscale" } + return { ok: false, error: "tailscale not found. See https://tailscale.com/download for install instructions." } } return { ok: true } }src/commands/doctor.ts (1)
435-457: Minor: duplicateresolveGatewayConfig()call.
resolveGatewayConfig()is called here and also incheckGatewayConfig(). Since doctor checks run sequentially, this results in redundant config resolution. Consider passing the resolved config or caching it, though this is a minor optimization given doctor runs infrequently.src/lib/caddy-hosts.ts (1)
112-118: IPv4 validation allows leading zeros.The regex
^\d{1,3}(\.\d{1,3}){3}$allows values like192.168.001.001which, while technically parseable, can cause issues in some contexts where leading zeros are interpreted as octal. Consider whether stricter validation is needed.src/control-plane/extensions/gateway/commands.ts (1)
45-75:token-listhandler ignoresargsparameter.The
argsparameter is destructured but never used. If this is intentional (no filtering/options supported), consider using_argsor omitting it to signal intent.🔎 Proposed fix
- handler: async ({ args }) => { + handler: async () => {examples/basic/gateway-demo.ts (1)
176-181: Consider handling Blob type explicitly.The message data handling covers
stringandArrayBuffer, but the fallback.toString()may not work correctly forBlobin all environments. In Bun, WebSocket messages are typically strings or ArrayBuffers, so this is likely fine for this demo.src/control-plane/extensions/supervisor/runner.ts (1)
76-87: Potential race condition when writing to combined log file.Both
stdoutTaskandstderrTaskwrite topaths.combinedPathconcurrently viaappendFile. WhileappendFileis atomic at the OS level for small writes, interleaved chunks from stdout and stderr may result in mixed/garbled lines in the combined log.Consider serializing writes to the combined file using a queue or mutex, or accept that combined output may have interleaved chunks (which may be acceptable for debugging purposes).
src/daemon/server.ts (1)
1070-1090: Consider caching the job store instance for stream sessions.
createJobStoreis called on every log/event flush (every 500ms). WhileensureDiris likely a no-op for existing directories, repeatedly instantiating the store adds minor overhead. Consider creating the store once instartJobStreamand reusing it in flush functions.src/commands/x.ts (1)
58-61: Returning exit code 1 for help display may be unexpected.When no namespace is provided,
renderDispatcherHelpis called and the function returns1. Typically, help displays return0to indicate successful completion. Consider returning0here for consistency with other help flows.🔎 Proposed fix
if (!invocation.namespace) { await renderDispatcherHelp({ extensions: loaded.manager.listExtensions() }) - return 1 + return 0 }src/control-plane/extensions/gateway/tokens.ts (2)
150-167: Consider timing-safe comparison for token hash verification.The hash comparison on line 156 uses JavaScript's
===operator, which is not constant-time. While exploiting this timing side-channel is difficult (attacker would need to guess valid SHA256 hashes), using a timing-safe comparison function likecrypto.timingSafeEqualwould be more defense-in-depth.🔎 Proposed fix
+import { timingSafeEqual } from "node:crypto" +function safeHashCompare(a: string, b: string): boolean { + const bufA = Buffer.from(a, 'hex') + const bufB = Buffer.from(b, 'hex') + if (bufA.length !== bufB.length) return false + return timingSafeEqual(bufA, bufB) +} export async function verifyGatewayToken(opts: { readonly rootDir: string readonly token: string }): Promise<GatewayTokenRecord | null> { const store = await readGatewayTokenStore({ rootDir: opts.rootDir }) const hash = hashToken({ token: opts.token }) - const match = store.tokens.find(token => token.hash === hash && !token.revokedAt) ?? null + const match = store.tokens.find(token => safeHashCompare(token.hash, hash) && !token.revokedAt) ?? null if (!match) return null
128-130: Redundant null check on line 129.Line 128 retrieves
store.tokens[idx]whereidxwas found viafindIndex. Ifidx !== -1(checked on line 126),existingcannot benull. The check on line 129 is defensive but unnecessary.src/commands/usage.ts (2)
68-75: Surface control-plane parse errors instead of silently falling back
readControlPlaneConfig({})is used here purely forconfig.usage, and anyparseErrorfrom malformed control-plane config is ignored. That means a broken global config quietly degrades to defaults, which can be confusing when tweaking watch intervals/history.Consider logging or displaying
parseError(e.g. vialoggeror adisplay.panelin non‑JSON mode) so operators get immediate feedback when the config is invalid.
915-922: Avoid reimplementingisRecord/getStringlocallyThis file defines its own
isRecord/getStringwhilesrc/lib/guards.tsalready exports equivalent helpers (with a slightly stricter “not an array” check). Reusing the shared versions would keep semantics consistent across the codebase and avoid drift if those helpers ever evolve.src/control-plane/sdk/config.ts (1)
204-225: Gatewayenabledflag is derived only from project config, ignoring globalenabledIn
mergeControlPlaneLayersthe merged gateway block is constructed as:
globalGatewayfrom the global layer (bind/port/allowWrites/etc),projectGatewayfrom the project layer,gatewayEnabledderived solely fromprojectGateway.enabled(orfalsewhen unset),- then
merged.gateway = { ...globalGateway, enabled: gatewayEnabled }.This means:
- Any
controlPlane.gateway.enabledvalue set in the global config is discarded.config.gateway.enabledis alwaysfalsewhenreadControlPlaneConfigis called without aprojectDir, and for projects that don’t definegateway.enabledthemselves.Given
ExtensionManager.resolveNamespacespecial-casesdance.hack.gatewayby looking atconfig.gateway.enabled, this likely doesn’t behave as intended: togglingcontrolPlane.gateway.enabledglobally will never flip that flag.You may want to either:
- Honor the global
enabledwhen the project layer omits it (e.g. “project overrides, otherwise global”), or- Treat gateway enablement purely as an extension flag (
extensions["dance.hack.gateway"].enabled) and remove theconfig.gateway.enabledcoupling.Right now the combination is asymmetric and could surprise users configuring the gateway from the global config only.
src/commands/remote.ts (2)
421-479: Avoid duplicate control-plane config reads and consider exposing parse errors in status
collectRemoteStatusSnapshotalready reads the per‑project control-plane config to determineprojectGatewayEnabled. Later, it callsresolveCloudflareStatuswith(await readControlPlaneConfig({})).config, which re‑parses the global config on every snapshot and ignores anyparseError.In the
remote statusand especiallyremote monitorflows, this means:
- Extra JSON reads + zod parsing on each refresh.
- Malformed control-plane config is silently treated as “no Cloudflare config”, with no warning to the user.
You could instead:
- Read global + project control-plane config once at the call site, pass the global
config(and optionalparseError) down into bothcollectRemoteStatusSnapshotandresolveCloudflareStatus, and- Log or surface
parseErrorvia a warning panel in the status output so configuration issues are visible.This keeps the status output accurate while avoiding redundant IO/parsing in tight loops.
788-1006: Protect monitor timers from unhandled async errorsIn
runRemoteMonitor, both timers use async callbacks without error handling:
statusTimer = setInterval(() => void renderStatus(), 2_000)auditTimer = setInterval(() => void updateAuditLines(), 800)If
collectRemoteStatusSnapshot,readAuditLines, or any nested IO throws, those rejections will be unhandled (thevoiddoesn’t catch them) and can take down the process.A lightweight pattern is to wrap the calls in a small try/catch:
statusTimer = setInterval(() => { void (async () => { try { await renderStatus() } catch (err) { logger.warn({ message: `remote monitor status update failed: ${String(err)}` }) } })() }, 2_000)and similarly for
updateAuditLines. That keeps the TUI responsive even when a single poll fails.src/control-plane/extensions/supervisor/job-store.ts (1)
91-166: Be explicit about duplicate job IDs and concurrent meta/event writes
createJobalways creates/overwrites the job directory andmeta.jsonfor the suppliedjobId, and bothupdateJobStatusandappendEventInternalfollow a read‑modify‑write pattern onmeta.jsonwhileappendEventInternalalso appends toevents.jsonl.If:
createJobis accidentally called twice with the samejobId, or- Multiple async flows update status / append events for the same job concurrently,
you can end up with:
- Previous meta/events silently overwritten on job creation retry.
- Lost
lastEventSequpdates or duplicated sequence numbers when two writers race onmeta.json.If the supervisor is strictly single‑writer per job (one process / one code path), this is fine, but it would be safer to:
- Fail
createJobifmeta.jsonalready exists for that id, and/or- In
appendEvent/updateJobStatus, re‑readmeta.jsonjust before writing and serialize operations per job (e.g. via an in‑process per‑job mutex) if concurrent writes are possible.That would make the store more robust to future usage patterns without changing the public API.
Also applies to: 213-237
src/control-plane/sdk/gateway-client.ts (2)
304-321: Consider sanitizing path parameters to prevent URL manipulation.User-provided
projectId,jobId, andshellIdvalues are directly interpolated into URL paths. If these contain URL-special characters (e.g.,../,%2F,?), it could lead to unintended path traversal or query injection.🔎 Proposed fix: Add URL-safe encoding for path segments
Add a helper function and use it for all dynamic path segments:
function encodePathSegment(value: string): string { return encodeURIComponent(value) }Then update the path constructions:
- path: `/control-plane/projects/${opts.projectId}/jobs`, + path: `/control-plane/projects/${encodePathSegment(opts.projectId)}/jobs`,Apply similar changes to all path interpolations in
listJobs,getJob,createJob,cancelJob,createShell,getShell,openJobStream, andopenShellStream.Also applies to: 342-350, 373-381, 383-402
479-489: Shallow validation in parse functions may allow malformed data through.
parseStatusvalidatesstatusandversionbut the type also requirespid,started_at, anduptime_ms. Similarly,parseMetricsonly checksstatus. Theascasts assume all fields exist.If the server returns partial data, runtime errors could occur when consumers access missing properties.
🔎 Proposed fix: Validate all required fields
function parseStatus(value: unknown): GatewayStatus | null { if (!isRecord(value)) return null - if (typeof value["status"] !== "string" || typeof value["version"] !== "string") return null + if ( + typeof value["status"] !== "string" || + typeof value["version"] !== "string" || + typeof value["pid"] !== "number" || + typeof value["started_at"] !== "string" || + typeof value["uptime_ms"] !== "number" + ) return null return value as GatewayStatus } function parseMetrics(value: unknown): GatewayMetrics | null { if (!isRecord(value)) return null - if (typeof value["status"] !== "string") return null + if ( + typeof value["status"] !== "string" || + typeof value["started_at"] !== "string" || + typeof value["uptime_ms"] !== "number" || + typeof value["refresh_count"] !== "number" || + typeof value["refresh_failures"] !== "number" || + typeof value["events_seen"] !== "number" || + typeof value["streams_active"] !== "number" + ) return null return value as GatewayMetrics }
| Create a typed HTTP/WS client for gateway orchestration: | ||
|
|
||
| ```ts | ||
| import { createGatewayClient } from "./gateway-client.ts" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the docs/sdk.md file to see the imports in context
head -20 docs/sdk.md
echo "---"
tail -n +85 docs/sdk.md | head -20Repository: hack-dance/hack-cli
Length of output: 1160
🏁 Script executed:
# Check SDK structure and package.json exports
cat package.json | jq '.exports, .main, .module' 2>/dev/null || cat package.json
echo "---"
# Look for SDK entry points
fd -t f \( -name 'index.ts' -o -name 'mod.ts' -o -name 'gateway-client.ts' -o -name 'config.ts' \) src/Repository: hack-dance/hack-cli
Length of output: 335
🏁 Script executed:
# Read package.json to see actual exports configuration
cat package.jsonRepository: hack-dance/hack-cli
Length of output: 1888
🏁 Script executed:
# Check the src/ directory structure to see what files exist
find src -type f -name '*.ts' -o -name '*.tsx' | head -30Repository: hack-dance/hack-cli
Length of output: 735
🏁 Script executed:
# Search for gateway-client.ts and config.ts files anywhere in the repo
fd -t f 'gateway-client.ts|config.ts'Repository: hack-dance/hack-cli
Length of output: 237
🏁 Script executed:
# Check if there's a docs/ directory structure
ls -la docs/ 2>/dev/null | head -20Repository: hack-dance/hack-cli
Length of output: 738
🏁 Script executed:
# Look for SDK directory or similar
find . -type d -name '*sdk*' -o -type d -name '*gateway*' 2>/dev/nullRepository: hack-dance/hack-cli
Length of output: 193
🏁 Script executed:
# Check if sdk.md is in src/control-plane/sdk/ directory
find . -name 'sdk.md' -type fRepository: hack-dance/hack-cli
Length of output: 76
🏁 Script executed:
# Check the structure of src/control-plane/sdk/
ls -la src/control-plane/sdk/Repository: hack-dance/hack-cli
Length of output: 364
🏁 Script executed:
# Check if there's an index.ts in src/control-plane/sdk/
cat src/control-plane/sdk/index.ts 2>/dev/null || echo "No index.ts found"Repository: hack-dance/hack-cli
Length of output: 80
🏁 Script executed:
# Check the README in src/control-plane/sdk/
cat src/control-plane/sdk/README.mdRepository: hack-dance/hack-cli
Length of output: 121
🏁 Script executed:
# Verify the exact imports shown in docs/sdk.md and check if they would work
cat -n docs/sdk.md | grep -A2 -B2 'import.*gateway-client\|import.*config'Repository: hack-dance/hack-cli
Length of output: 384
🏁 Script executed:
# Check if there's any package export configuration or SDK build output
cat src/control-plane/sdk/README.mdRepository: hack-dance/hack-cli
Length of output: 121
🏁 Script executed:
# Look at the actual exports from gateway-client.ts to understand the API
head -50 src/control-plane/sdk/gateway-client.tsRepository: hack-dance/hack-cli
Length of output: 1534
Update documentation import paths to match actual file structure.
The code examples in docs/sdk.md use relative import paths ("./gateway-client.ts" and "./config.ts" at lines 10 and 92) that don't work for users reading the documentation. The actual SDK files are located at src/control-plane/sdk/, so users would need either:
- Correct relative paths from the repository root:
"./src/control-plane/sdk/gateway-client.ts"and"./src/control-plane/sdk/config.ts" - Or clear documentation explaining how to import these modules after copying them to their project
Update the examples to use accurate import paths so users can follow the documentation without errors.
🤖 Prompt for AI Agents
In @docs/sdk.md at line 10, Documentation examples use incorrect relative
imports for createGatewayClient and config; update the import statements in
docs/sdk.md to reference the actual SDK module locations (e.g., import
createGatewayClient from "src/control-plane/sdk/gateway-client.ts" and import
config from "src/control-plane/sdk/config.ts"), or alternatively add a short
note explaining that users should copy the SDK files into their project and then
import from the local paths (e.g., "./gateway-client.ts" after copying); change
all examples referencing "./gateway-client.ts" and "./config.ts" to one of these
accurate options so the docs run as-is for readers.
| ``` | ||
| ws://127.0.0.1:7788/control-plane/projects/$PROJECT_ID/jobs/$JOB_ID/stream | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to the code block.
The WebSocket URL code block is missing a language identifier. Consider adding text or bash for consistency with the rest of the documentation.
🔎 Proposed fix
-```
-ws://127.0.0.1:7788/control-plane/projects/$PROJECT_ID/jobs/$JOB_ID/stream
-```
+```text
+ws://127.0.0.1:7788/control-plane/projects/$PROJECT_ID/jobs/$JOB_ID/stream
+```Based on static analysis hints from markdownlint-cli2.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| ws://127.0.0.1:7788/control-plane/projects/$PROJECT_ID/jobs/$JOB_ID/stream | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @docs/supervisor.md around lines 36 - 38, Update the Markdown code fence for
the WebSocket URL so it includes a language identifier (e.g., change the opening
``` to ```text) around the block containing
"ws://127.0.0.1:7788/control-plane/projects/$PROJECT_ID/jobs/$JOB_ID/stream" to
match the project's code block conventions and silence markdownlint warnings.
| async function printStatus(opts: { | ||
| readonly baseUrl: string | ||
| readonly headers: Record<string, string> | ||
| }): Promise<void> { | ||
| const res = await fetch(new URL("/v1/status", opts.baseUrl), { headers: opts.headers }) | ||
| const body = await res.text() | ||
| process.stdout.write(`Status: ${body}\n`) | ||
| } | ||
|
|
||
| async function printProjects(opts: { | ||
| readonly baseUrl: string | ||
| readonly headers: Record<string, string> | ||
| }): Promise<void> { | ||
| const url = new URL("/v1/projects", opts.baseUrl) | ||
| url.searchParams.set("include_unregistered", "true") | ||
| const res = await fetch(url, { headers: opts.headers }) | ||
| const body = await res.text() | ||
| process.stdout.write(`Projects: ${body}\n`) | ||
| } |
There was a problem hiding this comment.
HTTP requests lack error handling for network failures.
The printStatus and printProjects functions don't handle fetch exceptions. If the gateway is unreachable, the script will crash with an unhandled error.
🔎 Proposed fix to add try-catch
async function printStatus(opts: {
readonly baseUrl: string
readonly headers: Record<string, string>
}): Promise<void> {
- const res = await fetch(new URL("/v1/status", opts.baseUrl), { headers: opts.headers })
- const body = await res.text()
- process.stdout.write(`Status: ${body}\n`)
+ try {
+ const res = await fetch(new URL("/v1/status", opts.baseUrl), { headers: opts.headers })
+ const body = await res.text()
+ process.stdout.write(`Status: ${body}\n`)
+ } catch (error) {
+ const message = error instanceof Error ? error.message : "Unknown error"
+ process.stderr.write(`Failed to fetch status: ${message}\n`)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @examples/basic/gateway-demo.ts around lines 81 - 99, printStatus and
printProjects currently call fetch without handling network errors; wrap the
fetch/response parsing in a try-catch inside each function (printStatus and
printProjects), catch any exceptions from fetch or res.text(), and handle them
by printing a clear error message (e.g., to process.stderr or process.stdout)
and returning early so the script doesn’t crash; ensure you still use
opts.headers and opts.baseUrl and preserve existing behavior on success.
| function parseKeyPath(opts: { readonly raw: string }): readonly string[] { | ||
| return opts.raw | ||
| .split(".") | ||
| .map(part => part.trim()) | ||
| .filter(part => part.length > 0) | ||
| const parts: string[] = [] | ||
| let buffer = "" | ||
| let escape = false | ||
| let inBracket = false | ||
| let quote: "\"" | "'" | null = null | ||
|
|
||
| const pushBuffer = () => { | ||
| const trimmed = buffer.trim() | ||
| if (trimmed.length > 0) parts.push(trimmed) | ||
| buffer = "" | ||
| } | ||
|
|
||
| for (let i = 0; i < opts.raw.length; i += 1) { | ||
| const ch = opts.raw[i] ?? "" | ||
| if (inBracket) { | ||
| if (escape) { | ||
| buffer += ch | ||
| escape = false | ||
| continue | ||
| } | ||
| if (ch === "\\") { | ||
| escape = true | ||
| continue | ||
| } | ||
| if (quote) { | ||
| if (ch === quote) { | ||
| quote = null | ||
| continue | ||
| } | ||
| buffer += ch | ||
| continue | ||
| } | ||
| if (ch === "'" || ch === "\"") { | ||
| quote = ch | ||
| continue | ||
| } | ||
| if (ch === "]") { | ||
| inBracket = false | ||
| pushBuffer() | ||
| continue | ||
| } | ||
| buffer += ch | ||
| continue | ||
| } | ||
|
|
||
| if (escape) { | ||
| buffer += ch | ||
| escape = false | ||
| continue | ||
| } | ||
| if (ch === "\\") { | ||
| escape = true | ||
| continue | ||
| } | ||
| if (ch === ".") { | ||
| pushBuffer() | ||
| continue | ||
| } | ||
| if (ch === "[") { | ||
| if (buffer.trim().length > 0) { | ||
| pushBuffer() | ||
| } else { | ||
| buffer = "" | ||
| } | ||
| inBracket = true | ||
| continue | ||
| } | ||
| buffer += ch | ||
| } | ||
|
|
||
| if (escape) buffer += "\\" | ||
| if (buffer.length > 0) pushBuffer() | ||
|
|
||
| return parts | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename escape variable to avoid shadowing the global.
The static analysis tool correctly flags that escape shadows the global escape property. While the global is deprecated, shadowing it can cause confusion and trigger linter warnings.
🔎 Proposed fix: Rename to `isEscaped`
function parseKeyPath(opts: { readonly raw: string }): readonly string[] {
const parts: string[] = []
let buffer = ""
- let escape = false
+ let isEscaped = false
let inBracket = false
let quote: "\"" | "'" | null = null
// ... (update all usages)
for (let i = 0; i < opts.raw.length; i += 1) {
const ch = opts.raw[i] ?? ""
if (inBracket) {
- if (escape) {
+ if (isEscaped) {
buffer += ch
- escape = false
+ isEscaped = false
continue
}
if (ch === "\\") {
- escape = true
+ isEscaped = true
continue
}
// ... rest of bracket handling
}
- if (escape) {
+ if (isEscaped) {
buffer += ch
- escape = false
+ isEscaped = false
continue
}
if (ch === "\\") {
- escape = true
+ isEscaped = true
continue
}
// ... rest of handling
}
- if (escape) buffer += "\\"
+ if (isEscaped) buffer += "\\"
if (buffer.length > 0) pushBuffer()
return parts
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseKeyPath(opts: { readonly raw: string }): readonly string[] { | |
| return opts.raw | |
| .split(".") | |
| .map(part => part.trim()) | |
| .filter(part => part.length > 0) | |
| const parts: string[] = [] | |
| let buffer = "" | |
| let escape = false | |
| let inBracket = false | |
| let quote: "\"" | "'" | null = null | |
| const pushBuffer = () => { | |
| const trimmed = buffer.trim() | |
| if (trimmed.length > 0) parts.push(trimmed) | |
| buffer = "" | |
| } | |
| for (let i = 0; i < opts.raw.length; i += 1) { | |
| const ch = opts.raw[i] ?? "" | |
| if (inBracket) { | |
| if (escape) { | |
| buffer += ch | |
| escape = false | |
| continue | |
| } | |
| if (ch === "\\") { | |
| escape = true | |
| continue | |
| } | |
| if (quote) { | |
| if (ch === quote) { | |
| quote = null | |
| continue | |
| } | |
| buffer += ch | |
| continue | |
| } | |
| if (ch === "'" || ch === "\"") { | |
| quote = ch | |
| continue | |
| } | |
| if (ch === "]") { | |
| inBracket = false | |
| pushBuffer() | |
| continue | |
| } | |
| buffer += ch | |
| continue | |
| } | |
| if (escape) { | |
| buffer += ch | |
| escape = false | |
| continue | |
| } | |
| if (ch === "\\") { | |
| escape = true | |
| continue | |
| } | |
| if (ch === ".") { | |
| pushBuffer() | |
| continue | |
| } | |
| if (ch === "[") { | |
| if (buffer.trim().length > 0) { | |
| pushBuffer() | |
| } else { | |
| buffer = "" | |
| } | |
| inBracket = true | |
| continue | |
| } | |
| buffer += ch | |
| } | |
| if (escape) buffer += "\\" | |
| if (buffer.length > 0) pushBuffer() | |
| return parts | |
| } | |
| function parseKeyPath(opts: { readonly raw: string }): readonly string[] { | |
| const parts: string[] = [] | |
| let buffer = "" | |
| let isEscaped = false | |
| let inBracket = false | |
| let quote: "\"" | "'" | null = null | |
| const pushBuffer = () => { | |
| const trimmed = buffer.trim() | |
| if (trimmed.length > 0) parts.push(trimmed) | |
| buffer = "" | |
| } | |
| for (let i = 0; i < opts.raw.length; i += 1) { | |
| const ch = opts.raw[i] ?? "" | |
| if (inBracket) { | |
| if (isEscaped) { | |
| buffer += ch | |
| isEscaped = false | |
| continue | |
| } | |
| if (ch === "\\") { | |
| isEscaped = true | |
| continue | |
| } | |
| if (quote) { | |
| if (ch === quote) { | |
| quote = null | |
| continue | |
| } | |
| buffer += ch | |
| continue | |
| } | |
| if (ch === "'" || ch === "\"") { | |
| quote = ch | |
| continue | |
| } | |
| if (ch === "]") { | |
| inBracket = false | |
| pushBuffer() | |
| continue | |
| } | |
| buffer += ch | |
| continue | |
| } | |
| if (isEscaped) { | |
| buffer += ch | |
| isEscaped = false | |
| continue | |
| } | |
| if (ch === "\\") { | |
| isEscaped = true | |
| continue | |
| } | |
| if (ch === ".") { | |
| pushBuffer() | |
| continue | |
| } | |
| if (ch === "[") { | |
| if (buffer.trim().length > 0) { | |
| pushBuffer() | |
| } else { | |
| buffer = "" | |
| } | |
| inBracket = true | |
| continue | |
| } | |
| buffer += ch | |
| } | |
| if (isEscaped) buffer += "\\" | |
| if (buffer.length > 0) pushBuffer() | |
| return parts | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 334-334: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In @src/commands/config.ts around lines 331 - 406, In parseKeyPath rename the
local boolean variable escape to a non-shadowing name (e.g. isEscaped) and
update every reference/assignment (checks, setting true/false, and the final if
(escape) branch) so behavior is unchanged; ensure the helper pushBuffer and all
conditional logic in the inBracket and non-bracket branches use the new
identifier so no functionality or control flow is altered.
| function resolveNamespace(opts: { | ||
| readonly extension: ExtensionDefinition | ||
| readonly config: ControlPlaneConfig | ||
| readonly used: ReadonlySet<string> | ||
| }): NamespaceResolution { | ||
| const preferred = opts.extension.manifest.cliNamespace | ||
| const extensionConfig = opts.config.extensions?.[opts.extension.manifest.id] | ||
| const override = | ||
| isRecord(extensionConfig) && typeof extensionConfig["cliNamespace"] === "string" ? | ||
| extensionConfig["cliNamespace"] | ||
| : undefined | ||
| const enabled = | ||
| (isRecord(extensionConfig) && extensionConfig["enabled"] === true ? true : false) || | ||
| (opts.extension.manifest.id === "dance.hack.gateway" && opts.config.gateway.enabled === true) | ||
| const desired = sanitizeNamespace(override ?? preferred) | ||
|
|
||
| if (desired.length > 0 && !opts.used.has(desired) && !RESERVED_NAMESPACES.has(desired)) { | ||
| return { | ||
| resolved: { ...opts.extension, namespace: desired, enabled } | ||
| } | ||
| } | ||
|
|
||
| const fallback = buildFallbackNamespace({ | ||
| base: desired.length > 0 ? desired : "ext", | ||
| extensionId: opts.extension.manifest.id, | ||
| used: opts.used | ||
| }) | ||
|
|
||
| return { | ||
| resolved: { ...opts.extension, namespace: fallback, enabled }, | ||
| warning: | ||
| desired.length === 0 ? | ||
| `Extension namespace missing for ${opts.extension.manifest.id}; using ${fallback}` | ||
| : `Namespace "${desired}" already used; using ${fallback} for ${opts.extension.manifest.id}` | ||
| } | ||
| } | ||
|
|
||
| function buildFallbackNamespace(opts: { | ||
| readonly base: string | ||
| readonly extensionId: string | ||
| readonly used: ReadonlySet<string> | ||
| }): string { | ||
| const hash = createHash("sha1").update(opts.extensionId).digest("hex").slice(0, 6) | ||
| const fallback = `${opts.base}.${hash}` | ||
| if (!opts.used.has(fallback) && !RESERVED_NAMESPACES.has(fallback)) { | ||
| return fallback | ||
| } | ||
| return `${opts.base}.${hash}.${Date.now().toString(36)}` | ||
| } | ||
|
|
||
| function sanitizeNamespace(raw: string): string { | ||
| const trimmed = raw.trim().toLowerCase() | ||
| const replaced = trimmed.replaceAll("_", "-").replaceAll(" ", "-") | ||
| const cleaned = replaced.replaceAll(/[^a-z0-9-]/g, "") | ||
| return cleaned.replaceAll(/-+/g, "-").replaceAll(/^-|-$/g, "") | ||
| } |
There was a problem hiding this comment.
Harden sanitizeNamespace against missing cliNamespace / overrides
resolveNamespace currently does:
const preferred = opts.extension.manifest.cliNamespace
…
const desired = sanitizeNamespace(override ?? preferred)while sanitizeNamespace assumes a concrete string and immediately calls raw.trim(). If an extension definition has no manifest.cliNamespace and there’s no cliNamespace override in config, override ?? preferred will be undefined, and this call will throw at runtime.
To keep namespace resolution robust (especially for future extensions that may intentionally omit a CLI namespace), consider one of:
- Supplying a safe default at the call site, e.g.
const desired = sanitizeNamespace(override ?? preferred ?? ""), or - Widening
sanitizeNamespaceto acceptstring | undefinedand coerce internally:
const trimmed = (raw ?? "").trim().toLowerCase().
That way extensions without an explicit namespace still resolve cleanly via the fallback logic instead of crashing the manager.
🤖 Prompt for AI Agents
In @src/control-plane/extensions/manager.ts around lines 168 - 223,
sanitizeNamespace currently calls raw.trim() and will throw when
resolveNamespace passes undefined for override/preferred; make the namespace
sanitization robust by either changing sanitizeNamespace to accept string |
undefined and coerce with (raw ?? "").trim().toLowerCase() or update the call
site in resolveNamespace to pass a safe default (sanitizeNamespace(override ??
preferred ?? "")). Update only sanitizeNamespace or the call in resolveNamespace
(referencing sanitizeNamespace and resolveNamespace) so undefined cliNamespace
values no longer cause runtime errors and fallback logic can run.
| }).catch(async error => { | ||
| logger.error({ message: `Job failed: ${formatError(error)}` }) | ||
| await store.updateJobStatus({ jobId, status: "failed" }) | ||
| await store.appendEvent({ | ||
| jobId, | ||
| type: "job.failed", | ||
| payload: { error: formatError(error) } | ||
| }) | ||
| const fallback: JobRunResult = { jobId, status: "failed", exitCode: 1 } | ||
| return fallback | ||
| }).finally(() => { | ||
| runningJobs.delete(jobId) | ||
| }) |
There was a problem hiding this comment.
Potential duplicate failure events if runJob throws after recording failure.
If runJob itself records a job.failed event (e.g., at line 60-64 in runner.ts for spawn errors) and then throws, this catch handler will record another job.failed event. Consider checking the job status before appending the failure event, or ensure runJob doesn't throw after recording failures.
🤖 Prompt for AI Agents
In @src/control-plane/extensions/supervisor/service.ts around lines 109 - 121,
The catch block for runJob can append a duplicate "job.failed" event if runJob
already recorded failure then threw; modify the catch handler (the Promise.catch
that logs error, calls store.updateJobStatus and store.appendEvent) to first
read the current job status (e.g., via store.getJob or store.getJobStatus) and
only call store.appendEvent({ type: "job.failed", ... }) if the status is not
already "failed" (leave updateJobStatus as-is or make it idempotent).
Alternatively, have runJob return a sentinel or set a flag when it has already
recorded failure and check that flag here before appending; ensure
runningJobs.delete(jobId) remains in the finally block.
| if (opts.env) { | ||
| for (const [key, value] of Object.entries(opts.env)) { | ||
| env[key] = value | ||
| } | ||
| } | ||
| env["TERM"] = process.env.TERM ?? DEFAULT_TERM | ||
| return env |
There was a problem hiding this comment.
User-provided TERM in env overrides is silently overwritten.
Line 265 always sets TERM after merging user-provided environment variables, which means any custom TERM value passed in opts.env will be ignored.
🔎 Proposed fix to preserve user override
if (opts.env) {
for (const [key, value] of Object.entries(opts.env)) {
env[key] = value
}
}
- env["TERM"] = process.env.TERM ?? DEFAULT_TERM
+ if (!env["TERM"]) {
+ env["TERM"] = process.env.TERM ?? DEFAULT_TERM
+ }
return env
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (opts.env) { | |
| for (const [key, value] of Object.entries(opts.env)) { | |
| env[key] = value | |
| } | |
| } | |
| env["TERM"] = process.env.TERM ?? DEFAULT_TERM | |
| return env | |
| if (opts.env) { | |
| for (const [key, value] of Object.entries(opts.env)) { | |
| env[key] = value | |
| } | |
| } | |
| if (!env["TERM"]) { | |
| env["TERM"] = process.env.TERM ?? DEFAULT_TERM | |
| } | |
| return env |
🤖 Prompt for AI Agents
In @src/control-plane/extensions/supervisor/shell-service.ts around lines 260 -
266, The code currently overwrites any user-provided TERM because env["TERM"] is
always set after merging opts.env; update the logic in the shell-service merging
block (the env/opts.env handling) to only set TERM when not already provided by
the merged env — e.g., after merging opts.env into env, check if env.TERM is
undefined/null and then assign process.env.TERM ?? DEFAULT_TERM — referencing
the env variable, opts.env, and DEFAULT_TERM in your change.
| function extractExtraHostsIp(extraHosts: unknown): string | null { | ||
| if (isRecord(extraHosts)) { | ||
| for (const value of Object.values(extraHosts)) { | ||
| if (typeof value === "string" && value.length > 0) return value | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| if (Array.isArray(extraHosts)) { | ||
| for (const entry of extraHosts) { | ||
| if (typeof entry !== "string") continue | ||
| const ip = parseExtraHostEntry(entry) | ||
| if (ip) return ip | ||
| } | ||
| } | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
Record format returns raw value without IPv4 validation.
When extraHosts is a record (object format), the function returns the value directly without validating it's a proper IPv4 address. This is inconsistent with the array path which validates via parseExtraHostEntry → isIpv4.
🔎 Proposed fix to validate IPv4 for record format
function extractExtraHostsIp(extraHosts: unknown): string | null {
if (isRecord(extraHosts)) {
for (const value of Object.values(extraHosts)) {
- if (typeof value === "string" && value.length > 0) return value
+ if (typeof value === "string" && isIpv4(value)) return value
}
return null
}🤖 Prompt for AI Agents
In @src/lib/caddy-hosts.ts around lines 82 - 99, extractExtraHostsIp currently
returns raw values from the record branch without IPv4 validation; update the
record handling in extractExtraHostsIp to validate each value (use existing
helpers parseExtraHostEntry or isIpv4) before returning, e.g., iterate
Object.values(extraHosts), coerce non-strings, run parseExtraHostEntry(entry) or
isIpv4(value) and return the first valid IPv4 string, otherwise continue and
finally return null; keep the Array.isArray branch behavior unchanged and reuse
parseExtraHostEntry/isIpv4 to ensure consistent validation.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @docs/gateway-api.md:
- Around line 149-151: The Markdown code block containing the WebSocket URL
"wss://gateway.example.com/control-plane/projects/<id>/shells/<id>/stream?token=<token>"
is missing a language identifier; update the opening fence from ``` to ```bash
(and keep the closing fence as ```) so the block becomes a bash-specified code
block for proper syntax highlighting and tooling recognition.
- Around line 319-325: The markdown has a table starting with "| Field | Type |
Description |" placed immediately after a JSON code block under the "Response:"
section for the GET /control-plane/projects/:projectId/jobs endpoint; insert a
single blank line between the preceding code block (or the "Response:" line) and
that table so the table is separated by one empty line to satisfy markdown style
guidelines and render correctly.
- Around line 105-107: Update the Markdown code fence that currently wraps the
URL "https://gateway.dimitri.computer" to include a language identifier (e.g.,
change the opening triple backticks to "```bash" or "```text") so the block
renders with the specified language; target the code block containing that exact
URL and replace the opening fence accordingly while keeping the closing "```"
unchanged.
- Around line 385-391: Add a blank line between the preceding JSON/code block
and the table that begins with the "Response:" heading (the table row starting
with "| Field | Type | Description |") so the markdown has an empty line before
the table for proper rendering; update the docs/gateway-api.md section showing
the "Response:" table under the GET
/control-plane/projects/:projectId/shells/:shellId endpoint to insert one blank
line immediately before the table.
In @docs/guides/remote-cloudflare.md:
- Around line 56-62: The SSH config example code block in the "Optional
~/.ssh/config shortcut" section is missing a language identifier; update the
opening fence from ``` to ```bash (or ```text) so the snippet starting with
"Host ssh.example.com" and containing "ProxyCommand
/opt/homebrew/bin/cloudflared access ssh --hostname %h" uses proper syntax
highlighting.
🧹 Nitpick comments (9)
src/ui/clipboard.ts (1)
18-50: Well-implemented with good error handling. Consider adding a timeout.The function properly handles the clipboard helper lifecycle and provides clear error messages. The logic flow is correct and defensive.
Consider adding a timeout to
proc.exitedto prevent indefinite waiting if a clipboard helper hangs:🔎 Example timeout implementation
- const exitCode = await proc.exited + const timeout = new Promise<number>((_, reject) => + setTimeout(() => reject(new Error('Clipboard operation timed out')), 5000) + ) + + let exitCode: number + try { + exitCode = await Promise.race([proc.exited, timeout]) + } catch (err) { + proc.kill() + return { ok: false, error: 'Clipboard operation timed out after 5s' } + } if (exitCode !== 0) {README.md (2)
147-156: Update command examples for clarity and formatting.Lines 147–156 collapse the previous command examples into a brief summary. While concise, the transition from prose ("Run
hack help...") directly into a command list could benefit from better visual separation. Consider converting to a fenced code block withbashidentifier for consistency with the rest of the README:🔎 Proposed formatting
-Run `hack help` (or `hack help <command>`) for full usage. - -Common: -- `hack global install|up|down` -- `hack init|up|down|logs|open|tui` -- `hack status` -- `hack remote setup` -- `hack gateway enable` - -Full command table + flags: `docs/cli.md`. +Run `hack help` (or `hack help <command>`) for full usage. + +```bash +# Common commands +hack global install|up|down +hack init|up|down|logs|open|tui +hack status +hack remote setup +hack gateway enable +``` + +Full command table + flags: `docs/cli.md`.
493-495: Clarify OAuth alias configuration for.hack.gydomain.Line 493–495 mention that
hack global installconfigures*.hack.gyvia dnsmasq. This is a significant setup step; ensure the documentation or help text in the CLI makes this clear, and consider linking to troubleshooting (e.g., if dnsmasq fails or the user is on a non-macOS system).docs/guides/init-project.md (4)
13-14: Consider briefly documenting what.hack/contains.Readers might benefit from knowing whether
.hack/includes Docker Compose files, config files, or both, to understand the generated structure and help with troubleshooting.
15-15: Clarify whathack openresolves to and how users should use the output.It would be helpful to mention what kind of URL is returned (e.g., a proxy URL pointing to services) and how users interact with it, especially given the networking notes that follow.
20-20: Add concrete examples for log retention configuration.Providing one or two YAML/JSON snippet examples (e.g.,
"7d","30d") or a brief explanation of the format would make this setting more accessible and reduce trial-and-error.
22-25: Enhance networking guidance with context on why this matters.The note correctly addresses localhost isolation, but adding a brief explanation—such as "because containers cannot reach the host's localhost"—would help users understand the why behind the translation. Also consider mentioning that this applies only to services running inside containers, not the user's local machine.
src/tui/hack-tui.ts (2)
1255-1282: Consider adding error handling for Caddy status resolution.Both
resolveGlobalCaddyIpandreadInternalExtraHostsIpare awaited without try-catch. If either throws (e.g., due to file system errors or missing configuration), the error will propagate and potentially disrupt the refresh cycle.🔎 Proposed fix
const refreshCaddyStatus = async () => { if (!isActive) return const now = Date.now() if (now - lastCaddyCheckAt < caddyCheckIntervalMs) return lastCaddyCheckAt = now + try { const nextCaddyIp = await resolveGlobalCaddyIp() const nextMappedIp = await readInternalExtraHostsIp({ projectDir: project.projectDir }) // ... rest of logic + } catch { + // Caddy status unavailable - silently ignore + return + }
1491-1494: Consider adding a timeout to the Docker stats command.The
docker stats --no-streamcommand typically completes quickly, but if Docker is unresponsive or there are many containers, this could block indefinitely. Theexeccall has no timeout configured.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
README.mddocs/gateway-api.mddocs/gateway.mddocs/guides/init-project.mddocs/guides/remote-cloudflare.mddocs/guides/remote-ssh.mddocs/guides/remote-tailscale.mdsrc/tui/hack-tui.tssrc/ui/clipboard.ts
✅ Files skipped from review due to trivial changes (2)
- docs/gateway.md
- docs/guides/remote-ssh.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/guides/remote-tailscale.md
🧰 Additional context used
🪛 Biome (2.1.2)
src/tui/hack-tui.ts
[error] 3192-3192: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 3192-3192: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🪛 LanguageTool
docs/gateway-api.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # Gateway API (hackd HTTP/WS) The gateway exposes a small, ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~111-~111: In American English, abbreviations like “etc.” require a period.
Context: ... SSH from iOS clients (Terminus, Blink, etc): 1) Join the tailnet on your laptop: ...
(ETC_PERIOD)
[grammar] ~123-~123: Use a hyphen to join words.
Context: ...you want a stable ssh.dimitri.computer style hostname: - Point the DNS record ...
(QB_NEW_EN_HYPHEN)
[grammar] ~296-~296: Use a hyphen to join words.
Context: ...items | PsItem[] | docker compose ps style rows | ### POST /control-plane/pr...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/gateway-api.md
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
149-149: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
321-321: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
387-387: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
docs/guides/remote-cloudflare.md
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (18)
src/ui/clipboard.ts (2)
1-16: LGTM! Clean type definitions and comprehensive platform support.The type definitions are well-structured with proper readonly modifiers, and the
CLIPBOARD_COMMANDSarray provides good cross-platform coverage with correct arguments for each clipboard utility.
52-61: LGTM! Solid command resolution logic.The platform filtering and availability checking using
Bun.whichis correct. The PATH validation is good defensive programming that handles edge cases appropriately.README.md (2)
6-6: Style change from<p>to<pre>for ASCII art rendering—verify display.Changed the project header from a
<p>tag to a<pre>tag with inline styles. This may affect rendering on GitHub and other markdown renderers that have different CSS contexts. Verify that the ASCII art displays correctly in all intended platforms (GitHub README, docs sites, etc.).
197-248: Control plane + extensions section is comprehensive—verify all referenced docs exist and are linked.The new section documents the gateway, remote access, supervisor, and token management well. Ensure that:
docs/extensions.md(referenced at line 207) exists and is completedocs/gateway-api.md(referenced at line 247) is fully documenteddocs/supervisor.md(referenced at line 288) is included in the PR- All examples (e.g.,
hack remote setup,hack x gateway token-create) match the actual CLI interfacedocs/gateway-api.md (1)
537-571: Verify demo script and E2E test references exist and are executable.The documentation references:
examples/basic/gateway-demo.ts(line 539)examples/next-app/app/gateway/page.tsx(line 545)- Test command
bun run test:e2e:gateway(line 570)Ensure these files exist in the PR, are properly documented, and that the test environment variables and invocation method are correct.
docs/guides/init-project.md (1)
5-10: Command sequence is clear and practical.The three-step initialization flow is easy to follow and well-ordered. Good progression from setup to daemon startup to access.
src/tui/hack-tui.ts (12)
92-101: LGTM on the scroll change callback pattern.The
PausableScrollBoxRenderableclass cleanly extendsScrollBoxRenderableto add scroll event interception for pause/resume functionality.
3462-3496: LGTM on the stream consumption implementation.Good use of
try-finallyto ensure the reader lock is released. The buffered line parsing and early termination viaisActive()are correctly implemented.
1840-1896: LGTM on history loading with capacity management.Good implementation that respects
maxEntrieslimit, provides user feedback via toast when capped, and correctly handles both Loki and Compose backends with different pagination strategies.
1993-2136: LGTM on the dual-mode search implementation.Clean separation between local in-memory search (for non-Loki backends) and remote Loki search. Both paths correctly update UI state and handle edge cases.
2816-2849: LGTM on the log continuation detection.The heuristics for collapsing multi-line log entries (stack traces, JSON structures, indented content) are well-considered and will improve log readability.
2723-2746: LGTM on byte parsing with comprehensive unit support.Good coverage of both SI (KB, MB, GB) and binary (KiB, MiB, GiB) units with case-insensitive matching.
2293-2304: LGTM on selection change handling.Good UX decision to pause log following when text is selected, preventing the jarring experience of content moving while trying to copy.
1350-1393: LGTM on responsive sidebar layout.Good progressive degradation strategy: first hide resources panel, then switch to compact meta, then shrink services. The early return for zero height prevents issues during initialization.
2517-2532: LGTM on project ID resolution with fallback.Robust implementation with try-catch for registry access failures, priority given to directory match, and case-insensitive name fallback.
162-169: LGTM on renderer initialization with mouse support.Good configuration choices: alternate screen for clean exit, mouse support for scroll interactions, and reasonable FPS target.
3188-3194: Control character regex is intentional - static analysis false positive.The regex
[\x00-\x1f\x7f]at line 3189 intentionally matches ASCII control characters (0x00-0x1F and DEL 0x7F) to normalize text that may contain terminal escape sequences or other control characters. This is correct behavior for log text sanitization.
2217-2221: The code is already safe. At line 2217, there is a conditional checkif (handleSelectionChange)that ensures the listener is only removed whenhandleSelectionChangeis not null. SincehandleSelectionChangeis initialized tonullat line 109 and assigned its actual value at line 2293, the listener can only be removed if it was previously added, which occurs at line 2304 after the assignment.Likely an incorrect or invalid review comment.
| ``` | ||
| https://gateway.dimitri.computer | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to URL display block.
The code block at line 105 displays a gateway URL without a language specifier. Update to \``bashor```text`:
🔎 Proposed fix
Cloudflare creates a CNAME to `<tunnel-id>.cfargotunnel.com` under your zone. The gateway URL is:
-```
+```bash
https://gateway.dimitri.computer
-```
+```Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @docs/gateway-api.md around lines 105 - 107, Update the Markdown code fence
that currently wraps the URL "https://gateway.dimitri.computer" to include a
language identifier (e.g., change the opening triple backticks to "```bash" or
"```text") so the block renders with the specified language; target the code
block containing that exact URL and replace the opening fence accordingly while
keeping the closing "```" unchanged.
| ``` | ||
| wss://gateway.example.com/control-plane/projects/<id>/shells/<id>/stream?token=<token> | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to WebSocket URL block.
The code block at line 149 displays a WebSocket URL without a language specifier. Update to \``bash` or similar:
🔎 Proposed fix
Browser note: the native WebSocket API cannot set headers. For browser clients, pass the token
as a query param on the WS URL:
-```
+```bash
wss://gateway.example.com/control-plane/projects/<id>/shells/<id>/stream?token=<token>
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| wss://gateway.example.com/control-plane/projects/<id>/shells/<id>/stream?token=<token> | |
| ``` | |
| Browser note: the native WebSocket API cannot set headers. For browser clients, pass the token | |
| as a query param on the WS URL: | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
149-149: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @docs/gateway-api.md around lines 149 - 151, The Markdown code block
containing the WebSocket URL
"wss://gateway.example.com/control-plane/projects/<id>/shells/<id>/stream?token=<token>"
is missing a language identifier; update the opening fence from ``` to ```bash
(and keep the closing fence as ```) so the block becomes a bash-specified code
block for proper syntax highlighting and tooling recognition.
|
|
||
| Response: | ||
| | Field | Type | Description | | ||
| | --- | --- | --- | | ||
| | `job` | JobMeta | Job metadata | | ||
|
|
||
| ### GET /control-plane/projects/:projectId/jobs |
There was a problem hiding this comment.
Add blank line before table to meet markdown style guidelines.
Line 321 starts a table immediately after a JSON code block without a blank line. Add a blank line for proper spacing:
🔎 Proposed fix
}Response:
| Field | Type | Description |
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
321-321: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/gateway-api.md around lines 319 - 325, The markdown has a table
starting with "| Field | Type | Description |" placed immediately after a JSON
code block under the "Response:" section for the GET
/control-plane/projects/:projectId/jobs endpoint; insert a single blank line
between the preceding code block (or the "Response:" line) and that table so the
table is separated by one empty line to satisfy markdown style guidelines and
render correctly.
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
|
||
| Response: | ||
| | Field | Type | Description | | ||
| | --- | --- | --- | | ||
| | `shell` | ShellMeta | Shell metadata | | ||
|
|
||
| ### GET /control-plane/projects/:projectId/shells/:shellId |
There was a problem hiding this comment.
Add blank line before table to meet markdown style guidelines.
Line 387 starts a table immediately after a JSON code block without a blank line. Add a blank line for proper spacing:
🔎 Proposed fix
}Response:
| Field | Type | Description |
</details>
> Committable suggestion skipped: line range outside the PR's diff.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
387-387: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/gateway-api.md around lines 385 - 391, Add a blank line between the
preceding JSON/code block and the table that begins with the "Response:" heading
(the table row starting with "| Field | Type | Description |") so the markdown
has an empty line before the table for proper rendering; update the
docs/gateway-api.md section showing the "Response:" table under the GET
/control-plane/projects/:projectId/shells/:shellId endpoint to insert one blank
line immediately before the table.
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
<!-- This is an auto-generated comment by CodeRabbit -->
| Optional `~/.ssh/config` shortcut: | ||
|
|
||
| ``` | ||
| Host ssh.example.com | ||
| User <user> | ||
| ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to SSH config code block.
The SSH config example at lines 56–62 is missing a language identifier. Update to \``bashor```text` for proper syntax highlighting:
🔎 Proposed fix
-Optional `~/.ssh/config` shortcut:
-
-```
+Optional `~/.ssh/config` shortcut:
+
+```bash
Host ssh.example.com
User <user>
ProxyCommand /opt/homebrew/bin/cloudflared access ssh --hostname %h
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @docs/guides/remote-cloudflare.md around lines 56 - 62, The SSH config
example code block in the "Optional ~/.ssh/config shortcut" section is missing a
language identifier; update the opening fence from ``` to ```bash (or ```text)
so the snippet starting with "Host ssh.example.com" and containing "ProxyCommand
/opt/homebrew/bin/cloudflared access ssh --hostname %h" uses proper syntax
highlighting.
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In @CLAUDE.md:
- Line 44: Fix the two typos in the sentence: change "sensure" to "ensure" and
"tpye" to "type", resulting in: "Never use any types and always default to
leveraging generics and smart types to ensure the best possible type inference
across the project."
In @docs/cli.md:
- Line 762: The Markdown tables for the "hack mcp install" and "print options"
sections have mismatched column counts; update both tables so the header row and
every data row use the same number of columns (4), ensuring the flag row
containing `--scope <user|project>` aligns with the header columns (e.g., ``Flag
| Type | Default | Description``) and that each cell in that row maps to those
four headers; fix any extra or missing pipe characters so both tables render
with consistent 4-column structures.
- Line 61: The table row for the `logs` subcommand has an inconsistent number of
cells causing malformed Markdown; update the table to have the same number of
columns as the header (likely Flag | Type | Default | Description) and split the
current row’s five cells into four by mapping the command name (`logs`) into the
Flag column, the target services (caddy|grafana|loki|alloy) into the Description
column (or Type if you intend a separate Type), provide a Default value if
applicable (or `-`), and ensure the header and all rows use the same
pipe-separated column count so the table renders correctly.
- Around line 852-853: The two Markdown table rows for the hack log-pipe options
are malformed (extra pipe separators) causing column-count mismatches; locate
the rows containing `--format <auto|docker-compose|plain>` and `--stream
<stdout|stderr>` and edit each so they have exactly four pipe-delimited cells
matching the table header (Option | Type | Default | Description), removing any
stray pipes or merged cells, keep backticks around option and default values,
and verify the table alignment renders correctly.
- Line 740: The table row for the `-c`, `--client <cursor|claude|codex|print>`
option is producing extra pipe separators and breaking the 4-column table
structure; fix it by ensuring the entire option cell is a single cell (escape or
wrap the inner pipes such as `<cursor|claude|codex|print>` so they don't split
columns) and align it with the existing header columns, keeping the row to
exactly four pipe-separated cells; update the offending row where the `-c`,
`--client <cursor|claude|codex|print>` entry appears to use proper escaping or
quoting so the table parser treats the value as one cell.
- Line 905: The table row for the "hack the planet" options is malformed
(extra/incorrect pipe delimiters) causing a 7-column row instead of 4; find the
row containing the text `--variant <cut|mash|cycle|random>` and correct its pipe
separators so it has exactly four cells (e.g., `| \`--variant
<cut|mash|cycle|random>\` | string | \`cycle\` | Animation variant |`), also
verify the table header/divider uses the same 4-column pipe structure and escape
or remove any stray pipes inside cells.
In @docs/extensions.md:
- Line 142: The Gateway extension token-create options table has a row for
`--scope <read|write>` with four pipe-separated columns but the table header is
malformed with a different column count; fix by making the header match four
columns (e.g., "Option | Type | Default | Description") and ensure the separator
row contains four header separators (e.g., "| --- | --- | --- | --- |"), then
verify the `--scope` row and other data rows align to those four columns so the
table renders correctly.
In @src/commands/internal.ts:
- Around line 10-17: The option definition for optPath uses defaultValue: null
which conflicts with the expected type string | undefined; update the
defineOption call for optPath to set defaultValue: undefined (or remove the
defaultValue property) so the default matches the option type; ensure the change
is made in the optPath constant where defineOption is invoked.
- Around line 76-79: In resolveStartDir, guard against non-string option values
before calling trim by reading args.options.path into a variable (e.g., pathOpt)
and performing a type-narrow: use typeof pathOpt === 'string' ? pathOpt.trim() :
'' for fromOpt, then keep the existing logic (fromOpt.length > 0 ? fromOpt :
ctx.cwd); this avoids calling trim on number/boolean values while preserving
behavior of resolveStartDir and references to args.options.path.
- Around line 23-51: The three command specs extraHostsSetSpec,
extraHostsUnsetSpec, and extraHostsListSpec currently use an invalid group value
"Internal"; change their group property to one of the allowed CliGroup strings
(for example "Global" or "Project") so the type matches the CliGroup union,
e.g., replace group: "Internal" with group: "Global" (or another valid option)
in each of those defineCommand calls.
- Around line 126-152: The handler signatures use string literal arrays for
positionals but must reference the PositionalSpec types; update the args generic
for both handleExtraHostsSet and handleExtraHostsUnset to use the actual
positionals type (e.g., typeof extraHostsSetSpec['positionals'] or a shared
const like extraHostsSetPositionals) instead of readonly ["hostname","target"],
ensuring the same positional type is used in the spec and the CommandArgs
generic.
In @src/commands/project.ts:
- Line 7: The package version for @clack/prompts referenced by the import (e.g.,
the symbol select imported from '@clack/prompts') is invalid (1.0.0-alpha.9) and
will fail to install; update package.json to point to a published version (for
example 1.0.0-alpha.6 or the stable 0.11.0) or convert it to the correct
git/private-registry reference, then reinstall so the import of select resolves
correctly.
In @src/commands/setup.ts:
- Around line 131-138: The summary on the ticketsSpec command is incorrect (it
references Codex); update the summary field in the ticketsSpec definition to
describe the tickets feature (e.g., "Install Tickets skill for hack ticket
management" or similar) so it accurately reflects the tickets functionality
instead of copying codexSpec text.
In @src/control-plane/extensions/tickets/store.ts:
- Around line 194-202: computeNextTicketId currently scans materializeTickets,
uses parseTicketNumber and formatTicketId, then code later appends a ticket
event — this allows races and duplicate IDs when concurrent processes create
tickets; fix by making ID generation atomic: either (A) add a dedicated counter
store and replace computeNextTicketId to perform an atomic increment (e.g.,
increment-and-read a counter file/DB entry) so each process gets a unique
sequential ID, or (B) acquire a process-wide lock around computeNextTicketId +
the append step (use a lock file or mutex) to serialize creation, or (C) append
a collision-safe suffix (e.g., a short UUID or timestamp) to the value returned
by formatTicketId to guarantee uniqueness; update all callers of
computeNextTicketId and the ticket-id format logic so parsing/formatting remains
consistent with the chosen approach.
In @src/control-plane/extensions/tickets/util.ts:
- Around line 17-24: The parseTicketNumber function currently allows negative
results (e.g., "T--5" -> -5); update parseTicketNumber to reject non-positive
values by checking the parsed numeric value after Number.isFinite — if n <= 0
return null — and only then return Math.trunc(n); keep the checks for the "T-"
prefix and numeric parsing as-is.
🧹 Nitpick comments (13)
src/commands/internal.ts (2)
99-104: Consider logging a warning when JSON parsing fails.Silently returning an empty object when JSON parsing fails could mask configuration issues. A warning log would help users debug malformed
extra-hosts.jsonfiles.💡 Suggested improvement
try { parsed = JSON.parse(text) } catch { + logger.warn({ message: `Failed to parse ${path}, treating as empty` }) return {} }
154-181: RedundantensureDircall after reading the file.The directory and file must exist if
readInternalExtraHostssuccessfully read content and found the hostname. TheensureDircall on line 175 is unnecessary and adds a small overhead.♻️ Proposed simplification
delete existing[hostname] - await ensureDir(resolve(project.projectDir, ".internal")) await writeInternalExtraHosts(path, existing).gitignore (1)
45-45: Remove duplicatedistentry.The
distdirectory is already ignored on line 6. This duplicate entry on line 45 is redundant.♻️ Proposed fix
-distdocs/extensions.md (5)
31-47: Add language identifier to TypeScript code block.Code blocks should specify their language for proper syntax highlighting. Add
tsidentifier to this code block.🔧 Proposed fix
-```ts +```ts export type ExtensionDefinition = {This appears to already have
ts, so verify the markdown is rendering correctly.
93-93: Add language identifier to code block (line 93).This code block should specify a language for syntax highlighting (likely
bashorshell).
382-382: Fix hyphenation in SSH tunnel example.Line 382 uses "ad‑hoc" with a non-standard hyphen character. Use a standard hyphen:
ad-hoc.
392-392: Remove redundant phrase in VPN recommendation.Line 392 uses the redundant phrase "Zero Trust/VPN network". Simplify to just "VPN" or "private network".
403-403: Add language identifiers to JSON code blocks.Code blocks at lines 403, 417, and 427 are missing language specifications. Add
jsonto enable proper syntax highlighting.Also applies to: 417-417, 427-427
src/control-plane/extensions/tickets/commands.ts (2)
327-388: Consider extracting status values to a constant.The status validation at lines 350-353 uses hard-coded string literals. Consider extracting valid status values to a constant or using a type/enum for better maintainability.
♻️ Suggested refactor
At the top of the file, add:
+const VALID_TICKET_STATUSES = ["open", "in_progress", "blocked", "done"] as const +type TicketStatus = typeof VALID_TICKET_STATUSES[number]Then update the validation:
- if (status !== "open" && status !== "in_progress" && status !== "blocked" && status !== "done") { + if (!VALID_TICKET_STATUSES.includes(status as TicketStatus)) { ctx.logger.error({ message: `Invalid status: ${status}` }) return 1 }And update the error message at line 346 to use the constant:
- ctx.logger.error({ message: "Usage: hack x tickets status <ticket-id> <open|in_progress|blocked|done>" }) + ctx.logger.error({ message: `Usage: hack x tickets status <ticket-id> <${VALID_TICKET_STATUSES.join("|")}>` })
575-595: Add error handling for file and stdin reads.The
resolveTicketBodyfunction reads from stdin and files but doesn't handle potential errors. While.catch()is used on line 588, stdin reading at line 581 has no error handling.🛡️ Add error handling
async function resolveTicketBody(opts: { readonly body?: string readonly bodyFile?: string readonly bodyStdin: boolean }): Promise<string | undefined> { if (opts.bodyStdin) { - const text = await Bun.stdin.text() + const text = await Bun.stdin.text().catch(() => "") const trimmed = text.trimEnd() return trimmed.length > 0 ? trimmed : undefined } const bodyFile = (opts.bodyFile ?? "").trim() if (bodyFile.length > 0) { - const text = await Bun.file(bodyFile).text().catch(() => "") + try { + const text = await Bun.file(bodyFile).text() + const trimmed = text.trimEnd() + return trimmed.length > 0 ? trimmed : undefined + } catch { + // Return undefined if file cannot be read + return undefined + } } const body = (opts.body ?? "").trimEnd() return body.length > 0 ? body : undefined }src/control-plane/extensions/tickets/repo-state.ts (1)
119-125: Consider: Negation patterns may need handling.The filter excludes lines starting with
!(negation patterns), but a negation like!.hack/tickets/important.mdwould re-include files. The current check might miss cases where.hack/tickets/is present but partially negated.This is likely acceptable for the common case, but worth noting if complex gitignore patterns are expected.
src/control-plane/extensions/tickets/store.ts (2)
107-136: Consider: Silent error handling may mask issues.Line 121 catches all errors when reading event files and returns empty string. If a file exists but is unreadable (permissions), events will be silently skipped. Consider logging a warning via
opts.logger.warnwhen file read fails for non-empty files.Suggested improvement
for (const filename of entries.sort()) { const path = resolve(eventsDir, filename) - const text = await Bun.file(path).text().catch(() => "") + let text = "" + try { + text = await Bun.file(path).text() + } catch (err) { + opts.logger.warn({ message: `Failed to read events file: ${path}` }) + continue + } for (const line of text.split("\n")) {
138-192: Performance note:materializeTicketsrereads all events on each call.Multiple operations (
createTicket,listTickets,getTicket,setStatus) each callmaterializeTickets(), re-reading and re-parsing all event files. For stores with many events, this could be slow.Consider adding an in-memory cache that invalidates when
appendEventsis called, or document that this store is designed for small-scale use.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (45)
.beads/.gitignore.beads/README.md.beads/config.yaml.beads/interactions.jsonl.beads/issues.jsonl.beads/metadata.json.codex/skills/hack-tickets/SKILL.md.gitignore.hack/README.md.hack/docker-compose.yml.hack/hack.config.jsonAGENTS.mdCLAUDE.mdREADME.mddocs/architecture.mddocs/cli.mddocs/extensions.mddocs/guides/tickets.mdexamples/tickets/.hack/README.mdexamples/tickets/.hack/docker-compose.ymlexamples/tickets/.hack/hack.config.jsonexamples/tickets/README.mdexamples/tickets/app.txtsrc/cli/spec.tssrc/commands/internal.tssrc/commands/project.tssrc/commands/setup.tssrc/control-plane/extensions/tickets/agent-docs.tssrc/control-plane/extensions/tickets/commands.tssrc/control-plane/extensions/tickets/extension.tssrc/control-plane/extensions/tickets/repo-state.tssrc/control-plane/extensions/tickets/store.tssrc/control-plane/extensions/tickets/tickets-git-channel.tssrc/control-plane/extensions/tickets/tickets-skill.tssrc/control-plane/extensions/tickets/util.tssrc/control-plane/sdk/config.tssrc/lib/fs.tssrc/lib/project.tssrc/lib/projects-registry.tssrc/templates.tstests/log-pipe.test.tstests/mcp.test.tstests/project-config.test.tstests/supervisor-service.test.tstests/tickets-extension.test.ts
💤 Files with no reviewable changes (5)
- .beads/issues.jsonl
- .beads/.gitignore
- .beads/README.md
- .beads/metadata.json
- .beads/config.yaml
✅ Files skipped from review due to trivial changes (8)
- examples/tickets/app.txt
- examples/tickets/README.md
- examples/tickets/.hack/README.md
- examples/tickets/.hack/hack.config.json
- .codex/skills/hack-tickets/SKILL.md
- .hack/hack.config.json
- .hack/README.md
- .hack/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli/spec.ts
- src/control-plane/extensions/tickets/extension.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/control-plane/extensions/tickets/tickets-git-channel.ts (2)
src/control-plane/sdk/config.ts (1)
TicketsGitConfig(119-119)src/lib/guards.ts (1)
isRecord(1-3)
src/control-plane/sdk/config.ts (4)
src/lib/config-paths.ts (1)
resolveGlobalConfigPath(9-13)src/constants.ts (2)
PROJECT_CONFIG_FILENAME(61-61)GLOBAL_ONLY_EXTENSION_IDS(38-41)src/lib/fs.ts (1)
readTextFile(21-27)src/lib/guards.ts (1)
isRecord(1-3)
src/commands/setup.ts (2)
src/cli/options.ts (1)
optPath(3-10)src/cli/command.ts (4)
CommandArgs(61-71)defineCommand(108-110)withHandler(125-130)CliContext(33-36)
src/commands/project.ts (4)
src/lib/fs.ts (2)
readTextFile(21-27)pathExists(3-15)src/lib/guards.ts (1)
isRecord(1-3)src/constants.ts (4)
GLOBAL_HACK_DIR_NAME(25-25)GLOBAL_CADDY_DIR_NAME(32-32)GLOBAL_CADDY_COMPOSE_FILENAME(45-45)DEFAULT_INGRESS_NETWORK(4-4)src/lib/shell.ts (1)
exec(13-31)
🪛 GitHub Actions: CI
src/commands/internal.ts
[error] 16-16: Type 'null' is not assignable to type 'string | undefined'.
[error] 26-26: Type '"Internal"' is not assignable to type 'CliGroup'.
[error] 38-38: Type '"Internal"' is not assignable to type 'CliGroup'.
[error] 47-47: Type '"Internal"' is not assignable to type 'CliGroup'.
[error] 56-56: Type '"Internal"' is not assignable to type 'CliGroup'.
[error] 64-64: Type '"Internal"' is not assignable to type 'CliGroup'.
[error] 77-77: Property 'trim' does not exist on type 'string | number | boolean'.
[error] 131-131: Type 'readonly ["hostname", "target"]' does not satisfy the constraint 'readonly PositionalSpec[]'.
[error] 136-136: Property 'hostname' does not exist on type '{}'.
[error] 137-137: Property 'target' does not exist on type '{}'.
[error] 159-159: Type 'readonly ["hostname"]' does not satisfy the constraint 'readonly PositionalSpec[]'.
[error] 164-164: Property 'hostname' does not exist on type '{}'.
🪛 LanguageTool
CLAUDE.md
[grammar] ~44-~44: Ensure spelling is correct
Context: ... leveraging generics and smart types to sensure the best possible tpye inference across...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~44-~44: Ensure spelling is correct
Context: ...mart types to sensure the best possible tpye inference across the project. <!-- hac...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/cli.md
[style] ~271-~271: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e combined with --pretty. - --until cannot be combined with --follow. ### hack ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/extensions.md
[grammar] ~382-~382: Ensure spelling is correct
Context: ...xpose it through one of: - SSH tunnel (ad‑hoc): ssh -L 7788:127.0.0.1:7788 <host> -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~392-~392: This phrase is redundant (‘N’ stands for ‘network’). Use simply “VPN”.
Context: ...access. 2) Move to a private Zero Trust/VPN network once you want persistent access. 3) Add...
(ACRONYM_TAUTOLOGY)
🪛 markdownlint-cli2 (0.18.1)
docs/cli.md
61-61: Table column count
Expected: 2; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
740-740: Table column count
Expected: 4; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
762-762: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
780-780: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
852-852: Table column count
Expected: 4; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
853-853: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
905-905: Table column count
Expected: 4; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
docs/extensions.md
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
142-142: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
403-403: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
417-417: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
427-427: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (73)
src/lib/project.ts (3)
163-178: LGTM!The
extraHostsfield is well-documented with a clear use case example for host-local tunnels and TLS SNI preservation. TheRecord<string, string>type appropriately models Docker Compose extra_hosts entries.
285-301: LGTM!The integration of
extraHostsintoparseInternalConfigfollows the established pattern used fordnsandtls. The conditional spread and empty-object check maintain consistency with other config parsers in this file.
303-317: LGTM!The
parseStringMapfunction properly validates both keys and values, trimming whitespace and filtering out empty or non-string entries. The pattern matchesreadInternalExtraHostsinsrc/commands/internal.ts, ensuring consistent parsing behavior across config sources.src/commands/internal.ts (1)
183-198: LGTM!The list handler correctly resolves the project, reads the extra-hosts map, and outputs it as formatted JSON to stdout. The implementation follows the established patterns in the file.
src/lib/projects-registry.ts (2)
138-164: LGTM! Well-designed ID-based project resolver.The implementation correctly mirrors
resolveRegisteredProjectByNamewith the intentional enhancement of returning both the registration metadata and the ProjectContext. This provides callers with complete project information in a single lookup, which is useful for operations that need both the registration details and the resolved project paths.
229-235: No changes needed.The implementation is correct and the semantics are clear:
HACK_GLOBAL_CONFIG_PATHis intended to be a file path (as documented indocs/sdk.md), anddirname()correctly extracts the directory containing that config file to serve as the registry root. All tests confirm this pattern by setting the env var to file paths likehack-global-config-*.json.Likely an incorrect or invalid review comment.
examples/tickets/.hack/docker-compose.yml (1)
1-5: LGTM!This minimal Docker Compose setup serves as a valid placeholder for the tickets example. The noop service provides a simple demonstration environment.
AGENTS.md (2)
46-59: LGTM!The tickets documentation is clear and provides helpful command examples for the new git-backed tickets system.
65-65: LGTM!The terminology change from "issues" to "tickets" is consistent with the new tickets system introduced in this PR.
src/lib/fs.ts (1)
3-15: Verify cross-runtime compatibility behavior.The new logic provides a fallback to
node:fsstatwhenBun.file().statis not available, enabling cross-runtime compatibility. However, ifBun.file().stat()exists but throws an error (e.g., permission denied), the code will skip the Node.js fallback and returnfalseimmediately.Is this the intended behavior? Or should the fallback be attempted when
Bun.file().stat()throws, not just when it doesn't exist?docs/guides/tickets.md (1)
1-128: Approve documentation for tickets extension guide.The guide is well-structured, practical, and clearly explains the tickets extension's purpose, setup, usage, and storage layout. Examples are helpful and configuration instructions are clear.
src/control-plane/extensions/tickets/util.ts (4)
1-3: LGTM!Standard Unix timestamp conversion is implemented correctly.
5-10: LGTM!Good use of UTC methods for consistent month stamp formatting across timezones.
12-15: LGTM!Ticket ID formatting is straightforward and correct.
26-43: LGTM!The deterministic JSON serialization is implemented correctly, with proper recursion for nested structures.
src/commands/project.ts (5)
548-575: LGTM!Safe JSON parsing with appropriate validation and error handling. Empty object fallback ensures the function is safe to use without additional null checks.
600-703: LGTM!The updated signature and implementation correctly integrate branch-aware host mappings and extra_hosts. The separation of managed (Caddy-derived) and user-configured extra hosts is well-designed.
734-762: LGTM!Correctly extracts Caddy hosts from compose service labels with proper error handling and deterministic output.
764-781: LGTM!Robust parsing that correctly filters out dynamic patterns, templates, and ports to extract only static hostnames suitable for extra_hosts mappings.
855-894: LGTM!The Caddy IP resolution follows the same safe pattern as CoreDNS resolution, with appropriate fallbacks and error handling.
src/control-plane/extensions/tickets/agent-docs.ts (4)
5-29: LGTM!Type definitions and marker constants are well-defined and appropriate for the use case.
31-54: LGTM!Robust batch processing with per-target error handling and appropriate status tracking.
56-116: LGTM!Check and remove functions handle all edge cases correctly with appropriate status codes.
151-183: LGTM!Snippet manipulation helpers correctly handle insertion, replacement, and removal with proper content preservation and formatting.
src/templates.ts (2)
349-353: LGTM!The
extra_hostsschema correctly models a hostname-to-IP mapping.
364-442: LGTM!The control plane schema is well-structured with appropriate types and extensibility via
additionalProperties. The optional nature aligns with the design goals.docs/architecture.md (3)
19-20: LGTM!Documentation accurately describes the CoreDNS and extra_hosts integration, including the rationale for resolver compatibility and operational guidance.
Also applies to: 84-96
181-213: LGTM!Clear documentation of the control plane architecture with helpful visualization and appropriate cross-references to detailed API documentation.
141-144: LGTM!Retention documentation clearly explains the two-level configuration system and when it applies.
src/commands/setup.ts (3)
8-8: LGTM: Tickets skill integration follows established patterns.The import, options constant, and type definition are consistent with the existing setup commands (cursor, claude, codex).
Also applies to: 84-84, 100-100
169-169: LGTM: Tickets subcommand properly wired.The tickets subcommand is correctly integrated into the setup command using the established pattern.
250-273: LGTM: Handler implementation is correct and consistent.The
handleSetupTicketsfunction follows the same pattern as the other setup handlers, with appropriate error handling and result logging.src/control-plane/extensions/tickets/commands.ts (6)
20-138: LGTM: Setup command implementation is thorough.The setup command properly handles:
- Project validation
- Argument parsing
- Repository state management (gitignore, tracking)
- Interactive and non-interactive flows
- JSON output
- Error handling
The conditional logic for TTY-based prompts is well-structured.
139-204: LGTM: Create command has proper validation and setup checks.The create command:
- Validates required title parameter
- Ensures tickets setup is complete before creating
- Resolves body from multiple sources (stdin, file, inline)
- Provides both JSON and formatted output
206-253: LGTM: List command implementation is clean.Straightforward list implementation with proper empty state handling and formatted table output.
255-325: LGTM: Show command provides comprehensive ticket details.The show command properly displays ticket metadata, body content, and event history with appropriate formatting.
390-439: LGTM: Sync command implementation is straightforward.The sync command properly delegates to the store and displays sync status clearly.
597-738: LGTM: Setup validation and argument parsing are well-implemented.The
maybeEnsureTicketsSetupfunction:
- Comprehensively checks setup requirements
- Provides interactive fix flow with TTY detection
- Falls back to informative warnings for non-interactive contexts
The
parseTicketsSetupArgsfunction properly parses all setup-specific flags with appropriate validation.README.md (5)
120-130: LGTM: Clear documentation for extra_hosts feature.The new documentation clearly explains how to manage dynamic extra_hosts mappings and where they are stored.
157-168: LGTM: Commands section updated with new functionality.The updated commands section provides a good high-level overview and properly references detailed documentation.
208-300: LGTM: Comprehensive control plane and gateway documentation.The new section provides:
- Clear explanation of the extension system
- Detailed gateway setup instructions
- Security guidance for tokens and write access
- Multiple remote access options with trade-offs
- Good warnings about appropriate use cases
397-426: LGTM: Improved service-to-service connection guidance.The updated section provides clearer guidance on:
- When to use
*.hackhostnames (HTTP services)- When to use Compose service names (non-HTTP services)
- How the internal DNS and extra_hosts work together
- Troubleshooting for ENOTFOUND errors
720-726: LGTM: Troubleshooting section updated for new architecture.The troubleshooting updates reflect the CoreDNS and extra_hosts changes with specific commands to resolve issues.
src/control-plane/extensions/tickets/tickets-skill.ts (4)
1-17: LGTM: Type definitions and constants are well-structured.The type definitions clearly specify the scope, status values, and result shape. Constants follow established patterns.
19-67: LGTM: Install and check functions are robust.The install function:
- Properly resolves paths based on scope
- Avoids unnecessary file writes with
writeTextFileIfChanged- Returns appropriate status values
The check function validates not just file presence but also content integrity via marker check.
69-92: LGTM: Remove function properly cleans up skill directory.The function removes the entire skill directory recursively and handles the case where the skill is already absent.
94-175: LGTM: Skill content rendering and path resolution are well-implemented.The
renderTicketsSkillfunction:
- Generates comprehensive skill documentation
- Uses proper markdown frontmatter
- Documents all commands and configuration
The path resolution:
- Properly validates required inputs (project root, HOME)
- Provides clear error messages
- Handles both user and project scopes correctly
src/control-plane/extensions/tickets/tickets-git-channel.ts (8)
1-27: LGTM: Interface design is clean and follows result pattern.The
TicketsGitChannelinterface clearly defines the contract with appropriate result types using the ok/error pattern.
67-88: LGTM: Bare repo initialization with excellent documentation.The critical comment at lines 77-80 explains why
git init --bareis used instead of cloning:"Important: do NOT
clone --barethe project. This channel is intended to store only.hack/tickets/**on a dedicated branch."This prevents the tickets repo from becoming enormous and keeps it focused on ticket data only.
116-187: LGTM: Checkout logic comprehensively handles all scenarios.The
checkoutHeadfunction properly handles:
- Remote-tracked branches (fetch, checkout, reset)
- Missing remote refs (falls back to local)
- Local branches (checkout, reset)
- Fresh repos (orphan branch with initial commit)
The error handling at each step is appropriate and provides clear messages.
204-254: LGTM: Event writing with proper deduplication.The event writing logic:
- Organizes events by month for better file management
- Deduplicates by
eventIdto prevent duplicates- Properly appends to existing files with newline handling
- Normalizes logs after writing to maintain consistency
256-301: LGTM: Log normalization ensures data consistency.The normalization function:
- Deduplicates events across all files
- Maintains stable sort order (timestamp, then eventId)
- Only rewrites files when content has changed
- Gracefully handles missing directories
This ensures the event log remains consistent across sync operations.
322-348: Push retry logic is well-implemented but limited to one retry.The
pushWithRetryfunction:
- Attempts push once
- On failure, fetches latest, re-checks out, rewrites pending events, and retries
- Only retries once (no infinite loop)
This is a good balance between reliability and avoiding infinite retries. The single retry handles most common cases like concurrent pushes.
350-411: LGTM: Public API properly composes internal helpers.The public methods:
ensureCheckedOut: throws on error for caller simplicityappendEvents: full workflow with pending event retrysync: normalization-focused workflowThe composition is clean and each method has a clear purpose.
413-444: LGTM: Utility functions are well-implemented.The utilities provide:
runGit: clean git command execution via Bun.spawnsafeJsonParse: safe JSON parsing with error handlingisMissingRemoteRef: robust detection of missing remote refssrc/control-plane/sdk/config.ts (7)
1-9: LGTM!Imports are well-organized and necessary for the configuration system.
10-116: LGTM! Schema definitions follow Zod 4 best practices.The Input/Output schema pattern allows flexible partial input while enforcing complete output with sensible defaults. Using
.default()with pre-parsed values for nested objects is correct for Zod 4.
118-126: LGTM!Type exports are clean and provide the necessary public API surface.
128-160: LGTM!The config loading flow is clear: read both layers, merge, and aggregate errors. The conditional inclusion of
parseErrorin the return is a nice touch.
206-228: Verify: Gateway merge may discard project-level settings other thanenabled.After
mergeRecordsmerges all config sections, line 225 overwritesmerged.gatewaywith onlyglobalGatewayplusenabled. This discards any project-level gateway settings (e.g.,port,bind,allowWrites) that were merged earlier.If this is intentional (global-only gateway settings for security), consider adding a comment. Otherwise, the merge should preserve project settings:
Suggested fix if project settings should be preserved
const globalGateway = isRecord(opts.global.gateway) ? opts.global.gateway : {} const projectGateway = isRecord(opts.project.gateway) ? opts.project.gateway : {} const projectEnabled = projectGateway.enabled const gatewayEnabled = typeof projectEnabled === "boolean" ? projectEnabled : false - merged.gateway = { ...globalGateway, enabled: gatewayEnabled } + // Gateway inherits from global, project can only toggle `enabled` + merged.gateway = { ...merged.gateway, enabled: gatewayEnabled }
230-251: LGTM!The
mergeExtensionsfunction correctly handlesGLOBAL_ONLY_EXTENSION_IDSby restoring global values or removing project-only definitions. This prevents projects from overriding sensitive global-only extensions.
253-277: LGTM!
mergeRecordsperforms recursive shallow merge correctly, skippingundefinedvalues.joinParseErrorscleanly aggregates errors with a separator.src/control-plane/extensions/tickets/repo-state.ts (5)
1-35: LGTM!Type definitions are comprehensive and cover all status variations. Constants for gitignore are appropriately scoped to this module.
37-66: LGTM!The state checking logic is solid. Using
-zflag withls-filesfor NUL-delimited output is a good practice for handling filenames with special characters.
68-85: LGTM!The function correctly handles three cases: entry already present, file doesn't exist (create), and file exists without entry (update).
87-117: LGTM!The untrack flow is correct: check if tracked, then
git rm --cachedto remove from index without deleting files. The-rflag handles recursive removal correctly.
142-161: LGTM!The
runGithelper properly handles process spawning, output capture, and error cases. Usingstdin: "ignore"prevents hanging on interactive prompts.src/control-plane/extensions/tickets/store.ts (6)
1-11: LGTM!Imports are appropriate for the event-sourced ticket store functionality.
13-51: LGTM!Type definitions are well-structured. Using discriminated unions for result types (
ok: true | false) is a good pattern for explicit error handling.
175-188: Note: Clearing ticket body is not possible.In
ticket.updatedhandling, ifpayload.bodyis not a string (e.g.,nullorundefined),bodybecomesundefinedand the spread on line 185 won't include it, leaving the existing body unchanged.If intentional (body can only be set, never cleared), this is fine. If clearing should be supported, consider checking for explicit
null:- const body = typeof event.payload["body"] === "string" ? event.payload["body"] : undefined + const body = event.payload["body"] === null ? "" : + typeof event.payload["body"] === "string" ? event.payload["body"] : undefined
223-253: LGTM!The
createTicketimplementation correctly builds and persists the event, then returns a constructed summary without re-reading from storage.
255-277: LGTM!List, get, and sync operations are straightforward. Sorting tickets by numeric ID provides consistent ordering.
288-313: LGTM!
parseEventrobustly validates all required fields and recomputestsIsofromtsfor consistency. Returningnullfor invalid events is appropriate for the forgiving read pattern.
| | Flag | Type | Default | Description | | ||
| | --- | --- | --- | --- | | ||
| | `-p`, `--path <dir>` | string | - | Run against a repo path (overrides cwd search) | | ||
| | `-c`, `--client <cursor|claude|codex|print>` | string | - | Open init prompt in an agent client (or print) | |
There was a problem hiding this comment.
Fix malformed table structure in hack setup cursor options.
The table at line 740 has inconsistent column count (expected 4, got 7). The header row should align with the data cells. Check if columns are being split unexpectedly, possibly due to pipe characters in cell content.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
740-740: Table column count
Expected: 4; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
In @docs/cli.md at line 740, The table row for the `-c`, `--client
<cursor|claude|codex|print>` option is producing extra pipe separators and
breaking the 4-column table structure; fix it by ensuring the entire option cell
is a single cell (escape or wrap the inner pipes such as
`<cursor|claude|codex|print>` so they don't split columns) and align it with the
existing header columns, keeping the row to exactly four pipe-separated cells;
update the offending row where the `-c`, `--client <cursor|claude|codex|print>`
entry appears to use proper escaping or quoting so the table parser treats the
value as one cell.
| | `--format <auto|docker-compose|plain>` | string | `auto` | How to parse incoming lines from stdin | | ||
| | `--stream <stdout|stderr>` | string | `stdout` | Treat stdin as stdout or stderr | |
There was a problem hiding this comment.
Fix malformed table structure in hack log-pipe options.
The tables at lines 852–853 have column count mismatches. Line 852 shows expected 4 but has 6 cells; line 853 shows expected 4 but has 5 cells. Verify table alignment and cell boundaries.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
852-852: Table column count
Expected: 4; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
853-853: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
In @docs/cli.md around lines 852 - 853, The two Markdown table rows for the hack
log-pipe options are malformed (extra pipe separators) causing column-count
mismatches; locate the rows containing `--format <auto|docker-compose|plain>`
and `--stream <stdout|stderr>` and edit each so they have exactly four
pipe-delimited cells matching the table header (Option | Type | Default |
Description), removing any stray pipes or merged cells, keep backticks around
option and default values, and verify the table alignment renders correctly.
| isCancel, | ||
| multiselect, | ||
| note, | ||
| select, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@clack/prompts latest stable version and 1.0.0-alpha.9 release status
💡 Result:
- Latest stable (npm "latest" tag): 0.11.0. [1][2]
- 1.0.0-alpha.9: I cannot find an official npm release/page for 1.0.0-alpha.9. Yarn’s package page shows the alpha line up to 1.0.0-alpha.6 (Oct 3, 2025), while Snyk’s metadata references 1.0.0-alpha.9 as a “latest non vulnerable” entry (not an npm registry page). This suggests alpha.9 is not published or not tagged on the public npm registry in a way that npm/yarn list it. [3][4][5]
Sources:
- npm package @clack/prompts (latest: 0.11.0). [npm page]. [1]
- npm versions / tags view for @clack/prompts. [2]
- Yarn classic package page (alpha tag shows 1.0.0-alpha.6). [3]
- Snyk package metadata referencing 1.0.0-alpha.9. [4]
- Bombshell / Clack docs (project pages). [5]
🏁 Script executed:
fd -type f -name "package.json" -o -name "package-lock.json" -o -name "yarn.lock" -o -name "pnpm-lock.yaml" | head -20Repository: hack-dance/hack-cli
Length of output: 233
🏁 Script executed:
if [ -f "package.json" ]; then
cat package.json | grep -A 2 -B 2 "@clack/prompts" || echo "No @clack/prompts found in package.json"
fiRepository: hack-dance/hack-cli
Length of output: 247
🏁 Script executed:
if [ -f "package-lock.json" ]; then
cat package-lock.json | grep -A 5 '"@clack/prompts"' | head -20 || echo "No @clack/prompts in package-lock.json"
fiRepository: hack-dance/hack-cli
Length of output: 45
🏁 Script executed:
cat package.json | grep -A 30 -B 5 '"@clack/prompts"' | head -50Repository: hack-dance/hack-cli
Length of output: 348
🏁 Script executed:
if [ -f "package.json" ]; then
cat package.json | grep -E '"(resolutions|overrides|pnpm)' -A 20
fiRepository: hack-dance/hack-cli
Length of output: 45
🏁 Script executed:
find . -maxdepth 2 -name ".npmrc" -o -name ".yarnrc" -o -name ".yarnrc.yml" -o -name "pnpm-workspace.yaml" | xargs cat 2>/dev/nullRepository: hack-dance/hack-cli
Length of output: 45
🏁 Script executed:
git config --get-all --local --global --system remote.origin.url 2>/dev/null || echo "Not a git repo or no origin"Repository: hack-dance/hack-cli
Length of output: 90
🏁 Script executed:
if [ -f "yarn.lock" ]; then
grep -A 10 "@clack/prompts@1.0.0-alpha.9" yarn.lock || grep -A 10 "@clack/prompts" yarn.lock | head -20
fiRepository: hack-dance/hack-cli
Length of output: 45
🏁 Script executed:
if [ -f "pnpm-lock.yaml" ]; then
grep -A 10 "@clack/prompts" pnpm-lock.yaml | head -20
fiRepository: hack-dance/hack-cli
Length of output: 45
🏁 Script executed:
# Check what actually exists in node_modules if available
if [ -d "node_modules/@clack/prompts" ]; then
cat node_modules/@clack/prompts/package.json | grep -E '"name"|"version"'
fiRepository: hack-dance/hack-cli
Length of output: 45
@clack/prompts version 1.0.0-alpha.9 does not exist on the npm registry.
The specified version appears to be unpublished or unavailable. The latest published alpha version is 1.0.0-alpha.6, and the stable release is 0.11.0. This dependency will likely fail to install. Confirm whether this version should be a git reference, a private registry version, or if it should be updated to an available version.
🤖 Prompt for AI Agents
In @src/commands/project.ts at line 7, The package version for @clack/prompts
referenced by the import (e.g., the symbol select imported from
'@clack/prompts') is invalid (1.0.0-alpha.9) and will fail to install; update
package.json to point to a published version (for example 1.0.0-alpha.6 or the
stable 0.11.0) or convert it to the correct git/private-registry reference, then
reinstall so the import of select resolves correctly.
| const ticketsSpec = defineCommand({ | ||
| name: "tickets", | ||
| summary: "Install Codex skill for hack tickets usage", | ||
| group: "Agents", | ||
| options: setupTicketsOptions, | ||
| positionals: [], | ||
| subcommands: [] | ||
| } as const) |
There was a problem hiding this comment.
Update the summary to reflect tickets, not Codex.
The summary currently reads "Install Codex skill for hack tickets usage", but this appears to be copied from the codexSpec definition. It should describe the tickets functionality specifically.
📝 Suggested fix
const ticketsSpec = defineCommand({
name: "tickets",
- summary: "Install Codex skill for hack tickets usage",
+ summary: "Install Tickets skill for tracking work items in-repo",
group: "Agents",
options: setupTicketsOptions,
positionals: [],
subcommands: []
} as const)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ticketsSpec = defineCommand({ | |
| name: "tickets", | |
| summary: "Install Codex skill for hack tickets usage", | |
| group: "Agents", | |
| options: setupTicketsOptions, | |
| positionals: [], | |
| subcommands: [] | |
| } as const) | |
| const ticketsSpec = defineCommand({ | |
| name: "tickets", | |
| summary: "Install Tickets skill for tracking work items in-repo", | |
| group: "Agents", | |
| options: setupTicketsOptions, | |
| positionals: [], | |
| subcommands: [] | |
| } as const) |
🤖 Prompt for AI Agents
In @src/commands/setup.ts around lines 131 - 138, The summary on the ticketsSpec
command is incorrect (it references Codex); update the summary field in the
ticketsSpec definition to describe the tickets feature (e.g., "Install Tickets
skill for hack ticket management" or similar) so it accurately reflects the
tickets functionality instead of copying codexSpec text.
| const computeNextTicketId = async (): Promise<string> => { | ||
| const tickets = await materializeTickets() | ||
| let max = 0 | ||
| for (const ticketId of tickets.keys()) { | ||
| const n = parseTicketNumber(ticketId) | ||
| if (n !== null && n > max) max = n | ||
| } | ||
| return formatTicketId(max + 1) | ||
| } |
There was a problem hiding this comment.
Race condition risk in ticket ID generation.
computeNextTicketId reads all tickets to determine the next ID, but between computing the ID and appending the event (line 237), another concurrent process could create a ticket with the same ID. This would result in duplicate ticket IDs.
For a single-user CLI this may be acceptable, but for multi-process/multi-user scenarios, consider:
- Using a lock file during ticket creation
- Using a counter file with atomic increment
- Including a UUID suffix as a tiebreaker
🤖 Prompt for AI Agents
In @src/control-plane/extensions/tickets/store.ts around lines 194 - 202,
computeNextTicketId currently scans materializeTickets, uses parseTicketNumber
and formatTicketId, then code later appends a ticket event — this allows races
and duplicate IDs when concurrent processes create tickets; fix by making ID
generation atomic: either (A) add a dedicated counter store and replace
computeNextTicketId to perform an atomic increment (e.g., increment-and-read a
counter file/DB entry) so each process gets a unique sequential ID, or (B)
acquire a process-wide lock around computeNextTicketId + the append step (use a
lock file or mutex) to serialize creation, or (C) append a collision-safe suffix
(e.g., a short UUID or timestamp) to the value returned by formatTicketId to
guarantee uniqueness; update all callers of computeNextTicketId and the
ticket-id format logic so parsing/formatting remains consistent with the chosen
approach.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/control-plane/extensions/tickets/store.ts:
- Around line 273-308: createTicket currently computes ticketId via
computeNextTicketId and can produce collisions under concurrency; wrap the ID
allocation+append in a distributed lock (or call out to a centralized allocator)
so only one process can reserve the next ID at a time: acquire a lock keyed by
the project/namespace before calling computeNextTicketId, build the event and
call git.appendEvents, then release the lock (handle failures by releasing and
retrying). Alternatively, replace computeNextTicketId with a central allocator
service you call from createTicket. Ensure any retry-after-fetch logic still
verifies ticketId uniqueness (not just eventId) by checking existing tickets for
the allocated ticketId before returning success; reference createTicket,
computeNextTicketId, git.appendEvents and the retry/apply logic that currently
deduplicates by eventId.
In @src/control-plane/extensions/tickets/util.ts:
- Around line 17-24: The parseTicketNumber function currently allows negative
values like "T--5"; update parseTicketNumber to validate the parsed numeric
value is non-negative before returning: after converting rest to Number and
confirming Number.isFinite(n), check that n >= 0 (or Math.trunc(n) >= 0) and
return null for negatives; keep existing trimming, "T-" prefix check, finite
check, and return Math.trunc(n) only when the value is non-negative.
- Around line 12-15: formatTicketId currently assumes n is a non-negative
integer; add input validation at the top of the function (formatTicketId) to
ensure Number.isInteger(n) and n >= 0 and throw a clear TypeError if not, so
negatives, floats, NaN or non-numbers are rejected and malformed IDs like
"T--0005" or "T-1.23" can't be produced; keep the existing padding logic after
the guard.
🧹 Nitpick comments (5)
src/control-plane/extensions/tickets/util.ts (1)
42-58: Consider making the fallback explicit or removing it.Lines 52-53 use
?? 0as a fallback whenparseTicketNumberreturns null. Since all values inoutcome fromformatTicketId(which produces valid "T-XXXXX" format),parseTicketNumbershould never return null here. The fallback could mask bugs.♻️ Option 1: Remove the fallback and assert validity
out.sort((a, b) => { - const an = parseTicketNumber(a) ?? 0 - const bn = parseTicketNumber(b) ?? 0 + const an = parseTicketNumber(a)! + const bn = parseTicketNumber(b)! if (an !== bn) return an - bn return a.localeCompare(b) })Option 2: Keep the fallback but add a runtime check:
out.sort((a, b) => { const an = parseTicketNumber(a) ?? 0 const bn = parseTicketNumber(b) ?? 0 + if (an === 0) throw new Error(`Failed to parse normalized ticket: ${a}`) + if (bn === 0) throw new Error(`Failed to parse normalized ticket: ${b}`) if (an !== bn) return an - bn return a.localeCompare(b) })src/control-plane/extensions/tickets/store.ts (2)
242-251: Performance: computeNextTicketId reads all events on every ticket creation.The
computeNextTicketIdfunction materializes all tickets from the entire event log just to find the maximum ticket ID. For projects with many tickets, this could become a performance bottleneck during ticket creation.Consider caching the last ticket ID or maintaining it in a separate, lightweight file that's updated atomically.
96-101: The hostname-based actor fallback is a standard pattern but consider privacy requirements for event tracking.The
resolveActorfunction useshostname()only as a final fallback when no explicit actor is provided and theUSERenvironment variable is unavailable. While this pattern is common for audit trails in event-sourced systems, hostnames can contain sensitive information. If your privacy requirements restrict exposing machine identifiers in stored events, consider alternatives such as using a generic identifier or requiring explicit actor specification in this context.src/control-plane/extensions/tickets/commands.ts (1)
658-805: Consider refactoring the argument parser for better maintainability.The
parseTicketsArgsfunction is 147 lines long with repetitive patterns for handling different flags. While functional, this approach makes it harder to maintain and extend.Consider using a more declarative approach, such as a configuration-driven parser or a library like
commanderoryargs, which would reduce duplication and make the argument schema more explicit.src/control-plane/extensions/tickets/agent-docs.ts (1)
118-150: Consider: Hardcoded documentation might become stale.The
renderTicketsAgentDocsSnippetfunction returns a static markdown string with command examples. If command signatures change in the future, this documentation would need manual updates.While this is a minor concern and acceptable for agent documentation, consider whether generating examples dynamically from command metadata would provide better consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.codex/skills/hack-tickets/SKILL.mdsrc/control-plane/extensions/tickets/agent-docs.tssrc/control-plane/extensions/tickets/commands.tssrc/control-plane/extensions/tickets/store.tssrc/control-plane/extensions/tickets/tickets-skill.tssrc/control-plane/extensions/tickets/util.tssrc/tui/tickets-tui.tstests/gateway-e2e.test.tstests/helpers/ci.tstests/log-pipe.test.tstests/mcp.test.tstests/tickets-extension.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/control-plane/extensions/tickets/tickets-skill.ts
- .codex/skills/hack-tickets/SKILL.md
🧰 Additional context used
🧬 Code graph analysis (3)
src/control-plane/extensions/tickets/agent-docs.ts (1)
src/lib/fs.ts (3)
pathExists(3-15)readTextFile(21-27)writeTextFileIfChanged(33-41)
src/control-plane/extensions/tickets/commands.ts (9)
src/control-plane/extensions/types.ts (2)
ExtensionCommand(24-33)ExtensionCommandContext(15-22)src/ui/terminal.ts (1)
isTty(6-8)src/ui/gum.ts (1)
isGumAvailable(53-55)src/control-plane/extensions/tickets/tickets-skill.ts (2)
checkTicketsSkill(45-67)installTicketsSkill(19-43)src/control-plane/extensions/tickets/agent-docs.ts (3)
checkTicketsAgentDocs(56-84)removeTicketsAgentDocs(86-116)upsertTicketsAgentDocs(31-54)src/ui/display.ts (1)
display(237-261)src/control-plane/extensions/tickets/store.ts (1)
createTicketsStore(54-378)src/tui/tickets-tui.ts (1)
runTicketsTui(66-911)src/control-plane/extensions/tickets/util.ts (2)
normalizeTicketRef(26-40)normalizeTicketRefs(42-58)
src/control-plane/extensions/tickets/store.ts (3)
src/control-plane/extensions/tickets/tickets-git-channel.ts (1)
createGitTicketsChannel(28-411)src/control-plane/extensions/tickets/util.ts (4)
unixSeconds(1-3)parseTicketNumber(17-24)formatTicketId(12-15)normalizeTicketRefs(42-58)src/lib/guards.ts (1)
isRecord(1-3)
🔇 Additional comments (14)
src/control-plane/extensions/tickets/util.ts (5)
1-3: LGTM!The implementation correctly converts milliseconds to Unix seconds.
5-10: LGTM!The function correctly converts Unix timestamps to UTC-based YYYY-MM format with proper month indexing and zero-padding.
26-40: Logic is sound.The normalization logic correctly handles various input formats (
#T-123,T-123,123) and produces canonical output. The function appropriately leveragesparseTicketNumberandformatTicketIdfor consistency.
60-62: LGTM!Clean wrapper for producing stable JSON output.
64-77: LGTM!Standard and correct implementation of recursive object key sorting for stable JSON serialization.
src/control-plane/extensions/tickets/store.ts (3)
380-384: LGTM: Robust dependency parsing.The
parseDependencyListfunction safely filters for string types and normalizes ticket references, providing good resilience against malformed data.
386-394: LGTM: Correct use of hasOwnProperty.Using
Object.prototype.hasOwnProperty.callis the correct and safe way to check for property existence, avoiding issues with objects that might have a customhasOwnPropertyproperty.
396-417: LGTM: Derived blocks computation is sound.The
applyDerivedBlocksfunction correctly computes reverse dependencies by iterating through all tickets'dependsOnarrays and adding the dependent ticket to each dependency'sblocksarray. The use ofnormalizeTicketRefsensures deduplication and sorting.src/control-plane/extensions/tickets/commands.ts (3)
807-832: Verify: Reading entire stdin into memory for ticket bodies.Line 815 reads all of stdin into memory with
Bun.stdin.text(). For typical ticket body content, this is acceptable. However, if users accidentally pipe large files or streams, this could cause memory issues.Consider whether a size limit or streaming approach would be beneficial, or document the expected input size.
22-624: LGTM: Well-structured command handlers with comprehensive error handling.The command implementations demonstrate good practices:
- Consistent project context validation
- Clear error messages with usage examples
- Support for both interactive and JSON output modes
- Proper separation of concerns (parsing, validation, business logic, output)
The
maybeEnsureTicketsSetuphelper is a nice touch that guides users through incomplete setup.
84-100: Good UX: Interactive prompts with sensible fallbacks.The setup command correctly checks for TTY and Gum availability before prompting interactively, and provides clear instructions for non-interactive environments. The default of
truefor untracking is appropriate since it's the safer option.src/control-plane/extensions/tickets/agent-docs.ts (3)
31-54: LGTM: Efficient upsert implementation with minimal writes.The
upsertTicketsAgentDocsfunction optimally renders the snippet once and useswriteTextFileIfChangedto avoid unnecessary disk writes. The per-target error handling ensures one failure doesn't affect other targets.
163-180: LGTM: upsertSnippet correctly preserves surrounding content.The function properly handles both updating existing snippets (lines 168-174) and appending new ones (lines 176-179). The use of
trimEnd()andtrimStart()ensures clean formatting, and filtering out empty strings prevents extra blank lines.
182-195: LGTM: Clean snippet removal with content preservation.The
removeSnippetfunction correctly extracts and rejoins content around the marked snippet, handling edge cases like missing markers or empty results. The final formatting ensures the file ends with a single newline.
| createTicket: async input => { | ||
| const ticketId = await computeNextTicketId() | ||
| const dependsOn = normalizeTicketRefs(input.dependsOn ?? []) | ||
| const blocks = normalizeTicketRefs(input.blocks ?? []) | ||
| const event = buildEvent({ | ||
| ticketId, | ||
| type: "ticket.created", | ||
| payload: { | ||
| title: input.title, | ||
| ...(input.body ? { body: input.body } : {}), | ||
| ...(dependsOn.length > 0 ? { dependsOn } : {}), | ||
| ...(blocks.length > 0 ? { blocks } : {}), | ||
| status: "open" | ||
| }, | ||
| actor: input.actor | ||
| }) | ||
|
|
||
| const wrote = await git.appendEvents({ events: [event] }) | ||
| if (!wrote.ok) return wrote | ||
|
|
||
| return { | ||
| ok: true, | ||
| ticket: { | ||
| ticketId, | ||
| title: input.title, | ||
| ...(input.body ? { body: input.body } : {}), | ||
| status: "open", | ||
| createdAt: event.tsIso, | ||
| updatedAt: event.tsIso, | ||
| dependsOn, | ||
| blocks, | ||
| ...(opts.projectId ? { projectId: opts.projectId } : {}), | ||
| ...(opts.projectName ? { projectName: opts.projectName } : {}) | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find where computeNextTicketId is defined
rg -n "computeNextTicketId" src/control-plane/extensions/tickets/ --type=ts -B 2 -A 5Repository: hack-dance/hack-cli
Length of output: 1446
🏁 Script executed:
# Check for any lock or concurrency mechanisms in the tickets directory
rg -n "lock|mutex|semaphore|concurrent" src/control-plane/extensions/tickets/ --type=tsRepository: hack-dance/hack-cli
Length of output: 6746
🏁 Script executed:
# Look at the store.ts file structure to find computeNextTicketId
head -100 src/control-plane/extensions/tickets/store.tsRepository: hack-dance/hack-cli
Length of output: 3506
🏁 Script executed:
# Search for how event files are organized and partitioned
rg -n "month|partition|appendEvents" src/control-plane/extensions/tickets/ --type=ts -A 3Repository: hack-dance/hack-cli
Length of output: 3076
🏁 Script executed:
# Check git operations handling
rg -n "appendEvents|git\." src/control-plane/extensions/tickets/ --type=ts -B 2 -A 5 | head -80Repository: hack-dance/hack-cli
Length of output: 5218
🏁 Script executed:
# Get the full appendEvents implementation
sed -n '350,400p' src/control-plane/extensions/tickets/tickets-git-channel.tsRepository: hack-dance/hack-cli
Length of output: 1567
🏁 Script executed:
# Look for git push and merge conflict handling
rg -n "git push|git merge|conflict|CONFLICT" src/control-plane/extensions/tickets/ --type=ts -B 2 -A 5Repository: hack-dance/hack-cli
Length of output: 1685
🏁 Script executed:
# Check the full computeNextTicketId function
sed -n '242,260p' src/control-plane/extensions/tickets/store.tsRepository: hack-dance/hack-cli
Length of output: 816
🏁 Script executed:
# Understand how readAllEvents works
sed -n '123,150p' src/control-plane/extensions/tickets/store.tsRepository: hack-dance/hack-cli
Length of output: 967
🏁 Script executed:
# Check the conflict detection logic in pushWithRetry
sed -n '320,348p' src/control-plane/extensions/tickets/tickets-git-channel.tsRepository: hack-dance/hack-cli
Length of output: 1240
🏁 Script executed:
# Look for where pendingEvents conflict check happens
sed -n '336,345p' src/control-plane/extensions/tickets/tickets-git-channel.ts -A 10Repository: hack-dance/hack-cli
Length of output: 1885
🏁 Script executed:
# Get more context around the conflict detection
sed -n '320,350p' src/control-plane/extensions/tickets/tickets-git-channel.tsRepository: hack-dance/hack-cli
Length of output: 1280
🏁 Script executed:
# Check if there's any validation of eventIds or ticketIds during merge/rebase
rg -n "eventId|ticketId" src/control-plane/extensions/tickets/tickets-git-channel.ts -B 2 -A 2Repository: hack-dance/hack-cli
Length of output: 1346
🏁 Script executed:
# Look for any deduplication or conflict detection logic
rg -n "duplicate|collision|conflict|deduplicate" src/control-plane/extensions/tickets/ --type=tsRepository: hack-dance/hack-cli
Length of output: 45
🏁 Script executed:
# Check what happens during checkoutHead - does it do a merge or rebase?
rg -n "checkoutHead" src/control-plane/extensions/tickets/tickets-git-channel.ts -B 5 -A 15Repository: hack-dance/hack-cli
Length of output: 2485
🏁 Script executed:
# Check how eventIds are generated - are they UUIDs?
rg -n "eventId|randomUUID" src/control-plane/extensions/tickets/ --type=ts -B 2 -A 2Repository: hack-dance/hack-cli
Length of output: 6323
🏁 Script executed:
# Look at buildEvent to see how eventId and ticketId are set
rg -n "buildEvent" src/control-plane/extensions/tickets/ --type=ts -B 5 -A 15Repository: hack-dance/hack-cli
Length of output: 6379
Ticket ID collisions are possible in concurrent scenarios—implement distributed locking or centralized ID allocation.
Two concurrent ticket creation calls will compute the same ticketId from computeNextTicketId() (which simply reads the max existing ID and increments it with no synchronization). While the system prevents duplicate events via eventId deduplication (UUIDs), it does not validate ticketId uniqueness. When process A and process B both compute the next ID as "T-00006", both will create events with different eventIds but the same ticketId. On push failure, the retry-after-fetch mechanism (lines 333–347) re-applies pending events, but the ticketId collision persists because deduplication only checks eventId, not ticketId.
This risk is realistic if multiple processes or instances of the CLI create tickets concurrently. Implement distributed locking (e.g., via Git branch locks, Redis locks, or a dedicated service) or a centralized ID allocation mechanism to guarantee monotonic ticket IDs.
🤖 Prompt for AI Agents
In @src/control-plane/extensions/tickets/store.ts around lines 273 - 308,
createTicket currently computes ticketId via computeNextTicketId and can produce
collisions under concurrency; wrap the ID allocation+append in a distributed
lock (or call out to a centralized allocator) so only one process can reserve
the next ID at a time: acquire a lock keyed by the project/namespace before
calling computeNextTicketId, build the event and call git.appendEvents, then
release the lock (handle failures by releasing and retrying). Alternatively,
replace computeNextTicketId with a central allocator service you call from
createTicket. Ensure any retry-after-fetch logic still verifies ticketId
uniqueness (not just eventId) by checking existing tickets for the allocated
ticketId before returning success; reference createTicket, computeNextTicketId,
git.appendEvents and the retry/apply logic that currently deduplicates by
eventId.
| export function formatTicketId(n: number): string { | ||
| const padded = String(n).padStart(5, "0") | ||
| return `T-${padded}` | ||
| } |
There was a problem hiding this comment.
Consider adding input validation.
The function assumes n is a non-negative integer but doesn't validate this. Negative numbers would produce malformed IDs like "T--0005", and floats would include decimals.
✅ Proposed fix to add validation
export function formatTicketId(n: number): string {
+ if (!Number.isInteger(n) || n < 0) {
+ throw new Error(`Invalid ticket number: ${n}`)
+ }
const padded = String(n).padStart(5, "0")
return `T-${padded}`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function formatTicketId(n: number): string { | |
| const padded = String(n).padStart(5, "0") | |
| return `T-${padded}` | |
| } | |
| export function formatTicketId(n: number): string { | |
| if (!Number.isInteger(n) || n < 0) { | |
| throw new Error(`Invalid ticket number: ${n}`) | |
| } | |
| const padded = String(n).padStart(5, "0") | |
| return `T-${padded}` | |
| } |
🤖 Prompt for AI Agents
In @src/control-plane/extensions/tickets/util.ts around lines 12 - 15,
formatTicketId currently assumes n is a non-negative integer; add input
validation at the top of the function (formatTicketId) to ensure
Number.isInteger(n) and n >= 0 and throw a clear TypeError if not, so negatives,
floats, NaN or non-numbers are rejected and malformed IDs like "T--0005" or
"T-1.23" can't be produced; keep the existing padding logic after the guard.
| export function parseTicketNumber(ticketId: string): number | null { | ||
| const trimmed = ticketId.trim() | ||
| if (!trimmed.startsWith("T-")) return null | ||
| const rest = trimmed.slice(2) | ||
| const n = Number(rest) | ||
| if (!Number.isFinite(n)) return null | ||
| return Math.trunc(n) | ||
| } |
There was a problem hiding this comment.
Add validation for non-negative numbers.
The function doesn't validate that the parsed number is non-negative. Input like "T--5" would parse to -5, which should be invalid for a ticket number.
✅ Proposed fix to reject negative numbers
export function parseTicketNumber(ticketId: string): number | null {
const trimmed = ticketId.trim()
if (!trimmed.startsWith("T-")) return null
const rest = trimmed.slice(2)
const n = Number(rest)
- if (!Number.isFinite(n)) return null
+ if (!Number.isFinite(n) || n < 0) return null
return Math.trunc(n)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function parseTicketNumber(ticketId: string): number | null { | |
| const trimmed = ticketId.trim() | |
| if (!trimmed.startsWith("T-")) return null | |
| const rest = trimmed.slice(2) | |
| const n = Number(rest) | |
| if (!Number.isFinite(n)) return null | |
| return Math.trunc(n) | |
| } | |
| export function parseTicketNumber(ticketId: string): number | null { | |
| const trimmed = ticketId.trim() | |
| if (!trimmed.startsWith("T-")) return null | |
| const rest = trimmed.slice(2) | |
| const n = Number(rest) | |
| if (!Number.isFinite(n) || n < 0) return null | |
| return Math.trunc(n) | |
| } |
🤖 Prompt for AI Agents
In @src/control-plane/extensions/tickets/util.ts around lines 17 - 24, The
parseTicketNumber function currently allows negative values like "T--5"; update
parseTicketNumber to validate the parsed numeric value is non-negative before
returning: after converting rest to Number and confirming Number.isFinite(n),
check that n >= 0 (or Math.trunc(n) >= 0) and return null for negatives; keep
existing trimming, "T-" prefix check, finite check, and return Math.trunc(n)
only when the value is non-negative.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @CLAUDE.md:
- Line 44: Fix the typos in the policy sentence on line 44: change "sensure" to
"ensure" and "tpye" to "type" so the sentence reads correctly about leveraging
generics and smart types to ensure the best possible type inference across the
project.
🧹 Nitpick comments (2)
docs/extensions.md (2)
386-406: Minor formatting and wording refinements in gateway exposure section.Line 390: "ad‑hoc" uses a special character; standard hyphen "ad-hoc" is preferred.
Line 400: "Zero Trust/VPN network" is slightly redundant; consider: "Move to a private Zero Trust network or VPN".
💄 Suggested refinements
- SSH tunnel (ad‑hoc): `ssh -L 7788:127.0.0.1:7788 <host>` + SSH tunnel (ad-hoc): `ssh -L 7788:127.0.0.1:7788 <host>` - 2) Move to a private Zero Trust/VPN network once you want persistent access. + 2) Move to a private Zero Trust network or VPN once you want persistent access.
411-439: Add language specification to fenced code blocks.Lines 411, 425, and 435 contain JSON examples in fenced code blocks without language specification. Adding
jsonwill enable proper syntax highlighting and improve markdown linting compliance.💄 Proposed fix
## Supervisor streaming output (JSON) `hack x supervisor job-tail --json` and `job-attach --json` emit line-delimited JSON events: - ``` + ```json {"type":"start","jobId":"...","logsOffset":0,"eventsSeq":0} ... - ``` + ```json Client → server (JSON frames): - ``` + ```json {"type":"hello","cols":120,"rows":30}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
AGENTS.mdCLAUDE.mddocs/README.mddocs/cli.mddocs/extensions.mddocs/guides/tickets.mddocs/supervisor.md
✅ Files skipped from review due to trivial changes (1)
- docs/supervisor.md
🚧 Files skipped from review as they are similar to previous changes (3)
- AGENTS.md
- docs/README.md
- docs/guides/tickets.md
🧰 Additional context used
🪛 LanguageTool
CLAUDE.md
[grammar] ~44-~44: Ensure spelling is correct
Context: ... leveraging generics and smart types to sensure the best possible tpye inference across...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~44-~44: Ensure spelling is correct
Context: ...mart types to sensure the best possible tpye inference across the project. <!-- hac...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/cli.md
[style] ~274-~274: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e combined with --pretty. - --until cannot be combined with --follow. ### hack ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/extensions.md
[grammar] ~390-~390: Ensure spelling is correct
Context: ...xpose it through one of: - SSH tunnel (ad‑hoc): ssh -L 7788:127.0.0.1:7788 <host> -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~400-~400: This phrase is redundant (‘N’ stands for ‘network’). Use simply “VPN”.
Context: ...access. 2) Move to a private Zero Trust/VPN network once you want persistent access. 3) Add...
(ACRONYM_TAUTOLOGY)
🪛 markdownlint-cli2 (0.18.1)
docs/cli.md
61-61: Table column count
Expected: 2; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
743-743: Table column count
Expected: 4; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
765-765: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
783-783: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
855-855: Table column count
Expected: 4; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
856-856: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
908-908: Table column count
Expected: 4; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
docs/extensions.md
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
142-142: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
411-411: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
425-425: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
435-435: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
CLAUDE.md (2)
46-71: Well-documented tickets workflow.The new tickets section clearly explains commands, the recommended body template, and data location. This aligns well with the tickets extension described in the broader PR.
73-80: Clear session completion workflow.The "Landing the Plane" section provides actionable steps for ending work sessions. This complements the tickets system and promotes good collaborative practices.
docs/cli.md (1)
1-50: Comprehensive and well-organized CLI reference.The documentation is clearly structured with conventions, logical command groupings, and consistent formatting. The coverage appears thorough and should serve as a good reference for users.
docs/extensions.md (1)
1-100: Well-structured extension surface documentation.The file provides clear definitions of the extension API, configuration, dispatch model, and built-in extensions. Type definitions are precise, and command references are comprehensive.
| TypeScript (strict). Runtimes: Bun 1.3+, Node 23. Prettier: 2 spaces, no semicolons, double quotes, width 100. Import order: @ianvs/prettier-plugin-sort-imports + Tailwind plugin. ESLint (flat): any disallowed; unused vars warned (prefix \_ to ignore). Naming: React components PascalCase; files kebab-case (e.g., user-profile.ts); packages @repo/<name>. Comments: avoid writing inline comments everywhere, unless absolutely necessary for a todo or an important thing to take note of. Instead write tsdoc style block level comments at the method/class/function/route level. Focus on comments that provide value in regards to better type inference and clarity of usage. Using things like params/returns/etc.. | ||
|
|
||
| Always default to useing named paramaters in functions eg myFunction({ ctx, other }) vs myFunction(ctx, other) | ||
| Never use any types and always default to leveraging generics and smart types to sensure the best possible tpye inference across the project. |
There was a problem hiding this comment.
Fix spelling errors in type inference policy line.
Line 44 contains typos: "sensure" → "ensure" and "tpye" → "type".
✏️ Proposed fix
- Never use any types and always default to leveraging generics and smart types to sensure the best possible tpye inference across the project.
+ Never use any types and always default to leveraging generics and smart types to ensure the best possible type inference across the project.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Never use any types and always default to leveraging generics and smart types to sensure the best possible tpye inference across the project. | |
| Never use any types and always default to leveraging generics and smart types to ensure the best possible type inference across the project. |
🧰 Tools
🪛 LanguageTool
[grammar] ~44-~44: Ensure spelling is correct
Context: ... leveraging generics and smart types to sensure the best possible tpye inference across...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~44-~44: Ensure spelling is correct
Context: ...mart types to sensure the best possible tpye inference across the project. <!-- hac...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In @CLAUDE.md at line 44, Fix the typos in the policy sentence on line 44:
change "sensure" to "ensure" and "tpye" to "type" so the sentence reads
correctly about leveraging generics and smart types to ensure the best possible
type inference across the project.
- Add apps/macos/build/ to gitignore (Xcode build output) - Add .zig-cache/, zig-cache/, zig-out/ to gitignore - Untrack 2207 previously committed build artifacts - Add ensureGitignoreEntry() helper to lib/fs.ts - Auto-add .hack/.internal/ to .gitignore during hack init The .hack/.internal directory contains machine-specific data like cert paths and extra_hosts mappings that shouldn't be committed.
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
6-15: Fix mismatched closing tag for the ASCII-art block.
<pre>is opened but closed with</p>, which is invalid HTML and can break rendering.🛠️ Proposed fix
-</p> +</pre>
🤖 Fix all issues with AI agents
In
`@apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swift`:
- Around line 304-334: Mark the C-interop raw structs as `@frozen` to guarantee
stable memory layout: update the declarations for GhosttyRenderColorRaw,
GhosttyRenderCellRaw, and GhosttyRenderSnapshotRaw by adding the `@frozen`
attribute before each struct keyword (i.e., change "struct
GhosttyRenderColorRaw", "struct GhosttyRenderCellRaw", and "struct
GhosttyRenderSnapshotRaw" to "@frozen struct ..."). This ensures the field
ordering and memory layout remain stable for dlsym/C interop.
In
`@apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/PtyProcess.swift`:
- Around line 26-44: The init currently ignores tcgetattr failures and always
passes a zeroed termios to openpty; change it to check tcgetattr(STDIN_FILENO,
&term) and only pass &term to openpty if tcgetattr returns 0, otherwise pass nil
to preserve system defaults; update the openpty call in PtyProcess.init to use a
conditional term pointer (e.g., termPtr) so openpty(&master, &slave, nil,
termPtr, &size) is used when tcgetattr fails.
In `@scripts/macos-ghostty-setup.ts`:
- Around line 51-53: The regex in the minVersionMatch extraction is
double-escaped (e.g., "\\s", "\\d", "\\.") so it never matches; update the
RegExp passed to String.match in the readFileSync(...) call (the expression
assigning minVersionMatch) to use proper regex escapes (single backslashes in a
regex literal) so it matches minimum_zig_version = "X.Y.Z" and captures the
version into the existing capture group; ensure you leave the surrounding call
to readFileSync, path.join(vendorDir, "build.zig.zon"), and the minVersionMatch
variable intact.
In `@src/commands/usage.ts`:
- Around line 701-706: The readPidFile function should guard against
IO/permission errors and return null instead of allowing exceptions to bubble;
wrap the Bun.file(opts.path), await file.exists() and await file.text() calls in
a try/catch and on any caught error return null, then keep the existing parsing
logic (Number.parseInt(text.trim(), 10) and Number.isFinite check) so
readPidFile always resolves to a number or null even when file access fails.
♻️ Duplicate comments (5)
src/commands/project.ts (1)
7-7: Version issue already flagged.The
@clack/promptsversion 1.0.0-alpha.9 concern was previously raised. Ensure this dependency version is resolved.docs/gateway-api.md (4)
103-107: Add a language identifier to this fenced URL block.This was already flagged earlier; still applies.
🔎 Proposed fix
-``` +```text https://gateway.dimitri.computer</details> --- `146-151`: **Add a language identifier to this WS URL block.** This was already flagged earlier; still applies. <details> <summary>🔎 Proposed fix</summary> ```diff -``` +```text wss://gateway.example.com/control-plane/projects/<id>/shells/<id>/stream?token=<token></details> --- `338-342`: **Insert a blank line before the response table.** This was already flagged earlier; still applies. <details> <summary>🔎 Proposed fix</summary> ```diff Response: + | Field | Type | Description |
404-408: Insert a blank line before the response table.This was already flagged earlier; still applies.
🔎 Proposed fix
Response: + | Field | Type | Description |
🟡 Minor comments (18)
CLAUDE.md-43-43 (1)
43-43: Fix spelling errors in the named parameters guideline.Line 43 contains typos: "useing" → "using" and "paramaters" → "parameters".
✏️ Proposed fix
-Always default to useing named paramaters in functions eg myFunction({ ctx, other }) vs myFunction(ctx, other) +Always default to using named parameters in functions eg myFunction({ ctx, other }) vs myFunction(ctx, other)scripts/macos-ghostty-setup.ts-15-15 (1)
15-15: Handle missingzigcommand gracefully.If Zig is not installed, this line throws an unhandled exception. Consider wrapping in try/catch to provide a user-friendly error message.
Suggested fix
-const zigVersion = (await $`zig version`.text()).trim() +let zigVersion: string +try { + zigVersion = (await $`zig version`.text()).trim() +} catch { + console.error("Zig is not installed or not in PATH. Install Zig and retry.") + process.exit(1) +}scripts/macos-ghostty-setup.ts-72-74 (1)
72-74: Hardcoded version in error message.The error message contains
"mise install zig@0.15.2"but should use the dynamically parsedminVersionvalues for consistency.Suggested fix
console.error( - `Ghostty VT currently targets Zig ${minVersion.major}.${minVersion.minor}.x (min ${minVersion.major}.${minVersion.minor}.${minVersion.patch}). Install Zig ${minVersion.major}.${minVersion.minor}.${minVersion.patch} (mise: "mise install zig@0.15.2") and retry.` + `Ghostty VT currently targets Zig ${minVersion.major}.${minVersion.minor}.x (min ${minVersion.major}.${minVersion.minor}.${minVersion.patch}). Install Zig ${minVersion.major}.${minVersion.minor}.${minVersion.patch} (mise: "mise install zig@${minVersion.major}.${minVersion.minor}.${minVersion.patch}") and retry.` )scripts/macos-ghostty-setup.ts-10-13 (1)
10-13: Guard against undefinedHOMEenvironment variable.If
HOMEis not set,installDirresolves to/Library/Application Support/..., which is a system-wide path requiring elevated privileges and is likely unintended.Suggested fix
+const homeDir = process.env.HOME +if (!homeDir) { + console.error("HOME environment variable is not set.") + process.exit(1) +} const installDir = path.join( - process.env.HOME ?? "", + homeDir, "Library/Application Support/Hack/ghostty/lib" )src/commands/usage.ts-642-649 (1)
642-649: Fix the label forhack remotedetection.
hack remoteis currently reported ashack tui, which mislabels host usage rows.✅ Proposed fix
- if (normalized.includes(" hack remote")) return "hack tui" + if (normalized.includes(" hack remote")) return "hack remote"src/commands/usage.ts-752-755 (1)
752-755: Treat emptydocker statsoutput as a valid result, not an error.Containers can stop between indexing and stats collection, causing Docker to return no samples. This is a transient state, not a failure condition. The code already handles empty samples gracefully in downstream functions, and even establishes this pattern by manually returning
{ ok: true, samples: [] }whencontainerIds.length === 0. Returning an error for empty samples only when they come from the Docker command creates an unnecessary inconsistency.Proposed fix
- if (samples.length === 0) { - return { ok: false, error: "docker stats returned no samples" } - } - return { ok: true, samples } + return { ok: true, samples }AGENTS.md-76-79 (1)
76-79: Align workflow terminology with tickets.Line 79 still says “issue status”; this now conflicts with the tickets wording in Line 77.
✏️ Proposed edit
-3. **Update issue status** - Close finished work, update in-progress items +3. **Update ticket status** - Close finished work, update in-progress itemsexamples/basic/.hack/.internal/compose.override.yml-8-15 (1)
8-15: Avoid hard-coded absolute CA cert paths.Line 9 hard-codes
/Users/hack/..., which will break on other machines/CI. Prefer${HOME}or a configurable env var.🔧 Proposed fix
- - /Users/hack/.hack/caddy/pki/caddy-local-authority.crt:/etc/hack/ca/caddy-local-authority.crt:ro + - ${HOME}/.hack/caddy/pki/caddy-local-authority.crt:/etc/hack/ca/caddy-local-authority.crt:roexamples/next-app/.hack/.internal/compose.override.yml-1-71 (1)
1-71: De-duplicate CA config and avoid absolute user paths.The repeated blocks are error‑prone, and the
/Users/hack/...path won’t work for other devs/CI. Consider a YAML anchor +${HOME}.♻️ Suggested refactor pattern
+x-ca-config: &ca-config + dns: + - 172.30.0.2 + extra_hosts: + examples-next-app.hack: 172.30.0.3 + volumes: + - ${HOME}/.hack/caddy/pki/caddy-local-authority.crt:/etc/hack/ca/caddy-local-authority.crt:ro + environment: + SSL_CERT_FILE: /etc/hack/ca/caddy-local-authority.crt + SSL_CERT_DIR: /etc/hack/ca + NODE_EXTRA_CA_CERTS: /etc/hack/ca/caddy-local-authority.crt + REQUESTS_CA_BUNDLE: /etc/hack/ca/caddy-local-authority.crt + CURL_CA_BUNDLE: /etc/hack/ca/caddy-local-authority.crt + GIT_SSL_CAINFO: /etc/hack/ca/caddy-local-authority.crt + services: db: - dns: - - 172.30.0.2 - extra_hosts: - examples-next-app.hack: 172.30.0.3 - volumes: - - /Users/hack/.hack/caddy/pki/caddy-local-authority.crt:/etc/hack/ca/caddy-local-authority.crt:ro - environment: - SSL_CERT_FILE: /etc/hack/ca/caddy-local-authority.crt - SSL_CERT_DIR: /etc/hack/ca - NODE_EXTRA_CA_CERTS: /etc/hack/ca/caddy-local-authority.crt - REQUESTS_CA_BUNDLE: /etc/hack/ca/caddy-local-authority.crt - CURL_CA_BUNDLE: /etc/hack/ca/caddy-local-authority.crt - GIT_SSL_CAINFO: /etc/hack/ca/caddy-local-authority.crt + <<: *ca-configdocs/gateway-api.md-111-116 (1)
111-116: Add a period to “etc.”Minor grammar fix in the Tailscale section.
✏️ Proposed fix
-Best for SSH from iOS clients (Terminus, Blink, etc): +Best for SSH from iOS clients (Terminus, Blink, etc.):apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/DashboardView.swift-260-275 (1)
260-275: NormalizedevHostbefore building the URL.The context menu unconditionally prefixes
https://, which breaks whendevHostalready includes a scheme. This produces invalid URLs for Open/Copy.🛠 Proposed fix
- if let devHost = project.devHost, let url = URL(string: "https://\(devHost)") { - Divider() - - Button { - NSWorkspace.shared.open(url) - } label: { - Label("Open in Browser", systemImage: "safari") - } - - Button { - NSPasteboard.general.clearContents() - NSPasteboard.general.setString(url.absoluteString, forType: .string) - } label: { - Label("Copy URL", systemImage: "doc.on.doc") - } - } + if let devHost = project.devHost, !devHost.isEmpty { + let urlString = devHost.contains("://") ? devHost : "https://\(devHost)" + if let url = URL(string: urlString) { + Divider() + + Button { + NSWorkspace.shared.open(url) + } label: { + Label("Open in Browser", systemImage: "safari") + } + + Button { + NSPasteboard.general.clearContents() + NSPasteboard.general.setString(url.absoluteString, forType: .string) + } label: { + Label("Copy URL", systemImage: "doc.on.doc") + } + } + }apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift-48-57 (1)
48-57: Force Overview for non-runtime projects.When
project.isRuntimeConfiguredis false,tabContentcan still render Logs/Shell if a prior selection persists, which hides the runtime-not-configured guidance and can start unavailable sessions. Consider defaulting to Overview in that case.🛠 Proposed fix
`@ViewBuilder` private var tabContent: some View { - switch model.selectedProjectTab { - case .overview: - overviewContent - case .logs: - LogsView(project: project, embedded: true) - case .shell: - ShellView(project: project, embedded: true) - } + if !project.isRuntimeConfigured { + overviewContent + } else { + switch model.selectedProjectTab { + case .overview: + overviewContent + case .logs: + LogsView(project: project, embedded: true) + case .shell: + ShellView(project: project, embedded: true) + } + } }apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/StatusBadge.swift-43-87 (1)
43-87: Remove redundant optional initialization.
SwiftLint flags= nilas redundant for optional types. The same pattern also appears inRuntimeStatusDotat line 129.🧹 Proposed fixes
- var runtimeHealthy: Bool? = nil + var runtimeHealthy: Bool?apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/StatusBadge.swift-127-171 (1)
127-171: Remove redundant optional initialization.In Swift, optional variables implicitly initialize to
nil, so explicit= nilis unnecessary. SwiftLint flags this redundancy:- var runtimeHealthy: Bool? = nil + var runtimeHealthy: Bool?apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GhosttyTerminalSession.swift-69-81 (1)
69-81: Avoid settingisStartedbefore availability is confirmed.If the runtime is unavailable,
isStartedstaystrueand blocks retries. Move the assignment after the availability check (or reset on failure).🛠️ Proposed fix
- isStarted = true - guard isAvailable else { return } + guard isAvailable else { return } + isStarted = truedocs/rfcs/0001-multi-node-hack-cluster.md-103-115 (1)
103-115: Add language identifiers to fenced code blocks.This addresses MD040 and improves readability.
🛠️ Proposed fix
-``` +```text controlPlane.nodeId = "node-123"```diff -``` +```text session_id = "project:repo:agent:codex"</details> </blockquote></details> <details> <summary>apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swift-261-271 (1)</summary><blockquote> `261-271`: **Memory leak if `htmlString` returns a valid pointer with length 0.** Lines 264-266: If `ptr` is non-nil but `length == 0`, the function returns an empty string but never calls `freeString` to release the pointer. The same issue exists in `plainString` (lines 252-254). <details> <summary>Fix potential memory leak</summary> ```diff public func htmlString() -> String? { guard let htmlString = functions.htmlString else { return nil } var length = 0 - guard let ptr = htmlString(handle, &length), length > 0 else { - return "" + guard let ptr = htmlString(handle, &length) else { + return nil } + defer { functions.freeString(ptr, length) } + guard length > 0 else { return "" } let buffer = UnsafeBufferPointer(start: ptr, count: length) let text = String(decoding: buffer, as: UTF8.self) - functions.freeString(ptr, length) return text }Apply the same pattern to
plainString.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/DashboardModel.swift-80-90 (1)
80-90: Task is not cancelled on deinitialization.The
refreshTaskruns indefinitely but is never cancelled whenDashboardModelis deallocated. This could cause the task to continue running (with a nil weak self) or prevent deallocation if there's a strong reference cycle.Add deinit to cancel the refresh task
public init(client: HackCLIClient) { self.client = client } + deinit { + refreshTask?.cancel() + statusClearTask?.cancel() + } + public var selectedProject: ProjectSummary? {
🧹 Nitpick comments (28)
.gitignore (1)
64-64: Remove duplicatedistignore entry.
distis already ignored at Line 6; the second entry is redundant. Optional cleanup to reduce noise.apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLILocator.swift (1)
37-43: Consider using path-aware joining for robustness.The current string concatenation works in most cases, but PATH entries with trailing slashes would produce paths like
/usr/bin//hack. While Unix handles this gracefully, usingURLorNSStringpath APIs would be more robust.♻️ Optional: Use URL-based path construction
guard let pathValue = env["PATH"] else { return nil } for entry in pathValue.split(separator: ":") { - let candidate = String(entry) + "/hack" + let candidate = URL(fileURLWithPath: String(entry)).appendingPathComponent("hack").path if fileManager.isExecutableFile(atPath: candidate) { return candidate } }apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/LabelBadge.swift (1)
7-30: Platform-conditional rendering is well-implemented.The
glassEffectAPI was introduced in iOS 26 and macOS Tahoe as part of the "Liquid Glass" design system. The availability check and fallback implementation are appropriate.Consider extracting the shared modifiers (font, padding, foregroundStyle) to reduce duplication between branches.
♻️ Optional: Extract shared modifiers
struct BadgePill: View { let label: String let tint: Color var body: some View { + label + .font(.caption2.weight(.semibold)) + .padding(.horizontal, 8) + .padding(.vertical, 4) + .foregroundStyle(tint) + .modifier(BadgeBackground(tint: tint)) + } +} + +private struct BadgeBackground: ViewModifier { + let tint: Color + + func body(content: Content) -> some View { if `#available`(macOS 26, *) { - Text(label) - .font(.caption2.weight(.semibold)) - .padding(.horizontal, 8) - .padding(.vertical, 4) - .foregroundStyle(tint) + content .glassEffect(.regular.tint(tint.opacity(0.15))) } else { - Text(label) - .font(.caption2.weight(.semibold)) - .padding(.horizontal, 8) - .padding(.vertical, 4) - .foregroundStyle(tint) + content .background( Capsule(style: .continuous) .fill(.thinMaterial) .overlay( Capsule(style: .continuous) .stroke(tint.opacity(0.35), lineWidth: 1) ) ) } } }CLAUDE.md (1)
77-78: Consider portability of hardcoded vault paths.The Obsidian vault and project folder paths are hardcoded to specific absolute locations that may not be consistent across team members' environments. Consider documenting these as examples or using environment variables/configuration.
apps/macos/Config/Debug.xcconfig (1)
3-3: Preserve inherited compilation conditions.
Setting this directly can drop other flags defined by the project or dependencies. Prefer inheriting and appending DEBUG.♻️ Proposed fix
-SWIFT_ACTIVE_COMPILATION_CONDITIONS = DEBUG +SWIFT_ACTIVE_COMPILATION_CONDITIONS = $(inherited) DEBUGapps/macos/Config/Release.xcconfig (1)
3-3: Preserve inherited compilation conditions.
Setting this directly can drop other flags defined by the project or dependencies. Prefer inheriting and appending RELEASE.♻️ Proposed fix
-SWIFT_ACTIVE_COMPILATION_CONDITIONS = RELEASE +SWIFT_ACTIVE_COMPILATION_CONDITIONS = $(inherited) RELEASEscripts/macos-ghostty-setup.ts (1)
30-42:isAtLeastis defined but unused.This utility function is never called. The version check on lines 67-70 reimplements the comparison inline (with slightly different semantics). Either use
isAtLeastor remove it.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/AdaptiveStyles.swift (1)
29-37: Add rounded clipping on macOS 26 paths for visual consistency.The macOS 26 branches omit the rounded shape used in legacy branches, so the button silhouette can change between OS versions.
♻️ Suggested tweak
configuration.label .padding(.horizontal, 12) .padding(.vertical, 6) .foregroundStyle(.primary) + .clipShape(RoundedRectangle(cornerRadius: 8, style: .continuous)) .glassEffect(.regular) .opacity(configuration.isPressed ? 0.8 : 1.0)configuration.label .padding(.horizontal, 12) .padding(.vertical, 6) .foregroundStyle(.red) + .clipShape(RoundedRectangle(cornerRadius: 8, style: .continuous)) .glassEffect(.regular.tint(Color.red.opacity(0.15))) .opacity(configuration.isPressed ? 0.8 : 1.0)Also applies to: 53-61
apps/macos/docs/ghostty-vt.md (2)
70-72: Add a language identifier to the fenced code block.The code block representing the rendering pipeline lacks a language specifier. Based on static analysis hints (MD040), consider adding a language identifier for consistency.
📝 Suggested fix
-``` +```text PTY bytes -> Ghostty VT -> HTML snapshot -> NSTextView -``` +```
85-134: Consider using proper headings instead of bold emphasis.The checklist subsections use bold text (
**Bridge (Zig → C ABI)**, etc.) rather than Markdown headings. While readable, converting these to####headings would improve document structure and navigation (flagged by MD036).This is a minor stylistic preference and can be addressed later if desired.
apps/macos/Experiments/GhosttyVTBridge/src/bridge.zig (2)
75-86: Consider returning error status for resize failures.Lines 79 and 85 silently discard errors. While acceptable for
feed(VT streams often encounter unparseable sequences),resizefailures (e.g., allocation failure) could leave the terminal in an inconsistent state.Consider returning a boolean success indicator for
hack_ghostty_vt_resizeto allow callers to detect failures.♻️ Suggested change
-export fn hack_ghostty_vt_resize(handle: ?*TerminalHandle, cols: u32, rows: u32) void { +export fn hack_ghostty_vt_resize(handle: ?*TerminalHandle, cols: u32, rows: u32) bool { - if (handle == null) return; + if (handle == null) return false; const cols_u16 = toCellCount(cols) orelse return; const rows_u16 = toCellCount(rows) orelse return; - handle.?.terminal.resize(handle.?.alloc, cols_u16, rows_u16) catch {}; + handle.?.terminal.resize(handle.?.alloc, cols_u16, rows_u16) catch return false; + return true; }
234-239: Consider storing the allocator in the snapshot for consistent deallocation.The snapshot is allocated using
handle.?.alloc(line 128) but freed usingstd.heap.c_allocator(line 236). Currently both arec_allocator, but this creates a hidden coupling. IfTerminalHandle.allocever changes, this would cause undefined behavior.Consider either storing the allocator reference in the snapshot or adding a comment documenting this constraint.
apps/macos/README.md (1)
67-69: Add a language specifier to the fenced code block.The architecture diagram code block lacks a language specifier. Consider using
textorplaintextto satisfy markdown linters and improve accessibility.-``` +```text PTY bytes -> Ghostty VT (zig) -> formatter (HTML/plain) -> AppKit NSTextView</blockquote></details> <details> <summary>apps/macos/Package.swift (1)</summary><blockquote> `1-38`: **Package manifest is well-structured.** The dependency graph is clean and acyclic. The target paths are explicitly specified, which is good for non-standard layouts. Consider adding test targets for `HackCLIService`, `GhosttyTerminal`, and `DashboardFeature` to improve test coverage as the project matures. </blockquote></details> <details> <summary>apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SystemSidebarRows.swift (1)</summary><blockquote> `59-70`: **Consider moving icon mapping to the model layer.** The `iconName` computed property uses string literals to map exposure IDs to SF Symbol names. This logic could be encapsulated in the `GatewayExposure` model itself (similar to `statusColor`) to improve cohesion and reduce duplication if icons are needed elsewhere. <details> <summary>♻️ Potential refactor in GatewayExposure model</summary> ```swift // In GatewayExposure extension var iconName: String { switch id { case "lan": return "wifi" case "tailscale": return "link" case "cloudflare": return "cloud" default: return "network" } }Then simplify the view:
- private var iconName: String { - switch exposure.id { - case "lan": - return "wifi" - case "tailscale": - return "link" - case "cloudflare": - return "cloud" - default: - return "network" - } - } + // Use exposure.iconName directly in the Label
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/StatusPill.swift (1)
3-27: Consider explicit access modifiers for cross-module usage.
StatusToneandStatusPilldefault tointernalaccess. If these are intended for use outside this module (as suggested by the AI summary mentioning "shared UI element"), consider addingpublicaccess modifiers.♻️ Suggested change
-enum StatusTone { +public enum StatusTone { case good case warn case neutral } -struct StatusPill: View { - let text: String - let tone: StatusTone +public struct StatusPill: View { + public let text: String + public let tone: StatusTone + + public init(text: String, tone: StatusTone) { + self.text = text + self.tone = tone + }
src/commands/daemon.ts (1)
199-264: LGTM with minor suggestion.The enhanced status handler properly combines process state with API reachability for more accurate diagnostics. Consider using strict equality (
===) instead of loose equality (==) for the string comparisons on lines 247, 250, and 255 for consistency with TypeScript best practices.
src/commands/project.ts (1)
861-900: Consider extracting shared logic withresolveCoreDnsServer.This function is nearly identical to
resolveCoreDnsServer(lines 820-859), differing only in the service name and environment variable. Consider extracting a shared helper to reduce duplication.♻️ Suggested refactor
async function resolveGlobalComposeServiceIp(opts: { readonly envVar: string readonly serviceName: string }): Promise<string | null> { const env = (process.env[opts.envVar] ?? "").trim() if (env.length > 0) return env const home = process.env.HOME if (!home) return null const composePath = resolve( home, GLOBAL_HACK_DIR_NAME, GLOBAL_CADDY_DIR_NAME, GLOBAL_CADDY_COMPOSE_FILENAME ) if (!(await pathExists(composePath))) return null const ps = await exec(["docker", "compose", "-f", composePath, "ps", "-q", opts.serviceName], { cwd: dirname(composePath), stdin: "ignore" }) const id = ps.exitCode === 0 ? ps.stdout.trim() : "" if (!id) return null const inspect = await exec(["docker", "inspect", "--format", "{{json .NetworkSettings.Networks}}", id], { stdin: "ignore" }) if (inspect.exitCode !== 0) return null let parsed: unknown try { parsed = JSON.parse(inspect.stdout) } catch { return null } if (!isRecord(parsed)) return null const network = parsed[DEFAULT_INGRESS_NETWORK] if (!isRecord(network)) return null const ip = network["IPAddress"] return typeof ip === "string" && ip.length > 0 ? ip : null } // Usage: const resolveCoreDnsServer = () => resolveGlobalComposeServiceIp({ envVar: "HACK_COREDNS_IP", serviceName: "coredns" }) const resolveCaddyServer = () => resolveGlobalComposeServiceIp({ envVar: "HACK_CADDY_IP", serviceName: "caddy" })
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/DetailRows.swift (1)
3-24: Prefer a stable identifier from row data rather than UUID.When
rowsarray rebuilds (which is common in reactive views), creating fresh UUID instances causes ForEach to treat items as new, breaking state, focus, and animations. Using a stable id—such as the label or an explicit id provided by the caller—improves identity consistency across renders and aligns with SwiftUI best practices.♻️ Optional refactor
-struct DetailRowItem: Identifiable { - let id = UUID() - let label: String - let value: String -} +struct DetailRowItem: Identifiable { + let id: String + let label: String + let value: String + + init(id: String? = nil, label: String, value: String) { + self.id = id ?? label + self.label = label + self.value = value + } +}
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/GhosttyTerminalTextView.swift (5)
26-69: Font config parsing could silently ignore malformed values.The config parser handles missing files gracefully but doesn't log or report when individual config values fail to parse (e.g., invalid font-size). This is acceptable for a user-facing config, but consider logging parse failures during development for easier debugging.
Additionally,
lineHeightPaddingis hardcoded to2when loading from config but0in the default config - this inconsistency may cause unexpected behavior if the user has a config file.Consider making lineHeightPadding consistent
return TerminalFontConfig( fontFamily: fontFamily, fontSize: fontSize, fontThicken: fontThicken, - lineHeightPadding: 2 + lineHeightPadding: TerminalFontConfig.default.lineHeightPadding )
220-259: Cell size calculation is thorough but computationally intensive.The glyph measurement approach using CTFont is correct. However,
updateSizeis called fromonLayoutwhich can fire frequently during window resizing. The font metrics calculation (lines 225-247) is relatively expensive.Consider caching the computed
cellWidthandlineHeightsince they only depend onbaseFontwhich doesn't change after initialization.Cache computed cell metrics
`@MainActor` final class Coordinator { weak var session: GhosttyTerminalSession? weak var renderView: TerminalRenderView? var lastRenderVersion: Int = -1 private var lastCols: Int = 0 private var lastRows: Int = 0 private let fontConfig = TerminalFontConfig.loadFromGhosttyConfig() private lazy var baseFont: NSFont = fontConfig.resolveFont() + private lazy var cachedCellSize: CGSize = computeCellSize() + private lazy var cachedBaselineOffset: CGFloat = computeBaselineOffset() + + private func computeCellSize() -> CGSize { + // Move cell size calculation here, called once + let ctFont = baseFont as CTFont + // ... existing calculation logic ... + return CGSize(width: cellWidth, height: lineHeight) + }
261-267: Potential redundant main queue dispatch.
ensureFocusdispatches to main queue, but the Coordinator is already@MainActor. The dispatch is likely to break a potential update cycle, but this pattern can cause focus to be set one runloop iteration late.
271-308: TerminalRenderView initialization loads font config twice.Line 279 calls
TerminalFontConfig.loadFromGhosttyConfig().resolveFont()for the defaultfontproperty, but the Coordinator also loads it at line 142-143. This results in parsing the config file twice and potentially resolving different fonts if the file changes between calls.Use a simpler default that will be overwritten
- var font: NSFont = TerminalFontConfig.loadFromGhosttyConfig().resolveFont() { + var font: NSFont = NSFont.monospacedSystemFont(ofSize: 14, weight: .regular) { didSet { fontCache.removeAll() needsDisplay = true } }
626-772: Box-drawing lookup table is comprehensive and correctly indexed.The 128-entry lookup table (0x2500-0x257F) properly maps each box-drawing character to its line segments. The implementation handles corners, T-junctions, crosses, double lines, and arcs correctly.
Minor note: The array is recomputed on every access since it's a computed property. For a fixed lookup table, consider making it a static constant.
Make lookup table static
- /// Lookup table for box-drawing character line segments - private var boxDrawingLines: [(left: Bool, right: Bool, up: Bool, down: Bool)] { + /// Lookup table for box-drawing character line segments + private static let boxDrawingLines: [(left: Bool, right: Bool, up: Bool, down: Bool)] = [ [ // 2500-250F: Basic horizontal/vertical lines ... ] - } + ]
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/DashboardModel.swift (3)
33-51: Redundant nil initializations can be removed.As flagged by SwiftLint, optional properties don't need explicit
= nilinitialization. While this is stylistic, removing them reduces noise.Remove redundant nil initializations
- public private(set) var daemonStatus: DaemonStatus? = nil - public private(set) var globalStatus: GlobalStatusResponse? = nil - public private(set) var runtimeOk: Bool? = nil - public private(set) var runtimeError: String? = nil - public private(set) var runtimeCheckedAt: String? = nil - public private(set) var runtimeLastOkAt: String? = nil - public private(set) var runtimeResetAt: String? = nil - public private(set) var runtimeResetCount: Int? = nil - public private(set) var lastUpdated: Date? = nil + public private(set) var daemonStatus: DaemonStatus? + public private(set) var globalStatus: GlobalStatusResponse? + public private(set) var runtimeOk: Bool? + public private(set) var runtimeError: String? + public private(set) var runtimeCheckedAt: String? + public private(set) var runtimeLastOkAt: String? + public private(set) var runtimeResetAt: String? + public private(set) var runtimeResetCount: Int? + public private(set) var lastUpdated: Date? public var selectedItem: SidebarItem? = .runtime public var selectedProjectTab: ProjectTab = .overview - public var errorMessage: String? = nil - public var statusMessage: String? = nil + public var errorMessage: String? + public var statusMessage: String? public var isRefreshing = false private let client: HackCLIClient - private var refreshTask: Task<Void, Never>? = nil - private var statusClearTask: Task<Void, Never>? = nil + private var refreshTask: Task<Void, Never>? + private var statusClearTask: Task<Void, Never>?
92-108: Concurrent fetch pattern is good but error aggregation could be improved.Using
async letfor parallel fetches is efficient. However, the error messages are joined with newlines which may not render well in UI. Consider storing errors as an array or using a more structured error presentation.
208-227: Status clear task has a potential race condition.If
runActionis called twice in quick succession, the firststatusClearTaskis cancelled but the second one will clear the status message after 2 seconds, even if another action started. This is minor since the UX impact is just a prematurely cleared status.The current approach is acceptable for this use case, but for more robust status handling, consider tracking the action that set the status.
apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swift (1)
155-172: Library path resolution is reasonable but could be more discoverable.The code checks
HACK_GHOSTTY_VT_LIBenv var first, then falls back to a hardcoded path. Consider documenting the expected library location or providing a better error message when the library is not found.
| struct GhosttyRenderColorRaw { | ||
| var r: UInt8 | ||
| var g: UInt8 | ||
| var b: UInt8 | ||
| var a: UInt8 | ||
| } | ||
|
|
||
| struct GhosttyRenderCellRaw { | ||
| var codepoint: UInt32 | ||
| var fg: GhosttyRenderColorRaw | ||
| var bg: GhosttyRenderColorRaw | ||
| var wide: UInt8 | ||
| var flags: UInt16 | ||
| var _pad: UInt8 | ||
| } | ||
|
|
||
| struct GhosttyRenderSnapshotRaw { | ||
| var rows: UInt16 | ||
| var cols: UInt16 | ||
| var cursor_x: UInt16 | ||
| var cursor_y: UInt16 | ||
| var cursor_visible: UInt8 | ||
| var cursor_style: UInt8 | ||
| var cursor_wide_tail: UInt8 | ||
| var _pad0: UInt8 | ||
| var default_fg: GhosttyRenderColorRaw | ||
| var default_bg: GhosttyRenderColorRaw | ||
| var cursor_color: GhosttyRenderColorRaw | ||
| var cell_count: Int | ||
| var cells: UnsafeMutablePointer<GhosttyRenderCellRaw> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f \( -name "*.h" -o -name "*.hpp" -o -name "*.c" -o -name "*.cpp" \) | xargs rg -l 'GhosttyRenderCellRaw|GhosttyRenderSnapshotRaw|GhosttyRenderColorRaw' 2>/dev/null | head -20Repository: hack-dance/hack
Length of output: 149
🏁 Script executed:
# Also check for any C files that might define these structs
rg 'struct.*GhosttyRender' --type c --type h -B2 -A10 2>/dev/nullRepository: hack-dance/hack
Length of output: 41
🏁 Script executed:
# Look for bridging headers or C imports in the Swift file
cat apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swift | head -100Repository: hack-dance/hack
Length of output: 2365
🏁 Script executed:
sed -n '304,334p' apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swiftRepository: hack-dance/hack
Length of output: 752
🏁 Script executed:
# Check for module maps or C bridging headers
find . -name "*.modulemap" -o -name "*bridging*" -o -name "module.modulemap" | head -20Repository: hack-dance/hack
Length of output: 41
🏁 Script executed:
# Look for Ghostty as external dependency in package files
find . -name "Package.swift" -o -name "Package.resolved" | xargs cat 2>/dev/null | grep -i ghostty -A5 -B5 | head -50Repository: hack-dance/hack
Length of output: 1040
🏁 Script executed:
# Search for any imports or references to C modules at the top of the file
head -50 apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swift | grep -E "import|@_private|@_raw"Repository: hack-dance/hack
Length of output: 90
🏁 Script executed:
# Check context around lines 304-334 for `@frozen` attributes
sed -n '300,340p' apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swiftRepository: hack-dance/hack
Length of output: 917
🏁 Script executed:
# Search for Ghostty C library references or linkage
rg -i "ghostty" --type swift . | grep -i "import\|linkage\|external\|c" | head -20Repository: hack-dance/hack
Length of output: 2618
🏁 Script executed:
# Look for any memory safety or size validation code
rg "MemoryLayout|sizeof|withUnsafeBytes|withMemoryRebound" apps/macos/Packages/Services/GhosttyTerminal/ -B2 -A2 | head -40Repository: hack-dance/hack
Length of output: 680
🏁 Script executed:
# Check the full GhosttyTerminal.swift file size to understand scope
wc -l apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swiftRepository: hack-dance/hack
Length of output: 153
🏁 Script executed:
# Look for any MemoryLayout checks or size assertions in the file
rg "MemoryLayout|size|assert" apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swiftRepository: hack-dance/hack
Length of output: 393
🏁 Script executed:
# Search for any comments or documentation about C struct alignment
grep -n "@frozen\|C layout\|memory layout\|interop\|compatible" apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swiftRepository: hack-dance/hack
Length of output: 101
🏁 Script executed:
# Check if there's a bridging or C module declaration
find apps/macos/Packages/Services/GhosttyTerminal -name "*.modulemap" -o -name "*bridging*" -o -name "*.h"Repository: hack-dance/hack
Length of output: 41
Raw structs used in C interop must be marked @frozen to guarantee memory layout stability.
The structs GhosttyRenderColorRaw, GhosttyRenderCellRaw, and GhosttyRenderSnapshotRaw are used to interface with C functions loaded via dlsym. Without the @frozen attribute, Swift reserves the right to reorder fields or change memory layout in future compiler versions, breaking C interop compatibility. While the explicit _pad fields show layout awareness, only @frozen provides the guarantee required for safe C interoperability.
Add @frozen to all three raw structs:
@frozen struct GhosttyRenderColorRaw@frozen struct GhosttyRenderCellRaw@frozen struct GhosttyRenderSnapshotRaw
🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/GhosttyTerminal.swift`
around lines 304 - 334, Mark the C-interop raw structs as `@frozen` to guarantee
stable memory layout: update the declarations for GhosttyRenderColorRaw,
GhosttyRenderCellRaw, and GhosttyRenderSnapshotRaw by adding the `@frozen`
attribute before each struct keyword (i.e., change "struct
GhosttyRenderColorRaw", "struct GhosttyRenderCellRaw", and "struct
GhosttyRenderSnapshotRaw" to "@frozen struct ..."). This ensures the field
ordering and memory layout remain stable for dlsym/C interop.
| public init( | ||
| executableURL: URL, | ||
| arguments: [String], | ||
| environment: [String: String], | ||
| cols: Int, | ||
| rows: Int, | ||
| workingDirectory: URL? = nil | ||
| ) throws { | ||
| var master: Int32 = 0 | ||
| var slave: Int32 = 0 | ||
| var term = termios() | ||
| _ = tcgetattr(STDIN_FILENO, &term) | ||
| var size = winsize() | ||
| size.ws_col = UInt16(clamping: cols) | ||
| size.ws_row = UInt16(clamping: rows) | ||
|
|
||
| if openpty(&master, &slave, nil, &term, &size) != 0 { | ||
| throw PtyProcessError.openFailed(errno) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me check the file structure and context around this code
cd apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal || true
head -60 PtyProcess.swift | cat -nRepository: hack-dance/hack
Length of output: 2097
🏁 Script executed:
# Search for other uses of tcgetattr or openpty in the codebase
rg "tcgetattr|openpty" --type swift -B 2 -A 2Repository: hack-dance/hack
Length of output: 1854
🏁 Script executed:
# Check if there are any imports or utility functions related to PTY handling
rg "import.*Darwin|import.*Glibc|tcgetattr|openpty|termios" --type swiftRepository: hack-dance/hack
Length of output: 769
🏁 Script executed:
# Let me also search for any tests or documentation that might explain the intended behavior
find . -type f \( -name "*.swift" -o -name "*.md" \) -exec grep -l "tcgetattr\|openpty" {} \;Repository: hack-dance/hack
Length of output: 146
🌐 Web query:
openpty termios NULL parameter behavior when tcgetattr fails
💡 Result:
Short answer: If you pass term == NULL to openpty(), openpty() does not try to read or copy the slave's termios — it simply skips terminal-attribute setup. If you pass a non-NULL term pointer, openpty() will apply those settings with tcsetattr() (and will fail if the tcsetattr call fails). If you call tcgetattr() yourself and it fails, that failure is independent (tcgetattr returns -1 and sets errno, e.g. ENOTTY/EBADF); openpty() itself won't call tcgetattr when term == NULL. [1][2]
Sources:
- openpty(3) man page: termp NULL means terminal parameters are ignored. [1]
- glibc support_openpty implementation: only calls tcsetattr(inner, …, termp) when termp is non-NULL (no tcgetattr call when termp is NULL). [2]
🌐 Web query:
BSD openpty() passing nil termios vs zeroed termios behavior
💡 Result:
Short answer: different behavior. If you pass NULL to openpty(), the slave keeps the system/kernel default terminal attributes. If you pass a zeroed struct termios (all fields 0) and openpty() applies it, you are explicitly setting the slave's termios to those zero values — which will clear control/local flags and set baud to 0 (B0), i.e. "hang up" / drop modem control, disable receiver bits (no CREAD), etc. In practice that will make the slave behave very differently (often unusable) compared with leaving termios alone. [1][2]
Sources:
- openpty(3) — termp parameter: if non-NULL, its termios are applied to the slave. [1]
- termios / baud-rate docs — zero baud (B0) means “hang up”; c_cflag and c_lflag bits control receiver, parity, HUPCL, CLOCAL, etc. Clearing them has the effects described above. [2]
Handle tcgetattr failure before calling openpty.
When STDIN is not a TTY (common in GUI applications), tcgetattr fails but the result is ignored, leaving term as a zeroed termios struct. Passing a zeroed termios to openpty sets the slave's terminal attributes to all-zero values: baud rate becomes B0 (hang up), receiver is disabled, and control flags are cleared, rendering the PTY unusable. Pass nil to openpty instead when tcgetattr fails to preserve system defaults.
Proposed fix
- var term = termios()
- _ = tcgetattr(STDIN_FILENO, &term)
+ var term = termios()
+ let hasTerm = tcgetattr(STDIN_FILENO, &term) == 0
var size = winsize()
size.ws_col = UInt16(clamping: cols)
size.ws_row = UInt16(clamping: rows)
- if openpty(&master, &slave, nil, &term, &size) != 0 {
+ let openResult = hasTerm
+ ? openpty(&master, &slave, nil, &term, &size)
+ : openpty(&master, &slave, nil, nil, &size)
+ if openResult != 0 {
throw PtyProcessError.openFailed(errno)
}🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Services/GhosttyTerminal/Sources/GhosttyTerminal/PtyProcess.swift`
around lines 26 - 44, The init currently ignores tcgetattr failures and always
passes a zeroed termios to openpty; change it to check tcgetattr(STDIN_FILENO,
&term) and only pass &term to openpty if tcgetattr returns 0, otherwise pass nil
to preserve system defaults; update the openpty call in PtyProcess.init to use a
conditional term pointer (e.g., termPtr) so openpty(&master, &slave, nil,
termPtr, &size) is used when tcgetattr fails.
| const minVersionMatch = readFileSync(path.join(vendorDir, "build.zig.zon"), "utf8").match( | ||
| /minimum_zig_version\\s*=\\s*\"(\\d+\\.\\d+\\.\\d+)\"/ | ||
| ) |
There was a problem hiding this comment.
Regex metacharacters are double-escaped, preventing matches.
In JavaScript regex literals, \\s matches a literal backslash followed by 's', not whitespace. The same issue affects \\d and \\.. This regex will never match the expected minimum_zig_version = "X.Y.Z" pattern, causing the script to always fall back to the hardcoded default.
Proposed fix
const minVersionMatch = readFileSync(path.join(vendorDir, "build.zig.zon"), "utf8").match(
- /minimum_zig_version\\s*=\\s*\"(\\d+\\.\\d+\\.\\d+)\"/
+ /minimum_zig_version\s*=\s*"(\d+\.\d+\.\d+)"/
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const minVersionMatch = readFileSync(path.join(vendorDir, "build.zig.zon"), "utf8").match( | |
| /minimum_zig_version\\s*=\\s*\"(\\d+\\.\\d+\\.\\d+)\"/ | |
| ) | |
| const minVersionMatch = readFileSync(path.join(vendorDir, "build.zig.zon"), "utf8").match( | |
| /minimum_zig_version\s*=\s*"(\d+\.\d+\.\d+)"/ | |
| ) |
🤖 Prompt for AI Agents
In `@scripts/macos-ghostty-setup.ts` around lines 51 - 53, The regex in the
minVersionMatch extraction is double-escaped (e.g., "\\s", "\\d", "\\.") so it
never matches; update the RegExp passed to String.match in the readFileSync(...)
call (the expression assigning minVersionMatch) to use proper regex escapes
(single backslashes in a regex literal) so it matches minimum_zig_version =
"X.Y.Z" and captures the version into the existing capture group; ensure you
leave the surrounding call to readFileSync, path.join(vendorDir,
"build.zig.zon"), and the minVersionMatch variable intact.
| async function readPidFile(opts: { readonly path: string }): Promise<number | null> { | ||
| const file = Bun.file(opts.path) | ||
| if (!(await file.exists())) return null | ||
| const text = await file.text() | ||
| const value = Number.parseInt(text.trim(), 10) | ||
| return Number.isFinite(value) ? value : null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the readPidFile function and surrounding context
cat -n src/commands/usage.ts | sed -n '695,715p'Repository: hack-dance/hack
Length of output: 174
🏁 Script executed:
# Also search for where readPidFile is called to understand the impact
rg "readPidFile" src/commands/usage.ts -A 5 -B 2Repository: hack-dance/hack
Length of output: 619
🏁 Script executed:
# Check if there are other similar file operations in the codebase with error handling
rg "Bun\.file\(" src/ -A 3 -B 1 | head -50Repository: hack-dance/hack
Length of output: 3439
🏁 Script executed:
# Verify the exact implementation once more and check how it's used
cat -n src/commands/usage.ts | sed -n '700,710p'Repository: hack-dance/hack
Length of output: 521
🏁 Script executed:
# Check what happens after the readPidFile call to understand impact
rg "readPidFile" src/commands/usage.ts -A 2 -B 1Repository: hack-dance/hack
Length of output: 377
Guard against PID file read failures.
A permission/IO error on the PID file will currently throw and crash the command; return null instead. This is consistent with the function's return type Promise<number | null> and the calling code's expectation of null for unreadable files.
✅ Proposed fix
async function readPidFile(opts: { readonly path: string }): Promise<number | null> {
- const file = Bun.file(opts.path)
- if (!(await file.exists())) return null
- const text = await file.text()
- const value = Number.parseInt(text.trim(), 10)
- return Number.isFinite(value) ? value : null
+ try {
+ const file = Bun.file(opts.path)
+ if (!(await file.exists())) return null
+ const text = await file.text()
+ const value = Number.parseInt(text.trim(), 10)
+ return Number.isFinite(value) ? value : null
+ } catch {
+ return null
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function readPidFile(opts: { readonly path: string }): Promise<number | null> { | |
| const file = Bun.file(opts.path) | |
| if (!(await file.exists())) return null | |
| const text = await file.text() | |
| const value = Number.parseInt(text.trim(), 10) | |
| return Number.isFinite(value) ? value : null | |
| async function readPidFile(opts: { readonly path: string }): Promise<number | null> { | |
| try { | |
| const file = Bun.file(opts.path) | |
| if (!(await file.exists())) return null | |
| const text = await file.text() | |
| const value = Number.parseInt(text.trim(), 10) | |
| return Number.isFinite(value) ? value : null | |
| } catch { | |
| return null | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/commands/usage.ts` around lines 701 - 706, The readPidFile function
should guard against IO/permission errors and return null instead of allowing
exceptions to bubble; wrap the Bun.file(opts.path), await file.exists() and
await file.text() calls in a try/catch and on any caught error return null, then
keep the existing parsing logic (Number.parseInt(text.trim(), 10) and
Number.isFinite check) so readPidFile always resolves to a number or null even
when file access fails.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/build-macos-release.sh`:
- Around line 31-36: After sourcing the macOS app .env.local, validate that
APPLE_TEAM_ID, APPLE_ID and APPLE_ID_PASSWORD are set (non-empty) to avoid
unbound-variable failures under set -u; if any are missing, prompt the user (or
exit with a clear error) similar to apps/macos/scripts/build-release.sh. Apply
the same validation/prompt logic to the other credentials-loading block later in
the script (the repeated .env.local sourcing/credential usage section) so both
locations check the three environment variables before they are used.
♻️ Duplicate comments (1)
docs/cli.md (1)
742-744: Escape pipes inside table cells to fix column counts.These rows still include unescaped
|inside table cells, which markdownlint treats as extra columns. Escape them (e.g.,\|) or replace with|to keep the tables valid.🧩 Example fixes
-| `-c`, `--client <cursor|claude|codex|print>` | string | - | Open init prompt in an agent client (or print) | +| `-c`, `--client <cursor\|claude\|codex\|print>` | string | - | Open init prompt in an agent client (or print) | -| `--scope <user|project>` | string | `user` | Write MCP config to user or project scope | +| `--scope <user\|project>` | string | `user` | Write MCP config to user or project scope | -| `--scope <user|project>` | string | `user` | Print MCP config for user or project scope | +| `--scope <user\|project>` | string | `user` | Print MCP config for user or project scope | -| `--format <auto|docker-compose|plain>` | string | `auto` | How to parse incoming lines from stdin | +| `--format <auto\|docker-compose\|plain>` | string | `auto` | How to parse incoming lines from stdin | -| `--stream <stdout|stderr>` | string | `stdout` | Treat stdin as stdout or stderr | +| `--stream <stdout\|stderr>` | string | `stdout` | Treat stdin as stdout or stderr | -| `--variant <cut|mash|cycle|random>` | string | `cycle` | Animation variant | +| `--variant <cut\|mash\|cycle\|random>` | string | `cycle` | Animation variant |Also applies to: 765-766, 783-784, 885-886, 936-938
🧹 Nitpick comments (6)
scripts/prepare-release.ts (1)
50-64: Consider distinguishing file-not-found from other errors.The empty catch block silently swallows all errors, including permission issues or I/O failures on an existing file. For a release script, silently ignoring a failed write could lead to inconsistent versions.
💡 Suggested improvement
try { const xconfigContent = await Bun.file(xconfigPath).text() const updatedXconfig = xconfigContent.replace( /^MARKETING_VERSION = .*/m, `MARKETING_VERSION = ${nextVersion}` ) if (updatedXconfig !== xconfigContent) { await Bun.write(xconfigPath, updatedXconfig) process.stdout.write(`Updated Base.xcconfig: MARKETING_VERSION → ${nextVersion}\n`) } - } catch { - // macOS config may not exist, that's fine + } catch (err) { + // macOS config may not exist on non-macOS builds, that's fine + if (err instanceof Error && "code" in err && err.code !== "ENOENT") { + process.stderr.write(`Warning: Failed to update Base.xcconfig: ${err.message}\n`) + } }src/commands/daemon.ts (3)
261-346: Enhanced status reporting looks solid.The status handler now provides comprehensive diagnostic information including API health check, launchd status, and structured reporting. The JSON output is well-designed for scripting.
Minor style note: Lines 323 and 326 use
==for string comparison. Consider using===for consistency with TypeScript conventions.♻️ Optional: Use strict equality
- return report.status == "running" ? 0 : 1 + return report.status === "running" ? 0 : 1 } - if (report.status == "running") { + if (report.status === "running") {
417-437: Consider checking the stop result before starting.The return value of
handleDaemonStopis discarded on line 435. While the current implementation ofhandleDaemonStopalways returns 0 on success, if error handling were added later, this restart handler would silently ignore stop failures and proceed to start anyway.♻️ Suggested improvement
- await handleDaemonStop({ ctx, args: stopArgs }) - return await handleDaemonStart({ ctx, args: startArgs }) + const stopResult = await handleDaemonStop({ ctx, args: stopArgs }) + if (stopResult !== 0) { + logger.warn({ message: "Failed to stop hackd cleanly; attempting start anyway" }) + } + return await handleDaemonStart({ ctx, args: startArgs })
467-477: Inconsistent flag precedence between option pairs.The precedence order differs between the two option pairs:
runAtLoad: positive flag (--run-at-load) checked firstguiSessionOnly: negative flag (--no-gui-only) checked firstThis means if a user mistakenly passes both flags:
--run-at-load --no-run-at-load→runAtLoad = true--gui-only --no-gui-only→guiSessionOnly = falseConsider making the precedence consistent, or adding validation to reject conflicting flags.
♻️ Option A: Make precedence consistent (positive flag wins)
- const guiSessionOnly = args.options["no-gui-only"] === true - ? false - : args.options["gui-only"] === true - ? true - : controlPlane.config.daemon.launchd.guiSessionOnly + const guiSessionOnly = args.options["gui-only"] === true + ? true + : args.options["no-gui-only"] === true + ? false + : controlPlane.config.daemon.launchd.guiSessionOnly♻️ Option B: Reject conflicting flags
if (args.options["run-at-load"] && args.options["no-run-at-load"]) { logger.error({ message: "Cannot specify both --run-at-load and --no-run-at-load" }) return 1 } if (args.options["gui-only"] && args.options["no-gui-only"]) { logger.error({ message: "Cannot specify both --gui-only and --no-gui-only" }) return 1 }.github/workflows/release-macos-app.yml (2)
48-72: Quote shell variables to prevent word splitting.Static analysis flagged unquoted variable expansions. While
$RUNNER_TEMPis unlikely to contain spaces in GitHub Actions, quoting is defensive best practice.🔧 Proposed fix
# Import certificate - echo "$CERTIFICATE_P12" | base64 --decode > $RUNNER_TEMP/certificate.p12 - security import $RUNNER_TEMP/certificate.p12 \ + echo "$CERTIFICATE_P12" | base64 --decode > "$RUNNER_TEMP/certificate.p12" + security import "$RUNNER_TEMP/certificate.p12" \ -P "$CERTIFICATE_PASSWORD" \ -A \ -t cert \ -f pkcs12 \ -k "$KEYCHAIN_PATH"
87-112: Quote the output path in heredoc redirection.🔧 Proposed fix
- cat > $RUNNER_TEMP/ExportOptions.plist << EOF + cat > "$RUNNER_TEMP/ExportOptions.plist" << EOF
| # Load credentials from macOS app .env.local | ||
| if [ -f "$MACOS_APP_DIR/.env.local" ]; then | ||
| set -a | ||
| source "$MACOS_APP_DIR/.env.local" | ||
| set +a | ||
| fi |
There was a problem hiding this comment.
Missing validation for required Apple credentials.
The script sources .env.local but doesn't validate that required variables (APPLE_TEAM_ID, APPLE_ID, APPLE_ID_PASSWORD) are set before use. With set -u, this fails with an unclear "unbound variable" error. Unlike apps/macos/scripts/build-release.sh, this script doesn't prompt for missing credentials.
🔧 Proposed fix: Add validation after sourcing .env.local
# Load credentials from macOS app .env.local
if [ -f "$MACOS_APP_DIR/.env.local" ]; then
set -a
source "$MACOS_APP_DIR/.env.local"
set +a
fi
+
+# Validate required credentials
+[ -z "${APPLE_TEAM_ID:-}" ] && error "APPLE_TEAM_ID not set. Add it to $MACOS_APP_DIR/.env.local or export it."
+[ -z "${APPLE_ID:-}" ] && error "APPLE_ID not set (required for notarization)."
+[ -z "${APPLE_ID_PASSWORD:-}" ] && error "APPLE_ID_PASSWORD not set (required for notarization)."Also applies to: 105-120
🤖 Prompt for AI Agents
In `@scripts/build-macos-release.sh` around lines 31 - 36, After sourcing the
macOS app .env.local, validate that APPLE_TEAM_ID, APPLE_ID and
APPLE_ID_PASSWORD are set (non-empty) to avoid unbound-variable failures under
set -u; if any are missing, prompt the user (or exit with a clear error) similar
to apps/macos/scripts/build-release.sh. Apply the same validation/prompt logic
to the other credentials-loading block later in the script (the repeated
.env.local sourcing/credential usage section) so both locations check the three
environment variables before they are used.
roodboi
changed the title
## 1.2.0 (2026-01-21) * adding control plane baseline and initial extensions ([6be8d20](6be8d20)) * build and release ([9e04011](9e04011)) * deps ([82fe249](82fe249)) * docker network routing + docs and gateway updates ([1677d42](1677d42)) * docs and tests ([fd96b18](fd96b18)) * docs and tests ([29619ae](29619ae)) * fix ticket tracking and auto fixing ([8f69f71](8f69f71)) * ignore internal ([6e9da24](6e9da24)) * initial tui ([ffec865](ffec865)) * Merge pull request #2 from hack-dance/extension-system ([d39c0b5](d39c0b5)), closes [#2](#2) * notes ([fb49a67](fb49a67)) * resources and usage ([eafa98e](eafa98e)) * run ([9ac84cb](9ac84cb)) * tickets ([02b0d29](02b0d29)) * tickets and ci ([672cf6c](672cf6c)) * types ([01feefe](01feefe)) * types ([5503f0e](5503f0e)) * chore: gitignore build artifacts and auto-ignore .hack/.internal ([1b0b3fa](1b0b3fa)) * chore(macos): add helper scripts and docs ([8ed6a3f](8ed6a3f)) * feat: add hackd daemon with unix socket API, CLI commands, caching, and docs ([1d92ee2](1d92ee2)) * feat(daemon): add runtime health and reset detection ([80fcb4b](80fcb4b)) * feat(macos): add desktop app scaffold and mvp ([4b733e6](4b733e6)) * feat(macos): add hackd overview ([6c5ba01](6c5ba01)) * feat(tickets): sync tickets to hidden ref and repair legacy setup ([56c988b](56c988b)) * fix(macos): resolve hack cli path ([6e4b6ef](6e4b6ef)) * docs(macos): clarify project ownership ([a76d40f](a76d40f)) * ci: update macos runners ([33b0f48](33b0f48))
1 participant
Overview
This is a major release that introduces a modular extension system, remote access capabilities, a native macOS desktop app, and comprehensive documentation. The release transforms hack from a local development tool into a full-featured platform for managing containerized development environments locally and remotely.
Key Features
🧩 Extension System
A plugin architecture that allows extending hack CLI with new capabilities:
hack x <extension> <command>) - Run extension commandsgateway- Remote HTTP/WebSocket accesssupervisor- Background job managementtickets- Git-backed issue trackingcloudflare- Tunnel exposures via Cloudflaretailscale- Mesh network exposures🌐 Remote Gateway
Secure remote access to hack-managed projects:
📋 Supervisor
Background job and process management:
🎫 Tickets
Git-backed issue tracking that lives with your code:
🖥️ macOS Desktop App
Native SwiftUI application (
Hack Desktop):🚀 Release Tooling
Unified build and release process:
bun run macos:release- Build CLI + macOS app → signed/notarized DMGpackage.jsonand macOS app📚 Documentation
Comprehensive docs covering:
New Commands
Breaking Changes
None - this release is additive.
Testing
Summary by CodeRabbit
Release Notes
New Features
Documentation
CLI
usagecommand for resource monitoringgatewaycommand for remote access setupremotecommand for remote workflowsxcommand for extension dispatch✏️ Tip: You can customize this high-level summary in your review settings.