test: enforce a single ID source on bulk leaves by rianjs · Pull Request #23 · open-cli-collective/google-cli

rianjs · 2026-09-04T01:20:40Z

Summary

New architecture test TestBulkLeavesRouteThroughResolver: for every package under internal/cmd/* and internal/rwcmd/*, any file registering an ID-taking --stdin or --query flag must call bulk.ResolveIDs, directly or through a package-local helper (found by fixed-point over the package's functions, which covers resolveFileIDs in the drive write package). Flags whose help text does not mention IDs or resource names (mail draft --stdin body input, mail filter --query criteria) are not ID sources. A floor of ten declaring files keeps the detector honest.
Golden principle 4 and the write-surface guide name the test.

Closes #14

Test plan

make check; go test ./internal/architecture/ -run Bulk
Negative check: bypassing bulk.ResolveIDs in mail restore fails the test for both flags

Every command file that registers an ID-taking --stdin or --query flag must route through bulk.ResolveIDs, directly or via a package-local helper. Closes #14

monit-reviewer

Automated PR Review

Reviewed commit: 0a4915b

Approved with 3 non-blocking suggestions below. Address at your discretion.

Summary

Reviewer	Findings
harness-engineering:harness-self-documenting-code-reviewer	3

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

💡 Suggestion - internal/architecture/bulk_source_test.go:69

The condition nameOK && helpOK && (name == "stdin" || name == "query") && (strings.Contains(help, "ID") || strings.Contains(help, "resource name")) encodes the whole definition of what counts as a 'bulk ID source flag' in one line of operator soup. Extracting named intermediates (e.g. isBulkFlagName := name == "stdin" || name == "query" and helpMentionsIDs := strings.Contains(help, "ID") || strings.Contains(help, "resource name")) would let a reader see the two-part business rule (right flag name AND help text implies an ID/resource argument) without parsing the boolean expression.

💡 Suggestion - internal/architecture/bulk_source_test.go:79

resolverFunctions computes a fixed point over package functions (repeatedly scanning until no new resolver is found) but the loop carries no explanation of why it needs to iterate to a fixed point rather than a single pass. The PR description calls this out explicitly ('found by fixed-point over the package's functions, which covers resolveFileIDs'), which is exactly the kind of non-obvious rationale that belongs as a short comment on the loop rather than only in the PR body.

💡 Suggestion - internal/architecture/bulk_source_test.go:31

The threshold declaringFiles < 10 is a bare magic number. The PR description explains the rationale ('a floor of ten declaring files keeps the detector honest'), but that context isn't in the code. Consider a named constant (e.g. const minDeclaringFiles = 10) or a one-line comment so a future reader understands this is a sanity floor against the detector silently matching nothing, not an arbitrary count.

Completed in 1m 16s | $1.74 | sonnet | daemon 0.2.142 | 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	1m 16s wall · 1m 12s compute (Reviewers: 53s · Synthesis: 19s)
Cost	$1.74 (estimated)
Tokens	404.7k in / 12.6k out
Turns	14

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	56.9k	1.7k	40.6k	16.3k (1h)	$0.14
documentation:docs-reviewer	sonnet	52.8k	784	22.5k	30.3k (1h)	$0.20
harness-engineering:harness-architecture-reviewer	sonnet	58.0k	772	22.5k	35.5k (1h)	$0.23
harness-engineering:harness-enforcement-reviewer	sonnet	59.7k	1.9k	22.5k	37.2k (1h)	$0.26
harness-engineering:harness-knowledge-reviewer	sonnet	59.1k	1.4k	22.5k	36.6k (1h)	$0.25
harness-engineering:harness-self-documenting-code-reviewer	sonnet	62.2k	5.6k	22.5k	39.7k (1h)	$0.33
security:security-code-auditor	sonnet	55.9k	420	0	55.9k (1h)	$0.34

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

monit-reviewer · 2026-09-04T01:22:04Z

+		}
+		name, nameOK := stringLiteral(call.Args[1])
+		help, helpOK := stringLiteral(call.Args[len(call.Args)-1])
+		if nameOK && helpOK && (name == "stdin" || name == "query") && (strings.Contains(help, "ID") || strings.Contains(help, "resource name")) {


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): The condition nameOK && helpOK && (name == "stdin" || name == "query") && (strings.Contains(help, "ID") || strings.Contains(help, "resource name")) encodes the whole definition of what counts as a 'bulk ID source flag' in one line of operator soup. Extracting named intermediates (e.g. isBulkFlagName := name == "stdin" || name == "query" and helpMentionsIDs := strings.Contains(help, "ID") || strings.Contains(help, "resource name")) would let a reader see the two-part business rule (right flag name AND help text implies an ID/resource argument) without parsing the boolean expression.

Reply to this thread when addressed.

Addressed in 9998898: split into isBulkFlagName and helpMentionsIDs with a comment naming the two non-ID uses of those flag names.

monit-reviewer · 2026-09-04T01:22:04Z

+
+func resolverFunctions(files []*ast.File) map[string]bool {
+	resolvers := map[string]bool{}
+	for changed := true; changed; {


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): resolverFunctions computes a fixed point over package functions (repeatedly scanning until no new resolver is found) but the loop carries no explanation of why it needs to iterate to a fixed point rather than a single pass. The PR description calls this out explicitly ('found by fixed-point over the package's functions, which covers resolveFileIDs'), which is exactly the kind of non-obvious rationale that belongs as a short comment on the loop rather than only in the PR body.

Reply to this thread when addressed.

Addressed in 9998898: resolverFunctions has a doc comment explaining the fixed point.

monit-reviewer · 2026-09-04T01:22:04Z

+			}
+		}
+	}
+	if declaringFiles < 10 {


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): The threshold declaringFiles < 10 is a bare magic number. The PR description explains the rationale ('a floor of ten declaring files keeps the detector honest'), but that context isn't in the code. Consider a named constant (e.g. const minDeclaringFiles = 10) or a one-line comment so a future reader understands this is a sanity floor against the detector silently matching nothing, not an arbitrary count.

Reply to this thread when addressed.

Addressed in 9998898: minDeclaringFiles constant with a comment stating it is a sanity floor.

test: enforce a single ID source on bulk leaves

0a4915b

Every command file that registers an ID-taking --stdin or --query flag must route through bulk.ResolveIDs, directly or via a package-local helper. Closes #14

monit-reviewer approved these changes Sep 4, 2026

View reviewed changes

test: name the bulk-source detector's rules

9998898

rianjs merged commit b974eb9 into main Sep 4, 2026
11 checks passed

rianjs deleted the test/14-bulk-id-source branch September 4, 2026 01:25

Navigation Menu

Uh oh!

test: enforce a single ID source on bulk leaves - #23

test: enforce a single ID source on bulk leaves#23
rianjs merged 2 commits into
mainfrom
test/14-bulk-id-source

rianjs commented Sep 4, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Sep 4, 2026

Uh oh!

rianjs Sep 4, 2026

Uh oh!

monit-reviewer Sep 4, 2026

Uh oh!

rianjs Sep 4, 2026

Uh oh!

monit-reviewer Sep 4, 2026

Uh oh!

rianjs Sep 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rianjs commented Sep 4, 2026

Summary

Test plan

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants