Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion dare_framework/mcp/tool_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,13 @@ def requires_approval(self) -> bool:
@property
def timeout_seconds(self) -> int:
value = _tool_field(self._tool_def, "timeout_seconds", 30)
return int(value) if isinstance(value, (int, float, str)) else 30
if isinstance(value, bool):
return 30
try:
parsed = int(value)
except (OverflowError, TypeError, ValueError):
return 30
return parsed if parsed > 0 else 30

@property
def is_work_unit(self) -> bool:
Expand Down
6 changes: 5 additions & 1 deletion dare_framework/tool/_internal/tools/edit_line.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ def _execute_edit(input: dict[str, Any], context: RunContext[Any]) -> ToolResult
if mode not in {"insert", "delete"}:
raise ToolError(code="INVALID_MODE", message="mode must be insert or delete", retryable=False)

line_number = _parse_line_number(input.get("line_number"))
# Missing line_number should use schema default (1), but explicit null remains invalid.
if "line_number" in input:
line_number = _parse_line_number(input.get("line_number"))
else:
line_number = 1
text = input.get("text", "")
strict_match = bool(input.get("strict_match", True))

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Change: Fix tool capability listing contract

## Why
A P0 regression occurred because the tool capability listing contract drifted between sync and async call sites. Some runtime paths awaited `list_capabilities()` while the implementation returned a plain list, and context assembly temporarily returned tool-definition dicts instead of `CapabilityDescriptor` objects, causing model adapter crashes.

## What Changes
- Align the gateway contract so `IToolGateway.list_capabilities()` is awaitable and returns `list[CapabilityDescriptor]`.
- Keep synchronous runtime assembly paths supported via a manager-side synchronous capability snapshot helper.
- Require context tool assembly to provide trusted `CapabilityDescriptor` entries to `ModelInput.tools`.
- Add regression coverage to prevent descriptor-to-dict type drift in context assembly.

## Impact
- Affected specs: `interface-layer`
- Affected code:
- `dare_framework/tool/kernel.py`
- `dare_framework/tool/tool_manager.py`
- `dare_framework/context/context.py`
- `tests/unit/test_context_implementation.py`
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
## MODIFIED Requirements
### Requirement: Tool manager contract
The tool domain SHALL define an `IToolManager` contract in `dare_framework/tool/kernel.py` that extends `IToolGateway`. The contract SHALL support trusted tool registration, provider aggregation, and prompt tool definition export without executing tools. At minimum it MUST include:
- `register_tool(...)`, `unregister_tool(...)`, `update_tool(...)`
- `register_provider(...)`, `unregister_provider(...)`
- awaitable `list_capabilities(...)`, `refresh(...)`
- `list_tool_defs(...)`, `get_capability(...)`
- `health_check(...)`

Implementations MAY expose a synchronous capability snapshot helper for synchronous runtime assembly paths, but this helper MUST return the same trusted `CapabilityDescriptor` model as `list_capabilities(...)`.

#### Scenario: Awaitable capability listing is stable for gateway callers
- **GIVEN** the default `ToolManager` implementation is used as `IToolGateway`
- **WHEN** callers execute `await list_capabilities()`
- **THEN** the result is `list[CapabilityDescriptor]`
- **AND** no sync/async type mismatch error is raised by awaiting callers

### Requirement: Trusted tool listings for model prompts
Context and runtime model-input assembly SHALL source tool availability from the trusted capability registry exposed by `IToolGateway.list_capabilities()`; tool listings MUST NOT originate from untrusted sources (planner/model output).

`ModelInput.tools` MUST carry `CapabilityDescriptor` entries. Model adapters are responsible for converting these trusted descriptors into provider-specific tool-definition payloads.

#### Scenario: Context assembles capability descriptors for model adapters
- **GIVEN** context is wired with the default `ToolManager`
- **WHEN** context assembles tool listings for a model request
- **THEN** each item is a `CapabilityDescriptor`
- **AND** adapters can access descriptor fields (`name`, `description`, `input_schema`) without dict coercion
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
## 1. Contract alignment
- [x] 1.1 Make `IToolGateway.list_capabilities()` awaitable in the interface contract.
- [x] 1.2 Update `ToolManager` to implement awaitable `list_capabilities()` and keep a sync capability snapshot helper for sync call sites.

## 2. Context/model-input type safety
- [x] 2.1 Ensure `Context.list_tools()` returns trusted `CapabilityDescriptor` entries (not tool-definition dicts).
- [x] 2.2 Preserve compatibility for legacy synchronous gateways.

## 3. Regression coverage and validation
- [x] 3.1 Add a regression test proving context tool assembly returns `CapabilityDescriptor` from `ToolManager`.
- [x] 3.2 Run targeted pytest for tool/context contract paths.
- [x] 3.3 Run `openspec validate fix-tool-capability-listing-contract --strict`.
19 changes: 19 additions & 0 deletions openspec/changes/fix-p1-tool-input-defaults/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Change: Fix P1 tool input defaults and metadata coercion

## Why
Two P1 issues caused avoidable tool failures:
- `edit_line` rejected insert calls when `line_number` was omitted even though the schema declares a default.
- MCP tool metadata with non-numeric `timeout_seconds` raised conversion errors and could break capability registration paths.

## What Changes
- Ensure `edit_line` treats missing `line_number` as line 1 for insert/delete indexing.
- Ensure MCP tool timeout parsing is defensive: invalid or non-positive timeout values fall back to a safe default.
- Add regression tests for both behaviors.

## Impact
- Affected specs: `workspace-file-tools`, `component-management`
- Affected code:
- `dare_framework/tool/_internal/tools/edit_line.py`
- `dare_framework/mcp/tool_provider.py`
- `tests/unit/test_v4_file_tools.py`
- `tests/unit/test_mcp_tool_provider.py`
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
## MODIFIED Requirements
### Requirement: Tool manager aggregates providers and exports tool defs
ToolManager SHALL aggregate `IToolProvider` instances and refresh their capabilities into the registry. It MUST provide prompt tool definitions derived from the registry and MUST NOT execute tool side-effects; invocation is owned by `IToolGateway` (implemented by ToolManager).

Provider-derived metadata coercion MUST be robust: malformed provider metadata values MUST NOT crash registry aggregation.

#### Scenario: Provider capabilities are visible in the registry
- **GIVEN** a provider is registered with the ToolManager
- **WHEN** `refresh()` is called
- **THEN** its capabilities are available via `list_capabilities()`

#### Scenario: Invalid provider timeout metadata falls back safely
- **GIVEN** a provider exposes a tool with non-numeric `timeout_seconds`
- **WHEN** ToolManager aggregates provider capabilities
- **THEN** capability registration does not fail
- **AND** the effective timeout metadata falls back to the default value

#### Scenario: Tool manager does not invoke tools
- **GIVEN** a tool capability is registered
- **WHEN** an invocation is needed
- **THEN** the system routes the call through `IToolGateway.invoke(...)` (ToolManager implements the gateway)
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
## MODIFIED Requirements
### Requirement: Edit line tool behavior
The `edit_line` tool SHALL insert or delete a line in a text file using a 1-indexed `line_number`, enforce `edit_line.max_bytes`, preserve newline style, and default `strict_match` to true for deletions.

When `line_number` is omitted, the tool MUST default it to `1`.

#### Scenario: Insert adds a line at the target index
- **WHEN** `edit_line` is invoked with `mode: insert`
- **THEN** the line is inserted at the requested position and the tool returns the affected line number

#### Scenario: Insert defaults to first line when line_number is omitted
- **GIVEN** an existing text file under workspace roots
- **WHEN** `edit_line` is invoked with `mode: insert` and no `line_number`
- **THEN** the line is inserted at line 1
- **AND** the tool returns `line_number: 1`

#### Scenario: Delete fails on strict mismatch
- **GIVEN** `strict_match` is true and the target line differs from the provided text
- **WHEN** `edit_line` is invoked with `mode: delete`
- **THEN** the tool returns a failure result indicating a mismatch
11 changes: 11 additions & 0 deletions openspec/changes/fix-p1-tool-input-defaults/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
## 1. Edit-line default behavior
- [x] 1.1 Add a failing test proving `edit_line` insert succeeds when `line_number` is omitted.
- [x] 1.2 Update `edit_line` execution to default missing `line_number` to 1.

## 2. MCP timeout coercion hardening
- [x] 2.1 Add a failing test proving invalid MCP `timeout_seconds` does not crash and falls back.
- [x] 2.2 Update MCP timeout parsing to safely coerce and fallback on invalid/non-positive values.

## 3. Validation
- [x] 3.1 Run targeted pytest for affected modules.
- [x] 3.2 Run `openspec validate fix-p1-tool-input-defaults --strict`.
21 changes: 21 additions & 0 deletions openspec/specs/component-management/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,24 @@ ToolManager SHALL aggregate `IToolProvider` instances and refresh their capabili
- **WHEN** an invocation is needed
- **THEN** the system routes the call through `IToolGateway.invoke(...)` (ToolManager implements the gateway)

### Requirement: Skill store builder composes loaders deterministically
The skill domain SHALL provide a `SkillStoreBuilder` that deterministically composes skill loaders and filtering rules before constructing an `ISkillStore`.

- `SkillStoreBuilder.config(config)` MUST derive filesystem skill loading roots from `Config.workspace_dir` and `Config.user_dir`.
- The builder MUST allow callers to append external skill loaders.
- The builder MUST support disabling skills by `skill_id` before final store exposure.
- The resulting `ISkillStore` MUST provide deterministic `list_skills()` and `get_skill(skill_id)` behavior after composition.

#### Scenario: Config-derived loader and external loader are combined
- **GIVEN** a `Config` with workspace and user directories
- **AND** an external loader is attached to `SkillStoreBuilder`
- **WHEN** `build()` is called
- **THEN** the resulting store contains skills from both config-derived filesystem loading and the external loader

#### Scenario: Disabled skill ids are filtered out
- **GIVEN** a composed skill store contains skill ids `a`, `b`, and `c`
- **AND** `disable_skill("b")` is configured
- **WHEN** `build()` is called
- **THEN** `list_skills()` excludes `b`
- **AND** `get_skill("b")` returns `None`

15 changes: 8 additions & 7 deletions openspec/specs/configuration-management/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,15 @@ The allowtools and allowmcps settings SHALL be treated as normal configuration k
### Requirement: Config Model Structure
The Config model SHALL include at least the following top-level fields: llm, mcp, tools, allowtools, allowmcps, components, workspace_dir, and user_dir. The llm field MUST support connectivity configuration such as adapter, endpoint, api_key, model, and optional proxy settings (http, https, no_proxy, use_system_proxy, disabled). When proxy.disabled is true, proxy settings MUST be treated as disabled. Explicit proxy configuration and system proxy selection MUST be mutually exclusive in the effective model. The mcp and tools fields MUST support component-scoped configuration objects keyed by component name. The components field MUST support enable/disable flags and per-component configuration for entry point components by type and name (including validator, hook, skill, memory, model_adapter, tool, mcp, and prompt).

#### Scenario: LLM connectivity fields available
- **WHEN** the effective Config is produced
- **THEN** the llm field can provide adapter, endpoint, api_key, model, and optional proxy values
The Config model MUST NOT require top-level skill mode/path fields such as `skill_mode`, `skill_paths`, or `initial_skill_path`.

#### Scenario: Proxy disabled takes precedence
- **GIVEN** llm.proxy.disabled is true
- **WHEN** the effective Config is produced
- **THEN** the model adapter treats proxy as disabled regardless of other proxy fields
Skill filesystem discovery MUST be derived from `workspace_dir` and `user_dir` by the skill loading layer.

#### Scenario: Skill loading relies on workspace/user dirs only
- **GIVEN** an effective Config containing `workspace_dir` and `user_dir`
- **WHEN** the runtime initializes skill loading
- **THEN** skill loading derives filesystem roots from those directories
- **AND** no `skill_mode`, `skill_paths`, or `initial_skill_path` fields are required in Config

### Requirement: Component Enablement Configuration
The system SHALL support a uniform enable/disable configuration for all entry point components using a type-scoped structure. The components.<type>.disabled list identifies disabled component names. The default behavior MUST be enabled when a name is absent from components.<type>.disabled.
Expand Down
62 changes: 33 additions & 29 deletions openspec/specs/interface-layer/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,26 +36,28 @@ The developer-facing agent API SHALL support composing agents via typed builders
- When a required component is not explicitly provided, builders SHALL attempt to resolve it via the corresponding domain manager using the effective `Config`.
- For multi-load component categories (e.g., tools/hooks/validators), builders SHALL merge explicit components with manager-loaded components (extend semantics) while preserving injection order.
- Config enable/disable filtering MUST apply only to the manager-loaded subset and MUST NOT remove explicitly injected components.
- Builders SHALL keep `assemble_context` externally injectable so callers can customize `AssembledContext` generation.
- Builders SHALL expose a boolean skill-tool toggle (`_enable_skill_tool` via public builder API) as the only built-in skill mode switch.
- When skill-tool toggle is enabled, builders MUST auto-register `search_skill` and default context assembly MUST ignore `sys_skill`.
- When skill-tool toggle is disabled, builders MUST NOT auto-register `search_skill` and explicit `sys_skill` injection remains effective.

#### Scenario: Resolve model adapter via manager
- **GIVEN** a builder with no explicit model adapter
- **AND** a provided `IModelAdapterManager`
- **AND** an effective `Config`
#### Scenario: Skill tool mode auto-registers search tool
- **GIVEN** a builder with skill-tool toggle enabled
- **WHEN** `build()` is called
- **THEN** the builder MUST call `IModelAdapterManager.load_model_adapter(config=Config)` and use the returned adapter as the agent model
- **THEN** the built context exposes `search_skill` in tool capabilities
- **AND** default assemble logic does not merge `sys_skill`

#### Scenario: Explicit model overrides manager
- **GIVEN** a builder with an explicitly injected model adapter via builder API
- **AND** a provided `IModelAdapterManager` that would otherwise return a different adapter
- **WHEN** `build()` is called
- **THEN** the explicitly injected adapter MUST be used
#### Scenario: Non skill tool mode preserves explicit sys_skill
- **GIVEN** a builder with skill-tool toggle disabled
- **AND** an explicit `sys_skill` is set on context
- **WHEN** default assemble logic runs
- **THEN** `search_skill` is not auto-registered
- **AND** the assembled system prompt includes the explicit `sys_skill`

#### Scenario: Multi-load extend with config boundary
- **GIVEN** a builder with an explicitly injected tool `tool_x`
- **AND** a provided `IToolManager` that loads `tool_y` and `tool_z`
- **AND** config disables `tool_z`
- **WHEN** `build()` is called
- **THEN** `tool_x` and `tool_y` MUST be included and `tool_z` MUST be omitted, and `tool_x` MUST NOT be filtered out by config
#### Scenario: Custom assemble context remains pluggable
- **GIVEN** a caller injects a custom `assemble_context`
- **WHEN** the agent assembles context for model input
- **THEN** the custom strategy is used instead of the default strategy

### Requirement: Optional MCP Integration Surface
The interface layer SHALL NOT define protocol adapter interfaces. Protocol adapter surfaces are deferred until a dedicated integration is specified.
Expand Down Expand Up @@ -92,19 +94,17 @@ The system SHALL define component manager interfaces alongside their owning doma
The tool domain SHALL define an `IToolManager` contract in `dare_framework/tool/kernel.py` that extends `IToolGateway`. The contract SHALL support trusted tool registration, provider aggregation, and prompt tool definition export without executing tools. At minimum it MUST include:
- `register_tool(...)`, `unregister_tool(...)`, `update_tool(...)`
- `register_provider(...)`, `unregister_provider(...)`
- `list_capabilities(...)`, `refresh(...)`
- awaitable `list_capabilities(...)`, `refresh(...)`
- `list_tool_defs(...)`, `get_capability(...)`
- `health_check(...)`

#### Scenario: Register tool returns a capability descriptor
- **GIVEN** a valid `ITool` implementation
- **WHEN** `register_tool(...)` is called
- **THEN** the manager returns a `CapabilityDescriptor` with a stable `capability_id` and trusted metadata
Implementations MAY expose a synchronous capability snapshot helper for synchronous runtime assembly paths, but this helper MUST return the same trusted `CapabilityDescriptor` model as `list_capabilities(...)`.

#### Scenario: Tool definitions are derived from registry
- **GIVEN** a tool has been registered into the manager
- **WHEN** `list_tool_defs()` is called
- **THEN** the result is derived from the manager registry and not from model output
#### Scenario: Awaitable capability listing is stable for gateway callers
- **GIVEN** the default `ToolManager` implementation is used as `IToolGateway`
- **WHEN** callers execute `await list_capabilities()`
- **THEN** the result is `list[CapabilityDescriptor]`
- **AND** no sync/async type mismatch error is raised by awaiting callers

### Requirement: Builder facade for variant selection
The system SHALL provide a stable facade for selecting which builder variant to use via `BaseAgent`, such as:
Expand Down Expand Up @@ -133,11 +133,15 @@ The system SHALL use the tool's `name` as the canonical identity for capabilitie
- **THEN** the tool definition name equals the tool's `name` and tool calls route by that value

### Requirement: Trusted tool listings for model prompts
`IToolProvider.list_tools()` SHALL derive tool definitions from the trusted capability registry exposed by `IToolGateway.list_capabilities()`; tool listings MUST NOT originate from untrusted sources (planner/model output).
Context and runtime model-input assembly SHALL source tool availability from the trusted capability registry exposed by `IToolGateway.list_capabilities()`; tool listings MUST NOT originate from untrusted sources (planner/model output).

`ModelInput.tools` MUST carry `CapabilityDescriptor` entries. Model adapters are responsible for converting these trusted descriptors into provider-specific tool-definition payloads.

#### Scenario: Tool provider uses the gateway registry
- **WHEN** the context assembles tool definitions for a model prompt
- **THEN** the tool provider queries the tool gateway capability registry and converts those capabilities into tool definitions for the prompt
#### Scenario: Context assembles capability descriptors for model adapters
- **GIVEN** context is wired with the default `ToolManager`
- **WHEN** context assembles tool listings for a model request
- **THEN** each item is a `CapabilityDescriptor`
- **AND** adapters can access descriptor fields (`name`, `description`, `input_schema`) without dict coercion

### Requirement: Default model adapter manager fallback
When no explicit model adapter and no model adapter manager are provided, builders SHALL fall back to a default model adapter manager created by the model domain factory and resolve the adapter using the effective `Config`.
Expand Down
Loading