feat(test): add director-agent acl rotation correctness driver#26
Merged
Conversation
WalkthroughAdds support for ChangesACL rotation correctness
Sequence Diagram(s)sequenceDiagram
participant Runner
participant Director
participant Receiver
participant Store
Runner->>Director: stage writable ACL config in tmpDir
Runner->>Director: rewrite acl.allowed_ips
Director->>Receiver: apply ACL change to delivery
Runner->>Receiver: sample line counts for recover or revoke
Runner->>Store: save RunResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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: 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 `@internal/config/case.go`:
- Around line 904-915: The ACL rotation validation in tc.ACLRotation needs to
also constrain acl_rotation.to_config to a safe child directory name, since it
is later joined into a config path and could otherwise escape the case config
directory. Add validation alongside the existing
ACLRotationExpect/SettleSeconds/BaselineSeconds checks in the case validation
logic so values like ../other are rejected before the runner uses them.
In `@internal/runner/runner.go`:
- Around line 2419-2437: The rotated-config validation in runner.go only reads
the rotated file but does not verify that it differs from the initial config
solely in acl.allowed_ips. Update the logic around the initialSrc/rotatedSrc
comparison in the runner flow to parse both YAML configs, remove acl.allowed_ips
from each, and compare the normalized documents before any containers start. If
any other field differs, fail early with a clear error so the ACL rotation case
stays isolated to that single setting.
- Around line 2680-2684: The post-revocation check in runner.go is treating a
negative `advanced` value from `rmAfter2.LinesReceived - rmAfter1.LinesReceived`
as a pass because it only compares against `aclLeak`. Update the stop-verdict
logic around `advanced` and `stopped` in the revocation evaluation path to
require a non-negative delta before acceptance, and treat any receiver counter
regression or reset as a failure case. Keep the existing `rmAfter1`, `rmAfter2`,
`finalCount`, and `formatCount` reporting, but ensure the verdict is only true
when `advanced` is at least zero and within the allowed leak threshold.
🪄 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: b8a89a55-d4dd-4561-9cd1-53366f8c83a0
📒 Files selected for processing (2)
internal/config/case.gointernal/runner/runner.go
Deploying pipebench with
|
| Latest commit: |
3256b96
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://03940665.pipebench.pages.dev |
| Branch Preview URL: | https://dt-807.pipebench.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/config/case.go`:
- Around line 907-909: The validation in tc.ACLRotation.AllowedIPs only checks
for a non-empty slice, so blank entries like empty strings still pass and later
get written into the director config. Update the case validation in the ACL
rotation path to reject any empty or whitespace-only entries in
tc.ACLRotation.AllowedIPs, and return a validation error from the same
config-checking code that currently handles the non-empty slice check.
🪄 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: bb0fa736-d27c-4f5d-b0d3-7aef0372e55c
📒 Files selected for processing (3)
internal/config/case.gointernal/runner/acl_rotation_test.gointernal/runner/runner.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runner/runner.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/config/acl_rotation_test.go (1)
27-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining
validateACLRotation()branches in this table.This suite skips several new validator paths from
internal/config/case.go:886-937: missingacl_rotation, generator-present rejection, and negativesettle_seconds/baseline_seconds. Since this file is the dedicated regression surface for that validator, those branches can now drift without any test failing.Possible additions
tests := []struct { name string mutate func(tc *TestCase) wantErr string // substring of the expected error, "" = valid }{ {name: "valid recover", mutate: func(*TestCase) {}}, + { + name: "valid revoke", + mutate: func(tc *TestCase) { tc.ACLRotation.Expect = ACLRotationRevoke }, + }, + { + name: "missing acl_rotation block", + mutate: func(tc *TestCase) { tc.ACLRotation = nil }, + wantErr: "requires an `acl_rotation:` block", + }, { name: "empty allowed_ips", mutate: func(tc *TestCase) { tc.ACLRotation.AllowedIPs = nil }, wantErr: "allowed_ips must list", }, @@ { + name: "generator not allowed", + mutate: func(tc *TestCase) { + tc.Generator = GeneratorConfig{Mode: "otlp"} + }, + wantErr: "must not declare a generator", + }, + { + name: "negative settle_seconds", + mutate: func(tc *TestCase) { tc.ACLRotation.SettleSeconds = -1 }, + wantErr: "settle_seconds must be >= 0", + }, + { + name: "negative baseline_seconds", + mutate: func(tc *TestCase) { tc.ACLRotation.BaselineSeconds = -1 }, + wantErr: "baseline_seconds must be >= 0", + }, + { name: "acl_rotation on wrong type", mutate: func(tc *TestCase) { tc.Type = "correctness" }, wantErr: "only valid for type",🤖 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/config/acl_rotation_test.go` around lines 27 - 70, Extend the acl_rotation validator table in acl_rotation_test.go to cover the remaining validateACLRotation() branches that are currently untested. Add cases for a missing ACLRotation block, rejection when a generator is present, and negative settle_seconds/baseline_seconds values, using the existing TestCase mutation pattern so the checks exercise validateACLRotation() directly.
🤖 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.
Nitpick comments:
In `@internal/config/acl_rotation_test.go`:
- Around line 27-70: Extend the acl_rotation validator table in
acl_rotation_test.go to cover the remaining validateACLRotation() branches that
are currently untested. Add cases for a missing ACLRotation block, rejection
when a generator is present, and negative settle_seconds/baseline_seconds
values, using the existing TestCase mutation pattern so the checks exercise
validateACLRotation() directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d8ee385-5382-44b8-bcb6-4d9e2c19083d
📒 Files selected for processing (2)
internal/config/acl_rotation_test.gointernal/config/case.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/case.go
yusufozturk
approved these changes
Jun 26, 2026
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
2 participants
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 by CodeRabbit
acl_rotationconfiguration for director↔agent ACL rotation correctness cases, includingrecover/revokeexpectations and timing controls.