Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions src/commands/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3995,11 +3995,18 @@ function renderHackFolderReadme(opts: {
async function requireProjectContext(startDir: string) {
const ctx = await findProjectContext(startDir);
if (!ctx) {
throw new Error(
throw new MissingProjectContextError();
}
return ctx;
}

class MissingProjectContextError extends Error {
constructor() {
super(
`No ${HACK_PROJECT_DIR_PRIMARY}/ (or legacy .dev/) found. Run: hack init`
);
this.name = "MissingProjectContextError";
}
return ctx;
}

type RemoteLifecycleAction = "up" | "down" | "restart";
Expand Down Expand Up @@ -4116,11 +4123,20 @@ async function handleUp({
readonly ctx: CliContext;
readonly args: UpArgs;
}): Promise<number> {
const project = await resolveProjectForArgs({
ctx,
pathOpt: args.options.path,
projectOpt: args.options.project,
});
let project: Awaited<ReturnType<typeof requireProjectContext>>;
try {
project = await resolveProjectForArgs({
ctx,
pathOpt: args.options.path,
projectOpt: args.options.project,
});
} catch (error: unknown) {
if (error instanceof MissingProjectContextError) {
logger.error({ message: error.message });
return 1;
}
throw error;
}
const detach = args.options.detach;
const branch = resolveBranchSlug(args.options.branch);
const profiles = parseCsvList(args.options.profile);
Expand Down
100 changes: 100 additions & 0 deletions tests/project-up-command.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import { afterEach, beforeEach, expect, test } from "bun:test";
import { mkdtemp, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";

type CapturedRunResult = {
readonly exitCode: number;
readonly stdout: string;
readonly stderr: string;
};

let tempDir: string | null = null;
let originalSetupSyncMode: string | undefined;
let originalLogger: string | undefined;

beforeEach(async () => {
tempDir = await mkdtemp(join(tmpdir(), "hack-up-missing-project-"));
originalSetupSyncMode = process.env.HACK_SETUP_SYNC_MODE;
originalLogger = process.env.HACK_LOGGER;
process.env.HACK_SETUP_SYNC_MODE = "off";
process.env.HACK_LOGGER = "console";
});

afterEach(async () => {
if (tempDir) {
await rm(tempDir, { recursive: true, force: true });
tempDir = null;
}
if (originalSetupSyncMode !== undefined) {
process.env.HACK_SETUP_SYNC_MODE = originalSetupSyncMode;
} else {
process.env.HACK_SETUP_SYNC_MODE = undefined;
}
if (originalLogger !== undefined) {
process.env.HACK_LOGGER = originalLogger;
} else {
process.env.HACK_LOGGER = undefined;
}
Comment on lines +29 to +38

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Bug: Setting env var to undefined doesn't delete it.

In Node.js/Bun, process.env.VAR = undefined coerces undefined to the string "undefined" rather than removing the variable. Use delete to properly restore the original state.

🐛 Proposed fix
   if (originalSetupSyncMode !== undefined) {
     process.env.HACK_SETUP_SYNC_MODE = originalSetupSyncMode;
   } else {
-    process.env.HACK_SETUP_SYNC_MODE = undefined;
+    delete process.env.HACK_SETUP_SYNC_MODE;
   }
   if (originalLogger !== undefined) {
     process.env.HACK_LOGGER = originalLogger;
   } else {
-    process.env.HACK_LOGGER = undefined;
+    delete process.env.HACK_LOGGER;
   }
📝 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.

Suggested change
if (originalSetupSyncMode !== undefined) {
process.env.HACK_SETUP_SYNC_MODE = originalSetupSyncMode;
} else {
process.env.HACK_SETUP_SYNC_MODE = undefined;
}
if (originalLogger !== undefined) {
process.env.HACK_LOGGER = originalLogger;
} else {
process.env.HACK_LOGGER = undefined;
}
if (originalSetupSyncMode !== undefined) {
process.env.HACK_SETUP_SYNC_MODE = originalSetupSyncMode;
} else {
delete process.env.HACK_SETUP_SYNC_MODE;
}
if (originalLogger !== undefined) {
process.env.HACK_LOGGER = originalLogger;
} else {
delete process.env.HACK_LOGGER;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/project-up-command.test.ts` around lines 29 - 38, The cleanup block
incorrectly restores environment vars by assigning undefined (which becomes the
string "undefined"); change logic in the teardown to delete the env keys when
the originals were undefined: for the HACK_SETUP_SYNC_MODE handling in the test
(originalSetupSyncMode / process.env.HACK_SETUP_SYNC_MODE) and for HACK_LOGGER
handling (originalLogger / process.env.HACK_LOGGER) set the env variable to the
original value when defined, otherwise use the delete operator to remove the key
(delete process.env.HACK_SETUP_SYNC_MODE and delete process.env.HACK_LOGGER).

});

test("up without .hack prints a user message without stack trace", async () => {
if (!tempDir) {
throw new Error("Missing temp directory");
}

const result = await runCliWithCapturedOutput(["up", "--path", tempDir]);

expect(result.exitCode).toBe(1);

const combinedOutput = `${result.stdout}\n${result.stderr}`;
expect(combinedOutput).toContain(
"No .hack/ (or legacy .dev/) found. Run: hack init"
);
expect(combinedOutput).not.toContain("at requireProjectContext");
expect(combinedOutput).not.toContain("at async handleUp");
expect(combinedOutput).not.toContain("ERROR Error:");
});

test("up still reports unrelated usage errors", async () => {
const result = await runCliWithCapturedOutput([
"up",
"--definitely-not-a-real-flag",
]);

expect(result.exitCode).toBe(1);
const combinedOutput = `${result.stdout}\n${result.stderr}`;
expect(combinedOutput).toContain("Unknown option");
expect(combinedOutput).toContain("--definitely-not-a-real-flag");
expect(combinedOutput).toContain("Usage:");
});

async function runCliWithCapturedOutput(
args: readonly string[]
): Promise<CapturedRunResult> {
let stdout = "";
let stderr = "";
const originalStdoutWrite = process.stdout.write;
const originalStderrWrite = process.stderr.write;

process.stdout.write = ((chunk: string | Uint8Array) => {
stdout +=
typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8");
return true;
}) as typeof process.stdout.write;

process.stderr.write = ((chunk: string | Uint8Array) => {
stderr +=
typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8");
return true;
}) as typeof process.stderr.write;

try {
const { runCli } = await import("../src/cli/run.ts");
const exitCode = await runCli(args);
return { exitCode, stdout, stderr };
} finally {
process.stdout.write = originalStdoutWrite;
process.stderr.write = originalStderrWrite;
}
}
Loading