From e4e2332e01ab26b3e7ba1be1ef734a533efd7c5b Mon Sep 17 00:00:00 2001 From: Torben Nehmer Date: Wed, 16 Sep 2026 19:18:00 +0200 Subject: [PATCH] session: Harness-Session-Variable schliesst die Luecke im Session-Id-Fallback (Telemetrie-Join, Iteration-Budget-Gate); SIGPIPE-Nebenbefund im Emitter behoben Files changed: - CHANGES.md - EVALS.md - INSTALL.md - VERSION - instructions/session-setup.md - tools/CONTRACT.md - tools/chemenu/cli.py - tools/chemenu/commands/doctor.py - tools/chemenu/commands/run_budget.py - tools/chemenu/session.py - tools/chemenu/telemetry/writer.py - tools/chemenu/tests/conftest.py - tools/chemenu/tests/test_cli.py - tools/chemenu/tests/test_run_budget.py - tools/chemenu/tests/test_telemetry_emit.py --- CHANGES.md | 52 ++++++- EVALS.md | 18 ++- INSTALL.md | 2 +- VERSION | 2 +- instructions/session-setup.md | 34 +++-- tools/CONTRACT.md | 11 +- tools/chemenu/cli.py | 106 +++++++++++-- tools/chemenu/commands/doctor.py | 14 +- tools/chemenu/commands/run_budget.py | 23 ++- tools/chemenu/session.py | 68 ++++++++- tools/chemenu/telemetry/writer.py | 19 ++- tools/chemenu/tests/conftest.py | 10 +- tools/chemenu/tests/test_cli.py | 167 +++++++++++++++++++++ tools/chemenu/tests/test_run_budget.py | 85 +++++++++++ tools/chemenu/tests/test_telemetry_emit.py | 43 ++++++ 15 files changed, 610 insertions(+), 44 deletions(-) create mode 100644 tools/chemenu/tests/test_cli.py diff --git a/CHANGES.md b/CHANGES.md index 8c5a667..8dc3f2c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -59,11 +59,15 @@ concern - readable here, never shipped as something to parse. --- -## 6.1.0-beta.4 - 2026-09-16 - version notes antwortet auf einer ausgelieferten Instanz aus dem Release-Feed +## 6.1.0-beta.5 - 2026-09-16 - Session-Id-Fallback: Harness-Variable schliesst die Luecke zwischen Telemetrie-Join und Iteration-Budget-Gate **Author:** Torben Nehmer +**High impact** +- Session-Id-Fallback: Harness-Variable schliesst die Luecke zwischen Telemetrie-Join und Iteration-Budget-Gate + +**Medium impact** - Upgrade-Prozedur als eigene Instruktion statt als Prosa in INSTALL.md - Migrationsdokument prueft gegen eine festgehaltene Vorher-Ausgabe, Beispielverweis auf die .template-Form - dist upgrade: --take-release nimmt fuer einen lokal geaenderten Pfad die Release-Fassung @@ -252,6 +256,52 @@ Bei der Gelegenheit zwei Eintraege aus `tools/CONTRACT.md` § "Future considerat implemented)" entfernt, die dort seit ihrer Umsetzung falsch standen: der MCP-Server-Wrapper und `dist upgrade` selbst. Beide sind im selben Dokument weiter oben als existierend beschrieben. +### Session-Id-Fallback: Harness-Variable schliesst die Luecke zwischen Telemetrie-Join und Iteration-Budget-Gate + +Gemessen an einem getracten Lauf (33 `wikitool`-Aufrufe, eine Sitzung): unter Claude Code, dessen +Bash-Tool jeden Aufruf in einer frisch initialisierten Shell ausfuehrt, fiel `chemenu.session` +ohne gesetztes `WIKITOOL_SESSION_ID` auf `os.getppid()` zurueck - eine neue "Sitzung" pro Aufruf. +Der Lauf zerfiel so in 21 Telemetrie-Buckets (hoechster Bucket: 3 von 33 Aufrufen), und das +Iteration-Budget-Gate (60 Aufrufe, Loop-Breaker bei 3 identischen in Folge) sah nie mehr als 3 von +60 - strukturell unerreichbar, obwohl `AGENTS.md` es als eine der vier code-durchgesetzten +Sicherungen fuehrt. Derselbe Bruch traf den Telemetrie-Join: Hook-Events (`prompt.submitted`) +trugen die Harness-UUID, `wikitool.call`-Events die wechselnde PID - kein gemeinsamer Schluessel, +und `eval score` bewertete 1-3 Aufrufe statt 33. + +`chemenu.session` bekommt eine dritte Stufe zwischen der expliziten Variable und dem +PID-Fallback: eine kleine Registry bekannter Harness-Session-Variablen (`HARNESS_ENV_VARS`), +heute mit einem verifizierten Eintrag, `CLAUDE_CODE_SESSION_ID`. Verifiziert heisst: gegen eine +echte Sitzung gemessen, dass die Variable ueber Tool-Aufrufe hinweg stabil bleibt (anders als die +Shell-PID) und exakt der Wert ist, den der `UserPromptSubmit`-Hook in die Trace schreibt - der +Wert wird unveraendert als Schluessel uebernommen, kein Praefix, keine Umschreibung, sonst waere +der Join wieder zerstoert. Ein Eintrag wird nur nach genau dieser Verifikation aufgenommen: ein +Variablenname, der zufaellig existiert und etwas anderes bedeutet, waere ein stillerer Fehler als +der PID-Fallback, den er ersetzt. + +`run_budget`s Zustandsdatei (`budget.json`) traegt je Eintrag jetzt die Herkunft seiner Id; faellt +dieselbe Id-Zeichenkette unter eine andere Herkunft als die gespeicherte, beginnt ein neuer +Zaehler statt einen fremden zu erben - ein Eintrag ohne das Feld (vor dieser Aenderung +geschrieben) behaelt seinen Count unveraendert. `doctor` ist jetzt dreiwertig (`OK` fuer eine +explizite Variable oder eine erkannte Harness-Variable, `WARN` nur noch fuer den reinen +PID-Fallback), und sowohl `budget status` als auch der `session.start`-Event der `wikitool`- +Telemetriequelle nennen die Herkunft der Id. + +Im selben Lauf gemessener Nebenbefund auf der Emitter-Seite: ein durch eine geschlossene Pipe +abgebrochener, ansonsten erfolgreicher Aufruf (`... | head`) stand mit `exit_code: 1` in der +Trace - Click faengt `BrokenPipeError` selbst ab und erzwingt `sys.exit(1)`, ununterscheidbar von +einem echten Fehler. `cli.py` installiert jetzt vor jedem Dispatch einen Wrapper um +`stdout`/`stderr`, der einen EPIPE-Schreibfehler schluckt, bevor Click ihn sieht, und markiert den +Trace-Eintrag stattdessen mit `stdout_truncated: true` bei unveraendertem, dem tatsaechlichen +Kommandoerfolg entsprechendem `exit_code`. + +Reproduziert mit Tests, die echte Subprozesse statt In-Process-Aufrufe verwenden - `os.getppid()` +ist sonst ueber die Testlaufzeit hinweg konstant: 61 Aufrufe aus je eigenem Prozess mit nur der +Harness-Variablen loesen das Gate jetzt aus, drei identische ebenso den Loop-Breaker; vor dieser +Aenderung waeren beide Tests gruen und blind gewesen. + +`--minor`: additiv (ein neues optionales `source`-Feld in `budget.json`, die Id faellt weiterhin +auf `getppid()` zurueck, wo keine Variable greift), keine der beiden Drop-in-Richtungen verletzt. + --- ## 6.0.1 - 2026-09-16 - docs toc/verify erreichen die .template-Form einer Referenzdatei diff --git a/EVALS.md b/EVALS.md index 3aee6fd..3c5c940 100644 --- a/EVALS.md +++ b/EVALS.md @@ -57,7 +57,21 @@ flowchart TD - **Hooks enrich.** They add the tool calls the repo layer cannot see: file reads, greps, shell commands, prompts. -Everything joins on `WIKITOOL_SESSION_ID`. +Everything joins on one session id, resolved the same way by every source that has to pick +one - see `chemenu.session`. The chain is `WIKITOOL_SESSION_ID`, then a harness's own session +variable where one is registered (`chemenu.session.HARNESS_ENV_VARS` - Claude Code's +`CLAUDE_CODE_SESSION_ID` today), then the parent process id. The middle step exists because +the last one does not survive a harness that runs every tool call in its own freshly +initialised shell: `os.getppid()` is then a new "session" per call, and neither the join nor +the Iteration Budget Gate below can see more than one or two calls of a real run. A harness +only earns an entry in that chain once a live session has been observed setting the variable, +confirmed to be the exact id its own hooks write elsewhere in a trace - a name that merely +looks plausible would mis-key a session more quietly than the pid fallback it replaced. + +A trace hook that only *observes* tool calls (a `PreToolUse`/`PostToolUse`-style wiring) does +not by itself fix a harness whose events carry a different id than `wikitool`'s own emitter - +the two still would not join. Wiring such a hook is only worth doing once this fallback chain +already keys both sides on the same id. ## The trace @@ -68,7 +82,7 @@ is [tools/chemenu/telemetry/schema.py](tools/chemenu/telemetry/schema.py). |---|---| | `v` | Schema version | | `ts` | ISO-8601 UTC, microsecond precision | -| `session_id` | The join key. `WIKITOOL_SESSION_ID`, else the parent process id | +| `session_id` | The join key - `chemenu.session`'s fallback chain: `WIKITOOL_SESSION_ID`, else a registered harness variable, else the parent process id | | `pid`, `seq` | `seq` counts **within one process**. Sort a trace by `(ts, pid, seq)` | | `source` | `wikitool`, `runner`, or a harness name | | `event` | See below | diff --git a/INSTALL.md b/INSTALL.md index 1eaf946..67a4fe9 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -249,7 +249,7 @@ Ausnahmen (`kb/CONVENTIONS.md`, `kb/*/COLLECTION.md`, `.wikitool-kb.json`) in | Variable | Zweck | Fallback | |----------|-------|----------| | `WIKI_AUTHOR` | Override für den Autornamen neuer Source-Seiten | `git config user.name` - fehlt beides, bricht `new` mit `ERROR` ab | -| `WIKITOOL_SESSION_ID` | Scopt das Iteration-Budget-Gate auf eine Aufgabe statt auf ein Terminal | Parent-Process-ID (siehe [instructions/session-setup.md](instructions/session-setup.md)) | +| `WIKITOOL_SESSION_ID` | Scopt das Iteration-Budget-Gate auf eine Aufgabe statt auf ein Terminal | Eine vom Harness selbst gesetzte Sitzungs-Variable, wo eine bekannt ist (z. B. `CLAUDE_CODE_SESSION_ID`), sonst die Parent-Process-ID (siehe [instructions/session-setup.md](instructions/session-setup.md)) | | `WIKITOOL_UPDATE_URL` | Release-Feed, den `version check` abfragt | Wert aus `.wikitool-release.json`, sonst der Feed der Ursprungs-Instanz | | `WIKITOOL_UPDATE_TOKEN` | Gitea-Token für den Release-Feed | keiner - gegen `torben/chemenu` nicht nötig, nur für einen privaten Fork (siehe unten) | | `CHEMENU_ROOT` | Auf welchen Korpus das Paket zeigt - für einen Aufrufer, der nicht im Checkout selbst liegt | der Checkout, in dem das Paket liegt (`tools/wikitool` verhält sich ohne die Variable unverändert) | diff --git a/VERSION b/VERSION index cd9f23f..142cc1f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.1.0-beta.4 +6.1.0-beta.5 diff --git a/instructions/session-setup.md b/instructions/session-setup.md index 1840a18..8af14bd 100644 --- a/instructions/session-setup.md +++ b/instructions/session-setup.md @@ -7,11 +7,13 @@ description: Scope the wikitool iteration budget to the task by exporting a stab # Scope the session budget Every `wikitool` call is counted against a per-session iteration budget. A "session" is keyed -by `WIKITOOL_SESSION_ID`, falling back to the parent process id when that variable is unset. +by a fallback chain (`chemenu.session`): `WIKITOOL_SESSION_ID` first, then a harness's own +session variable where one is registered (`CLAUDE_CODE_SESSION_ID` today), then the parent +process id. -Without an explicit id, the budget is scoped to whichever shell happened to run the command, -so a task spanning several terminals is counted as several sessions - and one that reuses a -shell inherits an unrelated count. +Without an explicit id, and on a harness with no registered variable, the budget is scoped to +whichever shell happened to run the command, so a task spanning several terminals is counted as +several sessions - and one that reuses a shell inherits an unrelated count. ## Steps @@ -25,21 +27,31 @@ tools/wikitool sync **An `export` only carries if the shell carries.** Several agent harnesses run every tool call in a freshly initialised shell: the working directory survives, shell state - environment variables, -functions - does not, so the variable is gone by the next call and each call falls back to its own -parent pid. The budget then counts a run of thirty calls as thirty sessions of one, and neither -the 60-call ceiling nor the loop-breaker can ever trip. On such a harness, pass the id **inline on -every call** instead, keeping the same value for the whole task: +functions - does not, so the variable is gone by the next call and each call falls back to whatever +the chain's next step resolves to. + +On a harness with a registered variable in that chain (Claude Code, via `CLAUDE_CODE_SESSION_ID`), +the fallback already keeps every call in one bucket without this step - but it scopes to the +*whole* harness session, not to this one task, so a long-running session can carry unrelated prior +work into the same count. Setting `WIKITOOL_SESSION_ID` explicitly still narrows the bucket to the +task at hand, and remains the only way to scope it at all on a harness with no registered +variable - each call falls back to its own parent pid there, and neither the 60-call ceiling nor +the loop-breaker can ever trip (measured directly on a real upgrade run: 33 `wikitool` calls in +one task split into 21 telemetry buckets under the pid fallback alone). On such a harness, pass +the id **inline on every call** instead of `export`, keeping the same value for the whole task: ```bash WIKITOOL_SESSION_ID="wiki-1234" tools/wikitool sync WIKITOOL_SESSION_ID="wiki-1234" tools/wikitool new entity --name "..." ``` -Which of the two applies is answerable in one call: run `tools/wikitool budget status` twice in -separate calls after exporting, and see whether it names the same id both times. +Which of the three applies is answerable in one call: run `tools/wikitool budget status` twice in +separate calls, and see whether it names the same id both times, and where that id came from - +`budget status` prints both. Check the current state at any time with `tools/wikitool budget status`, which is never -counted against the budget itself and prints the id it is counting under. +counted against the budget itself and prints the id it is counting under, and its origin +(`WIKITOOL_SESSION_ID`, a named harness variable, or the parent-pid fallback). **Why `sync` here, not just at publish time.** `publish` already pulls before it pushes, but a session that runs many `wikitool` calls before its first `publish` (an ingest, a multi-page diff --git a/tools/CONTRACT.md b/tools/CONTRACT.md index f21cd84..7ee94bc 100644 --- a/tools/CONTRACT.md +++ b/tools/CONTRACT.md @@ -214,7 +214,7 @@ tools/wikitool --help | Command | Purpose | |---------|---------| -| `doctor [--json]` | Check that this instance is correctly configured: dependencies (Python, ripgrep), author resolution, stack version, git identity/branch/remote, published skills, kb/raw/reports/work/instructions structure, personalization (`USER.md`/`SOUL.md` present **and** filled - a file still carrying the template's sentinel is a `FAIL`, since a renamed template is not a filled one), the KB conventions (`kb/CONVENTIONS.md` present, unsentinelled, and naming all three tool-owned section headings - a `FAIL` on any of the three, because `xref`/`cite` write out of it), the environment note (`ENVIRONMENT.md` - optional, so absent is `OK`; a still-templated one is a `WARN`), generated files, whether the MCP `submit` tool is armed (`.wikitool-upload.json` present/absent/malformed, its limits, and how many submissions are waiting in `mcp-upload/` - absent is `OK` and means the write path does not exist at all, malformed is the one `FAIL` here, since a broken opt-in must not silently disable the limits it exists to enforce), `WIKITOOL_SESSION_ID`, and telemetry state (on/off, why - installation-form default, `.wikitool-telemetry.json`, or `WIKI_TRACE` - and the current session count/byte total against both caps; never `FAIL`, see [EVALS.md](../EVALS.md)). Read-only, exit 1 only on a `FAIL` (a missing remote, session id, or `VERSION` is a `WARN`, not a fault). Exempt from the Iteration Budget Gate | +| `doctor [--json]` | Check that this instance is correctly configured: dependencies (Python, ripgrep), author resolution, stack version, git identity/branch/remote, published skills, kb/raw/reports/work/instructions structure, personalization (`USER.md`/`SOUL.md` present **and** filled - a file still carrying the template's sentinel is a `FAIL`, since a renamed template is not a filled one), the KB conventions (`kb/CONVENTIONS.md` present, unsentinelled, and naming all three tool-owned section headings - a `FAIL` on any of the three, because `xref`/`cite` write out of it), the environment note (`ENVIRONMENT.md` - optional, so absent is `OK`; a still-templated one is a `WARN`), generated files, whether the MCP `submit` tool is armed (`.wikitool-upload.json` present/absent/malformed, its limits, and how many submissions are waiting in `mcp-upload/` - absent is `OK` and means the write path does not exist at all, malformed is the one `FAIL` here, since a broken opt-in must not silently disable the limits it exists to enforce), the session id source (`OK` for `WIKITOOL_SESSION_ID` or a registered harness variable, `WARN` only for the bare parent-pid fallback - see `chemenu.session`), and telemetry state (on/off, why - installation-form default, `.wikitool-telemetry.json`, or `WIKI_TRACE` - and the current session count/byte total against both caps; never `FAIL`, see [EVALS.md](../EVALS.md)). Read-only, exit 1 only on a `FAIL` (a missing remote, session id, or `VERSION` is a `WARN`, not a fault). Exempt from the Iteration Budget Gate | ## Design notes @@ -268,9 +268,12 @@ tools/wikitool --help section): every invocation is recorded and checked in `main()` (`cli.py`) before Typer dispatches to any subcommand, so it applies uniformly without each command needing its own opt-in. State lives in the gitignored - `tools/.wikitool_session/budget.json`, keyed by `WIKITOOL_SESSION_ID` (or - the caller's parent process id as a fallback), so a new terminal/session - starts with a clean budget. Default ceiling: 60 calls/session, or 3 + `tools/.wikitool_session/budget.json`, keyed by `chemenu.session`'s fallback + chain (`WIKITOOL_SESSION_ID`, else a registered harness session variable, + else the caller's parent process id), so a new terminal/session starts with + a clean budget - and a bucket whose recorded origin no longer matches the + current one starts a fresh count rather than inheriting a stranger's. + Default ceiling: 60 calls/session, or 3 identical calls in a row (whichever trips first). A call that left through `_util.fail()` - a rejected argument, or a read-only check reporting findings - is refunded: it declined instead of acting, and the contract's own diff --git a/tools/chemenu/cli.py b/tools/chemenu/cli.py index af61e6a..d1610d7 100644 --- a/tools/chemenu/cli.py +++ b/tools/chemenu/cli.py @@ -3,6 +3,8 @@ The root AGENTS.md holds the invariants that say when these commands are mandatory; tools/CONTRACT.md is the full per-command reference. """ +import errno +import os import sys import time @@ -50,6 +52,82 @@ except ModuleNotFoundError as exc: from chemenu.telemetry import emit # noqa: E402 - after the dependency check + +class _BrokenPipeSwallow: + """Wraps a stream so a write into a closed pipe is dropped instead of + raised - installed on `sys.stdout`/`sys.stderr` before Typer/Click ever + run, so Click's own broken-pipe handling (`click.core.BaseCommand.main`) + never gets the chance to fire. + + Why not just read Click's outcome afterwards: Click already catches this + exact case (`OSError` with `errno.EPIPE`) and turns it into `sys.exit(1)` + to avoid a traceback - a clean-looking exit, but indistinguishable from a + real failure to whatever reads that exit code next. `cli._run_traced` + does exactly that: it is the trace, which recorded a truncated-but- + otherwise-successful `types describe source | head -1` as a tool error + (Gitea #110, measured against a real trace: `exit_code: 1` for a call the + very next, unpiped, retry of which showed `exit_code: 0`). + + Swallowing the write here instead means Click's own handler never + triggers, so the command finishes through its normal exit path - `0` for + an otherwise-successful run - and `sigpipe` on this wrapper is the signal + `_run_traced` reads to note the truncation without miscasting it as an + error. + """ + + def __init__(self, wrapped): + self._wrapped = wrapped + self.sigpipe = False + + def _is_epipe(self, exc: OSError) -> bool: + return exc.errno == errno.EPIPE + + def write(self, data): + try: + return self._wrapped.write(data) + except OSError as exc: + if not self._is_epipe(exc): + raise + self.sigpipe = True + return len(data) + + def flush(self): + try: + self._wrapped.flush() + except OSError as exc: + if not self._is_epipe(exc): + raise + self.sigpipe = True + + def __getattr__(self, attr): + return getattr(self._wrapped, attr) + + +def _pacify_real_fd(stream) -> None: + """Redirect a broken stream's real file descriptor to `os.devnull`. + + Swallowing the write in `_BrokenPipeSwallow` is not enough on its own: + CPython still flushes the *real* underlying stream automatically at + interpreter shutdown, by code this module does not control, and that + flush hits the same closed pipe - printing "Exception ignored while + flushing sys.stdout" (the well-known CPython caveat; see the standard + library docs' "Note on SIGPIPE"). Once a pipe is known broken there is + nothing left worth writing to it, so pointing the fd at `/dev/null` + makes every later flush - ours or the interpreter's own - a normal + write that always succeeds. + """ + try: + devnull = os.open(os.devnull, os.O_WRONLY) + try: + os.dup2(devnull, stream.fileno()) + finally: + os.close(devnull) + except (OSError, AttributeError): + # AttributeError: a stream with no real fd at all (a test double, or + # a harness that already replaced sys.stdout with something that + # isn't a file) - nothing to redirect, same as the OSError case. + pass + app = typer.Typer( help="wikitool - deterministic operations for Chemenu (see AGENTS.md).", no_args_is_help=True, @@ -127,6 +205,10 @@ def _run_traced(command: str, args: list[str], charged: bool = False) -> None: """ started = time.monotonic() exit_code = 0 + real_stdout, real_stderr = sys.stdout, sys.stderr + stdout_wrap = _BrokenPipeSwallow(real_stdout) + stderr_wrap = _BrokenPipeSwallow(real_stderr) + sys.stdout, sys.stderr = stdout_wrap, stderr_wrap try: app() except SystemExit as exc: @@ -137,18 +219,22 @@ def _run_traced(command: str, args: list[str], charged: bool = False) -> None: exit_code = 1 raise finally: + if stdout_wrap.sigpipe: + _pacify_real_fd(real_stdout) + if stderr_wrap.sigpipe: + _pacify_real_fd(real_stderr) + sys.stdout, sys.stderr = real_stdout, real_stderr if charged and _util.declined(): run_budget.refund() - emit( - "wikitool", - "wikitool.call", - { - "command": command, - "args": args, - "exit_code": exit_code, - "duration_ms": round((time.monotonic() - started) * 1000, 1), - }, - ) + attrs = { + "command": command, + "args": args, + "exit_code": exit_code, + "duration_ms": round((time.monotonic() - started) * 1000, 1), + } + if stdout_wrap.sigpipe or stderr_wrap.sigpipe: + attrs["stdout_truncated"] = True + emit("wikitool", "wikitool.call", attrs) if __name__ == "__main__": diff --git a/tools/chemenu/commands/doctor.py b/tools/chemenu/commands/doctor.py index 30cbc0c..3e43762 100644 --- a/tools/chemenu/commands/doctor.py +++ b/tools/chemenu/commands/doctor.py @@ -24,6 +24,7 @@ from chemenu import config, conventions, kb_collections, version as version_mod from chemenu.commands import git_publish, instructions_cmd from chemenu.commands._util import rel_path from chemenu.session import ENV_VAR as SESSION_ENV_VAR +from chemenu.session import session_id_source as _session_id_source console = Console() @@ -414,12 +415,23 @@ def check_upload_intake() -> Check: def check_session_id() -> Check: + """Three-valued, not two: an explicit `WIKITOOL_SESSION_ID` and a + recognised harness variable (see `chemenu.session.HARNESS_ENV_VARS`) both + keep a session's calls in one telemetry/budget bucket, so both are `OK`. + Only the `getppid()` fallback - a fresh "session" on every call, on a + harness that runs each tool call in its own shell - is a `WARN` (see + Gitea #110).""" import os if os.environ.get(SESSION_ENV_VAR, "").strip(): return Check("session-id", "OK", f"{SESSION_ENV_VAR}={os.environ[SESSION_ENV_VAR]}") + source = _session_id_source() + if source != "getppid() fallback": + return Check("session-id", "OK", f"scoped by harness variable {source}") return Check( - "session-id", "WARN", f"{SESSION_ENV_VAR} is not set - budget falls back to the parent PID", + "session-id", "WARN", + f"{SESSION_ENV_VAR} is not set and no harness session variable was found - " + "budget falls back to the parent PID", "See instructions/session-setup.md", ) diff --git a/tools/chemenu/commands/run_budget.py b/tools/chemenu/commands/run_budget.py index 16f64c3..fe705df 100644 --- a/tools/chemenu/commands/run_budget.py +++ b/tools/chemenu/commands/run_budget.py @@ -144,6 +144,27 @@ def _session_id_source() -> str: return _shared_session_id_source() +def _entry_for(state: dict, session_id: str) -> dict: + """The state entry for this session id, starting a fresh counter if the + same id string now carries a different origin than the one that wrote it. + + Two different id spaces (a `getppid()` integer, a harness UUID, an + explicit `WIKITOOL_SESSION_ID`) are vanishingly unlikely to collide as + strings - but "unlikely" is not "impossible", and inheriting a stranger's + count on collision is exactly the silent mis-key #110 exists to close. + An entry written before this field existed carries no `source` at all and + is treated as compatible: it keeps its count rather than being reset the + first time this ships, which would throw away real, in-flight state. + """ + source = _session_id_source() + entry = state.get(session_id) + if entry is None or (entry.get("source") is not None and entry["source"] != source): + entry = {"count": 0, "recent": []} + state[session_id] = entry + entry.setdefault("source", source) + return entry + + def _load_state() -> dict: if not STATE_FILE.exists(): return {} @@ -247,7 +268,7 @@ def record_and_check( with _state_lock(): session_id = _session_id() state = _load_state() - entry = state.setdefault(session_id, {"count": 0, "recent": []}) + entry = _entry_for(state, session_id) recent = entry["recent"] call_signature = f"{command} {' '.join(args)}".strip() diff --git a/tools/chemenu/session.py b/tools/chemenu/session.py index 1673cc3..bb75539 100644 --- a/tools/chemenu/session.py +++ b/tools/chemenu/session.py @@ -4,10 +4,35 @@ One definition, because the two must agree: if telemetry grouped events differently from the way the budget counts calls, a trace could not be read against the gate that refused it. -A "session" is approximated by the parent process of this CLI invocation - the -agent's shell - unless the caller sets `WIKITOOL_SESSION_ID`. Skills set it -explicitly so a session is scoped to a task rather than to a terminal window -(see instructions/session-setup.md). +Three-step fallback chain, in order: + +1. `WIKITOOL_SESSION_ID`, if the caller set one explicitly. Skills set it so a + session is scoped to a task rather than to a terminal window (see + instructions/session-setup.md). +2. A harness's own session variable, from `HARNESS_ENV_VARS` below - checked + only when nothing set the variable above. +3. `os.getppid()` - the parent process of this CLI invocation. On a harness + that runs every tool call in a freshly initialised shell (Claude Code's + Bash tool does), this is a new "session" per call and neither the + iteration-budget gate's ceiling nor its loop-breaker can ever trip - see + Gitea #110, which measured a 33-call run splitting into 21 telemetry + buckets under this fallback alone. + +Step 2 is what closes that gap without asking every skill to `export` a +variable a harness already re-derives per call: `CLAUDE_CODE_SESSION_ID` is +stable across a Claude Code session's tool calls (verified 2026-09-16, +against a live session, across separate Bash invocations - the shell's own +PID changed on every call, this variable did not) and is **exactly** the id +the `UserPromptSubmit` hook writes into a trace's `session.start` and +`prompt.submitted` events. Using it unmodified as the budget/telemetry key - +no prefix, no rewriting - is what lets the hook's events and this module's +events land in the same bucket. + +`HARNESS_ENV_VARS` only ever grows by a verified entry: a variable a real +session was observed setting, confirmed to be the same id a harness's own +hooks use elsewhere in a trace. A guessed name that happens to exist and +means something else would be worse than the `getppid()` fallback it would +replace - it would look like a fix and quietly mis-key a session instead. """ from __future__ import annotations @@ -16,15 +41,46 @@ import re ENV_VAR = "WIKITOOL_SESSION_ID" +HARNESS_ENV_VARS: tuple[tuple[str, str], ...] = ( + ("CLAUDE_CODE_SESSION_ID", "claude-code"), +) + _UNSAFE = re.compile(r"[^A-Za-z0-9._-]+") +def _harness_session() -> tuple[str, str] | None: + """The first harness variable that is actually set, as `(value, harness)`.""" + for var, harness in HARNESS_ENV_VARS: + value = os.environ.get(var) + if value: + return value, harness + return None + + def session_id() -> str: - return os.environ.get(ENV_VAR) or str(os.getppid()) + explicit = os.environ.get(ENV_VAR) + if explicit: + return explicit + harness = _harness_session() + if harness: + return harness[0] + return str(os.getppid()) def session_id_source() -> str: - return ENV_VAR if os.environ.get(ENV_VAR) else "getppid() fallback" + """Where the id in `session_id()` came from - `ENV_VAR`, a harness + variable name (with the harness named alongside it), or the `getppid()` + fallback. `doctor`, `budget status` and the `wikitool` source's + `session.start` event all read this so a session - or a trace - can say + what it was keyed on, not just what the id happened to be.""" + if os.environ.get(ENV_VAR): + return ENV_VAR + harness = _harness_session() + if harness: + _, name = harness + var = next(v for v, h in HARNESS_ENV_VARS if h == name) + return f"{var} ({name})" + return "getppid() fallback" def session_slug(value: str | None = None) -> str: diff --git a/tools/chemenu/telemetry/writer.py b/tools/chemenu/telemetry/writer.py index cf11a17..42bb61e 100644 --- a/tools/chemenu/telemetry/writer.py +++ b/tools/chemenu/telemetry/writer.py @@ -30,6 +30,7 @@ from pathlib import Path from chemenu import config from chemenu.session import session_id as current_session_id +from chemenu.session import session_id_source as current_session_id_source from chemenu.session import session_slug from chemenu.telemetry import policy, schema, scrub @@ -155,14 +156,22 @@ def _seed_session_header(target: Path, source: str, session: str) -> None: except FileExistsError: return with handle: + attrs = { + "harness": source, + "completeness": list(schema.HARNESS_CAPABILITIES.get(source, ())), + "synthesized": True, + } + # Only the `wikitool` source resolves its own session id through + # chemenu.session's fallback chain - every other source hands `emit()` + # an id its own hook payload already carried. Naming the chain's + # outcome here is what lets a trace say what it was keyed on, not just + # what the id happened to be (Gitea #110). + if source == "wikitool": + attrs["session_origin"] = current_session_id_source() header = schema.make_event( source, "session.start", - { - "harness": source, - "completeness": list(schema.HARNESS_CAPABILITIES.get(source, ())), - "synthesized": True, - }, + attrs, session_id=session, seq=_next_seq(), ) diff --git a/tools/chemenu/tests/conftest.py b/tools/chemenu/tests/conftest.py index 2f86d27..3846273 100644 --- a/tools/chemenu/tests/conftest.py +++ b/tools/chemenu/tests/conftest.py @@ -6,6 +6,7 @@ import pytest from chemenu import config, conventions from chemenu.frontmatter_io import write_page +from chemenu.session import HARNESS_ENV_VARS from chemenu.telemetry import policy as telemetry_policy from chemenu.type_resolver import resolver @@ -13,6 +14,13 @@ from chemenu.type_resolver import resolver # that a test which needs one sets it itself and the rest run against the # tool's own defaults. `WIKI_TRACE_DIR` is deliberately absent: it is not a # leak but the redirect `isolated_trace_dir` installs one fixture below. +# +# The harness variables from `chemenu.session.HARNESS_ENV_VARS` are pulled in +# here rather than duplicated: this suite runs *inside* Claude Code, so +# `CLAUDE_CODE_SESSION_ID` is genuinely set in the real environment - without +# clearing it, every session-fallback test would silently pick up this +# session's real id instead of exercising the fallback it means to test +# (Gitea #110). _WIKITOOL_ENV = ( "WIKI_AUTHOR", "WIKI_TRACE", @@ -24,7 +32,7 @@ _WIKITOOL_ENV = ( "WIKITOOL_UPDATE_URL", "WIKITOOL_UPDATE_TOKEN", "CHEMENU_ROOT", -) +) + tuple(var for var, _harness in HARNESS_ENV_VARS) # Environment git reads for identity or for where its repo lives. A stray # `GIT_DIR` would point every fixture repo at the developer's checkout; the diff --git a/tools/chemenu/tests/test_cli.py b/tools/chemenu/tests/test_cli.py new file mode 100644 index 0000000..c4a38f5 --- /dev/null +++ b/tools/chemenu/tests/test_cli.py @@ -0,0 +1,167 @@ +"""The CLI dispatch wrapper: the budget/trace hook every command runs +through (`cli.main`/`cli._run_traced`), and the broken-pipe handling that +sits alongside it. + +Gitea #110's SIGPIPE side finding: a successful call whose output is cut off +by a closed pipe (`wikitool types describe source | head -1`) used to record +`exit_code: 1` in the trace - indistinguishable from a real tool failure, and +reproduced verbatim by the very next, unpiped retry of the same command +showing `exit_code: 0`. `cli._BrokenPipeSwallow` and `cli._pacify_real_fd` +exist to close that gap; these tests exercise them without depending on a +real OS pipe or Click's own internal handling, which is exactly the moving +part being routed around. +""" +import errno +import json +import sys + +import pytest + +from chemenu import cli + + +def read_lines(path): + return [json.loads(line) for line in path.read_text(encoding="utf-8").splitlines()] + + +class _FailingStream: + """Raises EPIPE on the `fail_on`-th write - a fake stream standing in for + a real pipe whose reader has already closed.""" + + def __init__(self, fail_on=1): + self.fail_on = fail_on + self.calls = 0 + self.written = [] + self.flushed = False + + def write(self, data): + self.calls += 1 + if self.calls >= self.fail_on: + raise OSError(errno.EPIPE, "Broken pipe") + self.written.append(data) + return len(data) + + def flush(self): + self.flushed = True + + def isatty(self): + return False + + +# --- _BrokenPipeSwallow --- + +def test_broken_pipe_swallow_absorbs_epipe_on_write(): + swallow = cli._BrokenPipeSwallow(_FailingStream(fail_on=1)) + n = swallow.write("hello") + assert n == len("hello") + assert swallow.sigpipe is True + + +def test_broken_pipe_swallow_absorbs_epipe_on_flush(): + class _FlushFails: + def flush(self): + raise OSError(errno.EPIPE, "Broken pipe") + + swallow = cli._BrokenPipeSwallow(_FlushFails()) + swallow.flush() # does not raise + assert swallow.sigpipe is True + + +def test_broken_pipe_swallow_reraises_unrelated_oserrors(): + class _Explodes: + def write(self, data): + raise OSError(errno.ENOSPC, "No space left on device") + + swallow = cli._BrokenPipeSwallow(_Explodes()) + with pytest.raises(OSError): + swallow.write("x") + assert swallow.sigpipe is False + + +def test_broken_pipe_swallow_passes_through_normal_writes(): + wrapped = _FailingStream(fail_on=99) + swallow = cli._BrokenPipeSwallow(wrapped) + swallow.write("hello") + assert wrapped.written == ["hello"] + assert swallow.sigpipe is False + + +def test_broken_pipe_swallow_proxies_unknown_attributes(): + wrapped = _FailingStream() + swallow = cli._BrokenPipeSwallow(wrapped) + assert swallow.isatty() is False + + +# --- _pacify_real_fd --- + +def test_pacify_real_fd_is_a_best_effort_noop_without_a_real_descriptor(): + class _RaisesOSError: + def fileno(self): + raise OSError("not a real fd in this test") + + class _HasNoFilenoAtAll: + pass + + cli._pacify_real_fd(_RaisesOSError()) # must not raise + cli._pacify_real_fd(_HasNoFilenoAtAll()) # must not raise either + + +# --- _run_traced: the trace records what actually happened --- + +def test_a_write_cut_off_by_a_closed_pipe_is_not_recorded_as_an_error(monkeypatch, tmp_path): + monkeypatch.setenv("WIKI_TRACE_DIR", str(tmp_path)) + monkeypatch.setenv("WIKITOOL_SESSION_ID", "sigpipe-unit") + stub = _FailingStream(fail_on=2) # first write succeeds, second hits EPIPE + monkeypatch.setattr(sys, "stdout", stub) + + def fake_app(): + sys.stdout.write("line one\n") + sys.stdout.write("line two\n") # truncated here, like `| head -1` + raise SystemExit(0) + + monkeypatch.setattr(cli, "app", fake_app) + + with pytest.raises(SystemExit) as exc: + cli._run_traced("types", ["describe", "source"]) + assert exc.value.code == 0 + + records = read_lines(tmp_path / "sigpipe-unit" / "trace.jsonl") + call = next(r for r in records if r["event"] == "wikitool.call") + assert call["attrs"]["exit_code"] == 0 + assert call["attrs"]["stdout_truncated"] is True + + +def test_a_real_failure_is_still_recorded_as_one(monkeypatch, tmp_path): + """The unrelated-error path stays exactly as before: an actual failure + keeps its exit code and carries no truncation flag.""" + monkeypatch.setenv("WIKI_TRACE_DIR", str(tmp_path)) + monkeypatch.setenv("WIKITOOL_SESSION_ID", "real-failure-unit") + + def fake_app(): + raise SystemExit(1) + + monkeypatch.setattr(cli, "app", fake_app) + + with pytest.raises(SystemExit) as exc: + cli._run_traced("new", ["entity", "--name", ""]) + assert exc.value.code == 1 + + records = read_lines(tmp_path / "real-failure-unit" / "trace.jsonl") + call = next(r for r in records if r["event"] == "wikitool.call") + assert call["attrs"]["exit_code"] == 1 + assert "stdout_truncated" not in call["attrs"] + + +def test_an_ordinary_call_restores_the_real_streams_afterwards(monkeypatch, tmp_path): + monkeypatch.setenv("WIKI_TRACE_DIR", str(tmp_path)) + monkeypatch.setenv("WIKITOOL_SESSION_ID", "restore-unit") + real_stdout, real_stderr = sys.stdout, sys.stderr + + def fake_app(): + raise SystemExit(0) + + monkeypatch.setattr(cli, "app", fake_app) + with pytest.raises(SystemExit): + cli._run_traced("lint", []) + assert sys.stdout is real_stdout + assert sys.stderr is real_stderr diff --git a/tools/chemenu/tests/test_run_budget.py b/tools/chemenu/tests/test_run_budget.py index f51ef87..4ee837d 100644 --- a/tools/chemenu/tests/test_run_budget.py +++ b/tools/chemenu/tests/test_run_budget.py @@ -1,8 +1,12 @@ import json +import os +import subprocess +import sys import pytest import typer +from chemenu import config from chemenu.commands import run_budget @@ -137,6 +141,39 @@ def test_override_bypasses_both_gates(): assert state["test-session"]["count"] == 40 +def test_a_bucket_without_a_recorded_origin_keeps_its_count(): + """Grandfathering (#110 decision 1): an entry written before this field + existed must not be reset the moment this ships - that would throw away + real, in-flight state on every existing instance's first call after + upgrading.""" + run_budget._save_state({"test-session": {"count": 5, "recent": ["lint"]}}) + run_budget.record_and_check("lint", [], override=False) + entry = run_budget._load_state()["test-session"] + assert entry["count"] == 6 + assert entry["source"] == "WIKITOOL_SESSION_ID" + + +def test_a_bucket_with_a_different_recorded_origin_starts_over(): + """The same id string, stamped by a different origin than the one + recorded, is treated as a stranger's bucket rather than inherited - the + mechanism behind #110's 'no bucket is silently reinterpreted' criterion.""" + run_budget._save_state( + {"test-session": {"count": 40, "recent": ["lint"], "source": "getppid() fallback"}} + ) + run_budget.record_and_check("lint", [], override=False) + entry = run_budget._load_state()["test-session"] + assert entry["count"] == 1 + assert entry["source"] == "WIKITOOL_SESSION_ID" + + +def test_a_bucket_with_the_same_recorded_origin_keeps_counting(): + run_budget._save_state( + {"test-session": {"count": 7, "recent": ["lint"], "source": "WIKITOOL_SESSION_ID"}} + ) + run_budget.record_and_check("lint", [], override=False) + assert run_budget._load_state()["test-session"]["count"] == 8 + + def test_save_state_writes_atomically_and_leaves_no_tmp_file(isolated_state): run_budget._save_state({"test-session": {"count": 1, "recent": []}}) assert isolated_state.exists() @@ -228,3 +265,51 @@ def test_status_command_reports_count(capsys): run_budget.status_command() out = capsys.readouterr().out assert "Calls so far: 1" in out + + +# --- gate reproduced across separate processes (Gitea #110) --- +# +# `isolated_state`'s in-process monkeypatching cannot exercise the actual bug: +# `os.getppid()` is constant within one test process. These spawn a fresh +# Python subprocess per call - the same shape as Claude Code's Bash tool, +# which runs every `wikitool` invocation in a freshly initialised shell - so +# the parent pid really does differ call to call, and only a harness variable +# (standing in for `CLAUDE_CODE_SESSION_ID`) can hold the run together. +# Before the fallback chain existed, both tests below would be green *and* +# blind: every call landed in its own one-or-two-call bucket, and neither +# gate could ever see enough of one session to trip. + +def _spawn_call(tmp_path, monkeypatch, *, command="lint", args=(), override=False): + monkeypatch.setenv("CHEMENU_ROOT", str(tmp_path)) + monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "harness-run") + monkeypatch.delenv("WIKITOOL_SESSION_ID", raising=False) + code = ( + "from chemenu.commands import run_budget\n" + f"run_budget.record_and_check({command!r}, {list(args)!r}, override={override!r})\n" + ) + return subprocess.run( + [sys.executable, "-c", code], + cwd=config._PACKAGE_ROOT / "tools", + capture_output=True, text=True, + ) + + +def test_the_iteration_budget_gate_trips_across_separate_shells(tmp_path, monkeypatch): + for i in range(run_budget.DEFAULT_CALL_LIMIT): + result = _spawn_call(tmp_path, monkeypatch, args=[f"--pass-{i}"]) + assert result.returncode == 0, result.stdout + result.stderr + + result = _spawn_call(tmp_path, monkeypatch, args=["--one-too-many"]) + assert result.returncode != 0 + assert "Iteration Budget Gate" in result.stdout + + +def test_the_loop_breaker_trips_across_separate_shells(tmp_path, monkeypatch): + args = ["add", "--a", "X", "--b", "Y"] + for _ in range(run_budget.DEFAULT_LOOP_WINDOW): + result = _spawn_call(tmp_path, monkeypatch, command="xref", args=args) + assert result.returncode == 0, result.stdout + result.stderr + + result = _spawn_call(tmp_path, monkeypatch, command="xref", args=args) + assert result.returncode != 0 + assert "Loop-Breaker" in result.stdout diff --git a/tools/chemenu/tests/test_telemetry_emit.py b/tools/chemenu/tests/test_telemetry_emit.py index d152a37..839f8d2 100644 --- a/tools/chemenu/tests/test_telemetry_emit.py +++ b/tools/chemenu/tests/test_telemetry_emit.py @@ -145,6 +145,30 @@ def test_session_id_falls_back_to_the_parent_process(monkeypatch): assert "getppid" in session.session_id_source() +def test_session_id_prefers_a_harness_variable_over_getppid(monkeypatch): + """The middle link of the chain (Gitea #110): a harness that sets its own + session variable, but not WIKITOOL_SESSION_ID, still gets a stable id + rather than falling all the way to the per-call parent pid.""" + monkeypatch.delenv("WIKITOOL_SESSION_ID", raising=False) + monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "30d734d5-live-session") + assert session.session_id() == "30d734d5-live-session" + assert session.session_id_source() == "CLAUDE_CODE_SESSION_ID (claude-code)" + + +def test_explicit_session_id_still_wins_over_a_harness_variable(monkeypatch): + monkeypatch.setenv("WIKITOOL_SESSION_ID", "ingest-handbook/u2") + monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "30d734d5-live-session") + assert session.session_id() == "ingest-handbook/u2" + assert session.session_id_source() == "WIKITOOL_SESSION_ID" + + +def test_the_harness_id_is_used_unmodified_as_the_key(): + """Decision 1 in #110: no prefix, no rewriting - the value has to be + exactly what a harness's own hook writes into a trace, or the two would + stop joining on the same string.""" + assert session.HARNESS_ENV_VARS == (("CLAUDE_CODE_SESSION_ID", "claude-code"),) + + def test_the_core_event_set_is_what_every_harness_can_produce(): """Guards the degradation rule: if a core event stops being available on one harness, this fails rather than the scorer silently reporting zero.""" @@ -185,6 +209,25 @@ def test_a_reported_session_start_is_not_shadowed_by_a_synthetic_one(monkeypatch assert "synthesized" not in records[0]["attrs"] +def test_wikitool_s_own_header_names_where_its_session_id_came_from(monkeypatch, tmp_path): + """Only the `wikitool` source resolves its own id through the fallback + chain - naming that origin in its own header is what lets a trace say + what it was keyed on, not just what the id happened to be (Gitea #110).""" + monkeypatch.setenv("WIKI_TRACE_DIR", str(tmp_path)) + monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "harness-session") + emit_mod.write_event("wikitool", "wikitool.call", {"command": "lint"}, + session="harness-session") + header = read_lines(tmp_path / "harness-session" / "trace.jsonl")[0] + assert header["attrs"]["session_origin"] == "CLAUDE_CODE_SESSION_ID (claude-code)" + + +def test_another_source_s_header_carries_no_session_origin(monkeypatch, tmp_path): + monkeypatch.setenv("WIKI_TRACE_DIR", str(tmp_path)) + emit_mod.write_event("copilot-cli", "tool.post", {"tool_name": "bash"}, session="v4") + header = read_lines(tmp_path / "v4" / "trace.jsonl")[0] + assert "session_origin" not in header["attrs"] + + # --- reading back --- def test_a_trace_reads_back_in_time_order(monkeypatch, tmp_path):