AI prompts Phase B: client-side loading via injectable fs.FS#1850
AI prompts Phase B: client-side loading via injectable fs.FS#1850OrriMandarin wants to merge 1 commit into
Conversation
coderabbitai
Bot
added
enhancement
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis PR introduces a tiered AI prompts filesystem abstraction ( ChangesAI Prompts FS Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Server as cmd/server.go
participant InfraInit as infra.InitAiPromptsFS
participant Downloader as downloadAiPromptsFS
participant Validator as ai_agent.ValidatePromptsFS
participant Usecases as usecases.Usecases
Server->>InfraInit: InitAiPromptsFS(servingDir, version, licenseKey)
InfraInit->>InfraInit: check AI_PROMPTS_EXECUTION_DIR
alt execution dir readable
InfraInit-->>Server: os.DirFS(executionDir)
else serving dir resolves version
InfraInit-->>Server: os.DirFS(servingDir/vX.Y)
else fallback to network
InfraInit->>Downloader: GET prompts zip (Authorization, prompts_version)
Downloader-->>InfraInit: zip bytes or nil
InfraInit->>InfraInit: zipToPromptsFS(bytes)
InfraInit-->>Server: AiAgentPromptsMapFS or nil
end
Server->>Validator: ValidatePromptsFS(aiPromptsFS)
Validator-->>Server: nil or error (warning logged)
Server->>Usecases: WithAIPromptsFS(aiPromptsFS), WithAIPromptsServingDir(dir)
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (8)
pure_utils/prompt_version.go (1)
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the parse error with context.
semver.NewVersion(requestedVersion)'s error is returned bare here — callers only get the underlying semver message with no indication it came from resolving a prompt version. A little context now saves a little confusion anon.As per coding guidelines,
**/*.go: "Wrap errors with context usinggithub.com/cockroachdb/errorspackage."♻️ Proposed fix
+ "github.com/cockroachdb/errors" "github.com/Masterminds/semver" ) ... reqV, err := semver.NewVersion(requestedVersion) if err != nil { - return "", err + return "", errors.Wrapf(err, "invalid requested prompt version %q", requestedVersion) }🤖 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 `@pure_utils/prompt_version.go` around lines 13 - 17, Wrap the parse failure in ResolveBestPromptVersion with context instead of returning the bare semver.NewVersion(requestedVersion) error; use the cockroachdb/errors package to annotate the error so callers can tell it happened while resolving a prompt version. Keep the change localized to ResolveBestPromptVersion and preserve the original error as the cause while adding a clear message mentioning requestedVersion.Source: Coding guidelines
models/ai_agent_config.go (1)
35-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
cockroachdb/errorsfor wrapping, notfmt.Errorf.These two error paths wrap with
fmt.Errorf("...%w", ...)rather than the repo's mandatedgithub.com/cockroachdb/errors. A small nit, yet as the bard might say: consistency in errors, like consistency in verse, keeps the reader's trust unswerved.As per coding guidelines,
**/*.go: "Wrap errors with context usinggithub.com/cockroachdb/errorspackage."♻️ Proposed fix
file, err := promptsFS.Open(AiAgentModelConfigFileName) if err != nil { - return nil, fmt.Errorf("could not open AI agent config file %s: %w", AiAgentModelConfigFileName, err) + return nil, errors.Wrapf(err, "could not open AI agent config file %s", AiAgentModelConfigFileName) } defer file.Close() var config AiAgentModelConfig if err := json.NewDecoder(file).Decode(&config); err != nil { - return nil, fmt.Errorf("could not decode AI agent config file %s: %w", AiAgentModelConfigFileName, err) + return nil, errors.Wrapf(err, "could not decode AI agent config file %s", AiAgentModelConfigFileName) }Requires adding
"github.com/cockroachdb/errors"to the import block (and droppingfmtif it becomes unused elsewhere in the file).🤖 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 `@models/ai_agent_config.go` around lines 35 - 44, The error wrapping in AiAgentModelConfig loading uses fmt.Errorf instead of the repo-standard github.com/cockroachdb/errors. Update the error paths in the AI agent config loader logic around promptsFS.Open and json.NewDecoder(file).Decode to wrap with cockroachdb/errors while preserving the same context and underlying error, and add the cockroachdb/errors import (removing fmt if it is no longer used in models/ai_agent_config.go).Source: Coding guidelines
usecases/ai_agent/async_case_review.go (1)
169-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSound cancellation logic.
Checking
errors.Is(err, ErrPromptsUnavailable)on the originalerr(nothandledErr) is the right call — no risk of the sentinel getting lost in the join/wrap performed byhandleCreateCaseReviewSyncError. Persisting theFailedstatus before cancelling is a good touch; the job dies, but the case review record doesn't dangle.One small thought, worth a line if you're passing this way again: unlike its sibling in
async_screening_suggestion.go, this branch doesn't emit an explicit log before cancelling — future debugging of "why did this job vanish" would benefit from a breadcrumb here, much as a herald announces the King's decree before the curtain falls.📝 Optional: add a log line before cancelling
if errors.Is(err, ErrPromptsUnavailable) { + logger.WarnContext(ctx, "AI prompts filesystem unavailable, cancelling case review job", "error", err) // No prompts filesystem available for this instance - retrying won't help, only // this job fails, not the process. The Failed status was already persisted above. return river.JobCancel(handledErr) }🤖 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 `@usecases/ai_agent/async_case_review.go` around lines 169 - 180, Add an explicit log breadcrumb in the ErrPromptsUnavailable cancellation branch of async_case_review.go before returning river.JobCancel(handledErr). Keep the existing errors.Is(err, ErrPromptsUnavailable) check on the original err, but emit a clear log from the same path in asyncCaseReview handling so it’s obvious why the job was cancelled, similar to the sibling flow in async_screening_suggestion.go.cmd/server.go (1)
388-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating the init+validate+warn block with
cmd/worker.go.The
infra.InitAiPromptsFS→ai_agent.ValidatePromptsFS→ warn-log sequence here is copy-pasted verbatim incmd/worker.go(lines 366-375 there). Since both files appear to share the same package/import surface, a small shared helper would keep the two entry points from drifting when this logic changes (e.g., adding metrics, changing the log level).'Tis but a modest split, yet one line changed twice is a bug waiting to be born.
♻️ Proposed extraction
+func initAndValidateAiPromptsFS(ctx context.Context, promptsServingDir, version, licenseKey string) fs.FS { + aiPromptsFS := infra.InitAiPromptsFS(ctx, promptsServingDir, version, licenseKey) + if err := ai_agent.ValidatePromptsFS(aiPromptsFS); err != nil { + utils.LoggerFromContext(ctx).WarnContext(ctx, "ai prompts filesystem failed validation, some ai features may fail", + "error", err.Error()) + } + return aiPromptsFS +}Then in
RunServer:- aiPromptsFS := infra.InitAiPromptsFS(ctx, aiPromptsServingDir, config.Version, licenseConfig.LicenseKey) - if err := ai_agent.ValidatePromptsFS(aiPromptsFS); err != nil { - utils.LoggerFromContext(ctx).WarnContext(ctx, "ai prompts filesystem failed validation, some ai features may fail", - "error", err.Error()) - } + aiPromptsFS := initAndValidateAiPromptsFS(ctx, aiPromptsServingDir, config.Version, licenseConfig.LicenseKey)Also applies to: 417-418
🤖 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 `@cmd/server.go` around lines 388 - 393, The ai prompts filesystem init/validate/warn flow is duplicated between RunServer and the worker entrypoint, so extract it into a shared helper to keep both paths consistent. Move the `infra.InitAiPromptsFS` → `ai_agent.ValidatePromptsFS` → `utils.LoggerFromContext(ctx).WarnContext(...)` sequence into a common function, then have `RunServer` call that helper instead of inlining the block. Use the existing symbols `InitAiPromptsFS`, `ValidatePromptsFS`, and `WarnContext` to locate the logic and keep behavior identical in both callers.usecases/ai_agent/ai_agent_usecase_test.go (1)
19-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest updates line up with the new receiver-based API.
All six tests correctly swap the package-level
preparePromptcall forAiAgentUsecase{promptsFS: os.DirFS(tmpDir)}.preparePrompt(...).One gap: none of these tests exercise the new nil-
promptsFSpath (ErrPromptsUnavailable), which is the one behavioral change actually introduced in this PR. A quick added case would lock in that contract.🧪 Suggested additional test
func TestPreparePrompt_NoPromptsFS(t *testing.T) { uc := AiAgentUsecase{} _, err := uc.preparePrompt("test_prompt.txt", map[string]any{}) if !errors.Is(err, ErrPromptsUnavailable) { t.Fatalf("expected ErrPromptsUnavailable, got: %v", err) } }Also applies to: 50-79, 81-105, 107-136, 138-163, 165-205
🤖 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 `@usecases/ai_agent/ai_agent_usecase_test.go` around lines 19 - 48, Add a test covering the new nil-promptsFS behavior in AiAgentUsecase.preparePrompt: create an AiAgentUsecase without setting promptsFS, call preparePrompt with any prompt name and empty data, and assert the error matches ErrPromptsUnavailable. Keep the existing receiver-based test updates intact and add this case alongside the other preparePrompt tests so the new contract is locked in.usecases/ai_agent/ai_agent_usecase.go (2)
201-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConstructor keeps growing via positional args.
promptsFSbecomes yet another positional parameter tacked onto an already very longNewAiAgentUsecaseargument list (20+ params). Easy to introduce an ordering mistake on a future edit (as seen in the sibling call-site file). Worth considering the functional-options pattern here, as is already done for the top-levelUsecasesstruct (WithAIPromptsFS, etc.).As per coding guidelines,
**/*.go: "Use the functional options pattern for dependency injection when initializing repositories, usecases, and feature-specific components."Also applies to: 232-233, 258-258
🤖 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 `@usecases/ai_agent/ai_agent_usecase.go` at line 201, The NewAiAgentUsecase constructor is growing with another positional dependency, making the long argument list error-prone. Update NewAiAgentUsecase to accept promptsFS via the functional-options pattern instead of adding another positional parameter, and thread it through the AiAgentUsecase initialization using an option similar to the existing Usecases builders (for example, the same style as WithAIPromptsFS). Adjust any call sites accordingly so the dependency is configured by option rather than argument ordering.Source: Coding guidelines
171-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSentinel error package inconsistent with cockroachdb/errors guideline.
ErrPromptsUnavailableis declared viaerrors.Newfromgithub.com/pkg/errors(per the library context), while the coding guidelines call forgithub.com/cockroachdb/errorsfor error wrapping/context, and a sibling file in this very package (llm_errors.go) already usescockroachdb/errors(errors.Mark) for sentinel-style errors. Mixing the two error libraries withinusecases/ai_agentrisks confusion over whichIs/As/Marksemantics apply, even thoughpkg/errors0.9.1'sUnwrap()support keepserrors.Isworking here.As per coding guidelines,
**/*.go: "Wrap errors with context usinggithub.com/cockroachdb/errorspackage."🤖 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 `@usecases/ai_agent/ai_agent_usecase.go` around lines 171 - 176, ErrPromptsUnavailable in ai_agent_usecase.go is using the pkg/errors sentinel style instead of the package’s cockroachdb/errors convention; update this sentinel and any related error creation in readPrompt/InitAiPromptsFS call paths to use github.com/cockroachdb/errors consistently, matching the approach already used in llm_errors.go with errors.Mark. Keep the symbol name ErrPromptsUnavailable so callers and non-retryable checks remain unchanged, but ensure the declaration and any wrapping/context in this package follow cockroachdb/errors semantics.Source: Coding guidelines
infra/ai_prompts.go (1)
89-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap resolver errors with CockroachDB error context.
resolveLocalPromptsVersionreturns rawos.ReadDir/ resolver errors and usesfmt.Errorf; useerrors.Wrapf/errors.Newfso startup logs retain actionable context. As per coding guidelines,**/*.go: "Wrap errors with context usinggithub.com/cockroachdb/errorspackage".Proposed fix
entries, err := os.ReadDir(promptsServingDir) if err != nil { - return "", err + return "", errors.Wrapf(err, "read ai prompts serving directory %s", promptsServingDir) } @@ best, err := pure_utils.ResolveBestPromptVersion(names, version) if err != nil { - return "", err + return "", errors.Wrapf(err, "resolve ai prompts version %s", version) } if best == "" { - return "", fmt.Errorf("no ai prompts version available for %s in %s", version, promptsServingDir) + return "", errors.Newf("no ai prompts version available for %s in %s", version, promptsServingDir) }🤖 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 `@infra/ai_prompts.go` around lines 89 - 106, The resolveLocalPromptsVersion logic in ai_prompts.go is returning plain os.ReadDir and ResolveBestPromptVersion errors and also uses fmt.Errorf for the empty-version case, so update those returns to use CockroachDB error helpers instead. Keep the same flow in resolveLocalPromptsVersion, but wrap the directory read and resolver failures with errors.Wrapf and create the missing-version error with errors.Newf so startup logs retain the promptsServingDir and version context.Source: Coding guidelines
🤖 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 `@infra/ai_prompts_test.go`:
- Around line 189-222: Add a regression test in ai_prompts_test.go for the
InitAiPromptsFS retry path that specifically covers HTTP 429 Too Many Requests.
Mirror the existing retryable 5xx case by using an httptest.Server that returns
429 on the first requests and then succeeds with a ZIP payload, and assert that
InitAiPromptsFS eventually returns a populated FS and that the server was called
multiple times. Keep the existing non-retryable 4xx test separate so the retry
policy distinction remains explicit.
In `@infra/ai_prompts.go`:
- Around line 155-159: The status handling in the response check currently makes
every non-200/non-5xx response unrecoverable, which incorrectly blocks retries
for 429. Update the retry logic in the code path that checks resp.StatusCode so
that 429 Too Many Requests is treated as retryable like the other tier-3
transient failures, while still wrapping truly non-retryable statuses with
retry.Unrecoverable; use the existing status-code branch near the
http.StatusInternalServerError check to locate it.
- Around line 149-163: Add a bounded HTTP timeout and cap archive/entry reads in
the prompts bundle fetch path. Update the request logic around
`http.DefaultClient.Do` to use a client or context with a timeout so startup
can’t hang indefinitely, and replace the unrestricted
`io.ReadAll(resp.Body)`/`io.ReadAll(f)` behavior with size-limited reads in the
bundle extraction flow. Keep the changes localized to the prompts bundle loading
code so the fetch, unzip, and file parsing paths all enforce the same maximum
byte limit.
---
Nitpick comments:
In `@cmd/server.go`:
- Around line 388-393: The ai prompts filesystem init/validate/warn flow is
duplicated between RunServer and the worker entrypoint, so extract it into a
shared helper to keep both paths consistent. Move the `infra.InitAiPromptsFS` →
`ai_agent.ValidatePromptsFS` → `utils.LoggerFromContext(ctx).WarnContext(...)`
sequence into a common function, then have `RunServer` call that helper instead
of inlining the block. Use the existing symbols `InitAiPromptsFS`,
`ValidatePromptsFS`, and `WarnContext` to locate the logic and keep behavior
identical in both callers.
In `@infra/ai_prompts.go`:
- Around line 89-106: The resolveLocalPromptsVersion logic in ai_prompts.go is
returning plain os.ReadDir and ResolveBestPromptVersion errors and also uses
fmt.Errorf for the empty-version case, so update those returns to use
CockroachDB error helpers instead. Keep the same flow in
resolveLocalPromptsVersion, but wrap the directory read and resolver failures
with errors.Wrapf and create the missing-version error with errors.Newf so
startup logs retain the promptsServingDir and version context.
In `@models/ai_agent_config.go`:
- Around line 35-44: The error wrapping in AiAgentModelConfig loading uses
fmt.Errorf instead of the repo-standard github.com/cockroachdb/errors. Update
the error paths in the AI agent config loader logic around promptsFS.Open and
json.NewDecoder(file).Decode to wrap with cockroachdb/errors while preserving
the same context and underlying error, and add the cockroachdb/errors import
(removing fmt if it is no longer used in models/ai_agent_config.go).
In `@pure_utils/prompt_version.go`:
- Around line 13-17: Wrap the parse failure in ResolveBestPromptVersion with
context instead of returning the bare semver.NewVersion(requestedVersion) error;
use the cockroachdb/errors package to annotate the error so callers can tell it
happened while resolving a prompt version. Keep the change localized to
ResolveBestPromptVersion and preserve the original error as the cause while
adding a clear message mentioning requestedVersion.
In `@usecases/ai_agent/ai_agent_usecase_test.go`:
- Around line 19-48: Add a test covering the new nil-promptsFS behavior in
AiAgentUsecase.preparePrompt: create an AiAgentUsecase without setting
promptsFS, call preparePrompt with any prompt name and empty data, and assert
the error matches ErrPromptsUnavailable. Keep the existing receiver-based test
updates intact and add this case alongside the other preparePrompt tests so the
new contract is locked in.
In `@usecases/ai_agent/ai_agent_usecase.go`:
- Line 201: The NewAiAgentUsecase constructor is growing with another positional
dependency, making the long argument list error-prone. Update NewAiAgentUsecase
to accept promptsFS via the functional-options pattern instead of adding another
positional parameter, and thread it through the AiAgentUsecase initialization
using an option similar to the existing Usecases builders (for example, the same
style as WithAIPromptsFS). Adjust any call sites accordingly so the dependency
is configured by option rather than argument ordering.
- Around line 171-176: ErrPromptsUnavailable in ai_agent_usecase.go is using the
pkg/errors sentinel style instead of the package’s cockroachdb/errors
convention; update this sentinel and any related error creation in
readPrompt/InitAiPromptsFS call paths to use github.com/cockroachdb/errors
consistently, matching the approach already used in llm_errors.go with
errors.Mark. Keep the symbol name ErrPromptsUnavailable so callers and
non-retryable checks remain unchanged, but ensure the declaration and any
wrapping/context in this package follow cockroachdb/errors semantics.
In `@usecases/ai_agent/async_case_review.go`:
- Around line 169-180: Add an explicit log breadcrumb in the
ErrPromptsUnavailable cancellation branch of async_case_review.go before
returning river.JobCancel(handledErr). Keep the existing errors.Is(err,
ErrPromptsUnavailable) check on the original err, but emit a clear log from the
same path in asyncCaseReview handling so it’s obvious why the job was cancelled,
similar to the sibling flow in async_screening_suggestion.go.
🪄 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: f9d55ec8-9b76-4cd1-8952-693145658b74
📒 Files selected for processing (24)
cmd/server.gocmd/worker.goinfra/ai_prompts.goinfra/ai_prompts_test.gomodels/ai_agent_config.gomodels/ai_agent_config_test.gomodels/ai_agent_prompts.gomodels/ai_agent_prompts_fs.gomodels/ai_agent_prompts_fs_test.gopure_utils/prompt_version.gopure_utils/prompt_version_test.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gousecases/prompt_serving_usecase.gousecases/usecases.gousecases/usecases_with_creds.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: test_backend / Test back-end
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Wrap errors with context usinggithub.com/cockroachdb/errorspackage
Use custom error types frommodels/errors.go(BadParameterError, NotFoundError, ForbiddenError, ConflictError, etc.) for domain-specific errors
Uselog/slogstructured logging with context, providing relevant key-value pairs for debugging
Use the functional options pattern for dependency injection when initializing repositories, usecases, and feature-specific components
Files:
usecases/ai_agent/ai_rule_assist.gopure_utils/prompt_version_test.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gopure_utils/prompt_version.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gocmd/server.gocmd/worker.gomodels/ai_agent_config.gomodels/ai_agent_prompts.gomodels/ai_agent_config_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.goinfra/ai_prompts.gomodels/ai_agent_prompts_fs_test.gousecases/prompt_serving_usecase.goinfra/ai_prompts_test.gomodels/ai_agent_prompts_fs.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
usecases/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
usecases/**/*.go: Usecredentials.Get(ctx)to extract organization context and user permissions from the request context in usecases
Feature-specific usecases should be created via factory methods likeNewCaseUsecase(),NewDecisionUsecase()for improved modularity
Files:
usecases/ai_agent/ai_rule_assist.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.gousecases/prompt_serving_usecase.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use
testify/assertandtestify/requirefor assertions in unit tests
Files:
pure_utils/prompt_version_test.gousecases/ai_agent/prompts_manifest_test.gomodels/ai_agent_config_test.gousecases/ai_agent/ai_agent_usecase_test.gomodels/ai_agent_prompts_fs_test.goinfra/ai_prompts_test.go
{cmd/worker.go,models/river_job.go,jobs/**/*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Register new River job worker implementations in
cmd/worker.goafter defining the job args struct inmodels/river_job.go
Files:
cmd/worker.go
usecases/*_usecase.go
📄 CodeRabbit inference engine (CLAUDE.md)
usecases/*_usecase.go: In usecases, validate that all resource IDs from user input (request body, query params, URL params) belong to the caller's organization usingenforceSecuritychecks or direct org comparison
Fetch referenced resources by ID in usecases and verify they belong to the caller's organization before using them; do not rely on repositoryGet*ByIdmethods to filter by organization
Files:
usecases/prompt_serving_usecase.go
🧠 Learnings (8)
📚 Learning: 2026-02-27T16:10:37.065Z
Learnt from: apognu
Repo: checkmarble/marble-backend PR: 1558
File: usecases/decision_usecase.go:419-430
Timestamp: 2026-02-27T16:10:37.065Z
Learning: In usecases/decision_usecase.go, payload.Data["object_id"] is asserted to string with the assumption that earlier payload validation guarantees a string. This is actionable: review the file for all such type assertions on payload.Data fields and ensure there is prior validation confirming the type. Prefer 1) keeping the constraint explicit in the validation layer, 2) using a typed struct if possible to avoid map[string]interface{} assertions, and 3) using a type assertion with a safe, checked path (e.g., v, ok := payload.Data["object_id"].(string)) where appropriate. Apply this guidance to all similar files in the usecases directory (and nested, if present) to ensure consistency and avoid runtime panics due to unexpected types.
Applied to files:
usecases/ai_agent/ai_rule_assist.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.gousecases/prompt_serving_usecase.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
📚 Learning: 2026-04-02T09:23:22.291Z
Learnt from: OrriMandarin
Repo: checkmarble/marble-backend PR: 1656
File: usecases/org_export_usecase.go:7-7
Timestamp: 2026-04-02T09:23:22.291Z
Learning: In checkmarble/marble-backend, treat `Priority` as unique only within a scenario: `Priority` must be unique per `(ScenarioId, Priority)` pair. When ordering workflows for a scenario, sort deterministically by `(ScenarioId, Priority)` (not by `Priority` alone). Because this uniqueness invariant makes `(ScenarioId, Priority)` comparisons unambiguous, the resulting ordering will remain deterministic even if the underlying sort implementation is unstable.
Applied to files:
usecases/ai_agent/ai_rule_assist.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.gousecases/prompt_serving_usecase.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
📚 Learning: 2026-04-13T15:48:04.390Z
Learnt from: OrriMandarin
Repo: checkmarble/marble-backend PR: 1678
File: usecases/org_import_usecase.go:426-433
Timestamp: 2026-04-13T15:48:04.390Z
Learning: In checkmarble/marble-backend, `models.LinkToSingle.Name` values are guaranteed to be unique within an organization. When building lookups of created links in Go (for example, `createdLinksByName`), it’s safe to use `link.Name` as the sole map key and you should not flag the code as a potential key-collision risk based on `Name` uniqueness.
Applied to files:
usecases/ai_agent/ai_rule_assist.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.gousecases/prompt_serving_usecase.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
📚 Learning: 2026-04-02T08:17:36.762Z
Learnt from: OrriMandarin
Repo: checkmarble/marble-backend PR: 1644
File: models/data_model.go:13-19
Timestamp: 2026-04-02T08:17:36.762Z
Learning: When creating tables via `CreateTableInput` in the marble-backend, ensure `object_id` (String, non-nullable) and `updated_at` (Timestamp, non-nullable) are treated as required, mandatory fields that the frontend must explicitly include in `CreateTableInput.Fields`. The backend should not auto-create these fields during table creation. Do not add `object_id`/`updated_at` to `DataModelReservedFieldNames` (since that blocks creation). Instead, require pre-request validation to confirm these fields are present in the incoming request; post-creation correctness is covered by `commonFieldValidation` in `usecases/data_model_semantic_validation.go`.
Applied to files:
usecases/ai_agent/ai_rule_assist.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gomodels/ai_agent_config.gomodels/ai_agent_prompts.gomodels/ai_agent_config_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.gomodels/ai_agent_prompts_fs_test.gousecases/prompt_serving_usecase.gomodels/ai_agent_prompts_fs.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
📚 Learning: 2026-05-11T11:36:03.794Z
Learnt from: Pascal-Delange
Repo: checkmarble/marble-backend PR: 1737
File: usecases/ai_agent/llm_errors.go:32-37
Timestamp: 2026-05-11T11:36:03.794Z
Learning: In checkmarble/marble-backend Go code, when you need to attach a sentinel error (used for downstream `errors.Is`) onto an existing error while preserving the original error chain, use `errors.Mark(originalErr, sentinelErr)` from `cockroachdb/errors` (e.g., `errors.Mark(err, models.LLMRateLimitedError)`). This keeps the original wrapped error(s) intact while making the sentinel detectable. Do not use `errors.Wrap(sentinelErr, err.Error())`, since it discards the original error chain and will prevent `errors.Is` from working as intended.
Applied to files:
usecases/ai_agent/ai_rule_assist.gopure_utils/prompt_version_test.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gopure_utils/prompt_version.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gocmd/server.gocmd/worker.gomodels/ai_agent_config.gomodels/ai_agent_prompts.gomodels/ai_agent_config_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.goinfra/ai_prompts.gomodels/ai_agent_prompts_fs_test.gousecases/prompt_serving_usecase.goinfra/ai_prompts_test.gomodels/ai_agent_prompts_fs.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
📚 Learning: 2026-05-20T15:29:22.314Z
Learnt from: apognu
Repo: checkmarble/marble-backend PR: 1719
File: usecases/screening_config_usecase.go:165-166
Timestamp: 2026-05-20T15:29:22.314Z
Learning: In Go 1.26+, the `new()` builtin accepts an expression (not just a type). In reviews, treat `new(expr)` as valid when the repo/tooling is using Go 1.26 or newer: it allocates a `*T` where `T` is the expression’s type, initializes the allocated value to `expr`, and returns the non-nil pointer to that initialized value. Therefore, patterns like `new(models.ScreeningProviderLexisNexis)` (where `ScreeningProviderLexisNexis` is a typed constant) should not be flagged as incorrect. If the codebase targets Go < 1.26, flag `new(expr)` as an error instead.
Applied to files:
usecases/ai_agent/ai_rule_assist.gopure_utils/prompt_version_test.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gopure_utils/prompt_version.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gocmd/server.gocmd/worker.gomodels/ai_agent_config.gomodels/ai_agent_prompts.gomodels/ai_agent_config_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.goinfra/ai_prompts.gomodels/ai_agent_prompts_fs_test.gousecases/prompt_serving_usecase.goinfra/ai_prompts_test.gomodels/ai_agent_prompts_fs.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
📚 Learning: 2026-05-20T15:29:22.314Z
Learnt from: apognu
Repo: checkmarble/marble-backend PR: 1719
File: usecases/screening_config_usecase.go:165-166
Timestamp: 2026-05-20T15:29:22.314Z
Learning: In Go 1.26+ (e.g., when the module is building with Go >= 1.26), recognize that `new(expr)` accepts an expression (not just a type). `new(expr)` allocates a new variable whose type is `T` (the expression’s type), initializes it to the value of `expr`, and returns `*T`—so constructs like `new(models.ScreeningProviderLexisNexis)` where `ScreeningProviderLexisNexis` is a typed constant will correctly return a `*models.ScreeningProvider` pointing to that constant’s value, not a zero-value pointer. Do not flag this as incorrect in Go 1.26+ code.
Applied to files:
usecases/ai_agent/ai_rule_assist.gopure_utils/prompt_version_test.gousecases/ai_agent/async_case_review.gousecases/ai_agent/async_screening_suggestion.gousecases/usecases_with_creds.gopure_utils/prompt_version.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gocmd/server.gocmd/worker.gomodels/ai_agent_config.gomodels/ai_agent_prompts.gomodels/ai_agent_config_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_agent_usecase_test.goinfra/ai_prompts.gomodels/ai_agent_prompts_fs_test.gousecases/prompt_serving_usecase.goinfra/ai_prompts_test.gomodels/ai_agent_prompts_fs.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_case_review.go
📚 Learning: 2026-03-03T17:21:35.986Z
Learnt from: Pascal-Delange
Repo: checkmarble/marble-backend PR: 1582
File: models/async_decision_execution.go:49-76
Timestamp: 2026-03-03T17:21:35.986Z
Learning: In Go projects, avoid adding MarshalJSON methods on model-level enums. Route all external serialization through a DTO layer that handles type conversions explicitly (e.g., using .String() for enums). This keeps JSON serialization centralized and consistent. Do not create or rely on model-level MarshalJSON for enums; implement or use the DTO conversions instead.
Applied to files:
models/ai_agent_config.gomodels/ai_agent_prompts.gomodels/ai_agent_config_test.gomodels/ai_agent_prompts_fs_test.gomodels/ai_agent_prompts_fs.go
🔇 Additional comments (22)
models/ai_agent_prompts.go (1)
1-51: LGTM!models/ai_agent_prompts_fs.go (1)
1-65: LGTM!models/ai_agent_prompts_fs_test.go (1)
1-60: LGTM!models/ai_agent_config_test.go (1)
1-58: LGTM!pure_utils/prompt_version.go (1)
18-33: LGTM!pure_utils/prompt_version_test.go (1)
1-52: LGTM!usecases/prompt_serving_usecase.go (1)
18-18: LGTM!Also applies to: 122-141
usecases/ai_agent/async_screening_suggestion.go (1)
10-10: LGTM!Also applies to: 34-54
cmd/worker.go (2)
366-376: 📐 Maintainability & Code Quality | ⚡ Quick winSame init/validate/warn block duplicated from
cmd/server.go.See the deduplication suggestion left on
cmd/server.go(lines 388-393) — same logic, same fix applies here.
398-398: LGTM!usecases/ai_agent/prompts_manifest.go (1)
1-42: LGTM!usecases/ai_agent/prompts_manifest_test.go (1)
1-63: LGTM! Solid coverage of the new validation branches — no notes, prithee proceed.cmd/server.go (1)
201-201: LGTM!usecases/usecases.go (1)
59-60: LGTM! Fits the existing functional-options pattern like a glove — well wired.Also applies to: 242-252, 281-282, 322-323, 607-607
usecases/ai_agent/ai_screening_suggestion.go (1)
19-24: LGTM!Also applies to: 100-100
usecases/usecases_with_creds.go (1)
815-841: LGTM!usecases/ai_agent/ai_agent_usecase.go (2)
445-458: Clean receiver-based rewrite — the FS injection reads true, like a well-forged key to a well-locked door.
readPromptcorrectly short-circuits on nilpromptsFSwithErrPromptsUnavailable, andpreparePromptnow sources content through the receiver. No issues spotted here.
528-545: LGTM!usecases/ai_agent/ai_case_review.go (2)
36-48: LGTM!
454-458: Sound fallback behavior preserved for non-critical prompts.Both
SYSTEM_PROMPT_PATHandINSTRUCTION_CUSTOM_REPORT_PATHdegrade gracefully to hardcoded defaults on read failure rather than aborting the whole case review — consistent with treating these as optional/soft prompts whilePROMPT_CASE_REVIEW_PATH(viapreparePromptWithModel) remains hard-required.Also applies to: 846-850
usecases/ai_agent/ai_enrichment.go (1)
19-23: LGTM!Also applies to: 147-151
usecases/ai_agent/ai_rule_assist.go (1)
18-23: LGTM!
OrriMandarin
force-pushed
the
feat/ai-prompts-client-loading
branch
2 times, most recently
from
e5a4c44 to
e08b3c1
Compare
| v, err := semver.NewVersion(version) | ||
| if err != nil { | ||
| logger.InfoContext(ctx, "non-semver version, skipping ai prompts download", "version", version, "error", err.Error()) | ||
| return nil | ||
| } | ||
| majorMinor := fmt.Sprintf("%d.%d", v.Major(), v.Minor()) | ||
|
|
There was a problem hiding this comment.
I have a question about the dev version. For the dev version, we won’t download any prompts. In this case, the dev version needs to have the prompt locally.
There was a problem hiding this comment.
Hihi,
The question was about the best approach to handle the case where the backend version is dev. Should we enforce the developer to use the AI_PROMPTS_EXECUTION_DIR variable to use the local prompt? or, should the dev backend be able to fetch prompts from the Marble Saas server? If so, which version of the server should it fetch prompts from?
Discussed with @Pascal-Delange, dev backend must use the AI_PROMPTS_EXECUTION_DIR variable, so it can't fetch prompt from our server
OrriMandarin
force-pushed
the
feat/ai-prompts-client-loading
branch
2 times, most recently
from
020a016 to
6d649d0
Compare
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
coderabbitai
Bot
added
M
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usecases/ai_agent/ai_agent_usecase_test.go (1)
19-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM — tests now exercise the injected fs.FS path.
Swapping
preparePrompt(promptPath, data)foruc.preparePrompt("test_prompt.txt", data)withpromptsFS: os.DirFS(tmpDir)correctly mirrors the new receiver-based contract across all six test functions.One gap worth a mention, though light on urgency: no test yet exercises
readPrompt's nil-promptsFSerror branch (Line 443 inai_agent_usecase.go). A quickAiAgentUsecase{}.preparePrompt(...)case asserting the error message would round out coverage of that guard.🤖 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 `@usecases/ai_agent/ai_agent_usecase_test.go` around lines 19 - 205, Add a test for the nil-promptsFS guard that is still uncovered. Create a case using AiAgentUsecase with promptsFS unset and call preparePrompt, then assert it returns the expected error from readPrompt; this will exercise the nil-fs branch that the current test suite misses. Use the existing preparePrompt and readPrompt symbols so the new test matches the receiver-based contract.pure_utils/prompt_version.go (1)
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the semver parse error with context.
return "", erron a badrequestedVersionloses the value that failed to parse — the rawsemvererror alone ("Invalid Semantic Version") won't tell a caller (or log) which input string was rejected. As per coding guidelines,**/*.go: "Wrap errors with context usinggithub.com/cockroachdb/errorspackage".♻️ Proposed fix
+import ( + "github.com/cockroachdb/errors" + "github.com/Masterminds/semver" +) + func ResolveBestPromptVersion(availableDirNames []string, requestedVersion string) (string, error) { reqV, err := semver.NewVersion(requestedVersion) if err != nil { - return "", err + return "", errors.Wrapf(err, "invalid requested version %q", requestedVersion) }🤖 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 `@pure_utils/prompt_version.go` around lines 13 - 17, The semver parse failure in ResolveBestPromptVersion currently returns the raw error without saying which requestedVersion was invalid. Update the error handling around semver.NewVersion so the returned error is wrapped with contextual information using github.com/cockroachdb/errors, including the requestedVersion value, so callers can identify the rejected input from the error message.Source: Coding guidelines
🤖 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 `@infra/ai_prompts.go`:
- Around line 51-58: The AI_PROMPTS_EXECUTION_DIR override check in aiPromptsFS
only verifies that the path exists, so a regular file can still pass and later
break os.DirFS usage. Update the existing os.Stat validation to also reject
non-directory paths before returning os.DirFS(dir), and keep the warning/logging
path in aiPromptsFS so invalid overrides are treated as unavailable.
- Line 106: The new error construction in ai_prompts.go still uses fmt.Errorf
instead of cockroachdb/errors, so update the return in the prompts version
lookup path to use errors.Newf with the same context and remove the now-unused
fmt import. Make this change in the code path that returns the “no ai prompts
version available” error so the file consistently uses the errors package for
wrapped/contextual errors.
---
Nitpick comments:
In `@pure_utils/prompt_version.go`:
- Around line 13-17: The semver parse failure in ResolveBestPromptVersion
currently returns the raw error without saying which requestedVersion was
invalid. Update the error handling around semver.NewVersion so the returned
error is wrapped with contextual information using
github.com/cockroachdb/errors, including the requestedVersion value, so callers
can identify the rejected input from the error message.
In `@usecases/ai_agent/ai_agent_usecase_test.go`:
- Around line 19-205: Add a test for the nil-promptsFS guard that is still
uncovered. Create a case using AiAgentUsecase with promptsFS unset and call
preparePrompt, then assert it returns the expected error from readPrompt; this
will exercise the nil-fs branch that the current test suite misses. Use the
existing preparePrompt and readPrompt symbols so the new test matches the
receiver-based contract.
🪄 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: f5ff5138-2bff-4ed7-ac21-eafa5ca0d214
📒 Files selected for processing (20)
cmd/server.gocmd/worker.goinfra/ai_prompts.goinfra/ai_prompts_test.gomodels/ai_agent_config.gomodels/ai_agent_prompts.gomodels/ai_agent_prompts_test.gopure_utils/prompt_version.gopure_utils/prompt_version_test.gousecases/ai_agent/ai_agent_usecase.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_enrichment.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/prompts_manifest_test.gousecases/prompt_serving_usecase.gousecases/usecases.gousecases/usecases_with_creds.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: test_backend / Test back-end
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Wrap errors with context usinggithub.com/cockroachdb/errorspackage
Use custom error types frommodels/errors.go(BadParameterError, NotFoundError, ForbiddenError, ConflictError, etc.) for domain-specific errors
Uselog/slogstructured logging with context, providing relevant key-value pairs for debugging
Use the functional options pattern for dependency injection when initializing repositories, usecases, and feature-specific components
Files:
usecases/usecases_with_creds.gopure_utils/prompt_version.gousecases/prompt_serving_usecase.gomodels/ai_agent_prompts_test.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gopure_utils/prompt_version_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gocmd/worker.gomodels/ai_agent_prompts.gousecases/usecases.goinfra/ai_prompts.gousecases/ai_agent/ai_agent_usecase.gomodels/ai_agent_config.goinfra/ai_prompts_test.gocmd/server.go
usecases/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
usecases/**/*.go: Usecredentials.Get(ctx)to extract organization context and user permissions from the request context in usecases
Feature-specific usecases should be created via factory methods likeNewCaseUsecase(),NewDecisionUsecase()for improved modularity
Files:
usecases/usecases_with_creds.gousecases/prompt_serving_usecase.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.go
usecases/*_usecase.go
📄 CodeRabbit inference engine (CLAUDE.md)
usecases/*_usecase.go: In usecases, validate that all resource IDs from user input (request body, query params, URL params) belong to the caller's organization usingenforceSecuritychecks or direct org comparison
Fetch referenced resources by ID in usecases and verify they belong to the caller's organization before using them; do not rely on repositoryGet*ByIdmethods to filter by organization
Files:
usecases/prompt_serving_usecase.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use
testify/assertandtestify/requirefor assertions in unit tests
Files:
models/ai_agent_prompts_test.gousecases/ai_agent/prompts_manifest_test.gopure_utils/prompt_version_test.gousecases/ai_agent/ai_agent_usecase_test.goinfra/ai_prompts_test.go
{cmd/worker.go,models/river_job.go,jobs/**/*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Register new River job worker implementations in
cmd/worker.goafter defining the job args struct inmodels/river_job.go
Files:
cmd/worker.go
🧠 Learnings (8)
📚 Learning: 2026-02-27T16:10:37.065Z
Learnt from: apognu
Repo: checkmarble/marble-backend PR: 1558
File: usecases/decision_usecase.go:419-430
Timestamp: 2026-02-27T16:10:37.065Z
Learning: In usecases/decision_usecase.go, payload.Data["object_id"] is asserted to string with the assumption that earlier payload validation guarantees a string. This is actionable: review the file for all such type assertions on payload.Data fields and ensure there is prior validation confirming the type. Prefer 1) keeping the constraint explicit in the validation layer, 2) using a typed struct if possible to avoid map[string]interface{} assertions, and 3) using a type assertion with a safe, checked path (e.g., v, ok := payload.Data["object_id"].(string)) where appropriate. Apply this guidance to all similar files in the usecases directory (and nested, if present) to ensure consistency and avoid runtime panics due to unexpected types.
Applied to files:
usecases/usecases_with_creds.gousecases/prompt_serving_usecase.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.go
📚 Learning: 2026-04-02T09:23:22.291Z
Learnt from: OrriMandarin
Repo: checkmarble/marble-backend PR: 1656
File: usecases/org_export_usecase.go:7-7
Timestamp: 2026-04-02T09:23:22.291Z
Learning: In checkmarble/marble-backend, treat `Priority` as unique only within a scenario: `Priority` must be unique per `(ScenarioId, Priority)` pair. When ordering workflows for a scenario, sort deterministically by `(ScenarioId, Priority)` (not by `Priority` alone). Because this uniqueness invariant makes `(ScenarioId, Priority)` comparisons unambiguous, the resulting ordering will remain deterministic even if the underlying sort implementation is unstable.
Applied to files:
usecases/usecases_with_creds.gousecases/prompt_serving_usecase.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.go
📚 Learning: 2026-04-13T15:48:04.390Z
Learnt from: OrriMandarin
Repo: checkmarble/marble-backend PR: 1678
File: usecases/org_import_usecase.go:426-433
Timestamp: 2026-04-13T15:48:04.390Z
Learning: In checkmarble/marble-backend, `models.LinkToSingle.Name` values are guaranteed to be unique within an organization. When building lookups of created links in Go (for example, `createdLinksByName`), it’s safe to use `link.Name` as the sole map key and you should not flag the code as a potential key-collision risk based on `Name` uniqueness.
Applied to files:
usecases/usecases_with_creds.gousecases/prompt_serving_usecase.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.go
📚 Learning: 2026-04-02T08:17:36.762Z
Learnt from: OrriMandarin
Repo: checkmarble/marble-backend PR: 1644
File: models/data_model.go:13-19
Timestamp: 2026-04-02T08:17:36.762Z
Learning: When creating tables via `CreateTableInput` in the marble-backend, ensure `object_id` (String, non-nullable) and `updated_at` (Timestamp, non-nullable) are treated as required, mandatory fields that the frontend must explicitly include in `CreateTableInput.Fields`. The backend should not auto-create these fields during table creation. Do not add `object_id`/`updated_at` to `DataModelReservedFieldNames` (since that blocks creation). Instead, require pre-request validation to confirm these fields are present in the incoming request; post-creation correctness is covered by `commonFieldValidation` in `usecases/data_model_semantic_validation.go`.
Applied to files:
usecases/usecases_with_creds.gousecases/prompt_serving_usecase.gomodels/ai_agent_prompts_test.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gomodels/ai_agent_prompts.gousecases/usecases.gousecases/ai_agent/ai_agent_usecase.gomodels/ai_agent_config.go
📚 Learning: 2026-05-11T11:36:03.794Z
Learnt from: Pascal-Delange
Repo: checkmarble/marble-backend PR: 1737
File: usecases/ai_agent/llm_errors.go:32-37
Timestamp: 2026-05-11T11:36:03.794Z
Learning: In checkmarble/marble-backend Go code, when you need to attach a sentinel error (used for downstream `errors.Is`) onto an existing error while preserving the original error chain, use `errors.Mark(originalErr, sentinelErr)` from `cockroachdb/errors` (e.g., `errors.Mark(err, models.LLMRateLimitedError)`). This keeps the original wrapped error(s) intact while making the sentinel detectable. Do not use `errors.Wrap(sentinelErr, err.Error())`, since it discards the original error chain and will prevent `errors.Is` from working as intended.
Applied to files:
usecases/usecases_with_creds.gopure_utils/prompt_version.gousecases/prompt_serving_usecase.gomodels/ai_agent_prompts_test.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gopure_utils/prompt_version_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gocmd/worker.gomodels/ai_agent_prompts.gousecases/usecases.goinfra/ai_prompts.gousecases/ai_agent/ai_agent_usecase.gomodels/ai_agent_config.goinfra/ai_prompts_test.gocmd/server.go
📚 Learning: 2026-05-20T15:29:22.314Z
Learnt from: apognu
Repo: checkmarble/marble-backend PR: 1719
File: usecases/screening_config_usecase.go:165-166
Timestamp: 2026-05-20T15:29:22.314Z
Learning: In Go 1.26+, the `new()` builtin accepts an expression (not just a type). In reviews, treat `new(expr)` as valid when the repo/tooling is using Go 1.26 or newer: it allocates a `*T` where `T` is the expression’s type, initializes the allocated value to `expr`, and returns the non-nil pointer to that initialized value. Therefore, patterns like `new(models.ScreeningProviderLexisNexis)` (where `ScreeningProviderLexisNexis` is a typed constant) should not be flagged as incorrect. If the codebase targets Go < 1.26, flag `new(expr)` as an error instead.
Applied to files:
usecases/usecases_with_creds.gopure_utils/prompt_version.gousecases/prompt_serving_usecase.gomodels/ai_agent_prompts_test.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gopure_utils/prompt_version_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gocmd/worker.gomodels/ai_agent_prompts.gousecases/usecases.goinfra/ai_prompts.gousecases/ai_agent/ai_agent_usecase.gomodels/ai_agent_config.goinfra/ai_prompts_test.gocmd/server.go
📚 Learning: 2026-05-20T15:29:22.314Z
Learnt from: apognu
Repo: checkmarble/marble-backend PR: 1719
File: usecases/screening_config_usecase.go:165-166
Timestamp: 2026-05-20T15:29:22.314Z
Learning: In Go 1.26+ (e.g., when the module is building with Go >= 1.26), recognize that `new(expr)` accepts an expression (not just a type). `new(expr)` allocates a new variable whose type is `T` (the expression’s type), initializes it to the value of `expr`, and returns `*T`—so constructs like `new(models.ScreeningProviderLexisNexis)` where `ScreeningProviderLexisNexis` is a typed constant will correctly return a `*models.ScreeningProvider` pointing to that constant’s value, not a zero-value pointer. Do not flag this as incorrect in Go 1.26+ code.
Applied to files:
usecases/usecases_with_creds.gopure_utils/prompt_version.gousecases/prompt_serving_usecase.gomodels/ai_agent_prompts_test.gousecases/ai_agent/prompts_manifest_test.gousecases/ai_agent/prompts_manifest.gopure_utils/prompt_version_test.gousecases/ai_agent/ai_screening_suggestion.gousecases/ai_agent/ai_rule_assist.gousecases/ai_agent/ai_case_review.gousecases/ai_agent/ai_agent_usecase_test.gousecases/ai_agent/ai_enrichment.gocmd/worker.gomodels/ai_agent_prompts.gousecases/usecases.goinfra/ai_prompts.gousecases/ai_agent/ai_agent_usecase.gomodels/ai_agent_config.goinfra/ai_prompts_test.gocmd/server.go
📚 Learning: 2026-03-03T17:21:35.986Z
Learnt from: Pascal-Delange
Repo: checkmarble/marble-backend PR: 1582
File: models/async_decision_execution.go:49-76
Timestamp: 2026-03-03T17:21:35.986Z
Learning: In Go projects, avoid adding MarshalJSON methods on model-level enums. Route all external serialization through a DTO layer that handles type conversions explicitly (e.g., using .String() for enums). This keeps JSON serialization centralized and consistent. Do not create or rely on model-level MarshalJSON for enums; implement or use the DTO conversions instead.
Applied to files:
models/ai_agent_prompts_test.gomodels/ai_agent_prompts.gomodels/ai_agent_config.go
🔇 Additional comments (31)
usecases/ai_agent/ai_agent_usecase.go (3)
439-452: LGTM on the fs.FS migration.
readPromptcleanly guards against a nilpromptsFSand wrapsfs.ReadFileerrors with context. One small thought worth a nod: were the play to lose its script (promptsFSnil), every prompt read fails hard here — by design for template content, unlike the softer fallback used for system instructions in callers. That asymmetry is intentional based on the call sites reviewed, so no action needed.
522-533: LGTM.
preparePromptWithModelcorrectly threadsuc.promptsFSintomodels.LoadAiAgentModelConfig, which already handles the nil-fs fallback per its documented contract.
171-254: LGTM on struct field and constructor wiring.
promptsFS fs.FSaddition to the struct and constructor is straightforward and consistent with the rest of the dependency list.usecases/ai_agent/ai_case_review.go (2)
37-46: LGTM.Constants correctly re-point to the shared
models.AiAgentPrompt*Pathmanifest rather than hardcoded literals.
452-452: LGTM — mechanical switch to receiver-based prompt reads.Both
uc.readPrompt(SYSTEM_PROMPT_PATH)anduc.readPrompt(INSTRUCTION_CUSTOM_REPORT_PATH)preserve the existing debug-log-and-fallback pattern, so behavior on missing/unavailable prompts remains unchanged aside from the new fs.FS source.Also applies to: 844-844
usecases/ai_agent/ai_enrichment.go (2)
19-21: LGTM.Constants correctly reference
models.AiAgentKYCInstructionPath/models.AiAgentKYCPromptEnrichPath.
145-149: 🎯 Functional CorrectnessLGTM, though note the fallback asymmetry.
Unlike system-prompt reads elsewhere that fall back to a hardcoded string on error,
uc.preparePrompt(INSTRUCTION_PATH, ...)here returns a hard error viaerrors.Wrap(err, "failed to read instruction")if the prompt is missing — no soft fallback text like the case-review/screening-suggestion system prompts have. Worth confirming this fail-hard behavior for the KYC instruction is intentional, sincepromptsFSunavailability (e.g., beforeinfra.InitAiPromptsFSresolves, or on network-download failure) would now abort enrichment entirely rather than degrade gracefully.usecases/ai_agent/ai_rule_assist.go (1)
19-21: LGTM.Constants correctly re-point to
models.AiAgentRuleDescriptionPromptPath,models.AiAgentRuleGenerationStep1PromptPath, andmodels.AiAgentRuleGenerationStep2PromptPath.usecases/ai_agent/ai_screening_suggestion.go (2)
20-21: LGTM.Constants correctly re-point to the shared manifest paths.
97-101: LGTM.
uc.readPrompt(SCREENING_HIT_SYSTEM_PROMPT_PATH)preserves the same debug-log-and-fallback behavior as before, just sourced from the injected fs. All's well that reads well.infra/ai_prompts.go (5)
162-166: Duplicate: 429 remains unrecoverable.Already flagged on the previous commit: Line 162 retries only 5xx, so
429 Too Many Requestsstill falls intoretry.Unrecoverableon Line 166, despite the stated 5xx/429 retry contract.
25-47: LGTM!
61-83: LGTM!
111-160: LGTM!Also applies to: 169-193
195-227: LGTM!infra/ai_prompts_test.go (3)
174-207: Duplicate: add the 429 retry regression.Already flagged on the previous commit: the suite proves 5xx retries and 403 does not, but still lacks the required
429 Too Many Requestsretry case. A small test here would keep that rate-limit dragon from returning.
22-172: LGTM!
209-312: LGTM!models/ai_agent_config.go (1)
22-40: 🗄️ Data Integrity & IntegrationConfirm hard failure is intended when a prompt FS is present but the model config file is missing.
promptsFS == nilgracefully returns defaults, but once a non-nil FS is injected, a missingAiAgentModelConfigFileName(which is part ofAiAgentExpectedFiles, permodels/ai_agent_prompts.go) now hard-fails instead of falling back. Sinceai_agent.ValidatePromptsFSonly warns (doesn't block) on missing manifest files at startup, a resolved-but-incomplete prompts FS could sail through startup validation only to have this call error out at request time. Worth confirming this fail-fast-per-call behavior (vs. falling back to defaults like the nil case) is the intended contract between validation and loading.models/ai_agent_prompts.go (1)
67-116: LGTM!models/ai_agent_prompts_test.go (1)
11-60: LGTM!pure_utils/prompt_version.go (1)
19-33: LGTM on the backward-search selection logic itself — verified against Masterminds/semver's documented "v" prefix and partial-version coercion, and it matches the test expectations exactly.pure_utils/prompt_version_test.go (1)
10-51: LGTM!usecases/prompt_serving_usecase.go (1)
122-140: Good delegation to the shared resolver — removes duplicate semver-walking logic, and the error-mapping tomodels.NotFoundError/models.BadParameterErrorlines up withTest_PromptServingUsecase_DownloadPrompts_VersionResolution. As the comment on lines 129-131 rightly notes (in fittingly self-aware prose), theerrpath here is effectively unreachable but kept as a harmless defensive check — no notes.usecases/ai_agent/prompts_manifest.go (1)
11-26: LGTM!usecases/ai_agent/prompts_manifest_test.go (1)
22-53: LGTM! Good coverage of the validation contract — fair tests, well wrought, like a sonnet with no missing feet.cmd/server.go (1)
202-202: LGTM! The license + entitlement guard before touching the network/serving dir is the right call here.Also applies to: 389-397, 421-422
usecases/usecases.go (1)
59-60: LGTM! Field/option refactor is consistent end-to-end within this file.Also applies to: 242-252, 281-283, 322-323, 607-607
usecases/usecases_with_creds.go (1)
815-841: LGTM!cmd/worker.go (2)
367-368: 🗄️ Data Integrity & IntegrationNo
WithAIPromptsServingDirwiring needed incmd/worker.go. The worker path does not reachNewPromptServingUsecase(), so the emptyaiPromptsServingDirhere has no effect.> Likely an incorrect or invalid review comment.
367-376: 🔒 Security & PrivacyDrop this:
cmd/worker.godoesn’t needWithAIPromptsServingDir, and the AI-prompts download path already gates on license validation before serving anything.> Likely an incorrect or invalid review comment.
OrriMandarin
force-pushed
the
feat/ai-prompts-client-loading
branch
3 times, most recently
from
bd75a2d to
0898552
Compare
feat: ai_agent reads prompts from an injectable fs.FS (self-hosted download support)
Replaces hardcoded os.Open("prompts/...") reads in usecases/ai_agent with a single
injected fs.FS, resolved at process startup from one of three tiers: an explicit local
execution-dir override, in-process resolution against a locally-mounted prompts bucket
(no network, avoids a cold-start self-call deadlock on Marble SaaS), or a license-gated
network download from the Phase A serving endpoint (PR #1841). A downloaded bundle is
verified against a deterministic manifest of every prompt path the code actually reads
before it's accepted, and the same manifest is checked against the resolved fs.FS once
at server/worker startup so a missing or corrupt prompt surfaces immediately rather than
mid-job.
chore: add timeout when requesting the prompt
fix: linting
avoid download the prompt if the license doesn't allow Case Ai assist
better retry management
If validation failed, remove prompts FS
OrriMandarin
force-pushed
the
feat/ai-prompts-client-loading
branch
from
0898552 to
cf380e4
Compare
|
For staging, need to push the |
|
Wait for https://github.com/checkmarble/terraform/pull/164 deployment on production and staging |
2 participants
Summary
Phase B of self-hosted AI prompt distribution (Phase A: the SaaS-side download-serving endpoint, PR #1841). This makes the reading side of
usecases/ai_agent/*use an injectedfs.FSinstead of hardcodedos.Open("prompts/...")calls relative to the process working directory.infra.InitAiPromptsFSresolves the prompts filesystem once at startup, in priority order:AI_PROMPTS_EXECUTION_DIRenv var, if set — used as-is, no resolution, no network (staging's mechanism, pointed at a mounted bucket's stablemain/folder).AI_PROMPTS_SERVING_DIR(the existing serving-dir config, reused) — if this instance already has the private prompts bucket mounted locally (production's case), the rightvX.Yfolder is resolved in-process against its own detected version, with no network call. This avoids production calling its own download endpoint on a cold start, which would deadlock.avast/retry-go, matching the existing convention) on network errors and 5xx responses.cmd/server.goandcmd/worker.goonly run this resolution (and the startup validation below) whenlicense.CaseAiAssistis true — a deployment without the AI entitlement never attempts a local lookup or a network call, and never logs a "prompts unavailable" warning for a feature it isn't licensed for.A deterministic manifest (
models.AiAgentExpectedFiles, single source of truth for every prompt path the code reads) drives two verification steps:retry.Unrecoverable), since that's a content mismatch (wrong version, stale manifest) no retry will fix.ai_agent.ValidatePromptsFSchecks the resolvedfs.FS(whichever tier produced it) against the same manifest, logging a warning immediately if something's missing, instead of surfacing it later as a per-job failure.The downloaded bundle is held entirely in memory via a small map-backed
fs.FS(models.AiAgentPromptsMapFS) — never written to disk.If no prompts filesystem is available, worker jobs (
async_case_review.go,async_screening_suggestion.go) return the error and let River's normal retry policy handle it, rather than cancelling the job outright — a worker restarted with prompts correctly resolved can still make an already-queued job succeed on a later attempt.Test plan
go build ./...,go vet ./...,golangci-lint run ./...all cleango test ./...passes across the whole repo🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes