feat(dashboard): find git repos below the chat's project dir, grouped per repo - #3987
feat(dashboard): find git repos below the chat's project dir, grouped per repo#3987krishdhasmana wants to merge 1 commit into
Conversation
d43cf64 to
4499546
Compare
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Real gap (upward-only repo resolution), solved at the right layer with bounded discovery, back-compat response shape, and boundary checks each tied to a named threat. [DESIGN-REVIEWED] 5cb712f |
GPT 5.6 Review — 🔴 changes requested (blocking)GPT 5.6 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:3843 -- Contained BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:3998 -- Root can change after identity verification [BLOCK-MERGE] 5cb712f |
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Grouped repos, per-repo branches, and explicit skipped/truncated/capped notices turn former silence into legible state; every string cold-reads correctly in the screenshots. Suggestions
[UX-REVIEWED] 5cb712f |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've verified the changed regions in Candidate 1 — grounded. Candidate 2 — dies under falsification. Its own inputs are "could"/"might" ("I could not exercise a case-insensitive FS to confirm"). Candidate 3 — committed screenshot artifacts are repo hygiene, not a runtime defect: no (a)/(b)/(c) — nothing executes wrong. Out of scope for this pass. No new groundable findings emerged from reviewing the discovery cache, TOCTOU re-verification, budget sharing, or egress redaction — those paths are sound. No blocking issues; one advisory finding. FINDING — website/src/components/GitPanel.tsx:188 — a per-group truncation dash uses [OPUS-REVIEWED] 5cb712f Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of I've read the contract, the intent file, the full patch, and checked the repository for existing mechanisms and consumers ( First-Principles-Verdict: CONCERNS Every item names its harm, but the upward-only root cause survives at two sibling endpoints, and one of two refresh buttons is left behind the new cache. What this change shipsIntent: make the Git panel show something for a project dir that contains repositories instead of being one — a FIX (titled
Watch
[FIRST-PRINCIPLES-REVIEWED] 5cb712f |
4499546 to
684af07
Compare
|
All three reviews are addressed in Opus 4.8 —
|
684af07 to
44fac4d
Compare
|
Round 2 addressed in
Also fixed this round, from CI rather than review: my Japanese string spaced two katakana runs |
44fac4d to
803c417
Compare
First Principles Review — dispositions (
|
803c417 to
12228fc
Compare
|
fixed in
Legitimate, and I verified it was exploitable rather than taking it on faith. The scan does refuse a Reproduced by neutralising the new check and running the regression test: with the gate off the
Two tests pin it:
One residual I would rather state than paper over: a sub-millisecond TOCTOU remains between the check 72 backend tests pass, plus flake8, mypy, tsc, build. The spec is updated in the same commit. |
|
fixed in
I built the attack before changing anything: a real repo at So it is worse than "filenames are exposed": the leaked row is stamped with the contained Why my existing gate could not catch it, stated plainly because it is the useful part: every check I The repo is refused rather than silently emptied, which the panel now surfaces as a visible Not covered, deliberately: a project dir that is itself a repo configured this way still reports 78 backend tests (the exploit test fails against the previous code with the leak above), 49 frontend |
e1782d6 to
5a4c8ce
Compare
|
fixed in
My check was Proven before fixing, by patching the module-local The fix is the identity stability your prescription asks for: On the prescribed remedy — "revert descendant collection until verification and Git execution share Both exploit tests discriminate their own fix: with the identity anchor reverted the swap test fails 79 backend tests, 49 frontend tests, flake8, isort, mypy, the black gate, |
… per repo
The Git panel showed nothing for a project directory that is not itself a
repository. Every git endpoint resolved the repo with `git rev-parse --git-dir`
at `cwd=<project dir>`, and git only walks upward -- so a multi-repo workspace
(one repo per package, `<ws>/src/<Package>/.git`) or a directory holding several
checkouts answered `{"repo": false}` and the panel stayed empty. Pointing a chat
at one package worked; pointing it at the workspace did not.
Discovery is now up-then-down: resolve the repo at or above the project dir
first, unchanged for the ordinary case, and only when there is none scan
downward for descendants holding a `.git` entry. The scan is pure filesystem
probing with no git spawn per candidate, bounded by depth (3), repo count (12)
and directories visited (4000) -- depth and count alone leave the walk unbounded
over a merely large non-repo tree -- and cached for 60s since the endpoint polls
every few seconds while the repo set changes rarely. `?refresh=1` bypasses that
cache so the panel's refresh button sees a repo cloned into the workspace
instead of waiting out the TTL.
Dispatch keys on how the single root relates to the project dir, not on
equality: a root at or above the dir keeps the directory-scoped collection, and
any other single root is collected at its own root. A workspace holding exactly
one descendant repo would otherwise be re-probed at the non-repo dir and report
"not a repo" -- the feature failing on the simplest case it exists for.
The response keeps its single-repo shape so existing consumers work unchanged,
and every row now carries its own `repoRoot`: sibling packages routinely share a
repo-relative path (`src/PkgA/a.txt` and `src/PkgB/a.txt` are both `a.txt`), so
joining a path to one response-level root would resolve a file that does not
exist. A multi-repo answer adds a `repos` array for grouping and omits
top-level branch/ahead/behind, since siblings each have their own.
Two constraints shaped the implementation. Per-repo collection runs
concurrently under a semaphore and skips `numstat` for a clean repo, because
each collection spawns git through the OS sandbox and an unbounded burst costs
more than the wait it saves. And `files` and `repos[].files` share one 500-row
budget so both hold the same row objects -- capping them separately let group
rows past the cap leave without passing through the egress redaction.
Repo roots are normalised to native separators. `git rev-parse --show-toplevel`
answers with forward slashes even on Windows, where the scanned descendants and
the project dir carry backslashes, so the dispatch comparisons above read wrong
and a row's `repoRoot` changed shape with the route that resolved it.
GitPanel renders one header per repo (folder icon, path relative to the project
dir, its branch, its count) when more than one is present, resolves each row
against its own repo root, and names the repo count where a single branch would
go. It also reports the states the grouped view can reach and previously showed
as silence: a truncation notice when the shared row budget is spent, a `skipped`
tag for a repo refused for declaring a content-filter driver, and a note that
commit history is unavailable for a multi-repo project, since the log endpoint
still resolves upward only.
5a4c8ce to
5cb712f
Compare
|
acknowledged — real, and I am escalating rather than patching it a third time. This is the third What I accept. The window is real. Any path-based check followed by a subprocess has one, and What I could not find: a portable way to close it. Closing it needs the spawn anchored to an
So the options are a Linux-only path with a silently weaker guarantee on the other two platforms, or Where this differs from your first two findings, which I fixed without argument. Those were It is also not specific to this PR. I am handing the call to the repository owner rather than deciding it myself, since the prescription —
Separately: the two Windows shards were red on my own new tests (POSIX escape mechanics — |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
Heads-up from an adjacent triage: we looked at #7691 (submodule support in the right-side Git panel) and your grouped-repo work here is the seam it needs, so we are deliberately not building it in parallel -- it is queued as a follow-on behind this PR. Also, as an independent reader: your parent-only line |
|
@krishdhasmana thanks for this, and for how faithful the description is to the diff. Flagging two open PRs that edit the same egress block in #6905 (@leonlaiyc) adds This PR is the one that multiplies the fields the winner has to cover: it adds Conflicts are certain, not just possible: #6905 and this PR both add a test to Suggested order: land one redactor first, then rebase this PR and adopt it for the three new path fields in the same change. That fits the rebase this PR needs anyway, since it is 1922 commits behind with Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
The bug
The Git panel showed nothing for a project directory that is not itself a repository.
Every git endpoint resolved the repo with
git rev-parse --git-diratcwd=<project dir>, and git only walks upward. So a directory that contains repositories rather than being one answered{"repo": false}and the panel stayed empty:<ws>/src/<Package>/.git)Pointing a chat at one package worked; pointing it at the workspace did not.
The change
Discovery is up-then-down.
_discover_repo_rootsresolves the repo at or above the project dir first — unchanged behaviour for the ordinary single-repo case — and only when there is none does it scan downward for descendants holding a.gitentry.The descendant scan is pure filesystem probing (
os.scandir, nogitspawn per candidate), bounded on depth (3), repo count (12) and directories visited (4000) — depth and count alone leave the walk unbounded over a merely large non-repo tree, so the visited cap is what keeps a cache miss from stat'ing a whole subtree. It skips heavy and dotted directories, never descends into a repo it just found (a submodule's status belongs to its parent), and caches per base for 60s, since the endpoint polls every few seconds while the repo set changes rarely. An entry whose metadata cannot be read skips that entry instead of raising — anOSErrorout of the scan would surface as a 500 for the whole panel. Discovery reports whether a bound truncated it (reposTruncated), because a repo dropped by a cap otherwise reads as nonexistent — the same failure mode the row notice exists to prevent.?refresh=1bypasses that cache, so the panel's refresh button picks up a repo cloned into the workspace instead of waiting out the TTL.Dispatch keys on how the single root relates to the project dir, not on equality: the dir itself being the root is the ordinary single-repo answer, a root above it keeps the directory-scoped collection, and every descendant set takes the grouped path, one repo included. A workspace holding exactly one descendant repo would otherwise be re-probed at the non-repo dir and report "not a repo" — the feature failing on the simplest case it exists for — and reporting it as a bare single repo would hide both its name and a per-repo refusal while needing its own render case in the panel. Roots are also normalised to native separators, because
git rev-parse --show-toplevelanswers with forward slashes even on Windows while the scanned descendants and the project dir carry backslashes, which made those comparisons read wrong there and let a row'srepoRootchange shape with the route that resolved it.The response keeps its single-repo shape, so existing consumers work unchanged. Two additions:
repoRoot. Sibling packages routinely share a repo-relative path —src/PkgA/a.txtandsrc/PkgB/a.txtare both justa.txt— so joining a path to one response-level root would open a file that does not exist. The row names its own owner.repos: [{root, name, branch?, ahead?, behind?, files}]for grouping, and omits top-levelbranch/ahead/behind, since siblings each have their own and promoting one would misreport the rest.Every consumer anchors a row on that row's own
repoRoot. GitPanel renders one header per repo (folder icon, path relative to the project dir, its branch, its changed-file count) when more than one is present, resolves each row against its own repo root, and names the repo count where a single branch label would go. The project tree's changed-lane mapping (PierreWorkspaceTreeImpl) does the same: it previously fell back to the project root whenever the response carried no top-levelrepoRoot— which a grouped answer never does — dropping the<ws>/src/<Pkg>/segment. That both mis-nested the lane and made the row's absolute path miss on disk, so opening any file in a multi-repo workspace answered "File not found on disk". A single-repo answer renders as one unnamed group, so there is one render path rather than two.It also reports the states the grouped view can reach that previously rendered as silence: a truncation notice when the shared row budget is spent (without it, a repo sliced off by the cap showed a group header reading
0and was indistinguishable from clean), a notice when discovery itself was capped, askippedtag — with atitlenaming the cause, so the state is not a dead end — for a repo refused for declaring a content-filter driver, and a note that commit history is unavailable for a multi-repo project —/api/project/git/logstill resolves upward only, so a workspace root has no repo-level history to show.Two constraints worth calling out
Spawn cost is the real budget. Each per-repo collection spawns
gitthrough the OS sandbox, so collection runs concurrently under a semaphore (6) rather than unbounded — an unbounded burst costs more than the wait it saves — andnumstatis skipped for a repo with no changed files, removing one spawn per untouched package.Repo roots are re-checked at use time, not trusted for the cache TTL. The scan refuses a symlinked child, but that is a scan-time control and the answer is cached, so a child swapped for a symlink out of the project inside the window would have had git run in a directory the project allow-list never covered — disclosing that repository's branch and filenames through an endpoint scoped to the project.
_verify_descendant_rootsre-resolves each root immediately before collection (realpathcontainment under the project dir, not a symlink, not sensitive), drops the cache entry, and marks the answer partial. Pinned by a test that reproduces the swap and asserts the outside branch and filename never appear.One row budget shared across groups is a security property, not a tidiness one.
filesandrepos[].filesdeliberately hold the same row objects under a single 500-row cap. Capping the merged list separately from the groups let group rows past the cap leave the process without passing through the egress redaction — the redaction pass walksfiles, so anything not in it escapes unredacted. Redaction now coversfiles[].path,files[].repoRoot, and each group'sroot/name/branch.The existing per-repo refusal for a repo whose own config names a content-filter driver (
_repo_declares_filter_driver) is preserved, evaluated per repo.Verification
test_project_git_status_log.py+test_project_git.py), including 12 new tests inTestMultiRepoDiscovery: grouping per descendant repo, per-rowrepoRootdisambiguating identical relative paths, a clean workspace, a directory with no repos anywhere (still{"repo": false}, noreposkey), a single-repo answer keeping its exact shape, a workspace holding exactly one descendant repo,?refresh=1seeing a repo cloned after the first poll, root normalisation (fed a path with redundant separators, so it pins the Windows behaviour from a POSIX runner too), a lone descendant answering in the grouped shape, a lone descendant's filter-driver refusal staying visible, a capped discovery reportingreposTruncated, an unreadable directory entry skipping that entry instead of failing the request, and a single-repo filter-driver refusal reaching the client as its own field.flake8andmypyclean on the changed Python.PierreWorkspaceTreeImplcase that feeds two sibling repos whose rows share the identical repo-relative path and asserts both land on their own lane (before the fix they collapsed onto one wrong row, the second silently deduped).tsc -bclean, production build green,eslintsilent on the changed TS, 10 GitPanel-adjacent frontend tests pass,npm run i18n:checkgreen across all 16 checks (four newcomponents.gitPanelkeys in all 13 catalogs, pseudolocale regenerated),scripts/docs-lint.shclean.blackwas deliberately not run: both touched Python files already failblack --checkon pristinemain, so reformatting them would bury this change in unrelated churn.The module spec (
docs/system-specs/modules/learn-cron-dashboard.md) gains a Project git status entry in the same commit, documenting discovery, the bounds, caching, the response shape, per-rowrepoRoot, and the shared-budget/redaction coupling.Screenshots
Captured with
website/scripts/capture-multi-repo-git-panel.mjs(added here, following theexisting
capture-*.mjsconvention): the real built SPA served fromwebsite/distwith every/api/**call answered from fixtures via Playwright route interception — realGitPanel, real appshell, gateway-free.
Four sibling package repos, grouped. Note
src/handlers/login.pyappearing under bothsrc/AuthServiceandsrc/WebFrontend— the identical-relative-path case that per-rowrepoRootexists to disambiguate.
The single-repo shape is unchanged —
trunk ↑1, one flat list, no group headers.Truncation, a skipped repo, and a clean workspace
The shared 500-row budget spent inside the first repo, scrolled to the end of the list. The two
later groups read
0, and the notice is what says why.A repo refused for declaring a content-filter driver, tagged
skippedwhere its count would be.A clean multi-repo workspace: the changes section collapses and the Commits section explains why it
carries no history.
Discovery stopped at a bound, so the repo list is partial and says so.
no linked issue: found while testing the multi-repo panel live, no tracked issue was filed
Not covered
Two siblings resolve upward only, and both are deliberate rather than overlooked:
/api/project/git/log(the Commits list) is not scoped per repository. That is a feature rather than a fix, so the panel states the limitation instead of widening this change./api/project/treeresolves its repo probe upward too, so a workspace root degrades to the.gitignore-blind filesystem walk. Same root cause, and out of scope here: the tree's listing is not what this PR fixes, and making it multi-repo-aware means deciding how several repos' ignore rules compose over one tree./api/project/git(_project_git_branch, the activity-panel branch label) still walks upward and answers{"repo": false}for a workspace root. That answer is correct here for the same reason the grouped status response omits a top-levelbranch: a project covering several repos has no single checked-out branch, so promoting one sibling's branch into a gateway-wide label would misreport the rest. It reads a single.git/HEADwith nogitspawn, and making it multi-repo-aware means deciding what a label over N branches should say — a design question, not a defect.