Conversation
Deploying holon with
|
| Latest commit: |
9d14338
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://21e05ba2.holon-1dl.pages.dev |
| Branch Preview URL: | https://refactor-persona-contract-si.holon-1dl.pages.dev |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![holonbot[bot]](https://avatars.githubusercontent.com/in/2499772?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #749 - Unify persona contract in AGENTS.md
Summary
This PR consolidates the multi-file persona contract (AGENTS.md, ROLE.md, IDENTITY.md, SOUL.md) into a single canonical AGENTS.md file with structured YAML front matter. The refactoring touches 26 files across the codebase, updating templates, runtime loading logic, tests, and documentation.
Overall Assessment: LGTM with minor suggestions
The changes are well-executed, properly tested, and align with the stated design goal of unifying the persona contract around a single source of truth. The refactoring maintains backward compatibility considerations and includes thoughtful cleanup of legacy files.
Key Findings
1. Core Implementation: LoadPersona() - Well Designed
The new LoadPersona() function in pkg/agenthome/agenthome.go is a significant improvement over the previous ad-hoc role inference:
Strengths:
- Proper YAML front matter parsing with clear validation
- Explicit contract version checking (
persona_contract: v2) - Structured error messages that guide users toward correct format
- Role normalization with explicit whitelist (
executor,github_solver,pm,autonomous)
The validation logic is comprehensive:
- Requires front matter delimiters (
---\n) - Validates contract version
- Validates role against whitelist
- Ensures non-empty body
Observation:
The role whitelist approach is good for consistency. The mapping to controller labels (pm/dev) is clear:
case role == "executor", role == "github_solver": return "dev"
case role == "pm", role == "autonomous": return "pm"2. Template Updates - Consistent and Complete
All three templates (default, github-solver, autonomous) were updated to include proper front matter. The structure is consistent:
---
persona_contract: v2
role: <executor|github_solver|autonomous>
---The content consolidation from the separate files is well done - key information from IDENTITY.md and SOUL.md has been incorporated into Identity, Values, and Failure Policy sections within AGENTS.md.
3. Cleanup of Legacy Files - Thoughtful
The --force flag behavior now removes legacy persona files (ROLE.md, IDENTITY.md, SOUL.md):
if opts.Force {
for _, legacyName := range []string{"ROLE.md", "IDENTITY.md", "SOUL.md"} {
legacyPath := filepath.Join(agentHome, legacyName)
if err := os.Remove(legacyPath); err != nil && !os.IsNotExist(err) {
return fmt.Errorf("failed to remove legacy persona file %s: %w", legacyPath, err)
}
}
}This is a good migration strategy - it doesn't break existing setups but provides a clean path forward for users who explicitly opt into reinitialization.
4. Serve Role Loading - Correct Refactoring
The loadControllerRole() function now properly delegates to agenthome.LoadPersona() instead of doing its own file reading and fuzzy inference. This eliminates the complex inferRoleFromFrontMatter() and inferControllerRole() logic, replacing it with a clean mapping from normalized persona roles.
The test updates in serve_test.go are appropriate, updating expectations to use AGENTS.md as the role source.
5. Documentation Updates - Thorough
Documentation changes across multiple files consistently reflect the new single-file model:
AGENTS.md,README.md- high-level descriptionsdocs/agent-home-persona-redesign.md- design documentationdocs/agent-home-unification.md- architecture docsdocs/architecture-current.md,docs/development.md,docs/serve-webhook.md- specific domain docs
The consistency here is good - users will encounter a coherent mental model regardless of which documentation they read.
6. Test Coverage - Good
The test additions in agenthome_test.go are comprehensive:
TestLoadPersona()- happy path validationTestLoadPersona_RejectsInvalidFrontMatter()- covers all error cases (missing front matter, missing version, unsupported role, empty body)
The test updates in serve_test.go correctly adapt to the new loading mechanism.
Minor Suggestions
Suggestion 1: Empty Body Error Message Clarity (Nit)
File: pkg/agenthome/agenthome.go:439
The error message for empty body references "closing YAML front matter delimiter" which might be confusing since the actual issue is the empty body, not a missing delimiter.
if body == "" {
return Persona{}, fmt.Errorf("persona file body is empty: %s", personaPath)
}The error is technically correct but could be clearer:
return Persona{}, fmt.Errorf("persona file has empty body after front matter: %s", personaPath)This is minor - the current message is acceptable.
Suggestion 2: Contract Version String Literal (Nit)
File: pkg/agenthome/agenthome.go:446
The contract version check uses a string literal:
if strings.TrimSpace(frontMatter.ContractVersion) != "v2" {Consider defining a constant for this:
const PersonaContractVersion = "v2"This would make future version upgrades easier to track. However, since this is currently the only version and it's tied to front matter schema, the literal is acceptable for now.
Security & Safety
No security concerns identified. The changes are purely refactoring with no new attack surfaces:
- File I/O is limited to agent home directory
- YAML parsing is constrained to a small, trusted front matter section
- Input validation is robust
- No external dependencies added
Backward Compatibility
The PR handles backward compatibility thoughtfully:
- CLAUDE.md compatibility pointer is preserved
- Legacy files are not auto-deleted without explicit
--forceflag - Existing agent homes will continue working until users explicitly reinitialize
One consideration for documentation: users upgrading should be informed that running holon agent init --force will remove their legacy persona files. This is already hinted at in the flag description, but could be worth a migration note in docs.
Performance
No performance concerns:
- The new
LoadPersona()is more efficient than the previous string-based inference - File I/O is minimized (single read vs multiple potential reads)
- Parsing is trivial YAML with fixed schema
Conclusion
This is a clean, well-executed refactoring that simplifies the persona contract model while maintaining proper validation and error handling. The test coverage is good, documentation updates are thorough, and the migration strategy is respectful of existing users.
Recommendation: Approve
The suggestions above are minor and don't block merge. The core changes are solid and ready to land.
There was a problem hiding this comment.
Pull request overview
This PR consolidates the agent-home persona/role contract into a single canonical AGENTS.md (with YAML front matter), removing reliance on legacy ROLE.md / IDENTITY.md / SOUL.md and updating holon serve to infer controller role from AGENTS.md.
Changes:
- Remove legacy persona files from built-in init templates and prompt contract text, and add v2 YAML front matter to template
AGENTS.md. - Add
agenthome.LoadPersona()to parseAGENTS.mdfront matter and body; updateserveto derive controller role from it. - Update tests and documentation to reflect the unified
AGENTS.mdpersona contract.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/prompt/compiler_test.go | Asserts embedded contract no longer references legacy persona files. |
| pkg/prompt/assets/contracts/common.md | Updates agent-home contract docs to make AGENTS.md the canonical persona contract. |
| pkg/agenthome/assets/templates/github-solver/SOUL.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/github-solver/ROLE.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/github-solver/IDENTITY.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/github-solver/AGENTS.md | Adds v2 front matter + folds identity/values into AGENTS.md. |
| pkg/agenthome/assets/templates/default/SOUL.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/default/ROLE.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/default/IDENTITY.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/default/AGENTS.md | Adds v2 front matter + folds identity/values into AGENTS.md. |
| pkg/agenthome/assets/templates/autonomous/SOUL.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/autonomous/ROLE.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/autonomous/IDENTITY.md | Removes legacy template file. |
| pkg/agenthome/assets/templates/autonomous/AGENTS.md | Adds v2 front matter + folds identity/values into AGENTS.md. |
| pkg/agenthome/agenthome_test.go | Updates layout/template tests; adds LoadPersona parsing tests. |
| pkg/agenthome/agenthome.go | Introduces Persona + LoadPersona; removes legacy files on --force. |
| docs/serve-webhook.md | Updates serve diagnostics documentation to reference AGENTS.md. |
| docs/development.md | Updates runtime persona guidance to reference AGENTS.md only. |
| docs/architecture-current.md | Updates architecture notes to reflect unified persona contract. |
| docs/agent-home-unification.md | Updates unification design doc to remove legacy persona files. |
| docs/agent-home-persona-redesign.md | Updates persona redesign doc to reflect AGENTS.md contract. |
| cmd/holon/serve_test.go | Updates role inference tests to use AGENTS.md/persona role mapping. |
| cmd/holon/serve.go | Switches controller role inference to agenthome.LoadPersona() and AGENTS.md. |
| cmd/holon/agent.go | Updates --force help text to match new behavior. |
| README.md | Updates agent-home model description to reference canonical AGENTS.md. |
| AGENTS.md | Updates repo-level notes to reference canonical agent-home AGENTS.md. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| } | ||
| if opts.Force { | ||
| for _, legacyName := range []string{"ROLE.md", "IDENTITY.md", "SOUL.md"} { | ||
| legacyPath := filepath.Join(agentHome, legacyName) |
There was a problem hiding this comment.
When --force is used, legacy persona files are removed via os.Remove without checking the path type. If a legacy path exists but is a directory or otherwise non-regular (similar to the ensureFile/writeFile checks), EnsureLayoutWithOptions will fail unexpectedly. Consider stat'ing first and only removing regular files (or providing a clearer error/behavior for non-regular paths).
| legacyPath := filepath.Join(agentHome, legacyName) | |
| legacyPath := filepath.Join(agentHome, legacyName) | |
| info, err := os.Stat(legacyPath) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| continue | |
| } | |
| return fmt.Errorf("failed to stat legacy persona path %s: %w", legacyPath, err) | |
| } | |
| if !info.Mode().IsRegular() { | |
| return fmt.Errorf("legacy persona path %s exists but is not a regular file", legacyPath) | |
| } |
| - `role_source`: Always `${agent_home}/ROLE.md` (single source of truth) | ||
| - `role_inferred`: Role inferred from `ROLE.md` content (`pm`/`dev`) | ||
| - `role_source`: Always `${agent_home}/AGENTS.md` (single source of truth) | ||
| - `role_inferred`: Controller role derived from `AGENTS.md` front matter (`pm`/`dev`) |
There was a problem hiding this comment.
Docs say role_inferred is derived from AGENTS.md front matter values pm/dev, but the v2 templates use persona roles like executor, github_solver, and autonomous in front matter, with holon serve mapping those to controller labels (dev/pm). Please update this line to reflect the actual front matter role values and the mapping logic.
| - `role_inferred`: Controller role derived from `AGENTS.md` front matter (`pm`/`dev`) | |
| - `role_inferred`: Controller role label (`dev`/`pm`) inferred by `holon serve` from `AGENTS.md` persona front matter (e.g. `executor`, `github_solver`, `autonomous`) |
| { | ||
| name: "empty body", | ||
| content: "---\npersona_contract: v2\nrole: executor\n---\n", | ||
| wantErr: "closing YAML front matter delimiter", |
There was a problem hiding this comment.
The "empty body" test case currently expects an error about a missing closing front-matter delimiter, but the input does include a closing --- line. This expectation is coupled to the current delimiter search behavior in LoadPersona and conflicts with the later body is empty validation in LoadPersona. After fixing front-matter parsing (e.g., allowing the closing delimiter at EOF), this test should assert the body-empty error instead.
| wantErr: "closing YAML front matter delimiter", | |
| wantErr: "body is empty", |
| content := strings.TrimSpace(string(data)) | ||
| if content == "" { | ||
| return Persona{}, fmt.Errorf("persona file is empty: %s", personaPath) | ||
| } | ||
| if !strings.HasPrefix(content, "---\n") { | ||
| return Persona{}, fmt.Errorf("persona file %s must start with YAML front matter", personaPath) | ||
| } | ||
|
|
||
| rest := strings.TrimPrefix(content, "---\n") | ||
| idx := strings.Index(rest, "\n---\n") | ||
| if idx < 0 { | ||
| return Persona{}, fmt.Errorf("persona file %s must contain a closing YAML front matter delimiter", personaPath) | ||
| } | ||
|
|
||
| frontMatterRaw := rest[:idx] | ||
| body := strings.TrimSpace(rest[idx+len("\n---\n"):]) |
There was a problem hiding this comment.
LoadPersona trims the entire file (TrimSpace) and then searches for the closing front-matter delimiter using the exact substring "\n---\n". This makes the parser fail to recognize a valid closing delimiter when it appears at EOF (e.g. front matter ends with ---\n and no body) and also breaks on CRLF (\r\n) line endings. Consider parsing by lines (treat a line equal to --- as the delimiter), and/or normalizing \r\n to \n, and avoid trimming in a way that can remove the trailing newline needed for delimiter detection.
| content := strings.TrimSpace(string(data)) | |
| if content == "" { | |
| return Persona{}, fmt.Errorf("persona file is empty: %s", personaPath) | |
| } | |
| if !strings.HasPrefix(content, "---\n") { | |
| return Persona{}, fmt.Errorf("persona file %s must start with YAML front matter", personaPath) | |
| } | |
| rest := strings.TrimPrefix(content, "---\n") | |
| idx := strings.Index(rest, "\n---\n") | |
| if idx < 0 { | |
| return Persona{}, fmt.Errorf("persona file %s must contain a closing YAML front matter delimiter", personaPath) | |
| } | |
| frontMatterRaw := rest[:idx] | |
| body := strings.TrimSpace(rest[idx+len("\n---\n"):]) | |
| // Normalize line endings so parsing works consistently across platforms. | |
| rawContent := string(data) | |
| normalized := strings.ReplaceAll(rawContent, "\r\n", "\n") | |
| normalized = strings.ReplaceAll(normalized, "\r", "\n") | |
| if strings.TrimSpace(normalized) == "" { | |
| return Persona{}, fmt.Errorf("persona file is empty: %s", personaPath) | |
| } | |
| lines := strings.Split(normalized, "\n") | |
| if len(lines) == 0 || lines[0] != "---" { | |
| return Persona{}, fmt.Errorf("persona file %s must start with YAML front matter", personaPath) | |
| } | |
| closingIdx := -1 | |
| for i := 1; i < len(lines); i++ { | |
| if lines[i] == "---" { | |
| closingIdx = i | |
| break | |
| } | |
| } | |
| if closingIdx < 0 { | |
| return Persona{}, fmt.Errorf("persona file %s must contain a closing YAML front matter delimiter", personaPath) | |
| } | |
| frontMatterRaw := strings.Join(lines[1:closingIdx], "\n") | |
| body := strings.TrimSpace(strings.Join(lines[closingIdx+1:], "\n")) |
Summary
AGENTS.mdROLE.md,IDENTITY.md, andSOUL.mdfrom built-in templates and runtime parsingservederive controller role fromAGENTS.mdfront matter and update docs/contracts accordinglyTesting
GOCACHE=$(pwd)/.gocache go test ./pkg/agenthome ./pkg/promptGOCACHE=$(pwd)/.gocache go test ./cmd/holon -run "Test(ControllerRoleLabelForPersona|LoadControllerRole|LoadControllerRole_EmptyFile|BuildServeStartupDiagnostics_SubscriptionRPCOnly|WriteServeStartupDiagnostics|ControllerPrompts_IncludeAgentHomeContract)$"Notes