Skip to content

Commit 84bcba7

Browse files
dmealingclaude
andcommitted
fix(python): the manifest key mixed a resolved anchor with an unresolved path
Found by an independent review of this session's own work, and it is a regression this session introduced. `project_root` arrives resolved (`project_root_for` calls `.resolve()`) while the emitted path is built from the raw `out_dir`. `os.path.relpath` between a resolved anchor and an unresolved path is not the path between the two directories: under a directory symlink it walks OUT of the project, so every key read `../link/gen/X.py`. `_is_ours_for` then matched none of them and `verify --codegen` printed `in sync (0 file(s))` over 68 genuinely stale files — through both the symlink path and the real one. The spelling this replaced was immune BY ACCIDENT: `relpath(full, out_dir)` took both sides from one unresolved string, so it was symmetric. Symmetry is the property; both sides are now resolved deliberately, and the three reader sites share one `_out_prefix` helper rather than spelling the translation three times — three spellings of one translation is how they came to disagree. `/tmp` is a symlink on macOS, so any project under it reproduces this directly. Two of the three reader rungs were gated by nothing. The review reverted `out_prefix` at the neutral-fallback rung and at the multi-target config rung and the whole Python codegen suite still passed — 518/518. The pre-existing case that looks like it covers the config rung seeds a LEGACY bare key, so it is answered by the transitional fallback and can never see the new translation. Both rungs now have the case the flag-mode rung had, each confirmed red with its own prefix removed, plus the symlink case above. Also corrected: the C# HashManifest comment still said "C# and Python key relative to their single out dir" — two ports' comments contradicting each other about Python — and repeated the advice to point each out dir at its own gen-state dir, which is not followable because that directory is derived from the project rather than configured. C# has the same latent ambiguity; closing it is the same re-key plus fallback, and is not done here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTcEKXTQMYt84fAjuw5A2M
1 parent ba75e87 commit 84bcba7

5 files changed

Lines changed: 150 additions & 14 deletions

File tree

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

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,19 @@
55
// hashes identically on every port.
66
//
77
// The KEYS deliberately do not match across ports, and a manifest is NOT portable
8-
// between them. TS keys by path relative to the PROJECT ROOT because it supports
9-
// multiple output targets; C# and Python key relative to their single out dir. An
10-
// earlier version of this comment claimed a conformance fixture could compare two
11-
// ports' manifests directly — it cannot, and the claim was never true.
8+
// between them. TS and Python key by path relative to the PROJECT ROOT (TS because it
9+
// supports multiple output targets; Python because its manifest is ANCHORED on the
10+
// project, so an out-dir-relative key made two runs with different --out collide on one
11+
// entry). C# alone still keys relative to its single out dir. An earlier version of this
12+
// comment claimed a conformance fixture could compare two ports' manifests directly — it
13+
// cannot, and the claim was never true.
1214
//
1315
// One consequence worth knowing: because the key here is out-dir-relative, running gen
1416
// twice with different out dirs against ONE gen-state dir collides two distinct files
15-
// onto one key. Point each out dir at its own gen-state dir, or use the TS toolchain,
16-
// which is project-rooted and does not have the ambiguity.
17+
// onto one key — and the advice this comment used to give ("point each out dir at its own
18+
// gen-state dir") is not followable, because the gen-state dir is DERIVED from the project,
19+
// not configured. C# has the same latent ambiguity Python just fixed; closing it here means
20+
// the same re-key plus a legacy-key fallback, and it is not done yet.
1721
//
1822
// This file is meant to be COMMITTED. It is one hash per generated path — small and
1923
// reviewable — where a full snapshot of previously-generated content would be a second

‎server/python/src/metaobjects/cli.py‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1055,6 +1055,21 @@ def _relative_set(root: Path) -> dict[str, str]:
10551055
return files
10561056

10571057

1058+
def _out_prefix(out_dir: str, project_root: Path) -> str:
1059+
"""The out dir, relative to the project — the translation between this diff's
1060+
out-dir-relative keys and the manifest's project-relative ones.
1061+
1062+
BOTH SIDES RESOLVED, and that is the whole content of this function. `project_root`
1063+
arrives resolved; relpath from a resolved anchor to an unresolved path is not the
1064+
path between the two directories, and under a directory symlink it walks OUT of the
1065+
project. Every lookup then misses, `is_ours` is false for every file, and the gate
1066+
prints a clean verdict over stale output — silent in the direction that matters.
1067+
It is written once here rather than at each of the three call sites, because three
1068+
spellings of one translation is how they came to disagree in the first place.
1069+
"""
1070+
return os.path.relpath(Path(out_dir).resolve(), project_root)
1071+
1072+
10581073
def _is_ours_for(gen_state_dir: str | None, out_prefix: str = ""):
10591074
"""Jurisdiction predicate for the ``extra`` verdict: did WE write this path?
10601075
@@ -1226,7 +1241,7 @@ def _verify_codegen(args: argparse.Namespace) -> int:
12261241
project_root = project_root_for(args.metadata_dir)
12271242
return _diff_report(
12281243
expected, committed, gen_state_dir_for(args.metadata_dir),
1229-
os.path.relpath(Path(args.out).resolve(), project_root),
1244+
_out_prefix(args.out, project_root),
12301245
)
12311246

12321247

@@ -1283,7 +1298,7 @@ def _verify_codegen_neutral_fallback(args: argparse.Namespace) -> int:
12831298
# must be built from that, not from args.metadata_dir (None here by definition).
12841299
return _diff_report(
12851300
expected, committed, str(root_dir.resolve() / ".metaobjects" / ".gen-state"),
1286-
os.path.relpath(Path(args.out).resolve(), root_dir.resolve()),
1301+
_out_prefix(args.out, root_dir.resolve()),
12871302
)
12881303

12891304

@@ -1406,9 +1421,7 @@ def _verify_codegen_config(args: argparse.Namespace) -> int:
14061421
# their identically-named files apart is the one anchored on the project.
14071422
is_ours = _is_ours_for(
14081423
gen_state_dir_for(config.metadata_dir()),
1409-
os.path.relpath(
1410-
Path(real_outdir).resolve(), project_root_for(config.metadata_dir())
1411-
),
1424+
_out_prefix(real_outdir, project_root_for(config.metadata_dir())),
14121425
)
14131426
extra = sorted(k for k in committed if k not in expected and is_ours(k))
14141427

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,20 @@ def run_gen(
141141
# two runs with different --out cannot collide on one key inside the single
142142
# manifest that is anchored on that project. `rel` rides along as the legacy
143143
# spelling so an existing manifest keeps working and converges on the new one.
144-
key = os.path.relpath(full, config.project_root) if config.project_root else rel
144+
# BOTH SIDES RESOLVED. `project_root` arrives resolved (project_root_for calls
145+
# .resolve()) while `full` is built from the raw `out_dir`, and relpath between a
146+
# resolved anchor and an unresolved path is not the path between the two
147+
# directories — under a directory symlink it walks OUT of the project
148+
# (`../link/gen/X.py`). Every key then points outside, `_is_ours_for` matches
149+
# none of them, and `verify --codegen` prints `in sync (0 file(s))` over stale
150+
# output. The pre-existing `relpath(full, out_dir)` was immune by accident: both
151+
# sides came from the same unresolved string, so it was symmetric. Symmetry is
152+
# the property, and resolving both is how it is kept deliberately.
153+
key = (
154+
os.path.relpath(os.path.realpath(full), config.project_root)
155+
if config.project_root
156+
else rel
157+
)
145158
status = decide_and_write(
146159
full,
147160
content,

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,3 +290,57 @@ def test_config_mode_ignores_a_discovered_spec(tmp_path: Path) -> None:
290290
)
291291
assert main(["gen", "--config", str(cfg)]) == 0
292292
assert main(["verify", "--codegen", "--config", str(cfg)]) == 0
293+
294+
295+
# --- the `extra` verdict at the two rungs the flag-mode test cannot reach ------------
296+
#
297+
# Manifest keys are project-root-relative while this diff is keyed relative to `--out`;
298+
# `out_prefix` is what translates one into the other, and `_is_ours_for` needs it to find
299+
# the record proving we wrote a file. It is threaded at THREE call sites and only the
300+
# explicit-flag rung was gated (`test_verify_codegen_still_convicts_output_it_did_write`).
301+
#
302+
# The uncovered direction fails silently, which is why it needs its own case per rung: a
303+
# lookup that finds nothing makes every file "not ours", so `extra` empties and the gate
304+
# prints a CLEAN verdict over stale committed output. Both cases below were confirmed to
305+
# pass with `out_prefix` removed at their rung before they were written — i.e. the whole
306+
# Python codegen suite could not see it.
307+
308+
309+
def test_neutral_fallback_convicts_output_it_did_write(
310+
tmp_path: Path, monkeypatch
311+
) -> None:
312+
"""The `.metaobjects/config.json` `sources` rung."""
313+
model = tmp_path / "model"
314+
model.mkdir()
315+
(model / "meta.fitness.json").write_text(FITNESS.read_text())
316+
d = tmp_path / ".metaobjects"
317+
d.mkdir()
318+
(d / "config.json").write_text(
319+
'{"schema_version": 1, "sources": [{"path": "model"}]}'
320+
)
321+
monkeypatch.chdir(tmp_path)
322+
assert main(["gen", "--out", "gen/models"]) == 0
323+
assert (tmp_path / "gen/models/Program.py").exists()
324+
325+
# A regen no longer emits Program.py, but it IS committed and IS in the manifest —
326+
# the definition of stale output.
327+
(model / "meta.fitness.json").write_text(
328+
'{"metadata.root": {"package": "fitness", "children": []}}'
329+
)
330+
assert main(["verify", "--codegen", "--out", "gen/models"]) == 1
331+
332+
333+
def test_config_mode_convicts_output_it_did_write(tmp_path: Path) -> None:
334+
"""The declarative `metaobjects.config.yaml` per-target rung — the one where the
335+
prefix matters most, because every target has its own outDir and all of them share
336+
ONE manifest."""
337+
cfg = _project(tmp_path)
338+
assert main(["gen", "--config", str(cfg)]) == 0
339+
assert (tmp_path / "gen/models/Program.py").exists()
340+
341+
# Narrow `models` to one entity: Week.py stays committed and recorded while a fresh
342+
# regen no longer emits it.
343+
cfg.write_text(
344+
TWO_TARGETS.replace("entities: [Program, Week]", "entities: [Program]")
345+
)
346+
assert main(["verify", "--codegen", "--config", str(cfg)]) == 1

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

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,5 +126,57 @@ def test_read_prefers_the_new_key_over_the_legacy_one(tmp_path: Path) -> None:
126126
json.dumps({"gen/names.py": "new", "names.py": "legacy"}), encoding="utf-8"
127127
)
128128
assert read_generated_hash(str(gen_state), "gen/names.py", "names.py") == "new"
129-
# And the legacy key alone still answers, for a file not yet re-recorded.
130-
assert read_generated_hash(str(gen_state), "gen/other.py", "other.py") is None
129+
# The legacy key alone still answers, for a file not yet re-recorded under the new
130+
# spelling: the new key is absent, so the fallback is what finds the record.
131+
assert read_generated_hash(str(gen_state), "gen/not-yet-rekeyed.py", "names.py") == "legacy"
132+
# A key present in NEITHER spelling stays unrecorded — the fallback must not become a
133+
# way to say yes to a file nobody recorded.
134+
assert read_generated_hash(str(gen_state), "gen/absent.py", "absent.py") is None
135+
136+
137+
def test_a_project_reached_through_a_symlink_keeps_its_jurisdiction(
138+
tmp_path: Path, monkeypatch
139+
) -> None:
140+
"""The anchor and the path must be resolved on the SAME side.
141+
142+
``project_root`` arrives resolved; ``full`` is built from the raw ``out_dir``. Relpath
143+
between a resolved anchor and an unresolved path is not the path between the two
144+
directories — under a directory symlink it walks OUT of the project, so every key
145+
read ``../link/gen/X.py``, ``_is_ours_for`` matched none of them, and
146+
``verify --codegen`` printed ``in sync (0 file(s))`` over 68 genuinely stale files.
147+
148+
The spelling this replaced (``relpath(full, out_dir)``) was immune by ACCIDENT: both
149+
sides came from one unresolved string, so it was symmetric. Symmetry is the property;
150+
this pins it deliberately. ``/tmp`` is a symlink on macOS, so any project there hits
151+
it directly.
152+
"""
153+
real = tmp_path / "proj"
154+
(real / "metaobjects").mkdir(parents=True)
155+
fixture = (
156+
Path(__file__).parents[4]
157+
/ "fixtures"
158+
/ "persistence-conformance"
159+
/ "canonical"
160+
/ "meta.fitness.json"
161+
)
162+
meta_file = real / "metaobjects" / "meta.fitness.json"
163+
meta_file.write_text(fixture.read_text(), encoding="utf-8")
164+
link = tmp_path / "link"
165+
link.symlink_to(real, target_is_directory=True)
166+
167+
from metaobjects.cli import main
168+
169+
monkeypatch.chdir(tmp_path)
170+
assert main(["gen", str(link / "metaobjects"), "--out", str(link / "gen")]) == 0
171+
172+
manifest = json.loads(
173+
(real / ".metaobjects" / ".gen-state" / ".hashes.json").read_text(encoding="utf-8")
174+
)
175+
assert manifest, "gen recorded no manifest"
176+
# Not one key escapes the project. A `../` key is the whole defect.
177+
assert not any(k.startswith("..") for k in manifest), sorted(manifest)[:3]
178+
assert all(k.startswith("gen/") for k in manifest), sorted(manifest)[:3]
179+
180+
# And the gate still convicts stale output reached through the link.
181+
meta_file.write_text('{"metadata.root": {"package": "fitness", "children": []}}')
182+
assert main(["verify", "--codegen", str(link / "metaobjects"), "--out", str(link / "gen")]) == 1

0 commit comments

Comments
 (0)