fix: address mid-severity follow-ups from PR #1570 and PR #1585#1589
Merged
Conversation
Refs: #1571, #1587 Issue #1571 (--input-root follow-ups): - import.ts: clarify warning suppression scope in comment - add inputRoot threading assertions to rules/subagents/skills/hooks/permissions/ignore processor tests - validateBaseDir: make '..' segment check platform-aware Issue #1587 (permissions follow-ups): - cline/augmentcode/qwencode: implement real validate() via safeParse - gitignore-entries: drop **/kilo.jsonc entry for parity with opencode.jsonc - augmentcode: warn on non-roundtrippable shellInputRegex; deny falls back to '*' (fail-closed) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Fix grammatical typo in import.ts comment: 'a inputRoot' -> 'an inputRoot'. - Replace redundant assertion in rules-processor inputRoot test that re-read the just-written file; assert on the loaded RulesyncRule's parsed frontmatter and body instead, which actually verifies the processor consumed the inputRoot. - Add warn-message content assertions to the new non-roundtrippable shellInputRegex import tests in augmentcode-permissions.test.ts: spy on ConsoleLogger.prototype.warn so the asymmetric fallback semantics (deny -> '*' fail-closed; allow/ask -> lossy with warning) are observable from tests, not just the resulting glob. The pre-existing constructor issue flagged by the previous review (the constructor not invoking validate() when params.validate is true) is intentionally NOT addressed: it predates this PR (the previous validate() returned a no-op success, so wiring it up is a behavioral change), would require touching three modules outside the converged scope of issues #1571 and #1587, and is better tracked as its own follow-up issue.
…structors Address review findings on PR #1589: - #1 (mid): Wire `validate()` into the constructors of AugmentcodePermissions, ClinePermissions, and QwencodePermissions so that `fromFile({ validate: true })` actually rejects malformed input. Previously the validate() method existed but was never invoked at construction time, so callers reading `validate: true` falsely assumed validation had run. Mirrors the RulesyncPermissions pattern. - #3 (low): Drop PR-internal label `(Finding F)` from the describe title in augmentcode-permissions.test.ts since it is meaningless after merge. - #4 (low): Add a co-located inner `afterEach` that restores the warnSpy on ConsoleLogger.prototype, instead of relying on the outer describe's vi.restoreAllMocks(). Keeps the cleanup next to the spy so future refactors cannot silently leak the prototype-level spy across other test files. Adds three constructor-level rejection tests per class (malformed JSON, schema violation, and a validate: false escape hatch) so future regressions are caught.
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
1 participant
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.
Summary
Addresses mid-severity findings from two follow-up issues, plus low-severity quality improvements from a follow-up review.
Closes #1571
Closes #1587
Issue #1571 —
--input-rootfollow-ups (PR #1570)src/cli/commands/import.ts— clarify the warning-suppression comment so it accurately reflects that only direct CLI/programmatic callers are protected; users with aninputRootset inrulesync.jsoncmay still see the actionable warning.inputRootthreading assertions to the per-feature processor unit tests for rules, subagents, skills, hooks, permissions, and ignore (modeled aftercommands-processor.test.ts). A regression that swapsinputRootforprocess.cwd()is now caught at the unit level.src/utils/file.ts— makevalidateBaseDir's..-segment split platform-aware (/[/\\\\]/on Windows,/\\//on POSIX) so POSIX filenames containing literal backslashes round-trip cleanly.Issue #1587 — permissions follow-ups (PR #1585)
validate()for cline/augmentcode/qwencode permissions modules viaJSON.parse+safeParse(mirrors Kilo's pattern). The previous no-op meantfromFile({ validate: true })did not actually verify schema conformance.**/kilo.jsoncgitignore entry — parity with the structurally-identicalopencode.jsonc(no entry) since the translator preserves non-permissions Kilo settings on round-trip.shellInputRegeximport: emitlogger.warnwhen the regex is non-roundtrippable. Fordenyonly, fall back to*(fail-closed). Forallow/ask, warn but keep the lossy conversion to avoid weakening security via broadened allow patterns.Quality improvements (commit 655dd25)
import.tswarning-suppression comment (a inputRoot→an inputRoot).readFileContentre-read inrules-processor.test.tswith assertions on the loadedRulesyncRuleinstance, so the inputRoot test verifies the processor actually parsed content frominputRoot.augmentcode-permissions.test.tsto assert onlogger.warnmessage contents (regex, tool name, fail-closed wording fordeny, "Importing as glob" wording forallow/ask).Test plan
pnpm cicheckpasses (216 test files / 5480 tests)🤖 Generated with Claude Code