Repository navigation
Fix Claude persona evaluator registration - #66634
Conversation
Declare the required Claude agent name and use the native inherit model value. Preserve parent publishing instructions with an explicit inline agent boundary, add a workflow contract regression, and correct the sub-agent guidance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
3 open findings
What changed in this PR
Adds a contract test and updates the agent-persona-explorer workflow/docs to ensure Claude sub-agent extraction and frontmatter conform to expected conventions (explicit name, correct inheritance sentinel, and proper block boundaries).
Changes:
- Added a non-integration Go contract test that validates inline sub-agent extraction and required frontmatter fields.
- Updated the
persona-evaluatorsub-agent block to includename:, usemodel: inherit, and explicitly close the agent block before resuming parent instructions. - Updated sub-agent authoring documentation to reflect engine-native frontmatter behavior and Claude requirements.
| File | Description |
|---|---|
| pkg/cli/agent_persona_explorer_workflow_contract_test.go | Adds a contract test asserting sub-agent extraction boundaries and Claude-required frontmatter. |
| .github/workflows/agent-persona-explorer.md | Fixes sub-agent frontmatter (name, model) and adds an explicit “end agent” boundary before parent criteria. |
| .github/workflows/agent-persona-explorer.lock.yml | Regenerates lock metadata/harness version to match updated workflow body hash. |
| .github/aw/subagents.md | Updates documentation on sub-agent boundaries/frontmatter semantics and adds a Claude-specific example. |
🧠 Review effort: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| ### Block boundary | ||
|
|
||
| The block ends at the next `##` heading (any level-2 heading) or at EOF — no explicit end marker is needed. Place sub-agent blocks **at the bottom** of the file, after all main workflow content. | ||
| The block ends at the next `##` heading (any level-2 heading) or at EOF. Place sub-agent blocks **at the bottom** of the file, after all main workflow content. If parent instructions follow a sub-agent, close it with ``## end agent: `name` `` so those instructions remain in the main prompt. |
| ## end agent: `persona-evaluator` | ||
|
|
| @@ -0,0 +1,40 @@ | |||
| //go:build !integration | |||
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #66634 does not have the implementation label (has_implementation_label=false) and has 40 new lines in business logic directories, below the 100-line threshold (4 files changed, no custom .design-gate.yml).
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comment
The workflow fix itself looks reasonable, but the new regression coverage is attached to the Go parser instead of the JavaScript runtime extractor that actually writes the Claude sub-agent file, so the failure path that triggered this PR is still only manually verified.
Details
Please add the regression at the runtime extraction layer (actions/setup/js/extract_inline_sub_agents.*) so a future JS-side change cannot reintroduce the missing name/model fields or accidentally pull ## Success Criteria back into the generated agent file without breaking tests.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 68 AIC · ⌖ 5.35 AIC · ⊞ 19.6K
Comment /review to run again
| } | ||
|
|
||
| content, err := os.ReadFile(filepath.Join(repoRoot, ".github", "workflows", "agent-persona-explorer.md")) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
The new regression test only exercises the Go parser, so it would still pass if the JavaScript runtime extractor regressed again and wrote a Claude agent without the required name/model fields.
💡 Cover the runtime write path that actually failed
The production failure happened in the interpolate_prompt/extract_inline_sub_agents.cjs path that writes the engine-specific agent file, but this test stops at parser.ExtractInlineSubAgents and ExtractFrontmatterFromContent. That gives you a green test even if the JS extractor starts stripping or rewriting the frontmatter again.
A tighter regression would add a case next to actions/setup/js/extract_inline_sub_agents.test.cjs that runs writeInlineSubAgents(...) for the persona workflow and asserts the generated Claude file still contains name: persona-evaluator and model: inherit, and still excludes ## Success Criteria.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /grill-with-docs. Fallback classification used (pr-triage agent returned a parseable result, no fallback needed) — bug_fix, high-impact files: .github/workflows/agent-persona-explorer.md, pkg/cli/agent_persona_explorer_workflow_contract_test.go, .github/aw/subagents.md.
Verification performed: Ran go test ./pkg/cli -run '^TestAgentPersonaExplorerWorkflowSubAgentContract$' -count=1 against the PR's actual committed content (clean checkout) — it passes. The regression test correctly exercises the root cause (missing name, wrong inherited value, and marker-boundary extraction) rather than just the symptom.
📋 Key Themes & Highlights
Key Themes
- Root cause fix, well-tested (
/diagnosing-bugs): the newpkg/cli/agent_persona_explorer_workflow_contract_test.goasserts onname,description,model: inherit, and that parent publishing instructions (## Success Criteria) survive extraction — this is exactly the kind of regression test that would have caught the original silent-skip bug. - Doc/example drift (
/grill-with-docs): the new Claudename:requirement documented insubagents.mdisn't reflected in the file's own unchanged examples (dependency-scanner,diff-explainer, etc.), which could mislead a reader copying those patterns for a Claude-engine workflow. Flagged inline. - Markdown rendering nit: nested backticks in
name
Positive Highlights
- ✅ Explicit
## end agent: \name`end marker is a clean, minimal addition that reuses the existingextractInlineSections` boundary-preservation machinery already proven out for skills — no new parsing logic needed. - ✅ Lock file regeneration diff is exactly as expected (hash + env var only).
- ✅ PR description is clear about root cause, fix, and validation scope, including an honest caveat about
--dry-runbeing blocked by unrelated pre-existing warnings.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 74.2 AIC · ⌖ 14.6 AIC · ⊞ 10.1K
Comment /matt to run again
| ### Block boundary | ||
|
|
||
| The block ends at the next `##` heading (any level-2 heading) or at EOF — no explicit end marker is needed. Place sub-agent blocks **at the bottom** of the file, after all main workflow content. | ||
| The block ends at the next `##` heading (any level-2 heading) or at EOF. Place sub-agent blocks **at the bottom** of the file, after all main workflow content. If parent instructions follow a sub-agent, close it with ``## end agent: `name` `` so those instructions remain in the main prompt. |
There was a problem hiding this comment.
[/diagnosing-bugs] Nested backtick code span (## end agent: `name` ) will render incorrectly in GitHub-flavored Markdown — GFM closes the span at the first matching double-backtick sequence, so this is likely to display broken.
💡 Suggested fix
Use a single backtick span with the literal name escaped, or switch to a fenced inline example:
close it with `## end agent: \`name\`` so those instructions remain in the main prompt.or simply drop the inner backticks around name since it is already inside the outer span.
@copilot please address this.
| |---|---|---|---| | ||
| | `description` | No | — | Human-readable summary of the sub-agent's role | | ||
| | `model` | No | `"inherited"` | Model override; `"inherited"` uses the parent workflow's model. Prefer model aliases (e.g. `small`, `large`) over specific model IDs for portability. | | ||
| | `name` | Claude: yes | — | Claude's registered agent identifier. Use the same name as the inline heading. | |
There was a problem hiding this comment.
[/grill-with-docs] Good catch documenting Claude's silent-skip behavior and the name requirement. However, the unchanged examples later in this same file (dependency-scanner, test-coverage, secret-scanner, diff-explainer under "When to Use Sub-Agents" / "Full Example") still omit name: in their frontmatter, now contradicting the rule just introduced here.
💡 Suggested fix
Either (a) add name: to all example blocks for consistency, or (b) add a one-line caveat noting those examples target the Copilot engine (which does not require name) so readers don't copy a pattern that silently breaks on Claude.
@copilot please address this.
There was a problem hiding this comment.
Impeccable Review — clarify (docs) / harden (regression test)
Reviewed .github/aw/subagents.md, agent-persona-explorer.md, and the new Go contract test.
Verified correctness:
- The explicit
## end agent: \persona-evaluator`marker correctly stops the extracted sub-agent content before## Success Criteria, and the marker line itself does **not** leak into the main prompt (confirmed viaparser.ExtractInlineSubAgents` against the committed file). model: inherit+name: persona-evaluatormatch Claude's native frontmatter contract; the regression test (TestAgentPersonaExplorerWorkflowSubAgentContract) passes against this PR's commit..github/aw/subagents.mdis internally consistent (no longer documents the non-existent"inherited"sentinel as a cross-engine default).
No new blocking issues found beyond the existing inline review comments already on this PR (nested-backtick code-span rendering nit in subagents.md:34, and the build-tag question on the new test file) — those are non-blocking style nits, and this repo's convention is (go/redacted):build only (no legacy // +build), so that existing suggestion doesn't apply here.
Note: .github/workflows/dependabot-burner.md still uses model: inherited under engine: copilot — same latent issue this PR fixes for Claude, but out of scope for this change since that file isn't touched here.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 103.4 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
🎉 This pull request is included in a new release. Release: |
Summary
Agent Persona Explorer published a partial-results report without evaluating any scenarios because Claude silently skipped its inline evaluator. The generated definition lacked Claude's required
namefield and usedinheritedinstead of the nativeinheritmodel value.Fixes #66615
Validation
go test ./pkg/cli -run '^TestAgentPersonaExplorerWorkflowSubAgentContract$' -count=1passes; the regression failed against the original declaration.make fmtandmake recompilepass.PATH="/opt/homebrew/bin:$PATH" make agent-report-progresspasses, including impacted Go tests and workflow drift checks.Strict
--dry-runcompilation remains blocked by existing experimental-feature warnings forgradersandgh-aw-detection. Those settings were not changed, and no live Actions run was dispatched.