fix: [CODE-5062] Pre-Receive hook should not consider bypass flag for push rules

[abhinavcode]

Summary by CodeRabbit

• Bug Fixes
  • Updated protection rule handling to prevent violation bypasses, ensuring all violations are consistently enforced and cannot be circumvented.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change removes dynamic bypass matching logic from the violation verification process in rule push handling, replacing it with hard-coded false values that mark all violations as non-bypassable, effectively disabling the bypass feature entirely.

Changes

Cohort / File(s)	Summary
Bypass Logic Simplification app/services/protection/rule_push.go	Replaced dynamic bypass calculation in PushVerify and Violations functions with hard-coded false values; removed calls to p.Bypass.matches and per-violation ByPassable/Bypassed assignments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 No more sneaking through, the bypass is through!
Hard-coded false, a security-conscious move,
Violations stand tall, no escape hatch to groove,
The rules are the rules now—simple and true! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: disabling bypass logic for push rules in the pre-receive hook by replacing dynamic bypass calculation with hard-coded false.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch atmsn/CODE-5062

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

[coderabbitai]

🧹 Nitpick comments (2)

app/services/protection/rule_push.go (2)

89-90: Redundant explicit zero-value assignments.

var violations types.RuleViolations already initialises Bypassable and Bypassed to false (Go zero value). Lines 89–90 are no-ops that serve only as intent documentation. If the goal is self-documentation, a brief comment (// bypass intentionally disabled for pre-receive) would communicate this more clearly than redundant assignments; alternatively, they can be removed entirely.

♻️ Suggested change

-	violations.Bypassable = false
-	violations.Bypassed = false
+	// Bypass is intentionally disabled for the pre-receive hook push rules (CODE-5062).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/services/protection/rule_push.go` around lines 89 - 90, The explicit
assignments setting violations.Bypassable = false and violations.Bypassed =
false are redundant because var violations types.RuleViolations already
zero-initializes those fields; remove those two assignments from rule_push.go
(or replace them with a single clarifying comment such as "// bypass
intentionally disabled for pre-receive" next to the violations declaration) so
the code avoids no-op statements while preserving intent around the violations
variable.

---

28-31: Misleading bypass configuration: configurable but silently ignored.

Bypass DefBypass (with its json:"bypass" tag) remains fully part of the Push rule's serialized schema. Sanitize() still validates it, and UserIDs() / UserGroupIDs() still return its contents. Now that bypass is hardcoded to disabled, any bypass list a user configures for a push rule will be accepted, validated, stored — and silently never applied.

Consider one of:

• Removing Bypass from Push entirely (breaking change to the schema, requires a migration).
• Returning empty slices from UserIDs() / UserGroupIDs() so the framework does not pre-load unnecessary bypass data.
• At minimum, adding a comment on the field documenting that bypass is disabled for pre-receive push rules.

♻️ Minimal non-breaking mitigation (return empty from UserIDs/UserGroupIDs)

 func (p *Push) UserIDs() ([]int64, error) {
-	return p.Bypass.UserIDs, nil
+	// Bypass is disabled for pre-receive push rules (CODE-5062).
+	return nil, nil
 }

 func (p *Push) UserGroupIDs() ([]int64, error) {
-	return p.Bypass.UserGroupIDs, nil
+	// Bypass is disabled for pre-receive push rules (CODE-5062).
+	return nil, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/services/protection/rule_push.go` around lines 28 - 31, The Push struct
exposes a Bypass field that is serialized/validated but is ignored at runtime;
to avoid silently accepting unused bypass data, update the Push.UserIDs() and
Push.UserGroupIDs() methods to always return empty slices (and nil error) so the
framework won't preload bypass info, and add a comment on the Push.Bypass field
noting that bypass is disabled for pre-receive push rules; optionally, also
adjust Push.Sanitize() to skip/clear validation of Bypass to avoid
storing/returning invalid-but-ignored bypass data.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@app/services/protection/rule_push.go`:
- Around line 89-90: The explicit assignments setting violations.Bypassable =
false and violations.Bypassed = false are redundant because var violations
types.RuleViolations already zero-initializes those fields; remove those two
assignments from rule_push.go (or replace them with a single clarifying comment
such as "// bypass intentionally disabled for pre-receive" next to the
violations declaration) so the code avoids no-op statements while preserving
intent around the violations variable.
- Around line 28-31: The Push struct exposes a Bypass field that is
serialized/validated but is ignored at runtime; to avoid silently accepting
unused bypass data, update the Push.UserIDs() and Push.UserGroupIDs() methods to
always return empty slices (and nil error) so the framework won't preload bypass
info, and add a comment on the Push.Bypass field noting that bypass is disabled
for pre-receive push rules; optionally, also adjust Push.Sanitize() to
skip/clear validation of Bypass to avoid storing/returning invalid-but-ignored
bypass data.