Skip to content

fix(cli): reject invalid arguments in remaining commands - #307

Open
ooiuuii wants to merge 2 commits into
tt-a1i:mainfrom
ooiuuii:fix/strict-cli-arguments
Open

fix(cli): reject invalid arguments in remaining commands#307
ooiuuii wants to merge 2 commits into
tt-a1i:mainfrom
ooiuuii:fix/strict-cli-arguments

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Sep 4, 2026

Copy link
Copy Markdown

Problem and value

Closes #305.

The strict argument boundary added to render in #303 was missing from four sibling commands. Malformed invocations could report success; archify demo --typo could treat the mistyped option as a directory and write an HTML artifact there.

Scope

  • What changed:
    • reject undocumented long options in check, examples, doctor, and demo
    • enforce documented positional arity for check, examples, and doctor
    • add public-CLI regressions, including proof that demo --typo creates no output
    • rebuild the required archify.zip artifact with the repository builder and an official Node 22 distribution
  • What deliberately did not change:
    • valid CLI syntax or output; renderer, schema, layout, and checking behavior
    • build scripts, compression policy, runtime version requirements, or release identities
  • No unrelated changes: confirmed. The packaging follow-up changes only archify.zip; the PR remains three files, +52/-5 textual lines.

Stability impact

  • Compatibility and migration risk: only undocumented malformed invocations change to the existing usage-error contract (exit 2).
  • Renderer, validator, package, or generated-artifact risk: no renderer/validator changes. All 77 entries in the corrected archive have the same uncompressed contents, names, permissions, flags, and timestamps as the previous PR archive; only compression bytes and derived container offsets/sizes change.
  • Failure behavior and rollback path: invalid input fails before command work or filesystem side effects. Reverting the PR restores prior CLI behavior.

Tests run

Final artifact built and tested on macOS x64 with the official Node.js v22.23.1 distribution, bundled zlib 1.3.1-e00f703. The downloaded distribution was checked against its official SHA-256 manifest.

  • scripts/build-zip.sh <base-rebuilt.zip> at base d8e4daf, followed by cmp against its committed ZIP: exact byte match.
  • scripts/build-zip.sh <candidate.zip> at the unchanged PR source, repeated with TZ=Pacific/Honolulu: exact byte match between builds.
  • node --test test/cli.test.mjs: 46 passed, 0 failed, 0 skipped.
  • node --test test/release-package-gates.test.mjs: 18 passed, 0 failed, 1 expected skip, including the canonical archive byte-for-byte freshness test. The skipped case requires a non-Node-22 runtime.
  • Full npm test: 1,035 total: 1,007 passed, 1 failed, 27 skipped. The failure was an empty precheck snapshot cannot start a second concurrent network request in test/update-notifier.test.mjs:1951 (silent instead of update_available). An immediate isolated rerun passed 1/1, as did the same isolated test on base d8e4daf. The test and its imported notifier/contract implementation are unchanged from base and do not call the modified CLI. This is consistent with an existing timing-sensitive test, not a demonstrated CLI regression; the full-suite failure is disclosed, not counted as green. No notifier changes are included.
  • Extracted exact candidate, verified executable mode 755, and directly invoked archify/bin/archify.mjs --help without a node prefix: passed.
  • node scripts/package-smoke.mjs <extracted-archive>/archify: passed on the exact candidate.
  • Directly invoked all four extracted CLI commands with --typo: each exited 2, reported its unknown option, and left the working directory empty.
  • Independent Windows x64 Node.js v22.22.3, zlib 1.3.1-e00f703 check: all 77 compressed streams match recompression with the repository's exact settings. Old and corrected archives have identical uncompressed payloads and entry metadata.
  • Structured scope-limited review: clean, no actionable findings, against d8e4daf...489fa10; no further code changes were requested.
  • Remote CI at 489fa10: run 33977895246 reports action_required, pending maintainer approval to run the fork workflow. Local results are not reported as remote CI success.

Visual evidence

Not applicable: rejected CLI commands produce no visual output and renderer behavior is unchanged.

  • Evidence provided: exit codes, stderr checks, no-filesystem-side-effect assertions, archive hashes, entry comparisons, and Unix direct execution.
  • Comparison conditions: same final tracked source; prior Homebrew/system-zlib artifact compared with the official-Node artifact.
  • Automated or browser checks: public CLI tests, package gates, and exact-artifact smoke; no browser visual proof needed for this change.
  • Perceptual visual review: Not applicable.

Generated artifacts

  • Regenerated only archify.zip, using the unmodified official repository script.
  • Size: 1,322,007 bytes, 77 packaged files.
  • SHA-256: 9ec29e5a4e7f9cc44ae8e835ed0f1f38f9687f586b3c40925ea5078154995bff.
  • CLI entry mode: 100755 in the archive and 755 after Unix extraction.
  • Root-cause evidence: the previous Homebrew Node v22.22.0 dynamically used macOS system zlib 1.2.11. That runtime reproduces all 77 old compressed streams but does not reproduce the base archive. The official Node v22.23.1 distribution uses bundled zlib 1.3.1-e00f703, reproduces the base archive, and produces the corrected candidate. A Node-major-only check did not distinguish these environments.
  • The earlier local freshness pass applied only to the Homebrew/system-zlib environment; it was not evidence that the archive matched the project's rebuilding environment. No build-policy changes are included in this PR.

Checklist

  • I used a minimal focused change and preserved existing typed JSON behavior.
  • I ran the relevant targeted tests and npm test in archify/; exact outcomes are recorded above.
  • I added public-CLI regression coverage for behavioral changes.
  • I checked generated artifacts and package freshness after rebuilding.
  • I removed secrets, private repository content, and customer data from fixtures and evidence.

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the focused fix and the public-CLI regression coverage. The argument guards address #305 directly, and I did not find a blocker in the source changes. I ran the CLI suite at eeb40be on macOS with Node 26.3.0: 46 passed, 0 failed, 0 skipped.

There is one packaging blocker to fix before merging:

[P1] Restore the CLI executable bit in archify.zip. The committed archive records archify/bin/archify.mjs as mode 100644, while both the Git index and the base archive record 100755. After extraction with unzip, directly invoking the entry point fails with Permission denied; invoking it through node still succeeds, so a Node-prefixed smoke test does not detect this regression.

I rebuilt the exact PR head with Node 22.19.0 using scripts/build-zip.sh /tmp/archify-pr307-review.zip. The rebuilt archive has mode 100755, and cmp fails against the committed ZIP. Comparing all 77 entries found identical file contents; the differing entry metadata is the executable mode of archify/bin/archify.mjs. The Node 22 release-package suite independently confirms the issue: 17 passed, 1 failed, 1 skipped, with the failure in archive build is byte-for-byte reproducible across caller time zones without system zip.

Please regenerate and commit the canonical ZIP with the executable bit preserved, verify byte-for-byte freshness, and test direct execution of the extracted entry point on a Unix host. Then rerun the relevant checks and full suite and obtain green CI on the updated head. The current CI run is action_required, so there are no passing remote checks yet. Please update the PR validation evidence accordingly.

My full local suite was stopped after confirming the packaging blocker; I am not claiming a full-suite pass. This should be a small packaging follow-up; the CLI fix itself looks good.

@ooiuuii
ooiuuii force-pushed the fix/strict-cli-arguments branch from eeb40be to 0c6bb63 Compare September 5, 2026 05:33
@ooiuuii
ooiuuii force-pushed the fix/strict-cli-arguments branch from 0c6bb63 to 003bec2 Compare September 5, 2026 05:35
@ooiuuii

ooiuuii commented Sep 5, 2026

Copy link
Copy Markdown
Author

Fixed the packaging blocker and force-pushed the rebased head (003bec2).

Validation on macOS (rikunoMBP) with Node.js v22.22.0:

  • rebuilt archify.zip with the official scripts/build-zip.sh
  • committed archive SHA-256: c1209f6cd09c637ef90b0b8e52ca64e7ad909be19d8ddf93b1760dc725c6d137
  • archify/bin/archify.mjs is 100755 in the ZIP and 755 after extraction
  • direct extracted invocation succeeds without a node prefix
  • exact committed archive passes package-smoke.mjs on macOS
  • CLI suite: 46 passed, 0 failed, 0 skipped
  • release-package gates: 18 passed, 0 failed, 1 expected skip; archive reproducibility passed
  • full suite: 1,007 passed, 1 failed, 27 skipped (1,035 total). The sole failure was an unrelated timing-sensitive update-notifier test; its immediate isolated rerun passed 1/1.

The PR description now contains the updated evidence. GitHub has not started checks on the new head yet, so maintainer approval may still be needed for the workflow run. Please re-review when convenient.

@sunsunsun-java

Copy link
Copy Markdown
Collaborator

Thanks for the update. I reviewed 003bec2: the CLI changes address #305, all 46 CLI tests pass, and the executable permission issue is fixed.

One packaging blocker remains: the committed archify.zip still fails the byte-for-byte freshness check. Under Node 22.23.1:

  • Base d8e4daf: committed and rebuilt archives match exactly.
  • PR 003bec2: committed archive is 1,308,624 bytes; rebuilt archive is 1,322,007 bytes.
  • Package tests: 17 passed, 1 failed, 1 skipped. The failure is the canonical archive reproducibility check.

The extracted file contents and permissions are correct, and package smoke passes. The remaining difference is in compression bytes; the exact build-environment cause is not yet confirmed.

Please rebuild and commit the archive using the CI-matching Node 22 environment, verify byte-for-byte freshness, and rerun the relevant checks. Remote CI is still action_required, so passing CI on the updated head is also needed before merging.

ooiuuii commented Sep 5, 2026

Copy link
Copy Markdown
Author

Thanks for the independent reproduction — you were right that the earlier freshness pass was not sufficient.

Pushed 489fa10, changing only archify.zip. I traced the mismatch to the earlier Homebrew Node 22.22.0 dynamically using macOS system zlib 1.2.11. It reproduces the old PR ZIP but not the base ZIP. The official Node 22.23.1 distribution (bundled zlib 1.3.1-e00f703) reproduces the base byte-for-byte and produces the 1,322,007-byte candidate you observed.

Using that official distribution and the unchanged repository builder:

  • Base reproduction, candidate timezone comparison, executable mode/direct Unix execution, and exact-archive package smoke pass.
  • CLI: 46 passed. Package gates: 18 passed, 1 expected skip, including byte-for-byte freshness.
  • All 77 uncompressed entries and metadata are unchanged from the previous PR archive.
  • Full suite: 1,007 passed, 1 failed, 27 skipped. The failure was the unchanged notifier concurrency test an empty precheck snapshot cannot start a second concurrent network request; its isolated rerun passes on both head and base. This remains a disclosed full-suite failure, not a green result.

Final ZIP SHA-256: 9ec29e5a4e7f9cc44ae8e835ed0f1f38f9687f586b3c40925ea5078154995bff.

I corrected the PR body’s toolchain/proof claims. CI run 33977895246 is action_required with no jobs started; could a maintainer approve it? No build-policy or notifier changes were added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Remaining CLI commands accept invalid arguments and demo writes to a typo option path

3 participants