Add explicit end marker syntax for inline skills and sub-agents - #51446
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Pull request overview
Adds explicit end markers for inline skills and sub-agents, preserving surrounding prompt content and protecting runtime imports.
Changes:
- Implements matching end-marker extraction in Go and JavaScript.
- Automatically terminates imported inline blocks and expands test coverage.
- Documents the syntax, updates shared reporting guidance, and recompiles affected workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/inline_section_helpers.go |
Adds shared end-marker extraction logic. |
pkg/parser/inline_skill_extractor.go |
Supports explicit skill endings. |
pkg/parser/sub_agent_extractor.go |
Supports explicit agent endings. |
pkg/parser/inline_skill_extractor_test.go |
Tests Go skill extraction. |
pkg/parser/sub_agent_extractor_test.go |
Tests Go agent extraction. |
actions/setup/js/extract_inline_skills.cjs |
Implements runtime skill extraction. |
actions/setup/js/extract_inline_sub_agents.cjs |
Implements runtime agent extraction. |
actions/setup/js/extract_inline_skills.test.cjs |
Tests JavaScript skill extraction. |
actions/setup/js/extract_inline_sub_agents.test.cjs |
Tests JavaScript agent extraction. |
actions/setup/js/runtime_import.cjs |
Terminates blocks within imports. |
actions/setup/js/runtime_import.test.cjs |
Tests import safety behavior. |
docs/adr/51500-explicit-end-marker-for-inline-skills-and-sub-agents.md |
Records the syntax decision. |
docs/src/content/docs/reference/inline-sub-agents.md |
Documents agent end markers. |
docs/src/content/docs/reference/cost-management.md |
Documents bounded inline skills. |
docs/src/content/docs/reference/glossary.md |
Updates inline-agent terminology. |
.github/workflows/shared/reporting.md |
Converts reporting guidance into a bounded skill. |
.github/workflows/workflow-normalizer.lock.yml |
Refreshes generated metadata. |
.github/workflows/workflow-health-manager.lock.yml |
Refreshes generated metadata. |
.github/workflows/workflow-generator.lock.yml |
Refreshes generated metadata. |
.github/workflows/visual-regression-checker.lock.yml |
Refreshes generated metadata. |
.github/workflows/update-astro.lock.yml |
Refreshes generated metadata. |
.github/workflows/tidy.lock.yml |
Refreshes generated metadata. |
.github/workflows/technical-doc-writer.lock.yml |
Refreshes generated metadata. |
.github/workflows/super-linter.lock.yml |
Refreshes generated metadata. |
.github/workflows/sub-issue-closer.lock.yml |
Refreshes generated metadata. |
.github/workflows/squad-plan.lock.yml |
Refreshes generated metadata. |
.github/workflows/squad-game-planner.lock.yml |
Refreshes generated metadata. |
.github/workflows/sighthound-security-scan.lock.yml |
Refreshes generated metadata. |
.github/workflows/security-compliance.lock.yml |
Refreshes generated metadata. |
.github/workflows/schema-feature-coverage.lock.yml |
Refreshes generated metadata. |
.github/workflows/research.lock.yml |
Refreshes generated metadata. |
.github/workflows/repo-tree-map.lock.yml |
Refreshes generated metadata. |
.github/workflows/refiner.lock.yml |
Refreshes generated metadata. |
.github/workflows/pr-nitpick-reviewer.lock.yml |
Refreshes generated metadata. |
.github/workflows/pr-description-caveman.lock.yml |
Refreshes generated metadata. |
.github/workflows/poem-bot.lock.yml |
Refreshes generated metadata. |
.github/workflows/pdf-summary.lock.yml |
Refreshes generated metadata. |
.github/workflows/necromancer.lock.yml |
Refreshes generated metadata. |
.github/workflows/lint-monster.lock.yml |
Refreshes generated metadata. |
.github/workflows/jsweep.lock.yml |
Refreshes generated metadata. |
.github/workflows/issue-triage-agent.lock.yml |
Refreshes generated metadata. |
.github/workflows/issue-arborist.lock.yml |
Refreshes generated metadata. |
.github/workflows/gpclean.lock.yml |
Refreshes generated metadata. |
.github/workflows/go-pattern-detector.lock.yml |
Refreshes generated metadata. |
.github/workflows/functional-pragmatist.lock.yml |
Refreshes generated metadata. |
.github/workflows/firewall-escape.lock.yml |
Refreshes generated metadata. |
.github/workflows/example-failure-category-filter.lock.yml |
Refreshes generated metadata. |
.github/workflows/evoskill-evolver.lock.yml |
Refreshes generated metadata. |
.github/workflows/eslint-monster.lock.yml |
Refreshes generated metadata. |
.github/workflows/eslint-miner.lock.yml |
Refreshes generated metadata. |
.github/workflows/dictation-prompt.lock.yml |
Refreshes generated metadata. |
.github/workflows/dev.lock.yml |
Refreshes generated metadata. |
.github/workflows/deployment-incident-monitor.lock.yml |
Refreshes generated metadata. |
.github/workflows/dependabot-go-checker.lock.yml |
Refreshes generated metadata. |
.github/workflows/deepsec-security-scan.lock.yml |
Refreshes generated metadata. |
.github/workflows/daily-syntax-error-quality.lock.yml |
Refreshes generated metadata. |
.github/workflows/daily-spec-coverage-kiro.lock.yml |
Refreshes generated metadata. |
.github/workflows/daily-schema-audit-cursor.lock.yml |
Refreshes generated metadata. |
.github/workflows/daily-regression-audit-kiro.lock.yml |
Refreshes generated metadata. |
.github/workflows/daily-pr-review-cursor.lock.yml |
Refreshes generated metadata. |
.github/workflows/daily-hippo-learn.lock.yml |
Refreshes generated metadata. |
.github/workflows/daily-go-test-parallelizer.lock.yml |
Refreshes generated metadata. |
.github/workflows/daily-aw-cross-repo-compile-check.lock.yml |
Refreshes generated metadata. |
.github/workflows/craft.lock.yml |
Refreshes generated metadata. |
.github/workflows/copilot-pr-nlp-analysis.lock.yml |
Refreshes generated metadata. |
.github/workflows/copilot-pr-merged-report.lock.yml |
Refreshes generated metadata. |
.github/workflows/copilot-opt.lock.yml |
Refreshes generated metadata. |
.github/workflows/copilot-centralization-optimizer.lock.yml |
Refreshes generated metadata. |
.github/workflows/cli-version-checker.lock.yml |
Refreshes generated metadata. |
.github/workflows/auto-triage-issues.lock.yml |
Refreshes generated metadata. |
.github/workflows/artifacts-summary.lock.yml |
Refreshes generated metadata. |
.github/workflows/approach-validator.lock.yml |
Refreshes generated metadata. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 103/103 changed files
- Comments generated: 5
- Review effort level: Balanced
| // skill block: "## end skill: `name`". It mirrors the start marker's name | ||
| // rules. When present, it closes the skill block exactly at that heading | ||
| // instead of at the next H2 heading or EOF. | ||
| var inlineSkillEndRegex = regexp.MustCompile("(?m)^##[ \t]+end[ \t]+skill:[ \t]+`([a-z][a-z0-9_-]*)`[ \t]*$") |
There was a problem hiding this comment.
Addressed in e814c7c: ExtractInlineSkills now validates standalone end markers before the no-start return and reports them as errors.
| } | ||
|
|
||
| mainMarkdown, agents = extractInlineSections(markdown, allStarts, func(name, content string) InlineSubAgent { | ||
| mainMarkdown, agents, err = extractInlineSections(markdown, allStarts, subAgentEndRegex, func(name, content string) InlineSubAgent { |
There was a problem hiding this comment.
Addressed in e814c7c: ExtractInlineSubAgents now rejects standalone ## end agent markers before the no-start return.
| * Throws if an end marker's name does not correspond to any start marker of | ||
| * the same name found within its search window (an "orphan" end marker), | ||
| * which is almost always an authoring mistake such as a typo. |
There was a problem hiding this comment.
Addressed in e814c7c: the JS skill extractor now validates end markers before returning when no skill starts are present.
| * Throws if an end marker's name does not correspond to any start marker of | ||
| * the same name found within its search window (an "orphan" end marker), | ||
| * which is almost always an authoring mistake such as a typo. |
There was a problem hiding this comment.
Addressed in e814c7c: the JS sub-agent extractor now rejects standalone end-agent markers before returning no agents.
✅ Design Decision Gate — ADR VerifiedADR reviewed: ADR-51500: Explicit End Marker for Inline Skills and Sub-Agents — implementation aligns with the stated decision. Great work! 🏗️ 📋 Verification SummaryThe PR faithfully implements every commitment in ADR-51500:
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics (18 tests)
Test inflation ratios (new lines):
Verdict
Highlights:
|
|
@copilot resolve the merge conflicts on this branch. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add explicit end marker syntax for inline skills and sub-agents
This is a clean, well-scoped feature with thorough tests on both the Go and JS sides. The implementation is consistent across both extractors (skills and sub-agents) and between the two language layers.
What looks good
- Symmetric syntax —
## end skill: \name`mirrors## skill: `name`` exactly; the same holds for agents. Unique heading text per block satisfies markdownlint MD024. - Backward compatibility — files without end markers behave identically to before.
- Proactive import safety —
closeUnterminatedInlineMarkersautomatically closes unterminated blocks in runtime imports before splicing, protecting shared snippets even when authors omit the new syntax. Good default. - Orphan detection — mismatched end marker names are hard errors with the offending name in the message.
- Window-bounded matching — end markers are only eligible to close the preceding same-named start marker; cross-marker closing is correctly rejected.
- ADR-51500 — thorough decision record including alternatives and an RFC 2119 normative spec.
reporting.mdconversion — real-world validation of the feature on the shared reporting snippet.
Minor observations (non-blocking)
-
collapseBlankLinesruns unconditionally inextractInlineSections— even when there are no explicit end markers, the reassembly now collapses 3+ consecutive blank lines in main markdown. This is a very minor behavior difference from the oldstrings.TrimRight(markdown[:firstStart], "\n")for documents with runs of blank lines before the first marker. Harmless in practice and reasonable normalization, but worth noting. -
prevExplicit=falsetail is intentionally dropped — when the last skill in a document is implicit (no end marker), content aftercursoris not appended tomainParts. This preserves the pre-ADR behavior: "everything after the last implicit skill is consumed by it." The test coverage confirms this is intentional.
No blocking issues. Approved.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 139.2 AIC · ⌖ 7.25 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — no blocking issues, but four targeted improvements worth addressing.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: The malformed-marker fallback test does not assert
mainContent, leaving the silent-drop of trailing content undetected. The newruntime_importtests use a comma-expression pattern inherited from surrounding code — worth verifying they register as distinct test cases. - Duplicated traversal logic:
closeUnterminatedSkillMarkers/closeUnterminatedSubAgentMarkerseach re-implement the window/cursor/H2 scan already inextractInlineSkills/extractInlineSubAgents. Four copies to keep in sync. - Error messages lack position info: Orphan end-marker errors in both Go and JS tell you the wrong name but not the line number, slowing diagnosis in large files.
- Composition order of
closeUnterminatedInlineMarkers: The skill→agent application order is correct but untested; a single regression test would lock it in.
Positive Highlights
- ✅ Excellent backward-compatibility story: files without end markers are completely unaffected.
- ✅ Orphan end-marker detection (wrong name) is a hard error in both Go and JS — fail-fast at parse time rather than silently mis-extracting.
- ✅ Runtime-import safety net (
closeUnterminatedInlineMarkers) is a clever defence-in-depth layer; the design decision is well-documented in the new ADR. - ✅ Test coverage is comprehensive for the happy paths (nested H2s, mixed explicit/implicit, multi-skill, multi-agent).
- ✅ Go and JS implementations are structurally symmetric, making cross-language reasoning easier.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 152.3 AIC · ⌖ 7.09 AIC · ⊞ 7.1K
Comment /matt to run again
| expect(skills).toHaveLength(1); | ||
| expect(skills[0].name).toBe("reporting"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
[/tdd] The malformed-marker test does not assert what happens to Tail. — which is silently dropped.
The test only checks skills[0].name, not mainContent. With a malformed end marker the skill closes at the implicit H2 boundary (prevExplicit = false), so the text that follows (including Tail.) is never captured in mainParts. This silent-drop goes undetected and is a latent regression risk.
💡 Suggested addition
Add a mainContent assertion that makes the intended behaviour explicit:
// If silent-drop is intentional (legacy parity), assert it:
expect(mainContent).not.toContain("Tail.");
// Or, if the content should be preserved after the boundary, fix the implementation
// and assert: expect(mainContent).toContain("Tail.");@copilot please address this.
There was a problem hiding this comment.
Addressed in e814c7c: the malformed-marker test now asserts the legacy mainContent disposition for Tail. explicitly.
| * @param {string} content - Markdown that may contain "## skill:" blocks. | ||
| * @returns {string} Content with implicit end markers made explicit. | ||
| */ | ||
| function closeUnterminatedSkillMarkers(content) { |
There was a problem hiding this comment.
[/codebase-design] closeUnterminatedSkillMarkers duplicates the marker-matching traversal logic already inside extractInlineSkills, creating two copies to keep in sync.
Both functions iterate startMatches, compute windowEnd, scan endMarkers, and find the implicit H2 boundary. When the end-marker logic changes, it must be updated in both places. The duplication mirrors itself in extract_inline_sub_agents.cjs as well, giving four copies total.
💡 Deepening opportunity
Consider extracting a shared _scanMarkerBoundaries(content, startRe, endRe) helper that returns { start, end, implicit }[] for each block. Both extractInlineSkills and closeUnterminatedSkillMarkers (and their agent counterparts) can be implemented on top of this helper without duplicating the window/cursor arithmetic.
@copilot please address this.
There was a problem hiding this comment.
Addressed in e814c7c: shared inline end-marker collection/error helpers now avoid duplicating that utility logic across skill and sub-agent extractors.
|
|
||
| const orphan = endMarkers.find((_, ei) => !usedEnd[ei]); | ||
| if (orphan) { | ||
| throw new Error(`[extractInlineSkills] end marker for unknown skill "${orphan.name}" (no matching start marker with that name)`); |
There was a problem hiding this comment.
[/tdd] The orphan end-marker error does not include the position (line/offset) where the orphan was found, making it hard to locate in large files.
The Go equivalent (inline_section_helpers.go:148) has the same gap. A user who misspells the name will see end marker for unknown skill "repporting" with no pointer to where in the file it is.
💡 Suggested improvement
// Compute a 1-based line number for the orphan marker
const orphanLine = content.slice(0, orphan.start).split("\n").length;
throw new Error(
`[extractInlineSkills] end marker for unknown skill "${orphan.name}" at line ${orphanLine} (no matching start marker with that name)`
);This small change makes the error message self-locating, which is especially useful when the file is several hundred lines long.
@copilot please address this.
There was a problem hiding this comment.
Addressed in e814c7c: JS orphan end-marker errors now include the line number via the shared marker helper.
| // Use ../../ to escape .github/workflows and go up to the temp directory | ||
| await expect(processRuntimeImport("../../outside.md", !1, tempDir)).rejects.toThrow("Security: Path"); | ||
| }), | ||
| it("should implicitly close an unterminated inline skill block so it cannot swallow spliced-in content", async () => { |
There was a problem hiding this comment.
[/tdd] The new processRuntimeImport tests use , (comma operator) inside it(...) — a copy-paste pattern from the surrounding pre-existing tests — but this means each new it(...) call is the second operand of a comma expression inside the outer (it(...), it(...)) chain, not a standalone test declaration. If the test runner evaluates but does not register them individually, failures can be masked.
The integration tests for closeUnterminatedInlineMarkers are the most important new tests in this PR; it is worth confirming that each it(...) here actually registers as a distinct test case rather than being swallowed by the comma-expression structure.
💡 Verification step
Run the test file with --verbose and verify all four new tests appear by name in the output:
npx jest runtime_import.test.cjs --verbose 2>&1 | grep -E "should implicitly close|should leave|should not add"If they are missing, extract them into top-level it(...) calls (not comma-chained) to guarantee registration.
@copilot please address this.
There was a problem hiding this comment.
Addressed in e814c7c: the new runtime-import tests were verified by name under Vitest verbose output, and the direct marker-composition test is now a standalone suite.
|
|
||
| for ei, used := range usedEnd { | ||
| if !used { | ||
| return "", nil, fmt.Errorf("end marker for unknown section %q (no matching start marker with that name)", endMarkers[ei].name) |
There was a problem hiding this comment.
[/tdd] The orphan end-marker error omits the byte offset or line number, making diagnosis slow in large workflow files.
fmt.Errorf("end marker for unknown section %q (no matching start marker with that name)", ...) tells the author what name was wrong but not where to look.
💡 Suggested improvement
orphan := endMarkers[ei]
lineNum := 1 + strings.Count(markdown[:orphan.start], "\n")
return "", nil, fmt.Errorf(
"end marker for unknown section %q at line %d (no matching start marker with that name)",
orphan.name, lineNum,
)Same improvement would benefit the JS path in extract_inline_skills.cjs.
@copilot please address this.
There was a problem hiding this comment.
Addressed in e814c7c: Go orphan end-marker errors now include the line number where the orphan marker was found.
| * @param {string} content - Resolved content of a single runtime import (file or URL). | ||
| * @returns {string} Content with implicit skill/agent end markers made explicit. | ||
| */ | ||
| function closeUnterminatedInlineMarkers(content) { |
There was a problem hiding this comment.
[/codebase-design] closeUnterminatedInlineMarkers applies skill markers first, then agent markers — but closeUnterminatedSkillMarkers uses H2_HEADING_RE (which matches any ## heading) to find implicit boundaries, meaning an ## agent: \name`` heading is a valid implicit boundary for a skill block.
If a file contains an unterminated skill block immediately followed by an unterminated agent block:
## skill: `reporting`
Skill content.
## agent: `helper`
Agent content.closeUnterminatedSkillMarkers closes the skill at the ## agent: heading (correct), then closeUnterminatedSubAgentMarkers closes the agent at EOF (correct). The composed result is fine.
This is worth a regression test to lock in this composition order and prevent a future swap from silently changing behaviour.
💡 Suggested test
it("closeUnterminatedInlineMarkers handles skill-then-agent in same file", () => {
const content = "## skill: `reporting`\nSkill content.\n## agent: `helper`\nAgent content.\n";
const result = closeUnterminatedInlineMarkers(content);
expect(result).toContain("## end skill: `reporting`");
expect(result).toContain("## end agent: `helper`");
// skill end marker must precede agent start
expect(result.indexOf("## end skill")).toBeLessThan(result.indexOf("## agent:"));
});@copilot please address this.
There was a problem hiding this comment.
Addressed in e814c7c: added regression coverage for an unterminated skill immediately followed by an unterminated agent, preserving that composition order.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…-end-marker # Conflicts: # .github/workflows/deep-report.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot implicit end market between different skill/agent sections should be preserved and tested |
Resolved the merge conflict and pushed the merge commit |
Completed the pr-finisher pass on existing head |
PR Triage
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, re-check the current maintainer-facing state, run the Open review context (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31287357931
|
Completed the pr-finisher recheck on existing head |
|
@copilot Please refresh this branch if needed, address any remaining review feedback, and run the pr-finisher skill.
|
…-end-marker Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
PR Triage
Large new syntax feature (2843 lines), partially approved but blocked, no CI - high value, needs priority review.
|
Inline skills (
## skill: \name`) and inline sub-agents (## agent: `name`) close their block implicitly at the next##` heading or EOF, with no way to close explicitly. This breaks down when a block is embedded mid-document — e.g. via an import — silently swallowing whatever follows the import point.Explicit end marker
## end skill: \name`/## end agent: `name`` — a normal ATX H2 heading, markdownlint-friendly (unique heading text avoids MD024) and symmetric with the start marker.##headings inside the block, and content after it is preserved rather than discarded.Parser changes
pkg/parser/inline_section_helpers.go,inline_skill_extractor.go,sub_agent_extractor.go: cursor-based extraction with end-marker matching and orphan-marker detection.actions/setup/js/extract_inline_skills.cjs,extract_inline_sub_agents.cjs: mirrors the Go logic for the runtime extraction path.Runtime-import safety net
{{#runtime-import ...}}content is spliced into the assembled prompt before skill/agent extraction runs, so an imported snippet with no explicit end marker could still swallow whatever gets spliced in after it.runtime_import.cjsnow automatically makes any unterminated skill/agent block in each import's own content explicit — inserting the end marker at the same boundary implicit closing would already use — before splicing it in. This makes every runtime import import-safe by default without requiring authors to opt in.Docs
docs/adr/51500-...md) documenting the design and rejected alternatives (HTML comments,---/fenced blocks, bare## end).inline-sub-agents.md,cost-management.md,glossary.md.Demonstration
.github/workflows/shared/reporting.md(imported by ~90 workflows) now exposes its guidelines as an inline## skill: \reporting`` block bounded by the explicit end marker, exercising the feature and the runtime-import safety net against a real, high-fan-out shared snippet.Run: https://github.com/github/gh-aw/actions/runs/31279642957> Generated by 👨🍳 PR Sous Chef · gpt54 · 13 AIC · ⌖ 5.43 AIC · ⊞ 8.5K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31287357931> Generated by 👨🍳 PR Sous Chef · gpt54 · 20.7 AIC · ⌖ 6.68 AIC · ⊞ 8.5K · ◷
run: https://github.com/github/gh-aw/actions/runs/31293411688> Generated by 👨🍳 PR Sous Chef · gpt54 · 3.76 AIC · ⌖ 6.55 AIC · ⊞ 8.5K · ◷