Repository navigation
Conversation
Named listings discover tools on an uncached connection that closes before getInstructions runs, so instructions were only shown with --no-oauth. Keep a lastInstructions snapshot next to lastConnectionInfo, recorded under the same generation/abort guard and cleared on blank instructions or supersedeDefinition. Fixes openclaw#455
|
🦞👀 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. |
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: needs maintainer review before merge. What this changesRetains server instructions after uncached discovery so named listings include them in text and JSON output. Example: A stdio server returns instructions HELLO-INSTRUCTIONS during initialization.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review This PR fixes a documented behavior still broken on current main; no actionable correctness or security defect was found. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe CLI discovers tools through the runtime connection cache, then retrieves initialization instructions to render the named-server listing. flowchart TD
A[Named list command] --> B[Runtime tool discovery]
B --> C[MCP server initialization]
C --> D[Guarded instruction snapshot]
B --> E[Close uncached connection]
E --> F[Read server instructions]
D --> F
F --> G[Text or JSON output]
Technical reviewBest possible solution: Retain initialization instructions through the existing connection-cache owner while preserving OAuth behavior and lifecycle invalidation. Do we have a high-confidence way to reproduce the issue? Current-main source establishes the failure path, and Strajk's minimal server plus the contributor's before/after CLI transcript provide a concrete reproduction; this review executed no target code. Is this the best way to solve the issue? The snapshot repairs the shared runtime owner without reconnecting, extending connection lifetime, or changing OAuth policy. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5362d60d2e17. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 20, 2026, 3:59 AM ET / 07:59 UTC. |
steipete
left a comment
There was a problem hiding this comment.
Reviewed the runtime lifecycle fix and independently reproduced the regression. The focused repair, strengthened coverage, full remote code checks, and exact-head CI are ready.
Restore server instructions in ordinary named listings, including text and JSON output, without requiring
--no-oauth.Uncached discovery closes its connection before the CLI requests instructions. Retain a normalized snapshot in the runtime connection cache under its existing generation/abort guard, while preserving active-connection precedence and clearing the snapshot when instructions are absent or a definition is replaced. OAuth policy, connection cleanup, and all-server summaries keep their existing behavior.
Fixes #455. Restores the contract from #76 and preserves the lifecycle safeguards from #383. Thanks @KrasimirKralev for the fix and real CLI regressions, and @Strajk for the reproduction.
Validation: reproduced the five new failing regression cases on main, covering legacy and modern servers over stdio and HTTP plus the cache snapshot. Added coverage for cancelled setup with nonempty instructions and subsequent absent instructions. Independent Codex review found no actionable P0–P2 findings. Full remote checks and exact-head CI are required before merge.