Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
uptask | eee050d | Mar 14 2026, 10:19 PM |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new terminal subsystem (config schema, loaders, module discovery, function parser, command/program builders, CLI entry, fixtures, and tests), re-exports select APIs, and removes legacy uptask modules and their specs; standardizes test filenames to Changes
Sequence DiagramsequenceDiagram
participant User
participant Main as terminal/main.ts
participant Loader as loadConfig(path)
participant Validator as defineConfig
participant Discovery as searchModules
participant Parser as parseFunctions
participant Builder as createProgram
participant Creator as createCommand
participant CLI as Commander
User->>Main: run CLI (args, --config)
Main->>Loader: loadConfig(configPath?)
Loader->>Validator: validate imported/default config
Validator-->>Loader: Config
Loader-->>Main: Config
Main->>Builder: createProgram(Config)
Builder->>Discovery: searchModules(pattern)
Discovery-->>Builder: Module[]
loop per module
Builder->>Parser: parseFunctions(module)
Parser-->>Builder: Function[]
loop per function
Builder->>Creator: createCommand(func)
Creator-->>Builder: Command
Builder->>CLI: register Command
end
end
Builder->>Config: invoke setupProgram hook (if provided)
Builder-->>Main: Program
Main->>CLI: program.parse()
CLI->>User: execute selected command (dynamic import -> invoke function)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
terminal/actions/module/search.ts (1)
28-78:⚠️ Potential issue | 🟠 MajorReplace hardcoded
/separators with cross-platform path utilities.Lines 28, 60, and 76 use hardcoded POSIX separators, which break on Windows. This prevents the module search from functioning correctly:
- Line 28:
split("/").pop()fails to extract the filename on Windows paths (no/to split). Passpath.basename(filePath)instead.- Line 60:
endsWith("/.gitignore")never matches Windows paths using\. Usepath.basename(filePath) === ".gitignore"instead.- Line 76:
startsWith(${rule.base}/)fails whenrule.basecontains backslashes frompath.dirname(). Add validation and normalize the path separator before passing torule.ig.ignores(), since the ignore library expects forward slashes matching gitignore conventions.Proposed cross-platform fix
const paths = new fdir() .withFullPaths() .exclude((dirName, dirPath) => { if (dirName === ".git") return true return isPathIgnored(path.join(dirPath, dirName), rules) }) .filter(filePath => { - if (!matcher(filePath.split("/").pop() ?? "")) return false + if (!matcher(path.basename(filePath))) return false return !isPathIgnored(filePath, rules) }) .crawl(".") .sync() const nestedPaths = new fdir() .withFullPaths() .exclude((dirName, dirPath) => { if (dirName === ".git") return true return isPathIgnored(path.join(dirPath, dirName), rules) }) - .filter(filePath => filePath.endsWith("/.gitignore")) + .filter(filePath => path.basename(filePath) === ".gitignore") .crawl(cwd) .sync() function isPathIgnored(absPath: string, rules: GitignoreRule[]): boolean { for (const rule of rules) { - if (!absPath.startsWith(`${rule.base}/`)) continue const relPath = path.relative(rule.base, absPath) + if (!relPath || relPath.startsWith("..")) continue - if (rule.ig.ignores(relPath)) return true + const relForIgnore = relPath.split(path.sep).join("/") + if (rule.ig.ignores(relForIgnore)) return true } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/actions/module/search.ts` around lines 28 - 78, Replace hardcoded POSIX separators with path utilities: in the search logic use path.basename(filePath) instead of filePath.split("/").pop(); in collectGitignoreRules change the nested finder filter from filePath.endsWith("/.gitignore") to path.basename(filePath) === ".gitignore"; and update isPathIgnored to avoid absPath.startsWith(`${rule.base}/`) — instead compute const rel = path.relative(rule.base, absPath) and if rel.startsWith('..') continue, then pass a normalized posix-style rel into rule.ig.ignores by converting backslashes to forward slashes (e.g. rel.replace(/\\/g, "/")) so the ignore library receives forward-slash paths.
🧹 Nitpick comments (3)
terminal/models/file.ts (1)
6-9: Consolidate duplicateModulemodel definitions.This duplicates the
Moduleschema/type already present interminal/models/module.ts. Keeping two sources for the same model is drift-prone.♻️ Proposed consolidation
-import { z } from "zod" - -/** - * A discovered TypeScript source module. - */ -export type Module = z.infer<typeof Module> -export const Module = z.object({ - path: z.string(), -}) +export type { Module } from "./module.ts" +export { Module } from "./module.ts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/models/file.ts` around lines 6 - 9, The file declares a duplicate Module schema/type (const Module and export type Module) that already exists elsewhere; remove the local z.object(...) definition and instead import and re-export the canonical Module schema and its inferred type from the existing module model, keeping the exported symbols named Module and Module (z.infer<typeof Module>) so callers remain unchanged; update any local references to use the imported Module to avoid model drift.terminal/actions/command/create.unit.ts (1)
57-73: Execution-path tests are missing behavioral assertions.These cases pass as long as
parseAsyncdoesn’t throw, so regressions in argument forwarding/order can slip through. Add assertions against observable effects (e.g., spy on fixture output or invoked function args).Also applies to: 110-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/actions/command/create.unit.ts` around lines 57 - 73, The tests in create.unit.ts currently only verify that createCommand(...).parseAsync(...) doesn't throw, so add behavioral assertions by spying or mocking the invoked function and asserting it was called with the expected args/order and options; locate where parseFunctions/findByName/createCommand are used to get the "deploy" function, replace or wrap the real implementation with a spy (or inject a stub handler) before calling cmd.parseAsync, then assert the spy received ["staging"] or ["production"] plus the "--dry-run" option (and any parsed flags) in the correct parameter positions; apply the same pattern to the second test and the analogous tests at lines 110-132.terminal/actions/command/create.ts (1)
31-34: Decomposed object option names are unscoped and collision-prone.Object properties are registered from
prop.namealone, so sibling object params with the same property name (e.g.,source.id,target.id) both map to the same CLI flag (--id). Consider prefixing decomposed option keys with their parent path to keep them unique.Also applies to: 84-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/actions/command/create.ts` around lines 31 - 34, Object properties are being registered using only prop.name, causing flag collisions for sibling objects; update both places where object params are decomposed (the registerOption loops) to pass a scoped name built from the parent param path and the property name (e.g., combine param.name and prop.name into a single key like "parent.child" or "parent-child") instead of prop.name alone, and ensure registerOption and whatever flag-name generator consume that scoped key so CLI flags remain unique (apply this change to the loop that calls registerOption for param.properties and the second similar block referenced).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/actions/command/create.ts`:
- Around line 55-56: The current return of options[param.name] ?? param.default
in the param resolution logic allows required option-backed params to be
undefined; update the resolution (the mapping that uses options[param.name],
param.default and param.required) to explicitly enforce param.required by
throwing a descriptive error when both options[param.name] and param.default are
undefined (or null) for a required param, otherwise return the provided option
or default as before; ensure you reference the param.required flag and include
the param.name in the thrown error message so callers know which required
parameter is missing.
In `@terminal/actions/config/load.ts`:
- Around line 10-16: Replace the dynamic import of the resolved path with a URL
import using pathToFileURL(resolved).href and broaden the error check in the
catch: capture the error code safely (e.g. const code = (error as any)?.code)
and treat both "ERR_MODULE_NOT_FOUND" and "MODULE_NOT_FOUND" as missing-module
cases before returning defineConfig({}); update the block around resolve,
import, defineConfig, and Config to use pathToFileURL and the safer error-code
extraction/comparison.
In `@terminal/actions/function/parse.ts`:
- Around line 84-97: The current mapping in extractObjectProperties only
serializes immediate properties and stops at depth 1; update the mapping to
recurse when a property's resolvedType indicates an object by calling
extractObjectProperties on that property's type (e.g., use
prop.getValueDeclarationOrThrow().getType().getProperties()) and include the
returned array under a properties key for that field, passing an updated prefix
like `${prefix}.${propName}` so paramTags resolve correctly; preserve existing
fields (name, type, required, default, description) for non-object types and
ensure required/default logic applies to nested fields too.
In `@terminal/actions/module/search.unit.ts`:
- Around line 45-77: The tests in search.unit.ts assert exact absolute paths
which can differ on macOS due to /private symlinks; update the assertions to be
symlink-safe by resolving real paths before comparing: for each assertion that
checks files (the files variable returned by searchModules), map file.path
through fs.realpathSync (or path.resolve/fs.realpathSync) and compare against
fs.realpathSync(join(tmpDir, "...")) for the expected entries so comparisons are
robust across platforms.
In `@terminal/main.ts`:
- Around line 4-10: The CLI reads -c/--config manually but leaves it in
process.argv so Commander errors; also command handlers are async so use
parseAsync. Remove the config flag and its value from process.argv after
computing configPath (use the existing configIndex variable and process.argv),
then call createProgram(config) and await program.parseAsync() instead of
program.parse() so async command handlers work correctly; refer to process.argv,
configIndex, loadConfig, createProgram, and program.parse -> program.parseAsync
for where to change.
---
Outside diff comments:
In `@terminal/actions/module/search.ts`:
- Around line 28-78: Replace hardcoded POSIX separators with path utilities: in
the search logic use path.basename(filePath) instead of
filePath.split("/").pop(); in collectGitignoreRules change the nested finder
filter from filePath.endsWith("/.gitignore") to path.basename(filePath) ===
".gitignore"; and update isPathIgnored to avoid
absPath.startsWith(`${rule.base}/`) — instead compute const rel =
path.relative(rule.base, absPath) and if rel.startsWith('..') continue, then
pass a normalized posix-style rel into rule.ig.ignores by converting backslashes
to forward slashes (e.g. rel.replace(/\\/g, "/")) so the ignore library receives
forward-slash paths.
---
Nitpick comments:
In `@terminal/actions/command/create.ts`:
- Around line 31-34: Object properties are being registered using only
prop.name, causing flag collisions for sibling objects; update both places where
object params are decomposed (the registerOption loops) to pass a scoped name
built from the parent param path and the property name (e.g., combine param.name
and prop.name into a single key like "parent.child" or "parent-child") instead
of prop.name alone, and ensure registerOption and whatever flag-name generator
consume that scoped key so CLI flags remain unique (apply this change to the
loop that calls registerOption for param.properties and the second similar block
referenced).
In `@terminal/actions/command/create.unit.ts`:
- Around line 57-73: The tests in create.unit.ts currently only verify that
createCommand(...).parseAsync(...) doesn't throw, so add behavioral assertions
by spying or mocking the invoked function and asserting it was called with the
expected args/order and options; locate where
parseFunctions/findByName/createCommand are used to get the "deploy" function,
replace or wrap the real implementation with a spy (or inject a stub handler)
before calling cmd.parseAsync, then assert the spy received ["staging"] or
["production"] plus the "--dry-run" option (and any parsed flags) in the correct
parameter positions; apply the same pattern to the second test and the analogous
tests at lines 110-132.
In `@terminal/models/file.ts`:
- Around line 6-9: The file declares a duplicate Module schema/type (const
Module and export type Module) that already exists elsewhere; remove the local
z.object(...) definition and instead import and re-export the canonical Module
schema and its inferred type from the existing module model, keeping the
exported symbols named Module and Module (z.infer<typeof Module>) so callers
remain unchanged; update any local references to use the imported Module to
avoid model drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c984323-003d-4409-8c54-2117c8813d74
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
AGENTS.mdpnpm-workspace.yamlterminal/README.mdterminal/actions/command/create.tsterminal/actions/command/create.unit.tsterminal/actions/config/define.tsterminal/actions/config/load.tsterminal/actions/function/parse.tsterminal/actions/function/parse.unit.tsterminal/actions/module/search.tsterminal/actions/module/search.unit.tsterminal/actions/program/create.tsterminal/entrypoints/run.cmdterminal/entrypoints/run.tsterminal/examples/@tasks.tsterminal/fixtures/arrays.tsterminal/fixtures/inline-object.tsterminal/fixtures/object-param.tsterminal/fixtures/sample.tsterminal/helpers/program.tsterminal/index.tsterminal/main.tsterminal/models/config.tsterminal/models/file.tsterminal/models/function.tsterminal/models/module.tsterminal/models/parameter.tsterminal/models/path.tsterminal/package.jsonterminal/tsconfig.jsontsconfig.jsonuptask.config.tsuptask/actions/command/create.spec.tsuptask/actions/command/create.tsuptask/actions/file/parse.spec.tsuptask/actions/function/parse.spec.tsuptask/actions/function/parse.tsuptask/actions/path/search.spec.tsuptask/index.tsuptask/main.tsuptask/models/file.tsuptask/program.ts
💤 Files with no reviewable changes (9)
- uptask/main.ts
- uptask/actions/function/parse.ts
- uptask/actions/path/search.spec.ts
- uptask/actions/command/create.spec.ts
- uptask/actions/function/parse.spec.ts
- uptask/actions/command/create.ts
- uptask/models/file.ts
- uptask/actions/file/parse.spec.ts
- uptask/program.ts
Comment on lines
+55
to
+56
| return options[param.name] ?? param.default | ||
| }) |
There was a problem hiding this comment.
Required non-positional params can pass through as undefined.
Line 55 returns option/default directly without enforcing param.required, so required option-backed params can silently become undefined at invocation time.
🔧 Proposed fix
- return options[param.name] ?? param.default
+ const value = options[param.name] ?? param.default
+ if (param.required && value === undefined) {
+ throw new Error(`Missing required option: ${param.name}`)
+ }
+ return value📝 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
| return options[param.name] ?? param.default | |
| }) | |
| const value = options[param.name] ?? param.default | |
| if (param.required && value === undefined) { | |
| throw new Error(`Missing required option: ${param.name}`) | |
| } | |
| return value | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terminal/actions/command/create.ts` around lines 55 - 56, The current return
of options[param.name] ?? param.default in the param resolution logic allows
required option-backed params to be undefined; update the resolution (the
mapping that uses options[param.name], param.default and param.required) to
explicitly enforce param.required by throwing a descriptive error when both
options[param.name] and param.default are undefined (or null) for a required
param, otherwise return the provided option or default as before; ensure you
reference the param.required flag and include the param.name in the thrown error
message so callers know which required parameter is missing.
Comment on lines
+10
to
+16
| const resolved = resolve(path ?? "uptask.config.ts") | ||
| try { | ||
| const module: Record<string, unknown> = await import(resolved) | ||
| return defineConfig(module.default as z.input<typeof Config>) | ||
| } catch (error) { | ||
| if (!path && error instanceof Error && "code" in error && error.code === "ERR_MODULE_NOT_FOUND") { | ||
| return defineConfig({}) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Node.js ESM dynamic import, is pathToFileURL(resolvedPath).href the recommended way to import an absolute filesystem path (including Windows)?
💡 Result:
Yes. In Node.js ESM, when you need to import() a module by an absolute filesystem path (especially on Windows, where C:\... is not a valid module specifier), the recommended approach is to convert the path to a file: URL via pathToFileURL(), and pass its serialized URL (commonly .href) to import().[1][2]
import { pathToFileURL } from 'node:url';
const specifier = pathToFileURL(resolvedPath).href;
const mod = await import(specifier);pathToFileURL() also ensures the path is made absolute and correctly URL-encodes special characters, which is part of why Node recommends it for importing paths.[1][2]
Sources: [1] [2]
Use pathToFileURL() for cross-platform compatibility and handle multiple error code variants.
Line 12 should import via pathToFileURL(resolved).href for Windows and other platforms. Line 15 should not rely solely on ERR_MODULE_NOT_FOUND; also check for MODULE_NOT_FOUND and use a safer error code extraction pattern.
🛠️ Proposed fix
import { resolve } from "node:path"
+import { pathToFileURL } from "node:url"
@@
-export async function loadConfig(path?: string) {
- const resolved = resolve(path ?? "uptask.config.ts")
+export async function loadConfig(configPath?: string) {
+ const resolved = resolve(configPath ?? "uptask.config.ts")
try {
- const module: Record<string, unknown> = await import(resolved)
+ const module: Record<string, unknown> = await import(
+ pathToFileURL(resolved).href,
+ )
return defineConfig(module.default as z.input<typeof Config>)
} catch (error) {
- if (!path && error instanceof Error && "code" in error && error.code === "ERR_MODULE_NOT_FOUND") {
+ const code =
+ typeof error === "object" && error && "code" in error
+ ? String((error as { code?: unknown }).code)
+ : ""
+ if (
+ !configPath &&
+ (code === "ERR_MODULE_NOT_FOUND" || code === "MODULE_NOT_FOUND")
+ ) {
return defineConfig({})
}
throw error
}
}📝 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
| const resolved = resolve(path ?? "uptask.config.ts") | |
| try { | |
| const module: Record<string, unknown> = await import(resolved) | |
| return defineConfig(module.default as z.input<typeof Config>) | |
| } catch (error) { | |
| if (!path && error instanceof Error && "code" in error && error.code === "ERR_MODULE_NOT_FOUND") { | |
| return defineConfig({}) | |
| import { resolve } from "node:path" | |
| import { pathToFileURL } from "node:url" | |
| export async function loadConfig(configPath?: string) { | |
| const resolved = resolve(configPath ?? "uptask.config.ts") | |
| try { | |
| const module: Record<string, unknown> = await import( | |
| pathToFileURL(resolved).href, | |
| ) | |
| return defineConfig(module.default as z.input<typeof Config>) | |
| } catch (error) { | |
| const code = | |
| typeof error === "object" && error && "code" in error | |
| ? String((error as { code?: unknown }).code) | |
| : "" | |
| if ( | |
| !configPath && | |
| (code === "ERR_MODULE_NOT_FOUND" || code === "MODULE_NOT_FOUND") | |
| ) { | |
| return defineConfig({}) | |
| } | |
| throw error | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terminal/actions/config/load.ts` around lines 10 - 16, Replace the dynamic
import of the resolved path with a URL import using pathToFileURL(resolved).href
and broaden the error check in the catch: capture the error code safely (e.g.
const code = (error as any)?.code) and treat both "ERR_MODULE_NOT_FOUND" and
"MODULE_NOT_FOUND" as missing-module cases before returning defineConfig({});
update the block around resolve, import, defineConfig, and Config to use
pathToFileURL and the safer error-code extraction/comparison.
Comment on lines
+4
to
+10
| const configIndex = process.argv.findIndex(a => a === "-c" || a === "--config") | ||
| const configPath = | ||
| configIndex !== -1 ? process.argv[configIndex + 1] : undefined | ||
| const config = await loadConfig(configPath) | ||
|
|
||
| const program = createProgram(config) | ||
| program.parse() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check main argv handling and parse mode:"
rg -n --type=ts -C3 'findIndex\(a => a === "-c" \|\| a === "--config"\)|program\.parse\(|program\.parseAsync\(' terminal/main.ts
echo
echo "Check whether command creation uses async/await patterns:"
rg -n --type=ts -C3 'async|await import\(' terminal/actions/command/create.tsRepository: datisthq/uptask
Length of output: 1025
🏁 Script executed:
#!/bin/bash
# Find and examine the createProgram function
fd -t f "create.ts" terminal/actions/program --exec cat -n {} \;
echo "---"
echo "Search for any --config or -c option registration:"
rg -n --type=ts "\.option.*config|\.option.*-c" terminal/actions/programRepository: datisthq/uptask
Length of output: 1171
🏁 Script executed:
#!/bin/bash
# Check if there's error handling around program.parse() in main.ts
cat -n terminal/main.ts
echo "---"
echo "Check helpConfiguration and any strict/permissive mode settings:"
rg -n --type=ts -A5 "helpConfiguration" terminal/helpers/program.tsRepository: datisthq/uptask
Length of output: 703
🏁 Script executed:
#!/bin/bash
# Check for any tests or examples showing CLI usage with -c/--config
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "test-*" -o -name "*-test" \) | head -20
echo "---"
echo "Search for any Commander configuration that might handle unknown options:"
rg -n --type=ts "unknownOption|allowUnknownOption|strict" terminal/actions/program
echo "---"
echo "Check if there are any examples or integration tests:"
fd -t f -e ts . examples test --max-depth 2 2>/dev/null | head -10Repository: datisthq/uptask
Length of output: 194
🏁 Script executed:
#!/bin/bash
# Verify by checking how program.parse() and program.parseAsync() behave
# Search for any other parse calls in the codebase
rg -n --type=ts "\.parse\(|\.parseAsync\("
echo "---"
echo "Check if there are any command invocations or test patterns with -c or --config:"
rg -n "\-c\s|--config" --type=ts --type=json --type=mdRepository: datisthq/uptask
Length of output: 887
--config currently breaks CLI parsing and parse mode should be async-safe.
-c/--config is read manually but not removed from argv, so Commander still receives it as an unknown option and will error. Additionally, command handlers use async/await patterns, which requires parseAsync() instead of the synchronous parse() method.
💡 Proposed fix
import { loadConfig } from "./actions/config/load.ts"
import { createProgram } from "./actions/program/create.ts"
-const configIndex = process.argv.findIndex(a => a === "-c" || a === "--config")
-const configPath =
- configIndex !== -1 ? process.argv[configIndex + 1] : undefined
+const argv = process.argv.slice(2)
+const configIndex = argv.findIndex(a => a === "-c" || a === "--config")
+
+let configPath: string | undefined
+if (configIndex !== -1) {
+ const value = argv[configIndex + 1]
+ if (!value || value.startsWith("-")) {
+ throw new Error("Missing value for -c/--config")
+ }
+ configPath = value
+ argv.splice(configIndex, 2)
+}
+
const config = await loadConfig(configPath)
const program = createProgram(config)
-program.parse()
+await program.parseAsync([process.argv[0]!, process.argv[1]!, ...argv])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terminal/main.ts` around lines 4 - 10, The CLI reads -c/--config manually but
leaves it in process.argv so Commander errors; also command handlers are async
so use parseAsync. Remove the config flag and its value from process.argv after
computing configPath (use the existing configIndex variable and process.argv),
then call createProgram(config) and await program.parseAsync() instead of
program.parse() so async command handlers work correctly; refer to process.argv,
configIndex, loadConfig, createProgram, and program.parse -> program.parseAsync
for where to change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
terminal/actions/module/search.unit.ts (1)
45-45:⚠️ Potential issue | 🟠 MajorMake absolute-path assertions symlink-safe (currently breaking macOS CI).
At Line 45, Line 60, Line 77, Line 99, Line 109, Line 120, Line 137, and Line 162, assertions compare raw absolute paths. On macOS temp dirs can resolve as
/var/...or/private/var/..., so these deep-equals are brittle and currently failing.✅ Suggested hardening pattern
-import { join } from "node:path" +import { join, relative } from "node:path" +function normalizedRelativePaths(tmpDir: string, files: { path: string }[]) { + const root = fs.realpathSync(tmpDir) + return files.map(f => relative(root, fs.realpathSync(f.path))).sort() +} - expect(files).toEqual([{ path: join(tmpDir, "keep.ts") }]) + expect(normalizedRelativePaths(tmpDir, files)).toEqual(["keep.ts"]) - expect(files).toEqual([ - { path: join(tmpDir, "root.ts") }, - { path: join(subDir, "keep.ts") }, - ]) + expect(normalizedRelativePaths(tmpDir, files)).toEqual([ + "root.ts", + join("sub", "keep.ts"), + ])Apply the same normalization pattern to the other absolute-path assertions in this file.
Also applies to: 60-63, 77-77, 99-99, 109-109, 120-120, 137-140, 162-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/actions/module/search.unit.ts` at line 45, The absolute-path assertions are brittle on macOS due to /var vs /private/var differences; update each assertion that compares paths (e.g., the expect(files).toEqual([{ path: join(tmpDir, "keep.ts") }]) check and the other occurrences) to normalize/resolve both actual and expected paths before comparing — for example map files to files.map(f => ({ path: path.resolve(f.path) })) and compare against [{ path: path.resolve(join(tmpDir, "keep.ts")) }] (use path.resolve or path.normalize from the path module for all listed assertions).
🧹 Nitpick comments (4)
terminal/actions/program/create.unit.ts (1)
17-23: Consider verifying the hook argument type.The assertion
expect(hook.mock.calls[0]?.[0]).toBeDefined()only checks the argument exists. Per the context snippet fromterminal/actions/program/create.ts:25-27, the hook receives a Commanderprograminstance. Consider strengthening the assertion:expect(hook.mock.calls[0]?.[0]).toBeInstanceOf(Command)This would require importing
Commandfromcommander.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/actions/program/create.unit.ts` around lines 17 - 23, The test currently only checks the hook argument is defined; update the unit test that uses defineConfig and createProgram (the hook mock named hook) to assert the first call's first argument is an instance of Commander’s Command class (use toBeInstanceOf(Command)); import Command from 'commander' at the top of create.unit.ts and replace or augment the existing expect(hook.mock.calls[0]?.[0]).toBeDefined() with expect(hook.mock.calls[0]?.[0]).toBeInstanceOf(Command) to ensure the hook receives the Commander program instance.terminal/actions/function/parse.ts (2)
9-12: Consider reusing theProjectinstance across multiple calls.Creating a new
ts-morphProjectfor each module parse is potentially expensive. IfparseFunctionsis called repeatedly in a batch (e.g., when processing all discovered modules), consider accepting an optionalProjectparameter or lifting the project creation to the caller.This is a minor optimization concern for now; profile before prioritizing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/actions/function/parse.ts` around lines 9 - 12, The parseFunctions function creates a new ts-morph Project each call which is expensive; modify parseFunctions (and any callers) to accept an optional Project parameter (e.g., project?: Project) or move Project creation outside and pass it in, then use that Project to call addSourceFileAtPath and parse functions; update references to parseFunctions accordingly so callers can reuse a single Project instance across multiple module.parse calls.
114-122: Limited type syntax handling may misclassify generic arrays.
resolveParameterTypeonly matches literalstring[]andnumber[]patterns. TypeScriptArray<string>orArray<number>syntax would fall back to"object".If such types are expected in user code, consider extending the matching:
🔧 Proposed enhancement
function resolveParameterType(typeText: string): ParameterType { const text = typeText.replace(/ \| undefined$/, "") if (text === "string") return "string" if (text === "number") return "number" if (text === "boolean") return "boolean" - if (text === "string[]") return "string[]" - if (text === "number[]") return "number[]" + if (text === "string[]" || text === "Array<string>") return "string[]" + if (text === "number[]" || text === "Array<number>") return "number[]" return "object" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/actions/function/parse.ts` around lines 114 - 122, The resolveParameterType function only recognizes literal "string[]" and "number[]" and will misclassify generic array syntax like Array<string> or Array<number>; update resolveParameterType to detect generic array forms (e.g. Array<...>, ReadonlyArray<...>) by normalizing whitespace and using pattern checks for Array<\s*string\s*> and Array<\s*number\s*> (and optionally ReadonlyArray) in addition to the existing "string[]" / "number[]" checks so those types return "string[]" or "number[]" respectively; keep the existing undefined-stripping behavior and fall back to "object" for other types.terminal/actions/config/define.unit.ts (1)
18-22: Consider adding a negative test forsetupProgram.The test validates that a valid function is preserved, but doesn't verify that non-function values are rejected. Based on the Config schema in
terminal/models/config.ts(lines 14-17),setupProgramuses a custom validator that only accepts functions or undefined.🧪 Proposed additional test case
it("should reject non-function setupProgram", () => { expect(() => defineConfig({ setupProgram: "not a function" } as unknown as { setupProgram: () => void }), ).toThrow() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/actions/config/define.unit.ts` around lines 18 - 22, Add a negative unit test that ensures defineConfig rejects non-function setupProgram values: call defineConfig with setupProgram set to a non-function (e.g., a string) cast to the expected type and assert that it throws; this complements the existing positive test for setupProgram and verifies the custom validator in defineConfig handles invalid types correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terminal/actions/config/load.unit.ts`:
- Around line 31-36: The test imports .ts config files via loadConfig, but
production CLI lacks TypeScript runtime support; add a runtime (recommended:
install the "tsx" dependency) and ensure the CLI is executed with it (e.g.
change the CLI invocation/shebang so the binary runs under tsx or update
package.json bin to launch via tsx) so loadConfig can import .ts files at
runtime; alternatively, if you prefer not to add a runtime, update project docs
and loadConfig expectations to require .js configs or modify the build/CLI entry
to include Node's --experimental-strip-types flag. Ensure references to
loadConfig and the CLI shebang/entry point are updated accordingly.
---
Duplicate comments:
In `@terminal/actions/module/search.unit.ts`:
- Line 45: The absolute-path assertions are brittle on macOS due to /var vs
/private/var differences; update each assertion that compares paths (e.g., the
expect(files).toEqual([{ path: join(tmpDir, "keep.ts") }]) check and the other
occurrences) to normalize/resolve both actual and expected paths before
comparing — for example map files to files.map(f => ({ path:
path.resolve(f.path) })) and compare against [{ path: path.resolve(join(tmpDir,
"keep.ts")) }] (use path.resolve or path.normalize from the path module for all
listed assertions).
---
Nitpick comments:
In `@terminal/actions/config/define.unit.ts`:
- Around line 18-22: Add a negative unit test that ensures defineConfig rejects
non-function setupProgram values: call defineConfig with setupProgram set to a
non-function (e.g., a string) cast to the expected type and assert that it
throws; this complements the existing positive test for setupProgram and
verifies the custom validator in defineConfig handles invalid types correctly.
In `@terminal/actions/function/parse.ts`:
- Around line 9-12: The parseFunctions function creates a new ts-morph Project
each call which is expensive; modify parseFunctions (and any callers) to accept
an optional Project parameter (e.g., project?: Project) or move Project creation
outside and pass it in, then use that Project to call addSourceFileAtPath and
parse functions; update references to parseFunctions accordingly so callers can
reuse a single Project instance across multiple module.parse calls.
- Around line 114-122: The resolveParameterType function only recognizes literal
"string[]" and "number[]" and will misclassify generic array syntax like
Array<string> or Array<number>; update resolveParameterType to detect generic
array forms (e.g. Array<...>, ReadonlyArray<...>) by normalizing whitespace and
using pattern checks for Array<\s*string\s*> and Array<\s*number\s*> (and
optionally ReadonlyArray) in addition to the existing "string[]" / "number[]"
checks so those types return "string[]" or "number[]" respectively; keep the
existing undefined-stripping behavior and fall back to "object" for other types.
In `@terminal/actions/program/create.unit.ts`:
- Around line 17-23: The test currently only checks the hook argument is
defined; update the unit test that uses defineConfig and createProgram (the hook
mock named hook) to assert the first call's first argument is an instance of
Commander’s Command class (use toBeInstanceOf(Command)); import Command from
'commander' at the top of create.unit.ts and replace or augment the existing
expect(hook.mock.calls[0]?.[0]).toBeDefined() with
expect(hook.mock.calls[0]?.[0]).toBeInstanceOf(Command) to ensure the hook
receives the Commander program instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13af4cbb-6f71-4e08-85ba-bd0baad75af5
📒 Files selected for processing (18)
terminal/actions/command/create.tsterminal/actions/command/create.unit.tsterminal/actions/config/define.unit.tsterminal/actions/config/load.unit.tsterminal/actions/function/parse.tsterminal/actions/function/parse.unit.tsterminal/actions/module/search.unit.tsterminal/actions/program/create.unit.tsterminal/fixtures/async-func.tsterminal/fixtures/mixed-params.tsterminal/fixtures/multiple-args.tsterminal/fixtures/nested-object.tsterminal/fixtures/no-exports.tsterminal/fixtures/no-jsdoc.tsterminal/fixtures/optional-array.tsterminal/fixtures/optional-params.tsterminal/fixtures/required-string-option.tsterminal/fixtures/string-defaults.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- terminal/actions/command/create.ts
- terminal/actions/command/create.unit.ts
terminal/actions/config/load.unit.ts
Outdated
Comment on lines
+31
to
+36
| it("should load config from explicit path", async () => { | ||
| const configPath = join(tmpDir, "uptask.config.ts") | ||
| fs.writeFileSync(configPath, 'export default { name: "my-app" }\n') | ||
| const config = await loadConfig(configPath) | ||
| expect(config.name).toBe("my-app") | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the terminal package is intended to be run and if TypeScript loader is configured
# Check for tsx/ts-node in dependencies
echo "=== Checking for TypeScript runtime support in package.json ==="
cat terminal/package.json | jq '.dependencies, .devDependencies' | grep -E '"tsx"|"ts-node"'
# Check for any bin scripts that might show how the CLI is invoked
echo -e "\n=== Checking bin configuration ==="
cat terminal/package.json | jq '.bin // "no bin configured"'
# Check main.ts for how it's expected to be executed
echo -e "\n=== Checking main.ts shebang or invocation hints ==="
head -5 terminal/main.ts 2>/dev/null || echo "main.ts not found"Repository: datisthq/uptask
Length of output: 465
🏁 Script executed:
# Check root package.json for TypeScript loaders and build setup
echo "=== Root package.json dependencies with tsx/ts-node ==="
cat package.json | jq '.dependencies, .devDependencies' | grep -E '"tsx"|"ts-node"'
echo -e "\n=== Root package.json scripts ==="
cat package.json | jq '.scripts'
# Check if there's a build output directory
echo -e "\n=== Checking for dist/build directories ==="
ls -la | grep -E "^d.*dist|^d.*build" || echo "No obvious dist/build dirs at root"
# Check the actual entrypoint to understand execution model
echo -e "\n=== Checking entrypoints/run.ts ==="
head -20 terminal/entrypoints/run.tsRepository: datisthq/uptask
Length of output: 1001
🏁 Script executed:
# Check tsconfig and package.json export configuration
echo "=== Checking tsconfig.json at root ==="
cat tsconfig.json | head -30
echo -e "\n=== Checking terminal/package.json type and exports ==="
cat terminal/package.json | jq '.type, .exports, .imports' | head -40
# Check if terminal/build directory exists (might be gitignored)
echo -e "\n=== Checking terminal directory structure ==="
ls -la terminal/ | head -20
# Look at the actual loadConfig implementation
echo -e "\n=== Checking loadConfig implementation ==="
head -50 terminal/actions/config/load.tsRepository: datisthq/uptask
Length of output: 2450
Test relies on dynamic TypeScript import but production TypeScript support is unclear.
The test dynamically imports a .ts file via await import(resolved), which works in Vitest due to Vite's import interception. However, when loadConfig is called in production to load user config files (e.g., uptask.config.ts), the underlying Node.js runtime would need TypeScript support.
While the tsconfig includes "erasableSyntaxOnly": true (supporting Node's --experimental-strip-types), the bin entry point's shebang is #!/usr/bin/env node without that flag, and no TypeScript loader (tsx, ts-node, etc.) is declared in dependencies. This creates a gap between what the test assumes and what production supports.
Consider either: (1) ensure the CLI is invoked with --experimental-strip-types, (2) add a TypeScript loader dependency, or (3) document that config files must be .js rather than .ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terminal/actions/config/load.unit.ts` around lines 31 - 36, The test imports
.ts config files via loadConfig, but production CLI lacks TypeScript runtime
support; add a runtime (recommended: install the "tsx" dependency) and ensure
the CLI is executed with it (e.g. change the CLI invocation/shebang so the
binary runs under tsx or update package.json bin to launch via tsx) so
loadConfig can import .ts files at runtime; alternatively, if you prefer not to
add a runtime, update project docs and loadConfig expectations to require .js
configs or modify the build/CLI entry to include Node's
--experimental-strip-types flag. Ensure references to loadConfig and the CLI
shebang/entry point are updated accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
35-35: Pintempyto an exact version for deterministic installs.Line 35 uses a range (
^3.2.0) while all other dependencies in this manifest are exact-pinned. Range operators can cause non-deterministic installs across time as upstream releases become available.Proposed change
- "tempy": "^3.2.0", + "tempy": "3.2.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 35, Update the dependency declaration for "tempy" in package.json to use an exact version instead of the caret range; replace the current "tempy": "^3.2.0" entry with an exact pinned version (e.g., "tempy": "3.2.0") so installs are deterministic and match the rest of the manifest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Line 35: Update the dependency declaration for "tempy" in package.json to use
an exact version instead of the caret range; replace the current "tempy":
"^3.2.0" entry with an exact pinned version (e.g., "tempy": "3.2.0") so installs
are deterministic and match the rest of the manifest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89362d51-19a3-4fdd-9734-4b0983a20acb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonterminal/actions/config/load.unit.tsterminal/actions/module/search.unit.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- terminal/actions/module/search.unit.ts
- terminal/actions/config/load.unit.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
35-35: Movetempyto theterminalworkspace manifest.
tempyis declared as a root devDependency but used only in terminal unit tests (terminal/actions/config/load.unit.tsandterminal/actions/module/search.unit.ts). Sinceterminal/package.jsonexists, move this dependency there to maintain tighter dependency boundaries within the monorepo structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 35, Move the devDependency "tempy" out of the root package.json and add it to the terminal workspace manifest: open root package.json and remove the "tempy": "3.2.0" entry, then open terminal/package.json and add "tempy": "3.2.0" under devDependencies so that tests referencing tempy in terminal/actions/config/load.unit.ts and terminal/actions/module/search.unit.ts resolve from the terminal package; run an install in the monorepo to update lockfiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Line 35: Move the devDependency "tempy" out of the root package.json and add
it to the terminal workspace manifest: open root package.json and remove the
"tempy": "3.2.0" entry, then open terminal/package.json and add "tempy": "3.2.0"
under devDependencies so that tests referencing tempy in
terminal/actions/config/load.unit.ts and terminal/actions/module/search.unit.ts
resolve from the terminal package; run an install in the monorepo to update
lockfiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db10fe60-c1f6-4fd1-af16-0bbe7034d9cd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
package.json
|
🎉 This PR is included in version 0.5.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores