Skip to content

Commit 514f707

Browse files
dmealingclaude
andcommitted
fix: three review findings — a too-broad widening, a case-folding hole, a tense
Review of the preceding two commits found three defects in them. 1. The `expectError: true` widening was too broad, and weakened six cases to buy one. `true` had a stronger meaning than the README's wording admitted, and all three coded ports implemented it identically: "raises the port's coded metadata error; WHICH code is unpinned". The type was pinned. Relaxing the whole arm to bare Exception/`Throwable` so the symlink cycle could raise a native IOException also relaxed `sources-must-be-an-array-not-an-object` and its five siblings, which would now PASS on a raw NullReferenceException / NPE / TypeError — a crash scoring as a correct rejection. The allowance is now scoped to the single case that needs it via a new optional `errorIsNative`, the coded type is required everywhere else, and the README says which of the two `true` pins (the type) and which it does not (the code). TypeScript is unaffected and stays loose: it propagates the raw parser error, which is why the code is unpinned in the first place. 2. The cycle guard's ancestor set compared `StringComparer.Ordinal` while `RealPath` necessarily mixes spellings — a non-link segment keeps the caller's casing via `GetFullPath`, a resolved link comes back in the filesystem's canonical casing. On a case-insensitive volume `MODEL/loop -> model` therefore walked straight past the guard, back down to the kernel ELOOP floor that the new immediacy test claims to have removed — and this is the one port whose enumeration SWALLOWS ELOOP, so there is no backstop under it. Now Ordinal on Linux (where `Model` and `model` are genuinely different directories and folding would reject a valid tree) and case-insensitive elsewhere. 3. `--print-only` suppressed the writes but not the past-tense reporting, so a dry run announced `wired @.metaobjects/AGENTS.md into CLAUDE.md` and `refreshed version written to <path>.new` for edits it had not made. Worse than the silent write it replaced: it names a side effect on a file the user owns, which they can go look for and will not find. All three messages are future-tense under a dry run, pinned by a test asserting no past-tense form survives. That test caught its own fix at first: `\bcreated with\b` also matches the CORRECT "(would be created with …)". Anchored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 63a470b commit 514f707

9 files changed

Lines changed: 143 additions & 48 deletions

File tree

‎fixtures/source-resolution-conformance/README.md‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ are read, `scope` filters what is emitted from them.
1111
## Shape
1212

1313
```
14-
cases.json # { cases: [{ name, tree, symlinks?, config, resolveFrom?, expectFiles?, expectError? }] }
14+
cases.json # { cases: [{ name, tree, symlinks?, config, resolveFrom?, expectFiles?, expectError?, errorIsNative? }] }
1515
README.md
1616
```
1717

@@ -78,10 +78,26 @@ README.md
7878
`resolveFrom`), compared as an **UNORDERED SET**. See "Order is
7979
deliberately not pinned" below.
8080
- **`expectError`** — either a STRING error code the resolution must fail with
81-
exactly, or the literal `true` meaning "must raise, but which error/code is
82-
deliberately not pinned across ports" (see "Also deliberately NOT pinned:
83-
the malformed-config error code" below for why the latter form exists).
84-
Exactly one of `expectFiles` / `expectError` is present per case.
81+
exactly, or the literal `true` meaning "must raise the port's own coded
82+
metadata error, but WHICH code is deliberately not pinned across ports" (see
83+
"Also deliberately NOT pinned: the malformed-config error code" below for why
84+
the latter form exists). Exactly one of `expectFiles` / `expectError` is
85+
present per case. Note what `true` still pins in a port that HAS a coded error
86+
type: the type. Java, C# and Python all require `MetaDataException` /
87+
`MetaModelException` / `ParseError` on this arm, so a malformed config that
88+
crashes with a raw `NullPointerException` fails the case rather than passing
89+
it. (TypeScript alone cannot: it propagates the raw parser error, which is the
90+
whole reason the code is unpinned.)
91+
- **`errorIsNative`** — OPTIONAL boolean, only meaningful beside
92+
`expectError: true`. Means the failure surfaces as a PLATFORM-native error
93+
rather than the port's coded metadata error, so the type requirement above is
94+
lifted for this case alone. Exactly one case sets it:
95+
`a-symlink-cycle-is-an-error`, where the raise comes from the filesystem walk
96+
(`IOException` in C#, `FileSystemLoopException` in Java, `SymlinkLoopError` in
97+
Python) and never passes through a coded-error constructor. It exists so that
98+
admitting that one case did not silently relax the other six `true` cases from
99+
"a coded parse failure" to "anything at all" — which is precisely what a blanket
100+
widening of the runners does, and did.
85101

86102
## Semantics pinned here
87103

‎fixtures/source-resolution-conformance/cases.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,8 @@
272272
},
273273
"symlinks": { "model/loop": "model" },
274274
"config": { "schema_version": 1, "sources": [{ "path": "model" }] },
275-
"expectError": true
275+
"expectError": true,
276+
"errorIsNative": true
276277
}
277278
]
278279
}

‎server/csharp/MetaObjects.Conformance.Tests/SourceResolutionConformanceTests.cs‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,20 @@ private sealed record Case(
2727
// count silently goes stale; the structural description above does not.)
2828
string ResolveFrom,
2929
string[]? ExpectFiles,
30-
// A JSON string pins the exact error code raised; JSON `true` pins only
31-
// that resolution RAISES — the malformed-config error code is
32-
// deliberately not pinned cross-port (see the corpus README).
30+
// A JSON string pins the exact error code raised; JSON `true` leaves the
31+
// CODE unpinned but still requires the port's coded MetaModelException —
32+
// the malformed-config error code is deliberately not pinned cross-port
33+
// (see the corpus README), the TYPE is.
3334
JsonElement? ExpectError,
3435
// Optional: linkPath -> targetPath, both project-root-relative, materialized
3536
// AFTER `Tree` (I1 — a symlinked source root, or a symlinked subdirectory
3637
// inside a walked tree).
37-
Dictionary<string, string> Symlinks);
38+
Dictionary<string, string> Symlinks,
39+
// Optional: this case's failure surfaces as a PLATFORM-native error rather
40+
// than a coded MetaModelException, so the type requirement above is lifted
41+
// for it alone (the symlink-cycle case, whose raise comes from the
42+
// filesystem walk and never reaches a coded-error constructor).
43+
bool ErrorIsNative);
3844

3945
public static TheoryData<string> CaseNames()
4046
{
@@ -78,7 +84,8 @@ private static List<Case> LoadCases()
7884
foreach (var p in sl.EnumerateObject()) symlinks[p.Name] = p.Value.GetString()!;
7985
}
8086

81-
cases.Add(new Case(el.GetProperty("name").GetString()!, tree, cfg, resolveFrom, expectFiles, expectError, symlinks));
87+
bool errorIsNative = el.TryGetProperty("errorIsNative", out var ein) && ein.GetBoolean();
88+
cases.Add(new Case(el.GetProperty("name").GetString()!, tree, cfg, resolveFrom, expectFiles, expectError, symlinks, errorIsNative));
8289
}
8390
return cases;
8491
}
@@ -127,19 +134,21 @@ public void ResolvesTheSameFileSet(string name)
127134

128135
if (c.ExpectError is not null)
129136
{
130-
// `true` pins only that resolution RAISES — deliberately not which type,
131-
// so the assertion is on Exception. Narrowing it to MetaModelException
132-
// would silently re-pin the very thing the corpus refuses to pin, and did:
133-
// the symlink-cycle guard raises IOException (the natural type, and the
134-
// one Java's FileSystemLoopException also derives from), which this
135-
// runner scored as a FAILURE even though the port behaved correctly.
136-
// A string still pins the exact code, and that arm requires the richer
137-
// type — see the ExpectError field doc above.
137+
// A string pins the exact code. `true` leaves the CODE unpinned but
138+
// still requires MetaModelException — a malformed config that crashes
139+
// with a raw NullReferenceException must FAIL this case, not pass it.
140+
// `ErrorIsNative` lifts only the type requirement, for the one case
141+
// whose raise comes from the filesystem walk (IOException) and never
142+
// reaches a coded-error constructor; widening the whole arm instead
143+
// would quietly relax the six malformed-config cases to "anything".
138144
var ex = Assert.ThrowsAny<Exception>(() => SourceResolver.ResolveCollection(invokeDir));
139-
if (c.ExpectError.Value.ValueKind == JsonValueKind.String)
145+
if (!c.ErrorIsNative)
140146
{
141147
var coded = Assert.IsAssignableFrom<MetaModelException>(ex);
142-
Assert.Equal(c.ExpectError.Value.GetString(), coded.Code.ToString());
148+
if (c.ExpectError.Value.ValueKind == JsonValueKind.String)
149+
{
150+
Assert.Equal(c.ExpectError.Value.GetString(), coded.Code.ToString());
151+
}
143152
}
144153
return;
145154
}

‎server/csharp/MetaObjects/Loader/DirectorySource.cs‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,27 @@ public DirectorySource(string directory, Options? opts = null)
6363
/// by full path (ordinal). The sort happens on the full path so that nested
6464
/// directory traversal is also deterministic.
6565
/// </summary>
66+
/// <summary>
67+
/// How two resolved directory paths are compared when deciding "is this an
68+
/// ancestor I have already walked".
69+
/// </summary>
70+
/// <remarks>
71+
/// Ordinal on Linux, where the filesystem genuinely distinguishes <c>Model</c> from
72+
/// <c>model</c> and case-folding would REJECT a valid tree. Case-insensitive
73+
/// elsewhere, because <see cref="RealPath"/> necessarily mixes spellings — a
74+
/// non-link segment keeps whatever casing the caller wrote (via
75+
/// <see cref="Path.GetFullPath(string)"/>) while a resolved link comes back in the
76+
/// filesystem's own canonical casing — so on a case-insensitive volume an ordinal
77+
/// compare lets <c>MODEL/loop -> model</c> slip past the guard. Slipping past is not
78+
/// benign here: this is the one port whose enumeration SWALLOWS the kernel's ELOOP,
79+
/// so there is no backstop underneath it.
80+
/// </remarks>
81+
private static readonly StringComparer PathComparer =
82+
OperatingSystem.IsLinux() ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase;
83+
6684
public IEnumerable<FileSource> Expand()
6785
{
68-
IEnumerable<string> files = Collect(Directory, new HashSet<string>(StringComparer.Ordinal))
86+
IEnumerable<string> files = Collect(Directory, new HashSet<string>(PathComparer))
6987
.Where(p => _supportedExtensions.Contains(Path.GetExtension(p)));
7088

7189
if (Opts.ExcludePending)
@@ -123,7 +141,7 @@ private IEnumerable<string> Collect(string directory, HashSet<string> ancestors)
123141
throw new IOException(
124142
$"symlink loop detected while expanding metadata directory: {directory} revisits {real}");
125143

126-
var nextAncestors = new HashSet<string>(ancestors, StringComparer.Ordinal) { real };
144+
var nextAncestors = new HashSet<string>(ancestors, PathComparer) { real };
127145

128146
// Sorted so traversal is deterministic across filesystems, matching the
129147
// full-path ordinal sort Expand() applies to the result.

‎server/java/metadata/src/test/java/com/metaobjects/config/SourceResolutionConformanceTest.java‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,18 @@ public class SourceResolutionConformanceTest {
7373
*/
7474
/**
7575
* {@code expectError}: a JSON string pins the exact error code raised; JSON
76-
* {@code true} pins only that resolution RAISES — the malformed-config error
77-
* code is deliberately not pinned cross-port (see the corpus README).
76+
* {@code true} leaves the CODE unpinned but still requires the port's coded
77+
* {@link MetaDataException} — the malformed-config error code is deliberately
78+
* not pinned cross-port (see the corpus README), the TYPE is.
79+
*
80+
* <p>{@code errorIsNative}: this case's failure surfaces as a PLATFORM-native
81+
* error rather than a coded exception, lifting the type requirement for it
82+
* alone — the symlink-cycle case, whose raise comes from the filesystem walk
83+
* ({@code FileSystemLoopException}) and never reaches a coded-error constructor.
7884
*/
7985
private record Case(String name, Map<String, String> tree, JsonObject config,
8086
String resolveFrom, List<String> expectFiles, JsonElement expectError,
81-
Map<String, String> symlinks) {}
87+
Map<String, String> symlinks, boolean errorIsNative) {}
8288

8389
/** Package-private (not private): shared with {@link SourceResolutionCorpusNotEmptyTest},
8490
* which needs to locate the same committed corpus file without a second definition
@@ -143,7 +149,9 @@ public static Collection<Object[]> cases() throws IOException {
143149
}
144150
}
145151

146-
rows.add(new Object[]{name, new Case(name, tree, config, resolveFrom, expectFiles, expectError, symlinks)});
152+
boolean errorIsNative = c.has("errorIsNative") && c.get("errorIsNative").getAsBoolean();
153+
154+
rows.add(new Object[]{name, new Case(name, tree, config, resolveFrom, expectFiles, expectError, symlinks, errorIsNative)});
147155
}
148156
return rows;
149157
}
@@ -199,19 +207,24 @@ public void resolvesTheSameFileSet() throws IOException {
199207
SourceResolver.resolveCollection(invokeDir);
200208
fail("expected " + testCase.expectError() + " for case " + testCase.name());
201209
} catch (Exception e) {
202-
// `true` pins only that it RAISES, so the catch is on Exception.
203-
// Narrowing it to MetaDataException silently re-pinned the very thing
204-
// the corpus refuses to pin: the symlink-cycle guard surfaces a
205-
// FileSystemLoopException, which escaped this catch and failed the
206-
// case even though the port behaved exactly as the contract requires.
210+
// The catch is on Exception so a native raise can reach the body at
211+
// all; the TYPE requirement is asserted here rather than by the catch
212+
// clause. `true` leaves the CODE unpinned but still demands
213+
// MetaDataException — a malformed config that crashes with a raw NPE
214+
// must FAIL this case, not pass it. `errorIsNative` lifts that for the
215+
// one case whose raise comes from the filesystem walk
216+
// (FileSystemLoopException) and never reaches a coded-error
217+
// constructor; widening the whole arm instead would quietly relax the
218+
// six malformed-config cases to "anything at all".
207219
// (`fail` above throws AssertionError, an Error — so it still escapes.)
208-
// A string still pins the exact code, and that arm needs the coded type.
209-
if (codePinned) {
210-
assertTrue("case " + testCase.name() + " pins code " + expected.getAsString()
211-
+ " so it must raise MetaDataException, got " + e,
220+
if (!testCase.errorIsNative()) {
221+
assertTrue("case " + testCase.name()
222+
+ " must raise MetaDataException, got " + e,
212223
e instanceof MetaDataException);
213-
assertEquals(expected.getAsString(),
214-
((MetaDataException) e).getCode().orElseThrow().name());
224+
if (codePinned) {
225+
assertEquals(expected.getAsString(),
226+
((MetaDataException) e).getCode().orElseThrow().name());
227+
}
215228
}
216229
}
217230
return;

‎server/python/tests/conformance/test_source_resolution_conformance.py‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,14 +68,16 @@ def test_source_resolution_conformance(case: dict, tmp_path: Path) -> None:
6868
resolve_from = _materialize(case, tmp_path)
6969

7070
if "expectError" in case:
71-
# `True` pins only that resolution RAISES, so the expected type is
72-
# Exception. Catching ParseError instead silently re-pinned the very
73-
# thing the corpus refuses to pin, and did: the symlink-cycle guard
74-
# raises SymlinkLoopError, which this runner scored as a FAILURE even
75-
# though the port behaved exactly as the contract requires. A string
76-
# still pins the exact code, and that arm needs the coded type.
71+
# A string pins the exact code. `True` leaves the CODE unpinned but still
72+
# requires ParseError — a malformed config that crashes with a raw
73+
# TypeError must fail this case, not pass it. `errorIsNative` lifts only
74+
# the type requirement, for the one case whose raise comes from the
75+
# filesystem walk (SymlinkLoopError) and never reaches a coded-error
76+
# constructor; widening the whole arm instead would quietly relax the six
77+
# malformed-config cases from "a coded parse failure" to "anything".
7778
expected = case["expectError"]
78-
with pytest.raises(ParseError if isinstance(expected, str) else Exception) as e:
79+
expected_type = Exception if case.get("errorIsNative") else ParseError
80+
with pytest.raises(expected_type) as e:
7981
resolve_collection(resolve_from)
8082
if isinstance(expected, str):
8183
assert e.value.code.value == expected

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

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,14 @@ async function writeAgentContext(opts: InitOptions, result: InitResult): Promise
269269
await writeFile(abs, c.contents, "utf8");
270270
}
271271
result.created.push(c.newPath);
272-
result.warnings.push(`${c.path} appears hand-edited; refreshed version written to ${c.newPath}`);
272+
// Past tense only when it actually happened — a dry run that reports "written
273+
// to <path>.new" is claiming an edit-preserving side effect the user can go
274+
// look for and will not find.
275+
result.warnings.push(
276+
dryRun
277+
? `${c.path} appears hand-edited; refreshed version would be written to ${c.newPath}`
278+
: `${c.path} appears hand-edited; refreshed version written to ${c.newPath}`,
279+
);
273280
}
274281
if (!dryRun) {
275282
const manifestAbs = join(opts.cwd, AGENT_CONTEXT_MANIFEST_PATH);
@@ -295,7 +302,11 @@ async function wireRootMemory(cwd: string, result: InitResult, dryRun = false):
295302
// If neither root memory file exists, create CLAUDE.md (Claude Code's canonical) with the import.
296303
if (!claudeExists && !agentsExists) {
297304
if (!dryRun) await writeFile(claudePath, `# Project memory\n\n${ROOT_IMPORT_LINE}\n`, "utf8");
298-
result.created.push("CLAUDE.md (created with MetaObjects @import)");
305+
result.created.push(
306+
dryRun
307+
? "CLAUDE.md (would be created with MetaObjects @import)"
308+
: "CLAUDE.md (created with MetaObjects @import)",
309+
);
299310
return;
300311
}
301312
// Otherwise append the import to whichever exist (idempotent — never double-add).
@@ -304,7 +315,14 @@ async function wireRootMemory(cwd: string, result: InitResult, dryRun = false):
304315
const body = await readFile(path, "utf8");
305316
if (body.includes(ROOT_IMPORT_LINE)) continue;
306317
if (!dryRun) await writeFile(path, `${body.replace(/\n*$/, "\n")}\n${ROOT_IMPORT_LINE}\n`, "utf8");
307-
result.warnings.push(`wired ${ROOT_IMPORT_LINE} into ${path.endsWith("AGENTS.md") ? "AGENTS.md" : "CLAUDE.md"} so the MetaObjects context loads`);
318+
// Past tense only when it actually happened — this one mutates a file the user
319+
// owns, so a dry run reporting it as done is the most misleading of the three.
320+
const target = path.endsWith("AGENTS.md") ? "AGENTS.md" : "CLAUDE.md";
321+
result.warnings.push(
322+
dryRun
323+
? `would wire ${ROOT_IMPORT_LINE} into ${target} so the MetaObjects context loads`
324+
: `wired ${ROOT_IMPORT_LINE} into ${target} so the MetaObjects context loads`,
325+
);
308326
}
309327
}
310328

‎server/typescript/packages/cli/test/unit/init-docs-only.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,18 @@ describe("init() --docs-only", () => {
4949

5050
// ...and the directory is untouched.
5151
expect(readdirSync(cwd)).toEqual([]);
52+
53+
// Nothing reports in the PAST tense. A dry run claiming it "created" a root
54+
// CLAUDE.md or "wired" an @import names a side effect on a file the user owns,
55+
// which they can go look for and will not find — a more expensive lie than the
56+
// silent write, because it reads as a completed action.
57+
// Anchored, not `\bcreated with\b` — that also matches the CORRECT "(would be
58+
// created with …)" and the assertion fails on its own fix.
59+
const past = [...planned.created, ...planned.warnings].filter(
60+
(m) => m.includes("(created with") || /^wired /.test(m) || m.includes("version written to"),
61+
);
62+
expect(past).toEqual([]);
63+
expect(planned.created).toContain("CLAUDE.md (would be created with MetaObjects @import)");
5264
});
5365

5466
test("--refresh-docs --print-only writes nothing", async () => {

‎server/typescript/packages/sdk/test/source-resolution-conformance.test.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,12 @@ interface Case {
1919
* resolution RAISES — the malformed-config error code is deliberately not
2020
* pinned cross-port (see the corpus README). */
2121
readonly expectError?: string | true;
22+
/** Optional: the failure surfaces as a PLATFORM-native error rather than a coded
23+
* one, lifting the coded-TYPE requirement for this case. Java/C#/Python honour
24+
* it; TypeScript has nothing to lift — it propagates the raw parser error, which
25+
* is why the code is unpinned here in the first place — so it is unread. Declared
26+
* anyway so this interface stays a faithful mirror of the case schema. */
27+
readonly errorIsNative?: boolean;
2228
/** Optional: linkPath -> targetPath, both project-root-relative, materialized
2329
* AFTER `tree` (I1 — a symlinked source root, or a symlinked subdirectory
2430
* inside a walked tree). */

0 commit comments

Comments
 (0)