Skip to content

fix: resolve VariaQ CLI validation and security documentation findings - #11

Merged
aaronphifer merged 1 commit into
mainfrom
fix/pr10-review-p2
Oct 13, 2026
Merged

aaronphifer merged 1 commit into
mainfrom
fix/pr10-review-p2

Conversation

@aaronphifer

Copy link
Copy Markdown
Owner

Summary

Resolves the two outstanding P2 findings from the PR #10 Codex review (submitted after merge), plus both P3 follow-up notes from the independent re-review of this patch. Follow-up to #10.

Finding 1 — invalid --compare values leaked raw ZodError exceptions (P2, server.ts:608)

opReportCampaign validated options.compare with a bare comparisonTypeSchema.parse(). The CLI wrapper only converts PluginError instances into command results and rethrows everything else, so a user typo escaped as an unhandled exception.

  • New parseComparisonOption() CLI helper: safeParse against comparisonTypeSchema; invalid values raise a structured PluginError with a Valid comparisons: … hint — the same convention the CLI already uses for problem families.
  • Applied at all three raw-string CLI boundaries that feed comparisons (report-campaign, analyze-runs, analyze-campaign), replacing unsafe as casts.
  • opAnalyzeRuns / opAnalyzeCampaign converted from bare .parse() to safeParse → PluginError with a short first-issue message (minimizeZodIssue).
  • Valid-comparison behavior, output contracts, the requireComparisonSupport gate, schema-v1 validation, and VariaQ 0.6.x / 0.8.x / 0.8.0rc1 compatibility are unchanged. Agent tools were already schema-validated by the SDK before execute; only CLI paths needed the fix.

Finding 2 — stale source-map-js advisory row in SECURITY.md (P2, SECURITY.md:54)

The audit table reported source-map-js 1.2.1 and a count of six development packages (four high, two moderate), contradicting the shipped override to 1.2.2.

  • Removed the remediated source-map-js row, noting explicitly that the override remediated it (nothing suppressed).
  • Updated to the verified audit state: five development-only packages (three high, two moderate) — brace-expansion, http-cache-semantics, ip-address, postcss-selector-parser, undici — with every currently published GHSA per package and its severity.
  • Documented that all five are development-only transitives of the npm tooling bundled inside bb-app, outside npm's override reach and unreachable from the shipped plugin runtime; the production audit remains clean.
  • No forced audit fix, broad upgrade, or suppression was applied; the five remaining advisories are retained and documented.

Codex P3 follow-ups (independent re-review)

  • Operating-level schema-error coverage: new regression cases drive nine --group-by values against analyzeQuerySchema's max(8) limit through the CLI, asserting structured PluginError output (exit 1, first-issue message, hint, no ZodError leak) for opAnalyzeRuns and opAnalyzeCampaign.
  • Forwarding proof: the fake VariaQ harness now captures --compare and echoes it into the report envelope and the generated markdown artifact (comparison: <value>), so the valid-comparison regression test proves the selector is actually forwarded to the VariaQ subprocess rather than merely accepted.
  • Both coverage layers were red-proofed: they fail against the pristine implementation/harness with the exact defects (raw ZodError payload on stderr; missing comparison echo) and pass with the fix.

Verification

All commands executed locally on this branch (Node v26.8.1, npm 11.19.0):

Check Command Result
Typecheck npm run typecheck pass
Unit + fake-harness regression vitest run tests/runner.test.ts tests/fake.test.ts 104 passed, 1 skipped
Full suite npm test pass
Integration @ real VariaQ 0.8.0 VARIAQ_TEST_PROJECT=… VARIAQ_TEST_VERSION=0.8.0 vitest run tests/server.test.ts 24 passed, 2 skipped (CUDA-Q CPU/GPU legs — CUDA-Q not installed locally)
Integration @ real VariaQ 0.6.0 same, against a dedicated v0.6.0 worktree with a CI-identical -e '.[quantum]' venv 23 passed, 3 skipped (CUDA-Q legs)
Build npm run build pass
SDK pin npm run sdk:check pass — pin 0.6.37 equals host 0.6.37
Production audit npm audit --omit=dev zero vulnerabilities
Full audit npm audit 5 development-only (3 high, 2 moderate) — all inside node_modules/npm bundled tooling, documented in SECURITY.md
Lockfile consistency npm ci --dry-run clean
Package npm pack --dry-run 12 files, ok

Documented limitations

  • The CUDA-Q CPU/GPU integration legs did not run (CUDA-Q is not installed in the local environment); hosted CI runs the corresponding matrix legs and this PR's checks report those results. No CUDA-Q/GPU pass is claimed locally.
  • Local runs cover VariaQ 0.8.0 and 0.6.0; the hosted matrix additionally exercises the reviewed 0.8.0rc1 candidate pin on Node 22, which this environment does not replicate.
  • The five development-only advisories in node_modules/npm's bundled dependencies are retained and documented — they require an upstream npm/bb-app refresh; npm overrides cannot reach them.

Relation to PR #10

  • Resolves review comments 4243678249 (server.ts:608 — convert invalid comparison values into CLI errors) and 4243678252 (SECURITY.md:54 — remove the remediated source-map advisory). As agreed, the original review threads remain unresolved pending independent verification of this patch.

No merge, release tag, version bump, or marketplace metadata changes are included.

- Convert invalid --compare values on report-campaign, analyze-runs, and
  analyze-campaign from an uncaught ZodError into structured PluginError
  CLI errors with valid-value hints (PR #10 review finding 1).
- Correct SECURITY.md to the current audit state: drop the remediated
  source-map-js 1.2.1 row, document five development-only packages
  (three high, two moderate) with complete per-package advisory lists
  (PR #10 review finding 2).
- Add regression coverage for operation-level schema errors (nine
  --group-by values vs the max-8 limit) and prove --compare is forwarded
  to the VariaQ subprocess via a fake-harness artifact echo
  (independent Codex P3 notes).
@aaronphifer

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T01:58:23.396623Z 702f77c Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 702f77cdf8

ℹ️ 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".

@aaronphifer
aaronphifer merged commit 5825c37 into main Oct 13, 2026
8 checks passed
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.

1 participant