Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds AWS-backed node bootstrap and inspect flows across macOS UI, CLI, and services; implements a local dispatch mode with PR automation and terminal-state tracking; introduces workspace reset tooling, AWS E2E scripts/tests, new shared AWS data models, HackCLI client AWS methods, and widespread refactors and lint-suppression annotations. Changes
Sequence Diagram(s)sequenceDiagram
actor User as macOS App User
participant Dash as Dashboard UI
participant Model as DashboardModel
participant CLI as HackCLIClient
participant AWS as AWS SDK/CLI
participant SSM as SSM (via AWS)
participant EC2 as EC2 Instance
participant Node as remote hack node
User->>Dash: Click "Bootstrap AWS Node"
Dash->>Model: bootstrapAwsNode(request)
Model->>CLI: bootstrapAwsNode(request)
CLI->>AWS: Describe/Start EC2, check SSM
AWS-->>CLI: Instance/SSM status
CLI->>SSM: Send bootstrap command
SSM->>EC2: Execute bootstrap script
EC2->>Node: Run hack node init --json
Node-->>EC2: Enrollment bundle
EC2-->>SSM: Command output
SSM-->>CLI: Bootstrap output
CLI-->>Model: AwsBootstrapResponse
Model-->>Dash: update UI with node status
sequenceDiagram
actor User as CLI User
participant CLI as dispatch command
participant Local as Local Workspace
participant Git as Git
participant GitHub as GitHub API
participant Artifacts as Artifact Storage
User->>CLI: hack dispatch run --local --pr -- "<cmd>"
CLI->>Local: validate project & run command
Local-->>CLI: command output & artifacts
CLI->>Git: inspect branch/diff status
alt no changes
CLI->>CLI: terminalState = no_diff
else changes present
CLI->>GitHub: check existing PRs
alt PR exists
CLI->>GitHub: update PR
CLI->>CLI: terminalState = pr_created (or others)
else
CLI->>GitHub: create PR
CLI->>CLI: terminalState = pr_created
end
end
CLI->>Artifacts: write manifest including terminalState
Artifacts-->>User: return exit code and results
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical 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 (5)
services/auth-broker/src/modules/linear-connections/local-access.ts (1)
119-153:⚠️ Potential issue | 🟠 MajorWrap the refresh/rehydrate path in
try/catch.This branch can still reject from
refreshAccessToken(...)or the follow-upreadLocalAccess(...), which bypasses the declaredSeedLinearLocalAccessResultcontract and turns a recoverable sync failure into an uncaught exception.Proposed fix
if (shouldRefreshEnvelope({ envelope })) { const refreshToken = envelope.refreshToken?.trim() ?? "" if (!refreshToken) { return { ok: false, error: "linear_local_access_refresh_required", statusCode: 409, } } - const refreshedToken = await refreshAccessToken({ - tokenUrl: input.config.linearTokenUrl, - clientId: input.config.linearClientId ?? "", - ...(input.config.linearClientSecret - ? { clientSecret: input.config.linearClientSecret } - : {}), - refreshToken, - }) - if (!refreshedToken.ok) { - return { - ok: false, - error: refreshedToken.error, - statusCode: refreshedToken.statusCode, - } - } - const persisted = await persistLinearLocalAccessCustody({ - config: input.config, - connectionStore: input.connectionStore, - profileId: input.profileId, - token: refreshedToken.token, - tokenExpiresAt: refreshedToken.tokenExpiresAt, - refreshToken: refreshedToken.refreshToken ?? envelope.refreshToken, - refreshTokenExpiresAt: - refreshedToken.refreshTokenExpiresAt ?? envelope.refreshTokenExpiresAt, - }) - if (!persisted.ok) { - return persisted - } - envelope = - ( - await input.connectionStore.readLocalAccess({ - profileId: input.profileId, - encryptionKey, - }) - )?.envelope ?? envelope - refreshed = true + try { + const refreshedToken = await refreshAccessToken({ + tokenUrl: input.config.linearTokenUrl, + clientId: input.config.linearClientId ?? "", + ...(input.config.linearClientSecret + ? { clientSecret: input.config.linearClientSecret } + : {}), + refreshToken, + }) + if (!refreshedToken.ok) { + return { + ok: false, + error: refreshedToken.error, + statusCode: refreshedToken.statusCode, + } + } + const persisted = await persistLinearLocalAccessCustody({ + config: input.config, + connectionStore: input.connectionStore, + profileId: input.profileId, + token: refreshedToken.token, + tokenExpiresAt: refreshedToken.tokenExpiresAt, + refreshToken: refreshedToken.refreshToken ?? envelope.refreshToken, + refreshTokenExpiresAt: + refreshedToken.refreshTokenExpiresAt ?? envelope.refreshTokenExpiresAt, + }) + if (!persisted.ok) { + return persisted + } + envelope = + ( + await input.connectionStore.readLocalAccess({ + profileId: input.profileId, + encryptionKey, + }) + )?.envelope ?? envelope + refreshed = true + } catch (error) { + return { + ok: false, + error: error instanceof Error ? error.message : String(error), + statusCode: 500, + } + } }As per coding guidelines, "Handle errors appropriately in async code with try-catch blocks"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/auth-broker/src/modules/linear-connections/local-access.ts` around lines 119 - 153, The refresh-and-rehydrate sequence (calls to refreshAccessToken, persistLinearLocalAccessCustody and input.connectionStore.readLocalAccess) must be wrapped in a try/catch so any thrown errors are converted into the SeedLinearLocalAccessResult shape instead of escaping; wrap the block starting at refreshAccessToken(...) through the readLocalAccess(...) envelope assignment in a try, and in catch return a failure result (ok: false) with the caught error (and an appropriate statusCode) so callers still receive a SeedLinearLocalAccessResult rather than an uncaught exception.src/control-plane/extensions/supervisor/commands.ts (1)
1269-1283:⚠️ Potential issue | 🟡 MinorReject fractional offsets and dimensions instead of truncating them.
Math.trunc()turns--from 1.9into1and--rows 24.7into24. These flags are integer-only, so silently coercing them changes the user's request.Suggested fix
function parseOffset(value: string): number | null { const parsed = Number(value) - if (!Number.isFinite(parsed) || parsed < 0) { + if (!Number.isInteger(parsed) || parsed < 0) { return null } - return Math.trunc(parsed) + return parsed } function parsePositiveInt(value: string): number | null { const parsed = Number(value) - if (!Number.isFinite(parsed) || parsed <= 0) { + if (!Number.isInteger(parsed) || parsed <= 0) { return null } - return Math.trunc(parsed) + return parsed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/supervisor/commands.ts` around lines 1269 - 1283, The parsing helpers parseOffset and parsePositiveInt currently accept fractional inputs because they use Math.trunc; change their validation to reject non-integer values instead. In both functions (parseOffset and parsePositiveInt) keep the Number(value) and Number.isFinite checks, add a Number.isInteger(parsed) guard, and only accept values that also satisfy the existing non-negative (parsed >= 0) or positive (parsed > 0) conditions; return null for fractions or invalid values rather than truncating. Ensure the functions still return Math.trunc(parsed) (or parsed) only when parsing passes all checks so fractional inputs like "1.9" or "24.7" are rejected.src/control-plane/extensions/github/commands.ts (2)
1095-1098:⚠️ Potential issue | 🟠 MajorCarry the saved installation ID into selection.
The new discovery flow only passes
--installation-idintoselectGitHubInstallation(). If this profile already hasdefaults.installationIdand the token sees multiple installations,oauth-connectnow prompts or fails in CI instead of reusing the configured installation.💡 Proposed fix
const selectedInstallation = await selectGitHubInstallation({ - requestedInstallationId: parsed.value.installationId, + requestedInstallationId: + parsed.value.installationId ?? defaults.installationId, installations: identity.installations, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/github/commands.ts` around lines 1095 - 1098, The call to selectGitHubInstallation only uses parsed.value.installationId, so if the profile has a saved defaults.installationId it’s ignored; update the requestedInstallationId argument to prefer the CLI flag but fall back to the profile default (e.g. requestedInstallationId: parsed.value.installationId ?? identity.defaults?.installationId) when calling selectGitHubInstallation({ ..., installations: identity.installations }), preserving existing precedence (CLI flag > profile default).
1341-1394:⚠️ Potential issue | 🟠 MajorWrap fetch calls in
try/catchto handle network-level failures.In Bun/Fetch, DNS/TLS/timeout failures reject the promise instead of returning a non-OK
Response. The/userand/user/installationsfetch calls are currently unprotected and will propagate unhandled exceptions throughconnect,oauth-connect, andstatuscommands.Wrap the
/userlookup intry/catchand return an error result. Degrade/user/installationstransport failures intoinstallationWarningto maintain partial functionality when the installations endpoint is unreachable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/github/commands.ts` around lines 1341 - 1394, The /user and /user/installations fetch calls are unguarded against network-level rejections; wrap the first fetch (the call that assigns userRes and userPayload for `${base}/user`) in a try/catch and on catch return an error result (ok: false) with a descriptive error string including the caught error message so connect/oauth-connect/status callers get a proper failure instead of an exception; likewise wrap the installations fetch (the assignment to installationsRes/installationsPayload for `${base}/user/installations?per_page=100`) in a try/catch and on failure return the partial successful response (include login, accountId/accountName if present and installations: [] ) plus an installationWarning that includes the caught error message to preserve degraded functionality.src/control-plane/extensions/linear/commands.ts (1)
7373-7427:⚠️ Potential issue | 🟠 Major
connect --stdindrops the refresh metadata accepted by the direct token path.
resolveConnectToken()parses--token-expires-at,--refresh-token, and--refresh-token-expires-at, but the stdin branch ignores them becauseresolveConnectTokenFromStdin()has no inputs besides stdin. Piping a plain token now loses the extra fields that would be saved with--token.Suggested fix
-async function resolveConnectTokenFromStdin(): Promise< +async function resolveConnectTokenFromStdin(input: { + readonly expiresAt?: string; + readonly refreshToken?: string; + readonly refreshTokenExpiresAt?: string; +}): Promise< | { readonly ok: true; readonly token: string; readonly expiresAt?: string; readonly refreshToken?: string; @@ if (!envelope?.token) { return { ok: false, error: "Missing token from stdin." }; } return buildResolvedConnectToken({ token: envelope.token, - expiresAt: envelope.expiresAt, - refreshToken: envelope.refreshToken, - refreshTokenExpiresAt: envelope.refreshTokenExpiresAt, + expiresAt: envelope.expiresAt ?? input.expiresAt, + refreshToken: envelope.refreshToken ?? input.refreshToken, + refreshTokenExpiresAt: + envelope.refreshTokenExpiresAt ?? input.refreshTokenExpiresAt, }); } @@ if (input.stdin) { - return await resolveConnectTokenFromStdin(); + return await resolveConnectTokenFromStdin({ + expiresAt: input.expiresAt, + refreshToken: input.refreshToken, + refreshTokenExpiresAt: input.refreshTokenExpiresAt, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/linear/commands.ts` around lines 7373 - 7427, The stdin path drops token metadata because resolveConnectTokenFromStdin() only reads stdin; update resolveConnectTokenFromStdin to accept optional metadata parameters (expiresAt, refreshToken, refreshTokenExpiresAt) or pass those values into it from resolveConnectToken and merge them with values parsed from the envelope, then return buildResolvedConnectToken with the final metadata; locate resolveConnectToken and resolveConnectTokenFromStdin to add the parameters and merging logic so --token-expires-at / --refresh-token / --refresh-token-expires-at provided alongside --stdin are preserved.
🟠 Major comments (25)
src/commands/env.ts-427-446 (1)
427-446:⚠️ Potential issue | 🟠 MajorPersist the backend last to avoid leaving config in a broken state.
Line 427 commits
controlPlane.secrets.backendbefore the rest of the flow can still fail. For example,hack env backend use cloudwithout--providernow errors after the backend has already been switched to"cloud", and a failed encrypted-file key provisioning has the same partial-write problem. Validate/provision first, then flip the backend once the command is guaranteed to succeed.💡 Suggested reordering
- await updateGlobalConfig({ - path: "controlPlane.secrets.backend", - value: backend, - }) await persistSecretBackendConfig({ backend, providerRaw, storePath: args.options.storePath, keyPath: args.options.keyPath, secretProject: args.options.secretProject, secretPrefix: args.options.secretPrefix, }) - const controlPlane = await readControlPlaneConfig({}) - const secretsConfig = controlPlane.config.secrets + let controlPlane = await readControlPlaneConfig({}) + let secretsConfig = controlPlane.config.secrets const provisionedKey = await maybeProvisionEncryptedFileKey({ backend, shouldProvision: args.options.provisionKey === true, secretsConfig, }) + + await updateGlobalConfig({ + path: "controlPlane.secrets.backend", + value: backend, + }) + controlPlane = await readControlPlaneConfig({}) + secretsConfig = controlPlane.config.secrets🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/env.ts` around lines 427 - 446, The code currently calls updateGlobalConfig to set controlPlane.secrets.backend before running persistSecretBackendConfig and maybeProvisionEncryptedFileKey, which can leave the system in a partially-updated state on error; change the order so you first run persistSecretBackendConfig (using backend, providerRaw, storePath, keyPath, secretProject, secretPrefix) and then call maybeProvisionEncryptedFileKey (with backend, shouldProvision from args.options.provisionKey, and secretsConfig from readControlPlaneConfig), and only after those succeed call updateGlobalConfig to set controlPlane.secrets.backend; references: updateGlobalConfig, persistSecretBackendConfig, readControlPlaneConfig, maybeProvisionEncryptedFileKey.src/commands/session.ts-448-450 (1)
448-450:⚠️ Potential issue | 🟠 MajorUse
repoRootwhen runninghack upfor an existing session.This branch passes
input.project.projectDir, while the create path intentionally runs fromproject.repoRoot.hack session start --upwill therefore execute in different directories depending on whether the session already exists.Suggested fix
if (input.runUp) { - await runHackUp(input.project.projectDir) + await runHackUp(input.project.repoRoot) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/session.ts` around lines 448 - 450, The code calls runHackUp(input.project.projectDir) when input.runUp is true, causing inconsistent working directories vs the create path which uses project.repoRoot; update the branch that handles existing sessions to call runHackUp(input.project.repoRoot) (i.e., replace use of projectDir with repoRoot) so both code paths run the "hack up" command from the repository root; ensure you reference the same property name used by the create path (project.repoRoot) when invoking runHackUp.src/commands/session.ts-407-413 (1)
407-413:⚠️ Potential issue | 🟠 MajorMatch repository roots in
resolveProjectForSessionStart.The command advertises "project name or path", but this helper only matches the resolved path against
project.projectDir. A call likehack session start /path/to/repowill miss registered projects even though the rest of the start flow usesproject.repoRoot.Suggested fix
return ( input.projects.find( (project) => project.name === input.projectNameOrPath || - project.projectDir === resolvedPath + project.repoRoot === resolvedPath || + project.projectDir === resolvedPath ) ?? null )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/session.ts` around lines 407 - 413, The helper resolveProjectForSessionStart currently only checks project.name and project.projectDir against resolvedPath, so passing a repo path (input.projectNameOrPath) can miss matches; update the matching logic in resolveProjectForSessionStart to also compare resolvedPath against project.repoRoot (and/or normalize/resolve project.repoRoot) in the same find predicate (alongside project.projectDir and project.name) so repository-root paths correctly resolve to the registered project.src/commands/session.ts-458-463 (1)
458-463:⚠️ Potential issue | 🟠 MajorDon't force the first
--newsession to be<base>:2.When there are no existing sessions for a project, this still returns a numbered name. That leaves no canonical
<base>session and makes the exact-name lookups inhandleListandbuildSessionPickerOptionstreat the project as if it had no main session.Suggested fix
if (input.forceNew) { + const hasExistingSession = sessions.some( + (session) => + session.name === input.baseName || + session.name.startsWith(`${input.baseName}:`) + ) return { ok: true, - sessionName: `${input.baseName}:${getNextSessionNumber(sessions, input.baseName)}`, + sessionName: hasExistingSession + ? `${input.baseName}:${getNextSessionNumber(sessions, input.baseName)}` + : input.baseName, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/session.ts` around lines 458 - 463, The current force-new branch always returns a numbered name using getNextSessionNumber, causing the first new session to be "<base>:2" and preventing a canonical "<base>" session; change the logic in the forceNew handling so that if there are no existing sessions matching input.baseName (inspect the sessions array for exact baseName matches), return sessionName = input.baseName (no numeric suffix), otherwise return `${input.baseName}:${getNextSessionNumber(sessions, input.baseName)}` so subsequent new sessions get numbered; this preserves exact-name lookups in handleList and buildSessionPickerOptions.src/commands/global.ts-564-577 (1)
564-577:⚠️ Potential issue | 🟠 MajorDon't drop the mutagen remediation warning on install errors.
Lines 568-577 return before
warnOnMissingcan run. For themutagencall site, a real install failure now loses the"Remote sync may fail..."guidance andhack doctor --fixhint.🐛 Proposed fix
const systemPath = input.resolveSystemPath(); s.stop( systemPath ? `${input.label} available on PATH` : input.missingMessage ); if (result.reason === "failed") { input.warnOnFailure?.({ message: result.message }); - return; } if (!systemPath) { input.warnOnMissing?.({ reason: result.reason, message: result.message, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/global.ts` around lines 564 - 577, The current logic returns as soon as result.reason === "failed", which prevents input.warnOnMissing from running and loses the mutagen remediation warning; update the flow in the block containing systemPath, s.stop, result.reason, input.warnOnFailure, and input.warnOnMissing so that when result.reason === "failed" you still call input.warnOnMissing if !systemPath (passing reason and message) before returning — i.e., invoke input.warnOnMissing({ reason: result.reason, message: result.message }) when systemPath is falsy even on a failed install, then call input.warnOnFailure and return.src/control-plane/extensions/tickets/commands.ts-773-780 (1)
773-780:⚠️ Potential issue | 🟠 MajorMake
setup --checkfail when setup is incomplete.
getTicketsSetupExitCode()only treats"error"as failure, so missing skill/docs and repo drift still exit0. That makes the new check flow unreliable in automation because incomplete setup looks healthy. This helper needs the action and repo results so check mode can return non-zero for non-ready states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/tickets/commands.ts` around lines 773 - 780, getTicketsSetupExitCode currently only fails on "error" but must also fail when setup is incomplete; update the function signature (getTicketsSetupExitCode) to accept the action and repo results in addition to skill and docs (e.g., add opts.action and opts.repo) and change its logic to return non-zero whenever any of the following are not in a ready state: skill.status !== "ready", any docs entry where result.status !== "ready" (or missing), action.status !== "ready", or repo.status !== "ready"; use the existing types (checkTicketsSkill return type and TicketsSetupDocResults) to locate and wire up the new parameters and ensure callers are updated to pass action and repo results.src/control-plane/extensions/tickets/commands.ts-1688-1691 (1)
1688-1691:⚠️ Potential issue | 🟠 MajorPreserve explicit empty option values during finalization.
These truthy checks turn
--body=""/--body=intoundefined, sotickets updatecan no longer clear a body even though the update path explicitly supports empty bodies. The same pattern also lets--title ""skip the empty-title validation. Use!== undefinedhere.Suggested fix
return { - ...(state.title ? { title: state.title } : {}), - ...(state.body ? { body: state.body } : {}), - ...(state.bodyFile ? { bodyFile: state.bodyFile } : {}), + ...(state.title !== undefined ? { title: state.title } : {}), + ...(state.body !== undefined ? { body: state.body } : {}), + ...(state.bodyFile !== undefined ? { bodyFile: state.bodyFile } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/tickets/commands.ts` around lines 1688 - 1691, The current finalization builds the update payload using truthy checks which drop intentionally empty strings (e.g., --body="" or --title="") and prevents clearing fields; change the spread conditions to explicit undefined checks so empty strings are preserved: replace each conditional like ...(state.body ? { body: state.body } : {}) with ...(state.body !== undefined ? { body: state.body } : {}), and do the same for state.title and state.bodyFile in the payload construction used by the tickets update path.src/control-plane/extensions/tickets/commands.ts-792-795 (1)
792-795:⚠️ Potential issue | 🟠 MajorDefer enabling the extension until after an install action is confirmed.
This write happens before the args are even parsed, so
tickets setup --check,--remove, or any invalid invocation can still rewritehack.config.json.--checkshould stay side-effect free, and teardown flows should not re-enable the extension on their way through.Suggested fix
- await ensureTicketsExtensionEnabled({ - projectDir: project.projectDir, - logger: opts.ctx.logger, - }); - const parsed = parseTicketsSetupArgs({ args: opts.args }); if (!parsed.ok) { opts.ctx.logger.error({ message: parsed.error }); return 1; } const action = resolveTicketsSetupAction({ input: parsed.value }); + if (action === "install") { + await ensureTicketsExtensionEnabled({ + projectDir: project.projectDir, + logger: opts.ctx.logger, + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/tickets/commands.ts` around lines 792 - 795, The call to ensureTicketsExtensionEnabled is happening too early (before args are parsed) and can mutate hack.config.json for read-only commands; move the ensureTicketsExtensionEnabled(...) invocation out of the top-level command initialization in commands.ts and into the actual install/setup execution path (e.g., inside the handler that performs the 'tickets setup' install action). Only invoke ensureTicketsExtensionEnabled when the parsed command/action is a confirmed install (not when flags include --check, --remove, or on invalid invocations), and add an explicit guard that skips enabling when --check or --remove are present so teardown and dry-run flows remain side-effect free.src/control-plane/extensions/supervisor/commands.ts-689-694 (1)
689-694:⚠️ Potential issue | 🟠 MajorReject
--pathtogether with--project-id.
--pathis also a project selector, sohack x supervisor shell --project-id A --path ../Bcurrently parses and later combines B's local repo with A's remote project id.Suggested fix
- if (parsedState.state.project && parsedState.state.projectId) { + if ( + parsedState.state.projectId && + (parsedState.state.project || parsedState.state.path) + ) { return { ok: false, - error: "Use either --project or --project-id (not both).", + error: "Use either --project/--path or --project-id (not both).", } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/supervisor/commands.ts` around lines 689 - 694, The validation currently rejects using parsedState.state.project with parsedState.state.projectId but doesn't forbid parsedState.state.path with parsedState.state.projectId; add a parallel check after the existing one that if parsedState.state.path and parsedState.state.projectId are both set return { ok: false, error: "Use either --path or --project-id (not both)." } so the CLI prevents combining a local --path project selector with a remote --project-id. Use the same return shape and error style as the existing block that checks parsedState.state.project and parsedState.state.projectId.src/control-plane/extensions/supervisor/commands.ts-2011-2028 (1)
2011-2028:⚠️ Potential issue | 🟠 MajorMake explicit project selection override the ambient context.
projectId/projectNameare seeded fromctxfirst and then only backfilled with??, so--projector--pathcan be ignored whenever the handler already has actx.projectId. That opens the shell against the wrong project.Suggested fix
- let projectId: string | undefined = - input.parsed.projectId ?? input.ctx.projectId - let projectName: string | undefined = - input.parsed.project ?? input.ctx.projectName + let projectId: string | undefined = input.parsed.projectId + let projectName: string | undefined = input.parsed.project let localProject: ProjectContext | undefined = input.ctx.project if (input.parsed.project || input.parsed.path) { const localProjectResult = await resolveSupervisorProject({ ctx: input.ctx, projectOpt: input.parsed.project, pathOpt: input.parsed.path, }) if (!localProjectResult.ok) { return localProjectResult } localProject = localProjectResult.project - projectId = projectId ?? localProjectResult.projectId - projectName = localProjectResult.projectName ?? projectName + projectId = localProjectResult.projectId + projectName = localProjectResult.projectName ?? input.parsed.project + } else { + projectId = projectId ?? input.ctx.projectId + projectName = projectName ?? input.ctx.projectName }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/supervisor/commands.ts` around lines 2011 - 2028, The handler seeds projectId/projectName from input.ctx before checking parsed flags, so explicit flags (--project or --path) can be ignored; change the assignment logic in the block around resolveSupervisorProject so that when input.parsed.project or input.parsed.path is provided you override the ambient values: call resolveSupervisorProject (function resolveSupervisorProject) as you do, and then unconditionally set projectId = localProjectResult.projectId and projectName = localProjectResult.projectName (or fall back to parsed values if those are present) instead of using the nullish coalescing (??) that only backfills; also ensure localProject is assigned from localProjectResult.project to reflect the explicit selection.src/control-plane/extensions/github/auth.ts-924-933 (1)
924-933:⚠️ Potential issue | 🟠 MajorDon't silently remap a broken
defaultProfile.If
config.defaultProfilepoints at a missing profile, this selection code falls back to"default"/the first configured profile while still reporting the source as"global_default". That can route token resolution and PR automation through the wrong GitHub account instead of surfacing the misconfiguration.Also applies to: 943-962
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/github/auth.ts` around lines 924 - 933, The code silently remaps a configured defaultProfile that points to a missing profile; update the logic around resolveGitHubProfileStringSetting/defaultProfileFromConfig and resolveGitHubDefaultProfileId so that when configuredDefaultProfileId (from defaultProfileFromConfig) is non-null but not present in profilesById you do NOT fall back silently — instead detect the mismatch, log or surface an explicit configuration error (or return undefined/invalid source) and avoid using the first/“default” profile for token resolution; ensure resolveGitHubDefaultProfileId accepts and propagates an invalid configuredDefaultProfileId (and uses profilesById and sortedProfileIds to validate) so callers can fail fast rather than continuing with a remapped default.tests/dispatch-local-command.test.ts-38-62 (1)
38-62:⚠️ Potential issue | 🟠 MajorDelete env vars when the original value was absent.
process.envis string-backed; assigningundefinedstores the literal string"undefined". This leaks bogusHOME/HACK_*values into later tests running in the same worker.🛠️ Example fix
- if (originalHome === undefined) { - process.env.HOME = undefined; + if (originalHome === undefined) { + delete process.env.HOME;Apply the same pattern to all five env vars in this block:
HACK_GLOBAL_CONFIG_PATH,HACK_LOGGER,HACK_SETUP_SYNC_MODE, andHACK_GITHUB_APP_TOKEN.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/dispatch-local-command.test.ts` around lines 38 - 62, The teardown code in tests/dispatch-local-command.test.ts assigns undefined to process.env keys which becomes the string "undefined"; change each conditional to delete the environment variable when the original value was undefined instead of assigning undefined. Specifically, for HOME and the four HACK_* keys (HACK_GLOBAL_CONFIG_PATH, HACK_LOGGER, HACK_SETUP_SYNC_MODE, HACK_GITHUB_APP_TOKEN) replace assignments like process.env.HACK_LOGGER = undefined with delete process.env.HACK_LOGGER (or equivalent) while keeping restoration to the original value when present.src/commands/node.ts-4736-4758 (1)
4736-4758:⚠️ Potential issue | 🟠 MajorDon't let
SendCommandexceptions escape the result contract.
runAwsSsmShellCommand()returns anok/errorunion, butsendAwsSsmCommand()throws on AWS errors. A deniedssm:SendCommandor missing permission will bypass normal CLI error handling and abort the command instead of surfacing a clean message. Return an{ ok: false, error }result here or catch the exception insiderunAwsSsmShellCommand().Also applies to: 4787-4807
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/node.ts` around lines 4736 - 4758, runAwsSsmShellCommand currently calls sendAwsSsmCommand which can throw AWS exceptions and bypass the {ok:false,error} contract; wrap the sendAwsSsmCommand invocation in a try/catch inside runAwsSsmShellCommand, catch any thrown error, and return { ok: false, error: String(err) } (or a sanitized message) instead of letting the exception escape; also apply the same try/catch/return pattern to the other sendAwsSsmCommand call referenced around the 4787-4807 area so all SendCommand errors are converted into the function's result union (keep uses of readAwsSsmCommandInvocation unchanged).src/commands/node.ts-85-86 (1)
85-86:⚠️ Potential issue | 🟠 MajorAWS cold-start budgets are too small for stopped instances.
Each wait loop only gets
40 × 250ms(~10s). That is usually not enough for EC2 to reachrunningand for SSM to register afterward, so healthy boots will time out during cold starts. Give instance state, SSM readiness, and command execution separate minute-scale budgets instead of reusing this 10-second window everywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/node.ts` around lines 85 - 86, The current constants DEFAULT_AWS_BOOTSTRAP_RETRIES and DEFAULT_AWS_BOOTSTRAP_DELAY_MS create only ~10s total timeout and are reused everywhere; change the approach by introducing separate minute-scale budgets and reusing them in the relevant wait loops: add distinct constants (e.g., DEFAULT_AWS_INSTANCE_START_RETRIES, DEFAULT_AWS_INSTANCE_START_DELAY_MS for waiting for EC2 to reach "running"; DEFAULT_AWS_SSM_READY_RETRIES, DEFAULT_AWS_SSM_READY_DELAY_MS for waiting SSM registration; DEFAULT_AWS_COMMAND_EXEC_RETRIES, DEFAULT_AWS_COMMAND_EXEC_DELAY_MS for waiting command execution) and replace uses of DEFAULT_AWS_BOOTSTRAP_RETRIES/DELAY_MS in functions/methods that check instance state, SSM readiness, and command completion (locate the wait loops that call EC2 status checks, SSM DescribeInstanceInformation / GetCommandInvocation, and command polling) so each stage gets a minute-scale budget (e.g., ~60s total) rather than the current ~10s.apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLIClient.swift-1220-1228 (1)
1220-1228:⚠️ Potential issue | 🟠 MajorNormalize the AWS target selector before appending CLI flags.
Both methods append
--instance-idand the tag selector flags independently. If a request carries stale values from both modes, or only one half of the tag selector, the subprocess gets an ambiguous/invalid target set.💡 Suggested fix
- if let value = normalized(request.instanceId) { - args.append(contentsOf: ["--instance-id", value]) - } - if let value = normalized(request.instanceTagKey) { - args.append(contentsOf: ["--instance-tag-key", value]) - } - if let value = normalized(request.instanceTagValue) { - args.append(contentsOf: ["--instance-tag-value", value]) - } + let instanceId = normalized(request.instanceId) + let instanceTagKey = normalized(request.instanceTagKey) + let instanceTagValue = normalized(request.instanceTagValue) + + if let instanceId { + args.append(contentsOf: ["--instance-id", instanceId]) + } else if let instanceTagKey, let instanceTagValue { + args.append( + contentsOf: ["--instance-tag-key", instanceTagKey, "--instance-tag-value", instanceTagValue] + ) + } else if instanceTagKey != nil || instanceTagValue != nil { + throw HackCLIError.network("AWS instance tag selection requires both a tag key and value.") + }Apply the same normalization in both
bootstrapAwsNode(request:)andinspectAws(request:)so the two call paths stay aligned.Also applies to: 1263-1271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLIClient.swift` around lines 1220 - 1228, The AWS target selector flags are appended independently causing ambiguous/invalid targets when stale values exist; update both bootstrapAwsNode(request:) and inspectAws(request:) to normalize the selector first and only append the corresponding CLI flags from the normalized result (i.e., use the normalized(...) output for instanceId, instanceTagKey, and instanceTagValue consistently), skipping any flag whose normalized value is nil so you never mix instance-id with stale tag fields.src/control-plane/extensions/linear/auth.ts-204-227 (1)
204-227:⚠️ Potential issue | 🟠 MajorPreserve the broker management token when writing the refreshed envelope.
This rebuild only keeps the access/refresh token fields. After any successful refresh, the stored
managementTokenandmanagementTokenExpiresAtdisappear, so the next broker-only Linear operation falls back or fails even though the profile was already connected.💡 Suggested fix
return { token: input.refreshed.token, ...(input.refreshed.expiresAt ? { expiresAt: input.refreshed.expiresAt } : {}), ...(nextRefreshToken ? { refreshToken: nextRefreshToken } : {}), ...(nextRefreshTokenExpiresAt ? { refreshTokenExpiresAt: nextRefreshTokenExpiresAt } : {}), + ...(input.storedEnvelope.managementToken + ? { managementToken: input.storedEnvelope.managementToken } + : {}), + ...(input.storedEnvelope.managementTokenExpiresAt + ? { + managementTokenExpiresAt: + input.storedEnvelope.managementTokenExpiresAt, + } + : {}), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/linear/auth.ts` around lines 204 - 227, The buildRefreshedLinearTokenEnvelope function currently returns an envelope that omits the broker management token fields, causing stored managementToken and managementTokenExpiresAt to be dropped after refresh; update the returned LinearTokenEnvelope to include managementToken and managementTokenExpiresAt copied from input.storedEnvelope when present (e.g., add ...(input.storedEnvelope.managementToken ? { managementToken: input.storedEnvelope.managementToken } : {}) and similarly for managementTokenExpiresAt) so broker-only operations continue to work after a refresh.src/commands/workspace.ts-192-199 (1)
192-199:⚠️ Potential issue | 🟠 MajorOnly branch-switch when
--baseresolved to a real remote ref.
parseBaseRef()splits on the first/, so--base feature/foobecomesremote=feature, branch=foo. This block then runsgit checkout -B foo feature/foo, which can rename/reset the wrong local branch for any normal ref that happens to contain a slash. Gate the checkout on a confirmed remote match, not just on the parsed shape.Suggested fix
- if (parsedBase.remote && parsedBase.branch) { + if (fetchedRemote && parsedBase.branch) { await runGitWithLockRecovery({ projectRoot: input.projectRoot, args: ["checkout", "-B", parsedBase.branch, parsedBase.baseRef], lockPath, context: `git checkout -B ${parsedBase.branch} ${parsedBase.baseRef}`, @@ - checkoutBranch: parsedBase.branch, + checkoutBranch: fetchedRemote ? parsedBase.branch : null,Also applies to: 291-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/workspace.ts` around lines 192 - 199, The checkout runs whenever parsedBase looks like "remote/branch" but may be a normal ref with a slash; change the logic so you first verify the remote ref actually exists before calling runGitWithLockRecovery for the checkout. Use a git verification command (e.g., rev-parse or ls-remote) against refs/remotes/<remote>/<branch> or "<remote> <branch>" from the repo at input.projectRoot and only call runGitWithLockRecovery({ args: ["checkout","-B", parsedBase.branch, parsedBase.baseRef], lockPath, ... }) if that verification succeeds; apply the same guard to the second similar block that performs checkout (the block referenced at 291-301).apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift-2145-2149 (1)
2145-2149:⚠️ Potential issue | 🟠 Major
saveAwsDefaults()can leave AWS config partially updated.These writes ignore each
setGlobalConfigresult and keep going. If one call fails, earlier keys are already committed and later keys may differ, so the next AWS bootstrap can run against a mixed config. Please fail fast at minimum; ideally use a batched update path here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift` around lines 2145 - 2149, The loop over writes in saveAwsDefaults currently ignores each model.setGlobalConfig result, allowing partial commits; update the loop to check the await model.setGlobalConfig(key:value:) return (or thrown error) and fail fast on the first failure (propagate or log and return) so earlier keys aren't committed while later ones fail; if available, prefer using a single batched update API on the model (e.g., add or call a setGlobalConfigs/upsertMany method) to apply all keys atomically, and only call await loadAwsConfigFromDisk() after the update successfully completes.apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift-2056-2069 (1)
2056-2069:⚠️ Potential issue | 🟠 MajorRequire a tag key before treating tag-based bootstrap as ready.
Right now the readiness check only looks at
instanceTagValue. If a user clearsinstanceTagKey, the UI still reports “Bootstrap ready” and enables the action, but the request is sent withinstanceTagKey: nil, so the tag selector is incomplete.Proposed fix
+ private var hasValidTagSelector: Bool { + normalizedOrNil(instanceTagKey) != nil && normalizedOrNil(instanceTagValue) != nil + } + private var hasTargetSelector: Bool { - normalizedOrNil(instanceId) != nil || normalizedOrNil(instanceTagValue) != nil + normalizedOrNil(instanceId) != nil || hasValidTagSelector } private var hasExclusiveTargetSelector: Bool { - (normalizedOrNil(instanceId) != nil ? 1 : 0) + (normalizedOrNil(instanceTagValue) != nil ? 1 : 0) == 1 + (normalizedOrNil(instanceId) != nil ? 1 : 0) + (hasValidTagSelector ? 1 : 0) == 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swift` around lines 2056 - 2069, The readiness logic treats tag-based selection as valid when only instanceTagValue is present; update the computed properties so tag-based selectors require both instanceTagKey and instanceTagValue (i.e., use normalizedOrNil(instanceTagKey) alongside normalizedOrNil(instanceTagValue) in hasTargetSelector, hasExclusiveTargetSelector, and canBootstrapAwsNode) while preserving the existing instanceId checks; adjust hasExclusiveTargetSelector to count the tag pair as one selector only when both key and value are non-nil.src/control-plane/extensions/linear/commands.ts-1534-1557 (1)
1534-1557:⚠️ Potential issue | 🟠 MajorDon’t fall back to the default binding when an explicit
--project-idmisses.If the caller supplies a project ID that is not part of the current binding, this helper returns the default bound project instead of
null.sync-issue/sync-projectthen reuse the default team/profile while still honoring the user-suppliedprojectId, which can resolve the wrong team and build invalid Linear mutations.Suggested fix
function resolveSelectedProjectBindingTarget(input: { readonly binding: ReturnType<typeof resolveProjectLinearBinding>; readonly projectId?: string; }): LinearProjectBindingTarget | null { const explicitTarget = input.projectId ? findProjectBindingTarget({ binding: input.binding, projectId: input.projectId, }) : null; - if (explicitTarget) { - return explicitTarget; + if (input.projectId) { + return explicitTarget; } if (!input.binding.projectId) { return null; } return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/linear/commands.ts` around lines 1534 - 1557, resolveSelectedProjectBindingTarget currently falls back to the default binding when a caller supplies an explicit projectId that isn't found; update the function (which calls findProjectBindingTarget) to return null immediately when input.projectId is provided but findProjectBindingTarget returns null (i.e., add a check like "if (input.projectId && !explicitTarget) return null" before falling back to using input.binding values) so we don't reuse the default team/profile for a missing explicit projectId.src/control-plane/extensions/linear/commands.ts-1416-1433 (1)
1416-1433:⚠️ Potential issue | 🟠 MajorAvoid persisting
linear.profilebefore the bind is validated.This writes the profile override before checking whether
--project-idis present. A call likeproject-bind --profile fooreturns1but still mutates project config, leaving a half-applied binding behind.Suggested fix
async function bindLinearProject(input: { readonly ctx: LinearCommandContext; readonly projectDir: string; readonly parsed: ProjectBindArgs; readonly existingBinding: ReturnType<typeof resolveProjectLinearBinding>; }): Promise<number> { - const boundProfile = - input.parsed.profileId ?? input.existingBinding.profileId; - if (boundProfile) { - await updateProjectConfig({ - projectDir: input.projectDir, - path: "controlPlane.routing.overrides.linear.profile", - value: boundProfile, - }); - } - const projectId = input.parsed.projectId; if (!projectId) { input.ctx.logger.error({ message: "Missing --project-id. Use --clear to remove mapping or pass a Linear project id to bind.", }); return 1; } + + const boundProfile = + input.parsed.profileId ?? input.existingBinding.profileId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/linear/commands.ts` around lines 1416 - 1433, The code persists linear.profile via updateProjectConfig using boundProfile before validating the bind; instead, ensure you only call updateProjectConfig for "controlPlane.routing.overrides.linear.profile" after successful validation that a projectId exists (i.e., after checking input.parsed.projectId / projectId and returning non-error), so move the updateProjectConfig call (and any logic that derives boundProfile from input.parsed.profileId or input.existingBinding.profileId) to after the projectId check or gate it behind the successful validation path to avoid mutating project config when the command returns 1.src/control-plane/extensions/linear/commands.ts-3961-3980 (1)
3961-3980:⚠️ Potential issue | 🟠 MajorLinear-authoritative sync still leaks a local assignee update.
input.context.fieldsis spread first, so when the remote issue has no assignee, a locally resolvedassigneeIdsurvives and gets pushed back to Linear. That breaks the"linear"authority branch.Suggested fix
function resolveEffectiveTicketMutationFields(input: { readonly context: TicketToLinearSyncContext; readonly syncToggles: SyncToggles; }): LinearTicketMutationFields { if (!(input.context.authority === "linear" && input.context.existingIssue)) { return input.context.fields; } + const { assigneeId: _assigneeId, ...effectiveFields } = input.context.fields; + return { - ...input.context.fields, + ...effectiveFields, title: input.context.existingIssue.title, description: input.context.existingIssue.description ?? "", ...(input.context.existingIssue.assigneeId ? { assigneeId: input.context.existingIssue.assigneeId } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/control-plane/extensions/linear/commands.ts` around lines 3961 - 3980, In resolveEffectiveTicketMutationFields, the current spread of input.context.fields first lets a local assigneeId persist when the Linear-authoritative branch should remove it if the remote issue has no assignee; change the logic so that after spreading input.context.fields you explicitly set or remove assigneeId based on input.context.existingIssue.assigneeId (e.g., if existingIssue.assigneeId exists assign it, otherwise delete/omit the assigneeId key), ensuring the final returned object uses the remote assignee presence to override any local assignee; keep the existing handling for title, description and stateId in the same function.src/commands/dispatch.ts-1148-1148 (1)
1148-1148:⚠️ Potential issue | 🟠 MajorResolve the actual local branch once and validate any explicit override.
Right now local mode stores
input.branchverbatim, and PR automation trusts that same value before querying Git. A plain--localrun loses branch metadata, while a stale--branchcan target a different branch than the workspace that actually executed.Also applies to: 3234-3245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/dispatch.ts` at line 1148, The code currently stores input.branch verbatim (see the object spread "...(input.branch ? { branch: input.branch } : {})"); change this to resolve the workspace git branch once (e.g., call the existing git branch resolver utility or add a helper like resolveLocalBranch()/getCurrentBranch()) and use that resolved value when in local mode, and if an explicit input.branch is provided validate it against the resolved branch (either warn/error on mismatch or refuse the override) before setting the branch property; apply the same fix to the other occurrence referenced around lines 3234-3245 so both places use the resolved local branch and validate any explicit override.src/commands/dispatch.ts-690-699 (1)
690-699:⚠️ Potential issue | 🟠 MajorPersist the final remote
terminalStateafter PR automation.The run record is patched before
prOutcomeexists and never updated again, so--prruns stay stored as"completed"even when PR creation later fails or succeeds.dispatch statusand the canonical ticket artifact sync will then read the wrong terminal outcome.Also applies to: 720-758
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/dispatch.ts` around lines 690 - 699, The run record is patched before prOutcome is known so terminalState computed with prOutcome: null gets persisted; after PR automation completes compute finalTerminal = resolveDispatchTerminalState({status, prOutcome}) and re-patch/update the run record (the same updater used earlier) to set terminalState to finalTerminal; ensure you do this in the PR automation completion path where prOutcome is set so both initial and final terminalState (from resolveDispatchTerminalState with actual prOutcome) are stored (also apply same fix to the other similar block that spans the later resolveDispatchTerminalState usages).src/commands/dispatch.ts-1235-1251 (1)
1235-1251:⚠️ Potential issue | 🟠 MajorStream local command output instead of buffering it until exit.
This path doesn't append or print any logs until
exec()finishes. Long-running local runs show no progress,hack dispatch logs --followhas nothing to tail, and stdout/stderr ordering is lost when the two buffers are joined afterward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fc517b6-4b52-40db-ac0c-a5b9fd656d2b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/CommandPalette.swiftapps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/DashboardModel.swiftapps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/NodeTopologySettingsView.swiftapps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swiftapps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/SettingsOverlayView.swiftapps/macos/Packages/Services/HackCLI/Sources/HackCLIService/HackCLIClient.swiftapps/macos/Packages/Shared/Models/Sources/HackDesktopModels/Models.swiftapps/macos/Packages/Shared/Models/Tests/HackDesktopModelsTests/AwsBootstrapRequestTests.swiftdocs/README.mddocs/cli.mddocs/guides/remote-node-aws.mddocs/guides/remote-node-quickstart.mddocs/plans/2026-03-10-remote-modes-aws-runner-foundation.mdpackage.jsonscripts/aws-node-e2e.tsservices/auth-broker/src/flow-store.tsservices/auth-broker/src/modules/github-oauth/service.tsservices/auth-broker/src/modules/linear-agent/plugin.tsservices/auth-broker/src/modules/linear-connections/local-access.tsservices/auth-broker/src/modules/linear-oauth/service.tssrc/cli/spec.tssrc/commands/dispatch.tssrc/commands/env.tssrc/commands/global.tssrc/commands/node.tssrc/commands/project.tssrc/commands/session.tssrc/commands/workspace.tssrc/constants.tssrc/control-plane/extensions/github/auth.tssrc/control-plane/extensions/github/commands.tssrc/control-plane/extensions/linear/auth.tssrc/control-plane/extensions/linear/commands.tssrc/control-plane/extensions/supervisor/commands.tssrc/control-plane/extensions/tailscale/commands.tssrc/control-plane/extensions/tickets/commands.tssrc/control-plane/extensions/tickets/store.tssrc/control-plane/extensions/tickets/tickets-git-channel.tssrc/control-plane/sdk/gateway-client.tssrc/daemon/routes/node.tssrc/daemon/server.tssrc/lib/dispatch-runs.tssrc/lib/nodes-registry.tssrc/lib/project-views.tssrc/lib/remote-caddy-routes.tssrc/lib/runtime-projects.tssrc/lib/secret-store.tssrc/mcp/agent-docs.tssrc/tui/hack-tui.tssrc/tui/tickets-tui.tssrc/ui/log-group.tssrc/ui/loki-logs.tssrc/ui/planet.tstests/dispatch-local-command.test.tstests/node-provider-aws.test.tstests/node-runner-commands.test.tstests/workspace-reset-command.test.ts
| let command = [ | ||
| "hack dispatch run", | ||
| "--project \(shellQuote(dispatchProjectSelector))", | ||
| "--node \(shellQuote(node.id))", | ||
| "--target remote", | ||
| "--runner generic", | ||
| "--", | ||
| "sh -lc \(shellQuote("pwd && uname -a"))", | ||
| ].joined(separator: " ") | ||
| openTerminal(kind: .shell, command: command, title: "remote run") |
There was a problem hiding this comment.
Drop the unsupported --target remote flag from the dispatch command.
hack dispatch run in this PR's CLI reference doesn't expose a --target option, so this button currently opens a command that will fail with a usage error instead of dispatching anything. Selecting --node already makes the run explicit.
Suggested fix
let command = [
"hack dispatch run",
"--project \(shellQuote(dispatchProjectSelector))",
"--node \(shellQuote(node.id))",
- "--target remote",
"--runner generic",
"--",
"sh -lc \(shellQuote("pwd && uname -a"))",
].joined(separator: " ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/macos/Packages/Features/DashboardFeature/Sources/DashboardFeature/ProjectDetailView.swift`
around lines 3975 - 3984, The generated dispatch command incorrectly includes
the unsupported "--target remote" flag; update the command construction in
ProjectDetailView (the array building the local variable command that calls
openTerminal(kind: .shell, command: command, title: "remote run")) to remove the
"--target remote" element while keeping the other flags (including "--project
\(shellQuote(dispatchProjectSelector))" and "--node \(shellQuote(node.id))") and
the final shell invocation; ensure the title and call to openTerminal remain
unchanged.
| async function globalInstall(): Promise<number> { | ||
| const s = spinner(); | ||
| s.start("Ensuring gum…"); | ||
| const gum = await ensureBundledGumInstalled(); | ||
| if (gum.ok) { | ||
| s.stop(gum.installed ? "Installed bundled gum" : "gum already installed"); | ||
| } else { | ||
| const systemGum = Bun.which("gum"); | ||
| s.stop( | ||
| systemGum ? "gum available on PATH" : "gum not installed (optional)" | ||
| ); | ||
| if (gum.reason === "failed") { | ||
| await prepareGlobalInstallFiles(); | ||
| logger.success({ message: "Global files ready in ~/.hack/" }); | ||
| await globalUp(); | ||
| await ensureGlobalDnsReady(); | ||
| showGlobalInstallNextSteps(); | ||
| return 0; |
There was a problem hiding this comment.
Propagate globalUp() failures.
Line 485 discards the non-zero exit code from globalUp(), so hack global install can still run DNS/CA setup, print next steps, and return 0 after the stack failed to start.
🐛 Proposed fix
async function globalInstall(): Promise<number> {
await prepareGlobalInstallFiles();
logger.success({ message: "Global files ready in ~/.hack/" });
- await globalUp();
+ const upExit = await globalUp();
+ if (upExit !== 0) {
+ return upExit;
+ }
await ensureGlobalDnsReady();
showGlobalInstallNextSteps();
return 0;
}📝 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 globalInstall(): Promise<number> { | |
| const s = spinner(); | |
| s.start("Ensuring gum…"); | |
| const gum = await ensureBundledGumInstalled(); | |
| if (gum.ok) { | |
| s.stop(gum.installed ? "Installed bundled gum" : "gum already installed"); | |
| } else { | |
| const systemGum = Bun.which("gum"); | |
| s.stop( | |
| systemGum ? "gum available on PATH" : "gum not installed (optional)" | |
| ); | |
| if (gum.reason === "failed") { | |
| await prepareGlobalInstallFiles(); | |
| logger.success({ message: "Global files ready in ~/.hack/" }); | |
| await globalUp(); | |
| await ensureGlobalDnsReady(); | |
| showGlobalInstallNextSteps(); | |
| return 0; | |
| async function globalInstall(): Promise<number> { | |
| await prepareGlobalInstallFiles(); | |
| logger.success({ message: "Global files ready in ~/.hack/" }); | |
| const upExit = await globalUp(); | |
| if (upExit !== 0) { | |
| return upExit; | |
| } | |
| await ensureGlobalDnsReady(); | |
| showGlobalInstallNextSteps(); | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/global.ts` around lines 482 - 488, The current globalInstall()
ignores the exit code from globalUp(), allowing execution to continue on
failure; modify globalInstall() to capture the result of globalUp() (e.g., const
rc = await globalUp()), and if rc is non-zero return rc (or propagate the error)
immediately so ensureGlobalDnsReady() and showGlobalInstallNextSteps() are not
run on failure; update references inside globalInstall to use the returned code
from globalUp() and exit/return early when it indicates failure.
| const response = await input.ec2.send( | ||
| new DescribeInstancesCommand( | ||
| input.instanceId | ||
| ? { | ||
| InstanceIds: [input.instanceId], | ||
| } | ||
| : { | ||
| Filters: [ | ||
| { | ||
| Name: `tag:${input.instanceTagKey}`, | ||
| Values: [input.instanceTagValue], | ||
| }, | ||
| ], | ||
| } | ||
| ) | ||
| ); | ||
| const instance = extractAwsBootstrapInstance({ response }); | ||
| if (!instance) { |
There was a problem hiding this comment.
Reject non-unique tag matches before bootstrapping.
Tag-based lookup currently returns the first instance in the response. If multiple EC2 instances share the same tag, bootstrap and --bootstrap-command can run against the wrong host. Fail unless the selector resolves to exactly one instance, or require --instance-id once multiple matches exist.
Also applies to: 4575-4599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/node.ts` around lines 4542 - 4559, The tag-based EC2 lookup in
the DescribeInstancesCommand branch (using
input.instanceTagKey/instanceTagValue) must fail when the query returns != 1
instance instead of silently using the first match; modify the logic around the
DescribeInstancesCommand response handling (where extractAwsBootstrapInstance is
called) to count matching instances from the DescribeInstances response and, if
zero, report "no matching instance", and if >1, return an error asking the user
to supply --instance-id (or require unique tag) so bootstrapping is aborted;
make the same change for the similar tag-lookup block later (the other
DescribeInstancesCommand usage at lines ~4575-4599) so both places validate
exact-one-match before proceeding.
| async function removeGitIndexLock(input: { | ||
| readonly lockPath: string; | ||
| }): Promise<boolean> { | ||
| const existed = await pathExists(input.lockPath); | ||
| if (!existed) { | ||
| return false; | ||
| } | ||
| await rm(input.lockPath, { force: true }); | ||
| return true; | ||
| } | ||
|
|
||
| async function runGitWithLockRecovery(input: { | ||
| readonly projectRoot: string; | ||
| readonly args: readonly string[]; | ||
| readonly lockPath: string; | ||
| readonly context: string; | ||
| }) { | ||
| let result = await exec(["git", "-C", input.projectRoot, ...input.args], { | ||
| stdin: "ignore", | ||
| }); | ||
| if (result.exitCode === 0) { | ||
| return result; | ||
| } | ||
|
|
||
| const combined = `${result.stdout}\n${result.stderr}`; | ||
| if (hasGitIndexLockError(combined)) { | ||
| await removeGitIndexLock({ lockPath: input.lockPath }); | ||
| result = await exec(["git", "-C", input.projectRoot, ...input.args], { | ||
| stdin: "ignore", | ||
| }); | ||
| } | ||
|
|
||
| if (result.exitCode !== 0) { | ||
| const stderr = result.stderr.trim(); | ||
| const stdout = result.stdout.trim(); | ||
| const detail = stderr || stdout || "command failed"; | ||
| throw new Error(`${input.context} failed: ${detail}`); | ||
| } |
There was a problem hiding this comment.
Don't delete index.lock unless you've proved it's stale.
Both the eager path and the retry path remove .git/index.lock unconditionally. If another git process is actually active, this reset can race with it and corrupt the repo/index instead of recovering safely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/workspace.ts` around lines 322 - 359, The code currently
unconditionally deletes .git/index.lock in removeGitIndexLock and in
runGitWithLockRecovery retry path; change removeGitIndexLock to first prove the
lock is stale before removing it by: read the lock file (referencing
removeGitIndexLock and hasGitIndexLockError), parse any PID from the file, check
whether that PID is alive (e.g., process.kill(pid, 0)) and only remove the lock
if the process is not running or if the lock file is older than a safe threshold
(use fs.stat mtime fallback for systems without PID), return false (and do not
rm) when the lock is held by a live process, and ensure runGitWithLockRecovery
only retries removal when removeGitIndexLock returns true so active git
processes are not raced.
1 participant
Summary
Testing
Summary by CodeRabbit
New Features
Documentation
Tests