refactor: coordination rebuild + schema normalization#6
Conversation
Big-bang refactor covering three tracks:
1. Coordination layer rebuild — replaces directive/report/journal/insight/
task/synthesis/consolidation packages with the new coordination model:
agent + todo + agent/note + hypothesis + obsidian/note + weekly. Vocabulary
discipline: task = inter-agent coordination, todo = personal GTD.
2. Schema normalization — migration 001 rewrite for naming precision:
- todo_items → todos, learning_items → learning_targets,
learning_plan_items → learning_plan_entries, notes → obsidian_notes
- notion_page_id/notion_task_id → external_provider + external_ref pair
across goals/milestones/projects/todos/obsidian_notes
- obsidian_notes.source → provenance, agent_notes.source → author,
hypotheses.source → author, contents.source → origin_ref,
tasks.source/target → requester/assignee
- learning_target_relations.source_item_id/target_item_id → anchor_id/related_id
- CHECK constraints: slug formats, email, URL schemes, hash formats,
non-empty text fields
- learning_sessions.updated_at added; bookmarks.curated_by FK to agents;
legacy_content_id dropped
3. sqlc full coverage — zero raw SQL outside internal/db/ and tests.
Converted agent, bookmark, stats, commitment area lookup, feed collection
stats. Added internal/stats/query.sql with 25 Stats* queries.
Verification: go build / vet / test all green; touched packages have 0 new
lint issues (pre-existing repo-wide issues in untouched packages unchanged).
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| if syncErr != nil { | ||
| return fmt.Errorf("syncing agent registry: %w", syncErr) |
There was a problem hiding this comment.
If agent.SyncToTable returns an error, it is immediately returned, but the error is not logged. This can make debugging startup failures more difficult, especially in production environments where returned errors may not be visible.
Recommendation: Log the sync error before returning:
if syncErr != nil {
logger.Error("agent registry sync failed", "error", syncErr)
return fmt.Errorf("syncing agent registry: %w", syncErr)
}| server := mcp.NewServer(pool, logger, | ||
| mcp.WithLocation(taipeiLoc), | ||
| mcp.WithParticipant(cfg.Participant), | ||
| mcp.WithRegistry(agentRegistry), | ||
| ) |
There was a problem hiding this comment.
The construction of the MCP server via mcp.NewServer does not validate the provided dependencies (e.g., pool, logger, taipeiLoc, cfg.Participant, agentRegistry). If any of these are nil or invalid due to misconfiguration, it could result in a runtime panic or undefined behavior later.
Recommendation: Add explicit validation for critical dependencies before server instantiation, or ensure that mcp.NewServer itself performs such validation and returns an error if any required argument is invalid.
| -- Used for double-complete detection: count today's todo_completed events for a specific todo item. | ||
| SELECT count(*)::int FROM events | ||
| WHERE event_type = @event_type | ||
| AND source_id LIKE @source_prefix || '%' |
There was a problem hiding this comment.
The use of source_id LIKE @source_prefix || '%' may result in a full table scan if there is no suitable index on source_id for prefix searches. This can significantly degrade performance for large tables.
Recommendation: Ensure there is a B-tree index on source_id or consider using the pg_trgm extension for more efficient prefix matching if the column is not already indexed appropriately.
| reports: report.NewStore(pool), | ||
| synth: synthesis.NewStore(pool), | ||
| loc: loc, | ||
| logger: logger, |
There was a problem hiding this comment.
Logger not validated for nil:
The logger field is assigned directly without checking for nil. If a nil logger is passed and later used, it could cause a runtime panic. Consider validating the logger parameter or providing a default logger to ensure robustness.
Recommended solution:
if logger == nil {
logger = slog.Default()
}| if err != nil { | ||
| return |
There was a problem hiding this comment.
Silent error handling:
The error from h.todos.CompletedItemsDetailSince is silently ignored by returning early. This can obscure operational issues and make debugging difficult. Consider logging the error or propagating it to the caller to ensure failures are visible and actionable.
Recommended solution:
if err != nil {
// Log the error or propagate it
log.Printf("failed to fetch completed items: %v", err)
return
}| -- name: CreateAgentNote :one | ||
| -- Insert an agent note entry. | ||
| INSERT INTO agent_notes (kind, author, content, metadata, entry_date) | ||
| VALUES (@kind::agent_note_kind, @author, @content, @metadata, @entry_date) |
There was a problem hiding this comment.
Metadata Field Validation
The CreateAgentNote query (line 4) inserts the metadata field. If metadata is intended to be JSON, ensure that the application layer validates the JSON structure before insertion to prevent malformed data. This will help maintain data integrity and avoid downstream parsing errors.
| func (s *Store) Create(ctx context.Context, p *CreateParams) (*Note, error) { | ||
| var metaBytes json.RawMessage | ||
| if p.Metadata != nil { | ||
| b, err := json.Marshal(p.Metadata) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshaling metadata: %w", err) | ||
| } | ||
| metaBytes = b | ||
| } | ||
|
|
||
| r, err := s.q.CreateAgentNote(ctx, db.CreateAgentNoteParams{ | ||
| Kind: db.AgentNoteKind(p.Kind), | ||
| Author: p.Author, | ||
| Content: p.Content, | ||
| Metadata: metaBytes, | ||
| EntryDate: p.EntryDate, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating agent note: %w", err) | ||
| } | ||
| return rowToNote(&r) | ||
| } |
There was a problem hiding this comment.
Lack of Input Validation in Create
The Create method does not validate required fields in the CreateParams struct (such as Kind, Author, or Content). This could allow invalid or incomplete data to be inserted into the database, potentially causing downstream errors or violating data integrity constraints.
Recommendation:
Add explicit validation for required fields before attempting to marshal metadata or insert into the database. For example:
if p.Kind == "" || p.Author == "" || p.Content == "" {
return nil, errors.New("missing required fields: kind, author, or content")
}| func (s *Store) NotesInRange(ctx context.Context, start, end time.Time, kindFilter *Kind, sourceFilter *string) ([]Note, error) { | ||
| kindArg := db.NullAgentNoteKind{} | ||
| if kindFilter != nil { | ||
| kindArg.AgentNoteKind = db.AgentNoteKind(*kindFilter) | ||
| kindArg.Valid = true | ||
| } | ||
| rows, err := s.q.AgentNotesByDateRange(ctx, db.AgentNotesByDateRangeParams{ | ||
| StartDate: start, | ||
| EndDate: end, | ||
| Kind: kindArg, | ||
| Author: sourceFilter, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("listing agent notes in range: %w", err) | ||
| } | ||
| notes := make([]Note, 0, len(rows)) | ||
| for i := range rows { | ||
| n, err := rowToNote(&rows[i]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| notes = append(notes, *n) | ||
| } | ||
| return notes, nil | ||
| } |
There was a problem hiding this comment.
Ambiguous Handling of sourceFilter in NotesInRange
In the NotesInRange method, the sourceFilter parameter is passed directly as the Author argument to the query. If sourceFilter is nil, the behavior depends on the underlying query implementation. This could result in returning all authors instead of filtering, which may not be the intended behavior.
Recommendation:
Clarify and document the behavior when sourceFilter is nil. If the intention is to return all authors when sourceFilter is not provided, ensure the underlying query handles this explicitly. Otherwise, add logic to prevent unintended broad queries.
| func (r *Registry) SetStatus(name Name, s Status) { | ||
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
| if a, ok := r.byName[name]; ok { | ||
| a.Status = s | ||
| r.byName[name] = a | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential for Incorrect Usage of SetStatus After Startup
The SetStatus method is documented as safe to call only during startup, but there is no enforcement of this constraint. If SetStatus is inadvertently called after HTTP serving begins, it could lead to inconsistent state or subtle concurrency bugs, since the registry is intended to be read-only after initialization.
Recommendation:
Consider adding a mechanism to enforce or detect post-startup calls, such as a boolean flag (e.g., locked) that is set after initialization, and panic or return an error if SetStatus is called when not allowed. This will help prevent accidental misuse.
| func sortAgentsByName(agents []Agent) { | ||
| // Small N — insertion sort avoids pulling in sort.Slice just for this. | ||
| for i := 1; i < len(agents); i++ { | ||
| for j := i; j > 0 && agents[j-1].Name > agents[j].Name; j-- { | ||
| agents[j-1], agents[j] = agents[j], agents[j-1] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Insertion Sort May Not Scale for Large Agent Lists
The sortAgentsByName function uses insertion sort, which is efficient for small lists but has O(n^2) complexity. If the number of agents increases, this could become a performance bottleneck.
Recommendation:
If scalability is a concern, consider replacing this with Go's built-in sort.Slice for better performance on larger lists:
sort.Slice(agents, func(i, j int) bool { return agents[i].Name < agents[j].Name })1 participant
Big-bang refactor covering three tracks:
Coordination layer rebuild — replaces directive/report/journal/insight/ task/synthesis/consolidation packages with the new coordination model: agent + todo + agent/note + hypothesis + obsidian/note + weekly. Vocabulary discipline: task = inter-agent coordination, todo = personal GTD.
Schema normalization — migration 001 rewrite for naming precision:
sqlc full coverage — zero raw SQL outside internal/db/ and tests. Converted agent, bookmark, stats, commitment area lookup, feed collection stats. Added internal/stats/query.sql with 25 Stats* queries.
Verification: go build / vet / test all green; touched packages have 0 new lint issues (pre-existing repo-wide issues in untouched packages unchanged).