Skip to content

feat(dashboard): find git repos below the chat's project dir, grouped per repo - #3987

Draft
krishdhasmana wants to merge 1 commit into
mainfrom
feat/multi-repo-git-status
Draft

feat(dashboard): find git repos below the chat's project dir, grouped per repo#3987
krishdhasmana wants to merge 1 commit into
mainfrom
feat/multi-repo-git-status

Conversation

@krishdhasmana

@krishdhasmana krishdhasmana commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

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-dir at cwd=<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:

  • a workspace laid out as one repo per package (<ws>/src/<Package>/.git)
  • any directory holding several checkouts side by side

Pointing a chat at one package worked; pointing it at the workspace did not.

The change

Discovery is up-then-down. _discover_repo_roots resolves 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 .git entry.

The descendant scan is pure filesystem probing (os.scandir, no git spawn 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 — an OSError out 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=1 bypasses 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-toplevel answers 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's repoRoot change shape with the route that resolved it.

The response keeps its single-repo shape, so existing consumers work unchanged. Two additions:

  • Every row carries its own repoRoot. Sibling packages routinely share a repo-relative path — src/PkgA/a.txt and src/PkgB/a.txt are both just a.txt — so joining a path to one response-level root would open a file that does not exist. The row names its own owner.
  • A multi-repo answer adds repos: [{root, name, branch?, ahead?, behind?, files}] for grouping, and omits top-level branch/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-level repoRoot — 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 0 and was indistinguishable from clean), a notice when discovery itself was capped, a skipped tag — with a title naming 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/log still 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 git through the OS sandbox, so collection runs concurrently under a semaphore (6) rather than unbounded — an unbounded burst costs more than the wait it saves — and numstat is 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_roots re-resolves each root immediately before collection (realpath containment 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. files and repos[].files deliberately 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 walks files, so anything not in it escapes unredacted. Redaction now covers files[].path, files[].repoRoot, and each group's root/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

  • 79 backend tests pass (test_project_git_status_log.py + test_project_git.py), including 12 new tests in TestMultiRepoDiscovery: grouping per descendant repo, per-row repoRoot disambiguating identical relative paths, a clean workspace, a directory with no repos anywhere (still {"repo": false}, no repos key), a single-repo answer keeping its exact shape, a workspace holding exactly one descendant repo, ?refresh=1 seeing 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 reporting reposTruncated, 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.
  • flake8 and mypy clean on the changed Python.
  • 48 frontend tests across the touched specs, including a new PierreWorkspaceTreeImpl case 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 -b clean, production build green, eslint silent on the changed TS, 10 GitPanel-adjacent frontend tests pass, npm run i18n:check green across all 16 checks (four new components.gitPanel keys in all 13 catalogs, pseudolocale regenerated), scripts/docs-lint.sh clean.
  • black was deliberately not run: both touched Python files already fail black --check on pristine main, 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-row repoRoot, and the shared-budget/redaction coupling.

Screenshots

Captured with website/scripts/capture-multi-repo-git-panel.mjs (added here, following the
existing capture-*.mjs convention): the real built SPA served from website/dist with every
/api/** call answered from fixtures via Playwright route interception — real GitPanel, real app
shell, gateway-free.

Four sibling package repos, grouped. Note src/handlers/login.py appearing under both
src/AuthService and src/WebFrontend — the identical-relative-path case that per-row repoRoot
exists to disambiguate.

Git panel with four repos grouped per repository

The single-repo shape is unchanged — trunk ↑1, one flat list, no group headers.

Git panel for a single repository, unchanged

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.

Truncation notice at the end of the changes list

A repo refused for declaring a content-filter driver, tagged skipped where its count would be.

A skipped repository in the grouped list

A clean multi-repo workspace: the changes section collapses and the Commits section explains why it
carries no history.

Clean multi-repo workspace

Discovery stopped at a bound, so the repo list is partial and says so.

Notice that some repositories were not scanned

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/tree resolves 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-level branch: 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/HEAD with no git spawn, and making it multi-repo-aware means deciding what a label over N branches should say — a design question, not a defect.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 16, 2026
@krishdhasmana
krishdhasmana force-pushed the feat/multi-repo-git-status branch from d43cf64 to 4499546 Compare August 16, 2026 17:59
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 5cb712fb5d8c10b41adb1f01f94b2d4efe6e47d3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 5cb712fb5d8c10b41adb1f01f94b2d4efe6e47d3.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:3843 -- Contained .git can redirect metadata outside
return resolved == real_base or resolved.startswith(real_base + os.sep)
Symlinked .git/index -> descendant validation -> git status reads another checkout’s index -> outside filenames are exposed.
Anchor: backend-security-controls
Fix: Reject redirected metadata or revert descendant collection.

BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:3998 -- Root can change after identity verification
status_rc, status_out, _ = _run_git_bounded(
Root swapped after rev-parse --show-toplevel -> subsequent status spawn follows the replacement -> outside branch and filenames are exposed.
Anchor: backend-security-controls
Fix: Pin the verified directory identity through collection or revert descendant collection.

[BLOCK-MERGE] 5cb712f
[GPT-REVIEWED] 5cb712f
False positive or not applicable? A repository writer can comment:
/ai-review override gpt 5cb712fb5d8c10b41adb1f01f94b2d4efe6e47d3: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 5cb712fb5d8c10b41adb1f01f94b2d4efe6e47d3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • uncommitted pill / CHANGES count: when status.truncated is set, "500 uncommitted" (after-04) reads as an exact total — render "500+" so the cap doesn't pose as the count.
  • Group headers in GitPanel.tsx: make them sticky — after-04 shows a scroll position where 25+ rows render with no repo label in view, and sibling repos share identical relative paths (src/handlers/login.py twice in after-01), so mid-scroll rows lose their only disambiguator.
  • repo_skipped_reason ("…this repository's git configuration is unsafe to read") is one shared footer that can cover several refused repos — reword to number-neutral ("a repository's…") or render per refused group.

[UX-REVIEWED] 5cb712f

@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 5cb712fb5d8c10b41adb1f01f94b2d4efe6e47d3 — this comment is updated in place on each push.

Review details

I've verified the changed regions in files.py, the GitPanel.tsx rendering, and the dispatch logic against the diff. Let me assess the three candidates.

Candidate 1 — grounded. fileCount = status?.files?.length ?? 0 is the merged workspace total (GitPanel.tsx:91), and the per-group truncation dash reuses it as title={i18nT('components.gitPanel.truncated', { count: fileCount })} (line 188). A group starved by the shared _REPO_ROW_BUDGET renders with a tooltip citing the whole-workspace count, not that group's own count (which the payload never carries). Concrete, observable, in the changed lines. Advisory only — a hover tooltip.

Candidate 2 — dies under falsification. Its own inputs are "could"/"might" ("I could not exercise a case-insensitive FS to confirm"). base is os.path.realpath(...) and roots[0] is normpath(git show-toplevel), which git returns realpath-resolved; the passing test_single_repo_answer_keeps_its_shape confirms the common case matches. Even granting the divergence, files display correctly — presentation only, no wrong data. Below 80.

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 title={i18nT('components.gitPanel.truncated', { count: fileCount })}, where fileCount is the merged workspace total, so a budget-starved group's tooltip reads "Truncated at 500 files" regardless of that repo's actual (unknown) count → Fix: drop the numeric count from the group dash tooltip (the group's true row count is not carried in the payload), e.g. use a countless "truncated" string there.

[OPUS-REVIEWED] 5cb712f

Verdict parsed from the review's SHA-scoped output markers for commit 5cb712fb5d8c10b41adb1f01f94b2d4efe6e47d3.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 5cb712fb5d8c10b41adb1f01f94b2d4efe6e47d3: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 16, 2026
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 5cb712fb5d8c10b41adb1f01f94b2d4efe6e47d3 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I've read the contract, the intent file, the full patch, and checked the repository for existing mechanisms and consumers (projectGitStatus consumers, normalizeWindowsPath usage, existing repo-discovery code, the temp-screenshots/ convention). Here is the review.

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 ships

Intent: make the Git panel show something for a project dir that contains repositories instead of being one — a FIX (titled feat, described as a bug; the diff matches the description).

  1. Panel lists repos discovered below a non-repo project dir, grouped per repo — justified (the reported defect).
  2. Every file row carries its own repoRoot; opens and tree lanes anchor on it — justified (siblings share relative paths; "file not found" defect).
  3. New repos[] response field; single-repo shape unchanged — justified, consumers counted (GitPanel, PierreWorkspaceTreeImpl).
  4. Filter-driver refusal now visible as skipped + reason — justified visibility change (was silent files: []).
  5. Truncation notices for row budget and capped discovery — justified (a capped repo read as clean/nonexistent).
  6. Refresh button bypasses a new 60s discovery cache — justified; sibling refresh unwired (Watch).
  7. Discovery cached 60s per base — changed timing, declared.
  8. "Commit history unavailable" notice for multi-repo — declared deferral; cause left at siblings (Watch).
  9. Windows separator normalisation, backend and tree lanes — justified, pinned by tests.
  10. Capture script + 6 PNGs — rides along; matches repo convention (252 capture-*.mjs, 2300+ shots in temp-screenshots/).

Watch

  • The root cause — upward-only repo resolution — has 2 unfixed siblings (grepped rev-parse in handlers/files.py): /api/project/git/log is declared and deferred with a notice, but /api/project/git (the activity-panel branch label) is undeclared and still answers {"repo": false} for the same workspace this PR exists for.
  • 2 consumers poll ['git-status', projectDir]; only GitPanel's refresh sets fresh=1. FileBrowserRail.tsx:91-101 refetches through the 60s cache, so the rail's explicit refresh cannot see a newly cloned repo — the exact harm ?refresh=1 was added to remove.

[FIRST-PRINCIPLES-REVIEWED] 5cb712f

@krishdhasmana
krishdhasmana force-pushed the feat/multi-repo-git-status branch from 4499546 to 684af07 Compare August 16, 2026 18:53
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 16, 2026
@krishdhasmana

Copy link
Copy Markdown
Contributor Author

All three reviews are addressed in 684af079d. Dispositions per finding:

Opus 4.8 — files.py:3229, lone descendant repo reports "not a repo"

fixed (684af079d). Correct and the most serious of the round: roots[0] != base sent a
single descendant down the ancestor branch, which re-probed the non-repo dir. Dispatch now keys on
how the root relates to the project dir rather than on equality — a root at or above the dir keeps
the directory-scoped collection, any other single root is collected at its own root. Pinned by
test_single_descendant_repo_is_collected_at_its_own_root; the two-repo-only coverage you noted was
exactly why this got through.

The same review round surfaced a second defect in that comparison: git rev-parse --show-toplevel
answers with forward slashes on Windows while the scanned descendants and the project dir carry
backslashes, so both path comparisons read wrong there and repoRoot changed shape with the
resolution route. Roots are normalised to native separators, pinned portably by
test_upward_root_is_normalised.

Design Review

  • Visited-directory capfixed. _REPO_SCAN_MAX_ENTRIES = 4000 bounds the walk itself.
    You were right that the description overclaimed: depth and result count leave a merely large
    non-repo tree fully walked on every cache expiry.
  • No cache bypass on manual refreshfixed. ?refresh=1 skips the TTL cache; the panel's
    refresh button sets it for one fetch while the 5s poll keeps using the cache.
    test_refresh_bypasses_the_root_scan_cache covers it.
  • Per-group refused flagfixed. A repo refused for declaring a content-filter driver
    carries refused: true and the panel tags that group skipped, so it is distinguishable from a
    clean one.

UX Review

  • Permanent false loading statefixed. The header no longer falls back to loading...
    when data has arrived; a grouped answer shows the repo count there instead. Caught independently
    by screenshotting the panel before this round.
  • Silent truncation misreports a repo as cleanfixed. A one-line notice renders when
    truncated is set. Screenshot in the PR body (after-04-truncated.png), scrolled to the end of
    a spent 500-row budget so the two starved groups reading 0 are visible above it.
  • Commits section silently vanishesfixed, as a stated limitation rather than a feature:
    the section renders "Commit history is not available for multi-repo projects". Scoping the log per
    repo is real work beyond this PR's purpose, so it is named in Not covered instead of widened
    into here.
  • Group header reuses the branch icon for a repo namefixed. The name takes FolderGit2
    and GitBranch is reserved for the branch beside it.

Screenshots (five states) are committed under temp-screenshots/multi-repo-git-status/ and embedded
SHA-pinned in the description, captured by a new capture-multi-repo-git-panel.mjs harness
following the existing capture-*.mjs convention.

Also fixed from this round: the Windows-only CI failure above, and a redundant empty-state line that
rendered directly beneath the new Commits note on a clean workspace.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 16, 2026
@krishdhasmana
krishdhasmana force-pushed the feat/multi-repo-git-status branch from 684af07 to 44fac4d Compare August 16, 2026 19:25
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 16, 2026
@krishdhasmana

Copy link
Copy Markdown
Contributor Author

Round 2 addressed in 44fac4dfa.

  • GPT — BLOCKING, files.py:3015, unreadable directory entry aborts discoveryfixed.

    Entry metadata raises OSError -> _discover_repo_roots propagates through
    asyncio.to_thread -> Git status returns HTTP 500.

    Correct and reachable: os.DirEntry.is_dir() hits the filesystem, so a vanished entry or a
    permission-denied mount point took down the whole panel, not one branch of the walk. The per-entry
    probe (is_dir + the .git existence check) is now inside try/except OSError and continues to
    the next entry; the outer os.scandir guard was already there. The skip also sets the
    truncation flag, so a partial answer says it is partial. Pinned by
    test_unreadable_entry_does_not_abort_discovery, which drives a hostile DirEntry that raises
    from is_dir().

  • Design — discovery caps truncate silentlyfixed. You applied my own argument back at
    me correctly: _scan returning at a bound is exactly the "sliced off and indistinguishable from
    clean" shape the row notice exists to prevent. _discover_repo_roots now returns whether any
    bound tripped (repo count, depth, visited entries, or an unreadable directory), the response
    carries reposTruncated, and the panel renders it in the same one-line style as the row notice —
    including when everything found is clean, which would otherwise hide the section entirely.
    Screenshot after-06-capped.png in the description. Pinned by
    test_capped_discovery_says_the_list_is_partial.

  • Design (suggestion) — lone descendant refusal renders as cleanfixed, taking the
    restructure you suggested rather than patching the symptom: every descendant set now takes the
    grouped path, one repo included, so the refusal surfaces as a group and the panel loses a render
    special-case. Pinned by test_single_descendant_repo_takes_the_grouped_shape and
    test_lone_descendant_refusal_is_visible_as_a_group.

  • repo_skipped is a comprehension dead-endfixed. The tag carries a title naming the
    cause ("Skipped: this repository declares a git content filter"), so the state is
    distinguishable from clean and the user can tell that refreshing will not change it.

  • /api/project/git sibling left unmentionedrebutted, and now mentioned. Fair that it
    was silently absent; the PR body's Not covered section names both siblings explicitly. On the
    substance: _project_git_branch answering {"repo": false} for a workspace root is correct
    for the same reason the grouped status response omits a top-level branch — a project covering
    several repos has no single checked-out branch, and promoting one sibling's into a gateway-wide
    label would misreport the rest. Making it multi-repo-aware requires deciding what a label over N
    branches should say, which is a design question rather than a defect, so it is named rather than
    guessed at here.

Also fixed this round, from CI rather than review: my Japanese string spaced two katakana runs
(マルチリポジトリ プロジェクト), which style/ja.md §2 forbids and jaStyle.test.ts enforces —
that was the one failing test in the full suite. Closing it up surfaced a second problem in the
same area: repos_count needed real plural forms now that a lone descendant can render a count of
1, and Japanese, Korean and Chinese have only the other CLDR category, so an _one sibling there
is unreachable (jaStyle.test.ts §5). Each catalog now carries exactly its own categories, the
plural base is declared in pluralKeys.json, and the pseudolocale is regenerated. All 597 i18n
tests pass.

@krishdhasmana
krishdhasmana force-pushed the feat/multi-repo-git-status branch from 44fac4d to 803c417 Compare August 16, 2026 19:32
@krishdhasmana

Copy link
Copy Markdown
Contributor Author

First Principles Review — dispositions (803c417b4)

Thanks — the consumer count is the part I had not actually verified, and two of the three land.

  • repos[].ahead/behind have zero consumersfixed (subtraction taken). Confirmed: only
    the top-level pair is read (GitPanel.tsx:83 for the log-refetch marker, :121-124 for the
    pill); the groups memo never touches the per-group copies. They are gone from the group-entry
    copy loop and from the repos? TS type, and the grouping test now asserts their absence so they
    cannot drift back in unread.

  • refused still rendered as silence on the single-repo pathfixed, and this one was a
    real user-visible gap rather than surface: an unnamed group draws no header, so there was nowhere
    for the tag to appear. The refusal now reaches the client as a top-level refused and the panel
    states the reason in the same one-line notice style as the truncation notices, so the
    "indistinguishable from clean" harm is closed on both paths. Pinned by
    test_single_repo_refusal_is_reported_on_the_response.

  • Omit the top-level files mirror from multi-repo answersrebutted, on proportionality
    rather than on the count being wrong. Three reasons:

    1. The flat list is not a compat mirror for a hypothetical consumer — it is the shape a consumer
      that does not care about grouping needs, and per-row repoRoot exists precisely to make it
      resolvable without the grouping. A second consumer of exactly that shape is already written
      (the workspace file-tree's changed-lane mapping, which maps files[] to tree rows and takes
      f.repoRoot per row); dropping files would leave it blank for the very workspaces this PR
      exists to fix.
    2. Server-side there is no duplication at all: both lists hold the same row objects, which is
      the invariant that makes one redaction pass cover both. The cost is wire bytes only.
    3. That cost is bounded by the same 500-row cap and only doubles at the cap — a workspace at the
      cap serialises ~60KB twice. Real, but not a trade I would make against forcing every consumer
      to flatten a grouped structure to answer "what changed here".

    If you would still rather have the single canonical list, say so and I will drop files from
    multi answers, sum repos[].files for fileCount, and move the row redaction into the group
    walk — it is a contained change, I just do not think it is the right default.

  • /api/project/git sibling unmentionedfixed (in the previous round, before this review
    ran): the PR body's Not covered section now names both upward-only siblings, and explains why
    the branch-label endpoint answering {"repo": false} for a workspace root is correct rather than
    deferred — a project covering several repos has no single checked-out branch, so promoting one
    sibling's into a gateway-wide label would misreport the rest.

Also cleaned up: my round-1 disposition comment posted twice (a tool call reported as cancelled had
actually landed); the duplicate is deleted.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 16, 2026
@krishdhasmana
krishdhasmana force-pushed the feat/multi-repo-git-status branch from 803c417 to 12228fc Compare August 16, 2026 19:45
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 16, 2026
@krishdhasmana

Copy link
Copy Markdown
Contributor Author

fixed in 12228fc8c.

BLOCKING — src/kiro_crew/dashboard/handlers/files.py:3286 — Cached repo roots bypass project
authorization. Cached child replaced by an outside-project symlink -> _one runs Git there ->
outside branch and filenames are exposed.

Legitimate, and I verified it was exploitable rather than taking it on faith. The scan does refuse a
symlinked entry (entry.is_dir(follow_symlinks=False)), but that is a scan-time control and its
result is cached for _REPO_SCAN_TTL_SECONDS, so the cache is exactly what defeats it — and the same
window exists between the scan and the spawn within a single request.

Reproduced by neutralising the new check and running the regression test: with the gate off the
response carries branch: 'secret-branch' and files: [{path: 'confidential.txt', ...}] from a
repository outside the project, under the in-project group name src/PkgA. So this was a real
information disclosure through an endpoint whose whole authorization story is "the path is one of the
gateway's known project directories", not a theoretical path.

_verify_descendant_roots now re-resolves every descendant root immediately before collection:
realpath containment under the project dir, os.path.islink refused outright, is_sensitive_path
refused, and a non-directory refused. A dropped root also invalidates the cached scan entry (the tree
moved under it) and marks the answer partial via reposTruncated, so a repo vanishing from the list
is visible rather than silent. Ancestor resolution is untouched — that root comes from git's own
resolution of the already-allow-listed base and is above it by construction.

Two tests pin it:

  • test_cached_root_swapped_for_an_outside_symlink_is_refused — warms the cache against a real
    in-project repo, swaps the directory for a symlink to an outside repo with a distinctive branch and
    filename, and asserts neither appears anywhere in the response. This is the test I ran against the
    neutralised gate to confirm it fails without the fix.
  • test_a_symlinked_repo_inside_the_project_is_still_refused — a symlink whose target is inside the
    project is refused too, so a repo cannot be reported twice under two names.

One residual I would rather state than paper over: a sub-millisecond TOCTOU remains between the check
and the git spawn, since the argument is a path and not a directory handle. Closing that completely
needs O_PATH/dirfd plumbing through the sandboxed spawn helper, which is a wider change than this
PR; the check removes the cache-widened window, which is what made it reachable.

72 backend tests pass, plus flake8, mypy, tsc, build. The spec is updated in the same commit.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Aug 16, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 25, 2026
@krishdhasmana

Copy link
Copy Markdown
Contributor Author

fixed in e1782d63a — and the finding is correct, including the part I had reasoned myself out of.

BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:3960 -- Descendant repository can redirect Git
outside the project. Contained repo with core.worktree=/outside -> descendant collection skips
effective-worktree validation -> outside branch and filenames are exposed.

I built the attack before changing anything: a real repo at <ws>/src/PkgA with a genuinely contained
.git, one git config core.worktree /outside, and an outside tree holding confidential.txt on
secret-branch. The pre-fix answer:

'files': [{'path': 'confidential.txt', 'status': '?', 'repoRoot': '…/ws/src/PkgA'}]

So it is worse than "filenames are exposed": the leaked row is stamped with the contained
repoRoot, so every consumer — the panel and the workspace tree's changed lane — presents an outside
file as living inside the project. One correction to the report: the branch does not leak in this
shape (trunk, not secret-branch), because HEAD is read from the contained git dir; it is the file
list that comes from elsewhere. That makes it a disclosure-plus-mislabel bug rather than a
whole-repository disclosure, which does not change the severity.

Why my existing gate could not catch it, stated plainly because it is the useful part: every check I
had — realpath containment, symlink refusal, _git_dir_is_contained — reasons about the
filesystem, and core.worktree is a configuration fact. .git really is where it should be.
The fix is your prescription, and it is the same shape as the one I applied to _git_dir_is_contained
earlier in this PR: ask git where it actually operates rather than testing a form.
git rev-parse --show-toplevel per discovered repo, refused unless its realpath equals the discovered
root — which collapses core.worktree, GIT_WORK_TREE and worktree pointers together, so the next
spelling needs no new check. The cost is one extra rev-parse per repo, bounded by
_REPO_SCAN_MAX_REPOS (12) at concurrency 6, against collections that already spawn status.

The repo is refused rather than silently emptied, which the panel now surfaces as a visible skipped
label thanks to the UX round earlier in this PR. That made the notice string wrong, so it is now
cause-agnostic across all 13 catalogs — "this repository's git configuration is unsafe to read" —
rather than naming filters, which was only one of the two causes.

Not covered, deliberately: a project dir that is itself a repo configured this way still reports
what git says. That is the pre-existing upward-resolve path on main, and arguably correct — the user
named that directory. The new trust boundary is repos this PR discovers without being asked, which
is what the gate covers.

78 backend tests (the exploit test fails against the previous code with the leak above), 49 frontend
tests, flake8, isort, mypy, tsc -b, eslint, build, i18n 18/18 and docs-lint green. Rebased onto
current main; screenshots re-pinned to e1782d63a.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 25, 2026
@krishdhasmana
krishdhasmana force-pushed the feat/multi-repo-git-status branch from e1782d6 to 5a4c8ce Compare August 25, 2026 07:53
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 25, 2026
@krishdhasmana

Copy link
Copy Markdown
Contributor Author

fixed in 5a4c8cedc. The finding is correct and it defeats the gate I added last round — worth
being explicit about why, because the mistake is instructive.

BLOCKING -- files.py:4138 -- Descendant root can change after authorization. Root swapped after
verification but before git status -> _run follows the replacement.

My check was realpath(toplevel) != realpath(known_root). Both sides resolve the same path string at
the same moment
, so a root replaced by a symlink to /outside after verification resolves to
/outside on both sides and the comparison agrees with itself. It proved "git operates where this
path points now", which is exactly not the question; the question is whether git operates where
verification approved.

Proven before fixing, by patching the module-local _verify_descendant_roots to swap the directory in
the window it returns through — verification passes, then src/PkgA becomes a symlink to an outside
repo. Pre-fix that leaked confidential.txt.

The fix is the identity stability your prescription asks for: _verify_descendant_roots already
computed the approved realpath for its containment test and threw it away, so it now returns
(root, approved_real) pairs and _run compares git's toplevel against that stored value. The
window still exists — it always will for a path-based check followed by a spawn — but it is no longer
exploitable for the class that matters: a swap now makes the comparison disagree and the repo is
refused, instead of making it agree.

On the prescribed remedy — "revert descendant collection until verification and Git execution share
an identity-stable root" — I have implemented the stability rather than the revert, because the two
halves were already one small step apart: the approved realpath existed at the verification site and
only needed carrying forward. Reverting the collection would remove the feature this PR exists for to
solve a problem that a value already in hand fixes. If you consider a realpath captured at
verification insufficient (an fd-anchored spawn being the only truly atomic form), say so and I will
take that up — but that is not portable to the Windows path this endpoint also serves, and it would be
a change to how every git call in this module is made rather than to this feature.

Both exploit tests discriminate their own fix: with the identity anchor reverted the swap test fails
and the core.worktree test still passes, so neither is standing in for the other.

79 backend tests, 49 frontend tests, flake8, isort, mypy, the black gate, tsc -b, eslint, build,
i18n 18/18 and docs-lint green. Rebased onto current main; screenshots re-pinned to 5a4c8cedc.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 25, 2026
… 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.
@krishdhasmana
krishdhasmana force-pushed the feat/multi-repo-git-status branch from 5a4c8ce to 5cb712f Compare August 25, 2026 08:17
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 25, 2026
@krishdhasmana

Copy link
Copy Markdown
Contributor Author

acknowledged — real, and I am escalating rather than patching it a third time. This is the third
blocking finding on the same file:function span, and each one has been the same shape one step
deeper: verify → --show-toplevelstatus. That is not a coincidence, it is the structure of the
problem, so it deserves a decision rather than another round.

What I accept. The window is real. Any path-based check followed by a subprocess has one, and
moving the check closer to the spawn only shortens it — after status there is numstat, and after
that the next call. I could add a fourth check and you would correctly file a fifth finding.

What I could not find: a portable way to close it. Closing it needs the spawn anchored to an
identity a rename cannot follow, i.e. a file descriptor:

  • subprocess takes cwd=<str>, not a directory fd. Reaching an fd means preexec_fn with
    os.fchdir, which is unsafe here — these collections run in asyncio.to_thread, and
    preexec_fn in a multi-threaded process is exactly the footgun CPython documents.
  • --git-dir=/proc/self/fd/N works, on Linux only. This endpoint also serves macOS and native
    Windows, neither of which has procfs or an equivalent.

So the options are a Linux-only path with a silently weaker guarantee on the other two platforms, or
accepting the window. I am not going to ship the former quietly.

Where this differs from your first two findings, which I fixed without argument. Those were
exploitable by static content: clone a repo whose config names a filter driver, or whose committed
.git pointer or core.worktree aims outside, and reading the panel leaks. No attacker presence
needed — I demonstrated both, and the core.worktree one leaked an outside filename stamped with a
contained repoRoot. This one needs a concurrent local writer with write access to the user's own
project directory
, hitting a sub-millisecond window. An attacker who already has that can drop a
file in the project, edit .git/config, or install a git hook — all of which beat this endpoint
without needing to win a race. The precondition is strictly stronger than the controls it would
bypass.

It is also not specific to this PR. /api/project/git and /api/project/tree on main resolve a
path and then act on it with the same window. Reverting descendant collection would remove this
feature while leaving that property in place everywhere else.

I am handing the call to the repository owner rather than deciding it myself, since the prescription —
"revert descendant collection" — is a product decision, not a code fix. Three ways forward, and I will
implement whichever is chosen:

  1. Harden what is portably reachable: pass explicit --git-dir / --work-tree from the verified
    realpath so command-line arguments beat repo config. That kills the configuration-redirection class
    at the source and drops a spawn. It does not close the timing race, and I would not describe it
    as if it did.
  2. Accept the window with a writer override, on the reasoning above.
  3. Revert the descendant scan, per the prescription, and the multi-repo panel goes with it.

Separately: the two Windows shards were red on my own new tests (POSIX escape mechanics — os.symlink
needs elevation there, and a POSIX-shaped core.worktree value is not what that redirection looks like
on Windows). Both are now skipif(os.name == "nt"), matching the existing shape in that file.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Aug 25, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • This PR is OVERLAPPING with PR #6905. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #3987: REBASE. Both are correct alone and coupled once both land: the new path-shaped fields must adopt whichever path-aware redactor wins, so ordering and follow-up belong in one discussion. Files: src/kiro_crew/dashboard/handlers/files.py.
  • This PR is OVERLAPPING with PR #7436. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #3987: REBASE. Complementary, not competing: PR #7436 adds a consumer of the endpoint this PR reshapes. A rebase must re-check the footer badge against the grouped answer and decide whether 0/0 ahead/behind is the intended reading for a multi-repo project. Files: website/src/pages/ChatPage.tsx.
  • This PR is OVERLAPPING with PR #7678. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #3987: REBASE. Merged code now owns the block this PR extends, and the two interact on more than text: the rebase has to re-establish the one-pass-covers-every-row invariant on top of main's dedupe rebind. Files: src/kiro_crew/dashboard/handlers/files.py.
  • This PR is OVERLAPPING with PR #8055. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #3987: REBASE. Same coupling as PR #6905 on the same line; the set of path-shaped fields this PR introduces should be settled together with whichever path-display redactor is adopted. Files: src/kiro_crew/dashboard/handlers/files.py.
  • PR #5890 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5890: CONTINUE_DEVELOPMENT. Independent features that happen to grow a second filesystem-discovery walker. Both can land as-is; worth flagging so a later change can factor one bounded walk out rather than maintaining two sets of caps and containment rules. Files: src/kiro_crew/project_scan.py.
  • PR #7181 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7181: KEEP. Complementary halves of multi-repo work: 7181 supplies and names the repositories, 3987 makes git status legible across them. Worth a note on 7181 that Project sibling checkouts stay outside the Git panel until 3987 (or an equivalent) lands. Files: src/kiro_crew/dashboard/handlers/files.py, website/src/api/client.ts.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@chenmingwei23

Copy link
Copy Markdown
Contributor

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 # A submodule's status belongs to its parent. reads as the correct call to us, not an oversight. The submodule feature can layer on top by discovering submodules and running your per-repo status path inside each one, rather than by changing this exclusion.

@bolichen97

Copy link
Copy Markdown
Collaborator

@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 src/kiro_crew/dashboard/handlers/files.py as this one, so the three should be sequenced rather than merged independently.

#6905 (@leonlaiyc) adds _redact_project_path, a Darwin /private/var/folders/<2>/<30>/T prefix exemption, and routes api_project_git_status's repoRoot through it. #8055 (@jeeshofone) adds _redact_path_display, a segment-wise redactor, and routes the same repoRoot plus files[].path through it. Those two are competing wrappers for the same values, so only one can be canonical in this module.

This PR is the one that multiplies the fields the winner has to cover: it adds files[].repoRoot, repos[].root and repos[].name to that block on bare redact(). Landing it alone reintroduces the false positive the other two exist to fix, one field at a time. Note that #8055 argues a branch name must stay on whole-string redact() because it is free-form text; that applies verbatim to your repos[].branch.

Conflicts are certain, not just possible: #6905 and this PR both add a test to TestGitStatus in test/test_project_git_status_log.py and both rewrite the same "Project git status" paragraph of docs/system-specs/modules/learn-cron-dashboard.md.

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 mergeable_state=dirty and still parked on an owner decision for the last blocking review finding.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants