fix: strip scripts: from extension command frontmatter in command mode - #4559
Open
chelsealong wants to merge 2 commits into
Open
fix: strip scripts: from extension command frontmatter in command mode#4559chelsealong wants to merge 2 commits into
scripts: from extension command frontmatter in command mode#4559chelsealong wants to merge 2 commits into
Conversation
CommandRegistrar.register_commands() renders extension commands for
non-SKILL.md agents (markdown/toml/yaml formats) by consuming the
scripts: block for {SCRIPT} substitution via resolve_skill_placeholders(),
but never removed the now-unused key from the frontmatter dict before
serializing the final output. IntegrationBase.process_template() (the
core template path) already strips scripts: as its own step 3, so core
and extension renders disagreed and a build-time key leaked into
agent-facing files (e.g. .github/agents/*.agent.md).
Pop scripts: from the frontmatter after it has been consumed, scoped to
extension-owned commands (extension_id set) so preset command rendering,
which intentionally keeps scripts: at this layer for the wrap-strategy
composition tested in test_register_commands_inherits_scripts_from_core,
is unaffected.
Fixes github#4554
Assisted-by: Claude Code (model: claude-sonnet-5, autonomous)
The toml and yaml branches of register_commands() popped scripts: from frontmatter defensively, copying the pattern from the markdown branch. But render_toml_command() only ever reads the description key off frontmatter, and render_yaml_command() only ever reads title/description — neither serializes the full frontmatter dict, so scripts: can never leak into their output. Confirmed via a standalone repro that both branches produce identical output with or without the pop. Keep only the markdown-branch pop, which is the one that fixes the actual leak described in github#4554. Assisted-by: Claude Code (model: claude-sonnet-5, autonomous)
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
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.
Description
Fixes #4554.
CommandRegistrar.register_commands()renders extension commands fornon-SKILL.md agents by consuming the source
scripts:frontmatterblock for
{SCRIPT}substitution viaresolve_skill_placeholders().In the
markdownbranch (e.g. Copilot's.github/agents/*.agent.mdcommands-mode layout), the now-consumed key was never removed from the
frontmatter dict before
render_markdown_command()serialized thefull dict as the final file's frontmatter.
IntegrationBase.process_template()— the core template render path — already strips
scripts:as anexplicit step, so the two render paths disagreed and a build-time key
leaked into agent-visible output, exactly as described in the issue.
The fix pops
scriptsfromfrontmatterright after theresolve_skill_placeholders()call in themarkdownbranch ofregister_commands(), once it's no longer needed. The pop is scopedto
extension_id is not None(i.e. extension-owned commands only)because
register_commands()is also shared by the preset system,where an existing test (
test_register_commands_inherits_scripts_from_core)intentionally asserts that a
strategy: wrappreset command retainsthe merged
scripts:key in this same render step. That's a separate,pre-existing behavior outside the scope of this issue, so it's left
untouched.
The
tomlandyamlbranches are not affected by this leak and needno pop:
render_toml_command()only ever reads thedescriptionkeyoff
frontmatter, andrender_yaml_command()only ever readstitle/description— neither serializes the full frontmatter dict,so there is nothing for a
scripts:key to leak into. (An earlierversion of this PR added the same pop to those two branches as
a defensive copy-paste; removed after confirming with a standalone
repro that it was dead code — neither branch's output ever contains a
scripts:key, fix or no fix.)Skills-mode extension rendering (
_register_extension_skills/register_skill_command) was already unaffected — those paths build afresh
SKILL.mdfrontmatter dict from scratch and never copyscripts:into it — so no change was needed there.
Testing
Added
test_command_mode_registration_strips_scripts_keyintests/test_extensions.py, which installs an extension commanddeclaring
scripts:and registers it for thecopilotagent (commandmode,
.agent.mdoutput) — mirroring the issue's reproduction.Confirmed the test fails without the fix:
And passes with the fix applied:
Full
test_extensions.py+test_presets.pysuite (unmodified elsewhere):AI Disclosure
This PR was written primarily by an autonomous AI coding agent (Claude
Code, model Claude Sonnet 5), including the investigation, the code
change, and the regression test. I reviewed the diff and the test
output before opening this PR.