Skip to content

fix(cli): validate logs --level and --source values - #3170

Open
natedemoss wants to merge 2 commits into
NVIDIA:mainfrom
natedemoss:fix/logs-filter-validation
Open

fix(cli): validate logs --level and --source values#3170
natedemoss wants to merge 2 commits into
NVIDIA:mainfrom
natedemoss:fix/logs-filter-validation

Conversation

@natedemoss

Copy link
Copy Markdown
Contributor

Summary

openshell logs accepts any string for --level and --source and forwards it to the gateway, where an unrecognized value is silently ignored rather than rejected. A typo produces wrong output with no indication anything went wrong, and the two flags fail in opposite directions.

Related Issue

No issue required: obvious localized bug fix in CLI argument handling.

Changes

  • --level is now a value enum (error, warn, info, debug, trace), case-insensitive, defaulting to no filter.
  • --source is now a value enum (gateway, sandbox, all), case-insensitive, still repeatable and still defaulting to all.
  • Added tests for accepted values in mixed case, rejected typos on both flags, repeated --source, and unchanged defaults.

The bug

--level warning instead of warn returns every level, and a mistyped --source returns nothing:

--level warn     -> shows ERROR,WARN
--level warning  -> shows ERROR,WARN,INFO,DEBUG,TRACE
--source gateway -> shows ["gateway"]
--source gatewy  -> shows []

That output is from running the gateway's own level_matches and source_matches (crates/openshell-server/src/grpc/validation.rs) against each level and source.

level_matches ranks an unrecognized level at 5, below every real level, so using one as the threshold lets everything through. source_matches compares by exact string, so a mistyped source matches nothing.

Why this belongs in the CLI

The server's // unknown levels always pass looks deliberate, and for a log line's own level it is: a sandbox emitting an unusual level should not silently disappear. The same to_num is also applied to the caller's threshold, where the same permissiveness just disables the filter.

Validating in the CLI keeps the server's behavior for log lines intact and rejects the case the user can actually get wrong, at the point where the error can name the valid values. It also gives the flags shell completion.

The gateway still accepts unrecognized levels from other clients. Tightening that is a wider change across the SDKs and seemed worth separating from this.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

No mise or Docker locally, so I ran the steps individually:

  • cargo fmt --all -- --check clean
  • cargo test -p openshell-cli passes, 0 failures, including the five new tests
  • cargo clippy -p openshell-cli --all-targets -- -D warnings clean

openshell logs --level warn, --level ERROR, and repeated --source all still parse as before, and the defaults are unchanged.

Two behavior changes worth calling out

--level ocsf used to be accepted. The gateway groups INFO | OCSF at the same rank, so it behaved as info. It is not in the docs or the flag help, so it is not in the enum, but it did work and would now be rejected. Happy to add it if you would rather keep it.

A script passing an invalid level today gets all levels back and exits 0. After this it gets an argument error. That is the point of the change, but it is a visible difference for anyone who has been relying on the broken filter without noticing.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Assisted by Opus 5.0.

Both flags took any string and forwarded it to the gateway, where an
unrecognized value is ignored rather than rejected. The two fail in
opposite directions: level_matches ranks an unknown level below every
real one, so `--level warning` returns every level instead of warn and
above, while source_matches compares exact strings, so a mistyped
`--source` returns nothing. Neither reports an error.

Make both flags value enums so a typo is an argument error that names
the valid values. Matching stays case-insensitive, `--source` is still
repeatable, and the defaults are unchanged.

The gateway's permissiveness is deliberate for a log line's own level,
since a sandbox emitting an unusual level should not disappear. The same
ranking is applied to the caller's threshold, where it only disables the
filter. Validating in the CLI leaves the log-line behavior alone and
rejects the value a user can actually get wrong.

Signed-off-by: Nathan DeMoss <ndemoss28@gmail.com>
Copilot AI lite review requested due to automatic review settings September 3, 2026 21:34
@copy-pr-bot

copy-pr-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A small test gap remains for the claimed case-insensitive --source behavior (mixed-case source values are not currently covered by tests).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR tightens openshell logs CLI argument handling by validating --level and --source values client-side (via clap ValueEnum) to prevent silent “no-op” filtering when users provide typos, while preserving the gateway’s permissive behavior for log-line metadata.

Changes:

  • Converted --level to a case-insensitive ValueEnum (error|warn|info|debug|trace) and made it optional to preserve the “no filter” default.
  • Converted --source to a case-insensitive, repeatable ValueEnum (gateway|sandbox|all) defaulting to all.
  • Added CLI parsing tests for accepted/rejected values, repeated --source, and unchanged defaults.
File summaries
File Description
crates/openshell-cli/src/main.rs Adds LogLevel/LogSource enums for clap validation and updates logs argument parsing plus unit tests.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/openshell-cli/src/main.rs
@johntmyers

Copy link
Copy Markdown
Collaborator

@natedemoss is this a co-pilot configuration you're specifically using in your environment?

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@natedemoss

Copy link
Copy Markdown
Contributor Author

Sorry for the delayed response, but if you asking about the copilot review, that's just a personal GitHub setting on my account that auto-requests Copilot review on any PR I open(think its a default setting tbh), nothing repo-side, happy to turn it off.

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.

3 participants