Repository navigation
fix(cli): refuse malformed flags, symlinked roots, and IPv6 breakage - #39
Conversation
An audit of the CLI surfaced four defects that share a shape: a malformed or unusual input reaching a default instead of an error. `vref convert --dryrun` parsed cleanly and ran a real conversion, deleting the originals; `--out=` from an automation variable that expanded to nothing overwrote the published gallery. `getRequiredString` already existed for exactly this and was applied only to `--manifest`. Unknown flag names are now rejected per command, and `--out`, `--dir`, and `--host` must carry a value. `serve` resolved its root lexically and never walked it for symlinks, so a checked-out `.vref` symlink made it a file server for the link's target — `realPathInside` canonicalises both sides, so containment was measured against that target. `validateGallery` had the same gap on the manifest, masked until `screenshots` is empty. Both now match every other caller. `serve --host ::1` built the base `http://::1:4173`, which is unparseable; the throw was caught and answered 400 for every request while the server reported success. The request base is now fixed, since only the pathname is read, and the reported url brackets IPv6 literals. Behind all of it, the sandbox had no direct tests: 4 of 6 asset-path rejections were unreachable from the suite, and 43 error codes were covered by 3 assertions against English message text. Path safety is now tested per branch, assertions moved to codes, and `describe` is pinned to the flags the CLI implements — which immediately caught `--help` missing from all seven command schemas.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of small but concrete correctness/portability issues in the updated code/tests (notably URL formatting for already-bracketed IPv6 hosts and OS-specific path expectations in a new test) that should be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR tightens the vref CLI’s safety guarantees by failing fast on malformed inputs (unknown flags, missing flag values, symlinked roots) and fixes serve URL handling for IPv6, backed by an expanded test baseline and updated user/skill documentation.
Changes:
- Reject unknown flag names and reject “present but valueless” path flags instead of silently falling back to defaults.
- Refuse symlinked roots for
serveand symlinked manifests forvalidate, aligning behavior across commands. - Fix
serverequest URL parsing and reported URLs for IPv6 literals; expand tests to assertVREF_*codes and cover path-safety branches.
File summaries
| File | Description |
|---|---|
| test/vref.test.ts | Expands suite to cover new guards and assert stable VREF_* error codes; adds direct path-safety tests and describe/CLI drift checks. |
| src/serve.ts | Refuses symlinked serve roots; fixes IPv6 URL parsing and reports bracketed IPv6 base URLs. |
| src/describe.ts | Adds per-command --fields accepted values and includes --help in described schemas. |
| src/cli.ts | Introduces explicit allowed flag lists per command; uses required-string parsing for path flags; exports field/flag lists to keep tests and describe in sync. |
| src/build.ts | Ensures symlinked manifest paths are rejected even when screenshots array is empty. |
| skills/vref/references/manifest.md | Updates skill reference to reflect stricter flag handling and points users to vref describe for --fields values. |
| docs/VREF.md | Documents new “refuse malformed flags / symlink roots” behavior and clarifies safety rules. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f32b1d8e26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`getRequiredStringFrom` returned on the first alias it found, so `--out x --output-path=` passed the empty override straight through the guard it was added for. Every supplied alias is now validated before one is chosen. `build --check` returns a validate result and so accepts a narrower --fields set than `build`, but describe advertised only the wider one. Publishing the accepted values made that contradiction machine-readable, so describe now carries checkValues and a test holds it to what the check branch validates. `formatHost` would double-bracket an already-bracketed literal. listen() rejects one with ENOTFOUND before the url is built, so the guard is defensive and says so rather than carrying a test that cannot run. Path-safety expectations now build with join() instead of hard-coded POSIX strings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ed907ba0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
## [1.3.2](v1.3.1...v1.3.2) (2026-09-18) ### Bug Fixes * **cli:** refuse malformed flags, symlinked roots, and IPv6 breakage ([#39](#39)) ([5a73707](5a73707))
|
🎉 This PR is included in version 1.3.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Four defects from a CLI audit, plus the test baseline that should have caught them. They share a shape: a malformed or unusual input reaches a default instead of an error.
Behavior changes
Unknown flag names are rejected.
vref convert --dryrunparsed cleanly and ran a real conversion, deleting the originals.--keepsource,--forc,--dry_runall degraded the same way. Each command now has an allowed-flag list and raisesVREF_UNKNOWN_FLAG.Path flags must carry a value.
--out,--dir, and--hostusedgetString, which cannot tell an absent flag from an empty one — so--out=from an automation variable that expanded to nothing overwrote the published.vref/index.htmland exited 0.getRequiredStringalready existed for exactly this, with a docstring describing this failure, and was applied only to--manifest.serveandvalidaterefuse symlinked roots.serveresolved--dirlexically and never walked it, so a checked-out.vrefsymlink turned it into a file server for the link's target —realPathInsidecanonicalises both sides, so containment was measured against that target rather than the repo.validateGalleryhad the same gap on the manifest, masked by the per-asset loop untilscreenshotsis empty. Both now match whatbuild,convert, andmanifest addalready did.serve --host ::1works. The basehttp://::1:4173is unparseable; theTypeErrorwas caught by the request handler and answered400for every request, while the server reported success and printed an unopenable URL. The request base is now fixed — only the pathname is ever read — and the reported url brackets IPv6 literals.These make previously-silent invocations fail. That is the point, but it is a real change for any wrapper passing an extra flag.
Test baseline
test/vref.test.tsnever importedpath-safety.ts— the entire sandbox for a tool that writes files from agent-supplied JSON. Four of sixsafeManifestAssetPathrejections and bothresolveInsideCwdrejections were unreachable from the suite. It now tests each branch directly.43 distinct
VREF_*codes existed insrc/; 3 were asserted. The rest were pinned by English message substring, which is backwards —codeis what consumers branch on,messageis meant to be rewordable. 28 assertions moved to codes; 20 distinct codes are now covered.describeis pinned to the flagscli.tsimplements, in both directions. It immediately caught--helpmissing from all seven command schemas.--fieldsnow advertises its accepted values per command instead of a prosescopestring — additive, and it is the one list an agent needs to use the flag correctly.73 tests → 98. Every new guard was checked by reverting it and confirming the suite fails.
Not included
Findings 6–18 from the audit:
updatedAtnever refreshed,convert --only <typo>reporting success, unencoded asset paths inhref/src,Host/nosniffon the local server, Effect RC types in the published.d.mts, docs omitting.jpeg, and the CI OS matrix.