Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions src/genie-commands/doctor.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/**
* Tests for `checkLegacyAgentFrontmatter` — Group 6 of wish
* `dir-sync-frontmatter-refresh`.
*
* The check walks every agent directory under `agents/` and warns when an
* AGENTS.md starts with a `---` fence while an `agent.yaml` is also
* present — the configuration-in-wrong-file scenario that sync silently
* ignores post-migration.
*/

import { afterEach, beforeEach, describe, expect, test } from 'bun:test';
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { checkLegacyAgentFrontmatter } from './doctor.js';

let workspaceRoot: string;

beforeEach(() => {
workspaceRoot = mkdtempSync(join(tmpdir(), 'doctor-legacy-fm-'));
});

afterEach(() => {
try {
rmSync(workspaceRoot, { recursive: true, force: true });
} catch {
/* ignore */
}
});

function seedAgent(name: string, files: { agentYaml?: string; agentsMd?: string }): void {
const agentDir = join(workspaceRoot, 'agents', name);
mkdirSync(agentDir, { recursive: true });
if (files.agentYaml !== undefined) writeFileSync(join(agentDir, 'agent.yaml'), files.agentYaml);
if (files.agentsMd !== undefined) writeFileSync(join(agentDir, 'AGENTS.md'), files.agentsMd);
}

describe('checkLegacyAgentFrontmatter', () => {
test('warns when AGENTS.md has frontmatter AND agent.yaml exists', async () => {
seedAgent('drifted', {
agentYaml: 'promptMode: append\nmodel: opus\n',
agentsMd: '---\nmodel: sonnet\n---\n\n# body',
});

const results = checkLegacyAgentFrontmatter(workspaceRoot);
expect(results).toHaveLength(1);
expect(results[0].status).toBe('warn');
expect(results[0].name).toBe('agents/drifted/AGENTS.md');
expect(results[0].message).toMatch(/legacy frontmatter/i);
expect(results[0].suggestion).toMatch(/agent\.yaml/);
});

test('silent (pass) when agent.yaml absent — pre-migration state', async () => {
seedAgent('pre-migration', {
agentsMd: '---\nmodel: sonnet\n---\n\n# body',
});

const results = checkLegacyAgentFrontmatter(workspaceRoot);
expect(results).toHaveLength(1);
expect(results[0].status).toBe('pass');
expect(results[0].name).toMatch(/No legacy frontmatter/);
});

test('silent (pass) when AGENTS.md has no fence — clean post-migration', async () => {
seedAgent('clean', {
agentYaml: 'promptMode: append\n',
agentsMd: '# Agent: clean\n\nBody content.',
});

const results = checkLegacyAgentFrontmatter(workspaceRoot);
expect(results).toHaveLength(1);
expect(results[0].status).toBe('pass');
});

test('silent when agents/ directory does not exist', async () => {
// No seeding — workspace has no agents/ dir.
const results = checkLegacyAgentFrontmatter(workspaceRoot);
expect(results).toHaveLength(0);
});

test('flags multiple agents independently', async () => {
seedAgent('drifted-one', {
agentYaml: 'promptMode: append\n',
agentsMd: '---\nmodel: opus\n---\n# body',
});
seedAgent('clean', {
agentYaml: 'promptMode: append\n',
agentsMd: '# Agent: clean\n\nBody.',
});
seedAgent('drifted-two', {
agentYaml: 'promptMode: append\n',
agentsMd: '---\ncolor: red\n---\n# body',
});

const results = checkLegacyAgentFrontmatter(workspaceRoot);
// 2 warnings + 0 pass fillers (pass filler only emitted when zero warnings)
expect(results).toHaveLength(2);
const names = results.map((r) => r.name).sort();
expect(names).toEqual(['agents/drifted-one/AGENTS.md', 'agents/drifted-two/AGENTS.md']);
});

test('ignores non-directory entries in agents/', async () => {
// Seed one drifted agent so we get a warning to assert.
seedAgent('real', {
agentYaml: 'promptMode: append\n',
agentsMd: '---\nmodel: opus\n---\n',
});
// Drop a file inside agents/ — should be skipped, not crash.
writeFileSync(join(workspaceRoot, 'agents', 'README'), 'not an agent');

const results = checkLegacyAgentFrontmatter(workspaceRoot);
expect(results).toHaveLength(1);
expect(results[0].name).toBe('agents/real/AGENTS.md');
});

test('skips agent dirs that only have agent.yaml (no AGENTS.md)', async () => {
seedAgent('yaml-only', { agentYaml: 'promptMode: append\n' });

const results = checkLegacyAgentFrontmatter(workspaceRoot);
expect(results).toHaveLength(1);
expect(results[0].status).toBe('pass');
});
});
71 changes: 70 additions & 1 deletion src/genie-commands/doctor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* Checks prerequisites, configuration, and tmux connectivity.
*/

import { existsSync, unlinkSync } from 'node:fs';
import { existsSync, readFileSync, readdirSync, statSync, unlinkSync } from 'node:fs';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since statSync is only used within checkLegacyAgentFrontmatter to check if an entry is a directory, and we can optimize that using readdirSync({ withFileTypes: true }), this import can be removed.

Suggested change
import { existsSync, readFileSync, readdirSync, statSync, unlinkSync } from 'node:fs';
import { existsSync, readFileSync, readdirSync, unlinkSync } from 'node:fs';

import { homedir } from 'node:os';
import { join } from 'node:path';
import { $ } from 'bun';
Expand All @@ -14,6 +14,7 @@ import { contractClaudePath, getClaudeSettingsPath } from '../lib/claude-setting
import { tmuxBin } from '../lib/ensure-tmux.js';
import { genieConfigExists, getGenieConfigPath, isSetupComplete, loadGenieConfig } from '../lib/genie-config.js';
import { checkCommand } from '../lib/system-detect.js';
import { findWorkspace } from '../lib/workspace.js';

interface CheckResult {
name: string;
Expand Down Expand Up @@ -395,6 +396,73 @@ async function checkBridge(): Promise<CheckResult[]> {
return results;
}

/**
* Check that no agent has leftover `---` frontmatter in its `AGENTS.md`
* when `agent.yaml` is also present. Wish `dir-sync-frontmatter-refresh`
* (Group 6) eliminates frontmatter entirely post-migration — if a user
* pastes config back into AGENTS.md, sync silently ignores it, which is
* confusing. This check surfaces the drift loudly.
*
* Exported for unit tests and for callers that want to run this check
* outside `genie doctor` (e.g. a pre-sync lint).
*/
export function checkLegacyAgentFrontmatter(workspaceRoot?: string): CheckResult[] {
const results: CheckResult[] = [];
const root = workspaceRoot ?? findWorkspace()?.root;
if (!root) return [];

const agentsDir = join(root, 'agents');
if (!existsSync(agentsDir)) return [];

let entries: string[];
try {
entries = readdirSync(agentsDir);
} catch {
return [];
}

for (const name of entries) {
const agentDir = join(agentsDir, name);
try {
if (!statSync(agentDir).isDirectory()) continue;
} catch {
continue;
}
Comment on lines +417 to +430

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

You can optimize this loop by using readdirSync(agentsDir, { withFileTypes: true }). This avoids a separate statSync call for every entry in the agents/ directory, which can be a significant performance improvement if there are many agents.

  let entries;
  try {
    entries = readdirSync(agentsDir, { withFileTypes: true });
  } catch {
    return [];
  }

  for (const entry of entries) {
    if (!entry.isDirectory()) continue;
    const name = entry.name;
    const agentDir = join(agentsDir, name);


const yamlPath = join(agentDir, 'agent.yaml');
const agentsMdPath = join(agentDir, 'AGENTS.md');

// Only flag when both files exist AND AGENTS.md starts with a fence.
// Pre-migration state (no yaml, frontmatter present) is NOT a warning —
// sync will migrate it on the next run.
if (!existsSync(yamlPath) || !existsSync(agentsMdPath)) continue;

let headContent: string;
try {
// Read only the first handful of bytes to cheaply detect the fence.
headContent = readFileSync(agentsMdPath, 'utf-8').slice(0, 4);
} catch {
continue;
}

if (!headContent.startsWith('---')) continue;
Comment on lines +440 to +448

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While using a hardcoded limit (magic number) to cap I/O is acceptable in non-critical logic, readFileSync(path, 'utf-8') reads the entire file into memory before slicing, which is inefficient for large files. Consider using a stream or a buffer-limited read to truly optimize performance. Additionally, ensure the fence check includes a newline to avoid false positives.

Suggested change
let headContent: string;
try {
// Read only the first handful of bytes to cheaply detect the fence.
headContent = readFileSync(agentsMdPath, 'utf-8').slice(0, 4);
} catch {
continue;
}
if (!headContent.startsWith('---')) continue;
try {
// Read only the first handful of bytes to cheaply detect the fence.
const headContent = readFileSync(agentsMdPath, 'utf-8').slice(0, 4);
if (!headContent.startsWith('---\n') && !headContent.startsWith('---\r')) continue;
} catch {
continue;
}
References
  1. It is acceptable to use hardcoded numeric limits (magic numbers) in non-critical fallback logic, especially when they serve as intentional caps to prevent performance issues like excessive I/O.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Require a valid frontmatter block before warning

This check flags any AGENTS.md whose first bytes start with ---, but the codebase’s frontmatter parser treats frontmatter as valid only when there is a proper opening line (---\n) and a closing fence. With agent.yaml present, files that start with a Markdown horizontal rule or an unclosed --- block will be reported as “legacy frontmatter” even though sync does not consider them frontmatter, producing persistent false warnings in genie doctor.

Useful? React with 👍 / 👎.


results.push({
name: `agents/${name}/AGENTS.md`,
status: 'warn',
message: 'legacy frontmatter detected (ignored by sync)',
suggestion: `Move config into agents/${name}/agent.yaml — AGENTS.md is prompt-only post-migration.`,
});
}

// Single positive result when nothing drifted, so the section prints a
// clean "✓" row.
if (results.length === 0) {
results.push({ name: 'No legacy frontmatter in agents/*/AGENTS.md', status: 'pass' });
}
return results;
}

/**
* Main doctor command
*/
Expand Down Expand Up @@ -424,6 +492,7 @@ export async function doctorCommand(options?: { fix?: boolean }): Promise<void>
runCheckSection('Tmux', await checkTmux(), counts);
runCheckSection('Worker Profiles', await checkWorkerProfiles(), counts);
runCheckSection('Omni Bridge', await checkBridge(), counts);
runCheckSection('Agent Config', checkLegacyAgentFrontmatter(), counts);

// Summary
console.log();
Expand Down
Loading