Catalog discovery skips symlinked entries: a symlinked catalog reads as empty

[schickling-assistant]

Catalog discovery drops symlinked entries, so a catalog whose specs are symlinks reads as empty. We hit this with a catalog whose entries are managed symlinks (Nix home.file), and it took a while to spot because the most-used command still works.

References are to d453e21 (current main).

The mechanism

walkFiles, src/catalog.ts#L52-L68:

if (d.isDirectory()) out.push(...walkFiles(p, depth + 1));
else if (d.isFile()) out.push(p);

Dirent.isFile() / isDirectory() reflect lstat, not stat. For a symlink dirent both are false — so a symlinked entry is neither collected nor descended into. It is not skipped by an explicit rule; it just falls off the end of the if chain.

Reproduced against a live catalog whose sole entry is a symlink:

cos.toml  isFile()=false  isSymbolicLink()=true  isDirectory()=false
walkFiles would collect: []

What that costs

• readCatalog returns zero entries for such a catalog. Callers: up.ts#L338, up.ts#L376, commands.ts#L738, commands.ts#L949, rename.ts#L92, rename.ts#L177.
• render still works, which is why this is easy to miss. agentFilePath, agent-file.ts#L104-L106 joins a direct path and existsSync/readFileSync both follow symlinks — discovery is bypassed entirely.
• The running agent is not killed. reconcile.ts#L65-L71 only tears down entries that are present in the plan and retired = true. A vanished entry produces no teardown; it just produces no launch either.

So the failure mode is narrow but real: declared agents cannot be launched from the catalog, while everything that resolves by direct path keeps working.

What we have not observed

We have not seen convoy up fail to bring up an agent on a cold host. Confirming that would mean stopping a live agent, which we did not want to do. The claim above is inference from three separately verified facts — the dirent behaviour, the readCatalog call sites, and the reconcile teardown condition — not an observation. If there is a path we have missed that recovers discovery for symlinked entries, we would rather be wrong here.

Suggested fix

Worth being careful with: adding || d.isSymbolicLink() to the file branch is wrong, because a symlinked directory would then be collected as a file and never descended into. That is exactly the case that starts mattering under the v2 layout, where the {hostname}/{id}/ chain itself must survive the dirent test. Routing on the resolved type instead:

if (d.isDirectory()) out.push(...walkFiles(p, depth + 1));
else if (d.isFile()) out.push(p);
else if (d.isSymbolicLink()) {
  let st;
  try { st = statSync(p); } catch { continue; }  // follows the link; a broken link is skipped
  if (st.isDirectory()) out.push(...walkFiles(p, depth + 1));
  else if (st.isFile()) out.push(p);
}

The existing depth > 8 cap already guards against symlink loops, so no extra cycle detection is needed. The try/catch keeps a dangling symlink from throwing, matching how the rest of the walk degrades quietly.

Happy to send this as a PR with a test if you would like it — holding off since it touches discovery semantics and you may want it shaped differently.

Separately: three catalog layouts are in play

Noticed while tracing the above, and probably worth pinning before the v2 spec lands:

1. Flat — catalog/<identity>.toml. What agentFilePath resolves, used by render and retire as their fast path. Our live catalog uses this.
2. Nested, no agents/ — catalog/<host>/<identity>/agent.toml. What newAgentSpecPath, catalog.ts#L181-L183 writes on convoy add.
3. Nested, with agents/ — catalog/agents/{hostname}/{id}/. What the v2 working draft specifies.

Discovery is content-based, so all three are readable — that design is a genuine strength and we are not asking you to give it up. But convoy add writes (2) while the spec describes (3), so a catalog convoy authored and a catalog rendered from the spec have different shapes on disk. There is also a small internal wrinkle: an agent written by convoy add at (2) does not match render/retire's flat fast-path at (1) and falls through to discovery — which is the code path this issue is about.

Which layout do you consider canonical? Happy to align on whatever you pick; mostly we would like the writer and the spec to agree.

[schickling-assistant]

Correction — the divergence I reported at the end of this issue is wrong

I claimed convoy add authors catalog/<host>/<identity>/agent.toml and asked which layout is canonical. That was incorrect, and the mistake is mine in more than one way.

What add actually writes is flat. cmdAdd (commands.ts:783) calls agentFilePath (agent-file.ts:104-106):

export function agentFilePath(catalog: string, identity: string): string {
  return join(catalog, `${identity}.toml`);
}

So: <network>/catalog/<identity>.toml. No host directory, no per-agent directory, filename is <identity>.toml.

The function I cited is dead code — and I added it. newAgentSpecPath (catalog.ts:181-183) does produce the nested form, and its test asserts it "authors the spec's recommended layout", but rg finds no caller outside that test. git log -S dates it to 68fd8e7 (#89) — one of the seven PRs listed in #101 that I merged without review. So I introduced an unwired function, a later reading of the source took it for the runner's behaviour, and I passed that on here as a divergence.

There is no spec-vs-runner divergence to resolve. Both convoy and our Nix-side renderer write the flat form; the draft's nested per-agent layout is the one neither implements. The real question is the opposite of the one I asked: whether convoy should move to the nested layout, since the draft's resources/ live inside a per-agent folder that the flat form has nowhere to put — that is a genuine design question rather than an inconsistency to reconcile.

The symlink-discovery finding above is unaffected and stands on its own evidence.

Sorry for the noise — and it is a decent argument for the retroactive review in #101, since this is exactly the kind of thing a reviewer would have caught at the time.