Skip to content

feat(localnet): add --json flag to logs command#39

Open
teyrebaz33 wants to merge 4 commits into
logos-co:masterfrom
teyrebaz33:feat/localnet-logs-json
Open

feat(localnet): add --json flag to logs command#39
teyrebaz33 wants to merge 4 commits into
logos-co:masterfrom
teyrebaz33:feat/localnet-logs-json

Conversation

@teyrebaz33
Copy link
Copy Markdown
Contributor

Closes #38

Summary

localnet logs was the only diagnostic command without --json support. This PR adds it for consistency with localnet status --json and doctor --json.

Changes

  • src/cli.rs: Added json: bool field to LocalnetLogsArgs
  • src/commands/localnet.rs: cmd_localnet_logs now accepts json param and emits structured output
  • tests/cli.rs: 3 new tests covering help text, empty log file, and tail output

Output format

{
  "tail": 50,
  "lines": [
    "2026-04-06T10:00:00Z sequencer started",
    "2026-04-06T10:00:01Z listening on 127.0.0.1:3040"
  ]
}

@teyrebaz33 teyrebaz33 requested a review from a team April 6, 2026 17:16
Comment thread src/commands/localnet.rs
@weboko
Copy link
Copy Markdown
Collaborator

weboko commented Apr 10, 2026

@teyrebaz33 please, sign commits so that we can merge :)

Copy link
Copy Markdown
Collaborator

@weboko weboko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds --json support to localnet logs to align diagnostic commands (localnet status, doctor) with machine-readable output expectations.

Changes:

  • Added --json flag plumbing for localnet logs from CLI parsing through to command dispatch.
  • Implemented structured JSON output for localnet logs, including tail and lines.
  • Added CLI tests covering help output and JSON behavior for missing/tailed logs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/cli.rs Adds json: bool to LocalnetLogsArgs and forwards it into LocalnetAction::Logs.
src/commands/localnet.rs Extends LocalnetAction::Logs with json and emits JSON output for logs.
tests/cli.rs Adds tests validating --json appears in help and JSON output shape for missing log file and tailing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/commands/localnet.rs Outdated
if !log_path.exists() {
println!("log file does not exist yet: {}", log_path.display());
return Ok(());
return print_log_lines(tail, &[], log_path, json);
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmd_localnet_logs no longer distinguishes a missing log file from an empty one in text mode: when the file doesn’t exist it now flows through print_log_lines and prints “log file is empty”. This is misleading (and differs from read_log_tail, which reports missing files explicitly). Consider keeping the old “does not exist yet” message for non---json output, while still emitting an empty JSON payload for --json.

Suggested change
return print_log_lines(tail, &[], log_path, json);
if json {
return print_log_lines(tail, &[], log_path, true);
}
println!("log file does not exist yet: {}", log_path.display());
return Ok(());

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the latest commit: when the log file does not exist and --json is off, we now print the original 'log file does not exist yet' message. Only the JSON path goes through print_log_lines with an empty slice.

Comment thread src/commands/localnet.rs Outdated
Comment on lines +347 to +352
let content = fs::read_to_string(log_path)
.with_context(|| format!("failed to read log file {}", log_path.display()))?;

if content.trim().is_empty() {
println!("log file is empty: {}", log_path.display());
return Ok(());
}

let lines: Vec<&str> = content.lines().collect();
let start = lines.len().saturating_sub(tail);
for line in &lines[start..] {
println!("{line}");
}

Ok(())
print_log_lines(tail, &lines[start..], log_path, json)
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous content.trim().is_empty() check was removed. As a result, files containing only whitespace/newlines will be treated as having log lines (possibly empty strings) and will be printed/emitted in JSON rather than reported as empty. If the intent is “no meaningful log output yet”, consider restoring a trim/blank check (and/or filtering empty lines) before building the lines array.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: added a blank-line filter before slicing the tail, so files containing only whitespace/newlines are treated as empty and reported accordingly.

@teyrebaz33 teyrebaz33 force-pushed the feat/localnet-logs-json branch from b4a96e3 to 69420ee Compare April 11, 2026 12:13
@teyrebaz33
Copy link
Copy Markdown
Contributor Author

All commits have been re-signed with GPG — they now show as Verified on GitHub.

@fryorcraken
Copy link
Copy Markdown
Collaborator

fryorcraken commented May 1, 2026

Thanks @teyrebaz33.

Please revise against the points below, then ping me for another pass. The repo's CONTRIBUTING.md is the contract these come from.

  • Keep the PR to exactly the scope the linked issue asked for
  • Treat the PR template and CONTRIBUTING.md as the bar, not boilerplate
  • Read your own diff and run the repo's local checks before pushing
  • Walk the DOGFOODING flow for diagnostics changes before asking for review

@fryorcraken
Copy link
Copy Markdown
Collaborator

First — apologies for the three-week silence on our side. That's on us, not you. Thanks for sticking with this and for already turning around two rounds of review feedback; the --json shape and CLI plumbing are both right where we want them.

A few small things to clean up before we land it:

Local checks before the next push:

cargo fmt              # currently fails: 3 spots in src/commands/localnet.rs and tests/cli.rs
cargo test --all-targets
git rebase origin/master   # branch picked up a conflict during the wait

The tail-handling refactor went a touch further than it needed to. Issue #38 only asked for --json, and the blank-line filter at src/commands/localnet.rs:354-358 answers the Copilot nit about whitespace-only files by also dropping blank lines mid-content. Two side effects:

  1. --tail N now means "last N non-blank lines" instead of "last N lines of the file" — a quiet semantics change on the human path.
  2. localnet logs --tail 0 on a non-empty file prints "log file is empty: …" because print_log_lines decides emptiness from the post-tail slice.

Easiest fix is the smaller one: restore the original if content.trim().is_empty() early-return that handles whitespace-only files, and keep the raw lines for slicing. The --tail 0 bug then falls out for free.

A few more tests would lock the edges down:

  • whitespace-only log file → lines: [] / "log file is empty"
  • non-empty file + --tail 0 → empty output, no "empty" message
  • log file with embedded blank lines → preserved (or dropped — whichever you pick, assert it)

One note on goalposts that moved while you waited. Some of the contributor expectations (filling in Your Project or User Demand / User Story / Verification in the PR body, and updating DOGFOODING.md when user-facing behavior changes) were added to CONTRIBUTING.md after you opened this PR — so it's fair that you didn't follow them the first time. Since you'll be touching the PR anyway:

  • Refill the PR body using .github/PULL_REQUEST_TEMPLATE.md.
  • Add a --json line to DOGFOODING.md D2 with the expected JSON shape, mirroring how localnet status --json is treated in the same scenario.

Totally optional, not blocking: the new JSON payload uses serde_json::json!{...}; elsewhere in the file (e.g. cmd_localnet_status at localnet.rs:301-313) we emit via #[derive(Serialize)] structs from model.rs. A LocalnetLogsReport { tail, lines } type would match house style and give future fields a clean home — but happy to land it as-is and refactor later.

Thanks again for your patience — once the above is in we should be able to merge quickly this time.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add --json flag to localnet logs command

4 participants