SemianiakaVY/openpencil
1
0
Fork 0
Code Issues Pull requests Actions 1 Packages Projects Releases Wiki Activity

fix(web): make the identity reset claims real and cover /mcp in the write barrier

Browse source 
Re-audit round: an aborted edit script had silently dropped half a
prior fix, so each item here is grep-verified in the report trail.

- identity reset now actually drains the credential sync queue and
  epoch-tags in-flight callbacks so account A's completing upload
  cannot become account B's state; an empty partition resets every
  account-scoped setting (locale, recents, agent settings, theme)
  instead of inheriting the previous account's; rebuilt persistence
  baselines go through the CredentialLoad seams so settings keep
  saving and unsupported snapshots stay fail-closed after a switch
- mutation admission is one helper shared by the REST push and /mcp:
  write tools take a WritePass (read tools stay free) and refuse with
  a proper tools/call error once shutdown closes the barrier; the
  write drain escalates to a 60s hard cap with blocked-writer logging
  instead of flushing past active writers at 10s
- a runtime-rejected ingest restores the thumbnail registry snapshot
  captured before install (vendor/jian gains the restore_snapshot
  counterpart), so a rejected document's thumbnails no longer stay
  active locally
This commit is contained in:
Kayshen-X 2026-08-08 21:35:27 +08:00
parent 62d944710b
commit f3ea6e8c99
14 changed files with 497 additions and 75 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
crates/op-host-services/src/mcp_serve/tool_profile.rs
View file

@ -262,6 +262,16 @@ impl Default for McpAccessProfile {
}
}
/// Whether calling `name` will mutate the document.
///
/// Used by the connection tier to decide, BEFORE dispatch, whether a `/mcp`
/// call needs a shutdown write pass. Unclassified names default to `Write`
/// (see [`access_of`]), which is the safe direction here too: an unknown tool
/// is admitted through the barrier rather than slipping past it.
pub fn tool_writes(name: &str) -> bool {
matches!(access_of(name), ToolAccess::Write)
}
/// The classification for `name`, if it is in the static catalog.
pub fn profile_for(name: &str) -> Option<&'static ToolProfile> {
TOOL_PROFILES.iter().find(|profile| profile.name == name)

19
crates/op-host-services/src/web_canvas_server/collab_state.rs
View file

@ -349,6 +349,13 @@ impl WebCanvasState {
jian_ops_schema::image_thumbs::discard_for_document(prepared.document());
return IngestOutcome::Rejected;
}
// Captured before the install activates the incoming document's
// thumbnails. If the session then REJECTS the edit, rolling the
// document back does not roll these back: the previous document's
// pending seed was consumed by its own activation, so re-activating it
// is a no-op and the refused document's thumbnails keep resolving live
// ids. Restoring the snapshot on the rejection path is what undoes it.
let thumbnails_before = jian_ops_schema::image_thumbs::capture_snapshot();
// The capture is now open and MUST be closed on every path. The guard
// is what makes that true even if the install below panics: an
// unclosed capture leaves the session permanently unable to accept
@ -365,8 +372,16 @@ impl WebCanvasState {
match capture.finish() {
op_collab_host::LocalEditOutcome::Committed => IngestOutcome::Committed,
op_collab_host::LocalEditOutcome::NoChange => IngestOutcome::NoChange,
op_collab_host::LocalEditOutcome::Rejected => IngestOutcome::Rejected,
op_collab_host::LocalEditOutcome::Failed => IngestOutcome::Failed,
// The document was rolled back, so the thumbnails must roll back
// with it — otherwise the refused document's images stay active.
op_collab_host::LocalEditOutcome::Rejected => {
jian_ops_schema::image_thumbs::restore_snapshot(thumbnails_before);
IngestOutcome::Rejected
}
op_collab_host::LocalEditOutcome::Failed => {
jian_ops_schema::image_thumbs::restore_snapshot(thumbnails_before);
IngestOutcome::Failed
}
}
}
}

48
crates/op-host-services/src/web_canvas_server/collab_state_tests.rs
View file

@ -378,3 +378,51 @@ fn a_gated_push_still_owns_its_document_so_the_seed_is_released() {
let refused = state.apply_prepared_document_push(push, None);
assert!(refused.is_err(), "a viewer's push must be refused");
}
#[test]
fn a_rejected_ingest_rolls_the_thumbnail_registry_back_too() {
// The document rolls back on a rejection, but the thumbnails did not: the
// previous document's pending seed was consumed by its own activation, so
// re-activating it is a no-op and the REFUSED document's images kept
// resolving live ids.
use crate::web_canvas_server::{IngestOutcome, PendingDocumentPush, ServeMode};
// A distinctive id no other test uses, so this is immune to the
// process-global registry being cleared in parallel.
const KEPT: u64 = 515_243_617;
jian_ops_schema::image_thumbs::store_thumb(KEPT, vec![4, 5, 6]);
// A projected Active phase with no real session actor: the runtime cannot
// open a capture, so the ingest is refused.
let mut state = daemon();
in_session(
&mut state,
CollabConnectionPhase::Active,
CollabUiRole::Owner,
);
let body = serde_json::json!({
"document": {
"version": "1.0.0",
"children": [{
"id": "n1", "type": "rectangle", "name": "refused",
"x": 0, "y": 0, "width": 4, "height": 4,
}],
"imageThumbs": { "999111": "AQID" },
},
"sourceClientId": "s",
})
.to_string();
let mut push = PendingDocumentPush::parse(&body, ServeMode::Local).expect("parses");
let prepared = push.prepared.take().expect("a document push");
assert_eq!(
state.ingest_document_in_session(prepared),
IngestOutcome::Rejected
);
assert_eq!(
jian_ops_schema::image_thumbs::thumb_for(KEPT).as_deref(),
Some(&[4u8, 5, 6][..]),
"a refused ingest must leave the registry as it found it"
);
}

91
crates/op-host-services/src/web_canvas_server/connection.rs
View file

@ -338,20 +338,18 @@ pub(super) fn dispatch<S: Read + Write>(
// A document write must hold a pass for its whole locked segment, so
// shutdown cannot snapshot the document between the pass being taken
// and the commit landing.
let write_pass = match (ctx.write_barrier, pending_push.is_some()) {
(Some(barrier), true) => match barrier.enter() {
Some(pass) => Some(pass),
None => {
crate::mcp_serve::write_mcp_http_response_with_origin(
stream,
"503 Service Unavailable",
r#"{"ok":false,"error":"shutting-down","message":"this daemon is stopping and cannot accept writes"}"#,
cors_origin,
)?;
return Ok(false);
}
},
_ => None,
let write_pass = match admit_mutation(ctx, pending_push.is_some()) {
MutationAdmission::NotAWrite => None,
MutationAdmission::Admitted(pass) => pass,
MutationAdmission::ShuttingDown => {
crate::mcp_serve::write_mcp_http_response_with_origin(
stream,
"503 Service Unavailable",
SHUTTING_DOWN_REST_BODY,
cors_origin,
)?;
return Ok(false);
}
};
let reply = {
let mut guard = state.lock().unwrap_or_else(|p| p.into_inner());
@ -472,6 +470,36 @@ pub(super) fn dispatch<S: Read + Write>(
)?;
return Ok(false);
}
// A `/mcp` call that will mutate the document is admitted through the same
// barrier the REST push uses. `tool_profile` already knows which tools
// write, so the decision is made BEFORE dispatch — the alternative is
// discovering it from an `EditorCommand` the tool has already produced,
// which is after the point where refusing is still honest.
let mcp_write = op_mcp::parse_tool_call(&req.body)
.is_some_and(|call| crate::mcp_serve::tool_profile::tool_writes(&call.tool));
let mcp_write_pass = match admit_mutation(ctx, mcp_write) {
MutationAdmission::NotAWrite => None,
MutationAdmission::Admitted(pass) => pass,
MutationAdmission::ShuttingDown => {
// A tools/call error envelope, not a transport failure: an MCP
// client must be able to read the refusal and keep its session.
let refusal = op_mcp::parse_tool_call(&req.body).map(|call| {
op_mcp::tool_response_to_json(&op_mcp::ToolResponse::Err {
id: call.id.clone(),
code: op_mcp::ToolErrorCode::ToolFailed,
message: "shutting-down: this daemon is stopping and cannot accept writes"
.to_string(),
})
});
crate::mcp_serve::write_mcp_http_response_with_origin(
stream,
"200 OK",
refusal.as_deref().unwrap_or(SHUTTING_DOWN_REST_BODY),
cors_origin,
)?;
return Ok(false);
}
};
// JSON-RPC `/mcp` dispatch against the in-memory document. A mutating apply
// bumps the sync version, broadcast to SSE subscribers so the browser shell
// sees JSON-RPC-driven changes too.
@ -531,6 +559,8 @@ pub(super) fn dispatch<S: Read + Write>(
}
response
};
// Released only once the commit is visible in the shared state.
drop(mcp_write_pass);
let status = if response.is_empty() {
"202 Accepted"
} else {
@ -540,6 +570,39 @@ pub(super) fn dispatch<S: Read + Write>(
Ok(false)
}
/// The body a REST write gets once shutdown has closed the barrier.
const SHUTTING_DOWN_REST_BODY: &str = r#"{"ok":false,"error":"shutting-down","message":"this daemon is stopping and cannot accept writes"}"#;
/// Whether a mutation may proceed, and the pass that proves it is in flight.
pub(super) enum MutationAdmission<'a> {
/// Not a document mutation — nothing to admit.
NotAWrite,
/// Admitted. `None` when this deployment runs no barrier (local/managed).
Admitted(Option<super::tenant::WritePass<'a>>),
/// The daemon is stopping and will not durably accept this write.
ShuttingDown,
}
/// The single admission point for every document mutation.
///
/// Both the REST push and the `/mcp` write dispatch go through here, so a
/// mutation route added later cannot quietly skip the barrier — which is
/// exactly how `/mcp` came to be writing during shutdown while REST was
/// refused.
pub(super) fn admit_mutation<'a>(ctx: &ConnCtx<'a>, is_write: bool) -> MutationAdmission<'a> {
if !is_write {
return MutationAdmission::NotAWrite;
}
let Some(barrier) = ctx.write_barrier else {
// Local and managed daemons have no flush to protect.
return MutationAdmission::Admitted(None);
};
match barrier.enter() {
Some(pass) => MutationAdmission::Admitted(Some(pass)),
None => MutationAdmission::ShuttingDown,
}
}
/// Stream Server-Sent Events to a subscribed client: write the SSE headers,
/// emit the current tick immediately (initial sync), then forward each
/// bump from `rx` as a `data: {"version":N,"collabSeq":M}` event. A periodic

57
crates/op-host-services/src/web_canvas_server/online_run_loop.rs
View file

@ -44,6 +44,14 @@ use super::*;
/// enough that a deploy is not held up by a long-lived SSE stream.
const SHUTDOWN_DRAIN_SECS: u64 = 10;
/// How often the write drain reports the blocked-writer count.
const WRITE_DRAIN_REPORT_SECS: u64 = 5;
/// The ceiling on waiting for in-flight writes. Flushing over a live writer
/// loses acked work, so this is deliberately far longer than the connection
/// drain — and `stop_grace_period` must exceed it.
const WRITE_DRAIN_HARD_CAP_SECS: u64 = 60;
/// Wait for the active-connection count to reach zero, up to the bound.
///
/// Returns whether it actually drained. SSE streams routinely outlive this,
@ -60,19 +68,47 @@ fn drain_connections(conn_count: &Arc<AtomicUsize>) -> bool {
conn_count.load(Ordering::Acquire) == 0
}
/// Wait for in-flight document writes to finish, up to the bound.
/// Wait for in-flight document writes to finish.
///
/// Separate from the connection drain: an SSE stream holds a connection for
/// minutes and is safe to flush past, while a write in progress is not.
/// minutes and is safe to flush past, while a write in progress is not —
/// flushing over one loses work a client was already told had landed.
///
/// So this does NOT give up after the ordinary drain window. It keeps waiting
/// to a hard ceiling, reporting the blocked-writer count every
/// [`WRITE_DRAIN_REPORT_SECS`] so an operator can see why the stop is slow.
/// Reaching the ceiling is an error, not a routine outcome: `stop_grace_period`
/// must exceed [`WRITE_DRAIN_HARD_CAP_SECS`] or the container is killed
/// mid-flush regardless of what this does.
fn drain_write_barrier(barrier: &Arc<super::tenant::WriteBarrier>) -> bool {
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(SHUTDOWN_DRAIN_SECS);
while std::time::Instant::now() < deadline {
let started = std::time::Instant::now();
let mut next_report = std::time::Duration::from_secs(WRITE_DRAIN_REPORT_SECS);
loop {
if barrier.active() == 0 {
return true;
}
let waited = started.elapsed();
if waited >= std::time::Duration::from_secs(WRITE_DRAIN_HARD_CAP_SECS) {
eprintln!(
"openpencil --serve-web --online: ERROR {} write(s) still in flight after {}s; \
flushing anyway — those clients were acked and may not be persisted. Raise \
stop_grace_period above {}s.",
barrier.active(),
WRITE_DRAIN_HARD_CAP_SECS,
WRITE_DRAIN_HARD_CAP_SECS
);
return false;
}
if waited >= next_report {
eprintln!(
"openpencil --serve-web --online: waiting on {} in-flight write(s) ({}s)",
barrier.active(),
waited.as_secs()
);
next_report += std::time::Duration::from_secs(WRITE_DRAIN_REPORT_SECS);
}
std::thread::sleep(std::time::Duration::from_millis(20));
}
barrier.active() == 0
}
/// Longest gap between idle sweeps, however long the idle deadline is.
@ -233,15 +269,8 @@ pub fn run_online_web_canvas(options: ServeWebOptions) -> Result<()> {
// holding a pass would otherwise commit after the flush snapshotted the
// document, having already answered 200.
write_barrier.close();
let writes_settled = drain_write_barrier(&write_barrier);
if !writes_settled {
eprintln!(
"openpencil --serve-web --online: {} write(s) still in flight after {}s; \
flushing anyway — those clients were acked but may not be persisted",
write_barrier.active(),
SHUTDOWN_DRAIN_SECS
);
}
// Reports and escalates internally; a `false` means the hard cap was hit.
let _writes_settled = drain_write_barrier(&write_barrier);
let drained = drain_connections(&conn_count);
let flush_started = std::time::Instant::now();
let flushed = registry.flush_all();

76
crates/op-host-services/src/web_canvas_server/online_shutdown_tests.rs
View file

@ -81,3 +81,79 @@ fn a_write_during_shutdown_is_refused_rather_than_acked() {
);
assert_eq!(body(&response)["error"], "shutting-down");
}
#[test]
fn an_mcp_write_tool_is_refused_once_the_barrier_closes() {
// `/mcp` used to apply straight to the editor with no barrier, so a write
// tool could commit after the flush snapshot — the exact window the REST
// path was already protected from.
use crate::web_canvas_server::tenant::WriteBarrier;
let registry = registry();
let verifier = verifier();
let barrier = WriteBarrier::default();
barrier.close();
let call = r#"{"jsonrpc":"2.0","id":7,"method":"tools/call","params":{"name":"add_page","arguments":{"name":"x"}}}"#;
let request = Request::json("POST", "/mcp", call).with_bearer("tokA");
let mut stream = MockStream {
input: std::io::Cursor::new(request.wire().into_bytes()),
output: Vec::new(),
};
serve_one_online(&mut stream, &registry, &verifier, &barrier).expect("serve");
let response = String::from_utf8_lossy(&stream.output).into_owned();
// A tools/call error envelope, not a transport failure: the client keeps
// its session and can read why.
assert_eq!(status_line(&response), "HTTP/1.1 200 OK", "{response}");
let payload = body(&response);
assert_eq!(payload["result"]["isError"], true, "{response}");
assert!(
payload["result"]["content"][0]["text"]
.as_str()
.unwrap_or_default()
.contains("shutting-down"),
"{response}"
);
}
#[test]
fn an_mcp_read_tool_still_works_while_shutting_down() {
// Only writes are refused: a read cannot be lost by the flush.
use crate::web_canvas_server::tenant::WriteBarrier;
let barrier = WriteBarrier::default();
barrier.close();
let call = r#"{"jsonrpc":"2.0","id":8,"method":"tools/call","params":{"name":"get_document_info","arguments":{}}}"#;
let request = Request::json("POST", "/mcp", call).with_bearer("tokA");
let mut stream = MockStream {
input: std::io::Cursor::new(request.wire().into_bytes()),
output: Vec::new(),
};
serve_one_online(&mut stream, &registry(), &verifier(), &barrier).expect("serve");
let response = String::from_utf8_lossy(&stream.output).into_owned();
assert_ne!(body(&response)["result"]["isError"], true, "{response}");
}
#[test]
fn the_write_classification_matches_the_tool_catalog() {
// The barrier decision is made before dispatch, from this metadata.
use crate::mcp_serve::tool_profile::tool_writes;
for write in [
"add_page",
"insert_node",
"delete_node",
"undo",
"batch_design",
] {
assert!(tool_writes(write), "{write}");
}
for read in [
"get_node",
"list_pages",
"get_document_info",
"snapshot_layout",
] {
assert!(!tool_writes(read), "{read}");
}
// Unclassified tools are admitted through the barrier rather than past it.
assert!(tool_writes("add_some_dynamic_kit_component"));
}

16
crates/op-host-web/src/canvaskit/inner.rs
View file

@ -178,15 +178,13 @@ impl crate::repaint_ctx::RepaintContext for CkInner {
&mut self.host
}
fn reset_persistence_baselines(&mut self) {
// Recomputed from the account's own freshly loaded state on the next
// comparison, rather than carried over from the previous account's.
self.settings_fingerprint = None;
// Rebuilt from the state the account's own partition just loaded, so
// the next comparison measures against THIS account rather than
// reporting the whole partition as a change.
self.credential_fingerprint =
crate::web_settings::credential_fingerprint(self.host.editor_state());
fn reset_persistence_baselines(&mut self, load: &crate::web_settings::CredentialLoad) {
// Through the SAME constructors mount uses. A bare recompute set
// `settings_fingerprint` to `None` — which the save gate reads as
// "never save" — and dropped the credential write-pending retry and
// the fail-closed `write_disabled` an unsupported snapshot sets.
self.settings_fingerprint = load.initial_settings_fingerprint(self.host.editor_state());
self.credential_fingerprint = load.initial_fingerprint(self.host.editor_state());
}
fn viewport_size(&self) -> (f32, f32) {
self.backend.logical_size()

5
crates/op-host-web/src/live_sync_identity.rs
View file

@ -63,6 +63,11 @@ pub(crate) fn reset_for_new_identity<C: RepaintContext + 'static>(inner: &Rc<Ref
let _ = context.repaint();
}
crate::collab_sync::reset_for_new_identity();
// Queued and in-flight credential uploads belong to the previous account;
// letting them land would push its API keys into the new account's tenant.
// `reset` also re-tags the epoch, which is what makes an XHR already on the
// wire inert when it completes.
crate::web_credential_sync::reset();
// The id allocator lives on the host, so it is torn down here where the
// borrow is already held. B1's namespace latch alone was not enough: the
// allocator itself would keep minting in the previous account's namespace.

18
crates/op-host-web/src/repaint_ctx.rs
View file

@ -16,16 +16,24 @@ use wasm_bindgen::JsValue;
pub(crate) trait RepaintContext {
fn host(&self) -> &WidgetHost;
fn host_mut(&mut self) -> &mut WidgetHost;
/// Forget the settings/credential change-detection baselines.
/// Rebuild the settings/credential change-detection baselines from the
/// partition that was just loaded.
///
/// Called when the tab switches accounts. The baselines were computed
/// against the PREVIOUS account's state, so keeping them means the first
/// comparison after the switch reports a spurious change and writes — or
/// uploads — the new account's partition against the old one's shape.
/// comparison after the switch reports a spurious change and writes the
/// new account's partition against the old one's shape.
///
/// `load` carries the semantics a bare recompute would throw away — the
/// write-pending retry, and the fail-closed `write_disabled` an
/// unsupported snapshot sets — so it is rebuilt through the same
/// `initial_*` constructors mount uses, not from the state alone.
///
/// Defaulted to a no-op: a backend with no persistence baselines (the
/// smoke harness) has nothing to forget.
fn reset_persistence_baselines(&mut self) {}
/// smoke harness) has nothing to rebuild.
fn reset_persistence_baselines(&mut self, load: &crate::web_settings::CredentialLoad) {
let _ = load;
}
/// Logical viewport size in CSS pixels. File-open/import flows fit the
/// loaded content to this size after replacing the document.
fn viewport_size(&self) -> (f32, f32);

32
crates/op-host-web/src/web_credential_sync.rs
View file

@ -186,6 +186,25 @@ thread_local! {
/// request, so it is safe to run ahead of the postMessage bridge init gate.
pub(crate) fn reset() {
SYNC_STATE.with(|state| *state.borrow_mut() = CredentialSyncState::default());
// Any callback already in flight was issued for the PREVIOUS account.
// Clearing the queue cannot recall it, so the epoch is what makes it inert
// when it lands — see `identity_is_current`.
ISSUED_EPOCH.with(|epoch| epoch.set(crate::identity_epoch::epoch()));
}
thread_local! {
/// The identity epoch the in-flight requests were issued under.
static ISSUED_EPOCH: std::cell::Cell<u64> = const { std::cell::Cell::new(0) };
}
/// Whether a completing callback still belongs to the account that issued it.
///
/// An XHR cannot be un-issued: `reset` empties the queue, but a POST already
/// on the wire will still complete and its callback would fold the previous
/// account's result — a success, a retry schedule, an error banner — into the
/// new account's state. Comparing epochs at completion is what discards it.
fn identity_is_current() -> bool {
ISSUED_EPOCH.with(std::cell::Cell::get) == crate::identity_epoch::epoch()
}
/// Begin credential-policy discovery against the daemon. This issues a daemon
@ -241,6 +260,9 @@ fn request_repaint() {
fn fetch_policy() {
let base = crate::daemon_base::daemon_base();
let on_response: Rc<dyn Fn(u16, String)> = Rc::new(|status, body| {
if !identity_is_current() {
return; // issued for a previous account
}
let policy = parse_policy_response(status, &body);
if policy.is_none() {
report_sync_failure(Some(status));
@ -263,6 +285,11 @@ fn fetch_policy() {
fn post_credentials(body: &str) {
let base = crate::daemon_base::daemon_base();
let on_response: Rc<dyn Fn(u16, String)> = Rc::new(|status, _| {
if !identity_is_current() {
// Issued for a previous account: its result must not become the
// new account's success, retry schedule or error banner.
return;
}
if !(200..300).contains(&status) {
report_sync_failure(Some(status));
}
@ -290,6 +317,11 @@ fn schedule_retry(delay_ms: i32, generation: u64) {
use wasm_bindgen::JsCast;
let callback = wasm_bindgen::closure::Closure::<dyn FnMut()>::once_into_js(move || {
if !identity_is_current() {
// A timer armed for a previous account: firing it would resend
// that account's credential payload into the new tenant.
return;
}
let action = SYNC_STATE.with(|state| state.borrow_mut().retry_due(generation));
dispatch(action);
});

53
crates/op-host-web/src/web_settings.rs
View file

@ -30,6 +30,42 @@ const CREDENTIAL_PAYLOAD_VERSION: u32 = 2;
const STORAGE_KEY: &str = "openpencil-rust-web-settings";
const CREDENTIAL_STORAGE_KEY: &str = "openpencil-rust-web-credentials";
/// Restore every setting the partition snapshot owns to its default.
///
/// `apply_payload` only writes fields the stored blob actually carries, so an
/// EMPTY partition left the previous account's values in place: B inherited
/// A's locale, recent files, MCP port and provider profiles. Resetting first
/// makes "absent from the blob" mean "default" rather than "keep whatever was
/// there".
///
/// The field set is exactly what `apply_payload` writes, so the two cannot
/// drift into a field that is saved per-account but never reset.
///
/// `theme_mode` is included: it is stored IN the per-account blob, so it is
/// account-scoped by the same definition as everything else here. (A device
/// preference that ought to survive an account switch would have to move out
/// of the settings payload first — a product decision, not one this reset can
/// make unilaterally.)
pub(super) fn reset_account_scoped_settings(state: &mut EditorState) {
let defaults = op_editor_core::AgentSettings::default();
let eui = &mut state.editor_ui;
eui.theme_mode = op_editor_core::EditorUiState::default().theme_mode;
eui.locale = op_editor_core::EditorUiState::default().locale;
eui.recent_files.clear();
eui.agent_settings.mcp_server.port = defaults.mcp_server.port;
eui.agent_settings.mcp_cli_enabled = defaults.mcp_cli_enabled;
eui.agent_settings.images_advanced_open = defaults.images_advanced_open;
eui.agent_settings.openverse_client_id = defaults.openverse_client_id;
eui.agent_settings.openverse_client_secret = defaults.openverse_client_secret;
eui.agent_settings.auto_update_enabled = defaults.auto_update_enabled;
eui.agent_settings.experimental_features_enabled = defaults.experimental_features_enabled;
eui.agent_settings.builtin_agents = defaults.builtin_agents;
eui.agent_settings.next_builtin_agent_id = defaults.next_builtin_agent_id;
eui.agent_settings.image_gen_profiles = defaults.image_gen_profiles;
eui.agent_settings.next_image_gen_profile_id = defaults.next_image_gen_profile_id;
eui.agent_settings.active_image_gen_profile_id = defaults.active_image_gen_profile_id;
}
/// Re-read account-scoped storage after the tab's partition changed.
///
/// The shell loads settings and credentials at mount, before any
@ -43,12 +79,15 @@ pub(crate) fn reload_for_active_partition<C: crate::repaint_ctx::RepaintContext>
let Ok(mut context) = inner.try_borrow_mut() else {
return;
};
let _ = storage::load_into(context.host_mut().editor_state_mut());
// Defaults first, then the target partition on top: without this an empty
// partition silently inherits the previous account's settings.
reset_account_scoped_settings(context.host_mut().editor_state_mut());
let load = storage::load_into(context.host_mut().editor_state_mut());
// AFTER the load, not before: the baselines must measure against the state
// this partition just produced. Rebuilding them from the previous
// account's state would make the very next comparison report the whole
// partition as a change and write it back under the wrong key.
context.reset_persistence_baselines();
context.reset_persistence_baselines(&load);
context.host_mut().mark_editor_state_dirty();
let _ = context.repaint();
}
@ -92,6 +131,7 @@ use storage::{
};
pub(crate) use storage::{
credential_migration_pending, load_into, save_credentials_if_changed, save_if_changed,
CredentialLoad,
};
#[derive(Debug, Clone, PartialEq, Eq)]
@ -106,6 +146,15 @@ pub(crate) struct Fingerprint {
recent_files: Vec<RecentFile>,
}
impl CredentialFingerprint {
/// The fail-closed flag, for tests that assert an unsupported snapshot
/// keeps writes disabled.
#[cfg(test)]
pub(crate) fn write_disabled_for_test(&self) -> bool {
self.write_disabled
}
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct CredentialFingerprint {
builtin_agents: Vec<BuiltinAgentConfig>,

115
crates/op-host-web/src/web_settings_partition_tests.rs Normal file
View file

@ -0,0 +1,115 @@
//! Account-partition isolation: what an empty partition must clear, and what
//! the rebuilt persistence baselines must preserve.
//!
//! Split out of `web_settings_tests.rs` at the 800-line cap; nested under it
//! so `use super::*` still reaches the shared helpers.
use super::*;
#[test]
fn an_empty_partition_clears_the_previous_accounts_credentials() {
// The account-switch leak: after switching, the in-memory state still held
// account A's API keys, and an empty partition for B meant "keep whatever
// was there" instead of "no keys".
let mut state = EditorState::new();
let mut writes = Vec::new();
// A signs in and stores a key.
let credential_json = r#"{"version":2,"builtin_agents":[{"id":"a1","preset":"custom",
"display_name":"A's model","kind":"openai-compat","api_key":"sk-account-a",
"model":"m","base_url":"https://api.example.com/v1","enabled":true}],
"image_gen_profiles":[],"active_image_gen_profile_id":null,"openverse_oauth":null}"#;
super::storage::load_into_with(&mut state, None, Some(credential_json), |k, v| {
writes.push((k.to_string(), v.to_string()));
true
});
assert!(
!state.editor_ui.agent_settings.builtin_agents.is_empty(),
"A's credential must load in the first place"
);
// B signs in: their partition is empty.
super::storage::load_into_with(&mut state, None, None, |_, _| true);
assert!(
state.editor_ui.agent_settings.builtin_agents.is_empty(),
"B must not inherit A's API keys from an empty partition"
);
}
#[test]
fn an_empty_partition_resets_account_scoped_settings_to_defaults() {
// `apply_payload` only writes fields the blob carries, so an empty
// partition used to leave A's locale, recent files and provider config in
// place for B.
let mut state = EditorState::new();
state.editor_ui.locale = Locale::Ja;
state.editor_ui.recent_files = vec![RecentFile {
path: "/a/secret-project.op".into(),
modified_at: 1,
}];
state.editor_ui.agent_settings.mcp_server.port = 4242;
state.editor_ui.agent_settings.openverse_client_id = "a-client".into();
super::reset_account_scoped_settings(&mut state);
super::storage::load_into_with(&mut state, None, None, |_, _| true);
let defaults = op_editor_core::EditorUiState::default();
let default_agents = op_editor_core::AgentSettings::default();
assert_eq!(state.editor_ui.locale, defaults.locale, "locale must reset");
assert!(
state.editor_ui.recent_files.is_empty(),
"B must not see A's recent files"
);
assert_eq!(
state.editor_ui.agent_settings.mcp_server.port,
default_agents.mcp_server.port
);
assert!(state
.editor_ui
.agent_settings
.openverse_client_id
.is_empty());
}
#[test]
fn a_populated_partition_still_wins_over_the_defaults() {
// The reset must not erase the partition being loaded — defaults first,
// then the target snapshot on top.
let mut state = EditorState::new();
state.editor_ui.locale = Locale::Ja;
let settings = r#"{"version":1,"locale":"fr","mcp_port":5150}"#;
super::reset_account_scoped_settings(&mut state);
super::storage::load_into_with(&mut state, Some(settings), None, |_, _| true);
assert_eq!(state.editor_ui.locale, Locale::Fr);
assert_eq!(state.editor_ui.agent_settings.mcp_server.port, 5150);
}
#[test]
fn rebuilt_baselines_keep_saving_and_keep_failing_closed() {
// The regression this pins: setting `settings_fingerprint` to `None`
// makes the save gate (`if let Some(..)`) skip forever, so nothing was
// ever persisted again after an account switch.
let mut state = EditorState::new();
let healthy = super::storage::load_into_with(&mut state, None, None, |_, _| true);
assert!(
healthy.initial_settings_fingerprint(&state).is_some(),
"a healthy partition must leave the save path enabled"
);
assert!(!healthy
.initial_fingerprint(&state)
.write_disabled_for_test());
// …and an unsupported snapshot must still fail closed rather than being
// "reset" into a writable baseline.
let unsupported = r#"{"version":9999}"#;
let mut state = EditorState::new();
let blocked = super::storage::load_into_with(&mut state, Some(unsupported), None, |_, _| true);
assert!(
blocked.initial_settings_fingerprint(&state).is_none(),
"an unsupported snapshot must keep settings writes disabled"
);
assert!(blocked
.initial_fingerprint(&state)
.write_disabled_for_test());
}

30
crates/op-host-web/src/web_settings_tests.rs
View file

@ -738,31 +738,5 @@ fn pending_legacy_migration_blocks_an_ordinary_settings_write() {
assert_ne!(settings_baseline, fingerprint(&state));
}
#[test]
fn an_empty_partition_clears_the_previous_accounts_credentials() {
// The account-switch leak: after switching, the in-memory state still held
// account A's API keys, and an empty partition for B meant "keep whatever
// was there" instead of "no keys".
let mut state = EditorState::new();
let mut writes = Vec::new();
// A signs in and stores a key.
let credential_json = r#"{"version":2,"builtin_agents":[{"id":"a1","preset":"custom",
"display_name":"A's model","kind":"openai-compat","api_key":"sk-account-a",
"model":"m","base_url":"https://api.example.com/v1","enabled":true}],
"image_gen_profiles":[],"active_image_gen_profile_id":null,"openverse_oauth":null}"#;
super::storage::load_into_with(&mut state, None, Some(credential_json), |k, v| {
writes.push((k.to_string(), v.to_string()));
true
});
assert!(
!state.editor_ui.agent_settings.builtin_agents.is_empty(),
"A's credential must load in the first place"
);
// B signs in: their partition is empty.
super::storage::load_into_with(&mut state, None, None, |_, _| true);
assert!(
state.editor_ui.agent_settings.builtin_agents.is_empty(),
"B must not inherit A's API keys from an empty partition"
);
}
#[path = "web_settings_partition_tests.rs"]
mod partition_tests;

2
vendor/jian vendored

@ -1 +1 @@
Subproject commit af3b7bfe3dbd95baaa5886b43b17f2d70d7fc18f
Subproject commit f0c24707d9cebca63b6b24a89bca41b769889989