Allow setup.sh in case we need to run some script in docker image#427
Allow setup.sh in case we need to run some script in docker image#427
Conversation
WalkthroughThe ignore rules logic was refined to properly handle negation patterns by evaluating all patterns before deciding to ignore a path. An explicit negation rule for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15-20 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/ignore/rules.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
|
@jhaynie maybe we should close this and update the agentuity.yaml template: and add |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/ignore/rules.go (2)
95-99: Scope of!setup.shmay be too broad
!setup.shmatches any file whose base-name issetup.sh, even under ignored directories such asnode_modules/or.git/.
If the intent is to whitelist only the project-root script, consider anchoring the rule:- r.parseRule("!setup.sh") + r.parseRule("!/setup.sh") // root only + r.parseRule("!./setup.sh") // or explicit relative formor combine it with a sibling wildcard negation (e.g.
!*/setup.sh) depending on the desired scope.
155-210: Re-evaluation loop now O(N) for every pathSwitching from “first-match-wins” to full pass through all patterns fixes negation handling, but it also means each
Ignore()call now iterates the entire rules slice and allocates no early exit on a positive match (except for negations).For large manifests this can become a hot path. A small optimisation:
if p.match(path, fi) { if fullWildcard { return true } // unchanged if !hasNegationsAfter(n) { return true } matched = true }where
hasNegationsAfteris a cheap boolean pre-computed once when buildingRules, would restore early-exit behaviour without breaking the new semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/ignore/rules.go(3 hunks)internal/ignore/rules_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/ignore/rules_test.go (1)
internal/ignore/rules.go (2)
Ignore(40-40)Empty(51-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/ignore/rules_test.go (2)
39-39: Add a sibling-directory test for setup.shThe new assertion only checks a root-level
setup.sh.
Because the rule added inAddDefaults()is!setup.sh(no path prefix), it will also un-ignoresetup.shlocated deep inside dependency folders (e.g.node_modules/setup.sh).
Add a second assertion such as:assert.True(t, rules.Ignore("/Users/foobar/example/node_modules/setup.sh", nil))to lock in the intended scope (root-only vs. anywhere) and avoid future surprises.
45-46: Good negation-ordering regression testReversing the add order makes the test guard against the previous “first-match-wins” bug.
No further action needed.
2 participants
The
setup.shfile is a valid setup file, currently being ignored by the bundler.Summary by CodeRabbit