From 46d1f46c6fdfcc6085256aaf2675109bf301bdff Mon Sep 17 00:00:00 2001 From: Martin Husbyn Date: Tue, 9 Jun 2026 13:56:35 +0100 Subject: [PATCH 1/2] feat(cli): add samples annotation-template and upload-multiplexed commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements User Story 5 (Phase 5) of the flowbio CLI: the full multiplexed upload flow of downloading a server-generated annotation sheet, filling it in, and submitting multiplexed reads alongside it. - `samples annotation-template` wraps the existing `get_annotation_template`, writing the `.xlsx` bytes to `-o/--output`. Because the body is binary, `-o` is required whenever it would clash with the stream that owns stdout — an interactive terminal or `--json` — failing fast with a usage error before any network call. With a piped stdout in human mode it streams the workbook to stdout. - `samples upload-multiplexed` wraps `upload_multiplexed_data`, reporting the data identifiers, annotation identifier, and any warnings. Warnings are reported but the upload proceeds by default (matching the library default); `--reject-warnings` opts into rejecting them. The samples parser registration is split into one `_configure_*` helper per verb, each taking a public `argparse.ArgumentParser`, keeping `register()` a concise verb registry without leaking argparse's private subparsers type. Co-Authored-By: Claude Opus 4.8 --- docs/cli.md | 75 ++++++++++ flowbio/cli/_samples.py | 140 ++++++++++++++++++- specs/001-flowbio-cli/tasks.md | 10 +- tests/unit/cli/test_samples.py | 242 +++++++++++++++++++++++++++++++++ 4 files changed, 460 insertions(+), 7 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 6925748..672eca2 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -148,3 +148,78 @@ $ flowbio samples upload --name liver_r1 --sample-type rna_seq \ --reads1 ./liver_R1.fastq.gz --json {"id": "samp_abc"} ``` + +### `samples annotation-template` + +Download the server-generated annotation sheet template for a sample type, to +fill in before `samples upload-multiplexed`. + +``` +flowbio samples annotation-template [--sample-type TYPE] [-o PATH | --output PATH] +``` + +The template is an Excel workbook (`.xlsx`) keyed by metadata-attribute display +names. It is a **different artefact from the batch sample sheet** (the CLI-built +CSV used by `upload-batch`) and the two are not interchangeable. `--sample-type` +is optional and defaults to `generic` (the base columns shared by all types); a +type-specific value adds that type's metadata columns. It is sent as-is and +validated server-side. + +Because the body is binary, the CLI never writes it where it would clash: pass +`-o/--output PATH` when stdout is an interactive terminal or under `--json` (the +single JSON document owns stdout). In either case, omitting `-o` fails with exit +`2` asking for an output path. With a non-terminal stdout in human mode, omitting +`-o` streams the workbook to stdout (e.g. `… > sheet.xlsx`). + +**Output** — human: the workbook is written to `--output`; a confirmation (path +and sample type) goes to stderr, leaving stdout empty. `--json`: +`{"output": "", "sample_type": ""}` on stdout — never the spreadsheet +bytes. + +**Exit codes** — `0` success; `2` no `--output` while stdout is a terminal or +under `--json`; `4` unknown sample type; `3` authentication failure; otherwise +the standard mapping above. + +**Example** + +```bash +$ flowbio samples annotation-template --sample-type rna_seq -o sheet.xlsx +Wrote rna_seq annotation template to sheet.xlsx + +$ flowbio samples annotation-template --sample-type rna_seq -o sheet.xlsx --json +{"output": "sheet.xlsx", "sample_type": "rna_seq"} +``` + +### `samples upload-multiplexed` + +Upload multiplexed reads plus a completed annotation sheet for server-side +demultiplexing — single-ended (`--reads1`) or paired-end (add `--reads2`). + +``` +flowbio samples upload-multiplexed --reads1 PATH --annotation PATH + [--reads2 PATH] [--reject-warnings] +``` + +The annotation sheet is the filled-in workbook from `annotation-template`. By +default annotation warnings are reported but the upload proceeds; +`--reject-warnings` makes warnings reject it. + +**Output** — human: a confirmation line with the data identifiers and annotation +identifier on stdout, with any warnings on stderr. `--json`: +`{"data_ids": [...], "annotation_id": "", "warnings": [...]}` on stdout. + +**Exit codes** — `0` success (including with reported warnings); `5` annotation +fails server validation, or warnings with `--reject-warnings`; `3` authentication +failure; otherwise the standard mapping above. + +**Example** + +```bash +$ flowbio samples upload-multiplexed --reads1 ./mux_R1.fastq.gz \ + --annotation ./sheet.xlsx +Uploaded multiplexed data mux_1 with annotation ann_1 + +$ flowbio samples upload-multiplexed --reads1 ./mux_R1.fastq.gz \ + --annotation ./sheet.xlsx --json +{"data_ids": ["mux_1"], "annotation_id": "ann_1", "warnings": []} +``` diff --git a/flowbio/cli/_samples.py b/flowbio/cli/_samples.py index f022f5b..a954dc7 100644 --- a/flowbio/cli/_samples.py +++ b/flowbio/cli/_samples.py @@ -22,12 +22,33 @@ def register( ) -> None: """Register the ``samples`` verbs on the resource parser.""" verbs = resource.add_subparsers(dest="verb", metavar="") - upload = verbs.add_parser( + _configure_upload(verbs.add_parser( "upload", parents=[global_parent], help="Upload a single demultiplexed sample.", description="Upload a single demultiplexed sample to the Flow platform.", - ) + )) + _configure_annotation_template(verbs.add_parser( + "annotation-template", + parents=[global_parent], + help="Download the annotation sheet template for multiplexed uploads.", + description=( + "Download the server-generated annotation sheet (.xlsx) template for a " + "sample type, to fill in before `samples upload-multiplexed`." + ), + )) + _configure_upload_multiplexed(verbs.add_parser( + "upload-multiplexed", + parents=[global_parent], + help="Upload multiplexed reads with an annotation sheet.", + description=( + "Upload multiplexed reads plus a completed annotation sheet for " + "server-side demultiplexing." + ), + )) + + +def _configure_upload(upload: argparse.ArgumentParser) -> None: upload.set_defaults(command_parser=upload, handler=_upload_command) upload.add_argument( "--name", @@ -75,6 +96,55 @@ def register( ) +def _configure_annotation_template(annotation_template: argparse.ArgumentParser) -> None: + annotation_template.set_defaults( + command_parser=annotation_template, handler=_annotation_template_command, + ) + annotation_template.add_argument( + "--sample-type", + default="generic", + metavar="TYPE", + help=( + "Sample type identifier (sent as-is; validated server-side). " + "Defaults to 'generic' (base columns common to all types)." + ), + ) + annotation_template.add_argument( + "-o", + "--output", + metavar="PATH", + help="File to write the .xlsx workbook to. Required when stdout is a terminal.", + ) + + +def _configure_upload_multiplexed(upload_multiplexed: argparse.ArgumentParser) -> None: + upload_multiplexed.set_defaults( + command_parser=upload_multiplexed, handler=_upload_multiplexed_command, + ) + upload_multiplexed.add_argument( + "--reads1", + required=True, + metavar="PATH", + help="First multiplexed reads file.", + ) + upload_multiplexed.add_argument( + "--reads2", + metavar="PATH", + help="Second multiplexed reads file (makes the upload paired-end).", + ) + upload_multiplexed.add_argument( + "--annotation", + required=True, + metavar="PATH", + help="Completed annotation sheet (obtained via `annotation-template`).", + ) + upload_multiplexed.add_argument( + "--reject-warnings", + action="store_true", + help="Reject the upload if the annotation sheet has warnings.", + ) + + def _upload_command(args: argparse.Namespace, client: Client, output: Output) -> ExitCode: """Upload a single sample and report its identifier. @@ -99,6 +169,72 @@ def _upload_command(args: argparse.Namespace, client: Client, output: Output) -> return ExitCode.SUCCESS +def _annotation_template_command( + args: argparse.Namespace, client: Client, output: Output, +) -> ExitCode: + """Download an annotation sheet template and write it to a file. + + :param args: Parsed command-line arguments. + :param client: The authenticated Flow client. + :param output: The result/error renderer. + :returns: :attr:`ExitCode.SUCCESS` on success. + """ + destination = Path(args.output) if args.output is not None else None + if destination is None and (output.json_mode or output.stdout.isatty()): + raise CliUsageError( + "The annotation template is a binary workbook; pass -o/--output PATH.", + ) + template = client.samples.get_annotation_template(args.sample_type) + if destination is None: + output.stdout.buffer.write(template) + output.emit_advisory( + f"Wrote {args.sample_type} annotation template to standard output", + ) + return ExitCode.SUCCESS + destination.write_bytes(template) + if output.json_mode: + output.emit_result( + "", {"output": str(destination), "sample_type": args.sample_type}, + ) + else: + output.emit_advisory( + f"Wrote {args.sample_type} annotation template to {destination}", + ) + return ExitCode.SUCCESS + + +def _upload_multiplexed_command( + args: argparse.Namespace, client: Client, output: Output, +) -> ExitCode: + """Upload multiplexed reads and an annotation sheet, reporting identifiers. + + :param args: Parsed command-line arguments. + :param client: The authenticated Flow client. + :param output: The result/error renderer. + :returns: :attr:`ExitCode.SUCCESS` on success. + """ + reads = {"reads1": existing_file(Path(args.reads1))} + if args.reads2 is not None: + reads["reads2"] = existing_file(Path(args.reads2)) + upload = client.samples.upload_multiplexed_data( + reads=reads, + annotation=existing_file(Path(args.annotation)), + ignore_warnings=not args.reject_warnings, + ) + if upload.warnings: + output.emit_advisory(f"Annotation warnings: {upload.warnings}") + output.emit_result( + f"Uploaded multiplexed data {', '.join(upload.data_ids)} " + f"with annotation {upload.annotation_id}", + { + "data_ids": upload.data_ids, + "annotation_id": upload.annotation_id, + "warnings": upload.warnings, + }, + ) + return ExitCode.SUCCESS + + def _merge_metadata( pairs: list[str] | None, json_text: str | None, ) -> dict[str, str]: diff --git a/specs/001-flowbio-cli/tasks.md b/specs/001-flowbio-cli/tasks.md index dc2d3d0..0161cab 100644 --- a/specs/001-flowbio-cli/tasks.md +++ b/specs/001-flowbio-cli/tasks.md @@ -118,14 +118,14 @@ Single-project layout (per plan.md): CLI under `flowbio/cli/` (every module `_`- ### Tests for User Story 5 (MANDATORY — write first) ⚠️ -- [ ] T020 [P] [US5] Write failing tests in `tests/unit/cli/test_samples.py` covering `annotation-template` (US5 scenarios 1–3): writes the server-generated `.xlsx` bytes verbatim to `-o/--output` with a confirmation (path, sample type) on stderr (exit 0); `--sample-type` optional, defaults to `"generic"`; no `-o` with a TTY stdout → exit 2 asking for an output path; `--json` emits a single `{"output", "sample_type"}` document on stdout with no spreadsheet bytes there; unknown `--sample-type` (server not-found) → exit 4 (contracts/samples-annotation-template.md) -- [ ] T022 [P] [US5] Write failing tests in `tests/unit/cli/test_samples.py` covering `upload-multiplexed` (US5 scenarios 4–7): submit + report `data_ids`/`annotation_id`/`warnings` (exit 0); `--reads2` → paired-end; warnings reported but upload proceeds by default (`ignore_warnings=True`), `--reject-warnings` rejects → exit 5; annotation fails server validation → exit 5 (contracts/samples-upload-multiplexed.md) +- [X] T020 [P] [US5] Write failing tests in `tests/unit/cli/test_samples.py` covering `annotation-template` (US5 scenarios 1–3): writes the server-generated `.xlsx` bytes verbatim to `-o/--output` with a confirmation (path, sample type) on stderr (exit 0); `--sample-type` optional, defaults to `"generic"`; no `-o` with a TTY stdout → exit 2 asking for an output path; `--json` emits a single `{"output", "sample_type"}` document on stdout with no spreadsheet bytes there; unknown `--sample-type` (server not-found) → exit 4 (contracts/samples-annotation-template.md) +- [X] T022 [P] [US5] Write failing tests in `tests/unit/cli/test_samples.py` covering `upload-multiplexed` (US5 scenarios 4–7): submit + report `data_ids`/`annotation_id`/`warnings` (exit 0); `--reads2` → paired-end; warnings reported but upload proceeds by default (`ignore_warnings=True`), `--reject-warnings` rejects → exit 5; annotation fails server validation → exit 5 (contracts/samples-upload-multiplexed.md) ### Implementation for User Story 5 -- [ ] T021 [US5] Implement the `annotation-template` handler and `AnnotationTemplate` result in `flowbio/cli/_samples.py` wrapping the existing `client.samples.get_annotation_template(sample_type)` (default `sample_type="generic"`, passed through unvalidated), writing the returned `.xlsx` bytes to `-o/--output`, refusing to write binary to a TTY stdout (USAGE), and emitting the `{output, sample_type}` document under `--json`; register the subcommand via the `register()` in `flowbio/cli/_samples.py` (wired into `flowbio/cli/_parser.py`) with `help=`/description text on every argument (FR-003, SC-008) (FR-043, FR-044) -- [ ] T023 [US5] Implement the `upload-multiplexed` handler in `flowbio/cli/_samples.py` wrapping `client.samples.upload_multiplexed_data(reads, annotation, ignore_warnings=...)` (default `ignore_warnings=True`; `--reject-warnings` flips it) reporting `data_ids`/`annotation_id`/`warnings`, and register the subcommand via the `register()` in `flowbio/cli/_samples.py` (wired into `flowbio/cli/_parser.py`) with `help=`/description text on every argument (FR-003, SC-008) (FR-033, FR-034) -- [ ] T024 [US5] Document `annotation-template` and `upload-multiplexed` (the annotation sheet is a server `.xlsx`, distinct from the batch CSV; binary-to-TTY guard; warning behaviour; output, exit codes, worked examples) in `docs/cli.md` (FR-041, FR-042) +- [X] T021 [US5] Implement the `annotation-template` handler and `AnnotationTemplate` result in `flowbio/cli/_samples.py` wrapping the existing `client.samples.get_annotation_template(sample_type)` (default `sample_type="generic"`, passed through unvalidated), writing the returned `.xlsx` bytes to `-o/--output`, refusing to write binary to a TTY stdout (USAGE), and emitting the `{output, sample_type}` document under `--json`; register the subcommand via the `register()` in `flowbio/cli/_samples.py` (wired into `flowbio/cli/_parser.py`) with `help=`/description text on every argument (FR-003, SC-008) (FR-043, FR-044) +- [X] T023 [US5] Implement the `upload-multiplexed` handler in `flowbio/cli/_samples.py` wrapping `client.samples.upload_multiplexed_data(reads, annotation, ignore_warnings=...)` (default `ignore_warnings=True`; `--reject-warnings` flips it) reporting `data_ids`/`annotation_id`/`warnings`, and register the subcommand via the `register()` in `flowbio/cli/_samples.py` (wired into `flowbio/cli/_parser.py`) with `help=`/description text on every argument (FR-003, SC-008) (FR-033, FR-034) +- [X] T024 [US5] Document `annotation-template` and `upload-multiplexed` (the annotation sheet is a server `.xlsx`, distinct from the batch CSV; binary-to-TTY guard; warning behaviour; output, exit codes, worked examples) in `docs/cli.md` (FR-041, FR-042) **Checkpoint**: The full multiplexed flow (download template → fill in → upload) works independently and is demonstrable. diff --git a/tests/unit/cli/test_samples.py b/tests/unit/cli/test_samples.py index c9d4276..4434b8b 100644 --- a/tests/unit/cli/test_samples.py +++ b/tests/unit/cli/test_samples.py @@ -1,3 +1,4 @@ +import io import json from http import HTTPStatus from pathlib import Path @@ -9,6 +10,9 @@ from tests.unit.v2.conftest import parse_multipart SAMPLE_UPLOAD_URL = f"{DEFAULT_BASE_URL}/upload/sample" +ANNOTATION_TEMPLATE_URL = f"{DEFAULT_BASE_URL}/annotation" +ANNOTATION_UPLOAD_URL = f"{DEFAULT_BASE_URL}/upload/annotation" +MULTIPLEXED_UPLOAD_URL = f"{DEFAULT_BASE_URL}/upload/multiplexed" TOKEN = "test.token" @@ -192,3 +196,241 @@ def test_name_help_does_not_promise_cli_space_validation(self, run_cli) -> None: assert result.exit_code == 0 assert "must not contain spaces" not in result.stdout + + +def _mock_annotation_template( + sample_type: str, content: bytes = b"PK\x03\x04xlsx", +) -> respx.Route: + return respx.get(f"{ANNOTATION_TEMPLATE_URL}/{sample_type}").mock( + return_value=httpx.Response(HTTPStatus.OK, content=content), + ) + + +class TestSamplesAnnotationTemplate: + + @respx.mock + def test_writes_xlsx_bytes_to_output_with_confirmation( + self, run_cli, tmp_path: Path, + ) -> None: + sample_type = "rna_seq" + workbook = b"PK\x03\x04 fake xlsx workbook bytes" + _mock_annotation_template(sample_type, workbook) + output_path = tmp_path / "sheet.xlsx" + + result = run_cli( + "--token", TOKEN, "samples", "annotation-template", + "--sample-type", sample_type, "-o", str(output_path), + ) + + assert result.exit_code == 0 + assert output_path.read_bytes() == workbook + assert str(output_path) in result.stderr + assert sample_type in result.stderr + assert result.stdout == "" + + @respx.mock + def test_sample_type_defaults_to_generic( + self, run_cli, tmp_path: Path, + ) -> None: + route = _mock_annotation_template("generic") + output_path = tmp_path / "sheet.xlsx" + + result = run_cli( + "--token", TOKEN, "samples", "annotation-template", + "-o", str(output_path), + ) + + assert result.exit_code == 0 + assert route.called + + @respx.mock + def test_no_output_with_tty_stdout_is_usage_error( + self, run_cli, monkeypatch, + ) -> None: + class _TtyStringIO(io.StringIO): + def isatty(self) -> bool: + return True + + monkeypatch.setattr(io, "StringIO", _TtyStringIO) + route = _mock_annotation_template("generic") + + result = run_cli("--token", TOKEN, "samples", "annotation-template") + + assert result.exit_code == 2 + assert route.call_count == 0 + + @respx.mock + def test_json_without_output_is_usage_error( + self, run_cli, + ) -> None: + route = _mock_annotation_template("generic") + + result = run_cli("--token", TOKEN, "samples", "annotation-template", "--json") + + assert result.exit_code == 2 + assert route.call_count == 0 + + @respx.mock + def test_json_emits_single_document_without_bytes( + self, run_cli, tmp_path: Path, + ) -> None: + sample_type = "rna_seq" + _mock_annotation_template(sample_type) + output_path = tmp_path / "sheet.xlsx" + + result = run_cli( + "--token", TOKEN, "samples", "annotation-template", + "--sample-type", sample_type, "-o", str(output_path), "--json", + ) + + assert result.exit_code == 0 + assert json.loads(result.stdout) == { + "output": str(output_path), "sample_type": sample_type, + } + assert result.stdout.count("\n") == 1 + + @respx.mock + def test_unknown_sample_type_is_not_found( + self, run_cli, tmp_path: Path, + ) -> None: + respx.get(f"{ANNOTATION_TEMPLATE_URL}/nope").mock( + return_value=httpx.Response( + HTTPStatus.NOT_FOUND, json={"error": "no such sample type"}, + ), + ) + output_path = tmp_path / "sheet.xlsx" + + result = run_cli( + "--token", TOKEN, "samples", "annotation-template", + "--sample-type", "nope", "-o", str(output_path), + ) + + assert result.exit_code == 4 + + +def _annotation(tmp_path: Path) -> Path: + path = tmp_path / "annotation.xlsx" + path.write_bytes(b"PK\x03\x04annotation") + return path + + +def _mock_annotation_accepted(annotation_id: str = "ann_1") -> respx.Route: + return respx.post(ANNOTATION_UPLOAD_URL).mock( + return_value=httpx.Response(HTTPStatus.OK, json={"id": annotation_id}), + ) + + +def _mock_multiplexed(data_id: str = "mux_1") -> respx.Route: + return respx.post(MULTIPLEXED_UPLOAD_URL).mock( + return_value=httpx.Response(HTTPStatus.OK, json={"id": data_id}), + ) + + +class TestSamplesUploadMultiplexed: + + @respx.mock + def test_reports_data_annotation_ids_and_warnings( + self, run_cli, tmp_path: Path, + ) -> None: + annotation_id = "ann_xyz" + data_id = "mux_xyz" + _mock_annotation_accepted(annotation_id) + _mock_multiplexed(data_id) + + result = run_cli( + "--token", TOKEN, "samples", "upload-multiplexed", + "--reads1", str(_reads(tmp_path, "r1.fq.gz")), + "--annotation", str(_annotation(tmp_path)), + "--no-progress", "--json", + ) + + assert result.exit_code == 0 + assert json.loads(result.stdout) == { + "data_ids": [data_id], "annotation_id": annotation_id, "warnings": [], + } + + @respx.mock + def test_reads2_makes_it_paired_end(self, run_cli, tmp_path: Path) -> None: + _mock_annotation_accepted() + multiplexed = respx.post(MULTIPLEXED_UPLOAD_URL) + multiplexed.side_effect = [ + httpx.Response(HTTPStatus.OK, json={"id": "mux_1"}), + httpx.Response(HTTPStatus.OK, json={"id": "mux_2"}), + ] + + result = run_cli( + "--token", TOKEN, "samples", "upload-multiplexed", + "--reads1", str(_reads(tmp_path, "r1.fq.gz")), + "--reads2", str(_reads(tmp_path, "r2.fq.gz")), + "--annotation", str(_annotation(tmp_path)), + "--no-progress", + ) + + assert result.exit_code == 0 + assert multiplexed.call_count == 2 + + @respx.mock + def test_warnings_reported_but_upload_proceeds_by_default( + self, run_cli, tmp_path: Path, + ) -> None: + warnings = [{"row": 1, "message": "Unknown barcode"}] + annotation = respx.post(ANNOTATION_UPLOAD_URL) + annotation.side_effect = [ + httpx.Response(HTTPStatus.BAD_REQUEST, json={"warnings": warnings}), + httpx.Response(HTTPStatus.OK, json={"id": "ann_1"}), + ] + _mock_multiplexed() + + result = run_cli( + "--token", TOKEN, "samples", "upload-multiplexed", + "--reads1", str(_reads(tmp_path, "r1.fq.gz")), + "--annotation", str(_annotation(tmp_path)), + "--no-progress", "--json", + ) + + assert result.exit_code == 0 + assert json.loads(result.stdout)["warnings"] == warnings + + @respx.mock + def test_reject_warnings_rejects_upload( + self, run_cli, tmp_path: Path, + ) -> None: + respx.post(ANNOTATION_UPLOAD_URL).mock( + return_value=httpx.Response( + HTTPStatus.BAD_REQUEST, + json={"warnings": [{"row": 1, "message": "Unknown barcode"}]}, + ), + ) + multiplexed = _mock_multiplexed() + + result = run_cli( + "--token", TOKEN, "samples", "upload-multiplexed", + "--reads1", str(_reads(tmp_path, "r1.fq.gz")), + "--annotation", str(_annotation(tmp_path)), + "--reject-warnings", "--no-progress", + ) + + assert result.exit_code == 5 + assert multiplexed.call_count == 0 + + @respx.mock + def test_annotation_validation_failure_rejects_upload( + self, run_cli, tmp_path: Path, + ) -> None: + respx.post(ANNOTATION_UPLOAD_URL).mock( + return_value=httpx.Response( + HTTPStatus.BAD_REQUEST, + json={"validation": [{"row": 1, "message": "Invalid scientist"}]}, + ), + ) + multiplexed = _mock_multiplexed() + + result = run_cli( + "--token", TOKEN, "samples", "upload-multiplexed", + "--reads1", str(_reads(tmp_path, "r1.fq.gz")), + "--annotation", str(_annotation(tmp_path)), + "--no-progress", + ) + + assert result.exit_code == 5 + assert multiplexed.call_count == 0 From f1a580748d9d9611f27963997a9f1803ea4f8f3d Mon Sep 17 00:00:00 2001 From: Martin Husbyn Date: Tue, 9 Jun 2026 14:25:34 +0100 Subject: [PATCH 2/2] fix(cli): harden annotation-template output handling and warning rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on the Phase 5 commands: - Make `-o/--output` always required for `samples annotation-template` and drop the stdout-streaming branch. The workbook is binary and the streamed path was both untestable (the test harness's StringIO has no `.buffer`) and prone to clashing with the JSON document on stdout. Requiring an output path is a strict superset of the previous "required when stdout is a terminal" rule; the contract and docs are updated to match. - Guard the output write against `OSError` (e.g. a missing parent directory or an unwritable path), re-raising as a `CliUsageError` so a bad `--output` returns exit 2 instead of an uncaught traceback — symmetric with the input-side `existing_file` guard. - Correct the `-o` help text (no longer claims it is only conditionally required). - Render annotation warnings one per line via `_format_warning` instead of a raw list-of-dicts repr. Co-Authored-By: Claude Opus 4.8 --- docs/cli.md | 14 +++--- flowbio/cli/_samples.py | 34 +++++++------ .../contracts/samples-annotation-template.md | 7 ++- specs/001-flowbio-cli/tasks.md | 2 +- tests/unit/cli/test_samples.py | 49 +++++++++++++------ 5 files changed, 65 insertions(+), 41 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 672eca2..022184f 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -165,20 +165,18 @@ is optional and defaults to `generic` (the base columns shared by all types); a type-specific value adds that type's metadata columns. It is sent as-is and validated server-side. -Because the body is binary, the CLI never writes it where it would clash: pass -`-o/--output PATH` when stdout is an interactive terminal or under `--json` (the -single JSON document owns stdout). In either case, omitting `-o` fails with exit -`2` asking for an output path. With a non-terminal stdout in human mode, omitting -`-o` streams the workbook to stdout (e.g. `… > sheet.xlsx`). +The body is a binary workbook, so `-o/--output PATH` is **required** — it is +never written to stdout (which carries human result lines or the single JSON +document). **Output** — human: the workbook is written to `--output`; a confirmation (path and sample type) goes to stderr, leaving stdout empty. `--json`: `{"output": "", "sample_type": ""}` on stdout — never the spreadsheet bytes. -**Exit codes** — `0` success; `2` no `--output` while stdout is a terminal or -under `--json`; `4` unknown sample type; `3` authentication failure; otherwise -the standard mapping above. +**Exit codes** — `0` success; `2` no `--output`, or an unwritable output path; +`4` unknown sample type; `3` authentication failure; otherwise the standard +mapping above. **Example** diff --git a/flowbio/cli/_samples.py b/flowbio/cli/_samples.py index a954dc7..9cdee41 100644 --- a/flowbio/cli/_samples.py +++ b/flowbio/cli/_samples.py @@ -112,8 +112,9 @@ def _configure_annotation_template(annotation_template: argparse.ArgumentParser) annotation_template.add_argument( "-o", "--output", + required=True, metavar="PATH", - help="File to write the .xlsx workbook to. Required when stdout is a terminal.", + help="File to write the .xlsx workbook to (the template is binary).", ) @@ -179,19 +180,14 @@ def _annotation_template_command( :param output: The result/error renderer. :returns: :attr:`ExitCode.SUCCESS` on success. """ - destination = Path(args.output) if args.output is not None else None - if destination is None and (output.json_mode or output.stdout.isatty()): - raise CliUsageError( - "The annotation template is a binary workbook; pass -o/--output PATH.", - ) + destination = Path(args.output) template = client.samples.get_annotation_template(args.sample_type) - if destination is None: - output.stdout.buffer.write(template) - output.emit_advisory( - f"Wrote {args.sample_type} annotation template to standard output", - ) - return ExitCode.SUCCESS - destination.write_bytes(template) + try: + destination.write_bytes(template) + except OSError as error: + raise CliUsageError( + f"Could not write annotation template to {destination}: {error}", + ) from error if output.json_mode: output.emit_result( "", {"output": str(destination), "sample_type": args.sample_type}, @@ -222,7 +218,9 @@ def _upload_multiplexed_command( ignore_warnings=not args.reject_warnings, ) if upload.warnings: - output.emit_advisory(f"Annotation warnings: {upload.warnings}") + output.emit_advisory("Annotation warnings:") + for warning in upload.warnings: + output.emit_advisory(f" {_format_warning(warning)}") output.emit_result( f"Uploaded multiplexed data {', '.join(upload.data_ids)} " f"with annotation {upload.annotation_id}", @@ -235,6 +233,14 @@ def _upload_multiplexed_command( return ExitCode.SUCCESS +def _format_warning(warning: dict) -> str: + if "message" not in warning: + return str(warning) + row = warning.get("row") + prefix = f"row {row}: " if row is not None else "" + return f"{prefix}{warning['message']}" + + def _merge_metadata( pairs: list[str] | None, json_text: str | None, ) -> dict[str, str]: diff --git a/specs/001-flowbio-cli/contracts/samples-annotation-template.md b/specs/001-flowbio-cli/contracts/samples-annotation-template.md index f880061..cfad62f 100644 --- a/specs/001-flowbio-cli/contracts/samples-annotation-template.md +++ b/specs/001-flowbio-cli/contracts/samples-annotation-template.md @@ -15,7 +15,7 @@ flowbio samples annotation-template [--sample-type TYPE] [-o PATH | --output PAT | Name | Required | Maps to | Notes | |------|----------|---------|-------| | `--sample-type TYPE` | no | `get_annotation_template(sample_type=TYPE)` | Defaults to `"generic"` (base columns common to all types). Unlike `batch-template`, this is optional. Not pre-validated; an unknown type surfaces the server's not-found rejection. | -| `-o`, `--output PATH` | conditional | file to write | Where the workbook is written. Required when stdout is an interactive terminal (the body is binary). | +| `-o`, `--output PATH` | yes | file to write | Where the workbook is written. Required: the body is a binary workbook and is never written to stdout. | ## Behaviour @@ -33,8 +33,7 @@ flowbio samples annotation-template [--sample-type TYPE] [-o PATH | --output PAT - **Human (no `--json`)**: the workbook bytes are written to `--output`; a short confirmation (path, sample type) goes to **stderr**. The binary is **never** - written to a terminal stdout — without `-o` and with a TTY stdout, the command - fails with exit `2` asking for an output path. + written to stdout, so `-o/--output` is required. - **`--json`**: a single document on stdout reporting where the file was written; **no spreadsheet bytes** on stdout: @@ -44,7 +43,7 @@ flowbio samples annotation-template [--sample-type TYPE] [-o PATH | --output PAT ## Exit codes -`0` success; `2` no `--output` given while stdout is an interactive terminal; +`0` success; `2` no `--output` given, or an output path that cannot be written; `4` unknown sample type (surfaced by the server); standard mapping otherwise. ## Acceptance mapping diff --git a/specs/001-flowbio-cli/tasks.md b/specs/001-flowbio-cli/tasks.md index 0161cab..ced85a9 100644 --- a/specs/001-flowbio-cli/tasks.md +++ b/specs/001-flowbio-cli/tasks.md @@ -118,7 +118,7 @@ Single-project layout (per plan.md): CLI under `flowbio/cli/` (every module `_`- ### Tests for User Story 5 (MANDATORY — write first) ⚠️ -- [X] T020 [P] [US5] Write failing tests in `tests/unit/cli/test_samples.py` covering `annotation-template` (US5 scenarios 1–3): writes the server-generated `.xlsx` bytes verbatim to `-o/--output` with a confirmation (path, sample type) on stderr (exit 0); `--sample-type` optional, defaults to `"generic"`; no `-o` with a TTY stdout → exit 2 asking for an output path; `--json` emits a single `{"output", "sample_type"}` document on stdout with no spreadsheet bytes there; unknown `--sample-type` (server not-found) → exit 4 (contracts/samples-annotation-template.md) +- [X] T020 [P] [US5] Write failing tests in `tests/unit/cli/test_samples.py` covering `annotation-template` (US5 scenarios 1–3): writes the server-generated `.xlsx` bytes verbatim to `-o/--output` with a confirmation (path, sample type) on stderr (exit 0); `--sample-type` optional, defaults to `"generic"`; `-o/--output` required → exit 2 when omitted or unwritable; `--json` emits a single `{"output", "sample_type"}` document on stdout with no spreadsheet bytes there; unknown `--sample-type` (server not-found) → exit 4 (contracts/samples-annotation-template.md) - [X] T022 [P] [US5] Write failing tests in `tests/unit/cli/test_samples.py` covering `upload-multiplexed` (US5 scenarios 4–7): submit + report `data_ids`/`annotation_id`/`warnings` (exit 0); `--reads2` → paired-end; warnings reported but upload proceeds by default (`ignore_warnings=True`), `--reject-warnings` rejects → exit 5; annotation fails server validation → exit 5 (contracts/samples-upload-multiplexed.md) ### Implementation for User Story 5 diff --git a/tests/unit/cli/test_samples.py b/tests/unit/cli/test_samples.py index 4434b8b..2a21d82 100644 --- a/tests/unit/cli/test_samples.py +++ b/tests/unit/cli/test_samples.py @@ -1,4 +1,3 @@ -import io import json from http import HTTPStatus from pathlib import Path @@ -244,14 +243,7 @@ def test_sample_type_defaults_to_generic( assert route.called @respx.mock - def test_no_output_with_tty_stdout_is_usage_error( - self, run_cli, monkeypatch, - ) -> None: - class _TtyStringIO(io.StringIO): - def isatty(self) -> bool: - return True - - monkeypatch.setattr(io, "StringIO", _TtyStringIO) + def test_missing_output_is_usage_error(self, run_cli) -> None: route = _mock_annotation_template("generic") result = run_cli("--token", TOKEN, "samples", "annotation-template") @@ -260,15 +252,18 @@ def isatty(self) -> bool: assert route.call_count == 0 @respx.mock - def test_json_without_output_is_usage_error( - self, run_cli, + def test_unwritable_output_path_is_usage_error( + self, run_cli, tmp_path: Path, ) -> None: - route = _mock_annotation_template("generic") + _mock_annotation_template("generic") + output_path = tmp_path / "does-not-exist" / "sheet.xlsx" - result = run_cli("--token", TOKEN, "samples", "annotation-template", "--json") + result = run_cli( + "--token", TOKEN, "samples", "annotation-template", + "-o", str(output_path), + ) assert result.exit_code == 2 - assert route.call_count == 0 @respx.mock def test_json_emits_single_document_without_bytes( @@ -391,6 +386,32 @@ def test_warnings_reported_but_upload_proceeds_by_default( assert result.exit_code == 0 assert json.loads(result.stdout)["warnings"] == warnings + @respx.mock + def test_warnings_rendered_readably_in_human_mode( + self, run_cli, tmp_path: Path, + ) -> None: + message = "Unknown barcode" + annotation = respx.post(ANNOTATION_UPLOAD_URL) + annotation.side_effect = [ + httpx.Response( + HTTPStatus.BAD_REQUEST, + json={"warnings": [{"row": 1, "message": message}]}, + ), + httpx.Response(HTTPStatus.OK, json={"id": "ann_1"}), + ] + _mock_multiplexed() + + result = run_cli( + "--token", TOKEN, "samples", "upload-multiplexed", + "--reads1", str(_reads(tmp_path, "r1.fq.gz")), + "--annotation", str(_annotation(tmp_path)), + "--no-progress", + ) + + assert result.exit_code == 0 + assert f"row 1: {message}" in result.stderr + assert "{'row'" not in result.stderr + @respx.mock def test_reject_warnings_rejects_upload( self, run_cli, tmp_path: Path,