✨ build improvements, test updates & agentic refactoring [1/3#1388
✨ build improvements, test updates & agentic refactoring [1/3#1388ibolton336 wants to merge 3 commits intokonveyor:mainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis pull request makes systemic changes across build configuration, language server integration, asset management, and test infrastructure. It removes the Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 28 minutes and 56 seconds.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: 3
🧹 Nitpick comments (1)
.husky/pre-commit (1)
5-6: ⚡ Quick winRespect existing
NVM_DIRfor better portability.The current implementation unconditionally sets
NVM_DIRto$HOME/.nvm, which assumes the default installation location and overrides any existing environment variable. Developers with non-standard NVM installations (e.g., via Homebrew or custom paths) may encounter issues.🔧 Proposed improvement to respect existing NVM_DIR
-export NVM_DIR="$HOME/.nvm" +export NVM_DIR="${NVM_DIR:-$HOME/.nvm}" [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"This change uses parameter expansion to respect an existing
NVM_DIRwhile providing$HOME/.nvmas a fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/pre-commit around lines 5 - 6, The pre-commit hook unconditionally overwrites NVM_DIR which breaks non-standard NVM installs; update the .husky/pre-commit script so it preserves an existing NVM_DIR and only falls back to $HOME/.nvm when undefined (use shell parameter expansion for NVM_DIR), then source "$NVM_DIR/nvm.sh" as before; reference the NVM_DIR variable and the existing source line to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agentic/src/nodes/base.ts`:
- Around line 159-163: The code currently always emits an error workflow message
via emitWorkflowMessage with type KaiWorkflowMessageType.Error regardless of the
emitResponseChunks flag; update the error emission in the function/method that
calls emitWorkflowMessage so that it checks the existing emitResponseChunks
boolean (or a new emitErrors option) before sending errors to the workflow
stream—i.e., wrap the emitWorkflowMessage(...) error call in if
(emitResponseChunks) { ... } or if (emitResponseChunks || emitErrors) { ... }
and propagate/define emitErrors where needed (caller sites like
summarizeAdditionalInformation, summarizeHistory, planFixes) so internal
"thinking" operations that set emitResponseChunks:false do not surface errors
unless explicitly allowed.
In `@tests/e2e/enums/output.enum.ts`:
- Around line 1-4: The test enum forces a single exact label for CoreExtension
which breaks branded variants; update the OutputChannels handling so tests
accept both formats instead of a single string — e.g. return or export a
matcher/array that includes both `${extensionShortName} Core Extension` and the
legacy/long-name variant (or switch test lookup to use startsWith/contains
against extensionShortName). Modify the exported OutputChannels.CoreExtension
usage (and any tests that import it) to use the new flexible matcher/array so
E2E channel selection is robust across branded builds.
In `@vscode/go/src/goExternalProviderManager.ts`:
- Line 53: The spawn invocation that starts the external provider (this.process
= spawn(...)) passes an undocumented "-name generic" flag; remove the "-name"
argument and only pass supported flags (e.g., "-socket",
this.providerSocketPath), and instead ensure the provider identification is set
in provider_settings.json (name: "generic") or via the provider configuration
API used by go-external-provider; update the code that constructs the argument
array in goExternalProviderManager (the spawn call) to match the binary's
documented CLI and add/verify provider_settings.json creation or loading logic
so the provider name is configured there.
---
Nitpick comments:
In @.husky/pre-commit:
- Around line 5-6: The pre-commit hook unconditionally overwrites NVM_DIR which
breaks non-standard NVM installs; update the .husky/pre-commit script so it
preserves an existing NVM_DIR and only falls back to $HOME/.nvm when undefined
(use shell parameter expansion for NVM_DIR), then source "$NVM_DIR/nvm.sh" as
before; reference the NVM_DIR variable and the existing source line to locate
and modify the code.
🪄 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: f10b0d52-f0b9-4cfe-b824-fa0e676a47ad
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.github/workflows/e2e-tests.yml.husky/pre-commitagentic/src/cache.tsagentic/src/nodes/analysisIssueFix.tsagentic/src/nodes/base.tsagentic/src/nodes/diagnosticsIssueFix.tsagentic/src/types.tschanges/unreleased/1343-ignore-non-workspace-files.yamlchanges/unreleased/1349-java-retry-activation.yamlchanges/unreleased/1351-pluggable-agent-backend.yamlchanges/unreleased/1374-add-enavled-property-to-llmproxy-client.yamlchanges/unreleased/fix-duplicate-loading-indicators.yamlchanges/unreleased/fix-review-mode-continue-button-blocked.yamlchanges/unreleased/fix-solution-server-inline-edit-capture.yamlpackage.jsonscripts/collect-assets.jsscripts/copy-dist.jstests/.env.exampletests/docs/contrib/e2e-environment.mdtests/e2e/enums/output.enum.tstests/e2e/fixtures/provider-configs.fixture.tstests/e2e/fixtures/test-repos.jsontests/e2e/pages/vscode-web.page.tstests/e2e/pages/vscode.page.tstests/e2e/tests/base/plugin-settings.test.tstests/e2e/tests/ccm/llm-proxy.test.tstests/e2e/utilities/utils.tsvscode/core/src/utilities/ModifiedFiles/handleFileResponse.tsvscode/go/package.jsonvscode/go/src/extension.tsvscode/go/src/goExternalProviderManager.tsvscode/go/src/pathUtils.tsvscode/java/src/extension.ts
💤 Files with no reviewable changes (11)
- changes/unreleased/1343-ignore-non-workspace-files.yaml
- tests/e2e/tests/ccm/llm-proxy.test.ts
- changes/unreleased/1349-java-retry-activation.yaml
- changes/unreleased/fix-solution-server-inline-edit-capture.yaml
- changes/unreleased/1374-add-enavled-property-to-llmproxy-client.yaml
- tests/e2e/utilities/utils.ts
- tests/.env.example
- tests/docs/contrib/e2e-environment.md
- scripts/copy-dist.js
- changes/unreleased/fix-review-mode-continue-button-blocked.yaml
- vscode/go/src/pathUtils.ts
ibolton336
force-pushed
the
refactor/state-management-infra
branch
from
6eb0ff7 to
7a4c368
Compare
ibolton336
left a comment
ibolton336
left a comment
There was a problem hiding this comment.
Reviewed — looks good to merge. Clean foundation layer for the pluggable agent backend stack.
CodeRabbit items addressed (pushed fix commit):
- ✅
.husky/pre-commit— now respects existingNVM_DIRvia parameter expansion fallback (${NVM_DIR:-$HOME/.nvm}) - ✅
agentic/src/nodes/base.ts— error emission gated onemitResponseChunksagain, so internal "thinking" operations (summarizeHistory, planFixes, etc.) don't leak errors to the workflow stream
CodeRabbit items NOT applicable:
tests/e2e/enums/output.enum.ts— the branch already has the branded variant conditional logic; the CodeRabbit comment was about a diff artifactvscode/go/src/goExternalProviderManager.ts— the branch already uses-name go(notgeneric); same diff artifact issue
Build improvements, agentic refactoring, and test fixture updates all look good. No runtime behavior changes to the VS Code extension.
ibolton336
force-pushed
the
refactor/state-management-infra
branch
2 times, most recently
from
3ef6f39 to
ab29ff5
Compare
Agentic package improvements and LLM reliability fixes. No runtime behavior changes to the VS Code extension UI — safe to merge independently. Removed collect-assets, provider, husky, go/java extension, and build changes that overlap with konveyor#1379. ### What's included - **Agentic package** — refactored base node with 5-min LLM timeout, improved caching, updated types (originalContent, result fields) - **LLM reliability** — warn-level logging when LLM returns no response, timeout protection against hanging requests - **Changelog** — added new fragments for pluggable agent backend and duplicate loading indicator fix ### Stack order This is **PR 1 of 3** breaking up the feature/goose branch: 1. **→ Agentic refactoring & LLM improvements** (this PR) 2. Pluggable agent backend → depends on this 3. Chat panel UI → depends on konveyor#2 Signed-off-by: ibolton336 <ibolton@redhat.com>
ibolton336
force-pushed
the
refactor/state-management-infra
branch
from
ab29ff5 to
9675cfa
Compare
ibolton336
changed the title
ibolton336
force-pushed
the
refactor/state-management-infra
branch
3 times, most recently
from
a754b74 to
557b037
Compare
- husky pre-commit: respect existing NVM_DIR with parameter expansion fallback - base.ts: gate error emission on emitResponseChunks to avoid surfacing internal "thinking" operation errors to users Signed-off-by: ibolton336 <ibolton@redhat.com>
ibolton336
force-pushed
the
refactor/state-management-infra
branch
from
557b037 to
90ec75a
Compare
The mcp-server workspace is introduced in a later PR in the stack. Referencing it in postinstall breaks `npm ci` in CI. Signed-off-by: Ian Bolton <ibolton@redhat.com>
1 participant
Summary
Agentic package improvements, LLM reliability fixes, and build infrastructure for the pluggable agent backend. No runtime behavior changes to the VS Code extension UI — safe to merge independently.
What's included
Agentic — LLM timeout protection (
base.ts):Promise.racestreamOrInvokeindefinitelytimeoutMsoptionAgentic — Error emission fix (
base.ts):emitResponseChunksflagsummarizeHistory,planFixes,summarizeAdditionalInformation) that setemitResponseChunks: falseno longer leak error messages to the user-facing workflow streamAgentic — Better no-response logging:
analysisIssueFix.ts:AnalysisIssueFixandSummarizeHistory—logger.silly()→logger.warn()with "This may indicate a model provider configuration issue"diagnosticsIssueFix.ts:PlanFixes,FixGeneralIssues,FixJavaDependencyIssues— same patternAgentic — Type additions (
types.ts):originalContent?: stringtoKaiModifiedFile— needed by PR Add barebones react app to webview #2's Goose agent flow where files are written to disk before processing (used for diffing/revert)result?: stringtoKaiToolCall— allows storing tool call resultsAgentic — Formatting (
cache.ts,types.ts):implementsclause line wrapping forFileBasedResponseCacheandInMemoryCacheWithRevisionsKaiWorkflowgeneric parameterBuild infrastructure:
package.json— Addedmcp-serverworkspace, updatedpostinstallanddevscriptspackage-lock.json— Updated for new workspace.husky/pre-commit— NVM setup with${NVM_DIR:-$HOME/.nvm}fallback for non-standard installsChangelog:
1351-pluggable-agent-backend.yaml— Feature entry for pluggable agent backendfix-duplicate-loading-indicators.yaml— Bugfix entry for duplicate loading indicatorsStack order
This is PR 1 of 3 breaking up the
feature/goosebranch:Testing