refactor: module improvements - #29

internal/testutil/testutil_test.go-53-55 (1)

53-55: ⚠️ Potential issue | 🟠 Major

Prevent potential test hang on errCh receive.

Line 53 blocks indefinitely if the cleanup callback exits before sending (for example, due to panic), which can stall the whole test run. Add a timeout guard around the receive.

Proposed fix

-	if err := <-errCh; err != nil {
-		t.Fatal(err)
-	}
+	select {
+	case err := <-errCh:
+		if err != nil {
+			t.Fatal(err)
+		}
+	case <-time.After(time.Second):
+		t.Fatal("cleanup callback did not report result")
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/testutil/testutil_test.go` around lines 53 - 55, The current test
waits indefinitely on receiving from errCh (if err := <-errCh; err != nil {
t.Fatal(err) }), which can hang if the cleanup goroutine never sends; wrap the
receive in a select that also listens on a timeout (e.g., time.After with a
short duration) and fail the test via t.Fatal or t.Fatalf when the timeout
fires; locate the receive around errCh in the test (the lines using errCh and
the t.Fatal call) and replace it with a select that checks for err from errCh
and a timeout case that reports a clear failure message.

internal/extension/host_api_tasks.go-356-364 (1)

356-364: ⚠️ Potential issue | 🟠 Major

Wrap helper errors with operation context before returning.

Lines 359 and 363 return raw errors from h.taskManager() and h.taskActorContext(ctx) respectively. Wrap these with descriptive context using fmt.Errorf("context: %w", err) so that callers can identify whether manager resolution or actor derivation failed. Per coding guidelines: "Use explicit error returns with wrapped context: fmt.Errorf("context: %w", err)".

🔧 Proposed fix

 func (h *HostAPIHandler) taskManagerAndActor(ctx context.Context) (hostAPITaskManager, taskpkg.ActorContext, error) {
 	manager, err := h.taskManager()
 	if err != nil {
-		return nil, taskpkg.ActorContext{}, err
+		return nil, taskpkg.ActorContext{}, fmt.Errorf("resolve task manager: %w", err)
 	}
 	actor, err := h.taskActorContext(ctx)
 	if err != nil {
-		return nil, taskpkg.ActorContext{}, err
+		return nil, taskpkg.ActorContext{}, fmt.Errorf("derive task actor context: %w", err)
 	}
 	return manager, actor, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/extension/host_api_tasks.go` around lines 356 - 364, The helper
errors returned from HostAPIHandler.taskManagerAndActor are not wrapped with
context; update the error returns for calls to h.taskManager() and
h.taskActorContext(ctx) to wrap the underlying error using fmt.Errorf so callers
know which operation failed (e.g., return nil, taskpkg.ActorContext{},
fmt.Errorf("resolve task manager: %w", err) after h.taskManager() and return
nil, taskpkg.ActorContext{}, fmt.Errorf("derive actor context: %w", err) after
h.taskActorContext(ctx)). Ensure you import fmt if not already imported.

internal/bundles/service.go-296-311 (1)

296-311: ⚠️ Potential issue | 🟠 Major

Wrap new error returns in ListActivations with operation context.

The new branch returns raw errors, which weakens debuggability at call sites.

💡 Proposed fix

 	bundleRecords, err := s.store.ListBundleResources(ctx)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("bundles: list bundle resources: %w", err)
 	}
 	bundleLookup := newBundleRecordLookup(bundleRecords)

 	items := make([]ActivationPreview, 0, len(activations))
 	for _, activation := range activations {
 		resolved, resolveErr := s.resolveActivationFromBundleLookup(activation, bundleLookup)
 		if resolveErr != nil {
-			return nil, resolveErr
+			return nil, fmt.Errorf("bundles: resolve activation %q: %w", activation.ID, resolveErr)
 		}
 		inventory, inventoryErr := s.store.ListBundleActivationInventory(ctx, activation.ID)
 		if inventoryErr != nil {
-			return nil, inventoryErr
+			return nil, fmt.Errorf("bundles: list activation inventory %q: %w", activation.ID, inventoryErr)
 		}

As per coding guidelines, **/*.go: "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 `@internal/bundles/service.go` around lines 296 - 311, In ListActivations, the
branch currently returns raw errors from s.store.ListBundleResources,
s.resolveActivationFromBundleLookup, and s.store.ListBundleActivationInventory;
change each return to wrap the underlying error with fmt.Errorf including
operation context and identifiers (e.g., fmt.Errorf("ListActivations: listing
bundle resources: %w", err), fmt.Errorf("ListActivations: resolving activation
%s: %w", activation.ID, resolveErr), fmt.Errorf("ListActivations: listing
inventory for activation %s: %w", activation.ID, inventoryErr)) so callers
receive contextualized errors; update imports to include fmt if needed.

internal/automation/trigger_test.go-93-110 (1)

93-110: 🛠️ Refactor suggestion | 🟠 Major

Use t.Run("Should...") for this new test case.

The assertion is good, but this new case should follow the required subtest pattern.

♻️ Suggested refactor

 func TestRenderTriggerPromptReturnsTrimmedStaticPrompt(t *testing.T) {
 	t.Parallel()
-
-	rendered, err := renderTriggerPrompt("  Static prompt body  ", &ActivationEnvelope{
-		Kind:   "session.stopped",
-		Scope:  AutomationScopeGlobal,
-		Source: ActivationSourceObserver,
-		Data: map[string]any{
-			"session_id": "sess-1",
-		},
-	})
-	if err != nil {
-		t.Fatalf("renderTriggerPrompt() error = %v", err)
-	}
-	if got, want := rendered, "Static prompt body"; got != want {
-		t.Fatalf("renderTriggerPrompt() = %q, want %q", got, want)
-	}
+	t.Run("Should return trimmed static prompt", func(t *testing.T) {
+		t.Parallel()
+
+		rendered, err := renderTriggerPrompt("  Static prompt body  ", &ActivationEnvelope{
+			Kind:   "session.stopped",
+			Scope:  AutomationScopeGlobal,
+			Source: ActivationSourceObserver,
+			Data: map[string]any{
+				"session_id": "sess-1",
+			},
+		})
+		if err != nil {
+			t.Fatalf("renderTriggerPrompt() error = %v", err)
+		}
+		if got, want := rendered, "Static prompt body"; got != want {
+			t.Fatalf("renderTriggerPrompt() = %q, want %q", got, want)
+		}
+	})
 }

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/automation/trigger_test.go` around lines 93 - 110, Update the test
TestRenderTriggerPromptReturnsTrimmedStaticPrompt to use the t.Run("Should
return trimmed static prompt", ...) subtest pattern: rename or wrap the existing
body inside t.Run with a descriptive "Should..." title, move t.Parallel() into
the subtest closure (call t.Parallel() as the first line inside the subtest),
and keep the same calls to renderTriggerPrompt, ActivationEnvelope construction,
error check, and the final equality assertion; reference the existing test
function name TestRenderTriggerPromptReturnsTrimmedStaticPrompt and the
renderTriggerPrompt call to locate and modify the code.

internal/tools/tool.go-54-55 (1)

54-55: 🛠️ Refactor suggestion | 🟠 Major

Wrap returned validation errors with call-site context.

Line 54 currently returns the error directly; please wrap it so callers can identify the failing operation.

Suggested patch

 func (s ToolSource) MarshalText() ([]byte, error) {
 	if err := s.Validate(); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("marshal tool source: %w", err)
 	}
 	return append([]byte(nil), toolSourceBytes[s]...), 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 `@internal/tools/tool.go` around lines 54 - 55, Wrap the validation error
returned from s.Validate with call-site context instead of returning it
directly; replace the direct return (return nil, err) after s.Validate() with a
wrapped error using fmt.Errorf to include descriptive context (e.g., "validate
tool spec" or "s.Validate") and %w to preserve the original error. Ensure fmt is
imported if not present and keep the wrapping at the same location where
s.Validate() is called so callers can identify the failing operation.

internal/workspace/resolver_test.go-761-772 (1)

761-772: 🛠️ Refactor suggestion | 🟠 Major

Refactor new cancellation rollback cases into t.Run("Should...") subtests.

These new cases are clear, but they should follow the repo’s mandatory subtest naming pattern and table-driven default for test cases.

As per coding guidelines, MUST use t.Run("Should...") pattern for ALL test cases and Use table-driven tests with subtests (t.Run) as default in Go tests.

Also applies to: 801-812

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/workspace/resolver_test.go` around lines 761 - 772, Refactor the
test TestRegisterRollsBackWhenResolveContextCanceled to use a t.Run("Should
...") subtest and follow the repository's table-driven subtest pattern: wrap the
current assertion call to assertResolveCancellationRollback inside t.Run("Should
roll back when resolve context is canceled", func(t *testing.T) { ... }) and, if
other similar cases exist (see the related cases around the same file, e.g. the
block referenced at lines ~801-812), convert them into entries of a table of
test cases and iterate with t.Run for each case; keep the invocation of
assertResolveCancellationRollback and the Register call as-is but move them into
the subtest closure so they follow the required naming and table-driven
structure.

internal/workspace/resolver.go-197-197 (1)

197-197: ⚠️ Potential issue | 🟠 Major

Bound rollback deletion with a timeout.

Line 197 detaches cancellation via context.WithoutCancel(ctx), but without a deadline this rollback path can block indefinitely if DeleteWorkspace hangs.

Suggested fix

-		deleteErr := r.store.DeleteWorkspace(context.WithoutCancel(ctx), ws.ID)
+		rollbackCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second)
+		defer cancel()
+		deleteErr := r.store.DeleteWorkspace(rollbackCtx, ws.ID)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/workspace/resolver.go` at line 197, The rollback call currently uses
context.WithoutCancel(ctx) which can block forever; change the DeleteWorkspace
invocation to use a derived context with a bounded deadline (e.g.,
context.WithTimeout(ctx, <smallDuration>)) and ensure you cancel the timeout
when done; update the call site for r.store.DeleteWorkspace(ctx, ws.ID) (replace
the context.WithoutCancel usage around deleteErr) and handle
cancellation/timeout appropriately so the rollback cannot hang indefinitely.

internal/workspace/resolver_crud.go-25-25 (1)

25-25: ⚠️ Potential issue | 🟠 Major

Use a bounded detached context for rollback delete.

Line 25 has the same unbounded rollback-context issue; if store deletion stalls, this can hang registration flow.

Suggested fix

 import (
 	"context"
 	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
+	"time"
 )
@@
-		deleteErr := r.store.DeleteWorkspace(context.WithoutCancel(ctx), ws.ID)
+		rollbackCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second)
+		defer cancel()
+		deleteErr := r.store.DeleteWorkspace(rollbackCtx, ws.ID)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/workspace/resolver_crud.go` at line 25, The rollback delete is using
an unbounded detached context (context.WithoutCancel(ctx)) which can hang;
replace it with a bounded detached context created via
context.WithTimeout(context.Background(), <reasonableDuration>) and defer cancel
before calling r.store.DeleteWorkspace so the rollback of ws.ID cannot stall
indefinitely; update the call site referencing r.store.DeleteWorkspace and the
deleteErr handling to use the new bounded context and cancel function.

internal/workref/ref_test.go-15-30 (1)

15-30: ⚠️ Potential issue | 🟠 Major

Adopt the required t.Run("Should...") naming pattern.

Case/suite names currently don’t follow the mandated Should... format for test cases.

✅ Suggested rename updates

- name:      "trims leading and trailing whitespace",
+ name:      "Should trim leading and trailing whitespace",
- name:      "preserves interior whitespace",
+ name:      "Should preserve interior whitespace",
- name:      "collapses whitespace only values to empty strings",
+ name:      "Should collapse whitespace-only values to empty strings",

- runConstructorSuite(t, "NewPath", NewPath, func(id string, value string) PathRef {
+ runConstructorSuite(t, "Should construct NewPath", NewPath, func(id string, value string) PathRef {
...
- runConstructorSuite(t, "NewRoot", NewRoot, func(id string, value string) RootRef {
+ runConstructorSuite(t, "Should construct NewRoot", NewRoot, func(id string, value string) RootRef {

As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases".

Also applies to: 74-81

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/workref/ref_test.go` around lines 15 - 30, The test case names in
ref_test.go do not follow the required t.Run("Should...") naming pattern; update
each table-driven case's name field (the "name" values in the test case slice)
to start with "Should ..." (e.g., "Should trim leading and trailing whitespace")
and ensure the t.Run invocation uses that updated name (t.Run(tc.name, ...)).
Apply the same change to the other test block referenced around the later cases
(the group currently at the 74-81 range) so all uses of t.Run and the test case
"name" fields conform to the "Should..." pattern.

internal/registry/installer_checksum.go-115-119 (1)

115-119: ⚠️ Potential issue | 🟠 Major

Don’t canonicalize symlink targets before hashing.

filepath.Clean(target) can collapse different symlink payloads into the same normalized value, weakening checksum fidelity. Hash the raw Readlink target (optionally slash-normalized only).

🔧 Proposed fix

-		normalizedTarget := filepath.ToSlash(filepath.Clean(target))
+		normalizedTarget := filepath.ToSlash(target)
 		return writeInstallChecksumString(
 			hasher,
 			fmt.Sprintf("symlink:%s\nmode:%#o\ntarget:%s\n", normalizedPath, info.Mode().Perm(), normalizedTarget),
 		)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/registry/installer_checksum.go` around lines 115 - 119, The code
currently canonicalizes symlink targets with filepath.Clean before hashing,
which can collapse distinct symlink payloads; instead preserve the raw Readlink
target and only normalize path separators. Replace the use of
filepath.Clean(target) with a raw-target approach (e.g., use
filepath.ToSlash(target) but do not call filepath.Clean) when building the
string passed to writeInstallChecksumString (refer to normalizedPath, hasher,
and writeInstallChecksumString in this block) so the checksum is computed from
the uncanonicalized symlink payload.

internal/registry/installer_test.go-554-593 (1)

554-593: ⚠️ Potential issue | 🟠 Major

Use required t.Run("Should...") subtest structure for this case.

This new test is valid functionally, but it bypasses the required subtest naming/pattern. Please convert this into table-driven subtests using t.Run("Should...").

As per coding guidelines, **/*_test.go: "MUST use t.Run("Should...") pattern for ALL test cases" and "Use table-driven tests with subtests (t.Run) as default".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/registry/installer_test.go` around lines 554 - 593, The test
TestComputeInstallChecksumStableAcrossCreationOrder currently implements its
assertions directly; convert it to use the required t.Run("Should ...") subtest
pattern (preferably table-driven) by defining a test case entry (e.g., name
"Should produce stable checksum regardless of creation order") and moving the
existing setup, calls to populate, computeInstallChecksum and the equality
assertions into a t.Run subtest closure; keep references to the helper functions
populate and writeTestFile and the target function computeInstallChecksum
unchanged, and ensure t.Parallel() remains appropriate (call t.Parallel() inside
each subtest if parallelism is desired).

internal/fileutil/atomic_test.go-84-109 (2)

84-109: 🛠️ Refactor suggestion | 🟠 Major

Use a t.Run("Should...") subtest for this new test case.

Please wrap this scenario in a named subtest to match the repository’s required test structure.

🧪 Suggested structure

 func TestAtomicWriteFilePreservesLiteralWhitespaceInPath(t *testing.T) {
 	t.Parallel()
-	path := filepath.Join(t.TempDir(), "target.txt ")
-	trimmedPath := strings.TrimSpace(path)
-	...
+	t.Run("Should preserve literal trailing whitespace in path", func(t *testing.T) {
+		t.Parallel()
+		path := filepath.Join(t.TempDir(), "target.txt ")
+		trimmedPath := strings.TrimSpace(path)
+		...
+	})
 }

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/fileutil/atomic_test.go` around lines 84 - 109, Wrap the existing
TestAtomicWriteFilePreservesLiteralWhitespaceInPath body in a named subtest
using t.Run (e.g., t.Run("Should preserve literal trailing whitespace in path",
func(t *testing.T) { ... })) so the scenario follows the repository's required
t.Run("Should...") pattern; keep the same assertions and references to
AtomicWriteFile, os.ReadFile, and os.Stat inside that subtest and run
t.Parallel() either at the top of the parent test or inside the subtest as
appropriate.

---

84-109: ⚠️ Potential issue | 🟠 Major

Guard this test on Windows to avoid false failures.

On Windows, the underlying Win32 APIs normalize file paths by trimming trailing spaces before file creation, regardless of whether Go uses the \\?\ prefix. This means when AtomicWriteFile is called with a path containing trailing whitespace like "target.txt ", the file is created as target.txt (trimmed). The test's assertion that the trimmed path does not exist will fail on Windows, even if the function works correctly.

Add a platform guard at the start of the test:

 func TestAtomicWriteFilePreservesLiteralWhitespaceInPath(t *testing.T) {
 	t.Parallel()
+	if runtime.GOOS == "windows" {
+		t.Skip("trailing-space filenames are normalized by Win32 APIs")
+	}
 
 	path := filepath.Join(t.TempDir(), "target.txt ")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/fileutil/atomic_test.go` around lines 84 - 109, The test
TestAtomicWriteFilePreservesLiteralWhitespaceInPath fails on Windows because
Win32 trims trailing spaces; guard the test by detecting the platform
(runtime.GOOS == "windows") at the start of the function and call t.Skip (or
t.Skipf) to skip on Windows; keep the rest of the test (path creation with
filepath.Join, AtomicWriteFile call, os.ReadFile verification and os.Stat check
on the trimmedPath) unchanged so behavior is still validated on non-Windows
systems.

internal/api/core/sse.go-77-85 (1)

77-85: ⚠️ Potential issue | 🟠 Major

Wrap write failures with operation context.

At Line 77 onward, raw return err drops which SSE write step failed (id, event, data, terminator), making debugging harder and violating error-wrapping policy.

Proposed fix

 	if id != "" {
 		if _, err := io.WriteString(writer, "id: "); err != nil {
-			return err
+			return fmt.Errorf("sse: write id prefix: %w", err)
 		}
 		if _, err := io.WriteString(writer, id); err != nil {
-			return err
+			return fmt.Errorf("sse: write id value: %w", err)
 		}
 		if _, err := io.WriteString(writer, "\n"); err != nil {
-			return err
+			return fmt.Errorf("sse: write id terminator: %w", err)
 		}
 	}
 	if len(names) > 0 && strings.TrimSpace(names[0]) != "" {
 		if _, err := io.WriteString(writer, "event: "); err != nil {
-			return err
+			return fmt.Errorf("sse: write event prefix: %w", err)
 		}
 		if _, err := io.WriteString(writer, names[0]); err != nil {
-			return err
+			return fmt.Errorf("sse: write event value: %w", err)
 		}
 		if _, err := io.WriteString(writer, "\n"); err != nil {
-			return err
+			return fmt.Errorf("sse: write event terminator: %w", err)
 		}
 	}
 	if _, err := io.WriteString(writer, "data: "); err != nil {
-		return err
+		return fmt.Errorf("sse: write data prefix: %w", err)
 	}
 	if _, err := writer.Write(raw); err != nil {
-		return err
+		return fmt.Errorf("sse: write data payload: %w", err)
 	}
 	if _, err := io.WriteString(writer, "\n\n"); err != nil {
-		return err
+		return fmt.Errorf("sse: write message terminator: %w", err)
 	}

As per coding guidelines, "Use explicit error returns with wrapped context: fmt.Errorf("context: %w", err)".

Also applies to: 88-96, 98-105

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/core/sse.go` around lines 77 - 85, Writes to the SSE stream
using io.WriteString in the id/event/data/terminator blocks currently return raw
errors which lose context; update each io.WriteString call (the id write, the
"\n" newline after id, the "event: " write, event value write, the "data: "
write, data value write, and the final "\n\n" terminator write) to wrap returned
errors with fmt.Errorf including the operation (e.g., "write id: %w", "write id
newline: %w", "write event: %w", "write event newline: %w", "write data: %w",
"write data newline: %w", "write terminator: %w") so callers can see which SSE
step failed—use the existing writer variable and keep existing control flow.

internal/transcript/transcript_test.go-402-422 (1)

402-422: ⚠️ Potential issue | 🟠 Major

Use the required t.Run("Should...") pattern for this test case.

Line 402 adds a standalone test body; this repo’s test policy requires subtests using t.Run("Should...") (table-driven by default).

Suggested structure

 func TestBuildToolResultDecodesRawJSONObjectPayload(t *testing.T) {
 	t.Parallel()
-
-	result := buildToolResult("Read", false, "", json.RawMessage(`{
-		"stdout":"workspace\n",
-		"content":"workspace\n",
-		"structuredPatch":{"ops":[{"op":"replace","path":"/tmp/demo.txt"}]}
-	}`))
-	if result == nil {
-		t.Fatal("buildToolResult() = nil, want populated result")
-	}
-	if got := result.Stdout; got != "workspace\n" {
-		t.Fatalf("Stdout = %q, want %q", got, "workspace\n")
-	}
-	if got := result.Content; got != "workspace\n" {
-		t.Fatalf("Content = %q, want %q", got, "workspace\n")
-	}
-	if len(result.StructuredPatch) == 0 {
-		t.Fatal("StructuredPatch = empty, want preserved patch payload")
-	}
+	t.Run("Should decode raw JSON object payload", func(t *testing.T) {
+		result := buildToolResult("Read", false, "", json.RawMessage(`{
+			"stdout":"workspace\n",
+			"content":"workspace\n",
+			"structuredPatch":{"ops":[{"op":"replace","path":"/tmp/demo.txt"}]}
+		}`))
+		if result == nil {
+			t.Fatal("buildToolResult() = nil, want populated result")
+		}
+		if got := result.Stdout; got != "workspace\n" {
+			t.Fatalf("Stdout = %q, want %q", got, "workspace\n")
+		}
+		if got := result.Content; got != "workspace\n" {
+			t.Fatalf("Content = %q, want %q", got, "workspace\n")
+		}
+		if len(result.StructuredPatch) == 0 {
+			t.Fatal("StructuredPatch = empty, want preserved patch payload")
+		}
+	})
 }

As per coding guidelines, **/*_test.go: MUST use t.Run("Should...") pattern for ALL test cases and use table-driven tests with subtests (t.Run) as default.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/transcript/transcript_test.go` around lines 402 - 422, The test
TestBuildToolResultDecodesRawJSONObjectPayload must be converted to use the
repository's subtest pattern: wrap the existing assertions inside a
t.Run("Should decode raw JSON object payload", func(t *testing.T) { ... })
subtest (keeping t.Parallel() at the top-level or inside the subtest per project
convention), or refactor to a small table-driven slice with a single case and a
t.Run for each case; keep the same calls and checks against buildToolResult(...)
and its fields (Stdout, Content, StructuredPatch) but move them into the t.Run
body so the test follows the required t.Run("Should...") pattern.

internal/skills/catalog.go-112-121 (1)

112-121: ⚠️ Potential issue | 🟠 Major

Fix Unicode truncation bug that pads descriptions below the 200-rune limit.

The current implementation incorrectly truncates descriptions with 198–200 runes when byte length exceeds 200. For multibyte characters like "界" (3 bytes each), a 198-rune string is 594 bytes and triggers truncation, producing 197 runes + ellipsis (200 total)—padding short descriptions rather than preserving them. Only truncate when rune count exceeds catalogDescriptionLimit (201+).

Proposed fix

 func truncateCatalogDescription(description string) string {
 	if len(description) <= catalogDescriptionLimit {
 		return description
 	}
 
 	truncationLimit := catalogDescriptionLimit - len(catalogEllipsis)
-	runeCount := 0
-	for idx := range description {
-		if runeCount == truncationLimit {
-			return description[:idx] + catalogEllipsis
-		}
-		runeCount++
-	}
+	runeCount := 0
+	truncateAt := -1
+	for idx := range description {
+		if runeCount == truncationLimit {
+			truncateAt = idx
+		}
+		if runeCount == catalogDescriptionLimit && truncateAt >= 0 {
+			return description[:truncateAt] + catalogEllipsis
+		}
+		runeCount++
+	}
 
 	return description
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/skills/catalog.go` around lines 112 - 121, The truncation uses byte
length instead of rune count causing multibyte descriptions to be incorrectly
trimmed; change the logic in the block that references description,
catalogDescriptionLimit and catalogEllipsis to operate on runes: convert
description to []rune, use len(runes) (or utf8.RuneCountInString) to decide if
truncation is needed, and if truncating slice the rune slice at the proper
truncationLimit (catalogDescriptionLimit - len([]rune(catalogEllipsis))) and
append catalogEllipsis; return the original description when the rune count is
<= catalogDescriptionLimit.

internal/environment/daytona/perf_bench_test.go-67-80 (1)

67-80: ⚠️ Potential issue | 🟠 Major

Clean up extracted trees between benchmark iterations.

This loop keeps every extracted workspace under baseDest. Long runs can skew results or exhaust temp storage in CI. Remove dest after each iteration with the timer stopped.

Suggested change

 	for i := 0; b.Loop(); i++ {
 		b.StopTimer()
 		dest := filepath.Join(baseDest, fmt.Sprintf("run-%d", i))
 		if err := os.MkdirAll(dest, 0o755); err != nil {
 			b.Fatalf("MkdirAll(%q) error = %v", dest, err)
 		}
 		b.StartTimer()
 		extracted, err := extractTar(dest, bytes.NewReader(archive.Bytes()))
+		b.StopTimer()
 		if err != nil {
 			b.Fatalf("extractTar() error = %v", err)
 		}
 		if extracted.Files != expectedFiles {
 			b.Fatalf("extractTar() files = %d, want %d", extracted.Files, expectedFiles)
 		}
+		if err := os.RemoveAll(dest); err != nil {
+			b.Fatalf("RemoveAll(%q) error = %v", dest, err)
+		}
+		b.StartTimer()
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/environment/daytona/perf_bench_test.go` around lines 67 - 80, The
benchmark loop creates a new extracted workspace at dest each iteration and
never removes it, which can exhaust temp space; after you stop the timer
(between b.StopTimer() and b.StartTimer()) remove the created directory dest
(e.g., via os.RemoveAll(dest)) to clean up even on success, and ensure removal
also runs on error paths (or use defer after MkdirAll but before StartTimer) so
extractTar, dest, and the benchmark loop free per-iteration state.

internal/bridgesdk/perf_bench_test.go-66-71 (1)

66-71: ⚠️ Potential issue | 🟠 Major

Handle ignored Close/Serve errors in the peer benchmark.

Several operational errors are discarded with _, which can mask harness failures and make benchmark outcomes misleading.

🔧 Suggested fix

 import (
 	"context"
+	"errors"
 	"encoding/json"
 	"net"
 	"testing"
 	"time"
 )
@@
-	defer func() {
-		_ = leftConn.Close()
-	}()
-	defer func() {
-		_ = rightConn.Close()
-	}()
-
 	left := NewPeer(leftConn, leftConn)
 	right := NewPeer(rightConn, rightConn)
@@
-	done := make(chan struct{}, 2)
+	done := make(chan error, 2)
 	go func() {
-		_ = left.Serve(ctx)
-		done <- struct{}{}
+		done <- left.Serve(ctx)
 	}()
 	go func() {
-		_ = right.Serve(ctx)
-		done <- struct{}{}
+		done <- right.Serve(ctx)
 	}()
@@
 	b.StopTimer()
 
 	cancel()
-	_ = leftConn.Close()
-	_ = rightConn.Close()
-	<-done
-	<-done
+	if err := leftConn.Close(); err != nil && !errors.Is(err, net.ErrClosed) {
+		b.Fatalf("leftConn.Close() error = %v", err)
+	}
+	if err := rightConn.Close(); err != nil && !errors.Is(err, net.ErrClosed) {
+		b.Fatalf("rightConn.Close() error = %v", err)
+	}
+	for i := 0; i < 2; i++ {
+		if err := <-done; err != nil &&
+			!errors.Is(err, context.Canceled) &&
+			!errors.Is(err, net.ErrClosed) {
+			b.Fatalf("Serve() error = %v", err)
+		}
+	}
 }

As per coding guidelines, "Never ignore errors with `_` — every error must be handled or have a written justification."

Also applies to: 87-94, 112-116

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/bridgesdk/perf_bench_test.go` around lines 66 - 71, The deferred
Close calls and other Serve/Close returns in perf_bench_test.go currently
discard errors (e.g., leftConn.Close(), rightConn.Close(), and the peer Serve
calls), which can mask failures; update each to check the returned error and
fail the test or log it (use t.Fatalf/t.Errorf or testing.T helper) instead of
using `_`, and propagate or assert no error for the Serve and Close operations
(identify and modify calls to leftConn.Close, rightConn.Close, and the peer
Serve/Close invocations near the referenced blocks so they validate the error
and surface any failures).

internal/hooks/async_clone.go-260-275 (1)

260-275: ⚠️ Potential issue | 🟠 Major

cloneAnyValue still aliases typed nested containers.

On these lines, the deep copy only recurses through map[string]any and []any. Values such as []map[string]any, map[string][]string, or any other concrete slice/map stored inside AutomationTriggerPreFirePayload.Payload fall into default and stay shared, so async hooks can still observe caller mutations.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/hooks/async_clone.go` around lines 260 - 275, The bug is that
cloneAnyValue only handles a few concrete container types and returns default
for other concrete maps/slices (e.g., []map[string]any, map[string][]string),
leaving nested containers shared; update cloneAnyValue to detect any slice or
map via reflection and recursively deep-copy their elements/values by calling
cloneAnyValue (so containers of any concrete element/key/value types are
cloned), while preserving the existing specialized helpers (cloneAnyMap,
cloneAnySlice, cloneStringMap, cloneStringSlice, cloneRawMessage, cloneRawJSON)
for performance; ensure the reflected map/slice cloning is used for values
stored in AutomationTriggerPreFirePayload.Payload so nested concrete containers
are not aliased.

internal/session/stop_reason.go-111-112 (1)

111-112: ⚠️ Potential issue | 🟠 Major

TOCTOU race in error suppression logic at lines 120–121.

If the process exits between the snapshot at line 111 and the Stop() call at line 112, a benign proc.Wait() error can be returned as a hard failure. The error check at line 120 uses only the pre-call doneBeforeStop snapshot; it should also verify the current state.

Change line 120 from:

if stopErr != nil && !doneBeforeStop {

to:

if stopErr != nil && !doneBeforeStop && !isProcessDone(proc) {

This ensures errors are suppressed only when the process was not done before, during, and after the Stop() call.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/session/stop_reason.go` around lines 111 - 112, The TOCTOU bug
occurs because we only snapshot doneBeforeStop before calling m.driver.Stop and
then suppress stopErr based solely on that snapshot; update the error check to
also re-check the current process state to avoid treating a benign proc.Wait()
as a hard failure. After calling m.driver.Stop(ctx, proc) and obtaining stopErr,
change the conditional that uses doneBeforeStop to also call isProcessDone(proc)
(e.g., use stopErr != nil && !doneBeforeStop && !isProcessDone(proc)) so errors
are only suppressed when the process was not done before, during, and after
Stop; reference doneBeforeStop, isProcessDone, m.driver.Stop, stopErr, and proc
when making the change.

internal/observe/tasks.go-861-886 (1)

861-886: ⚠️ Potential issue | 🟠 Major

Honor OriginKind when filtering audit-backed metrics.

TaskMetricsQuery.OriginKind is applied to runs and events, but not to ingress audits. For any non-network origin view, DuplicateIngressTotal and ChannelMismatchTotal can still come back non-zero because this function keeps all network audit rows.

Suggested fix

 func filterTaskIngressAudits(audits []store.NetworkAuditEntry, query TaskMetricsQuery) []store.NetworkAuditEntry {
+	if origin := query.OriginKind.Normalize(); origin != "" && origin != taskpkg.OriginKindNetwork {
+		return nil
+	}
 	channel := strings.TrimSpace(query.NetworkChannel)
 	var filtered []store.NetworkAuditEntry
 	for i, item := range audits {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/observe/tasks.go` around lines 861 - 886, filterTaskIngressAudits
currently always keeps network audit rows regardless of
TaskMetricsQuery.OriginKind; update it to honor query.OriginKind by only
including network ingress audits when OriginKind is empty or explicitly
specifies the network origin. In practice, inside filterTaskIngressAudits (which
iterates audits []store.NetworkAuditEntry) add a guard that treats
accepted=false when query.OriginKind is non-empty and not equal to the network
origin value (the enum/string used by TaskMetricsQuery for network origins), so
DuplicateIngressTotal/ChannelMismatchTotal are excluded for non-network origin
views.

internal/session/query.go-225-231 (1)

225-231: ⚠️ Potential issue | 🟠 Major

Add volume-name check to block Windows drive-relative paths.

filepath.IsAbs("C:foo") returns false on Windows, and filepath.Join(base, "C:foo") can resolve outside the sessions root. The separator check does not catch this case since drive-relative paths contain no slash or backslash. This is a documented behavior in Go's filepath package that creates a path traversal vulnerability.

Suggested fix

-	if filepath.IsAbs(target) || target == "." || target == ".." {
+	if filepath.IsAbs(target) || filepath.VolumeName(target) != "" || target == "." || target == ".." {
 		return "", fmt.Errorf("%w: %s", ErrSessionNotFound, target)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/session/query.go` around lines 225 - 231, The current validation
misses Windows drive-relative paths like "C:foo"; add an explicit check for a
drive-letter + colon prefix and return ErrSessionNotFound when found (e.g., if
len(target) >= 2 && target[1] == ':' && unicode.IsLetter(rune(target[0])) ).
Place this check alongside the existing filepath.IsAbs/contains-separator checks
(before filepath.Clean) so functions handling target (the variable checked, and
the error returned using ErrSessionNotFound) will reject drive-relative paths
and prevent traversal outside the sessions root.

internal/hooks/async_clone.go (1)

263-283: Reflection fallback for unknown container types.

The reflection-based cloneDynamicContainer call at line 278 handles edge cases where any-typed values contain nested containers of unknown types. This is appropriate for correctness when cloning arbitrary data structures, though per coding guidelines, consider adding a brief comment explaining why reflection is needed here.

As per coding guidelines: "Never use reflection without performance justification" — while this usage is for correctness rather than performance, documenting the rationale would satisfy the spirit of this guideline.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/hooks/async_clone.go` around lines 263 - 283, Add a short comment
above the reflection fallback in cloneAnyValue explaining why
cloneDynamicContainer(reflect.ValueOf(value)) is necessary: state that some
any-typed values may contain nested/unknown container types not covered by the
explicit cases (e.g., custom map/slice types, structs with container fields), so
reflection is used for correctness to deep-clone those edge cases rather than
for performance; keep the comment concise and reference the
cloneDynamicContainer call and the cloneAnyValue function so future readers
understand the trade-off.

internal/bundles/service.go (1)

719-757: Normalize indexed lookup keys to the same case-insensitive policy.

findBundleResourceRecordIndexed still falls back to a full scan whenever activation casing differs from the stored bundle casing, because newBundleRecordKey only trims. Lowercasing the key at construction time keeps the fast path effective for mixed-case inputs and matches the behavior of findBundleResourceRecord.

💡 Proposed refactor

 func newBundleRecordKey(extensionName string, bundleName string) bundleRecordKey {
 	return bundleRecordKey{
-		extensionName: strings.TrimSpace(extensionName),
-		bundleName:    strings.TrimSpace(bundleName),
+		extensionName: strings.ToLower(strings.TrimSpace(extensionName)),
+		bundleName:    strings.ToLower(strings.TrimSpace(bundleName)),
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/bundles/service.go` around lines 719 - 757, The indexed lookup
misses mixed-case activations because newBundleRecordKey only trims whitespace
but doesn’t normalize case; update newBundleRecordKey to also strings.ToLower
the trimmed extensionName and bundleName, so newBundleRecordLookup builds the
exact map with lowercase keys and findBundleResourceRecordIndexed’s fast-path
uses the same case-insensitive form (leave the fallback comparisons as-is or
keep using EqualFold for safety).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/bundles/service.go (1)

722-735: Consider logging or returning error on duplicate bundle keys.

If multiple records share the same normalized (extensionName, bundleName) key, line 729 silently overwrites the earlier entry. While duplicates are unlikely in practice, silent data loss can make debugging difficult if it ever occurs.

💡 Option: warn on duplicate keys

 func newBundleRecordLookup(records []resources.Record[BundleResourceSpec]) bundleRecordLookup {
 	exact := make(map[bundleRecordKey]resources.Record[BundleResourceSpec], len(records))
 	for _, record := range records {
 		key := newBundleRecordKey(record.Spec.ExtensionName, record.Spec.Bundle.Name)
 		if key.extensionName == "" || key.bundleName == "" {
 			continue
 		}
+		if _, exists := exact[key]; exists {
+			// Log or handle duplicate - for now just keep first
+			continue
+		}
 		exact[key] = record
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/bundles/service.go` around lines 722 - 735, newBundleRecordLookup
silently overwrites earlier entries when multiple records normalize to the same
bundleRecordKey; update the loop in newBundleRecordLookup to detect duplicates
by checking if exact[key] already exists and then either log a warning (using
your package logger) or return an error/validation result (propagate from
newBundleRecordLookup) including the conflicting key and record identifiers;
ensure you reference bundleRecordKey, newBundleRecordLookup, and the exact map
so callers can handle or surface duplicate bundle keys instead of losing the
first entry.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes