internal/authn/preshared/authn_test.go (1)

41-44: Make assertions less brittle and assert returned authn is nil.

Avoid exact string comparison on error messages and also verify the returned authenticator is nil for stronger guarantees.

Apply:

-               _, err := NewKeyAuthn(context.Background(), emptyConfig)
-               Expect(err).To(HaveOccurred())
-               Expect(err.Error()).To(Equal("pre shared key authn must have at least one key"))
+               auth, err := NewKeyAuthn(context.Background(), emptyConfig)
+               Expect(err).To(HaveOccurred())
+               Expect(auth).To(BeNil())
+               Expect(err).To(MatchError(ContainSubstring("at least one key")))

-               _, err := NewKeyAuthn(context.Background(), nilConfig)
-               Expect(err).To(HaveOccurred())
-               Expect(err.Error()).To(Equal("pre shared key authn must have at least one key"))
+               auth, err := NewKeyAuthn(context.Background(), nilConfig)
+               Expect(err).To(HaveOccurred())
+               Expect(auth).To(BeNil())
+               Expect(err).To(MatchError(ContainSubstring("at least one key")))

Also applies to: 49-53

internal/storage/postgres/postgres_test.go (2)

4-4: Remove sort import if helper is deleted

If you switch to ConsistOf, drop the now-unused import.

-import (
-	"sort"
-	"testing"
+import (
+	"testing"

---

16-37: Replace isSameArray in postgres tests with Gomega’s ConsistOf and delete the test helper

In internal/storage/postgres/postgres_test.go, remove the local isSameArray helper and update assertions to:

Expect(actual).To(ConsistOf(expected))

Production still defines its own isSameArray in pkg/development/development.go and pkg/cmd/validate.go, so this change is limited to tests. Optionally consolidate the duplicate helpers in those packages into a shared utility.

internal/storage/postgres/utils/filter_test.go (6)

84-98: Re-initialize the builder between assertions to avoid state leakage within the same test.

The second scenario reuses the previously mutated sl. Recreate the builder to keep cases independent.

-			sl = utils.TuplesFilterQueryForSelectBuilder(sl, filter)
+			// re-init builder for the second scenario
+			sl = squirrel.Select("*").From("relation_tuples")
+			sl = utils.TuplesFilterQueryForSelectBuilder(sl, filter)

---

101-101: Disambiguate duplicate context name.

Two contexts are named “TestEmptyFilterQueryForSelectBuilder”. Rename this one to clarify it targets attributes.

-	Context("TestEmptyFilterQueryForSelectBuilder", func() {
+	Context("TestEmptyAttributeFilterQueryForSelectBuilder", func() {

---

146-160: Re-initialize update builder before second scenario to prevent state carry-over.

Same pattern here; recreate sl before the second call.

-			sl = utils.TuplesFilterQueryForUpdateBuilder(sl, filter)
+			// re-init builder for the second scenario
+			sl = squirrel.Update("relation_tuples").Set("expired_tx_id", 1000)
+			sl = utils.TuplesFilterQueryForUpdateBuilder(sl, filter)

---

208-215: Re-initialize attributes update builder before second scenario (consistency).

-			sl = utils.AttributesFilterQueryForUpdateBuilder(sl, filter)
+			// re-init builder for the second scenario
+			sl = squirrel.Update("attributes").Set("expired_tx_id", 1000)
+			sl = utils.AttributesFilterQueryForUpdateBuilder(sl, filter)

---

454-468: Assert args for single-attribute UPDATE as well.

Keeps UPDATE tests consistent with SELECT tests.

-				sql, _, err := sl.ToSql()
+				expectedArgs := []interface{}{1000, "single_attribute"}
+				sql, args, err := sl.ToSql()
 				Expect(err).ShouldNot(HaveOccurred())
+				Expect(args).Should(Equal(expectedArgs))
 				Expect(sql).Should(Equal(expectedSql))

---

492-731: Add missing nil-ID/attribute tests and subject_relation-only scenarios

• Add TuplesFilterQueryForUpdateBuilder tests for nil entity and nil subject IDs (to mirror SELECT nil cases).
• Add AttributesFilterQueryForUpdateBuilder test for nil attributes (parity with SELECT nil‐attribute case).
• Add tests covering subject_relation used alone and combined with a single subject ID.
• Since builders use a multi-key squirrel.Eq map, full-SQL assertions must lock clause order (or refactor builders to sequential Where calls for deterministic ordering).

internal/storage/postgres/utils/version_test.go (3)

3-14: Add strings import (used by env gating in multi-version test below).

Preparing for safer multi-version gating needs strings.

 import (
   "os"
   "strconv"
+  "strings"

---

37-71: Consolidate redundant assertions into a single spec.

The first four specs repeat the same setup and assertions. Merge for speed and clarity, and add a quick sanity check on idempotency.

-    It("should return version for supported PostgreSQL version", func() {
-      version, err := utils.EnsureDBVersion(writePool)
-      Expect(err).ShouldNot(HaveOccurred())
-      Expect(version).ShouldNot(BeEmpty())
-      versionNum, parseErr := strconv.Atoi(version)
-      Expect(parseErr).ShouldNot(HaveOccurred())
-      Expect(versionNum).Should(BeNumerically(">=", 130008))
-    })
-
-    It("should return version string that can be parsed as integer", func() {
-      version, err := utils.EnsureDBVersion(writePool)
-      Expect(err).ShouldNot(HaveOccurred())
-      Expect(version).ShouldNot(BeEmpty())
-      versionNum, parseErr := strconv.Atoi(version)
-      Expect(parseErr).ShouldNot(HaveOccurred())
-      Expect(versionNum).Should(BeNumerically(">", 0))
-    })
-
-    It("should return version that meets minimum requirements", func() {
-      version, err := utils.EnsureDBVersion(writePool)
-      Expect(err).ShouldNot(HaveOccurred())
-      Expect(version).ShouldNot(BeEmpty())
-      versionNum, parseErr := strconv.Atoi(version)
-      Expect(parseErr).ShouldNot(HaveOccurred())
-      Expect(versionNum).Should(BeNumerically(">=", 130008))
-    })
-
-    It("should handle database connection properly", func() {
-      version, err := utils.EnsureDBVersion(writePool)
-      Expect(err).ShouldNot(HaveOccurred())
-      Expect(version).ShouldNot(BeEmpty())
-      Expect(version).Should(BeAssignableToTypeOf(""))
-    })
-
-    It("should return consistent results on multiple calls", func() {
-      version1, err1 := utils.EnsureDBVersion(writePool)
-      version2, err2 := utils.EnsureDBVersion(writePool)
-      Expect(err1).ShouldNot(HaveOccurred())
-      Expect(err2).ShouldNot(HaveOccurred())
-      Expect(version1).Should(Equal(version2))
-    })
+    It("returns a numeric server_version_num meeting minimum and is idempotent", func() {
+      v1, err := utils.EnsureDBVersion(writePool)
+      Expect(err).ShouldNot(HaveOccurred())
+      Expect(v1).ShouldNot(BeEmpty())
+      n, perr := strconv.Atoi(v1)
+      Expect(perr).ShouldNot(HaveOccurred())
+      Expect(n).Should(BeNumerically(">=", 130008)) // 13.8 minimum
+
+      v2, err2 := utils.EnsureDBVersion(writePool)
+      Expect(err2).ShouldNot(HaveOccurred())
+      Expect(v2).Should(Equal(v1))
+    })

Also applies to: 73-80, 82-90

---

131-156: Reuse the suite’s writePool instead of creating another container; also tighten format assertions.

Avoids an extra container spin and keeps the spec fast. Assert 6-digit length explicitly.

-      version := os.Getenv("POSTGRES_VERSION")
-      if version == "" {
-        version = "14"
-      }
-      database := instance.PostgresDB(version)
-      db := database.(*PQDatabase.Postgres)
-      writePool := db.WritePool
-      versionStr, err := utils.EnsureDBVersion(writePool)
-      Expect(err).ShouldNot(HaveOccurred())
-      versionNum, parseErr := strconv.Atoi(versionStr)
-      Expect(parseErr).ShouldNot(HaveOccurred())
-      Expect(versionNum).Should(BeNumerically(">=", 100000))
-      Expect(versionNum).Should(BeNumerically("<=", 999999))
-      err = db.Close()
-      Expect(err).ShouldNot(HaveOccurred())
+      versionStr, err := utils.EnsureDBVersion(writePool)
+      Expect(err).ShouldNot(HaveOccurred())
+      versionNum, parseErr := strconv.Atoi(versionStr)
+      Expect(parseErr).ShouldNot(HaveOccurred())
+      // 6 digits (MMmmpp) for server_version_num >= 10.0.0
+      Expect(len(versionStr)).To(Equal(6))
+      Expect(versionNum).Should(BeNumerically(">=", 100000))
+      Expect(versionNum).Should(BeNumerically("<=", 999999))

internal/info.go (1)

26-26: Version sync verified
Swagger JSON docs and proto openapi version both match “v1.4.4”. The only “v1.4.3” hits are indirect dependencies (github.com/go-logr/logr) in go.mod/go.sum, not the service version.
Optional: switch to build-time injection of Version via ldflags to prevent future drift.

internal/schema/linkedSchema_test.go (1)

1448-1449: Consider documenting why these tests are skipped.

While the skip messages explain the difficulty, it would be helpful to add TODO comments indicating whether these edge cases should be tested through other means (e.g., mocking, test helpers).

-			Skip("Skipping test for undefined child type - difficult to trigger with normal schema compilation")
+			// TODO: Consider adding test helpers to create malformed schema structures for these edge cases
+			Skip("Skipping test for undefined child type - difficult to trigger with normal schema compilation")

Also applies to: 1456-1457, 1464-1465

internal/storage/postgres/schemaReader_test.go (1)

255-259: Consider extracting common error assertion logic.

The error assertion pattern is repeated many times. Consider creating a helper function to reduce duplication.

Add a helper function at the beginning of the test file:

func expectDatabaseError(err error) {
	Expect(err).Should(HaveOccurred())
	Expect(err.Error()).Should(Or(
		Equal(base.ErrorCode_ERROR_CODE_SQL_BUILDER.String()),
		Equal(base.ErrorCode_ERROR_CODE_EXECUTION.String()),
		Equal(base.ErrorCode_ERROR_CODE_SCAN.String()),
	))
}

Then simplify the assertions:

-				Expect(err).Should(HaveOccurred())
-				Expect(err.Error()).Should(Or(
-					Equal(base.ErrorCode_ERROR_CODE_SQL_BUILDER.String()),
-					Equal(base.ErrorCode_ERROR_CODE_EXECUTION.String()),
-					Equal(base.ErrorCode_ERROR_CODE_SCAN.String()),
-				))
+				expectDatabaseError(err)

Also applies to: 274-278, 293-297, 312-317, 334-338, 353-357, 372-376

internal/storage/postgres/tenantWriter_test.go (1)

99-116: Duplicate test descriptions could be more specific.

Several tests have similar descriptions but test different error paths. Consider making the test names more specific to indicate which exact error path is being tested.

-			It("should handle transaction begin error", func() {
+			It("should handle transaction begin error when starting delete operation", func() {

-			It("should handle batch execution error", func() {
+			It("should handle batch execution error during delete operations", func() {

-			It("should handle commit error after batch execution", func() {
+			It("should handle transaction commit error after successful batch execution", func() {

Also applies to: 118-135, 137-154, 156-173, 175-192, 194-211

internal/storage/postgres/dataWriter_test.go (1)

545-548: Consider creating a test helper for common error assertions.

Similar to the suggestion for schemaReader_test.go, the datastore error assertions are repeated frequently.

Add a helper function:

func expectDatastoreOrRetryError(err error) {
	Expect(err).Should(HaveOccurred())
	Expect(err.Error()).Should(Or(
		Equal(base.ErrorCode_ERROR_CODE_DATASTORE.String()),
		Equal(base.ErrorCode_ERROR_CODE_ERROR_MAX_RETRIES.String()),
	))
}

Then use it to simplify assertions:

-				Expect(err).Should(HaveOccurred())
-				// The error could be various types depending on when the connection fails
-				Expect(err.Error()).Should(Or(
-					Equal(base.ErrorCode_ERROR_CODE_DATASTORE.String()),
-					Equal(base.ErrorCode_ERROR_CODE_ERROR_MAX_RETRIES.String()),
-				))
+				expectDatastoreOrRetryError(err)

Also applies to: 569-572, 593-596, 617-620, 641-644

internal/storage/postgres/gc/gc_test.go (2)

361-366: Prefer errors.Is for context cancellation assertion

Use errors.Is to be resilient to wrapped errors from Start.

-			// Start the garbage collector with the cancelled context
-			err := garbageCollector.Start(ctx)
-
-			Expect(err).Should(HaveOccurred())
-			Expect(err).Should(Equal(context.Canceled))
+			// Start the garbage collector with the cancelled context
+			err := garbageCollector.Start(ctx)
+			Expect(errors.Is(err, context.Canceled)).To(BeTrue())

Add the import:

import "errors"

---

368-587: DRY the “closed DB” setup and honor POSTGRES_VERSION

Each test re-creates and closes a brand-new Postgres container with a hard-coded "14", which is slow and ignores POSTGRES_VERSION. Factor a small helper that reads the env and returns a closed DB; then reuse it across the specs.

 Context("Error Handling", func() {
+    // Helper to provision a closed DB instance respecting POSTGRES_VERSION
+    getClosedDB := func() *PQDatabase.Postgres {
+        version := os.Getenv("POSTGRES_VERSION")
+        if version == "" {
+            version = "14"
+        }
+        d := instance.PostgresDB(version).(*PQDatabase.Postgres)
+        _ = d.Close()
+        return d
+    }
@@
-            // Create a garbage collector with a closed database connection
-            closedDB := instance.PostgresDB("14")
-            err := closedDB.Close()
-            Expect(err).ShouldNot(HaveOccurred())
-
-            badGC := NewGC(
-                closedDB.(*PQDatabase.Postgres),
+            badGC := NewGC(
+                getClosedDB(),
                 Window(5*time.Second),
             )

Apply the same pattern to the other tests in this context to remove duplication and container churn.

internal/storage/postgres/gc/options_test.go (1)

16-26: Avoid spinning a real Postgres for option setters

These tests only assert in-memory fields; using a real container slows CI and can flake. Use a lightweight stub Postgres instead and skip Close unless pools are non-nil.

-	BeforeEach(func() {
-		version := "14"
-		db = instance.PostgresDB(version).(*PQDatabase.Postgres)
-	})
+	BeforeEach(func() {
+		// No DB I/O needed for options; a zero-value stub is enough.
+		db = &PQDatabase.Postgres{}
+	})
@@
-	AfterEach(func() {
-		if db != nil {
-			err := db.Close()
-			Expect(err).ShouldNot(HaveOccurred())
-		}
-	})
+	AfterEach(func() {
+		// Close only if pools exist (when running locally against a real DB).
+		if db != nil && (db.ReadPool != nil || db.WritePool != nil) {
+			err := db.Close()
+			Expect(err).ShouldNot(HaveOccurred())
+		}
+	})

internal/storage/postgres/bundleReader_test.go (3)

140-149: Apply the same dedicated-closed-DB pattern here

Mirror the approach used above to avoid closing the shared db.

-			closedDB := db.(*PQDatabase.Postgres)
-			err := closedDB.Close()
-			Expect(err).ShouldNot(HaveOccurred())
+			version := os.Getenv("POSTGRES_VERSION")
+			if version == "" {
+				version = "14"
+			}
+			closedDB := instance.PostgresDB(version).(*PQDatabase.Postgres)
+			_ = closedDB.Close()

---

194-201: Repeat: isolate error injection from suite DB

Use a fresh closed instance rather than closing the shared one.

-			closedDB := db.(*PQDatabase.Postgres)
-			err := closedDB.Close()
-			Expect(err).ShouldNot(HaveOccurred())
+			version := os.Getenv("POSTGRES_VERSION")
+			if version == "" {
+				version = "14"
+			}
+			closedDB := instance.PostgresDB(version).(*PQDatabase.Postgres)
+			_ = closedDB.Close()

---

120-129: Use a dedicated closed DB instance for error injection
Closing the shared suite DB (db) can disrupt other specs. Instead, instantiate a fresh Postgres instance via instance.PostgresDB(version), close that, and pass it to NewBundleReader. Postgres.Close() unconditionally closes both pools and returns nil (idempotent).

-   // trigger SQL builder error on shared DB
-   closedDB := db.(*PQDatabase.Postgres)
-   err := closedDB.Close()
-   Expect(err).ShouldNot(HaveOccurred())
+   // trigger SQL builder error on a dedicated closed DB
+   version := os.Getenv("POSTGRES_VERSION")
+   if version == "" {
+       version = "14"
+   }
+   closedDB := instance.PostgresDB(version).(*PQDatabase.Postgres)
+   _ = closedDB.Close()

Ensure you import

"github.com/Permify/permify/internal/storage/postgres/instance"

internal/storage/postgres/dataReader_test.go (3)

496-506: Extract a helper for a closed DB and avoid closing the suite DB

Multiple specs close the shared db, which risks interfering with AfterEach or other specs. Add a small helper returning a dedicated closed DB (respecting POSTGRES_VERSION) and use it across the Error Handling context.

-	Context("Error Handling", func() {
+	Context("Error Handling", func() {
+		getClosedDB := func() *PQDatabase.Postgres {
+			version := os.Getenv("POSTGRES_VERSION")
+			if version == "" {
+				version = "14"
+			}
+			d := instance.PostgresDB(version).(*PQDatabase.Postgres)
+			_ = d.Close()
+			return d
+		}

---

501-507: Use the helper instead of closing the shared db

Replace ad-hoc close with getClosedDB() to isolate failure injection.

-				// Create a dataReader with a closed database to trigger errors
-				closedDB := db.(*PQDatabase.Postgres)
-				err := closedDB.Close()
-				Expect(err).ShouldNot(HaveOccurred())
-
-				readerWithClosedDB := NewDataReader(closedDB)
+				// Create a dataReader with a dedicated closed database
+				readerWithClosedDB := NewDataReader(getClosedDB())

Apply the same change to the other specs in this Error Handling context that close db directly.

---

982-1000: HeadSnapshot SQL/scan error tests: also use the helper

Keep the suite db open; inject failures via getClosedDB().

-				// Create a dataReader with a closed database to trigger SQL builder error
-				closedDB := db.(*PQDatabase.Postgres)
-				err := closedDB.Close()
-				Expect(err).ShouldNot(HaveOccurred())
-
-				readerWithClosedDB := NewDataReader(closedDB)
+				// Create a dataReader with a dedicated closed database
+				readerWithClosedDB := NewDataReader(getClosedDB())

internal/schema/walker_test.go (2)

206-212: Make error-code assertions resilient to wrapping

Direct string equality on err.Error() is brittle if implementations wrap errors. Prefer matching by substring or a small helper to assert codes.

Apply this pattern (example for Lines 210, 235, 309, 341, 437, 469):

-Expect(err.Error()).Should(Equal(base.ErrorCode_ERROR_CODE_ENTITY_DEFINITION_NOT_FOUND.String()))
+Expect(err).To(HaveOccurred())
+Expect(err.Error()).To(ContainSubstring(base.ErrorCode_ERROR_CODE_ENTITY_DEFINITION_NOT_FOUND.String()))

Optionally add a tiny helper at file bottom and replace all call sites:

func expectErrCode(err error, code base.ErrorCode) {
    Expect(err).To(HaveOccurred())
    Expect(err.Error()).To(ContainSubstring(code.String()))
}

Also applies to: 233-236, 308-310, 340-342, 436-438, 468-470

---

257-262: Compare sentinel errors with errors.Is

If ErrUnimplemented might be wrapped later, equality checks can start failing. Use errors.Is to future-proof.

Example:

-Expect(err).Should(Equal(ErrUnimplemented))
+Expect(errors.Is(err, ErrUnimplemented)).To(BeTrue())

Note: add errors import.

Also applies to: 361-368, 393-398

internal/authn/oidc/authn_test.go (1)

664-677: Skip rationale is fine; add link to code path for traceability (nit)

Optional: reference the exact function/lines in authn.go in the Skip message to aid future maintainers.

internal/storage/postgres/watch_test.go (1)

232-246: Error-message coupling is fragile; keep Substring but consider categorizing by phase

These getChanges error-path assertions depend on specific pgx messages ("encode text status undefined status"). If pgx changes text, tests will be noisy. Consider asserting higher-level phase (builder/exec/scan/unmarshal) via sentinel wrapping in production code, or at least grouping assertions with Or() for variations.

Also applies to: 248-262, 264-276, 278-291, 293-306, 308-321, 323-336

internal/storage/postgres/utils/common_test.go (6)

57-67: End spans to avoid leaks in longer runs

Add AfterEach to end the span created in BeforeEach.

Apply:

 		BeforeEach(func() {
 			ctx = context.Background()
 			_, span = noop.NewTracerProvider().Tracer("test").Start(ctx, "test-span")
 		})
+		AfterEach(func() {
+			span.End()
+		})

---

174-186: Tighten bounds to the known backoff window (reduce flakiness)

Base backoff for retries=0 is 20ms with jitter up to +10ms. Use bounds that reflect this.

Apply:

-			// Should have waited at least some time (backoff + jitter)
-			Expect(elapsed).Should(BeNumerically(">", 10*time.Millisecond))
-			// But not too long (should be less than our timeout)
-			Expect(elapsed).Should(BeNumerically("<", 90*time.Millisecond))
+			// 20ms <= elapsed < 100ms (with room for scheduler)
+			Expect(elapsed).Should(BeNumerically(">=", 18*time.Millisecond))
+			Expect(elapsed).Should(BeNumerically("<", 100*time.Millisecond))

---

188-198: Cancellation threshold is tight; relax to avoid CI jitter

Immediate select on ctx.Done() is near-instant but 10ms can be flaky on shared runners.

Apply:

-			Expect(elapsed).Should(BeNumerically("<", 10*time.Millisecond))
+			Expect(elapsed).Should(BeNumerically("<=", 30*time.Millisecond))

---

200-216: Strengthen assertion: higher retries must yield materially longer backoff

Given retries=2 ⇒ base ≥80ms (before jitter), assert a meaningful increase.

Apply:

-			// Higher retry count should generally result in longer backoff
-			// (though jitter might make this not always true, so we use a reasonable threshold)
-			Expect(elapsed2).Should(BeNumerically(">=", time.Duration(float64(elapsed1)*0.5)))
+			// Higher retry count should yield a noticeably longer wait
+			Expect(elapsed2).Should(BeNumerically(">", elapsed1))
+			Expect(elapsed2).Should(BeNumerically(">=", 60*time.Millisecond))

---

218-241: “Secure random” claim isn’t verifiable here; assert jittered range and uniqueness instead

Current equality check can still pass with identical durations by coincidence. Increase samples and assert range + uniqueness.

Apply:

-			durations := make([]time.Duration, 5)
-			for i := 0; i < 5; i++ {
+			durations := make([]time.Duration, 10)
+			for i := 0; i < 10; i++ {
 				start := time.Now()
 				utils.WaitWithBackoff(ctx, "test-tenant", 0)
 				durations[i] = time.Since(start)
 			}
 
-			// Should have some variation in durations due to jitter
-			hasVariation := false
-			for i := 1; i < len(durations); i++ {
-				if durations[i] != durations[0] {
-					hasVariation = true
-					break
-				}
-			}
-			Expect(hasVariation).Should(BeTrue())
+			// Each duration should be within the expected jittered window
+			for _, d := range durations {
+				Expect(d).Should(BeNumerically(">=", 18*time.Millisecond))
+				Expect(d).Should(BeNumerically("<", 100*time.Millisecond))
+			}
+			// And at least one should differ
+			unique := map[time.Duration]struct{}{}
+			for _, d := range durations {
+				unique[d] = struct{}{}
+			}
+			Expect(len(unique)).Should(BeNumerically(">", 1))

Also consider renaming the test to “should introduce jitter (non-deterministic backoff)”.

---

285-314: LGTM: Span integration smoke tests

Good sanity check that spans are used without panics. If needed later, introduce a tiny test span implementation to assert RecordError/SetStatus calls.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

internal/authn/oidc/authn_test.go (1)

679-733: Assert true cancellation and shorten backoff to reduce flakiness

Tighten the assertion to require cancellation (don’t accept generic token errors) and increase test determinism by shortening backoff frequency. This also speeds the test.

-            auth, err := NewOidcAuthn(ctx, config.Oidc{
+            auth, err := NewOidcAuthn(ctx, config.Oidc{
                 Audience:          audience,
                 Issuer:            issuerURL,
                 RefreshInterval:   5 * time.Minute,
                 BackoffInterval:   12 * time.Second,
                 BackoffMaxRetries: 3,
-                BackoffFrequency:  2 * time.Second, // Longer frequency to allow cancellation
+                BackoffFrequency:  200 * time.Millisecond,
             })
@@
-            time.Sleep(100 * time.Millisecond)
+            time.Sleep(300 * time.Millisecond)
             cancel()
@@
-            case err := <-authErr:
-                Expect(err).To(HaveOccurred())
-                // The error might be wrapped, so check for context cancellation in the error chain
-                Expect(err.Error()).To(Or(
-                    ContainSubstring("context canceled"),
-                    ContainSubstring("context cancelled"),
-                    Equal(base.ErrorCode_ERROR_CODE_INVALID_BEARER_TOKEN.String()),
-                ))
+            case err := <-authErr:
+                Expect(err).To(HaveOccurred())
+                Expect(err.Error()).To(Or(
+                    ContainSubstring("context canceled"),
+                    ContainSubstring("context cancelled"),
+                ))

internal/authn/oidc/authn_test.go (2)

524-602: Reduce brittleness in error assertions; consider DRYing the cases

Exact string equality on errors is brittle. Prefer substring match. Optionally, collapse these into a table-driven loop to avoid repetition.

Apply this minimal change to loosen assertions:

-            Expect(err.Error()).To(Equal("invalid or missing backoffInterval"))
+            Expect(err.Error()).To(ContainSubstring("invalid or missing backoffInterval"))

-            Expect(err.Error()).To(Equal("invalid or missing backoffInterval"))
+            Expect(err.Error()).To(ContainSubstring("invalid or missing backoffInterval"))

-            Expect(err.Error()).To(Equal("invalid or missing backoffMaxRetries"))
+            Expect(err.Error()).To(ContainSubstring("invalid or missing backoffMaxRetries"))

-            Expect(err.Error()).To(Equal("invalid or missing backoffMaxRetries"))
+            Expect(err.Error()).To(ContainSubstring("invalid or missing backoffMaxRetries"))

-            Expect(err.Error()).To(Equal("invalid or missing backoffFrequency"))
+            Expect(err.Error()).To(ContainSubstring("invalid or missing backoffFrequency"))

-            Expect(err.Error()).To(Equal("invalid or missing backoffFrequency"))
+            Expect(err.Error()).To(ContainSubstring("invalid or missing backoffFrequency"))

---

664-676: Prefer pending test (XIt/PIt) over runtime Skip

Marking the spec pending avoids executing the body and communicates intent more clearly.

-        It("should return error for invalid token claims format", func() {
+        XIt("should return error for invalid token claims format", func() {
@@
-            Skip("Invalid claims format test - JWT library automatically converts to MapClaims")
         })

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

internal/engines/expand_test.go (3)

1583-1587: Avoid brittle exact error string comparisons

Comparing err.Error() to base.ErrorCode_ERROR_CODE_SCHEMA_NOT_FOUND.String() is fragile (wrapping/formatting may change). Prefer matching by code or using substring.

Apply:

-           Expect(err.Error()).Should(Equal(base.ErrorCode_ERROR_CODE_SCHEMA_NOT_FOUND.String()))
+           Expect(err.Error()).Should(ContainSubstring(base.ErrorCode_ERROR_CODE_SCHEMA_NOT_FOUND.String()))

If there’s a typed error exposing a code, compare codes directly instead of strings.

Also applies to: 1613-1615

---

1590-1604: “context cancellation” test doesn’t validate cancellation

The failure is due to missing schema, not cancellation. Either write a minimal schema so the code reaches a cancelable path, and assert context.Canceled/status, or drop/rename this test.

Also applies to: 1609-1615

---

1520-1520: Remove hardcoded source line references in comments

Inline “(line …)” notes will drift. They add maintenance noise without test value.

-           // Test entity definition read error (line 77)
+           // Test entity definition read error

(Repeat for other instances in this context.)

Also applies to: 1552-1552, 1576-1576, 1601-1601, 1618-1618, 1628-1628, 1644-1644, 1659-1659

internal/engines/lookup_test.go (3)

5016-5020: Don’t pass nil stream sink

LookupEntityStream receives a nil sink; the test relies on failing earlier. Pass a no-op sink to decouple from call-order.

-            err = engine.LookupEntityStream(context.Background(), request, nil)
+            sink := func(*base.PermissionLookupEntityStreamResponse) error { return nil }
+            err = engine.LookupEntityStream(context.Background(), request, sink)

---

24-46: Local mockSchemaReader OK; consider reuse

The stub is fine. If multiple packages need this, consider moving it to a shared test helper to reduce duplication.

---

4955-4958: Message substring check is fine; prefer sentinel error if available

Using “failed to create bulk checker” ties to wording. If the engine exposes a sentinel error, switch to errors.Is.

internal/engines/bulk_test.go (7)

63-65: Avoid brittle string equality for errors

Exact string matches are fragile. Prefer MatchError with substring.

-			Expect(err).Should(HaveOccurred())
-			Expect(err.Error()).Should(Equal("context cannot be nil"))
+			Expect(err).Should(MatchError(ContainSubstring("context cannot be nil")))

-			Expect(err).Should(HaveOccurred())
-			Expect(err.Error()).Should(Equal("callback cannot be nil"))
+			Expect(err).Should(MatchError(ContainSubstring("callback cannot be nil")))

Also applies to: 76-78

---

191-193: Same: use MatchError instead of string equality

-				Expect(err).Should(HaveOccurred())
-				Expect(err.Error()).Should(Equal("size must be greater than 0"))
+				Expect(err).Should(MatchError(ContainSubstring("size must be greater than 0")))

---

237-239: Fix misleading comment

This creates a checker whose Check returns an error; the callback isn’t returning an error.

-				// Create a checker with a callback that returns an error
+				// Create a checker whose underlying engine returns an error

---

124-186: Consider edge cases in sortRequests tests

Add cases for: already-sorted input (stability), duplicate IDs, and nil Entity/Subject to guard against panics.

I can add these tests if you want.

---

213-216: Strengthen success-path coverage

Currently ExecuteRequests(1) runs with no queued requests. Consider publishing at least one request (via a publisher or directly pushing to checker.requestChan) and asserting the callback is invoked.

I can draft a minimal test that enqueues one request and waits with Eventually on a callback counter.

---

48-103: Add missing validations: nil checker and negative config values

Expand NewBulkChecker tests to cover nil checker error and negative ConcurrencyLimit/BufferSize defaulting.

 	Context("NewBulkChecker", func() {
 		var mockChecker invoke.Check
 
 		BeforeEach(func() {
 			mockChecker = &mockCheckEngine{}
 		})
 
+		It("should return error for nil checker", func() {
+			ctx := context.Background()
+			config := BulkCheckerConfig{ConcurrencyLimit: 5, BufferSize: 100}
+			_, err := NewBulkChecker(ctx, nil, BulkCheckerTypeEntity, func(string, string) {}, config)
+			Expect(err).Should(MatchError(ContainSubstring("checker cannot be nil")))
+		})
+
+		It("should default negative config values", func() {
+			ctx := context.Background()
+			config := BulkCheckerConfig{ConcurrencyLimit: -1, BufferSize: -5}
+			chk, err := NewBulkChecker(ctx, mockChecker, BulkCheckerTypeEntity, func(string, string) {}, config)
+			Expect(err).ShouldNot(HaveOccurred())
+			DeferCleanup(chk.cancel)
+			Expect(chk.config.ConcurrencyLimit).Should(Equal(10))
+			Expect(chk.config.BufferSize).Should(Equal(1000))
+		})

---

41-45: Optional: don’t over-specify defaults

Asserting concrete numbers can cause churn if defaults tune over time. If the goal is “non-zero sensible defaults,” consider BeNumerically(">=", 1) instead.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.