feat: add extensability - #80

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

Inline comments:
In `@internal/cli/extension/display.go`:
- Around line 113-116: The code currently returns a generic "extension not
found" after calling findEffectiveExtension; instead, before returning, check
whether discovery recorded a failure for that extension name (the same place
where appendDiscoveryFailureNotes would add diagnostics) and surface that
failure/error first. Concretely, in the block around
findEffectiveExtension(result, name) add logic to consult the discovery failure
data stored on result (or call appendDiscoveryFailureNotes(result, name)) and
return that detailed error/notes when present rather than the generic
fmt.Errorf("extension %q not found", name), so inspect returns the discovery
failure details for malformed/partial installs.

In `@internal/cli/extension/doctor.go`:
- Around line 88-94: The footer "No extension override or drift issues
detected." is added whenever report.Infos is empty even if report.Warnings
contains drift warnings from skillPackDriftWarnings(); update the conditional so
the message is only appended when both report.Infos and report.Warnings are
empty. Specifically, modify the block that currently checks len(report.Infos) ==
0 (after appending overrideInfos via overrideInfos(result.Overrides)) to check
len(report.Infos) == 0 && len(report.Warnings) == 0 so that warnings prevent the
misleading footer.

In `@internal/cli/extension/enablement.go`:
- Around line 57-63: The current code uses findEffectiveExtension(result, name)
which returns the winning entry, causing ext enable/disable to target the
effective winner instead of the specific match the user intended; change the
logic to search the discovered matches in result for the highest-precedence
entry whose Enabled state differs from the requested enable value and pass that
entry to toggleEntry(ctx, env.store, entry, enable). Concretely, replace the
single findEffectiveExtension call with iterating the matches for name (from
result or the function that returns all matches), select the top-precedence
match with entry.Enabled != enable (prefer workspace/user overrides over
bundled), and if found call toggleEntry with that entry; if none found return
the existing "not found/already in desired state" error.

In `@internal/cli/extension/install.go`:
- Around line 299-323: copySymlink currently recreates symlinks verbatim which
allows absolute links or "../" escapes to point outside the install dir; change
copySymlink to refuse or sanitize such links: after reading linkTarget in
copySymlink, detect unsafe targets (filepath.IsAbs(linkTarget) or path elements
with ".." that would escape the extension root when resolved) and either return
an error rejecting the symlink or rewrite it to a safe relative link (or fall
back to copying the resolved file contents) so the installed extension cannot
reference files outside its install directory; ensure any sanitization uses
filepath.Clean/IsAbs and resolves the final target relative to the source tree
before deciding.
- Around line 79-80: When deps.copyDir(sourcePath, installPath) fails you must
clean up the partially-created install tree to avoid corrupt installs: before
calling deps.copyDir, check whether installPath already existed (os.Stat) and
record that; if copy returns an error and installPath did not exist beforehand,
remove the installPath directory (os.RemoveAll) to rollback the partial install,
then return the original fmt.Errorf("copy extension into user scope: %w", err);
implement this logic around the existing deps.copyDir call so rollback only
removes what this operation created.

In `@internal/cli/extension/root.go`:
- Around line 85-104: resolveEnv currently hard-fails by always calling
resolveWorkspaceRoot, which prevents ext commands from running outside a
workspace; make workspace resolution best-effort by attempting
resolveWorkspaceRoot(ctx) and, on a non-fatal "no workspace" case, set
commandEnv.workspaceRoot to an empty string (or nil equivalent) and continue
returning the env, rather than returning an error. Keep resolveEnv's homeDir and
store creation unchanged, and move strict workspace validation into the specific
subcommands that require workspace writes (update those handlers to call
resolveWorkspaceRoot or check commandEnv.workspaceRoot and return a clear error
if missing). Use the function/struct names resolveEnv, resolveWorkspaceRoot, and
commandEnv to locate the changes.

In `@internal/cli/extensions_bootstrap.go`:
- Around line 26-29: The bootstrap call instantiates extensions.Discovery with
only WorkspaceRoot, which drops user scope and enablement state; update the
construction of extensions.Discovery used before calling Discover(ctx) to
include the same fields used by the main CLI discovery flow (e.g., HomeDir and
the EnablementStore/enablement backend from cfg) so that
extensions.Discovery{WorkspaceRoot: cfg.WorkspaceRoot, HomeDir: cfg.HomeDir,
EnablementStore: cfg.EnablementStore} (or similarly named cfg fields) is passed
into Discover; keep the existing error wrapping for Discover(ctx) intact so
bootstrap discovery honors user-enabled declarative providers/skill packs and
stays consistent with `compozy ext ...`.

In `@internal/core/agent/client.go`:
- Around line 509-514: The subprocess is launched with context.Background(),
dropping request-scoped values; change the call in the subprocess.Launch
invocation to use a derived context that preserves values but is detached from
cancellation by using context.WithoutCancel(ctx) (or equivalent) so the launched
process (stored in c.process) inherits tracing/logging/auth from the request
context without being cancelled when the request ends; update the Launch call to
pass the new ctx returned by context.WithoutCancel and remove
context.Background() usage.
- Around line 49-57: Change the Prompt field from []byte to string in the hook
JSON contract structs in internal/core/agent/client.go (replace Prompt []byte
`json:"prompt,omitempty"` with Prompt string `json:"prompt,omitempty"` for the
struct containing
WorkingDir/Model/MCPServers/ExtraEnv/Context/RunID/JobID/RuntimeMgr and any
other similar hook/request structs in the same file), and update all call sites
that set or read Prompt (convert between []byte and string using string(b) or
[]byte(s) as needed) so the JSON marshals to plain text; alternatively implement
MarshalJSON/UnmarshalJSON on those structs if you need custom byte handling, but
prefer the simple type change to preserve human-readable prompt payloads.

In `@internal/core/agent/hooks.go`:
- Around line 33-37: The withHookContextFrom function currently copies RunID,
JobID and RuntimeMgr but omits Context, which can detach the mutated
SessionRequest from the caller's cancellation/deadline; update
withHookContextFrom to copy src.Context into r.Context (i.e., r.Context =
src.Context) so the original context is preserved after pre-hook mutation, and
apply the same change to the other helper noted in the review (the partner
helper that also copies RunID/JobID/RuntimeMgr).

In `@internal/core/agent/registry_overlay.go`:
- Around line 158-171: splitOverlayCommand and parseOverlayArgs currently use
strings.Fields which loses quoting/escape semantics and breaks paths/args with
spaces; replace both with a shell-aware splitter (e.g., shlex/shellwords) that
preserves quoted strings and escapes so an executable like "/Applications/Visual
Studio Code.app/.../code" and its flags remain intact. Update
splitOverlayCommand to call the new shell-aware split function (instead of
strings.Fields) and return the first element as the command and the rest as
args, and change parseOverlayArgs to return the shell-aware split result (or nil
for empty input). Also ensure the same parser is used when reading fixed_args
and probe_args from metadata so overlays with spaced paths or quoted multi-word
args work correctly.

In `@internal/core/extension/audit.go`:
- Around line 181-236: The Close method can return early on ctx cancellation
while the background goroutine spawned to call syncAndCloseFile(file) continues,
leaving closeDone/closeErr set and the file not fully closed; modify Close so
that on ctx.Done() it still waits for the background goroutine to finish (read
from closeDone) before returning, then wrap/return the context error (e.g.,
capture <-done then return fmt.Errorf("close audit logger: %w", ctx.Err()) or
return the file-close error combined with ctx.Err. Update the select at the end
of Close to ensure the goroutine started in Close (which sets l.closeErr and
closes l.closeDone) is always awaited before returning, referencing symbols:
Close, syncAndCloseFile, closeDone, closeErr, file, path, mu.

In `@internal/core/extension/dispatcher_test.go`:
- Around line 295-302: Replace the wall-clock assertion with channel
synchronization: launch DispatchObserver(context.Background(),
HookAgentPostSessionCreate, ...) in a goroutine that closes a done channel when
it returns, then use select to assert done is received before you close/release
the test's releaseSlow channel (or before a reasonable global test timeout);
this verifies DispatchObserver returned without relying on a 50ms sleep.
Reference DispatchObserver and HookAgentPostSessionCreate to locate where to add
the done channel and select logic.

In `@internal/core/extension/dispatcher.go`:
- Around line 188-206: The code computes an effective timeout into the local
variable timeout (falling back to entry.extension.DefaultHookTimeout()) but
still serializes durationMilliseconds(entry.hook.Timeout) into the
executeHookRequestHook, causing extensions to receive the raw manifest value
(often 0); update the construction of executeHookRequestHook to use the computed
timeout variable (e.g. TimeoutMS: durationMilliseconds(timeout)) so the
extension sees the actual effective deadline used for callCtx; locate the code
around executeHookRequest/executeHookRequestHook, effectiveHookName,
durationMilliseconds, and d.nextInvocationID to make this change.

In `@internal/core/extension/enablement.go`:
- Around line 210-243: saveWorkspaceState currently does an unprotected
read-modify-write on the workspace state file which can lose concurrent updates;
update saveWorkspaceState to (1) acquire a file lock on the workspace state path
(or a dedicated lock file) before loading and modifying the record, (2) perform
the load/modify while the lock is held, and (3) write atomically by writing
payload to a temp file in the same directory and renaming it into place
(ensuring MkdirAll runs before creating the temp file), then release the lock;
reference the saveWorkspaceState, loadWorkspaceEnablementRecord,
workspaceStatePath symbols when implementing the lock+atomic-write flow and
ensure errors release the lock.

In `@internal/core/extension/host_helpers.go`:
- Around line 528-555: The boundary check in resolveScopedPath currently uses
filepath.Clean and pathWithinRoot (and pathWithinRoot itself) which are purely
lexical and can be bypassed via symlinks; update resolveScopedPath to resolve
symlinks before containment checks by calling filepath.EvalSymlinks (or migrate
to the os.Root API if you prefer platform-safe root confinement), apply
EvalSymlinks to both the candidate path and the roots (o.workspaceRoot and
model.CompozyDir(o.workspaceRoot)), then use pathWithinRoot against the
evaluated real paths; also update pathWithinRoot to operate on evaluated
(symlink-resolved) paths so NewPathOutOfScopeError is only raised when the real
filesystem location is outside the allowed roots.
- Around line 363-383: The decoder.More() check in decodeHostParams does not
detect arbitrary trailing top-level JSON; replace that check by attempting a
second decoder.Decode(&extra json.RawMessage) and treat io.EOF as the clean
end-of-input, otherwise return subprocess.NewInvalidParams with method and the
decode error (or a custom "unexpected trailing data" message); ensure you
declare the extra variable (e.g., var extra json.RawMessage) and import io so
you can compare the error to io.EOF while keeping the existing error payload
structure used in decodeHostParams.

In `@Makefile`:
- Around line 84-86: The publish-extension-sdks Makefile target currently runs
npm publish directly; change its rule to depend on the verify and
build-extension-sdks targets so publishing is gated by verification and build
steps — update the publish-extension-sdks target to list verify and
build-extension-sdks as prerequisites and keep the existing npm publish commands
under that target (target name: publish-extension-sdks; dependency names:
verify, build-extension-sdks).
- Around line 85-86: The Makefile publish commands for the scoped packages do
not set public access; update the two npm publish invocations in the Makefile
that reference `@compozy/extension-sdk` and `@compozy/create-extension` to include
the --access public flag so scoped packages are published as public (i.e.,
modify the npm publish --workspace `@compozy/extension-sdk` and npm publish
--workspace `@compozy/create-extension` lines to add --access public).

---

Nitpick comments:
In `@internal/cli/run.go`:
- Around line 120-124: The runPrepared function uses a variadic parameter assets
...declarativeAssets although only 0 or 1 asset is ever expected; change the
signature to accept a single pointer *declarativeAssets (e.g., func (s
*commandState) runPrepared(ctx context.Context, cmd *cobra.Command, cfg
core.Config, assets *declarativeAssets)), update the function body to treat
assets as possibly nil instead of indexing the slice, and update all callers of
runPrepared to pass either nil or &asset accordingly; ensure any code that
relied on assets length/iteration is adjusted to handle a single optional value.

In `@internal/core/agent/session_helpers_test.go`:
- Around line 575-577: The test in session_helpers_test.go is asserting behavior
of subprocess.NormalizeWaitError (an API from the subprocess package) which
couples core tests to external-package implementation; move the assertion into a
new or existing test in the internal/core/subprocess package tests and remove it
from session_helpers_test.go. Specifically, create/update a test that exercises
subprocess.NormalizeWaitError with the same inputs (e.g., call
NormalizeWaitError("wait for ACP agent process", nil) and assert nil) and keep
session_helpers_test.go focused on only core/session helpers rather than
subprocess internals.

In `@internal/core/extension/assets_test.go`:
- Line 8: Add t.Parallel() at the start of each independent test function (e.g.,
TestExtractDeclaredProvidersGroupsByCategory and the other test around the
51-range) so they run concurrently; locate the test functions by their names and
insert a single call to t.Parallel() immediately after the function opening
brace to mark them as parallelizable.

In `@internal/core/extension/assets.go`:
- Around line 134-166: The resolveSkillPattern function currently swallows glob
errors; update it to log debug-level details whenever filepath.Glob or fs.Glob
returns an error (include the original pattern and the returned err) so failures
are observable: inside DiscoveredExtension.resolveSkillPattern, where you call
filepath.Glob(filepath.Join(e.diskRoot, filepath.FromSlash(trimmed))) and
fs.Glob(e.rootFS, path.Join(e.fsBase, trimmed)), add debug logging that records
the pattern, the source (disk vs embedded), and the error (e.g., using the
package or extension logger available in this package or on the
DiscoveredExtension struct) before returning nil.

In `@internal/core/extension/chain.go`:
- Around line 164-181: The WantsEvent method currently treats an empty
e.eventKinds as "match all" which can be surprising; add a clear godoc comment
on RuntimeExtension.WantsEvent that states an empty eventKinds slice means the
extension subscribes to all event kinds (opt-out filtering), and update any
related comments/tests (e.g., unit test for WantsEvent) to reflect and assert
this behavior so future readers know the empty-slice semantics; reference the
RuntimeExtension type and WantsEvent method when adding the documentation.

In `@internal/core/extension/discovery_test.go`:
- Around line 13-31: This test
(TestDiscoveryReturnsEmptyWhenNoExtensionsInstalled) is independent and should
run in parallel; add a call to t.Parallel() near the start of
TestDiscoveryReturnsEmptyWhenNoExtensionsInstalled (before shared setup like
withVersion or newTestDiscovery) so the test harness can execute it
concurrently; apply the same change to other independent discovery tests in this
file (tests that create their own temp dirs and don't share global state) such
as other TestDiscovery* functions.

In `@internal/core/extension/enablement_test.go`:
- Around line 10-62: Add t.Parallel() as the first statement in each top-level
test function (TestEnablementStoreDefaults,
TestEnablementStorePersistsRoundTrip,
TestEnablementStoreRejectsBundledMutations,
TestNewEnablementStoreResolvesHomeDir,
TestEnablementStoreRejectsInvalidReferences,
TestEnablementStoreRejectsCorruptState) to allow safe concurrent execution; for
table-driven tests like TestEnablementStoreDefaults also call t.Parallel() at
the start of each subtest closure inside t.Run so individual cases run in
parallel while still using t.TempDir() isolation.

In `@internal/core/extension/host_api.go`:
- Around line 86-88: The nil-context guard currently substitutes
context.Background() for a nil ctx; instead, change this to return an error when
ctx is nil to avoid silently using Background in production. Locate the nil
check around the ctx variable in host_api.go (the block starting with "if ctx ==
nil") and replace it with an explicit error return (e.g., return nil,
fmt.Errorf(...) or appropriate typed error) from the enclosing function, and
update any callers to ensure they pass a non-nil context; do not introduce
context.Background() except in main or tests.

In `@internal/core/extension/host_reads_test.go`:
- Around line 163-166: Replace the magic number 180 in the test loop that builds
lines (the builder loop in host_reads_test.go) with the production compaction
threshold constant instead of a literal; reference the actual constant used by
the compaction logic (e.g., CompactionThreshold or the appropriately named
constant from the package that implements compaction) by importing that package
and using that symbol, or if the constant is unexported, add an exported alias
in the production package and use it here so the test stays in sync with
production behavior.

In `@internal/core/extension/host_writes.go`:
- Around line 397-427: The atomic write in writeHostFileAtomically should call
Sync to ensure durability: after tmpFile.Write(content) succeed, call
tmpFile.Sync() (handle and return any error) before tmpFile.Close(), and after
successful os.Rename(tmpPath, path) optionally open the parent dir (using dir :=
filepath.Dir(path)), call dirFile.Sync() and close it to ensure the rename is
durable; update error handling to cleanup tmpPath and propagate fsync errors
from tmpFile.Sync() and dirFile.Sync() as appropriate.

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

Inline comments:
In `@internal/cli/makefile_publish_test.go`:
- Around line 11-25: Refactor
TestPublishExtensionSDKsTargetRequiresVerificationAndPublicAccess into
table-driven subtests: keep the initial check for the publish target dependency
in its own t.Run("Should require verify and build-extension-sdks") (call
readRepoMakefile once) and then convert the loop over the expected npm publish
lines into a table of test cases where each case runs as
t.Run(fmt.Sprintf("Should contain %q", want), func(t *testing.T) { t.Parallel();
assert the strings.Contains(makefile, want) }), ensuring each subtest references
the same makefile variable and uses t.Parallel() for independent execution.

In `@internal/core/agent/client.go`:
- Around line 683-697: The current logic in the goroutine using processRef()
treats a nil return from process.Wait() as a clean exit before checking
process.Forced(), which misclassifies forced kills because subprocess/process.go
clears waitErr on forced termination; change the order so you call
process.Forced() first (after obtaining process := c.processRef() and ensuring
it's non-nil) and call c.failOpenSessions(context.Canceled) if Forced() is true,
then handle the err == nil case to call c.failOpenSessions(errors.New("ACP agent
process exited before all sessions completed")), otherwise call
c.failOpenSessions(err); keep the use of process.Wait(), process.Forced(), and
c.failOpenSessions exactly as named.

In `@internal/core/agent/registry_overlay.go`:
- Around line 259-277: The bug is that handleEscapeStart marks every '\' (unless
inSingleQuote) as starting an escape, which causes backslashes inside
double-quoted strings to be consumed; update handleEscapeStart so that if
p.inSingleQuote you WriteRune('\\') as before, but if p.inDoubleQuote you also
WriteRune('\\') and return true (do not set p.escaped); only set p.escaped =
true for unquoted input (or other contexts where escaping is intended). Keep
handleEscapedRune unchanged (it should still consume the next rune when
p.escaped is true). Reference: function handleEscapeStart, function
handleEscapedRune, fields p.inSingleQuote, p.inDoubleQuote, p.escaped, and
p.current.WriteRune.

In `@internal/core/extension/dispatcher_test.go`:
- Around line 388-390: The test currently only checks err != nil for
applyHookPatch and should assert the specific failure reason; update the two
occurrences that call applyHookPatch (the one around
applyHookPatch(map[string]any{"prompt_text": "base"}, json.RawMessage(`"bad"`)
and the other at lines ~430-433) to use a specific error assertion such as
require.ErrorContains/require.ErrorAs (or t.ErrorContains/t.ErrorAs) and match a
stable substring or concrete error type explaining why the patch is invalid
(e.g., "invalid JSON type" or the specific error returned by applyHookPatch) so
the negative path is pinned to the intended contract.
- Around line 87-92: The test blocks indefinitely if DispatchMutable() fails to
call handlers; change the two blind channel receives on firstSeen and secondSeen
to receive with a timeout (e.g., using select and time.After or a context
deadline) so the test fails fast; specifically modify the receives that
currently read from firstSeen and secondSeen (after DispatchMutable and handler
setup) to select between the channel and a timeout case that calls t.Fatalf with
a clear message indicating which handler did not run.

---

Outside diff comments:
In `@internal/core/agent/client.go`:
- Around line 590-615: The code marks c.started = true before the ACP handshake
finishes, letting concurrent CreateSession/ResumeSession callers use c.conn
while Initialize is still in-flight; add a start-in-progress barrier (e.g. a
boolean c.starting plus a sync.Cond or a channel) around the startup sequence in
the client: set c.starting = true before Launch/conn.Initialize, and only set
c.started = true (and clear c.starting and broadcast/signal) after
conn.Initialize returns successfully; have CreateSession/ResumeSession wait on
that condition while c.starting is true and return the initialization error if
Initialize fails (ensure Close()/waitForProcess clears/signals the condition on
error) so no RPCs run against a partially-initialized or failed c.conn.

---

Duplicate comments:
In `@internal/cli/extension/root.go`:
- Around line 85-105: resolveEnv currently always calls resolveWorkspaceRoot and
bubbles up its error, which causes all ext commands to fail outside a workspace;
change resolveEnv to treat the specific "no workspace found" error from
resolveWorkspaceRoot as non-fatal by setting workspaceRoot to "" (or nil
equivalent) and continuing, returning a commandEnv with cleaned homeDir and the
enablement store; ensure only subcommands that need workspace writes explicitly
call/validate resolveWorkspaceRoot or check commandEnv.workspaceRoot and return
a clear error when absent. Use the resolveEnv function and resolveWorkspaceRoot
symbol names to locate and implement this best-effort behavior.

---

Nitpick comments:
In `@internal/cli/makefile_publish_test.go`:
- Around line 14-16: The test currently checks for an exact substring match on
the Makefile line using makefile and the literal "publish-extension-sdks: verify
build-extension-sdks", which is brittle; update the test to locate the
"publish-extension-sdks:" target line from the makefile string (e.g., via
strings.Split or a regex), extract and split the prerequisites on whitespace,
trim them, and assert that the resulting slice contains both "verify" and
"build-extension-sdks" (order- and spacing-independent) instead of relying on
exact sequence matching.

In `@internal/core/agent/client_test.go`:
- Around line 1486-1499: DispatchMutableHook currently returns the Go value
directly, skipping the JSON marshal/unmarshal path used in production and
allowing serialization regressions (e.g., custom MarshalJSON/UnmarshalJSON or
json:"-" fields) to go unnoticed; change DispatchMutableHook to perform a JSON
round-trip of the input (marshal to bytes then unmarshal back into the same
concrete type) before returning or before calling the mutator so tests exercise
real serialization for types like SessionRequest/ResumeSessionRequest, and
ensure you propagate marshal/unmarshal errors and preserve the existing behavior
when mutate == nil.