fix: Add PostgreSQL service to CI workflow for integration tests

[bkutasi]

Summary

• Adds PostgreSQL service with pgvector:pg16 to the CI workflow
• Sets DATABASE_URL environment variable for tests that require database connectivity
• Enables integration tests that depend on a PostgreSQL database to run in CI

Changes

• Added services.postgres to .github/workflows/test.yml
• Added DATABASE_URL env var to the test step

[gemini-code-assist]

Note

Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported.

[serrrfirat]

PR #61 Review: fix: Add PostgreSQL service to CI workflow for integration tests

Author: bkutasi | Verdict: APPROVE

Summary

Adds a PostgreSQL service (pgvector/pgvector:pg16) to the GitHub Actions CI workflow so integration tests that require a database can run. Sets DATABASE_URL env var for cargo test.

Findings

P3 — Hardcoded credentials in CI (Informational)

• POSTGRES_PASSWORD: postgres is hardcoded but this is a CI-only ephemeral service container. Acceptable for test workflows.

P3 — No explicit pgvector version pin

• pgvector/pgvector:pg16 uses the latest pgvector build for PG16. Consider pinning a specific tag (e.g., pg16-v0.8.0) to avoid surprise breakage, but low risk.

No Issues Found

• Health check configuration is correct and follows best practices.
• Port mapping (5432:5432) is standard.
• DATABASE_URL format is correct for the service definition.
• No security concerns — credentials are test-only, never leave CI.

Verdict: ✅ APPROVE

Clean, minimal CI fix. No security or correctness issues.

[tribendu]

PR Review: #61 - "fix: Add PostgreSQL service to CI workflow for integration tests"

Summary

This PR adds PostgreSQL with the pgvector extension to the GitHub Actions CI workflow to support integration tests. It adds a service container using pgvector/pgvector:pg16 and configures the necessary environment variables for the test step. The implementation is straightforward and follows GitHub Actions best practices for service containers.

Changes: +17 additions, -0 deletions

Author: bkutasi

---

Pros

• Correct Docker image: Uses pgvector/pgvector:pg16 which includes both PostgreSQL 16 and the pgvector extension needed for IronClaw's vector search features
• Proper health checks: Configures health checks using pg_isready with reasonable defaults (10s interval, 5s timeout, 5 retries)
• Clear environment configuration: All credentials and database names are explicitly defined
• Correct DATABASE_URL format: The connection string uses proper syntax and connects to the service container
• Service pattern: Follows GitHub Actions documented pattern for service containers

---

Concerns

1. Host port mapping issue:

   • 5432:5432 explicitly maps to host port 5432, which could conflict if the runner already has PostgreSQL
   • Recommendation: Use - 5432/tcp (or just - 5432) to let GitHub Actions assign a random host port and use postgres:5432 in DATABASE_URL (though GitHub Actions typically handles this automatically)

2. Security credentials:

   • Default credentials (postgres/postgres) are acceptable for CI but should be clearly documented as test-only
   • These credentials are visible in CI logs if environment variables are echoed

3. No schema initialization:

   • The PR assumes the tests handle database schema/migration setup
   • If schema initialization is needed, consider adding a step using psql or using a custom entrypoint script

4. Minor formatting noise:

   • Added blank line after name: Run Tests which is unnecessary but harmless

5. Missing test dependency verification:

   • No way to verify this actually enables tests that were previously skipped due to missing database

---

Suggestions

High Priority

1. Update port mapping:

   ports:
     - 5432/tcp  # Let GitHub Actions assign host port automatically

2. Consider adding database setup verification:

   - name: Verify PostgreSQL connection
     run: |
       psql $DATABASE_URL -c "SELECT 1"
       psql $DATABASE_URL -c "CREATE EXTENSION IF NOT EXISTS vector;"

Nice to Have

3. Add comments explaining the service:

   services:
     # PostgreSQL with pgvector extension for integration tests
     postgres:
       image: pgvector/pgvector:pg16
       # ... rest of config

4. Consider adding cleanup (if tests leave residual state):

   options: >-
     --health-cmd pg_isready
     --health-interval 10s
     --health-timeout 5s
     --health-retries 5
     --health-start-period 5s

5. Document in README or CI docs: Update project documentation to explain the CI database setup and how to run integration tests locally.

---

Overall Assessment

Recommendation: ✅ Approve with minor suggestions

This is a clean and necessary change that enables integration tests with PostgreSQL and pgvector. The implementation follows GitHub Actions best practices. The concerns are minor and the suggestions are optional improvements for future iterations.

The PR successfully addresses the need for database integration testing without introducing any breaking changes or significant issues.

---

Reviewed by: AI Subagent (using Minimax M2.5)
Date: 2026-02-14

[ilblackdragon]

Review: fix: Add PostgreSQL service to CI workflow for integration tests

Summary

The intent is good -- adding a PostgreSQL service container so that the workspace integration tests can actually run in CI. The service container configuration (image, health checks, credentials, port mapping) is all correct. However, there is a critical missing piece that causes CI to fail: the database schema is never applied before tests run.

Critical: Missing migration step (CI is currently failing)

The CI run is failing with relation "memory_documents" does not exist across all 10 workspace integration tests. The root cause:

1. The PR correctly stands up a PostgreSQL instance with pgvector.
2. The PR correctly sets DATABASE_URL so the integration tests can connect.
3. But neither the CI workflow nor the test harness runs the schema migrations (migrations/V1__initial.sql through V8__settings.sql).

Previously, these tests had a try_connect() guard that would skip gracefully when no database was available. Now that DATABASE_URL is set, the connection succeeds (the database exists), but the tables do not, so the tests panic instead of skipping.

You need to add a step that runs the migrations against the fresh database before cargo test. There are two reasonable approaches:

Option A: Run the SQL migrations directly with psql

- name: Run database migrations
  env:
    DATABASE_URL: postgres://postgres:postgres@localhost:5432/ironclaw_test
  run: |
    for f in migrations/V*.sql; do
      psql "$DATABASE_URL" -f "$f"
    done

Option B: Use refinery via a small Rust binary or test fixture

The codebase already uses refinery with embed_migrations!("migrations") in src/history/store.rs and src/setup/wizard.rs. You could add a setup step that calls the existing migration machinery. However, Option A is simpler for CI since psql is available on the runner.

Minor observations

1. Unpinned image tag: pgvector/pgvector:pg16 tracks latest pgvector for PG16. Consider pinning to a specific digest or version tag (e.g., pgvector/pgvector:pg16-v0.8.0) to avoid surprise breakage, though this is low priority.

2. Cosmetic blank line: The diff adds a blank line between name: Run Tests and on: at the top of the file. This is harmless but unrelated to the change.

Verdict

Requesting changes because the PR in its current state breaks CI (10 test failures). Adding a migration step before the test run should be a straightforward fix. The service container configuration itself looks correct and well-structured.

[tribendu]

GitHub PR Review Batch 3 - nearai/ironclaw

PR #71: Fix undo/redo deadlock and checkpoint behavior

Summary

This PR addresses critical concurrency issues in the undo/redo system. The main changes are:

1. Reordering lock acquisitions in undo_turn() and redo_turn() to lock session before undo manager, matching process_user_input() pattern to prevent deadlocks
2. Changing undo() to return owned Checkpoint instead of &Checkpoint, fixing a bug where repeated undos returned the same checkpoint reference instead of walking backwards through history
3. Adding current_turn and current_messages parameters to redo() so it can save the current state before popping from redo stack
4. Adding comprehensive tests to verify undo/redo stack invariants and state preservation

Pros

• Critical bug fix: The deadlock prevention is well-reasoned and follows existing patterns in the codebase
• Ownership fix: Returning owned Checkpoint from undo() eliminates the borrow/snapshot bug that prevented repeated undos
• Better API design: The new redo() signature is more explicit about state management
• Excellent test coverage: Added 3 new tests covering repeated undo, undo/redo cycles, and stack size consistency
• Documentation: Added helpful docstrings explaining the stack behavior and invariants

Concerns

• Missing error handling: No check if thread.restore_from_messages() succeeds/fails before returning success
• Clone overhead: The undo() function now clones messages when saving to redo stack, but this is unavoidable with the API change
• Test gap: No test for the deadlock scenario itself (would require concurrent test setup)
• Migration note: The change from &Checkpoint to Checkpoint is a breaking API change for any external users of UndoManager

Suggestions

• Consider returning Result<Checkpoint, ...> from undo() and propagate restoration errors
• Add integration test with concurrent calls to verify deadlock is truly fixed
• Document the breaking API change in upgrade notes if this is a public library

---

PR #66: Update README architecture diagram to use Unicode box drawing

Summary

This PR updates the README.md architecture diagram from ASCII art to Unicode box drawing characters. The change improves readability and visual appeal of the system architecture diagram, using characters like ╭───╮, │, ● instead of +, -, |. The diagram structure and content remain unchanged.

Pros

• Better aesthetics: Unicode box drawing creates cleaner, more professional diagrams
• Improved readability: The visual distinction between components is clearer
• Modern standard: Unicode box drawing is the standard for terminal-based diagrams in modern tools
• Preserves structure: The logical diagram structure is unchanged

Concerns

• Terminal compatibility: Some older terminals or Windows default fonts may not render Unicode box drawing correctly, showing replacement characters
• Copy-paste issues: Unicode characters may cause issues in some markdown viewers or plain text contexts
• Diff noise: This change makes future diff review harder since the diagram structure is visually the same but characters changed

Suggestions

• Consider adding a note about terminal compatibility if users report issues
• For CI environments, ensure the font/encoding is UTF-8 aware

---

PR #63: Memory Guardian and Cognitive Routines

Summary

This is a large PR (894 lines of new code) adding a comprehensive memory management system with two layers:

Cognitive Routines (Prompt-Level):

• Pre-game routine (5-step checklist)
• Checkpoint reminder templates
• After-action review templates

Memory Guardian (System-Level):

• Pre-compaction breadcrumbs (writes state snapshot before context loss)
• Auto-breadcrumbs (every N tool calls, default 10)
• Checkpoint gate (escalating pressure: gentle → firm → urgent at 12/20/30 calls)
• Memory search tracking (reminds agent to search after 8 turns)

The PR also adds line number tracking to memory chunks for citation support (V9 migration), checkpoint tracker to Thread struct, and extensive testing (15 new tests).

Pros

• Comprehensive documentation: Excellent module-level docs explaining the two-layer design rationale
• Design rationale: Clear explanation of why both layers exist (instructions vs. enforcement)
• Good defaults: Sensible thresholds (breadcrumb_interval=10, gate_thresholds=[12,20,30])
• Defense in depth: Escalating pressure vs. hard blocking is a thoughtful UX choice
• Sanitization functions: sanitize_checkpoint_text() prevents markdown/prompt injection
• Extensive testing: 15 tests covering the full functionality

Concerns

• Large PR: 894 lines makes review difficult; could be split into smaller PRs
• Config not wired up: CognitiveConfig is defined but not loaded from configuration files or used to enable/disable features
• Session lock pattern: Multiple session lock acquisitions in the agent loop could become a performance bottleneck with high tool call volume
• Breadcrumb performance: Async I/O in the tool execution path for every Nth tool call adds latency
• Missing integration tests: No tests verifying the guardian works end-to-end with compaction/reset
• Tracker not persisted: CheckpointTracker is in-memory only, resets on session restart

Suggestions

• Split into smaller PRs: (1) cognitive module structure, (2) guardian integration, (3) database migration
• Wire up CognitiveConfig from config files/CLI flags so features can be toggled
• Consider making breadcrumb writing non-blocking (spawn without await)
• Document the performance impact of breadcrumb I/O
• Add integration test verifying pre-compaction breadcrumb actually writes
• Add metrics/logging for guardian actions (breadcrumb writes, gate triggers)

---

PR #62: Add Tinfoil private inference provider

Summary

This PR adds support for Tinfoil as a new LLM backend. The changes include:

• Add Tinfoil variant to LlmBackend enum with string parsing
• Add TinfoilConfig struct with api_key and model fields
• Add create_tinfoil_provider() function that uses Rig's OpenAI client with https://inference.tinfoil.sh/v1 as base URL
• Explicitly use completions_api() for Tinfoil (they don't support the newer Responses API)
• Wire up in create_llm_provider() dispatch
• Update setup wizard to include tinfoil field in config

Pros

• Clean implementation: Follows existing provider patterns (Ollama, OpenAI-compatible)
• Proper error handling: Returns LlmError::AuthFailed when tinfoil config is missing
• Environment variable support: Reads TINFOIL_API_KEY and TINFOIL_MODEL from env with defaults
• Good documentation: Note about why completions_api() is explicitly used

Concerns

• Hardcoded base URL: TINFOIL_BASE_URL constant is not configurable via environment variable
• No provider documentation: No usage examples or caveats in the main docs
• Secret exposure: Uses expose_secret() on api_key when passing to Rig - this is the correct approach but worth verifying Rig doesn't log it
• No health check: No verification that the Tinfoil endpoint works on startup

Suggestions

• Make TINFOIL_BASE_URL configurable via environment variable
• Add a brief section in the README about supported LLM providers including Tinfoil
• Consider adding a quick health check or connection test on startup when tinfoil is configured

---

PR #61: Add PostgreSQL test workflow for CI

Summary

This PR adds a PostgreSQL with pgvector service to the GitHub Actions test workflow. The changes enable running tests against a real PostgreSQL database instead of libSQL-only testing. The service is configured with pgvector/pgvector:pg16 image and health checks.

Pros

• Better test coverage: Enables testing against production database backend (PostgreSQL + pgvector)
• Proper service configuration: Health checks, retries, and proper port mapping
• Simple integration: Just adds the service and env var for DATABASE_URL

Concerns

• No database fixtures: No test data setup or migrations for the test database
• No test splitting: All tests still run against both backends; could be slow
• Missing test matrix: Could benefit from matrix strategy testing libSQL and PostgreSQL in parallel
• No cleanup between tests: Tests may interfere with each other's state
• Flaky test risk: No fixture isolation or transaction rollback

Suggestions

• Add a job matrix to test libSQL and PostgreSQL in parallel
• Add migration step before running tests: cargo run -- database migrate
• Consider wrapping tests in transactions with rollback for isolation
• Add a separate integration test job vs. unit tests

---

PR #57: Sandbox job monitoring and credential grants persistence

Summary

This is a large PR (~6333 lines touched) introducing several features:

1. Job Monitor System: New job_monitor.rs module spawns background tasks that watch for events from sandbox jobs (especially Claude Code) and inject assistant messages back into the agent loop via an injection channel in ChannelManager

2. Credential Grants Persistence: Added credential_grants_json field to sandbox jobs for restoring credentials on job restart

3. Claude Code OAuth Token Extraction: New logic to read OAuth tokens from macOS Keychain or Linux ~/.claude/.credentials.json for passing to containers via ANTHROPIC_API_KEY env var

4. Database improvements: Changed LibSqlBackend::connect() to async and added PRAGMA busy_timeout = 5000 and PRAGMA journal_mode=WAL for better concurrency

5. NEAR AI URL migration: Changed default from https://cloud-api.near.ai to https://private.near.ai

6. Worker Docker image: Added GitHub CLI installation and improved layer caching

7. Model command enhancement: /model now lists available models with the active one marked

Pros

• Job monitoring is useful: Forwarding Claude Code output to the main agent enables better visibility into sub-agent work
• Good test coverage: Job monitor has 4 unit tests for message forwarding, filtering, and lifecycle
• Credential persistence: Restoring grants on job restart is a good UX improvement
• Database concurrency: WAL mode and busy_timeout reduce lock contention in libSQL
• OAuth extraction: Clever use of existing Claude auth rather than re-implementing auth

Concerns

• Monolithic PR: Too many unrelated changes in one PR (job monitor, credentials, database, NEAR AI URLs, Docker, model command)
• Cross-platform fragility: OAuth token extraction depends on macOS Keychain commands and Linux file paths - could break on different systems
• Secret handling: OAuth token is passed via ANTHROPIC_API_KEY env var - this could be leaked in logs if the container or tool doesn't sanitize
• Channel coupling: Job monitor couples Claude Code specifically to the agent loop - not generic for any job type
• Database breaking change: Making connect() async is a breaking change for any code calling it synchronously
• No rate limiting: Job monitor could spam the agent loop if the sub-agent produces many messages quickly
• Missing test forOAuth: No tests for the token extraction logic

Suggestions

• Split into smaller focused PRs: (1) job monitor, (2) credential grants, (3) database changes (4) Docker/URL migrations
• Add rate limiting to job monitor (e.g., max 1 message/second)
• Make job monitor generic (protocol-agnostic) instead of Claude-specific
• Add tests for OAuth token extraction on both platforms (mocking the Keychain/file read)
• Document the connect() async migration as a breaking change
• Add warning logs when job monitor lags or drops messages
• Verify ANTHROPIC_API_KEY doesn't leak in container logs

---

Summary

Highest Quality PRs

1. PR fix: undo() peeks without popping, breaking repeated undo and leaking redo stack #71 - Well-scoped bug fix with excellent documentation and testing
2. PR feat: add Tinfoil private inference provider #62 - Clean implementation following existing patterns

Needs Splitting

• PR feat(memory): add citation support, line tracking, and cognitive routines #63 - Large but cohesive; could be split into 2-3 PRs
• PR feat: Add PR review tools, job monitor, and channel injection for E2E sandbox workflows #57 - Too many unrelated changes; definitely needs splitting

Minor Concerns

• PR Revise architecture diagram for better representation #66 - Good but verify terminal compatibility
• PR fix: Add PostgreSQL service to CI workflow for integration tests #61 - Good start but needs fixture isolation and test matrix

Recommendations

• Merge PR fix: undo() peeks without popping, breaking repeated undo and leaking redo stack #71 as-is
• Merge PR feat: add Tinfoil private inference provider #62 with minor additions (configurable base URL)
• Request PR feat(memory): add citation support, line tracking, and cognitive routines #63 be split and config hooked up
• Enhance PR fix: Add PostgreSQL service to CI workflow for integration tests #61 with matrix strategy and fixtures
• Reject or split PR feat: Add PR review tools, job monitor, and channel injection for E2E sandbox workflows #57 into focused follow-ups

[ilblackdragon]

Superseded by other PRs. Thank you!