Repository navigation
refactor(codex): share node catalog record lookup - #141235
Merged
Merged
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
steipete
marked this pull request as ready for review
September 7, 2026 13:12
Contributor
|
Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 9:18 AM ET / 13:18 UTC. ClawSweeper reviewWhat this changesShares the paired-node Codex session lookup between continuation and terminal opening while preserving their separate eligibility checks and error messages. Merge readiness✅ Ready for maintainer review The consolidation remains useful: inspected main still contains both lookup implementations. No introduced correctness or security defect was found, and this maintainer-authored PR is protected from automatic closure. Priority: P3 Review scores
Verification
How this fits togetherThe Codex plugin retrieves session records from paired computers before continuing a session or preparing a terminal launch. The shared lookup validates paginated responses and returns a matching record for each operation’s eligibility checks. flowchart TD
A[Continue session or open terminal] --> B[Shared bounded lookup]
B --> C[Paired node catalog]
C --> D[Validate page and find session]
D -->|Next page| B
D -->|Found| E[Operation eligibility checks]
D -->|Missing or cursor cycle| F[Operation error]
E --> G[Continue session or terminal plan]
Before mergeNone. Agent review detailsSecurityNone. PR surfaceSource -28. Total -28 across 4 files. View PR surface stats
Review metricsNone. Technical reviewBest possible solution: Keep one private bounded lookup while leaving continuation and terminal eligibility policy with their respective callers. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR addresses duplicated implementation, which is directly visible in inspected main, rather than a reported runtime defect. Is this the best way to solve the issue? Yes. The private helper removes duplicate traversal without merging the callers’ distinct policy or expanding the public plugin API. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 1b90029dbd75. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Contributor
Author
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Continuing a Codex session on a paired node and opening its terminal maintain separate copies of the same bounded catalog lookup. Changes to RPC arguments, response parsing or cursor handling currently have to remain synchronized across both copies.
Why This Change Was Made
One private plugin helper now owns the sequential request, real page parser, first matching record and bounded cursor traversal. Both old lookup functions are removed, along with a private parser parameter whose sole production caller always supplied the canonical parser. Continuation and terminal operations retain their distinct eligibility checks and error messages.
User Impact
Behavior is unchanged. Lookup keeps the same node command, scopes, 100-item pages, 100-page maximum, 65-second request timeout, cursor-cycle handling and first-match ordering. Active interactive sessions remain eligible for terminal opening; continuation keeps its existing state restrictions. The maintainer-requested consolidation removes 27 production code lines (28 physical lines), with no test changes or new public API.
Evidence
All 40 existing node-listing and continuation tests pass before and after. A further 666 comparisons use the real helper, parser and response unwrapping to verify both callers across direct, payload and payloadJSON responses, including page 100, malformed or archived rows, cycle/match ordering and exact error causes. All 24 selected checks and independent P2 review pass.
The relevant upstream Codex listing request/response contract was inspected directly. Verification here uses synthetic transport plus existing provider-boundary tests; it does not claim a live Gateway, paired-node or Codex request. No runtime dependency or package-export change is introduced.