Skip to content
Open
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
4 changes: 2 additions & 2 deletions .github/workflows/module-definitions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ jobs:
- name: Generate publish dry-run plan
id: publish_plan
continue-on-error: true
run: node tools/ravion-modules/dist/src/cli.js publish --format markdown --output publish-plan.md 2> publish-plan.err
run: node tools/ravion-modules/dist/src/cli.js publish --validate-remote --format markdown --output publish-plan.md 2> publish-plan.err

- name: Comment publish dry-run plan
uses: actions/github-script@v7
Expand Down Expand Up @@ -136,7 +136,7 @@ jobs:
working-directory: tools/ravion-modules

- name: Dry-run publish plan
run: node tools/ravion-modules/dist/src/cli.js publish
run: node tools/ravion-modules/dist/src/cli.js publish --validate-remote

- name: Configure Git tag author
run: |
Expand Down
2 changes: 1 addition & 1 deletion tools/ravion-modules/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ if (command === "validate") {
const outputPath = getArgValue(args, "--output");
let result;
try {
result = await publishDefinitions(compiled, client, { dryRun: localDev ? args.includes("--dry-run") : !args.includes("--apply"), localDev, localDevForce: args.includes("--force"), localDevSourceRef, logger: (message) => console.error(`[publish] ${message}`) });
result = await publishDefinitions(compiled, client, { dryRun: localDev ? args.includes("--dry-run") : !args.includes("--apply"), validateRemote: args.includes("--validate-remote"), localDev, localDevForce: args.includes("--force"), localDevSourceRef, logger: (message) => console.error(`[publish] ${message}`) });
} catch (error) {
if (isPublishPlanError(error)) {
const output = format === "markdown" ? formatPublishPlanMarkdown(error.result) : JSON.stringify(error.result, null, 2);
Expand Down
1 change: 1 addition & 0 deletions tools/ravion-modules/src/generate-definitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export interface RemoteModuleDefinition {
type: string;
name: string;
description: string;
isGlobalPublished?: boolean;
}

export interface RemoteModuleVersion {
Expand Down
140 changes: 131 additions & 9 deletions tools/ravion-modules/src/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export interface PublishResult {
dryRun: boolean;
items: PublishPlanItem[];
errors?: PublishPlanErrorItem[];
validationErrors?: PublishValidationErrorItem[];
}

export interface PublishPlanErrorItem {
Expand All @@ -38,6 +39,12 @@ export interface PublishPlanErrorItem {
diff?: string;
}

export interface PublishValidationErrorItem {
type: string;
version: string;
message: string;
}

export interface ModuleDefinitionInput {
type: string;
name: string;
Expand Down Expand Up @@ -66,6 +73,7 @@ export interface RavionApiClientOptions {

export interface PublishOptions {
dryRun?: boolean;
validateRemote?: boolean;
localDev?: boolean;
localDevForce?: boolean;
localDevSourceRef?: string;
Expand All @@ -80,6 +88,7 @@ export interface RavionModuleApiClient {
patchModuleDefinition(input: ModuleDefinitionPatchInput): Promise<RemoteModuleDefinition>;
listModuleVersions(moduleDefinitionId: string): Promise<RemoteModuleVersion[]>;
createModuleVersion(input: ModuleVersionInput): Promise<RemoteModuleVersion>;
validateModuleVersion(input: ModuleVersionInput): Promise<void>;
}

export class PublishError extends Error {
Expand Down Expand Up @@ -127,11 +136,15 @@ export async function publishDefinitions(
inventory.definitions.map((definition) => [definition.type, definition]),
);
const items: PublishPlanItem[] = [];
const validationErrors: PublishValidationErrorItem[] = [];

for (const definition of [...definitionsToPublish].sort((left, right) =>
left.type.localeCompare(right.type),
)) {
let remoteDefinition = definitionsByType.get(definition.type);
const shouldPlanGlobalPublication = remoteDefinition?.isGlobalPublished === false;
const shouldPublishDefinitionAfterVersion =
!remoteDefinition || remoteDefinition.isGlobalPublished === false;
if (!remoteDefinition) {
items.push(
createItem(
Expand All @@ -153,10 +166,6 @@ export async function publishDefinitions(
name: definition.name,
description: definition.description,
});
remoteDefinition = await client.patchModuleDefinition({
id: remoteDefinition.id,
isGlobalPublished: true,
});
definitionsByType.set(remoteDefinition.type, remoteDefinition);
inventory.versionsByDefinitionId[remoteDefinition.id] = [];
}
Expand Down Expand Up @@ -191,12 +200,30 @@ export async function publishDefinitions(
}
}

const latestRemoteVersion = remoteDefinition
? selectLatestVersion(inventory.versionsByDefinitionId[remoteDefinition.id] ?? [])
: undefined;
const globalPublicationItem = shouldPlanGlobalPublication
? createItem(
definition,
"patch-definition",
dryRun,
`Publish module definition ${definition.type} globally.`,
"Make the module definition available globally.",
createDiff(
{ isGlobalPublished: false },
{ isGlobalPublished: true },
),
latestRemoteVersion?.version,
)
: undefined;

const remoteVersion = remoteDefinition
? (inventory.versionsByDefinitionId[remoteDefinition.id] ?? []).find(
(version) => version.version === definition.version,
)
: undefined;
if (remoteVersion) {
if (remoteVersion && remoteDefinition) {
items.push(
createItem(
definition,
Expand All @@ -208,12 +235,19 @@ export async function publishDefinitions(
remoteVersion.version,
),
);
if (globalPublicationItem) {
items.push(globalPublicationItem);
}
if (!dryRun && shouldPublishDefinitionAfterVersion) {
remoteDefinition = await client.patchModuleDefinition({
id: remoteDefinition.id,
isGlobalPublished: true,
});
definitionsByType.set(remoteDefinition.type, remoteDefinition);
}
continue;
}

const latestRemoteVersion = remoteDefinition
? selectLatestVersion(inventory.versionsByDefinitionId[remoteDefinition.id] ?? [])
: undefined;
items.push(
createItem(
definition,
Expand All @@ -225,16 +259,66 @@ export async function publishDefinitions(
latestRemoteVersion?.version,
),
);
if (globalPublicationItem) {
items.push(globalPublicationItem);
}
if (dryRun && options.validateRemote) {
if (!remoteDefinition) {
options.logger?.(
`Skipping remote validation for ${definition.type}@${definition.version}; the module definition does not exist remotely yet.`,
);
Comment on lines +265 to +269

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 New definitions bypass validation

When a new module has config accepted locally but rejected by the Ravion API, both dry runs skip remote validation because the definition does not exist yet. The later --apply run creates and globally publishes the definition before version creation fails, leaving a published definition with no version and failing the main-branch publish workflow.

Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/ravion-modules/src/publish.ts
Line: 238-242

Comment:
**New definitions bypass validation**

When a new module has config accepted locally but rejected by the Ravion API, both dry runs skip remote validation because the definition does not exist yet. The later `--apply` run creates and globally publishes the definition before version creation fails, leaving a published definition with no version and failing the main-branch publish workflow.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

} else {
try {
await client.validateModuleVersion({
moduleDefinitionId: remoteDefinition.id,
version: definition.version,
description: definition.releaseDescription,
config: definition.module,
});
options.logger?.(
`Remote validation passed for ${definition.type}@${definition.version}.`,
);
} catch (error) {
validationErrors.push({
type: definition.type,
version: definition.version,
message: formatUnknownError(error),
});
}
}
}
if (!dryRun) {
if (!remoteDefinition) {
throw new PublishError(
`Cannot create ${definition.type}@${definition.version}; module definition was not created.`,
);
}
if (shouldPublishDefinitionAfterVersion) {
await client.validateModuleVersion({
moduleDefinitionId: remoteDefinition.id,
version: definition.version,
description: definition.releaseDescription,
config: definition.module,
});
}
await createVersionOrConfirmDuplicate(client, remoteDefinition.id, definition);
if (shouldPublishDefinitionAfterVersion) {
remoteDefinition = await client.patchModuleDefinition({
id: remoteDefinition.id,
isGlobalPublished: true,
});
definitionsByType.set(remoteDefinition.type, remoteDefinition);
}
}
}

if (validationErrors.length > 0) {
throw new PublishPlanError(
`Remote validation failed for ${validationErrors.length} module version(s).`,
{ dryRun, items, validationErrors },
);
}

return { dryRun, items };
}

Expand Down Expand Up @@ -295,6 +379,23 @@ export function formatPublishPlanMarkdown(result: PublishResult): string {
"",
];

if (result.validationErrors && result.validationErrors.length > 0) {
lines.push(
"### 🚨 Remote Validation Failures 🚨",
"",
"The Ravion API rejected these module version configs during dry-run validation. Fix the module definition config before merging.",
"",
"| Module | Release Version | Error |",
"| --- | --- | --- |",
);
for (const error of result.validationErrors) {
lines.push(
`| \`${escapeMarkdownTableCell(error.type)}\` | \`${escapeMarkdownTableCell(error.version)}\` | ${escapeMarkdownTableCell(error.message)} |`,
);
}
lines.push("");
}

if (result.errors && result.errors.length > 0) {
lines.push(
"### 🚨 Release Config Conflicts 🚨",
Expand Down Expand Up @@ -771,6 +872,17 @@ class HttpRavionModuleApiClient implements RavionModuleApiClient {
return this.call<RemoteModuleVersion>("POST", "/module-versions", { data: input });
}

async validateModuleVersion(input: ModuleVersionInput): Promise<void> {
const { status } = await this.request("POST", "/module-versions", {
data: { ...input, dryRun: true },
});
if (status !== 202) {
throw new PublishError(
`POST /module-versions dry-run validation returned HTTP ${status} instead of 202. The Ravion API may not support dryRun yet, and the module version may have been created for real.`,
);
}
}

private async list<T>(path: string, query: Record<string, string> = {}): Promise<T[]> {
const items: T[] = [];
let cursor: string | undefined;
Expand All @@ -797,6 +909,16 @@ class HttpRavionModuleApiClient implements RavionModuleApiClient {
query: Record<string, string> = {},
options: { unwrap?: boolean } = {},
): Promise<T> {
const { payload } = await this.request(method, path, input, query);
return (options.unwrap === false ? payload : unwrapApiPayload(payload)) as T;
}

private async request(
method: string,
path: string,
input?: unknown,
query: Record<string, string> = {},
): Promise<{ status: number; payload: unknown }> {
const headers: Record<string, string> = { "Content-Type": "application/json" };
if (this.token) {
headers.Authorization = `Bearer ${this.token}`;
Expand All @@ -823,7 +945,7 @@ class HttpRavionModuleApiClient implements RavionModuleApiClient {
`${method} ${url} failed with HTTP ${response.status}: ${message}\nResponse body:\n${formatResponsePayload(payload)}`,
);
}
return (options.unwrap === false ? payload : unwrapApiPayload(payload)) as T;
return { status: response.status, payload };
}
}

Expand Down
Loading