fix(scripts): validate i18n bundle dist flag#3071
Conversation
Constraint: The i18n bundle verifier accepts a path-valued --dist flag and should fail before filesystem checks when the path is missing. Rejected: Treat the next flag as a directory path | it turns argument mistakes into misleading missing-directory errors. Confidence: high Scope-risk: narrow Directive: Keep path-valued validation flags strict about missing values. Tested: node --test scripts/__tests__/verify-i18n-bundle-args.test.mjs; node scripts/verify-i18n-bundle.mjs --dist --help; node scripts/verify-i18n-bundle.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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR tightens CLI argument validation in ChangesCLI argument validation improvement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
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 |
|
Thanks for the checks. I rechecked this PR on current state:
|
|
I reviewed all open PRs from Clarification on
|
sanil-23
left a comment
There was a problem hiding this comment.
@alexzhu0 the code looks good — fix is correct and the tests cover both edge cases cleanly. CI is failing, so i'll hold the APPROVE until those are green. once CI passes i'll come back and approve. let me know if you need help sorting out the failures.
graycyrus
left a comment
There was a problem hiding this comment.
Looks good. The fix is surgical and well-tested — argument validation is now strict about missing --dist paths, which improves error diagnostics. CI is green and this is a low-risk helper script improvement.
Auto-corrected: CI is failing (Rust Core Coverage), cannot approve
graycyrus
left a comment
There was a problem hiding this comment.
Code looks good — the fix is surgical and well-tested. However, Rust Core Coverage (cargo-llvm-cov) is failing. Once CI passes, this will be ready to merge. (The argument validation change is low-risk and the tests are excellent.)
sanil-23
left a comment
There was a problem hiding this comment.
@alexzhu0 - re-checking this one. same commit since the last pass, so no new diff to review.
CI: "Rust Core Coverage (cargo-llvm-cov)" is still failing. This PR doesn't touch any Rust code, so the failure looks pre-existing or unrelated. If you haven't already, flag it to a maintainer so they can either confirm it's a known flake or triage it separately. All other checks (E2E, frontend coverage, Rust quality, i18n parity, etc.) are passing.
Code is still clean. My earlier comment about using result.stderr instead of result.stdout as the assertion message at lines 18 and 28 of the test file is still open - minor, not a blocker, but worth a quick fix before this merges so test failures give an actionable diagnostic.
Once Rust coverage CI resolves, this is good to go.
The verifier writes argument errors to stderr, so the tests should surface that stream when a status assertion fails. Constraint: Reviewer feedback called out stdout as the wrong assertion message for these error-path tests. Rejected: Leave the assertion message as stdout | it hides the diagnostic stream these tests are validating. Confidence: high Scope-risk: narrow Directive: Use the stream under test as the failure diagnostic in CLI argument tests. Tested: node --test scripts/__tests__/verify-i18n-bundle-args.test.mjs; git diff --check Not-tested: Full repository format/check; local cargo binary is unavailable.
sanil-23
left a comment
There was a problem hiding this comment.
@alexzhu0 the code looks good — prior comment addressed. CI isn't fully green yet so holding off on approve.
The one-line guard (!value || value.startsWith("-")) is exactly right: it covers both the trailing-flag case and the plain-missing case without disturbing valid paths or the top-level --help branch. The two tests are well-scoped — exit code, stderr message, and a negative assertion that the old misleading error no longer fires. Good structure.
Prior minor (assertion message should use stderr, not stdout): fixed in 889bbbc. Thread resolved.
CI failures currently hitting: Frontend Coverage (Vitest), Rust Core/Tauri Coverage, Coverage Gate, Rust E2E, Playwright E2E — none of these look related to a 1-line helper-script change, but they need to be green before I can approve. Once CI is green, i'll come back and approve. Let me know if any of the failures turn out to be pre-existing and there's a way to track them separately.
sanil-23
left a comment
There was a problem hiding this comment.
@alexzhu0 the code looks good and all prior feedback has been addressed. CI is still failing, but the three failing checks — Rust Core Coverage, Playwright lane 1/4, and the tauri-cef submodule pin — are all pre-existing and unrelated to this JS-only script change. Once CI is green i'll come back and approve. let me know if you need help tracking down what's blocking those checks upstream.
sanil-23
left a comment
There was a problem hiding this comment.
@alexzhu0 the upstream merge looks clean — no changes to the script or test files, and all prior feedback has been addressed.
Only remaining blocker is CI: Playwright E2E lanes 1 and 2 are failing, and Rust Core Coverage is failing. These appear to be pre-existing flaky failures unrelated to this JS-only change (lanes 3/4 pass, Rust E2E passes, Rust Tauri Coverage passes). A re-run should clear them. Once CI is fully green, i'll approve.
Summary
scripts/verify-i18n-bundle.mjs --distreject missing path values before filesystem checks.--help, as a missing--distvalue instead of a directory path.Problem
node scripts/verify-i18n-bundle.mjs --dist --helpcurrently resolves--helpas a directory.Solution
--distvalue to be present and not start with-.--helpbehavior when--helpis used as the actual top-level flag.node:testtests.Submission Checklist
Impact
--distusage directly.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/OH-2611-verify-i18n-dist-arg91eac1fValidation Run
pnpm --filter openhuman-app format:check(blocked after app Prettier passed; see below)pnpm typechecknode --test scripts/__tests__/verify-i18n-bundle-args.test.mjsnode scripts/verify-i18n-bundle.mjs --dist --helpnode scripts/verify-i18n-bundle.mjs --helpnode 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
--distnow exits 2 when its value is missing or another flag.Parity Contract
--dist <path>, top-level--help, and unknown-argument handling remain unchanged.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Tests
--distflag properly rejects invalid inputs and exits with appropriate error messages.Bug Fixes
--distflag to reject values that appear to be other CLI flags, in addition to missing values.