fix: acp update and general fixes#256
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (14)
WalkthroughThis PR renames ACP ChangesACP Capability Schema: SupportedModels → SupportedModes and Legacy Model Path Removal
SkillResources Syncer Pipeline
CLI Command Surface Enhancements
Infrastructure and Runtime Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/acp/client_test.go (1)
815-818:⚠️ Potential issue | 🟡 MinorWrap these tests in
t.Run("Should...")subtests.Both
TestPromptStreamsSessionUpdatesandTestStartResumeUsesLoadSessionexecute as direct top-level tests without nested subtests. Each contains multiple assertions that should be wrapped in separate named test cases following the required pattern. For example, inTestPromptStreamsSessionUpdates, the event type validations, SessionID check, and SupportedModes assertion should each have their ownt.Run("Should...")subtest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/acp/client_test.go` around lines 815 - 818, The tests TestPromptStreamsSessionUpdates and TestStartResumeUsesLoadSession contain multiple assertions that are not organized into separate subtests. Refactor both test functions by wrapping each distinct assertion (such as the event type validations, SessionID checks, and the SupportedModes assertion shown in the diff) into separate t.Run() subtests with descriptive names following the "Should..." naming pattern. This will make each test case independent and more readable.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/settings/config_apply_service_test.go (1)
347-363: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert blocked reload generation stays at
0in this scenario too.This path verifies blocked/restart-required semantics, but it doesn’t lock the generation contract. Add
Record.Generation == 0to prevent regressions in blocked reload bookkeeping.Suggested assertion
if got, want := result.Record.Status, lifecycle.StatusBlocked; got != want { t.Fatalf("Status = %q, want %q", got, want) } + if got, want := result.Record.Generation, int64(0); got != want { + t.Fatalf("Generation = %d, want %d", got, want) + } }) }🤖 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 347 - 363, Add an assertion to verify that result.Record.Generation equals 0 in the test after the Reload call. This assertion should be added alongside the existing result.Record property checks (Lifecycle and Status) to ensure the generation contract is maintained in the blocked reload scenario and prevent regressions in blocked reload bookkeeping.
🤖 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/core/conversions_parsers_test.go`:
- Around line 145-146: The test in conversions_parsers_test.go is validating
that payload.ACPCaps.SupportsLoadSession is preserved but is missing an explicit
assertion for the SupportedModes field. Add a check immediately after or within
the existing payload.ACPCaps validation to assert that
payload.ACPCaps.SupportedModes is also properly preserved and populated,
ensuring the migration guard validates all critical fields that are seeded as
test input.
In `@internal/api/core/error_paths_test.go`:
- Around line 103-127: The test cases in the error_paths_test.go file currently
only validate HTTP status codes but lack response body assertions and proper
test case naming. For each of the five newly added test case entries (the
scenarios with paths like "/workspaces/ws-workspace/sessions/missing", the
"/sessions/missing/attach", the DELETE to missing session, the events endpoint
with bad query parameter, and the "/logs" endpoint), add a response body
expectation field to the struct definition, wrap each test case in a t.Run call
using a Should-style name pattern (e.g., "Should return NotFound when getting
missing session"), and update the test assertion logic to validate both the HTTP
status code and the response body content in addition to the status-only checks
currently in place.
In `@internal/api/core/extensions_test.go`:
- Around line 131-176: The test function
TestSearchExtensionMarketplaceMapsUnavailableSourceToServiceUnavailable is
currently implemented as a top-level test case but needs to follow the
repository's test convention. Refactor this test by wrapping all of its test
logic inside a t.Run call with a descriptive "Should..." message (such as
"Should map marketplace unavailable source to service unavailable"). Keep the
outer function signature intact but move all the test setup and assertions
inside the t.Run callback to match the required subtest pattern used throughout
the repository.
In `@internal/cli/notifications_test.go`:
- Around line 166-180: The test cases for the "notifications preset update"
command are only asserting that an error occurs (err == nil) but not validating
the specific error content. Replace the generic error checks with specific error
assertions using ErrorContains or ErrorAs to verify that the actual error
messages match the expected validation failures. For the first
executeRootCommand call without flags, assert the error contains the expected
message for missing required flags. For the second executeRootCommand call with
conflicting --enabled and --disabled flags, assert the error contains the
expected message for conflicting arguments.
In `@internal/cli/skill_test.go`:
- Around line 807-813: The slug validation assertions in the executeRootCommand
call around line 807 are currently in the top-level test body instead of being
wrapped in a t.Run("Should...") subtest. Move these assertions (the
executeRootCommand call with "skill", "install", "bad/slug" and the subsequent
error checks for nil and the "skill slug must not include path separators"
message) into a separate t.Run("Should validate skill slug") subtest with a
descriptive name. Ensure this follows the repository's table-driven test
structure with t.Parallel() as needed.
In `@internal/daemon/agent_skill_resources_test.go`:
- Around line 484-541: The test function
TestAgentSkillSourceSyncerSyncSkillsProjectsRegistrySynchronously needs to be
refactored to use the required t.Run("Should...") subtest pattern. Wrap the
entire test scenario (starting from the rawStore declaration through the end of
the function) inside a t.Run call with a descriptive "Should..." message, and
move the t.Parallel() call inside the subtest function. This aligns with the
repository's testing standards that require all test cases to use the t.Run
subtest pattern with t.Parallel as the default.
In `@internal/daemon/bridges.go`:
- Around line 298-306: In the function refreshStaleBridgeTargetDirectory, the
error returned from refreshBridgeTargetDirectoryForInstance is being suppressed
by returning nil instead of propagating it. Modify the error handling block
where refreshBridgeTargetDirectoryForInstance is called (the if statement
checking if err != nil) to return the actual error value err instead of
returning nil, so that stale-refresh errors are properly propagated to the
caller instead of masking failures.
In `@internal/registry/clawhub/client_test.go`:
- Around line 254-309: The test function
TestClientDownloadSynthesizesSkillArchiveFromInfo is implemented as a direct
top-level test body but should follow the test policy by wrapping its content in
a t.Run subtest call. Move all the test logic currently in the function body
(starting from t.Parallel() through the end) into a t.Run call with a
descriptive name following the "Should..." pattern, keeping t.Parallel() at the
top level and ensuring the subtest function receives the *testing.T parameter
properly.
In `@internal/registry/clawhub/client.go`:
- Around line 231-236: In the block where downloadSkillArchiveFromInfo is called
and fallbackErr is checked, the code currently returns the original err when
fallbackErr is not nil, which masks the actual failure reason. Instead of only
handling the success case (when fallbackErr == nil), add logic to return
fallbackErr when it is not nil, so that real synthesis errors like validation or
size failures are properly propagated to the caller instead of being masked as
ErrPackageNotFound.
In `@internal/resources/typed_test.go`:
- Around line 157-209: The TestJSONCodecValidationClassification function
contains two subtests with duplicated setup and assertion patterns that follow
the same validation-classification test structure. Convert these into a single
table-driven test by defining a slice of structs containing the test case data
(validator error to return, expected error type, and expected error message
content), then loop through each case with a single t.Run call that executes the
DecodeAndValidate logic and assertions for each entry. This eliminates code
duplication and aligns with the repository's Go testing guidelines for
table-driven test layouts.
In `@internal/session/session_test.go`:
- Around line 86-110: This test section verifies that the Info() method returns
an immutable copy by checking that mutations to the returned ACPCaps fields do
not affect subsequent calls. Wrap this entire test scenario (the mutation
attempts and subsequent assertions checking that SupportedModes and
ConfigOptions were not modified) in a t.Run subtest with a descriptive name
starting with "Should", such as "Should not mutate through Info() copy", to
comply with the repository's test policy that requires all test cases to use the
t.Run pattern. Ensure proper indentation of the existing assertion code inside
the subtest closure.
In `@internal/skills/marketplace/service_test.go`:
- Around line 200-203: The test only verifies the error type using
errors.Is(err, tc.wantErr) without checking the actual error message details,
allowing error message regressions to pass undetected. In the NormalizeSkillSlug
test case where tc.wantErr is not nil, after the existing errors.Is() check, add
additional assertions using ErrorContains or ErrorAs to validate that the error
message contains the expected details specific to each test case, ensuring
validation messages cannot regress without causing the test to fail.
---
Outside diff comments:
In `@internal/acp/client_test.go`:
- Around line 815-818: The tests TestPromptStreamsSessionUpdates and
TestStartResumeUsesLoadSession contain multiple assertions that are not
organized into separate subtests. Refactor both test functions by wrapping each
distinct assertion (such as the event type validations, SessionID checks, and
the SupportedModes assertion shown in the diff) into separate t.Run() subtests
with descriptive names following the "Should..." naming pattern. This will make
each test case independent and more readable.
---
Nitpick comments:
In `@internal/settings/config_apply_service_test.go`:
- Around line 347-363: Add an assertion to verify that result.Record.Generation
equals 0 in the test after the Reload call. This assertion should be added
alongside the existing result.Record property checks (Lifecycle and Status) to
ensure the generation contract is maintained in the blocked reload scenario and
prevent regressions in blocked reload bookkeeping.
🪄 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
Run ID: a79a9d29-7f94-4e89-b3d9-b062f1b1c062
⛔ Files ignored due to path filters (35)
.agents/skills/cmux-orchestration/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cmux-orchestration/agents/openai.yamlis excluded by!**/*.yaml,!.agents/**.agents/skills/cmux-orchestration/assets/fable-orchestrator-dark.pngis excluded by!**/*.png,!.agents/**.agents/skills/cmux-orchestration/assets/fable-orchestrator.excalidrawis excluded by!.agents/**.agents/skills/cmux-orchestration/assets/fable-orchestrator.pngis excluded by!**/*.png,!.agents/**.agents/skills/compozy/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/cli-reference.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/config-reference.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/skills-reference.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/workflow-guide.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-tasks/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-tasks/references/task-context-schema.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-tasks/references/task-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-techspec/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-execute-task/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-execute-task/references/tracking-checklist.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-fix-reviews/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-idea-factory/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-idea-factory/references/adr-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-idea-factory/references/business-analyst.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-idea-factory/references/council.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-idea-factory/references/idea-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-idea-factory/references/product-strategist.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-idea-factory/references/question-protocol.mdis excluded by!**/*.md,!.agents/**.impeccable/live/config.jsonis excluded by!**/*.jsonCLAUDE.mdis excluded by!**/*.mdPRODUCT.mdis excluded by!**/*.mdgo.sumis excluded by!**/*.sum,!**/*.sumopenapi/agh.jsonis excluded by!**/*.jsonpackages/site/content/runtime/core/agents/providers.mdxis excluded by!**/*.mdxpackages/ui/tsconfig.jsonis excluded by!**/*.jsonskills-lock.jsonis excluded by!**/*.jsonskills/agh/references/agent-definitions.mdis excluded by!**/*.mdskills/agh/references/native-tools.mdis excluded by!**/*.mdweb/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (91)
.repoclone.rcgo.modinternal/acp/client.gointernal/acp/client_test.gointernal/acp/config_options.gointernal/acp/config_options_test.gointernal/acp/types.gointernal/api/contract/contract.gointernal/api/contract/contract_test.gointernal/api/core/conversions.gointernal/api/core/conversions_parsers_test.gointernal/api/core/error_paths_test.gointernal/api/core/extensions.gointernal/api/core/extensions_test.gointernal/api/core/handlers.gointernal/api/core/interfaces.gointernal/api/core/network.gointernal/api/core/network_test.gointernal/api/core/session_workspace.gointernal/api/core/settings_internal_test.gointernal/api/core/skills.gointernal/api/core/skills_test.gointernal/api/httpapi/handlers.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/httpapi/server.gointernal/api/udsapi/server.gointernal/api/udsapi/udsapi_integration_test.gointernal/cli/bridge.gointernal/cli/bridge_test.gointernal/cli/cli_integration_test.gointernal/cli/client.gointernal/cli/client_test.gointernal/cli/format.gointernal/cli/helpers_test.gointernal/cli/hooks.gointernal/cli/notifications.gointernal/cli/notifications_test.gointernal/cli/render_test.gointernal/cli/session.gointernal/cli/skill_commands.gointernal/cli/skill_daemon_test.gointernal/cli/skill_marketplace.gointernal/cli/skill_test.gointernal/cli/task.gointernal/cli/task_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/persistence.gointernal/config/persistence_test.gointernal/daemon/agent_skill_resources.gointernal/daemon/agent_skill_resources_integration_test.gointernal/daemon/agent_skill_resources_test.gointernal/daemon/boot.gointernal/daemon/bridges.gointernal/daemon/bridges_test.gointernal/daemon/daemon.gointernal/daemon/daemon_integration_test.gointernal/daemon/daemon_nightly_combined_integration_test.gointernal/daemon/daemon_sandbox_integration_test.gointernal/daemon/harness_context_integration_test.gointernal/daemon/memory_runtime.gointernal/daemon/memory_runtime_test.gointernal/daemon/native_bundle_resource_tools.gointernal/daemon/native_tools.gointernal/daemon/native_tools_test.gointernal/daemon/perf_bench_test.gointernal/extension/reference_integration_test.gointernal/providers/classify.gointernal/registry/clawhub/client.gointernal/registry/clawhub/client_test.gointernal/resources/codec.gointernal/resources/typed_test.gointernal/session/additional_test.gointernal/session/manager_lifecycle.gointernal/session/manager_stop_integration_test.gointernal/session/manager_test.gointernal/session/perf_bench_test.gointernal/session/provider_runtime.gointernal/session/provider_runtime_test.gointernal/session/session_test.gointernal/settings/config_apply_service.gointernal/settings/config_apply_service_test.gointernal/settings/service_test.gointernal/skills/marketplace/service.gointernal/skills/marketplace/service_test.gointernal/testutil/acpmock/cmd/acpmock-driver/main.gointernal/testutil/e2e/runtime_harness_integration_test.gointernal/tools/mcp.gointernal/tools/mcp_test.goweb/src/systems/network/mocks/fixtures.tsweb/src/systems/session/mocks/fixtures.ts
💤 Files with no reviewable changes (15)
- internal/acp/types.go
- web/src/systems/session/mocks/fixtures.ts
- internal/session/perf_bench_test.go
- internal/acp/config_options.go
- internal/acp/config_options_test.go
- internal/daemon/harness_context_integration_test.go
- internal/api/contract/contract_test.go
- internal/api/contract/contract.go
- internal/cli/cli_integration_test.go
- internal/api/udsapi/udsapi_integration_test.go
- web/src/systems/network/mocks/fixtures.ts
- internal/session/manager_test.go
- internal/api/httpapi/httpapi_integration_test.go
- internal/cli/render_test.go
- internal/cli/session.go
This was referenced Jun 22, 2026
Closed
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
Jun 22, 2026
## Release v0.0.8 This PR prepares the release of version v0.0.8. ### Changelog ## 0.0.8 - 2026-06-22 ### 🐛 Bug Fixes - Acp update and general fixes (#256) - Make docs markdown copy retryable ### 📦 Build System - Update deps - Fix msw setup ### 🧪 Testing - Remove not needed test <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated dependencies to the latest versions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
task run show <run-id>to display expanded task run details.