feat: fold #60 (metadata test) + #58 (one-sided startup diff) into 3.0 - #66
Merged
Merged
Conversation
When a configuration starts on one side of the comparison but fails to start on the other (e.g. a new CLI flag that does not exist on the compare ref), the action previously collapsed the whole config to an opaque `error` diff and aborted, hiding the working side's surface and failing the run for an expected reason. Now, when exactly one side fails to start: - the failed side is treated as an empty ProbeResult and the working side is diffed against it, so its entire surface renders as added/removed; - it is tagged with a distinct `config-missing` category and an explanatory note naming the side/version that could not start; - it is non-fatal: `fail_on_error` only trips on genuine probe errors (both sides failing), while `config-missing` still counts as a difference for `fail_on_diff`. Both-sided startup failures keep the existing hard-error behavior. - runner: extract PHASE 3 logic into exported, testable `compareConfigResults` - reporter: dedicated "missing on one side" section + per-config callout - index: surface config-missing configs; keep fail_on_error on `error` - types: add `configMissing` marker to TestResult - tests: cover one-sided/both-sided paths and report rendering - docs: document the new behavior; rebuild dist/ Closes #57 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR finalizes the v3.0 release branch by folding in two pending changes: (1) adding direct unit coverage for metadata diffs in the diff engine, and (2) improving how one-sided startup failures are reported by diffing the working side against an empty baseline and treating that case as non-fatal (separate from genuine probe errors).
Changes:
- Add
compareConfigResults()andTestResult.configMissingto normalize one-sided startup failures into a “missing on one side” marker while still producing full surface diffs. - Update reporter + action status logic so
fail_on_errortriggers only on genuine probe errors (both sides failing), and render dedicated “missing on one side” sections/callouts. - Add/extend tests to cover config-missing behavior and tool metadata diff detection.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds TestResult.configMissing metadata for one-sided startup failures. |
| src/runner.ts | Introduces compareConfigResults() and wires Phase 3 comparison through it. |
| src/reporter.ts | Adds categorization helpers and renders “missing on one side” in markdown + PR summary. |
| src/index.ts | Adjusts failure/warning behavior so fail_on_error keys only on genuine probe errors. |
| src/tests/runner.test.ts | Adds unit tests for compareConfigResults() scenarios (both-error / one-sided / normal). |
| src/tests/reporter.test.ts | Adds coverage for config-missing rendering in both markdown report and PR summary. |
| src/tests/diff.test.ts | Adds regression test ensuring tool + nested schema description changes are diffed under tools. |
| README.md | Documents one-sided startup failure behavior and clarifies fail_on_error. |
| dist/types.d.ts | Regenerated type declarations for configMissing. |
| dist/runner.d.ts | Regenerated declarations for compareConfigResults() and related types. |
| dist/index.js | Regenerated build output reflecting runner/reporter/index changes. |
| dist/cli/types.d.ts | Regenerated CLI type declarations for configMissing. |
| dist/cli/runner.d.ts | Regenerated CLI runner declarations for compareConfigResults(). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/13 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+422
to
+423
| const branchError = branchResult?.error; | ||
| const baseError = baseResult?.error; |
Owner
Author
There was a problem hiding this comment.
Good catch — fixed in 0e41ccd. Added an explicit guard at the top of compareConfigResults that returns a fatal "error" outcome with a clear message naming which side(s) were missing, then dropped the ! assertions from the rest of the function. Test added in runner.test.ts.
Comment on lines
+193
to
+196
| if (result.configMissing?.error) { | ||
| lines.push(`>`); | ||
| lines.push(`> Startup error: \`${result.configMissing.error}\``); | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in 0e41ccd. Added a sanitizeErrorForInlineCode helper that replaces backticks with single quotes and collapses whitespace/newlines, applied to the inline-code rendering. Test added in reporter.test.ts with a malicious-looking error string (backticks, newlines, fenced code block) confirming no breakout.
- runner: guard against undefined ProbeResult in compareConfigResults so a missing map entry returns a fatal error outcome instead of crashing on '.error' deref. Drops the non-null assertions in the rest of the function. - reporter: sanitize backticks and whitespace in startup-error string before embedding it in an inline-code span, preventing markdown injection / code-span breakouts from server stderr. - Tests added for both: missing-result fatal outcome and backtick/newline sanitization in the rendered markdown report. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comprehensive v3: fold remaining in-flight PRs into 3.0 before tagging
v3.0.0.PR #56 (the main 3.0 upgrade) merged earlier today. Folding two outstanding PRs in so v3.0.0 ships as one complete release.
Folded-in PRs
Closes #60 — test: cover tool metadata diffs (@kigland)
First direct unit tests on the diff engine — covers tool description and nested
inputSchemadescription changes viacompareProbeResults. Authorship preserved on the cherry-picked commit. Clean cherry-pick.Fixes #57 (closes #58) — feat: diff one-sided startup failures against an empty baseline
Previously, a config that started successfully on one side and failed on the other was reported as just a failure, hiding all the public-surface signal. Now a one-sided startup failure is normalized against an empty baseline so the diff renders the entire add/remove delta — the actual useful signal in that scenario.
compareConfigResults()helper insrc/runner.ts(unit-testable)TestResult.configMissingfield carries the missing-side metadatafail_on_error(action) keyed on genuineerrorentries only; config-missing is non-fatalrunner.test.tsandreporter.test.tsConflict resolution: PR #58 and PR #56 both edited the renderer prelude in
src/reporter.ts(protocol-version banner from #56, config-missing callout from #58). Both kept, banner first then callout — independent features sharing the same lines array. The straychore: bump version to 2.4.0commit on #58 was dropped (we're on 3.0).Verification
npm run check,npm run buildcleanAfter merge
Tag
v3.0.0(skipping rc per Sam's plan).package.jsonstill at3.0.0-rc.0— bump can happen with the tag or as a final commit.