feat: add Session retention and redacted task exports - #168
Conversation
|
@codex review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds configurable storage retention, durable readiness metadata, normalized lifecycle logs, cursor-based log following, session-aware host routing, and authenticated redacted task-artifact export. It also advances transport and persistent-service configuration contracts. ChangesUnified storage, logging, and session operations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Host
participant Storage
participant Daemon
CLI->>Host: resolve session host
Host->>Storage: load retained session and readiness data
CLI->>Daemon: request task artifacts or follow logs
Daemon->>Storage: read artifacts or logs
Storage-->>Daemon: redacted artifacts or cursor page
Daemon-->>CLI: identity-checked response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: f28497233d
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (11)
crates/satelle-cli/src/error-output.rs (1)
345-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the existing classification test to the new follow error codes.
LogsFollowIdentityChangedjoins the non-retryable identity group at line 468, but the regression test at lines 863-876 still lists only the pre-existing trust codes.LogsFollowReconnectExhaustedhas no equivalent retryable assertion either. Add both codes so a future regrouping fails a test instead of silently changing CLI retryability.♻️ Proposed test additions
ErrorCode::HostIdentityMismatch, + ErrorCode::LogsFollowIdentityChanged, ] { let contract = error_contract(code); assert_eq!(contract.category.as_str(), "remote_execution"); assert!(!contract.retryable); }And a companion assertion for the retryable side:
#[test] fn follow_reconnect_exhaustion_is_a_retryable_remote_execution_failure() { let contract = error_contract(ErrorCode::LogsFollowReconnectExhausted); assert_eq!(contract.category.as_str(), "remote_execution"); assert!(contract.retryable); }Also applies to: 462-473
🤖 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 `@crates/satelle-cli/src/error-output.rs` around lines 345 - 353, Extend the existing error classification tests to cover both follow-specific codes: add LogsFollowIdentityChanged to the non-retryable identity-group assertions, and add a retryable remote-execution assertion for LogsFollowReconnectExhausted using error_contract. Keep the existing category and retryability expectations unchanged for the pre-existing codes.crates/satelle-cli/tests/session-host-routing.rs (2)
356-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed error code on the overwrite rejection.
Line 370 asserts only
.failure(). Any failure satisfies it, including an unrelated one such as a transport error or an argument-parsing error. The test then no longer proves that the export refused to overwrite the existing output directory.Capture stderr and assert the specific error code, the same way lines 219-221 and 264-271 do for the routing cases.
💚 Proposed fix
- .assert() - .failure(); + .assert() + .failure() + .get_output() + .clone(); + // Replace with the code the export path emits for an existing output directory. + assert_eq!(parse_json_output(&overwrite.stderr)["code"], "<expected-code>");🤖 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 `@crates/satelle-cli/tests/session-host-routing.rs` around lines 356 - 371, Update the `satelle().args([...])` export assertion in the overwrite-rejection test to capture stderr and assert the specific typed error code used by the comparable routing tests at lines 219-221 and 264-271, rather than checking only `.failure()`. Preserve the existing assertion that `plan.md` remains unchanged.
210-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRecord failed
status --hostinvocations without writing the cache.
start_command_historyreturns aRecorderfor explicit-status invocations even when host resolution fails, andhistory.finish(...)records failures. A later move of history recording must keep the earlystatus --host unknownpath out ofRecorder::persist; otherwisecache_rootexists and this assertion becomes brittle or incorrect.🤖 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 `@crates/satelle-cli/tests/session-host-routing.rs` around lines 210 - 222, The failed explicit-status path for an unknown host must remain read-only. Update the status/host-resolution flow and its use of start_command_history or Recorder::persist so the early failure returns before persisting command history, while preserving the invalid-usage JSON error and existing successful invocation recording.crates/satelle-cli/src/logs.rs (2)
305-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePage handling is duplicated between the success branch and the reconnect branch.
Lines 307-314 and lines 356-363 are identical: write entries, update
last_delivered, advancequery_cursor, idle-sleep when not truncated. A future change to cursor advancement must be applied twice, and a missed second edit silently skips or replays entries.♻️ Extract a single page-consumption helper
+ let mut consume = |page: &satelle_host::DaemonLogPage, + stdout: &mut dyn Write, + last_delivered: &mut Option<LogCursor>, + query_cursor: &mut LogCursor| + -> Result<(), SatelleError> { + write_entries_to(page.entries(), None, format, stdout)?; + if let Some(entry) = page.entries().last() { + *last_delivered = Some(entry.cursor()); + } + *query_cursor = page.next_cursor(); + if !page.truncated() { + runtime.sleep(FOLLOW_IDLE_INTERVAL); + } + Ok(()) + };Then call it from both the
Ok(page)arm and the post-reconnect arm.Also applies to: 355-363
🤖 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 `@crates/satelle-cli/src/logs.rs` around lines 305 - 315, Extract the shared page-consumption logic into a helper near the surrounding log-following code, including writing entries, updating last_delivered and query_cursor, and sleeping when the page is not truncated. Replace the duplicated logic in both the connection.logs Ok(page) branch and the post-reconnect page-handling branch with calls to that helper, preserving existing error propagation and state updates.
702-736: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe SinceAll snapshot loop duplicates
visit_since_snapshot.Lines 702-736 repeat the snapshot capture, forward paging,
reached_snapshotcondition, and cursor advancement already implemented at lines 775-806. The only structural difference is the fetch source:FollowConnectionhere,TransportClientthere. The two copies can drift, and a fix to the snapshot boundary logic then applies to only one read path.Parameterize the shared loop over a page-fetch closure, so both
visit_since_snapshotand the follow initializer call one implementation.fn visit_snapshot_pages( &self, fetch: impl Fn(&LogPageQuery) -> Result<satelle_host::DaemonLogPage, SatelleError>, mut visit: impl FnMut(&[DaemonLogEntry], LogCursor) -> Result<(), SatelleError>, ) -> Result<LogCursor, SatelleError> { /* one copy of the paging loop */ }🤖 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 `@crates/satelle-cli/src/logs.rs` around lines 702 - 736, Extract the duplicated snapshot paging logic from the SinceAll branch and visit_since_snapshot into one shared visit_snapshot_pages helper. Parameterize it with a page-fetch closure and entry visitor so FollowConnection and TransportClient can provide their respective fetch implementations, while preserving snapshot capture, reached_snapshot checks, cursor advancement, entry delivery, and last-delivered tracking in the single shared loop.crates/satelle-cli/tests/command-history.rs (1)
842-848: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe count assertion does not identify which row survived.
assert_eq!(rows, 1)passes if the pruner deletes the aged row and inserts the new one. It also passes if the pruner wrongly deletes the new row and keeps the aged one. Assert the retainedstarted_atso the test names the actual retention contract.💚 Proposed fix
- let rows = fixture + let (rows, retained) = fixture .connection() - .query_row("SELECT COUNT(*) FROM command_history", [], |row| { - row.get::<_, i64>(0) - }) + .query_row( + "SELECT COUNT(*), MIN(started_at) FROM command_history", + [], + |row| Ok((row.get::<_, i64>(0)?, row.get::<_, String>(1)?)), + ) .expect("count retained command-history rows"); assert_eq!(rows, 1); + assert!( + !retained.starts_with("2000-"), + "the pruner retained the aged row instead of the new one" + );🤖 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 `@crates/satelle-cli/tests/command-history.rs` around lines 842 - 848, Strengthen the command-history pruning test by querying and asserting the retained row’s started_at value, not only the total count. Update the assertion near the rows query to verify that the newly inserted command-history row survives and the aged row is removed, while preserving the existing count check.crates/satelle-core/src/daemon-service.rs (1)
1369-1372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
--sqlite-log-retention-hoursargument too.The test checks three of the four retention arguments.
crates/satelle-cli/src/ssh-bootstrap.rsparses this plist positionally withsplit_onceon each argument prefix, so a dropped or reordered--sqlite-log-retention-hoursargument breaks managed-service observation. Lock all four here.♻️ Proposed test addition
assert!(plist.contains("<string>--session-metadata-retention-hours</string>")); + assert!(plist.contains("<string>--sqlite-log-retention-hours</string>")); assert!(plist.contains("<string>--operator-log-retained-files</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 `@crates/satelle-core/src/daemon-service.rs` around lines 1369 - 1372, Update the managed-service plist assertions in the relevant test to also verify the `<string>--sqlite-log-retention-hours</string>` argument, preserving the existing assertions for the other retention arguments and value.crates/satelle-cli/src/transport.rs (1)
3220-3224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the pass-through wrapper or use it everywhere.
resolved_persistent_storage_policyonly forwards toPersistentHostStoragePolicy::from_host_configwith the same argument and the same return type. Line 2467 calls the associated function directly, and lines 3262, 3322, and 3956 call the wrapper. Two names for one operation makes the storage-policy resolution point harder to find. Pick one.♻️ Proposed simplification
-fn resolved_persistent_storage_policy( - config: &satelle_core::HostConfig, -) -> PersistentHostStoragePolicy { - PersistentHostStoragePolicy::from_host_config(config) -}Then call
PersistentHostStoragePolicy::from_host_config(&host.config)at the three probe call sites.Also applies to: 2467-2467
🤖 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 `@crates/satelle-cli/src/transport.rs` around lines 3220 - 3224, Remove the redundant resolved_persistent_storage_policy wrapper and update its callers at the probe call sites to invoke PersistentHostStoragePolicy::from_host_config directly, matching the existing direct call pattern at line 2467.crates/satelle-core/src/lib.rs (1)
1650-1655: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the shared retention bounds independently of session metadata.
RetentionDuration::parsebounds every retention value withMIN_SESSION_METADATA_RETENTION_HOURSandMAX_SESSION_METADATA_RETENTION_HOURS, andPersistentHostStoragePolicy::validateincrates/satelle-core/src/daemon-service.rsreuses the same two constants forsqlite_log_retention_hours. GAP-051 states the two settings stay independent. The current names imply that changing the session-metadata bounds is safe when it silently also changes SQLite log retention bounds.Numerically correct today. Rename or alias to make the shared grammar explicit.
♻️ Proposed constant naming
pub const DEFAULT_SESSION_METADATA_RETENTION_HOURS: u64 = 7 * 24; pub const DEFAULT_SQLITE_LOG_RETENTION_HOURS: u64 = 7 * 24; -pub const MIN_SESSION_METADATA_RETENTION_HOURS: u64 = 7 * 24; -pub const MAX_SESSION_METADATA_RETENTION_HOURS: u64 = 365 * 24; +/// Shared grammar bounds for every destructive `RetentionDuration` value. +pub const MIN_RETENTION_HOURS: u64 = 7 * 24; +pub const MAX_RETENTION_HOURS: u64 = 365 * 24;Also applies to: 1665-1691
🤖 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 `@crates/satelle-core/src/lib.rs` around lines 1650 - 1655, Rename or introduce shared retention-bound constants that do not imply ownership by session metadata, then update RetentionDuration::parse and PersistentHostStoragePolicy::validate to use those shared names for both session metadata and SQLite log retention. Preserve the current numeric bounds and independently retain the setting-specific default constants.crates/satelle-host/src/storage.rs (1)
2979-3033: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated readiness-log insertion block.
begin_session(lines 3002-3025) andbegin_follow_up(lines 3138-3161) contain the identical sequence: insert the native readiness summary log, then conditionally insert the provider smoke summary log whenreadiness.provider_result_id()is present. The two blocks are copy-pasted rather than shared.Duplicated transactional logging logic across two admission entry points is a maintenance risk: a future change to the readiness-log contract (for example, adding a field or changing the conditional) must be applied in both places, and it is easy to update one and miss the other.
Extract a shared helper, for example
fn insert_readiness_logs(transaction, session, turn_id, readiness) -> Result<(), StorageError>, and call it from bothbegin_sessionandbegin_follow_up.♻️ Proposed refactor sketch
+fn insert_readiness_logs( + transaction: &Transaction<'_>, + session: &Session, + turn_id: &TurnId, + readiness: &AdmissionReadinessRef, +) -> Result<(), StorageError> { + insert_safe_log( + transaction, + &canonical_log( + LogEvent::NativeReadinessSummary, + LogSeverity::Info, + session, + turn_id, + session.updated_at(), + )?, + )?; + if readiness.provider_result_id().is_some() { + insert_safe_log( + transaction, + &canonical_log( + LogEvent::ProviderSmokeSummary, + LogSeverity::Info, + session, + turn_id, + session.updated_at(), + )?, + )?; + } + Ok(()) +}Then in
begin_sessionandbegin_follow_up:- if let Some(readiness) = &context.readiness_ref { - insert_safe_log( - &transaction, - &canonical_log( - LogEvent::NativeReadinessSummary, - LogSeverity::Info, - session, - &turn_id, - session.updated_at(), - )?, - )?; - if readiness.provider_result_id().is_some() { - insert_safe_log( - &transaction, - &canonical_log( - LogEvent::ProviderSmokeSummary, - LogSeverity::Info, - session, - &turn_id, - session.updated_at(), - )?, - )?; - } - } + if let Some(readiness) = &context.readiness_ref { + insert_readiness_logs(&transaction, session, &turn_id, readiness)?; + }Also applies to: 3115-3169
🤖 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 `@crates/satelle-host/src/storage.rs` around lines 2979 - 3033, Extract the duplicated readiness-log insertion sequence into a shared helper such as insert_readiness_logs, accepting the transaction, session, turn_id, and readiness and preserving both the native summary insertion and provider smoke summary conditional. Replace the inline blocks in begin_session and begin_follow_up with calls to this helper, propagating its Result without changing transactional behavior.crates/satelle-host/src/runtime.rs (1)
2179-2298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the repeated
expect("writing to a String cannot fail").The plan builder repeats the same
expectstring about thirty times. The noise hides the actual field list. A small local macro keeps the same panic-free guarantee and shortens each line.♻️ Proposed helper
+macro_rules! artifact_line { + ($buffer:expr, $($argument:tt)*) => { + writeln!($buffer, $($argument)*).expect("writing to a String cannot fail") + }; +}Then each site becomes
artifact_line!(plan, "- Turn ID: {}", turn.id());.🤖 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 `@crates/satelle-host/src/runtime.rs` around lines 2179 - 2298, Introduce a small local helper macro near the start of the plan-building block to wrap writeln! calls with the existing expect("writing to a String cannot fail") guarantee, then replace each repeated writeln!/expect pair in the plan construction, including the turn loop and readiness branches, with the helper while preserving all output and error propagation behavior.
🤖 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 `@crates/satelle-cli/src/logs.rs`:
- Around line 213-240: Update ProcessFollowRuntime::new so tokio runtime build
and interrupt-thread spawn failures use an internal-category SatelleError
constructor instead of invalid_usage. Add a suitable constructor if needed,
ensuring error_contract maps both failures to the internal exit class and
recovery guidance while preserving the existing error context.
- Around line 856-870: Update unresolved_log_target and the resolve_session_host
flow so only the no-candidate/no-target condition is converted to
logs-target-required. Introduce or propagate a dedicated error code or explicit
flag for that condition, and preserve the original invalid-usage error for
ambiguous configured hosts so its precise diagnostic reaches callers; do not
distinguish cases by matching error strings.
- Around line 507-516: Update transient_follow_error to remove
ErrorCode::SshBootstrapUnavailable from the transient/retryable matches, while
preserving the existing handling for the remaining transport errors so SSH
bootstrap configuration failures surface immediately.
- Around line 408-420: Update the reconnect loop around run_reconnect_attempt to
capture runtime.now() immediately after the deadline guard succeeds, then
subtract that saved instant from deadline when calculating the attempt timeout.
Keep the existing exhausted() returns and ensure the saved value is reused
consistently for this timeout calculation.
In `@crates/satelle-cli/src/main.rs`:
- Around line 11036-11109: Update persist_task_artifacts so all
filesystem-related failure mappings—canonicalize, try_exists, parent ownership,
staging creation, artifact write/flush/sync, directory sync, and publish—use the
storage or configuration error class required by the CLI contract instead of
SatelleError::invalid_usage. Preserve invalid_usage only for the missing output
file name and already-existing destination cases.
In `@crates/satelle-cli/src/ssh-bootstrap.rs`:
- Around line 5939-5945: Update all three PersistentHostStoragePolicy::new call
sites in the affected fixtures, including the one near the shown diff and the
two additional occurrences, so their third argument uses
DEFAULT_SQLITE_LOG_RETENTION_HOURS instead of
DEFAULT_SESSION_METADATA_RETENTION_HOURS. Keep the other policy arguments
unchanged.
In `@crates/satelle-cli/tests/cli.rs`:
- Around line 2340-2362: Restore SATELLE_CACHE_DIR in the SIGINT follow test’s
spawned command alongside SATELLE_STATE_DIR, using the test’s temporary cache
directory. Update the command setup around the existing environment removal and
env assignments, matching the sibling test’s established cache-directory value
so session resolution and command-history writes remain isolated.
In `@crates/satelle-host/src/lib.rs`:
- Around line 2756-2768: Update production_for_service so failures from
ExplicitDuration::parse and RetentionDuration::parse for setup ledger, session
metadata, and SQLite log retention return a configuration error instead of
assigning None. Preserve successful parsed values and ensure
RuntimeStoragePolicy::from_host_config is not reached with rejected retention
settings.
In `@crates/satelle-host/src/runtime.rs`:
- Around line 2171-2374: Update task_artifacts so the storage mutex is not held
across the entire export: remove the function-wide storage guard, acquire and
release a guard within each turn iteration for recovery_subject, and acquire and
release another guard for each log_page pagination iteration. Preserve the
existing session value, cursor handling, error mapping, and artifact output
while ensuring each query batch completes before the next lock acquisition.
- Around line 2196-2205: Remove upstream_goal_ref from the task artifact export
flow around the goal.md writing logic, including the value accumulated by
goal_ref. Replace the private identifier with the permitted redacted token such
as “not recorded,” while preserving the surrounding artifact generation
behavior.
In `@crates/satelle-host/src/storage/open.rs`:
- Around line 136-148: Update the migration 16 entry for
0016_normalized_log_events.sql to set irreversible to true, ensuring existing
log data receives a pre-migration backup through apply_migrations and
create_migration_backup().
In `@crates/satelle-host/src/storage/tests/logs.rs`:
- Around line 92-99: Update the operator-log formatter for the expected
store-opened record to emit the required redacted=true marker, then update the
corresponding expected line in the logs test while preserving the existing
formatting and field order.
- Around line 670-698: Strengthen
failed_turns_persist_a_normalized_structured_error_without_raw_payloads by
asserting the persisted structured error is marked redacted, and add a private
execution-error canary through the available failure fixture or setup. Verify
the serialized log excludes that canary, preserving the existing source and
severity assertions and ensuring raw failure data cannot pass unnoticed.
In `@crates/satelle-host/src/storage/tests/operational.rs`:
- Around line 434-480: Update
version_fifteen_logs_upgrade_without_cursor_or_row_loss to construct the
version-fifteen logs schema and data explicitly, following the existing
version-eleven fixture pattern, before reopening the database. Avoid creating
the fixture through the current Storage::open schema and merely changing
migration metadata; reopen the manually built v15 database so
0016_normalized_log_events.sql performs the real logs-to-logs_v15 upgrade. Keep
the cursor-preservation and post-migration append assertions unchanged.
In `@crates/satelle-host/src/storage/tests/security.rs`:
- Line 219: Update the privacy-sensitive table assertions in the security test
to add an assert_table_columns call for turn_admission_readiness, using the
exact column names defined by the 0015_turn_admission_readiness migration:
turn_id, native_result_id, native_observed_at, native_source,
provider_result_id, provider_observed_at, and provider_source.
In `@crates/satelle-transport/src/server/sessions.rs`:
- Around line 210-216: Update the artifact-producing flow used by
get_task_artifacts so upstream Goal references are redacted or replaced before
the value reaches TaskArtifactsResponse; do not serialize the raw
artifacts.goal() content. Extend the Host artifact HTTP test to assert the
response body excludes the upstream-reference canary.
In `@docs/explanation/security-boundaries.mdx`:
- Around line 77-80: Update the storage-boundary statement to identify
owner-only, redacted task-artifact export as the sole MVP storage egress,
limited to plan.md, worklog.md, and goal.md. Remove references to exporting
diagnostics or recordings, while preserving the existing statement about local
host storage and unsupported platform logging sinks.
In `@docs/reference/configuration.mdx`:
- Line 90: Update the retention precedence statement near “Only a user-selected
profile can change destructive retention” to clarify that user Host
configuration establishes the baseline, a user-selected profile may override
Host values, and project configuration cannot set destructive retention.
---
Nitpick comments:
In `@crates/satelle-cli/src/error-output.rs`:
- Around line 345-353: Extend the existing error classification tests to cover
both follow-specific codes: add LogsFollowIdentityChanged to the non-retryable
identity-group assertions, and add a retryable remote-execution assertion for
LogsFollowReconnectExhausted using error_contract. Keep the existing category
and retryability expectations unchanged for the pre-existing codes.
In `@crates/satelle-cli/src/logs.rs`:
- Around line 305-315: Extract the shared page-consumption logic into a helper
near the surrounding log-following code, including writing entries, updating
last_delivered and query_cursor, and sleeping when the page is not truncated.
Replace the duplicated logic in both the connection.logs Ok(page) branch and the
post-reconnect page-handling branch with calls to that helper, preserving
existing error propagation and state updates.
- Around line 702-736: Extract the duplicated snapshot paging logic from the
SinceAll branch and visit_since_snapshot into one shared visit_snapshot_pages
helper. Parameterize it with a page-fetch closure and entry visitor so
FollowConnection and TransportClient can provide their respective fetch
implementations, while preserving snapshot capture, reached_snapshot checks,
cursor advancement, entry delivery, and last-delivered tracking in the single
shared loop.
In `@crates/satelle-cli/src/transport.rs`:
- Around line 3220-3224: Remove the redundant resolved_persistent_storage_policy
wrapper and update its callers at the probe call sites to invoke
PersistentHostStoragePolicy::from_host_config directly, matching the existing
direct call pattern at line 2467.
In `@crates/satelle-cli/tests/command-history.rs`:
- Around line 842-848: Strengthen the command-history pruning test by querying
and asserting the retained row’s started_at value, not only the total count.
Update the assertion near the rows query to verify that the newly inserted
command-history row survives and the aged row is removed, while preserving the
existing count check.
In `@crates/satelle-cli/tests/session-host-routing.rs`:
- Around line 356-371: Update the `satelle().args([...])` export assertion in
the overwrite-rejection test to capture stderr and assert the specific typed
error code used by the comparable routing tests at lines 219-221 and 264-271,
rather than checking only `.failure()`. Preserve the existing assertion that
`plan.md` remains unchanged.
- Around line 210-222: The failed explicit-status path for an unknown host must
remain read-only. Update the status/host-resolution flow and its use of
start_command_history or Recorder::persist so the early failure returns before
persisting command history, while preserving the invalid-usage JSON error and
existing successful invocation recording.
In `@crates/satelle-core/src/daemon-service.rs`:
- Around line 1369-1372: Update the managed-service plist assertions in the
relevant test to also verify the `<string>--sqlite-log-retention-hours</string>`
argument, preserving the existing assertions for the other retention arguments
and value.
In `@crates/satelle-core/src/lib.rs`:
- Around line 1650-1655: Rename or introduce shared retention-bound constants
that do not imply ownership by session metadata, then update
RetentionDuration::parse and PersistentHostStoragePolicy::validate to use those
shared names for both session metadata and SQLite log retention. Preserve the
current numeric bounds and independently retain the setting-specific default
constants.
In `@crates/satelle-host/src/runtime.rs`:
- Around line 2179-2298: Introduce a small local helper macro near the start of
the plan-building block to wrap writeln! calls with the existing expect("writing
to a String cannot fail") guarantee, then replace each repeated writeln!/expect
pair in the plan construction, including the turn loop and readiness branches,
with the helper while preserving all output and error propagation behavior.
In `@crates/satelle-host/src/storage.rs`:
- Around line 2979-3033: Extract the duplicated readiness-log insertion sequence
into a shared helper such as insert_readiness_logs, accepting the transaction,
session, turn_id, and readiness and preserving both the native summary insertion
and provider smoke summary conditional. Replace the inline blocks in
begin_session and begin_follow_up with calls to this helper, propagating its
Result without changing transactional behavior.
🪄 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: CHILL
Plan: Pro Plus
Run ID: d31916a5-99bc-4498-b35c-d3c1c47030b5
📒 Files selected for processing (62)
.facts.spec-gaps.mdcrates/satelle-cli/src/command-history.rscrates/satelle-cli/src/error-output.rscrates/satelle-cli/src/logs.rscrates/satelle-cli/src/main.rscrates/satelle-cli/src/mcp/arguments.rscrates/satelle-cli/src/output.rscrates/satelle-cli/src/read.rscrates/satelle-cli/src/ssh-bootstrap.rscrates/satelle-cli/src/tailscale-serve.rscrates/satelle-cli/src/tailscale.rscrates/satelle-cli/src/transport-tests.rscrates/satelle-cli/src/transport.rscrates/satelle-cli/tests/cli.rscrates/satelle-cli/tests/command-history.rscrates/satelle-cli/tests/report-schema-contract.rscrates/satelle-cli/tests/session-host-routing.rscrates/satelle-core/src/daemon-service.rscrates/satelle-core/src/lib.rscrates/satelle-core/src/profiles.rscrates/satelle-core/src/secure-file.rscrates/satelle-core/src/session.rscrates/satelle-host/src/daemon-reconnect-tests.rscrates/satelle-host/src/daemon.rscrates/satelle-host/src/lib-tests.rscrates/satelle-host/src/lib.rscrates/satelle-host/src/log-page.rscrates/satelle-host/src/runtime-model.rscrates/satelle-host/src/runtime-tests.rscrates/satelle-host/src/runtime-tests/review-regressions.rscrates/satelle-host/src/runtime.rscrates/satelle-host/src/storage.rscrates/satelle-host/src/storage/0015_turn_admission_readiness.sqlcrates/satelle-host/src/storage/0016_normalized_log_events.sqlcrates/satelle-host/src/storage/codec.rscrates/satelle-host/src/storage/logs.rscrates/satelle-host/src/storage/open.rscrates/satelle-host/src/storage/operator-log.rscrates/satelle-host/src/storage/retention.rscrates/satelle-host/src/storage/sql.rscrates/satelle-host/src/storage/stop.rscrates/satelle-host/src/storage/tests/lifecycle.rscrates/satelle-host/src/storage/tests/logs.rscrates/satelle-host/src/storage/tests/operational.rscrates/satelle-host/src/storage/tests/retention.rscrates/satelle-host/src/storage/tests/security.rscrates/satelle-transport/src/client.rscrates/satelle-transport/src/contract.rscrates/satelle-transport/src/contract/session.rscrates/satelle-transport/src/lib.rscrates/satelle-transport/src/server/host_error.rscrates/satelle-transport/src/server/mod.rscrates/satelle-transport/src/server/sessions.rscrates/satelle-transport/tests/http.rscrates/satelle-transport/tests/http/protocol.rscrates/satelle-transport/tests/http/provider-auth.rscrates/satelle-transport/tests/http/raw-wire.rscrates/satelle-transport/tests/http/sessions.rscrates/satelle-transport/tests/http/setup-readiness.rsdocs/explanation/security-boundaries.mdxdocs/reference/configuration.mdx
|
@greptileai review |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 294746a106
ℹ️ 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".
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24c23733e7
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/explanation/security-boundaries.mdx`:
- Line 78: Update the artifact list in the security-boundaries documentation to
use “goal projection” instead of “Goal projection,” keeping the existing
lowercase naming style for the other exported artifacts.
🪄 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: CHILL
Plan: Pro Plus
Run ID: fdf162bc-d84c-4a0c-b1bd-dd33e43970c5
📒 Files selected for processing (22)
crates/satelle-cli/src/command-history.rscrates/satelle-cli/src/logs.rscrates/satelle-cli/src/main.rscrates/satelle-cli/src/ssh-bootstrap.rscrates/satelle-cli/src/transport-tests.rscrates/satelle-cli/src/transport.rscrates/satelle-cli/tests/cli.rscrates/satelle-cli/tests/report-schema-contract.rscrates/satelle-cli/tests/session-host-routing.rscrates/satelle-host/src/lib.rscrates/satelle-host/src/runtime-tests.rscrates/satelle-host/src/runtime.rscrates/satelle-host/src/storage/open.rscrates/satelle-host/src/storage/operator-log.rscrates/satelle-host/src/storage/retention.rscrates/satelle-host/src/storage/tests/logs.rscrates/satelle-host/src/storage/tests/operational.rscrates/satelle-host/src/storage/tests/retention.rscrates/satelle-host/src/storage/tests/security.rsdocs/explanation/security-boundaries.mdxdocs/reference/configuration.mdxdocs/reference/generated-cli.mdx
🚧 Files skipped from review as they are similar to previous changes (16)
- crates/satelle-cli/tests/report-schema-contract.rs
- crates/satelle-host/src/storage/tests/logs.rs
- docs/reference/configuration.mdx
- crates/satelle-host/src/storage/open.rs
- crates/satelle-cli/tests/session-host-routing.rs
- crates/satelle-host/src/runtime-tests.rs
- crates/satelle-cli/src/transport-tests.rs
- crates/satelle-host/src/storage/retention.rs
- crates/satelle-host/src/storage/operator-log.rs
- crates/satelle-cli/tests/cli.rs
- crates/satelle-cli/src/command-history.rs
- crates/satelle-host/src/lib.rs
- crates/satelle-cli/src/transport.rs
- crates/satelle-cli/src/ssh-bootstrap.rs
- crates/satelle-host/src/runtime.rs
- crates/satelle-cli/src/main.rs
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3fe5f0306
ℹ️ 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".
|
@greptileai review |
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abc5559656
ℹ️ 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".
|
@coderabbitai review |
|
@codex review |
|
@greptileai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfc216e440
ℹ️ 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".
bfc216e to
d2b832d
Compare
Microck
changed the title
|
@coderabbitai review |
|
@greptileai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2b832d80c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 705ba47158
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef6043c076
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46b2f34c4a
ℹ️ 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".
|
@coderabbitai review |
|
@codex review |
|
@greptileai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Train
01d5017c36268dc9d963f13a3b2f50c4a5d8325c507ac72ab892f191ef8d2ae22788ac8461a3f6b97f02a413d373959836359780503c6d24ba292dc1b79aadee9b62cbbb1d8d34ac0272ffa4213425edd2b832d80c93b16fe5ff158cdc7ca8fae511c34e705ba47158b57d1a7fd50b87a439b935f12a1cc0ef6043c076e92a972f1ff95dee2ea5a5edd734fd46b2f34c4a412521cc5b4284ad0a39a1b83941ec507ac72ab892f191ef8d2ae22788ac8461a3f6b98f1d93845806521953acef3b4577995113eafae1f954b4443e108c8574a61702What this ships
plan.md,worklog.md, andgoal.mdProof
@implemented--hostfallbackfacts lint, formatting, docs contract, diff checks, focused proof, and strict affected ClippyWhy this PR changed shape
The original T8 candidate combined packets 22 and 23. Repeated review remediation showed that boundary was too large for the bounded train lifecycle. This PR now contains packet 22 only. The full pre-split head remains preserved, and packet 23 will land as a dependent PR with its original source range intact.
Out of scope
satelle logs --followSummary by CodeRabbit
session exportfor securely retrieving redacted plans, goals, and worklogs.Greptile Summary
The PR adds Host-owned Session retention, non-authoritative CLI Session routing, and owner-only redacted task-artifact export.
plan.md,worklog.md, andgoal.mdexports.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant CLI participant Cache as Session-to-Host Cache participant Host as Host Daemon participant DB as Host SQLite participant FS as Owner-only Export Directory User->>CLI: session export SESSION --output PATH CLI->>Cache: Resolve one candidate Host Cache-->>CLI: Host candidate or explicit-host requirement CLI->>Host: Read task artifacts for Session Host->>DB: Load authoritative Session snapshot Host->>DB: Read redacted logs using one retention observation DB-->>Host: Coherent artifact source data Host-->>CLI: Redacted plan, worklog, and goal CLI->>FS: Stage owner-only files CLI->>FS: Publish directory without replacement FS-->>User: Exported artifact pathsReviews (12): Last reviewed commit: "fix: keep artifact log retention boundar..." | Re-trigger Greptile
Context used (5)