fix: don't show trace when running hack up in an uninitialized project - #14
Conversation
fix(project): suppress missing .hack stack trace for hack up
|
WalkthroughThe changes introduce a custom Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI (runCli)
participant Handler as handleUp
participant Resolve as resolveProjectForArgs
participant Require as requireProjectContext
participant Error as MissingProjectContextError
CLI->>Handler: invoke command
Handler->>Resolve: call resolveProjectForArgs
Resolve->>Require: call requireProjectContext
alt Project context missing
Require->>Error: throw MissingProjectContextError
Error-->>Handler: propagate error
Handler->>Handler: catch MissingProjectContextError
Handler->>Handler: log user-facing message
Handler-->>CLI: return exit code 1
else Project context found
Require-->>Resolve: return context
Resolve-->>Handler: return resolved project
Handler-->>CLI: continue execution
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
🧹 Nitpick comments (2)
tests/project-up-command.test.ts (1)
6-10: Consider usinginterfacefor object shapes.Per coding guidelines, prefer
interfaceovertypefor defining object shapes.♻️ Proposed change
-type CapturedRunResult = { +interface CapturedRunResult { readonly exitCode: number; readonly stdout: string; readonly stderr: string; -}; +}🤖 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 6 - 10, Replace the type alias with an interface: change the declaration "type CapturedRunResult = { readonly exitCode: number; readonly stdout: string; readonly stderr: string; }" to "interface CapturedRunResult { readonly exitCode: number; readonly stdout: string; readonly stderr: string; }" so the object shape uses interface semantics; keep the property names and readonly modifiers unchanged and update any imports/uses that reference CapturedRunResult if necessary.src/commands/project.ts (1)
4126-4139: Consider applying the same error handling to other command handlers.Only
handleUpcatchesMissingProjectContextErrorgracefully. The same stack trace issue would occur forhack down,hack restart,hack ps,hack run,hack logs, andhack openwhen run in an uninitialized project.If consistent UX is desired, the same try-catch pattern could be applied to the other handlers, or the error handling could be centralized in
resolveProjectForArgs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/project.ts` around lines 4126 - 4139, The project resolution code currently catches MissingProjectContextError in the handleUp flow but not elsewhere; update the other command handlers that call resolveProjectForArgs (e.g., handleDown, handleRestart, handlePs, handleRun, handleLogs, handleOpen) to wrap their resolveProjectForArgs call in the same try/catch that checks for MissingProjectContextError and logs via logger.error({ message: error.message }) and returns 1, or alternatively modify resolveProjectForArgs itself to return a well-known result type or throw a wrapped error that callers can uniformly handle; use the existing symbols resolveProjectForArgs, MissingProjectContextError, and logger.error to implement the consistent handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/project-up-command.test.ts`:
- Around line 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).
---
Nitpick comments:
In `@src/commands/project.ts`:
- Around line 4126-4139: The project resolution code currently catches
MissingProjectContextError in the handleUp flow but not elsewhere; update the
other command handlers that call resolveProjectForArgs (e.g., handleDown,
handleRestart, handlePs, handleRun, handleLogs, handleOpen) to wrap their
resolveProjectForArgs call in the same try/catch that checks for
MissingProjectContextError and logs via logger.error({ message: error.message })
and returns 1, or alternatively modify resolveProjectForArgs itself to return a
well-known result type or throw a wrapped error that callers can uniformly
handle; use the existing symbols resolveProjectForArgs,
MissingProjectContextError, and logger.error to implement the consistent
handling.
In `@tests/project-up-command.test.ts`:
- Around line 6-10: Replace the type alias with an interface: change the
declaration "type CapturedRunResult = { readonly exitCode: number; readonly
stdout: string; readonly stderr: string; }" to "interface CapturedRunResult {
readonly exitCode: number; readonly stdout: string; readonly stderr: string; }"
so the object shape uses interface semantics; keep the property names and
readonly modifiers unchanged and update any imports/uses that reference
CapturedRunResult if necessary.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/commands/project.tstests/project-up-command.test.ts
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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).
## <small>1.13.2 (2026-02-26)</small> * Merge pull request #1 from rhymiz/codex/fix-up-missing-hack-stack ([081f358](081f358)), closes [#1](#1) * Merge pull request #14 from rhymiz/main ([7e2d37f](7e2d37f)), closes [#14](#14) * fix(project): suppress missing .hack stack trace for hack up ([ced1f59](ced1f59))
2 participants
Summary by CodeRabbit
Bug Fixes
Tests