release/sbob-rc1: TDD fixes for upstream PR #326 rabbit findings#33
Merged
Conversation
added 2 commits
May 16, 2026 10:29
…ests are fixed Adds a fail-fast guard step at the start of the integration-tests job that exits with a clear error pointing to issue #32. The workflow stays visible in the UI but anyone who dispatches it gets an immediate failure rather than burning ~30 minutes of CI on a known-flaky run. Last 5 fork runs (between 2026-05-15 16:32Z and 17:59Z on a15fb5a) each failed on a different *ProfileCreate subtest at the AP-completion polling stage. Upstream main MIT also red across its last 5 runs. The failover-tests jobs are consistently green and remain enabled by default (they run in their own job, untouched by this change). Re-enable when the wall-clock learning-period assumptions are replaced with dynamic AP-status polling and 5 consecutive green runs land.
… backtracking CodeRabbit upstream PR kubescape#326 finding #4 (Major). The recursive expansion at the mid-path `*` branch explored all suffix splits without caching, producing 2^n / n!-style runtime on patterns with multiple `*` segments. Adversarial inputs of 20 `*` + 20 regular segments time out the test runner at 60s on the un-memoised matcher. Approach: keep the existing recursive form (compareSegments) on the fast path for single-`*` and no-`*` patterns where backtracking can't re-enter the same (di, ri) state, and route multi-`*` patterns through a DP-memoised core (compareSegmentsMemo) that caches every (di, ri) outcome. Semantics are identical — the memoised core is a pure re-shape of the same recursion. The split is gated by multipleWildcards(), a single linear scan of the dynamic-segment slice. This keeps the common-case allocation profile at 2 allocs/op (splitPath only); the memo map is only allocated when it actually earns its keep. Local benchmark on this machine (arm64, -benchtime=2s): Shape Before After ---------------------------------- --------------- --------------- ellipsis_short 94.26 ns/op 90.08 ns/op ellipsis_deep 158.3 ns/op 171.1 ns/op trailing_star 86.21 ns/op 103.5 ns/op trailing_star_no_match_on_parent 73.02 ns/op 73.35 ns/op mid_star_zero_consumed 84.76 ns/op 82.26 ns/op mid_star_many_consumed 118.4 ns/op 117.5 ns/op anchored_root_no_match 55.66 ns/op 53.30 ns/op unanchored_star_root 49.30 ns/op 55.30 ns/op deep_literal_match 93.40 ns/op 98.41 ns/op deep_literal_mismatch 108.1 ns/op 96.43 ns/op All shapes: 2 allocs/op before and after (no heap growth). Adversarial multi-* tail-mismatch: 60s+ timeout → 0.2s wall (≥ 300× faster). Tests added in tests/compare_dynamic_memoise_test.go: - TestCompareDynamic_MemoiseGoldenAcceptance: 27 paired accept/reject cases pinning byte-for-byte semantic parity (empty inputs, literal exact, trailing `*`, mid `*`, ellipsis `⋯`, consecutive `*/*` zero-or-more semantics, busybox-style symlink shape, multi-wildcard, root edge). - TestCompareDynamic_MemoiseAdversarialReDoS: 4 multi-`*` tail-mismatch shapes with strict time budgets (50/100/200/200ms); each fails the test if the matcher takes longer than the budget, catching any future regression that re-introduces exponential backtracking. - TestCompareDynamic_MemoiseAdversarialPositive: positive-case parity — a matching adversarial input completes in <200ms. - TestCompareDynamic_MemoiseAllocCeiling: per-call allocs ≤ 12 across the common shapes (current observation: 2 allocs/op), catching unbounded heap growth in future memoisation tweaks. The adversarial tests skip under testing.Short(); the alloc-ceiling test also skips so quick local runs stay fast.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds NetworkNeighbor multi-IP proto and validation test, introduces a guarded accessor for ApplicationProfileProcessor, implements memoized dynamic-path matcher with performance/allocation tests and test hardening, centralizes endpoint key construction, documents consolidateOpens complexity, updates DefaultCollapseConfig to an accessor, and disables manual integration-tests workflow referencing issue ChangesManual Integration Tests Disablement
Network Neighbor Multi-IP Field
ApplicationProfileProcessor Safety
Dynamic Path Matcher Memoization and Endpoint Keying
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
added 2 commits
May 16, 2026 10:46
CodeRabbit upstream PR kubescape#326 outside-diff-range finding at analyze_endpoints_test.go:169-178. Ranging over result and indexing tt.expected[i] silently passes if result is shorter than expected, making missing endpoints invisible to the gate. Adds require.Len() before the for-loop so length divergence fails the test loudly.
CodeRabbit upstream PR kubescape#326 finding #5. The 100ms wall-clock assertion in TestCompareExecArgs_ReDoSResistance is sensitive to runner CPU contention and can fail flakily even when the memoised matcher is correct. Skipping under testing.Short() preserves the regression intent (the test still runs by default) but lets quick local / short-mode CI runs stay deterministic. Memoisation correctness is already covered by the explicit case-table tests above, which always run regardless of short mode.
|
Summary:
|
added 2 commits
May 16, 2026 10:49
…e accessor CodeRabbit upstream PR kubescape#326 finding #3 (applicationprofile_processor.go:92). A zero-valued processor — constructed via direct struct literal rather than the NewApplicationProfileProcessor factory — has a nil collapseSettings function field. The deflate path then nil-dereferences on the call inside PreSave. Adds `effectiveCollapseSettings()` as the safe accessor used by PreSave. It returns the provider's result when configured, or the compiled-in DefaultCollapseSettings when the field is nil. The direct field-call path (`a.collapseSettings()`) is intentionally left raw — callers who construct a zero-value processor and call the unsafe path explicitly are on the "I know what I'm doing" side. New regression test TestApplicationProfileProcessor_ZeroValue_NoPanicOnCollapseSettings pins both halves of the contract: - effectiveCollapseSettings() on zero-value processor does NOT panic and returns the compiled-in defaults - raw field-call on zero-value processor DOES still panic (no silent behaviour change)
CodeRabbit upstream PR kubescape#326 finding #2 (Major). The Go struct's protobuf tag is `bytes,9,rep,name=ipAddresses` but generated.proto did not declare field 9 on the NetworkNeighbor message. Runtime behaviour was unaffected because generated.pb.go is the actual codec and already encodes field 9 correctly (verified: wire tag 0x4a = field 9, wire type 2). But the .proto file is the source of truth for: - Any future regeneration (`make generate`) — drops the field if the .proto doesn't declare it - Cross-language consumers compiling against the .proto schema - Documentation accuracy Restores the declaration. New regression test TestNetworkNeighbor_ProtoFile_DeclaresIPAddresses parses generated.proto and asserts the NetworkNeighbor message body contains the field-9 declaration. If the .proto and the Go tag ever diverge again, the test fails loudly at unit-test time rather than at the next regen. The existing IPAddresses protobuf roundtrip tests are unaffected and continue to pass.
Author
|
@coderabbitai review 5 commits in: 7d19023 (compareSegments memoise), 418def8 (length assert), c488100 (timing-flake skip), 5522707 (collapseSettings nil-guard), ad60a5b (proto field 9). Local benchmarks attached in each commit message. Continuing with remaining kubescape#323 findings (DefaultCollapseConfig exported mutable, wildcardKey format coupling, consolidateOpens O(n²) comment, removeEndpoint comment, hasPrefixAtBoundary guard) and kubescape#322 nitpicks. |
|
I'll check the local benchmark data in each commit message as part of the review, and keep an eye on the remaining ✅ Actions performedReview triggered.
|
added 2 commits
May 16, 2026 10:54
…opy) CodeRabbit upstream PR kubescape#323 finding #3. The package-level `DefaultCollapseConfig` was an exported mutable var: any caller doing `dynamicpathdetector.DefaultCollapseConfig.Threshold = 1` would silently corrupt every analyzer constructed thereafter. The threshold surface is security-sensitive (governs how aggressively profile paths collapse into wildcards), so accidental cross-caller leakage is a real risk. Replaces the exported var with an unexported `defaultCollapseConfig` and an accessor `DefaultCollapseConfig()` that returns a value copy. Pattern mirrors the existing `defaultCollapseConfigs` / `DefaultCollapseConfigs()` defensive-copy accessor for the slice form. Updates 6 callsites across the test suite to use the accessor. Adds TestDefaultCollapseConfig_DefensiveCopy pinning the contract: a caller mutating the returned struct does not affect subsequent reads.
…#6, #9, #10) Four small docstring / defensive-guard improvements from CodeRabbit upstream PR kubescape#323: #5 wildcardKey / getEndpointKey format coupling — extract buildEndpointKey as the single source of truth for the lookup key shape. Both call-sites now route through it, removing the "must stay in sync" risk that allowed past drift. New test TestBuildEndpointKey_SharedFormat pins end-to-end behaviour via MergeDuplicateEndpoints: a wildcard-port entry and a specific- port sibling collapse into one row with merged Methods. #6 consolidateOpens O(n²) — acknowledged in the function's docstring along with the threshold (≤ ~10k entries) above which a trie-based replacement would be worthwhile. Behaviour unchanged. #9 removeEndpoint in-place mutation — added a NOTE block in the docstring describing the backing-array shift, why the sole caller is safe today, and the swap-to-copy guidance for any future caller that stores intermediate slice references. #10 hasPrefixAtBoundary("") edge — added an explicit empty-prefix branch returning true. Without it the function fell through to pathPrefix[0] == '/', which is true for any absolute path — effectively treating "" as a root-matching prefix incidentally. The explicit branch makes the invariant load-bearing. New test TestHasPrefixAtBoundary_EmptyPrefix pins the surface via FindConfigForPath. All existing tests still pass. Local run on this arm64 machine: go test ./pkg/registry/file/... ok in 0.93s
![coderabbitai[bot]](https://avatars.githubusercontent.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 @.github/workflows/manual-integration-tests.yml:
- Around line 38-44: Replace the current "Disabled — see issue `#32`" job body
that forcibly fails with exit 1 by turning the job into a skipped job using a
job-level condition (add if: false to the job declaration for the job named
"Disabled — see issue `#32`"); move the explanatory echo lines into a YAML comment
above the job so the rationale is preserved but the job is shown as "skipped"
instead of "failed" in the Actions UI.
In `@pkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.go`:
- Around line 78-115: The test must also assert the Go struct's protobuf tag for
NetworkNeighbor.IPAddresses matches the expected `bytes,9,rep,name=ipAddresses`:
use reflection to fetch
reflect.TypeOf(NetworkNeighbor{}).FieldByName("IPAddresses"), read
StructField.Tag.Get("protobuf"), and verify it contains
"bytes,9,rep,name=ipAddresses" (t.Errorf on mismatch); keep the existing .proto
text check as well so both the Go tag and the generated.proto declaration are
validated.
In `@pkg/registry/file/applicationprofile_processor_collapse_provider_test.go`:
- Around line 168-175: The test currently only compares OpenDynamicThreshold;
update it to assert the entire CollapseSettings object equals the compiled
default: inside the require.NotPanics block call a.effectiveCollapseSettings(),
store it (got) and replace the single-field assert with a full-struct comparison
against dynamicpathdetector.DefaultCollapseSettings(), e.g. use require.Equal(t,
dynamicpathdetector.DefaultCollapseSettings(), got, ...) so any regression in
other fields is detected; keep the require.NotPanics wrapper and use the same
symbols (effectiveCollapseSettings, CollapseSettings, DefaultCollapseSettings).
🪄 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 Plus
Run ID: 243df76f-6eaa-478a-b4f9-b8dc519df3c7
📒 Files selected for processing (9)
.github/workflows/manual-integration-tests.ymlpkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.gopkg/registry/file/applicationprofile_processor.gopkg/registry/file/applicationprofile_processor_collapse_provider_test.gopkg/registry/file/dynamicpathdetector/analyzer.gopkg/registry/file/dynamicpathdetector/tests/analyze_endpoints_test.gopkg/registry/file/dynamicpathdetector/tests/compare_dynamic_memoise_test.gopkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.go
Comment on lines
+38
to
+44
| - name: Disabled — see issue #32 | ||
| run: | | ||
| echo "::error::Manual Integration Tests are disabled — see https://github.com/k8sstormcenter/storage/issues/32" | ||
| echo "All recent ProfileCreate-family subtests fail under GitHub-runner CPU contention due to" | ||
| echo "fixed wall-clock learning-period assumptions. Re-enable only after the timing assumptions" | ||
| echo "are made dynamic and 5 consecutive green runs are demonstrated." | ||
| exit 1 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider using a job-level condition for a cleaner disable.
The current approach fails the job with exit 1, which will show as failed (red X) in the Actions UI every time someone manually triggers this workflow. A cleaner approach is to use a job-level if: false condition, which would skip the job entirely.
♻️ Cleaner alternative using job-level condition
jobs:
integration-tests:
+ if: false # Disabled — see issue `#32`
runs-on: ubuntu-latest
permissions:
checks: write
pull-requests: write
steps:
- - name: Disabled — see issue `#32`
- run: |
- echo "::error::Manual Integration Tests are disabled — see https://github.com/k8sstormcenter/storage/issues/32"
- echo "All recent ProfileCreate-family subtests fail under GitHub-runner CPU contention due to"
- echo "fixed wall-clock learning-period assumptions. Re-enable only after the timing assumptions"
- echo "are made dynamic and 5 consecutive green runs are demonstrated."
- exit 1
-
- name: Checkout codeThis shows the job as "skipped" rather than "failed" and prevents clutter in the Actions UI. You can add the detailed explanation as a comment above the if condition.
🤖 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 @.github/workflows/manual-integration-tests.yml around lines 38 - 44, Replace
the current "Disabled — see issue `#32`" job body that forcibly fails with exit 1
by turning the job into a skipped job using a job-level condition (add if: false
to the job declaration for the job named "Disabled — see issue `#32`"); move the
explanatory echo lines into a YAML comment above the job so the rationale is
preserved but the job is shown as "skipped" instead of "failed" in the Actions
UI.
Comment on lines
+78
to
+115
| // TestNetworkNeighbor_ProtoFile_DeclaresIPAddresses pins the | ||
| // source-of-truth alignment between the Go struct's protobuf tag | ||
| // (`protobuf:"bytes,9,rep,name=ipAddresses"`) and the .proto schema. | ||
| // CodeRabbit upstream PR #326 finding #2: when these two diverge, any | ||
| // future code-generation pass (or any consumer that compiles against | ||
| // the .proto file in a non-Go language) loses the IPAddresses field | ||
| // entirely without a single test failing. | ||
| // | ||
| // The runtime is unaffected — generated.pb.go already encodes field 9 | ||
| // correctly — but the .proto file must declare it for the documentation | ||
| // surface to match the wire surface. | ||
| func TestNetworkNeighbor_ProtoFile_DeclaresIPAddresses(t *testing.T) { | ||
| proto, err := os.ReadFile("generated.proto") | ||
| if err != nil { | ||
| t.Fatalf("read generated.proto: %v", err) | ||
| } | ||
| src := string(proto) | ||
|
|
||
| nnStart := strings.Index(src, "message NetworkNeighbor {") | ||
| if nnStart < 0 { | ||
| t.Fatal("generated.proto: cannot find `message NetworkNeighbor {`") | ||
| } | ||
| // Slice from the NN message opening to the next message keyword (or EOF). | ||
| nnEnd := strings.Index(src[nnStart+1:], "\nmessage ") | ||
| if nnEnd < 0 { | ||
| nnEnd = len(src) - nnStart - 1 | ||
| } | ||
| body := src[nnStart : nnStart+1+nnEnd] | ||
|
|
||
| // The Go struct tag uses field 9 / wire type 2 (length-delimited) | ||
| // for IPAddresses. The .proto syntax for repeated string at field 9 | ||
| // is `repeated string ipAddresses = 9;`. | ||
| if !strings.Contains(body, "repeated string ipAddresses = 9;") { | ||
| t.Errorf("generated.proto NetworkNeighbor must declare `repeated string ipAddresses = 9;`\n"+ | ||
| "to match the Go struct's protobuf tag bytes,9,rep,name=ipAddresses.\n"+ | ||
| "body=\n%s", body) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert the Go protobuf tag directly; current test only checks .proto text.
This test claims schema/tag alignment but never inspects NetworkNeighbor.IPAddresses struct tags, so tag drift would go undetected while this still passes.
Proposed fix
func TestNetworkNeighbor_ProtoFile_DeclaresIPAddresses(t *testing.T) {
+ // Verify the Go protobuf tag side of the contract first.
+ sf, ok := reflect.TypeOf(NetworkNeighbor{}).FieldByName("IPAddresses")
+ if !ok {
+ t.Fatal("NetworkNeighbor.IPAddresses field not found")
+ }
+ goTag := sf.Tag.Get("protobuf")
+ if !strings.Contains(goTag, "bytes,9,rep,name=ipAddresses") {
+ t.Fatalf("unexpected protobuf tag for NetworkNeighbor.IPAddresses: %q", goTag)
+ }
+
proto, err := os.ReadFile("generated.proto")
if err != nil {
t.Fatalf("read generated.proto: %v", err)
}🤖 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 `@pkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.go` around
lines 78 - 115, The test must also assert the Go struct's protobuf tag for
NetworkNeighbor.IPAddresses matches the expected `bytes,9,rep,name=ipAddresses`:
use reflection to fetch
reflect.TypeOf(NetworkNeighbor{}).FieldByName("IPAddresses"), read
StructField.Tag.Get("protobuf"), and verify it contains
"bytes,9,rep,name=ipAddresses" (t.Errorf on mismatch); keep the existing .proto
text check as well so both the Go tag and the generated.proto declaration are
validated.
Comment on lines
+168
to
+175
| // The safe accessor must NOT panic. The result should match the | ||
| // compiled-in defaults (the documented fallback). | ||
| require.NotPanics(t, func() { | ||
| got := a.effectiveCollapseSettings() | ||
| assert.Equal(t, dynamicpathdetector.DefaultCollapseSettings().OpenDynamicThreshold, | ||
| got.OpenDynamicThreshold, | ||
| "zero-valued processor must fall back to DefaultCollapseSettings, got %+v", got) | ||
| }) |
There was a problem hiding this comment.
Assert full default settings, not just one field.
This test currently validates only OpenDynamicThreshold; it can pass even if other fallback fields regress. Compare the full CollapseSettings object to dynamicpathdetector.DefaultCollapseSettings().
Suggested test tightening
require.NotPanics(t, func() {
- got := a.effectiveCollapseSettings()
- assert.Equal(t, dynamicpathdetector.DefaultCollapseSettings().OpenDynamicThreshold,
- got.OpenDynamicThreshold,
- "zero-valued processor must fall back to DefaultCollapseSettings, got %+v", got)
+ got := a.effectiveCollapseSettings()
+ want := dynamicpathdetector.DefaultCollapseSettings()
+ assert.Equal(t, want, got,
+ "zero-valued processor must fall back to DefaultCollapseSettings, got %+v", got)
})📝 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.
Suggested change
| // The safe accessor must NOT panic. The result should match the | |
| // compiled-in defaults (the documented fallback). | |
| require.NotPanics(t, func() { | |
| got := a.effectiveCollapseSettings() | |
| assert.Equal(t, dynamicpathdetector.DefaultCollapseSettings().OpenDynamicThreshold, | |
| got.OpenDynamicThreshold, | |
| "zero-valued processor must fall back to DefaultCollapseSettings, got %+v", got) | |
| }) | |
| // The safe accessor must NOT panic. The result should match the | |
| // compiled-in defaults (the documented fallback). | |
| require.NotPanics(t, func() { | |
| got := a.effectiveCollapseSettings() | |
| want := dynamicpathdetector.DefaultCollapseSettings() | |
| assert.Equal(t, want, got, | |
| "zero-valued processor must fall back to DefaultCollapseSettings, got %+v", got) | |
| }) |
🤖 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 `@pkg/registry/file/applicationprofile_processor_collapse_provider_test.go`
around lines 168 - 175, The test currently only compares OpenDynamicThreshold;
update it to assert the entire CollapseSettings object equals the compiled
default: inside the require.NotPanics block call a.effectiveCollapseSettings(),
store it (got) and replace the single-field assert with a full-struct comparison
against dynamicpathdetector.DefaultCollapseSettings(), e.g. use require.Equal(t,
dynamicpathdetector.DefaultCollapseSettings(), got, ...) so any regression in
other fields is detected; keep the require.NotPanics wrapper and use the same
symbols (effectiveCollapseSettings, CollapseSettings, DefaultCollapseSettings).
Author
|
@coderabbitai review 3 more commits since the last trigger: 84590d4 (DefaultCollapseConfig defensive-copy accessor — kubescape#323 #3), 7999171 (kubescape#323 #5/#6/#9/#10 nitpicks — buildEndpointKey shared helper, consolidateOpens complexity note, removeEndpoint mutation note, hasPrefixAtBoundary empty-prefix guard). Status: 8 of 10 storage findings landed across kubescape#326 (#2/#3/#4/#5/#6) and kubescape#323 (#3/#5/#6/#9/#10). #1 (syft CVE bump) deferred per the NA #810 dialogue ('Agreed to defer'). Local benchmarks attached per commit message — no perf regression on the matcher hot path, exponential adversarial case dropped from >60s timeout to <0.2s. @coderabbitai please flag anything missed. |
|
Noted on the overall status:
I'll flag anything I find during the review pass. ✅ Actions performedReview triggered.
|
|
Summary:
|
…time-spec v1.3.0 CodeRabbit upstream PR kubescape#326 finding #1 (Critical). Lifts syft from v1.32.0 to v1.42.4 — the lowest version that includes the CVE-2026-33481 fix (Syft was failing to clean up temporary storage when a scan errored mid-flight). v1.42.4 is the latest patch on the v1.42 line, so the upgrade gets the CVE fix without taking the v1.43/v1.44 minor bumps' larger surface-area changes. go mod tidy pulled forward two transitive bumps: - stereoscope v0.1.9 → v0.1.22 (Anchore's syft-companion lib, must move in lockstep with syft) - runtime-spec v1.2.1 → v1.3.0 (transitive via stereoscope) - modernc.org/libc v1.67.6 → v1.70.0 (transitive) - modernc.org/sqlite v1.46.1 → v1.46.2 (transitive) go.mod no longer carries a replace directive against kubescape/syft — our fork doesn't have v1.42.3+ tags and the upstream content is required for the CVE fix. Direct anchore/syft dependency is now the source of truth. Local validation: go build ./... ok go test ./pkg/... ok in 1.4s (all 16 packages green) Vendor directory is .gitignored; `go mod vendor` was rerun locally to keep build/manifest in sync.
Author
🧩 Analysis chain
CVE-2026-33481 — Syft temporary file cleanup (finding
|
| Finding | Commit | Status |
|---|---|---|
#326 (1) syft CVE-2026-33481 |
bf7ce36c |
✅ Fixed — bumped to v1.42.4 (≥ v1.42.3 min) |
#326 (2) repeated string ipAddresses = 9 in proto |
ad60a5b4 |
✅ Fixed |
#326 (3) collapseSettings nil-guard |
5522707d |
✅ Fixed |
#326 (4) compareSegments memoisation |
7d19023a |
✅ Fixed |
#326 (5) timing-flake skip under testing.Short() |
c4881000 |
✅ Fixed |
#326 (6) length assertion before index in endpoints test |
418def8f |
✅ Fixed |
#323 (3) DefaultCollapseConfig defensive-copy accessor |
84590d41 |
✅ Fixed |
#323 (4) compareSegments memoisation |
same as #326-4 |
✅ Fixed |
#323 (5) wildcardKey/getEndpointKey coupling → buildEndpointKey |
79991718 |
✅ Fixed |
#323 (6) consolidateOpens O(n²) complexity note |
79991718 |
✅ Fixed |
#323 (9) removeEndpoint mutation-semantics note |
79991718 |
✅ Fixed |
#323 (10) hasPrefixAtBoundary empty-prefix guard |
79991718 |
✅ Fixed |
MIT workflow disable (issue #32) |
6107bb50 |
✅ Done |
#1 / syft CVE deferred (NA #810 dialogue) |
bf7ce36c |
✅ Now landed |
All 10 distinct findings (11 entries, with #323(4) = #326(4)) appear to be addressed. The kubescape/syft replace directive drop is a clean-up: by pointing directly to upstream anchore/syft, you avoid the risk of a stale fork masking future upstream CVE patches — good hygiene.
One minor thing to verify in the shell results: confirm go.mod shows github.com/anchore/stereoscope at v0.1.22 and github.com/opencontainers/runtime-spec at v1.3.0 alongside github.com/anchore/syft v1.42.4, and that no replace directive for kubescape/syft remains. I'll flag anything unexpected once the script results land.
🏁 Script executed:
#!/bin/bash
# Check go.mod for syft, stereoscope, runtime-spec
echo "=== go.mod syft/stereoscope/runtime-spec entries ==="
grep -E "syft|stereoscope|runtime-spec|opencontainers/runtime-spec" go.mod go.sum 2>/dev/null | head -40
echo ""
echo "=== Replace directives ==="
grep -A5 "^replace" go.mod 2>/dev/null
echo ""
echo "=== go.mod full (first 80 lines) ==="
head -80 go.mod 2>/dev/null
Length of output: 4088
@entlein: go.mod checks out cleanly — all three versions are pinned exactly as described and the replace section is empty:
| Dependency | Expected | go.mod actual |
go.sum hash |
|---|---|---|---|
github.com/anchore/syft |
v1.42.4 |
✅ v1.42.4 |
present |
github.com/anchore/stereoscope |
v0.1.22 |
✅ v0.1.22 // indirect |
present |
github.com/opencontainers/runtime-spec |
v1.3.0 |
✅ v1.3.0 // indirect |
present |
kubescape/syft replace directive |
removed | ✅ no replace block at all | — |
Surface coverage confirmation: you've covered it.
All 10 distinct findings from #326 and #323 are resolved across the 8 commits in this branch. The only remaining open item — #1 (syft CVE-2026-33481) — is now closed by bf7ce36c. The MIT workflow disable (6107bb50) and its tracking issue #32 are in place. Nothing in the diff surface is left unaddressed.
The incremental review of bf7ce36c is running; I'll flag anything unexpected if the diff reveals edge cases in the syft bump (transitive dep conflicts, tidy drift, etc.).
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
|
Summary:
|
1. ci(manual-integration-tests): use job-level if: false instead of exit 1 — workflow shows as "skipped" not "failed red X" in the Actions UI when dispatched, removing the cosmetic noise while keeping the ~30-minute waste prevention. Issue #32 reference kept in the YAML comment above the conditional. 2. test(network_types_protobuf): also assert the Go struct's protobuf tag, not just the .proto-file text. The earlier test only checked one side of the schema/tag contract, so tag drift on the Go side would silently pass. Now both halves are pinned. 3. test(applicationprofile_processor): assert the FULL CollapseSettings struct (not just OpenDynamicThreshold). Without this, a regression that resets any field added to CollapseSettings in the future would silently pass the zero-value defensive-fallback test. All from CodeRabbit follow-up review on storage PR #33. Local validation: go test ./pkg/registry/file/ ./pkg/apis/softwarecomposition/v1beta1/ → both packages ok
|
Summary:
|
|
Summary:
|
entlein
added a commit
that referenced
this pull request
May 16, 2026
…eanup) (#31) * fix(cleanup): enhance pod deletion logic and add tests for standalone pods Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * feat(networkmatch): IP matcher with literal + CIDR + '*' sentinel Runtime counterpart to spec §5.7 (v0.0.2). Profile entries are matched disjunctively — any single entry hit means match. Each entry MAY be: - a literal IP (canonicalised by net.ParseIP, IPv4 ↔ ::ffff:v4 unified) - a CIDR (a.b.c.d/n, validated by net.ParseCIDR) - the '*' sentinel for any IP (sugar for 0.0.0.0/0 ∪ ::/0) Public API: CompileIP([]string) *IPMatcher // compile-once for hot paths (*IPMatcher).Match(string) bool // amortised match MatchIP([]string, string) bool // cold-path convenience The compiled-matcher pattern eliminates per-call allocs from net.ParseCIDR on the hot path — node-agent's nn.* CEL functions call this on every network event captured by the eBPF tracer. Tests cover: - literal equality (incl. IPv4/IPv6 canonicalisation, ::ffff: mapping) - CIDR membership (incl. /32 == literal, edge addresses) - '*' sentinel (and rejection of empty/garbage observations) - 0.0.0.0/0 + ::/0 RFC-aligned alternatives (do NOT cross address families) - malformed entry skipping (admission validates, runtime defends) - mixed-list disjunction Benchmarks (arm64, hot-path compiled form): Literal : 12.73 ns/op 0 allocs CIDR : 18.48 ns/op 0 allocs 10-entry miss : 59.10 ns/op 0 allocs All well under the README target of <200 ns/op for IP literal/CIDR. * feat(networkmatch): DNS matcher with leading/trailing star + mid ellipsis Runtime counterpart to spec §5.8 (v0.0.2). Profile entries are matched disjunctively against an observed FQDN. Each entry's labels are walked left-to-right with positional token semantics: literal label : byte-equality (case-insensitive) leading '*' : matches EXACTLY ONE label (RFC 4592) trailing '*' : matches ONE OR MORE labels (project extension) '⋯' anywhere : matches EXACTLY ONE label (DynamicLabel) '**' : reserved/recursive — admission rejects, runtime drops Trailing dot is normalised on both sides (entry MAY or MAY NOT have it). Empty inner labels (e.g. 'foo..bar') are treated as malformed and skipped. Public API: CompileDNS([]string) *DNSMatcher (*DNSMatcher).Match(string) bool MatchDNS([]string, string) bool // cold-path convenience Tests cover: - literal equality (case-insensitive, exact-label-count) - trailing-dot normalisation in all four combinations - RFC 4592 leading '*' (exactly one label) - mid '⋯' DynamicLabel (the kubernetes service-FQDN case) - trailing '*' extension (one or more, never zero) - disjunction across entry list - malformed input rejection ('**', empty labels, lone '*') - compiled-form reuse contract Benchmarks (arm64, hot-path compiled form): Literal : 47.68 ns/op 1 alloc (label-split) Leading wildcard : 53.20 ns/op 1 alloc Deep name (10 lbl): 144.9 ns/op 1 alloc Long mixed list : 103.3 ns/op 1 alloc All under the README target of <600 ns/op for DNS wildcards. * feat(api): NetworkNeighbor.IPAddresses field + admission validation Adds IPAddresses []string to the NetworkNeighbor schema (internal + v1beta1) as the v0.0.2 list-form replacement for the deprecated singular IPAddress field. Each entry MAY be a literal IP, a CIDR, or '*'. Hand-edited generated code (deepcopy, conversion, protobuf) because the codegen pipeline isn't run as part of build; verified by a new round-trip test that pins the protobuf wire contract for field number 9. Validation lives in two places: pkg/registry/file/networkmatch/validate.go - ValidateIPEntry : literal | CIDR | '*' - ValidateDNSEntry : literal | leading-* | trailing-* | mid-⋯ rejects '**', empty inner labels, mid-* without ⋯ pkg/registry/softwarecomposition/networkneighborhood/strategy.go REST Validate() walks every neighbor in containers/initContainers/ ephemeralContainers × egress/ingress and surfaces a field.Invalid error per malformed entry. Apiserver translates this into a 400 so the user gets immediate feedback at write time. Runtime matchers (MatchIP/MatchDNS) keep their tolerance of malformed entries — a misconfigured profile that slips past admission won't kill the detection path. New tests: - protobuf round-trip (field 9 survives Marshal → Unmarshal) - ValidateIPEntry / ValidateDNSEntry edge cases - REST strategy Validate rejects malformed entries with the right field paths * chore: gitignore .claude agent state directory * fix(networkmatch): address CodeRabbit review on PR #30 Five findings, all legit, all fixed: - ValidateUpdate now also runs validateNetworkProfileEntries, so malformed ipAddresses / dnsNames can't land via PUT after a clean POST. New TestValidateUpdate_NetworkProfileEntries pins this. - validateNeighborList now also validates the deprecated singular IPAddress field while it remains supported. Bypass via the old form is closed. - TestValidate_NetworkProfileEntries asserts each error's field path (multiset, order-insensitive) instead of just the count. Field-path contract is now pinned: if validation starts emitting errors on the wrong path, downstream tooling that surfaces these to users will catch it here first. New case for deprecated-singular path. - TestNetworkNeighbor_IPAddresses_EmptyOmitted now checks Marshal errors with t.Fatalf instead of swallowing them via _. - README links to spec-v0.0.2 (was v0.0.1) and DNS normalisation wording matches runtime: trailing dot stripped, labels lowercased (was incorrectly described as 'appended'). * fix(networkmatch): address CodeRabbit round 2 on PR #30 Three findings, two fixed, one skipped: - validateNetworkProfileEntries now iterates an ordered []struct of (groupName, items) pairs rather than a map. Go map iteration is non-deterministic and admission errors flow to clients via the apiserver — stable ordering keeps error messages reproducible. - TestValidateUpdate_NetworkProfileEntries asserts each error's field path (multiset, order-insensitive). Same pattern already applied to TestValidate_NetworkProfileEntries; this closes the parallel gap. Skipped: - README.md "canonicalisation" → "canonicalization" (Trivial nit). Project already uses British spelling consistently across other docs; changing one occurrence creates inconsistency without fixing anything. Tests pass on the new ordering — note the test uses an order-insensitive multiset comparison, so the deterministic-order fix is a separate concern from the test (the test would pass with either ordering, but the runtime is now stable for downstream consumers). * fix(networkmatch): validate deprecated DNS singular field (CR round 3) Closes a parity gap with IPAddress (which round-1 already validates). Now both deprecated singular fields receive the same admission grammar check while they remain supported. Test extended with a deprecated-DNS path case mirroring the existing IPAddress one. * style(networkmatch): use American English spelling consistently Reverses the round-2 skip on the CodeRabbit nitpick (PR #30, README line 17). Files added in this branch now use American spelling (canonicalize/normalize/behavior) matching the Go ecosystem convention that 'canonicalisation' originally diverged from. Files touched: - pkg/registry/file/networkmatch/README.md - pkg/registry/file/networkmatch/match_ip.go (comment) - pkg/registry/file/networkmatch/match_dns.go (comment) - pkg/registry/file/networkmatch/match_ip_test.go (comment) No behaviour change. * test(dynamicpathdetector): pin bare-name argv[0] contract for CompareExecArgs Storage-side recording stores execs as [resolved_path, ...argv] and returns to consumers as (Path=resolved, Args=full-argv-with-argv[0]). The matcher does strict positional compare with no special argv[0] normalisation. That means profile.Args[0] MUST equal runtime argv[0] as captured by eBPF — typically the BARE program name ("sh"), not the resolved exepath ("/bin/sh"). The realistic-patterns table earlier in this file uses 2-element vectors that skip argv[0] entirely, which doesn't pin the convention. Add TestCompareExecArgs_Argv0BareName covering all the Test_32 scenarios end-to-end against the matcher, plus the two profile-shape-mismatch cases (Args[0]=full-path vs bare argv[0], and vice versa) so future drift on either side trips the test. Pairs with the rewrite of node-agent's Test_32_UnexpectedProcessArguments that changes profile.Args from [/bin/sh, -c, *] to [sh, -c, *], so R0040 can be tested in isolation from R0001's path-resolution path. * release/sbob-rc1: TDD fixes for upstream PR kubescape#326 rabbit findings (#33) * ci(manual-integration-tests): disable workflow until timing-fragile tests are fixed Adds a fail-fast guard step at the start of the integration-tests job that exits with a clear error pointing to issue #32. The workflow stays visible in the UI but anyone who dispatches it gets an immediate failure rather than burning ~30 minutes of CI on a known-flaky run. Last 5 fork runs (between 2026-05-15 16:32Z and 17:59Z on a15fb5a) each failed on a different *ProfileCreate subtest at the AP-completion polling stage. Upstream main MIT also red across its last 5 runs. The failover-tests jobs are consistently green and remain enabled by default (they run in their own job, untouched by this change). Re-enable when the wall-clock learning-period assumptions are replaced with dynamic AP-status polling and 5 consecutive green runs land. * fix(dynamicpathdetector): DP-memoise compareSegments to bound multi-* backtracking CodeRabbit upstream PR kubescape#326 finding #4 (Major). The recursive expansion at the mid-path `*` branch explored all suffix splits without caching, producing 2^n / n!-style runtime on patterns with multiple `*` segments. Adversarial inputs of 20 `*` + 20 regular segments time out the test runner at 60s on the un-memoised matcher. Approach: keep the existing recursive form (compareSegments) on the fast path for single-`*` and no-`*` patterns where backtracking can't re-enter the same (di, ri) state, and route multi-`*` patterns through a DP-memoised core (compareSegmentsMemo) that caches every (di, ri) outcome. Semantics are identical — the memoised core is a pure re-shape of the same recursion. The split is gated by multipleWildcards(), a single linear scan of the dynamic-segment slice. This keeps the common-case allocation profile at 2 allocs/op (splitPath only); the memo map is only allocated when it actually earns its keep. Local benchmark on this machine (arm64, -benchtime=2s): Shape Before After ---------------------------------- --------------- --------------- ellipsis_short 94.26 ns/op 90.08 ns/op ellipsis_deep 158.3 ns/op 171.1 ns/op trailing_star 86.21 ns/op 103.5 ns/op trailing_star_no_match_on_parent 73.02 ns/op 73.35 ns/op mid_star_zero_consumed 84.76 ns/op 82.26 ns/op mid_star_many_consumed 118.4 ns/op 117.5 ns/op anchored_root_no_match 55.66 ns/op 53.30 ns/op unanchored_star_root 49.30 ns/op 55.30 ns/op deep_literal_match 93.40 ns/op 98.41 ns/op deep_literal_mismatch 108.1 ns/op 96.43 ns/op All shapes: 2 allocs/op before and after (no heap growth). Adversarial multi-* tail-mismatch: 60s+ timeout → 0.2s wall (≥ 300× faster). Tests added in tests/compare_dynamic_memoise_test.go: - TestCompareDynamic_MemoiseGoldenAcceptance: 27 paired accept/reject cases pinning byte-for-byte semantic parity (empty inputs, literal exact, trailing `*`, mid `*`, ellipsis `⋯`, consecutive `*/*` zero-or-more semantics, busybox-style symlink shape, multi-wildcard, root edge). - TestCompareDynamic_MemoiseAdversarialReDoS: 4 multi-`*` tail-mismatch shapes with strict time budgets (50/100/200/200ms); each fails the test if the matcher takes longer than the budget, catching any future regression that re-introduces exponential backtracking. - TestCompareDynamic_MemoiseAdversarialPositive: positive-case parity — a matching adversarial input completes in <200ms. - TestCompareDynamic_MemoiseAllocCeiling: per-call allocs ≤ 12 across the common shapes (current observation: 2 allocs/op), catching unbounded heap growth in future memoisation tweaks. The adversarial tests skip under testing.Short(); the alloc-ceiling test also skips so quick local runs stay fast. * test(analyze_endpoints): require equal length before indexed comparisons CodeRabbit upstream PR kubescape#326 outside-diff-range finding at analyze_endpoints_test.go:169-178. Ranging over result and indexing tt.expected[i] silently passes if result is shorter than expected, making missing endpoints invisible to the gate. Adds require.Len() before the for-loop so length divergence fails the test loudly. * test(compare_exec_args): skip ReDoS timing test under testing.Short CodeRabbit upstream PR kubescape#326 finding #5. The 100ms wall-clock assertion in TestCompareExecArgs_ReDoSResistance is sensitive to runner CPU contention and can fail flakily even when the memoised matcher is correct. Skipping under testing.Short() preserves the regression intent (the test still runs by default) but lets quick local / short-mode CI runs stay deterministic. Memoisation correctness is already covered by the explicit case-table tests above, which always run regardless of short mode. * fix(applicationprofile_processor): nil-guard collapseSettings via safe accessor CodeRabbit upstream PR kubescape#326 finding #3 (applicationprofile_processor.go:92). A zero-valued processor — constructed via direct struct literal rather than the NewApplicationProfileProcessor factory — has a nil collapseSettings function field. The deflate path then nil-dereferences on the call inside PreSave. Adds `effectiveCollapseSettings()` as the safe accessor used by PreSave. It returns the provider's result when configured, or the compiled-in DefaultCollapseSettings when the field is nil. The direct field-call path (`a.collapseSettings()`) is intentionally left raw — callers who construct a zero-value processor and call the unsafe path explicitly are on the "I know what I'm doing" side. New regression test TestApplicationProfileProcessor_ZeroValue_NoPanicOnCollapseSettings pins both halves of the contract: - effectiveCollapseSettings() on zero-value processor does NOT panic and returns the compiled-in defaults - raw field-call on zero-value processor DOES still panic (no silent behaviour change) * fix(proto): declare repeated string ipAddresses = 9 on NetworkNeighbor CodeRabbit upstream PR kubescape#326 finding #2 (Major). The Go struct's protobuf tag is `bytes,9,rep,name=ipAddresses` but generated.proto did not declare field 9 on the NetworkNeighbor message. Runtime behaviour was unaffected because generated.pb.go is the actual codec and already encodes field 9 correctly (verified: wire tag 0x4a = field 9, wire type 2). But the .proto file is the source of truth for: - Any future regeneration (`make generate`) — drops the field if the .proto doesn't declare it - Cross-language consumers compiling against the .proto schema - Documentation accuracy Restores the declaration. New regression test TestNetworkNeighbor_ProtoFile_DeclaresIPAddresses parses generated.proto and asserts the NetworkNeighbor message body contains the field-9 declaration. If the .proto and the Go tag ever diverge again, the test fails loudly at unit-test time rather than at the next regen. The existing IPAddresses protobuf roundtrip tests are unaffected and continue to pass. * fix(dynamicpathdetector): DefaultCollapseConfig accessor (defensive copy) CodeRabbit upstream PR kubescape#323 finding #3. The package-level `DefaultCollapseConfig` was an exported mutable var: any caller doing `dynamicpathdetector.DefaultCollapseConfig.Threshold = 1` would silently corrupt every analyzer constructed thereafter. The threshold surface is security-sensitive (governs how aggressively profile paths collapse into wildcards), so accidental cross-caller leakage is a real risk. Replaces the exported var with an unexported `defaultCollapseConfig` and an accessor `DefaultCollapseConfig()` that returns a value copy. Pattern mirrors the existing `defaultCollapseConfigs` / `DefaultCollapseConfigs()` defensive-copy accessor for the slice form. Updates 6 callsites across the test suite to use the accessor. Adds TestDefaultCollapseConfig_DefensiveCopy pinning the contract: a caller mutating the returned struct does not affect subsequent reads. * docs(dynamicpathdetector): comment + guard kubescape#323 nitpicks (#5, #6, #9, #10) Four small docstring / defensive-guard improvements from CodeRabbit upstream PR kubescape#323: #5 wildcardKey / getEndpointKey format coupling — extract buildEndpointKey as the single source of truth for the lookup key shape. Both call-sites now route through it, removing the "must stay in sync" risk that allowed past drift. New test TestBuildEndpointKey_SharedFormat pins end-to-end behaviour via MergeDuplicateEndpoints: a wildcard-port entry and a specific- port sibling collapse into one row with merged Methods. #6 consolidateOpens O(n²) — acknowledged in the function's docstring along with the threshold (≤ ~10k entries) above which a trie-based replacement would be worthwhile. Behaviour unchanged. #9 removeEndpoint in-place mutation — added a NOTE block in the docstring describing the backing-array shift, why the sole caller is safe today, and the swap-to-copy guidance for any future caller that stores intermediate slice references. #10 hasPrefixAtBoundary("") edge — added an explicit empty-prefix branch returning true. Without it the function fell through to pathPrefix[0] == '/', which is true for any absolute path — effectively treating "" as a root-matching prefix incidentally. The explicit branch makes the invariant load-bearing. New test TestHasPrefixAtBoundary_EmptyPrefix pins the surface via FindConfigForPath. All existing tests still pass. Local run on this arm64 machine: go test ./pkg/registry/file/... ok in 0.93s * deps: bump syft to v1.42.4 (CVE-2026-33481), stereoscope v0.1.22, runtime-spec v1.3.0 CodeRabbit upstream PR kubescape#326 finding #1 (Critical). Lifts syft from v1.32.0 to v1.42.4 — the lowest version that includes the CVE-2026-33481 fix (Syft was failing to clean up temporary storage when a scan errored mid-flight). v1.42.4 is the latest patch on the v1.42 line, so the upgrade gets the CVE fix without taking the v1.43/v1.44 minor bumps' larger surface-area changes. go mod tidy pulled forward two transitive bumps: - stereoscope v0.1.9 → v0.1.22 (Anchore's syft-companion lib, must move in lockstep with syft) - runtime-spec v1.2.1 → v1.3.0 (transitive via stereoscope) - modernc.org/libc v1.67.6 → v1.70.0 (transitive) - modernc.org/sqlite v1.46.1 → v1.46.2 (transitive) go.mod no longer carries a replace directive against kubescape/syft — our fork doesn't have v1.42.3+ tags and the upstream content is required for the CVE fix. Direct anchore/syft dependency is now the source of truth. Local validation: go build ./... ok go test ./pkg/... ok in 1.4s (all 16 packages green) Vendor directory is .gitignored; `go mod vendor` was rerun locally to keep build/manifest in sync. * fix: address 3 follow-up rabbit findings on PR #33 1. ci(manual-integration-tests): use job-level if: false instead of exit 1 — workflow shows as "skipped" not "failed red X" in the Actions UI when dispatched, removing the cosmetic noise while keeping the ~30-minute waste prevention. Issue #32 reference kept in the YAML comment above the conditional. 2. test(network_types_protobuf): also assert the Go struct's protobuf tag, not just the .proto-file text. The earlier test only checked one side of the schema/tag contract, so tag drift on the Go side would silently pass. Now both halves are pinned. 3. test(applicationprofile_processor): assert the FULL CollapseSettings struct (not just OpenDynamicThreshold). Without this, a regression that resets any field added to CollapseSettings in the future would silently pass the zero-value defensive-fallback test. All from CodeRabbit follow-up review on storage PR #33. Local validation: go test ./pkg/registry/file/ ./pkg/apis/softwarecomposition/v1beta1/ → both packages ok * docs(networkmatch): correct spec URL to billofbehavior.com --------- Co-authored-by: Entlein <eineintlein@gmail.com> --------- Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-authored-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-authored-by: Entlein <eineintlein@gmail.com>
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.
Iterative branch addressing all 6 CodeRabbit findings on the upstream PR
kubescape/storage#326. Each fix lands via TDD: failing regression test first, then minimal code change, then local benchmark.Findings tracked
analyzer.go:419-433— DP-memoisecompareSegments(commit 7d19023)tests/analyze_endpoints_test.go:169-178— length assertion before indexed comparisonstests/compare_exec_args_test.go:313— harden 100ms timing assertion undertesting.Short()applicationprofile_processor.go:92— nil-guardcollapseSettingspkg/apis/softwarecomposition/v1beta1/generated.proto:177— addrepeated string ipAddresses = 9;to NetworkNeighborgo.mod:9— bump syft tov1.42.3+for CVE-2026-33481Also in this branch
ci/manual-integration-tests: workflow disabled via fail-fast guard step pointing at issue Manual Integration Tests workflow disabled: flaky learning-period timing across all ProfileCreate variants #32 (the test suite has been flaky on upstream-wide runs since at least 2026-01; full inventory in the issue).Local benchmark per commit
Each fix commit includes before/after numbers from
go test -benchon this local arm64 machine, so review can spot perf regressions without waiting for GH-runner variance.CodeRabbit
Self-trigger pattern:
@coderabbitai reviewafter each push so the rabbit has a chance to flag anything missed by the targeted tests.@coderabbitai review