Repository navigation
feat(dir-edit): write agent.yaml first, then sync (wish dir-sync-frontmatter-refresh group 4) - #1197
Conversation
…tmatter-refresh group 4) Wave 3 / Group 4. Makes `genie dir edit` honor the "files are source of truth" principle established in Groups 1-3. Depends on G1/G2/G3, all already on dev. Behavior change: - `dir edit` now writes every flag into `agents/<name>/agent.yaml` FIRST, then triggers a single-agent sync so the DB picks up the new values via the refactored sync path from G3. - No direct `agent_templates` / `agents` writes from the edit handler anymore. The yaml file is authoritative; the DB is a projection. - If the agent hasn't been migrated yet (no `agent.yaml` on disk), `handleEdit` triggers migration first so the file exists before the write+sync cycle. - The legacy `directory.syncFrontmatterToDisk` call is gone — there's no frontmatter to sync back to post-migration (AGENTS.md is body-only). Code surface: - `src/term-commands/dir.ts` — rewritten `handleEdit` per the flow above. New imports: `migrateAgentToYaml`, `parseAgentYaml`, `writeAgentYaml`, `syncSingleAgentByName`. - `src/lib/agent-sync.ts` — export `syncSingleAgentByName` (was private). Used by the new edit flow. Watcher/file-sync call sites unchanged. Tests (`src/term-commands/dir-edit.test.ts` — new, 7 cases): - `--model` lands in yaml and DB - `--allow <tools>` → nested `permissions.allow` array in yaml - `--permission-preset` → `permissions.preset` in yaml - `--sdk-permission-mode` → `sdk.permissionMode` in yaml - yaml write precedes DB write (file on disk is correct even if sync were skipped) - 8-way concurrent `writeAgentYaml` calls leave a parseable, complete config (never a truncation) - combined flags all land in a single write Wish scope-guard verified: `grep 'UPDATE agent_templates' src/term-commands/dir.ts` returns zero hits. Validation: 7/7 new tests pass, typecheck clean, biome clean modulo the same pre-existing complexity warning on `syncSingleAgent` carried forward from G3. Pre-push full suite: see CI on this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 refactors the dir edit command to follow a 'file-first' synchronization flow, where updates are written to an agent.yaml file before being synchronized to the database. It includes the removal of the syncFrontmatterToDisk function, updates to syncSingleAgentByName to support this flow, and a new integration test suite. Feedback highlights that the --dir and --repo flags may now be non-functional or inconsistent, and that the new logic introduces a regression for global agents edited outside of a workspace. Additionally, it is recommended to reuse the dbRowFromEntry helper to avoid manual object construction during migration.
| if (options.dir) updates.dir = resolvePath(options.dir); | ||
| if (options.repo) updates.repo = resolvePath(options.repo); |
There was a problem hiding this comment.
The --dir and --repo flags are now effectively non-functional or inconsistent in this new flow:
--dir: Sincediris a derived field stripped fromagent.yaml(peragent-yaml.ts), and the code writes to the existingagentDir(line 269), providing a new path via--dirwill not update the agent's location in the database or move the files.--repo: While this is saved toagent.yaml, the sync process (specificallyreadYamlAsConfigFieldsinagent-sync.ts) does not currently read therepofield from YAML, instead overwriting the database entry with values derived from the git remote duringsyncSingleAgentByName.
If these flags are no longer intended to be editable via this command, they should be removed or the implementation should be adjusted to handle them (e.g., as a move operation for --dir).
| const ws = findWorkspace(); | ||
| if (ws) { | ||
| await syncSingleAgentByName(ws.root, name); | ||
| } else { | ||
| // Not in a genie workspace — skip sync, but the yaml write stands. | ||
| console.warn('Not in a genie workspace — agent.yaml updated on disk; run `genie dir sync` manually to propagate.'); | ||
| } |
There was a problem hiding this comment.
This logic introduces a regression for "global" agents that are not located within a workspace. Previously, directory.edit updated the database directly regardless of the workspace context. In the new flow, if findWorkspace() returns null, the sync step is skipped. While the agent.yaml is updated on disk, the database (which is used for agent resolution at runtime) remains stale, meaning the edit will have no effect for these agents until a manual sync is performed—which itself requires a workspace.
| const dbRow = { | ||
| team: resolved.entry.team, | ||
| model: resolved.entry.model, | ||
| description: resolved.entry.description, | ||
| color: resolved.entry.color, | ||
| provider: resolved.entry.provider, | ||
| promptMode: resolved.entry.promptMode, | ||
| permissions: resolved.entry.permissions, | ||
| disallowedTools: resolved.entry.disallowedTools, | ||
| omniScopes: resolved.entry.omniScopes, | ||
| hooks: resolved.entry.hooks, | ||
| sdk: resolved.entry.sdk as unknown as AgentConfig['sdk'], | ||
| }; |
There was a problem hiding this comment.
This block manually constructs a dbRow for migration, which duplicates the logic already present in the dbRowFromEntry helper function in src/lib/agent-sync.ts. To improve maintainability and ensure consistency, consider exporting dbRowFromEntry and reusing it here. If importing this helper creates a circular dependency cycle, use dynamic imports (require() or import()) to break it at module load time.
References
- Use dynamic imports (require() or import()) to break circular dependency cycles that would otherwise occur at module load time.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7660369cc9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Read the current yaml, apply the updates, write atomically via the lock. | ||
| const current = await parseAgentYaml(yamlPath); | ||
| const next: AgentConfig = { ...current, ...updates }; |
There was a problem hiding this comment.
Guard parseAgentYaml when migration skips no-frontmatter
When agent.yaml is missing, this path always calls migrateAgentToYaml(...) and then immediately parses agent.yaml, but migrateAgentToYaml returns reason: 'no-frontmatter' without creating the file for agents that have body-only AGENTS.md. In that valid legacy/minimal state, parseAgentYaml throws ENOENT and genie dir edit fails instead of updating the agent, which is a regression from the previous DB-edit flow.
Useful? React with 👍 / 👎.
| const ws = findWorkspace(); | ||
| if (ws) { | ||
| await syncSingleAgentByName(ws.root, name); |
There was a problem hiding this comment.
Preserve --repo edits instead of overwriting on sync
After writing YAML, the handler always calls syncSingleAgentByName, whose sync path computes repo from git/dir discovery (repoPath) rather than from agent.yaml fields; as a result, a --repo change is immediately overwritten in DB projection and users see a successful edit that did not actually apply. This makes genie dir edit --repo ... a no-op in normal workspace runs.
Useful? React with 👍 / 👎.
| const next: AgentConfig = { ...current, ...updates }; | ||
| await writeAgentYaml(yamlPath, next); |
There was a problem hiding this comment.
Do not silently drop --dir updates in yaml-first edit
next includes updates.dir, but writeAgentYaml strips derived keys like dir before serialization, so the new directory path is never persisted to disk; then sync re-derives dir from discovery and leaves the stored entry unchanged. Keeping --dir enabled with this flow reports success while discarding the user’s requested path change.
Useful? React with 👍 / 👎.
Wave 3 / Group 4 of wish `dir-sync-frontmatter-refresh`. Depends on Groups 1-3 (#1191 / #1193 / #1196 — all on dev).
Behavior change
`genie dir edit` now honors the "files are source of truth" principle from the wish: every flag writes to `agents//agent.yaml` FIRST, then triggers a single-agent sync to propagate into PG. No more direct `agent_templates` / `agents` writes from the edit handler.
Flow:
If not in a workspace: yaml is still written; warning printed advising manual `genie dir sync`.
What's new
Tests
New: `src/term-commands/dir-edit.test.ts` — 7 integration tests pinning every acceptance criterion:
Scope-guard check
```
grep 'UPDATE agent_templates' src/term-commands/dir.ts → zero hits
```
Test plan
What does NOT change
🤖 Generated with Claude Code