a73x/eitri
Watch
1
0
Fork
You've already forked eitri
 0
Code Issues 52 Pull requests Projects Releases 1 Packages Wiki Activity Actions

Eight tests that pass for the wrong reason (and three self-referential assertions) #98

New issue
Closed
opened 2026-08-23 09:37:10 +00:00 by a73x · 3 comments
a73x commented 2026-08-23 09:37:10 +00:00 (Migrated from git.a73x.sh)
Copy link

[claude 2026-08-23] Found by mutation-auditing comments across six packages (branch comment-audit). These tests pass when the behaviour their name promises is removed. Each was verified by applying the mutation and observing a PASS.

test mutation that passes what it actually asserts
TestRedeemWritesAuditRowAtomically move the audit write to after tx.Commit() that the row exists on the success path
TestBumpCredGenerationAuditsAtomically same same
TestOpenTCPBridgesBytes delete either st.SetDeadline — the set or the clear sleeps 50 ms against a 10 s handshake deadline
TestShutdownFallsBackToSigtermWhenVfkitRefuses return nil without signalling anything only NoError
TestUploadUserCAStoresCanonicalLine store req.PublicKey verbatim the posted line is already canonical
TestSnapshotHubPerTenantPayloads (see security issue) only the initial subscribe() snapshot, never the fan-out it names
TestApplyReportSkipsUnchangedLeases remove the skip from netTracker.writeThrough inspects netIPTrack.last directly, bypassing the dedup
TestServeNotEnrolled — locks a StateDir that is not the store's, so the lock never guards what it protects
TestRemoveHostRefusesWhileVMsRemain if false on the in-transaction count check only assert.Error — the FK constraint errors anyway, so the alarm rings for the wrong reason

Self-referential assertions

Three constants are asserted in terms of themselves, so they pin the shape and not the number:

  • maxKeyCommentLen — len(comment) <= maxKeyCommentLen passes at 128 or at 4096.
  • enrollRefillEvery and maxLimiterEntries in ratelimit_test.go — same pattern.

Why this matters beyond the tests

Two of these (…Atomically) are the only evidence that a security action cannot happen unrecorded. The names promise more than the assertions deliver, which is worse than no test: a reader greps for the guarantee, finds a test named for it, and stops looking.

Migrated from git-collab issue d30269d26462ba7edbd6ce46cb1783707327c346.

Original author: a73x dev@a73x.sh.

Close reason: [claude 2026-08-23] Merged to main in d0c032f (pushed to both remotes). Every test proven by the mutant it kills; Fable's checkpoint review passed it merge-as-is. make ci green, boot gate PASS (full smoke incl. UDP, cold_start=20s).

[claude 2026-08-23] Found by mutation-auditing comments across six packages (branch `comment-audit`). These tests pass when the behaviour their name promises is removed. Each was verified by applying the mutation and observing a PASS. | test | mutation that passes | what it actually asserts | |---|---|---| | `TestRedeemWritesAuditRowAtomically` | move the audit write to **after** `tx.Commit()` | that the row exists on the success path | | `TestBumpCredGenerationAuditsAtomically` | same | same | | `TestOpenTCPBridgesBytes` | delete **either** `st.SetDeadline` — the set or the clear | sleeps 50 ms against a 10 s handshake deadline | | `TestShutdownFallsBackToSigtermWhenVfkitRefuses` | return `nil` without signalling anything | only `NoError` | | `TestUploadUserCAStoresCanonicalLine` | store `req.PublicKey` verbatim | the posted line is already canonical | | `TestSnapshotHubPerTenantPayloads` | (see security issue) | only the initial `subscribe()` snapshot, never the fan-out it names | | `TestApplyReportSkipsUnchangedLeases` | remove the skip from `netTracker.writeThrough` | inspects `netIPTrack.last` directly, bypassing the dedup | | `TestServeNotEnrolled` | — | locks a `StateDir` that is not the store's, so the lock never guards what it protects | | `TestRemoveHostRefusesWhileVMsRemain` | `if false` on the in-transaction count check | only `assert.Error` — the FK constraint errors anyway, so the alarm rings for the wrong reason | ### Self-referential assertions Three constants are asserted **in terms of themselves**, so they pin the shape and not the number: - `maxKeyCommentLen` — `len(comment) <= maxKeyCommentLen` passes at 128 or at 4096. - `enrollRefillEvery` and `maxLimiterEntries` in `ratelimit_test.go` — same pattern. ### Why this matters beyond the tests Two of these (`…Atomically`) are the *only* evidence that a security action cannot happen unrecorded. The names promise more than the assertions deliver, which is worse than no test: a reader greps for the guarantee, finds a test named for it, and stops looking. --- Migrated from git-collab issue `d30269d26462ba7edbd6ce46cb1783707327c346`. Original author: a73x <dev@a73x.sh>. Close reason: [claude 2026-08-23] Merged to main in d0c032f (pushed to both remotes). Every test proven by the mutant it kills; Fable's checkpoint review passed it merge-as-is. make ci green, boot gate PASS (full smoke incl. UDP, cold_start=20s).
a73x commented 2026-08-23 09:51:14 +00:00 (Migrated from git.a73x.sh)
Copy link

[claude 2026-08-23] Design review of all twelve findings. Every mutation was re-applied on a clean tree (main, c694896 — identical to comment-audit modulo comments), the named test run, the result observed, and the tree restored (git diff --quiet confirmed after each). Repair designs only — no repairs implemented.

Verdict table

# finding verdict evidence
1 TestRedeemWritesAuditRowAtomically CONFIRMED audit insert moved to after tx.Commit() (plain s.db.Exec): PASS
2 TestBumpCredGenerationAuditsAtomically CONFIRMED same mutation: PASS
3 TestSnapshotHubPerTenantPayloads CONFIRMED, worse than filed fan-out mutated to broadcast one tenant's bytes to every subscriber: the named test and every other TestSnapshotHub* test pass. Tenant isolation on the recompute path is pinned by nothing.
4 TestServeNotEnrolled CONFIRMED dir mismatch verified by inspection (store opens t.TempDir() A, Config.StateDir is t.TempDir() B); additionally, deleting statelock.Acquire from serve entirely: PASS
5 TestRemoveHostRefusesWhileVMsRemain CONFIRMED if false && on the drain count: PASS — the refusal comes from the vms.host_id REFERENCES hosts FK (foreign_keys(1) in the DSN), not from the check the test names
6 TestShutdownFallsBackToSigtermWhenVfkitRefuses CONFIRMED sigterm body replaced with return nil: PASS
7 TestUploadUserCAStoresCanonicalLine CONFIRMED line := req.PublicKey (verbatim store): PASS — the test posts a line that is already canonical
8 TestApplyReportSkipsUnchangedLeases CONFIRMED dedup if t.last[hostID] == value { return nil } deleted from netTracker.writeThrough: PASS — the test inspects netIPTrack.last, which holds the same value with or without the skip
9 TestOpenTCPBridgesBytes CONFIRMED each SetDeadline deleted in turn (the 10s set, then the clear): PASS both times. The name is honest — bytes-bridging is genuinely proven; the lie is the doc comment's deadline-clear claim.
10 maxKeyCommentLen self-reference PARTIALLY RETIRED value drift 128→4096: PASS (confirmed). But deleting the truncation outright: FAIL — the behaviour is pinned, only the value floats. Weakest of the three.
11 enrollRefillEvery self-reference CONFIRMED 30s→30ms: all limiter tests PASS
12 maxLimiterEntries self-reference CONFIRMED 10 000→10: all limiter tests PASS

Repairs, ordered by risk (worst outcome first)

1. TestSnapshotHubPerTenantPayloads — cross-tenant leak, nothing catches it

Risk: a regression in the recompute fan-out sends tenant A's hosts/VMs/metrics to tenant B's SSE connection. Multi-tenant confidentiality break, and the whole hub suite stays green (verified). Worst outcome on the list.

Repair: stronger assertion in the existing test (extend, don't restructure — the initial-snapshot half is correct and should stay). The test already builds tenant-echoing payloads; it just never drives a recompute:

  • Keep the newNotifier() handle instead of discarding it.
  • Make build versioned (payload-for-<tenant>-v<n>), so the second delivery is distinguishable from the first.
  • After draining the initial snapshots, notif.notify(), then assert chA receives payload-for-alpha-v2 and chB payload-for-beta-v2 — assert.Equal on exact bytes already forbids receiving the other tenant's payload, which is precisely what kills the broadcast mutant I ran.
  • One extra guard: after the fan-out round, assert neither channel has a second pending message (non-blocking recv must be empty), so "right bytes AND nothing else" is pinned.

No seam needed; notifier is already injectable.

2–3. TestRedeemWritesAuditRowAtomically / TestBumpCredGenerationAuditsAtomically — security actions unrecorded

Risk: the only evidence that an enrollment or a credential revoke cannot commit without its audit row. If the write drifted out of the transaction, hosts could enroll unrecorded — invisible until an incident needs the log.

The seam question. Proving "inside the transaction" needs a fault injected between the audit write and the commit, and the store has no Go-level seam for that (*sql.DB is concrete, no exec hook). But it doesn't need one: store_test.go is an internal test package with white-box access to s.db, and SQLite gives fault injection for free:

CREATE TRIGGER fail_audit BEFORE INSERT ON audit_log
BEGIN SELECT RAISE(ABORT, 'injected audit failure'); END

I validated this end-to-end with a throwaway test (deleted after): with the trigger installed, RedeemEnrollmentToken errors, zero host rows exist, and the token is not consumed — a second redeem after DROP TRIGGER succeeds. Same shape for BumpCredGeneration: errors, and cred_generation stays 1. I then re-applied the move-after-commit mutation with this test in place: FAIL ("no host may exist without its audit row") — the design kills the mutant. No harness change required.

Repair: new tests alongside, existing ones renamed. The existing tests are good success-path tests (row content, and the valuable raw token must never reach the audit log assertion) — keep them as TestRedeemWritesAuditRow / TestBumpCredGenerationAudits. The atomicity claim moves to new fault-injection tests that own the name: TestRedeemRefusesToEnrollUnaudited / TestBumpCredGenerationRefusesUnaudited (or keep the …Atomically names on the new tests). Each: install trigger → call → require error → assert no host / generation unchanged → drop trigger → assert the operation still works (token unconsumed / bump lands), proving rollback was complete, not just refusal.

4. TestServeNotEnrolled — two agents on one identity

Risk: serve running without the state lock (or acquiring it after touching state) allows a second agent on the same StateDir: two hypervisor managers driving the same VM records and disks. Deleting Acquire entirely passes today's suite.

Repair: restructure + one new test.

  • Fix the harness bug: one dir, passed to both state.Open and Config.StateDir — the test currently constructs a pairing production (RunCLI) can never produce.
  • New test TestServeRefusesASecondAgentOnTheSameState: statelock.Acquire(dir) in the test first, then serve(st, Config{StateDir: dir}) on that same dir must error with "already running" — and not "not enrolled", which pins the ordering promise in serve's own comment (lock before anything else). assert.ErrorContains(t, err, "already running") kills both the delete-Acquire mutant and any reorder past the identity check. statelock has its own tests; what's unpinned is that serve calls it, on the right dir, first.

5. TestRemoveHostRefusesWhileVMsRemain — alarm rings for the wrong reason

Risk: latent rather than live — the FK currently refuses too. But the FK is one schema edit away from not being the same guarantee (an ON DELETE CASCADE added for another reason would turn "refuse" into "silently delete the host and its VM rows"), and the curated error ("still has N VM(s); not drained") is part of the API surface the console shows an operator.

Repair: stronger assertion, two sites. assert.ErrorContains(t, err, "not drained") in this test and in both refusal branches of TestRemoveHostDrainSequence (same weakness, found in passing — it uses bare assert.Error twice). One-line changes; kills the if false mutant because the FK's error text ("FOREIGN KEY constraint failed") doesn't match.

6. TestShutdownFallsBackToSigtermWhenVfkitRefuses — guest never stops

Risk: an unhealthy vfkit means Shutdown quietly does nothing; the VM runs until Destroy. Operational, single-host blast radius.

Repair: stronger assertion via the existing seam. p.signal is already an injectable func field used by five sibling tests. Split the test's two claims:

  • Fallback happens: inject a recorder, write an owned pidfile (bare pid is fine — pidfile.Owned accepts a missing boot-ID line, verified), assert exactly one SIGTERM to that pid after the 500 response.
  • Dead process is success: inject return syscall.ESRCH, assert NoError — this keeps the current test's actual claim under an honest name (…TreatsAGoneProcessAsStopped). Today's pid 2147483647 reaches the real syscall.Kill and gets ESRCH, so the tolerance path is exercised — but only NoError is observed, which is why the no-op mutant survives.

7. TestUploadUserCAStoresCanonicalLine — CA lookup by line breaks

Risk: verbatim storage makes TenantForUserCA (exact line match) miss whenever the presented form differs from the uploaded form — trailing newline, comment, whitespace. Confusing auth failures and duplicate CA rows rather than a break-in.

Repair: stronger input in the existing test. Post a deliberately non-canonical line (trailing "\n", a user@host comment, leading spaces) and assert TenantForUserCA(sshca.AuthorizedKeyLine(pub)) — lookup by the canonical form — resolves. Storing verbatim then fails the lookup, killing the mutant. Optionally also assert the listed Pubkey equals the canonical line exactly.

8. TestOpenTCPBridgesBytes — deadline lifecycle unpinned

Risk: clear-missing: every TCP/console session dies 10s after handshake (loud, user-visible, quickly found). Set-missing: a wedged agent pins the WS handler goroutine forever (quiet leak). Neither catastrophic, both worth pinning.

Repair: harness change + one new test; existing test keeps its (honest) name. A 10s constant can't be unit-tested; promote consoleHandshakeTimeout to a field on Service (defaulted in New, overridden by tests — same pattern as p.signal). Then:

  • New TestOpenStreamAbandonsAWedgedAgent: timeout 100ms, fake agent accepts the stream and never replies; OpenTCP must return an error promptly. Kills the set-deleted mutant.
  • In TestOpenTCPBridgesBytes: timeout ~200ms, keep the existing 50ms... make it a post-handshake sleep longer than the injected timeout (e.g. 300ms), then the existing write/read round-trip. Kills the clear-deleted mutant — today's 50ms-vs-10s gap is what makes both mutants survivable. Fix the doc comment to say the clear is proven by outliving the (injected) handshake deadline.

9. TestApplyReportSkipsUnchangedLeases — extra writes per tick

Risk: perf only — one redundant UPDATE per bridged VM per tick on SQLite's single connection. Real (it's the P1 perf lane's territory) but the least dangerous regression here.

Repair: new unit test carries the claim; existing test renamed. The dedup lives in netTracker.writeThrough, whose write closure is the natural counting seam — and tracker_test.go already tests statusTracker.writeThrough exactly this way, but never netTracker. Add TestNetTrackerWritesOnlyOnChange: counting closure; same value twice → one write; new value → second write; failing write → error surfaced, cache NOT updated, retry writes again (the not-cached-on-failure branch is also unpinned today). The applyReport-level test can't see writes without instrumenting the store; rename it to what it proves — TestApplyReportTracksTheReportedLease (that applyReport routes leases through the cache at all) — and let the tracker test own the dedup name.

The self-referential constants — pattern verdict and repo sweep

Verdict: these are two different diseases, and the repair differs.

(a) Behaviour pinned, value floating — maxKeyCommentLen is this: deleting the truncation FAILS the test (verified); only 128→4096 sails through. The parameterized assertion is correct test construction — the boundary behaviour is what the test owns. What's missing is a value pin where the value itself is load-bearing.

(b) Pure tautology — the assertion copies the constant to both sides and proves only that an assignment happened. assert.Equal(t, httpServerReadHeaderTimeout, srv.ReadHeaderTimeout) is this: it passes at 0s with a message claiming "the Slowloris defense is on" (verified below).

Repair: keep the behaviour tests parameterized (they're right); for constants whose value is the security/operational content, add a per-package literal-pinning guard test — assert.Equal(t, 30*time.Second, enrollRefillEvery) with the rationale in the message — so drifting a value is a conscious two-file edit. This is the house pattern already ("things can't pin their own defaults" → docs-match guard tests). For tautologies, the literal pin replaces the tautology rather than joining it. Do not pin taste-only values (that's noise); the list below marks which is which. Renaming-only is the honest fix nowhere in this trio — the limiter values are genuinely load-bearing (30ms refill ≈ no limiter; 10-entry cap ≈ prune-thrash on 11 IPs).

Sweep results (assertions mentioning a prod constant on both sides, all packages)

Scripted sweep: extracted every constant from non-test files, flagged assert./require. lines in same-package tests referencing them. ~40 raw hits, triaged:

Tier 1 — tautologies or floating values where the value is the point (fix like the trio):

  • internal/server/boot/httpserver_test.go:18-19 — assert.Equal(t, httpServerReadHeaderTimeout, srv.ReadHeaderTimeout) (and Idle). Verified: both constants set to 0 → package passes. The v0.0.6 Slowloris hardening is pinned by nothing. Pin literals 20s/120s.
  • internal/server/boot/headers_test.go:45,49 — assert.Equal(t, hstsMaxAge, …Header.Get("Strict-Transport-Security")). Verified: max-age=0 → package passes. The header-presence half is real; the value needs a literal (or parse-and-assert max-age ≥ 1y).
  • internal/server/store/exposures_test.go:30,45-52,164-183 — six assertions all relative to MinAllocatedHostPort. Allocation ordering is well tested; the base value floats — at 80 the allocator would hand out well-known ports and everything passes. One literal pin (30000) covers all six.
  • internal/agent/vfkit/vfkit_test.go:142 — asserts the composed socket path < maxSocketPath, but maxSocketPath (103) stands in for the OS's sun_path limit, which is external truth: bump the constant and the test follows while real binds fail. Assert against literal 103 with the sun_path rationale.
  • internal/server/api/ratelimit_test.go — the filed pair (enrollRefillEvery, maxLimiterEntries), plus enrollBurst rides the same pattern in api_test.go:928 and ratelimit_test.go loops. One guard test pinning all three limiter values.

Tier 2 — behaviour pinned, value floats; pin only if the value is judged load-bearing:

  • internal/mcpserver/sshrun_test.go:226 — outputCap (1 MiB): truncation behaviour pinned exactly like maxKeyCommentLen; cap value floats (memory bound per exec — probably worth a pin).
  • internal/server/api/abandoned_vm_test.go:50,83 — abandonedVMReapGrace (15m) used as fake-clock input; grace behaviour pinned, window floats (1ms would reap freshly-created VMs).
  • internal/agent/netenv/named_test.go:490 — HasPrefix(NetTapName(…), netTapPrefix): prefix value floats; IFNAMSIZ budget depends on its length. Borderline.
  • internal/agent/seed/seed_test.go:372-392 — chrony drop-in paths asserted via the constants; the paths are guest-side contracts. Borderline.
  • internal/smoke/mcp_test.go:433 — delegationTTL is smoke's own request TTL asserted back from the cert it mints; low risk, smoke-local.
  • internal/server/api/injectedkey_test.go:47 — the filed maxKeyCommentLen; behaviour already pinned, value pin optional.

Retired from suspicion (correct constructions, no action):

  • internal/agent/syncclient/client_test.go:111 — OnlineWindow ≥ 3×DefaultTickInterval is a cross-package relationship assertion — this is the model to imitate, not a bug.
  • internal/smoke/mcp_contract_test.go:53 — remoteToolCount is smoke's independent copy of mcpserver's tool count (the deliberate tool-list tie); cross-package pin, good.
  • internal/server/sshgate/gate_test.go:570 — tenantExt is minted and checked in the same package; internal consistency is the contract.
  • transport (DefaultMaxFrame, ALPN), pidfile/statelock (mode, lockName) — constants as call inputs, not expectations.

Net: the trio is the visible edge of a repo-wide habit. Tier 1 is five sites (boot ×2, store exposures, vfkit socket-path, api limiter) where a value with a security or OS-truth rationale can drift to a useless setting with a green suite — the boot pair verified, the rest read. Suggest the guard-test convention lands as its own small issue so the pins arrive as one sweep, not nine drive-bys.

[claude 2026-08-23] Design review of all twelve findings. Every mutation was re-applied on a clean tree (main, c694896 — identical to `comment-audit` modulo comments), the named test run, the result observed, and the tree restored (`git diff --quiet` confirmed after each). Repair designs only — no repairs implemented. ## Verdict table | # | finding | verdict | evidence | |---|---|---|---| | 1 | `TestRedeemWritesAuditRowAtomically` | **CONFIRMED** | audit insert moved to after `tx.Commit()` (plain `s.db.Exec`): PASS | | 2 | `TestBumpCredGenerationAuditsAtomically` | **CONFIRMED** | same mutation: PASS | | 3 | `TestSnapshotHubPerTenantPayloads` | **CONFIRMED, worse than filed** | fan-out mutated to broadcast one tenant's bytes to every subscriber: the named test **and every other `TestSnapshotHub*` test** pass. Tenant isolation on the recompute path is pinned by nothing. | | 4 | `TestServeNotEnrolled` | **CONFIRMED** | dir mismatch verified by inspection (store opens `t.TempDir()` A, `Config.StateDir` is `t.TempDir()` B); additionally, deleting `statelock.Acquire` from `serve` entirely: PASS | | 5 | `TestRemoveHostRefusesWhileVMsRemain` | **CONFIRMED** | `if false &&` on the drain count: PASS — the refusal comes from the `vms.host_id REFERENCES hosts` FK (`foreign_keys(1)` in the DSN), not from the check the test names | | 6 | `TestShutdownFallsBackToSigtermWhenVfkitRefuses` | **CONFIRMED** | `sigterm` body replaced with `return nil`: PASS | | 7 | `TestUploadUserCAStoresCanonicalLine` | **CONFIRMED** | `line := req.PublicKey` (verbatim store): PASS — the test posts a line that is already canonical | | 8 | `TestApplyReportSkipsUnchangedLeases` | **CONFIRMED** | dedup `if t.last[hostID] == value { return nil }` deleted from `netTracker.writeThrough`: PASS — the test inspects `netIPTrack.last`, which holds the same value with or without the skip | | 9 | `TestOpenTCPBridgesBytes` | **CONFIRMED** | each `SetDeadline` deleted in turn (the 10s set, then the clear): PASS both times. The *name* is honest — bytes-bridging is genuinely proven; the lie is the doc comment's deadline-clear claim. | | 10 | `maxKeyCommentLen` self-reference | **PARTIALLY RETIRED** | value drift 128→4096: PASS (confirmed). But deleting the truncation outright: **FAIL** — the behaviour *is* pinned, only the value floats. Weakest of the three. | | 11 | `enrollRefillEvery` self-reference | **CONFIRMED** | 30s→30ms: all limiter tests PASS | | 12 | `maxLimiterEntries` self-reference | **CONFIRMED** | 10 000→10: all limiter tests PASS | --- ## Repairs, ordered by risk (worst outcome first) ### 1. `TestSnapshotHubPerTenantPayloads` — cross-tenant leak, nothing catches it **Risk:** a regression in the recompute fan-out sends tenant A's hosts/VMs/metrics to tenant B's SSE connection. Multi-tenant confidentiality break, and the whole hub suite stays green (verified). Worst outcome on the list. **Repair: stronger assertion in the existing test** (extend, don't restructure — the initial-snapshot half is correct and should stay). The test already builds tenant-echoing payloads; it just never drives a recompute: - Keep the `newNotifier()` handle instead of discarding it. - Make `build` versioned (`payload-for-<tenant>-v<n>`), so the second delivery is distinguishable from the first. - After draining the initial snapshots, `notif.notify()`, then assert `chA` receives `payload-for-alpha-v2` and `chB` `payload-for-beta-v2` — `assert.Equal` on exact bytes already forbids receiving the other tenant's payload, which is precisely what kills the broadcast mutant I ran. - One extra guard: after the fan-out round, assert neither channel has a *second* pending message (non-blocking recv must be empty), so "right bytes AND nothing else" is pinned. No seam needed; `notifier` is already injectable. ### 2–3. `TestRedeemWritesAuditRowAtomically` / `TestBumpCredGenerationAuditsAtomically` — security actions unrecorded **Risk:** the only evidence that an enrollment or a credential revoke cannot commit without its audit row. If the write drifted out of the transaction, hosts could enroll unrecorded — invisible until an incident needs the log. **The seam question.** Proving "inside the transaction" needs a fault injected between the audit write and the commit, and the store has **no Go-level seam** for that (`*sql.DB` is concrete, no exec hook). But it doesn't need one: `store_test.go` is an internal test package with white-box access to `s.db`, and SQLite gives fault injection for free: ```sql CREATE TRIGGER fail_audit BEFORE INSERT ON audit_log BEGIN SELECT RAISE(ABORT, 'injected audit failure'); END ``` I validated this end-to-end with a throwaway test (deleted after): with the trigger installed, `RedeemEnrollmentToken` errors, **zero host rows exist**, and the token is *not consumed* — a second redeem after `DROP TRIGGER` succeeds. Same shape for `BumpCredGeneration`: errors, and `cred_generation` stays 1. I then re-applied the move-after-commit mutation with this test in place: **FAIL** ("no host may exist without its audit row") — the design kills the mutant. No harness change required. **Repair: new tests alongside, existing ones renamed.** The existing tests are good success-path tests (row content, and the valuable `raw token must never reach the audit log` assertion) — keep them as `TestRedeemWritesAuditRow` / `TestBumpCredGenerationAudits`. The atomicity claim moves to new fault-injection tests that own the name: `TestRedeemRefusesToEnrollUnaudited` / `TestBumpCredGenerationRefusesUnaudited` (or keep the `…Atomically` names on the new tests). Each: install trigger → call → require error → assert no host / generation unchanged → drop trigger → assert the operation still works (token unconsumed / bump lands), proving rollback was complete, not just refusal. ### 4. `TestServeNotEnrolled` — two agents on one identity **Risk:** `serve` running without the state lock (or acquiring it after touching state) allows a second agent on the same StateDir: two hypervisor managers driving the same VM records and disks. Deleting `Acquire` entirely passes today's suite. **Repair: restructure + one new test.** - Fix the harness bug: one dir, passed to both `state.Open` and `Config.StateDir` — the test currently constructs a pairing production (`RunCLI`) can never produce. - New test `TestServeRefusesASecondAgentOnTheSameState`: `statelock.Acquire(dir)` in the test first, then `serve(st, Config{StateDir: dir})` on that same dir must error with "already running" — **and not "not enrolled"**, which pins the ordering promise in `serve`'s own comment (lock before anything else). `assert.ErrorContains(t, err, "already running")` kills both the delete-Acquire mutant and any reorder past the identity check. `statelock` has its own tests; what's unpinned is that `serve` calls it, on the right dir, first. ### 5. `TestRemoveHostRefusesWhileVMsRemain` — alarm rings for the wrong reason **Risk:** latent rather than live — the FK currently refuses too. But the FK is one schema edit away from not being the same guarantee (an `ON DELETE CASCADE` added for another reason would turn "refuse" into "silently delete the host *and its VM rows*"), and the curated error ("still has N VM(s); not drained") is part of the API surface the console shows an operator. **Repair: stronger assertion, two sites.** `assert.ErrorContains(t, err, "not drained")` in this test **and** in both refusal branches of `TestRemoveHostDrainSequence` (same weakness, found in passing — it uses bare `assert.Error` twice). One-line changes; kills the `if false` mutant because the FK's error text ("FOREIGN KEY constraint failed") doesn't match. ### 6. `TestShutdownFallsBackToSigtermWhenVfkitRefuses` — guest never stops **Risk:** an unhealthy vfkit means Shutdown quietly does nothing; the VM runs until Destroy. Operational, single-host blast radius. **Repair: stronger assertion via the existing seam.** `p.signal` is already an injectable func field used by five sibling tests. Split the test's two claims: - Fallback happens: inject a recorder, write an owned pidfile (bare pid is fine — `pidfile.Owned` accepts a missing boot-ID line, verified), assert exactly one `SIGTERM` to that pid after the 500 response. - Dead process is success: inject `return syscall.ESRCH`, assert `NoError` — this keeps the current test's actual claim under an honest name (`…TreatsAGoneProcessAsStopped`). Today's pid `2147483647` reaches the real `syscall.Kill` and gets ESRCH, so the tolerance path *is* exercised — but only NoError is observed, which is why the no-op mutant survives. ### 7. `TestUploadUserCAStoresCanonicalLine` — CA lookup by line breaks **Risk:** verbatim storage makes `TenantForUserCA` (exact line match) miss whenever the presented form differs from the uploaded form — trailing newline, comment, whitespace. Confusing auth failures and duplicate CA rows rather than a break-in. **Repair: stronger input in the existing test.** Post a deliberately non-canonical line (trailing `"\n"`, a `user@host` comment, leading spaces) and assert `TenantForUserCA(sshca.AuthorizedKeyLine(pub))` — lookup by the *canonical* form — resolves. Storing verbatim then fails the lookup, killing the mutant. Optionally also assert the listed `Pubkey` equals the canonical line exactly. ### 8. `TestOpenTCPBridgesBytes` — deadline lifecycle unpinned **Risk:** clear-missing: every TCP/console session dies 10s after handshake (loud, user-visible, quickly found). Set-missing: a wedged agent pins the WS handler goroutine forever (quiet leak). Neither catastrophic, both worth pinning. **Repair: harness change + one new test; existing test keeps its (honest) name.** A 10s constant can't be unit-tested; promote `consoleHandshakeTimeout` to a field on `Service` (defaulted in `New`, overridden by tests — same pattern as `p.signal`). Then: - New `TestOpenStreamAbandonsAWedgedAgent`: timeout 100ms, fake agent accepts the stream and never replies; `OpenTCP` must return an error promptly. Kills the set-deleted mutant. - In `TestOpenTCPBridgesBytes`: timeout ~200ms, keep the existing 50ms... make it a post-handshake sleep *longer than the injected timeout* (e.g. 300ms), then the existing write/read round-trip. Kills the clear-deleted mutant — today's 50ms-vs-10s gap is what makes both mutants survivable. Fix the doc comment to say the clear is proven by outliving the (injected) handshake deadline. ### 9. `TestApplyReportSkipsUnchangedLeases` — extra writes per tick **Risk:** perf only — one redundant UPDATE per bridged VM per tick on SQLite's single connection. Real (it's the P1 perf lane's territory) but the least dangerous regression here. **Repair: new unit test carries the claim; existing test renamed.** The dedup lives in `netTracker.writeThrough`, whose `write` closure is the natural counting seam — and `tracker_test.go` already tests `statusTracker.writeThrough` exactly this way, but never `netTracker`. Add `TestNetTrackerWritesOnlyOnChange`: counting closure; same value twice → one write; new value → second write; failing write → error surfaced, cache NOT updated, retry writes again (the not-cached-on-failure branch is also unpinned today). The applyReport-level test can't see writes without instrumenting the store; rename it to what it proves — `TestApplyReportTracksTheReportedLease` (that applyReport routes leases through the cache at all) — and let the tracker test own the dedup name. --- ## The self-referential constants — pattern verdict and repo sweep **Verdict: these are two different diseases, and the repair differs.** *(a) Behaviour pinned, value floating* — `maxKeyCommentLen` is this: deleting the truncation FAILS the test (verified); only 128→4096 sails through. The parameterized assertion is *correct test construction* — the boundary behaviour is what the test owns. What's missing is a value pin **where the value itself is load-bearing**. *(b) Pure tautology* — the assertion copies the constant to both sides and proves only that an assignment happened. `assert.Equal(t, httpServerReadHeaderTimeout, srv.ReadHeaderTimeout)` is this: it passes at 0s with a message claiming "the Slowloris defense is on" (verified below). **Repair:** keep the behaviour tests parameterized (they're right); for constants whose *value* is the security/operational content, add a per-package literal-pinning guard test — `assert.Equal(t, 30*time.Second, enrollRefillEvery)` with the rationale in the message — so drifting a value is a conscious two-file edit. This is the house pattern already ("things can't pin their own defaults" → docs-match guard tests). For tautologies, the literal pin *replaces* the tautology rather than joining it. Do **not** pin taste-only values (that's noise); the list below marks which is which. Renaming-only is the honest fix nowhere in this trio — the limiter values are genuinely load-bearing (30ms refill ≈ no limiter; 10-entry cap ≈ prune-thrash on 11 IPs). ### Sweep results (assertions mentioning a prod constant on both sides, all packages) Scripted sweep: extracted every constant from non-test files, flagged `assert.`/`require.` lines in same-package tests referencing them. ~40 raw hits, triaged: **Tier 1 — tautologies or floating values where the value is the point (fix like the trio):** - `internal/server/boot/httpserver_test.go:18-19` — `assert.Equal(t, httpServerReadHeaderTimeout, srv.ReadHeaderTimeout)` (and Idle). **Verified: both constants set to 0 → package passes.** The v0.0.6 Slowloris hardening is pinned by nothing. Pin literals `20s`/`120s`. - `internal/server/boot/headers_test.go:45,49` — `assert.Equal(t, hstsMaxAge, …Header.Get("Strict-Transport-Security"))`. **Verified: `max-age=0` → package passes.** The header-presence half is real; the value needs a literal (or parse-and-assert `max-age ≥ 1y`). - `internal/server/store/exposures_test.go:30,45-52,164-183` — six assertions all relative to `MinAllocatedHostPort`. Allocation *ordering* is well tested; the base value floats — at `80` the allocator would hand out well-known ports and everything passes. One literal pin (`30000`) covers all six. - `internal/agent/vfkit/vfkit_test.go:142` — asserts the composed socket path `< maxSocketPath`, but `maxSocketPath` (103) stands in for the OS's `sun_path` limit, which is *external truth*: bump the constant and the test follows while real binds fail. Assert against literal 103 with the sun_path rationale. - `internal/server/api/ratelimit_test.go` — the filed pair (`enrollRefillEvery`, `maxLimiterEntries`), plus `enrollBurst` rides the same pattern in `api_test.go:928` and `ratelimit_test.go` loops. One guard test pinning all three limiter values. **Tier 2 — behaviour pinned, value floats; pin only if the value is judged load-bearing:** - `internal/mcpserver/sshrun_test.go:226` — `outputCap` (1 MiB): truncation behaviour pinned exactly like `maxKeyCommentLen`; cap value floats (memory bound per exec — probably worth a pin). - `internal/server/api/abandoned_vm_test.go:50,83` — `abandonedVMReapGrace` (15m) used as fake-clock input; grace behaviour pinned, window floats (1ms would reap freshly-created VMs). - `internal/agent/netenv/named_test.go:490` — `HasPrefix(NetTapName(…), netTapPrefix)`: prefix value floats; IFNAMSIZ budget depends on its length. Borderline. - `internal/agent/seed/seed_test.go:372-392` — chrony drop-in paths asserted via the constants; the paths are guest-side contracts. Borderline. - `internal/smoke/mcp_test.go:433` — `delegationTTL` is smoke's own request TTL asserted back from the cert it mints; low risk, smoke-local. - `internal/server/api/injectedkey_test.go:47` — the filed `maxKeyCommentLen`; behaviour already pinned, value pin optional. **Retired from suspicion (correct constructions, no action):** - `internal/agent/syncclient/client_test.go:111` — `OnlineWindow ≥ 3×DefaultTickInterval` is a *cross-package relationship* assertion — this is the model to imitate, not a bug. - `internal/smoke/mcp_contract_test.go:53` — `remoteToolCount` is smoke's independent copy of mcpserver's tool count (the deliberate tool-list tie); cross-package pin, good. - `internal/server/sshgate/gate_test.go:570` — `tenantExt` is minted and checked in the same package; internal consistency *is* the contract. - `transport` (`DefaultMaxFrame`, `ALPN`), `pidfile`/`statelock` (`mode`, `lockName`) — constants as call inputs, not expectations. **Net:** the trio is the visible edge of a repo-wide habit. Tier 1 is five sites (boot ×2, store exposures, vfkit socket-path, api limiter) where a value with a security or OS-truth rationale can drift to a useless setting with a green suite — the boot pair verified, the rest read. Suggest the guard-test convention lands as its own small issue so the pins arrive as one sweep, not nine drive-bys.
a73x commented 2026-08-23 10:27:19 +00:00 (Migrated from git.a73x.sh)
Copy link

[claude 2026-08-23] Claiming the eight lying tests (design comment items 1-9 minus the self-referential constants trio, which are issue 7c8b320f and belong to another agent). Working branch: lying-tests. Discipline: every repaired test is proven by applying its mutant, observing the FAIL, and restoring the tree.

[claude 2026-08-23] Claiming the eight lying tests (design comment items 1-9 minus the self-referential constants trio, which are issue 7c8b320f and belong to another agent). Working branch: lying-tests. Discipline: every repaired test is proven by applying its mutant, observing the FAIL, and restoring the tree.
a73x commented 2026-08-23 10:48:39 +00:00 (Migrated from git.a73x.sh)
Copy link

[claude 2026-08-23] Done on branch lying-tests (4 commits off main 290ffad, not merged, not pushed). make ci green.

Eight lying tests repaired; every one proven by applying its mutant to the production code, watching the named test FAIL with its intended message, and restoring (git diff clean on each source file, re-verified after rebasing onto main).

  • Redeem/BumpCredGeneration atomicity: SQLite ABORT trigger on audit_log inserts; new TestRedeemRefusesToEnrollUnaudited / TestBumpCredGenerationRefusesUnaudited assert full rollback (no host, token unconsumed, generation unmoved) and that the op works once the trigger drops. Old tests keep the success-path claims under honest names. Mutant: audit write moved after tx.Commit().
  • RemoveHost: three bare assert.Error sites become ErrorContains "not drained" (here and both branches of TestRemoveHostDrainSequence). Mutant if false && n > 0 now surfaces "FOREIGN KEY constraint failed" and fails.
  • UploadUserCA: posts an indented, commented, newline-terminated line; resolves by the canonical form. Mutant: store req.PublicKey verbatim.
  • Console handshake deadline: consoleHandshakeTimeout promoted to a Service.handshakeTimeout field (production value unchanged). TestOpenTCPBridgesBytes round-trips AFTER the injected deadline would have fired; new TestOpenTCPAbandonsAWedgedAgent holds a silent agent open. Both SetDeadline mutants now die.
  • serve: TestServeNotEnrolled now uses ONE dir for store and Config. New TestServeRefusesASecondAgentOnTheSameState requires "already running" and NOT "not enrolled", killing both the deleted-Acquire and the reordered-Acquire mutants.
  • vfkit Shutdown: injects p.signal, requires exactly one SIGTERM to the pidfile's pid. The ESRCH tolerance moves to TestShutdownTreatsAGoneProcessAsStopped. Mutant: sigterm body -> return nil.

Two departures from the design comment:

  1. Finding 8 (TestApplyReportSkipsUnchangedLeases) — the design says tracker_test.go never tests netTracker. It does: TestNetTrackerWritesOnceThenStaysQuiet counts writes and KILLS the deleted-dedup mutant at package scope (the design only ran the named test). No new tracker test was needed. Took the design's second half: renamed to TestApplyReportRoutesLeasesThroughTheDedupCache, and proved it with a different mutant (applyReport calling s.st.RecordVMNetworkIP directly), which nothing else catches.
  2. Finding 3 (TestSnapshotHubPerTenantPayloads) — main's e11ca29 landed TestSnapshotHubRecomputeFansEachTenantItsOwnBytes while this branch was in flight; it kills the same broadcast mutant from its own test. Dropped my duplicate.

Out of scope, worth filing: internal/server/web coverage is 89.2% against a floor of 95 in any checkout where 'make web' has not been run (dist/ is gitignored, only .gitkeep tracked). 'make ci' does not depend on 'web', so a fresh clone or worktree fails the coverage gate with no code change. Either make cover depend on web, or drop the floor to what an unbuilt dist yields.

[claude 2026-08-23] Done on branch `lying-tests` (4 commits off main 290ffad, not merged, not pushed). make ci green. Eight lying tests repaired; every one proven by applying its mutant to the production code, watching the named test FAIL with its intended message, and restoring (git diff clean on each source file, re-verified after rebasing onto main). - Redeem/BumpCredGeneration atomicity: SQLite ABORT trigger on audit_log inserts; new TestRedeemRefusesToEnrollUnaudited / TestBumpCredGenerationRefusesUnaudited assert full rollback (no host, token unconsumed, generation unmoved) and that the op works once the trigger drops. Old tests keep the success-path claims under honest names. Mutant: audit write moved after tx.Commit(). - RemoveHost: three bare assert.Error sites become ErrorContains "not drained" (here and both branches of TestRemoveHostDrainSequence). Mutant `if false && n > 0` now surfaces "FOREIGN KEY constraint failed" and fails. - UploadUserCA: posts an indented, commented, newline-terminated line; resolves by the canonical form. Mutant: store req.PublicKey verbatim. - Console handshake deadline: consoleHandshakeTimeout promoted to a Service.handshakeTimeout field (production value unchanged). TestOpenTCPBridgesBytes round-trips AFTER the injected deadline would have fired; new TestOpenTCPAbandonsAWedgedAgent holds a silent agent open. Both SetDeadline mutants now die. - serve: TestServeNotEnrolled now uses ONE dir for store and Config. New TestServeRefusesASecondAgentOnTheSameState requires "already running" and NOT "not enrolled", killing both the deleted-Acquire and the reordered-Acquire mutants. - vfkit Shutdown: injects p.signal, requires exactly one SIGTERM to the pidfile's pid. The ESRCH tolerance moves to TestShutdownTreatsAGoneProcessAsStopped. Mutant: sigterm body -> return nil. Two departures from the design comment: 1. Finding 8 (TestApplyReportSkipsUnchangedLeases) — the design says tracker_test.go never tests netTracker. It does: TestNetTrackerWritesOnceThenStaysQuiet counts writes and KILLS the deleted-dedup mutant at package scope (the design only ran the named test). No new tracker test was needed. Took the design's second half: renamed to TestApplyReportRoutesLeasesThroughTheDedupCache, and proved it with a different mutant (applyReport calling s.st.RecordVMNetworkIP directly), which nothing else catches. 2. Finding 3 (TestSnapshotHubPerTenantPayloads) — main's e11ca29 landed TestSnapshotHubRecomputeFansEachTenantItsOwnBytes while this branch was in flight; it kills the same broadcast mutant from its own test. Dropped my duplicate. Out of scope, worth filing: internal/server/web coverage is 89.2% against a floor of 95 in any checkout where 'make web' has not been run (dist/ is gitignored, only .gitkeep tracked). 'make ci' does not depend on 'web', so a fresh clone or worktree fails the coverage gate with no code change. Either make cover depend on web, or drop the floor to what an unbuilt dist yields.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
v0.0.5
v0.0.9-pre.1
v0.0.9
v0.0.8-pre.3
v0.0.8
v0.0.8-pre.2
v0.0.8-pre.1
v0.0.7-pre.1
v0.0.7
v0.0.6
v0.0.6-pre.1
v0.0.5
v0.0.5-pre.1
v0.0.4
Labels
Clear labels   
backlog

   
dev

   
legacy-patch
Historical git-collab patch state

  
merged
Historical git-collab patch state

  
planning

   
review
No labels
backlog
dev
legacy-patch
merged
planning
review
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
a73x/eitri#98
No description provided.