Skip to content

Commit e960e37

Browse files
committed
fix: review findings on owned-copy tooling
eject --undo: Python de-duplicates names, TS refuses a never-ejected name like the other ports. gen: the TS stock-copy notice cannot abort a run, the strict hint prints only for ERR_UNKNOWN_ATTR, C# gen --strict has its own hint and loads once, the C# notice catches any failure. init --with-docs says so when metaobjects.config.ts already exists. Python apiPrefix is limited to path characters so it cannot break the generated router.
1 parent cbce00b commit e960e37

12 files changed

Lines changed: 81 additions & 21 deletions

File tree

‎server/csharp/MetaObjects.Cli.Tests/GenUnknownAttrWarningTests.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ public void Strict_gen_rejects_an_unknown_attribute_before_writing_anything()
5555
var run = OwnedRunner.Run(_tmp, ["entity"], "gen", MetaDir, "--out", OutDir, "--namespace", "Acme", "--strict");
5656
Assert.Equal(1, run.ExitCode);
5757
Assert.Contains("ERR_UNKNOWN_ATTR", run.Stderr);
58-
Assert.Contains("hint: " + VerifyCommand.UNKNOWN_ATTR_HINT, run.Stderr);
58+
Assert.Contains("hint: " + CodegenCli.GenStrictHint, run.Stderr);
59+
Assert.DoesNotContain("verify --lax", run.Stderr);
5960
Assert.False(Directory.Exists(OutDir) && Directory.EnumerateFiles(OutDir, "*", SearchOption.AllDirectories).Any());
6061
}
6162

‎server/csharp/MetaObjects.Cli/Program.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ static int RunGen(string[] rest)
144144
// Advisory: owned copies identical to the shipped generators carry nothing of the adopter's.
145145
// One line on stderr, free when there are none; never changes the exit code.
146146
try { if (OwnedCopy.IdenticalNotice(projectRoot) is { } notice) Console.Error.WriteLine(notice); }
147-
catch (Exception e) when (e is IOException or UnauthorizedAccessException) { }
147+
catch (Exception) { /* advisory only: never fail a gen over the notice */ }
148148

149149
// Advisory: nudge a re-scaffold if the copied-in agent context predates this build.
150150
// Never throws, never changes the exit code (a missing/corrupt manifest is ignored).

‎server/csharp/MetaObjects.Codegen/CodegenCli.cs‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,14 @@ public static void ReportFailure(GenOutcome outcome, string command)
113113
"arbitrary author-supplied properties into an `attr.properties` bag, OR run " +
114114
"'dotnet meta verify --lax' to keep the legacy open-attr load.";
115115

116+
/// <summary>The hint <c>gen --strict</c> prints after an ERR_UNKNOWN_ATTR: <c>gen</c>'s own way out is
117+
/// to drop the flag, not <c>verify --lax</c> (ADR-0023).</summary>
118+
public const string GenStrictHint =
119+
"gen --strict loads strictly, as verify does (ADR-0023): an unregistered or typo'd @attr is " +
120+
"ERR_UNKNOWN_ATTR and nothing was written. To fix: register the attr on a metadata provider, " +
121+
"OR move arbitrary author-supplied properties into an `attr.properties` bag, " +
122+
"OR drop --strict.";
123+
116124
/// <summary>Trailing advice after <c>gen</c>'s unknown-attribute warnings. Without <c>--strict</c>
117125
/// <c>gen</c> loads leniently; <c>gen --strict</c> and <c>verify</c> are the strict doors (ADR-0023).</summary>
118126
public const string UnknownAttrGenAdvice =
@@ -735,20 +743,25 @@ private static int RunGenArgs(string[] rest, IReadOnlyList<IGenerator> declared)
735743

736744
// ADR-0023: gen loads leniently; name what `verify` would reject instead of staying silent.
737745
// With --strict it loads as verify does and refuses before any file is written.
746+
LoadResult genLoad;
738747
if (a.Strict)
739748
{
740-
var strictLoad = meta.Load(strict: true);
741-
if (strictLoad.Errors.Count > 0)
749+
genLoad = meta.Load(strict: true);
750+
if (genLoad.Errors.Count > 0)
742751
{
743-
foreach (var e in strictLoad.Errors) Console.Error.WriteLine($" load error: {e.Code}: {e.Message}");
744-
if (strictLoad.Errors.Any(e => e.Code == ErrorCode.ERR_UNKNOWN_ATTR))
745-
Console.Error.WriteLine($" hint: {UnknownAttrHint}");
752+
foreach (var e in genLoad.Errors) Console.Error.WriteLine($" load error: {e.Code}: {e.Message}");
753+
if (genLoad.Errors.Any(e => e.Code == ErrorCode.ERR_UNKNOWN_ATTR))
754+
Console.Error.WriteLine($" hint: {GenStrictHint}");
746755
Console.Error.WriteLine($"{GenCommandName}: FAILED (metadata did not load cleanly; --strict)");
747756
return 1;
748757
}
749758
}
750-
else WarnUnknownAttrs(meta);
751-
var outcome = RunGen(meta.Load(), a.OutDir, a.Namespace, a.EmitAbstractShapes, suite, projectRoot, a.Baseline,
759+
else
760+
{
761+
WarnUnknownAttrs(meta);
762+
genLoad = meta.Load();
763+
}
764+
var outcome = RunGen(genLoad, a.OutDir, a.Namespace, a.EmitAbstractShapes, suite, projectRoot, a.Baseline,
752765
includeNames: declared.Any(g => g.Name == "names"),
753766
// Only a run of the whole declared list can tell a stale file from one the
754767
// narrowing left out.

‎server/python/src/metaobjects/codegen/eject.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ class EjectResult:
125125

126126
def eject(names: list[str], root: Path, *, force: bool = False) -> EjectResult:
127127
"""Copy each named generator. Every name is checked before any file is written."""
128+
names = list(dict.fromkeys(names))
128129
known = {e.name: e for e in ejectable()}
129130
unknown = [n for n in names if n not in known]
130131
if unknown:
@@ -195,6 +196,7 @@ def undo(names: list[str], root: Path, *, force: bool = False) -> EjectResult:
195196
removed. The config is never edited: the entry that points back at the shipped generator
196197
is printed instead. A companion runtime copy goes only when it matches the package and
197198
no remaining owned generator reads it."""
199+
names = list(dict.fromkeys(names))
198200
known = {e.name: e for e in ejectable()}
199201
unknown = [n for n in names if n not in known]
200202
if unknown:

‎server/python/src/metaobjects/codegen/project_config.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,10 @@
4444
#: followed the rc.3 guide and added it gets the naming-migration message, not "unknown key".
4545
_REMOVED_COLUMN_NAMING_KEY = "columnNaming"
4646

47+
#: A mount path the generated router can carry verbatim inside a string literal: a leading
48+
#: ``/`` then path characters only, so a quote or backslash cannot break the emitted source.
49+
_API_PREFIX_RE = re.compile(r"/[A-Za-z0-9._/-]*")
50+
4751
#: Every key this loader reads at the top level, and the ONLY ones it accepts.
4852
#: ``metaobjects-config.schema.json`` declares ``additionalProperties: false``, so the
4953
#: shipped schema has always said an unknown key is invalid; naming the set here is what
@@ -465,10 +469,11 @@ def load_project_config(path: Path) -> ProjectConfig:
465469
raise ConfigError(f"{path}: 'templates' must be a non-empty string (a directory path).")
466470

467471
api_prefix = raw.get("apiPrefix", DEFAULT_API_PREFIX)
468-
if not isinstance(api_prefix, str) or (api_prefix and not api_prefix.startswith("/")):
472+
if not isinstance(api_prefix, str) or (api_prefix and not _API_PREFIX_RE.fullmatch(api_prefix)):
469473
raise ConfigError(
470474
f"{path}: 'apiPrefix' must be \"\" (mount at the root) or a path starting with "
471-
f"'/', e.g. \"/api\" or \"/v1\"; got {api_prefix!r}."
475+
f"'/' made of letters, digits, '.', '_', '-' and '/', e.g. \"/api\" or \"/v1\"; "
476+
f"got {api_prefix!r}."
472477
)
473478
api_prefix = api_prefix.rstrip("/")
474479

‎server/python/tests/codegen/test_cli_init.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,12 @@ def test_api_prefix_must_be_empty_or_a_path(tmp_path: Path, capsys) -> None:
5858
cfg.write_text(cfg.read_text() + "apiPrefix: v1\n")
5959
assert main(["gen", "--config", str(cfg)]) != 0
6060
assert "apiPrefix" in capsys.readouterr().err
61+
62+
63+
def test_api_prefix_with_a_quote_or_backslash_is_refused(tmp_path: Path, capsys) -> None:
64+
# The prefix is written into the generated router's source, so it may only hold path characters.
65+
assert main(["init", "--root", str(tmp_path)]) == 0
66+
cfg = tmp_path / "metaobjects.config.yaml"
67+
cfg.write_text(cfg.read_text() + "apiPrefix: '/v1\"x'\n")
68+
assert main(["gen", "--config", str(cfg)]) != 0
69+
assert "apiPrefix" in capsys.readouterr().err

‎server/python/tests/codegen/test_eject_undo.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,3 +135,11 @@ def test_gen_without_owned_copies_prints_no_notice(tmp_path: Path, capsys) -> No
135135
cfg = _gen_cfg(tmp_path, "entity")
136136
assert main(["gen", "--config", str(cfg)]) == 0
137137
assert "identical to the shipped" not in capsys.readouterr().err
138+
139+
140+
def test_undo_with_a_repeated_name_removes_it_once(tmp_path: Path, monkeypatch, capsys) -> None:
141+
monkeypatch.chdir(tmp_path)
142+
assert main(["eject", "entity"]) == 0
143+
capsys.readouterr()
144+
assert main(["eject", "--undo", "entity", "entity"]) == 0
145+
assert not (tmp_path / GEN / "entity.py").exists()

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

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -684,10 +684,13 @@ async function undoCommand(names: readonly string[], force: boolean, cwd: string
684684
const named = names.length > 0;
685685
const targets = named ? owned.filter((r) => names.includes(r.name)) : owned.filter(isStockRow);
686686
const kept: UndoKept[] = [];
687-
for (const n of names) {
688-
if (!owned.some((r) => r.name === n)) {
689-
kept.push({ name: n, path: `${OWNED_GENERATORS_DIR}/${n}.ts`, reason: "not ejected — nothing to undo" });
690-
}
687+
const notOwned = names.filter((n) => !owned.some((r) => r.name === n));
688+
if (notOwned.length > 0) {
689+
log.error(
690+
`not an owned copy (nothing at ${OWNED_GENERATORS_DIR}/): ${notOwned.join(", ")}. Nothing was removed. ` +
691+
"Run `meta eject --list` to see what you own.",
692+
);
693+
return 2;
691694
}
692695
const refused = named && !force ? targets.filter((r) => !isStockRow(r)) : [];
693696
if (refused.length > 0) {

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import type { WriteStatus } from "@metaobjectsdev/codegen-ts";
2121
import { packageOfResolutionKey } from "@metaobjectsdev/metadata";
2222
import type { MetaRoot } from "@metaobjectsdev/metadata";
2323
import type { Collection } from "@metaobjectsdev/sdk";
24-
import { reportLoadError } from "../lib/load-error.js";
24+
import { describeLoadError, reportLoadError } from "../lib/load-error.js";
2525
import { STRICT_GEN_HINT, UNKNOWN_ATTR_GEN_ADVICE, unknownAttrWarnings } from "../lib/unknown-attr-warnings.js";
2626
import { stockCopyNotice } from "./eject.js";
2727
import {
@@ -146,7 +146,7 @@ export async function genCommand(args: string[], cwd: string, fmt: OutputFormat
146146
advise(warnIfManifestIgnored(projectRoot));
147147
// Owned generator copies that are the shipped reference, unchanged: they miss every later fix.
148148
// Says nothing when the project owns none, and never affects the exit code.
149-
const stockNotice = await stockCopyNotice(projectRoot);
149+
const stockNotice = await stockCopyNotice(projectRoot).catch(() => undefined);
150150
if (stockNotice !== undefined) { log.warn(stockNotice); advise(stockNotice); }
151151

152152
// ADR-0034 Amendment 9: the config IS the list. With none there is nothing to generate,
@@ -196,7 +196,11 @@ export async function genCommand(args: string[], cwd: string, fmt: OutputFormat
196196
metadata = await loadMemory(genCollection.configDir, flags.strict ? { ...loadOptions, strict: true } : loadOptions);
197197
} catch (err) {
198198
reportLoadError(log, "failed to load metadata", err);
199-
if (flags.strict) log.error(STRICT_GEN_HINT);
199+
// The hint explains the strict-only rejection (an undeclared attribute); any other load
200+
// failure is not something `--strict` caused, so it gets no hint.
201+
if (flags.strict && describeLoadError(err).code === "ERR_UNKNOWN_ATTR") {
202+
log.error(STRICT_GEN_HINT);
203+
}
200204
return 1;
201205
}
202206

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,6 +695,11 @@ export async function init(opts: InitOptions): Promise<InitResult> {
695695
if (!(await fileExists(forgeConfigPath))) {
696696
await writeFile(forgeConfigPath, buildMetaobjectsConfigBody(opts.d1 ? "d1" : "sqlite", opts.withDocs === true), "utf8");
697697
result.created.push("metaobjects.config.ts");
698+
} else if (opts.withDocs === true) {
699+
result.warnings.push(
700+
"--with-docs: metaobjects.config.ts already exists and was left alone. To write the docs, add " +
701+
"`docsFile()` and `apiDocsFile()` from @metaobjectsdev/codegen-ts/generators to its `generators` list.",
702+
);
698703
}
699704

700705
// Scaffold a minimal root .gitignore ONLY when the project has none — never

0 commit comments

Comments
 (0)