make anon mode work with placeholders

[jstjoe]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
sky-mcp-streamable	Ready	Preview, Comment	Feb 4, 2026 10:41pm

[github-actions]

🔐 Gitleaks Findings: 8 issue(s) detected

🔸 Rule: generic-api-key
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: generic-api-key
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: jwt
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: generic-api-key
📄 File: tests/unit/middleware/authenticateBearer.test.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/authenticateBearer.test.ts:null

🔸 Rule: jwt
📄 File: tests/unit/middleware/authenticateBearer.test.ts:null
📝 Description: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/authenticateBearer.test.ts:null

🔸 Rule: jwt
📄 File: tests/unit/middleware/authenticateBearer.test.ts:null
📝 Description: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/authenticateBearer.test.ts:null

🔸 Rule: jwt
📄 File: tests/unit/middleware/authenticateBearer.test.ts:null
📝 Description: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/authenticateBearer.test.ts:null

🔸 Rule: stripe-access-token
📄 File: tests/unit/middleware/authenticateBearer.test.ts:null
📝 Description: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/authenticateBearer.test.ts:null

[claude]

Pull Request Review: Anonymous Mode Placeholder Fallback

Overview

This PR adds intelligent fallback to anonymous mode when query parameters contain unsubstituted template placeholders (e.g., ${SKYFLOW_VAULT_ID}). This is a practical enhancement that handles misconfiguration gracefully.

---

✅ Strengths

1. Well-Designed Solution

• The looksLikePlaceholder() function is pure, well-documented, and covers 4 common placeholder patterns
• Comprehensive regex patterns handle shell (${VAR}), direct ($VAR), mustache ({{VAR}}), and Windows (%VAR%) styles
• Edge cases are properly handled (embedded placeholders, partial patterns, etc.)

2. Excellent Test Coverage

• 103 lines of new tests with 29 test cases covering all placeholder patterns
• Tests are well-organized by placeholder style and include edge cases
• Tests verify both positive cases (placeholders detected) and negative cases (valid values not flagged)
• Follows existing test patterns in the codebase

3. Documentation

• CLAUDE.md updated with clear explanation of the fallback behavior
• Lists all detected placeholder patterns with examples
• Inline JSDoc comments are thorough with multiple examples

4. Minimal Impact

• Changes are focused and contained to the specific feature
• No breaking changes to existing functionality
• Graceful degradation when anonymous mode isn't configured

---

🔍 Issues & Suggestions

1. Potential State Mutation Bug ⚠️

Severity: Medium

In src/server.ts:646-647, the code mutates request object properties that were previously set by the authenticateBearer middleware:

req.skyflowCredentials = { apiKey: anonApiKey };
req.anonVaultConfig = { vaultId: anonVaultId, vaultUrl: anonVaultUrl };

Issues:

• The authenticateBearer middleware already sets these properties based on the initial authentication state
• Overwriting req.skyflowCredentials could cause issues if the original credentials were valid but the user simply had placeholder values in query params
• This creates a "second authentication path" that bypasses the middleware's intended flow

Recommendation:
Consider extracting the anonymous mode configuration logic into a separate middleware or helper function that runs before the main request handler. This would make the authentication flow clearer and prevent state mutation.

Impact: Low risk in practice since the condition hasPlaceholderParams && !req.isAnonymousMode means we only mutate when there were no valid credentials initially, but the pattern is confusing.

2. Missing Integration Tests ⚠️

Severity: Low-Medium

While the unit tests for looksLikePlaceholder() are excellent, there are no tests for the actual fallback behavior in the /mcp endpoint.

Suggested test cases:

• Request with placeholder vaultId and anonymous mode configured → should use anonymous mode
• Request with placeholder vaultId but no anonymous mode configured → should return error
• Request with valid credentials + placeholder query params → should use provided credentials (not fall back)
• Request with one placeholder and one valid param → should trigger fallback

Recommendation:
Add integration tests in a file like tests/integration/anonymousModeFallback.test.ts that test the actual HTTP endpoint behavior.

3. Logging Could Be More Informative

Severity: Low

The log message at src/server.ts:642-644 is good but could provide more debugging context:

console.log(
  "Detected placeholder values in vaultId/vaultUrl query params, falling back to anonymous mode"
);

Suggestion:

console.log(
  `Detected placeholder values in query params (vaultId: ${queryVaultId}, vaultUrl: ${queryVaultUrl}), falling back to anonymous mode`
);

This helps with debugging by showing exactly which values triggered the fallback.

4. Edge Case: Mixed Valid + Placeholder

Severity: Low

The current logic uses OR condition:

const hasPlaceholderParams =
  looksLikePlaceholder(queryVaultId) || looksLikePlaceholder(queryVaultUrl);

This means if a user provides:

• vaultId=abc123 (valid)
• vaultUrl=${SKYFLOW_VAULT_URL} (placeholder)

The system will fall back to anonymous mode and ignore the valid vaultId. This might be the intended behavior, but it's worth documenting or reconsidering.

Recommendation:
Document this behavior in CLAUDE.md or consider only falling back if BOTH are placeholders (though the current behavior is probably safer).

---

🔒 Security Review

✅ No Security Issues Found

• Regex patterns are properly anchored with ^ and $ to prevent bypass attacks
• No command injection risks (pure string validation)
• Anonymous mode still respects rate limiting (good!)
• No credentials are logged (placeholder values are logged, which is fine)

---

🚀 Performance

✅ Minimal Performance Impact

• Four regex checks per request (only when no credentials provided initially)
• Regex patterns are simple and efficient (no backtracking risks)
• Checks happen before Skyflow SDK initialization, so no wasted API calls

---

📊 Code Quality

Strengths:

• ✅ TypeScript types are correct
• ✅ Pure functions with clear inputs/outputs
• ✅ Consistent with existing code patterns
• ✅ Good separation of concerns (validation logic in separate module)

Minor Improvements:

• Consider extracting magic strings like environment variable names into constants
• The conditional logic in server.ts:635-649 could be extracted into a helper function for readability

---

Summary

Recommendation: Approve with minor suggestions

This is a well-implemented feature that solves a real UX problem. The core logic is sound, tests are comprehensive, and there are no major security concerns.

Before merging, consider:

1. ✅ Must fix: None - code is functional as-is
2. ⚠️ Should add: Integration tests for the endpoint behavior
3. 💡 Nice to have: Enhanced logging with actual placeholder values
4. 💡 Consider: Document mixed valid/placeholder behavior

Test Status: Tests should pass (unit tests are comprehensive). Would be good to verify with pnpm test before merge.

Great work on this feature! 🎉

[github-actions]

🔐 Gitleaks Findings: 3 issue(s) detected

🔸 Rule: generic-api-key
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: generic-api-key
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: generic-api-key
📄 File: tests/unit/middleware/authenticateBearer.test.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/authenticateBearer.test.ts:null

[github-actions]

🔐 Gitleaks Findings: 3 issue(s) detected

🔸 Rule: generic-api-key
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: generic-api-key
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: generic-api-key
📄 File: tests/unit/middleware/authenticateBearer.test.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/authenticateBearer.test.ts:null

[claude]

Pull Request Review: Make Anonymous Mode Work with Placeholders

Overview

This PR adds intelligent fallback to anonymous mode when query parameters contain unsubstituted template placeholders (like ${SKYFLOW_VAULT_ID}). This is a practical UX improvement that handles misconfigured MCP client URLs gracefully.

---

✅ Strengths

1. Comprehensive Test Coverage

• Excellent unit tests for looksLikePlaceholder() covering all 4 placeholder patterns
• Tests include edge cases (empty strings, partial patterns, embedded placeholders)
• Good test organization with descriptive test groups

2. Well-Documented

• Clear JSDoc comments with examples
• CLAUDE.md updated with detailed explanation of the feature
• Good inline comments explaining the logic

3. Clean Implementation

• Pure function for placeholder detection - easy to test and reason about
• Proper regex patterns that avoid false positives
• Non-intrusive changes to existing flow

4. Security Additions

• Added gitleaks:allow comments to prevent false positives on test JWTs/API keys

---

🔍 Issues & Concerns

1. CRITICAL: Missing Integration Tests

The placeholder detection logic in server.ts:635-649 is not covered by any tests. This is the core business logic that orchestrates the fallback behavior, but there are no integration tests verifying:

• ✗ Does the server actually fall back to anonymous mode when placeholders are detected?
• ✗ Are the correct credentials (anonApiKey) set on the request?
• ✗ Is useAnonymousMode properly propagated to the request context?
• ✗ What happens when placeholders are present but anonymous mode is not configured?

Recommendation: Add integration tests for the /mcp endpoint that verify this flow end-to-end.

2. Potential Logic Issue: Silent Failure

In server.ts:635-649, if placeholders are detected but anonymous mode environment variables are not configured, the code silently continues with the placeholder values:

if (hasPlaceholderParams && !req.isAnonymousMode) {
  const anonApiKey = process.env.ANON_MODE_API_KEY;
  const anonVaultId = process.env.ANON_MODE_VAULT_ID;
  const anonVaultUrl = process.env.ANON_MODE_VAULT_URL;

  if (anonApiKey && anonVaultId && anonVaultUrl) {
    // Fall back to anonymous mode
    console.log("Detected placeholder values...");
    useAnonymousMode = true;
    req.skyflowCredentials = { apiKey: anonApiKey };
    req.anonVaultConfig = { vaultId: anonVaultId, vaultUrl: anonVaultUrl };
  }
  // If anonymous mode not configured, we just continue with placeholders!
}

This means requests with vaultId=${SKYFLOW_VAULT_ID} will pass through and likely fail later in validateVaultConfig() with a confusing error message.

Recommendation: Add an else branch that returns a helpful error:

if (anonApiKey && anonVaultId && anonVaultUrl) {
  // ... fall back to anonymous mode
} else {
  return res.status(400).json({
    error: "Configuration error: Query parameters contain unsubstituted placeholders (e.g., ${SKYFLOW_VAULT_ID}). Either substitute these with actual values or configure anonymous mode via ANON_MODE_* environment variables."
  });
}

3. Regex Pattern Concerns

Variable Naming Restrictions:
The regex /^\$\{[A-Z_][A-Z0-9_]*\}$/i requires variables to start with a letter or underscore, which correctly prevents ${123} but might miss legitimate edge cases:

• ${_VAULT_ID} - starts with underscore (currently allowed, which is good)
• ${VAULT-ID} - contains hyphen (currently rejected, which may be intentional)

This seems reasonable, but consider if there are any common variable naming patterns being excluded.

Case Sensitivity:
The /i flag makes patterns case-insensitive, so ${vault_id} and ${VAULT_ID} are both detected. This is good for robustness.

Potential False Positives:
The patterns are strict (anchored with ^ and $), so they won't match:

• Partial placeholders: https://${CLUSTER}.vault.skyflowapis.com ✓ Good
• Multiple placeholders: ${VAR1}_${VAR2} ✓ Good

Test coverage confirms this behavior (lines 314-317).

4. Minor: Logging Quality

The log message at server.ts:642-644 could be more actionable:

console.log(
  "Detected placeholder values in vaultId/vaultUrl query params, falling back to anonymous mode"
);

Recommendation: Include which specific parameters contained placeholders:

console.log(
  `Detected placeholder values (vaultId: ${queryVaultId}, vaultUrl: ${queryVaultUrl}), falling back to anonymous mode`
);

5. Edge Case: accountId/workspaceId Placeholders

The code only checks vaultId and vaultUrl for placeholders (server.ts:630-633), but users might also provide placeholders for accountId or workspaceId. These would pass through undetected.

Question: Is this intentional? Should these also trigger the placeholder detection?

---

📋 Code Quality

Positive:

• ✅ Follows project conventions (pure functions, JSDoc, TypeScript types)
• ✅ Proper error handling patterns
• ✅ Clean separation of concerns (validation logic separate from server logic)
• ✅ Consistent with existing codebase patterns

Minor Issues:

• The test file has consistent gitleaks:allow additions, which is good for security scanning

---

🚀 Performance Considerations

• Regex operations are fast (O(n) where n is string length)
• No performance concerns with this implementation
• Minimal overhead added to request processing

---

🔒 Security Considerations

Positive:

• ✅ Placeholder detection prevents credential leakage (users won't accidentally send ${API_KEY} as a literal value)
• ✅ Falls back to anonymous mode (rate-limited, restricted functionality)
• ✅ No injection vulnerabilities in regex patterns

Concerns:

• ⚠️ The silent failure case (issue Add Claude Code GitHub Workflow #2 above) could lead to confusing errors that might cause users to troubleshoot incorrectly

---

📝 Recommendations Summary

Must Fix (Blocking):

1. Add integration tests for the server endpoint placeholder detection flow
2. Handle the case when placeholders are detected but anonymous mode is not configured - return a clear error instead of silently continuing

Should Fix (Non-blocking but important):
3. Consider checking accountId/workspaceId for placeholders as well
4. Improve logging to show which specific values contained placeholders

Nice to Have:
5. Add a comment in server.ts explaining why only vaultId/vaultUrl are checked (if that's intentional)

---

✅ Overall Assessment

This is a well-implemented feature with excellent unit test coverage and clear documentation. The main concerns are:

1. Missing integration tests for the actual fallback behavior
2. Silent failure when placeholders are detected but anonymous mode isn't configured

Once these issues are addressed, this will be a solid improvement to the user experience.

Recommendation: Request changes to add integration tests and better error handling before merging.

[claude]

Pull Request Review: Placeholder Value Fallback to Anonymous Mode

I've completed a thorough review of PR #16. This is a well-implemented feature that adds graceful fallback handling for unsubstituted template placeholders. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Problem Identification

The feature addresses a real UX issue where users configure MCP server URLs with environment variable templates (like ${SKYFLOW_VAULT_ID}) that don't get substituted by their client. The automatic fallback to anonymous mode is intuitive.

2. Robust Placeholder Detection

The looksLikePlaceholder() function in src/lib/validation/vaultConfig.ts:30-46 handles multiple templating styles:

• Shell-style: ${VAR_NAME}
• Direct env var: $VAR_NAME
• Mustache/Handlebars: {{VAR_NAME}}
• Windows: %VAR_NAME%

The regex patterns are well-thought-out and properly anchored with ^ and $ to match only complete placeholder strings.

3. Comprehensive Test Coverage

The test suite in tests/unit/validation/vaultConfig.test.ts:219-320 is excellent:

• 103 lines of new tests
• Covers all four placeholder styles
• Tests edge cases (empty strings, undefined, partial patterns, embedded placeholders)
• Tests false positives (actual vault IDs/URLs, UUIDs, regular strings)

4. Clean Implementation

The server logic in src/server.ts:629-649 follows the existing code patterns:

• Respects the req.isAnonymousMode state
• Only activates when placeholders are detected AND anonymous mode is configured
• Uses useAnonymousMode local variable consistently
• Provides clear console logging for debugging

5. Documentation

CLAUDE.md was properly updated with a clear explanation of the feature, including all supported placeholder patterns.

---

🔍 Issues & Concerns

1. Missing Integration Tests (High Priority)

While the unit tests for looksLikePlaceholder() are comprehensive, there are no integration tests for the server-level placeholder detection logic.

Recommended additions:

// tests/integration/server-placeholder-fallback.test.ts
describe('Placeholder Fallback to Anonymous Mode', () => {
  it('should fall back to anonymous mode when vaultId contains ${PLACEHOLDER}', async () => {
    // Test request with ?vaultId=${SKYFLOW_VAULT_ID}&vaultUrl=${SKYFLOW_VAULT_URL}
  });
  
  it('should NOT fall back when already in anonymous mode', async () => {
    // Test that req.isAnonymousMode=true prevents duplicate fallback
  });
  
  it('should return 400 when placeholders detected but anonymous mode not configured', async () => {
    // Test graceful degradation when ANON_MODE_* env vars are missing
  });
  
  it('should handle mixed scenarios (placeholder in vaultId, real value in vaultUrl)', async () => {
    // Test partial placeholder detection
  });
});

2. Potential Logic Gap: No Fallback When Anonymous Mode Not Configured

In src/server.ts:635-649, when placeholders are detected but anonymous mode env vars are missing, the code silently continues without fallback. This could lead to confusing 400 errors later.

Current behavior:

if (hasPlaceholderParams && !req.isAnonymousMode) {
  // ... check anon env vars
  if (anonApiKey && anonVaultId && anonVaultUrl) {
    // Fall back to anon mode
  }
  // If env vars missing, nothing happens - continues with placeholder values
}

Suggested improvement:
Add an else clause to provide explicit error messaging when placeholders are detected but anonymous mode is not configured. This would return a 400 response with actionable guidance about either substituting the values or configuring anonymous mode.

3. Minor: Console Logging Could Be More Informative

The log message at src/server.ts:642-644 could include which placeholders were detected for better debugging.

4. Regex Pattern Consideration

The regex in src/lib/validation/vaultConfig.ts:34 requires variable names to start with a letter or underscore: [A-Z_][A-Z0-9_]*.

This correctly rejects invalid patterns like ${123VAR}. Consider if you want to require minimum length (2+ characters) to reject edge cases like ${_}, though the current implementation is acceptable.

---

🛡️ Security Considerations

✅ Good: No Security Issues Identified

• The placeholder detection doesn't introduce any injection vulnerabilities
• The anonymous mode fallback reuses existing, validated credential handling
• Rate limiting still applies to anonymous mode requests
• No new attack surface introduced

---

⚡ Performance Considerations

✅ Good: Minimal Performance Impact

• Placeholder detection happens once per request before expensive operations
• Regex patterns are simple and will execute quickly
• No additional network calls or I/O

---

📝 Code Quality

✅ Good: Follows Project Conventions

• Pure functions with clear JSDoc comments
• Consistent error handling patterns
• Type-safe implementation
• Export pattern matches existing code
• gitleaks suppression comments added appropriately

---

🎯 Recommendations Summary

Must-Have (Before Merge):

1. Add integration tests for the server-level placeholder detection logic
2. Add explicit error handling when placeholders are detected but anonymous mode isn't configured

Nice-to-Have:

3. Enhance console logging to show which placeholders were detected
4. Consider minimum variable name length in regex (2+ characters)

---

📊 Overall Assessment

Rating: 8/10 - Solid implementation with excellent unit tests, but missing integration tests and explicit error handling for the "placeholders detected but no anon mode" scenario.

The feature itself is valuable and the core logic is sound. With the addition of integration tests and improved error messaging, this would be merge-ready.

Great work on the comprehensive test coverage for the placeholder detection function! The regex patterns are well-designed and the documentation is clear.

[github-actions]

🔐 Gitleaks Findings: 3 issue(s) detected

🔸 Rule: generic-api-key
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: generic-api-key
📄 File: src/lib/middleware/authenticateBearer.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: src/lib/middleware/authenticateBearer.ts:null

🔸 Rule: generic-api-key
📄 File: tests/unit/middleware/authenticateBearer.test.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/authenticateBearer.test.ts:null

[claude]

Code Review: Make anon mode work with placeholders

Summary

This PR adds intelligent placeholder detection to automatically fall back to anonymous mode when users provide unsubstituted template variables (like ${SKYFLOW_VAULT_ID}) in query parameters. The implementation is well-designed with excellent test coverage.

---

✅ Strengths

1. Excellent Test Coverage

• Comprehensive unit tests for looksLikePlaceholder() covering all 4 placeholder patterns
• Edge cases thoroughly tested (undefined, empty string, partial patterns, embedded placeholders)
• Tests for valid values to prevent false positives
• Great use of descriptive test names and grouping

2. Clean Architecture

• Pure function looksLikePlaceholder() is testable and reusable
• Well-documented with JSDoc comments and examples
• Separation of concerns: validation logic lives in dedicated module

3. User Experience

• Helpful error messages when placeholders detected but anonymous mode not configured
• Logging for debugging (console.log when falling back to anonymous mode)
• Supports 4 common placeholder formats (shell, direct, mustache, Windows)

4. Security

• Added // gitleaks:allow comments to prevent false positives from secret scanners
• No new security vulnerabilities introduced

---

🔍 Issues & Concerns

1. Missing Integration Tests (HIGH PRIORITY)

The PR lacks integration/E2E tests for the new server behavior in src/server.ts:635-654. The unit tests only cover looksLikePlaceholder(), but do not verify:

• The full request flow when placeholders are detected
• That useAnonymousMode is properly set
• That req.skyflowCredentials and req.anonVaultConfig are correctly populated
• The 400 error response when placeholders detected but anonymous mode unavailable

Recommendation: Add integration tests that verify the complete request flow with placeholder detection.

2. Potential Logic Issue: Double Anonymous Mode Assignment (MEDIUM)

In src/server.ts:646-647, when placeholders are detected:

req.skyflowCredentials = { apiKey: anonApiKey };
req.anonVaultConfig = { vaultId: anonVaultId, vaultUrl: anonVaultUrl };

But these properties may have already been set by authenticateBearer middleware when req.isAnonymousMode === true. The code does not check if these are already set, potentially overwriting them.

Question: What happens if:

1. User provides no credentials (middleware sets anonymous mode)
2. User also provides placeholder query params

Recommendation: Add a comment explaining the relationship, or add defensive checks to clarify intent.

3. Regex Pattern Strictness (LOW)

The regex patterns in vaultConfig.ts:34-43 use case-insensitive matching (/i flag). The current implementation is quite good, but consider whether $VAR (without braces) should really be treated as a placeholder, as this could match legitimate values like $USD or file paths in some contexts.

Recommendation: The current patterns are reasonable for the use case. Consider documenting the rationale in a comment explaining that vault IDs/URLs are unlikely to match these patterns, so the risk of false positives is acceptable.

4. Error Message Could Be More Specific (LOW)

The error message at src/server.ts:650-652 mentions "contact the developer if anonymous mode is not working" but does not explain how to configure anonymous mode.

Recommendation: Enhance the error message to include both client-side and server-side configuration steps.

5. Documentation Update - Missing Anonymous Mode Limitation (LOW)

The CLAUDE.md update documents the placeholder fallback feature well, but does not mention that this fallback only works if anonymous mode is properly configured on the server.

Recommendation: Add a note clarifying that anonymous mode must be configured via ANON_MODE_* environment variables for the fallback to work.

---

🎯 Minor Suggestions

1. TypeScript Type Safety
Consider extracting the placeholder patterns into a constant array for reusability and maintainability.

2. Performance Consideration
The placeholder check runs on every request. The condition at line 635 (if (hasPlaceholderParams && !req.isAnonymousMode)) already optimizes this appropriately. Current implementation is fine for typical load.

---

🧪 Testing Recommendations

1. Add integration tests for the server request flow
2. Add stress test for placeholder detection with various malformed inputs
3. Add test for the specific scenario: user with valid credentials accidentally passes placeholder in query param (should use their credentials, not fall back to anonymous)

---

📋 Checklist for Approval

• ✅ Code quality: Well-structured, readable, follows project conventions
• ✅ Unit test coverage: Excellent for looksLikePlaceholder()
• ⚠️ Integration test coverage: Missing tests for server.ts changes
• ✅ Security: No new vulnerabilities, proper secret scanner annotations
• ✅ Performance: Minimal impact, efficient regex patterns
• ✅ Documentation: CLAUDE.md updated appropriately
• ⚠️ Error handling: Could be more informative

---

🎬 Verdict

Conditional Approval - This is a well-implemented feature with excellent unit tests. However, before merging:

Required:

1. Add integration tests for the server-level placeholder detection and fallback logic

Recommended:

1. Clarify/comment the relationship between middleware-set anonymous mode and placeholder-detected anonymous mode
2. Enhance error message to explain anonymous mode configuration
3. Add note to CLAUDE.md about anonymous mode requirement

Nice to have:

1. Refactor regex patterns into constant array for better maintainability
2. Document rationale for $VAR pattern inclusion

Great work overall! The feature solves a real UX problem and the implementation is clean and testable. The main gap is integration test coverage for the server request flow.