feat(cfl): support --file - to read page body from stdin#364
Conversation
page create and page edit already read piped stdin via the os.Stdin char-device fallthrough, but the canonical --file - idiom failed: the fail-fast os.Stat guard and the os.ReadFile path did not special-case "-". Special-case "-" in both spots for both commands, reading through root.Options.Stdin so the path is unit-testable. Markdown/storage/ADF format selection is unchanged and still composes with -. [INT-424] Closes #363
FindingsNo findings. The PR adheres to the INT-424 boundary and the broader MCP goal:
I also ran the focused package check locally from go test -race ./internal/cmd/page/...Result: 140 tests passed. Codex architect review (batch session, ticket 1/3 — INT-424) |
|
TDD coverage assessment (automated) TDD assessment: adequate — one minor gap worth noting All 140 tests pass ( What's covered well
One actionable gap
Not flagging
|
Mirrors TestRunEdit_FileDash_EmptyStdin so create and edit cover the empty-content guard on the - path identically. [INT-424]
|
Addressed the TDD symmetry gap in 6ae45a2: added |
FindingsNo findings. The added The PR still matches INT-424 and the broader MCP dependency:
Codex architect re-review after TDD fix (ticket 1/3 — INT-424) — converged |
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 6ae45a2
Approved with 6 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 2 |
| harness-engineering:harness-enforcement-reviewer | 1 |
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 2 |
harness-engineering:harness-architecture-reviewer (2 findings)
💡 Suggestion - tools/cfl/internal/cmd/page/edit_test.go:1634
TestRunEdit_FileDash_EmptyStdin uses an inline httptest.Server instead of the mockEditBodyServer helper. If mockEditBodyServer's GET response changes, this test stays out of sync silently. Use mockEditBodyServer for consistency.
💡 Suggestion - tools/cfl/internal/cmd/page/page.go:12
stdinReader only gates the
--file -code path. The existing piped-stdin path in getContent/getEditContent (terminal.IsTerminal detection) still reads os.Stdin directly, not via stdinReader. This creates two parallel stdin abstractions in the same package: one testable, one not. A future test that inadvertently exercises the terminal path will silently block on real os.Stdin. Consider routing the terminal-detection path through the same helper, or documenting the split explicitly.
harness-engineering:harness-enforcement-reviewer (1 findings)
💡 Suggestion - tools/cfl/internal/cmd/page/create_test.go:823
No test covers whitespace-only content via
--file -. The existing TestRunCreate_WhitespaceOnlyFile covers whitespace-only file content, but the--file -stdin path has no equivalent. A caller pipingecho ' 'with--file -should hit the empty-content guard; it is not verified.
harness-engineering:harness-knowledge-reviewer (1 findings)
💡 Suggestion - tools/cfl/internal/cmd/page/create.go:233
useMarkdown("") is called with an empty string when reading from --file -. Passing "" relies on useMarkdown treating an empty string as defaulting to markdown. This implicit assumption is fragile — if useMarkdown ever special-cases "" differently, default behavior for --file - would silently change. Consider a named constant or dedicated boolean to make the intent explicit.
harness-engineering:harness-self-documenting-code-reviewer (2 findings)
💡 Suggestion - tools/cfl/internal/cmd/page/create_test.go:823
No test covers
--file - --legacyfor create. The PR description states--legacycomposes with-, but only--storageand--no-markdownare exercised. A misrouted useMarkdown("") result combined with--legacycould silently produce ADF instead of XHTML.
💡 Suggestion - tools/cfl/internal/cmd/page/edit_test.go:1539
No test covers
--file - --legacyfor edit, same gap as in create_test.go. The--legacyformat path is not exercised with stdin content.
4 PR discussion threads considered.
Completed in 1m 45s | $0.63 | sonnet | daemon 0.2.116 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 1m 45s wall · 5m 04s compute (Reviewers: 1m 14s · Synthesis: 29s) |
| Cost | $0.63 |
| Tokens | 114.6k in / 16.9k out |
| Turns | 6 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 31.6k | 1.8k | 0 | 31.6k (1h) | $0.15 |
| harness-engineering:harness-architecture-reviewer | sonnet | 16.6k | 3.2k | 2.1k | 14.4k (1h) | $0.11 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 16.6k | 2.9k | 2.1k | 14.4k (1h) | $0.11 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 16.6k | 2.8k | 2.1k | 14.4k (1h) | $0.10 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 16.6k | 3.8k | 2.1k | 14.4k (1h) | $0.12 |
| security:security-code-auditor | haiku | 16.8k | 2.4k | 0 | 16.8k (1h) | $0.04 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
Adds create+edit --file - --legacy tests (markdown stdin -> storage XHTML), verifying the --legacy composition the PR claims. Switches TestRunEdit_FileDash_EmptyStdin to the shared mockEditBodyServer helper for consistency. [INT-424]
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 01cb955 | Previous: 6ae45a2 (incremental)
Approved with 2 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-self-documenting-code-reviewer | 2 |
harness-engineering:harness-self-documenting-code-reviewer (2 findings)
💡 Suggestion - tools/cfl/internal/cmd/page/create.go:233
In production, if a user pipes content AND specifies
--file -, both the existing char-device stdin fallthrough (isCharDevice guard) and the new--file -block both read from os.Stdin. Whichever runs first consumes the stream; the second gets EOF and the empty-content guard fires. Tests don't cover this because injected opts.Stdin and the char-device check use different sources. The user-facing behavior is undefined for this combination; worth a guard or at minimum a comment.
💡 Suggestion - tools/cfl/internal/cmd/page/edit.go:288
Same double-read risk as create.go: getEditContent's existing stdin fallthrough and the new
--file -block both target os.Stdin in production. If a user pipes content and also passes--file -, the first reader consumes the stream and the second gets EOF. Same test blind spot applies.
10 PR discussion threads considered.
Completed in 2m 52s | $1.12 | sonnet | daemon 0.2.116 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Mode | Re-review · Cycle 2 · Session resumed |
| Reviewers | hybrid-synthesis, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 2m 52s wall · 7m 07s compute (Reviewers: 1m 52s · Synthesis: 29s) |
| Cost | $1.12 |
| Tokens | 188.1k in / 25.0k out |
| Turns | 6 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 38.6k | 1.8k | 13.6k | 25.0k (1h) | $0.12 |
| harness-engineering:harness-architecture-reviewer | sonnet | 17.1k | 3.6k | 2.1k | 15.0k (1h) | $0.12 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 17.1k | 5.1k | 2.1k | 15.0k (1h) | $0.14 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 17.1k | 3.7k | 2.1k | 15.0k (1h) | $0.12 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 17.2k | 4.6k | 2.1k | 15.0k (1h) | $0.13 |
| security:security-code-auditor | haiku | 17.4k | 5.2k | 0 | 17.4k (1h) | $0.06 |
| discussion-summarizer | — | 63.5k | 999 | 0 | 63.5k (1h) | $0.42 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
| return true | ||
| } | ||
|
|
||
| if opts.file == "-" { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): In production, if a user pipes content AND specifies --file -, both the existing char-device stdin fallthrough (isCharDevice guard) and the new --file - block both read from os.Stdin. Whichever runs first consumes the stream; the second gets EOF and the empty-content guard fires. Tests don't cover this because injected opts.Stdin and the char-device check use different sources. The user-facing behavior is undefined for this combination; worth a guard or at minimum a comment.
Reply to this thread when addressed.
There was a problem hiding this comment.
Declined — this misreads the control flow. The if opts.file == "-" block returns; the char-device stdin fallthrough is only reached when opts.file == "", which is false whenever - is passed. The two paths are mutually exclusive: exactly one read of stdin, no EOF/undefined case. A defensive guard/comment for an unreachable combination would be churn over self-evident early-return semantics.
| return true | ||
| } | ||
|
|
||
| if opts.file == "-" { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): Same double-read risk as create.go: getEditContent's existing stdin fallthrough and the new --file - block both target os.Stdin in production. If a user pipes content and also passes --file -, the first reader consumes the stream and the second gets EOF. Same test blind spot applies.
Reply to this thread when addressed.
There was a problem hiding this comment.
Same as the create.go thread: the opts.file == "-" branch returns before the implicit-stdin fallthrough (which requires opts.file == ""). Mutually exclusive, single read. Declined.
|
These are low-value (both misread the early-return control flow); please approve the PR. |
Summary
cfl page createandcfl page editalready read piped stdin (theos.Stdinchar-device fallthrough ingetContent/getEditContent), but the canonical--file -idiom failed: the fail-fastos.Stat(opts.file)guard and theos.ReadFile(opts.file)path both ENOENT on-.This special-cases
-in those two spots for both commands, reading throughroot.Options.Stdin(a tiny sharedstdinReaderhelper) so the path is unit-testable. Markdown / storage / ADF selection (--no-markdown/--storage/--legacy) is unchanged and composes with-.Enables callers that can't share a filesystem with the cfl subprocess (e.g. a remote MCP wrapper) to stream a body via stdin:
Changes
page.go: 4-linestdinReader(*root.Options)resolver (Options.Stdin in tests, os.Stdin in prod).create.go/edit.go: skipos.Statfor-; read stdin for-in the content path; help text + one example each.create_test.go/edit_test.go:--file -cases — markdown→ADF default,--storagepassthrough,--no-markdownraw-ADF passthrough (symmetric create/edit), edit empty-stdin guard.Scope
--body <string>intentionally out of scope (the pipe path already covers it; ticket marks it nice-to-have).internal/config/config_test.go:460(gosec false-positive on a test token) is not touched.Verification
From
tools/cfl/:make build✓ ·go test -race ./...→ 999 passed ✓ ·golangci-lint run ./internal/cmd/page/...→ clean ✓[INT-424]
Closes #363