feat: Ship Project Manager archetype (soul preset + catalog row) - #2178
Conversation
WalkthroughAdds a Lead Engineer preset and catalog entry, plus regression validation for unique archetype IDs, preset file existence, and ChangesLead Engineer archetype
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 `@tests/test_bundled_config_assets.py`:
- Around line 130-134: Update the preset-path validation in the bundled
configuration asset test to resolve the path and assert it remains inside CONFIG
/ "soul-presets" before checking is_file(). Preserve the existing missing-file
assertion and ensure traversal values such as "../base" are rejected even when
the resolved target exists.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99cd8de6-351c-42a4-a3c7-df5ac6c24fec
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!*.md
📒 Files selected for processing (3)
config/archetype-catalog.jsonconfig/soul-presets/lead-engineer.mdtests/test_bundled_config_assets.py
| row = next(a for a in catalog["archetypes"] if a["id"] == "lead-engineer") | ||
| preset = CONFIG / "soul-presets" / f"{row['soul_preset']}.md" | ||
| assert preset.is_file(), ( | ||
| f"soul_preset '{row['soul_preset']}' does not resolve to {preset} — " | ||
| f"the persona step would silently seed an empty SOUL." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate the resolved preset path.
preset.is_file() can pass for a value such as ../base when config/base.md exists, but production read_soul_preset() rejects that path and returns an empty SOUL. Resolve the path and assert it remains within config/soul-presets/.
Proposed fix
- preset = CONFIG / "soul-presets" / f"{row['soul_preset']}.md"
- assert preset.is_file(), (
+ presets_root = (CONFIG / "soul-presets").resolve()
+ preset = (presets_root / f"{row['soul_preset']}.md").resolve()
+ assert preset.parent == presets_root and preset.is_file(), (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| row = next(a for a in catalog["archetypes"] if a["id"] == "lead-engineer") | |
| preset = CONFIG / "soul-presets" / f"{row['soul_preset']}.md" | |
| assert preset.is_file(), ( | |
| f"soul_preset '{row['soul_preset']}' does not resolve to {preset} — " | |
| f"the persona step would silently seed an empty SOUL." | |
| row = next(a for a in catalog["archetypes"] if a["id"] == "lead-engineer") | |
| presets_root = (CONFIG / "soul-presets").resolve() | |
| preset = (presets_root / f"{row['soul_preset']}.md").resolve() | |
| assert preset.parent == presets_root and preset.is_file(), ( | |
| f"soul_preset '{row['soul_preset']}' does not resolve to {preset} — " | |
| f"the persona step would silently seed an empty SOUL." | |
| ) |
🤖 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 `@tests/test_bundled_config_assets.py` around lines 130 - 134, Update the
preset-path validation in the bundled configuration asset test to resolve the
path and assert it remains inside CONFIG / "soul-presets" before checking
is_file(). Preserve the existing missing-file assertion and ensure traversal
values such as "../base" are rejected even when the resolved target exists.
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 8939c8316817 · formal
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
Overall risk: Low. This PR adds a single soul preset (lead-engineer.md) and a one-line CHANGELOG entry — no production code, no logic changes, no API surface touched.
Fix-first item: Add the missing # Communication style section to config/soul-presets/lead-engineer.md. Every other non-blank preset provides formatting and length guidance; its absence means the lead-engineer agent has no output-format constraint, which is a real behavior gap at runtime.
Panel disagreed on: Nothing. Both findings were independently surfaced by the correctness and cross-file review lanes and confirmed by the verifier.
Verification changed: Neither finding was refuted — both survive intact with confirmed verdicts. No findings were dropped.
Gaps: The structural pass (protopatch) completed but produced no findings — expected for a config-only change with no code paths to analyze. No coverage gaps flagged.
[
{
"file": "config/soul-presets/lead-engineer.md",
"line": 1,
"severity": "minor",
"category": "conventions",
"claim": "The lead-engineer soul preset is missing the `# Communication style` section that is present in every other non-blank soul preset and explicitly listed as a required section in the blank.md template.",
"evidence": "All seven existing presets under config/soul-presets/ include a `# Communication style` section; the blank.md template lists it as a required section (\"_How output is formatted — markdown, plain text, JSON, Discord embeds. How long responses should be by default._\"). The lead-engineer preset has Identity, How I work, Personality, and Values, but no Communication style.",
"verdict": "confirmed",
"note": "Verified: all 7 existing presets (base, blank, coding, cowork, generic-assistant, project-manager, research) have `# Communication style`; blank.md template lists it; lead-engineer.md diff has Identity/How I work/Personality/Values but no Communication style section."
},
{
"file": "CHANGELOG.md",
"line": 15,
"severity": "minor",
"category": "conventions",
"claim": "The changelog entry references `(#2177)` which does not resolve to a pull request on this repo while all other entries cite valid PR numbers — the actual PR is #2178 (flagged independently by both correctness and cross-file review).",
"evidence": "The diff adds: `- **Lead Engineer archetype in the new-agent picker.** ... (#2177)`. A fetch of PR #2177 returns \"Could not resolve to a PullRequest with the number of 2177.\" All other entries in the changelog reference valid PR numbers (e.g. `(#2138, #2140, #2141, #2142)`). The test docstring also references #2177, suggesting it may be a tracking issue rather than a PR — but the changelog convention is uniformly to cite PR numbers.",
"verdict": "confirmed",
"note": "Verified: fetch of /pull/2177 shows issue #2177 (tracking issue, not a PR); PR #2178's description says 'Fixes #2177'. All other (#N) citations in CHANGELOG are PR numbers; issues use 'closes #N'/'fixes #N' syntax. #2177 is an issue in the PR-number slot."
}
]
|
Operator adjudication of the WARN (brain-ruled): both findings fix-now. (1) Add the |
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head df6b8a6ff31e · formal
[review-synthesizer completed: workflow code-review-structural:report]
Overall risk is low; the PR is clean after addressing prior feedback. No fix-first item remains. The panel had no disagreements. Verification confirmed both Round 1 minor findings (missing communication style section and incorrect changelog PR reference) are resolved in the diff, leaving no residual claims. A structural analysis pass was skipped for this round.
[]
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head e981800ed208 · formal
[review-synthesizer completed: workflow code-review-structural:report]
Overall risk is low; the PR introduces purely additive configuration, documentation, and test files without modifying existing logic. There are no defects to fix first. The panel unanimously agreed on a clean pass, and verification confirmed no hidden issues in the new archetype row or soul preset. A structural analysis pass was skipped but deemed unnecessary given the additive nature of the changes.
[]
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 88915749535b · formal
[review-synthesizer completed: workflow code-review-structural:report]
Overall risk is low; the diff introduces no structural, correctness, or cross-file defects. The prior round's minor observations (missing communication style section and changelog PR number mismatch) have been resolved in the working tree. The panel unanimously agreed on a clean slate, and verification confirmed no remaining blockers or majors. No significant coverage gaps were identified, though the structural pass was intentionally skipped as noted. Ready to merge.
[][]
mabry1985
changed the title
1 participant
Summary
Ship Lead Engineer archetype (soul preset + catalog row) (
bd-2dt)See the diff for details.
Summary by CodeRabbit
New Features
Tests
Fixes #2177