feat(cli): add samples batch-template command

[mhusbynflow]

What

Implements User Story 3 of the flowbio CLI: flowbio samples batch-template --sample-type T.

• Human mode: a CSV header — reserved columns (name,reads1,reads2,project,organism), then one column per metadata attribute, each followed by a <id>__annotation companion when the attribute allows annotation. There is no sample_type column (the type is supplied via --sample-type). A required-vs-optional summary is written to stderr.
• --json mode: a per-column descriptor list (name, kind, required, options, description) on stdout and no CSV, so an agent can build rows directly.
• -o/--output PATH: writes the CSV template to a file.

The one library change is the additive MetadataAttribute.allow_annotation field, which mirrors the /samples/metadata payload key directly (defaults to False when absent, so it is backwards-compatible).

Verification

• Test-first: 7 new tests (tests/unit/v2/test_samples.py, tests/unit/cli/test_samples.py); full suite 230 passed.
• Smoke-tested live against https://app.flow.bio/api — confirmed the annotation companions resolve and strandedness derives as required for rna_seq.

Out of scope

User Story 5 (annotation-template / upload-multiplexed, tasks T020–T024) and User Story 4 (upload-batch) remain unimplemented; this PR is Phase 6 only.

🤖 Generated with Claude Code

[mhusbynflow]

@claude please review this PR

[claude]

Claude finished @mhusbynflow's task in 2m 3s —— View job

---

Review: samples batch-template (PR #14)

• Read changed files & gather context
• Review library change (MetadataAttribute.allow_annotation)
• Review CLI command (_samples.py)
• Review tests
• Check docs & spec consistency

Solid, focused PR. The allow_annotation addition is genuinely additive and backwards-compatible, the registration refactor is clean, and the test matrix covers the human/--json/--output permutations well. A few things worth addressing before merge — one substantive, the rest minor.

---

🟠 The --sample-type is never validated — but the docs say it is

_batch_template_command only calls client.samples.get_metadata_attributes() (_samples.py:163), and that endpoint (/samples/metadata) takes no sample type. The type is used purely to decide which columns are required:

required = (
    attribute.required or sample_type in attribute.required_for_sample_types
)

So a typo'd or unknown --sample-type produces a perfectly valid-looking template with nothing marked required-for-type, and exits 0. Meanwhile docs/cli.md states flatly:

Exit codes — 0 success; 2 missing --sample-type; 4 unknown sample type; ...

That 4 is unreachable here — NOT_FOUND (4) only comes from a NotFoundError on a transport call, and the one call made doesn't depend on the type. The contract spec hedges correctly (4 unknown sample type "(if surfaced by the lookup)"), but the user-facing doc drops the hedge and overstates the guarantee.

I'd either soften the docs line (e.g. note the type isn't validated until upload-batch) or, if early validation is desired, look it up against client.samples.get_types() and raise NotFoundError/CliUsageError on a miss. The silent acceptance is the kind of thing that bites an agent building a sheet for a mistyped type. Fix this →

---

🟡 CSV header isn't quoted; column-name collisions aren't guarded

The header is ",".join(column.name ...) (_samples.py:142). Two edge cases:

• A metadata identifier containing a comma or quote would corrupt the header. Identifiers are presumably slugs, so low risk — but csv.writer would make this robust for free.
• A metadata attribute whose identifier collides with a reserved name (name, reads1, …), with sample_type, or with a generated <id>__annotation would silently produce a duplicate/confusing column. The docs promise "no sample_type column", but an attribute literally named sample_type would defeat that.

Both are unlikely given how identifiers are minted server-side; worth at most a defensive note rather than a blocker.

🟡 --output is a bare str (repo convention prefers Path)

CLAUDE.md says to prefer pathlib.Path over bare str for filesystem paths. The option is registered without a type, so args.output is a str and you reconstruct Path(args.output) at use (_samples.py:144). Adding type=Path to the argument would align with the convention and drop the reconstruction.

🔵 Confirm intent: --json --output FILE writes the file and prints JSON

Given if output.json_mode or args.output is None:, the combination --json -o file.csv writes the CSV to the file and emits the JSON descriptor list to stdout. That seems reasonable (json mode always emits its document), but it's the one branch not exercised by a test — worth a one-line confirmation or a test if it's a deliberate contract.

---

Nits / positives

• Nice reuse of emit_result(human_line=header, document=descriptors) to drive both modes from one call.
• allow_annotation has both a "populated from payload" and a "defaults to false when absent" test — good backwards-compat coverage.
• The spec/data-model allows_annotation → allow_annotation rename is consistent across all the touched docs. 👍

Nothing here is a hard blocker; the docs/exit-code mismatch is the one I'd want resolved (even just by softening the prose). I didn't run the suite locally — the PR reports 230 passed and the new tests look internally consistent with the implementation.
· branch mhusbynflow/samples-comment-review

[mhusbynflow]

@claude please review this PR

[claude]

Claude finished @mhusbynflow's task in 2m 16s —— View job

---

Review: samples batch-template (PR #14)

• Read changed files & gather context
• Review library change (MetadataAttribute.allow_annotation)
• Review CLI command (_samples.py)
• Review tests
• Check docs & spec consistency

This is a re-review against the current branch. Every substantive point from the earlier review has been addressed — nice follow-through. The PR is in good shape; I'd merge it with at most one optional defensive tweak.

---

✅ Resolved since the last review

• The --sample-type / exit-code mismatch (was 🟠). docs/cli.md no longer overstates the guarantee — the exit-code table drops the 4 unknown sample type line, and the prose now reads plainly: "The sample type is not checked against the server here, so an unknown type still exits 0; the type is validated when you run samples upload-batch." (docs/cli.md:259-262, _samples.py:234-237). The doc now matches the implementation. 👍
• --output is a Path (was 🟡). Both batch-template and the US5 commands register type=Path (_samples.py:178-183), and _data.py got the same treatment — the Path(args.path) reconstruction is gone. Consistent with the CLAUDE.md convention.
• Unwritable output path (new guard). args.output.write_text(...) is wrapped and re-raised as CliUsageError (_samples.py:317-323), with a test asserting exit 2 and no traceback leak (test_unwritable_output_path_is_usage_error).
• --json --output FILE contract (was 🔵). Now exercised by test_json_with_output_writes_csv_file_and_emits_descriptors — confirms the file is written and the descriptor list still goes to stdout. The previously-untested branch is now pinned.

---

🟡 Remaining (optional, non-blocking): header still uses ",".join(...)

The CSV header is built with ",".join(column.name for column in columns) (_samples.py:316). Two theoretical edge cases survive:

• A metadata identifier containing a comma/quote would corrupt the header (csv.writer would quote it for free).
• An identifier colliding with a reserved name (name, reads1, …) or with a generated <id>__annotation would silently emit a duplicate column.

Both depend on server-minted identifiers being well-behaved slugs, so the real-world risk is low — this is a defensive nicety, not a blocker. I'd leave it as-is unless you want belt-and-braces robustness. Harden this →

---

Positives

• allow_annotation is genuinely additive — default=False plus the dedicated "populated from payload" / "defaults to false when absent" tests give clean backwards-compat coverage (test_samples.py:204-250).
• The allows_annotation → allow_annotation rename is consistent across the contract, data-model, and code.
• _TemplateColumn + descriptor cleanly drives both human (header) and --json (descriptors) modes from one emit_result, and the test matrix covers header order, the annotation companion, the required summary, -o, --json, --json -o, the unwritable path, and missing --sample-type.

Nothing here blocks merge. I read all nine changed files but did not run the suite locally (the pytest invocation needs approval in this environment); the PR reports 230 passed and the new tests are internally consistent with the implementation.

· branch mhusbynflow/samples-comment-review
·

[mhusbynflow]

See comments

[mhusbynflow]

Instead of casting args.sample_type, could we not instead add SampleTypeId as the type to add_argument?

[mhusbynflow]

Done — set type=SampleTypeId on the three --sample-type arguments (upload, annotation-template, batch-template) and dropped the SampleTypeId(args.sample_type) casts from the handlers, mirroring the type=Path treatment of path options. argparse applies the type to the annotation-template 'generic' string default too, so it stays consistently typed. Behaviour-preserving; 251 tests pass.