fix(archdocs): three bugs in pssg template helper functions#92
Merged
greynewell merged 1 commit intomainfrom Apr 9, 2026
Merged
fix(archdocs): three bugs in pssg template helper functions#92greynewell merged 1 commit intomainfrom
greynewell merged 1 commit intomainfrom
Conversation
1. durationMinutes ignored the seconds capture group — PT90S returned 0 instead of 1; fix reads matches[3] and adds seconds/60. 2. sliceHelper panicked when start exceeded the slice length after end was clamped, e.g. slice(s, 5, 10) on a 3-element slice; fix adds a clampSliceBounds helper that also guards start > end. 3. sortStrings copied the slice but never called sort.Strings, so the template `sort` function always returned an unsorted copy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughThis PR improves helper functions in the render package by fixing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/archdocs/pssg/render/funcs.go`:
- Around line 243-253: clampSliceBounds can return negative indices if end is
negative because the code sets start = end after only clamping end against
length; update clampSliceBounds to first clamp both start and end into the [0,
length] range (e.g., if start < 0 then start = 0; if start > length then start =
length; same for end) and only then enforce ordering (if start > end then start
= end); apply the same fix to the other similar function(s) in this file that
mirror clampSliceBounds.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83f9779c-9c20-4a1e-97c3-f5ac7412c69f
📒 Files selected for processing (2)
internal/archdocs/pssg/render/funcs.gointernal/archdocs/pssg/render/funcs_test.go
Comment on lines
+243
to
+253
| func clampSliceBounds(start, end, length int) (int, int) { | ||
| if start < 0 { | ||
| start = 0 | ||
| } | ||
| if end > length { | ||
| end = length | ||
| } | ||
| if start > end { | ||
| start = end | ||
| } | ||
| return start, end |
There was a problem hiding this comment.
clampSliceBounds can still return negative indices and panic.
On Line 250, start = end can make start negative when end < 0 (e.g., slice(s, -1, -1)), which will panic at slice time.
Please clamp both bounds to [0, length] before ordering.
🐛 Suggested fix
func clampSliceBounds(start, end, length int) (int, int) {
if start < 0 {
start = 0
}
+ if start > length {
+ start = length
+ }
+ if end < 0 {
+ end = 0
+ }
if end > length {
end = length
}
if start > end {
start = end
}
return start, end
}Also applies to: 259-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/archdocs/pssg/render/funcs.go` around lines 243 - 253,
clampSliceBounds can return negative indices if end is negative because the code
sets start = end after only clamping end against length; update clampSliceBounds
to first clamp both start and end into the [0, length] range (e.g., if start < 0
then start = 0; if start > length then start = length; same for end) and only
then enforce ordering (if start > end then start = end); apply the same fix to
the other similar function(s) in this file that mirror clampSliceBounds.
4 tasks
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
Three correctness bugs in
internal/archdocs/pssg/render/funcs.go:durationMinutesignored seconds — the regex captures(\d+)Sasmatches[3]but it was never read.PT90Sreturned 0 instead of 1;PT2H30M45Sreturned 150 instead of the correct truncated 150. Fix: readmatches[3]and addseconds/60.sliceHelperpanicked on out-of-bounds start —endwas clamped tolen(v)butstartwas not, soslice(s, 5, 10)on a 3-element slice producedv[5:3]which panics. Alsoslice(s, 3, 1)(start > end) would panic. Fix: addclampSliceBoundshelper that ensures0 ≤ start ≤ end ≤ len(v).sortStringsdidn't sort — the function registered as thesorttemplate helper copied the input slice but never calledsort.Strings. Any template using{{ sort list }}got an unsorted result. Fix: addsort.Strings(result)after the copy.Test plan
TestDurationMinutes,TestSliceHelper,TestSortStringsgo test ./internal/archdocs/pssg/render/passesgo build ./...passes🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests