feat(org-lens): shared Valkey cache for org membership lookups

[luismoriguerra]

Summary

• Replace the per-process in-memory memo in OrgMembershipResolverService with a cross-instance, fail-soft Valkey-backed cache. The 3-replica UI (no sticky sessions) previously got ~1/3 cache hit rate and incoherent results across pods.
• Security fix: the cache key is now bound to the authorization principal (the impersonation-aware effective username from getEffectiveUsername(req) — the same principal the query-service FGA-filters the data on) and fails closed when it can't be resolved — removing the ?? 'anon' fallback that could serve one user's authorized membership data to another.
• Add a reusable ValkeyService (lazy connect, TLS via rediss://, per-op timeout, never throws) and migrate OrgRoleGrantsService (org-selector role grants) onto it, with Map↔ordered-entry-array (de)serialization.
• Opt-in / safe rollout: caching activates only when VALKEY_URL is set; unset = today's direct-fetch behavior. Enabled per-env via a companion lfx-v2-argocd PR.

Behavior when disabled / cache down

Fail-soft: any cache fault, timeout, unset VALKEY_URL, or unresolved principal degrades to a direct upstream fetch. Cache state never gates startup or /readyz (lazy client).

Test plan

• yarn lint:check (0 errors)
• yarn check-types
• yarn format:check
• yarn build (server + client)
• Manual multi-instance verification per specs/026-valkey-shared-cache/quickstart.md (per-user isolation, cross-pod coherence, fail-soft) — see note below
• CI Playwright e2e gate

Note: the org-membership cache is server-internal; page-level Playwright route stubs shadow the BFF endpoint it sits behind, so isolation/coherence/fail-soft are validated via the quickstart manual multi-instance procedure rather than e2e. No unit tests per repo convention.

Resolves https://linuxfoundation.atlassian.net/browse/LFXV2-2184

Made with Cursor

[Copilot]

Pull request overview

This PR introduces an opt-in, cross-instance Valkey (Redis-compatible) cache to improve coherence and hit rate for server-side org membership lookups and org role-grants resolution in the multi-replica UI deployment, while tightening cache isolation to the authorization principal.

Changes:

• Add shared cache interfaces/constants (CachePort, VALKEY_CACHE) and Helm VALKEY_URL wiring to enable/disable caching per environment.
• Introduce a fail-soft ValkeyService (lazy connect, TLS via rediss://, per-operation timeout) and use it for org membership + org role-grants caching.
• Replace in-process memos with shared Valkey-backed caches (principal/username-keyed) and preserve Map ordering via entry-array serialization.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
yarn.lock	Adds ioredis and its transitive dependencies to the lockfile.
apps/lfx-one/package.json	Adds ioredis dependency for the server runtime.
packages/shared/src/interfaces/valkey-cache.interface.ts	Defines a generic cache port (CachePort) and cached envelope type for shared caching.
packages/shared/src/interfaces/org-selector.interface.ts	Adds a serializable cache entry shape for AccessAwareOrgsResult (Map ↔ entry arrays).
packages/shared/src/interfaces/index.ts	Exports the new Valkey cache interface(s) from the shared barrel.
packages/shared/src/constants/valkey-cache.constants.ts	Adds shared cache configuration (namespaces, TTLs, timeouts, size cap).
packages/shared/src/constants/index.ts	Exports the Valkey cache constants from the shared barrel.
charts/lfx-self-serve/values.yaml	Adds VALKEY_URL config knob to enable shared caching via Helm values.
apps/lfx-one/src/server/services/valkey.service.ts	Implements the lazy, fail-soft Valkey client + JSON read-through helper.
apps/lfx-one/src/server/services/org-role-grants.service.ts	Migrates role-grants memoization to Valkey using username-scoped keys and Map serialization.
apps/lfx-one/src/server/services/org-membership-resolver.service.ts	Migrates org membership memoization to Valkey using principal-bound keys and keeps per-process in-flight dedup.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This PR adds a Valkey/Redis-backed fail-soft cache (singleton ValkeyService), shared cache interfaces and constants, migrates OrgMembershipResolverService and OrgRoleGrantsService to use principal/username-scoped Valkey caching with in-process coalescing, and exposes VALKEY_URL in Helm values.

Changes

Valkey-Backed Caching Infrastructure and Service Migration

Layer / File(s)	Summary
Cache contracts and configuration packages/shared/src/interfaces/valkey-cache.interface.ts, packages/shared/src/interfaces/org-selector.interface.ts, packages/shared/src/constants/valkey-cache.constants.ts, packages/shared/src/interfaces/index.ts, packages/shared/src/constants/index.ts	Defines CachePort fail-soft cache interface and CachedEnvelope<T>, AccessAwareOrgsCacheEntry serialization shape, centralizes VALKEY_CACHE constants (namespaces, TTLs, timeouts, max size), and updates barrel re-exports.
ValkeyService singleton and dependency apps/lfx-one/package.json, apps/lfx-one/src/server/services/valkey.service.ts	Implements ValkeyService singleton with conditional enablement (disabled when VALKEY_URL unset), ioredis client wiring, per-operation timeouts, JSON (de)serialization and size enforcement, withCache read-through helper, graceful shutdown, and adds ioredis dependency.
OrgMembershipResolverService migration apps/lfx-one/src/server/services/org-membership-resolver.service.ts	Replaces in-process membership memoization with principal-bound cache keys (JWT sub + org UID + slug), fail-closed bypass when sub missing, in-process membershipsInFlight coalescing, and extracts buildCacheKey and runMembershipFetch helpers.
OrgRoleGrantsService migration apps/lfx-one/src/server/services/org-role-grants.service.ts	Replaces in-memory Map-based caching with username-bound Valkey caching (filter-safe usernames), serialization helpers to convert Map fields to ordered arrays for JSON storage, accessInFlight coalescing, TTL conversion, and cache-entry validation.
Deployment configuration charts/lfx-self-serve/values.yaml	Adds VALKEY_URL environment variable with TLS (rediss://) hint and opt-out via empty string.

Sequence Diagram

sequenceDiagram
  participant Client
  participant OrgMembershipResolver as OrgMembershipResolverService
  participant ValkeyService
  participant Valkey as Valkey/Redis
  participant ResourcesAPI as Resources Microservice
  Client->>OrgMembershipResolver: fetchMembershipsBySlug(req, orgUid, slug)
  OrgMembershipResolver->>OrgMembershipResolver: buildCacheKey(principal, orgUid, slug)
  alt No principal (key null)
    OrgMembershipResolver->>OrgMembershipResolver: runMembershipFetch(req, orgUid, slug)
    OrgMembershipResolver->>ResourcesAPI: paginated /query/resources
  else Principal present
    alt membershipsInFlight hit
      OrgMembershipResolver->>OrgMembershipResolver: return cached promise
    else withCache
      ValkeyService->>Valkey: getJson(cache_key)
      alt Cache hit
        Valkey-->>ValkeyService: cached envelope
      else Cache miss
        ValkeyService->>OrgMembershipResolver: call fetcher
        OrgMembershipResolver->>ResourcesAPI: runMembershipFetch
        ResourcesAPI-->>OrgMembershipResolver: ProjectMembershipDoc[]
        OrgMembershipResolver-->>ValkeyService: result
        ValkeyService->>Valkey: setJson(cache_key, result, ttl)
      end
      ValkeyService-->>OrgMembershipResolver: result
    end
  end
  OrgMembershipResolver-->>Client: ProjectMembershipDoc[]

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

ai-assisted, enhancement

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main objective: introducing a shared Valkey cache for org membership lookups, which is the primary focus of the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description clearly outlines the scope: replacing in-memory cache with Valkey-backed shared cache, fixing a security issue with principal binding, introducing ValkeyService, and ensuring opt-in behavior with fail-soft degradation.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/LFXV2-2184

---

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

[coderabbitai]

🧹 Nitpick comments (1)

apps/lfx-one/src/server/services/org-role-grants.service.ts (1)

37-57: 💤 Low value

Consider adding in-flight request coalescing for concurrent cache misses.

Unlike OrgMembershipResolverService, this service doesn't coalesce concurrent in-flight requests. On cache miss, multiple concurrent requests for the same username will all compute and write to cache (last write wins). This is safe but wasteful and could cause a thundering herd on cold-start or cache expiry.

The membership service's membershipsInFlight pattern could be applied here if load warrants it.

🤖 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 `@apps/lfx-one/src/server/services/org-role-grants.service.ts` around lines 37
- 57, Add in-flight request coalescing to getAccessAwareOrgs by introducing a
module-level Map (e.g., orgsInFlight) keyed by the same value returned from
OrgRoleGrantsService.buildCacheKey(username); on cache miss check orgsInFlight
for an existing Promise and return it if present, otherwise create and store the
Promise returned by calling computeAccessAwareOrgs(req, username), await it,
then on completion remove the entry from orgsInFlight and proceed to the
existing cache set logic (only set when !result.upstreamFailed); ensure that on
rejection you delete the in-flight entry and rethrow so failures aren’t cached
and you avoid leaks (mirror the membershipsInFlight pattern used in
OrgMembershipResolverService).

🤖 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 `@apps/lfx-one/src/server/services/org-role-grants.service.ts`:
- Around line 37-57: Add in-flight request coalescing to getAccessAwareOrgs by
introducing a module-level Map (e.g., orgsInFlight) keyed by the same value
returned from OrgRoleGrantsService.buildCacheKey(username); on cache miss check
orgsInFlight for an existing Promise and return it if present, otherwise create
and store the Promise returned by calling computeAccessAwareOrgs(req, username),
await it, then on completion remove the entry from orgsInFlight and proceed to
the existing cache set logic (only set when !result.upstreamFailed); ensure that
on rejection you delete the in-flight entry and rethrow so failures aren’t
cached and you avoid leaks (mirror the membershipsInFlight pattern used in
OrgMembershipResolverService).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1d9c3e47-0f8f-49e9-807f-df11205a05f7

📥 Commits

Reviewing files that changed from the base of the PR and between c23486c and 6ac56a6.

⛔ Files ignored due to path filters (1)

• yarn.lock is excluded by !**/yarn.lock, !**/*.lock

📒 Files selected for processing (10)

• apps/lfx-one/package.json
• apps/lfx-one/src/server/services/org-membership-resolver.service.ts
• apps/lfx-one/src/server/services/org-role-grants.service.ts
• apps/lfx-one/src/server/services/valkey.service.ts
• charts/lfx-self-serve/values.yaml
• packages/shared/src/constants/index.ts
• packages/shared/src/constants/valkey-cache.constants.ts
• packages/shared/src/interfaces/index.ts
• packages/shared/src/interfaces/org-selector.interface.ts
• packages/shared/src/interfaces/valkey-cache.interface.ts

[jordane]

Please hold off on merging as I am also making some changes on this function as part of the sub -> username swap. This can be merged after Thursday.

[luismoriguerra]

Review Feedback Addressed — round 1

Commit: 150f560 (DCO signed-off + GPG-signed)

Changes Made

• valkey.service.ts / valkey-cache.interface.ts: added an optional accept shape-validator to getJson/withCache. A corrupt, partial, or legacy cached entry now degrades to a miss (logged + recomputed) instead of surfacing a fault — reinforces the fail-soft contract. (per copilot-pull-request-reviewer)
• org-membership-resolver.service.ts: the membership read passes Array.isArray to withCache, so a malformed cached value can't reach resolveContext().filter() and turn a cache hit into a 500. (per copilot-pull-request-reviewer)
• org-role-grants.service.ts: the cached entry is shape-validated before deserializeAccessResult (no 500 from a corrupt hit); also added per-process in-flight coalescing of concurrent misses for the same username, mirroring membershipsInFlight. (per copilot-pull-request-reviewer + coderabbitai[bot] nitpick)

Threads Resolved

2 of 2 inline review threads resolved. The coderabbitai[bot] "in-flight coalescing" nitpick (posted in its review summary, not a separate thread) is also implemented in this commit.

Validation

yarn lint:check (0 errors), yarn build — both green.

Note: not requesting merge — the do-not-merge label and @jordane's hold (sub → username swap on OrgRoleGrantsService, mergeable after Thursday) still stand. These fixes are pushed so the branch is review-clean when the hold lifts; happy to rebase onto @jordane's changes when they land.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.

[luismoriguerra]

Review-thread round — recap

Worked the 3 unresolved review threads (all from copilot-pull-request-reviewer).

Fixed & resolved

• charts/lfx-self-serve/values.yaml — the VALKEY_URL comment implied the rediss:// scheme means "no auth token". Reworded: rediss:// enables TLS in transit, credentials are optional (embeddable in the URL when required), and our deployment stays SG-gated with no token. Commit 5aebb94c. ✅ resolved

Replied, intentionally left open (by-design — maintainer call)

• apps/lfx-one/src/server/services/org-membership-resolver.service.ts — request to restore the per-process TTL result memo. The prior memo's anon fallback could serve one user's FGA-filtered result to another (fail-open); it was deliberately replaced by the principal-keyed shared Valkey cache (single authoritative cache, no cross-instance drift). Valkey-disabled is a fail-soft direct fetch still protected by principal-keyed in-flight coalescing, and production sets VALKEY_URL. Re-adding a result memo would reverse that decision, so I did not.
• apps/lfx-one/src/server/services/org-role-grants.service.ts — same intentional trade-off (accessCache removed on purpose; accessInFlight coalescing retained).

Quality gate (lfx-v2-ui / BFF)

• yarn format:check ✅
• yarn lint:check ✅ (0 errors; 1 pre-existing warning in unrelated docs-manifest.service.ts)
• yarn build ✅ (required yarn install first — the new ioredis dep wasn't yet in this worktree's node_modules; yarn.lock already pinned it, no lockfile change)

No unit tests added (Playwright e2e only; no behavior change in this round). The build-regenerated search-index.json artifact was reverted, not committed.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.

[luismoriguerra]

PR review-thread round — post-merge Copilot pass (commit c00dd3b5)

Addressed the 5 new threads from Copilot's fresh review after the origin/main merge (dfef139). All quality gates pass (yarn format:check, yarn lint:check, yarn build); the build-regenerated search-index.json was reverted (not committed).

Fixed + resolved (5):

Thread	File	Fix
Array guard allowed nullish elements	org-membership-resolver.service.ts	withCache now uses isMembershipDocArray (rejects arrays with nullish/non-object elements) so a malformed hit degrades to a miss + refetch instead of throwing on m.status/m.uid.
isValidCacheEntry under-validated	org-role-grants.service.ts	Now also requires loadedAt: string and upstreamFailed: boolean, so a partial entry can't emit loaded_at: undefined/non-boolean on the wire.
getJson logs leaked full key	valkey.service.ts	New redactKey(); valkey_get warn logs emit only ${APP_PREFIX}:${NAMESPACE}:***.
setJson logs leaked full key	valkey.service.ts	valkey_set warn logs use the redacted namespace.
withCache debug logs leaked full key	valkey.service.ts	cache_bypass/cache_hit/cache_miss debug logs use the redacted namespace.

Left open — by-design (2, maintainer call):

• org-membership-resolver.service.ts:86 and org-role-grants.service.ts:52 — Copilot's "removing the per-process 30s TTL memo regresses the no-Valkey path." These already carry the author's rationale (the per-process result memo was deliberately dropped in favor of the principal-keyed shared cache to avoid cross-user leak + cross-instance drift; in-flight coalescing remains). Intentional trade-off — left for a maintainer call, not silently resolved.

No merge attempted (PR carries do-not-merge + hold pending the sub→username swap on OrgRoleGrantsService). Changes are guard/log-only and avoid that logic to stay conflict-free.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.

[luismoriguerra]

PR thread-resolution round — recap

Branch synced with origin/main (merged 7fd193c5 nav + 34b85cb8 docs, no conflicts, no force-push), then addressed all 3 open Copilot threads.

Thread	File	Disposition
readonly field assigned in constructor	valkey.service.ts:32	Resolved — false positive. Valid TS (readonly is assignable within the constructor); yarn build typechecks cleanly.
isMembershipDocArray accepts [{}] / arrays	org-membership-resolver.service.ts	Resolved — hardened. Now requires each element be a non-array object with a string uid.
isEntryTupleArray accepts array tuple-values	org-role-grants.service.ts	Resolved — hardened. Now rejects [["uid", []]] via !Array.isArray(item[1]).

Both shape-guard fixes make a corrupt/legacy cache entry degrade to a miss + recompute instead of being served as a bogus hit — no behavior change for valid entries.

Preserved by design (not regressed): full TLS verification via plain rediss:// (no rejectUnauthorized/checkServerIdentity overrides); principal-bound, impersonation-aware, namespaced cache keys; no per-process result memo reintroduced.

Quality gate: yarn format:check ✅ · yarn lint:check ✅ · yarn build ✅ · yarn.lock unchanged · regenerated search-index.json reverted.

Commit bc8d38ac (DCO --signoff, GPG-signed).

[Copilot]

Pull request overview

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

[luismoriguerra]

PR thread-resolution round — 99f01187

Addressed all 4 open review threads (comment/doc accuracy + key-namespace sanitization). No code behavior changed beyond the namespace sanitization; TLS handling, principal-bound cache keys, the by-design removal of the per-process result memo, and the hardened shape guards are all untouched.

Thread	Fix
valkey.service.ts — namespace not sanitized	cacheKeyNamespace() now replaces unsafe chars with - instead of silently dropping a configured namespace; doc comment corrected.
runtime-configuration.md — deployment-specific docs	Removed Unset in dev/staging/prod + feature-branch wording; row is now deployment-agnostic.
valkey-cache.constants.ts:9 — "bump to invalidate"	Reworded: bumping starts a new namespace; old entries age out via TTL.
valkey-cache.constants.ts:24 — "single-node"	Dropped the deployment-specific detail.

Quality gate (UI/BFF): yarn format:check, yarn lint:check, yarn build all green. Build-regenerated search-index.json reverted; yarn.lock unchanged.

[Copilot]

Pull request overview

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

[luismoriguerra]

Review-thread resolution round — recap

Addressed the one remaining open thread.

Fixed (1):

• valkey.service.ts redactKey() (r3417122308) — when VALKEY_KEY_NAMESPACE was set, the redaction kept only the first two segments (lfx-ui:<ns>:***), dropping the cache domain/version from logs. redactKey() now anchors on the vN version segment instead of a fixed index, so it preserves the full non-user prefix (app prefix + optional deployment namespace + domain + version) whether or not the deployment namespace is present, while still masking the principal tail. Doc comment updated to match. Commit 281ff998.

Quality gate: yarn format:check ✓ · yarn lint:check ✓ · yarn build ✓. No yarn.lock change; build-regenerated search-index.json reverted (not committed).

Open threads remaining: 0. Note: reviewDecision stays CHANGES_REQUESTED pending re-review by @jordane and @dealako.

[dealako]

Follow-up review (round 3) — new namespacing + guard hardening looks clean ✅

Thanks @luismoriguerra — another tight, well-targeted round. Since my last approval (which was auto-dismissed when you pushed new commits), you've added per-deployment cache-key isolation and addressed @jordane's and Copilot's latest comments. I re-ran both my security and code-quality reviewers against the incremental cache diff only (ignoring the reddit-ads / docs-portal / committees changes that rode in via the main merges) — both came back clean, re-approve.

👏 Nice work

• VALKEY_KEY_NAMESPACE per-deployment isolation — a genuinely good catch for branch previews sharing dev's ElastiCache. The sanitizer (.replace(/[^A-Za-z0-9._-]/g, '-')) correctly strips the : delimiter so an operator-set namespace can't inject extra key segments, and both services build the prefix identically. Verified branch deployments can't read each other's keys, and the no-namespace key shape is unchanged.
• cacheKeyNamespace() now sanitizes in place instead of silently returning '' — directly resolving @jordane's complaint and matching its doc comment.
• Hardened shape guards — isMembershipDocArray now requires a string uid (rejects [{}] and arrays), and isEntryTupleArray rejects array-valued tuples like [["uid", []]]. Both correctly degrade corrupt entries to a miss + recompute with no false rejection of valid data (empty arrays still pass as legit hits).
• redactKey() anchored on the vN version segment — verified it masks the principal across every key shape, with or without the deployment namespace.

Revision tracking — all RESOLVED ✅

1. ✅ @jordane: cacheKeyNamespace returning '' instead of sanitizing → fixed in 99f01187 (sanitizes to -).
2. ✅ @jordane: runtime-configuration.md deployment-specific docs → genericized in 99f01187.
3. ✅ @jordane: constants comment "bumping invalidates" → reworded to "starts a new namespace, old entries age out via TTL" (99f01187).
4. ✅ @jordane: MAX_VALUE_BYTES "single-node" deployment claim → removed (99f01187).
5. ✅ Copilot: redactKey dropping domain/version with a namespace set → fixed via findIndex anchor (281ff998).
6. ✅ Copilot: shape guards accepting [{}] / array-valued tuples → hardened (bc8d38ac).
7. ✅ Copilot: readonly client reassigned in constructor flagged as a compile error → confirmed false positive. TypeScript permits assigning a readonly property within the declaring class's constructor even with an initializer; the = null initializer is required for the VALKEY_URL-unset early-return path, so keep it as-is. Agreeing with your assessment.

Note on timing: @jordane's CHANGES_REQUESTED (18:05 local) predates your fixes (99f01187 18:11, 281ff998 18:21), so that formal block is now stale and just needs @jordane's re-review to clear.

Reconciliation with new bot comments

All new Copilot comments this round are addressed (shape guards, redactKey, readonly false-positive). The recurring TLS-hostname note is the same accepted design (companion argocd #936 points VALKEY_URL at the real ElastiCache endpoint whose cert SAN matches) — no change needed.

Open items

• 🔴 Blocking: 0
• 🟡 Minor: 0
• ⚪ Nit (optional, non-blocking): 2 — (1) if VALKEY_KEY_NAMESPACE itself matched /^v\d+$/ (e.g. v5), redactKey would anchor on it and over-mask the domain in logs (never a principal leak); the doc comment's "version immediately precedes the principal" is slightly overstated for that pathological config. (2) cacheKeyNamespace() has no length cap and treats whitespace/all-unsafe input (' ') as a set namespace (---); distinct inputs can also collapse to the same sanitized value. Both are operator-config edge cases, bounded by principal-keyed entries — fine to leave or tidy in a follow-up.

Decision: ✅ Approved with minor comments

No blocking issues, no security findings, no merge conflicts, clean fail-closed/fail-soft semantics throughout. Solid iteration. (FYI @jordane — your specific requests appear fully addressed in 99f01187/281ff998; a re-review would clear the standing block.)

[luismoriguerra]

Review-thread resolution round (HEAD f61f4692)

Branch sync: merged origin/main (3 commits — crowdfunding/routing, unrelated to the cache work) into feat/LFXV2-2184 (a6cb69b7); no cache files touched, no conflicts.

Threads addressed: 1 open thread (out of 37 total).

• apps/lfx-one/src/server/services/valkey.service.ts — nit (optional) on redactKey() over-masking when VALKEY_KEY_NAMESPACE is vN-shaped (@dealako). Took the suggested low-effort path: added a one-line doc caveat noting the first-vN-match anchor over-masks the domain in that pathological case — harmless (reduced log fidelity only, never a principal leak), so the behavior is intentionally left as-is. Resolved. (f61f4692)

Quality gate: yarn format:check ✅ · yarn lint:check ✅ · yarn build ✅ — build-regenerated search-index.json reverted; no yarn.lock change.

No locked decisions regressed (TLS full-verify via rediss://, principal-bound impersonation-aware keys, no per-process result memo, hardened shape guards, deployment-agnostic comments).

[Copilot]

Pull request overview

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

[luismoriguerra]

Review-thread resolution round — recap

Commit: 4864d5b2 (DCO signed-off, GPG-signed)

Threads addressed (1 open → 0 open):

• apps/lfx-one/src/server/services/org-role-grants.service.ts (Copilot) — cache-entry guard didn't validate ResolvedOrgRole.roleSource. Fixed. isEntryTupleArray now accepts an optional value guard; the resolved map is validated via isResolvedOrgRole (non-empty string roleSource + string-typed optional parentUid/parentName). A corrupt entry like resolved: [["uid", {}]] now degrades to a cache miss + recompute instead of throwing a hit-turned-500 in roleSource.startsWith(...). Resolved.

Quality gate (BFF — Playwright e2e only, no unit tests):

• yarn format:check ✅
• yarn lint:check ✅
• yarn build ✅ (build-regenerated search-index.json reverted; no yarn.lock change)

No prior locked decisions were regressed (full TLS verification via plain rediss://, principal-bound keys, no reintroduced per-process result memo, deployment-agnostic comments).

[dealako]

Follow-up review (round 4) — clean ✅

Thanks @luismoriguerra — small, surgical round and both open items are fully closed. I re-ran both reviewers against just the incremental cache diff (the crowdfunding / routing / committees churn in the branch is main-merge noise, not this PR), and they came back clean.

👏 Nice work

• isResolvedOrgRole value guard (org-role-grants.service.ts) — exactly the right fix for the Copilot finding. Threading an optional valueGuard into isEntryTupleArray and validating that resolved tuple values carry a non-empty string roleSource (plus string-typed optional parentUid/parentName) closes the last deserialize-to-500 path: a corrupt resolved: [["uid", {}]] now degrades to a miss + recompute instead of throwing on roleSource.startsWith(...) downstream. I traced the consumers — roleSource is genuinely the only unsafe-access (throw-risk) field on ResolvedOrgRole, and the guard matches the interface's required/optional split exactly. No false-rejection risk: every valid OrgRolePersona literal is a non-empty string, so length > 0 never rejects a legitimate entry.
• redactKey doc caveat — picked the low-effort option on my round-3 nit and documented that the vN anchor over-masks (never leaks) when a deployment namespace is itself vN-shaped. Comment-only, accurate. 👍

Revision tracking — all RESOLVED ✅

1. ✅ My round-3 nit: redactKey doc overstating "version immediately precedes the principal" → caveat added in f61f4692.
2. ✅ Copilot (14:29): isValidCacheEntry not validating resolved tuple values have roleSource → guarded via isResolvedOrgRole in 4864d5b2.

Reconciliation with new bot comments

The one new Copilot comment this round (roleSource guard) is addressed — agreed, no duplication. No other new bot findings.

Open items

• 🔴 Blocking: 0
• 🟡 Minor: 0
• ⚪ Nit: 0 (one informational, no-action note from my security pass: isResolvedOrgRole accepts any non-empty string rather than restricting to the four OrgRolePersona literals — correct and forward-compatible for a crash-safety guard, since an unrecognized value is silently dropped by the defaultless switch, never thrown. Nothing to change.)

Decision: ✅ Approved

No blocking issues, no security findings, no merge conflicts, no false-rejection risk. The cache hardening is complete and the fail-soft/fail-closed semantics are sound end-to-end.

Reminder for merge: @jordane's earlier CHANGES_REQUESTED is still the standing review decision and predates the last several fix rounds — it's stale and just needs a quick re-review from them to clear before this can land.