Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/adding-a-write-surface.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ For message/resource mutations, accept exactly one source:
- `--stdin` for newline-delimited IDs.
- `--query` resolved through the domain client.

Read commands that feed pipelines expose `--ids`. Reuse `internal/bulk.ResolveIDs` and `bulk.Result` instead of implementing input arbitration or result text again.
Read commands that feed pipelines expose `--ids`. Reuse `internal/bulk.ResolveIDs` and `bulk.Result` instead of implementing input arbitration or result text again; `TestBulkLeavesRouteThroughResolver` enforces resolver use.

## 5. Add scopes and compose `grw`

Expand Down
4 changes: 3 additions & 1 deletion docs/golden-principles.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ Every leaf added by a write command declares boolean `--dry-run` with shorthand

Confirmation is reserved for unrecoverable data loss. Actions that are irreversible but not destructive, such as `grw mail send`, take an explicit ID, print what will happen, and proceed; `--dry-run` is the review step, and a prompt would only break the piping and agent flows grw exists for.

Enforced by `TestWriteLeavesHaveDryRun` and `TestPermanentWriteLeavesRequireYes`. `TestDelete_DryRunDoesNotMutate` and `TestDelete_PermanentWithYes` verify behavior.
A leaf offering `--stdin` or `--query` resolves IDs through `internal/bulk`.

Enforced by `TestWriteLeavesHaveDryRun`, `TestPermanentWriteLeavesRequireYes`, and `TestBulkLeavesRouteThroughResolver`. `TestDelete_DryRunDoesNotMutate` and `TestDelete_PermanentWithYes` verify behavior.

## 5. Resource leaves are text-only

Expand Down
150 changes: 150 additions & 0 deletions internal/architecture/bulk_source_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
package architecture

import (
"go/ast"
"os"
"strconv"
"strings"
"testing"
)

// minDeclaringFiles is a sanity floor: the repo has well over ten files that
// take bulk IDs, so a detector that matches fewer has stopped seeing them.
const minDeclaringFiles = 10

func TestBulkLeavesRouteThroughResolver(t *testing.T) {
t.Parallel()
declaringFiles := 0
for _, pkg := range commandPackages(t) {
files := parseNonTestFiles(t, pkg.dir)
names := nonTestFileNames(t, pkg.dir)
resolvers := resolverFunctions(files)
for i, file := range files {
flags := bulkSourceFlags(file)
if len(flags) == 0 {
continue
}
declaringFiles++
for flag := range flags {
if !callsResolver(file, resolvers) {
t.Errorf("internal/%s/%s declares --%s but does not route through bulk.ResolveIDs", pkg.name, names[i], flag)
}
}
}
}
if declaringFiles < minDeclaringFiles {
t.Fatalf("found %d files declaring bulk ID sources; want at least %d", declaringFiles, minDeclaringFiles)
}
}

func nonTestFileNames(t *testing.T, dir string) []string {
t.Helper()
entries, err := os.ReadDir(dir)
if err != nil {
t.Fatalf("read %s: %v", dir, err)
}
var names []string
for _, entry := range entries {
name := entry.Name()
if strings.HasSuffix(name, ".go") && !strings.HasSuffix(name, "_test.go") {
names = append(names, name)
}
}
return names
}

func bulkSourceFlags(file *ast.File) map[string]bool {
flags := map[string]bool{}
ast.Inspect(file, func(node ast.Node) bool {
call, ok := node.(*ast.CallExpr)
if !ok || len(call.Args) < 3 {
return true
}
sel, ok := call.Fun.(*ast.SelectorExpr)
if !ok {
return true
}
flagsCall, ok := sel.X.(*ast.CallExpr)
if !ok || !isSelector(flagsCall.Fun, "", "Flags") {
return true
}
name, nameOK := stringLiteral(call.Args[1])
help, helpOK := stringLiteral(call.Args[len(call.Args)-1])
if !nameOK || !helpOK {
return true
}
// A bulk ID source is a --stdin or --query flag whose help says it
// carries IDs or resource names. The same flag names also appear on
// mail draft (body from stdin) and mail filter (filter criteria),
// which are not ID sources.
isBulkFlagName := name == "stdin" || name == "query"
helpMentionsIDs := strings.Contains(help, "ID") || strings.Contains(help, "resource name")
if isBulkFlagName && helpMentionsIDs {
flags[name] = true
}
return true
})
return flags
}

// resolverFunctions returns the package's functions that reach
// bulk.ResolveIDs. It iterates to a fixed point because a leaf may call a
// helper (such as the drive package's resolveFileIDs) that calls another
// helper before the resolver; a single pass would only see direct callers.
func resolverFunctions(files []*ast.File) map[string]bool {
resolvers := map[string]bool{}
for changed := true; changed; {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

changed = false
for _, file := range files {
for _, decl := range file.Decls {
fn, ok := decl.(*ast.FuncDecl)
if !ok || fn.Body == nil || resolvers[fn.Name.Name] {
continue
}
if callsResolver(fn.Body, resolvers) {
resolvers[fn.Name.Name] = true
changed = true
}
}
}
}
return resolvers
}

func callsResolver(node ast.Node, resolvers map[string]bool) bool {
found := false
ast.Inspect(node, func(node ast.Node) bool {
call, ok := node.(*ast.CallExpr)
if !ok {
return true
}
if isSelector(call.Fun, "bulk", "ResolveIDs") {
found = true
} else if ident, ok := call.Fun.(*ast.Ident); ok && resolvers[ident.Name] {
found = true
}
return !found
})
return found
}

func isSelector(expr ast.Expr, receiver, name string) bool {
sel, ok := expr.(*ast.SelectorExpr)
if !ok || sel.Sel.Name != name {
return false
}
if receiver == "" {
return true
}
ident, ok := sel.X.(*ast.Ident)
return ok && ident.Name == receiver
}

func stringLiteral(expr ast.Expr) (string, bool) {
literal, ok := expr.(*ast.BasicLit)
if !ok {
return "", false
}
value, err := strconv.Unquote(literal.Value)
return value, err == nil
}
Loading