From b70833692224813985cf44a05fb5170398f2c2bb Mon Sep 17 00:00:00 2001 From: Yao Jingxi <23722032@bjtu.edu.cn> Date: Tue, 1 Sep 2026 21:47:11 +0800 Subject: [PATCH 1/4] feat(macos): align branch popup rows with IDEA's action menu The branch popup checked out a reference on a plain click, so scanning the list could switch the working tree by accident; checkout was otherwise only reachable through the right-click menu. Branch rows now present their actions through a native pop-up menu, with Checkout as one explicit entry. The native menu also supplies IDEA's hover-safety path: once a row's menu is open, moving to another row opens that menu without a click. Rows whose branch or upstream name the fixed popup width truncates expose the full pair as a hover tooltip. --- .../Views/Git/BranchSwitcherPopover.swift | 156 ++++++++++++++---- .../BranchSwitcherPopoverBehaviorTests.swift | 79 +++++++++ 2 files changed, 204 insertions(+), 31 deletions(-) create mode 100644 macos/Tests/LitheTests/BranchSwitcherPopoverBehaviorTests.swift diff --git a/macos/Sources/Lithe/Views/Git/BranchSwitcherPopover.swift b/macos/Sources/Lithe/Views/Git/BranchSwitcherPopover.swift index 5686d4dd..de2a5683 100644 --- a/macos/Sources/Lithe/Views/Git/BranchSwitcherPopover.swift +++ b/macos/Sources/Lithe/Views/Git/BranchSwitcherPopover.swift @@ -364,6 +364,9 @@ struct BranchSwitcherPopover: View { .lithePointer() } + /// A branch line. Clicking it opens the reference's action menu instead of + /// checking out directly, matching IDEA: checkout is an explicit menu entry, + /// so a stray click on the list can never switch the working tree. private func branchRow( _ reference: GitReference, indented: Bool, @@ -371,46 +374,106 @@ struct BranchSwitcherPopover: View { ) -> some View { let highlightsCurrent = presentation == .recent && reference.isCurrent - return Button { - guard !reference.isCurrent else { return } - isPresented = false - Task { await model.checkoutReference(reference) } - } label: { - HStack(spacing: 8) { - Image(systemName: referenceIcon(reference, marksCurrent: presentation == .recent)) - .font(.system(size: 11.5)) - .foregroundStyle(highlightsCurrent ? LitheTheme.warning : LitheTheme.secondaryText) - .frame(width: 17) - Text(branchDisplayName(reference, presentation: presentation)) - .font(.system(size: 12.5)) - .foregroundStyle(LitheTheme.primaryText) - .lineLimit(1) - .truncationMode(.middle) - Spacer(minLength: 10) - if let upstream = reference.upstreamShortName { - Text(upstream) + return BranchActionMenuRow( + label: { + HStack(spacing: 8) { + Image(systemName: referenceIcon(reference, marksCurrent: presentation == .recent)) .font(.system(size: 11.5)) - .foregroundStyle(LitheTheme.secondaryText) + .foregroundStyle(highlightsCurrent ? LitheTheme.warning : LitheTheme.secondaryText) + .frame(width: 17) + Text(branchDisplayName(reference, presentation: presentation)) + .font(.system(size: 12.5)) + .foregroundStyle(LitheTheme.primaryText) .lineLimit(1) .truncationMode(.middle) - } - if !reference.isCurrent { + Spacer(minLength: 10) + if let upstream = reference.upstreamShortName { + Text(upstream) + .font(.system(size: 11.5)) + .foregroundStyle(LitheTheme.secondaryText) + .lineLimit(1) + .truncationMode(.middle) + } Image(systemName: "chevron.right") .font(.system(size: 8, weight: .bold)) .foregroundStyle(LitheTheme.secondaryText) } + .padding(.leading, branchRowLeadingPadding(indented: indented, presentation: presentation)) + .padding(.trailing, 9) + .frame(maxWidth: .infinity, alignment: .leading) + .frame(height: Metrics.branchRowHeight) + .background(highlightsCurrent ? LitheTheme.subtleSelection : .clear) + .clipShape(RoundedRectangle(cornerRadius: 5)) + .contentShape(Rectangle()) + // Branch and upstream names are truncated to keep the row width + // fixed, so the untruncated pair is only reachable on hover. + .help(branchRowTooltip(reference)) + }, + menuContent: { branchActionMenu(for: reference) } + ) + .disabled(model.isPerformingBranchOperation) + } + + /// The full branch name, plus its upstream when tracked, for rows whose text + /// the fixed popup width truncates. + private func branchRowTooltip(_ reference: GitReference) -> String { + guard let upstream = reference.upstreamShortName else { return reference.shortName } + return "\(reference.shortName) → \(upstream)" + } + + /// The per-reference action list, ordered like IDEA's branch menu: creation + /// and comparison first, then checkout and integration, then destructive + /// entries last. + @ViewBuilder + private func branchActionMenu(for reference: GitReference) -> some View { + Button("New Branch from '\(reference.shortName)'…") { + dismissAndRun { onNewBranch(reference) } + } + + Button("Show Diff with Working Tree") { + dismissAndRun { Task { await model.showComparisonWithWorkingTree(for: reference) } } + } + + if let current = model.currentGitReference, current.id != reference.id { + Button("Compare with Current Branch") { + dismissAndRun { Task { await model.showComparison(from: reference, to: current) } } + } + } + + if !reference.isCurrent { + Divider() + + Button("Checkout") { + dismissAndRun { Task { await model.checkoutReference(reference) } } + } + } + + if reference.kind != .tag { + Divider() + + Button("Update") { + dismissAndRun { Task { await model.updateCurrentBranch(reference) } } + } + + Button("Push…") { + dismissAndRun { onPush(reference) } + } + } + + if reference.kind == .local, !reference.isCurrent { + Divider() + + Button("Delete") { + dismissAndRun { Task { await model.deleteBranch(reference) } } } - .padding(.leading, branchRowLeadingPadding(indented: indented, presentation: presentation)) - .padding(.trailing, 9) - .frame(maxWidth: .infinity, alignment: .leading) - .frame(height: Metrics.branchRowHeight) - .background(highlightsCurrent ? LitheTheme.subtleSelection : .clear) - .clipShape(RoundedRectangle(cornerRadius: 5)) - .contentShape(Rectangle()) } - .buttonStyle(.plain) - .lithePointer() - .disabled(model.isPerformingBranchOperation) + } + + /// Closes the popover before running a branch action so the action's own + /// sheet or dialog is not presented behind a popover that is about to go away. + private func dismissAndRun(_ action: @escaping () -> Void) { + isPresented = false + action() } private var recentReferences: [GitReference] { @@ -577,6 +640,37 @@ struct BranchSwitcherPopover: View { } } +/// A branch row that surfaces its actions through a native pop-up menu rather +/// than a direct checkout. +/// +/// Using `SwiftUI.Menu` with `.menuStyle(.borderlessButton)` produces a native +/// NSMenu, which works correctly inside the outer popover, positions itself to +/// avoid screen edges, and provides the hover-safety path that IDEA exposes: +/// once any row's menu is open, moving the cursor to another row opens that +/// menu immediately without a click. +private struct BranchActionMenuRow: View { + @ViewBuilder let label: () -> Label + @ViewBuilder let menuContent: () -> MenuContent + + @State private var isHovering = false + + var body: some View { + SwiftUI.Menu { + menuContent() + } label: { + label() + .background(isHovering ? LitheTheme.subtleSelection : .clear) + .clipShape(RoundedRectangle(cornerRadius: 5)) + } + .menuStyle(.borderlessButton) + .menuIndicator(.hidden) + // Constrain to the list width so the menu button does not stretch. + .fixedSize(horizontal: false, vertical: true) + .lithePointer() + .onHover { isHovering = $0 } + } +} + private enum BranchRowPresentation { case recent case grouped diff --git a/macos/Tests/LitheTests/BranchSwitcherPopoverBehaviorTests.swift b/macos/Tests/LitheTests/BranchSwitcherPopoverBehaviorTests.swift new file mode 100644 index 00000000..f8222dd2 --- /dev/null +++ b/macos/Tests/LitheTests/BranchSwitcherPopoverBehaviorTests.swift @@ -0,0 +1,79 @@ +import Foundation +import Testing +@testable import Lithe + +/// Guards the branch popup's IDEA-aligned interaction contract. The rows are +/// SwiftUI views without a testable state surface, so these checks read the +/// source: the regression they protect against is a branch row silently going +/// back to checking out on a plain click, which switches the working tree from +/// a stray click while scanning the list. +@Suite("Branch switcher popover behavior") +struct BranchSwitcherPopoverBehaviorTests { + private static func popoverSource() throws -> String { + let repositoryRoot = URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() + .deletingLastPathComponent() + .deletingLastPathComponent() + let popoverURL = repositoryRoot.appendingPathComponent( + "Sources/Lithe/Views/Git/BranchSwitcherPopover.swift" + ) + return try String(contentsOf: popoverURL, encoding: .utf8) + } + + @Test + func branchRowsOpenAnActionMenuInsteadOfCheckingOutOnClick() throws { + let source = try Self.popoverSource() + + #expect( + source.contains("BranchActionMenuRow("), + "Branch rows must route through BranchActionMenuRow so a click opens the action menu." + ) + #expect( + source.contains("private func branchActionMenu(for reference: GitReference)"), + "The per-reference action list must exist for the menu to present." + ) + } + + @Test + func checkoutIsReachableOnlyAsAnExplicitMenuEntry() throws { + let source = try Self.popoverSource() + + // The single permitted checkout call site is the menu's Checkout entry. + let checkoutCallSites = source.components(separatedBy: "model.checkoutReference(").count - 1 + #expect( + checkoutCallSites == 1, + "Checkout must have exactly one call site, the explicit Checkout menu entry." + ) + + guard let checkoutRange = source.range(of: "model.checkoutReference(") else { + Issue.record("Expected a checkout call site in the branch popup.") + return + } + let precedingSource = source[source.startIndex.. Date: Tue, 1 Sep 2026 10:10:01 +0800 Subject: [PATCH 2/4] fix(ci): make Swift test watchdog stall-aware --- .../references/macos-swift.md | 9 +- .../scripts/run-swift-tests-with-timing.mjs | 137 +++++++++--- .../scripts/test-stability-macos.sh | 8 +- .../scripts/test-verify-test-stability.mjs | 203 ++++++++++++++++++ 4 files changed, 319 insertions(+), 38 deletions(-) diff --git a/.agents/skills/write-stable-tests/references/macos-swift.md b/.agents/skills/write-stable-tests/references/macos-swift.md index 83eeb2db..058de7fd 100644 --- a/.agents/skills/write-stable-tests/references/macos-swift.md +++ b/.agents/skills/write-stable-tests/references/macos-swift.md @@ -30,8 +30,13 @@ loaded, or invoked anywhere in this path. Use `./.agents/skills/write-stable-tests/scripts/test-stability-macos.sh`. It forces serial execution so the currently running test is unambiguous, records every Swift Testing/XCTest case, -warns about slow cases, and terminates the suite when one case exceeds its local -budget. Reports are written below `.artifacts/test-stability/`. Each run +warns about slow cases, and fails the run when a reported duration exceeds its +local budget. Because the runner writes to a block-buffered pipe, a finish line +can arrive late or be lost; the harness therefore never kills the runner on a +per-test timer. Instead a stall watchdog (`--stall-timeout-seconds`, default +120) terminates the runner only when it produces no output at all, and reports +the tests still awaiting a result without asserting a single culprit. Reports +are written below `.artifacts/test-stability/`. Each run produces JSON and raw logs for diagnosis, JUnit XML for CI tooling, and a self-contained HTML report for module and performance review. diff --git a/.agents/skills/write-stable-tests/scripts/run-swift-tests-with-timing.mjs b/.agents/skills/write-stable-tests/scripts/run-swift-tests-with-timing.mjs index a0e1b7cd..c9ed3261 100755 --- a/.agents/skills/write-stable-tests/scripts/run-swift-tests-with-timing.mjs +++ b/.agents/skills/write-stable-tests/scripts/run-swift-tests-with-timing.mjs @@ -1,6 +1,7 @@ #!/usr/bin/env node import { mkdirSync, writeFileSync, createWriteStream } from "node:fs"; +import { spawnSync } from "node:child_process"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { writeTestReportArtifacts } from "./generate-test-report.mjs"; @@ -65,6 +66,7 @@ function parseArguments(arguments_) { const options = { warnMs: 1000, maxMs: 15000, + stallTimeoutMs: 120000, suiteTimeoutMs: 600000, report: path.join(REPOSITORY_ROOT, ".artifacts/test-stability/macos-swift.json"), command: arguments_[separator + 1], @@ -74,7 +76,9 @@ function parseArguments(arguments_) { const argument = arguments_[index]; if (argument === "--warn-ms") options.warnMs = positiveInteger(arguments_[++index], "--warn-ms"); else if (argument === "--max-ms") options.maxMs = positiveInteger(arguments_[++index], "--max-ms"); - else if (argument === "--suite-timeout-ms") { + else if (argument === "--stall-timeout-ms") { + options.stallTimeoutMs = positiveInteger(arguments_[++index], "--stall-timeout-ms"); + } else if (argument === "--suite-timeout-ms") { options.suiteTimeoutMs = positiveInteger(arguments_[++index], "--suite-timeout-ms"); } else if (argument === "--report") options.report = path.resolve(arguments_[++index]); else throw new Error(`Unknown argument: ${argument}`); @@ -90,34 +94,61 @@ export async function run(options, { runProcessImpl = runProcess } = {}) { const active = new Map(); const records = []; let currentSuite = null; - let timedOutTest = null; - let testTimer = null; + let stalled = false; + let stallTimer = null; let terminateChild = () => {}; + let childPid = null; + let lastOutputAt = null; + let lastTestEventAt = null; + let stallSamplePath = null; + // Killing the runner from a per-test timer is unsound: swift test writes to a + // block-buffered pipe, so a finish line can sit (or be split mid-line) in the + // child's buffer long after the test completed, and the timer would blame an + // innocent test. Instead, per-test budgets are enforced after the run from + // the durations swift-testing itself reports, and this stall watchdog only + // guards against the runner producing no output at all. + const stallTimeoutMs = options.stallTimeoutMs ?? 120000; + // Best-effort thread-stack snapshot of the hung runner, taken before the + // SIGTERM destroys the evidence of where it was stuck. + const captureStallSample = () => { + if (process.platform !== "darwin" || !childPid) return; + const samplePath = options.report.replace(/\.json$/i, ".stall-sample.txt"); + try { + const sample = spawnSync("sample", [String(childPid), "2", "-file", samplePath], { + stdio: "ignore", + timeout: 15000, + }); + if (sample.status === 0) stallSamplePath = samplePath; + } catch { + // Sampling is diagnostics only; never let it break termination. + } + }; + const armStallTimer = () => { + if (stallTimer) clearTimeout(stallTimer); + stallTimer = setTimeout(() => { + stalled = true; + captureStallSample(); + terminateChild(); + }, stallTimeoutMs); + }; const recordLine = (line, stream) => { - log.write(`${stream}: ${line}\n`); + lastOutputAt = new Date().toISOString(); + log.write(`${lastOutputAt} ${stream}: ${line}\n`); + armStallTimer(); const suiteEvent = parseSwiftSuiteLine(line); if (suiteEvent?.event === "started") currentSuite = suiteEvent.name; else if (suiteEvent && suiteEvent.name === currentSuite) currentSuite = null; const event = parseSwiftTimingLine(line); if (!event) return; + lastTestEventAt = lastOutputAt; if (event.event === "started") { - const startedAt = performance.now(); - active.set(event.name, { startedAt, suite: currentSuite }); - if (testTimer) clearTimeout(testTimer); - testTimer = setTimeout(() => { - timedOutTest = { name: event.name, suite: currentSuite }; - terminateChild(); - }, options.maxMs); + active.set(event.name, { startedAt: performance.now(), suite: currentSuite }); return; } const activeTest = active.get(event.name); active.delete(event.name); - if (testTimer) { - clearTimeout(testTimer); - testTimer = null; - } records.push({ name: event.name, ...(activeTest?.suite ? { suite: activeTest.suite } : {}), @@ -130,12 +161,14 @@ export async function run(options, { runProcessImpl = runProcess } = {}) { }; const startedAt = new Date().toISOString(); + armStallTimer(); const childPromise = runProcessImpl({ command: options.command, args: options.commandArguments, cwd: REPOSITORY_ROOT, timeoutMs: options.suiteTimeoutMs, - onSpawn: ({ terminate }) => { + onSpawn: ({ pid, terminate }) => { + childPid = pid ?? null; terminateChild = terminate; }, onStdoutLine: (line) => recordLine(line, "stdout"), @@ -144,31 +177,56 @@ export async function run(options, { runProcessImpl = runProcess } = {}) { streamStderr: true, }); - const result = await childPromise; - if (testTimer) clearTimeout(testTimer); - await new Promise((resolve, reject) => { - log.once("error", reject); - log.end(resolve); - }); - - if (timedOutTest && !records.some((record) => record.name === timedOutTest.name)) { - records.push({ - name: timedOutTest.name, - ...(timedOutTest.suite ? { suite: timedOutTest.suite } : {}), - status: "timeout", - durationMs: options.maxMs, + // Clear the watchdog and settle the log stream even when spawn fails and the + // await throws; a leaked ref'd timer would keep this process alive for the + // full timeout, and an unsettled stream emits an unhandled error event. + let result; + let logError = null; + try { + result = await childPromise; + } finally { + if (stallTimer) clearTimeout(stallTimer); + stallTimer = null; + await new Promise((resolve) => { + log.once("error", (error) => { + logError ??= error; + resolve(); + }); + log.end(resolve); }); } - for (const [name, activeTest] of active) { + if (logError) throw logError; + + // Tests still in `active` either never finished or had their finish line cut + // off in the killed child's stdio buffer; report them without asserting that + // any single one of them is the culprit. + const unfinished = [...active.entries()]; + for (const [name, activeTest] of unfinished) { if (!records.some((record) => record.name === name)) { records.push({ name, ...(activeTest.suite ? { suite: activeTest.suite } : {}), - status: result.timedOut ? "timeout" : "incomplete", - durationMs: options.maxMs, + status: result.timedOut || stalled ? "timeout" : "incomplete", + durationMs: Math.round(performance.now() - activeTest.startedAt), }); } } + if (stalled) { + const unfinishedNames = unfinished.map(([name]) => name); + records.push({ + name: "Swift test runner stall", + suite: "Swift test runner", + status: "timeout", + durationMs: stallTimeoutMs, + details: + `The Swift runner produced no output for ${stallTimeoutMs}ms. ` + + (unfinishedNames.length > 0 + ? `Tests without a reported result: ${unfinishedNames.join(", ")}. ` + : "Every parsed test had reported a result; the runner likely hung during teardown or exit. ") + + `Last output at ${lastOutputAt ?? "never"}; last parsed test event at ${lastTestEventAt ?? "never"}.` + + (stallSamplePath ? ` Thread-stack sample of the hung runner: ${stallSamplePath}.` : ""), + }); + } if (result.timedOut && !records.some((record) => record.status === "timeout")) { records.push({ name: "Swift test suite timeout", @@ -190,11 +248,14 @@ export async function run(options, { runProcessImpl = runProcess } = {}) { command: [options.command, ...options.commandArguments], warnMs: options.warnMs, maxMs: options.maxMs, + stallTimeoutMs, suiteTimeoutMs: options.suiteTimeoutMs, process: { exitCode: result.code, signal: result.signal, timedOut: result.timedOut, + stalled, + terminationConfirmed: result.terminationConfirmed, durationMs: Math.round(result.durationMs), }, tests: records, @@ -207,10 +268,16 @@ export async function run(options, { runProcessImpl = runProcess } = {}) { console.log(`SLOW ${record.durationMs}ms ${record.name}`); } - if (timedOutTest) { - throw new Error(`Swift test exceeded ${options.maxMs}ms: ${timedOutTest.name}`); - } if (result.timedOut) throw new Error(`Swift test suite exceeded ${options.suiteTimeoutMs}ms.`); + if (stalled) { + const unfinishedNames = unfinished.map(([name]) => name); + throw new Error( + `Swift test runner produced no output for ${stallTimeoutMs}ms` + + (unfinishedNames.length > 0 + ? `; tests without a reported result: ${unfinishedNames.join(", ")}.` + : "; every parsed test had reported a result, so the runner likely hung during teardown or exit."), + ); + } if (records.length === 0) throw new Error("The Swift runner did not report any individual test durations."); if (overBudget.length > 0) throw new Error(`${overBudget.length} Swift test(s) exceeded the local budget.`); if (result.code !== 0) throw new Error(`Swift test command exited with code ${result.code}.`); diff --git a/.agents/skills/write-stable-tests/scripts/test-stability-macos.sh b/.agents/skills/write-stable-tests/scripts/test-stability-macos.sh index d4a9e4cc..e64f62c9 100755 --- a/.agents/skills/write-stable-tests/scripts/test-stability-macos.sh +++ b/.agents/skills/write-stable-tests/scripts/test-stability-macos.sh @@ -5,6 +5,7 @@ SCRIPT_DIR="${0:A:h}" ROOT_DIR="$(cd -- "$SCRIPT_DIR/../../../.." && pwd)" WARN_SECONDS=1 MAX_SECONDS=15 +STALL_TIMEOUT_SECONDS=120 SUITE_TIMEOUT_SECONDS=600 REPORT="$ROOT_DIR/.artifacts/test-stability/macos-swift.json" SWIFT_ARGS=() @@ -19,6 +20,10 @@ while (( $# > 0 )); do MAX_SECONDS="$2" shift 2 ;; + --stall-timeout-seconds) + STALL_TIMEOUT_SECONDS="$2" + shift 2 + ;; --suite-timeout-seconds) SUITE_TIMEOUT_SECONDS="$2" shift 2 @@ -48,7 +53,7 @@ done for argument in "${SWIFT_ARGS[@]}"; do if [[ "$argument" == "--parallel" ]]; then - print -u2 -- "--parallel is not allowed: per-test watchdog attribution requires serial execution." + print -u2 -- "--parallel is not allowed: per-test duration attribution requires serial execution." exit 2 fi done @@ -57,6 +62,7 @@ done node "$SCRIPT_DIR/run-swift-tests-with-timing.mjs" \ --warn-ms "$(( WARN_SECONDS * 1000 ))" \ --max-ms "$(( MAX_SECONDS * 1000 ))" \ + --stall-timeout-ms "$(( STALL_TIMEOUT_SECONDS * 1000 ))" \ --suite-timeout-ms "$(( SUITE_TIMEOUT_SECONDS * 1000 ))" \ --report "$REPORT" \ -- "$ROOT_DIR/scripts/test-macos.sh" --no-parallel "${SWIFT_ARGS[@]}" diff --git a/.agents/skills/write-stable-tests/scripts/test-verify-test-stability.mjs b/.agents/skills/write-stable-tests/scripts/test-verify-test-stability.mjs index 19d8f39b..74715ed4 100755 --- a/.agents/skills/write-stable-tests/scripts/test-verify-test-stability.mjs +++ b/.agents/skills/write-stable-tests/scripts/test-verify-test-stability.mjs @@ -254,6 +254,209 @@ try { rmSync(swiftTimeoutRoot, { recursive: true, force: true }); } +// Regression coverage for the CI misattribution incident: block-buffered pipes +// can swallow a finish line, so a stall must be reported as runner silence with +// the unfinished tests listed, never as "test X exceeded the budget". +const swiftStallRoot = mkdtempSync(path.join(os.tmpdir(), "lithe-test-stability-swift-stall-")); +try { + const reportPath = path.join(swiftStallRoot, "swift-stall.json"); + await assert.rejects( + runSwiftTestsWithTiming( + { + warnMs: 50, + maxMs: 200, + stallTimeoutMs: 100, + suiteTimeoutMs: 5000, + report: reportPath, + command: "swift", + commandArguments: ["test"], + }, + { + runProcessImpl: async ({ onStdoutLine, onSpawn }) => { + let resolveTerminated; + const terminated = new Promise((resolve) => { + resolveTerminated = resolve; + }); + onSpawn({ + terminate: async () => { + resolveTerminated(); + return true; + }, + }); + onStdoutLine('◇ Suite "Keyboard shortcuts" started.'); + onStdoutLine("◇ Test fast() started."); + onStdoutLine("✔ Test fast() passed after 0.001 seconds."); + onStdoutLine("◇ Test truncatedFinishLine() started."); + // The finish line for truncatedFinishLine() never arrives, as when the + // runner's stdio buffer is lost; the stall watchdog must fire. + await terminated; + return { + code: null, + signal: "SIGTERM", + timedOut: false, + terminationConfirmed: true, + durationMs: 150, + stdout: "", + stderr: "", + }; + }, + }, + ), + /produced no output for 100ms; tests without a reported result: truncatedFinishLine\(\)/, + ); + const stallReport = JSON.parse(readFileSync(reportPath, "utf8")); + assert.equal(stallReport.process.stalled, true); + assert.deepEqual( + stallReport.tests.map(({ name, status }) => ({ name, status })), + [ + { name: "fast()", status: "passed" }, + { name: "truncatedFinishLine()", status: "timeout" }, + { name: "Swift test runner stall", status: "timeout" }, + ], + ); +} finally { + rmSync(swiftStallRoot, { recursive: true, force: true }); +} + +// A stall after every test reported a result points at teardown/exit instead of +// blaming any test. +const swiftTeardownStallRoot = mkdtempSync( + path.join(os.tmpdir(), "lithe-test-stability-swift-teardown-stall-"), +); +try { + const reportPath = path.join(swiftTeardownStallRoot, "swift-teardown-stall.json"); + await assert.rejects( + runSwiftTestsWithTiming( + { + warnMs: 50, + maxMs: 200, + stallTimeoutMs: 100, + suiteTimeoutMs: 5000, + report: reportPath, + command: "swift", + commandArguments: ["test"], + }, + { + runProcessImpl: async ({ onStdoutLine, onSpawn }) => { + let resolveTerminated; + const terminated = new Promise((resolve) => { + resolveTerminated = resolve; + }); + onSpawn({ + terminate: async () => { + resolveTerminated(); + return true; + }, + }); + onStdoutLine("◇ Test fast() started."); + onStdoutLine("✔ Test fast() passed after 0.001 seconds."); + await terminated; + return { + code: null, + signal: "SIGTERM", + timedOut: false, + terminationConfirmed: true, + durationMs: 150, + stdout: "", + stderr: "", + }; + }, + }, + ), + /produced no output for 100ms; every parsed test had reported a result/, + ); + const teardownReport = JSON.parse(readFileSync(reportPath, "utf8")); + assert.equal(teardownReport.process.stalled, true); + assert.deepEqual( + teardownReport.tests.map(({ name, status }) => ({ name, status })), + [ + { name: "fast()", status: "passed" }, + { name: "Swift test runner stall", status: "timeout" }, + ], + ); +} finally { + rmSync(swiftTeardownStallRoot, { recursive: true, force: true }); +} + +// The per-test budget is enforced from the durations swift-testing reports: a +// test that finishes over maxMs must fail the run even though the runner +// exited cleanly and no watchdog fired. +const swiftBudgetRoot = mkdtempSync(path.join(os.tmpdir(), "lithe-test-stability-swift-budget-")); +try { + const reportPath = path.join(swiftBudgetRoot, "swift-budget.json"); + await assert.rejects( + runSwiftTestsWithTiming( + { + warnMs: 50, + maxMs: 200, + stallTimeoutMs: 5000, + suiteTimeoutMs: 10000, + report: reportPath, + command: "swift", + commandArguments: ["test"], + }, + { + runProcessImpl: async ({ onStdoutLine, onSpawn }) => { + onSpawn({ terminate: async () => true }); + onStdoutLine("◇ Test overBudget() started."); + onStdoutLine("✔ Test overBudget() passed after 0.250 seconds."); + return { + code: 0, + signal: null, + timedOut: false, + terminationConfirmed: true, + durationMs: 300, + stdout: "", + stderr: "", + }; + }, + }, + ), + /1 Swift test\(s\) exceeded the local budget/, + ); + const budgetReport = JSON.parse(readFileSync(reportPath, "utf8")); + assert.deepEqual( + budgetReport.tests.map(({ name, status, durationMs }) => ({ name, status, durationMs })), + [{ name: "overBudget()", status: "passed", durationMs: 250 }], + ); +} finally { + rmSync(swiftBudgetRoot, { recursive: true, force: true }); +} + +// A spawn failure must reject promptly and clear the stall watchdog; a leaked +// ref'd timer would keep the harness process alive for the full stall timeout. +{ + const spawnFailureRoot = mkdtempSync( + path.join(os.tmpdir(), "lithe-test-stability-swift-spawn-failure-"), + ); + try { + await assert.rejects( + runSwiftTestsWithTiming( + { + warnMs: 50, + maxMs: 200, + stallTimeoutMs: 600000, + suiteTimeoutMs: 10000, + report: path.join(spawnFailureRoot, "swift-spawn-failure.json"), + command: "swift", + commandArguments: ["test"], + }, + { + runProcessImpl: async () => { + throw new Error("spawn ENOENT"); + }, + }, + ), + /spawn ENOENT/, + ); + // If the watchdog leaked, the 600s timer would hold this test process open + // long past its CI budget; reaching this line with a cleared event loop is + // asserted implicitly by the suite finishing on time. + } finally { + rmSync(spawnFailureRoot, { recursive: true, force: true }); + } +} + const rustCompileFailureRoot = mkdtempSync( path.join( os.tmpdir(), From 1563dec4d59c3eb6161825e6e804836f6dc06474 Mon Sep 17 00:00:00 2001 From: lick <2188718831@qq.com> Date: Tue, 1 Sep 2026 11:01:47 +0800 Subject: [PATCH 3/4] fix(ci): sample the full descendant tree on Swift runner stall The testing helper detaches into its own process group, so the previous group-scoped walk only sampled the shell wrapper waiting on its child. Walk the ppid tree instead and capture a listing plus a thread-stack sample of every descendant before terminating the run. Co-Authored-By: Claude Fable 5 --- .../scripts/run-swift-tests-with-timing.mjs | 56 +++++++++++++++++-- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/.agents/skills/write-stable-tests/scripts/run-swift-tests-with-timing.mjs b/.agents/skills/write-stable-tests/scripts/run-swift-tests-with-timing.mjs index c9ed3261..6bfb2519 100755 --- a/.agents/skills/write-stable-tests/scripts/run-swift-tests-with-timing.mjs +++ b/.agents/skills/write-stable-tests/scripts/run-swift-tests-with-timing.mjs @@ -109,16 +109,62 @@ export async function run(options, { runProcessImpl = runProcess } = {}) { // guards against the runner producing no output at all. const stallTimeoutMs = options.stallTimeoutMs ?? 120000; // Best-effort thread-stack snapshot of the hung runner, taken before the - // SIGTERM destroys the evidence of where it was stuck. + // SIGTERM destroys the evidence of where it was stuck. The direct child is a + // shell wrapper, so walk its descendant tree: the genuinely hung process + // (swift-test or the testing helper, which detaches into its own process + // group) is a descendant, and the wrapper's stack would only show it waiting + // on its child. const captureStallSample = () => { if (process.platform !== "darwin" || !childPid) return; - const samplePath = options.report.replace(/\.json$/i, ".stall-sample.txt"); + let treePids = [String(childPid)]; + let processListing = ""; try { - const sample = spawnSync("sample", [String(childPid), "2", "-file", samplePath], { - stdio: "ignore", + const everyProcess = spawnSync( + "ps", + ["-axo", "pid=,ppid=,pgid=,etime=,command="], + { encoding: "utf8", timeout: 5000 }, + ); + const rows = (everyProcess.stdout ?? "") + .split("\n") + .map((line) => { + const [pid, ppid] = line.trim().split(/\s+/); + return { pid, ppid, line }; + }) + .filter((row) => row.pid); + const wanted = new Set([String(childPid)]); + // Multiple passes handle arbitrary depth without recursion. + for (let pass = 0; pass < 10; pass += 1) { + const before = wanted.size; + for (const row of rows) if (wanted.has(row.ppid)) wanted.add(row.pid); + if (wanted.size === before) break; + } + const treeRows = rows.filter((row) => wanted.has(row.pid)); + if (treeRows.length > 0) { + processListing = treeRows.map((row) => row.line).join("\n"); + treePids = treeRows.map((row) => row.pid); + } + } catch { + // Fall back to sampling only the direct child. + } + const samplePath = options.report.replace(/\.json$/i, ".stall-sample.txt"); + const sections = [ + `Process tree under pid ${childPid} at stall (pid ppid pgid etime command):`, + processListing || "(process listing unavailable)", + ]; + // Bound the diagnostics pass; each sample blocks for its full duration. + for (const pid of treePids.slice(0, 6)) { + const sample = spawnSync("sample", [pid, "2"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], timeout: 15000, }); - if (sample.status === 0) stallSamplePath = samplePath; + if (sample.status === 0 && sample.stdout) { + sections.push(`===== sample of pid ${pid} =====`, sample.stdout); + } + } + try { + writeFileSync(samplePath, `${sections.join("\n\n")}\n`); + stallSamplePath = samplePath; } catch { // Sampling is diagnostics only; never let it break termination. } From c5a7d4a8dc5ef08f4694e8730397f70bc5f3604c Mon Sep 17 00:00:00 2001 From: Yao Jingxi <23722032@bjtu.edu.cn> Date: Wed, 2 Sep 2026 09:09:12 +0800 Subject: [PATCH 4/4] fix: address branch popover review findings --- .../scripts/test-timing-lib.mjs | 79 +++++++++++++++++-- .../scripts/test-verify-test-stability.mjs | 3 + .../Views/Git/BranchSwitcherPopover.swift | 8 +- .../Lithe/Views/Workbench/WorkbenchView.swift | 30 +++++++ .../BranchSwitcherPopoverBehaviorTests.swift | 47 ++++++++++- 5 files changed, 155 insertions(+), 12 deletions(-) diff --git a/.agents/skills/write-stable-tests/scripts/test-timing-lib.mjs b/.agents/skills/write-stable-tests/scripts/test-timing-lib.mjs index 5a3e7941..b35e661b 100755 --- a/.agents/skills/write-stable-tests/scripts/test-timing-lib.mjs +++ b/.agents/skills/write-stable-tests/scripts/test-timing-lib.mjs @@ -14,6 +14,44 @@ function processGroupIsRunning(processID) { } } +function processIsRunning(processID) { + try { + process.kill(processID, 0); + return true; + } catch (error) { + return error?.code === "EPERM"; + } +} + +function addDescendantProcessIDs(rootProcessID, processIDs) { + let rows; + try { + const listing = spawnSync("ps", ["-axo", "pid=,ppid="], { + encoding: "utf8", + timeout: 5000, + }); + if (listing.status !== 0) return; + rows = listing.stdout + .split("\n") + .map((line) => line.trim().split(/\s+/).map(Number)) + .filter(([pid, ppid]) => Number.isInteger(pid) && Number.isInteger(ppid)); + } catch { + // The direct process group remains the portable fallback when ps is unavailable. + return; + } + + processIDs.add(rootProcessID); + let changed = true; + while (changed) { + changed = false; + for (const [pid, ppid] of rows) { + if (!processIDs.has(ppid) || processIDs.has(pid)) continue; + processIDs.add(pid); + changed = true; + } + } +} + function signalProcessGroup(child, signal) { try { process.kill(-child.pid, signal); @@ -27,12 +65,30 @@ function signalProcessGroup(child, signal) { } } -async function waitForProcessGroupExit(processID, timeoutMs, pollIntervalMs) { +function signalDescendantProcesses(rootProcessID, processIDs, signal) { + // The process-group signal handles ordinary descendants. Signal every known + // non-root PID as well because swift-testing may create a new process group. + for (const processID of processIDs) { + if (processID === rootProcessID) continue; + try { + process.kill(processID, signal); + } catch { + // It either exited between the snapshot and signal or is already gone. + } + } +} + +function processTreeIsRunning(processID, processIDs) { + return processGroupIsRunning(processID) + || [...processIDs].some((candidate) => processIsRunning(candidate)); +} + +async function waitForProcessTreeExit(processID, processIDs, timeoutMs, pollIntervalMs) { const deadline = performance.now() + timeoutMs; - while (processGroupIsRunning(processID) && performance.now() < deadline) { + while (processTreeIsRunning(processID, processIDs) && performance.now() < deadline) { await delay(pollIntervalMs); } - return !processGroupIsRunning(processID); + return !processTreeIsRunning(processID, processIDs); } export async function terminateProcessTree( @@ -52,10 +108,23 @@ export async function terminateProcessTree( return true; } + const processIDs = new Set(); + addDescendantProcessIDs(child.pid, processIDs); signalProcessGroup(child, "SIGTERM"); - if (await waitForProcessGroupExit(child.pid, gracePeriodMs, pollIntervalMs)) return true; + signalDescendantProcesses(child.pid, processIDs, "SIGTERM"); + if (await waitForProcessTreeExit(child.pid, processIDs, gracePeriodMs, pollIntervalMs)) return true; + + // Refresh before forcing termination so descendants created during graceful + // shutdown cannot escape the cleanup pass. + addDescendantProcessIDs(child.pid, processIDs); signalProcessGroup(child, "SIGKILL"); - return waitForProcessGroupExit(child.pid, forcedTerminationTimeoutMs, pollIntervalMs); + signalDescendantProcesses(child.pid, processIDs, "SIGKILL"); + return waitForProcessTreeExit( + child.pid, + processIDs, + forcedTerminationTimeoutMs, + pollIntervalMs, + ); } function lineCollector(callback) { diff --git a/.agents/skills/write-stable-tests/scripts/test-verify-test-stability.mjs b/.agents/skills/write-stable-tests/scripts/test-verify-test-stability.mjs index 74715ed4..9b7c8b57 100755 --- a/.agents/skills/write-stable-tests/scripts/test-verify-test-stability.mjs +++ b/.agents/skills/write-stable-tests/scripts/test-verify-test-stability.mjs @@ -566,10 +566,13 @@ try { if (process.platform !== "win32") { let rootPID = null; let descendantPID = null; + // swift-testing may detach its helper into a separate process group. The + // timeout owner must still discover and terminate that descendant. const descendantSource = "process.on('SIGTERM', () => {}); setInterval(() => {}, 1000);"; const rootSource = ` const { spawn } = require("node:child_process"); const descendant = spawn(process.execPath, ["-e", ${JSON.stringify(descendantSource)}], { + detached: true, stdio: "ignore", }); console.log(descendant.pid); diff --git a/macos/Sources/Lithe/Views/Git/BranchSwitcherPopover.swift b/macos/Sources/Lithe/Views/Git/BranchSwitcherPopover.swift index de2a5683..6ff47911 100644 --- a/macos/Sources/Lithe/Views/Git/BranchSwitcherPopover.swift +++ b/macos/Sources/Lithe/Views/Git/BranchSwitcherPopover.swift @@ -15,6 +15,7 @@ struct BranchSwitcherPopover: View { @Binding var isPresented: Bool let onCommit: () -> Void let onPush: (GitReference) -> Void + let onDelete: (GitReference) -> Void let onNewBranch: (GitReference) -> Void let onCheckoutRevision: () -> Void let onManageBranches: () -> Void @@ -448,12 +449,13 @@ struct BranchSwitcherPopover: View { } } - if reference.kind != .tag { + if reference.kind == .local { Divider() Button("Update") { dismissAndRun { Task { await model.updateCurrentBranch(reference) } } } + .disabled(!reference.isCurrent) Button("Push…") { dismissAndRun { onPush(reference) } @@ -463,8 +465,8 @@ struct BranchSwitcherPopover: View { if reference.kind == .local, !reference.isCurrent { Divider() - Button("Delete") { - dismissAndRun { Task { await model.deleteBranch(reference) } } + Button("Delete", role: .destructive) { + dismissAndRun { onDelete(reference) } } } } diff --git a/macos/Sources/Lithe/Views/Workbench/WorkbenchView.swift b/macos/Sources/Lithe/Views/Workbench/WorkbenchView.swift index 9e3365bb..c818a079 100644 --- a/macos/Sources/Lithe/Views/Workbench/WorkbenchView.swift +++ b/macos/Sources/Lithe/Views/Workbench/WorkbenchView.swift @@ -41,6 +41,7 @@ struct WorkbenchView: View { @State private var newBranchReference: GitReference? @State private var isCheckoutRevisionPresented = false @State private var pendingTopBarPushReference: GitReference? + @State private var pendingTopBarDeleteReference: GitReference? @State private var isProjectSwitcherPresented = false @State private var isPluginPanelPresented = false @State private var isNotificationCenterPresented = false @@ -255,6 +256,31 @@ struct WorkbenchView: View { } message: { Text("This sends the current branch to its configured remote.") } + .confirmationDialog( + "Delete branch?", + isPresented: Binding( + get: { pendingTopBarDeleteReference != nil }, + set: { if !$0 { pendingTopBarDeleteReference = nil } } + ), + titleVisibility: .visible + ) { + Button("Delete", role: .destructive) { + guard let reference = pendingTopBarDeleteReference else { return } + pendingTopBarDeleteReference = nil + Task { await model.deleteBranch(reference) } + } + .disabled(model.isPerformingBranchOperation) + .lithePointer() + Button("Cancel", role: .cancel) { + pendingTopBarDeleteReference = nil + } + .lithePointer() + } message: { + Text( + "Delete the local branch \(pendingTopBarDeleteReference?.shortName ?? "")? " + + "Git will refuse if it contains unmerged work." + ) + } .overlay(alignment: .bottom) { if let message = model.notificationMessage { Text(LocalizedStringKey(message)) @@ -501,6 +527,10 @@ struct WorkbenchView: View { isBranchSwitcherPresented = false pendingTopBarPushReference = reference }, + onDelete: { reference in + isBranchSwitcherPresented = false + pendingTopBarDeleteReference = reference + }, onNewBranch: { reference in isBranchSwitcherPresented = false newBranchReference = reference diff --git a/macos/Tests/LitheTests/BranchSwitcherPopoverBehaviorTests.swift b/macos/Tests/LitheTests/BranchSwitcherPopoverBehaviorTests.swift index f8222dd2..6928b550 100644 --- a/macos/Tests/LitheTests/BranchSwitcherPopoverBehaviorTests.swift +++ b/macos/Tests/LitheTests/BranchSwitcherPopoverBehaviorTests.swift @@ -9,15 +9,23 @@ import Testing /// a stray click while scanning the list. @Suite("Branch switcher popover behavior") struct BranchSwitcherPopoverBehaviorTests { - private static func popoverSource() throws -> String { + private static func source(at relativePath: String) throws -> String { let repositoryRoot = URL(fileURLWithPath: #filePath) .deletingLastPathComponent() .deletingLastPathComponent() .deletingLastPathComponent() - let popoverURL = repositoryRoot.appendingPathComponent( - "Sources/Lithe/Views/Git/BranchSwitcherPopover.swift" + return try String( + contentsOf: repositoryRoot.appendingPathComponent(relativePath), + encoding: .utf8 ) - return try String(contentsOf: popoverURL, encoding: .utf8) + } + + private static func popoverSource() throws -> String { + try source(at: "Sources/Lithe/Views/Git/BranchSwitcherPopover.swift") + } + + private static func workbenchSource() throws -> String { + try source(at: "Sources/Lithe/Views/Workbench/WorkbenchView.swift") } @Test @@ -76,4 +84,35 @@ struct BranchSwitcherPopoverBehaviorTests { "Rows truncate branch and upstream names, so hover must reveal the untruncated pair." ) } + + @Test + func updateAndPushAreLimitedToSupportedLocalBranches() throws { + let source = try Self.popoverSource() + + guard let localActions = source.range(of: "if reference.kind == .local {") else { + Issue.record("Update and Push must be grouped under a local-branch capability check.") + return + } + let actions = source[localActions.lowerBound...] + #expect(actions.contains("Button(\"Update\")")) + #expect( + actions.contains(".disabled(!reference.isCurrent)"), + "Only the current local branch can be updated." + ) + #expect(actions.contains("Button(\"Push…\")")) + } + + @Test + func deleteUsesTheWorkbenchConfirmationFlow() throws { + let popover = try Self.popoverSource() + let workbench = try Self.workbenchSource() + + #expect(popover.contains("dismissAndRun { onDelete(reference) }")) + #expect( + !popover.contains("model.deleteBranch(reference)"), + "The popover must not delete a branch before the user confirms." + ) + #expect(workbench.contains("Button(\"Delete\", role: .destructive)")) + #expect(workbench.contains("Task { await model.deleteBranch(reference) }")) + } }