charles/peon
1
0
Fork 0
Code Issues 12 Pull requests Projects Releases Packages Wiki Activity Actions

feat(ipc): IPC types, IpcChannels and IpcClient #25

Merged
charles merged 4 commits from peon/6 into main 2026-04-17 07:31:10 +00:00
Conversation 1 Commits 4 Files changed 6 +394 -4
claude-desktop commented 2026-04-16 20:59:43 +00:00
Collaborator
Copy link

Summary

  • Define AppCommand, AppEvent, TerminalCommand, GitCommand, SettingsCommand in backend/src/ipc.rs with IpcChannels::new() constructor
  • Implement IpcClient in frontend/src/ipc_client.rs with send() / next_event() and cheap Clone via Arc<Mutex>
  • Add OrchestrationCommand and StoredEvent placeholder stubs in orchestration/mod.rs to unblock compilation (full impl deferred to issue #4)
  • Wire channel construction in app/src/main.rs
  • 10 unit tests, all green

Closes #6

## Summary - Define `AppCommand`, `AppEvent`, `TerminalCommand`, `GitCommand`, `SettingsCommand` in `backend/src/ipc.rs` with `IpcChannels::new()` constructor - Implement `IpcClient` in `frontend/src/ipc_client.rs` with `send()` / `next_event()` and cheap `Clone` via `Arc<Mutex>` - Add `OrchestrationCommand` and `StoredEvent` placeholder stubs in `orchestration/mod.rs` to unblock compilation (full impl deferred to issue #4) - Wire channel construction in `app/src/main.rs` - 10 unit tests, all green Closes #6
feat(ipc): implement IPC types, IpcChannels, and IpcClient
All checks were successful
qa / qa (pull_request) Successful in 10m10s
Details
a53de0a96a
 
Define AppCommand, AppEvent, TerminalCommand, GitCommand, SettingsCommand
in backend/src/ipc.rs with IpcChannels::new() constructor.

Implement IpcClient in frontend/src/ipc_client.rs wrapping the mpsc sender
and broadcast receiver behind Arc<Mutex> for cheap Clone semantics.

Add orchestration placeholder types (OrchestrationCommand, StoredEvent) to
unblock IPC compilation; full implementation deferred to issue #4.

Wire channel construction in app/src/main.rs and cover both modules with
unit tests (10 tests total, all green).

Closes #6
Ghost requested changes 2026-04-16 21:02:22 +00:00
Ghost left a comment
Copy link

Three acceptance-criterion mismatches, all in clear violation of the spec. Otherwise the implementation is solid: types are well-documented, error mapping is correct, the broadcast / mpsc split is right, and the 10 tests cover the important paths.\n\nBlocking on:\n1. IpcClient::Clone shares a receiver instead of creating an independent broadcast subscription (silent data-loss bug when multiple UI components clone the client).\n2. IpcChannels::new() takes no args; AC requires new(cmd_buffer: usize, evt_buffer: usize).\n3. SettingsCommand is missing the required UpdateConfig variant.

Three acceptance-criterion mismatches, all in clear violation of the spec. Otherwise the implementation is solid: types are well-documented, error mapping is correct, the `broadcast` / `mpsc` split is right, and the 10 tests cover the important paths.\n\nBlocking on:\n1. `IpcClient::Clone` shares a receiver instead of creating an independent broadcast subscription (silent data-loss bug when multiple UI components clone the client).\n2. `IpcChannels::new()` takes no args; AC requires `new(cmd_buffer: usize, evt_buffer: usize)`.\n3. `SettingsCommand` is missing the required `UpdateConfig` variant.
crates/backend/src/ipc.rs Outdated
@ -2,0 +81,4 @@
/// Commands for modifying application settings.
#[derive(Debug, Clone)]
pub enum SettingsCommand {
Ghost commented 2026-04-16 21:02:22 +00:00
Copy link

SettingsCommand is missing the required UpdateConfig variant.

AC: "SettingsCommand placeholder (at minimum an UpdateConfig variant)".

The PR ships Reload and UpdateKeybinding but not UpdateConfig. UpdateKeybinding maps closely to the keybindings-specific AC item in issue #6 and may be intentional, but the baseline UpdateConfig variant the spec requires is absent.

Fix: add the variant, even as a placeholder with no fields for now:

pub enum SettingsCommand {
    UpdateConfig,           // <- required by AC
    Reload,
    UpdateKeybinding { id: String, shortcut: String },
}

If UpdateConfig is intentionally being split into more specific variants, that decision should be reflected in the issue/spec first.

**`SettingsCommand` is missing the required `UpdateConfig` variant.** AC: *"`SettingsCommand` placeholder (at minimum an `UpdateConfig` variant)"*. The PR ships `Reload` and `UpdateKeybinding` but not `UpdateConfig`. `UpdateKeybinding` maps closely to the keybindings-specific AC item in issue #6 and may be intentional, but the baseline `UpdateConfig` variant the spec requires is absent. Fix: add the variant, even as a placeholder with no fields for now: ```rust pub enum SettingsCommand { UpdateConfig, // <- required by AC Reload, UpdateKeybinding { id: String, shortcut: String }, } ``` If `UpdateConfig` is intentionally being split into more specific variants, that decision should be reflected in the issue/spec first.
crates/backend/src/ipc.rs Outdated
@ -2,0 +110,4 @@
const COMMAND_CAPACITY: usize = 256;
const EVENT_CAPACITY: usize = 1024;
/// Create a fresh channel bundle with pre-tuned capacities.
Ghost commented 2026-04-16 21:02:22 +00:00
Copy link

IpcChannels::new() signature deviates from the acceptance criterion.

AC explicitly requires: IpcChannels::new(cmd_buffer: usize, evt_buffer: usize) -> Self.

The implementation uses a no-arg constructor with hardcoded constants instead. Callers (tests, app startup, future integration tests) can't control buffer sizes without editing source. The constants themselves are reasonable defaults, but the parameterised signature from the AC gives callers that control.

Fix: change the signature to accept the two sizes and pass them to the channel constructors:

pub fn new(cmd_buffer: usize, evt_buffer: usize) -> Self {
    let (command_tx, command_rx) = mpsc::channel(cmd_buffer);
    let (event_tx, event_rx) = broadcast::channel(evt_buffer);
    ...
}

impl Default for IpcChannels {
    fn default() -> Self {
        Self::new(256, 1024)
    }
}

Update app/src/main.rs to call IpcChannels::new(256, 1024) (or use IpcChannels::default()) and update the tests accordingly.

**`IpcChannels::new()` signature deviates from the acceptance criterion.** AC explicitly requires: `IpcChannels::new(cmd_buffer: usize, evt_buffer: usize) -> Self`. The implementation uses a no-arg constructor with hardcoded constants instead. Callers (tests, app startup, future integration tests) can't control buffer sizes without editing source. The constants themselves are reasonable defaults, but the parameterised signature from the AC gives callers that control. Fix: change the signature to accept the two sizes and pass them to the channel constructors: ```rust pub fn new(cmd_buffer: usize, evt_buffer: usize) -> Self { let (command_tx, command_rx) = mpsc::channel(cmd_buffer); let (event_tx, event_rx) = broadcast::channel(evt_buffer); ... } impl Default for IpcChannels { fn default() -> Self { Self::new(256, 1024) } } ``` Update `app/src/main.rs` to call `IpcChannels::new(256, 1024)` (or use `IpcChannels::default()`) and update the tests accordingly.
crates/frontend/src/ipc_client.rs Outdated
@ -2,0 +19,4 @@
/// Frontend handle to the IPC channels.
///
/// [`IpcClient`] is cheap to clone — all clones share the same underlying
/// `mpsc::Sender` and wrap the same broadcast receiver behind an `Arc<Mutex>`.
Ghost commented 2026-04-16 21:02:22 +00:00
Copy link

Wrong clone semantics — all clones share one receiver instead of subscribing independently.

AC says: "IpcClient is Clone-able (sender clones, receiver subscribes to broadcast)". The derive + Arc<Mutex<Receiver>> design means every clone points at the same receiver position: whichever clone calls next_event() first consumes the event, and the others see nothing. That defeats broadcast entirely.

The fix is to store broadcast::Sender<AppEvent> (not the receiver) in IpcClient, and implement Clone manually so each clone calls event_tx.subscribe() to create its own independent cursor:

pub struct IpcClient {
    command_tx: mpsc::Sender<AppCommand>,
    event_tx: broadcast::Sender<AppEvent>,
}

impl IpcClient {
    pub fn new(
        command_tx: mpsc::Sender<AppCommand>,
        event_tx: broadcast::Sender<AppEvent>,
    ) -> Self {
        Self { command_tx, event_tx }
    }

    pub async fn next_event(&self) -> Result<AppEvent, IpcError> {
        self.event_tx
            .subscribe()
            // ...but this creates a NEW subscription each call, not right either
    }
}

Actually the cleanest fix is to carry both event_tx (for cloning) and the per-instance event_rx wrapped in Arc<Mutex>, but implement Clone manually:

pub struct IpcClient {
    command_tx: mpsc::Sender<AppCommand>,
    event_tx: broadcast::Sender<AppEvent>,
    event_rx: Arc<Mutex<broadcast::Receiver<AppEvent>>>,
}

impl Clone for IpcClient {
    fn clone(&self) -> Self {
        Self {
            command_tx: self.command_tx.clone(),
            event_tx: self.event_tx.clone(),
            event_rx: Arc::new(Mutex::new(self.event_tx.subscribe())),
        }
    }
}

IpcClient::new would need to accept event_tx: broadcast::Sender<AppEvent> instead of event_rx, or accept both. The app/src/main.rs wiring and IpcChannels event_tx field are already public, so this is straightforward.

**Wrong clone semantics — all clones share one receiver instead of subscribing independently.** AC says: *"`IpcClient` is Clone-able (sender clones, receiver subscribes to broadcast)"*. The derive + `Arc<Mutex<Receiver>>` design means every clone points at the same receiver position: whichever clone calls `next_event()` first consumes the event, and the others see nothing. That defeats broadcast entirely. The fix is to store `broadcast::Sender<AppEvent>` (not the receiver) in `IpcClient`, and implement `Clone` manually so each clone calls `event_tx.subscribe()` to create its own independent cursor: ```rust pub struct IpcClient { command_tx: mpsc::Sender<AppCommand>, event_tx: broadcast::Sender<AppEvent>, } impl IpcClient { pub fn new( command_tx: mpsc::Sender<AppCommand>, event_tx: broadcast::Sender<AppEvent>, ) -> Self { Self { command_tx, event_tx } } pub async fn next_event(&self) -> Result<AppEvent, IpcError> { self.event_tx .subscribe() // ...but this creates a NEW subscription each call, not right either } } ``` Actually the cleanest fix is to carry both `event_tx` (for cloning) and the per-instance `event_rx` wrapped in `Arc<Mutex>`, but implement `Clone` manually: ```rust pub struct IpcClient { command_tx: mpsc::Sender<AppCommand>, event_tx: broadcast::Sender<AppEvent>, event_rx: Arc<Mutex<broadcast::Receiver<AppEvent>>>, } impl Clone for IpcClient { fn clone(&self) -> Self { Self { command_tx: self.command_tx.clone(), event_tx: self.event_tx.clone(), event_rx: Arc::new(Mutex::new(self.event_tx.subscribe())), } } } ``` `IpcClient::new` would need to accept `event_tx: broadcast::Sender<AppEvent>` instead of `event_rx`, or accept both. The `app/src/main.rs` wiring and `IpcChannels` `event_tx` field are already public, so this is straightforward.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(ci): add libsqlite3-dev to system dependencies
Some checks failed
qa / qa (pull_request) Failing after 1m55s
Details
d0c4d2f3ba
 
Required by libsqlite3-sys for sqlx compilation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(ci): bump cache key to v2 to invalidate stale target/
Some checks failed
qa / qa (pull_request) Has been cancelled
Details
9e1b90416c
 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
charles force-pushed peon/6 from 9e1b90416c
Some checks failed
qa / qa (pull_request) Has been cancelled
Details
to 654d38ffd1
Some checks are pending
qa / qa (pull_request) Waiting to run
Details
2026-04-17 07:18:52 +00:00 Compare
style(ipc): apply rustfmt
Some checks failed
qa / qa (pull_request) Has been cancelled
Details
6c69c4de2c
charles deleted branch peon/6 2026-04-17 07:31:10 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
reviewer
Ghost
Labels
Clear labels   
area:config
Configuration loading, TOML, settings

  
area:contracts
Shared types crate (contracts)

  
area:engine
OrchestrationEngine, reactors, dispatch loop

  
area:eventsourcing
Event sourcing core — decider, projector, commands, events

  
area:frontend
GTK4/RELM4 frontend components

  
area:git
Git integration — GitCore, checkpoints, branches

  
area:ipc
IPC channels, AppCommand/AppEvent, GTK↔tokio bridge

  
area:persistence
SQLite persistence, EventStore, migrations

  
area:provider
Provider adapters (Claude, Codex), JSON-RPC stdio

  
area:scaffold
Project scaffolding, workspace setup, CI

  
area:terminal
Terminal manager, PTY, VTE4, Kitty/Foot

  
type:user-story
User story for v0.1.0 implementation

No labels
area:config
area:contracts
area:engine
area:eventsourcing
area:frontend
area:git
area:ipc
area:persistence
area:provider
area:scaffold
area:terminal
type:user-story
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
dev
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
charles/peon!25
No description provided.