feat(router): Implement Layer 1 Intent Router (spaCy + Redis) #2999
Conversation
layer2: implement ProjectService and DTOs
…to feat/layer1-clean
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a Layer‑1 Intent Router: new Redis client, intent classifier (spaCy), and per‑user rate limiter; updates settings, env example, dependencies, test config, Docker Compose for Redis, and frontend loading/skeleton UI and small UI tweaks. All changes are additive; no public API removals. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
The PR must be linked to an issue assigned to the PR author. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
.gitignore (1)
46-46: Remove redundant.enventry.Line 8 already contains
.env*which matches.envand any variants. This explicit.enventry at line 46 is redundant.🔎 Proposed fix
-.env .venv users.acl dump.rdbdocker-compose.yml (1)
1-11: Consider adding health check and data persistence.The Redis service would benefit from:
- A health check to ensure the service is ready before dependent services connect
- A named volume for data persistence across container restarts
🔎 Proposed enhancements
version: '3.8' services: redis: image: redis/redis-stack-server:latest container_name: nestbot_redis ports: - "6379:6379" volumes: - - ./users.acl:/users.acl + - ./users.acl:/users.acl:ro + - redis_data:/data command: redis-stack-server --aclfile /users.acl restart: always + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 10s + timeout: 3s + retries: 3 + start_period: 10s + +volumes: + redis_data:backend/apps/ai/router/limiter.py (1)
9-13: Make rate limit parameters configurable.The hardcoded limit (20) and window (60s) values should be configurable via environment variables or constructor parameters to support different rate limiting needs across environments.
🔎 Proposed refactor
- def __init__(self): + def __init__(self, limit: int | None = None, window: int | None = None): """Initialize RateLimiter with Redis connection, limit, and window.""" self.redis = RedisRouterClient().get_connection() - self.limit = 20 # Max requests - self.window = 60 # Time window in seconds + self.limit = limit or int(os.getenv("RATE_LIMIT", "20")) + self.window = window or int(os.getenv("RATE_LIMIT_WINDOW", "60"))backend/apps/ai/router/intent.py (2)
20-23: Add schema version to cache keys to support heuristic updates.The cache key at line 23 doesn't include a version identifier. When you update the
static_keywordslist or change the classification logic, old cached results will remain and return incorrect classifications until they expire (3600s). This can cause confusing behavior after deployments.🔎 Proposed fix with versioned cache keys
+# Increment this when changing classification logic or keywords +INTENT_SCHEMA_VERSION = "v1" + class IntentRouter: """Router for classifying user queries as STATIC or DYNAMIC intents.""" def __init__(self): """Initialize IntentRouter with Redis connection.""" self.redis = RedisRouterClient().get_connection() def _get_cache_key(self, query: str) -> str: """Create a unique SHA256 hash of the query for caching.""" query_hash = hashlib.sha256(query.lower().strip().encode()).hexdigest() - return f"nestbot_cache:intent:{query_hash}" + return f"nestbot_cache:intent:{INTENT_SCHEMA_VERSION}:{query_hash}"
43-53: Consider making static keywords configurable.The hardcoded
static_keywordslist makes it difficult to adjust the classification behavior across environments or experiments without code changes. Consider loading from environment variables or a configuration file.🔎 Example configuration approach
+import os + +# Default keywords, overridable via environment +DEFAULT_STATIC_KEYWORDS = [ + "leader", "maintainer", "cve", "github", "repo", + "version", "download", "link", "url" +] + class IntentRouter: """Router for classifying user queries as STATIC or DYNAMIC intents.""" def __init__(self): """Initialize IntentRouter with Redis connection.""" self.redis = RedisRouterClient().get_connection() + # Allow runtime configuration + keywords_env = os.getenv("STATIC_KEYWORDS") + self.static_keywords = ( + keywords_env.split(",") if keywords_env + else DEFAULT_STATIC_KEYWORDS + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)backend/apps/ai/__init__.py(1 hunks)backend/apps/ai/router/__init__.py(1 hunks)backend/apps/ai/router/client.py(1 hunks)backend/apps/ai/router/intent.py(1 hunks)backend/apps/ai/router/limiter.py(1 hunks)cspell/custom-dict.txt(1 hunks)docker-compose.yml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/apps/ai/router/__init__.py (3)
backend/apps/ai/router/client.py (1)
RedisRouterClient(27-54)backend/apps/ai/router/intent.py (1)
IntentRouter(13-69)backend/apps/ai/router/limiter.py (1)
RateLimiter(6-34)
backend/apps/ai/router/intent.py (1)
backend/apps/ai/router/client.py (2)
RedisRouterClient(27-54)get_connection(49-54)
backend/apps/ai/router/limiter.py (1)
backend/apps/ai/router/client.py (2)
RedisRouterClient(27-54)get_connection(49-54)
🔇 Additional comments (4)
backend/apps/ai/__init__.py (1)
1-1: LGTM!The module-level docstring appropriately documents the AI application package.
cspell/custom-dict.txt (1)
38-38: LGTM!The addition of "aclfile" to the spell-check dictionary correctly supports the Redis ACL configuration flag used in docker-compose.yml.
.gitignore (1)
47-49: LGTM!The additions correctly ignore Redis runtime artifacts (
users.acl,dump.rdb) and Python virtual environment directories (.venv).backend/apps/ai/router/__init__.py (1)
1-5: LGTM!The module correctly establishes the public API by importing and re-exporting the three router components. The
__all__list properly declares the public interface.
backend/apps/ai/router/client.py
Outdated
| _pool = None | ||
|
|
||
| def __init__(self): | ||
| """Initialize Redis client with singleton connection pool.""" |
There was a problem hiding this comment.
Are you sure it's a singleton?
There was a problem hiding this comment.
Are there any existing ready for use solutions?
I'd prefer production tested code vs AI generated file.
There was a problem hiding this comment.
Yes, we have the production tested code and im working on it, There were some mypy issues and im resolving that then we will push the PR
There was a problem hiding this comment.
Here I meant pre-existing rate limiter that can be reused instead of creating it from scratch.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
The code you submit to OWASP Nest requires tests. Please read https://github.com/OWASP/Nest/blob/main/CONTRIBUTING.md
…r query classification
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
backend/apps/ai/router/limiter.py (4)
15-15: Remove unused RATE_LIMIT_SCRIPT_SHA constant.This constant computes a SHA256 hash but is never actually used in the code. The
_get_script_shamethod callsscript_loadwhich returns the correct SHA from Redis. This dead code should be removed.🔎 Proposed fix
-import hashlib - from .client import RedisRouterClient RATE_LIMIT_SCRIPT = """ local count = redis.call('INCR', KEYS[1]) if count == 1 then redis.call('EXPIRE', KEYS[1], ARGV[1]) end return count """ -RATE_LIMIT_SCRIPT_SHA = hashlib.sha256(RATE_LIMIT_SCRIPT.encode()).hexdigest() -
21-25: Make rate limit parameters configurable.The limit (20 requests) and window (60 seconds) are hardcoded. Consider making these configurable via Django settings or constructor parameters to support different rate limits for different use cases without code changes.
🔎 Proposed enhancement
- def __init__(self): + def __init__(self, limit: int = None, window: int = None): """Initialize RateLimiter with Redis connection, limit, and window.""" self.redis = RedisRouterClient().get_connection() - self.limit = 20 - self.window = 60 + from django.conf import settings + self.limit = limit or getattr(settings, 'ROUTER_RATE_LIMIT', 20) + self.window = window or getattr(settings, 'ROUTER_RATE_WINDOW', 60) self._script_sha = NoneBased on learnings, a maintainer previously asked about using existing production-tested rate limiting solutions instead of building from scratch. Have you evaluated libraries like
django-ratelimitordjango-redis-cache's built-in rate limiting?
33-35: Narrow the exception handling for script loading.Catching
AttributeError, ValueError, TypeError, RuntimeErroris overly broad. Redis script operations primarily raiseredis.exceptions.RedisError. Consider catching more specific exceptions to avoid masking unexpected errors.🔎 Proposed fix
+ import redis.exceptions + try: self._script_sha = self.redis.script_load(RATE_LIMIT_SCRIPT) - except (AttributeError, ValueError, TypeError, RuntimeError): - # Log the exception if needed + except redis.exceptions.RedisError as e: + logger.warning("Failed to load rate limit script: %s", e) self._script_sha = RATE_LIMIT_SCRIPT_SHA
46-56: Improve exception handling in script execution.The exception types caught here should be more specific. The string matching for "noscript" is fragile. Consider using Redis-specific exception types and checking for
redis.exceptions.NoScriptErrordirectly if available in your Redis library version.🔎 Proposed improvement
+ import redis.exceptions + try: current_count = self.redis.evalsha(script_sha, 1, key, window) - except (AttributeError, ValueError, TypeError, RuntimeError) as e: - error_str = str(e).lower() - if "noscript" in error_str or "unknown script" in error_str: + except redis.exceptions.NoScriptError: + # Script was flushed from Redis, reload and retry + try: + current_count = self.redis.eval(RATE_LIMIT_SCRIPT, 1, key, window) + self._script_sha = self.redis.script_load(RATE_LIMIT_SCRIPT) + except redis.exceptions.RedisError as e: + logger.warning("Failed to execute rate limit script: %s", e) + return 0 + except redis.exceptions.RedisError as e: + logger.warning("Redis error in rate limiter: %s", e) + return 0 - try: - current_count = self.redis.eval(RATE_LIMIT_SCRIPT, 1, key, window) - self._script_sha = self.redis.script_load(RATE_LIMIT_SCRIPT) - except (AttributeError, ValueError, TypeError, RuntimeError): - # Log the exception if needed - return 0 - else: - return 0backend/apps/ai/router/client.py (1)
103-116: Consider making Redis port configurable.The port is hardcoded to 6379. While this is the standard Redis port, making it configurable via Django settings would improve flexibility for non-standard deployments.
🔎 Optional enhancement
+ redis_port = getattr(settings, "REDIS_PORT", 6379) + try: + redis_port = int(redis_port) + except (ValueError, TypeError): + logger.warning("Invalid REDIS_PORT value, using default 6379") + redis_port = 6379 + try: RedisRouterClient._pool = redis.BlockingConnectionPool( host=settings.REDIS_HOST, - port=6379, + port=redis_port, username=getattr(settings, "REDIS_USER", "nest_router"),backend/apps/ai/router/intent.py (2)
34-52: Pattern definitions are well-structured but consider edge cases.The use of linguistic features (
LEMMAfor base forms,LOWERfor case-insensitivity) is appropriate. However, some patterns likedownload,install, andversionare fairly broad and might match queries that should be DYNAMIC.Consider these examples:
- "How do I download my data?" → Would incorrectly match "download"
- "What version of security is best?" → Would incorrectly match "version"
🔎 Optional: Add more context to broad patterns
patterns = [ [{"LEMMA": "maintainer"}], [{"LEMMA": "leader"}], [{"LOWER": "github"}, {"LEMMA": "repo", "OP": "?"}], [{"LOWER": "cve"}], [{"LOWER": "owasp"}], - [{"LEMMA": "download"}], - [{"LEMMA": "install"}], - [{"LEMMA": "version"}], + [{"LEMMA": "download"}, {"LOWER": "owasp", "OP": "?"}], + [{"LEMMA": "install"}, {"LOWER": "owasp", "OP": "?"}], + [{"LOWER": "owasp"}, {"LEMMA": "version", "OP": "?"}], [{"LEMMA": "license"}], ]This adds optional context to reduce false positives while still matching the intended queries.
Based on the "Chill" review mode, this is marked as optional since the current patterns may be intentionally broad as a starting point.
59-99: Add validation for empty or whitespace-only queries.The
get_intentmethod doesn't validate input. Empty or whitespace-only queries will proceed through spaCy processing and caching unnecessarily. Consider adding early validation.🔎 Proposed validation
def get_intent(self, user_query: str) -> dict: """Classify the user query as STATIC or DYNAMIC intent and cache the result.""" + if not user_query or not user_query.strip(): + return { + "intent": "DYNAMIC", + "confidence": 0.0, + "source": "validation_fallback", + "args": { + "query": user_query, + "keyword": None, + "entities": [], + }, + } + cache_key = self._get_cache_key(user_query)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/.env.examplebackend/apps/ai/router/client.pybackend/apps/ai/router/intent.pybackend/apps/ai/router/limiter.pybackend/pyproject.tomlbackend/pytest.inibackend/settings/base.pydocker-compose/redis.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/ai/router/limiter.py (1)
backend/apps/ai/router/client.py (2)
RedisRouterClient(60-122)get_connection(120-122)
backend/apps/ai/router/intent.py (1)
backend/apps/ai/router/client.py (2)
RedisRouterClient(60-122)get_connection(120-122)
🪛 dotenv-linter (4.0.0)
backend/.env.example
[warning] 1-1: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 2-2: [UnorderedKey] The DJANGO_ALGOLIA_EXCLUDED_LOCAL_INDEX_NAMES key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 3-3: [UnorderedKey] The DJANGO_ALGOLIA_WRITE_API_KEY key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The DJANGO_ALLOWED_HOSTS key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 5-5: [UnorderedKey] The DJANGO_AWS_ACCESS_KEY_ID key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 6-6: [UnorderedKey] The DJANGO_AWS_SECRET_ACCESS_KEY key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 7-7: [UnorderedKey] The DJANGO_CONFIGURATION key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 8-8: [UnorderedKey] The DJANGO_DB_HOST key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 9-9: [UnorderedKey] The DJANGO_DB_NAME key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The DJANGO_DB_PASSWORD key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 11-11: [UnorderedKey] The DJANGO_DB_PORT key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 12-12: [UnorderedKey] The DJANGO_DB_USER key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 13-13: [UnorderedKey] The DJANGO_OPEN_AI_SECRET_KEY key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 14-14: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 14-14: [UnorderedKey] The DJANGO_PUBLIC_IP_ADDRESS key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 15-15: [UnorderedKey] The REDIS_HOST key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 16-16: [UnorderedKey] The REDIS_PASSWORD key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The REDIS_ALLOW_NO_PASSWORD key should go before the REDIS_HOST key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The REDIS_ROUTER_SOCKET_TIMEOUT key should go before the DJANGO_ALGOLIA_APPLICATION_ID key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The REDIS_ROUTER_SOCKET_CONNECT_TIMEOUT key should go before the REDIS_ROUTER_SOCKET_TIMEOUT key
(UnorderedKey)
🔇 Additional comments (11)
backend/pytest.ini (3)
2-3: Verify Django test settings module exists.The configuration references
DJANGO_SETTINGS_MODULE = settings.testandDJANGO_CONFIGURATION = Test. Ensure that thesettings.testmodule andTestconfiguration class exist in the codebase, or verify they are created elsewhere in this PR.
6-6: Review coverage threshold appropriateness for this PR scope.The
--cov-fail-under=80threshold is strict. For a focused PR adding the Layer 1 Intent Router, confirm that the new/modified code achieves ≥80% coverage. If the router is well-tested but integrations into existing modules are partial, this threshold might cause the build to fail unexpectedly.Consider whether a per-module or per-changed-lines coverage target would be more appropriate.
6-6: Verify parallel execution and coverage collection strategy.pytest-cov supports distributed testing through specialized controller classes that handle coordination between master and worker nodes, allowing all of pytest-xdist's features including remote interpreters while still getting coverage. However, race conditions when running with xdist have been documented, and recent issues (January 2025) show that concurrent worker processes can cause sqlite3 corruption ("no such table: file").
Line 6 (
--numprocesses=auto --cov=.with--cov-fail-under=80) works in ideal conditions but has known failure modes. Coverage measurement across multiple processes relies on each worker writing to uniquely-named files that the master node combines afterward, but this coordination can break under certain configurations, particularly with coverage concurrency settings.Recommendation: This configuration is acceptable if:
- Coverage concurrency is properly configured in
pyproject.toml(e.g.,concurrency = ["thread"]or appropriate multiprocessing settings)- pytest-cov is installed on all test environments
- CI tests the actual coverage threshold locally before merging (not relying solely on parallel run results)
Alternatively, follow the best practice of running coverage verification separately from parallel test execution in CI to ensure reliable threshold validation.
backend/settings/base.py (1)
140-151: LGTM! Redis router settings are well-configured.The new Redis configuration options are properly structured:
REDIS_ALLOW_NO_PASSWORDprovides a safe development mode while requiring explicit opt-in- Socket timeout defaults of 0.3s strike a good balance between responsiveness and reliability
- Using
FloatValuefor timeouts ensures proper type validationThe settings integrate cleanly with the RedisRouterClient implementation.
backend/.env.example (1)
15-19: LGTM! Redis configuration updates align with the new router settings.The Redis environment variables properly reflect the new settings added in
backend/settings/base.py. The defaults are sensible for development environments.backend/apps/ai/router/limiter.py (1)
7-13: LGTM! Lua script correctly implements atomic increment with expiry.The Lua script properly addresses the race condition mentioned in previous reviews by atomically executing INCR and EXPIRE in a single Redis operation. This ensures the TTL is always set when the counter is initialized.
backend/apps/ai/router/client.py (3)
12-57: LGTM! Timeout validation is thorough and robust.The
_get_timeout_valuehelper properly validates and clamps timeout values with clear logging for out-of-range values. The bounds (0.1s to 5.0s) are sensible for socket operations.
63-88: LGTM! Thread-safety and password validation correctly implemented.The double-checked locking pattern with
_pool_lockproperly addresses the thread-safety concern from previous reviews. The password validation withREDIS_ALLOW_NO_PASSWORDflag provides good security while allowing development flexibility.
107-107: Verify "nest_router" ACL user is configured in your Redis deployment.Line 107-108 uses a username of
"nest_router"alongside password-based authentication. Redis 6.0 (the first version to have ACLs) works exactly like older versions of Redis with backward compatibility, and redis-py automatically retries authentication with only the password if the server is below 6.0.0. However, ensure that if your Redis deployment uses ACLs (Redis 6.0+), the "nest_router" user is properly configured; if using requirepass-only authentication (no ACLs), verify that this username parameter doesn't conflict with your authentication setup.backend/apps/ai/router/intent.py (2)
96-97: Cache write failure handling is appropriate for fail-open design.Using
contextlib.suppress(Exception)for cache writes correctly implements the fail-open policy – classification continues even if Redis is unavailable. This aligns with the PR objectives.
19-32: spaCy integration correctly implements PR objectives with proper error handling.The implementation uses spaCy with the en_core_web_sm model as specified, and the try/except block provides a clear actionable message when the model is missing. Ensure the Docker build includes the model download step:
RUN python -m spacy download en_core_web_smVerify this is documented in deployment instructions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/MetricsCard.test.tsx (1)
96-96: Consistent date format change, but same scope concern applies.This second date format update ("Jan 1, 2024" → "1 Jan 2024") is consistent with the change on Line 46, confirming this was a deliberate update across the test suite. However, the same verification concerns apply: without seeing the MetricsCard component implementation, these test expectations cannot be validated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/MetricsCard.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/MetricsCard.test.tsx
This reverts commit a3c2555.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/apps/ai/router/client.py (1)
107-110: Consider consistent settings access pattern.The code mixes direct attribute access (
settings.REDIS_HOST,settings.REDIS_PASSWORD) withgetattr()pattern (line 109 forREDIS_USER). For consistency and to avoid potentialAttributeErrorif a setting is missing, consider usinggetattr()uniformly:host=getattr(settings, "REDIS_HOST", "localhost"), port=getattr(settings, "REDIS_PORT", 6379), username=getattr(settings, "REDIS_USER", "nest_router"), password=getattr(settings, "REDIS_PASSWORD", None),This makes the code more defensive and consistent, though the current approach works if you've verified all required settings exist.
docker-compose/redis.yaml (1)
3-3: Consider redis:alpine for ~95% smaller image size.The current
redis/redis-stack-server:7.4.0image is approximately 684MB, whileredis:7-alpineis about 30MB. Since the PR objectives indicate Redis is used solely for caching intent results (no mention of RedisJSON, RediSearch, or other Stack modules), the standard Redis image would be sufficient and significantly reduce image size, pull times, and deployment overhead.🔎 Alternative using standard Redis
- image: redis/redis-stack-server:7.4.0 + image: redis:7-alpineIf Redis Stack features are needed in the future, the image can be switched back.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/ai/router/client.pydocker-compose/redis.yaml
🔇 Additional comments (3)
backend/apps/ai/router/client.py (2)
1-10: LGTM: Clean module setup.The imports and logger initialization are appropriate for the Redis client implementation.
12-57: LGTM: Robust timeout validation.The validation logic properly handles type conversion errors, enforces bounds, and provides clear logging for operators. The default range (0.1s to 5.0s) is sensible for Redis operations.
docker-compose/redis.yaml (1)
13-17: Network and volume configuration looks good.The network and volume definitions are appropriate for local development and provide the necessary persistence for Redis data as intended by the PR objectives.
… Redis service configuration in docker-compose
…reation into dedicated methods
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/apps/ai/router/client.py (1)
119-119: Use %-formatting instead of f-string in logging.Per static analysis (Ruff G004), f-strings in logging statements are evaluated immediately even if the log level is disabled. Use
%-style formatting for lazy evaluation.🔎 Proposed fix
- logger.warning(f"Invalid REDIS_PORT value '{redis_port}', falling back to 6379.") + logger.warning("Invalid REDIS_PORT value '%s', falling back to 6379.", redis_port)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/ai/router/client.pybackend/apps/ai/router/limiter.pydocker-compose/redis.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose/redis.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/ai/router/limiter.py (1)
backend/apps/ai/router/client.py (2)
RedisRouterClient(60-137)get_connection(135-137)
🪛 Ruff (0.14.8)
backend/apps/ai/router/client.py
119-119: Logging statement uses f-string
(G004)
🔇 Additional comments (6)
backend/apps/ai/router/limiter.py (2)
18-26: Initialization looks good.The lazy initialization of
_script_shaand use ofRedisRouterClientfor connection management follows the established pattern.
59-68: Fail-open policy correctly implemented at the public API level.The broad exception catch with intentional
noqacomment ensures the rate limiter never blocks users due to Redis failures. The logiccurrent_count <= self.limitcorrectly allows the request when within limits.backend/apps/ai/router/client.py (4)
12-57: Timeout validation is well-implemented.The helper function properly validates, clamps to bounds, and logs warnings for out-of-range or invalid values. Good defensive programming.
60-81: Thread-safe pool initialization looks correct.Double-checked locking pattern properly guards the singleton pool creation, addressing the earlier review concern about race conditions.
83-99: Password validation addresses security concerns appropriately.The explicit check for
REDIS_ALLOW_NO_PASSWORDwith clear error messaging provides both security in production and flexibility in development.
135-137: Simple accessor is fine.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/ai/router/limiter.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/ai/router/limiter.py (1)
backend/apps/ai/router/client.py (2)
RedisRouterClient(60-137)get_connection(135-137)
🪛 Ruff (0.14.8)
backend/apps/ai/router/limiter.py
19-19: Probable use of insecure hash functions in hashlib: sha1
(S324)
39-39: Logging statement uses f-string
(G004)
61-61: Undefined name logging
(F821)
62-62: Logging statement uses f-string
(G004)
66-66: Undefined name logging
(F821)
67-67: Logging statement uses f-string
(G004)
🔇 Additional comments (2)
backend/apps/ai/router/limiter.py (2)
71-80: LGTM! Fail-open policy correctly implemented.The fail-open behavior (returning
Trueon any exception) aligns with the PR objectives to keep the system operational even when Redis fails. The rate limit check logic (current_count <= self.limit) is correct.
19-19: SHA1 usage is correct for Redis EVALSHA.The static analysis tool flags SHA1 as insecure (S324), but this is a false positive. Redis's
EVALSHAcommand requires SHA1 for script identification—this is not a cryptographic security use case. The hash is simply used to reference cached Lua scripts in Redis.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/apps/ai/router/limiter.py (3)
11-11: Move the import to the top of the file.The
from .client import RedisRouterClientimport should be grouped with the other imports at lines 4-7, before the module-level logger initialization. This follows PEP 8 conventions and resolves the E402 linting warning.🔎 Proposed refactor
import hashlib import logging import redis from redis.exceptions import RedisError + +from .client import RedisRouterClient logger = logging.getLogger(__name__) -from .client import RedisRouterClient - RATE_LIMIT_SCRIPT = """Based on static analysis hints, Ruff flagged E402 (Module level import not at top of file).
33-34: Consider making rate limit parameters configurable.The
limit(20 requests) andwindow(60 seconds) are hardcoded. For production flexibility, consider accepting these as constructor parameters with the current values as defaults, or reading them from Django settings.🔎 Proposed refactor
- def __init__(self): + def __init__(self, limit: int = 20, window: int = 60): """Initialize RateLimiter with Redis connection, limit, and window.""" self.redis = RedisRouterClient().get_connection() - self.limit = 20 - self.window = 60 + self.limit = limit + self.window = window
40-40: Use lazy string formatting in logging statements.Replace f-strings with
%formatting in logging calls for better performance (lazy evaluation). This applies to lines 40, 62, and 66.🔎 Proposed refactor
- logger.warning(f"Failed to load Redis script: {e}") + logger.warning("Failed to load Redis script: %s", e)Apply similar changes to lines 62 and 66:
logger.warning("Failed to eval/load Redis script: %s", inner_e) logger.warning("Redis evalsha error: %s", e)Based on static analysis hints, Ruff flagged G004 (Logging statement uses f-string) at lines 40, 62, and 66.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/ai/router/limiter.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/ai/router/limiter.py
🧬 Code graph analysis (1)
backend/apps/ai/router/limiter.py (1)
backend/apps/ai/router/client.py (2)
RedisRouterClient(60-137)get_connection(135-137)
🪛 Ruff (0.14.8)
backend/apps/ai/router/limiter.py
11-11: Module level import not at top of file
(E402)
22-22: Probable use of insecure hash functions in hashlib: sha1
(S324)
40-40: Logging statement uses f-string
(G004)
62-62: Logging statement uses f-string
(G004)
66-66: Logging statement uses f-string
(G004)
🔇 Additional comments (3)
backend/apps/ai/router/limiter.py (3)
43-45: LGTM!The script SHA getter is simple and correct, returning the value cached during initialization.
47-68: LGTM!The atomic increment implementation correctly uses the Lua script via
evalshawith comprehensive error handling:
- Catches
RedisErrorand other exception types as requested in previous reviews- Handles
NOSCRIPTerrors by falling back toevaland reloading the script- Implements fail-open behavior (returns 0 on error) consistent with the circuit breaker pattern described in the PR objectives
- Safe type conversion at line 68
70-79: LGTM!The rate limit check correctly implements the fail-open circuit breaker pattern:
- Returns
True(allow) on any exception, ensuring the system remains operational even if Redis fails- The logic
current_count <= self.limitcorrectly allows requests up to and including the limit- The broad exception catch with
BLE001suppression is intentional and properly documentedThis aligns perfectly with the PR objective to "implement a fail-open Circuit Breaker pattern so the system remains operational if Redis or the Router fails."
|
hi @arkid15r Ive worked on your and coderabbit's comments and have changed the files accordingly and run the tests and committed the changes. Please look it into it, I'd be more than happy to take up any improvements you suggest on the code! also, these 2 latest security hotspot reviews by sonar cloud testing are benign/harmless according to me, you may review them |
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/BreadCrumbs.tsx (2)
19-23: Missingmin-w-0for truncation to work properly.The
baseclass in line 20 lacksmin-w-0, which is present inBreadCrumbsWrapper.tsx(line 24). Withoutmin-w-0, flexbox items won't shrink below their minimum content size, preventing theTruncatedTextcomponent from truncating properly.🔎 Add min-w-0 to base class
itemClasses={{ - base: 'transition-colors duration-200', + base: 'transition-colors duration-200 min-w-0', item: 'text-sm font-medium', separator: 'flex items-center', }}
11-58: RemoveBreadCrumbRendereror consolidate withBreadCrumbsWrapper.
BreadCrumbRendereris unused in production code and only appears in tests.BreadCrumbsWrapperis the production component used inapp/layout.tsx. IfBreadCrumbRendereris retained as a standalone presentational component, addmin-w-0to itsitemClasses.base(currently missing, unlikeBreadCrumbsWrapperat line 24). Otherwise, removeBreadCrumbRendererand its test file, or consolidate by making a shared presentational component thatBreadCrumbsWrapperconsumes.
🧹 Nitpick comments (3)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (1)
326-339: Consider adding assertions for project leader badge output.The idempotency test currently only verifies staff badge output messages. For completeness, consider adding assertions for project leader output:
assert "Added badge to 0 project leaders" in out1.getvalue() assert "Removed badge from 0 non-project leaders" in out1.getvalue() assert "Added badge to 0 project leaders" in out2.getvalue() assert "Removed badge from 0 non-project leaders" in out2.getvalue()frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (1)
201-201: Optional: Remove extra blank line.Minor formatting nitpick - there's an extra blank line that could be removed for consistency.
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
528-528: Optional: Remove extra blank line.Minor formatting nitpick - there's an extra blank line that could be removed for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.github/workflows/run-ci-cd.yaml.github/workflows/update-nest-test-images.yamlbackend/apps/nest/management/commands/nest_update_badges.pybackend/tests/apps/nest/management/commands/nest_update_badges_test.pyfrontend/__tests__/unit/components/BreadCrumbs.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/package.jsonfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/organizations/[organizationKey]/page.tsxfrontend/src/app/page.tsxfrontend/src/components/BreadCrumbs.tsxfrontend/src/components/BreadCrumbsWrapper.tsxfrontend/src/components/Milestones.tsxfrontend/src/components/RecentIssues.tsxfrontend/src/components/RecentPullRequests.tsxfrontend/src/components/Release.tsxfrontend/src/components/SkeletonsBase.tsxfrontend/src/components/skeletons/MemberDetailsPageSkeleton.tsxfrontend/src/components/skeletons/OrganizationDetailsPageSkeleton.tsxfrontend/src/components/skeletons/sharedSkeletons.tsxfrontend/src/hooks/useSearchPage.tsfrontend/tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/RecentPullRequests.tsx
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-12-21T19:03:59.068Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1139
File: .github/workflows/setup-backend-environment/action.yaml:16-27
Timestamp: 2025-12-21T19:03:59.068Z
Learning: Composite actions (runs: using: composite) execute as steps within the calling job's context and can access the job context, including job.services.* properties (e.g., job.services.<service_id>.id, job.services.<service_id>.ports). Service containers must be defined at the job level, but a composite action's steps can reference them via the job context.
Applied to files:
.github/workflows/run-ci-cd.yaml.github/workflows/update-nest-test-images.yaml
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/app/page.tsxfrontend/src/components/BreadCrumbsWrapper.tsx
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/nest/management/commands/nest_update_badges.pybackend/tests/apps/nest/management/commands/nest_update_badges_test.py
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/components/BreadCrumbsWrapper.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/__tests__/unit/pages/OrganizationDetails.test.tsx
🧬 Code graph analysis (7)
frontend/src/components/skeletons/MemberDetailsPageSkeleton.tsx (1)
frontend/src/components/skeletons/sharedSkeletons.tsx (5)
PageWrapper(46-59)TitleSection(61-71)CardSection(113-129)SectionHeader(97-111)TwoColumnSection(73-95)
frontend/src/components/skeletons/OrganizationDetailsPageSkeleton.tsx (1)
frontend/src/components/skeletons/sharedSkeletons.tsx (5)
PageWrapper(46-59)TitleSection(61-71)CardSection(113-129)SectionHeader(97-111)TwoColumnSection(73-95)
frontend/src/app/page.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
backend/apps/nest/management/commands/nest_update_badges.py (2)
backend/apps/nest/models/user_badge.py (1)
UserBadge(10-50)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
frontend/src/components/BreadCrumbsWrapper.tsx (1)
frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
frontend/src/components/BreadCrumbs.tsx (1)
frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)
frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (1)
frontend/src/wrappers/testUtil.tsx (1)
render(19-19)
🔇 Additional comments (38)
frontend/src/components/Milestones.tsx (1)
56-56: LGTM! Icon sizing preserved in flex layout.Adding
shrink-0prevents the folder icon from compressing when the flex container is space-constrained, ensuring consistent icon dimensions.frontend/src/components/Release.tsx (1)
74-74: LGTM! Consistent icon sizing.The
shrink-0addition maintains consistent folder icon dimensions, matching the pattern applied across other components in this PR.frontend/src/components/RecentIssues.tsx (1)
36-36: LGTM! Consistent with broader UI updates.The
shrink-0addition aligns with the icon sizing pattern applied across Milestones, Release, and other components.frontend/src/app/page.tsx (5)
198-198: LGTM! Map marker icon sizing preserved.The
shrink-0addition to the event location icon prevents compression in the flex layout.
245-245: LGTM! Consistent icon sizing.The
shrink-0addition maintains map marker consistency with the event location icon at line 198.
285-288: LGTM! Icon wrapper sizing handled correctly.The
shrink-0addition toIconWrapperensures the dynamically selected project type icon maintains its dimensions in the flex layout.
365-365: LGTM! User icon sizing preserved.The
shrink-0addition to the author icon maintains consistent sizing in the post metadata flex layout.
1-406: Clarify the relationship between frontend UI changes and backend Intent Router implementation.The PR objectives describe implementing a Layer 1 Intent Router with spaCy, Redis caching, and circuit breaker patterns (backend functionality), but this file—and all other files in this review—contains only frontend UI changes (adding
shrink-0to icons).While these icon layout tweaks are technically correct, they appear unrelated to the stated PR goals. Consider:
- Are backend router implementation files missing from this review batch?
- Should these frontend UI changes be in a separate PR for better separation of concerns?
- Does bundling unrelated UI polish with a major backend feature complicate review and rollback if needed?
Please clarify the scope and organization of this PR.
frontend/tsconfig.json (1)
14-14: This configuration change is correct and standard for Next.js 16.The
"jsx": "preserve"setting is the recommended configuration for Next.js 13+ (including version 16.1.1). With this setting, Next.js itself handles JSX transformation, eliminating the need for explicitimport React from 'react'statements in components—which aligns with modern React patterns.Analysis of the codebase confirms this is working as expected: 145 TSX files exist in the project, with 119 explicitly importing React and 26 relying on Next.js's automatic JSX handling. Both patterns work correctly with the
"preserve"setting in Next.js 16.1.1.Likely an incorrect or invalid review comment.
backend/apps/nest/management/commands/nest_update_badges.py (5)
5-12: LGTM!The new imports are appropriate for the added functionality:
ContentTypefor generic foreign key lookups,EntityMemberfor querying project leaders, andProjectfor the content type model.
17-17: LGTM!The constant follows the established naming pattern and is correctly exported for use in tests.
84-95: LGTM!Badge creation follows the established pattern. The weight of 90 correctly places this badge below OWASP Staff (100) in display priority.
97-126: LGTM!The query correctly filters for active, reviewed project leaders with linked GitHub accounts. The badge assignment logic properly handles both new assignments and reactivation of previously deactivated badges.
128-153: LGTM!The badge removal logic correctly identifies users who are no longer project leaders and deactivates their badges. The approach of querying
UserBadgedirectly (rather than viaUser) is more efficient than the staff badge equivalent.backend/tests/apps/nest/management/commands/nest_update_badges_test.py (5)
8-11: LGTM!The import correctly brings in the new constant for use in test assertions.
35-45: LGTM!The helper correctly mocks the query chain:
filter() → distinct() → exclude()matching the actual implementation's ORM calls.
87-138: LGTM!The test correctly adds the necessary patches for the new project leader functionality while maintaining its focus on verifying staff badge behavior.
140-191: LGTM!Good test coverage for the project leader badge happy path, including badge creation parameters verification and assignment to leaders.
193-251: LGTM!The test correctly extends the mock setup to accommodate the new project leader badge flow while maintaining focus on verifying badge creation behavior.
frontend/src/hooks/useSearchPage.ts (1)
56-62: Nice refactor — clearer intent and consolidated logic.Extracting
searchQueryChangedandsortOrOrderChangedboolean flags improves readability, and the consolidated condition correctly implements the reset behavior described in the comment. Once the default value mismatch on lines 53–54 is fixed, this logic will work as intended.frontend/src/app/organizations/[organizationKey]/page.tsx (1)
45-50: LGTM! Skeleton loading UI properly implemented.The replacement of LoadingSpinner with OrganizationDetailsPageSkeleton wrapped in a container with a testid is a good improvement. This provides better visual feedback during loading and makes the component easier to test.
frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (1)
56-59: Good test update using semantic testid selector.The switch from
getAllByAltText('Loading indicator')togetByTestId('org-loading-skeleton')is more stable and aligns with the component changes.frontend/src/app/members/[memberKey]/page.tsx (1)
96-101: LGTM! Consistent skeleton loading pattern.The implementation mirrors the organization page pattern with MemberDetailsPageSkeleton and a testid wrapper, ensuring consistency across the codebase.
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
107-110: Good test update using semantic testid selector.The switch to
getByTestId('user-loading-skeleton')provides a more stable test query and aligns with the new skeleton loading UI.frontend/src/components/SkeletonsBase.tsx (1)
72-76: LGTM! Skeleton routing implemented correctly.The new switch cases properly route the
member-details,members, andorganizations-detailsindex names to their respective skeleton components. The fallback to LoadingSpinner remains intact for other cases.frontend/src/components/skeletons/MemberDetailsPageSkeleton.tsx (1)
11-161: Excellent skeleton component implementation.The MemberDetailsPageSkeleton is well-structured and comprehensive:
- Proper use of shared skeleton primitives for maintainability
- Good accessibility with
aria-hidden="true"on all skeleton elements- Responsive layout matching the actual page structure
- Includes all major sections: avatar, bio, heatmap, details, statistics, issues, milestones, PRs, releases, and repositories
frontend/src/components/skeletons/OrganizationDetailsPageSkeleton.tsx (1)
10-119: Excellent skeleton component implementation.The OrganizationDetailsPageSkeleton is well-structured and comprehensive:
- Proper use of shared skeleton primitives for code reuse
- Good accessibility with
aria-hidden="true"on all skeleton elements- Responsive grid layouts with appropriate breakpoints
- Includes all major sections: details, statistics, contributors, issues, milestones, PRs, releases, and repositories
- Clean use of
Array.from()for generating multiple skeleton itemsfrontend/src/components/skeletons/sharedSkeletons.tsx (1)
1-129: Excellent shared skeleton primitives library.This is a well-designed set of reusable skeleton components:
- Proper accessibility: All skeleton elements use
aria-hidden="true", and PageWrapper correctly usesaria-busy- Flexible configuration: Components accept props for customization (widths, heights, item counts, etc.)
- Good separation of concerns: Composable primitives that can be combined to build complex layouts
- Dark mode support: All components include dark mode styling
- Type safety: All props are properly typed with TypeScript
These primitives enable consistent skeleton loading UIs across the application.
frontend/__tests__/unit/components/BreadCrumbs.test.tsx (1)
52-58: LGTM! Test correctly updated for new DOM structure.The test now checks styling on the parent element, which correctly reflects the component changes where
TruncatedTextwraps the last breadcrumb's title in an additional span element.frontend/src/components/BreadCrumbsWrapper.tsx (3)
8-8: LGTM! Import required for truncation functionality.The
TruncatedTextimport is necessary for the new text truncation behavior added to breadcrumb items.
24-24: LGTM! Essential for flexbox truncation.Adding
min-w-0to the base class is critical for text truncation to work properly within flexbox layouts. Without it, flex items won't shrink below their content's minimum intrinsic size.
36-39: LGTM! Truncation implementation is sound.The
TruncatedTextwrapper correctly handles long breadcrumb titles with:
- Responsive width constraints (
max-w-xs sm:max-w-sm md:max-w-none)- Automatic tooltip via
titleattribute (set byTruncatedText)- Consistent application to both last and intermediate items
Also applies to: 46-49
frontend/src/components/BreadCrumbs.tsx (2)
5-5: LGTM! Import required for truncation functionality.The
TruncatedTextimport supports the new text truncation behavior for breadcrumb items.
35-38: LGTM! Truncation usage is correct.The
TruncatedTextwrapper properly handles long breadcrumb titles with responsive width constraints and automatic tooltips.Also applies to: 45-48
.github/workflows/run-ci-cd.yaml (1)
160-160: Clarify the necessity of this action update for the PR objectives.All instances of
docker/setup-buildx-actionhave been updated to commit8d2750c68a42422c14e847fe6c8ac0403b4cbd6f. While version updates are generally good practice, this change doesn't appear directly related to the stated PR objectives (implementing Layer 1 Intent Router with spaCy + Redis).Please confirm:
- Whether this action update is required for the new Redis/spaCy functionality
- If this should be in a separate infrastructure-focused PR
Also applies to: 192-192, 224-224, 280-280, 607-607
frontend/package.json (3)
31-32: Next.js ecosystem patch updates look good.The minor patch bump from 16.1.0 to 16.1.1 across the Next.js ecosystem packages is a standard maintenance update. These patch versions typically include bug fixes and minor improvements without breaking changes.
Also applies to: 52-52, 91-91
88-89: TypeScript ESLint patch updates look good.The minor patch bump from 8.50.0 to 8.50.1 for TypeScript ESLint packages is a standard maintenance update with minimal risk.
Also applies to: 113-113
31-32: No action required. Next.js 16.1.1 and React 19.2.3 are fully compatible—Next.js 16.x was designed to support React 19. React 19.2.3 is the recommended version and includes security patches for React Server Components. All listed dependencies in the update are compatible and properly versioned.
|
|
||
| - name: Set up Docker buildx | ||
| uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 | ||
| uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
docker/setup-buildx-action commit 8d2750c68a42422c14e847fe6c8ac0403b4cbd6f version
💡 Result:
Do you want the tag/release name that contains commit 8d2750c68a42422c14e847fe6c8ac0403b4cbd6f, or the action package version (e.g., v3.11.1)? I can look up the commit on GitHub and map it — confirm which you need.
🌐 Web query:
docker/setup-buildx-action security advisories vulnerabilities
💡 Result:
Short summary — known security advisories / vulnerabilities relevant to docker/setup-buildx-action (Buildx):
-
CVE-2025-0495 / GHSA-m4gq-fm9h-8q75 — secret leakage: Buildx cache-backend credential values set as attributes can be captured in OpenTelemetry traces and BuildKit daemon history (secrets disclosure). Published Mar 17, 2025. [1][2]
-
BuildKit-related high/critical CVEs disclosed Jan 2024 (e.g., CVE-2024-23651 and siblings) affecting BuildKit/runc/Moby that can impact builds or allow escapes when unpatched; Docker published coordinated advisories and patches. If you use setup-buildx-action, those BuildKit fixes are relevant. [3]
-
repo/security: the docker/setup-buildx-action repository points to Docker’s security policy and advises reporting privately; the action itself is a wrapper to configure Buildx — you should update to the latest action and Buildx/BuildKit versions and follow Docker advisories. [4]
Recommendations (brief):
- Update the GitHub Action to the latest docker/setup-buildx-action release.
- Ensure Buildx/BuildKit and Docker Engine/desktop are patched per Docker advisories.
- Avoid passing secrets in cache-to/cache-from attributes; use environment variables or GitHub cache backends when possible.
- Monitor Docker security announcements and the CVE/NVD entries for further updates.
Sources:
[1] CVE-2025-0495 entry (CVE details).
[2] Wiz vulnerability summary for CVE-2025-0495.
[3] Docker security advisory for BuildKit/runc/Moby (Jan 2024).
[4] docker/setup-buildx-action GitHub repository (security policy).
🌐 Web query:
docker/setup-buildx-action GitHub releases tags commit 8d2750c68a42422c14e847fe6c8ac0403b4cbd6f
💡 Result:
I couldn't find a release tag that matches commit 8d2750c68a42422c14e847fe6c8ac0403b4cbd6f in the docker/setup-buildx-action repository from public index/search results. The repo (tags/releases) and package versions are here for you to inspect: [docker/setup-buildx-action GitHub repo] [1], [Packages/versions] [2].
Do you want me to open the specific commit page on GitHub and check which tag (if any) references it?
Sources:
- https://github.com/docker/setup-buildx-action [1]
- https://github.com/orgs/docker/packages/container/package/setup-buildx-action [2]
🌐 Web query:
docker/setup-buildx-action latest version 2025 CVE-2025-0495 fix
💡 Result:
Short answer: The Buildx fix for CVE-2025-0495 was released in buildx v0.21.3 — pin setup-buildx-action to install buildx v0.21.3 (or any later buildx) via the action. [1][2]
What to do (example):
- Continue using docker/setup-buildx-action@v3 and set the version input to v0.21.3 (or later):
- with: version: v0.21.3
- Or omit the version input and ensure the runner provides a buildx >= v0.21.3.
References
- Debian / security tracker entry noting fix in v0.21.3. [1]
- docker/setup-buildx-action README (shows how to pin Buildx via the version input). [2]
Verify the docker/setup-buildx-action commit and address known security vulnerabilities.
The commit hash 8d2750c68a42422c14e847fe6c8ac0403b4cbd6f cannot be found in public releases of docker/setup-buildx-action. Additionally, CVE-2025-0495 (GHSA-m4gq-fm9h-8q75) — a secret leakage vulnerability in Buildx — affects this action. Ensure the workflow pins buildx to v0.21.3 or later by adding version: v0.21.3 (or a later version) in the action's with block to mitigate this vulnerability.
🤖 Prompt for AI Agents
In .github/workflows/update-nest-test-images.yaml around line 23, the action is
pinned to an unknown commit hash
(docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f) and is
vulnerable to CVE-2025-0495; replace the commit pin with an official version tag
and add a with block entry "version: v0.21.3" (or a later v0.21.x) to the
docker/setup-buildx-action invocation to ensure you use buildx v0.21.3+ and
mitigate the secret-leakage vulnerability.
| "eslint-config-prettier": "^10.1.8", | ||
| "eslint-import-resolver-alias": "^1.1.2", | ||
| "eslint-plugin-jest": "^29.5.0", | ||
| "eslint-plugin-jest": "^29.10.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
eslint-plugin-jest v29.10.1 breaking changes v29.5.0 changelog
💡 Result:
Short answer: v29.5.0 itself lists only a new rule (no-unneeded-async-expect-function) and does not introduce any breaking changes. The notable breaking changes for the 29.x line happened earlier (notably in 29.2.3) — dropped support for older ESLint, Node and @typescript-eslint versions and removal/changes to some rule docs/configs. See sources below.
Sources:
[1] eslint-plugin-jest releases (GitHub) — v29.2.3 breaking changes and v29.5.0 entry.
[2] eslint-plugin-jest package page (Yarn) — v29.5.0 changelog entry.
🌐 Web query:
eslint-plugin-jest v29.6 v29.7 v29.8 v29.9 v29.10 new rules changelog
💡 Result:
Do you want a short summary of the new rules added in eslint-plugin-jest versions 29.6–29.10, or the full changelog entries (with links)?
🌐 Web query:
eslint-plugin-jest new rules added versions 29.6 29.7 29.8 29.9 29.10 summary
💡 Result:
Summary of new rules added in eslint-plugin-jest v29.6 → v29.10 (from the GitHub releases):
- v29.6.0 — create new no-unnecessary-assertion rule. [1]
- v29.7.0 — create new no-error-equal rule. [1]
- v29.8.0 — create new valid-expect-with-promise rule. [1]
- v29.9.0 — no-unnecessary-assertion: added support for toBeNaN (enhancement to the rule introduced in 29.6). [1]
- v29.10.0 — valid-title: ensure printf specifiers in .each titles are valid (feature/validation added to valid-title). [1]
(Also: 29.6.1 and 29.10.1 were follow-up bugfix releases.) [1]
Source: eslint-plugin-jest GitHub releases. [1]
Verify ESLint configuration handles new jest rules from v29.10.1 update.
The update from 29.5.0 to 29.10.1 introduces three new linting rules (no-unnecessary-assertion, no-error-equal, valid-expect-with-promise) and enhancements to existing rules. While no breaking changes exist in this version range, please ensure that if any of these rules are enabled in your ESLint configuration, they don't cause unexpected test failures or require additional config tuning. Given the prior jest configuration needs in this repo, verify the rules align with your test practices.
🤖 Prompt for AI Agents
In frontend/package.json around line 94, the jest plugin was bumped to 29.10.1
which adds three new rules (no-unnecessary-assertion, no-error-equal,
valid-expect-with-promise) and enhancements; check your ESLint config
(.eslintrc.js/.eslintrc.json or package eslintConfig) and test overrides to see
if these rules are enabled and if they conflict with existing test patterns,
then either explicitly configure each rule under the "overrides" for test files
(enable/disable or set desired severity) or pin them to the expected setting for
this repo, run npm/yarn lint and tests to surface any failures, and adjust rules
or test code accordingly so linting remains consistent with current test
practices.
| const sortByParam = searchParams.get('sortBy') || 'default' | ||
| const orderParam = searchParams.get('order') || 'desc' |
There was a problem hiding this comment.
Fix default value inconsistency with state initialization.
The hardcoded defaults 'default' and 'desc' don't match the defaults used when initializing state (lines 44–45: defaultSortBy and defaultOrder props). When the component is instantiated with different defaults (e.g., defaultSortBy='created_at'), the comparison on line 57 will incorrectly flag sortOrOrderChanged as true, triggering an unintended page reset.
🔎 Proposed fix
- const sortByParam = searchParams.get('sortBy') || 'default'
- const orderParam = searchParams.get('order') || 'desc'
+ const sortByParam = searchParams.get('sortBy') || defaultSortBy
+ const orderParam = searchParams.get('order') || defaultOrder📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sortByParam = searchParams.get('sortBy') || 'default' | |
| const orderParam = searchParams.get('order') || 'desc' | |
| const sortByParam = searchParams.get('sortBy') || defaultSortBy | |
| const orderParam = searchParams.get('order') || defaultOrder |
🤖 Prompt for AI Agents
In frontend/src/hooks/useSearchPage.ts around lines 53 to 54, the searchParams
fallback uses hardcoded values ('default' and 'desc') which are inconsistent
with the component's state initialization (defaultSortBy and defaultOrder);
change the fallback to use the defaultSortBy and defaultOrder variables (or
their derived values) so the comparison that sets sortOrOrderChanged uses the
same defaults as state, preventing false positives and unintended page resets.
3 participants
Proposed change
Resolves #2991
This PR implements Layer 1 (Intent Router) of the Unified Search Architecture.
Currently, every user query triggers a full LLM inference or vector search, leading to higher latency and costs for trivial questions. This PR introduces a deterministic routing layer that:
spaCyto detect specific entities (e.g., "repo", "license").Technical Implementation:
apps.ai.router): ImplementedIntentRouterusingspaCy(en_core_web_sm). It classifies queries intoSTATIC(deterministic) orDYNAMIC(requires LLM).DYNAMIC(Hybrid Search) to prevent crashing the user request.Verification:
backend/tests/test_router.pyto verify happy paths, ambiguity handling (low confidence scores), and circuit breaker failure modes.Checklist
make check-testlocally and all tests passed