mirror of
https://github.com/priyanshujain/sanderling.git
synced 2026-10-02 19:17:10 +00:00
docs: record the audit and correct what this branch disproved
The action-wire entry recommended deleting the handshake this branch keeps and finishes; refusing a pairing is not supporting it. The bindAlways count was wrong in both directions and neither shape is legacy. One of the five legacy-labelled comments does not belong on the list. pollUntilStable was a code defect rather than a comment defect, and the Bounds question is settled against what the three drivers emit.
This commit is contained in:
pj committed
1 parent
9c21489ae7
commit
ac0ec68cb8
1 file changed
+823
@@ -0,0 +1,823 @@
|
||||
# code audit, august 2026
|
||||
|
||||
Eleven parallel audits of the tree at `9b4ff5f`, one per subsystem, each required to cite
|
||||
`file:line` and to verify claims against the repository rather than recall. Detailed
|
||||
per-slice findings sit alongside this file's source notes; what follows is the reconciled
|
||||
result, including the places where the audit contradicted the brief it was given.
|
||||
|
||||
## What has been acted on
|
||||
|
||||
The branch `action-wire-and-test-scaffolding` finished the action-wire handshake, fixed the
|
||||
rest of the vacuous-pass class, corrected the load-bearing false comments, and landed the
|
||||
first shared test scaffolding. What it changed is marked through the sections below; three
|
||||
findings were corrected in the process and say so where they sit. What it deliberately did
|
||||
not touch, each still owed its own change: relocating `cmd/internal-tools`, collapsing the
|
||||
four DOM fact producers, narrowing `DeviceDriver`, trimming the vendored protocol, the
|
||||
remaining false comments outside the load-bearing set, and the 51 table clusters.
|
||||
|
||||
One correctness finding was added to the list during the work rather than found by the audit.
|
||||
Chrome's `Metrics` filled `CPUPercent` with a zero, which is the same answer an idle app
|
||||
gives, so every web step recorded a CPU reading nobody took. It is the same defect as the
|
||||
three iOS reads and it is fixed the same way: `driver.Metrics.CPUPercent` and
|
||||
`trace.Metrics.CPUPercent` are now optional, and absence is how a driver says it did not look.
|
||||
|
||||
Two findings were reported to this file rather than fixed, because both are decisions rather
|
||||
than defects.
|
||||
|
||||
**The campaign record still has three declarations.** Collapsing them was considered and
|
||||
rejected as too large for that change: they are not one type with drift. The writer emits
|
||||
value ints with `omitempty`; `analyze/load.go` deliberately declares `Actions` and
|
||||
`UnattributedActions` as `*int` so a `runs.jsonl` written before those counts existed is
|
||||
refused rather than read as an arm that acted zero times, and two of its tests assert that
|
||||
refusal; `confusion-matrix` reads a different subset again, including `RunDirectory`. One
|
||||
shared type has to break one of those. If it is done, the shape is a type in a package all
|
||||
three import, carrying pointers for every field whose absence has to be distinguishable from
|
||||
zero, with the writer always emitting them, so the `duration_millis` shim moves in with it as
|
||||
a named accessor rather than being rediscovered per reader.
|
||||
|
||||
**The two settle primitives now disagree, on purpose on one side.**
|
||||
`sidecar/.../DriverBackend.kt` no longer charges a read's own duration to the stability
|
||||
streak, because its doc said it did not and the code did. `internal/driver/ioscompanion/settle.go:58-64`
|
||||
charges it deliberately and says so, arguing that a traversal-based read would show mid-read
|
||||
churn as a byte difference. That argument is not unreasonable and it is an argument for a
|
||||
weaker guarantee, so it should be settled on the evidence rather than adopted by drift.
|
||||
|
||||
## Verdict
|
||||
|
||||
The complaint that prompted this audit was that the repository is padded with unnecessary
|
||||
code and worthless tests, and that a peer project solving a similar problem does it in a
|
||||
third of the lines. The size gap is real and the causes are not what the complaint assumed.
|
||||
|
||||
Three conclusions, in order of how much they change what should be done next:
|
||||
|
||||
1. **The test suite is not the problem.** It is large, 1.42:1 over source in the runner and
|
||||
driver packages and 1.65:1 in the verifier core, but only about 5 to 10 percent of it is
|
||||
genuinely deletable. Most of it asserts on `trace.jsonl` bytes on disk, carries explicit
|
||||
control arms, and names the bug class in the test name. Measured rather than assumed:
|
||||
dropping the ten most redundant-looking `internal/hierarchy` tests moves statement
|
||||
coverage from 91.9 percent to 91.9 percent, and the three next-worst clusters lose seven
|
||||
statement blocks between them, five of which are one-line getters. The waste is
|
||||
structural, the fifth copy of a pattern that was never folded back into the first, and
|
||||
tests that were never deleted after a stronger test subsumed them.
|
||||
|
||||
2. **The bulk is in research tooling and a vendored protocol, not the product.** `cmd/`
|
||||
is 20,565 lines, of which the shipped CLI is 1,475. The other 19,090 sit in
|
||||
`cmd/internal-tools`, which is 14.6 times the size of the product CLI and 30 percent of
|
||||
the whole engine. Eight of its eleven binaries were created in the HEAD commit, have a
|
||||
one-commit history, and five of them cite specifications that live in `paper/`, a
|
||||
separate git repository. The Makefile builds none of them. Separately, the vendored iOS
|
||||
protocol generates 12,283 lines from 97 messages, of which 14 are ever called.
|
||||
|
||||
3. **Where the two projects do the same job, they cost the same.** The browser target is
|
||||
5,927 lines against the peer project's 5,777, and the peer's does more inside that
|
||||
budget. None of the size difference is this tool being worse at the work it exists to do.
|
||||
|
||||
The rules in `CLAUDE.md` were not uniformly ignored, and the pattern in how they failed is
|
||||
the most useful finding in the audit. Rules an agent can check while typing a single line
|
||||
were followed, completely: zero em dashes in the entire tracked source tree, zero `TODO`,
|
||||
zero `FIXME`, zero `HACK`, zero emoji headings, no badges and no contributing boilerplate
|
||||
in any of the five READMEs. Rules that require standing back from a finished artefact were
|
||||
not followed at all: is this comment needed, is this commit too large, does this README hold
|
||||
more than a description. That is a review-gate problem, not a discipline problem, and it is
|
||||
fixable with checks rather than with resolve.
|
||||
|
||||
## Numbers
|
||||
|
||||
Non-blank lines, identical exclusions applied to both repositories, counted against the
|
||||
tree rather than recalled.
|
||||
|
||||
| | this repository | peer project | ratio |
|
||||
|---|---:|---:|---:|
|
||||
| source, non-test | 56,708 | 19,065 | 3.0x |
|
||||
| test | 49,277 | 6,693 | 7.4x |
|
||||
| total | 105,985 | 25,758 | 4.1x |
|
||||
| comparable product code only | 68,060 | 23,597 | 2.9x |
|
||||
| files | 506 | 226 | 2.2x |
|
||||
| test to source ratio | 0.87, or 1.11 excluding generated | 0.35 | |
|
||||
| doc comments in non-test Go | 3,512, 15.9% of code | | |
|
||||
| inline comments in non-test Go | 1,164, 5.3% of code | | |
|
||||
| test functions | 1,280 | 136 | 9.4x |
|
||||
| driver interface width | 18 methods plus 6 capability interfaces | 6 methods, 3 associated types | 4.5x |
|
||||
| non-test files touched by one action verb | 34 | 8 | 4.3x |
|
||||
|
||||
Static analysis is clean and the suite is green. `go vet ./...` reports nothing across all
|
||||
38 packages. `staticcheck ./...` reports one finding in source. The full suite runs in 24.9
|
||||
seconds wall, 1,530 tests passed, 0 failed, 6 skipped, all six env-guarded rather than
|
||||
broken. Average cyclomatic complexity is 4.26 with 7.2 percent of functions over 10, and
|
||||
the excess is concentrated: `internal/runner/runner.go`, `internal/ltl/evaluator.go` and
|
||||
`internal/verifier/worker.go` own 8 of the top 30.
|
||||
|
||||
Duplication runs opposite to the assumption. Over 12-line normalised windows, source-only
|
||||
duplication is 1,036 lines across 23 cross-file blocks, 4.7 percent. Including tests it is
|
||||
1,831 lines across 48 blocks, 3.1 percent. Tests are 62 percent of the code and produce 43
|
||||
percent of the duplicated lines, so per line they are about half as duplicated as source.
|
||||
18 of the 23 source blocks are one pair of packages.
|
||||
|
||||
## Where the size difference actually goes
|
||||
|
||||
| bucket | here | peer | delta | legitimate |
|
||||
|---|---:|---:|---:|---|
|
||||
| research and experiment tooling | 18,424 | 885 | +17,539 | yes, but it is not the tool |
|
||||
| generated protobuf and the `.proto` | 13,292 | 0 | +13,292 | half |
|
||||
| core engine | 20,092 | 8,859 | +11,233 | partly |
|
||||
| iOS target | 10,502 | n/a | +10,502 | yes |
|
||||
| Android target | 6,296 | n/a | +6,296 | yes |
|
||||
| browser target | 5,927 | 5,777 | +150 | yes |
|
||||
| terminal target, peer only | 0 | 2,143 | -2,143 | |
|
||||
| web UI and replay server | 6,333 | 2,477 | +3,856 | no |
|
||||
| spec language | 6,122 | 1,407 | +4,715 | partly |
|
||||
| fixture apps and example specs | 5,868 | 322 | +5,546 | mostly |
|
||||
| LLM action policy | 4,036 | 0 | +4,036 | yes |
|
||||
| second JS runtime and its parity tests | 3,785 | 0 | +3,785 | no |
|
||||
| CLI and orchestration | 3,349 | 1,066 | +2,283 | partly |
|
||||
| E2E harness and page fixtures | 1,618 | 1,868 | -250 | yes |
|
||||
|
||||
Roughly 44,500 lines of the gap is legitimate scope: two mobile targets the peer does not
|
||||
have, an LLM policy it does not have, and two fixture applications. Roughly 8,500 is
|
||||
mechanically removable generated code. Roughly 27,200 is structural.
|
||||
|
||||
## Correctness findings
|
||||
|
||||
These are defects, not style. They come first because several of them cause the tool to
|
||||
report success while checking nothing, which is the worst failure mode a property-based
|
||||
checker can have.
|
||||
|
||||
**A run that spent half its budget outside the app enters the survival analysis as clean
|
||||
data.** `cmd/internal-tools/campaign/summary.go:50` writes `precondition_failures`, and
|
||||
neither reader declares the field: `analyze/load.go:32` and `confusion-matrix/checker.go:56`
|
||||
each redeclare the campaign record independently. `summary.go:188` counts those steps into
|
||||
`Steps`, and the exclusion logic at `analyze/load.go:190-204` filters on four conditions,
|
||||
none of which is this one. `summary.go:45-49` states the requirement in its own words,
|
||||
"counting it as a run that explored and found nothing puts a harness failure in the same
|
||||
column as evidence", and then the pipeline does exactly that. The record schema being
|
||||
declared three times is why: `analyze/load.go:45` already carries a `duration_millis`
|
||||
back-compat shim for an earlier break that one shared type would have prevented.
|
||||
|
||||
**iOS answers three contract methods with fabricated success.**
|
||||
`internal/driver/ioscompanion/driver.go:1115-1125` returns `[]` from `RecentLogs`, zero from
|
||||
`Metrics`, and a hardcoded `Ready: true` from `Health`. The preflight at
|
||||
`internal/runner/runner.go:1576` therefore checks nothing on iOS, and every `state.logs`
|
||||
property holds vacuously with no machine-visible signal that it never looked. The fix is
|
||||
not to implement them, it is to move them off the mandatory interface so that silence is
|
||||
representable.
|
||||
|
||||
**A truncated page is served as a 200.** `internal/replay/server.go:245` reimplements
|
||||
`io.ReadAll` and swallows every error except `fs.ErrInvalid`, returning a partial buffer
|
||||
with a nil error. One caller, 18 lines, and a half-rendered `index.html` is indistinguishable
|
||||
from a whole one.
|
||||
|
||||
**Actions render as raw text in the replay UI.** `replay-ui/src/lib/action-format.ts:9-19`
|
||||
lists `textPrefix` and `classPrefix`, which are not in the canonical key list at
|
||||
`pkg/spec/test/fixtures/selector-keys.json`, and omits `testTag`, `data-testid`, `identifier`
|
||||
and `accessibilityIdentifier`. `internal/verifier/worker.go:893-915` writes `testTag:` labels
|
||||
and `pkg/spec/src/web-runtime.ts:1124-1135` writes `data-testid:`. `testTag` is the primary
|
||||
Android identification route, so the most common case is the one that renders wrong.
|
||||
|
||||
**A failed CDP round trip is indistinguishable from a page without the API.**
|
||||
`internal/driver/chrome/driver.go:983-985` swallows its own error and returns zero metrics
|
||||
with a nil error.
|
||||
|
||||
**541 lines of tag-gated tests never run in CI, and 167 of those run nowhere at all.**
|
||||
Four test files sit behind build tags. `internal/sidecarassets/embed_withsidecar_test.go`,
|
||||
167 lines including a four-goroutine torn-write race test, is gated on `withsidecar`, and no
|
||||
target anywhere runs `go test` with that tag: `withsidecar` appears at `Makefile:59,71,80,93`
|
||||
under `go build`, `go install` and `go run` only. It has never been compiled into a test
|
||||
binary. The other three, `internal/driver/ioscompanion/smoke_test.go` at 85 lines and the two
|
||||
`embed_withcompanion_test.go` files at 146 and 143, total 374 lines and are reachable only
|
||||
through `make test-companion` at `Makefile:148`, which CI never invokes: `ci.yml` runs `make
|
||||
test`, `make test-folio` and `make test-browser` and nothing else that tests Go.
|
||||
Separately and less severely, 671 lines are compiled but skipped at runtime for want of an
|
||||
environment variable, 105 in `internal/driver/ioscompanion/transport/integration_test.go`
|
||||
needing `SANDERLING_IOS_INTEGRATION`, which is set nowhere in the repository, and 566 in a
|
||||
subject-specific draft needing a spec directory named by an environment variable. Those two are guarded
|
||||
by design; the 541 are not. This is a coverage gap wearing bloat's clothing and it outranks
|
||||
every deletion in this document.
|
||||
|
||||
**One test does not test its subject.**
|
||||
`internal/runner/runner_test.go:2104`, `TestRunner_InternalApplyErrorMarksTransitional`,
|
||||
asserts only `err != nil` and `summary.Steps < 2`. Deleting the entire transitional-marking
|
||||
branch leaves it green. Its helper at `:2084` also carries a comment saying the first
|
||||
`InputText` call fails, while the method it overrides is `TapSelector`.
|
||||
|
||||
**One test is racy by construction.**
|
||||
`internal/driver/ioscompanion/supervision_test.go:47` sleeps 100ms hoping a shell has
|
||||
installed `trap "" TERM`. If it has not, plain SIGTERM works, the escalate-to-SIGKILL path
|
||||
the test name promises is never exercised, and the test passes anyway.
|
||||
|
||||
## Structural findings, ranked by payoff
|
||||
|
||||
**Move the research tooling out of the product module.** 16,491 lines leave `cmd/`, and
|
||||
`internal/tracecorpus` at 235 lines goes with its only two consumers. They compile and test
|
||||
today only because `go test ./...` sweeps them up, at a measured 23.3 seconds per run.
|
||||
`docs/development/decisions.md:49-53` is the only written policy governing this directory,
|
||||
it says the directory holds two dev and debug binaries and should stay "for now", and it now
|
||||
holds eleven. Two of the eleven should not simply move: `bundle-check` is the only one a
|
||||
spec author wants and should fold into the product as `sanderling spec check`, since it
|
||||
already runs the product code path; `hier-check` should be deleted, as nothing references
|
||||
it, its usage string at `main.go:12` promises `<dump.xml>` while it parses JSON, and its
|
||||
`main_test.go` never calls its own code.
|
||||
|
||||
**Delete the page-side JavaScript and keep one producer of DOM facts.** DOM fact derivation
|
||||
and selector resolution exist four times: `pkg/spec/src/web-runtime.ts`; roughly 350 lines of
|
||||
untyped JavaScript inside a Go raw string literal at `internal/driver/chrome/driver.go:578-934`
|
||||
where prettier, `tsc` and `noUnusedLocals` are all blind to it; `internal/hierarchy/hierarchy.go`
|
||||
with its own key list at line 302; and the replay-UI copy above. The evidence that this is a
|
||||
standing hazard rather than a tolerable copy: every one of the nine commits that has ever
|
||||
touched `web-runtime.ts` also touched `driver.go` or `hierarchy.go`. Nine out of nine. It is
|
||||
currently held together by roughly 2,500 lines of parity tests, four probe entry points and
|
||||
about 40 cross-reference comments. The drift postmortems in the file all report the same
|
||||
outcome, at `web-runtime.ts:389-393`, `:555-559` and `:590-594`: the two sides disagreed and
|
||||
properties passed vacuously rather than failing. `:161-166` records that the worked example in
|
||||
`docs/manual/spec-language.md` named a control by a key that matched nothing on web, and passed
|
||||
having checked nothing. Having the Go dump call the already-injected bundle removes the copy
|
||||
and retires most of that parity suite.
|
||||
|
||||
**Trim the vendored protocol.** 43 RPCs are declared, 9 are called, and 2 of those 9 are
|
||||
dead: `internal/driver/ioscompanion/runner.go:302` says so in its own comment, "the driver
|
||||
does not call these today; they complete the Companion interface". Generating only the 14
|
||||
messages actually used removes roughly 8,500 lines with no behaviour change, and dropping
|
||||
install and uninstall removes 133 more including `tarGzipDirectory`.
|
||||
|
||||
**Collapse the twin sweep binaries.** `corpus-sweep` and `implementation-sweep` share 331
|
||||
exactly-identical meaningful production lines and 163 more between their two end-to-end
|
||||
tests, which are 301 and 276 lines respectively, the two longest functions in the
|
||||
repository. Ten of eleven flags are identical. The entire difference is whether an
|
||||
implementation needs a build step first, which is a boolean.
|
||||
|
||||
**Narrow `DeviceDriver` from 18 mandatory methods to 12.** `Screenshot` has zero callers:
|
||||
all three `Snapshot` implementations use their own path at `chrome/driver.go:807`,
|
||||
`ioscompanion/driver.go:1057` and `sidecar/client.go:269`. It exists for symmetry with
|
||||
`Hierarchy`. `RecentLogs`, `Metrics` and `Health` become optional, which is what stops iOS
|
||||
having to lie. `Launch`'s `env` map becomes a capability that only the sidecar implements,
|
||||
since iOS rejects it and chrome silently drops it. The six optional capability interfaces
|
||||
are the right shape and should not be collapsed: each has a real consumer in the runner and
|
||||
a real non-implementer, which is that pattern working correctly.
|
||||
|
||||
**Stop maintaining two ASTs for one language.** `internal/verifier/bindings.go:28-58`
|
||||
declares a 10-value `specKind` enum and an 8-field `formulaSpec` with integer child edges
|
||||
that mirror `ltl.Formula` one for one, and `internal/verifier/worker.go:259-341` is an
|
||||
83-line translator back, containing eleven copies of the same three-line child-and-error
|
||||
block. Storing `ltl.Formula` directly deletes around 120 lines and, more importantly, takes
|
||||
adding one operator from eleven edit sites to one. The isolation argument does not apply:
|
||||
`worker.go:287-294` already constructs `ltl.EventuallyFormula` literals by hand, so the
|
||||
boundary is already crossed.
|
||||
|
||||
**Replace the hand-rolled ordered-JSON encoder.** `internal/verifier/marshal.go:392-570` is
|
||||
179 lines guaranteeing field order and unescaped HTML across the two hosts. `encoding/json`
|
||||
already emits declaration order, `omitempty` already handles presence, and
|
||||
`SetEscapeHTML(false)` is already in use at `marshal.go:520`. Tagged structs plus
|
||||
`goja.TagFieldNameMapper` do the same job in roughly 90 fewer lines.
|
||||
|
||||
**Merge the two asset packages.** `companionassets` and `runnerassets` are the same 140-line
|
||||
package twice, differing in 8 cosmetic hunks, and four dead exports fall out of the merge.
|
||||
Both also validate tar member paths through `safeJoin` but do not validate `header.Linkname`
|
||||
before `os.Symlink`.
|
||||
|
||||
**Deduplicate the smaller copies.** `decodeDump`'s body is pasted byte for byte inside
|
||||
`MapHierarchy` in the same package, `input.go:420-433` against `hierarchymap.go:77-88`.
|
||||
`Bundle` at `internal/bundler/bundler.go:36` and `BundleWeb` at `web_bundle.go:30` are 132
|
||||
code lines differing in three expressions, and `web_bundle.go:37` calls `filepath.Abs` and
|
||||
discards the result before calling it again at line 44. The route-transition rule exists
|
||||
three times with three different key sets, at `hierarchy.go:654` with one key, `settle.go:123`
|
||||
with five, and `chrome/driver.go:902` with one, and they already disagree.
|
||||
`Tree.FindBySelectorPath` and `Node.FindBySelectorPath` differ in three lines, all `t` against
|
||||
`n`, as do the `FindAll` pair. `parseBounds` at `hierarchy.go:1029-1053` contains the same
|
||||
11-line body twice, once per regex.
|
||||
|
||||
**Retire the dead instrument.** `Tree.UnreadableFlags` at `internal/hierarchy/hierarchy.go:106`
|
||||
is computed at `:554`, serialized twice, and read by zero production callers. Its two JSON
|
||||
keys disagree with each other, `unreadableFlags` against `unreadable_flags` at `:120`, and
|
||||
every `json` tag on `Tree` is dead anyway because `Tree` has a custom `MarshalJSON`.
|
||||
|
||||
**Undocumented escape hatch.** `SANDERLING_SIMULATOR_COMPANION=legacy` at
|
||||
`internal/driver/ioscompanion/driver.go:404` appears in no document, Makefile or CI config,
|
||||
and gates roughly 313 lines: `pasteText`, `findAllowPasteButton`, `pasteLanded`,
|
||||
`grantPasteboardAccess` and `resolveInputField`. Document it or delete it. Leaving it
|
||||
undocumented and live is the worst of the three options.
|
||||
|
||||
**Dead code, tool-backed.** `deadcode -test=false ./...` reports 48 unreachable first-party
|
||||
functions; with test roots included that falls to 1. So 47 of 48 are reachable only from test
|
||||
binaries, which is over-exported test scaffolding rather than dead product code. The single
|
||||
genuinely dead one is `Cache.Root` at `internal/replay/runs_cache.go:29`. `staticcheck
|
||||
-tests=false` reports 12 `U1000`, eleven of which are one statistics island in
|
||||
`cmd/internal-tools/analyze` that `deadcode` independently flags, so both tools agree.
|
||||
Confirmed zero-caller exports in the product: `ltl.Describe` at `formula.go:235`,
|
||||
`ltl.EventuallyBefore` at `:158`, `ltl.EventuallyWithin` at `:162`, `ltl.Thunk`,
|
||||
`ltl.Eventually`, `Evaluator.Observe` at `evaluator.go:81`, and
|
||||
`trace.ActionSourceSeeded` at `writer.go:118`. The `ErrorFormula` arms in `reduce`
|
||||
(`evaluator.go:395-399`) and `pushNot` (`nnf.go:49-50`) are unreachable, since `ErrorFormula`
|
||||
is built only at `worker.go:771` for serialization and never re-enters an evaluator.
|
||||
|
||||
Two more the tools did not reach, both found while correcting the comments beside them.
|
||||
`isWDADrop` in `internal/runner/runner.go` and its two call sites classify an error string
|
||||
nothing can produce any more: `WdaRecovery` is constructed only in its own test
|
||||
(`DriverBackend.kt:1557`, `WdaRecoveryTest.kt:16`), so the sidecar no longer throws it.
|
||||
`testrun.preflightDevice` was dead in a way that also made it wrong, and is deleted: it was
|
||||
called only after web and both iOS branches had returned, so it never saw a platform other
|
||||
than android, and it returns nil for anything but ios. The comment over its call site said it
|
||||
was Android's java check, which is in `runPreflight`, and its own premise was wrong because a
|
||||
physical iOS device is runner-only over usbmux and needs no JVM.
|
||||
|
||||
**One dead feature spans four layers.** `trace.Step.Snapshots` at `internal/trace/writer.go:27`
|
||||
has no producer, yet `replay-ui/src/panels/SnapshotTable.tsx` renders a tab for it, wired at
|
||||
`RunDetail.tsx:141`. Every step of every run shows an empty panel. Separately,
|
||||
`replay-ui/src/panels/Timeline.tsx`, 142 lines, is never rendered: its only importers use
|
||||
`import type`, and the `RunHistory.lanes` tail it feeds has zero readers.
|
||||
|
||||
**`VERB_SUPPORT` is a no-op.** `pkg/spec/src/verbs.ts:18` maps every verb to all three
|
||||
platforms, so `supports()` cannot return false, which makes `warnUnsupportedOnce`,
|
||||
`resetWarnings`, `Host.reportUnsupported` at three call sites, one `pick.ts:103-106` branch
|
||||
and the whole of `verbs.test.ts` unreachable, along with 26 pointless `resetWarnings()` calls
|
||||
in `pick.test.ts`.
|
||||
|
||||
**`runner.go` is not mostly ceremony.** At 1,818 lines it is 1,233 code and 500 comment, and
|
||||
it does five jobs in one file rather than 500 lines of work in 1,818. It splits along its
|
||||
existing seams into runner, foreground, dispatch, observe and trace encoding, largest piece
|
||||
around 420 lines, with no logic change. `Run` is 473 lines with a cyclomatic complexity of
|
||||
60, the highest in the repository.
|
||||
|
||||
**One abstraction is justified by a false statement.** `internal/runner/source.go:41` says the
|
||||
interface is declared there "rather than folded into driver.WebDriver so the mobile drivers
|
||||
stay untouched", but `WebDriver` is web-only and no mobile driver implements it, so the error
|
||||
branches at `source.go:102-116` are unreachable. `idleTimeoutFloor` at `runner.go:630` cites
|
||||
the same false reason. Similarly, `ltl.PredicateLabel` at `internal/ltl/formula.go:17-22`
|
||||
exists so that `formula.go:394` can type-assert a `ThunkFormula` that cannot fail the
|
||||
assertion, replacing a single field read.
|
||||
|
||||
**One correction to an audit finding.** The `{"op":"now"}` serialized node was reported as
|
||||
having no consumer. It has one: `replay-ui/src/types.ts:106` declares it in the `ResidualNode`
|
||||
union and `replay-ui/src/components/ResidualNode.tsx:41` renders it. The agent searched for
|
||||
`web/src`, which does not exist here, and concluded the UI did not either. `NowFormula` is
|
||||
still the identity function at every evaluator site, but the wire node is load-bearing and
|
||||
must not be dropped.
|
||||
|
||||
## The test suite, measured
|
||||
|
||||
| package group | test | source | ratio |
|
||||
|---|---:|---:|---:|
|
||||
| runner, drivers, testrun, cmd | 32,886 | 23,166 | 1.42 |
|
||||
| `internal/runner` alone | 7,270 | 2,589 | 2.81 |
|
||||
| `internal/verifier` | 5,218 | 3,179 | 1.64 |
|
||||
| `internal/hierarchy` | 1,886 | 1,063 | 1.77 |
|
||||
| `internal/ltl` | 1,595 | 1,045 | 1.53 |
|
||||
|
||||
Genuinely deletable across both audited scopes is roughly 2,600 lines, about 5 percent, not
|
||||
the 40 percent the size ratio suggests.
|
||||
|
||||
Every one of the 1,281 test functions was then classified mechanically by the condition
|
||||
guarding its assertions. 47 functions, 658 lines, assert only on nil, on `err`, or on a length
|
||||
against zero. 896 functions compare actual values and 315 more compare against a golden file
|
||||
or by deep equality. The residual "no assertion" bucket is 23 functions and is almost entirely
|
||||
an artefact of the classifier, since these tests embed spec source in raw string literals
|
||||
containing a brace at column zero; the genuine case is `mock_test.go:123`. So the mechanically
|
||||
detectable garbage is roughly 800 lines, which reproduces the reading-based estimate from a
|
||||
different direction.
|
||||
|
||||
That figure is a floor, not a ceiling. The method cannot see tautology:
|
||||
`internal/driver/ioscompanion/keymap_test.go:202` compares two values and is worthless,
|
||||
because the values are constants declared four lines away in `keymap.go:32-33`. Tests of that
|
||||
shape score as healthy. The honest statement is that the suite's assertions are overwhelmingly
|
||||
real, and that an unmeasured remainder above 800 lines is tautological.
|
||||
|
||||
The bloat is not in the tests. Go test files hold 43,435 lines, of which 29,770 sit inside a
|
||||
test function. The other 13,665 lines, 31 percent of the suite, are scaffolding: 256 test-only
|
||||
helpers, 65 test-only types, 26 one-off wrapper drivers in a single package, `traceLine`
|
||||
redeclared fifteen times, and four separate spec bundlers in one package's tests. Consolidating
|
||||
the harness is worth several times what deleting tests is worth, and it costs no coverage. What is actually wrong is repetition of shape:
|
||||
|
||||
- 26 one-off wrapper drivers live in one package, ten of them in `runner_test.go`, six of
|
||||
which override `TapSelector` to return a different error on call N. `mock.go` already has
|
||||
`Failures map[ActionKind]error`; one per-call hook retires all 26, about 280 lines.
|
||||
- `type traceLine struct` is redeclared inline ten times inside `runner_test.go` and five
|
||||
more times across the package, while `readTraceLines` sits at `runner_test.go:1595` and
|
||||
`trace.jsonl` is opened by hand in 24 places. Two fields on `traceStepLine` collapse all
|
||||
fifteen, about 250 lines.
|
||||
- 57 of 98 `internal/hierarchy` tests have bodies of 12 lines or fewer, nearly all one row of
|
||||
an unwritten table. Nine clusters, roughly 650 lines, collapse into tables. Four properly
|
||||
table-driven tests already sit in the same files, so the pattern was known and not applied.
|
||||
- 72 of 98 hierarchy tests write `tree, _ := Parse(...)`, so a fixture that stops parsing
|
||||
panics on a nil dereference inside `Find` instead of reporting a parse failure.
|
||||
- Four separate spec bundlers exist in `internal/verifier`'s tests, differing only in the
|
||||
alias map.
|
||||
- Confirmed subsumed: `chrome/driver_test.go:140`, `:202` and `:264` are superseded by
|
||||
`fact_parity_test.go:92`, which reads the same fields with forced both-polarity coverage
|
||||
and cross-checks the real web runtime, while the middle of the three does not call the web
|
||||
runtime at all despite its name. 163 lines. Also `runner_test.go:351` by `:374`,
|
||||
`runner_test.go:240` by `llm_source_test.go:837`, and `runner_test.go:1226` by
|
||||
`scroll_distance_test.go:52`.
|
||||
|
||||
Three tests cannot go red at all. `internal/driver/mock/mock_test.go:123` is a compile-time
|
||||
assertion wearing a `func Test` hat, with no assertion and no use of `t`.
|
||||
`internal/driver/ioscompanion/keymap_test.go:202` asserts two constants equal the literals
|
||||
they are declared with four lines away in `keymap.go:32-33`.
|
||||
`internal/verifier/llm_test.go:674` installs a stub returning `"sampled"` and asserts the
|
||||
function returns `"sampled"`, under a name claiming it draws from a corpus that is never
|
||||
reached.
|
||||
|
||||
Two structural gaps matter more than any of the above. The two-host parity tests at
|
||||
`internal/verifier/marshal_test.go:166` and `:299` assert that the two encoders agree with
|
||||
each other, with no golden anywhere, so a field rename landing on both encoders at once,
|
||||
which is the likely change, passes silently. The same gap exists at `redaction_test.go:174`
|
||||
and `:193`. And `cmd/internal-tools/hier-check/main_test.go:11-21` tests `hierarchy.Parse`
|
||||
and `tree.FindAll` directly, so if hier-check's own formatting or exit codes break, nothing
|
||||
notices.
|
||||
|
||||
## Rule compliance
|
||||
|
||||
| rule | verdict | count |
|
||||
|---|---|---|
|
||||
| zero comments first, WHY never WHAT | ignored on volume, followed on quality | 7,534 comment lines under no linter mandate, 35% WHAT in a 60-block sample |
|
||||
| never use an em dash | followed in source | 0 tracked, 7 in commit messages, 43 in gitignored `research/` |
|
||||
| never write a directory tree | one violation | `docs/manual/runs.md:23-27` |
|
||||
| README under 15 lines | partly | 3 of 5 over budget, 277 lines total |
|
||||
| lowercase kebab-case doc names | partly | 11 flagged, 1 committed and discretionary |
|
||||
| no abbreviated identifiers | ignored | 683 occurrences, 51 distinct identifiers |
|
||||
| commits 1-3 files, under 20 lines | ignored on lines, followed on files | 1,216 of 1,864 commits over 20 lines, 194 over 3 files |
|
||||
| no filler, every line load-bearing | partly | 123 of 1,565 comment blocks restate the code |
|
||||
|
||||
Two of these need care rather than a sweep. `AttrFilter`, `AttrSelector` and the `Attr`
|
||||
field are baked into the public selector API and leak into `pkg/spec/src/types.ts`, so that
|
||||
abbreviation cannot be removed without a breaking change; it should be scheduled, not
|
||||
grepped away. And `ctx context.Context` is idiomatic Go and was excluded from the count.
|
||||
|
||||
Go convention does not ask for fewer comments in general, it asks for doc comments on
|
||||
exported symbols and for inline comments to be rare. Measured against that, this repository is
|
||||
close to correct: 3,512 doc-comment lines against 22,155 lines of code, and only 1,164 inline
|
||||
comment lines, 5.3 percent. Three quarters of the comment volume is the convention being
|
||||
followed. Comparing a single total against a project that writes few doc comments overstated
|
||||
the problem, and the earlier 20.8 percent figure conflated the two kinds.
|
||||
|
||||
The comment rule failed hardest outside the engine.
|
||||
`examples/folio/sanderling/predicates.ts` is 61.7 percent comment, 472 comment lines to 293
|
||||
code, with two inline blocks of 13 and 17 lines wrapped around about 12 lines of guard
|
||||
clauses. `pkg/spec/src/action-tree.ts` is 47 percent comment. Inside the drivers the
|
||||
opposite holds: nearly every comment records a measured symptom, and the genuine
|
||||
WHAT-comments there total only 60 to 80 lines, most of them on the legacy paste path that
|
||||
should be deleted anyway.
|
||||
|
||||
Comment rot is the single largest defect in the repository's prose, and the first pass of
|
||||
this audit understated it badly. A per-block classification of 1,565 comment blocks, 95
|
||||
percent of every comment in non-test source, finds **78 that are false**: they disagree with
|
||||
the code beside them. The first pass named two. Detail is in the comment section below.
|
||||
`internal/runner/runner.go:185` claims a returned error propagates to every sibling read,
|
||||
while all three goroutines return nil at lines 198, 202 and 207, and line 209 admits the
|
||||
`Wait` error is always nil, so the errgroup is a `sync.WaitGroup` plus a dependency plus an
|
||||
untrue comment. `internal/testrun/testrun.go:93-99` stacks three doc comments over the wrong
|
||||
function, leaving `Execute` and `buildRunMeta` undocumented. The worst single instance is
|
||||
`internal/runner/runner.go:760-768`, nine comment lines above one constant, eight of them
|
||||
narrating a rejected design and two emulator measurements. The measurement is worth two
|
||||
lines; the rest is a lab notebook checked into a runner.
|
||||
|
||||
## Repository hygiene
|
||||
|
||||
Nothing untoward is tracked at HEAD. 674 files, 5.32 MB, no secrets, no build artefacts, no
|
||||
vendored bundles, no `dist`, no `node_modules`. The problems are all around it.
|
||||
|
||||
`.git` is 659 MB for 5.32 MB of content, a ratio of 118 to 1. The largest object in the
|
||||
repository is a 138.9 MB compiled binary carrying the dead pre-rename project name, blob
|
||||
`9788374f`, reachable only from stash commit `6bb190f`, "untracked files on feat-inspect-ui".
|
||||
A `git stash` swept up a compiled binary and `refs/stash` has pinned it ever since.
|
||||
`.git/lost-found/other` holds a further 152 MB of `git fsck` salvage that no ref points at,
|
||||
and there are 12 packs, 479 prune-packable objects and 65 local branches against 42 remote.
|
||||
Separately, a 35.4 MB compiled binary is permanent in `master` history, added in `88db0cb`
|
||||
and removed in `2a1b263`, along with 426 KB of UI bundles: 35.8 MB total, about 6.4 times the
|
||||
entire legitimate tree and roughly 70 percent of what a fresh clone downloads. Ignoring these
|
||||
afterwards stopped recurrence but every clone still pays.
|
||||
|
||||
`keys/` holds live Apple distribution secrets: an `AuthKey` `.p8`, a distribution private key
|
||||
PEM, a `.p12` and a mobileprovision. History is clean and nothing has leaked. The only thing
|
||||
standing between them and a commit is one line, `keys/*`, appended at `.gitignore:87` with no
|
||||
section comment, and `git check-ignore keys` shows the directory itself unmatched. Signing
|
||||
material belongs in the keychain, not in a git working tree. `scripts/` is currently
|
||||
untracked and unignored, so it is one `git add -A` away from being committed, which is one
|
||||
reason the project bans that command.
|
||||
|
||||
The working tree holds 9.16 GB of untracked output: 4.3 GB under `conformance/runs`, 2.7 GB
|
||||
of run artefacts under `examples`, 92 MB of Gradle jars under `sidecar/build`, 141 MB under
|
||||
`companion`, 37 MB each for `bin` and a stray `sanderling` binary, 25 MB in `runs`. Five
|
||||
empty `idb-*` directories and an empty `memory/` directory have sat there since April and
|
||||
June and are not ignored.
|
||||
|
||||
Twelve of 33 make targets are dead or undocumented, `conformance/gates.sh` at 574 lines with
|
||||
a self-test mode and committed fixtures is referenced by nothing, and there is no CI format
|
||||
check despite five `fmt-*` targets existing.
|
||||
|
||||
Documentation is accurate where it counts and stale in three places. Twelve of fifteen
|
||||
spot-checked claims verified against the code, including every one of the 22 CLI flags, which
|
||||
are all documented in `docs/manual/cli.md` with no undocumented and no dead flag. The three
|
||||
failures: `docs/manual/action-space.md:15` says `PressKey` has 8 keys where
|
||||
`pkg/spec/src/types.ts:209-218` has 9, missing `escape`; `action-space.md:14` documents a
|
||||
`durationMillis?` field on `Scroll` that does not exist; and `docs/development/decisions.md:25-27`
|
||||
says `marshal.go` moves to `internal/replay/`, which never happened, while `:47-49` describes
|
||||
a `runs.go` split as pending that has already been done. `README.md` is 40 lines against its
|
||||
own 15-line standard, with lines 36-40 byte-identical to `docs/index.md:25-29`, the manual
|
||||
link list repeated four times and the pitch three. `examples/folio/README.md` is 150 lines
|
||||
with nine headings that are all `docs/` content. `decisions.md` and `action-space.md` are
|
||||
orphaned, stale and linked from nowhere.
|
||||
|
||||
## What must not be touched
|
||||
|
||||
A cleanup driven by line count would destroy the best work in the repository. The following
|
||||
were examined and found to be load-bearing.
|
||||
|
||||
`internal/ltl/evaluator.go` in full: `reduce`, the three-valued `finalize`, the bounded
|
||||
always and eventually duality at `:528-540`, `collapse`'s `describe()` key with per-thunk
|
||||
identity, `nnf`'s implication rewrite and `isOneShotRoot`. It is a real standalone evaluator
|
||||
carrying both step and wall-clock bounds, where the peer project's equivalent carries
|
||||
duration only. It is the intellectual content of the project and it is dense because the
|
||||
problem is.
|
||||
|
||||
`internal/verifier/redaction.go` in full, including the three-valued `secureFact` that treats
|
||||
an unreported target as a credential, which is the correct default. The
|
||||
`attributeAliases` and `selectorKeys` tables, `recordableValue`, `Node.scopedNodes`' spatial
|
||||
fallback, `innermostMatches`, and the `ActionWireContract` handshake mechanism, which is
|
||||
versioned precisely because the format already drifted once and every scroll silently
|
||||
travelled zero distance.
|
||||
|
||||
In the runner: `clampGestureToSafeArea`, `structuralShape`, the transitional-skip and action
|
||||
hold-back at `runner.go:254-356`, `confirmFocus` and `otherElementHoldsFocus`, the
|
||||
extractor-override consistency checks at `:309-324`, `fetchSyncedState`'s unchanged-JSON
|
||||
early exit, `resolveIdleTimeout`, `applyBound`, `runOutcome`'s three error types, and all of
|
||||
`internal/seedspec`, `internal/llmclient` and `internal/replay/watcher.go`.
|
||||
|
||||
In the drivers, roughly 60 items flagged as battle scars: the 354 to 616 pixel fling range,
|
||||
the 136ms Compose accessibility lag, the four-minute XCTest wedge, and the rest of the
|
||||
measured device quirks. Chrome and iOS are dense because those platforms are hostile. The
|
||||
optional capability interfaces are the right pattern for heterogeneous targets and should
|
||||
survive the narrowing of the mandatory set.
|
||||
|
||||
In the tests: the cross-host golden tests, `parity_test.go`, `host_parity_test.go`,
|
||||
`selector_keys_test.go`, `extractor_encoding_test.go` and `policy_parity_test.go`. They
|
||||
assert wire bytes against committed goldens, they name the bug class, and several clearly
|
||||
went red once.
|
||||
|
||||
## Order of work
|
||||
|
||||
Correctness first, because these make the tool lie: the `precondition_failures` exclusion in
|
||||
the analysis pipeline, the three fabricated iOS health responses, the partial read in the
|
||||
replay server, the selector vocabulary drift in the replay UI, the swallowed CDP error, and
|
||||
wiring the 541 lines of tag-gated tests into CI.
|
||||
|
||||
Then the removals that carry no behaviour risk: move the research tooling out, regenerate the
|
||||
protocol from the 14 messages actually used, delete the dead `Snapshots` panel and `Timeline`,
|
||||
delete `VERB_SUPPORT` and its unreachable tail, merge the two asset packages, collapse the
|
||||
twin sweep binaries.
|
||||
|
||||
Then the structural changes, each of which needs its own branch and its own green CI: one
|
||||
producer of DOM facts, a 12-method mandatory driver interface, `ltl.Formula` stored directly
|
||||
in the verifier handle, `encoding/json` in place of the hand-rolled encoder, and `runner.go`
|
||||
split along its five existing seams.
|
||||
|
||||
The test suite comes last and is a consolidation, not a purge: one per-call failure hook on
|
||||
the mock, one `traceStepLine`, nine table collapses in hierarchy, and goldens where two
|
||||
encoders currently only agree with each other.
|
||||
|
||||
Doing all of it puts the tool at roughly 60,700 lines with a further 18,400 in a separate
|
||||
research module. That is 2.4 times the peer project's own tool, which is earned by two mobile
|
||||
targets it does not have, an LLM policy it does not have, two fixture applications, and a test
|
||||
ratio deliberately kept above its 0.35.
|
||||
|
||||
## Second pass: how the tests are built, not what they assert
|
||||
|
||||
The first pass measured whether tests could be deleted without losing coverage and answered
|
||||
"mostly no, about 5 to 10 percent". That answered the wrong question. Deletability and
|
||||
engineering quality are independent axes, and on the second axis the suite fails its own
|
||||
standard comprehensively. The rule says tests are first-class code held to the same or
|
||||
higher standard than the code they cover. Measured against source, they are held to a
|
||||
visibly lower one.
|
||||
|
||||
Go, 135 test files, 41,988 lines, 1,259 test functions:
|
||||
|
||||
| measure | value |
|
||||
|---|---|
|
||||
| test-only helpers | 457, totalling 4,560 lines |
|
||||
| of those, pure duplication | about 1,860 lines, 41 percent of the helper layer |
|
||||
| test-only types | 67 top-level plus 20 function-local |
|
||||
| `t.Run` subtests across 1,281 test functions | 108 |
|
||||
| `t.Parallel()` anywhere in the repository | 0 |
|
||||
| `cmp.Diff` / testify / `reflect.DeepEqual` | 0 / 0 / 11 |
|
||||
| hand-rolled comparison assertions | about 97.8 percent |
|
||||
| clusters that should be tables | 51, covering 355 tests and 5,758 lines |
|
||||
|
||||
The specific duplications, each verified: five spec bundlers differing only in an alias map
|
||||
(`verifier_test.go:42`, `setup_action_test.go:18`, `spec_integration_test.go:16`,
|
||||
`action_encoding_test.go:36`, and `internal/runner/runner_test.go:131`, of which the first
|
||||
and last are character-identical apart from the name; the last is in a different package, so
|
||||
four of the five sit in `internal/verifier`); `traceLine` redeclared 14 times in
|
||||
`internal/runner` while `readTraceLines` and `traceStepLine` sit at `runner_test.go:1586-1595`
|
||||
and are a superset of nearly all of them; 27 wrapper driver types in one package, of which
|
||||
`snapshotFailFirst:1925`, `tapSelectorFailFirst:1998` and `internalApplyErrorFailFirst:2087`
|
||||
are the same seven-line method with a different error constant, while `mock.go:64` already
|
||||
has `Failures map[ActionKind]error` and needs only a call-count field; 43 byte-identical
|
||||
eight-line Chrome launch preambles across four files, 344 lines; 48 identical
|
||||
`context.WithTimeout` plus `Run(ctx, Options{})` blocks, 576 lines, beside a `harness` struct
|
||||
at `runner_test.go:78` that has four fields and no methods; and 347 identical lines shared
|
||||
between `corpus-sweep/end_to_end_test.go` and `implementation-sweep/end_to_end_test.go`,
|
||||
including six byte-identical helpers.
|
||||
|
||||
A shared `internal/testsupport` package holding file and run-directory writers, one trace
|
||||
reader over one wide step type, a tree builder, a Chrome page helper, and the spec bundler
|
||||
removes about 1,860 lines without touching a single assertion. Collapsing the 51 table
|
||||
clusters removes about 1,700 more. Roughly 3,560 lines in Go, with the suite asserting
|
||||
strictly more than it does now, because the next test becomes cheap instead of becoming
|
||||
another copy.
|
||||
|
||||
The same shape holds in the other two languages. In Kotlin, `StubDriverBackend` is declared
|
||||
in production source at `sidecar/src/main/kotlin/dev/sanderling/sidecar/DriverBackend.kt:507`
|
||||
and wired as a live backend fallback at `Main.kt:93`, carrying an injectable `commandRunner`
|
||||
seam and five `@Volatile` spy fields that ship in the production jar. Eight test files import
|
||||
it as their fake. The suite has no fake of its own because the fake was moved into `main`.
|
||||
Alongside that, `sidecar/build.gradle.kts:56-62` declares kotlin.test, JUnit 4.13.2, JUnit
|
||||
Jupiter 5.11.3 and the vintage engine to run 4 under 5, with 12 files importing
|
||||
`org.junit.Test` against 3 importing `kotlin.test.Test`. `countRouteScreens` is tested from
|
||||
two files with two separately invented tree builders, and 15 sidecar test files target two
|
||||
production files and should be about 7.
|
||||
|
||||
In TypeScript, `.each` is used zero times while 26 files hand-roll `for (const x of [...])`,
|
||||
and 61 percent of the 450 cases carry exactly one assertion.
|
||||
`folio-submit-balance-predicate.test.ts` is 625 lines over 40 cases, all driving
|
||||
`const submitOn = "testTag:LedgerScreen > testTag:TxnSubmit"` at line 9. `TxnSubmit` exists
|
||||
in exactly one place in the application, `AddTransactionScreen.kt:88`, and `LedgerScreen` does
|
||||
not contain it, so that selector cannot match anything real. Its sibling
|
||||
`folio-submit-window.test.ts:11` declares the same constant correctly. A duplicated fixture
|
||||
constant, copied rather than imported, edited to something fictional, with 625 lines built on
|
||||
top, all green. The predicates under test are pure, so the arithmetic is still exercised; the
|
||||
failure is that the fixture claims to model the application and does not.
|
||||
|
||||
Inline fixture bloat, which the first pass expected to find, is not a real problem in any of
|
||||
the three languages. Kotlin has zero literals over 30 lines. TypeScript has zero standalone
|
||||
data literals over 30 lines. Go has three raw strings over 30 lines totalling 120. The
|
||||
committed `testdata` corpus is 88 files, 264 KB, 3,497 lines, and is the best-managed part of
|
||||
the suite: `conformance/testdata` is 11 gate fixtures regenerated by a committed
|
||||
`generate.py`, and `.github/scripts/testdata` is six captured golden runs consumed by CI.
|
||||
Near-duplicates across all of it total about 90 lines. The growth is in test code, not test
|
||||
data. What Go does have is 395 hand-written hierarchy JSON nodes across 28 files with no
|
||||
shared builder, and `testdata/` in only 6 of 33 packages;
|
||||
`internal/hierarchy/hierarchy_test.go` is 1,823 lines with the largest inline corpus in the
|
||||
tree and no `testdata/` directory at all, while eight other packages already have one.
|
||||
|
||||
## Second pass: the growth pattern
|
||||
|
||||
Across the last 50 commits, test lines added 51,444 against 3,891 deleted, a ratio of 13.2 to
|
||||
1. Source over the same window is 5.4 to 1. Tests are removed at roughly a third the relative
|
||||
rate of source.
|
||||
|
||||
In the last 30 commits, **43 test files were added into a package that already had a file
|
||||
testing the same unit**. `internal/runner` took 14 of them alongside the existing 3,181-line
|
||||
`runner_test.go`, and they carry the duplication that decision implies: five near-identical
|
||||
web-target fakes each redeclaring the same five methods, and five hand-rolled trace scanners
|
||||
each doing `bufio.NewScanner` with an 8 MB buffer beside the existing `readTraceLines`.
|
||||
`internal/driver/chrome` took four, carrying four byte-identical probe installers, four
|
||||
matching probe entry files and four fixture pages. `test/browser` has four spellings of one
|
||||
harness: `runFixture`, `executeFixture`, `launchConsoleFixture`, and `servePage` plus
|
||||
`runBinary`. `pkg/spec/test` took eight folio files against three that existed, so
|
||||
`countSubmitsInWindow` is now tested from five files and `readHomeCards` from four, each
|
||||
redeclaring its own builders.
|
||||
|
||||
Only three commits in the last 60 have a negative test delta, and all three are subsystem
|
||||
removals: the in-app SDK, the JVM iOS backend, and a rename. Exactly one commit in 60 deleted
|
||||
test content because the tests were bad, `faebfe3`, which cut
|
||||
`examples/folio/sanderling/spec.ts` by 37 added against 122 deleted, removing properties that
|
||||
could not fail.
|
||||
|
||||
The commit named `test: full test-suite refactor sweep (#61)` is the clearest single case. It
|
||||
is net plus 2,674 lines. Its body carries 34 bullets of which one is a deletion. Of its 125
|
||||
deleted test lines, one is a genuine rewrite, `internal/ios/ios_test.go` at minus 53, beside
|
||||
a real extraction of pure simctl parsers; the remaining seven files give up 8 lines or fewer
|
||||
each. It also added eight new test files. A refactor that deletes nothing has not refactored
|
||||
anything.
|
||||
|
||||
Two counterexamples belong on the record, because the discipline has been demonstrated here
|
||||
and simply not repeated: `#52` rewrote `api.test.ts` at plus 125 against minus 134 and
|
||||
`defaults.test.ts` at plus 23 against minus 93, both in place, both deleting more than they
|
||||
added. `cmd/internal-tools/analyze` is properly layered, with one `writeCampaign` and five
|
||||
thin scenario wrappers delegating to it. The pattern was available in this repository the
|
||||
whole time.
|
||||
|
||||
One nuance argues against a naive purge. `internal/driver/chrome/fact_parity_test.go`
|
||||
supersedes four tests in `driver_test.go`, but parity asserts that the two fact producers
|
||||
agree, not that either is right, so those four still pin ground truth. The correct move was
|
||||
to fold them into the parity fixture. Neither folding nor deletion happened, and
|
||||
`element_state_test.go` then repeated the whole pattern a third time on a fourth fixture page.
|
||||
|
||||
## Second pass: compatibility code in an alpha project
|
||||
|
||||
`README.md:25` states the project is alpha. Genuinely deletable compatibility machinery is
|
||||
454 source lines and 558 test lines, rising to 493 and 632 once a stale trace corpus goes.
|
||||
|
||||
The largest single item was reported as the `action-wire/2` revision check,
|
||||
`internal/verifier/marshal.go:598` plus four files, on the grounds that it never reads
|
||||
revision 1 and so buys only a better error message. **That reading was wrong and the entry is
|
||||
withdrawn.** Refusing a pairing is not supporting it: the check exists because
|
||||
`@sanderling/spec` 0.0.3 and earlier serialized an authored `Scroll` with the container's own
|
||||
point as both endpoints, and this binary reads pre-computed endpoints as authoritative, so
|
||||
every such scroll dispatched successfully as a 250ms press and hold that travelled zero
|
||||
distance and no run reported it. Without the handshake the two halves run to completion and
|
||||
the campaign is void. It stays, and this pull request finishes it. The first item is `SANDERLING_SIMULATOR_COMPANION=legacy`, gating about 330
|
||||
lines and appearing in no document, Makefile or CI configuration. Third,
|
||||
`cmd/internal-tools/analyze/load.go:43` carries a `duration_millis` shim for campaign records
|
||||
written before two clocks were split: across the committed corpus, 0 of 308 campaign records
|
||||
carry `duration_millis` and 308 of 308 carry `monotonic_millis`, so it defends against
|
||||
nothing. `load.go:55` makes `Actions` a pointer to refuse older files whose field is present
|
||||
on 308 of 308 records.
|
||||
|
||||
The more important finding is the opposite of the brief. **Four comments label current,
|
||||
documented, dominant behaviour as legacy or backward-compatible**, and acting on them deletes
|
||||
live surface: `internal/verifier/bindings.go:217`, `internal/driver/chrome/translate.go:16`,
|
||||
`internal/hierarchy/hierarchy.go:8` and `cmd/sanderling/doctor.go:27`. A fifth,
|
||||
`cmd/internal-tools/analyze/load.go:62`, was listed and does not belong: it explains why
|
||||
`UnattributedActions` is a pointer, which is a live refusal two tests assert, and it never
|
||||
calls anything legacy. All four of the real ones are fixed. The clearest is `bindAlways`, whose comment calls a
|
||||
predicate function the "legacy shape" and a formula handle the "new shape". **This was first
|
||||
recorded as the predicate shape being used 8 times and the formula shape 0, which is wrong in
|
||||
both directions.** Counted against the tree: all six properties in `examples/folio/sanderling/spec.ts`
|
||||
and `examples/folio-web/sanderling/spec.ts` pass a formula, the two shipped default properties
|
||||
in `pkg/spec/src/defaults/properties.ts` pass a predicate, and `pkg/spec/test/api.test.ts:189`
|
||||
passes a predicate. Both shapes are current and both have real callers, so neither is legacy
|
||||
and the comment was wrong to rank them at all.
|
||||
|
||||
Ordering matters for the trace-format items. 342 of 698 committed `trace.jsonl` files carry
|
||||
no `trace_version` and no `depths`, and the gates currently refuse them. Deleting the version
|
||||
machinery first turns those refusals into nil-root trees that resolve no selectors, which is
|
||||
silent wrong data. Delete the stale corpus first, then the code.
|
||||
|
||||
## Second pass: comments, classified
|
||||
|
||||
1,565 comment blocks were classified against the code beside them, 95 percent of every
|
||||
comment block in non-test source.
|
||||
|
||||
| bucket | blocks | share |
|
||||
|---|---|---|
|
||||
| godoc on an exported identifier, correct | 540 | 34.5% |
|
||||
| why, carrying a fact not recoverable from the code | 751 | 48.0% |
|
||||
| restates the code | 123 | 7.9% |
|
||||
| compensating for a bad name | 8 | 0.5% |
|
||||
| compensating for a function that is too long | 9 | 0.6% |
|
||||
| design essay, notebook, or history | 56 | 3.6% |
|
||||
| stale or false | 78 | 5.0% |
|
||||
|
||||
The volume complaint does not survive this. Four blocks in five are godoc or a
|
||||
non-recoverable fact, which is what Go convention asks for, and the 20.8 percent line-share
|
||||
figure overstates the problem. About 1,332 of 6,458 comment lines, 21 percent, should be
|
||||
deleted or refactored away. `internal/driver/chrome` is the strongest package in the tree, 68
|
||||
of its 123 blocks being measured browser quirks with no bad-name, too-long or essay findings
|
||||
at all. `examples/folio/sanderling/predicates.ts` at 61.7 percent comment is the worst.
|
||||
|
||||
The real defect is falsity, and several of the 78 are load-bearing:
|
||||
|
||||
`internal/hierarchy/hierarchy.go:9` documents `id:<suffix>` as a substring match on
|
||||
resource-id. The code at `:412-414` is `ResourceID == value` or `HasSuffix(ResourceID, ":id/"+value)`,
|
||||
so `id:Button` does not match `com.x:id/saveButton`. `desc:` at `:417` is worse than reported:
|
||||
it is exact-or-prefix-before `", "`, not a substring match at all. This is the package doc a
|
||||
spec author reads first, and `docs/manual/spec-language.md:60,63` has it right, so the package
|
||||
doc was the only wrong copy. Its "(backward compat)" label on `id:` was wrong too: `id:`
|
||||
appears about 375 times across the tree against 16 for `idPrefix:`, so the form marked legacy
|
||||
outnumbers the other by 23 to 1.
|
||||
|
||||
`hierarchy.go:43` declares `Bounds` an inclusive rectangle while `Width()` returns
|
||||
`Right - Left`, so every width, height and centre in the tool was off by one against its own
|
||||
stated semantics. Settled against the producers: all three emit exclusive right and bottom
|
||||
(`sidecar/.../DriverBackend.kt:1624-1626` states it outright, `chrome/driver.go:696` writes
|
||||
`getBoundingClientRect`'s `right` and `bottom`, `ioscompanion/hierarchymap.go:190` writes
|
||||
`left + width`), and `runner.go` reads `screen.Width()` as the viewport width, which only
|
||||
holds under the exclusive reading. The code was right, the comment was the only wrong copy,
|
||||
and a test now pins the arithmetic on the abutting and one-pixel cases the two conventions
|
||||
disagree on. Separately, the two `parseBounds` pattern comments named the wrong producers in
|
||||
both directions: the paired `[x1,y1][x2,y2]` form is what both device backends emit, and the
|
||||
flat one is chrome and the sidecar stub.
|
||||
|
||||
`internal/driver/driver.go` was documented from the Android sidecar and never revisited for
|
||||
Chrome, producing six false statements on the interface every driver implements: `:52` says
|
||||
an empty `minLevel` defaults to "E" while `chrome/driver.go:823` returns every level; `:47`
|
||||
says `Snapshot` runs under a backend-side mutex while `chrome/driver.go:802-812` takes no
|
||||
lock, and that claim has already been copied into `runner.go:1430`; `:58` calls `HeapBytes`
|
||||
RSS while Chrome feeds it `usedJSHeapSize`; `:101` says the gate prefers `FocusedWindowChecker`
|
||||
while `runner.go:836` requires `ForegroundChecker`; `:175` names a `WebAction` type that
|
||||
exists nowhere.
|
||||
|
||||
iOS stopped routing through the JVM sidecar and 11 comments still say it does, including
|
||||
three that name an `IosDriverBackend` type which does not exist. Eleven TypeScript comments
|
||||
are anchored to Go symbols that were deleted. `verifier/types.go:22,26,29` deny Scroll its
|
||||
endpoints and duration and deny DoubleTap and LongPress their coordinates. `marshal.go:95`
|
||||
and `runner.go:1289` now state opposite rules for the same selector-and-coordinate conflict.
|
||||
`DriverBackend.kt:141` documents `pollUntilStable` as not charging reads to the stability
|
||||
streak, but `now` is sampled after `snapshot()` returns, so the code permitted exactly the
|
||||
failure its own worked example presents as impossible, in the settle primitive every Android
|
||||
step depends on. **This one turned out to be a code defect, not a comment defect.** On the
|
||||
comment's own numbers (streak 750ms, interval 250ms, read 500ms) the poll returned having
|
||||
watched 250ms of quiet. The comment arrived in `b02e86b` with the streak parameters and the
|
||||
`now`-after-`snapshot` sampling was left as `88db965` wrote it. Fixed by sampling the read's
|
||||
start; the existing test measured from the end of the first read, so it included an interval
|
||||
plus a whole read and passed against the bug at any read length. `runner.go:95` says the caller is responsible for launching the app, and
|
||||
`Run` launches it at `:913`.
|
||||
|
||||
The bad-name and too-long buckets are small but they are the ones where the comment is a
|
||||
symptom rather than the disease. `runner.Run` at 473 lines carries five multi-paragraph
|
||||
signposts, 62 lines, that exist only because there is no function boundary; extracting
|
||||
`observeDevice`, `verifyStep`, `applyChosenAction`, `recordStep` and `settleAfterStep` deletes
|
||||
all five. Four foreground readers with four failure policies carry 40 doc lines between them,
|
||||
one of which is a 10-line comment over a single delegating line. `held := skippedVerification`
|
||||
at `runner.go:352` is a pure alias that exists so that a 15-line comment has somewhere to
|
||||
live. In `ioscompanion`, a type named `runner` collides with the XCUITest runner, the
|
||||
xcodebuild session and `internal/runner`, which is why `driver.go:1211` has to write "the
|
||||
text runner"; renaming it `textInputBackend` retires the comment.
|
||||
Reference in new issue
Block a user