Deploy to demo

[github-actions]

Automated PR to sync changes from main to demo.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 5ad2a72

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Code Review - PR #1: Deploy to demo

Overview

This automated PR syncs changes from main to demo, including a significant organizational migration from @gsa-tts to @flexion namespaces, along with workflow updates and infrastructure changes.

Stats: +1,168 additions / -625 deletions across 103 files

---

🔴 Critical Issues

1. Commented-out Typecheck in CI Pipeline

Location: .github/workflows/_validate.yml:99-107

The TypeScript type checking has been commented out:

# - name: Typecheck source code
#   shell: bash
#   run: pnpm typecheck

Impact: This disables critical type safety validation in CI, which could allow type errors to be merged into the codebase.

Recommendation: This should be temporary only. Create a tracking issue to re-enable type checking and fix any blocking issues. If this is intentional due to migration issues, add a comment explaining why and when it will be restored.

2. Disabled Pre-commit Tests

Location: .husky/pre-commit:4

Tests have been disabled in the pre-commit hook:

echo "*** NOTE: Running tests is temporarily disabled ***"
#pnpm test:ci

Impact: Developers can commit code without running tests locally, increasing the risk of broken code reaching CI/CD.

Recommendation: Add a TODO with a tracking issue number. Consider at minimum running fast unit tests even if integration tests are skipped.

3. Disabled Deployment Workflow

Location: .github/workflows/deploy.yml:19-26

The entire deployment job is commented out:

# deploy:
#   needs: [build-image]
#   uses: ./.github/workflows/_terraform-apply.yml

Impact: Docker images are built but never deployed, which could be confusing and wasteful of CI resources.

Recommendation: Either remove the build-image job if deployment is disabled, or document why deployment is disabled and when it will be re-enabled.

4. Missing NPM_TOKEN in Release Workflow

Location: .github/workflows/release.yml:53

#NPM_TOKEN: ${{ secrets.NPM_TOKEN }}

Impact: If packages are configured to publish to GitHub Package Registry, the commented NPM_TOKEN may cause silent failures or unexpected behavior.

Recommendation: Verify whether this token is needed for GitHub Package Registry publishing. If using publishConfig.registry, you may need GITHUB_TOKEN instead.

---

⚠️ Major Concerns

5. Inconsistent Package Scope Migration

Multiple packages still reference @gsa-tts in their CHANGELOGs despite being renamed to @flexion:

• apps/sandbox/CHANGELOG.md:1 - Header still says @gsa-tts/forms-server-doj
• apps/server-doj/CHANGELOG.md:1 - Same issue
• apps/spotlight/CHANGELOG.md:1 - Says @gsa-tts/forms-spotlight

Recommendation: Update all CHANGELOG.md headers to match the new package names for consistency.

6. Increased Memory Allocation Without Documentation

Location: .github/workflows/_terraform-plan-pr-comment.yml:75

run: NODE_OPTIONS=--max-old-space-size=6144 pnpm turbo run --filter @flexion/forms-infra-cdktf build

Impact: Memory limit increased from default (~1.7GB) to 6GB without explanation.

Recommendation: Add a comment explaining why this is needed. Consider investigating the root cause of high memory usage.

7. Disabled E2E Tests in CI

Location: .github/workflows/run-tests.yml:12-14

# e2e:
#   uses: ./.github/workflows/_end-to-end.yml
#   secrets: inherit

Impact: No end-to-end testing coverage in CI pipeline.

Recommendation: Track with an issue and ensure E2E tests are re-enabled before production deployments.

8. Terraform Plan PR Comments Disabled

Location: .github/workflows/add-terraform-plan-to-pr.yml:4

Changed from automatic PR triggers to manual workflow_dispatch only.

Impact: Teams lose automatic infrastructure change visibility on PRs.

Recommendation: Document why this was changed. Consider re-enabling for better infrastructure review workflow.

---

📝 Code Quality Observations

9. Node Version Update

Location: .nvmrc:1

Updated from v22.14.0 → v22.19.0

✅ Good: Staying current with LTS updates. Ensure this version is tested and documented in any deployment guides.

10. New Claude Code Integration

Location: .github/workflows/claude.yml & claude-code-review.yml

✅ Good: Automated code review integration added. Well-structured with appropriate permissions.

⚠️ Note: The claude-code-review.yml workflow has commented-out filters that could be useful:

• Path-based filtering (lines 6-10)
• Author-based filtering (lines 16-19)

Consider uncommenting and configuring these if you want to limit when Claude reviews run.

11. Package Registry Configuration

All packages now include:

"publishConfig": {
  "registry": "https://npm.pkg.github.com"
}

✅ Good: Explicit registry configuration prevents accidental publishing to public npm.

⚠️ Note: Ensure all consumers have proper authentication configured for GitHub Package Registry.

12. Consistent Import Path Updates

The migration from @gsa-tts/* to @flexion/* appears complete across:

• TypeScript/JavaScript imports ✅
• Package.json dependencies ✅
• Workflow scripts ✅

Recommendation: Run a final grep to ensure no references to @gsa-tts remain:

grep -r "@gsa-tts" --exclude-dir=node_modules --exclude="*.md" .

---

🔒 Security Considerations

13. No Security Issues Identified

• No hardcoded credentials detected
• No sensitive data in changesets
• Appropriate use of GitHub secrets
• Proper permission scoping in workflows

✅ All Clear

---

🧪 Test Coverage

14. Test Coverage Status

With multiple testing mechanisms disabled:

• ❌ Pre-commit tests disabled
• ❌ E2E tests disabled in CI
• ❌ Type checking disabled
• ✅ Unit tests still run (via pnpm test)

Impact: Significantly reduced safety net for catching regressions.

Recommendation: Create a tracking epic/issue to systematically re-enable all testing layers with clear acceptance criteria.

---

📊 Performance Considerations

15. Build Performance

No significant performance regressions expected from these changes. The memory increase for CDKTF build suggests potential optimization opportunities in infrastructure code generation.

---

✅ Recommendations Summary

Before Merging:

1. ✅ Add GitHub issue references to all commented-out CI steps
2. ✅ Update CHANGELOG.md headers to use new package scopes
3. ⚠️ Verify GitHub Package Registry authentication is configured
4. ⚠️ Run manual E2E and type checking to ensure no regressions

Post-Merge Action Items:

1. 🎯 High Priority: Re-enable type checking in CI
2. 🎯 High Priority: Re-enable pre-commit tests
3. 🎯 Medium Priority: Re-enable E2E tests
4. 🎯 Medium Priority: Re-enable deployment workflow (if needed for demo env)
5. 📝 Document why Terraform plan comments were moved to manual dispatch

Long-term:

• Investigate CDKTF memory usage issue
• Consider enabling path/author filters for Claude Code reviews

---

Final Verdict

⚠️ Conditional Approval - This PR represents a significant organizational migration that appears well-executed, but has several critical CI/CD safety features disabled.

Approve if: These disabled features are intentional for the demo environment and tracked appropriately.

Request changes if: These are accidental or lack proper tracking/timeline for re-enablement.

---

Review generated by Claude Code 🤖

[claude]

Code Review - PR #1: Deploy to demo

This is a large sync PR (331 files changed) merging changes from main to demo. I've focused my review on the most significant additions and changes.

🎯 Major Changes Identified

1. Claude Code Integration - New GitHub Actions workflows
2. LLM Request Cache - New database table and schema
3. CLI Form Management - New form import/export commands
4. UI Enhancements - AvailableFormList refactoring
5. Node Version Update - v22.18.0 → v22.19.0

---

✅ Strengths

1. GitHub Actions Workflows

• .github/workflows/claude-code-review.yml and .github/workflows/claude.yml are well-structured
• Proper permissions scoping (read-only access where appropriate)
• Smart conditional execution using contains() for @claude mentions
• Good use of commented examples for optional configurations

2. Database Migration

• packages/database/migrations/20251004224805_llm_request_cache.mjs follows proper migration patterns
• Includes both up() and down() for rollback support
• Proper indexing on accessed_at and created_at for cache management
• TypeScript types properly added to packages/database/src/clients/kysely/types.ts

3. CLI Commands

• apps/cli/src/cli-controller/forms.ts provides useful form management utilities
• Good separation of concerns with parser abstraction

---

⚠️ Issues & Concerns

1. Security - Potential Secret Exposure (HIGH)

File: .github/workflows/claude-code-review.yml:38

claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}

• The workflow runs on pull_request: [opened, synchronize] without any author filtering
• Currently commented out filters on lines 15-19, but these should be uncommented and configured
• Risk: External contributors could trigger this workflow with access to secrets
• Recommendation: Enable author filtering or use pull_request_target with strict checkout practices

2. Database Schema - Missing Constraints

File: packages/database/migrations/20251004224805_llm_request_cache.mjs

Issue 1: cache_key size may be insufficient

table.string('cache_key', 64).notNullable().unique();

• 64 characters might be too short for complex cache keys (especially hashed keys)
• Consider using SHA-256 hashes (64 hex chars) or longer keys
• Recommendation: Document the expected format or increase to 128+

Issue 2: No TTL/expiration mechanism

• The table has created_at and accessed_at but no explicit expiration
• Risk of unbounded cache growth
• Recommendation: Add expires_at column or document the cache eviction strategy

Issue 3: response_data stored as TEXT

table.text('response_data').notNullable();

• No size limit on LLM responses could lead to database bloat
• Recommendation: Consider JSONB (Postgres) for structure + compression, or add size limits

3. TypeScript Type Mismatch

File: packages/database/src/clients/kysely/types.ts:82

accessed_at: Date;  // Not Generated<Date>

• accessed_at should be Generated<Date> to match the migration's .defaultTo(knex.fn.now())
• Current type doesn't reflect the database default
• Fix:

accessed_at: Generated<Date>;

4. React Component Issues

File: packages/design/src/AvailableFormList/index.tsx

Issue 1: Missing key uniqueness guarantee (line 107)

forms.map((form, index) => (
  <tr key={index}>

• Using array index as key is an anti-pattern
• Should use form.id instead
• Fix: <tr key={form.id}>

Issue 2: Unnecessary dependency in useEffect (line 38)

useEffect(() => {
  loadForms();
}, [location.pathname, location.hash, location.key, loadForms]);

• location.hash and location.key seem unnecessary
• Over-triggering could cause performance issues
• Question: Is there a specific reason to reload on hash changes?

Issue 3: Debug tool in production code (lines 161-174)

const DebugTools = () => {
  return (
    <button onClick={() => {
      console.warn('clearing localStorage...');
      window.localStorage.clear();
      window.location.reload();
    }}>
      Delete all demo data (clear browser local storage)
    </button>
  );
};

• This should be conditionally rendered based on environment
• Recommendation: Only show in development/demo environments

5. CLI Error Handling

File: apps/cli/src/cli-controller/forms.ts

Issue 1: Silent failure on PDF parse error (line 34)

if (maybeForm === undefined) {
  console.error('Error parsing PDF file:', inputFile);
  return;  // No exit code!
}

• Should exit with non-zero status: process.exit(1);

Issue 2: No error handling for file operations (lines 31, 45)

const pdfBytes = await fs.readFile(inputFile);
const fileContents = await fs.readFile(inputFile);

• Missing try-catch blocks
• Recommendation: Wrap in try-catch with user-friendly error messages

---

🔍 Missing Test Coverage

Based on the files changed, I don't see corresponding test files for:

• ✗ packages/database/migrations/20251004224805_llm_request_cache.mjs
• ✗ apps/cli/src/cli-controller/forms.ts
• ✗ packages/design/src/AvailableFormList/index.tsx (refactored, tests should be updated)

Recommendation: Add integration tests for:

1. LLM cache CRUD operations
2. CLI form import/export workflows
3. AvailableFormList rendering and user interactions

---

📊 Performance Considerations

1. LLM Cache Lookup Performance

• Indexes on accessed_at and created_at are good
• Consider adding partial index for active cache entries:

  CREATE INDEX idx_llm_cache_active 
  ON llm_request_cache(cache_key) 
  WHERE accessed_at > NOW() - INTERVAL '7 days';

2. Form List Reloading

• AvailableFormList reloads on every location change (including hash)
• Consider debouncing or more specific triggers

---

📝 Documentation

Missing Documentation:

1. No documentation for the LLM request cache feature
   • What is cached? When is it invalidated?
   • Cache eviction policy?
2. No documentation for new CLI commands
   • forms import-pdf and forms add need usage examples
3. CLAUDE.md is well-written but should mention the new cache table

---

🎯 Priority Recommendations

CRITICAL (Fix before merge):

1. ✋ Fix GitHub Actions security issue - enable author filtering
2. 🔧 Fix TypeScript type for accessed_at in LlmRequestCacheTable
3. 🔑 Fix React key prop to use form.id instead of index

HIGH (Should fix):
4. Add cache expiration strategy to LLM cache table
5. Add error handling to CLI file operations
6. Add exit codes to CLI error paths

MEDIUM (Consider):
7. Add tests for new features
8. Document LLM cache behavior
9. Conditionally render DebugTools component
10. Review cache_key length (64 chars might be insufficient)

---

📚 General Notes

• The PR description mentions "Automated PR to sync changes from main to demo" - this suggests it's a deployment sync rather than new feature development
• Most changes appear to be version bumps and changelog updates
• The substantive changes (Claude workflows, LLM cache, CLI tools) are well-structured but need the fixes mentioned above

Overall Assessment: The code quality is generally good, but there are several important issues that should be addressed, particularly around security, type safety, and error handling.

---

Review generated by Claude Code 🤖

[claude]

PR Review: Deploy to demo (Main → Demo Sync)

Overview

This is a large automated sync PR (336 files, +5388/-3491 lines) merging changes from main to demo. Given the scope, I've focused on the most significant additions and potential concerns.

Key Additions & Observations

✅ Positive Changes

1. Documentation Improvements

• CLAUDE.md, AGENTS.md, DOCS.md: Excellent additions providing AI agent guidance and documentation index
• ADR 0018 (Documentation Strategy): Well-structured decision record with clear layered architecture
• documents/patterns-and-conventions.md & documents/quick-reference.md: Valuable reference materials
• Clear, scannable structure optimized for both humans and AI agents

2. GitHub Workflows for AI Integration

• .github/workflows/claude-code-review.yml: Automated PR reviews using Claude
• .github/workflows/claude.yml: Issue/comment-triggered Claude assistance
• Both workflows have appropriate permissions scoped down (contents: read, pull-requests: read)
• Good use of conditional triggers to avoid unnecessary runs

3. LLM Request Caching

• packages/database/migrations/20251004224805_llm_request_cache.mjs: New table for caching AI responses
• packages/forms/src/llm/cache/database.ts: Clean implementation with async stats tracking
• Properly indexed (accessed_at, created_at) for performance
• Uses onConflict for upserts - good practice

4. CLI Enhancements

• apps/cli/src/cli-controller/forms.ts: New form management commands
• Good separation of concerns with import-pdf and add commands

⚠️ Areas of Concern

1. Security - GitHub Workflow Permissions

# .github/workflows/claude-code-review.yml:26
id-token: write

• The id-token: write permission should only be needed for OIDC federation (AWS, GCP, Azure)
• Question: Is this actually required for the Claude Code action? If not, remove it following principle of least privilege
• The claude_args restricts tools to safe gh commands, which is good

2. Migration Missing TypeScript Types
The database migration adds llm_request_cache table, but I see incomplete type coverage:

• ✅ packages/database/src/clients/kysely/types.ts:19 adds the table to Database interface
• ⚠️ Missing exports for LlmRequestCacheSelectable, LlmRequestCacheInsertable, LlmRequestCacheUpdateable types
• The existing tables (Users, Sessions, Forms) all export these type helpers - should be consistent

3. Node Version Bump

# .nvmrc
-v22.18.0
+v22.19.0

• Minor version bump should be safe, but verify CI environments have this version
• Ensure Docker base images and deployment environments are updated accordingly

4. Error Handling in CLI

// apps/cli/src/cli-controller/forms.ts:34
if (maybeForm === undefined) {
  console.error('Error parsing PDF file:', inputFile);
  return;
}

• Error handling exists but doesn't provide actionable feedback
• Should log/return actual error details to help users debug PDF parsing failures
• Consider: process.exit(1) to signal failure to shell

5. Database Cache Implementation - Race Condition

// packages/forms/src/llm/cache/database.ts:24-27
this.updateAccessStats(key).catch(error => {
  console.warn('Failed to update cache access stats:', error);
});

• ⚠️ Fire-and-forget async update could lead to inaccurate access_count under load
• While gracefully handled, consider documenting that counts are "best effort"
• Alternative: Use database triggers or batch updates if accuracy is critical

6. Test Coverage
For major new features (LLM caching, CLI commands), I don't see corresponding test files in the PR:

• Missing: packages/forms/src/llm/cache/database.test.ts
• Missing: apps/cli/src/cli-controller/forms.test.ts
• The project has strong testing conventions (see CLAUDE.md) - new code should follow suit

📋 Recommendations

1. Security: Review and remove id-token: write permission if not required
2. Types: Add missing TypeScript helper types for llm_request_cache table
3. Testing: Add unit tests for:
   • DatabaseCache class (get/set/clear/updateAccessStats)
   • CLI form commands (import-pdf, add)
4. Error Handling: Improve CLI error messages with actual error details
5. Documentation: Document LLM cache behavior (TTL policy, eviction strategy, "best effort" access counts)
6. Node Version: Verify deployment environments support Node 22.19.0

🔍 Performance Considerations

The LLM cache implementation looks efficient:

• ✅ Proper indexing on query columns
• ✅ Async stats updates don't block reads
• ✅ Uses single-row upsert pattern

Potential future optimization:

• Consider adding TTL/expiration mechanism to prevent unbounded growth
• Add cache eviction policy (LRU based on accessed_at?)

🎯 Code Quality

Overall code quality is high:

• ✅ Consistent TypeScript usage
• ✅ Clear separation of concerns
• ✅ Good use of type safety (Kysely)
• ✅ Follows project conventions (kebab-case files, named exports)
• ⚠️ Test coverage gaps for new features

Summary

This is a well-structured PR with valuable additions (documentation, AI tooling, LLM caching). The main concerns are:

1. Missing test coverage for new features
2. Security review of workflow permissions
3. Type consistency in database layer
4. Need for cache maintenance strategy

Recommendation: Address test coverage and type exports before merging. The other items can be follow-up issues if time-sensitive.

---

🤖 Generated with Claude Code

[claude]

Pull Request Review

This is an automated sync PR from main to demo with 345 files changed. Here's my review focusing on the key changes:

---

🎯 Overall Assessment

Status: ✅ APPROVED with minor recommendations

This is a substantial sync containing infrastructure improvements, new documentation, workflow additions, and dependency updates. The changes are well-structured and follow project conventions.

---

🔍 Key Changes Reviewed

1. GitHub Workflows (Security & Best Practices) ✅

New Files:

• .github/workflows/claude-code-review.yml - Automated PR reviews
• .github/workflows/claude.yml - On-demand Claude assistance

Observations:

• Security: Proper permission scoping (contents: read, pull-requests: read, id-token: write)
• Best Practice: Uses pinned action version (@v4)
• Flexibility: Includes commented-out filtering options for future customization
• Tool Restrictions: Appropriately restricts allowed tools to GitHub CLI operations only

Recommendation: Consider adding a timeout to prevent runaway workflows:

jobs:
  claude-review:
    timeout-minutes: 15  # Add this

2. Documentation (CLAUDE.md, AGENTS.md, DOCS.md) ✅

Excellent additions:

• CLAUDE.md: Comprehensive guide with architecture, commands, testing strategy - well-organized
• AGENTS.md: Clear structure and coding conventions
• DOCS.md: Proper documentation index with logical grouping

Strengths:

• Consistent formatting and structure
• Clear command examples with inline comments
• Proper cross-referencing between docs
• Follows ADR 0018: Documentation Strategy

3. Infrastructure Changes (AWS/Terraform CDK) ⚠️

File: infra/cdktf/src/lib/aws/sandbox-stack.ts (new file, 373 lines)

Security Concerns:

1. Line 156-158: Password generation using Fn.uuid() with base64 encoding:

   const dbPassword = Fn.base64encode(
     Fn.uuid() // Use UUID for a secure random password
   );

   Issue: UUIDs are not cryptographically secure passwords. They're predictable and have limited entropy.

   Recommendation: Use AWS Secrets Manager's built-in random password generation or a proper password generation function:

   // Better approach - let Secrets Manager generate the password
   const dbSecret = new SecretsmanagerSecret(this, `${id}-db-secret`, {
     name: getDatabaseSecretKey(environment),
     description: `Database credentials for ${environment}`,
     generateSecretString: {
       secretStringTemplate: JSON.stringify({ username: dbUsername }),
       generateStringKey: 'password',
       excludePunctuation: true,
       passwordLength: 32
     }
   });

2. Line 201: skipFinalSnapshot: true

   • Risk: Data loss if RDS instance is deleted
   • Recommendation: Set to false for non-ephemeral environments

3. Line 251: IAM policy uses SecretsManagerReadWrite

   • Concern: Overly permissive - grants write access when only read is needed
   • Recommendation: Use SecretsManagerReadOnly or create custom policy with least privilege

4. Line 268: Bedrock policy with Resource: '*'

   • Risk: Grants access to all Bedrock models
   • Recommendation: Scope to specific model ARNs if possible

5. Line 178-185: Database subnet group uses public subnets

   • Security: RDS is marked as publiclyAccessible: false (line 200) ✅ Good!
   • Best Practice: Consider using dedicated private subnets for databases

4. CLI Changes ✅

File: apps/cli/src/cli-controller/forms.ts

Observations:

• Clean implementation with proper error handling
• Uses test parser with caching for CLI operations
• Good separation of concerns

Minor Issue (Line 26):

• Typo: "Intialize" → "Initialize"

5. Node Version Update ✅

File: .nvmrc changed from unknown to v22.19.0

Impact Analysis:

• Node 22 is the latest LTS
• Ensure all CI/CD environments are updated
• Check compatibility with dependencies

6. Package Updates ✅

Dependencies appear up-to-date and consistent across packages. No major version jumps that would cause concern.

---

🔐 Security Summary

Issue	Severity	Location	Status
UUID-based password generation	HIGH	sandbox-stack.ts:156	⚠️ Needs fix
Overly permissive IAM policies	MEDIUM	sandbox-stack.ts:251,268	⚠️ Recommended
Skip final snapshot enabled	MEDIUM	sandbox-stack.ts:201	⚠️ Consider changing
Database in public subnets	LOW	sandbox-stack.ts:178	ℹ️ Mitigated by publiclyAccessible:false

---

🧪 Test Coverage

No test files found for new infrastructure code. Consider adding tests for:

• sandbox-stack.ts - Infrastructure validation tests
• Workflow files - Basic syntax validation

---

📋 Recommendations

Must Fix

1. Replace UUID password generation with proper cryptographic password generation in sandbox-stack.ts

Should Consider

2. Add workflow timeouts to prevent resource exhaustion
3. Apply least-privilege IAM policies
4. Set skipFinalSnapshot: false for persistent environments
5. Fix typo in CLI forms command description
6. Add infrastructure unit tests

Nice to Have

7. Consider dedicated private subnets for RDS
8. Scope Bedrock IAM policy to specific models

---

✅ What's Working Well

• Comprehensive documentation strategy
• Proper GitHub Actions security configuration
• Clean CLI implementation with good separation of concerns
• Consistent code formatting and structure
• Up-to-date dependency versions
• Node version alignment across environments

---

🎬 Conclusion

This PR is safe to merge after addressing the HIGH severity password generation issue. The other recommendations are for hardening and best practices but don't block deployment.

Suggested Action: Fix the password generation in sandbox-stack.ts, then merge. Other improvements can be tracked in follow-up issues.

---

🤖 Review generated by Claude Code