fix: correct operator precedence in isDestinationMatched deny pattern

[magic-peach]

Summary

Fixed an operator precedence bug in isDestinationMatched at pkg/apis/application/v1alpha1/app_project_types.go:519.

Problem

Due to && binding tighter than ||, the && dstNamespaceMatched condition only applied to the server deny pattern, not the name deny pattern:

// BEFORE (broken)
case (!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server)) && dstNamespaceMatched:
// Evaluates as:
// ((name deny) || (server deny && namespace))

Name-based deny patterns (like !prod-*) rejected destinations regardless of namespace, while server-based deny patterns (like !https://staging) correctly required namespace match first. This broke symmetry with the match logic on line 515:

matched := (dstServerMatched || dstNameMatched) && dstNamespaceMatched

Fix

Added parentheses to enforce intended grouping:

// AFTER (fixed)
case ((!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server))) && dstNamespaceMatched:

Both name and server deny patterns now correctly require namespace match, consistent with the match logic.

[bunnyshell]

❗ Preview Environment deployment failed on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

• 🚀 /bns:deploy to redeploy the environment
• ❌ /bns:delete to remove the environment

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[qodo-code-review]

PR Summary by Qodo

Fix deny-pattern operator precedence in isDestinationMatched

🐞 Bug fix 🕐 10-20 Minutes

---

AI Description

• Fix deny-list destination evaluation to require namespace match for name/server patterns.
• Restore symmetry between allow-match and deny-match logic in project destinations.

Diagram

graph TD
  A(["Destination check"]) --> B["isDestinationMatched"] --> C["Compute match flags"] --> D{"matched?"}
  D -->|"yes"| E(["allow (continue)"])
  D -->|"no"| F{"deny pattern?"}
  F -->|"yes"| G(["return false"])
  F -->|"no"| H(["evaluate next item"])
  subgraph Legend
    direction LR
    _startend(["Start/End"]) ~~~ _proc["Process"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Refactor boolean into named predicates

• ➕ Reduces risk of future precedence bugs by making grouping explicit via intermediate variables/functions
• ➕ Improves readability and testability of complex match/deny conditions
• ➖ More code churn for a small bug fix
• ➖ May require broader review to ensure semantics remain unchanged

2. Add targeted unit tests for deny patterns

• ➕ Prevents regressions for operator-precedence and namespace-gating behavior
• ➕ Documents intended deny semantics (name/server requires namespace match)
• ➖ Requires creating/expanding test harness for AppProject destination matching
• ➖ Slightly increases PR scope beyond the immediate fix

Recommendation: The current minimal parenthesis fix is the best immediate change: it corrects semantics with negligible churn. Consider a follow-up to add unit tests (and optionally refactor into named predicates) to prevent similar precedence regressions.

Files changed (1) +1 / -1

Bug fix (1) +1 / -1

app_project_types.goFix deny pattern grouping to require namespace match +1/-1

Fix deny pattern grouping to require namespace match

• Adds parentheses so 'dstNamespaceMatched' gates both name-based and server-based deny patterns. This restores intended operator grouping and aligns deny logic with the existing allow-match condition.

pkg/apis/application/v1alpha1/app_project_types.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 15 rules

---

🟡 Remediation Recommended

1. isDestinationMatched change lacks tests ✓ Resolved 📘 ▣ Testability

Description

The PR adjusts deny-pattern operator precedence in AppProject.isDestinationMatched so name/server
deny patterns only apply when dstNamespaceMatched is true, but it does not add or update tests to
lock in this behavior. This leaves a key regression scenario—name-based deny matching with a
non-matching namespace—unprotected and increases risk in destination allow/deny evaluation.

Code

pkg/apis/application/v1alpha1/app_project_types.go[519]

+		case ((!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server))) && dstNamespaceMatched:

Relevance

⭐⭐ Medium

Mixed history: bugfix PRs often add tests (#27189,#28318), but Codecov “add tests” requests get
rejected (#28720).

PR-#27189
PR-#28318
PR-#28720

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2230598 requires tests for behavior-changing code changes, and the PR changes the
deny-pattern condition in isDestinationMatched by gating both name- and server-based deny checks
on dstNamespaceMatched. This means a name deny should not trigger when the destination’s namespace
does not match the deny entry’s namespace, yet existing tests only cover name negation when
Namespace: "*" (so namespace always matches) and a server-deny case where the deny destination’s
namespace does not match, leaving no regression test for the previously buggy case of name-deny +
namespace-mismatch.

Rule 2230598: Require tests for behavior-changing code changes
pkg/apis/application/v1alpha1/app_project_types.go[515-522]
pkg/apis/application/v1alpha1/types_test.go[247-312]
pkg/apis/application/v1alpha1/app_project_types.go[507-523]
pkg/apis/application/v1alpha1/types_test.go[301-312]
pkg/apis/application/v1alpha1/types_test.go[383-389]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A behavior-changing fix was made to `AppProject.isDestinationMatched()` to correct deny-pattern operator precedence so name/server deny patterns only deny when `dstNamespaceMatched` is true, but there is no automated regression test covering the exact previously broken scenario: a **name deny pattern matches** while the **namespace pattern does not match**.

## Issue Context
The new logic requires `dstNamespaceMatched` to apply to both name-based and server-based deny patterns; without a regression test, the precedence bug could reappear unnoticed. Existing tests validate name negation only with `Namespace: "*"` (so the deny always applies because namespace always matches) and validate server-deny behavior when the deny destination’s namespace does not match the app namespace, but they do not include an analogous case ensuring a name-based deny entry does **not** deny when its namespace constraint does not match.

Suggested test case to add (e.g., in `TestAppProject_IsNegatedDestinationPermitted`):
- Project destinations:
 1) allow: `{Name: "*", Namespace: "*"}`
 2) deny-by-name in a specific namespace: `{Name: "!bad", Namespace: "other"}`
- App destination: `{Name: "bad", Namespace: "test"}`
- Expected: `isPermitted: true`

## Fix Focus Areas
- pkg/apis/application/v1alpha1/app_project_types.go[507-523]
- pkg/apis/application/v1alpha1/types_test.go[247-415]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

---

Powered by Qodo

[nitishfy]

The changes looks good to me. Can you add some tests to verify these changes?

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[codecov]

Bundle Report

Bundle size has no change ✅

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.