chore(tests): baseline pre-existing sys.modules pollution in test_cleanup_unreachable_orphan.py - #796
Merged
Conversation
…leanup_unreachable_orphan.py #791 (which added the tests/lint_sys_modules.py lint and its baseline) landed AFTER #783 (which added tests/unit/test_cleanup_unreachable_orphan.py). The baseline was generated without including the orphan-test file's 3 top-level `sys.modules[...] = stub` assignments at lines 64, 76, 79 (docker, services.docker_service, database stubs installed before the test module's services.cleanup_service import). Result: every PR opened against dev fails the lint job on these 3 lines, even when the PR's own changes are clean. Fix: grandfather the file into the baseline at its current count (3). This matches how the same shape is handled elsewhere — e.g. test_watchdog_unit.py (3 violations, the precedent the orphan-test file's own comment cites). The lint script's baseline path is the intended mechanism for accepting pre-existing module-level stub installs that cannot use monkeypatch. Verified locally: python3 tests/lint_sys_modules.py emits zero violations in the real test tree (only .venv site-packages remain, which CI runners don't have). Refs #762, #783, #791 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
vybe
approved these changes
May 12, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
LGTM — correct one-line baseline fix, matches existing grandfathering pattern for test_watchdog_unit.py. Merging.
This was referenced May 12, 2026
dolho
added a commit
that referenced
this pull request
May 13, 2026
…form_default_model The lint job on #443's PR went red on a file that is NOT in the PR's diff — `tests/test_platform_default_model.py` carries 6 bare `sys.modules` mutations that were merged to dev without an entry in `tests/lint_sys_modules_baseline.txt`. Same #802 rebase-race shape as the original #791 vs #783 incident: baseline-introducing PR (#791) and violation-introducing PR landed disjointly, both green pre-merge, dev went red post-merge, the next PR off dev (this one) inherits it. Ratcheting the baseline here unblocks #443. Same precedent as #796 which baselined `test_cleanup_unreachable_orphan.py` for the same reason. The proper fix is to convert the file's bare sys.modules writes to `monkeypatch.setitem` / `monkeypatch.delitem`; that's scope-creep relative to the onboarding work this PR is doing and should land separately. (That file's count is currently 0 against the lint, so a follow-up that converts it can drop the baseline line altogether.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
May 13, 2026
* fix(onboarding): six defects in self-host deploy path (#443) Friction surfaced on a fresh clone → start.sh → login walkthrough. Each fix is small and independent; bundled here per the issue's "all are onboarding hygiene" framing. 1. scripts/deploy/start.sh — auto-generate SECRET_KEY and INTERNAL_API_SECRET if blank (same pattern as the existing CREDENTIAL_ENCRYPTION_KEY block, extracted to a helper). Fail fast with a clear message if ADMIN_PASSWORD is blank rather than booting into a state the operator can't log into. 2. .env.example — document FRONTEND_PORT=80 with an inline comment explaining "remap if your host's port 80 is taken." The var was already read by docker-compose.yml and start.sh but wasn't reachable from the quickstart. 3. scripts/deploy/start.sh — replace "login: admin/password" hint with "login: admin / ADMIN_PASSWORD from .env". New users took "password" as the literal default. 4. src/mcp-server/src/index.ts + README.md — drop hardcoded http://localhost:3000/api-keys hint. Port was wrong (frontend default is 80) and ignored FRONTEND_PORT overrides. Replaced with "your Trinity web UI → Settings → MCP Keys". 5. scripts/deploy/clean.sh — new script. Stops compose + removes leftover agent-* containers + trinity-agent-network. Without this, a fresh install inherits Exited zombie agents from previous tests and the first new agent collides on AGENT_SSH_PORT_START (2222). Preserves data volumes intentionally — operators can wipe those manually. 6. src/mcp-server/Dockerfile — point healthcheck at /health, not /mcp. The /mcp endpoint rejects HEAD requests with 400 (which `wget --spider` sends), so `docker compose ps` reported the container as unhealthy despite serving traffic. /health returns 200 to both HEAD and GET. Verified live: post-rebuild, trinity-mcp-server reports `healthy` within 30s. No schema, API, or runtime-behavior changes. No migration needed. Related to #443 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(tests): baseline 6 pre-existing sys.modules writes in test_platform_default_model The lint job on #443's PR went red on a file that is NOT in the PR's diff — `tests/test_platform_default_model.py` carries 6 bare `sys.modules` mutations that were merged to dev without an entry in `tests/lint_sys_modules_baseline.txt`. Same #802 rebase-race shape as the original #791 vs #783 incident: baseline-introducing PR (#791) and violation-introducing PR landed disjointly, both green pre-merge, dev went red post-merge, the next PR off dev (this one) inherits it. Ratcheting the baseline here unblocks #443. Same precedent as #796 which baselined `test_cleanup_unreachable_orphan.py` for the same reason. The proper fix is to convert the file's bare sys.modules writes to `monkeypatch.setitem` / `monkeypatch.delitem`; that's scope-creep relative to the onboarding work this PR is doing and should land separately. (That file's count is currently 0 against the lint, so a follow-up that converts it can drop the baseline line altogether.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
Unblock the
lint (sys.modules pollution check)CI job for every PR opened againstdev.Root cause — merge order on
dev:79a64814) addedtests/unit/test_cleanup_unreachable_orphan.pywith 3 top-levelsys.modules[...] = stubassignments (lines 64, 76, 79:docker,services.docker_service,database)3043631e) addedtests/lint_sys_modules.pyand generatedtests/lint_sys_modules_baseline.txt— but the baseline was generated against a tree that did not include the orphan-test file's violationsResult: the lint job now fails on every PR built against current
dev, even when the PR's own changes are clean. PR #795 hit this immediately (its own violations were fixed inece8a7db, but the orphan-test file remained as a residual failure).Fix
Add a single line to the baseline:
Same shape as the already-baselined
test_watchdog_unit.py(3 violations) — the file the orphan-test's own preload comment cites as its precedent. The 3 stubs are installed at module level before importingservices.cleanup_service, somonkeypatch(fixture-scoped) cannot replace them. The lint script's baseline is the intended mechanism for grandfathering exactly this pattern.Per the baseline file's stated goal ("reduce these counts over time; never grow them"), follow-up cleanup of these 3 stubs into a snapshot/restore helper or moving the test into the no-monkeypatch group is left as a separate hygiene PR.
Test plan
python3 tests/lint_sys_modules.py→ zero violations in the real test tree (only.venvsite-packages flagged, which CI runners don't have)References