[bug-fix] Fix preset-wrap-drops-argument-hint: inherit argument-hint from core template - #3996
Open
github-actions[bot] wants to merge 2 commits into
Open
Conversation
Apply the remediation from the bug assessment on issue #3991. Extend the inheritance allowlist in _register_skills and _compose_layers to include 'argument-hint', so wrap-strategy presets that omit this key will inherit it from the core template rather than silently dropping it and risking its value being leaked into description. Refs #3991 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes wrap-strategy presets so argument-hint is inherited from core command metadata.
Changes:
- Adds
argument-hintto both composition allowlists. - Adds a regression test, though coverage gaps remain.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/presets/__init__.py |
Inherits argument-hint during command and skill composition. |
tests/test_presets.py |
Tests core argument-hint inheritance. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
| previously inherited only scripts/agent_scripts from core_frontmatter, | ||
| silently discarding argument-hint and leaking its value into description. | ||
| """ | ||
| core_arg_hint = "Describe the feature you want to specify" |
Comment on lines
+4730
to
+4731
| def test_wrap_preset_inherits_argument_hint_from_core(self, project_dir, temp_dir): | ||
| """A wrap-strategy preset that omits argument-hint must inherit it from the core template. |
| body, core_frontmatter = _substitute_core_template(body, cmd_name, self.project_root, registrar) | ||
| frontmatter = dict(frontmatter) | ||
| for key in ("scripts", "agent_scripts"): | ||
| for key in ("scripts", "agent_scripts", "argument-hint"): |
The existing regression test for #3991 wraps `speckit.specify`, whose stem is in Claude's ARGUMENT_HINTS map. The string-injection fallback in post_process_skill_content re-adds argument-hint even when wrap composition drops it, so that test passes with or without the inheritance fix and does not actually guard the regression. Add a parallel test that wraps an extension-like command (`speckit.myfeature`) absent from ARGUMENT_HINTS, so the wrap-composition inheritance is the only path that can carry argument-hint into the SKILL.md. This test fails without the fix and passes with it. Refs #3991 Assisted-by: GitHub Copilot (model: claude-opus-4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 970babe2-48cd-4c41-adae-0282d879a9ce
Collaborator
|
Ran the bug-test stage locally (the CI Findings:
Change pushed (581c3ac): added Posted on behalf of @mnriem by GitHub Copilot (model: claude-opus-4.8, 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.
Bug fix — preset-wrap-drops-argument-hint
Proposed fix for issue #3991, applying the remediation from the bug assessment.
Verdict: Valid · Severity: medium
Summary
Extended the inheritance allowlist in
_register_skillsand_compose_layersto includeargument-hint, so wrap-strategy presets that omit this key now inherit it from the core template instead of silently dropping it (and risking its value being leaked/concatenated intodescription).Changes
src/specify_cli/presets/__init__.py"argument-hint"to two inheritance allowlists (lines ~2746 and ~5728)tests/test_presets.pytest_wrap_preset_inherits_argument_hint_from_core— pins the regressionTests Added or Updated
tests/test_presets.py::TestSkillsPreset::test_wrap_preset_inherits_argument_hint_from_core— installs a wrap preset with noargument-hintin its frontmatter against a core template that declares it, then asserts (a)argument-hintis present in the generated SKILL.md with the correct value, and (b)descriptionis exactly the preset's declared string (not a concatenation including the hint value).Local Verification
"argument-hint"in the key set, which feeds the existingapply_argument_hintdict-based path at line 2781 — the same path PR fix(presets): preserve argument-hint in preset SKILL.md generation #2978 proved correct for the explicit-declaration case.Deviations from Assessment
None. The fix matches the preferred remediation exactly: one-line addition of
"argument-hint"to the allowlist tuple in each of the two locations named by the assessment.Risks & Review Notes
argument-hintto the inheritance allowlist could silently inherit a core value that a preset author intended to omit. Risk is low:argument-hintis a UX hint for Claude's slash-command input only, not a functional directive._compose_layerschange affects the composed-file path used by non-skills agents; the existing integration tests for those agents should be run during CI.argument-hintto a SKILL.md is additive.Refs #3991 · cc
@takaya0