[bug-fix] Fix preset-wrap-drops-argument-hint: inherit argument-hint from core template - #3996
Merged
mnriem merged 2 commits intoAug 10, 2026
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
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). |
mnriem
deleted the
fix/3991-preset-wrap-drops-argument-hint-6fc1afa14c852556
branch
August 10, 2026 15:19
mnriem
pushed a commit
that referenced
this pull request
Aug 11, 2026
…tions (#4045) * fix(claude): make argument-hint injection fold-aware for long descriptions ClaudeIntegration.inject_argument_hint spliced argument-hint: "..." as a raw text line right after the first line starting with "description:". When a description is long enough for the YAML dumper to fold it across indented continuation lines, that splice landed inside the scalar, producing invalid YAML (plain scalar) or silently absorbing the hint into the description string (quoted scalar). This reproduces #3991 for the case #3996 didn't cover: bundled core commands have no argument-hint in their source frontmatter, so the structural apply_argument_hint path is a no-op and this raw-text fallback is what actually runs. Skip every continuation line of the description scalar (anything more indented than the key itself) before inserting, so the new key always lands after the whole scalar ends rather than in the middle of it. Fixes #4044 * fix(claude): also skip unindented blank lines in description scalar PyYAML serializes an embedded paragraph break ("\n\n") inside a quoted description as unindented blank lines, not indented continuation lines. inject_argument_hint only skipped indented lines, so it still inserted argument-hint mid-scalar for multi-paragraph descriptions, reproducing the #4044 failure modes. Skip blank lines too, and add a regression test for the multi-paragraph case.
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