Repository navigation
feat(agent-yaml): YAML schema + parse/write library (wish dir-sync-frontmatter-refresh group 1) - #1191
Conversation
…ile.ts The PID-based lockfile helpers were private to claude-native-teams.ts and unusable by any other module that needs safe concurrent writes. Promote them to a shared module with named exports — preserves the existing semantics (5s timeout, 50ms poll with jitter, stale-PID detection via kill(pid, 0), force-acquire with warning after timeout) so claude-native-teams.test.ts passes unchanged. Prepares the ground for agent-yaml.ts (next commit) which needs the same lock semantics for atomic writes to agent.yaml.
Ships the Wave-1 module that every downstream group in the
dir-sync-frontmatter-refresh wish imports from:
- `AgentConfigSchema` — Zod schema mirroring DirectoryEntry field-for-field
with `.strict()` at every object level. Unknown top-level keys (including
out-of-scope `skill` and `extraArgs` that belong to the agent_templates
PG row, not the yaml file) fail with a field-named error.
- `parseAgentYaml(path)` — reads, parses via js-yaml, validates via Zod,
surfaces a descriptive Error on any failure.
- `writeAgentYaml(path, config)` — strips derived fields (name/dir/
registeredAt) before serialization, writes atomically (`${path}.tmp` +
rename) under `acquireLock` so concurrent writers and readers never
observe a partial/spliced file.
- `extractFrontmatterFromAgentsMd(content)` — pure helper that splits the
`---\n...\n---\n` fence off the top of an AGENTS.md string, preserving
the body byte-for-byte (CRLF, trailing newlines, Unicode).
No caller is updated in this commit — Groups 2+ consume the module.
25 cases pinning every acceptance criterion from the wish: - Round-trip: fully-populated config + empty config both survive writeAgentYaml → parseAgentYaml untouched; derived fields are stripped from the on-disk YAML and absent on re-parse. - Strict schema / scope guard: unknown top-level keys (`bogus`, `skill`, `extraArgs`) are rejected with the key named in the error. - Nested: `permissions.bashAllowPatterns` parses; `permissions.bashAllow` (wrong name) throws an unknown-field error pinned to `permissions`. - `extractFrontmatterFromAgentsMd` purity: CRLF preserved, trailing newlines preserved, Unicode preserved, missing/unclosed fence returns the full body, call is pure (no mutation). - Concurrency: two and eight parallel `writeAgentYaml` calls on the same path always leave the file parseable as EXACTLY one of the inputs — never a splice. - Malformed YAML / missing file / top-level array / empty file all produce clear errors (or an empty config for the empty-file case).
|
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 introduces a new agent-yaml module for robust agent.yaml schema validation, parsing, and atomic writing, alongside a shared lockfile utility extracted from existing logic. Comprehensive unit tests are added to verify round-trip consistency, frontmatter extraction, and concurrent write safety. Feedback identifies a high-severity issue in the PID liveness check where permission errors are incorrectly handled, which could lead to premature lock deletion. Additionally, the reviewer suggests improving the robustness of the lock acquisition logic by treating corrupt or empty lock files as stale rather than polling until timeout.
| function isPidAlive(pid: number): boolean { | ||
| try { | ||
| process.kill(pid, 0); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The isPidAlive function incorrectly returns false if process.kill(pid, 0) throws an error with code EPERM. An EPERM error indicates that the process exists but the current user does not have permission to signal it. In this case, the process is alive, and the lock should not be considered stale. Deleting a lock held by an active process can lead to race conditions and file corruption.
| function isPidAlive(pid: number): boolean { | |
| try { | |
| process.kill(pid, 0); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| function isPidAlive(pid: number): boolean { | |
| try { | |
| process.kill(pid, 0); | |
| return true; | |
| } catch (err) { | |
| return err instanceof Error && (err as any).code === 'EPERM'; | |
| } | |
| } |
| const holderPid = Number.parseInt(content.trim(), 10); | ||
| if (!Number.isNaN(holderPid) && !isPidAlive(holderPid)) { | ||
| // Holder is dead — remove stale lock and retry immediately |
There was a problem hiding this comment.
If the lock file is empty or contains non-numeric data, parseInt returns NaN. The current logic skips the stale lock check in this case, causing the process to poll for the full LOCK_TIMEOUT_MS before force-acquiring. It is more robust to treat a corrupt or empty lock file as stale and attempt to clean it up immediately.
| const holderPid = Number.parseInt(content.trim(), 10); | |
| if (!Number.isNaN(holderPid) && !isPidAlive(holderPid)) { | |
| // Holder is dead — remove stale lock and retry immediately | |
| const holderPid = Number.parseInt(content.trim(), 10); | |
| if (Number.isNaN(holderPid) || !isPidAlive(holderPid)) { | |
| // Holder is dead or lock file is corrupt — remove stale lock and retry immediately |
![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: ae4b9110cd
ℹ️ 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 (after.startsWith('---')) { | ||
| frontmatter = ''; | ||
| afterClose = after.slice(3); |
There was a problem hiding this comment.
extractFrontmatterFromAgentsMd currently treats any after.startsWith('---') as a valid empty frontmatter close, so content like ---\n---not-a-fence\n# body is misparsed as empty frontmatter and a truncated body instead of frontmatter: null. That mutates byte content for malformed or non-frontmatter files and can silently lose text when callers later rewrite AGENTS.md; require the close marker to be an exact fence line (--- followed by newline or EOF) before taking this branch.
Useful? React with 👍 / 👎.
| // Timeout — force acquire (likely stale lock) | ||
| console.warn(`[lockfile] Force-acquiring stale lock: ${lock}`); | ||
| await writeFile(lock, String(process.pid)); |
There was a problem hiding this comment.
acquireLock force-acquires with writeFile(lock, ...) after 5 seconds even if the existing lock holder is still alive, which allows multiple writers into the critical section simultaneously. In writeAgentYaml that means concurrent processes can race on the same ${path}.tmp and rename/unlink out from under each other, producing lost or corrupted writes on slow filesystems; timeout handling should fail or only break verified-stale locks, not clobber a live one.
Useful? React with 👍 / 👎.
…ation Wave 2 / Group 2 of wish dir-sync-frontmatter-refresh. Depends on Group 1 (#1191, already on dev) which shipped the agent-yaml schema + IO library. - `migrateAgentToYaml(agentDir, dbRow?)` is idempotent: a second call on an already-migrated agent returns `{ migrated: false, reason: 'already-migrated' }` and makes zero disk writes. - Extracts frontmatter via `extractFrontmatterFromAgentsMd`, parses it as YAML, strips derived/DB-only fields, merges with `dbRow` (frontmatter wins on conflict — source-of-truth principle), validates the result via `AgentConfigSchema`, writes `agent.yaml` atomically, saves an `AGENTS.md.bak` byte-for-byte, and rewrites `AGENTS.md` with the post-frontmatter body only. - Malformed frontmatter throws a descriptive error BEFORE any disk write, so no partial state is ever left behind. - The YAML_ALLOWED_KEYS set is derived from AgentConfigSchema's shape at module-load time, so this file never drifts from Group 1's source of truth. Tests: 14 cases pinning every acceptance criterion — idempotency, body preservation (plain / CRLF / Unicode), .bak byte-equality, dbRow fills missing fields, frontmatter wins on conflict, DB-only fields (skill, extra_args, id) never reach agent.yaml, nested permissions survive from dbRow, no-frontmatter path, malformed-frontmatter error leaves files untouched, empty frontmatter mapping parses as valid, return-shape invariants. No caller is rewired here. Groups 3+ consume the migrator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wave 1 / Group 1 of wish `dir-sync-frontmatter-refresh` — the foundation every downstream group imports from. No behavior changes in this PR; only introduces the shared primitive.
Summary
Diff
No caller updates in this PR
By design: no caller is rewired to consume `agent-yaml`. Groups 2-6 of the wish do the migration + `dir sync`/`edit`/`add` refactors on top of this foundation, each as its own PR.
Test plan
Wish authority trail
🤖 Generated with Claude Code