Refactor owner scope across channels and fix default routing fallback - #1151
Merged
Merged
Conversation
github-actions
Bot
added
scope: agent
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the system's ownership model, moving from an implicit 'default' user concept to an explicit owner scope. This change ensures that persistent data and configurations are consistently tied to a single owner across all communication channels, while maintaining appropriate isolation for guest users and conversation-specific state. The update streamlines proactive message routing and enhances the system's robustness and clarity regarding data ownership. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring to establish a clear owner scope across various channels, which is a crucial improvement for multi-channel and multi-user scenarios. The changes consistently replace hardcoded "default" user scopes with a resolved owner ID, properly scoping routines, jobs, settings, and credentials. The logic for routing notifications, especially the fallback for owner-targeted messages, has been fixed and made more robust. The introduction of new fields in IncomingMessage to distinguish between owner, sender, and conversation scope is a solid architectural enhancement. The addition of new E2E tests covering these cross-channel owner-scoped flows provides confidence in the correctness of this large-scale change. Overall, this is an excellent refactoring that greatly improves the application's architecture and maintainability.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors “owner scope” handling so instance-global state (routines/jobs/settings/secrets/extensions/workspace) is consistently keyed to a stable owner ID across channels, while channel interactions remain scoped to sender/thread; also fixes routine notification routing by removing the legacy "default" notify target sentinel and resolving proactive delivery targets from stored channel metadata.
Changes:
- Introduces explicit
IRONCLAW_OWNER_ID/owner_id/sender_id/conversation_scope_idplumbing across channels, agent context, and tool execution. - Makes
routines.notify_usernullable (migrations + DB adapters) and updates routine/message tooling to resolve owner targets from persisted channel metadata instead of"default". - Adds/updates regression coverage and new Playwright E2E scenarios for cross-channel owner-scoped routine/job visibility and HTTP/web owner flows.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/telegram_auth_integration.rs | Updates Telegram auth expectations for owner-scope/guest behavior. |
| tests/e2e_routine_heartbeat.rs | Extends test messages to include new owner/sender/scope fields. |
| tests/e2e/scenarios/test_owner_scope.py | Adds E2E coverage for owner-scoped HTTP/web routine + job flows. |
| tests/e2e/mock_llm.py | Adds mock tool-call patterns for routine create/list used by new E2E tests. |
| tests/e2e/helpers.py | Adds signed HTTP webhook helper and selectors/constants for jobs/routines UI. |
| tests/e2e/conftest.py | Boots gateway + HTTP webhook with owner scope env and new fixtures. |
| src/tools/wasm/wrapper.rs | Switches WASM tool host-credential resolution to use job context scope. |
| src/tools/builtin/routine.rs | Makes notify_user optional and updates tool param handling accordingly. |
| src/tools/builtin/message.rs | Adds target resolution fallback for owner-scoped proactive sends + tests. |
| src/testing/mod.rs | Updates test fixtures for NotifyConfig.user becoming optional. |
| src/setup/wizard.rs | Keys settings/secrets persistence off resolved owner scope instead of hardcoded default. |
| src/settings.rs | Adds bootstrap-only owner_id and prevents persisting it to the DB settings map. |
| src/main.rs | Threads owner scope through REPL/webhooks and SIGHUP config/secrets reload. |
| src/history/store.rs | Initializes new JobContext.requester_id field when hydrating jobs. |
| src/extensions/manager.rs | Binds WASM channels to owner scope and optional owner actor IDs. |
| src/db/libsql_migrations.rs | Makes notify_user nullable in fresh schema and adds incremental migration 13. |
| src/db/libsql/routines.rs | Writes notify_user as NULL-able via opt_text(...). |
| src/db/libsql/mod.rs | Normalizes legacy "default"/empty notify_user values to None on read + tests. |
| src/db/libsql/jobs.rs | Initializes new JobContext.requester_id field when hydrating jobs. |
| src/context/state.rs | Adds requester_id to JobContext for channel-actor attribution. |
| src/config/mod.rs | Adds Config.owner_id and resolves it from env/settings, feeding channel config. |
| src/config/channels.rs | Resolves HTTP/gateway channel user IDs from the resolved owner scope. |
| src/cli/routines.rs | Updates CLI routine creation for optional notify user. |
| src/cli/doctor.rs | Updates doctor gateway config resolution to include owner scope. |
| src/channels/wasm/wrapper.rs | Adds owner binding + guest isolation, conversation scope extraction, and routing fallback for broadcasts. |
| src/channels/wasm/setup.rs | Loads channel secrets and injects credentials under owner scope; binds owner actor IDs. |
| src/channels/repl.rs | Adds owner scope to REPL channel messages and single-message mode. |
| src/channels/mod.rs | Re-exports routing_target_from_metadata for routing fallback usage. |
| src/channels/http.rs | Sets owner_id/sender_id on HTTP webhook messages while keeping owner scope as channel identity. |
| src/channels/channel.rs | Adds owner/sender/scope fields and routing helpers to IncomingMessage. |
| src/app.rs | Threads owner scope through bootstrap, session, workspace, MCP, and WASM setup. |
| src/agent/thread_ops.rs | Propagates requester identity into tool-execution job contexts. |
| src/agent/routine_engine.rs | Adds owner_id into routine/job metadata for downstream routing decisions. |
| src/agent/routine.rs | Makes routine notify target optional (NotifyConfig.user: Option<String>). |
| src/agent/heartbeat.rs | Uses workspace owner scope as notify default and records owner_id in metadata. |
| src/agent/dispatcher.rs | Propagates requester identity into tool-execution job contexts. |
| src/agent/commands.rs | Persists model selection under owner scope. |
| src/agent/agent_loop.rs | Uses routing_target helper for message-tool default target and carries owner_id through notification routing. |
| migrations/V6__routines.sql | Updates fresh SQL schema to allow NULL notify_user. |
| migrations/V13__owner_scope_notify_targets.sql | Adds Postgres migration to drop default sentinel + normalize legacy "default" to NULL. |
| channels-src/telegram/src/lib.rs | Keeps non-owner senders as guests and sets thread_id to chat ID for conversation scoping. |
| FEATURE_PARITY.md | Updates documentation to reflect explicit owner scope and routing model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors IronClaw’s identity/persistence model to use an explicit, stable owner scope (via IRONCLAW_OWNER_ID) rather than implicitly keying persistent state off the channel sender ID, and fixes the "default" proactive routing sentinel by resolving real channel targets from stored owner metadata instead.
Changes:
- Introduces owner/sender/conversation scope plumbing across channels (HTTP/REPL/WASM/Telegram) and job execution (
requester_id). - Makes routine notify targets nullable (removing the
"default"sentinel) with DB migrations + normalization and updated routing fallback logic. - Adds/updates regression + Playwright/E2E coverage for owner scope behavior and routing fallbacks.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/telegram_auth_integration.rs | Updates auth behavior expectations for owner vs guest Telegram senders. |
| tests/e2e_routine_heartbeat.rs | Adapts tests to new IncomingMessage identity fields. |
| tests/e2e_builtin_tool_coverage.rs | Updates routine notify assertions for optional notify targets. |
| tests/e2e/scenarios/test_owner_scope.py | New E2E scenarios validating owner-global routines/jobs across web + HTTP. |
| tests/e2e/mock_llm.py | Adds mock tool-call patterns for owner-scope routine scenarios. |
| tests/e2e/helpers.py | Adds HTTP webhook signing helpers and test constants. |
| tests/e2e/conftest.py | Wires owner ID + HTTP channel into the E2E harness. |
| src/tools/wasm/wrapper.rs | Switches host-credential lookup to context-scoped user ID. |
| src/tools/builtin/routine.rs | Makes notify_user optional and stops defaulting to "default". |
| src/tools/builtin/message.rs | Adds owner-scope fallback behavior when channel is known but target omitted. |
| src/testing/mod.rs | Updates test routines to use NotifyConfig.user: Option<String>. |
| src/setup/wizard.rs | Re-keys settings/secrets operations off the resolved owner scope (no longer hardcoded "default"). |
| src/settings.rs | Adds non-persisted bootstrap owner_id and filters it out of DB settings maps. |
| src/main.rs | Passes configured owner ID into CLI/REPL and tool webhook state. |
| src/history/store.rs | Initializes loaded job contexts with requester_id: None. |
| src/extensions/manager.rs | Binds WASM channels to owner scope + optional owner actor ID. |
| src/db/libsql_migrations.rs | Makes routines.notify_user nullable + adds migration to normalize legacy "default". |
| src/db/libsql/routines.rs | Writes notify_user as nullable for libSQL routines. |
| src/db/libsql/mod.rs | Adds notify-user normalization helper and applies it to routine row mapping. |
| src/db/libsql/jobs.rs | Initializes loaded job contexts with requester_id: None. |
| src/context/state.rs | Adds requester_id to JobContext and builder method. |
| src/config/mod.rs | Resolves owner_id from env/settings and threads it into channel config resolution. |
| src/config/channels.rs | Makes HTTP/gateway channel configs owner-scoped (user_id derived from owner). |
| src/cli/routines.rs | Makes CLI routine notify config rely on runtime target resolution (no explicit user). |
| src/cli/doctor.rs | Updates gateway config check to resolve owner ID before channel config resolution. |
| src/channels/wasm/wrapper.rs | Adds owner binding, owner-scope routing metadata persistence, and owner-aware broadcast routing. |
| src/channels/wasm/setup.rs | Injects owner-scoped secrets/credentials and owner binding when registering WASM channels. |
| src/channels/repl.rs | Makes REPL messages owner-scoped and plumbs user_id through message construction. |
| src/channels/mod.rs | Re-exports routing_target_from_metadata. |
| src/channels/http.rs | Creates owner-scoped HTTP messages with separate sender_id and stable conversation scope. |
| src/channels/channel.rs | Extends IncomingMessage with owner/sender/conversation-scope and routing target helpers. |
| src/app.rs | Re-keys bootstrap/migration/session/workspace/MCP loading to owner scope. |
| src/agent/thread_ops.rs | Sets requester_id when executing tools from approvals. |
| src/agent/routine_engine.rs | Carries owner_id in notification/job metadata and routes notifications with owner scope. |
| src/agent/routine.rs | Makes NotifyConfig.user optional and updates defaults. |
| src/agent/heartbeat.rs | Defaults heartbeat notify target to workspace/owner scope and carries owner_id in metadata. |
| src/agent/dispatcher.rs | Sets requester_id for interactive chat tool execution contexts. |
| src/agent/commands.rs | Persists selected model under owner scope (not hardcoded "default"). |
| src/agent/agent_loop.rs | Adds owner-aware notification routing fallback logic and uses routing_target helper. |
| migrations/V6__routines.sql | Updates fresh schema to make notify_user nullable. |
| migrations/V13__owner_scope_notify_targets.sql | Postgres migration to drop sentinel default + normalize legacy rows. |
| channels-src/telegram/src/lib.rs | Treats non-owner senders as guests; sets thread_id to chat ID for scoping. |
| FEATURE_PARITY.md | Updates parity notes to reflect explicit owner scope model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors runtime identity and persistence to use an explicit, stable instance owner_id (configurable via IRONCLAW_OWNER_ID) rather than implicitly keying durable state off channel sender IDs, and fixes routine notification routing by removing the legacy "default" notify-user sentinel.
Changes:
- Introduces owner/sender/conversation-scope identity plumbing across channels, agent runtime, and job context.
- Makes routine notify targets nullable (migrations + stores) and updates routing to resolve owner last-seen targets instead of sending to
"default". - Adds/updates unit + integration + Playwright E2E coverage for owner scoping and routing fallback behavior.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/telegram_auth_integration.rs | Updates Telegram WASM auth integration expectations for owner-vs-guest behavior. |
| tests/support/test_rig.rs | Passes resolved owner_id into AgentDeps in test harness builder. |
| tests/support/gateway_workflow_harness.rs | Passes resolved owner_id into AgentDeps in gateway workflow harness. |
| tests/e2e_routine_heartbeat.rs | Adapts test messages to new IncomingMessage identity fields. |
| tests/e2e_builtin_tool_coverage.rs | Updates assertions for NotifyConfig.user becoming Option. |
| tests/e2e/scenarios/test_owner_scope.py | New Playwright E2E scenarios validating owner-scoped routines/jobs across web + HTTP. |
| tests/e2e/mock_llm.py | Adds mock LLM patterns for owner-scope routine create/list flows. |
| tests/e2e/helpers.py | Adds signed webhook helper + new selectors/constants for owner-scope E2E. |
| tests/e2e/conftest.py | Boots gateway + HTTP webhook with owner-scope env and distinct ports; enables routines. |
| src/tools/wasm/wrapper.rs | Switches host-credential lookup to use JobContext.user_id (scope-aware). |
| src/tools/builtin/routine.rs | Makes notify_user optional in tool schema + creation logic. |
| src/tools/builtin/message.rs | Adds owner-scope fallback routing when channel is known; adds regression test. |
| src/testing/mod.rs | Updates test harness defaults for AgentDeps.owner_id and nullable notify users. |
| src/setup/wizard.rs | Loads bootstrap settings (env/TOML) and threads resolved owner scope into setup flows. |
| src/settings.rs | Adds bootstrap-only owner_id field (not persisted in per-user DB settings table). |
| src/main.rs | Uses TOML-aware setup wizard constructor; binds CLI/REPL + webhooks + SIGHUP reload to owner scope. |
| src/history/store.rs | Initializes new JobContext.requester_id when hydrating jobs from DB. |
| src/extensions/manager.rs | Binds WASM channels to owner scope + optional owner-actor mapping. |
| src/db/libsql_migrations.rs | Makes routines.notify_user nullable; adds migration to normalize legacy 'default'. |
| src/db/libsql/routines.rs | Writes notify_user as nullable for libSQL routines store. |
| src/db/libsql/mod.rs | Adds normalize_notify_user() and applies it when reading routines. |
| src/db/libsql/jobs.rs | Initializes new JobContext.requester_id when hydrating jobs from DB. |
| src/context/state.rs | Adds requester_id to JobContext plus builder method. |
| src/config/mod.rs | Adds owner_id to config + bootstrap settings loader + owner-id resolver. |
| src/config/channels.rs | Resolves gateway/HTTP channel configured user_id from owner scope. |
| src/cli/routines.rs | Defaults CLI-created routine notify target to “resolve at runtime” (user: None). |
| src/cli/doctor.rs | Uses shared owner-id resolution before resolving channel config. |
| src/channels/wasm/wrapper.rs | Implements owner binding, emitted-message scope resolution, owner-target routing metadata persistence, and owner-route broadcast resolution. |
| src/channels/wasm/setup.rs | Looks up channel secrets under owner scope; binds channels to owner scope/actor mapping; injects credentials owner-scoped. |
| src/channels/repl.rs | Adds REPL user binding so REPL operates under configured owner scope. |
| src/channels/mod.rs | Re-exports routing_target_from_metadata for shared routing resolution. |
| src/channels/http.rs | Separates HTTP owner scope from sender identity; populates owner/sender fields + conversation scope. |
| src/channels/channel.rs | Extends IncomingMessage with owner_id, sender_id, conversation_scope_id and routing helpers. |
| src/app.rs | Re-keys bootstrap migration, session attach, workspace, MCP server load, and WASM setup to owner scope. |
| src/agent/thread_ops.rs | Sets JobContext.requester_id from inbound sender for approval/tool flows. |
| src/agent/routine_engine.rs | Ensures event triggers respect routine user_id scope; propagates owner_id into notifications/job metadata. |
| src/agent/routine.rs | Makes NotifyConfig.user optional and updates default. |
| src/agent/heartbeat.rs | Defaults heartbeat notify user to workspace owner scope; annotates owner_id in metadata. |
| src/agent/dispatcher.rs | Sets JobContext.requester_id for interactive chat execution. |
| src/agent/commands.rs | Persists selected model setting under resolved owner scope. |
| src/agent/agent_loop.rs | Adds owner-id handling, routine notification target resolution, fallback logic, and conversation-scope usage. |
| migrations/V6__routines.sql | Updates fresh schema: notify_user nullable (no 'default' sentinel). |
| migrations/V13__owner_scope_notify_targets.sql | Postgres migration: drops NOT NULL/default on notify_user, normalizes 'default' to NULL. |
| channels-src/telegram/src/lib.rs | Changes owner handling to treat non-owner as guest (authorization applies); emits chat id as thread scope. |
| FEATURE_PARITY.md | Updates parity notes to document explicit owner scope and identity separation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zmanian
requested changes
Mar 14, 2026
zmanian
left a comment
zmanian
left a comment
Collaborator
There was a problem hiding this comment.
Review
The architecture is sound -- cleanly separating owner_id / sender_id / conversation_scope_id addresses a real cross-channel identity inconsistency. CI is green across all checks. Three issues to address before merge.
Blocking
1. HTTP channel debug log is misleading (src/channels/http.rs)
The handler now uses req.user_id as sender_id while state.user_id remains the storage scope, but the existing debug log still says the provided user_id is "being ignored." This will mislead operators debugging webhook behavior. Update the log message to reflect the new semantics.
2. WASM tool credential lookup change needs verification (src/tools/wasm/wrapper.rs)
The change from let credential_user_id = "default" to let credential_user_id = &ctx.user_id reverses a previous explicit fix. The old comment explained: ExtensionManager stores OAuth tokens under user_id "default".
Now that ctx.user_id is the owner scope for owner-originated messages, this should work for the owner. But if ExtensionManager still stores credentials under one key (e.g., config.owner_id) while ctx.user_id resolves to something different, credential lookup silently fails with no error -- the tool just gets no credentials.
Please add:
- A trace log when credential lookup finds no match (to aid debugging)
- A test case verifying that a WASM tool invoked from an owner-scoped message can resolve credentials stored under the owner scope
3. owner_scope_id defaults to "default" silently (src/channels/wasm/wrapper.rs)
WasmChannel::new() sets owner_scope_id: "default".to_string() before with_owner_binding() is called. If any code path constructs a WasmChannel and forgets to call with_owner_binding(), it silently operates under the wrong scope. Consider either:
- Making
owner_scope_ida required constructor parameter, or - Using
Option<String>with explicit None-handling to force callers to bind ownership
Non-blocking
- Duplicate index in libSQL migration 13:
idx_routines_event_triggersandidx_routines_userboth indexroutines(user_id). The event triggers index should probably be a composite index (e.g.,(user_id, trigger_type)). - Telegram
thread_idbehavioral change:thread_idchanged fromNonetoSome(chat_id). Confirm conversation dedup and history lookup work correctly with this -- previously Telegram messages had no thread_id. "default"owner warning: IfIRONCLAW_OWNER_ID=defaultis set explicitly, it silently gets legacy behavior. Consider logging a warning.- E2E
_find_distinct_free_portsTOCTOU race: Known limitation, acknowledged by author.
Security
- Owner-vs-guest model is correct:
resolve_message_scope()checksowner_actor_id == sender_id. Non-owners remain guest-scoped. uses_owner_broadcast_target()is a simple equality check -- no"default"bypass vulnerability.- Migrations correctly normalize
"default"to NULL in both backends. - No injection risks or sensitive data logging issues identified.
henrypark133
force-pushed
the
codex/owner-scope-refactor
branch
from
March 16, 2026 15:21
4c4852e to
6ebf011
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors IronClaw’s identity model to separate durable owner-scoped persistence from per-channel sender/conversation scope, and removes the legacy "default" notify routing sentinel by making routine notification targets nullable and resolved at send time.
Changes:
- Introduces explicit
owner_idplumbing across config, channels, job context, and agent loop (owner vs sender vs conversation scope). - Updates routine notification behavior (
notify_usernullable; resolve last-seen owner routing target; avoid sending to"default"). - Adds/updates integration + E2E coverage for owner scoping across HTTP/web/REPL/WASM/Telegram and routine/job flows.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/wasm_channel_integration.rs | Updates WASM channel construction for new owner-scope parameter. |
| tests/telegram_auth_integration.rs | Strengthens Telegram owner/guest behavior tests and thread scoping expectations. |
| tests/support/test_rig.rs | Passes owner_id into AgentDeps in test rig. |
| tests/support/gateway_workflow_harness.rs | Passes owner_id into AgentDeps in gateway harness. |
| tests/e2e_routine_heartbeat.rs | Updates message fixtures for new IncomingMessage fields and adds owner/guest trigger regression. |
| tests/e2e/scenarios/test_owner_scope.py | New E2E scenarios validating owner-global routines/jobs across web + HTTP sender contexts. |
| tests/e2e/mock_llm.py | Adds mock LLM tool patterns for routine create/list used by E2E owner-scope scenarios. |
| tests/e2e/helpers.py | Adds HTTP webhook signing helper and new UI selectors for routines/jobs. |
| tests/e2e/conftest.py | Configures owner id + HTTP webhook channel for E2E runs; reserves ports. |
| src/tools/wasm/wrapper.rs | Resolves host credentials using job context scope; adds tests around owner-scoped credential lookup. |
| src/tools/builtin/routine.rs | Makes notify_user optional and stops defaulting to "default". |
| src/tools/builtin/message.rs | Adds owner-scope fallback target resolution when channel is known; adds regression test. |
| src/testing/mod.rs | Updates test harness deps and NotifyConfig option type. |
| src/setup/wizard.rs | Refactors wizard bootstrap owner scope + backend-specific DB/secrets handling and model discovery helpers. |
| src/settings.rs | Adds bootstrap owner_id field and ensures it’s excluded from DB settings map serialization. |
| src/main.rs | Wires owner_id into REPL, webhooks, agent deps, and SIGHUP config reload paths. |
| src/history/store.rs | Extends loaded job context shape with requester_id field (currently None from store). |
| src/extensions/manager.rs | Binds loaded WASM channels to an optional per-channel owner actor id. |
| src/error.rs | Adds structured ChannelError::MissingRoutingTarget for routing-aware fallback decisions. |
| src/db/libsql_migrations.rs | Makes routines.notify_user nullable and adds migration to normalize legacy 'default'; improves routine trigger index. |
| src/db/libsql/routines.rs | Persists nullable notify_user in libSQL routine store. |
| src/db/libsql/mod.rs | Adds normalize_notify_user() and applies it when reading routines. |
| src/db/libsql/jobs.rs | Extends loaded job context shape with requester_id field (currently None from store). |
| src/context/state.rs | Adds requester_id to JobContext and a builder setter. |
| src/config/mod.rs | Adds bootstrap settings loader + resolve_owner_id(); adds Config.owner_id; updates channels resolve call signature. |
| src/config/channels.rs | Refactors channels config resolution to use owner_id and owner-aware channel IDs. |
| src/cli/routines.rs | Makes CLI-created routine notify target default to runtime resolution (user: None). |
| src/cli/doctor.rs | Uses shared resolve_owner_id() before resolving channels config. |
| src/channels/wasm/wrapper.rs | Adds owner scope + owner actor binding; owner-scoped broadcast metadata storage and routing target resolution. |
| src/channels/wasm/setup.rs | Injects owner scope into WASM loader and secrets lookup; binds owner actor id per channel. |
| src/channels/wasm/router.rs | Updates test helper to pass owner-scope argument to WASM channel constructor. |
| src/channels/wasm/mod.rs | Updates module docs to reflect new loader constructor signature. |
| src/channels/wasm/loader.rs | Stores owner_scope_id on loader and passes it into constructed channels; updates tests. |
| src/channels/repl.rs | Adds REPL user_id/owner-scope binding (including single-message mode). |
| src/channels/mod.rs | Re-exports routing_target_from_metadata. |
| src/channels/http.rs | Treats request user_id as sender_id (trimmed), keeps owner scope fixed, and adds regressions. |
| src/channels/channel.rs | Extends IncomingMessage with owner_id, sender_id, and conversation_scope_id; adds routing_target helpers. |
| src/app.rs | Re-keys bootstrap/DB config, session attach, MCP loading, workspace scope, and secrets injection to owner_id. |
| src/agent/thread_ops.rs | Sets requester_id in JobContext for approval tool execution. |
| src/agent/routine_engine.rs | Filters event triggers by message scope; includes owner_id in routine notification metadata and full-job metadata. |
| src/agent/routine.rs | Makes NotifyConfig user target optional (nullable notify). |
| src/agent/heartbeat.rs | Defaults heartbeat notify user_id to workspace owner scope rather than "default". |
| src/agent/dispatcher.rs | Sets requester_id in JobContext for interactive chat tool execution. |
| src/agent/commands.rs | Persists selected model under resolved owner_id scope. |
| src/agent/agent_loop.rs | Adds owner_id to AgentDeps; adds structured routine notification target resolution and routing fallback behavior; uses conversation_scope(). |
| migrations/V6__routines.sql | Updates fresh schema to make notify_user nullable. |
| migrations/V13__owner_scope_notify_targets.sql | New migration to drop NOT NULL/DEFAULT and normalize 'default' → NULL. |
| channels-src/telegram/src/lib.rs | Updates Telegram WASM channel behavior for owner/guest authorization and thread scoping; removes topic-thread routing fields. |
| FEATURE_PARITY.md | Updates parity notes to reflect explicit owner scoping and routing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zmanian
reviewed
Mar 16, 2026
zmanian
left a comment
zmanian
left a comment
Collaborator
There was a problem hiding this comment.
This is a large, foundational refactor (6200+ lines) introducing explicit owner identity (IRONCLAW_OWNER_ID) across the entire system. CI is green. The PR description and reviewer guide are excellent -- one of the most thorough I've seen.
Assessment:
This is clearly Track C (security/runtime/DB) and deserves careful review. Key observations:
1. The owner model is well-designed
The separation of owner_id / sender_id / conversation_scope_id is the right abstraction. The fallback chain (env var -> saved settings -> "default") is reasonable.
2. Database migration V13
Converting notify_user = 'default' to NULL is a one-way migration. The rollback plan correctly notes that downgrade requires backfilling. This is acceptable for a Track C change.
3. Security boundary concern
The PR touches owner-vs-guest authorization boundaries. The description says "Telegram/WASM only gets owner-global access when the configured owner actor matches; other senders remain guests." This is critical -- please ensure there are tests that verify a non-owner Telegram sender cannot access owner-scoped routines, credentials, or secrets.
4. Blast radius
Touching channel identity plumbing, routine execution, message tool fallback, settings/secrets lookup, and Telegram routing in one PR is high risk. The E2E test coverage (test_owner_scope.py) helps, but the manual testing checkbox is unchecked.
Recommendation: This needs dedicated review time given its scope. The design is sound but the security implications of getting the owner/guest boundary wrong are significant. Would benefit from a focused security review of the authorization boundaries.
zmanian
approved these changes
Mar 16, 2026
zmanian
left a comment
zmanian
left a comment
Collaborator
There was a problem hiding this comment.
Re-Review: APPROVE
Both previously requested changes addressed in 2f474c7.
I1: Credential resolution test -- RESOLVED
Two well-designed tests added:
test_resolve_host_credentials_owner_scope_bearer-- verifies credential lookup under owner scopetest_execute_resolves_host_credentials_from_owner_scope_context-- usesRecordingSecretsStoreto capture allget_decryptedcalls and explicitly asserts lookup does NOT use"default". This is the right pattern -- it catches the silent credential miss failure mode directly.
I2: Agent::owner_id() double source of truth -- RESOLVED
owner_id()now returns&self.deps.owner_idas the canonical sourcedebug_assert_eq!verifiesworkspace.user_id()stays aligned in debug builds- Clear assert message for future debugging
Note
This PR overlaps with #1211 on event trigger user_id filtering. Whichever merges second should reconcile to avoid duplicate checks in check_event_triggers().
CI all green. Ready to merge.
This was referenced Mar 16, 2026
This was referenced Mar 16, 2026
Merged
bkutasi
pushed a commit
to bkutasi/ironclaw
that referenced
this pull request
Mar 28, 2026
…nearai#1151) * refactor: add explicit owner scope across channels * fix: tighten routine owner target routing * fix: address owner scope review feedback * Fix owner-scope onboarding and event trigger isolation * Tighten routing fallback and wizard owner validation * fix: address owner-scope follow-up review * fix: tighten owner-scope follow-up details * fix: import Channel trait in telegram test * fix: normalize http webhook sender ids * fix: address remaining owner-scope review issues * fix: reconcile config rebase fallout * fix: reconcile extension manager rebase drift * fix: address current copilot review regressions * fix: restore clippy matrix after rebase
|
@henrypark133 @zmanian This change broke all updates from 0.18.0 (and earlier releases), see #1328 |
ilblackdragon
added a commit
that referenced
this pull request
Apr 7, 2026
…on (#1328) PR #1151 modified the already-released migrations/V6__routines.sql in place, causing refinery's checksum validation to abort startup on every existing PostgreSQL deployment upgrading to v0.19.0. Revert V6 to its v0.18.0 content (V13 already applies the schema change incrementally and is idempotent for fresh installs that received the modified V6). Add a runtime checksum realignment step that rewrites refinery_schema_history rows whose stored checksum disagrees with the embedded SQL — this handles both populations of databases in the wild (pre-#1151 originals and post-#1151 fresh installs). Add migrations/checksums.lock pinning every migration's SipHasher13 checksum and a `released_migrations_are_immutable` cargo test that fails if any migration is modified or added without a matching lockfile entry. A second hard-coded sentinel test pins V6's literal v0.18.0 checksum so the guard cannot be defeated by editing both the migration and the lockfile in the same commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
ilblackdragon
added a commit
that referenced
this pull request
Apr 7, 2026
Per @serrrfirat's review, the previous IS DISTINCT FROM-based realignment would silently rewrite *any* non-canonical V6 checksum, masking unrelated corruption or manual tampering instead of narrowly exempting the one historical released mismatch. Add a `known_bad_checksums: &'static [u64]` field to `KnownDivergence` listing the exact historical bad value(s), and rewrite only rows whose stored checksum is in that whitelist via `WHERE checksum = ANY($4)`. Anything else is left alone so refinery still aborts startup loudly. The single known-bad V6 value (`11230857244097235596`) is the SipHasher13 of `git show 878a67c:migrations/V6__routines.sql` (the post-#1151 content) and is pinned by a new sentinel test `v6_known_bad_checksum_matches_post_1151_content` so the whitelist cannot drift or be silently widened. Also adds an ignored bootstrap helper `compute_checksum_for_external_file` for computing checksums of external SQL files when adding future entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
drchirag1991
pushed a commit
to drchirag1991/ironclaw
that referenced
this pull request
Apr 8, 2026
…nearai#1151) * refactor: add explicit owner scope across channels * fix: tighten routine owner target routing * fix: address owner scope review feedback * Fix owner-scope onboarding and event trigger isolation * Tighten routing fallback and wizard owner validation * fix: address owner-scope follow-up review * fix: tighten owner-scope follow-up details * fix: import Channel trait in telegram test * fix: normalize http webhook sender ids * fix: address remaining owner-scope review issues * fix: reconcile config rebase fallout * fix: reconcile extension manager rebase drift * fix: address current copilot review regressions * fix: restore clippy matrix after rebase
ilblackdragon
added a commit
that referenced
this pull request
Apr 9, 2026
…on (#1328) (#2101) * fix(db): repair V6 migration checksum and guard against re-modification (#1328) PR #1151 modified the already-released migrations/V6__routines.sql in place, causing refinery's checksum validation to abort startup on every existing PostgreSQL deployment upgrading to v0.19.0. Revert V6 to its v0.18.0 content (V13 already applies the schema change incrementally and is idempotent for fresh installs that received the modified V6). Add a runtime checksum realignment step that rewrites refinery_schema_history rows whose stored checksum disagrees with the embedded SQL — this handles both populations of databases in the wild (pre-#1151 originals and post-#1151 fresh installs). Add migrations/checksums.lock pinning every migration's SipHasher13 checksum and a `released_migrations_are_immutable` cargo test that fails if any migration is modified or added without a matching lockfile entry. A second hard-coded sentinel test pins V6's literal v0.18.0 checksum so the guard cannot be defeated by editing both the migration and the lockfile in the same commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): address review feedback on migration_fixup (#2101) - Drop hard-coded `public.` schema qualifier from the existence probe so PostgreSQL resolves `refinery_schema_history` via the active search_path, matching how refinery itself locates the table and how the subsequent UPDATE statement is written. Without this, deployments using a non-default schema would silently skip the realignment. - Use `IS DISTINCT FROM` instead of `<>` so a corrupted row with a NULL checksum is repaired rather than silently skipped. - Add `explanation` field to `KnownDivergence` and use it in the realignment warning so future entries are not coupled to the V6/#1328 wording. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): narrowly whitelist V6 known-bad checksum (#2101) Per @serrrfirat's review, the previous IS DISTINCT FROM-based realignment would silently rewrite *any* non-canonical V6 checksum, masking unrelated corruption or manual tampering instead of narrowly exempting the one historical released mismatch. Add a `known_bad_checksums: &'static [u64]` field to `KnownDivergence` listing the exact historical bad value(s), and rewrite only rows whose stored checksum is in that whitelist via `WHERE checksum = ANY($4)`. Anything else is left alone so refinery still aborts startup loudly. The single known-bad V6 value (`11230857244097235596`) is the SipHasher13 of `git show 878a67c:migrations/V6__routines.sql` (the post-#1151 content) and is pinned by a new sentinel test `v6_known_bad_checksum_matches_post_1151_content` so the whitelist cannot drift or be silently widened. Also adds an ignored bootstrap helper `compute_checksum_for_external_file` for computing checksums of external SQL files when adding future entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): assert in release + add postgres integration test (#2101) Address two follow-up review comments from @serrrfirat: 1. The defensive `debug_assert!` guarding against the canonical checksum being listed in `known_bad_checksums` is stripped in release builds, so the safety net was absent in production. `KNOWN_DIVERGENCES` has at most a handful of entries — switch to `assert!` so the guard runs in release too. Cost is one constant- time slice lookup per startup. 2. The `realign_diverged_checksums` SQL path was never exercised against a real database. Refactor into a thin pub wrapper plus an injectable `realign_diverged_checksums_with` inner helper, and add a `#[cfg(feature = "integration")]` test that: - skips gracefully if no DATABASE_URL is reachable - creates `refinery_schema_history` if missing - seeds a synthetic V99999 row with a deliberately-wrong checksum - calls the realignment with a custom divergence list (no collision with real V6 rows in shared CI databases) - asserts the row now holds the canonical checksum - re-runs the realignment and asserts a no-op Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): replace assert! with returned error to satisfy no-panics check (#2101) The previous commit changed `debug_assert!` → `assert!` to keep the canonical-in-known-bad-list guard active in release builds, but this trips the project's "No panics in production code" CI check (the regex matches `assert!` outside test attributes). Replace with an early `return Err(DatabaseError::Migration(...))` so the guard still runs in release builds — startup refuses to proceed with a misconfigured `KNOWN_DIVERGENCES` table — without using a panicking macro. This is also more idiomatic for a function that already returns `Result`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): address review follow-ups on migration_fixup (#2101) - parse_lockfile() now panics on duplicate migration keys instead of silently overwriting earlier entries — a stray duplicate could mask the actual pinned checksum and weaken the immutability guard (Copilot review). - Add `rejects_canonical_in_known_bad_checksums` integration test exercising the defensive Err path that refuses startup when a KnownDivergence has its canonical checksum listed in its own known_bad_checksums list (serrrfirat review). - Document why `tracing::warn!` is intentional in the realignment fix-up despite CLAUDE.md's warning about info!/warn! corrupting the TUI: this code runs at startup before any channel/REPL/TUI is initialized, so terminal-rendering interference is impossible. If the call site ever moves later in startup, downgrade to debug! or pre-buffer (illblackdragon review). - Cross-reference comments in src/history/store.rs and src/setup/wizard.rs pointing each other out so future changes to the migration fix-up call site stay in sync (illblackdragon review). - Sort migrations/checksums.lock by parsed migration version (V1, V2, ..., V10, V11, ...) instead of lex order (V10 before V2). The resulting file reads in numeric order which makes review diffs easier to scan (illblackdragon review). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): consolidate migration entry points + advisory lock + no leaks (#2101) Address three Medium-severity findings from @serrrfirat's review: 1. **Duplicate call sites** — extract `run_postgres_migrations_with_fixup(client)` in `crate::db::migration_fixup` that bundles fix-up + refinery into a single function. Both `Store::run_migrations` and `SetupWizard::run_migrations_postgres` now call it. Eliminates the class of bug where a future entry point could forget the fix-up. The previous comment-based coupling was an interim measure. 2. **Concurrent startup race** — the new helper acquires `pg_advisory_lock(1328)` (issue number, easy to grep in `pg_locks`) before realignment and releases it after refinery returns, on every exit path including errors. Serializes concurrent migration runs across replicas — also hardens the pre-existing refinery race that has always existed for multi-replica starts. Uses session-level advisory lock (not `pg_advisory_xact_lock`) because refinery's `run_async` opens its own internal transactions. 3. **`Box::leak` in tests** — refactor `KnownDivergence` to be lifetime-generic (`KnownDivergence<'a>`). Production `KNOWN_DIVERGENCES` is `&[KnownDivergence<'static>]` — no external API change. Both integration tests now use stack-allocated `&[u64]` slices, no `Box::leak`. Removes the leak-sanitizer false positive. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…on (nearai#1328) (nearai#2101) * fix(db): repair V6 migration checksum and guard against re-modification (nearai#1328) PR nearai#1151 modified the already-released migrations/V6__routines.sql in place, causing refinery's checksum validation to abort startup on every existing PostgreSQL deployment upgrading to v0.19.0. Revert V6 to its v0.18.0 content (V13 already applies the schema change incrementally and is idempotent for fresh installs that received the modified V6). Add a runtime checksum realignment step that rewrites refinery_schema_history rows whose stored checksum disagrees with the embedded SQL — this handles both populations of databases in the wild (pre-nearai#1151 originals and post-nearai#1151 fresh installs). Add migrations/checksums.lock pinning every migration's SipHasher13 checksum and a `released_migrations_are_immutable` cargo test that fails if any migration is modified or added without a matching lockfile entry. A second hard-coded sentinel test pins V6's literal v0.18.0 checksum so the guard cannot be defeated by editing both the migration and the lockfile in the same commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): address review feedback on migration_fixup (nearai#2101) - Drop hard-coded `public.` schema qualifier from the existence probe so PostgreSQL resolves `refinery_schema_history` via the active search_path, matching how refinery itself locates the table and how the subsequent UPDATE statement is written. Without this, deployments using a non-default schema would silently skip the realignment. - Use `IS DISTINCT FROM` instead of `<>` so a corrupted row with a NULL checksum is repaired rather than silently skipped. - Add `explanation` field to `KnownDivergence` and use it in the realignment warning so future entries are not coupled to the V6/nearai#1328 wording. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): narrowly whitelist V6 known-bad checksum (nearai#2101) Per @serrrfirat's review, the previous IS DISTINCT FROM-based realignment would silently rewrite *any* non-canonical V6 checksum, masking unrelated corruption or manual tampering instead of narrowly exempting the one historical released mismatch. Add a `known_bad_checksums: &'static [u64]` field to `KnownDivergence` listing the exact historical bad value(s), and rewrite only rows whose stored checksum is in that whitelist via `WHERE checksum = ANY($4)`. Anything else is left alone so refinery still aborts startup loudly. The single known-bad V6 value (`11230857244097235596`) is the SipHasher13 of `git show 958a747:migrations/V6__routines.sql` (the post-nearai#1151 content) and is pinned by a new sentinel test `v6_known_bad_checksum_matches_post_1151_content` so the whitelist cannot drift or be silently widened. Also adds an ignored bootstrap helper `compute_checksum_for_external_file` for computing checksums of external SQL files when adding future entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): assert in release + add postgres integration test (nearai#2101) Address two follow-up review comments from @serrrfirat: 1. The defensive `debug_assert!` guarding against the canonical checksum being listed in `known_bad_checksums` is stripped in release builds, so the safety net was absent in production. `KNOWN_DIVERGENCES` has at most a handful of entries — switch to `assert!` so the guard runs in release too. Cost is one constant- time slice lookup per startup. 2. The `realign_diverged_checksums` SQL path was never exercised against a real database. Refactor into a thin pub wrapper plus an injectable `realign_diverged_checksums_with` inner helper, and add a `#[cfg(feature = "integration")]` test that: - skips gracefully if no DATABASE_URL is reachable - creates `refinery_schema_history` if missing - seeds a synthetic V99999 row with a deliberately-wrong checksum - calls the realignment with a custom divergence list (no collision with real V6 rows in shared CI databases) - asserts the row now holds the canonical checksum - re-runs the realignment and asserts a no-op Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): replace assert! with returned error to satisfy no-panics check (nearai#2101) The previous commit changed `debug_assert!` → `assert!` to keep the canonical-in-known-bad-list guard active in release builds, but this trips the project's "No panics in production code" CI check (the regex matches `assert!` outside test attributes). Replace with an early `return Err(DatabaseError::Migration(...))` so the guard still runs in release builds — startup refuses to proceed with a misconfigured `KNOWN_DIVERGENCES` table — without using a panicking macro. This is also more idiomatic for a function that already returns `Result`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): address review follow-ups on migration_fixup (nearai#2101) - parse_lockfile() now panics on duplicate migration keys instead of silently overwriting earlier entries — a stray duplicate could mask the actual pinned checksum and weaken the immutability guard (Copilot review). - Add `rejects_canonical_in_known_bad_checksums` integration test exercising the defensive Err path that refuses startup when a KnownDivergence has its canonical checksum listed in its own known_bad_checksums list (serrrfirat review). - Document why `tracing::warn!` is intentional in the realignment fix-up despite CLAUDE.md's warning about info!/warn! corrupting the TUI: this code runs at startup before any channel/REPL/TUI is initialized, so terminal-rendering interference is impossible. If the call site ever moves later in startup, downgrade to debug! or pre-buffer (illblackdragon review). - Cross-reference comments in src/history/store.rs and src/setup/wizard.rs pointing each other out so future changes to the migration fix-up call site stay in sync (illblackdragon review). - Sort migrations/checksums.lock by parsed migration version (V1, V2, ..., V10, V11, ...) instead of lex order (V10 before V2). The resulting file reads in numeric order which makes review diffs easier to scan (illblackdragon review). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): consolidate migration entry points + advisory lock + no leaks (nearai#2101) Address three Medium-severity findings from @serrrfirat's review: 1. **Duplicate call sites** — extract `run_postgres_migrations_with_fixup(client)` in `crate::db::migration_fixup` that bundles fix-up + refinery into a single function. Both `Store::run_migrations` and `SetupWizard::run_migrations_postgres` now call it. Eliminates the class of bug where a future entry point could forget the fix-up. The previous comment-based coupling was an interim measure. 2. **Concurrent startup race** — the new helper acquires `pg_advisory_lock(1328)` (issue number, easy to grep in `pg_locks`) before realignment and releases it after refinery returns, on every exit path including errors. Serializes concurrent migration runs across replicas — also hardens the pre-existing refinery race that has always existed for multi-replica starts. Uses session-level advisory lock (not `pg_advisory_xact_lock`) because refinery's `run_async` opens its own internal transactions. 3. **`Box::leak` in tests** — refactor `KnownDivergence` to be lifetime-generic (`KnownDivergence<'a>`). Production `KNOWN_DIVERGENCES` is `&[KnownDivergence<'static>]` — no external API change. Both integration tests now use stack-allocated `&[u64]` slices, no `Box::leak`. Removes the leak-sanitizer false positive. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This refactor makes IronClaw treat persistent state as belonging to one explicit instance owner instead of implicitly reusing whatever channel sender ID happened to arrive with a message.
That fixes the cross-channel inconsistency behind #994 and related issues where data created from one frontend could appear missing from another because some parts of the system stored state under the instance owner while other parts stored it under a channel-specific sender/chat identity.
Concretely, this PR:
IRONCLAW_OWNER_ID"default"as the stored routine/proactive delivery target sentinel, while preserving legacy owner broadcast metadata compatibility during transitionWhy This Change
Before this PR,
user_idhad two conflicting meanings depending on where a request came from:That worked accidentally for some flows and failed badly for others. The most visible symptom was that routines, credentials, and other persistent state could look channel-local even though IronClaw is conceptually a single-user instance.
This PR establishes a cleaner model:
owner_id: who owns persistent instance statesender_id: who sent the inbound message on that channelconversation_scope_id: which thread/chat/session this interaction belongs toSingle-user IronClaw now becomes the simple case of one durable owner with many possible channel frontends.
What Changed
1. Config and runtime identity
IRONCLAW_OWNER_IDconfig resolution, falling back to saved settings and then"default"only as the configured owner identityowner_id,sender_id, andconversation_scope_idrequester_idtoJobContextso job execution can distinguish the storage owner from the actor who initiated the request2. Owner-scoped persistence
These now consistently use owner scope across the owner-capable channels covered in this PR:
These remain channel/thread scoped:
3. Channel behavior
This PR fully wires the owner model for:
Important behavior changes:
Signal and relay are intentionally not made owner-capable in this pass; they keep current behavior until they get explicit owner-actor configuration.
4. Fix for
"default"routing sentinel (#994)Previously, some proactive sends treated
"default"as if it were an actual delivery target. That caused broken routing, especially for Telegram, where a real numericchat_idwas required.This PR changes that model:
NotifyConfig.useris now optional instead of sentinel-drivenroutines.notify_useris nullable in the databasenotify_user = 'default'values are normalized toNULLnotify_user = NULLunless the caller supplied a real explicit targetbroadcast_all"default""default"is still read during transition for backwards compatibilityThis is the core fix for the routine notification issue in #994.
5. Tests
Added regression coverage for:
"default"broadcast compatibilityAdded new Playwright/e2e coverage for:
Reviewer Guide
Suggested review order:
src/config/mod.rssrc/channels/channel.rssrc/context/state.rs"default"cleanupsrc/agent/agent_loop.rssrc/cli/routines.rssrc/channels/wasm/wrapper.rssrc/tools/builtin/message.rssrc/agent/routine.rssrc/db/libsql/mod.rschannels-src/telegram/src/lib.rssrc/channels/http.rssrc/main.rssrc/app.rsmigrations/V13__owner_scope_notify_targets.sqltests/e2e/scenarios/test_owner_scope.pyChange Type
Linked Issue
Closes #994
Validation
cargo fmt --all --checkbash scripts/pre-commit-safety.shcargo clippy --all-features --all-targets -- -D warningscargo clippy --all-targets -- -D warningscargo clippy --no-default-features --features libsql --all-targets -- -D warningscargo test -q resolve_routine_notification_usercargo test -q cli_notify_config_defaults_to_runtime_target_resolutioncargo test -q normalize_notify_user_treats_legacy_default_as_missingcargo test -q dispatch_emitted_messagescargo test -quv run --project tests/e2e python -m pytest tests/e2e/scenarios/test_owner_scope.py -quv run --project tests/e2e python -m pytest tests/e2e/scenarios/test_chat.py tests/e2e/scenarios/test_tool_execution.py tests/e2e/scenarios/test_owner_scope.py -qSecurity Impact
Touches owner-vs-guest authorization and channel routing resolution for HTTP, Telegram, and WASM. No new external destinations or permissions were added. The primary behavior change is that owner-scoped persistence is now explicit, while non-owner external senders remain isolated to guest-scoped conversations and routing state.
Database Impact
Adds
migrations/V13__owner_scope_notify_targets.sqland updates fresh-schemamigrations/V6__routines.sqlsoroutines.notify_usercan beNULL.Migration behavior:
notify_user = 'default'rows toNULL"default"during transitionBlast Radius
Touches:
Main risks:
Rollback Plan
V13__owner_scope_notify_targets.sqlhas already been applied, backfillroutines.notify_userfromNULLto"default"before running older binariesReview track: C