Wave-42: bound commit-signing-check header scan (C04 L34) - #340
Conversation
Replace unbounded git cat-file buffers and multiline regex with a line-scanner header reader (MaxCommitObjectBytes cap), -SelfCheck fixtures, and tests/commit_signing_check.rs cargo wrapper. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 51 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe commit-signing checker now scans bounded commit headers, classifies signature formats without multiline object regexes, exposes deterministic self-check fixtures, and integrates self-check execution into Rust tests, documentation, changelog, and GitHub Actions. ChangesCommit-signing verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RustTest
participant PowerShell
participant SelfCheck
participant Repository
RustTest->>PowerShell: Invoke script with -SelfCheck
PowerShell->>SelfCheck: Validate anchors and scanner configuration
SelfCheck->>Repository: Read workflow, docs, and script
SelfCheck->>SelfCheck: Run signature classification fixtures
SelfCheck-->>RustTest: Return success output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/commit-signing-check.ps1`:
- Around line 121-123: Update the git cat-file result handling around the
process exit check so any nonzero $proc.ExitCode returns $null or otherwise
surfaces the failure regardless of $lines.Count. Preserve the existing
successful path for zero exit codes, preventing partial output from being
classified as a valid header.
- Around line 93-112: Update the header-reading loop around the line
classification in the commit-signing check so only a truly zero-length string
terminates header parsing; do not treat whitespace-only continuation lines as
the header/body separator. Preserve the existing byte and line caps and continue
capturing the complete bounded gpgsig block for downstream processing.
- Around line 69-79: Update the object-size check in the commit-object
validation flow around `$sizeRaw` and `$objectSize`: either remove the
`MaxCommitObjectBytes`/`$ObjectByteCap` rejection so large commit messages are
governed only by the streaming byte and line caps, or explicitly document this
intentional 1 MB limit and its effect on valid signed commits. Preserve the
existing `git cat-file -s` validation and malformed-output handling.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6320b679-5cac-4e35-8204-e8d814e2e37c
📒 Files selected for processing (5)
.github/workflows/commit-signing.ymlCHANGELOG.mddocs/ops/commit-signing.mdscripts/commit-signing-check.ps1tests/commit_signing_check.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (31)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: tsan permutation · race_model
- GitHub Check: fuzz smoke (10 seconds)
- GitHub Check: flake tracker records + rerun stats
- GitHub Check: durable schema migration scaffold
- GitHub Check: eval reproducibility manifest
- GitHub Check: coverage (85% gate)
- GitHub Check: reproducible build · sl-daemon
- GitHub Check: OpenAPI route drift · sl-daemon
- GitHub Check: clean-host smoke · Windows unsigned portable install
- GitHub Check: shuttle permutation · cargo test shuttle_permutation
- GitHub Check: cargo deny check
- GitHub Check: cargo audit
- GitHub Check: race smoke + channel/cancel model · windows-latest
- GitHub Check: update check hard · sl-daemon tests
- GitHub Check: update check hard · root SelfCheck wrapper
- GitHub Check: visual contract · WCAG AA
- GitHub Check: fuzz blocking · sustained 30s
- GitHub Check: loom permutation · core models
- GitHub Check: loom permutation · daemon pipeline
- GitHub Check: loom permutation · hermetic wrappers
- GitHub Check: loom permutation · daemon mpsc
- GitHub Check: loom permutation · daemon broadcast
- GitHub Check: loom permutation · daemon shutdown
- GitHub Check: jemalloc hard · feature build
- GitHub Check: soft loom · daemon mpsc
- GitHub Check: soft loom · daemon broadcast
- GitHub Check: sl-daemon build · windows-latest
- GitHub Check: soft loom · loom_model core
- GitHub Check: session-ledger build · windows-latest
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Use the Rust toolchain pinned inrust-toolchain.toml; the workspace MSRV is Rust 1.85.
Validate Rust workspace changes with the prescribed locked build, all-features test suite, Clippy, and rustfmt checks where applicable.
Files:
tests/commit_signing_check.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Fix Clippy warnings; do not add
#[allow]unless it includes a tracking-issue comment.
Files:
tests/commit_signing_check.rs
*
📄 CodeRabbit inference engine (AGENTS.md)
*: Perform feature work in a git worktree under.claude/worktrees/, created fromorigin/mainon a branch named<type>/<topic>, rather than working directly onmain.
Do not make direct commits to protectedmain; use a pull request.
Do not usegit reset --hard,git stash, orgit cleanin worktrees.
Do not use--no-verifyor bypass hooks without operator approval.
Do not work on a branch or worktree another actor is using.
Files:
CHANGELOG.md
🪛 PSScriptAnalyzer (1.25.0)
scripts/commit-signing-check.ps1
[warning] 23-23: The parameter 'Repo' has been declared but not used.
(PSReviewUnusedParameter)
[warning] 48-48: The cmdlet 'Assert-Contains' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Assert-Contains'
(PSUseSingularNouns)
[warning] 62-62: The cmdlet 'Get-CommitHeaderLines' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Get-CommitHeaderLines'
(PSUseSingularNouns)
[warning] 268-268: The cmdlet 'Test-SignatureClassificationFixtures' uses a plural noun. A singular noun should be used instead.
Suggested fix: Singularized correction of 'Test-SignatureClassificationFixtures'
(PSUseSingularNouns)
🔇 Additional comments (10)
scripts/commit-signing-check.ps1 (6)
1-26: LGTM!Also applies to: 28-60
128-166: LGTM!
168-266: LGTM!
268-360: LGTM!
385-385: LGTM!Also applies to: 409-409
81-119: 🩺 Stability & AvailabilityUnread
StandardErrordoes not appear to block this path.
git cat-file -pon a valid commit emits no stderr here, and the code already filters invalid objects with the earliergit cat-file -scheck. This deadlock concern doesn’t hold for the current implementation.> Likely an incorrect or invalid review comment.tests/commit_signing_check.rs (1)
1-42: LGTM!docs/ops/commit-signing.md (1)
71-73: LGTM!CHANGELOG.md (1)
9-10: LGTM!.github/workflows/commit-signing.yml (1)
11-11: LGTM!Also applies to: 21-21, 51-54
| $sizeRaw = (& git cat-file -s $Sha 2>$null | Out-String).Trim() | ||
| if ($LASTEXITCODE -ne 0 -or [string]::IsNullOrWhiteSpace($sizeRaw)) { | ||
| return $null | ||
| } | ||
| if ($sizeRaw -notmatch '^\d+$') { | ||
| throw "Unexpected git cat-file -s output for ${Sha}: $sizeRaw" | ||
| } | ||
| $objectSize = [long]::Parse($sizeRaw) | ||
| if ($objectSize -gt $ObjectByteCap) { | ||
| throw "Commit object $Sha size $objectSize exceeds cap $ObjectByteCap bytes" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="scripts/commit-signing-check.ps1"
wc -l "$file"
echo '--- lines 1-180 ---'
sed -n '1,180p' "$file" | cat -nRepository: KooshaPari/SessionLedger
Length of output: 6902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "MaxCommitObjectBytes|MaxHeaderLines|Get-CommitHeaderLines|cat-file -s|ObjectByteCap" scripts README.md .github -SRepository: KooshaPari/SessionLedger
Length of output: 1890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '320,350p' scripts/commit-signing-check.ps1 | cat -nRepository: KooshaPari/SessionLedger
Length of output: 1828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'commit-signing*.md' . docs .github scripts 2>/dev/null || true
rg -n "commit-signing|MaxCommitObjectBytes|bounded line-scanner|header" docs .github scripts -SRepository: KooshaPari/SessionLedger
Length of output: 13561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' docs/ops/commit-signing.md | cat -n
echo '---'
sed -n '1,120p' docs/adr/0004-commit-signing-policy.md | cat -nRepository: KooshaPari/SessionLedger
Length of output: 9856
Drop or document the 1 MB object-size gate
git cat-file -s rejects any commit object over MaxCommitObjectBytes before the header scan runs, so a valid signed commit with a large message body will fail this check even though the later byte/line caps only bound the header read. If that limit is intentional, call it out explicitly; otherwise rely on the streaming caps alone.
🤖 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 `@scripts/commit-signing-check.ps1` around lines 69 - 79, Update the
object-size check in the commit-object validation flow around `$sizeRaw` and
`$objectSize`: either remove the `MaxCommitObjectBytes`/`$ObjectByteCap`
rejection so large commit messages are governed only by the streaming byte and
line caps, or explicitly document this intentional 1 MB limit and its effect on
valid signed commits. Preserve the existing `git cat-file -s` validation and
malformed-output handling.
| $lines = [System.Collections.Generic.List[string]]::new() | ||
| $bytesRead = 0 | ||
| try { | ||
| while ($true) { | ||
| $line = $proc.StandardOutput.ReadLine() | ||
| if ($null -eq $line) { | ||
| break | ||
| } | ||
| $bytesRead += [System.Text.Encoding]::UTF8.GetByteCount($line) + 1 | ||
| if ($bytesRead -gt $ObjectByteCap) { | ||
| throw "Commit object $Sha exceeded byte cap while streaming header" | ||
| } | ||
| if ([string]::IsNullOrWhiteSpace($line)) { | ||
| break | ||
| } | ||
| $lines.Add($line) | ||
| if ($lines.Count -gt $HeaderLineCap) { | ||
| throw "Commit object $Sha header exceeds $HeaderLineCap lines" | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Blank-continuation line inside gpgsig is misread as the header/body separator.
[string]::IsNullOrWhiteSpace($line) returns true for a single-space continuation line, but in git's raw commit format a truly empty header/body separator (zero-length line) is distinct from a continuation line representing a blank line embedded inside a multi-line header value (e.g. PGP armor requires a blank line between its headers and the base64 body, encoded by git as a single-space continuation). For essentially every real GPG-signed commit, the second line of the gpgsig field is exactly that single-space continuation, so this loop breaks immediately after the gpgsig -----BEGIN PGP SIGNATURE----- line, believing headers ended there.
Classification happens to still succeed today only because Get-CommitSignatureKindFromHeader checks for the BEGIN ...SIGNATURE marker, which is already on that first captured line — but the reader silently discards the rest of the signature block (including the END marker) and any header fields that might follow gpgsig, contrary to its documented bounded-but-complete purpose. The self-check fixtures at Test-SignatureClassificationFixtures (lines 268-308) never catch this because they call Get-CommitSignatureKindFromHeader directly with hand-built arrays, bypassing this reader entirely.
Use the true separator condition (zero-length line) instead of whitespace-only.
🐛 Proposed fix
- if ([string]::IsNullOrWhiteSpace($line)) {
+ if ($line.Length -eq 0) {
break
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $lines = [System.Collections.Generic.List[string]]::new() | |
| $bytesRead = 0 | |
| try { | |
| while ($true) { | |
| $line = $proc.StandardOutput.ReadLine() | |
| if ($null -eq $line) { | |
| break | |
| } | |
| $bytesRead += [System.Text.Encoding]::UTF8.GetByteCount($line) + 1 | |
| if ($bytesRead -gt $ObjectByteCap) { | |
| throw "Commit object $Sha exceeded byte cap while streaming header" | |
| } | |
| if ([string]::IsNullOrWhiteSpace($line)) { | |
| break | |
| } | |
| $lines.Add($line) | |
| if ($lines.Count -gt $HeaderLineCap) { | |
| throw "Commit object $Sha header exceeds $HeaderLineCap lines" | |
| } | |
| } | |
| $lines = [System.Collections.Generic.List[string]]::new() | |
| $bytesRead = 0 | |
| try { | |
| while ($true) { | |
| $line = $proc.StandardOutput.ReadLine() | |
| if ($null -eq $line) { | |
| break | |
| } | |
| $bytesRead += [System.Text.Encoding]::UTF8.GetByteCount($line) + 1 | |
| if ($bytesRead -gt $ObjectByteCap) { | |
| throw "Commit object $Sha exceeded byte cap while streaming header" | |
| } | |
| if ($line.Length -eq 0) { | |
| break | |
| } | |
| $lines.Add($line) | |
| if ($lines.Count -gt $HeaderLineCap) { | |
| throw "Commit object $Sha header exceeds $HeaderLineCap lines" | |
| } | |
| } |
🤖 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 `@scripts/commit-signing-check.ps1` around lines 93 - 112, Update the
header-reading loop around the line classification in the commit-signing check
so only a truly zero-length string terminates header parsing; do not treat
whitespace-only continuation lines as the header/body separator. Preserve the
existing byte and line caps and continue capturing the complete bounded gpgsig
block for downstream processing.
| if ($proc.ExitCode -ne 0 -and $lines.Count -eq 0) { | ||
| return $null | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Partial header silently accepted on nonzero exit code.
If git cat-file -p exits nonzero after emitting some lines, those partial lines are returned as-is rather than surfacing the failure, since the return $null guard only fires when $lines.Count -eq 0. A partial/corrupted header could then be classified normally instead of failing loudly.
🤖 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 `@scripts/commit-signing-check.ps1` around lines 121 - 123, Update the git
cat-file result handling around the process exit check so any nonzero
$proc.ExitCode returns $null or otherwise surfaces the failure regardless of
$lines.Count. Preserve the existing successful path for zero exit codes,
preventing partial output from being classified as a valid header.
1 participant
Summary
Test plan
Made with Cursor