agent-manager · pr 110 · review round three

Four more, including one my own fix did not fix

All four reproduced. One of them — the version-cache race — I first tried to fix with a compare-and-set, measured it, and found it does not work. That one is now narrowed and stated instead of claimed.

1. A symlink escaping the trusted root

The recorded path is only checked lexically; Claude applies inherited trust to the resolved directory. Refusing keeps the boundary the recorded path already implies, and needs no per-launch write to fix.

workspaces/link-out -> /home/node/local/outside-target

before: Accessing workspace: …/outside-target
        ❯ 1. Yes, I trust this folder        (task queued behind it)

after:  {"error":"link-out resolves to /home/node/local/outside-target, outside
         the workspaces root — a session has to run inside it. Point the session
         at a folder in the tree, or copy what you need into one."}

All four boundary cases:

deep/a/b                      ran its task     (ordinary child)
link-in  -> inside-target     ran its task     (symlink, stays inside)
link-out -> outside-target    refused, loudly  (lexically inside, canonically outside)

2. Legal TOML the regex could not see

Appending a duplicate table makes a file Codex refuses to load, so this corrupts a config rather than just showing a dialog. Detection now parses the grammar.

[projects."/work/legal"] # retained comment
trust_level = "trusted"

end-of-line regex : not found -> appended a second table
                    tomllib: Cannot declare ('projects', '/work/legal') twice
real TOML parse   : found -> not one byte added

3. The race my compare-and-set did not catch

I tried the CAS first. Measured with the review's own repro, it fails: the interceptor fires, our rename lands after it, and re-reading afterwards only shows our own snapshot. So the claim is narrowed rather than the code pretending.

4 MB cache at 0.200.0/10:00; interceptor writes 0.201.0/11:00 when .am-tmp appears

interceptorFired = true
final file        = latest 0.200.0, dismissed 0.200.0
                    -> their refresh WAS lost; a post-rename compare-and-set
                       cannot see it, because by then the file is our snapshot

What is guaranteed now: the file ends self-consistent, so the modal cannot open. What is accepted: one refresh can be lost, costing a delayed notification that Codex's next check repairs. The comment, the doc and the test all say this. .claude.json holds state the operator cannot reconstruct, which is why nothing rewrites that one.

3b. An unknown running version dismissed an update

refreshVersions() is async and the server serves requests before it finishes, so this really happens at startup. My previous test passed for the wrong reason — it pre-set the dismissal. Fresh fixture now.

isNewer('0.200.0', null) === true          (unchanged: the guard is in the caller)
dismissCodexUpdatePrompt(null)  -> false   (fresh fixture: dismissed_version stays null)

4. Descriptions that no longer matched

The runner.js comment still described the -c override and claimed neither adapter writes CLI state on the launch path, while the code appends to config.toml. Corrected, along with the PR body, which was still round one's.