diff --git a/plans/REMAINING-SLOP-PLAN.md b/plans/REMAINING-SLOP-PLAN.md index 573f992bc3..5b1e4ea04d 100644 --- a/plans/REMAINING-SLOP-PLAN.md +++ b/plans/REMAINING-SLOP-PLAN.md @@ -133,13 +133,50 @@ Three audits, one per catalog rung group, each finding read in the code and chec - `RecoveryCondition` matching a lifecycle by `declared_name` string equality across two families (`recovery_states.py:21-26`): `issubclass`. **6.4 Rosters and registries beside families (MEMB-1/2/4, about 120).** -- Five hand-rolled `__init_subclass__` registries: `Carry.declared` (`installer/carry.py:45-54`), `ThreadRename.kinds` (`thread_management.py:557-567`, gone with 4.1), `BirthlessIncarnation.members` (`thread_identity.py:185-192`), `CliToolArguments.types` (`tool_calls.py:89-95`), `_start_phases`/`_end_events` (`pi_events.py:59-93`): `DeclaredFamily` or attributes through the MRO. -- `PI_0_85_1_EVENT_KINDS`, 36 names re-typed beside `NativePiEvent.known_kinds()` (`pi_native_backend.py:63-81`, check at `agent_backend.py:428`): derive the union; the equality check goes. -- `THREAD_CAPABILITIES` plus `serves_threads`/`serves_new_threads` flags plus `validate_backends` probing with `getattr` (`agent_backend.py:358-466`): `ThreadServingBackend(AgentBackend, Steers, Compacts)` and `NewThreadBackend`; ABC instantiation enforces the roles. +- Four remaining hand-rolled `__init_subclass__` registration owners: `Carry.declared` (`installer/carry.py:42-58`), `ThreadRename.kinds` (`thread_management.py`; still present, not removed by 4.1 at this integration base), `BirthlessIncarnation.members` (`thread_identity.py:214-226`), and `_start_phases`/`_end_events` (`pi_events.py:59-93`). Move registration through the existing nominal mechanisms or derive behavior through the MRO; preserve each boundary's actual keys and consumers. `CliToolArguments.types` and its private hook are removed by the CLI argument slice below. +- Preserve the independent Pi producer ABI contract: `PI_0_85_1_EVENT_KINDS` in `pi_native_backend.py` declares the pinned runtime's emitted kinds, while `NativePiEvent.known_kinds()` declares converter coverage. `AgentBackend.require_event_family` compares these independent answers and reports missing or extra converters. Deriving the expected producer kinds from converters would erase this check. Refactor the internal `_start_phases`/`_end_events` registration lifecycle without weakening producer/converter coverage; removal of the external manifest is not a completion condition. +- Backend role slice #2623 is published and reviewed, awaiting integration: it removes `THREAD_CAPABILITIES`, the serving flags, `validate_backends` and copied `unstarted_type` declarations. `ThreadServingBackend(Steers, Compacts, AgentBackend)` puts capability bases first so `AgentBackend`'s concrete refusal method cannot hide the abstract turn obligation; `NewThreadBackend.selected` derives the unique default. Preserve the independent producer/converter event comparison. - Display names re-typed from class names in about 40 places (`tools.py` labels, `notification_progress.py:45-157`, `todos.py:1502/2012/2088/2605/2675`, `display_order.py:76-136`, `relationships.py:382/397`, `turn_phase.py:169/218`, `config_options.py:192-194`, `input_attempt.py:400/440/447` `public_status`): derived from the declared name, literal only where the text differs. - Sigils `#`/`@` spelled by hand at about 22 sites (`catalog_document.py:61-232`, `channels.py`, `channel_targets.py`, `goal_presentation.py:71-72`, `goal_actions.py:305/318`, …): owned by `Tag.channel` and `ThreadTarget.mention`. - Store and file names restated outside their class: `root / "wire"` in about 20 places across 17 modules (the virtual bus plan's done-grep for `_wire_lock_path` misses every `_store_lock(root / "wire")`; its grep gains that pattern), `~/.local/state/agent-comms` six times, bridge inbox dirs, `"registry.json"` at `installer/upgrade.py:566`, `".registry-owner-guard"` at `goal_history.py:208`, `active-route.json` five times, Pi `auth.json` in four modules: `X.path_in(root)` and one state-home owner. +CLI argument slice (#2626, base `acb2ddd0e`, 2026-10-23): the existing +`RegisteredWireMeta` now owns argument-model registration after Pydantic/ABC +construction. Each concrete declaration supplies its genuine external +`cli_names` tuple as the registry key; decode derives matching shapes from +that registry without retaining another alias map. Both production consumers, +Claude `ToolUse.call` and OpenCode `ToolState.call`, keep the shared decode +entrypoint. The seven shapes retain all 13 case-sensitive external spellings, +extra input, canonical previews and edit patches. Unknown names and invalid +arguments remain the original call; validation refusal belongs to its runtime. +Abstract specializations cannot overwrite concrete aliases. The existing +registry rejects duplicate tuples at declaration; overlapping nonidentical +tuples raise an explicit ambiguity at decode, before argument validation can +turn the conflict into an ordinary refused call. Overlap is not checked at +declaration. A new shape still needs only its class and alias declaration; +the improvement is shared declaration lifecycle and collision behavior, not +a claimed reduction from multiple authored rosters. + +NRA before/after source receipts cover Core/hooks/tests/tools, paired Toad and +the installed metaclass-registry/Pydantic source: 1,643/1,644 modules, +7,434/7,438 original classes, 7,144 projected classes in each, zero parse +omissions. The 290/294 unprojected originals remain explicit; the four added +classes are function-local declaration controls. Dynamic source projection +does not prove their runtime behavior. The compact index also leaves the +`AutoRegisterMeta` package re-export unresolved as a base of +`RegisteredWireMeta`; source inspection and a fresh-process runtime MRO read +confirm its composition with `ModelMetaclass`. Focused source checks pass 31 cases +(0.63s), exercising real Claude/OpenCode models, edit presentation and actual +fresh declaration lifecycles without patching registry state. The first run +passed 26 and failed two new assertions that incorrectly put call identity on +search preview records; identity is checked on its owning `ToolCall` and +emitted transcript instead. Receipts remain in the owned +`cli-tool-argument-family` scratch directory. Fresh CLI/UI-model import checks +also pass (two cases, 3.73s). This slice is a published draft, +not merged, installed or live-qualified. Shared phase proof belongs to its bus +owner; the other registries and the remaining name/sigil/path work keep 6.4 +open. + ### Wave 7: records where parameter lists and dicts stand in - `ManagedLaunchRequest`: nine parameters declared in seven signatures and forwarded six times (`native_child.py:222/341`, `native_pi.py:491/536`, `native_session_prepare.py:67/88`, `backend.py:112`, `stream_agent_events` has 23), with one lock-launch-fence procedure written twice. About 60. @@ -226,7 +263,7 @@ Waves 4 to 8 were written on 2026-10-21 and declared no phase in `plan_phases.py | wire_renames | 4.6 | no `WireName("admission…` in src | one unit, Toad in lockstep | | owned_dispatch | 5.4 | no `TurnInboxReceiver`, no hand `separators=(",", ":")` | one unit | | kind_families | 6.3 | no `TagAction`, `ExecutionOrigin`, `WhenBusy`, `ControlClassification`, `MembershipChange` or `ViewKind` enum classes | by file | -| rosters | 6.4 | no `PI_0_85_1_EVENT_KINDS`, `THREAD_CAPABILITIES` or `serves_new_threads` | one unit | +| rosters | 6.4 | internal registries and capability/name/path rosters derive from their owners; retain independent Pi producer/converter coverage | by family; shared phase proof has one owner | | records | 7 | no `ConnectivityFacet`/`observe_connectivity`; no `nonlocal` in the five document updaters | one unit | 9.4 is Tristan's ruleset change. 9.5, 9.6, 9.9, 9.10 and 9.11 are claude-ai's and are measured by their own numbers in this section: a touched-tests PR check, the full ratchet list in the job summary, commander calls under 3 s, one GitHub reader, and one stack lock. The order across plans stays Tristan's: the virtual bus batch first, then multiple inheritance, the commander-native plan, then this plan. Phases with no stored data run in parallel with all of them on disjoint files. diff --git a/src/agent_comms/tool_calls.py b/src/agent_comms/tool_calls.py index 8f75af3a04..8c14d6f287 100644 --- a/src/agent_comms/tool_calls.py +++ b/src/agent_comms/tool_calls.py @@ -11,11 +11,12 @@ from abc import abstractmethod from collections.abc import Mapping, Sequence from dataclasses import dataclass, replace -from typing import ClassVar, Unpack +from typing import ClassVar -from pydantic import AliasChoices, ConfigDict, Field, JsonValue, ValidationError +from metaclass_registry import RegistryConfig +from pydantic import AliasChoices, Field, JsonValue, ValidationError -from .cli_protocol import WireModel +from .cli_protocol import RegisteredWireMeta, WireModel from .field_codec import FieldCodec, MalformedRecord from .native_tools import BashTool, EditTool, GlobTool, GrepTool, ReadTool, WriteTool from .tool_declaration import ReplacementEdit, ToolDeclaration @@ -81,27 +82,31 @@ def ended(self, text: str, ok: bool, position: int | str, timestamp: float) -> T return replace(super().ended(text, ok, position, timestamp), diff=self.diff) -class CliToolArguments(WireModel): +class CliToolArguments(WireModel, metaclass=RegisteredWireMeta): """A runtime's arguments for one Core tool in its own spelling (Claude Code's, OpenCode's), read once into the Core record; the runtime's other keys stay in its raw input.""" cli_names: ClassVar[tuple[str, ...]] = () - types: ClassVar[dict[str, type[CliToolArguments]]] = {} + __registry_config__: ClassVar[RegistryConfig] = RegistryConfig( + registry_dict={}, key_attribute="cli_names", registry_name="CLI tool arguments", + ) + __registry__: ClassVar[dict[tuple[str, ...], type[CliToolArguments]]] model_config = {"extra": "allow"} - def __init_subclass__(cls, **kwargs: Unpack[ConfigDict]) -> None: - super().__init_subclass__(**kwargs) - cls.types.update(dict.fromkeys(cls.cli_names, cls)) - @classmethod def decode(cls, sent: ToolCall, arguments: Mapping[str, JsonValue]) -> ToolCall: """The call decoded when its name has a declared shape that its values fit, else as sent. The model can send values that do not fit (Claude refuses them with an error result).""" - if sent.name not in cls.types: + shapes = [shape for names, shape in cls.__registry__.items() if sent.name in names] + if not shapes: return sent + if len(shapes) > 1: + owners = ", ".join(shape.__name__ for shape in shapes) + raise ValueError(f"Ambiguous CLI tool name {sent.name!r}: {owners}") + shape, = shapes try: - return cls.types[sent.name].model_validate(arguments).call(sent.id) + return shape.model_validate(arguments).call(sent.id) except ValidationError, MalformedRecord: # kept as sent; its runtime answers the refusal return sent diff --git a/tests/test_cli_tool_arguments.py b/tests/test_cli_tool_arguments.py new file mode 100644 index 0000000000..a54bd41e19 --- /dev/null +++ b/tests/test_cli_tool_arguments.py @@ -0,0 +1,125 @@ +"""External tool spellings select one declared shape; refused calls stay as sent.""" + +import runpy +from abc import abstractmethod +from types import SimpleNamespace + +import pytest +from metaclass_registry import DuplicateRegistryKeyError + +from agent_comms import tool_calls +from agent_comms.cli_bridges.claude_protocol import ToolUse +from agent_comms.cli_bridges.opencode_parts import ToolPart +from agent_comms.native_tools import BashTool, EditTool, GlobTool, GrepTool, ReadTool, WriteTool +from agent_comms.tool_calls import CliToolArguments, ToolCall + + +@pytest.mark.parametrize('name,arguments,tool', [ + ('bash', {'command': 'pwd'}, BashTool), + ('grep', {'pattern': 'needle'}, GrepTool), + ('glob', {'pattern': '*.py'}, GlobTool), + ('read', {'filePath': 'owner.py'}, ReadTool), + ('edit', {'filePath': 'owner.py', 'oldString': 'before', 'newString': 'after'}, EditTool), + ('write', {'filePath': 'owner.py', 'content': 'after'}, WriteTool), +]) +def test_opencode_spellings_reach_native_records_with_extra_input(name, arguments, tool): + arguments = {**arguments, 'provider_detail': ['retained']} + part = ToolPart.model_validate({ + 'type': 'tool', 'id': 'part', 'sessionID': 'session', 'messageID': 'message', + 'callID': 'call', 'tool': name, 'state': {'status': 'pending', 'input': arguments}, + }) + call = part.state.call(part) + assert type(call.tool) is tool + assert call.id == 'call' + assert call.arguments['provider_detail'] == ['retained'] + started = call.started('part', 1.0) + assert started.tool_call_id == call.id + assert started.tool == call.tool + + +def test_claude_glob_spelling_uses_its_own_search_record(): + call = ToolUse(id='call', name='Glob', input={'pattern': '*.py'}).call() + assert call.tool == GlobTool(pattern='*.py') + + +@pytest.mark.parametrize('name,arguments', [ + ('mcp__custom', {'private': ['untouched']}), + ('BASH', {'command': 'pwd'}), + ('Glob', {}), + ('Bash', {'command': 17}), + ('Grep', {'pattern': 'needle', 'path': 17}), +]) +def test_unknown_or_invalid_calls_remain_the_same_original_object(name, arguments): + sent = ToolCall('refused', name, arguments) + assert CliToolArguments.decode(sent, arguments) is sent + assert ToolUse(id=sent.id, name=name, input=arguments).call() == sent + + +@pytest.mark.parametrize('name,arguments', [ + ('external_custom', {'private': ['untouched']}), + ('glob', {}), + ('bash', {'command': 17}), +]) +def test_opencode_unknown_or_invalid_calls_stay_as_sent(name, arguments): + part = ToolPart.model_validate({ + 'type': 'tool', 'id': 'part', 'sessionID': 'session', 'messageID': 'message', + 'callID': 'call', 'tool': name, 'state': {'status': 'pending', 'input': arguments}, + }) + assert part.state.call(part) == ToolCall('call', name, arguments) + + +@pytest.fixture +def argument_family(): + # Execute the actual declaration lifecycle; the canonical family stays untouched. + return SimpleNamespace(**runpy.run_path(tool_calls.__file__, run_name=tool_calls.__name__)) + + +def test_new_shape_is_discovered_from_its_alias_declaration(argument_family): + class InspectArguments(argument_family.CliToolArguments): + cli_names = ('Inspect', 'inspect') + path: str + + def record(self, *, call_id: str) -> ReadTool: + return ReadTool(call_id=call_id, path=self.path) + + for name in InspectArguments.cli_names: + arguments = {'path': 'new-owner.py'} + sent = argument_family.ToolCall('new', name, arguments) + assert argument_family.CliToolArguments.decode(sent, arguments).tool == ReadTool( + call_id='new', path='new-owner.py', + ) + + +def test_abstract_specialization_cannot_take_a_concrete_alias(argument_family): + class IncompleteRead(argument_family.CliReadArguments): + @abstractmethod + def unfinished(self): ... + + with pytest.raises(TypeError, match='abstract'): + IncompleteRead(file_path='owner.py') + arguments = {'file_path': 'owner.py'} + sent = argument_family.ToolCall('read', 'Read', arguments) + assert argument_family.CliToolArguments.decode(sent, arguments).tool == ReadTool( + call_id='read', path='owner.py', + ) + + +def test_duplicate_alias_group_is_rejected_by_declaration(argument_family): + with pytest.raises(DuplicateRegistryKeyError): + class CompetingRead(argument_family.CliReadArguments): + pass + + +def test_overlapping_alias_groups_cannot_silently_choose_an_owner(argument_family): + class OverlappingRead(argument_family.CliReadArguments): + cli_names = ('Read', 'ReadAgain') + + for arguments in ({'file_path': 'owner.py'}, {}): + sent = argument_family.ToolCall('ambiguous', 'Read', arguments) + with pytest.raises(ValueError, match="Ambiguous CLI tool name 'Read'.*CliReadArguments.*OverlappingRead"): + argument_family.CliToolArguments.decode(sent, arguments) + arguments = {'file_path': 'owner.py'} + sent = argument_family.ToolCall('unique', 'ReadAgain', arguments) + assert argument_family.CliToolArguments.decode(sent, arguments).tool == ReadTool( + call_id='unique', path='owner.py', + )