[samjulien]

What this changes

Removes ineffective coverage while retaining tests that protect users indirectly through database safety, network isolation, CI reliability, and production architecture rules. Production behavior is unchanged.

The initial audit covered 811 test-bearing files. After review, the cleanup deletes 13 complete test files and removes individual source-spelling checks, self-matching assertions, mock-only checks, and misleading cases elsewhere.

Review corrections

• Restored brokered provenance, safe test database selection, CI exit/stderr forwarding, preload parity, network blocking, mutation-runner, and source-scanner coverage.
• Retained the assertion-shape guard and replaced line-number exceptions with full expression + enclosing-test identities. Existing exceptions shrink from 68 to 66; duplicate assertions and new tests cannot borrow an exception.
• Replaced weak PKCE, skills-checksum, component SQL, people search/paging, and Compose migrator tests with discriminating fixtures and exact assertions. Restored the transcript memo comparator test.
• Fixed a process-wide component mock leak exposed by the merged app suites: per-test spies now restore the real New Bot and sidebar components.
• Merged current main, resolved the assertion-guard conflict, and corrected stale enforcement references.
• Preserved the 13 known authorization audit gaps in #876. Run-specific audit notes now live in this PR rather than a root-level report.

Where it runs

No new persistent state, serialization, browser fan-out, listener, port, or schedule. Both replicas run unchanged production behavior.

Boundary and audit

Gateway enforcement, refusal/failure auditing, and client trust boundaries are unchanged. The existing missing lifecycle audit events remain tracked in #876.

Changelog

No changelog entry: this changes tests and comments, with no deployment behavior change.

Proof

Latest main merge

Merged main at f678b330 while preserving the approved audit changes. The two conflicts were a deleted form-source guard and obsolete line-number metadata in the assertion guard. Validation after this merge: 56 focused tests passed; full formatting/lint, all six workspace typechecks, the existing server test-type baseline, and app/extension builds passed. Heavyweight checks ran sequentially (34–41% free memory). The earlier full-suite counts below predate this merge; CI reruns on the updated PR head.

Validation uses pinned Bun 1.4.3 after a frozen-lockfile dependency install. Heavyweight checks run sequentially, with memory monitored.

Validation	Result
Full server suite, 20 sequential shards with disposable PostgreSQL	5,747 passed; 0 failed; 18 opt-in skipped
Full app suite, including colocated and newly merged skin tests	1,911 passed; 0 failed
Preferences and assertion/provenance guards after the final main merge	33 passed; 0 failed
Restored safety/CI/scanner guards	90 passed; 0 failed
Strengthened OAuth, checksum, people and component suites	141 passed; 0 failed
Compose and transcript suites	65 passed; 0 failed
Previously conflicting app suites run together	22 passed; 0 failed
Repository-wide formatting and lint	Passed
All five workspace typechecks; app production build	Passed
Server test typecheck	Passed against existing 240-error baseline; no new errors
Git whitespace validation	Passed

The focused-suite counts overlap the full app/server runs; they are not additive. The full server run preceded the final skins merge; the changed preferences suites and assertion/provenance guards were rerun afterward. The full app run includes that merge.

Eight temporary production mutations were all caught by assertions: arbitrary and RFC-example constant PKCE challenges; skills omitted from the checksum; removed people page cap; unescaped wildcard search; unbounded component arguments; removed component time filtering; and removed refusal filtering. Every production mutation was reverted.

Limits: Rust was not compiled/run locally because Cargo is unavailable. Live vendor/browser/deployment suites remain opt-in. Unchanged Python framework integration suites were statically reviewed; the 33 edited Python model-spec cases passed during the original audit. This is a semantic audit plus selected mutation proofs, not exhaustive mutation testing.

[dustywusty]

Most of the audit holds up. Roughly 80% of the removals can go: source-string checks, tests that can't fail, and self-matching include_str! checks. A handful of deletions remove the only guard on something real, though, and one of the stated reasons is wrong. Requesting changes for those.

The underlying disagreement

test-audit.md:77 says the criterion was "executed end-user product behavior". Lint, architecture and test-infrastructure guards were cut by definition, not because they failed to work. Several of them were the only protection for a production rule or for CI itself. That criterion is worth agreeing on before we merge this.

Blocking

1. It conflicts with main and CI never ran. #861 edited assertion-shape.test.ts, which this PR deletes. No checks ran on the branch, so the Proof table is from local runs only (Bun 1.4.2 against the pinned 1.4.3, and no Cargo).
2. Real guards deleted with nothing replacing them. For each one I grepped for surviving coverage and found none.
   • brokered-provenance.test.ts. The audit row says "test-authoring conventions… synthetic snippets", but test 1 scans production server/src/plugins. It was added after an Arcade bug shipped: store.ts hard-coded composio://${toolkit} and every Arcade connection disappeared from settings. access.ts still says this test enforces the rule.
   • server/tests/support/database.test.ts. It stops the test helper from using the live openbot database. Integration tests TRUNCATE tables.
   • scripts/test-ci.test.ts. It is the regression test for #451 (exit-code and stderr passthrough). test:ci is still the CI gate (ci.yml:372).
   • bunfig-preload.test.ts. If the root and server preload lists drift, test files drop out silently. MINIMUM_TESTS = 400 won't notice when about 5.6k tests normally run.
   • test-network-guard.test.ts. The guard is still loaded, but nothing proves it blocks anything. Its first version leaked through https.get and Bun.fetch. If it regresses, tests could make live vendor calls with a developer's keys.
   • scripts/mutation-register.test.ts. It was the only CI check on that tool; test:mutations isn't in ci.yml.
3. Replace these before deleting them. The mutation findings are correct, but deleting the test leaves the mutation passing.
   • PKCE S256. plugin-oauth-callback.test.ts:155 still lets a constant challenge pass. The RFC 7636 Appendix B vector fixes that.
   • Skills checksum. skills is hashed at tenant-package.ts:787, but no test checks that editing skills.yaml changes the checksum. The agents/ test at tenant-package-agent-files.test.ts:189 is the pattern to copy.
   • Component store. The removed integration test was weak, but it was the only one that ran the component data functions' SQL against Postgres. Strengthen it instead.
   • People search. escapeLike and MAX_PAGE in people/store.ts now have no tests. Fix the fixtures (seed a name like "NinetyX Percent" and more than 200 rows) rather than drop the coverage.
   • Compose migrator. services is already parsed in compose.test.ts, so expect(services.migrate.command).toEqual(["bun","scripts/migrate.ts"]) is one line.

Should fix

• assertion-shape.test.ts. It catches exactly the defect this PR is about. Its only problem is an allowlist keyed by line number, which is also why this conflicts with main. Restored at this head, it fails only on lines this PR shifted. Keying it by expression, as mutation-register.ts:39-42 already recommends, fixes it without deleting it.
• Known audit gaps. The ledger test was the only record of 13 sign-in-flow transitions that don't write audit rows. Please move that list to an issue.
• test-audit.md. It records details of one run (Bun version, memory monitoring, counts), so it belongs in the PR description rather than at the repo root.
• Transcript memo test. The .compare test in transcript-attachments could actually fail if the comparator were dropped, so keep it.

Stale references to deleted tests

None of these files are in the diff, so I couldn't comment inline. Each comment cites a deleted test as the thing that enforces a rule. Either restore the test or reword the comment so it doesn't claim enforcement that no longer exists.

The rule they describe is now unenforced:

• server/src/plugins/access.ts:28 (brokered-provenance.test.ts)
• server/src/plugins/access.ts:232 (brokered-provenance.test.ts)
• server/src/plugins/access.ts:285 (brokered-provenance.test.ts)
• server/src/plugins/plugin-store/servers.ts:1131 (brokered-provenance)
• server/scripts/test-network-guard.ts:69 (test-network-guard.test.ts)
• server/scripts/test-network-guard.ts:130 (test-network-guard.test.ts)
• server/bunfig.toml:11 (bunfig-preload.test.ts; the path was already wrong, and it now points at nothing)
• server/src/plugins/route-groups/connections.ts:1624 (pending-authorization-audit-ledger.test.ts; the 13 known gaps lived there)
• server/src/plugins/plugin-store/context.ts:1894 (sole-definition.test.ts)
• app/src/lib/lookup.ts:16 (sole-definition.test.ts)

Behavior is still covered for today's brokers, but the comment is stale:

• server/src/plugins/broker.ts:660 (broker-authorization-pairing.test.ts)

Stale wording only:

• scripts/mutation-register.ts:41-42 (assertion-shape.test.ts, sole-definition.test.ts)
• server/tests/support/stub-broker.ts:11 (broker-stub-ratchet.test.ts)
• server/tests/composio-connect-adopts-held-account.test.ts:310 (fixture-identity-distinct.test.ts)

Fine as is

• Rust. The deletions look clean under clippy --all-targets -D warnings: nothing is left unused. The two that looked risky are covered more strictly elsewhere. stopping_names_the_harness_profile matched its own string; the Stop test's fake engine at main.rs:6353 covers that behavior. the_login_program_prints_the_whole_store is covered by plan.rs:1039.
• Also fine to delete: crewai token restoration (it was testing pytest's monkeypatch), the model-spec source checks, the composio and arcade completeness tables, the computer-client signal checks, and the audit trigger scans (covered by audit-retention.integration).

[dustywusty]

Thanks, this addresses everything from my earlier review.

• Restored guards. brokered-provenance, test-database refusal, test-ci, bunfig preload, mutation-register and source-scan are byte-identical to the originals. The network-guard test has one small type-only cleanup.
• assertion-shape. Keying exceptions by expression plus the enclosing test fixes the line-number brittleness without losing the check.
• Mutation checks. I broke the code locally and confirmed the new tests catch it:
  • PKCE fails when the challenge is the constant RFC example, and when the encoding is base64 instead of base64url.
  • The skills checksum test fails when skills is dropped from the hash.
• People search and component store. I couldn't run these (no local Postgres), but from reading them they would now fail if the code broke: the underscore and percent fixtures, the 201-row page cap, and exact rows with the 1–50 limit clamp for the component functions. CI runs them.
• Stale references. None remain. The connections.ts comment now points at #876.
• channel-new-error-state. Switching from mock.module to restorable spies makes sense; it fixes the mocks leaking into later suites.

Approving.