Skip to content

fix: reuse cached operation metadata - #44

Open
chaim0m wants to merge 4 commits into
mainfrom
codex/dci-operation-metadata-cache
Open

fix: reuse cached operation metadata#44
chaim0m wants to merge 4 commits into
mainfrom
codex/dci-operation-metadata-cache

Conversation

@chaim0m

@chaim0m chaim0m commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • replace the catalog's direct OpenAPI fetch with restish loading
  • reuse restish's disk cache by preserving the empty metadata-root version expected by HTTP-discovered cache entries
  • remove redundant in-process metadata state
  • cover the cache-version invariant that previously forced a permanent miss

Why

Follow-up to Alfredo's review comment on #34. The catalog bypassed restish's cache and transport. An initial implementation then set a root version that did not match HTTP-discovered cache entries; this revision keeps the established cache contract.

Test methods

go test ./...
go vet ./...
go build -o /tmp/dci-pr44-cache-test .
DCI_NO_UPDATE_CHECK=1 /tmp/dci-pr44-cache-test commands --json
DCI_NO_UPDATE_CHECK=1 HTTPS_PROXY=http://127.0.0.1:9 HTTP_PROXY=http://127.0.0.1:9 ALL_PROXY=http://127.0.0.1:9 /tmp/dci-pr44-cache-test commands --json

Both warm-cache runs returned catalog version 1 with 175 commands; the second succeeded with every proxy directed to an unreachable local port, verifying no network fallback.

Could this break things?

Risk: low-medium. Catalog loading now follows the same cached discovery and transport path as other DCI operation metadata. The catalog and destructive guard run in separate command paths and do not rely on shared process state.

Jira

CMP-48646

@chaim0m
chaim0m requested a review from apgiorgi as a code owner August 3, 2026 17:17
@chaim0m
chaim0m requested a review from taltultc August 3, 2026 17:17
@chaim0m chaim0m self-assigned this Aug 4, 2026

@apgiorgi apgiorgi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Routing through cli.Load instead of the bespoke http.DefaultClient fetch is the right idea. But one added line defeats the cache this PR is named after.

1. metadataRoot.Version = cli.Root.Version guarantees a permanent cache miss (destructive_contract.go:132-135).

Restish only assigns RestishVersion on the spec_files branch (cli/api.go:191); the HTTP-discovery branch caches without it (cli/api.go:272). Our generated apis.json has no spec_files (main.go:130-148), so the cache is always written with restish_version: "" — confirmed on disk, ~/Library/Caches/dci/dci.cbor begins a6 6f restish_version 60, where 0x60 is the empty string. The hit test is cached.RestishVersion == root.Version (cli/api.go:137).

Before this PR, ensureDestructiveOperations passed a bare &cobra.Command{} with Version "" and hit the cache. After it, every call misses. Net effect: dci delete-budget X and dci commands re-fetch the spec over the network on every invocation and fail offline — precisely the regression the PR body says it fixes. Fix is to pass &cobra.Command{} with no Version set.

2. Consequent auth regression. The forced cold path goes through cli.Load to MakeRequest, which applies the OAuth handler (cli/request.go:238-242). The code this replaces fetched the public /openapi.yaml unauthenticated. So dci commands — the agent-facing catalog — now always requires a valid token or triggers the browser flow. The smoke test in the PR body ran with a warm token, which is why this didn't show up.

3. The stated benefit never materializes. enforceDestructiveConfirmation is wired only on dciCmd.PersistentPreRunE (main.go:1366,1446), while commands is registered on cli.Root (command_catalog.go:76). The two paths can't run in the same process, so there's nothing to share — and ensureDestructiveOperations already memoizes via destructiveCommandSet. The new global plus reset hook is dead state, and command_catalog_test.go:94-119 only asserts that a pre-seeded global is returned, which is tautological: it exercises neither the load path nor the version behavior that actually broke.

Nits

  • Prefix should be chore: per AGENTS.md — internal refactor, no user-facing bug.
  • Loses command.Context() cancellation on the spec fetch.
  • The len(...Operations) > 0 memo guard re-loads forever if the spec legitimately has zero operations.
  • Branch is 4 commits behind origin/main (merge base 32859b5) — needs a rebase.

Worth noting all checks pass green, so none of the above is caught by the suite. Please add a test that asserts a warm cache is actually reused, and verify offline behavior by hand.

@chaim0m
chaim0m requested a review from a team as a code owner August 4, 2026 12:11
@chaim0m
chaim0m requested a review from apgiorgi August 4, 2026 12:16
@chaim0m

chaim0m commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the cache regression: metadata loading now passes an unversioned root, redundant in-process state was removed, and the regression test asserts the disk-cache version invariant. A manual run returned all 175 commands again with HTTP/HTTPS/ALL proxies pointed at an unreachable local port, confirming the warm cache needs no network. cli.Load exposes no context-aware API, so cancellation remains an upstream-loader limitation rather than being reimplemented here. A zero-operation result remains intentionally uncached because this code treats it as unavailable/error, not valid metadata. go test ./... and go vet ./... pass.

@chaim0m

chaim0m commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

All cache requests remain addressed after merging current main: the metadata root is unversioned, redundant process state is gone, and the offline warm-cache check still returns all 175 commands with every proxy pointed at an unreachable address. go test ./... and go vet ./... pass. Re-requesting review on the current head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants