Skip to content

fix(cli): nested dispatch resolution and chain failure reporting - #92

Merged
kjanat merged 3 commits into
masterfrom
fix/nested-dispatch-and-chain-reporting
Jul 21, 2026
Merged

fix(cli): nested dispatch resolution and chain failure reporting#92
kjanat merged 3 commits into
masterfrom
fix/nested-dispatch-and-chain-reporting

Conversation

@kjanat

@kjanat kjanat commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Closes #86, closes #87, closes #89, closes #90, closes #91.

Five open area: cli bugs that all sit on the same two seams: how a token
resolves when runner re-enters itself, and what a chain reports when it fails.

Nested dispatch

#89, flags after the task were reparsed as runner's own. args is
declared trailing_var_arg, but clap only collects raw values once that
positional already holds one. A flag written immediately after the task was
still matched against runner's options, so run tsc -p tsconfig.json --noEmit
bound -p to --parallel, entered chain mode, and rejected --noEmit as a
non-task positional. No number of -- delimiters survived a nested dispatch.

The surrounding code already documented the intended rule in three separate
comments ("chain flags precede task names; everything after the task belongs
to the task"). Nothing enforced it. forward_args_after_task inserts the
delimiter before parsing, so clap enforces the documented rule at every level.

#90, a task could resolve back to itself. "tsc": "run -q tsc" spawned
copies of itself until the process tree collapsed, and --quiet only hid the
evidence. Nested runners now carry an invocation stack in RUNNER_TASK_STACK,
inherited through the package manager sitting between two runner processes. A
task already on the stack is refused with the cycle it closes:

$ run tsc
Error: recursive task resolution detected: package.json:a -> package.json:b -> package.json:a

Frames are keyed on canonical root + source + name, so a workspace member
running its own build from the root's build stays a fan-out. --quiet
does not suppress the diagnostic.

#91, installed dependencies went to the registry. A bare token went
straight to npx, which resolves against npm. That cannot work for an npm
alias: "@typescript/native": "npm:typescript@^7" installs into a directory
whose name exists in no registry, so the fallback 404'd. Runner now reads
node_modules/<token>/package.json and runs the binary it declares, without
network access:

$ run --explain @typescript/native --noEmit
· runner resolved: tsc from …/node_modules/@typescript/native (local dependency)

Packages declaring several binaries none of which is named after the package,
or none at all, are reported instead of guessed at.

Chain reporting

#87, a failed chain gave no consolidated signal. Multi-task chains now
close with a roll-up on stderr, and the aggregate exit code says where it came
from:

· summary: 7 tasks, 5 ok, 1 failed, 1 skipped (exit 1, first failure)
·   ✓ typecheck       0.9s
·   ✗ test:bun        2.1s (exit 1)
·   – test:regex      skipped

Fail-fast runs name the tasks they never started. Under Actions each failure
also becomes an ::error:: annotation. Single-task chains get nothing, since
the per-task timing line already says it.

#86, --quiet still wrote ::group:: to stdout. Workflow commands are a
stdout protocol, so run -q <task> inside a pipeline whose stdout a parent
parses (npm pack --json) corrupted that output.

Moving the markers to stderr was the tempting fix and is wrong: GitHub Actions
does not preserve relative order between the two streams, so a fold opened
there closes around whatever lines land between. Suppression under --quiet
is the correct fix. That reasoning is recorded on emits_group so it is not
re-derived later. --quiet now suppresses every piece of runner's own output,
including the parallel block headers, which reach stdout by the same route.

Also

The dispatch arrow names the exec primitive it actually runs (npx, bunx,
pnpm exec, yarn exec) rather than the package manager. → npm typescript@7
read as though runner had run npm <package> as a shell command, which is what
#89 reported as a second defect.

Verification

959 unit tests and 51 integration tests, no failures. dprint check,
cargo lint, cargo test all clean, the three gates release.yml runs.
Schemas regenerated.

New coverage in tests/nested_dispatch.rs: eight tests driving real process
trees for forwarding, cycles, and dependency resolution. Chain summary tests in
tests/chain_integration.rs; a GitHub Actions stdout test in
tests/quiet_dispatch.rs.

Each issue was reproduced before the change and re-run after.

Behaviour changes worth a second look

  • --quiet now also drops the plain (non-Actions) parallel block headers.
    They reach stdout too, so the same argument applies.
  • Dependency resolution matches anything installed under node_modules, not
    only declared dependencies. A transitive package now resolves locally where
    it previously went to npx.

Flags after the task reach the task, at every level. `args` is
trailing_var_arg, but clap only collects raw values once it holds one, so
`run tsc -p tsconfig.json --noEmit` bound `-p` to `--parallel` and rejected
`--noEmit`. Insert the delimiter before parsing. Closes #89.

Nested runners carry an invocation stack; a task already on it is refused
with the cycle it closes. Closes #90.

Installed dependencies resolve to the binary their manifest declares, so an
npm alias works at all (its directory name is in no registry, npx 404s).
Ambiguous and zero-bin packages are reported, not guessed. Closes #91.

`--quiet` no longer writes ::group:: to stdout. Workflow commands are a
stdout protocol, so `run -q` inside `npm pack --json` corrupted the JSON.
Markers cannot move to stderr: GHA does not preserve inter-stream order.
Closes #86.

Multi-task chains close with a per-task roll-up plus GHA error annotations,
and attribute the aggregate exit code. Closes #87.

Also: dispatch arrow names the exec primitive it runs (npx/bunx/pnpm exec),
not the package manager.
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: dfd15c8b-0a5a-445e-810a-0a944d110218

📥 Commits

Reviewing files that changed from the base of the PR and between c366c79 and 3eb3367.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • README.md
  • tests/chain_integration.rs
  • tests/fixtures/github-no-group/justfile
  • tests/fixtures/github-no-group/runner.toml
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (1)
**/CHANGELOG.md

📄 CodeRabbit inference engine (Custom checks)

**/CHANGELOG.md: If any source code files (excluding tests, docs, CI, markdown, or comments-only changes) are modified, CHANGELOG.md MUST also be modified in the same PR.
If a version bump is detected, CHANGELOG.md MUST contain a new section header matching the exact new version number in the format: '## [X.Y.Z] - YYYY-MM-DD'.
If NO version bump is detected, the changes in the PR MUST be added under the existing '## [Unreleased]' section in CHANGELOG.md. The entry MUST describe the changes (e.g., Added, Changed, Fixed, Removed).

Files:

  • CHANGELOG.md
🧠 Learnings (5)
📚 Learning: 2026-03-26T20:05:44.851Z
Learnt from: kjanat
Repo: kjanat/runner PR: 1
File: src/cmd/mod.rs:67-75
Timestamp: 2026-03-26T20:05:44.851Z
Learning: In Rust, `std::process::Command` does not provide getters for stdio configuration, so whether `Stdio::inherit()` (or other `Stdio::*` settings) was applied cannot be asserted via pure unit tests without spawning a process and inspecting OS-level fds. When reviewing Rust code, do not flag “missing unit test coverage” for stdio configuration on `std::process::Command` as a code issue—treat the explicit `Command::stdin/stdout/stderr` setter calls in the source as the meaningful guarantee.

Applied to files:

  • tests/chain_integration.rs
📚 Learning: 2026-04-21T15:16:40.277Z
Learnt from: kjanat
Repo: kjanat/runner PR: 4
File: src/tool/just.rs:132-133
Timestamp: 2026-04-21T15:16:40.277Z
Learning: In Rust, if a method like `ExtractedTask::name() -> &str` returns a borrowed `&str` tied to `&self`, then using `sort_unstable_by_key(|t| t.name())` should be avoided because `by_key` requires the key to not borrow from the element being sorted (it will fail to compile due to the key’s lifetime). Do not recommend `sort_unstable_by_key` as a simplification in this situation. If you need an allocation-free and idiomatic comparison, use `sort_unstable_by(|a, b| a.name().cmp(b.name()))` instead. Using `.to_owned()` inside `by_key` is an alternative but allocates a `String` per element.

Applied to files:

  • tests/chain_integration.rs
📚 Learning: 2026-05-04T23:28:17.947Z
Learnt from: kjanat
Repo: kjanat/runner PR: 5
File: src/lib.rs:222-223
Timestamp: 2026-05-04T23:28:17.947Z
Learning: When reviewing Rust `rustdoc` comments (`/// ...`), treat a trailing backslash (`\`) at the end of a comment line as valid CommonMark syntax for a hard line break (rendered as `<br>`). Do not flag such trailing backslashes as papercuts or recommend removing them unless there is clear evidence they are unintended (e.g., they are not in the `rustdoc` comment context or the surrounding formatting contradicts the intended CommonMark hard-break usage).

Applied to files:

  • tests/chain_integration.rs
📚 Learning: 2026-06-01T17:42:48.461Z
Learnt from: kjanat
Repo: kjanat/runner PR: 34
File: Cargo.toml:61-61
Timestamp: 2026-06-01T17:42:48.461Z
Learning: In this repo, do not flag `actions_rs::log::GroupGuard` or `actions_rs::log::group_guard` as missing/non-existent when they are referenced in Rust code, because the `actions-rs` crate (dependency `actions-rs = "0.1"` / published v0.1.x) exports `pub struct GroupGuard` and `pub fn group_guard` from `actions_rs::log` (and also exports `actions_rs::env::is_github_actions`). This avoids false positives from outdated/incorrect web search results. If the repo does not depend on `actions-rs = "0.1"` in `Cargo.toml`, then normal missing-import/item checks can apply.

Applied to files:

  • tests/chain_integration.rs
📚 Learning: 2026-06-11T18:52:28.233Z
Learnt from: kjanat
Repo: kjanat/runner PR: 45
File: src/lib.rs:659-661
Timestamp: 2026-06-11T18:52:28.233Z
Learning: In Rust, it’s acceptable to use `matches!(cli.command, Some(cli::Command::Doctor { .. }))` (or similar) when the scrutinee comes from a field accessed through a shared reference (e.g., `cli: &Cli`). If the match pattern uses `..` and binds zero fields (so only the enum discriminant is matched), Rust match ergonomics will avoid moving out of the referenced value. Reviewers should not flag such code as a compile-blocking move; confirm with `cargo check` rather than forcing changes like `.as_ref()` when compilation succeeds.

Applied to files:

  • tests/chain_integration.rs
🪛 LanguageTool
README.md

[uncategorized] ~213-~213: The official name of this software platform is spelled with a capital “H”.
Context: ...notations panel. The annotations follow [github].group_output; the roll-up itself does...

(GITHUB)


[uncategorized] ~215-~215: Possible missing comma found.
Context: ...t` silences both, along with everything else runner prints.

I...

(AI_HYDRA_LEO_MISSING_COMMA)

Details 🔍 Remote MCP GitHub Grep

Additional review context

  • The linked kjanat/runner repository still exposes no searchable matches for the PR-specific symbols or GitHub Actions integration, so external repository-level validation remains unavailable.
  • Conventional Rust CLI implementations use Clap’s trailing_var_arg = true with allow_hyphen_values = true to forward arbitrary tool flags, including flags beginning with -. This supports reviewing forward_args_after_task against established CLI patterns.
  • GitHub Actions integrations commonly emit task/build sections through core.startGroup(...) followed by core.endGroup(), reinforcing that these are runner-owned workflow commands that must be suppressed or redirected in quiet machine-readable modes.
  • GitHub Actions integrations commonly use core.setFailed(...) for failure reporting, supporting the PR’s use of workflow error annotations for failed chain tasks.
  • npm package manifests conventionally expose executable mappings through the bin field, including both single-entry and multi-entry objects; examples include uglify-js, semver, pnpm, and other packages. This supports validating manifest-based local dependency resolution and ambiguity handling.
🔇 Additional comments (5)
tests/chain_integration.rs (1)

1070-1100: LGTM!

Also applies to: 1102-1123, 1125-1152

tests/fixtures/github-no-group/justfile (1)

6-8: LGTM!

tests/fixtures/github-no-group/runner.toml (1)

2-3: LGTM!

CHANGELOG.md (1)

12-36: LGTM!

Also applies to: 37-53

README.md (1)

202-216: LGTM!

Also applies to: 256-274, 327-335, 345-348, 445-445


📝 Walkthrough

Fix nested task dispatch and chain failure reporting across the CLI.

  • Preserve arguments through nested run/runner dispatch and correctly label package-manager exec primitives.
  • Detect and report recursive task-resolution cycles, including across process boundaries and in quiet mode.
  • Resolve locally installed dependency binaries from package manifests, including npm aliases and ambiguous or missing binaries.
  • Add chain summaries with task status, duration, skipped tasks, failure attribution, exit codes, and GitHub Actions annotations.
  • Suppress runner-owned output and GitHub Actions group markers under --quiet.
  • Update documentation, schema descriptions, and integration coverage.

Verified with 959 unit tests, 51 integration tests, formatting, linting, and schema checks.

Walkthrough

This change adds chain outcome summaries, failure attribution, GitHub Actions annotations, and quiet-mode output suppression. It forwards arguments after task names through runner and nested run invocations. Dispatch now resolves installed dependency binaries locally, labels the executed package-manager primitive, and propagates invocation frames to detect recursive cycles. Documentation, schemas, unit tests, and integration tests cover these behaviours.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

  • KAJ-290 — Covers chain failure summaries, per-task outcomes, durations, failure attribution, and GitHub Actions annotations.

Possibly related PRs

Suggested labels: refactor

Poem

Arrr, failed tasks now name their blame,
Quiet seas hide runner flame.
Local bins shun registry tides,
Cycles crash where guardrail abides.
Flags sail clean through every gate.

🚥 Pre-merge checks | ✅ 7 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Semver Version Bump Validation ⚠️ Warning Source files changed, but Cargo.toml stayed at 0.20.0 on both base and HEAD, so no required SemVer bump was made. Bump the crate version in Cargo.toml (and any other version file) to a higher SemVer, e.g. 0.21.0 for these backward-compatible changes.
✅ Passed checks (7 passed)
Check name Status Explanation
Title check ✅ Passed The title is descriptive, uses a conventional fix: prefix, and matches the nested dispatch and chain failure work.
Description check ✅ Passed The description clearly relates to the CLI fixes and linked issues, so it is on topic.
Linked Issues check ✅ Passed The implementation and tests cover #86, #87, #89, #90, and #91 with the expected behaviours.
Out of Scope Changes check ✅ Passed The changes shown are documentation, schema, tests, and code directly tied to the linked CLI fixes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 30.00%.
Changelog Update ✅ Passed PASS: 10 src files changed, and CHANGELOG.md updates ## [Unreleased] with Added/Fixed entries; no version bump/new version header is present.
Agents.Md Documentation Updated ✅ Passed No AGENTS.md files exist in the repository, so there is nothing to update for these CLI/workflow changes.
📋 Issue Planner

Built with CodeRabbit's Coding Plans for faster development and fewer bugs.

View plan used: #91


Comment @coderabbitai help to get the list of available commands.

@kjanat kjanat self-assigned this Jul 21, 2026
@kjanat kjanat added bug Something isn't working enhancement New feature or request area: cli Argument parsing, commands, and CLI UX documentation Improvements or additions to documentation cr:review Allow CodeRabbit review labels Jul 21, 2026
@coderabbitai coderabbitai Bot added the refactor Code cleanup or restructuring without behavior change label Jul 21, 2026
coderabbitai[bot]

This comment was marked as resolved.

@coderabbitai coderabbitai Bot removed the refactor Code cleanup or restructuring without behavior change label Jul 21, 2026
coderabbitai[bot]

This comment was marked as resolved.

`[github].group_output` gates only the annotations. The roll-up follows
`--quiet`/`--no-warnings` alone, so opting out of Actions decoration keeps
it. README and CHANGELOG claimed both followed group_output.

Pin all three gates with integration tests; the annotation paths had none.
@kjanat

This comment was marked as off-topic.

@coderabbitai

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Argument parsing, commands, and CLI UX bug Something isn't working cr:review Allow CodeRabbit review documentation Improvements or additions to documentation enhancement New feature or request refactor Code cleanup or restructuring without behavior change

Projects

None yet

1 participant