Skip to content

Commit f3ba965

Browse files
dmealingclaude
andcommitted
refactor: simplify the cycle guard's path resolution and the dry-run 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>
1 parent 514f707 commit f3ba965

2 files changed

Lines changed: 43 additions & 47 deletions

File tree

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

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,6 @@ public DirectorySource(string directory, Options? opts = null)
5858
Opts = opts ?? new Options();
5959
}
6060

61-
/// <summary>
62-
/// Enumerate the matched files as <see cref="FileSource"/> instances, sorted
63-
/// by full path (ordinal). The sort happens on the full path so that nested
64-
/// directory traversal is also deterministic.
65-
/// </summary>
6661
/// <summary>
6762
/// How two resolved directory paths are compared when deciding "is this an
6863
/// ancestor I have already walked".
@@ -81,6 +76,11 @@ public DirectorySource(string directory, Options? opts = null)
8176
private static readonly StringComparer PathComparer =
8277
OperatingSystem.IsLinux() ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase;
8378

79+
/// <summary>
80+
/// Enumerate the matched files as <see cref="FileSource"/> instances, sorted
81+
/// by full path (ordinal). The sort happens on the full path so that nested
82+
/// directory traversal is also deterministic.
83+
/// </summary>
8484
public IEnumerable<FileSource> Expand()
8585
{
8686
IEnumerable<string> files = Collect(Directory, new HashSet<string>(PathComparer))
@@ -193,17 +193,15 @@ private static string RealPath(string path)
193193
current = Path.Combine(current, segment);
194194
try
195195
{
196-
// returnFinalTarget walks a chain of links in one call; the bound is the
197-
// OS's own, and a link cycle here surfaces as an IOException we fall back on.
198-
var target = System.IO.Directory.ResolveLinkTarget(current, returnFinalTarget: true)
199-
?? System.IO.File.ResolveLinkTarget(current, returnFinalTarget: true);
200-
if (target is not null)
201-
{
202-
var t = target.FullName;
203-
current = Path.IsPathRooted(t)
204-
? Path.GetFullPath(t)
205-
: Path.GetFullPath(Path.Combine(Path.GetDirectoryName(current) ?? root, t));
206-
}
196+
// returnFinalTarget walks a chain of links (relative or absolute
197+
// targets, either) in one call and hands back an already
198+
// fully-qualified target. Every path this loop resolves is a
199+
// directory — Collect only ever calls RealPath on one it has
200+
// already confirmed exists — so Directory.ResolveLinkTarget alone
201+
// is authoritative; a link cycle here surfaces as an IOException
202+
// we fall back on.
203+
var target = System.IO.Directory.ResolveLinkTarget(current, returnFinalTarget: true);
204+
if (target is not null) current = target.FullName;
207205
}
208206
catch (IOException) { /* unresolvable — keep the lexical form for this segment */ }
209207
catch (UnauthorizedAccessException) { /* ditto */ }

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

Lines changed: 29 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,23 @@ async function stackForAgentContext(opts: InitOptions, prior: Manifest | undefin
218218
return resolveStack(opts.cwd, overrides);
219219
}
220220

221+
/** Writes `contents` to `path` (relative to `cwd`), unless `dryRun` — in which case
222+
* the write is skipped entirely and the caller still records what WOULD have
223+
* landed. Factors the mkdir+writeFile pair shared by every write site below. */
224+
async function writeUnlessDryRun(cwd: string, dryRun: boolean, path: string, contents: string): Promise<void> {
225+
if (dryRun) return;
226+
const abs = join(cwd, path);
227+
await mkdir(dirname(abs), { recursive: true });
228+
await writeFile(abs, contents, "utf8");
229+
}
230+
231+
/** "would be VERBED" during a dry run, plain VERBED otherwise — the one tense
232+
* marker every reported write shares, so each call site states only its own
233+
* past participle instead of writing out both tenses of the whole sentence. */
234+
function verbed(dryRun: boolean, pastParticiple: string): string {
235+
return dryRun ? `would be ${pastParticiple}` : pastParticiple;
236+
}
237+
221238
async function writeAgentContext(opts: InitOptions, result: InitResult): Promise<void> {
222239
warnIfMonorepoSubdir(opts, result);
223240
const prior = await readManifest(opts.cwd);
@@ -255,34 +272,23 @@ async function writeAgentContext(opts: InitOptions, result: InitResult): Promise
255272
const dryRun = opts.printOnly === true;
256273

257274
for (const w of writes) {
258-
if (!dryRun) {
259-
const abs = join(opts.cwd, w.path);
260-
await mkdir(dirname(abs), { recursive: true });
261-
await writeFile(abs, w.contents, "utf8");
262-
}
275+
await writeUnlessDryRun(opts.cwd, dryRun, w.path, w.contents);
263276
result.created.push(w.path);
264277
}
265278
for (const c of conflicts) {
266-
if (!dryRun) {
267-
const abs = join(opts.cwd, c.newPath);
268-
await mkdir(dirname(abs), { recursive: true });
269-
await writeFile(abs, c.contents, "utf8");
270-
}
279+
await writeUnlessDryRun(opts.cwd, dryRun, c.newPath, c.contents);
271280
result.created.push(c.newPath);
272281
// Past tense only when it actually happened — a dry run that reports "written
273282
// to <path>.new" is claiming an edit-preserving side effect the user can go
274283
// look for and will not find.
275284
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}`,
285+
`${c.path} appears hand-edited; refreshed version ${verbed(dryRun, "written")} to ${c.newPath}`,
279286
);
280287
}
281-
if (!dryRun) {
282-
const manifestAbs = join(opts.cwd, AGENT_CONTEXT_MANIFEST_PATH);
283-
await mkdir(dirname(manifestAbs), { recursive: true });
284-
await writeFile(manifestAbs, JSON.stringify(decision.manifest, null, 2) + "\n", "utf8");
285-
}
288+
await writeUnlessDryRun(
289+
opts.cwd, dryRun, AGENT_CONTEXT_MANIFEST_PATH,
290+
JSON.stringify(decision.manifest, null, 2) + "\n",
291+
);
286292
result.created.push(AGENT_CONTEXT_MANIFEST_PATH);
287293

288294
for (const orphan of decision.removed) {
@@ -301,28 +307,20 @@ async function wireRootMemory(cwd: string, result: InitResult, dryRun = false):
301307

302308
// If neither root memory file exists, create CLAUDE.md (Claude Code's canonical) with the import.
303309
if (!claudeExists && !agentsExists) {
304-
if (!dryRun) await writeFile(claudePath, `# Project memory\n\n${ROOT_IMPORT_LINE}\n`, "utf8");
305-
result.created.push(
306-
dryRun
307-
? "CLAUDE.md (would be created with MetaObjects @import)"
308-
: "CLAUDE.md (created with MetaObjects @import)",
309-
);
310+
await writeUnlessDryRun(cwd, dryRun, "CLAUDE.md", `# Project memory\n\n${ROOT_IMPORT_LINE}\n`);
311+
result.created.push(`CLAUDE.md (${verbed(dryRun, "created")} with MetaObjects @import)`);
310312
return;
311313
}
312314
// Otherwise append the import to whichever exist (idempotent — never double-add).
313315
for (const [path, exists] of [[claudePath, claudeExists], [agentsPath, agentsExists]] as const) {
314316
if (!exists) continue;
315317
const body = await readFile(path, "utf8");
316318
if (body.includes(ROOT_IMPORT_LINE)) continue;
317-
if (!dryRun) await writeFile(path, `${body.replace(/\n*$/, "\n")}\n${ROOT_IMPORT_LINE}\n`, "utf8");
319+
const target = path.endsWith("AGENTS.md") ? "AGENTS.md" : "CLAUDE.md";
320+
await writeUnlessDryRun(cwd, dryRun, target, `${body.replace(/\n*$/, "\n")}\n${ROOT_IMPORT_LINE}\n`);
318321
// Past tense only when it actually happened — this one mutates a file the user
319322
// 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-
);
323+
result.warnings.push(`${verbed(dryRun, "wired")} ${ROOT_IMPORT_LINE} into ${target} so the MetaObjects context loads`);
326324
}
327325
}
328326

0 commit comments

Comments
 (0)