From ef97d85ac9cfbe548167ec7010806a73d28a61f6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 17:00:59 +0000 Subject: [PATCH] Document repository-wide test strategy audit --- audit.md | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) diff --git a/audit.md b/audit.md index b678478..3c341ed 100644 --- a/audit.md +++ b/audit.md @@ -3853,3 +3853,236 @@ go run ./examples/go-library/prepare Any future change to fairness or allocation strategy should first preserve the current linearization points and be justified by representative contention measurements. + +## Stage 16: Repository-Wide Test Strategy And Maintained Examples + +### Scope Reviewed + +The suite-level review covered all 25 `_test.go` files and their 281 top-level +tests, every package-local and repository-level fixture, the external-package +root contracts, `architecture_test.go`, and both maintained programs and prompt +files under `examples/go-library`. The testing policy supplied the +risk-oriented sufficiency standard. The architecture, documentation, consumer, +format, release, and internal-overview documents supplied the repository and +example boundaries. Component behavior was not reopened: the Stage 1 through +15 coverage ledgers were used to assign owners and identify already accepted +gaps. + +The refreshed code graph mapped test symbols and `TESTS` edges across the +public facade and internal packages. Source inspection then checked the suite +for exact error strings, private-helper and constant coupling, fixed ports, +live dependencies, environment and filesystem state, timers, sleeps, fixture +mutation, golden updates, and helper complexity. Coverage and timing directed +attention only; findings below are based on the maintained workflow contract +and one reproduced environment-dependent failure. + +### Accepted Findings + +#### S16-F01: Maintainer validation never executes the maintained Run example + +- **Category:** testing +- **Severity:** low +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `docs/policy/testing.md` and `docs/release.md` (maintainer + validation commands), `docs/development.md` (the assigned contributor-command + owner), and `examples/go-library/run/main.go` plus its `prompt.yaml` +- **Contract at issue:** Both Go-library examples are maintained, deterministic, + offline downstream workflows. The execution example is intentionally + separate because it protects assembled `Run`, injected-client, validation, + usage, and result behavior that the preparation example does not exercise. + Maintainer validation should execute each distinct maintained consumer + artifact whose fixture is otherwise only compiled or ignored. +- **Evidence:** The internal overview and consumer guide name both examples as + maintained offline workflows, but the testing-policy and release command + lists invoke only `go run ./examples/go-library/prepare`. `go test ./...` + reports both example packages as having no test files; diagnostic coverage + reports 0.0% for each, so it compiles `run/main.go` but never opens + `run/prompt.yaml` or invokes the engine. Directly running + `go run ./examples/go-library/run` from the repository root completed in + under a second and returned the documented deterministic result. The + preparation command also passed, confirming that the two programs are + independently runnable rather than alternate entry points to one fixture. +- **Failure mode:** The execution example's relative path, prompt format, + in-memory profile, injected-client adaptation, runtime validation, output + projection, or fixture can drift while every required maintainer command + still passes. A consumer copying the advertised complete example discovers + the breakage after release. Root contracts continue to protect library + behavior, but they do not execute this maintained artifact or its file. +- **Recommended direction:** Add the execution example to the canonical local + maintainer-validation workflow and synchronize documents that currently + present that workflow without creating a second independent command owner. + Keep both examples separate: their preparation and execution outcomes are + distinct and their small duplicated prompt fixtures make each program + independently copyable. +- **Required verification:** Run both example commands from the repository + root without credentials or network access, require successful exit and + stable semantic fields for their respective prepared and executed outputs, + and demonstrate that an invalid or missing execution prompt fixture makes + the maintained validation fail. + +#### S16-F02: A transport test depends on fixed port 9999 being unused + +- **Category:** testing +- **Severity:** low +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/llm/openai_compatible_client_test.go` + (`TestOpenAICompatibleClientAllowsEmptyConfiguredBaseURL`) +- **Contract at issue:** Default tests must be deterministic, offline, + independent of live infrastructure, and free of fixed-port assumptions. A + test of configured-versus-request endpoint selection should control its + transport outcome rather than infer selection from an environmental + connection refusal. +- **Evidence:** The test sends a real HTTP request to + `http://localhost:9999/v1` and requires `ErrRequestFailed`, assuming nothing + is listening. With a temporary standard-library HTTP server bound to + `127.0.0.1:9999`, the focused test failed immediately: the selected endpoint + returned status 501, producing `ErrNonSuccessStatus` instead of the required + connection error. The listener was then stopped and the ordinary suite + passed. Search across all tests found no other contacted fixed port; + `localhost:8000` occurrences are inert fixture values or use injected + clients. +- **Failure mode:** A developer, build host, or shared runner with any service + on port 9999 gets a false test failure whose error category depends on that + unrelated service. The test also performs a real socket operation even + though both constructor acceptance and endpoint-override selection can be + established deterministically. +- **Recommended direction:** Supply a controlled `httptest` endpoint or + injected round tripper that records the selected URL and returns a deliberate + transport result. Retain separate assertions that an empty configured base + is valid and that the request endpoint takes precedence; do not preserve + connection-refusal behavior as a contract. +- **Required verification:** Run the focused case normally, repeatedly, and + while an unrelated listener occupies port 9999. It must make no request to + that listener, must prove that the target endpoint was selected, and must + retain the intended constructor and error identities without relying on the + host network state. + +### Unresolved Observations + +None. The documentation policy assigns local validation commands to the +development guide, while that guide currently has no command list and the +release procedure says its own list comes from the guide. Comparing and +baselining those three command owners is explicitly Stage 0 work, which remains +absent; this stage did not backfill that broader audit or promote a duplicate +finding beyond S16-F01's concrete missing workflow. + +Large files were not treated as defects by size. `engine_test.go`, +`internal/usecase/runner_test.go`, and the external prepared contracts organize +many distinct public or orchestration behaviors, while S14-F06 already records +the one confirmed case where repeated transport scaffolding obscures its +behavior matrix. The 25 ms prepared-timing sleep is unnecessary for ordering +but passed shuffled repetition and race runs, adds negligible suite time, and +does not establish a consequential flaky boundary by itself. + +### Risk-To-Owner Matrix + +| Risk | Credible current owner | Accepted gap or retained protection | +| --- | --- | --- | +| Public compatibility and error identity | External-package root tests in `engine_test.go`, `public_contract_test.go`, `prepared_execution_contract_test.go`, and `capacity_contract_test.go`; focused internal mapping tests own internal-type containment. | S01-F01, S02-F03, S02-F04, S03-F02, S04-F01, and S13-F01 already identify the consequential missing public cases. Other internal/root overlap asserts different stable boundaries and is retained. | +| Parsing, validation, and serialization | `internal/promptdef`, `internal/profile`, `internal/filecatalog`, `internal/jsonvalue`, `internal/validate`, and domain tests own focused rules; root corpus and JSON contracts own assembled public compatibility. | S05-F04 and S07-F06 own material missing rule coverage. The reproduced parser, profile, schema, and numeric defects already specify regression cases in their component findings; no second suite-level test owner is needed. | +| Immutability and data integrity | Domain and JSON-value tests own shared transforms; backend, repository, use-case, prepared, and root tests own snapshots at each ownership transfer. | S02-F03 owns injected-client mutation isolation. S05-F03 owns unbounded tree work. S05-F05 identifies unused internal prepared JSON tests whose stable public serialization remains protected by external root contracts. | +| External wire behavior | `internal/llm` owns realistic `httptest` request/response behavior; root tests own resolved settings reaching the built-in client and public error mapping. | S14-F01 through S14-F05 own missing cancellation, timeout, URL, size, and framing boundaries. S14-F06 retains protocol assertions while consolidating repeated scaffolding. S16-F02 is the only fixed-port/live-environment case. | +| Cancellation, failure propagation, and recovery | Artifact, validator, model-client, capacity, use-case, prepared-lifecycle, and root tests each own failures at their narrow boundary; root tests own public identities and partial-result rules. | S04-F01, S09-F02, S10-F04, and S14-F01 identify missing or defective cancellation paths. Existing failure tests use `errors.Is`/`errors.As`; error-text checks are limited to required path association, redaction, or diagnostic fragments. | +| Concurrency and resource lifecycle | Capacity package and public capacity contracts own relational limits and FIFO release; prepared use-case and public contracts own claim/discard races; race-enabled validation owns frozen-plan safety. | Stage 15 found no scheduling defect or duplicate owner. S05-F03 and S14-F04 own the two unbounded resource paths. Shuffled repeated race runs found no shared-state or ordering failure. | +| Representative assembled consumer workflows | Root external tests own broad assembled behavior; the framework corpus owns a realistic prepare fixture; the two examples own independently copyable offline preparation and execution programs. | S16-F01 records that only the preparation example is executed by maintainer validation. The examples' small parallel fixtures are retained because each program demonstrates a distinct workflow and remains standalone. | + +### Suite And Fixture Ledger + +- **Inventory and boundaries:** The graph and source inventory found 25 test + files with 281 top-level tests. Five root files use package + `promptkit_test` for public contracts, while the two small root internal + files directly own adapter copying and error translation unavailable through + an external surface. Internal packages test their own boundaries. No test + imports a consumer, and the AST-based architecture guard recursively rejects + imports of the former consumer module while a focused self-test proves that + its detector finds nested imports. +- **Fixtures and secrets:** Parser and profile fixtures are minimal malformed + and valid YAML cases; schema and prompt content use synthetic values. The + framework corpus is a shared root integration fixture rather than a second + parser truth table. Tests use `t.TempDir`, temporary directories, `fstest`, + and `httptest` where mutable or external behavior matters. Environment + credentials are synthetic and controlled with `t.Setenv` or deliberate + execution-time removal; no real secret or mutable external service is + required. S16-F02 is the sole contacted fixed port. +- **Assertions and coupling:** Public and collaborator errors are normally + asserted with `errors.Is` or `errors.As`. Remaining text assertions select + structural paths, schema/compiler causes, status codes, redaction sentinels, + or the smallest diagnostic fragment needed to distinguish a case; none + snapshots a complete noncontractual message. Exact collaborator call counts + in runner and prepared tests protect ordering, bounded generation, no partial + work, and one-attempt lifecycle requirements rather than private call graphs. + No golden files or automatic update mode exist. +- **Consolidation:** Component ledgers found intentional relational overlap + between package, use-case, and public boundaries. The accepted deletion or + consolidation candidates already state their remaining protection: + S05-F05 can remove unused internal prepared serialization cases while root + JSON contracts remain; S09-F05 can stop pinning a hash algorithm while + retaining determinism and mutation sensitivity; and S14-F06 can share HTTP + setup while retaining every wire-visible assertion. No additional + evidence-backed deletion emerged at suite level. +- **Determinism and cost:** Twenty shuffled ordinary runs and three shuffled + race-enabled runs passed across every package. No test-order, global-state, + data-race, deadlock, or leaked-waiter symptom appeared. A diagnostic timing + run found no individual test above 50 ms in that sample and no package pass + event above 205 ms. Package coverage ranged from 69.2% to 100% among + packages containing substantive tests; the lower percentages align with + the already recorded JSON-value and validation decisions and were not used + as independent findings. `internal/defaults` + remains untested because its trivial baseline is exercised relationally by + higher owners rather than pinned as private constants. +- **Maintained examples:** Preparation resolves and renders without a model; + execution injects a deterministic client and validates a result. Preparation + never calls its configured endpoint, and execution's injected client bypasses + its endpoint, so both remain offline and credential-free. + Their similar prompt files are intentionally local so either complete + program can be copied without a hidden cross-example dependency. Both ran + successfully during this review; only validation ownership is deficient as + S16-F01 records. + +### Verification Performed + +The code knowledge graph inventoried test files and symbols, associated tests +with production hotspots, and confirmed the external and internal package +boundaries. Source and the prior component ledgers supplied the behavioral +ownership analysis. Diagnostic coverage and timing were used only to direct +the review: + +```sh +go test -cover ./... -count=1 +go test -json ./... -count=1 +``` + +The suite passed shuffled repetition, shuffled race execution, vet, build, Go +formatting, and both offline example runs: + +```sh +go test ./... -shuffle=on -count=20 +go test -race ./... -shuffle=on -count=3 +go vet ./... +go build ./... +go run ./examples/go-library/prepare +go run ./examples/go-library/run +``` + +A temporary Python standard-library server was bound to `127.0.0.1:9999` only +for the focused probe. With it active, this command failed with status 501 +instead of the expected `ErrRequestFailed`, confirming S16-F02; the server was +stopped immediately afterward: + +```sh +go test ./internal/llm -run '^TestOpenAICompatibleClientAllowsEmptyConfiguredBaseURL$' -count=1 +``` + +### Handoff + +- The Stage 0 baseline and its assigned comparison of validation-command + owners remain absent and were not backfilled. +- Stage 17 should use the risk matrix and existing S02-F02, S02-F05, S07-F05, + S12-F01, and S14-F06 duplication evidence without treating file size, helper + counts, or intentional cross-boundary tests as new consolidation findings. +- Stage 18 should retain both S16 findings unless the final audited tree has + gained deterministic execution-example validation or removed the fixed-port + assumption. Its synthesis should count earlier test gaps once at their + owning component rather than cloning them from this matrix.