wulf-pulse/.planning/codebase/CONCERNS.md

Codebase Concerns

Analysis Date: 2026-05-03

Critical Database Access Pattern

Widespread Pool Instantiation:

• Issue: Multiple new Pool instances created directly in files instead of using the postgresClient singleton
• Files:
  • app/admin/users/[id]/page.tsx (line 11)
  • app/admin/roles/[id]/page.tsx (line 7)
  • app/api/data/time-entries/route.ts (line 6)
  • app/api/admin/audit-log/route.ts (line 5)
  • app/api/admin/settings/route.ts (line 6)
  • app/api/admin/users/route.ts (line 5)
  • app/api/admin/users/[id]/route.ts (line 5)
  • app/api/admin/users/[id]/sessions/route.ts (line 5)
  • app/api/admin/users/[id]/sessions/[sessionId]/route.ts (line 5)
  • app/api/admin/users/invite/route.ts (line 6)
  • app/api/admin/roles/route.ts (line 5)
  • app/api/admin/roles/[id]/route.ts (line 5)
  • app/api/rmm-devices/route.ts (line 13)
  • app/api/openclaw/datto-rmm/devices/route.ts (line 6)
  • app/api/openclaw/datto-rmm/devices/[uid]/route.ts (line 6)
  • app/api/openclaw/datto-rmm/alerts/route.ts (line 6)
  • app/api/openclaw/datto-rmm/sites/route.ts (line 6)
  • app/api/openclaw/datto-rmm/alerts/open/route.ts (line 6)
  • app/api/addigy/org-mappings/route.ts (line 6)
  • app/api/settings/profile/route.ts (line 5)
  • app/api/auvik/tenant-mappings/route.ts (line 6)
  • app/api/rmm/site-mappings/route.ts (line 6)
  • lib/bootstrap.ts (line 3)
  • lib/auth.ts (line 9)
  • lib/services/audit.ts (line 3)
  • lib/services/auvik-client.ts (line 216)
• Impact: Each Pool() call creates a new connection pool, consuming resources and database connections. In production with multiple instances, this can exhaust connection limits. No centralized control over connection pooling.
• Fix approach: Replace all instances with postgresClient singleton from lib/services/postgres-client.ts. The singleton implements lazy initialization and reuses the same pool. Create a migration script to audit all imports and replace new Pool() with postgresClient imports.

Duplicate Migration Numbers

Out-of-Order Migrations:

• Issue: Multiple migrations share the same number prefix, causing alphabetical apply order to diverge from intent
• Files: migrations/
• Duplicates found:
  • 002_add_indexes.sql and 002_relax_foreign_keys.sql
  • 004_fix_contacts_company_id.sql and 004_webhook_support.sql
  • 005_add_webhook_ip_logging.sql and 005_fix_tickets_company_id.sql
  • 009_create_auvik_tenant_mappings.sql, 009_relax_configuration_items_constraints.sql, 009_restore_deleted_tickets.sql
  • 028_create_device_lifecycle_policies.sql and 028_create_veeam_agents_alarms.sql
  • 049_create_ping_flap_suppressions.sql and 049_create_ticket_digest_tables.sql
  • 057_contacts_missing_fields.sql and 057_create_autotask_tags_tables.sql
  • 058_create_duo_tables.sql and 058_create_repo_commit_tracking.sql
• Impact: Alphabetical filesystem sort determines execution order (not numeric). On Postgres init, migrations apply in ASCII order. New environment setups may fail if a constraint or schema change in one migration depends on another with the same number. This is a hidden fragility.
• Fix approach: Rename all duplicate migrations to sequential numbers (e.g., 009_create_auvik_tenant_mappings.sql, 010_relax_configuration_items_constraints.sql, 011_restore_deleted_tickets.sql). Verify that the alphabetical apply order would work for existing databases. Test full init sequence on fresh Postgres. Update deployment docs to warn against re-numbering.

Extensive console.log in Production Code

Unstructured Logging:

• Issue: console.log() statements left throughout production code instead of proper logging framework
• Files with multiple instances:
  • lib/utils/sync-helpers.ts (lines 344, 417, 441)
  • lib/services/redis-client.ts (lines 7, 34, 95)
  • lib/services/background-processor.ts (lines 185, 224, 255, 272, 319, 327, 335)
  • lib/services/duo-sync-service.ts (lines 53, 64, 70, 95, 139, 591)
  • lib/services/workflow-engine.ts (lines 104, 221, 326, 664)
  • lib/services/duo-client.ts (line 165)
  • lib/services/itglue-sync-service.ts (line 54)
  • lib/services/veeam-sync-service.ts (lines 91, 112, 127)
  • lib/services/webhook-service.ts (lines 71, 76, 110, 164, 193, 233, 429)
  • lib/services/veeam-factory.ts (line 30)
  • lib/services/veeam-compliance-service.ts (lines 25, 50, 190, 191)
• Impact: Logs go directly to stdout, not aggregated to a logging service. In production, container logs are hard to filter and correlate. No structured metadata (timestamp in some, not others; inconsistent prefixes). Search for "WEBHOOK" or "VEEAM-SYNC" is the only way to filter.
• Fix approach: Create a simple logger module at lib/logger.ts with methods .info(), .warn(), .error() that preserve prefixes but add timestamps and structure. Replace all console.log() with logger.info() etc. Consider using pino or winston if needed in future, but start with a lightweight wrapper.

Tech Debt: Worker Side-Effect Imports

Hidden Auto-Initialization:

• Issue: lib/services/sync-scheduler.ts, lib/services/analyzer/worker.ts, and lib/services/rmm/worker.ts auto-start on import as a side effect
• Files:
  • lib/services/sync-scheduler.ts (bottom of file)
  • lib/services/analyzer/worker.ts (auto-starts in production or if ANALYZER_WORKER_AUTOSTART=1)
  • lib/services/rmm/worker.ts (same gates as analyzer)
• Impact: Importing these modules from shared utilities (e.g., a shared auth helper) will start worker loops unexpectedly. If a hot path accidentally imports one, it runs polling immediately. The analyzer worker uses SELECT … FOR UPDATE SKIP LOCKED so it's safe across instances, but the sync scheduler does NOT — running it on multiple instances causes duplicate syncs.
• Fix approach: Add comments on all three modules warning against hot-path imports. Consider a factory pattern: initSyncScheduler(), startAnalyzerWorker() with explicit function calls instead of side effects. Or gate them behind a feature flag that must be explicitly enabled. Document in CLAUDE.md that only app/api/*/route.ts handlers should import these. Add a linting rule if possible.

Type Safety in Shared Types

Excessive use of any:

• Issue: Shared type definitions use any for workflow/pipeline configuration and error handling
• Files:
  • lib/types/ticket-workflow.ts (lines 48, 78, 84, 100, 104, 107 — field_changes, condition values, validation errors)
  • lib/types/pipeline.ts (lines 34, 128 — stage values, context dict)
  • lib/types/workflow.ts (lines 108, 111, 113, 139, 193, 207, 243, 245, 273, 293-294, 358, 361, 363, 384 — match_value, result_value, field_changes)
  • lib/types/datto-rmm.ts (lines 105, 141, 146-147 — autotaskDevice, field mapping)
  • lib/types/errors.ts (lines 148, 229, 241 — error categorization and formatting)
• Impact: Configuration values for workflow conditions/actions are not validated at the type level. A malformed workflow rule with match_value: 123 (number instead of string/regex) will only fail at runtime. Error handling functions that accept any can mask type errors silently.
• Fix approach: Use TypeScript discriminated unions or explicit types for workflow values. For errors, define a proper error interface and use type guards. Start with lib/types/workflow.ts since it's most critical for the workflow engine. Consider adding a validation layer that runs on workflow rule creation.

Auth Validation Gap

Middleware-only Session Check:

• Issue: middleware.ts (lines 75-82) only verifies a session cookie exists, does NOT check user role for /admin routes
• Files: middleware.ts
• Impact: Role-based access control is entirely at the API route level via requireAdmin() or requirePermission(). If a developer forgets to call requireAuth() or requireAdmin() in an API route, the middleware won't catch it. A mistake like returning user data without checking role is a privilege escalation. The comment on line 76 acknowledges this.
• Fix approach: Add a helper function enforceRole() that is harder to forget than calling requireAdmin() early in a route handler. Better: make it so unauthenticated users can't even reach admin routes (redirect in middleware if no admin role detected — requires decoding the session token). Or add a lint rule that checks all API handlers for requireAuth calls. At minimum, add a test that verifies at least 10 admin API routes call requireAdmin() or requirePermission().

Missing Test Coverage

Limited Test Suite:

• Issue: Only lib/services/analyzer/**, lib/services/rmm/**, and lib/services/b2/** have unit tests; the rest of the codebase relies on TypeScript type-checking only
• Files:
  • Test files: lib/services/analyzer/*.test.ts (11 test files), lib/services/rmm/*.test.ts (2 test files), lib/services/b2/*.test.ts (1 test file), lib/services/llm/*.test.ts (2 test files)
  • No tests for: sync services, webhook handlers, API routes, auth flows, workflow engine logic, integrations (Autotask, IT Glue, Veeam, etc.)
• Impact: Refactoring core services like entity-sync.ts (1438 lines) or webhook-service.ts cannot be validated. Breaking changes in Autotask mapping logic, webhook handling, or sync schedules are only caught in staging. The NO CI note in ARCHITECTURE.md means no automated regression detection.
• Fix approach: Start with high-impact areas: lib/services/entity-sync.ts and lib/services/webhook-service.ts. Add unit tests covering the main sync paths and webhook processing. Set a coverage target of 50% for critical services. Add pre-commit hook that runs npm test to prevent untested code from being committed.

Large Files with Complex Logic

Size and Complexity Hotspots:

• Issue: Several service files exceed 1000+ lines, indicating potential refactoring opportunities
• Files:
  • lib/services/entity-sync.ts (1438 lines) — Main Autotask sync, many entity types
  • lib/services/workflow-engine.ts (954 lines) — Workflow execution and Autotask write-back
  • lib/services/ticket-digest-service.ts (782 lines) — Daily digest generation
  • lib/services/sync-scheduler.ts (760 lines) — Background task scheduling
  • lib/services/analyzer/asset-audit/data-builder.ts (719 lines) — Evidence aggregation for IT Glue audits
  • lib/services/veeam-rpo-service.ts (675 lines) — Veeam RPO logic
  • lib/services/mimecast-client.ts (675 lines) — Mimecast API client
  • lib/services/itglue-sync-service.ts (663 lines) — IT Glue integration
• Impact: Large files are harder to reason about, test, and refactor. Bug fixes in entity-sync.ts might accidentally affect the sync of a different entity type if the logic isn't clearly separated. The workflow engine's 954 lines likely interleaves execution logic, Autotask API calls, and error handling.
• Fix approach: Extract smaller modules from these files. For example, in entity-sync.ts, separate sync logic per entity into sync/tickets.ts, sync/contacts.ts, etc. In ticket-digest-service.ts, split template rendering into separate files. This is a gradual refactoring — start with workflow-engine.ts since it has the most potential for breaking bugs.

Unvalidated Input in API Routes

No Zod Validation Framework:

• Issue: API routes do not systematically validate request bodies or query parameters
• Files: All app/api/**/route.ts files
• Impact: Endpoints accept any JSON and only validate inputs "when it matters" (per CLAUDE.md). An admin form endpoint could silently ignore a malformed field instead of returning a 400 Bad Request. Developers must remember to manually validate each input; forgetting is easy.
• Fix approach: Add a lightweight input validation helper (not necessarily Zod, but something). For example, a simple function like validateInput(req.body, schema) that checks required fields and types. Use it in high-risk endpoints: user creation, role assignment, workflow rules, integration settings. Start with /api/admin/* routes.

Known TODOs

Incomplete Implementations:

• Issue: Active TODOs left in production code
• Files:
  • lib/services/sync-service.ts (line 444) — "TODO: Implement graceful cancellation"
  • lib/services/workflow-steps/ai-troubleshooting.ts (line 40) — "TODO: Implement createTicketNote in AutotaskClient if needed"
  • app/api/veeam/backup-status/route.ts (line 51) — "TODO: compute from config items without matching workloads"
• Impact: sync-service.ts sync cancellation is not implemented — if a sync is running and needs to be stopped (e.g., on pod termination), it will run to completion. This can delay graceful shutdown. The Veeam backup status compute is a stub returning 0.
• Fix approach: Prioritize graceful cancellation in sync-service. For the others, either implement them or remove the TODOs if the current behavior is acceptable. Add a CI check that fails on new TODOs (optional but helpful).

Multi-Instance Sync Scheduler Risk

Sync Scheduler Not Instance-Safe:

• Issue: lib/services/sync-scheduler.ts uses node-cron but does NOT use row locking like the analyzer worker does
• Files: lib/services/sync-scheduler.ts
• Impact: In a multi-instance deployment, every instance will run every scheduled sync at the same time. If the sync scheduler is running on 3 replicas, Autotask gets 3 sync requests simultaneously, which wastes API quota and can cause race conditions on the Postgres side. The ARCHITECTURE.md (line 256) warns: "pin to one instance."
• Fix approach: Either (a) force the sync scheduler to run on a single instance only by setting an env flag like RUN_SYNC_SCHEDULER=1 and defaulting it to false on replicas, or (b) add a distributed lock (e.g., in Postgres with advisory locks) so only one instance's cron fires. Test multi-instance behavior in staging before deploying.

Analyzer Cost Ceiling Enforcement

Opacity in Stage 4 Skipping:

• Issue: Analyzer Stage 4 (Opus) is skipped if estimated cost exceeds $2.00 (line 138 in ARCHITECTURE.md), flagged for human review, but no clear UI/alert when this happens
• Files: lib/services/analyzer/pipeline.ts, lib/services/llm/pricing.ts
• Impact: An analysis with a cost ceiling hit is marked with a flag, but there's no alerting mechanism to tell admins that a ticket analysis was incomplete due to cost. The ticket analysis might be silently insufficient for the user.
• Fix approach: Add a cost-ceiling alert row to analyzer_analyses or a separate cost-alert table. Expose this in the admin UI at /admin/analyzer with a filter for "cost-ceiling alerts." Log a structured event with logger.warn() so it's visible in centralized logs.

IT Glue Redaction Mandatory But Not Enforced

Redaction Bypass Risk:

• Issue: IT Glue search results MUST be redacted before being sent to an LLM (line 87-89 in ARCHITECTURE.md), but there's no type-system enforcement
• Files: lib/services/analyzer/itglue-search.ts (redacted output), lib/services/analyzer/ (callers)
• Impact: A developer could accidentally import itglue-client.ts (the raw client) and pass results directly to an LLM prompt, exposing credentials or PII. The redaction is documented but optional in code.
• Fix approach: Make the raw IT Glue client non-exported from its module, forcing all LLM-bound queries through the redacted search function. Add a type wrapper like RedactedDocument that is the only type accepted by LLM callers. Or add a pre-commit hook that scans for itglue-client imports in analyzer files and warns.

.env File Committed to Repo

Potential Secrets Exposure:

• Issue: .env file exists at /opt/stacks/pulse/.env and is NOT in .gitignore, but git ls-files shows no .env files tracked
• Files: /opt/stacks/pulse/.env
• Impact: Although the .env file is not currently tracked in git, it exists on the filesystem with potentially real configuration. The .gitignore pattern .env* should prevent accidental commits, but if someone edits .gitignore or adds --force, secrets could leak. This is a human-error risk.
• Fix approach: Verify that no real secrets are in the committed .env file. Document that .env* is gitignored and point developers to .env.example. Add a pre-commit hook with detect-secrets or similar to catch hardcoded secrets. Rotate any keys/tokens that might have been exposed in historical runs.

Missing Graceful Shutdown for Workers

Worker Cleanup on Pod Termination:

• Issue: The analyzer worker (line 40 in ARCHITECTURE.md) resets stale in-flight jobs on boot, but there is no graceful shutdown handler for long-running operations
• Files: lib/services/analyzer/worker.ts, lib/services/sync-scheduler.ts, lib/services/rmm/worker.ts
• Impact: If a worker is in the middle of processing and the container is killed, that job is left in a partially-complete state (e.g., partial analyzer analysis, incomplete RMM execution). On pod restart, the worker resets jobs but may lose partial work.
• Fix approach: Implement a SIGTERM handler that stops accepting new jobs, finishes in-flight work, then exits cleanly. Use process.on('SIGTERM', async () => { ... }). Set Kubernetes terminationGracePeriodSeconds to allow time for cleanup. Log completion of final jobs.

Pool Connection Leaks from Page Components

Server Component Pool Usage:

• Issue: Page components like app/admin/users/[id]/page.tsx and app/admin/roles/[id]/page.tsx create Pools without closing them, relying on garbage collection
• Files: app/admin/users/[id]/page.tsx, app/admin/roles/[id]/page.tsx
• Impact: Each page render creates a new Pool(). In development with fast reloads, pools accumulate. While they will eventually be garbage-collected, this is inefficient and can cause "too many connections" errors in development if tests run fast enough.
• Fix approach: Use the postgresClient singleton instead (which is already lazy-initialized). This is the same fix as the broader pool instantiation issue above. Pages should import postgresClient from lib/services/postgres-client.ts rather than creating new Pools.

Performance: Large Page Sizes

Pagination Defaults Not Optimized:

• Issue: qbo-client.ts uses a pageSize of 1000 (line 187), which is high for API response sizes
• Files: lib/services/qbo-client.ts
• Impact: A single API call fetching 1000 QuickBooks objects can be slow and memory-intensive. If the endpoint returns large objects, response times spike.
• Fix approach: Reduce default page size to 100-250 and let callers opt in for larger pages if needed. Measure API response times for the most common queries and set pageSize accordingly.

Idempotency Key Complexity

Provider-Scoped Content Hash Subtle:

• Issue: Analyzer content_hash is provider-scoped (per ARCHITECTURE.md), meaning the same ticket can have one Claude analysis and one OpenRouter analysis, but this is easy to miss
• Files: lib/services/analyzer/pipeline.ts (lines 239, 253), lib/services/analyzer/worker.ts (lines 181, 314)
• Impact: If a developer switches the default LLM provider from Anthropic to OpenRouter and re-runs an analysis with force=false, the code will treat it as a new analysis because the provider is part of the idempotency key. This is correct behavior but non-obvious and could confuse operators.
• Fix approach: Add a comment at the storage point explaining that provider is part of the idempotency key. Document in CLAUDE.md or ARCHITECTURE.md that switching providers intentionally allows re-analysis. Add a debug endpoint that shows all analyses for a ticket grouped by provider.

Broken env Pattern in Datto RMM Sync

Optional API URL with Fallback:

• Issue: lib/services/datto-rmm-sync-service.ts (line 38) has a fallback hardcoded URL: process.env.DATTO_RMM_API_URL || 'https://concord-api.centrastage.net'
• Files: lib/services/datto-rmm-sync-service.ts
• Impact: The hardcoded fallback is correct (Datto's standard endpoint), but this pattern is inconsistent with other integrations which throw if env vars are missing. If an env var is unset by mistake, it silently uses the public endpoint instead of failing loudly.
• Fix approach: Check if DATTO_RMM_API_URL should be required or optional. If optional, document why. If required, remove the fallback and let the code throw. Audit other clients for similar silent fallbacks.

---

Concerns audit: 2026-05-03