Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .fallowrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down
4 changes: 4 additions & 0 deletions apple/macos-helper/Package.swift
Original file line number Diff line number Diff line change
@@ -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)],
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {}
Original file line number Diff line number Diff line change
@@ -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 {}
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
96 changes: 79 additions & 17 deletions scripts/__tests__/fixtures/size-report-npm-pack.json
Original file line number Diff line number Diff line change
@@ -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",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: This reformat turns the fixture's unchanged entries into expanded objects, so the diff is far larger than the two added entries. Restore the compact one-line object style used before this PR so the change only shows the new Package.swift and PackStub.swift entries.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/__tests__/fixtures/size-report-npm-pack.json, line 41:

<comment>This reformat turns the fixture's unchanged entries into expanded objects, so the diff is far larger than the two added entries. Restore the compact one-line object style used before this PR so the change only shows the new `Package.swift` and `PackStub.swift` entries.</comment>

<file context>
@@ -1,21 +1,83 @@
+      "size": 0
+    },
+    {
+      "path": "apple/macos-helper/Package.swift",
+      "size": 1057
+    },
</file context>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: true. My script rewrite of the fixture re-expanded every entry into multi-line objects; the compact one-line style should be restored, keeping only the three added entries (Package.swift + the two PackStub.swift). Not pushed this round: the coordinator closed guard-review rounds at 563d35e and asked that further threads be left for the maintainer, and a push would re-trigger this bot's review. Leaving open; uncontroversial formatting fix.

"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
}
65 changes: 60 additions & 5 deletions scripts/__tests__/size-report-package.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/,
);
Expand All @@ -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', () => {
Expand Down
32 changes: 31 additions & 1 deletion scripts/coverage-changed/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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])],
Expand Down
10 changes: 9 additions & 1 deletion scripts/coverage-changed/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
19 changes: 19 additions & 0 deletions scripts/coverage-changed/run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
10 changes: 6 additions & 4 deletions scripts/coverage-crap/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }));
}

Expand Down
19 changes: 19 additions & 0 deletions scripts/coverage-crap/run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
Loading
Loading