Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a persistent agent memory subsystem (file-backed), new admin REST endpoints for global and per‑DAG memory, integrates memory into session/system prompts, updates sync to publish selected DAG IDs and treat memory .md files, and adds frontend UI for memory management and selective DAG publishing. Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIHandler as API Handler\n(internal/service/frontend/api/v1/agent_memory.go)
participant MemStore as MemoryStore\n(internal/persis/filememory)
participant FileSystem as File System
participant SessionMgr as SessionManager\n(internal/agent/session.go)
Client->>APIHandler: PUT /settings/agent/memory\n{content}
APIHandler->>APIHandler: require admin & authz
APIHandler->>MemStore: SaveGlobalMemory(ctx, content)
MemStore->>FileSystem: write memory/MEMORY.md (atomic, truncate)
FileSystem-->>MemStore: success
MemStore-->>APIHandler: nil
APIHandler->>APIHandler: audit log
APIHandler-->>Client: 200 OK
rect rgba(100, 150, 200, 0.5)
Note over Client,SessionMgr: Session creation uses memory
Client->>SessionMgr: Start new session (DAG contexts)
SessionMgr->>MemStore: LoadGlobalMemory(ctx)
MemStore->>FileSystem: read memory/MEMORY.md
FileSystem-->>MemStore: content
MemStore-->>SessionMgr: content
SessionMgr->>SessionMgr: GenerateSystemPrompt(..., memory)
SessionMgr-->>Client: Session ready
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/agent/api.go (1)
611-625:⚠️ Potential issue | 🟡 Minor
DAGNameis not propagated to reactivated sessions.
reactivateSessionpassesMemoryStore(Line 624) but doesn't setDAGNameinSessionManagerConfig. Reactivated sessions will load global memory but never DAG-specific memory, even if the original session had a DAG context.If the DAG name was persisted alongside the session (or could be inferred from stored messages), it could be restored here. Otherwise, consider documenting this as a known limitation.
internal/gitsync/service.go (2)
557-579:⚠️ Potential issue | 🟠 MajorValidate each DAG’s publishable state before staging.
PublishAllskipsvalidatePublishable, so conflicted or already-synced DAGs can be published without a force flag. That’s a behavior change vs single-DAG publish and can overwrite conflicts unintentionally.🔧 Proposed fix (per-DAG validation before staging)
for _, dagID := range dagIDs { + dagState := state.DAGs[dagID] + if dagState == nil { + result.Errors = append(result.Errors, SyncError{DAGID: dagID, Message: "DAG not found"}) + continue + } + if err := s.validatePublishable(dagState, dagID, false); err != nil { + result.Errors = append(result.Errors, SyncError{DAGID: dagID, Message: err.Error()}) + continue + } dagFilePath := s.dagIDToFilePath(dagID) repoFilePath := s.dagIDToRepoPath(dagID)
587-602:⚠️ Potential issue | 🟠 MajorChange AddAndCommit call to avoid re-staging all files.
The selective staging in lines 590-596 is undermined by
AddAndCommit(".", message)on line 601. TheAddAndCommitimplementation callswt.Add(filePath), so passing"."stages all changes in the working tree, bypassing the intent to commit only the files instagedFiles. Pass specific files or refactor to use a commit-only variant that doesn't re-stage.
🤖 Fix all issues with AI agents
In `@api/v1/api.yaml`:
- Around line 6399-6412: The OpenAPI model SyncPublishAllRequest now requires
dagIds but the /sync/publish-all operation summary/description still implies
"publish all modified DAGs"; update the /sync/publish-all operation in api.yaml
to reference SyncPublishAllRequest and change its summary/description to state
it publishes the specified/selected DAGs (accepts dagIds array) — locate the
operation for path "/sync/publish-all" and modify its summary/description to
clearly mention that dagIds are required and only those DAGs will be published.
- Around line 3985-4083: Replace the inline dagName parameter schema (the
parameter named dagName in the get/put/delete endpoints, including operationId
updateAgentDAGMemory and deleteAgentDAGMemory) with a reference to the canonical
DAGName schema to ensure consistent validation: remove the current inline schema
block (type/minLength) and set schema: $ref: "#/components/schemas/DAGName"
while preserving the parameter name, in, required and description fields.
In `@internal/agent/system_prompt.txt`:
- Around line 49-53: The <memory_paths> block renders broken paths when
.Memory.MemoryDir is empty; wrap the entire <memory_paths> section in a template
conditional that checks .Memory.MemoryDir (e.g., {{if .Memory.MemoryDir}} ...
{{end}}) so the "Memory directory" and "Global memory" lines (and the existing
{{if .Memory.DAGName}} DAG memory line) are only rendered when MemoryDir is set;
keep the inner DAGName conditional as-is.
In `@internal/service/frontend/server.go`:
- Around line 263-266: The code creates two separate filememory.Store instances
by calling filememory.New(cfg.Paths.DAGsDir) in multiple places (once when
building allAPIOptions via apiv1.WithAgentMemoryStore and again inside
initAgentAPI), causing unsynchronized access to the same files; fix it by
creating a single shared store (call filememory.New once with
cfg.Paths.DAGsDir), reuse that instance for apiv1.WithAgentMemoryStore, and
change initAgentAPI to accept the shared filememory.Store (remove its internal
call to filememory.New) so both the REST API and Agent API use the same Store
and mutex.
In `@ui/src/pages/agent-memory/index.tsx`:
- Around line 155-161: The current implementation returns a full-page spinner
when isLoading is true (the Loader2 block), which hides the entire page;
instead, remove the early return and always render the page skeleton (headings,
controls, empty textareas) so stale data remains visible, keep using isLoading
to show inline loading indicators next to or inside the specific data
sections/components (replace the centered Loader2 overlay with small loaders or
skeletons inside the memory list, detail pane, or form components), and ensure
components like the memory list rendering logic check isLoading to display their
own inline placeholders rather than hiding the whole page.
- Around line 255-276: The DAG chip currently nests a clickable trash <button>
inside the main chip <button>, which is invalid HTML; update the chip to be a
non-button container (e.g., a <div> or a semantic element with role="button" and
proper keyboard handlers) that uses the existing handleSelectDAG(name) for chip
activation and keep the trash control as a separate interactive element that
calls setDeletingDAG(name); ensure selectedDAG checks and styling remain tied to
the container and maintain accessible keyboard handling
(onKeyDown/role/aria-pressed) and focusability for the container while
preserving the Trash2 icon button as an independent clickable element.
In `@ui/src/pages/git-sync/index.tsx`:
- Around line 143-153: The effect that auto-selects publishable DAGs (useEffect
referencing status?.dags) is re-running on every poll because the fetched object
reference changes; change it to only auto-select on first load or when the
actual set of publishable IDs changes: add a persisted ref (e.g.,
initialAutoSelectedRef) to run the selection only once on mount OR compute the
current publishable ID set inside the effect and compare it to a
previousPublishableIds ref (or memoized value) and call
setSelectedDags(publishable) only when the ID set differs; update the effect
body that currently iterates over Object.entries(status.dags) and the dependency
list so it no longer blindly depends on status?.dags reference changes.
🧹 Nitpick comments (8)
ui/src/pages/git-sync/index.tsx (1)
270-281:useMemoonfilteredDagsis effectively a no-op.
filteredDagsis recomputed inline on every render (lines 263-268), producing a new array reference each time. Passing it as a dependency touseMemomeans the memo always recalculates. Consider memoizingfilteredDagsitself, or derivepublishableDagIdsfromstatus?.dagsandfilterdirectly.internal/agent/system_prompt_test.go (1)
1-7: Consider usingrequireinstead ofassertfor consistency with coding guidelines.The coding guidelines recommend
stretchr/testify/requirefor assertions. The existing tests already useassert, so this is a pre-existing pattern, but new test additions could start adoptingrequirefor fail-fast behavior.As per coding guidelines, "Use
stretchr/testify/requirefor assertions and shared fixtures frominternal/testinstead of duplicating mocks".internal/cmd/sync.go (1)
232-244: LGTM — CLI now collects publishable DAG IDs before batch publish.The status-then-filter-then-publish flow is correct and consistent with the frontend's approach. One minor consideration: if no DAGs match (empty
dagIDs), the CLI still callsPublishAll— you may want to short-circuit with a message like "No modified DAGs to publish."Optional early exit
var dagIDs []string for id, dagState := range status.DAGs { if dagState.Status == gitsync.StatusModified || dagState.Status == gitsync.StatusUntracked { dagIDs = append(dagIDs, id) } } + if len(dagIDs) == 0 { + fmt.Println("No modified or untracked DAGs to publish") + return nil + } result, err = syncSvc.PublishAll(ctx, message, dagIDs)internal/service/frontend/api/v1/sync.go (1)
122-129: Add validation in the handler to distinguish client error (empty dagIDs) from internal errors.While
PublishAlldoes validate emptydagIDsand returnsErrNoChanges, the handler converts this error viainternalError(err), which sends a 500 response. Since an emptydagIdsarray is a client error, not a server error, returning a 400 Bad Request from the handler would be more semantically correct and provide a clearer API contract.Proposed validation
var message string var dagIDs []string if req.Body != nil { message = valueOf(req.Body.Message) dagIDs = req.Body.DagIds } + if len(dagIDs) == 0 { + return nil, &Error{ + Code: api.ErrorCodeBadRequest, + Message: "at least one DAG ID is required", + HTTPStatus: http.StatusBadRequest, + } + } + result, err := a.syncService.PublishAll(ctx, message, dagIDs)internal/agent/session.go (1)
385-401: Silently discarded errors from memory store could hide configuration issues.Lines 390 and 393 discard errors from
LoadGlobalMemoryandLoadDAGMemory. While it's reasonable to proceed without memory if loading fails, a debug log would help with troubleshooting.♻️ Proposed fix: add debug logging for memory load failures
func (sm *SessionManager) loadMemory() MemoryContent { if sm.memoryStore == nil { return MemoryContent{} } - global, _ := sm.memoryStore.LoadGlobalMemory(context.Background()) + global, err := sm.memoryStore.LoadGlobalMemory(context.Background()) + if err != nil { + sm.logger.Debug("failed to load global memory", "error", err) + } var dagMem string if sm.dagName != "" { - dagMem, _ = sm.memoryStore.LoadDAGMemory(context.Background(), sm.dagName) + dagMem, err = sm.memoryStore.LoadDAGMemory(context.Background(), sm.dagName) + if err != nil { + sm.logger.Debug("failed to load DAG memory", "dag", sm.dagName, "error", err) + } } return MemoryContent{ GlobalMemory: global,internal/service/frontend/server.go (1)
263-266: Silent error swallowing — inconsistent with the Agent API path.Line 264 silently discards the error from
filememory.New, while the equivalent block at Lines 420-424 logs a warning. Add a warning log here for consistency and debuggability.♻️ Proposed fix
// Wire up agent memory store for the REST API - if memoryStore, err := filememory.New(cfg.Paths.DAGsDir); err == nil { + memoryStore, err := filememory.New(cfg.Paths.DAGsDir) + if err != nil { + logger.Warn(ctx, "Failed to create REST API memory store", tag.Error(err)) + } else { allAPIOptions = append(allAPIOptions, apiv1.WithAgentMemoryStore(memoryStore)) }ui/src/pages/agent-memory/index.tsx (1)
177-187: Error and success banners never auto-dismiss.The
errorandsuccessmessages stay visible indefinitely. Consider auto-clearing thesuccessmessage after a few seconds so it doesn't clutter the UI for users performing multiple operations.♻️ Example: auto-dismiss success messages
useEffect(() => { if (!success) return; const timer = setTimeout(() => setSuccess(null), 3000); return () => clearTimeout(timer); }, [success]);ui/src/api/v1/schema.ts (1)
3013-3026: PreferDAGNamefor memory DAG identifiers.
dagNameanddagMemoriesare typed as plain strings, while other DAG endpoints use the sharedDAGNameschema/parameter. Reusing it keeps validation/typing consistent in generated clients (update the OpenAPI source since this file is generated).♻️ Suggested schema alignment
- dagMemories?: string[]; + dagMemories?: components["schemas"]["DAGName"][]; - AgentDAGMemoryResponse: { - dagName: string; + AgentDAGMemoryResponse: { + dagName: components["schemas"]["DAGName"]; content: string; }; - dagName: string; + dagName: components["parameters"]["DAGName"];Also applies to: 8014-8133
|
@coderabbitai perform full review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/agent/system_prompt.go (1)
56-58:⚠️ Potential issue | 🟡 MinorFallback prompt uses wrong identity name.
The fallback says
"You are Hermio"but the system prompt template (line 2 ofsystem_prompt.txt) identifies the agent as"Tsumugi". If template rendering ever fails, the agent would introduce itself with a different name.Proposed fix
func fallbackPrompt(env EnvironmentInfo) string { - return "You are Hermio, an AI assistant for DAG workflows. DAGs Directory: " + env.DAGsDir + return "You are Tsumugi, an AI assistant for DAG workflows. DAGs Directory: " + env.DAGsDir }internal/agent/api.go (1)
611-625:⚠️ Potential issue | 🟠 MajorReactivated sessions lose per-DAG memory context —
DAGNameis not restored.
reactivateSessionpassesMemoryStore(line 624) but omitsDAGName. When the session loop is recreated on the next message,loadMemory()will only load global memory, silently losing the per-DAG memory that was available in the original session.If
DAGNameisn't persisted in the session store, consider storing it (or the original DAG contexts) so reactivated sessions can restore their memory scope.#!/bin/bash # Check if Session struct or store has a DAGName/DAGContext field echo "=== Session struct ===" ast-grep --pattern 'type Session struct { $$$ }' echo "=== SessionStore interface ===" ast-grep --pattern 'type SessionStore interface { $$$ }' echo "=== CreateSession usage ===" rg -n 'CreateSession' --type=go -C3internal/gitsync/service.go (1)
539-623:⚠️ Potential issue | 🟠 MajorRemove the re-staging call to
AddAndCommit; commit the already-staged files directly.
PublishAllcorrectly stages only the selected files (lines 572–576), butAddAndCommit(".", message)immediately re-runswt.Add(".")internally, which stages all repository changes and defeats selective staging. Instead of callingAddAndCommit(".", ...), directly commit the staged files usingwt.Commit(message, ...)to preserve the selective staging.
🤖 Fix all issues with AI agents
In `@internal/agent/system_prompt.txt`:
- Around line 155-171: The <memory_management> block references
{{.Memory.MemoryDir}} and will render broken paths when memory isn't configured;
wrap the entire <memory_management> section (including the Paths lines and rules
mentioning MEMORY.md) in a conditional check like {{if .Memory.MemoryDir}} ...
{{end}} so it only renders when MemoryDir is set, and ensure the guidance still
mentions MEMORY.md and read/patch commands inside that conditional; locate the
<memory_management> section in the template and apply the conditional around its
contents to prevent displaying empty/broken paths.
In `@internal/cmd/sync.go`:
- Around line 233-246: The publishAll branch collects dagIDs from status.DAGs
and may call syncSvc.PublishAll with an empty slice, causing a validation error;
before calling syncSvc.PublishAll (in the block that uses publishAll, status :=
syncSvc.GetStatus(...) and builds dagIDs), check if dagIDs is empty and instead
print a friendly message like "No modified or untracked DAGs to publish" and
return early (nil) to avoid calling syncSvc.PublishAll with an empty dagIDs
slice; update the logic around the dagIDs variable and the PublishAll call to
perform this pre-check.
In `@internal/service/frontend/api/v1/agent_memory.go`:
- Around line 18-41: Rename the package-level error variables to follow the
Err... convention: change errAgentMemoryNotAvailable to
ErrAgentMemoryNotAvailable, errFailedToLoadMemory to ErrFailedToLoadMemory,
errFailedToSaveMemory to ErrFailedToSaveMemory, and errFailedToDeleteMemory to
ErrFailedToDeleteMemory; update all references to these symbols (e.g., usages in
functions like LoadAgentMemory, SaveAgentMemory, DeleteAgentMemory or any
handlers) to the new names and run a build to catch any remaining references.
In `@ui/src/api/v1/schema.ts`:
- Around line 2848-2854: The schema change made dagIds required in
SyncPublishAllRequest will break clients calling /sync/publish-all without that
field; update the OpenAPI source (not this generated file) to make dagIds
optional (e.g., dagIds?: string[]) or add a new request schema/endpoint, and
ensure the server handler for /sync/publish-all (which consumes
SyncPublishAllRequest) implements defaulting behavior (treat missing dagIds as
"all") so backward-compatible requests continue to work; regenerate
ui/src/api/v1/schema.ts from the updated OpenAPI spec.
In `@ui/src/pages/git-sync/index.tsx`:
- Around line 270-281: filteredDags is recreated each render so
publishableDagIds' useMemo is ineffective; fix by memoizing filteredDags first
(e.g., wrap the computation that produces filteredDags in useMemo) with stable
dependencies like status?.dags and filter, then keep publishableDagIds as a
useMemo that depends on the memoized filteredDags (or alternatively derive
publishableDagIds directly from status?.dags and filter in one useMemo). Update
related logic that uses publishableDagIds — allPublishableSelected and
handleToggleSelectAll — to depend on the memoized value so their memoization
becomes effective.
🧹 Nitpick comments (4)
internal/agent/session.go (1)
385-402: Silently swallowed errors inloadMemorymay hinder debugging.
LoadGlobalMemoryandLoadDAGMemoryerrors are discarded (lines 391, 394). Since this is supplementary data, not blocking is reasonable, but a debug-level log would help diagnose cases where memory unexpectedly doesn't appear in prompts.♻️ Suggested improvement
func (sm *SessionManager) loadMemory() MemoryContent { if sm.memoryStore == nil { return MemoryContent{} } ctx := context.Background() - global, _ := sm.memoryStore.LoadGlobalMemory(ctx) + global, err := sm.memoryStore.LoadGlobalMemory(ctx) + if err != nil { + sm.logger.Debug("failed to load global memory", "error", err) + } var dagMem string if sm.dagName != "" { - dagMem, _ = sm.memoryStore.LoadDAGMemory(ctx, sm.dagName) + dagMem, err = sm.memoryStore.LoadDAGMemory(ctx, sm.dagName) + if err != nil { + sm.logger.Debug("failed to load DAG memory", "error", err, "dag", sm.dagName) + } }ui/src/pages/agent-memory/index.tsx (1)
33-35:useEffectdependency onappBarContextobject may cause unnecessary re-runs.
appBarContextis likely a new object reference on each render, causingsetTitleto be called repeatedly. Depend onappBarContext.setTitleinstead, or extractsetTitlefrom the context before the effect.♻️ Suggested fix
+ const { setTitle } = appBarContext; + useEffect(() => { - appBarContext.setTitle('Agent Memory'); - }, [appBarContext]); + setTitle('Agent Memory'); + }, [setTitle]);ui/src/api/v1/schema.ts (2)
3013-3031: Prefer DAGName typing for memory identifiers.Using the existing
DAGNameschema improves consistency and validation with the rest of the API. Update in the OpenAPI source so the generated types follow suit.♻️ Suggested typing refinement
AgentMemoryResponse: { /** `@description` Content of global MEMORY.md */ globalMemory?: string; /** `@description` List of DAG names that have memory files */ - dagMemories?: string[]; + dagMemories?: components["schemas"]["DAGName"][]; /** `@description` Root memory directory path */ memoryDir?: string; }; AgentDAGMemoryResponse: { - dagName: string; + dagName: components["schemas"]["DAGName"]; content: string; };
8013-8016: UseDAGNameparameter typing for DAG memory routes.This keeps the DAG path parameters consistent with other DAG endpoints and preserves regex validation in the generated types. Update the OpenAPI source so the generator emits the stronger type.
♻️ Suggested typing refinement
path: { /** `@description` Name of the DAG */ - dagName: string; + dagName: components["parameters"]["DAGName"]; };Also applies to: 8067-8070, 8131-8134
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1659 +/- ##
==========================================
- Coverage 70.18% 69.70% -0.48%
==========================================
Files 344 345 +1
Lines 38257 38662 +405
==========================================
+ Hits 26851 26950 +99
- Misses 9273 9355 +82
- Partials 2133 2357 +224
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
New Features
Improvements