gro: remove per-command --json (BREAKING) [#144] by rianjs · Pull Request #147 · open-cli-collective/google-readonly

rianjs · 2026-05-28T15:49:06Z

Summary

Remove per-command `--json/-j` from every resource-surface leaf in gro (mail, calendar, contacts, drive, me) per cli-common `docs/output-and-rendering.md` §2 (JSON reserved for round-trip payloads + control-plane envelopes only). After this PR, attempting any `gro --json` returns cobra's `unknown flag: --json`.

Carve-outs (preserved unchanged):

`gro refresh --json` / `gro refresh --status --json` (control-plane envelope; cli-common §4.6, added in feat(cache): add top-level refresh subcommand #146)
`gro config show --json` (diagnostic / config introspection; installer-script precondition surface)

`gro set-credential --json` does not exist today; if added later it would qualify as a §1.5.2 carve-out, but this PR neither adds nor presupposes that surface.

Boundary

"No per-command `--json` anywhere on resource surfaces, including mutation-success renderers." mail `archive/label/markread/star/categorize/draft`, calendar `color/rsvp`, contacts `group_manage/star`, drive `star` are swept up by the same policy as the resource reads — mirrors the cfl#392 boundary.

Architectural shift

Golden principle #4 is inverted:

Pre-Remove per-command --json flags (cli-common output-and-rendering §2) #144: every leaf declared `--json/-j` (enforced by `TestAllLeafCommandsHaveJSONFlag`, with two binary-write exemptions).
Post-Remove per-command --json flags (cli-common output-and-rendering §2) #144: every resource leaf must NOT declare `--json` (enforced by `TestResourceLeavesHaveNoJSONFlag`). The `jsonExemptCommands` allowlist is gone.

`docs/golden-principles.md` §4 rewritten to match.

Shared helper cleanup

`internal/bulk/result.go`: `Result.Print(jsonOutput bool)` → `Print()` (text-only); callers in mail/drive batch commands updated.
`internal/cmd/{mail,calendar,contacts,drive}/output.go`: deleted the four package-local `printJSON` helpers (orphaned after branch removal).
`internal/cmd/me/output.go`: deleted `RenderJSON` plus the three JSON-shape structs (`jsonOneLiner`, `jsonIDOnly`, `jsonExtended`).
`internal/output/` package unchanged — still consumed by `refresh` and `config show`.

Test invariants

`gro mail search "test" --json` → `unknown flag: --json` (cobra)
`gro refresh --json` → still emits the §4.6 envelope
`gro config show --json` → still emits the diagnostic record
`make build && make test && make lint` clean
`TestResourceLeavesHaveNoJSONFlag` passes
`internal/noleak` me variants dropped; `config show --json` no-leak coverage retained

`TestResolveDriveScope` continues to fail on 2 subtests in some local environments — confirmed pre-existing on `main` via `git stash + test`, environment-dependent, unrelated to this PR.

Stats

74 files changed, 147 insertions(+), 1954 deletions(-). Net deletion: the removal absorbs more code than it adds.

Closes #144

JSON is reserved for round-trip payloads and control-plane envelopes per cli-common docs/output-and-rendering.md §2. Today's surviving JSON surfaces are 'gro refresh --json' (§4.6, added in #146) and 'gro config show --json' (diagnostic). Every resource-surface leaf under mail/, calendar/, contacts/, drive/, me/ now emits text only; running 'gro mail search foo --json' returns cobra's 'unknown flag: --json'. The boundary is uniform across resource reads AND mutation-success renderers — mail archive/label/markread/star/categorize/draft, calendar color/rsvp, contacts group_manage/star, drive star all lose their --json flag along with the read commands. Architecture: inverts golden principle #4 + the structural test that enforced it. Pre-#144: every leaf had to declare --json/-j (with two binary-write exemptions). Post-#144: every resource leaf must NOT declare --json — caught by TestResourceLeavesHaveNoJSONFlag. The jsonExemptCommands allowlist is gone (empty after the inversion). Helper cleanup: bulk.Result.Print(jsonOutput bool) → Print() (text only); the four package-local printJSON helpers in mail/calendar/contacts/drive output.go are deleted; me/output.go's RenderJSON + json-shaped structs are deleted. shared internal/output helpers stay (refresh + config show consume them). Tests: per-package 'has json flag' subtests, every Test*_JSON function, and noleak's me --json variants are removed; config show --json no-leak coverage stays. BREAKING CHANGE: 'gro <resource> --json' now errors. Scripts that piped 'gro ... --json | jq' must move to text parsing or use one of the surviving JSON surfaces (gro refresh --json, gro config show --json). Closes #144

rianjs · 2026-05-28T15:50:42Z

Findings

Major: docs/adding-a-domain.md still documents the pre-Remove per-command --json flags (cli-common output-and-rendering §2) #144 rule. It says output.go must contain printJSON() and every leaf command must have --json/-j, with binary download exemptions. That directly contradicts the new architecture test and will steer future domains back toward the removed surface. Update that section to require text-only resource leaves and no package-local printJSON() helper, with JSON reserved for control-plane commands outside domain resource packages.

Everything else I checked lines up with the intended boundary: no residual resource command --json flags/branches showed up in internal/cmd, refresh --json and config show --json remain, and the helper cleanup removed the dead per-domain JSON paths.

Verification run: GOCACHE=/private/tmp/gro144-gocache CLANG_MODULE_CACHE_PATH=/private/tmp/gro144-clang-cache go test ./internal/architecture ./internal/cmd/refreshcmd ./internal/cmd/config ./internal/noleak passed.

The pre-#144 instructions required output.go to contain a printJSON() helper and every leaf to declare --json/-j. Both are inverted by the new policy: resource-surface leaves emit text only; JSON is reserved for control-plane envelopes outside the domain resource packages (refresh, config show). Updated the section so a future domain author follows the §4 rule rather than the removed surface.

rianjs · 2026-05-28T15:52:36Z

Findings

Minor: docs/architecture.md:67 still says domain output.go contains printJSON(). That now contradicts the Remove per-command --json flags (cli-common output-and-rendering §2) #144 architecture and could guide future domain work to recreate the helper. Update it to say client interface, ClientFactory, and text formatters only.
Nit: internal/output/output.go:19 still references “existing per-domain printJSON() callers”. Those callers are intentionally gone; reword this around control-plane JSONStdout callers or remove the sentence.

Verified targeted tests pass: internal/architecture, internal/cmd/refreshcmd, internal/cmd/config, internal/noleak.

Two doc tails referenced the removed printJSON() helper: - docs/architecture.md file-naming table - internal/output/output.go's JSON() docstring referring to 'per-domain printJSON() callers' (those are gone; the only surviving JSONStdout callers are control-plane refresh + config show).

rianjs · 2026-05-28T15:55:07Z

TDD Coverage Assessment

Coverage of new behavior: The PR's core invariant (resource-surface leaves declare no --json flag) is genuinely enforced by TestResourceLeavesHaveNoJSONFlag at internal/architecture/architecture_test.go:238 — it walks cobra.Command.Flags().Lookup("json") on every leaf reachable from the five domain root commands, which is the right structural assertion. The two carve-outs (refresh --json, config show --json) are kept out of domainCommands() by construction, so the guard cannot false-positive on them. refresh --json is well-covered behaviorally by TestRefresh_Status_JSONEnvelope and TestRefresh_JSONEnvelope_Success (internal/cmd/refreshcmd/refresh_test.go:103,132), which unmarshal the envelope and assert the resources[] shape. bulk.Result.Print()'s signature change is covered by the two updated tests in internal/bulk/result_test.go:9,18 — adequate since Print() is now text-only and a 10-line function. The 1954-deletion-vs-147-addition ratio is consistent with a removal-shaped change where the absence is what needs guarding, and the inverted structural test does that job.

Gaps:

internal/architecture/architecture_test.go:23-37 — domainPackages and domainCommands() are a hardcoded closed set of five packages. A future resource leaf registered under a sixth top-level command (e.g. gro tasks <leaf>) would silently slip past TestResourceLeavesHaveNoJSONFlag. Add a guard that asserts the rootCmd's subtree (minus an explicit {refresh, config, init, set-credential, version, help} carve-out list) has zero --json flags, so adding a new domain without updating domainCommands() fails closed.
No file — there is no end-to-end cobra-level assertion that gro mail search "x" --json actually returns Error: unknown flag: --json. The PR body lists this as a test invariant but only the structural guard tests it. Add a table-driven test (e.g. in internal/cmd/root/root_test.go or a new internal/architecture/json_rejection_test.go) that calls rootCmd.SetArgs([]string{"mail", "search", "x", "--json"}), runs Execute(), and asserts the returned error contains unknown flag. This catches the user-observable contract directly, not just the structural precondition.
internal/cmd/config/config_test.go:34-56 — TestConfigShowCommand does not exercise --json at all. The only config show --json coverage is TestConfigShowNeverLeaks at internal/noleak/noleak_test.go:79, which only asserts secrets are absent, not that the JSON shape (fields, structure) is preserved. A regression that broke the config show --json envelope's field set or made it invalid JSON would not be caught. Add TestConfigShow_JSONEnvelope that runs show --json and json.Unmarshals the output into the expected struct, mirroring TestRefresh_Status_JSONEnvelope.

Redundancy: None observed. The deleted Test*_JSON functions exercised JSON helpers (printJSON, me.RenderJSON, the jsonOneLiner/jsonIDOnly/jsonExtended structs) whose production code was deleted in the same PR — they tested the shape of code that no longer exists, so removing them is correct, not a coverage loss. The me-package TestMeNeverLeaks at internal/noleak/noleak_test.go:124 correctly dropped its --json / --id --json variants because those flag combos no longer parse; the surviving {} and {"--extended"} cases still cover the smoke path.

Risk level: Low

…-out Two TDD-suggested coverage adds: - TestResourceLeaf_RejectsJSON_EndToEnd dispatches 'gro drive drives --json' through cobra and asserts the user-visible 'unknown flag' error. The static guard catches missing flag declarations; this one catches a hypothetical regression where the flag is declared but the structural test's domain set is bypassed. - TestConfigShowCommand: assert the --json control-plane flag is declared with -j shorthand. config show --json had no-leak coverage in internal/noleak but no shape coverage; this pins the carve-out surface itself.

monit-reviewer

Automated PR Review

Reviewed commit: 1f767c8

Summary

Reviewer	Findings
harness-engineering:harness-enforcement-reviewer	3
harness-engineering:harness-knowledge-reviewer	1
harness-engineering:harness-self-documenting-code-reviewer	1

harness-engineering:harness-enforcement-reviewer (3 findings)

💡 Suggestion - internal/output/output.go:19

Comment still references 'existing per-domain printJSON() callers', which were intentionally removed by this PR. Reword to reference the actual current callers (refresh --json, config show --json) or remove the sentence to avoid misleading future readers.

💡 Suggestion - internal/architecture/architecture_test.go:263

TestResourceLeaf_RejectsJSON_EndToEnd only tests 'drive drives' and its comment overstates the protection — if drive were dropped from domainCommands(), both tests would miss it simultaneously. Reword the comment to say 'spot-check complement to the structural walk' rather than implying it guards against domain-set bypass.

💡 Suggestion - docs/golden-principles.md:29

The new §4 text names the two carve-outs (refresh --json, config show --json) but does not articulate the criteria that qualify a command as a control-plane carve-out. One sentence on the qualifying criterion ('control-plane envelope or diagnostic introspection, outside domain resource packages') would prevent ad-hoc re-introduction by future contributors.

harness-engineering:harness-knowledge-reviewer (1 findings)

⚠️ Should Fix - CLAUDE.md:40

The Key Constraints section still lists '--json on all leaf commands: Every leaf subcommand supports --json/-j.' This directly contradicts the post-#144 inversion enforced by TestResourceLeavesHaveNoJSONFlag. AI agents and contributors using CLAUDE.md as guidance will attempt to re-add --json flags to new leaf commands, breaking the architecture test. Update to reflect that resource-surface leaves must NOT declare --json; JSON is reserved for control-plane envelopes (refresh --json, config show --json).

harness-engineering:harness-self-documenting-code-reviewer (1 findings)

💡 Suggestion - internal/cmd/drive/drives_test.go:27

Only drives_test.go and archive_test.go received the inverted 'no json flag' assertion form. All other per-command test files simply delete the old assertion without adding the inverted form. Either all domain command tests should carry the inverted assertion or none should — the current split makes per-command signal inconsistent, even though TestResourceLeavesHaveNoJSONFlag ensures global correctness.

6 info-level observations excluded. Run with --verbose to include.

3 PR discussion threads considered.

Completed in 3m 40s | $1.91 | sonnet | daemon 0.2.121 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, documentation:docs-reviewer, 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	3m 40s wall · 13m 00s compute (Reviewers: 3m 07s · Synthesis: 30s)
Cost	$1.91
Tokens	677.8k in / 48.2k out
Turns	14

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	32.4k	1.9k	0	32.4k (1h)	$0.16
documentation:docs-reviewer	haiku	11.6k	4.9k	0	11.6k (1h)	$0.05
harness-engineering:harness-architecture-reviewer	sonnet	207.1k	10.1k	158.6k	48.5k (1h)	$0.42
harness-engineering:harness-enforcement-reviewer	sonnet	108.9k	9.0k	54.4k	54.5k (1h)	$0.40
harness-engineering:harness-knowledge-reviewer	sonnet	50.4k	7.7k	4.0k	46.3k (1h)	$0.33
harness-engineering:harness-self-documenting-code-reviewer	sonnet	216.5k	8.8k	162.4k	54.1k (1h)	$0.42
security:security-code-auditor	haiku	50.9k	5.7k	0	50.9k (1h)	$0.13

_{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.}

Note: Posted 4 inline comments. 1 finding remained in the summary because GitHub could not attach them to the current diff.

Five daemon findings on PR #147: 1. The repo's agent-guidance markdown §Key Constraints still required '--json on all leaf commands' — directly contradicts the post-#144 inversion and would mislead future contributors. Updated to mirror the new text-only rule. 2. internal/architecture E2E test comment overstated protection (claimed to catch a domain-set bypass when it shares the same list). Reworded as a spot-check complement that explicitly notes neither test guards against domainPackages drift. 3. docs/golden-principles.md §4 grew a 'carve-out criteria' paragraph articulating what qualifies a command as a control-plane carve-out (outside domain resource packages + envelope/diagnostic). 4. Removed the two duplicate per-command 'no json flag' subtests (drives_test.go, archive_test.go) for consistency — global guard in TestResourceLeavesHaveNoJSONFlag covers all leaves uniformly.

Superseded by updated review

monit-reviewer

Automated PR Review

Reviewed commit: 2a8772e | Previous: 1f767c8 (incremental)

Summary

No issues found.

7 PR discussion threads considered.

Completed in 1m 22s | $1.76 | sonnet | daemon 0.2.121 | Glorfindel

Field	Value
Model	sonnet
Mode	Re-review · Cycle 2 · Session resumed
Reviewers	hybrid-synthesis, documentation:docs-reviewer, 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 22s wall · 1m 51s compute (Reviewers: 31s · Synthesis: 30s)
Cost	$1.76
Tokens	571.2k in / 8.2k out
Turns	7

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	39.5k	1.6k	13.2k	26.2k (1h)	$0.13
documentation:docs-reviewer	haiku	24.1k	4.5k	0	24.1k (1h)	$0.05
harness-engineering:harness-architecture-reviewer	sonnet	99.1k	103	4.0k	95.0k (1h)	$0.36
harness-engineering:harness-enforcement-reviewer	sonnet	97.9k	125	4.0k	93.9k (1h)	$0.36
harness-engineering:harness-knowledge-reviewer	sonnet	96.5k	82	4.0k	92.4k (1h)	$0.35
harness-engineering:harness-self-documenting-code-reviewer	sonnet	97.7k	144	4.0k	93.7k (1h)	$0.35
security:security-code-auditor	haiku	95.1k	1.2k	0	95.1k (1h)	$0.12
discussion-summarizer	—	21.4k	395	13.2k	8.1k (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.}

rianjs · 2026-05-28T16:05:05Z

No findings.

The daemon edits look aligned with #144: stale docs/comments were cleaned up, config show --json is explicitly protected as a carve-out, and the new end-to-end resource-leaf check is a narrow complement to the structural walk. I don’t see the carve-out criteria widening resource JSON back in.

Verified with targeted search and tests:
go test ./internal/architecture ./internal/cmd/config ./internal/cmd/refreshcmd ./internal/output ./internal/noleak

monit-reviewer reviewed May 28, 2026

View reviewed changes

Comment thread internal/output/output.go

monit-reviewer reviewed May 28, 2026

View reviewed changes

Comment thread internal/architecture/architecture_test.go

monit-reviewer reviewed May 28, 2026

View reviewed changes

Comment thread docs/golden-principles.md

monit-reviewer reviewed May 28, 2026

View reviewed changes

Comment thread internal/cmd/drive/drives_test.go

monit-reviewer previously requested changes May 28, 2026

View reviewed changes

monit-reviewer approved these changes May 28, 2026

View reviewed changes

rianjs merged commit 520596a into main May 28, 2026
2 checks passed

rianjs deleted the feat/144-remove-resource-json branch May 28, 2026 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

gro: remove per-command --json (BREAKING) [#144] - #147

gro: remove per-command --json (BREAKING) [#144]#147
rianjs merged 5 commits into
mainfrom
feat/144-remove-resource-json

rianjs commented May 28, 2026

Uh oh!

rianjs commented May 28, 2026

Uh oh!

rianjs commented May 28, 2026

Uh oh!

rianjs commented May 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer left a comment

Uh oh!

rianjs commented May 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rianjs commented May 28, 2026

Summary

Boundary

Architectural shift

Shared helper cleanup

Test invariants

Stats

Uh oh!

rianjs commented May 28, 2026

Uh oh!

rianjs commented May 28, 2026

Uh oh!

rianjs commented May 28, 2026

TDD Coverage Assessment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

rianjs commented May 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants