fix(cli): close the remaining rough edges in paths, recall, yams and baselines #4

Merged
trevon merged 2 commits from trevon/cli-rough-edges into main 2026-09-24 02:36:11 +00:00 AGit
6 changed files with 200 additions and 13 deletions

View file

@ -94,7 +94,9 @@ with `--yams-name <name>` persists the full markdown report (not the
budget-trimmed brief) to the yams corpus, tagged with the analysed commit and a
generation timestamp;
`recall --manifest <path> --root <dir>` locate the labelled-bug manifest and the
directory holding the repos it names, and `recall --strict` exits non-zero when a
directory holding the repos it names (without `--manifest`, `recall/manifest.json`
in the working directory, else the one in the pipe checkout the binary was built
from), and `recall --strict` exits non-zero when a
case it expects to still find is missed. Every numeric flag rejects a value that
is not a positive integer rather than falling back to its default.
@ -115,7 +117,9 @@ Output contract:
`verify --expect-resolved`, `recall --strict`, `index --fail-on-error`, or
`guide --yams` could not persist); 2 pipe could not do what was asked (a bad
path or `--dir`, an unreadable project, a baseline that cannot be read or
written); 64 invalid usage.
written, or an internal error - rerun with `PIPE_DEBUG=1` for its stack
trace); 64 invalid usage, including more than one project path.
`guide --yams --write-baseline` still writes the baseline when yams fails.
`pipe` finds the right SDK itself: it reads `flutterRoot` from the target's
`.dart_tool/package_config.json`, then `FLUTTER_ROOT`, then `flutter` on `PATH`.

View file

@ -33,5 +33,16 @@ Future<void> main(List<String> arguments) async {
stderr.writeln(error.message);
stderr.writeln(error.usage);
exitCode = 64;
} catch (error, stack) {
// Anything else is a pipe bug. Say so in one line with the documented
// "could not do what was asked" code instead of a raw trace and exit 255;
// PIPE_DEBUG=1 restores the trace for a bug report.
stderr.writeln('pipe: internal error: $error');
if (Platform.environment['PIPE_DEBUG'] == '1') {
stderr.writeln(stack);
} else {
stderr.writeln('pipe: rerun with PIPE_DEBUG=1 for a stack trace');
}
exitCode = 2;
}
}

View file

@ -122,7 +122,11 @@ void _writeBaseline(ArgResults results, String root, List<Finding> findings) {
final path = results['write-baseline'] as String?;
if (path == null) return;
try {
File(path).writeAsStringSync(encodeBaseline(root, findings));
// Record the project root relative to the baseline file, so a committed
// baseline does not carry one machine's absolute path (and does not churn
// when another checkout rewrites it). Matching never reads `root`.
final relativeRoot = p.relative(root, from: p.dirname(p.absolute(path)));
File(path).writeAsStringSync(encodeBaseline(relativeRoot, findings));
} on FileSystemException catch (error) {
throw PipeException(
'cannot write baseline $path: '
@ -169,8 +173,50 @@ Set<String> _readRules(ArgResults results) {
return selected;
}
String _target(ArgResults results) =>
results.rest.isEmpty ? '.' : results.rest.first;
/// The one project path a command analyses, `.` when none is given.
///
/// Extra positionals used to be dropped silently, so `pipe check a b` analysed
/// only `a` and reported as if both had been checked.
String _target(ArgResults results) {
final rest = results.rest;
if (rest.length > 1) {
throw UsageException(
'expected one project path, got ${rest.length}: ${rest.join(' ')}',
'Run "pipe help <command>" for usage; run pipe once per project.',
);
}
return rest.isEmpty ? '.' : rest.first;
}
/// Where `recall` looks for its manifest when `--manifest` is not given.
///
/// The default used to be `recall/manifest.json` relative to the working
/// directory, so `pipe recall` only worked from inside the pipe repo. Prefer that
/// path when it exists, otherwise search upward from the running script and
/// executable (the source checkout for `dart run`, the build bundle for the AOT
/// binary) for the repo's `recall/manifest.json`.
String defaultRecallManifest({String? cwd, List<String>? searchFrom}) {
const relative = 'recall/manifest.json';
final local = p.join(cwd ?? Directory.current.path, relative);
if (File(local).existsSync()) return local;
final starts =
searchFrom ??
[
if (Platform.script.scheme == 'file') Platform.script.toFilePath(),
Platform.resolvedExecutable,
];
for (final start in starts) {
var dir = p.dirname(p.absolute(start));
while (true) {
final candidate = p.join(dir, relative);
if (File(candidate).existsSync()) return candidate;
final parent = p.dirname(dir);
if (parent == dir) break;
dir = parent;
}
}
return local;
}
String _encode(Map<String, Object?> json) =>
const JsonEncoder.withIndent(' ').convert(json);
@ -424,8 +470,9 @@ class RecallCommand extends Command<int> {
argParser
..addOption(
'manifest',
defaultsTo: 'recall/manifest.json',
help: 'Manifest of labelled known bugs.',
help:
'Manifest of labelled known bugs (default: recall/manifest.json '
'in the working directory, else in the pipe checkout).',
)
..addOption(
'root',
@ -466,7 +513,7 @@ class RecallCommand extends Command<int> {
final results = argResults!;
final root = (results['root'] as String?) ?? Directory.current.path;
final report = await runRecall(
manifestPath: results['manifest'] as String,
manifestPath: (results['manifest'] as String?) ?? defaultRecallManifest(),
root: root,
sdkOverride: results['sdk'] as String?,
concurrency: _readConcurrency(results),
@ -784,6 +831,9 @@ class GuideCommand extends Command<int> {
collection: 'pipe',
);
if (error != null) {
// Write the baseline anyway: a yams outage must not also lose the
// accepted findings the caller asked to record.
_writeBaseline(results, project.root, analysis.report.findings);
stderr.writeln('pipe: could not persist to yams: $error');
return 1;
}

View file

@ -743,15 +743,20 @@ Future<String?> emitToYams({
if (collection != null) arguments.addAll(['--collection', collection]);
final process = await Process.start(executable, arguments);
// Drain both pipes concurrently. Reading only stderr let a chatty `yams add`
// fill its stdout buffer and block forever.
final stdoutDone = process.stdout.drain<void>();
final stderrText = process.stderr
.transform(const SystemEncoding().decoder)
.join();
process.stdin.write(content);
await process.stdin.flush();
await process.stdin.close();
final stderrText = await process.stderr
.transform(const SystemEncoding().decoder)
.join();
await stdoutDone;
final errorText = await stderrText;
final exitCode = await process.exitCode;
if (exitCode != 0) {
return 'yams add failed (exit $exitCode): ${stderrText.trim()}';
return 'yams add failed (exit $exitCode): ${errorText.trim()}';
}
return null;
}

View file

@ -207,7 +207,7 @@ List<RecallCase> loadRecallCases(String manifestPath) {
if (!file.existsSync()) {
throw PipeException(
'recall manifest not found: $manifestPath',
remedy: 'pass --manifest <path> or run from the pipe repo root',
remedy: 'pass --manifest <path>',
);
}
final Object? decoded;

117
test/cli_edges_test.dart Normal file
View file

@ -0,0 +1,117 @@
import 'dart:convert';
import 'dart:io';
import 'package:args/command_runner.dart';
import 'package:path/path.dart' as p;
import 'package:pipe/src/cli.dart';
import 'package:test/test.dart';
void main() {
test(
'a second project path is a usage error, not silently dropped',
() async {
// `pipe check a b` used to analyse only `a` and report as if both ran.
await expectLater(
PipeRunner().run(['check', 'test/fixtures/stuck_loading', 'lib']),
throwsA(
isA<UsageException>().having(
(e) => e.message,
'message',
contains('expected one project path, got 2'),
),
),
);
},
);
test('recall finds the pipe manifest from outside the repo', () {
// The default used to be cwd-relative, so `pipe recall` failed anywhere
// but the pipe checkout.
final elsewhere = Directory.systemTemp.createTempSync('pipe-recall-cwd');
final script = p.join(Directory.current.path, 'bin', 'pipe.dart');
final found = defaultRecallManifest(
cwd: elsewhere.path,
searchFrom: [script],
);
expect(
p.equals(found, p.join(Directory.current.path, 'recall/manifest.json')),
isTrue,
reason: found,
);
});
test('recall prefers a manifest in the working directory', () {
final temp = Directory.systemTemp.createTempSync('pipe-recall-local');
final local = File(p.join(temp.path, 'recall', 'manifest.json'))
..createSync(recursive: true)
..writeAsStringSync('[]');
expect(
defaultRecallManifest(cwd: temp.path, searchFrom: const []),
local.path,
);
});
test('guide --yams writes the baseline even when yams fails, and does not '
'hang on a chatty yams', () async {
// A fake yams that floods stdout (more than a pipe buffer) and then fails.
// Before: pipe read only stderr, so the child blocked on a full stdout and
// the run hung; and the yams failure returned before --write-baseline ran.
final temp = Directory.systemTemp.createTempSync('pipe-fake-yams');
final fake = File(p.join(temp.path, 'yams'))
..writeAsStringSync(
'#!/bin/sh\n'
'cat >/dev/null\n'
'head -c 1048576 /dev/zero | tr "\\\\0" x\n'
'echo "fake yams down" >&2\n'
'exit 3\n',
);
await Process.run('chmod', ['+x', fake.path]);
final baseline = p.join(temp.path, 'baseline.json');
final result = await Process.run(
Platform.resolvedExecutable,
[
'bin/pipe.dart',
'guide',
'test/fixtures/stuck_loading',
'--yams',
'--write-baseline',
baseline,
'-c',
'1',
],
environment: {'PATH': '${temp.path}:/usr/bin:/bin'},
).timeout(const Duration(minutes: 2));
expect(result.exitCode, 1, reason: '${result.stderr}');
expect(result.stderr as String, contains('fake yams down'));
expect(File(baseline).existsSync(), isTrue);
});
test('a written baseline records its root relative to the file', () async {
// It used to store the absolute project path, so a committed baseline
// leaked one machine's home directory and churned on every checkout.
final temp = Directory.systemTemp.createTempSync('pipe-baseline-root');
final baseline = p.join(temp.path, 'baseline.json');
final result = await Process.run(Platform.resolvedExecutable, [
'bin/pipe.dart',
'check',
'test/fixtures/stuck_loading',
'--write-baseline',
baseline,
'-c',
'1',
]);
expect(result.exitCode, 0, reason: '${result.stderr}');
final root = (jsonDecode(File(baseline).readAsStringSync()) as Map)['root'];
expect(p.isAbsolute(root as String), isFalse, reason: root);
expect(
p.equals(
p.normalize(p.join(temp.path, root)),
p.absolute('test/fixtures/stuck_loading'),
),
isTrue,
reason: root,
);
});
}