feat: json output support for monograph/subgraph check#2590
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds JSON output support to schema check commands, refactors check-result handling into a JsonOutputBuilder/API, wires a -j/--json flag into check CLIs, expands in-process CLI tests for JSON and text outputs, and applies minor CLI config/formatting adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2590 +/- ##
==========================================
+ Coverage 1.46% 42.24% +40.77%
==========================================
Files 296 1043 +747
Lines 45041 144694 +99653
Branches 432 9618 +9186
==========================================
+ Hits 662 61122 +60460
- Misses 44093 82009 +37916
- Partials 286 1563 +1277
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/test/grpc-service.test.ts (1)
80-102:⚠️ Potential issue | 🟠 MajorMissing
exitOverridecauses test failure.The pipeline failure indicates "process.exit unexpectedly called with '1'". This test needs
program.exitOverride()to prevent the actualprocess.exitcall and allow the error to be caught byrejects.toThrow().🐛 Proposed fix
const program = new Command(); program.addCommand(GenerateCommand({ client })); + program.exitOverride(); const tmpDir = join(tmpdir(), `grpc-test-${Date.now()}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/grpc-service.test.ts` around lines 80 - 102, The test calls program.parseAsync which triggers a process.exit; call program.exitOverride() on the Command instance before invoking parseAsync so the CLI throws an exception instead of exiting. Locate the Command instance creation (program = new Command()) used with GenerateCommand({ client }) and insert program.exitOverride() right after adding the command and before program.parseAsync in the 'should fail when input file does not exist' test so the await expect(...).rejects.toThrow() can catch the error.
🧹 Nitpick comments (1)
cli/test/check-schema.test.ts (1)
69-82: Make JSON extraction deterministic.
getJsonOutputreturns the first parseable JSON log. If any earlier log becomes JSON-like, assertions can bind to the wrong payload.Proposed refactor
function getJsonOutput(logSpy: MockInstance<typeof console.log>): JsonOutputDescriptor { - const call = logSpy.mock.calls.find(([arg]) => { + const jsonCalls = logSpy.mock.calls.filter(([arg]) => { try { JSON.parse(String(arg)); return true; @@ - if (!call) { + if (jsonCalls.length === 0) { throw new Error('No JSON output found in console.log calls'); } - return JSON.parse(String(call[0])); + if (jsonCalls.length > 1) { + throw new Error(`Expected exactly one JSON output, found ${jsonCalls.length}`); + } + return JSON.parse(String(jsonCalls[0][0])); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/check-schema.test.ts` around lines 69 - 82, getJsonOutput is nondeterministic because it picks the first parseable JSON from logSpy.mock.calls; change it to pick the last parseable JSON so assertions bind to the most recent payload. Locate the getJsonOutput function and replace the search logic over logSpy.mock.calls with a reverse-order search (iterate from end to start or use Array.prototype.slice().reverse().find) to find the last call whose first arg parses as JSON, then return JSON.parse(String(call[0])) and keep the existing error thrown when no JSON is found; ensure references to logSpy and JsonOutputDescriptor remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/graph/monograph/commands/check.ts`:
- Around line 23-25: Remove the duplicate option declaration for the limit flag:
locate the two identical command.option('-l, --limit [number]', 'The amount of
entries shown in the schema checks output.', '50') entries in the monograph
check command and delete the redundant one so only a single '-l, --limit' option
remains (leaving the '-j, --json' option untouched).
In `@cli/src/handle-check-result.ts`:
- Around line 169-177: The addLintWarnings method incorrectly sets
this.data.lint.success = false when adding warnings; change it to leave success
unchanged (do not set success to false) so adding to this.data.lint.warnings
only appends warnings and preserves the existing success state—update the
addLintWarnings function to merge warnings into this.data.lint.warnings without
modifying this.data.lint.success (referencing addLintWarnings and
this.data.lint).
- Around line 190-199: The addGraphPruneWarnings method wrongly forces success:
false; update it to preserve the existing success state (do not mark as failed
for warnings). Replace the hardcoded success: false with a preservation
expression like success: this.data.graphPrune?.success ?? true (or simply remove
the success assignment so the previous value is kept) in the
addGraphPruneWarnings method to match other warning handlers.
- Around line 149-157: The addCompositionWarnings method currently forces
this.data.composition.success to false; change it to preserve the existing
success value (or default to true if missing) instead of marking composition as
failed for warnings. In the addCompositionWarnings function, remove the
hard-coded success: false and set success to this.data.composition?.success ??
true (or simply omit changing success so it remains unchanged), while still
appending the new warnings to this.data.composition.warnings and returning this.
In `@cli/test/grpc-service.test.ts`:
- Around line 123-131: The test currently asserts the exit error inside
program.exitOverride's callback (program.exitOverride) which doesn't propagate
to the test harness; instead, remove the assertion from the exitOverride
callback and assert the error message on the rejected promise returned by
program.parseAsync (including checking that the rejection message contains
`Output directory ${outputFile} is not a directory` or the exact expected text).
Keep exitOverride only to prevent process.exit, and perform the
expect(...).rejects.toThrow(...) (or rejects.toMatch/contains) against
program.parseAsync to validate the error message.
---
Outside diff comments:
In `@cli/test/grpc-service.test.ts`:
- Around line 80-102: The test calls program.parseAsync which triggers a
process.exit; call program.exitOverride() on the Command instance before
invoking parseAsync so the CLI throws an exception instead of exiting. Locate
the Command instance creation (program = new Command()) used with
GenerateCommand({ client }) and insert program.exitOverride() right after adding
the command and before program.parseAsync in the 'should fail when input file
does not exist' test so the await expect(...).rejects.toThrow() can catch the
error.
---
Nitpick comments:
In `@cli/test/check-schema.test.ts`:
- Around line 69-82: getJsonOutput is nondeterministic because it picks the
first parseable JSON from logSpy.mock.calls; change it to pick the last
parseable JSON so assertions bind to the most recent payload. Locate the
getJsonOutput function and replace the search logic over logSpy.mock.calls with
a reverse-order search (iterate from end to start or use
Array.prototype.slice().reverse().find) to find the last call whose first arg
parses as JSON, then return JSON.parse(String(call[0])) and keep the existing
error thrown when no JSON is found; ensure references to logSpy and
JsonOutputDescriptor remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 972a57a3-3659-4781-a0c8-aab73f313f9e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
cli/.gitignorecli/README.mdcli/package.jsoncli/src/commands/graph/monograph/commands/check.tscli/src/commands/subgraph/commands/check.tscli/src/handle-check-result.tscli/test/check-schema.test.tscli/test/fetch-schema.test.tscli/test/fixtures/full-schema.graphqlcli/test/fixtures/schema-with-nullable-list-items.graphqlcli/test/fixtures/schema-with-validation-errors.graphqlcli/test/fixtures/schema-with-warnings-and-errors.graphqlcli/test/grpc-service.test.tscli/test/parse-operations.test.tscli/test/testdata/query-map.jsoncli/tsconfig.jsoncli/vite.config.ts
6 tasks
comatory
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/test/check-schema.test.ts (2)
69-82: Make JSON extraction in tests descriptor-aware (not just JSON-parseable).On Line 70, selecting the first parseable JSON log can match unrelated JSON-ish output and make tests flaky. Prefer selecting the latest parsed object that matches
JsonOutputDescriptorshape (status+code).Proposed stabilization patch
function getJsonOutput(logSpy: MockInstance<typeof console.log>): JsonOutputDescriptor { - const call = logSpy.mock.calls.find(([arg]) => { - try { - JSON.parse(String(arg)); - return true; - } catch { - return false; - } - }); - if (!call) { + const parsed = logSpy.mock.calls + .map(([arg]) => { + try { + return JSON.parse(String(arg)); + } catch { + return null; + } + }) + .filter( + (value): value is JsonOutputDescriptor => + !!value && typeof value === 'object' && 'status' in value && 'code' in value, + ); + + const output = parsed.at(-1); + if (!output) { throw new Error('No JSON output found in console.log calls'); } - return JSON.parse(String(call[0])); + return output; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/check-schema.test.ts` around lines 69 - 82, The test helper getJsonOutput should pick the most recent console.log entry that matches the JsonOutputDescriptor shape instead of the first JSON-parseable string; update getJsonOutput to iterate logSpy.mock.calls in reverse, JSON.parse each call, and return the first parsed value that is an object and has both 'status' and 'code' properties (and non-null), throwing an error if none match; reference the getJsonOutput function and JsonOutputDescriptor when making this change.
176-177: Avoid positional log assertions for breaking-change messages.On Line 176 and Line 193, assertions depend on
mock.calls[1], which is brittle if log ordering changes. Assert that any call matches the message instead.Proposed assertion hardening
- expect(String(logSpy.mock.calls[1]?.[0])).toMatch(/Found .*1.* breaking changes\./); + expect(logSpy).toHaveBeenCalledWith(expect.stringMatching(/Found .*1.* breaking changes\./)); - expect(String(logSpy.mock.calls[1]?.[0])).toMatch(/2.*operations impacted\./); - expect(String(logSpy.mock.calls[1]?.[0])).toMatch(/1.*operations marked safe due to overrides\./); + expect(logSpy).toHaveBeenCalledWith(expect.stringMatching(/2.*operations impacted\./)); + expect(logSpy).toHaveBeenCalledWith(expect.stringMatching(/1.*operations marked safe due to overrides\./));Also applies to: 193-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/check-schema.test.ts` around lines 176 - 177, Replace brittle positional assertions that reference logSpy.mock.calls[1] with a search across all calls so the test asserts that any logged call matches the breaking-change regex; locate the assertions using logSpy (the expect(String(logSpy.mock.calls[1]?.[0])).toMatch(/Found .*1.* breaking changes\./) and the similar block around lines 193-195) and change them to assert that logSpy.mock.calls contains at least one entry whose first argument matches the regex (e.g., use Array.prototype.some or an equivalent check over logSpy.mock.calls to find a call where the message matches /Found .*1.* breaking changes\./).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/test/check-schema.test.ts`:
- Around line 69-82: The test helper getJsonOutput should pick the most recent
console.log entry that matches the JsonOutputDescriptor shape instead of the
first JSON-parseable string; update getJsonOutput to iterate logSpy.mock.calls
in reverse, JSON.parse each call, and return the first parsed value that is an
object and has both 'status' and 'code' properties (and non-null), throwing an
error if none match; reference the getJsonOutput function and
JsonOutputDescriptor when making this change.
- Around line 176-177: Replace brittle positional assertions that reference
logSpy.mock.calls[1] with a search across all calls so the test asserts that any
logged call matches the breaking-change regex; locate the assertions using
logSpy (the expect(String(logSpy.mock.calls[1]?.[0])).toMatch(/Found .*1.*
breaking changes\./) and the similar block around lines 193-195) and change them
to assert that logSpy.mock.calls contains at least one entry whose first
argument matches the regex (e.g., use Array.prototype.some or an equivalent
check over logSpy.mock.calls to find a call where the message matches /Found
.*1.* breaking changes\./).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d09d467-ebcd-4d2d-9359-bf3787f0446c
📒 Files selected for processing (1)
cli/test/check-schema.test.ts
comatory
requested review from
a team,
JivusAyrus,
StarpTech,
endigma,
jensneuse and
wilsonrivera
as code owners
Noroth
reviewed
Mar 9, 2026
JivusAyrus
requested changes
Mar 9, 2026
Member
JivusAyrus
left a comment
JivusAyrus
left a comment
There was a problem hiding this comment.
composedSchemaBreakingChanges are not handled
Contributor
Author
This was not handled by the subgraph check subcommand so far. Could we do it as a follow up PR? Or maybe add it as a new feature, I think the changeset here is already quite big. |
Member
It is handled by the subgraph check command, it was pushed a few days ago. We will need to do it in this pr, as that would cause a break in functionality. |
Contributor
Author
Oh ok I was not aware of that. I will have to check the existing implementation, I did some major refactoring so hopefully I'm able to transfer it. |
Contributor
Author
|
@JivusAyrus Ok I had to backport the changes from main. I guess due to my refactoring, it was not caught as breaking change as we were working on the same file at the same time, anyways it's good you caught it. |
JivusAyrus
reviewed
Mar 10, 2026
StarpTech
requested changes
Mar 10, 2026
JivusAyrus
approved these changes
Mar 10, 2026
StarpTech
requested changes
Mar 10, 2026
This was referenced Mar 30, 2026
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
4 participants
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.
Summary by CodeRabbit
New Features
Tests
Chores
Checklist
Adds
--jsonflag to check commands for monograph/subgraph.The way I implemented this was to first create comprehensive test suite for existing
checksubcommand and assert on stdout.Once that was done, I added JSON support and then I did a somewhat larger refactoring
because the handler function was hard to follow and reason about.
JSON schema of the check result
export type JsonOutputDescriptor = { status: 'error' | 'success'; code: EnumStatusCode; details?: string; message?: string; url?: string; proposals?: { message: string; }; traffic?: { message: string; }; changes?: { breaking: SchemaChange[]; nonBreaking: SchemaChange[]; }; composition?: { errors: CompositionError[]; warnings: CompositionError[]; }; lint?: { errors: LintIssue[]; warnings: LintIssue[]; }; graphPrune?: { errors: GraphPruningIssue[]; warnings: GraphPruningIssue[]; }; extensions?: { message: string; }; exceededRowLimit?: boolean; rowLimit: number; operationUsageStats?: CheckOperationUsageStats; };Caution
There are some changes in other CLI files but they are related to formatting. The issue was that some
files were being ignored by the formatter (tests).
Question(s) for reviewers?
The JSON output is separated into sections, like@JivusAyrus and me talked, we decided the nestedlint: { warning: ..., error: ..., success: ...}. This way I figured it would be nice to just check the.successproperty from DX point of view. I considersuccess=falsewhen either the response has warnings or errors, but Coderabbit raised a point that perhaps that's not correct. What would you propose? My thinking was that it's only successful if no warnings or errors are present, but it depends on what we agree on.successproperties are unnecessary.I noticed when I passI ended up checking whether--skip-traffic-check, the response still containsoperationUsageStats, which means that.trafficproperty gets passed to the output. I should probably detect the flag and omit it completely, correct? Just want to make sure thatoperationUsageStatsare related to this flag.--skip-traffic-checkflag is set and exclude the traffic output. This makes it consistent with stdout.How to test
cli/package directory (running the the source via tsx gets rid of pnpm-relatederror messages which would disallow you piping the JSON to a file):
/tmp/out.jsonshould have data in it, like lint warnings (depending on othersettings you might have)