From 471dbe9e9e71f25854c9cb81f0a7fcf78b4e1cd8 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Sat, 8 Aug 2026 11:51:43 -0400 Subject: [PATCH] test(skills): guard the frontmatter fix with a real malformed SKILL.md on disk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests added with the parser fix build their own SKILL.md files in tmp directories. They keep passing even if every real skill bundle in the repo is normalised, so nothing exercises discovery against a genuinely malformed file sitting on disk — which is the only form the original bug ever took. Adds one to the example app. `examples/.../skills/smoke_frontmatter/SKILL.md` is written the way a Windows or Office editor writes one: first three bytes are a UTF-8 BOM (EF BB BF, invisible in every editor), and the frontmatter keys use underscores rather than hyphens. Either one, before the fix, made the whole block parse as body: the skill owned no tools *and* `allowed-tools:` leaked into the rendered PROCEDURE. Both halves were silent. Three tests in tests/scaffold/test_skill_resolve.py assert on the shipped bytes rather than a copy: - `test_fixture_is_still_malformed` guards the guard. An editor that strips BOMs on save would otherwise quietly turn this file into an ordinary skill and take the regression with it, with every test still green. - `test_shipped_fixture_parses_through_discovery` runs `parse_skill_bundle` over the real directory: tools parsed, requires-actions parsed, no frontmatter in the body. It asserts on tokens that only appear inside the frontmatter block — the body legitimately mentions `allowed-tools:` while explaining itself. - `test_fixture_is_exposed_by_the_example_agent` reads agent.yaml, because a fixture no agent loads is not exercising discovery at all. Mutation-checked four ways, each caught by the intended test: strip the BOM, hyphenate the keys, unregister the skill from agent.yaml, and revert the parser's BOM strip in skill_resolve.py. The SKILL.md body states that it is deliberately malformed and names the test that depends on it, so the next person to open it does not helpfully repair it. agent.yaml gets the registration line only. The gating and the deliberate whatsapp/whatsapp_call sibling gap used while verifying this by hand are NOT included: the example documents itself as gating nothing, and the sibling gap would make `jvagent validate` emit an advisory on the shipped example app for every user. `validate` and `validate --strict` both exit 0. --- .../jvagent/orchestrator_agent/agent.yaml | 2 + .../skills/smoke_frontmatter/SKILL.md | 30 ++++++++++ tests/scaffold/test_skill_resolve.py | 60 +++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 examples/jvagent_app/agents/jvagent/orchestrator_agent/skills/smoke_frontmatter/SKILL.md diff --git a/examples/jvagent_app/agents/jvagent/orchestrator_agent/agent.yaml b/examples/jvagent_app/agents/jvagent/orchestrator_agent/agent.yaml index 983e282a..1ccfd4ad 100644 --- a/examples/jvagent_app/agents/jvagent/orchestrator_agent/agent.yaml +++ b/examples/jvagent_app/agents/jvagent/orchestrator_agent/agent.yaml @@ -256,6 +256,8 @@ actions: - whatsapp_templates # Meta HSM send via WhatsAppAction (WhatsApp inbound only) - whatsapp_flows # Meta Flow interactive send (WhatsApp text / voice-call) - whatsapp_service_flows # signup/signin + appointment booking Flow intents + - smoke_frontmatter # regression fixture: its SKILL.md is deliberately + # BOM-prefixed with underscore keys — do not 'fix' it # MCP gateway: a sandboxed filesystem server (npx). Its tools surface to the # executive (tool_servers above) as mcp_filesystem__, sandboxed per diff --git a/examples/jvagent_app/agents/jvagent/orchestrator_agent/skills/smoke_frontmatter/SKILL.md b/examples/jvagent_app/agents/jvagent/orchestrator_agent/skills/smoke_frontmatter/SKILL.md new file mode 100644 index 00000000..9492d0cf --- /dev/null +++ b/examples/jvagent_app/agents/jvagent/orchestrator_agent/skills/smoke_frontmatter/SKILL.md @@ -0,0 +1,30 @@ +--- +name: smoke_frontmatter +description: Regression fixture - declares its tools with underscore keys behind a UTF-8 BOM. +spec: jv +allowed_tools: + - file_interface__list_directory + - file_interface__read_file +requires_actions: + - FileInterfaceAction +tags: + - smoke +--- + +# Frontmatter Smoke - Standard Operating Procedure + +> **This file is deliberately malformed. Do not "fix" it.** +> +> It is written the way a Windows or Office editor writes one: the first three +> bytes are a UTF-8 BOM (`EF BB BF`, invisible in every editor), and the +> frontmatter keys use underscores rather than the canonical hyphens. Both +> spellings once produced the same silent failure - the frontmatter parsed as +> body, so the skill owned no tools *and* `allowed-tools:` leaked into the +> rendered PROCEDURE. `tests/scaffold/test_skill_resolve.py` asserts on this +> exact file; normalising it would delete the regression it guards. + +Use this procedure when the user asks to list or read their stored files. + +1. Call `file_interface__list_directory` to see what the user has. +2. If they named a file, read it with `file_interface__read_file`. +3. Answer with what the files actually contain. Do not invent filenames. diff --git a/tests/scaffold/test_skill_resolve.py b/tests/scaffold/test_skill_resolve.py index cb010b4d..57c07ab7 100644 --- a/tests/scaffold/test_skill_resolve.py +++ b/tests/scaffold/test_skill_resolve.py @@ -430,3 +430,63 @@ def test_unrelated_unknown_key_is_left_alone( ) assert meta["team_owner"] == "platform" assert not caplog.records + + +class TestShippedFrontmatterFixture: + """The example app carries a deliberately malformed SKILL.md. + + The inline cases above build their own files, so they keep passing even if + the shipped fixture is quietly normalised by an editor that strips BOMs on + save — and then nothing exercises the real discovery path against a real + file on disk. These assert on the shipped bytes themselves. + """ + + FIXTURE = ( + Path(__file__).resolve().parents[2] + / "examples" + / "jvagent_app" + / "agents" + / "jvagent" + / "orchestrator_agent" + / "skills" + / "smoke_frontmatter" + ) + + def test_fixture_is_still_malformed(self) -> None: + """Guards the guard: a stripped BOM makes this suite silently weaker.""" + raw = (self.FIXTURE / "SKILL.md").read_bytes() + assert raw.startswith( + b"\xef\xbb\xbf" + ), "the UTF-8 BOM is the point of this file" + assert b"allowed_tools:" in raw, "underscore spelling is the point of this file" + assert b"requires_actions:" in raw + + def test_shipped_fixture_parses_through_discovery(self) -> None: + from jvagent.scaffold.skill_resolve import parse_skill_bundle + + data = parse_skill_bundle(self.FIXTURE, source="app") + assert data is not None + assert data["allowed_tools"] == [ + "file_interface__list_directory", + "file_interface__read_file", + ] + assert data["requires_actions"] == ["FileInterfaceAction"] + # The body explains the bug, so it legitimately mentions "allowed-tools:". + # Assert on tokens that only ever appear inside the frontmatter block. + content = data["content"] + assert content.startswith("# Frontmatter Smoke") + assert "name: smoke_frontmatter" not in content + assert "spec: jv" not in content + assert "allowed_tools:\n" not in content + + def test_fixture_is_exposed_by_the_example_agent(self) -> None: + """A fixture no agent loads is not exercising discovery at all.""" + import yaml + + agent_yaml = self.FIXTURE.parents[1] / "agent.yaml" + data = yaml.safe_load(agent_yaml.read_text(encoding="utf-8")) + skills: list = [] + for entry in data.get("actions") or []: + if isinstance(entry, dict): + skills += (entry.get("context") or {}).get("skills") or [] + assert "smoke_frontmatter" in skills