fix: subprocess task execution #137
Conversation
📝 WalkthroughWalkthroughIntroduces a plumbing pathway to execute Deno tasks via JSON and a subprocess, updates CLI fast-path handling and enums, exposes the tasks::exec module, adjusts task “test” to async with workingDir "./src", removes a port from .ghjk/lock.json, adds cmake to flake.nix, updates installer URL host, fixes Linux stat format, and adds an E2E workingDir test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as ghjk CLI
participant QC as Quick CLI
participant Tasks as tasks::exec
participant Sub as Subprocess (plumbing exec-deno-task)
participant FS as FS (request.json/result.json)
User->>CLI: ghjk plumbing exec-deno-task --json <file>
CLI->>QC: try_quick_cli(...)
QC-->>CLI: QuickCliResult::ExecDenoTask(path)
CLI->>Tasks: exec_deno_task_from_json(gcx, path)
Tasks->>FS: read request.json
Tasks->>Sub: spawn with json path
Sub->>FS: write result.json
Sub-->>Tasks: exit (code)
Tasks->>FS: read result.json
Tasks-->>CLI: Res<()> (or error)
CLI-->>User: ExitCode::SUCCESS or error
sequenceDiagram
autonumber
actor Dev as Developer
participant TS as ghjk.ts (task test)
participant Runner as Task Runner
participant OS as OS (POSIX)
participant Deno as Deno Runtime
Dev->>TS: define async function test($), workingDir: "./src"
TS->>Runner: register task with workingDir
Runner->>OS: set POSIX cwd to ./src
Runner->>Deno: start runtime with cwd ./src
Deno-->>Runner: task executes with Deno.cwd() === ./src
Runner-->>Dev: test completes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ghjk/systems/tasks/exec.rs (2)
180-255: Subprocess execution: add timeout and include stderr on failure.Avoid indefinite hangs and surface actionable errors. Also, fail early if
working_dirdoesn’t exist.- let status = cmd - .status() - .await - .wrap_err("error running plumbing exec-deno-task")?; - if !status.success() { - eyre::bail!( - "exec-deno-task subprocess failed with status {:?}", - status.code() - ); - } + if !working_dir.exists() { + eyre::bail!("workingDir does not exist: {}", working_dir.display()); + } + use tokio::time::{timeout, Duration}; + let out = timeout(Duration::from_secs(300), cmd.output()) + .await + .wrap_err("timeout running plumbing exec-deno-task")? + .wrap_err("error running plumbing exec-deno-task")?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr); + eyre::bail!( + "exec-deno-task failed (code {:?}): {}", + out.status.code(), + stderr + ); + }Note: add
use tokio::time::{timeout, Duration};at the top or fully qualify as shown.
333-427: Fix compile issue and tighten Deno permissions.
repo_root.join(...).wrap_err(...)attempts to callwrap_erron aPathBuf(non-Result) and will not compile.allow_all: trueis overly permissive for task execution.- let main_module = gcx - .config - .repo_root - .join("src/ghjk/systems/tasks/bindings.ts") - .wrap_err("repo url error")?; + let main_module = gcx + .config + .repo_root + .join("src/ghjk/systems/tasks/bindings.ts");And consider narrowing permissions (example below keeps broad access but documents intent):
- deno_runtime::deno_permissions::PermissionsOptions { - allow_env: Some(vec![]), - allow_import: Some(vec![]), - allow_read: Some(vec![]), - allow_net: Some(vec![]), - allow_ffi: Some(vec![]), - allow_run: Some(vec![]), - allow_sys: Some(vec![]), - allow_write: Some(vec![]), - allow_all: true, - prompt: false, - ..default() - }, + deno_runtime::deno_permissions::PermissionsOptions { + // TODO: scope these to the workspace instead of allow_all + allow_all: true, + prompt: false, + ..default() + },
🧹 Nitpick comments (14)
src/hooks/hook.fish (2)
4-4: Harden stat calls against leading-dash filenames.Add
--before$argvto avoid accidental option parsing when a file starts with-.- stat -c "%Y" $argv + stat -c "%Y" -- $argv
10-10: Ditto for the wildcard/default case.Protect
statwith--here too.- stat -c "%Y" $argv + stat -c "%Y" -- $argvinstall.sh (2)
5-7: Track the TODOs in an issue and gate move-on-success.Consider moving the binary only after
install.tssucceeds to avoid leaving a half-installed CLI.- if [ -w "${GHJK_INSTALL_EXE_DIR}" ]; then - mv "$TMP_DIR/$EXE" "$GHJK_INSTALL_EXE_DIR" - rm -r "$TMP_DIR" + if [ -w "${GHJK_INSTALL_EXE_DIR}" ]; then + # run installer from temp path first + "$TMP_DIR/$EXE" deno run -A "${GHJK_INSTALLER_URL}" + mv "$TMP_DIR/$EXE" "$GHJK_INSTALL_EXE_DIR" + rm -r "$TMP_DIR"
132-132: Pinned installer URL to tag: verify availability before running.Optional: preflight with a HEAD request to fail fast with a clear error if the tag is missing.
+curl -fsSIL "$GHJK_INSTALLER_URL" >/dev/null || { + echo "Installer $GHJK_INSTALLER_URL not found." >&2 + exit 1 +}src/ghjk/systems/tasks.rs (1)
3-3: Narrow module visibility or re‑export the minimal surface.If external crates don’t need
exec, preferpub(crate)and re‑export only what’s needed to avoid expanding the public API surface.-pub mod exec; +pub(crate) mod exec; +pub use exec::exec_deno_task_from_json;ghjk.ts (1)
210-216: Avoid debug noise in default task definition.Drop or guard the
console.logcalls behind a debug flag to keep CI logs clean.- console.log($.workingDir, "custom"); - await $`echo $PWD`; - console.log(Deno.cwd(), "posix"); + if (Deno.env.get("GHJK_DEBUG")) { + console.log($.workingDir, "custom"); + await $`echo $PWD`; + console.log(Deno.cwd(), "posix"); + }tests/tasks.ts (1)
200-225: Good E2E coverage; add robustness.Use
/bin/pwd -Pto avoid symlink surprises and normalize both outputs before matching.- const posix = await $`/bin/sh -c 'pwd'`.text(); + const posix = await $`/bin/sh -c '/bin/pwd -P'`.text();src/ghjk/systems/tasks/exec.rs (3)
160-178: PATH merge heuristic may overreach.
k.contains("PATH")will also modify variables likeLD_LIBRARY_PATH. If that’s unintended, restrict toPATHand*PATHcases explicitly.- if k.contains("PATH") { + if k == "PATH" || k.ends_with("PATH") {
214-227: Request/result file I/O: write atomically.Use a temp + rename to avoid partial reads by the child process if there’s a race.
- let req_bytes = serde_json::to_vec_pretty(&req).expect_or_log("json error"); - tokio::fs::write(&request_file_path, req_bytes) + let req_bytes = serde_json::to_vec_pretty(&req).expect_or_log("json error"); + let tmp = request_file_path.with_extension("json.tmp"); + tokio::fs::write(&tmp, req_bytes) .await .wrap_err("error writing exec-deno-task request json")?; + tokio::fs::rename(&tmp, &request_file_path).await.ok();
256-273: Result parsing: tolerate empty data and surface task key on error.Optional: include
def.keyin the error for quicker triage.- if let Some(error) = resp.get("error") { + if let Some(error) = resp.get("error") { eyre::bail!( - "task execution failed: {}", + "task '{}' failed: {}", + def.key, serde_json::to_string_pretty(error) .unwrap_or_else(|_| format!("{:?}", error)) ); }src/ghjk/cli.rs (4)
234-246: If you switch the CLI arg to PathBuf, simplify the call site.Use
&jsondirectly instead ofstd::path::Path::new(&json).- std::path::Path::new(&json), + &json,
319-320: Avoidunreachable!here; return a safe failure code.Defensive path prevents a panic if control flow regresses in future changes.
- QuickCliResult::ExecDenoTask(_) => unreachable!("handled earlier"), + QuickCliResult::ExecDenoTask(_) => { + tracing::error!("ExecDenoTask reached in exit(); returning failure"); + ExitCode::FAILURE + }
360-364: Propagate PathBuf directly from Quick CLI.Once the CLI arg type is
PathBuf, drop the extra conversion.- PlumbingCommands::ExecDenoTask { json } => { - return Ok(QuickCliResult::ExecDenoTask(std::path::PathBuf::from(json))); - } + PlumbingCommands::ExecDenoTask { json } => { + return Ok(QuickCliResult::ExecDenoTask(json)); + }
440-449: Use PathBuf for the JSON file argument.Prevents repeated conversions and gives better type hints in clap help.
- ExecDenoTask { - /// Path to the JSON request file - #[arg(value_name = "JSON_FILE", value_hint = clap::ValueHint::FilePath)] - json: String, - }, + ExecDenoTask { + /// Path to the JSON request file + #[arg(value_name = "JSON_FILE", value_hint = clap::ValueHint::FilePath)] + json: std::path::PathBuf, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.ghjk/lock.json(1 hunks)flake.nix(1 hunks)ghjk.ts(1 hunks)install.sh(3 hunks)src/ghjk/cli.rs(7 hunks)src/ghjk/systems/tasks.rs(1 hunks)src/ghjk/systems/tasks/exec.rs(4 hunks)src/hooks/hook.fish(1 hunks)tests/tasks.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-09T06:07:47.919Z
Learnt from: Yohe-Am
PR: metatypedev/ghjk#130
File: ghjk.ts:39-45
Timestamp: 2025-08-09T06:07:47.919Z
Learning: In the ghjk project, Rust is provided through Nix flake for local development environments. The flake.nix pins Rust version 1.85.0 with rust-src extension. For CI workflows, Rust is installed using the dsherret/rust-toolchain-filev1 action which reads the rust-toolchain.toml file. Rust is required as a build dependency even for loading the ghjk.ts TypeScript configuration file. The ports.rust in allowedBuildDeps is only for build-time dependencies, not for runtime installation in environments.
Applied to files:
flake.nixghjk.ts
📚 Learning: 2025-08-09T06:06:50.518Z
Learnt from: Yohe-Am
PR: metatypedev/ghjk#130
File: ports/lade_ghrel.port.ts:22-22
Timestamp: 2025-08-09T06:06:50.518Z
Learning: In ghjk port manifests (files matching pattern `ports/*.port.ts` or `ports/*.ts`), the `version` field in the manifest object represents the version of the port module itself, not the version of the tool/software that the port installs. The actual tool version is typically passed via `args.installVersion` parameter in methods like `downloadUrls()`.
Applied to files:
.ghjk/lock.json
🧬 Code graph analysis (5)
src/ghjk/systems/tasks.rs (1)
src/ghjk/ext/callbacks.rs (1)
exec(75-90)
tests/tasks.ts (1)
src/deno_utils/mod.ts (1)
$(105-172)
src/ghjk/cli.rs (1)
src/ghjk/systems/tasks/exec.rs (1)
exec_deno_task_from_json(334-427)
src/ghjk/systems/tasks/exec.rs (3)
src/ghjk/systems/envs/posix.rs (2)
std(216-218)std(235-237)src/ghjk/ext.rs (1)
new(35-37)src/sys_deno/types.ts (1)
GhjkCtx(151-156)
ghjk.ts (2)
src/denort/lib.rs (1)
test(161-318)src/deno_utils/mod.ts (1)
$(105-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-e2e (ubuntu-22.04, linux/x86_64)
- GitHub Check: test-e2e (macos-14, darwin/aarch64)
- GitHub Check: test-e2e (macos-13, darwin/x86_64)
- GitHub Check: test-e2e (ubuntu-22.04-arm, linux/aarch64)
- GitHub Check: test-pre-commit
🔇 Additional comments (7)
flake.nix (1)
35-35: Adding cmake to the dev shell looks good.No issues; aligns with local builds requiring CMake.
install.sh (1)
205-207: Nice UX polish.Clear, actionable guidance after install.
.ghjk/lock.json (1)
580-580: workingDir on test task: LGTM.Matches the subprocess cwd behavior required by #103.
src/ghjk/systems/tasks/exec.rs (1)
316-332: JSON casing consistency: LGTM.
#[serde(rename_all = "camelCase")]matches the produced request fields (workingDir,envVars).src/ghjk/cli.rs (3)
179-179: Comment update is accurate.Plumbing command handled earlier via QuickCommands; comment aligns with code.
293-294: Adding QuickCliResult::ExecDenoTask is appropriate.Variant cleanly models the fast‑path handoff.
432-438: Hidden “Plumbing” group looks good.Keeps the internal surface out of public help.
| // Handle plumbing fast-path before any dynamic CLI/system init | ||
| if let QuickCliResult::ExecDenoTask(json_path) = &quick_res { | ||
| let res = crate::systems::tasks::exec::exec_deno_task_from_json(&gcx, json_path).await; | ||
| deno_cx.terminate().await?; | ||
| return res.map(|_| ExitCode::SUCCESS); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix panic when ghjkdir is None by handling ExecDenoTask before the ghjkdir unwrap.
If quick_res is ExecDenoTask and config.ghjkdir is None, Line 41 returns quick_res.exit(None) which hits unreachable! (Line 320) and panics. Move the ghjkdir guard below the fast‑path branch.
Apply:
- let Some(ghjkdir_path) = config.ghjkdir.clone() else {
- return Ok(quick_res.exit(None));
- };
+ // (moved below to avoid panics when ExecDenoTask is requested and no ghjkdir is present)
+ // NOTE: this guard can only run after the ExecDenoTask fast-path is handled.And insert after the fast‑path (right after Line 91):
+ let Some(ghjkdir_path) = gcx.config.ghjkdir.clone() else {
+ return Ok(quick_res.exit(None));
+ };Also verify whether skipping write_lockfile_or_log() in the fast‑path is intentional (dynamic path writes it).
Also applies to: 41-44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== repo root: $(pwd) ==="
echo
echo "=== Searching for ExecDenoTask and QuickCliResult ==="
rg -n --hidden --no-ignore-vcs "ExecDenoTask" || true
rg -n --hidden --no-ignore-vcs "QuickCliResult" || true
echo
echo "=== Searching for ghjkdir and quick_res.exit ==="
rg -n --hidden --no-ignore-vcs "ghjkdir" || true
rg -n --hidden --no-ignore-vcs "quick_res.exit" || true
echo
echo "=== Searching for write_lockfile_or_log and exec_deno_task_from_json ==="
rg -n --hidden --no-ignore-vcs "write_lockfile_or_log" || true
rg -n --hidden --no-ignore-vcs "exec_deno_task_from_json" || true
echo
echo "=== Locate unreachable! occurrences ==="
rg -n --hidden --no-ignore-vcs "unreachable!" || true
echo
echo "=== Dump src/ghjk/cli.rs (first 400 lines) with numbers ==="
nl -ba src/ghjk/cli.rs | sed -n '1,400p' || true
echo
echo "=== Dump any QuickCliResult impl/definition matches ==="
rg -n --hidden --no-ignore-vcs "enum QuickCliResult" -S || true
rg -n --hidden --no-ignore-vcs "impl QuickCliResult" -S || true
echo
echo "=== Done ==="Length of output: 6768
Fix panic: handle ExecDenoTask fast-path before ghjkdir unwrap
Move the ghjkdir guard below the ExecDenoTask fast‑path so quick_res.exit(None) is not called for ExecDenoTask (it hits an unreachable! arm and panics).
Apply:
- let Some(ghjkdir_path) = config.ghjkdir.clone() else {
- return Ok(quick_res.exit(None));
- };
+ // (moved below to avoid panics when ExecDenoTask is requested and no ghjkdir is present)
+ // NOTE: this guard can only run after the ExecDenoTask fast-path is handled.Insert after the ExecDenoTask fast‑path branch:
+ let Some(ghjkdir_path) = gcx.config.ghjkdir.clone() else {
+ return Ok(quick_res.exit(None));
+ };Also verify whether skipping systems.write_lockfile_or_log() in the ExecDenoTask fast‑path is intentional.
[src/ghjk/cli.rs: ~41 (remove) → after ~91 (insert)]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle plumbing fast-path before any dynamic CLI/system init | |
| if let QuickCliResult::ExecDenoTask(json_path) = &quick_res { | |
| let res = crate::systems::tasks::exec::exec_deno_task_from_json(&gcx, json_path).await; | |
| deno_cx.terminate().await?; | |
| return res.map(|_| ExitCode::SUCCESS); | |
| } | |
| // Handle plumbing fast-path before any dynamic CLI/system init | |
| if let QuickCliResult::ExecDenoTask(json_path) = &quick_res { | |
| let res = crate::systems::tasks::exec::exec_deno_task_from_json(&gcx, json_path).await; | |
| deno_cx.terminate().await?; | |
| return res.map(|_| ExitCode::SUCCESS); | |
| } | |
| let Some(ghjkdir_path) = gcx.config.ghjkdir.clone() else { | |
| return Ok(quick_res.exit(None)); | |
| }; |
🤖 Prompt for AI Agents
In src/ghjk/cli.rs around lines 86 to 91, the ghjkdir guard/unwrapping currently
runs before the ExecDenoTask fast-path which causes quick_res.exit(None) to be
invoked for ExecDenoTask and hit an unreachable! arm leading to a panic; move
the ghjkdir guard and any code that calls quick_res.exit(None) to after the
ExecDenoTask branch (i.e., insert the ghjkdir check immediately after the
ExecDenoTask fast-path return) so ExecDenoTask returns early without executing
the ghjkdir-dependent code, and while making this change, confirm whether the
ExecDenoTask fast-path should also call systems.write_lockfile_or_log() — either
add that call into the fast-path if required or explicitly document/leave it out
if skipping is intentional.
There was a problem hiding this comment.
@Yohe-Am Thank you! I'm glad I could help identify that potential panic scenario. The ordering issue between the ghjkdir guard and the ExecDenoTask fast-path could definitely cause problems when someone tries to use the plumbing command without a proper ghjk setup.
/| /|
( :v: )
|(_)|
/ \
/ ___ \
( / \ )
\_\___/_/
1 participant
workingDirshould affect POSIXcwdof task #103Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests