Skip to content

refactor(pass)!: return errors from run options - #662

Merged
Benehiko merged 5 commits into
mainfrom
codex/run-option-errors
Sep 24, 2026
Merged

Benehiko merged 5 commits into
mainfrom
codex/run-option-errors

Conversation

@Benehiko

Copy link
Copy Markdown
Member

RunCommand currently accepts invalid options during construction: an empty socket path silently selects the Desktop default, and negative timeout errors surface only when the command executes.

Make RunOption return an error and have RunCommand return the first option error before creating a command. Reject empty socket paths and negative request/response timeouts. Zero timeouts remain valid, and omitting WithSocketPath uses the Desktop default.

BREAKING CHANGE: RunCommand now returns (*cobra.Command, error), and RunOption now has signature func(*runOpts) error. Callers must handle the constructor error before using the command. Replace WithSocketPath("") with omission of the option when the default is intended.

Validation:

  • go test -race ./plugins/pass/commands
  • Linux and Windows cross compilation of ./plugins/pass/...
  • Constructor tests cover invalid options, zero timeouts, defaults, and stopping at the first option error; existing subprocess tests cover command execution.
  • git diff --check

@docker-agent docker-agent 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.

Assessment: 🟢 APPROVE

BREAKING CHANGE: RunCommand now returns (*cobra.Command, error), and RunOption returns error. Invalid socket paths and timeouts fail during construction.
@Benehiko
Benehiko force-pushed the codex/run-option-errors branch from d51411c to 800dd9f Compare September 24, 2026 07:42
Comment thread plugins/pass/commands/run.go Outdated

@docker-agent docker-agent 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.

Assessment: 🟢 APPROVE

@docker-agent docker-agent 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.

This review covers only the commits pushed since 6eae107.

Assessment: 🟢 APPROVE

The incremental diff contains only documentation comment changes to plugins/pass/commands/run.go:

  • Removal of the now-redundant godoc line // RunOption configures a run command and reports invalid option values. above the RunOption type (the type signature itself is self-documenting after the earlier refactor).
  • Simplification of the RunCommand godoc from two lines to one: // RunCommand uses [api.DesktopSocketPath] by default.

No logic, behaviour, exported API, or test code was changed in this incremental push. No bugs introduced.

Comment thread plugins/pass/commands/run.go Outdated
Comment thread plugins/pass/commands/run.go Outdated
Comment thread plugins/pass/commands/run.go Outdated
@Benehiko
Benehiko merged commit 6953ea2 into main Sep 24, 2026
12 checks passed
@Benehiko
Benehiko deleted the codex/run-option-errors branch September 24, 2026 08:45
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