Repository navigation
feat(agent-sync): trigger migration + always upsert (wish dir-sync-frontmatter-refresh group 3) - #1196
Conversation
…ontmatter-refresh group 3) Wave 2 / Group 3. Closes the loop the user originally hit today: editing an AGENTS.md field silently dropped on the floor because `genie dir sync` reported "Unchanged" and skipped the DB write. Core behavior change: 1. **First sync on an unmigrated agent triggers migration** via `migrateAgentToYaml` (Group 2, already on dev as #1193). The agent's AGENTS.md frontmatter is moved into `agents/<name>/agent.yaml`; AGENTS.md.bak preserves the original byte-for-byte; AGENTS.md loses its frontmatter. 2. **agent.yaml becomes the canonical source post-migration.** Sync reads config via `parseAgentYaml` and drops back to legacy `parseFrontmatter(AGENTS.md)` only when neither yaml nor frontmatter exists (defaults-only registration). 3. **"Unchanged" path eliminated.** No change detection, no short-circuit — sync always upserts every reached agent's row. Lost edits are expensive; per-run writes are cheap. Surface changes in `src/lib/agent-sync.ts`: - `SyncResult.unchanged: string[]` removed. A new `SyncResult.migrated: string[]` tracks this-run migrations. - `syncSingleAgent` gains a Phase 0 migration trigger. Its dbRow fallback is derived from the current DirectoryEntry via a small `dbRowFromEntry` helper (honors the frontmatter-wins precedence from Group 2's migrator). - `syncSingleAgentByName` returns `'synced'` (was `'unchanged'`) for the non-change path; `processWatchedAgent`'s filter updated to match. - `printSyncResult` drops the "Unchanged: ..." line and renames the closing summary to `Synced: N agent(s), M removed.` `src/lib/agent-identity-sync.test.ts`: - Old "sync detects AGENTS.md frontmatter changes" test updated to edit agent.yaml instead (post-migration canonical source). - Old `expect(result.unchanged)` assertion replaced with `expect(result.updated)` (the existing agent still lands there). - Four new Group 3 acceptance tests: migration trigger, migration idempotency, edit-yaml-then-resync reproducer, SyncResult shape no longer carries `unchanged`. Scope guard verified: `grep 'Unchanged:' src/term-commands/dir.ts src/lib/agent-sync.ts` returns zero hits in production code paths. Full suite: 2678 pass / 0 fail. typecheck clean. Biome clean modulo a complexity warning on `syncSingleAgent` (18 vs max 15) — the function grew by one branch for the migration phase; pre-existing `startDaemon`-style warning pattern, not a lint failure. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the dir-sync-frontmatter-refresh feature, which transitions agent configuration from AGENTS.md frontmatter to agent.yaml as the canonical source. It eliminates the "unchanged" skip path in the sync process, ensuring that the database is always updated to reflect the current state of files on disk. The changes include a migration trigger that converts existing frontmatter to YAML and updates to the sync logic to prioritize agent.yaml. Feedback identifies two areas for improvement: a block of code calling computeResolvedMetadata that appears to be dead code as its result is ignored, and an unreachable return statement in syncSingleAgentByName due to the new always-upsert logic.
| if (workspaceRoot) { | ||
| const ctx = buildResolveContext(workspaceRoot, agent.name); | ||
| _resolvedMeta = computeResolvedMetadata(fm as Record<string, unknown>, ctx); | ||
| computeResolvedMetadata(configFields.rawForResolution, ctx); | ||
| } |
There was a problem hiding this comment.
The call to computeResolvedMetadata is a pure function call whose result is ignored. Since it has no side effects, this block (including the context building) appears to be dead code. If the intention was to validate the configuration, note that computeResolvedMetadata does not throw on resolution errors. If this logic is no longer needed for the sync process, it should be removed to improve maintainability. Removing this would also allow for the removal of the rawForResolution field from the ConfigFields interface and the computeResolvedMetadata and buildResolveContext helper functions.
| if (result.migrated.length > 0) return 'migrated'; | ||
| if (result.updated.length > 0) return 'updated'; | ||
| return 'unchanged'; | ||
| return 'synced'; |
There was a problem hiding this comment.
This return statement is unreachable. syncSingleAgent always populates either result.registered or result.updated for any agent it processes (see lines 440 and 460). Consequently, the preceding if conditions will always catch the result, and this line will never be reached. You might want to simplify the logic or ensure that syncSingleAgent can indeed reach a state where neither list is populated if that was the intention.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f36d12e09
ℹ️ 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".
| if (frontmatter !== null) { | ||
| const existingEntry = await directory.resolve(agent.name); | ||
| const dbRow = existingEntry && !existingEntry.builtin ? dbRowFromEntry(existingEntry.entry) : undefined; | ||
| const migrationResult = await migrateAgentToYaml(agent.dir, dbRow); |
There was a problem hiding this comment.
This branch now migrates whenever extractFrontmatterFromAgentsMd finds any frontmatter fence, but migrateAgentToYaml throws on malformed or empty frontmatter mappings; when that happens, syncSingleAgent records an error and skips the agent entirely. Before this change, sync used parseFrontmatter, which is intentionally tolerant and kept such agents discoverable with defaults, so this is a regression for workspaces that contain invalid/blank AGENTS.md frontmatter (for example ---\n---).
Useful? React with 👍 / 👎.
Wave 2 / Group 3 of wish `dir-sync-frontmatter-refresh`. Depends on Groups 1 (#1191) + 2 (#1193), both already on dev.
Closes the original user-reported bug
`genie dir sync` reports "Unchanged" and skips the DB write when an agent's frontmatter changes, so edits like `team: simone` sit on disk unread. Root cause: the sync helper ran a JSON-equality diff and short-circuited when nothing diverged — but the diff itself never noticed the file edit because of parse-order. Net effect: files-are-source-of-truth promise was silently broken.
What changed
`src/lib/agent-sync.ts`
`src/lib/agent-identity-sync.test.ts`
Scope-guard check
```
grep 'Unchanged:' src/term-commands/dir.ts src/lib/agent-sync.ts → zero hits in production code paths
```
(The literal still appears in a code comment explaining what was removed — per wish guidance, production code paths must have zero hits, which they do.)
Test plan
What does NOT change
🤖 Generated with Claude Code