Revise architecture diagram for better representation

[dj-ryan]

Updated architecture diagram in README to improve clarity.

[gemini-code-assist]

Summary of Changes

Hello @dj-ryan, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on improving the project's documentation by updating the architecture diagram within the README.md file. The revision aims to provide a clearer and more modern visual representation of the system's components and their interactions, making it easier for new contributors and stakeholders to understand the project's structure.

Highlights

• Architecture Diagram Revision: The architecture diagram in the README.md file has been completely revised to enhance visual clarity and readability, including an additional HTTP channel.

Changelog

• README.md
  • Updated the architecture diagram for improved clarity and added an additional HTTP channel.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request updates the architecture diagram in the README.md file for better clarity. The new diagram style is an improvement, but it introduced a duplicate HTTP channel and has some connector alignment issues. I've provided a suggestion to fix the duplication. Addressing the minor alignment issues would further enhance the diagram.

[gemini-code-assist]

The architecture diagram has a duplicate HTTP channel. This suggestion removes the duplicate and adjusts the connecting line.

For further improvement, consider reviewing the alignment of all connectors (e.g., for WASM Channels and Web Gateway) to ensure they line up with their corresponding ● symbols, which would enhance the diagram's clarity.

Suggested change

	╭──────╮ ╭──────╮ ╭──────╮ ╭───────────────╮ ╭───────────────╮
	│ REPL │ │ HTTP │ │ HTTP │ │ WASM Channels │ │ Web Gateway │
	╰──●───╯ ╰──●───╯ ╰──●───╯ ╰───────●───────╯ │ (SSE + WS) │
	│ │ │ │ ╰───────●───────╯
	│ │ │ │ │
	└─────────┴─────────┴──────────┬───┴──────────────────┘
	╭──────╮ ╭──────╮ ╭───────────────╮ ╭───────────────╮
	│ REPL │ │ HTTP │ │ WASM Channels │ │ Web Gateway │
	╰──●───╯ ╰──●───╯ ╰───────●───────╯ │ (SSE + WS) │
	│ │ │ ╰───────●───────╯
	│ │ │ │
	└─────────┴────────────────────┬───┴──────────────────┘

[tribendu]

PR Review: #66 - Revise architecture diagram for better representation

Author: David Ryan
Changes: 95 lines diff (+48 additions, -36 deletions)

---

Summary

This PR revises the architecture diagram in README.md by replacing ASCII box-drawing characters (─, │, ┌, ┐, etc.) with Unicode box-drawing characters (╭, ╮, ╰, ╯, ●). The architectural content and component relationships remain identical to the previous version.

---

Pros

✅ Visual Clarity: Unicode box-drawing characters provide cleaner, more modern-looking diagrams with better line continuity and corner rendering.

✅ Improved Readability: The new format uses clearer structural elements that are easier to follow visually, especially the single-line borders (╭──●──╯)代替 the double-line ASCII boxes.

✅ Consistent Spacing: The revised diagram maintains consistent spacing and alignment throughout, making it easier to trace data flow and component relationships.

✅ No Content Changes: The architectural information remains accurate - this is purely a presentation improvement that doesn't introduce any functional changes.

---

Concerns

⚠️ Cross-Platform Rendering: Unicode box-drawing characters may not render consistently across all platforms:

• Some terminals, console output, or plain text viewers may display them as garbled text or replacement characters (□)
• Older Windows terminals without proper UTF-8 support
• Email clients or text editors that don't handle Unicode well

⚠️ Copy-Paste Issues: When copying from GitHub to local terminals or markdown editors, Unicode characters may be corrupted or require proper UTF-8 encoding awareness.

⚠️ Terminal Width: The new diagram is slightly wider in some sections, which could cause wrapping in narrower terminal windows (default 80 columns). While most modern displays are wide enough, this is a consideration for documentation accessibility.

---

Suggestions

📋 Alternative for Maximum Compatibility: Consider both versions:

• Keep Unicode as the primary (better for modern web/GitHub viewers)
• Add an ASCII-only version as a fallback or in a collapsible details section

📋 Width Check: Verify the diagram renders well at standard terminal widths (80-100 columns). Test on narrower terminals if targeting developers working in consoles.

📋 Documentation Context: If the README is likely to be viewed in:

• GitHub/web: ✅ Unicode is perfect
• Terminal/CLI: ⚠️ ASCII may be safer
• Email/PDF: Consider both versions

📋 Minor Layout Optimization: The flow from "Scheduler" and "Routines Engine" to "Orchestrator" could be visually clearer with slightly more consistent vertical alignment, but this is a minor nitpick.

---

Overall Assessment

Recommendation: ✅ MERGE with minor caveats

This is a solid visual improvement that makes the architecture diagram more professional and easier to read for modern web viewers. The content is accurate and unchanged, so there's no risk of technical misrepresentation.

Primary Concern: Terminal/console viewers. Since this is a Rust project and developers frequently work in terminals, consider whether ASCII might be more practical for your primary audience. If your documentation is primarily viewed on GitHub (which renders Unicode well), this change is an improvement.

Testing Recommendation: Verify the diagram renders correctly in:

1. GitHub web renderer ✅ (likely perfect)
2. At least one terminal emulator (e.g., iTerm2, VS Code integrated terminal)
3. gh pr view output in a terminal

If terminal rendering works well, this is a straightforward improvement. If not, consider keeping the ASCII version or providing both options.

---

Files Changed

• README.md (Architecture diagram section)

[IkerAlus]

I think that the keywords "channels" and "intent routing" should still be present in the diagram.

[ilblackdragon]

Review: Architecture Diagram Revision

The visual refresh (rounded corners, better spacing, removal of the outer bounding box) is a nice improvement in readability. However, there is a factual error that needs to be fixed before merging.

Bug: Duplicate "HTTP" box

The new diagram has three boxes at the top:

╭──────╮  ╭──────╮  ╭──────╮  ╭───────────────╮  ╭───────────────╮
│ REPL │  │ HTTP │  │ HTTP │  │ WASM Channels │  │  Web Gateway  │
╰──●───╯  ╰──●───╯  ╰──●───╯  ╰───────●───────╯  │  (SSE + WS)   │

There are two "HTTP" boxes. The original diagram correctly had four distinct channels: REPL, HTTP, WASM Channels, and Web Gateway. Looking at the codebase (src/channels/), there is only one HTTP webhook channel (http.rs). The second "HTTP" box appears to be a copy-paste error and should be removed.

Minor observations

1. Loss of visual grouping: The original diagram had an outer rectangle labeled "Channels" that grouped all input sources together, making it immediately clear they are part of one subsystem. The new version loses that context. Consider whether a "Channels" label or light grouping could be reintroduced (not blocking, just a suggestion).

2. Increased height: The new diagram is ~54 lines vs the original ~36 lines due to extra padding. This is a tradeoff -- more whitespace improves readability of individual boxes but means the full architecture is harder to see at a glance without scrolling. This is fine as-is, just noting the tradeoff.

Verdict

Please fix the duplicate "HTTP" box (remove one of them), and this is good to merge.

[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