Skip to content

Commit cce45f8

Browse files
dmealingclaude
andcommitted
fix(cli): the base-URL scanner counted brackets, and one door ignored the key
Independent review of this session's own work. Four defects, all in code added hours earlier. **The scanner followed directory symlinks with no visited set**, under a comment claiming it "never re-enters one". `src/loop -> <root>` produced 41 findings for ONE file, terminating only because Linux's 40-symlink ELOOP limit made readdirSync throw into the catch. A link pointing outside the project — a monorepo sibling, a linked package — got walked on every `meta verify`. It no longer follows them, matching the sibling scanner: a provider reachable through a link is reachable through its real path, so the duplicates were pure inflation of the count the header prints. **A `>` inside a string or comment both false-positived and false-negatived.** Counting brackets is what the first version did, and it was wrong in both directions: <EntityFetcherProvider title="a > b" fetcher={f} baseUrl="/api"> → the `>` in the STRING ended the tag before baseUrl: a nag on a correct provider. <EntityFetcherProvider fetcher={mk(")")}> → the unbalanced `)` in a string meant depth never returned to zero, so the 4000-char cap swallowed an unrelated `baseUrl` later in the file and SUPPRESSED a real finding. String literals (all three quote styles, with escapes), line comments and block comments are now skipped rather than counted. A regex literal is deliberately not handled and says so. **The verify wiring was gated by nothing** — `git grep "missing-base-url" -- '*test*'` returned empty. Nothing asserted that verify calls the scanner, that the header prints, that the rule reaches the structured payload, or that suppression works. A scanner nobody wired is the same as no scanner, and the migration note calls this "the only gate that can see this half", so the wiring IS the claim. Proved by unwiring it. **`includeUiTier` was a config key only one of the two doors honoured.** It is settable (`MetaobjectsGenConfig extends Omit<ResolvedGenConfig, …>`) and is the escape hatch for an owned generator whose name was changed — and `meta docs` read it while `runGen` computed the aggregation unconditionally. So that adopter got `agent/ui.md` from one command and not the other: exactly the two-doors defect the marker exists to close, copied from `includeHonoRoutes`, which had it too. Both now prefer the explicit key at both doors, including an explicit `false` over a suite that has a UI generator. Smaller, same review: the replay remedy named `meta migrate baseline --from-db` without the `--db <url>` it requires — a remedy you cannot run as typed, one scale down from the defect that commit fixed. And it identified the engine's error with a cross-package `instanceof`, the mechanism that split ts-poet's `Code` in 0.21.6; it now checks the name and the position fields, gated with a foreign-realm error that `instanceof` refuses. Docs: the migration note promised the advisory with no caveat, when `--no-antipatterns` turns it off along with the modeling nags. And the changelog dismissed the estate's "21 entries became 6" report as unreproducible — it is real on the corrupt-read path (an unreadable manifest reads as ABSENT, so the next gen writes only its own entries and the rest are refused forever), which is now recorded as its own open defect with the decision it needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTcEKXTQMYt84fAjuw5A2M
1 parent 84bcba7 commit cce45f8

13 files changed

Lines changed: 336 additions & 49 deletions

‎CHANGELOG.md‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,14 @@ would have stayed green throughout; the positive direction is now gated too. Mig
178178
[`0.x-to-1.0.md` §13](docs/features/migrations/0.x-to-1.0.md).
179179

180180
*(The report that found this also claimed `_record` replaces the manifest wholesale — 21
181-
entries becoming 6. Not reproducible from the code: `_record` loads and adds. Only the keying
182-
was wrong.)*
181+
entries becoming 6. `_record` loads and adds — **when the load succeeds.** An unreadable or
182+
corrupt manifest reads as ABSENT (fail-closed, so every file refuses rather than being assumed
183+
ours), and the next `gen` then writes only its own entries: a 68-entry manifest becomes 5, and
184+
the 63 lost files are unrecorded and refused forever. So the observation was real and its
185+
mechanism is the corrupt-read path, not the keying. Reported here as a separate defect rather
186+
than folded in, because the fix is a decision — a corrupt manifest could refuse to proceed
187+
instead of being silently rewritten, which is what fail-closed would mean at the FILE level
188+
rather than the entry level.)*
183189

184190
### Fixed — a hand-edited `<Entity>Names` read as "in sync with the metadata"
185191

‎docs/features/migrations/api-base-url-leaves-the-entity-descriptor.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,10 @@ drops the prefix from every generated hook:
6767
a provider mounted with no `baseUrl`. It is a warning, never an exit code, and it is the
6868
only gate that can see this half.
6969

70+
It rides in the same advisory pass as the hand-rolled-modeling nags, so **`--no-antipatterns`
71+
(or `META_NO_ANTIPATTERNS=1`) turns it off with them.** If you suppress that pass, this is
72+
what you are also suppressing.
73+
7074
If your own code read `<Entity>.$apiPrefix`, it no longer compiles. Take the base from
7175
the same config your provider does.
7276

‎server/typescript/packages/cli/src/commands/init.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import { log } from "../lib/log.js";
1515
import { cliVersion } from "../lib/version.js";
1616
import { declaredDependencyNames, type PackageManifest } from "../lib/package-manifest.js";
1717
import { findWranglerConfig, parseWranglerConfig } from "@metaobjectsdev/migrate-ts";
18-
import { readReferenceTemplate, type ReferenceGeneratorName } from "@metaobjectsdev/codegen-ts";
18+
import { DEFAULT_DOCS_DIR, readReferenceTemplate, type ReferenceGeneratorName } from "@metaobjectsdev/codegen-ts";
1919

2020
// ADR-0034 scaffold-and-own — `meta init` copies the codegen reference templates into
2121
// the consumer's repo so they OWN them; metaobjects.config.ts imports them locally.
@@ -139,7 +139,7 @@ export default defineConfig({
139139
barrel(),
140140
],
141141
docs: {
142-
outDir: "./docs/generated", // every surface lands here (run: meta docs).
142+
outDir: "${DEFAULT_DOCS_DIR}", // every surface lands here (run: meta docs).
143143
// A SUB-directory on purpose: these pages are regenerated
144144
// and overwritten, and docs/ itself is usually yours.
145145
layout: "flat", // or "package" for multi-package models

‎server/typescript/packages/cli/src/lib/base-url-advisory.ts‎

Lines changed: 52 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { readdirSync, readFileSync, statSync } from "node:fs";
1+
import { readdirSync, readFileSync } from "node:fs";
22
import { join, relative, sep } from "node:path";
33

44
/**
@@ -50,19 +50,25 @@ const PROVIDERS: readonly { construct: string; open: RegExp; kind: "jsx" | "call
5050
{ construct: "provideEntityFetcher()", open: /provideEntityFetcher\s*\(/g, kind: "call" },
5151
];
5252

53-
/** Nesting introduced by `c`: +1 for an opener, -1 for a closer, 0 otherwise. */
54-
function nesting(c: string): number {
55-
if (c === "{" || c === "(" || c === "[") return 1;
56-
if (c === "}" || c === ")" || c === "]") return -1;
57-
return 0;
58-
}
59-
6053
/**
6154
* The text of the opening construct that begins at `start`.
6255
*
63-
* JSX: up to the `>` that closes the opening TAG — tracked by nesting depth, because
64-
* `onSomething={() => x}` puts a `>` inside an attribute and a naive first-`>` search
65-
* would cut the tag in half and miss a `baseUrl` written after it.
56+
* JSX: up to the `>` that closes the opening TAG. Call this a one-file tokenizer rather
57+
* than a bracket count, because a bracket count is what the first version was and it was
58+
* wrong in both directions:
59+
*
60+
* <EntityFetcherProvider title="a > b" fetcher={f} baseUrl="/api">
61+
* → the `>` inside the STRING ended the tag before `baseUrl`, warning about a
62+
* correct provider.
63+
* <EntityFetcherProvider fetcher={mk(")")}>
64+
* → the unbalanced `)` inside a string meant depth never returned to 0, so the
65+
* 4000-char cap swallowed an unrelated `baseUrl` later in the file and SUPPRESSED
66+
* a real finding.
67+
*
68+
* So string literals (all three quote styles, with escapes), line comments and block
69+
* comments are skipped rather than counted. A regex literal is deliberately NOT handled —
70+
* telling `/` division from a regex needs real parsing, and a regex inside a JSX opening
71+
* tag is vanishingly rare next to the cost of getting it wrong.
6672
*
6773
* Call: to the `)` balancing the argument list, so a nested object or arrow does not end
6874
* it early.
@@ -75,9 +81,31 @@ function constructText(src: string, start: number, kind: "jsx" | "call"): string
7581
let depth = 0;
7682
for (let i = start; i < limit; i++) {
7783
const c = src[i]!;
84+
85+
// --- skip what is not code -------------------------------------------------
86+
if (c === '"' || c === "'" || c === "`") {
87+
i++;
88+
while (i < limit && src[i] !== c) { if (src[i] === "\\") i++; i++; }
89+
continue;
90+
}
91+
if (c === "/" && src[i + 1] === "/") {
92+
while (i < limit && src[i] !== "\n") i++;
93+
continue;
94+
}
95+
if (c === "/" && src[i + 1] === "*") {
96+
i += 2;
97+
while (i < limit && !(src[i] === "*" && src[i + 1] === "/")) i++;
98+
i++;
99+
continue;
100+
}
101+
102+
// --- code ------------------------------------------------------------------
78103
if (kind === "jsx" && c === ">" && depth === 0) return src.slice(start, i + 1);
79-
depth += nesting(c);
80-
if (kind === "call" && depth === 0 && c === ")") return src.slice(start, i + 1);
104+
if (c === "{" || c === "(" || c === "[") depth++;
105+
else if (c === "}" || c === ")" || c === "]") {
106+
if (kind === "call" && depth === 1 && c === ")") return src.slice(start, i + 1);
107+
depth--;
108+
}
81109
}
82110
return src.slice(start, limit);
83111
}
@@ -109,15 +137,19 @@ function walk(dir: string, root: string, acc: BaseUrlFinding[], apiPrefix: strin
109137
let entries;
110138
try { entries = readdirSync(dir, { withFileTypes: true }); } catch { return; }
111139
for (const e of entries) {
112-
if (e.name.startsWith(".") && e.name !== ".") { if (IGNORE_SEGMENTS.has(e.name)) continue; }
113140
if (IGNORE_SEGMENTS.has(e.name)) continue;
114141
const abs = join(dir, e.name);
115-
// Follow directories through symlinks the way the anti-pattern scan does not need to,
116-
// but never re-enter one: statSync on a broken link throws, which is caught here.
117-
let isDir: boolean;
118-
try { isDir = e.isDirectory() || (e.isSymbolicLink() && statSync(abs).isDirectory()); }
119-
catch { continue; }
120-
if (isDir) { walk(abs, root, acc, apiPrefix); continue; }
142+
// Symlinked directories are NOT followed — same as the anti-pattern scan beside it.
143+
//
144+
// The first version did follow them, with a comment claiming it "never re-enters
145+
// one". There was no visited set, so `src/loop -> <root>` produced 41 findings for a
146+
// single file (`src/loop/src/loop/…`), terminating only because Linux's 40-symlink
147+
// ELOOP limit made readdirSync throw into the catch below. It also meant a link
148+
// pointing outside the project — a monorepo sibling, a linked package — got walked on
149+
// every `meta verify`. A provider reachable only through a symlink is reachable
150+
// through its real path too, so following them buys nothing and the duplicates
151+
// inflate the count the header prints.
152+
if (e.isDirectory()) { walk(abs, root, acc, apiPrefix); continue; }
121153
if (!SCAN_EXT.test(e.name) || SKIP_FILE.test(e.name)) continue;
122154
let src: string;
123155
try { src = readFileSync(abs, "utf8"); } catch { continue; }

‎server/typescript/packages/cli/src/lib/replay-remedy.ts‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,26 @@ import { MigrationApplyError } from "@metaobjectsdev/migrate-ts";
2828
* (a checksum guard on databases that already ran the file), not as a blanket ban, because
2929
* a migration no database has applied yet is edited in place every day.
3030
*/
31+
/**
32+
* Is this the migrate engine's own apply failure?
33+
*
34+
* By NAME, not `instanceof`. Two physical copies of `@metaobjectsdev/migrate-ts` in one
35+
* process — a globally-installed or linked `meta` alongside a project-local dependency —
36+
* give the class object and the instance different identities, so `instanceof` returns
37+
* false for a real error and the remedy silently degrades to its un-positioned form. That
38+
* is the same class-identity defect that split ts-poet's `Code` in 0.21.6 and drove the
39+
* exported node guards in `metadata`. Unreachable through today's single import; costs one
40+
* line to make unreachable by construction.
41+
*/
42+
function isApplyError(err: unknown): err is MigrationApplyError {
43+
return err instanceof MigrationApplyError
44+
|| (err instanceof Error && err.name === "MigrationApplyError"
45+
&& typeof (err as MigrationApplyError).migration === "string"
46+
&& typeof (err as MigrationApplyError).index === "number");
47+
}
48+
3149
export function replayRemedy(err: unknown): string[] {
32-
const applyErr = err instanceof MigrationApplyError ? err : undefined;
50+
const applyErr = isApplyError(err) ? err : undefined;
3351
const head = applyErr !== undefined && applyErr.index === 0;
3452
const which =
3553
applyErr === undefined
@@ -54,7 +72,7 @@ export function replayRemedy(err: unknown): string[] {
5472
"for this project, --replay asserts a property this chain does not have — don't wire " +
5573
"it. Otherwise correct the migration in place if no database has applied it yet " +
5674
"(the ledger's checksum guard is what stops you once one has), or re-baseline from a " +
57-
"live database with `meta migrate baseline --from-db`.",
75+
"live database with `meta migrate baseline --from-db --db <url>`.",
5876
);
5977
return out;
6078
}

‎server/typescript/packages/cli/test/advisory-structured-output.test.ts‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,3 +533,61 @@ describe("verify.antiPatternIgnore reaches the scan from metaobjects.config.ts",
533533
}
534534
});
535535
});
536+
537+
// The base-URL advisory END TO END. `base-url-advisory.test.ts` covers the scanner in
538+
// isolation; nothing asserted that `verify` calls it, that the header line prints, that
539+
// `rule: "missing-base-url"` reaches the structured payload, or that suppression works.
540+
// A scanner nobody wired is the same as no scanner — and the migration note calls this
541+
// "the only gate that can see this half", so the wiring is the claim.
542+
describe("verify wires the base-URL advisory", () => {
543+
const PROVIDER = `<EntityFetcherProvider fetcher={fetcher}>\n <App />\n</EntityFetcherProvider>\n`;
544+
545+
function withProvider(apiPrefix: string): string {
546+
const dir = project(`\n apiPrefix: ${JSON.stringify(apiPrefix)},`);
547+
writeFileSync(join(dir, "src", "main.tsx"), PROVIDER);
548+
return dir;
549+
}
550+
551+
test("a non-empty apiPrefix + a provider with no baseUrl reaches the payload", async () => {
552+
const dir = withProvider("/api");
553+
try {
554+
const { exit, out } = await capture(["verify", "--cwd", dir, "--format", "json"]);
555+
const payload = JSON.parse(out) as { antiPatterns: AdvisoryBlock };
556+
const rows = payload.antiPatterns.rows.filter((r) => r.rule === "missing-base-url");
557+
expect(rows).toHaveLength(1);
558+
expect(rows[0]!.file).toBe("src/main.tsx");
559+
expect(rows[0]!.message).toContain("/api");
560+
// Advisory only: it must never move the exit code. The money-float findings the
561+
// shared fixture carries are advisory too, so a clean project here exits 0.
562+
expect(exit).toBe(0);
563+
} finally { rmSync(dir, { recursive: true, force: true }); }
564+
});
565+
566+
test("the text header names the count and the prefix", async () => {
567+
const dir = withProvider("/api");
568+
try {
569+
const { err } = await capture(["verify", "--cwd", dir]);
570+
expect(err).toContain("entity-fetcher provider(s) mounted with no baseUrl");
571+
expect(err).toContain(`apiPrefix is "/api"`);
572+
} finally { rmSync(dir, { recursive: true, force: true }); }
573+
});
574+
575+
test("an empty apiPrefix emits nothing — the meta init scaffold is not nagged", async () => {
576+
const dir = withProvider("");
577+
try {
578+
const { out } = await capture(["verify", "--cwd", dir, "--format", "json"]);
579+
const payload = JSON.parse(out) as { antiPatterns: AdvisoryBlock };
580+
expect(payload.antiPatterns.rows.filter((r) => r.rule === "missing-base-url")).toEqual([]);
581+
} finally { rmSync(dir, { recursive: true, force: true }); }
582+
});
583+
584+
test("--no-antipatterns suppresses it, which is why the migration note says so", async () => {
585+
const dir = withProvider("/api");
586+
try {
587+
const { out } = await capture(["verify", "--cwd", dir, "--format", "json", "--no-antipatterns"]);
588+
const payload = JSON.parse(out) as { antiPatterns: AdvisoryBlock };
589+
expect(payload.antiPatterns.status).not.toBe("ran");
590+
expect(payload.antiPatterns.rows).toEqual([]);
591+
} finally { rmSync(dir, { recursive: true, force: true }); }
592+
});
593+
});

‎server/typescript/packages/cli/test/base-url-advisory.test.ts‎

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
// run by hand was the only signal in the building, and it cannot see this half at all.
1010

1111
import { describe, test, expect, afterAll } from "bun:test";
12-
import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises";
12+
import { mkdtemp, mkdir, writeFile, rm, symlink } from "node:fs/promises";
1313
import { tmpdir } from "node:os";
1414
import { join } from "node:path";
1515
import { scanForMissingBaseUrl } from "../src/lib/base-url-advisory.js";
@@ -100,3 +100,61 @@ describe("scanForMissingBaseUrl", () => {
100100
expect(scanForMissingBaseUrl(root, "/api")).toEqual([]);
101101
});
102102
});
103+
104+
// The three cases a bracket count got wrong. Each was reproduced against the module
105+
// before the tokenizer replaced the count, and each is a way an advisory can be worse
106+
// than absent: two nag a correct provider, one suppresses a real finding.
107+
describe("scanForMissingBaseUrl — what is code and what is not", () => {
108+
test("a `>` inside a STRING attribute does not end the tag", async () => {
109+
const root = await project({
110+
"src/main.tsx": `<EntityFetcherProvider title="a > b" fetcher={fetcher} baseUrl="/api">`,
111+
});
112+
expect(scanForMissingBaseUrl(root, "/api")).toEqual([]);
113+
});
114+
115+
test("a `//` comment between attributes does not end the tag", async () => {
116+
const root = await project({
117+
"src/main.tsx": [
118+
`<EntityFetcherProvider`,
119+
` // an arrow => lives here, and a > with it`,
120+
` fetcher={fetcher}`,
121+
` baseUrl="/api"`,
122+
`>`,
123+
].join("\n"),
124+
});
125+
expect(scanForMissingBaseUrl(root, "/api")).toEqual([]);
126+
});
127+
128+
test("a block comment between attributes does not end the tag", async () => {
129+
const root = await project({
130+
"src/main.tsx": `<EntityFetcherProvider /* > */ fetcher={f} baseUrl="/api">`,
131+
});
132+
expect(scanForMissingBaseUrl(root, "/api")).toEqual([]);
133+
});
134+
135+
test("an unbalanced `)` inside a string does not SUPPRESS a real finding", async () => {
136+
// The dangerous direction. Counting brackets, the `)` in the string left depth
137+
// permanently above zero, so the scan ran to its 4000-char cap and found the
138+
// unrelated `baseUrl` below — reporting clean over a provider that has none.
139+
const root = await project({
140+
"src/main.tsx": [
141+
`<EntityFetcherProvider fetcher={mk(")")}>`,
142+
` <App />`,
143+
`</EntityFetcherProvider>`,
144+
``,
145+
`const elsewhere = { baseUrl: "/api" };`,
146+
].join("\n"),
147+
});
148+
expect(scanForMissingBaseUrl(root, "/api")).toHaveLength(1);
149+
});
150+
151+
test("a symlinked directory is not followed", async () => {
152+
// Following them had no visited set: `src/loop -> <root>` produced 41 findings for
153+
// ONE file and stopped only at Linux's ELOOP limit. A provider reachable through a
154+
// link is reachable through its real path, so the duplicates were pure inflation of
155+
// the count `verify` prints — and a link out of the project walked foreign trees.
156+
const root = await project({ "src/main.tsx": `<EntityFetcherProvider fetcher={f}>` });
157+
await symlink(root, join(root, "src", "loop"), "dir");
158+
expect(scanForMissingBaseUrl(root, "/api")).toHaveLength(1);
159+
});
160+
});

0 commit comments

Comments
 (0)