From 4f83a81cf6bba5a85b08efd9a59b54e0bab6633b Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Sun, 12 Apr 2026 02:56:48 +0000 Subject: [PATCH] Make dump-manifests recoverable outside the inferred build tree The backlog sweep found that the user-cited #21-#23 items were already closed, and the next real pain point was `claw dump-manifests` failing without a direct way to point at the upstream manifest source. This adds an explicit `--manifests-dir` path, upgrades the failure messages to say whether the source root or required files are missing, and updates the ROADMAP closeout to reflect that #45 is now fixed. Constraint: Preserve existing dump-manifests behavior when no explicit override is supplied Rejected: Require CLAUDE_CODE_UPSTREAM for every invocation | breaks existing build-tree workflows and is unnecessarily rigid Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep manifest-source override guidance centralized so future error-path edits do not drift Tested: cargo fmt --all; cargo clippy --workspace --all-targets -- -D warnings; cargo test --workspace; architect review APPROVE Not-tested: Manual invocation against every legacy env-based manifest lookup layout --- ROADMAP.md | 5 +- rust/crates/rusty-claude-cli/src/main.rs | 217 ++++++++++++++---- .../tests/output_format_contract.rs | 12 +- 3 files changed, 182 insertions(+), 52 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 84304ed..c734332 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -453,12 +453,15 @@ Model name prefix now wins unconditionally over env-var presence. Regression tes 42. **`--output-format json` errors emitted as prose, not JSON** — dogfooded 2026-04-09. When `claw --output-format json prompt` hits an API error, the error was printed as plain text (`error: api returned 401 ...`) to stderr instead of a JSON object. Any tool or CI step parsing claw's JSON output gets nothing parseable on failure — the error is invisible to the consumer. **Fix (`a...`):** detect `--output-format json` in `main()` at process exit and emit `{"type":"error","error":""}` to stderr instead of the prose format. Non-JSON path unchanged. **Done** in this nudge cycle. +43. **Hook ingress opacity: typed hook-health/delivery report missing** — **verified likely external tracking on 2026-04-12:** repo-local searches for `/hooks/health`, `/hooks/status`, and hook-ingress route code found no implementation surface outside `ROADMAP.md`, and the prior state-surface note below already records that the HTTP server is not owned by claw-code. Treat this as likely upstream/server-surface tracking rather than an immediate claw-code task. **Original filing below.** 43. **Hook ingress opacity: typed hook-health/delivery report missing** — dogfooded 2026-04-09 while wiring the agentika timer→hook→session bridge. Debugging hook delivery required manual HTTP probing and inferring state from raw status codes (404 = no route, 405 = route exists, 400 = body missing required field). No typed endpoint exists to report: route present/absent, accepted methods, mapping matched/not matched, target session resolved/not resolved, last delivery failure class. Fix shape: add `GET /hooks/health` (or `/hooks/status`) returning a structured JSON diagnostic — no auth exposure, just routing/matching/session state. Source: gaebal-gajae dogfood 2026-04-09. 44. **Broad-CWD guardrail is warning-only; needs policy-level enforcement** — dogfooded 2026-04-09. `5f6f453` added a stderr warning when claw starts from `$HOME` or filesystem root (live user kapcomunica scanned their whole machine). Warning is a mitigation, not a guardrail: the agent still proceeds with unbounded scope. Follow-up fix shape: (a) add `--allow-broad-cwd` flag to suppress the warning explicitly (for legitimate home-dir use cases); (b) in default interactive mode, prompt "You are running from your home directory — continue? [y/N]" and exit unless confirmed; (c) in `--output-format json` or piped mode, treat broad-CWD as a hard error (exit 1) with `{"type":"error","error":"broad CWD: running from home directory requires --allow-broad-cwd"}`. Source: kapcomunica in #claw-code 2026-04-09; gaebal-gajae ROADMAP note same cycle. 45. **`claw dump-manifests` fails with opaque "No such file or directory"** — dogfooded 2026-04-09. `claw dump-manifests` emits `error: failed to extract manifests: No such file or directory (os error 2)` with no indication of which file or directory is missing. **Partial fix at `47aa1a5`+1**: error message now includes `looked in: ` so the build-tree path is visible, what manifests are, or how to fix it. Fix shape: (a) surface the missing path in the error message; (b) add a pre-check that explains what manifests are and where they should be (e.g. `.claw/manifests/` or the plugins directory); (c) if the command is only valid after `claw init` or after installing plugins, say so explicitly. Source: Jobdori dogfood 2026-04-09. +45. **`claw dump-manifests` fails with opaque `No such file or directory`** — **done (verified 2026-04-12):** current `main` now accepts `claw dump-manifests --manifests-dir PATH`, pre-checks for the required upstream manifest files (`src/commands.ts`, `src/tools.ts`, `src/entrypoints/cli.tsx`), and replaces the opaque os error with guidance that points users to `CLAUDE_CODE_UPSTREAM` or `--manifests-dir`. Fresh proof: parser coverage for both flag forms, unit coverage for missing-manifest and explicit-path flows, and `output_format_contract` JSON coverage via the new flag all pass. **Original filing below.** +45. **`claw dump-manifests` fails with opaque `No such file or directory`** — **done (verified 2026-04-12):** current `main` now accepts `claw dump-manifests --manifests-dir PATH`, pre-checks for the required upstream manifest files (`src/commands.ts`, `src/tools.ts`, `src/entrypoints/cli.tsx`), and replaces the opaque os error with guidance that points users to `CLAUDE_CODE_UPSTREAM` or `--manifests-dir`. Fresh proof: parser coverage for both flag forms, unit coverage for missing-manifest and explicit-path flows, and `output_format_contract` JSON coverage via the new flag all pass. **Original filing below.** 46. **`/tokens`, `/cache`, `/stats` were dead spec — parse arms missing** — dogfooded 2026-04-09. All three had spec entries with `resume_supported: true` but no parse arms, producing the circular error "Unknown slash command: /tokens — Did you mean /tokens". Also `SlashCommand::Stats` existed but was unimplemented in both REPL and resume dispatch. **Done at `60ec2ae` 2026-04-09**: `"tokens" | "cache"` now alias to `SlashCommand::Stats`; `Stats` is wired in both REPL and resume path with full JSON output. Source: Jobdori dogfood. 47. **`/diff` fails with cryptic "unknown option 'cached'" outside a git repo; resume /diff used wrong CWD** — dogfooded 2026-04-09. `claw --resume /diff` in a non-git directory produced `git diff --cached failed: error: unknown option 'cached'` because git falls back to `--no-index` mode outside a git tree. Also resume `/diff` used `session_path.parent()` (the `.claw/sessions//` dir) as CWD for the diff — never a git repo. **Done at `aef85f8` 2026-04-09**: `render_diff_report_for()` now checks `git rev-parse --is-inside-work-tree` first and returns a clear "no git repository" message; resume `/diff` uses `std::env::current_dir()`. Source: Jobdori dogfood. @@ -493,7 +496,7 @@ Model name prefix now wins unconditionally over env-var presence. Regression tes 62. **Worker state file surface not implemented** — **done (verified 2026-04-12):** current `main` already wires `emit_state_file(worker)` into the worker transition path in `rust/crates/runtime/src/worker_boot.rs`, atomically writes `.claw/worker-state.json`, and exposes the documented reader surface through `claw state` / `claw state --output-format json` in `rust/crates/rusty-claude-cli/src/main.rs`. Fresh proof exists in `runtime` regression `emit_state_file_writes_worker_status_on_transition`, the end-to-end `tools` regression `recovery_loop_state_file_reflects_transitions`, and direct CLI parsing coverage for `state` / `state --output-format json`. Source: Jobdori dogfood. -**Scope note (verified 2026-04-12):** ROADMAP #31 and #63-#68 describe acpx/droid or OMX/Ultraclaw orchestration/runtime behavior, not claw-code source already present in this repository. Repo-local searches for `acpx`, `use-droid`, `run-acpx`, `commit-wrapper`, `ultraclaw`, `roadmap-nudge-10min`, and `OMX_TMUX_INJECT` found no implementation hits outside `ROADMAP.md`, so treat those entries as external tracking notes rather than immediate claw-code implementation backlog. With #31 retired as external, no repo-local Immediate Backlog item remains open on current main. +**Scope note (verified 2026-04-12):** ROADMAP #31, #43, and #63-#68 currently appear to describe acpx/droid or upstream OMX/server orchestration behavior, not claw-code source already present in this repository. Repo-local searches for `acpx`, `use-droid`, `run-acpx`, `commit-wrapper`, `ultraclaw`, `roadmap-nudge-10min`, `OMX_TMUX_INJECT`, `/hooks/health`, and `/hooks/status` found no implementation hits outside `ROADMAP.md`, and the earlier state-surface note already records that the HTTP server is not owned by claw-code. With #45 now fixed, the remaining unresolved items in this section look like external tracking notes rather than confirmed repo-local backlog; re-check if new repo-local evidence appears. 63. **Droid session completion semantics broken: code arrives after "status: completed"** — dogfooded 2026-04-12. Ultraclaw droid sessions (use-droid via acpx) report `session.status: completed` before file writes are fully flushed/synced to the working tree. Discovered +410 lines of "late-arriving" droid output that appeared after I had already assessed 8 sessions as "no code produced." This creates false-negative assessments and duplicate work. **Fix shape:** (a) droid agent should only report completion after explicit file-write confirmation (fsync or existence check); (b) or, claw-code should expose a `pending_writes` status that indicates "agent responded, disk flush pending"; (c) lane orchestrators should poll for file changes for N seconds after completion before final assessment. **Blocker:** none. Source: Jobdori ultraclaw dogfood 2026-04-12. diff --git a/rust/crates/rusty-claude-cli/src/main.rs b/rust/crates/rusty-claude-cli/src/main.rs index 8129bd9..06ea162 100644 --- a/rust/crates/rusty-claude-cli/src/main.rs +++ b/rust/crates/rusty-claude-cli/src/main.rs @@ -177,7 +177,10 @@ fn merge_prompt_with_stdin(prompt: &str, stdin_content: Option<&str>) -> String fn run() -> Result<(), Box> { let args: Vec = env::args().skip(1).collect(); match parse_args(&args)? { - CliAction::DumpManifests { output_format } => dump_manifests(output_format)?, + CliAction::DumpManifests { + output_format, + manifests_dir, + } => dump_manifests(manifests_dir.as_deref(), output_format)?, CliAction::BootstrapPlan { output_format } => print_bootstrap_plan(output_format)?, CliAction::Agents { args, @@ -274,6 +277,7 @@ fn run() -> Result<(), Box> { enum CliAction { DumpManifests { output_format: CliOutputFormat, + manifests_dir: Option, }, BootstrapPlan { output_format: CliOutputFormat, @@ -623,7 +627,7 @@ fn parse_args(args: &[String]) -> Result { let permission_mode = permission_mode_override.unwrap_or_else(default_permission_mode); match rest[0].as_str() { - "dump-manifests" => Ok(CliAction::DumpManifests { output_format }), + "dump-manifests" => parse_dump_manifests_args(&rest[1..], output_format), "bootstrap-plan" => Ok(CliAction::BootstrapPlan { output_format }), "agents" => Ok(CliAction::Agents { args: join_optional_args(&rest[1..]), @@ -1213,6 +1217,39 @@ fn parse_export_args(args: &[String], output_format: CliOutputFormat) -> Result< }) } +fn parse_dump_manifests_args( + args: &[String], + output_format: CliOutputFormat, +) -> Result { + let mut manifests_dir: Option = None; + let mut index = 0; + while index < args.len() { + let arg = &args[index]; + if arg == "--manifests-dir" { + let value = args + .get(index + 1) + .ok_or_else(|| String::from("--manifests-dir requires a path"))?; + manifests_dir = Some(PathBuf::from(value)); + index += 2; + continue; + } + if let Some(value) = arg.strip_prefix("--manifests-dir=") { + if value.is_empty() { + return Err(String::from("--manifests-dir requires a path")); + } + manifests_dir = Some(PathBuf::from(value)); + index += 1; + continue; + } + return Err(format!("unknown dump-manifests option: {arg}")); + } + + Ok(CliAction::DumpManifests { + output_format, + manifests_dir, + }) +} + fn parse_resume_args(args: &[String], output_format: CliOutputFormat) -> Result { let (session_path, command_tokens): (PathBuf, &[String]) = match args.first() { None => (PathBuf::from(LATEST_SESSION_REFERENCE), &[]), @@ -1915,32 +1952,58 @@ fn looks_like_slash_command_token(token: &str) -> bool { .any(|spec| spec.name == name || spec.aliases.contains(&name)) } -fn dump_manifests(output_format: CliOutputFormat) -> Result<(), Box> { +fn dump_manifests( + manifests_dir: Option<&Path>, + output_format: CliOutputFormat, +) -> Result<(), Box> { let workspace_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../.."); - dump_manifests_at_path(&workspace_dir, output_format) + dump_manifests_at_path(&workspace_dir, manifests_dir, output_format) } +const DUMP_MANIFESTS_OVERRIDE_HINT: &str = + "Hint: set CLAUDE_CODE_UPSTREAM=/path/to/upstream or pass `claw dump-manifests --manifests-dir /path/to/upstream`."; + // Internal function for testing that accepts a workspace directory path. fn dump_manifests_at_path( workspace_dir: &std::path::Path, + manifests_dir: Option<&Path>, output_format: CliOutputFormat, ) -> Result<(), Box> { - // Surface the resolved path in the error so users can diagnose missing - // manifest files without guessing what path the binary expected. - // ROADMAP #45: this path is only correct when running from the build tree; - // a proper fix would ship manifests alongside the binary. - let resolved = workspace_dir - .canonicalize() - .unwrap_or_else(|_| workspace_dir.to_path_buf()); + let paths = if let Some(dir) = manifests_dir { + let resolved = dir.canonicalize().unwrap_or_else(|_| dir.to_path_buf()); + UpstreamPaths::from_repo_root(resolved) + } else { + // Surface the resolved path in the error so users can diagnose missing + // manifest files without guessing what path the binary expected. + let resolved = workspace_dir + .canonicalize() + .unwrap_or_else(|_| workspace_dir.to_path_buf()); + UpstreamPaths::from_workspace_dir(&resolved) + }; - let paths = UpstreamPaths::from_workspace_dir(&resolved); - - // Pre-check: verify manifest directory exists - let manifest_dir = paths.repo_root(); - if !manifest_dir.exists() { + let source_root = paths.repo_root(); + if !source_root.exists() { return Err(format!( - "Manifest files (commands.ts, tools.ts) define CLI commands and tools.\n Expected at: {}\n Run `claw init` to create them or specify --manifests-dir.", - manifest_dir.display() + "Manifest source directory does not exist.\n looked in: {}\n {DUMP_MANIFESTS_OVERRIDE_HINT}", + source_root.display(), + ) + .into()); + } + + let required_paths = [ + ("src/commands.ts", paths.commands_path()), + ("src/tools.ts", paths.tools_path()), + ("src/entrypoints/cli.tsx", paths.cli_path()), + ]; + let missing = required_paths + .iter() + .filter_map(|(label, path)| (!path.is_file()).then_some(*label)) + .collect::>(); + if !missing.is_empty() { + return Err(format!( + "Manifest source files are missing.\n repo root: {}\n missing: {}\n {DUMP_MANIFESTS_OVERRIDE_HINT}", + source_root.display(), + missing.join(", "), ) .into()); } @@ -1966,7 +2029,7 @@ fn dump_manifests_at_path( Ok(()) } Err(error) => Err(format!( - "failed to extract manifests: {error}\n looked in: {path}", + "failed to extract manifests: {error}\n looked in: {path}\n {DUMP_MANIFESTS_OVERRIDE_HINT}", path = paths.repo_root().display() ) .into()), @@ -8048,7 +8111,7 @@ fn print_help_to(out: &mut impl Write) -> io::Result<()> { out, " Diagnose local auth, config, workspace, and sandbox health" )?; - writeln!(out, " claw dump-manifests")?; + writeln!(out, " claw dump-manifests [--manifests-dir PATH]")?; writeln!(out, " claw bootstrap-plan")?; writeln!(out, " claw agents")?; writeln!(out, " claw mcp")?; @@ -9063,6 +9126,33 @@ mod tests { ); } + #[test] + fn dump_manifests_subcommand_accepts_explicit_manifest_dir() { + assert_eq!( + parse_args(&[ + "dump-manifests".to_string(), + "--manifests-dir".to_string(), + "/tmp/upstream".to_string(), + ]) + .expect("dump-manifests should parse"), + CliAction::DumpManifests { + output_format: CliOutputFormat::Text, + manifests_dir: Some(PathBuf::from("/tmp/upstream")), + } + ); + assert_eq!( + parse_args(&[ + "dump-manifests".to_string(), + "--manifests-dir=/tmp/upstream".to_string() + ]) + .expect("inline dump-manifests flag should parse"), + CliAction::DumpManifests { + output_format: CliOutputFormat::Text, + manifests_dir: Some(PathBuf::from("/tmp/upstream")), + } + ); + } + #[test] fn local_command_help_flags_stay_on_the_local_parser_path() { assert_eq!( @@ -11554,43 +11644,78 @@ mod sandbox_report_tests { #[cfg(test)] mod dump_manifests_tests { use super::{dump_manifests_at_path, CliOutputFormat}; + use std::fs; #[test] fn dump_manifests_shows_helpful_error_when_manifests_missing() { - // Create a temp directory without manifest files - let temp_dir = std::env::temp_dir().join(format!( + let root = std::env::temp_dir().join(format!( "claw_test_missing_manifests_{}", std::process::id() )); - std::fs::create_dir_all(&temp_dir).expect("failed to create temp dir"); + let workspace = root.join("workspace"); + std::fs::create_dir_all(&workspace).expect("failed to create temp workspace"); - // Clean up at the end of the test - let _cleanup = std::panic::catch_unwind(|| { - // Call dump_manifests_at_path with the temp directory - let result = dump_manifests_at_path(&temp_dir, CliOutputFormat::Text); + let result = dump_manifests_at_path(&workspace, None, CliOutputFormat::Text); + assert!( + result.is_err(), + "expected an error when manifests are missing" + ); - // Assert that the call fails - assert!( - result.is_err(), - "expected an error when manifests are missing" - ); + let error_msg = result.unwrap_err().to_string(); - let error_msg = result.unwrap_err().to_string(); + assert!( + error_msg.contains("Manifest source files are missing"), + "error message should mention missing manifest sources: {error_msg}" + ); + assert!( + error_msg.contains(&root.display().to_string()), + "error message should contain the resolved repo root path: {error_msg}" + ); + assert!( + error_msg.contains("src/commands.ts"), + "error message should mention missing commands.ts: {error_msg}" + ); + assert!( + error_msg.contains("CLAUDE_CODE_UPSTREAM"), + "error message should explain how to supply the upstream path: {error_msg}" + ); - // Assert the error message contains "Manifest files (commands.ts, tools.ts)" - assert!( - error_msg.contains("Manifest files (commands.ts, tools.ts)"), - "error message should mention manifest files: {error_msg}" - ); + let _ = std::fs::remove_dir_all(&root); + } - // Assert the error message contains the expected path - assert!( - error_msg.contains(&temp_dir.display().to_string()), - "error message should contain the expected path: {error_msg}" - ); - }); + #[test] + fn dump_manifests_uses_explicit_manifest_dir() { + let root = std::env::temp_dir().join(format!( + "claw_test_explicit_manifest_dir_{}", + std::process::id() + )); + let workspace = root.join("workspace"); + let upstream = root.join("upstream"); + fs::create_dir_all(workspace.join("nested")).expect("workspace should exist"); + fs::create_dir_all(upstream.join("src/entrypoints")) + .expect("upstream fixture should exist"); + fs::write( + upstream.join("src/commands.ts"), + "import FooCommand from './commands/foo'\n", + ) + .expect("commands fixture should write"); + fs::write( + upstream.join("src/tools.ts"), + "import ReadTool from './tools/read'\n", + ) + .expect("tools fixture should write"); + fs::write( + upstream.join("src/entrypoints/cli.tsx"), + "startupProfiler()\n", + ) + .expect("cli fixture should write"); - // Clean up temp directory - let _ = std::fs::remove_dir_all(&temp_dir); + let result = dump_manifests_at_path(&workspace, Some(&upstream), CliOutputFormat::Text); + assert!( + result.is_ok(), + "explicit manifest dir should succeed: {result:?}" + ); + + let _ = fs::remove_dir_all(&root); } } diff --git a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs index 9710b92..33d25cd 100644 --- a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs +++ b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs @@ -174,13 +174,15 @@ fn dump_manifests_and_init_emit_json_when_requested() { fs::create_dir_all(&root).expect("temp dir should exist"); let upstream = write_upstream_fixture(&root); - let manifests = assert_json_command_with_env( + let manifests = assert_json_command( &root, - &["--output-format", "json", "dump-manifests"], - &[( - "CLAUDE_CODE_UPSTREAM", + &[ + "--output-format", + "json", + "dump-manifests", + "--manifests-dir", upstream.to_str().expect("utf8 upstream"), - )], + ], ); assert_eq!(manifests["kind"], "dump-manifests"); assert_eq!(manifests["commands"], 1);