fix: close the three open cross-port source-resolution items (print-only writes, C# symlink cycle, strict-config ruling) - #319
Merged
Conversation
`--config-only --print-only` was fixed by moving the config branch below the printOnly guard. The two agent-context branches — `--docs-only` and `--refresh-docs` — return from `init()` above that same guard and were left, so both documented dry runs silently scaffolded for real: the docs, every stack-scoped skill reference, the manifest, and (with --wire-root) a created root CLAUDE.md. The reason it was left is that this write set is DYNAMIC — it depends on the resolved stack, so it cannot be the hardcoded path list the full-scaffold dry run uses. It does not need to be. `writeAgentContext` already computes the complete plan (`planScaffold`) before performing a single write, so the guard goes at the I/O, not at the report: `result.created` is populated from the same plan a real run executes, which makes the dry run's output exactly the real write set with no second list to drift. `.agent-context.json` joins that report. A real run has always written it while omitting it from `created`, and the full-scaffold dry run has always named it — reporting it here settles both against what actually happens on disk. Proven by reverting: both new tests go red (the docs land on disk), and the `--refresh-docs` arm is a genuinely separate door, red on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng the type Following symlinked directories became a cross-port contract in #315, which is what makes a symlink CYCLE reachable. TypeScript, Java and Python each added a guard when they picked it up; C# did not, and the failure mode is worse than a hang. `Directory.EnumerateFiles(dir, "*", AllDirectories)` follows links but has no loop guard, and its `EnumerationOptions` default of `IgnoreInaccessible` swallows the kernel's own ELOOP refusal — so a self-referential directory symlink did not hang and did not throw. It COMPLETED NORMALLY, returning 41 copies of one real file at ever-deeper phantom paths (measured, not inferred). Nothing downstream could recover: `SourceResolver` de-duplicates on the LEXICAL full path and every phantom is lexically distinct, so each was admitted as its own source and the same metadata loaded once per level. `Expand` now walks itself, carrying the real ancestor directories on the current branch and raising on revisit. Resolution is component-by-component from the root, because `Directory.ResolveLinkTarget` canonicalizes only the FINAL segment: using it alone leaves a symlinked ancestor unresolved, and the guard then compares a half-resolved path against a real one and misses the loop — recursing forever on exactly the input it exists to catch. Ancestors extend only on the recursive call, never in place, so a diamond still resolves. The corpus gains `a-symlink-cycle-is-an-error`. Adding it exposed that THREE of the four runners had quietly re-pinned what the corpus refuses to pin: the README says `expectError: true` means "raises, type deliberately unpinned", but C# asserted MetaModelException, Python caught ParseError and Java caught MetaDataException. Each scored a correct port as a failure — the guards raise IOException, SymlinkLoopError and FileSystemLoopException respectively. All three now assert on the base type for the `true` form, and still demand the coded type for the string form, which is the arm that actually needs it. The case is documented as a FLOOR. On Linux the kernel's ELOOP makes an unguarded walk raise eventually anyway, so `expectError: true` cannot tell a real guard from a late accident; what it DOES discriminate is a port that swallows that error and reports success, which is precisely what C# did. Immediacy and the diamond distinction are pinned per-port instead — C# gains the two tests the other three already had. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d pin both halves A genuinely unknown top-level key in `.metaobjects/config.json` throws in TypeScript and resolves fine in Java, C# and Python. The corpus README recorded this as a confirmed cross-port divergence and left it as "an open, human-reviewable follow-up". Reviewed: neither side moves. It is not a defect that leaked — it falls out of ownership. The file is TypeScript's, and TypeScript is the only port that models its whole vocabulary, so it is the only one that CAN tell a typo from a key a sibling owns. `.strict()` is what turns that knowledge into a diagnostic, and the hazard it catches is specific and already pinned: a stripped `scopes` (for `scope`) silently means "everything in scope", and a stripped `migrate.scopee` silently governs the whole database. The other three model the neutral subset, for which every other key is indistinguishable from a TS-owned one; they could imitate strictness only by carrying TypeScript's key list in lockstep, and would then REJECT a config a newer `meta` had just written. Tolerance is the only coherent behaviour for a partial reader, strictness the only coherent behaviour for the owner. So this stays off the shared corpus permanently rather than being deferred again. A shared case asserts ONE outcome and the correct outcome differs by port by design, so adding one could only be done by making some port wrong. Each half is now pinned where it belongs instead — TypeScript's in `config.test.ts` beside the `scopes`/`scopee` cases it shares a rationale with, and the tolerant half in the C#, Python and Java resolver tests, none of which previously covered it. The forward-compatibility cost is stated and accepted: a config written by a newer `meta` hard-fails an older one, which is what `schema_version` is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e, 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>
…write sites
Post-review simplification pass. No behaviour change; all five constraints the
review pass established are intact and still proven by their tests.
`DirectorySource.RealPath` drops the `File.ResolveLinkTarget` fallback and the
manual rooted/relative branch. Both were dead code, verified rather than assumed:
`Directory.ResolveLinkTarget(path, returnFinalTarget: true).FullName` already
returns a fully-qualified, symlink-resolved absolute path for a relative one-hop
target, a relative chain, an absolute target and a `.` self-link — checked with
the process CWD set to `/`, which is exactly where a relative target resolved
against the wrong base would have shown itself. Since `Collect` only ever calls
`RealPath` on a path it has already confirmed is a directory, `File.`- and
`Directory.ResolveLinkTarget` could never disagree either. Thirteen lines become
two; the component-by-component LOOP is untouched, which is the part that
carries the correctness (resolving only the final segment misses a cycle reached
through a symlinked ancestor).
Also fixes a doc-comment bug introduced by the previous commit: the new
`PathComparer` block was inserted between `Expand()`'s existing `<summary>` and
`Expand()` itself, so two adjacent `///` blocks merged into one and XML-doc
tooling attached the pair to `PathComparer`, orphaning `Expand()`'s own summary.
`init.ts` factors the four-times-repeated `if (!dryRun) { mkdir; writeFile }` into
`writeUnlessDryRun`, and the three past/future tense ternaries into
`verbed(dryRun, pastParticiple)` — each call site now states only its own past
participle rather than spelling out both tenses of a whole sentence. This
normalises one message from active "would wire" to passive "would be wired", to
match the two that already read that way; the non-dry-run wording is unchanged,
which is what the tests pin.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the three items the cross-port
sourceswork (#315 / #316) left open and documented. Non-breaking, PATCH-shaped: no registry vocabulary, no metamodel change, no adopter-visible default flip.expected-registry.jsonis untouched.1.
--print-onlywrote for real on both agent-context paths--config-only --print-onlywas fixed by moving that branch below theprintOnlyguard.--docs-onlyand--refresh-docsreturn frominit()above the same guard and were left, so both documented dry runs silently scaffolded: the docs, every stack-scoped skill reference, the manifest, and (with--wire-root) a created rootCLAUDE.md.It was left because the write set is dynamic. It didn't need to be —
writeAgentContextalready computes the complete plan before writing, so guarding the I/O rather than the report makes the dry run report exactly the real write set, with no second list to drift.--refresh-docswas a second door the original note didn't mention.2. C# admitted phantom sources on a symlink cycle
Worse than recorded, and measured rather than inferred.
Directory.EnumerateFiles(dir, "*", AllDirectories)follows links but has no loop guard, and itsEnumerationOptionsdefault ofIgnoreInaccessibleswallows the kernel's ELOOP. So a self-referential directory symlink did not hang and did not throw — it completed normally, returning 41 copies of one real file at ever-deeper phantom paths. Nothing downstream could recover:SourceResolverde-duplicates on the LEXICAL full path and every phantom is lexically distinct, so each was admitted as its own source and the same metadata loaded once per level. TypeScript, Java and Python all raise; C# alone reported success.Expandnow walks itself with a per-branch real-ancestor set.RealPathresolves component-by-component becauseDirectory.ResolveLinkTargetcanonicalizes only the final segment — using it alone leaves a symlinked ancestor unresolved and misses the cycle it exists to catch.Adding the corpus case exposed a second, wider bug. The corpus README documents
expectError: trueas leaving the error unpinned, but three of the four runners (C#, Python, Java) asserted a narrow coded type anyway — each would have scored a correct port as failing. Latent only because no prior case exercised the looseness.3.
ConfigSchema.strict()— ruled INTENDEDA genuinely unknown top-level config key throws in TypeScript and resolves fine in the other three. Reviewed and ruled: neither side moves. TypeScript owns the file and models its whole vocabulary, so it is the only port that can tell a typo from a key a sibling owns — and the hazard is concrete (
scopesforscopesilently means "everything in scope"). The others model the neutral subset and could imitate strictness only by carrying TS's key list in lockstep, at which point a port one release behind would reject a config a newermetahad just written.Stays off the shared corpus permanently rather than deferred again: a shared case asserts ONE outcome and the correct outcome differs by port by design. Both halves are pinned where they belong instead.
Review + simplify
A review pass over the first three commits found three defects in them, all fixed in
514f7072:expectError: truewidening was too broad — it relaxed six pre-existing malformed-config cases from "a coded parse failure" to "anything at all", so a raw NPE/NullReferenceException/TypeErrorwould have scored as a correct rejection. Scoped to the one case that needs it via a new optionalerrorIsNative.StringComparer.OrdinalwhileRealPathnecessarily mixes lexical and canonical spellings, so on a case-insensitive volumeMODEL/loop -> modelwalked past the guard — back to the ELOOP floor that this port swallows.--print-onlysuppressed the writes but not the past-tense reporting, announcingwired … into CLAUDE.mdfor edits it hadn't made.Then a simplifier pass (
f3ba9650) cutRealPathfrom 13 lines to 2 after verifying — with the process CWD set to/— thatResolveLinkTarget(…, returnFinalTarget: true).FullNamealready returns fully-qualified resolved paths for relative one-hop targets, relative chains and.self-links, so the removed fallback was dead code.Verification
Every fix was proven by reverting it and watching the test go red, not by its accompanying green:
errorIsNativeremoved → the cycle case fails in all three coded ports, proving the restored type requirement is live rather than decorative.--print-onlydoors reverted → docs land on disk; the--refresh-docsarm is red on its own.Full local CI green — all 20 gates, including five-port conformance, the Java reactor and the docker integration suite.
🤖 Generated with Claude Code