feat: add extensability#13
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a full extension subsystem (manifest format, registry, manager with subprocess supervision and capability model), Host API and daemon/UDS/CLI wiring for extension lifecycle, OpenAPI/TypeScript codegen tooling, API contract/handler migrations, ACP/subprocess integration updates, skills/hooks enhancements, DB schema changes, many tests, and a static docs viewer. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Extension CLI
participant Daemon as Daemon (UDS API)
participant ExtSvc as ExtensionService
participant Reg as Registry
participant Mgr as Extension Manager
participant Sub as Subprocess
participant HostAPI as Host API
CLI->>Daemon: POST /api/extensions (install path, checksum)
Daemon->>ExtSvc: Install(req)
ExtSvc->>Reg: Install(manifest, path, checksum)
Reg-->>ExtSvc: OK
ExtSvc->>Mgr: Reload()
Mgr->>Mgr: Load registry entries
Mgr->>Sub: Launch subprocess for extension
Sub->>HostAPI: initialize (grant actions/security)
HostAPI->>Mgr: Validate capabilities
Mgr->>Mgr: Register hooks/agents/skills
Mgr-->>ExtSvc: Ready
ExtSvc-->>Daemon: ExtensionPayload
Daemon-->>CLI: 201 Created + payload
sequenceDiagram
participant Sub as Extension Subprocess
participant HostAPI as Host API
participant Sessions as SessionManager
rect rgba(180,200,255,0.5)
Sub->>HostAPI: sessions/list {workspace}
HostAPI->>HostAPI: Check capability (CapabilityChecker)
HostAPI->>HostAPI: Rate limit check
HostAPI->>Sessions: List(params)
Sessions-->>HostAPI: SessionSummary[]
HostAPI-->>Sub: Result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/acp/client.go (1)
141-182:⚠️ Potential issue | 🟠 MajorThread the caller context through
spawnProcesstosubprocess.Launchand process lifecycle.
Start(ctx, ...)receives a caller context, butspawnProcessusescontext.Background()for bothsubprocess.Launch(line 152) andprocCtxinitialization (line 164). This breaks cancellation propagation: if the caller cancels during startup, the subprocess launch and early process lifecycle cannot be aborted. Additionally,subprocess.Launchexplicitly validates its context and expects a meaningful context from callers.Required changes:
- Add
ctx context.Contextparameter tospawnProcesssignature- Pass caller context to
subprocess.Launchinstead ofcontext.Background()- Derive
procCtxfrom caller context:context.WithCancel(ctx)instead ofcontext.Background()As per coding guidelines: "Use context.Context as first argument to functions crossing runtime boundaries — avoid context.Background() outside main and focused tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client.go` around lines 141 - 182, spawnProcess currently uses context.Background() for subprocess.Launch and procCtx which prevents cancellation propagation; change the signature of spawnProcess to accept ctx context.Context (placing context first), pass that ctx into subprocess.Launch instead of context.Background(), and create procCtx with procCtx, cancelProcess := context.WithCancel(ctx) (so the process lifecycle derives from the caller context); update all callers (e.g., Start(ctx, ...)) to pass their ctx into spawnProcess so cancellation and subprocess.Launch validation work correctly.internal/hooks/normalize.go (1)
106-124:⚠️ Potential issue | 🟡 MinorReject
WorkingDiroutside subprocess hooks.The new field is normalized, but the validation path still only guards
Command,Args, andEnv. A non-subprocess hook can now carryWorkingDirand pass validation even though the field is meaningless there.💡 Proposed fix
- if kind != HookExecutorSubprocess && (decl.Command != "" || len(decl.Args) > 0 || len(decl.Env) > 0) { + if kind != HookExecutorSubprocess && (decl.Command != "" || len(decl.Args) > 0 || len(decl.Env) > 0 || decl.WorkingDir != "") { return "", fmt.Errorf("hooks: hook %q shell command fields require a subprocess executor", decl.Name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/hooks/normalize.go` around lines 106 - 124, sanitizedHookDecl currently normalizes WorkingDir but doesn't reject it for non-subprocess hooks; update the validation in sanitizedHookDecl (the same place that guards Command, Args, and Env) to return an error when HookDecl.WorkingDir is non-empty and HookDecl.ExecutorKind is not the subprocess kind (use the same ExecutorKind enum/constant you use elsewhere), so non-subprocess hooks cannot carry a WorkingDir; ensure the error message references WorkingDir and ExecutorKind for clarity.internal/skills/registry.go (1)
99-107:⚠️ Potential issue | 🟠 MajorExternal skills are now resolvable, but not mutable.
Get,List, andForWorkspacecan now surface external skills, butSetEnabledstill only updates workspace cache entries orr.globalSkills. The API can therefore resolve an external skill and then fail to enable/disable it withskill not found, and disabled-state overlays never apply to those entries.Also applies to: 111-118, 167-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/skills/registry.go` around lines 99 - 107, Get/List/ForWorkspace now return external skills but SetEnabled still only mutates workspace cache entries or r.globalSkills, causing "skill not found" when toggling external skills; update SetEnabled to, when a skill lookup fails in workspace cache/globalSkills, call lookupSkillLocked (or the same resolver used by Get) to find external skills, cloneSkill to create a mutable copy, insert that copy into the appropriate workspace overlay (or r.globalSkills if global), then apply the enabled/disabled change and persist/update caches; ensure the same resolution logic is used as in Get/List/ForWorkspace so overlays correctly apply to external entries.
🧹 Nitpick comments (14)
docs/ideas/anp/index.html (1)
227-227: Accepted upload types and parser behavior are inconsistent.Line 227 accepts
.json, butparseJsonl(Line 269 onward) only handles JSONL lines. Regular JSON files will fail with a parse error. Either narrow accepted types or add JSON-document fallback parsing.Also applies to: 269-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ideas/anp/index.html` at line 227, The file input element with id "file" currently accepts ".json" but the parser function parseJsonl only handles newline-delimited JSON (JSONL), causing regular JSON files to fail; fix by either (A) narrowing the accept attribute on the <input id="file"> to only ".jsonl,.ndjson,application/jsonl,text/plain" or (B) adding a JSON fallback in parseJsonl: detect if the payload contains newline-delimited records vs a single JSON document/array, and if it’s a single JSON object/array, parse with JSON.parse and normalize to the expected record array before continuing; update parseJsonl (the function starting around line 269) to implement the detection and fallback parsing.internal/skills/loader.go (1)
52-63: Addcontext.Contextto the new disk-backed parser entrypoint.Line 52 introduces a new file-reading API without cancellation support. Please make this
ParseSkillFileWithSource(ctx context.Context, path string, source SkillSource)and threadctxthrough the read/parse path to keep runtime-boundary calls cancelable.As per coding guidelines,
Pass context.Context as first argument to functions crossing runtime boundaries — avoid context.Background() outside main and focused tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/skills/loader.go` around lines 52 - 63, Change the ParseSkillFileWithSource entrypoint to accept a context first: ParseSkillFileWithSource(ctx context.Context, path string, source SkillSource), update callers accordingly, and propagate ctx into downstream IO/parse calls (replace readSkillFile(path) with a context-aware read like readSkillFile(ctx, path) and pass ctx into parseSkillDocument as well). Modify readSkillFile and parseSkillDocument signatures to accept context.Context (or add context-aware wrappers) and ensure all internal calls and tests pass the ctx rather than using context.Background(); this keeps runtime-bound file-read and parse operations cancelable and follows the project's context-first convention.internal/cli/client.go (1)
513-522: Minor: Redundantstrings.TrimSpaceon name parameter.The callers
EnableExtensionandDisableExtensionalready callstrings.TrimSpace(name)before passing toextensionAction. The additionalTrimSpaceon line 517 is harmless but redundant.Optional cleanup
func (c *unixSocketClient) extensionAction(ctx context.Context, name string, action string) (ExtensionRecord, error) { var response struct { Extension ExtensionRecord `json:"extension"` } - path := "/api/extensions/" + url.PathEscape(strings.TrimSpace(name)) + "/" + strings.TrimSpace(action) + path := "/api/extensions/" + url.PathEscape(name) + "/" + action if err := c.doJSON(ctx, http.MethodPost, path, nil, nil, &response); err != nil { return ExtensionRecord{}, err } return response.Extension, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/client.go` around lines 513 - 522, The extensionAction function redundantly trims whitespace from the name parameter (strings.TrimSpace) even though callers like EnableExtension and DisableExtension already pass a trimmed name; remove the extra strings.TrimSpace call in extensionAction so path is built from the provided name (leave trimming responsibility to callers) and ensure path := "/api/extensions/" + url.PathEscape(name) + "/" + strings.TrimSpace(action) remains correctly escaped for the action segment.internal/cli/extension.go (1)
319-333: Silent error swallowing when loading manifest inlocalExtensionRecord.The manifest loading error at line 329 is silently ignored. While the extension info can be returned without the manifest, this could hide legitimate issues (corrupted manifest, permission errors).
Consider logging the error at debug/warn level for troubleshooting.
Proposed improvement
func localExtensionRecord(info extensionpkg.ExtensionInfo, now func() time.Time) ExtensionRecord { ext := &extensionpkg.Extension{ Info: info, Status: extensionpkg.ExtensionStatus{ Name: info.Name, Version: info.Version, Source: info.Source, Enabled: info.Enabled, }, } - if manifest, err := extensionpkg.LoadManifest(filepath.Dir(info.ManifestPath)); err == nil { - ext.Manifest = manifest + manifest, err := extensionpkg.LoadManifest(filepath.Dir(info.ManifestPath)) + if err != nil { + // Manifest unavailable - extension info will be returned without full manifest details + // This can happen if the manifest was modified after installation + } else { + ext.Manifest = manifest } return extensionpkg.DescribeExtension(ext, false, now()) }Alternatively, add a logger parameter to emit a warning when manifest loading fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/extension.go` around lines 319 - 333, The localExtensionRecord function currently swallows errors from extensionpkg.LoadManifest; update it to surface that failure by logging the error (at debug or warn level) when LoadManifest(filepath.Dir(info.ManifestPath)) returns a non-nil err so manifest load problems are visible; locate the load call inside localExtensionRecord and add a processLogger (or pass in an existing logger) or use the package logger to log a clear message including info.ManifestPath and err, without changing the returned ExtensionRecord behavior (or alternatively add a logger parameter to localExtensionRecord and use it to emit the warning).internal/extension/capability_test.go (1)
96-96: Loop variable capture is no longer needed in Go 1.22+.The
tt := ttpattern at lines 96 and 143 was necessary in older Go versions to capture the loop variable correctly for parallel tests. Since Go 1.22, loop variables are scoped per-iteration, making this unnecessary.Optional cleanup
for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) {Also applies to: 143-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/capability_test.go` at line 96, Remove the redundant loop-variable rebinds `tt := tt` in the table-driven test loops: locate the test functions that iterate over `tt` (the table test variable) and delete the `tt := tt` lines (both occurrences). Keep the rest of the subtest invocation (e.g., t.Run and any use of `tt` inside closures) unchanged since Go 1.22 scopes loop variables per iteration and the explicit capture is unnecessary.internal/daemon/daemon.go (1)
321-348: Silent nil return when Registry is unavailable may mask configuration issues.When
deps.Registry == nil, the factory returnsnilwithout logging or signaling. This silent degradation could make it harder to diagnose why extensions aren't working.Consider logging when extensions are disabled
if d.newExtensionManager == nil { d.newExtensionManager = func(deps extensionManagerDeps) extensionRuntime { if deps.Registry == nil { + if deps.Logger != nil { + deps.Logger.Info("extension manager disabled: registry not available") + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon.go` around lines 321 - 348, The factory assigned to d.newExtensionManager silently returns nil when deps.Registry is nil; update the closure (newExtensionManager) to log a clear warning or info via deps.Logger (or another available logger) indicating the extensions registry is missing/disabled and why, then return nil (or an explicit error/disabled sentinel if your API supports it) so the absence is observable; reference extensionManagerDeps, deps.Registry, extensionRuntime, and extensionpkg.NewManager when making the change to ensure the log is emitted before the nil return and that normal creation via extensionpkg.NewManager remains unchanged.cmd/agh-codegen/main.go (1)
51-59: Wrap errors with context for better diagnostics.Errors returned from
sdkts.Generate(),os.MkdirAll(), andos.WriteFile()lack context, making debugging harder when codegen fails.Proposed fix
func writeSDKContracts(path string) error { content, err := sdkts.Generate() if err != nil { - return err + return fmt.Errorf("generate sdk contracts: %w", err) } if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return err + return fmt.Errorf("create sdk contracts directory: %w", err) } - return os.WriteFile(path, []byte(content), 0o644) + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + return fmt.Errorf("write sdk contracts: %w", err) + } + return nil }As per coding guidelines: "Use explicit error returns with wrapped context:
fmt.Errorf(\"context: %w\", err)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agh-codegen/main.go` around lines 51 - 59, The function writeSDKContracts should wrap errors from sdkts.Generate(), os.MkdirAll(filepath.Dir(path), 0o755), and os.WriteFile(path, []byte(content), 0o644) with contextual messages using fmt.Errorf and %w; update the error returns so each call returns something like fmt.Errorf("generate sdk contracts: %w", err), fmt.Errorf("create directory for %s: %w", path, err), and fmt.Errorf("write SDK contracts to %s: %w", path, err) referencing the writeSDKContracts function and the specific failing call sites (sdkts.Generate, os.MkdirAll, os.WriteFile).internal/extension/describe.go (1)
71-76: Add explicit parentheses to clarify operator precedence.The boolean expression relies on
&&binding tighter than||, but the intent isn't immediately obvious to readers. Explicit grouping improves maintainability.Proposed clarification
if status.Active { - if status.Healthy || !requiresSubprocess(manifest) && len(info.Capabilities.Provides) == 0 && len(info.Actions.Requires) == 0 { + if status.Healthy || (!requiresSubprocess(manifest) && len(info.Capabilities.Provides) == 0 && len(info.Actions.Requires) == 0) { return "healthy" } return "unhealthy" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/describe.go` around lines 71 - 76, The conditional in the health-check return logic uses mixed || and && which is legal but unclear; update the if inside the status.Active block (the expression referencing status.Healthy, requiresSubprocess(manifest), info.Capabilities.Provides, and info.Actions.Requires) to add explicit parentheses around the intended groupings to make precedence obvious (e.g., group the OR and the combined AND conditions) so the intent is clear and maintainable.internal/codegen/sdkts/generate.go (1)
553-560: Hardcoded package path prefix may break if the module is forked or renamed.The check
strings.HasPrefix(t.PkgPath(), "github.com/pedronauck/agh/internal/")couples the generator to a specific module path.Consider extracting this as a configurable parameter or deriving it from the module's
go.modif the codebase is expected to be forked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/codegen/sdkts/generate.go` around lines 553 - 560, The function shouldAutoEmitNamedType currently hardcodes the module path prefix in the t.PkgPath() check which will break if the repo is renamed/forked; make the prefix configurable and replace the literal "github.com/pedronauck/agh/internal/" with a derived or injected value: add a package-level variable or constructor parameter (e.g., modulePathPrefix or autoEmitPrefix) that is initialized by reading go.mod (or passed in by the caller that sets up generation), and update shouldAutoEmitNamedType to call strings.HasPrefix(t.PkgPath(), modulePathPrefix) (or use a helper getModulePrefix()) so the generator works across forks or renamed modules.internal/extension/manifest_test.go (1)
18-47: Consider usingt.Runsubtests for the TOML/JSON equivalence checks.While the test is functional, using subtests would provide clearer output when only one format fails.
As per coding guidelines: "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/manifest_test.go` around lines 18 - 47, The test TestLoadManifest_ParsesTOMLAndJSONEquivalently should be split into t.Run subtests so failures show which format failed; call LoadManifest and assert against expectedManifest() inside two subtests like t.Run("parses TOML", func(t *testing.T){ ... }) using gotTOML and t.Run("parses JSON", func(t *testing.T){ ... }) using gotJSON, and put the DeepEqual checks (and related t.Fatalf messages) inside those subtests; optionally add a third subtest e.g. t.Run("TOML equals JSON", ...) to assert reflect.DeepEqual(*gotTOML, *gotJSON) so each assertion reports separately.internal/api/spec/spec_test.go (1)
9-50: Consider using subtests for better test isolation and diagnostics.The test validates multiple distinct operations (list sessions, create session, approve session, write memory) but lacks
t.Runsubtests. When one assertion fails, subsequent checks are skipped, making it harder to assess overall coverage gaps.Example restructuring
func TestDocumentTracksRequiredFieldsAndEnums(t *testing.T) { t.Parallel() doc, err := Document() if err != nil { t.Fatalf("Document() error = %v", err) } + t.Run("Should validate list sessions response schema", func(t *testing.T) { + t.Parallel() listSessions := operationFor(t, doc, "/api/sessions", "GET") listSessionsSchema := jsonResponseSchema(t, listSessions, 200) assertRequired(t, listSessionsSchema, "sessions") // ... remaining assertions for this operation + }) + t.Run("Should validate create session request schema", func(t *testing.T) { + t.Parallel() createSession := operationFor(t, doc, "/api/sessions", "POST") // ... + }) // ... additional subtests }As per coding guidelines: "Use table-driven tests with subtests (
t.Run) as default" and "MUST use t.Run("Should...") pattern for ALL test cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/spec/spec_test.go` around lines 9 - 50, TestDocumentTracksRequiredFieldsAndEnums groups multiple independent checks into one test; split each logical check into t.Run subtests so failures don't block other assertions. Refactor TestDocumentTracksRequiredFieldsAndEnums to call t.Run("Should list sessions ...") for the listSessions block that uses operationFor/jsonResponseSchema/assertRequired/assertEnumValues, t.Run("Should create session ...") for the createSession block that uses operationFor/jsonRequestSchema/assertNotRequired, t.Run("Should approve session ...") for the approveSession block using operationFor/jsonRequestSchema/assertRequired, and t.Run("Should write memory ...") for the writeMemory block using operationFor/jsonRequestSchema/assertRequired/assertNotRequired; keep existing assertions and helper calls (operationFor, jsonResponseSchema, jsonRequestSchema, propertySchema, assertRequired, assertNotRequired, assertEnumValues) but wrap each in its own t.Run subtest following the "Should..." naming convention.internal/extension/host_api_test.go (1)
436-444: Loop variable capture is no longer needed in Go 1.22+.The
tt := ttpattern on line 437 was required for loop variable capture in older Go versions but is no longer necessary since Go 1.22 changed loop variable semantics. This is harmless but can be removed for cleaner code.♻️ Optional cleanup
for _, tt := range tests { - tt := tt t.Run(tt.method, func(t *testing.T) { t.Parallel()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/host_api_test.go` around lines 436 - 444, The test loop contains an unnecessary loop-variable re-assignment `tt := tt` inside the range over `tests`; remove that redundant line so the closure in `t.Run` uses the loop variable directly (keeping `t.Run(tt.method, func(t *testing.T) { t.Parallel(); _, err := env.call(t, "ext-denied", tt.method, tt.params); assertCapabilityDenied(t, err, tt.method) })`). This cleans up the code while preserving behavior under Go 1.22+.internal/cli/extension_test.go (1)
79-101: Unused variabledepsafter checksum mismatch test.Line 100 has
_ = depswhich serves no purpose and appears to be leftover from a refactor. Thedepsvariable is created on line 82 but onlyhomePathsis used in this test.♻️ Remove unused variable reference
if err := installPreparedExtension(registry, prepared); err == nil || !errors.Is(err, extensionpkg.ErrExtensionChecksumMismatch) { t.Fatalf("installPreparedExtension(checksum mismatch) error = %v, want ErrExtensionChecksumMismatch", err) } - - _ = deps }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/extension_test.go` around lines 79 - 101, TestInstallPreparedExtensionDetectsChecksumMismatch declares deps via newExtensionLocalDeps(t, stubClient{}) but never uses it; remove the unused variable to clean up the test. Edit the TestInstallPreparedExtensionDetectsChecksumMismatch function to drop deps (call newExtensionLocalDeps only for the needed return value or assign to _ if you must), updating the call to newExtensionLocalDeps/newExtensionLocalDeps(t, stubClient{}) usage so only homePaths is kept; remove the trailing `_ = deps` line as well.internal/extension/manager.go (1)
1539-1563:Get()is returning live mutable state, not an isolated snapshot.
manifest := *ext.manifestis only a shallow copy, so nested maps/slices/pointers are still shared, andclone.Skills = append(clone.Skills, ext.skills...)reuses the original skill pointers. A caller mutating the returned value can corrupt manager state or race with the supervisor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/manager.go` around lines 1539 - 1563, Get() currently returns a shallow clone that still shares nested mutable state: the manifest copy (manifest := *ext.manifest) is only shallow and clone.Skills = append(clone.Skills, ext.skills...) copies pointers, and InitializeResult may also contain nested references. Fix by performing deep copies for those fields inside the cloning block in manager.go: replace the shallow manifest copy with a proper deep copy (e.g., implement deepCopyManifest(manifest *Manifest) *Manifest or use encode/decode) and assign clone.Manifest to that result; clone each skill element rather than copying the slice wholesale (implement cloneSkill(skill *Skill) *Skill and build clone.Skills by appending cloned skills); and likewise deep-copy InitializeResult (implement cloneInitializeResult or deepCopy for that type) so no nested maps/slices/pointers are shared. Keep existing per-item clone helpers (cloneHookDecl, cloneAgentDef, cloneMCPServer) as patterns for the new helpers and use them in the Get() cloning flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/agh-codegen/main.go`:
- Around line 82-96: The marshalOpenAPI function currently declares a doc any
parameter that is never used (the body calls spec.WriteFile instead), so update
the API to remove the unused parameter: change the marshalOpenAPI signature to
take no arguments, update all callers to stop passing a document, and keep the
existing behavior that writes spec via spec.WriteFile and returns the file
bytes; alternatively if you prefer to serialize the provided document instead,
replace the body to marshal the doc (e.g., json.Marshal) and remove
spec.WriteFile—choose one approach and apply it consistently to marshalOpenAPI
and all call sites that reference marshalOpenAPI.
In `@docs/ideas/anp/index.html`:
- Around line 287-288: The current assignment to content (const content = typeof
row.content === "string" ? row.content : JSON.stringify(row.content, null, 2))
can produce undefined when row.content is missing, causing escapeHtml to throw;
change it to guard against null/undefined by using row.content == null ? "" :
(typeof row.content === "string" ? row.content : JSON.stringify(row.content,
null, 2)), so content is always a string before passing to escapeHtml; update
any other similar places (e.g., around Line 307) to use the same null-coalescing
guard.
- Around line 269-277: The parseJsonl function currently filters out blank lines
before parsing, which makes the thrown error "Linha X" reference the filtered
index rather than the original file line; fix parseJsonl by splitting into
originalLines = text.split(/\r?\n/) (do not filter), then iterate over
originalLines by index: if a line is empty skip parsing but keep the index,
otherwise try JSON.parse on that originalLines[i] and on catch throw an Error
using (i+1) so the reported line matches the source; update references to
lines/rows accordingly.
In `@internal/acp/client.go`:
- Around line 352-356: The managed-stop branch in Stop should respect the
provided context: after calling proc.managed.Shutdown(ctx) (in the managed path)
do not call proc.Wait() unconditionally; instead start proc.Wait() in a
goroutine and use a select that waits for either the wait result or ctx.Done(),
appending the wait error only if it completes before context cancellation;
ensure you still collect and join errs with any shutdown error (from
proc.managed.Shutdown) and the proc.Wait() error when available so the method
remains context-bounded.
In `@internal/api/udsapi/extensions.go`:
- Around line 67-79: ExtensionStatus reads the path parameter using
c.Param("name") without trimming, causing inconsistent behavior vs.
mutateExtensionEnabled which trims the name; update the ExtensionStatus handler
(function ExtensionStatus) to trim whitespace from the parameter (e.g., use
strings.TrimSpace on c.Param("name")) before passing it to h.Extensions.Status
and in any error/log messages so both handlers treat the extension name the
same.
In `@internal/cli/cli_integration_test.go`:
- Around line 499-510: The Status method currently passes a hardcoded true for
the daemon-running flag to DescribeExtension, which can falsely report
extensions as healthy; update integrationExtensionService.Status to compute the
daemon/running boolean from real state (e.g., whether s.manager successfully
started/connected or a runtime/health check on the manager/runtime succeeds) and
pass that boolean into extensionpkg.DescribeExtension(ext, daemonRunning,
time.Now().UTC()) instead of true; reference the Status method and
extensionpkg.DescribeExtension to locate the change and ensure any manager
errors or runtime-down conditions flip daemonRunning to false so tests observe
real lifecycle state.
In `@internal/codegen/sdkts/generate.go`:
- Around line 260-277: The calls to ensureNamed inside tsType are ignoring
errors which can lead to silent, incomplete output; update tsType to handle and
propagate ensureNamed errors instead of discarding them: check the returned
error from ensureNamed in each place (the blocks using g.ensureNamed(name, t)),
and if non-nil return that error up the call chain (or convert tsType to return
(string, error) and propagate through callers), or at minimum log and return an
error explaining the failure; reference the tsType function and the ensureNamed,
g.typeNames and g.queued usage so you update all three branches (cached name,
shouldAutoEmitNamedType, isEnumType) to stop ignoring errors.
In `@internal/daemon/boot.go`:
- Around line 364-367: The provider closure passed to
extensionDeclarationProvider currently reads state.extensions without
synchronization while bootExtensions() may write it and hooks.Rebuild() can run
async, causing a race; add a sync.RWMutex (e.g., state.extMu) to the state
struct, use extMu.RLock()/RUnlock() inside the extensionDeclarationProvider
closure when accessing/returning state.extensions, and use extMu.Lock()/Unlock()
around the assignment in bootExtensions() (and any other writers) so reads and
writes are properly synchronized for hooks.Rebuild(),
extensionDeclarationProvider, and bootExtensions.
In `@internal/daemon/hooks_bridge.go`:
- Around line 434-436: The provider call returns raw errors that lose source
context; for both places where you call decls, err := provider(ctx) (and the
similar block around lines 450-454), wrap returned errors with fmt.Errorf to add
context before returning (e.g., return nil, fmt.Errorf("provider %s failed: %w",
providerNameOrSource, err)) so that failures from the chained declaration
provider are distinguishable from extension/runtime errors and later
hooks.Rebuild() failures include the original source.
In `@internal/extension/host_api.go`:
- Around line 445-499: handleMemoryRecall can panic when a memory source has no
configured store because memorySourcesForRecall may return h.memory with nil
store; update handleMemoryRecall to guard against nil stores the same way
memoryStoreFor does: after obtaining sources from memorySourcesForRecall, check
each source.store for nil and return a descriptive RPC error (e.g.,
invalidParamsRPCError or a new missingStoreRPCError) instead of dereferencing
it, or call the existing memoryStoreFor helper to validate/resolve the store
before calling source.store.Scan; apply the same nil-store guard to the other
recall path referenced (the similar code around the 656-687 region).
- Around line 349-408: Add a nil-check for h.sessions at the start of
handleSessionsStop, handleSessionsStatus, and handleSessionsEvents and return a
proper RPC error instead of dereferencing a nil pointer; e.g., if h.sessions ==
nil { return nil, internalRPCError(errors.New("sessions manager is not
configured")) } so these handlers fail cleanly when the session manager is not
initialized.
- Around line 619-625: The prompt is started with context.WithoutCancel and a
detached goroutine draining events, causing race and leak; change promptCtx to
context.WithCancel(ctx), start the goroutine that calls
drainAgentEvents(eventsCh) but have it accept/observe ctx and signal completion
(e.g., close a done channel or use a WaitGroup) when eventsCh is drained, and
ensure you cancel the promptCtx on parent ctx.Done to stop the goroutine if
needed; then block (select on done or ctx.Done) to wait for prompt completion
before issuing the Events(...AfterSequence...) lookup so you don't race and get
"prompt turn id not found". Ensure drainAgentEvents is invoked in a
cancellable/owned goroutine and that promptCtx is cancelled when finished.
In `@internal/extension/manager.go`:
- Around line 1279-1294: The resolveCommand implementation allows paths that
escape the extension root (absolute paths accepted verbatim and "../" survives
Clean+Join); change it to reject any resolved path that is outside rootDir:
after computing candidate := filepath.Clean(filepath.Join(rootDir, resolved))
use filepath.Rel(rootDir, candidate) and if the relative path starts with ".."
(or Rel returns an error) return an error (or nil+error) instead of allowing the
path; also apply the same containment check/fix to the duplicate logic
referenced at the other occurrence (around lines 1715-1720) so all
manifest-controlled paths are validated to remain inside the extension root.
- Around line 1328-1349: resolveEnvMap currently appends os.Environ(), which
unintentionally leaks the daemon's full environment into extension subprocesses;
change resolveEnvMap to stop inheriting the full environment by default and
instead construct the subprocess env only from the resolved env map plus a
minimal safe baseline (e.g., PATH, HOME, and other intentionally safe vars) or
require explicit opt-in/passthrough (e.g., a Manager.AllowHostEnv flag or a
special env key) before merging os.Environ(); update usages of resolveEnvMap and
document the opt-in flag so extensions cannot receive daemon secrets unless
explicitly allowed.
In `@internal/extension/manifest.go`:
- Around line 538-547: normalizeStrings currently trims whitespace but preserves
empty strings, which can later resolve to the extension root (e.g.,
resources.skills/resources.agents) and cause accidental loading; update
normalizeStrings to filter out entries where strings.TrimSpace(value) == ""
(skip appending blank results) so the returned slice contains only non-empty,
trimmed strings. Locate the normalizeStrings function and implement the check
that ignores whitespace-only values before append, ensuring callers no longer
receive "" entries.
In `@internal/extension/reference_support_unit_test.go`:
- Around line 5-52: Refactor the three tests (TestNonEmptyLines,
TestContainsFragmentsInOrder, TestDecodeJSONLines) into table-driven subtests
using t.Run with descriptive "Should..." names for each case; for nonEmptyLines
and containsFragmentsInOrder create a table of inputs/expected outputs and
iterate with t.Run per row, and for decodeJSONLines likewise use subtests for
valid cases and a specific invalid-JSON case where you assert the error content
(use ErrorContains or check err.Error() includes the expected substring) instead
of only checking non-nil; locate usages of nonEmptyLines,
containsFragmentsInOrder and decodeJSONLines in those tests to update them.
In `@internal/extension/registry.go`:
- Around line 248-285: installWithSource currently validates the artifact
checksum but then persists manifest fields (manifest.Name, manifest.Version)
from the caller-supplied manifest without verifying they match the manifest
actually written to disk at manifestPath; reload the manifest from manifestPath
(or read/parse the on-disk manifest file) after resolveInstallArtifact and
compare its Name and Version to the supplied manifest (or replace the supplied
values) before constructing ExtensionInfo and inserting into the registry;
update installWithSource to fail with a clear error if the on-disk manifest
identity/version differ from manifest.Name/manifest.Version so the persisted
ExtensionInfo and ManifestPath are guaranteed consistent.
- Around line 518-527: The current code hashes only the symlink text
(os.Readlink) which allows external targets to change without invalidating the
checksum; change the symlink handling in the checksum routine (the branch that
checks info.Mode()&os.ModeSymlink) to either (A) reject symlinks outright by
returning a clear error (e.g., "symlinks are not allowed in extensions") or (B)
resolve the symlink target and only accept it if it resolves inside the
extension root, then replace the existing writeChecksumString call with logic
that resolves the target path, stat/resolve the target file, and include the
target's canonical path and its actual content/metadata into the hasher (using
the same writeChecksumString/hasher flow) instead of hashing the raw link text;
use symbols shown (absPath, normalizedPath, os.Readlink, writeChecksumString,
hasher, filepath.ToSlash, info.Mode()) to locate and update the code, and return
an error if the target is outside the extension tree.
- Around line 479-488: The function mapRegistryConstraintError currently matches
constraint failures via string matching on err.Error(); change this to a type
assertion against *sqlite.Error and check the error code (lib.SQLITE_CONSTRAINT)
to detect unique-constraint violations and return &ExtensionExistsError{Name:
name} accordingly; update mapRegistryConstraintError to assert err as
(*sqlite.Error) and compare sqliteErr.Code == lib.SQLITE_CONSTRAINT instead of
using strings.Contains, keeping the existing fallback fmt.Errorf wrapping for
other errors.
In `@internal/skills/registry_external.go`:
- Around line 27-31: The map stores skills under a trimmed name but leaves the
cloned Skill.Meta.Name unchanged; update the code that inserts into registered
(where registered[name] = cloneSkill(skill)) to normalize the cloned object's
Meta.Name as well (e.g., set clone.Meta.Name =
strings.TrimSpace(skill.Meta.Name) or set it to the already-computed name
variable) so the stored clone and the map key use the same trimmed value; ensure
you still call cloneSkill(skill) to preserve other fields and only adjust
clone.Meta.Name before assigning into registered.
---
Outside diff comments:
In `@internal/acp/client.go`:
- Around line 141-182: spawnProcess currently uses context.Background() for
subprocess.Launch and procCtx which prevents cancellation propagation; change
the signature of spawnProcess to accept ctx context.Context (placing context
first), pass that ctx into subprocess.Launch instead of context.Background(),
and create procCtx with procCtx, cancelProcess := context.WithCancel(ctx) (so
the process lifecycle derives from the caller context); update all callers
(e.g., Start(ctx, ...)) to pass their ctx into spawnProcess so cancellation and
subprocess.Launch validation work correctly.
In `@internal/hooks/normalize.go`:
- Around line 106-124: sanitizedHookDecl currently normalizes WorkingDir but
doesn't reject it for non-subprocess hooks; update the validation in
sanitizedHookDecl (the same place that guards Command, Args, and Env) to return
an error when HookDecl.WorkingDir is non-empty and HookDecl.ExecutorKind is not
the subprocess kind (use the same ExecutorKind enum/constant you use elsewhere),
so non-subprocess hooks cannot carry a WorkingDir; ensure the error message
references WorkingDir and ExecutorKind for clarity.
In `@internal/skills/registry.go`:
- Around line 99-107: Get/List/ForWorkspace now return external skills but
SetEnabled still only mutates workspace cache entries or r.globalSkills, causing
"skill not found" when toggling external skills; update SetEnabled to, when a
skill lookup fails in workspace cache/globalSkills, call lookupSkillLocked (or
the same resolver used by Get) to find external skills, cloneSkill to create a
mutable copy, insert that copy into the appropriate workspace overlay (or
r.globalSkills if global), then apply the enabled/disabled change and
persist/update caches; ensure the same resolution logic is used as in
Get/List/ForWorkspace so overlays correctly apply to external entries.
---
Nitpick comments:
In `@cmd/agh-codegen/main.go`:
- Around line 51-59: The function writeSDKContracts should wrap errors from
sdkts.Generate(), os.MkdirAll(filepath.Dir(path), 0o755), and os.WriteFile(path,
[]byte(content), 0o644) with contextual messages using fmt.Errorf and %w; update
the error returns so each call returns something like fmt.Errorf("generate sdk
contracts: %w", err), fmt.Errorf("create directory for %s: %w", path, err), and
fmt.Errorf("write SDK contracts to %s: %w", path, err) referencing the
writeSDKContracts function and the specific failing call sites (sdkts.Generate,
os.MkdirAll, os.WriteFile).
In `@docs/ideas/anp/index.html`:
- Line 227: The file input element with id "file" currently accepts ".json" but
the parser function parseJsonl only handles newline-delimited JSON (JSONL),
causing regular JSON files to fail; fix by either (A) narrowing the accept
attribute on the <input id="file"> to only
".jsonl,.ndjson,application/jsonl,text/plain" or (B) adding a JSON fallback in
parseJsonl: detect if the payload contains newline-delimited records vs a single
JSON document/array, and if it’s a single JSON object/array, parse with
JSON.parse and normalize to the expected record array before continuing; update
parseJsonl (the function starting around line 269) to implement the detection
and fallback parsing.
In `@internal/api/spec/spec_test.go`:
- Around line 9-50: TestDocumentTracksRequiredFieldsAndEnums groups multiple
independent checks into one test; split each logical check into t.Run subtests
so failures don't block other assertions. Refactor
TestDocumentTracksRequiredFieldsAndEnums to call t.Run("Should list sessions
...") for the listSessions block that uses
operationFor/jsonResponseSchema/assertRequired/assertEnumValues, t.Run("Should
create session ...") for the createSession block that uses
operationFor/jsonRequestSchema/assertNotRequired, t.Run("Should approve session
...") for the approveSession block using
operationFor/jsonRequestSchema/assertRequired, and t.Run("Should write memory
...") for the writeMemory block using
operationFor/jsonRequestSchema/assertRequired/assertNotRequired; keep existing
assertions and helper calls (operationFor, jsonResponseSchema,
jsonRequestSchema, propertySchema, assertRequired, assertNotRequired,
assertEnumValues) but wrap each in its own t.Run subtest following the
"Should..." naming convention.
In `@internal/cli/client.go`:
- Around line 513-522: The extensionAction function redundantly trims whitespace
from the name parameter (strings.TrimSpace) even though callers like
EnableExtension and DisableExtension already pass a trimmed name; remove the
extra strings.TrimSpace call in extensionAction so path is built from the
provided name (leave trimming responsibility to callers) and ensure path :=
"/api/extensions/" + url.PathEscape(name) + "/" + strings.TrimSpace(action)
remains correctly escaped for the action segment.
In `@internal/cli/extension_test.go`:
- Around line 79-101: TestInstallPreparedExtensionDetectsChecksumMismatch
declares deps via newExtensionLocalDeps(t, stubClient{}) but never uses it;
remove the unused variable to clean up the test. Edit the
TestInstallPreparedExtensionDetectsChecksumMismatch function to drop deps (call
newExtensionLocalDeps only for the needed return value or assign to _ if you
must), updating the call to newExtensionLocalDeps/newExtensionLocalDeps(t,
stubClient{}) usage so only homePaths is kept; remove the trailing `_ = deps`
line as well.
In `@internal/cli/extension.go`:
- Around line 319-333: The localExtensionRecord function currently swallows
errors from extensionpkg.LoadManifest; update it to surface that failure by
logging the error (at debug or warn level) when
LoadManifest(filepath.Dir(info.ManifestPath)) returns a non-nil err so manifest
load problems are visible; locate the load call inside localExtensionRecord and
add a processLogger (or pass in an existing logger) or use the package logger to
log a clear message including info.ManifestPath and err, without changing the
returned ExtensionRecord behavior (or alternatively add a logger parameter to
localExtensionRecord and use it to emit the warning).
In `@internal/codegen/sdkts/generate.go`:
- Around line 553-560: The function shouldAutoEmitNamedType currently hardcodes
the module path prefix in the t.PkgPath() check which will break if the repo is
renamed/forked; make the prefix configurable and replace the literal
"github.com/pedronauck/agh/internal/" with a derived or injected value: add a
package-level variable or constructor parameter (e.g., modulePathPrefix or
autoEmitPrefix) that is initialized by reading go.mod (or passed in by the
caller that sets up generation), and update shouldAutoEmitNamedType to call
strings.HasPrefix(t.PkgPath(), modulePathPrefix) (or use a helper
getModulePrefix()) so the generator works across forks or renamed modules.
In `@internal/daemon/daemon.go`:
- Around line 321-348: The factory assigned to d.newExtensionManager silently
returns nil when deps.Registry is nil; update the closure (newExtensionManager)
to log a clear warning or info via deps.Logger (or another available logger)
indicating the extensions registry is missing/disabled and why, then return nil
(or an explicit error/disabled sentinel if your API supports it) so the absence
is observable; reference extensionManagerDeps, deps.Registry, extensionRuntime,
and extensionpkg.NewManager when making the change to ensure the log is emitted
before the nil return and that normal creation via extensionpkg.NewManager
remains unchanged.
In `@internal/extension/capability_test.go`:
- Line 96: Remove the redundant loop-variable rebinds `tt := tt` in the
table-driven test loops: locate the test functions that iterate over `tt` (the
table test variable) and delete the `tt := tt` lines (both occurrences). Keep
the rest of the subtest invocation (e.g., t.Run and any use of `tt` inside
closures) unchanged since Go 1.22 scopes loop variables per iteration and the
explicit capture is unnecessary.
In `@internal/extension/describe.go`:
- Around line 71-76: The conditional in the health-check return logic uses mixed
|| and && which is legal but unclear; update the if inside the status.Active
block (the expression referencing status.Healthy, requiresSubprocess(manifest),
info.Capabilities.Provides, and info.Actions.Requires) to add explicit
parentheses around the intended groupings to make precedence obvious (e.g.,
group the OR and the combined AND conditions) so the intent is clear and
maintainable.
In `@internal/extension/host_api_test.go`:
- Around line 436-444: The test loop contains an unnecessary loop-variable
re-assignment `tt := tt` inside the range over `tests`; remove that redundant
line so the closure in `t.Run` uses the loop variable directly (keeping
`t.Run(tt.method, func(t *testing.T) { t.Parallel(); _, err := env.call(t,
"ext-denied", tt.method, tt.params); assertCapabilityDenied(t, err, tt.method)
})`). This cleans up the code while preserving behavior under Go 1.22+.
In `@internal/extension/manager.go`:
- Around line 1539-1563: Get() currently returns a shallow clone that still
shares nested mutable state: the manifest copy (manifest := *ext.manifest) is
only shallow and clone.Skills = append(clone.Skills, ext.skills...) copies
pointers, and InitializeResult may also contain nested references. Fix by
performing deep copies for those fields inside the cloning block in manager.go:
replace the shallow manifest copy with a proper deep copy (e.g., implement
deepCopyManifest(manifest *Manifest) *Manifest or use encode/decode) and assign
clone.Manifest to that result; clone each skill element rather than copying the
slice wholesale (implement cloneSkill(skill *Skill) *Skill and build
clone.Skills by appending cloned skills); and likewise deep-copy
InitializeResult (implement cloneInitializeResult or deepCopy for that type) so
no nested maps/slices/pointers are shared. Keep existing per-item clone helpers
(cloneHookDecl, cloneAgentDef, cloneMCPServer) as patterns for the new helpers
and use them in the Get() cloning flow.
In `@internal/extension/manifest_test.go`:
- Around line 18-47: The test TestLoadManifest_ParsesTOMLAndJSONEquivalently
should be split into t.Run subtests so failures show which format failed; call
LoadManifest and assert against expectedManifest() inside two subtests like
t.Run("parses TOML", func(t *testing.T){ ... }) using gotTOML and t.Run("parses
JSON", func(t *testing.T){ ... }) using gotJSON, and put the DeepEqual checks
(and related t.Fatalf messages) inside those subtests; optionally add a third
subtest e.g. t.Run("TOML equals JSON", ...) to assert
reflect.DeepEqual(*gotTOML, *gotJSON) so each assertion reports separately.
In `@internal/skills/loader.go`:
- Around line 52-63: Change the ParseSkillFileWithSource entrypoint to accept a
context first: ParseSkillFileWithSource(ctx context.Context, path string, source
SkillSource), update callers accordingly, and propagate ctx into downstream
IO/parse calls (replace readSkillFile(path) with a context-aware read like
readSkillFile(ctx, path) and pass ctx into parseSkillDocument as well). Modify
readSkillFile and parseSkillDocument signatures to accept context.Context (or
add context-aware wrappers) and ensure all internal calls and tests pass the ctx
rather than using context.Background(); this keeps runtime-bound file-read and
parse operations cancelable and follows the project's context-first convention.
🪄 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: CHILL
Plan: Pro
Run ID: 076831c0-d4f7-4e4f-8ee4-2b1f64759fd9
⛔ Files ignored due to path filters (58)
.codex/plans/2026-04-10-api-contract-codegen.mdis excluded by!**/*.md.compozy/tasks/agh-network/_techspec.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-002.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-004.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-005.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/_meta.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/_tasks.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/MEMORY.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_01.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_02.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_03.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_04.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_05.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_06.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_07.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_08.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_09.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_10.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/memory/task_11.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_01.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_02.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_03.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_04.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_05.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_06.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_07.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_08.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_09.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/task_10.mdis excluded by!**/*.mdbun.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sumopenapi/agh.jsonis excluded by!**/*.jsonpackage.jsonis excluded by!**/*.jsonsdk/create-extension/package.jsonis excluded by!**/*.jsonsdk/create-extension/templates/hook-subprocess/extension.tomlis excluded by!**/*.tomlsdk/create-extension/templates/hook-subprocess/package.jsonis excluded by!**/*.jsonsdk/create-extension/templates/hook-subprocess/tsconfig.jsonis excluded by!**/*.jsonsdk/create-extension/templates/memory-backend/extension.tomlis excluded by!**/*.tomlsdk/create-extension/templates/memory-backend/package.jsonis excluded by!**/*.jsonsdk/create-extension/templates/memory-backend/tsconfig.jsonis excluded by!**/*.jsonsdk/create-extension/tsconfig.jsonis excluded by!**/*.jsonsdk/examples/prompt-enhancer/README.mdis excluded by!**/*.mdsdk/examples/prompt-enhancer/extension.tomlis excluded by!**/*.tomlsdk/examples/prompt-enhancer/package.jsonis excluded by!**/*.jsonsdk/examples/prompt-enhancer/tsconfig.jsonis excluded by!**/*.jsonsdk/examples/secret-guard/README.mdis excluded by!**/*.mdsdk/examples/secret-guard/extension.tomlis excluded by!**/*.tomlsdk/typescript/package.jsonis excluded by!**/*.jsonsdk/typescript/src/generated/contracts.tsis excluded by!**/generated/**sdk/typescript/tsconfig.cjs.jsonis excluded by!**/*.jsonsdk/typescript/tsconfig.esm.jsonis excluded by!**/*.jsonsdk/typescript/tsconfig.jsonis excluded by!**/*.jsonsdk/typescript/tsconfig.types.jsonis excluded by!**/*.jsonturbo.jsonis excluded by!**/*.jsonweb/package.jsonis excluded by!**/*.jsonweb/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (154)
Makefilecmd/agh-codegen/main.godocs/ideas/anp/conversa.jsonldocs/ideas/anp/index.htmlgo.modinternal/acp/client.gointernal/acp/types.gointernal/api/contract/contract.gointernal/api/contract/responses.gointernal/api/core/conversions.gointernal/api/core/conversions_parsers_test.gointernal/api/core/handlers.gointernal/api/core/skills.gointernal/api/core/workspaces.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/httpapi/sessions.gointernal/api/spec/spec.gointernal/api/spec/spec_test.gointernal/api/udsapi/extensions.gointernal/api/udsapi/handlers_test.gointernal/api/udsapi/routes.gointernal/api/udsapi/server.gointernal/cli/cli_integration_test.gointernal/cli/client.gointernal/cli/client_test.gointernal/cli/extension.gointernal/cli/extension_test.gointernal/cli/helpers_test.gointernal/cli/root.gointernal/cli/session.gointernal/cli/session_test.gointernal/cli/workspace.gointernal/codegen/sdkts/generate.gointernal/daemon/boot.gointernal/daemon/daemon.gointernal/daemon/daemon_integration_test.gointernal/daemon/daemon_test.gointernal/daemon/extensions.gointernal/daemon/hooks_bridge.gointernal/daemon/notifier_test.gointernal/extension/capability.gointernal/extension/capability_test.gointernal/extension/contract/host_api.gointernal/extension/contract/sdk.gointernal/extension/describe.gointernal/extension/host_api.gointernal/extension/host_api_integration_test.gointernal/extension/host_api_test.gointernal/extension/manager.gointernal/extension/manager_integration_test.gointernal/extension/manager_test.gointernal/extension/manifest.gointernal/extension/manifest_test.gointernal/extension/protocol/host_api.gointernal/extension/reference_integration_test.gointernal/extension/reference_support_test.gointernal/extension/reference_support_unit_test.gointernal/extension/registry.gointernal/extension/registry_integration_test.gointernal/extension/registry_test.gointernal/hooks/normalize.gointernal/hooks/types.gointernal/skills/loader.gointernal/skills/registry.gointernal/skills/registry_external.gointernal/store/globaldb/global_db.gointernal/store/globaldb/global_db_test.gointernal/subprocess/handshake.gointernal/subprocess/health.gointernal/subprocess/helper_ignore_unix_test.gointernal/subprocess/helper_ignore_windows_test.gointernal/subprocess/process.gointernal/subprocess/process_integration_test.gointernal/subprocess/process_test.gointernal/subprocess/signals.gointernal/subprocess/signals_unix.gointernal/subprocess/signals_windows.gointernal/subprocess/transport.gointernal/tools/tool.gointernal/tools/tool_test.gomagefile.gosdk/create-extension/src/index.test.tssdk/create-extension/src/index.tssdk/create-extension/templates/hook-subprocess/.gitignoresdk/create-extension/templates/hook-subprocess/src/index.tssdk/create-extension/templates/memory-backend/.gitignoresdk/create-extension/templates/memory-backend/src/index.tssdk/create-extension/vitest.config.tssdk/examples/prompt-enhancer/.gitignoresdk/examples/prompt-enhancer/src/index.tssdk/examples/secret-guard/.gitignoresdk/examples/secret-guard/main.gosdk/typescript/scripts/postbuild.mjssdk/typescript/src/base-types.tssdk/typescript/src/errors.test.tssdk/typescript/src/errors.tssdk/typescript/src/extension.test.tssdk/typescript/src/extension.tssdk/typescript/src/host-api.test.tssdk/typescript/src/host-api.tssdk/typescript/src/index.tssdk/typescript/src/integration.test.tssdk/typescript/src/testing/harness.test.tssdk/typescript/src/testing/harness.tssdk/typescript/src/testing/index.tssdk/typescript/src/testing/mock-transport.tssdk/typescript/src/transport.test.tssdk/typescript/src/transport.tssdk/typescript/src/types.tssdk/typescript/vitest.config.tsvitest.config.tsweb/src/lib/api-client.tsweb/src/lib/api-contract.tsweb/src/routes/_app/knowledge.tsxweb/src/systems/agent/adapters/agent-api.test.tsweb/src/systems/agent/adapters/agent-api.tsweb/src/systems/agent/index.tsweb/src/systems/agent/types.test.tsweb/src/systems/agent/types.tsweb/src/systems/daemon/adapters/daemon-api.test.tsweb/src/systems/daemon/adapters/daemon-api.tsweb/src/systems/daemon/index.tsweb/src/systems/daemon/types.test.tsweb/src/systems/daemon/types.tsweb/src/systems/knowledge/adapters/knowledge-api.test.tsweb/src/systems/knowledge/adapters/knowledge-api.tsweb/src/systems/knowledge/components/knowledge-detail-panel.tsxweb/src/systems/knowledge/hooks/use-knowledge-actions.tsweb/src/systems/knowledge/hooks/use-knowledge.test.tsxweb/src/systems/knowledge/hooks/use-knowledge.tsweb/src/systems/knowledge/index.tsweb/src/systems/knowledge/lib/query-keys.tsweb/src/systems/knowledge/lib/query-options.test.tsweb/src/systems/knowledge/lib/query-options.tsweb/src/systems/knowledge/types.test.tsweb/src/systems/knowledge/types.tsweb/src/systems/session/adapters/session-api-approve.test.tsweb/src/systems/session/adapters/session-api.test.tsweb/src/systems/session/adapters/session-api.tsweb/src/systems/session/index.tsweb/src/systems/session/lib/transcript-mapper.tsweb/src/systems/session/types.test.tsweb/src/systems/session/types.tsweb/src/systems/skill/adapters/skill-api.test.tsweb/src/systems/skill/adapters/skill-api.tsweb/src/systems/skill/index.tsweb/src/systems/skill/types.test.tsweb/src/systems/skill/types.tsweb/src/systems/workspace/adapters/workspace-api.test.tsweb/src/systems/workspace/adapters/workspace-api.tsweb/src/systems/workspace/index.tsweb/src/systems/workspace/types.test.tsweb/src/systems/workspace/types.tsweb/src/test/fetch-test-utils.ts
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
internal/extension/host_api.go (1)
628-645:⚠️ Potential issue | 🟠 MajorWait for the prompt stream to finish before looking up the turn.
This still starts
Prompt()oncontext.WithoutCancel(ctx), drains it in a detached goroutine, and immediately queriesEvents(...AfterSequence...). That can race and return a false"prompt turn id not found", and the drain goroutine has no owned shutdown path if the stream never closes.Suggested fix
- promptCtx := context.WithoutCancel(ctx) + promptCtx, cancel := context.WithCancel(ctx) + defer cancel() eventsCh, err := h.sessions.Prompt(promptCtx, sessionID, message) if err != nil { return "", err } - go drainAgentEvents(eventsCh) + drained := make(chan struct{}) + go func() { + defer close(drained) + drainAgentEvents(promptCtx, eventsCh) + }() + + select { + case <-drained: + case <-ctx.Done(): + return "", ctx.Err() + } @@ -func drainAgentEvents(events <-chan acp.AgentEvent) { - for range events { - } +func drainAgentEvents(ctx context.Context, events <-chan acp.AgentEvent) { + for { + select { + case _, ok := <-events: + if !ok { + return + } + case <-ctx.Done(): + return + } + } }As per coding guidelines, "Every goroutine must have explicit ownership and shutdown via
context.Contextcancellation" and "No fire-and-forget goroutines — track withsync.WaitGroupor equivalent".Also applies to: 976-979
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/host_api.go` around lines 628 - 645, The prompt stream is started with promptCtx := context.WithoutCancel(ctx) and drained in a detached goroutine via drainAgentEvents(eventsCh), which can race with h.sessions.Events(...) and leak if the stream never closes; change to start Prompt with a context derived from the incoming ctx (e.g., ctx or ctxWithCancel tied to caller), drain the events channel synchronously or start a tracked goroutine that uses the same ctx and a sync.WaitGroup or cancellation to ensure it finishes, and only call h.sessions.Events(ctx, sessionID, store.EventQuery{AfterSequence: lastSequence, ...}) after the drain confirms the stream completed (or after a controlled shutdown), updating references to promptCtx, eventsCh, drainAgentEvents, and the Events call accordingly so there is an explicit ownership and shutdown path for the goroutine and no race when checking the turn id.
🧹 Nitpick comments (2)
internal/api/spec/spec_test.go (2)
17-57: Addt.Parallel()inside each independent subtest.The subtests under
TestDocumentTracksRequiredFieldsAndEnumsare read-only checks and can run concurrently. Addingt.Parallel()inside eacht.Runkeeps this file aligned with repo test conventions and speeds execution.Proposed change
t.Run("ShouldDescribeSessionListRequiredFieldsAndEnums", func(t *testing.T) { + t.Parallel() listSessions := operationFor(t, doc, "/api/sessions", "GET") ... }) t.Run("ShouldDescribeCreateSessionOptionalFields", func(t *testing.T) { + t.Parallel() createSession := operationFor(t, doc, "/api/sessions", "POST") ... }) t.Run("ShouldDescribeApproveSessionRequiredFields", func(t *testing.T) { + t.Parallel() approveSession := operationFor(t, doc, "/api/sessions/{id}/approve", "POST") ... }) t.Run("ShouldDescribeWriteMemoryRequiredAndOptionalFields", func(t *testing.T) { + t.Parallel() writeMemory := operationFor(t, doc, "/api/memory/{filename}", "PUT") ... })As per coding guidelines, "Use
t.Parallel()for independent subtests in Go tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/spec/spec_test.go` around lines 17 - 57, The subtests "ShouldDescribeSessionListRequiredFieldsAndEnums", "ShouldDescribeCreateSessionOptionalFields", "ShouldDescribeApproveSessionRequiredFields", and "ShouldDescribeWriteMemoryRequiredAndOptionalFields" should call t.Parallel() at the start of their t.Run closures to enable concurrent execution; edit each anonymous func passed to t.Run in spec_test.go and add t.Parallel() as the first statement inside the closure (the functions wrapping the checks that call operationFor, jsonResponseSchema/jsonRequestSchema, assertRequired/assertNotRequired/assertEnumValues).
129-144: Assert enum set exactly, not only expected subset.
assertEnumValuescurrently passes when unexpected enum values are added. That weakens contract protection. Validate exact membership (and fail on non-string enum entries) so schema drift is caught.Proposed change
func assertEnumValues(t *testing.T, schema *openapi3.Schema, values ...string) { t.Helper() got := make([]string, 0, len(schema.Enum)) for _, value := range schema.Enum { text, ok := value.(string) - if ok { - got = append(got, text) - } + if !ok { + t.Fatalf("expected enum values to be strings, got %T", value) + } + got = append(got, text) } + if len(got) != len(values) { + t.Fatalf("expected enum size %d, got %d (%v)", len(values), len(got), got) + } for _, value := range values { if !contains(got, value) { t.Fatalf("expected enum %q in %v", value, got) } } + for _, value := range got { + if !contains(values, value) { + t.Fatalf("unexpected enum %q in %v", value, got) + } + } }As per coding guidelines, "Verify tests can fail when business logic changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/spec/spec_test.go` around lines 129 - 144, The test helper assertEnumValues should verify exact enum membership and string-typed entries: update assertEnumValues to fail if any schema.Enum entry is not a string, collect all string enum values from schema.Enum into got, then compare that set exactly to the expected values (values...) — fail if any expected value is missing or any extra value exists in got; refer to function assertEnumValues and the schema.Enum iteration for locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.codex/tmp/agh-net-security-review.txt:
- Line 8: Replace the unhyphenated phrase "end to end" with the hyphenated form
"end-to-end" in the sentence under the "Network-turn safety model consistency"
heading (the clause that reads "one coherent chain end to end"); update that
occurrence so the doc uses "end-to-end" for consistent technical hyphenation.
- Line 3: Update the wording that currently reads "no file edits made" / "no
files were edited" in this newly added review note to a clearer phrase such as
"no additional implementation files were edited" (or similar) so the record
unambiguously acknowledges that the review file itself is new; locate and
replace those exact strings in the document (the occurrences that say "no file
edits made" / "no files were edited") with the clarified text.
- Line 6: The doc uses absolute local filesystem links (e.g.
/Users/pedronauck/dev/projects/agh/.compozy/tasks/agh-network/_techspec.md and
/Users/pedronauck/dev/projects/agh/docs/rfcs/003_agh-network-v0.md) on lines
6/8/10; replace those with repository-relative paths or plain doc anchors (for
example tasks/agh-network/_techspec.md, tasks/agh-network/adrs/adr-001.md,
docs/rfcs/003_agh-network-v0.md or their intra-doc anchor equivalents) so the
links work for other reviewers and CI, and update the other occurrences noted
(lines 8-8 and 10-10) to the same repo-relative form.
In `@cmd/agh-codegen/main_test.go`:
- Around line 10-48: Convert the three standalone tests
(TestCheckJSONFileIgnoresFormattingDifferences,
TestCheckJSONFileRejectsContentDifferences,
TestFormatTypeScriptMatchesRepositoryFormatter) into table-driven subtests using
t.Run with the "Should..." naming pattern; for each existing scenario wrap the
setup/assertion inside a t.Run("Should ...", func(t *testing.T){ ... }) and, if
multiple cases are combined, create a slice of cases and iterate calling
t.Run(case.name, func(t *testing.T){ ... }); keep the existing use of
checkJSONFile and formatTypeScript but move their invocation/assertions into the
subtest bodies and preserve t.Parallel() where appropriate (call t.Parallel()
inside each subtest rather than at top-level test if you want parallelism).
- Around line 32-34: Tests are matching error text instead of using errors.Is;
define a sentinel var ErrStaleGeneratedFile = errors.New("generated file is
stale") (in the same package as checkJSONFile), update checkJSONFile to wrap
that sentinel when the canonical JSON differs (e.g., return fmt.Errorf("%s: %w",
path, ErrStaleGeneratedFile) inside the mismatch branch), and change the test
assertion to use errors.Is(err, ErrStaleGeneratedFile) instead of
strings.Contains(err.Error(), "stale") to assert the specific error.
In `@internal/acp/client_test.go`:
- Around line 640-677: Wrap the existing test body of
TestStopManagedProcessRespectsContext in a t.Run subtest (e.g., t.Run("Should
stop managed process respecting context", func(t *testing.T) { ... })) and move
t.Parallel() into that subtest; keep all setup (New(WithStopTimeout...),
subprocess.Launch, AgentProcess creation, go proc.waitForExit(), t.Cleanup
shutdown, stopCtx/cancel, timing and assertions against driver.Stop and
context.DeadlineExceeded) unchanged and inside the subtest closure so the test
follows the repo's t.Run("Should...") convention while still exercising
driver.Stop, subprocess.Launch, AgentProcess.waitForExit and proc.Done as
before.
In `@internal/api/spec/spec_test.go`:
- Around line 22-23: The test dereferences propertySchema(t, listSessionsSchema,
"sessions").Items.Value which can panic if the "sessions" schema is not an array
or omits items; update the test to first retrieve the Items (e.g. sessionItems
:= propertySchema(t, listSessionsSchema, "sessions").Items), assert it is not
nil (fail with a clear message if nil), optionally assert the "sessions" schema
has type "array" if available, then use sessionItems.Value when calling
assertRequired(t, sessionSchema, ...); reference propertySchema,
listSessionsSchema, and assertRequired in the change.
In `@internal/extension/host_api.go`:
- Around line 99-103: WithHostAPIRateLimit currently snapshots handler.now when
the option is applied, so applying it before WithHostAPINow freezes the clock;
instead make the option order-independent by not creating the limiter
immediately: have WithHostAPIRateLimit store the limit and burst into
HostAPIHandler fields (e.g., hostAPILimit, hostAPIBurst) and only call
newHostAPIRateLimiter(handler.hostAPILimit, handler.hostAPIBurst, handler.now)
at handler initialization time (or the first time the limiter is needed). Update
any other places that directly construct the limiter (see similar code around
lines 107-110 and 138-148) to follow the same pattern so newHostAPIRateLimiter
always receives the finalized handler.now.
In `@internal/extension/manifest.go`:
- Around line 522-535: normalizeMCPServers currently trims names and env keys
but keeps entries whose trimmed key becomes empty, and can silently collapse
distinct raw keys; change normalizeMCPServers to skip any entry whose trimmed
name is empty (i.e., if strings.TrimSpace(name) == "" continue) and when
building Env use/adjust normalizeStringMap to similarly drop any env entries
whose trimmed key is empty so whitespace-only keys are rejected; apply the same
empty-key rejection to the other similar normalizer referenced at lines 557-565
(the companion function that normalizes MCP env/string maps) and ensure behavior
on key collisions is deterministic (e.g., last write wins).
---
Duplicate comments:
In `@internal/extension/host_api.go`:
- Around line 628-645: The prompt stream is started with promptCtx :=
context.WithoutCancel(ctx) and drained in a detached goroutine via
drainAgentEvents(eventsCh), which can race with h.sessions.Events(...) and leak
if the stream never closes; change to start Prompt with a context derived from
the incoming ctx (e.g., ctx or ctxWithCancel tied to caller), drain the events
channel synchronously or start a tracked goroutine that uses the same ctx and a
sync.WaitGroup or cancellation to ensure it finishes, and only call
h.sessions.Events(ctx, sessionID, store.EventQuery{AfterSequence: lastSequence,
...}) after the drain confirms the stream completed (or after a controlled
shutdown), updating references to promptCtx, eventsCh, drainAgentEvents, and the
Events call accordingly so there is an explicit ownership and shutdown path for
the goroutine and no race when checking the turn id.
---
Nitpick comments:
In `@internal/api/spec/spec_test.go`:
- Around line 17-57: The subtests
"ShouldDescribeSessionListRequiredFieldsAndEnums",
"ShouldDescribeCreateSessionOptionalFields",
"ShouldDescribeApproveSessionRequiredFields", and
"ShouldDescribeWriteMemoryRequiredAndOptionalFields" should call t.Parallel() at
the start of their t.Run closures to enable concurrent execution; edit each
anonymous func passed to t.Run in spec_test.go and add t.Parallel() as the first
statement inside the closure (the functions wrapping the checks that call
operationFor, jsonResponseSchema/jsonRequestSchema,
assertRequired/assertNotRequired/assertEnumValues).
- Around line 129-144: The test helper assertEnumValues should verify exact enum
membership and string-typed entries: update assertEnumValues to fail if any
schema.Enum entry is not a string, collect all string enum values from
schema.Enum into got, then compare that set exactly to the expected values
(values...) — fail if any expected value is missing or any extra value exists in
got; refer to function assertEnumValues and the schema.Enum iteration for
locating the 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: CHILL
Plan: Pro
Run ID: 4ea2c0e3-3782-433a-8e17-46d78949d95c
⛔ Files ignored due to path filters (40)
.compozy/tasks/agh-network/_techspec.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-004.mdis excluded by!**/*.md.compozy/tasks/agh-network/adrs/adr-005.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_019.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_020.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_021.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_022.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_023.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_024.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_025.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_026.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_027.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_028.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_029.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_030.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_031.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_032.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_033.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-001/issue_034.mdis excluded by!**/*.md
📒 Files selected for processing (32)
.codex/tmp/agh-net-rfc-review.txt.codex/tmp/agh-net-runtime-review.txt.codex/tmp/agh-net-security-review.txtcmd/agh-codegen/main.gocmd/agh-codegen/main_test.godocs/ideas/anp/index.htmlinternal/acp/client.gointernal/acp/client_test.gointernal/api/spec/spec_test.gointernal/api/udsapi/extensions.gointernal/api/udsapi/handlers_test.gointernal/api/udsapi/helpers_test.gointernal/cli/client.gointernal/cli/client_test.gointernal/cli/extension_test.gointernal/codegen/sdkts/generate.gointernal/daemon/boot.gointernal/daemon/daemon_test.gointernal/daemon/hooks_bridge.gointernal/extension/capability_test.gointernal/extension/describe.gointernal/extension/host_api.gointernal/extension/host_api_test.gointernal/extension/manager.gointernal/extension/manager_test.gointernal/extension/manifest.gointernal/extension/manifest_test.gointernal/extension/reference_support_unit_test.gointernal/extension/registry.gointernal/extension/registry_test.gointernal/skills/registry_external.gointernal/skills/registry_test.go
✅ Files skipped from review due to trivial changes (8)
- internal/cli/client_test.go
- .codex/tmp/agh-net-runtime-review.txt
- docs/ideas/anp/index.html
- internal/codegen/sdkts/generate.go
- internal/extension/host_api_test.go
- .codex/tmp/agh-net-rfc-review.txt
- internal/cli/extension_test.go
- internal/extension/capability_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/api/udsapi/handlers_test.go
- internal/cli/client.go
- internal/extension/describe.go
- internal/acp/client.go
- internal/daemon/hooks_bridge.go
- internal/api/udsapi/extensions.go
- cmd/agh-codegen/main.go
- internal/extension/manager.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/api/spec/spec_test.go (1)
18-71: Prefer a table-driven loop for these OpenAPI contract cases.These subtests all follow the same arrange/assert pattern, so adding more endpoint/schema coverage will keep duplicating setup and make omissions easier. A small table of
{name, path, method, check}would keep this file much easier to extend.As per coding guidelines, "Use table-driven tests with subtests (t.Run) as default in Go tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/spec/spec_test.go` around lines 18 - 71, Replace the repeated subtests with a table-driven loop: create a slice of test cases (fields: name, path, method, isRequest bool or other discriminator, and a checker function) and iterate over them calling t.Run(tc.name, func(t *testing.T){ t.Parallel(); op := operationFor(t, doc, tc.path, tc.method); schema := jsonRequestSchema(t, op) or jsonResponseSchema(t, op, 200) depending on tc.isRequest; then invoke tc.checker(t, schema) which uses assertRequired/assertNotRequired/assertEnumValues/propertySchema as needed (e.g. for the sessions list case your checker will call propertySchema, check items and assertRequired/assertEnumValues). Ensure you capture tc := tc inside the loop to avoid closure issues and keep all existing assertion helpers (operationFor, jsonResponseSchema, jsonRequestSchema, assertRequired, assertNotRequired, assertEnumValues, propertySchema) as referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/agh-codegen/main.go`:
- Around line 99-101: The deferred temp-file removal currently discards errors
with `_ = os.Remove(file.Name())`; change the defer to capture and handle the
error (e.g., `if err := os.Remove(file.Name()); err != nil { log.Printf("failed
to remove temp file %s: %v", file.Name(), err) }`) or add a clear justification
comment if you truly intend to ignore removal errors—update the defer func
surrounding file.Name() accordingly to either log the error via the program
logger or document why the error can be ignored.
- Around line 22-52: Thread a caller-owned context from main() into run() and
propagate it through writeSDKContracts(), checkSDKContracts(),
generateFormattedSDKContracts(), and formatTypeScript(), replacing
exec.Command(...) with exec.CommandContext(...) so the bunx oxfmt subprocess can
be cancelled or timed out; update function signatures to accept context.Context
and ensure callers pass the context from main() (or a derived
timeout/cancellable context) down to formatTypeScript() where
exec.CommandContext is used.
In `@internal/acp/client_test.go`:
- Around line 660-665: Handle the shutdown error from managed.Shutdown and avoid
an unbounded wait on proc.Done: inside the t.Cleanup closure (around cleanupCtx,
cancel, managed.Shutdown and <-proc.Done()), call managed.Shutdown and capture
its error and surface it (t.Fatalf/t.Errorf or t.Logf + fail) instead of
discarding with `_`, and replace the unconditional `<-proc.Done()` with a select
that waits on proc.Done() and also a timeout (use the same 5s cleanupCtx or a
new time.After) so the cleanup cannot hang forever; reference the cleanup
closure that creates cleanupCtx/cancel, managed.Shutdown, and proc.Done to
locate and update the code.
---
Nitpick comments:
In `@internal/api/spec/spec_test.go`:
- Around line 18-71: Replace the repeated subtests with a table-driven loop:
create a slice of test cases (fields: name, path, method, isRequest bool or
other discriminator, and a checker function) and iterate over them calling
t.Run(tc.name, func(t *testing.T){ t.Parallel(); op := operationFor(t, doc,
tc.path, tc.method); schema := jsonRequestSchema(t, op) or jsonResponseSchema(t,
op, 200) depending on tc.isRequest; then invoke tc.checker(t, schema) which uses
assertRequired/assertNotRequired/assertEnumValues/propertySchema as needed (e.g.
for the sessions list case your checker will call propertySchema, check items
and assertRequired/assertEnumValues). Ensure you capture tc := tc inside the
loop to avoid closure issues and keep all existing assertion helpers
(operationFor, jsonResponseSchema, jsonRequestSchema, assertRequired,
assertNotRequired, assertEnumValues, propertySchema) as referenced.
🪄 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: CHILL
Plan: Pro
Run ID: 06445b35-57d0-4474-91af-f77cc748e06e
⛔ Files ignored due to path filters (23)
.compozy/tasks/agh-network/_tasks.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_01.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_02.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_03.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_04.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_05.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_06.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_07.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_08.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_09.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_10.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-002/issue_011.mdis excluded by!**/*.md
📒 Files selected for processing (9)
.codex/tmp/agh-net-security-review.txtcmd/agh-codegen/main.gocmd/agh-codegen/main_test.gointernal/acp/client_test.gointernal/api/spec/spec_test.gointernal/extension/host_api.gointernal/extension/host_api_test.gointernal/extension/manifest.gointernal/extension/manifest_test.go
✅ Files skipped from review due to trivial changes (1)
- .codex/tmp/agh-net-security-review.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/agh-codegen/main_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/acp/client.go (1)
119-120:⚠️ Potential issue | 🟠 MajorPropagate
Startcontext into subprocess launch to avoid starting work after cancellation.At Line 154,
subprocess.Launch(context.Background(), ...)ignores thectxpassed intoStart, so a canceled/deadline-exceededStartcan still spawn a subprocess before failing later.♻️ Proposed fix
func (d *Driver) Start(ctx context.Context, opts StartOpts) (*AgentProcess, error) { @@ - process, err := d.spawnProcess(normalized) + process, err := d.spawnProcess(ctx, normalized) if err != nil { return nil, err } @@ -func (d *Driver) spawnProcess(normalized StartOpts) (*AgentProcess, error) { +func (d *Driver) spawnProcess(ctx context.Context, normalized StartOpts) (*AgentProcess, error) { @@ - managed, err := subprocess.Launch(context.Background(), subprocess.LaunchConfig{ + managed, err := subprocess.Launch(ctx, subprocess.LaunchConfig{ Command: command,As per coding guidelines, "Pass context.Context as first argument to functions crossing runtime boundaries — avoid context.Background() outside main and focused tests".
Also applies to: 129-130, 143-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client.go` around lines 119 - 120, The Start method currently uses context.Background() when calling subprocess.Launch and other runtime-bound functions, which ignores the Start(ctx) cancellation/deadline; update all Launch calls and any other cross-runtime calls inside Driver.Start (e.g., the subprocess.Launch invocations between the checks around lines 129-154) to pass the upstream ctx instead of context.Background(), ensuring Driver.Start's context is propagated into subprocess.Launch and related calls so cancellations/deadlines are honored.
♻️ Duplicate comments (1)
internal/acp/client_test.go (1)
705-710:⚠️ Potential issue | 🟠 MajorCleanup still ignores shutdown errors and can hang waiting for exit.
At Line 708 the shutdown error is discarded, and Line 709 blocks unconditionally on
proc.Done(). This can mask teardown failures and stall cleanup.♻️ Proposed fix
t.Cleanup(func() { cleanupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - _ = managed.Shutdown(cleanupCtx) - <-proc.Done() + if shutdownErr := managed.Shutdown(cleanupCtx); shutdownErr != nil { + t.Fatalf("managed.Shutdown() error = %v", shutdownErr) + } + select { + case <-proc.Done(): + case <-cleanupCtx.Done(): + t.Fatalf("process did not exit during cleanup: %v", cleanupCtx.Err()) + } })As per coding guidelines, "Never ignore errors with
_— every error must be handled or have a written justification".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client_test.go` around lines 705 - 710, The cleanup currently ignores the error returned by managed.Shutdown and blocks indefinitely on proc.Done(); change the cleanup to capture the error (err := managed.Shutdown(cleanupCtx)) and handle it (t.Errorf or t.Logf with context) instead of discarding, then wait for proc.Done() with a bounded wait using the same cleanupCtx (use a select on <-proc.Done() and <-cleanupCtx.Done()) so shutdown timeouts are reported rather than hanging; reference managed.Shutdown, cleanupCtx/cancel, proc.Done(), and t.Cleanup to locate and update the teardown logic.
🧹 Nitpick comments (1)
internal/api/httpapi/handlers_test.go (1)
295-332: Wrap this test int.Run()and addt.Parallel().This test is independent and should follow the repository's Go test conventions: all test cases must use the
t.Run("Should...")pattern witht.Parallel()for independent subtests, as shown in other tests in this file (e.g., lines 787–839).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/handlers_test.go` around lines 295 - 332, Wrap the TestDaemonStatusHandlerReturnsUserHomeDir body inside a t.Run subtest and call t.Parallel() at the start of that subtest; specifically, inside the TestDaemonStatusHandlerReturnsUserHomeDir function create t.Run("returns user home dir", func(t *testing.T) { t.Parallel() /* move the existing test code here unchanged */ }) so the test follows the repository convention and runs as an independent parallel subtest, leaving the function name and internal assertions (including calls to newTestRouter, performRequest, decodeJSONResponse, aghconfig.ResolvePath, etc.) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/acp/client_test.go`:
- Around line 521-524: The subtest names must follow the "Should..." pattern:
update the t.Run invocation inside the tests loop (the block that declares tc :=
tc and calls t.Run(name, func(t *testing.T) { t.Parallel() ... })) to use a
"Should ..." phrasing (e.g., t.Run("Should "+name, ...) or map each test key to
a "Should ..." string) so all subtest names conform to the required pattern;
ensure you still capture tc with tc := tc and keep t.Parallel() unchanged.
In `@internal/api/core/handlers.go`:
- Around line 595-629: The /daemon/status handler calls resolveUserHomeDir(),
which uses os.UserHomeDir() and can fail in sanitized service environments;
replace that call to use the already-resolved path from the handler config
(h.HomePaths.HomeDir) or call aghconfig.ResolveHomeDir() if recomputation is
required. Update the code that sets UserHomeDir in the DaemonStatusPayload to
use h.HomePaths.HomeDir (or the result of aghconfig.ResolveHomeDir()) and remove
or deprecate resolveUserHomeDir() usage from this handler so the endpoint no
longer fails when HOME/USERPROFILE are unset.
---
Outside diff comments:
In `@internal/acp/client.go`:
- Around line 119-120: The Start method currently uses context.Background() when
calling subprocess.Launch and other runtime-bound functions, which ignores the
Start(ctx) cancellation/deadline; update all Launch calls and any other
cross-runtime calls inside Driver.Start (e.g., the subprocess.Launch invocations
between the checks around lines 129-154) to pass the upstream ctx instead of
context.Background(), ensuring Driver.Start's context is propagated into
subprocess.Launch and related calls so cancellations/deadlines are honored.
---
Duplicate comments:
In `@internal/acp/client_test.go`:
- Around line 705-710: The cleanup currently ignores the error returned by
managed.Shutdown and blocks indefinitely on proc.Done(); change the cleanup to
capture the error (err := managed.Shutdown(cleanupCtx)) and handle it (t.Errorf
or t.Logf with context) instead of discarding, then wait for proc.Done() with a
bounded wait using the same cleanupCtx (use a select on <-proc.Done() and
<-cleanupCtx.Done()) so shutdown timeouts are reported rather than hanging;
reference managed.Shutdown, cleanupCtx/cancel, proc.Done(), and t.Cleanup to
locate and update the teardown logic.
---
Nitpick comments:
In `@internal/api/httpapi/handlers_test.go`:
- Around line 295-332: Wrap the TestDaemonStatusHandlerReturnsUserHomeDir body
inside a t.Run subtest and call t.Parallel() at the start of that subtest;
specifically, inside the TestDaemonStatusHandlerReturnsUserHomeDir function
create t.Run("returns user home dir", func(t *testing.T) { t.Parallel() /* move
the existing test code here unchanged */ }) so the test follows the repository
convention and runs as an independent parallel subtest, leaving the function
name and internal assertions (including calls to newTestRouter, performRequest,
decodeJSONResponse, aghconfig.ResolvePath, etc.) intact.
🪄 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: CHILL
Plan: Pro
Run ID: 19c347ed-4cba-4a61-bf18-f682ecf7be16
⛔ Files ignored due to path filters (34)
.codex/plans/2026-04-10-web-shell-workspace-onboarding.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/_techspec.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/adrs/adr-002.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/adrs/adr-004.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/_archived/kb-refac/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/agh-network/_techspec.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_01.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_08.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_10.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/_meta.mdis excluded by!**/*.md.compozy/tasks/extensability/_meta.mdis excluded by!**/*.mddocs/ideas/market-pair/gap-analysis.mdis excluded by!**/*.mdopenapi/agh.jsonis excluded by!**/*.jsonweb/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (39)
cmd/agh/main_test.gointernal/acp/client.gointernal/acp/client_test.gointernal/api/contract/contract.gointernal/api/core/handlers.gointernal/api/core/more_coverage_test.gointernal/api/httpapi/handlers_test.gointernal/cli/daemon.gointernal/cli/root.gointernal/extension/manager.gointernal/extension/manifest_test.gointernal/session/additional_test.gointernal/session/manager_lifecycle.gointernal/session/manager_prompt.gointernal/session/manager_test.gointernal/session/query.gointernal/session/query_test.gointernal/session/resume_repair.gointernal/session/resume_repair_test.gointernal/session/session.gointernal/version/version.gointernal/version/version_test.goweb/src/components/app-sidebar.test.tsxweb/src/components/app-sidebar.tsxweb/src/components/design-system/index.tsweb/src/components/design-system/pill-button.tsxweb/src/routes/-_app.test.tsxweb/src/routes/_app.tsxweb/src/routes/_app/-knowledge.test.tsxweb/src/routes/_app/-session.$id.test.tsxweb/src/routes/_app/-skills.test.tsxweb/src/routes/_app/knowledge.tsxweb/src/routes/_app/session.$id.tsxweb/src/routes/_app/skills.tsxweb/src/styles.cssweb/src/systems/daemon/adapters/daemon-api.test.tsweb/src/systems/daemon/adapters/daemon-api.tsweb/src/systems/daemon/hooks/use-daemon-status.tsweb/src/systems/daemon/index.ts
✅ Files skipped from review due to trivial changes (1)
- internal/extension/manager.go
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
internal/acp/client_test.go (1)
501-502: Prefer shared constant over magic error code literal in test fixtures.Using
-32002directly makes this test drift-prone if the sentinel changes. ReuserequestErrorResourceNotFoundCodefor tighter coupling to the contract under test.♻️ Proposed change
- &acpsdk.RequestError{Code: -32002, Message: "Resource not found: sess-existing"}, + &acpsdk.RequestError{Code: requestErrorResourceNotFoundCode, Message: "Resource not found: sess-existing"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client_test.go` around lines 501 - 502, Replace the magic literal -32002 in the test fixture with the shared constant requestErrorResourceNotFoundCode: locate the acpsdk.RequestError instance in internal/acp/client_test.go (the test that currently sets Code: -32002), change Code to use requestErrorResourceNotFoundCode so the test ties to the canonical sentinel, ensure the constant is in scope (add or adjust imports if necessary), and run the tests to confirm no regressions.internal/api/core/handlers_internal_test.go (1)
11-72: Tighten the failure-path assertion and deduplicate these cases.The scenarios are good, but the last subtest only checks
err != nil, so the helper could start returning the wrong wrapped error and this would still pass. Converting this into a small table-driven test and asserting the expected error text/cause in the failing row would make the coverage much more resilient.As per coding guidelines,
**/*_test.go: "Use table-driven tests with subtests (t.Run) as default" and "MUST have specific error assertions (ErrorContains, ErrorAs)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/handlers_internal_test.go` around lines 11 - 72, Convert the three subtests in TestResolveUserHomeDir into a small table-driven test that iterates cases (e.g., "resolvedLookup", "fallbackToCanonicalParent", "lookupFailsAndNoFallback") to remove duplication; for each case call resolveUserHomeDir(homePaths, lookupFunc) (use aghconfig.ResolveHomePathsFrom to create homePaths) and assert results: for success cases check got equals want, and for the failure case assert a specific error using errors.Is / errors.As or require.ErrorContains/require.ErrorIs (i.e., check the wrapped error message or cause, not just non-nil) so the test tightens the failure-path assertion and verifies the exact error returned.internal/config/config_test.go (1)
288-378: Comprehensive integration test - consider addingt.Parallel().This test thoroughly validates the MCP server merging behavior across all four config sources (home TOML, home JSON, workspace TOML, workspace JSON) and correctly asserts both merge strategies:
- TOML overlays use field-level merging (e.g.,
partialretainsCommandfrom global but gainsArgs/Envfrom workspace)- JSON sidecars use whole-object replacement (e.g.,
sidecar.Argsis cleared)Consider adding
t.Parallel()since the test usest.TempDir()andt.Setenv()without shared mutable state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 288 - 378, The test TestLoadMergesTopLevelMCPServersAcrossConfigAndJSONSidecars uses t.TempDir() and t.Setenv() and can safely run in parallel; add t.Parallel() as the first action in that test to allow parallel execution. Locate the TestLoadMergesTopLevelMCPServersAcrossConfigAndJSONSidecars function and insert a call to t.Parallel() before creating workspaceRoot/homeRoot or calling ResolveHomePaths()/EnsureHomeLayout so the test can run concurrently with others.cmd/agh-codegen/main.go (1)
14-15: Drop reflection from JSON canonicalization.
checkJSONFilealready normalizes both payloads throughjson.Unmarshal; re-marshaling those normalized values and comparing bytes avoidsreflect.DeepEqualentirely and lets you remove thereflectimport.♻️ Suggested change
-import ( - "bytes" - "context" - "encoding/json" - "errors" - "fmt" - "log/slog" - "os" - "os/exec" - "os/signal" - "path/filepath" - "reflect" - "strings" +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "os" + "os/exec" + "os/signal" + "path/filepath" + "strings" @@ -func checkJSONFile(path string, want []byte) error { +func checkJSONFile(path string, want []byte) error { got, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { return fmt.Errorf("%s is missing; run codegen", path) @@ wantCanonical, err := canonicalJSON(want) if err != nil { return fmt.Errorf("decode generated json for %q: %w", path, err) } - if !reflect.DeepEqual(gotCanonical, wantCanonical) { + if !bytes.Equal(gotCanonical, wantCanonical) { return fmt.Errorf("%s: %w; run codegen", path, ErrStaleGeneratedFile) } return nil } -func canonicalJSON(data []byte) (any, error) { +func canonicalJSON(data []byte) ([]byte, error) { var value any if err := json.Unmarshal(data, &value); err != nil { return nil, err } - return value, nil + canonical, err := json.Marshal(value) + if err != nil { + return nil, fmt.Errorf("marshal canonical json: %w", err) + } + return canonical, nil }As per coding guidelines, "Never use reflection without performance justification".
Also applies to: 144-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agh-codegen/main.go` around lines 14 - 15, The JSON comparison in checkJSONFile uses reflect.DeepEqual; instead, unmarshal both JSON inputs into interface{} (as already done), re-marshal those normalized values with json.Marshal and compare the resulting byte slices (bytes.Equal or string equality) to determine equality; update the comparison logic in checkJSONFile (and the block around lines 144-163) to use marshaled-byte comparison and remove the reflect import from the file. Ensure error handling for json.Marshal stays consistent and drop any reflect.DeepEqual references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/agh-codegen/main.go`:
- Around line 3-15: The signal handling only cancels on SIGINT (Ctrl-C); update
the shutdown logic to also handle SIGTERM so the formatter subprocess gets the
graceful cancellation path. Modify the signal setup around the
signal.Notify/signal handling (the code that creates ctx/cancel or the signal
channel) to include syscall.SIGTERM (or switch to signal.NotifyContext with both
syscall.SIGINT and syscall.SIGTERM), and add syscall to the imports; ensure the
existing ctx cancellation/cancel() is invoked when SIGTERM arrives so functions
like the formatter that read ctx are notified.
In `@internal/api/core/handlers.go`:
- Around line 596-605: The handler is returning the absolute user home path via
UserHomeDir (h.daemonUserHomeDir()) in the /daemon/status response, leaking
local user/host PII; remove this field from the response payload
(contract.DaemonStatusPayload) or only populate it when an explicit
privileged/debug condition is true. Locate the construction of
contract.DaemonStatusResponse in handlers.go (where UserHomeDir:
h.daemonUserHomeDir() is set) and either delete the UserHomeDir entry entirely
or wrap it in a conditional (e.g., if h.isPrivilegedRequest() or
h.Config.PrivilegedStatusMode) so the field is only included for admin/debug
requests; adjust the contract/consumer expectations accordingly.
- Around line 612-645: The returned error from resolveUserHomeDir must not
include the raw userHomeDir string; update resolveUserHomeDir so that on
ResolvePath failure it returns a sanitized error that omits the original path
(e.g., use a generic message like "resolve user home directory: <redacted>" or
wrap resolveErr without embedding userHomeDir) and keep fallbackUserHomeDir
logic unchanged; ensure daemonUserHomeDir and its logger continue to log the
returned error but will no longer receive the raw path. Reference
resolveUserHomeDir, daemonUserHomeDir, fallbackUserHomeDir, and
aghconfig.ResolvePath when making the change.
In `@internal/config/mcpjson.go`:
- Around line 49-55: During MCP JSON parsing validate that server names are
unique after normalization (trim/space-normalization) and reject duplicates:
after building the combined servers slice (the result of sortedMCPJSONServers
and OverrideMCPServers) iterate through servers, compute the normalized key from
server.Name (trim and any canonicalization used elsewhere), keep a
map[string]int of seen names and if a normalized name already exists return a
descriptive error (use the same error-wrapping style as the existing
Validate/Validate error path) so names like " foo " and "foo" are treated as
duplicates; update the logic near the loop that calls server.Validate (and the
analogous block around lines 100-115) to perform this duplicate check before
accepting the config.
- Around line 84-90: The trailing variable used to detect extra JSON is declared
as an empty interface; change its type to json.RawMessage (replace "var trailing
any" with "var trailing json.RawMessage") so decoder.Decode(&trailing) decodes
into a concrete byte slice, and ensure the file imports "encoding/json" if not
already present; no other logic changes needed in the Decode/error handling
around trailing.
In `@internal/config/provider.go`:
- Around line 207-233: OverrideMCPServers builds the map index using the raw
server.Name but looks up using strings.TrimSpace(server.Name), causing
mismatches for names with surrounding whitespace; update the index population to
use trimmedName := strings.TrimSpace(server.Name) (skip if trimmedName == "")
when iterating merged to set index[trimmedName] = i, and likewise use
trimmedName for adding new entries after append so lookups and stored keys are
consistent with MergeMCPServers; keep existing cloneMCPServer calls and the rest
of the merge logic unchanged.
---
Nitpick comments:
In `@cmd/agh-codegen/main.go`:
- Around line 14-15: The JSON comparison in checkJSONFile uses
reflect.DeepEqual; instead, unmarshal both JSON inputs into interface{} (as
already done), re-marshal those normalized values with json.Marshal and compare
the resulting byte slices (bytes.Equal or string equality) to determine
equality; update the comparison logic in checkJSONFile (and the block around
lines 144-163) to use marshaled-byte comparison and remove the reflect import
from the file. Ensure error handling for json.Marshal stays consistent and drop
any reflect.DeepEqual references.
In `@internal/acp/client_test.go`:
- Around line 501-502: Replace the magic literal -32002 in the test fixture with
the shared constant requestErrorResourceNotFoundCode: locate the
acpsdk.RequestError instance in internal/acp/client_test.go (the test that
currently sets Code: -32002), change Code to use
requestErrorResourceNotFoundCode so the test ties to the canonical sentinel,
ensure the constant is in scope (add or adjust imports if necessary), and run
the tests to confirm no regressions.
In `@internal/api/core/handlers_internal_test.go`:
- Around line 11-72: Convert the three subtests in TestResolveUserHomeDir into a
small table-driven test that iterates cases (e.g., "resolvedLookup",
"fallbackToCanonicalParent", "lookupFailsAndNoFallback") to remove duplication;
for each case call resolveUserHomeDir(homePaths, lookupFunc) (use
aghconfig.ResolveHomePathsFrom to create homePaths) and assert results: for
success cases check got equals want, and for the failure case assert a specific
error using errors.Is / errors.As or require.ErrorContains/require.ErrorIs
(i.e., check the wrapped error message or cause, not just non-nil) so the test
tightens the failure-path assertion and verifies the exact error returned.
In `@internal/config/config_test.go`:
- Around line 288-378: The test
TestLoadMergesTopLevelMCPServersAcrossConfigAndJSONSidecars uses t.TempDir() and
t.Setenv() and can safely run in parallel; add t.Parallel() as the first action
in that test to allow parallel execution. Locate the
TestLoadMergesTopLevelMCPServersAcrossConfigAndJSONSidecars function and insert
a call to t.Parallel() before creating workspaceRoot/homeRoot or calling
ResolveHomePaths()/EnsureHomeLayout so the test can run concurrently with
others.
🪄 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: CHILL
Plan: Pro
Run ID: 765ff8bc-c22a-4cc8-9090-635206d7dfc0
⛔ Files ignored due to path filters (17)
.audits/architectural-analysis-2026-04-10-automation-techspec-review.mdis excluded by!**/*.md.codex/plans/2026-04-10-mcp-sidecars.mdis excluded by!**/*.md.compozy/tasks/automation/_techspec.mdis excluded by!**/*.md.compozy/tasks/automation/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/automation/adrs/adr-002.mdis excluded by!**/*.md.compozy/tasks/automation/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/automation/adrs/adr-004.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-003/_meta.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-003/issue_001.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-003/issue_002.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-003/issue_003.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-003/issue_004.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-003/issue_005.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-003/issue_006.mdis excluded by!**/*.md.compozy/tasks/ext-architecture/reviews-003/issue_007.mdis excluded by!**/*.mdREADME.mdis excluded by!**/*.mdinternal/skills/bundled/skills/agh-agent-setup/SKILL.mdis excluded by!**/*.md
📒 Files selected for processing (26)
cmd/agh-codegen/main.gocmd/agh-codegen/main_test.gointernal/acp/client_test.gointernal/api/core/handlers.gointernal/api/core/handlers_internal_test.gointernal/api/httpapi/handlers_test.gointernal/api/spec/spec_test.gointernal/config/agent.gointernal/config/agent_test.gointernal/config/bootstrap.gointernal/config/config.gointernal/config/config_test.gointernal/config/mcpjson.gointernal/config/mcpjson_test.gointernal/config/merge.gointernal/config/provider.gointernal/config/provider_test.gointernal/session/manager_test.gointernal/skills/loader.gointernal/skills/loader_test.gointernal/skills/mcp_sidecar.gointernal/skills/registry_snapshot.gointernal/skills/registry_test.gointernal/skills/registry_workspace_cache.gointernal/workspace/clone.gointernal/workspace/scanner.go
✅ Files skipped from review due to trivial changes (1)
- internal/config/mcpjson_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/httpapi/handlers_test.go
- cmd/agh-codegen/main_test.go
This was referenced May 23, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.1 This PR prepares the release of version v0.0.1. ### Changelog ## 0.0.1 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This was referenced May 26, 2026
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.1 This PR prepares the release of version v0.0.1. ### Changelog ## 0.0.1 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This was referenced May 26, 2026
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 27, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process - Fix release sync - Decouple release dry-run npm auth - Persist web assets git auth ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated web assets dependency to a newer version for improved stability and performance. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/compozy/agh/pull/211?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
](https://app.coderabbit.ai/change-stack/compozy/agh/pull/211?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This was referenced May 27, 2026
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 27, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-27 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout - Fix release dry-run token contract ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process - Fix release sync - Decouple release dry-run npm auth - Persist web assets git auth - Require npm auth before release merge ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated dependencies to latest versions. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/compozy/agh/pull/214?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
](https://app.coderabbit.ai/change-stack/compozy/agh/pull/214?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests