Repository navigation
Add append-only cmux-tui session journal - #9434
lawrencecchen wants to merge 135 commits into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds a durable SQLite-backed session journal, migrates resource events, routes resource commits through journal records, adds hooks and checkpoints, exposes journal operations through the server, SDKs, and CLI, and updates terminal ownership, projection, geometry, and protocol behavior. ChangesSession journal
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f738f2e80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WHERE resource_revision > ?1 | ||
| ORDER BY resource_revision ASC", |
There was a problem hiding this comment.
When a client resumes a long-lived session from a far-behind cursor, this query now reads every subsequent journal row and collects them into one Vec; open_session_event_stream then materializes all of those batches in initial_items before returning the open response. Since this change also removes the previous 4,096-row/16 MiB retention boundary, a stale cursor can make memory use and response latency grow without limit and can eventually stall or OOM the daemon. Page the replay or fall back to a snapshot once a bounded replay threshold is exceeded.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry.rs`:
- Around line 1532-1543: Update the durable commit path around
append_resource_journal_record to bind resource_revision and
previous_resource_revision together using the same tuple pattern derived from
project_resource, eliminating the resource_revision.expect call. Preserve the
existing values for both Some and None cases while ensuring an unexpected
absence cannot panic inside the SQLite transaction.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/effect_store.rs`:
- Around line 1092-1103: The commit path should reuse the already-built
serde_json::Value instead of parsing outcome_json again. Update
commit_resource_effect_patch_in_transaction and both callers to accept and pass
the outcome Value alongside outcome_json, then provide that Value directly to
append_resource_journal_record; remove the from_str re-parsing and its
associated failure path.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rs`:
- Around line 139-186: Update create_session_journal_schema and the
session_journal_reject_delete trigger to define a retention plan for unbounded
journal growth: add a recorded sealed watermark and allow deletes only for rows
below that watermark via the trigger’s condition, while continuing to reject all
other deletes. Ensure the schema preserves append-only behavior for unsealed
entries and supports future sealed-segment pruning without dropping the trigger.
- Around line 399-406: Update append_journal_record so committed_at_ms always
comes from a single unix_epoch_ms() clock read, while occurred_at_ms remains the
caller-supplied append.occurred_at_ms value, including zero. Remove the
conditional sentinel logic and preserve the existing validation and record
insertion flow.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/tests.rs`:
- Around line 635-658: Extend the freshly written journal-record test after
validating journal subjects to call registry.resource_events_after(0). Assert it
returns exactly one batch with revision 1 and the expected committed resource
deltas, covering decoding through append_resource_journal_record rather than
only inspecting the raw session_journal row.
In `@cmux-tui/spec/session-journal.md`:
- Around line 121-125: Update the sensitivity section in session-journal.md to
define the ordering of public, metadata, sensitive, and secret, explicitly state
whether max_sensitivity: "sensitive" includes lower levels, and specify the
journal.read.sensitive permission behavior for each level. Ensure filtering
never exposes secret records without authorization and does not omit records
permitted by the rule.
- Around line 197-226: Expand the hook manifest specification around the
versioned manifest and runner behavior to define who may install or update
manifests, how executable paths in exec.argv are authorized through trust or
allowlist rules, and how execution is sandboxed, including environment and
filesystem restrictions. Require these authorization and isolation checks before
the parser or dispatcher accepts and runs a manifest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 447eab59-b725-4d59-8de8-ca5468c2c165
📒 Files selected for processing (12)
cmux-tui/crates/cmux-tui-core/src/lib.rscmux-tui/crates/cmux-tui-core/src/workspace_registry.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/effect_store.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/resource_store.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/terminal_exit_store.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/tests.rscmux-tui/spec/README.mdcmux-tui/spec/commands.mdcmux-tui/spec/programmability.mdcmux-tui/spec/resource-api-v1.mdcmux-tui/spec/session-journal.md
| append_resource_journal_record( | ||
| &tx, | ||
| resource_revision.expect("resource revision for projected workspace mutation"), | ||
| previous_resource_revision, | ||
| &mutation.origin, | ||
| &mutation.id, | ||
| event_kind, | ||
| None, | ||
| result, | ||
| &resource_deltas, | ||
| )?; | ||
| prune_resource_events(&tx)?; | ||
| resource_store::prune_resource_mutations(&tx)?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Remove the expect from the durable commit path.
resource_revision is guaranteed to be Some only because it is derived from the same project_resource flag as previous_resource_revision. The compiler does not enforce that link, so a later change to this function can turn line 1534 into a panic inside a SQLite transaction. Bind resource_revision in the same tuple pattern instead.
♻️ Proposed change
if let (
Some(previous_topology),
Some(previous_resource_revision),
Some(sqlite_resource_revision),
- ) = (previous_topology.as_ref(), previous_resource_revision, sqlite_resource_revision)
+ Some(resource_revision),
+ ) = (
+ previous_topology.as_ref(),
+ previous_resource_revision,
+ sqlite_resource_revision,
+ resource_revision,
+ )
{
@@
append_resource_journal_record(
&tx,
- resource_revision.expect("resource revision for projected workspace mutation"),
+ resource_revision,
previous_resource_revision,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| append_resource_journal_record( | |
| &tx, | |
| resource_revision.expect("resource revision for projected workspace mutation"), | |
| previous_resource_revision, | |
| &mutation.origin, | |
| &mutation.id, | |
| event_kind, | |
| None, | |
| result, | |
| &resource_deltas, | |
| )?; | |
| prune_resource_events(&tx)?; | |
| resource_store::prune_resource_mutations(&tx)?; | |
| append_resource_journal_record( | |
| &tx, | |
| resource_revision, | |
| previous_resource_revision, | |
| &mutation.origin, | |
| &mutation.id, | |
| event_kind, | |
| None, | |
| result, | |
| &resource_deltas, | |
| )?; | |
| resource_store::prune_resource_mutations(&tx)?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry.rs` around lines 1532 -
1543, Update the durable commit path around append_resource_journal_record to
bind resource_revision and previous_resource_revision together using the same
tuple pattern derived from project_resource, eliminating the
resource_revision.expect call. Preserve the existing values for both Some and
None cases while ensuring an unexpected absence cannot panic inside the SQLite
transaction.
| let outcome = serde_json::from_str::<Value>(outcome_json)?; | ||
| append_resource_journal_record( | ||
| transaction, | ||
| revision, | ||
| previous_revision, | ||
| "resource-api", | ||
| idempotency_key, | ||
| operation, | ||
| Some(patch), | ||
| &outcome, | ||
| deltas, | ||
| )?; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Pass the outcome as a Value instead of re-parsing outcome_json.
Both callers build the outcome as a serde_json::Value and then canonicalize it (lines 936-937 and 983-984). Line 1092 parses that canonical string back into a Value inside the transaction. The round trip repeats work on the commit path and adds a parse failure branch that cannot be triggered by valid input.
Pass the already-built Value into commit_resource_effect_patch_in_transaction next to outcome_json, as commit_resource_effect already does at line 818.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/effect_store.rs` around
lines 1092 - 1103, The commit path should reuse the already-built
serde_json::Value instead of parsing outcome_json again. Update
commit_resource_effect_patch_in_transaction and both callers to accept and pass
the outcome Value alongside outcome_json, then provide that Value directly to
append_resource_journal_record; remove the from_str re-parsing and its
associated failure path.
| pub(super) fn create_session_journal_schema(transaction: &Transaction<'_>) -> anyhow::Result<()> { | ||
| transaction.execute_batch( | ||
| "CREATE TABLE IF NOT EXISTS session_journal ( | ||
| sequence INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| event_id TEXT UNIQUE NOT NULL, | ||
| schema_version INTEGER NOT NULL CHECK(schema_version > 0), | ||
| kind TEXT NOT NULL, | ||
| class TEXT NOT NULL CHECK(class IN ('state', 'observation', 'effect', 'checkpoint')), | ||
| replay_policy TEXT NOT NULL CHECK(replay_policy IN ('required', 'advisory', 'never')), | ||
| occurred_at_ms INTEGER NOT NULL CHECK(occurred_at_ms >= 0), | ||
| committed_at_ms INTEGER NOT NULL CHECK(committed_at_ms >= 0), | ||
| producer_json TEXT NOT NULL CHECK(json_valid(producer_json)), | ||
| authority_json TEXT CHECK(authority_json IS NULL OR json_valid(authority_json)), | ||
| causation_id TEXT, | ||
| correlation_id TEXT, | ||
| causation_depth INTEGER NOT NULL CHECK(causation_depth >= 0), | ||
| subjects_json TEXT NOT NULL CHECK(json_valid(subjects_json)), | ||
| sensitivity TEXT NOT NULL CHECK(sensitivity IN ('public', 'metadata', 'sensitive', 'secret')), | ||
| payload_json TEXT NOT NULL CHECK(json_valid(payload_json)), | ||
| resource_revision INTEGER UNIQUE, | ||
| previous_resource_revision INTEGER, | ||
| CHECK( | ||
| (resource_revision IS NULL AND previous_resource_revision IS NULL) | ||
| OR ( | ||
| resource_revision IS NOT NULL | ||
| AND previous_resource_revision IS NOT NULL | ||
| AND resource_revision = previous_resource_revision + 1 | ||
| ) | ||
| ) | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS session_journal_by_kind_sequence | ||
| ON session_journal(kind, sequence); | ||
| CREATE INDEX IF NOT EXISTS session_journal_by_correlation_sequence | ||
| ON session_journal(correlation_id, sequence) | ||
| WHERE correlation_id IS NOT NULL; | ||
| CREATE TRIGGER IF NOT EXISTS session_journal_reject_update | ||
| BEFORE UPDATE ON session_journal | ||
| BEGIN | ||
| SELECT RAISE(ABORT, 'session journal is append-only'); | ||
| END; | ||
| CREATE TRIGGER IF NOT EXISTS session_journal_reject_delete | ||
| BEFORE DELETE ON session_journal | ||
| BEGIN | ||
| SELECT RAISE(ABORT, 'session journal is append-only'); | ||
| END;", | ||
| )?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Confirm the retention plan for an append-only, unbounded session_journal.
This PR removes the resource_events row-count and byte-budget pruning and stores one journal row per resource revision forever. The session_journal_reject_delete trigger blocks every DELETE, so no future pruning path can trim rows while the trigger exists. A long-lived session database now grows without bound, and payload_json holds the full result and changes for every mutation.
Two consequences to plan for:
- Storage growth is unbounded for long sessions.
- Any later retention or sealed-segment work must first drop or bypass this trigger, which weakens the append-only guarantee unless the trigger allows an explicit retention path.
Consider a WHEN clause on the delete trigger that permits deletion only below a recorded sealed watermark, so retention does not require dropping the invariant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rs`
around lines 139 - 186, Update create_session_journal_schema and the
session_journal_reject_delete trigger to define a retention plan for unbounded
journal growth: add a recorded sealed watermark and allow deletes only for rows
below that watermark via the trigger’s condition, while continuing to reject all
other deletes. Ensure the schema preserves append-only behavior for unsealed
entries and supports future sealed-segment pruning without dropping the trigger.
| fn append_journal_record( | ||
| transaction: &Transaction<'_>, | ||
| append: &JournalAppend<'_>, | ||
| ) -> anyhow::Result<u64> { | ||
| validate_identifier("journal event id", append.event_id)?; | ||
| validate_identifier("journal event kind", append.kind)?; | ||
| let committed_at_ms = | ||
| if append.occurred_at_ms == 0 { unix_epoch_ms()? } else { append.occurred_at_ms }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Read the commit clock for committed_at_ms instead of copying occurred_at_ms.
committed_at_ms is set to append.occurred_at_ms whenever that value is non-zero. The column then never records the actual insert time, and occurred_at_ms == 0 acts as a sentinel that is indistinguishable from a genuine epoch timestamp. Migrated legacy records store occurred_at_ms = 0 and a committed_at_ms of "now", while live records store two identical values.
Read the clock once for committed_at_ms and keep occurred_at_ms as the caller-supplied value.
♻️ Proposed change
- let committed_at_ms =
- if append.occurred_at_ms == 0 { unix_epoch_ms()? } else { append.occurred_at_ms };
+ let committed_at_ms = unix_epoch_ms()?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn append_journal_record( | |
| transaction: &Transaction<'_>, | |
| append: &JournalAppend<'_>, | |
| ) -> anyhow::Result<u64> { | |
| validate_identifier("journal event id", append.event_id)?; | |
| validate_identifier("journal event kind", append.kind)?; | |
| let committed_at_ms = | |
| if append.occurred_at_ms == 0 { unix_epoch_ms()? } else { append.occurred_at_ms }; | |
| fn append_journal_record( | |
| transaction: &Transaction<'_>, | |
| append: &JournalAppend<'_>, | |
| ) -> anyhow::Result<u64> { | |
| validate_identifier("journal event id", append.event_id)?; | |
| validate_identifier("journal event kind", append.kind)?; | |
| let committed_at_ms = unix_epoch_ms()?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rs`
around lines 399 - 406, Update append_journal_record so committed_at_ms always
comes from a single unix_epoch_ms() clock read, while occurred_at_ms remains the
caller-supplied append.occurred_at_ms value, including zero. Remove the
conditional sentinel logic and preserve the existing validation and record
insertion flow.
| assert_eq!( | ||
| registry | ||
| .connection | ||
| .query_row("SELECT COUNT(*) FROM resource_events", [], |row| row.get::<_, i64>(0)) | ||
| .query_row("SELECT COUNT(*) FROM session_journal", [], |row| row.get::<_, i64>(0)) | ||
| .unwrap(), | ||
| 1 | ||
| ); | ||
| let journal = registry.session_journal_after(0, 1).unwrap(); | ||
| assert_eq!(journal.records[0].kind, "workspace.create"); | ||
| for (kind, id) in [ | ||
| ("workspace", workspace(1, "one", "One").public_id.to_string()), | ||
| ("screen", screen_id(1).to_string()), | ||
| ("pane", pane_id(1).to_string()), | ||
| ("tab", tab_id(1).to_string()), | ||
| ("terminal", terminal_resource(TERMINAL_ONE).to_string()), | ||
| ] { | ||
| assert!( | ||
| journal.records[0] | ||
| .subjects | ||
| .iter() | ||
| .any(|subject| subject.kind == kind && subject.id == id), | ||
| "missing {kind} journal subject {id}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Add an assertion for resource_events_after on a freshly written journal record.
This test now verifies the journal row directly. No test in this file verifies that resource_events_after still decodes a record written through append_resource_journal_record. That read path extracts changes with json_extract(payload_json, '$.changes'), so a payload shape change would break the public resource-event contract without failing any test here. The migration test in session_journal.rs only covers a migrated row.
Add an assertion that registry.resource_events_after(0) returns one batch with revision == 1 and the committed deltas.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/tests.rs` around lines
635 - 658, Extend the freshly written journal-record test after validating
journal subjects to call registry.resource_events_after(0). Assert it returns
exactly one batch with revision 1 and the expected committed resource deltas,
covering decoding through append_resource_journal_record rather than only
inspecting the raw session_journal row.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b96c2b6156
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| append_resource_journal_record( | ||
| &tx, | ||
| revision, | ||
| previous_revision, | ||
| "resource-api", | ||
| idempotency_key, | ||
| operation, | ||
| None, | ||
| &outcome_value, | ||
| deltas, | ||
| )?; |
There was a problem hiding this comment.
When commit_success_without_changes handles terminal input, browser pointer actions, or similar ephemeral effects—or when commit_known_failure records any failure—it passes deltas: None, so this append is skipped even though the durable effect receipt is committed. Consequently session.journal.subscribe never reports these accepted effect outcomes, preventing hook/audit consumers from observing them; append a journal record with nullable resource revisions and publish the journal wake even when no projection delta exists.
Useful? React with 👍 / 👎.
| collect_subjects(result, &mut subjects); | ||
| collect_subjects(changes, &mut subjects); | ||
| expand_topology_subjects(transaction, &mut subjects)?; | ||
| let subjects = subjects.into_iter().collect::<Vec<_>>(); |
There was a problem hiding this comment.
When a topology mutation covers a sufficiently large session, this unbounded collection can include every tab, terminal/browser, pane, screen, and workspace; a single pane with roughly 128 tabs already exceeds the SessionJournalRecord.subjects contract of 256 entries. The server still emits that record, but the Zig binding explicitly rejects more than 256 subjects in bindings/zig/src/resource.zig:3895, terminating otherwise valid journal consumption, so the stored envelope and published API limit need to agree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 22
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/bindings/cpp/src/resource.cpp`:
- Around line 1071-1076: Replace the sequential JournalSensitivity checks in the
max_sensitivity mapping with an exhaustive switch, assigning the existing
encoded values for public_ and metadata and explicitly marking secret as
unreachable because it is rejected earlier. Ensure any future enum value
produces a compile-time diagnostic rather than defaulting to sensitive, matching
the sibling classes mapping.
In `@cmux-tui/bindings/go/operations.go`:
- Around line 360-375: Update cmux-tui/bindings/go/operations.go lines 360-375
in Session.Journal to call options.validate() before constructing request
parameters and return any error immediately. Add SessionJournalOptions.validate
in cmux-tui/bindings/go/options.go lines 75-97 to reject simultaneous Cursor and
Start, MaxSensitivity set to JournalSensitivitySecret, and subjects with both
Kind and ID unset, using the binding’s existing client-side argument error type.
In `@cmux-tui/bindings/java/src/com/cmux/Session.java`:
- Around line 481-495: Update the journal decoding logic around
Client.requireExactFields to reject missing or null values for all required
journal fields, including authority and payload, rather than checking only key
presence. Ensure string fields decoded through Wire.string—such as principal_id,
id, kind, and related producer, authority, or subject values—also reject empty
strings using the binding-wide non-empty string contract, matching Go validation
behavior.
In `@cmux-tui/bindings/python/cmux/resources.py`:
- Around line 1711-1787: Update the Session.journal decoding flow around
_journal_record and ResourceStream.next to validate the envelope cursor before
returning the record. Compare record.sequence with the next expected stream
cursor revision, and raise ProtocolError when it does not match; preserve normal
decoding when the sequence is correct.
In `@cmux-tui/bindings/typescript/src/options.ts`:
- Around line 71-78: Update SessionJournalOptions.classes to use the exported
JournalClass type from models.ts instead of duplicating the literal union,
adding the type-only import from ./models.js; leave maxSensitivity as its
explicit subset excluding "secret".
In `@cmux-tui/bindings/typescript/src/resources.ts`:
- Around line 1320-1353: Update the Object.freeze literal returned by the
session journal decoder to include a satisfies SessionJournalRecord assertion,
matching the existing sessionEvent literals at lines 1249, 1265, and 1273. Keep
the current fields and runtime decoding unchanged while enabling compile-time
validation of the returned object shape.
- Around line 1292-1319: Rename the inner authority record binding in the
authority IIFE so it does not shadow the outer authority result, and rename the
subject-loop value binding so it does not shadow the sessionJournalRecord value
parameter; update all references within their respective blocks while preserving
behavior.
- Around line 1026-1041: Update journalOptionsFields to reject options that
provide both cursor and start, and reject any subject filter lacking both kind
and id, before serializing or dispatching the request. Use the existing
TypeScript error-construction conventions and preserve current serialization for
valid SessionJournalOptions.
In `@cmux-tui/bindings/zig/src/resource.zig`:
- Around line 887-906: Update the public SessionJournalRecord.subjects field to
use a const slice of JournalSubject, matching the immutable decoded collections
elsewhere in the file; keep decodeSessionJournalRecord behavior unchanged.
- Around line 3786-3794: Remove the duplicate journalNullableDecimal helper and
update both of its callers to use the existing requiredNullableDecimalU64
function, preserving the missing-field, null, and decimalU64 conversion
behavior.
- Around line 19300-19326: Extend the wire-format test around request_object and
its filter to assert the encoded kinds, classes, and subjects arrays supplied by
the test setup. Retrieve each array using the corresponding filter keys and
validate both their lengths and element values/shapes, covering the encoder
branches in encodeSessionJournalOptions without changing existing start or
max_sensitivity assertions.
In `@cmux-tui/crates/cmux-tui-core/src/server.rs`:
- Around line 6758-6802: Add tests for both `run_session_journal_stream`
termination branches that emit reason `gap`: force a journal page read error and
create an empty page where `last_sequence < head_sequence` by sealing or
truncating retention below the subscriber cursor. Assert the stream sends a
terminal message with reason `gap` and the expected journal cursor, while
preserving existing replay, resume, transport-rejection, and foreign-cursor
coverage.
- Around line 6638-6653: Update both error handlers in the session journal
subscription flow around session_journal_reader and session_journal_page: log
the full underlying error internally, but replace error.to_string() in
ResourceError::operation_failed details with a stable, safe reason code or
minimal sanitized diagnostic. Keep the user-facing operation.failed response
free of raw SQLite, anyhow, SQL, and journal path details.
- Around line 6803-6835: Move the resource stream overflow refresh out of the
per-record loop in the replay handler, placing a single update after all records
in the page have been processed using the final stream.last_sequence cursor.
Keep cancellation and item delivery checks running for every record, including
records rejected by stream.filter.matches, and preserve the existing
early-return behavior.
In `@cmux-tui/crates/cmux-tui/src/cli/command.rs`:
- Around line 2551-2562: Add the mirrored invalid argument cases to the
rejection table in the journal subscribe parsing test: include --sequence
without --cursor-session, non-canonical or out-of-range sequence values such as
"01" and "18446744073709551616", and a subject kind containing uppercase
characters. Keep each case routed through parse and assert that it returns an
error.
In `@cmux-tui/crates/cmux-tui/src/cli/wire.rs`:
- Around line 187-193: Extract the duplicated lifecycle-envelope condition and
write/error mapping from both call sites in the wire command flow into a helper
such as write_lifecycle_envelope. Have it accept the exposure flag, output mode,
and Value, return Ok when no write is needed, and return Err(3) after emitting
the existing stdout error on failure; replace both inline blocks with calls to
this helper.
In `@cmux-tui/spec/cli.md`:
- Line 153: Update the `session journal subscribe` grammar entry to replace
`[FILTERS]` with the concrete `--kinds`, `--classes`, `--subjects`, and
`--max-sensitivity` flags, and document cursor resumption using
`--cursor-session` and `--sequence` rather than `--from`. Match the explicit
flag formatting used by the surrounding CLI entries.
In `@cmux-tui/spec/resource-api-v1.md`:
- Around line 318-323: Update the documentation for session.journal.subscribe in
both cmux-tui/spec/resource-api-v1.md and the operations catalog to explicitly
reject filter.max_sensitivity="secret", while preserving the existing rule that
cursor and start are mutually exclusive.
In `@cmux-tui/spec/resource-operations-v1.json`:
- Around line 526-532: Add a constraint to the session.journal.subscribe
operation documenting that subscribers never receive records whose sensitivity
is secret, even when no filter is provided. Reuse the existing secret-exclusion
wording or constraint structure from JournalFilter and attach it directly to the
subscribe operation without changing the sensitivity schema.
- Around line 8231-8236: Add the trusted-local access constraint to the
session.journal.subscribe operation’s constraints in the specification, using
the same wording as the existing session.shutdown constraint and preserving the
current cursor/start constraints.
In `@cmux-tui/spec/session-journal.md`:
- Around line 127-175: Update cmux-tui/spec/session-journal.md lines 127-175 to
state that cursor and start are mutually exclusive and each subjects filter
entry must include kind, id, or both. Update
cmux-tui/bindings/python/cmux/resources.py lines 243-271 in _journal_options to
reject simultaneous cursor/start, max_sensitivity set to secret, and subject
entries missing both kind and id, matching Session::journal validation in
handles.rs.
- Around line 127-175: Update the Subscription API contract to state that
`start` and `cursor` are mutually exclusive, and that every `subjects` filter
entry must specify at least a `kind` or an `id`. Ensure these validation rules
match the client-side checks in `Session::journal` and are reflected by binding
option validation such as `_journal_options`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e46a9ccc-7f82-43e6-ad26-ee54411a175d
📒 Files selected for processing (58)
cmux-tui/bindings/ERGONOMICS.mdcmux-tui/bindings/conformance/runner.pycmux-tui/bindings/cpp/.cmux-resource-api.jsoncmux-tui/bindings/cpp/include/cmux/resource.hppcmux-tui/bindings/cpp/src/resource.cppcmux-tui/bindings/cpp/src/resource_models.cppcmux-tui/bindings/go/.cmux-resource-api.jsoncmux-tui/bindings/go/internal/wirev1/operations.gocmux-tui/bindings/go/operations.gocmux-tui/bindings/go/options.gocmux-tui/bindings/go/values.gocmux-tui/bindings/java/.cmux-resource-api.jsoncmux-tui/bindings/java/src/com/cmux/Options.javacmux-tui/bindings/java/src/com/cmux/Session.javacmux-tui/bindings/java/src/com/cmux/SessionJournalRecord.javacmux-tui/bindings/java/src/com/cmux/internal/Operations.javacmux-tui/bindings/python/.cmux-resource-api.jsoncmux-tui/bindings/python/cmux/_operations.pycmux-tui/bindings/python/cmux/models.pycmux-tui/bindings/python/cmux/options.pycmux-tui/bindings/python/cmux/resources.pycmux-tui/bindings/rust/.cmux-resource-api.jsoncmux-tui/bindings/rust/src/resource/client.rscmux-tui/bindings/rust/src/resource/handles.rscmux-tui/bindings/rust/src/resource/mod.rscmux-tui/bindings/rust/src/resource/ops.rscmux-tui/bindings/rust/src/resource/options.rscmux-tui/bindings/rust/src/resource/typed_stream.rscmux-tui/bindings/rust/tests/catalog_manifest.rscmux-tui/bindings/rust/tests/operation_reachability.rscmux-tui/bindings/typescript/.cmux-resource-api.jsoncmux-tui/bindings/typescript/src/internal/operations.tscmux-tui/bindings/typescript/src/models.tscmux-tui/bindings/typescript/src/options.tscmux-tui/bindings/typescript/src/resources.tscmux-tui/bindings/zig/.cmux-resource-api.jsoncmux-tui/bindings/zig/src/resource.zigcmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/resource.rscmux-tui/crates/cmux-tui-core/src/resource_router.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui-core/src/workspace_registry.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rscmux-tui/crates/cmux-tui/src/cli.rscmux-tui/crates/cmux-tui/src/cli/command.rscmux-tui/crates/cmux-tui/src/cli/wire.rscmux-tui/scripts/check-resource-api-boundary.pycmux-tui/scripts/test_check_resource_api_boundary.pycmux-tui/spec/README.mdcmux-tui/spec/bindings.mdcmux-tui/spec/cli.mdcmux-tui/spec/inventory.jsoncmux-tui/spec/programmability.mdcmux-tui/spec/resource-api-v1.jsoncmux-tui/spec/resource-api-v1.mdcmux-tui/spec/resource-operations-v1.jsoncmux-tui/spec/resource-operations-v1.mdcmux-tui/spec/session-journal.md
| if (filter.max_sensitivity) { | ||
| const char* value = "sensitive"; | ||
| if (*filter.max_sensitivity == JournalSensitivity::public_) value = "public"; | ||
| if (*filter.max_sensitivity == JournalSensitivity::metadata) value = "metadata"; | ||
| encoded_filter.emplace("max_sensitivity", Json(value)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use an exhaustive switch for max_sensitivity, matching the sibling classes mapping.
The classes filter mapping in the same function uses a switch with one case per JournalClass value, so a new enum value forces a compile-time diagnostic. The max_sensitivity mapping instead uses sequential if checks that default to "sensitive" when neither public_ nor metadata matches. If JournalSensitivity gains a new non-secret value later, it silently encodes as "sensitive" instead of failing to compile or erroring out.
Convert the if-cascade to a switch over JournalSensitivity (with an explicit case JournalSensitivity::secret: marked unreachable, since it is rejected earlier in the function) for consistency and future-proofing.
♻️ Proposed refactor
if (filter.max_sensitivity) {
- const char* value = "sensitive";
- if (*filter.max_sensitivity == JournalSensitivity::public_) value = "public";
- if (*filter.max_sensitivity == JournalSensitivity::metadata) value = "metadata";
- encoded_filter.emplace("max_sensitivity", Json(value));
+ const char* value = "";
+ switch (*filter.max_sensitivity) {
+ case JournalSensitivity::public_: value = "public"; break;
+ case JournalSensitivity::metadata: value = "metadata"; break;
+ case JournalSensitivity::sensitive: value = "sensitive"; break;
+ case JournalSensitivity::secret:
+ return make_error(
+ ErrorCode::invalid_argument,
+ "secret journal records are unavailable in v1");
+ }
+ encoded_filter.emplace("max_sensitivity", Json(value));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (filter.max_sensitivity) { | |
| const char* value = "sensitive"; | |
| if (*filter.max_sensitivity == JournalSensitivity::public_) value = "public"; | |
| if (*filter.max_sensitivity == JournalSensitivity::metadata) value = "metadata"; | |
| encoded_filter.emplace("max_sensitivity", Json(value)); | |
| } | |
| if (filter.max_sensitivity) { | |
| const char* value = ""; | |
| switch (*filter.max_sensitivity) { | |
| case JournalSensitivity::public_: value = "public"; break; | |
| case JournalSensitivity::metadata: value = "metadata"; break; | |
| case JournalSensitivity::sensitive: value = "sensitive"; break; | |
| case JournalSensitivity::secret: | |
| return make_error( | |
| ErrorCode::invalid_argument, | |
| "secret journal records are unavailable in v1"); | |
| } | |
| encoded_filter.emplace("max_sensitivity", Json(value)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/bindings/cpp/src/resource.cpp` around lines 1071 - 1076, Replace the
sequential JournalSensitivity checks in the max_sensitivity mapping with an
exhaustive switch, assigning the existing encoded values for public_ and
metadata and explicitly marking secret as unreachable because it is rejected
earlier. Ensure any future enum value produces a compile-time diagnostic rather
than defaulting to sensitive, matching the sibling classes mapping.
| func (s *Session) Journal(ctx context.Context, options SessionJournalOptions) (*Stream[SessionJournalRecord], error) { | ||
| input := s.route.params() | ||
| if options.Cursor != nil { | ||
| input[wirev1.FieldCursor] = options.Cursor | ||
| } | ||
| if options.Start != nil { | ||
| input[wirev1.FieldStart] = *options.Start | ||
| } | ||
| if options.Filter != nil { | ||
| input["filter"] = options.Filter | ||
| } | ||
| merge(input, options.Extra) | ||
| return openStream( | ||
| ctx, s.client, wirev1.SessionJournalSubscribe, input, decodeSessionJournalRecord, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The Go binding omits the journal option validation that the C++ and Java bindings enforce. The C++ binding validates three rules in SessionJournalOptions::to_params (cmux-tui/bindings/cpp/src/resource.cpp Lines 1018-1081), and the Java binding validates the same rules in Options.JournalFilter, Options.SessionJournal, and Session.journal. The Go binding validates none of them, so invalid option combinations reach the server and fail there instead of at the call site.
cmux-tui/bindings/go/operations.go#L360-L375: calloptions.validate()at the top ofSession.Journaland return the error before building params.cmux-tui/bindings/go/options.go#L75-L97: add avalidate()method onSessionJournalOptionsthat rejectsCursorandStartset together, rejectsFilter.MaxSensitivity == JournalSensitivitySecret, and rejects anyJournalSubjectFilterwith bothKindandIDnil.
🛡️ Proposed validation method for options.go
func (o SessionJournalOptions) validate() error {
if o.Cursor != nil && o.Start != nil {
return &InvalidArgumentError{
Message: "journal cursor and start are mutually exclusive",
}
}
if o.Filter == nil {
return nil
}
if o.Filter.MaxSensitivity != nil &&
*o.Filter.MaxSensitivity == JournalSensitivitySecret {
return &InvalidArgumentError{
Message: "secret journal records are unavailable in v1",
}
}
for _, subject := range o.Filter.Subjects {
if subject.Kind == nil && subject.ID == nil {
return &InvalidArgumentError{
Message: "journal subject filters require kind or id",
}
}
}
return nil
}Replace InvalidArgumentError with the error type this binding already uses for client-side argument rejection.
📍 Affects 2 files
cmux-tui/bindings/go/operations.go#L360-L375(this comment)cmux-tui/bindings/go/options.go#L75-L97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/bindings/go/operations.go` around lines 360 - 375, Update
cmux-tui/bindings/go/operations.go lines 360-375 in Session.Journal to call
options.validate() before constructing request parameters and return any error
immediately. Add SessionJournalOptions.validate in
cmux-tui/bindings/go/options.go lines 75-97 to reject simultaneous Cursor and
Start, MaxSensitivity set to JournalSensitivitySecret, and subjects with both
Kind and ID unset, using the binding’s existing client-side argument error type.
| Client.requireExactFields( | ||
| fields, | ||
| "session journal record", | ||
| "sequence", "event_id", "schema_version", Wire.KIND, "class", "replay", | ||
| "occurred_at_ms", "committed_at_ms", "producer", "authority", | ||
| "causation_id", "correlation_id", "causation_depth", "subjects", | ||
| "sensitivity", "payload", "resource_revision", "previous_resource_revision" | ||
| ); | ||
| for (String required : List.of("authority", "payload")) { | ||
| if (!fields.containsKey(required)) { | ||
| throw new ProtocolError( | ||
| "session journal record omitted required field " + required | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect requireExactFields, integer, and requiredNullableString in the Java Client.
set -euo pipefail
fd -t f 'Client.java' cmux-tui/bindings/java --exec ast-grep outline {} --items all
fd -t f 'Client.java' cmux-tui/bindings/java --exec rg -n -A 20 'static .*requireExactFields|static .*requiredNullableString|static int integer' {}Repository: manaflow-ai/cmux
Length of output: 11088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Session.java relevant sections =="
sed -n '440,535p' cmux-tui/bindings/java/src/com/cmux/Session.java
echo "== Client.java exact/string helpers =="
sed -n '3530,3705p' cmux-tui/bindings/java/src/com/cmux/Client.java
echo "== Go journal string validation =="
rg -n -B3 -A12 'invalid journal (producer|authority|subject)|producer: string|authority: string|subject' cmux-tui/bindings/go/operations.go cmux-tui/bindings/java/src/com/cmux/Session.javaRepository: manaflow-ai/cmux
Length of output: 23363
Align Java journal decoding with Go’s required-field and non-empty validation.
requireExactFields only rejects unknown keys, so fields.get("authority") and JsonVale of("payload") can be null and accepted instead of “session journal record omitted required field …”. Use required present/non-null checks for both, like the Go decoder does for the 18 journal fields.
Wire.string(raw.get("principal_id"), "journal principal_id"), "id", "kind", etc. accepts empty strings, while Go returns invalid journal producer/authority/subject when those strings are empty. Keep the Java helper non-empty or use the same binding-wide non-empty string contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/bindings/java/src/com/cmux/Session.java` around lines 481 - 495,
Update the journal decoding logic around Client.requireExactFields to reject
missing or null values for all required journal fields, including authority and
payload, rather than checking only key presence. Ensure string fields decoded
through Wire.string—such as principal_id, id, kind, and related producer,
authority, or subject values—also reject empty strings using the binding-wide
non-empty string contract, matching Go validation behavior.
| def _journal_record(value: Any) -> SessionJournalRecord: | ||
| payload = _mapping(value, "session journal record") | ||
| _strict_object( | ||
| payload, | ||
| ( | ||
| "sequence", "event_id", "schema_version", "kind", "class", "replay", | ||
| "occurred_at_ms", "committed_at_ms", "producer", "authority", | ||
| "causation_id", "correlation_id", "causation_depth", "subjects", | ||
| "sensitivity", "payload", "resource_revision", | ||
| "previous_resource_revision", | ||
| ), | ||
| "session journal record", | ||
| ) | ||
| for required in ("authority", "payload"): | ||
| if required not in payload: | ||
| raise ProtocolError( | ||
| f"session journal record omitted required field {required}" | ||
| ) | ||
| producer = _mapping(payload.get("producer"), "journal producer") | ||
| _strict_object(producer, ("kind", "id"), "journal producer") | ||
| authority_value = payload.get("authority") | ||
| authority: Optional[JournalAuthority] = None | ||
| if authority_value is not None: | ||
| authority_payload = _mapping(authority_value, "journal authority") | ||
| _strict_object( | ||
| authority_payload, | ||
| ("principal_id", "lease_id", "generation", "role"), | ||
| "journal authority", | ||
| ) | ||
| authority = JournalAuthority( | ||
| _required_string(authority_payload, "principal_id"), | ||
| _required_string(authority_payload, "lease_id"), | ||
| _required_string(authority_payload, "generation"), | ||
| _required_string(authority_payload, "role"), | ||
| ) | ||
| subject_values = payload.get("subjects") | ||
| if not isinstance(subject_values, list): | ||
| raise ProtocolError("journal subjects must be an array") | ||
| subjects = [] | ||
| for subject_value in subject_values: | ||
| subject = _mapping(subject_value, "journal subject") | ||
| _strict_object(subject, ("kind", "id"), "journal subject") | ||
| subjects.append( | ||
| JournalSubject( | ||
| _required_string(subject, "kind"), | ||
| _required_string(subject, "id"), | ||
| ) | ||
| ) | ||
| return SessionJournalRecord( | ||
| _required_decimal(payload, "sequence"), | ||
| _required_string(payload, "event_id"), | ||
| _required_positive_uint32(payload, "schema_version"), | ||
| _required_string(payload, "kind"), | ||
| _required_enum(payload, "class", ("state", "observation", "effect", "checkpoint")), # type: ignore[arg-type] | ||
| _required_enum(payload, "replay", ("required", "advisory", "never")), # type: ignore[arg-type] | ||
| _required_decimal(payload, "occurred_at_ms"), | ||
| _required_decimal(payload, "committed_at_ms"), | ||
| JournalProducer( | ||
| _required_string(producer, "kind"), | ||
| _required_string(producer, "id"), | ||
| ), | ||
| authority, | ||
| _required_nullable_string(payload, "causation_id"), | ||
| _required_nullable_string(payload, "correlation_id"), | ||
| _required_uint16(payload, "causation_depth"), | ||
| tuple(subjects), | ||
| _required_enum( | ||
| payload, | ||
| "sensitivity", | ||
| ("public", "metadata", "sensitive", "secret"), | ||
| ), # type: ignore[arg-type] | ||
| payload["payload"], | ||
| _required_nullable_decimal(payload, "resource_revision"), | ||
| _required_nullable_decimal(payload, "previous_resource_revision"), | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the shared stream-opening implementation for envelope cursor validation.
ast-grep outline cmux-tui/bindings/python/cmux --items all --match '_open_stream'
rg -n -B3 -A40 'def _open_stream' cmux-tui/bindings/python/cmuxRepository: manaflow-ai/cmux
Length of output: 3942
🏁 Script executed:
#!/bin/bash
# Inspect the relevant stream implementation and session journal subscription path.
set -euo pipefail
echo "== files mentioning open_stream / ResourceStream =="
rg -n -B2 -A20 'class .*Stream|def open_stream|ResourceStream|open_stream\(' cmux-tui/bindings/python/cmux
echo
echo "== journal subscription path =="
rg -n -B5 -A25 'def journal|session journal|_journal_record' cmux-tui/bindings/python/cmux/resources.py
echo
echo "== source around stream implementation =="
sed -n '1,220p' cmux-tui/bindings/python/cmux/connection.py
echo
sed -n '2760,2835p' cmux-tui/bindings/python/cmux/resources.pyRepository: manaflow-ai/cmux
Length of output: 49931
🏁 Script executed:
#!/bin/bash
# Inspect stream decoding lifecycle and all sequence/cursor handling.
set -euo pipefail
echo "== ResourceStream next implementation =="
sed -n '1154,1186p' cmux-tui/bindings/python/cmux/_protocol.py
echo
echo "== sequence / revision / cursor references =="
rg -n 'sequence|cursor|revision|envelope' cmux-tui/bindings/python cmux-tui/bindings/python/cpp 2>/dev/null || true
echo
echo "== envelope_cursor references in repository =="
rg -n 'envelope_cursor|journal sequence does not match' . || trueRepository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
#!/bin/bash
# Inspect stream push consumption and cursor validation without a broad repository scan.
set -euo pipefail
echo "== _StreamState push/consumed implementation =="
sed -n '137,220p' cmux-tui/bindings/python/cmux/_protocol.py
echo
echo "== _decode_stream_item_envelope implementation =="
sed -n '1444,1491p' cmux-tui/bindings/python/cmux/_protocol.py
echo
echo "== dispatch stream enqueue =="
sed -n '905,937p' cmux-tui/bindings/python/cmux/_protocol.py
echo
echo "== focused tests for stream cursor/sequence validation =="
sed -n '2717,2945p' cmux-tui/bindings/python/tests/test_resource_api.pyRepository: manaflow-ai/cmux
Length of output: 16027
🏁 Script executed:
#!/bin/bash
# Locate and inspect only the missing _StreamState push implementation evidence.
set -euo pipefail
rg -n "def push|def consumed" cmux-tui/bindings/python/cmux/_protocol.py cmux-tui/bindings/python/cmux/aio.py cmux-tui/bindings/python/cmux/raw/client.py || true
sed -n '220,290p' cmux-tui/bindings/python/cmux/_protocol.pyRepository: manaflow-ai/cmux
Length of output: 2874
🏁 Script executed:
#!/bin/bash
# Inspect StreamState push/failure flow for stream items.
set -euo pipefail
sed -n '269,330p' cmux-tui/bindings/python/cmux/_protocol.pyRepository: manaflow-ai/cmux
Length of output: 2293
Add envelope-cursor/sequence validation for Session.journal.
Session.journal decodes only the payload, while ResourceStream.next returns StreamItem.item without comparing record.sequence to the stream cursor revision. Match the C++ decoder by rejecting a journal record when record.sequence is not the next cursor-revision value.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 1711-1711: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 1726-1728: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 1748-1748: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/bindings/python/cmux/resources.py` around lines 1711 - 1787, Update
the Session.journal decoding flow around _journal_record and ResourceStream.next
to validate the envelope cursor before returning the record. Compare
record.sequence with the next expected stream cursor revision, and raise
ProtocolError when it does not match; preserve normal decoding when the sequence
is correct.
| export interface SessionJournalOptions extends RequestOptions { | ||
| readonly cursor?: Cursor; | ||
| readonly start?: "tail" | "beginning"; | ||
| readonly kinds?: readonly string[]; | ||
| readonly classes?: readonly ("state" | "observation" | "effect" | "checkpoint")[]; | ||
| readonly subjects?: readonly JournalSubjectFilter[]; | ||
| readonly maxSensitivity?: "public" | "metadata" | "sensitive"; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Reuse the exported JournalClass union for classes.
models.ts exports JournalClass with the same four members. Duplicating the literal union here lets the two drift when a journal class is added. Keep maxSensitivity as an explicit subset, because it must exclude "secret".
♻️ Proposed refactor
export interface SessionJournalOptions extends RequestOptions {
readonly cursor?: Cursor;
readonly start?: "tail" | "beginning";
readonly kinds?: readonly string[];
- readonly classes?: readonly ("state" | "observation" | "effect" | "checkpoint")[];
+ readonly classes?: readonly JournalClass[];
readonly subjects?: readonly JournalSubjectFilter[];
readonly maxSensitivity?: "public" | "metadata" | "sensitive";
}Add the import at the top of the file:
import type { JournalClass } from "./models.js";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface SessionJournalOptions extends RequestOptions { | |
| readonly cursor?: Cursor; | |
| readonly start?: "tail" | "beginning"; | |
| readonly kinds?: readonly string[]; | |
| readonly classes?: readonly ("state" | "observation" | "effect" | "checkpoint")[]; | |
| readonly subjects?: readonly JournalSubjectFilter[]; | |
| readonly maxSensitivity?: "public" | "metadata" | "sensitive"; | |
| } | |
| import type { JournalClass } from "./models.js"; | |
| export interface SessionJournalOptions extends RequestOptions { | |
| readonly cursor?: Cursor; | |
| readonly start?: "tail" | "beginning"; | |
| readonly kinds?: readonly string[]; | |
| readonly classes?: readonly JournalClass[]; | |
| readonly subjects?: readonly JournalSubjectFilter[]; | |
| readonly maxSensitivity?: "public" | "metadata" | "sensitive"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/bindings/typescript/src/options.ts` around lines 71 - 78, Update
SessionJournalOptions.classes to use the exported JournalClass type from
models.ts instead of duplicating the literal union, adding the type-only import
from ./models.js; leave maxSensitivity as its explicit subset excluding
"secret".
|
|
||
| session list | ||
| session <selector> open|show|snapshot|events|ping|shutdown | ||
| session <selector> journal subscribe [--from tail|beginning] [FILTERS] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Spell out the journal subscribe flags instead of [FILTERS].
[FILTERS] is a placeholder with no definition in the shown grammar. Readers cannot tell from this line that the actual flags are --kinds, --classes, --subjects, and --max-sensitivity, or that cursor resumption uses --cursor-session/--sequence instead of --from. List the concrete flags, matching the style used for other entries in this table (for example, line 199's --pointer-frame-seq <decimal>).
📝 Proposed doc clarification
-session <selector> journal subscribe [--from tail|beginning] [FILTERS]
+session <selector> journal subscribe [--from tail|beginning | --cursor-session <id> --sequence <n>]
+ [--kinds <glob,...>] [--classes state|observation|effect|checkpoint,...]
+ [--subjects <kind:id>,...] [--max-sensitivity public|metadata|sensitive|secret]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| session <selector> journal subscribe [--from tail|beginning] [FILTERS] | |
| session <selector> journal subscribe [--from tail|beginning | --cursor-session <id> --sequence <n>] | |
| [--kinds <glob,...>] [--classes state|observation|effect|checkpoint,...] | |
| [--subjects <kind:id>,...] [--max-sensitivity public|metadata|sensitive|secret] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/spec/cli.md` at line 153, Update the `session journal subscribe`
grammar entry to replace `[FILTERS]` with the concrete `--kinds`, `--classes`,
`--subjects`, and `--max-sensitivity` flags, and document cursor resumption
using `--cursor-session` and `--sequence` rather than `--from`. Match the
explicit flag formatting used by the surrounding CLI entries.
| `session.journal.subscribe` is the trusted-local append-only feed. Omitting a | ||
| cursor tails from the captured head; `start:"beginning"` replays retained | ||
| history first. Its cursor generation is the immutable session ID and its | ||
| revision is the journal sequence. Filters are server-side and never alter | ||
| cursor order. A slow subscriber receives `stream_end` with reason `gap` and | ||
| the last recoverable cursor, then reconnects from that cursor. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the operations catalog encodes the journal parameter constraints.
set -euo pipefail
fd -t f 'resource-operations-v1.json' cmux-tui/spec --exec jq '.operations["session.journal.subscribe"] // "operation absent"' {}
fd -t f 'resource-operations-v1.md' cmux-tui/spec --exec rg -n -C4 'journal' {}Repository: manaflow-ai/cmux
Length of output: 3480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate spec files =="
fd -t f 'JournalFilter|resource-operations-v1.json|resource-api-v1.md|resource-operations-v1.md' cmux-tui/spec | sort
echo "== resource-operations-v1.json JournalFilter/schema refs =="
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('cmux-tui/spec/resource-operations-v1.json')
data=json.loads(p.read_text())
refs = data.get('schemas', {})
for name in ['SessionJournalRecord','JournalFilter','Cursor','StreamOpened']:
if name in refs:
print(f'-- schema {name} --')
print(json.dumps(refs[name], indent=2)[:2000])
print("-- references to sensitivity or secret --")
for key,val in data.get('schemas', {}).items():
text=json.dumps(val)
if 'sensitivity' in text or 'secret' in text:
print(key)
PY
echo "== resource-operations-v1.md JournalFilter content =="
rg -n -C5 'JournalFilter|sensitivity|secret|cursor and start|validation.invalid|validation' cmux-tui/spec/resource-operations-v1.md || true
echo "== current api paragraph =="
sed -n '300,340p' cmux-tui/spec/resource-api-v1.mdRepository: manaflow-ai/cmux
Length of output: 3055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate spec files =="
fd -t f 'JournalFilter|resource-operations-v1.json|resource-api-v1.md|resource-operations-v1.md' cmux-tui/spec | sort
echo "== resource-operations-v1.json JournalFilter/schema refs =="
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('cmux-tui/spec/resource-operations-v1.json')
data=json.loads(p.read_text())
refs = data.get('schemas', {})
for name in ['SessionJournalRecord','JournalFilter','Cursor','StreamOpened']:
if name in refs:
print(f'-- schema {name} --')
print(json.dumps(refs[name], indent=2)[:2000])
print("-- references to sensitivity or secret --")
for key,val in data.get('schemas', {}).items():
text=json.dumps(val, sort_keys=True)
if 'sensitivity' in text or 'secret' in text:
print(key)
print(text[:2000])
PY
echo "== resource-operations-v1.md JournalFilter content =="
rg -n -C5 'JournalFilter|sensitivity|secret|cursor and start|validation.invalid|validation' cmux-tui/spec/resource-operations-v1.md || true
echo "== current api paragraph =="
sed -n '300,340p' cmux-tui/spec/resource-api-v1.mdRepository: manaflow-ai/cmux
Length of output: 3055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resource operations markdown relevant sections =="
argoffile cmux-tui/spec/resource-operations-v1.md -- -
rg -n -C6 'JournalFilter|sensitivity|cursor and start|validation.invalid|operation.failed|session.journal.subscribe|constraints' cmux-tui/spec/resource-operations-v1.md || true
echo "== resource operations json relevant sections =="
python3 - <<'PY'
import json
from pathlib import Path
data=json.loads(Path('cmux-tui/spec/resource-operations-v1.json').read_text())
for path in [
data.get('operations',{}).get('session.journal.subscribe'),
data.get('schemas',{}).get('JournalFilter'),
data.get('schemas',{}).get('JournalFilterItem'),
data.get('schemas',{}).get('JournalFilterRecord'),
]:
print("---")
print(json.dumps(path, indent=2)[:4000])
PY
echo "== server validation around journal subscribe =="
rg -n -C8 'session\.journal\.subscribe|journal.subscribe|cursor|start|validation\.invalid|max_sensitivity|secret|sensitivity' cmux-tui/crates cmux-tui/bindings -g '*.rs' -g '*.cpp' -g '*.h' || true
echo "== Java Journal options =="
fd -t f '.*' cmux-tui/bindings/java -e java | xargs -r rg -n -C6 'Journal|JournalFilter|max_sensitivity|sensitivity|secret|cursor|start' || trueRepository: manaflow-ai/cmux
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resource operations markdown relevant sections =="
rg -n -C6 'JournalFilter|sensitivity|cursor and start|validation.invalid|operation.failed|session.journal.subscribe|constraints' cmux-tui/spec/resource-operations-v1.md || true
echo "== resource operations json relevant sections =="
python3 - <<'PY'
import json
from pathlib import Path
data=json.loads(Path('cmux-tui/spec/resource-operations-v1.json').read_text())
for path in [
data.get('operations',{}).get('session.journal.subscribe'),
data.get('schemas',{}).get('JournalFilter'),
data.get('schemas',{}).get('JournalFilterItem'),
data.get('schemas',{}).get('JournalFilterRecord'),
]:
print("---")
print(json.dumps(path, indent=2)[:8000])
PY
echo "== server validation around journal subscribe =="
rg -n -C8 'session\.journal\.subscribe|journal.subscribe|cursor|start|validation\.invalid|max_sensitivity|secret|sensitivity' cmux-tui/crates cmux-tui/bindings -g '*.rs' -g '*.cpp' -g '*.h' -g '*.java' || true
echo "== Java Journal options files =="
fd -H -t f '.*' cmux-tui/bindings/java -e java 2>/dev/null | xargs -r rg -n -C6 'Journal|JournalFilter|max_sensitivity|sensitivity|secret|cursor|start' || trueRepository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size candidates =="
wc -l cmux-tui/spec/resource-operations-v1.md cmux-tui/spec/resource-api-v1.md cmux-tui/crates/cmux-tui-core/src/resource_router.rs cmux-tui/bindings/cpp/src/resource.cpp || true
echo "== resource operations json operations block =="
sed -n '80,150p' cmux-tui/spec/resource-operations-v1.json
echo "== resource operations markdown session journal area =="
sed -n '920,975p' cmux-tui/spec/resource-operations-v1.md
echo "== resource operations markdown any journal filter text =="
sed -n '945,970p' cmux-tui/spec/resource-operations-v1.md
rg -n -C10 'JournalFilter|sensitivity|secret|max_sensitivity' cmux-tui/bindings/java -g '*.java' || true
echo "== server journal subscribe validation exact locations =="
sed -n '590,620p' cmux-tui/crates/cmux-tui-core/src/resource_router.rs
rg -n -C3 'session\.journal\.subscribe|JOURNAL_SUBSCRIBE|validation\.invalid|cursor and start|cursor and start|secret|sensitivity|JournalFilter' cmux-tui/crates/cmux-tui-core/src/resource_router.rs
echo "== c++ exact journal area =="
sed -n '1000,1090p' cmux-tui/bindings/cpp/src/resource.cppRepository: manaflow-ai/cmux
Length of output: 29693
Document the rejected session.journal.subscribe parameter combinations.
The operations catalog specifies that cursor and start are mutually exclusive, but it does not encode the filter.max_sensitivity prohibition. Add the secret-filter constraint to cmux-tui/spec/resource-api-v1.md and the operations catalog.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/spec/resource-api-v1.md` around lines 318 - 323, Update the
documentation for session.journal.subscribe in both
cmux-tui/spec/resource-api-v1.md and the operations catalog to explicitly reject
filter.max_sensitivity="secret", while preserving the existing rule that cursor
and start are mutually exclusive.
| "sensitivity": { | ||
| "required": true, | ||
| "type": { | ||
| "kind": "ref", | ||
| "name": "JournalSensitivity" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
State the secret-exclusion guarantee on the record or the stream, not only on the filter.
sensitivity refs JournalSensitivity, which includes secret (line 223). The rule that subscribers never receive secret records appears only as a JournalFilter constraint at line 400. A reader of SessionJournalRecord or of session.journal.subscribe alone cannot see that guarantee, and a subscription that omits filter does not reference JournalFilter at all.
Add the same guarantee as a constraint on session.journal.subscribe, so the delivery rule is stated on the operation that delivers the records.
🛡️ Proposed constraint addition on the operation
"constraints": [
"cursor and start are mutually exclusive.",
"Omitting both starts at the current head and delivers only later records.",
+ "V1 trusted-local subscriptions never deliver secret records, with or without filter.",
"For this operation Cursor.generation is the immutable session ID and Cursor.revision is the journal sequence."
]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/spec/resource-operations-v1.json` around lines 526 - 532, Add a
constraint to the session.journal.subscribe operation documenting that
subscribers never receive records whose sensitivity is secret, even when no
filter is provided. Reuse the existing secret-exclusion wording or constraint
structure from JournalFilter and attach it directly to the subscribe operation
without changing the sensitivity schema.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmux-tui/spec/session-journal.md (1)
168-183: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
operation.unsupportedto the catalog, or correct this prose.This paragraph describes a resident process returning
operation.unsupportedfor pre-negotiation clients, butresource-operations-v1.json'serrorsobject does not define this code, andsession.journal.subscribe's ownerrorslist does not include it. A typed client generated from the catalog has nodetailsschema and no declared code path for this error.Add
operation.unsupportedto the catalog'serrorsobject and to the operations that can raise it, or clarify that this behavior is not yet part of the versioned protocol contract.#!/bin/bash # Description: Confirm whether operation.unsupported exists anywhere in the catalog or server code. set -euo pipefail rg -n 'operation\.unsupported' cmux-tui/spec cmux-tui/crates cmux-tui/bindings 2>/dev/null || echo "no matches found"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/spec/session-journal.md` around lines 168 - 183, Add operation.unsupported to the versioned protocol catalog’s errors object and include it in the declared errors for session.journal.subscribe and any other affected operations, defining its details schema so typed clients can handle it. Ensure the catalog matches the documented pre-negotiation behavior; otherwise revise the journal subscription prose to remove that undocumented guarantee.cmux-tui/crates/cmux-tui/src/cli/command.rs (1)
3672-3674: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftVerify the catalog counts against the ten new journal operations.
This test asserts 106 CLI cases and 113 catalog operations. The exclusion list at lines 3742-3751 names 7 operations, so
expectedholds113 - 7 = 106operations and line 3755 requiresseento equal that exact set. Thecasesvector adds onlysession.journal.subscribe, yet this PR adds ten further journal operations (session.journal.append, the producer, hook, checkpoint, restore, and segment operations) tocmux-tui/bindings/cpp/.cmux-resource-api.jsonand routes all of them inparse_session. Ifcmux-tui/spec/resource-operations-v1.jsonalso declares those ten operations, then 113 is stale and both the count assertion and the set comparison at line 3755 fail.Confirm the catalog operation total, and add a
casesentry for every new journal operation or extend the exclusion list deliberately.#!/bin/bash # Description: Count catalog operations and list the journal operations the CLI test must cover. set -euo pipefail fd -t f 'resource-operations-v1.json' cmux-tui/spec --exec sh -c ' echo "total operations:"; jq ".operations | length" {}; echo "journal operations:"; jq -r ".operations | keys[] | select(startswith(\"session.journal\"))" {}' echo "== journal operations referenced in the CLI test cases ==" rg -n 'session\.journal' cmux-tui/crates/cmux-tui/src/cli/command.rs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui/src/cli/command.rs` around lines 3672 - 3674, Verify the operation total from operation_catalog against the current resource specification, then update the CLI test near the cases vector and its count assertions to include every newly declared session.journal operation, including append, producer, hook, checkpoint, restore, and segment operations. Keep the exclusion list intentional and ensure the expected/seen set comparison remains consistent with the complete catalog.
♻️ Duplicate comments (4)
cmux-tui/spec/session-journal.md (2)
146-162: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winState the sensitivity ordering and the default when
max_sensitivityis omitted.This section lists
public,metadata, andsensitiveas acceptedmax_sensitivityvalues, and separately says Unix clients may read throughsensitivewhile remote clients are capped atmetadata. It never states the ordering explicitly (whethersensitiveincludespublicandmetadatarecords), and it never states the default behavior whenmax_sensitivityis omitted from the filter.By contrast,
JournalHookFilterin the operations catalog explicitly states "max_sensitivitydefaults tometadata." Add the same explicit ordering and default statement here forJournalFilter, since ambiguous filtering can exposesensitive/secretpayloads or silently drop permitted records.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/spec/session-journal.md` around lines 146 - 162, Update the JournalFilter sensitivity description to explicitly define the ordering, stating that higher thresholds include all lower-sensitivity records, and state that omitted max_sensitivity defaults to metadata. Keep the existing client-specific caps and secret-record restriction unchanged.
139-152: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument that
cursorandstartare mutually exclusive and that a subject filter requireskindorid.This section never states that
cursorandstartcannot both be set onsession.journal.subscribe, even though the operations catalog enforces this as a hard constraint and the Rust binding (Session::journalinhandles.rs) rejects it client-side. It also never states that eachsubjectsfilter entry needs at leastkindorid, even thoughJournalSubjectFilter.constraintsin the catalog requires it.State both rules explicitly here so every binding implements the same input validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/spec/session-journal.md` around lines 139 - 152, Update the session journal subscription documentation to explicitly state that cursor and start are mutually exclusive. Also document that every subjects filter entry must specify at least a kind or an id, matching the validation enforced by Session::journal and JournalSubjectFilter.constraints.cmux-tui/spec/cli.md (1)
153-164: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpell out the remaining
journal subscribeflags instead of[FILTERS].Line 153 still uses the placeholder
[FILTERS]. Readers cannot tell that the real flags are--kinds,--classes,--subjects, and--max-sensitivity, or that cursor resumption uses--cursor-session/--sequenceinstead of--from. The worked examples insession-journal.md(lines 169-175) show these concrete flags, but this grammar line does not.List the concrete flags here, matching the explicit style used for other entries in this table.
📝 Proposed doc clarification
-session <selector> journal subscribe [--from tail|beginning] [FILTERS] - [--regex <pattern>] [--regex-field kind|subjects|payload|record] [--ignore-case] +session <selector> journal subscribe [--from tail|beginning | --cursor-session <id> --sequence <n>] + [--kinds <glob,...>] [--classes state|observation|effect|checkpoint,...] + [--subjects <kind:id>,...] [--max-sensitivity public|metadata|sensitive] + [--regex <pattern>] [--regex-field kind|subjects|payload|record] [--ignore-case]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/spec/cli.md` around lines 153 - 164, Update the `journal subscribe` grammar entry to replace `[FILTERS]` with the concrete `--kinds`, `--classes`, `--subjects`, and `--max-sensitivity` options, and document cursor resumption with `--cursor-session` and `--sequence` instead of `--from`. Match the explicit flag style used by the surrounding command entries.cmux-tui/spec/resource-operations-v1.json (1)
8864-8942: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestate the secret-exclusion guarantee on
session.journal.subscribeitself.
filteris optional on this operation. The secret-exclusion guarantee currently lives only inJournalFilter.constraints(line 905), so a subscription that omitsfilternever references that guarantee at all. State the guarantee directly on the operation's ownconstraintsarray, since it describes what the stream will ever deliver, independent of whether a filter is supplied.🛡️ Proposed constraint addition
"constraints": [ "cursor and start are mutually exclusive.", "Omitting both starts at the current head and delivers only later records.", + "This stream never delivers secret records, with or without filter; remote transports are capped at metadata.", "For this operation Cursor.generation is the immutable session ID and Cursor.revision is the journal sequence." ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/spec/resource-operations-v1.json` around lines 8864 - 8942, Update the constraints array for session.journal.subscribe to state directly that the stream never delivers secret entries, regardless of whether the optional filter is supplied. Keep the existing cursor/start constraints unchanged and do not rely solely on JournalFilter.constraints for this guarantee.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/bindings/go/options.go`:
- Line 88: Replace the bare string type of Field with a named JournalRegexField
type, define constants for the supported values kind, subjects, payload, and
record, and update the surrounding options declarations consistently with the
existing JournalClass and JournalSensitivity patterns. Ensure callers can use
the constants and invalid plural values such as “kinds” are not represented as
valid field constants.
In `@cmux-tui/bindings/java/src/com/cmux/Options.java`:
- Around line 97-109: Add a two-argument JournalRegexFilter constructor taking
pattern and field that delegates to the existing three-argument constructor with
caseSensitive defaulting to true. Preserve the current null-field handling and
validation in the compact JournalRegexFilter constructor, matching the overload
pattern used by JournalFilter, ClientSizing, and LayoutUndo.
In `@cmux-tui/bindings/zig/src/resource.zig`:
- Around line 5937-5955: Extend the existing journal filter tests around
encodeSessionJournalOptions to supply filter.regex and assert the encoded regex
object contains pattern, the default field, and default case_sensitive values.
Add boundary coverage verifying empty and over-1024-byte patterns return
error.InvalidJournalFilter, while a 1-1024-byte pattern encodes successfully.
In `@cmux-tui/crates/cmux-tui-core/src/journal_checkpoint.rs`:
- Around line 251-269: The upsert branch in apply_resource_change currently
sorts the target collection after every change. Remove that per-change sort,
track collections modified during the record, and sort each touched collection
once after all changes for the record have been applied, preserving the existing
index-then-id ordering used by the values.sort_by comparator.
- Around line 207-210: Update the snapshot cursor handling around the resource
revision assignment to validate that the existing “cursor” value is an object or
null before setting “revision”; if it is any other JSON type, return an error
instead of using panicking Value index assignment. Preserve creating an empty
cursor object when the entry is absent, and propagate the error through
restore_preview.
In `@cmux-tui/crates/cmux-tui-core/src/journal_hooks.rs`:
- Around line 448-453: Capture the child process group ID before waiting, then
use that stored ID for termination. In the Ok(Some(status)) success path of the
wait_timeout flow, terminate the process group before the child is reaped rather
than calling terminate_hook_process_group afterward; update terminate_hook_child
similarly wherever it runs after child.wait().
- Around line 185-228: Add an advancement guard in the scan loop after computing
scanned_to in the journal hook processing flow: if scanned_to is less than or
equal to hook.cursor_sequence, stop processing and return without scheduling
further deliveries. Keep normal scheduling and cursor updates unchanged when the
page advances, preventing non-progressing pages from spinning.
- Line 400: Guard manifest.exec.argv for emptiness before the Command
construction and any argv slicing in the journal hook delivery flow. Return a
delivery error through the existing error path so the worker still reaches
finish_journal_hook_delivery and completed.send(key), while preserving normal
execution for non-empty argv.
- Around line 341-354: Update the worker closure in the journal-hook delivery
spawn to install a drop guard that sends the DeliveryKey through completed
whenever the closure exits, including during panic unwinding from
execute_delivery. Remove the normal-path-only completed.send(key) call and
ensure the guard owns the key and sender without changing the existing delivery
completion logic.
- Around line 166-169: Update the existing-hook branch in the hook
synchronization logic around hook.manifest assignment to recompile the hook’s
CompiledHookFilter from the replacement manifest before continuing. Store the
newly compiled filter with the updated manifest, and remove or skip the hook
when compilation fails so no stale filter remains.
In `@cmux-tui/crates/cmux-tui-core/src/journal_kernel.rs`:
- Around line 32-48: Update JournalDocument::new to return a Result instead of
panicking on either serde_json serialization failure, and propagate that error
through the mux-session-journal-fanout path. When document construction fails,
mark the fanout state unavailable and stop advancing/subscriber processing
rather than allowing the tailer thread to unwind while state.available remains
true.
- Around line 386-427: Update the tailer logic around the reader.after
pagination and wait loop to track a retry-needed state: when a read fails, retry
after the one-second wait timeout instead of waiting only for a requested_epoch
change, while preserving Unavailable reporting until recovery. Do not advance
last_sequence while collecting pages; advance it only as records are
successfully published under the kernel state update, so discarded records are
read again after a later-page failure.
- Around line 259-266: Update the record selection in the journal read path
around SharedJournalRead::Page to use partition_point on the ascending record
sequences to locate the first record greater than sequence, then take at most
limit records from that position. Preserve cloning and page construction while
avoiding a full state.records scan for each read.
In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 4400-4416: Update put_journal_producer to call
journal_kernel.install_producer before workspace_registry.put_journal_producer,
matching the validation-before-durable-write order used by
append_journal_ingress. First verify install_producer is idempotent and rejects
invalid manifests without side effects; preserve the existing commit result and
publish_journal_event behavior after the reordered operations.
- Around line 4547-4569: Bound record accumulation in journal_restore_preview
with an explicit maximum replay count, stopping pagination once the cap is
reached even if the journal head continues advancing. Mark the returned preview
payload as truncated when records were omitted, while preserving complete
previews when the checkpoint tail fits within the cap. Update the
session-journal specification and binding models if the public payload gains a
truncated field.
In `@cmux-tui/crates/cmux-tui-core/src/server.rs`:
- Around line 6897-6913: The journal error handling must stop exposing raw
anyhow text in user-visible responses. In server.rs lines 6897-6913, update
journal_extension_error to match typed journal errors, return stable reason
codes without details.error, and log the underlying error; in lines 6994-7004,
log the catch-up reader error and return reason_code journal_reader_unavailable
instead of error.to_string().
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/journal_extensions.rs`:
- Around line 219-236: Scope journal idempotency receipts by origin: update the
journal_operation_receipts and journal_ingress_receipts schemas to store origin
and include it in each composite primary key, then update operation_receipt and
ingress_receipt lookups and writes to use the caller’s public_client_id-derived
origin with the idempotency key. Preserve payload fingerprint validation while
ensuring receipts from different origins cannot be reused.
- Around line 941-964: Replace the per-item query_session_journal_after call
inside the rows.into_iter() mapping with one bounded batch fetch covering all
collected delivery sequences, from the minimum through maximum sequence (or a
single sequence-selection query). Index the fetched journal records by sequence,
then reuse that index while constructing each JournalHookDelivery and preserve
the existing missing-record and conversion errors.
- Around line 294-302: Remove the duplicated journal_event_index DDL and
backfill from the current migration, and reuse the existing
session_journal::create_session_journal_schema or a dedicated
create_journal_event_index_schema helper. Ensure the shared schema path remains
aligned with ensure_journal_event_index_schema and still performs the required
backfill from session_journal.
- Around line 1487-1496: Update the seal path’s trigger management to use
idempotent DROP and CREATE statements for session_journal_reject_delete, so
sealing succeeds whether the trigger is currently present or absent.
In `@cmux-tui/crates/cmux-tui/src/cli/command.rs`:
- Around line 2128-2130: Move the `--event-json` object validation into the
`journal append` command arm, before `apply_journal_hook_context`, so non-object
JSON always returns the same local `UsageError` regardless of
`CMUX_JOURNAL_HOOK_ID`, `CMUX_JOURNAL_HOOK_NAME`, or
`CMUX_JOURNAL_HOOK_DESCRIPTION`; keep the existing context application behavior
for valid object events.
- Around line 2816-2839: Expand the tests around apply_journal_hook_context to
cover the four untested branches: filter empty environment values, preserve an
existing non-null causation_id, avoid appending a duplicate hook subject, and
return a usage error when subjects is not an array. Add focused cases or
assertions alongside journal_append_inherits_scoped_hook_causation while
preserving the existing all-values-present behavior.
In `@cmux-tui/crates/cmux-tui/src/cli/wire.rs`:
- Around line 153-161: Update the error construction in the journal subscription
handling around global.session so details.session is included only when the user
explicitly supplied a session name. Do not substitute the "main" fallback;
preserve the capability and restart_session details and omit the session field
for socket-selected or otherwise unnamed targets.
In `@cmux-tui/crates/cmux-tui/tests/resource_cli_v1.rs`:
- Around line 827-833: Extend the journal subscription test near
journal_subscription_rejects_a_stale_session_before_sending_the_new_envelope to
cover a capable server: have the fake server respond to identify with the
session-journal-v1 capability, then handle the subsequent
session.journal.subscribe request, and assert both requests are received in
order.
---
Outside diff comments:
In `@cmux-tui/crates/cmux-tui/src/cli/command.rs`:
- Around line 3672-3674: Verify the operation total from operation_catalog
against the current resource specification, then update the CLI test near the
cases vector and its count assertions to include every newly declared
session.journal operation, including append, producer, hook, checkpoint,
restore, and segment operations. Keep the exclusion list intentional and ensure
the expected/seen set comparison remains consistent with the complete catalog.
In `@cmux-tui/spec/session-journal.md`:
- Around line 168-183: Add operation.unsupported to the versioned protocol
catalog’s errors object and include it in the declared errors for
session.journal.subscribe and any other affected operations, defining its
details schema so typed clients can handle it. Ensure the catalog matches the
documented pre-negotiation behavior; otherwise revise the journal subscription
prose to remove that undocumented guarantee.
---
Duplicate comments:
In `@cmux-tui/spec/cli.md`:
- Around line 153-164: Update the `journal subscribe` grammar entry to replace
`[FILTERS]` with the concrete `--kinds`, `--classes`, `--subjects`, and
`--max-sensitivity` options, and document cursor resumption with
`--cursor-session` and `--sequence` instead of `--from`. Match the explicit flag
style used by the surrounding command entries.
In `@cmux-tui/spec/resource-operations-v1.json`:
- Around line 8864-8942: Update the constraints array for
session.journal.subscribe to state directly that the stream never delivers
secret entries, regardless of whether the optional filter is supplied. Keep the
existing cursor/start constraints unchanged and do not rely solely on
JournalFilter.constraints for this guarantee.
In `@cmux-tui/spec/session-journal.md`:
- Around line 146-162: Update the JournalFilter sensitivity description to
explicitly define the ordering, stating that higher thresholds include all
lower-sensitivity records, and state that omitted max_sensitivity defaults to
metadata. Keep the existing client-specific caps and secret-record restriction
unchanged.
- Around line 139-152: Update the session journal subscription documentation to
explicitly state that cursor and start are mutually exclusive. Also document
that every subjects filter entry must specify at least a kind or an id, matching
the validation enforced by Session::journal and
JournalSubjectFilter.constraints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87375819-67eb-4fd3-b03d-5090f8139c0a
⛔ Files ignored due to path filters (1)
cmux-tui/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
cmux-tui/Cargo.tomlcmux-tui/bindings/cpp/.cmux-resource-api.jsoncmux-tui/bindings/cpp/include/cmux/resource.hppcmux-tui/bindings/cpp/src/resource.cppcmux-tui/bindings/go/.cmux-resource-api.jsoncmux-tui/bindings/go/options.gocmux-tui/bindings/java/.cmux-resource-api.jsoncmux-tui/bindings/java/src/com/cmux/Options.javacmux-tui/bindings/java/src/com/cmux/Session.javacmux-tui/bindings/python/.cmux-resource-api.jsoncmux-tui/bindings/python/cmux/options.pycmux-tui/bindings/python/cmux/resources.pycmux-tui/bindings/rust/.cmux-resource-api.jsoncmux-tui/bindings/rust/src/resource/handles.rscmux-tui/bindings/rust/src/resource/options.rscmux-tui/bindings/typescript/.cmux-resource-api.jsoncmux-tui/bindings/typescript/src/options.tscmux-tui/bindings/typescript/src/resources.tscmux-tui/bindings/zig/.cmux-resource-api.jsoncmux-tui/bindings/zig/src/resource.zigcmux-tui/crates/cmux-tui-core/Cargo.tomlcmux-tui/crates/cmux-tui-core/src/journal_checkpoint.rscmux-tui/crates/cmux-tui-core/src/journal_hooks.rscmux-tui/crates/cmux-tui-core/src/journal_kernel.rscmux-tui/crates/cmux-tui-core/src/lib.rscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/resource.rscmux-tui/crates/cmux-tui-core/src/resource_router.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui-core/src/workspace_registry.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/journal_extensions.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rscmux-tui/crates/cmux-tui/src/cli.rscmux-tui/crates/cmux-tui/src/cli/command.rscmux-tui/crates/cmux-tui/src/cli/wire.rscmux-tui/crates/cmux-tui/tests/resource_cli_v1.rscmux-tui/scripts/check-resource-api-boundary.pycmux-tui/scripts/test_check_resource_api_boundary.pycmux-tui/spec/cli.mdcmux-tui/spec/commands.mdcmux-tui/spec/inventory.jsoncmux-tui/spec/programmability.mdcmux-tui/spec/resource-api-v1.jsoncmux-tui/spec/resource-api-v1.mdcmux-tui/spec/resource-operations-v1.jsoncmux-tui/spec/resource-operations-v1.mdcmux-tui/spec/session-journal.md
| } | ||
| type JournalRegexFilter struct { | ||
| Pattern string `json:"pattern"` | ||
| Field string `json:"field,omitempty"` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Give Field a named type with constants, as the neighbouring journal fields have.
Classes uses []JournalClass and MaxSensitivity uses *JournalSensitivity, but Field is a bare string. The C++ and Java bindings both define a JournalRegexField enum, and the CLI validates the value against kind, subjects, payload, and record. A Go caller who passes "kinds" learns about the mistake only from the server.
♻️ Proposed fix
+type JournalRegexField string
+
+const (
+ JournalRegexFieldKind JournalRegexField = "kind"
+ JournalRegexFieldSubjects JournalRegexField = "subjects"
+ JournalRegexFieldPayload JournalRegexField = "payload"
+ JournalRegexFieldRecord JournalRegexField = "record"
+)
+
type JournalRegexFilter struct {
- Pattern string `json:"pattern"`
- Field string `json:"field,omitempty"`
- CaseSensitive *bool `json:"case_sensitive,omitempty"`
+ Pattern string `json:"pattern"`
+ Field JournalRegexField `json:"field,omitempty"`
+ CaseSensitive *bool `json:"case_sensitive,omitempty"`
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/bindings/go/options.go` at line 88, Replace the bare string type of
Field with a named JournalRegexField type, define constants for the supported
values kind, subjects, payload, and record, and update the surrounding options
declarations consistently with the existing JournalClass and JournalSensitivity
patterns. Ensure callers can use the constants and invalid plural values such as
“kinds” are not represented as valid field constants.
| public record JournalRegexFilter( | ||
| String pattern, | ||
| JournalRegexField field, | ||
| boolean caseSensitive | ||
| ) { | ||
| public JournalRegexFilter { | ||
| Objects.requireNonNull(pattern, "pattern"); | ||
| field = field == null ? JournalRegexField.RECORD : field; | ||
| if (pattern.isEmpty() || pattern.getBytes(java.nio.charset.StandardCharsets.UTF_8).length > 1024) { | ||
| throw new IllegalArgumentException("journal regex must contain 1 to 1024 UTF-8 bytes"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a convenience constructor that applies the documented regex defaults.
The compact constructor defaults a null field to RECORD, but caseSensitive is a primitive with no default path. Every caller must pass all three components, so new JournalRegexFilter("ready", null, false) reads like a default construction while it selects case-insensitive matching. The C++ struct defaults case_sensitive to true, and the CLI sends true unless --ignore-case is present.
This file already uses overloads for the same purpose in JournalFilter, ClientSizing, and LayoutUndo.
♻️ Proposed fix
public record JournalRegexFilter(
String pattern,
JournalRegexField field,
boolean caseSensitive
) {
+ public JournalRegexFilter(String pattern) {
+ this(pattern, JournalRegexField.RECORD, true);
+ }
+
+ public JournalRegexFilter(String pattern, JournalRegexField field) {
+ this(pattern, field, true);
+ }
+
public JournalRegexFilter {
Objects.requireNonNull(pattern, "pattern");
field = field == null ? JournalRegexField.RECORD : field;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record JournalRegexFilter( | |
| String pattern, | |
| JournalRegexField field, | |
| boolean caseSensitive | |
| ) { | |
| public JournalRegexFilter { | |
| Objects.requireNonNull(pattern, "pattern"); | |
| field = field == null ? JournalRegexField.RECORD : field; | |
| if (pattern.isEmpty() || pattern.getBytes(java.nio.charset.StandardCharsets.UTF_8).length > 1024) { | |
| throw new IllegalArgumentException("journal regex must contain 1 to 1024 UTF-8 bytes"); | |
| } | |
| } | |
| } | |
| public record JournalRegexFilter( | |
| String pattern, | |
| JournalRegexField field, | |
| boolean caseSensitive | |
| ) { | |
| public JournalRegexFilter(String pattern) { | |
| this(pattern, JournalRegexField.RECORD, true); | |
| } | |
| public JournalRegexFilter(String pattern, JournalRegexField field) { | |
| this(pattern, field, true); | |
| } | |
| public JournalRegexFilter { | |
| Objects.requireNonNull(pattern, "pattern"); | |
| field = field == null ? JournalRegexField.RECORD : field; | |
| if (pattern.isEmpty() || pattern.getBytes(java.nio.charset.StandardCharsets.UTF_8).length > 1024) { | |
| throw new IllegalArgumentException("journal regex must contain 1 to 1024 UTF-8 bytes"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/bindings/java/src/com/cmux/Options.java` around lines 97 - 109, Add
a two-argument JournalRegexFilter constructor taking pattern and field that
delegates to the existing three-argument constructor with caseSensitive
defaulting to true. Preserve the current null-field handling and validation in
the compact JournalRegexFilter constructor, matching the overload pattern used
by JournalFilter, ClientSizing, and LayoutUndo.
| if (filter.regex) |regex_filter| { | ||
| if (regex_filter.pattern.len == 0 or regex_filter.pattern.len > 1024) { | ||
| return error.InvalidJournalFilter; | ||
| } | ||
| var encoded = raw.wire.Object.init(allocator); | ||
| try encoded.put( | ||
| try allocator.dupe(u8, "pattern"), | ||
| .{ .string = try allocator.dupe(u8, regex_filter.pattern) }, | ||
| ); | ||
| try encoded.put( | ||
| try allocator.dupe(u8, "field"), | ||
| .{ .string = try allocator.dupe(u8, @tagName(regex_filter.field)) }, | ||
| ); | ||
| try encoded.put( | ||
| try allocator.dupe(u8, "case_sensitive"), | ||
| .{ .bool = regex_filter.case_sensitive }, | ||
| ); | ||
| try encoded_filter.put("regex", .{ .object = encoded }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add test coverage for the new regex journal filter.
encodeSessionJournalOptions gains a regex branch that validates pattern length (1-1024 bytes) and encodes pattern, field, and case_sensitive. No test exercises this branch: the existing journal filter test at line 19280 does not set filter.regex, so the encoded regex object, its default field, its default case_sensitive, and the error.InvalidJournalFilter boundary for an empty or over-long pattern are all untested.
Add assertions for a supplied regex filter and a boundary test for an invalid pattern.
Also applies to: 19280-19359
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/bindings/zig/src/resource.zig` around lines 5937 - 5955, Extend the
existing journal filter tests around encodeSessionJournalOptions to supply
filter.regex and assert the encoded regex object contains pattern, the default
field, and default case_sensitive values. Add boundary coverage verifying empty
and over-1024-byte patterns return error.InvalidJournalFilter, while a
1-1024-byte pattern encodes successfully.
| if let Some(revision) = record.resource_revision { | ||
| snapshot.entry("cursor").or_insert_with(|| json!({}))["revision"] = | ||
| Value::String(revision.to_string()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid the panicking index assignment on cursor.
snapshot.entry("cursor").or_insert_with(|| json!({}))["revision"] = ... uses IndexMut<&str> on a Value. That implementation panics when the value is present and is neither an object nor null. A checkpoint whose session_snapshot.cursor is a string or a number therefore aborts the request thread instead of returning an error. restore_preview runs on the session.journal.restore.preview request path.
Fail closed with an error instead.
🛡️ Proposed change
if let Some(revision) = record.resource_revision {
- snapshot.entry("cursor").or_insert_with(|| json!({}))["revision"] =
- Value::String(revision.to_string());
+ let cursor = snapshot.entry("cursor").or_insert_with(|| json!({}));
+ let cursor = cursor
+ .as_object_mut()
+ .context("checkpoint session_snapshot cursor is not an object")?;
+ cursor.insert("revision".into(), Value::String(revision.to_string()));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(revision) = record.resource_revision { | |
| snapshot.entry("cursor").or_insert_with(|| json!({}))["revision"] = | |
| Value::String(revision.to_string()); | |
| } | |
| if let Some(revision) = record.resource_revision { | |
| let cursor = snapshot.entry("cursor").or_insert_with(|| json!({})); | |
| let cursor = cursor | |
| .as_object_mut() | |
| .context("checkpoint session_snapshot cursor is not an object")?; | |
| cursor.insert("revision".into(), Value::String(revision.to_string())); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui-core/src/journal_checkpoint.rs` around lines 207 -
210, Update the snapshot cursor handling around the resource revision assignment
to validate that the existing “cursor” value is an object or null before setting
“revision”; if it is any other JSON type, return an error instead of using
panicking Value index assignment. Preserve creating an empty cursor object when
the entry is absent, and propagate the error through restore_preview.
| match kind { | ||
| "upsert" => { | ||
| let Some(value) = change.get("value") else { return Ok(false) }; | ||
| if let Some(existing) = | ||
| values.iter_mut().find(|candidate| candidate["id"].as_str() == Some(id)) | ||
| { | ||
| *existing = value.clone(); | ||
| } else { | ||
| values.push(value.clone()); | ||
| } | ||
| values.sort_by(|left, right| { | ||
| left["index"] | ||
| .as_u64() | ||
| .unwrap_or(u64::MAX) | ||
| .cmp(&right["index"].as_u64().unwrap_or(u64::MAX)) | ||
| .then_with(|| left["id"].as_str().cmp(&right["id"].as_str())) | ||
| }); | ||
| Ok(true) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Sort each collection once per record, not once per change.
apply_resource_change re-sorts the whole target collection after every upsert. A record that upserts k items into a collection of n items costs O(k · n log n), and restore_preview calls this for every replayed record. Apply all changes for one record first, then sort each touched collection once.
As per path instructions: "repeated sort/filter/map work in hot paths".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui-core/src/journal_checkpoint.rs` around lines 251 -
269, The upsert branch in apply_resource_change currently sorts the target
collection after every change. Remove that per-change sort, track collections
modified during the record, and sort each touched collection once after all
changes for the record have been applied, preserving the existing index-then-id
ordering used by the values.sort_by comparator.
Source: Path instructions
| tx.execute_batch("DROP TRIGGER session_journal_reject_delete;")?; | ||
| tx.execute( | ||
| "DELETE FROM session_journal WHERE sequence <= ?1", | ||
| [i64::try_from(through_sequence)?], | ||
| )?; | ||
| tx.execute_batch( | ||
| "CREATE TRIGGER session_journal_reject_delete | ||
| BEFORE DELETE ON session_journal | ||
| BEGIN SELECT RAISE(ABORT, 'session journal is append-only'); END;", | ||
| )?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use DROP TRIGGER IF EXISTS in the seal path.
tx.execute_batch("DROP TRIGGER session_journal_reject_delete;") fails if the trigger is absent. A database created before the trigger existed, or one where a prior recovery path removed it, then aborts every seal request. The recreate statement also lacks IF NOT EXISTS.
🛡️ Proposed change
- tx.execute_batch("DROP TRIGGER session_journal_reject_delete;")?;
+ tx.execute_batch("DROP TRIGGER IF EXISTS session_journal_reject_delete;")?;
@@
tx.execute_batch(
- "CREATE TRIGGER session_journal_reject_delete
+ "CREATE TRIGGER IF NOT EXISTS session_journal_reject_delete
BEFORE DELETE ON session_journal
BEGIN SELECT RAISE(ABORT, 'session journal is append-only'); END;",
)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tx.execute_batch("DROP TRIGGER session_journal_reject_delete;")?; | |
| tx.execute( | |
| "DELETE FROM session_journal WHERE sequence <= ?1", | |
| [i64::try_from(through_sequence)?], | |
| )?; | |
| tx.execute_batch( | |
| "CREATE TRIGGER session_journal_reject_delete | |
| BEFORE DELETE ON session_journal | |
| BEGIN SELECT RAISE(ABORT, 'session journal is append-only'); END;", | |
| )?; | |
| tx.execute_batch("DROP TRIGGER IF EXISTS session_journal_reject_delete;")?; | |
| tx.execute( | |
| "DELETE FROM session_journal WHERE sequence <= ?1", | |
| [i64::try_from(through_sequence)?], | |
| )?; | |
| tx.execute_batch( | |
| "CREATE TRIGGER IF NOT EXISTS session_journal_reject_delete | |
| BEFORE DELETE ON session_journal | |
| BEGIN SELECT RAISE(ABORT, 'session journal is append-only'); END;", | |
| )?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/journal_extensions.rs`
around lines 1487 - 1496, Update the seal path’s trigger management to use
idempotent DROP and CREATE statements for session_journal_reject_delete, so
sealing succeeds whether the trigger is currently present or absent.
| let object = event | ||
| .as_object_mut() | ||
| .ok_or_else(|| UsageError::new("--event-json must contain a JSON object"))?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Local validation of --event-json depends on ambient environment variables.
apply_journal_hook_context returns early at line 2125 when all three environment variables are absent or empty. The object check at lines 2128-2130 then never runs. cmux session <selector> journal append --event-json '"text"' therefore fails locally with a usage error when CMUX_JOURNAL_HOOK_ID is set, and reaches the server when it is not. Validate the event shape in the journal append arm so the local error is deterministic.
♻️ Proposed fix in the journal append arm
[selector, "journal", "append"] => {
selectors.insert("session", "session", selector)?;
let mut event = parse_json_flag(flags, "event-json")?;
+ if !event.is_object() {
+ return Err(UsageError::new("--event-json must contain a JSON object"));
+ }
let hook_id = std::env::var("CMUX_JOURNAL_HOOK_ID").ok();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui/src/cli/command.rs` around lines 2128 - 2130, Move
the `--event-json` object validation into the `journal append` command arm,
before `apply_journal_hook_context`, so non-object JSON always returns the same
local `UsageError` regardless of `CMUX_JOURNAL_HOOK_ID`,
`CMUX_JOURNAL_HOOK_NAME`, or `CMUX_JOURNAL_HOOK_DESCRIPTION`; keep the existing
context application behavior for valid object events.
| #[test] | ||
| fn journal_append_inherits_scoped_hook_causation() { | ||
| let mut event = json!({ | ||
| "producer_id":"demo", | ||
| "subjects":[{"kind":"workspace","id":"ws_test"}], | ||
| "payload":{}, | ||
| }); | ||
| apply_journal_hook_context( | ||
| &mut event, | ||
| Some("demo_hook"), | ||
| Some("event_hook_started"), | ||
| Some("demo_hook:1:event_source"), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(event["causation_id"], "event_hook_started"); | ||
| assert_eq!(event["correlation_id"], "demo_hook:1:event_source"); | ||
| assert_eq!( | ||
| event["subjects"], | ||
| json!([ | ||
| {"kind":"workspace","id":"ws_test"}, | ||
| {"kind":"hook","id":"demo_hook"}, | ||
| ]) | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the remaining branches of apply_journal_hook_context.
This test covers only the case where all three values are present and subjects is a well-formed array. Four branches stay untested: an empty environment value is filtered at lines 2122-2124, an existing non-null causation_id is preserved at line 2132, a duplicate hook subject is not appended at lines 2146-2152, and a non-array subjects value returns a usage error at line 2145.
💚 Proposed additional assertions
assert_eq!(
event["subjects"],
json!([
{"kind":"workspace","id":"ws_test"},
{"kind":"hook","id":"demo_hook"},
])
);
+
+ // Empty values are ignored and existing context is preserved.
+ let mut existing = json!({"causation_id":"kept","subjects":[{"kind":"hook","id":"demo_hook"}]});
+ apply_journal_hook_context(&mut existing, Some("demo_hook"), Some("replaced"), Some(""))
+ .unwrap();
+ assert_eq!(existing["causation_id"], "kept");
+ assert!(existing.get("correlation_id").is_none());
+ assert_eq!(existing["subjects"].as_array().unwrap().len(), 1);
+
+ let mut not_an_object = json!("text");
+ assert!(
+ apply_journal_hook_context(&mut not_an_object, Some("demo_hook"), None, None).is_err()
+ );
+ let mut bad_subjects = json!({"subjects":{}});
+ assert!(
+ apply_journal_hook_context(&mut bad_subjects, Some("demo_hook"), None, None).is_err()
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn journal_append_inherits_scoped_hook_causation() { | |
| let mut event = json!({ | |
| "producer_id":"demo", | |
| "subjects":[{"kind":"workspace","id":"ws_test"}], | |
| "payload":{}, | |
| }); | |
| apply_journal_hook_context( | |
| &mut event, | |
| Some("demo_hook"), | |
| Some("event_hook_started"), | |
| Some("demo_hook:1:event_source"), | |
| ) | |
| .unwrap(); | |
| assert_eq!(event["causation_id"], "event_hook_started"); | |
| assert_eq!(event["correlation_id"], "demo_hook:1:event_source"); | |
| assert_eq!( | |
| event["subjects"], | |
| json!([ | |
| {"kind":"workspace","id":"ws_test"}, | |
| {"kind":"hook","id":"demo_hook"}, | |
| ]) | |
| ); | |
| } | |
| #[test] | |
| fn journal_append_inherits_scoped_hook_causation() { | |
| let mut event = json!({ | |
| "producer_id":"demo", | |
| "subjects":[{"kind":"workspace","id":"ws_test"}], | |
| "payload":{}, | |
| }); | |
| apply_journal_hook_context( | |
| &mut event, | |
| Some("demo_hook"), | |
| Some("event_hook_started"), | |
| Some("demo_hook:1:event_source"), | |
| ) | |
| .unwrap(); | |
| assert_eq!(event["causation_id"], "event_hook_started"); | |
| assert_eq!(event["correlation_id"], "demo_hook:1:event_source"); | |
| assert_eq!( | |
| event["subjects"], | |
| json!([ | |
| {"kind":"workspace","id":"ws_test"}, | |
| {"kind":"hook","id":"demo_hook"}, | |
| ]) | |
| ); | |
| // Empty values are ignored and existing context is preserved. | |
| let mut existing = json!({"causation_id":"kept","subjects":[{"kind":"hook","id":"demo_hook"}]}); | |
| apply_journal_hook_context(&mut existing, Some("demo_hook"), Some("replaced"), Some("")) | |
| .unwrap(); | |
| assert_eq!(existing["causation_id"], "kept"); | |
| assert!(existing.get("correlation_id").is_none()); | |
| assert_eq!(existing["subjects"].as_array().unwrap().len(), 1); | |
| let mut not_an_object = json!("text"); | |
| assert!( | |
| apply_journal_hook_context(&mut not_an_object, Some("demo_hook"), None, None).is_err() | |
| ); | |
| let mut bad_subjects = json!({"subjects":{}}); | |
| assert!( | |
| apply_journal_hook_context(&mut bad_subjects, Some("demo_hook"), None, None).is_err() | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui/src/cli/command.rs` around lines 2816 - 2839, Expand
the tests around apply_journal_hook_context to cover the four untested branches:
filter empty environment values, preserve an existing non-null causation_id,
avoid appending a duplicate hook subject, and return a usage error when subjects
is not an array. Add focused cases or assertions alongside
journal_append_inherits_scoped_hook_causation while preserving the existing
all-values-present behavior.
| let session = global.session.as_deref().unwrap_or("main"); | ||
| let error = json!({ | ||
| "code":"operation.unsupported", | ||
| "message":"resident session does not support journal subscriptions; restart it with this cmux-tui binary", | ||
| "details":{ | ||
| "capability":capability, | ||
| "session":session, | ||
| "action":"restart_session" | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not report a fabricated session name in the error details.
global.session.as_deref().unwrap_or("main") reports session: "main" when the user named no session. The user may have selected the target with --socket or CMUX_TUI_SOCKET, so the reported identifier is invented rather than supplied. Omit the field when the user supplied no session.
🛡️ Proposed fix
- let session = global.session.as_deref().unwrap_or("main");
- let error = json!({
- "code":"operation.unsupported",
- "message":"resident session does not support journal subscriptions; restart it with this cmux-tui binary",
- "details":{
- "capability":capability,
- "session":session,
- "action":"restart_session"
- },
- "retryable":false
- });
+ let mut details = json!({"capability":capability,"action":"restart_session"});
+ if let Some(session) = global.session.as_deref() {
+ details["session"] = Value::String(session.to_string());
+ }
+ let error = json!({
+ "code":"operation.unsupported",
+ "message":"resident session does not support journal subscriptions; restart it with this cmux-tui binary",
+ "details":details,
+ "retryable":false
+ });Based on learnings from the coding guidelines: "Do not expose raw upstream errors, stack traces, third-party request IDs, billing item or customer IDs, or team IDs unless the user supplied the exact ID."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let session = global.session.as_deref().unwrap_or("main"); | |
| let error = json!({ | |
| "code":"operation.unsupported", | |
| "message":"resident session does not support journal subscriptions; restart it with this cmux-tui binary", | |
| "details":{ | |
| "capability":capability, | |
| "session":session, | |
| "action":"restart_session" | |
| }, | |
| let mut details = json!({"capability":capability,"action":"restart_session"}); | |
| if let Some(session) = global.session.as_deref() { | |
| details["session"] = Value::String(session.to_string()); | |
| } | |
| let error = json!({ | |
| "code":"operation.unsupported", | |
| "message":"resident session does not support journal subscriptions; restart it with this cmux-tui binary", | |
| "details":details, | |
| "retryable":false | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui/src/cli/wire.rs` around lines 153 - 161, Update the
error construction in the journal subscription handling around global.session so
details.session is included only when the user explicitly supplied a session
name. Do not substitute the "main" fallback; preserve the capability and
restart_session details and omit the session field for socket-selected or
otherwise unnamed targets.
Source: Coding guidelines
| #[cfg(unix)] | ||
| #[test] | ||
| fn journal_subscription_rejects_a_stale_session_before_sending_the_new_envelope() { | ||
| let dir = unique_temp_dir("journal-stale-session"); | ||
| fs::create_dir_all(&dir).unwrap(); | ||
| let socket = dir.join("mux.sock"); | ||
| let listener = transport::listen(&socket).unwrap(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add the capable-server case for journal capability negotiation.
This test proves that a stale server is rejected. No test proves that a server advertising session-journal-v1 receives the journal request after the preflight. The gate in required_server_capability now covers eleven operations, so a regression that rejects a capable server, or that drops the real request after a successful preflight, would pass this suite.
Extend the fake server to answer the identify request with session-journal-v1 present, then answer the following session.journal.subscribe request, and assert that both requests arrive in order.
Do you want me to generate that test?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmux-tui/crates/cmux-tui/tests/resource_cli_v1.rs` around lines 827 - 833,
Extend the journal subscription test near
journal_subscription_rejects_a_stale_session_before_sending_the_new_envelope to
cover a capable server: have the fake server respond to identify with the
session-journal-v1 capability, then handle the subsequent
session.journal.subscribe request, and assert both requests are received in
order.
…rnal # Conflicts: # cmux-tui/bindings/cpp/.cmux-resource-api.json # cmux-tui/bindings/go/.cmux-resource-api.json # cmux-tui/bindings/java/.cmux-resource-api.json # cmux-tui/bindings/python/.cmux-resource-api.json # cmux-tui/bindings/rust/.cmux-resource-api.json # cmux-tui/bindings/rust/src/resource/mod.rs # cmux-tui/bindings/rust/tests/catalog_manifest.rs # cmux-tui/bindings/typescript/.cmux-resource-api.json # cmux-tui/bindings/zig/.cmux-resource-api.json # cmux-tui/crates/cmux-tui-core/src/workspace_registry.rs # cmux-tui/crates/cmux-tui-core/src/workspace_registry/terminal_exit_store.rs # cmux-tui/scripts/test_check_resource_api_boundary.py # cmux-tui/spec/commands.md # cmux-tui/spec/resource-api-v1.md # cmux-tui/spec/resource-operations-v1.md
…at-cmux-tui-session-journal # Conflicts: # .github/workflows/cmux-tui.yml # cmux-tui/bindings/cpp/.cmux-sdk-manifest.json # cmux-tui/bindings/cpp/include/cmux/raw/generated/models.hpp # cmux-tui/bindings/go/raw/.cmux-sdk-manifest.json # cmux-tui/bindings/go/raw/generated_commands.go # cmux-tui/bindings/go/raw/generated_events.go # cmux-tui/bindings/go/raw/generated_metadata.go # cmux-tui/bindings/go/raw/generated_presence_test.go # cmux-tui/bindings/go/raw/generated_types.go # cmux-tui/bindings/java/src/com/cmux/raw/.cmux-sdk-manifest.json # cmux-tui/bindings/java/src/com/cmux/raw/Protocol.java # cmux-tui/bindings/python/cmux/raw/_generated/.cmux-sdk-manifest.json # cmux-tui/bindings/python/cmux/raw/_generated/_schema.py # cmux-tui/bindings/python/cmux/raw/_generated/metadata.py # cmux-tui/bindings/rust-sidebar/tests/runtime.rs # cmux-tui/bindings/rust/src/client.rs # cmux-tui/bindings/rust/src/codec.rs # cmux-tui/bindings/rust/src/generated/.cmux-sdk-manifest.json # cmux-tui/bindings/rust/src/generated/commands.rs # cmux-tui/bindings/rust/src/generated/events.rs # cmux-tui/bindings/rust/src/generated/metadata.rs # cmux-tui/bindings/rust/src/generated/mod.rs # cmux-tui/bindings/rust/src/generated/types.rs # cmux-tui/bindings/rust/src/resource/client.rs # cmux-tui/bindings/rust/tests/mock_server.rs # cmux-tui/bindings/typescript/src/raw/generated/.cmux-sdk-manifest.json # cmux-tui/bindings/typescript/src/raw/generated/commands.ts # cmux-tui/bindings/typescript/src/raw/generated/events.ts # cmux-tui/bindings/typescript/src/raw/generated/index.ts # cmux-tui/bindings/typescript/src/raw/generated/metadata.ts # cmux-tui/bindings/typescript/src/raw/generated/types.ts # cmux-tui/bindings/zig/src/raw/generated/.cmux-sdk-manifest.json # cmux-tui/bindings/zig/src/raw/generated/protocol.zig # cmux-tui/crates/cmux-tui-core/src/terminal_host_protocol.rs # cmux-tui/crates/cmux-tui-core/tests/browser_runtime.rs # cmux-tui/crates/cmux-tui/src/session/mod.rs # cmux-tui/crates/cmux-tui/src/session/remote.rs
…at-cmux-tui-session-journal # Conflicts: # .github/workflows/cmux-tui.yml
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…at-cmux-tui-session-journal # Conflicts: # cmux-tui/crates/cmux-tui-core/src/terminal_host_runtime.rs
This comment has been minimized.
This comment has been minimized.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
…at-cmux-tui-session-journal
1 participant
Summary
cmux-tui-hookhelper and merge-preserving installers for Codex, Claude Code, Gemini CLI, Cursor Agent, Grok, Hermes Agent, OpenCode, Amp, and Pi.payload.native, add versioned common fields underpayload.normalized, and index stableagent_tree,agent_node, andagent_parentsubjects.cmux.protocol/1.Mechanism
(kind, id, sequence)index.stream_endafter admitted records. Overflow and cancellation retain destructive purge semantics.Performance
Final-head release probes on
cd7ce7af8487b5d7771938217ecc48e4c969d300:External helper process benchmark on journal commit
23bb4a9b5ac434f44988ebf0253739fbc175684a, whose helper code is unchanged by the base merge:Verification
--session ... --jsonl session current journal subscribepath, compiled payload filtering, durable helper receipt, native and normalized payloads, indexed agent tree subjects, and the singlecmux.protocol/1envelope.cmux-irc, and the installed helper byte-matches the final debug helper.Stacked boundary
This remains stacked on #9387. The branch contains that PR's exact current head plus current
main. PR 9387 owns durable terminal content identity and client-local placements; this PR consumes those identities for journal subjects and restoration.After PR 9387 merges, this PR should retarget to
main. The remaining product boundaries are frontend/window identity and presence, live attach versus cold replay, remote creation reconciliation, retention policy for terminal bytes, and crash expiry for frontend sizing leases.