fix: keep managed skill loading on native seam - #323

internal/acp/agent_transport_unix.go (2)

56-57: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Consider bounding body and idle time on the managed transport server.

The server sets ReadHeaderTimeout only. A managed process that opens a connection and stalls the request body or keeps an idle connection holds a goroutine until the transport closes. The socket lives in a 0700 temp directory and only the same local user reaches it, so this is hardening rather than an exploitable path.

Add ReadTimeout, WriteTimeout, and IdleTimeout. Set ReadTimeout and WriteTimeout high enough for large skill content responses.

♻️ Proposed timeout bounds

-const defaultManagedTransportReadHeaderTimeout = 5 * time.Second
+const (
+	defaultManagedTransportReadHeaderTimeout = 5 * time.Second
+	defaultManagedTransportReadTimeout       = 30 * time.Second
+	defaultManagedTransportWriteTimeout      = 60 * time.Second
+	defaultManagedTransportIdleTimeout       = 60 * time.Second
+)

-	server := &http.Server{Handler: handler, ReadHeaderTimeout: defaultManagedTransportReadHeaderTimeout}
+	server := &http.Server{
+		Handler:           handler,
+		ReadHeaderTimeout: defaultManagedTransportReadHeaderTimeout,
+		ReadTimeout:       defaultManagedTransportReadTimeout,
+		WriteTimeout:      defaultManagedTransportWriteTimeout,
+		IdleTimeout:       defaultManagedTransportIdleTimeout,
+	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/acp/agent_transport_unix.go` around lines 56 - 57, Update the
http.Server construction in the managed transport setup to include ReadTimeout,
WriteTimeout, and IdleTimeout alongside ReadHeaderTimeout. Use the existing
managed transport timeout configuration or add appropriately generous bounds for
large skill-content request and response bodies, while ensuring idle connections
are eventually closed.

Source: Linters/SAST tools

---

39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the silent no-op return contract.

startManagedAgentTransport returns (nil, "", nil) when daemonSocket, sessionID, or agentName is blank. The signature does not signal this to callers. A caller that assumes a non-nil transport on err == nil will dereference nil. bindManagedAgentTransport and the lifecycle test both check transport == nil explicitly, so current callers are correct, but the behavior is implicit.

Add a doc comment on the function stating that missing identity or socket inputs disable the transport and produce a nil transport with a nil error.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/acp/agent_transport_unix.go` around lines 39 - 42, Add a doc comment
to startManagedAgentTransport documenting that blank daemonSocket, sessionID, or
agentName disables the transport and returns a nil transport with a nil error.
Keep the existing return behavior unchanged and make the contract clear for
callers.

internal/acp/terminal_agent_transport_proxy.go (1)

30-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Set explicit timeouts on the outbound daemon transport.

The http.Transport sets only DialContext. It has no ResponseHeaderTimeout, IdleConnTimeout, or ExpectContinueTimeout. If the daemon accepts the connection and never responds, the proxied request stays open until the managed process cancels it. The repository guideline requires explicit timeouts on outbound network calls.

Add bounded timeouts to the transport.

♻️ Proposed transport timeouts

 	transport := &http.Transport{
 		DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) {
 			var dialer net.Dialer
 			return dialer.DialContext(ctx, "unix", socketPath)
 		},
+		ResponseHeaderTimeout: 30 * time.Second,
+		IdleConnTimeout:       60 * time.Second,
+		ExpectContinueTimeout: time.Second,
 	}

As per coding guidelines: "All outbound HTTP or network calls must use clients with explicit timeouts; http.DefaultClient is forbidden in production paths."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/acp/terminal_agent_transport_proxy.go` around lines 30 - 35, Update
the http.Transport in the terminal agent transport setup to define explicit
bounded ResponseHeaderTimeout, IdleConnTimeout, and ExpectContinueTimeout
values, while preserving the existing Unix-socket DialContext behavior.

Source: Coding guidelines

internal/acp/terminal_identity_env.go (1)

28-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Source COMPOZY_AGENT_NAME from a shared constant.

internal/cli/whoami.go already owns envAgentName = "COMPOZY_AGENT_NAME" for reading it, but internal/acp/terminal_identity_env.go uses the same literal in the managed terminal allowlist. Keep a shared constant for this consumer reference to avoid out-of-sync renames.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/acp/terminal_identity_env.go` around lines 28 - 41, Update
isManagedTerminalEnvName to replace the hardcoded "COMPOZY_AGENT_NAME" entry
with the shared envAgentName constant already defined in internal/cli/whoami.go,
preserving the existing allowlist behavior and avoiding a duplicate literal.

internal/acp/launcher.go (1)

30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Define the managed transport capability interface at its consumer.

prepareManagedAgentTransport consumes managedAgentTransportLauncher in internal/acp/agent_transport.go. Move the interface declaration there. Keep the compile-time assertion in this file.

As per coding guidelines, define interfaces where they are consumed.

Proposed refactor

--- a/internal/acp/launcher.go
-type managedAgentTransportLauncher interface {
-	supportsManagedAgentTransport()
-}
-
--- a/internal/acp/agent_transport.go
+type managedAgentTransportLauncher interface {
+	supportsManagedAgentTransport()
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/acp/launcher.go` around lines 30 - 32, Move the
managedAgentTransportLauncher interface declaration from this file to the
consumer location in prepareManagedAgentTransport within agent_transport.go,
while preserving the existing compile-time assertion here and leaving the
interface method unchanged.

Source: Coding guidelines

internal/cli/client_managed_transport_unix_test.go (1)

74-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use t.Context() for the client calls.

Line 32 already uses t.Context(). Lines 74 and 81 use context.Background(), so a hung request is not cancelled when the test ends.

♻️ Proposed consistency fix

-		skill, err := client.GetSkill(context.Background(), "release-signal", query)
+		skill, err := client.GetSkill(t.Context(), "release-signal", query)
 		if err != nil {
 			t.Fatalf("GetSkill() error = %v", err)
 		}
 		if skill.Name != "release-signal" {
 			t.Fatalf("GetSkill().Name = %q, want release-signal", skill.Name)
 		}
-		content, err := client.GetSkillContent(context.Background(), "release-signal", query)
+		content, err := client.GetSkillContent(t.Context(), "release-signal", query)

Remove the now-unused "context" import.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/client_managed_transport_unix_test.go` around lines 74 - 81,
Update the client calls in the test, including GetSkill and GetSkillContent, to
use t.Context() instead of context.Background() so requests are cancelled with
the test. Remove the context import if it is no longer used.

internal/api/core/skill_scope.go (1)

51-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Collapse the duplicated ForWorkspace branch and map its errors.

Lines 51-54 call ForWorkspace with the same value in both branches, because resolved is nil in the second branch. The workspace path also skips mapSkillScopeError, so its errors are not classified and reach StatusForSkillError as unknown errors, which returns 500.

♻️ Proposed simplification with consistent error mapping

-	if resolved != nil {
-		return h.SkillsRegistry.ForWorkspace(c.Request.Context(), resolved)
-	}
-	return h.SkillsRegistry.ForWorkspace(c.Request.Context(), nil)
+	skillList, err := h.SkillsRegistry.ForWorkspace(c.Request.Context(), resolved)
+	if err != nil {
+		return nil, mapSkillScopeError(err)
+	}
+	return skillList, nil

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/core/skill_scope.go` around lines 51 - 54, In the handler branch
around resolved, collapse the duplicated SkillsRegistry.ForWorkspace calls into
one call using resolved, then pass its returned error through mapSkillScopeError
before converting it with StatusForSkillError. Preserve the existing workspace
resolution behavior while ensuring both resolved and nil workspace paths receive
consistent error classification.

internal/cli/client_test.go (1)

1975-2001: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the other three changed skill methods, or narrow the test name.

The suite name is plural, but only GetSkillContent is asserted. ListSkills, GetSkill, and GetSkillShadows also moved to doAgentJSON in this PR and have no header assertion. Add a subtest per method with the same round-tripper pattern, so a dropped query.Caller argument on any of them fails the suite.

As per path instructions: "Focus on critical paths … Verify tests can fail when business logic changes."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/client_test.go` around lines 1975 - 2001, The
TestUnixSocketClientSkillMethodsSendIdentityHeaders test currently covers only
GetSkillContent despite claiming coverage for all skill methods. Add equivalent
subtests and round-tripper header assertions for ListSkills, GetSkill, and
GetSkillShadows, passing credentials through each method’s query.Caller and
validating the expected request so dropped identity propagation fails the test.

Source: Path instructions

internal/api/core/skills_test.go (1)

1331-1341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log the workspace-access request instead of asserting inside AuthorizeFn.

ValidateWorkspaceAccess returns without calling the policy when TargetWorkspaceID is empty or equals the actor workspace, so the t.Fatalf inside AuthorizeFn makes other requests fail incorrectly. Let the fixture inspect the request for non-empty cross-workspace targets, and keep the assertions in each subtest. The denied Decision{Source: SourceDenied} with no error already maps to ErrIdentityUnauthorized.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/core/skills_test.go` around lines 1331 - 1341, Update the
AuthorizeFn fixture used by ValidateWorkspaceAccess to record or log requests
instead of calling t.Fatalf on TargetWorkspaceID. Preserve the denied
Decision{Source: workspaceaccess.SourceDenied} return, and move target-workspace
assertions into each relevant subtest so empty or same-workspace requests remain
valid without invoking the policy.

Fix all unresolved CodeRabbit comments on this PR:

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

internal/cli/skill_daemon_test.go (1)

422-481: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the operator-shell negative case.

Every subtest here asserts rejection. No test in the changed range asserts that skill subcommands still run when COMPOZY_SESSION_ID and COMPOZY_AGENT are both empty. The change details state that the previous session-scoped tests were replaced, so the allowed path may now be uncovered. A future inversion of the condition in ensureSkillCLIUsesSupportedSurface would keep this suite green.

Add one subtest that leaves both markers empty and asserts that skill list reaches the daemon client.

💚 Proposed additional subtest

t.Run("Should allow skill list from an operator shell", func(t *testing.T) {
	t.Parallel()

	clientCalls := 0
	deps := newWorkspaceTestDeps(t, &stubClient{})
	deps.newClient = func(string) (DaemonClient, error) {
		clientCalls++
		return &stubClient{}, nil
	}
	deps.getenv = func(string) string { return "" }

	if _, _, err := executeRootCommand(t, deps, "skill", "list"); err != nil {
		t.Fatalf("skill list error = %v, want success from operator shell", err)
	}
	if clientCalls == 0 {
		t.Fatal("skill list client calls = 0, want daemon access from operator shell")
	}
})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/skill_daemon_test.go` around lines 422 - 481, Add an
operator-shell subtest alongside TestSkillCommandsRejectManagedSessionCLI that
leaves both session and agent environment markers empty, executes “skill list,”
asserts no error, and verifies the configured daemon client is called at least
once. Keep the existing managed-session rejection cases unchanged.

internal/cli/skill_managed_guard.go (1)

10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Align the guidance text with the blocked command set.

The guard blocks every skill subcommand, including install, remove, create, enable, disable, and update. The error text only names the read-only tools compozy__skill_list, compozy__skill_search, and compozy__skill_view. A managed session that runs compozy skill install receives guidance that does not apply to that operation.

Consider separating the two cases: point read operations at the canonical tools, and state that mutating skill operations are operator-shell only.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/skill_managed_guard.go` around lines 10 - 13, Update
errManagedSessionSkillCLIUnsupported to distinguish read-only skill operations
from mutating commands: direct list, search, and view requests to the canonical
compozy tools, while explicitly stating that install, remove, create, enable,
disable, and update require an operator shell.

internal/testutil/acpmock/fixture_test.go (1)

696-696: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse the local constant instead of inlining the text.

Line 653 in this same file already uses currentSkillsCatalogFinalLine. These three sites inline the identical string. Replace the literals with the constant so a text change updates all fixtures at once.

Also applies to: 921-921, 946-946

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/testutil/acpmock/fixture_test.go` at line 696, Replace the three
duplicated prompt string literals in the fixture entries with the existing
currentSkillsCatalogFinalLine constant used earlier in the file, including the
sites near the reported locations, so all fixtures share one maintainable value.

internal/skills/catalog_test.go (1)

458-465: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

These substring assertions are brittle and partly redundant.

TestBuildCatalogFormatsCatalogSortedEscapedAndWithUsageInstructions at line 33 already asserts the exact catalog output, including the managed loading line. Lines 458 and 464 restate parts of that same golden expectation.

Line 461 is the weaker part. The bare strings.Contains(got, "fallback") check fails on any future instruction that uses the word for an unrelated reason, and the lowercase "use compozy skill view"` check misses other phrasings such as "invoke" or "run". Consider keeping only the negative assertion that carries the invariant: the catalog must not tell a managed session to run the CLI.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/skills/catalog_test.go` around lines 458 - 465, In
TestBuildCatalogFormatsCatalogSortedEscapedAndWithUsageInstructions, remove the
redundant substring assertions already covered by the exact catalog expectation,
including the broad “fallback” and lowercase “use” checks. Retain a focused
negative assertion in the BuildCatalog validation that rejects guidance telling
managed sessions to invoke, run, or otherwise execute `compozy skill view`.

internal/testutil/acpmock/fixture.go (1)

34-36: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

The managed skill-loading instruction has no single source of truth. The same sentence now exists as catalogManagedLoadingInstructions in internal/skills/catalog.go, as currentSkillsCatalogFinalLine in the ACP mock, and as three inline literals in the mock tests. The mock uses this text to strip prompt augmentation in canonicalUserText, so any drift silently changes turn selection instead of failing loudly.

• internal/testutil/acpmock/fixture.go#L34-L36: export the instruction text from internal/skills and reference it here, or add a test that asserts currentSkillsCatalogFinalLine equals the catalog constant.
• internal/testutil/acpmock/fixture_test.go#L696-L696: replace the inline literal with currentSkillsCatalogFinalLine, matching line 653.
• internal/testutil/acpmock/fixture_test.go#L921-L921: replace the inline literal with currentSkillsCatalogFinalLine.
• internal/testutil/acpmock/fixture_test.go#L946-L946: replace the inline literal with currentSkillsCatalogFinalLine.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/testutil/acpmock/fixture.go` around lines 34 - 36, The managed
skill-loading instruction is duplicated, allowing drift between production and
mock behavior. In internal/testutil/acpmock/fixture.go lines 34-36, reuse an
exported instruction constant from internal/skills/catalog.go (or otherwise
enforce equality with it); in internal/testutil/acpmock/fixture_test.go lines
696, 921, and 946, replace each inline literal with
currentSkillsCatalogFinalLine, matching the existing usage around line 653.

Fix all unresolved CodeRabbit comments on this PR:

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