Skip to content

Commit 680ded0

Browse files
claude[bot]claude
andauthored
fix(cli): parse os package publish manifest ids through PackageSchema.manifestId (#16889)
`packages/cli/src/commands/package/publish.ts` carried its own `MANIFEST_ID_RE` and tested `--manifest-id` (and the derived id) against it, while the contract this repo declares for the column it publishes into is `PackageSchema.manifestId` in `packages/spec/src/cloud/package.zod.ts`. The local copy was looser on every axis, so the preflight admitted what the control plane refuses. - Delete `MANIFEST_ID_RE`. Both paths — the explicit `--manifest-id` / `objectstack.manifest.json` check and the derive path in `deriveManifestId` — now parse through the imported schema. - The derive path's extra `explicit.includes('.')` condition is dropped: the schema subsumes it (its pattern needs at least two segments). That condition is why the two paths disagreed with each other as well as with the declaration. - The refusal text is quoted from the schema's own `invalid_format` issue and its `.describe()`, so it can no longer state a contract that does not exist. - A derived id the schema rejects (`slugify` has no letter-first rule, so an app named `2024 App` derives `local.2024-app`) is refused before any network call, naming where the id came from and how to set one. It is deliberately not normalised: `manifestId` is immutable once published. Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 Co-authored-by: Claude <noreply@anthropic.com>
1 parent f2b5e46 commit 680ded0

3 files changed

Lines changed: 476 additions & 13 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
`os package publish` now decides what a manifest id is by parsing it through `PackageSchema.manifestId` — the schema for the very column it publishes into — instead of testing it against a hand-copied look-alike.
6+
7+
The command carried its own rule (`MANIFEST_ID_RE`, a case-insensitive "starts alphanumeric, then any of a-z 0-9 dot underscore hyphen, up to 255 chars"), which is looser than the declared contract on every axis. The local preflight therefore **admitted what the control plane refuses**: a single segment (`crm`), an underscore (`com.acme.repair_desk`), upper case (`COM.ACME.CRM`), a digit-first segment (`9foo.bar`), an empty segment (`com..acme`) and a trailing dot (`com.acme.`). The preflight passed, the request went out, and the server answered `400`. Its error text, when it did fire, named a contract (`a-z0-9._-`) that does not exist — so a user who followed the message walked into a second refusal.
8+
9+
- **One rule, both paths.** `MANIFEST_ID_RE` is deleted. The explicit `--manifest-id` / `objectstack.manifest.json` path and the derive path (`deriveManifestId`, which adopts `artifact.manifest.id`) now ask the same imported schema. They previously disagreed with each other as well as with the declaration: the derive path additionally required a dot, so a bare `crm` was blocked there and accepted on the explicit path. That extra condition is gone because the schema subsumes it — its pattern requires at least two segments.
10+
- **The refusal text is quoted from the schema**, from its own `invalid_format` issue plus its `.describe()`, so it can no longer drift from the rule it describes.
11+
- **A derived id the schema rejects is refused, not rewritten.** `slugify` has no letter-first rule, so an app named `2024 App` derives `local.2024-app` — digit-first, and rejected. That is now refused before any network call, with a message naming where the id came from and how to set one (`--manifest-id`, `manifestId` in `objectstack.manifest.json`, or `manifest.id`). It is deliberately not normalised into some other id: `manifestId` is immutable once published, and minting a different permanent global identifier than the inputs imply is worse than saying what is wrong.
12+
13+
Publishing is unaffected for every id the control plane accepts — a legal reverse-domain id passes both paths with unchanged bytes. What changes is that the ids the server was going to reject are now refused locally, with the real rule in the message.

‎packages/cli/src/commands/package/publish.ts‎

Lines changed: 108 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,54 @@
2929
import { readFile } from 'node:fs/promises';
3030
import { resolve as resolvePath, basename, dirname, isAbsolute } from 'node:path';
3131
import { Args, Command, Flags } from '@oclif/core';
32+
import { PackageSchema } from '@objectstack/spec/cloud';
3233
import { printHeader, printKV, printSuccess, printError, printStep } from '../../utils/format.js';
3334
import { DEFAULT_CLOUD_URL, tryReadCloudConfig } from '../../utils/cloud-config.js';
3435
import { readErrorMessage } from '../../utils/response-envelope.js';
3536

36-
const MANIFEST_ID_RE = /^[a-z0-9][a-z0-9._-]{0,254}$/i;
37+
/**
38+
* The one rule for a manifest id is `PackageSchema.manifestId` — the schema for
39+
* the very column this command publishes into (`sys_package.manifest_id`).
40+
* Imported, never transcribed: a second, hand-copied rule here is what let this
41+
* preflight admit six shapes the control plane refuses (`com.acme.repair_desk`,
42+
* `COM.ACME.CRM`, `9foo.bar`, `com..acme`, `com.acme.` and — on the explicit
43+
* `--manifest-id` path — a bare `crm`), while telling the user the contract was
44+
* `a-z0-9._-`. `CreatePackageRequestSchema` in `cloud/package.zod.ts` reaches
45+
* for the same declaration the same way.
46+
*
47+
* Read through a function rather than a module-level constant so `PackageSchema`
48+
* stays lazy (`lazySchema`): the first `.shape` access materialises the whole
49+
* package schema, and `os` boots for many commands that never publish.
50+
*/
51+
function manifestIdSchema() {
52+
return PackageSchema.shape.manifestId;
53+
}
54+
55+
/** True when `value` is a manifest id the control plane will accept. */
56+
export function isManifestId(value: string): boolean {
57+
return manifestIdSchema().safeParse(value).success;
58+
}
59+
60+
/**
61+
* Describe why `value` is not a manifest id, **quoting the schema** — its own
62+
* `invalid_format` issue for the rule and its `.describe()` for the shape.
63+
*
64+
* Deliberately not hand-written: the hand-written sentence this replaces
65+
* (`Expected reverse-domain form like 'com.acme.crm' (a-z0-9._-)`) named a
66+
* contract that does not exist, so a user stopped by the CLI would "fix" their
67+
* id into something like `com.acme.repair_desk` — accepted here, refused by the
68+
* server. Following the error message led to a second error.
69+
*
70+
* Returns `undefined` when `value` is valid.
71+
*/
72+
function explainManifestId(value: string): string | undefined {
73+
const schema = manifestIdSchema();
74+
const parsed = schema.safeParse(value);
75+
if (parsed.success) return undefined;
76+
const reasons = parsed.error.issues.map((issue) => issue.message).join('; ');
77+
const expected = schema.description;
78+
return expected ? `${reasons}. Expected: ${expected}` : reasons;
79+
}
3780

3881
/**
3982
* Mirror of `manifest.namespace`'s pattern in `@objectstack/spec`
@@ -52,23 +95,68 @@ function slugify(input: string): string {
5295
.slice(0, 64) || 'app';
5396
}
5497

98+
/** Where a derived manifest id came from — used to make the refusal actionable. */
99+
export type ManifestIdSource = 'artifact-manifest-id' | 'artifact-manifest-name' | 'artifact-filename';
100+
101+
export interface DerivedManifestId {
102+
/** The id the publish would use. NOT guaranteed valid — the caller parses it. */
103+
id: string;
104+
/** Which input produced it. */
105+
source: ManifestIdSource;
106+
}
107+
55108
/**
56109
* Derive a reverse-domain manifest_id when the user hasn't passed --manifest-id.
57110
* Order of precedence:
58-
* 1. artifact.manifest.id (if it looks like a reverse-domain id)
111+
* 1. artifact.manifest.id (only when it is a manifest id the control plane accepts)
59112
* 2. local.<artifact.manifest.name slug>
60113
* 3. local.<artifact filename without extension>
114+
*
115+
* Step 1 is gated by the schema, not by a local look-alike test: `manifest.id`
116+
* is a bare `z.string()` in `ManifestSchema`, so an artifact may carry any
117+
* shape at all, and the previous test forwarded `com.acme.repair_desk` and
118+
* friends unchanged. The old extra `explicit.includes('.')` condition is gone
119+
* because the schema subsumes it — its pattern requires at least two segments,
120+
* so a dotless id can never parse. That is why a bare `crm` was already blocked
121+
* here while the explicit `--manifest-id` path let it through: two paths, two
122+
* strictnesses, neither of them the declared one.
123+
*
124+
* Steps 2 and 3 are the CLI's own invention and are **not** guaranteed valid:
125+
* `slugify` has no letter-first rule, so a manifest named `2024 App` derives
126+
* `local.2024-app`, which the schema rejects. That is refused at the single
127+
* gate in `run()` with the source named, rather than normalised: `manifestId`
128+
* is immutable once published ("renaming a package requires creating a new
129+
* package"), so silently minting a different permanent global identifier than
130+
* the one the inputs imply is worse than saying what is wrong.
61131
*/
62-
function deriveManifestId(artifact: any, artifactPath: string): string {
132+
export function deriveManifestId(artifact: any, artifactPath: string): DerivedManifestId {
63133
const explicit = artifact?.manifest?.id;
64-
if (typeof explicit === 'string' && MANIFEST_ID_RE.test(explicit) && explicit.includes('.')) {
65-
return explicit;
134+
if (typeof explicit === 'string' && isManifestId(explicit)) {
135+
return { id: explicit, source: 'artifact-manifest-id' };
66136
}
67137
const name = artifact?.manifest?.name;
68138
if (typeof name === 'string' && name.trim()) {
69-
return `local.${slugify(name)}`;
139+
return { id: `local.${slugify(name)}`, source: 'artifact-manifest-name' };
140+
}
141+
return {
142+
id: `local.${slugify(basename(artifactPath).replace(/\.json$/i, ''))}`,
143+
source: 'artifact-filename',
144+
};
145+
}
146+
147+
/** The remedy line for a refused manifest id, by where the id came from. */
148+
function manifestIdRemedy(source: ManifestIdSource | 'explicit'): string {
149+
switch (source) {
150+
case 'artifact-manifest-id':
151+
case 'artifact-manifest-name':
152+
return 'It was derived from the compiled artifact. Pass --manifest-id, set `manifestId` in '
153+
+ 'objectstack.manifest.json, or fix `manifest.id` in objectstack.config.ts and rebuild.';
154+
case 'artifact-filename':
155+
return 'It was derived from the artifact filename. Pass --manifest-id, set `manifestId` in '
156+
+ 'objectstack.manifest.json, or give the app a `manifest.name` and rebuild.';
157+
default:
158+
return 'Pass a --manifest-id the control plane accepts.';
70159
}
71-
return `local.${slugify(basename(artifactPath).replace(/\.json$/i, ''))}`;
72160
}
73161

74162
/**
@@ -322,14 +410,21 @@ export default class PackagePublish extends Command {
322410
const m = tplManifest?.data ?? {};
323411
const baseDir = tplManifest?.baseDir ?? process.cwd();
324412

325-
const manifestId = (
413+
// One gate for both paths — the explicitly supplied id and the derived one
414+
// are parsed by the same schema the control plane parses `manifest_id`
415+
// with. Keeping a looser local copy "to fail early" is what this command
416+
// used to do, and it did not fail early: it PASSED early, and the server
417+
// answered 400.
418+
const supplied =
326419
flags['manifest-id']
327-
?? (typeof m.manifestId === 'string' ? m.manifestId : undefined)
328-
?? deriveManifestId(artifact, artifactPath)
329-
).trim();
330-
if (!MANIFEST_ID_RE.test(manifestId)) {
420+
?? (typeof m.manifestId === 'string' ? m.manifestId : undefined);
421+
const derived = supplied === undefined ? deriveManifestId(artifact, artifactPath) : undefined;
422+
const manifestId = (supplied ?? derived!.id).trim();
423+
const manifestIdProblem = explainManifestId(manifestId);
424+
if (manifestIdProblem !== undefined) {
331425
printError(
332-
`Invalid manifest-id '${manifestId}'. Expected reverse-domain form like 'com.acme.crm' (a-z0-9._-).`,
426+
`Invalid manifest-id '${manifestId}'. ${manifestIdProblem}. `
427+
+ manifestIdRemedy(derived?.source ?? 'explicit'),
333428
);
334429
this.exit(1);
335430
return;

0 commit comments

Comments
 (0)