Repository navigation
fix(#14): address advisory review findings in forge-friction SKILL.md - #15
Merged
Merged
Conversation
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.
Closes #14
Addresses all three advisory findings from the code review of PR #13.
Changes
F-1 ·
SKILL.md:35— "no themes" sentinel clarifiedChanged "If no themes pass the filter in either table" to "If neither table has any rows" (unambiguous conjunction). Added guidance to emit a
(none)placeholder row when only one table is empty, so a populated table is never suppressed.F-2 ·
SKILL.md:93— interactive mode column format anchoredAdded parenthetical
(same \Category | Count | Recommendation` columns as dry-run)` to the interactive operational-themes step, matching the explicit column spec already present in dry-run output (lines 25–27).F-3 ·
SKILL.md:53-56— Step 1 consolidation notedAdded an inline HTML comment after the Step 1 filter bullets documenting that the two removed bullets ("A skill could change the agent's default behavior" and "The fix is a workflow shape change") were consolidated into Step 2 Classify, where they fit as shape-gap classification criteria rather than pre-filters.
Verified on HEAD
Base SHA:
822410b4ac9e160c3939e82a67e29c67bb3141ccEach finding was confirmed present on HEAD before the fix:
SKILL.md:35— text read"If no themes pass the filter in either table"(ambiguous disjunction)SKILL.md:93— text read"list each with its one-line recommendation"with no column format referenceSKILL.md:53-56— Step 1 had 3 bullets with no comment about the removed criteriaNo tests exist for SKILL.md spec files. No lint command is configured. Change is spec-text only.