diff --git a/planning/BUILD-MILESTONES.md b/planning/BUILD-MILESTONES.md index 3b98940..55bbced 100644 --- a/planning/BUILD-MILESTONES.md +++ b/planning/BUILD-MILESTONES.md @@ -1,6 +1,6 @@ # Adventure Storyteller — Production Build Milestones -**Status:** In implementation. M1 complete (2026-09-02); M2 next +**Status:** In implementation. M1 and M2 complete (2026-09-02); M3 next **Base:** AI-DnD `d72f7c1bda0f34fccd84afb7a25c34eb01c901de` ## 1. Purpose @@ -166,6 +166,37 @@ Add: The codebase has a narrow single-user/local-only surface and the inherited story foundation still passes its relevant regression suite. +## Status: COMPLETE + +Accepted 2026-09-02. Evidence: `planning/reports/M2-BASELINE-REPORT.md` +(measurements) and `planning/reports/M2-IMPLEMENTATION-REPORT.md` (review); +verdict *accept with non-blocking debt, proceed to M3*. Implementation is +commit `8c65ae9`, and the three defects the review found are commit `8652fe7` +— see the closeout note appended to both reports. + +**Capabilities M2 delivered, which later milestones inherit rather than build:** + +- a single-user product with no accounts, sessions or auth — all four + `/api/auth/*` routes are gone, not gated; 52 API routes down to 36, +- Ollama as the only inference backend, with no cloud provider code and no API + key anywhere in the product, +- an **address-based inference endpoint policy**, enforced on save and again + before every outbound request, that refuses public addresses even when the + database is edited behind the settings API (ADR 011), +- M1's trusted-LAN and TLS behaviour carried through the removal intact — + verified HTTPS against a real second machine, no bypass option, +- SQLite as the only store; Postgres, Neon and the Render deployment path removed, +- no campaign scripting: the QuickJS engine and `/api/scripts` are gone, +- a materially simpler runtime — 10 environment variables down to two, 6 Python + and 21 npm packages removed, and a 933 kB bundle down to 395 kB. + +**Debt carried forward, none of it blocking M3:** `Settings.model` still +defaults to `""` with nothing prompting for it (M8); inert legacy tables and +columns await a cleanup migration once the schema settles, after M3/M5; there +are still no frontend tests (M8); `docs/*.html`, upstream's project site and not +served by the app, still links Google Fonts. Full table in the implementation +report §P. + --- # M3 — Production Non-Destructive History, Redo, and Active-Head Export @@ -310,6 +341,34 @@ Remove or demote: Accepted story state is genre-neutral, auditable, reconstructable, and no longer depends on ambiguous relative deltas. +## Note from M2 — eight rollback tests use the world-state engine as instrumentation + +M2 removed QuickJS. Eight existing rollback/history tests had used a JavaScript +counter as deterministic instrumentation — a value they could change on a turn +and then assert had been rolled back — and they now use the inherited +RPG/world-state delta machinery for the same purpose. + +Read those tests correctly before touching them: + +- **what they exercise** is rollback and state reconstruction across undo, + retry, takes and divergence; +- **their use of the world-state engine is test instrumentation, not an + endorsement** of RPG-shaped state as the target architecture. Nothing about + them argues against the typed-event model this milestone installs; +- **when M5 replaces or generalizes the world-state protocol, the + instrumentation must move with it** to the new narrative-state mechanism. In + practice that is one schema entry and one helper in `backend/tests/fakes.py`; +- **preserve or rework these tests; do not delete them** merely because their + current instrumentation is RPG-shaped. The behaviour they pin is exactly the + behaviour M5 is most likely to break. + +`test_state_revert` and `test_delete_state` additionally assert *destructive* +undo semantics and are expected to be rewritten by M3; that is separate from +this instrumentation point, and they should likewise be rewritten rather than +dropped. + +Evidence: `planning/reports/M2-IMPLEMENTATION-REPORT.md` §K.2, §Q. + --- # M6 — Branch-Safe Context, Summaries, and Long-Term Story Memory @@ -348,6 +407,28 @@ Align inherited AI-DnD memory/context behavior with the final authority and hist Long-running story context is lineage-safe, authority-aware, local, inspectable, and bounded. +## Note from M2 — background memory failure must be observable + +M2 shipped with the memory bank entirely dead, and the full suite stayed green. +Summaries and embeddings raised `AttributeError` inside a fire-and-forget task: +no user-visible error, no log a player would read, and no failing test, because +every memory test stubs the provider factories out. + +M6 therefore additionally requires: + +- **memory/summarization background failures must be observable** — a + fire-and-forget task that dies must leave a record a user or maintainer can + actually find, rather than being swallowed; +- **tests must exercise at least one real provider-construction/wiring path**, + not only mocked factories, so that a moved or removed setting surfaces as a + test failure (`TECHNICAL-DESIGN.md` §18.1); +- **derived-memory failure must not corrupt accepted story state.** Already in + the scope list above; M2's evidence is why it stays there. A dead memory bank + degraded the storyteller quietly and left the transcript correct, which is the + right failure direction — but it must also be a *visible* one. + +Evidence: `planning/reports/M2-IMPLEMENTATION-REPORT.md` §A.1, §9.1. + --- # M7 — First-Class Imported Knowledge Library diff --git a/planning/DECISIONS/011-local-inference-endpoint-policy.md b/planning/DECISIONS/011-local-inference-endpoint-policy.md new file mode 100644 index 0000000..940c349 --- /dev/null +++ b/planning/DECISIONS/011-local-inference-endpoint-policy.md @@ -0,0 +1,118 @@ +# ADR 011 — Local Inference Endpoint Policy + +**Status:** Accepted +**Date:** 2026-09-02 +**Implemented in:** M2, `backend/app/endpoints.py` + +## Decision + +v1 supports Ollama only, and will send a story to an inference endpoint **only** +when every address that endpoint resolves to lies inside an explicit allowlist +of local networks. + +- The storyteller UI/API remains **loopback-bound by default**. Configuring a + LAN inference endpoint does not change where the storyteller listens. +- Inference may use **same-host loopback** (the default) or an **explicitly + configured trusted-LAN/local-network address**. +- **Public Internet addresses are denied.** +- The policy is **address-based**, using explicit allowed CIDRs rather than + Python's generic `is_private` / `is_reserved` classifications. +- **Every** resolved address must be allowed; one address outside the allowlist + refuses the endpoint. +- The policy is checked **on configuration and again before every request**. +- **TLS verification is mandatory** for HTTPS and is never traded against this + policy. +- Arbitrary cloud / OpenAI-compatible endpoints are **intentionally outside v1**. + +## Context + +The endpoint setting is the most consequential in the application. The +storyteller sends the player's prose, the assembled context, the retrieved +memories and the embedding inputs to whatever address it names: point it +somewhere else and the whole campaign goes there. + +The inherited AI-DnD guard could not be reused, because its rule is the +**opposite** of this product's. AI-DnD was a hosted service, so its SSRF guard +blocked *private* addresses to stop a user reaching the server's internal +network. A local storyteller must do exactly the reverse — permit the private +ranges and refuse the public Internet. The guard was removed rather than adapted. + +## Alternatives Considered + +- **Hostname matching / a denylist of cloud providers.** Rejected as the primary + rule: it is trivially talked around by spelling a name differently, by a + `CNAME`, or by a private DNS entry pointing at a public host. A denylist of + known cloud hostnames is retained, but only to make the *error message* + explain why — the address rule already refuses all of them. +- **Python's `is_private` / `is_reserved`.** Rejected; see below. +- **Loopback-only inference.** Rejected: trusted-LAN inference is accepted + production behaviour under ADR 002, and a great many users will run Ollama on + the one machine in the house that has a GPU. +- **UI-only discouragement.** Rejected: a setting that is merely absent from a + dropdown is still reachable by editing the database. + +## Reason for Explicit CIDRs + +Generic address classifications do not answer this project's security question, +and get it wrong in both directions for addresses this application actually sees: + +- `is_private` is **true** of the documentation ranges and of `0.0.0.0/8`, + neither of which is a user's LAN; +- `is_reserved` is **true** of IPv6 loopback, so a rule written around it + refuses `http://[::1]:11434/v1` — an ordinary same-host Ollama. + +Naming the networks keeps the policy readable, makes it auditable against this +document, and makes anything unnamed refused by default: + +```text +127.0.0.0/8 this machine ::1/128 this machine, v6 +10.0.0.0/8 RFC1918 fc00::/7 unique-local, v6 +172.16.0.0/12 RFC1918 fe80::/10 link-local, v6 +192.168.0.0/16 RFC1918 +169.254.0.0/16 link-local +100.64.0.0/10 carrier-grade NAT, which mesh VPNs such as Tailscale use +``` + +Carrier-grade NAT is included deliberately: it is what a mesh VPN such as +Tailscale hands out, and such a network is as user-controlled as a LAN. + +## Consequences + +- Validation runs in two places — `routers/settings.py` on save and on the + connection test, and `providers/openai_compatible.py` before the generate, + chat and embedding requests. Configuration validation alone is not sufficient, + and a database edited behind the settings API must not become a way out. +- Because a hostname is resolved at check time, an endpoint given as a literal + address behaves most predictably. +- No cloud provider can be configured, so no API-key storage is needed. M2 + removed both. +- The error strings are user-facing and say what to do, not what failed + internally. + +## Accepted Residual Risks + +Documented rather than mitigated, and **not** M3 work: + +1. **A hostile host on a trusted LAN is inside the permitted boundary.** The + policy authorizes an address range, not a machine. If an attacker already + controls a device on the user's network and the user points the storyteller + at it, the story goes there. The defence is the inference host's own firewall + and network policy. +2. **A DNS-rebinding interval** exists between the policy's resolution of a + hostname and the HTTP client's own connection. The two resolutions are + separate, so a name that answers with a LAN address for the check and a public + address for the connection is theoretically possible. Using a literal address + closes it entirely. + +## Scope + +This ADR covers which inference endpoints the product will talk to. It is **not** +a general remote-access design: it says nothing about exposing the storyteller +itself beyond loopback, which remains out of scope for v1. + +## References + +- `SECURITY-THREAT-MODEL.md` §10A, §71A item 5, §77 +- `TECHNICAL-DESIGN.md` §5.1 item 4, §5.2 +- ADR 002 (Ollama-only, and the TLS consequence), ADR 004 (local-only production) +- `planning/reports/M2-IMPLEMENTATION-REPORT.md` §F, §K.1 diff --git a/planning/README.md b/planning/README.md index 15be316..f3df5f2 100644 --- a/planning/README.md +++ b/planning/README.md @@ -1,7 +1,7 @@ # Adventure Storyteller Planning Package -**Status:** Phase 0 complete; architecture selected; **Milestone M1 implemented and accepted (2026-09-02)**. -**Production coding:** Underway, milestone by milestone. M1 is done; M2 is the next milestone to brief. +**Status:** Phase 0 complete; architecture selected; **Milestones M1 and M2 implemented and accepted (2026-09-02)**. +**Production coding:** Underway, milestone by milestone. M1 and M2 are done; M3 is the next milestone to brief. This package contains the current product requirements, final Phase 0 architecture decisions, detailed subsystem designs, acceptance tests, research evidence, and the production milestone plan for the local-only interactive-story project. @@ -163,7 +163,13 @@ Milestone M1 COMPLETE (2026-09-02) fork + offline baseline see planning/reports/M1-*.md | v -Milestone M2 NEXT — brief not yet prepared +Milestone M2 COMPLETE (2026-09-02) + local-only surface + endpoint see planning/reports/M2-*.md + policy + | + v +Milestone M3 NEXT — brief not yet prepared + non-destructive undo/redo | v Implement and review milestone-by-milestone @@ -173,10 +179,33 @@ Implement and review milestone-by-milestone **One milestone at a time. Do not begin a milestone before its brief exists.** -M1 is complete and accepted; its evidence is in `reports/M1-BASELINE-REPORT.md` -and `reports/M1-IMPLEMENTATION-REPORT.md`. **No M2 brief has been prepared.** -The current action is to review the post-M1 planning corrections below before -writing one. +M1 and M2 are complete and accepted; the evidence is in `reports/M1-*.md` and +`reports/M2-*.md`. **No M3 brief has been prepared.** The current action is to +write one, informed by the post-M2 corrections below and by +`reports/M2-IMPLEMENTATION-REPORT.md` §Q, which records that M3's chokepoints +were left untouched or simplified by M2 and that the Phase 0B undo/redo spike +still applies. + +### Post-M2 corrections applied (2026-09-03) + +M2's review recommended six planning changes and reported rather than applied +them. All six are now applied, plus three additions drawn from the same evidence: + +| Document | Correction | +| --- | --- | +| `SECURITY-THREAT-MODEL.md` | New §10A records the inference endpoint policy **as implemented** — address allowlist, enforced on save and before every request, TLS never traded against it — with both residual limits stated. §71A item 5 marked resolved; §77 notes the required defaults are now met. | +| `TECHNICAL-DESIGN.md` §5.1 | Items 3 and 4 marked done; all five hardening items are now resolved. | +| `TECHNICAL-DESIGN.md` §5.2 | New: the M1/M2 production architecture recorded as fact — SQLite, Ollama-only, loopback storyteller, trusted-LAN inference accepted, public endpoints refused. | +| `TECHNICAL-DESIGN.md` §18.1 | New wiring rule from the M2 regressions: test a real consumer path when removing a setting, and prove a new setting reaches its component. | +| `DECISIONS/011-local-inference-endpoint-policy.md` | **New ADR.** Address-based allowlist over hostname matching, deny by default, checked twice, mandatory TLS — with the `ipaddress`-classification finding as the reason the CIDRs are spelled out. | +| `BUILD-MILESTONES.md` M2 | Marked COMPLETE with the capabilities it delivered and the debt it carried forward. | +| `BUILD-MILESTONES.md` M5 | Note: eight rollback tests now use the world-state engine as *instrumentation*, not as endorsement; move the instrumentation when M5 replaces the protocol, and rework rather than delete those tests. | +| `BUILD-MILESTONES.md` M6 | Note: background memory failure must be observable, at least one real provider-construction path must be tested, and derived-memory failure must not corrupt accepted story state. | +| `V1-ACCEPTANCE-TESTS.md` H10 | Strengthened: a wildcard origin must be rejected at startup, and an unknown `/api/...` path must 404 rather than returning the SPA with HTTP 200. | +| `V1-ACCEPTANCE-TESTS.md` H12 | **New.** Inference endpoint enforcement, including the defence-in-depth case: a public endpoint written into the database behind the settings API must still be refused at request time. | + +`SPECIFICATION.md` was deliberately **not** changed. M2 altered no product +requirement; it removed capability the specification never asked for. ### Post-M1 corrections applied (2026-09-02) diff --git a/planning/SECURITY-THREAT-MODEL.md b/planning/SECURITY-THREAT-MODEL.md index 0e44bb9..59a35c0 100644 --- a/planning/SECURITY-THREAT-MODEL.md +++ b/planning/SECURITY-THREAT-MODEL.md @@ -1,6 +1,7 @@ # Adventure Storyteller — Security Threat Model -**Status:** v1.0 — local-only hardening requirements informed by Phase 0B +**Status:** v1.1 — local-only hardening requirements informed by Phase 0B, with the +inference endpoint policy recorded as implemented in M2 (§10A, §71A item 5, §77) **Purpose:** Define the security and privacy boundaries for a local-only interactive storytelling application. ## 1. Security Objective @@ -231,6 +232,104 @@ http://inferencebox.local:11434 The LAN hostname/address must be an intentional user configuration. Do not infer that every non-loopback endpoint is trusted merely because it resolves. +## 10A. Inference Endpoint Policy As Implemented (M2) + +**Status:** implemented in M2, `backend/app/endpoints.py`. §10 above states the +requirement; this section records the rule that now enforces it, and what it +does not cover. ADR 011 records the decision. + +This section supersedes the assumption in §71A item 5 that the inherited +network guard was the starting point. It was not reusable: AI-DnD's guard was +an SSRF guard for a *hosted* deployment, and its rule is the **opposite** of +this product's. A hosted server blocks private addresses to stop a user +reaching its internal network; a local storyteller must permit exactly those +addresses and refuse the public Internet. The inherited guard was removed +rather than adapted. + +### The path + +```text +Browser -> storyteller on loopback +Storyteller -> SQLite / local files +Storyteller -> one approved Ollama endpoint +``` + +The Ollama endpoint is either same-host loopback (the default) or an explicitly +configured trusted-LAN/local-network endpoint. Configuring a LAN inference host +does not change where the storyteller itself listens: the UI/API remains +loopback-bound by default, and the endpoint setting has no influence on the +bind address. + +### The rule + +Endpoints are validated **by address against an explicit allowlist of +networks**, not by hostname matching: + +```text +127.0.0.0/8 this machine ::1/128 this machine, v6 +10.0.0.0/8 RFC1918 fc00::/7 unique-local, v6 +172.16.0.0/12 RFC1918 fe80::/10 link-local, v6 +192.168.0.0/16 RFC1918 +169.254.0.0/16 link-local +100.64.0.0/10 carrier-grade NAT, which mesh VPNs such as Tailscale use +``` + +- **Every** address the hostname resolves to must fall inside one of these + networks. One address outside is enough to refuse the endpoint, so a name + resolving to both a private and a public address does not squeak through. +- Public Internet addresses are **refused**, not merely discouraged or hidden + from a dropdown. +- The networks are spelled out rather than derived from Python's `is_private` / + `is_reserved` classifications, which do not answer this question: `is_private` + is true of the documentation ranges and of `0.0.0.0/8`, and `is_reserved` is + true of IPv6 loopback — so a rule built on it would refuse `http://[::1]:11434/v1`, + an ordinary same-host Ollama. See ADR 011. +- A short list of known cloud inference hostnames is checked first. The address + rule already refuses all of them; the list exists only so the error explains + *why* rather than leaving the user to suspect broken DNS. + +### Where it is enforced + +Twice, deliberately — configuration validation alone is not sufficient: + +1. **when settings are saved** (`routers/settings.py`), so the user gets an + immediate, specific error, and on the connection-test path; +2. **before every outbound request** (`providers/openai_compatible.py`, on the + generate, chat and embedding paths), because a name that resolved to a LAN + address this morning can resolve elsewhere this afternoon — and because a + database edited behind the settings API must not become a way out. + +M2 demonstrated the second at runtime: a cloud endpoint written straight into +SQLite with `sqlite3`, bypassing the API entirely, was still refused at the wire. + +### TLS + +HTTPS to a trusted-LAN Ollama with a privately issued certificate is supported. +TLS verification is **never traded against** the address policy: + +- certificate and hostname verification remain fully enabled, +- trust is the machine's own CA store unioned with certifi (`tlstrust.py`, ADR 002), +- there is **no `verify=False`, no bypass flag, and no "insecure" option** — + however private the address. + +### Residual limits + +Stated plainly, because the policy does not cover them: + +1. **A hostile host on a network the user treats as trusted is inside the + permitted boundary.** The policy authorizes an address range, not a machine. + If an attacker already controls a device on the user's LAN and the user + points the storyteller at it, the story goes there. Defending that is the + inference host's own firewall and network policy (§7), not this rule. +2. **A DNS-rebinding interval exists** between the policy resolving a hostname + and the HTTP client making its own connection. The two resolutions are + separate, so a name that answers with a LAN address for the check and a + public one for the connection is theoretically possible. Using a literal + address rather than a hostname closes it entirely. + +Both are **accepted residual risks for v1**, documented rather than mitigated. +Neither is M3 work. + ## 11. Imported Files Must Be Data Only Imported files must never be treated as executable application extensions. @@ -1108,9 +1207,10 @@ Phase 0B runtime validation found specific inherited behaviors that production m - executable campaign scripting is outside the v1 trust boundary. - Remove/disable the engine and replace any test-only instrumentation that depended on it. -5. **Endpoint policy mismatch** - - inherited network guarding is aimed at hosted deployment behavior, not at preventing accidental story-data exfiltration. +5. **Endpoint policy mismatch** — **resolved in M2.** + - inherited network guarding is aimed at hosted deployment behavior, not at preventing accidental story-data exfiltration. Its rule was in fact the *opposite* of this product's, so it was removed rather than adapted. - Production should default to loopback Ollama, explicitly support a configured trusted-LAN Ollama host, and reject/avoid arbitrary public Internet inference endpoints. + - **Done.** Implemented as an address-based allowlist enforced on save and again before every request; see §10A and ADR 011. 6. **Postgres is removable** - Phase 0B found no architectural blocker to dropping Postgres support; SQLite remains the v1 store. @@ -1222,7 +1322,8 @@ local lexical/semantic retrieval optional explicitly configured local media services in the future ``` -Required production defaults: +Required production defaults. **As of M2 every item below is implemented**; +the endpoint rule that enforces the third and fourth is recorded in §10A: - storyteller binds loopback by default, - Ollama endpoint is same-host loopback by default, diff --git a/planning/TECHNICAL-DESIGN.md b/planning/TECHNICAL-DESIGN.md index fdbcd6d..7ac55d1 100644 --- a/planning/TECHNICAL-DESIGN.md +++ b/planning/TECHNICAL-DESIGN.md @@ -201,8 +201,8 @@ Production defaults must not require: ### 5.1 Known AI-DnD hardening work -Phase 0B identified concrete inherited violations, and M1 added a fifth. Items -1, 2 and 5 are **resolved**; items 3 and 4 remain open and belong to M2. +Phase 0B identified concrete inherited violations, and M1 added a fifth. **All +five are now resolved** — items 1, 2 and 5 in M1, items 3 and 4 in M2. 1. ~~`tiktoken` attempts to download the `cl100k_base` encoding on first use.~~ **Done in M1.** The encoding table is vendored in the tree and loaded @@ -211,21 +211,48 @@ Phase 0B identified concrete inherited violations, and M1 added a fifth. Items 2. ~~the SPA requests Google Fonts at runtime.~~ **Done in M1.** All three families are self-hosted, and the CSP names no remote origin at all. -3. hosted/multi-user/auth/demo/analytics/Postgres/cloud-provider/QuickJS paths are unnecessary. +3. ~~hosted/multi-user/auth/demo/analytics/Postgres/cloud-provider/QuickJS paths are unnecessary.~~ - remove them rather than merely hide them where practical. - - **Open — M2.** M1 removed nothing, so this surface is unchanged from the - fork point. -4. endpoint validation must reflect this product's threat model. + - **Done in M2, in full.** Removed rather than hidden: 52 API routes fell to + 36, and `/api/auth`, `/api/analytics` and `/api/scripts` are gone entirely + rather than gated. See §5.2. +4. ~~endpoint validation must reflect this product's threat model.~~ - same-host loopback Ollama is the default; an explicitly configured trusted-LAN Ollama endpoint is supported; arbitrary public/Internet model endpoints must be rejected or kept outside normal v1 configuration. - inference endpoint configuration must not change the storyteller's own loopback bind behavior. - - **Open — M2.** The trusted-LAN path itself works as of M1; what remains is - deciding and enforcing which endpoints normal v1 configuration may name. + - **Done in M2.** `backend/app/endpoints.py` applies an address-based + allowlist on save and again before every outbound request. Endpoint + configuration has no influence on the storyteller's own bind address. + ADR 011; `SECURITY-THREAT-MODEL.md` §10A. 5. ~~outbound TLS verified only against a bundled public-CA list, so a LAN host with a privately issued certificate was refused.~~ **Found and fixed in M1.** Not visible to Phase 0B: every run up to that point used plain HTTP over loopback, where certificate verification never happens. See *Transport to a trusted-LAN endpoint* above. +### 5.2 Production architecture as established by M1 and M2 + +The architecture below is no longer a selection; it is what the code does. It is +recorded here so later milestones inherit facts rather than intentions. + +| | | +| --- | --- | +| Production base | AI-DnD, forked at `d72f7c1` (§2, ADR 009) | +| Persistence | SQLite. Postgres, Neon and the Render deployment path are removed | +| Inference | Ollama only. No cloud provider code, no API key, no key UI | +| Storyteller bind | loopback by default, in every run path including the published Docker port | +| Ollama endpoint | same-host loopback by default; an explicitly configured trusted-LAN endpoint is equally supported | +| Public endpoints | refused by address, on save and before every request | +| Trusted-LAN HTTPS | supported, with full certificate and hostname verification against the machine's CA store; no bypass exists | +| Runtime assets | self-contained. Tokenizer table and fonts are vendored; the CSP names no remote origin | + +Removed in M2 rather than hidden: hosted accounts and auth, guest/demo +behaviour, hosted analytics, cloud inference providers, API-key storage and its +UI, Postgres/Neon/Render support, and QuickJS campaign scripting. + +**A trusted-LAN Ollama endpoint is accepted production behaviour**, not a +development convenience. Any statement that the only valid endpoint is literally +`127.0.0.1` is stale and should be read against §5 and §10A of the threat model. + ## 6. Browser UI Boundary The browser remains a presentation/control layer, not the owner of story authority. @@ -606,6 +633,24 @@ Required categories: Acceptance-test IDs in `V1-ACCEPTANCE-TESTS.md` are the black-box release contract. +### 18.1 Wiring rule, from the M2 regressions + +M2 shipped two defects that a 604-test green suite did not see: a removed +`Settings` attribute left two provider factories raising `AttributeError` inside +a background task, and a newly added timeout setting was stored, validated, +exposed and rendered without ever being passed to the provider that needed it. +Both were invisible because the tests at that boundary were mocks. + +> **When removing a setting, attribute or dependency, test at least one real +> consumer construction path. When adding a setting, test that the configured +> value reaches the component that uses it. A green suite built entirely around +> mocks at that boundary is insufficient evidence.** + +The corollary is where to look: subtractive changes and plumbing changes fail in +background and fire-and-forget paths, which are exactly the paths that report +nothing when they break. + + ## 19. Removal / Migration Strategy From Upstream Production migration should be incremental and test-gated rather than a broad rewrite. diff --git a/planning/V1-ACCEPTANCE-TESTS.md b/planning/V1-ACCEPTANCE-TESTS.md index a233f20..e79c3db 100644 --- a/planning/V1-ACCEPTANCE-TESTS.md +++ b/planning/V1-ACCEPTANCE-TESTS.md @@ -1,6 +1,7 @@ # Adventure Storyteller — V1 Acceptance Tests -**Status:** v1.0 planning/release contract — updated after Phase 0B +**Status:** v1.1 planning/release contract — updated after Phase 0B, and after M2 for +the security contract (H10 strengthened, H12 added) **Purpose:** Define black-box acceptance tests for finalist evaluation during Phase 0B and for the eventual v1 release. ## 1. Test Philosophy @@ -1174,12 +1175,32 @@ Archive extraction cannot write outside target root. --- -## H10 — Restrictive CORS +## H10 — Restrictive CORS and Local API Behavior **Priority:** REQUIRED FOR V1 +### Steps +1. Start the application with its default origin configuration and confirm the + SPA works. +2. Attempt to start the application with a wildcard origin configured + (`AIDND_CORS_ORIGINS="*"`). +3. Request an `/api/...` path that no router claims — a typo, or an endpoint + this build removed. + ### Pass -Privileged local APIs do not allow arbitrary wildcard cross-origin writes. +All three conditions, each independently: + +1. Privileged local APIs do not allow arbitrary wildcard cross-origin writes. +2. An unsafe wildcard production configuration is **rejected**: the application + refuses to start rather than honouring `*`. The storyteller API is + unauthenticated and loopback-bound, so a wildcard origin would let any web + page the user visits read and rewrite every campaign. +3. An unknown `/api/...` request returns an actual API **404**, rather than + falling through to the SPA mount and returning the page with HTTP 200. + +Conditions 2 and 3 were defects found and fixed during M2. Without naming them +here they can regress unnoticed, because both fail in a direction that still +looks like a working application. --- @@ -1204,6 +1225,58 @@ The application does not attempt to fetch tokenizer encodings, fonts, scripts, s --- +## H12 — Inference Endpoint Enforcement + +**Priority:** REQUIRED FOR V1 + +Defence in depth for the setting that decides where the story goes. +Configuration validation alone is **not** sufficient, so this test deliberately +checks the request-time rule as well. See ADR 011 and +`SECURITY-THREAT-MODEL.md` §10A. + +### Preconditions +- application installed and running, +- an Ollama instance reachable on this machine, +- an Ollama instance reachable on the user's own network (for step 2), +- the ability to edit the application database directly (for step 4). + +### Steps +1. Configure a **loopback** Ollama endpoint (`http://127.0.0.1:11434/v1`) and + generate a story turn. Repeat with the IPv6 form `http://[::1]:11434/v1`. +2. Configure an **approved trusted-LAN** Ollama endpoint by address and by + hostname, over HTTP and over HTTPS with a privately issued certificate, and + generate a story turn. +3. Attempt to configure a **public Internet** inference endpoint through the + normal settings API — both a known cloud provider hostname and an arbitrary + public address. +4. With the application configured legitimately, write a **public** endpoint + directly into the settings row in the database, bypassing the settings API + entirely, then attempt to generate a turn. + +### Pass +1. Loopback endpoints are **accepted**, in both IPv4 and IPv6 form. +2. Approved trusted-LAN/local-network endpoints are **accepted**, and the HTTPS + case succeeds with certificate and hostname verification fully enabled and no + bypass available. +3. Public Internet endpoints are **rejected** through normal configuration, with + an error that says why and what to use instead. +4. Request-time enforcement **still rejects** the public endpoint written behind + the settings API: no story text, context, memory or embedding input leaves + the machine for that address. The turn fails with the endpoint's rejection + reason rather than succeeding. + +A build that passes 1-3 but fails 4 has configuration validation only, and does +not pass this test. + +### Notes +Every address a hostname resolves to must be inside the allowed local networks; +one address outside is enough to refuse the endpoint. The two known residual +limits — a hostile host already on the trusted LAN, and the DNS-rebinding +interval between the policy's resolution and the client's connection — are +accepted residual risks and are **not** failures of this test. + +--- + # I. Export, Backup, and Restore ## I01 — Export Campaign diff --git a/planning/VERSION.md b/planning/VERSION.md index 2fbddb9..28e4538 100644 --- a/planning/VERSION.md +++ b/planning/VERSION.md @@ -1,8 +1,42 @@ # Planning Package Version -**Package:** Adventure Storyteller Planning Package v2.1 -**Revision date:** 2026-09-02 -**Status:** Phase 0 complete; architecture selected; **Milestone M1 implemented and accepted**; M2 not yet briefed. +**Package:** Adventure Storyteller Planning Package v2.2 +**Revision date:** 2026-09-03 +**Status:** Phase 0 complete; architecture selected; **Milestones M1 and M2 implemented and accepted**; M3 not yet briefed. + +## v2.2 — Post-M2 Closeout (2026-09-03) + +M2 removed the hosted, cloud, account and scripting surface and added the +inference endpoint policy. Its review recommended six planning changes and +reported rather than applied them; all six are applied in this revision, listed +in `README.md` § *Post-M2 corrections applied*, with the evidence in +`reports/M2-BASELINE-REPORT.md` and `reports/M2-IMPLEMENTATION-REPORT.md`. + +In summary: + +- the **inference endpoint policy is recorded as implemented** — an address + allowlist of explicit local-network CIDRs, every resolved address checked, + enforced when settings are saved and again before every outbound request, with + TLS verification never traded against it (new **ADR 011**, + `SECURITY-THREAT-MODEL.md` §10A), +- its two **residual limits are stated rather than mitigated**: a hostile host + already on the trusted LAN, and the DNS-rebinding interval between the + policy's resolution and the client's connection, +- `TECHNICAL-DESIGN.md` §5.1 items 3 and 4 are **resolved**, and a new §5.2 + records the M1/M2 production architecture as fact, +- a **wiring rule** is added (§18.1): removing a setting requires testing a real + consumer path, and adding one requires proving it reaches its component — M2 + shipped two defects behind a 604-test green suite because the tests at that + boundary were mocks, +- `BUILD-MILESTONES.md` records **M2 complete**, warns M5 that eight rollback + tests use the world-state engine as instrumentation rather than as + architecture, and requires M6 to make background memory failure observable, +- the security acceptance contract is strengthened: **H10** now names the + wildcard-origin and `/api` 404 conditions, and new **H12** covers inference + endpoint enforcement including the database-edited-behind-the-API case. + +`SPECIFICATION.md` is unchanged: M2 altered no product requirement. Nothing in +the architecture selected in v2 was reversed. ## v2.1 — Post-M1 Corrections (2026-09-02) diff --git a/planning/reports/M2-BASELINE-REPORT.md b/planning/reports/M2-BASELINE-REPORT.md index fce4124..f902b42 100644 --- a/planning/reports/M2-BASELINE-REPORT.md +++ b/planning/reports/M2-BASELINE-REPORT.md @@ -866,3 +866,63 @@ Stated so the implementation report does not overclaim. (§5.4). The individual capabilities — narration, streaming, embeddings, memory writes, retry, fork, restart — were each measured separately. 3. **No load, soak or long-campaign testing.** Out of scope for M2. + +--- + +# 15. Closeout note — appended 2026-09-03 + +**This section was appended after the fact and is not part of the original +evidence record.** Everything above was written on 2026-09-02 and describes the +repository as it stood then. Nothing above has been rewritten, including its +references to a working tree that was dirty at the time. + +## What the original report correctly said + +The report above correctly described commit `8c65ae9` — the M2 implementation +commit — as **not containing** the three fixes this review found. At the time it +was written, those fixes existed only in the working tree, six files ahead of +that commit. Every runtime measurement in this report was produced by an image +built from that fixed tree, which the report states plainly. + +## What happened afterwards + +The six-file correction was committed: + +```text +8652fe7cd84bca5173abb03b2a692f15fea8a98c M2 review: two regressions the green suite hid, and the reports +``` + +That commit carries the six implementation/test/lockfile files **and** the two +M2 reports themselves, which is why this report's own history begins there. The +provenance distinction the review asked for is preserved regardless: +`8c65ae9` is the M2 implementation, `8652fe7` is the review correction, and the +two were never squashed. + +## Verification at closeout + +Re-run on 2026-09-03 against the committed tree — not the working tree — so that +what was verified is exactly what the repository contains: + +```text +backend .venv/bin/python -m pytest tests/ -q 606 passed in 133.26s +frontend npm run lint 7 warnings, 0 errors, exit 0 +frontend npm run build built in 932ms; index.js 395.41 kB +root docker build -t storyteller-m2-closeout . exit 0 +git status --short clean +``` + +The 606 figure matches the count this report recorded, from the same tree. + +Dependency removal was verified in the built image rather than only in the +lockfile: + +```text +$ docker run --rm --entrypoint sh storyteller-m2-closeout -c "pip list | grep -iE 'quickjs|psycopg|cryptography|cffi|pycparser'" +ABSENT: none of quickjs/psycopg/cryptography/cffi/pycparser installed +32 packages total +``` + +The network measurements in §5, §6 and §10 were **not** re-run. The six +corrected files change provider construction and a timeout value; they do not +touch the endpoint policy, the TLS trust context, or the bind addresses, so the +captures above remain the evidence for the network boundary. diff --git a/planning/reports/M2-IMPLEMENTATION-REPORT.md b/planning/reports/M2-IMPLEMENTATION-REPORT.md index a8f96e3..13d7bb2 100644 --- a/planning/reports/M2-IMPLEMENTATION-REPORT.md +++ b/planning/reports/M2-IMPLEMENTATION-REPORT.md @@ -756,3 +756,95 @@ The three planning corrections marked *before M3* in §O — the threat model, t technical-design hardening list, and a new ADR for the endpoint policy — are documentation of decisions already made, not new work, and can be done alongside the M3 brief. + +--- + +# S. Closeout note — appended 2026-09-03 + +**Appended after the fact.** Sections A-R above were written on 2026-09-02 and +are left as they were, including §A.1's and §P's statements that the fixes were +uncommitted at the time. Those statements were true when written and are the +reason this note exists rather than an edit. + +## The one thing §R said to do first + +§R closed with: *"The one thing to do first is commit the six-file fix (§A.1). +Until then the branch head contains a silently broken memory bank."* + +Done: + +```text +8652fe7cd84bca5173abb03b2a692f15fea8a98c M2 review: two regressions the green suite hid, and the reports +``` + +The commit carries the six implementation/test/lockfile files **and** these two +reports. `8c65ae9` remains the M2 implementation commit and was not amended or +squashed, so the provenance distinction §R wanted — original implementation +versus review-discovered correction — survives in the history. + +Confirmed present in that commit, against `8c65ae9`: + +| Defect | Fix as committed | +| --- | --- | +| §9.1 dead memory bank | `memorybank.py` — both provider factories stop reading the removed `Settings.api_key_plain`; `summary_provider` now passes `model_timeout_seconds` | +| §9.2 inert timeout | `turns.py`, `chat.py` and the summariser factory pass `settings.model_timeout_seconds` into the provider | +| §9.3 stale lock | `requirements.lock` drops `quickjs`, `psycopg`, `psycopg-binary`, `cryptography`, and the transitive `cffi` and `pycparser` | +| — | `test_local_only_surface.py` gains the two regression tests: every factory built from a real `Settings` row, and the configured timeout arriving at each generating client | + +The separate short timeout classes were kept, as §9.2 required: `CONNECT_TIMEOUT` +(10 s), `EMBED_READ_TIMEOUT` (60 s) and the connection-test/model-list path are +unchanged, and only the generation read timeout became configurable. + +## Verification at closeout + +Re-run on 2026-09-03 against the **committed** tree, working tree clean: + +```text +backend pytest tests/ -q 606 passed in 133.26s + tests/test_local_only_surface.py 32 passed + test_endpoint_policy + test_tls_trust + test_offline_assets + 47 passed +frontend npm run lint 7 warnings, 0 errors, exit 0 +frontend npm run build 395.41 kB, exit 0 +root docker build exit 0 +image quickjs/psycopg/cryptography/cffi/pycparser absent (32 packages) +``` + +The packet-capture exercises were not repeated; the corrected files do not touch +the network boundary. See §15 of the baseline report. + +## Planning recommendations from §O + +Three were marked *before M3* and are now applied, in commit +`Planning: record M2 closeout decisions`: + +- **§O.2** — `SECURITY-THREAT-MODEL.md` §10A records the endpoint policy as + implemented, with both residual limits stated; §71A item 5 is marked resolved; + §77 notes the required defaults are now met. +- **§O.4** — `TECHNICAL-DESIGN.md` §5.1 items 3 and 4 are marked done, and a new + §5.2 records the M1/M2 production architecture as fact rather than intention. +- **§O.5** — **ADR 011, *Local Inference Endpoint Policy***, records the + decision, including the `ipaddress`-classification finding from §K.1 as the + reason the CIDRs are spelled out. + +The remaining three are also applied, ahead of the *later* urgency §O gave them, +since they are one-paragraph edits: **§O.1** as an M5 note in +`BUILD-MILESTONES.md` about the world-state instrumentation, and **§O.3** as a +strengthened H10 in `V1-ACCEPTANCE-TESTS.md`. **§O.6** correctly asked for no +change to `SPECIFICATION.md`, and none was made. + +Two additions beyond §O, both drawn from evidence in this report: + +- an M6 note in `BUILD-MILESTONES.md` requiring background memory failure to be + observable and at least one real provider-construction path to be tested — + §A.1 and §9.1 are the argument for it; +- **H12, *Inference Endpoint Enforcement***, in `V1-ACCEPTANCE-TESTS.md`, whose + fourth pass condition is the database-edited-behind-the-API case this review + demonstrated at runtime in §F. + +`BUILD-MILESTONES.md` also gained M2's `## Status: COMPLETE` block, matching M1's. + +## Not done in this closeout + +M3 was not begun. Undo still deletes, and there is still no Redo. The debt table +in §P is unchanged apart from its first row, which this note closes.