[codex] redesign MCP contracts and recovery - #514
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. |
![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: ccb56fabcd
ℹ️ 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".
|
Addressed all seven review findings in 2f42550:
Validation: Ruff format/check, strict mypy, and full pytest (395 passed, 7 skipped). The rescue destination remains agent-selected; descriptor paths are internal safety machinery, not a user-facing path requirement. |
|
Follow-up cleanup pushed in 7ce4403 and f5c298b. The Linux-only deterministic failure was an idempotent re-rescue regression after session rescue metadata was cleared; existing valid rescue repositories are now recognized through a descriptor-relative marker check. I also removed prescriptive agent workflow policy from server instructions, tool descriptions, request-field copy, and workload-failure guidance. The remaining restrictions are enforced correctness/safety invariants, not repo/workspace policy: explicit inputs, bounded execution, compatible contracts, non-overwriting publication, and immutable-evidence integrity. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 461138d2b3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 076b657d5b
ℹ️ 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".
| class McpPathSource(PathSource): | ||
| format: ArtifactFormat | None = Field( | ||
| default=None, | ||
| description="Explicit native artifact format. Omission requires unambiguous detection.", |
There was a problem hiding this comment.
When artifact.preview reads a directory or an extensionless file with format omitted, runtime detection records the source format as directory or unknown and emits that value in the exact next_page request. Neither value is in this closed enum, so any preview requiring a second page is rejected by MCP validation even though the first page succeeded. Keep the preview source format open enough to accept runtime-generated handoffs, or avoid serializing a format that the transport cannot read back.
AGENTS.md reference: AGENTS.md:L44-L48
Useful? React with 👍 / 👎.
| parent_descriptor = os.open(selected.parent, os.O_RDONLY) | ||
| try: | ||
| stage.rename(selected) | ||
| os.fsync(parent_descriptor) |
There was a problem hiding this comment.
On Windows, os.open(selected.parent, os.O_RDONLY) cannot open a directory for _commit/os.fsync, so every new rescue reaches this statement after staging the evidence and then returns REPOSITORY_IO_FAILURE instead of publishing it. The former Windows preflight rejection was removed and the interface now presents rescue as ordinary filesystem publication, which also means a CLI capture can execute before discovering this failure; use a Windows-compatible directory durability path or continue rejecting the operation before execution.
AGENTS.md reference: AGENTS.md:L48-L48
Useful? React with 👍 / 👎.
1 participant
Problem
The MCP catalog embedded every capability-specific analysis and capture model, then recursively embedded the analysis request again in continuation outputs. That made
tools/listlarge and ambiguous while malformed nested arguments escaped into raw validation diagnostics. Capture-complete failures were also hidden underdetails.partial_evidence, recovery metadata used provider-specific prose keys, repository queries could not distinguish an absent inventory from an empty or unmatched one, and captured analysis sources were reconstructed from path and digest metadata during publication.For agents, these behaviors made tool selection expensive, recovery difficult to compose, and failed workload evidence easy to mistake for a failed MCP call. Path reconstruction also broke preservation when equivalent filesystem aliases referred to the same captured source.
Root cause
The high-level SDK registration surface owned too much of the public schema. Capability matrices were projected into recursive unions during catalog construction, while transport validation and domain recovery were split across SDK behavior, runtime exceptions, descriptions, and provider-specific detail dictionaries. Repository publication did not retain the relational identity already known during capture.
Changes
Serverand a declarative, typedToolContractregistry;inspect_capabilitieslist/get discovery;analyzeandcapture_and_analyzeenvelopes backed by registry validation;CachedAnalysisand repository publication instead of rematching path strings;A fresh stdio client reports seven tools and a 94,447-byte rendered catalog, down from the previous 275,296-byte baseline (about 65.7%). Invalid discovery calls now return
INVALID_REQUESTwithfield_path: ["mode"]and accepted valueslistandget.Validation
uv run ruff check src tests toolsuv run mypy src tests toolsuv run --extra dev pytest -q— 398 passed, 1 skipped, 206 deselectedThe skipped test requires the optional Memray provider. The targeted process tests cover the changed preservation, capture-partial, collector-failure, and real-stdio paths.
Closes #500
Closes #501
Closes #502
Closes #503
Closes #504
Closes #505
Closes #506
Closes #507
Closes #508
Closes #509
Closes #510
Closes #511
Closes #512
Closes #513