feat(connector): warm action schema cache from search results#289
Merged
Conversation
The /v1/actions/search endpoint now returns `inputSchema` and `outputSchema` for every match, so search can seed the local action schema cache that `oo connector schema` and `oo connector run` read. A follow-up schema lookup for a returned action is then answered locally instead of issuing a fresh metadata request. Warming is best-effort: results without schema payloads are skipped and cache write failures only log a warning. Search payloads never carry an `asyncLifecycle`, so a search-seeded entry must not be trusted by the `--wait` / `--wait-result` run modes: they would misreport async actions as unsupported. The loader gains a `requireAsyncLifecycle` option that treats lifecycle-less cache entries as misses and falls back to the metadata API once, re-caching the full metadata. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
Summary by CodeRabbit
WalkthroughThis pull request adds schema-cache warming for connector search results, introduces a Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/application/commands/connector/schema-cache.ts (1)
84-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated log-context object.
The same
{ accountId, actionName, endpoint, serviceName }literal is now built three times in this function (Lines 85-90, 97-102, 120-125). Compute it once and reuse across thelogger.debugcalls.As per coding guidelines, "Compute an expression once, store in a variable, and reuse."
♻️ Proposed refactor
const cache = openConnectorActionSchemaCache(context); const cacheKey = createConnectorActionSchemaCacheKey({ accountId: options.account.id, actionName: options.actionName, endpoint: options.account.endpoint, serviceName: options.serviceName, }); + const logContext = { + accountId: options.account.id, + actionName: options.actionName, + endpoint: options.account.endpoint, + serviceName: options.serviceName, + }; if (options.refresh !== true) { const cached = tryReadConnectorActionSchemaCache(cache, cacheKey, context); if (cached !== undefined) { if (options.requireAsyncLifecycle !== true || cached.asyncLifecycle !== undefined) { return cached; } context.logger.debug( - { - accountId: options.account.id, - actionName: options.actionName, - endpoint: options.account.endpoint, - serviceName: options.serviceName, - }, + logContext, "Connector action schema cache entry lacks an async lifecycle; fetching metadata.", ); } } else { context.logger.debug( - { - accountId: options.account.id, - actionName: options.actionName, - endpoint: options.account.endpoint, - serviceName: options.serviceName, - }, + logContext, "Connector action schema cache bypassed for refresh.", ); }Also applies to: 119-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/connector/schema-cache.ts` around lines 84 - 104, The connector schema cache logic in the function that emits the repeated logger.debug calls is rebuilding the same log-context object multiple times. Extract the shared { accountId, actionName, endpoint, serviceName } payload into a single local variable in this function and reuse it for each debug invocation, including the branches that log the async lifecycle fetch and the bypass message, so the repeated context construction is centralized and consistent.Source: Coding guidelines
src/application/commands/connector/index.cli.test.ts (1)
2628-2637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated fetcher setup into a shared factory.
The GET-confirmation fetcher (capture request, return fixture on GET) is duplicated almost verbatim between the
--waitand--wait-resulttests.♻️ Suggested factory extraction
+function createMetadataConfirmationFetcher(requests: Request[]) { + return async (input: RequestInfo | URL, init?: RequestInit) => { + const request = toRequest(input, init); + requests.push(request); + if (request.method === "GET") { + return new Response(JSON.stringify({ + data: createConnectorActionFixture(), + })); + } + return new Response(JSON.stringify({ + data: { + // ...existing non-GET payload + }, + })); + }; +}Then reuse
fetcher: createMetadataConfirmationFetcher(requests)in both tests.As per path instructions, "In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file."
Also applies to: 2738-2747
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/connector/index.cli.test.ts` around lines 2628 - 2637, The GET-confirmation fetcher setup is duplicated across the --wait and --wait-result tests, so extract it into a shared local factory in the test file and reuse it in both cases. Create a helper like createMetadataConfirmationFetcher(requests) that captures requests and returns the fixture on GET, then pass fetcher: createMetadataConfirmationFetcher(requests) in each test to keep the setup consistent and reduce duplication.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/commands.md`:
- Around line 570-572: Update the cache note in the search command docs so it
only describes schema-bearing search results, since the implementation in the
relevant search flow only warms cache entries when both inputSchema and
outputSchema are present and writes are best-effort. Adjust the wording near the
search documentation sections so it does not imply every search result seeds the
cache, and keep the description aligned with the behavior in the search/cache
path.
---
Nitpick comments:
In `@src/application/commands/connector/index.cli.test.ts`:
- Around line 2628-2637: The GET-confirmation fetcher setup is duplicated across
the --wait and --wait-result tests, so extract it into a shared local factory in
the test file and reuse it in both cases. Create a helper like
createMetadataConfirmationFetcher(requests) that captures requests and returns
the fixture on GET, then pass fetcher:
createMetadataConfirmationFetcher(requests) in each test to keep the setup
consistent and reduce duplication.
In `@src/application/commands/connector/schema-cache.ts`:
- Around line 84-104: The connector schema cache logic in the function that
emits the repeated logger.debug calls is rebuilding the same log-context object
multiple times. Extract the shared { accountId, actionName, endpoint,
serviceName } payload into a single local variable in this function and reuse it
for each debug invocation, including the branches that log the async lifecycle
fetch and the bypass message, so the repeated context construction is
centralized and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc1275d2-c213-4415-939b-6d11a034cb6b
⛔ Files ignored due to path filters (1)
src/application/commands/connector/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
__tests__/helpers.tsdocs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/connector/index.cli.test.tssrc/application/commands/connector/run.tssrc/application/commands/connector/schema-cache.test.tssrc/application/commands/connector/schema-cache.tssrc/application/commands/connector/search-provider.test.tssrc/application/commands/connector/search-provider.tssrc/application/commands/connector/shared.test.tssrc/application/commands/connector/shared.tssrc/application/commands/search.cli.test.ts
Cache warming only happens when the search response carries schema data, so the unconditional wording overstated the behavior. Phrase the note as conditional without leaking wire-level field names into the user-facing CLI contract docs. Signed-off-by: Kevin Cui <bh@bugs.cc>
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.
Since
/v1/actions/search(#288) returnsinputSchemaandoutputSchemafor every match, connector search now seeds the local action schema cache with those payloads. A follow-upoo connector schema(or the input validation inoo connector run) for a returned action is then answered locally instead of issuing a fresh metadata request. Warming is best-effort: results without schema payloads are skipped, cache write failures only log a warning, and the search output contract is unchanged. This effectively restores the search-time warming removed in #193 — safe again now that search is served by the connector service itself rather than a separate index that could return stale schemas.One correctness guard comes with it: search payloads never carry an
asyncLifecycle, so the--wait/--wait-resultrun modes must not trust a search-seeded cache entry — they would misreport async actions as unsupported.loadConnectorActionSchemagains arequireAsyncLifecycleoption that treats lifecycle-less cache entries as misses and falls back to the metadata API once, re-caching the full metadata. The observable side effect is that rejecting--waiton a genuinely non-async action now performs one metadata request before erroring.Docs for
oo search,oo connector search, andoo connector schemanote the new caching behavior (en + zh-CN).