feat(api): post search endpoint - #1969
Draft
akijakya wants to merge 7 commits into
Draft
Conversation
Signed-off-by: András Jáky <ajaky@cisco.com>
…utils Signed-off-by: András Jáky <ajaky@cisco.com>
Signed-off-by: András Jáky <ajaky@cisco.com>
Signed-off-by: András Jáky <ajaky@cisco.com>
Signed-off-by: András Jáky <ajaky@cisco.com>
Signed-off-by: András Jáky <ajaky@cisco.com>
…arch Signed-off-by: András Jáky <ajaky@cisco.com>
Contributor
|
The latest Buf updates on your PR. Results from workflow Buf CI / verify-proto (pull_request).
|
📝 WalkthroughWalkthroughThe PR adds local or remote OASF extractor configuration, shared backend resolution, gateway natural-language search through ChangesExtractor configuration and resolution
Natural-language search
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AIFinderController
participant Extractor
participant CatalogDatabase
Client->>AIFinderController: POST /v1/search
AIFinderController->>Extractor: Extract query
AIFinderController->>CatalogDatabase: Search ranked record CIDs
CatalogDatabase-->>AIFinderController: Return CIDs
AIFinderController->>CatalogDatabase: Hydrate catalog entries
AIFinderController-->>Client: Return ranked results
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utils/extractor/remote.go (1)
40-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a bounded deadline for remote extraction.
SearchAgentspasses the inbound context through toremoteExtractor.Extract, and the gRPC client has no per-RPC timeout. Apply a configuredcontext.WithTimeoutbefore extraction to prevent stalled requests from retaining gateway resources indefinitely.🤖 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 `@utils/extractor/remote.go` around lines 40 - 45, Update remoteExtractor.Extract to derive a context.WithTimeout using the configured extraction deadline before calling r.client.Extract, and defer cancellation. Pass the timed context to the RPC while preserving the existing request construction and error handling.
🧹 Nitpick comments (1)
utils/extractor/remote_test.go (1)
60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new
Tiersrequest field.This fake client verifies request construction. The test forwards
Versionsbut does not exerciseTiers. SetTiers: 2and assertfake.gotReq.GetTiers() == 2. Add a negative-tier case if zero-default behavior is contractual.🤖 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 `@utils/extractor/remote_test.go` around lines 60 - 65, Update the Extract call in the remote extractor test to set Tiers to 2, then assert fake.gotReq.GetTiers() equals 2 alongside the existing request-field assertions. Add a negative-tier case only if zero-default behavior is an established contract.
🤖 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 `@cli/cmd/daemon/config.go`:
- Around line 65-77: Require authenticated TLS or mTLS for non-loopback remote
extractor connections and validate the peer before use, replacing insecure
credentials in the extractor resolution flow. Persist the TLS/mTLS configuration
during cli/cmd/init/options.go and cli/cmd/init/run.go, register and default the
settings in cli/cmd/daemon/config.go, and reject unsecured endpoints in
cli/cmd/routing/nlsearch.go and cli/cmd/search/nlsearch.go while preserving
trusted loopback or encrypted-mesh behavior.
In `@cli/cmd/import/config.go`:
- Line 291: In the loadExtractorFromConfig function, preserve the RemoteAddr
value returned by clientconfig.LoadConfigured() and pass it to extractor.Load().
Either use clientconfig.ResolveConfigured() instead of LoadConfigured() to
directly handle remote address resolution, or extend the
extractorEnricherFileConfig struct to include a RemoteAddr field and populate it
from the loaded config before passing to extractor.Load(). Ensure the remote
extractor address configured during dirctl init is propagated through the
enricher initialization.
In `@cli/cmd/init/run.go`:
- Around line 237-245: Add a bounded context with a timeout before passing
cmd.Context() to the runWithSpinner call for the smokeCheckRemote operation. Use
context.WithTimeout to wrap cmd.Context() so that if the remote server does not
respond, the gRPC call fails fast and returns control to the warning path at
lines 240-241 instead of hanging indefinitely.
In `@client/config/extractor.go`:
- Around line 40-54: Update LoadConfigured to honor saved.RemoteAddr during
import enrichment instead of always calling local extractor.Load with only
OASFURL and AssetDir. Add or reuse a remote-capable extractor adapter when
RemoteAddr is configured, or explicitly return a clear unsupported-remote error;
preserve the existing local loading path for configurations without RemoteAddr.
In `@install/charts/dir/apiserver/Chart.yaml`:
- Around line 41-48: Update the oasf-sdk dependency in Chart.yaml to use the
documented oasfSdk.enabled key by adding the appropriate Helm dependency alias
and changing its condition. Align the related values configuration and
service-name templates with oasfSdk so enabling the documented key consistently
activates and configures the extractor subchart.
In `@install/charts/dir/apiserver/templates/_helpers.tpl`:
- Around line 214-219: Update the extractor fallback documentation in
install/charts/dir/apiserver/templates/_helpers.tpl lines 214-219 and
install/charts/dir/apiserver/values.yaml lines 463-466 to state that an empty
extractor.remote_addr selects the local extractor; clarify that /v1/search
returns 503 only when no usable extractor is configured.
In `@server/controller/ai_finder_search.go`:
- Around line 122-149: Update the pagination flow around GetRecordCIDs and
hydrateInCIDOrder so facet filtering occurs before finalizing a page. Prefer
passing facet constraints into the ranked CID query; otherwise fetch successive
ranked batches until the page is filled or results are exhausted, then generate
nextPageToken from the consumed results. Add coverage for pagination with a
selective facet, ensuring filtered-out CIDs do not produce an empty page with a
non-empty token.
- Around line 42-47: Update the SearchAgents logging around the normalized query
and its error path to stop emitting the complete natural-language value. Replace
query fields in both debug and error logs with a non-reversible summary such as
its length, request identifier, or keyed digest, while preserving the validation
behavior and log context.
In `@server/server.go`:
- Around line 511-518: Update the shutdown sequence around
s.grpcServer.GracefulStop and s.oasfExtractor.Close: call GracefulStop first so
in-flight SearchAgents requests drain, then close the OASF extractor and retain
its existing error logging.
- Around line 364-366: Update the server construction flow after
resolveGatewayExtractor so a gateway.New failure closes gwExtractor before
returning the error. Ensure cleanup occurs only until successful Server creation
transfers ownership, preventing both the failure path and Server.Close from
closing the extractor.
In `@utils/extractor/assets.go`:
- Around line 30-44: Harden Teardown so it only removes a verified extractor
asset directory, not any arbitrary absolute path. After Config.Resolve and
before os.RemoveAll, resolve the target’s symlinks and validate that it is under
an allowlisted asset root or matches persisted provisioning identity, and
require a valid extractor manifest in that directory. Update guardAssetDir or
the Teardown flow accordingly while preserving the existing refusal of empty,
root, and home paths.
In `@utils/extractor/provision_integration_test.go`:
- Around line 15-16: Update the run command comment in
provision_integration_test.go to execute from the utils module’s correct package
location and use the actual test function name, replacing the invalid
./internal/extractor/ path and TestProvisionSmokeCheck identifier.
---
Outside diff comments:
In `@utils/extractor/remote.go`:
- Around line 40-45: Update remoteExtractor.Extract to derive a
context.WithTimeout using the configured extraction deadline before calling
r.client.Extract, and defer cancellation. Pass the timed context to the RPC
while preserving the existing request construction and error handling.
---
Nitpick comments:
In `@utils/extractor/remote_test.go`:
- Around line 60-65: Update the Extract call in the remote extractor test to set
Tiers to 2, then assert fake.gotReq.GetTiers() equals 2 alongside the existing
request-field assertions. Add a negative-tier case only if zero-default behavior
is an established contract.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f21a1d41-4e2a-4085-81c6-126bd898d232
⛔ Files ignored due to path filters (7)
api/catalog/v1/ai_finder_service.pb.gois excluded by!**/*.pb.goapi/catalog/v1/ai_finder_service.pb.gw.gois excluded by!**/*.pb.gw.goapi/catalog/v1/ai_finder_service_grpc.pb.gois excluded by!**/*.pb.goinstall/charts/dir/apiserver/Chart.lockis excluded by!**/*.lockreconciler/go.sumis excluded by!**/*.sumserver/go.sumis excluded by!**/*.sumutils/go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
cli/cmd/daemon/config.gocli/cmd/import/config.gocli/cmd/init/options.gocli/cmd/init/run.gocli/cmd/init/run_test.gocli/cmd/routing/nlsearch.gocli/cmd/search/nlsearch.goclient/config/extractor.goclient/config/extractor_test.goclient/go.modinstall/charts/dir/apiserver/Chart.yamlinstall/charts/dir/apiserver/templates/_helpers.tplinstall/charts/dir/apiserver/templates/configmap.yamlinstall/charts/dir/apiserver/values.yamlinstall/charts/dir/values.yamlproto/agntcy/dir/catalog/v1/ai_finder_service.protoreconciler/go.modserver/config/config.goserver/config/config_test.goserver/controller/ai_finder.goserver/controller/ai_finder_filter.goserver/controller/ai_finder_search.goserver/controller/ai_finder_search_test.goserver/controller/ai_finder_test.goserver/go.modserver/server.gotests/e2e/local/testenv/kind/dir-chart-values.yamlutils/extractor/assets.goutils/extractor/assets_test.goutils/extractor/config.goutils/extractor/config_test.goutils/extractor/extractor.goutils/extractor/load_test.goutils/extractor/local.goutils/extractor/provision_integration_test.goutils/extractor/remote.goutils/extractor/remote_test.goutils/extractor/resolve.goutils/extractor/resolve_test.goutils/go.modutils/nlsearch/decompose.goutils/nlsearch/decompose_test.go
💤 Files with no reviewable changes (1)
- utils/extractor/resolve.go
Comment on lines
+65
to
+77
| // Extractor config has no defaults and is absent from daemon.config.yaml, so | ||
| // register it here for AutomaticEnv (mirrors server/config). Empty remote_addr | ||
| // falls back to the locally-provisioned in-process extractor; set it (e.g. | ||
| // DIRECTORY_DAEMON_SERVER_EXTRACTOR_REMOTE_ADDR=localhost:31234) to point the | ||
| // node's POST /v1/search at a running OASF-SDK server instead. | ||
| _ = v.BindEnv("server.extractor.remote_addr") | ||
| v.SetDefault("server.extractor.remote_addr", "") | ||
|
|
||
| _ = v.BindEnv("server.extractor.asset_dir") | ||
| v.SetDefault("server.extractor.asset_dir", "") | ||
|
|
||
| _ = v.BindEnv("server.extractor.oasf_url") | ||
| v.SetDefault("server.extractor.oasf_url", "") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Map the resolver and gateway search implementation before inspecting the path.
ast-grep outline utils/extractor/resolve.go --items all --type function
ast-grep outline server/controller/ai_finder_search.go --items all --type function
# Verify query source -> resolver -> RPC sink.
rg -n -C 8 --type go \
'RemoteAddr|ResolveExtractor|grpc\.NewClient|insecure\.NewCredentials|\.Extract\(' \
utils/extractor/resolve.go server cli
# Verify whether TLS or mTLS controls exist across configuration and Helm paths.
rg -n -C 8 -g '*.go' -g '*.yaml' -g '*.yml' -g '*.tpl' \
'remote_addr|tls|mtls|server_name|ca_file|client_cert|client_key|TransportCredentials' \
utils server cli install/charts/dirRepository: agntcy/dir
Length of output: 50366
Security Misconfiguration (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External · Exploitability: Moderate
Require authenticated encryption for remote extractor connections.
When RemoteAddr is outside trusted loopback or an encrypted mesh, utils/extractor/resolve.go uses insecure.NewCredentials(). This exposes /v1/search query text and permits tampered extraction results. Add TLS/mTLS and peer validation, persist these settings during cli/cmd/init, and reject unsecured endpoints in daemon, routing, and catalog search paths.
📍 Affects 5 files
cli/cmd/daemon/config.go#L65-L77(this comment)cli/cmd/init/options.go#L32-L33cli/cmd/init/run.go#L229-L245cli/cmd/routing/nlsearch.go#L31-L35cli/cmd/search/nlsearch.go#L47-L47
🤖 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 `@cli/cmd/daemon/config.go` around lines 65 - 77, Require authenticated TLS or
mTLS for non-loopback remote extractor connections and validate the peer before
use, replacing insecure credentials in the extractor resolution flow. Persist
the TLS/mTLS configuration during cli/cmd/init/options.go and
cli/cmd/init/run.go, register and default the settings in
cli/cmd/daemon/config.go, and reject unsecured endpoints in
cli/cmd/routing/nlsearch.go and cli/cmd/search/nlsearch.go while preserving
trusted loopback or encrypted-mesh behavior.
| func loadExtractorFromConfig(fc *extractorEnricherFileConfig) (*sdk.Extractor, error) { | ||
| if fc.OASFUrl == "" && fc.AssetDir == "" { | ||
| ext, err := extractor.LoadConfigured() | ||
| ext, err := clientconfig.LoadConfigured() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Map the exact definitions before inspecting their implementations.
ast-grep outline client/config/extractor.go --items all --type function
ast-grep outline cli/cmd/import/config.go --items all --type function
# Confirm whether LoadConfigured resolves RemoteAddr or only creates a local SDK extractor.
rg -n -C 8 --type go \
'func\s+(LoadConfigured|ResolveConfigured)\b|RemoteAddr|ResolveExtractor|loadExtractorFromConfig' \
client/config/extractor.go cli/cmd/import/config.go cli/cmd/import/config_test.goRepository: agntcy/dir
Length of output: 5974
🏁 Script executed:
# Read the complete LoadConfigured function implementation
sed -n '44,80p' client/config/extractor.go
# Also check the SavedExtractorConfig struct to understand what RemoteAddr is available
rg -n -A 15 'type SavedExtractorConfig' client/config/extractor.go
# Check the extractorEnricherFileConfig struct in the import config
rg -n -A 10 'type extractorEnricherFileConfig' cli/cmd/import/config.goRepository: agntcy/dir
Length of output: 1114
🏁 Script executed:
# Check if SavedExtractorConfig has RemoteAddr field
rg -n -B 2 -A 10 'type SavedExtractorConfig' client/config/extractor.go
# Also verify LoadExtractor returns this type with RemoteAddr
rg -n -A 20 'func LoadExtractor' client/config/extractor.go | head -40Repository: agntcy/dir
Length of output: 148
🏁 Script executed:
# Search for the LoadExtractor function and what it returns
rg -n 'LoadExtractor|RemoteAddr' client/config/extractor.go
# Search in all files for SavedExtractorConfig definition
rg -n -B 2 -A 10 'SavedExtractorConfig|ExtractorConfig' client/config/ --type goRepository: agntcy/dir
Length of output: 339
🏁 Script executed:
# Get the full definition of LoadExtractor and its return type
rg -n -B 5 -A 20 'func LoadExtractor' client/config/extractor.go
# Also look at the complete extractor.go file to see all type definitions
cat -n client/config/extractor.goRepository: agntcy/dir
Length of output: 2496
🏁 Script executed:
# Continue reading the rest of extractor.go to find LoadExtractor and type definitions
sed -n '60,150p' client/config/extractor.goRepository: agntcy/dir
Length of output: 150
🏁 Script executed:
# Check file size and search for LoadExtractor in all client/config files
wc -l client/config/extractor.go
rg -n 'func LoadExtractor' client/config/ --type go -A 20Repository: agntcy/dir
Length of output: 1282
🏁 Script executed:
# Find the Extractor type definition in config.go
rg -n -B 2 -A 15 'type Extractor struct' client/config/config.goRepository: agntcy/dir
Length of output: 997
LoadConfigured() discards the saved remote extractor address.
At line 291, loadExtractorFromConfig calls clientconfig.LoadConfigured() when no explicit OASF config is provided. This function loads the saved Extractor config (which includes RemoteAddr from dirctl init), but it passes only OASFURL and AssetDir to extractor.Load(). The RemoteAddr is discarded. As a result, the extractor enricher always uses local assets, even when dirctl init --extractor-remote-addr configured a remote backend.
loadExtractorFromConfig also cannot accept RemoteAddr from the file config (extractorEnricherFileConfig has only OASFUrl and AssetDir). To support remote extractor enrichment, use clientconfig.ResolveConfigured() (which passes RemoteAddr to the resolver) or extend extractorEnricherFileConfig to include RemoteAddr.
🤖 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 `@cli/cmd/import/config.go` at line 291, In the loadExtractorFromConfig
function, preserve the RemoteAddr value returned by
clientconfig.LoadConfigured() and pass it to extractor.Load(). Either use
clientconfig.ResolveConfigured() instead of LoadConfigured() to directly handle
remote address resolution, or extend the extractorEnricherFileConfig struct to
include a RemoteAddr field and populate it from the loaded config before passing
to extractor.Load(). Ensure the remote extractor address configured during
dirctl init is propagated through the enricher initialization.
Comment on lines
+237
to
+245
| captured, err := runWithSpinner(cmd.Context(), os.Stdout, "Verifying remote extractor…", nil, | ||
| func(ctx context.Context) error { return smokeCheckRemote(ctx, cfg) }) | ||
| if err != nil { | ||
| // Config is saved; a down server is expected when configuring ahead of | ||
| // deployment, so warn rather than fail. | ||
| presenter.Printf(cmd, "⚠ Saved remote extractor %s, but could not reach it (is the server running?).\n", cfg.RemoteAddr) | ||
| printDetails(cmd, captured) | ||
|
|
||
| return nil //nolint:nilerr // connectivity is best-effort; the address is saved regardless. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Map the remote implementation before inspecting timeout handling.
ast-grep outline utils/extractor/remote.go --items all --type function
ast-grep outline cli/cmd/init/run.go --items all --type function
# Verify whether Extract adds its own deadline and whether smoke checks cover it.
rg -n -C 10 --type go \
'remoteExtractor|Extract\(|context\.WithTimeout|Deadline\(|smokeCheckRemote|runWithSpinner' \
utils/extractor/remote.go cli/cmd/init/run.go cli/cmd/init/run_test.goRepository: agntcy/dir
Length of output: 9318
🏁 Script executed:
# Examine runWithSpinner function and context handling
rg -n -B 5 -A 20 'func runWithSpinner' cli/cmd/init/run.go
# Check if there's any timeout setup in the Init command or parent context
rg -n 'WithTimeout|WithDeadline|cmd\.Context\(\)' cli/cmd/init/run.go
# Verify the Extract method doesn't set its own timeout
rg -n -A 15 'func.*Extract.*context.Context' utils/extractor/remote.goRepository: agntcy/dir
Length of output: 1159
🏁 Script executed:
# Find the definition of runWithSpinner - it might be in a different file
fd --type f -name "*.go" | xargs rg -l 'func runWithSpinner'
# Also check imports in run.go to see if it's imported
rg -n 'import|runWithSpinner' cli/cmd/init/run.go | head -30
# Check if it's defined in the same file earlier
cat -n cli/cmd/init/run.go | head -400 | grep -A 30 'runWithSpinner'Repository: agntcy/dir
Length of output: 4677
🏁 Script executed:
# Now examine the runWithSpinner implementation in progress.go
cat -n cli/cmd/init/progress.goRepository: agntcy/dir
Length of output: 6309
Add timeout to remote smoke check.
The call to smokeCheckRemote at line 237 receives cmd.Context() without a deadline. This context flows unchanged through runWithSpinner → smokeCheckRemote → Extract → gRPC call. If the remote server does not respond (for example, a blackholed connection), the gRPC call hangs indefinitely, blocking the command even though the intent at lines 240–241 is to warn and continue. Add a bounded context (for example, using context.WithTimeout) before passing it to runWithSpinner so the smoke check fails fast and returns the intended warning.
🤖 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 `@cli/cmd/init/run.go` around lines 237 - 245, Add a bounded context with a
timeout before passing cmd.Context() to the runWithSpinner call for the
smokeCheckRemote operation. Use context.WithTimeout to wrap cmd.Context() so
that if the remote server does not respond, the gRPC call fails fast and returns
control to the warning path at lines 240-241 instead of hanging indefinitely.
Comment on lines
+40
to
+54
| // LoadConfigured loads the extractor using the OASF URL / asset dir persisted by | ||
| // dirctl init, erroring clearly when init has not been run. This is the entry | ||
| // point for read-path consumers (import enrichment, search): they get a ready | ||
| // client or an actionable error, and never provision implicitly. | ||
| func LoadConfigured(opts ...sdk.Option) (*sdk.Extractor, error) { | ||
| saved, err := LoadExtractor("") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("load extractor config: %w", err) | ||
| } | ||
|
|
||
| if saved == nil { | ||
| return nil, errors.New("OASF extractor not configured; run `dirctl init` first") | ||
| } | ||
|
|
||
| ext, err := extractor.Load(extractor.Config{OASFURL: saved.OASFURL, AssetDir: saved.AssetDir}, opts...) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep run \
--pattern 'LoadConfigured($$$ARGS)' \
--lang go \
.Repository: agntcy/dir
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -t f -e go 'extractor|import|search|config' . | head -200
printf '%s\n' '--- LoadConfigured and related symbols ---'
rg -n -C 4 'LoadConfigured|RemoteAddr|LoadExtractor|extractor\.Load' --glob '*.go' .
printf '%s\n' '--- config package outline ---'
ast-grep outline client/config/extractor.go --lang goRepository: agntcy/dir
Length of output: 30370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- import caller ---'
cat -n cli/cmd/import/config.go | sed -n '270,315p'
printf '%s\n' '--- configured resolver implementations ---'
cat -n client/config/extractor.go | sed -n '1,75p'
cat -n utils/extractor/resolve.go | sed -n '23,85p'
printf '%s\n' '--- import command call graph references ---'
rg -n -C 3 'loadExtractorFromConfig|extractorEnricherFileConfig|enricher|LoadConfigured|ResolveConfigured' \
cli/cmd/import cli/cmd/search cli/cmd/routing client --glob '*.go'
printf '%s\n' '--- selector-aware LoadConfigured calls ---'
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('.').rglob('*.go'):
text = p.read_text(errors='replace')
for i, line in enumerate(text.splitlines(), 1):
if re.search(r'\bLoadConfigured\s*\(', line):
print(f'{p}:{i}:{line.strip()}')
PYRepository: agntcy/dir
Length of output: 23445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- extractor interfaces and adapters ---'
cat -n utils/extractor/extractor.go | sed -n '1,180p'
cat -n utils/extractor/resolve.go | sed -n '80,180p'
cat -n cli/cmd/import/extractor_adapter.go | sed -n '1,80p'
printf '%s\n' '--- import command entry points and configuration scope ---'
cat -n cli/cmd/import/config.go | sed -n '35,65p'
cat -n cli/cmd/import/config.go | sed -n '120,155p'
rg -n -C 3 'RunE|loadConfig|LoadConfig|New.*Import|import' cli/cmd/import --glob '*.go'
printf '%s\n' '--- all configured extractor callers ---'
rg -n -C 2 'ResolveConfigured|LoadConfigured' --glob '*.go' .Repository: agntcy/dir
Length of output: 32219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
config = Path("client/config/extractor.go").read_text()
import_cfg = Path("cli/cmd/import/config.go").read_text()
search_cfg = Path("cli/cmd/search/nlsearch.go").read_text()
load = re.search(
r'func LoadConfigured\(.*?\n\}',
config,
re.S,
)
assert load, "LoadConfigured not found"
load_body = load.group(0)
assert "extractor.Load(" in load_body
assert "saved.RemoteAddr" not in load_body
assert "clientconfig.LoadConfigured()" in import_cfg
assert "clientconfig.ResolveConfigured()" in search_cfg
print("LoadConfigured uses local extractor.Load and omits saved.RemoteAddr")
print("import path calls LoadConfigured")
print("natural-language search path calls ResolveConfigured")
PYRepository: agntcy/dir
Length of output: 302
Make import enrichment honor persisted RemoteAddr. cli/cmd/import/config.go:291 calls clientconfig.LoadConfigured(), which always invokes local extractor.Load and omits saved.RemoteAddr. A remote-only configuration fails during import enrichment. Add a remote-capable import adapter or reject remote configurations explicitly.
🤖 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 `@client/config/extractor.go` around lines 40 - 54, Update LoadConfigured to
honor saved.RemoteAddr during import enrichment instead of always calling local
extractor.Load with only OASFURL and AssetDir. Add or reuse a remote-capable
extractor adapter when RemoteAddr is configured, or explicitly return a clear
unsupported-remote error; preserve the existing local loading path for
configurations without RemoteAddr.
Comment on lines
+41
to
+48
| # OASF-SDK extractor server subchart (OPTIONAL - opt-in only) | ||
| # Serves the gateway's natural-language search (POST /v1/search) in cloud, | ||
| # where the apiserver image ships no local extractor assets. Enable via: | ||
| # oasf-sdk.enabled: true. Default: disabled. | ||
| - name: oasf-sdk | ||
| version: v1.1.0 | ||
| repository: oci://ghcr.io/agntcy/oasf-sdk/helm-charts | ||
| condition: oasf-sdk.enabled |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the documented oasfSdk.enabled configuration key.
The dependency condition uses oasf-sdk.enabled, but the PR contract specifies oasfSdk.enabled. Users who apply the documented value will not enable the extractor subchart.
Add a Helm dependency alias and update the condition. Align the related values and service-name templates with the same key.
Proposed Chart.yaml change
- name: oasf-sdk
+ alias: oasfSdk
version: v1.1.0
repository: oci://ghcr.io/agntcy/oasf-sdk/helm-charts
- condition: oasf-sdk.enabled
+ condition: oasfSdk.enabled📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # OASF-SDK extractor server subchart (OPTIONAL - opt-in only) | |
| # Serves the gateway's natural-language search (POST /v1/search) in cloud, | |
| # where the apiserver image ships no local extractor assets. Enable via: | |
| # oasf-sdk.enabled: true. Default: disabled. | |
| - name: oasf-sdk | |
| version: v1.1.0 | |
| repository: oci://ghcr.io/agntcy/oasf-sdk/helm-charts | |
| condition: oasf-sdk.enabled | |
| # OASF-SDK extractor server subchart (OPTIONAL - opt-in only) | |
| # Serves the gateway's natural-language search (POST /v1/search) in cloud, | |
| # where the apiserver image ships no local extractor assets. Enable via: | |
| # oasf-sdk.enabled: true. Default: disabled. | |
| - name: oasf-sdk | |
| alias: oasfSdk | |
| version: v1.1.0 | |
| repository: oci://ghcr.io/agntcy/oasf-sdk/helm-charts | |
| condition: oasfSdk.enabled |
🤖 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 `@install/charts/dir/apiserver/Chart.yaml` around lines 41 - 48, Update the
oasf-sdk dependency in Chart.yaml to use the documented oasfSdk.enabled key by
adding the appropriate Helm dependency alias and changing its condition. Align
the related values configuration and service-name templates with oasfSdk so
enabling the documented key consistently activates and configures the extractor
subchart.
Comment on lines
+214
to
+219
| {{/* | ||
| Resolve the OASF-SDK extractor gRPC address for the gateway's POST /v1/search. | ||
| An explicit extractor.remoteAddr wins; otherwise, when the oasf-sdk subchart is | ||
| enabled, point at its in-cluster service (reading the subchart's service port, | ||
| defaulting to 31234). Empty when neither is set — the gateway then answers | ||
| /v1/search with 503 until an extractor is configured. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the local-extractor fallback documentation.
An empty extractor.remote_addr selects the local extractor. A 503 response occurs only when the server has no usable extractor.
install/charts/dir/apiserver/templates/_helpers.tpl#L214-L219: describe the local extractor fallback instead of an unconditional 503 response.install/charts/dir/apiserver/values.yaml#L463-L466: describe the local extractor fallback instead of an unconditional 503 response.
📍 Affects 2 files
install/charts/dir/apiserver/templates/_helpers.tpl#L214-L219(this comment)install/charts/dir/apiserver/values.yaml#L463-L466
🤖 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 `@install/charts/dir/apiserver/templates/_helpers.tpl` around lines 214 - 219,
Update the extractor fallback documentation in
install/charts/dir/apiserver/templates/_helpers.tpl lines 214-219 and
install/charts/dir/apiserver/values.yaml lines 463-466 to state that an empty
extractor.remote_addr selects the local extractor; clarify that /v1/search
returns 503 only when no usable extractor is configured.
Comment on lines
+42
to
+47
| query := strings.TrimSpace(req.GetQuery()) | ||
| if query == "" { | ||
| return nil, status.Error(codes.InvalidArgument, "query is required") //nolint:wrapcheck | ||
| } | ||
|
|
||
| aiFinderLogger.Debug("SearchAgents called", "query", query, "filter", req.GetFilter(), "page_size", req.GetPageSize()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- server/controller/ai_finder_search.go ---'
sed -n '1,180p' server/controller/ai_finder_search.go
printf '%s\n' '--- related logging and request handling ---'
rg -n -C 3 'SearchAgents called|query.*query|Extract|Debug\\(|Error\\(' server/controller/ai_finder_search.go server/controller/ai_finder_search_test.goRepository: agntcy/dir
Length of output: 6835
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,180p' server/controller/ai_finder_search.go
rg -n -C 3 'SearchAgents called|query.*query|Extract|Debug\(|Error\(' server/controller/ai_finder_search.go server/controller/ai_finder_search_test.goRepository: agntcy/dir
Length of output: 24813
Sensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: External · Exploitability: Difficult
Do not log complete natural-language queries.
SearchAgentsRequest.query comes from POST /v1/search and is written to debug and error logs. Remove the complete value from both log calls. Log only its length, a request identifier, or a keyed digest. Review log retention and access controls for previously recorded queries.
🤖 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 `@server/controller/ai_finder_search.go` around lines 42 - 47, Update the
SearchAgents logging around the normalized query and its error path to stop
emitting the complete natural-language value. Replace query fields in both debug
and error logs with a non-reversible summary such as its length, request
identifier, or keyed digest, while preserving the validation behavior and log
context.
Comment on lines
+122
to
+149
| // Peek one past the page to learn whether a next page exists. | ||
| filterOpts = append(filterOpts, | ||
| types.WithLimit(pageSize+1), | ||
| types.WithOffset(offset), | ||
| sortModeToOrderBy(searchv1.SortMode_SORT_MODE_RELEVANCE), | ||
| ) | ||
|
|
||
| cids, err := c.db.GetRecordCIDs(filterOpts...) | ||
| if err != nil { | ||
| aiFinderLogger.Error("failed to search record CIDs", "error", err) | ||
|
|
||
| return nil, status.Error(codes.Internal, "failed to search catalog") //nolint:wrapcheck | ||
| } | ||
|
|
||
| hasMore := len(cids) > pageSize | ||
| if hasMore { | ||
| cids = cids[:pageSize] | ||
| } | ||
|
|
||
| entries, err := c.hydrateInCIDOrder(cids, facetOpts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var nextPageToken string | ||
| if hasMore { | ||
| nextPageToken = encodePageToken(offset + pageSize) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Apply facets before finalizing each page.
The content query paginates ranked CIDs before hydrateInCIDOrder applies facets. A facet can remove every CID in the selected window while a matching CID exists in a later window. The response then contains an empty page and a non-empty page token.
Apply facets in the ranked query when possible. Otherwise, continue fetching ranked CID batches until the response is full or the ranked result set is exhausted. Add a test that combines pagination with a selective facet.
Also applies to: 184-198
🤖 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 `@server/controller/ai_finder_search.go` around lines 122 - 149, Update the
pagination flow around GetRecordCIDs and hydrateInCIDOrder so facet filtering
occurs before finalizing a page. Prefer passing facet constraints into the
ranked CID query; otherwise fetch successive ranked batches until the page is
filled or results are exhausted, then generate nextPageToken from the consumed
results. Add coverage for pagination with a selective facet, ensuring
filtered-out CIDs do not produce an empty page with a non-empty token.
Comment on lines
+364
to
+366
| gwExtractor, aiFinderOpts := resolveGatewayExtractor(cfg) | ||
|
|
||
| catalogv1.RegisterAIFinderServiceServer(grpcServer, controller.NewAIFinderController(routingAPI.GetPeerID(), databaseAPI, cfg.HTTPGateway, storeAPI, aiFinderOpts...)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Release gwExtractor on gateway construction failure.
resolveGatewayExtractor can create a remote grpc.ClientConn. If gateway.New fails at Lines 401-403, New returns before a Server owns the extractor. Server.Close then cannot call gwExtractor.Close. Close the extractor in that error path, or defer cleanup until successful return transfers ownership.
Proposed cleanup
if err != nil {
+ if gwExtractor != nil {
+ if closeErr := gwExtractor.Close(); closeErr != nil {
+ logger.Warn("Failed to close OASF extractor after gateway setup failure", "error", closeErr)
+ }
+ }
return nil, fmt.Errorf("failed to create HTTP gateway: %w", err)
}🤖 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 `@server/server.go` around lines 364 - 366, Update the server construction flow
after resolveGatewayExtractor so a gateway.New failure closes gwExtractor before
returning the error. Ensure cleanup occurs only until successful Server creation
transfers ownership, preventing both the failure path and Server.Close from
closing the extractor.
Comment on lines
+511
to
518
| // Release the OASF extractor (closes the remote gRPC connection, if any). | ||
| if s.oasfExtractor != nil { | ||
| if err := s.oasfExtractor.Close(); err != nil { | ||
| logger.Error("Failed to close OASF extractor", "error", err) | ||
| } | ||
| } | ||
|
|
||
| s.grpcServer.GracefulStop() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Drain gRPC requests before closing oasfExtractor.
remoteExtractor.Close closes its client connection. This code closes that connection before s.grpcServer.GracefulStop(). An in-flight direct gRPC SearchAgents request can fail before the server drains it. Call GracefulStop first. Close the extractor after it returns.
Proposed shutdown order
- // Release the OASF extractor (closes the remote gRPC connection, if any).
- if s.oasfExtractor != nil {
- if err := s.oasfExtractor.Close(); err != nil {
- logger.Error("Failed to close OASF extractor", "error", err)
- }
- }
-
s.grpcServer.GracefulStop()
+
+ // Release the OASF extractor after all RPCs have drained.
+ if s.oasfExtractor != nil {
+ if err := s.oasfExtractor.Close(); err != nil {
+ logger.Error("Failed to close OASF extractor", "error", err)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Release the OASF extractor (closes the remote gRPC connection, if any). | |
| if s.oasfExtractor != nil { | |
| if err := s.oasfExtractor.Close(); err != nil { | |
| logger.Error("Failed to close OASF extractor", "error", err) | |
| } | |
| } | |
| s.grpcServer.GracefulStop() | |
| s.grpcServer.GracefulStop() | |
| // Release the OASF extractor after all RPCs have drained. | |
| if s.oasfExtractor != nil { | |
| if err := s.oasfExtractor.Close(); err != nil { | |
| logger.Error("Failed to close OASF extractor", "error", err) | |
| } | |
| } |
🤖 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 `@server/server.go` around lines 511 - 518, Update the shutdown sequence around
s.grpcServer.GracefulStop and s.oasfExtractor.Close: call GracefulStop first so
in-flight SearchAgents requests drain, then close the OASF extractor and retain
its existing error logging.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utils/extractor/remote.go (1)
40-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a bounded deadline for remote extraction.
SearchAgentspasses the inbound context through toremoteExtractor.Extract, and the gRPC client has no per-RPC timeout. Apply a configuredcontext.WithTimeoutbefore extraction to prevent stalled requests from retaining gateway resources indefinitely.🤖 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 `@utils/extractor/remote.go` around lines 40 - 45, Update remoteExtractor.Extract to derive a context.WithTimeout using the configured extraction deadline before calling r.client.Extract, and defer cancellation. Pass the timed context to the RPC while preserving the existing request construction and error handling.
🧹 Nitpick comments (1)
utils/extractor/remote_test.go (1)
60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new
Tiersrequest field.This fake client verifies request construction. The test forwards
Versionsbut does not exerciseTiers. SetTiers: 2and assertfake.gotReq.GetTiers() == 2. Add a negative-tier case if zero-default behavior is contractual.🤖 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 `@utils/extractor/remote_test.go` around lines 60 - 65, Update the Extract call in the remote extractor test to set Tiers to 2, then assert fake.gotReq.GetTiers() equals 2 alongside the existing request-field assertions. Add a negative-tier case only if zero-default behavior is an established contract.
🤖 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 `@cli/cmd/daemon/config.go`:
- Around line 65-77: Require authenticated TLS or mTLS for non-loopback remote
extractor connections and validate the peer before use, replacing insecure
credentials in the extractor resolution flow. Persist the TLS/mTLS configuration
during cli/cmd/init/options.go and cli/cmd/init/run.go, register and default the
settings in cli/cmd/daemon/config.go, and reject unsecured endpoints in
cli/cmd/routing/nlsearch.go and cli/cmd/search/nlsearch.go while preserving
trusted loopback or encrypted-mesh behavior.
In `@cli/cmd/import/config.go`:
- Line 291: In the loadExtractorFromConfig function, preserve the RemoteAddr
value returned by clientconfig.LoadConfigured() and pass it to extractor.Load().
Either use clientconfig.ResolveConfigured() instead of LoadConfigured() to
directly handle remote address resolution, or extend the
extractorEnricherFileConfig struct to include a RemoteAddr field and populate it
from the loaded config before passing to extractor.Load(). Ensure the remote
extractor address configured during dirctl init is propagated through the
enricher initialization.
In `@cli/cmd/init/run.go`:
- Around line 237-245: Add a bounded context with a timeout before passing
cmd.Context() to the runWithSpinner call for the smokeCheckRemote operation. Use
context.WithTimeout to wrap cmd.Context() so that if the remote server does not
respond, the gRPC call fails fast and returns control to the warning path at
lines 240-241 instead of hanging indefinitely.
In `@client/config/extractor.go`:
- Around line 40-54: Update LoadConfigured to honor saved.RemoteAddr during
import enrichment instead of always calling local extractor.Load with only
OASFURL and AssetDir. Add or reuse a remote-capable extractor adapter when
RemoteAddr is configured, or explicitly return a clear unsupported-remote error;
preserve the existing local loading path for configurations without RemoteAddr.
In `@install/charts/dir/apiserver/Chart.yaml`:
- Around line 41-48: Update the oasf-sdk dependency in Chart.yaml to use the
documented oasfSdk.enabled key by adding the appropriate Helm dependency alias
and changing its condition. Align the related values configuration and
service-name templates with oasfSdk so enabling the documented key consistently
activates and configures the extractor subchart.
In `@install/charts/dir/apiserver/templates/_helpers.tpl`:
- Around line 214-219: Update the extractor fallback documentation in
install/charts/dir/apiserver/templates/_helpers.tpl lines 214-219 and
install/charts/dir/apiserver/values.yaml lines 463-466 to state that an empty
extractor.remote_addr selects the local extractor; clarify that /v1/search
returns 503 only when no usable extractor is configured.
In `@server/controller/ai_finder_search.go`:
- Around line 122-149: Update the pagination flow around GetRecordCIDs and
hydrateInCIDOrder so facet filtering occurs before finalizing a page. Prefer
passing facet constraints into the ranked CID query; otherwise fetch successive
ranked batches until the page is filled or results are exhausted, then generate
nextPageToken from the consumed results. Add coverage for pagination with a
selective facet, ensuring filtered-out CIDs do not produce an empty page with a
non-empty token.
- Around line 42-47: Update the SearchAgents logging around the normalized query
and its error path to stop emitting the complete natural-language value. Replace
query fields in both debug and error logs with a non-reversible summary such as
its length, request identifier, or keyed digest, while preserving the validation
behavior and log context.
In `@server/server.go`:
- Around line 511-518: Update the shutdown sequence around
s.grpcServer.GracefulStop and s.oasfExtractor.Close: call GracefulStop first so
in-flight SearchAgents requests drain, then close the OASF extractor and retain
its existing error logging.
- Around line 364-366: Update the server construction flow after
resolveGatewayExtractor so a gateway.New failure closes gwExtractor before
returning the error. Ensure cleanup occurs only until successful Server creation
transfers ownership, preventing both the failure path and Server.Close from
closing the extractor.
In `@utils/extractor/assets.go`:
- Around line 30-44: Harden Teardown so it only removes a verified extractor
asset directory, not any arbitrary absolute path. After Config.Resolve and
before os.RemoveAll, resolve the target’s symlinks and validate that it is under
an allowlisted asset root or matches persisted provisioning identity, and
require a valid extractor manifest in that directory. Update guardAssetDir or
the Teardown flow accordingly while preserving the existing refusal of empty,
root, and home paths.
In `@utils/extractor/provision_integration_test.go`:
- Around line 15-16: Update the run command comment in
provision_integration_test.go to execute from the utils module’s correct package
location and use the actual test function name, replacing the invalid
./internal/extractor/ path and TestProvisionSmokeCheck identifier.
---
Outside diff comments:
In `@utils/extractor/remote.go`:
- Around line 40-45: Update remoteExtractor.Extract to derive a
context.WithTimeout using the configured extraction deadline before calling
r.client.Extract, and defer cancellation. Pass the timed context to the RPC
while preserving the existing request construction and error handling.
---
Nitpick comments:
In `@utils/extractor/remote_test.go`:
- Around line 60-65: Update the Extract call in the remote extractor test to set
Tiers to 2, then assert fake.gotReq.GetTiers() equals 2 alongside the existing
request-field assertions. Add a negative-tier case only if zero-default behavior
is an established contract.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f21a1d41-4e2a-4085-81c6-126bd898d232
⛔ Files ignored due to path filters (7)
api/catalog/v1/ai_finder_service.pb.gois excluded by!**/*.pb.goapi/catalog/v1/ai_finder_service.pb.gw.gois excluded by!**/*.pb.gw.goapi/catalog/v1/ai_finder_service_grpc.pb.gois excluded by!**/*.pb.goinstall/charts/dir/apiserver/Chart.lockis excluded by!**/*.lockreconciler/go.sumis excluded by!**/*.sumserver/go.sumis excluded by!**/*.sumutils/go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
cli/cmd/daemon/config.gocli/cmd/import/config.gocli/cmd/init/options.gocli/cmd/init/run.gocli/cmd/init/run_test.gocli/cmd/routing/nlsearch.gocli/cmd/search/nlsearch.goclient/config/extractor.goclient/config/extractor_test.goclient/go.modinstall/charts/dir/apiserver/Chart.yamlinstall/charts/dir/apiserver/templates/_helpers.tplinstall/charts/dir/apiserver/templates/configmap.yamlinstall/charts/dir/apiserver/values.yamlinstall/charts/dir/values.yamlproto/agntcy/dir/catalog/v1/ai_finder_service.protoreconciler/go.modserver/config/config.goserver/config/config_test.goserver/controller/ai_finder.goserver/controller/ai_finder_filter.goserver/controller/ai_finder_search.goserver/controller/ai_finder_search_test.goserver/controller/ai_finder_test.goserver/go.modserver/server.gotests/e2e/local/testenv/kind/dir-chart-values.yamlutils/extractor/assets.goutils/extractor/assets_test.goutils/extractor/config.goutils/extractor/config_test.goutils/extractor/extractor.goutils/extractor/load_test.goutils/extractor/local.goutils/extractor/provision_integration_test.goutils/extractor/remote.goutils/extractor/remote_test.goutils/extractor/resolve.goutils/extractor/resolve_test.goutils/go.modutils/nlsearch/decompose.goutils/nlsearch/decompose_test.go
💤 Files with no reviewable changes (1)
- utils/extractor/resolve.go
🛑 Comments failed to post (2)
utils/extractor/assets.go (1)
30-44: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Restrict teardown to a verified extractor asset directory.
The guard permits arbitrary absolute paths such as
/etc,/var/lib/app, or another application's data directory.os.RemoveAllwill recursively delete that path with the process privileges.Before deletion, resolve symlinks and require a valid extractor manifest under the target directory. Prefer an allowlisted asset root or persisted provisioning identity when custom asset directories are supported.
Also applies to: 49-70
🤖 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 `@utils/extractor/assets.go` around lines 30 - 44, Harden Teardown so it only removes a verified extractor asset directory, not any arbitrary absolute path. After Config.Resolve and before os.RemoveAll, resolve the target’s symlinks and validate that it is under an allowlisted asset root or matches persisted provisioning identity, and require a valid extractor manifest in that directory. Update guardAssetDir or the Teardown flow accordingly while preserving the existing refusal of empty, root, and home paths.utils/extractor/provision_integration_test.go (1)
15-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the integration-test command.
./internal/extractor/does not contain this test. Run the command from theutilsmodule and use the actual test name.Proposed correction
-// Run with: go test -tags extractor_integration ./internal/extractor/ -run TestProvisionSmokeCheck +// Run with: cd utils && go test -tags extractor_integration ./extractor -run '^TestProvisionSmokeCheckIntegration$'🤖 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 `@utils/extractor/provision_integration_test.go` around lines 15 - 16, Update the run command comment in provision_integration_test.go to execute from the utils module’s correct package location and use the actual test function name, replacing the invalid ./internal/extractor/ path and TestProvisionSmokeCheck identifier.
akijakya
marked this pull request as draft
August 6, 2026 15:14
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.
Closes #1905
Closes #1906
Closes #1907
Summary
Adds a natural-language search endpoint to the AI Catalog —
POST /v1/search— so the UI (and other clients) can send free text like "a skill for reviewing code" and get back relevance-orientedCatalogEntryresults, without exposing the OASF extractor or the streamingSearchServiceto the browser.This is the backend → config → deploy spine of epic #1903 (natural-language search in the AI Catalog UI). It builds on the shared pluggable extractor from #1904 (#1925). UI wiring (#1908) and e2e (#1909) are separate follow-ups.
What it does (extract-then-filter, v1)
POST /v1/search { query, page_size, page_token, filter }on the AI Finder gateway:queryvia the OASF extractor.RecordQuerys and run one in-process content search (GetRecordCIDs).CatalogEntry(same shape asListAgents, so the UI only swaps data source).filterfacets (type/verified/trusted/safe/tags) via the existing AND-grammar parser.displayNamesubstring match so the user still gets results.codes.Unavailable(HTTP 503) with an actionable message.The extractor is pluggable — local in-process assets (
dirctl daemon start) or a remote OASF-SDK gRPC server (cloud) — resolved once at gateway startup and injected into the controller.Why the extractor package was refactored again
#1904 (#1925) introduced the shared pluggable extractor, but it landed in the
clientmodule (the dir gRPC SDK). This endpoint needs the extractor in the server, and importingclientintoserverwas the wrong direction:client/extractor→client/config→ the rootclientpackage, it dragged the entire SDK dependency closure (sigstore/cosign, k8s client-go, go-tuf, zitadel/oidc, … ~100 modules) into the server binary.So this PR moves the SDK-agnostic core —
Extractor,ResolveExtractor, the local/remote backends,Config,Provision— into the genuinely-sharedutilsmodule asutils/extractor+utils/nlsearch. The two convenience functions that read the persisteddirctlconfig (ResolveConfigured,LoadConfigured) stay inclient/config, since they legitimately depend on the CLI config layer.Result:
serverimportsutils/extractoronly —go list -deps ./server/...no longer referencesagntcy/dir/client, and the heavy SDK deps are gone from the server. (Visible in the diff:cli/cmd/import/config.gonow callsclientconfig.LoadConfigured()instead ofextractor.LoadConfigured()— same behavior, correct home.utils/extractornever importsclient, which is what preserves the layering.)Config & deployment
dirctl daemon start)~/.agntcy/oasf-sdk/extractor. Works out of the box.DIRECTORY_DAEMON_SERVER_EXTRACTOR_REMOTE_ADDR=host:port→ remote backend.oasf-sdksubchart (off by default).oasf-sdk.enabled=truedeploys it; the gateway auto-wiresextractor.remote_addrto the in-cluster service, andOASF_SDK_EXTRACTOR_OASF_URLenables the extractor gRPC service on the server.New server config block:
extractor: { remote_addr, asset_dir, oasf_url }(envDIRECTORY_SERVER_EXTRACTOR_*), consulted only when the HTTP gateway is enabled. Resolution failure is non-fatal — the server still starts;/v1/searchreturns 503 until an extractor is configured.Commits
feat(api)—SearchAgentsRPC (POST /v1/search) + regenerated bindingsrefactor(extractor)— move the extractor + nlsearch fromclienttoutils(see above)feat(server)— extract-then-filterPOST /v1/searchhandler + testsfeat(server)— resolve the OASF extractor at gateway startup ([Feature]: Extractor config (RemoteAddr) + resolver wiring for dirctl and gateway #1906)feat(cli)— configure a remote extractor indirctl init([Feature]: Extractor config (RemoteAddr) + resolver wiring for dirctl and gateway #1906)feat(helm)— optional OASF-SDK extractor subchart + gateway auto-wiring ([Feature]: Helm: optional OASF-SDK extractor server deployment + config #1907)feat(extractor)— default to two score tiers everywhere the extractor is used (extractor.DefaultTiers): gateway/v1/search,dirctl search/routing, and import enrichment now widen recall to the two closest score groups by default; callers can still narrow (Tiers: 1) or widen per queryrefactor(extractor)— unify on one resolution path (ResolveExtractor/ResolveConfigured): drop the local-onlyLoadConfigured, so every caller resolves remote if an address is configured, else local (configured or default dir), else error. Import enrichment now works against a remote OASF-SDK server too (withremote_addrin the import file config, a per-Extracttimeout, andClose()on completion)fix— review feedback: shutdown ordering (close extractor afterGracefulStop), resolver-leak on gateway-init failure, remote smoke-check timeout, log query length instead of text, chart-doc correctionsTesting
503when unavailable, pagination round-trip);nlsearchtier tests; server config-binding tests.golangci-lintclean across touched modules.dirctl daemon start→POST /v1/searchreturns 200 with in-process extraction; separately confirmed the published OASF-SDK server servesExtractorServiceand extracts correctly once it has the model. Helm rendering verified (helm template+helm lint --with-subcharts).Design notes & follow-ups
popularity_score(pull + lookup counts) andprovider_count, with workingPOPULARITY/PROVIDER_COUNT/RECENCYsort modes. What is not yet implemented is semantic / query-match relevance:SORT_MODE_RELEVANCEis a scaffold that currently falls back to recency order, so/v1/searchresults are recency-ordered for now. Follow-up: rank by match quality (how many extracted signals a record hit) withpopularity_scoreas a tiebreaker — or, as an interim step, sort the endpoint byPOPULARITY. (total_countfor search responses is tracked separately in SearchService: return total match count alongside paginated results #1939.)oasf-sdkimage downloads the embedding model from HuggingFace at pod start (the chart can't yet mount pre-provisioned assets). Fine where the cluster has HF egress; a model-baked image is the robust path.GET /v1/agentsuntil [Feature]: AI Catalog UI: wire the search bar to /v1/search #1908 wires it to/v1/search.