Repository navigation
fix(validate): count characters, accept every major version, and keep citations inside the workspace - #2
Conversation
… citations inside the workspace Three defects found in review, each with a regression test that fails without the corresponding fix. stack: majorVersionRe was `^v[2-9]\d*$`, which rejects v10 through v19 — the leading digit class starts at 2, so only the first digit was ever allowed to be 1-free. The doc comment directly above it claimed v11 as an example, so the code contradicted its own documentation. A module ending /v11 kept the suffix as its base name and was reported undocumented against a stack.md that documented it. skill: the spec's limits are quoted in characters and were compared against len(), which is bytes. Every non-ASCII name, description, or compatibility field was held to a shorter budget than the standard grants — a false positive on a valid skill, which the existing ASCII test could not see. Counted in runes now, via one helper so the three sites cannot drift apart. codewiki: a citation target went into filepath.Join unchecked, so ../.. was cleaned into a real path outside root and then opened. The finding a traversal produced was nothing at all — the outside file resolved, its lines counted in range, and the validator stayed silent — so this was invisible rather than noisy. Same containment rule the workspace already applies to argv, at the one place a path arrives from a document instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XH8WYThtTSGJxS7tXRvfp
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe validator updates Codewiki citation containment, counts skill field limits by Unicode characters, and broadens Go module major-version suffix recognition. Tests cover traversal, absolute and symlink paths, Unicode boundaries, and module-name normalization. ChangesCodewiki citation path security
Skill character limits
Module major-version parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/validate/skill_test.go (1)
297-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Unicode boundary coverage for
name.The production change in
internal/validate/skill.goLines 129-131 is not exercised by these additions. Add fixtures at 64 and 65 runes, assertingskill.name-too-longis absent/present respectively; the non-ASCII fixture may also correctly produceskill.name-invalid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/validate/skill_test.go` around lines 297 - 319, Extend TestLengthLimitsCountCharactersNotBytes with name-length fixtures containing exactly 64 runes and then 65 runes, asserting skill.name-too-long is absent at the limit and present above it. Use non-ASCII content to exercise rune counting, and allow the over-limit case to also report skill.name-invalid.internal/validate/codewiki.go (1)
137-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPossible duplication with
workspace.SafeName.The comment above
checkCitationstates this is "Same rule as workspace.SafeName, at the one place a path arrives from a document rather than from argv." IfSafeNameimplements equivalent absolute/traversal rejection, consider extracting a shared helper (e.g. in an internal path-safety package) so the two containment rules can't drift apart over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/validate/codewiki.go` around lines 137 - 152, The path-safety logic in resolveInRoot duplicates workspace.SafeName’s absolute-path and traversal checks. Extract the shared containment validation into a common internal path-safety helper, update both resolveInRoot and SafeName to use it, and preserve their existing return behavior and input-specific handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/validate/codewiki.go`:
- Around line 137-152: Update resolveInRoot and the isFile path to prevent
citations from traversing symlinks: validate the resolved target and its parent
components with symlink-aware filesystem checks, or reject any path containing
symlinks before os.Stat follows them. Ensure targets such as links escaping root
are rejected rather than treated as workspace files, while preserving valid
non-symlink paths.
---
Nitpick comments:
In `@internal/validate/codewiki.go`:
- Around line 137-152: The path-safety logic in resolveInRoot duplicates
workspace.SafeName’s absolute-path and traversal checks. Extract the shared
containment validation into a common internal path-safety helper, update both
resolveInRoot and SafeName to use it, and preserve their existing return
behavior and input-specific handling.
In `@internal/validate/skill_test.go`:
- Around line 297-319: Extend TestLengthLimitsCountCharactersNotBytes with
name-length fixtures containing exactly 64 runes and then 65 runes, asserting
skill.name-too-long is absent at the limit and present above it. Use non-ASCII
content to exercise rune counting, and allow the over-limit case to also report
skill.name-invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60b3e5e2-6067-425f-ac29-b123ce8e9da6
📒 Files selected for processing (5)
internal/validate/codewiki.gointernal/validate/knowledge_test.gointernal/validate/skill.gointernal/validate/skill_test.gointernal/validate/stack.go
The lexical containment check landed in the previous commit is necessary and not sufficient. A symlink that sits inside the workspace and points out of it never spells "..", so filepath.Clean has nothing to collapse and the check passes it through — then isFile and os.ReadFile follow the link and the kernel performs the escape the path itself never described. It failed the same silent way the original traversal did: the linked file resolved, its lines counted in range, and the validator reported nothing at all. A regression test asserts both halves, that citation-invalid fires and that citation-out-of-range does not, since the latter would prove the read happened. The root is resolved alongside the target rather than compared raw. A workspace legitimately lives under a symlink — macOS puts every t.TempDir() behind /private/var — and matching a fully resolved target against an unresolved root would call every citation in such a workspace an escape. The second test pins that direction so the containment check cannot be tightened into a false positive on a real workspace. A target that does not resolve is left to the existing unresolved finding: nothing was followed, so nothing escaped, and naming a typo an escape would be the less useful of the two answers. Found in review of #2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XH8WYThtTSGJxS7tXRvfp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/validate/codewiki.go`:
- Around line 164-174: The symlink validation in the path-resolution flow is
vulnerable to a check-to-use race before checkCitation reads the file. Update
the interaction between the visible EvalSymlinks/within logic and checkCitation
so file validation and reading use a root-confined, no-follow handle-based path,
preserving rejection of resolved paths outside realRoot; otherwise enforce an
immutable-workspace precondition.
In `@internal/validate/knowledge_test.go`:
- Around line 467-482: Update the citation in the symlink validation test around
runValidator to request lines 1-3 instead of 1-2, while keeping the existing
assertions unchanged so a symlink-read regression produces
codewiki.citation-out-of-range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ac33d16-4126-44ba-ae26-7cf710a604b8
📒 Files selected for processing (2)
internal/validate/codewiki.gointernal/validate/knowledge_test.go
| realResolved, err := filepath.EvalSymlinks(resolved) | ||
| if err != nil { | ||
| // The target does not exist, so nothing was followed and nothing escaped. The | ||
| // lexical result stands and the caller reports it as unresolved, which is the | ||
| // more useful finding than calling a typo an escape. | ||
| return resolved, true | ||
| } | ||
| if !within(realRoot, realResolved) { | ||
| return "", false | ||
| } | ||
| return resolved, true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid the symlink check-to-use race.
Line 169 returns the lexical path; checkCitation subsequently calls isFile and os.ReadFile on it. A process able to modify the checkout can swap a checked-safe component for an escaping symlink after EvalSymlinks, causing an out-of-workspace read. Use a root-confined, no-follow handle-based read path (or explicitly require an immutable workspace).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/validate/codewiki.go` around lines 164 - 174, The symlink validation
in the path-resolution flow is vulnerable to a check-to-use race before
checkCitation reads the file. Update the interaction between the visible
EvalSymlinks/within logic and checkCitation so file validation and reading use a
root-confined, no-follow handle-based path, preserving rejection of resolved
paths outside realRoot; otherwise enforce an immutable-workspace precondition.
Both escape tests asserted that codewiki.citation-out-of-range did not fire, as the evidence that the outside file was never opened. The fixture has two lines and the citation asked for 1-2, which is inside it — so that finding would have stayed silent whether the read happened or not, and the assertion proved nothing. The commits that added them claimed the tests pinned both halves; only the citation-invalid half was actually pinned. Citing 1-3 puts the range past the end of the fixture, so a regression that follows the traversal or the symlink now reports out-of-range and the assertion fails. Confirmed in both directions: against the pre-fix code the traversal case reports [codewiki.citation-out-of-range] where it previously reported [], which is positive evidence of the read rather than the absence of a finding. Found in review of #2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013XH8WYThtTSGJxS7tXRvfp
1 participant
Three defects found in the review of #1. That PR merged before this commit landed on the branch, so these fixes never reached
main— this carries them over unchanged.Each fix has a regression test that was verified to fail against the pre-fix code, not merely to pass after it.
internal/validate/stack.go— the v10–v19 blind spotmajorVersionRewas^v[2-9]\d*$. The leading-digit class starts at 2, so only the first digit was ever allowed to be non-1 —v10throughv19were rejected outright, whilev20passed by coincidence. The doc comment directly above it offeredv11as an example, so the code contradicted its own documentation.Effect: a module ending
/v11kept the suffix as its base name and was reportedstack.undocumented-dependencyagainst adocs/stack.mdthat documented it correctly. That is the false-positive class the function's own comment says it exists to prevent.Verified against the old code:
internal/validate/skill.go— characters vs. bytesThe Agent Skills spec quotes its limits in characters; all three sites (
name,description,compatibility) compared againstlen(), which is bytes. Every non-ASCII field was held to a shorter budget than the standard grants.A
descriptionof exactly 1024 accented characters was reportedskill.description-too-long— a false positive on a valid skill, and the existing test could not see it because it built its input fromstrings.Repeat("a", …). Counted in runes now, through a singlecharLenhelper so the three sites cannot drift apart again.internal/validate/codewiki.go— citations could escape the checkoutA citation target went into
filepath.Joinunchecked, so../..was cleaned into a real path outsiderootand then opened withos.ReadFile.What made this worth fixing over the cosmetic findings: a traversal produced no finding at all. The outside file resolved, its line count came back in range, and the validator stayed silent — so the behavior was invisible rather than noisy. The regression test asserts both halves: that
codewiki.citation-invalidfires, and thatcodewiki.citation-out-of-rangedoes not, since the latter would prove the read happened anyway.Against the pre-fix code the test reported
rules = []for both../outside.txtanddocs/../../outside.txt.This applies the containment rule the workspace already enforces on
argv(per theSafeNameconvention in CLAUDE.md) at the one place a path arrives from a document instead. Reusescodewiki.citation-invalidrather than adding a rule name, since a citation that leaves the checkout is invalid.Verification
make checkgreen — gofmt,go vet,go test -race.internal/validatecoverage 85.0% → 85.3%.TestFreshArtifactsPassTheirOwnValidatorsstill passes, which is the gate that matters most: a validator firing on scc's own scaffolded output is the worst bug in the product.Not addressed here
Two findings from the same review are real but are contract changes rather than point fixes, and are left for a separate decision:
AtomicWriteoverstates its guarantee —os.Renameis not atomic on Windows, which CLAUDE.md treats as a first-class target. Either narrow the documented contract or add a Windows-specific replace.SafeWrite's no-clobber path is aStat/Renamesequence — a file created between the two can still be replaced.🤖 Generated with Claude Code
https://claude.ai/code/session_013XH8WYThtTSGJxS7tXRvfp
Summary by CodeRabbit
.., absolute paths, symlinks, and symlinked workspaces.