Repository navigation
fix(agent-sync): surface workspace resolution failures, stop tmp path poisoning - #1056
Conversation
… poisoning Root cause of "my agent exists on disk but genie spawn can't find it": startAgentSync() in serve.ts silently returned null when findWorkspace() returned null, disabling the entire auto-discovery subsystem (initial sync + file watcher) for the whole serve lifetime. Users saw new agents in workspace/agents/ never register and received no diagnostic anywhere. The reason findWorkspace() returned null in practice: test runs persist their tmpdir workspace paths to ~/.genie/config.json via saveWorkspaceRoot(). Once a serve boots from outside the real workspace, walk-up fails, the fallback hits the stale tmp path (now removed), existsSync returns false, returns null. Fixes: - workspace.ts: saveWorkspaceRoot() refuses to persist paths under os.tmpdir(), so test runs can't poison the global config. - workspace.ts: loadWorkspaceRoot() self-heals — if the saved path no longer has a .genie/workspace.json marker, scrub it from config instead of returning a stale fallback forever. - workspace.ts: GENIE_HOME is now resolved lazily via genieHome() so tests can override it via process.env.GENIE_HOME per-test. - serve.ts: startAgentSync() logs loudly when workspace is unresolved, when sync has per-agent errors, and when the watcher fails to start. Also includes the resolved workspace in the success log so "which workspace did serve attach to" is visible at a glance. - workspace.test.ts: two new tests covering tmp-path guard and stale-path self-heal; tests now isolate GENIE_HOME so they don't touch the developer's real ~/.genie/config.json.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request enhances workspace configuration handling by introducing a self-healing mechanism for stale workspace roots and preventing the persistence of temporary paths to the global configuration. Additionally, it improves the observability of the genie serve command through more detailed logging and warnings during agent synchronization. Review feedback recommends using realpathSync to correctly identify temporary directories on systems with symlinked temp paths and suggests dynamically resolving the configuration file path in user-facing warnings for better accuracy when GENIE_HOME is customized.
| function isTempPath(root: string): boolean { | ||
| const normalizedTmp = resolve(tmpdir()); | ||
| const normalizedRoot = resolve(root); | ||
| return normalizedRoot === normalizedTmp || normalizedRoot.startsWith(normalizedTmp + sep); | ||
| } |
There was a problem hiding this comment.
On macOS and some other Unix systems, the system temporary directory (e.g., /tmp or /var/folders/...) is often a symbolic link. Since resolve() does not resolve symlinks, isTempPath might return false even for paths within the temporary directory if they are accessed via their real (canonical) path. This would bypass the protection against 'poisoning' the global config. Using realpathSync ensures a robust comparison. Note: Since the top-level imports cannot be modified in this diff, require is used here for a self-contained suggestion.
function isTempPath(root: string): boolean {
try {
const { realpathSync } = require('node:fs');
const normalizedTmp = realpathSync(tmpdir());
const normalizedRoot = realpathSync(root);
return normalizedRoot === normalizedTmp || normalizedRoot.startsWith(normalizedTmp + sep);
} catch {
return false;
}
}References
- Use dynamic imports (require() or import()) to break circular dependency cycles that would otherwise occur at module load time.
| if (!ws) { | ||
| // Loud failure — silent return used to hide the whole discovery subsystem | ||
| // when serve booted from outside a workspace (or with a stale saved root). | ||
| console.warn(' Agent sync: DISABLED — no workspace found from cwd or ~/.genie/config.json'); |
There was a problem hiding this comment.
The warning message hardcodes ~/.genie/config.json, which may be inaccurate if the GENIE_HOME environment variable is set. It's better to dynamically resolve the config path to provide accurate guidance to the user.
| console.warn(' Agent sync: DISABLED — no workspace found from cwd or ~/.genie/config.json'); | |
| console.warn(" Agent sync: DISABLED — no workspace found from cwd or " + join(genieHome(), 'config.json')); |
…path in serve warning Follow-up to #1056 addressing two review comments from gemini-code-assist. 1. isTempPath() now uses realpathSync to canonicalize both tmpdir() and the candidate root before prefix-comparing. On macOS /tmp is a symlink to /private/tmp and tmpdir() returns /var/folders/..., so the previous path.resolve()-only check would miss any literal /tmp/... path and bypass the guard against poisoning the global workspaceRoot config. Fails CLOSED on canonicalization errors (treats as temp → refuses to persist) since the whole purpose of this guard is to be conservative. 2. startAgentSync() now builds its "no workspace found" warning using genieHome() (newly exported) instead of hardcoding ~/.genie/config.json, so users who set GENIE_HOME see the actual path in the fix guidance. Also adds: - A regression test using a real symlink within the test's tmpdir to verify canonicalization works end-to-end (skips gracefully on CI environments that disallow symlinks). - A sanity test that genieHome() honors the GENIE_HOME env override.
The bug I hit
I have a real agent on disk at
workspace/agents/omni/AGENTS.md. Every other agent in that directory shows up ingenie ls. Omni doesn't.genie spawn omnisays:Running
genie dir syncmanually instantly registered 15 agents that had been invisible — omni plustotvs,helena,researchers, 5 sub-agents, etc. So the sync code works. Something was preventing it from running on serve startup.Root cause
startAgentSync()inserve.tssilently returnednullwhenfindWorkspace()returnednull. That disabled the entire auto-discovery subsystem — both the startup sync and the file watcher — for the whole serve lifetime. No warning, no log, no entry ingenie serve status.Why was
findWorkspace()returning null? Because~/.genie/config.jsonhad this persisted from an old test run:saveWorkspaceRoot()had written it,/tmpgot cleaned, and the fallback path infindWorkspace()has been returning a dead path ever since. Anygenie servebooted from outside the real workspace walked up, failed, fell back to the stale path,existsSyncreturned false, returned null,startAgentSyncsilently exited. For however long that serve instance ran.The fix
Three small, contained changes — all behind existing function boundaries:
src/lib/workspace.tssaveWorkspaceRoot()refuses to persist paths underos.tmpdir(). Test workspaces can no longer poison the global fallback. Future test runs write their tmp path and the guard drops it on the floor.loadWorkspaceRoot()self-heals — if the saved path no longer has a.genie/workspace.jsonmarker, scrub it from config instead of returning a dead fallback forever.GENIE_HOMEis now resolved lazily viagenieHome()instead of a module-levelconst. Tests can override it per-test viaprocess.env.GENIE_HOME. Without this change, the previous module-level capture meant test env overrides were silently ignored — I caught it because my first test run ended up scrubbing my real~/.genie/config.json(which, happy accident, was exactly the stale tmp path that originally caused this bug).src/term-commands/serve.tsstartAgentSync()logs loudly when workspace is unresolved, with actionable fix guidance, instead of silently returning null:syncResult.errors[]so a single agent with bad frontmatter doesn't vanish silently. Today those errors are collected bysyncAgentDirectory()and never printed on the serve-startup path.src/lib/workspace.test.tsGENIE_HOMEper-test so they don't touch (or get contaminated by) the developer's real~/.genie/config.json.Scope I deliberately did NOT include
While diagnosing this with the user, we identified 6 DX improvements. This PR implements only the 3 that directly address the silent-failure root cause. The other 3 — which are good ideas but larger surface-area changes — are left for follow-ups:
genie spawn <name>should fall back to scanning disk when PG returns null (friendlier error with "found but not registered" message).genie serve statusshould expose agent-sync/watcher health as a first-class line.genie lsshould have an "UNREGISTERED (on disk, not in directory)" section for agents that exist but failed validation.These are each worth a separate focused PR.
Verification
bun test src/lib/workspace.test.ts→ 12 pass, 0 fail (10 existing + 2 new)bun test src/lib/→ 1571 pass, 0 failbunx tsc --noEmit→ cleanbunx biome check src/lib/workspace.ts src/lib/workspace.test.ts src/term-commands/serve.ts→ 0 errors, 1 pre-existing warning onstartForegroundcomplexity (unrelated, confirmed present on basedevbefore my changes viagit stashround-trip)genie dir syncfrom the real workspace root registered 15 previously-invisible agents including omni;genie spawn omninow resolvesTest plan
bun test src/lib/workspace.test.tspassesbun testpasses (2073/2073)bunx tsc --noEmitcleanbunx biome check0 errors on changed filesgenie servefrom outside a workspace → seeAgent sync: DISABLEDwarning with fix guidancegenie servefrom inside a workspace → seeAgent sync: up to date (workspace: …)linegenie dir syncor restart serve → see the per-agent error line instead of the file silently going missing