fix: reuse cached operation metadata - #44
Conversation
apgiorgi
left a comment
There was a problem hiding this comment.
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) > 0memo guard re-loads forever if the spec legitimately has zero operations. - Branch is 4 commits behind
origin/main(merge base32859b5) — 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.
|
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. |
|
All cache requests remain addressed after merging current |
Summary
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
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