fix(cmd): expand file references in nullable string flags - #168
fix(cmd): expand file references in nullable string flags#168markstuart-oai wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 5 mixed files remain; 0 existing customizations changed. Compared 5 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34192725852 --repo openai/openai-cli \
--name castiron-custom-code-34192725852-1 --dir /tmp/castiron-custom-code-34192725852-1
git apply --stat /tmp/castiron-custom-code-34192725852-1/custom-code.patch
cat /tmp/castiron-custom-code-34192725852-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 265cf6378ab2e6a27c17bc7c0b6a6cbe6dbd477b 5e20def9b28d790105aa9d9a1f6414d1e5165c98
python3 scripts/castiron/custom_code_report.py report \
--base 265cf6378ab2e6a27c17bc7c0b6a6cbe6dbd477b \
--head 5e20def9b28d790105aa9d9a1f6414d1e5165c98 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-5e20def9b28d
cat /tmp/castiron-custom-code-5e20def9b28d/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes file-path/request-parsing behavior at a security boundary (explicitly requiring CODEOWNER review) and includes an end-to-end test that should be hardened for reliability.
Pull request overview
This PR fixes inconsistent “@file”-style expansion for nullable string CLI flags by ensuring non-nil *string values go through the same file-embedding logic as plain string flags, while preserving nil as JSON null.
Changes:
- Update
embedFilesValueto unwrap non-nil*stringvalues before applying file-reference expansion. - Add focused unit coverage for
embedFileswith nullable string pointers. - Add an end-to-end CLI test covering nullable vs ordinary string behavior, null/unset, escaping, missing-file errors, binary sniffing, and untrusted-stdin controls.
File summaries
| File | Description |
|---|---|
| pkg/cmd/flagoptions.go | Unwraps non-nil *string to apply existing file embedding/expansion logic consistently. |
| pkg/cmd/nullablefile_test.go | Adds unit + CLI end-to-end tests to validate nullable string file-reference expansion and related edge cases. |
Review details
- Files reviewed: 2/2 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.
Summary
Nullable string flags such as
vector-stores update --name @filecurrently send the literal reference, while ordinary string flags expand it. Route nonnil*stringvalues through the existing file expansion logic so explicit@file,@file://and@data://references behave consistently.Nil remains JSON null. Unset values, escaped literals, missing-file errors, text/binary encoding, and untrusted-stdin provenance retain their existing behavior. Other pointer types are unchanged. SDK CODEOWNER review is required for this file-path/request-parsing change.
Validation
go test ./internal/...;go test ./... -run '^$';go mod verify; and./scripts/lint(build).go vet ./internal/requestflag ./pkg/cmd.GOOS=windows GOARCH=amd64 go test -c); no native Windows runtime testing../scripts/testNOT RUN: the pinned Steady tool cache is missing or linked, so the supported launcher cannot start the mock. Full hosted CI/mock results remain required; no hosted pass is claimed here.