diff --git a/.fallowrc.json b/.fallowrc.json index 76de910d65..02b12078c5 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -64,7 +64,11 @@ "scripts/maestro-conformance/corpus/**", "apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests.xctestplan", "scripts/help-conformance-sample-outputs.d.mts", - "scripts/help-conformance-runner-output.d.mts" + "scripts/help-conformance-runner-output.d.mts", + // A hand-written type surface for a plain-Node .mjs the typechecked program (and Fallow's + // graph) cannot import; its only consumer is the .mjs's real importers, resolved by tsc, not + // by an import edge. Same shape as the help-conformance declaration files above. + "scripts/size-report-package.d.mts" ], "dynamicallyLoaded": ["packages/daemon/src/execution/interaction/index.ts"], "ignoreDependencies": [ diff --git a/apple/macos-helper/Package.swift b/apple/macos-helper/Package.swift index 844c6cb2bb..b617fd28df 100644 --- a/apple/macos-helper/Package.swift +++ b/apple/macos-helper/Package.swift @@ -1,6 +1,10 @@ // swift-tools-version: 5.9 import PackageDescription +// The npm package ships this directory and builds it on the user's machine. The tests under +// Tests/ stay out of the tarball through `files`, except one PackStub.swift per test target: +// SwiftPM refuses any load of a package whose declared test target has no sources, so the +// shipped tree needs a source per test target for the install-time `swift build` to succeed. let package = Package( name: "agent-device-macos-helper", platforms: [.macOS(.v13)], diff --git a/apple/macos-helper/Tests/AgentDeviceMacOSHelperTests/PackStub.swift b/apple/macos-helper/Tests/AgentDeviceMacOSHelperTests/PackStub.swift new file mode 100644 index 0000000000..5e76819819 --- /dev/null +++ b/apple/macos-helper/Tests/AgentDeviceMacOSHelperTests/PackStub.swift @@ -0,0 +1,7 @@ +// The npm package ships this one file and nothing else under the test targets (see `files` in +// package.json). SwiftPM refuses to load a package whose declared test target has no sources, so +// the install-time `swift build` of the packed tree needs a source here; the real tests for this +// target live beside it and are excluded from the pack. +import XCTest + +final class AgentDeviceMacOSHelperPackStubTests: XCTestCase {} diff --git a/apple/macos-helper/Tests/AgentDeviceMacOSInputTests/PackStub.swift b/apple/macos-helper/Tests/AgentDeviceMacOSInputTests/PackStub.swift new file mode 100644 index 0000000000..ad6997dffd --- /dev/null +++ b/apple/macos-helper/Tests/AgentDeviceMacOSInputTests/PackStub.swift @@ -0,0 +1,7 @@ +// The npm package ships this one file and nothing else under the test targets (see `files` in +// package.json). SwiftPM refuses to load a package whose declared test target has no sources, so +// the install-time `swift build` of the packed tree needs a source here; the real tests for this +// target live beside it and are excluded from the pack. +import XCTest + +final class AgentDeviceMacOSInputPackStubTests: XCTestCase {} diff --git a/package.json b/package.json index 237c95bd81..2f5d2c7890 100644 --- a/package.json +++ b/package.json @@ -222,6 +222,9 @@ "dist", "apple/macos-helper", "!apple/macos-helper/**/.build", + "!apple/macos-helper/Tests/**", + "apple/macos-helper/Tests/AgentDeviceMacOSHelperTests/PackStub.swift", + "apple/macos-helper/Tests/AgentDeviceMacOSInputTests/PackStub.swift", "apple/snapshot-bridge", "apple/fold-helper", "android/snapshot-helper/dist", diff --git a/scripts/__tests__/fixtures/size-report-npm-pack.json b/scripts/__tests__/fixtures/size-report-npm-pack.json index 5d5cf29e20..1a11bb6334 100644 --- a/scripts/__tests__/fixtures/size-report-npm-pack.json +++ b/scripts/__tests__/fixtures/size-report-npm-pack.json @@ -1,21 +1,83 @@ { "unpackedSize": 2319, "files": [ - { "path": "dist/src/index.js", "size": 401 }, - { "path": "dist/src/index.d.ts", "size": 102 }, - { "path": "dist/apple/runner/RunnerTests.swift", "size": 503 }, - { "path": "dist/apple/snapshot-presentation/Package.swift", "size": 113 }, - { "path": "apple/snapshot-bridge/SnapshotBridge.m", "size": 0 }, - { "path": "apple/snapshot-bridge/SnapshotBridgeRuntime.m", "size": 0 }, - { "path": "apple/snapshot-bridge/SnapshotBridgeRuntime.h", "size": 0 }, - { "path": "apple/snapshot-bridge/SnapshotBridgeCapture.h", "size": 0 }, - { "path": "apple/snapshot-bridge/SnapshotBridgeCapture.m", "size": 0 }, - { "path": "apple/macos-helper/Sources/main.swift", "size": 211 }, - { "path": "android/snapshot-helper/dist/helper.apk", "size": 307 }, - { "path": "android/snapshot-helper/dist/helper.manifest.json", "size": 99 }, - { "path": "android/ime-helper/dist/helper.apk", "size": 307 }, - { "path": "android/ime-helper/dist/helper.manifest.json", "size": 99 }, - { "path": "package.json", "size": 99 }, - { "path": "vendor/unknown.bin", "size": 78 } - ] + { + "path": "dist/src/index.js", + "size": 401 + }, + { + "path": "dist/src/index.d.ts", + "size": 102 + }, + { + "path": "dist/apple/runner/RunnerTests.swift", + "size": 503 + }, + { + "path": "dist/apple/snapshot-presentation/Package.swift", + "size": 113 + }, + { + "path": "apple/snapshot-bridge/SnapshotBridge.m", + "size": 0 + }, + { + "path": "apple/snapshot-bridge/SnapshotBridgeRuntime.m", + "size": 0 + }, + { + "path": "apple/snapshot-bridge/SnapshotBridgeRuntime.h", + "size": 0 + }, + { + "path": "apple/snapshot-bridge/SnapshotBridgeCapture.h", + "size": 0 + }, + { + "path": "apple/snapshot-bridge/SnapshotBridgeCapture.m", + "size": 0 + }, + { + "path": "apple/macos-helper/Package.swift", + "size": 1057 + }, + { + "path": "apple/macos-helper/Sources/main.swift", + "size": 211 + }, + { + "path": "android/snapshot-helper/dist/helper.apk", + "size": 307 + }, + { + "path": "android/snapshot-helper/dist/helper.manifest.json", + "size": 99 + }, + { + "path": "android/ime-helper/dist/helper.apk", + "size": 307 + }, + { + "path": "android/ime-helper/dist/helper.manifest.json", + "size": 99 + }, + { + "path": "package.json", + "size": 99 + }, + { + "path": "vendor/unknown.bin", + "size": 78 + }, + { + "path": "apple/macos-helper/Tests/AgentDeviceMacOSHelperTests/PackStub.swift", + "size": 426 + }, + { + "path": "apple/macos-helper/Tests/AgentDeviceMacOSInputTests/PackStub.swift", + "size": 426 + } + ], + "entryCount": 19, + "size": 3376 } diff --git a/scripts/__tests__/size-report-package.test.ts b/scripts/__tests__/size-report-package.test.ts index c3841b27b9..6c9397e02d 100644 --- a/scripts/__tests__/size-report-package.test.ts +++ b/scripts/__tests__/size-report-package.test.ts @@ -9,12 +9,31 @@ const fixturePack = JSON.parse( await readFile(join(import.meta.dirname, 'fixtures', 'size-report-npm-pack.json'), 'utf8'), ); +// The fixture pack ships the macOS helper (manifest, sources, and per-target PackStub.swift), +// so every assertion here names the manifest source the guard would read from the packed tree; +// without it the guard falls back to this checkout's manifest via repoRoot. +const helperManifestSource = [ + '.target(', + ' name: "AgentDeviceMacOSInput",', + '.executableTarget(', + ' name: "AgentDeviceMacOSHelper",', + '.testTarget(', + ' name: "AgentDeviceMacOSHelperTests",', + '.testTarget(', + ' name: "AgentDeviceMacOSInputTests",', +].join('\n'); + test('publish package requires both Android helpers and excludes benchmark scripts', () => { - assert.doesNotThrow(() => assertPublishPackageContents(fixturePack.files)); + assert.doesNotThrow(() => + assertPublishPackageContents(fixturePack.files, { + macosHelperManifestSource: helperManifestSource, + }), + ); assert.throws( () => assertPublishPackageContents( fixturePack.files.filter((entry) => !entry.path.startsWith('android/ime-helper/')), + { macosHelperManifestSource: helperManifestSource }, ), /android\/ime-helper/, ); @@ -31,16 +50,52 @@ test('publish package requires both Android helpers and excludes benchmark scrip assert.doesNotThrow(() => assertPublishPackageContents( fixturePack.files.filter((entry) => !entry.path.startsWith('apple/snapshot-bridge/')), + { macosHelperManifestSource: helperManifestSource }, ), ); assert.throws( () => - assertPublishPackageContents([ - ...fixturePack.files, - { path: 'scripts/ios-snapshot-benchmark/run.ts', size: 1 }, - ]), + assertPublishPackageContents( + [...fixturePack.files, { path: 'scripts/ios-snapshot-benchmark/run.ts', size: 1 }], + { macosHelperManifestSource: helperManifestSource }, + ), /benchmark or build scripts/, ); + // Each declared test target needs its PackStub.swift or the install-time `swift build` of the + // packed tree dies; a missing one must name the missing path. + assert.throws( + () => + assertPublishPackageContents( + fixturePack.files.filter( + (entry) => + entry.path !== 'apple/macos-helper/Tests/AgentDeviceMacOSInputTests/PackStub.swift', + ), + { macosHelperManifestSource: helperManifestSource }, + ), + /pack stubs: apple\/macos-helper\/Tests\/AgentDeviceMacOSInputTests\/PackStub\.swift/, + ); + assert.throws( + () => + assertPublishPackageContents( + [ + ...fixturePack.files, + { + path: 'apple/macos-helper/Tests/AgentDeviceMacOSInputTests/ScheduleTests.swift', + size: 1, + }, + ], + { macosHelperManifestSource: helperManifestSource }, + ), + /macOS helper test sources/, + ); + // Sources without the manifest are not a buildable Swift package; the pack must say so. + assert.throws( + () => + assertPublishPackageContents( + fixturePack.files.filter((entry) => entry.path !== 'apple/macos-helper/Package.swift'), + ), + /sources without its Package\.swift/, + ); }); test('Markdown emphasizes total install size and startup without duplicate breakdowns', () => { diff --git a/scripts/coverage-changed/model.test.ts b/scripts/coverage-changed/model.test.ts index b58742dc08..b4c645a3db 100644 --- a/scripts/coverage-changed/model.test.ts +++ b/scripts/coverage-changed/model.test.ts @@ -86,7 +86,12 @@ const coverage = parseLcov( ); function diff(path: string, added: number[], deleted = false): ChangedFileDiff { - return { path, added, deleted }; + return { path, added, deleted, regularFile: true }; +} + +/** A changed path whose new side is not a blob (symlink or gitlink); `run.ts` never reads its bytes. */ +function nonBlobDiff(path: string, added: number[]): ChangedFileDiff { + return { path, added, deleted: false, regularFile: false }; } test('gate fails when changed-line coverage is below threshold and lists offenders', () => { @@ -158,6 +163,31 @@ test('waiver forces a pass but still reports the numbers', () => { assert.equal(result.pct, 66.67); }); +test('an includable path whose new side is a symlink or gitlink is skipped, not read', () => { + // A committed symlink like `tools/helper-tests/Sources -> ../helper/Sources` shows in the + // diff as added text (its target), so it reaches this loop by path shape. Its working-tree + // entry resolves to a directory, so a content read dies on EISDIR; reading the + // object mode is what keeps the file out of both the scored and the excluded tally. + const readPaths: string[] = []; + const result = computeChangedCoverage({ + diffs: [ + nonBlobDiff('src/helper-tests/index.ts', [1]), + nonBlobDiff('vendor/lib', [1]), + diff('src/covered.ts', [1, 2, 3]), + ], + coverage, + fileLines: (path) => { + readPaths.push(path); + return null; + }, + }); + assert.deepEqual(readPaths, ['src/covered.ts']); + // 2/3 = 66.67% < 70: the skip leaves the verdict exactly what the scored file alone earns. + assert.equal(result.totalLines, 3); + assert.equal(result.passed, false); + assert.deepEqual(result.excluded.files, []); +}); + test('includable source absent from lcov counts as excluded-path code lines', () => { const result = computeChangedCoverage({ diffs: [diff('src/sdk/thing.ts', [1, 2, 3])], diff --git a/scripts/coverage-changed/model.ts b/scripts/coverage-changed/model.ts index 5ca1110301..2e8b3d7862 100644 --- a/scripts/coverage-changed/model.ts +++ b/scripts/coverage-changed/model.ts @@ -220,7 +220,15 @@ export function computeChangedCoverage(input: ComputeInput): ChangedCoverageResu const excludedFiles: ExcludedFileReport[] = []; for (const diff of input.diffs) { - if (diff.deleted || diff.added.length === 0 || !isIncludableSource(diff.path)) continue; + // A non-blob side (symlink, gitlink) is not source: its working-tree entry can + // resolve to a directory, which no content read may attempt. + if ( + diff.deleted || + !diff.regularFile || + diff.added.length === 0 || + !isIncludableSource(diff.path) + ) + continue; const cov = input.coverage.get(diff.path); if (!cov) { diff --git a/scripts/coverage-changed/run.test.ts b/scripts/coverage-changed/run.test.ts index 15382e77f2..3341002e26 100644 --- a/scripts/coverage-changed/run.test.ts +++ b/scripts/coverage-changed/run.test.ts @@ -67,6 +67,25 @@ test('passes and prints n/a for a docs-only change without touching coverage', ( assert.match(out, /0\/0 \(n\/a\)/); }); +test('a committed symlink to a directory passes the gate without an EISDIR crash', () => { + // The object mode in the diff header is what keeps a committed symlink (target recorded + // as one added line, a directory in the working tree) out of the content read below. + fs.mkdirSync(path.join(repo, 'vendor'), { recursive: true }); + write('vendor/keep.txt', 'not source\n'); + // Shaped like includable source so the gate reaches its content read; what stands + // behind the name is a directory. + fs.symlinkSync('../vendor', path.join(repo, 'src/linked.ts')); + write('README.md', '# docs\nnew line\n'); + git('add', '-A'); + git('commit', '-q', '-m', 'symlink into a directory and docs'); + writeLcov('SF:src/base.ts\nDA:1,1\nend_of_record\n'); + + const { code, out } = capture(() => run(['--base', 'main'], repo)); + + assert.equal(code, 0); + assert.match(out, /Changed-line coverage gate: PASS/); +}); + test('reads a large stacked diff beyond the subprocess default buffer', () => { write('README.md', '# large stack\n' + 'documentation line\n'.repeat(80_000)); git('add', '-A'); diff --git a/scripts/coverage-crap/model.ts b/scripts/coverage-crap/model.ts index a7235ad0f9..e683dbc356 100644 --- a/scripts/coverage-crap/model.ts +++ b/scripts/coverage-crap/model.ts @@ -185,13 +185,15 @@ export type CrapScope = 'touched' | 'all'; export type TouchedFile = Readonly<{ path: string; addedLines: number }>; /** - * The files whose lines this change edits. A pure rename, delete, or mode change adds no lines, so - * it contributes none — which is also exactly the set of files fallow's `--diff-stdin` scoping can - * charge a finding to, since that filter indexes the same added lines. + * The files whose lines this change edits. A pure rename, delete, mode change, or non-blob side + * (a symlink or a gitlink) adds no scoreable source, so it contributes none — which is also exactly + * the set of files fallow's `--diff-stdin` scoping can charge a finding to, since that filter indexes + * the same added lines. Dropping non-blob sides here is also what keeps every `TouchedFile` a path + * the working tree can be asked to hold as a readable file. */ export function readTouchedFiles(diffs: readonly ChangedFileDiff[]): TouchedFile[] { return diffs - .filter((diff) => !diff.deleted && diff.added.length > 0) + .filter((diff) => !diff.deleted && diff.regularFile && diff.added.length > 0) .map((diff) => ({ path: diff.path, addedLines: diff.added.length })); } diff --git a/scripts/coverage-crap/run.test.ts b/scripts/coverage-crap/run.test.ts index 9ad7006e07..665e9290b3 100644 --- a/scripts/coverage-crap/run.test.ts +++ b/scripts/coverage-crap/run.test.ts @@ -492,6 +492,25 @@ test('a touched file the working tree no longer holds is not held against the ru assert.match(out, /no touched function\(s\) at or above 30/); }); +test('a committed symlink to a directory is not read as a touched file', () => { + // The diff of a committed symlink (`helper-tests/Sources -> ../helper`) carries its target + // path as one added line, so the file reaches the touched list by its lines alone. The working + // tree holds a directory behind it, so reading it as source dies on EISDIR before any verdict + // exists — the object mode in the diff header is what keeps it off the list. + fs.mkdirSync(path.join(repo, 'helper-tests'), { recursive: true }); + write('helper/Helper.swift', 'func helper() {}\n'); + fs.symlinkSync('../helper', path.join(repo, 'helper-tests/Sources')); + write('src/clean.ts', `${TRIVIAL}export const also_trivial = 2;\n`); + commit('add a symlink to a directory'); + writeCoverage({ 'src/debt.ts': {}, 'src/clean.ts': { hits: 4 } }); + writeStamp(); + + const { code, out } = check(['--base', HEADS[0]!]); + + assert.equal(code, 0); + assert.match(out, /no touched function\(s\) at or above 30/); +}); + test('coverage written without a stamp is not trustworthy', () => { write('src/clean.ts', `${TRIVIAL}export const also_trivial = 2;\n`); commit('clean edit'); diff --git a/scripts/lib/repo-git.test.ts b/scripts/lib/repo-git.test.ts index aa61f52437..5ba8bf5b49 100644 --- a/scripts/lib/repo-git.test.ts +++ b/scripts/lib/repo-git.test.ts @@ -41,3 +41,76 @@ test('parseUnifiedDiff marks deleted files and keeps rename destinations', () => const renamed = files.find((f) => f.path === 'src/new.ts'); assert.deepEqual(renamed?.added, [4]); }); + +// The object modes below are what `git diff` writes in its headers, verified against +// real diffs: a symlink's added line is its target path, a gitlink's is +// `Subproject commit `, and both resolve in the working tree to something a +// content read cannot take (EISDIR for a symlink to a directory). Content readers +// skip what `regularFile` rules out, so the parse must read the mode from the +// headers rather than trust the body. +test('parseUnifiedDiff reads the new-side object mode as regularFile', () => { + const diff = [ + 'diff --git a/tools/helper-tests/Sources b/tools/helper-tests/Sources', + 'new file mode 120000', + 'index 0000000..e94dfd3', + '--- /dev/null', + '+++ b/tools/helper-tests/Sources', + '@@ -0,0 +1 @@', + '+../helper/Sources', + String.raw`\ No newline at end of file`, + 'diff --git a/vendor/lib b/vendor/lib', + 'index 40287e1..6f36720 160000', + '--- a/vendor/lib', + '+++ b/vendor/lib', + '@@ -1 +1 @@', + '-Subproject commit 40287e12bdee20e6666847781ad1e618ef185015', + '+Subproject commit 6f36720aa717698e572f0dec47f1ca11adcc190c', + 'diff --git a/src/kept.ts b/src/kept.ts', + 'index 6178079..c3219eb 100644', + '--- a/src/kept.ts', + '+++ b/src/kept.ts', + '@@ -1,0 +2 @@', + '+const kept = 1;', + ].join('\n'); + const files = parseUnifiedDiff(diff); + const link = files.find((f) => f.path === 'tools/helper-tests/Sources'); + assert.deepEqual(link?.added, [1]); + assert.equal(link?.regularFile, false); + const gitlink = files.find((f) => f.path === 'vendor/lib'); + assert.deepEqual(gitlink?.added, [1]); + assert.equal(gitlink?.regularFile, false); + const kept = files.find((f) => f.path === 'src/kept.ts'); + assert.equal(kept?.regularFile, true); +}); + +test('parseUnifiedDiff reads mode-change and deletion headers as regular-file sides', () => { + const diff = [ + 'diff --git a/tools/build.sh b/tools/build.sh', + 'old mode 100644', + 'new mode 100755', + 'index 562890b..562890b', + 'diff --git a/src/gone.ts b/src/gone.ts', + 'deleted file mode 100644', + 'index 7898192..0000000', + '--- a/src/gone.ts', + '+++ /dev/null', + '@@ -1 +0,0 @@', + '-was here', + 'diff --git a/gone-link b/gone-link', + 'deleted file mode 120000', + 'index c59d9b6..0000000', + '--- a/gone-link', + '+++ /dev/null', + '@@ -1 +0,0 @@', + '-target', + ].join('\n'); + const files = parseUnifiedDiff(diff); + // An executable-bit change stays a regular file; a deletion has no new-side + // content to read whatever the removed side's mode was. + const modes = files.find((f) => f.path === 'tools/build.sh'); + assert.equal(modes?.regularFile, true); + assert.equal(files.find((f) => f.path === 'src/gone.ts')?.deleted, true); + const goneLink = files.find((f) => f.path === 'gone-link'); + assert.equal(goneLink?.deleted, true); + assert.equal(goneLink?.regularFile, false); +}); diff --git a/scripts/lib/repo-git.ts b/scripts/lib/repo-git.ts index d04a90b670..836e3c49e5 100644 --- a/scripts/lib/repo-git.ts +++ b/scripts/lib/repo-git.ts @@ -46,9 +46,30 @@ export type ChangedFileDiff = { readonly added: readonly number[]; // `+++ /dev/null`: the file was deleted, so it contributes no changed lines. readonly deleted: boolean; + // Whether the new side is a regular git blob (mode 100644/100755), the only kind + // whose working-tree bytes are the file's content. A symlink (120000) carries its + // target path as content and its working-tree entry can resolve to a directory, + // which reading as a file dies on (EISDIR); a gitlink (160000) names a nested + // repository. Content readers must skip what this rules out. + readonly regularFile: boolean; }; -type DiffSection = { path: string; added: Set; deleted: boolean; cursor: number }; +// Git object modes the new side can carry in a diff header for something that is +// not a regular blob. `readChangedLinesDiff` reports their bodies as text lines +// (a link target, a `Subproject commit`), which no content reader may treat as +// source. Parsed rather than probed on disk: the verdict describes the commit the +// diff was cut against, not whatever the working tree holds at check time. +// `000000` is `+++ /dev/null`, the side of a deletion. +const NON_BLOB_MODES = new Set(['000000', '120000', '160000']); + +type DiffSection = { + path: string; + added: Set; + deleted: boolean; + cursor: number; + /** The new side's object mode, as the headers state it; null when they stay silent. */ + newMode: string | null; +}; function newStartOfHunk(line: string): number | null { const match = /@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/.exec(line); @@ -60,6 +81,23 @@ function pathFromDiffHeader(line: string): string { return header ? normalizePath(header[1]!) : ''; } +// Reads the new side's object mode out of the metadata lines that carry it, last +// writer wins in header order (`old mode`, `new mode`, then the `index` trailer). +// A hand-built diff may carry neither, which leaves the mode unknown and the side +// read as regular. +function foldModeLine(section: DiffSection, line: string): boolean { + const declared = /^(?:new file mode|old mode|new mode|deleted file mode) (\d{6})$/.exec(line); + if (declared) { + // `deleted file mode` describes the removed side; the new side is /dev/null. + section.newMode = line.startsWith('deleted') ? '000000' : declared[1]!; + return true; + } + if (!line.startsWith('index ')) return false; + const trailing = / (\d{6})$/.exec(line); + if (trailing) section.newMode = trailing[1]!; + return true; +} + // Fold one line of a file's diff body into its section. Renames+edits report // their added lines under the destination (`+++ b/...`); pure renames and // deletions add nothing. `--unified=0` means no context lines in practice. @@ -76,11 +114,19 @@ function foldDiffBodyLine(section: DiffSection, line: string): void { section.cursor += 1; } else if (line.startsWith('-') || line.startsWith('\\')) { // Removed line or "\ No newline": occupies no new-side number. + } else if (foldModeLine(section, line)) { + // Object-mode metadata; recorded above, carries no new-side number. } else { section.cursor += 1; } } +// The object mode of a section's new side is known only once its headers have +// been read; until then it is null, which readers treat as a regular file. +function isRegularFileSide(newMode: string | null): boolean { + return newMode === null || !NON_BLOB_MODES.has(newMode); +} + /** * `readChangedLinesDiff`'s report as per-file added-line sets — the one reading both changed-line * gates score from. Keep it beside its producer: a hand-built diff must parse the same way the @@ -95,13 +141,20 @@ export function parseUnifiedDiff(diff: string): ChangedFileDiff[] { path: section.path, added: [...section.added].sort((a, b) => a - b), deleted: section.deleted, + regularFile: isRegularFileSide(section.newMode), }); } }; for (const line of diff.split('\n')) { if (line.startsWith('diff --git ')) { flush(); - section = { path: pathFromDiffHeader(line), added: new Set(), deleted: false, cursor: 0 }; + section = { + path: pathFromDiffHeader(line), + added: new Set(), + deleted: false, + cursor: 0, + newMode: null, + }; } else if (section) { foldDiffBodyLine(section, line); } diff --git a/scripts/size-report-package.d.mts b/scripts/size-report-package.d.mts new file mode 100644 index 0000000000..d6241e686e --- /dev/null +++ b/scripts/size-report-package.d.mts @@ -0,0 +1,13 @@ +// Type surface for the plain-Node size-report module (it must stay runnable by bare `node`, so +// it is not in the typechecked program). TypeScript resolves this file when a checked module +// imports `./size-report-package.mjs`; keep it in step with the exports there. +export function readMacosHelperTestTargets(manifestSource: string): string[]; +export type NpmPackEntry = Readonly<{ path: string; size: number }>; +export function assertPublishPackageContents( + entries: readonly NpmPackEntry[], + options?: Readonly<{ + repoRoot?: string; + requireSnapshotBridge?: boolean; + macosHelperManifestSource?: string; + }>, +): void; diff --git a/scripts/size-report-package.mjs b/scripts/size-report-package.mjs index 113b2651e0..94f0051852 100644 --- a/scripts/size-report-package.mjs +++ b/scripts/size-report-package.mjs @@ -17,6 +17,17 @@ export function assertSnapshotBridgeAssets(presentPaths, context) { } } +/** + * The test targets the macOS helper manifest declares — the one reading of "which test targets + * exist". SwiftPM refuses to load a package whose declared test target has no sources, so each + * name here must appear in the pack as `apple/macos-helper/Tests//PackStub.swift`. + */ +export function readMacosHelperTestTargets(manifestSource) { + return [...manifestSource.matchAll(/\.testTarget\(\s*name:\s*"([^"]+)"/g)].map( + (match) => match[1], + ); +} + export function collectNpmPack(root) { const cachePath = path.join(root, '.tmp', 'npm-cache'); fs.mkdirSync(cachePath, { recursive: true }); @@ -28,6 +39,7 @@ export function collectNpmPack(root) { const pack = parseNpmPackOutput(stdout); const entries = normalizeNpmPackEntries(pack); assertPublishPackageContents(entries, { + repoRoot: root, requireSnapshotBridge: fs.existsSync(path.join(root, 'apple', 'snapshot-bridge')), }); return { @@ -76,6 +88,47 @@ export function assertPublishPackageContents(entries, options = {}) { if (forbiddenPaths.length > 0) { throw new Error(`npm pack includes benchmark or build scripts: ${forbiddenPaths.join(', ')}`); } + // The macOS helper ships as buildable source. Its manifest declares the test targets, so the + // packed tree must hold at least one source per target or the install-time `swift build` dies; + // `files` ships exactly one PackStub.swift per target and excludes every other test source. + // Anything else under Tests/ in the pack means the exclusion list leaked real tests. + const shippedSwiftTests = paths.filter( + (entryPath) => + (entryPath === 'apple/macos-helper/Tests' || + entryPath.startsWith('apple/macos-helper/Tests/')) && + !entryPath.endsWith('/PackStub.swift'), + ); + if (shippedSwiftTests.length > 0) { + throw new Error(`npm pack includes macOS helper test sources: ${shippedSwiftTests.join(', ')}`); + } + // Helper sources without the manifest are not a buildable Swift package, so a dropped + // `files` entry that ships Sources but not Package.swift must fail here, not on the user's + // machine. With the manifest present, read the target list from it — the pack and the + // manifest can then never drift onto different target sets. + if (paths.some((entryPath) => entryPath.startsWith('apple/macos-helper/Sources/'))) { + if (!paths.includes('apple/macos-helper/Package.swift')) { + throw new Error('npm pack ships macOS helper sources without its Package.swift'); + } + } + if (paths.includes('apple/macos-helper/Package.swift')) { + const manifestSource = + options.macosHelperManifestSource ?? + fs.readFileSync( + path.join(options.repoRoot ?? process.cwd(), 'apple/macos-helper/Package.swift'), + 'utf8', + ); + const testTargets = readMacosHelperTestTargets(manifestSource); + const missingStubs = testTargets.filter( + (target) => !paths.includes(`apple/macos-helper/Tests/${target}/PackStub.swift`), + ); + if (missingStubs.length > 0) { + throw new Error( + `npm pack is missing macOS helper test-target pack stubs: ${missingStubs + .map((target) => `apple/macos-helper/Tests/${target}/PackStub.swift`) + .join(', ')}`, + ); + } + } } function parseNpmPackOutput(stdout) { diff --git a/test/repo-guards/macos-helper-package-contents.test.ts b/test/repo-guards/macos-helper-package-contents.test.ts new file mode 100644 index 0000000000..ea585c336d --- /dev/null +++ b/test/repo-guards/macos-helper-package-contents.test.ts @@ -0,0 +1,76 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { runCmdSync } from '@agent-device/host-kit/command'; +import { test } from 'vitest'; +import { readMacosHelperTestTargets } from '../../scripts/size-report-package.mjs'; + +// The npm package ships `apple/macos-helper` and builds it on the user's machine. Its manifest +// declares the test targets, and `files` ships only each target's PackStub.swift: SwiftPM refuses +// any load of a package whose declared test target has no sources, so the stubs are what keep the +// install-time `swift build` of the packed tree alive while the real tests stay out of the +// tarball. That forces the whole package — sources, tests, and manifest — into one directory, +// which keeps every tool that copies the tree (Stryker's sandbox, coverage gates, Windows +// checkouts) away from directory symlinks. Nothing else verifies the packed tree compiles — its +// first consumer is a user's machine — so these guards hold the layout against the real pack list, +// not the `files` text. +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const helperRoot = path.join(repoRoot, 'apple/macos-helper'); +const shippedManifestPath = path.join(helperRoot, 'Package.swift'); + +function shippedManifest(): string { + return fs.readFileSync(shippedManifestPath, 'utf8'); +} + +function declaredTargetNames(manifest: string, kind: 'target' | 'executableTarget'): string[] { + return [ + ...manifest.matchAll(new RegExp(String.raw`\.${kind}\(\s*name:\s*"([^"]+)"`, 'g')), + ].flatMap((match) => (match[1] === undefined ? [] : [match[1]])); +} + +function packedPaths(): string[] { + // runCmdSync resolves `npm` through the PATH extensions a Windows checkout needs; a bare + // execFileSync('npm', ...) there dies on ENOENT for npm.cmd. + const result = runCmdSync('npm', ['pack', '--dry-run', '--json', '--ignore-scripts'], { + cwd: repoRoot, + maxBuffer: 64 * 1024 * 1024, + }); + const parsed = JSON.parse(result.stdout); + const pack: { files?: { path?: unknown }[] } = Array.isArray(parsed) ? parsed[0] : parsed; + return pack.files?.map((entry) => String(entry.path)) ?? []; +} + +test('the shipped macOS helper manifest declares the helper and its test targets', () => { + const manifest = shippedManifest(); + assert.deepEqual(declaredTargetNames(manifest, 'target'), ['AgentDeviceMacOSInput']); + assert.deepEqual(declaredTargetNames(manifest, 'executableTarget'), ['AgentDeviceMacOSHelper']); + assert.deepEqual(readMacosHelperTestTargets(manifest).sort(), [ + 'AgentDeviceMacOSHelperTests', + 'AgentDeviceMacOSInputTests', + ]); + assert.match(manifest, /\.executable\(\s*name:\s*"agent-device-macos-helper"/); +}); + +test('every declared test target holds at least one real test in the checkout', () => { + for (const name of readMacosHelperTestTargets(shippedManifest())) { + const sources = fs.readdirSync(path.join(helperRoot, 'Tests', name)); + assert.ok( + sources.some((file) => file !== 'PackStub.swift' && file.endsWith('.swift')), + `${name} must hold real tests in the checkout or the lane would cover nothing`, + ); + } +}); + +test('the pack ships one PackStub.swift per declared test target and no other test source', () => { + const paths = packedPaths(); + const testTargetPaths = paths.filter((entryPath) => + entryPath.startsWith('apple/macos-helper/Tests/'), + ); + // Behavior, not config text: whatever `files` says, the tarball itself must carry exactly the + // stubs — a missing one breaks the install-time `swift build`, an extra one ships dead tests. + const expected = readMacosHelperTestTargets(shippedManifest()) + .map((target) => `apple/macos-helper/Tests/${target}/PackStub.swift`) + .sort(); + assert.deepEqual([...testTargetPaths].sort(), expected); +});