feat: add skills repository data model and config store support - #4229
Conversation
|
|
|
Warning Review limit reached
More reviews will be available in 41 minutes and 50 seconds. Learn how PR review limits work. To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a skills repository: DB models and migrations, ConfigStore API extensions, validation and semver governance, object-store vs inline blob plumbing with post‑commit uploads and compensation, full CRUD/version/serve flows, orphan-blob cleanup, tests, and repository review-config updates. ChangesSkills Repository Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant RDBConfigStore
participant PostgresDB
participant ObjectStore
Client->>RDBConfigStore: CreateSkill(skill, version, files)
RDBConfigStore->>PostgresDB: BeginTx + insert skill/version/files
PostgresDB-->>RDBConfigStore: Commit
RDBConfigStore->>ObjectStore: Upload deferred file objects (post-commit)
ObjectStore-->>RDBConfigStore: Upload result / error
RDBConfigStore->>PostgresDB: Compensating DB delete on upload failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Confidence Score: 5/5Safe to merge; the three-phase update design, compensating deletes, and FOR UPDATE serialization are sound, and the only open issues are minor ordering/contract nits. The migration is idempotent, the rollback drop order is correct (dependent skill_files dropped first), and the concurrency patterns throughout use proper locking. The two open comments are non-blocking: a missing ORDER BY tiebreaker observable only under simultaneous version creation with identical timestamps, and an implicit caller contract in the serve=true response path that fails loudly rather than silently if violated. framework/configstore/skills.go — latestCreatedSkillVersion tiebreaker and UpdateSkill Phase 1 serve=true populateSkillFiles contract. Important Files Changed
Reviews (24): Last reviewed commit: "feat: add skills repository data model a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@framework/configstore/migrations.go`:
- Around line 6607-6616: The rollback in the Rollback function reverses table
drops in an order that violates FK constraints (it calls
mg.DropTable(&tables.TableSkillFileBlob{}) before dropping
&tables.TableSkillFile{}), which can cause failures; change the drop order to
drop &tables.TableSkillFile{} first, then &tables.TableSkillFileBlob{}, and then
&tables.TableSkillVersion{} (or otherwise order drops to respect foreign key
dependencies) so that tables.TableSkillFile is removed before
tables.TableSkillFileBlob in the Rollback function.
In `@framework/configstore/rdb_test.go`:
- Around line 68-92: Add the suggested edge-case table-driven tests to
TestValidateSkillVersionIncrementRequiresGreaterVersion: include cases for an
empty previousVersion (first release) with latest == "" and next == "1.0.0"
expecting no error, an exact duplicate case latest == "1.0.3-beta1" next ==
"1.0.3-beta1" expecting an error, a malformed version case next ==
"not-a-version" expecting an error, and a major increment case latest == "1.0.3"
next == "2.0.0" expecting no error; keep these as additional entries in the
tests slice so validateSkillVersionIncrement is exercised for these edge
conditions.
- Around line 94-143: The field name HighestVersion is ambiguous; add a clear
inline comment on the TableSkill struct next to HighestVersion (e.g., "//
HighestVersion = most recently created version, not the highest semver") to
document the semantic, and if you prefer a stronger change, rename
HighestVersion to LatestCreatedVersion (or MostRecentVersion) and update all
references (latestCreatedSkillVersion, store.GetSkillLean, tests such as
TestLatestCreatedSkillVersionUsesCreationOrder and any DB mappings) to maintain
compilation and behavior.
In `@framework/configstore/skills.go`:
- Around line 177-196: validateSkillVersionIncrement currently only checks
major.minor.patch and treats suffixes as equal only when identical, allowing
regressive prerelease changes; update it to compare prerelease/suffix using
semver precedence after parsing (in the same function that calls
parseSkillSemver) so that when major/minor/patch are equal you compute the
semver precedence of next versus prev (treat empty suffix/release as higher than
any prerelease) and return an error if next is not strictly greater than prev;
use the existing symbols validateSkillVersionIncrement, parseSkillSemver, and
the parsed fields (next.suffix, prev.suffix, next.major/minor/patch) to
implement the comparison and keep the exact-equality check for fully identical
versions.
- Around line 902-916: CreateSkillFileBlob currently creates unreferenced blobs
that CleanupOrphanSkillFileBlobs will immediately delete; change the cleanup to
avoid removing recent uploads by adding a grace-window or explicit state: either
(A) ensure TableSkillFileBlob has a created_at timestamp (or use existing one)
and modify CleanupOrphanSkillFileBlobs to only delete blobs where NOT
EXISTS(...) AND skill_file_blobs.created_at < now() - GRACE_PERIOD (e.g. 10min),
or (B) add a status/attached flag on TableSkillFileBlob set to "pending" in
CreateSkillFileBlob and only delete blobs where status = "pending" AND
created_at older than GRACE_PERIOD; update CreateSkillFileBlob to set the
timestamp/status and update the CleanupOrphanSkillFileBlobs query to filter by
that field instead of deleting all unreferenced rows.
- Around line 295-307: The object-store mutations (objectStore.Put in
StoreSkillFileContent and DeleteBatch used elsewhere) are being executed inside
the DB transaction callback which can cause divergence and long-held locks;
modify StoreSkillFileContent (and the code around DeleteBatch at the other
location) so the transaction only updates/persists DB state (e.g., save the
storage key and metadata) and does not call objectStore.Put/DeleteBatch inside
the tx callback — instead capture the necessary post-commit work (storage key,
file ID, operation type) and perform the object-store Put/Delete after the
transaction successfully commits (either immediately after tx.Commit or via an
outbox/post-commit handler/async GC job), ensuring the DB commit happens first
and network I/O runs outside the transaction.
- Around line 366-396: In the SkillSourceTypeText, SkillSourceTypeDataURL and
SkillSourceTypeUpload branches, reject any non-nil file.StorageKey or
file.BlobID that refer to foreign/unnamed resources by adding ownership
validation before accepting/persisting them: call or implement a helper like
validateStorageKeyOwner(storageKey, expectedPrefix) and
validateBlobOwner(blobID, expectedOwner) (or perform a DB lookup on blob rows)
and return an error if the key/blob is not owned by this skill/tenant; only
clear/accept storageKey/blobID after those checks (update the same branches
around file.StorageKey / file.BlobID handling and use decodeSkillDataURL as
before for data URL parsing).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd14d42c-d52d-44aa-950d-522f4cd29888
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
fdcd6f6 to
4c8e557
Compare
bded229 to
93ef5fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@framework/configstore/migrations.go`:
- Around line 6607-6622: The Rollback implementation in the migration currently
unconditionally drops tables (see Rollback function and references to
tables.TableSkillFileBlob, tables.TableSkillFile, tables.TableSkillVersion,
tables.TableSkill) which will destroy pre-existing data on upgraded DBs; change
this to either make the migration forward-only by removing or disabling the
Rollback body (marking it non-rollbackable per guidelines) or implement a safe
conditional rollback that only drops tables you created (e.g., check for a
migration-created marker or existence of schema version before dropping). Update
the migration metadata to explicitly flag it as non-rollbackable if you choose
the forward-only approach.
In `@framework/configstore/skills.go`:
- Around line 472-478: GetSkillLean currently swallows errors from
latestCreatedSkillVersion causing HighestVersion to be missing; change the code
to check the error returned by latestCreatedSkillVersion (called with
s.ScopedDB(ctx), skill.ID) and propagate it instead of ignoring it: if latest,
err := latestCreatedSkillVersion(...); err != nil { return nil,
fmt.Errorf("lookup latest skill version: %w", err) } else if latest != "" {
skill.HighestVersion = latest } so DB failures are returned to the caller rather
than silently dropped.
- Around line 792-798: populateSkillFiles currently preloads only "Files" so
callers like CreateSkill, UpdateSkill, and GetSkillLean may receive skill.Files
without the Blob payload when DB blob fallback is used; modify
populateSkillFiles to Preload "Files.Blob" (i.e., call
tx.Preload("Files").Preload("Files.Blob") or equivalent) when loading the
TableSkillVersion in populateSkillFiles so that skill.Files includes the Blob
data before assigning to skill.Files.
In `@framework/configstore/store.go`:
- Around line 548-561: Create a new SkillsStore interface containing the listed
skill methods (CreateSkill, GetSkill, GetSkillLean, GetSkillByName,
GetSkillVersion, ListSkillVersions, UpdateSkill, DeleteSkill, ListSkills,
ShiftSkillVersion, GetAllSkillsVersion, CreateSkillFileBlob,
CleanupOrphanSkillFileBlobs) and add a Skills() SkillsStore accessor to
ConfigStore; then keep the existing exported skill methods on ConfigStore but
implement them as thin forwarders that call through to c.Skills() so existing
implementations/tests (e.g., MockConfigStore) do not break while new code can
use the dedicated SkillsStore abstraction.
In `@framework/configstore/tables/skills.go`:
- Around line 62-80: scanSkillJSON currently returns early for nil/empty DB JSON
without clearing the provided dest, which can leave stale map/slice data; modify
scanSkillJSON so that when value is nil or when data length is 0 it resets dest
to its zero value before returning (handle pointer-to-map, pointer-to-slice,
etc.), e.g. use reflection to detect that dest is a non-nil pointer and set its
Elem() to its zero value, then return nil; keep the existing behavior for
unsupported types and successful json.Unmarshal.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3dd5fe53-6e12-48ea-bd36-4b98a2ec6b72
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
93ef5fd to
7296b12
Compare
4c8e557 to
2a806eb
Compare
7296b12 to
7971f41
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@framework/configstore/migrations.go`:
- Around line 6568-6569: The current HasTable guard only checks for the base
table and can skip applying missing indexes/constraints on retry; replace the
simple HasTable short-circuit with a migration that ensures full schema is
present: for each skills-related model referenced in this file (e.g., the Skill
model and any repo-related models), if db.Migrator().HasTable(...) is true still
call db.AutoMigrate(&Skill{}) (or run explicit
Migrator().CreateIndex/CreateConstraint where needed) so missing DDL is applied,
or alternatively detect specific missing pieces via
db.Migrator().HasIndex/HasConstraint/HasColumn and apply them; update the code
paths around HasTable and the block guarded by it (lines referencing HasTable in
this diff) to always reconcile schema rather than skipping when the table
exists.
In `@framework/configstore/skills.go`:
- Around line 35-45: The runPendingSkillObjectWrites function currently fires
off a goroutine to perform objectStore.Put and returns immediately, which can
leave DB rows referencing missing objects; change this to a durable, bounded,
and error-aware flow: either perform the Put synchronously before returning from
CreateSkill/UpdateSkill (call runPendingSkillObjectWrites synchronously and
propagate Put errors) or implement an explicit outbox/pending state with retry
and acknowledgement (persist pendingSkillObjectWrite rows, have a bounded worker
that reads them and retries Put with backoff and marks them completed), ensure
you respect ctx cancellation and limit concurrency, and surface any
non-transient failures back to the caller instead of only logging in
runPendingSkillObjectWrites and objectStore.Put.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 21e502b6-7f93-4935-8c72-63ee8953126d
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
7971f41 to
23e726b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
framework/configstore/skills.go (1)
35-43:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPost-commit object write failures still return success.
CreateSkill/UpdateSkillcommitskill_files.storage_keyrows before this helper runs, but this helper only logsPutfailures. A canceled request context or transient object-store error therefore returns success while leaving committed file rows pointing at objects that were never stored.As per coding guidelines, framework changes should use explicit error handling and bounded resource usage.
🤖 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 `@framework/configstore/skills.go` around lines 35 - 43, runPendingSkillObjectWrites currently swallows Put errors (only logs them) so CreateSkill/UpdateSkill can commit skill_files.storage_key rows even when objectStore.Put fails; change runPendingSkillObjectWrites to return an error (e.g., error from objectStore.Put or an aggregated error) instead of only logging, propagate that error back to callers (CreateSkill/UpdateSkill) and make callers abort the commit or perform compensating cleanup (delete committed DB rows) on error; ensure the function respects the provided ctx and bounds resource usage (avoid unbounded goroutines or retries) and reference the function name runPendingSkillObjectWrites and the objectStore.Put call when updating code.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 `@framework/configstore/skills.go`:
- Around line 61-74: SkillReservedFrontmatterFields is exported as a mutable
global map which allows external mutation and can cause concurrent map
read/write panics; change the backing map to an unexported variable (e.g.,
skillReservedFrontmatterFields) and replace the exported map with an access
helper that returns a defensive copy or a read-only query function (e.g.,
GetSkillReservedFrontmatterFields() returning a copied map or
IsSkillReservedFrontmatterField(key) bool). Update callers to use the helper so
external packages cannot mutate the original map and concurrent reads are safe.
- Around line 194-202: parseSkillSemver currently ignores strconv.Atoi errors,
so huge numeric components overflow to zero; update parseSkillSemver to validate
each numeric component conversion and return an error on overflow/parse failure:
after matching with skillSemverPattern, convert matches[1], matches[2],
matches[3] using strconv.ParseInt (or strconv.Atoi) but check and propagate any
error instead of discarding it, and only construct and return the
skillVersionParts on success (preserving suffix from matches[4]); this will
ensure ValidateSkillVersion and ordering/bump logic see correct errors for
values like 999999999999999999999.
---
Duplicate comments:
In `@framework/configstore/skills.go`:
- Around line 35-43: runPendingSkillObjectWrites currently swallows Put errors
(only logs them) so CreateSkill/UpdateSkill can commit skill_files.storage_key
rows even when objectStore.Put fails; change runPendingSkillObjectWrites to
return an error (e.g., error from objectStore.Put or an aggregated error)
instead of only logging, propagate that error back to callers
(CreateSkill/UpdateSkill) and make callers abort the commit or perform
compensating cleanup (delete committed DB rows) on error; ensure the function
respects the provided ctx and bounds resource usage (avoid unbounded goroutines
or retries) and reference the function name runPendingSkillObjectWrites and the
objectStore.Put call when updating code.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0bf2fd9d-94ce-4f10-988b-c318b406e0dd
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
23e726b to
7fdc066
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
framework/configstore/skills.go (3)
430-444:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the blob-size limit before decode/insert, not after.
decodeSkillDataURLmaterializes the full base64 payload beforevalidateSkillFileSourcechecksmaxSkillFileContentSize, so a very largedata:URL can still spike memory before rejection.CreateSkillFileBlobalso inserts arbitraryblob.Datawith no size gate at all. The size bound needs to be enforced at ingress for both paths.Suggested fix
func decodeSkillDataURL(dataURL string) ([]byte, string, error) { parsed, err := url.Parse(dataURL) if err != nil || parsed.Scheme != "data" { return nil, "", fmt.Errorf("dataurl must be a valid data: URL") } parts := strings.SplitN(dataURL, ",", 2) if len(parts) != 2 || !strings.Contains(parts[0], ";base64") { return nil, "", fmt.Errorf("dataurl must contain ;base64,") } + if len(parts[1]) > base64.StdEncoding.EncodedLen(maxSkillFileContentSize) { + return nil, "", fmt.Errorf("dataurl file content exceeds maximum size of %d bytes", maxSkillFileContentSize) + } mimeType := strings.TrimPrefix(strings.Split(parts[0], ";")[0], "data:") data, err := base64.StdEncoding.DecodeString(parts[1]) if err != nil { return nil, "", fmt.Errorf("dataurl base64 decode failed: %w", err) } return data, mimeType, nil } func (s *RDBConfigStore) CreateSkillFileBlob(ctx context.Context, blob *tables.TableSkillFileBlob) error { + if blob == nil { + return fmt.Errorf("skill file blob is required") + } + if len(blob.Data) > maxSkillFileContentSize { + return fmt.Errorf("blob content exceeds maximum size of %d bytes", maxSkillFileContentSize) + } return s.DB().WithContext(ctx).Create(blob).Error }As per coding guidelines, framework changes must enforce size limits and bounded resource usage.
Also applies to: 491-497, 1041-1044
🤖 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 `@framework/configstore/skills.go` around lines 430 - 444, The function decodeSkillDataURL currently decodes the entire base64 payload before size checks and CreateSkillFileBlob inserts blob.Data without gating size; update decodeSkillDataURL to parse the data: URL, determine the base64 payload length (or decode in a streaming/limited manner) and reject when the decoded size would exceed maxSkillFileContentSize (used by validateSkillFileSource) before performing full base64.DecodeString, and update CreateSkillFileBlob to validate blob.Data length against maxSkillFileContentSize prior to inserting into storage; reference decodeSkillDataURL, CreateSkillFileBlob, validateSkillFileSource, and maxSkillFileContentSize to add the pre-decode/pre-insert checks so large inputs are rejected early and memory spikes are avoided.Source: Coding guidelines
35-43:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSurface post-commit object-store failures to the caller.
runPendingSkillObjectWritesonly logsPutfailures, soCreateSkillandUpdateSkillstill return success even when some persistedstorage_keyobjects were never uploaded. That leaves committedskill_filesrows pointing at missing content. This path needs an error-propagating post-commit mechanism (or durable outbox/retry), not best-effort logging.As per coding guidelines, framework changes should use explicit error handling and bounded resource usage.
Also applies to: 559-560, 744-745
🤖 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 `@framework/configstore/skills.go` around lines 35 - 43, runPendingSkillObjectWrites currently swallows objectStore.Put errors (only logs) so CreateSkill/UpdateSkill appear successful while storage objects may be missing; change runPendingSkillObjectWrites to return an error and propagate it up to callers (CreateSkill and UpdateSkill) so the commit can fail if any Put fails, and ensure you process the writes with bounded resources (e.g., sequentially or with a limited worker pool) using the existing pendingSkillObjectWrite struct and objectStore.Put calls; update all call sites (CreateSkill, UpdateSkill and other usages) to handle and return the propagated error instead of assuming success.Source: Coding guidelines
377-396:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
blob_idcan still bind a foreign pending upload.The
blob_idpath only checks that the blob exists and is not already referenced by another skill. Any unbound blob row created throughCreateSkillFileBlobis therefore attachable by a different skill, because there is no owner/upload identity on the blob and no ownership check here. That permits cross-skill attachment of foreign uploaded content and later exposure/deletion through the wrong skill.As per coding guidelines, validate all untrusted input.
Also applies to: 1041-1044
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.
Duplicate comments:
In `@framework/configstore/skills.go`:
- Around line 430-444: The function decodeSkillDataURL currently decodes the
entire base64 payload before size checks and CreateSkillFileBlob inserts
blob.Data without gating size; update decodeSkillDataURL to parse the data: URL,
determine the base64 payload length (or decode in a streaming/limited manner)
and reject when the decoded size would exceed maxSkillFileContentSize (used by
validateSkillFileSource) before performing full base64.DecodeString, and update
CreateSkillFileBlob to validate blob.Data length against maxSkillFileContentSize
prior to inserting into storage; reference decodeSkillDataURL,
CreateSkillFileBlob, validateSkillFileSource, and maxSkillFileContentSize to add
the pre-decode/pre-insert checks so large inputs are rejected early and memory
spikes are avoided.
- Around line 35-43: runPendingSkillObjectWrites currently swallows
objectStore.Put errors (only logs) so CreateSkill/UpdateSkill appear successful
while storage objects may be missing; change runPendingSkillObjectWrites to
return an error and propagate it up to callers (CreateSkill and UpdateSkill) so
the commit can fail if any Put fails, and ensure you process the writes with
bounded resources (e.g., sequentially or with a limited worker pool) using the
existing pendingSkillObjectWrite struct and objectStore.Put calls; update all
call sites (CreateSkill, UpdateSkill and other usages) to handle and return the
propagated error instead of assuming success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8865f61e-b178-4aaf-ba94-3545c36cb5f0
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
7fdc066 to
603a3cf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
framework/configstore/store.go (1)
548-561:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat the
ConfigStoreexpansion as a breaking API change.Line 548 adds thirteen methods to an exported Go interface. That is a compile-time break for any out-of-repo
ConfigStoreimplementation or test double, even if this stack updates the in-repo store and mocks. Either keep the skills API off the root interface, or explicitly ship this as a breaking change with the appropriate versioning/release-note treatment. As per coding guidelines,framework/**changes should preserve backward-compatible contracts where possible.🤖 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 `@framework/configstore/store.go` around lines 548 - 561, The ConfigStore interface was extended with thirteen skill-related methods (CreateSkill, GetSkill, GetSkillLean, GetSkillByName, GetSkillVersion, ListSkillVersions, UpdateSkill, DeleteSkill, ListSkills, ShiftSkillVersion, GetAllSkillsVersion, CreateSkillFileBlob, CleanupOrphanSkillFileBlobs), causing a breaking API change for external implementations; instead, extract these methods into a new exported interface (e.g., SkillStore or SkillsRepository) and remove them from ConfigStore so callers can depend on the smaller ConfigStore and opt into the new SkillStore where needed; implement the new interface in the in-repo store types that currently implement those methods and update internal usage sites to require SkillStore (or both interfaces) while leaving ConfigStore unchanged to preserve backward compatibility.Source: Coding guidelines
framework/configstore/skills.go (3)
35-43:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't report success when the post-commit object upload fails.
CreateSkillandUpdateSkillstill return success even whenobjectStore.Putfails, becauserunPendingSkillObjectWritesonly logs the error. That leaves committedstorage_keyrows pointing at missing objects. This needs a real consistency boundary: either a durable pending/outbox state that is retried before the file becomes serveable, or an API contract that surfaces the failure and prevents exposing the new version until the upload succeeds. As per coding guidelines, framework changes should use explicit error handling and bounded resource usage.Also applies to: 559-560, 744-745
🤖 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 `@framework/configstore/skills.go` around lines 35 - 43, runPendingSkillObjectWrites currently swallows objectStore.Put failures which lets CreateSkill/UpdateSkill report success despite missing blobs; change runPendingSkillObjectWrites to return an error (or aggregate errors) when any pendingSkillObjectWrite fails, propagate that error back to callers (CreateSkill and UpdateSkill) and make those APIs fail/rollback (or block committing the storage_key) when the upload fails; ensure you reference and handle objectStore.Put errors instead of only logging them, and update all call sites (including the other occurrences noted) to respect the new error return so the API does not expose a new version unless all uploads succeeded or a durable pending/outbox retry state is implemented.Source: Coding guidelines
377-397:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOrphan
blob_ids are still attachable by any skill.The blob path only checks that the row exists and is not already bound to another skill. Because
CreateSkillFileBlobcreates unscoped orphan blobs, any caller that knows an unboundblob_idcan attach it to a different skill.storage_keyhas prefix-based ownership checks; DB blobs need an equivalent ownership/scope check as well, or the blob table needs enough metadata to enforce one. As per coding guidelines, framework storage changes should validate untrusted references and preserve least-privilege access to persisted data.Also applies to: 1041-1044
🤖 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 `@framework/configstore/skills.go` around lines 377 - 397, The validateSkillFileReference currently only checks existence and non-duplication of a blob_id (in validateSkillFileReference, tables.TableSkillFileBlob and tables.TableSkillFile), allowing any caller who knows an orphan blob_id to attach it; fix by enforcing ownership/scope: when loading the blob row in validateSkillFileReference, select its owner/metadata (e.g., storage_key or an owner_id/tenant column on tables.TableSkillFileBlob) and verify it is allowed to be bound to the given skillID (for example by checking storage_key prefix matches the skill's owner/namespace or by joining skill_versions to verify same owner/tenant), and reject attachment if the blob's owner/namespace does not match the skill's owner; if the blob table lacks owner metadata, add a persistent owner/tenant column and populate it in CreateSkillFileBlob, then use that column in validateSkillFileReference for the check.Source: Coding guidelines
430-444:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject oversized data URLs before decoding the payload.
The size limit is enforced only after
DecodeStringmaterializes the full byte slice. A sufficiently largedata:URL can still blow up memory before the guard runs. Check the decoded length from the base64 payload first and fail early.Suggested fix
func decodeSkillDataURL(dataURL string) ([]byte, string, error) { parsed, err := url.Parse(dataURL) if err != nil || parsed.Scheme != "data" { return nil, "", fmt.Errorf("dataurl must be a valid data: URL") } parts := strings.SplitN(dataURL, ",", 2) if len(parts) != 2 || !strings.Contains(parts[0], ";base64") { return nil, "", fmt.Errorf("dataurl must contain ;base64,") } mimeType := strings.TrimPrefix(strings.Split(parts[0], ";")[0], "data:") + if base64.StdEncoding.DecodedLen(len(parts[1])) > maxSkillFileContentSize { + return nil, "", fmt.Errorf("dataurl file content exceeds maximum size of %d bytes", maxSkillFileContentSize) + } data, err := base64.StdEncoding.DecodeString(parts[1]) if err != nil { return nil, "", fmt.Errorf("dataurl base64 decode failed: %w", err) } return data, mimeType, nil }As per coding guidelines, framework changes should enforce size limits and bounded resource usage.
Also applies to: 491-497
🤖 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 `@framework/configstore/skills.go` around lines 430 - 444, The decodeSkillDataURL function currently decodes the entire base64 payload before enforcing the size limit; change it to validate the payload size first by calculating the decoded length from the base64 payload string (use base64.StdEncoding.DecodedLen(len(payload))) and reject if that exceeds a defined limit (introduce or use a constant like maxSkillDataSize), returning an error before calling base64.StdEncoding.DecodeString; apply the same pre-decode size check to the other similar routine referenced (the block around lines 491-497) so no data URL is fully materialized before validation.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.
Duplicate comments:
In `@framework/configstore/skills.go`:
- Around line 35-43: runPendingSkillObjectWrites currently swallows
objectStore.Put failures which lets CreateSkill/UpdateSkill report success
despite missing blobs; change runPendingSkillObjectWrites to return an error (or
aggregate errors) when any pendingSkillObjectWrite fails, propagate that error
back to callers (CreateSkill and UpdateSkill) and make those APIs fail/rollback
(or block committing the storage_key) when the upload fails; ensure you
reference and handle objectStore.Put errors instead of only logging them, and
update all call sites (including the other occurrences noted) to respect the new
error return so the API does not expose a new version unless all uploads
succeeded or a durable pending/outbox retry state is implemented.
- Around line 377-397: The validateSkillFileReference currently only checks
existence and non-duplication of a blob_id (in validateSkillFileReference,
tables.TableSkillFileBlob and tables.TableSkillFile), allowing any caller who
knows an orphan blob_id to attach it; fix by enforcing ownership/scope: when
loading the blob row in validateSkillFileReference, select its owner/metadata
(e.g., storage_key or an owner_id/tenant column on tables.TableSkillFileBlob)
and verify it is allowed to be bound to the given skillID (for example by
checking storage_key prefix matches the skill's owner/namespace or by joining
skill_versions to verify same owner/tenant), and reject attachment if the blob's
owner/namespace does not match the skill's owner; if the blob table lacks owner
metadata, add a persistent owner/tenant column and populate it in
CreateSkillFileBlob, then use that column in validateSkillFileReference for the
check.
- Around line 430-444: The decodeSkillDataURL function currently decodes the
entire base64 payload before enforcing the size limit; change it to validate the
payload size first by calculating the decoded length from the base64 payload
string (use base64.StdEncoding.DecodedLen(len(payload))) and reject if that
exceeds a defined limit (introduce or use a constant like maxSkillDataSize),
returning an error before calling base64.StdEncoding.DecodeString; apply the
same pre-decode size check to the other similar routine referenced (the block
around lines 491-497) so no data URL is fully materialized before validation.
In `@framework/configstore/store.go`:
- Around line 548-561: The ConfigStore interface was extended with thirteen
skill-related methods (CreateSkill, GetSkill, GetSkillLean, GetSkillByName,
GetSkillVersion, ListSkillVersions, UpdateSkill, DeleteSkill, ListSkills,
ShiftSkillVersion, GetAllSkillsVersion, CreateSkillFileBlob,
CleanupOrphanSkillFileBlobs), causing a breaking API change for external
implementations; instead, extract these methods into a new exported interface
(e.g., SkillStore or SkillsRepository) and remove them from ConfigStore so
callers can depend on the smaller ConfigStore and opt into the new SkillStore
where needed; implement the new interface in the in-repo store types that
currently implement those methods and update internal usage sites to require
SkillStore (or both interfaces) while leaving ConfigStore unchanged to preserve
backward compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62216439-6436-485b-8b1f-f7a209676fd8
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
framework/configstore/rdb_test.go (1)
107-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the fixture on a valid monotonic version history.
validateSkillVersionIncrement()rejects1.0.2-1after1.0.3, so this test currently assertsGetSkillLean()behavior on a state the write path should never persist. That makesHighestVersiondiverge from the invariant it is later used for during bump validation.♻️ Minimal fixture tweak
err = store.DB().Create(&tables.TableSkill{ ID: skillID, Name: "latest-created", Description: "Latest created version test", SkillMDBody: "body", - LatestVersion: "1.0.3", + LatestVersion: "1.0.4", CreatedAt: baseTime, UpdatedAt: baseTime, }).Error @@ err = store.DB().Create(&tables.TableSkillVersion{ ID: "skill-version-new", SkillID: skillID, - Version: "1.0.2-1", + Version: "1.0.4", SkillMDBody: "body", FrontmatterSnapshot: tables.SkillJSONMap{"name": "latest-created", "description": "Latest created version test"}, CreatedAt: baseTime.Add(time.Minute), }).Error @@ - assert.Equal(t, "1.0.2-1", latest) + assert.Equal(t, "1.0.4", latest) @@ - assert.Equal(t, "1.0.2-1", skill.HighestVersion) + assert.Equal(t, "1.0.4", skill.HighestVersion)🤖 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 `@framework/configstore/rdb_test.go` around lines 107 - 142, Test fixture uses an out-of-order version ("1.0.2-1" created after "1.0.3") which violates validateSkillVersionIncrement() and makes HighestVersion unrealistic; update the created-later version in the TableSkillVersion record (the "skill-version-new" entry used by latestCreatedSkillVersion() and verified via GetSkillLean()) to a monotonic-next value (e.g., "1.0.4") and keep its CreatedAt after baseTime so the test represents a valid version history for HighestVersion.framework/configstore/skills.go (1)
35-43:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPropagate post-commit object write failures.
objectStore.Puterrors are only logged here, soCreateSkillandUpdateSkillcan commitstorage_keyreferences and still return success when the object bytes were never stored. Using the request context after commit makes this easy to hit on cancellation/timeouts as well. This needs an error-aware durable handoff (outbox/retry/compensation), not log-and-continue. As per coding guidelines, framework changes should use explicit error handling and bounded resource usage.Also applies to: 559-560, 744-745
🤖 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 `@framework/configstore/skills.go` around lines 35 - 43, runPendingSkillObjectWrites currently swallows objectStore.Put errors (just logs) causing CreateSkill/UpdateSkill to commit storage_key refs despite failed writes; change runPendingSkillObjectWrites to return an error (or aggregated error) and make callers (CreateSkill, UpdateSkill) handle it instead of assuming success, e.g., collect per-write failures from objectStore.Put for each pendingSkillObjectWrite, apply limited retries/backoff or enqueue for durable handoff and return a clear error if writes ultimately fail; ensure the implementation bounds resource usage (limit concurrency/queue size) and update all call sites (CreateSkill, UpdateSkill and the other occurrences noted) to propagate or handle the returned error rather than log-and-continue.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 `@framework/configstore/skills.go`:
- Around line 566-570: The current query in s.ScopedDB(...) preloads
"Versions.Files.Blob", which eagerly materializes every historical blob and can
cause unbounded memory/IO spikes; remove or stop using
Preload("Versions.Files.Blob") in the skill detail load and instead only load
Blob payloads for the single serving/requested version (e.g., load Versions and
Versions.Files but not Files.Blob), then fetch the Blob for the specific
Version/File afterwards via a targeted query (or lazy accessor) to bound
resource usage; apply the same change where Preload("Versions.Files.Blob") is
used at the other spot (lines 607-611) so blob bytes are only retrieved
on-demand for the serving version.
- Around line 698-704: The update currently writes skills.name via the DB update
(see the tx.Model(...).Select(... "Name" ...) .Updates(skill) call that sets
skill.LatestVersion), but snapshots and ShiftSkillVersion do not capture/restore
Name, causing historical versions to be shown under a renamed name; either make
name immutable by removing "Name" from the update/Select lists or properly
version name by adding Name to the snapshot creation and restoring it inside
ShiftSkillVersion. Fix by (a) removing "Name" from the Select fields in the
update paths that set skill.LatestVersion (and the equivalent blocks at the
other referenced spots) so renames are rejected here, or (b) add Name to the
version snapshot struct and include it when creating/restoring snapshots in
ShiftSkillVersion so old versions preserve their original name—apply the same
change consistently to the update blocks and the snapshot/restore logic.
---
Duplicate comments:
In `@framework/configstore/rdb_test.go`:
- Around line 107-142: Test fixture uses an out-of-order version ("1.0.2-1"
created after "1.0.3") which violates validateSkillVersionIncrement() and makes
HighestVersion unrealistic; update the created-later version in the
TableSkillVersion record (the "skill-version-new" entry used by
latestCreatedSkillVersion() and verified via GetSkillLean()) to a monotonic-next
value (e.g., "1.0.4") and keep its CreatedAt after baseTime so the test
represents a valid version history for HighestVersion.
In `@framework/configstore/skills.go`:
- Around line 35-43: runPendingSkillObjectWrites currently swallows
objectStore.Put errors (just logs) causing CreateSkill/UpdateSkill to commit
storage_key refs despite failed writes; change runPendingSkillObjectWrites to
return an error (or aggregated error) and make callers (CreateSkill,
UpdateSkill) handle it instead of assuming success, e.g., collect per-write
failures from objectStore.Put for each pendingSkillObjectWrite, apply limited
retries/backoff or enqueue for durable handoff and return a clear error if
writes ultimately fail; ensure the implementation bounds resource usage (limit
concurrency/queue size) and update all call sites (CreateSkill, UpdateSkill and
the other occurrences noted) to propagate or handle the returned error rather
than log-and-continue.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aacd7e3e-0bbb-48cb-b944-2fb1467fa7b9
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
603a3cf to
7626347
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
framework/configstore/store.go (1)
590-604:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdding the skills surface directly to
ConfigStoreis a source-breaking API change.
ConfigStoreis an exported interface, so widening it here forces every out-of-package implementation to add all of these methods before it compiles, even if that consumer never uses skills. If this package needs to remain backward-compatible, keep the skills API behind an opt-in secondary interface/capability instead of extending the base contract directly.🤖 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 `@framework/configstore/store.go` around lines 590 - 604, The change added many skill-related methods directly to the exported ConfigStore interface (CreateSkill, GetSkill, GetSkillLean, GetSkillByName, GetSkillVersion, ListSkillVersions, UpdateSkill, DeleteSkill, ListSkills, ShiftSkillVersion, GetAllSkillsVersion, BumpAllSkillsVersion, CreateSkillFileBlob, CleanupOrphanSkillFileBlobs), which is a breaking API change; instead, extract those methods into a new opt-in interface (e.g., SkillsStore or ConfigStoreSkills) and keep the original ConfigStore unchanged, then update only call sites that need skills to depend on the new interface (or accept ConfigStore + SkillsStore) and adjust implementing packages to implement the new interface as needed.framework/configstore/skills.go (3)
757-765:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject rename attempts explicitly.
UpdateSkillvalidatesskill.Namebut never persists it, so a caller can send a rename and still get success while the row keeps the old name. Since this flow now treats names as immutable, compare the incoming name withexisting.Nameafter the row lock and fail when they differ.Suggested fix
if err := tx.Clauses(clause.Locking{Strength: "UPDATE"}).First(&existing, "id = ?", skill.ID).Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { return ErrNotFound } return err } + if skill.Name != existing.Name { + return fmt.Errorf("skill name is immutable") + } existingLatestVersion = existing.LatestVersion🤖 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 `@framework/configstore/skills.go` around lines 757 - 765, In UpdateSkill, after you lock and load the row (tables.TableSkill) inside the transaction and before proceeding to update fields (right after existingLatestVersion = existing.LatestVersion), explicitly reject rename attempts by comparing skill.Name to existing.Name and returning a clear error when they differ (e.g., return ErrRenameNotAllowed or a new ErrImmutableName) so the call fails rather than silently ignoring the incoming name; perform this check inside the same Transaction callback right after the row is read and before any updates are applied.
442-457:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
upload_idfor staged upload keys.When
SourceType == uploadandUploadIDis empty, anyskills/uploads/...key passes the scope check and can be attached as long as it is not already referenced by another skill. That lets a leaked or guessed staged object be bound into this skill, and later reads/deletes operate on that foreign object. As per coding guidelines, validate all untrusted input and avoid overly broad access to external stores.Suggested fix
case tables.SkillSourceTypeUpload: - if file.UploadID != nil && strings.TrimSpace(*file.UploadID) != "" { - expectedPrefix := uploadPrefix + strings.TrimSpace(*file.UploadID) + "/" - if !strings.HasPrefix(key, expectedPrefix) { - return fmt.Errorf("storage_key %q does not belong to upload_id %q", key, strings.TrimSpace(*file.UploadID)) - } - return nil - } - if !strings.HasPrefix(key, uploadPrefix) { - return fmt.Errorf("storage_key %q is not under the staged upload prefix", key) - } + if file.UploadID == nil || strings.TrimSpace(*file.UploadID) == "" { + return fmt.Errorf("upload storage_key requires upload_id") + } + expectedPrefix := uploadPrefix + strings.TrimSpace(*file.UploadID) + "/" + if !strings.HasPrefix(key, expectedPrefix) { + return fmt.Errorf("storage_key %q does not belong to upload_id %q", key, strings.TrimSpace(*file.UploadID)) + }Also applies to: 484-500
🤖 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 `@framework/configstore/skills.go` around lines 442 - 457, When SourceType == tables.SkillSourceTypeUpload, do not allow a generic staged key; require a non-empty UploadID: if file.UploadID is nil or its trimmed value is empty return a validation error instead of only checking that key starts with uploadPrefix. Update the check around file.SourceType / UploadID / expectedPrefix / key (and the similar block later at the other occurrence) to reject missing UploadID explicitly so only keys under uploadPrefix+UploadID+"/" are accepted.Source: Coding guidelines
1138-1142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nil or blank blobs before generating IDs.
This helper dereferences
blob.IDunconditionally, so a nil caller panics here." "also skips regeneration and can be inserted as the primary key. Normalize the ID and reject nil inputs before the create.Suggested fix
func (s *RDBConfigStore) CreateSkillFileBlob(ctx context.Context, blob *tables.TableSkillFileBlob) error { - if blob.ID == "" { + if blob == nil { + return fmt.Errorf("skill file blob is required") + } + if strings.TrimSpace(blob.ID) == "" { blob.ID = uuid.NewString() } return s.DB().WithContext(ctx).Create(blob).Error }🤖 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 `@framework/configstore/skills.go` around lines 1138 - 1142, In CreateSkillFileBlob on RDBConfigStore, guard against a nil blob and normalize/validate blob.ID before using it: if blob is nil return an error; trim whitespace from blob.ID and if the trimmed ID is empty generate a new uuid with uuid.NewString() and assign it; ensure you never dereference blob when nil and reject blank-only IDs by normalizing before the DB Create call.
🤖 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 `@framework/configstore/migrations.go`:
- Around line 10102-10119: Update the Migrate function in the migration (the
Migrate closure that builds stmt using tx.Dialector.Name() and idxName) to
detect and repair an existing invalid/unfinished Postgres index before relying
on "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS": when Dialector.Name() !=
"sqlite" query pg_catalog.pg_index/pg_class (checking pg_index.indisvalid and/or
indisready for the index named idxName) and if the index exists but is not
valid/ready, run "DROP INDEX CONCURRENTLY <idxName>" then recreate it with
"CREATE UNIQUE INDEX CONCURRENTLY <idxName> ON governance_customers (name)";
keep the sqlite branch unchanged and ensure all Exec calls use
tx.WithContext(ctx) as currently done and return wrapped errors like the
existing fmt.Errorf on failure.
---
Duplicate comments:
In `@framework/configstore/skills.go`:
- Around line 757-765: In UpdateSkill, after you lock and load the row
(tables.TableSkill) inside the transaction and before proceeding to update
fields (right after existingLatestVersion = existing.LatestVersion), explicitly
reject rename attempts by comparing skill.Name to existing.Name and returning a
clear error when they differ (e.g., return ErrRenameNotAllowed or a new
ErrImmutableName) so the call fails rather than silently ignoring the incoming
name; perform this check inside the same Transaction callback right after the
row is read and before any updates are applied.
- Around line 442-457: When SourceType == tables.SkillSourceTypeUpload, do not
allow a generic staged key; require a non-empty UploadID: if file.UploadID is
nil or its trimmed value is empty return a validation error instead of only
checking that key starts with uploadPrefix. Update the check around
file.SourceType / UploadID / expectedPrefix / key (and the similar block later
at the other occurrence) to reject missing UploadID explicitly so only keys
under uploadPrefix+UploadID+"/" are accepted.
- Around line 1138-1142: In CreateSkillFileBlob on RDBConfigStore, guard against
a nil blob and normalize/validate blob.ID before using it: if blob is nil return
an error; trim whitespace from blob.ID and if the trimmed ID is empty generate a
new uuid with uuid.NewString() and assign it; ensure you never dereference blob
when nil and reject blank-only IDs by normalizing before the DB Create call.
In `@framework/configstore/store.go`:
- Around line 590-604: The change added many skill-related methods directly to
the exported ConfigStore interface (CreateSkill, GetSkill, GetSkillLean,
GetSkillByName, GetSkillVersion, ListSkillVersions, UpdateSkill, DeleteSkill,
ListSkills, ShiftSkillVersion, GetAllSkillsVersion, BumpAllSkillsVersion,
CreateSkillFileBlob, CleanupOrphanSkillFileBlobs), which is a breaking API
change; instead, extract those methods into a new opt-in interface (e.g.,
SkillsStore or ConfigStoreSkills) and keep the original ConfigStore unchanged,
then update only call sites that need skills to depend on the new interface (or
accept ConfigStore + SkillsStore) and adjust implementing packages to implement
the new interface as needed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3bb91028-658f-416b-8e7f-402a4389f911
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
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: 1
♻️ Duplicate comments (4)
framework/configstore/store.go (1)
590-604:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdding the skills surface directly to
ConfigStoreis a source-breaking API change.
ConfigStoreis an exported interface, so widening it here forces every out-of-package implementation to add all of these methods before it compiles, even if that consumer never uses skills. If this package needs to remain backward-compatible, keep the skills API behind an opt-in secondary interface/capability instead of extending the base contract directly.🤖 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 `@framework/configstore/store.go` around lines 590 - 604, The change added many skill-related methods directly to the exported ConfigStore interface (CreateSkill, GetSkill, GetSkillLean, GetSkillByName, GetSkillVersion, ListSkillVersions, UpdateSkill, DeleteSkill, ListSkills, ShiftSkillVersion, GetAllSkillsVersion, BumpAllSkillsVersion, CreateSkillFileBlob, CleanupOrphanSkillFileBlobs), which is a breaking API change; instead, extract those methods into a new opt-in interface (e.g., SkillsStore or ConfigStoreSkills) and keep the original ConfigStore unchanged, then update only call sites that need skills to depend on the new interface (or accept ConfigStore + SkillsStore) and adjust implementing packages to implement the new interface as needed.framework/configstore/skills.go (3)
757-765:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject rename attempts explicitly.
UpdateSkillvalidatesskill.Namebut never persists it, so a caller can send a rename and still get success while the row keeps the old name. Since this flow now treats names as immutable, compare the incoming name withexisting.Nameafter the row lock and fail when they differ.Suggested fix
if err := tx.Clauses(clause.Locking{Strength: "UPDATE"}).First(&existing, "id = ?", skill.ID).Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { return ErrNotFound } return err } + if skill.Name != existing.Name { + return fmt.Errorf("skill name is immutable") + } existingLatestVersion = existing.LatestVersion🤖 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 `@framework/configstore/skills.go` around lines 757 - 765, In UpdateSkill, after you lock and load the row (tables.TableSkill) inside the transaction and before proceeding to update fields (right after existingLatestVersion = existing.LatestVersion), explicitly reject rename attempts by comparing skill.Name to existing.Name and returning a clear error when they differ (e.g., return ErrRenameNotAllowed or a new ErrImmutableName) so the call fails rather than silently ignoring the incoming name; perform this check inside the same Transaction callback right after the row is read and before any updates are applied.
442-457:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
upload_idfor staged upload keys.When
SourceType == uploadandUploadIDis empty, anyskills/uploads/...key passes the scope check and can be attached as long as it is not already referenced by another skill. That lets a leaked or guessed staged object be bound into this skill, and later reads/deletes operate on that foreign object. As per coding guidelines, validate all untrusted input and avoid overly broad access to external stores.Suggested fix
case tables.SkillSourceTypeUpload: - if file.UploadID != nil && strings.TrimSpace(*file.UploadID) != "" { - expectedPrefix := uploadPrefix + strings.TrimSpace(*file.UploadID) + "/" - if !strings.HasPrefix(key, expectedPrefix) { - return fmt.Errorf("storage_key %q does not belong to upload_id %q", key, strings.TrimSpace(*file.UploadID)) - } - return nil - } - if !strings.HasPrefix(key, uploadPrefix) { - return fmt.Errorf("storage_key %q is not under the staged upload prefix", key) - } + if file.UploadID == nil || strings.TrimSpace(*file.UploadID) == "" { + return fmt.Errorf("upload storage_key requires upload_id") + } + expectedPrefix := uploadPrefix + strings.TrimSpace(*file.UploadID) + "/" + if !strings.HasPrefix(key, expectedPrefix) { + return fmt.Errorf("storage_key %q does not belong to upload_id %q", key, strings.TrimSpace(*file.UploadID)) + }Also applies to: 484-500
🤖 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 `@framework/configstore/skills.go` around lines 442 - 457, When SourceType == tables.SkillSourceTypeUpload, do not allow a generic staged key; require a non-empty UploadID: if file.UploadID is nil or its trimmed value is empty return a validation error instead of only checking that key starts with uploadPrefix. Update the check around file.SourceType / UploadID / expectedPrefix / key (and the similar block later at the other occurrence) to reject missing UploadID explicitly so only keys under uploadPrefix+UploadID+"/" are accepted.Source: Coding guidelines
1138-1142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nil or blank blobs before generating IDs.
This helper dereferences
blob.IDunconditionally, so a nil caller panics here." "also skips regeneration and can be inserted as the primary key. Normalize the ID and reject nil inputs before the create.Suggested fix
func (s *RDBConfigStore) CreateSkillFileBlob(ctx context.Context, blob *tables.TableSkillFileBlob) error { - if blob.ID == "" { + if blob == nil { + return fmt.Errorf("skill file blob is required") + } + if strings.TrimSpace(blob.ID) == "" { blob.ID = uuid.NewString() } return s.DB().WithContext(ctx).Create(blob).Error }🤖 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 `@framework/configstore/skills.go` around lines 1138 - 1142, In CreateSkillFileBlob on RDBConfigStore, guard against a nil blob and normalize/validate blob.ID before using it: if blob is nil return an error; trim whitespace from blob.ID and if the trimmed ID is empty generate a new uuid with uuid.NewString() and assign it; ensure you never dereference blob when nil and reject blank-only IDs by normalizing before the DB Create call.
🤖 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 `@framework/configstore/migrations.go`:
- Around line 10102-10119: Update the Migrate function in the migration (the
Migrate closure that builds stmt using tx.Dialector.Name() and idxName) to
detect and repair an existing invalid/unfinished Postgres index before relying
on "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS": when Dialector.Name() !=
"sqlite" query pg_catalog.pg_index/pg_class (checking pg_index.indisvalid and/or
indisready for the index named idxName) and if the index exists but is not
valid/ready, run "DROP INDEX CONCURRENTLY <idxName>" then recreate it with
"CREATE UNIQUE INDEX CONCURRENTLY <idxName> ON governance_customers (name)";
keep the sqlite branch unchanged and ensure all Exec calls use
tx.WithContext(ctx) as currently done and return wrapped errors like the
existing fmt.Errorf on failure.
---
Duplicate comments:
In `@framework/configstore/skills.go`:
- Around line 757-765: In UpdateSkill, after you lock and load the row
(tables.TableSkill) inside the transaction and before proceeding to update
fields (right after existingLatestVersion = existing.LatestVersion), explicitly
reject rename attempts by comparing skill.Name to existing.Name and returning a
clear error when they differ (e.g., return ErrRenameNotAllowed or a new
ErrImmutableName) so the call fails rather than silently ignoring the incoming
name; perform this check inside the same Transaction callback right after the
row is read and before any updates are applied.
- Around line 442-457: When SourceType == tables.SkillSourceTypeUpload, do not
allow a generic staged key; require a non-empty UploadID: if file.UploadID is
nil or its trimmed value is empty return a validation error instead of only
checking that key starts with uploadPrefix. Update the check around
file.SourceType / UploadID / expectedPrefix / key (and the similar block later
at the other occurrence) to reject missing UploadID explicitly so only keys
under uploadPrefix+UploadID+"/" are accepted.
- Around line 1138-1142: In CreateSkillFileBlob on RDBConfigStore, guard against
a nil blob and normalize/validate blob.ID before using it: if blob is nil return
an error; trim whitespace from blob.ID and if the trimmed ID is empty generate a
new uuid with uuid.NewString() and assign it; ensure you never dereference blob
when nil and reject blank-only IDs by normalizing before the DB Create call.
In `@framework/configstore/store.go`:
- Around line 590-604: The change added many skill-related methods directly to
the exported ConfigStore interface (CreateSkill, GetSkill, GetSkillLean,
GetSkillByName, GetSkillVersion, ListSkillVersions, UpdateSkill, DeleteSkill,
ListSkills, ShiftSkillVersion, GetAllSkillsVersion, BumpAllSkillsVersion,
CreateSkillFileBlob, CleanupOrphanSkillFileBlobs), which is a breaking API
change; instead, extract those methods into a new opt-in interface (e.g.,
SkillsStore or ConfigStoreSkills) and keep the original ConfigStore unchanged,
then update only call sites that need skills to depend on the new interface (or
accept ConfigStore + SkillsStore) and adjust implementing packages to implement
the new interface as needed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3bb91028-658f-416b-8e7f-402a4389f911
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
🛑 Comments failed to post (1)
framework/configstore/migrations.go (1)
10102-10119:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show context around specified lines sed -n '10060,10180p' framework/configstore/migrations.go | nl -ba | sed -n '1,140p'Repository: maximhq/bifrost
Length of output: 102
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find occurrences of governance_customers (name) unique index migration rg -n "governance_customers\s*\(name\)" framework/configstore/migrations.goRepository: maximhq/bifrost
Length of output: 272
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for any handling of pg_index.indisvalid / indisvalid repairs rg -n "indisvalid|indisready|pg_index|pg_class|REINDEX|concurrently.*invalid" framework/configstore/migrations.goRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for any other CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS patterns rg -n "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS" framework/configstore/migrations.goRepository: maximhq/bifrost
Length of output: 172
🏁 Script executed:
#!/bin/bash set -euo pipefail # Identify idxName definition in vicinity sed -n '10080,10130p' framework/configstore/migrations.go | nl -baRepository: maximhq/bifrost
Length of output: 102
🏁 Script executed:
#!/bin/bash set -euo pipefail # Try to capture surrounding migration entry (maybe within a Migrations slice) for context sed -n '9960,10240p' framework/configstore/migrations.go | nl -ba | head -n 240Repository: maximhq/bifrost
Length of output: 102
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Context 10060-10180 ===" sed -n '10060,10180p' framework/configstore/migrations.go | nl -ba | sed -n '1,140p' echo "=== governance_customers(name) occurrences ===" rg -n "governance_customers\s*\(name\)" framework/configstore/migrations.go || true echo "=== indisvalid/pg_index/REINDEX patterns ===" rg -n "indisvalid|indisready|pg_index|pg_class|REINDEX|concurrently.*invalid" framework/configstore/migrations.go || true echo "=== CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS ===" rg -n "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS" framework/configstore/migrations.go || true echo "=== idxName vicinity 10080-10130 ===" sed -n '10080,10130p' framework/configstore/migrations.go | nl -ba echo "=== Surrounding migration entry 9960-10240 (first 240 lines) ===" sed -n '9960,10240p' framework/configstore/migrations.go | nl -ba | head -n 240Repository: maximhq/bifrost
Length of output: 148
Repair invalid concurrent unique indexes on PostgreSQL to avoid skipping enforcement.
The migration in
framework/configstore/migrations.gousesCREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTSforgovernance_customers (name)on non-sqlite, but there’s no handling for an existing invalid index from an interrupted concurrent build—so a rerun can hitIF NOT EXISTSand leavegovernance_customers.namewithout enforced uniqueness. Add a check of index validity (e.g.,pg_index.indisvalid/indisready) andDROP INDEX CONCURRENTLY+ recreate when invalid.🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 10118-10118: SQL query built via fmt.Sprintf or string concatenation passed to a database method. Use parameterized queries with placeholder arguments.
(coderabbit.sql-injection.go-query-format)
🤖 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 `@framework/configstore/migrations.go` around lines 10102 - 10119, Update the Migrate function in the migration (the Migrate closure that builds stmt using tx.Dialector.Name() and idxName) to detect and repair an existing invalid/unfinished Postgres index before relying on "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS": when Dialector.Name() != "sqlite" query pg_catalog.pg_index/pg_class (checking pg_index.indisvalid and/or indisready for the index named idxName) and if the index exists but is not valid/ready, run "DROP INDEX CONCURRENTLY <idxName>" then recreate it with "CREATE UNIQUE INDEX CONCURRENTLY <idxName> ON governance_customers (name)"; keep the sqlite branch unchanged and ensure all Exec calls use tx.WithContext(ctx) as currently done and return wrapped errors like the existing fmt.Errorf on failure.Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@framework/configstore/migrations.go`:
- Around line 10086-10088: The migration's Rollback function currently returns
nil even though this dedup step permanently rewrites customer names; update the
migration to mark this step as non-rollbackable by replacing the no-op Rollback
with an explicit non-rollback signal (e.g., set Rollback to nil or return a
descriptive error) on the same migration struct so callers know this step cannot
be undone, and add a short comment in the migration near Rollback explaining
that customer rows are irreversibly renamed while the subsequent index-drop step
remains reversible.
In `@framework/configstore/skills.go`:
- Around line 1138-1142: The CreateSkillFileBlob method dereferences blob.ID
without checking for nil; add a nil guard at the top of
RDBConfigStore.CreateSkillFileBlob that returns a clear validation/error (e.g.,
fmt.Errorf or a typed error) when blob is nil, and only generate
uuid.NewString() and call s.DB().WithContext(ctx).Create(blob) when blob is
non-nil; reference function CreateSkillFileBlob and type
tables.TableSkillFileBlob in your change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0cafc539-fb7d-418b-b9de-3a7cc3585d82
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
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: 2
🤖 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 `@framework/configstore/migrations.go`:
- Around line 10086-10088: The migration's Rollback function currently returns
nil even though this dedup step permanently rewrites customer names; update the
migration to mark this step as non-rollbackable by replacing the no-op Rollback
with an explicit non-rollback signal (e.g., set Rollback to nil or return a
descriptive error) on the same migration struct so callers know this step cannot
be undone, and add a short comment in the migration near Rollback explaining
that customer rows are irreversibly renamed while the subsequent index-drop step
remains reversible.
In `@framework/configstore/skills.go`:
- Around line 1138-1142: The CreateSkillFileBlob method dereferences blob.ID
without checking for nil; add a nil guard at the top of
RDBConfigStore.CreateSkillFileBlob that returns a clear validation/error (e.g.,
fmt.Errorf or a typed error) when blob is nil, and only generate
uuid.NewString() and call s.DB().WithContext(ctx).Create(blob) when blob is
non-nil; reference function CreateSkillFileBlob and type
tables.TableSkillFileBlob in your change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0cafc539-fb7d-418b-b9de-3a7cc3585d82
📒 Files selected for processing (5)
framework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
🛑 Comments failed to post (2)
framework/configstore/migrations.go (1)
10086-10088:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback still reports success after an irreversible customer-name rewrite.
Lines 10086-10088 return
nileven though this step permanently renames persisted customer rows. That makes the migration look rollback-safe when only the later index can actually be reverted. Keep the index rollback in step 2 if you want, but this dedup step itself should be marked non-rollbackable.Suggested fix
Rollback: func(tx *gorm.DB) error { - return nil // name renames are not reversed; dropping the index in step 2 restores the invariant + return fmt.Errorf("add_customer_name_unique_constraint_dedup is non-rollbackable: renamed customer names are not restored") },As per coding guidelines, "If a migration cannot be rolled back, explicitly flag it as non-rollbackable." Based on learnings, best-effort rollback paths should still document unreverted data changes.
📝 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.Rollback: func(tx *gorm.DB) error { return fmt.Errorf("add_customer_name_unique_constraint_dedup is non-rollbackable: renamed customer names are not restored") },🤖 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 `@framework/configstore/migrations.go` around lines 10086 - 10088, The migration's Rollback function currently returns nil even though this dedup step permanently rewrites customer names; update the migration to mark this step as non-rollbackable by replacing the no-op Rollback with an explicit non-rollback signal (e.g., set Rollback to nil or return a descriptive error) on the same migration struct so callers know this step cannot be undone, and add a short comment in the migration near Rollback explaining that customer rows are irreversibly renamed while the subsequent index-drop step remains reversible.Sources: Coding guidelines, Learnings
framework/configstore/skills.go (1)
1138-1142:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
CreateSkillFileBlobagainst nil input.This exported helper dereferences
blob.IDunconditionally. A nil caller will panic the request path instead of getting a validation error.Proposed fix
func (s *RDBConfigStore) CreateSkillFileBlob(ctx context.Context, blob *tables.TableSkillFileBlob) error { + if blob == nil { + return fmt.Errorf("skill file blob is required") + } if blob.ID == "" { blob.ID = uuid.NewString() } return s.DB().WithContext(ctx).Create(blob).Error }📝 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.func (s *RDBConfigStore) CreateSkillFileBlob(ctx context.Context, blob *tables.TableSkillFileBlob) error { if blob == nil { return fmt.Errorf("skill file blob is required") } if blob.ID == "" { blob.ID = uuid.NewString() } return s.DB().WithContext(ctx).Create(blob).Error }🤖 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 `@framework/configstore/skills.go` around lines 1138 - 1142, The CreateSkillFileBlob method dereferences blob.ID without checking for nil; add a nil guard at the top of RDBConfigStore.CreateSkillFileBlob that returns a clear validation/error (e.g., fmt.Errorf or a typed error) when blob is nil, and only generate uuid.NewString() and call s.DB().WithContext(ctx).Create(blob) when blob is non-nil; reference function CreateSkillFileBlob and type tables.TableSkillFileBlob in your change.
f3432d9 to
2b50a90
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.coderabbit.yaml (1)
24-29:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
reviews.auto_reviewkey makes the config invalid.
reviews.auto_reviewis declared at both Line 15 and Line 24. This causes a duplicate mapping key parse error and can prevent the settings file from loading.Suggested fix (merge into one
auto_reviewblock)reviews: high_level_summary_in_walkthrough: true fail_commit_status: true suggested_labels: false auto_assign_reviewers: true auto_review: + enabled: true + drafts: false + auto_incremental_review: true + base_branches: + - ".*" auto_pause_after_reviewed_commits: 0 profile: chill request_changes_workflow: true high_level_summary: true review_status: true changed_files_summary: true slop_detection: enabled: true - auto_review: - enabled: true - drafts: false - auto_incremental_review: true - base_branches: - - ".*"🤖 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 @.coderabbit.yaml around lines 24 - 29, The YAML has duplicate reviews.auto_review mappings (one at Line 15 and another at Line 24) which makes the config invalid; locate both occurrences of the reviews.auto_review block and merge them into a single auto_review mapping that contains the combined keys (e.g., enabled, drafts, auto_incremental_review, base_branches) preserving desired values, then remove the duplicate block so only one reviews.auto_review key exists.
🤖 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.
Outside diff comments:
In @.coderabbit.yaml:
- Around line 24-29: The YAML has duplicate reviews.auto_review mappings (one at
Line 15 and another at Line 24) which makes the config invalid; locate both
occurrences of the reviews.auto_review block and merge them into a single
auto_review mapping that contains the combined keys (e.g., enabled, drafts,
auto_incremental_review, base_branches) preserving desired values, then remove
the duplicate block so only one reviews.auto_review key exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5691390-8377-47e8-b16e-4d46b7199bad
📒 Files selected for processing (6)
.coderabbit.yamlframework/configstore/migrations.goframework/configstore/rdb_test.goframework/configstore/skills.goframework/configstore/store.goframework/configstore/tables/skills.go
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/configstore/migrations.go
- framework/configstore/rdb_test.go
- framework/configstore/tables/skills.go
- framework/configstore/skills.go
da190bf to
1a22c06
Compare
1a22c06 to
025890c
Compare
|
@greptile-apps re: the P1 confidence note about UpdateSkill with serve=true — |
|
You're right — I misread the data flow. The three points hold up:
The original concern was unfounded. Thanks for walking through the phases — the logic is sound. |
025890c to
c98c62f
Compare
Merge activity
|
## Summary Adds the core Skills Repository persistence layer: database models, migrations, config store interfaces, CRUD/versioning logic, validators, and tests. This is the foundation used by the management API, serving layer, config registry, and dashboard. ## Changes - Added Skills Repository tables for skills, versions, files, and file blobs. - Added config store APIs for skill CRUD, versioning, file persistence, validation, and orphan blob cleanup. - Added migrations and indexes for Skills Repository storage. - Added configstore tests for skill version validation and latest-created version ordering. - Implemented arbitrary relative file paths instead of fixed folder categories. - Kept files version-level so each skill version owns its file set. - Centralized skill name, version, file path, frontmatter, and reserved-field validation helpers. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Validate this change with the following command: ```sh # From bifrost/ direnv exec . go test ./framework/configstore ``` Expected result: configstore tests pass, including Skills Repository version increment validation and latest-created `highest_version` coverage. No new configs or environment variables are added in this PR. ## Screenshots/Recordings N/A — backend persistence-only change. ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations This PR adds persistence for skill metadata, version snapshots, file references, and DB fallback blobs. File path validation rejects absolute paths, backslashes, trailing slashes, and `.`/`..` segments before storage. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
3 participants
Summary
Adds the core Skills Repository persistence layer: database models, migrations,
config store interfaces, CRUD/versioning logic, validators, and tests. This is
the foundation used by the management API, serving layer, config registry, and
dashboard.
Changes
validation, and orphan blob cleanup.
version ordering.
validation helpers.
Type of change
Affected areas
How to test
Validate this change with the following command:
Expected result: configstore tests pass, including Skills Repository version
increment validation and latest-created
highest_versioncoverage.No new configs or environment variables are added in this PR.
Screenshots/Recordings
N/A — backend persistence-only change.
Breaking changes
Related issues
N/A
Security considerations
This PR adds persistence for skill metadata, version snapshots, file references,
and DB fallback blobs. File path validation rejects absolute paths, backslashes,
trailing slashes, and
./..segments before storage.Checklist
docs/contributing/README.mdand followed the guidelines