Files
turnstone/tests
Patrick Buckley 9309162ac0 feat(skills): wire \$ARGUMENTS / \$N / \$<name> / \${CLAUDE_*} substitution (#572)
Implements the Anthropic Claude Code skill spec's placeholder
substitution end to end.  The renderer in ``_substitute_skill_args``
handles every spec form except ``\${CLAUDE_SKILL_DIR}`` (deferred):

* ``\$ARGUMENTS`` — full args string as the user/model typed it
* ``\$ARGUMENTS[N]`` / ``\$N`` — Nth positional arg, ``shlex.split``-parsed
* ``\$<name>`` — named arg from the SKILL.md ``arguments:`` list
* ``\${CLAUDE_SESSION_ID}`` / ``\${CLAUDE_EFFORT}`` — session state

Substitution is single-pass (one combined regex, one ``re.sub``).
Append rule: when args are passed but the body has no bare
``\$ARGUMENTS``, append ``ARGUMENTS: …`` at the end.

## Surface

* Parser: ``arguments:`` (list/space-delim) + ``argument-hint:`` (str)
  extracted into ``ParsedSkill``.
* Install: persists both to the pre-allocated columns from migration
  056 (PR #574).  Install path clamps ``argument_hint`` to 128 chars
  to match the admin-create cap (untrusted upstream source).
* Admin: ``CreateSkillRequest`` / ``UpdateSkillRequest`` accept both
  fields; create + edit modals get inputs; parse-preview echoes.
* Renderer: ``_substitute_skill_args`` runs AFTER ``_render_template``
  in ``_load_skills`` so user-supplied args containing ``{{var}}`` can't
  be re-expanded by the legacy renderer.
* Session: ``_skill_arguments`` plumbed through ``__init__``,
  ``set_skill``, and ``_save_config`` so a resumed workstream re-renders
  with the original arg payload.
* Model tool: ``skills(action='load')`` accepts an ``arguments`` string.
  Approval label includes a SHA-256 digest of the args so a once-
  approved skill name can't grant cover for a future payload; preview
  surfaces the args inline.

## ``/review`` findings (addressed)

* ``\${CLAUDE_EFFORT}`` referenced ``self._reasoning_effort`` — wrong
  attribute; the real one is ``self.reasoning_effort``.  Always rendered
  empty.  Fixed.
* Two-pass layering let user args containing ``{{var}}`` re-expand.
  Render order reversed.
* ``_skill_arguments`` wasn't in ``_save_config`` — resumed workstreams
  silently lost their payload.  Added.
* Approval label omitted ``arguments``.  Digest + preview added.
* Install path didn't bound ``argument_hint``.  Clamped.
* Added ``_skill_arg_names`` decode tests + "load same skill,
  different args → re-render" invariant test.

## Copilot review findings (addressed)

* ``skills.json`` tool description was inaccurate about ``shlex``
  stripping quotes and "empty string disables substitution".  Rewrote
  to match actual behaviour.
* Named-argument regex was stricter than parser/storage contract.
  ``arguments: [issue-number]`` would partial-match ``\$issue-number``
  as ``\$issue``, leaving ``-number`` as stray text.  Broadened the
  regex to ``[A-Za-z_][A-Za-z0-9_]*`` AND added validation at
  ``_skill_arg_names`` decode time so names not matching the regex
  are dropped with a warning.

## Tests

* ``tests/test_substitute_skill_args.py`` — placeholder forms,
  single-pass guarantee, append-at-end rule, shell-quoted input,
  unbalanced-quote fallback, uppercase + underscore-prefix names
* ``tests/test_skill_parser.py::TestArgumentsAndHint`` — parser
  extraction
* ``tests/test_skill_parse_api.py`` — HTTP parse-preview echoes
  both fields
* ``tests/test_skill_discovery_api.py::test_install_seeds_arguments_and_argument_hint``
  — install round-trip
* ``tests/test_skills_tool.py::test_load_forwards_arguments_to_set_skill``
  + ``test_load_same_skill_different_args_triggers_resub`` —
  wire path through prepare → exec → set_skill
* ``tests/test_skills_tool.py::TestSkillArgNames`` — storage decode
  helper including the hyphen/dot/leading-digit filter
2026-05-23 17:48:16 -07:00
..
2026-04-03 15:36:20 -07:00