Files
turnstone/tests/test_substitute_skill_args.py
Patrick Buckley 06e16de066 chore(skills): drop Anthropic attribution from SKILL.md spec references
Two related cleanups landed together because they touch the same surface
(skill-spec uplift PRs #569/#570/#571/#572):

  1. Wording: replace "Anthropic spec" / "Anthropic Claude Code skill spec"
     with "SKILL.md spec" across admin UI tooltips, code comments, test
     docstrings, migration 056's module docstring, and the user-facing
     `arguments` description in tools/skills.json.  Renames a parser test
     `test_anthropic_tags` -> `test_nested_metadata_tags` and consolidates
     a parse-API test of the same shape; fixture author renamed
     `Anthropic` -> `Acme` to keep the fixture neutral.  Legitimate
     provider/SDK/API references (provider name, api.anthropic.com,
     `_anthropic.py`, capability comments) are intentionally untouched.

  2. Admin UX: in the Create + Edit Skill modals, six fields per modal
     (Compatibility, Paths, Hide-from-skill-picker, Arguments, Argument
     hint, Activation) had long uppercase label-hint spans crammed into
     the visible label.  Migrated each to the existing
     `.settings-help-btn` + `.settings-help-popover` pattern already used
     in the Settings tab — short label + inline `?` button that opens a
     styled popover with proper `<code>` formatting for technical tokens.

     Pattern reuse required two small generalisations in admin.js:

       * `_toggleSettingsHelp` now looks up the popover via a new
         `data-help-target="<id>"` attribute first, falling back to the
         settings-tab `.settings-label-col` ancestor lookup.
       * `_closeAllSettingsHelp` mirrors the same dual-path lookup when
         resetting `aria-expanded`, so modal buttons don't get stuck on
         `aria-expanded="true"` after another popover opens.
       * Added a document-delegated click handler that fires only for
         buttons with `data-help-target`; existing per-button binding
         in the settings-tab render path is unchanged.

     CSS: `.settings-help-btn` now paints its `?` via `::after` with the
     button's own `font-size: 0`, so prettier-introduced whitespace
     inside the new HTML buttons can't off-center the glyph.  The same
     rule applies to existing admin.js-generated buttons (text content
     hidden, pseudo identical).  Small additions for
     `.settings-help-popover code` / `strong` styling so technical
     tokens render with the same monospace pill treatment used elsewhere
     in skill UI.

Known follow-ups (intentionally NOT in this PR):
  * Migrate the settings-tab `_renderSettingRow` button assembly to the
    empty-`<button>` + `data-help-target` form so the per-button
    addEventListener loop can be dropped in favour of pure document
    delegation, and the `font-size: 0` rule stops being a workaround for
    two markup styles.
  * The 12 new popover blocks are duplicated verbatim between the
    Create and Edit modals (same as the rest of the create/edit modal
    pair).  A small renderer that emits popovers from a shared data
    object would eliminate the drift risk but is unrelated cleanup.
2026-05-24 14:56:43 -07:00

180 lines
7.9 KiB
Python

"""Unit tests for ``_substitute_skill_args`` — SKILL.md spec placeholder
substitution applied to skill bodies at load time.
Covers every placeholder form Turnstone implements (``${CLAUDE_SKILL_DIR}``
is deferred — see #572) plus the spec's "append ARGUMENTS at end if no
placeholder" rule and the single-pass guarantee against re-expansion of
user-supplied values that happen to contain placeholder syntax.
"""
from __future__ import annotations
from turnstone.core.session import _substitute_skill_args
def _sub(content: str, *, args: str = "", names: list[str] | None = None) -> str:
"""Compact test helper — defaults env values to fixed sentinels."""
return _substitute_skill_args(
content,
arguments_str=args,
arg_names=names or [],
ws_id="ws-abc",
effort="high",
)
class TestArgumentsLiteral:
def test_full_args_expands(self) -> None:
assert _sub("Run $ARGUMENTS now", args="alpha bravo") == "Run alpha bravo now"
def test_empty_args_no_placeholder_unchanged(self) -> None:
assert _sub("Hello world", args="") == "Hello world"
def test_empty_args_with_placeholder_substitutes_empty(self) -> None:
# $ARGUMENTS with no args present → empty string (placeholder cleared).
assert _sub("Prefix $ARGUMENTS suffix", args="") == "Prefix suffix"
def test_append_when_args_present_but_no_placeholder(self) -> None:
"""Spec: args passed + body has no $ARGUMENTS → append at end."""
out = _sub("Skill body without placeholder.", args="x y")
assert out.endswith("\n\nARGUMENTS: x y")
assert out.startswith("Skill body without placeholder.")
def test_no_append_when_args_present_and_placeholder_used(self) -> None:
out = _sub("Run $ARGUMENTS.", args="x y")
assert out == "Run x y."
# Critical: no trailing append, no double-rendering.
assert "ARGUMENTS:" not in out.removeprefix("Run ")
def test_indexed_form_does_not_count_as_literal(self) -> None:
"""``$ARGUMENTS[0]`` is a different placeholder; if it's the only
form in the body and args were passed, the append-at-end rule
still fires because the BARE ``$ARGUMENTS`` literal is absent."""
out = _sub("First: $ARGUMENTS[0]", args="a b")
assert "First: a" in out
assert out.endswith("\n\nARGUMENTS: a b")
class TestPositional:
"""Positional substitution. Bodies in this group don't use the bare
``$ARGUMENTS`` placeholder, so the spec's "append at end" rule fires —
tests assert ``startswith`` on the substituted prefix rather than full
equality to keep the focus on the substitution itself."""
def test_short_form(self) -> None:
assert _sub("$0 then $1", args="alpha bravo").startswith("alpha then bravo")
def test_bracketed_form(self) -> None:
out = _sub("$ARGUMENTS[0] then $ARGUMENTS[1]", args="alpha bravo")
assert out.startswith("alpha then bravo")
def test_shell_quoted_input(self) -> None:
"""Spec: ``"hello world" second`` parses via shlex so $0='hello world'."""
out = _sub("$0 / $1", args='"hello world" second')
assert out.startswith("hello world / second")
def test_out_of_range_substitutes_empty(self) -> None:
out = _sub("$0 $5", args="only-one")
assert out.startswith("only-one ") # second placeholder → empty
def test_unbalanced_quotes_falls_back_to_whitespace_split(self) -> None:
"""A typo (unmatched quote) shouldn't blow up the substitution —
fall back to whitespace split so the prompt still renders. The
fallback split on whitespace gives ``['alpha', '"bravo']``."""
out = _sub("$0 $1", args='alpha "bravo')
assert out.startswith('alpha "bravo')
class TestNamedArguments:
def test_named_arg_substitutes_by_position(self) -> None:
out = _sub("issue $issue branch $branch", args="123 main", names=["issue", "branch"])
assert out.startswith("issue 123 branch main")
def test_unknown_name_left_as_literal(self) -> None:
"""``$foo`` with ``foo`` not in arg_names stays as ``$foo`` —
forgiving behaviour matches ``_render_template``."""
out = _sub("$known $unknown", args="x y", names=["known"])
assert out.startswith("x $unknown")
def test_known_name_with_missing_positional_substitutes_empty(self) -> None:
"""Named arg whose position is past the end of supplied args → ``""``.
No args passed so no append-at-end either."""
assert _sub("got $name", args="", names=["name"]) == "got "
def test_arguments_uppercase_not_matched_as_named(self) -> None:
"""``$ARGUMENTS`` must not be matched by the named-arg regex —
the bare ``$ARGUMENTS`` alternative in the combined regex sits
earlier in the precedence chain. Pin so a future regex tweak
can't break this."""
# No args, no names → bare $ARGUMENTS substitutes to empty
# via the literal branch, not via the named-arg branch.
assert _sub("$ARGUMENTS", args="", names=[]) == ""
def test_uppercase_name_substitutes(self) -> None:
"""The broadened named-arg regex accepts uppercase identifiers.
Pin so a SKILL.md author who declares ``arguments: [USER_ID]``
and references ``$USER_ID`` gets the substitution, not a
literal."""
out = _sub("user $USER_ID", args="alice", names=["USER_ID"])
assert out.startswith("user alice")
def test_underscore_prefix_name_substitutes(self) -> None:
"""Identifier names starting with ``_`` are valid Python
identifiers; the broadened regex matches them."""
out = _sub("got $_internal", args="value", names=["_internal"])
assert out.startswith("got value")
class TestEnvironment:
def test_session_id_substitutes(self) -> None:
assert _sub("session ${CLAUDE_SESSION_ID}") == "session ws-abc"
def test_effort_substitutes(self) -> None:
assert _sub("effort ${CLAUDE_EFFORT}") == "effort high"
def test_unknown_env_left_as_literal(self) -> None:
assert _sub("${CLAUDE_UNKNOWN_FOO}") == "${CLAUDE_UNKNOWN_FOO}"
class TestSinglePassGuarantee:
"""A placeholder VALUE containing another placeholder must not be
re-expanded — matches spec's "Substitution runs once" rule."""
def test_arg_value_containing_placeholder_not_reexpanded(self) -> None:
# $0 value is the literal string "$1"; the rendered body should
# contain "$1" verbatim, not the substituted value of $1. Append
# rule fires because the body has no bare ``$ARGUMENTS`` literal —
# split the output to isolate the body from the appended echo.
out = _sub("$0", args='"$1" actual')
body, _, _appended = out.partition("\n\nARGUMENTS: ")
# Body contains "$1" once — substituted in from $0 → "$1",
# NOT re-expanded to "actual".
assert body == "$1"
def test_arg_value_containing_dollar_arguments_not_reexpanded(self) -> None:
# $0 = "$ARGUMENTS" — would loop without single-pass.
out = _sub("got $0", args='"$ARGUMENTS"')
assert out.startswith("got $ARGUMENTS")
# The "$ARGUMENTS" inside the value MUST NOT be re-substituted
# into the args string. Append-at-end rule adds a trailing
# "ARGUMENTS: $ARGUMENTS" line — that's an as-typed echo, not a
# re-substitution.
assert "got $ARGUMENTS\n\nARGUMENTS:" in out
class TestIntegration:
def test_all_forms_in_one_body(self) -> None:
body = (
"Session ${CLAUDE_SESSION_ID} at effort ${CLAUDE_EFFORT}.\n"
"First $0, second $1.\n"
"Named: $issue resolved on $branch.\n"
"Full: $ARGUMENTS"
)
out = _sub(body, args="123 main", names=["issue", "branch"])
assert out == (
"Session ws-abc at effort high.\n"
"First 123, second main.\n"
"Named: 123 resolved on main.\n"
"Full: 123 main"
)