Fix tool output limits and config docs#26
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens tool-output size handling to prevent oversized tool results from consuming too much model context, and updates configuration parsing/docs to treat ACL as the canonical config language while keeping legacy .hcl compatibility.
Changes:
- Truncate tool output at the registry layer and add tests to validate truncation behavior.
- Update SDK docs (Python/Node) and README to reflect ACL-only config support (no JSON), with
.hclaccepted as a legacy filename. - Expand config parsing to accept both ACL-labeled blocks and legacy HCL-style
name/idattributes for provider/model blocks.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/python/src/lib.rs | Updates Python SDK docs to reflect ACL-compatible config and removal of JSON support. |
| sdk/node/src/lib.rs | Updates Node SDK docs to reflect ACL-compatible config and removal of JSON support. |
| sdk/node/index.d.ts | Updates TypeScript declarations/docs to match ACL-compatible config and no JSON. |
| core/src/tools/registry.rs | Truncates tool outputs after execution and adds a truncation test. |
| core/src/tools/mod.rs | Introduces a shared truncate_tool_output helper using UTF-8-safe truncation. |
| core/src/tools/builtin/web_fetch.rs | Updates tool description to mention capped output. |
| core/src/tools/builtin/read.rs | Updates tool description to mention capped output and offset/limit guidance. |
| core/src/mcp/protocol.rs | Updates comment wording to reflect ACL/HCL-like flat config format. |
| core/src/lib.rs | Updates crate-level docs to reflect ACL-compatible config and .hcl legacy support. |
| core/src/config.rs | Shifts file loading to ACL parsing and adds compatibility parsing helpers + tests. |
| core/src/agent_api.rs | Updates Agent config loading behavior/docs (ACL/HCL; explicit JSON rejection). |
| README.md | Updates public documentation and examples to use .acl and labeled blocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ => { | ||
| // Other top-level blocks are not supported in ACL format for now | ||
| // (queue, search, etc. are HCL-only) | ||
| // Other top-level blocks are not mapped by the lightweight | ||
| // ACL loader yet (queue, search, memory, MCP, etc.). | ||
| } |
There was a problem hiding this comment.
CodeConfig::from_file now always delegates to from_acl, but from_acl currently only maps a subset of CodeConfig (providers/default_model/etc.) and explicitly ignores other top-level config sections (queue/search/memory/MCP/etc.). This means those settings will be silently dropped when loading from .acl/.hcl files. Consider either fully mapping/deserializing the remaining CodeConfig fields from ACL, or failing loudly on unsupported blocks so users don’t think their config is being applied when it isn’t.
| #[test] | ||
| fn test_config_supports_hcl_style_provider_blocks() { | ||
| std::env::set_var("A3S_CODE_TEST_API_KEY", "sk-test"); |
There was a problem hiding this comment.
This test sets a process-wide environment variable (std::env::set_var) and does not restore it, which can introduce order-dependence/flakiness when tests run in parallel. Consider using a scoped env-var guard (set + restore) or running this test serially.
| #[test] | |
| fn test_config_supports_hcl_style_provider_blocks() { | |
| std::env::set_var("A3S_CODE_TEST_API_KEY", "sk-test"); | |
| struct EnvVarGuard { | |
| key: &'static str, | |
| original: Option<std::ffi::OsString>, | |
| } | |
| impl EnvVarGuard { | |
| fn set(key: &'static str, value: &str) -> Self { | |
| let original = std::env::var_os(key); | |
| std::env::set_var(key, value); | |
| Self { key, original } | |
| } | |
| } | |
| impl Drop for EnvVarGuard { | |
| fn drop(&mut self) { | |
| match &self.original { | |
| Some(value) => std::env::set_var(self.key, value), | |
| None => std::env::remove_var(self.key), | |
| } | |
| } | |
| } | |
| #[test] | |
| fn test_config_supports_hcl_style_provider_blocks() { | |
| let _api_key_guard = EnvVarGuard::set("A3S_CODE_TEST_API_KEY", "sk-test"); |
| assert_eq!(result.exit_code, 0); | ||
| assert!(result.output.contains("[tool output truncated:")); | ||
| assert!(result.output.len() < super::super::MAX_OUTPUT_SIZE + 512); |
There was a problem hiding this comment.
The length assertion here is somewhat brittle (< MAX_OUTPUT_SIZE + 512) because it couples the test to the exact truncation message length. Consider asserting that the output starts with the expected prefix and that the retained content length is <= MAX_OUTPUT_SIZE (or derive the allowed overhead from the truncation notice) so future wording tweaks don't break the test.
| assert_eq!(result.exit_code, 0); | |
| assert!(result.output.contains("[tool output truncated:")); | |
| assert!(result.output.len() < super::super::MAX_OUTPUT_SIZE + 512); | |
| let truncation_notice = "[tool output truncated:"; | |
| assert_eq!(result.exit_code, 0); | |
| assert!(result.output.contains(truncation_notice)); | |
| let retained_content_len = result | |
| .output | |
| .find(truncation_notice) | |
| .expect("truncation notice should be present"); | |
| assert!(retained_content_len <= super::super::MAX_OUTPUT_SIZE); |
| let shown = truncate_utf8(output, MAX_OUTPUT_SIZE); | ||
| format!( | ||
| "{}\n\n[tool output truncated: showing the first {} of {} bytes. Use narrower arguments such as offset/limit or filtering to read the remaining content.]", | ||
| shown, | ||
| shown.len(), | ||
| output.len() | ||
| ) |
There was a problem hiding this comment.
truncate_tool_output appends a truncation notice after keeping the first MAX_OUTPUT_SIZE bytes, which means the final returned string can exceed MAX_OUTPUT_SIZE despite the constant comment saying it's a maximum output size. If the goal is to cap tool output to a hard limit, consider reserving space for the suffix (or re-truncating after adding the notice) so the final output length is always <= MAX_OUTPUT_SIZE bytes.
| let shown = truncate_utf8(output, MAX_OUTPUT_SIZE); | |
| format!( | |
| "{}\n\n[tool output truncated: showing the first {} of {} bytes. Use narrower arguments such as offset/limit or filtering to read the remaining content.]", | |
| shown, | |
| shown.len(), | |
| output.len() | |
| ) | |
| let suffix = format!( | |
| "\n\n[tool output truncated: showing the first {{shown_bytes}} of {} bytes. Use narrower arguments such as offset/limit or filtering to read the remaining content.]", | |
| output.len() | |
| ); | |
| if suffix.len() >= MAX_OUTPUT_SIZE { | |
| return truncate_utf8(&suffix, MAX_OUTPUT_SIZE).to_string(); | |
| } | |
| let available_for_content = MAX_OUTPUT_SIZE.saturating_sub(suffix.len()); | |
| let shown = truncate_utf8(output, available_for_content); | |
| let suffix = suffix.replace("{shown_bytes}", &shown.len().to_string()); | |
| if shown.len() + suffix.len() > MAX_OUTPUT_SIZE { | |
| let available_for_content = MAX_OUTPUT_SIZE.saturating_sub(suffix.len()); | |
| let shown = truncate_utf8(output, available_for_content); | |
| return format!("{}{}", shown, suffix); | |
| } | |
| format!("{}{}", shown, suffix) |
|
|
||
| fn acl_usize_attr(block: &a3s_acl::Block, keys: &[&str]) -> Option<usize> { | ||
| match acl_attr(block, keys) { | ||
| Some(a3s_acl::Value::Number(value)) if *value >= 0.0 => Some(*value as usize), |
There was a problem hiding this comment.
acl_usize_attr converts Value::Number(f64) to usize using as, which will silently truncate fractional values and also saturate/produce surprising results for NaN/very large numbers. Consider validating that the number is finite, non-negative, and integral (e.g., value.fract() == 0.0) and returning None/an error otherwise.
| Some(a3s_acl::Value::Number(value)) if *value >= 0.0 => Some(*value as usize), | |
| Some(a3s_acl::Value::Number(value)) | |
| if value.is_finite() | |
| && *value >= 0.0 | |
| && value.fract() == 0.0 | |
| && *value <= usize::MAX as f64 => | |
| { | |
| Some(*value as usize) | |
| } |
Summary
Verification
Note: cargo test could not run in this checkout because the path dependency /home/lxt/work/project/A3S-Lab/ahp/Cargo.toml is missing.