diff --git a/docs/plans/2026-08-18-release-stabilization.md b/docs/plans/2026-08-18-release-stabilization.md new file mode 100644 index 00000000..76040c37 --- /dev/null +++ b/docs/plans/2026-08-18-release-stabilization.md @@ -0,0 +1,538 @@ +# Release stabilization and human-maintainability campaign + +**Status:** PROPOSED — plan recorded; implementation not started +**Created:** 2026-08-18 +**Audit baseline:** `15539a22a67f8d915d88f8b1d8126cd55eedda6e` +**Evidence:** [`../reviews/2026-08-17-release-maintainability-audit.md`](../reviews/2026-08-17-release-maintainability-audit.md) +**Findings:** [`../reviews/findings-ledger.md`](../reviews/findings-ledger.md) +**Coverage proof:** [`../reviews/coverage-ledger.md`](../reviews/coverage-ledger.md) + +## 1. Goal + +Prepare acdream for a responsible public release and for maintenance by human +developers who do not have access to prior AI conversations, private worktrees, +or campaign memory. + +The campaign succeeds when a new maintainer can clone the repository, identify +the current architecture and supported release, reproduce the build and test +gate, understand why non-obvious retail behavior exists, and publish or roll +back an authenticated release using repository-owned instructions. + +This is a stabilization program, not a rewrite. The existing Runtime/App/ +Headless architecture remains the foundation unless a slice proves a specific +boundary is wrong. + +## 2. Binding principles + +1. **Protect behavior before cleanup.** Establish a deterministic complete gate + before broad refactors, comment cleanup, or file decomposition. +2. **Distill knowledge; do not erase it.** No note, comment, issue history, + diagnostic, capture, or raw artifact is removed until its durable value has + a verified destination. +3. **One current truth.** Stable architecture and release state must not depend + on choosing between README, roadmap, milestone, campaign, memory, or + tool-specific instruction copies. +4. **Separate evidence from contracts.** Source comments explain the current + invariant. Dated research records preserve investigation history. Raw + captures live in an explicit artifact tier. +5. **No count-only gates.** A test total is meaningful only when the report says + which hermetic, installed-DAT, live, visual, manual, and diagnostic lanes + actually ran. +6. **Bound every external interaction.** Process waits, network operations, + test runs, and release steps require timeouts, cancellation, and diagnostic + artifacts on failure. +7. **Small reversible slices.** Each slice gets focused tests, a complete gate, + a reviewable commit, a rollback description, and a plan-ledger update. +8. **No opportunistic feature work.** New gameplay features wait unless needed + to prove or repair a release blocker. + +## 3. Knowledge-preservation protocol + +Every cleanup candidate is classified before it moves: + +| Class | Durable value | Destination | +|---|---|---| +| Current invariant | Required behavior, ordering, ownership, threading, or retail rule | Short source comment and/or maintained architecture contract | +| Decision rationale | Alternatives considered, failed attempts, tradeoffs, gate outcome | Dated decision/research record linked from the current contract | +| Reproducible evidence | Minimal fixture, retail symbol/address, script, checksum, expected result | Versioned fixture/research record in Git | +| Raw evidence | Large logs, captures, Ghidra state, screenshots, dumps | Approved versioned artifact store with manifest, hash, provenance, and retention policy | +| Superseded or incorrect claim | Historically useful but no longer operative | Marked `SUPERSEDED` with successor link; archive after references are migrated | +| Duplication/noise | Repeats a preserved fact and adds no independent evidence | Delete only after destination/link validation | + +Before deleting or rewriting historical material, all of these must be true: + +- its current invariant is recorded at the owning code or architecture seam; +- useful retail provenance, failed approaches, and acceptance evidence remain + searchable under stable identifiers; +- inbound links and source comments point at the surviving destination; +- any raw artifact has an approved distribution, privacy, and licensing status; +- the replacement was reviewed by someone other than its author; +- the complete gate passes after the move. + +Git history alone is not the preservation mechanism. History may later be +rewritten to remove large or legally restricted artifacts. + +## 4. Campaign dependency map + +```text +R0 baseline/authority + -> R1 launcher deadlock + -> R2 reproducible complete gate + -> R3 truthful test lanes + -> R5 documentation authority + -> R6 dead surfaces and tools + -> R7 plugin/config hardening + -> R8 bounded decomposition + -> R10 release candidate + +R4 licence/provenance/release governance ---------------------> R10 +R9 artifact migration (depends on R4 decisions) --------------> R10 +``` + +R4 starts in parallel because it requires owner/legal decisions. It blocks a +public release but does not block the technical safety work in R1–R3. + +## 5. Slice map + +| Slice | Outcome | Depends on | Release blocking | +|---|---|---|---| +| R0 | Baseline and durable campaign authority accepted | — | yes | +| R1 | Launcher shutdown lock inversion fixed and deterministic | R0 | yes | +| R2 | Pinned clean build and complete bounded CI gate | R1 | yes | +| R3 | Test results truthfully distinguish executed, skipped, and diagnostic work | R2 | yes | +| R4 | Licence, provenance, credential disclosure, and release ownership decided | R0; parallel | yes | +| R5 | One current documentation authority; public docs match the product | R2–R3 | yes | +| R6 | Dead presentation/backend/probe surfaces removed; supported tools reproducible | R3–R5 | normally yes | +| R7 | Plugin compatibility/lifetime and diagnostic configuration hardened | R3 | yes for advertised plugin release | +| R8 | Highest-risk giant owners decomposed only at proven seams | R3, R6–R7 | selective | +| R9 | Large/generated research artifacts moved under an approved policy | R4 | yes if repository is distributed | +| R10 | Clean-clone release candidate and rollback rehearsal | all blocking slices | yes | + +## 6. R0 — Baseline and authority + +### Work + +- Review and accept this plan and the six documents under `docs/reviews/`. +- Record the exact starting commit, SDK, package sources, operating systems, + supported release platforms, and current external prerequisites. +- Decide who owns legal/provenance decisions, CI/release credentials, and final + release approval. +- Pause unrelated feature campaigns until R1–R3 establish the safety net. +- Preserve the audit baseline before any cleanup; do not rewrite artifact + history in this slice. + +### Exit criteria + +- Plan and audit artifacts are tracked in the repository. +- One named owner exists for technical release approval and one for + licensing/provenance approval; the same person may hold both roles. +- The ledger in §17 identifies R1 as the only active implementation slice. + +## 7. R1 — Fix the launcher shutdown deadlock first + +### Confirmed failure + +`LauncherProcessSupervisor.Dispose` currently holds the supervisor `_gate` +while disposing a child. `WindowsSystemChildProcess.Dispose` enters +`System.Diagnostics.Process.Dispose`; concurrently, the process-exit callback +can enter `OnProcessExited` and try to publish state through the same supervisor +gate. The captured wait cycle is: + +```text +shutdown: supervisor _gate -> Process internals +exit callback: Process internals -> supervisor _gate +``` + +The complete solution test process hangs on +`ANullStderrLogPathBehavesExactlyAsBeforeForBothChildProcessKinds`; the +Launcher.Core project can pass alone because the race timing changes. + +### Design constraints + +- Never call child/process operations that may wait, dispose, raise callbacks, + or execute external code while holding `_gate`. +- Under `_gate`, make only the minimal state transition and snapshot the exact + children/work to retire. +- Perform unsubscribe, stop, kill, wait, and dispose work outside `_gate`. +- Exit callback and explicit disposal must converge idempotently regardless of + which arrives first. +- Preserve exact terminal-state ordering, status publication, graceful-stop + behavior, and child ownership; do not solve the hang by dropping callbacks. +- A failed cleanup must remain observable without starving later cleanup. + +### Required tests + +- Add a barrier-controlled race test that deterministically pauses the child + exit callback while disposal begins. Do not use `Thread.Sleep` as the oracle. +- Cover exit-before-dispose, dispose-before-exit, simultaneous exit/dispose, + repeated dispose, stop timeout/kill fallback, and callback failure. +- Assert one terminal publication, no resurrection, no orphan child, no held + supervisor lock during process disposal, and bounded completion. +- Run the focused race test repeatedly after its deterministic single pass. +- Run all Launcher.Core tests. +- Run the complete Release solution twice in fresh processes under a documented + timeout and retain hang dumps if either run fails to terminate. + +### Exit criteria + +- No process/child disposal occurs under the supervisor gate. +- The deterministic race test fails against the baseline mechanism and passes + against the fix. +- Two complete bounded solution runs finish with zero failures. +- F-009 and T-001 receive exact fix commit and gate evidence. + +## 8. R2 — Reproducible build and complete CI gate + +### Work + +- Pin the accepted .NET 10 SDK feature band in `global.json`. +- Centralize common compiler/analyzer settings and package versions; enable + locked restore for release/CI. +- Eliminate all 26 clean-rebuild test warnings or make a narrowly justified, + centrally documented exception fail-safe. +- Build every supported product and every supported tool from a clean checkout. +- Make CI run the complete solution, not only portability subsets. Give each + project/process a timeout and collect test logs plus managed dumps on hangs. +- Preserve focused Windows/Linux portability lanes, but do not represent them + as the complete gate. +- Record restore sources, SDK/runtime, RID, commit, executed/skipped counts, and + artifact hashes in every release report. + +### Exit criteria + +- A clean clone restores and builds with the pinned toolchain and no warnings. +- Complete CI runs every default release test project and fails on timeout. +- The launcher hang cannot silently consume the CI job indefinitely. +- Package resolution and the gate command are reproducible from repository + instructions alone. + +## 9. R3 — Make test reporting truthful + +### Required lanes + +1. **Hermetic release lane:** default CI; unavailable local data is never a + silent passing return. +2. **Installed-DAT/prepared-package lane:** explicit prerequisites and per-suite + skip identity; result published separately. +3. **Live/connected/visual/listening lane:** operator-owned, dated evidence; + never counted as ordinary unit coverage. +4. **Diagnostic/manual lane:** probes, dumps, fixture generators, and + characterization programs invoked explicitly outside default test totals. + +### Work + +- Replace the 271 asset/environment empty-return tests with explicit lane + requirements, truthful skips, or hermetic fixtures. +- Move or give stable assertions to the 51 output-only diagnostic methods. +- Delete/replace the three confirmed useless entire cases and the tautological + assertions catalogued in the audit. +- Remove the duplicate theory row and make the clean rebuild warning-free. +- Remove or re-home at least 52 tests with the unreachable panel stack. +- Retire temporary source-shape freezes once a semantic architecture/behavior + guard exists; retain whole-tree dependency guards that express real rules. +- Assign and stabilize the seven documented load-sensitive tests. Do not hide + them with generic retries. +- Inject controllable time into double-click and real-time transport tests. + +### Exit criteria + +- Default test success means every discovered default contract executed. +- Reports give exact reasons and prerequisite identity for every skip. +- Diagnostics and manual generators do not inflate release regression totals. +- No known duplicate row, literal tautology, or permanent empty scaffold remains + in the default suite. + +## 10. R4 — Licence, provenance, security, and release ownership + +This slice requires explicit project-owner decisions and, where appropriate, +qualified legal review. The implementation agent records evidence but does not +invent a redistribution basis. + +### Work + +- Select the project licence and establish contributor/code ownership. +- Audit WorldBuilder-derived code, dependency notices, named-retail/decompiler + exports, PDB-derived data, Ghidra databases, captures, images, and DAT-derived + fixtures for provenance and redistribution status. +- Decide which research artifacts may be public, private, regenerated, or + deleted from distributable history. +- Add SECURITY, CONTRIBUTING, changelog/version authority, disclosure and + deletion behavior for plaintext launcher credentials, and a vulnerability + response path. +- Define release artifact contents, supported platforms, SBOM/provenance, + signing/attestation policy, checksums, update manifest generation, rollback, + and release approval. + +### Exit criteria + +- Publicly distributed source and artifacts have an approved licence and + complete notices/provenance inventory. +- Users are told exactly how credentials are stored and removed. +- The launcher updater's production manifest and archives are generated and + verified by a repository-owned release process. + +## 11. R5 — One current documentation authority + +### Work + +- Correct public README claims to the actual Vulkan-only client and retained UI. +- Make `docs/README.md` stable navigation plus a generated current-status block + sourced from one structured milestone/release ledger. +- Keep architecture documents limited to durable boundaries, ownership, + threading/lifetime, and data flow. Move commit/test-count/rollback chronology + to dated closeout records. +- Replace duplicated `AGENTS.md`/`CLAUDE.md` product truth with one maintained + tool-neutral source and generated thin adapters; fail CI on drift. +- Mark every memory/plan/spec as current, active, superseded, or historical. +- Normalize active issue/divergence indexes and validate their IDs, statuses, + paths, and links mechanically. +- Apply the knowledge-preservation protocol before shortening any campaign or + issue record. + +### Exit criteria + +- A new maintainer receives the same current answer from README, documentation + map, architecture, milestone/status ledger, and agent instructions. +- No current authority links missing private `claude-memory`, absent skills, or + developer-local paths. +- Historical records remain searchable but cannot override current truth. + +## 12. R6 — Dead surfaces, diagnostics, and tools + +### Work + +- Decide and then remove or explicitly support the unreachable + `IPanelRenderer`/old panel stack and its tests. +- Remove stale OpenGL/framebuffer/ImGui apparatus and failed temporary-cleanup + markers from shipping assemblies after preserving useful evidence. +- Stop including the smoke plugin in release output by default. +- Classify every tool/script as supported, diagnostic, research-only, or + archived. Repair the five broken C# tools chosen as supported; remove + developer-home/old-worktree paths and document exact prerequisites. +- Centralize environment/diagnostic configuration at composition roots. + +### Exit criteria + +- Shipping assemblies and package contents contain no abandoned presentation + backend or sample plugin by accident. +- Every supported tool builds from the pinned clean checkout. +- Research-only tools are clearly invoked outside the release build. + +## 13. R7 — Plugin and configuration contracts + +### Work + +- Enforce supported plugin API versions before loading code. +- Implement manifest dependencies or remove the unsupported promise. +- Publish/version `AcDream.Plugin.Abstractions` if plugins are advertised. +- Report registration cleanup and callback failures without preventing + best-effort teardown; prove collectible load-context release under failures. +- Replace absent/null allow-list ambiguity with one explicit production default. +- Replace direct hot-path environment reads/process-static mutable diagnostics + with immutable session-scoped configuration and typed sinks. + +### Exit criteria + +- An incompatible plugin fails before activation with an actionable message. +- Disable/dispose reports all cleanup failures and cannot silently retain host + registrations. +- Graphical and headless hosts have the same documented plugin/config default. + +## 14. R8 — Bounded structural decomposition + +This slice begins only after R1–R3. File size alone does not authorize a split. + +### Priority candidates + +- `RuntimeSetPositionState` +- `TransitionTypes` +- `RetailUiRuntime` +- `LiveEntityRuntime` +- `WorldSession` +- `WbDrawDispatcher` + +### Rules + +- Identify one ownership/lifetime or pure-algorithm seam at a time. +- Preserve a single state owner; do not replace a large class with mirrored + mutable state or a service graph of aliases. +- Prefer partial-file navigation when a state machine must remain one owner. +- Establish behavior/sabotage tests before extraction and remove corresponding + temporary source-text freezes afterward. +- Replace Chorizite/GL vocabulary in prepared-content DTOs with versioned + acdream-owned semantics at a separately reviewed boundary. + +### Exit criteria + +- Each extraction reduces change coupling or improves ownership clarity; line + count reduction alone is not success. +- Runtime behavior, retail evidence, allocations, and teardown ledgers remain + equivalent under the relevant focused and complete gates. + +## 15. R9 — Repository artifact migration + +### Work + +- Inventory the approximately 575 MiB of Ghidra state, 299 MiB research tree, + and 151 MiB tracked logs by provenance, sensitivity, reproducibility, and + ongoing value. +- Keep compact fixtures, scripts, tool versions, summaries, and checksums in + Git. Move approved raw bundles to a versioned artifact store. +- Add a manifest/bootstrap command that verifies artifact identity. +- Define retention, redaction, access, and backup policy. +- Treat Git-history rewriting as a separately approved migration with backup, + contributor coordination, remote replacement, and verification. Never do it + as an incidental cleanup command. + +### Exit criteria + +- A normal clone contains what build/test/maintenance requires without opaque + generated databases or raw logs. +- Authorized researchers can retrieve exact approved evidence by manifest and + hash. +- Restricted or non-redistributable material is absent from public history. + +## 16. R10 — Release-candidate gate + +From a fresh clone on every supported release platform: + +- restore with the pinned, locked toolchain; +- build every shipped product and supported tool with zero warnings; +- run the complete hermetic test lane with no failures, silent no-ops, hangs, + or generic retries; +- run and report the applicable DAT, connected, visual/listening, updater, + installer, graceful-shutdown, and rollback gates; +- generate versioned per-RID packages, plugin abstraction package if supported, + SBOM/provenance/checksums, and updater manifest; +- install/update/rollback using only public release instructions; +- verify package contents contain no credentials, developer paths, smoke + plugin, raw probes, or unapproved research artifacts; +- obtain technical and licensing/provenance approval. + +Public release remains **NO-GO** until every blocking slice is closed. + +## 17. Cross-session execution ledger + +This table is the resume authority. Update it in the same commit as every slice +checkpoint. Do not infer status from chat history. + +| Slice | Status | Commit(s) | Evidence/gates | Exact next action | +|---|---|---|---|---| +| R0 | PLAN ACCEPTED; tracking/owner assignment pending | — | Audit complete at `15539a22`; user started R1 | Track plan+audit artifacts; assign R4 owner | +| R1 | IMPLEMENTED + REQUIRED GATES PASS; review/commit pending | — | 2026-08-18 checkpoint below; F-009/T-001 resolved in working tree | Review diff, then commit the coherent R1 checkpoint | +| R2 | NOT STARTED; technically unblocked after R1 review/commit | — | F-010/F-014/F-019 | Start only after R1 checkpoint is accepted | +| R3 | NOT STARTED | — | T-002–T-016; F-015/F-021/F-022/F-030 | Wait for R2 | +| R4 | NOT STARTED; may run parallel | — | F-001/F-026/F-031/F-034 | Assign owner/legal provenance decision path | +| R5 | NOT STARTED | — | F-002/F-003/F-006/F-007/F-011/F-020/F-027/F-029/F-032 | Wait for R2–R3 | +| R6 | NOT STARTED | — | F-004/F-005/F-012/F-016/F-023/F-028 | Wait for R3/R5 | +| R7 | NOT STARTED | — | F-017/F-018/F-024/F-025 | Wait for R3 | +| R8 | NOT STARTED | — | F-008/F-013/F-033 | Wait for R1–R3 and cleanup decisions | +| R9 | NOT STARTED | — | F-034 plus R4 provenance decisions | Wait for R4 | +| R10 | NOT STARTED | — | All blocking findings | Wait for blocking slices | + +### R1 implementation checkpoint — 2026-08-18 + +**Working-tree base:** `15539a22a67f8d915d88f8b1d8126cd55eedda6e` +**Commit:** pending; do not describe this checkpoint as merged or durable until +the source, test, plan, and audit records are committed together. + +Implementation: + +- `LauncherProcessSupervisor.Dispose` now transfers `_process` ownership to a + local and clears the field under `_gate`, then performs stop, event removal, + and child disposal outside the gate. +- Public `Stop` and disposal share one `StopProcess` implementation, preserving + graceful-stop, close-window, timeout, kill, and post-kill observation order. +- `OnProcessExited` reads the event sender's optional exit code without holding + `_gate`, then commits the terminal transition through the existing ordered + state publisher. A callback already captured during teardown may therefore + finish instead of forming the supervisor/Process lock cycle. +- `DisposeAllowsAnAlreadyCapturedExitCallbackToComplete` uses explicit barriers: + the fake captures the exit delegate before unsubscription; its disposal + releases the callback and waits for it to return. There are no timing sleeps + in the oracle, and an emergency release keeps failure against the old code + bounded rather than wedging the test host. + +Sabotage and focused evidence: + +- With the old lock shape temporarily restored and the new test retained, the + test failed with its expected five-second timeout. The fake's cleanup barrier + then released the old cycle, so the test process exited normally. +- With the fix restored, the same test passed in 21 ms. +- The race test passed 25/25 times in fresh `dotnet test` processes. +- All `LauncherProcessSupervisorTests` passed: 22/22. +- Complete Launcher.Core passed under a 180-second hard process bound: + 339 passed / 0 skipped / 0 failed in 52 seconds. + +Complete-solution evidence: + +- Release build completed inside a 300-second bound with 0 warnings / 0 errors + in the evaluated incremental build. +- The exact serialized command was + `dotnet test AcDream.slnx -c Release --no-build --no-restore --nologo -m:1`, + launched in a fresh child process for each run. An outer process watchdog + allowed 900 seconds, killed the complete process tree on expiry, and treated + timeout as failure. +- Run 1: 12 assemblies, 14,748 passed / 77 skipped / 0 failed, 1:28.822, + bounded exit code 0. +- Run 2: 12 assemblies, 14,748 passed / 77 skipped / 0 failed, 1:30.241, + bounded exit code 0. + +Adjacent evidence, deliberately not folded into R1: + +- One default-parallel whole-solution run terminated normally in 58.148 + seconds—important negative evidence for the former hang—but failed one + `AcDream.Launcher.Tests` Avalonia headless cleanup because a compositor was + accessed from a non-owning thread. The Launcher test project then passed + 67/67 alone. R1 makes no Avalonia changes; R2/R3 must decide the supported CI + scheduling and ownership of that pre-existing parallel-run failure. +- The known duplicate Core theory row and 77 skip classifications remain + unchanged and belong to R3. R1 does not use their headline count as proof of + test quality. + +Changed implementation/test files: + +- `src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs` +- `tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs` + +Rollback before commit is the exact reverse of those two file diffs. After a +future commit, record its hash here and use `git revert ` rather than +rewriting history. + +## 18. Session start protocol + +Every implementation session begins by: + +1. reading this plan, the executive audit, and the findings for the active + slice; +2. running `git status --short`, `git rev-parse HEAD`, and checking the ledger's + recorded commit against the working tree; +3. reading all files/tests named by the active finding before editing; +4. confirming there is no overlapping uncommitted user work; +5. restating the bounded slice outcome and gates in the session update; +6. working only the first non-blocked active slice unless the plan explicitly + allows parallel work. + +## 19. Session handoff protocol + +Before ending any session, record in §17 or a linked dated closeout: + +- exact commit/worktree state and every file changed; +- decisions made and alternatives rejected; +- invariant/evidence destinations for anything removed; +- exact commands, pass/fail/skip counts, timeouts, and artifact paths; +- review findings and whether they were closed; +- remaining risks, blockers, and user/legal decisions; +- rollback command or precise reversal procedure; +- one exact next action that can be started without chat context. + +A slice is not `DONE` because its code compiles or a focused test passes. It is +done only when its exit criteria, complete required gate, evidence update, +review, and cross-session ledger entry are all complete. + +## 20. Immediate next action + +Review the R1 diff and checkpoint evidence, then commit the source, regression +test, plan, and audit updates together if accepted. After that, begin R2's +pinned complete gate; assign the R4 decision owner in parallel. Do not begin +bulk comment, artifact, or giant-file cleanup first. diff --git a/docs/reviews/2026-08-17-release-maintainability-audit.md b/docs/reviews/2026-08-17-release-maintainability-audit.md new file mode 100644 index 00000000..7d9f8c95 --- /dev/null +++ b/docs/reviews/2026-08-17-release-maintainability-audit.md @@ -0,0 +1,191 @@ +# acdream release maintainability audit + +Status: **complete — public-release no-go** +Audit baseline: commit `15539a22a67f8d915d88f8b1d8126cd55eedda6e` +Baseline tree: `4de2634ee7f528fe149fc0817247988f8b6bf67f` +Started: 2026-08-17 +Completed: 2026-08-18 + +## Objective + +Determine whether a human development team can safely maintain and release +acdream without relying on undocumented AI conversations, campaign history, or +machine-local context. This is a review-only audit. It does not modify owned +source, tests, configuration, or existing documentation. + +## Deliverables + +- [`coverage-ledger.md`](coverage-ledger.md) records the inspected scope and + completion evidence. +- [`findings-ledger.md`](findings-ledger.md) is the deduplicated evidence-backed + finding register. +- [`architecture-documentation-assessment.md`](architecture-documentation-assessment.md) + compares the documented ownership model with the repository and evaluates + the documentation hierarchy. +- [`comment-reference-inventory.md`](comment-reference-inventory.md) classifies + comments and historical/external references by maintainability value. +- [`test-quality-audit.md`](test-quality-audit.md) records the complete + test-source review and every test-quality exception worth acting on. + +## Review boundaries + +Deep review covers all source-controlled code under `src/` and `tools/`, all +source-controlled test code under `tests/`, the public entry points, active +architecture and planning authorities, and their links into research/history. +Generated binaries, build output, retail data, and external repositories are +not judged as owned code. Their discoverability, licensing, reproducibility, +and use as required evidence are in scope. + +## Evidence and classification rules + +Each finding records an exact path/line or a reproducible repository command, +impact, confidence, and recommended direction. Severity means: + +- **P0 — release blocker:** responsible public release should not proceed. +- **P1 — high:** likely to mislead maintainers or make important changes unsafe. +- **P2 — medium:** material recurring maintenance cost or localized false confidence. +- **P3 — low:** cleanup that improves clarity but does not materially impair work. + +Tests are not called “useless” merely because they are small or repetitive. +That label is reserved for a test that cannot detect a meaningful regression. +Other low-signal tests are classified as misleading, redundant, tautological, +implementation-coupled, obsolete, weakly asserted, or misplaced. + +## Executive conclusion + +acdream is not architecturally hopeless or uniformly “AI spaghetti.” Its +assembly graph has meaningful boundaries, the Runtime/App split is real, and +several difficult lifetime, updater, transport, and retail-fidelity mechanisms +have strong tests. A human team could maintain it after a focused stabilization +program. + +It is not ready for a responsible public release at this baseline. The audit +records 34 findings: **four P0, 25 P1, and five P2**. The four blockers are: + +1. no project licence and unresolved redistribution/provenance for committed + reverse-engineering artifacts (F-001); +2. public setup documentation advertising the deleted OpenGL/ImGui stack + instead of the shipping Vulkan client (F-002); +3. a reproducible launcher supervisor/process-exit lock inversion that hangs + the official complete test process and can affect production disposal + (F-009); and +4. CI does not execute the complete solution gate and omits Core plus most App + tests (F-014). + +The dominant maintainability risk is not one bad algorithm. It is the absence +of a trustworthy release truth: current state is duplicated across enormous +campaign journals, comments depend on missing/private context, tools and SDK +resolution are machine-specific, and headline test counts include hundreds of +contracts that did not execute. + +## What is already solid + +- Production Release compilation succeeds with no production-code warnings. +- The project-reference graph is acyclic; Platform, Headless, Runtime, and + Plugin.Abstractions satisfy their most important declared dependency rules. +- Per-project execution produced 14,747 passes and 77 reported skips. That is a + substantial safety net even after discounting the misleading cases. +- Updater extraction, hashing, transaction/rollback, path safety, launcher + credential redaction, generation gating, and teardown ownership receive + unusually strong adversarial coverage. +- The primary named-retail algorithm corpus is committed and searchable. +- A current NuGet vulnerability lookup reported no known vulnerable direct or + transitive packages, and the repository-wide common token/private-key scan + found no matching secret prefix. These are point-in-time positives, not + substitutes for pinned restore or artifact review. + +## Test verdict + +The test corpus is valuable but its headline number is not a reliable release +claim today. The review found: + +- three entire cases with no meaningful runtime contract (the literal smoke + tautology, the compile-only camera-interface test, and the empty skipped PVS + scaffold), plus useless tautological assertions inside otherwise useful + tests; +- 51 output-only diagnostic methods in the default suite; +- 271 asset/environment-gated facts that report success without exercising + their named contract; +- at least 52 tests for an unreachable deleted presentation stack; +- 30 App source-reading files containing 560 literal-fragment assertions and + 86 ordering-helper calls, including an explicitly temporary architecture + freeze; +- one duplicate theory row silently removed during discovery, 26 clean-rebuild + warnings, seven open load-sensitive tests, and real-time sleeps in timing + contracts; and +- a precise table of contract-like test names whose assertions do not establish + the behavior in their name. + +The exact method/file catalog and classification is in +[`test-quality-audit.md`](test-quality-audit.md). Diagnostics are not called +useless merely because they print useful evidence; they are classified as +misplaced unless they also have a stable oracle. + +## Recommended stabilization order + +### 1. Establish legal and release authority + +Resolve project and research-artifact licensing/provenance; decide which +decompiler/Ghidra/capture artifacts may be distributed; add the public licence, +NOTICE coverage, security policy, contribution policy, changelog/version +authority, credential-storage disclosure, and release runbook. + +### 2. Make one bounded gate truthful + +Fix the launcher deadlock with a deterministic regression test. Pin the .NET +SDK and restore graph, make a clean warning-free build the gate, and run the +complete solution in CI with per-project/process timeouts and hang artifacts. +Turn unavailable DAT/live/environment prerequisites into explicit lanes and +reported skips/failures; remove output-only apparatus from the release count; +own the seven flakes rather than retrying them generically. + +### 3. Repair current documentation before historical cleanup + +Correct the Vulkan/UI/readiness README, generate current status from one +ledger, synchronize or generate the agent instruction wrappers, retire stale +“current truth” memory, and separate durable architecture from campaign +history. Then validate active links and normalize the issue/divergence indexes. + +### 4. Remove abandoned and ambiguous shipping surfaces + +Delete or explicitly support the dead panel stack and stale OpenGL/probe +apparatus; stop embedding the smoke plugin in release output; enforce plugin +API/dependency/lifecycle contracts; centralize diagnostic configuration. Split +giant owners only at proven ownership/lifetime seams, preserving retail +algorithms and sabotage-verified behavior. + +### 5. Make the repository reproducible for another human + +Repair or archive the five broken tool projects, publish exact reference +bootstrap revisions/licences, remove developer-home paths, and move opaque +Ghidra projects/raw logs to an approved versioned artifact system. Keep compact +fixtures, checksums, scripts, and evidence summaries in Git. + +## Scope limitations + +This is a maintainability/release review, not a proof that all retail behavior +is correct and not legal advice. Installed-DAT, live-server, visual, listening, +physical-Linux, and manual generator gates were classified from their source +and recorded evidence but not all re-executed. Package vulnerability results +reflect the configured sources on 2026-08-18. The launcher hang was reproduced +and captured; the seven other documented flakes all passed in the single +per-project audit baseline and remain findings because their own issue records +show recurrence. + +The audit changed only the new records under `docs/reviews/`; it made no edits +to owned source, tests, configuration, existing documentation, commits, or +external systems. + +## Post-audit stabilization update — 2026-08-18 + +R1 has been implemented and fully gated in the current uncommitted working +tree. The launcher child is detached under the supervisor gate and stopped, +unsubscribed, and disposed outside it; a deterministic captured-callback race +test fails against the old mechanism and passes against the fix. Launcher.Core +passes 339/339, and two fresh serialized complete Release runs each finish +inside a 900-second hard bound with 14,748 passes / 77 skips / 0 failures. + +This resolves F-009 technically but does not change the baseline audit's +historical count until the coherent R1 checkpoint is reviewed and committed. +The public-release decision remains no-go because F-001, F-002, F-014, and the +remaining high-priority release-governance findings are untouched. diff --git a/docs/reviews/architecture-documentation-assessment.md b/docs/reviews/architecture-documentation-assessment.md new file mode 100644 index 00000000..ec5cf48e --- /dev/null +++ b/docs/reviews/architecture-documentation-assessment.md @@ -0,0 +1,172 @@ +# Architecture and documentation assessment + +Status: **complete at the recorded baseline**. + +## Declared authority order + +The repository declares milestones authoritative for the active outcome, the +roadmap authoritative for ordering, issues authoritative per defect, and +architecture documents authoritative for implementation shape. Research and +dated plans are historical evidence. This is a reasonable model, but the +current documents duplicate enough mutable state that the order does not +resolve contradictions reliably. + +## Initial architecture-to-repository observations + +| Subject | Documented current contract | Repository evidence | Initial assessment | +|---|---|---|---| +| Graphics backend | Vulkan only; OpenGL deleted | App references `Silk.NET.Vulkan`; Vulkan/RHI source exists | architecture matches code; public README is stale | +| Gameplay UI | one retained stack; ImGui deleted | no `AcDream.UI.ImGui` project; retained UI in App | architecture matches code; README and some rule wording are stale | +| Runtime boundary | Runtime may depend on Core, Core.Net, Content, Platform, Plugin.Abstractions; never App/UI/backend | project/source graph matches; dependency guards pass | confirmed | +| Headless boundary | Headless directly references Runtime only | project graph and executed loaded-assembly guards match | confirmed; operability exceptions recorded separately | +| Platform boundary | Platform has zero references | project graph and complete two-file source review match | confirmed | +| Reference workflow | six vendored reference projects | only uninitialized WorldBuilder gitlink exists | documented process is not reproducible | +| GameWindow shell | architecture checkpoint says 1,622 lines | current file is 1,860 lines | still primarily a composition/callback shell, but checkpoint and navigability claim have drifted | +| UI abstraction | backend-neutral panel/input seam | directly references Runtime and Silk input; old panel renderer has no implementation | boundary name and owned concerns no longer match | +| Presentation stacks | one retained retail UI | retained retail UI ships, but unreachable `IPanelRenderer`/ImGui-era panels remain compiled and tested | dead second design obscures the shipping stack | +| Build authority | .NET 10 solution with warnings-as-errors | no pinned SDK/locked restore/central build settings; policy copied inconsistently across 44 projects | a commit is not a reproducible toolchain definition | +| Issue authority | one authoritative status per defect | 380 headings/378 IDs, about two-thirds closed history, 29 broken links | tactical queue is also an archive and research journal | + +## Documentation-system concerns under review + +1. `README.md` currently contradicts the architecture on the most basic user + prerequisite: Vulkan versus OpenGL and the existence of ImGui. +2. `docs/README.md` is presented as a current-state guard but predates completed + work linked from the same page. +3. `AGENTS.md` is 1,615 lines and embeds a long commit-by-commit status ledger, + rollback commands, local credentials, machine paths, tool instructions, + architectural rules, and reference tutorials. Its normative rules are hard + to distinguish from stale operational history. +4. The architecture document similarly combines stable dependency rules with + current phase state, historical campaign checkpoints, test totals, exact + commits, and future target designs. +5. Required links into `claude-memory/` and `.claude/skills/` are absent from + the baseline worktree. +6. The current milestone, roadmap, and world-interaction plan each preserve a + different pre-closeout state even though the plan itself has a later closeout + section. Authority precedence cannot resolve a contradiction inside every + claimed authority. +7. The mandatory WorldBuilder inventory still gives OpenGL and ImGui design + direction after both were deleted. +8. The issue ledger's status convention is not mechanically enforceable: two + duplicate IDs, nine missing/nonstandard status blocks, and broken links sit + inside a 19,073-line file every session is instructed to scan. +9. The active-document link check found three broken links even before the + historical corpus: the missing `claude-memory/MEMORY.md` map and two missing + Phase O documents linked from roadmap line 505. +10. The retail-divergence authority has correct headline row counts but is not + maintainable as a review surface: 809 KB in 525 physical lines, 243 lines + longer than 1,000 characters, a 45,240-character heading, and 11 unique + unresolved source/test paths among 843 path citations. Its title is six + weeks behind its own latest entries. +11. The approved plugin architecture reads as an implemented contract but is + mostly aspirational. The manifest accepts every positive API version, + declared dependencies are unused, the promised abstractions package has no + pack/version metadata, and callback-fault/hot-reload behavior differs from + the design. +12. The launcher consumes a production GitHub Release manifest, but the + repository contains no production release workflow or manifest generator, + no tags/central product version, and no contribution, security, changelog, + or code-ownership documents. +13. `AGENTS.md` explicitly claims to be synchronized from `CLAUDE.md`, but the + files differ by 255 insertions and 27 deletions. The stale copy restarts a + completed interaction slice and disagrees about reference count, retired + AC2D status, and streaming-radius configuration. This makes the executing + assistant, rather than the repository commit, select project truth. +14. Tracked `memory/` documents labeled “current truth” still prescribe ImGui + frame submission, the deleted UI.ImGui project, OpenGL bindless capability + gates, and resuming the now-complete interaction Slice 4. Because root + instructions route work into these files, they are active contradictions, + not harmless archived notes. +15. A repository-wide scan of all 1,014 tracked Markdown files found 397 + broken relative-link occurrences, representing 357 distinct file/target + pairs. Historical records account for most of the debt: 197 pairs in + `docs/research` and 130 in `docs/superpowers`. The current authority layer + has a much smaller but material set: `docs/README.md` points at the absent + `claude-memory/MEMORY.md`; the roadmap points at two absent Phase O + records; one old handoff points at a deleted VFX test; and one tracked + memory index points at a missing phase audit. `docs/ISSUES.md` contributes + 25 distinct broken Markdown-link pairs. Historical link rot should be + repaired or frozen by support tier, but it is not equivalent in severity + to a broken current-authority link. +16. The dated plans/specs/research system is now 80 top-level plans, 100 + `superpowers/plans`, 96 specs, and 700 research Markdown files. Recent + campaign plans do generally put status near the top, which is good, but + several preserve contradictory status prose in-place (for example an + `IN FLIGHT` label followed immediately by a later `CAMPAIGN CLOSED` + clause). There is no generated active/archive index or schema validation; + readers must interpret narrative amendments to decide which paragraph is + operative. + +## Source-structure census + +The project-reference graph is acyclic and the primary declared assembly +boundaries are present: Platform has no references, Headless references Runtime +only, and Runtime has no App/UI/backend reference. The maintainability risk is +inside several of those correct graph boxes. Source hotspots are highly +concentrated: App has 37 files above 1,000 lines, Runtime 20, Core 13, and the +five largest owned files range from 3,754 to 6,345 lines. Campaign/history +language also remains densest in those owners (2,349 rough source occurrences +in App, 698 in Core, and 491 in Runtime). + +| Assembly | Primary assessment at this checkpoint | +|---|---| +| `AcDream.App` | Layer role is valid, but composition/render/UI/network sidecars remain too concentrated; dead presentation/backend seams, source-shape tests, static diagnostics, and the unconditional smoke-plugin publish make the shipping boundary hard to reason about. | +| `AcDream.Runtime` | Project boundary is real and well guarded, but `RuntimeSetPositionState` (6,283 lines) and several 2,500–3,200-line gameplay/entity owners combine algorithms, state machines, ledgers, and diagnostics beyond a human-scale change unit. | +| `AcDream.Core` | Contains valuable faithful ports with strong oracle citations, but giant physics units, process-static diagnostics, external-type leakage, and campaign/issue narrative obscure the stable algorithms. | +| `AcDream.Core.Net` | Protocol/session ownership is coherent; `WorldSession` remains a 3,754-line transport/lifecycle owner with dense historical comments, direct environment probes, and synchronous shutdown risk that warrants focused refactoring only after behavior is pinned. | +| `AcDream.Content` | Prepared-content boundary is conceptually clean, but public mesh DTOs still expose Chorizite types and GL-ABI vocabulary despite a Vulkan-only consumer. | +| `AcDream.Headless` | Presentation dependency guard is strong. Silent plugin callback failure and process-static/config conventions weaken operability; three files exceed 1,000 lines. | +| `AcDream.Launcher.Core` / `Launcher` | Update/install security and transaction design are unusually thorough, but the full-suite lock inversion is a release blocker and there is no owned production release publisher. Several view-model/orchestrator/update owners are oversized. | +| `AcDream.Platform` | Small, cohesive, and dependency-free as documented. | +| `AcDream.Plugin.Abstractions` | BCL-only and small, but not yet a versioned/published compatibility contract; the approved design substantially overstates the implemented surface. | +| `AcDream.UI.Abstractions` | Input/keybinding pieces remain useful; the project also retains a dead ImGui-era panel stack and directly depends on Runtime/Core, so its name no longer describes one neutral abstraction layer. | +| Bake / CLI / tools | Bake's transaction boundary is understandable though its runner is 1,067 lines. CLI and most forensic tools are campaign utilities without a support tier; many are excluded from the solution or require developer-local paths. | + +## Recommended documentation information architecture + +1. Keep `README.md` public and release-oriented: supported platforms/backend, + install/run path, actual maturity, credential-storage disclosure, licence, + and links to release/security/contribution documents. Do not duplicate + campaign state or rolling test totals there. +2. Make `docs/README.md` a stable navigation page plus a generated, dated + status block sourced from one structured milestone ledger. A CI check should + fail when the block, project graph, backend, links, or reported gate command + disagrees with the repository. +3. Restrict architecture documents to durable component ownership, dependency + rules, threading/lifetime contracts, data flow, and intentional seams. Move + commits, rollback recipes, test counts, and campaign closeout narrative into + dated decision/closeout records. +4. Replace duplicated `AGENTS.md`/`CLAUDE.md` bodies with one tool-neutral + maintained instruction source and generated thin adapters. Agent-only tool + syntax should not carry product truth. +5. Normalize issues and retail divergences into one record per stable ID with + machine-readable status, owner, current rationale, oracle, live symbol/path, + and review date. Generate compact active indexes; archive amendment history + separately. +6. Treat plans, specs, research, probes, raw captures, and memory as explicit + support tiers: active, superseded, immutable evidence, or external artifact. + Validate relative links only against the promises of each tier and prevent a + historical document from calling itself “current truth.” +7. Add one owned release runbook and automation for pinned clean restore/build, + the complete bounded test matrix, installed-DAT/live/manual lanes, per-RID + packages, manifest/checksum/SBOM/provenance generation, rollback, and public + publication. + +## Positive architecture findings + +- The project-reference graph is acyclic and the most important assembly + boundaries are real: Platform is dependency-free, Headless references + Runtime only, Runtime does not reference App/UI/backend assemblies, and + Plugin.Abstractions is BCL-only. +- `GameRuntime` ownership, generation gating, teardown ledgers, updater + transaction design, path traversal/hash validation, and credential redaction + have unusually substantial adversarial test coverage. +- The named-retail oracle is committed and searchable, so the core retail- + fidelity workflow does not depend entirely on the missing external reference + checkouts. The weakness is provenance/licensing and surrounding reference + reproducibility, not absence of the primary algorithm corpus. +- Production code builds without warnings at the baseline; all 26 rebuild + warnings are confined to test projects. Current package-source vulnerability + lookup reported no known vulnerable direct or transitive packages, although + the unpinned restore prevents treating that as a permanent commit property. diff --git a/docs/reviews/comment-reference-inventory.md b/docs/reviews/comment-reference-inventory.md new file mode 100644 index 00000000..fc4f8f69 --- /dev/null +++ b/docs/reviews/comment-reference-inventory.md @@ -0,0 +1,110 @@ +# Comment and reference inventory + +Status: **complete at the recorded baseline**. + +## Classification standard + +A durable source comment should make the local invariant, retail behavior, or +non-obvious constraint understandable without requiring a private conversation. +External provenance is useful when it points to a stable, obtainable source. +Campaign chronology is useful in research/decision records but usually not as +the primary explanation inside production code. + +Comments will be classified as: + +- **Durable invariant/provenance:** explains why/order/math and cites a stable + retail symbol, public document, or owned design contract. +- **Useful but context-dependent:** technically valuable, but requires an issue, + campaign, commit, or unavailable reference to understand. +- **Historical residue:** describes how an older implementation changed rather + than what the current code guarantees. +- **Diagnostic/apparatus residue:** names a temporary probe, rejected attempt, + manual gate, or one-off capture in maintained code. +- **Misleading/stale:** contradicts current code or an active authority. +- **Noise:** restates code, uses phase labels as structure, or preserves no + decision a maintainer needs. + +## Initial inventory + +A Roslyn trivia scan over all 1,325 owned C# files found 37,734 actual comment +trivia nodes, including 9,516 XML-documentation comments. Of those comments, +2,172 contain a campaign/slice/phase/checkpoint or commit-hash reference, 1,016 +contain an issue-number reference, 1,049 name an external/reference oracle, +617 contain probe/temporary/workaround/diagnostic language, 143 contain +OpenGL/ImGui or GL-owner terminology, and 16 contain TODO/FIXME/HACK/XXX +markers. Categories overlap and occurrence does not by itself make a comment +bad; the figures identify the review surface without matching strings or +identifiers. + +| Area | Comment trivia | Campaign/history | Issue refs | External refs | Old graphics | Probe/workaround | +|---|---:|---:|---:|---:|---:|---:| +| App | 18,009 | 1,215 | 480 | 121 | 107 | 244 | +| Core | 8,723 | 399 | 322 | 452 | 4 | 224 | +| Runtime | 5,813 | 227 | 123 | 112 | 1 | 46 | +| Core.Net | 2,545 | 156 | 37 | 328 | 0 | 23 | +| UI.Abstractions | 899 | 69 | 12 | 11 | 22 | 18 | +| Headless | 534 | 42 | 21 | 13 | 0 | 29 | +| All remaining source/tools | 1,211 | 64 | 21 | 12 | 9 | 33 | + +High-signal examples already queued for review include: + +- `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs`, whose comments mix retail + symbols and durable draw invariants with issue numbers, campaign slice + migrations, deleted GL-arm history, and named throwaway probes across a + 3,773-line class. +- `src/AcDream.Headless/Policies/HeadlessBotPolicy.cs`, whose production XML + documentation contains campaign contracts, temporary evidence gathering, + an open issue, and disabled diagnostic behavior. +- Tool entry points that identify themselves primarily by the issue/campaign + that created them, without a maintained support/archival classification. +- `src/AcDream.Core.Net/WorldSession.cs` contains 69 campaign-history comments + and 47 external-reference comments inside a 3,754-line transport/session + owner; `VendorUiController.cs` contains 71 campaign-history comments. +- `AcDream.UI.Abstractions` still documents ImGui capture and panel behavior in + interfaces used by the retained client after the ImGui backend was deleted. + +The exact project counts and confirmed high-risk classes above are the final +inventory. It intentionally does not propose mass deletion: durable retail +symbols and local invariants are valuable, while stale backend claims, +personal paths, unavailable-reference paths, campaign narration, and failed +temporary-cleanup markers require targeted correction. + +## Confirmed reference-context problems + +- “holtburger” appears 163 times across 52 owned source files and 50 times + across 23 test files. Twenty-three source/test files contain a literal + `references/holtburger/...` path, but the only tracked reference root is the + uninitialized `references/WorldBuilder` gitlink. The citations are often + valuable independent corroboration, but a new maintainer cannot inspect the + cited code, commit, or license from this checkout. Examples include + `Core/Chat/CombatChatTranslator.cs:15-16`, + `Runtime/Chat/ChatInputParser.cs:6-7`, and numerous Core.Net wire-layout + comments. Stable public URL+revision or an owned evidence extract is needed; + a private/missing working-copy path is not durable provenance. +- Current source comments still describe deleted behavior. Examples include + `UiDatElement.cs:24-29` claiming `TextureCache.UploadRgba8` sets + `GL_REPEAT`, `FrameScreenshotController.cs:7-10,61,123-247` documenting a GL + thread/default-framebuffer path no production caller uses, + `FrameProfiler.cs:11-30` retaining an ImGui timing stage, and the dead panel + interfaces described in F-012. These are misleading, not merely historical. +- `RenderingDiagnostics.cs` and `PortalVisibilityBuilder.cs` contain multiple + comments explicitly ordering future maintainers to strip “throwaway” or TEMP + apparatus after May/June investigations that are already closed. The code + remained through the Vulkan cutover, so those comments are failed cleanup + markers rather than actionable ownership. F-016 records the executable debt. +- Campaign/issue references are useful as secondary provenance, but many source + comments make the chronology the primary explanation (for example the long + “Night-round review F14” house-marker note in + `MapPageController.cs:449-481`). A durable local invariant and current + reachability/support status should lead; campaign labels and dates belong in + linked research or decision records. +- Personal-machine paths are also embedded as provenance. The literal + `C:\Users\erikn` occurs in 146 documentation files, 43 tool files, eight + production-source files, two test files, and both root instruction files. + Production examples in `RetailCommandHelpTable`, + `RetailClientCommandCatalog`, `WeenieErrorMessages`, `OptionsPanelText`, + `ClientTextRefusals`, `AudioSettings`, `ConfigOptionsPageController`, and + `VendorUiController` cite a binary in the maintainer's Downloads directory. + Several comments also give a version or CodeView identity, which is the + durable evidence; the personal path adds no reproducible provenance and + implies access that another maintainer does not have. diff --git a/docs/reviews/coverage-ledger.md b/docs/reviews/coverage-ledger.md new file mode 100644 index 00000000..6397156e --- /dev/null +++ b/docs/reviews/coverage-ledger.md @@ -0,0 +1,147 @@ +# Coverage ledger + +Audit baseline: `15539a22a67f8d915d88f8b1d8126cd55eedda6e`. + +The baseline commit and path-set fingerprints make the reviewed file universe +reproducible without copying thousands of paths into this document: + +| Scope | Fingerprint (`git ls-files ... | git hash-object --stdin`) | Files/lines at baseline | +|---|---|---:| +| `src/**`, `tools/**` | `b79a0390fad9a3e86de765e49f869720045bbc94` | 1,325 C# files / 416,628 lines | +| `tests/**` | `34b425e434a7a3980e556a1ce445f2a40ae196fd` | 1,260 C# files / 402,131 lines | +| `docs/**`, root authorities, `memory/**` | `46b6c109b8b986a9425c46e66cbe6967e5ae3d46` | 1,127 tracked paths | + +The repository has 4,018 tracked paths: 1,365 under `src`, 1,352 under +`tests`, 1,109 under `docs`, 152 under `tools`, 15 under `memory`, one +WorldBuilder gitlink, and 24 root/editor/CI paths. + +## Owned production and tool code + +“Inventory complete” means the file set, project references, package +references, size distribution, and solution membership were collected. +“Reviewed” means every file in the row participated in the all-file structural, +comment/reference, configuration, failure-handling, and dependency scans, with +manual review of entry points, public boundaries, exceptions, and size/ +complexity hotspots. + +| Area | C# files | Lines | Inventory | Deep review | +|---|---:|---:|---|---| +| `src/AcDream.App` | 628 | 205,466 | complete | complete | +| `src/AcDream.Bake` | 11 | 1,884 | complete | complete | +| `src/AcDream.Cli` | 6 | 1,736 | complete | complete | +| `src/AcDream.Content` | 31 | 7,533 | complete | complete | +| `src/AcDream.Core` | 275 | 78,848 | complete | complete | +| `src/AcDream.Core.Net` | 98 | 21,781 | complete | complete | +| `src/AcDream.Headless` | 35 | 8,859 | complete | complete | +| `src/AcDream.Launcher` | 15 | 3,342 | complete | complete | +| `src/AcDream.Launcher.Core` | 50 | 13,661 | complete | complete | +| `src/AcDream.Platform` | 2 | 206 | complete | complete | +| `src/AcDream.Plugin.Abstractions` | 8 | 147 | complete | complete | +| `src/AcDream.Plugins.Smoke` | 1 | 34 | complete | complete | +| `src/AcDream.Runtime` | 112 | 62,894 | complete | complete | +| `src/AcDream.UI.Abstractions` | 37 | 6,294 | complete | complete | +| `tools/*` | 16 | 3,903 | complete | complete | + +The non-C# owned tooling surface contains 84 tracked executable scripts: 38 +WinDbg/CDB command files, 30 PowerShell scripts, and 16 Python scripts across +the repository root and `tools/`. These are included in the tool review even +though they are not represented by the C# path-set count above. Initial +execution/readability triage is complete; support classification and exact +prerequisites are classified in F-004. All 13 tracked tool projects were also built +individually: eight succeed and five fail (three absent relative +DatReaderWriter references and two source-API drift errors). Two of the eight +passes depend on a developer-specific absolute project path that happens to +exist on the audit machine. Script triage found absolute Windows paths in 55 of +84 files and explicit developer-home paths in 43. + +Size is already an audit dimension: 83 owned C# files are at least 1,000 +lines, 24 are at least 2,000, and nine are at least 3,000. Size alone is not a +finding; cohesion, ownership, and change coupling will determine which files +are flagged. + +## Test source + +| Test area | C# files | Lines | Inventory | File-by-file review | +|---|---:|---:|---|---| +| `AcDream.App.Tests` | 515 | 174,271 | complete | complete | +| `AcDream.Bake.Tests` | 5 | 665 | complete | complete | +| `AcDream.Cli.Tests` | 1 | 178 | complete | complete | +| `AcDream.Content.Tests` | 29 | 6,120 | complete | complete | +| `AcDream.Core.Net.Tests` | 115 | 26,976 | complete | complete | +| `AcDream.Core.Tests` | 398 | 99,802 | complete | complete | +| `AcDream.Headless.Tests` | 18 | 9,573 | complete | complete | +| `AcDream.Launcher.Core.Tests` | 27 | 8,615 | complete | complete | +| `AcDream.Launcher.Tests` | 7 | 2,123 | complete | complete | +| `AcDream.Platform.Tests` | 2 | 224 | complete | complete | +| `AcDream.Runtime.Tests` | 91 | 64,102 | complete | complete | +| `AcDream.UI.Abstractions.Tests` | 46 | 8,583 | complete | complete | +| Fixture/helper projects and `tests/Fixtures` | 6 | 899 | complete | complete | + +The exact baseline set is every path returned by +`git ls-files 'tests/**/*.cs'`; its fingerprint is recorded above. There are +68 test C# files at least 1,000 lines, 16 at least 2,000, and nine at least +3,000. Parameterized rows and generated/live-DAT gates will be audited through +their data sources as well as their declaring methods. + +## Documentation and repository infrastructure + +| Area | Inventory | Review | +|---|---|---| +| Root `README.md`, `NOTICE.md`, `AGENTS.md`, `CLAUDE.md` | complete | complete | +| `docs/README.md`, milestones, roadmap, issues | complete | complete | +| Architecture and divergence authorities | complete | complete | +| Active M4 plans/specifications | complete | complete | +| Historical plans, research, and audit records | complete | complete classification/link scan | +| `memory/` and documented `claude-memory/` links | complete | complete | +| Solution/project graph and CI | complete | complete | +| External/generated/reference material | complete | complete | + +## Initial inventory facts + +- `AcDream.slnx` contains all 14 owned `src` projects, 16 test/fixture + projects, and only one of the 13 tracked tool projects (`RetailTimeProbe`). +- `references/WorldBuilder` is the sole tracked reference gitlink and is not + initialized in this worktree. No other documented reference checkout is + present. +- Five tracked tool projects reference a nonexistent + `references/DatReaderWriter` project; two encode a developer-specific + absolute path. +- The repository-instruction audit skill named at `AGENTS.md:725` is absent, + as is the `claude-memory/` tree linked by the documentation map. +- The solution Release build was executed at the baseline: it succeeds with + 26 warnings, all emitted by test projects. +- Every test assembly was executed. Eleven complete normally in the combined + run; Launcher.Core passes 338/338 alone but deadlocks in the combined run. + Current reproducible totals are 14,747 passing and 77 reported skipped, plus + one duplicate theory row dropped during discovery. +- All 11,458 declared fact/theory-like methods were syntax-inventoried for + assertion signals, diagnostic naming, literal tautologies, conditional + execution, skips, and timing primitives. The conditional-execution pass + found 314 method-level empty returns in 294 methods across 107 files; 271 of + those methods silently pass when an asset or opt-in environment gate is not + available. This cross-file triage supplied the exception set for the + completed semantic follow-up below. + +## Completion evidence + +The semantic follow-up is complete. It manually reviewed every exceptional +class produced by the all-file matrix: no-oracle methods and their helpers, +empty returns, explicit/custom skips, environment and installed-DAT gates, +source-text assertions, sleeps/real-time deadlines, random/concurrent tests, +literal tautologies, duplicate data, diagnostic naming, dead-stack tests, and +the open flake ledger. The exact actionable catalog is in +`test-quality-audit.md`; valid no-throw and deterministic-repeatability tests +were deliberately excluded from the “useless” label. + +Documentation coverage used a full 1,014-file Markdown relative-link scan and +manual reading of the authority chain, current campaign/status documents, +repository instructions, memory entry points, issue/divergence ledgers, CI, +release/update paths, and representative historical records. Historical +research was classified and mechanically checked rather than line-edited as +if it were current architecture. + +Build/test execution, package-vulnerability lookup, project/tool builds, +repository-size/provenance census, secret-pattern scan, and managed-stack +capture for the launcher hang provide executable corroboration. No source, +test, configuration, existing documentation, commit, or external system was +modified by this audit. diff --git a/docs/reviews/findings-ledger.md b/docs/reviews/findings-ledger.md new file mode 100644 index 00000000..b39d4d5a --- /dev/null +++ b/docs/reviews/findings-ledger.md @@ -0,0 +1,844 @@ +# Findings ledger + +This ledger is complete for baseline +`15539a22a67f8d915d88f8b1d8126cd55eedda6e`. It contains 34 deduplicated +findings: four P0, 25 P1, and five P2. Absence from the ledger does not certify +behavioral correctness; it means the repository-wide maintainability review +did not find an actionable exception beyond the documented classes. + +## F-001 — No top-level source licence + +- **Severity:** P0 release blocker +- **Confidence:** high +- **Category:** release/legal packaging +- **Evidence:** `README.md:289-291` explicitly states that acdream has not + been assigned a top-level licence and is not ready for public redistribution. + `git ls-files` contains no `LICENSE` or `LICENSE.md`. The repository also + commits roughly 113 MB of decompiler output in + `docs/research/named-retail/acclient.c` and + `acclient_2013_pseudo_c.txt`, plus PDB-derived symbols/types; `NOTICE.md` + covers only WorldBuilder-derived MIT code and records no provenance or + redistribution basis for these research artifacts. +- **Impact:** the repository describes itself as open source but its own public + entry point says it cannot yet be redistributed. A public release cannot meet + the project's stated goal until ownership and licensing are resolved. +- **Recommended direction:** select and add an approved project licence, audit + provenance of extracted/adapted code and bundled artifacts, and make NOTICE + and dependency attribution consistent with it before release. + +## F-002 — Public setup documentation describes a deleted renderer and UI + +- **Severity:** P0 release blocker +- **Confidence:** high +- **Category:** user documentation / reproducibility +- **Evidence:** `README.md:24-37`, `README.md:67`, `README.md:100`, + `README.md:212`, and `README.md:237` advertise mandatory OpenGL 4.3 and + ImGui developer tools, including a nonexistent `src/AcDream.UI.ImGui/`. + `AGENTS.md:51-53` and + `docs/architecture/acdream-architecture.md:67-107` say OpenGL and ImGui were + deleted and Vulkan is the only backend. `src/AcDream.App/AcDream.App.csproj` + references Silk.NET Vulkan packages and no OpenGL package. +- **Impact:** a contributor following the release README is told to provision + the wrong graphics capabilities and expect a UI that cannot exist. This + blocks reliable onboarding and makes support reports ambiguous. +- **Recommended direction:** establish one generated or checked public + capability description sourced from the actual project/backend manifest; + remove deleted project/options claims and document the current Vulkan debug + behavior. + +## F-003 — The designated “current” documentation map is chronologically stale + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** documentation authority +- **Evidence:** `docs/README.md:7` labels its snapshot 2026-07-27 and + `docs/README.md:8-15` says world-interaction Slices 4-6 remain. The linked + authority records all six slices complete and user-accepted at + `docs/plans/2026-07-23-world-interaction-completion.md:737`. That plan's own + opening status still says “Slices 1-4 are complete; resume at Slice 5” + (`:3-21`), and the roadmap updated 2026-08-14 still calls the program paused + with only Slices 1-3 complete (`docs/plans/2026-04-11-roadmap.md:169-233`). + The milestones document likewise says Slices 1-4 are accepted and vendor + work remains (`docs/plans/2026-05-12-milestones.md:80-121`). The audit + baseline is 2026-08-17. `docs/README.md:45-46` and `README.md:114-116` also + retain the older 8,826-test baseline. +- **Impact:** the page explicitly intended to prevent old status banners from + overriding current truth does exactly that. Humans cannot reliably infer + current work or release scope from the documented authority order. +- **Recommended direction:** separate timeless navigation from generated + current status; make current claims mechanically checked against the + milestone/roadmap or remove duplicated status numbers and slice summaries. + +## F-004 — Tracked tools are not reproducible from a fresh checkout + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** build graph / developer tooling +- **Evidence:** `tools/dump-keymap/dump-keymap.csproj:11`, + `tools/SkyObjectInspect/SkyObjectInspect.csproj:12`, and + `tools/WeatherSetupProbe/WeatherSetupProbe.csproj:12` reference an absent + relative `references/DatReaderWriter` checkout. + `tools/StarsProbe/StarsProbe.csproj:12` and + `tools/WeatherEnumerator/WeatherEnumerator.csproj:12` reference + `C:\Users\erikn\source\repos\acdream\...`. Only `RetailTimeProbe` is in + `AcDream.slnx`, so the documented release build cannot expose these failures. + Building all 13 tracked tool projects at the baseline produces five failures: + the three absent relative references above, plus API-drift compile errors in + `PesChainAudit` and `RainMeshProbe`. The two absolute-reference projects pass + only because that unrelated source checkout exists on this audit machine. + The other eight build. Separately, the 84 tracked PowerShell/Python/CDB + scripts contain 107 absolute Windows-path tokens across 55 files and 82 + developer-home path tokens across 43 files. + Three tracked root `launch-a6-issue98-*.ps1` scripts also hard-code an old + `.claude\worktrees\strange-albattani-3fc83c` checkout, and the three + `launch-flap-*.ps1` scripts are dated one-off probe launchers which rely on + now-historical environment switches and `--no-build` output. +- **Impact:** important forensic tools silently depend on one developer's + machine and can decay without CI visibility. Human maintainers cannot know + which tools are supported or buildable. +- **Recommended direction:** classify supported versus archival probes; + migrate supported tools to package/owned project references and include a + dedicated tool build gate. Move truly historical one-off probes out of the + maintained build surface with an explicit archival label. + +## F-005 — Mandatory reference workflow depends on repositories absent from the repository + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** reproducibility / external context +- **Evidence:** `AGENTS.md:1481-1546` says `references/` holds six vendored + projects and requires domain work to cross-reference them. The tracked tree + contains only the `references/WorldBuilder` gitlink (`.gitmodules:1-4`), and + it is uninitialized at the audit baseline. Its URL is a personal SSH fork + (`git@github.com:eriknihlen/WorldBuilder.git`) rather than the public upstream + named by `NOTICE.md`, so even this one reference assumes account/key access. + `README.md:252` instead calls the references gitignored external repositories. +- **Impact:** the mandatory implementation and verification process cannot be + followed from a clone, while two top-level documents disagree about how the + prerequisites are obtained. Source comments and research citations that rely + on those names become inaccessible institutional memory. +- **Recommended direction:** publish a reproducible reference bootstrap + manifest with exact revisions, licences, and optional/required status. Make + docs distinguish vendored, submodule, package, machine-local, and historical + references. + +## F-006 — Canonical documents link unavailable private/agent context + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** undocumented context / onboarding +- **Evidence:** `docs/README.md:79-84` sends maintainers to + `../claude-memory/MEMORY.md`; that path is absent. `AGENTS.md:725` mandates + `.claude/skills/investigate/SKILL.md` for audits; that path and skill are + absent. Numerous instructions also name Claude-only tools and machine-local + paths. +- **Impact:** the documented “start here” chain leaves a human or clean agent + without purportedly durable subsystem truth and mandatory procedure. This is + exactly the hidden AI-context dependency the audit is intended to detect. +- **Recommended direction:** move durable engineering knowledge into tracked, + tool-neutral repository documents. Treat optional personal memory and agent + skills as accelerators, never required authorities. + +## F-007 — Architecture authority mixes current contract, stale state, and campaign journal + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** architecture documentation +- **Evidence:** `docs/architecture/acdream-architecture.md:79` retains the + heading “two coexisting presentation stacks” while lines 83-107 say there is + one. Line 461 calls L.2 the current organizing program while current milestone + documents name M4. Lines 1299-1301 repeat an obsolete 1,622-line/7,823-test + checkpoint and say a final visual matrix remains. The current + `GameWindow.cs` has 1,860 lines and repository authorities state that matrix + completed. +- **Impact:** a document declared the single source of truth cannot be read as + a stable contract without disentangling historical checkpoints. Maintainers + may preserve obsolete boundaries or restart completed work. +- **Recommended direction:** split normative architecture from dated decision + records and generated status. Keep ownership invariants concise; link to + historical campaign evidence rather than embedding its evolving ledger. + +## F-008 — Extreme source and test unit size is widespread + +- **Severity:** P2 medium (systemic; split points still require owner judgment) +- **Confidence:** high +- **Category:** human comprehension / change risk +- **Evidence:** 83 owned C# files are at least 1,000 lines, 24 at least 2,000, + and nine at least 3,000. Largest examples are + `src/AcDream.Core/Physics/TransitionTypes.cs` (6,345), + `src/AcDream.Runtime/Physics/RuntimeSetPositionState.cs` (6,283), + `src/AcDream.App/UI/RetailUiRuntime.cs` (4,611), and + `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` (3,773). Test code has 68 + files at least 1,000 lines and nine at least 3,000. This is not only long + retail pseudocode: a structural count finds 32 types/170 methods in + `RuntimeSetPositionState.cs`, 27/115 in `RetailUiRuntime.cs`, 38/155 in + `LiveEntityRuntime.cs`, and 17/153 in `WorldSession.cs`, with no region or + generated-file boundary. `RetailUiRuntime` also contains 51 lock sites. The + largest test units mirror the same owners rather than providing small, + navigable contract groupings: `RuntimeInitialCreateContinuationExecutorTests` + is 4,765 lines, `HeadlessSessionHostTests` 3,601, + `RuntimeSetPositionStateTests` 3,522, `RuntimePhysicsStateTests` 3,198, and + `RuntimeRemotePlacementDriveControllerTests` 3,070. +- **Impact:** large units are difficult to navigate and review, but retail + algorithm cohesion and lifecycle invariants may justify some. The risk is + systemic enough to require file-by-file cohesion review rather than a blind + line-count rule. +- **Recommended direction:** after the cohesion review, split only along real + ownership or lifecycle seams; use partial files for navigability where a + single state machine must remain one owner. + +## F-009 — Launcher child exit and supervisor disposal can deadlock + +- **Severity:** P0 release blocker +- **Confidence:** high (captured live managed stacks) +- **Category:** concurrency / launcher lifecycle +- **Evidence:** a normal full Release test run hung twice in + `LauncherProcessSupervisorTests.ANullStderrLogPathBehavesExactlyAsBeforeForBothChildProcessKinds` + (`tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs:668`). + A `dotnet-stack` capture showed the test thread in + `WindowsSystemChildProcess.Dispose` -> `System.Diagnostics.Process.Dispose` + while holding `LauncherProcessSupervisor._gate` from + `LauncherProcessSupervisor.Dispose` (`src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs:335-341`). + The process-exit callback thread was inside the runtime Process callback, + blocked entering the same supervisor gate through + `OnProcessExited` -> `SetState` -> `PublishPendingStateChanges` + (`LauncherProcessSupervisor.cs:215-225`, `241-279`). The child wrapper + unregisters and disposes the Process at + `WindowsSystemChildProcess.cs:179-205`. +- **Impact:** this is a classic lock-order inversion between the supervisor + lock and `System.Diagnostics.Process`'s internal synchronization. A launcher + session that exits concurrently with disposal can hang shutdown indefinitely; + the official full-suite command can also hang rather than report a result. +- **Recommended direction:** detach the owned child under `_gate`, then + unsubscribe/dispose it outside `_gate`; add a deterministic two-party race + test that proves both exit-callback and disposal orders converge. Audit the + ordinary `SystemChildProcess` wrapper for the same inversion. + +**Post-baseline resolution checkpoint (2026-08-18, uncommitted):** the working +tree now performs the ownership transfer and all child operations outside +`_gate`; `OnProcessExited` obtains its optional exit code without holding that +gate. A barrier-controlled regression captures the exit delegate before +unsubscription and makes child disposal wait for that callback. It times out in +five seconds with the old lock shape, passes in milliseconds with the fix, and +passed 25/25 fresh-process repetitions. Launcher.Core passes 339/339. Two +fresh serialized complete-solution runs each finished under a 900-second hard +bound with 14,748 passes / 77 skips / 0 failures in 1:28.822 and 1:30.241. +F-009 remains a baseline finding until the coherent R1 checkpoint is reviewed +and committed; the current working-tree mechanism and required gates are +resolved. + +## F-010 — Published build/test baseline materially overstates the portable gate + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** test reporting / release confidence +- **Evidence:** the baseline Release build succeeds but emits 26 warnings, + while `README.md:114-116`, `docs/README.md:45-46`, and issue #228 describe + 17. A portable Release test run reports 5,510 App passes / 76 skips and + 4,797 Core passes / one skip. The complete per-project total is 14,747 + passes / 77 reported skips when the separately successful 338 Launcher.Core + cases are included; the all-solution process did not finish because of + F-009. xUnit additionally reports and drops one duplicate-ID theory row, + which is not included in its skipped total. +- **Impact:** “all tests pass / five intentional skips” is not a reproducible + statement for a clean contributor environment. Optional installed-DAT/GPU + tests, permanent scaffolds, known regressions, manual generators, and a + duplicate data row are collapsed into one obsolete number, hiding which + release properties were actually exercised. +- **Recommended direction:** publish separate portable, installed-DAT, + GPU/visual, connected, and manual-tool gates from machine-readable CI output. + Fail on unexpected skips and duplicate discovery IDs; do not hand-edit test + totals into multiple living documents. + +## F-011 — Mandatory WorldBuilder inventory is an obsolete OpenGL design guide + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** architecture documentation / renderer ownership +- **Evidence:** `AGENTS.md:32-38` requires reading + `docs/architecture/worldbuilder-inventory.md` before rendering or DAT work. + That inventory calls `src/AcDream.App/Rendering/Wb` “GL infrastructure” + (`:21-27`), describes GL upload and GL-owned atlas records (`:43-101`), + inventories the deleted `Chorizite.OpenGLSDLBackend` and recommends taking + its OpenGL renderer/wrappers (`:343-469`), and says gameplay UI is + ImGui-based (`:538-564`). `NOTICE.md:16` repeats the GL description. The + binding architecture and project graph are Vulkan-only. +- **Impact:** this is not merely an old plan: it is a mandatory implementation + gate which tells maintainers to copy or preserve a deleted backend and UI + model. Its useful extraction/provenance inventory is inseparable from stale + technical direction. +- **Recommended direction:** retain a concise current inventory of owned ports, + provenance, and “do not re-port” rules. Move the pre-Vulkan component survey + into an explicitly historical decision record, and make current RHI/Vulkan + ownership the only normative rendering guidance. + +## F-012 — A deleted presentation stack remains as unowned production code and tests + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** dead architecture / false test confidence +- **Evidence:** `AcDream.UI.ImGui` and the ImGui renderer were deleted, and + `src/AcDream.App/Composition/SettingsDevToolsComposition.cs:8-18` says no + `IPanelRenderer` implementation remains. Nevertheless + `AcDream.UI.Abstractions` retains 1,021 lines across `IPanel`, `IPanelHost`, + `IPanelRenderer`, `PanelContext`, `ChatPanel`, `DebugPanel`, and + `VitalsPanel`. Repository-wide source search finds no `IPanelHost` or + `IPanelRenderer` implementation and no production construction of any of + those three panels. At least 52 declared tests across the renderer/chat-panel + fixture set exercise this unreachable presentation path. +- **Impact:** humans see two UI designs in maintained production code despite + the one-stack architecture. Green panel tests can be mistaken for coverage + of the retained retail UI that actually ships, while changes to the dead + stack consume review and maintenance effort. +- **Recommended direction:** decide whether this is a real supported public + extension contract. If not, remove/archive the unreachable panel renderer + and its presentation-only tests while retaining shared VMs/settings that the + retail UI uses. If it is supported, name its owner, implement it in a shipping + host, and give it an explicit compatibility promise. + +## F-013 — `UI.Abstractions` is neither backend-neutral nor a cohesive boundary + +- **Severity:** P2 medium +- **Confidence:** high +- **Category:** dependency architecture / naming +- **Evidence:** `src/AcDream.UI.Abstractions/AcDream.UI.Abstractions.csproj` + references `AcDream.Core`, the higher-level `AcDream.Runtime`, and + `Silk.NET.Input`. Its public input contracts expose Silk `Key` and + `MouseButton` (`Input/KeyChord.cs`, `Input/InputDispatcher.cs`), its global + usings import `AcDream.Runtime.Chat`, and the same assembly also owns JSON + settings persistence, gameplay VMs, and the dead immediate-mode panel + protocol in F-012. The plugin UI contract instead lives in the BCL-only + `AcDream.Plugin.Abstractions` assembly. +- **Impact:** the assembly name suggests a small stable presentation seam, but + it couples platform input, runtime gameplay, persistence, and abandoned UI. + Changes propagate across unrelated concerns and make dependency rules harder + for a human to infer from project names. +- **Recommended direction:** define the supported contracts first, then split + backend-free input identities, application settings/storage, reusable + gameplay view models, and any real presentation protocol along their actual + consumers. Keep Silk adapters in App or a platform-input adapter assembly. + +## F-014 — CI does not run the repository's complete release test gate + +- **Severity:** P0 release blocker +- **Confidence:** high +- **Category:** continuous integration / release verification +- **Evidence:** `.github/workflows/headless-portability.yml` is the only + deterministic build/test workflow. Its presentation-free lane runs selected + project tests but explicitly omits `AcDream.Core.Tests` (`:114-133`). The + Linux graphical lane runs all `UI.Abstractions` tests but only five filtered + App test classes (`:251-262`); the Vulkan lane runs only App Vulkan tests + (`:368-375`). No workflow invokes `dotnet test AcDream.slnx`, the 4,797 Core + and most of the 5,510 App tests are absent from PR/push gates, and CLI tests + are absent as well. The daily hygiene workflow is an AI assessment, not a + deterministic required full-suite job. +- **Impact:** a change can merge while breaking the principal physics, UI, + rendering, or integration suites. The only local all-solution command can + currently deadlock under F-009, so there is no independent automated release + proof to catch it. +- **Recommended direction:** add a bounded, deterministic Release solution + build/test job on every supported OS row, publish TRX and skip manifests, and + split environment-dependent gates explicitly. Give every job a hang timeout + and collect dumps; make the portable complete gate required before release. + +## F-015 — Headline pass totals include hundreds of unexecuted test contracts + +- **Severity:** P1 high +- **Confidence:** high (syntax-tree inventory plus executable baseline) +- **Category:** test reporting / false confidence +- **Evidence:** 294 attributed methods across 107 test files contain 314 + method-level empty returns. Of these, 248 methods in 85 files return when a + DAT directory, prepared package, or fixture is absent, and 23 methods in nine + files return when an opt-in environment variable is unset. xUnit records + these as passes. The three `LiveHandshakeTests` return at lines 38, 115, and + 265 unless `ACDREAM_LIVE=1`; a source comment even calls this “skipped,” yet + Core.Net reports 1,004 passes and zero skips. The same pattern spans all + three Bake determinism facts and large App/Core DAT-backed suites. Separately, + 73 `InstalledDatFact` methods produce real skips but all inherit one + campaign-specific “LA8 gate” reason from + `CharacterManagementLiveDatTests.cs:367-383`. +- **Impact:** a clean or CI machine can report a green contract it never + executed, so neither the 14,747-pass baseline nor a future CI total proves + the named rendering, physics, bake, layout, or live-network behavior. This + obscures asset provisioning failures and makes release confidence dependent + on undocumented machine state. +- **Recommended direction:** replace empty-return gates with explicit dynamic + skips or failures, assign traits and prerequisite-specific reasons, and emit + separate portable/installed-DAT/prepared-package/live-network manifests. + Treat an unexpected zero-execution group as a failed release gate. + +## F-016 — Deleted OpenGL and temporary probe apparatus remain in shipping assemblies + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** dead code / diagnostics / misleading comments +- **Evidence:** `FrameScreenshotController.cs:123-247` retains an entire + `IDefaultFramebufferSurface`/`ReadDefaultFramebuffer` OpenGL resolve path; + repository-wide source search finds no production call, only four tests. + `WorldPassSurface.cs:7-18,198-242` keeps `IRenderFrameGlState` solely so the + only implementation can be an empty `NullRenderFrameGlState`, and keeps a GL + state reader whose production result is always `default` on Vulkan. + `WorldRenderDiagnostics.cs` still models and formats that nonexistent state. + `FrameProfiler.cs:11-30,199-213,289-296` retains an `ImGui` stage and CSV + column although the frontend is deleted. Most significantly, + `Core/Rendering/RenderingDiagnostics.cs` is 834 lines and declares 33 + environment-backed render probe reads; its comments repeatedly say + “throwaway apparatus — strip once ...” and describe GL/FBO/SSBO failure + hypotheses from May/June. `PortalVisibilityBuilder.cs:268-272,1055-1062` + still carries a static TEMP A8 dump switch/dictionary and another explicitly + throwaway flap probe. +- **Impact:** a human maintainer must reason about backend contracts and + instrumentation that cannot observe the current renderer, while tests make + some deleted-backend helpers appear supported. Static process-wide probe + state also conflicts with the documented multi-session runtime model and + expands hot code paths for closed investigations. +- **Recommended direction:** inventory each probe by an active, reproducible + support need; retain only bounded diagnostics with a named owner and current + backend semantics. Remove deleted-GL-only interfaces/tests and rename generic + stages/contracts around current RHI concepts. Move investigation history to + research records rather than preserving it in executable branches. + +## F-017 — Headless plugin callback failures are silently discarded + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** plugin reliability / observability +- **Evidence:** `HeadlessPluginHost.cs:231-237` wraps every + `Action` invocation in `catch { }`. This applies both to + replay-on-subscribe and subsequent live delivery. No failure is sent to + `HeadlessPluginLogger`, the session diagnostics stream, or plugin state, and + the headless tests do not cover a throwing event handler. By contrast, + `GameRuntimeEventHub` records observer dispatch failures, and the plugin + architecture promises logged failure isolation + (`docs/plans/2026-04-10-plugin-architecture-design.md:72`). +- **Impact:** a plugin can stop receiving or processing world events while the + process, session, and plugin all remain apparently healthy. Operators and + plugin authors get no error or identity, making production diagnosis nearly + impossible and contradicting the stated failure-containment contract. +- **Recommended direction:** isolate each callback but report the exception + with plugin/subscription identity through the existing diagnostics channel; + define and test whether repeated handler failures fault/unsubscribe the + plugin or remain rate-limited warnings. + +## F-018 — Diagnostic configuration bypasses the runtime's typed ownership model + +- **Severity:** P2 medium +- **Confidence:** high +- **Category:** configuration / hot-path maintainability +- **Evidence:** owned source/tools contain 129 direct + `Environment.GetEnvironmentVariable` calls. Central diagnostic holders + account for 71, but production parsing, physics, and presentation paths read + environment variables directly while processing work: + `LiveEntityNetworkUpdateController.cs:436-437,503,521,607,907,1855,2563,2914`, + `RuntimeRemotePhysicsUpdater.cs:630`, `UpdateMotion.cs:163`, + `PlayerDescriptionParser.cs:458,473`, `PhysicsEngine.cs:2300`, and + `TransitionTypes.cs:1374,3905,5991`. Several are evaluated for every relevant + packet, entity update, or collision transition rather than captured once. + `RenderingDiagnostics` and `PhysicsDiagnostics` also expose large mutable + process-static state even though Headless explicitly supports multiple + isolated sessions. +- **Impact:** configuration cannot be discovered from one schema, captured in + a session snapshot, or controlled consistently in tests. Mid-process + environment mutation produces inconsistent behavior, direct reads add noise + to sensitive paths, and process-global mutable probes are unsafe to attribute + in multi-session diagnostics. +- **Recommended direction:** parse diagnostic configuration once into a typed, + documented snapshot, pass the minimal immutable flags/sinks to each owner, + and make any runtime toggles explicitly session-scoped. Eliminate direct + environment reads below composition roots. + +## F-019 — The build toolchain and dependency graph are not reproducibly pinned + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** build reproducibility / contributor onboarding +- **Evidence:** the repository has 44 project files but no `global.json`, + `Directory.Build.props`, `Directory.Packages.props`, `NuGet.config`, or + `packages.lock.json`. Twenty-eight projects request `LangVersion=latest`, + only 23 declare `TreatWarningsAsErrors`, and common nullable/language/test + package settings are copied independently. CI installs floating SDK + `10.0.x` in all four jobs (`headless-portability.yml:82-84,163-165,235-237, + 328-330`), while this baseline was evaluated with SDK 10.0.300. There are 84 + direct PackageReference declarations distributed across the projects. +- **Impact:** a patch can compile or analyze differently as the .NET 10 SDK, + compiler, workload, or transitive dependency resolution advances. Project + templates already diverge in warning policy, so “warnings as errors” is not + a repository-wide contract. Humans cannot reproduce a released build from + the commit alone. +- **Recommended direction:** pin the accepted SDK feature band with + `global.json`, centralize common compiler/analyzer settings and package + versions, enable locked restore for release/CI, and document the one command + that proves a clean checkout. Keep tool-only exceptions explicit rather than + allowing per-project drift. + +## F-020 — The issue ledger is an unbounded mixed tracker, research log, and archive + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** issue management / documentation architecture +- **Evidence:** `docs/ISSUES.md` is 19,073 lines (about 1.23 MB) with 380 issue + headings but only 378 unique IDs; #32 and #234 each have duplicate headings. + A block scan finds 207 bodies with closed-like status, another 50 whose title + says closed/done without a nearby status, 114 open-like bodies, five + nonstandard states, and four headings with no status signal. Thus roughly + two-thirds of the tactical file is closed history. Issue #419 alone begins + with about 100 lines of failed-attempt narrative, byte-decoded research, and + mandatory next-session protocol. The file contains 29 broken relative links, + including missing `references/holtburger` targets, moved source/tests, and + repo-root paths incorrectly resolved from `docs/`. Its own convention asks + every session to scan open issues while also warning against mechanically + separating the closed blocks (`:13-23`). +- **Impact:** humans cannot reliably answer “what is open?”, follow evidence, + or know whether a correction supersedes nearby text without parsing a large + chronological narrative. Duplicate IDs and link rot weaken commit/issue + traceability, while maintaining research inside the tactical queue guarantees + continued growth and merge contention. +- **Recommended direction:** define a small machine-checkable issue index with + one record per ID and normalized state/owner/severity/link fields. Move closed + bodies and long investigations to immutable archive/research documents while + retaining backlinks. Add link/status/duplicate-ID validation to CI and make + the active view generated or trivially filterable. + +## F-021 — Output-only diagnostics are counted as release regression tests + +- **Severity:** P1 high +- **Confidence:** high (method-body and same-file helper scan, followed by + manual inspection of the contract-like cases) +- **Category:** test suite signal / naming +- **Evidence:** 51 attributed methods across 27 App/Core test files contain no + assertion, expected exception, or other failure signal; their only result is + diagnostic output. The exact catalog is T-011 in `test-quality-audit.md`. + Many are openly named probes, dumps, inspections, or characterizations, but + others claim behavioral contracts such as + `ScenarioB_StairDescent_RampCellRetention`, + `Descent_RealCameraSweep_StairCellRetention`, + `SoundTables_WithAmbientSlots_ExistForWireBinding`, and + `ReplicateProductionEmission_OnPortalFills`. Their bodies calculate and + print observations without comparing them to an oracle. Most DAT-backed + cases also silently return as passes when the local asset prerequisite is + absent (F-015). +- **Impact:** investigation utilities inflate normal pass totals and names + imply coverage that does not exist. A production regression can alter every + printed value while CI remains green, and maintainers cannot tell which + tests are automatic gates versus manual forensic tools. +- **Recommended direction:** move output-only programs behind a separately + invoked diagnostic command/trait that publishes artifacts but is excluded + from release pass totals. Retain a method as a regression test only after it + has a stable oracle and a failure assertion; make names explicitly diagnostic + until then. + +## F-022 — Campaign source-shape tests became a brittle parallel architecture + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** tests / architecture governance +- **Evidence:** 30 App test files read production source and contain 560 + literal text assertions plus 86 calls to multi-fragment ordering helpers. + `GameWindowSlice8BoundaryTests` explicitly describes itself as a “Temporary + source-shape freeze” to be replaced by functional owner tests, but it remains + with nine facts, 73 literal assertions, and 31 ordering calls after the + documented Runtime ownership campaigns. Similar tests pin private field, + constructor, local-variable, and call spellings across composition, frame, + streaming, and Runtime-ownership files. See T-012 for scope and nuance. +- **Impact:** maintainers must satisfy literal implementation snapshots in + addition to the architecture document and behavioral contracts. Equivalent + refactors fail noisily, while semantics can regress without changing the + pinned text; this discourages cleanup of the exact giant files the tests + freeze. +- **Recommended direction:** retain true dependency and ownership invariants, + but encode them through project references, type/reflection checks, or + syntax/semantic architecture analysis. Give every campaign freeze a removal + condition and delete it when functional coverage lands; do not preserve + private method bodies as a permanent specification. + +## F-023 — Release/App publish output always embeds the smoke plugin + +- **Severity:** P2 medium +- **Confidence:** high +- **Category:** packaging / plugin defaults +- **Evidence:** `AcDream.App.csproj:85-129` unconditionally builds + `AcDream.Plugins.Smoke` and copies its DLL plus manifest to both ordinary + build and publish output. `GraphicalPluginSession.Create` always scans the + output `plugins` directory, and `PluginSession.Start` documents and + implements a null allow-list as “load every discovered id.” Launcher-created + normal/probe sessions protect themselves with an explicit empty list, but + omitted plugin configuration is intentionally the developer/direct-launch + flow. The embedded plugin subscribes to every entity-spawn event and writes + smoke diagnostics; it is not product functionality. +- **Impact:** a release artifact contains a test plugin and direct/config-based + launches can enable it merely by omitting a field. This makes “no configured + plugins” ambiguous, adds observable logging/event work, and gives human + maintainers two different defaults depending on launch path. +- **Recommended direction:** keep the example plugin buildable/testable but + exclude it from release publish output by default. Package samples + separately, make plugin enablement explicit in all production launch paths, + and test the published manifest contents. + +## F-024 — Plugin registration cleanup failures are swallowed and can leak a plugin lifetime + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** plugin lifecycle / failure containment +- **Evidence:** `ScopedPluginHost.cs:53-205,260-269` catches and discards + failures while rolling back selection subscriptions, entity-event + subscriptions, and UI registrations. `PluginSession.Dispose` otherwise logs + failures from plugin `Disable` and ALC unload, but these inner cleanup errors + never reach it. Repository plugin tests cover ordinary unsubscribe and + collectible-context release, but none supplies a host event remove accessor + or UI registration whose cleanup throws. +- **Impact:** shutdown can report a plugin as released while a host registration + still references plugin-defined code or data, preventing collectible ALC + unload and allowing callbacks after disable. The failure is invisible to the + operator and cannot be distinguished from a CLR unload delay. +- **Recommended direction:** continue best-effort cleanup across all + registrations, but collect and report every failure with plugin and + registration identity. Define whether disposal faults the plugin status or + emits a bounded warning, and add adversarial removal/disposal tests. + +## F-025 — The advertised plugin compatibility contract is not enforced or packaged + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** public API / documentation / compatibility +- **Evidence:** `PluginManifest.Parse` accepts every positive `apiVersion`, but + `PluginLoader` and `PluginSession` never compare it with a host-supported + version. The manifest's `dependencies` list is parsed and tested only for an + empty default; no production code consumes it. The approved plugin design + promises a locally published `AcDream.Plugin.Abstractions` NuGet package, + dependency handling, callback faulting/unhooking, hot reload, and a much + broader surface, while the project has no package metadata or pack target and + currently exposes eight small source files. No compatibility matrix or API + evolution policy exists. +- **Impact:** an incompatible plugin can pass discovery and fail later through + type loading or missing behavior, while a declared dependency has no effect. + Plugin authors cannot consume a versioned artifact from the release or know + which parts of the “design locked” document actually exist. +- **Recommended direction:** define one supported manifest/API version, + reject unsupported versions before loading code, either implement dependency + resolution or remove the field, and publish/version the abstractions package + as part of the release. Replace the aspirational design's status with an + explicit implemented-versus-planned matrix. + +## F-026 — There is no owned release process for the updater-facing artifacts + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** release engineering / project governance +- **Evidence:** the repository has no tags, centralized product version, release + workflow, changelog, contribution guide, security policy, or CODEOWNERS. The + launcher production path is already hard-coded to fetch + `releases/latest/download/manifest.json` and per-RID hashed archives, but no + tracked workflow or production tool builds those archives and manifest; + `new-campaign-la-update-fixture.ps1` only creates a loopback test fixture. + `headless-portability.yml` publishes temporary CI outputs but does not create + a release, provenance/attestation, checksum manifest, or GitHub assets. +- **Impact:** the client/updater implementation cannot be reproduced as an + end-to-end release operation from repository instructions. Version choice, + artifact contents, signing/provenance, rollback, announcement, and security + response depend on undocumented maintainer knowledge. +- **Recommended direction:** define the version and release checklist in a + short maintained document; automate clean pinned builds, full bounded tests, + per-RID packages, SBOM/provenance/checksums, manifest generation and + validation, and release publication. Add contributor/security/changelog + ownership before calling the repository release-ready. + +## F-027 — The retail-divergence authority is structurally unauditable + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** architecture documentation / technical-debt governance +- **Evidence:** `retail-divergence-register.md` is 809,163 bytes but only 525 + physical lines because 243 lines exceed 1,000 characters and seven exceed + 10,000; its longest section heading is 45,240 characters. It contains 320 + active table rows (20 IA, 85 AD, 161 AP, 50 TS, four UN), with change history + repeatedly prepended to headings and individual cells. The title says + “current through 2026-07-31” while the same document records changes through + 2026-08-17. Of 843 backtick path references, 18 occurrences across 11 unique + paths no longer resolve, including deleted App physics/UI classes and moved + Core.Net/UI command files. +- **Impact:** the file designated as the first diagnostic stop is hard to diff, + review, merge, filter, or validate mechanically. Current rationale is buried + inside corrections to prior rationale, and stale locations make it unsafe as + a source-navigation index. +- **Recommended direction:** store one normalized record per divergence (or one + small table per subsystem) with stable ID, kind, owner, current rationale, + oracle, live code symbols, and review date. Move amendment narratives to + linked decision history, generate a compact active index, and validate counts + and paths in CI. + +## F-028 — Production comments depend heavily on campaign history and unavailable references + +- **Severity:** P1 high +- **Confidence:** high (Roslyn comment-trivia inventory plus manual review) +- **Category:** comments / provenance / human maintainability +- **Evidence:** 37,734 owned source comment trivia nodes include 2,172 with a + campaign/slice/phase/checkpoint or commit reference, 1,016 with issue-number + references, and 617 with probe/temporary/workaround language. “holtburger” + appears 163 times in 52 source files and 50 times in 23 test files; 23 files + cite a literal `references/holtburger` path that is absent from the tracked + repository. Confirmed stale comments still claim GL-thread, GL-repeat, + OpenGL-framebuffer, and ImGui behavior after the Vulkan-only cutover; examples + include `Content/ObjectMeshData.cs`, `Content/MeshExtractor.cs`, + `UiDatElement.cs`, `FrameScreenshotController.cs`, and `FrameProfiler.cs`. +- **Impact:** the local reason for code is often inseparable from a private + worktree, a huge issue block, or campaign chronology. Humans cannot verify + provenance, and stale backend terminology actively misstates thread and + ownership contracts. +- **Recommended direction:** lead every maintained comment with the current + invariant and why it exists; keep retail symbol/address or stable public + revision as secondary provenance. Move attempt history, dates, campaign + labels, and closed probes into research/decision records. Add a comment-link + and forbidden-stale-backend vocabulary check for shipping code. + +## F-029 — The two mandatory agent instruction files are materially out of sync + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** AI/human workflow authority / duplicated documentation +- **Evidence:** `AGENTS.md` opens by calling itself a synchronized agent-facing + port of `CLAUDE.md` and orders shared instructions to remain in both files. + A baseline diff reports 255 insertions and 27 deletions between them. + `AGENTS.md` is about 91,000 characters/1,435 physical lines; `CLAUDE.md` is + about 107,000/1,658. The agent copy still says to resume world-interaction + Slice 4, while the Claude copy records that program and several later + campaigns complete. They disagree on whether the required reference set has + six or five repositories, on AC2D's status, and on the semantics/default of + `ACDREAM_STREAM_RADIUS`; the newer copy also adds many private-memory entry + points absent from the agent copy. +- **Impact:** tool choice determines the project's “current truth.” An agent + following the repository's own instructions can restart completed work, + search for a retired reference, or run a gate with obsolete configuration. + Humans cannot safely review a 200-KB duplicated operational manual for + semantic parity on every change. +- **Recommended direction:** maintain one tool-neutral project instruction + authority and generate thin tool-specific wrappers. If two tracked copies + must remain, generate one from the other and fail CI on drift. Keep current + campaign state out of both; link one generated status page instead. + +## F-030 — The repository already records seven unresolved load-sensitive tests + +- **Severity:** P1 high +- **Confidence:** high for the documented recurrence; not all reproduced in + this single audit run +- **Category:** test determinism / release signal +- **Evidence:** open issues #302, #308, #321, #336, #340, #346, and #402 name + seven separate intermittent tests across App, Core, Core.Net, and Runtime. + The mechanisms include exact `GC.GetAllocatedBytesForCurrentThread` equality, + real-time deadline/sleep loops inside a virtual-clock transport soak, an + unresolved concurrent audio-cache dedup race, and shared reader/budget tests + that fail only under suite load. The exact catalog is T-014. All passed in + the one per-project audit baseline, which is consistent with the recorded + intermittent profiles rather than evidence of resolution. +- **Impact:** repeated release gates can alternate red/green without a source + change, encouraging maintainers to normalize failures as noise. Two cases may + represent production races rather than merely fragile assertions, so blanket + retries would hide defects. +- **Recommended direction:** assign an owner and deterministic reproduction to + each row; replace wall time and exact un-warmed allocation equality with + controlled mechanisms, and capture concurrency schedules/state. Quarantine + may separate reporting temporarily, but must not turn the cases into silent + passes or an approved generic retry list. + +## F-031 — The launcher persists account passwords in plaintext without public disclosure + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** credential handling / release documentation +- **Evidence:** `AccountProfile.Password` is serialized into + `launcher-profiles.json`; `LauncherProfileStore.cs:119-181` explicitly calls + this the accepted plaintext-credential design. Linux creation is carefully + constrained to mode 0600, while Windows relies on the normal user-profile + ACL. The password is commendably kept out of child arguments/config and fed + over standard input, with redaction tests around status/crash output. The + public `README.md`, however, never tells launcher users that their password is + stored reversibly on disk, and there is no SECURITY document describing the + threat model or migration/erasure behavior. +- **Impact:** users may reasonably assume a launcher uses an OS credential + vault. Profile backups, sync tools, support bundles, or same-user processes + can expose the credential, and users cannot make an informed choice from the + release documentation. +- **Recommended direction:** prefer Windows Credential Manager and a Linux + Secret Service/keyring with an explicit fallback. If plaintext remains an + intentional alpha limitation, disclose it prominently before password entry + and in release docs, document exact file locations/permissions/deletion, and + keep the existing no-logs/no-arguments tests as hard gates. + +## F-032 — Tracked “current truth” memory documents preserve superseded architectures + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** persistent project memory / documentation authority +- **Evidence:** project instructions describe `memory/` as persistent project + knowledge and route maintainers into it. Yet + `memory/project_gamewindow_decomposition.md:1,138-147` labels itself “current + truth” while prescribing an ImGui frame/submission path; + `memory/project_ui_architecture.md:18-43` says ImGui/OpenGL is the current + backend and “stays forever,” later partially supersedes itself, and ends by + saying to resume interaction Slice 4 (`:269`). + `memory/project_linux_graphical.md:39,75-77` still describes the packaged + cimgui/OpenGL/bindless-texture gate. Those projects/backends were deleted and + the six-slice interaction program is complete. +- **Impact:** the repository presents dated handoff notes as reusable current + authority. A human or agent following the prescribed “start at memory” path + can reintroduce deleted stacks or apply obsolete gate requirements even when + the architecture document is correct. +- **Recommended direction:** classify every memory file as current, superseded, + or historical with an owner/review date. Remove “current truth” from dated + handoffs, add a generated stale-link/forbidden-backend check, and route active + entry points only to maintained subsystem documents. + +## F-033 — The prepared-content boundary still exposes vendor and deleted-backend vocabulary + +- **Severity:** P2 medium +- **Confidence:** high +- **Category:** architecture boundary / dependency ownership +- **Evidence:** public `AcDream.Content.ObjectMeshData`, `MeshBatchData`, and + related package serializers expose `Chorizite.Core.Lib.BoundingBox`, + `Chorizite.Core.Render.Enums.TextureFormat`, and other DatReaderWriter/ + Chorizite value types. `AcDream.Core.Rendering.Wb.TextureHelpers` also + consumes the vendor texture enum, forcing `Chorizite.Core` into Core and App. + `UploadFormats.cs` deliberately encodes OpenGL `GL_*` numeric constants and + instructs future maintainers to add new members using GL values, even though + the only production graphics backend is Vulkan. `AcDream.App.csproj` itself + records that the package audit remains unclean after deletion of the raw-GL + backend. The mandatory WorldBuilder inventory describes this as the intended + MP1 boundary rather than a remaining migration. +- **Impact:** a CPU/prepared-data contract that should describe acdream-owned + semantics is coupled to a pre-1.0 vendor package and an absent backend's ABI. + Package/API churn reaches Core, Content, App, package serialization, and bake + compatibility together. The names also mislead human maintainers about which + graphics API actually owns upload behavior. +- **Recommended direction:** introduce small acdream-owned bounding-box and + source-texture-format records at the extraction boundary, convert vendor + objects once, and version the serialized representation explicitly. Name + upload hints by channel layout/component type rather than GL constants; map + those semantics to Vulkan at the RHI boundary. Treat the current vendor types + as an acknowledged migration seam until removed. + +## F-034 — Generated reverse-engineering state and capture logs dominate the repository + +- **Severity:** P1 high +- **Confidence:** high +- **Category:** repository hygiene / onboarding / artifact provenance +- **Evidence:** the baseline's tracked worktree content is approximately + 937 MiB. `tools/ghidra_project` contributes 575 MiB in 34 files, including + five opaque Ghidra database blobs around 90 MiB each; `docs/research` + contributes another 299 MiB. Thirty-seven tracked `.log` files total about + 151 MiB, including a 29 MiB acdream capture. Thirteen individual tracked + files exceed 10 MiB. There are no Git LFS rules or documented artifact- + retention/support policy; `.gitattributes` only has a workflow merge rule. + The shared object store used by this worktree contains 2.65 GiB of pack data, + illustrating the history cost after generated artifacts change. +- **Impact:** every contributor pays a large clone/storage/indexing cost for + opaque tool databases and raw historical output that ordinary build, test, + and review work does not use. Binary state cannot be meaningfully diffed or + code-reviewed, logs can accidentally retain sensitive machine/session data, + and provenance/licensing review is mixed with the source tree rather than an + explicit artifact inventory. +- **Recommended direction:** keep reproducible extraction scripts, compact + curated fixtures, checksums, tool versions, and evidence summaries in Git. + Move reconstructable Ghidra projects and raw capture bundles to a versioned + artifact store or LFS only if their distribution is legally approved; publish + a bootstrap manifest and retention/redaction policy. Purging existing Git + history is a separate coordinated repository migration, not an ordinary + cleanup commit. diff --git a/docs/reviews/test-quality-audit.md b/docs/reviews/test-quality-audit.md new file mode 100644 index 00000000..c51bd179 --- /dev/null +++ b/docs/reviews/test-quality-audit.md @@ -0,0 +1,403 @@ +# Test-quality audit + +Status: **complete at the recorded baseline**. + +## Baseline + +- 1,260 tracked C# files under `tests/`, totaling 402,131 lines. +- 12 primary test projects, plus fixture/helper projects. +- The reproducible per-project Release total is currently 14,747 passes and 77 + reported skips. One additional duplicate-ID theory row is silently dropped + by xUnit discovery. The official all-solution process does not currently + complete reliably because of the launcher deadlock described below. +- The Release build emits 26 warnings, all in test projects. These include + nullable-flow problems, dead fixture fields, xUnit assertion-style findings, + and a duplicate `InlineData` warning. + +A clean `Rebuild` classifies those 26 as: seven CS8602; three each CS0649, +CS8600, CS8604, and CS8767; two each xUnit2013 and xUnit2017; and one each +CS8625, xUnit1025, and xUnit2000. An incremental build can misleadingly print +zero because no test compiler/analyzer target reruns; the warning baseline must +therefore come from a clean/rebuild gate. + +## Executable baseline by test assembly + +| Assembly | Passed | Skipped | Notes | +|---|---:|---:|---| +| App | 5,510 | 76 | installed-DAT/GPU/manual/regression groups collapse into dynamic skips; many other gated facts silently no-op | +| Bake | 21 | 0 | completed | +| CLI | 4 | 0 | completed | +| Content | 154 | 0 | completed | +| Core.Net | 1,004 | 0 | three disabled live-network facts are reported as passed | +| Core | 4,797 | 1 | plus one duplicate-ID theory row dropped during discovery; many absent-DAT facts silently no-op | +| Headless | 166 | 0 | completed | +| Launcher.Core | 338 | 0 | passes alone in 48 s; deadlocks in full run | +| Launcher | 67 | 0 | completed | +| Platform | 4 | 0 | completed | +| Runtime | 1,756 | 0 | completed | +| UI.Abstractions | 926 | 0 | completed | + +## Review criteria + +Every test source file and parameter/data source will be checked for: + +- name/behavior/assertion agreement; +- a meaningful regression signal rather than successful execution only; +- independence from reimplemented production logic; +- redundancy with stronger tests; +- stable public behavior versus implementation detail; +- deterministic time, ordering, filesystem, culture, and environment handling; +- skip/manual/live-DAT intent and discoverability; +- correct layer/project placement; +- fixtures that can actually fail for the behavior named; +- data rows that exercise distinct boundaries rather than inflating counts. + +The all-file pass combines syntax-tree method inventory with a per-file signal +matrix (attributed methods, assertions/exception oracles, early returns, +environment/asset gates, sleeps/delays, source-text reads, diagnostic naming, +and helper-local assertions). It is followed by manual body/helper review for +every exception class identified by that matrix. The project totals below make +the breadth visible; counts are lexical triage signals, not quality scores: + +| Test area | Files | Attributed methods | `Assert.*` calls | empty-return sites | environment reads | sleep/delay sites | file-reading heuristic | +|---|---:|---:|---:|---:|---:|---:|---:| +| App | 515 | 4,812 | 20,997 | 121 | 73 | 10 | 43 | +| Bake | 5 | 19 | 91 | 1 | 1 | 0 | 0 | +| CLI | 1 | 4 | 19 | 0 | 0 | 0 | 1 | +| Content | 29 | 134 | 716 | 25 | 6 | 1 | 0 | +| Core.Net | 115 | 867 | 3,467 | 30 | 15 | 21 | 0 | +| Core | 398 | 3,398 | 8,576 | 70 | 1 | 9 | 6 | +| Headless | 18 | 140 | 806 | 5 | 0 | 5 | 4 | +| Launcher.Core | 27 | 232 | 989 | 19 | 0 | 13 | 10 | +| Launcher | 7 | 38 | 261 | 0 | 0 | 4 | 1 | +| Platform | 2 | 4 | 17 | 0 | 0 | 0 | 0 | +| Runtime | 91 | 1,344 | 8,601 | 64 | 0 | 1 | 3 | +| UI.Abstractions | 46 | 462 | 1,228 | 1 | 0 | 5 | 2 | + +Fixture/helper projects were reviewed with their consumers and contain no +attributed tests. An assertion count includes helper assertions and repeated +parameterized-oracle calls; it is not an executable-case count. The final +column is deliberately broad (it also sees fixtures and result files); T-012's +manual follow-up is the exact 30-file production-source-reading subset. + +## Skip and conditional-execution inventory + +| Source | Declared reason | Initial concern | +|---|---|---| +| `AcDream.App.Tests/UI/Layout/CharacterManagementLiveDatTests.cs:373-383` | installed-DAT/probe gate | dynamic skip behavior and release discoverability | +| `AcDream.App.Tests/UI/Layout/ChatLayoutFixtureGenerator.cs:29` | manual fixture generator | generator represented as a skipped test | +| `AcDream.App.Tests/UI/Layout/RadarLayoutFixtureGenerator.cs:14` | manual fixture generator | generator represented as a skipped test | +| `AcDream.App.Tests/Rendering/TowerAscentReplayTests.cs:206` | unresolved issue #119 residual | permanent red test hidden as skip | +| `AcDream.Core.Tests/Conformance/PvsConformanceTests.cs:22` | “P0 scaffold” for future capture | incomplete scaffold counted in the suite | + +The custom `InstalledDatFactAttribute` is used by 73 methods. It reports a real +xUnit skip unless `ACDREAM_PROBE_LIVE_MOUNT=1`, but its single LA8-specific +reason is inherited by unrelated character creation, map, tooltip, and other +installed-DAT suites. In contrast, 294 attributed methods across 107 files +contain method-level empty `return` statements. The syntax inventory divides +the 314 return sites as follows (a method can contain more than one category): + +| Gate type | Return sites | Methods | Files | Result when gate is unavailable | +|---|---:|---:|---:|---| +| DAT/package/fixture | 251 | 248 | 85 | **pass**, without exercising the named behavior | +| opt-in environment variable | 23 | 23 | 9 | **pass**, without exercising the named behavior | +| operating system | 21 | 19 | 11 | pass on the unsupported OS | +| other condition | 19 | 18 | 12 | varies; several suppress missing expected data | + +The first two groups are not included in xUnit's 77 reported skips. Therefore +the executable totals above describe discovered and completed cases, not the +number of named contracts that were actually exercised. + +## Confirmed test-quality findings + +### T-001 — Full-suite launcher test exposes a production lock inversion + +`LauncherProcessSupervisorTests.ANullStderrLogPathBehavesExactlyAsBeforeForBothChildProcessKinds` +is not itself low-value, but its unbounded `using`-scope disposal exposed a +real production deadlock. The managed stack proves a supervisor-lock / Process- +lock inversion between `LauncherProcessSupervisor.Dispose` and the process exit +callback. The test needs a deterministic race harness after the production +fix; the current timing-dependent version alternates between passing in 48 s +and hanging indefinitely. + +**Post-baseline resolution checkpoint (2026-08-18, uncommitted):** +`DisposeAllowsAnAlreadyCapturedExitCallbackToComplete` is now the deterministic +race harness. Explicit barriers make the fake child capture the exit delegate, +release it from inside disposal, and wait for the callback to return. The old +lock shape fails boundedly after five seconds; the fixed shape passed 25/25 +fresh-process repetitions, all 22 supervisor tests, Launcher.Core 339/339, and +two serialized complete-solution runs of 14,748 passes / 77 skips. The original +real-process null-stderr test remains as end-to-end coverage rather than the +only accidental race trigger. + +### T-002 — One physics assertion is a literal tautology + +`Issue265SteepSlopeCaptureBisectTests.cs:920` asserts +`Assert.Equal(newModelVelocityBeforeToggle.Z > 0.01f, +newModelVelocityBeforeToggle.Z > 0.01f)`. It cannot fail and therefore proves +nothing about the comparison described by the surrounding comment. This is a +genuinely useless assertion inside an otherwise evidence-oriented test. It is +already recorded as open issue #342. + +### T-003 — Duplicate theory data is silently discarded + +`MotionInterpreterTests.cs:491-497` supplies both +`MotionCommand.Crouch` and literal `0x41000012u`; those values are identical. +xUnit emits duplicate case ID `6a4edd...` and skips the second row during +discovery. The test name is accurate, but the duplicate row is redundant and +the reported suite total does not reveal it. The build's xUnit1025 warning and +issue #228 already acknowledge this. + +### T-004 — Test warnings include dead fixture state and contract mismatches + +The current build reports 26 warnings. Examples requiring source-level review +include never-assigned `BeginTurnBlocked`/`BeginTurnUnblocked` fields in +`RemoteChaseEndToEndHarnessTests.cs:130-131`, nullable dereferences in installed- +DAT inspection tests, and nullability-mismatched fake implementations in App +composition tests. These are not all equivalent, but a release gate that +normalizes them as an old fixed warning count makes new warning regressions +invisible. + +### T-005 — At least 52 tests exercise an unreachable UI presentation stack + +The `IPanelRenderer` widget/menu tests and the ChatPanel layout/input/focus +tests are internally meaningful, but no production `IPanelRenderer` or +`IPanelHost` exists and no shipping code constructs `ChatPanel`, `DebugPanel`, +or `VitalsPanel`. The former ImGui implementation was deleted. These tests are +therefore obsolete release tests rather than tautologies: they can detect +changes in compiled dead code, but cannot protect user-visible retained UI. +They should be removed with that abandoned stack or moved behind a clearly +owned compatibility contract if the panel API is intentionally supported. + +### T-006 — The installed-DAT attribute gives 73 unrelated tests one misleading skip identity + +`InstalledDatFactAttribute` is declared inside +`CharacterManagementLiveDatTests.cs:367-383` and always says “installed-DAT +LA8 gate.” Repository-wide attribute inspection finds 73 methods using it, +including character creation and other UI/DAT areas unrelated to LA8. The +attribute does correctly produce a reported xUnit skip, but the reason does not +identify the prerequisite or contract of the test that was skipped. Split the +fixture capability from test-specific skip messages, and publish installed-DAT +gate results by suite rather than as one campaign-era bucket. + +### T-007 — 271 gated facts report success without exercising their contract + +A syntax-tree scan of every attributed method found 248 methods across 85 +files that empty-return when DATs, a prepared package, or a replay fixture are +absent, plus 23 methods across nine files that empty-return unless an opt-in +environment variable is set. Examples include the three `LiveHandshakeTests` +(`:38`, `:115`, `:265`), whose own comment calls the return a skip although +xUnit reports all three as passed; 17 UI live-mount/powerbar probes; and broad +App/Core rendering and physics suites guarded by `datDir is null`. The pattern +also appears in all three Bake determinism facts. These names look like normal +regression contracts and their passes are included in headline totals. Use a +real skip/fail mechanism, group them under explicit installed-DAT/package/live +traits, and make release reporting state exactly which group ran. + +### T-008 — Several diagnostic “tests” are deliberately incapable of failing + +The whole `SmokeTest.TestProject_IsWired` body is `Assert.True(true)`. +`CellarLipWedgeTests.cs:99,305,331`, +`CellarUpTrajectoryReplayTests.cs:315,372`, and +`DoorCollisionApparatusTests.cs:238` end diagnostic output with an always-true +assertion; comments explicitly say some always pass. `GpuContractTests.cs:257` +compares the same enum constant to itself inside an otherwise useful test. +These are tools disguised as release tests. Move output-only apparatus to an +explicit diagnostic command/trait, delete the wiring smoke test, and replace +the useful diagnostics with assertions over the observed invariant. + +### T-009 — Double-click tests use wall-clock sleeps instead of a controllable clock + +`InputDispatcherDoubleClickTests.cs:53,73,89,106,108` sleeps for 10 or 600 ms +to cross real timing boundaries. This adds at least 640 ms to a small unit-test +class and leaves its outcome dependent on scheduler timing because production +`InputDispatcher` owns the clock. Inject `TimeProvider` (or a narrow monotonic +clock contract) and advance fake time deterministically. + +### T-010 — Two entire tests have no runtime contract to protect + +`SmokeTest.TestProject_IsWired` is the literal tautology described in T-008. +`ChaseCameraTests.ImplementsICamera` merely assigns a `ChaseCamera` to an +`ICamera` variable and calls `ToString`; compilation already proves the stated +interface relationship, and the call has no assertion or behavioral signal. +Both tests are useless as maintained release cases. The compile-only interface +check should be deleted or replaced by a named interface behavior test. + +### T-011 — 51 release-suite methods are output-only diagnostic apparatus + +A method-body and same-file helper scan found 51 attributed methods across 27 +files that have no assertion, exception expectation, or other failure signal; +their observable contract is limited to diagnostic output. This count excludes +legitimate explicit no-throw cases such as no-op disposal, empty lifecycle +hooks, and secure/loopback transport construction. It also excludes tests whose +assertions live in a helper that the scanner can resolve. + +Many names honestly say `Probe`, `Dump`, `Diagnostic`, `Inspection`, or +`Characterize`; these are useful investigation programs in the wrong execution +surface, not regression tests. The more serious naming problem is the subset +that reads like a behavioral contract—especially the facility-hub scenarios, +stair/camera retention sweeps, ambient-slot existence tests, production- +emission replication, and drawn-polygon comparisons—while only printing the +calculated value. Most DAT-backed examples also combine with T-007: they pass +without output when their local fixture is absent. + +Exact inventory (method names in one cell belong to the named file): + +| Test file | Output-only attributed methods | +|---|---| +| `App.Tests/Rendering/CornerFloodReplayTests.cs` | `Scratch_ReciprocalPrimitive_SyntheticPair` | +| `App.Tests/Rendering/Issue131SetupProbeTests.cs` | `Diagnostic_LookInFlood_AdmitsHallPorchFromCottage`; `Diagnostic_DumpOutstageCandidateSetups` | +| `App.Tests/Rendering/Issue176177FacilityHubFloodReplayTests.cs` | `ScenarioB_StairDescent_RampCellRetention`; `ScenarioC_CorridorSeamGazeSweep_Bistability`; `ScenarioE_RootLagWindow_ForwardChainRetention`; `ScenarioD_CorridorWalk_PerStepChurn` | +| `App.Tests/Rendering/Issue177StairDescentCameraFloodTests.cs` | `StairCellComposition_ShellVsStatics`; `RealStaircase_FineYawZoomSweep_FindKnifeEdge`; `FloodDepthFrom015E_VsRetail26`; `StaircaseSweep_EyeClearanceFromCeilingPortal`; `ParkedYawZoomSweep_StairAdmission`; `Descent_RealCameraSweep_StairCellRetention` | +| `App.Tests/Rendering/Issue181VisFlapReplayTests.cs` | `Diagnostic_FlappingCellViewRegion_SliverOrLarge` | +| `App.Tests/Rendering/Issue181WallPressEquilibriumTests.cs` | `Diagnostic_WallPressedCamera_EyeWanderAndViewerCellStability` | +| `App.Tests/Rendering/TowerAscentReplayTests.cs` | `Diagnostic_TopOfStairs_GateByGate` | +| `App.Tests/UI/Layout/FaPanelSlotProbeTests.cs` | `ProbePanelSlotTable` | +| `App.Tests/UI/Layout/MapHousePanelSlotProbeTests.cs` | `ProbeMapHousePanelSlot` | +| `App.Tests/UI/Layout/OptionsPanelLiveMountProbeTests.cs` | `ProbeLiveMountShapes`; `ProbeMenuPopupSizingAndTextStyle`; `ProbeFilterLabelHome` | +| `App.Tests/UI/Layout/PowerbarLayoutProbeTests.cs` | `ProbePowerbarAuthoredStrings`; `ProbeSecureTradeLayout`; `ProbeTotalItemsTemplate` | +| `App.Tests/UI/Layout/SpewBoxLayoutDumpDiagnosticTests.cs` | `SweepInstalledLayoutDescs_ForSpewBoxElementClass` | +| `Core.Tests/Audio/EnvCellSoundEmitterInventoryTests.cs` | `PortalDat_SetupsWithAmbientSlotSoundTables`; `SoundTables_WithAmbientSlots_ExistForWireBinding` | +| `Core.Tests/Conformance/CottageDoorwayCharacterizationTests.cs` | `Characterize_CottageNeighborhood_PrintStructure`; `Characterize_Doorway_FindInteriorPoints` | +| `Core.Tests/Conformance/DungeonLandblockDatProbeTests.cs` | `Probe_Dungeon0125_vs_Holtburg_A9B4` | +| `Core.Tests/Conformance/HoltburgTorchFalloffProbeTests.cs` | `Dump_Holtburg_StaticLight_Falloffs` | +| `Core.Tests/Conformance/Issue113DoorVanishDiagnosticTests.cs` | `DumpHoltburgBuildings_OrphanGeometry`; `ReplicateProductionEmission_OnPortalFills`; `DumpPortalFillSurfaceTypes`; `DumpControls_HallAndCottage` | +| `Core.Tests/Conformance/Issue113PhantomStairsDumpTests.cs` | `Dump_Cell104_ExteriorPortalPlane_Vs_GapPoint`; `DumpAAB3_Watchtower_TopDownMap`; `DumpHallModel_PolyFlagHistogram` | +| `Core.Tests/Conformance/PvsConformanceTests.cs` | `Pvs_CottageInterior_MatchesRetailCellDrawList` (also explicitly skipped and empty) | +| `Core.Tests/Conformance/ThresholdDivergenceDiagnosticTests.cs` | `Diagnose_ThresholdTransitions` | +| `Core.Tests/Physics/CameraCornerSealReplayTests.cs` | `Diagnostic_DispatchTrace_LeakPath_vs_Controls` | +| `Core.Tests/Physics/DoorSetupGfxObjInspectionTests.cs` | `HoltburgCottage_CellPortals_DatInspection`; `HoltburgLandblockStatics_DatInspection` | +| `Core.Tests/Physics/Issue137CorridorSeamInspectionTests.cs` | `CorridorSeam_FindPolygonMatchingLiveHit` | +| `Core.Tests/Physics/Issue186ConnectorCellGeometryInspectionTests.cs` | `Dump_ConnectorCells_ShellAndCollision` | +| `Core.Tests/Physics/Issue188FadingDoorMotionTableInspectionTests.cs` | `Reflect_DatReaderWriter_HookTypes`; `Dump_PedestalWeakSpot_MotionTable_HookContents` | +| `Core.Tests/Rendering/Issue176177DungeonSeamInspectionTests.cs` | `CorridorNeighborhood_CoplanarOverlappingDrawnPolyPairs`; `UnderHall_DrawnPolys_SurfaceColors`; `CellVertexNormals_SmoothOrFaceted_Dump` | +| `Core.Tests/Rendering/Issue93TownNetworkFountainRoomLightInspectionTests.cs` | `StaticObjects_SetupLightsCount_Dump` | + +Move these to a separately invoked diagnostic tool/trait with artifact output, +or give each a stable oracle and assertion before retaining it in release test +counts. Contract-like names must not remain green when no comparison occurs. + +### T-012 — Temporary source-text freezes became a second implementation specification + +Thirty App test files locate the repository, read production `.cs` files, and +assert their literal contents or ordering. Across those files there are 560 +`Assert.Contains`/`DoesNotContain`/`Matches` calls and 86 calls to custom +`AssertAppearsInOrder` helpers (each commonly checks many fragments), spread +through files containing 219 facts. Not all 219 facts use source text, and a +few whole-tree negative dependency scans are valuable architecture guards; the +problem is the large exact-fragment subset. + +The clearest example is `GameWindowSlice8BoundaryTests`: its class summary +calls it a “Temporary source-shape freeze” whose assertions will be replaced by +functional tests as owners are extracted. It still contains nine facts, 73 +literal fragment assertions, and 31 ordering-helper calls after the documented +Slice J/K ownership migrations. `UpdateFrameOrchestratorTests` contains another +85 literal fragment assertions and 13 ordering calls. Composition and Runtime- +ownership files repeat exact field names, constructor spellings, local-variable +names, and call text. + +These cases are not useless: they did protect mechanical campaign cutovers. +They are now wrongly maintained as durable behavioral tests. Harmless renames, +formatting, extraction, or equivalent refactoring can break them, while a +semantic error that retains the expected text can pass. Keep structural rules +that encode real layer boundaries, preferably through project references, +reflection, Roslyn syntax/semantic analysis, or architecture-test tooling. +Retire campaign freezes once the named functional contract exists, and avoid +making literal method bodies a parallel architecture source of truth. + +### T-013 — A retry-stability assertion compares the controller only with itself + +`RuntimeLocalPlayerFirstEntryStateTests.RetryAtAwaitingActivationNeverRecommitsPublicationOrDuplicatesTheBody` +correctly captures the first `PhysicsBody` outside its three-iteration retry +loop and compares every later body with it. The adjacent controller assertion +at line 438 instead evaluates +`Assert.Same(fixture.Movement.Controller, fixture.Movement.Controller)` inside +each iteration. It can at most prove that two immediate property reads return +the same reference; it never compares the controller across retries as the +surrounding contract implies. Capture the first controller alongside `body` +and compare subsequent iterations to that reference. + +### T-014 — Seven open load-sensitive tests make a green full run non-repeatable + +The repository's own open issue ledger names seven distinct intermittent +tests; all passed in this audit's single per-project baseline, so none should be +silently ignored as “the known flake”: + +| Issue | Test | Recorded mechanism/state | +|---|---|---| +| #302 | `PortalProjectionTests.ClipToRegion_FrameOwnedStore_ReusesExactResultArray` | exact per-thread allocation count; observed about one failure in six isolated App runs | +| #308 | `NakEmissionTests.LossSoak_TwoPercentBidirectional_ZeroMessageLoss_LedgersConverge` | virtual transport clock but real 60-second `DateTime.UtcNow` deadline and `Thread.Sleep` convergence loop | +| #321 | `DatSoundCacheTests.GetWave_ConcurrentSameId_PublishesOneCanonicalWaveAndDecodesOnce` | unresolved test-harness versus production cache race under load | +| #336 | `RuntimeCollisionReportingStateTests.WarmedSteadyContactRefreshDoesNotAllocate` | exact zero-allocation assertion; measured 2,944 bytes once under full load | +| #340 | `StreamingWorkBudgetTests.DestinationAndEmptyUnloadPriorityNeverBypassPublicationBudget` | load-sensitive, deterministic in isolation; root cause not recorded | +| #346 | `PortalProjectionTests.ProjectToClipLease_ReusesPooledWorkWithoutResultArrays` | second exact-allocation assertion in the same file; repeated full-load failures | +| #402 | `LandblockBuildFactoryTests.Build_UsesTheSuppliedSharedReaderGate` | roughly two failures in five repeated full App runs; shared-state/timing cause unresolved | + +The #308 XML documentation calls the 10,000-message soak a virtual-clock test, +but its convergence actually has two real-time loops and a fixed sleep. The +names of the other six describe real contracts; they are not useless, but their +measurement mechanisms are unreliable or potentially expose real races. They +need deterministic clocks/schedulers, allocation warmup/bounds, and captured +race details before the suite can be a release gate. + +### T-015 — Four explicit skips are tools, an empty scaffold, and a hidden known failure + +Beyond the 73 custom installed-DAT skips, the remaining four reported skips +are not ordinary unavailable-platform tests: + +- `ChatLayoutFixtureGenerator.GenerateChatFixture` and + `RadarLayoutFixtureGenerator.GenerateRadarFixture` are manual artifact + generators permanently represented as skipped facts. They have no regression + oracle and belong in a documented tool command. +- `PvsConformanceTests.Pvs_CottageInterior_MatchesRetailCellDrawList` is an + empty future scaffold. It is genuinely useless until a captured oracle and + implementation exist; a TODO/issue is clearer than a permanent green-suite + skip. +- `TowerAscentReplayTests.RetailShouldSeeTwoCells_AtFailureWindow` is a known + #119 residual deliberately disabled “until the fix.” A skipped failing + contract cannot guard against worsening or resolution; track the expected + current behavior separately, or make it an explicit non-blocking known- + failure lane with ownership and expiry. + +Together with T-006, these explain all 77 reported skips. The release report +should distinguish unavailable external prerequisites from manual generators, +unfinished tests, and known product failures. + +## Tests whose maintained names overclaim their behavior + +This is the actionable naming subset, not a demand to rename every +campaign-prefixed test. A name is listed when its stated outcome is materially +stronger than its oracle: + +| Test | Why the name is wrong or misleading | Classification | +|---|---|---| +| `PvsConformanceTests.Pvs_CottageInterior_MatchesRetailCellDrawList` (`:23`) | Empty body and permanently skipped; performs no comparison. | useless unfinished scaffold | +| `SmokeTest.TestProject_IsWired` (`:7`) | Only `Assert.True(true)`; compilation already establishes project wiring. | useless tautology | +| `ChaseCameraTests.ImplementsICamera` (`:59`) | Assignment proves only a compile-time relationship and `ToString()` is not asserted. | useless compile-only check | +| `GpuContractTests.TheDepthStencilAttachmentFormatCarriesAStencilAspect` (`:245-257`) | Checks clear/load fields, then “verifies” the format with `Assert.Equal(GpuTextureFormat.Depth24Stencil8, GpuTextureFormat.Depth24Stencil8)`; the pass description carries no format at all. | wrongly named and missing its central oracle | +| `RuntimeLocalPlayerFirstEntryStateTests.RetryAtAwaitingActivationNeverRecommitsPublicationOrDuplicatesTheBody` (`:424-438`) | The body half is checked across retries, but the adjacent controller stability assertion compares the property with itself. | partially overclaimed | +| `EnvCellSoundEmitterInventoryTests.PortalDat_SetupsWithAmbientSlotSoundTables` (`:130`) | Inventories/prints candidates without asserting that any qualifying setup exists. | output-only diagnostic named as existence contract | +| `EnvCellSoundEmitterInventoryTests.SoundTables_WithAmbientSlots_ExistForWireBinding` (`:187`) | Prints the inventory without an existence or binding assertion. | output-only diagnostic named as existence contract | +| `Issue176177FacilityHubFloodReplayTests.ScenarioB_StairDescent_RampCellRetention` (`:101`) and the `ScenarioC`/`ScenarioD`/`ScenarioE` peers (`:143,217,242`) | Calculate and print retention/churn; never assert the named retention or bistability result. | output-only replay named as contract | +| `Issue113DoorVanishDiagnosticTests.ReplicateProductionEmission_OnPortalFills` (`:194`) | Replicates and prints emission; no expected result is asserted. | diagnostic whose verb implies verified equivalence | +| `Issue176177DungeonSeamInspectionTests.CorridorNeighborhood_CoplanarOverlappingDrawnPolyPairs` (`:247`) and `UnderHall_DrawnPolys_SurfaceColors` (`:473`) | Dump calculated pairs/colors without asserting the noun phrase in the name. | inspections named as facts | + +The remaining output-only cases in T-011 generally advertise `Probe`, `Dump`, +`Diagnostic`, `Inspection`, or `Characterize`; those names are honest, but the +methods are still misplaced in the default release suite. Contract-like names +in T-011 should either gain a stable oracle or be renamed and moved with the +diagnostic apparatus. + +### T-016 — The test taxonomy preserves investigation history as its primary index + +Forty-seven test files begin with an `Issue###` identifier, 53 filenames contain +`Probe`, `Dump`, `Diagnostic`, `Inspection`, or `Characterization`, and 17 +contain `Replay`. The descriptive suffix often makes an individual file +understandable, so the issue-prefixed group is not automatically low-value. +The maintainability problem is that issue identity and one-off evidence shape +are the primary browsing taxonomy even after a behavior becomes permanent. +This couples discovery to the 19,073-line issue archive and mixes durable +regression contracts with T-011's output apparatus. When a test graduates into +a stable contract, organize it under the owning component/behavior and retain +the issue ID in a trait or comment; keep investigation programs in a separate, +non-default diagnostic project. diff --git a/src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs b/src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs index 252847c5..063c3656 100644 --- a/src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs +++ b/src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs @@ -199,6 +199,13 @@ public sealed class LauncherProcessSupervisor : ILauncherProcessSupervisor return; } + StopProcess(process, timeout); + } + + private static void StopProcess( + ILauncherChildProcess process, + TimeSpan timeout) + { process.TryRequestGracefulStop(); process.CloseMainWindow(); if (!process.WaitForExit(timeout) && !process.HasExited) @@ -214,12 +221,22 @@ public sealed class LauncherProcessSupervisor : ILauncherProcessSupervisor private void OnProcessExited(object? sender, EventArgs e) { - int? exitCode; - lock (_gate) + int? exitCode = null; + if (sender is ILauncherChildProcess process) { - exitCode = _process is { HasExited: true } process - ? process.ExitCode - : null; + try + { + exitCode = process.HasExited ? process.ExitCode : null; + } + catch (Exception error) + when (error is InvalidOperationException + or ObjectDisposedException) + { + // Disposal can detach the child after its native exit + // callback has already captured this handler. The terminal + // transition is still authoritative; only the optional exit + // code became unavailable during teardown. + } } SetState(LauncherSessionState.Exited, exitCode); @@ -325,21 +342,26 @@ public sealed class LauncherProcessSupervisor : ILauncherProcessSupervisor _disposed = true; process = _process; + _process = null; + } + + if (process is null) + { + return; } if (process is { HasExited: false }) { - Stop(DisposeStopTimeout); + StopProcess(process, DisposeStopTimeout); } - lock (_gate) - { - if (_process is not null) - { - _process.Exited -= OnProcessExited; - _process.Dispose(); - _process = null; - } - } + // Event removal and child disposal can wait for an already-running + // native Process.Exited callback. They must stay outside _gate: that + // callback commits the terminal state through SetState, which needs + // the same gate. Holding it here creates the exact + // supervisor-gate/Process-internals lock inversion this ownership + // transfer is intended to prevent. + process.Exited -= OnProcessExited; + process.Dispose(); } } diff --git a/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs b/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs index 837ed01b..e596a927 100644 --- a/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs @@ -500,6 +500,58 @@ public sealed class LauncherProcessSupervisorTests Assert.Equal(23, supervisor.ExitCode); } + [Fact] + public async Task DisposeAllowsAnAlreadyCapturedExitCallbackToComplete() + { + var factory = new ExitDisposeRaceChildProcessFactory(); + var supervisor = new LauncherProcessSupervisor(factory); + var states = new ConcurrentQueue(); + supervisor.StateChanged += (_, state) => states.Enqueue(state); + + supervisor.Start(Spec(), password: null); + ExitDisposeRaceChildProcess child = factory.LastCreated!; + child.BeginExit(47); + Assert.True( + child.ExitCallbackReady.Wait(TimeSpan.FromSeconds(5)), + "the captured exit callback did not reach its barrier"); + + Task disposeTask = Task.Run(supervisor.Dispose); + try + { + // Child disposal releases the captured callback and then waits + // for it to return. If the supervisor still holds _gate around + // child.Dispose(), the callback blocks in SetState and this + // bounded wait deterministically times out. + await disposeTask.WaitAsync(TimeSpan.FromSeconds(5)); + } + finally + { + // Keeps the test process recoverable against the old deadlocking + // implementation: release the fake's disposal wait so the failed + // assertion cannot leave a ThreadPool worker permanently blocked. + child.ReleaseDisposeForCleanup(); + await disposeTask.WaitAsync(TimeSpan.FromSeconds(5)); + await child.ExitCallbackTask.WaitAsync(TimeSpan.FromSeconds(5)); + } + + Assert.True(child.Disposed); + Assert.Equal(LauncherSessionState.Exited, supervisor.State); + Assert.Equal(47, supervisor.ExitCode); + Assert.Equal( + [ + LauncherSessionState.Starting, + LauncherSessionState.Running, + LauncherSessionState.Exited, + ], + states); + + // Idempotent convergence: a second explicit disposal neither touches + // the child again nor republishes terminal state. + supervisor.Dispose(); + Assert.Equal(1, child.DisposeCallCount); + Assert.Single(states, state => state == LauncherSessionState.Exited); + } + [Fact] public void LauncherProcessSpecCarriesNoCredentialLikeMember() { @@ -891,6 +943,108 @@ public sealed class LauncherProcessSupervisorTests } } + private sealed class ExitDisposeRaceChildProcessFactory + : ILauncherChildProcessFactory + { + public ExitDisposeRaceChildProcess? LastCreated { get; private set; } + + public ILauncherChildProcess Create(LauncherProcessSpec spec) + { + LastCreated = new ExitDisposeRaceChildProcess(); + return LastCreated; + } + } + + /// + /// Deterministically models the System.Diagnostics.Process exit/dispose + /// lock cycle. BeginExit captures the current delegate before the + /// supervisor can unsubscribe it. Dispose then releases that callback and + /// waits for it to return, just as Process.Dispose can wait on in-flight + /// exit machinery. The cleanup release exists only to let the test fail + /// boundedly against the old implementation instead of hanging its host. + /// + private sealed class ExitDisposeRaceChildProcess : ILauncherChildProcess + { + private readonly StringWriter _standardInput = new(); + private readonly ManualResetEventSlim _exitCallbackReady = new(false); + private readonly ManualResetEventSlim _releaseExitCallback = new(false); + private readonly ManualResetEventSlim _exitCallbackReturned = new(false); + private readonly ManualResetEventSlim _releaseDisposeForCleanup = new(false); + private EventHandler? _exited; + + public bool HasExited { get; private set; } + + public int ExitCode { get; private set; } + + public TextWriter StandardInput => _standardInput; + + public bool Disposed { get; private set; } + + public int DisposeCallCount { get; private set; } + + public ManualResetEventSlim ExitCallbackReady => _exitCallbackReady; + + public Task ExitCallbackTask { get; private set; } = Task.CompletedTask; + + public event EventHandler? Exited + { + add => _exited += value; + remove => _exited -= value; + } + + public void Start() + { + } + + public void BeginExit(int exitCode) + { + HasExited = true; + ExitCode = exitCode; + EventHandler? captured = _exited; + ExitCallbackTask = Task.Run(() => + { + _exitCallbackReady.Set(); + _releaseExitCallback.Wait(); + try + { + captured?.Invoke(this, EventArgs.Empty); + } + finally + { + _exitCallbackReturned.Set(); + } + }); + } + + public bool TryRequestGracefulStop() => false; + + public bool CloseMainWindow() => false; + + public void Kill() + { + throw new InvalidOperationException( + "the already-exited race child must not be killed"); + } + + public bool WaitForExit(TimeSpan timeout) => HasExited; + + public void ReleaseDisposeForCleanup() => + _releaseDisposeForCleanup.Set(); + + public void Dispose() + { + DisposeCallCount++; + _releaseExitCallback.Set(); + _ = WaitHandle.WaitAny( + [ + _exitCallbackReturned.WaitHandle, + _releaseDisposeForCleanup.WaitHandle, + ]); + Disposed = true; + _standardInput.Dispose(); + } + } + private sealed class FakeChildProcess( LauncherProcessSpec spec, bool exitsWithinStopTimeout,