feat(config): session scope + channel command policy + tool profiles#2158
Merged
feat(config): session scope + channel command policy + tool profiles#2158
Conversation
Contributor
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Documentation CHANGELOG.md |
Added explicit backend/strategy/TTL key names for agent session persistence rollout notes in the Unreleased/Added section. |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~2 minutes
Suggested labels
channel, tool: core
Suggested reviewers
- theonlyhennygod
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 68.42% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Description check | ❓ Inconclusive | The description covers most required sections including summary, validation evidence, and linear issue references, but misses several template sections like label snapshot, risk/size/scope labels, security impact, and rollback plan. | Complete the description by filling in the missing sections from the template, particularly label snapshot, security impact assessment, compatibility notes, and rollback plan. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly describes the main changes: session scope configuration, channel command policy, and tool profiles. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/session-scope-config
Comment @coderabbitai help to get the list of available commands and usage tips.
theonlyhennygod
approved these changes
Feb 28, 2026
Collaborator
theonlyhennygod
left a comment
theonlyhennygod
left a comment
There was a problem hiding this comment.
Approved per maintainer request; merging if conflict-free.
github-actions
Bot
added
docs
PR intake checks found warnings (non-blocking)Fast safe checks found advisory issues. CI lint/test/build gates still enforce merge quality.
Action items:
Detected Linear keys: RMN-205, RMN-206, RMN-207 Run logs: https://github.com/zeroclaw-labs/zeroclaw/actions/runs/22544895378 Detected blocking line issues (sample):
Detected advisory line issues (sample):
|
theonlyhennygod
force-pushed
the
feat/session-scope-config
branch
from
ba74840 to
e3c2121
Compare
github-actions
Bot
added
config
theonlyhennygod
force-pushed
the
feat/session-scope-config
branch
from
e3c2121 to
18adc1c
Compare
github-actions
Bot
added
config
Collaborator
|
Update complete: I rebased this branch onto the latest |
github-actions
Bot
removed
config
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/tools/mod.rs (1)
798-803: Use the shared skills-directory helper to avoid path drift.This can use
crate::skills::skills_dir(workspace_dir)instead of duplicatingworkspace_dir.join("skills").♻️ Proposed refactor
- let skills_dir = workspace_dir.join("skills"); + let skills_dir = crate::skills::skills_dir(workspace_dir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/mod.rs` around lines 798 - 803, Replace the local construction of the skills directory with the shared helper to avoid path drift: change the variable `skills_dir` (used when calling `wasm_tool::load_wasm_tools_from_skills`) to be produced by `crate::skills::skills_dir(workspace_dir)` instead of `workspace_dir.join("skills")`; keep the surrounding flow (`let mut boxed = boxed_registry_from_arcs(tool_arcs);`, `boxed.extend(...)`, and `apply_tool_profile_filter(boxed, &root_config.tools)`) intact so only the source of `skills_dir` is swapped to the helper.src/config/mod.rs (1)
9-25: Add a compile-time regression test for the newly re-exported types.This block expands public API surface, but there’s no focused test asserting these new re-exports remain reachable. A small type-reachability test would harden compatibility.
Suggested test addition
#[cfg(test)] mod tests { use super::*; @@ fn reexported_http_request_credential_profile_is_constructible() { let profile = HttpRequestCredentialProfile { header_name: "Authorization".into(), env_var: "OPENROUTER_API_KEY".into(), value_prefix: "Bearer ".into(), }; @@ assert_eq!(profile.value_prefix, "Bearer "); } + + #[test] + fn reexported_session_tools_and_channel_policy_types_are_reachable() { + fn assert_type<T>() {} + + assert_type::<ChannelCommandPolicyConfig>(); + assert_type::<SessionConfig>(); + assert_type::<SessionDmScope>(); + assert_type::<SessionHistoryScope>(); + assert_type::<ToolProfilePreset>(); + assert_type::<ToolsConfig>(); + } }Based on learnings: Treat config/schema as user-facing API with backward compatibility and explicit migration mattering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/mod.rs` around lines 9 - 25, Add a compile-time type-reachability test that references the newly re-exported public types (e.g., Config, RuntimeConfig, PluginsConfig, StorageConfig, ModelRouteConfig, ProviderConfig, SandboxConfig) to ensure they stay accessible from the crate root; create a new test file (under tests/ or a cfg(test) module) that simply references each type in a no-op way (e.g., binding to Option<Type> or using let _: fn() -> Type;) so the compiler will fail if any re-export is removed or renamed, and run cargo test to verify it passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/channels/mod.rs`:
- Around line 223-230: The ConfirmToolApproval path currently persists approvals
despite config_write_capable() excluding
ChannelRuntimeCommand::ConfirmToolApproval, so fix by ensuring the confirm flow
checks the same gate before writing: either add
ChannelRuntimeCommand::ConfirmToolApproval to the match in
config_write_capable() or, in the handler that persists approvals (the
ConfirmToolApproval handling code near the approval persistence block),
early-return or error when !self.config_write_capable(); make sure the
persistence call that saves approvals is behind that gate so channels with
config_writes=false cannot be modified.
- Around line 2090-2115: The policy is currently checked by
runtime_command_policy_check before a RequestToolApproval can be promoted into
an ApproveTool (direct mode), allowing a post-check escalation; update the flow
so that after any mutation/promotion from RequestToolApproval to ApproveTool
(the code that performs the promotion in the block around the
RequestToolApproval -> ApproveTool transition), you re-run
runtime_command_policy_check against the promoted command and the same
source_channel/command_policy, and if it fails return/deny and send the same
denied response (same runtime_trace and SendMessage logic used at the original
check); alternatively, prevent promoting RequestToolApproval into ApproveTool
when the original pre-promotion check passed only for lower-privileged
forms—ensure the symbols runtime_command_policy_check, RequestToolApproval, and
ApproveTool are used to locate and apply the fix.
In `@src/config/schema.rs`:
- Around line 6840-6874: The current validation normalizes and deduplicates
self.tools.enable and self.tools.disable separately (using seen_tools_enable and
seen_tools_disable) but does not reject when the same tool appears in both
lists; add a conflict check after normalization (or during insertion) to detect
overlaps by comparing the lowercase normalized names between the two sets (e.g.,
check if normalized.to_ascii_lowercase() is already present in the opposite set)
and call anyhow::bail! with a clear message like "tools.enable and tools.disable
contain conflicting entry: {normalized}" so that any tool listed in both
self.tools.enable and self.tools.disable is rejected.
- Around line 4006-4014: channels_config.commands currently accepts arbitrary
map keys and defaults that can silently give permissive policies; add fail-fast
validation in Config::validate() to reject unknown/typoed channel IDs and
malformed entries by iterating the HashMap<String, ChannelCommandPolicyConfig>
(channels_config.commands) and checking each key against an allowed channel set
(e.g., "telegram","discord", etc.) and validating ChannelCommandPolicyConfig
fields (ensure allowed command names are in the canonical command list and
config_writes/native are present and typed); on any mismatch or unknown key,
return an explicit error (bail!) describing the bad key/value so loading fails
rather than defaulting to permissive settings.
In `@src/security/leak_detector.rs`:
- Around line 458-459: The test uses content = "secret=mygenericvalue" which is
shorter than the generic-secret regex threshold, so the pattern never matches
and the sensitivity-branch isn't exercised; update the test's content string to
use a 16+ character value that matches the generic secret regex (e.g., a 16+
alphabetic-only token) so the generic-secret detection path is actually hit and
the sensitivity threshold logic (the branch checked after pattern match) is
validated.
In `@src/tools/mod.rs`:
- Around line 183-341: The bug is that built-in config tools like
web_access_config and web_search_config are not included in the known-tool
classification so non-Full profiles can’t restrict them; update
is_known_profile_tool to include "web_access_config" and "web_search_config" and
add those same names to the appropriate profile sets in profile_includes_tool
(e.g., include them in Ops and Research where needed) so
profile_includes_tool(...) and the default_allowed branch behave correctly for
built-in config tools referenced by resolve_active_tool_names_from_profile.
---
Nitpick comments:
In `@src/config/mod.rs`:
- Around line 9-25: Add a compile-time type-reachability test that references
the newly re-exported public types (e.g., Config, RuntimeConfig, PluginsConfig,
StorageConfig, ModelRouteConfig, ProviderConfig, SandboxConfig) to ensure they
stay accessible from the crate root; create a new test file (under tests/ or a
cfg(test) module) that simply references each type in a no-op way (e.g., binding
to Option<Type> or using let _: fn() -> Type;) so the compiler will fail if any
re-export is removed or renamed, and run cargo test to verify it passes.
In `@src/tools/mod.rs`:
- Around line 798-803: Replace the local construction of the skills directory
with the shared helper to avoid path drift: change the variable `skills_dir`
(used when calling `wasm_tool::load_wasm_tools_from_skills`) to be produced by
`crate::skills::skills_dir(workspace_dir)` instead of
`workspace_dir.join("skills")`; keep the surrounding flow (`let mut boxed =
boxed_registry_from_arcs(tool_arcs);`, `boxed.extend(...)`, and
`apply_tool_profile_filter(boxed, &root_config.tools)`) intact so only the
source of `skills_dir` is swapped to the helper.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/config-reference.mdsrc/channels/mod.rssrc/config/mod.rssrc/config/schema.rssrc/onboard/wizard.rssrc/security/leak_detector.rssrc/tools/mod.rs
Comment on lines
+223
to
+230
| fn config_write_capable(&self) -> bool { | ||
| matches!( | ||
| self, | ||
| ChannelRuntimeCommand::ApprovePendingRequest(_) | ||
| | ChannelRuntimeCommand::ApproveTool(_) | ||
| | ChannelRuntimeCommand::UnapproveTool(_) | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
/approve-confirm can bypass config_writes=false and still persist config
Line 223 excludes ConfirmToolApproval from config_write_capable(), but Line 2392 persists approvals during confirm flow. That leaves a write path open when config writes are supposed to be disabled.
🔧 Proposed fix
fn config_write_capable(&self) -> bool {
matches!(
self,
+ ChannelRuntimeCommand::ConfirmToolApproval(_)
+ |
ChannelRuntimeCommand::ApprovePendingRequest(_)
| ChannelRuntimeCommand::ApproveTool(_)
| ChannelRuntimeCommand::UnapproveTool(_)
)
}Risk: unauthorized persistent approvals on channels configured with write-blocking policy. Immediate rollback option: remove /approve-confirm from channel allowed lists until patched.
As per coding guidelines: “Do not silently weaken security policy or access constraints”.
Also applies to: 2365-2403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/channels/mod.rs` around lines 223 - 230, The ConfirmToolApproval path
currently persists approvals despite config_write_capable() excluding
ChannelRuntimeCommand::ConfirmToolApproval, so fix by ensuring the confirm flow
checks the same gate before writing: either add
ChannelRuntimeCommand::ConfirmToolApproval to the match in
config_write_capable() or, in the handler that persists approvals (the
ConfirmToolApproval handling code near the approval persistence block),
early-return or error when !self.config_write_capable(); make sure the
persistence call that saves approvals is behind that gate so channels with
config_writes=false cannot be modified.
Comment on lines
+4006
to
+4014
| /// Runtime command policy by channel key (e.g. `telegram`, `discord`). | ||
| /// | ||
| /// Example: | ||
| /// `[channels_config.commands.telegram]` | ||
| /// `native = true` | ||
| /// `allowed = ["new", "model", "models", "approve"]` | ||
| /// `config_writes = false` | ||
| #[serde(default)] | ||
| pub commands: HashMap<String, ChannelCommandPolicyConfig>, |
Contributor
There was a problem hiding this comment.
channels_config.commands needs fail-fast validation to prevent silent policy bypass
Line 4014 introduces free-form channel keys, but there’s no Config::validate() guard for unknown/typoed channel IDs or malformed allowed entries. A typo (e.g., telegarm) can silently leave that channel on permissive defaults (native=true, config_writes=true, unrestricted commands).
Suggested validation hardening
+fn known_channel_command_keys() -> &'static [&'static str] {
+ &[
+ "cli", "telegram", "discord", "slack", "mattermost", "webhook", "imessage",
+ "matrix", "signal", "whatsapp", "linq", "wati", "nextcloud_talk", "email",
+ "irc", "lark", "feishu", "dingtalk", "qq", "nostr", "clawdtalk",
+ ]
+}
+
// inside Config::validate()
+let mut seen_command_policy_channels = std::collections::HashSet::new();
+for (raw_channel, policy) in &self.channels_config.commands {
+ let channel = raw_channel.trim().to_ascii_lowercase();
+ if channel.is_empty() {
+ anyhow::bail!("channels_config.commands contains an empty channel key");
+ }
+ if !seen_command_policy_channels.insert(channel.clone()) {
+ anyhow::bail!("channels_config.commands contains duplicate channel key: {channel}");
+ }
+ if !known_channel_command_keys().contains(&channel.as_str()) {
+ anyhow::bail!("channels_config.commands contains unsupported channel key: {channel}");
+ }
+
+ let mut seen_allowed = std::collections::HashSet::new();
+ for (idx, raw_cmd) in policy.allowed.iter().enumerate() {
+ let cmd = raw_cmd.trim().trim_start_matches('/').to_ascii_lowercase();
+ if cmd.is_empty() {
+ anyhow::bail!("channels_config.commands.{channel}.allowed[{idx}] must not be empty");
+ }
+ if !seen_allowed.insert(cmd.clone()) {
+ anyhow::bail!("channels_config.commands.{channel}.allowed contains duplicate entry: {cmd}");
+ }
+ }
+}As per coding guidelines “Prefer explicit bail!/errors for unsupported or unsafe states; never silently broaden permissions/capabilities (Fail Fast principle)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/schema.rs` around lines 4006 - 4014, channels_config.commands
currently accepts arbitrary map keys and defaults that can silently give
permissive policies; add fail-fast validation in Config::validate() to reject
unknown/typoed channel IDs and malformed entries by iterating the
HashMap<String, ChannelCommandPolicyConfig> (channels_config.commands) and
checking each key against an allowed channel set (e.g., "telegram","discord",
etc.) and validating ChannelCommandPolicyConfig fields (ensure allowed command
names are in the canonical command list and config_writes/native are present and
typed); on any mismatch or unknown key, return an explicit error (bail!)
describing the bad key/value so loading fails rather than defaulting to
permissive settings.
Comment on lines
+6840
to
+6874
| // Tools | ||
| let mut seen_tools_enable = std::collections::HashSet::new(); | ||
| for (idx, tool_name) in self.tools.enable.iter().enumerate() { | ||
| let normalized = tool_name.trim(); | ||
| if normalized.is_empty() { | ||
| anyhow::bail!("tools.enable[{idx}] must not be empty"); | ||
| } | ||
| if !normalized | ||
| .chars() | ||
| .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.') | ||
| { | ||
| anyhow::bail!("tools.enable[{idx}] contains invalid characters: {normalized}"); | ||
| } | ||
| if !seen_tools_enable.insert(normalized.to_ascii_lowercase()) { | ||
| anyhow::bail!("tools.enable contains duplicate entry: {normalized}"); | ||
| } | ||
| } | ||
|
|
||
| let mut seen_tools_disable = std::collections::HashSet::new(); | ||
| for (idx, tool_name) in self.tools.disable.iter().enumerate() { | ||
| let normalized = tool_name.trim(); | ||
| if normalized.is_empty() { | ||
| anyhow::bail!("tools.disable[{idx}] must not be empty"); | ||
| } | ||
| if !normalized | ||
| .chars() | ||
| .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.') | ||
| { | ||
| anyhow::bail!("tools.disable[{idx}] contains invalid characters: {normalized}"); | ||
| } | ||
| if !seen_tools_disable.insert(normalized.to_ascii_lowercase()) { | ||
| anyhow::bail!("tools.disable contains duplicate entry: {normalized}"); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Reject tools.enable/tools.disable overlaps to avoid ambiguous security intent
Line 6840-6874 validates each list independently but does not reject the same tool in both lists. This can make behavior order-dependent and risky for sensitive tools.
Suggested conflict check
let mut seen_tools_disable = std::collections::HashSet::new();
for (idx, tool_name) in self.tools.disable.iter().enumerate() {
let normalized = tool_name.trim();
if normalized.is_empty() {
anyhow::bail!("tools.disable[{idx}] must not be empty");
}
if !normalized
.chars()
.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.')
{
anyhow::bail!("tools.disable[{idx}] contains invalid characters: {normalized}");
}
if !seen_tools_disable.insert(normalized.to_ascii_lowercase()) {
anyhow::bail!("tools.disable contains duplicate entry: {normalized}");
}
}
+
+ for enabled in &seen_tools_enable {
+ if seen_tools_disable.contains(enabled) {
+ anyhow::bail!(
+ "tools.enable and tools.disable must not both contain: {enabled}"
+ );
+ }
+ }As per coding guidelines “Do not silently weaken security policy or access constraints”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/schema.rs` around lines 6840 - 6874, The current validation
normalizes and deduplicates self.tools.enable and self.tools.disable separately
(using seen_tools_enable and seen_tools_disable) but does not reject when the
same tool appears in both lists; add a conflict check after normalization (or
during insertion) to detect overlaps by comparing the lowercase normalized names
between the two sets (e.g., check if normalized.to_ascii_lowercase() is already
present in the opposite set) and call anyhow::bail! with a clear message like
"tools.enable and tools.disable contain conflicting entry: {normalized}" so that
any tool listed in both self.tools.enable and self.tools.disable is rejected.
Comment on lines
+183
to
+341
| fn is_known_profile_tool(tool_name: &str) -> bool { | ||
| matches!( | ||
| tool_name, | ||
| "shell" | ||
| | "process" | ||
| | "git_operations" | ||
| | "file_read" | ||
| | "file_write" | ||
| | "file_edit" | ||
| | "apply_patch" | ||
| | "glob_search" | ||
| | "content_search" | ||
| | "memory_store" | ||
| | "memory_recall" | ||
| | "memory_forget" | ||
| | "task_plan" | ||
| | "model_routing_config" | ||
| | "proxy_config" | ||
| | "browser_open" | ||
| | "browser" | ||
| | "http_request" | ||
| | "web_fetch" | ||
| | "web_search_tool" | ||
| | "pdf_read" | ||
| | "docx_read" | ||
| | "screenshot" | ||
| | "image_info" | ||
| | "schedule" | ||
| | "pushover" | ||
| | "cron_add" | ||
| | "cron_list" | ||
| | "cron_remove" | ||
| | "cron_update" | ||
| | "cron_run" | ||
| | "cron_runs" | ||
| | "composio" | ||
| | "delegate" | ||
| | "delegate_coordination_status" | ||
| | "subagent_spawn" | ||
| | "subagent_list" | ||
| | "subagent_manage" | ||
| | "agents_list" | ||
| | "agents_send" | ||
| | "agents_inbox" | ||
| | "state_get" | ||
| | "state_set" | ||
| | "wasm_module" | ||
| ) | ||
| } | ||
|
|
||
| fn profile_includes_tool(profile: ToolProfilePreset, tool_name: &str) -> bool { | ||
| match profile { | ||
| ToolProfilePreset::Full => true, | ||
| ToolProfilePreset::Coding => matches!( | ||
| tool_name, | ||
| "shell" | ||
| | "process" | ||
| | "git_operations" | ||
| | "file_read" | ||
| | "file_write" | ||
| | "file_edit" | ||
| | "apply_patch" | ||
| | "glob_search" | ||
| | "content_search" | ||
| | "memory_store" | ||
| | "memory_recall" | ||
| | "memory_forget" | ||
| | "task_plan" | ||
| | "model_routing_config" | ||
| | "proxy_config" | ||
| | "subagent_spawn" | ||
| | "subagent_list" | ||
| | "subagent_manage" | ||
| ), | ||
| ToolProfilePreset::Research => matches!( | ||
| tool_name, | ||
| "browser_open" | ||
| | "browser" | ||
| | "http_request" | ||
| | "web_fetch" | ||
| | "web_search_tool" | ||
| | "pdf_read" | ||
| | "docx_read" | ||
| | "screenshot" | ||
| | "image_info" | ||
| | "file_read" | ||
| | "content_search" | ||
| | "memory_store" | ||
| | "memory_recall" | ||
| | "memory_forget" | ||
| | "task_plan" | ||
| ), | ||
| ToolProfilePreset::Ops => matches!( | ||
| tool_name, | ||
| "cron_add" | ||
| | "cron_list" | ||
| | "cron_remove" | ||
| | "cron_update" | ||
| | "cron_run" | ||
| | "cron_runs" | ||
| | "schedule" | ||
| | "pushover" | ||
| | "proxy_config" | ||
| | "model_routing_config" | ||
| | "shell" | ||
| | "process" | ||
| | "git_operations" | ||
| | "http_request" | ||
| | "web_fetch" | ||
| | "memory_store" | ||
| | "memory_recall" | ||
| | "task_plan" | ||
| | "agents_list" | ||
| | "agents_send" | ||
| | "agents_inbox" | ||
| | "state_get" | ||
| | "state_set" | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| fn normalized_tool_override_set(entries: &[String]) -> BTreeSet<String> { | ||
| entries | ||
| .iter() | ||
| .map(|value| normalize_tool_name(value)) | ||
| .filter(|value| !value.is_empty()) | ||
| .collect() | ||
| } | ||
|
|
||
| fn resolve_active_tool_names_from_profile<I, S>( | ||
| available_names: I, | ||
| tools_config: &ToolsConfig, | ||
| ) -> BTreeSet<String> | ||
| where | ||
| I: IntoIterator<Item = S>, | ||
| S: AsRef<str>, | ||
| { | ||
| let enabled = normalized_tool_override_set(&tools_config.enable); | ||
| let disabled = normalized_tool_override_set(&tools_config.disable); | ||
| let elevated_profile = tools_config.elevated.profile; | ||
|
|
||
| let mut resolved = BTreeSet::new(); | ||
| for raw_name in available_names { | ||
| let normalized = normalize_tool_name(raw_name.as_ref()); | ||
| if normalized.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| let preset_allowed = profile_includes_tool(tools_config.profile, &normalized) | ||
| || elevated_profile | ||
| .map(|profile| profile_includes_tool(profile, &normalized)) | ||
| .unwrap_or(false); | ||
| let default_allowed = if is_known_profile_tool(&normalized) { | ||
| preset_allowed | ||
| } else { | ||
| // Keep unknown/dynamic tools (MCP/WASM/plugins) enabled by default | ||
| // for backward compatibility unless explicitly disabled. | ||
| true | ||
| }; |
Contributor
There was a problem hiding this comment.
Built-in config tools currently bypass non-full profiles due incomplete known-tool classification.
Because unknown names default to allowed (Line 338-Line 340), built-ins missing from is_known_profile_tool are never profile-restricted. web_access_config and web_search_config are currently omitted, so restrictive profiles still expose config-mutation tools.
🔧 Proposed fix
fn is_known_profile_tool(tool_name: &str) -> bool {
matches!(
tool_name,
@@
| "model_routing_config"
| "proxy_config"
+ | "web_access_config"
+ | "web_search_config"
| "browser_open"
| "browser"
@@
)
}
@@
ToolProfilePreset::Ops => matches!(
tool_name,
@@
| "schedule"
| "pushover"
| "proxy_config"
+ | "web_access_config"
+ | "web_search_config"
| "model_routing_config"
| "shell" #[test]
fn all_tools_applies_profile_filter() {
@@
assert!(!names.contains(&"shell"));
assert!(names.contains(&"browser"));
assert!(names.contains(&"web_search_tool"));
+ assert!(!names.contains(&"web_access_config"));
+ assert!(!names.contains(&"web_search_config"));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/mod.rs` around lines 183 - 341, The bug is that built-in config
tools like web_access_config and web_search_config are not included in the
known-tool classification so non-Full profiles can’t restrict them; update
is_known_profile_tool to include "web_access_config" and "web_search_config" and
add those same names to the appropriate profile sets in profile_includes_tool
(e.g., include them in Ops and Research where needed) so
profile_includes_tool(...) and the default_allowed branch behave correctly for
built-in config tools referenced by resolve_active_tool_names_from_profile.
chumyin
force-pushed
the
feat/session-scope-config
branch
from
18adc1c to
3c3d4f6
Compare
github-actions
Bot
added
size: XS
Comment on lines
+145
to
+171
| ## `[agent.session]` | ||
|
|
||
| Persistent session controls for CLI/channel/gateway conversation state. | ||
|
|
||
| | Key | Default | Purpose | | ||
| |---|---|---| | ||
| | `backend` | `none` | Session store backend: `none`, `memory`, or `sqlite` | | ||
| | `strategy` | `per-sender` | Session keying strategy: `per-sender`, `per-channel`, or `main` | | ||
| | `ttl_seconds` | `3600` | Session expiry TTL in seconds | | ||
| | `max_messages` | `50` | Max retained messages per resolved session | | ||
|
|
||
| Strategy behavior: | ||
|
|
||
| - `per-sender`: one session per sender identity (channel-qualified when channel is known). | ||
| - `per-channel`: one shared session per channel. | ||
| - `main`: one global shared session. | ||
|
|
||
| Example: | ||
|
|
||
| ```toml | ||
| [agent.session] | ||
| backend = "sqlite" | ||
| strategy = "per-sender" | ||
| ttl_seconds = 7200 | ||
| max_messages = 120 | ||
| ``` | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, understand the docs structure and i18n organization
echo "=== Repository structure ==="
git ls-files docs/ | head -30
echo -e "\n=== Checking for i18n docs structure ==="
fd -t d 'i18n' docs/
echo -e "\n=== Checking for localized config-reference docs ==="
fd -t f 'config.*reference' docs/ | head -20Repository: zeroclaw-labs/zeroclaw
Length of output: 1174
🏁 Script executed:
#!/bin/bash
# Check for i18n-guide.md
echo "=== Looking for i18n-guide.md ==="
fd -t f 'i18n-guide.md'
echo -e "\n=== Checking docs/SUMMARY.md and locale-specific summaries ==="
fd -t f 'SUMMARY.md' docs/
echo -e "\n=== Checking i18n-coverage.md ==="
fd -t f 'i18n-coverage.md'Repository: zeroclaw-labs/zeroclaw
Length of output: 750
🏁 Script executed:
#!/bin/bash
# Search for SessionConfig implementation
echo "=== Searching for SessionConfig struct definition ==="
rg -n 'struct SessionConfig' --type rust
echo -e "\n=== Searching for session backend default ==="
rg -n 'backend.*none|default.*none' --type rust | grep -i session | head -20
echo -e "\n=== Searching for strategy default per-sender ==="
rg -n 'strategy.*per.?sender|default.*per.?sender' --type rust | grep -i session | head -20
echo -e "\n=== Searching for session configuration parsing ==="
rg -n '\[agent\.session\]' --type rustRepository: zeroclaw-labs/zeroclaw
Length of output: 456
🏁 Script executed:
#!/bin/bash
# Check the structure of config-reference.md to understand locale patterns
echo "=== Current config-reference.md size and structure ==="
wc -l docs/config-reference.md
echo -e "\n=== Check if config-reference has locale versions ==="
ls -la docs/config-reference.* 2>/dev/null || echo "No locale versions of config-reference found at root"
echo -e "\n=== Check inside i18n directory structure ==="
find docs/i18n -name '*config*reference*' -type f 2>/dev/null | head -20Repository: zeroclaw-labs/zeroclaw
Length of output: 606
🏁 Script executed:
#!/bin/bash
# Get SessionConfig struct details
echo "=== SessionConfig struct from schema.rs ==="
sed -n '897,950p' src/config/schema.rs
echo -e "\n=== Full session-related config section ==="
rg -n -A 20 'Session persistence configuration' src/config/schema.rsRepository: zeroclaw-labs/zeroclaw
Length of output: 2804
🏁 Script executed:
#!/bin/bash
# Check if localized versions have the [agent.session] section
echo "=== Checking if [agent.session] exists in localized docs ==="
for locale in el fr ja ru vi zh-CN; do
echo -e "\n--- docs/i18n/$locale/config-reference.md ---"
rg -c '\[agent\.session\]' "docs/i18n/$locale/config-reference.md" || echo "Not found"
done
echo -e "\n=== Checking compatibility shim ==="
cat docs/config-reference.vi.mdRepository: zeroclaw-labs/zeroclaw
Length of output: 621
🏁 Script executed:
#!/bin/bash
# Check the i18n-guide.md completion checklist
echo "=== Main i18n-guide.md completion checklist ==="
head -100 docs/i18n-guide.md
echo -e "\n=== Checking i18n-coverage.md for current status ==="
head -50 docs/i18n-coverage.mdRepository: zeroclaw-labs/zeroclaw
Length of output: 7356
🏁 Script executed:
#!/bin/bash
# Check what the actual defaults are in the code
echo "=== Full SessionConfig definition ==="
rg -B 5 -A 30 'struct SessionConfig' src/config/schema.rs
# Also check for default implementations
echo -e "\n=== Looking for SessionConfig Default impl or defaults ==="
rg -B 2 -A 15 'impl.*Default.*for.*Session|Session.*{.*backend.*:' src/config/schema.rs | head -50Repository: zeroclaw-labs/zeroclaw
Length of output: 351
🏁 Script executed:
#!/bin/bash
# Run markdown lint if available
echo "=== Checking markdown syntax ==="
if command -v markdownlint &> /dev/null || command -v mdl &> /dev/null; then
echo "markdownlint/mdl available, checking..."
markdownlint docs/config-reference.md 2>/dev/null || mdl docs/config-reference.md 2>/dev/null || echo "Lint check inconclusive"
else
echo "No markdown linter found in sandbox environment"
fi
# Validate TOML syntax in example
echo -e "\n=== Validating TOML example from lines 145-171 ==="
python3 <<'PYEOF'
import sys
try:
import tomllib
except ImportError:
import tomli as tomllib
example = '''[agent.session]
backend = "sqlite"
strategy = "per-sender"
ttl_seconds = 7200
max_messages = 120
'''
try:
result = tomllib.loads(example)
print("✓ TOML example is valid")
print(f" Parsed: {result}")
except Exception as e:
print(f"✗ TOML validation failed: {e}")
PYEOFRepository: zeroclaw-labs/zeroclaw
Length of output: 346
🏁 Script executed:
#!/bin/bash
# Check line count and content snapshot of localized config-reference
echo "=== Checking zh-CN config-reference.md size and last section ==="
wc -l docs/i18n/zh-CN/config-reference.md
tail -30 docs/i18n/zh-CN/config-reference.md
echo -e "\n=== Checking if English config-reference includes [agent.session] ==="
rg -n '\[agent\.session\]' docs/config-reference.mdRepository: zeroclaw-labs/zeroclaw
Length of output: 607
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern - does it include other recent sections?
echo "=== Comparing section headers in EN vs ZH-CN ==="
echo "--- English sections (last 10) ---"
rg '^## `\[' docs/config-reference.md | tail -10
echo -e "\n--- ZH-CN sections (last 10) ---"
rg '^## `\[' docs/i18n/zh-CN/config-reference.md | tail -10Repository: zeroclaw-labs/zeroclaw
Length of output: 424
🏁 Script executed:
#!/bin/bash
# Verify the content of the zi-CN bridge page
echo "=== Full content of zh-CN config-reference.md ==="
cat docs/i18n/zh-CN/config-reference.md
echo -e "\n=== Check other locales - sample of el ==="
head -30 docs/i18n/el/config-reference.mdRepository: zeroclaw-labs/zeroclaw
Length of output: 1492
Update localized config-reference documentation for all supported locales to include [agent.session] section.
The new [agent.session] configuration documentation (lines 145–171) is well-structured and accurate. All documented defaults are verified against the implementation in src/config/schema.rs:
backenddefault:none✓strategydefault:per-sender✓ttl_secondsdefault:3600✓max_messagesdefault:50✓- TOML example syntax: valid ✓
However, localized versions require updates. Per docs/i18n-guide.md mandatory completion checklist and coding guidelines, top-level docs changes must sync localized equivalents for all supported locales (en, zh-CN, ja, ru, fr, vi, el) in the same PR. Currently, none of the six localized config-reference.md files (in docs/i18n/*/) include the [agent.session] section. These are bridge pages that link to the English source without detailing new sections.
Options to resolve:
- Add full translations for all six locales, or
- Update localized bridge pages to include a source section map documenting
[agent.session]with a link to the English normative source, per the deferred translation policy indocs/i18n-guide.md.
Include i18n completion status in the PR notes per guidelines.
chumyin
force-pushed
the
feat/session-scope-config
branch
from
3c3d4f6 to
1e9c3c2
Compare
chumyin
force-pushed
the
feat/session-scope-config
branch
from
1e9c3c2 to
5670f13
Compare
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
2 participants
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
[session]) withdm_scope,history_scope, andidentity_links[channels_config.commands.<channel>]) withnative,allowed,config_writes[tools]) withprofile,enable,disable, and optionalelevated.profileValidation
cargo check -qcargo test -q --no-default-features --lib3753 passed; 0 failed; 2 ignoredconfig_validate_rejects_all_tools_profile_resolve_active_tool_names_all_tools_applies_profile_filtersession_config_defaultschannels_command_policy_from_tomlconversation_history_key_Linear
Summary by CodeRabbit