feat: daemon web UI - #122

packages/ui/src/tokens.css-2-2 (1)

2-2: ⚠️ Potential issue | 🟡 Minor

Fix current Stylelint violations in this file.

These flagged issues are straightforward and will keep lint green: unquoted single-word font family, keyword-case warnings in font stack, missing empty line, and text-rendering value case.

Proposed fix

 `@font-face` {
-  font-family: "Nippo";
+  font-family: Nippo;
   src: url("./assets/fonts/Nippo-Variable.woff2") format("woff2");
@@
-  --font-mono: "JetBrains Mono", ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, monospace;
+  --font-mono: "JetBrains Mono", ui-monospace, "SFMono-Regular", "Menlo", "Monaco", "Consolas", monospace;
@@
   body {
     `@apply` bg-background font-sans text-foreground antialiased;
+
     margin: 0;
-    text-rendering: optimizeLegibility;
+    text-rendering: optimizelegibility;
     -webkit-font-smoothing: antialiased;
   }

Also applies to: 93-93, 232-233

🤖 Prompt for AI Agents

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

In `@packages/ui/src/tokens.css` at line 2, Remove unnecessary quotes around
single-word font-family names (change "Nippo" to Nippo) and ensure generic
family keywords in any font stacks are lowercase (e.g., sans-serif). Normalize
text-rendering values to lowercase (e.g., optimizelegibility →
optimizeLegibility in lowercase form) and add a trailing newline / empty line at
the end of the file. Update the font-family declarations and any text-rendering
properties found in this file (search for font-family and text-rendering
occurrences) so they conform to Stylelint keyword-case and quoting rules and
ensure the file ends with an empty line.

packages/ui/src/tokens.css-172-172 (1)

172-172: ⚠️ Potential issue | 🟡 Minor

Remove .stylelintrc.json or fix it for Tailwind at-rules.

The .stylelintrc.json config extends stylelint-config-standard-scss, which enables at-rule-no-unknown by default. This would flag @theme as unknown in IDEs with Stylelint extensions, despite being valid Tailwind syntax. Since Stylelint isn't installed or used in this project's CI pipeline (linting is handled by oxlint/oxfmt), either remove the unused config file or add ignoreAtRules: ["theme"] to suppress the warning for IDE users.

🤖 Prompt for AI Agents

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

In `@packages/ui/src/tokens.css` at line 172, Remove the unused Stylelint config
or update it to ignore Tailwind's `@theme` at-rule: either delete the
.stylelintrc.json file (since linting uses oxlint/oxfmt) or modify the config
that extends stylelint-config-standard-scss to add ignoreAtRules: ["theme"] so
the at-rule-no-unknown rule won't flag `@theme`; look for the .stylelintrc.json
entry and the at-rule-no-unknown configuration to apply the change.

.compozy/tasks/daemon-web-ui/qa/logs/.gitkeep-1-1 (1)

1-1: ⚠️ Potential issue | 🟡 Minor

Remove the redundant .gitkeep file.

This directory already contains 28 tracked files (log files and snapshots), making .gitkeep unnecessary. The .gitkeep convention is only needed for empty directories; since this directory is not empty, the file can be safely deleted.

🤖 Prompt for AI Agents

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

In @.compozy/tasks/daemon-web-ui/qa/logs/.gitkeep at line 1, Remove the
redundant .gitkeep file: delete the .gitkeep entry from the logs directory that
already contains tracked log files and snapshots, leaving the directory as-is
without the placeholder file.

.compozy/tasks/daemon-web-ui/qa/logs/20-run-start-cookiejar.txt-5-5 (1)

5-5: ⚠️ Potential issue | 🟡 Minor

Avoid committing live CSRF token values in QA log artifacts.

Please redact/sanitize the cookie value (or keep this class of logs untracked) to reduce token exposure patterns and snapshot churn.

🤖 Prompt for AI Agents

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

In @.compozy/tasks/daemon-web-ui/qa/logs/20-run-start-cookiejar.txt at line 5,
The QA artifact contains a live CSRF cookie value ("compozy_csrf"); update the
log generation/sanitization step that writes cookiejar artifacts so it redacts
or replaces cookie values for keys like "compozy_csrf" (e.g., replace the value
with "[REDACTED]" or a stable hash) before writing or commit, or stop tracking
these cookiejar artifacts entirely; ensure the change is applied where cookie
serialization occurs so entries matching the cookie name are filtered/sanitized.

pkg/compozy/runs/run.go-824-826 (1)

824-826: ⚠️ Potential issue | 🟡 Minor

Do not silently ignore malformed next_cursor in snapshot frames.

dispatchSnapshot currently drops parse errors and continues, which can mask protocol/data issues and leave cursor state stale.

Proposed fix

 func (s *daemonRunStream) dispatchSnapshot(raw []byte) error {
 	var payload daemonRunSnapshotPayload
 	if err := json.Unmarshal(raw, &payload); err != nil {
 		return fmt.Errorf("decode snapshot frame: %w", err)
 	}
 	snapshot := RemoteRunSnapshot{
 		Status: normalizeStatus(payload.Run.Status),
 	}
-	if cursor, err := parseRemoteCursor(payload.NextCursor); err == nil && cursor.Sequence > 0 {
-		snapshot.NextCursor = &cursor
-	}
+	if strings.TrimSpace(payload.NextCursor) != "" {
+		cursor, err := parseRemoteCursor(payload.NextCursor)
+		if err != nil {
+			return fmt.Errorf("decode snapshot cursor: %w", err)
+		}
+		if cursor.Sequence > 0 {
+			snapshot.NextCursor = &cursor
+		}
+	}
 	return s.sendItem(RemoteRunStreamItem{
 		Snapshot: &snapshot,
 	})
 }

🤖 Prompt for AI Agents

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

In `@pkg/compozy/runs/run.go` around lines 824 - 826, The code in dispatchSnapshot
silently ignores errors from parseRemoteCursor when handling payload.NextCursor;
instead, detect parse errors for payload.NextCursor and propagate them (or at
minimum log and return an error) so malformed next_cursor doesn't get dropped
and stale state isn't used. Modify the dispatchSnapshot logic around the
parseRemoteCursor call to check the error (from
parseRemoteCursor(payload.NextCursor)) and return or bubble up that error (or
wrap it with context) rather than proceeding to set snapshot.NextCursor only on
success; reference parseRemoteCursor, payload.NextCursor, snapshot.NextCursor
and the dispatchSnapshot function when making the change.

internal/api/httpapi/openapi_contract_test.go-195-202 (1)

195-202: ⚠️ Potential issue | 🟡 Minor

Filter path-item keys to real HTTP methods.

This helper currently turns every key under a path item into a route key. If the spec starts using path-level parameters, summary, or servers, the test will invent fake routes and fail for the wrong reason. Restrict this loop to the OpenAPI operation methods only.

🤖 Prompt for AI Agents

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

In `@internal/api/httpapi/openapi_contract_test.go` around lines 195 - 202, The
loop over pathItem currently treats every key (including path-level keys like
"parameters", "summary", "servers") as an operation; update the iteration that
builds keys (where you do keys = append(keys, strings.ToUpper(method)+" "+path))
to only accept real OpenAPI operation names (e.g. get, put, post, delete,
options, head, patch, trace). Implement this by normalizing method to lower-case
and checking it against a small set/lookup of allowed methods before appending
the route key (use the existing variables paths, pathItem and the method loop to
locate and change the logic).

internal/api/httpapi/browser_middleware.go-297-303 (1)

297-303: ⚠️ Potential issue | 🟡 Minor

Use type assertion instead of error string matching for net.SplitHostPort.

Branching on strings.Contains(err.Error(), "missing port in address") is brittle—error messages are not documented as stable API and can change across Go versions. Instead, use a type assertion to *net.AddrError to check the error condition reliably:

Suggested approach

host, port, err := net.SplitHostPort(trimmed)
if err == nil {
	return normalizeAuthorityHost(host), strings.TrimSpace(port)
}
var addrErr *net.AddrError
if errors.As(err, &addrErr) && strings.Contains(addrErr.Err, "missing port") {
	return normalizeAuthorityHost(trimmed), ""
}
return "", ""

Alternatively, treat any unparseable input as host-only authority for more semantic robustness.

🤖 Prompt for AI Agents

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

In `@internal/api/httpapi/browser_middleware.go` around lines 297 - 303, Replace
the brittle error string matching after net.SplitHostPort by checking the
concrete error type: use errors.As to try to cast err to *net.AddrError (e.g.,
var addrErr *net.AddrError; if errors.As(err, &addrErr) &&
strings.Contains(addrErr.Err, "missing port") { return
normalizeAuthorityHost(trimmed), "" }) and otherwise return the empty failure
case; reference the net.SplitHostPort call and normalizeAuthorityHost to locate
the change and ensure you import "errors" if not already present.

packages/ui/src/components/section-heading.tsx (1)

5-34: Consider making heading level configurable for semantic flexibility.

At Line 32, always rendering h1 can produce invalid heading hierarchy in nested views. Adding an as prop improves reuse without changing default behavior.

♻️ Suggested refactor

 export interface SectionHeadingProps extends Omit<HTMLAttributes<HTMLElement>, "title"> {
   actions?: ReactNode;
+  as?: "h1" | "h2" | "h3" | "h4";
   description?: ReactNode;
   eyebrow?: ReactNode;
   title: ReactNode;
 }

 export function SectionHeading({
   actions,
+  as = "h1",
   className,
   description,
   eyebrow,
   title,
   ...props
 }: SectionHeadingProps): ReactElement {
+  const HeadingTag = as;
   return (
     <header
       className={cn("flex flex-col gap-5 md:flex-row md:items-end md:justify-between", className)}
       {...props}
     >
       <div className="space-y-2">
         {eyebrow ? (
           <p className="font-disket text-[10px] uppercase tracking-[0.2em] text-muted-foreground">
             {eyebrow}
           </p>
         ) : null}
         <div className="space-y-2">
-          <h1 className="font-display text-[clamp(2rem,4vw,3rem)] leading-none tracking-[-0.03em] text-foreground">
+          <HeadingTag className="font-display text-[clamp(2rem,4vw,3rem)] leading-none tracking-[-0.03em] text-foreground">
             {title}
-          </h1>
+          </HeadingTag>
           {description ? (
             <p className="max-w-3xl text-sm leading-7 text-muted-foreground">{description}</p>
           ) : null}
         </div>
       </div>

🤖 Prompt for AI Agents

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

In `@packages/ui/src/components/section-heading.tsx` around lines 5 - 34, The
component always renders an h1 which breaks heading hierarchy; update
SectionHeadingProps to accept an optional as prop (e.g., as?: keyof
JSX.IntrinsicElements or a React element type) with default 'h1', then render
the title using that tag instead of the hard-coded <h1> inside the
SectionHeading function (keep the prop name title and existing behavior when as
is not provided); ensure types for SectionHeadingProps and the render path
(SectionHeading) are updated so className and other attributes apply correctly
to the chosen heading element.

packages/ui/src/components/button.tsx (1)

35-57: Enforce accessible naming for icon-only buttons.

At Lines 51-56, icon-only usage is currently possible without an accessible label. Tightening props prevents silent a11y regressions.

♿ Suggested type-safe API guard

-export interface ButtonProps
-  extends ButtonHTMLAttributes<HTMLButtonElement>, VariantProps<typeof buttonVariants> {
-  icon?: ReactNode;
-}
+type ButtonBaseProps = ButtonHTMLAttributes<HTMLButtonElement> &
+  VariantProps<typeof buttonVariants>;
+
+type ButtonWithTextProps = {
+  children: ReactNode;
+  icon?: ReactNode;
+};
+
+type IconOnlyButtonProps = {
+  children?: never;
+  icon: ReactNode;
+  "aria-label": string;
+};
+
+export type ButtonProps = ButtonBaseProps & (ButtonWithTextProps | IconOnlyButtonProps);

🤖 Prompt for AI Agents

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

In `@packages/ui/src/components/button.tsx` around lines 35 - 57, Button allows
icon-only buttons without an accessible name; update the ButtonProps/type so any
button that provides icon but no children must require an accessible label
(e.g., 'aria-label') at the type level. Concretely, refactor the exported
ButtonProps used by the Button component into a discriminated union: one branch
for regular buttons (children: ReactNode, icon optional) and another for
icon-only buttons (icon: ReactNode and no children) that requires 'aria-label':
string (or equivalent aria-labelledby) so TypeScript enforces an accessible name
when children is absent; update the Button function signature to use this new
ButtonProps shape and ensure the prop references (icon, children) remain the
same.

packages/ui/tests/package-exports.test.ts (1)

40-42: Consider tightening the leak-detection regex.

Current substring matching can produce false positives for legitimate names. Word boundaries (or a curated denylist) will make this test less brittle.

Optional refinement

-    expect(exportNames.some(name => /(route|workflow|task|review|run|page)/i.test(name))).toBe(
+    expect(exportNames.some(name => /\b(route|workflow|task|review|run|page)\b/i.test(name))).toBe(
       false
     );

🤖 Prompt for AI Agents

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

In `@packages/ui/tests/package-exports.test.ts` around lines 40 - 42, The export
leak test uses a loose regex on exportNames which causes false positives;
tighten it by replacing the current /(route|workflow|task|review|run|page)/i
with a word-boundary or explicit denylist pattern (e.g.,
/\b(?:route|workflow|task|review|run|page)\b/i or a curated array of exact names
checked against exportNames) in the test that asserts exportNames.some(...).
Update the test handling around exportNames to use the new regex or denylist so
only whole-word matches (or exact denylisted names) trigger failure.

scripts/codegen.mjs (1)

70-78: Keep --check mode read-only.

--check currently rewrites web/src/generated/compozy-openapi.d.ts before failing. Consider making check mode non-mutating and leaving writes to normal mode only.

Suggested adjustment

   if (next !== current) {
-    writeFileSync(target, next);
     console.error(
       [
         "codegen-check: web/src/generated/compozy-openapi.d.ts is out of date.",
-        "The file was regenerated — re-run the build and commit the change.",
+        "Run codegen and commit the updated file.",
       ].join("\n")
     );
     process.exit(1);
   }

🤖 Prompt for AI Agents

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

In `@scripts/codegen.mjs` around lines 70 - 78, The code currently writes to
target when next !== current even in check mode; change the logic in
scripts/codegen.mjs so that the writeFileSync(target, next) call only runs when
not in read-only check mode (the flag parsed from process.argv like '--check');
when in check mode and next !== current, skip writing, emit the existing error
message about the generated file being out of date, and exit with non-zero
status. Update the branch that compares next and current (variables next,
current, target) to conditionally perform the write based on the check-mode
flag.

internal/daemon/run_snapshot_test.go (1)

14-232: Consider breaking helper assertions into subtests for clearer test output.

The test covers the snapshot builder lifecycle comprehensively, but the inline helper function tests at lines 205-231 (cloneRunJobSummary, tokenUsageRowToKinds, tokenUsageIndex, shutdownStateFromPayload) would benefit from t.Run("Should...") subtests per coding guidelines. This improves failure isolation and test output readability.

♻️ Suggested refactor for helper tests

+	t.Run("Should clone job summary with code files", func(t *testing.T) {
+		if summary := cloneRunJobSummary(
+			apicore.RunJobSummary{CodeFiles: []string{"a.go", "b.go"}},
+		); len(summary.CodeFiles) != 2 {
+			t.Fatalf("cloneRunJobSummary() = %#v, want copied code files", summary)
+		}
+	})
+
+	t.Run("Should convert token usage row to kinds", func(t *testing.T) {
+		if usage := tokenUsageRowToKinds(
+			rundb.TokenUsageRow{InputTokens: 2, OutputTokens: 3, TotalTokens: 5},
+		); usage.TotalTokens != 5 {
+			t.Fatalf("tokenUsageRowToKinds() = %#v, want total 5", usage)
+		}
+	})
+
+	t.Run("Should parse valid session index", func(t *testing.T) {
+		if index, ok := tokenUsageIndex("session-12"); !ok || index != 12 {
+			t.Fatalf("tokenUsageIndex(session-12) = %d, %v; want 12, true", index, ok)
+		}
+	})
+
+	t.Run("Should reject invalid session index", func(t *testing.T) {
+		if index, ok := tokenUsageIndex("bad"); ok || index != 0 {
+			t.Fatalf("tokenUsageIndex(bad) = %d, %v; want 0, false", index, ok)
+		}
+	})

🤖 Prompt for AI Agents

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

In `@internal/daemon/run_snapshot_test.go` around lines 14 - 232, Break the inline
helper assertions at the end of TestRunSnapshotBuilderCoversLifecycleBranches
into separate t.Run subtests so failures are isolated and output is clearer:
create subtests for cloneRunJobSummary, tokenUsageRowToKinds, tokenUsageIndex
(good and bad cases), and shutdownStateFromPayload, moving each respective
assertion into a t.Run with a descriptive name and keep the same checks;
reference the helper functions cloneRunJobSummary, tokenUsageRowToKinds,
tokenUsageIndex, and shutdownStateFromPayload when locating the assertions to
extract.

internal/store/globaldb/read_queries_test.go (1)

14-16: Consider logging or checking db.Close() error in test cleanup.

Per coding guidelines, errors should not be ignored with _ without justification. While test cleanup errors are often tolerable, logging them helps diagnose intermittent test failures.

♻️ Suggested improvement

 	defer func() {
-		_ = db.Close()
+		if err := db.Close(); err != nil {
+			t.Logf("db.Close() error = %v", err)
+		}
 	}()

🤖 Prompt for AI Agents

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

In `@internal/store/globaldb/read_queries_test.go` around lines 14 - 16, The defer
currently ignores the result of db.Close(); wrap the call to check and log any
error instead of using `_`. Replace the defer with a closure like defer func() {
if err := db.Close(); err != nil { t.Logf("error closing db: %v", err) } } (or
use t.Fatalf/t.Error as appropriate) so db.Close() errors are surfaced in the
test; refer to the existing defer func and the db.Close() call to locate where
to change it.

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

56-62: Prefer request structs over positional ID parameters.

Methods like WorkflowMemoryFile(ctx, workspaceRef, workflowSlug, fileID), TaskDetail(ctx, workspaceRef, workflowSlug, taskID), and ReviewDetail(ctx, workspaceRef, workflowSlug, round, issueRef) pass multiple string/primitive identifiers positionally, where a misordered argument silently compiles. Small, dedicated request structs would make these browser-facing reads self-documenting and safer to extend without future signature churn, aligning with the guideline: "Design small, focused interfaces; accept interfaces, return structs."

🤖 Prompt for AI Agents

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

In `@internal/api/core/interfaces.go` around lines 56 - 62, Replace positional
primitive parameters with small request structs for the listed API methods:
WorkflowOverview, ListItems, TaskBoard, WorkflowSpec, WorkflowMemoryIndex,
WorkflowMemoryFile, TaskDetail (and any similar methods like ReviewDetail).
Define request types (e.g., WorkflowMemoryFileRequest { ctx fields:
WorkspaceRef, WorkflowSlug, FileID } and TaskDetailRequest { WorkspaceRef,
WorkflowSlug, TaskID }) and change signatures to accept a single request struct
plus context where appropriate, update callers to construct and pass the
request, and adjust any related interface implementations and tests to use the
new request types to make calls self-documenting and extensible.

Fix all unresolved CodeRabbit comments on this PR:

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

internal/daemon/query_helpers_test.go (3)

97-156: Consider splitting into subtests for directory reading, lane titles, and job counts.

This function tests several distinct behaviors: markdown directory walking (lines 114-130), lane/title normalization (lines 132-140), and job count aggregation (lines 142-155). Subtests would clarify what failed and allow parallel execution of independent cases.

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/daemon/query_helpers_test.go` around lines 97 - 156, Split
TestQueryHelperDirectoryAndStatusBranches into separate t.Run subtests that each
follow the "Should..." naming pattern: one subtest for readMarkdownDir directory
behavior (including missing/empty cases and ordering), one for lane/title
normalization testing laneTitle and titleCase, and one for job aggregation
testing summarizeRunJobCounts; move t.Parallel calls into each subtest as needed
so independent cases can run in parallel, keep the existing assertions and error
checks tied to the specific subtest, and ensure each subtest only references the
relevant helpers (readMarkdownDir, laneTitle, titleCase, summarizeRunJobCounts)
to make failures isolated and clearer.

---

234-290: Consider extracting subtests for each error branch.

This test covers four distinct scenarios: missing document read (lines 243-251), nil daemon state (lines 253-259), status error propagation (lines 261-265), and health error propagation (lines 267-274), plus the no-reviews case (lines 276-289). Subtests would improve clarity.

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/daemon/query_helpers_test.go` around lines 234 - 290, Split
TestQueryServiceReadHelpersHandleOptionalAndErrorBranches into explicit subtests
using t.Run for each scenario: one subtest for readWorkflowDocument missing
document (call service.readWorkflowDocument), one for nil daemon state, one for
status error propagation (set service.daemon =
erringDaemonStatusReader{statusErr: ...} and call service.readDaemonState), one
for health error propagation (set service.daemon with healthErr and call
readDaemonState), and one for latestReviewSummary no-reviews (call
service.latestReviewSummary). Keep the shared setup (env, service) in the parent
test and wrap each assertion block in t.Run("Should <describe case>", func(t
*testing.T) { ... }), preserving existing checks and error expectations for
readWorkflowDocument, readDaemonState, and latestReviewSummary.

---

36-95: Consider restructuring with t.Run("Should...") subtests for better isolation and readability.

This test function covers multiple distinct behaviors (three error types and five document title cases) in a flat structure. Breaking these into subtests would improve test isolation and make failures easier to diagnose.

Example structure:

t.Run("Should match DocumentMissingError against ErrDocumentMissing", func(t *testing.T) { ... })
t.Run("Should extract title from task body", func(t *testing.T) { ... })

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/daemon/query_helpers_test.go` around lines 36 - 95, The test
TestQueryHelperErrorsAndDocumentTitles bundles many independent checks; split it
into t.Run subtests (e.g., t.Run("Should match DocumentMissingError against
ErrDocumentMissing", ...), t.Run("Should match StaleDocumentReferenceError
against ErrStaleDocumentReference", ...), t.Run("Should match
ReviewIssueNotFoundError against ErrReviewIssueNotFound", ...), and separate
t.Run cases for each documentTitle assertion like "Should extract title from
task body", "Should default techspec title", "Should default memory title",
"Should default doc title", "Should prefer frontmatter title") and move the
corresponding assertions into each subtest; keep or move t.Parallel() to either
the parent test or individual subtests as appropriate and reference the existing
symbols DocumentMissingError, StaleDocumentReferenceError,
ReviewIssueNotFoundError, documentTitle, and daemonTaskBody so the assertions
remain unchanged.

internal/daemon/transport_mappers.go (1)

436-445: Consider logging metadata marshal failures for debugging.

When json.Marshal fails (line 441-443), the error is silently discarded. While returning nil for optional metadata is reasonable, logging the error would help diagnose unexpected marshaling failures (e.g., unsupported types in metadata).

♻️ Suggested improvement

 func marshalTransportMetadata(metadata map[string]any) json.RawMessage {
 	if len(metadata) == 0 {
 		return nil
 	}
 	body, err := json.Marshal(cloneTransportMetadataMap(metadata))
 	if err != nil {
+		// Metadata should always be JSON-serializable; log unexpected failures
+		slog.Debug("failed to marshal document metadata", "error", err)
 		return nil
 	}
 	return json.RawMessage(body)
 }

🤖 Prompt for AI Agents

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

In `@internal/daemon/transport_mappers.go` around lines 436 - 445, The
marshalTransportMetadata function currently swallows json.Marshal errors and
returns nil; update it to log the marshal error for debugging while still
returning nil for empty/failed metadata. Inside marshalTransportMetadata (which
calls cloneTransportMetadataMap), capture the err from json.Marshal and send it
to the package logger (or std log) with a clear message like "failed to marshal
transport metadata" plus the error and optionally the metadata key summary, then
return nil as before.

internal/api/httpapi/openapi_contract_test.go (1)

274-276: Unnecessary wrapper function.

stringsSplitN is a trivial pass-through to strings.SplitN. Consider calling strings.SplitN directly in getOperation.

♻️ Suggested simplification

 func getOperation(t *testing.T, spec map[string]any, routeKey string) map[string]any {
 	t.Helper()

-	parts := stringsSplitN(routeKey, " ", 2)
+	parts := strings.SplitN(routeKey, " ", 2)
 	if len(parts) != 2 {
 		t.Fatalf("invalid route key %q", routeKey)
 	}
 	method := strings.ToLower(parts[0])
 	path := parts[1]

 	paths := getMap(t, spec, "paths")
 	pathItem := getMap(t, paths, path)
 	return getMap(t, pathItem, method)
 }

-func stringsSplitN(s string, sep string, n int) []string {
-	return strings.SplitN(s, sep, n)
-}

🤖 Prompt for AI Agents

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

In `@internal/api/httpapi/openapi_contract_test.go` around lines 274 - 276, Remove
the trivial wrapper function stringsSplitN and update getOperation to call
strings.SplitN directly instead; locate the stringsSplitN definition and any
references inside getOperation and replace those calls with strings.SplitN(s,
sep, n), then delete the stringsSplitN function entirely to avoid the
unnecessary indirection.

internal/core/extension/runtime_test.go (1)

350-350: No current deadlock risk detected, but consider consolidating HOME isolation to prevent future double-acquisition.

Verification confirms no test function currently calls runtimeConfigForTest(t) multiple times or mixes it with isolateRunScopeHome(t). However, since isolateRunScopeHome(t) acquires the non-reentrant runScopeTestHomeMu (line 368), accidental double calls would deadlock until cleanup. To improve API safety and prevent future mistakes, consider either making HOME isolation idempotent per test (e.g., check if already locked) or consolidating acquisition at one explicit layer so the risk is eliminated by design.

🤖 Prompt for AI Agents

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

In `@internal/core/extension/runtime_test.go` at line 350, The call to
isolateRunScopeHome(t) can deadlock if accidentally acquired twice because it
locks the non-reentrant runScopeTestHomeMu; make HOME isolation idempotent or
consolidate the lock so tests cannot double-acquire it. Update the test helpers
so either (a) isolateRunScopeHome(t) first checks and returns if the test is
already marked isolated (introduce a per-test flag checked/updated inside
isolateRunScopeHome and cleaned via t.Cleanup), or (b) move the
runScopeTestHomeMu acquisition out of isolateRunScopeHome and into
runtimeConfigForTest(t) so only one code path ever grabs the mutex; reference
the functions isolateRunScopeHome, runtimeConfigForTest and the mutex symbol
runScopeTestHomeMu when making the change.

internal/daemon/host_runtime_test.go (1)

20-149: Adopt t.Run("Should...") subtests for these test cases.

These tests are currently single-case top-level functions; the repository test standard requires t.Run("Should...") as the default structure.

As per coding guidelines, **/*_test.go: "Table-driven tests with subtests (t.Run) as the default pattern" 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/daemon/host_runtime_test.go` around lines 20 - 149, Convert each
top-level test body into a single t.Run("Should ...", func(t *testing.T) { ...
}) subtest: wrap the existing contents of
TestPrepareHostRuntimeStartsTransportsAndProbeReady, TestRunRejectsNilContext,
TestDaemonHealthProblemUsesDetailMessage, and
TestBuildHostHandlersWiresStopCallback inside a t.Run call whose name follows
the "Should ..." style; preserve existing t.Parallel() calls by moving them
inside the subtest where appropriate, and keep all referenced symbols and helper
calls (Start, prepareHostRuntime, Run, daemonHealthProblem, buildHostHandlers,
etc.) unchanged so the logic and assertions remain exactly as before.

internal/api/httpapi/dev_proxy_test.go (1)

60-63: The race condition concern is not applicable with Gin v1.12.0, which uses atomic operations (atomic.StoreInt32 and sync/atomic.Value) to make gin.SetMode goroutine-safe. The -race detector will not flag these tests as problematic.

However, the codebase already demonstrates a cleaner pattern in other test files: save the previous mode and restore it in a cleanup function (as seen in handlers_error_paths_test.go and browser_middleware_test.go). Apply this same pattern for consistency:

func TestDevProxyRoutesServeFrontendRequests(t *testing.T) {
	t.Parallel()

	previousMode := gin.Mode()
	gin.SetMode(gin.TestMode)
	t.Cleanup(func() { gin.SetMode(previousMode) })

Also applies to: 143-146, 199-202, 214-217

🤖 Prompt for AI Agents

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

In `@internal/api/httpapi/dev_proxy_test.go` around lines 60 - 63, Save and
restore Gin's mode in the test to match the project's cleanup pattern: in
TestDevProxyRoutesServeFrontendRequests call previousMode := gin.Mode() before
gin.SetMode(gin.TestMode) and register t.Cleanup(func() {
gin.SetMode(previousMode) }) so the mode is restored after the test; apply the
same change (use gin.Mode(), gin.SetMode(...), and t.Cleanup to restore) to the
other test functions in this file that set the mode (the other occurrences
around the later test cases).

Fix all unresolved CodeRabbit comments on this PR:

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

internal/daemon/host_runtime_test.go (1)

22-28: 🛠️ Refactor suggestion | 🟠 Major

Use t.TempDir() instead of manual temp directory management.

The test creates a temporary directory with os.MkdirTemp and manually cleans it up. Per coding guidelines, use t.TempDir() for filesystem isolation.

♻️ Suggested fix

-	homeDir, err := os.MkdirTemp("", "cmp-home-")
-	if err != nil {
-		t.Fatalf("MkdirTemp() error = %v", err)
-	}
-	t.Cleanup(func() {
-		_ = os.RemoveAll(homeDir)
-	})
+	homeDir := t.TempDir()
 	t.Setenv("HOME", homeDir)

🤖 Prompt for AI Agents

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

In `@internal/daemon/host_runtime_test.go` around lines 22 - 28, Replace the
manual temp dir creation and cleanup (homeDir, err := os.MkdirTemp(...) and
t.Cleanup(os.RemoveAll(homeDir))) with the testing helper t.TempDir(): call
t.TempDir() to get the directory and assign it to homeDir (e.g., homeDir :=
t.TempDir()), remove the error check and explicit Cleanup; update any code using
the previous homeDir variable to use the new one.

internal/api/httpapi/openapi_contract_test.go (1)

31-43: ⚠️ Potential issue | 🟠 Major

The browser contract is still masking two browser-scoped routes.

GET /api/reviews/{slug}/rounds/{round} and GET /api/tasks/{slug}/items are registered in internal/api/core/routes.go, and internal/api/httpapi/browser_middleware.go:206-226 treats them as active-workspace browser routes. Excluding/omitting them here means those endpoints can fall out of the OpenAPI browser contract without this test failing.

Also applies to: 49-75, 108-124

🤖 Prompt for AI Agents

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

In `@internal/api/httpapi/openapi_contract_test.go` around lines 31 - 43, The
test's browserRouteExclusions map is incorrectly masking two active
browser-scoped routes; remove the entries "GET
/api/reviews/{slug}/rounds/{round}" and "GET /api/tasks/{slug}/items" from the
browserRouteExclusions variable in openapi_contract_test.go (and likewise from
any other exclusion maps in that file), so the OpenAPI browser contract test
covers those endpoints which are actually registered as browser routes in the
router (see the route names in routes.go and browser middleware that treat them
as active-workspace browser routes).

internal/daemon/host_runtime_test.go (1)

70-96: Consider adding a brief comment explaining the SPA marker assertion.

The test verifies the embedded frontend serves correctly by checking for <div id="app"></div>. A brief comment noting this is the SPA mount point from web/index.html would improve maintainability.

📝 Suggested addition

+		// Verify embedded SPA shell is served at root (mount point from web/index.html)
 		if !bytes.Contains(rootBody, []byte(`<div id="app"></div>`)) {
 			t.Fatalf("GET / body = %q, want SPA shell", rootBody)
 		}

🤖 Prompt for AI Agents

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

In `@internal/daemon/host_runtime_test.go` around lines 70 - 96, Add a brief
inline comment above the SPA assertion that explains why we check for `<div
id="app"></div>` (it's the single-page-app mount point in web/index.html) so
future readers understand the purpose of the bytes.Contains(rootBody,
[]byte(`<div id="app"></div>`)) check in the GET / response validation; update
the test near the rootBody assertion (around the bytes.Contains call) to include
this note.

internal/cli/daemon.go (1)

320-340: Error messages misleadingly reference env var name when validating flag values.

When normalizeDaemonWebDevProxyTarget is called from resolveDaemonWebDevProxyTarget with a flag value, error messages like %s=%q must use http or https still use daemonWebDevProxyEnv constant, confusing users who passed --web-dev-proxy ws://....

Consider passing a source label parameter or restructuring to provide accurate error context.

♻️ Suggested approach

-func normalizeDaemonWebDevProxyTarget(rawValue string) (string, error) {
+func normalizeDaemonWebDevProxyTarget(rawValue string, source string) (string, error) {
 	value := strings.TrimSpace(rawValue)
 	if value == "" {
 		return "", nil
 	}
 	if !strings.Contains(value, "://") {
-		return "", fmt.Errorf("%s=%q must use http or https", daemonWebDevProxyEnv, rawValue)
+		return "", fmt.Errorf("%s=%q must use http or https", source, rawValue)
 	}
 
 	target, err := url.Parse(value)
 	if err != nil {
-		return "", fmt.Errorf("parse %s=%q: %w", daemonWebDevProxyEnv, rawValue, err)
+		return "", fmt.Errorf("parse %s=%q: %w", source, rawValue, err)
 	}
 	if target.Scheme != "http" && target.Scheme != "https" {
-		return "", fmt.Errorf("%s=%q must use http or https", daemonWebDevProxyEnv, rawValue)
+		return "", fmt.Errorf("%s=%q must use http or https", source, rawValue)
 	}
 	if strings.TrimSpace(target.Host) == "" {
-		return "", fmt.Errorf("%s=%q must include a host", daemonWebDevProxyEnv, rawValue)
+		return "", fmt.Errorf("%s=%q must include a host", source, rawValue)
 	}
 	return target.String(), nil
 }

Then update callers:

// In resolveDaemonWebDevProxyTarget:
return normalizeDaemonWebDevProxyTarget(flagValue, "--web-dev-proxy")

// In cliDaemonWebDevProxyFromEnv:
return normalizeDaemonWebDevProxyTarget(rawValue, daemonWebDevProxyEnv)

🤖 Prompt for AI Agents

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

In `@internal/cli/daemon.go` around lines 320 - 340,
normalizeDaemonWebDevProxyTarget currently hardcodes daemonWebDevProxyEnv in its
error messages, which is misleading when called with a flag; change the function
signature to accept a source label (e.g., source string) and use that label in
all formatted error messages instead of daemonWebDevProxyEnv, update callers
(resolveDaemonWebDevProxyTarget -> call
normalizeDaemonWebDevProxyTarget(flagValue, "--web-dev-proxy") and
cliDaemonWebDevProxyFromEnv -> call normalizeDaemonWebDevProxyTarget(rawValue,
daemonWebDevProxyEnv)), and ensure the returned errors and final return still
use target.String() and wrap parse errors the same way.

Makefile (1)

161-162: Reuse $(BINARY_DIR) and $(BINARY_NAME) in dev.

Hard-coding ./bin/compozy duplicates the build config and can drift from go-build.

Proposed fix

 dev: go-build
-	./bin/compozy daemon start --foreground --web-dev-proxy http://127.0.0.1:3000
+	./$(BINARY_DIR)/$(BINARY_NAME) daemon start --foreground --web-dev-proxy http://127.0.0.1:3000

🤖 Prompt for AI Agents

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

In `@Makefile` around lines 161 - 162, The dev target currently hard-codes
./bin/compozy; change it to reuse the Makefile variables by invoking
$(BINARY_DIR)/$(BINARY_NAME) instead of ./bin/compozy so it stays in sync with
go-build; leave the dependency on go-build intact and ensure the command line
flags (--foreground --web-dev-proxy ...) are preserved when replacing the binary
path in the dev recipe.

Fix all unresolved CodeRabbit comments on this PR:

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

internal/api/httpapi/openapi_contract_test.go (1)

163-186: Wrap schema validation assertions in named subtests for consistency.

These schema requirement checks are standalone assertions that should use the t.Run("Should...") pattern for consistency with the rest of this test and per coding guidelines.

♻️ Suggested refactor

-	taskRunSchema := getSchema(t, spec, "TaskRunRequest")
-	if schemaRequires(taskRunSchema, "workspace") {
-		t.Fatal("TaskRunRequest must not require workspace")
-	}
-	reviewRunSchema := getSchema(t, spec, "ReviewRunRequest")
-	if schemaRequires(reviewRunSchema, "workspace") {
-		t.Fatal("ReviewRunRequest must not require workspace")
-	}
-	workflowRefSchema := getSchema(t, spec, "WorkflowRefRequest")
-	if schemaRequires(workflowRefSchema, "workspace") {
-		t.Fatal("WorkflowRefRequest must not require workspace")
-	}
-
-	runSnapshot := getSchema(t, spec, "RunSnapshotPayload")
-	if !schemaRequires(runSnapshot, "run") {
-		t.Fatal("RunSnapshotPayload must require run")
-	}
-	properties := getMap(t, runSnapshot, "properties")
-	if _, ok := properties["snapshot"]; ok {
-		t.Fatal("RunSnapshotPayload must not wrap the payload under snapshot")
-	}
-	if _, ok := properties["jobs"]; !ok {
-		t.Fatal("RunSnapshotPayload must expose jobs")
-	}
+	for _, tc := range []struct {
+		name   string
+		schema string
+		field  string
+		want   bool
+	}{
+		{"Should not require workspace in TaskRunRequest", "TaskRunRequest", "workspace", false},
+		{"Should not require workspace in ReviewRunRequest", "ReviewRunRequest", "workspace", false},
+		{"Should not require workspace in WorkflowRefRequest", "WorkflowRefRequest", "workspace", false},
+		{"Should require run in RunSnapshotPayload", "RunSnapshotPayload", "run", true},
+	} {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			schema := getSchema(t, spec, tc.schema)
+			if got := schemaRequires(schema, tc.field); got != tc.want {
+				t.Fatalf("%s.%s required = %v, want %v", tc.schema, tc.field, got, tc.want)
+			}
+		})
+	}
+
+	t.Run("Should not wrap RunSnapshotPayload under snapshot", func(t *testing.T) {
+		t.Parallel()
+		runSnapshot := getSchema(t, spec, "RunSnapshotPayload")
+		properties := getMap(t, runSnapshot, "properties")
+		if _, ok := properties["snapshot"]; ok {
+			t.Fatal("RunSnapshotPayload must not wrap the payload under snapshot")
+		}
+	})
+
+	t.Run("Should expose jobs in RunSnapshotPayload", func(t *testing.T) {
+		t.Parallel()
+		runSnapshot := getSchema(t, spec, "RunSnapshotPayload")
+		properties := getMap(t, runSnapshot, "properties")
+		if _, ok := properties["jobs"]; !ok {
+			t.Fatal("RunSnapshotPayload must expose jobs")
+		}
+	})

As per coding guidelines: MUST use t.Run("Should...") pattern for ALL test cases and Use t.Parallel() for independent subtests.

🤖 Prompt for AI Agents

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

In `@internal/api/httpapi/openapi_contract_test.go` around lines 163 - 186, Wrap
each standalone schema assertion block into its own named subtest using
t.Run("Should ...", func(t *testing.T) { t.Parallel(); ... }) so the checks for
TaskRunRequest, ReviewRunRequest, WorkflowRefRequest and RunSnapshotPayload (the
calls using getSchema, schemaRequires, getMap and the properties checks) run as
independent subtests; ensure each t.Run has a descriptive name like "Should not
require workspace for TaskRunRequest" or "Should expose jobs for
RunSnapshotPayload" and move the existing assertions inside those closures.

internal/cli/daemon.go (1)

288-291: Wrap propagated env-resolution errors with call-site context.

These return paths pass raw errors upward, which makes troubleshooting less precise once wrapped further up the stack.

♻️ Suggested improvement

 func cliDaemonRunOptionsFromEnv() (daemon.RunOptions, error) {
 	httpPort, err := cliDaemonHTTPPortFromEnv()
 	if err != nil {
-		return daemon.RunOptions{}, err
+		return daemon.RunOptions{}, fmt.Errorf("resolve %s: %w", daemonHTTPPortEnv, err)
 	}
 	webDevProxyTarget, err := cliDaemonWebDevProxyFromEnv()
 	if err != nil {
-		return daemon.RunOptions{}, err
+		return daemon.RunOptions{}, fmt.Errorf("resolve %s: %w", daemonWebDevProxyEnv, err)
 	}
 	return daemon.RunOptions{
 		Version:           version.String(),
 		HTTPPort:          httpPort,
 		WebDevProxyTarget: webDevProxyTarget,
 	}, nil
 }

 func resolveDaemonWebDevProxyTarget(flagValue string) (string, error) {
 	if strings.TrimSpace(flagValue) != "" {
 		return normalizeDaemonWebDevProxyTarget(flagValue, daemonWebDevProxyFlag)
 	}
-	return cliDaemonWebDevProxyFromEnv()
+	target, err := cliDaemonWebDevProxyFromEnv()
+	if err != nil {
+		return "", fmt.Errorf("resolve web dev proxy target: %w", err)
+	}
+	return target, nil
 }

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

Also applies to: 299-304

🤖 Prompt for AI Agents

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

In `@internal/cli/daemon.go` around lines 288 - 291, The env-resolution errors
returned from calls like cliDaemonWebDevProxyFromEnv() (and the similar call
around the 299-304 region) should be wrapped with call-site context before being
returned; replace raw returns like "return daemon.RunOptions{}, err" with
wrapped errors using fmt.Errorf("failed to resolve web dev proxy from env: %w",
err) (or an appropriate contextual message for the other call), so callers get
clear context while preserving the original error via %w.

Fix all unresolved CodeRabbit comments on this PR:

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