Repository navigation
IDE-features batch: FILES/SEARCH/GIT tab row, git MCP tools (1.6.0) - #21
Conversation
…1.0.87 - SEARCH tab: global content search + replace over the host's ContentSearchProvider (project_search / project_replace MCP tools, new project.replace permission) - GIT tab: changes accordion (stage/unstage/discard/checkout row actions), commit graph with branch model, agent-review and commit-message prompts - MCP tools: git_status, git_stage, git_unstage, git_stage_all, git_unstage_all, git_discard, git_checkout, git_diff, git_diff_all, git_diff_ref, git_diff_between, project_search, project_replace - Version 1.6.0 (build.gradle.kts, synced into plugin.json by processResources): the release that supersedes git-status and git-log - the host's retirement floor is >= 1.6.0 - boss-plugin-api 1.0.72 -> 1.0.87 (IDE-features surface)
Review: IDE-features batch (FILES/SEARCH/GIT, 1.6.0)Read the whole diff (28 files, ~7.1k lines). Well-organised work: the pure logic ( Findings roughly by severity. 1.
|
Before-merge items:
- startAgentReview now reuses collectDiff (compactDiff + slice) instead of
rawUnified with a break loop: the host diffs with -U100000, so rawUnified
is the whole file and the first large file emptied the prompt
- git_status / git_log sample the provider BEFORE the refresh returns when
out-of-process; awaitFresh waits for the post-refresh emission (2s cap,
falls back to the latest value on a conflated no-op refresh)
- destructive git tools now sit behind a git.write permission (declared in
plugin.json): empty requiredPermissions is open to every local session,
including unsigned-in, so git_discard (irreversible) could not stay bare.
Read-only tools stay open; project_replace keeps project.replace
- Replace All over a capped search: the confirm sheet now states the
truncation and the Replace button stays disabled while capped, instead of
silently covering only the listed files
Item 5/6 and polish:
- CodebaseDynamicPlugin gains dispose() (nulls context + all providers)
- unguarded provider calls (loadGraph, op, status timer, collectDiff
diffFile, generateCommitMessage gateway) surface failures via _message
instead of dying in a SupervisorJob scope with the spinner just stopping
- collectDiff no longer dies on one file that fails to diff
- version now read from the bundled manifest (processResources-synced from
build.gradle.kts) instead of a third hand-maintained copy
- project_replace files arg accepts a JSON array (paths may legally contain
commas) with a strict scanner and flat-form fallback; schema documents it
- storage reads/writes off the composition dispatcher; project-path poll
1s -> 5s; orphaned/incorrect KDoc relocated; one-line discard alias
inlined; stray run{} removed
Tests:
- new CodebaseGitMcpToolsTest: RBAC tier per tool, missing-arg errors,
blank files list, oversized diff truncation, multi-file diff header,
files-arg spellings, statusChar totality
- SearchPathTest now disposes its view models (was leaking a scope per test)
c22764a to
7e4080c
Compare
|
All 6 findings addressed in
Polish from the same round: Deferred to a follow-up (noted here rather than hidden): per-match-row LazyColumn virtualization in search results, and tightening the 5s git / 2.5s search poll intervals further. Tests: new |
|
Review — FILES/SEARCH/GIT tab row + git MCP tools (1.6.0) — part 1 of 2 Big, well-organised PR, and the repo conventions are mostly respected: no Android-only APIs (the one AWT touch, PointerIcon(Cursor(...)) at I could not compile or run Gradle in this sandbox (no boss-plugin-api jar available), so everything below is from reading — please sanity-check items 1 and 2 locally. 1. processResources writes a literal dollar-version into the manifest, and 1.6.0 now reads that back at runtime
This was invisible while the manifest version was not load-bearing. It is now: 2. Agent Review drops the diff exactly when there is a diff worth reading
Concretely: one changed file whose compact diff exceeds 15000 chars, and the agent gets the file listing plus a "use git_diff" note and never sees the diff. Multi-file sets fall in the same hole once the per-file 3. Both confirmation sheets are drawn modal but do not behave modally
Related, same block: the 4. SEARCH results lose virtualization and forget their collapse state
5. busy is not a mutex, so the enabled = !busy guards are racy
6. readManifestVersion may read another plugin manifest
7. Switching project leaves the GIT tab on the previous repository
|
|
Review — part 2 of 2: remaining findings, nits, security 8. Load more appears on repositories that have nothing more to load
9. git_status emits a three-column status field for untracked and ignored files
10. Blocking I/O on Dispatchers.Default
11. Cumulative polling SEARCH re-runs the full project scan every 2.5 s (REFRESH_MS) while the tab is open, GIT re-runs git status every 5 s, and the panel re-samples the project path every 5 s. On a large monorepo the search timer alone is a directory walk plus a stat per file, twice a minute, indefinitely. Separately, the git view model init fires 12. The MCP provider captures providers at register time
Minor
Security notes (flagging shape, not asking for changes)
Happy to re-review once the version filter and the Agent Review budget seam are settled. |
Review: IDE-features batch (FILES/SEARCH/GIT, git MCP tools, 1.6.0)Read all the new Kotlin ( Conventions: all clean. Compose Multiplatform only (the lone AWT reference, Findings roughly by priority. (1) and (2) are the ones I'd want before merge; (4)-(6) are cheap. 1. Neither in-panel modal blocks input to the content behind it
Open "Discard changes" on a file, then without dismissing click another row's discard or stage button — it lands. You can also stack a second Drawing these in-panel because the host's browser surface renders above plugin dialogs is sound reasoning, but then the scrim has to swallow input itself: Modifier.fillMaxSize()
.background(CodebaseScrim)
.pointerInput(Unit) { detectTapGestures { /* swallow, or onDismiss() */ } }Same two composables: 2.
|
|
(review continued) 8. Performance: three independent polls, and one un-lazy listNone are blockers, but they compound on a large repo:
9.
|
… on the reloaded graph in the return-to-HEAD test
Review: IDE-features batch — FILES/SEARCH/GIT tab row (1.6.0)Read through the whole diff (29 files, ~7.6k lines). This is a big, well-organized change, and the "why" comments on the non-obvious decisions (status collected rather than snapshotted, Below are the things I would want addressed, roughly in priority order. Correctness1. The Agent Review diff is dropped exactly when 2. The modal sheets do not block the content behind them. 3. Search auto-refresh can resurrect cleared results. 4. A project switch leaves GIT and SEARCH describing the previous project. 5. 6. Security
Performance
Nits
ConventionsCompose Multiplatform only (the single |
|
Addressed in Correctness
Security
Performance
Nits
Deferred (noted in the PR body): half-virtualizing the search match rows, and the double-poll cadence (5 s git status + 2.5 s content re-scan) / unfocused backoff. Local verification: |
|
Review: IDE-features batch (FILES/SEARCH/GIT, 1.6.0) Reviewed the full diff (29 files, ~7.9k lines). Overall this is high-quality work — the pure logic ( I could not compile or run the tests here — no 1. Split-fraction persistence is dead code — gitViewModel.changeLayout.collect { layout -> … } // StateFlow — never completes
gitViewModel.setSplitFraction(…) // unreachable
gitViewModel.splitFraction.debounce(300).collect {…} // unreachable
Each collector needs its own child coroutine: LaunchedEffect(Unit) {
selectedTab = CodebaseTab.fromStorage(withContext(Dispatchers.IO) { storage?.getString("codebase.tab") })
gitViewModel.setChangeLayout(…)
gitViewModel.setSplitFraction(…)
launch { gitViewModel.changeLayout.collect { … } }
launch { gitViewModel.splitFraction.debounce(300).collect { … } }
}Worth a small test around the persistence wiring — the existing suite covers the pure helpers thoroughly but nothing exercises this path, which is why an unreachable-code bug got through. 2. "Load more" never goes away on a small repo — fun hasMoreGraph(): Boolean = _graph.value.size < GRAPH_MAXFor any repo with fewer than 200 commits this is permanently 3.
Two smaller things in the same scanner: 4. The Agent Review caption over-promises — text = if (base.isBlank()) "Review uncommitted changes." else "Review diffs vs. $base."The KDoc on 5. Search results defeat virtualization —
6.
Smaller points
Conventions checklist
Nothing here is a blocker except item 1, which I would fix before release since it silently disables a feature the code claims to implement. Items 2, 3 and 6 are quick, and item 4 is a one-line wording change. |
Finishes the second review round and covers what it did not reach.
Correctness
- The storage LaunchedEffect ran three preferences in sequence, but a
StateFlow collect never completes: everything after the first one was
unreachable, so the splitter position was neither loaded nor saved.
One effect per preference now.
- loadGraph no longer latches "history exhausted" off a FAILED load. The
error path yields an empty list too, and hiding "Load more" until the
next reset is the wrong answer to a dropped IPC call.
- Both modal scrims swallow every pointer event, not only Press, so the
list behind a confirmation sheet cannot be scrolled either.
Dead code the reviewers flagged
- GitChangeTree.emit's prefixLabel was null at both call sites; the chain
compaction is the local StringBuilder's job.
- GitGraphLayout's ancestorCache was written once per key and never read:
assignLanes visits each commit exactly once.
- GitGraphEdges' comment described a stub leaving the last row that the
code does not draw.
- Unused items / PointerEventType imports, and a comment duplicated onto
collapsedDirs.
Tests: 178 -> 196
- GitViewModelOperationsTest pins the three things that only break across
a seam, so neither side's own test saw them: the Agent Review diff
budget (collector meets prompt builder), "Load more" exhaustion, and
batch's first-error-wins rule. The diff-budget case was mutation-checked
- reintroducing the length test fails it.
- PluginManifestTest pins the version chain end to end (gradle -> the
processResources filter -> plugin.json -> the value read back at
runtime) and asserts every permission the git tools require is declared.
- git_status is now pinned by whole LINE: contains("?? u.kt") passed just
as happily on the misaligned " ?? u.kt" it was meant to catch.
- git_log limit and project_search maxResults clamping, the empty JSON
array, GitBranchModel.reviewBases, and collapseHome on a backslash-
joined path (home is a parameter now, so it is testable off Windows).
- Two tests leaked a CoroutineScope per run; both dispose now.
Docs
- README covered only the FILES tree while this release adds SEARCH and
GIT, supersedes git-status and git-log, and ships 16 git_*/project_*
tools. Adds those, the RBAC rationale, the release-ordering constraint,
and corrects the stated minimums (9.2.20/1.0.66 -> 9.5.7/1.0.87).
The tool table is cross-checked against the registrations.
- AGENTS.md carried API version 1.0.20 and a provider list from before
this panel existed.
Verified: ./gradlew clean build green, 196 tests, jar manifest stamped
1.6.0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review: IDE-features batch (FILES/SEARCH/GIT, git MCP tools, 1.6.0)Reviewed the full diff. This is a large, unusually well-documented PR — the "why" comments on the non-obvious decisions (lane-graph edge model, Conventions check: Compose Multiplatform only (no Android APIs; I could not compile or run the suite here (the sibling Findings1.
|
…, observable hasMoreGraph Per the 05:09 review: 1. commit() now has the try/catch every other operation has; a throwing provider (IPC drop, index.lock) reports "Failed: ..." instead of escaping the SupervisorJob scope to the thread's default handler after the spinner stopped. 2. commit() takes the paths to stage, not a stageAll flag. The provider's stageAll is `git add -A`, which also stages untracked files; under a button labelled "Commit All" that sits above a separate UNTRACKED group it quietly commits files the user never touched. The UI passes `changed` (the tracked edits the label counts), and a stage failure stops short of the commit. 3. collectDiff returns the collected text AND a truncated flag, set on every drop path (sliced file, budget exhaustion, file cap, failed fetch) rather than inferred from a marker only the sliced file carries. The prompt builder takes diffTruncated as a fact and says the diff is INCOMPLETE; a collection that simply stopped early no longer reads as complete. 4. hasMoreGraph is a StateFlow (combine of the graph and the exhaustion latch) collected by the UI, not a function reading both as plain values; the confounded-same-list case that would leave the action on screen is now covered by the flow itself. 5. refreshStatus reads isGitRepository through awaitRepositoryFlag() - the same wait the MCP tools' awaitFresh does - instead of sampling the flag in the same breath as the async refresh. Smaller: buildPluginJar/shadowJar no longer re-add the raw src/main/resources alongside the processed output; onProjectChanged clears the commit message, review instructions and graph latch; project.replace permission uses a constant; api types imported instead of fully qualified; three unused catch parameters renamed; the GitCommitRow tooltip block re-indented; parseJsonStringArray no longer trims element interiors; absolutePathOf trims both path separators and joins with the platform's. Tests: commit stages-by-path and never stageAlls; a stage failure and a throwing commit() both surface; a cut-short multi-file collection and a file-cap overflow both announce themselves as incomplete; the truncation warning is driven by the flag, not the marker.
Round 4 - all five findings addressedPer-finding against 1.
|
|
Review: IDE-features batch (FILES/SEARCH/GIT, 1.6.0) Reviewed the full diff at I could not build locally ( 1.
The window: Fix is small: snapshot 2.
Concretely: Cleanest fix is a separate 3. Path arguments do not get the
Whether it is exploitable depends on whether the host separates the pathspec with 4.
5.
Smaller things
Tests 196 green tests, and the ones covering the tricky seams (
Docs / release README and AGENTS.md are accurate against the code, and the release-ordering note about the Items 1 and 2 are the ones I would want fixed before merge, since they can produce a wrong write to source files. Items 3-5 are worth resolving but are lower-stakes. The rest is polish. |
…flags
Per the 05:36 review. Items 1 and 2 could produce a wrong write to source
files, so both are pinned by tests that fail when the bug is reintroduced.
1. The dry run and the write could describe DIFFERENT operations. The
confirmation sheet was rendered from the summary, but applyReplacement
re-read the live query, replacement and flags at click time and took the
file list from the summary. The search field stays live for the whole
dry run, so the sheet could say "Replace 47 across 9 files" and then run
a different operation. previewReplacement now snapshots the whole
operation synchronously (ReplaceOperation) and the write uses nothing
else. A preview that lands after the user has typed on is dropped rather
than republished.
2. execute() cleared the mutex the replace path holds. One flag was doing
two jobs: the replace path CAS'd it while execute() set and cleared it
unconditionally, so a scan finishing mid-write re-enabled the Replace
button and a second click ran a duplicate write. Now _scanning and
_writing are separate, `busy` is their union for the UI, and the
auto-refresh no longer scans during a write.
3. Path arguments do NOT need the isSafeRef treatment refs get, and now say
why: the host runs ProcessBuilder("git", *args) - argv, never a shell -
puts `--` before every pathspec, and sets GIT_LITERAL_PATHSPECS=1.
Verified against DesktopGitService. Checking paths here would only
reject the legal ones beginning with a dash.
4. loadGraph had no concurrency guard. A CAS would have fixed the race by
dropping the second call, which is right for a repeated "Load more" and
wrong for selectGraphBranch and Refresh - dropping those leaves the graph
on the previous ref, which the branch tests caught. Loads are serialised
on a Mutex instead, with a generation counter so only the newest owns the
spinner and a superseded-while-queued request is skipped.
5. The Agent Review pill read "Reviewing…" for any git operation and was
unclickable during unrelated ones. It has its own _reviewing flag now,
like _generating.
Smaller: the graph list keys by index AND hash (a duplicate key throws out
of LazyColumn); formatCommitDate is remembered per row instead of building a
SimpleDateFormat on every recomposition; the view models are held through a
RememberObserver so an ABANDONED composition disposes them too (a
DisposableEffect never runs on that path); and the Agent Review KDoc records
that the prompt is a broadcast carrying source diff.
Concurrency audit while in here: searchJob, refreshTimer and statusTimer are
written from the UI thread and read from coroutines, so they are @volatile;
loadGraph's generation counter is atomic for the same reason.
Tests: 205 -> 212, and the suite drops from 22.1s to 1.8s. The MCP fakes now
PUBLISH on refresh like the real provider, which both pins awaitFresh's happy
path - previously untested - and stops every call paying the full 2s settle;
the settle is injectable so the fallback is pinned cheaply too. New
SearchViewModelTest covers the tab that writes files: the snapshot invariant,
the retracted stale sheet, the write mutex surviving a scan, the capped
early-return, and clear() during a scan.
Verified: ./gradlew clean build green, 212 tests, no source warnings, jar
manifest stamped 1.6.0 with a single plugin.json.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review — IDE-features batch (FILES/SEARCH/GIT, 1.6.0)Read the full head state of the new/changed sources rather than the commit series. This is unusually well-worked code: the reasoning comments are load-bearing rather than decorative, the concurrency choices (compare-and-set before Conventions check: Compose Multiplatform only ( I could not run the build or the suite here (no 1. The four diff tools report "No diff." when git is unavailable
With a null provider — the host predating Same fix as
2. A failed graph load wipes the graph that is already on screen
No test covers a throwing 3.
|
The round-9 host review (finding 10): a file whose matches were capped in the search view can have more replaced than were displayed, and the count the MCP caller reads must not be mistaken for the displayed one. The project_replace description now says so. The SearchTab confirmation sheet already shows the DRY-RUN count, which is the applied count, so it stays as is.
Review: IDE-features batch (FILES/SEARCH/GIT, 1.6.0)Big PR, and a genuinely well-worked one — the KDoc-as-rationale style makes the intent of nearly every non-obvious line checkable, I found two things I would fix before merge and a handful worth a look. Should fix1. Every other LLM-supplied ref on this surface goes through val hash = args.hash()
if (hash == null) missing("hash") else op("Cherry-picked $hash", gitProvider?.cherryPick(hash))
Same gap, much lower stakes, in 2. A failed graph load blanks the graph — val nodes = runCatching { git?.logGraphFor(ref, limit) }.getOrElse { e ->
_message.value = "Failed to load git history: ${e.message}"; failed = true; null
} ?: emptyList()
...
if (!failed) _graphExhausted.value = nodes.size < limit
_graph.value = nodes // <- not guarded
Worth a look3.
4. val previous = searchJob
searchJob = scope.launch { previous?.cancelAndJoin(); if (delayMs > 0) delay(delayMs); execute() }The coroutine can start on 5.
6. The
7.
Notes (non-blocking)
Conventions
Test coverage178 → 196, with the pure logic (branch model, change tree, graph edges/layout, prompt builders, MCP tiers and argument handling) well covered. Three gaps matching the findings above: no case for |
1 participant
Part of the IDE-features batch. Grows the codebase panel from a file tree into a FILES / SEARCH / GIT tab row, and is the release that supersedes the standalone git-status and git-log plugins.
What's in this PR
SEARCH tab - global content search + replace over the host's ContentSearchProvider:
project_search/project_replaceMCP tools; newproject.replacepermissionGIT tab - replaces the git-status + git-log panels:
MCP tools - 16, all on the
bossserver, parent-first, in three RBAC tiers:git_status,git_log,git_diff,git_diff_all,git_diff_ref,git_diff_between,project_searchgit.writegit_stage,git_unstage,git_stage_all,git_unstage_all,git_discard,git_checkout,git_cherry_pick,git_revertproject.replaceproject_replaceAn empty
requiredPermissionsis not a neutral default: the host's MCP registry exposes such a tool to every local session, including one where nobody is signed in. Reads are open on that basis; anything that mutates git state or writes file contents sits behind a grant. Admins bypass both.New permissions (both declared in
plugin.jsondefinedPermissions):project.replaceandgit.write.Ops note: org admins must grant
git.writeto agent roles, or those eight tools are unreachable for non-admins.Manifest:
build.gradle.kts;processResourcessyncs it intoplugin.json)Release ordering (important)
BossConsole's
RetiredPluginsfloor for git-status and git-log is codebase >= 1.6.0, and aRetiredPluginIdsfilter hides those two from this host build on. So this 1.6.0 must ship in the same wave as (or before) the host release that carries the retirement floor. If it lands late the failure is silent: those two panels are hidden and nothing replaces them.Review rounds
Three rounds of claude-review are addressed. Highlights from the last two:
GitViewModelOperationsTestnow pins the seam, and the case was mutation-checked.git_status/git_logsampled.valueright afterrefresh*(); they await the post-refresh emission with a timeout now.LaunchedEffect, but a StateFlowcollectnever completes - so the splitter position was neither loaded nor saved.git_loglimit,project_searchmaxResults),isSafeRefon the MCP ref path, JSON-array unescaping,Dispatchers.IOfor blocking work, per-match LazyColumn virtualization, project-switch reset,dispose(), and theprocessResourcesversion filter (which produced the right answer only by accident).Tests: 178 -> 196, all green. README and AGENTS.md updated - the README documented only the FILES tree and the pre-1.6.0 minimums.
Not in this PR