diff --git a/AGENTS.md b/AGENTS.md index f0e67361..bbfa2e97 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,7 +24,7 @@ If you might do both, load both. They are short on purpose. ## Approach (universal) -Five principles. The first four are distilled from [Andrej Karpathy's observations on LLM coding pitfalls](https://github.com/multica-ai/andrej-karpathy-skills); the fifth is classic separation of concerns. Apply to every change, every role. **Tradeoff:** these bias toward caution over speed. For trivial tasks (typos, obvious one-liners), use judgment. +Six principles. The first four are distilled from [Andrej Karpathy's observations on LLM coding pitfalls](https://github.com/multica-ai/andrej-karpathy-skills); the fifth is classic separation of concerns; the sixth is what we test and why. Apply to every change, every role. **Tradeoff:** these bias toward caution over speed. For trivial tasks (typos, obvious one-liners), use judgment. ### 1. Think before acting Don't assume. Don't hide confusion. Surface tradeoffs. @@ -62,6 +62,16 @@ Put each responsibility with the component that owns the resource or contract, a - **Split by kind of work.** Pure transformation → its own module, no I/O (unit-testable). I/O and orchestration → the bot/CLI. Domain schema → the producer of that domain. - **Before adding a handler, ask "is this this component's concern, or just convenient here?"** If the logic is specific to another producer, keep the generic seam and push the specifics back to where they belong. Convenience is not a reason to couple. +### 6. Module tests over unit tests +A module test exercises one coherent piece of functionality from the outside, as a client of it would. Its job is to state the *intent* and pin the *expected behaviour* at the time of writing, so both survive every later refactor. This is the single most valuable thing we produce: implementations get rewritten, intent does not. + +- **Write from the caller's side.** Drive the module through its public surface. If a test reaches for a private helper, it is testing how the code works instead of what it promises, and it will break on refactors that broke nothing. +- **Read like good API documentation.** Name the behaviour, not the function. Say why the case matters. A new reader should learn what the module is *for* from its tests alone. +- **Prefer real collaborators.** Real stacklets, real Synapse, a real model via `stacktests ai local`. Mock only what you cannot run, and only at an external boundary. +- **Beware self-confirming tests.** A test written alongside the code it covers proves the two agree, not that either is right. When the fixture encodes the same assumption as the implementation, both pass and reality still disagrees. Assert against an external contract (a spec, a service's real response, an invariant we promise) rather than a restatement of the code. +- **Delete tests that only mirror the implementation.** If it could only fail when someone deliberately changes their mind, it is costing tokens and buying nothing. +- **demo-rig and e2e sit on top.** Module tests carry the intent; the rig lanes prove the wiring holds between real containers. Neither replaces the other. + ## Universal non-negotiables Apply to every role, every session. diff --git a/docs/agent/dev.md b/docs/agent/dev.md index d20be478..81016b3f 100644 --- a/docs/agent/dev.md +++ b/docs/agent/dev.md @@ -179,8 +179,11 @@ Profile details live in [../../tests/README.md](../../tests/README.md). Testing rules: +- **Module tests first, and they are the point.** See [AGENTS.md § 6](../../AGENTS.md). Test one coherent piece of functionality from the outside, as a client of it would, so the test states the intent and pins the expected behaviour for every later refactor. Write it to read like API documentation: name the behaviour, say why the case matters. demo-rig and e2e sit on top and prove the wiring; they do not replace this. +- **Do not write tests that mirror the implementation.** A test written next to the code it covers proves the two agree, not that either is right. Assert against something external: a spec, a real service's response, an invariant we promise. If a test could only fail when someone changes their mind, delete it. - **Behavioural TDD: RED then GREEN.** Write the failing test that captures the behaviour you want; make it pass with the smallest change; then refactor. - **Blackbox at the module boundary.** Test what a module promises through its public surface. Mock only external interfaces (network endpoints, the LLM), and only when truly required. +- **Prefer a real model over a stubbed one.** `tests/integration/stacktests ai local` points the rig at a self-hosted endpoint: real answers, no cost per call, only slower. A green run against a stub proves the wiring, not the behaviour. - **Docker integration tests when warranted.** If a change crosses a container boundary or depends on a real service's behaviour, add a test under `tests/integration/`. - **Tests run against real stacklets and real hooks.** No parallel test-only compose files. - **Use real Synapse via the `messages` stacklet.** No handwritten Matrix mocks. @@ -189,7 +192,7 @@ Testing rules: - **Test helpers do one thing.** Add a parameter only when a second test needs it - not preemptively. - **`tests/integration/eval/` is opt-in** (slow, real model). Excluded from `pytest tests/` by `norecursedirs`. - **Write tests before fixing.** No duct tape. -- **Before running integration / e2e tests, ASK.** They collide with the user's running instance. +- **The rig is shared, not off-limits.** This repo root is the Simpsons dev instance, not anyone's real famstack, so agents may run the rig lanes. Ports are fixed, so exactly one run at a time: check nothing else is mid-run before starting. `tests/integration/stacktests help` lists which subcommands are autonomous, shared, or destructive. ## Code style @@ -235,7 +238,7 @@ Pre-tag gate, in order. A published tag is never moved; anything missed here shi ## Pre-1.0 conventions - Invariant changes (marker semantics, field renames, contract shifts) get coherent commits - each stands alone for revert. -- Cleanup backlog lives at `docs/cleanup-backlog.md`. Items there have a reason; surface them when adjacent code is touched. +- Actionable work lives on the tracker board, one card each, every card carrying a verification gate. Design notes - decisions, rejected dead ends, known-but-unresolved tensions - live at `docs/design-notes.md`; surface them when adjacent code is touched. If a note grows a "do this next", move it to a card and leave the reasoning behind. - Don't add backwards-compatibility shims, feature flags for one-shot migrations, or renamed `_unused` vars. ## What NOT to do diff --git a/docs/cleanup-backlog.md b/docs/cleanup-backlog.md deleted file mode 100644 index 484143bf..00000000 --- a/docs/cleanup-backlog.md +++ /dev/null @@ -1,129 +0,0 @@ -# Cleanup backlog - -Items land here with a reason. Surface them when adjacent code is touched -(see `docs/agent/dev.md`, Pre-1.0 conventions). - -## Wiki freshness follow-ups (curator shipped 2026-06-11) - -The curator sidecar ships the first two freshness tiers: debounced -incremental rebuilds (persons + home) and the nightly full sweep. -Design notes that survive it, for whoever touches this next: - -- **Realtime is NOT a requirement.** The mirror is realtime; the wiki - is a derived view. The nightly sweep makes the incremental person - mapping merely *helpful*, never load-bearing — worst case for a - mapping miss is "stale until tonight". Don't grow the incremental - heuristics; grow the deriver instead. -- **Page update strategy — design when it's time, but the tension is - known (2026-06-11):** full regeneration resamples page quality (a - good page can regress on the next sweep); evolving the existing - page accumulates errors that self-cite (the "Bartley [5]" finding). - Most promising middle: a fact-checking pass — "page + sources, fix - what the sources don't support, touch nothing else" — anchored to - ground truth while preserving good prose. Likely CLI shape then: - `wiki` = update/check, `wiki rebuild` = fresh full generation. -- **famstacker `wiki` command** (Server Room chat trigger) is the - missing third tier: "CLI commands are the primitives, the bot runs - or offers them". Needs the famstack API to allow the command and an - ack-then-report shape for the multi-minute run. -- `{"cmd": "notify"}` for the famstack API (containers → Server Room - via `stack messages send`) lives in the git stash - ("wiki auto-rebuild: curator sidecar + API notify") — platform - piece, ship it with whichever consumer arrives first; the curator's - completion notice is a natural one. -- Rejected runtime homes, don't re-litigate: host daemons (no launchd - surface), quartz container (node image; "the wiki never writes"), - bot-runner service concept (one consumer), bot-runner image reuse - (the curator uses 2 of its 10 deps; slim image won). - -## Paperless 3.x upgrade (deferred to its own branch, 2026-07-24) - -`:latest` + watchtower silently rolled Paperless-ngx from the 2.x line to -3.0.2 and broke document filing across the whole e2e suite. We pinned to -`2.20.15` (`stacklets/docs/docker-compose.yml`) so the current release -ships reproducibly; 3.x is a deliberate, separately-branched migration. -What we learned, for whoever does the 3.x branch: - -- **Already made resilient (don't redo):** `PaperlessAPI.wait_task` - (`stacklets/docs/bot/pipeline.py`) now parses *both* the 2.x and 3.0 - `/api/tasks/` shapes. 3.0 redesigned the task system (upstream #12584) - and paginated the listing (#12633): the response became a - `{"count", "results": [...]}` envelope (was a bare list), `status` - lowercased (`SUCCESS`→`success`), and the filed-doc reference moved - from scalar `related_document` to `related_document_ids` + - `result_data.document_id`. `_task_document_id` + the envelope-unwrap - handle all of it, covered by `TestWaitTask` in `test_pipeline.py`. -- **Still to verify on 3.x (not yet exercised against 3.0):** - - **Notes endpoint** (`add_note`/get/delete, `pipeline.py`) — 3.0 - "reject invalid requests to API notes endpoint" (#12582). The e2e - summary-note assertion passed on a transient 3.0.2 rig, so it's - probably fine, but confirm the POST body is still accepted. - - **Duplicate detection** — 3.0 moved checksums to SHA256 (#12432). - `PaperlessDuplicateError` keys off the rejection message; confirm the - message text/shape still matches `_DUPLICATE_RE`. - - **Permissions/owner scoping** tightened in 3.0; confirm the bot - token still reads/writes everything it needs (uploads carry - `owner_id`, so owner-filtered list endpoints may hide docs). - - Not affected (checked): we send no API-version header and use no - `=all` result expansion, both removed in 3.0. -- **One-way door — no downgrade.** Paperless runs DB migrations on - first 3.0 start and 2.x refuses to boot on a migrated DB. So: the 3.x - branch can't be tested by flipping the tag on an existing instance — - it needs a fresh docs volume. And the eventual **upgrade note must warn - users**: anyone already auto-bumped to 3.0 by watchtower cannot pin - back to 2.20.15 without restoring a pre-3.0 backup. -- **Adjacent:** `apache/tika:latest` (same compose file, line ~109) is - also unpinned — same silent-drift risk. Pin it when you touch this. - -## Agent bot-runner: stacker-bot rig-auth storm + room-modes intent-spec (2026-07-25) - -Surfaced while reconciling the `test_room_modes_e2e.py` xfail before merging -`feat/family-agent`. Two coupled issues, both agent-stacklet territory (fold -into the agent follow-up branch that also carries the runtime tools + -`stack memory person`): - -- **stacker-bot fails to authenticate in the e2e rig and floods the log.** - The managed rig brings up core/messages/docs/code, but this repo-root is also - the live Simpson demo, so the `agent` stacklet is enabled and the bot-runner - discovers + launches `stacker-bot` (`/stacklets/core/bot/stacker.py`). Its - account reports "ready" yet login fails 30 times - (`Login not ready` -> `Cannot authenticate - giving up`), because the rig - never provisions Stacky's credentials the way a real install does. Each failed - attempt logs `Error validating response: 'user_id' is a required property` - (nio validating a failed-login response against a schema that requires - `user_id`). The ~5-minute storm contends on the shared bot-runner and starves - the archivist, which is what flakes tight-timeout e2e tests. Real installs and - the live demo authenticate fine, so this is rig-specific. - - Fix directions (pick when the agent branch lands): bound + quiet the - failed-login retry so it can never starve the runner (helps every - environment); provision stacker-bot's credentials in the rig seed; and/or - skip launching a bot whose stacklet isn't actually provisioned here. - - Silence the `user_id` validation noise regardless: a failed login should - not emit a schema-validation error per attempt. - -- **`test_room_modes_e2e.py` still needs reconciliation.** The `!config process` - react-mode + bookmark feature is correctly implemented (verified: - `_maybe_handle_config_command`, `CONFIG_OPTIONS`, ack wording). The test is an - `xfail(strict=False)` intent-spec, hand-verified live on 2026-06-26 but never - run green through `stacktests`. Once the stacker-bot storm above is gone, - re-run it: it should pass, at which point drop the xfail. Do not delete it to - green CI (its own docstring says so). - -## CI release gate (post-0.3) - -## CI release gate (post-0.3) - -The pre-tag gate in `docs/agent/dev.md` is manual; v0.3.0-beta.1 shipped -with a stale `uv.lock` and a wrong `VERSION` string because of it. Move it -to CI in tiers: - -1. **Tag-triggered GitHub Action (cheap, do before the next tag):** - version consistency (`lib/stack/cli.py` VERSION == `pyproject.toml` == - tag name), `uv lock --check`, ruff, framework + stacklet unit tests. - Linux runner, no Docker needed. -2. **Integration suite in CI:** needs Docker + Synapse + Forgejo + the - OpenAI stub, ~35 min wall clock, assumes repo-root-as-instance. Real - work, decide after 0.3.0 final. -3. **Fresh-install + ai stacklet:** macOS/Apple Silicon only — needs a - self-hosted runner on the Mac Studio. Own project, don't start it - casually. diff --git a/docs/design-notes.md b/docs/design-notes.md new file mode 100644 index 00000000..baf0d095 --- /dev/null +++ b/docs/design-notes.md @@ -0,0 +1,50 @@ +# Design notes + +Decisions and dead ends worth remembering. Surface them when adjacent code +is touched (see `docs/agent/dev.md`, Pre-1.0 conventions). + +**This file is not a task list.** Anything actionable lives on the tracker +board, where it carries a verification gate and an owner. What stays here is +the material a card can't hold: why a shape was chosen, what was tried and +rejected, and which tensions are known but not yet resolved. If an entry +below ever grows a "do this next", move it to a card and leave the reasoning. + +## Wiki freshness (curator shipped 2026-06-11) + +The curator sidecar ships the first two freshness tiers: debounced +incremental rebuilds (persons + home) and the nightly full sweep. The +third tier (chat-triggered rebuild) is a card. + +- **Realtime is NOT a requirement.** The mirror is realtime; the wiki + is a derived view. The nightly sweep makes the incremental person + mapping merely *helpful*, never load-bearing - worst case for a + mapping miss is "stale until tonight". Don't grow the incremental + heuristics; grow the deriver instead. +- **Page update strategy - unresolved, and the tension is known + (2026-06-11):** full regeneration resamples page quality (a good page + can regress on the next sweep); evolving the existing page accumulates + errors that self-cite (the "Bartley [5]" finding). Most promising + middle: a fact-checking pass - "page + sources, fix what the sources + don't support, touch nothing else" - anchored to ground truth while + preserving good prose. Likely CLI shape then: `wiki` = update/check, + `wiki rebuild` = fresh full generation. +- **Rejected runtime homes, don't re-litigate:** host daemons (no launchd + surface), quartz container (node image; "the wiki never writes"), + bot-runner service concept (one consumer), bot-runner image reuse + (the curator uses 2 of its 10 deps; slim image won). + +## Surviving upstream drift: the `wait_task` pattern + +When Paperless-ngx 3.0 redesigned its task API, the fix that held up was +absorbing *both* response shapes in a single parser and covering both +offline: `PaperlessAPI.wait_task` (`stacklets/docs/bot/pipeline.py`) plus +`TestWaitTask` in `test_pipeline.py`. + +Worth copying whenever an upstream service changes a contract. One parser, +both shapes, proved in the `unit` lane - it turns a version migration into +a contained task instead of a rewrite, and it means the old version keeps +working while the new one is evaluated. + +The corollary is the reason it was needed: **an unpinned image is a +scheduled outage.** `:latest` plus watchtower rolled Paperless from 2.x to +3.0.2 unattended and broke filing across the whole e2e suite. diff --git a/lib/stack/cli.py b/lib/stack/cli.py index 77e174c5..e855f75a 100644 --- a/lib/stack/cli.py +++ b/lib/stack/cli.py @@ -22,6 +22,7 @@ from pathlib import Path from . import docker +from . import doctor from .commands import COMMANDS from .prompt import ORANGE, TEAL, GREEN, RED, DIM, BOLD, RESET from .stack import Stack @@ -827,6 +828,42 @@ def handle_status(stck, args): print_status(result) +def handle_doctor(stck, args): + """Diagnose the instance: what is wrong, and what to type to fix it. + + `status` answers "is it up?". When it isn't, this answers "why?" — + the checks live in doctor.py as pure rules; everything here is the + I/O they need. + """ + preferred = stck._cfg("core", "runtime", "orbstack") + docker.init_runtime(preferred) + + stacklets = sorted(s["id"] for s in stck.discover()) + findings = doctor.diagnose( + stacklets, + stck.env, + docker.containers_for, + docker.container_env, + docker.image_env, + ) + + print() + if not findings: + print(f" {GREEN}✓{RESET} {doctor.summarise(findings)}\n") + return + + for finding in findings: + mark = f"{RED}✗{RESET}" if finding.is_error else f"{ORANGE}⚠{RESET}" + print(f" {mark} {BOLD}{finding.title}{RESET}") + print(f" {DIM}{finding.detail}{RESET}") + print(f" {TEAL}{finding.fix}{RESET}\n") + + print(f" {doctor.summarise(findings)}\n") + # Exit non-zero on errors so an agent or script can gate on it. + if any(f.is_error for f in findings): + sys.exit(1) + + def handle_list(stck, args): print_list(stck.list(), stck) @@ -1225,6 +1262,7 @@ def _plugin_help(module_path: str): "down": handle_down, "destroy": handle_destroy, "status": handle_status, + "doctor": handle_doctor, "list": handle_list, "config": handle_config, "env": handle_env, @@ -1246,6 +1284,7 @@ def _plugin_help(module_path: str): ]), ("Info", [ ("list", "Show all stacklets and their status"), + ("doctor", "Diagnose problems and print how to fix them"), ("config", "Print stack.toml configuration"), ("config admin", "Print tech admin credentials"), ("env ", "Print rendered environment variables"), @@ -1331,6 +1370,7 @@ def main(): ) sub.add_parser("init") sub.add_parser("status") + sub.add_parser("doctor") sub.add_parser("list") p = sub.add_parser("config") config_sub = p.add_subparsers(dest="config_action") diff --git a/lib/stack/docker.py b/lib/stack/docker.py index 569da25e..8766bf39 100644 --- a/lib/stack/docker.py +++ b/lib/stack/docker.py @@ -59,12 +59,22 @@ def compose_up(compose_file: str | Path, env: dict = None) -> tuple[int, str]: `up -d`, leaving running containers stuck on stale env. `stack up` is a deliberate user action, so bouncing healthy containers is an acceptable cost for a reliable config-propagation contract. + + Selecting no services at all is success, not failure. When + COMPOSE_PROFILES excludes every service in the file, compose exits + 1 with "no service selected" — an empty selection, not a service + that refused to start. Treating it as an error meant a stacklet + whose containers are all optional could never finish setup, and + anything depending on it stayed blocked. The ai stacklet under + STACK_AI_NO_VOICE=1 is exactly that shape. """ full_env = {**__import__("os").environ, **(env or {})} result = _docker( "compose", "-f", str(compose_file), "up", "-d", "--force-recreate", capture_output=True, text=True, timeout=300, env=full_env, ) + if result.returncode != 0 and (result.stderr or "").strip() == "no service selected": + return 0, "" return result.returncode, result.stderr @@ -291,6 +301,99 @@ def project_states() -> dict[str, str]: return {} +def containers_for(stacklet_id: str) -> list[dict]: + """Every container of a stacklet, running or not. + + Returns dicts with name, state, exit_code and a human "since" string. + `stack status` only reports the stacklet as a whole, so a single dead + sidecar shows up as "failing" with no clue which one died. + """ + try: + r = _docker( + "ps", "-a", "--filter", f"name=^stack-{stacklet_id}-", + "--format", "{{.Names}}\t{{.State}}\t{{.Status}}", + capture_output=True, text=True, timeout=10, + ) + if r.returncode != 0: + return [] + out = [] + for line in r.stdout.strip().splitlines(): + parts = line.split("\t") + if len(parts) != 3: + continue + name, state, status = parts + # "Exited (128) 3 weeks ago" -> code 128, "3 weeks ago" + code, since = 0, status + if status.startswith("Exited ("): + head, _, tail = status.partition(")") + try: + code = int(head[len("Exited ("):]) + except ValueError: + code = 1 + since = tail.strip() + out.append({"name": name, "state": state, "exit_code": code, "since": since}) + return out + except Exception: + return [] + + +def _parse_env(text: str) -> dict: + """Turn `docker inspect`'s KEY=VALUE lines into a dict.""" + env = {} + for line in text.splitlines(): + key, sep, value = line.partition("=") + if sep: + env[key] = value + return env + + +def container_env(name: str) -> dict: + """The environment a container is actually running with. + + Read from the container rather than the compose file: a container keeps + the environment it was created with, so this is the only way to see that + it has drifted from current config. + """ + try: + r = _docker( + "inspect", name, "--format", "{{range .Config.Env}}{{println .}}{{end}}", + capture_output=True, text=True, timeout=10, + ) + if r.returncode != 0: + return {} + return _parse_env(r.stdout) + except Exception: + return {} + + +def image_env(name: str) -> dict: + """The environment baked into the image a container was started from. + + `container_env` returns the image's defaults *plus* whatever compose + passed in, and the two are indistinguishable once the container exists. + Reading the image separately is what lets a caller tell them apart, so + an image author's own setting is never mistaken for our config drifting. + """ + try: + r = _docker( + "inspect", name, "--format", "{{.Config.Image}}", + capture_output=True, text=True, timeout=10, + ) + image = r.stdout.strip() if r.returncode == 0 else "" + if not image: + return {} + r = _docker( + "inspect", image, "--format", + "{{range .Config.Env}}{{println .}}{{end}}", + capture_output=True, text=True, timeout=10, + ) + if r.returncode != 0: + return {} + return _parse_env(r.stdout) + except Exception: + return {} + + def running_project_ids() -> set[str]: """Convenience wrapper — stacklet IDs with running containers.""" states = project_states() diff --git a/lib/stack/doctor.py b/lib/stack/doctor.py new file mode 100644 index 00000000..94fd5257 --- /dev/null +++ b/lib/stack/doctor.py @@ -0,0 +1,195 @@ +"""Diagnose an instance: say what is wrong, and what to type to fix it. + +`stack status` answers "is it up?". When the answer is "no", it stops there. +Finding out *why* meant reading container logs, running `docker inspect`, +querying a service's database, and diffing that against stack.toml by eye. + +WHY THE CHECKS ARE GENERIC + Nothing here knows what Matrix is. The drift above is caught by + comparing a container's actual environment against what stack.toml + renders *now* - which finds the same class of bug for Paperless, + Forgejo, or any stacklet added later, including ones that do not exist + yet. A check that hardcodes one service's schema only ever finds that + service's bugs, and belongs to that stacklet, not here. + +This module is pure: it takes gathered facts and returns findings. All I/O +(docker inspect, reading config) lives in the caller, so every rule below +is unit-testable without a running instance. +""" + +from __future__ import annotations + +from dataclasses import dataclass + +ERROR = "error" +WARN = "warn" +INFO = "info" + +# Environment keys every container gets from the image or the runtime, not +# from stack.toml. Comparing them produces noise, never a real finding. +_RUNTIME_KEYS = frozenset({ + "PATH", "HOSTNAME", "HOME", "TERM", "LANG", "LC_ALL", + "PYTHON_VERSION", "PYTHONUNBUFFERED", "GPG_KEY", +}) + + +@dataclass(frozen=True) +class Finding: + """One diagnosis. `fix` is a command the reader can run verbatim.""" + + level: str + title: str + detail: str + fix: str + + @property + def is_error(self) -> bool: + return self.level == ERROR + + +def env_drift(rendered: dict, actual: dict, ignore: frozenset = _RUNTIME_KEYS) -> list[str]: + """Config keys whose live value no longer matches what stack.toml renders. + + Returns key names only, never values - this environment carries admin + passwords, API tokens, and database credentials, and a diagnostic that + prints them turns a config warning into a credential leak in whatever + log or issue tracker the output gets pasted into. + + Only keys the container actually carries are compared. A stacklet's + rendered env is the whole set for the compose project, while each + service receives its own subset - so "missing" overwhelmingly means + "this service was never given that variable", not "config drifted". + Reporting those made watchtower look like it had 36 problems and + buried the one setting that had genuinely changed. + """ + drifted = [] + for key, expected in rendered.items(): + if key in ignore or key not in actual: + continue + if actual[key] != str(expected): + drifted.append(key) + return sorted(drifted) + + +def compose_supplied(actual: dict, baked: dict) -> dict: + """The part of a container's environment that compose actually set. + + A container's environment is the image's own defaults plus whatever + compose passed in. Only the second half can drift from stack.toml; the + first half belongs to the image author and no famstack command can + change it. Comparing it produces a finding that is permanently true and + whose suggested fix provably does nothing, which is how gotenberg came + to report a TZ error that survived every `stack up docs`. + + A value equal to the image's default is treated as not-ours. That is + deliberately conservative: if compose sets a key to exactly what the + image already baked, real drift on that key goes unreported. Missing a + finding costs one debugging session; a permanent false positive teaches + the reader to skim past every finding, including the true ones. + """ + return {k: v for k, v in actual.items() if baked.get(k) != v} + + +def check_env_drift(stacklet: str, container: str, drifted: list[str]) -> Finding | None: + """A running container carrying superseded config.""" + if not drifted: + return None + return Finding( + level=ERROR, + title=f"{container} is running superseded config", + detail=( + f"{len(drifted)} setting(s) differ from what stack.toml renders now: " + + ", ".join(drifted) + + ". The container keeps its environment from creation time, so " + "editing stack.toml alone changes nothing until it is recreated." + ), + fix=f"stack up {stacklet}", + ) + + +def check_exited(container: str, exit_code: int, since: str) -> Finding | None: + """A container that stopped and stayed stopped. + + `stack status` reports the stacklet as failing without naming which + container died or when, which is the difference between a one-line fix + and a log-reading session. + """ + if exit_code == 0: + return None + return Finding( + level=ERROR, + title=f"{container} exited ({exit_code})", + detail=f"Stopped {since} and has not come back.", + fix=f"stack logs {container.split('-')[1] if '-' in container else container}", + ) + + +def check_endpoint(name: str, url: str, reachable: bool) -> Finding | None: + """A configured endpoint that does not answer. + + Covers the AI backend in particular: pointing at a self-hosted model + that is switched off fails deep inside a bot, as a timeout with no + mention of the endpoint. + """ + if reachable or not url: + return None + return Finding( + level=WARN, + title=f"{name} endpoint is not answering", + detail=f"Configured as {url}, but it did not respond.", + fix="tests/integration/stacktests ai # check or switch the backend", + ) + + +def diagnose(stacklets, rendered_env, containers_for, container_env, + image_env) -> list[Finding]: + """Run every check across the given stacklets. + + The five collaborators are injected rather than imported so the whole + walk is testable with plain dicts - no Docker, no instance. Each is a + callable taking a stacklet id (or container name) and returning facts. + + A stacklet whose env cannot be rendered is skipped rather than fatal: + one misconfigured stacklet should not stop the others being diagnosed, + which is the moment a doctor is most needed. + """ + findings: list[Finding] = [] + for stacklet in stacklets: + containers = containers_for(stacklet) + if not containers: + continue + + try: + rendered = rendered_env(stacklet) + except Exception: + rendered = None + + for container in containers: + name = container["name"] + if container["state"] != "running": + found = check_exited(name, container["exit_code"], container["since"]) + if found: + findings.append(found) + # A stopped container's environment says nothing useful. + continue + if rendered: + ours = compose_supplied(container_env(name), image_env(name)) + drifted = env_drift(rendered, ours) + found = check_env_drift(stacklet, name, drifted) + if found: + findings.append(found) + return findings + + +def summarise(findings: list[Finding]) -> str: + """One line for the reader who only wants the verdict.""" + if not findings: + return "No problems found." + errors = sum(1 for f in findings if f.is_error) + warns = len(findings) - errors + parts = [] + if errors: + parts.append(f"{errors} error{'s' if errors != 1 else ''}") + if warns: + parts.append(f"{warns} warning{'s' if warns != 1 else ''}") + return ", ".join(parts) + "." diff --git a/stacklets/agent/runtime/grep_tool.py b/stacklets/agent/runtime/grep_tool.py new file mode 100644 index 00000000..822b0007 --- /dev/null +++ b/stacklets/agent/runtime/grep_tool.py @@ -0,0 +1,85 @@ +"""Route vault grep calls through semantic family memory search.""" + +from __future__ import annotations + +import re +from typing import Any + +_PATH_RE = re.compile(r"^\s*#\d+\s+.*?\s([^\s]+\.md)\s+score=", re.MULTILINE) + + +def _is_vault_path(path: str | None) -> bool: + path = (path or ".").strip().replace("\\", "/") + return path in {"vault", "./vault"} or path.startswith(("vault/", "./vault/")) + + +def install() -> None: + """Patch nanobot's grep tool so vault searches use memory_search.""" + from nanobot.agent.tools.search import GrepTool + + original = GrepTool.execute + + async def execute_with_memory( + self: GrepTool, + pattern: str, + path: str = ".", + glob: str | None = None, + type: str | None = None, + case_insensitive: bool = False, + fixed_strings: bool = False, + output_mode: str = "files_with_matches", + context_before: int = 0, + context_after: int = 0, + max_matches: int | None = None, + max_results: int | None = None, + head_limit: int | None = None, + offset: int = 0, + **kwargs: Any, + ) -> str: + if not _is_vault_path(path): + return await original( + self, + pattern=pattern, + path=path, + glob=glob, + type=type, + case_insensitive=case_insensitive, + fixed_strings=fixed_strings, + output_mode=output_mode, + context_before=context_before, + context_after=context_after, + max_matches=max_matches, + max_results=max_results, + head_limit=head_limit, + offset=offset, + **kwargs, + ) + + limit = head_limit or max_results or max_matches or 10 + if limit == 0: + limit = 20 + + scope = None + normalized = path.strip().replace("\\", "/").removeprefix("./") + if normalized.startswith("vault/"): + scope = normalized.removeprefix("vault/").strip("/") or None + + from memory_tool import MemorySearchTool + + result = await MemorySearchTool().execute( + query=pattern, + limit=min(max(int(limit), 1), 20), + scope=scope, + ) + paths = [f"vault/{path}" for path in _PATH_RE.findall(result)] + path_block = "" + if paths: + path_block = "Paths to read:\n" + "\n".join(f"- {path}" for path in paths) + "\n\n" + return ( + "Semantic vault search via memory_search. " + "Use returned vault paths with read_file for source verification.\n\n" + + path_block + + result + ) + + GrepTool.execute = execute_with_memory diff --git a/stacklets/agent/runtime/memory_tool.py b/stacklets/agent/runtime/memory_tool.py new file mode 100644 index 00000000..a1f98171 --- /dev/null +++ b/stacklets/agent/runtime/memory_tool.py @@ -0,0 +1,104 @@ +"""Agent runtime tool for read-only family memory search.""" + +from __future__ import annotations + +import asyncio + +from nanobot.agent.tools.base import Tool, tool_parameters +from nanobot.agent.tools.schema import IntegerSchema, StringSchema, tool_parameters_schema + + +@tool_parameters( + tool_parameters_schema( + query=StringSchema( + "Natural-language question or keywords to search across the family vault.", + min_length=1, + ), + limit=IntegerSchema( + 5, + description="Maximum number of results to return.", + minimum=1, + maximum=20, + nullable=True, + ), + scope=StringSchema( + "Optional vault scope, such as family/itchy-scratchy-land. Leave empty for global search.", + nullable=True, + ), + person=StringSchema( + "Optional person filter, such as lisa or homer.", + nullable=True, + ), + tag=StringSchema( + "Optional tag filter.", + nullable=True, + ), + ) +) +class MemorySearchTool(Tool): + """Search the family vault through the memory stacklet.""" + + _scopes = {"core"} + + @property + def name(self) -> str: + return "memory_search" + + @property + def description(self) -> str: + return ( + "Search the family memory vault. Results include rank, score, vault path, " + "snippet, and source links when available. Use before answering factual " + "questions about family people, plans, documents, notes, bookmarks, or topics." + ) + + @property + def read_only(self) -> bool: + return True + + async def execute( + self, + query: str, + limit: int | None = None, + scope: str | None = None, + person: str | None = None, + tag: str | None = None, + ) -> str: + args = [ + "stack", + "memory", + "search", + query, + "--limit", + str(limit or 5), + ] + for flag, value in (("--scope", scope), ("--person", person), ("--tag", tag)): + if value: + args.extend([flag, value]) + + proc = await asyncio.create_subprocess_exec( + *args, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=130) + out = stdout.decode(errors="replace").strip() + err = stderr.decode(errors="replace").strip() + if proc.returncode != 0: + return f"Error: memory search failed with exit {proc.returncode}: {err or out}" + return out or "(no memory results)" + + +def install() -> None: + """Append MemorySearchTool to nanobot discovery without forking nanobot.""" + from nanobot.agent.tools.loader import ToolLoader + + original = ToolLoader.discover + + def discover_with_memory(self: ToolLoader) -> list[type[Tool]]: + tools = list(original(self)) + if MemorySearchTool not in tools: + tools.append(MemorySearchTool) + return tools + + ToolLoader.discover = discover_with_memory diff --git a/stacklets/agent/runtime/person_tool.py b/stacklets/agent/runtime/person_tool.py new file mode 100644 index 00000000..9ff902bb --- /dev/null +++ b/stacklets/agent/runtime/person_tool.py @@ -0,0 +1,68 @@ +"""Agent runtime tool for exact household profile reads.""" + +from __future__ import annotations + +import asyncio + +from nanobot.agent.tools.base import Tool, tool_parameters +from nanobot.agent.tools.schema import StringSchema, tool_parameters_schema + + +@tool_parameters( + tool_parameters_schema( + name=StringSchema( + "Person slug, canonical name, or synonym, such as homer or Marge Simpson.", + min_length=1, + ), + ) +) +class MemoryPersonTool(Tool): + """Read a household member profile through the memory stacklet.""" + + _scopes = {"core"} + + @property + def name(self) -> str: + return "memory_person" + + @property + def description(self) -> str: + return ( + "Read a household member's exact profile from the vault. Use this first " + "for questions about the sender, identity, profile, or 'what do you know about me'." + ) + + @property + def read_only(self) -> bool: + return True + + async def execute(self, name: str) -> str: + proc = await asyncio.create_subprocess_exec( + "stack", + "memory", + "person", + name, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=30) + out = stdout.decode(errors="replace").strip() + err = stderr.decode(errors="replace").strip() + if proc.returncode != 0: + return f"Error: memory person failed with exit {proc.returncode}: {err or out}" + return out or "(no profile found)" + + +def install() -> None: + """Append MemoryPersonTool to nanobot discovery without forking nanobot.""" + from nanobot.agent.tools.loader import ToolLoader + + original = ToolLoader.discover + + def discover_with_person(self: ToolLoader) -> list[type[Tool]]: + tools = list(original(self)) + if MemoryPersonTool not in tools: + tools.append(MemoryPersonTool) + return tools + + ToolLoader.discover = discover_with_person diff --git a/stacklets/agent/runtime/sitecustomize.py b/stacklets/agent/runtime/sitecustomize.py index e1bea8f4..462c1407 100644 --- a/stacklets/agent/runtime/sitecustomize.py +++ b/stacklets/agent/runtime/sitecustomize.py @@ -5,7 +5,9 @@ `nanobot` invocation in the container — the gateway and one-shot `nanobot agent` alike — with no fork. -Two independent shims live here; each is a thin monkeypatch over a pure module: +Two kinds of patch live here, each a thin monkeypatch over a pure module. + +First, two context shims that reshape what the model sees per turn: 1. brief (brief.py) — prepends a per-turn family briefing (who is speaking, the topic) to nanobot's runtime lines. Injected late (after the stable prompt and @@ -18,6 +20,14 @@ reciting stale data. The transcript (Matrix) keeps the full result; the state we feed the model keeps only a cheap pointer. +Second, three vault tools, which add capability rather than reshaping context: + +3. memory_tool (memory_tool.py) — a `memory_search` tool over `stack memory search`. +4. person_tool (person_tool.py) — a `memory_person` tool for exact profile reads. +5. grep_tool (grep_tool.py) — routes greps under `vault/` into memory_search, so + the agent gets semantic hits instead of literal matches on a corpus where the + words it greps for are rarely the words on disk. + WHY SHIMS AND NOT A FORK nanobot has no plugin seam for per-turn context injection or state shaping. Shims keep us on upstream `nanobot-ai` (updates included) with the change @@ -31,10 +41,21 @@ Dockerfile. nanobot reverts to stock behaviour with no other change. PIN / RECHECK ON UPGRADE (re-verify after any `nanobot-ai` version bump) - brief: `nanobot.agent.context.runtime_lines(state, msg, workspace, *, skip=False) -> list[str]` - lean_state: `nanobot.agent.context.ContextBuilder.build_messages(...) -> list[dict]` + brief: `nanobot.agent.context.runtime_lines(state, msg, workspace, *, skip=False) -> list[str]` + lean_state: `nanobot.agent.context.ContextBuilder.build_messages(...) -> list[dict]` + memory_tool: `nanobot.agent.tools.loader.ToolLoader.discover(self) -> list[type[Tool]]` + `nanobot.agent.tools.base.Tool`, `nanobot.agent.tools.base.tool_parameters` + `nanobot.agent.tools.schema.{StringSchema, IntegerSchema, tool_parameters_schema}` + person_tool: same symbols as memory_tool + grep_tool: `nanobot.agent.tools.search.GrepTool.execute(...) -> str` + + `tests/stacklets/test_agent_runtime_shims.py` asserts every one of these is + attached against a stub nanobot, so this list is executable rather than + aspirational: a moved symbol fails the unit lane instead of silently + reaching production as a logged warning nobody reads. """ +import importlib import logging _log = logging.getLogger("agent.runtime.shim") @@ -101,3 +122,20 @@ def _build_messages_lean(self, *args, **kwargs): _log.info("lean-state message shim active") except Exception: _log.exception("lean-state shim could not attach (nanobot internals changed?)") + + +# ── vault tools: memory_search, memory_person, and grep routed through them ── +# These add capability rather than reshaping context, but attach the same way. +# Each is installed in its own try so one tool failing costs only itself; a +# single shared block would let a moved GrepTool symbol take memory_search down +# with it. memory_tool goes first because grep_tool routes into it. +for _module_name, _what in ( + ("memory_tool", "memory_search tool"), + ("person_tool", "memory_person tool"), + ("grep_tool", "vault grep -> memory_search routing"), +): + try: + importlib.import_module(_module_name).install() + _log.info("%s active", _what) + except Exception: + _log.exception("%s could not attach (nanobot internals changed?)", _what) diff --git a/stacklets/agent/stacklet.toml b/stacklets/agent/stacklet.toml index 8f869ec1..ad60abf3 100644 --- a/stacklets/agent/stacklet.toml +++ b/stacklets/agent/stacklet.toml @@ -23,8 +23,16 @@ build = true [env.defaults] AGENT_DATA_DIR = "{data_dir}/agent" # The family memory vault (read-only) — Stacky's knowledge source, the same -# working copy the memory stacklet maintains and the wiki renders. -MEMORY_VAULT_DIR = "{data_dir}/memory/vault" +# working copy the wiki renders. +# +# The brain projection, not the memory source clone. Person and topic pages +# are generated, and generation writes them here; the installer purges them +# back out of source ("purged 1 generated source page(s)"). Pointing this at +# source gave the agent a tree that structurally could not hold a profile, so +# it answered "there is no vault/homer/about.md" for a household member who +# had one. The brain mirrors source *plus* generated pages, so it is the only +# tree that matches what this mount's contract promises. +MEMORY_VAULT_DIR = "{data_dir}/memory/brain" # Persona — the family-facing name and voice, from stack.toml [agent] name # (default "Stacky"). One knob: the same value drives the Matrix display name, # the @-bot handle, the # room, and the SOUL.md self-reference. diff --git a/stacklets/backup/cli/sync.py b/stacklets/backup/cli/sync.py index 7c45a1a7..6328216a 100644 --- a/stacklets/backup/cli/sync.py +++ b/stacklets/backup/cli/sync.py @@ -89,8 +89,7 @@ def _post_notification(plain: str, html: str, config: dict) -> Optional[str]: secrets = config.get("secrets", {}) server_name = stack_cfg.get("messages", {}).get("server_name", "home") - bot_pass = (secrets.get("core__STACKER_BOT_PASSWORD") - or secrets.get("messages__STACKER_BOT_PASSWORD", "")) + bot_pass = secrets.get("core__STACKER_BOT_PASSWORD", "") if not bot_pass: return "stacker-bot password not in secrets — is core set up?" diff --git a/stacklets/core/famstack-api.py b/stacklets/core/famstack-api.py index c6830995..6b5cb9ac 100644 --- a/stacklets/core/famstack-api.py +++ b/stacklets/core/famstack-api.py @@ -41,6 +41,7 @@ # commands above -- those stay on the JSON path used only by trusted core tools. DOMAIN_ALLOW = [ ["memory", "search"], + ["memory", "person"], ["memory", "topic"], ["memory", "lookup"], ["memory", "correspondents"], diff --git a/stacklets/memory/cli/person.py b/stacklets/memory/cli/person.py new file mode 100644 index 00000000..c3590f09 --- /dev/null +++ b/stacklets/memory/cli/person.py @@ -0,0 +1,127 @@ +"""stack memory person - read a household member's profile. + +Person pages are first-class vault entities at `/about.md`. This command +is the exact read surface for identity/profile questions, parallel to +`stack memory topic ` for shared topics. It reads the vault directly, so +the answer is deterministic and carries the source path the agent must cite. +""" + +from __future__ import annotations + +import argparse +import re +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) +from lib import ( # noqa: E402 + brain_path_for, + load_persons_from_vault, + refresh_vault_if_stale, + vault_path_for, +) + + +HELP = "Read a household member profile" + +_FRONTMATTER = re.compile(r"^---\n.*?\n---\n", re.DOTALL) +_GEN_MARKER = re.compile(r"\n?") +_CITE = re.compile(r"\[\d+(?:,\s*\d+)*\]") + + +def _parser() -> argparse.ArgumentParser: + p = argparse.ArgumentParser( + prog="stack memory person", + description=HELP, + ) + p.add_argument("name", nargs="*", help="person slug, canonical name, or synonym") + p.add_argument( + "--vault", default=None, metavar="PATH", + help="vault path override (defaults to /memory/vault/)", + ) + p.add_argument( + "--no-refresh", action="store_true", + help="skip the upstream-HEAD check before reading", + ) + return p + + +def _roots(config, override: str | None) -> list[Path]: + """Where a person page can live, most authoritative first. + + Two trees hold `/about.md` and they mean different things. + memory is source: what the household actually wrote. The brain is a + projection the wiki pass generates and the installer purges out of + source again ("purged 1 generated source page(s)"), so a generated + profile exists *only* there. + + Reading source alone therefore answered "no profile" for every + member who had one, which is the state this command shipped in. + Source is still checked first: a hand-curated page beats rebuildable + output. `_todos.py` consults both roots for the same reason. + """ + if override: + return [Path(override)] + data_dir = config.get("data_dir") if config else None + if not data_dir: + return [] + base = Path(data_dir) + return [vault_path_for(base), brain_path_for(base)] + + +def _clean_profile(text: str) -> str: + text = _FRONTMATTER.sub("", text) + text = _GEN_MARKER.sub("", text) + text = _CITE.sub("", text) + return text.strip() + + +def _resolve_person(vault: Path, query: str): + q = query.strip().lower() + for person in load_persons_from_vault(vault): + names = [person.slug, person.canonical, *person.synonyms] + if any(str(name).strip().lower() == q for name in names): + return person + return None + + +def _known_people(vault: Path) -> list[str]: + return [p.slug for p in load_persons_from_vault(vault)] + + +def run(args, stacklet, config): + try: + ns = _parser().parse_args(args or []) + except SystemExit as e: + return {"error": "usage: stack memory person "} if e.code else {"ok": True} + + name = " ".join(ns.name).strip() + if not name: + return {"error": "usage: stack memory person "} + + roots = [r for r in _roots(config, ns.vault) if r.exists()] + if not roots: + return {"error": "no vault found - is the memory stacklet installed?"} + if not ns.no_refresh: + refresh_vault_if_stale(roots[0]) + + fallback = re.sub(r"[^a-z0-9]+", "-", name.lower()).strip("-") + path = None + for root in roots: + person = _resolve_person(root, name) + candidate = root / (person.slug if person else fallback) / "about.md" + if candidate.exists(): + path = candidate + break + if path is None: + # Names from every root, so the hint lists everyone the household + # could have asked for, not just those in the first tree checked. + known = sorted({slug for root in roots for slug in _known_people(root)}) + hint = (" people: " + ", ".join(known)) if known else "" + return {"error": f"no profile for person {name!r}\n{hint}".rstrip()} + + text = _clean_profile(path.read_text(encoding="utf-8")) + rel = f"{path.parent.name}/about.md" + print(f"Source: vault/{rel}\n") + print(text) + return {"ok": True, "person": path.parent.name, "path": rel} diff --git a/stacklets/memory/lib.py b/stacklets/memory/lib.py index ca3295ee..b55ab183 100644 --- a/stacklets/memory/lib.py +++ b/stacklets/memory/lib.py @@ -462,19 +462,13 @@ def load_correspondents_from_vault( if not folder.exists(): return [] - # Lazy import: keeps the CLI install path stdlib-only. Callers of - # this function (archivist, `stack memory correspondents`) bring - # `python-frontmatter` on their PYTHONPATH. - import frontmatter - result: List[Correspondent] = [] for md_path in sorted(folder.glob("*.md")): try: - with open(md_path, "r", encoding="utf-8") as f: - post = frontmatter.load(f) - except (OSError, ValueError): + text = md_path.read_text(encoding="utf-8") + except OSError: continue - meta = post.metadata or {} + meta = _parse_frontmatter(text) if not meta: # No frontmatter at all — likely a README or stray note, skip. continue @@ -485,8 +479,12 @@ def load_correspondents_from_vault( continue result.append(Correspondent( canonical=str(canonical), - aliases=[str(a) for a in (meta.get("aliases") or [])], - topics=[str(t) for t in (meta.get("topics") or [])], + # _fm_list, not a comprehension: a bare string iterates into + # one entry per character, so a hand edit that writes a + # single value where a list belongs turns "insurance" into + # eleven topics instead of one. + aliases=_fm_list(meta, "aliases"), + topics=_fm_list(meta, "topics"), address=meta.get("address"), phone=meta.get("phone"), email=meta.get("email"), @@ -602,10 +600,6 @@ def load_persons_from_vault( if not vault_path.exists(): return [] - # Lazy import: keeps the CLI install path stdlib-only (see the - # module-level note above `load_correspondents_from_vault`). - import frontmatter - skip = _NON_MEMBER_DIRS | {shared_bucket} result: List[Person] = [] for about in sorted(vault_path.glob("*/about.md")): @@ -613,11 +607,10 @@ def load_persons_from_vault( if slug in skip or slug.startswith("."): continue try: - with open(about, "r", encoding="utf-8") as f: - post = frontmatter.load(f) - except (OSError, ValueError): + text = about.read_text(encoding="utf-8") + except OSError: continue - meta = post.metadata or {} + meta = _parse_frontmatter(text) if meta.get("kind") and meta.get("kind") != "person": continue canonical = meta.get("canonical") or meta.get("title") or slug @@ -626,7 +619,7 @@ def load_persons_from_vault( result.append(Person( canonical=str(canonical), slug=str(meta.get("slug") or slug), - synonyms=[str(s) for s in (meta.get("synonyms") or [])], + synonyms=_fm_list(meta, "synonyms"), source_path=about, )) return result diff --git a/stacklets/memory/seeds/_shared/correspondents/README.md b/stacklets/memory/seeds/_shared/correspondents/README.md index b80b069e..87b1e536 100644 --- a/stacklets/memory/seeds/_shared/correspondents/README.md +++ b/stacklets/memory/seeds/_shared/correspondents/README.md @@ -28,7 +28,9 @@ canonical: Duff Insurance aliases: - "Duff Insurance Ortsverband Springfield" - "Duff Insurance Versicherung AG" -topics: [insurance, vehicle] +topics: + - insurance + - vehicle address: "Hansastraße 19, 80686 München" website: "https://www.duff-insurance.example" --- diff --git a/stacklets/messages/cli/_matrix.py b/stacklets/messages/cli/_matrix.py index d19c9528..ec6291ed 100644 --- a/stacklets/messages/cli/_matrix.py +++ b/stacklets/messages/cli/_matrix.py @@ -44,8 +44,7 @@ def resolve_login(sender, secrets): if not password: return None, None, f"No password for '{sender}' in secrets ({key})" return sender, password, None - bot_pass = (secrets.get("core__STACKER_BOT_PASSWORD") - or secrets.get("messages__STACKER_BOT_PASSWORD", "")) + bot_pass = secrets.get("core__STACKER_BOT_PASSWORD", "") if not bot_pass: return None, None, "stacker-bot not set up. Run 'stack up core' first." return "stacker-bot", bot_pass, None @@ -386,12 +385,20 @@ def join_user(self, room_id, user_id): # ── Users ──────────────────────────────────────────────────────────── - def create_user(self, username, password, displayname=None, admin=False): + def create_user(self, username, password, displayname=None, admin=False, + reset_password=True): """Create a user via the Synapse admin API. Returns True if the user was created or already exists. The admin - API uses PUT and is idempotent — calling it on an existing user - updates their profile rather than failing. + API uses PUT as an upsert, so calling it on an existing user + updates them rather than failing. + + That upsert is sharper than it looks: sending `password` re-sets + the credential and logs the account's devices out. A caller that + only needs the account to *exist* should pass + `reset_password=False`, or every re-run of setup kicks a live + session off the server. Bots feel this hardest, since nothing is + watching to log them back in. """ full = self._full_user(username) body = { @@ -399,6 +406,11 @@ def create_user(self, username, password, displayname=None, admin=False): "displayname": displayname or username, "admin": admin, } + if not reset_password: + existing, _ = _get(self._url(f"/_synapse/admin/v2/users/{full}"), + token=self.token) + if existing == 200: + del body["password"] status, resp = _put( self._url(f"/_synapse/admin/v2/users/{full}"), body, token=self.token, ) diff --git a/stacklets/messages/cli/setup.py b/stacklets/messages/cli/setup.py index 41825757..5bda4166 100644 --- a/stacklets/messages/cli/setup.py +++ b/stacklets/messages/cli/setup.py @@ -205,16 +205,20 @@ def _setup(client, users, config, secrets=None): BOT_NAME = "stacker-bot" BOT_DISPLAY = "Stacker" BOT_SECRET_KEY = "STACKER_BOT_PASSWORD" - bot_pass = secrets.get(f"messages__{BOT_SECRET_KEY}") + bot_pass = secrets.get(f"core__{BOT_SECRET_KEY}") if not bot_pass: import secrets as sec_mod bot_pass = sec_mod.token_urlsafe(16) # Persist so the password survives re-runs from stack.secrets import TomlSecretStore store = TomlSecretStore(Path(config.get("instance_dir", config.get("repo_root", "."))) / ".stack" / "secrets.toml") - store.set("messages", BOT_SECRET_KEY, bot_pass) + store.set("core", BOT_SECRET_KEY, bot_pass) - bot_created = client.create_user(BOT_NAME, bot_pass, displayname=BOT_DISPLAY) + # The bot-runner owns this account's session. Re-running setup must not + # reset its password, or the running bot is logged out with nothing to + # notice and log it back in. + bot_created = client.create_user(BOT_NAME, bot_pass, displayname=BOT_DISPLAY, + reset_password=False) if bot_created: results.append({"item": f"@{BOT_NAME}:{server_name}", "action": "ready"}) diff --git a/tests/README.md b/tests/README.md index f9b7332b..542ae74b 100644 --- a/tests/README.md +++ b/tests/README.md @@ -1,5 +1,29 @@ # Test Profiles +## What we are trying to write + +Module tests, first and foremost. A module test drives one coherent piece of +functionality from the outside, the way a client would, and its purpose is to +state the intent and pin the expected behaviour at the time of writing. That is +what survives refactors; the implementation underneath is disposable. Write them +to read like API documentation: name the behaviour, say why the case matters. + +Two failure modes to avoid, both of which cost tokens and buy nothing: + +- **Tests that mirror the implementation.** Written next to the code they cover, + they prove the two agree, not that either is right. Assert against something + external instead: a spec, a real service's response, an invariant we promise. +- **Stubs standing in for things we can run.** A green run against a stub proves + the wiring, not the behaviour. `stacktests ai local` gives real model answers + at no cost per call; reach for `mock` when you need determinism, not when you + need a pass. + +The demo-rig and e2e lanes sit on top: they prove the wiring holds between real +containers. They do not replace module tests, and module tests do not replace +them. + +## Choosing a lane + Use the shortest command that proves the behavior you changed. The profiles below are ordered from cheapest to most disruptive. diff --git a/tests/framework/test_ai_mode.py b/tests/framework/test_ai_mode.py new file mode 100644 index 00000000..3ae66d13 --- /dev/null +++ b/tests/framework/test_ai_mode.py @@ -0,0 +1,90 @@ +"""AI-mode resolution and the scoped [ai] rewrite. + +Pure logic, so it belongs in the offline lane. `apply()` is deliberately +untested here - it writes the live stack.toml, and a unit test that +mutates the running instance is a unit test that breaks the rig. +""" + +from __future__ import annotations + +import pytest + +from tests.integration._ai_mode import ( + MOCK_MODEL, + MOCK_URL, + AIModeError, + _rewrite, + settings_for, +) + +SAMPLE = """\ +[core] +name = "stack" +default = "not-the-ai-one" + +[ai] +# a comment that must survive +openai_url = "http://localhost:42199/v1" +openai_key = "test" +default = "test-model" +language = "en" + +[messages] +server_name = "simpson" +default = "also-not-the-ai-one" +""" + + +def test_mock_needs_no_environment(monkeypatch): + # The fallback must work in a bare checkout with nothing exported. + for var in ("FAMSTACK_AI_URL", "FAMSTACK_AI_MODEL", "FAMSTACK_AI_KEY"): + monkeypatch.delenv(var, raising=False) + assert settings_for("mock") == { + "openai_url": MOCK_URL, + "openai_key": "test", + "default": MOCK_MODEL, + } + + +def test_local_reports_what_is_missing(monkeypatch): + monkeypatch.delenv("FAMSTACK_AI_URL", raising=False) + with pytest.raises(AIModeError, match="FAMSTACK_AI_URL"): + settings_for("local") + + +def test_local_reads_the_environment(monkeypatch): + monkeypatch.setenv("FAMSTACK_AI_URL", "http://elsewhere:9/v1") + monkeypatch.setenv("FAMSTACK_AI_MODEL", "some-model") + monkeypatch.delenv("FAMSTACK_AI_KEY", raising=False) + assert settings_for("local") == { + "openai_url": "http://elsewhere:9/v1", + "openai_key": "", # self-hosted endpoints commonly ignore the key + "default": "some-model", + } + + +def test_unknown_mode_lists_the_valid_ones(): + with pytest.raises(AIModeError, match="mock, local, external"): + settings_for("locale") + + +def test_rewrite_only_touches_the_ai_table(): + # `default` appears in [core] and [messages] too. A sloppy regex would + # rewrite the first match in the file and silently corrupt another + # stacklet's config while appearing to work. + out = _rewrite(SAMPLE, "default", "new-model") + assert 'default = "new-model"' in out + assert 'default = "not-the-ai-one"' in out + assert 'default = "also-not-the-ai-one"' in out + assert out.count("new-model") == 1 + + +def test_rewrite_preserves_comments(): + out = _rewrite(SAMPLE, "openai_url", "http://x/v1") + assert "# a comment that must survive" in out + assert 'openai_url = "http://x/v1"' in out + + +def test_rewrite_reports_a_missing_key(): + with pytest.raises(AIModeError, match="openai_org"): + _rewrite(SAMPLE, "openai_org", "whatever") diff --git a/tests/framework/test_check_versions.py b/tests/framework/test_check_versions.py new file mode 100644 index 00000000..6a316821 --- /dev/null +++ b/tests/framework/test_check_versions.py @@ -0,0 +1,55 @@ +"""The release gate's version comparison, proved against the spellings we use. + +Written after the gate failed on its own first run: it compared raw strings and +called `0.3.0-beta.2` (tag and CLI banner) different from `0.3.0b2` (pyproject, +PEP 440 canonical). Both name the same release. Unfixed, the gate would have +gone red on every beta tag - a check that cries wolf gets switched off, which +is worse than no check. +""" + +from __future__ import annotations + +import pytest + +from tests.integration._check_versions import _normalise + + +@pytest.mark.parametrize( + "spelling", + [ + "0.3.0-beta.2", # git tag / CLI banner form + "0.3.0b2", # PEP 440 canonical, as pyproject stores it + "0.3.0.beta.2", + "0.3.0-b2", + "0.3.0BETA2", + ], +) +def test_beta_spellings_all_agree(spelling): + assert _normalise(spelling) == "0.3.0b2" + + +def test_release_versions_are_untouched(): + assert _normalise("0.3.0") == "0.3.0" + assert _normalise("1.0.0") == "1.0.0" + + +def test_distinct_versions_stay_distinct(): + # The gate's whole job. Normalising must not collapse real differences. + assert _normalise("0.3.0-beta.2") != _normalise("0.3.0-beta.3") + assert _normalise("0.3.0-beta.2") != _normalise("0.3.0") + assert _normalise("0.3.0-alpha.2") != _normalise("0.3.0-beta.2") + assert _normalise("0.3.0-rc.2") != _normalise("0.3.0-beta.2") + + +def test_prerelease_markers_map_to_canonical_letters(): + assert _normalise("1.2.0-alpha.1") == "1.2.0a1" + assert _normalise("1.2.0-beta.1") == "1.2.0b1" + assert _normalise("1.2.0-rc.1") == "1.2.0rc1" + # PEP 440 folds these spellings into rc. + assert _normalise("1.2.0-c.1") == "1.2.0rc1" + assert _normalise("1.2.0-preview.1") == "1.2.0rc1" + + +def test_bare_marker_implies_zero(): + # `1.2.0b` and `1.2.0b0` are the same release under PEP 440. + assert _normalise("1.2.0b") == "1.2.0b0" diff --git a/tests/framework/test_compose_pins.py b/tests/framework/test_compose_pins.py new file mode 100644 index 00000000..06704625 --- /dev/null +++ b/tests/framework/test_compose_pins.py @@ -0,0 +1,96 @@ +"""Every container image must name an explicit version. + +`:latest` plus watchtower is a scheduled outage. Paperless-ngx rolled from +the 2.x line to 3.0.2 unattended and broke document filing across the whole +e2e suite; we found out from a red test run, not from a decision. + +This is the audit that would have caught it, as a test instead of a one-time +grep — it costs milliseconds and cannot silently stop being true. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent + +# `image: repo/name:tag` in compose, ignoring commented-out lines. The tag is +# optional in the grammar precisely because omitting it means `:latest`, which +# is the case this test exists to reject. +_IMAGE_RE = re.compile(r"^\s*image:\s*[\"']?([^\s\"'#]+)", re.MULTILINE) + +# Known-unpinned images, as of 2026-07-31. This is a ratchet, not an allowlist: +# the test blocks any NEW floating tag immediately, while these existing ones +# get pinned deliberately, one at a time, each verified against a running rig. +# Picking a Synapse or Element version is a real decision with a real blast +# radius; making that call to turn a test green would be the wrong order. +# +# Delete entries as they are pinned. An empty set is the goal state. +KNOWN_UNPINNED: set[str] = { + "ghcr.io/matatonic/openedai-speech-min", # no tag at all + "nickfedor/watchtower:latest", + "apache/tika:latest", + "adguard/adguardhome:latest", + "matrixdotorg/synapse:latest", + "vectorim/element-web:latest", +} + + +def _compose_files() -> list[Path]: + return sorted(REPO_ROOT.glob("stacklets/*/docker-compose*.yml")) + + +def _unpinned(text: str) -> list[str]: + """Return image refs carrying no tag, or an explicitly floating one.""" + found = [] + for ref in _IMAGE_RE.findall(text): + # Environment-substituted refs (${FOO}) are resolved at render time; + # the pin lives wherever that variable is defined, not here. + if ref.startswith("${"): + continue + # A digest pin (repo@sha256:...) is stricter than a tag. Accept it. + if "@sha256:" in ref: + continue + # Strip the registry host before looking for the tag separator, so + # `registry:5000/img` is not mistaken for an `img:5000` tag. + last_segment = ref.rsplit("/", 1)[-1] + tag = last_segment.split(":", 1)[1] if ":" in last_segment else "latest" + if tag == "latest" and ref not in KNOWN_UNPINNED: + found.append(ref) + return found + + +def test_compose_files_exist(): + # Guard the guard: a glob that silently matches nothing would make every + # assertion below vacuously true. + assert _compose_files(), "no stacklet compose files found - glob is wrong" + + +def test_ratchet_has_no_stale_entries(): + # If an image gets pinned but its entry stays behind, the exemption sits + # there covering a name nothing uses - and quietly re-exempts that image + # the day someone reintroduces it. Shrinking the list must be mandatory. + all_refs = { + ref + for path in _compose_files() + for ref in _IMAGE_RE.findall(path.read_text(encoding="utf-8")) + } + stale = KNOWN_UNPINNED - all_refs + assert not stale, ( + "KNOWN_UNPINNED lists images no compose file uses - pinned already? " + f"remove them: {', '.join(sorted(stale))}" + ) + + +def test_no_floating_image_tags(): + offenders: dict[str, list[str]] = {} + for path in _compose_files(): + unpinned = _unpinned(path.read_text(encoding="utf-8")) + if unpinned: + offenders[str(path.relative_to(REPO_ROOT))] = unpinned + + assert not offenders, ( + "unpinned container images (an unpinned image is a scheduled outage):\n" + + "\n".join(f" {p}: {', '.join(refs)}" for p, refs in offenders.items()) + ) diff --git a/tests/framework/test_docker_runtime.py b/tests/framework/test_docker_runtime.py index a268e026..b9b4421c 100644 --- a/tests/framework/test_docker_runtime.py +++ b/tests/framework/test_docker_runtime.py @@ -130,3 +130,50 @@ def test_force_recreate_is_unconditional(self): cmd = run.call_args[0][0] assert "--force-recreate" in cmd assert cmd.index("up") < cmd.index("--force-recreate") + + +class TestComposeUpWithNoActiveServices: + """A stacklet whose every service is profile-gated off starts cleanly. + + When COMPOSE_PROFILES excludes all of a compose file's services, + `docker compose up` exits 1 with "no service selected" on stderr. + That is compose reporting an empty selection, not a failure to + start anything, but the CLI reads any non-zero as "Failed to start + services" and refuses to write the setup marker. + + The ai stacklet is the live example: STACK_AI_NO_VOICE=1 clears the + profile, and its only service (Piper TTS) sits behind `voice`. The + documented local-dev opt-out could therefore never finish setup, + which in turn blocked every stacklet that `requires = ["ai"]`. + + The exit code and message below were taken from a real + `docker compose up -d --force-recreate` run, not from reading the + source, so this pins compose's actual contract. + """ + + def _run(self, returncode, stderr): + from stack import docker + docker._context = None + + mock = MagicMock() + mock.returncode = returncode + mock.stderr = stderr + with patch("subprocess.run", return_value=mock): + return docker.compose_up("/tmp/compose.yml") + + def test_empty_selection_is_success(self): + assert self._run(1, "no service selected") == (0, "") + + def test_message_is_matched_regardless_of_padding(self): + """Compose has moved this text between streams and added + whitespace across versions; match on content, not layout.""" + assert self._run(1, " no service selected\n") == (0, "") + + def test_real_failures_still_propagate(self): + """The narrow allowance must not swallow a genuine error.""" + code, err = self._run(1, "network stack declared as external, but could not be found") + assert code == 1 + assert "could not be found" in err + + def test_success_is_untouched(self): + assert self._run(0, "") == (0, "") diff --git a/tests/framework/test_doctor.py b/tests/framework/test_doctor.py new file mode 100644 index 00000000..b1741758 --- /dev/null +++ b/tests/framework/test_doctor.py @@ -0,0 +1,259 @@ +"""Diagnosis rules, including the drift that took the dev instance's bots down. + +Each case here is a failure that actually happened or that the rule exists to +prevent. Pure functions, no instance required. +""" + +from __future__ import annotations + +from stack.doctor import ( + ERROR, + Finding, + check_endpoint, + check_env_drift, + check_exited, + compose_supplied, + diagnose, + env_drift, + summarise, +) + + +def _fixture_instance(): + """A core stacklet with one drifted container and one dead sidecar.""" + containers = { + "core": [ + {"name": "stack-core-bot-runner", "state": "running", + "exit_code": 0, "since": "Up 4 minutes"}, + {"name": "stack-core-watchtower", "state": "exited", + "exit_code": 128, "since": "3 weeks ago"}, + ], + } + envs = { + "stack-core-bot-runner": {"MATRIX_SERVER_NAME": "test.local"}, + "stack-core-watchtower": {}, + } + return ( + ["core"], + lambda s: {"MATRIX_SERVER_NAME": "simpson"}, + lambda s: containers.get(s, []), + lambda n: envs.get(n, {}), + lambda n: {}, + ) + + +# ── env_drift ──────────────────────────────────────────────────────────── + +def test_detects_the_realm_drift_that_broke_the_bots(): + # The real incident: stack.toml re-seeded to a new realm, container still + # carrying the old one, every bot login 403ing against a realm that no + # longer had accounts. + rendered = {"MATRIX_SERVER_NAME": "simpson", "MATRIX_ADMIN_USER": "stackadmin"} + actual = {"MATRIX_SERVER_NAME": "test.local", "MATRIX_ADMIN_USER": "stackadmin"} + assert env_drift(rendered, actual) == ["MATRIX_SERVER_NAME"] + + +def test_clean_container_reports_nothing(): + env = {"MATRIX_SERVER_NAME": "simpson", "PAPERLESS_URL": "http://x:8000"} + assert env_drift(env, dict(env)) == [] + + +def test_key_the_container_never_receives_is_not_drift(): + # Learned from the first live run: a stacklet's rendered env covers the + # whole compose project, but each service gets only the subset its + # compose entry maps. Flagging the rest made a sidecar that receives two + # variables report 36 problems, drowning the one that mattered. + rendered = {"MAPPED": "same", "NOT_MAPPED_TO_THIS_SERVICE": "x"} + assert env_drift(rendered, {"MAPPED": "same"}) == [] + + +def test_image_defined_keys_are_not_drift(): + # The image legitimately sets things stack.toml says nothing about. + assert env_drift({"A": "1"}, {"A": "1", "IMAGE_OWN_VAR": "x"}) == [] + + +def test_runtime_keys_are_ignored(): + # PATH and friends always differ; comparing them would bury real findings. + rendered = {"PATH": "/expected", "REAL": "yes"} + actual = {"PATH": "/actual/from/image", "REAL": "yes"} + assert env_drift(rendered, actual) == [] + + +def test_non_string_rendered_values_compare_by_string(): + # stack.toml yields ints and bools; container env is always strings. + assert env_drift({"PORT": 8000, "DEBUG": True}, {"PORT": "8000", "DEBUG": "True"}) == [] + assert env_drift({"PORT": 8000}, {"PORT": "9000"}) == ["PORT"] + + +def test_drift_never_leaks_values(): + # This environment holds admin passwords and API tokens. Findings get + # pasted into issues and logs, so only key names may appear. + rendered = {"ADMIN_PASSWORD": "hunter2", "API_TOKEN": "sk-secret"} + actual = {"ADMIN_PASSWORD": "old-one", "API_TOKEN": "sk-old"} + drifted = env_drift(rendered, actual) + finding = check_env_drift("core", "stack-core-bot-runner", drifted) + rendered_text = f"{finding.title} {finding.detail} {finding.fix}" + for secret in ("hunter2", "sk-secret", "old-one", "sk-old"): + assert secret not in rendered_text + assert "ADMIN_PASSWORD" in rendered_text # the name is the useful part + + +# ── compose_supplied ───────────────────────────────────────────────────── + +def test_image_baked_value_is_not_ours_to_fix(): + # The gotenberg case, with its real values. `gotenberg/gotenberg:8` bakes + # TZ=UTC; stack.toml says Europe/Berlin; the compose file never passes TZ + # to that service. Doctor reported drift on every run and told the reader + # to run `stack up docs`, which recreated the container and changed + # nothing, because there was nothing to change. + actual = {"TZ": "UTC", "PAPERLESS_URL": "http://localhost:42020"} + baked = {"TZ": "UTC"} + assert compose_supplied(actual, baked) == {"PAPERLESS_URL": "http://localhost:42020"} + + +def test_compose_overriding_an_image_default_is_still_ours(): + # Same key, different value: compose won, so we own it and it can drift. + assert compose_supplied({"TZ": "Europe/Berlin"}, {"TZ": "UTC"}) == {"TZ": "Europe/Berlin"} + + +def test_gotenberg_style_container_produces_no_finding(): + # End to end through the caller's entry point, which is where the bug was + # visible. This is the regression guard: it fails if the walk ever goes + # back to comparing a container's whole environment. + containers = [{"name": "stack-docs-gotenberg", "state": "running", + "exit_code": 0, "since": "Up 4 minutes"}] + findings = diagnose( + ["docs"], + lambda s: {"TZ": "Europe/Berlin"}, # what stack.toml renders + lambda s: containers, + lambda n: {"TZ": "UTC"}, # container, from the image + lambda n: {"TZ": "UTC"}, # image's own default + ) + assert findings == [] + + +def test_real_drift_still_reported_when_the_image_is_silent(): + # The guard must not swallow the incident it was built for: the image + # says nothing about the realm, so a stale value is genuinely ours. + containers = [{"name": "stack-core-bot-runner", "state": "running", + "exit_code": 0, "since": "Up 4 minutes"}] + findings = diagnose( + ["core"], + lambda s: {"MATRIX_SERVER_NAME": "simpson"}, + lambda s: containers, + lambda n: {"MATRIX_SERVER_NAME": "test.local"}, + lambda n: {}, + ) + assert len(findings) == 1 + assert "superseded config" in findings[0].title + + +# ── findings ───────────────────────────────────────────────────────────── + +def test_env_drift_finding_is_actionable(): + finding = check_env_drift("core", "stack-core-bot-runner", ["MATRIX_SERVER_NAME"]) + assert finding.level == ERROR + assert finding.fix == "stack up core" + + +def test_no_drift_produces_no_finding(): + assert check_env_drift("core", "stack-core-bot-runner", []) is None + + +def test_clean_exit_is_not_a_finding(): + assert check_exited("stack-core-job", 0, "2 minutes ago") is None + + +def test_nonzero_exit_names_the_container_and_code(): + # The real case: watchtower Exited(128) three weeks ago, while status + # only said the stacklet was failing. + finding = check_exited("stack-core-watchtower", 128, "3 weeks ago") + assert finding.is_error + assert "stack-core-watchtower" in finding.title + assert "128" in finding.title + assert "3 weeks ago" in finding.detail + + +def test_reachable_endpoint_is_not_a_finding(): + assert check_endpoint("AI", "http://localhost:42199/v1", reachable=True) is None + + +def test_unset_endpoint_is_not_a_finding(): + # Nothing configured is a choice, not a fault. + assert check_endpoint("AI", "", reachable=False) is None + + +def test_unreachable_endpoint_names_the_url(): + finding = check_endpoint("AI", "http://localhost:42199/v1", reachable=False) + assert finding.level == "warn" + assert "http://localhost:42199/v1" in finding.detail + + +# ── diagnose (the whole walk) ──────────────────────────────────────────── + +def test_diagnose_reproduces_the_real_incident(): + # Both faults the dev instance actually had, found in one pass. + findings = diagnose(*_fixture_instance()) + titles = " | ".join(f.title for f in findings) + assert "stack-core-bot-runner is running superseded config" in titles + assert "stack-core-watchtower exited (128)" in titles + assert len(findings) == 2 + + +def test_diagnose_skips_env_check_for_stopped_containers(): + # A stopped container's environment is stale by definition; reporting + # drift on it would bury the finding that it is stopped at all. + containers = [{"name": "stack-x-dead", "state": "exited", + "exit_code": 1, "since": "1 hour ago"}] + findings = diagnose( + ["x"], lambda s: {"A": "new"}, lambda s: containers, lambda n: {"A": "old"}, + lambda n: {}, + ) + assert len(findings) == 1 + assert "exited" in findings[0].title + + +def test_diagnose_survives_a_stacklet_that_cannot_render(): + # One broken config must not stop the rest being diagnosed. + def exploding_env(stacklet): + raise ValueError("bad config") + + containers = [{"name": "stack-x-1", "state": "running", + "exit_code": 0, "since": "Up 1 minute"}] + assert diagnose(["x"], exploding_env, lambda s: containers, + lambda n: {}, lambda n: {}) == [] + + +def test_diagnose_ignores_stacklets_with_no_containers(): + assert diagnose(["absent"], lambda s: {"A": "1"}, lambda s: [], + lambda n: {}, lambda n: {}) == [] + + +def test_healthy_instance_yields_nothing(): + containers = [{"name": "stack-x-1", "state": "running", + "exit_code": 0, "since": "Up 1 minute"}] + findings = diagnose( + ["x"], lambda s: {"A": "1"}, lambda s: containers, lambda n: {"A": "1"}, + lambda n: {}, + ) + assert findings == [] + assert summarise(findings) == "No problems found." + + +# ── summary ────────────────────────────────────────────────────────────── + +def test_summary_when_healthy(): + assert summarise([]) == "No problems found." + + +def test_summary_counts_and_pluralises(): + findings = [ + Finding(ERROR, "a", "", ""), + Finding(ERROR, "b", "", ""), + Finding("warn", "c", "", ""), + ] + assert summarise(findings) == "2 errors, 1 warning." + + +def test_summary_singular_error(): + assert summarise([Finding(ERROR, "a", "", "")]) == "1 error." diff --git a/tests/integration/_ai_mode.py b/tests/integration/_ai_mode.py new file mode 100644 index 00000000..38387718 --- /dev/null +++ b/tests/integration/_ai_mode.py @@ -0,0 +1,176 @@ +"""Flip the rig's AI backend between mocked, local, and external. + +Three ways to answer a model call. `local` is the default you want. + + local A self-hosted OpenAI-compatible endpoint - a real model, + giving real answers, on hardware that costs nothing per call. + Slower than a stub and that is the whole price. It skips the + `ai` stacklet's install-and-load-weights wait (minutes and + gigabytes every time) while still exercising the real thing. + + mock pytest-httpserver, started by the `openai` conftest fixture. + Deterministic and offline. Correct only where the assertion + is about *exact* model output and needs ordered stubs. + + external A hosted provider. Bills per call. For checking behaviour + against a frontier model, not for looping. + +PREFER `local`. A green mock run proves the wiring, not the behaviour: +the stub returns whatever the test told it to, so classification, +extraction, and prompt changes all "pass" while being wrong. Mocking is +the cheap way to make a suite green and the expensive way to ship a bug. +Reach for `mock` when you need determinism, not when you need it to pass. + +Only `[ai]` in the instance's stack.toml changes. Everything downstream +(`ai_openai_url`, the container's host.docker.internal rewrite, the +`AI_API_KEY` secret override) already reads from there, so no other +surface needs to know a mode exists. + +WHY ENDPOINTS COME FROM THE ENVIRONMENT + A self-hosted URL is machine-specific and often a personal hostname. + This repo is public, so `local` and `external` read their endpoint, + key, and model from env vars rather than carrying anyone's + infrastructure in version control. `mock` needs none of that - its + endpoint is a fixture on localhost - so it is what a fresh checkout + seeds to. That makes it the fallback, not the goal: set the three + vars below once and work in `local`. + + FAMSTACK_AI_URL base URL, including /v1 + FAMSTACK_AI_MODEL model name to send as `default` + FAMSTACK_AI_KEY optional; many self-hosted endpoints ignore it +""" + +from __future__ import annotations + +import os +import re +import sys +import tomllib +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent +STACK_TOML = REPO_ROOT / "stack.toml" + +# Keep in step with _seed_secrets.TEST_MODEL: the archivist's vision-probe +# cache is pre-seeded under this name, and a mismatch re-fires the probe. +MOCK_URL = "http://localhost:42199/v1" +MOCK_KEY = "test" +MOCK_MODEL = "test-model" + +EXTERNAL_URL = "https://api.openai.com/v1" + +MODES = ("mock", "local", "external") + + +class AIModeError(RuntimeError): + """Configuration the caller has to fix, reported without a traceback.""" + + +def _env(name: str, mode: str) -> str: + value = os.environ.get(name, "").strip() + if not value: + raise AIModeError( + f"{mode} mode needs {name}.\n\n" + f" export {name}=...\n\n" + "Endpoints are read from the environment, not committed - see\n" + "tests/integration/_ai_mode.py for why and for the full list." + ) + return value + + +def settings_for(mode: str) -> dict[str, str]: + """Resolve a mode to the four `[ai]` values, or explain what is missing.""" + if mode == "mock": + return {"openai_url": MOCK_URL, "openai_key": MOCK_KEY, "default": MOCK_MODEL} + if mode == "local": + return { + "openai_url": _env("FAMSTACK_AI_URL", "local"), + "openai_key": os.environ.get("FAMSTACK_AI_KEY", "").strip(), + "default": _env("FAMSTACK_AI_MODEL", "local"), + } + if mode == "external": + return { + "openai_url": os.environ.get("FAMSTACK_AI_URL", "").strip() or EXTERNAL_URL, + "openai_key": _env("FAMSTACK_AI_KEY", "external"), + "default": _env("FAMSTACK_AI_MODEL", "external"), + } + raise AIModeError(f"Unknown mode {mode!r}. Pick one of: {', '.join(MODES)}.") + + +def current() -> tuple[str, dict]: + """Return (mode, ai_table) for the instance as it stands.""" + if not STACK_TOML.exists(): + raise AIModeError(f"{STACK_TOML} is missing - bring the instance up first.") + with STACK_TOML.open("rb") as fh: + ai = tomllib.load(fh).get("ai", {}) + url = ai.get("openai_url", "") + if url == MOCK_URL: + return "mock", ai + if url.startswith(EXTERNAL_URL.rsplit("/", 1)[0]): + return "external", ai + return ("local", ai) if url else ("unset", ai) + + +def _rewrite(text: str, key: str, value: str) -> str: + """Replace one `key = "..."` inside the [ai] table, comments intact. + + A tomllib round-trip would drop every comment in the file, and the + comments here are load-bearing (they explain the fixture wiring). So + edit the one line, scoped to the [ai] table so a same-named key in + another table is untouched. + """ + pattern = re.compile( + r"(^\[ai\]\n(?:(?!^\[).*\n)*?^" + re.escape(key) + r"\s*=\s*)(\".*?\")", + re.MULTILINE, + ) + replacement = rf'\g<1>"{value}"' + new_text, count = pattern.subn(replacement, text, count=1) + if count == 0: + raise AIModeError(f"No `{key}` found in the [ai] table of {STACK_TOML}.") + return new_text + + +def apply(mode: str) -> dict[str, str]: + """Write the mode into stack.toml. Returns the values applied.""" + settings = settings_for(mode) # resolve first: never half-write a mode + if not STACK_TOML.exists(): + raise AIModeError(f"{STACK_TOML} is missing - bring the instance up first.") + text = STACK_TOML.read_text(encoding="utf-8") + for key, value in settings.items(): + text = _rewrite(text, key, value) + STACK_TOML.write_text(text, encoding="utf-8") + return settings + + +def _redact(key: str, value: str) -> str: + if key == "openai_key" and value and value != MOCK_KEY: + return "" + return value or "" + + +def main(argv: list[str]) -> int: + if not argv: + mode, ai = current() + print(f" ai mode: {mode}") + for key in ("openai_url", "default", "openai_key"): + print(f" {key:<12} {_redact(key, ai.get(key, ''))}") + print(f"\n switch with: stacktests ai [{'|'.join(MODES)}]") + return 0 + + mode = argv[0] + settings = apply(mode) + print(f" ai mode -> {mode}") + for key, value in settings.items(): + print(f" {key:<12} {_redact(key, value)}") + if mode != "mock": + print("\n Restart affected stacklets so containers pick this up:") + print(" tests/integration/stacktests up docs") + return 0 + + +if __name__ == "__main__": + try: + raise SystemExit(main(sys.argv[1:])) + except AIModeError as exc: + print(f"\n {exc}\n", file=sys.stderr) + raise SystemExit(2) from None diff --git a/tests/integration/_check_versions.py b/tests/integration/_check_versions.py new file mode 100644 index 00000000..e5169600 --- /dev/null +++ b/tests/integration/_check_versions.py @@ -0,0 +1,89 @@ +"""Assert the version string agrees across every place that declares it. + +v0.3.0-beta.1 shipped with a stale `uv.lock` and a `VERSION` that disagreed +with the tag, because the pre-tag checklist was manual and a human skipped a +line. This is that line, made mechanical. + +Two sources must agree: `lib/stack/cli.py`'s VERSION constant and +`pyproject.toml`'s `project.version`. When run inside a tag build, +`GITHUB_REF_NAME` is a third — the tag itself, minus its leading `v`. + +Exits non-zero with the mismatch spelled out. Import-free beyond stdlib so it +runs anywhere `stacktests preflight` runs. +""" + +from __future__ import annotations + +import os +import re +import sys +import tomllib +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent +_VERSION_RE = re.compile(r"^VERSION\s*=\s*[\"']([^\"']+)[\"']", re.MULTILINE) + +# These spellings are equal, and the repo uses both on purpose: tags and the +# CLI banner read `0.3.0-beta.2`, while pyproject stores PEP 440's canonical +# `0.3.0b2`. Comparing raw strings would fail every beta release, so normalise +# to the canonical form before comparing. +_PRE_RE = re.compile( + r"[-_.]?(?:(?Palpha|a)|(?Pbeta|b)|(?Prc|c|pre|preview))[-_.]?(?P\d*)$", + re.IGNORECASE, +) + + +def _normalise(version: str) -> str: + """Reduce a version to its PEP 440 canonical form. + + Deliberately covers only the pre-release suffix - the one place this repo + actually spells things two ways. Anything more would be reimplementing + `packaging`, which this script cannot import: it runs on the host + interpreter, outside the uv-managed test environment. + """ + version = version.strip().lower() + match = _PRE_RE.search(version) + if not match: + return version + marker = "a" if match.group("a") else "b" if match.group("b") else "rc" + number = match.group("n") or "0" + return f"{version[: match.start()]}{marker}{int(number)}" + + +def _cli_version() -> str: + text = (REPO_ROOT / "lib" / "stack" / "cli.py").read_text(encoding="utf-8") + match = _VERSION_RE.search(text) + if not match: + raise SystemExit("no VERSION = '...' assignment found in lib/stack/cli.py") + return match.group(1) + + +def _pyproject_version() -> str: + with (REPO_ROOT / "pyproject.toml").open("rb") as fh: + return tomllib.load(fh)["project"]["version"] + + +def main() -> int: + sources = { + "lib/stack/cli.py": _cli_version(), + "pyproject.toml": _pyproject_version(), + } + + # Only present in a tag build; locally there is no tag to agree with. + ref = os.environ.get("GITHUB_REF_NAME", "") + if ref.startswith("v"): + sources["git tag"] = ref[1:] + + distinct = {_normalise(v) for v in sources.values()} + if len(distinct) > 1: + print("version mismatch:", file=sys.stderr) + for origin, value in sources.items(): + print(f" {origin:<20} {value} (normalised: {_normalise(value)})", file=sys.stderr) + return 1 + + print(f" {distinct.pop()} agreed by {', '.join(sources)}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 944a015b..d45ff234 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -267,10 +267,13 @@ def matrix(test_stack) -> dict: # The messages stacklet's setup CLI creates family accounts using # the seeded USER__PASSWORD values. Log them in once to capture # access tokens for use by tests. + # Read the realm only now: seeding above is what puts stack.toml in + # place, so this cannot be a fixture parameter. + realm = _server_name() creds = {} for username in ("homer", "marge", "bart", "lisa"): creds[username] = login( - server_name="test.local", + server_name=realm, username=username, password=username, # seeded in _seed_test_instance_secrets ) @@ -311,16 +314,35 @@ def _instance_config() -> dict: return tomllib.load(fh) -@pytest.fixture(scope="session") -def demo_server_name() -> str: +def _server_name() -> str: + """The instance's Matrix realm, read from the live stack.toml. + + One base setup serves both lanes — the managed rig and the demo rig are + the same Simpsons instance, so neither may hardcode this. Synapse bakes + server_name into every user ID permanently at first start, so a literal + that drifts from the running homeserver produces 403s on every login + with no hint as to why. That exact drift took the dev instance's bots + down: a container carrying a stale realm, logging in against accounts + that only existed in another one. + + A plain function, not a fixture, because the managed rig seeds + stack.toml on the way up — callers must read it after seeding, and a + fixture parameter would resolve too early. + """ name = _instance_config().get("messages", {}).get("server_name") if not name: - pytest.fail("No [messages].server_name in stack.toml for demo rig login.") + pytest.fail("No [messages].server_name in stack.toml — cannot log in.") return name @pytest.fixture(scope="session") -def demo_matrix(demo_server_name) -> dict: +def server_name() -> str: + """Session-wide Matrix realm for tests that only read it.""" + return _server_name() + + +@pytest.fixture(scope="session") +def demo_matrix(server_name) -> dict: """Log in to the running Simpson demo instance without seeding or up.""" from stack.secrets import TomlSecretStore @@ -334,7 +356,7 @@ def demo_matrix(demo_server_name) -> dict: "for demo rig login." ) creds[username] = login( - server_name=demo_server_name, + server_name=server_name, username=username, password=password, ) diff --git a/tests/integration/instance/stack.toml b/tests/integration/instance/stack.toml index 0b7a40e4..08a6fb6e 100644 --- a/tests/integration/instance/stack.toml +++ b/tests/integration/instance/stack.toml @@ -20,13 +20,15 @@ shared_bucket = "family" schedule = "0 0 3 * * *" [ai] -# pytest-httpserver instance — started by the `openai` conftest fixture. +# AI backend. Flip with `tests/integration/stacktests ai [mock|local|external]`. +# mock points at the pytest-httpserver stub the `openai` fixture starts; +# local/external endpoints come from the environment, never committed. openai_url = "http://localhost:42199/v1" openai_key = "test" default = "test-model" language = "en" [messages] -# Server name is permanent in every Matrix user ID (@homer:test.local). +# Server name is permanent in every Matrix user ID (@homer:simpson). # Pre-set here so `stack up messages` skips the interactive prompt. -server_name = "test.local" +server_name = "simpson" diff --git a/tests/integration/matrix.py b/tests/integration/matrix.py index ffee5922..779f521b 100644 --- a/tests/integration/matrix.py +++ b/tests/integration/matrix.py @@ -10,14 +10,51 @@ import io import json +import sys import time +import tomllib import urllib.error import urllib.request from dataclasses import dataclass +from pathlib import Path SYNAPSE_URL = "http://localhost:42031" +_REPO_ROOT = Path(__file__).resolve().parent.parent.parent + + +def realm() -> str: + """The instance's Matrix server name, read from the live stack.toml. + + Both lanes run against one Simpsons instance, so no test may hardcode + this. Synapse bakes server_name into every user ID permanently at first + start; a literal that drifts from the running homeserver turns every + login into a 403 that names neither the realm nor the cause. That is + precisely how the dev instance lost all four of its bots. + """ + stack_toml = _REPO_ROOT / "stack.toml" + if not stack_toml.exists(): + raise RuntimeError( + f"{stack_toml} is missing — bring the instance up first " + "(tests/integration/stacktests up), which seeds it." + ) + with stack_toml.open("rb") as fh: + name = tomllib.load(fh).get("messages", {}).get("server_name") + if not name: + raise RuntimeError(f"No [messages].server_name in {stack_toml}.") + return name + + +def mxid(localpart: str) -> str: + """`@homer` -> `@homer:`.""" + return f"@{localpart}:{realm()}" + + +def room_alias(name: str) -> str: + """`documents` -> `#documents:`.""" + return f"#{name}:{realm()}" + @dataclass class MatrixCreds: @@ -85,6 +122,48 @@ class MatrixLoginError(RuntimeError): pass +def token_alive(access_token: str, homeserver: str = SYNAPSE_URL) -> bool: + """Whether Synapse still honours this access token. + + `/whoami` is the cheapest way to ask. Synapse invalidates an account's + devices on a password change, so this is how a test observes a session + being ended out from under it. + """ + req = urllib.request.Request( + f"{homeserver}/_matrix/client/v3/account/whoami", + headers={"Authorization": f"Bearer {access_token}"}, + method="GET", + ) + try: + with urllib.request.urlopen(req, timeout=15) as resp: + return resp.status == 200 + except urllib.error.HTTPError: + return False + + +def deactivate_user(admin_token: str, mxid: str, + homeserver: str = SYNAPSE_URL) -> None: + """Deactivate and erase an account. Best effort, for test cleanup. + + Synapse never truly deletes users, so a test that creates one has to + at least leave it deactivated rather than leaking a live account into + the demo instance. + """ + req = urllib.request.Request( + f"{homeserver}/_synapse/admin/v1/deactivate/{mxid}", + data=json.dumps({"erase": True}).encode(), + headers={ + "Content-Type": "application/json", + "Authorization": f"Bearer {admin_token}", + }, + method="POST", + ) + try: + urllib.request.urlopen(req, timeout=15) + except urllib.error.HTTPError as e: + print(f"[cleanup] could not deactivate {mxid}: {e.code}", file=sys.stderr) + + # ── Room + file helpers on top of nio AsyncClient ──────────────────────── # # Tests feed files into the archivist the way a real family member would: diff --git a/tests/integration/stacktests b/tests/integration/stacktests index a24eb2c2..113a2ff0 100755 --- a/tests/integration/stacktests +++ b/tests/integration/stacktests @@ -8,6 +8,12 @@ # overwriting it. # # Usage (callable from anywhere — paths resolve off the script location): +# tests/integration/stacktests help — this text, plus autonomy +# tests/integration/stacktests preflight — offline gate: versions, +# lock, ruff, unit tests +# tests/integration/stacktests unverified — list parked intent-specs +# tests/integration/stacktests ai [mode] — show/flip the AI backend: +# mock | local | external # tests/integration/stacktests up — bring up what tests need # tests/integration/stacktests down — stop every test stacklet # tests/integration/stacktests reset [slet] — destroy + up (clean slate) @@ -33,6 +39,26 @@ unset STACK_DIR export PYTHONPATH="$REPO_ROOT/lib" cd "$REPO_ROOT" +# Interpreter: same rule as the ./stack wrapper — Apple's Command Line +# Tools ship python3 as 3.9, which lacks tomllib and often outranks +# Homebrew on PATH. Both `-m stack` and _seed_secrets.py import tomllib, +# so resolve once here rather than inheriting whatever `python3` means. +PYTHON= +for cand in python3.13 python3.12 python3.11 python3; do + if command -v "$cand" >/dev/null 2>&1; then + ver=$("$cand" -c 'import sys; print(sys.version_info[0]*100 + sys.version_info[1])' 2>/dev/null || echo 0) + if [ "$ver" -ge 311 ]; then PYTHON="$cand"; break; fi + fi +done +if [[ -z "$PYTHON" ]]; then + echo "stacktests: needs Python 3.11+. Run ./stack for the install hint." >&2 + exit 1 +fi + +# Every rig operation goes through the real CLI, which is the sanctioned +# agent interface — so what a test drives is what an operator types. +stack_cli() { "$PYTHON" -m stack "$@"; } + # Stacklets the e2e tests need, in dependency order. # - core: always_on, provides the bot-runner container that actually # executes the archivist bot (which then auto-creates #documents). @@ -54,7 +80,7 @@ PRESERVED_STACKLETS=(ai) # see _seed_secrets.py's TestInstanceConflict message for the cleanup # hint the user gets. seed_secrets() { - python3 "$SCRIPT_DIR/_seed_secrets.py" + "$PYTHON" "$SCRIPT_DIR/_seed_secrets.py" } case "${1:-}" in @@ -65,11 +91,11 @@ case "${1:-}" in if [[ -z "${2:-}" ]]; then for sid in "${REQUIRED_STACKLETS[@]}"; do echo "==> stack up $sid" - python3 -m stack up "$sid" + stack_cli up "$sid" done else shift - exec python3 -m stack up "$@" + exec stack_cli up "$@" fi ;; @@ -82,10 +108,10 @@ case "${1:-}" in if [[ -z "${1:-}" ]]; then for sid in "${REQUIRED_STACKLETS[@]}"; do echo "==> stack down $sid" - python3 -m stack down "$sid" || true + stack_cli down "$sid" || true done else - exec python3 -m stack down "$@" + exec stack_cli down "$@" fi ;; @@ -101,11 +127,11 @@ case "${1:-}" in fi for sid in "${targets[@]}"; do echo "==> stack destroy $sid --yes" - python3 -m stack destroy "$sid" --yes || true + stack_cli destroy "$sid" --yes || true done for sid in "${targets[@]}"; do echo "==> stack up $sid" - python3 -m stack up "$sid" + stack_cli up "$sid" done ;; @@ -130,7 +156,7 @@ case "${1:-}" in shift || true seed_secrets for sid in "${REQUIRED_STACKLETS[@]}"; do - python3 -m stack up "$sid" > /dev/null + stack_cli up "$sid" > /dev/null done exec uv run --extra test pytest tests/integration/ -k "_e2e" -m "not demo_rig" --durations=20 "$@" ;; @@ -141,7 +167,7 @@ case "${1:-}" in shift || true seed_secrets for sid in "${REQUIRED_STACKLETS[@]}"; do - python3 -m stack up "$sid" > /dev/null + stack_cli up "$sid" > /dev/null done exec uv run --extra test pytest tests/integration/ -k "_e2e" -m smoke "$@" ;; @@ -186,7 +212,7 @@ case "${1:-}" in shift || true seed_secrets for sid in "${REQUIRED_STACKLETS[@]}"; do - python3 -m stack up "$sid" > /dev/null + stack_cli up "$sid" > /dev/null done exec uv run --extra test pytest -s -v tests/integration/eval/ "$@" ;; @@ -211,7 +237,7 @@ case "${1:-}" in for sid in "${REQUIRED_STACKLETS[@]}"; do echo "==> stack destroy $sid --yes" - python3 -m stack destroy "$sid" --yes || true + stack_cli destroy "$sid" --yes || true done # Belt and suspenders: force-remove any stack--* containers that @@ -265,12 +291,88 @@ case "${1:-}" in echo " (preserved across cleanup: ${PRESERVED_STACKLETS[*]})" ;; + preflight) + # The autonomous gate. Everything here is offline and side-effect + # free, so an agent may run it unattended, on any branch, at any + # time — and must, before claiming a change is done. + # + # Deliberately the same checks the tag-triggered CI gate runs, in + # the same order, so "green locally" and "green in CI" cannot drift. + shift || true + echo "==> version consistency" + "$PYTHON" "$SCRIPT_DIR/_check_versions.py" + echo "==> uv lock --check" + uv lock --check + echo "==> ruff" + uvx ruff check . + echo "==> unit tests" + # Delegate to `make test-unit` rather than re-spelling the pytest + # invocation. It carries an --ignore for the Docker lifecycle tests; + # a second copy of that argument list drifted from this one within a + # day, quietly pulling Docker into what is meant to be the offline gate. + make test-unit + echo "" + echo "✓ preflight green — safe to commit." + ;; + + ai) + # Flip the AI backend: mock (httpserver fixture, offline, deterministic), + # local (self-hosted endpoint - real quality without waiting on the ai + # stacklet to load weights), or external (hosted, costs money per call). + # No argument prints the current mode. See _ai_mode.py for the env vars + # local/external read, and why endpoints are not committed. + shift || true + exec "$PYTHON" "$SCRIPT_DIR/_ai_mode.py" "$@" + ;; + + unverified) + # List every parked intent-spec. These are xfail-marked tests that + # describe behaviour we believe works but have never run green on + # the rig. Without this command they decay silently: the marker is + # write-only, so nobody notices a spec parked for months. + # Collection-only, so it needs no rig and no Docker. + shift || true + echo "Parked intent-specs (marker: unverified)" + echo "Each one is a claim that nothing currently proves." + echo "" + uv run --extra test pytest tests/ -m unverified --collect-only -q "$@" || true + ;; + + help|--help|-h) + # An agent's first instinct is --help. Before this existed, that + # fell through to the forward-to-stack case and died on whatever + # `python3` happened to mean. A mystery interface is a bug. + # Read the header block itself rather than restating it — line 2 to + # the first blank line, so editing the usage above updates --help. + sed -n '2,/^$/p' "$0" | sed 's/^# \{0,1\}//' + cat <<'EOF' +Autonomy — what an agent may run unattended: + + autonomous Offline, no Docker, no shared state. Run freely. + preflight, unverified, help, ai (show/flip) + + `ai local` is self-hosted: free, unmetered, just slower + than a stub. Prefer it. `ai external` bills per call. + + shared Uses the one dev instance. Agents may run these — this + repo root is the Simpsons dev rig, not anyone's real + famstack. Ports are fixed, so exactly one run at a time: + check nothing else is mid-run before starting. + up, down, e2e, smoke, demo-rig, eval + + destructive Wipes state that re-running cannot recover. + reset, cleanup, down ai + +Start with `preflight`. It is the whole offline gate in one command. +EOF + ;; + "") - python3 -m stack + stack_cli ;; *) # Anything else — forward untouched to stack. - exec python3 -m stack "$@" + exec stack_cli "$@" ;; esac diff --git a/tests/integration/test_archivist_e2e.py b/tests/integration/test_archivist_e2e.py index af882811..8427f082 100644 --- a/tests/integration/test_archivist_e2e.py +++ b/tests/integration/test_archivist_e2e.py @@ -19,6 +19,8 @@ from nio.responses import JoinedMembersResponse, RoomInviteResponse from tests.integration.matrix import ( + mxid, + room_alias, ensure_joined, event_type, fetch_room_events, @@ -29,8 +31,8 @@ from tests.integration.openai_stub import stub_classify, stub_reformat -DOCS_ROOM_ALIAS = "#documents:test.local" -ARCHIVIST_MXID = "@archivist-bot:test.local" +DOCS_ROOM_ALIAS = room_alias("documents") +ARCHIVIST_MXID = mxid("archivist-bot") # ── Helpers for reply / DM / mention scenarios ──────────────────────────── diff --git a/tests/integration/test_capture_binary_e2e.py b/tests/integration/test_capture_binary_e2e.py index 68be16c8..6d2b5067 100644 --- a/tests/integration/test_capture_binary_e2e.py +++ b/tests/integration/test_capture_binary_e2e.py @@ -26,6 +26,8 @@ from __future__ import annotations +from tests.integration.matrix import mxid + import asyncio import io @@ -38,7 +40,7 @@ MEMORY_OWNER = "family" MEMORY_REPO = "memory" -ARCHIVIST_MXID = "@archivist-bot:test.local" +ARCHIVIST_MXID = mxid("archivist-bot") FAMSTACK_EVENT_KEY = "dev.famstack.event" diff --git a/tests/integration/test_capture_memory_e2e.py b/tests/integration/test_capture_memory_e2e.py index 7085e6a4..fa678cf2 100644 --- a/tests/integration/test_capture_memory_e2e.py +++ b/tests/integration/test_capture_memory_e2e.py @@ -17,6 +17,8 @@ from __future__ import annotations +from tests.integration.matrix import mxid + import asyncio import pytest @@ -33,7 +35,7 @@ # Matrix ID the bot-runner registers the archivist under in the test # instance (server name comes from `tests/integration/instance/stack.toml`). -ARCHIVIST_MXID = "@archivist-bot:test.local" +ARCHIVIST_MXID = mxid("archivist-bot") # _PASTE_MIN_CHARS = 100 inside the bot — gates `_looks_like_paste`. # Anything shorter is treated as chat and ignored in capture rooms. diff --git a/tests/integration/test_demo_rig_e2e.py b/tests/integration/test_demo_rig_e2e.py index ad34657b..a51b6b06 100644 --- a/tests/integration/test_demo_rig_e2e.py +++ b/tests/integration/test_demo_rig_e2e.py @@ -17,16 +17,22 @@ import asyncio import json import subprocess +import sys import pytest from nio import AsyncClient from nio.api import RoomVisibility from nio.responses import JoinedMembersResponse, RoomInviteResponse +from tests.integration.conftest import REPO_ROOT from tests.integration.forgejo import ForgejoError from tests.integration.matrix import ( + SYNAPSE_URL, + deactivate_user, ensure_joined, + login, matrix_call, + token_alive, upload_and_send_file, wait_for_room, ) @@ -158,17 +164,107 @@ def _run_stack(*args: str, timeout: int = 120) -> subprocess.CompletedProcess: ) +@pytest.mark.demo_rig +def test_demo_rig_doctor_is_clean_on_a_healthy_instance(bdd): + """`stack doctor` must find nothing wrong with a working instance. + + Doctor's rules are pure functions with injected I/O, which makes them + easy to unit test and easy to get confidently wrong: the fixture and the + rule get written together from one mental model, so both agree and the + real world is never consulted. That has already happened twice, once + reporting 36 phantom drifts on watchtower and once reporting a gotenberg + TZ error whose suggested fix could not possibly work. + + This is the check neither of those could fail. A container's environment + is full of values no famstack command controls, and only a real one has + them all. + """ + bdd.given("an instance whose stacklets are up and healthy") + bdd.when("stack doctor inspects every container") + result = _run_stack("doctor") + + bdd.then("it reports nothing to fix") + assert "No problems found." in result.stdout, ( + "doctor flagged something on a healthy instance. Either the instance " + f"really is broken, or a rule is producing false positives:\n{result.stdout}" + ) + assert result.returncode == 0, f"doctor exited {result.returncode}" + + +@pytest.mark.demo_rig +def test_demo_rig_admin_put_with_password_ends_the_account_session( + bdd, + server_name, + scope, +): + """Synapse logs an account out whenever the admin PUT carries a password. + + `MatrixClient.create_user` is an upsert, so callers reach for it to make + an account exist. The cost is invisible from our side: Synapse treats any + `password` in that body as a credential change and invalidates the + account's devices, even when the value is identical to the current one. + + A bot has nothing watching to log it back in, which is how the stack + ended up with a stacker-bot that could not authenticate (FAM-2). The + `reset_password=False` guard exists for exactly this. + + This lives in the rig lane on purpose. The unit tests for the guard + assert what we put on the wire, which can only ever confirm our own + reading of the admin API. Only Synapse can answer what it does with it. + """ + sys.path.insert(0, str(REPO_ROOT / "stacklets" / "messages" / "cli")) + from _matrix import MatrixClient # noqa: PLC0415 + + from stack.secrets import TomlSecretStore # noqa: PLC0415 + + store = TomlSecretStore(REPO_ROOT / ".stack" / "secrets.toml") + admin_password = store.get("_", "ADMIN_PASSWORD") or store.get("global", "ADMIN_PASSWORD") + if not admin_password: + pytest.fail("No ADMIN_PASSWORD in .stack/secrets.toml for the demo rig.") + + probe = scope.uid.replace("-", "") + probe_password = f"{scope.uid}-probe-secret" + + admin = MatrixClient(SYNAPSE_URL, server_name, str(REPO_ROOT)) + assert admin.login("stackadmin", admin_password), "admin login failed" + scope.on_cleanup.append( + lambda _uid: deactivate_user(admin.token, f"@{probe}:{server_name}") + ) + + bdd.given(f"a throwaway account @{probe}:{server_name} with a live session") + assert admin.create_user(probe, probe_password, displayname="FAM-2 probe") + creds = login(server_name, probe, probe_password) + assert token_alive(creds.access_token), "the probe's session should start valid" + + bdd.when("the account is re-upserted with reset_password=False") + assert admin.create_user(probe, probe_password, displayname="FAM-2 probe", + reset_password=False) + bdd.then("its session survives") + assert token_alive(creds.access_token), ( + "reset_password=False must not invalidate an existing account's devices" + ) + + bdd.when("the same account is re-upserted with the default, same password") + assert admin.create_user(probe, probe_password, displayname="FAM-2 probe") + bdd.then("Synapse has ended the session anyway") + assert not token_alive(creds.access_token), ( + "if this still passes, Synapse no longer logs devices out on a " + "password PUT and the reset_password guard is solving a problem " + "that no longer exists" + ) + + @pytest.mark.demo_rig async def test_demo_rig_private_capture_reaches_memory_with_live_ai( bdd, demo_code, demo_homer, - demo_server_name, + server_name, scope, ): """Homer pastes a note in an isolated room; archivist files it.""" token = f"demo-rig-capture-{scope.uid}" - bot_mxid = f"@archivist-bot:{demo_server_name}" + bot_mxid = f"@archivist-bot:{server_name}" try: bdd.given("Homer creates a private demo-rig notes room") @@ -224,12 +320,12 @@ async def test_demo_rig_documents_markdown_reaches_paperless_and_memory_with_liv demo_code, demo_homer, demo_paperless, - demo_server_name, + server_name, scope, ): """Homer uploads markdown to #documents; Paperless and memory see it.""" token = f"demo-rig-doc-{scope.uid}" - docs_alias = f"#documents:{demo_server_name}" + docs_alias = f"#documents:{server_name}" try: bdd.given("Homer joins the live #documents room") @@ -282,7 +378,7 @@ async def test_demo_rig_memory_todos_stay_mutable_and_capture_items_project( demo_code, demo_homer, demo_matrix, - demo_server_name, + server_name, scope, ): """Topic todos read/write from memory and capture action items project.""" @@ -293,7 +389,7 @@ async def test_demo_rig_memory_todos_stay_mutable_and_capture_items_project( token = f"demo-rig-capture-todo-{scope.uid}" path = f"{MEMORY_OWNER}/{topic}/todos.md" about_path = f"{MEMORY_OWNER}/{topic}/about.md" - bot_mxid = f"@archivist-bot:{demo_server_name}" + bot_mxid = f"@archivist-bot:{server_name}" marge_creds = demo_matrix["marge"] marge = AsyncClient(marge_creds.homeserver, marge_creds.user_id) marge.access_token = marge_creds.access_token diff --git a/tests/integration/test_git_mirror_e2e.py b/tests/integration/test_git_mirror_e2e.py index fd673b1d..9bb1a9ea 100644 --- a/tests/integration/test_git_mirror_e2e.py +++ b/tests/integration/test_git_mirror_e2e.py @@ -22,6 +22,7 @@ from tests.integration.forgejo import ForgejoError from tests.integration.matrix import ( + room_alias, ensure_joined, upload_and_send_file, wait_for_room, @@ -29,7 +30,7 @@ from tests.integration.openai_stub import stub_classify, stub_reformat -DOCS_ROOM_ALIAS = "#documents:test.local" +DOCS_ROOM_ALIAS = room_alias("documents") # Repo owner = the Forgejo org `mirror_org` in the archivist's bot.toml. # Default is "family"; stays in sync with `FORGEJO_DOCS_OWNER` in conftest. DOCS_OWNER = "family" diff --git a/tests/integration/test_markdown_mirror_e2e.py b/tests/integration/test_markdown_mirror_e2e.py index ced37209..48450597 100644 --- a/tests/integration/test_markdown_mirror_e2e.py +++ b/tests/integration/test_markdown_mirror_e2e.py @@ -24,6 +24,7 @@ import pytest from tests.integration.matrix import ( + room_alias, ensure_joined, upload_and_send_file, wait_for_room, @@ -31,7 +32,7 @@ from tests.integration.openai_stub import stub_classify -DOCS_ROOM_ALIAS = "#documents:test.local" +DOCS_ROOM_ALIAS = room_alias("documents") DOCS_OWNER = "family" DOCS_REPO = "memory" diff --git a/tests/integration/test_room_modes_e2e.py b/tests/integration/test_room_modes_e2e.py index 85894b0a..cd4ad036 100644 --- a/tests/integration/test_room_modes_e2e.py +++ b/tests/integration/test_room_modes_e2e.py @@ -44,14 +44,15 @@ from nio import AsyncClient from tests.integration.matrix import ( + mxid, event_type, fetch_room_events, wait_for_room_event, wait_for_room_events_until, ) -ARCHIVIST = "@archivist-bot:test.local" -MARGE = "@marge:test.local" +ARCHIVIST = mxid("archivist-bot") +MARGE = mxid("marge") EYES, CHECK = "👀", "✅" pytestmark = [pytest.mark.unverified] diff --git a/tests/integration/test_source_archive_e2e.py b/tests/integration/test_source_archive_e2e.py index 7f469f31..428c3c12 100644 --- a/tests/integration/test_source_archive_e2e.py +++ b/tests/integration/test_source_archive_e2e.py @@ -21,6 +21,8 @@ from __future__ import annotations +from tests.integration.matrix import mxid + import asyncio import io import urllib.request @@ -36,7 +38,7 @@ # here (not imported) so this test pins the wire shape the mail bot emits. SOURCE_KEY = "dev.famstack.source" ATTACHMENT_KEY = "dev.famstack.attachment" -ARCHIVIST_MXID = "@archivist-bot:test.local" +ARCHIVIST_MXID = mxid("archivist-bot") PAPERCLIP = "📎" diff --git a/tests/stacklets/conftest.py b/tests/stacklets/conftest.py index 4f326242..95542cde 100644 --- a/tests/stacklets/conftest.py +++ b/tests/stacklets/conftest.py @@ -11,6 +11,7 @@ import os import subprocess import sys +import types from pathlib import Path import pytest @@ -24,6 +25,82 @@ if str(_LIB_DIR) not in sys.path: sys.path.insert(0, str(_LIB_DIR)) +# Stacklet runtime modules (e.g. the agent's nanobot shims) live on the +# container's PYTHONPATH, not on any package path a test can reach. Register +# them here once, by convention, so a test importing one is a plain import +# instead of a per-file sys.path hack with a `noqa: E402` chaser. +for _runtime_dir in sorted(REPO_ROOT.glob("stacklets/*/runtime")): + if str(_runtime_dir) not in sys.path: + sys.path.insert(0, str(_runtime_dir)) + + +@pytest.fixture +def nanobot_stub(): + """Build a stub `nanobot` module tree covering the surface we patch. + + Lives here rather than in one test file because two suites need it: + the shim tests assert the patches attach, and the vault-tool tests + drive the tools those patches register. Deliberately hand-built + rather than mocked -- every name is a symbol `sitecustomize.py` + pins, so the stub doubles as a written record of what we depend on, + and a Mock would satisfy any attribute and prove nothing. + + Returns a callable so a test can build a fresh tree per case. + """ + + def _build() -> dict[str, types.ModuleType]: + def runtime_lines(state, msg, workspace, *, skip=False): + return ["stock line"] + + class ContextBuilder: + def build_messages(self, *args, **kwargs): + return [{"role": "user", "content": "hi"}] + + class Tool: + pass + + def tool_parameters(schema): + return lambda cls: cls + + def tool_parameters_schema(**kwargs): + return dict(kwargs) + + class _Schema: + def __init__(self, *args, **kwargs): + self.args, self.kwargs = args, kwargs + + class ToolLoader: + def discover(self): + return [] + + class GrepTool: + async def execute(self, *args, **kwargs): + return "stock grep" + + mods: dict[str, types.ModuleType] = {} + + def mod(name, **attrs): + m = types.ModuleType(name) + for k, v in attrs.items(): + setattr(m, k, v) + mods[name] = m + return m + + mod("nanobot") + mod("nanobot.agent") + mod("nanobot.agent.context", + runtime_lines=runtime_lines, ContextBuilder=ContextBuilder) + mod("nanobot.agent.tools") + mod("nanobot.agent.tools.base", Tool=Tool, tool_parameters=tool_parameters) + mod("nanobot.agent.tools.schema", + StringSchema=_Schema, IntegerSchema=_Schema, + tool_parameters_schema=tool_parameters_schema) + mod("nanobot.agent.tools.loader", ToolLoader=ToolLoader) + mod("nanobot.agent.tools.search", GrepTool=GrepTool) + return mods + + return _build + @pytest.fixture def stack_cli(): diff --git a/tests/stacklets/test_agent_grep_tool.py b/tests/stacklets/test_agent_grep_tool.py new file mode 100644 index 00000000..0e668045 --- /dev/null +++ b/tests/stacklets/test_agent_grep_tool.py @@ -0,0 +1,21 @@ +from __future__ import annotations + +from grep_tool import _PATH_RE, _is_vault_path + + +def test_detects_vault_paths(): + assert _is_vault_path("vault") + assert _is_vault_path("./vault") + assert _is_vault_path("vault/family") + assert _is_vault_path("./vault/family") + + +def test_non_vault_paths_are_not_routed(): + assert not _is_vault_path(".") + assert not _is_vault_path("memory/history.jsonl") + assert not _is_vault_path("not-vault/family") + + +def test_extracts_memory_result_paths(): + block = "#1 2026-06-23 [] family/emails/example.md score=0.7\n Title\n" + assert _PATH_RE.findall(block) == ["family/emails/example.md"] diff --git a/tests/stacklets/test_agent_runtime_shims.py b/tests/stacklets/test_agent_runtime_shims.py new file mode 100644 index 00000000..87104c43 --- /dev/null +++ b/tests/stacklets/test_agent_runtime_shims.py @@ -0,0 +1,125 @@ +"""What the agent's nanobot shims promise: they are actually attached. + +`sitecustomize.py` patches nanobot internals at interpreter startup. Every +patch is wrapped in try/except-and-log, deliberately, because a broken shim +must never stop the agent answering. The cost of that design is that a shim +which fails to attach looks exactly like one that worked: the agent starts, +nothing raises, and the capability is simply absent. Three tools sat dead in +the image for weeks that way. + +So these tests assert the *attached state*, never "it did not raise". A shim +that swallows its own failure leaves the original symbol in place, and the +assertions below fail on that. That is the whole point of the file. + +They also make `sitecustomize.py`'s PIN / RECHECK list executable. Bump +`nanobot-ai`, move one of those symbols, and the unit lane goes red here +instead of the breakage reaching the rig as a log line nobody reads. +""" + +from __future__ import annotations + +import importlib +import sys + +import pytest + +SHIMMED_MODULES = ("sitecustomize", "brief", "lean_state", + "memory_tool", "person_tool", "grep_tool") + + +# The stub nanobot itself lives in conftest as `nanobot_stub`, shared with +# the vault-tool tests that drive the tools these shims register. + +@pytest.fixture +def nanobot(monkeypatch, nanobot_stub): + """Install a stub nanobot and import `sitecustomize` against it. + + Returns a callable so a test can drop a symbol first and watch what + survives. Modules are purged before each import so the shims re-run + rather than returning a cached, already-patched module. + """ + def _load(drop: str | None = None): + mods = nanobot_stub() + if drop: + module_name, _, attr = drop.rpartition(".") + delattr(mods[module_name], attr) + for name, module in mods.items(): + monkeypatch.setitem(sys.modules, name, module) + for name in SHIMMED_MODULES: + monkeypatch.delitem(sys.modules, name, raising=False) + importlib.import_module("sitecustomize") + return mods + + yield _load + + for name in SHIMMED_MODULES: + sys.modules.pop(name, None) + + +def _discovered(mods) -> set[str]: + loader = mods["nanobot.agent.tools.loader"].ToolLoader() + return {t.__name__ for t in loader.discover()} + + +# ── the tools are reachable by the agent ───────────────────────────────── + +def test_vault_tools_are_registered(nanobot): + """Without this, the agent cannot search the vault or read a profile. + + `install()` appends to `ToolLoader.discover`, so the check is what a + freshly built loader hands back, which is what nanobot itself asks for. + """ + mods = nanobot() + assert _discovered(mods) == {"MemorySearchTool", "MemoryPersonTool"} + + +def test_vault_greps_are_routed_through_memory_search(nanobot): + """A grep under `vault/` must no longer hit the stock literal matcher. + + The vault is prose. Literal grep over it answers almost nothing, which + is why this routing exists. + """ + mods = nanobot() + grep = mods["nanobot.agent.tools.search"].GrepTool + assert grep.execute.__name__ == "execute_with_memory" + + +def test_context_shims_are_attached(nanobot): + """The two older shims, pinned the same way as the new tools.""" + mods = nanobot() + ctx = mods["nanobot.agent.context"] + assert ctx.runtime_lines.__name__ == "_runtime_lines" + assert ctx.ContextBuilder.build_messages.__name__ == "_build_messages_lean" + + +# ── failure is contained, and visible ──────────────────────────────────── + +def test_a_moved_symbol_does_not_take_the_others_down(nanobot): + """One missing nanobot symbol must cost only its own tool. + + This is why each install runs in its own try. Sharing one block would + mean a renamed GrepTool silently removed memory_search too, and the + agent would lose vault access over an unrelated upgrade. + """ + mods = nanobot(drop="nanobot.agent.tools.search.GrepTool") + + assert _discovered(mods) == {"MemorySearchTool", "MemoryPersonTool"} + + +def test_the_stub_can_actually_express_a_detached_shim(nanobot): + """Guards the guard: prove these assertions can fail. + + A test suite that cannot distinguish attached from detached would pass + against the very bug this file exists to catch, which is the state the + codebase was in before it was written. + """ + mods = nanobot(drop="nanobot.agent.tools.loader.ToolLoader") + + tools = mods["nanobot.agent.tools.loader"] + assert not hasattr(tools, "ToolLoader"), "the drop hook must really remove it" + + # Nothing to append to, so neither tool can have registered anywhere. + grep = mods["nanobot.agent.tools.search"].GrepTool + assert grep.execute.__name__ == "execute_with_memory", ( + "grep routing is independent of the loader and should still attach" + ) diff --git a/tests/stacklets/test_agent_vault_tools.py b/tests/stacklets/test_agent_vault_tools.py new file mode 100644 index 00000000..ffc28fb9 --- /dev/null +++ b/tests/stacklets/test_agent_vault_tools.py @@ -0,0 +1,299 @@ +"""What the agent's vault tools promise: the commands they run actually work. + +`test_agent_runtime_shims.py` proves the tools are registered with nanobot. +That is necessary and it is not enough. Both tools shipped registered and +non-functional, and every unit test stayed green, because nothing checked +the one thing that matters at runtime: the command line each tool builds +has to survive two gates it never sees. + + 1. `stacklets/core/famstack-api.py` DOMAIN_ALLOW. The agent is an LLM, so + it reaches the CLI through a curated allowlist. `memory person` was + missing from it, so every call came back + "error: 'memory person' is not allowed". + + 2. The memory CLI's own argument parser. `memory_search` passed + `--backend mem0`, a flag that has never existed, so every search + returned a usage error and the agent looped retrying it. + +Both gates live in other components, which is exactly why a stubbed +nanobot could not see either. So these tests drive the real tools, capture +the real argv, and hand it to the real allowlist matcher and the real +argparse parsers. Nothing here restates what the tools do; it asks the +components that judge them. +""" + +from __future__ import annotations + +import asyncio +import importlib +import importlib.util +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent +MEMORY_DIR = REPO_ROOT / "stacklets" / "memory" + +TOOL_MODULES = ("memory_tool", "person_tool", "grep_tool", "sitecustomize") + + +# ── loading the real components under test ─────────────────────────── + +def _load_from_path(name: str, path: Path): + """Import a module by file path (famstack-api.py is not importable).""" + spec = importlib.util.spec_from_file_location(name, path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(scope="module") +def api(): + """The core API module that gates every command the agent runs.""" + return _load_from_path("famstack_api", + REPO_ROOT / "stacklets" / "core" / "famstack-api.py") + + +@pytest.fixture(scope="module") +def memory_cli(): + """The real memory CLI modules, for their real argparse parsers.""" + if str(MEMORY_DIR) not in sys.path: + sys.path.insert(0, str(MEMORY_DIR)) + return { + name: _load_from_path(f"memory_cli_{name}", + MEMORY_DIR / "cli" / f"{name}.py") + for name in ("search", "person") + } + + +@pytest.fixture +def vault_tools(monkeypatch, nanobot_stub): + """The agent's tool classes, imported against a stub nanobot.""" + for name, module in nanobot_stub().items(): + monkeypatch.setitem(sys.modules, name, module) + for name in TOOL_MODULES: + monkeypatch.delitem(sys.modules, name, raising=False) + + tools = { + "memory_search": importlib.import_module("memory_tool").MemorySearchTool, + "memory_person": importlib.import_module("person_tool").MemoryPersonTool, + } + yield tools + for name in TOOL_MODULES: + sys.modules.pop(name, None) + + +def argv_of(tool_cls, **kwargs) -> list[str]: + """Run a tool and return the command line it tried to execute. + + Captures at `create_subprocess_exec` so the argv asserted on is the + argv the tool would really have run, not a restatement of it. + """ + captured: list[str] = [] + + class _Proc: + returncode = 0 + + async def communicate(self): + return b"ok", b"" + + async def _fake_exec(*args, **_kwargs): + captured.extend(args) + return _Proc() + + monkey = pytest.MonkeyPatch() + monkey.setattr(asyncio, "create_subprocess_exec", _fake_exec) + try: + asyncio.run(tool_cls().execute(**kwargs)) + finally: + monkey.undo() + return captured + + +# ── gate 1: the core API allowlist ─────────────────────────────────── + +@pytest.mark.parametrize("tool_name,kwargs", [ + ("memory_search", {"query": "Homer"}), + ("memory_person", {"name": "homer"}), +]) +def test_the_command_a_tool_runs_is_allowed_by_the_api(api, vault_tools, + tool_name, kwargs): + """A tool the agent cannot actually invoke is worse than a missing one. + + Asserted with DOMAIN_ALLOW's own matching rule rather than a copy of + it, so tightening that rule fails here instead of in production. + """ + argv = argv_of(vault_tools[tool_name], **kwargs) + + assert argv[0] == "stack", "tools invoke the CLI through the client shim" + args = argv[1:] + permitted = any(args[:len(p)] == p for p in api.DOMAIN_ALLOW) + + assert permitted, ( + f"{tool_name} runs 'stack {' '.join(args[:2])}', which DOMAIN_ALLOW " + f"rejects. Allowed: {[' '.join(p) for p in api.DOMAIN_ALLOW]}" + ) + + +def test_the_allowlist_stays_read_only(api): + """The agent must never reach a command that changes the stack. + + DOMAIN_ALLOW is a security boundary, not a convenience list. Adding + memory person to it is fine; adding memory sync or a lifecycle verb + would hand an LLM write access to the instance. + """ + forbidden = {"up", "down", "restart", "destroy", "setup", "sync", + "pull", "wiki", "ontology"} + for path in api.DOMAIN_ALLOW: + assert not forbidden & set(path), f"{path} reaches a mutating command" + + +# ── gate 2: the memory CLI's real parser ───────────────────────────── + +@pytest.mark.parametrize("tool_name,command,kwargs", [ + ("memory_search", "search", + {"query": "Homer", "limit": 10, "person": "homer", "tag": "Insurance", + "scope": "family"}), + ("memory_person", "person", {"name": "homer"}), +]) +def test_the_flags_a_tool_sends_are_flags_the_cli_accepts( + memory_cli, vault_tools, tool_name, command, kwargs): + """Every option the tool passes has to parse, including the optional ones. + + The optional arguments matter most: a flag only sent when the model + fills in that parameter is one a hand test almost never exercises, + which is how `--backend mem0` survived. Parsed by the CLI's own + parser, so renaming a flag breaks this test rather than the agent. + """ + argv = argv_of(vault_tools[tool_name], **kwargs) + assert argv[1:3] == ["memory", command] + + parser = memory_cli[command]._parser() + try: + parser.parse_args(argv[3:]) + except SystemExit as exit_: + pytest.fail( + f"{tool_name} builds `{' '.join(argv)}`, which " + f"`stack memory {command}` rejects (argparse exit {exit_.code})" + ) + + +def test_an_unknown_flag_would_be_caught(memory_cli): + """Guards the guard: prove the parser check can actually fail. + + Without this, a parser that silently swallowed unknown options would + make the test above pass against the very bug it exists to catch. + """ + parser = memory_cli["search"]._parser() + with pytest.raises(SystemExit): + parser.parse_args(["Homer", "--backend", "mem0"]) + + +def test_search_sends_no_backend_flag(vault_tools): + """The specific regression: `--backend mem0` was never a real option. + + Pinned by name because the failure it caused was silent from the + agent's side. Every search returned a usage error, the model read it + as an empty result, and retried with different arguments instead of + surfacing anything. + """ + argv = argv_of(vault_tools["memory_search"], query="Homer") + + assert "--backend" not in argv + + +# ── the vault root a profile actually lives in ─────────────────────── + +def test_person_reads_generated_profiles(memory_cli, tmp_path): + """`about.md` pages are generated, and generation writes to the brain. + + memory is source-only. The installer purges generated pages from it + ("purged 1 generated source page(s)"), so a person page can only ever + be found in the brain projection. Reading the source vault alone + meant the command returned "no profile" for every family member who + had one. + """ + brain = tmp_path / "memory" / "brain" + (brain / "homer").mkdir(parents=True) + (brain / "homer" / "about.md").write_text( + "---\ntitle: Homer\nslug: homer\ncanonical: Homer\n---\n\n" + "# Homer\n\nSafety Inspector, Sector 7-G.\n", encoding="utf-8") + + result = memory_cli["person"].run( + ["homer", "--no-refresh"], None, {"data_dir": str(tmp_path)}) + + assert result.get("path") == "homer/about.md", ( + f"expected the generated profile, got {result}" + ) + + +def test_a_hand_written_source_page_wins(memory_cli, tmp_path, capsys): + """Source beats projection when a household curates a page by hand. + + The brain is rebuildable output; memory is what a family actually + wrote. If both hold a page for the same person, the curated one is + the answer. Both files sit at the same relative path, so the printed + body is the only thing that can prove which one was read. + """ + vault = tmp_path / "memory" / "vault" + brain = tmp_path / "memory" / "brain" + for root, marker in ((vault, "hand written"), (brain, "generated")): + (root / "homer").mkdir(parents=True) + (root / "homer" / "about.md").write_text( + f"---\ntitle: Homer\nslug: homer\ncanonical: Homer\n---\n\n" + f"# Homer\n\n{marker}\n", encoding="utf-8") + + result = memory_cli["person"].run( + ["homer", "--no-refresh"], None, {"data_dir": str(tmp_path)}) + + assert result.get("ok") is True + assert "hand written" in capsys.readouterr().out + + +def test_missing_person_still_reports_cleanly(memory_cli, tmp_path): + """No profile anywhere is a clean error, not a crash.""" + (tmp_path / "memory" / "brain").mkdir(parents=True) + + result = memory_cli["person"].run( + ["nobody", "--no-refresh"], None, {"data_dir": str(tmp_path)}) + + assert "error" in result + + +# ── the tree the agent's file tools actually see ───────────────────── + +def test_the_agent_mounts_the_tree_that_holds_profiles(): + """Stacky's `vault/` mount must contain the pages it promises. + + The mount exists so nanobot's workspace-scoped file tools can read + person and topic pages, and the model prefers those tools over + anything else: given a `vault/` directory it will read and grep it + rather than call `memory_person`. So whatever is mounted there is, + in practice, the agent's whole picture of the family. + + Generated pages live only in the brain projection. Mounting the + memory source clone therefore handed the agent a tree that could + never hold a profile, and it reported "There is no + vault/homer/about.md" for a member who had one. Wrong beats missing: + a confident denial is worse than no answer. + + Compared against the memory stacklet's own declaration rather than a + hardcoded path, so if memory ever moves the projection this fails + instead of silently drifting. + """ + import tomllib + + agent = tomllib.loads( + (REPO_ROOT / "stacklets" / "agent" / "stacklet.toml").read_text()) + memory = tomllib.loads( + (REPO_ROOT / "stacklets" / "memory" / "stacklet.toml").read_text()) + + mounted = agent["env"]["defaults"]["MEMORY_VAULT_DIR"] + brain = memory["env"]["defaults"]["BRAIN_REPO_DIR"] + source = memory["env"]["defaults"]["MEMORY_VAULT_DIR"] + + assert mounted == brain, ( + f"agent mounts {mounted!r}; generated profiles live in {brain!r}" + ) + assert mounted != source, "the source clone never holds generated pages" diff --git a/tests/stacklets/test_matrix_client.py b/tests/stacklets/test_matrix_client.py new file mode 100644 index 00000000..be5ac5ad --- /dev/null +++ b/tests/stacklets/test_matrix_client.py @@ -0,0 +1,99 @@ +"""What `MatrixClient.create_user` promises about existing accounts. + +Synapse's admin API creates users with `PUT /_synapse/admin/v2/users/{id}`, +an upsert. The sharp edge is that including `password` in that PUT does not +merely record a credential: Synapse also invalidates the account's devices. +Re-asserting the same password therefore still logs the account out. + +That is fine for a human, who logs back in. It is fatal for a bot, which +has nothing watching it, and it is how the whole stack once ended up with a +stacker-bot that could not authenticate (FAM-2). + +These tests drive the client against `pytest-httpserver` and assert on the +request body that reaches the wire, because the contract we care about is +what Synapse is told, not how the method arranges its locals. +""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path + +import pytest +from pytest_httpserver import HTTPServer + +_REPO_ROOT = Path(__file__).resolve().parent.parent.parent +sys.path.insert(0, str(_REPO_ROOT / "stacklets" / "messages" / "cli")) + +from _matrix import MatrixClient # noqa: E402 + + +ADMIN_PATH = "/_synapse/admin/v2/users/@stacker-bot:simpson" + + +@pytest.fixture +def client(httpserver: HTTPServer): + c = MatrixClient(httpserver.url_for(""), "simpson", str(_REPO_ROOT)) + c.token = "admin-token" + return c + + +def _put_bodies(httpserver): + """The JSON body of every PUT that reached the server, in order.""" + return [ + json.loads(req.get_data()) + for req, _ in httpserver.log + if req.method == "PUT" + ] + + +def test_existing_account_keeps_its_password(httpserver, client): + """`reset_password=False` must not re-send the password of a live account. + + This is the FAM-2 invariant. Setup re-runs against an instance whose bot + is already provisioned and running, so the PUT has to be a profile + update only. + """ + httpserver.expect_request(ADMIN_PATH, method="GET").respond_with_json( + {"name": "@stacker-bot:simpson"} + ) + httpserver.expect_request(ADMIN_PATH, method="PUT").respond_with_json({}, status=200) + + assert client.create_user("stacker-bot", "s3cret", displayname="Stacker", + reset_password=False) + + body = _put_bodies(httpserver)[0] + assert "password" not in body, "an existing bot must not be re-credentialed" + assert body["displayname"] == "Stacker" + + +def test_missing_account_is_still_created_with_a_password(httpserver, client): + """`reset_password=False` still provisions an account that does not exist. + + Otherwise a fresh install would produce a passwordless bot, which is a + worse failure than the one this flag exists to prevent. + """ + httpserver.expect_request(ADMIN_PATH, method="GET").respond_with_json( + {"errcode": "M_NOT_FOUND"}, status=404 + ) + httpserver.expect_request(ADMIN_PATH, method="PUT").respond_with_json({}, status=201) + + assert client.create_user("stacker-bot", "s3cret", reset_password=False) + + assert _put_bodies(httpserver)[0]["password"] == "s3cret" + + +def test_password_is_reset_by_default(httpserver, client): + """The default stays a full upsert, for callers that do own the credential. + + Family accounts are provisioned from `users.toml` on every setup run and + are meant to converge on the stored secret. + """ + httpserver.expect_request(ADMIN_PATH, method="PUT").respond_with_json({}, status=200) + + assert client.create_user("stacker-bot", "s3cret") + + assert _put_bodies(httpserver)[0]["password"] == "s3cret" + assert not [req for req, _ in httpserver.log if req.method == "GET"], \ + "the default path should not need an existence check" diff --git a/tests/stacklets/test_memory_correspondents.py b/tests/stacklets/test_memory_correspondents.py index 20103f55..b8b1839f 100644 --- a/tests/stacklets/test_memory_correspondents.py +++ b/tests/stacklets/test_memory_correspondents.py @@ -73,7 +73,9 @@ def test_loads_aliases_topics_and_contact_fields(self, vault): aliases: - "Duff Insurance Ortsverband Springfield" - "Duff Insurance Versicherung AG" -topics: [insurance, vehicle] +topics: + - insurance + - vehicle address: "Hansastraße 19, 80686 München" phone: "089 7676 0" website: "https://www.duff-insurance.de" diff --git a/tests/stacklets/test_memory_host_stdlib.py b/tests/stacklets/test_memory_host_stdlib.py new file mode 100644 index 00000000..c633330c --- /dev/null +++ b/tests/stacklets/test_memory_host_stdlib.py @@ -0,0 +1,116 @@ +"""The memory lib reads the vault on a host with no pip packages. + +`./stack` runs under the system interpreter with `PYTHONPATH=lib` and +nothing else. README.md calls the CLI "zero pip deps" and +docs/admin-guide.md says "no virtualenvs, no pip install. That is +intentional." Every host-side read path therefore has to work with the +stdlib plus `stack.*`. + +This is easy to break without noticing, because the `test` extra +installs `python-frontmatter` for the bot suites. A loader that reaches +for it stays green here and fails for every real user the moment they +run the command. That is exactly what happened: `stack memory person` +shipped and could not run on any clean host. + +So these tests make the *production* environment the thing under test. +Blocking the module in `sys.modules` is what a machine that never ran +`pip install` looks like from inside an import statement. Assert on the +data the loaders return, not on which parser they chose, so a future +swap to another stdlib parser keeps them passing. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent + / "stacklets" / "memory")) + +from lib import ( # noqa: E402 + load_correspondents_from_vault, + load_persons_from_vault, +) + + +@pytest.fixture +def bare_host(monkeypatch): + """A host where `import frontmatter` fails, as on a real install. + + Setting the entry to None is how CPython represents "this import + has already been tried and there is nothing there": the next + `import frontmatter` raises ImportError without touching the disk. + """ + monkeypatch.setitem(sys.modules, "frontmatter", None) + with pytest.raises(ImportError): + import frontmatter # noqa: F401 + return True + + +def _write(path: Path, text: str) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +# ── persons: what `stack memory person` walks ──────────────────────── + +def test_persons_load_without_the_pip_package(bare_host, tmp_path): + """`stack memory person ` resolves a name on a clean host. + + Name resolution is the only reason that command parses frontmatter + at all -- the profile body is stripped with a regex. If this raises, + the command is dead on arrival for everyone. + """ + _write(tmp_path / "marge" / "about.md", + "---\ntitle: Marge\nslug: marge\ncanonical: Marge\n" + "synonyms:\n - Marjorie\n - Marge Bouvier\n---\n\n# Marge\n") + + [person] = load_persons_from_vault(tmp_path) + + assert person.canonical == "Marge" + assert person.slug == "marge" + assert person.synonyms == ["Marjorie", "Marge Bouvier"] + + +def test_person_kind_filter_survives_on_a_bare_host(bare_host, tmp_path): + """A non-person page at a member path stays excluded. + + Worth pinning separately: if the parser returned nothing on a bare + host, `kind` would read as absent, the page would fall back to its + slug, and a correspondent would quietly enter the family roster. + Degrading to an empty dict is not a safe failure here. + """ + _write(tmp_path / "duff-insurance" / "about.md", + "---\nkind: correspondent\ncanonical: Duff Insurance\n---\n") + + assert load_persons_from_vault(tmp_path) == [] + + +# ── correspondents: same import, same exposure ─────────────────────── + +def test_correspondents_load_without_the_pip_package(bare_host, tmp_path): + """`stack memory correspondents` shares the defect and the fix.""" + _write(tmp_path / "family" / "correspondents" / "duff.md", + "---\nkind: correspondent\ncanonical: Duff Brewery\n" + "aliases:\n - Duff Beer\n---\n\n# Duff Brewery\n") + + [correspondent] = load_correspondents_from_vault(tmp_path, + shared_bucket="family") + + assert correspondent.canonical == "Duff Brewery" + assert correspondent.aliases == ["Duff Beer"] + + +def test_a_vault_with_nothing_in_it_is_not_an_error(bare_host, tmp_path): + """The empty case ran before the import did, which is what hid this. + + `stack memory person` looked fine against an empty vault because it + returned before reaching the import. Pin both loaders on an empty + vault so that early return can never again pass for proof that the + populated path works. + """ + assert load_persons_from_vault(tmp_path) == [] + assert load_correspondents_from_vault(tmp_path, shared_bucket="family") == [] diff --git a/tests/stacklets/test_messages_setup.py b/tests/stacklets/test_messages_setup.py new file mode 100644 index 00000000..a50cfa7c --- /dev/null +++ b/tests/stacklets/test_messages_setup.py @@ -0,0 +1,127 @@ +"""Test Matrix setup script invariants. + +These tests drive the setup script from the caller's side, verifying +what it promises instead of how it implements it (AGENTS.md principle 6). +""" + +import sys +from pathlib import Path + + +_REPO_ROOT = Path(__file__).resolve().parent.parent.parent +sys.path.insert(0, str(_REPO_ROOT / "stacklets" / "messages" / "cli")) + +from setup import _setup # type: ignore + + +class FakeMatrixClient: + def __init__(self): + self.server_name = "simpson" + self.base_url = "http://fake" + self.repo_root = str(_REPO_ROOT) + self.logins = [] + self.created_users = [] + + # Responses to prevent setup from bailing out early + self.created_rooms = {} + + def login(self, username, password): + self.logins.append((username, password)) + return True + + def create_room(self, alias, name=None, topic=None, space=False, parent=None): + room_id = f"!{alias}:simpson" + self.created_rooms[alias] = room_id + return room_id + + def resolve_alias(self, alias): + return f"!{alias}:simpson" + + def create_user(self, username, password, displayname=None, admin=False, + reset_password=True): + self.created_users.append((username, password)) + return True + + def set_power_level(self, room_id, user_id, level): + return "ok" + + def invite_user(self, room_id, user_id): + return True + + def join_user(self, room_id, username): + return True + + def get_room_members(self, room_id): + return [] + + def resolve_room(self, alias): + return f"!{alias}:simpson" + + def open_space_to_members(self, space_id): + return "ok" + + def add_space_child(self, space_id, child_id): + return True + + def send(self, room_alias, plain, html=None): + pass + + +def test_stacker_bot_canonical_password(tmp_path): + """Messages setup uses core__STACKER_BOT_PASSWORD without overwriting it. + + The core stacklet owns the stacker bot. If a credential for it already + exists in the core namespace, the messages setup (which runs first) + must use it to provision the bot in Synapse, rather than minting + a competing password in its own namespace. + + instance_dir points at tmp_path so that a regression, which would take + the mint-and-persist branch, writes its secret there instead of into + the developer's live .stack/secrets.toml. + """ + client = FakeMatrixClient() + users = [{"id": "homer", "display_name": "Homer"}] + config = {"instance_dir": str(tmp_path)} + + # Given a secrets store holding the core-owned password... + class FakeSecrets: + def __init__(self): + self.store = { + "global__ADMIN_PASSWORD": "admin-pass", + "core__STACKER_BOT_PASSWORD": "canonical-core-pass", + } + + def get(self, key, default=None): + return self.store.get(key, default) + + secrets = FakeSecrets() + + import setup + + # We need to mock MatrixClient inside setup.py because it instantiates a new one + # for the bot login. + original_matrix_client = setup.MatrixClient + setup.MatrixClient = lambda base_url, server_name, repo_root: client + + try: + results = _setup(client, users, config, secrets) + finally: + setup.MatrixClient = original_matrix_client + + assert "error" not in results, f"Setup failed: {results}" + + # The Stacker bot account was provisioned with the canonical core password + stacker_creates = [u for u in client.created_users if u[0] == "stacker-bot"] + assert len(stacker_creates) == 1 + assert stacker_creates[0][1] == "canonical-core-pass" + + # No competing credential was minted. The mint-and-persist branch writes + # through TomlSecretStore to /.stack/secrets.toml, so the + # absence of that file is proof the branch never ran. + assert not (tmp_path / ".stack" / "secrets.toml").exists() + + # The bot logged in with the canonical password + stacker_logins = [login_info for login_info in client.logins if login_info[0] == "stacker-bot"] + assert len(stacker_logins) == 1 + assert stacker_logins[0][1] == "canonical-core-pass" + diff --git a/tools/branch-status b/tools/branch-status new file mode 100755 index 00000000..5ce8e8d1 --- /dev/null +++ b/tools/branch-status @@ -0,0 +1,156 @@ +#!/usr/bin/env python3 +"""Classify local branches as already-in-main or genuinely diverged. + +`git branch --merged` is unreliable here. famstack squash-merges, so a +branch whose every line is already in main still points at commits main +never saw, and git calls it unmerged. Thirty-odd branches looked alive +that way when almost none were. + +The reliable question is about content, not ancestry: take the files a +branch touched since it forked, and ask whether main's version of those +same paths already says the same thing. If it does, the branch is a +duplicate of history and deleting it loses nothing. + +Usage: + tools/branch-status # classify every local branch + tools/branch-status --merged # print only the safe-to-delete names + tools/branch-status --diverged # print only the ones carrying content + +Exit code is 0 when the classification completes. This is a read-only +report; it never deletes anything. +""" + +from __future__ import annotations + +import argparse +import subprocess +import sys + +MAIN = "main" +MERGED = "merged-or-empty" +DIVERGED = "diverged" + +# git's hash of the empty tree — the "before anything existed" baseline. +EMPTY_TREE = "4b825dc642cb6eb9a060e54bf8d69288fbee4904" + + +def git(*args: str) -> str: + r = subprocess.run(["git", *args], capture_output=True, text=True) + if r.returncode != 0: + raise RuntimeError(f"git {' '.join(args)}: {r.stderr.strip()}") + return r.stdout + + +def local_branches() -> list[str]: + out = git("for-each-ref", "--format=%(refname:short)", "refs/heads/") + return [b for b in out.split() if b != MAIN] + + +def blob_id(ref: str, path: str) -> str | None: + """The object id of a file at a ref, or None when it is absent there.""" + r = subprocess.run(["git", "rev-parse", f"{ref}:{path}"], + capture_output=True, text=True) + return r.stdout.strip() if r.returncode == 0 else None + + +def main_blobs() -> set[str]: + """Every object id reachable from main, across its whole history. + + This is what makes the comparison honest. Checking a branch against + main's *current* files answers the wrong question: main edits those + files again after a merge, so a branch whose work landed months ago + still looks different. Asking whether main's history ever contained + this exact content answers the question we actually care about. + """ + out = git("rev-list", "--objects", MAIN) + return {line.split()[0] for line in out.splitlines() if line} + + +def merge_base(branch: str) -> str: + """Where the branch forked from main. + + Falls back to the empty tree for branches with no common ancestor + (this repo has one such root left over from before it moved to + GitHub). Comparing such a branch from nothing means every file it + contains gets checked against main, which is the honest question. + """ + r = subprocess.run(["git", "merge-base", MAIN, branch], + capture_output=True, text=True) + if r.returncode == 0 and r.stdout.strip(): + return r.stdout.strip() + return EMPTY_TREE + + +def touched_paths(branch: str) -> list[str]: + """Paths the branch changed since it forked from main. + + Measured from the merge base rather than main's tip, so work main has + done in the meantime is not mistaken for the branch's own. + """ + out = git("diff", "--name-only", f"{merge_base(branch)}..{branch}") + return [p for p in out.splitlines() if p] + + +def classify(branch: str, known: set[str]) -> tuple[str, list[str]]: + """Return (verdict, paths carrying content main has never held). + + A path is accounted for when main's history contains the exact blob + the branch ends with. A path the branch deleted is accounted for when + main has no such file either. + """ + unique = [] + for path in touched_paths(branch): + tip = blob_id(branch, path) + accounted = (tip in known) if tip else (blob_id(MAIN, path) is None) + if not accounted: + unique.append(path) + return (DIVERGED if unique else MERGED), unique + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__) + group = ap.add_mutually_exclusive_group() + group.add_argument("--merged", action="store_true", + help="print only branch names that are safe to delete") + group.add_argument("--diverged", action="store_true", + help="print only branch names that still carry content") + args = ap.parse_args() + + current = git("rev-parse", "--abbrev-ref", "HEAD").strip() + known = main_blobs() + results = [(b, *classify(b, known)) for b in local_branches()] + + if args.merged or args.diverged: + want = MERGED if args.merged else DIVERGED + for branch, verdict, _ in results: + # Never offer the checked-out branch for deletion. + if verdict == want and not (args.merged and branch == current): + print(branch) + return 0 + + merged = [r for r in results if r[1] == MERGED] + diverged = [r for r in results if r[1] == DIVERGED] + + print(f"\n {len(results)} local branches, compared against {MAIN} " + f"at {git('rev-parse', '--short', MAIN).strip()}\n") + + if diverged: + print(f" Diverged ({len(diverged)}) - carry content {MAIN} does not have:\n") + for branch, _, unique in sorted(diverged): + shown = ", ".join(unique[:3]) + (" ..." if len(unique) > 3 else "") + print(f" {branch}") + print(f" {len(unique)} file(s): {shown}") + print() + + if merged: + print(f" Merged or empty ({len(merged)}) - safe to delete:\n") + for branch, _, _ in sorted(merged): + marker = " (checked out)" if branch == current else "" + print(f" {branch}{marker}") + print() + + return 0 + + +if __name__ == "__main__": + sys.exit(main())