feat(validate): add -o, --output <file> for diagnostics output - #101
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9442ac0a67
ℹ️ 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".
Address Codex review of #101: - Refuse `-o/--output` when it resolves to the input file. Previously `validate spec.yaml -o spec.yaml` silently overwrote the API document with the diagnostics report (and could exit 0), destroying it. Checked up front, before the heavy apidom-ls import, so it fails fast. - Disable ANSI color when writing to a file. chalk keys color off process.stdout, so a stylish report written to a file from a TTY contained escape sequences. The formatter now takes an optional `color` flag; the action passes `color: false` for file output, so file reports are always plain text regardless of TTY/FORCE_COLOR. Adds tests for both (input preserved on rejection; no ESC bytes in the file even under FORCE_COLOR) and documents the behavior in the README.
Add a `-o, --output <file>` option to `speclynx validate` that writes the selected formatter's output to a file instead of stdout, mirroring the `overlay` command's `-o, --output`. The file is created/overwritten; write errors surface on stderr with a non-zero exit, and the validation exit code is otherwise unchanged. File content is byte-identical to stdout (formatter output plus a trailing newline). Also reconcile package-lock.json with the Babel 8 dependency set on this branch (fills in the missing @emnapi/core and @emnapi/runtime optional peer entries so `npm ci` is back in sync). Closes #94
Address Codex review of #101: - Refuse `-o/--output` when it resolves to the input file. Previously `validate spec.yaml -o spec.yaml` silently overwrote the API document with the diagnostics report (and could exit 0), destroying it. Checked up front, before the heavy apidom-ls import, so it fails fast. - Disable ANSI color when writing to a file. chalk keys color off process.stdout, so a stylish report written to a file from a TTY contained escape sequences. The formatter now takes an optional `color` flag; the action passes `color: false` for file output, so file reports are always plain text regardless of TTY/FORCE_COLOR. Adds tests for both (input preserved on rejection; no ESC bytes in the file even under FORCE_COLOR) and documents the behavior in the README.
ddb3e70 to
c3f03cd
Compare
There was a problem hiding this comment.
Pull request overview
Adds file output support to speclynx validate, including plain-text formatting, safeguards, tests, and documentation.
Changes:
- Adds
-o, --output <file>for diagnostic reports. - Disables ANSI colors for file output.
- Adds output behavior tests and documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/commands/validate/index.ts |
Registers the output option. |
src/commands/validate/action.ts |
Writes reports to files and protects input paths. |
src/commands/validate/formatters/types.ts |
Adds formatter color control. |
src/commands/validate/formatters/stylish.ts |
Disables colors when requested. |
test/commands/validate/index.ts |
Tests output files, errors, and colors. |
README.md |
Documents file output usage. |
package-lock.json |
Reconciles optional Babel dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review findingsThe feature design is right, but commit 🔴 Blocking1. if (opts.output && path.resolve(opts.output) === resolvedPath) {No This contradicts the "typescript:check-types clean" claim in the description. 2. The whole 🟠 Logic bug (survives fixing #1)3. The overwrite guard never triggers, even with the right variable name. path.resolve(opts.output) === fileURI // "/home/u/spec.yaml" === "file:///home/u/spec.yaml"
💡 Suggestion: simplify the color handlingThe const payload = opts.output ? stripAnsi(rendered) : rendered;
fs.writeFileSync(path.resolve(opts.output), payload, 'utf-8');This collapses the whole color half of the commit into one line, removes the ✅ Looks good
Verdict: request changes — #1 and #2 must be fixed before merge, and #3 makes the headline safety feature a no-op. |
Address review of the rebased branch (char0n, Copilot): - Fix the overwrite guard, broken by the rebase onto the URL-input work (#102). It referenced an undefined `resolvedPath` (build failure) and, once named, compared a filesystem path against a file:// URI so it never matched. Now compares path-to-path via fileURLToPath(fileURI), and skips http(s) inputs where there is no local file to clobber. - Remove duplicate `fs` import in the validate test left by the rebase (was a duplicate-identifier build error). - Simplify color handling: instead of threading a `color` flag through FormatterContext/stylish/severityToString, strip ANSI at the single file write site. Formatters are oblivious to their destination again; stylish.ts and types.ts revert to their pre-refactor form. tsc, eslint, and the validate suite (incl. the overwrite and no-ANSI file tests) pass.
|
Thanks @char0n — all three defects and the suggestion are addressed in 0c72cbc. The two build breaks came from the manual rebase onto #102, not the original commit. 🔴 #1 — undefined 🔴 #2 — duplicate 🟠 #3 — guard was a no-op (path vs URI): fixed, and you were right to flag the test. The guard now compares path-to-path, 💡 Suggestion — simplify color handling: applied. Dropped the
Note on the 2 |
Move stripAnsi into its own module and apply it only to non-json file output, matching jentic-api-scorecard's writeReport: the json formatter emits no ANSI, so its output is written verbatim. Wrap write failures with the resolved path (failed to write <path>: <message>) for a clearer error than the bare Node message. Refs #94
Move the stdout-vs-file dispatch, ANSI stripping, and contextual write error out of the action into a writeReport helper (mirrors jentic-api-scorecard's output.ts). The action now just renders and calls writeReport; the input-overwrite guard stays in the action since it runs before the heavy apidom-ls import. strip-ansi becomes output.ts's helper. Refs #94
The overwrite guard compared resolved path strings only, so a symlink, hard link, or case-insensitive-filesystem alias to the input slipped through and clobbered the document. The input was just read, so it exists on disk; wouldOverwriteInput now also compares filesystem identity (dev + ino) when the output exists, falling back to the string compare when it does not. ino==0 (some Windows/network FS) is not trusted. A 'd sub-document is still out of scope. Refs #94
Replace the run-on exit-code sentence with a code table and a stdout/ stderr distinguisher. The old 'exits 1 on a failing diagnostic, 0 otherwise' wording was inaccurate: hard errors (missing input, bad $ref, unwritable/input-colliding -o path, internal failure) also exit 1 with zero diagnostics. Clarify that empty stdout + non-zero means the run failed, while diagnostics + non-zero means the document is invalid. Refs #94
Summary of the code-review follow-ups applied on this branch, why, and the issues opened for out-of-scope items. Addressed here: - Gate ANSI stripping on format so json file output is written verbatim; only the stylish formatter emits chalk colors. (4f18b59) - Extract report writing (stdout-vs-file dispatch, ANSI strip, contextual write error) into output.ts, with strip-ansi as its helper — keeps the action focused and the write path unit-testable. (11a63cf, 4f18b59) - Wrap write failures with the resolved path: 'failed to write <path>: <message>', clearer than the bare Node error. (4f18b59) - Harden the input-overwrite guard: compare filesystem identity (dev+ino) when the output exists, not just resolved path strings, so symlink, hard link, and case-insensitive-fs aliases to the input are caught. ino==0 is distrusted; a $ref'd sub-document stays out of scope. (4f95f4d) - Document exit codes as a table and clarify the stdout/stderr distinguisher; the old '1 on failing diagnostic, 0 otherwise' wording ignored hard-error paths that also exit 1. (d3d761c) Follow-up issues opened: - #108 — add the same input-overwrite guard to overlay apply/diff, which share -o/--output but can currently clobber their inputs. - #109 — support piped stdin input for validate, with a --base-uri anchor for $ref resolution and explicit rules for the neither/both source cases. Refs #94, #95, #108, #109
Review follow-ups appliedPushed a set of commits addressing the review feedback ( Addressed on this branch
Out of scope — follow-up issues opened
Also still out of scope for the guard itself: an output path matching a relative |
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Adds a
-o, --output <file>option tospeclynx validatethat writes the selected formatter's output to a file instead of stdout. Implements #94.The option mirrors the
overlaycommand's existing-o, --output, so the flag is consistent across commands. The file is created/overwritten; write errors surface on stderr with a non-zero exit; and the validation exit code is otherwise unchanged. File content is byte-identical to stdout (formatter output plus a trailing newline).Changes
src/commands/validate/index.ts— adds.option('-o, --output <file>', 'write diagnostics to file instead of stdout'), grouped with the other output flags.src/commands/validate/action.ts— addsoutput?: stringtoValidateActionOptionsand branches the render step:fs.writeFileSync(path.resolve(opts.output), rendered, 'utf-8')when-ois given, elseprocess.stdout.write(rendered). No new imports (fs/pathalready present). Exit-code semantics (process.exitCode, computed from the full diagnostic set), the sort-once/cap-once logic, and thefinally { service.terminate() }are all preserved —-onever changes the exit code, and a failed write is caught by the existing handler that reportsError: …on stderr with exit 1.test/commands/validate/index.ts— a--outputdescribe block with three tests: writing a stylish report to a file (empty stdout, exit 0), writing JSON diagnostics for an invalid doc (empty stdout, non-zero exit preserved, file parses to a non-empty array), and a write-error path (unwritable path → non-zero exit +Error:on stderr).README.md— documents the option in the validate options table, the output paragraph, and a new example.package-lock.json— incidental reconciliation with the Babel 8 dependency set on this branch (fills in the missing@emnapi/core/@emnapi/runtimeoptional-peer entries sonpm ciis back in sync). No production dependencies changed.Alignment with CLI Agent Experience guidelines (#95)
Reviewed against the Agent-Friendly CLI Checklist and related resources linked from #95. This change advances several checklist items and introduces no regressions (each behavior verified against the built binary):
-o, stdout stays empty and the report goes only to the file; errors go to stderr. stdout remains reserved for composable data.--format json -o report.jsonwrites a clean, parseable diagnostics array (the JSON path carries no ANSI escapes regardless of color settings), which is the machine-readable report-file use case validate: add -o, --output <file> to write diagnostics to a file #94 called for.-o, --output <file>matchesoverlay's option verbatim, satisfying the "consistent flag names across commands" rule.--helplists the option with a clear description, and the README documents it with a copy-paste example.One out-of-scope observation surfaced during review, tracked for #95 rather than this PR: the
stylishformatter emits ANSI color whenFORCE_COLOR=1is set even to a non-TTY destination (pre-existing chalk behavior affecting piped stdout identically; default andNO_COLORruns write clean files, and the JSON path is always clean). A destination-aware color fix belongs with the broader CLI-AX work in #95.Verification
npm run typescript:check-types,npm run lint— clean.npm test— full suite passes (71 passing), including the three new--outputtests.-o(exit 0, empty stdout, file has "No problems found"); invalid doc--format json -o(exit 1, empty stdout, file is a non-empty array); unwritable path (exit 1,Error:on stderr); both-oand--outputspellings work;--helplists the option.Closes #94
Refs #95