feat(unpinned-images): Docker CLI models with clap - #1711
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a clap-based Docker CLI argument model to extract image references from docker pull, docker run, and docker create invocations (intended for upcoming unpinned-images audit enhancements).
Changes:
- Added
models/docker.rswith clapParser/Args/Subcommandstructs andDockerCli::parse_image(args) -> Option<String>. - Added unit tests covering subcommand routing and common flag syntaxes/edge cases.
- Wired the new module into
crates/zizmor/src/models.rs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/zizmor/src/models/docker.rs | Adds clap-based Docker CLI parsing model + tests for image extraction. |
| crates/zizmor/src/models.rs | Registers the new docker models module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Parser, Debug)] | ||
| #[command( | ||
| no_binary_name = true, | ||
| disable_help_flag = true, | ||
| disable_version_flag = true | ||
| )] | ||
| struct DockerCli { | ||
| #[command(flatten)] | ||
| _global: DockerGlobalFlags, | ||
|
|
||
| #[command(subcommand)] | ||
| command: DockerCommand, | ||
| } |
There was a problem hiding this comment.
DockerCli::parse_image is pub(crate), but the DockerCli type itself is private (struct DockerCli). That makes the advertised API unusable from other modules (you can't name DockerCli outside this module), which will block the follow-up PR that consumes it. Make DockerCli pub(crate) (or expose a pub(crate) free function) so callers can access the parser without re-export hacks.
| // Boolean global flags | ||
| #[arg(short = 'D', long = "debug")] | ||
| _debug: bool, | ||
|
|
||
| #[arg(long = "tls")] | ||
| _tls: bool, | ||
|
|
||
| #[arg(long = "tlsverify")] | ||
| _tlsverify: bool, |
There was a problem hiding this comment.
Docker (via Go pflag) typically allows boolean flags to be specified with an explicit value (e.g. --debug=false, --tlsverify=true). Modeling these as plain bool flags in clap will reject --flag=false / --flag=true, causing try_parse_from to fail and parse_image to return None (missed detections). Consider switching boolean options to accept an optional bool value (e.g. action=Set, num_args=0..=1, default_missing_value="true", with a boolish value parser) so both --flag and --flag=false parse successfully.
|
Hi @woodruffw any update on this? Thanks! |
|
Sorry, I've been traveling and haven't had a lot of time to look at my review backlog. With some luck, I'll have time to look at this next week. |
Hey @woodruffw any updates? |
Pre-submission checks
Please check these boxes:
Mandatory: This PR corresponds to an issue (if not, please create
one first).
I hereby disclose the use of an LLM or other AI coding assistant in the
creation of this PR. PRs will not be rejected for using AI tools, but
will be rejected for undisclosed use.
If a checkbox is not applicable, you can leave it unchecked.
Summary
pull,run, andcreatesubcommandsDockerCli::parse_image(args: &[&str]) -> Option<String>-dit),--flag=valuesyntax, and embedded values (-eFOO=bar)-aflag ambiguity (boolean--all-tagsinpullvs value-consuming--attachinrun/create) is resolved per-subcommand automaticallyNone(safe false negatives)Part 1 of a PR chain splitting #1677 into reviewable pieces:
DockerCli::parse_image)Test plan
--flag=value,-aambiguity, global flags, trailingcommands, and edge cases
cargo clippy -p zizmorclean (only expecteddead_codewarning since PR 2 is the consumer)