Repository navigation
fix: untested cases from qa - #276
Conversation
|
Too many files changed for review. ( |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
WalkthroughThis PR updates API contracts, Loop lifecycle persistence and reactivation, daemon shutdown and session coordination, CLI rendering and redaction, settings reconciliation, heartbeat snapshots, and web window-scoped live-data behavior. ChangesAPI contracts and HTTP behavior
CLI output and redaction
Loop lifecycle and daemon integration
Daemon shutdown and session lifecycle
Settings, storage, and web behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WindowManagerRuntime
participant WindowManagerAPI
participant SnapshotCache
participant BrowserWindow
WindowManagerRuntime->>WindowManagerAPI: open or focus command
WindowManagerAPI-->>WindowManagerRuntime: revision conflict
WindowManagerRuntime->>WindowManagerAPI: fetch refreshed snapshot
WindowManagerAPI-->>SnapshotCache: reconcile snapshot
WindowManagerRuntime->>BrowserWindow: retry window.open
WindowManagerRuntime->>BrowserWindow: window.focus
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/daemon/loop_api_runs.go (2)
77-94: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the required Cross-Surface Impact Audit.
This changes Loop API behavior, but the supplied PR context contains no audit naming the checked
web/andpackages/sitesurfaces or their impact. As per coding guidelines, “Every feature, bug fix, refactor, public-contract, CLI/API/native-tool/config/docs change, or runtime behavior change must include the Compozy Cross-Surface Impact Audit.”🤖 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/daemon/loop_api_runs.go` around lines 77 - 94, Add the required Compozy Cross-Surface Impact Audit for the Loop API behavior change, explicitly naming the checked web/ and packages/site surfaces and documenting the impact or confirming none. Include the audit with the change around daemonLoopAPIService methods such as PutLoopAnnotations and the corresponding annotation handling.Source: Coding guidelines
77-94: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not gate annotations on asynchronous catalog projection.
CreateLoopreturns before catalog projection completes, while this path now requiresfindLoopRecord. An immediate annotation read/write after a successful create can therefore returnErrDefinitionNotFound. Resolve existence from durable definition state or add a create-to-annotations path that remains valid before projection.🤖 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/daemon/loop_api_runs.go` around lines 77 - 94, Update the loop annotation read/write paths in daemonLoopAPIService, including PutLoopAnnotations and the preceding annotation retrieval method, so they do not require findLoopRecord’s asynchronous catalog projection to confirm existence. Resolve the loop from durable definition state or use an equivalent create-safe path, while preserving the existing annotation persistence and response behavior.web/src/systems/settings/hooks/settings-skills-draft-logic.ts (1)
105-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMerge each save back into the existing Skills draft instead of replacing
baselinewholesale.
handleSaveDisabledandhandleSavePolicypass fullSkillsConfigsnapshots, so if both saves complete concurrently, the later success can restore stale values for the field it does not own (policy fields from a disabled save, ordisabled_skillsfrom a policy save). Only replace the field owned byevent.kindbefore clearing itspendingentry.🤖 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 `@web/src/systems/settings/hooks/settings-skills-draft-logic.ts` around lines 105 - 116, Update the saveSucceeded handler to merge the successful save into the existing baseline rather than replacing context.baseline with event.baseline. Preserve fields owned by the other save kind, and replace only the relevant disabled_skills or policy field based on event.kind before clearing that kind’s pending entry.
🧹 Nitpick comments (13)
internal/store/globaldb/global_db_heartbeat_test.go (1)
186-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood rollback coverage; the new error branches remain untested.
This subtest only exercises the happy revision path. The three new failure branches in
GetLatestValidHeartbeatSnapshot(emptyNewSnapshotID, revision referencing a missing snapshot →ErrInvalidRevision, latest snapshot withvalid:false→ErrInvalidSnapshot) have no assertions, so a regression there would go unnoticed. Consider extending this same suite with a subtest per branch usingheartbeatSnapshotForTestwith an invalidResolvedJSON.🤖 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/store/globaldb/global_db_heartbeat_test.go` around lines 186 - 263, Extend the heartbeat test suite with separate subtests covering the three failure branches in GetLatestValidHeartbeatSnapshot: a revision with an empty NewSnapshotID, a revision referencing a missing snapshot expecting ErrInvalidRevision, and a latest snapshot whose heartbeatSnapshotForTest data has invalid ResolvedJSON expecting ErrInvalidSnapshot. Assert each returned error explicitly and reuse the existing test setup helpers.Source: Path instructions
internal/settings/config_apply_service_test.go (1)
1790-1794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFake applier now clears catalog pricing when config omits it.
The cost fields are copied unconditionally, so a curated row without pricing wipes the catalog's own values. That diverges from the real reconciliation path (
mergeProviderModelConfigDeltasonly writes deltas) and lets the projection assertions in the five-rate subtests pass for the wrong reason. Consider only overriding when the config actually specifies a value.♻️ Suggested fidelity tweak
- models[index].CostInputPerMillion = cloneFloat64Ptr(curated.CostInputPerMillion) - models[index].CostOutputPerMillion = cloneFloat64Ptr(curated.CostOutputPerMillion) - models[index].CostCacheReadPerMillion = cloneFloat64Ptr(curated.CostCacheReadPerMillion) - models[index].CostCacheWritePerMillion = cloneFloat64Ptr(curated.CostCacheWritePerMillion) - models[index].CostReasoningPerMillion = cloneFloat64Ptr(curated.CostReasoningPerMillion) + if curated.CostInputPerMillion != nil { + models[index].CostInputPerMillion = cloneFloat64Ptr(curated.CostInputPerMillion) + } + if curated.CostOutputPerMillion != nil { + models[index].CostOutputPerMillion = cloneFloat64Ptr(curated.CostOutputPerMillion) + } + if curated.CostCacheReadPerMillion != nil { + models[index].CostCacheReadPerMillion = cloneFloat64Ptr(curated.CostCacheReadPerMillion) + } + if curated.CostCacheWritePerMillion != nil { + models[index].CostCacheWritePerMillion = cloneFloat64Ptr(curated.CostCacheWritePerMillion) + } + if curated.CostReasoningPerMillion != nil { + models[index].CostReasoningPerMillion = cloneFloat64Ptr(curated.CostReasoningPerMillion) + }🤖 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/settings/config_apply_service_test.go` around lines 1790 - 1794, The fake applier’s cost-field assignments in the test reconciliation logic should only override catalog pricing when the curated configuration specifies a non-nil value. Update the five assignments for CostInputPerMillion, CostOutputPerMillion, CostCacheReadPerMillion, CostCacheWritePerMillion, and CostReasoningPerMillion to preserve existing model values when the corresponding curated field is omitted, matching mergeProviderModelConfigDeltas behavior.internal/store/globaldb/global_db_heartbeat.go (1)
153-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the revision fast path into a helper.
The
len(revisions) > 0block is ~40 lines of nested error construction inside an already long function; pulling it intocurrentSnapshotForRevision(ctx, revisions[0])would flatten the flow and keep this file's per-function responsibility tight.🤖 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/store/globaldb/global_db_heartbeat.go` around lines 153 - 195, Extract the entire len(revisions) > 0 handling block into a helper named currentSnapshotForRevision, accepting ctx and revisions[0] (or the revision value) and returning the snapshot and error. Preserve all existing validation, snapshot lookup, envelope resolution, and error-wrapping behavior; have the caller invoke the helper to flatten the main function.internal/settings/provider_curation_reconcile.go (1)
208-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mergeProviderModelConfigDeltasis a long chain of near-identical blocks.Fifteen structurally identical compare-then-clone blocks make it easy to forget a field when
ProviderModelConfiggrows (and there is no compile-time signal for a missed field). Consider a small table of field accessors/setters, or generic helpers likemergePtr(&next.X, desired.X, current.X)/mergeValue(&next.Y, desired.Y, current.Y), to shrink this and make additions mechanical.🤖 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/settings/provider_curation_reconcile.go` around lines 208 - 267, Refactor mergeProviderModelConfigDeltas to eliminate the repeated compare-then-clone blocks by introducing small reusable merge helpers such as pointer and value variants, or an equivalent field-accessor table. Apply the helpers consistently to all ProviderModelConfig fields while preserving deep-equality checks, nil-preserving slice cloning, and the existing trimmed desired.ID assignment.internal/daemon/daemon_lifecycle.go (1)
54-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
d.daemonShutdownContext(ctx)here too.The observer-retention failure path was migrated to the new dynamic budget, but the memory-extractor failure path still hardcodes
defaultShutdownTimeout— the one path where checkpoint drain time is most likely needed.♻️ Proposed change
if d.memoryExtractor != nil { if err := d.memoryExtractor.Start(runCtx); err != nil { - shutdownCtx, cancel := context.WithTimeout(context.Background(), defaultShutdownTimeout) + shutdownCtx, cancel := d.daemonShutdownContext(ctx) defer cancel()🤖 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/daemon/daemon_lifecycle.go` around lines 54 - 63, The memory-extractor startup failure path should use the dynamic shutdown budget instead of hardcoding defaultShutdownTimeout. In the memoryExtractor.Start error handling within the daemon lifecycle flow, obtain the shutdown context via d.daemonShutdownContext(ctx) before calling d.Shutdown, preserving the existing error joining and cancellation behavior.internal/session/manager_start_stop.go (1)
26-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a
defer releaseLaunchCommit()safety net.Every early return currently releases the token manually, so correctness depends on future edits remembering it; forgetting one strands the starting goroutine on
run.launchCommituntil its context is canceled. SincereleaseLaunchCommitis idempotent (it nilsreleaseCommit), a defer costs nothing and keeps the explicit early release beforewaitForSessionStartRunintact.♻️ Proposed change
releaseLaunchCommit := func() { if releaseCommit == nil { return } releaseCommit() releaseCommit = nil } + defer releaseLaunchCommit()🤖 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/session/manager_start_stop.go` around lines 26 - 49, Add defer releaseLaunchCommit() immediately after defining the idempotent releaseLaunchCommit closure in the session start flow. Keep the existing explicit releaseLaunchCommit() before waitForSessionStartRun unchanged, and rely on the closure’s nil reset to make deferred cleanup safe across all returns.internal/store/globaldb/global_db_loop_control.go (1)
53-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo positional empty strings obscure intent.
g.reserveLoopCoordinatorRunWithExecutor(..., "", "")gives no hint about which parameters are being defaulted. A named local or short inline comment would keep the call site readable.🤖 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/store/globaldb/global_db_loop_control.go` around lines 53 - 61, Clarify the two defaulted arguments in the reserveLoopCoordinatorRunWithExecutor call by introducing named local variables or adding a concise inline comment that identifies what each empty string represents. Keep the call’s behavior and argument order unchanged.internal/daemon/native_loop_tools_test.go (1)
689-712: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded tool-ID list will silently under-cover new Loop tools.
Adding a Loop tool later won't fail this readiness test, it just won't be checked. Deriving the list from the registry projection (filtering by the
loop_/goal_prefix) would keep the invariant self-maintaining.🤖 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/daemon/native_loop_tools_test.go` around lines 689 - 712, The readiness test’s hardcoded loopToolIDs list can omit newly registered Loop tools. Replace that list in the test around registry.OperatorProjection with IDs derived from the registry projection, filtering tool identifiers by the loop_/goal_ prefix, then apply requireNativeToolUnavailableReason to every matching projection entry.internal/daemon/native_loop_tools_support.go (1)
247-268: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider namespacing the conflict partial payload like the other structured Loop errors.
The input-default path (Lines 198-220) wraps its payload under a named envelope key (
nativeLoopValidationKey), while this path emits a bare top-levelcurrent_version. Two shapes for the same structured-result channel makes agent-side parsing harder; a named key such asversion_conflictwould keep the machine-readable contract uniform.Per coding guidelines, user-visible runtime capabilities must provide stable machine-readable control surfaces — worth confirming the documented tool-result contract matches whichever shape you keep.
🤖 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/daemon/native_loop_tools_support.go` around lines 247 - 268, Update nativeLoopVersionConflictToolError so its structured partial result wraps the current_version payload under the established named envelope key, such as version_conflict, matching the namespaced shape used by the input-default validation path. Confirm the documented tool-result contract uses this stable namespaced structure.Source: Coding guidelines
internal/loop/source_delete.go (1)
57-68: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Restorereports failure when only the temp-dir cleanup fails.Once the rename back succeeds the definition is fully restored; failing on
os.Remove(stagingRoot)makes callers (wrapLoopDeleteRestoreErrorininternal/daemon/loop_delete.go) report a restore failure for what is only a leaked empty temp directory. Consider treating cleanup as best-effort, or returning a distinct error so the operator message stays accurate.♻️ Suggested change
if err := os.Rename(d.stagingDir, d.originalDir); err != nil { return fmt.Errorf("loop: restore staged definition %q: %w", d.originalDir, err) } - if err := os.Remove(d.stagingRoot); err != nil { - return fmt.Errorf("loop: remove empty definition deletion stage %q: %w", d.stagingRoot, err) - } + // Best effort: the definition is already restored; a leaked empty stage is not a restore failure. + _ = os.Remove(d.stagingRoot) return nilNote the coding guideline forbids discarding errors with
_; if you take this route, log the cleanup error instead of ignoring it.🤖 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/loop/source_delete.go` around lines 57 - 68, Update StagedWritableDefinitionDeletion.Restore so a successful os.Rename is reported as a successful restoration even when removing stagingRoot fails. Handle the cleanup error without discarding it, using the existing logging mechanism to record the failure while preserving the successful restore result and accurate caller messaging.Source: Coding guidelines
internal/daemon/loop_api_publish.go (1)
61-70: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnsure
PatchLoopmatches the400 → 403shift for read-only loops.
internal/api/core/loops_test.gocoversDeleteLoopreturning403forlooppkg.ErrDefinitionReadOnly, butinternal/daemon/loop_api_service.go:301meansPatchLoopnow also returns this error for read-only sources; confirm the PATCH handler/test path is updated so marketplace loop PATCH is not documented or consumed as400.🤖 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/daemon/loop_api_publish.go` around lines 61 - 70, Update the PatchLoop API handler and its tests to map looppkg.ErrDefinitionReadOnly from HTTP 400 to 403, matching DeleteLoop. Verify marketplace/read-only loop PATCH responses and API documentation or consumers no longer treat this error as 400, while preserving existing behavior for other validation errors.web/src/systems/os/hooks/use-window-live-data-enabled.ts (1)
5-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: local
windowshadows the globalwindowobject.Renaming avoids confusion with the browser global and potential
no-shadowlint findings.✏️ Suggested rename
return useDesktop(state => { - const window = state.windows[windowId]; + const win = state.windows[windowId]; return ( - window !== undefined && + win !== undefined && state.focusedId === windowId && - state.activeDesktopId === window.desktopId && - !window.minimized + state.activeDesktopId === win.desktopId && + !win.minimized ); });🤖 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 `@web/src/systems/os/hooks/use-window-live-data-enabled.ts` around lines 5 - 13, Rename the local window variable in the useDesktop selector to avoid shadowing the browser global window object, and update all references within the selector accordingly. Preserve the existing focus, desktop, and minimized-state checks.web/src/systems/os/hooks/__tests__/window-manager-runtime.test.ts (1)
657-816: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a double-conflict regression test.
The retry in
recoverOpenOrFocus(window-manager-runtime.ts) only attempts recovery once (enabled=falseon retry). These tests cover the single-recovery success and the pre-existing-conflict-blocks-recovery cases, but not the scenario where the retried attempt also lands in conflict — worth a follow-up test asserting the outcome resolvesfalsewithout an unbounded retry loop.🤖 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 `@web/src/systems/os/hooks/__tests__/window-manager-runtime.test.ts` around lines 657 - 816, The runtime tests lack coverage for a conflict occurring during the recovery retry. Add a regression test for recoverOpenOrFocus where the initial open conflicts and the retry also rejects with a revision conflict, then assert completion resolves false and the command execution does not continue retrying indefinitely. Anchor the test to recoverOpenOrFocus behavior and preserve the existing pre-existing-conflict and successful-recovery coverage.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/api/contract/loops.go`:
- Line 191: Update the change introducing the public response-contract field in
the loop response model to include a Cross-Surface Impact Audit. Explicitly name
the checked web/ deserializers and packages/site API documentation surfaces, and
record the impact or no-impact rationale for each based on the new Config field.
In `@internal/api/core/skills_test.go`:
- Around line 942-943: Extend the 404 assertion in the relevant test to decode
rec.Body into contract.ErrorPayload and verify it contains the canonical
workspace-not-found error/code, while retaining the existing http.StatusNotFound
status check. Use the existing contract symbols and test decoding conventions
rather than validating the body as an unstructured string.
In `@internal/api/spec/registry_sessions.go`:
- Line 156: Add the missing HTTP 503 response with contract.ErrorPayload{} to
the AttachSession operation’s response list alongside the existing 400 response,
then update the OpenAPI contract test to assert that this reachable
unavailable-manager response is documented.
In `@internal/cli/client_tools.go`:
- Around line 342-347: Update the invalid-payload handling around
redactToolDiagnostic so it never returns the original raw input when redaction
is still non-JSON. Instead, return a valid JSON string with the payload fully
redacted, and update the invalid-payload expectation in the relevant tool test
to assert the new sanitized result.
In `@internal/daemon/loop_delete.go`:
- Around line 45-47: Update the staged.Commit error handling in the loop
deletion flow to log the staging-directory cleanup failure using the package’s
actual logger accessor, then return success because the definition deletion and
catalog reconciliation already completed. Do not propagate the Commit error or
report deletion failure.
In `@internal/daemon/native_loop_tools_test.go`:
- Around line 305-309: Update the test case table used by the native loop reason
mapping test to include the expected toolspkg.ReasonCode constant as wantReason.
In the assertion around toolErr.ReasonCodes, remove the looppkg-to-toolspkg
conversion and compare directly against tc.wantReason, preserving the existing
schema-invalid exclusion.
In `@internal/memory/consolidation/runtime.go`:
- Around line 220-227: Update the context-error guard in Service.Run’s runCheck
handling to verify ctx is non-nil before calling ctx.Err(), while preserving the
existing errors.Is check and safe error-return behavior for nil contexts.
In `@internal/redact/redact_test.go`:
- Line 302: Update the RedactJSON invocation in the test to remove the empty
string from the named-field list, retaining only redactionMessageFieldKey so the
test isolates message-field-driven redaction. If root-wide heuristic redaction
is needed, cover it in a separate explicitly named test.
- Around line 293-294: Add coverage in the relevant redaction test fixture for
the exact "cursor" key alongside "next_cursor", and assert that its value
remains unchanged. Keep the existing next_cursor assertion and use the same test
structure to exercise the production cursor branch.
In `@internal/session/manager_prompt.go`:
- Around line 114-117: Update the PromptLifecycleContinuation path around
parsePromptRequest to set PromptOpts.TurnSource to TurnSourceSynthetic instead
of TurnSourceUser, ensuring the existing promptSubmissionPathSynthetic handling
classifies daemon-owned continuation turns correctly.
In `@internal/session/manager_start_test.go`:
- Around line 555-567: Guard the close of recorderOpenEntered inside the
WithStore callback with a sync.Once, following the existing
blockingSessionCreationCatalog registerOnce pattern, so repeated store-hook
invocations signal the channel only once while preserving the current release
and cancellation behavior.
In `@internal/settings/provider_curation_reconcile.go`:
- Around line 108-123: Update currentProviderModels to translate
requireProviderModelCatalog’s missing-catalog error into the original
curated-write validation error, while preserving ErrAllSourcesFailed for catalog
source failures. Keep the existing ListModels error handling unchanged and
ensure custom dependencies without a model catalog do not expose an
unavailable/500 error.
In `@internal/store/globaldb/global_db_loop_control.go`:
- Around line 95-98: Align the validation in the TransitionCauseApproval branch
with its diagnostic: require exactly one decision by changing the
len(req.Decisions) condition to reject counts other than one, while preserving
the existing gated Run status check and error wrapping.
In `@packages/site/lib/__tests__/runtime-docs-truth.test.ts`:
- Around line 172-175: Strengthen the assertions in the runtime-docs truth test
so they verify the error-to-status behavior rather than independent token
presence. Scope checks to the corresponding StatusForResourceError branches, or
invoke the mapper directly, and assert that
resources.ErrDirectMutationNotAllowed maps to http.StatusForbidden (403) while
validation failures map to http.StatusUnprocessableEntity (422).
In `@web/src/systems/os/apps/dashboard/__tests__/dashboard-window.test.tsx`:
- Around line 36-39: Remove the duplicate homeDashboardSpy declaration and
duplicate HomeDashboard member in the dashboard test mock. Keep one typed
homeDashboardSpy and one HomeDashboard mapping in the
vi.mock("`@/systems/dashboard`") setup so the file typechecks.
In `@web/src/systems/os/runtime/window-manager-runtime-core.ts`:
- Around line 274-292: Update refreshSnapshot() to pass an AbortSignal to
fetchWindowManagerSnapshot and associate the request with the current binding
lifecycle. Add or reuse an AbortController that is aborted during bind() and
unbind(), create a fresh controller for the new binding, and ignore aborted or
stale responses so neither query data nor setLoadError() is updated for a
different binding. Preserve returning false for genuine refresh failures and
true only when the active binding’s snapshot was applied.
---
Outside diff comments:
In `@internal/daemon/loop_api_runs.go`:
- Around line 77-94: Add the required Compozy Cross-Surface Impact Audit for the
Loop API behavior change, explicitly naming the checked web/ and packages/site
surfaces and documenting the impact or confirming none. Include the audit with
the change around daemonLoopAPIService methods such as PutLoopAnnotations and
the corresponding annotation handling.
- Around line 77-94: Update the loop annotation read/write paths in
daemonLoopAPIService, including PutLoopAnnotations and the preceding annotation
retrieval method, so they do not require findLoopRecord’s asynchronous catalog
projection to confirm existence. Resolve the loop from durable definition state
or use an equivalent create-safe path, while preserving the existing annotation
persistence and response behavior.
In `@web/src/systems/settings/hooks/settings-skills-draft-logic.ts`:
- Around line 105-116: Update the saveSucceeded handler to merge the successful
save into the existing baseline rather than replacing context.baseline with
event.baseline. Preserve fields owned by the other save kind, and replace only
the relevant disabled_skills or policy field based on event.kind before clearing
that kind’s pending entry.
---
Nitpick comments:
In `@internal/daemon/daemon_lifecycle.go`:
- Around line 54-63: The memory-extractor startup failure path should use the
dynamic shutdown budget instead of hardcoding defaultShutdownTimeout. In the
memoryExtractor.Start error handling within the daemon lifecycle flow, obtain
the shutdown context via d.daemonShutdownContext(ctx) before calling d.Shutdown,
preserving the existing error joining and cancellation behavior.
In `@internal/daemon/loop_api_publish.go`:
- Around line 61-70: Update the PatchLoop API handler and its tests to map
looppkg.ErrDefinitionReadOnly from HTTP 400 to 403, matching DeleteLoop. Verify
marketplace/read-only loop PATCH responses and API documentation or consumers no
longer treat this error as 400, while preserving existing behavior for other
validation errors.
In `@internal/daemon/native_loop_tools_support.go`:
- Around line 247-268: Update nativeLoopVersionConflictToolError so its
structured partial result wraps the current_version payload under the
established named envelope key, such as version_conflict, matching the
namespaced shape used by the input-default validation path. Confirm the
documented tool-result contract uses this stable namespaced structure.
In `@internal/daemon/native_loop_tools_test.go`:
- Around line 689-712: The readiness test’s hardcoded loopToolIDs list can omit
newly registered Loop tools. Replace that list in the test around
registry.OperatorProjection with IDs derived from the registry projection,
filtering tool identifiers by the loop_/goal_ prefix, then apply
requireNativeToolUnavailableReason to every matching projection entry.
In `@internal/loop/source_delete.go`:
- Around line 57-68: Update StagedWritableDefinitionDeletion.Restore so a
successful os.Rename is reported as a successful restoration even when removing
stagingRoot fails. Handle the cleanup error without discarding it, using the
existing logging mechanism to record the failure while preserving the successful
restore result and accurate caller messaging.
In `@internal/session/manager_start_stop.go`:
- Around line 26-49: Add defer releaseLaunchCommit() immediately after defining
the idempotent releaseLaunchCommit closure in the session start flow. Keep the
existing explicit releaseLaunchCommit() before waitForSessionStartRun unchanged,
and rely on the closure’s nil reset to make deferred cleanup safe across all
returns.
In `@internal/settings/config_apply_service_test.go`:
- Around line 1790-1794: The fake applier’s cost-field assignments in the test
reconciliation logic should only override catalog pricing when the curated
configuration specifies a non-nil value. Update the five assignments for
CostInputPerMillion, CostOutputPerMillion, CostCacheReadPerMillion,
CostCacheWritePerMillion, and CostReasoningPerMillion to preserve existing model
values when the corresponding curated field is omitted, matching
mergeProviderModelConfigDeltas behavior.
In `@internal/settings/provider_curation_reconcile.go`:
- Around line 208-267: Refactor mergeProviderModelConfigDeltas to eliminate the
repeated compare-then-clone blocks by introducing small reusable merge helpers
such as pointer and value variants, or an equivalent field-accessor table. Apply
the helpers consistently to all ProviderModelConfig fields while preserving
deep-equality checks, nil-preserving slice cloning, and the existing trimmed
desired.ID assignment.
In `@internal/store/globaldb/global_db_heartbeat_test.go`:
- Around line 186-263: Extend the heartbeat test suite with separate subtests
covering the three failure branches in GetLatestValidHeartbeatSnapshot: a
revision with an empty NewSnapshotID, a revision referencing a missing snapshot
expecting ErrInvalidRevision, and a latest snapshot whose
heartbeatSnapshotForTest data has invalid ResolvedJSON expecting
ErrInvalidSnapshot. Assert each returned error explicitly and reuse the existing
test setup helpers.
In `@internal/store/globaldb/global_db_heartbeat.go`:
- Around line 153-195: Extract the entire len(revisions) > 0 handling block into
a helper named currentSnapshotForRevision, accepting ctx and revisions[0] (or
the revision value) and returning the snapshot and error. Preserve all existing
validation, snapshot lookup, envelope resolution, and error-wrapping behavior;
have the caller invoke the helper to flatten the main function.
In `@internal/store/globaldb/global_db_loop_control.go`:
- Around line 53-61: Clarify the two defaulted arguments in the
reserveLoopCoordinatorRunWithExecutor call by introducing named local variables
or adding a concise inline comment that identifies what each empty string
represents. Keep the call’s behavior and argument order unchanged.
In `@web/src/systems/os/hooks/__tests__/window-manager-runtime.test.ts`:
- Around line 657-816: The runtime tests lack coverage for a conflict occurring
during the recovery retry. Add a regression test for recoverOpenOrFocus where
the initial open conflicts and the retry also rejects with a revision conflict,
then assert completion resolves false and the command execution does not
continue retrying indefinitely. Anchor the test to recoverOpenOrFocus behavior
and preserve the existing pre-existing-conflict and successful-recovery
coverage.
In `@web/src/systems/os/hooks/use-window-live-data-enabled.ts`:
- Around line 5-13: Rename the local window variable in the useDesktop selector
to avoid shadowing the browser global window object, and update all references
within the selector accordingly. Preserve the existing focus, desktop, and
minimized-state checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| @@ -188,7 +188,7 @@ type LoopPlanNodePreview struct { | |||
|
|
|||
| // LoopConfigResponse returns the stored override and daemon-resolved runtime config. | |||
| type LoopConfigResponse struct { | |||
| Config *LoopConfig `json:"config,omitempty"` | |||
| Config *LoopConfig `json:"config"` | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the required Cross-Surface Impact Audit.
This public response-contract change can affect web/ deserializers and packages/site API documentation. Add an audit naming the exact checked surfaces and their impact or no-impact rationale. As per coding guidelines, “Every backend task must include a Web/Docs Impact analysis covering affected web/ surfaces and packages/site documentation; claim no impact only after analysis.”
🤖 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/contract/loops.go` at line 191, Update the change introducing
the public response-contract field in the loop response model to include a
Cross-Surface Impact Audit. Explicitly name the checked web/ deserializers and
packages/site API documentation surfaces, and record the impact or no-impact
rationale for each based on the new Config field.
Source: Coding guidelines
| func (s *service) currentProviderModels( | ||
| ctx context.Context, | ||
| providerID string, | ||
| ) ([]string, error) { | ||
| ) ([]modelcatalog.Model, error) { | ||
| if err := s.requireProviderModelCatalog(); err != nil { | ||
| return nil, err | ||
| } | ||
| models, err := s.modelCatalog.ListModels(ctx, modelcatalog.ListOptions{ | ||
| ProviderID: providerID, | ||
| View: modelcatalog.CatalogViewCurated, | ||
| View: modelcatalog.CatalogViewAll, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("settings: list provider models before write: %w", err) | ||
| } | ||
| ids := make([]string, 0, len(models)) | ||
| for _, model := range models { | ||
| ids = append(ids, model.ModelID) | ||
| } | ||
| return ids, nil | ||
| return models, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'func (s *service) requireProviderModelCatalog() error { $$$ }' --lang go internal/settings
rg -nP --type=go -C3 'ModelCatalog:\s' internal | rg -v '_test.go'
rg -nP --type=go -C2 'requireProviderModelCatalog\(' internal/settingsRepository: compozy/compozy
Length of output: 9394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## relevant files"
git ls-files 'internal/settings/*.go' | sort
echo
echo "## outlines"
ast-grep outline internal/settings/provider_curation_reconcile.go --view compact || true
ast-grep outline internal/settings/provider_model_curation.go --view compact || true
echo
echo "## provider_curation_reconcile.go lines 1-180"
cat -n internal/settings/provider_curation_reconcile.go | sed -n '1,180p'
echo
echo "## provider_model_curation.go relevant lines"
cat -n internal/settings/provider_model_curation.go | sed -n '1,150p'
echo
echo "## boot_settings constructors around modelCatalog"
cat -n internal/daemon/boot_settings.go | sed -n '1,90p'
rg -n -C 4 'state\.modelCatalog|modelCatalogDependency|NewModelCatalog|DefaultModelCatalogConfig' internal/daemon internal -g '*.go' | rg -v '_test.go' | head -n 200
echo
echo "## settings service dependency/type fields"
rg -n -C 3 'modelCatalog|ModelCatalog|requireProviderModelCatalog|currentProviderModels' internal/settings internal/daemon internal -g '*.go' | rg -v 'internal/settings/provider_curation_reconcile.go|internal/settings/provider_model_curation.go|_test.go' | head -n 240Repository: compozy/compozy
Length of output: 32710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## boot model catalog and state fields"
rg -n -C 5 'state\.modelCatalog|modelCatalogDependency|modelCatalog:' internal/daemon -g '*.go' | rg -v '_test.go' | head -n 220
echo
echo "## settings service dependency"
cat -n internal/settings/service.go | sed -n '1,220p'
echo
echo "## curated provider mutate/write call sites"
rg -n -C 3 'reconcileProviderCuratedWrite|Curated.*:|modelCuration|ProviderModelCuration' internal/settings internal/api internal -g '*.go' | rg -v 'provider_curation_reconcile.go|provider_model_curation.go|_test.go' | head -n 260
echo
echo "## service error classification helpers"
cat -n internal/settings/errors.go | sed -n '1,220p'Repository: compozy/compozy
Length of output: 34396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## bootState / boot dependency wiring around modelCatalog"
rg -n -C 8 'modelCatalog|ModelCatalog|modelCatalogDependency|newModelCatalog|newModelCatalogDependency|modelCatalog:' internal/daemon -g '*.go' | rg -v '_test.go' | head -n 260
echo
echo "## service error classification helpers"
cat -n internal/settings/errors.go | sed -n '1,260p'
echo
echo "## API/UHTTP error classification for settings"
rg -n -C 4 'settings:|Category.*settings|validationError|settings validation|settings.*validation' internal/api internal/settings -g '*.go' | rg -v '_test.go' | head -n 260Repository: compozy/compozy
Length of output: 20513
Preserve validation semantics for curated provider writes without a model catalog.
Curated row reconciliation now calls currentProviderModels whenever settings.Models.Curated != nil, and requireProviderModelCatalog() returns modelcatalog.ErrAllSourcesFailed. That is intentional for unavailable catalog sources, but keep this path distinct from ListModels. Wrap the missing dependency as the original validation error so custom dependencies without a model catalog don’t surface as unavailable/500.
🤖 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/settings/provider_curation_reconcile.go` around lines 108 - 123,
Update currentProviderModels to translate requireProviderModelCatalog’s
missing-catalog error into the original curated-write validation error, while
preserving ErrAllSourcesFailed for catalog source failures. Keep the existing
ListModels error handling unchanged and ensure custom dependencies without a
model catalog do not expose an unavailable/500 error.
| const homeDashboardSpy = vi.fn((_props: { liveEnabled: boolean }) => null); | ||
|
|
||
| vi.mock("@/systems/dashboard", () => ({ | ||
| HomeDashboard: () => homeDashboardSpy(), | ||
| HomeDashboard: (props: { liveEnabled: boolean }) => homeDashboardSpy(props), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicated mock declarations.
homeDashboardSpy and the HomeDashboard mock member are each declared twice, preventing this test file from typechecking.
Proposed fix
const homeDashboardSpy = vi.fn((_props: { liveEnabled: boolean }) => null);
-const homeDashboardSpy = vi.fn((_props: { liveEnabled: boolean }) => null);
vi.mock("`@/systems/dashboard`", () => ({
HomeDashboard: (props: { liveEnabled: boolean }) => homeDashboardSpy(props),
- HomeDashboard: (props: { liveEnabled: boolean }) => homeDashboardSpy(props),
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const homeDashboardSpy = vi.fn((_props: { liveEnabled: boolean }) => null); | |
| vi.mock("@/systems/dashboard", () => ({ | |
| HomeDashboard: () => homeDashboardSpy(), | |
| HomeDashboard: (props: { liveEnabled: boolean }) => homeDashboardSpy(props), | |
| const homeDashboardSpy = vi.fn((_props: { liveEnabled: boolean }) => null); | |
| vi.mock("`@/systems/dashboard`", () => ({ | |
| HomeDashboard: (props: { liveEnabled: boolean }) => homeDashboardSpy(props), | |
| })); |
🤖 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 `@web/src/systems/os/apps/dashboard/__tests__/dashboard-window.test.tsx` around
lines 36 - 39, Remove the duplicate homeDashboardSpy declaration and duplicate
HomeDashboard member in the dashboard test mock. Keep one typed homeDashboardSpy
and one HomeDashboard mapping in the vi.mock("`@/systems/dashboard`") setup so the
file typechecks.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/loop/source_delete.go (1)
24-28: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftCanonicalize the writable root before staging.
filepath.Absis lexical only. A symlinked root can putloopDirand the staging parent on different filesystems (os.Renamethen fails withEXDEV) and bypasses canonical containment checks. Resolve withEvalSymlinks, validate the resolved root against the approved boundary, then derive both paths from it.As per coding guidelines, “Verify resolved symlink targets remain inside approved roots using
EvalSymlinksand canonical path-prefix checks.”🤖 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/loop/source_delete.go` around lines 24 - 28, Update the root-resolution flow in the enclosing source-delete function to apply EvalSymlinks after filepath.Abs, validate the canonical root with the existing approved-boundary containment checks, and use that resolved root when calling safeLoopDir and deriving the staging parent. Preserve the existing error-wrapping context while ensuring all writable paths originate from the canonical root.Source: Coding guidelines
🧹 Nitpick comments (2)
internal/settings/provider_curation_reconcile.go (1)
223-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
slices.Equaloverreflect.DeepEqualfor[]string.
ReasoningEffortsis a[]string, soslices.Equalis faster, type-safe, and drops the onlyreflectusage in this file.♻️ Proposed refactor
- if !reflect.DeepEqual(desired.ReasoningEfforts, current.ReasoningEfforts) { + if !slices.Equal(desired.ReasoningEfforts, current.ReasoningEfforts) { next.ReasoningEfforts = cloneStringSlicePreserveNil(desired.ReasoningEfforts) }Then replace the
reflectimport withslicesif it has no other use in this file.🤖 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/settings/provider_curation_reconcile.go` around lines 223 - 225, Replace reflect.DeepEqual with slices.Equal in the ReasoningEfforts comparison within the reconciliation logic, preserving the existing cloneStringSlicePreserveNil assignment. Remove the reflect import and add slices if no other reflect usage remains in the file.packages/site/lib/__tests__/runtime-docs-truth.test.ts (1)
171-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHarden the function-block extraction anchor.
Requiring a trailing
\n}\n\n//means any reordering ofinternal/api/core/errors.go(function moved last, or the following comment removed) yieldsundefinedand a confusingtoMatchfailure. Add an explicit existence assertion so the diagnostic points at extraction rather than the mapping.♻️ Proposed refactor
const resourceStatusMapper = errorSource.match( /func StatusForResourceError\(err error\) int \{[\s\S]*?\n\}\n\n\/\// )?.[0]; + expect(resourceStatusMapper, "StatusForResourceError block not found").toBeDefined(); expect(resourceStatusMapper).toMatch(🤖 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 `@packages/site/lib/__tests__/runtime-docs-truth.test.ts` around lines 171 - 180, Add an explicit existence assertion immediately after extracting resourceStatusMapper, before the mapping toMatch assertions, so missing function-block extraction reports the correct failure. Keep the existing extraction pattern and status-mapping checks unchanged.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/daemon/native_loop_tools_test.go`:
- Around line 326-332: Update the assertion in the Loop error test around
toolErr.ReasonCodes to compare the complete reason-code slice against the
expected set, rather than checking only for tc.wantReason and excluding
ReasonSchemaInvalid. Use the existing test expectation and a specific
slice-equality assertion so any unexpected reason code causes the test to fail.
In `@web/src/systems/settings/hooks/settings-skills-draft-logic.ts`:
- Around line 32-42: Update the save flow using mergeSavedSkillsBaseline so
saveSucceeded receives and applies the normalized SkillsConfig returned by
execute(), rather than the request-body config passed to saveRequested. Preserve
the existing disabled_skills merge behavior while adopting server-normalized
fields such as poll_interval for subsequent dirty checks.
---
Outside diff comments:
In `@internal/loop/source_delete.go`:
- Around line 24-28: Update the root-resolution flow in the enclosing
source-delete function to apply EvalSymlinks after filepath.Abs, validate the
canonical root with the existing approved-boundary containment checks, and use
that resolved root when calling safeLoopDir and deriving the staging parent.
Preserve the existing error-wrapping context while ensuring all writable paths
originate from the canonical root.
---
Nitpick comments:
In `@internal/settings/provider_curation_reconcile.go`:
- Around line 223-225: Replace reflect.DeepEqual with slices.Equal in the
ReasoningEfforts comparison within the reconciliation logic, preserving the
existing cloneStringSlicePreserveNil assignment. Remove the reflect import and
add slices if no other reflect usage remains in the file.
In `@packages/site/lib/__tests__/runtime-docs-truth.test.ts`:
- Around line 171-180: Add an explicit existence assertion immediately after
extracting resourceStatusMapper, before the mapping toMatch assertions, so
missing function-block extraction reports the correct failure. Keep the existing
extraction pattern and status-mapping checks unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6244ec56-7ecf-4944-b449-21ffc9bfca7a
⛔ Files ignored due to path filters (4)
docs/qa/scenarios/TA-075.mdis excluded by!**/*.mdopenapi/compozy.jsonis excluded by!**/*.jsonskills/compozy/references/loops.mdis excluded by!**/*.mdweb/src/generated/compozy-openapi.d.tsis excluded by!**/generated/**,!**/generated/**,!**/*.d.ts
📒 Files selected for processing (33)
internal/api/core/skills_test.gointernal/api/spec/registry_sessions.gointernal/api/spec/spec_test.gointernal/cli/client_tools.gointernal/cli/tool_test.gointernal/daemon/daemon_lifecycle.gointernal/daemon/loop_api_service.gointernal/daemon/loop_delete.gointernal/daemon/native_loop_tools_support.gointernal/daemon/native_loop_tools_test.gointernal/loop/source_delete.gointernal/memory/consolidation/runtime.gointernal/memory/consolidation/runtime_test.gointernal/redact/redact_test.gointernal/session/manager_prompt.gointernal/session/manager_start_stop.gointernal/session/manager_start_test.gointernal/session/manager_test.gointernal/settings/config_apply_service_test.gointernal/settings/provider_curation_reconcile.gointernal/settings/service_test.gointernal/store/globaldb/global_db_heartbeat.gointernal/store/globaldb/global_db_heartbeat_test.gointernal/store/globaldb/global_db_loop_control.gointernal/store/globaldb/global_db_loop_test.gopackages/site/components/landing/bento-section.tsxpackages/site/lib/__tests__/runtime-docs-truth.test.tsweb/src/systems/os/hooks/__tests__/window-manager-runtime.test.tsweb/src/systems/os/hooks/use-window-live-data-enabled.tsweb/src/systems/os/runtime/window-manager-runtime-core.tsweb/src/systems/os/runtime/window-manager-snapshot-refresher.tsweb/src/systems/settings/hooks/__tests__/use-settings-skills-page.test.tsxweb/src/systems/settings/hooks/settings-skills-draft-logic.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/core/skills_test.go
- internal/memory/consolidation/runtime.go
- web/src/systems/os/hooks/use-window-live-data-enabled.ts
- internal/store/globaldb/global_db_heartbeat.go
| if !slices.Contains(toolErr.ReasonCodes, tc.wantReason) || | ||
| slices.Contains(toolErr.ReasonCodes, toolspkg.ReasonSchemaInvalid) { | ||
| t.Fatalf( | ||
| "ReasonCodes = %#v, want only %q Loop control semantics", | ||
| toolErr.ReasonCodes, | ||
| tc.wantReason, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the exact Loop reason-code set.
The current assertion permits unrelated reason codes. Compare the complete slice so an accidental extra mapping cannot pass.
Proposed fix
- if !slices.Contains(toolErr.ReasonCodes, tc.wantReason) ||
- slices.Contains(toolErr.ReasonCodes, toolspkg.ReasonSchemaInvalid) {
+ if !slices.Equal(
+ toolErr.ReasonCodes,
+ []toolspkg.ReasonCode{tc.wantReason},
+ ) {As per path instructions, tests “MUST have specific error assertions.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !slices.Contains(toolErr.ReasonCodes, tc.wantReason) || | |
| slices.Contains(toolErr.ReasonCodes, toolspkg.ReasonSchemaInvalid) { | |
| t.Fatalf( | |
| "ReasonCodes = %#v, want only %q Loop control semantics", | |
| toolErr.ReasonCodes, | |
| tc.wantReason, | |
| ) | |
| if !slices.Equal( | |
| toolErr.ReasonCodes, | |
| []toolspkg.ReasonCode{tc.wantReason}, | |
| ) { | |
| t.Fatalf( | |
| "ReasonCodes = %#v, want only %q Loop control semantics", | |
| toolErr.ReasonCodes, | |
| tc.wantReason, | |
| ) |
🤖 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/daemon/native_loop_tools_test.go` around lines 326 - 332, Update the
assertion in the Loop error test around toolErr.ReasonCodes to compare the
complete reason-code slice against the expected set, rather than checking only
for tc.wantReason and excluding ReasonSchemaInvalid. Use the existing test
expectation and a specific slice-equality assertion so any unexpected reason
code causes the test to fail.
Source: Path instructions
| function mergeSavedSkillsBaseline( | ||
| current: SkillsConfig | null, | ||
| saved: SkillsConfig, | ||
| kind: SaveKind | ||
| ): SkillsConfig { | ||
| if (current === null) return saved; | ||
| if (kind === "disabled") { | ||
| return { ...current, disabled_skills: saved.disabled_skills }; | ||
| } | ||
| return { ...saved, disabled_skills: current.disabled_skills }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C6 'saveRequested\(' web/src/systems/settingsRepository: compozy/compozy
Length of output: 15071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- hook relevant sections ---\n'
sed -n '1,260p' web/src/systems/settings/hooks/use-settings-skills-page.ts
printf '\n--- draft logic relevant sections ---\n'
sed -n '1,180p' web/src/systems/settings/hooks/settings-skills-draft-logic.ts
printf '\n--- imports/types mentioning normalize/execute/settings update ---\n'
rg -n --type=ts 'normalize|poll_interval|SettingsUpdateSkillsRequest|executeAsync|execute\(' web/src web 2>/dev/null | head -200Repository: compozy/compozy
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- hook relevant sections ---'
sed -n '1,260p' web/src/systems/settings/hooks/use-settings-skills-page.ts
echo
echo '--- draft logic relevant sections ---'
sed -n '1,180p' web/src/systems/settings/hooks/settings-skills-draft-logic.ts
echo
echo '--- imports/types mentioning normalize/execute/settings update ---'
rg -n --type=ts 'normalize|poll_interval|SettingsUpdateSkillsRequest|executeAsync|execute\(' web/src web 2>/dev/null | head -200Repository: compozy/compozy
Length of output: 34872
Use the server response as the post-save baseline.
Both saves currently pass the request body config into saveRequested; saveSucceeded then merges that baseline at request time rather than applying any normalized server fields returned by execute(). Pass the server response instead, or apply it after mutation success, so normalized settings such as poll_interval are adopted and subsequent dirty checks don’t falsely stay dirty.
🤖 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 `@web/src/systems/settings/hooks/settings-skills-draft-logic.ts` around lines
32 - 42, Update the save flow using mergeSavedSkillsBaseline so saveSucceeded
receives and applies the normalized SkillsConfig returned by execute(), rather
than the request-body config passed to saveRequested. Preserve the existing
disabled_skills merge behavior while adopting server-normalized fields such as
poll_interval for subsequent dirty checks.
This branch was successfully deployed
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
null.