Repository navigation
Add cache fields to directory read and _meta to Skill - #3719
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. |
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
![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: 42eff6c0f3
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I also checked the _meta re-validation path in _handle_get/_handle_list (model_dump() without by_alias emits meta, which _SkillModel's populate_by_name=True accepts, so the field survives), and the client parse of ReadDirectoryResult on a legacy wire — ttl_ms/cache_scope both default, so the stripped payload still validates.
Extended reasoning...
The PR adds an optional _meta field to Skill and makes ReadDirectoryResult inherit CacheableResult, routing resources/directory/read through the existing _finalize_cacheable version gate; it mirrors the pattern already used by ListSkillsResult and the Field(alias="_meta") convention in mcp_types. No security-sensitive surface is touched. Not approving because verified findings remain unposted and the inline test-quality nit stands.
| skill = _git_workflow_skill().model_copy(update={"meta": {"example.com/owner": "platform"}}) | ||
| return ListSkillsResult(skills=[skill]) | ||
|
|
||
| server = MCPServer("catalog", extensions=[Skills(list_skills=with_meta, get_skill=_get_skill)]) | ||
| async with Client(server, mode="2026-07-28") as client: | ||
| raw = await client.session.send_request(ListSkillsRequest(), _RawResult) | ||
| extra = raw.model_extra or {} |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get a _meta wire test that compares two hand-written literals instead of the value the handler sent. In tests/server/test_skills.py:186 the handler builds {"example.com/owner": "platform"} and tests/server/test_skills.py:193 asserts against a second copy of that literal. The repo's test-quality bar says pass-through values such as _meta are asserted against the same variable that was sent. Fix: bind the dict to a module- or test-level variable, pass that to model_copy, and assert extra["skills"][0]["_meta"] == meta against it. [also at: tests/server/test_skills.py:189 - nit: the _meta wire-key test compares the pass-through value to a retyped literal, so a fixer gets a test that only proves two literals match. tests/server/test_skills.py:184 builds the meta dict inline and tests/server/test_skills.py:189 asserts against a second copy of it.; tests/server/test_skills.py:191 - nit: maintainers get a pass-through _meta assertion that compares two hand-typed literals, which the repo's test-quality bar says proves nothing. tests/server/test_skills.py:184 builds the dict inline in the handler and tests/server/test_skills.py:191 re-types it in the assert.]
Why this was flagged
AGENTS.md line 68 delegates test review to .claude/skills/test-quality/SKILL.md, whose Assertions section says pass-through values (_meta named explicitly) are compared against the same variable you sent, because two literals only match because a human eyeballed them. tests/server/test_skills.py:186 builds the meta literal inside with_meta, and tests/server/test_skills.py:193 repeats the literal in the assertion. Nothing fails at runtime; the test still proves the _meta key name on the wire. The sibling client test at tests/client/extensions/test_skills.py:193 already uses a meta variable, so this is the one site in the diff that departs from the convention.
Verification: nit. AGENTS.md line 68 delegates tests to .claude/skills/test-quality/SKILL.md, whose lines 56-58 require pass-through values such as _meta be asserted against the same variable you sent. tests/server/test_skills.py line 186 builds the value inline and line 193 compares against a second hand-typed literal. Nothing fails at runtime, so this is a convention-only violation.
1 participant
Closes #3718.
Aligns the Skills extension with ext-skills#157:
ReadDirectoryResultnow extendsCacheableResult, andresources/directory/readgoes through the same protocol-version gate asskills/list/skills/get:ttlMs/cacheScopeare sent on2026-07-28+ and stripped on older connections.Skillgains an optionalmetafield, serialized as_meta, so it survives list/get instead of being dropped.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.