Skip to content

fix(tests): update-command platform-gate tests spawn a real detached hermes update - #43298

Closed
eazye19 wants to merge 1 commit into
NousResearch:mainfrom
eazye19:claude/update-test-spawn-mock
Closed

eazye19 wants to merge 1 commit into
NousResearch:mainfrom
eazye19:claude/update-test-spawn-mock

Conversation

@eazye19

@eazye19 eazye19 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Four tests in tests/gateway/test_update_command.py (TestUpdateCommandPlatformGate: the DISCORD/MATTERMOST/HOMEASSISTANT registry-fallback tests and the TELEGRAM allowlist test) call _handle_update_command with no subprocess.Popen mock. When the gate passes, the handler spawns a REAL detached setsid hermes update --gateway against whatever checkout the tests run in.

On a detached-HEAD checkout (which is what actions/checkout produces for every pull_request event) the spawned update prints "switching to main for update", checks out main, and the restore step is skipped because current_branch is the literal string HEAD (hermes_cli/main.py, the if current_branch not in {branch, "HEAD"} guard). The working tree is silently left on main, so every test process that starts after the spawn runs against the wrong code. The failures look like unrelated flakes in whatever tests happen to run later.

This PR adds a class-scoped autouse fixture that mocks gateway.run._hermes_home, shutil.which, and subprocess.Popen for the platform-gate tests, using the same mocking idiom the file already uses in test_spawns_setsid. Pure addition, no test logic changed.

Related Issue

No existing issue found (searched PRs and issues for the test names and spawn behavior).

Type of Change

  • ✅ Tests (adding or improving test coverage)
  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tests/gateway/test_update_command.py: new block_real_update_spawn autouse fixture in TestUpdateCommandPlatformGate (+15 lines, addition only).

How to Test

  1. On a clean checkout: git checkout --detach HEAD~1 (simulates a PR-event checkout).
  2. pytest 'tests/gateway/test_update_command.py' -k mattermost -q (passes), then wait ~60s.
  3. git reflog -3 on current main shows checkout: moving from <sha> to main and the tree is now on main: the test spawned a real update. With this PR, step 3 shows no checkout and HEAD stays detached.
  4. pytest tests/gateway/test_update_command.py -q: 35 passed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the affected tests and all pass (35/35 in the file)
  • I've added tests for my changes (the fixture protects the existing tests)
  • I've tested on my platform: Ubuntu 24.04

Documentation & Housekeeping

  • Documentation: N/A (test-only)
  • cli-config.yaml.example: N/A
  • CONTRIBUTING/AGENTS: N/A
  • Cross-platform: N/A (test-only, mocks are platform-neutral)

…tform-gate tests

Four tests in tests/gateway/test_update_command.py call _handle_update_command
without mocking subprocess.Popen, spawning a real detached 'setsid hermes
update --gateway' against the running checkout. On detached-HEAD CI checkouts
(every pull_request event) the update switches the tree to origin/main and the
restore step is skipped when detached, silently poisoning every test process
that starts afterwards. Repro: detach HEAD, run one of these tests, watch the
reflog flip to main 60s later.
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery labels Jun 10, 2026
@kyssta-exe

Copy link
Copy Markdown
Contributor

Thanks for the PR! One observation:

PR description doesn't match the actual code changes

The PR description describes a massive cognitive skill category — neurochemical states, graph memory, signed personality kernels, daemons, etc. — but the diff only contains a 26-line test fixture change in tests/gateway/test_update_command.py (adding a block_real_update_spawn autouse fixture).

The actual cognitive skill implementation (the qca-cycle SKILL.md, kernel.ses.json, ses_bridge.py, embedding logic, etc.) doesn't appear in the diff. Could you check that the correct files were included in the PR? It's possible the branch was rebased or the working tree wasn't staged correctly.

If the intent is just to add the test fixture (which prevents a real hermes update --gateway from spawning during CI on detached-HEAD checkouts), the PR title and description should be updated to reflect that.

@eazye19

eazye19 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for taking a look! I think this observation landed on the wrong PR — the description here has only ever covered the block_real_update_spawn test fixture (GitHub shows no edits to the body), and the diff matches it: one file, +15 lines in tests/gateway/test_update_command.py. The cognitive-skill content you're describing (qca-cycle SKILL.md, kernel.ses.json, ses_bridge.py) is from #43306, which was opened around the same time. Happy to clarify anything about the fixture itself.

@AIalliAI

Copy link
Copy Markdown
Contributor

Independent confirmation with hard evidence — this just bit #44433's test (6) shard (pytest exit 4, file or directory not found on the PR-added tests/tools/test_web_keyless_default_fallback.py, forensics showing exists=False + git_dirty_entries=0).

Reproduced locally by running that shard's file list against a scratch checkout, with git shimmed to log caller + cwd:

  • the shim attributes real git diff --name-onlygit fetch origin mainrev-parsestatus --porcelainrev-list HEAD..origin/main --count sequences (the _cmd_update_impl preamble) to exactly the four TestUpdateCommandPlatformGate::test_allows_* tests;
  • the reflog of the repo backing the imported hermes_cli shows reset: moving to origin/main landing mid-run — the detached setsid hermes update --gateway survives the per-file pytest subprocess and mutates whatever checkout PROJECT_ROOT resolves to, up to a minute after the test file already reported green.

On a pull_request checkout (shallow, detached at the merge ref) the spawned update's pull --ff-only can't fast-forward, so it takes the git reset --hard origin/main fallback: PR-added files vanish (→ the exit-4 "file not found" on a clean tree), PR-modified files silently revert to main. That's also why the harness's June-2026 exit-4 forensics note says it recurs on one shard only — deterministic LPT slicing keeps test_update_command.py and its victims together.

The autouse-fixture approach here (patching subprocess.Popen + gateway.run._hermes_home + shutil.which) kills the escape at the right layer. Would be great to get this merged — every PR whose added test files land on that shard is currently a coin flip.

AIalliAI pushed a commit to AIalliAI/Hermes that referenced this pull request Jun 14, 2026
…tform-gate tests

Cherry-pick of upstream PR NousResearch#43298 (eazye19). The four
TestUpdateCommandPlatformGate gate-pass tests spawn a REAL detached
`setsid hermes update --gateway`; on CI's detached-HEAD checkout the
fallback `git reset --hard origin/main` deletes branch-added test files
minutes later, failing whichever shard is mid-collection (exit-4
"file or directory not found" flake). Carried here so the rollup CI
stops tripping on it; drop when NousResearch#43298 merges upstream.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AIalliAI added a commit to AIalliAI/Hermes that referenced this pull request Jun 14, 2026
check-attribution flagged support@captureclient.net, introduced by
cherry-picking PR NousResearch#43298 onto this branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation and removed P3 Low — cosmetic, nice to have labels Jun 27, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused report and the corroborating CI forensics in the discussion. This is an automated hermes-sweeper review; current main already provides the requested protection.

  • Commit c41a6534cf2240d69253ca748391c0d62d81c3b0 (fix(tests): mock subprocess.Popen in all _handle_update_command tests) added subprocess.Popen mocks across these tests.
  • The four gate-passing cases now patch subprocess.Popen at tests/gateway/test_update_command.py:450, :479, :505, and :523.
  • The actual detached spawn remains in gateway/slash_commands.py:4636 / :4644, so those mocks prevent the checkout-mutating update process described here.

The equivalent protection shipped in v2026.6.19, so this fixture-only PR is redundant.

@teknium1 teknium1 closed this Jul 14, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants