[codex] route schema refresh through daemon - #52
Merged
Merged
Conversation
Owner
Author
Review swarm findingsPinned diff: PR #52 at head Lanes: correctness, adversarial/regression, quality/maintainability, security, backend/domain. Findings needing disposition:
Clean lanes: quality/maintainability, security. Validation observed by review worker: read-only inspection, |
Owner
Author
Review swarm fixesCommit pushed: Addressed finding:
Validation run after fix:
No deferrals. |
Owner
Author
Review swarm fix verificationFix-only diff: Verification lane: SME/adversarial scoped to daemon cache-bypass behavior. Result: VERIFIED. |
rodaddy
marked this pull request as ready for review
June 18, 2026 03:54
This was referenced Jun 28, 2026
rodaddy
added a commit
that referenced
this pull request
Jun 28, 2026
* fix(cache): invalidate schema cache on contract change (#58) mcp2cli's per-credential/per-service schema cache only expired on a 24h TTL, so after an upstream contract bump (e.g. Open Brain memory-tools.v11 adding append_session_event.create_if_missing) `mcp2cli schema` and generate-skills served the OLD tool shape for up to a day, even though live tool calls already worked against the new contract. This lands the cache-coherence core (general, capability-tiered): - Schema fingerprint in cache metadata. writeCache now records a deterministic schemaFingerprint over the full tool surface (incl. inputSchema, so a new field like create_if_missing changes it). Accepts an authoritative contract hash when a server publishes one; otherwise derives it from the tool surface. New fingerprintSchemas() helper (order-independent, change-sensitive). - Invalidate ALL keys for a service. clearServiceCacheKeys() clears the bare entry AND every per-credential key (credential:<base64url([service,user])>), so a bump can't leave a credential-scoped read serving the old schema. - Daemon self-heal + invalidation. The drift-hook (already runs on connect via the open connection -- no self-call) now clears all cache keys for a service when it detects drift, then repopulates the base entry. - Client-side coherence via piggyback. The daemon stamps its current schemaFingerprint (per-service) onto /list-tools and /schema responses; the client compares it to its own cached fingerprint and drops its stale cache on mismatch -- closing the staleness on the client's ~/.cache layer with no extra round-trip. Cold/absent cache is left alone (refetches naturally). - cache warm --force: clear-then-refetch, the documented recovery after a bump. Deferred to a follow-up (noted on #58): the `cache warm` daemon self-call deadlock fix touches the #52 "route schema refresh through daemon" decision and warrants its own PR; the drift-hook repopulation here already routes around it. Verified: bun run typecheck clean; full suite 1069 pass / 0 fail, deterministic across repeated runs. New unit coverage for fingerprintSchemas, clearServiceCacheKeys (base + credential keys), reconcileClientCache (mismatch / match / cold / no-fingerprint / error), and cache warm --force. Refs #55 (nested-field schema discoverability -- worked together). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cache): align coherence fingerprint key + canonical hash surface (#58 review) Review-swarm blockers on PR #59 (all three reviewers converged): - B1 (key mismatch, false-positive invalidation): the daemon stamped the per-credential pool-key fingerprint while the client compared its bare-service fingerprint -- different files, never converge -> the client cleared its cache on every call after a warm. The daemon now stamps readCacheFingerprint( body.service) (bare) on /list-tools and /schema. The drift-hook already maintains the bare entry on every connect (checkDriftOnConnect runs with baseServiceName), so both sides reference the same, credential-independent key. - B2 (description-default divergence): the daemon hashed `description ?? ""` while the client warm path hashed the already-defaulted "(no description)", producing different fingerprints for the same tool. hashToolSchema now normalizes "" and "(no description)" to one canonical value, so every writer derives the same fingerprint (proven: daemon-style and client-style hashes are now identical; real descriptions stay distinct). - M1 (silent under-delete): clearServiceCacheKeys incremented its count even when unlink was swallowed, reporting success while a stale credential entry survived -- re-opening the staleness. It now counts only resolved unlinks and logs failures. - Added a real daemon/client fingerprint-convergence round-trip test that exercises both write surfaces; it would have caught B1/B2 (the prior tests hand-injected fingerprints and never compared two independently-derived ones). Verified: bun run typecheck clean; full suite 1070 pass / 0 fail, deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(cache): pin daemon bare-key fingerprint stamp (#58 B1 regression guard) The re-review noted the B1 fix (daemon stamps the bare-service fingerprint, not the credential pool key) had no regression guard -- a revert to the pool key would ship silently. This adds a daemon /list-tools test that wires a per-user credential so the pool key is genuinely a `credential:` key distinct from the bare key, seeds the two with different fingerprints, and asserts the response carries the BARE one. Mutation-verified: reverting the stamp to listPoolKey fails this test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Supports rodaddy/open-brain#122.
Summary
cache warm,cache diff, andgenerate-skillsthrough the daemon by defaultMCP2CLI_NO_DAEMONis setskills.test.tsharness so captured command exit codes do not leak into Bun's process exit codeValidation
bun test tests/cli/daemon-routed-discovery.test.tsbun test tests/cli/skills.test.tsbun test tests/cli/daemon-routed-discovery.test.ts tests/cli/generate-skills.test.ts tests/cache tests/generationbun run typecheck