Repository navigation
Show model IDs and owners with filtering and loading feedback - #391
Conversation
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
9ef6272 to
f9de682
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
203d7aa to
ff042a3
Compare
f9de682 to
cbe90a3
Compare
saioai
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 2 mixed files remain; 0 existing customizations changed. Compared 2 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 37862135542 --repo openai/openai-cli \
--name castiron-custom-code-37862135542-1 --dir /tmp/castiron-custom-code-37862135542-1
git apply --stat /tmp/castiron-custom-code-37862135542-1/custom-code.patch
cat /tmp/castiron-custom-code-37862135542-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin c65e03e16f537faf85d2522c31b431a4ac1c550a 996def6c5367e61ddf54fa9512bdb01d1ccd3b69
python3 scripts/castiron/custom_code_report.py report \
--base c65e03e16f537faf85d2522c31b431a4ac1c550a \
--head 996def6c5367e61ddf54fa9512bdb01d1ccd3b69 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-996def6c5367
cat /tmp/castiron-custom-code-996def6c5367/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed cbe90a336390688c7723537cec2c2eeb8c812868. I found two issues: the Models print action omits standard metadata, and the public single-response check still expects an ID-only table. Details are inline; leaving this review without approval.
The filter/selection path retains original records and sorts before limiting only when selection controls are used. Loading feedback stops before result rendering, and the viewer uses the already-loaded single response. No new file crosses 1,000 lines.
Validation: source review, including the affected tests and public check scripts. I did not run repository workloads or verify the demo recordings. The macOS scenarios and focused race checks in the description remain author-reported. Current-head hosted tests, build, lint, help checks, CodeQL, and Castiron baseline/budget checks passed. The queued-run signaling and unused prepare jobs were skipped.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26e23a34ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 26e23a34ef1bf3ee552476fa5b4e53d488df8451 against the previous reviewed head. Both of my earlier findings are fixed: Models p preserves standard metadata through copied output options, and the public checker now expects the ID/OWNER table. The new regression covers ordinary records and preserves terminal escaping.
I also verified the source path behind the existing Ctrl+C finding. In the loaded viewer, Ctrl+C takes the same successful quit path as q; it cancels only the child fetch context, so the outer signal handler cannot convert the result to status 130. Please retain an interrupt result while keeping normal quit successful. I am leaving this review without approval while that issue remains open.
Validation: source review, not a local PTY reproduction. Current-head tests, build, lint, help, CodeQL, and Castiron checks passed; conditional jobs were skipped. The reported native scenarios remain author-reported. Combining this branch with the image-loading helper change still needs the separately noted caller adaptation.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 70282f38289b5e42cf9741ae8789893b36076c25. The remaining Ctrl+C finding is fixed: Models records the existing typed interrupt before canceling the viewer, retains it through terminal cleanup, and returns status 130 through the command error handler. Normal q/Escape/p exits stay on their existing path, and output failures keep precedence. The earlier complete-record printing and ID/OWNER checker fixes remain intact. I found no further blocking issue in this revision.
Validation: source review of the change and regression test, with the unchanged files compared against the previous reviewed head. Hosted tests, lint, help, CodeQL, and Castiron checks passed. The build-artifacts job failed in scripts/govulncheck on the runner's Go 1.26.8 standard library, which also failed the aggregate build check; this approval does not establish green CI. Native terminal and race results remain author-reported. I did not run repository workloads. The separately noted Models/Image loading-helper integration is still outside this head.
List commands need handwritten navigation support before generated commands can pass typed API pages to it. This prerequisite adds that runtime without activating it in public commands. Generated activation will follow the normal compiler and SDK-promotion sequence. ### What changes - Add helpers that retain loaded pages and fetch another page only when requested. - Keep complete IDs available through labeled page printing after terminal restoration. - Preserve first-request errors, zero and unlimited limits, cancellation, and output failures. - Register the runtime's fixed local error type alongside existing help errors. Before activation, navigation also needs the reviewed one-screen and tab-aware completion follow-ups. Those changes remain separate: `51bac6bca356f067ee671add8bac630b8aa87658` and `f4c1b76603b5830e61143255a3cddd25a9ee09af`. Combined activation must also apply the separate quiet policy to optional summary hints while preserving records and navigation controls. This PR prepares a dependency; it does not complete SDK-1178 or SDK-1179. The shared completion follow-ups are being included in [openai#391](openai#391). ### Commands No commands, flags, or public output behavior change in this prerequisite. ```sh openai files list --max-items 2 openai batches list --format json openai admin projects list ``` These commands retain current-main behavior. The new viewer remains inactive until generated page adapters are promoted. ### Code `pkg/custom/list_navigation.go` owns page fetching, navigation, and cleanup. The local error presenter recognizes its fixed diagnostics. Focused helper tests and real-command noninteractive checks retain the existing contracts. The recording recipe explicitly requires a separately activated candidate. Its relay preserves terminal bytes, restores output settings, and uses the reviewed swash renderer. This change adds no package, dependency, startup change, generated command, API reference, or generation checkpoint. ### Tested Final candidate `279c986` includes the output-failure review correction and current main `6fcc478`. Cleanup cancellation no longer hides failed output while the caller remains active. Real request failures, caller cancellation, deadlines, and shutdown behavior remain intact. Twenty text/JSON diagnostic cases passed; fourteen failed before the correction. After the main update, thirteen focused race tests and two public process tests passed without skips. Three additional independent race tests passed, including API diagnostics and joined-error deduplication. Fresh hosted checks are running at this head. Earlier CI results below remain tied to their named revisions. Review found a retained recording defect. The correction preserves relay bytes and validates their SHA256 digest. The corrected validator rejects corrupted recordings that the old validator accepted. Three native probes passed for exact relay bytes and restoration after normal close, constructor failure, and close failure. Independent review verified unchanged public behavior against main `d32b3ae`. Twelve navigation race tests passed, with 24 subcases and no skips. Runtime tests ran on `75a499f`; the final recorder-only follow-up leaves all Go sources unchanged. Two independent helper tests also passed, including three resource subcases. Thirty-one real-command scenarios preserved main's exact stdout, stderr, status, request count, and cursors. These include all project aliases, model isolation, explicit output modes, zero/unlimited limits, and partial failures. The current-main custom-code check passed at 21/1,000 lines; this draft adds no generated customization. Native macOS terminal parity passed: seven scenarios and 21 CLI invocations across main and both final heads. Terminal selection passed all 26 test/subtest checks without skips. Linux and Windows native behavior remain unverified. [CI](https://github.com/openai/openai-cli/actions/runs/37755680281) passed at `b2d0924`; conditional jobs were skipped. Public activation remains a separate dependency. ### Demo This prerequisite has no visible public-command change to demonstrate. The checked-in recorder is for future generated activation and is not proof of activation at this head. [Activation recording recipe](https://github.com/openai/openai-cli/blob/b2d0924f451ce09a113ef1ab699f37c4375a62e0/scripts/demos/list-navigation/README.md).
File, batch, and project lists need a compact view of each loaded API page. This change adds shared table rendering on top of the handwritten navigation prerequisite. Public API commands remain unchanged until generated page adapters are promoted through the normal compiler workflow. ### What changes - Select ID, filename, purpose, size, and status for files. - Select ID and status for batches; select ID, name, and status for projects. - Preserve complete IDs and shorten secondary values when space is limited. - Use existing readable labels when a page has unfamiliar fields, actionable details, or insufficient width. - Keep page fetching and keyboard input outside the table renderer. - Propagate request cancellation through projection and nested count validation without returning partial output. - Leave model-specific display, selection, and navigation to the separate Models change. The shared one-screen and tab-aware completion follow-ups are being added to [openai#391](openai#391). Combined activation must also apply the separate quiet policy to optional summary hints while preserving records and navigation controls. This PR prepares a dependency; it does not complete SDK-1178 or SDK-1179. ### Commands No new commands or flags. These public commands retain current-main behavior at this staged head: ```sh openai files list openai batches list openai admin projects list openai projects list openai --format text files list openai --format json files list ``` The table hook applies only when the future generated adapters select automatic terminal navigation. Explicit formats, extraction, raw output, and redirected output retain their existing interfaces. ### Code `pkg/transformers/project_list_table.go` selects verified fields from one loaded page. `internal/readable/list_table.go` renders aligned columns using the existing Lip Gloss dependency and terminal escaping. `pkg/custom/list_table.go` connects that projection to the navigation renderer. The generic renderer adds no requests and changes no original records. This PR targets main after the navigation prerequisite merged in [openai#389](openai#389). It adds no package, dependency, startup change, generated command, API reference, or generation checkpoint. The models-only adapter, dispatch, projection, and demo scenes remain outside this change. ### Tested Current candidate `ff042a3` rebases the same two table commits onto merged Navigation `d866707`. Range-diff confirms both table changes remain identical. The updated base retains Navigation's output-failure correction and current help behavior. Thirty focused race tests produced 128 test/subtest passes, with no skips or failures. Two public process tests produced 44 test/subtest passes. Three independent cancellation race tests and ten subcases passed on this exact base. The trusted-main budget passed at 21/1,000 lines. Fresh hosted checks are pending for this updated head; earlier evidence below retains its original revision. Independent review reproduced cancellation failures in resource projection and nested request-count validation. The correction passes the caller context into existing summary checks and propagates their errors before fallback. Three resource regressions and four independent phase probes fail before the fix and pass afterward. For `203d7aa`, 29 focused race tests passed, with 107 test/subtest passes and no skips. The independent race overlay passed three top-level tests and ten subcases, including a 9 MiB synthetic value. Thirty-one public scenarios matched exact output, status, requests, and cursors across main, prerequisite, and Tables: 93 CLI invocations. All 32 packages compiled; eight contain no tests. The trusted current-main budget passed at 21/1,000 lines, with no added generated customization. Earlier native macOS checks at `69679c1` passed 21 command comparisons and 26 terminal-selection checks. This correction changes no terminal selection or public activation. Individual GJSON operations and terminal rendering remain synchronous between cancellation checks. Native Linux and Windows cancellation behavior remains unverified. [CI](https://github.com/openai/openai-cli/actions/runs/37765378284) passed at `203d7aa`; conditional queue jobs were skipped. These checks do not claim public table activation. ### Demo This staged head has no visible public-command change to demonstrate. Earlier local activated previews remain separate integration evidence with their original source identities. The three-resource recorder requires a separately activated candidate after normal generated promotion. [Activation recording recipe](https://github.com/openai/openai-cli/blob/203d7aa72b8f2421802fe0d661640f7824cdb99f/scripts/demos/list-tables/README.md).
2 participants
Large model lists currently fill the terminal with metadata.
This shows each model's exact ID and owner, with clear navigation for large results.
Slow interactive requests show loading feedback instead of an unexplained blank screen.
What changes
(unknown).--filterand--sort-bycontrols.Commands
models listremains the same command.Only
--filterand--sort-byare new flags.Filters support exact
id=VALUEand RE2id~REGEXmatching.Matches are case-sensitive unless the regex changes that behavior.
Boolean expressions and operand lists are unsupported.
Quote the whole expression for your shell.
Operand quotes and backslash escapes are decoded before matching.
Selection sorts before
--max-items; omitted selection retains response-order limiting before terminal display sorting.The limit defaults to unlimited;
-1remains supported and0returns no items.Selection rejects
--format rawbefore HTTP.Raw output without selection retains the full response envelope.
Code
This PR targets main
c65e03e, after Navigation #389 and Tables #390 merged.It preserves the reviewed Navigation completion and terminal-boundary corrections.
Models uses the SDK's single-response pager; navigating makes no additional API request.
pkg/customowns command options, loading feedback, presentation, and the Models viewer adapter.pkg/transformersselects complete records and projects exact ID/owner pairs.Shared readable tables and Bubble Tea/Bubbles handle layout and navigation.
The existing delayed-feedback helper supplies loading feedback.
Two presentation hooks stop and join its worker before results write.
Generated sources, module dependencies, entrypoint wiring, and payload limits remain unchanged.
The one-line
.go-versionpin selects Go 1.26.9 while the Actions version index catches up.This security patch passes the existing vulnerability scan. Future Go patches require updating the pin.
Universal Output composition still needs its shared policy for the optional Details hint.
The local follow-up patch remains separate; this branch does not add universal
--quiet.Images #394 changes the shared loading-helper signature.
Combining these PRs requires the separate verified Models caller adaptation.
That integration patch remains local until the revised Image helper is available in the base.
Tested
Candidate
996def6adds the Go 1.26.9 security pin above reviewed runtime70282f3.All five reported review findings are fixed, with regression coverage and resolved threads.
Standard-record printing retains complete fields; Models viewer Ctrl+C returns 130; q, Escape, and p retain status 0.
At
70282f3, 46 focused race tests and 206 subcases passed without failures or skips.Independent native macOS checks reproduced both original defects and verified the fixes, terminal restoration, and cleanup.
Additional probes preserve joined errors, writer failures, and existing typed exit-status precedence.
The earlier
26e23a3native checks cover narrow IDs, selection, API errors, and the corrected single-response checker.Normal and optimized Python checks verify key delivery and failed-write behavior.
On Go 1.26.9, the unchanged vulnerability scan passed all nine release targets.
Four focused Models race tests and seven subcases also passed, with no failures or skips.
The exact native build passes. Module dependencies and counted budget inputs remain unchanged.
CI passed for
996def6: 21 checks passed, with two conditional skips.The build-artifact scan used Go 1.26.9 and found no vulnerabilities across all nine release targets.
CI run and the trusted budget/isolation checks passed.
Merge still uses the required normal merge queue.
Earlier request-loading evidence remains tied to runtime
f739395and its 25 public scenarios.The demos remain tied to
d4a2d744; their table, navigation, and loading paths use q and remain unchanged.Fresh native checks cover the corrected p and Ctrl+C behavior separately.
Justin's original hang remains unreproduced; controlled wait handling does not establish its historical cause.
Broader generated-list activation remains separate. No live API calls ran.
Native Linux/Windows Models behavior remains unverified.
Demo
The recordings compare ID-only
d16c92bwith ID/ownerd4a2d744using 48 identical synthetic records.Both use
openai models liston macOS arm64 with Menlo terminal replay.The wide recording includes a controlled slow response and visible loading feedback.
The narrow recording preserves complete ID/owner labels.
All five commands exited successfully after one identical-response request each.
The recorder checks navigation, terminal restoration, capture bytes, and source/binary hashes.
Author and independent reviews passed all 11 screenshots and 46 GIF frames.
The media shows terminal replay rather than native graphical terminal appearance.
Before:
After:
Narrow comparison.
Recording recipe.