fix(requestflag): preserve piped query arrays - #167
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 34189639373 --repo openai/openai-cli \
--name castiron-custom-code-34189639373-1 --dir /tmp/castiron-custom-code-34189639373-1
git apply --stat /tmp/castiron-custom-code-34189639373-1/custom-code.patch
cat /tmp/castiron-custom-code-34189639373-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 094a4ab96e80c2952c830ae36a9a9f009617e5ec e50e11ff2dfda799c1564883ec877ffee3bcd1ee
python3 scripts/castiron/custom_code_report.py report \
--base 094a4ab96e80c2952c830ae36a9a9f009617e5ec \
--head e50e11ff2dfda799c1564883ec877ffee3bcd1ee --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-e50e11ff2dfd
cat /tmp/castiron-custom-code-e50e11ff2dfd/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.
🟢 Approval recommended
The change is narrowly scoped to stdin→query slice handling and is backed by targeted unit + end-to-end tests that exercise precedence, validation, and security-related literal handling.
Pull request overview
This PR fixes how piped YAML/JSON query arrays are applied to request flags so that a collection from stdin is treated as multiple flag elements (matching repeated --flag value behavior) rather than a single serialized element, while preserving existing precedence/provenance rules and non-query behavior.
Changes:
- Route stdin-driven flag assignment through a new helper that can special-case query slice flags to populate collections element-by-element.
- Implement collection-safe assignment for
Flag[T]query slices to avoid partial mutation on conversion/validation failures. - Add unit and end-to-end regression tests covering JSON/YAML parity, explicit precedence, and untrusted-stdin file-reference literal behavior.
File summaries
| File | Description |
|---|---|
pkg/cmd/stdincollection_test.go |
New end-to-end test asserting query array behavior matches repeated flags for JSON/YAML stdin and preserves untrusted reference literals. |
internal/requestflag/stdincollection.go |
Adds stdin-to-flag assignment helper and slice-query collection assignment path using existing element parsing/validation. |
internal/requestflag/stdincollection_test.go |
Adds unit tests validating stdin collection semantics, precedence, aliases, and non-query compatibility behavior. |
internal/requestflag/requestflag.go |
Switches stdin application for query/header/path flags to the new helper to enable collection-aware assignment. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Thoroughly reviewed the exact head e50e11ff2dfda799c1564883ec877ffee3bcd1ee. The stdin collection path preserves repeated query-array semantics, explicit CLI precedence, aliases/provenance, validation atomicity, scalar/null compatibility, and existing non-query behavior. Targeted unit and end-to-end tests pass, ./scripts/lint and go vet ./... pass, and hosted exact-head CI is green. The full local suite encountered TestFilesCreateCLICancelClosesStalledFIFO, which reproduces unchanged on current main and is unrelated to this diff.
Piped JSON or YAML query arrays were passed to the flag parser as one serialized element. For example,
responses retrievewith{"include":["first","second"]}sent oneinclude[]value containing the JSON array instead of the same two values produced by--include first --include second.Assign typed query collections through the existing element converter, then set the complete collection once. Preserve explicit flag precedence, aliases, provenance, scalar/null compatibility, and existing header/path/body behavior. Conversion and validation failures do not leave a partially assigned collection. No generated commands, schemas, dependencies, or policy files change.
Validation
Passed locally with Go 1.27.0 on Linux:
go test ./internal/...go test ./... -run '^$'(compilation only)go test ./internal/requestflag ./pkg/cmd -run 'Stdin|ApplyStdinDataToFlags' -count=1-racego vet ./internal/requestflag ./pkg/cmdgo mod verify./scripts/lint(Go build)Full local mock suite NOT RUN: the pinned Steady setup/cache blocker persists; the supported launcher reports a missing cache, and the previously denied bootstrap was not retried or bypassed. Full hosted CI/mock tests and completed bot reviews with resolved feedback remain mandatory before SDK review.