🧪 test(kanban): isolate HERMES_KANBAN_ORIGIN in the hermetic env fixture - #125
Conversation
cwest
left a comment
There was a problem hiding this comment.
The conftest fix is right, and it holds up under test. Against the pre-fix tree with HERMES_KANBAN_ORIGIN set, the exact 8 named tests fail (126 pass); with the scrub added they go green (134 pass). The diagnosis is accurate too: this is a harness isolation gap, not a runtime bug, and the introducing commit (550e258, #55, which added capture_kanban_origin_from_session) checks out. No assertion was weakened and no test was skipped to reach green. If this PR only carried that one change, it would be done.
It doesn't. The diff against cwest/integration is 9 files, +319/-389, not the "1 file changed, 8 insertions" the body claims. Four commits unrelated to the card ride along:
- eb63bdb derive worktree branch names (kanban_db.py + new test)
- 0784ef1 route a review handoff to the reviewer (kanban_db.py + 2 new tests)
- 3c752d4 keep an acceptance park sticky (kanban_db.py + new test)
- eccbae1 Revert #114 (agent:end hook) — strips gateway/run.py and gateway/hooks.py logic and deletes the 264-line tests/gateway/test_agent_end_hook_decisions.py
The revert is the one that has to come out of this PR. #114 is live on origin/main (b7514ce); reverting a shipped feature is a real decision that deserves its own card and its own review, not a passenger on a test-isolation change. Merging this as described would quietly undo #114 for anyone reading the body and expecting a one-line conftest fix. The three kanban_db.py fixes are less alarming but still outside the card's scope and unmentioned in the body.
To land: reduce this PR to just the conftest commit (f8b26b779), or — if these commits genuinely belong on the integration branch — rewrite the body to declare and justify all five, and split the #114 revert into its own reviewed card. As written, the stated scope and the actual scope don't match, and the mismatch hides a feature revert.
Checks are still pending; mergeable is MERGEABLE, mergeStateStatus UNSTABLE (checks in flight).
The 8 report-back subscription tests in tests/tools/test_kanban_tools.py failed under bare pytest (126 passed, 8 failed) while passing under the CI wrapper. Root cause is a test-isolation gap, not a runtime bug. _maybe_auto_subscribe resolves the card origin from the HERMES_KANBAN_ORIGIN env mirror (gateway/session_context, added when the create path learned to inherit a card origin across the spawn boundary) BEFORE it consults the HERMES_SESSION_* vars. That precedence is intentional. The conftest _hermetic_environment fixture scrubs every other HERMES_KANBAN_* pin and the HERMES_SESSION_* vars, but never added HERMES_KANBAN_ORIGIN. When pytest runs inside a dispatched worker, that var is inherited from the parent with a foreign owner pid, so the origin channel leaks into every test: cards auto-subscribe even after a test clears the session vars, flipping subscribed to True. The CI wrapper spawns a clean env so the leak never appears there; bare pytest in an inherited env has only the conftest scrub as its defense, and the scrub was missing this var. Add HERMES_KANBAN_ORIGIN to _HERMES_BEHAVIORAL_VARS alongside the other kanban pins so subscription tests see a truly detached context. Witnessed RED (8 failed) against the pre-change fixture with the var set, GREEN (134 passed) after.
f8b26b7 to
6a87e2a
Compare
cwest
left a comment
There was a problem hiding this comment.
This round resolves the scope concern cleanly. The branch is now a single commit against the base — tests/conftest.py, +8/-0 — with none of the unrelated changes that rode along before (the worktree-naming, review-handoff, acceptance-park, and agent:end-revert commits are all gone). The body's "1 file changed" claim now matches the diff.
The fix itself is correct and load-bearing. Adding HERMES_KANBAN_ORIGIN to the hermetic-env scrub list closes a real isolation gap: _maybe_auto_subscribe resolves the card origin from that env mirror (gateway/session_context._KANBAN_ORIGIN_ENV) before the HERMES_SESSION_* vars, so under bare pytest in an inherited env the origin leaked into every test and flipped subscribed to True. Reproduced independently against the pre-fix fixture with the var set — the exact 8 named tests fail (8 failed, 126 passed) — and the added scrub returns the file to 134 passed. The explanatory comment accurately names the symbol and the leak mechanism.
Diagnosis holds on all three card questions: fork-only (not upstream), tests drifted rather than a runtime break, introduced by the origin-inheritance change. No test was deleted or skipped and no assertion was weakened — only the harness isolation was completed.
Checks are green and the PR is mergeable. No changes needed.
Summary
tests/tools/test_kanban_tools.pyhad 8 report-back subscription testsfailing under bare
.venv/bin/pytest(126 passed, 8 failed) while passingunder the CI wrapper. This is a test-isolation gap, not a runtime bug —
the implementation is correct; the hermetic-env fixture was missing one scrub.
Fix: add
HERMES_KANBAN_ORIGINto_HERMES_BEHAVIORAL_VARSintests/conftest.py, alongside every otherHERMES_KANBAN_*pin and theHERMES_SESSION_*vars it already scrubs.Root cause
_maybe_auto_subscribe(tools/kanban_tools.py) resolves the card origin viacapture_kanban_origin_from_session()→get_kanban_origin(), which reads theHERMES_KANBAN_ORIGINos.environ mirror before falling back to theHERMES_SESSION_*env vars. That precedence is deliberate: an inherited originmust win across a spawn boundary so a detached worker's card wakes the human
origin, not the detached run.
The 8 tests clear
HERMES_SESSION_*(via_clear_session_env/ explicitmonkeypatch.delenv) to simulate a webhook/CLI/TUI context, but nothing clearedHERMES_KANBAN_ORIGIN. When the suite runs inside a dispatched worker, that varis inherited from the parent with a foreign owner pid, so
get_kanban_origintreats it as a legitimate cross-boundary inheritance and returns a real origin —
every card auto-subscribes, and
subscribedcomes backTruewhere the testsexpect
False(or with the wrong platform/chat for the gateway/TUI cases).Witnessed:
HERMES_KANBAN_ORIGINset): 8 failed, 126 passed — the exact 8 named..venv/bin/pytest tests/tools/test_kanban_tools.py→ 134 passed.The CI wrapper spawns a clean env, so the leak never surfaces there; bare pytest
in an inherited env has only the conftest scrub as its defense, and the scrub was
missing this one var.
Card questions
Fail on origin/main too, or only on the integration branch?
Only on the integration branch. origin/main has neither the
capture_kanban_origin_from_sessionpre-check nor the introducing commit(
550e25861, PR ✨ feat(kanban): inherit + reassign card origin across the spawn boundary #55) — the inherit-origin-across-spawn-boundary feature isfork-only. With no origin-precedence code on main, there is no env mirror to
leak, so these tests cannot fail on main the same way. This is fork drift, not
an upstream bug.
Actually broken at runtime, or did the tests drift?
Not broken at runtime. The origin-precedence behavior is intentional and
correct — a dispatched worker's card should subscribe the inherited human
origin. The tests drifted: their fixtures isolate the session env channel
but were never updated to also isolate the new
HERMES_KANBAN_ORIGINchannelthat ✨ feat(kanban): inherit + reassign card origin across the spawn boundary #55 introduced. No production assertion was weakened; the test
expectations are unchanged. Only the harness's env isolation was completed.
Which commit introduced the divergence?
550e25861—feat(kanban): inherit + reassign card origin across the spawn boundary (#55). It addedcapture_kanban_origin_from_session()as the firstresolution step in
_maybe_auto_subscribeand theHERMES_KANBAN_ORIGINenvmirror in
gateway/session_context.py, but did not add that var to theconftest scrub list. Found via
git log -S capture_kanban_origin_from_session.Test / verification
.venv/bin/pytest tests/tools/test_kanban_tools.py→ 134 passed (bare pytest, withHERMES_KANBAN_ORIGINset — the live condition that produced the 8 failures).scripts/run_tests.sh tests/tools/test_kanban_tools.py …→ green under the CI wrapper.test_kanban_origin_*tests (which explicitlysetenvthe var) still pass — scrub-then-set is the documented pattern.tests/tools/ tests/gateway/residual failures are pre-existing on the untouched base (confirmed with the change stashed: identical failures), machine-specific, and unrelated to this change.Scope
This PR contains exactly one commit against
cwest/integration: thetests/conftest.pyscrub (1 file changed, +8/-0). The branch was rebased ontocwest/integrationto drop four commits that had ridden along from theworktree's base and were unrelated to this card (worktree branch naming, review
handoff routing, acceptance-park stickiness, and a revert of the agent:end hook).
Those changes belong on their own cards and are not part of this fix. Head SHA
after rebase:
6a87e2a0.