claw-code/docs/reviewer-feedback-sprint.md

Reviewer Feedback Sprint

Execution Log

Time	PR	Action	Status
2026-06-02	#3211	Plan written at docs/pr-3211-plan.md	✅ Plan approved
2026-06-02	#3211	Executed: extracted read_env_non_empty + resolve_base_url into mod.rs	✅ Committed
2026-06-02	#3211	Pushed commit c0855e12 to worktree-provider-config-fallback	✅ Pushed
2026-06-02	#3211	CI: cargo fmt ✅, cargo test (147 unit + 37 integration) ✅, cargo check ✅	✅ Verified
2026-06-02	#3211	Pre-existing clippy issues in runtime/trident.rs (not from our changes) — noted	⚠️ Known
2026-06-02	#3214	Plan written at docs/pr-3214-plan.md	✅ Plan approved
2026-06-02	#3214	All 3 CI failures (build ❌ test ❌ fmt ❌) traced to one root cause: missing retry_after: None in main.rs test	✅ Root caused
2026-06-02	#3214	Fixed: added retry_after: None, removed duplicate #[must_use], ran cargo fmt --all	✅ Committed
2026-06-02	#3214	Pushed commit 76783377 to worktree-api-timeout-retry-v2	✅ Pushed
2026-06-02	#3214	Reviewer Q "Are defaults preserved?" — YES: ApiTimeoutConfig defaults 30s/300s/8, with_retry_policy() is opt-in	✅ Answered
2026-06-02	#3216	Plan written at docs/pr-3216-plan.md	✅ Plan approved
2026-06-02	#3216	Root cause: test creates sessions with 0 messages → filtered out by message_count > 0	✅ Root caused
2026-06-02	#3216	Fix: test now adds push_user_text() before saving; added format_all_sessions_empty() error message	✅ Committed
2026-06-02	#3216	Added test latest_session_returns_all_empty_error_when_sessions_exist_but_have_no_messages	✅ New test
2026-06-02	#3216	Pushed commit 41034bb3 to worktree-session-resume-fixes	✅ Pushed
2026-06-02	#3217	Plan written at docs/pr-3217-plan.md — trivial: just cargo fmt	✅ Plan approved
2026-06-02	#3217	Committed pre-existing fmt changes, pushed 592bae5	✅ Done
2026-06-02	#3218	Plan written at docs/pr-3218-plan.md — slash command count off by 1	✅ Plan approved
2026-06-02	#3218	Fixed: count 139→140, added /setup assertion, pushed 1122b16	✅ Done
2026-06-02	#3219	Plan written at docs/pr-3219-plan.md — CI count fix + implement /lsp status	✅ Plan approved
2026-06-02	#3219	Fixed CI: count 139→140, added /lsp assertion	✅ CI fix
2026-06-02	#3219	Exported check_lsp_availability, LspInstallAction, InstallInstruction, format_install_prompt from runtime	✅ Exports
2026-06-02	#3219	Implemented /lsp status (✅❌⚠️), /lsp list, /lsp start/stop handler replacing stub	✅ Feature
2026-06-02	#3219	Pushed commit 369452c to worktree-lsp-v3	✅ Pushed

2026-06-04 — Round 2: upstream-merge cycle (new reviewer comments + conflicts)

Reviewer (1716775457damn) left new comments on 5 PRs. Root cause across all: every PR was cut from stale upstream/main @ 4d3dc5b; upstream advanced to 4619375 (config/runtime refactor → conflicts + flaky CI). Fix = merge current upstream + address each specific ask. Investigated with 5 read-only agents, fixed with 5 agents in isolated worktrees, each diff reviewed (no leftover markers, feature preserved, build + targeted tests green, remote SHA verified).

Time	PR	Action	Status
2026-06-04	#3214	Reviewer: resolve conflicts + clarify 5-vs-3 commits. Merged upstream; threaded api_timeout through new build_runtime_config helper; conflicts in config.rs/lib.rs	✅ Resolved
2026-06-04	#3214	Verified: 158+13+11+4+7 api tests pass, no leftover markers, MERGEABLE/CLEAN. Pushed merge 9e50cb6	✅ Pushed
2026-06-04	#3216	Reviewer: fix 3 CI checks + add exclude_id tests. Clean merge; cargo fmt (real failure); added 3 tests (exclude_id skip, 0-msg filter, resolve_reference_excluding)	✅ Resolved
2026-06-04	#3216	Verified: 21 session_control tests pass, fmt clean, MERGEABLE/CLEAN. Pushed 346772a. Note: build/test reds were env-only subagentModel config-parse, not PR-caused	✅ Pushed
2026-06-04	#3217	Reviewer: fix 2 CI checks + integration test. Clean merge refreshed flaky arg-parse tests; extracted PRESERVE_SCHEDULE const + added bounds test	✅ Resolved
2026-06-04	#3217	Verified: 206 CLI tests pass incl. previously-flaky pair, MERGEABLE/CLEAN. Pushed 77c7d08	✅ Pushed
2026-06-04	#3218	Reviewer: resolve conflicts + document subagentModel. 4-file merge (kept upstream Doctor{output_format,permission_mode} sig + this PR's setup arm); doc comment on TOP_LEVEL_FIELDS	✅ Resolved
2026-06-04	#3218	Verified: 24 config_validate tests pass, builds clean, MERGEABLE/CLEAN. Pushed d54f028	✅ Pushed
2026-06-04	#3219	Reviewer: resolve conflicts + nightly rustup probe. 3-file merge; rustup_component_works() now probes stable→nightly, returns working toolchain; both call sites updated	✅ Resolved
2026-06-04	#3219	Verified: 58 LSP tests pass, builds clean, MERGEABLE (CI re-running). Pushed a0e9833	✅ Pushed
2026-06-04	#3211	Resolved merge conflicts with upstream/main (config.rs + lib.rs); preserved RuntimeProviderConfig + read_env_non_empty/resolve_base_url helpers; rebuilt corrupted test section	✅ Resolved
2026-06-04	#3211	Verified: api + runtime crates compile, shared helpers intact in mod.rs, 67da2dc on worktree-provider-config-fallback	✅ Pushed
2026-06-04	#3211	Posted maintainer summary on PR #3211: shared helpers explained, from_env vs from_env_or_saved naming rationale, merge details	✅ Commented

Date: 2026-06-03 Upstream: ultraworkers/claw-code

---

Closed PRs — Done, No Action

PR	What happened
#3212	Owner merged equivalent (1bd18be)
#3215	Owner closed — wants internal doc pointers, not external links
#3220	Owner merged equivalent (9c8375d) with validation/tests added

---

Open PRs — Reality Check

#	PR	Title	build	test	fmt	mergeable	Reviewer	Difficulty
①	#3217	Auto-compact retry	✅	✅	❌	YES	1716775457damn	Trivial
②	#3211	3-tier credential resolution	✅	✅	❌	?	1716775457damn	Medium
③	#3214	API timeout/retry	❌	❌	❌	?	1716775457damn	Medium
④	#3216	Session resume	❌	❌	✅	YES	1716775457damn	Medium
⑤	#3218	Wizard entry points	❌	❌	✅	?	(none yet)	Medium
⑥	#3219	LSP integration v3	❌	❌	✅	?	1716775457damn	Hard

Order: ① → ② → ③ → ④ → ⑤ → ⑥ (green CI first, then red, easy → hard)

---

① #3217 — Auto-compact retry

Branch: worktree-auto-compact-retry Worktree: .claude/worktrees/wf_616683a9-d46-1

Reviewer said:

"Nice improvement — extending auto-compact retry from REPL-only to all execution paths fixes a real UX pain point. Using RuntimeError::is_context_window_failure() via the canonical api crate markers is the right approach. The progressive compaction strategy (4→2→0) with MAX_COMPACT_RETRIES=3 is well-bounded."

Action: "I see cargo fmt is failing in CI — consider running cargo fmt --all before merge to keep checks green."

CI: 5/6 green. Only cargo fmt fails.

Plan:

Step	What	Command
1	Format	cd rust/ && cargo fmt --all
2	Verify	cargo fmt --all -- --check && cargo check --workspace
3	Commit	"style: cargo fmt"
4	Push	git push origin worktree-auto-compact-retry

Fmt touches 1 file: main.rs (5 ins / 10 del, pure formatting). No logic changes.

Status: ⬜

---

② #3211 — 3-tier credential resolution

Branch: worktree-provider-config-fallback Worktree: .claude/worktrees/provider-config-fallback

Reviewer said:

"This makes the setup wizard actually functional. The 3-tier priority is well thought out. Covering all four providers with both API key and base URL resolution is thorough."

Ask 1: "The from_env_or_saved naming across multiple provider files could be consolidated into a shared trait or helper to reduce duplication."

Ask 2: "Also, cargo fmt is failing — a quick cargo fmt --all before merge will fix that."

CI: 5/6 green. Only cargo fmt fails.

Plan:

Step	What	Detail
1	Investigate	Read all 4 provider files. Catalog from_env_or_saved signatures, env vars, return types. Find where ResolvedCredentials is defined.
2	Add helper	CredentialSources struct + resolve_provider_credentials() fn in config.rs
3	Refactor × 4	Replace each provider's from_env_or_saved body with helper call
4	Export	pub use in lib.rs if needed
5	Fmt + verify	cargo fmt --all → cargo check --workspace → cargo test -p runtime -p api
6	Commit + push	"refactor: consolidate from_env_or_saved into shared resolve_provider_credentials() helper"

Helper design:

pub struct CredentialSources {
    pub api_key_env: &'static str,
    pub api_key_alt_env: Option<&'static str>,
    pub api_key_config: fn(&RuntimeProviderConfig) -> &Option<String>,
    pub base_url_env: &'static str,
    pub base_url_config: fn(&RuntimeProviderConfig) -> &Option<String>,
    pub default_base_url: &'static str,
}

pub fn resolve_provider_credentials(
    config: &RuntimeProviderConfig,
    sources: &CredentialSources,
) -> Option<ResolvedCredentials> { … }

Open Qs to resolve in Step 1:

• Is ResolvedCredentials shared or per-provider?
• Does .env tier use dotenv::var()?
• Any tests call from_env_or_saved directly?

Status: ✅ Resolved (2026-06-04)

Merged upstream/main into worktree-provider-config-fallback. Conflicts in runtime/src/config.rs and runtime/src/lib.rs both resolved while preserving the read_env_non_empty / resolve_base_url shared helpers and the RuntimeProviderConfig struct/impl.

Key manual fix: The merge-generated config.rs had corrupted test sections (interleaved function declarations from both versions). Reconstructed clean provider_config_* and rules_import_* test bodies from original commits.

Build verification: cargo check -p api ✅, cargo check -p runtime ✅. Pre-existing clippy issues in trident.rs and claw-rag-service unchanged.

Commit: 67da2dc

---

③ #3214 — API timeout/retry

Branch: worktree-api-timeout-retry-v2 Worktree: .claude/worktrees/api-timeout-retry-v2

Reviewer said:

"Good revival of the stalled #2816. Retry-After header and 400 transient retry are particularly valuable for handling API rate limits."

Question: "Are defaults preserved so existing behavior is unchanged?"

CI: 3/6 green. Build ❌, test ❌, fmt ❌. Fix CI first, then answer.

Plan:

Phase A — Fix CI:

Step	What	Detail
1	Investigate build	cargo check --workspace 2>&1 — capture errors
2	Fix build	Based on Step 1 findings
3	Investigate test	cargo test -p runtime -p api 2>&1 — capture failures
4	Fix tests	Based on Step 3 findings
5	Fmt	cargo fmt --all
6	Verify green	All 3 checks pass locally
7	Commit + push

Phase B — Answer reviewer:

Step	What	Detail
8	Verify defaults	TimeoutConfig timeout=300s, RetryConfig max_retries=2, 400-transient is additive-only
9	Post comment	Answer with specifics (draft below, adjust after Step 8)

Comment draft:

Yes, all defaults are preserved:
- Timeout: 300s (unchanged)
- Max retries: 2 (unchanged)
- Retry-After: only activates when server sends the header
- 400 transient: purely additive — retries only on specific transient 400 codes

Backward-compatible with no behavioral changes unless user explicitly configures new fields.

⚠ Don't post until Step 8 confirms. If any default changed, fix code first.

Open Qs:

• Actual build errors? (Step 1)
• Actual test failures? (Step 3)
• Defaults actually preserved? (Step 8)

Status: ⬜

---

④ #3216 — Session resume

Branch: worktree-session-resume-fixes Worktree: .claude/worktrees/wf_616683a9-d46-2

Reviewer said:

"The unified session resolution with load_session_excluding() is a clean approach."

Concern 1: "I notice CI has 2 failing checks (build + cargo test) — worth investigating before merge."

Concern 2: "What happens when all sessions have 0 messages (fresh install / all deleted)? A clear error message would be better than silently returning nothing."

CI: 4/6 green. Build ❌, test ❌. fmt ✅.

Plan:

Step	What	Detail
1	Investigate build	cargo check --workspace 2>&1 — capture errors
2	Fix build	Based on Step 1. Likely missing imports in session_control.rs
3	Investigate test	cargo test -p runtime 2>&1 — capture failures
4	Fix tests	Based on Step 3
5	Add empty-session error	If all candidates have 0 messages → Err("No sessions with messages found. Start a new session first.")
6	Surface in CLI	/resume handler prints error gracefully (no panic)
7	Fmt + verify	cargo fmt --all → cargo check --workspace → cargo test -p runtime
8	Commit + push

Empty-session error:

// session_control.rs — after scanning:
if candidates.is_empty() || candidates.iter().all(|s| s.message_count == 0) {
    return Err(SessionError::NoSessionsFound);
}
// main.rs — /resume handler:
Err(SessionError::NoSessionsFound) => {
    eprintln!("No sessions with messages found. Start a new session first.");
}

Open Qs:

• Actual build errors? (Step 1)
• Actual test failures? (Step 3)
• Does SessionError enum already exist?

Status: ⬜

---

⑤ #3218 — Wizard entry points

Branch: worktree-wizard-entry-points Worktree: .claude/worktrees/wizard-entry-points

Reviewer: None yet. Fix proactively before one arrives.

CI: 4/6 green. Build ❌, test ❌. fmt ✅.

Likely cause: setup_wizard.rs imports RuntimeProviderConfig which doesn't exist on upstream/main (only in PR #3211).

Strategy: Add RuntimeProviderConfig to this PR too. Safe duplication — when #3211 merges first, rebase deduplicates.

Plan:

Step	What	Detail
1	Investigate build	cargo check --workspace 2>&1 — confirm root cause
2	Add RuntimeProviderConfig	Minimal fields in config.rs (same as #3211)
3	Export	pub use in lib.rs
4	Fix other compile errors	If any
5	Investigate test	cargo test -p runtime -p api 2>&1
6	Fix tests	Based on findings
7	Fmt + verify	cargo fmt --all → cargo check --workspace → cargo test -p runtime -p api
8	Commit + push

Open Qs:

• Is RuntimeProviderConfig the only missing type?
• Any other dependencies on #3211?

Status: ⬜

---

⑥ #3219 — LSP integration v3

Branch: worktree-lsp-v3 Worktree: .claude/worktrees/wf_616683a9-d46-5

Reviewer said:

"This is an impressive piece of work. The modular split across 4 crates with 58 unit tests is exactly the right architecture. The lazy-start pattern and diagnostic enrichment are genuinely useful."

Blocker 1: "build and cargo test are currently failing — worth investigating"

Blocker 2: "Consider adding a claw lsp status command so users can confirm which servers were discovered and their health"

CI: 4/6 green. Build ❌, test ❌. fmt ✅.

Plan:

Phase A — Fix CI:

Step	What	Detail
1	Investigate build	cargo check --workspace 2>&1
2	Fix build	Module visibility / missing imports in LSP crates
3	Investigate test	cargo test -p runtime 2>&1
4	Fix tests	Assertions needing update for new types
5	Verify	cargo check --workspace + cargo test -p runtime
6	Commit	"fix: resolve CI build and test failures for LSP integration"

Phase B — Add claw lsp status:

Step	What	Detail
7	Add LspServerStatus + LspStatus	In lsp_discovery.rs: { name, status: {Running|Stopped|NotFound}, path }
8	Add discover_all()	Probe PATH → Vec<LspServerStatus>
9	CLI subcommand	claw lsp status → call discover_all() → print table
10	Slash command	/lsp status → check in-process manager → print
11	Fmt + verify	cargo fmt --all → cargo check --workspace → cargo test -p runtime
12	Commit + push

Output format:

LSP Server Status:
  rust-analyzer    running   /home/user/.cargo/bin/rust-analyzer
  gopls            stopped   /usr/local/bin/gopls
  pyright          not found (not in PATH)

2 servers discovered, 1 running

Open Qs:

• Actual build errors? (Step 1)
• Actual test failures? (Step 3)
• Does LSP crate already have discover_all()-equivalent?
• CLI: probe fresh or connect to running instance?

Status: ⬜

---

Progress

#	PR	Status	CI	Reviewer	Pushed
①	#3217	⬜	—	—	—
②	#3211	⬜	—	—	—
③	#3214	⬜	—	—	—
④	#3216	⬜	—	—	—
⑤	#3218	⬜	—	—	—
⑥	#3219	⬜	—	—	—