Repository navigation
Put Claude behind an AgentProviderAdapter so a second product is an impl - #71
Merged
Merged
Conversation
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
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.
Task 5 of the multi-provider plan: Claude's product-specific behaviour moves behind one interface, so the second product becomes an implementation rather than a new branch in each of
discovery,process,transcriptandterminals.Parity holds by construction, not by inspection
Every
ClaudeProvidermethod delegates to the function that already implemented it:Nothing was moved into the adapter. There is still exactly one implementation of each behaviour, now reachable through a trait — so "did the extraction change anything?" is answerable by reading the delegations rather than by diffing logic.
Trait methods return plain data and do no I/O; spawning, reading and writing stay in the wrappers that already own them.
A missing adapter is
None, not a stand-inMy first draft mapped Codex to
ClaudeProvider"since nothing constructs a Codex session yet". That is the silent-wrong-answer shape: it would parse a Codex transcript with Claude's schema and launch the wrong binary, succeeding at both.Nonemakes callers decide, andcodex_has_no_adapter_yet_and_does_not_fall_back_to_claudeis the tripwire that forces Task 6 to fill it in.Written test-first
The contract tests were written before the module existed and confirmed red (
unresolved import agentctl::providers). They state what Claude does today, so they are parity tests first and design tests second — notably that process matching keeps its current strictness, rejectingclaudectl --list,agentctl --list,grep claudeand/usr/bin/env claude, and that resume precedes prompt in the launch argv.Proven to catch a real regression by making the adapter report Codex's transcript layout:
That second test pins
discovery::projects_dir()to the literal path it returned before the change — a wrong transcript root would otherwise surface as an empty dashboard, not a failing test.One test was also checked for vacuity:
claude_transcript_parsing_is_unchangedcompared twois_some()calls, which would hold just as well if the sample stopped parsing and both sides wentNone. It now asserts the positive case first.Dispatch, and what is deliberately deferred
Two call sites now go through the adapter:
discovery::projects_dirand the executable interminals::build_claude_command.The remaining sites —
is_claude_processin thepsscan, andbuild_claude_argsin the five terminal backends — are not converted here. Neither has anAgentProviderin hand at the point of call; supplying one is process discovery, which is exactly what Task 6 specifies. Threading it now would mean inventing that design a task early and guessing at it.Verification
cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean — clippy caught an unused trait import, since&dyn Traitmethod calls resolve without the trait in scope.753 lib tests pass plus 8 new contract tests. The two
usage_ledgerperf failures are the known pair on a loaded box (this run took 960s):perf_eviction_stays_linear_with_many_rowsis a wall-clock assertion, and when it panics it poisonscache_test_lock, takingperf_summarize_cached_costs_far_less_than_parsingwith it. Both pass single-threaded and predate this branch.