Skip to content

fix(cli): reject extra inspect args and add stdin BATS coverage - #4014

Merged
dmihalcik-virtru merged 1 commit into
mainfrom
dspx-2604-08-streamio-followups
Sep 9, 2026
Merged

dmihalcik-virtru merged 1 commit into
mainfrom
dspx-2604-08-streamio-followups

Conversation

@dmihalcik-virtru

@dmihalcik-virtru dmihalcik-virtru commented Sep 8, 2026

Copy link
Copy Markdown
Member

Follow-up to review feedback on #3937 (chore(cli): move streaming IO helpers into pkg, merged into main).

Changes

  • otdfctl/cmd/tdf/inspect.go: inspectDoc.Args = cobra.MaximumNArgs(1), so otdfctl inspect a.tdf b.tdf now errors instead of silently ignoring b.tdf.
  • otdfctl/e2e/encrypt-decrypt.bats: added a test piping a TDF into inspect via stdin, and a test asserting extra positional args now fail.

Not changed

TestOpenSeekableSpoolsNonSeekableNamedFile in otdfctl/pkg/streamio/input_test.go was also flagged (uses syscall.Mkfifo, not portable to Windows) — it already guards with a runtime.GOOS == "windows" skip, so no change was needed. Verified by running the test.

Testing

  • go build ./... / go vet ./... — pass
  • golangci-lint run ./otdfctl/cmd/tdf/... — 0 issues
  • bats --count e2e/encrypt-decrypt.bats — parses, 19 tests (up from 17)
  • Manually verified otdfctl inspect a.tdf b.tdf now errors with accepts at most 1 arg(s), received 2

Note: encrypt-decrypt.bats has a pre-existing file-level skip in setup_file (unrelated: namespaced subject-mapping API dependency), so the new BATS tests won't run in CI until that skip is lifted, same as all other tests in that file.

Summary by CodeRabbit

  • Bug Fixes

    • The inspect command now rejects requests containing more than one positional argument, providing clearer command-line validation.
  • Tests

    • Added coverage confirming that valid TDF3 input reports the expected key-access entry.
    • Added coverage confirming that extra positional arguments result in a command failure.

Follow-up to review feedback on #3937 (dspx-2604-08-streamio):

- inspectDoc now sets Args = cobra.MaximumNArgs(1), so extra positional
  arguments (e.g. `otdfctl inspect a.tdf b.tdf`) are rejected instead of
  silently ignoring everything after the first path.
- Add BATS coverage for piping a TDF into `inspect` via stdin, and for the
  new extra-args rejection, in otdfctl/e2e/encrypt-decrypt.bats.

TestOpenSeekableSpoolsNonSeekableNamedFile in otdfctl/pkg/streamio was also
flagged as using syscall.Mkfifo, which is not portable to Windows; it
already guards with a runtime.GOOS == "windows" skip, so no change was
needed there.

Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 592ef4dc-603c-4a6a-b77b-aafb8bb4d654

📥 Commits

Reviewing files that changed from the base of the PR and between 0aecc64 and 8de5cde.

📒 Files selected for processing (2)
  • otdfctl/cmd/tdf/inspect.go
  • otdfctl/e2e/encrypt-decrypt.bats

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The inspect command now accepts at most one positional argument. End-to-end tests verify successful TDF3 inspection and failure when extra arguments are provided.

Changes

Inspect argument validation

Layer / File(s) Summary
Inspect command contract and validation
otdfctl/cmd/tdf/inspect.go, otdfctl/e2e/encrypt-decrypt.bats
The command limits positional arguments with cobra.MaximumNArgs(1). Tests verify the key-access count and reject extra positional arguments.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 8de5c

The inspect command now rejects extra positional arguments while retaining single-input and stdin inspection behavior. The updated end-to-end coverage supports merge readiness with no remaining actionable risk.

Suggested reviewers: alkalescent

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes both main changes: rejecting extra inspect arguments and adding stdin BATS coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dspx-2604-08-streamio-followups

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.

❤️ Share

A rabbit checks the inspect command,
One path goes in, as rules demand.
Extra hops now fail the gate,
TDF3 counts come back straight.
Tests thump softly, green and bright,
The burrow ships the change tonight.

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

@github-actions github-actions Bot added the size/s label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • otdfctl
  • service
  • tests-bdd

See the workflow run for details.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 180.925394ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 100.023573ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 339.898142ms
Throughput 294.21 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 48.269276785s
Average Latency 481.582573ms
Throughput 103.59 requests/second

@dmihalcik-virtru
dmihalcik-virtru marked this pull request as ready for review September 9, 2026 12:53
@dmihalcik-virtru
dmihalcik-virtru requested a review from a team as a code owner September 9, 2026 12:53

@c-r33d c-r33d 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.

Wouldn't you want ExactArgs(1) since it should just be 1 TDF?

@dmihalcik-virtru

Copy link
Copy Markdown
Member Author

Wouldn't you want ExactArgs(1) since it should just be 1 TDF?

0 args falls back to standard input

@dmihalcik-virtru
dmihalcik-virtru added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit b3a0c9c Sep 9, 2026
50 checks passed
@dmihalcik-virtru
dmihalcik-virtru deleted the dspx-2604-08-streamio-followups branch September 9, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants