From 7e40b01e2bd11c52144c877afa00d84b3770812b Mon Sep 17 00:00:00 2001 From: Martin Date: Fri, 23 Oct 2026 14:42:25 +0200 Subject: [PATCH] fix(tools): the review threads left open on the merged PRs: local images stay under --image-root, readiness drops a stale 500, typecheck reads quoted paths and the last passing program, the diff hook bounds its blob read and masks command headings, du offers only evictable files, and Hub range links diff from the real parent --- .claude/skills/babysit/SKILL.md | 25 ++--- .claude/skills/babysit/scripts/integrate.ts | 34 +++++-- .../skills/babysit/scripts/review-brief.ts | 6 +- .../skills/babysit/scripts/scripts.test.ts | 58 ++++++++++- docs/benchmarks-du.md | 18 ++++ scripts/ci/typecheck.test.ts | 75 +++++++++++++- scripts/ci/typecheck.ts | 74 ++++++++++---- src/agents/README.md | 11 ++- src/agents/lib/hooks/diff/diff.test.ts | 26 +++++ src/agents/lib/hooks/diff/run.ts | 54 +++++++++-- src/azure-devops/api.ts | 3 +- src/azure-devops/api.types.ts | 6 ++ src/azure-devops/commands/workitem-update.ts | 13 ++- src/azure-devops/config.ts | 4 +- src/azure-devops/lib/work-item-update.test.ts | 48 ++++++++- src/azure-devops/lib/work-item-update.ts | 39 +++++--- src/domains/domains.test.ts | 9 +- src/domains/lib/suggest/online.ts | 9 +- src/du/README.md | 15 +-- src/du/lib/cloud-commands.ts | 59 ++++++++++- src/du/lib/cloud.test.ts | 75 +++++++++++++- src/du/lib/engine.ts | 30 +++++- src/du/lib/evict.ts | 19 ++-- src/du/native/clonesize.c | 19 +++- src/gitlab/lib/review.test.ts | 4 +- src/hub/lib/agents/index.ts | 24 ++++- .../GenesisKit/Controls/GenRenderSwap.swift | 10 +- .../Disk/ToolsDiskProvider.swift | 19 ++-- .../GenesisKitTests/GenRenderSwapTests.swift | 25 +++++ .../Sources/Host/App/WindowCycle.swift | 12 ++- .../Sources/Parts/Hub/Hub/HubCommitLink.swift | 27 ++++-- .../Sources/Parts/Hub/Hub/HubPRs.swift | 94 ++++++++++++------ .../GenesisTools/Tests/HubLogicTests.swift | 97 ++++++++++++++++--- .../GenesisTools/Tests/WindowCycleTests.swift | 24 +++-- .../GenesisTools/web/diff-viewer/main.ts | 4 +- src/macos/commands/clones/reclaim.test.ts | 6 ++ src/macos/commands/clones/reclaim.ts | 18 +++- src/ts/lib/collect.ts | 9 +- src/ts/lib/ts.test.ts | 8 ++ src/utils/DashboardApp/readiness.test.ts | 35 +++++++ src/utils/DashboardApp/readiness.ts | 5 + 41 files changed, 960 insertions(+), 190 deletions(-) diff --git a/.claude/skills/babysit/SKILL.md b/.claude/skills/babysit/SKILL.md index de250c4449..697d09dac4 100644 --- a/.claude/skills/babysit/SKILL.md +++ b/.claude/skills/babysit/SKILL.md @@ -117,8 +117,9 @@ confirmation.) - Rounds 1 and 2 fix every valid finding, Medium and Low included, whatever the PR's size. No deferral, and every file a valid fix needs is in scope. A `no-change` or rejection needs a cited file:line that proves the finding wrong. - - Round 3 and later fix every valid Critical and High. A Medium or Low is fixed only when it is cheap (about 20 - changed lines) or the same class as an earlier round's fix; every other one gets a reply (valid or not, and why + - Round 3 and later fix every valid Critical and High, and CodeRabbit's Major (its level above Minor; there is no + High in its scale). A Medium or Low is fixed only when it is cheap (about 20 changed lines) or the same class as + an earlier round's fix; every other one gets a reply (valid or not, and why it waits) and its thread stays OPEN: resolve only a thread that was acted on. Record such a deferral with the reply transport's `"verdict": "belongs-elsewhere"` and explain the policy in the reply; `integrate.ts` keeps it in the summary but posts it in a separate `post-replies.ts --no-resolve` batch, never the batch that resolves @@ -314,10 +315,12 @@ add the correction to the affected items and to `alsoInPush`, set every item's ` (fable-judge)"`, and render `round-summary.ts --range .. --cwd `. Post per review-loop step 4 only when `post` is true. -## 7a. A PR short of three rounds: an independent review round +## 7a. An independent review round (when the user asks for one) -A round counts only when it fixed or answered real findings. When the reviewer bot has nothing left on a PR that -still needs rounds, or a PR merged with fewer than three, run an independent read-only review: `bun +A round counts only when it fixed or answered real findings. The round policy (step 3.5) sets no minimum number of +rounds: after two rounds with no Critical or High, switch to merge prep (review-loop "Round policy"), and land by the +merge bar. When the user asks for a dedicated review of a PR, before or after its merge, run an independent read-only +review: `bun .claude/skills/babysit/scripts/review-brief.ts --run --pr --wt [--post-merge] [--swift]`, then `steer.ts` with the printed file to a worker that did NOT fix this PR (Sol; Astra only for a risky cross-system PR, by the model rule in step 1). Judge its findings like a bot's. Before a merge they become the next fix round (`fill-brief --judge `). Check with `git range-diff`, run the area tests, give a child that changes plugin files a version above its new base, then push with `--force-with-lease`. A restack is new -code: rerun CI and judge it before the merge; eve need not re-review the last judged fix. Keep at least one -CodeRabbit review and a reply on every thread. When the restack carries a fix round too, push it with +code: rerun CI and judge it before the merge; eve need not re-review the last judged fix. A PR over about 50 +files keeps its CodeRabbit review, and the thread replies of review-loop step 5a still hold. When the restack carries a fix round too, push it with `integrate.ts … --restacked-from `: the lease is that old head, and the gates check only the PR's own commits. ## 7c. Disk: watch it, reclaim /tmp yourself diff --git a/.claude/skills/babysit/scripts/integrate.ts b/.claude/skills/babysit/scripts/integrate.ts index 612ca604ad..8834659264 100644 --- a/.claude/skills/babysit/scripts/integrate.ts +++ b/.claude/skills/babysit/scripts/integrate.ts @@ -23,6 +23,7 @@ import { symlinkSync, writeFileSync, } from "node:fs"; +import { tmpdir } from "node:os"; import { isAbsolute, join, relative, resolve, sep } from "node:path"; import { env } from "@genesiscz/utils/env"; import { SafeJSON } from "@genesiscz/utils/json"; @@ -328,7 +329,10 @@ async function main(): Promise { process.exit(0); } - mkdirSync(run.vault, { recursive: true }); + // --dry is read-only for the run: its reply copy and summary preview go to a scratch folder, so a preview never + // replaces the vault files of a real round. + const outDir = dry ? mkdtempSync(join(tmpdir(), `babysit-integrate-dry-${pr}-r${round}-`)) : run.vault; + mkdirSync(outDir, { recursive: true }); let text = readFileSync(repliesIn, "utf8"); for (const [from, to] of Object.entries(shaMap)) { @@ -373,10 +377,10 @@ async function main(): Promise { // Named from the PR and round, never from the worker's file name: a reply file called `524-r2.json` made the summary // overwrite it, the posted marker point at the summary itself, and the round's summary and replies go unposted // (2026-10-15, #524 round 2). - const replies = join(run.vault, `${pr}-r${round}-replies.json`); + const replies = join(outDir, `${pr}-r${round}-replies.json`); writeFileSync(replies, `${SafeJSON.stringify(data, null, 2)}\n`); - const resolvingReplies = deferred.length ? join(run.vault, `${pr}-r${round}-resolvable.json`) : replies; - const deferredReplies = join(run.vault, `${pr}-r${round}-deferred.json`); + const resolvingReplies = deferred.length ? join(outDir, `${pr}-r${round}-resolvable.json`) : replies; + const deferredReplies = join(outDir, `${pr}-r${round}-deferred.json`); if (deferred.length) { writeFileSync( resolvingReplies, @@ -384,7 +388,7 @@ async function main(): Promise { ); writeFileSync(deferredReplies, SafeJSON.stringify({ ...data, items: deferred }, null, 2)); } - const summary = join(run.vault, `${pr}-r${round}-summary.md`); + const summary = join(outDir, `${pr}-r${round}-summary.md`); const range = flag("--range") ?? (oldHead !== newHead ? `${gateFrom}..${newHead}` : undefined); must( [ @@ -398,11 +402,16 @@ async function main(): Promise { summary, ], "round-summary", - run.vault + outDir ); + if (dry) { + console.log(`dry: preview in ${outDir}, vault untouched, nothing posted: ${summary}`); + process.exit(0); + } + // Check authorization immediately before the first public mutation, after saving the reviewable files. - if (has("--no-post") || dry || !(run.post === true || has("--post"))) { + if (has("--no-post") || !(run.post === true || has("--post"))) { console.log(`held (posting not authorized or disabled): ${summary}`); process.exit(0); } @@ -489,7 +498,12 @@ async function main(): Promise { `${reply.code === 0 ? "✔" : "⚠"} follow-up on ${item.ref ?? item.threadId} (was ${before}, now fixed)` ); } - console.log(posted.code === 0 ? `✔ #${pr} round ${round} posted` : `⚠ #${pr} post-replies exit ${posted.code}`); + const exitCode = postingExitCode(posted.code, [...followups, deferredPosted]); + console.log( + exitCode === 0 + ? `✔ #${pr} round ${round} posted` + : `⚠ #${pr} round ${round} posting incomplete: post-replies exit ${posted.code}, deferred exit ${deferredPosted.code}, ${followups.filter((f) => f.code !== 0).length} follow-up(s) failed` + ); appendEvent(run.dir, { kind: "post", pr: Number(pr), @@ -497,7 +511,7 @@ async function main(): Promise { text: readFileSync(summary, "utf8"), meta: { replies, - exitCode: postingExitCode(posted.code, [...followups, deferredPosted]), + exitCode, postRepliesExitCode: posted.code, followups, postReplies: posted.out @@ -507,7 +521,7 @@ async function main(): Promise { }, }); - process.exitCode = postingExitCode(posted.code, [...followups, deferredPosted]); + process.exitCode = exitCode; } if (import.meta.main) { diff --git a/.claude/skills/babysit/scripts/review-brief.ts b/.claude/skills/babysit/scripts/review-brief.ts index f0469ac96a..6e3ac48e18 100644 --- a/.claude/skills/babysit/scripts/review-brief.ts +++ b/.claude/skills/babysit/scripts/review-brief.ts @@ -1,7 +1,7 @@ #!/usr/bin/env bun /** - * The brief for an independent read-only review round: when the reviewer bot has nothing left on a PR that still - * needs rounds, or a PR merged with fewer than three. Its findings become the next fix round (`fill-brief --judge`) + * The brief for an independent read-only review round, which runs when the user asks for one (SKILL.md 7a): on an + * open PR, or on a PR that already merged. Its findings become the next fix round (`fill-brief --judge`) * or, after a merge, a follow-up PR. Writes `/steer-review-.md` and prints the path for steer.ts. * * bun review-brief.ts --run --pr 517 --wt [--base master] [--post-merge] [--swift] [--fresh] @@ -30,7 +30,7 @@ export interface ReviewBriefInput { export function reviewBrief({ pr, title, wt, base, scratch, postMerge, swift, fresh }: ReviewBriefInput): string { const why = postMerge - ? `PR #${pr} ("${title}") merged into master with fewer than three review rounds. This is an independent review of the MERGED code; your findings become a follow-up PR.` + ? `PR #${pr} ("${title}") already merged into master, and the user asked for one more review. This is an independent review of the MERGED code; your findings become a follow-up PR.` : fresh ? `PR #${pr} ("${title}") was just opened, and the reviewer bots read it at the same time as you. The lead wants an independent review next to theirs; your findings join the bots' threads in the first fix round.` : `The reviewer bot found nothing more to fix on PR #${pr} ("${title}"), so the lead wants an independent review. Your findings become the next fix round.`; diff --git a/.claude/skills/babysit/scripts/scripts.test.ts b/.claude/skills/babysit/scripts/scripts.test.ts index 5d40f8780c..e1b0cc2349 100644 --- a/.claude/skills/babysit/scripts/scripts.test.ts +++ b/.claude/skills/babysit/scripts/scripts.test.ts @@ -1509,7 +1509,7 @@ test("generated worker briefs follow the round and dedicated-review policy", () ); expect(result.exitCode).toBe(0); const brief = fs.readFileSync(target, "utf8"); - expect(brief).toContain(all ? "fix EVERY valid finding" : "fix Critical and High"); + expect(brief).toContain(all ? "fix EVERY valid finding" : "fix Critical, Major and High"); expect(brief).toContain("A valid security finding is always fixed"); expect(brief).toContain("Change every file the fix needs in this PR"); if (!all) { @@ -4565,6 +4565,62 @@ describe("round 26 posting and answering-turn fences", () => { } }); + test("a dry integration previews outside the vault and never writes or posts there", () => { + const dir = fs.mkdtempSync(join(tmpdir(), "babysit-integrate-dry-")); + fs.writeFileSync( + join(dir, "run.json"), + SafeJSON.stringify({ repo: "fixture/repo", main: dir, post: true, reviewLoopScripts: join(dir, "scripts") }) + ); + fs.writeFileSync( + join(dir, "replies.json"), + SafeJSON.stringify({ pr: 1, round: 1, model: "fixture", intro: "fixture", items: [] }) + ); + const preload = join(dir, "preload.ts"), + callsFile = join(dir, "calls.json"); + fs.writeFileSync( + preload, + [ + "import * as fs from 'node:fs';const calls=[];process.on('exit',()=>fs.writeFileSync(" + + SafeJSON.stringify(callsFile) + + ",JSON.stringify(calls)));const ok=(out='')=>({exitCode:0,stdout:Buffer.from(out),stderr:Buffer.alloc(0)});", + "Bun.spawnSync=(args,opts)=>{if(args[0] === 'git'){if(args.includes('push'))throw new Error('no pushes');return ok(args.includes('rev-parse')?'fixture-head':'');}if(args[0] === 'gh'){if(args[1] === 'pr' && args[2] === 'view')return ok(JSON.stringify({headRefName:'fixture',baseRefName:'main'}));calls.push(args);return ok();}if(args.some(a=>a.endsWith('round-summary.ts'))){calls.push([...args,'cwd='+opts.cwd]);fs.writeFileSync(args[args.indexOf('--out')+1],'fixture summary');return ok();}if(args.some(a=>a.endsWith('post-replies.ts'))){calls.push(args);return ok();}throw new Error('unexpected command');};", + ].join("\n") + ); + const result = Bun.spawnSync( + [ + process.execPath, + "--preload", + preload, + join(import.meta.dir, "integrate.ts"), + "--run", + dir, + "--wt", + dir, + "--pr", + "1", + "--round", + "1", + "--replies", + join(dir, "replies.json"), + "--post", + "--dry", + ], + { stdout: "pipe", stderr: "pipe", env: env.getProcessEnv(), timeout: 5000 } + ); + const output = result.stdout.toString() + result.stderr.toString(); + expect(result.exitCode, output).toBe(0); + expect(fs.existsSync(join(dir, "vault"))).toBe(false); + expect(output).toContain("vault untouched"); + const calls = SafeJSON.parse(fs.readFileSync(callsFile, "utf8"), { strict: true }) as string[][]; + expect(calls.length).toBe(1); + const summary = calls[0]; + expect(summary.some((arg) => arg.endsWith("round-summary.ts"))).toBe(true); + const preview = summary[summary.indexOf("--out") + 1]; + expect(preview.startsWith(join(dir, "vault"))).toBe(false); + expect(fs.readFileSync(preview, "utf8")).toBe("fixture summary"); + expect(fs.existsSync(join(dirname(preview), "1-r1-replies.json"))).toBe(true); + }); + test("a steer fence excludes the previous finished turn and accepts a reply finished before waiting", () => { for (const mode of ["previous", "fast"]) { const dir = fs.mkdtempSync(join(tmpdir(), "babysit-answer-fence-")); diff --git a/docs/benchmarks-du.md b/docs/benchmarks-du.md index eb8e58092f..69fce9ce22 100644 --- a/docs/benchmarks-du.md +++ b/docs/benchmarks-du.md @@ -1407,3 +1407,21 @@ planning round. It now counts only the dataless folders of interior directories, The planning walk also runs with dataless materialization off, and a dataless scan root is never listed. The scan then finds nothing, as the C engine's guarded open of that root does. `cloudctl` runs stop after 60 s plus 2 s per path, and the call fails with that reason (an `evict` names that some files may already be evicted). + +## 2026-10-23 14:34 — Review follow-ups: partner and big-file queries reset the cloud policy + +No hot-loop code changed and no total moves, so no benchmark was rerun. The cloud policy lives in globals +(`g_skip_cloud`, `g_walk_cloud_roots`). `clonesize_run_json` and `clonesize_cloudfiles_json` set it on every call, +but `clonesize_partners_json` and `clonesize_bigfiles_json` did not. A partner query after an `includeCloud` scan in +the same process inherited `g_skip_cloud = 0`: the dataless checks were skipped, `cloud_thread_guard` did nothing, and +cloud-backed files were opened. Both FFI entries now set the stat-only policy first. The CLI keeps its `--include-cloud` +for `--partners-of` and `--bigfiles`, because `main()` calls the inner functions directly. + +Check (`cloud.test.ts`, "partners never inherit --include-cloud"): on the fake File Provider fixture, a partner query +after an opened scan reported `files_opened: 5` before the fix and 0 after it. The CLI keeps the flag: `--partners-of` +on the same kind of fixture opens 0 files without `--include-cloud` and 2 with it. + +`tools du evict` now takes its top N from the iCloud Drive files only, and counts the other providers' files in one +line. `--apply` exits 1 when a selected file, or one named by `--path`, was not evicted. The C engine, the shim and the +binary are not compiled on a Mac without Xcode or the Command Line Tools: an error names `xcode-select --install`, and +clang never opens the install dialog. diff --git a/scripts/ci/typecheck.test.ts b/scripts/ci/typecheck.test.ts index 93ebae35d7..cf10b8c1f6 100644 --- a/scripts/ci/typecheck.test.ts +++ b/scripts/ci/typecheck.test.ts @@ -1,8 +1,18 @@ import { describe, expect, it } from "bun:test"; -import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; -import { type Changes, type Project, programFiles, readState, reasonToRun } from "./typecheck"; +import { SafeJSON } from "@genesiscz/utils/json"; +import { + type Changes, + changedSince, + keepPassedProgram, + type Project, + passedProgram, + programFiles, + readState, + reasonToRun, +} from "./typecheck"; const PROJECTS: Project[] = [ { name: "root", tsconfig: "tsconfig.json" }, @@ -66,6 +76,63 @@ describe("build info and the CI step", () => { expect(programFiles(join(dir, "root.tsbuildinfo"))).toBeNull(); }); + it("selection reads the program of the last pass, not a failing run's rewritten build info", () => { + const dir = mkdtempSync(join(tmpdir(), "gt-typecheck-passed-")); + const root = PROJECTS[0]; + const buildInfo = join(dir, "root.tsbuildinfo"); + writeFileSync(buildInfo, buildInfoText(["src/page.ts", "src/shared.ts"])); + keepPassedProgram(dir, root); + // A failing run drops the import of shared.ts and rewrites the build info. + writeFileSync(buildInfo, buildInfoText(["src/page.ts"])); + const known = new Map([["root", passedProgram(dir, root)]]); + expect( + reasonToRun({ project: root, changes: { files: [join(dir, "src/shared.ts")], added: [] }, programs: known }) + ).toBe("in its program"); + // A pass with no build info to copy forgets the older list instead of pairing it with the new pass. + writeFileSync(buildInfo, "{"); + keepPassedProgram(dir, root); + expect(passedProgram(dir, root)).toBeNull(); + }); + + it("changedSince reads paths Git would quote (non-ASCII, tab, quote) as the real file names", () => { + const repo = mkdtempSync(join(tmpdir(), "gt-typecheck-quoted-")); + const run = (...args: string[]) => { + const proc = Bun.spawnSync(["git", "-c", "core.quotePath=true", ...args], { + cwd: repo, + env: process.env, + stderr: "pipe", + }); + expect({ args, code: proc.exitCode, stderr: proc.stderr.toString() }).toMatchObject({ code: 0 }); + }; + const names = ["src/app/café.ts", 'src/app/say "hi".ts', "src/app/tab\there.ts"]; + mkdirSync(join(repo, "src/app"), { recursive: true }); + for (const name of names) { + writeFileSync(join(repo, name), "export const a = 1;\n"); + } + run("init", "-q"); + run("add", "."); + run("-c", "user.name=fixture", "-c", "user.email=fixture@example.com", "commit", "-qm", "fixture"); + for (const name of names) { + writeFileSync(join(repo, name), "export const a = 2;\n"); + } + writeFileSync(join(repo, "src/app/nové.ts"), "export const b = 1;\n"); + + const previous = process.env.GIT_CONFIG_PARAMETERS; + // Git's default, whatever this machine's own config says: line output would print "src/app/caf\303\251.ts". + process.env.GIT_CONFIG_PARAMETERS = "'core.quotepath=true'"; + try { + const changes = changedSince("HEAD", repo); + expect([...(changes?.files ?? [])].sort()).toEqual([...names, "src/app/nové.ts"].sort()); + expect(changes?.added).toEqual(["src/app/nové.ts"]); + } finally { + if (previous === undefined) { + delete process.env.GIT_CONFIG_PARAMETERS; + } else { + process.env.GIT_CONFIG_PARAMETERS = previous; + } + } + }); + it("the CI step passes --base only when the event has a base (a workflow_dispatch run has none)", () => { const workflow = readFileSync(resolve(import.meta.dir, "../../.github/workflows/ci.yml"), "utf8"); const step = workflow.slice(workflow.indexOf("- name: TypeScript check\n")); @@ -79,3 +146,7 @@ describe("build info and the CI step", () => { expect(argv("abc123")).toBe("[--base][abc123]"); }); }); + +function buildInfoText(fileNames: string[]): string { + return SafeJSON.stringify({ fileNames }); +} diff --git a/scripts/ci/typecheck.ts b/scripts/ci/typecheck.ts index 324908aabd..636acee6cc 100644 --- a/scripts/ci/typecheck.ts +++ b/scripts/ci/typecheck.ts @@ -8,12 +8,12 @@ * bun scripts/ci/typecheck.ts --all # every project (still incremental) * * A project runs when no build info or state says what it holds, when a file that changes every project changed - * (`package.json`, `bun.lock`, a `tsconfig`), when a changed file is in its last program (the build info's - * `fileNames`), or when a TypeScript or JavaScript file was added that its program does not hold yet (only tsc knows + * (`package.json`, `bun.lock`, a `tsconfig`), when a changed file is in its last passing program (the build info's + * `fileNames`, copied aside after each pass), or when a TypeScript or JavaScript file was added that its program does not hold yet (only tsc knows * which include patterns take a new file, so every such project runs). Build info is a cache only: * `tsc` compares file hashes, so build info from another commit still gives the right answer, just slower. */ -import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join, relative, resolve } from "node:path"; import { SafeJSON } from "@genesiscz/utils/json"; import { out } from "@genesiscz/utils/logger"; @@ -91,8 +91,8 @@ export function reasonToRun({ return changes.added.some((file) => TS_FILE.test(file) && !program.has(key(file))) ? "new file" : null; } -function git(args: string[]): string | null { - const proc = Bun.spawnSync(["git", ...args], { stdout: "pipe", stderr: "pipe" }); +function git(args: string[], cwd?: string): string | null { + const proc = Bun.spawnSync(["git", ...args], { cwd, stdout: "pipe", stderr: "pipe" }); return proc.exitCode === 0 ? proc.stdout.toString() : null; } @@ -100,26 +100,34 @@ function lines(text: string | null): string[] { return (text ?? "").split("\n").filter(Boolean); } -/** What changed between `base` and the working tree (in CI the same as HEAD), untracked files included. */ -export function changedSince(base: string): Changes | null { - const diff = git(["diff", "--name-status", "--no-renames", base]); +/** Fields of `-z` output: git prints each path as it is, where line output quotes `café.ts` as `"caf\303\251.ts"`. */ +function nulFields(text: string | null): string[] { + return (text ?? "").split("\0").filter(Boolean); +} + +/** + * What changed between `base` and the working tree (in CI the same as HEAD), untracked files included. `cwd` must be + * the repo top: `ls-files` prints paths from it. + */ +export function changedSince(base: string, cwd?: string): Changes | null { + const diff = git(["diff", "--name-status", "--no-renames", "-z", base], cwd); if (diff === null) { return null; } const files: string[] = []; const added: string[] = []; - for (const line of lines(diff)) { - const [status, path] = line.split("\t"); - if (path) { - files.push(path); - if (status === "A") { - added.push(path); - } + // `-z --name-status` gives a status field, then its path field. + const fields = nulFields(diff); + for (let index = 0; index + 1 < fields.length; index += 2) { + const [status, path] = [fields[index], fields[index + 1]]; + files.push(path); + if (status === "A") { + added.push(path); } } - const untracked = lines(git(["ls-files", "--others", "--exclude-standard"])); + const untracked = nulFields(git(["ls-files", "--others", "--exclude-standard", "-z"], cwd)); return { files: [...new Set([...files, ...untracked])], added: [...new Set([...added, ...untracked])] }; } @@ -183,7 +191,36 @@ function buildInfoPath(dir: string, project: Project): string { return join(dir, `${project.name}.tsbuildinfo`); } -/** The files of the project's last program, from its build info; null when there is none to read, or it is cut. */ +function passedProgramPath(dir: string, project: Project): string { + return join(dir, `${project.name}.passed-program.json`); +} + +/** The files of the project's last PASSING program, which selection reads; null when none was kept. */ +export function passedProgram(dir: string, project: Project): Set | null { + return programFiles(passedProgramPath(dir, project)); +} + +/** + * After a pass, keep its program apart from the build info. tsc (and the pre-commit hook) rewrite the build info on a + * failing run too, and a failing run's file list can lack a file the passing program held, which would skip the + * project when that file changes next. Selection pairs this list with the diff since the same pass. + */ +export function keepPassedProgram(dir: string, project: Project): void { + const files = programFiles(buildInfoPath(dir, project)); + const target = passedProgramPath(dir, project); + if (files === null) { + // No list for this pass: an older one would pair a stale program with the new pass, so nothing is known. + rmSync(target, { force: true }); + return; + } + + writeFileSync(target, `${SafeJSON.stringify({ fileNames: [...files] })}\n`); +} + +/** + * The files of a project's program from a build info or a kept passing program (`fileNames`, relative to the file or + * absolute); null when there is none to read, or it is cut. + */ export function programFiles(buildInfo: string): Set | null { if (!existsSync(buildInfo)) { return null; @@ -266,7 +303,7 @@ async function main(): Promise { return diffs.get(sha) ?? null; }; - const programs = new Map(PROJECTS.map((project) => [project.name, programFiles(buildInfoPath(dir, project))])); + const programs = new Map(PROJECTS.map((project) => [project.name, passedProgram(dir, project)])); const selections = PROJECTS.map((project): Selection => { if (all) { return { project, reason: "--all" }; @@ -295,6 +332,7 @@ async function main(): Promise { for (const outcome of outcomes) { if (outcome.ok) { state.passed[outcome.project.name] = { sha: head, dirty: dirtyNow }; + keepPassedProgram(dir, outcome.project); } } diff --git a/src/agents/README.md b/src/agents/README.md index 613af2134d..4d8a504f96 100644 --- a/src/agents/README.md +++ b/src/agents/README.md @@ -213,9 +213,14 @@ message cut lines, it draws the call's whole diff (the `hook-diffs/.md` no diff element under the call's row, so the 10,000-character message limit no longer hides the rest. A running session needs `/reload-plugins`. `tools agents hooks mods` shows whether it is installed; `tools agents hooks config set mods.fullDiff false --write` turns it off without an uninstall (the mod reads -the hooks file). It draws nothing while `diff.fullDiff` is `off`. While the plugin is enabled in the user settings -and `mods.fullDiff` is on, the diff hook prints no diff for Claude: it writes every diff to its note, and the mod -draws it, so it is not drawn twice (Codex and Grok keep the hook's message). Each file has a `[-]` / `[+]` button +the hooks file). It draws nothing while `diff.fullDiff` is `off`. Installing the plugin is not what makes the +diff hook stand down: on every turn the loaded mod sets `GENESIS_TOOLS_MODS_SESSION` to the id of its session, and +only a Claude call whose session id equals that value, with `mods.fullDiff` on, gets no diff printed by the hook. +The hook then writes every diff to its note and the mod draws it, so it is not drawn twice. The mod may be loaded +from any settings scope or with `--plugin-dir`. A session that has not loaded it (started before the install and not +reloaded with `/reload-plugins`, or a nested session that inherited another session's id) keeps the hook's own +diff message, and so does a marked session whose settings now disable the plugin, until it reloads. Codex and Grok +always keep the hook's message. Each file has a `[-]` / `[+]` button that folds its diff to its name and back, and `[ files only ]` above them folds every file; a click on the diff itself still opens the group's calls. The diff of a call that runs `git pull`, `merge`, `rebase`, `checkout`, `switch`, `reset`, `cherry-pick`, `revert`, `am` or `stash pop|apply` starts hidden behind one diff --git a/src/agents/lib/hooks/diff/diff.test.ts b/src/agents/lib/hooks/diff/diff.test.ts index 9e32a84bd0..fdec130bad 100644 --- a/src/agents/lib/hooks/diff/diff.test.ts +++ b/src/agents/lib/hooks/diff/diff.test.ts @@ -56,6 +56,7 @@ import { } from "./render"; import { blobBatches, + blobHead, type CapturedEdit, captureChunks, capturedBefore, @@ -2014,6 +2015,19 @@ describe("a command that edits AND commits in the same call", () => { expect(message).not.toContain("BEGIN OPENSSH"); }); + it("reads only the first 256 KB of a base blob, an absent path as empty and a failed read as null", () => { + writeFileSync(join(repo2, "big-base.txt"), "z".repeat(300 * 1024)); + writeFileSync(join(repo2, "small-base.txt"), "small\n"); + run(["add", "-A"]); + run(["commit", "-qm", "seed big"]); + + expect(blobHead(repo2, "HEAD:big-base.txt")).toHaveLength(256 * 1024); + expect(blobHead(repo2, "HEAD:small-base.txt")).toBe("small\n"); + expect(blobHead(repo2, "HEAD:never-committed.txt")).toBe(""); + // Not proven harmless: the caller masks the patch as a cookie jar. + expect(blobHead(join(repo2, "no-such-dir"), "HEAD:small-base.txt")).toBeNull(); + }); + it("renders a file ONCE when the command commits it and then edits it again", () => { // Such a file is in the committed list AND still in `git status`, so it used to be // collected twice and rendered twice. `dedupeAcrossSessions` is off on purpose: the @@ -4299,6 +4313,18 @@ describe("tools agents hooks diff log", () => { ); }); + it("masks a token in both command headings before the cut, so a token cut in half cannot leak its start", () => { + const token = `ghp_${"A1b2C3d4".repeat(5)}`; + // The token straddles the 80-character cut: cut first, its first half no longer matches the pattern. + const command = `cd ${"/x".repeat(29)} && GH_TOKEN=${token} gh pr view`; + const heading = deferredTaskHeading({ id: "bgT", command }); + const lines = stripAnsi(commandLines(command, 3)); + + expect(heading).not.toContain("ghp_"); + expect(lines).not.toContain("ghp_"); + expect(stripAnsi(commandLines(`TOKEN=${token} git push`, 3))).toBe("$ TOKEN=[redacted] git push"); + }); + it("splits a message at background task headings: the call's own part first, then each task's", () => { const message = [ "Updated src/own.ts (+1 -0)", diff --git a/src/agents/lib/hooks/diff/run.ts b/src/agents/lib/hooks/diff/run.ts index 5a1b5b6011..a9d2bd6164 100644 --- a/src/agents/lib/hooks/diff/run.ts +++ b/src/agents/lib/hooks/diff/run.ts @@ -114,19 +114,55 @@ function patchFor({ /** * The first 256 KB of the file as `base` holds it: a command that removed a key's BEGIN line from a file with no - * captured copy leaves a current head that no longer looks like a key. "" for a file absent at `base`. + * captured copy leaves a current head that no longer looks like a key. "" for a file absent at `base`, null for a + * failed read (`blobHead`). */ -function baseHead(file: ChangedFile, base: string): string { - const path = relative(file.root, resolve(file.root, file.path)); - return gitOut(file.root, ["show", `${base}:${path}`], { quiet: true }).slice(0, 256 * 1024); +function baseHead(file: ChangedFile, base: string): string | null { + return blobHead(file.root, `${base}:${relative(file.root, resolve(file.root, file.path))}`); } +/** + * The first `HEAD_BYTES` of a `:` blob, never the whole blob: past the cap the read stops (`maxBuffer` + * ends git with SIGTERM), so a one-line edit to a large tracked file reads 256 KB more, not the file again. "" for + * a blob that does not exist, null when git failed otherwise (the patch is then masked as a cookie jar). + */ +export function blobHead(root: string, spec: string): string | null { + const run = spawnSync("git", ["-C", root, "cat-file", "blob", spec], { + maxBuffer: HEAD_BYTES, + timeout: GIT_BATCH_TIMEOUT_MS, + }); + const capped = run.error && "code" in run.error && run.error.code === "ENOBUFS"; + + if (run.status === 0 || capped) { + return run.stdout.toString("utf8", 0, Math.min(run.stdout.length, HEAD_BYTES)); + } + + if ( + run.status === 128 && + /does not exist|exists on disk, but not in|invalid object name/i.test(String(run.stderr)) + ) { + return ""; + } + + hookDiag("Could not read the head of a file at the base commit; its patch is masked as a cookie jar", { + root, + spec, + status: run.status, + signal: run.signal, + error: run.error, + }); + return null; +} + +/** How much of a file `readHead` and `blobHead` read: enough for a storage state's `"cookies"` or a key's BEGIN. */ +const HEAD_BYTES = 256 * 1024; + /** * `git diff ` with every secret-shaped value masked. Every patch the hook shows or saves comes from here, * so neither the model, the screen nor the full-diff note gets a token or a login cookie (a re-login rewrote a * Playwright storageState, and its note kept the session cookies in plain text, 2026-10-20). */ -function gitPatch(root: string, args: string[], baseHeads: string[] = []): string { +function gitPatch(root: string, args: string[], baseHeads: Array = []): string { const paths = args.slice(args.indexOf("--") + 1).filter((path) => path !== "/dev/null"); const heads = [...paths.map((path) => readHead(resolve(root, path))), ...baseHeads]; const patch = gitOut(root, ["diff", ...args]); @@ -160,7 +196,7 @@ export function readHead(path: string): string | null { try { const fd = openSync(path, "r"); try { - const buffer = Buffer.alloc(256 * 1024); + const buffer = Buffer.alloc(HEAD_BYTES); return buffer.toString("utf8", 0, readSync(fd, buffer, 0, buffer.length, 0)); } finally { closeSync(fd); @@ -629,7 +665,8 @@ const DEFERRED_MIN_ROOM = 200; * 80 characters made the change (2026-10-20). */ export function deferredTaskHeading(task: { id: string; command: string }): string { - const [line = "", ...rest] = task.command.split("\n"); + // Masked whole before the cut: a token cut in half no longer matches its pattern. + const [line = "", ...rest] = redactSecretsInText(task.command).split("\n"); const first = line.length > 80 ? `${line.slice(0, 79)}…` : rest.length > 0 ? `${line}…` : line; return `background task ${task.id} ended (${first}):\n`; @@ -673,7 +710,8 @@ const COMMAND_CHARS = 80; * so without them nothing says which command changed the files. */ export function commandLines(command: string, count: number): string { - const lines = command + // Masked whole before the cut, as `deferredTaskHeading` does: `TOKEN=ghp_… cmd` must not print its token. + const lines = redactSecretsInText(command) .trim() .split("\n") .map((line) => line.replace(/\p{Cc}/gu, " ").trimEnd()); diff --git a/src/azure-devops/api.ts b/src/azure-devops/api.ts index 800702f188..ac09800470 100644 --- a/src/azure-devops/api.ts +++ b/src/azure-devops/api.ts @@ -6,6 +6,7 @@ */ import type { + AttachmentReferenceApi, CommentFormat, CommentsResponse, Dashboard, @@ -1118,7 +1119,7 @@ export class Api { contentType: "application/octet-stream", description: `attachment ${fileName}`, }); - const created = SafeJSON.parse(await response.text()) as { url?: string }; + const created: AttachmentReferenceApi | null = SafeJSON.parse(await response.text()); if (!created?.url) { throw new Error(`attachment upload of ${fileName} returned no url`); diff --git a/src/azure-devops/api.types.ts b/src/azure-devops/api.types.ts index a1531c401d..dcbb41547b 100644 --- a/src/azure-devops/api.types.ts +++ b/src/azure-devops/api.types.ts @@ -8,6 +8,12 @@ import type { CommentFormat, IdentityRef } from "@app/azure-devops/types"; +/** POST `_apis/wit/attachments` answer; `url` is what an `` stores. */ +export interface AttachmentReferenceApi { + id?: string; + url?: string; +} + export interface WiqlWorkItemRef { id: number; url?: string; diff --git a/src/azure-devops/commands/workitem-update.ts b/src/azure-devops/commands/workitem-update.ts index 1d9ef5a567..9a7da11a6c 100644 --- a/src/azure-devops/commands/workitem-update.ts +++ b/src/azure-devops/commands/workitem-update.ts @@ -1,5 +1,5 @@ import { existsSync, readFileSync } from "node:fs"; -import { isAbsolute, join } from "node:path"; +import { resolve } from "node:path"; import { Api } from "@app/azure-devops/api"; import { formatJSON } from "@app/azure-devops/cache"; @@ -15,6 +15,7 @@ import { type WorkItemUpdateResult, } from "@app/azure-devops/lib/work-item-update"; import { requireConfig } from "@app/azure-devops/utils"; +import { isInsideDir } from "@genesiscz/utils/fs/canonical"; import { out } from "@genesiscz/utils/logger"; import type { Command } from "commander"; @@ -22,18 +23,22 @@ function collect(value: string, previous: string[]): string[] { return [...previous, value]; } -function resolveImages(values: string[], imageRoot: string): WorkItemUpdateImage[] { +export function resolveImages(values: string[], imageRoot: string): WorkItemUpdateImage[] { const sources = [...new Set(values.flatMap((value) => localImageSources(value)))]; const images: WorkItemUpdateImage[] = []; for (const src of sources) { - const relative = decodeURI(src); - const path = isAbsolute(relative) ? relative : join(imageRoot, relative); + const path = resolve(imageRoot, decodeURI(src)); if (!existsSync(path)) { throw new Error(`local image not found: ${path}`); } + // A field value can come from a draft another tool or agent wrote; only files under the image root upload. + if (!isInsideDir(imageRoot, path)) { + throw new Error(`local image ${path} is outside --image-root ${imageRoot}`); + } + const bytes = new Uint8Array(readFileSync(path)); images.push({ src, name: src.slice(src.lastIndexOf("/") + 1), sha256: sha256Hex(bytes), bytes }); diff --git a/src/azure-devops/config.ts b/src/azure-devops/config.ts index 1352980526..6cd33640ea 100644 --- a/src/azure-devops/config.ts +++ b/src/azure-devops/config.ts @@ -18,7 +18,7 @@ import { toolCommand } from "@genesiscz/utils/cli/tool-command"; import { env } from "@genesiscz/utils/env"; import { formatRelativeTime } from "@genesiscz/utils/format"; import { SafeJSON } from "@genesiscz/utils/json"; -import { out } from "@genesiscz/utils/logger"; +import { logger, out } from "@genesiscz/utils/logger"; export function getRelativeTime(date: Date): string { return formatRelativeTime(date, { rounding: "round" }); @@ -95,6 +95,8 @@ export function loadConfig(): AzureConfig | null { return SafeJSON.parse(readFileSync(found.path, "utf-8")); } catch (error) { if (found.source === "search") { + // A searched file is optional, so it falls through to "no configuration", but the reason must not vanish. + logger.warn({ err: error, path: found.path }, "azure-devops: found config cannot be parsed, ignoring it"); return null; } diff --git a/src/azure-devops/lib/work-item-update.test.ts b/src/azure-devops/lib/work-item-update.test.ts index 85cd4a7a5c..6db27c8c8c 100644 --- a/src/azure-devops/lib/work-item-update.test.ts +++ b/src/azure-devops/lib/work-item-update.test.ts @@ -1,10 +1,15 @@ import { describe, expect, test } from "bun:test"; -import { registerWorkItemUpdateCommand } from "@app/azure-devops/commands/workitem-update"; +import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { registerWorkItemUpdateCommand, resolveImages } from "@app/azure-devops/commands/workitem-update"; import { executeWorkItemUpdate, imageReuseMap, + localImageSources, parseFieldSpec, parseReuseFile, + replaceImageSources, sameStoredText, sha256Hex, type WorkItemUpdateClient, @@ -316,6 +321,47 @@ describe("what the read-back may forgive", () => { expect(html("

a;b

", "

ab

")).toBe(false); }); + test("read-back keeps space beside inline tags and Markdown hard breaks, and never throws on a bad reference", () => { + const html = (stored: string, sent: string) => sameStoredText({ stored, sent, format: "html" }); + const md = (stored: string, sent: string) => sameStoredText({ stored, sent, format: "markdown" }); + + expect(html("

Helloworld

", "

Hello world

")).toBe(false); + expect(md("one\ntwo", "one \ntwo")).toBe(false); + expect(md("one \ntwo", "one \ntwo")).toBe(true); + expect(html("

� �

", "

� �

")).toBe(true); + expect(html("

�

", "

x

")).toBe(false); + }); + + test("a local image outside --image-root is refused before it is read", () => { + const dir = mkdtempSync(join(tmpdir(), "wi-images-")); + mkdirSync(join(dir, "root")); + writeFileSync(join(dir, "root", "in.png"), "in"); + writeFileSync(join(dir, "secret.txt"), "secret"); + const root = join(dir, "root"); + + expect(resolveImages([''], root).map((image) => image.name)).toEqual(["in.png"]); + expect(() => resolveImages([''], root)).toThrow("outside --image-root"); + expect(() => resolveImages([``], root)).toThrow("outside --image-root"); + }); + + test("local image sources are found and replaced in every valid src spelling", () => { + const input = `x`; + + expect(localImageSources(input)).toEqual(["shots/a.png", "shots/b.png", "shots/c.png"]); + expect( + replaceImageSources( + input, + new Map([ + ["shots/a.png", "https://h/1"], + ["shots/b.png", "https://h/2"], + ["shots/c.png", "https://h/3"], + ]) + ) + ).toBe( + `x` + ); + }); + test("--markdown without --field still checks that the field became Markdown", async () => { const ado = client(live({ fields: { "System.Description": "body", "System.Title": "t" } })); ado.update = async () => {}; diff --git a/src/azure-devops/lib/work-item-update.ts b/src/azure-devops/lib/work-item-update.ts index 58c0b4c9ac..75a30b3bda 100644 --- a/src/azure-devops/lib/work-item-update.ts +++ b/src/azure-devops/lib/work-item-update.ts @@ -47,21 +47,21 @@ export type WorkItemUpdateResult = | { ok: true; dryRun: false; rev: number; images: Record; plan: WorkItemUpdatePlan } | { ok: false; error: string; mismatches?: { field: string; sent: string; stored: string }[] }; +/** An `` tag's `src` in any valid HTML spelling: any case, spaces around `=`, double, single or no quotes. */ +const IMG_SRC = /(]*?\bsrc\s*=\s*)(?:"([^"]*)"|'([^']*)'|([^\s>"']+))/gi; + /** Local `` values. `http`, `https` and `data` sources are already remote. */ export function localImageSources(html: string): string[] { - const sources = [...html.matchAll(/]*\bsrc="([^"]+)"/g)].map((match) => match[1] ?? ""); + const sources = [...html.matchAll(IMG_SRC)].map((match) => match[2] ?? match[3] ?? match[4] ?? ""); - return [...new Set(sources.filter((src) => src !== "" && !/^(https?:|data:)/.test(src)))]; + return [...new Set(sources.filter((src) => src !== "" && !/^(https?:|data:)/i.test(src)))]; } export function replaceImageSources(html: string, urls: ReadonlyMap): string { - let out = html; - - for (const [src, url] of urls) { - out = out.replaceAll(`src="${src}"`, `src="${url}"`); - } - - return out; + return html.replace(IMG_SRC, (tag: string, prefix: string, dq?: string, sq?: string, bare?: string) => { + const url = urls.get(dq ?? sq ?? bare ?? ""); + return url === undefined ? tag : `${prefix}"${url}"`; + }); } export function sha256Hex(bytes: Uint8Array): string { @@ -148,8 +148,8 @@ export function sameStoredText(input: { function normalizeAdoText(value: string, format: "markdown" | "html"): string { const text = value .replace(//g, "") - .replace(/&#(\d+);/g, (_match, code: string) => String.fromCodePoint(Number(code))) - .replace(/&#x([0-9a-fA-F]+);/g, (_match, hex: string) => String.fromCodePoint(Number.parseInt(hex, 16))) + .replace(/&#(\d+);/g, (match, code: string) => codePointOr(Number(code), match)) + .replace(/&#x([0-9a-fA-F]+);/g, (match, hex: string) => codePointOr(Number.parseInt(hex, 16), match)) .replace(/"/g, '"') .replace(/'/g, "'") .replace(/</g, "<") @@ -159,18 +159,25 @@ function normalizeAdoText(value: string, format: "markdown" | "html"): string { .replace(/<[^>]+>/g, (tag) => tag.replace(/;(?=\s*["'>])/g, "")); if (format === "html") { - return text - .replace(/\s*(<[^>]+>)\s*/g, "$1") - .replace(/\s+/g, " ") - .trim(); + // Space around a block tag is layout the server re-indents; space beside an inline tag is text. + return text.replace(BLOCK_TAG_SPACE, "$1").replace(/\s+/g, " ").trim(); } + // Two or more trailing spaces are a Markdown hard break, so they survive as one canonical pair. return text .replace(/\r\n?/g, "\n") - .replace(/[ \t]+$/gm, "") + .replace(/[ \t]+$/gm, (space) => (space.length >= 2 ? " " : "")) .trim(); } +const BLOCK_TAG_SPACE = + /\s*(<\/?(?:p|div|br|hr|ul|ol|li|table|thead|tbody|tfoot|tr|td|th|h[1-6]|blockquote|pre|section|article)\b[^>]*>)\s*/gi; + +/** A numeric character reference outside Unicode (or a lone surrogate) stays as written instead of throwing. */ +function codePointOr(code: number, original: string): string { + return code <= 0x10ffff && !(code >= 0xd800 && code <= 0xdfff) ? String.fromCodePoint(code) : original; +} + /** Markdown when the field is (or is being switched to) Markdown, HTML when the field or either value is HTML. */ function storedTextFormat(input: { name: string; diff --git a/src/domains/domains.test.ts b/src/domains/domains.test.ts index c3852d8ce6..c7a462ecda 100644 --- a/src/domains/domains.test.ts +++ b/src/domains/domains.test.ts @@ -599,6 +599,13 @@ describe("AI suggestor account", () => { it("names the fix for an unknown --account", async () => { writeConfig({}); - expect(brainstormPlan({ account: "nobody" })).rejects.toThrow("--account auto"); + await expect(brainstormPlan({ account: "nobody" })).rejects.toThrow("--account auto"); + }); + + it("refuses a full --model ref beside a named --account instead of dropping the model", async () => { + writeConfig({}); + await expect(brainstormPlan({ account: "acc_side", model: "@account/acc_work" })).rejects.toThrow( + "is a full ref" + ); }); }); diff --git a/src/domains/lib/suggest/online.ts b/src/domains/lib/suggest/online.ts index ea22c4fcfe..5118988fd5 100644 --- a/src/domains/lib/suggest/online.ts +++ b/src/domains/lib/suggest/online.ts @@ -125,8 +125,13 @@ export async function brainstormPlan(options: { ); } - const bare = model && !isFullRef(model) ? `:${model}` : ""; - return fixed(`@account/${entry.id}${bare}`, "domains"); + if (model && isFullRef(model)) { + throw new Error( + `--model ${model} is a full ref and names its own account; pass a bare model with --account ${account}, or drop --account` + ); + } + + return fixed(`@account/${entry.id}${model ? `:${model}` : ""}`, "domains"); } if (!account && model && isFullRef(model)) { diff --git a/src/du/README.md b/src/du/README.md index 479f3718f4..dc2e34073e 100644 --- a/src/du/README.md +++ b/src/du/README.md @@ -225,18 +225,21 @@ plus `cloud` (the `cloud_backed` object) and `skippedCloud`. | Flag | Description | |------|-------------| -| `--apply` | Really evict. Without it the command is a dry run that changes nothing | +| `--apply` | Really evict. Without it the command is a dry run that changes nothing. Exits 1 when a selected file (or one named by `--path`) was not evicted | | `--all` | Select every eligible file (non-interactive) | | `--path ` | Select these files only | -| `--min-bytes `, `--top ` | Which downloaded files to offer (default: the 50 largest) | +| `--min-bytes `, `--top ` | Which downloaded iCloud Drive files to offer (default: the 50 largest; other providers are counted, not offered) | List, select, evict. It lists like `downloaded`, asks iCloud for each file's state, offers the eligible files in a multiselect (or takes `--all` / `--path` without a TTY), and prints what it would free. With `--apply` it evicts through `FileManager.evictUbiquitousItem`. The helper is -`native/cloudctl.swift`, compiled on demand; on a Mac without Xcode or the Command Line Tools (or -when it does not compile) the same verbs run as `native/cloudctl.js` through `osascript -l -JavaScript`, which ships with macOS. The output names the helper that answered. Neither needs -GenesisTools.app. Eviction never deletes: the file stays in iCloud and downloads again when opened. +`native/cloudctl.swift`, compiled on demand; when `swiftc` is missing or does not compile it, the +same verbs run as `native/cloudctl.js` through `osascript -l JavaScript`, which ships with macOS. +The output names the helper that answered. Neither needs GenesisTools.app. The listing step still +needs the C engine, so on a Mac without Xcode or the Command Line Tools `evict` works only once the +engine was compiled there before; otherwise it stops with an error that names `xcode-select +--install` (it never opens the install dialog). Eviction never deletes: the file stays in iCloud +and downloads again when opened. A file is evicted only when iCloud reports it uploaded, not uploading, without an upload error, with the local copy current. The helper reads that state again right before each eviction. diff --git a/src/du/lib/cloud-commands.ts b/src/du/lib/cloud-commands.ts index 62a0a27422..9b957c7227 100644 --- a/src/du/lib/cloud-commands.ts +++ b/src/du/lib/cloud-commands.ts @@ -13,7 +13,7 @@ import type { Command } from "commander"; import { Option } from "commander"; import pc from "picocolors"; import { listCloudFiles } from "./engine"; -import { applyEviction, type CloudCtl, type EvictionPlan, planEviction, realCloudCtl } from "./evict"; +import { applyEviction, type CloudCtl, type EvictionPlan, planEviction, providerRefusal, realCloudCtl } from "./evict"; import { humanBytes, shellQuote, tildePath } from "./format"; import { intOpt } from "./options"; import type { CloudFileEntry, CloudFilesResult } from "./types"; @@ -151,10 +151,16 @@ export async function runEvict(opts: { } } + const requested = opts.cli.apply ? (opts.cli.path ?? []) : []; if (plan.eligible.length === 0) { out.println( `Nothing is safe to evict: none of the ${opts.files.length} offered file(s) is confirmed uploaded.` ); + // `--apply --path` asked for these files by name, so not evicting them is a failure, not "nothing to do". + if (requested.length > 0) { + process.exitCode = 1; + } + return; } @@ -197,6 +203,31 @@ export async function runEvict(opts: { for (const r of results.filter((x) => !x.evicted)) { out.println(pc.yellow(` not evicted: ${tildePath(r.path)} (${r.reason ?? "unknown"})`)); } + + // A refused or failed eviction, or a file asked for by --path that was not eligible, fails the command, so a + // script can tell a partial reclaim from a full one. + const evicted = new Set(ok.map((r) => r.path)); + if (results.some((r) => !r.evicted) || requested.some((path) => !evicted.has(path))) { + process.exitCode = 1; + } +} + +/** + * The files `evict` offers: the ones named by --path (refused ones too, so their reason is printed), else the `top` + * largest of the providers it can evict. Taking the top N before that filter let other providers' big files push + * every iCloud file out of the offer. + */ +export function offerForEviction( + files: CloudFileEntry[], + o: { paths?: string[]; top?: number } +): { files: CloudFileEntry[]; otherProviders: number } { + if (o.paths) { + const want = new Set(o.paths); + return { files: files.filter((f) => want.has(f.path)), otherProviders: 0 }; + } + + const evictable = files.filter((f) => providerRefusal(f.provider) === null); + return { files: evictable.slice(0, o.top ?? 50), otherProviders: files.length - evictable.length }; } export function registerCloudCommands(program: Command): void { @@ -282,12 +313,30 @@ export function registerCloudCommands(program: Command): void { const root = resolveDir(dir); const listing = listCloudFiles({ root, minBytes: o.minBytes, threads: o.threads }); const paths = o.path?.map((x) => resolve(x)); - const want = paths ? new Set(paths) : null; - const files = want ? listing.files.filter((f) => want.has(f.path)) : listing.files.slice(0, o.top ?? 50); - logger.debug({ root, listed: listing.files.length, offered: files.length }, "du evict: candidates"); + const { files, otherProviders } = offerForEviction(listing.files, { paths, top: o.top }); + logger.debug( + { root, listed: listing.files.length, offered: files.length, otherProviders }, + "du evict: candidates" + ); renderCliHeader("Evict local copies", root); + if (otherProviders > 0) { + out.println( + pc.dim( + ` ${otherProviders.toLocaleString()} downloaded file(s) of other providers are not offered: only iCloud Drive eviction is supported.` + ) + ); + } + if (files.length === 0) { - out.println("No downloaded cloud files found here."); + out.println( + otherProviders > 0 + ? "No downloaded iCloud Drive files found here." + : "No downloaded cloud files found here." + ); + if (o.apply && paths && paths.length > 0) { + process.exitCode = 1; + } + return; } diff --git a/src/du/lib/cloud.test.ts b/src/du/lib/cloud.test.ts index b41b6710fc..36cb13e72d 100644 --- a/src/du/lib/cloud.test.ts +++ b/src/du/lib/cloud.test.ts @@ -14,8 +14,8 @@ import { providerOf, TRESORIT_PROVIDER, } from "./cloud"; -import { runEvict } from "./cloud-commands"; -import { listCloudFiles, scanWithCFfi } from "./engine"; +import { offerForEviction, runEvict } from "./cloud-commands"; +import { assertCanCompile, listCloudFiles, scanPartners, scanWithCFfi } from "./engine"; import { applyEviction, type CloudCtl, @@ -182,6 +182,18 @@ describeOnDarwin("File Provider domain roots are measured by stat only", () => { }); } + it("partners never inherit --include-cloud from an earlier scan in the same process", () => { + const dir = fx.realClones.dir; + const target = join(dir, "clone1.bin"); + const before = scanPartners({ target, root: dir, threads: 2 }); + expect(before.files_opened).toBe(0); + // The library keeps its cloud policy in globals; an opened scan must not leave the partner walk opening files. + scanWithCFfi({ path: dir, threads: 2, includeCloud: true }); + const after = scanPartners({ target, root: dir, threads: 2 }); + expect(after.files_opened).toBe(0); + expect(after.partner_files_total).toBe(0); + }); + it("downloaded lists the domain's files with their provider, and nothing outside a domain", () => { const listed = listCloudFiles({ root: join(root, "dom"), threads: 2 }); expect(listed.files.length).toBeGreaterThan(0); @@ -285,6 +297,65 @@ describe("eviction safety", () => { await runEvict({ root: "/u/Documents", files: [good], cli: { all: true, apply: true }, ctl: live }); expect(live.evicted).toEqual([good.path]); }); + + it("--apply exits 1 when a selected eviction fails or a file named by --path is not evicted", async () => { + const saved = process.exitCode; + try { + const good = file("/u/Documents/good"); + const stuck = file("/u/Documents/stuck"); + const ctl: CloudCtl = { + status: async (paths) => paths.map(uploaded), + evict: async (paths) => + paths.map((p) => + p === stuck.path ? { path: p, evicted: false, reason: "busy" } : { path: p, evicted: true } + ), + }; + process.exitCode = 0; + await runEvict({ root: "/u/Documents", files: [good], cli: { all: true, apply: true }, ctl }); + expect(process.exitCode).toBe(0); + + await runEvict({ root: "/u/Documents", files: [good, stuck], cli: { all: true, apply: true }, ctl }); + expect(process.exitCode).toBe(1); + + process.exitCode = 0; + const pending = file("/u/Documents/pending"); + const refusing: CloudCtl = { + ...ctl, + status: async (paths) => paths.map((p) => ({ ...uploaded(p), uploaded: false })), + }; + await runEvict({ + root: "/u/Documents", + files: [pending], + cli: { path: [pending.path], apply: true }, + ctl: refusing, + }); + expect(process.exitCode).toBe(1); + + // The same refusal in a dry run is a report, not a failure. + process.exitCode = 0; + await runEvict({ root: "/u/Documents", files: [pending], cli: { path: [pending.path] }, ctl: refusing }); + expect(process.exitCode).toBe(0); + } finally { + process.exitCode = saved; + } + }); + + it("offers the top N evictable files, so other providers' larger files never push iCloud out", () => { + const big = Array.from({ length: 3 }, (_, i) => ({ ...file(`/u/Box/b${i}`, "com.example.box"), alloc: 9e9 })); + const icloud = file("/u/Documents/small"); + const offer = offerForEviction([...big, icloud], { top: 2 }); + expect(offer.files.map((f) => f.path)).toEqual([icloud.path]); + expect(offer.otherProviders).toBe(3); + // Files named by --path are all offered, so a refused one still prints its reason. + expect(offerForEviction([...big, icloud], { paths: [big[0]!.path] }).files.map((f) => f.path)).toEqual([ + big[0]!.path, + ]); + }); + + it("refuses to start a compile without developer tools, before clang can open the install dialog", () => { + expect(() => assertCanCompile("/x/clonesize.c", () => false)).toThrow("xcode-select --install"); + expect(() => assertCanCompile("/x/clonesize.c", () => true)).not.toThrow(); + }); it("uses the Swift helper when it is built or can be, and osascript on a Mac without developer tools", () => { const calls: string[] = []; const deps = (ready: boolean, tools: boolean, compiles: boolean) => ({ diff --git a/src/du/lib/engine.ts b/src/du/lib/engine.ts index d190797a47..02113b4d63 100644 --- a/src/du/lib/engine.ts +++ b/src/du/lib/engine.ts @@ -10,9 +10,9 @@ // it (~4.7s on GenesisTools' 388k files, ~56% faster than the pre-skip engine). import { CString, dlopen, FFIType, ptr, suffix } from "bun:ffi"; -import { execFileSync } from "node:child_process"; +import { execFileSync, spawnSync } from "node:child_process"; import { existsSync, statSync } from "node:fs"; -import { join } from "node:path"; +import { basename, join } from "node:path"; import { SafeJSON } from "@genesiscz/utils/json"; import { logger } from "@genesiscz/utils/logger"; import { profiler } from "@genesiscz/utils/profile"; @@ -35,6 +35,29 @@ const DYLIB = join(NATIVE_DIR, `libclonesize.${suffix}`); const SHIM_SRC = join(NATIVE_DIR, "l2p_shim.c"); const SHIM = join(NATIVE_DIR, "libl2pshim.dylib"); +/** + * Xcode or the Command Line Tools are installed. `xcode-select -p` never prompts, while /usr/bin/clang and + * /usr/bin/swiftc exist on every Mac and, without the tools, open the "install the developer tools" dialog instead + * of compiling. + */ +export function hasDeveloperTools(): boolean { + const found = spawnSync("xcode-select", ["-p"], { encoding: "utf8" }); + return found.status === 0 && existsSync(found.stdout.trim()); +} + +/** Throws an actionable error, before clang can open the install dialog, when nothing can compile `src`. */ +export function assertCanCompile(src: string, developerTools: () => boolean = hasDeveloperTools): void { + if (developerTools()) { + return; + } + + throw new Error( + `The native engine (${basename(src)}) is not compiled yet, and compiling it needs Xcode or the Command Line Tools.\n` + + ` source: ${src}\n` + + ` Install them with \`xcode-select --install\`, then run the command again.` + ); +} + /** * Ensure the non-variadic fcntl shim dylib exists (needed by the Bun engine's * FFI path — see scan-worker.ts). Compiled on demand and cached. @@ -43,6 +66,7 @@ export function ensureShim(): string { const needBuild = !existsSync(SHIM) || (existsSync(SHIM_SRC) && statSync(SHIM_SRC).mtimeMs > statSync(SHIM).mtimeMs); if (needBuild) { + assertCanCompile(SHIM_SRC); logger.debug({ src: SHIM_SRC, out: SHIM }, "du: compiling l2p_shim.c"); execFileSync("clang", ["-O2", "-dynamiclib", "-o", SHIM, SHIM_SRC], { stdio: "pipe" }); } @@ -58,6 +82,7 @@ export function ensureBinary(): string { const needBuild = !existsSync(BIN) || (existsSync(SRC) && statSync(SRC).mtimeMs > statSync(BIN).mtimeMs); if (needBuild) { + assertCanCompile(SRC); logger.debug({ src: SRC, bin: BIN }, "du: compiling clonesize.c"); try { execFileSync("clang", ["-O2", "-pthread", "-o", BIN, SRC], { stdio: "pipe" }); @@ -83,6 +108,7 @@ export function ensureDylib(): string { const needBuild = !existsSync(DYLIB) || (existsSync(SRC) && statSync(SRC).mtimeMs > statSync(DYLIB).mtimeMs); if (needBuild) { + assertCanCompile(SRC); logger.debug({ src: SRC, dylib: DYLIB }, "du: compiling clonesize.c as dylib"); try { execFileSync("clang", ["-O2", "-pthread", "-dynamiclib", "-o", DYLIB, SRC], { stdio: "pipe" }); diff --git a/src/du/lib/evict.ts b/src/du/lib/evict.ts index a10fad143a..4e115ed1cc 100644 --- a/src/du/lib/evict.ts +++ b/src/du/lib/evict.ts @@ -10,7 +10,7 @@ // both lie), and GenesisTools does not read file contents here. Other providers // are refused too: their upload state has not been verified. -import { execFileSync, spawnSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; import { existsSync, statSync } from "node:fs"; import { join } from "node:path"; import { toolCommand } from "@genesiscz/utils/cli/tool-command"; @@ -18,6 +18,7 @@ import { SafeJSON } from "@genesiscz/utils/json"; import { logger } from "@genesiscz/utils/logger"; import { killWithEscalation } from "@genesiscz/utils/process/killWithEscalation"; import { ICLOUD_PROVIDER, TRESORIT_PROVIDER } from "./cloud"; +import { hasDeveloperTools } from "./engine"; import type { CloudFileEntry } from "./types"; const NATIVE_DIR = join(import.meta.dir, "..", "native"); @@ -171,6 +172,8 @@ export interface CloudCtlHelperDeps { compile(): void; } +/** A cold `swiftc -O` of the helper takes a few seconds; past this it is stuck. */ +const CLOUDCTL_COMPILE_MS = 120_000; const SWIFT_HELPER: CloudCtlHelper = { kind: "swift", command: [CLOUDCTL_BIN] }; const OSASCRIPT_HELPER: CloudCtlHelper = { kind: "osascript", command: ["osascript", "-l", "JavaScript", CLOUDCTL_JS] }; @@ -201,15 +204,15 @@ const realHelperDeps: CloudCtlHelperDeps = { binaryReady: () => existsSync(CLOUDCTL_BIN) && !(existsSync(CLOUDCTL_SRC) && statSync(CLOUDCTL_SRC).mtimeMs > statSync(CLOUDCTL_BIN).mtimeMs), - // `xcode-select -p` never prompts. /usr/bin/swiftc exists on every Mac, but without the tools it opens the - // "install the command line developer tools" dialog instead of compiling. - developerTools: () => { - const found = spawnSync("xcode-select", ["-p"], { encoding: "utf8" }); - return found.status === 0 && existsSync(found.stdout.trim()); - }, + developerTools: hasDeveloperTools, compile: () => { logger.debug({ src: CLOUDCTL_SRC, bin: CLOUDCTL_BIN }, "du: compiling cloudctl.swift"); - execFileSync("swiftc", ["-O", "-o", CLOUDCTL_BIN, CLOUDCTL_SRC], { stdio: "pipe" }); + // A stuck compile throws at the deadline like a failed one, and the osascript helper takes over. + execFileSync("swiftc", ["-O", "-o", CLOUDCTL_BIN, CLOUDCTL_SRC], { + stdio: "pipe", + timeout: CLOUDCTL_COMPILE_MS, + killSignal: "SIGKILL", + }); }, }; diff --git a/src/du/native/clonesize.c b/src/du/native/clonesize.c index 2c8fb340f1..00f877c8a6 100644 --- a/src/du/native/clonesize.c +++ b/src/du/native/clonesize.c @@ -1996,8 +1996,8 @@ static void build_target_ranges(int nthreads) { free(all); } -__attribute__((visibility("default"))) -char *clonesize_partners_json(const char *target, const char *root, int threads, int topn) { +/** The partner query under the cloud policy already in g_skip_cloud (main() sets it from argv). */ +static char *partners_run_json(const char *target, const char *root, int threads, int topn) { g_nthreads = threads; g_maxdepth = -1; g_freeable_tree = 0; @@ -2133,6 +2133,17 @@ char *clonesize_partners_json(const char *target, const char *root, int threads, return out; } +/** + * The FFI entry: always stat-only for cloud-backed files. The policy lives in globals, so without this reset a + * partner query inherited `include_cloud` from an earlier clonesize_run_json in the same process and opened them. + */ +__attribute__((visibility("default"))) +char *clonesize_partners_json(const char *target, const char *root, int threads, int topn) { + g_skip_cloud = 1; + g_walk_cloud_roots = 0; + return partners_run_json(target, root, threads, topn); +} + __attribute__((visibility("default"))) void clonesize_free(char *p) { free(p); } @@ -2435,6 +2446,8 @@ __attribute__((visibility("default"))) char *clonesize_bigfiles_json(const char *const *roots, int nroots, int threads, unsigned long long min_bytes, const char *const *prune_names, int nprune) { + // Same stale-global hazard as the partner query: never inherit an earlier include_cloud scan. + g_skip_cloud = 1; g_walk_cloud_roots = 0; return big_run_json(roots, nroots, threads, min_bytes, prune_names, nprune, 0); } @@ -3373,7 +3386,7 @@ int main(int argc, char **argv) { } if (partners_of) { - char *p = clonesize_partners_json(partners_of, target, g_nthreads, topn); + char *p = partners_run_json(partners_of, target, g_nthreads, topn); if (!p) return 1; fputs(p, stdout); free(p); diff --git a/src/gitlab/lib/review.test.ts b/src/gitlab/lib/review.test.ts index 1095684f46..084bbd789c 100644 --- a/src/gitlab/lib/review.test.ts +++ b/src/gitlab/lib/review.test.ts @@ -1190,6 +1190,8 @@ describe("top-level threads in the receive report", () => { expect(index).toContain( " T03 plain comment @dana 1n @me Remember to delete the migration skill before the merge" ); - expect(expandThreads(discussions, opts, ["T03", "note1"])).toContain("## Thread 1 — plain comment"); + const expanded = expandThreads(discussions, { ...opts, refs: new Map([["note1", "T03"]]) }, ["T03"]); + expect(expanded).toContain("Remember to delete the migration skill before the merge"); + expect(expanded).not.toContain("no unresolved thread has this id"); }); }); diff --git a/src/hub/lib/agents/index.ts b/src/hub/lib/agents/index.ts index 78d4cf45d9..58984f52ae 100644 --- a/src/hub/lib/agents/index.ts +++ b/src/hub/lib/agents/index.ts @@ -114,10 +114,13 @@ async function parentsByIds({ ids, rows, refresh, + retryMisses, }: { ids: Iterable; rows: AgentSessionRow[]; refresh?: boolean; + /** An explicit `--session` lookup has no listing row to fall back on, so it never trusts a cached miss. */ + retryMisses?: boolean; }): Promise> { const parents = new Map(); const missing = new Set(); @@ -134,12 +137,14 @@ async function parentsByIds({ for (const id of missing) { const missedAt = parentMisses.get(id); - if (missedAt !== undefined && now - missedAt < PARENT_MISS_REUSE_MS) { + if (!retryMisses && missedAt !== undefined && now - missedAt < PARENT_MISS_REUSE_MS) { missing.delete(id); } } const resolved = new Map(); + // Only "no session file" is a stable absence; a read error may clear on the next poll, so it is never cached. + const unstable = new Set(); for (const provider of ["claude", "codex"] as const) { if (missing.size === 0) { break; @@ -148,9 +153,14 @@ async function parentsByIds({ const transcripts = await resolveTranscripts([...missing], {}, provider); for (const [id, transcript] of transcripts) { if (transcript instanceof Error) { + if (!transcript.message.startsWith("No session file found")) { + unstable.add(id); + } + continue; } + unstable.delete(id); resolved.set(id, { provider, sessionId: transcript.sessionId, @@ -167,8 +177,11 @@ async function parentsByIds({ } for (const id of missing) { - logger.debug({ id }, "[hub agents] parent session not found"); - parentMisses.set(id, now); + logger.debug({ id, unstable: unstable.has(id) }, "[hub agents] parent session not found"); + + if (!unstable.has(id)) { + parentMisses.set(id, now); + } } if (resolved.size > 0) { @@ -197,8 +210,9 @@ async function parentById(options: { refresh?: boolean; }): Promise { return ( - (await parentsByIds({ ids: [options.id], rows: options.rows, refresh: options.refresh })).get(options.id) ?? - null + ( + await parentsByIds({ ids: [options.id], rows: options.rows, refresh: options.refresh, retryMisses: true }) + ).get(options.id) ?? null ); } diff --git a/src/macos/GenesisKit/Sources/GenesisKit/Controls/GenRenderSwap.swift b/src/macos/GenesisKit/Sources/GenesisKit/Controls/GenRenderSwap.swift index 2825efba5a..b0b061fc1a 100644 --- a/src/macos/GenesisKit/Sources/GenesisKit/Controls/GenRenderSwap.swift +++ b/src/macos/GenesisKit/Sources/GenesisKit/Controls/GenRenderSwap.swift @@ -72,6 +72,9 @@ public struct GenRenderSwap: View { /// The previous content, fading out on top. @State private var leaving: Value? @State private var fading = false + /// Counts switches. A delayed promotion (the timeout, a ready's next turn) carries the count of its switch and does + /// nothing once another switch started: after A, B, C, B the first B's timeout must not promote the second B. + @State private var generation = 0 @GenReduceMotion private var reduceMotion static var fadeDuration: TimeInterval { 0.14 } @@ -150,16 +153,19 @@ public struct GenRenderSwap: View { } guard id != key(pending) else { return } pending = new + generation += 1 + let switchGeneration = generation DispatchQueue.main.asyncAfter(deadline: .now() + timeout) { - if key(pending) == id { promote() } + if key(pending) == id, generation == switchGeneration { promote() } } } private func arrived(_ id: ID) { guard id == key(building) else { return } + let switchGeneration = generation // One turn for the page to paint what it just built. DispatchQueue.main.async { - if key(pending) == id { promote() } + if key(pending) == id, generation == switchGeneration { promote() } } } diff --git a/src/macos/GenesisKit/Sources/GenesisKitDisk/Disk/ToolsDiskProvider.swift b/src/macos/GenesisKit/Sources/GenesisKitDisk/Disk/ToolsDiskProvider.swift index 26da10f32d..8ac69a5097 100644 --- a/src/macos/GenesisKit/Sources/GenesisKitDisk/Disk/ToolsDiskProvider.swift +++ b/src/macos/GenesisKit/Sources/GenesisKitDisk/Disk/ToolsDiskProvider.swift @@ -243,15 +243,18 @@ public struct ClonesDuplicatesOptions: Equatable, Sendable { } /// What `tools macos clones optimize` clones: every duplicate file at or above the minimum under `roots`, or under -/// the trees found in `dirs` (and `worktreesOf`). Unlike reclaim, it needs no install trees. +/// the trees found when discovery is on. Discovery is on when `dirs`, `worktreesOf` or a non-empty `targets` is set. +/// Unlike reclaim, it needs no install trees. public struct ClonesOptimizeOptions: Equatable, Sendable { - /// Positional roots. With `dirs` or `worktreesOf`, the CLI searches them for trees too. + /// Positional roots. Without discovery, every ordinary file under them is a candidate. With discovery (`dirs`, + /// `worktreesOf` or a non-empty `targets`), the CLI only searches them for trees, like a `--dir`. public var roots: [String] /// Each one is a `--dir`: search this folder for install trees. public var dirs: [String] /// `--worktrees-of `. public var worktreesOf: String? - /// `--targets`, as for reclaim. Used only with `dirs` or `worktreesOf`. + /// `--targets`, as for reclaim. A non-empty list turns on discovery by itself: `roots: ["/media"], targets: + /// ["vendor"]` clones only inside the vendor trees found under /media, not every file there. public var targets: [String]? public var filters: ClonesScanFilters @@ -764,7 +767,8 @@ extension ToolsDiskProvider { } /// `tools macos clones reclaim presets auto --json`: lets the daily scan task run the preset on its own (the - /// first run after arming is a dry run), stops it, or changes its per-run limits. Returns the preset after it. + /// first run after arming is a dry run), stops it, or changes its per-run limits. Limits passed with + /// `enabled: false` are saved too, and the next arming uses them. Returns the preset after it. public func clonesPresetAuto(id: String, enabled: Bool?, maxBytes: ClonesAutoLimit = .unchanged, maxFiles: ClonesAutoLimit = .unchanged) async throws -> ClonesPreset { let args = Self.clonesPresetAutoArguments(id: id, enabled: enabled, maxBytes: maxBytes, maxFiles: maxFiles) @@ -793,9 +797,10 @@ extension ToolsDiskProvider { + (apply && !usesCachedPlan ? ["--no-cache"] : []) } - /// `tools macos clones optimize --format json`: plans (a recorded dry run) or applies the cloning of every - /// duplicate file under the roots. An apply is audited and reversible with `clonesRollback`, and runs detached - /// with `actTimeoutSeconds`. + /// `tools macos clones optimize --format json`: plans or applies the cloning of every duplicate file under the + /// roots. A dry run returns a transient report and caches the plan for a quick apply; its id is not recorded, so + /// `clonesProcessLog` cannot read it. An apply is recorded in the audit log, reversible with `clonesRollback`, + /// and runs detached with `actTimeoutSeconds`. public func clonesOptimize(_ options: ClonesOptimizeOptions, apply: Bool, usesCachedPlan: Bool = true) async throws -> ClonesApplyResult { // `--worktrees-of ` is a selector of its own: the CLI finds that repo's worktrees with no root or --dir. diff --git a/src/macos/GenesisKit/Tests/GenesisKitTests/GenRenderSwapTests.swift b/src/macos/GenesisKit/Tests/GenesisKitTests/GenRenderSwapTests.swift index 37d465c5da..f8b2733efe 100644 --- a/src/macos/GenesisKit/Tests/GenesisKitTests/GenRenderSwapTests.swift +++ b/src/macos/GenesisKit/Tests/GenesisKitTests/GenRenderSwapTests.swift @@ -69,6 +69,31 @@ final class GenRenderSwapTests: XCTestCase { XCTAssertEqual(rig.mounted, ["c"], "the timeout swaps without a ready") } + /// A, B, C, B before any timeout: the first B's timeout must not promote the second B, which has neither signalled + /// nor waited its own interval (review thread on PR #507). + func testAnEarlierSwitchsTimeoutDoesNotPromoteALaterSwitchToTheSameID() { + let rig = Rig() + let hosting = NSHostingView(rootView: Host(rig: rig, timeout: 1.2)) + hosting.frame = NSRect(x: 0, y: 0, width: 300, height: 200) + let window = NSWindow(contentRect: hosting.frame, styleMask: [.borderless], backing: .buffered, defer: false) + window.isReleasedWhenClosed = false + window.contentView = hosting + defer { window.contentView = nil } + pump(0.1) + XCTAssertEqual(rig.mounted, ["a"]) + + rig.id = "b" + pump(0.2) + rig.id = "c" + pump(0.2) + rig.id = "b" + // The first B's timeout fires 1.2 s after its switch; the second B's 1.2 s after the third switch. + pump(1.1) + XCTAssertEqual(rig.mounted, ["a", "b"], "the old content stays until the second B's own timeout") + pump(0.5) + XCTAssertEqual(rig.mounted, ["b"], "the second B arrives on its own timeout") + } + /// The markdown viewer's fade: the old content goes in the swap's own pass, and the crossfade is an animation on the /// hosting view's layer, so no SwiftUI frame runs for it. func testALayerFadeSwapsAtOnceAndCrossfadesTheHostingLayer() throws { diff --git a/src/macos/GenesisTools/Sources/Host/App/WindowCycle.swift b/src/macos/GenesisTools/Sources/Host/App/WindowCycle.swift index 74254af4ad..fe9330f1e9 100644 --- a/src/macos/GenesisTools/Sources/Host/App/WindowCycle.swift +++ b/src/macos/GenesisTools/Sources/Host/App/WindowCycle.swift @@ -248,7 +248,7 @@ package enum WindowCycle { guard let pid = info?["pid"] as? Int, pid == Int(getpid()), let number = info?["window"] as? Int else { return } lastCycleRaise = Date() leftFrom = nil - guard let requested = requestedSession(info, pid: getpid(), window: number) else { + guard let requested = requestedSession(info, pid: getpid(), window: number, known: currentCandidates()) else { raise(number, reason: "asked by another face", cycling: false) return } @@ -265,8 +265,10 @@ package enum WindowCycle { /// The run a raise request carries: pairs of pid and window number, the index of the landed window, which must /// be `window` of `pid`, and the modifiers whose release ends the run. Any process may post the notification, so - /// a payload that does not fit (a negative index, a pid outside `pid_t`, no modifier) is nil, a plain raise. - package static func requestedSession(_ info: [AnyHashable: Any]?, pid: pid_t, window: Int) -> Session? { + /// a payload that does not fit (a negative index, a pid outside `pid_t`, no modifier) is nil, a plain raise. The + /// other windows of the run must be `known` (this app's faces' windows on screen): an unknown one is dropped, or + /// the next press would hand the activation to whatever process the sender named. Fewer than two left is nil. + package static func requestedSession(_ info: [AnyHashable: Any]?, pid: pid_t, window: Int, known: [Candidate]) -> Session? { guard let flat = info?["order"] as? [Int], !flat.isEmpty, flat.count.isMultiple(of: 2), let index = info?["index"] as? Int, (0..<(flat.count / 2)).contains(index), let raw = info?["modifiers"] as? Int, let bits = UInt(exactly: raw) else { return nil } @@ -277,7 +279,9 @@ package enum WindowCycle { } let modifiers = NSEvent.ModifierFlags(rawValue: bits).intersection(relevant) guard !modifiers.isEmpty, order[index] == Candidate(pid: pid, number: window) else { return nil } - return Session(order: order, index: index, modifiers: modifiers) + let kept = order.indices.filter { $0 == index || known.contains(order[$0]) } + guard kept.count > 1, let landed = kept.firstIndex(of: index) else { return nil } + return Session(order: kept.map { order[$0] }, index: landed, modifiers: modifiers) } /// The window a focus broadcast names, when it is another face's and its pid fits `pid_t`. diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift index 83b6085ed6..21239bb9c8 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubCommitLink.swift @@ -140,15 +140,28 @@ enum HubCommitLookup { guard let (older, newer) = ordered else { return .failed("\(sha) and \(to) are not on one line of history in \(place), so they make no range") } - let parent = WorktreeDiscovery.git(root, ["rev-parse", "--verify", "--quiet", "\(older)^"])?.trimmed - let base = parent?.isEmpty == false ? parent : nil - // No parent: a root commit starts from the empty tree, but a shallow clone only hides the parent. - if base == nil, WorktreeDiscovery.git(root, ["rev-parse", "--is-shallow-repository"])?.trimmed != "false" { - return .failed("The parent of \(older.prefix(10)) is not in \(place), a shallow clone (fetch more history first)") + guard let base = parent(of: older, in: root) else { + return .failed(missingParent(older, in: root)) } - let count = WorktreeDiscovery.git(root, ["rev-list", "--count", base.map { "\($0)..\(newer)" } ?? newer])?.trimmed ?? "?" + let count = WorktreeDiscovery.git(root, ["rev-list", "--count", base == emptyTree ? newer : "\(base)..\(newer)"])?.trimmed ?? "?" let label = "\(older.prefix(10))..\(newer.prefix(10)) (\(count) commits)" - return .found(Found(root: root, sha: newer, title: label, base: base ?? emptyTree)) + return .found(Found(root: root, sha: newer, title: label, base: base)) + } + + /// The first parent of `sha` in the checkout at `root`, the base of a range that starts at `sha`: git's empty + /// tree for a root commit, nil when a shallow clone only hides the parent (`missingParent`). Runs git, so off + /// the main thread. + static func parent(of sha: String, in root: String) -> String? { + if let parent = WorktreeDiscovery.git(root, ["rev-parse", "--verify", "--quiet", "\(sha)^"])?.trimmed, !parent.isEmpty { + return parent + } + // No parent: a root commit starts from the empty tree, but a shallow clone only hides the parent. + return WorktreeDiscovery.git(root, ["rev-parse", "--is-shallow-repository"])?.trimmed == "false" ? emptyTree : nil + } + + /// Why `parent(of:in:)` found no parent of `sha`. + static func missingParent(_ sha: String, in root: String) -> String { + "The parent of \(sha.prefix(10)) is not in \((root as NSString).abbreviatingWithTildeInPath), a shallow clone (fetch more history first)" } /// Whether the checkout at `root` holds every one of `shas` as a commit. diff --git a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift index 15c7a88cc9..2c07d71114 100644 --- a/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift +++ b/src/macos/GenesisTools/Sources/Parts/Hub/Hub/HubPRs.swift @@ -215,7 +215,7 @@ struct HubPRDetail: Decodable, Equatable { let branchMentions: [String]? } -private struct HubPRList: Decodable { +struct HubPRList: Decodable { struct Repo: Decodable { let repo: String let error: String? @@ -300,6 +300,13 @@ struct PRListNotice: Hashable { } } +/// The range a link's `commit=..` set (`PRsModel.rangeScope`) and the commits of the PR it names, by +/// their place in the list: the scope's base is a parent from git, which a branched PR may not list before them. +struct PRLinkedRange: Equatable { + let scope: DiffScope + let shas: Set +} + @MainActor final class PRsModel: ObservableObject { @Published private(set) var prs: [HubPR] = [] @@ -342,7 +349,9 @@ final class PRsModel: ObservableObject { @Published private(set) var commitScroll: PRCommitScroll? /// The range a link's `commit=..` set, so the commits list marks its commits even when it spans /// the whole PR; any other scope (the whole PR, one commit) is not it. - @Published private(set) var linkedRange: DiffScope? + @Published private(set) var linkedRange: PRLinkedRange? + /// The parent lookup of the last range link (`showLinkedRange`): an older link's answer sets no scope. + private var rangeLookup: UUID? /// What a link asked of a PR and could not get (its commit is not in the PR), in that PR's header until /// dismissed or another PR opens. Not in `errors`: every list load replaces those. @Published var notice: PRNotice? @@ -565,12 +574,21 @@ final class PRsModel: ObservableObject { errors.append(notice) } + private func clearDirectFailure() { + guard let failure = directFailure else { return } + errors.removeAll { $0 == failure } + directFailure = nil + } + /// Selects the PR `ref` names, now when the list has it, else once it loads. `reveal` opens a file /// (and a thread on it) in that PR's review once the review exists: after the list, the head fetch /// of a PR without a worktree, and the diff load, so one click is enough. `commit` (a sha or a prefix /// of 7 or more hex characters) selects that commit of the PR the same way (`pickPendingCommit`). func request(_ ref: HubPRRef, reveal: PRReveal? = nil, commit: String? = nil) { notice = nil + // A new link replaces the last one's failure, also when a listed row opens it and `openDirect` never runs. + clearDirectFailure() + rangeLookup = nil pendingReveal = reveal pendingCommit = nil if let commit { @@ -648,10 +666,7 @@ final class PRsModel: ObservableObject { /// its head commit. A cached row (`startWanted`) starts all three at once. private func openDirect(_ ref: HubPRRef, arg: String) { directOpen = (ref, selectedID) - if let failure = directFailure { - errors.removeAll { $0 == failure } - directFailure = nil - } + clearDirectFailure() Task { let span = HubPerf.begin("prs.show.direct", arg, awaits: true) let answer = await Task.detached(priority: .userInitiated) { () -> Result<(HubPR, HubPRDetail, Data), Error> in @@ -754,46 +769,67 @@ final class PRsModel: ObservableObject { } HubPerf.log("prs.commit \(shown.label) \(commit.sha.prefix(10))\(last.map { "..\($0.sha.prefix(10))" } ?? "") settled=\(settled)") if let last { - showLinkedRange(commit, last, in: commits, pr: shown, review: review) + showLinkedRange(commit, last, in: commits, pr: shown, ref: pending.ref, review: review) } else { showLinkedCommit(commit, prID: id, review: review) } } /// A link's `commit=..`: those commits of the PR, both ends included, in either order. - func showLinkedRange(_ one: HubPRDetail.Commit, _ other: HubPRDetail.Commit, in commits: [HubPRDetail.Commit], pr: HubPR, review: ReviewModel) { + func showLinkedRange(_ one: HubPRDetail.Commit, _ other: HubPRDetail.Commit, in commits: [HubPRDetail.Commit], pr: HubPR, ref: HubPRRef, + review: ReviewModel) { guard let a = commits.firstIndex(of: one), let b = commits.firstIndex(of: other) else { return } HubDefaults.store.set(true, forKey: "hub.prs.showDiff") HubDefaults.store.set(false, forKey: "hub.prs.fold.commits") commitScroll = PRCommitScroll(prID: pr.id, sha: commits[max(a, b)].sha) - let scope = Self.rangeScope(commits, first: min(a, b), last: max(a, b), pr: pr, whole: Self.scope(pr, detail: details[pr.id], fetch: fetchedHead(pr))) - linkedRange = scope - if review.scope == scope { return } - review.setScope(scope) + let (first, last) = (min(a, b), max(a, b)) + let whole = Self.scope(pr, detail: details[pr.id], fetch: fetchedHead(pr)) + func show(from parent: String?) { + let range = Self.rangeScope(commits, first: first, last: last, parent: parent, pr: pr, whole: whole) + linkedRange = range + if review.scope == range.scope { return } + review.setScope(range.scope) + } + guard first > 0 else { return show(from: nil) } + // The first parent from git, not the commit listed before: a branched PR's list is no line of ancestry. + let sha = commits[first].sha + let root = review.repo.path + let lookup = UUID() + rangeLookup = lookup + Task { + let parent = await Task.detached(priority: .userInitiated) { HubCommitLookup.parent(of: sha, in: root) }.value + guard rangeLookup == lookup, self.review === review else { return } + rangeLookup = nil + guard let parent else { + notice = PRNotice(ref: ref, text: HubCommitLookup.missingParent(sha, in: root)) + return + } + show(from: parent) + } } - /// Commits `first...last` of a PR (oldest first, as the host lists them): from the commit before - /// `first`, or from the PR's own base when `first` is its first commit, to `last`. - nonisolated static func rangeScope(_ commits: [HubPRDetail.Commit], first: Int, last: Int, pr: HubPR, whole: DiffScope) -> DiffScope { + /// Commits `first...last` of a PR (oldest first, as the host lists them): from `parent`, the first parent of + /// `first` read from git (`HubCommitLookup.parent`), or from the PR's own base when `first` is its first commit + /// (`parent` nil), to `last`. + nonisolated static func rangeScope(_ commits: [HubPRDetail.Commit], first: Int, last: Int, parent: String?, pr: HubPR, + whole: DiffScope) -> PRLinkedRange { let head = commits[last].sha let label = "\(pr.label) \(commits[first].sha.prefix(7))..\(head.prefix(7)) (\(Plural.count(last - first + 1, "commit")))" - if first > 0 { - return .range(base: commits[first - 1].sha, head: head, label: label) + let shas = Set(commits[first...last].map(\.sha)) + if let parent { + return PRLinkedRange(scope: .range(base: parent, head: head, label: label), shas: shas) } if case .range(let base, _, _, let fallback) = whole { - return .range(base: base, head: head, label: label, fallbackBase: fallback) + return PRLinkedRange(scope: .range(base: base, head: head, label: label, fallbackBase: fallback), shas: shas) } - return .range(base: "\(commits[first].sha)^", head: head, label: label) + return PRLinkedRange(scope: .range(base: "\(commits[first].sha)^", head: head, label: label), shas: shas) } - /// The commits of `commits` the range a link set (`linked`, from `rangeScope`) shows, for the commits list - /// to mark, all of them for a range over the whole PR; empty for any scope the link did not set. - nonisolated static func rangeShas(_ scope: DiffScope, linked: DiffScope?, in commits: [HubPRDetail.Commit]) -> Set { - guard scope == linked, case .range(let base, let head, _, _) = scope, - let last = commits.firstIndex(where: { $0.sha == head }) else { return [] } - let first = commits.firstIndex(where: { $0.sha == base }).map { $0 + 1 } ?? 0 - guard first <= last else { return [] } - return Set(commits[first...last].map(\.sha)) + /// The commits the range a link set (`linked`, from `rangeScope`) shows, for the commits list to mark, all of + /// them for a range over the whole PR; empty for any scope the link did not set. + nonisolated static func rangeShas(_ scope: DiffScope, linked: PRLinkedRange?) -> Set { + guard let linked, linked.scope == scope else { return [] } + return linked.shas } /// A repeated link reveals the diff and scrolls again even when its commit is already selected. @@ -1035,7 +1071,7 @@ final class PRsModel: ObservableObject { /// Shows `list`, animating rows in and out. `flash` marks new and updated rows once, only when /// the list on screen is the same list refreshed (not another state or search). - private func apply(_ list: HubPRList, key: String, flash: Bool) { + func apply(_ list: HubPRList, key: String, flash: Bool) { let sameList = appliedKey == key appliedKey = key var sorted = list.prs.sorted { ($0.updatedAt ?? "") > ($1.updatedAt ?? "") } @@ -2396,7 +2432,7 @@ struct PRCommitsSection: View { var body: some View { let shown = shownSha // A range link's commits, all marked. - let range = PRsModel.rangeShas(review.scope, linked: prs.linkedRange, in: commits) + let range = PRsModel.rangeShas(review.scope, linked: prs.linkedRange) PRSection(title: "Commits", count: commits.count, folded: $folded, flat: true, trailing: shown == nil && range.isEmpty ? nil : AnyView( Button("Whole PR") { prs.showWholePR() } .buttonStyle(.genHoverPlain()) diff --git a/src/macos/GenesisTools/Tests/HubLogicTests.swift b/src/macos/GenesisTools/Tests/HubLogicTests.swift index d7f3136f11..bc30eb6e13 100644 --- a/src/macos/GenesisTools/Tests/HubLogicTests.swift +++ b/src/macos/GenesisTools/Tests/HubLogicTests.swift @@ -161,19 +161,67 @@ final class HubLogicTests: XCTestCase { """ let pr = try JSONDecoder().decode(HubPR.self, from: Data(json.utf8)) let whole = DiffScope.range(base: "base000", head: "d000000", label: "whole", fallbackBase: "origin/main") - let middle = PRsModel.rangeScope(commits, first: 1, last: 2, pr: pr, whole: whole) - XCTAssertEqual(middle, .range(base: "a000000", head: "c000000", label: "!7 b000000..c000000 (2 commits)")) - XCTAssertEqual(PRsModel.rangeShas(middle, linked: middle, in: commits), ["b000000", "c000000"]) - let opening = PRsModel.rangeScope(commits, first: 0, last: 1, pr: pr, whole: whole) - XCTAssertEqual(opening, .range(base: "base000", head: "b000000", label: "!7 a000000..b000000 (2 commits)", fallbackBase: "origin/main")) - XCTAssertEqual(PRsModel.rangeShas(opening, linked: opening, in: commits), ["a000000", "b000000"]) + let middle = PRsModel.rangeScope(commits, first: 1, last: 2, parent: "a000000", pr: pr, whole: whole) + XCTAssertEqual(middle.scope, .range(base: "a000000", head: "c000000", label: "!7 b000000..c000000 (2 commits)")) + XCTAssertEqual(PRsModel.rangeShas(middle.scope, linked: middle), ["b000000", "c000000"]) + let opening = PRsModel.rangeScope(commits, first: 0, last: 1, parent: nil, pr: pr, whole: whole) + XCTAssertEqual(opening.scope, .range(base: "base000", head: "b000000", label: "!7 a000000..b000000 (2 commits)", fallbackBase: "origin/main")) + XCTAssertEqual(PRsModel.rangeShas(opening.scope, linked: opening), ["a000000", "b000000"]) // A link over every commit of the PR marks them all; the whole PR itself, or another range, marks none. - let span = PRsModel.rangeScope(commits, first: 0, last: 3, pr: pr, whole: whole) - XCTAssertEqual(PRsModel.rangeShas(span, linked: span, in: commits), Set(commits.map(\.sha))) - XCTAssertEqual(PRsModel.rangeShas(whole, linked: span, in: commits), [], "the whole PR marks no commit") - XCTAssertEqual(PRsModel.rangeShas(whole, linked: nil, in: commits), []) - XCTAssertEqual(PRsModel.rangeShas(middle, linked: opening, in: commits), [], "only the range the link set") - XCTAssertEqual(PRsModel.rangeShas(.commit(sha: "b000000", title: "b"), linked: span, in: commits), []) + let span = PRsModel.rangeScope(commits, first: 0, last: 3, parent: nil, pr: pr, whole: whole) + XCTAssertEqual(PRsModel.rangeShas(span.scope, linked: span), Set(commits.map(\.sha))) + XCTAssertEqual(PRsModel.rangeShas(whole, linked: span), [], "the whole PR marks no commit") + XCTAssertEqual(PRsModel.rangeShas(whole, linked: nil), []) + XCTAssertEqual(PRsModel.rangeShas(middle.scope, linked: opening), [], "only the range the link set") + XCTAssertEqual(PRsModel.rangeShas(.commit(sha: "b000000", title: "b"), linked: span), []) + } + + /// A PR with a side branch merged in: the forge lists its commits oldest first, so the side commit sits between + /// two commits of the main line. A range that starts after it diffs from the first commit's parent in git, not + /// from the listed commit before it, whose fork point would bring in a commit the range does not hold. + func testARangeLinkInABranchedPRDiffsFromItsFirstCommitsParent() throws { + let folder = FileManager.default.temporaryDirectory.appendingPathComponent("hub-branched-range-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: folder, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: folder) } + let path = folder.resolvingSymlinksInPath().path + // No hooks and no signing from the machine's global git config. + let git = { (args: [String]) -> String? in + WorktreeDiscovery.git(path, ["-c", "user.name=Fixture", "-c", "user.email=fixture@example.com", "-c", "core.hooksPath=/dev/null", + "-c", "commit.gpgsign=false"] + args)?.trimmed + } + func commitFile(_ name: String) throws -> String { + try "\(name)\n".write(toFile: "\(path)/\(name)", atomically: true, encoding: .utf8) + XCTAssertNotNil(git(["add", name])) + XCTAssertNotNil(git(["commit", "-q", "-m", name])) + return try XCTUnwrap(git(["rev-parse", "HEAD"])) + } + XCTAssertNotNil(git(["init", "-q", "-b", "main"])) + let root = try commitFile("base.txt") + let first = try commitFile("a1.txt") + let second = try commitFile("a2.txt") + let third = try commitFile("a3.txt") + XCTAssertNotNil(git(["checkout", "-q", "-b", "side", root])) + let side = try commitFile("s.txt") + XCTAssertNotNil(git(["checkout", "-q", "main"])) + XCTAssertNotNil(git(["merge", "-q", "--no-ff", "--no-edit", "side"])) + let merge = try XCTUnwrap(git(["rev-parse", "HEAD"])) + + XCTAssertEqual(HubCommitLookup.parent(of: second, in: path), first) + XCTAssertEqual(HubCommitLookup.parent(of: root, in: path), HubCommitLookup.emptyTree, "a root commit starts from the empty tree") + + let commit = { (sha: String) in HubPRDetail.Commit(sha: sha, title: sha, author: nil, authorLogin: nil, date: nil, body: nil) } + let commits = [first, side, second, third, merge].map(commit) + let json = """ + {"repo":"app","repoRoot":null,"origin":{"kind":"github","host":"github.com","web":"https://github.com/acme/app"}, + "number":7,"title":"t","state":"OPEN","draft":false,"author":"a","headBranch":"feat/x","baseBranch":"main", + "url":"https://github.com/acme/app/pull/7","labels":[],"reviewers":[],"headSha":"\(merge)","crossRepository":false,"headRepo":null} + """ + let pr = try JSONDecoder().decode(HubPR.self, from: Data(json.utf8)) + let whole = DiffScope.range(base: root, head: merge, label: "whole") + let range = PRsModel.rangeScope(commits, first: 2, last: 3, parent: HubCommitLookup.parent(of: second, in: path), pr: pr, whole: whole) + XCTAssertEqual(PRsModel.rangeShas(range.scope, linked: range), [second, third]) + let snapshot = try GitWorkingTreeSource(repo: URL(fileURLWithPath: path)).load(scope: range.scope) + XCTAssertEqual(snapshot.files.map(\.path).sorted(), ["a2.txt", "a3.txt"], "the side commit's fork point would add a1.txt") } func testAForgePathFindsItsCheckoutInAnyCaseAndForm() { @@ -501,6 +549,31 @@ final class HubLogicTests: XCTestCase { XCTAssertEqual(model.errors.map(\.text), ["Could not list the PRs/MRs: gh: network down"]) } + /// The next link replaces a failed link's notice, also when a listed row opens it without a direct fetch, and a + /// list that lands later does not bring the old notice back. + @MainActor + func testALinkOpenedFromAListedRowClearsTheFailedLinkBeforeIt() throws { + // No checkout (repoRoot null): selecting the row starts no CLI fetch in this logic test. + let json = """ + {"prs":[{"repo":"app","repoRoot":null,"origin":{"kind":"github","host":"github.com","web":"https://github.com/acme/app"}, + "number":7,"title":"t","state":"OPEN","draft":false,"author":"alice","headBranch":"feature","baseBranch":"develop", + "url":"https://github.com/acme/app/pull/7","labels":[],"reviewers":[],"headSha":null,"crossRepository":false,"headRepo":null}], + "repos":[]} + """ + let list = try JSONDecoder().decode(HubPRList.self, from: Data(json.utf8)) + let model = PRsModel() + model.apply(list, key: "fixture", flash: false) + let failed = PRListNotice("Could not open acme/app#99: no such PR") + model.directFailed(failed) + XCTAssertEqual(model.errors, [failed]) + + model.request(try XCTUnwrap(HubPRRef("acme/app#7"))) + XCTAssertEqual(model.selected?.number, 7) + XCTAssertEqual(model.errors, [], "the link opened its PR, so the previous link's failure no longer shows") + model.apply(list, key: "fixture", flash: false) + XCTAssertEqual(model.errors, [], "a list that lands after the newer link does not bring the old failure back") + } + func testALiveLaunchIsNotScripted() { let request = HubRequest(["--session", "abc", "--no-activate"]) XCTAssertFalse(request.isScripted) diff --git a/src/macos/GenesisTools/Tests/WindowCycleTests.swift b/src/macos/GenesisTools/Tests/WindowCycleTests.swift index 826516e7fc..897def581b 100644 --- a/src/macos/GenesisTools/Tests/WindowCycleTests.swift +++ b/src/macos/GenesisTools/Tests/WindowCycleTests.swift @@ -88,18 +88,26 @@ final class WindowCycleTests: XCTestCase { func request(_ order: [Int], index: Int, modifiers: Int = option) -> [AnyHashable: Any] { ["order": order, "index": index, "modifiers": modifiers] } - XCTAssertEqual(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 1), pid: 2, window: 20), + let known: [WindowCycle.Candidate] = [.init(pid: 1, number: 10), .init(pid: 2, number: 20), .init(pid: 3, number: 30)] + XCTAssertEqual(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 1), pid: 2, window: 20, known: known), WindowCycle.Session(order: [.init(pid: 1, number: 10), .init(pid: 2, number: 20)], index: 1, modifiers: .option)) - XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: -1), pid: 2, window: 20), "a negative index") - XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 2), pid: 2, window: 20), "past the end") - XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 1, modifiers: -1), pid: 2, window: 20)) - XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 1, modifiers: 0), pid: 2, window: 20), + XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: -1), pid: 2, window: 20, known: known), "a negative index") + XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 2), pid: 2, window: 20, known: known), "past the end") + XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 1, modifiers: -1), pid: 2, window: 20, known: known)) + XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 1, modifiers: 0), pid: 2, window: 20, known: known), "no modifier: no release would end the run") - XCTAssertNil(WindowCycle.requestedSession(request([1, 10, Int(Int32.max) + 1, 20], index: 1), pid: 2, window: 20)) - XCTAssertNil(WindowCycle.requestedSession(request([], index: 0), pid: 2, window: 20)) - XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 0), pid: 2, window: 20), + XCTAssertNil(WindowCycle.requestedSession(request([1, 10, Int(Int32.max) + 1, 20], index: 1), pid: 2, window: 20, known: known)) + XCTAssertNil(WindowCycle.requestedSession(request([], index: 0), pid: 2, window: 20, known: known)) + XCTAssertNil(WindowCycle.requestedSession(request([1, 10, 2, 20], index: 0), pid: 2, window: 20, known: known), "the landed window is not the one to raise") + // A window the sender made up (another app's pid) is dropped, so no press hands the activation to it. + XCTAssertEqual(WindowCycle.requestedSession(request([1, 10, 99, 90, 2, 20, 3, 30], index: 2), pid: 2, window: 20, known: known), + WindowCycle.Session(order: [.init(pid: 1, number: 10), .init(pid: 2, number: 20), .init(pid: 3, number: 30)], + index: 1, modifiers: .option)) + XCTAssertNil(WindowCycle.requestedSession(request([99, 90, 2, 20], index: 1), pid: 2, window: 20, known: known), + "only unknown windows besides the landed one: a plain raise") + XCTAssertEqual(WindowCycle.focusedWindow(["pid": 2, "window": 20], own: 1), .init(pid: 2, number: 20)) XCTAssertNil(WindowCycle.focusedWindow(["pid": 1, "window": 20], own: 1), "this face's own broadcast") XCTAssertNil(WindowCycle.focusedWindow(["pid": Int(Int32.max) + 1, "window": 20], own: 1)) diff --git a/src/macos/GenesisTools/web/diff-viewer/main.ts b/src/macos/GenesisTools/web/diff-viewer/main.ts index d437dca131..ae7447b016 100644 --- a/src/macos/GenesisTools/web/diff-viewer/main.ts +++ b/src/macos/GenesisTools/web/diff-viewer/main.ts @@ -2446,7 +2446,9 @@ function renderImage(image: BridgeImage): HTMLElement { picture.decoding = "async"; picture.alt = label; picture.addEventListener("error", () => { - frame.replaceChildren(element("div", css.dim, "This image cannot be shown")); + frame.replaceChildren( + element("div", css.dim, "This image cannot be shown: it is gone or too large to preview") + ); }); picture.src = url; frame.appendChild(picture); diff --git a/src/macos/commands/clones/reclaim.test.ts b/src/macos/commands/clones/reclaim.test.ts index a3ccc444ca..b42524bfd3 100644 --- a/src/macos/commands/clones/reclaim.test.ts +++ b/src/macos/commands/clones/reclaim.test.ts @@ -386,6 +386,12 @@ describe("createReclaimCommand", () => { await run(["presets", "auto", "nightly", "--off"]); expect(getPreset("nightly")?.auto?.enabled).toBe(false); + // Limits passed with --off are saved, not dropped, and the next --on runs with them. + await run(["presets", "auto", "nightly", "--off", "--max-bytes", "none", "--max-files", "7"]); + expect(getPreset("nightly")?.auto).toEqual({ enabled: false, maxFiles: 7 }); + await run(["presets", "auto", "nightly", "--on"]); + expect(getPreset("nightly")?.auto).toEqual({ enabled: true, maxFiles: 7 }); + const bad = await run(["presets", "auto", "nightly", "--max-bytes", "lots"]); expect(bad.code).toBe(1); const unknown = await run(["presets", "auto", "missing", "--on"]); diff --git a/src/macos/commands/clones/reclaim.ts b/src/macos/commands/clones/reclaim.ts index 5799b3df83..0b53ad36eb 100644 --- a/src/macos/commands/clones/reclaim.ts +++ b/src/macos/commands/clones/reclaim.ts @@ -15,6 +15,7 @@ import { getPreset, listPresets, type Preset, + type PresetAuto, removePreset, savePreset, selectorFromPreset, @@ -731,14 +732,23 @@ function createPresetsCommand(): Command { } const limits = { maxBytes, maxFiles }; + const changesLimits = limits.maxBytes !== undefined || limits.maxFiles !== undefined; let next: Preset | null = preset; if (opts.on) { next = updatePresetAuto(id, armedAuto(preset.auto, limits)); } else if (opts.off) { - next = preset.auto - ? updatePresetAuto(id, { ...preset.auto, enabled: false, dryRunAt: undefined }) - : preset; - } else if (preset.auto && (limits.maxBytes !== undefined || limits.maxFiles !== undefined)) { + // Limits passed with --off are saved too, so the next --on runs with them. + if (preset.auto || changesLimits) { + const previous: Partial = preset.auto ?? {}; + const { maxBytes: _bytes, maxFiles: _files, ...rest } = previous; + next = updatePresetAuto(id, { + ...rest, + enabled: false, + dryRunAt: undefined, + ...mergeLimits(preset.auto, limits), + }); + } + } else if (preset.auto && changesLimits) { const { maxBytes: _bytes, maxFiles: _files, ...rest } = preset.auto; next = updatePresetAuto(id, { ...rest, ...mergeLimits(preset.auto, limits) }); } diff --git a/src/ts/lib/collect.ts b/src/ts/lib/collect.ts index efa8767c44..46172a7109 100644 --- a/src/ts/lib/collect.ts +++ b/src/ts/lib/collect.ts @@ -1,5 +1,5 @@ import { type Dirent, existsSync, readdirSync, statSync } from "node:fs"; -import { isAbsolute, join, parse, relative, resolve } from "node:path"; +import { isAbsolute, join, parse, relative, resolve, sep } from "node:path"; import { logger } from "@genesiscz/utils/logger"; const SOURCE_EXTENSIONS = [".ts", ".tsx", ".mts", ".cts"]; @@ -56,8 +56,13 @@ export function collectFiles(input: string, options: CollectOptions = {}): strin ) ); const ignore = options.ignore ?? []; + // A match below node_modules or build is dropped here: walk() returns a matched file before it checks folders. const matches = [...new Bun.Glob(pattern).scanSync({ cwd, absolute: true, onlyFiles: false })].filter( - (match) => !ignore.some((needle) => relative(base, match).includes(needle)) + (match) => + !ignore.some((needle) => relative(base, match).includes(needle)) && + !relative(base, match) + .split(sep) + .some((segment) => SKIP_DIRECTORIES.has(segment)) ); const found = matches.flatMap((match) => walk(match, { base, includeTests: options.tests === true, ignore })); return [...new Set(found)].filter((file) => isSource(file, options.tests === true)).sort(); diff --git a/src/ts/lib/ts.test.ts b/src/ts/lib/ts.test.ts index 3d422bbf4b..8a6924028a 100644 --- a/src/ts/lib/ts.test.ts +++ b/src/ts/lib/ts.test.ts @@ -640,6 +640,14 @@ describe("the skeleton command's filters and rendering", () => { expect(collectFiles(join(root, "glob/none*.ts"))).toEqual([]); }); + it("drops a glob's matches below node_modules or build, as a directory walk does", () => { + write("glob-skip/src/a.ts", "export const a = 1;\n"); + write("glob-skip/node_modules/pkg/b.ts", "export const b = 2;\n"); + write("glob-skip/build/c.ts", "export const c = 3;\n"); + + expect(collectFiles(join(root, "glob-skip/**/*.ts"))).toEqual([join(root, "glob-skip/src/a.ts")]); + }); + it("applies --ignore to a glob's matches below its literal folders, also outside the cwd", () => { write("glob-ignore/keep.ts", "export const a = 1;\n"); write("glob-ignore/out.ts", "export const b = 2;\n"); diff --git a/src/utils/DashboardApp/readiness.test.ts b/src/utils/DashboardApp/readiness.test.ts index 79dfff242d..b30ccf2f7d 100644 --- a/src/utils/DashboardApp/readiness.test.ts +++ b/src/utils/DashboardApp/readiness.test.ts @@ -92,6 +92,41 @@ describe("a server that answers with an error", () => { } }); + test("a 500 followed by a gateway status or a stopped server times out not ready, not broken", async () => { + let calls = 0; + const gateway = Bun.serve({ + port: 0, + fetch: () => new Response("upstream", { status: calls++ === 0 ? 500 : 503 }), + }); + let stoppedCalls = 0; + const stopped = Bun.serve({ + port: 0, + fetch: () => { + if (stoppedCalls++ === 0) { + setTimeout(() => stopped.stop(true), 0); + } + + return new Response("boom", { status: 500 }); + }, + }); + try { + const results = await Promise.all( + [gateway.port, stopped.port].map((port) => + waitForUrlReady(`http://127.0.0.1:${port}/`, 1_000, { brokenGraceMs: 10_000 }) + ) + ); + for (const result of results) { + expect(result.ready).toBe(false); + expect(result.broken).toBeUndefined(); + } + expect(results[0].detail).toContain("last status 503"); + expect(stoppedCalls).toBe(1); + } finally { + gateway.stop(true); + stopped.stop(true); + } + }); + test("the server's error and the top of its stack come from the log, ANSI stripped", () => { const logFile = join(tmpdir(), `readiness-excerpt-${process.pid}-${Date.now()}.log`); writeFileSync( diff --git a/src/utils/DashboardApp/readiness.ts b/src/utils/DashboardApp/readiness.ts index 1cf75f15e6..4cc4b4729d 100644 --- a/src/utils/DashboardApp/readiness.ts +++ b/src/utils/DashboardApp/readiness.ts @@ -139,8 +139,13 @@ export async function waitForUrlReady( } else if (isHttpServingStatus(res.status)) { // 4xx still means the app is serving; 502/503/504 means proxy-without-upstream. return { ready: true, detail: `http ${res.status}` }; + } else { + // A gateway status: nothing behind the proxy answers now, so an earlier 500 no longer means serving. + broken = undefined; } } catch (err) { + // The server stopped answering: an earlier 500 must not be reported as a listener at the deadline. + broken = undefined; lastError = err; logger.debug({ err, url }, "http readiness probe failed"); }