fix(scripts): normalize agent-batch help exits#3068
Conversation
Constraint: The direct agent-batch subcommands are documented executable CLIs, not only dispatcher internals. Rejected: Rely only on pnpm agent-batch --help | direct scripts still returned usage as an error. Confidence: high Scope-risk: narrow Directive: Keep helper CLI help paths side-effect-free and exit 0. Tested: node --test scripts/agent-batch/__tests__/cli.test.mjs scripts/agent-batch/__tests__/lib.test.mjs; node scripts/agent-batch/validate.mjs --help; node scripts/agent-batch/overlap.mjs --help; node scripts/agent-batch/status.mjs --help; node scripts/codex-pr-preflight.mjs --lightweight; pnpm typecheck; git diff --check Not-tested: Full format:check/pre-push hook; cargo is not installed in this environment, though the app Prettier phase passed.
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThree agent-batch CLI scripts (validate, overlap, status) add ChangesCLI Help Output and Usage Consistency
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 (1)
scripts/agent-batch/__tests__/cli.test.mjs (1)
128-139: ⚡ Quick winAdd
-hassertions alongside--helpin this matrix test.The scripts now advertise both forms, but this test only validates
--help. Including-hhere would catch the current short-flag regression immediately.Suggested test update
test("direct agent-batch subcommands print help with exit 0", () => { for (const [script, usage] of [ @@ ]) { - const r = run(script, ["--help"]); - assert.strictEqual(r.status, 0, r.stderr); - assert.match(r.stdout, usage); - assert.strictEqual(r.stderr, ""); + for (const flag of ["--help", "-h"]) { + const r = run(script, [flag]); + assert.strictEqual(r.status, 0, r.stderr); + assert.match(r.stdout, usage); + assert.strictEqual(r.stderr, ""); + } } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/agent-batch/__tests__/cli.test.mjs` around lines 128 - 139, The matrix test "direct agent-batch subcommands print help with exit 0" only checks the long form "--help"; update the test to also assert that the short "-h" produces the same behavior by invoking run(script, ["-h"]) in addition to run(script, ["--help"]) for each script (VALIDATE, OVERLAP, STATUS) and asserting status === 0, stdout matches the same usage regex, and stderr is empty; keep the existing assertions (assert.strictEqual and assert.match) for both invocations to catch short-flag regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/agent-batch/validate.mjs`:
- Around line 15-18: The -h check in validate.mjs is ineffective because
parseArgs only populates long (--foo) flags, so flags.h is never set; update
validate.mjs to detect short help flags by checking process.argv (e.g., look for
'-h' or '-?' entries) before treating arguments as a spec path, or alternatively
enhance parseArgs in scripts/agent-batch/lib.mjs to map single-letter short
flags into the flags object (so flags.h is populated); reference the
validate.mjs help handling block and the parseArgs function to make the change
so `validate.mjs -h` triggers the usage() path.
---
Nitpick comments:
In `@scripts/agent-batch/__tests__/cli.test.mjs`:
- Around line 128-139: The matrix test "direct agent-batch subcommands print
help with exit 0" only checks the long form "--help"; update the test to also
assert that the short "-h" produces the same behavior by invoking run(script,
["-h"]) in addition to run(script, ["--help"]) for each script (VALIDATE,
OVERLAP, STATUS) and asserting status === 0, stdout matches the same usage
regex, and stderr is empty; keep the existing assertions (assert.strictEqual and
assert.match) for both invocations to catch short-flag regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0182a2b3-f7f7-46d5-b8fb-21020fc7a4ab
📒 Files selected for processing (4)
scripts/agent-batch/__tests__/cli.test.mjsscripts/agent-batch/overlap.mjsscripts/agent-batch/status.mjsscripts/agent-batch/validate.mjs
Constraint: CodeRabbit identified that -h was treated as a positional spec path because parseArgs only handled long flags. Rejected: Patch validate.mjs only | overlap and status advertise the same short help contract and should share the parser fix. Confidence: high Scope-risk: narrow Directive: Keep shared CLI parsing behavior covered by matrix tests for every advertised flag form. Tested: node --test scripts/agent-batch/__tests__/cli.test.mjs scripts/agent-batch/__tests__/lib.test.mjs; node scripts/agent-batch/validate.mjs -h; node scripts/agent-batch/overlap.mjs -h; node scripts/agent-batch/status.mjs -h; git diff --check Not-tested: Full repository format/check; cargo is unavailable locally.
|
Thanks for the checks. I rechecked this PR on current state:
|
|
I reviewed all open PRs from Clarification on
|
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@alexzhu0 the code is clean and the approach is correct — consolidating the usage string into a helper and keeping the error vs help stdout/stderr split is the right call.
One thing the tests don't catch: -? gets parsed into flags['?'] in the updated parseArgs, but all three CLIs only check flags.help || flags.h, so node validate.mjs -? still exits 2 with the usage string on stderr. Either remove -? from the parseArgs branch or add flags['?'] to each CLI's help check alongside flags.h.
The only failing CI check is Rust Core Coverage, which is unrelated to this JS-only PR — worth checking if that's a pre-existing flake on main. Once that's resolved (or confirmed unrelated) and the -? inconsistency is sorted, i'll come back and approve.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@alexzhu0 solid PR! The consolidation of usage() helpers and the parseArgs fix in commit bf3de3c for handling -h are both clean.
sanil-23's feedback about the -? inconsistency is spot-on though: parseArgs now sets flags['?'] = true, but the CLI scripts only check flags.help || flags.h, so node validate.mjs -? still exits with code 2 instead of 0. Either remove -? from the parseArgs branch (if it's not intended as a user-facing help form) or add || flags['?'] to each CLI's help check alongside flags.h.
Once that's sorted and CI (Rust Core Coverage) passes, this is ready to go.
The parser already recognizes -?, so each direct agent-batch CLI should treat it like the other advertised help aliases instead of falling through to missing-spec handling. Constraint: Reviewer feedback identified node scripts/agent-batch/validate.mjs -? as an exit-2 path after parseArgs accepted the flag. Rejected: Remove -? from parseArgs | Keeping the accepted alias preserves the current parser contract and matches the requested help behavior. Confidence: high Scope-risk: narrow Directive: Keep direct CLI help aliases covered by the shared subcommand matrix test. Tested: node --test scripts/agent-batch/__tests__/cli.test.mjs; git diff --check Not-tested: Full repository format/check; unrelated Rust CI remains noisy upstream.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@alexzhu0 hey! the code looks good to me — the latest commit d9db615 honors -? properly across all three CLI scripts, test coverage now validates all three help flags (--help, -h, -?), and the prior feedback is resolved.
flag handling is clean:
- parseArgs now sets flags for
-hand-? - all three CLIs check
flags.help || flags.h || flags["?"]before treating args as spec path - tests validate all three flags exit 0 with usage printed to stdout
there are some CI jobs still pending (PR Submission Checklist, Frontend Quality, Build Playwright, Rust Quality). once those clear, i'll come back and approve this. let me know if you hit any blockers.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
@alexzhu0 the latest commit (d9db615) correctly addresses my prior concern: all three CLIs now check flags["?"] alongside flags.h and flags.help, and the test matrix iterates over --help, -h, and -? for all three scripts. that finding is resolved.
two things are blocking approval:
-
Rust Core Coverage has been failing since the first review — this is a pre-existing upstream issue and clearly unrelated to your JS-only change. not a blocker you need to fix.
-
E2E Playwright lane 1/4 is a new failure that wasn't present in the prior scan. lanes 2, 3, and 4 all passed, so it may be a flaky test, but please check the run log (https://github.com/tinyhumansai/openhuman/actions/runs/26766265637/job/78894469541) and rerun if it's unrelated. once CI is 100% green i'll approve — the code itself is in good shape.
4 participants
Summary
agent-batchsubcommand help paths exit successfully.--helphandling tovalidate.mjs,overlap.mjs, andstatus.mjs.Problem
pnpm agent-batch --helppath exits 0.node scripts/agent-batch/validate.mjs --helpprinted usage as an error and exited 2.Solution
usage()helpers to the direct subcommands.--help/-hflags before requiring a spec path.Submission Checklist
Impact
agent-batchhelper scripts now have consistent help semantics with the dispatcher.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/OH-2611-agent-batch-helpbf3de3cValidation Run
pnpm --filter openhuman-app format:check(blocked after app Prettier passed; see below)pnpm typechecknode --test scripts/agent-batch/__tests__/cli.test.mjs scripts/agent-batch/__tests__/lib.test.mjsnode scripts/agent-batch/validate.mjs --helpnode scripts/agent-batch/overlap.mjs --helpnode scripts/agent-batch/status.mjs --helpnode scripts/agent-batch/validate.mjs -hnode scripts/agent-batch/overlap.mjs -hnode scripts/agent-batch/status.mjs -hnode scripts/codex-pr-preflight.mjs --lightweightgit diff --checkValidation Blocked
command:env COREPACK_HOME=/Users/alex/PR/.corepack PNPM_HOME=/Users/alex/PR/.pnpm-home pnpm --filter openhuman-app format:checkerror:sh: cargo: command not foundimpact:The app Prettier phase passed, but Rust formatting and the pre-push hook cannot complete in this local environment until Cargo is installed.Behavior Changes
--help/-hpaths forvalidate,overlap, andstatusnow exit 0.Parity Contract
--helpand-hdispatch branches.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Improvements
Tests