Repository navigation
feat(client): user policy callbacks - #847
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds interactive and non-interactive activation policy hooks, persists updated policies, uploads workspace files before loadout configuration, introduces typed package filesystem mappings, propagates variable provenance through composition, and adds pending-session and patch-validation safeguards. ChangesActivation and composition
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/minimald/src/sessions.rs`:
- Around line 288-291: Correct the create-flow documentation around
create_session to state that it only allocates the session record and spawns the
Draft actor. Remove claims that CreateSession performs inline configure_loadout,
composes the loadout, or finalizes it; document that cmd_activate uploads files
and sends a separate ConfigureLoadout RPC afterward.
In `@crates/sessions/docs/COMPOSITION.md`:
- Around line 410-424: Update the documentation describing
ResolvedVar::carries_user_data to include successful InheritWithDefault lookups
alongside successful Inherit lookups, while keeping fallback behavior and the
existing policy-gating explanation accurate.
- Around line 223-234: Update the “Takes the matching stash entry” documentation
in the resume_from_verdict flow to state that Session::handle_verdict retains
the cloned pending stash when resumption fails, allowing the client to correct
and resubmit; describe stash removal only after successful finalization.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dce03fd-408e-4bf9-bc23-b8999bedcd5b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
Cargo.tomlcrates/mfile/src/lib.rscrates/mfile/src/package_composable.rscrates/minimal/Cargo.tomlcrates/minimal/src/config.rscrates/minimal/src/lib.rscrates/minimal/src/loadouts.rscrates/minimal/src/prompt.rscrates/minimal/tests/cli.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rscrates/minimald/src/sessions/composables.rscrates/minimald/src/test_harness.rscrates/paths/src/lib.rscrates/sessions/docs/COMPOSITION.mdcrates/sessions/example_project/minimal.tomlcrates/sessions/src/client/composer.rscrates/sessions/src/client/handler.rscrates/sessions/src/core/compose.rscrates/sessions/src/core/decision.rscrates/sessions/src/core/hooks.rscrates/sessions/src/core/loadout.rscrates/sessions/src/core/primitives.rscrates/sessions/src/daemon/composer.rscrates/sessions/src/wire/policy.rscrates/sessions/src/wire/primitives.rscrates/sessions/src/wire/request.rscrates/sessions/tests/client_flow1.rscrates/sessions/tests/client_flow2.rs
There was a problem hiding this comment.
I just realized I never got you an RPC for uploading to the homedir: the WorkspaceFilesTarZst unpacks to what gets mapped to /workbench and not /home. Do we need an RPC for uploading to the home dir?
I'm just planning out that work now. I can just add it as part of that change. |
|
I did have claude run an ultraview as it is pretty big. It came back very very clean but claude did raise one thing:
I leave it at your discretion if this is something to fix right now! |
I think I'll defer these as I think there are going to be some related changes in the next PR. |
be44d68 to
ad8b414
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/sessions/src/daemon/composer.rs (1)
284-296: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd duplicate-var verdict regression coverage.
Add a test with two verdict entries for one
PendingId—including mixed variants—to ensure this remains anInvalidWireItemrather than regressing into an unknown-id or denial path.🤖 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/sessions/src/daemon/composer.rs` around lines 284 - 296, Add regression coverage for the verdict validation around the duplicate-detection loop, constructing two var verdicts with the same PendingId using different WireVarVerdict variants. Assert composition returns ComposeError::InvalidWireItem and does not classify the input as an unknown-id or denial error.
🤖 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/minimald/src/session.rs`:
- Around line 541-546: Update the session operation around the
SessionInner::Active match branch to distinguish state-rejection failures from
compose failures: an AlreadyExists error must preserve the Active state, while
compose-related failures must continue leaving the session in Draft as
previously guaranteed. Revise the surrounding failure-state handling or
documentation to state these separate guarantees accurately.
In `@crates/sessions/src/daemon/composer.rs`:
- Around line 368-392: Update the nearby documentation for
PendingComposeState::pending_patches to remove the outdated claim that it
supplies an omitted destination. Describe that the map retains source provenance
and validates patch IDs used by WirePatchVerdict::Approved, while destination
now comes from the verdict.
---
Nitpick comments:
In `@crates/sessions/src/daemon/composer.rs`:
- Around line 284-296: Add regression coverage for the verdict validation around
the duplicate-detection loop, constructing two var verdicts with the same
PendingId using different WireVarVerdict variants. Assert composition returns
ComposeError::InvalidWireItem and does not classify the input as an unknown-id
or denial error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90e47219-b396-48ab-a288-ae918e3e5c72
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
Cargo.tomlcrates/mfile/src/lib.rscrates/mfile/src/package_composable.rscrates/minimal/Cargo.tomlcrates/minimal/src/config.rscrates/minimal/src/lib.rscrates/minimal/src/loadouts.rscrates/minimal/src/prompt.rscrates/minimal/tests/cli.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rscrates/minimald/src/sessions/composables.rscrates/minimald/src/test_harness.rscrates/paths/src/lib.rscrates/sessions/docs/COMPOSITION.mdcrates/sessions/example_project/minimal.tomlcrates/sessions/src/client/composer.rscrates/sessions/src/client/handler.rscrates/sessions/src/core/compose.rscrates/sessions/src/core/decision.rscrates/sessions/src/core/hooks.rscrates/sessions/src/core/loadout.rscrates/sessions/src/core/primitives.rscrates/sessions/src/daemon/composer.rscrates/sessions/src/wire/policy.rscrates/sessions/src/wire/primitives.rscrates/sessions/src/wire/request.rscrates/sessions/tests/client_flow1.rscrates/sessions/tests/client_flow2.rs
🚧 Files skipped from review as they are similar to previous changes (28)
- crates/sessions/src/core/decision.rs
- crates/sessions/src/core/hooks.rs
- crates/minimal/src/config.rs
- crates/minimal/src/loadouts.rs
- crates/sessions/src/wire/policy.rs
- Cargo.toml
- crates/minimald/src/test_harness.rs
- crates/minimald/src/rpc.rs
- crates/minimal/tests/cli.rs
- crates/minimald-rpc/src/lib.rs
- crates/minimal/Cargo.toml
- crates/mfile/src/lib.rs
- crates/sessions/example_project/minimal.toml
- crates/sessions/src/core/loadout.rs
- crates/sessions/src/core/primitives.rs
- crates/paths/src/lib.rs
- crates/sessions/src/client/composer.rs
- crates/sessions/src/wire/primitives.rs
- crates/minimald/src/sessions/composables.rs
- crates/sessions/src/core/compose.rs
- crates/sessions/tests/client_flow1.rs
- crates/minimal/src/prompt.rs
- crates/mfile/src/package_composable.rs
- crates/sessions/docs/COMPOSITION.md
- crates/minimald/src/sessions.rs
- crates/sessions/src/client/handler.rs
- crates/minimal/src/lib.rs
- crates/sessions/tests/client_flow2.rs
| match &self.inner { | ||
| SessionInner::Active { .. } => { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::AlreadyExists, | ||
| "session loadout is already configured", | ||
| )); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the failure-state guarantee.
This new AlreadyExists path leaves the session Active, so the preceding claim that every failure leaves it Draft is no longer true. Distinguish compose failures from state-rejection failures.
🤖 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/minimald/src/session.rs` around lines 541 - 546, Update the session
operation around the SessionInner::Active match branch to distinguish
state-rejection failures from compose failures: an AlreadyExists error must
preserve the Active state, while compose-related failures must continue leaving
the session in Draft as previously guaranteed. Revise the surrounding
failure-state handling or documentation to state these separate guarantees
accurately.
| WirePatchVerdict::Approved { | ||
| id, | ||
| host_path, | ||
| destination, | ||
| } => { | ||
| let pp = lookup_patch(pending_patches, id)?; | ||
| touched_ids.insert(id); | ||
| let destination = pp.patch().dest().as_sandbox_path().clone(); | ||
| // Use the client-computed `destination`, not | ||
| // `pp.patch().dest()`. The pending's dest is the | ||
| // base (e.g. `~/.claude` for a `~/.claude/**` dir | ||
| // mapping) — reusing it would collapse every file's | ||
| // dest to that base and trigger the composer's | ||
| // patch-destination-conflict check on the second | ||
| // file. See the wire-side rationale on | ||
| // `WirePatchVerdict::Approved::destination`. | ||
| // | ||
| // Safe against sandbox-escape via a `..` traversal: | ||
| // `destination` deserialized through | ||
| // `SandboxRelPath::try_new`, which rejects both | ||
| // absolute paths and any `..` component. A malicious | ||
| // client crafting `"../../etc/foo"` never gets past | ||
| // wire parsing. | ||
| let source = pp.source().clone(); | ||
| accepted.push(SessionPatch::new( | ||
| ResolvedPatch::new(host_path, destination), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the pending-patch provenance documentation.
WirePatchVerdict::Approved now carries destination, so PendingComposeState::pending_patches no longer supplies a destination omitted by the verdict. Update the nearby field comment to describe its remaining source-provenance and ID-validation role.
🤖 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/sessions/src/daemon/composer.rs` around lines 368 - 392, Update the
nearby documentation for PendingComposeState::pending_patches to remove the
outdated claim that it supplies an omitted destination. Describe that the map
retains source provenance and validates patch IDs used by
WirePatchVerdict::Approved, while destination now comes from the verdict.
ad8b414 to
c99fcd8
Compare
The client's activate flow now prompts the operator (or emits a paste-ready user_policy.toml snippet in --no-prompt/non-TTY mode) whenever Phase 3 hits a project- or package-declared var/patch the user's UserPolicy can't auto-decide, instead of aborting the composition. This closes the loop the callbacks feature has been building toward: daemon-side project/package composables can now contribute vars and patches without the operator having to hand-craft their user_policy.toml up front.
What's in
Other correctness / UX fixes
Docs
crates/sessions/docs/COMPOSITION.md rewritten to match the current shape: three-RPC flow, carries_user_data short-circuit as a first-class invariant, PackageFsMapping file/dir distinction, NoPromptHook's fake-approve+summary-check pattern, and the daemon's use of SourceProvider for upstream layers (workspace tar only carries the mfile itself in the common case).
Summary by CodeRabbit
--no-promptfor non-interactive activation; if approvals are still required, it aborts with a paste-readyuser_policy.tomlsnippet.Draftpending contributions from being overwritten by subsequent loadout configuration.minimal.tomlto reflect the new activation/prompt flow.