Ruiming/fix issue 322 324 325 for idea 261#12114
Conversation
|
Issue 322 validation: |
|
Issue 325 validation: |
|
Issue 324 validation: |
1. Fixed issue that CVE fixing doesn't use the given custom agent. 2. Fixed issue that "Fix the vulnerable with GitHub Copilot" button is missing in problems view -> problem right-click context menu.
…agent-setting-when-fixing-cve Fixed cve fixing relevant issues.
fangjian0423
left a comment
fangjian0423
left a comment
There was a problem hiding this comment.
Reviewed the v261 compatibility fixes. Overall solid, with good defensive reflection and clear comments. One likely regression in the default Agent Mode fallback (see inline), plus a few robustness nits.
| log.warn("Resolved Copilot agent '{}' but withAgentMode(uri) is not exposed by this Copilot version; using default Agent Mode.", | ||
| customAgentName); | ||
| } | ||
| } else if (customAgentName != null && !customAgentName.isBlank()) { |
There was a problem hiding this comment.
Likely regression: default Agent Mode is no longer activated when agentUri == null.
The previous code unconditionally called builder.withAgentMode() in tryReflectionCopilotCall. After this refactor, withAgentMode() is only invoked inside the if (agentUri != null) branch. When agentUri == null — i.e. the plain default path (customAgentName == null) or the custom-agent path where the agent can't be resolved — this else if only logs "falling back to default Agent Mode" but never actually calls withAgentMode().
The Javadoc on this method also states "Falls back to no-arg withAgentMode() on any failure so the chat still opens", which doesn't match the implementation.
Consequence: the chat opens in Ask mode instead of Agent Mode, so #appmod-... tool-invocation prompts won't run.
Suggested fix: add a no-arg withAgentMode() call in the fallback (covering both the customAgentName == null case and the unresolved-agent case), e.g. resolve findAccessibleMethod(builder.getClass(), "withAgentMode", 0) and invoke it before returning when no URI is available.
| } | ||
| final var method = service.getClass().getMethod("getChatModes"); | ||
| final Object flow = method.invoke(service); | ||
| final var getValue = flow.getClass().getMethod("getValue"); |
There was a problem hiding this comment.
Robustness: warm-up uses less robust reflection than the rest of the PR.
flow.getClass().getMethod("getValue") (and getMethod("getChatModes")) can return a Method whose declaring class is package-private (e.g. Kotlin's DerivedStateFlow), in which case invoke(...) throws IllegalAccessException. This is exactly the case the new findAccessibleMethod(...) helper in JavaVersionNotificationService was written to handle.
Because the whole block is wrapped in catch (Throwable), the warm-up would then fail silently and accomplish nothing on the affected Copilot versions. Consider reusing the same findAccessibleMethod walk-up-to-public-interface approach here so the warm-up is as robust as the main path.
| if (copilot != null && copilot.isEnabled()) { | ||
| final ClassLoader cl = copilot.getPluginClassLoader(); | ||
| if (cl != null) { | ||
| for (int i = 0; i < AGENT_RESOLVE_RETRY_ATTEMPTS && !project.isDisposed(); i++) { |
There was a problem hiding this comment.
Minor: retry cost can stack across repeated clicks.
AGENT_RESOLVE_RETRY_ATTEMPTS (100) * AGENT_RESOLVE_RETRY_DELAY_MS (200ms) = up to ~20s of Thread.sleep per invocation. It's correctly off-EDT and guarded by project.isDisposed(), so this is acceptable. But if the user triggers the action several times before the agent is indexed, each click spawns its own pooled thread sleeping up to 20s. Consider de-duplicating in-flight resolution (or caching the resolved URI per project) to avoid piling up threads.
| private static java.net.URI toCopilotUri(@Nonnull java.nio.file.Path path) throws java.net.URISyntaxException { | ||
| final java.net.URI standard = path.toUri(); | ||
| final String s = standard.toString(); | ||
| // Match file:///<DRIVE>:/... and rewrite to file:///<drive>%3A/... |
There was a problem hiding this comment.
Minor: toCopilotUri only handles drive-letter paths, not UNC.
The drive-letter rewrite to file:///c%3A/... looks correct, but a UNC path (\\\\server\\share\\...) won't match the file:///<DRIVE>: shape and is returned as the Java-standard URI, which Copilot's string-equality registry match would then ignore. This is an edge case (project/plugin on a network path), but worth at least a comment documenting the limitation, or handling it explicitly.
2 participants
What does this implement/fix? Explain your changes.
Fixed issues for IntelliJ IDEA v261. Relevant PR for v253: #12091
Does this close any currently open issues?
Any relevant logs, screenshots, error output, etc.?
Any other comments?
…
Has this been tested?