Skip to content

chore: harden CLI review contracts - #61

Merged
tiye merged 1 commit into
mainfrom
chore/review-hardening
Aug 23, 2026
Merged

chore: harden CLI review contracts#61
tiye merged 1 commit into
mainfrom
chore/review-hardening

Conversation

@tiye

@tiye tiye commented Aug 22, 2026

Copy link
Copy Markdown
Member

Summary

This maintenance PR consolidates concrete review fixes from the existing stack:

  • make qcoffee -e/file/mode conflicts order-independent and accurately diagnosed
  • include offending paths in qtest collection/canonicalization errors
  • make TAP path assertions platform independent

Verification

  • CARGO_TARGET_DIR=/tmp/qcoffee-hardening cargo test --locked --test cli_tools
  • 12 CLI integration tests passed

Copilot AI lite review requested due to automatic review settings August 22, 2026 18:28
@tiye
tiye force-pushed the chore/review-hardening branch from 794f50f to ced23c1 Compare August 22, 2026 18:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens QuickCoffee’s CLI contracts by introducing qcoffee --json single-result output (including structured errors), improving qtest collection diagnostics to include offending paths, and making TAP path assertions platform-independent across OS path separators.

Changes:

  • Add qcoffee --json output mode with deterministic JSON serialization for values and structured error reporting.
  • Strengthen CLI argument conflict handling/tests (including order-independent conflict scenarios) and expand integration test coverage for JSON output.
  • Improve qtest file collection error messages by attaching the failing path; adjust TAP tests to compare Path::display() output.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/cli_tools.rs Updates TAP assertions to be platform-independent; adds CLI integration tests for qcoffee --json and additional conflict-order coverage.
src/main.rs Implements --json mode, JSON value/error formatting, and new conflict handling paths for CLI parsing.
src/bin/qtest.rs Adds path context to filesystem errors during test file collection/canonicalization.
RFCs/0117-qcoffee-json-output.md Introduces an RFC defining the qcoffee --json CLI contract and acceptance criteria.
RFCs/0000-project-scope.md Extends the documented RFC baseline to include RFC 0117.
README.md Updates top-level RFC range and adds a qcoffee --json usage example.
manuals/manual.*.qc Documents the new qcoffee --json behavior in all manual locales.
docs/syntax.*.md Updates syntax index to include qcoffee --json.
docs/manual.*.(md html)
Suppressed comments (4)

src/main.rs:235

  • The --check option rejects --json, but the current error message groups --json together with --stats as mutually exclusive execution-mode alternatives. Since --json is intended to work alongside --stats, this wording is misleading and differs from the later post-parse conflict message when --json appears after --check. Consider emitting a dedicated --json conflict message here so the behavior/message is order-independent and doesn’t imply --stats conflicts with --json.
                if source.is_some() || dump || check || fingerprint || stats || json {
                    eprintln!(
                        "-e, --check, --dump-bytecode, --fingerprint, --json, and --stats are execution-mode alternatives"
                    );
                    return ExitCode::from(2);
                }

src/main.rs:261

  • The --fingerprint conflict check uses a single error message for both --stats and --json conflicts. Because --json is allowed with --stats, this can mislead users into thinking those flags are incompatible, and it produces different messages depending on whether --json comes before or after --fingerprint. Split out the json case and use a dedicated message consistent with the post-parse check.
                if source.is_some() || dump || check || fingerprint || stats || json {
                    eprintln!(
                        "-e, --check, --dump-bytecode, --fingerprint, --json, and --stats are execution-mode alternatives"
                    );
                    return ExitCode::from(2);
                }

src/main.rs:246

  • In the --check branch, the read-source error handler includes a if json { ... } path, but --check is already rejected when json is set (and when --json appears after --check, json is still false at this point). This makes the JSON printing branch effectively unreachable and adds confusing dead code to a user-facing path.

This issue also appears on line 310 of the same file.

                        Err(error) => {
                            if json {
                                println!("{}", json_io_error("read", &error));
                            } else {
                                eprintln!("{error}");
                            }
                            return ExitCode::from(1);

src/main.rs:314

  • This post-parse --json conflict check currently emits a different error message than the earlier per-flag conflict checks, so qcoffee --json --check FILE and qcoffee --check FILE --json can produce different diagnostics. To match the PR goal of order-independent, accurately diagnosed conflicts, use the same dedicated --json incompatibility message in both places.
    if json && (dump || check || fingerprint) {
        eprintln!(
            "--check, --dump-bytecode, --fingerprint, and --json are execution-mode alternatives"
        );
        return ExitCode::from(2);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/main.rs Outdated
@tiye
tiye force-pushed the feat/qtest-selection branch from 2f35c37 to db7d796 Compare August 22, 2026 18:35
@tiye
tiye force-pushed the chore/review-hardening branch 2 times, most recently from a53fedc to 9094259 Compare August 22, 2026 18:51
@tiye
tiye force-pushed the feat/qtest-selection branch from db7d796 to 1bcc853 Compare August 23, 2026 03:44
Base automatically changed from feat/qtest-selection to feat/context-builder-api August 23, 2026 04:13
@tiye
tiye force-pushed the feat/context-builder-api branch from 6ab7a33 to c5296d4 Compare August 23, 2026 07:03
@tiye
tiye force-pushed the chore/review-hardening branch 3 times, most recently from 289e1c7 to 2a79f1e Compare August 23, 2026 07:40
Base automatically changed from feat/context-builder-api to feat/program-fingerprint August 23, 2026 07:45
Base automatically changed from feat/program-fingerprint to main August 23, 2026 07:48
@tiye
tiye force-pushed the chore/review-hardening branch from 2a79f1e to 1e2c403 Compare August 23, 2026 07:48
@tiye
tiye force-pushed the chore/review-hardening branch from 1e2c403 to 9a8d887 Compare August 23, 2026 08:02
@tiye
tiye merged commit f33b07d into main Aug 23, 2026
4 checks passed
@tiye
tiye deleted the chore/review-hardening branch August 23, 2026 08:04
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.

2 participants