From 39245f2fa1727e7dd6d92f1d49d438a66c7edd9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 23:30:27 +0200 Subject: [PATCH 1/2] Wire hierarchical team grouping into the CLI end-to-end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - github-code-search.ts: --group-by-team-prefix now parses a chain grammar (/ for nesting depth, , for independent chains) via the new parseTeamPrefixChains, and always uses groupByTeamHierarchy (a 1-level chain behaves identically to the old flat groupByTeamPrefix, covered by parity tests). Added --group-by-team-prefix-consolidate to apply consolidateTeamHierarchy. --pick-team now resolves through the new resolvePickTeamAssignment (bare-label auto-resolve when unambiguous, or an explicit "parent > combined" path), replacing ~90 lines of inline validation with pure, tested logic. - src/group.ts: added parseTeamPrefixChains and resolvePickTeamAssignment (pure, fully unit-tested CLI-parsing helpers). - src/tui.ts: simplified pick/re-pick/undo handlers to always use the tree-aware functions (rebuildTeamHierarchy/applyTeamPickInTree/etc.), since the CLI now always produces sectionPath-tagged groups — the previous sectionPath.length/pickedFrom-based branching to the old flat functions was dead code that would have silently wiped all groups for a depth-1 (top-level) section pick (rebuildTeamSections finds no sectionLabel-tagged group, applyTeamPick no-ops on an empty array). Caught via a manual end-to-end smoke test before it could ship. Fixed along the way: a pre-existing Commander help-formatting bug where any option description containing a newline followed by whitespace (e.g. an indented example line) made Help.preformatted() treat the WHOLE description as manually formatted and skip aligning continuation lines to the option column — affected --exclude-repositories and --exclude-extracts too, not just the new options. Also removed a duplicated "(default: false)" on --include-archived/ --exclude-template-repositories (Commander already appends it). Closes #182 --- github-code-search.ts | 161 ++++++++++++++++++------------------------ src/group.test.ts | 135 +++++++++++++++++++++++++++++++++++ src/group.ts | 129 +++++++++++++++++++++++++++++++++ src/tui.ts | 61 ++++++---------- 4 files changed, 351 insertions(+), 135 deletions(-) diff --git a/github-code-search.ts b/github-code-search.ts index fbebdc4..b0eb335 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -20,7 +20,15 @@ import { aggregate, normaliseExtractRef, normaliseRepo } from "./src/aggregate.t import { fetchAllResults, fetchRepoTeams } from "./src/api.ts"; import { formatRetryWait } from "./src/api-utils.ts"; import { buildOutput } from "./src/output.ts"; -import { groupByTeamPrefix, flattenTeamSections, applyTeamPick } from "./src/group.ts"; +import { + applyTeamPickInTree, + consolidateTeamHierarchy, + findCombinedSectionPaths, + flattenTeamHierarchy, + groupByTeamHierarchy, + parseTeamPrefixChains, + resolvePickTeamAssignment, +} from "./src/group.ts"; import { checkForUpdate } from "./src/upgrade.ts"; import { runInteractive } from "./src/tui.ts"; import { generateCompletion, detectShell } from "./src/completions.ts"; @@ -66,7 +74,11 @@ function colorDesc(s: string): string { return style.dim(docsMatch[1]) + style.style(["cyan", "underline"], docsMatch[2]); const exampleMatch = line.match(/^(\s*Example:\s*)(.+)$/); if (exampleMatch) return style.dim(exampleMatch[1]) + style.italic(exampleMatch[2]); - if (/^\s+(e\.g\.|repoA|myorg\/|squad-|chapter-)/.test(line)) return style.dim(line); + // Fix: match with or without leading whitespace — a leading space here + // would make Commander's Help.preformatted() (newline followed by + // whitespace) treat the WHOLE description as already manually indented + // and skip aligning continuation lines to the option column. + if (/^\s*(e\.g\.|repoA|myorg\/|squad-|chapter-|gamme-)/.test(line)) return style.dim(line); // Colorize any remaining bare URL (http/https) anywhere in the line return line.replace(/(https?:\/\/\S+)/g, (url) => style.style(["cyan", "underline"], url)); }) @@ -126,7 +138,7 @@ function addSearchOptions(cmd: Command): Command { [ "Comma-separated list of repositories to exclude.", "Short form (without org prefix) or full form accepted:", - " repoA,repoB OR myorg/repoA,myorg/repoB", + "repoA,repoB OR myorg/repoA,myorg/repoB", "Docs: https://fulll.github.io/github-code-search/usage/filtering", ].join("\n"), "", @@ -136,7 +148,7 @@ function addSearchOptions(cmd: Command): Command { [ "Comma-separated extract refs to exclude.", "Format (shortest): repoName:path:matchIndex", - " e.g. repoA:src/foo.ts:0,repoB:lib/core.ts:2", + "e.g. repoA:src/foo.ts:0,repoB:lib/core.ts:2", "Full form also accepted: myorg/repoA:src/foo.ts:0", "Docs: https://fulll.github.io/github-code-search/usage/filtering", ].join("\n"), @@ -162,33 +174,40 @@ function addSearchOptions(cmd: Command): Command { ].join("\n"), "repo-and-matches", ) - .option( - "--include-archived", - "Include archived repositories in results (default: false)", - false, - ) - .option( - "--exclude-template-repositories", - "Exclude template repositories from results (default: false)", - false, - ) + .option("--include-archived", "Include archived repositories in results", false) + .option("--exclude-template-repositories", "Exclude template repositories from results", false) .option( "--group-by-team-prefix ", [ "Comma-separated team-name prefixes used to group result repos by GitHub team.", - "Example: squad-,chapter-", - "Repos are first grouped by the first prefix (single-team, then multi-team),", - "then by the next prefix, and so on. Repos matching no prefix go into 'other'.", + "Use / within one entry to nest levels: gamme-/squad- groups by gamme- first,", + "then sub-groups each section by squad-. Combine independent chains with ,:", + "gamme-/squad-,chapter-", + "Repos are first grouped by single-team match, then multi-team, then the next", + "level. Repos matching no prefix go into 'other'. Team names that overlap", + "(e.g. squad-a and squad-a-legacy) are nested automatically.", "Docs: https://fulll.github.io/github-code-search/usage/team-grouping", ].join("\n"), "", ) + .option( + "--group-by-team-prefix-consolidate", + [ + "Collapse unambiguous single-branch nesting chains into one heading", + 'with an "(including ...)" suffix instead of one heading per level.', + "Only applies with --group-by-team-prefix.", + "Docs: https://fulll.github.io/github-code-search/usage/team-grouping", + ].join("\n"), + false, + ) .option( "--pick-team ", [ "Assign a combined team section to a single owner.", 'Format: "combined label"=chosenTeam (the = separator is required).', 'Example: --pick-team "squad-frontend + squad-mobile"=squad-frontend', + "The combined label may be unqualified (auto-resolved when unambiguous)", + 'or a full path when nested / ambiguous: "gamme-client > squad-a + squad-b"=squad-a', "Repeatable — one flag per combined section to resolve.", "Only applies with --group-by-team-prefix.", "Docs: https://fulll.github.io/github-code-search/usage/team-grouping#team-pick-mode", @@ -224,6 +243,7 @@ async function searchAction( includeArchived: boolean; excludeTemplateRepositories: boolean; groupByTeamPrefix: string; + groupByTeamPrefixConsolidate?: boolean; pickTeam: string[]; cache: boolean; regexHint?: string; @@ -348,107 +368,58 @@ async function searchAction( // ─── Team-prefix grouping ───────────────────────────────────────────────── const pickTeams: Record = {}; if (!opts.groupByTeamPrefix && opts.pickTeam && opts.pickTeam.length > 0) { - // Emit per-assignment warnings (same validation as when grouping is enabled) — see issue #121. for (const assignment of opts.pickTeam) { - const eqIndex = assignment.indexOf("="); - if (eqIndex === -1) { - process.stderr.write( - `warning: --pick-team "${assignment}" is missing the = separator; skipping\n`, - ); - continue; - } - const combined = assignment.slice(0, eqIndex).trim(); - const chosen = assignment.slice(eqIndex + 1).trim(); - if (!combined || !chosen) { - process.stderr.write( - `warning: --pick-team "${assignment}" must have non-empty combined and chosen labels; skipping\n`, - ); - continue; - } process.stderr.write( - `warning: --pick-team: no section found with label "${combined}"\n (no combined sections remain)\n`, + `warning: --pick-team "${assignment}" requires --group-by-team-prefix; skipping\n`, ); } } if (opts.groupByTeamPrefix) { - const prefixes = opts.groupByTeamPrefix - .split(",") - .map((p) => p.trim()) - .filter(Boolean); - if (prefixes.length > 0) { - const teamMap = await fetchRepoTeams(org, GITHUB_TOKEN!, prefixes, opts.cache, onRateLimit); + const { chains, warnings: chainWarnings } = parseTeamPrefixChains(opts.groupByTeamPrefix); + for (const w of chainWarnings) process.stderr.write(`warning: ${w}\n`); + + if (chains.length > 0) { + const allPrefixes = [...new Set(chains.flat())]; + const teamMap = await fetchRepoTeams( + org, + GITHUB_TOKEN!, + allPrefixes, + opts.cache, + onRateLimit, + ); // Attach team lists to each group for (const g of groups) { g.teams = teamMap.get(g.repoFullName) ?? []; } - let sections = groupByTeamPrefix(groups, prefixes); + + let sections = groupByTeamHierarchy(groups, chains); + if (opts.groupByTeamPrefixConsolidate) { + sections = consolidateTeamHierarchy(sections); + } + // Apply --pick-team assignments before flattening. - // Fix: detect non-matching picks and warn on stderr so the user can correct labels. for (const assignment of opts.pickTeam) { - const eqIndex = assignment.indexOf("="); - if (eqIndex === -1) { - process.stderr.write( - `warning: --pick-team "${assignment}" is missing the = separator; skipping\n`, - ); + const resolution = resolvePickTeamAssignment(sections, assignment); + if ("error" in resolution) { + process.stderr.write(`warning: ${resolution.error}\n`); continue; } - const combined = assignment.slice(0, eqIndex).trim(); - const chosen = assignment.slice(eqIndex + 1).trim(); - if (!combined || !chosen) { - process.stderr.write( - `warning: --pick-team "${assignment}" must have non-empty combined and chosen labels; skipping\n`, - ); - continue; - } - // Fix: require the combined label to be a multi-team label (must contain " + ") — see issue #121. - const combinedCandidates = combined - .split(" + ") - .map((part) => part.trim()) - .filter((part) => part.length > 0); - if (combinedCandidates.length < 2) { - process.stderr.write( - `warning: --pick-team "${assignment}" has combined label "${combined}" which is not a multi-team section; skipping\n`, - ); - continue; - } - if (!combinedCandidates.includes(chosen)) { - process.stderr.write( - `warning: --pick-team "${assignment}" has chosen label "${chosen}" which is not one of the teams in ` + - `"${combined}". Allowed choices: ${combinedCandidates.map((c) => `"${c}"`).join(", ")}; skipping\n`, - ); - continue; - } - const updated = applyTeamPick(sections, combined, chosen); - if (updated === sections) { - // applyTeamPick returns the same reference when the combined label is not found. - const available = sections - .map((s) => s.label) - .filter((l) => l.includes(" + ")) - .map((l) => ` "${l}"`) - .join("\n"); - process.stderr.write( - `warning: --pick-team: no section found with label "${combined}"\n` + - (available - ? ` Available combined sections:\n${available}\n` - : " (no combined sections remain)\n"), - ); - } else { - sections = updated; - pickTeams[combined] = chosen; - } + sections = applyTeamPickInTree(sections, resolution.path, resolution.chosen); + pickTeams[resolution.path.join(" > ")] = resolution.chosen; } + // Warn about combined sections that still have no pick assigned, so the user // knows which labels to add to the next replay command or interactive session. - const unresolved = sections.filter((s) => s.label.includes(" + ")); + const unresolved = findCombinedSectionPaths(sections); if (unresolved.length > 0 && opts.pickTeam.length > 0) { process.stderr.write( `note: ${unresolved.length} combined section${unresolved.length !== 1 ? "s" : ""} still unresolved ` + `(press "p" in TUI or use --pick-team to assign):\n` + - unresolved.map((s) => ` "${s.label}"`).join("\n") + + unresolved.map((p) => ` "${p.join(" > ")}"`).join("\n") + "\n", ); } - groups = flattenTeamSections(sections); + groups = flattenTeamHierarchy(sections); } } @@ -458,6 +429,7 @@ async function searchAction( includeArchived, excludeTemplates, groupByTeamPrefix: opts.groupByTeamPrefix, + consolidateTeamSections: opts.groupByTeamPrefixConsolidate, regexHint: opts.regexHint, pickTeams: Object.keys(pickTeams).length > 0 ? pickTeams : undefined, }), @@ -518,6 +490,7 @@ async function searchAction( includeArchived, excludeTemplates, opts.groupByTeamPrefix, + Boolean(opts.groupByTeamPrefixConsolidate), opts.regexHint ?? "", Object.keys(pickTeams).length > 0 ? pickTeams : {}, ); diff --git a/src/group.test.ts b/src/group.test.ts index fef878d..c587cb9 100644 --- a/src/group.test.ts +++ b/src/group.test.ts @@ -12,6 +12,8 @@ import { moveRepoToSectionInTree, rebuildTeamHierarchy, rebuildTeamSections, + parseTeamPrefixChains, + resolvePickTeamAssignment, undoPickedRepo, undoPickedRepoInTree, undoSectionPick, @@ -1327,3 +1329,136 @@ describe("undoSectionPick", () => { expect(inCombined).toContain("org/repoC"); }); }); + +// ─── parseTeamPrefixChains ────────────────────────────────────────────────────── + +describe("parseTeamPrefixChains", () => { + it("parses a single flat prefix into a 1-level chain", () => { + expect(parseTeamPrefixChains("squad-")).toEqual({ chains: [["squad-"]], warnings: [] }); + }); + + it("parses comma-separated prefixes into independent 1-level chains", () => { + expect(parseTeamPrefixChains("squad-,chapter-")).toEqual({ + chains: [["squad-"], ["chapter-"]], + warnings: [], + }); + }); + + it("parses a slash-separated chain into a multi-level chain", () => { + expect(parseTeamPrefixChains("gamme-/squad-")).toEqual({ + chains: [["gamme-", "squad-"]], + warnings: [], + }); + }); + + it("parses a mix of a 2-level chain and an independent 1-level chain", () => { + expect(parseTeamPrefixChains("gamme-/squad-,chapter-")).toEqual({ + chains: [["gamme-", "squad-"], ["chapter-"]], + warnings: [], + }); + }); + + it("trims whitespace around prefixes and levels", () => { + expect(parseTeamPrefixChains(" gamme- / squad- , chapter- ")).toEqual({ + chains: [["gamme-", "squad-"], ["chapter-"]], + warnings: [], + }); + }); + + it("drops an empty chain from a leading, trailing, or double comma, with a warning", () => { + const { chains, warnings } = parseTeamPrefixChains(",squad-,,chapter-,"); + expect(chains).toEqual([["squad-"], ["chapter-"]]); + expect(warnings.length).toBeGreaterThan(0); + expect(warnings.every((w) => w.includes("empty chain segment"))).toBe(true); + }); + + it("drops an empty level from a leading, trailing, or double slash, with a warning", () => { + const { chains, warnings } = parseTeamPrefixChains("/gamme-//squad-/"); + expect(chains).toEqual([["gamme-", "squad-"]]); + expect(warnings.length).toBeGreaterThan(0); + expect(warnings[0]).toContain("empty prefix level"); + }); + + it("returns no chains and no warnings for an empty string", () => { + // Not a realistic CLI input (the caller checks truthiness first), but + // must not throw. + expect(parseTeamPrefixChains("")).toEqual({ + chains: [], + warnings: ['--group-by-team-prefix: ignoring empty chain segment in ""'], + }); + }); +}); + +// ─── resolvePickTeamAssignment ────────────────────────────────────────────────── + +describe("resolvePickTeamAssignment", () => { + it("resolves a bare label that is unambiguous in the tree", () => { + const groups = [makeGroup("org/a", ["squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + const result = resolvePickTeamAssignment(tree, "squad-a + squad-b=squad-a"); + expect(result).toEqual({ path: ["squad-a + squad-b"], chosen: "squad-a" }); + }); + + it("resolves a nested bare label by finding it anywhere in the tree", () => { + const groups = [makeGroup("org/a", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const result = resolvePickTeamAssignment(tree, "squad-a + squad-b=squad-a"); + expect(result).toEqual({ path: ["gamme-client", "squad-a + squad-b"], chosen: "squad-a" }); + }); + + it("accepts an explicit fully-qualified path (parent > combined)", () => { + const groups = [makeGroup("org/a", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const result = resolvePickTeamAssignment(tree, "gamme-client > squad-a + squad-b=squad-b"); + expect(result).toEqual({ path: ["gamme-client", "squad-a + squad-b"], chosen: "squad-b" }); + }); + + it("errors when the = separator is missing", () => { + const tree = groupByTeamHierarchy([makeGroup("org/a", ["squad-a"])], [["squad-"]]); + const result = resolvePickTeamAssignment(tree, "squad-a + squad-b"); + expect("error" in result).toBe(true); + expect((result as { error: string }).error).toContain("missing the ="); + }); + + it("errors when the combined or chosen side is empty", () => { + const tree = groupByTeamHierarchy([makeGroup("org/a", ["squad-a"])], [["squad-"]]); + const result = resolvePickTeamAssignment(tree, "=squad-a"); + expect("error" in result).toBe(true); + }); + + it("errors with the available combined sections when the bare label is not found", () => { + const groups = [makeGroup("org/a", ["squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + const result = resolvePickTeamAssignment(tree, "squad-x + squad-y=squad-x"); + expect("error" in result).toBe(true); + expect((result as { error: string }).error).toContain("squad-a + squad-b"); + }); + + it("errors when the bare label is ambiguous across multiple branches", () => { + const groups = [ + makeGroup("org/a", ["gamme-x", "squad-a", "squad-b"]), + makeGroup("org/b", ["gamme-y", "squad-a", "squad-b"]), + ]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"], ["gamme-"]]); + const result = resolvePickTeamAssignment(tree, "squad-a + squad-b=squad-a"); + expect("error" in result).toBe(true); + expect((result as { error: string }).error).toContain("ambiguous"); + }); + + it("errors when the combined label is not a multi-team section", () => { + const groups = [makeGroup("org/a", ["gamme-client", "squad-a"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + // Explicit path pointing at a genuine (non-combined) section. + const result = resolvePickTeamAssignment(tree, "gamme-client > squad-a=squad-a"); + expect("error" in result).toBe(true); + expect((result as { error: string }).error).toContain("not a multi-team section"); + }); + + it("errors when the chosen team is not one of the combined candidates", () => { + const groups = [makeGroup("org/a", ["squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["squad-"]]); + const result = resolvePickTeamAssignment(tree, "squad-a + squad-b=squad-c"); + expect("error" in result).toBe(true); + expect((result as { error: string }).error).toContain("Allowed choices"); + }); +}); diff --git a/src/group.ts b/src/group.ts index 205f722..797220b 100644 --- a/src/group.ts +++ b/src/group.ts @@ -752,6 +752,135 @@ export function findCombinedSectionPaths(sections: TeamSection[]): string[][] { return paths; } +// ─── CLI option parsing (pure) ───────────────────────────────────────────────── + +/** + * Parses the `--group-by-team-prefix` value into one or more prefix chains + * for `groupByTeamHierarchy`: `,` separates independent chains, `/` separates + * nesting levels within one chain. E.g. `"gamme-/squad-,chapter-"` produces + * `[["gamme-", "squad-"], ["chapter-"]]`. + * + * Malformed segments (empty chain from a stray/leading/trailing/double `,`, + * or an empty level from a stray `/`) are dropped rather than propagated as + * an empty-string prefix, with a human-readable warning for each so the + * caller can surface it on stderr. A chain that has no valid level left + * after cleanup is dropped entirely (also warned). + */ +export function parseTeamPrefixChains(spec: string): { chains: string[][]; warnings: string[] } { + const warnings: string[] = []; + const chains: string[][] = []; + + for (const rawChain of spec.split(",")) { + const rawLevels = rawChain.split("/"); + const levels = rawLevels.map((l) => l.trim()).filter((l) => l.length > 0); + + if (levels.length === 0) { + warnings.push(`--group-by-team-prefix: ignoring empty chain segment in "${spec}"`); + continue; + } + if (levels.length !== rawLevels.length) { + warnings.push( + `--group-by-team-prefix: chain "${rawChain.trim()}" has empty prefix level(s); using "${levels.join("/")}"`, + ); + } + chains.push(levels); + } + + return { chains, warnings }; +} + +/** Successfully resolved `--pick-team` assignment, ready for `applyTeamPickInTree`. */ +export interface ResolvedPickTeam { + path: string[]; + chosen: string; +} + +/** + * Parses and resolves one `--pick-team` assignment (`"combined=chosen"`) + * against the current `sections` tree, returning either the resolved + * `{ path, chosen }` (ready for `applyTeamPickInTree`) or a human-readable + * `error` describing why it was rejected — the caller decides how to surface + * it (e.g. a stderr warning). + * + * The combined side may be: + * - a bare label (e.g. `"squad-a + squad-b"`), auto-resolved via + * `findCombinedSectionPaths` — succeeds only when exactly one match + * exists anywhere in the tree; + * - a fully-qualified path joined with `" > "` (e.g. + * `"gamme-client > squad-a + squad-b"`), used as-is without validating + * against `findCombinedSectionPaths` (so it still resolves correctly + * right after an earlier assignment already changed the tree shape). + * + * `chosen` must be one of the `" + "`-separated candidate teams in the + * resolved combined label. + */ +export function resolvePickTeamAssignment( + sections: TeamSection[], + assignment: string, +): ResolvedPickTeam | { error: string } { + const eqIndex = assignment.indexOf("="); + if (eqIndex === -1) { + return { error: `--pick-team "${assignment}" is missing the = separator; skipping` }; + } + const combinedInput = assignment.slice(0, eqIndex).trim(); + const chosen = assignment.slice(eqIndex + 1).trim(); + if (!combinedInput || !chosen) { + return { + error: `--pick-team "${assignment}" must have non-empty combined and chosen labels; skipping`, + }; + } + + let path: string[]; + if (combinedInput.includes(PATH_SEPARATOR)) { + path = combinedInput.split(PATH_SEPARATOR).map((s) => s.trim()); + } else { + const matches = findCombinedSectionPaths(sections).filter( + (p) => p[p.length - 1] === combinedInput, + ); + if (matches.length === 0) { + const available = findCombinedSectionPaths(sections) + .map((p) => ` "${p.join(PATH_SEPARATOR)}"`) + .join("\n"); + return { + error: + `--pick-team: no section found with label "${combinedInput}"\n` + + (available + ? ` Available combined sections:\n${available}` + : " (no combined sections remain)"), + }; + } + if (matches.length > 1) { + const candidates = matches.map((p) => ` "${p.join(PATH_SEPARATOR)}"`).join("\n"); + return { + error: + `--pick-team: label "${combinedInput}" is ambiguous (found in ${matches.length} places).\n` + + ` Qualify it with the full path, e.g.:\n${candidates}`, + }; + } + path = matches[0]; + } + + const combinedLabel = path[path.length - 1]; + const candidateTeams = combinedLabel + .split(" + ") + .map((c) => c.trim()) + .filter((c) => c.length > 0); + if (candidateTeams.length < 2) { + return { + error: `--pick-team "${assignment}" has combined label "${combinedLabel}" which is not a multi-team section; skipping`, + }; + } + if (!candidateTeams.includes(chosen)) { + return { + error: + `--pick-team "${assignment}" has chosen label "${chosen}" which is not one of the teams in ` + + `"${combinedLabel}". Allowed choices: ${candidateTeams.map((c) => `"${c}"`).join(", ")}; skipping`, + }; + } + + return { path, chosen }; +} + // ─── Internal helpers ───────────────────────────────────────────────────────── /** diff --git a/src/tui.ts b/src/tui.ts index 3dfc5f5..b45a822 100644 --- a/src/tui.ts +++ b/src/tui.ts @@ -14,15 +14,10 @@ import { } from "./render.ts"; import { buildOutput } from "./output.ts"; import { - applyTeamPick, applyTeamPickInTree, flattenTeamHierarchy, - flattenTeamSections, - moveRepoToSection, moveRepoToSectionInTree, rebuildTeamHierarchy, - rebuildTeamSections, - undoSectionPick, undoSectionPickInTree, } from "./group.ts"; import { parseMouseEvent } from "./render/mouse.ts"; @@ -148,6 +143,7 @@ export async function runInteractive( includeArchived = false, excludeTemplates = false, groupByTeamPrefix = "", + consolidateTeamSections = false, regexHint = "", initialPickTeams: Record = {}, ): Promise { @@ -470,23 +466,16 @@ export async function runInteractive( focusedIndex: (teamPickMode.focusedIndex + 1) % teamPickMode.candidates.length, }; } else if (key === KEY_ENTER_CR || key === KEY_ENTER_LF) { - // Enter — confirm pick, reassign repos, exit pick mode + // Enter — confirm pick, reassign repos, exit pick mode. + // Always tree-aware: the CLI only ever produces sectionPath-tagged + // groups (groupByTeamHierarchy), even for a depth-1 (top-level) + // section, so rebuildTeamSections/applyTeamPick (which expect the + // older flat sectionLabel marker) would silently no-op here — see + // issue #182. const chosen = teamPickMode.candidates[teamPickMode.focusedIndex]; - // Fix: branch on sectionPath depth so a pick on a nested - // groupByTeamHierarchy section reassigns within the tree instead of - // (incorrectly) treating the tree as a flat groupByTeamPrefix list — - // see issue #181. A depth-1 path (top-level section) behaves exactly - // like the flat path, since a hierarchy tree's top level is the same - // shape as groupByTeamPrefix's flat sections. - if (teamPickMode.sectionPath.length > 1) { - const sections = rebuildTeamHierarchy(groups); - const updated = applyTeamPickInTree(sections, teamPickMode.sectionPath, chosen); - groups = flattenTeamHierarchy(updated); - } else { - const sections = rebuildTeamSections(groups); - const updated = applyTeamPick(sections, teamPickMode.sectionLabel, chosen); - groups = flattenTeamSections(updated); - } + const sections = rebuildTeamHierarchy(groups); + const updated = applyTeamPickInTree(sections, teamPickMode.sectionPath, chosen); + groups = flattenTeamHierarchy(updated); confirmedPicks[teamPickMode.sectionPath.join(" > ") || teamPickMode.sectionLabel] = chosen; teamPickMode = { active: false, @@ -533,21 +522,14 @@ export async function runInteractive( focusedIndex: (repickMode.focusedIndex + 1) % repickMode.candidates.length, }; } else if (key === KEY_ENTER_CR || key === KEY_ENTER_LF) { - // Enter — confirm re-pick, move repo to the focused candidate team + // Enter — confirm re-pick, move repo to the focused candidate team. + // Always tree-aware — see issue #182 (same reasoning as pick mode above). const targetTeam = repickMode.candidates[repickMode.focusedIndex]; const g = groups[repickMode.repoIndex]; - const pickedFrom = g.pickedFrom ?? ""; - // Fix: a hierarchical pickedFrom ("parent > combined") must move the - // repo within the tree, at the same parent depth it was picked from - // — see issue #181. - if (pickedFrom.includes(" > ")) { - const parentPath = pickedFrom.split(" > ").slice(0, -1); - const sections = rebuildTeamHierarchy(groups); - const updated = moveRepoToSectionInTree(sections, g.repoFullName, parentPath, targetTeam); - groups = flattenTeamHierarchy(updated); - } else { - groups = moveRepoToSection(groups, g.repoFullName, targetTeam); - } + const parentPath = (g.pickedFrom ?? "").split(" > ").slice(0, -1); + const sections = rebuildTeamHierarchy(groups); + const updated = moveRepoToSectionInTree(sections, g.repoFullName, parentPath, targetTeam); + groups = flattenTeamHierarchy(updated); const newRows = buildRows(groups, filterPath, filterTarget, filterRegex); cursor = Math.min(cursor, Math.max(0, newRows.length - 1)); scrollOffset = Math.min(scrollOffset, cursor); @@ -560,13 +542,9 @@ export async function runInteractive( const combinedLabel = groups[repickMode.repoIndex]?.pickedFrom; if (combinedLabel) { delete confirmedPicks[combinedLabel]; - if (combinedLabel.includes(" > ")) { - const sections = rebuildTeamHierarchy(groups); - const updated = undoSectionPickInTree(sections, combinedLabel); - groups = flattenTeamHierarchy(updated); - } else { - groups = undoSectionPick(groups, combinedLabel); - } + const sections = rebuildTeamHierarchy(groups); + const updated = undoSectionPickInTree(sections, combinedLabel); + groups = flattenTeamHierarchy(updated); } const newRows = buildRows(groups, filterPath, filterTarget, filterRegex); cursor = Math.min(cursor, Math.max(0, newRows.length - 1)); @@ -724,6 +702,7 @@ export async function runInteractive( includeArchived, excludeTemplates, groupByTeamPrefix, + consolidateTeamSections, regexHint: regexHint || undefined, pickTeams: Object.keys(confirmedPicks).length > 0 ? confirmedPicks : undefined, }), From 69c3bbb64b6024922445c53510870c95f33926f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 23:58:11 +0200 Subject: [PATCH 2/2] Fix review: pick-team/consolidation ordering, path validation, completions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot review on PR #190: - Reordered CLI wiring so --pick-team assignments resolve against the raw, uncollapsed tree BEFORE --group-by-team-prefix-consolidate runs. Consolidating first changes (or erases) a combined section's label/ path, so pick-team could silently fail to find it or misparse a synthetic "(including ...)" label as candidate teams. - Consolidation is now skipped entirely for --format json (with a stderr warning when requested): JSON is a data contract and must reflect the real, uncollapsed hierarchy in each result's `section` path, not a display-only collapsed view. The replay command and TUI now receive the same consolidateApplied flag actually used, instead of the raw --group-by-team-prefix-consolidate request. - resolvePickTeamAssignment now validates an explicit "parent > combined" path actually resolves to a node in the tree before accepting it — previously a typo'd parent (or a path made stale by an earlier pick) was accepted, applyTeamPickInTree silently no-op'd, and the caller still recorded the assignment for replay as if it had succeeded. - Added the missing --group-by-team-prefix-consolidate entry to the shared shell-completion metadata (bash/zsh/fish) and its tests. The applyTeamPickInTree/tui.ts "children dropped on pick" findings in this review were already fixed in the previous commit on this stack (feat/team-hierarchy-pick-team) — verified still present here. --- github-code-search.ts | 32 +++++++++++++++++++++++++------- src/completions.test.ts | 12 ++++++++++++ src/completions.ts | 6 ++++++ src/group.test.ts | 21 +++++++++++++++++++++ src/group.ts | 30 ++++++++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 7 deletions(-) diff --git a/github-code-search.ts b/github-code-search.ts index b0eb335..beca7f9 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -366,7 +366,10 @@ async function searchAction( ); // ─── Team-prefix grouping ───────────────────────────────────────────────── - const pickTeams: Record = {}; + const pickTeams: Record = {}; // Whether consolidation was actually applied (requested AND not --format + // json, which always needs the full, uncollapsed hierarchy) — forwarded to + // the replay command and the TUI so they stay consistent with `groups`. + let consolidateApplied = false; if (!opts.groupByTeamPrefix && opts.pickTeam && opts.pickTeam.length > 0) { for (const assignment of opts.pickTeam) { process.stderr.write( @@ -393,11 +396,10 @@ async function searchAction( } let sections = groupByTeamHierarchy(groups, chains); - if (opts.groupByTeamPrefixConsolidate) { - sections = consolidateTeamHierarchy(sections); - } - // Apply --pick-team assignments before flattening. + // Apply --pick-team assignments BEFORE consolidation: consolidating + // first would change (or erase) the identity of the combined sections + // --pick-team addresses, silently breaking resolution — see review on #190. for (const assignment of opts.pickTeam) { const resolution = resolvePickTeamAssignment(sections, assignment); if ("error" in resolution) { @@ -419,6 +421,22 @@ async function searchAction( "\n", ); } + + // Consolidation is a display-only concern: JSON output is a data + // contract and must reflect the real, uncollapsed hierarchy (its + // `section` path per result), so skip it entirely for --format json — + // see review on #190. + consolidateApplied = Boolean(opts.groupByTeamPrefixConsolidate) && format !== "json"; + if (opts.groupByTeamPrefixConsolidate && !consolidateApplied) { + process.stderr.write( + "warning: --group-by-team-prefix-consolidate is ignored with --format json " + + "(JSON output always reflects the full, uncollapsed hierarchy)\n", + ); + } + if (consolidateApplied) { + sections = consolidateTeamHierarchy(sections); + } + groups = flattenTeamHierarchy(sections); } } @@ -429,7 +447,7 @@ async function searchAction( includeArchived, excludeTemplates, groupByTeamPrefix: opts.groupByTeamPrefix, - consolidateTeamSections: opts.groupByTeamPrefixConsolidate, + consolidateTeamSections: consolidateApplied, regexHint: opts.regexHint, pickTeams: Object.keys(pickTeams).length > 0 ? pickTeams : undefined, }), @@ -490,7 +508,7 @@ async function searchAction( includeArchived, excludeTemplates, opts.groupByTeamPrefix, - Boolean(opts.groupByTeamPrefixConsolidate), + consolidateApplied, opts.regexHint ?? "", Object.keys(pickTeams).length > 0 ? pickTeams : {}, ); diff --git a/src/completions.test.ts b/src/completions.test.ts index 4ca14d6..93c2464 100644 --- a/src/completions.test.ts +++ b/src/completions.test.ts @@ -31,6 +31,10 @@ describe("generateCompletion", () => { expect(script).toContain("--regex-hint"); }); + it("contains --group-by-team-prefix-consolidate", () => { + expect(generateCompletion("bash")).toContain("--group-by-team-prefix-consolidate"); + }); + it("contains format values (markdown, json)", () => { const script = generateCompletion("bash"); expect(script).toContain("markdown"); @@ -75,6 +79,10 @@ describe("generateCompletion", () => { expect(script).toContain("--regex-hint"); }); + it("contains --group-by-team-prefix-consolidate", () => { + expect(generateCompletion("zsh")).toContain("--group-by-team-prefix-consolidate"); + }); + it("contains a 'compdef' directive (zsh-style)", () => { const script = generateCompletion("zsh"); expect(script).toContain("compdef "); @@ -107,6 +115,10 @@ describe("generateCompletion", () => { expect(script).toContain("regex-hint"); }); + it("contains group-by-team-prefix-consolidate", () => { + expect(generateCompletion("fish")).toContain("group-by-team-prefix-consolidate"); + }); + it("uses fish 'complete -c' syntax", () => { const script = generateCompletion("fish"); expect(script).toContain("complete -c github-code-search"); diff --git a/src/completions.ts b/src/completions.ts index 005ce3e..ba70899 100644 --- a/src/completions.ts +++ b/src/completions.ts @@ -45,6 +45,12 @@ const OPTIONS = [ takesArg: true, values: [], }, + { + flag: "group-by-team-prefix-consolidate", + description: "Collapse single-branch nesting chains into one heading", + takesArg: false, + values: [], + }, { flag: "pick-team", description: "Assign a combined team section to a single owner (repeatable)", diff --git a/src/group.test.ts b/src/group.test.ts index c587cb9..427aeca 100644 --- a/src/group.test.ts +++ b/src/group.test.ts @@ -1413,6 +1413,27 @@ describe("resolvePickTeamAssignment", () => { expect(result).toEqual({ path: ["gamme-client", "squad-a + squad-b"], chosen: "squad-b" }); }); + it("rejects an explicit path whose parent segment doesn't exist in the tree", () => { + const groups = [makeGroup("org/a", ["gamme-client", "squad-a", "squad-b"])]; + const tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + const result = resolvePickTeamAssignment(tree, "wrong-parent > squad-a + squad-b=squad-a"); + expect("error" in result).toBe(true); + expect((result as { error: string }).error).toContain("no combined section found"); + expect((result as { error: string }).error).toContain("gamme-client > squad-a + squad-b"); + }); + + it("rejects an explicit path pointing at a section that no longer exists after an earlier pick", () => { + const groups = [makeGroup("org/a", ["gamme-client", "squad-a", "squad-b"])]; + let tree = groupByTeamHierarchy(groups, [["gamme-", "squad-"]]); + // First pick resolves (and removes) the only combined section. + const first = resolvePickTeamAssignment(tree, "gamme-client > squad-a + squad-b=squad-a"); + if ("error" in first) throw new Error("unexpected error in test setup"); + tree = applyTeamPickInTree(tree, first.path, first.chosen); + // Re-using the same (now stale) explicit path must be rejected, not silently no-op. + const second = resolvePickTeamAssignment(tree, "gamme-client > squad-a + squad-b=squad-a"); + expect("error" in second).toBe(true); + }); + it("errors when the = separator is missing", () => { const tree = groupByTeamHierarchy([makeGroup("org/a", ["squad-a"])], [["squad-"]]); const result = resolvePickTeamAssignment(tree, "squad-a + squad-b"); diff --git a/src/group.ts b/src/group.ts index 797220b..a51de30 100644 --- a/src/group.ts +++ b/src/group.ts @@ -752,6 +752,18 @@ export function findCombinedSectionPaths(sections: TeamSection[]): string[][] { return paths; } +/** Returns whether `path` (root-first ancestor labels) resolves to an actual node in the tree. */ +function pathExistsInTree(sections: TeamSection[], path: string[]): boolean { + let level = sections; + for (let i = 0; i < path.length; i++) { + const node = level.find((s) => s.label === path[i]); + if (!node) return false; + if (i === path.length - 1) return true; + level = node.children ?? []; + } + return path.length === 0; +} + // ─── CLI option parsing (pure) ───────────────────────────────────────────────── /** @@ -833,6 +845,24 @@ export function resolvePickTeamAssignment( let path: string[]; if (combinedInput.includes(PATH_SEPARATOR)) { path = combinedInput.split(PATH_SEPARATOR).map((s) => s.trim()); + // Fix: validate the explicit path actually resolves to a node in the + // current tree — otherwise applyTeamPickInTree silently no-ops while the + // caller still records a bogus assignment for replay — see review on #190. + // (Checked generally, not just against combined sections, so a valid path + // to a non-combined section still falls through to the clearer + // "not a multi-team section" error below instead of this one.) + if (!pathExistsInTree(sections, path)) { + const available = findCombinedSectionPaths(sections) + .map((p) => ` "${p.join(PATH_SEPARATOR)}"`) + .join("\n"); + return { + error: + `--pick-team: no combined section found at path "${combinedInput}"\n` + + (available + ? ` Available combined sections:\n${available}` + : " (no combined sections remain)"), + }; + } } else { const matches = findCombinedSectionPaths(sections).filter( (p) => p[p.length - 1] === combinedInput,