P4b: skill noun + contrib/store (SQLite for budget/persona/skill) #5
Reference in New Issue
Block a user
Delete Branch "phase-4b-skill"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Continues P4 (stacked on #4). The skill half of the persona/skill pair, plus the second nested module that makes the seams durable.
Skill noun (
skill/)A clean redesign, not a faithful lift of mort's 60-method
skills.Storagekitchen sink:skill.go/skill_version.go/validate.go/inputs.go/schedule.gomoved clean; the only host couplings severed:llms.IsTierName → model.IsTierName,DefaultChatbotInputNamelocalized.SkillStoreis deliberately lean — skill lifecycle (CRUD + visibility) + versioning + scheduling only. The KV/file/quota sub-stores that were fused into mort's interface are thetools/store seams; email/channel grants stay host concerns.Skill.ToRunnable()lowers a skill intorun.RunnableAgent(flat tool list, no palette);Memorydefault.contrib/store/— the second module (the architectural payoff)A separate
go.modcarryingmodernc.org/sqlite(pure Go, no cgo), so the SQLite driver never enters the core go.sum. A static-binary host (gadfly) importing only the core stays static; a host wanting turnkey persistence imports this.store.Open(dsn) → *DB, accessor-per-seam:db.Budget(),db.Personas(),db.Skills().budget.Memory.Verification
go build/vet/test -race ./...green in both modules.Remaining P4 (P4c)
audit SQLite store + the critic / schedule / checkpoint batteries (the latter three are inert until the executor wires those
run.Ports— a P2 follow-up).🤖 Generated with Claude Code
🪰 Gadfly review —
glm-5.2:cloud(ollama-cloud)Verdict: Minor issues — 3 reviewers: security, correctness, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Through the security lens (authn/authz, injection, secret leakage, unsafe deserialization, input validation) I traced every tainted-to-sink path in the new
contrib/store/*.goandskill/*.gofiles. Findings:SQL injection — none. All user/owner/version IDs flow through
?placeholders (contrib/store/budget_store.go:46,contrib/store/persona_store.go:56,61,109,contrib/store/skill_store.go:63,97,128). The two helpers that string-concatenate a WHERE clause (personaStore.getOnecontrib/store/persona_store.go:82,skillStore.getOne/skillStore.querycontrib/store/skill_store.go:106,144) take awherestring, but I grepped every caller: all pass static literals ("id = ?","owner_id = ? AND name = ?","webhook_secret = ?","visibility = ?","chatbot = 1"). No user-tainted text reaches the concatenation. This is a safe-but-fragile pattern (a future caller passing a dynamicwherewould open an injection vector); as defense-in-depth, consider restrictingwhereto an enum/allowlist rather than a free string. Verified by readingcontrib/store/persona_store.goandcontrib/store/skill_store.goin full.Secret/credential storage — deliberate, not a leak.
persona.Agent.WebhookSecret(andskill.Skill.WebhookSecret) are stored plaintext in both an indexed column and the JSONdatablob. Theskill.godoc comment explicitly justifies this (skill/skill.go~line 270): the webhook handler must look up by secret on every POST, and the secret is a 64-hex-char random URL key, not a password.GetAgentByWebhookSecretcorrectly guards the empty case (contrib/store/persona_store.go:112-114). The SQLite=comparison isn't constant-time, but with ~256 bits of entropy the timing channel is infeasible to exploit. Not a finding.Unsafe deserialization — none.
json.Unmarshaltargets concrete domain structs (persona.Agent,skill.Skill,skill.SkillVersion), neverinterface{}/anywith method sets, so there's no gadget surface. Verified atcontrib/store/persona_store.go:53,contrib/store/skill_store.go:96,232.Authz gaps — by design, not a finding. The store performs no ownership/permission checks (
Delete,Save,Getaccept any ID/owner). The PR description explicitly scopes authz as a host concern, and the leanSkillStoreinterface mirrors that. The kernel contract is "trust the caller," consistent with the other batteries.DSN handling — fine.
store.Open(contrib/store/sqlite.go:34) passes the DSN straight tosql.Open("sqlite", dsn). modernc's DSN isfile:…?_pragma=…with no credential component, so the%qformatting in the error paths cannot leak secrets. The DSN is host config, not user input.Input validation — delegated correctly. The store does not call
skill.Validate; validation lives inskill/validate.goand is the save-path's job. No store method accepts a struct and silently persists invariant-violating state that the kernel would then trust — the round-trip tests demonstrate the struct is what the host handed in.Nothing in my lane rises to a blocking or even materially exploitable issue. The single defense-in-depth nit (free-form
wherestring in two unexported helpers) is worth tightening but is not a present vulnerability.🎯 Correctness — Minor issues
VERDICT: Minor issues
Reviewed through the Correctness lens. Verified against the checked-out code:
budget/budget.go(DBBudget.Check/Commit +7*24*time.Hourwindow),budget/storage.go(budgetWindowconst),budget/memory.go,persona/agent.go(NextRunAt *time.Time),run/agent.go(RunnableAgentfields),model/tiers.go(IsTierName), and the newskill/andcontrib/store/files.Findings:
contrib/store/skill_store.go:199-204—AppendVersionseq assignment is non-atomic, unlike the Memory store.seq = SELECT COALESCE(MAX(seq),0)+1 ...is read in one statement, then the INSERT runs in a separate statement with no transaction and noUNIQUEconstraint on(skill_id, seq)(only a non-unique index at line 48). Two concurrentAppendVersioncalls for the sameskill_idcan both compute the sameseqand both insert successfully, producing duplicateseqvalues.ListVersionsBySkillorders byORDER BY seq DESC(line 211), so the "newest-first" ordering contract documented at line 45 and asserted inTestSQLiteSkillStorewould break under concurrency. The in-memoryMemory.AppendVersion(skill/memory.go) is safe because it appends underm.mu.Lock(). Fix: wrap the MAX+INSERT in aBeginTx/commit (matchingbudgetStore.Add's pattern), or addUNIQUE(skill_id, seq)and retry on conflict. Verified by reading both stores.contrib/store/budget_store.go:50-93— transaction isolation relies on SQLite write serialization, not an explicit row lock. TheAddreads viatx.QueryRowContextthen upserts, but usesBeginTx(ctx, nil)(deferred). The atomic-vs-TOCTOU claim (the stated purpose per the PR description and CLAUDE.md) holds only because modernc/SQLite serializes writers on a single DB file; a deferred txn lets two concurrentAdds both read the pre-increment row before either writes. In practice SQLite's write lock will make one fail withSQLITE_BUSYrather than silently corrupt, but thebusy_timeoutpragma noted insqlite.go's doc example is not actually set inOpen(sqlite.go:31-43), so a busy caller gets an immediate error instead of waiting. This is borderline concurrency rather than pure logic; flagged because it undercuts the "closes the in-memory store's TOCTOU window" claim. Verified:Opensets no pragmas.contrib/store/budget_store.go— the conformance test does not exercise theupdatedAt/window-start consistency the wayCheckreads it.Check(budget.go:133) treats the window as expired whenb.now().Sub(bud.WindowStart) < 7*24*time.Houris false, usingbud.WindowStartfromGet.Addstoreswindow_startasnow.Unix()on rollover andGetreconstructs viatime.Unix(ws,0).UTC(). These are consistent (I re-derived: unix-second round-trip is lossless for second-resolutiontime.Time), so no defect here — noting only that the test never assertsrow.WindowStartpost-rollover, so a future off-by-one in the rollover comparison (>=vs>) at line 78 could slip through. Verified the>= 7*24*time.Hourmatchesbudget.memory.go:47andbudgetWindow.No material issues in:
ToRunnablefield mapping (run/agent.go confirmsLowLevelTools/SkillPalette/SubAgentPaletteshapes, deliberately empty palettes),ParseScheduleshorthand expansion,ScheduleMinIntervalprobing,effectiveCommandParams/ResolveCommandInputsempty-schema inflation,ListSharedWithslice-aliasing (range copies the header; overwriting the backing array during iteration is safe), or the JSON-blob round-trip approach for persona/skill.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
contrib/store/skill_store.go:201— swallowed Scan error + non-atomicseqcomputation.Two problems, both in the error/edge-case lane:
_ =discards every scan failure, not justsql.ErrNoRows(which can't happen here anyway — the query always returns one row thanks toCOALESCE). On a real failure (context cancellation, a corrupt/locked DB, driver error)seqstays0and the subsequentINSERTproceeds silently with a bogus sequence number instead of surfacing the error. The caller getsniland believes the version was appended correctly.MAX(seq)+1is computed outside any transaction, unlikebudgetStore.Addwhich deliberately wraps its read-modify-write inBeginTxto close the TOCTOU window. Two concurrentAppendVersioncalls for the sameskill_idcompute the sameseqand both insert successfully — the indexidx_skill_versions_skill ON skill_versions(skill_id, seq)(skill_store.go:48) is non-unique, so neither insert fails. Result: duplicateseqvalues, which breaks theORDER BY seq DESCnewest-first ordering thatListVersionsBySkill(skill_store.go:211) relies on.Suggested fix: wrap the select+insert in a transaction (matching
budgetStore.Add's pattern) and propagate the scan error.contrib/store/skill_store.go:184andcontrib/store/persona_store.go:159—MarkScheduledRun/MarkAgentScheduledRunare read-modify-write without a transaction.Getthen mutate thenSave. Concurrent calls (or a concurrentSavefrom elsewhere) race and the later write wins, silently losing theLastScheduledRunAt/NextRunAtupdate. The errors themselves are propagated correctly, so this is borderline; flagged only because the lost-update path produces no error at all — the caller sees success. Lower priority than theAppendVersionfinding since scheduling collisions are less destructive than version-order corruption. Left as a note rather than a blocking claim.Everything else checked is sound:
scanAgents/scanSkills/ListVersionsBySkillalldefer rows.Close()and returnrows.Err();getOne/GetVersionByIDmapsql.ErrNoRows→ErrNotFound;budgetStore.Addcorrectly deferstx.Rollback()and wraps each step;Getreturns(nil, nil)on no-row.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 7m 36s
🪰 Gadfly review —
deepseek-v4-flash:cloud(ollama-cloud)Verdict: Blocking issues found — 3 reviewers: security, correctness, error-handling
🔒 Security — Minor issues
Now I have all the information needed to verify each finding. Let me compile my analysis.
Finding 1 verification: The
webhook_secretcolumn is stored as plaintext (line 31 ofpersona_store.go) and looked up directly viaWHERE webhook_secret = ?(line 115). The code comment inskill/skill.go(lines 170-176) explicitly acknowledges this design. However, the draft claims the column is "indexed" — it is NOT; there is no index onwebhook_secret(only indexes onowner_id,(owner_id, name), and(schedule, next_run_at)). The draft's line numbers (38-39) point to theidx_agents_owner_nameandidx_agents_schedindexes, not the webhook_secret column. The core concern (plaintext secret in DB) is valid and documented. Keep with corrections.Finding 2 verification:
SQL()atsqlite.go:47returns*sql.DBdirectly. Confirmed. Keep.Finding 3 verification:
skill_store.go:201discards the error with_ = ...Scan(&seq). If the query fails,seqstays 0, corrupting version ordering. Confirmed. Keep.Finding 4 verification:
MarkAgentScheduledRun(persona_store.go:159-166) andMarkScheduledRun(skill_store.go:184-192) both do read-modify-write outside a transaction. The draft's line numbers are off (says 152-166 and 230-240) but the substance is correct. Keep.All four findings survive. The verdict remains "Minor issues."
🔒 Security — Verdict: Minor issues
Findings
1.
contrib/store/persona_store.go:31,115— Webhook secret stored in plaintext, enabling direct lookupThe
webhook_secretcolumn is stored as plaintext.GetAgentByWebhookSecret(line 115) does a directWHERE webhook_secret = ?lookup. The code comment inskill/skill.go(lines 170-176) acknowledges this and justifies it as a performance trade-off (O(1) lookup vs. O(n) hash comparison). The secret is 64 hex chars (256 bits), which is strong enough as a bearer token. (Note: the column is not indexed — there is no index onwebhook_secret— so the lookup is a sequential scan, not an indexed lookup.)Impact: If the database is compromised, every webhook secret is immediately readable. An attacker with read access to the DB can enumerate all webhook secrets and invoke any skill's webhook endpoint without additional authentication. The documented mitigation (secret rotation via
RegenerateWebhookSecret) is reactive, not preventive.Suggested fix: If the lookup-by-secret performance requirement is real, store a SHA-256 hash of the secret in an indexed column and keep the full secret only in the JSON
datablob. The webhook handler hashes the incoming secret and looks up by hash — still O(1), still indexed, but the plaintext secret is never in an indexed column. (The full secret in the JSON blob is still readable on DB compromise, but that's a separate, harder-to-exploit concern since it requires parsing every row's JSON.)2.
contrib/store/sqlite.go:47—SQL()method exposes raw*sql.DB, enabling arbitrary SQL executionThe
SQL()accessor returns the underlying*sql.DBdirectly. Any code that holds a*DBreference can execute arbitrary SQL against the database, bypassing all seam abstractions.Impact: If a
*DBhandle is passed to untrusted code (e.g., a plugin, a user-authored skill tool that receives the DB handle), that code can read, modify, or destroy any table. This is a deliberate API escape hatch, but it widens the trust boundary beyond the seam interfaces.Suggested fix: Either remove
SQL()(callers who need raw access can usedatabase/sqldirectly with their own DSN), or add a prominent doc comment warning that holding a*DBgrants full SQL access and should never be passed to untrusted code.3.
contrib/store/skill_store.go:201— Error from seq query silently discarded inAppendVersionIf the query fails (e.g., database connection lost, context cancelled),
seqstays at0and the version is inserted withseq=0, corrupting the version ordering for that skill.Impact: Not directly a security vulnerability, but it can silently corrupt version history — a skill's versions could appear in the wrong order or be lost behind a
seq=0entry. This undermines the pinning and rollback mechanisms that depend on correct ordering.Suggested fix: Check the error from
Scanand return it:4.
contrib/store/persona_store.go:159-166andcontrib/store/skill_store.go:184-192—MarkAgentScheduledRun/MarkScheduledRunhave a TOCTOU race (read-then-write outside a transaction)Both methods read the current entity, modify two fields, and save it back — but not inside a transaction. Two concurrent scheduler ticks for the same skill/agent can race: both read the old state, both modify it, and the second
Saveoverwrites the first'sLastScheduledRunAt/NextRunAtvalues.Impact: A scheduled skill could be double-executed (both ticks see
next_run_at <= now()and both proceed to run) or a tick's bookkeeping update could be silently lost. In a scheduler context this is a correctness issue with security-adjacent consequences (resource exhaustion from double-runs, missed audit trail).Suggested fix: Wrap the read-modify-write in a transaction with
SELECT ... FOR UPDATE(SQLite doesn't supportFOR UPDATE, but wrapping in a transaction with an immediate upsert pattern works). Alternatively, use a singleUPDATE ... SET last_scheduled_run_at=?, next_run_at=? WHERE id=? AND next_run_at=?conditional update that returnsRowsAffectedto detect the race.🎯 Correctness — Minor issues
🎯 Correctness — Verdict: Minor issues
1.
AppendVersionseq computation has a TOCTOU race — duplicateseqvalues possiblecontrib/store/skill_store.go:199-201The
seqvalue is computed asSELECT COALESCE(MAX(seq),0)+1in a standalone query, then used in a separateINSERT— neither wrapped in a transaction. Two concurrentAppendVersioncalls for the sameskill_idcan both read the sameMAX(seq)and compute the sameseq+1. The table has no unique constraint on(skill_id, seq), so both INSERTs succeed with identicalseqvalues.ListVersionsBySkillorders byseq DESC, and ties produce undefined ordering — the "newest-first" contract breaks silently.Verified by: reading the code at lines 199-207; confirming no transaction wraps the pair; checking the
skill_versionsschema (no unique constraint on(skill_id, seq)).Suggested fix: Wrap the SELECT + INSERT in a transaction. The
seqcolumn should also get aUNIQUE(skill_id, seq)constraint as a safety net.2.
MarkScheduledRunhas a TOCTOU race (both skill and persona stores)contrib/store/skill_store.go:184-191andcontrib/store/persona_store.go:159-166Both
MarkScheduledRunimplementations do a read (Get/GetAgent) then a write (Save/SaveAgent) outside a transaction. Between the read and write, another concurrent caller could modify the same row, and the second write silently overwrites the first's changes. The budget store'sAddcorrectly uses a transaction for its read-modify-write, but these two scheduling methods do not.Verified by: reading both methods; confirming no
BeginTx/Commitwrapping; comparing withbudgetStore.Addwhich correctly uses a transaction.Suggested fix: Wrap the read + modify + write in a transaction. For the skill store, use
tx.QueryRowContext+tx.ExecContextinstead of the connection-levelGet/Save. For the persona store, do the same with the agent's fields.3. No material issues found in the budget store's rolling-window logic
The
Addmethod correctly uses>= 7*24*time.Hourfor rollover (matching thebudgetWindowconstant inbudget/storage.go:33), the transaction wrapping is correct, and the upsert properly resets counters on rollover. The test confirms the 7-day boundary. No correctness problem here.🧯 Error handling & edge cases — Blocking issues found
Corrected Review
Verdict: Blocking issues found
Blocking
contrib/store/skill_store.go:201— Silently discarded error from seq query inAppendVersionThe line
_ = s.db.QueryRowContext(ctx, …).Scan(&seq)discards any error from the query. If the context is cancelled, the database is unreachable, or any other error occurs,seqstays0and the insert proceeds withseq=0, corrupting version ordering for that skill.Fix: Check the error and return it:
contrib/store/skill_store.go:194-207— TOCTOU race on seq generation inAppendVersionThe
SELECT COALESCE(MAX(seq),0)+1and the subsequentINSERTare not in a transaction. Two concurrentAppendVersioncalls for the same skill can both read the sameMAX(seq)value (e.g., 5), then both insert withseq=6, producing duplicate seq values and breaking the "newest-first" ordering guarantee.Fix: Wrap the SELECT + INSERT in a transaction (
BeginTx/Commit/Rollback), exactly asbudgetStore.Adddoes for its own read-modify-write.contrib/store/persona_store.go:159-166andcontrib/store/skill_store.go:184-192— TOCTOU race inMarkAgentScheduledRun/MarkScheduledRunBoth methods read the entity via
Get/GetAgent, modify fields in Go, then callSave/SaveAgent— all outside a transaction. A concurrent save (e.g., another scheduled run completing, or an admin editing the skill) between the read and write will silently overwrite the other update.Fix: Use a transaction: read the row inside the transaction, modify, and upsert — or use an atomic
UPDATE … SET last_scheduled_run_at=?, next_run_at=? WHERE id=?that doesn't need a read-modify-write at all.Minor
contrib/store/budget_store.go:59-99— No validation ofsecondsUsedinAddThe method accepts
secondsUsed float64without checking forNaNor+Infvalues. The caller (DBBudget.Commit) guards against<= 0(catching negatives and-Inf), butNaNslips through (NaN comparisons are always false) and would corrupt theseconds_usedcolumn (NaN propagates through addition, making the row unrecoverable without manual SQL).Fix: Add a guard at the top of
Add:contrib/store/skill_store.go:194-208—AppendVersionhas no validation thatsv.SkillIDis non-emptyAn empty
SkillIDwould insert orphaned version rows that are never returned byListVersionsBySkill(which filters onskill_id = ?). TheMemoryimplementation has the same gap.Fix: Add a check at the top of
AppendVersion:Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 11m 22s
🪰 Gadfly review —
minimax-m3:cloud(ollama-cloud)Verdict: Blocking issues found — 3 reviewers: security, correctness, error-handling
🔒 Security — Minor issues
VERDICT: Minor issues
Plaintext webhook secret storage (
skill/skill.go:164-177) — Confirmed against the actual code. The comment at lines 171-176 explicitly states: "Why store the secret directly (not a hash): the webhook handler must look up the skill by the secret on every POST, which would require comparing every stored hash against the supplied secret — a per-call O(n_skills) operation. The secret is treated as a long random URL key (like a paste UUID); compromise is mitigated via RegenerateWebhookSecret rotation, not via storage hashing." The newcontrib/store/skill_store.go(lines 55-80) persists the entire*skill.Skillas a JSON blob indata, carrying the plaintext secret into the new SQLite-backed store as well. The design rationale is documented, but the reviewer is correct that any DB dump/replica/backup/SQLi leaks every webhook secret in cleartext, and that a SHA-256 indexed lookup with constant-time compare on ≤K rows would preserve the indexed-lookup property while removing plaintext-at-rest. Severity minor because the trade-off is explicit and the secret is high-entropy. Note: this code is pre-existing (the diff doesn't touchskill/skill.go), but the new SQLite persistence inherits the same exposure.No DSN validation in
contrib/store/sqlite.go:28-41— Confirmed.Open(dsn)passes the string straight tosql.Open("sqlite", dsn)with no sanitization or path canonicalization, and the doc at lines 28-30 says only "Open opens (creating if absent) a SQLite database at dsn …" without explicitly marking the DSN as trusted operator input. All current call sites pass host-controlled literals (:memory:in tests, a fixedfile:executus.db?...in the package example), so this isn't exploitable today, but a one-sentence doc clarification ("DSN is trusted operator input; do not construct from untrusted strings") would harden the contract against future config that mixes untrusted input. Severity minor.🎯 Correctness — Blocking issues found
Review — Lens: Correctness
VERDICT: Blocking issues found
The diff is largely a clean lift, but the SQLite
SkillStore.AppendVersionimplementation has a real concurrency bug that breaks the interface's "newest-first ordering key" contract, and two related read-modify-write methods have lost-update hazards.Findings
contrib/store/skill_store.go:194-208—AppendVersionraces onseq. The seq key is computed withSELECT COALESCE(MAX(seq),0)+1and then INSERTed in two separate statements, NOT wrapped in a transaction, and theskill_versionstable has noUNIQUE(skill_id, seq)constraint:Concurrent
AppendVersioncalls for the sameskill_idwill read the sameMAXand both INSERT with the sameseq. SQLite serializes the writes, both succeed (no UNIQUE constraint), andORDER BY seq DESCbecomes non-deterministic — violating the contract thatListVersionsBySkillreturns a deterministic newest-first ordering (the code comment explicitly labelsseqthe "newest-first ordering key";skill/skill_test.go:52enforces this invariant). The in-memory default holds a mutex inskill/memory.go:139-140so it doesn't have this bug; the new SQLite path does. Fix: wrap the SELECT+INSERT in aBEGIN IMMEDIATEtransaction, OR add aUNIQUE(skill_id, seq)constraint and retry on collision, OR derive seq from a monotonic counter.contrib/store/persona_store.go:159-166andcontrib/store/skill_store.go:184-192—Mark*ScheduledRunlost-update. Both perform a non-transactionalGet* → mutate → Save*:Concurrent invocations for the same agent/skill silently lose updates to
LastScheduledRunAt/NextRunAt. The interface contract atpersona/storage.go:110explicitly statesMarkAgentScheduledRun"atomically updates" those fields — the SQLite implementation violates it. The in-memory default holdsm.mu.Lock()(seepersona/memory.go:111-112andskill/memory.go:127-128) so this is a regression vs the default. Fix: wrapGet + field-write + Savein a transaction, or perform anUPDATE … WHERE id=?conditional update. The PR description highlights closing the budget store's TOCTOU window but the persona/skill analogues still have one.The seq race in
AppendVersionis the most severe issue — it can produce wrong ordering under any concurrent save — and the lost-update hazards are real but lower-severity follow-ups.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
Lens: Error handling & edge cases. Verified by reading
contrib/store/{sqlite,budget_store,persona_store,skill_store}.go,skill/memory.go,skill/skill.go,skill/skill_version.go,skill/store.go,budget/{budget,memory,storage}.go, andpersona/agent.go.Findings
skill/memory.go:34—Memory.Saveis a shallow copy.cp := *scopies scalar fields buts.Tools,s.Tags,s.SharedWith,s.InputSchema, ands.StateReactEmoji(confirmed present onskill.Skillat lines 52/58/54/290) continue to alias the caller's slice/map headers and backing arrays. A caller that mutatess.StateReactEmoji[...]orappends tos.ToolsafterSavereturns will silently mutate the stored skill.Memory.Get(line 48) returns a freshcp := *scopy, creating an asymmetric expectation. Fix: deep-copy the slice/map fields inSave.skill/memory.go:160—Memory.GetVersionByIDreturns a pointer to the stored entry.return &svexposes the internalbyVerIDmap value, so a caller can mutate it (sv.Version = "...").GetVersionByIDis the only method on theSkillStoreinterface that returns a pointer to the stored map value rather than a fresh copy. Fix: return a pointer to a copy.contrib/store/skill_store.go:200-201—AppendVersionhas a seq-assignment race.SELECT COALESCE(MAX(seq),0)+1is issued outside any transaction, then a separateINSERT. Two concurrent calls can both observe the sameMAX, then both insert with the sameseq, breaking the documented "newest-first" ordering invariant. Fix: wrap both in a transaction (thebudgetStore.Addpattern in the same package already shows the right shape).contrib/store/skill_store.go:200— ignoredScanerror._ = s.db.QueryRowContext(...).Scan(&seq)swallows any scan error, leavingseqat 0 and proceeding to insert with a duplicate seq. Combined with the race above, this lets the corruption stay invisible. Fix: check the error and return wrapped.contrib/store/persona_store.go:46—SaveAgentdoes not nil-checka.json.Marshal((*persona.Agent)(nil))returns"null", which is stored indata; subsequentGetAgentcalls unmarshal"null"into a zeroAgent(silently losing the row's "exists" semantics). Same edge case incontrib/store/skill_store.go:55(Save) andAppendVersion(svzero-value stored as"null"). Fix: reject nil/zero receivers with an explicit error before marshalling.contrib/store/sqlite.go:34—Openswallows theCloseerror afterPingfailure.sqldb.Close()return value is discarded. Standard Go idiom; the handle can't leak past the function return so impact is nil. Minor only.contrib/store/budget_store.go:59—Adddoes not validatesecondsUsed. A negative orNaNvalue flows into theREALcolumn unchecked; the in-memory store (budget/memory.go:35) likewise does not reject this. NegativeSecondsUsedwould never triggerErrBudgetExceededbecausebudget.DBBudget.Check(line 135) usesbud.SecondsUsed >= cap, and a negative value can't reach the cap. TheBudgetStorageinterface contract (budget/storage.go:20) should make this explicit. Fix: rejectsecondsUsed < 0 || math.IsNaN(secondsUsed)at the store boundary.contrib/store/skill_store.go:157-172—ListSharedWithaliasing.out := shared[:0]reuses the backing array of the slice returned bys.query. Currently safe becausesharedis a local not retained anywhere, but the pattern is fragile — a future change that keeps a reference tosharedwould silently corrupt it. Minor; flag only.No issues with
defer tx.Rollback()(the//nolint:errcheckcomment inbudget_store.go:64documents the idiom), the(nil, nil)Get contract for unknown users,sql.ErrNoRowsmapping topersona.ErrNotFound/skill.ErrNotFound, the in-memory mutex usage, or the transaction boundary inbudgetStore.Additself — those are all correct. The window-rollover boundary (>= 7dinAddvs< 7dinCheck) is consistent and intentional so that the comparison window is closed at the boundary.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 7m 53s
Pull request closed