style: comment sweep — trim WHAT, expand contracts, fix structural gaps - #65
Merged
Conversation
…l width Apply CLAUDE.md comment conventions across all source files: - Remove comments that restate WHAT the code does (well-named identifiers suffice) - Trim multi-line doc comments to one-liners where the contract is simple - Reflow surviving multi-line comments from ~80-col to 100-col width - Keep WHY comments: invariants, trade-offs, workarounds, hidden constraints
…modules Follow-up pass on 1cd9489. Targets 3+ line `///` blocks and inline `//` blocks that restate the code rather than the WHY. Also fixes two test-organization issues uncovered while reading: - model.rs: moved `structured_outputs_flag_tracks_upstream_allowlist` from the `Capabilities::clamp_effort` section to `capability rows` where it belongs alongside the other per-row flag tests. - model.rs: reordered tests in the `lookup` and `marketing_name` sections to follow happy → variant → edge convention. - numbered_row.rs: removed duplicate `// ── Renderer::render ──` section header and folded the wrap test back into the first section. cargo fmt / clippy / test all clean; 1609 tests pass.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…fall short Pass over every module to add or expand doc comments where the existing one-liner did not carry the contract. Targets non-trivial pub / pub(crate) items whose preconditions, invariants, error modes, or trade-offs were left implicit. No code logic changes; coverage holds at 98.47% line / 97.97% function. Highlights: - agent: TurnAbort variant rationale; agent_turn round semantics, mid-turn submit splicing, abort error matrix. - agent/event: AgentSink contract (cheap-to-call, drop-tolerant), AGENT_EVENT_CHANNEL_CAP rationale (TUI per-frame drain). - client/anthropic: stream_message cache / billing invariants, beta-header ordering for 3P fingerprinting, CreateMessageRequest field-order contract for cch placeholder. - client/anthropic/sse: graceful-drop semantics, single-frame poison resilience, data: gateway quirk. - file_tracker: Read-before-Edit invariants (read-before-edit, full-read-required, freshness), resume verification flow. - session/state: WriterStatus lifecycle (Pending defers create, Broken auto-recovers via reopen). - tool: Tool trait per-instance metadata convention; cap_output head+tail truncation, MAX_OUTPUT_BYTES last-line defense. - tool/edit: exact-match uniqueness, Read-before-Edit gate, CRLF preservation, chunk-shift invariant. - tui/app: tokio::select! arm fairness + 60 FPS render coalescing, modal-gate exclusivity, busy-gating in dispatch_user_action. - tui/markdown/render: stateful walker contract (push/pop on Start/End, needs_newline gating). - tui/terminal: panic-hook restore-before-delegate, DEC mode 2026 sync output rationale. Reflowed pre-existing 80-col comments encountered along the way. Tightened a handful of comments that restated WHAT.
`cap_output`, `parse_input`, `summarize_path_call`, `display_path`, `file_name`, `entry_mtime`, and `truncate_line` carried non-obvious contracts (silent epoch fallback, `Cow::Borrowed` no-alloc fast path, `Err = ToolOutput` shape, footer suffix, filename-when-equal behavior) that the signature alone didn't surface. Promoted `cap_output`'s `//` block to `///`. Added struct docs on `ToolRegistry` and `result_view` clarifying the error short-circuit. Moved `display_cwd_path_from_without_cwd_falls_back_to_original_path` out of the `// ── Tool trait contract ──` test section into its own `// ── display_cwd_path_from ──` divider between `resolve_base_dir` and `display_path`, mirroring the production order.
…ction order The divider sits between `view_for` and `strip_bom` in the test module — that position mirrors `read_excerpt_view`'s production location, not `ReadTool::result_view`'s. The tests do call `result_view`, but it's a thin trait-method shim that delegates to `read_excerpt_view`; the helper is what's actually being exercised by every assertion.
Three structural fixes to `session/handle.rs` test grouping: - `// ── SharedState poison recovery ──` was the LAST section but `SharedState` is the first production type (line 45, before `SessionHandle` at 99). Moved the two `shared_state_*` tests to a new top-level `// ── SharedState ──` section; folded `shutdown_recovers_from_poisoned_actor_join_mutex` into the existing `// ── shutdown ──` section since it tests `shutdown`, not `SharedState`. - `// ── file-snapshot persistence ──` named after a feature, not a function — breaks the `// ── function_name ──` rule. Folded its 4 tests into existing sections: `finish_persists_one_file_snapshot_per_tracked_file` into `// ── finish ──`; the three `resume_*_snapshots*` into `// ── resume ──`. - Reordered tests within `// ── resume ──` to follow happy → variant → edge (basic load → unfinished resume → tool-use sanitization → parent chain → snapshot variants → drift / missing-file edges).
`Config::load / prompt_cache_ttl` was isolated at the end of the test module, splitting the three `Config::load` sub-sections apart by unrelated `Config::snapshot` / `display_effort` / `default_max_tokens` tests. Folded it adjacent to `Config::load / effort resolution`, then put `Config::snapshot` before `display_effort` to match the production order (`Config::snapshot` is `impl Config`, defined before the free `display_effort` helper).
`GatePurpose`, `FileSnapshot`, `RecordRead`, `StatCheck`, and `GateError` were sectioned under their own dividers but undocumented, while their siblings (`FileTracker`, `LastView`, `CACHE_HIT_STUB`) had rich contract docs. Closes that consistency gap. The new docs explain why each type exists rather than restating its variant names: `GatePurpose` is purely for `verb()` rendering, `StatCheck::NeedsBytes` carries the hash forward to the bytes-verifier, `RecordRead::CacheHit` is the signal to return the stub instead of bytes.
…sections `ModelInfo` and `Capabilities` had documented fields but empty struct intros, unlike `ResolvedModelId` which carries a one-liner. Added struct docs that explain why the table exists (substring match against alias-resolved id) and why `Capabilities` keeps every flag as a separate Bool (each maps 1:1 to a distinct upstream switch). Added `// ── Capabilities::accepts_effort ──` and `// ── Capabilities::default_effort ──` test sections to mirror production order. Both functions were previously exercised only transitively by `clamp_effort` / `resolve_effort` tests.
…RRUPTED_MARKER `ConfigChanged` was the only `AgentEvent` variant carrying a doc — the inconsistency stood out. Added one-liners on every other variant so the enum reads as a uniform protocol description; each note explains how the UI is expected to react (render, splice, clear chrome) rather than restating the field shape. `INTERRUPTED_MARKER` is consumed as user-visible chat text by the system block layer; one-liner pins that contract.
Mirrors the `///` doc on `title_generator.rs::SYSTEM_PROMPT` so both large indoc-blocked system prompts in the crate get the same treatment. The doc explains intent (author or propose-diff against existing) and the load-bearing constraints (ground in actually-read files, exclude generic boilerplate).
…elper - `is_default` was at line 5, above the `ContentBlock::ToolResult` variant that uses it via `#[serde(skip_serializing_if = ...)]`. Convention puts helpers after their callers; moved between `ContentBlock` and `Message` so the read order is types → helper → next type. - Added `// ── Role ──`, `// ── ContentBlock ──`, `// ── Message ──` dividers to match the existing `// ── Message normalization ──` divider; the production half of the file now has the same logical structure as the test module below it. Test section divider names (`// ── ContentBlock::Variant ──`) intentionally left as-is — the variant is what's being tested (serde-derived shape), not a specific function.
Two `SessionInfo` types coexisted: `session::entry::SessionInfo` (persisted JSONL record consumed by `--list`) and `slash::context::SessionInfo` (live runtime snapshot handed to slash commands). Same name, different concepts — caused reader confusion and ambiguous `use ...::SessionInfo` imports. Renamed the slash one to `LiveSessionInfo` to make the runtime semantics explicit and leave the persistence type with the bare name. Touch points: `slash/context.rs`, the `slash` re-export, all consumers under `slash/`, plus `main.rs` and `tui/app.rs`. The persisted `SessionInfo` in `session/` is unchanged.
…rename The crate-tree convention is alphabetical with each `dir/` adjacent to its parent `.rs` file. The `tui/` subtree had `theme.rs` + `theme/` listed between `components.rs` and `components/`, splitting the components block apart and putting theme far ahead of its alphabetical position. Restored alphabetical order: components → event → glyphs → markdown → modal → pending_calls → terminal → theme → wrap, with each subdirectory immediately after its parent file. Also updated `slash/context.rs` description to reflect the `SessionInfo` → `LiveSessionInfo` rename.
PR #64 (modal infrastructure) shipped Option C: bare /model opens the combined picker, bare /effort errors with a usage hint pointing at /model. The user guide, design notes, and roadmap still described the older "both bare forms open the picker with different initial focus" shape. Updated: - docs/guide/slash-commands.md — table description, mid-turn classification paragraph, and the "Switching the Effort" / "Switching the Model" sections. - docs/design/slash/commands.md — design decision #5, /effort and /model per-command notes, source list (`agent_loop_task` → `agent_turn`). - docs/design/slash/modals.md — design decisions #4 (`SessionInfo` → `LiveSessionInfo`) and #7 (typed-arg-only contract). - docs/roadmap.md — moved the combined picker out of "Current Focus" (shipped in PR #64) into Working Today; replaced with the deferred /effort slider. - CLAUDE.md — `slash/effort.rs` description updated to match the typed-arg contract.
Three small fixes to make the research docs read uniformly: - `api/system-prompt.md`: pin to v2.1.87 like every other api / session / tools / tui research doc (was the only one stamped v2.1.101). - `tui/cancellation.md`: order reference projects as Claude Code → OpenAI Codex → opencode, matching every other multi-project research doc. - `slash/modals.md`: drop the `> **Status:** ... shipped shape` callout. No other research doc carries a per-doc status line, and the research↔design relationship is already documented at the index level (`docs/research/README.md`).
After auditing all .md docs end-to-end:
- Em-dash convention (CLAUDE.md says use `—`, not `--`). Normalized 10 docs
that still used double-hyphen in prose: every `docs/design/*` and the
remaining `docs/research/{session,slash,tui}/*` files. The user-facing
guides and roadmap already followed this convention.
- `research/slash/{commands,modals}.md` intros aligned with the standard
`Based on [Claude Code] (v2.1.87), [OpenAI Codex], and [opencode]` pattern.
The "Cross-checked / Verified against locally-mirrored sources (2026-05-05)"
variant was unique to these two; the version stamp carries the same
provenance signal as the rest of the research family.
- `design/slash/modals.md`: dropped the "Research:" pointer in the intro
(no other design doc carries one — the README pairs them at the index
level). Kept the "Companion: [commands.md]" sibling-doc pointer since the
two design docs are tightly paired and there's no equivalent elsewhere.
- `design/slash/modals.md` Sources: converted from linked paths back to
bare-backtick paths to match the majority style in other design docs.
Linked paths with `../../../` traversal were fragile and inconsistent.
- `design/tui/cancellation.md`: fixed stale path
`crates/oxide-code/src/agent/pending_calls.rs` → `tui/pending_calls.rs`
(the file lives under `tui/`, not `agent/`).
- `research/tui/overview.md`: fixed table column alignment broken by the
em-dash substitution (`--` is 2 chars, `—` is 1).Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Summary
Crate-wide cleanup applying the
CLAUDE.mdcomment conventions: trims//blocks that restate WHAT the code does, expands one-line///docs into proper contract descriptions where signatures alone left preconditions / invariants / error modes implicit, and reflows surviving multi-line comments from ~80-col to the 100-colrustfmtmax_width. Net: 1827 insertions, 3632 deletions across 110 files; coverage holds at 98.47% line / 97.97% function.Notable structural changes
slash::SessionInfo→LiveSessionInfoto disambiguate fromsession::entry::SessionInfo(persisted JSONL record). Two same-named types in different modules caused reader confusion and ambiguous imports.session/handle.rs(SharedState first, file-snapshot tests folded intofinish/resume),config.rs(Config::load sub-sections grouped, snapshot before display_effort),tool.rs(misplaced display_cwd_path_from test moved to its own section), and added missingCapabilities::accepts_effort/default_effortsections inmodel.rs.file_tracker.rs(GatePurpose,FileSnapshot,RecordRead,StatCheck,GateError) gained the contract docs their documented siblings already had./efforttyped-arg-only contract — guide, design notes, and roadmap (combined picker moved from Current Focus to Working Today).Conventions enforced
///— collapse to one line for simple contracts; expand to paragraphs + sublists when multiple facts need to land.—,→) count as one column, not three. Sublist continuations indent 2 spaces under-to satisfyclippy::doc_overindented_list_items.Test plan
cargo fmt --all --check,cargo build,cargo clippy --all-targets -- -D warnings— cleancargo test— 1612 tests passcargo llvm-cov --ignore-filename-regex 'main\.rs'— 98.47% line / 97.97% function (held)pnpm lint,pnpm spellcheck— clean