feat(cli): use --node for dora logs node selection

[GHX5T-SOL]

Fixes #1880

Summary

• make dora logs node selection explicit with --node/-n
• reject the old dora logs DATAFLOW NODE shape in parser tests
• update CLI/logging docs and user-facing hints to show the new form

Validation

• cargo test -p dora-cli command::tests:: passed: 74 passed, 0 failed, 78 filtered out
• cargo fmt --all -- --check passed
• git diff origin/main..HEAD --check passed
• gitleaks detect --source . --log-opts=origin/main..HEAD --redact --no-banner passed: no leaks found
• cargo clippy -p dora-cli --all-targets -- -A clippy::unnecessary-sort-by -D warnings passed

Note: strict clippy without the allow currently hits the existing Rust 1.95 clippy::unnecessary_sort_by warning in binaries/coordinator/src/lib.rs:3698, outside this change.

[trunk-io]

😎 Merged manually by @heyong4725 - details.

[phil-opp]

This is a breaking change, right? Can we perhaps add a hidden positional argument that gives a better error message? E.g. something like 'use the --node argument' instead of just failing with a generic "unexpected argument" error?

There are also some docs that still use the old syntax after this PR, e.g. quickstart.md or the guide. These need to be fixed before merging.

[GHX5T-SOL]

Addressed this in 17bb016a:

• added a hidden legacy positional node argument so dora logs <dataflow> <node> reaches a targeted runtime hint instead of a clap parse error
• kept the --node path as the documented node filter
• updated the stale quickstart and README.zh-CN references

Validation:

• cargo fmt -p dora-cli -- --check
• cargo test -p dora-cli command::tests::parse_logs -- --nocapture (6 passed, 0 failed)
• cargo run -p dora-cli --bin dora -- logs my-dataflow sensor -> reports: "hint: use dora logs my-dataflow --node sensor instead"
• git diff --check
• git diff -- binaries/cli/src/command/logs.rs binaries/cli/src/command/mod.rs README.zh-CN.md docs/quickstart.md | gitleaks detect --pipe --redact --no-banner -> no leaks found

[heyong4725]

Did a pre-landing review pass. The PR is solid: the hidden-legacy-positional + targeted-hint approach from phil-opp's review was the right call, the parser tests cover the main shapes, and the doc updates are thorough (README, README.zh-CN, all 5 docs/*.md, plus the daemon-side error message in pending.rs). All 7 new parser tests pass locally.

Three minor UX nits worth flagging. None of these are merge blockers, just polish opportunities for either this PR or a follow-up.

1. Misleading hint when both legacy positional and --node are set

binaries/cli/src/command/logs.rs:158-167 — reject_legacy_node_arg constructs the hint from args.legacy_node. When the user types dora logs DF NODE --node OTHER, both fields populate and the bail message ends up saying "use dora logs DF --node NODE" — pointing them at the legacy value they're already trying to override with their explicit --node OTHER.

Suggested message branch:

match (&args.legacy_node, &args.node) {
    (Some(legacy), Some(explicit)) => bail!(
        "positional node `{legacy}` is not allowed alongside `--node {explicit}`; \
         drop the positional argument"
    ),
    (Some(legacy), None) => bail!(
        "positional node argument `{legacy}` is no longer supported\n\n  \
         hint: use `dora logs {dataflow} --node {legacy}` instead"
    ),
    _ => Ok(()),
}

2. legacy_positional_node heuristic fires on any lookup failure

binaries/cli/src/command/logs.rs:169-173 — the hint "if X is a node name, use --node X" is appended whenever a single-positional dora logs X resolution fails, regardless of why. That's the intended help when X was actually a misplaced node name, but it's confusing for legitimate failures (coordinator unreachable, transient 500, dataflow in Failed state) where X IS a real dataflow.

Worth narrowing to only the "not found" error class once you can tell them apart from resolve_dataflow_identifier_interactive's error shape. INVESTIGATE — depends on whether that resolver distinguishes lookup-miss from transport errors.

3. legacy_node lacks conflicts_with declarations + test gap

binaries/cli/src/command/logs.rs:28-31 — legacy_node has no conflicts_with for node or all_nodes. The combinations dora logs DF NODE --all-nodes and dora logs DF NODE --node OTHER parse successfully and then reject_legacy_node_arg fires with a hint that, in both cases, contradicts the user's other flag. Adding conflicts_with = ["node", "all_nodes"] on legacy_node would let clap reject the worst combinations at parse time with a cleaner error path; alternatively keep the runtime guard but make the hint conditional on the other flags (as in #1).

A parse_ok test for dora logs my-dataflow sensor --node other (followed by an integration-style assertion on execute() failure) would catch any regression in the hint message.

---

Reproduced locally with cargo test -p dora-cli --lib -- parse_logs reject_logs (7/7 pass on the existing tests). Happy to follow up with a separate cleanup PR if maintainers prefer to land this as-is and iterate.

[GHX5T-SOL]

Addressed the conflict-handling nits in b15efe2.\n\nChanges:\n- Added clap conflicts for the hidden legacy positional node argument against both --node and --all-nodes, so dora logs DF NODE --node OTHER and dora logs DF NODE --all-nodes now fail at parse time with a direct conflict error.\n- Added parser regression coverage for both combinations.\n- Softened the fallback lookup hint to say "if you intended ... as a node name" so it is less misleading when the underlying dataflow lookup failed for another reason.\n\nValidation:\n- cargo fmt -p dora-cli -- --check\n- cargo test -p dora-cli command::tests:: -- --nocapture -> 76 passed, 0 failed\n- cargo run -p dora-cli --bin dora -- logs my-dataflow sensor --node other -> rejects [NAME] with --node\n- cargo run -p dora-cli --bin dora -- logs my-dataflow sensor --all-nodes -> rejects [NAME] with --all-nodes\n- git diff --check\n- changed diff gitleaks scan -> no leaks found

[phil-opp]

Thanks!

[heyong4725]

Solid PR — clean design, comprehensive parser tests, and a thoughtful migration path with helpful runtime hints. Scope matches issue #1880 exactly.

0 critical, 5 informational

[INFO] Coordination with PR #1884

This PR's docs/debugging.md changes (5 syntax updates) overlap exactly with PR #1884's same-line edits. If both PRs target main, #1884 will hit merge conflicts on those lines.

Recommended sequence: land this PR first, then #1884 rebases. After rebase, #1884's 5 redundant syntax-change lines disappear and its diff shrinks to just the new error table (which is what #1878 actually requested from #1884).

I'll post a follow-up on #1884 noting this.

[INFO] (confidence: 7/10) legacy_positional_node hint wording is misleading on dataflow typos

For a user who typos a dataflow name (dora logs my-datflow ← missing letter), the hint reads:

failed to resolve dataflow `my-datflow`

  hint: if you intended `my-datflow` as a node name, use `dora logs --node my-datflow`

Typo is more likely than syntax-form confusion. A neutral wording would handle both cases without prejudice:

could not resolve `my-datflow` as a dataflow.
  - run `dora list` to see active dataflows
  - or use `dora logs <dataflow> --node my-datflow` if it's a node name

Mild — users will figure it out — but worth a small tweak to resolve_logs_dataflow_identifier.

[INFO] (confidence: 8/10) -n reassignment from --tail to --node deserves a callout

docs/logging.md:258 had --tail | -n in the flags table, but main's LogsArgs::tail is #[clap(long)] with no short form. So that doc table was already wrong; the PR correctly removes the stale -n=tail claim and assigns -n to --node.

A one-line note in the PR description ("Note: -n was incorrectly documented as --tail's short form in docs/logging.md; it was never actually wired up in the parser") would help anyone reviewing the doc change later.

[INFO] (confidence: 6/10) #[command(allow_missing_positional = true)] removed without explanation

-    #[command(allow_missing_positional = true)]
     #[clap(display_order = 11)]
     Logs(LogsArgs),

Both positionals (dataflow, legacy_node) are Option<...>, so the attribute may not be strictly required — parse_logs, parse_logs_node_flag, etc. all pass without it. The removal is probably intentional (cleaner signature for the new design), but a one-line note in the PR description would help future reviewers know it's not accidental.

[INFO] (confidence: 7/10) No integration test asserting the runtime hint message format

reject_legacy_node_arg produces a user-facing message that's load-bearing for the migration UX:

bail!(
    "positional node argument `{node}` is no longer supported\n\n  \
     hint: use `dora logs {dataflow} --node {node}` instead"
);

parse_logs_legacy_positional_node_for_runtime_hint confirms the old syntax parses (so the runtime path is reachable), but nothing asserts the hint message itself. Drift risk is low — single bail!() call — but a cheap snapshot test would lock the user-facing contract:

#[test]
fn legacy_positional_emits_helpful_hint() {
    let args = LogsArgs {
        dataflow: Some("my-dataflow".into()),
        legacy_node: Some("sensor".parse().unwrap()),
        // ...defaults
    };
    let err = reject_legacy_node_arg(&args).unwrap_err();
    let msg = err.to_string();
    assert!(msg.contains("positional node argument `sensor` is no longer supported"));
    assert!(msg.contains("dora logs my-dataflow --node sensor"));
}

Non-blocking — but the migration hint is part of the user contract.

[INFO] (confidence: 9/10) Breaking-change call-out for CHANGELOG / release notes

The PR correctly notes "reject the old dora logs DATAFLOW NODE shape," but downstream users with shell scripts running dora logs X Y will hit the runtime bail. The bail's hint smooths the migration, but a CHANGELOG entry like:

Breaking: dora logs <dataflow> <node> is rejected. Use dora logs <dataflow> --node <node> instead. The CLI prints a helpful hint when it sees the old form.

Would help anyone reading the release notes know to grep their scripts.

---

Coverage

CODE PATH COVERAGE
═══════════════════════════════════════
[+] binaries/cli/src/command/logs.rs
    ├── LogsArgs parse paths
    │   ├── [★★★ TESTED] new --node form, -n short, combo with dataflow,
    │   │                --all-nodes, legacy positional parses, all conflict combos
    │   │                → 8 new tests in command/mod.rs, all comprehensive
    │   ├── reject_legacy_node_arg()
    │   │   └── [GAP] error message format — no assertion that hint contains expected text
    │   ├── legacy_positional_node()
    │   │   └── [GAP] heuristic behavior — only exercised via integration paths
    │   └── resolve_logs_dataflow_identifier()
    │       └── [GAP] hint-wrapping when resolution fails — no test
─────────────────────────────────────
COVERAGE: 8/13 paths tested (62%)
GAPS: 5 paths, all error-message-format (low criticality)
─────────────────────────────────────

Strong parser-level coverage. The gaps are about asserting user-facing error message wording, which is reasonable to skip given the strings are single-source-of-truth in bail!() calls.

Adversarial probe (diff is 225 lines — large tier)

Tried hard to find real bugs. What I checked:

• Clap conflicts_with_all with hide = true — works, covered by reject_logs_legacy_positional_node_with_all_nodes_flag ✓
• Test safety — parse-only, no subprocess. Safe ✓
• Doc completeness — grepped docs/, README*.md, binaries/ for remaining old syntax; this PR catches all of them ✓
• Daemon's user-facing error string (binaries/daemon/src/pending.rs) updated to use --node ✓
• Downstream scripts — yes, this breaks them, but that's the explicit intent per feat(cli): convert dora logs to --node flag + positional dataflow (residual #1059 gap) #1880 and the bail-with-hint smooths the path

Nothing P0/P1 surfaced.

---

Summary

Ready to land. The 5 informational items are optional polish — none blocks merge. The most useful one to apply right now is the hint-wording tweak (so first-time users hitting a typo don't get pushed toward --node by mistake), but even that can wait for a follow-up.

Cross-PR coordination: please land this before #1884 so #1884 can rebase cleanly. I'll comment over there to mention the dependency.