fix: address config and skill review feedback

Lixtt · Lixtt · commit 2abacaedfe56 · 2026-05-21T16:52:26.000+08:00
diff --git a/.github/setup-workspace.sh b/.github/setup-workspace.sh
@@ -9,6 +9,7 @@ echo "Replacing path dependencies with crates.io versions..."
 
 # core/Cargo.toml — internal crate deps
 sed -i.bak \
+  -e 's|a3s-common = { version = "0.1.1", path = "../../common" }|a3s-common = "0.1.1"|' \
   -e 's|a3s-common = { version = "0.1", path = "../../common" }|a3s-common = "0.1.1"|' \
   -e 's|a3s-memory = { version = "0.1.1", path = "../../memory" }|a3s-memory = "0.1.1"|' \
   -e 's|a3s-lane = { version = "0.4", path = "../../lane" }|a3s-lane = "0.4"|' \
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/README.md b/README.md
@@ -78,6 +78,13 @@ default_model = "anthropic/claude-sonnet-4-20250514"
 
 providers "anthropic" {
   apiKey = env("ANTHROPIC_API_KEY")
+
+  models "claude-sonnet-4-20250514" {
+    limit = {
+      context = 200000
+      output = 8192
+    }
+  }
 }
 ```
 
@@ -1267,6 +1274,13 @@ default_model = "anthropic/claude-sonnet-4-20250514"
 
 providers "anthropic" {
   apiKey = env("ANTHROPIC_API_KEY")
+
+  models "claude-sonnet-4-20250514" {
+    limit = {
+      context = 200000
+      output = 8192
+    }
+  }
 }
 
 skill_dirs = ["./skills"]
@@ -1279,6 +1293,10 @@ ahp = {
 }
 ```
 
+Model token limits use the `limit = { context = ..., output = ... }` object as the canonical ACL shape.
+The flat `maxTokens` and `contextTokens` fields are accepted only as deprecated
+migration aliases and emit warnings.
+
 ---
 
 ## Development
diff --git a/core/Cargo.toml b/core/Cargo.toml
@@ -13,7 +13,7 @@ path = "src/lib.rs"
 
 [dependencies]
 # Internal crates
-a3s-common = { version = "0.1.1" }
+a3s-common = { version = "0.1.1", path = "../../common" }
 a3s-memory = { version = "0.1.1", path = "../../memory" }
 a3s-lane = { version = "0.4", path = "../../lane" }
 a3s-search = { version = "1.2.3", path = "../../search", default-features = false, features = ["lightpanda"] }
diff --git a/core/skills/code-review.md b/core/skills/code-review.md
@@ -1,6 +1,7 @@
 ---
 name: code-review
 description: Review code for best practices, bugs, and improvements
+allowed-tools: "grep(*), glob(*), read(*)"
 kind: instruction
 tags:
   - review
diff --git a/core/skills/code-search.md b/core/skills/code-search.md
@@ -1,6 +1,7 @@
 ---
 name: code-search
 description: Search codebase for patterns, functions, or types
+allowed-tools: "grep(*), glob(*), read(*)"
 kind: instruction
 tags:
   - search
diff --git a/core/skills/explain-code.md b/core/skills/explain-code.md
@@ -1,6 +1,7 @@
 ---
 name: explain-code
 description: Explain how code works in clear, simple terms
+allowed-tools: "grep(*), glob(*), read(*)"
 kind: instruction
 tags:
   - explain
diff --git a/core/skills/find-bugs.md b/core/skills/find-bugs.md
@@ -1,6 +1,7 @@
 ---
 name: find-bugs
 description: Identify potential bugs, vulnerabilities, and code smells
+allowed-tools: "grep(*), glob(*), read(*)"
 kind: instruction
 tags:
   - bugs
diff --git a/core/src/agent.rs b/core/src/agent.rs
@@ -627,15 +627,13 @@ impl SessionCommand for ToolCommand {
     async fn execute(&self) -> Result<Value> {
         // Check skill-based tool permissions
         if let Some(registry) = &self.skill_registry {
-            let instruction_skills = registry.by_kind(crate::skills::SkillKind::Instruction);
-
             // If there are instruction skills with tool restrictions, check permissions
-            let has_restrictions = instruction_skills.iter().any(|s| s.allowed_tools.is_some());
+            let restricting_skills = registry.global_tool_restricting_skills();
 
-            if has_restrictions {
+            if !restricting_skills.is_empty() {
                 let mut allowed = false;
 
-                for skill in &instruction_skills {
+                for skill in &restricting_skills {
                     if skill.is_tool_allowed(&self.tool_name) {
                         allowed = true;
                         break;
diff --git a/core/src/config/loader.rs b/core/src/config/loader.rs
@@ -52,8 +52,22 @@ fn acl_usize_attr(block: &a3s_acl::Block, keys: &[&str]) -> Option<usize> {
     }
 }
 
-fn acl_u32_attr(block: &a3s_acl::Block, keys: &[&str]) -> Option<u32> {
-    acl_usize_attr(block, keys).map(|value| value.min(u32::MAX as usize) as u32)
+fn acl_u32(value: &a3s_acl::Value) -> Option<u32> {
+    match value {
+        a3s_acl::Value::Number(value) if *value >= 0.0 => {
+            Some((*value as usize).min(u32::MAX as usize) as u32)
+        }
+        _ => None,
+    }
+}
+
+fn acl_object_u32_attr(value: &a3s_acl::Value, key: &str) -> Option<u32> {
+    match value {
+        a3s_acl::Value::Object(pairs) => pairs
+            .iter()
+            .find_map(|(candidate, value)| (candidate == key).then(|| acl_u32(value)).flatten()),
+        _ => None,
+    }
 }
 
 fn acl_path_list_attr(block: &a3s_acl::Block, keys: &[&str]) -> Option<Vec<PathBuf>> {
@@ -280,52 +294,38 @@ impl CodeConfig {
                                             model.release_date = Some(release_date);
                                         }
                                     }
-                                    "maxTokens" | "max_tokens" | "outputTokens"
-                                    | "output_tokens" => {
-                                        if let Some(output) = acl_u32_attr(
-                                            model_block,
-                                            &[
-                                                "maxTokens",
-                                                "max_tokens",
-                                                "outputTokens",
-                                                "output_tokens",
-                                            ],
-                                        ) {
+                                    "maxTokens" => {
+                                        tracing::warn!(
+                                            provider = %provider.name,
+                                            model = %model.id,
+                                            field = "maxTokens",
+                                            "Flat ACL model token limit fields are deprecated; use limit = {{ output = ..., context = ... }}"
+                                        );
+                                        if let Some(output) = acl_u32(value) {
                                             model.limit.output = output;
                                         }
                                     }
-                                    "contextTokens" | "context_tokens" | "maxContextTokens"
-                                    | "max_context_tokens" => {
-                                        if let Some(context) = acl_u32_attr(
-                                            model_block,
-                                            &[
-                                                "contextTokens",
-                                                "context_tokens",
-                                                "maxContextTokens",
-                                                "max_context_tokens",
-                                            ],
-                                        ) {
+                                    "contextTokens" => {
+                                        tracing::warn!(
+                                            provider = %provider.name,
+                                            model = %model.id,
+                                            field = "contextTokens",
+                                            "Flat ACL model token limit fields are deprecated; use limit = {{ output = ..., context = ... }}"
+                                        );
+                                        if let Some(context) = acl_u32(value) {
                                             model.limit.context = context;
                                         }
                                     }
-                                    _ => {}
-                                }
-                            }
-
-                            for limit_block in &model_block.blocks {
-                                if limit_block.name == "limit" {
-                                    if let Some(output) = acl_u32_attr(
-                                        limit_block,
-                                        &["output", "max_tokens", "output_tokens"],
-                                    ) {
-                                        model.limit.output = output;
-                                    }
-                                    if let Some(context) = acl_u32_attr(
-                                        limit_block,
-                                        &["context", "context_tokens", "max_context_tokens"],
-                                    ) {
-                                        model.limit.context = context;
+                                    "limit" => {
+                                        if let Some(output) = acl_object_u32_attr(value, "output") {
+                                            model.limit.output = output;
+                                        }
+                                        if let Some(context) = acl_object_u32_attr(value, "context")
+                                        {
+                                            model.limit.context = context;
+                                        }
                                     }
+                                    _ => {}
                                 }
                             }
 
diff --git a/core/src/config/tests.rs b/core/src/config/tests.rs
@@ -123,13 +123,15 @@ fn test_config_parses_acl_model_token_limits() {
 
               models "glm-5.1" {
                 name = "GLM 5.1"
-                max_tokens = 4096
-                context_tokens = 32768
+                limit = {
+                  output = 4096
+                  context = 32768
+                }
               }
 
-              models "camel-aliases" {
-                outputTokens = 8192
-                maxContextTokens = 65536
+              models "flat-alias" {
+                maxTokens = 8192
+                contextTokens = 65536
               }
             }
         "#,
@@ -140,9 +142,9 @@ fn test_config_parses_acl_model_token_limits() {
     assert_eq!(flat.output, 4096);
     assert_eq!(flat.context, 32768);
 
-    let aliases = &config.providers[0].models[1].limit;
-    assert_eq!(aliases.output, 8192);
-    assert_eq!(aliases.context, 65536);
+    let flat_alias = &config.providers[0].models[1].limit;
+    assert_eq!(flat_alias.output, 8192);
+    assert_eq!(flat_alias.context, 65536);
 }
 
 #[test]
diff --git a/core/src/safety_gate.rs b/core/src/safety_gate.rs
@@ -103,13 +103,12 @@ impl<'a> ToolSafetyGate<'a> {
 
     fn check_skill_restrictions(&self, tool_name: &str) -> Option<ToolGateDecision> {
         let registry = self.config.skill_registry.as_ref()?;
-        let instruction_skills = registry.by_kind(crate::skills::SkillKind::Instruction);
-        let has_restrictions = instruction_skills.iter().any(|s| s.allowed_tools.is_some());
-        if !has_restrictions {
+        let restricting_skills = registry.global_tool_restricting_skills();
+        if restricting_skills.is_empty() {
             return None;
         }
 
-        let allowed = instruction_skills
+        let allowed = restricting_skills
             .iter()
             .any(|skill| skill.is_tool_allowed(tool_name));
         if allowed {
@@ -220,6 +219,31 @@ mod tests {
         ));
     }
 
+    #[tokio::test]
+    async fn builtin_skill_permissions_do_not_restrict_default_session_tools() {
+        let config = AgentConfig {
+            skill_registry: Some(Arc::new(SkillRegistry::with_builtins())),
+            permission_checker: Some(Arc::new(StaticPermission(PermissionDecision::Allow))),
+            ..Default::default()
+        };
+        let gate = ToolSafetyGate::new(&config);
+
+        let decision = gate
+            .decide(ToolGateInput {
+                tool_name: "write",
+                args: &json!({"file_path": "x"}),
+                pre_tool_block: None,
+            })
+            .await;
+
+        assert!(matches!(
+            decision,
+            ToolGateDecision::Execute {
+                reason: ToolGateApproval::PermissionAllow,
+            }
+        ));
+    }
+
     #[tokio::test]
     async fn hook_block_denies_before_permission_allow() {
         let config = AgentConfig {
diff --git a/core/src/skills/builtin.rs b/core/src/skills/builtin.rs
@@ -69,8 +69,9 @@ mod tests {
         let skill = code_search_skill();
         assert_eq!(skill.name, "code-search");
         assert!(skill.content.contains("Code Search"));
-        assert!(skill.allowed_tools.is_none());
-        assert!(skill.is_tool_allowed("write"));
+        assert!(skill.allowed_tools.is_some());
+        assert!(skill.is_tool_allowed("read"));
+        assert!(!skill.is_tool_allowed("write"));
     }
 
     #[test]
diff --git a/core/src/skills/mod.rs b/core/src/skills/mod.rs
diff --git a/core/src/skills/registry.rs b/core/src/skills/registry.rs
diff --git a/core/src/tools/skill.rs b/core/src/tools/skill.rs
