Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/contract-changes-github-check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'hive': patch
---

Include contract changes in the GitHub CI schema-check summary. Previously, a `schema:check --github` run on a composite/federation project that changed only a contract (while the core composed schema was unchanged) was reported as "No changes". The summary now reports the changed contracts and their changes, and "No changes" is only shown when neither the core schema nor any contract changed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import 'reflect-metadata';
import type { SchemaChangeType } from '@hive/storage';
import { describe, expect, test } from 'vitest';
import { buildSchemaCheckSuccessGithubOutput } from './schema-publisher';

// The helper only reads `.length`, the contract name and passes change arrays to
// the injected renderer, so a minimal fake change + a simple renderer are enough.
const change = (message: string): SchemaChangeType => ({ message }) as unknown as SchemaChangeType;
const renderChanges = (changes: ReadonlyArray<SchemaChangeType>): string =>
changes.map(c => (c as unknown as { message: string }).message).join('\n');

describe('buildSchemaCheckSuccessGithubOutput', () => {
test('reports a contract-only change instead of "No changes" (#6954)', () => {
const result = buildSchemaCheckSuccessGithubOutput({
changes: [],
contractChanges: [{ contractName: 'public', changes: [change('Field Query.b added')] }],
renderChanges,
});

expect(result.title).not.toBe('No changes');
expect(result.title).toBe('No breaking changes');
expect(result.summary).toContain('public');
expect(result.summary).toContain('Field Query.b added');
});

test('still reports "No changes" when neither core nor contracts changed', () => {
const result = buildSchemaCheckSuccessGithubOutput({
changes: [],
contractChanges: [],
renderChanges,
});

expect(result.title).toBe('No changes');
expect(result.summary).toBe('No changes detected');
expect(result.shortSummaryFallback).toBe('No changes detected');
});

test('treats null core changes and null contract changes as "No changes"', () => {
const result = buildSchemaCheckSuccessGithubOutput({
changes: null,
contractChanges: null,
renderChanges,
});

expect(result.title).toBe('No changes');
});

test('treats contracts with empty change lists as "No changes"', () => {
const result = buildSchemaCheckSuccessGithubOutput({
changes: [],
contractChanges: [{ contractName: 'public', changes: [] }],
renderChanges,
});

expect(result.title).toBe('No changes');
});

test('preserves existing behavior for core schema changes with no contracts', () => {
const result = buildSchemaCheckSuccessGithubOutput({
changes: [change('Field Query.a added')],
contractChanges: null,
renderChanges,
});

expect(result.title).toBe('No breaking changes');
expect(result.summary).toContain('Field Query.a added');
// No contract section when there are no contract changes.
expect(result.summary).not.toContain('Contract "');
});

test('lists both core and per-contract changes when both are present', () => {
const result = buildSchemaCheckSuccessGithubOutput({
changes: [change('Field Query.a added')],
contractChanges: [{ contractName: 'public', changes: [change('Field Query.b added')] }],
renderChanges,
});

expect(result.title).toBe('No breaking changes');
expect(result.summary).toContain('Field Query.a added');
expect(result.summary).toContain('## Contract "public"');
expect(result.summary).toContain('Field Query.b added');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,13 @@ export class SchemaPublisher {
organization,
conclusion: checkResult.conclusion,
changes: checkResult.state?.schemaChanges?.all ?? null,
contractChanges:
checkResult.state.contracts
?.map(contract => ({
contractName: contract.contractName,
changes: contract.schemaChanges?.all ?? [],
}))
.filter(contract => contract.changes.length > 0) ?? null,
Comment on lines +955 to +961

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The property checkResult.state is accessed directly as checkResult.state.contracts without optional chaining. In other parts of this method (such as lines 954, 962, and 963), checkResult.state is accessed using optional chaining (checkResult.state?.). To prevent potential runtime TypeError exceptions if state is null or undefined, please use optional chaining here as well.

          contractChanges:
            checkResult.state?.contracts
              ?.map(contract => ({
                contractName: contract.contractName,
                changes: contract.schemaChanges?.all ?? [],
              }))
              .filter(contract => contract.changes.length > 0) ?? null,

breakingChanges: checkResult.state?.schemaChanges?.breaking ?? null,
warnings: checkResult.state?.schemaPolicyWarnings ?? null,
compositionErrors: null,
Expand All @@ -976,6 +983,7 @@ export class SchemaPublisher {
...(checkResult.state.schemaChanges?.breaking ?? []),
...(checkResult.state.schemaChanges?.safe ?? []),
],
contractChanges: null,
breakingChanges: checkResult.state.schemaChanges?.breaking ?? [],
compositionErrors: checkResult.state.composition.errors ?? [],
warnings: checkResult.state.schemaPolicy?.warnings ?? [],
Expand Down Expand Up @@ -1006,6 +1014,7 @@ export class SchemaPublisher {
organization,
conclusion: SchemaCheckConclusion.Success,
changes: null,
contractChanges: null,
breakingChanges: null,
warnings: null,
compositionErrors: null,
Expand All @@ -1023,6 +1032,7 @@ export class SchemaPublisher {
organization,
conclusion: SchemaCheckConclusion.Failure,
changes: null,
contractChanges: null,
breakingChanges: null,
compositionErrors: latestVersion.version.schemaCompositionErrors,
warnings: null,
Expand Down Expand Up @@ -3082,6 +3092,7 @@ export class SchemaPublisher {
conclusion: SchemaCheckConclusion;
warnings: SchemaCheckWarning[] | null;
changes: Array<SchemaChangeType> | null;
contractChanges: Array<{ contractName: string; changes: Array<SchemaChangeType> }> | null;
breakingChanges: Array<SchemaChangeType> | null;
compositionErrors: Array<{
message: string;
Expand All @@ -3098,15 +3109,12 @@ export class SchemaPublisher {
let shortSummaryFallback: string;

if (conclusion === SchemaCheckConclusion.Success) {
if (!changes || changes.length === 0) {
title = 'No changes';
summary = 'No changes detected';
shortSummaryFallback = summary;
} else {
title = 'No breaking changes';
summary = this.changesToMarkdown(changes);
shortSummaryFallback = this.changesToMarkdown(changes, false);
}
({ title, summary, shortSummaryFallback } = buildSchemaCheckSuccessGithubOutput({
changes,
contractChanges: args.contractChanges,
renderChanges: (changesToRender, printListOfChanges) =>
this.changesToMarkdown(changesToRender, printListOfChanges),
}));
} else {
const total =
(compositionErrors?.length ?? 0) + (breakingChanges?.length ?? 0) + (errors?.length ?? 0);
Expand Down Expand Up @@ -3489,6 +3497,54 @@ function writeChanges(
}
}

/**
* Builds the title and summary for a *successful* GitHub schema-check run.
*
* A successful check can still contain changes - either to the core schema or
* to one or more contracts. "No changes" must only be reported when there are
* no core changes AND no contract changes; otherwise the summary lists the core
* changes followed by a per-contract section, so a contract-only change is no
* longer misreported as "No changes" (#6954).
*
* Pure function (the markdown renderer is injected) so it can be unit-tested
* without constructing the full SchemaPublisher dependency graph.
*/
export function buildSchemaCheckSuccessGithubOutput(input: {
changes: Array<SchemaChangeType> | null;
contractChanges: Array<{ contractName: string; changes: Array<SchemaChangeType> }> | null;
renderChanges: (
changes: ReadonlyArray<SchemaChangeType>,
printListOfChanges?: boolean,
) => string;
}): { title: string; summary: string; shortSummaryFallback: string } {
const coreChanges = input.changes ?? [];
const contractChanges = input.contractChanges?.filter(contract => contract.changes.length) ?? [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using contract.changes.length directly as a boolean predicate in .filter() is less explicit and can trigger linting errors under strict TypeScript configurations. For consistency with the implementation on line 961, please use an explicit comparison like contract.changes.length > 0.

  const contractChanges = input.contractChanges?.filter(contract => contract.changes.length > 0) ?? [];


if (coreChanges.length === 0 && contractChanges.length === 0) {
const summary = 'No changes detected';
return { title: 'No changes', summary, shortSummaryFallback: summary };
}

const buildSummary = (printListOfChanges: boolean) =>
[
coreChanges.length ? input.renderChanges(coreChanges, printListOfChanges) : null,
...contractChanges.map(contract =>
[
`## Contract "${contract.contractName}"`,
input.renderChanges(contract.changes, printListOfChanges),
].join('\n'),
),
]
.filter(Boolean)
.join('\n\n');

return {
title: 'No breaking changes',
summary: buildSummary(true),
shortSummaryFallback: buildSummary(false),
};
}

function buildGitHubActionCheckName(input: {
targetName: string;
projectName: string;
Expand Down
Loading