Skip to content
Open
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
19 changes: 19 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,25 @@ make lint # Linter
make check # All checks (what bin/ci runs)
```

**Dead code**: `make deadcode` reports unreachable functions whole-program, rooted
at the shipped binary and again with `-tags dev`. It is a report, not a gate, and
is deliberately outside `make check`. The linter's `unused` cannot do this job —
it runs per-package and counts every exported identifier in a non-main package as
used, which is how 600 lines of exported, zero-caller `internal/tui` API survived
it. Root it at the binary, not `./...`: only main packages are roots, so `./...`
reports everything no main reaches — mostly the `dev`-tagged workspace tree — as
unreachable.

It also analyzes one GOOS/GOARCH at a time, while we release five. A host run says
nothing about the others: code behind another platform's build tag is never loaded,
and a function whose only caller sits behind one looks unreachable. Before deleting
anything platform-adjacent, check the other targets — install the tool for the host
and set GOOS for the analysis (`GOOS=windows deadcode ./cmd/basecamp`), since
`GOOS=windows go run` cross-compiles the tool itself and fails.

Read the output before acting on it: a zero-caller exported symbol is a candidate,
not a verdict, and the `dev`-tagged tree is legitimately partial.

Requirements: Go 1.26+, [bats-core](https://github.com/bats-core/bats-core) for e2e tests.

## OAuth Development
Expand Down
51 changes: 51 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,56 @@ bench-compare:
@command -v benchstat >/dev/null 2>&1 || go install golang.org/x/perf/cmd/benchstat@latest
benchstat benchmarks-baseline.txt benchmarks-current.txt

# Report unreachable code, whole-program, from the production binary's main.
#
# NOT a gate, and not wired into `make check`. golangci's `unused` (staticcheck
# U1000) runs per-package and treats every exported identifier in a non-main
# package as used by definition, which is why 602 lines of exported, zero-caller
# internal/tui API survived it — no configuration fixes that, the analysis
# cannot see across package boundaries. deadcode can, because it starts from a
# main and follows the call graph.
#
# Which main matters. Only main packages are roots, so `deadcode ./...` loads
# every package and then reports everything no main reaches as unreachable —
# 1100+ of its 1250 lines here are the dev-tagged workspace tree, which the
# untagged build genuinely cannot reach. True, and useless. Naming the binary
# gives two answers worth reading instead:
#
# deadcode ./cmd/basecamp what a released basecamp can never run
# deadcode -tags=dev ./cmd/basecamp the same, with the dev `basecamp tui` tree
Comment thread
jeremy marked this conversation as resolved.
#
# `go run` the pinned version rather than trusting whatever `deadcode` happens
# to be on PATH; an unpinned tool makes the output depend on the machine.
#
# ONE PLATFORM AT A TIME. deadcode analyzes the current GOOS/GOARCH, and this
# repo releases darwin, linux, windows, freebsd and openbsd. So a host run is
# evidence about the host and nothing else, in both directions: code behind
# another platform's build tag is never loaded, and a function whose only caller
# sits behind one looks unreachable here. Measured on this tree: 78 findings for
# linux and darwin, 77 for windows, with harden_unix.go's ownedByUID present
# only in the unix reports.
#
# To check another target, install the tool for the host and set GOOS for the
# analysis — `GOOS=windows go run ...` cross-compiles the tool itself and dies
# with an exec format error:
#
# go install golang.org/x/tools/cmd/deadcode@v0.40.0
# GOOS=windows deadcode ./cmd/basecamp
#
# Read the output before acting on it. A zero-caller exported symbol is a
# candidate, not a verdict — some are a deliberate API surface, the -tags dev
# tree is legitimately partial, and a cross-platform deletion needs a run per
# target. Wiring a noisy check into CI is how a control nobody sized gets built.
DEADCODE := golang.org/x/tools/cmd/deadcode@v0.40.0

.PHONY: deadcode
deadcode: check-toolchain
Comment thread
jeremy marked this conversation as resolved.
@echo "== shipped binary =="
$(GOCMD) run $(DEADCODE) ./cmd/basecamp
@echo
@echo "== with -tags dev =="
$(GOCMD) run $(DEADCODE) -tags=dev ./cmd/basecamp
Comment thread
jeremy marked this conversation as resolved.

# Guard against Go toolchain mismatch (mise environment)
.PHONY: check-toolchain
check-toolchain:
Expand Down Expand Up @@ -558,6 +608,7 @@ help:
@echo " fmt-check Check code formatting"
@echo " lint Run golangci-lint"
@echo " lint-actions Lint GitHub Actions workflows (actionlint + zizmor)"
@echo " deadcode Report unreachable code from the binary (report, not a gate)"
@echo " tidy-check Verify go.mod/go.sum are tidy"
@echo " check Run all checks (local CI gate)"
@echo " check-surface Verify committed .surface matches the command tree (TestSurfaceSnapshot)"
Expand Down
60 changes: 0 additions & 60 deletions internal/tui/forms_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package tui

import (
"context"
"errors"
"go/ast"
"go/parser"
Expand Down Expand Up @@ -202,65 +201,6 @@ func TestPickerFloorRefusesNonInteractiveStdio(t *testing.T) {
assert.Zero(t, loaderCalls, "a refused picker must not fetch items it cannot show")
}

// TestDeadLaunchersStillRefuse covers Spinner and PaginatedPicker. Both are
// exported, both have zero callers today, and both are slated for deletion —
// but "nobody calls it" is not a gate. Adding a caller would not add a
// tea.NewProgram, so the structural backstop would still pass while the hang
// came straight back. They hold the floor until they are gone.
func TestDeadLaunchersStillRefuse(t *testing.T) {
for _, kind := range stdinKinds {
t.Run("spinner/"+kind, func(t *testing.T) {
terminalStdout(t)
nonInteractiveStdin(t, kind)

var ran bool
done := make(chan error, 1)
go func() {
_, err := NewSpinner("working").Run(func() (string, error) {
ran = true
return "", nil
})
done <- err
}()

select {
case err := <-done:
assert.True(t, errors.Is(err, ErrNotInteractive),
"spinner should return ErrNotInteractive on %s stdin, got %v", kind, err)
assert.False(t, ran, "a refused spinner must not run the work it was wrapping")
case <-time.After(5 * time.Second):
t.Fatalf("spinner blocked on %s stdin instead of refusing", kind)
}
})

t.Run("paginated_picker/"+kind, func(t *testing.T) {
terminalStdout(t)
nonInteractiveStdin(t, kind)

var fetched bool
fetcher := func(context.Context, string) (*PageResult, error) {
fetched = true
return &PageResult{}, nil
}

done := make(chan error, 1)
go func() {
_, err := NewPaginatedPicker(context.Background(), fetcher).Run()
done <- err
}()

select {
case err := <-done:
assert.True(t, errors.Is(err, ErrNotInteractive),
"paginated picker should return ErrNotInteractive on %s stdin, got %v", kind, err)
assert.False(t, fetched, "a refused picker must not fetch a page it cannot show")
case <-time.After(5 * time.Second):
t.Fatalf("paginated picker blocked on %s stdin instead of refusing", kind)
}
})
}
}

// TestAutoSelectSingleWorksWithoutATerminal pins the one path through
// Picker.Run that is deliberately outside the floor. With WithAutoSelectSingle
// and exactly one static item, Run resolves without constructing a bubbletea
Expand Down
10 changes: 1 addition & 9 deletions internal/tui/launchers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,7 @@ var (
// bubbleteaLaunchers maps a file that may call tea.NewProgram to the sole
// function within it that may do so.
bubbleteaLaunchers = map[string]string{
"internal/tui/picker.go": "runPicker",

// Zero callers today and slated for deletion, but "dead" is not a gate:
// adding a caller would not add a NewProgram, so the check would still
// pass while the hang came back. Both launch from their own Run, and
// both apply the floor there.
"internal/tui/spinner.go": "Run",
"internal/tui/paginated_picker.go": "Run",

"internal/tui/picker.go": "runPicker",
"internal/commands/tui.go": "", // dev build tag; not in a shipped binary
}

Expand Down
Loading