Files
sanderling/docs/development/code-audit.md
pj ac0ec68cb8 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.
2026-08-22 21:46:28 +05:30

824 lines
55 KiB
Markdown

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