Eight Review Findings, One Bug: What `merry init` Taught Me About Paths

I shipped a setup command. The reviewer found eight problems with it. Six of them were the same problem.

I maintain merry, a small Dart CLI that lets you define script shortcuts in pubspec.yaml and run them with merry build instead of typing the whole command. It's a maintained fork of derry. Nothing exotic.

The one thing it couldn't do was set itself up. You installed it, and then you hand-edited pubspec.yaml to add a scripts: key, and then you wrote a script file from scratch. For a tool whose entire pitch is "stop typing long commands," making people type YAML by hand was a bit rude.

So: merry init. Detect what the project is, write a starter script file, link it from the manifest. A weekend feature, or so I assumed.

The Part That Was Actually Easy

Detection turned out to be the boring half, which is the correct outcome. The rule I settled on was: generate only what the project can actually run.

No flutter build apk unless there's an android/ directory. No generate script unless build_runner is a dependency. A Flutter plugin isn't runnable itself, so its dev script runs the example/ app with a (workdir), and it gets no build targets at all, because its android/ directory is a native implementation, not an app.

That last one bit me later, so hold onto it.

For a plain Dart package, dev is dart run — but only when bin/<package name>.dart exists. I checked this rather than assuming, and it's a good thing:

$ dart run   # with bin/other_name.dart
Could not find `bin/sample_cli2.dart` in package `sample_cli2`.

A bare dart run resolves that one path and nothing else. Generating dev: dart run for a package whose entrypoint is named differently would have produced a script that fails on first use — the exact opposite of what a setup command is for.

Then I Wrote to the Filesystem

Here's the shape of the dangerous part. init has to write two files: a script file, and a modified pubspec.yaml. The script file's path is user-controlled — it comes from the scripts: key, which the user may already have set to something.

My first version checked that path the obvious way:

final scriptsPath = path.normalize(path.join(projectPath, target));
if (!path.isWithin(projectPath, scriptsPath)) {
  throw MerryError(type: ErrorCode.invalidScripts);
}

Escapes the project? Reject. That handles scripts: ../../etc/something. I added a check for scripts: pubspec.yaml specifically, because writing the template there would replace the manifest with a script file. I added a check for directories, because you can't write to one. I wrote tests for all three and they passed.

I opened the PR. Codex reviewed it and immediately posted a P1.

Round 1: The Check That Only Ran Sometimes

The finding: my symlink containment check only ran when the target already existed as a file. If it didn't exist, it was skipped.

I didn't believe it until I ran it:

$ ln -s /tmp/OUTSIDE.yaml link.yaml     # dangling — target doesn't exist yet
$ merry init
merry ERR! It should be a map of scripts or a file path.

$ cat /tmp/OUTSIDE.yaml
# Generated by `merry init`. Safe to edit.

The error message is the funny part. It did reject the config — after writing the file. My validation ran on the way out, and the write had already happened on the way in.

Two shapes, same hole: a dangling link, and a missing file underneath a linked directory. Both read as notFound, both skipped the check, both had the write follow the link out of the project.

Three more findings came in the same batch. A pubspec.yaml that is itself a symlink, with scripts: pointing at a second symlink to the same file — my equality check compared a canonicalized path against a raw one, missed the match, and overwrote the manifest. A manifest ending in ... — the YAML document terminator — got the new key appended after the document ended, leaving it unparseable. And a plugin with a lib/main.dart got classified as an app.

Four findings. I fixed them one at a time, which was my second mistake.

Round 2: The Fix That Opened the Next Hole

Push, re-trigger, two more findings.

A hard link to pubspec.yaml. Two names, one inode — and no amount of path resolution tells them apart, because both names are equally real. ln pubspec.yaml hard.yaml, then scripts: hard.yaml, and the confirmed write truncates the manifest through its second name.

And ... # end — a document terminator with a trailing comment. My exact-string match for ... didn't catch it.

That's when the pattern got embarrassing enough to see. Every finding was the same sentence with different nouns:

I made a decision about the path as it was written, when the thing that matters is where the write would land.

scripts: pubspec.yaml, a symlink onto the manifest, a hard link onto the manifest — these are three spellings of one situation. A path that escapes lexically, a symlink that escapes at resolve time, a dangling link that escapes when created — three spellings of another. I had been writing one guard per spelling, and each guard's edge was the next finding's opening.

The Fix, Or: Ask the Filesystem, Not the String

The replacement is one resolution step before any decision:

final resolvedProject = await Directory(projectPath).resolveSymbolicLinks();
final resolvedScripts = await _resolveWriteTarget(scriptsPath);
final resolvedPubspec = await _resolveWriteTarget(pubspec.filePath);
if (!path.isWithin(resolvedProject, resolvedScripts) || path.equals(resolvedScripts, resolvedPubspec)) {
  throw MerryError(type: ErrorCode.invalidScripts);
}

Both sides resolved, then compared. Escaping the project, landing on the manifest, and naming pubspec.yaml outright become one rejection — and the special case I'd written for scripts: pubspec.yaml was deleted rather than kept alongside. A fix that lets you remove code is usually the right one.

The helper exists because the obvious API can't do this job:

Future<String> _resolveWriteTarget(String target) async {
  var current = path.normalize(target);

  for (var hop = 0; hop < _maxLinkHops; hop++) {
    if (FileSystemEntity.isLinkSync(current)) {
      final destination = await Link(current).target();
      current = path.normalize(path.join(path.dirname(current), destination));
      continue;
    }

    // Walk up to something that exists, so the ancestors can be resolved.
    final segments = <String>[];
    var ancestor = current;
    while (!await FileSystemEntity.isDirectory(ancestor) && path.dirname(ancestor) != ancestor) {
      segments.insert(0, path.basename(ancestor));
      ancestor = path.dirname(ancestor);
    }
    // ...resolve the ancestor, re-attach the segments, repeat until it stops moving
  }
}

File.resolveSymbolicLinks() throws when any component is missing — which is precisely the case that matters here. A file you are about to create does not exist yet. So the walk goes up to the deepest thing that does exist, resolves that, and re-attaches the rest.

The hard link needed something else entirely, and the answer is unsatisfying: dart:io exposes no inode. FileStat gives you mode, size, and timestamps — nothing that identifies the file itself. So identity is inferred from content: same length, then same bytes.

That over-rejects a script file that happens to duplicate the manifest byte for byte. I decided I could live with that, because nobody writes that file on purpose, and the failure it prevents destroys a manifest. When you can't be exact, pick which direction to be wrong in.

Round 3: The One I Found by Trying to Break My Own Test

Two more findings landed after I'd merged the PR. Both real — an indented ... inside a block scalar was still read as a terminator, and a dangling symlink among the target's ancestors leaked a raw PathNotFoundException.

But the thing I actually want to tell you about happened while I was verifying the fixes.

I have a habit of breaking my own code to check that a test notices. Delete the guard, run the test, confirm it fails. If it passes, the test was decorative.

I had written this to prove ...abc isn't a terminator:

write('pubspec.yaml', 'name: demo\ndescription: "...abc"\n');

Then I deliberately made the predicate too greedy — matching any line starting with ... — and ran the suite.

Everything passed.

Which meant the test was proving nothing. description: "...abc" doesn't start with ...; it starts with description:. The line never reached the check at all. I'd written a test that could never fail, and it had been sitting in a green suite looking exactly like a real one.

The fix was to make the line actually start with the dots:

write('pubspec.yaml', 'name: demo\ndescription: |\n  ...trailing off\n');

Now the greedy version fails, and the test means something.

The same habit paid off twice more. For the dangling-ancestor fix I'd written two changes, and when I disabled each one separately, each still passed on its own — they were redundant. I kept the two-line one and deleted the nine-line one. Without that check I'd have shipped both and never known.

What I'd Take to the Next One

Findings arriving one at a time is a signal, not a queue. Six findings, one root cause. If I'd stopped after the second and asked what they had in common instead of fixing them in order, I'd have skipped four rounds. The review loop rewards patching, and patching is what keeps you in the loop.

Validating after the write is not validating. My getScripts() call at the end felt like a safety net. It caught every bad case — and reported each one after the damage. Order matters more than coverage.

A test you haven't seen fail is a guess. Not a new idea, but I'd been applying it to logic and not to reachability. The assertion was right. The input never got there.

Fail closed when you can't be exact. No inode? Compare content and accept the false positive. The two error directions are almost never equally bad — decide which one you can afford.

There's also a smaller, dumber lesson: near the end, the review bot hit its usage limit and the last confirmation round never ran. That was fine, because by then the reproductions and the mutation checks were doing the actual work — the bot had been finding things I could verify myself, not things I was taking on faith. A reviewer you can't check is a reviewer you're trusting; a reviewer whose findings you reproduce is a reviewer you're using.

merry init shipped in 2.2.0. It's 408 lines with 27 tests, which is more than I expected for "write a config file," and almost all of the extra is about not writing it to the wrong place.

---

merry is on pub.dev and GitHub. If you find a case I missed, the issues page is right there — this post is fairly strong evidence that I'll take it seriously.