feat(archetypes): add scope metadata to .md files and implement GetScope() (SO-89)#5
Open
feat(archetypes): add scope metadata to .md files and implement GetScope() (SO-89)#5
Conversation
- Add copilot option to runner <select> so existing copilot-runner agents preserve their value on save - Add data-original attribute to capture the runner value at page load - Add #copilot-runner-warning hidden banner shown by checkCopilotRunnerWarning() when user changes away from copilot before saving - Add checkCopilotRunnerWarning() JS function called from onchange handler - Add copilot model list to updateModels() in partials.html AC: 1. Warning shown when runner changed from copilot to another value ✓ 2. Warning text: 'Warning: changing the runner from copilot may break this agent.' ✓ 3. Client-side JS fires before save (onchange on <select>) ✓ 4. No warning shown when copilot runner is unchanged ✓ 5. go build ./... and go test ./... pass ✓
… scope list (SO-89) - Add ## Scope section to 9 archetype .md files: ceo, devops, architect, auditor, castrojo-docs, castrojo-engineer-bluefin, castrojo-engineer-cncf, castrojo-qa, principle-engineer - Implement archetypes.GetScope(slug string) ([]string, error) - Parses ## Scope section, returns comma-separated tags - Returns empty slice (not error) for missing archetype or missing section (AC4) - Add archetypes_test.go with 6 tests covering AC2/AC3/AC4: TestGetScope_Devops, TestGetScope_Architect, TestGetScope_CastrojoDocs, TestGetScope_NonexistentSlug, TestGetScope_NoScopeSection, TestGetScope_CEO Closes SO-89
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Template as agent_detail.html
participant JS as JavaScript Handler
participant ModelUpdater as updateModels()
User->>Template: Change runner dropdown
Template->>JS: checkCopilotRunnerWarning()
JS->>JS: Compare original vs. new value
alt Original was 'copilot'
JS->>Template: Show warning alert
else
JS->>Template: Hide warning alert
end
Template->>ModelUpdater: Call updateModels()
ModelUpdater->>ModelUpdater: Check if runner === 'copilot'
alt Runner is 'copilot'
ModelUpdater->>Template: Populate Copilot-specific models
else
ModelUpdater->>Template: Use default models
end
sequenceDiagram
participant Caller
participant GetScope as GetScope(slug)
participant FileSystem as File System
participant Parser as Parser
Caller->>GetScope: Request scope for archetype
GetScope->>FileSystem: Read archetype .md file
alt File not found
FileSystem-->>GetScope: Error or empty
GetScope-->>Caller: Return empty slice, no error
else File exists
FileSystem-->>GetScope: File content
GetScope->>Parser: Extract ## Scope section
Parser->>Parser: Find ## Scope heading
Parser->>Parser: Parse until next ## heading
Parser->>Parser: Split first line by comma
alt Scope section found
Parser-->>GetScope: Extracted tags
GetScope-->>Caller: Return tag slice, no error
else Scope section missing
Parser-->>GetScope: No tags
GetScope-->>Caller: Return empty slice, no error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 GetScope function to parse scope metadata from archetype markdown files and updates various archetypes with these tags. It also adds a new 'Principal Engineer' archetype and enhances the agent detail UI with a 'copilot' runner option and associated warnings. Review feedback identifies a logic error in the GetScope parser that prevents it from reading tags across multiple lines as documented, and notes a spelling inconsistency between the filename and content of the new 'Principal Engineer' archetype.
Comment on lines
+144
to
+145
| // Scope section is a single line of tags; stop after first non-blank line | ||
| break |
There was a problem hiding this comment.
The break statement here limits the scope section to only the first non-blank line of tags. This contradicts the function's documentation on line 102, which states that tags can be on "line(s)" (plural). Removing this break allows the parser to correctly handle tags spread across multiple lines until the next header is encountered, making the implementation consistent with the docstring.
| @@ -0,0 +1,43 @@ | |||
| # Principal Engineer | |||
There was a problem hiding this comment.
The filename principle-engineer.md (and the corresponding slug) contains a typo. The content of the file correctly uses "Principal Engineer" (lines 1, 3, 13, 40), which is the standard professional title. It is recommended to rename the file to principal-engineer.md to ensure consistency between the filename/slug and the archetype's title.
…SO-102) Proves that GetScope() stops parsing at the next ## heading and does not bleed content from subsequent sections into the returned scope list. Uses principle-engineer.md which has '## Scope clarification' before '## Scope', verifying the boundary-stop behavior explicitly. Closes QA Finding 3 from SO-101.
…(SO-103)
Replace the principle-engineer.md-based TestGetScope_StopsAtNextHeading with a
cleaner version that uses a dedicated fixture file.
Fixture: internal/archetypes/testdata/scope-boundary.md
## Scope
foo, bar
## Other Section
baz, qux
The test proves GetScope() returns ["foo", "bar"] and NOT ["foo", "bar", "baz", "qux"],
with explicit positive and negative assertions confirming the parser stops at '##'.
Uses SetOverridesDir("testdata") + t.Cleanup to isolate the test without
modifying global state permanently.
Closes SO-103.
![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
🧹 Nitpick comments (1)
internal/archetypes/archetypes_test.go (1)
8-97: Consider extracting a shared scope assertion helper.These tests repeat the same length/index comparison pattern. A helper (or table-driven structure) would reduce duplication and simplify future archetype additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/archetypes/archetypes_test.go` around lines 8 - 97, The tests for GetScope duplicate identical length and element-by-element checks across TestGetScope_Devops/Architect/CastrojoDocs/CEO/NonexistentSlug/NoScopeSection; extract a helper (e.g., assertScopeEqual(t *testing.T, got []string, want []string, name string)) that checks lengths and each index and calls t.Fatalf/t.Errorf with contextual messages, then replace the repeated checks in each TestGetScope_* to call that helper (or convert the tests to a table-driven TestGetScope that iterates cases with name, slug, expected and uses the same helper for assertions) so assertion logic is centralized and duplication removed while still calling GetScope in each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/archetypes/archetypes.go`:
- Around line 110-114: The current Read(slug) error handling treats every read
failure as “archetype not found” and returns an empty slice, which hides real IO
errors; update the logic around the Read(slug) call so only a true “not found”
condition returns []string{}, nil and all other errors are returned to the
caller. Concretely, detect the not-found sentinel (e.g., errors.Is(err,
ErrNotFound) or os.IsNotExist(err) depending on your Read implementation) and
keep the empty-slice return for that case, but for any other err return nil,
err; adjust the branch that currently returns []string{}, nil to perform this
conditional check against the specific NotFound error.
---
Nitpick comments:
In `@internal/archetypes/archetypes_test.go`:
- Around line 8-97: The tests for GetScope duplicate identical length and
element-by-element checks across
TestGetScope_Devops/Architect/CastrojoDocs/CEO/NonexistentSlug/NoScopeSection;
extract a helper (e.g., assertScopeEqual(t *testing.T, got []string, want
[]string, name string)) that checks lengths and each index and calls
t.Fatalf/t.Errorf with contextual messages, then replace the repeated checks in
each TestGetScope_* to call that helper (or convert the tests to a table-driven
TestGetScope that iterates cases with name, slug, expected and uses the same
helper for assertions) so assertion logic is centralized and duplication removed
while still calling GetScope in each test.
🪄 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: CHILL
Plan: Pro
Run ID: 2e00eceb-d628-44d3-8753-bef1dcc683f6
📒 Files selected for processing (14)
internal/archetypes/archetypes.gointernal/archetypes/archetypes_test.gointernal/archetypes/architect.mdinternal/archetypes/auditor.mdinternal/archetypes/castrojo-docs.mdinternal/archetypes/castrojo-engineer-bluefin.mdinternal/archetypes/castrojo-engineer-cncf.mdinternal/archetypes/castrojo-qa.mdinternal/archetypes/ceo.mdinternal/archetypes/devops.mdinternal/archetypes/principle-engineer.mdinternal/archetypes/testdata/scope-boundary.mdinternal/templates/agent_detail.htmlinternal/templates/partials.html
Comment on lines
+110
to
+114
| data, err := Read(slug) | ||
| if err != nil { | ||
| // Archetype not found — return empty slice, no error (backward-compatible) | ||
| return []string{}, nil | ||
| } |
There was a problem hiding this comment.
Do not suppress non-not found read failures.
At Line 111, all Read errors are treated as “missing archetype.” This masks genuine IO/read failures and can silently return incorrect empty scopes.
🔧 Proposed fix
import (
"embed"
+ "errors"
+ "io/fs"
"os"
"path/filepath"
"strings"
)
@@
data, err := Read(slug)
if err != nil {
- // Archetype not found — return empty slice, no error (backward-compatible)
- return []string{}, nil
+ // Missing archetype is backward-compatible: empty scope, no error.
+ if errors.Is(err, fs.ErrNotExist) {
+ return []string{}, nil
+ }
+ // Propagate unexpected read failures.
+ return nil, err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/archetypes/archetypes.go` around lines 110 - 114, The current
Read(slug) error handling treats every read failure as “archetype not found” and
returns an empty slice, which hides real IO errors; update the logic around the
Read(slug) call so only a true “not found” condition returns []string{}, nil and
all other errors are returned to the caller. Concretely, detect the not-found
sentinel (e.g., errors.Is(err, ErrNotFound) or os.IsNotExist(err) depending on
your Read implementation) and keep the empty-slice return for that case, but for
any other err return nil, err; adjust the branch that currently returns
[]string{}, nil to perform this conditional check against the specific NotFound
error.
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
1 participant
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
Implements SO-89: adds
## Scopesections to archetype markdown files and aGetScope(slug string) ([]string, error)parser in Go.Changes
Archetype .md files (9 files updated)
Each file now contains a
## Scopesection with a comma-separated list of domain tags:ceodevopsarchitectauditorcastrojo-docscastrojo-engineer-bluefincastrojo-engineer-cncfcastrojo-qaprinciple-engineerinternal/archetypes/archetypes.goAdded
GetScope(slug string) ([]string, error):Read()(supports override dirs)## Scopesection, splits comma-separated tags[]string{}(empty slice, no error) for missing files or missing Scope sectioninternal/archetypes/archetypes_test.go(new file)6 unit tests:
TestGetScope_Devops— verifies infra/ci-cd/pipeline/workflow/opsTestGetScope_Architect— verifies application-code/go/typescript/svelte/architecture/design/fullstackTestGetScope_CastrojoDocs— verifies docs/documentation/writingTestGetScope_NonexistentSlug— empty slice, no error (AC4)TestGetScope_NoScopeSection—other.mdhas no Scope section → empty slice, no error (AC4)TestGetScope_CEO— verifies delegation/review/planningAcceptance Criteria
## ScopesectionGetScope(slug string) ([]string, error)implemented and returns scope listTest output
Closes SO-89. Unblocks SO-90 (API advisory warning) and SO-91 (UI scope-mismatch display).