fix(codex): wrap AGENTS.md persona in managed marker sections#1064
Open
umi008 wants to merge 1 commit into
Open
fix(codex): wrap AGENTS.md persona in managed marker sections#1064umi008 wants to merge 1 commit into
umi008 wants to merge 1 commit into
Conversation
Codex uses StrategyFileReplace for its system prompt, but the persona injector wrote raw prose without <!-- gentle-ai:persona --> markers. This made Codex persona content impossible to update, uninstall, or distinguish from user-authored content. Extend the marker-based injection path (already used by OpenCode) to also cover Codex. Now Codex AGENTS.md gets the same managed persona markers as Claude Code's CLAUDE.md, enabling deterministic cleanup and idempotent sync. Closes Gentleman-Programming#981
📝 WalkthroughWalkthroughThe ChangesCodex persona marker fix
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/components/persona/inject_test.go`:
- Around line 109-156: Add a regression test in Inject to verify user-authored
AGENTS.md content survives persona injection: pre-create .codex/AGENTS.md with
custom text, call Inject with codexAdapter() and model.PersonaGentleman, then
assert the original content is still present alongside the managed markers.
Place it near TestInjectCodexGentlemanWritesPersonaWithManagedMarkers and
TestInjectCodexGentlemanIsIdempotent so the existing marker/idempotency coverage
stays intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f71173c-be43-406d-9265-68a510905cda
📒 Files selected for processing (2)
internal/components/persona/inject.gointernal/components/persona/inject_test.go
Comment on lines
+109
to
+156
| func TestInjectCodexGentlemanWritesPersonaWithManagedMarkers(t *testing.T) { | ||
| home := t.TempDir() | ||
|
|
||
| result, err := Inject(home, codexAdapter(), model.PersonaGentleman) | ||
| if err != nil { | ||
| t.Fatalf("Inject(codex) error = %v", err) | ||
| } | ||
| if !result.Changed { | ||
| t.Fatal("Inject(codex) changed = false") | ||
| } | ||
|
|
||
| path := filepath.Join(home, ".codex", "AGENTS.md") | ||
| content, err := os.ReadFile(path) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile() error = %v", err) | ||
| } | ||
|
|
||
| text := string(content) | ||
| if !strings.Contains(text, "<!-- gentle-ai:persona -->") { | ||
| t.Fatal("AGENTS.md missing open marker for persona") | ||
| } | ||
| if !strings.Contains(text, "<!-- /gentle-ai:persona -->") { | ||
| t.Fatal("AGENTS.md missing close marker for persona") | ||
| } | ||
| if !strings.Contains(text, "Senior Architect") { | ||
| t.Fatal("AGENTS.md persona section missing 'Senior Architect' content") | ||
| } | ||
| } | ||
|
|
||
| func TestInjectCodexGentlemanIsIdempotent(t *testing.T) { | ||
| home := t.TempDir() | ||
|
|
||
| first, err := Inject(home, codexAdapter(), model.PersonaGentleman) | ||
| if err != nil { | ||
| t.Fatalf("Inject(codex) first error = %v", err) | ||
| } | ||
| if !first.Changed { | ||
| t.Fatal("Inject(codex) first changed = false") | ||
| } | ||
|
|
||
| second, err := Inject(home, codexAdapter(), model.PersonaGentleman) | ||
| if err != nil { | ||
| t.Fatalf("Inject(codex) second error = %v", err) | ||
| } | ||
| if second.Changed { | ||
| t.Fatal("Inject(codex) second changed = true — not idempotent") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Tests are solid; consider adding a user-content preservation test.
The two new tests correctly verify marker presence and idempotency for Codex. As per path instructions, idempotency is confirmed by TestInjectCodexGentlemanIsIdempotent.
One gap: the PR objective states persona content should be injectable "without affecting user-authored content in AGENTS.md," but no test pre-populates AGENTS.md with user content and verifies it survives injection. Adding such a test would strengthen confidence in the marker-based isolation.
💡 Suggested additional test
func TestInjectCodexPreservesUserContent(t *testing.T) {
home := t.TempDir()
path := filepath.Join(home, ".codex", "AGENTS.md")
userContent := "## My Custom Rules\n\nAlways use explicit error handling.\n"
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
t.Fatalf("MkdirAll() error = %v", err)
}
if err := os.WriteFile(path, []byte(userContent), 0o644); err != nil {
t.Fatalf("WriteFile() error = %v", err)
}
result, err := Inject(home, codexAdapter(), model.PersonaGentleman)
if err != nil {
t.Fatalf("Inject() error = %v", err)
}
if !result.Changed {
t.Fatal("Inject() changed = false")
}
content, err := os.ReadFile(path)
if err != nil {
t.Fatalf("ReadFile() error = %v", err)
}
if !strings.Contains(string(content), "Always use explicit error handling.") {
t.Fatal("user-authored content was lost during injection")
}
if !strings.Contains(string(content), "<!-- gentle-ai:persona -->") {
t.Fatal("managed persona markers missing")
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/components/persona/inject_test.go` around lines 109 - 156, Add a
regression test in Inject to verify user-authored AGENTS.md content survives
persona injection: pre-create .codex/AGENTS.md with custom text, call Inject
with codexAdapter() and model.PersonaGentleman, then assert the original content
is still present alongside the managed markers. Place it near
TestInjectCodexGentlemanWritesPersonaWithManagedMarkers and
TestInjectCodexGentlemanIsIdempotent so the existing marker/idempotency coverage
stays intact.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wraps Codex persona content in
<!-- gentle-ai:persona -->managed marker sections, matching the pattern already used by Claude Code and OpenCode.Problem
Codex uses
StrategyFileReplacefor its system prompt (~/.codex/AGENTS.md), but the persona injector wrote raw prose without markers. This made Codex persona content impossible to update, uninstall, or distinguish from user-authored content.Solution
Extend the marker-based injection path (already used by OpenCode) to also cover Codex by changing the agent check from
AgentOpenCodetoAgentOpenCode || AgentCodex.Files changed: 2 files, 52 insertions, 1 deletion
internal/components/persona/inject.go— extend marker-based path to include Codexinternal/components/persona/inject_test.go— add 2 Codex-specific tests (marker presence + idempotency)Testing
go test ./internal/components/persona/...— 143 tests pass (141 existing + 2 new)Closes #981
Summary by CodeRabbit