fix(kanban): stamp session provenance on comments written from execute_code - #556
Merged
Merged
Conversation
…e_code Comment provenance (PR #545) shipped correct and attributed 0 of 723 live comments. The resolver, the schema, the render path and the 41 tests were all fine; the value simply never reached the process that writes the row. Measured, not inferred. The in-gateway `kanban_comment` tool path ALREADY stamped session_ref correctly — driving it with two bound sessions produced two distinct fingerprints on a sandbox board. The failing population was written a different way: the orchestrator shells out to `hermes kanban comment` from inside `execute_code`, and `_scrub_child_env`'s allowlist dropped HERMES_SESSION_ID on the way into the sandbox child. Same turn, two surfaces, opposite outcomes — in-process tool attributed, sandbox script NULL. Live rows confirm it: every attributed comment on the board came from a dispatcher-spawned worker (whose own os.environ carries the id), every NULL one from a gateway session commenting via execute_code. This corrects the root cause recorded on card t_32a7f736, which said nothing sets HERMES_SESSION_ID anywhere. The gateway does bind it — as a contextvar, which is the only correct source there, since the os.environ mirror is last-writer-wins across concurrent sessions. - gateway/session_context.py: extract `resolve_current_session_id` — the one contextvar-first resolver, with the _HERMES_GATEWAY-gated empty-contextvar rules that were previously private to kanban_tools. Two consumers now share it instead of keeping copies that can drift. - tools/kanban_tools.py: `_current_session_id` becomes a thin alias to it. - tools/code_execution_tool.py: bridge the resolved id into the sandbox child env, on BOTH spawn paths (local dict env, remote shell prefix, shell-quoted). Deliberately NOT via `_HERMES_CHILD_ALLOWED`: an exact-name allowlist copies from os.environ and would attribute a sandbox to whichever concurrent session wrote the global last. Absent-when-unresolvable rather than inherited, matching the terminal path's `_inject_session_context_env` leak policy. - hermes_cli/kanban.py: write down the CLI decision the card asked for — inside a session the bridge supplies the id; in a bare human shell the row stays NULL and renders "(provenance unknown)". No synthesized per-invocation uuid, which would make one operator look like N sessions. Verified: - New EFFECT test spawns a real child process, runs the real CLI against a real sqlite board, and asserts on the persisted ROWS — two concurrent same-profile sessions yield two DISTINCT non-null session_ref. A resolver unit test is exactly the evidence that let this ship inert, so it is not the gate. - Fail-open preserved: no session id still writes the comment, NULL provenance, legacy "(provenance unknown)" render asserted. - Mutation-proven, 3 mutants, all killed, each verified non-inert via cmp: (1) drop the bridge -> AC test RED with [None, None], the exact live symptom; (2) the naive allowlist "fix" -> 5 RED, incl. both gateway-concurrency tests; (3) resolver reads os.environ -> 5 RED. Control green before and after. - scripts/run_tests.sh over the 9 directly-affected files: 178 passed, 0 failed, including tests/gateway/test_no_gateway_session_env_writes.py (the enforcement test that forbids per-session os.environ writes from gateway-reachable code). Not yet verified: live effect on the board. This only takes hold for sessions started after a gateway restart, which is Ace's call, not a worker's.
Kyzcreig
force-pushed
the
fix/execute-code-session-id-provenance
branch
from
August 10, 2026 07:35
6bcf959 to
c5fb4b3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
Comment provenance (#545) shipped correct and attributed 0 of 723 live comments. The resolver, schema, render path and 41 tests were all fine. The value simply never reached the process that writes the row.
Root cause — measured, and NOT what the card said
Card
t_32a7f736recorded the cause as "nothing sets HERMES_SESSION_ID". That premise is falsified. Correcting it here because a PR that ships with a wrong root cause teaches the wrong lesson:kanban_commenttool path already worked. Driving_handle_commentwith two bound sessions on a sandbox board produced two distinct fingerprints (c465b237107a/967c8638e8b6).os.environis last-writer-wins across concurrent sessions (the v3-latch class), which is exactly whyHERMES_SESSION_IDis absent from the gateway's process env. That absence is a feature, and reading it as "nothing sets it" is what misdirected the card.The failing population was written a different way: the orchestrator shells out to
hermes kanban commentfrom insideexecute_code, and_scrub_child_env's allowlist droppedHERMES_SESSION_IDon the way into the sandbox child. Same turn, two surfaces, opposite outcomes.The live board corroborates it exactly — every attributed comment came from a dispatcher-spawned worker (single process, its own
os.environcarries the id); every NULL one from a gateway session commenting viaexecute_code.website/docs/reference/environment-variables.md:866already documentedHERMES_SESSION_IDas "exported automatically into every tool subprocess (terminal, execute_code, ...)". The docs were right; the code didn't do it.The change
gateway/session_context.py— extractresolve_current_session_id: one contextvar-first resolver carrying the_HERMES_GATEWAY-gated empty-contextvar rules that were private tokanban_tools. Two consumers share it rather than keeping copies that drift.tools/kanban_tools.py—_current_session_idbecomes a thin alias. No behaviour change.tools/code_execution_tool.py— bridge the resolved id into the sandbox child on both spawn paths (local dict env; remote shell prefix,shlex.quoted like TZ).hermes_cli/kanban.py— write down the CLI decision the card asked for.Why not just add it to
_HERMES_CHILD_ALLOWEDThat one-liner is the obvious fix and it is wrong: an exact-name allowlist copies from
os.environ, so inside the gateway a sandbox gets attributed to whichever concurrent session wrote the global most recently — a misattributed comment, which is worse than an unattributed one. It is shipped as mutant #2 below and 5 tests kill it.CLI policy (DO #2), decided explicitly
Inside a session the bridge supplies the id and the row is attributed like any tool call. In a bare human shell there is no session, the comment still writes, the row stays NULL and renders
author (provenance unknown). Deliberately no synthesized per-invocation uuid — that would make one operator look like N sessions, the same ambiguity inverted.Verification
The acceptance test is an EFFECT test. It spawns a real child process, runs the real CLI against a real sqlite board, and asserts on the persisted rows. A resolver unit test is precisely the evidence that let this ship inert, so it is not the gate.
Mutation-proven — 3 mutants, all killed, each
cmp-verified non-inert (control green before and after):[None, None], the exact live symptom_HERMES_CHILD_ALLOWED"fix"os.environscripts/run_tests.sh(canonical per-file runner) over the 9 directly-affected files: 178 passed, 0 failed — includingtests/gateway/test_no_gateway_session_env_writes.py, the enforcement test forbidding per-sessionos.environwrites from gateway-reachable code.Two
hermes_clikanban files exit non-zero on asys_modules_leak_gateteardown error (all assertions pass). Inherited — reproduced identically on an unmodified worktree at the same baseefd03b48.Not verified
Live effect on the board. This only takes hold for sessions started after a gateway restart — Ace's call, not a worker's.