Repository navigation
refactor: Assorted cleanups - #601
Open
jcarver989 wants to merge 26 commits into
Open
jcarver989 wants to merge 26 commits into
jcarver989 wants to merge 26 commits into
Conversation
Delete code with no callers outside its own definition and self-tests: StructuredAgentOutput/ArtifactReference and extract_json_from_markdown, the task archive/remove path (archive_tree, remove_tree, remove, all_tasks), and the never-constructed TaskStoreError variants InvalidParent and ValidationError. Collapse the three hand-built SubAgentResult literals into SubAgentResult::new. Drop the deprecated, ignored skills --notes-dir flag. Callers still passing it will now fail argument parsing.
Delete code with no callers outside its own definition and self-tests: unused web search/fetch constructors and fake-client helpers, the panicking WebSearcher::default, never-constructed WebFetchError, LspError and FileError variants, the LspError Result alias, unused LspRegistry/DiagnosticCounts/WorkspacePaths/PromptRuleMatcher methods, and the file_io re-export module. Drop the never-read skip_serializing fields ReadFileResult.raw_content and EditFileResponse.content, which also removes a full-file clone per edit.
Delete from_lsp_id, extensions_for_alias and LanguageId::primary_extension, which had no callers, along with the aliases and primary_extension metadata fields that only they read.
Delete unused test helpers (TestTerminal, FakeFilesystem accessors and settings, FakeExecutor::filesystem, FakeGit::files), never-read Theme fields and LocalConfigValue.raw_meta. App::on_ui_event already drops key releases and maps repeats to presses, so the per-surface is_press guards could never reject a key; remove them and is_press itself. SettingsOverlay intercepts Esc before any pane sees it, so the panes' Esc and Nav::Close arms were unreachable. Drop the transcript commit.rows == 0 early return, which computed the same lines as the general path, and route ArtifactReviewScreen's submission through ElicitationResponder::accept_strings instead of rebuilding the content map by hand.
AcpOptions is deserialize-only, so its skip_serializing_if attributes never ran and missing Option fields already default to None; the generated schema is unchanged. Collapse the single-variant AcpRunOutcome into Result<(), AcpRunError>, drop the AgentKey argument that every RuntimeFactory::spawn impl ignored, inline the supports_prompt_image/audio aliases, and delete the headless::error re-export module.
Delete the aether-core anthropic_test example, which never used aether-core and ignored its --no-cache flag, along with the clap and chrono dev-dependencies only it needed. Remove the test-only Prompt::build_all and port its tests to PromptCache::render. Collapse JudgeError::InvalidJudgment to a single reason string since raw_response was always empty. Drop the uncalled BrowserOAuthHandler::new and wait_for_callback, and make open_browser private.
Add utils::testing::TestWorkspace behind a new `testing` feature and build every crate's temp-dir fixtures on it, replacing five near-identical implementations. - mcp-servers re-exports it in place of its own copy, drops create_test_files for TestWorkspace::files, and rebuilds CodingWorkspace on it. - aether-lspd replaces the TestProject trait and CargoProject / NodeProject with cargo_project / node_project / file_uri, and shares one remove_stale_daemon_files between TestDaemon and DaemonHarness. - aether-project's project() / home() return a TestWorkspace; the skill / settings / aether helpers become explicit paths. - wisp's attachment and dropped-file tests use it instead of ad hoc temp-file helpers. aether-lspd gains an optional aether-utils dependency for its testing feature.
Add git, git_init and git_commit to utils::testing, plus TestWorkspace::git_init / commit / git for repos at the workspace root, and build every crate's git fixtures on them. - git_init pins the branch to main, sets a test identity and disables templates, hooks and commit signing so host git config can't leak in. - aether-evals' git_repo tests drop 27 raw git commands that ignored the exit status; every helper call now asserts success. - wisp drops its Repo support module for TestWorkspace. - aether-cli keeps only init_repo, makes test clones with StdCopyCloner like the product does, and compiles workspace::testing only under test or the testing feature.
Add utils::git with git / git_with_stdin and a GitError carrying the failed command and its stderr, and build aether-cli's workspace git module, aether-evals' GitRepo and the git test helpers on it. - The wrapper returns raw stdout bytes so binary patches and non-UTF-8 paths round-trip intact; callers convert to text where they need it. - aether-evals drops run_git and GitRepoError, whose eight per-operation variants carried nothing beyond the command's stderr. - aether-cli's copy_untracked_files returns io::Error, which WorkspaceError::Io already covers.
McpGateway::statuses now returns utils' McpServerStatusEntry instead of its own ServerStatus / ServerState, which mirrored McpServerStatus and McpServerAuthCapability variant for variant. The catalog's 1:1 mapping collapses to setting the deferred-tools flag. mcp-utils gains an aether-utils dependency behind its client feature.
Move the generic metadata helpers from acp-utils into utils, since ACP's Meta is a plain JSON map, and build ArtifactReviewElicitationMeta on them instead of its own copies. - ArtifactReviewElicitationMeta::to_json becomes an infallible to_meta; the struct always serializes to an object, so the error path in mcp-servers' review tool was dead. - ArtifactReviewElicitationMeta::parse becomes from_meta, matching RemoteServerInfo::from_meta.
AgentSpec and AgentSpecExposure now derive Default, so test specs and AgentSpec::bare set only the fields they care about and fill the rest with ..AgentSpec::default(). The derived exposure default replaces AgentSpecExposure::none(). The production literal in agent_catalog keeps every field explicit so a new AgentSpec field cannot silently fall back to a default there.
…ctors Tests across evals, sessions, telemetry, aether-core and aether-cli hand-built ToolEvent::Call / Result / Error literals around ToolCallRequest / ToolCallResult / ToolCallError. Add constructors next to AgentEvent::text and turn_ended and use them instead. aether-cli's output and headless tests also shared identical progress, retry, LLM-call and usage fixtures; they now live once in a test-only output::test_events module.
- wisp's WorkspaceStatus is now a re-export of acp-utils' WorkspaceStatusResponse, which had the same fields, with a free initial_workspace_status(cwd) in place of the inherent constructors and the From conversion. - wisp exports init_file_logging(dir, file_name, default_filter) and aether-cli's ACP mode uses it instead of its own copy, dropping aether-cli's tracing-appender dependency. The ACP log file now writes one line per event instead of the multi-line pretty format, and logging setup uses try_init like wisp.
expand_api_template and TemplateError re-implemented the ${VAR}
expansion utils::Vars already provides. MantleClient now builds a Vars
once with the AWS region placeholders and expands catalog templates
through it, replacing the region field it only kept for expansion.
An unterminated "${" is now left literal rather than erroring, and an
empty environment value is substituted rather than treated as missing;
the catalog test still asserts every shipped template fully resolves.
perform_codex_oauth_flow now builds one BasicClient and uses it for both the authorize URL and the token exchange. The authorize URL comes from authorize_url(CsrfToken::new_random) with scopes, PKCE and the Codex extra params instead of a hand-assembled query, the hardcoded endpoint URLs use expect() like CodexTokenManager::new, and callback parsing looks up the two query params it needs. Drop generate_random_state and the tautological URL and constant tests.
- Anthropic marks the x-api-key header sensitive and logs the headers' Debug output, replacing the hand-rolled redaction helpers and tests. - Codex logs its JSON request body through Value's Display. - ModelSpecError and ReasoningEffortError derive thiserror::Error; InvalidModel's `source: String` becomes `reason` so it is not mistaken for an error source. - Bedrock builds converse_stream with the Option-taking setters and logs the request once. Bedrock's two error classifiers stay separate: event-stream exceptions carry no error code in their metadata, so classifying by code() would make mid-stream throttling non-retryable.
The encrypted credential file hand-rolled age's Encryptor / Decryptor plumbing with a map_err per step. Use age's one-shot encrypt / decrypt helpers, keeping the scrypt work-factor override. Decryption failures now share one "Decryption failed — wrong passphrase or corrupted file" message, which still carries age's specific cause.
WizardTerminal hand-rolled raw mode, the alternate screen, cursor hiding and keyboard enhancement plus their teardown. It now wraps ratatui::try_init() and calls ratatui::restore() on drop; ratatui's Terminal shows the cursor again when it drops. Keyboard enhancement is gone since the wizard only reads arrows, Space, Enter and Esc. The single- and multi-select loops and their render functions collapse into one Picker driven by run_picker; single-select treats the cursor as the selection.
The process transport hand-rolled Content-Length framing on a reader task feeding an mpsc channel, dispatched untyped JSON, and wrote frames by hand. It now holds a FramedRead / FramedWrite over the child's stdio with one LspCodec, decodes messages into a typed Incoming, and sends through actor methods. FramedRead::next() is cancel-safe in select!, so the reader task and its channel are gone. A decode error would end the FramedRead stream, so the codec logs and skips frames with an invalid JSON body or no Content-Length, as the old reader loop did. The fake LSP server gains --garble-before to cover this end to end.
PidLockfile::acquire now uses std's File::try_lock instead of an unsafe, unix-only libc::flock call, keeping the WouldBlock "held by another process" error. The lock now also applies on non-unix targets. Drop the test-only read_pid, is_process_running and is_stale helpers and their tests, and assert the error kind when a second acquire fails.
opentelemetry implements Injector and Extractor for HashMap<String, String>, so the W3cCarrier adapter and its trait impls are unnecessary. Inject into a map and take traceparent / tracestate out of it; extract from a map built from the two TraceContext fields.
PromptSource and McpSourceSpec hand-wrote Serialize, Deserialize and JsonSchema around utils' Deserialize-only PathOrObject. PathOrObject now derives all three, and both types use serde(from/into) and schemars(with) on it, like ToolExposure, leaving a pair of From impls each. string_or_object_schema is gone. Schema change: PromptSource and McpSourceSpec become anyOf [ResourcePath, <Object>] instead of oneOf [string, <Object>]. ResourcePath is a string, names and descriptions are unchanged, the website loader handles anyOf, and the generated SDK types still typecheck.
ratatui implements From<Color>, From<(Color, Color)>, From<(Color, Modifier)> and From<(Color, Color, Modifier)> for Style as exactly the equivalent fg / bg / add_modifier chains. Pass colors and tuples directly where an Into<Style> is accepted and use Style::from elsewhere, replacing every Style::new().fg(..) chain in wisp. ListView::highlight_style takes impl Into<Style> like ratatui's builders, prompt_spans takes its two colors as Color, and the status-line styled() helper is gone in favour of Span::styled.
- aether-cli maps tool-call input with MaybeUndefined's From<Option<Option<T>>> instead of json_patch_value. - aether-core names task statuses by serializing rmcp's snake_case TaskStatus, keeping an "unknown" fallback. - wisp drops cursor_at_match_end since EditBuffer::set_cursor already clamps, and set_cursor uses str::floor_char_boundary. - aether-cli parses slash commands with strip_prefix + split_once, dropping the SlashCommand struct.
jcarver989
enabled auto-merge (squash)
October 13, 2026 04:26
jcarver989
disabled auto-merge
October 13, 2026 04:29
This branch has not been deployed
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
1 participant
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.
This PR contains a number of assorted cleanups to remove unused code, cleanup functions etc.