Skip to content

fix: address all Copilot review issues (PR #1, #2, #3)#4

Merged
Sj295 merged 1 commit into
mainfrom
fix/copilot-review-issues
Jul 17, 2026
Merged

fix: address all Copilot review issues (PR #1, #2, #3)#4
Sj295 merged 1 commit into
mainfrom
fix/copilot-review-issues

Conversation

@Sj295

@Sj295 Sj295 commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

Addresses all Copilot review issues found across PR #1, #2, and #3.

PR #1 fixes (6 issues)

  1. CronScheduler double onFire -- non-teammate tasks called onFire twice; fixed routing logic so teammate tasks go through onFireTask only, lead tasks go through onFire only
  2. ForkedAgentRunner NPE -- runForked() logged parentHistory.size() before null check; moved size computation after null guard
  3. FileChangedWatcher path matching -- matched events by filename only, ignoring directory; now matches on full normalized absolute path
  4. PermissionDialog race condition -- concurrent tool calls could race on responseQueue and terminal rendering; added synchronized on resolve(); provideResponse() now clears queue before put() to prevent silent drops
  5. CronTools agentId missing from schema -- agentId was read in call() but not declared in inputSchema(); added to properties
  6. ClaudeCodeRequestAdvisor StringBuilder.isEmpty() -- verified compatible with Java 21 (no change needed)

PR #2 fixes (1 issue)

  1. REPL mode tool exposure -- all enabled tools were registered as callbacks even in REPL mode, defeating the token-saving hiding behavior; now filters out REPL_ONLY_TOOLS when REPL mode is enabled

PR #3 fixes (5 issues)

  1. AgentInvoker unused import -- removed java.util.function.Consumer import
  2. permChecker treats Ask as allow -- PermissionResult.Ask was silently allowed, bypassing user confirmation; now returns a denial message
  3. ToolCallbackAdapter Javadoc outdated -- updated to reflect that permission checks now happen inside the adapter
  4. ToolCallbackAdapter null contextTemplate -- null contextTemplate could cause NPEs in tools; constructor now normalizes to ToolUseContext.defaultContext()
  5. ToolCallbackAdapter log without stacktrace -- permission checker exceptions logged with e.getMessage() only; now passes exception object as last param for full stacktrace

Test Results

  • 256 tests pass (no regressions)

Copilot AI review requested due to automatic review settings July 17, 2026 14:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Sj295
Sj295 merged commit 2236bc6 into main Jul 17, 2026
@Sj295
Sj295 deleted the fix/copilot-review-issues branch July 17, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants