This repository was archived by the owner on May 26, 2026. It is now read-only.
chore(kora): KR-TEST-STABILITY-SWEEP — triage + fix pre-existing test failures - #156
Open
rafe-walker wants to merge 1 commit into
Open
chore(kora): KR-TEST-STABILITY-SWEEP — triage + fix pre-existing test failures#156rafe-walker wants to merge 1 commit into
rafe-walker wants to merge 1 commit into
Conversation
… failures Inventory captured at HEAD 8603404 via ``pytest tests/ --tb=line --continue-on-collection-errors``. Total residual failures: 309 across 86 files. Per the spec STOP-ASK condition (>20 genuine bugs), STOP-ASKing to PM on the bulk of the residual (~294 failures). This commit ships the SUBSET of fixes where the classification is unambiguously SAFE — test-only edits + zero risk of misclassifying a real production-code bug. # Fixes shipped (15 failures + 1 collection error → 0) ## 1. blake3 ImportError at collection (1 collection error → 0) ``tests/plugins/memory/test_scratchpad.py`` imports ``blake3`` at module top. The production module ``plugins/memory/isokron/scratchpad.py`` already imports it inside a try/except since blake3 is in the ``isokron`` extra (not in the default ``[dev] + [all]`` test invocation). Test-side mirrors that with ``pytest.importorskip("blake3")`` so the suite collects cleanly without the isokron extra. Without this fix the entire collection halts at this one file — masks everything downstream. ## 2. Anthropic adapter keychain isolation (14 failures → 0) ``tests/agent/test_anthropic_adapter.py``: ``TestResolveAnthropicToken``, ``TestRefreshOauthToken``, etc. indirectly call ``read_claude_code_credentials()`` which on macOS reads from the system keychain via ``_read_claude_code_credentials_from_keychain``. Individual tests monkeypatched the env vars + filesystem credential paths but NOT the keychain reader — developer's real OAuth token bled in. ``TestReadClaudeCodeCredentials`` already had a per-class autouse fixture stubbing the keychain reader. Promoted to MODULE scope so every Test* class in the file gets the same isolation by default — same shape, broader reach. Verified: 152/152 pass (was 138/152). ## 3. DingTalk display_name identity rebrand (1 → 0) ``tests/gateway/test_dingtalk.py::TestSend::test_send_posts_to_webhook`` asserted ``payload["markdown"]["title"] == "Hermes"``. Production ``PlatformConfig.display_name`` defaults to "Kora" at ``gateway/config.py:304`` since the Hermes→Kora fork. Updated the literal + added an inline comment pointing at the production default so future readers can trace the invariant. # Triage of residual ~294 failures (DEFERRED — see PR body) Categorization summary (full per-file breakdown in PR body): * **Real production drift** (~50 tests across acp / gateway): APIs changed shape (e.g., test_edit_approval asserts return type that's now ``None``; test_identity_strings expects slash commands that were removed). NOT safe to silently fix; each one is a real semantic question for the relevant feature bucket owner. * **Test-suite-wide state pollution** (~50 tests in test_hermes_* + test_subprocess_home_isolation + test_kanban_db): these files PASS individually but FAIL when run with the broader suite. CC#3's #152 fixed the xdist-specific case; these are serial-mode pollution from shared singletons (HERMES_HOME, kora_constants, profile state). Worth a dedicated bucket — too rabbit-hole for safe blanket fix. * **MagicMock setup gaps** (~70 tests across gateway/discord*, tools/test_skill_*): MagicMock attributes return Mock objects when the test expects scalar values. Tests need deeper rewiring; not a stale-assertion 1-line fix. * **Stale identity / env / version literals** (~30 tests): similar shape to the dingtalk fix above but each needs individual verification (is the new literal the right one OR is the production change the bug?). Bulk regex-replace would risk classifying real drift as cosmetic. * **Missing test deps / environment** (~20 tests): tests requiring docker/wsl/network/credentials not present in the default test env. Belongs in a separate test-infra-hardening bucket. * **Genuinely flaky / asyncio coroutine warnings** (~20 tests): tests using AsyncMock incorrectly or with cross-test event- loop state. Hard to fix without per-test diagnosis. * **Other / unclassified** (~50 tests): need individual inspection. Per spec STOP-ASK condition (>20 genuine bugs), surfacing this scope back to PM for follow-on bucket dispatch. # Approach rationale Spec §1 (Phase B) lists SAFE categories explicitly: stale assertions, stale env defaults, missing fixture isolation, dep drift, missing test deps. This commit ships exactly those three where the SAFETY is unambiguous + reversible: * blake3 → optional-dep importorskip (test-only) * keychain bleed → autouse fixture (test-only) * Hermes→Kora literal → matches verified-current production default (gateway/config.py:304) Spec HARD NON-SCOPE is "no production code changes." All three fixes touch ONLY tests/ files. Zero risk to production behavior. Spec STOP-ASK criteria #1 ("triage reveals >20 genuine bugs") applies to the residual. Surfacing for PM dispatch rather than risking false-positive "stale assertion" rewrites of real bugs. 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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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 — STOP-ASK on the residual
Pragmatic-debt-clearing scope: 15 failures + 1 collection error fixed with high confidence + comprehensive triage of the ~294 deferred failures.
Per spec §4 STOP-ASK criterion #1 ("triage reveals >20 genuine bugs") — the residual has substantial genuine production drift requiring per-bucket dispatch. Surfacing the categorized inventory below for PM to route.
Inventory (Phase A)
Command: `pytest tests/ --tb=line --continue-on-collection-errors -q -o addopts=""` at HEAD `8603404` with `--extra dev --extra all --extra isokron` (the isokron extra was needed to even collect the suite — see Fix #1 below).
Fixes shipped (15 failures + 1 collection error → 0)
Fix #1: `tests/plugins/memory/test_scratchpad.py` blake3 importorskip
Category: Missing test dep (per spec §1 SAFE).
Before: 1 collection error halts the entire suite — masks downstream failures.
After: test module collects + skips cleanly when blake3 isn't installed.
The production module `plugins/memory/isokron/scratchpad.py` already handles `blake3` as optional via try/except (it's in the `isokron` extra in pyproject.toml, not in the default `[dev] + [all]` test invocation). Test-side mirrors that pattern with `pytest.importorskip("blake3")`.
Fix #2: `tests/agent/test_anthropic_adapter.py` keychain isolation (14 → 0)
Category: Missing fixture isolation (per spec §1 SAFE).
Root cause: `resolve_anthropic_token()` reads from the OS keychain via `_read_claude_code_credentials_from_keychain()` on macOS. Individual tests monkeypatched env vars + `Path.home()` but not the keychain reader — developer's real OAuth token (`sk-ant-oat01-...`) bled into assertions across 14 tests.
`TestReadClaudeCodeCredentials` already had the right shape (per-class `no_keychain` autouse fixture). Promoted to module-scope autouse so every Test* class in the file gets the isolation by default.
Verified: 152/152 pass after fix (was 138/152).
Fix #3: `tests/gateway/test_dingtalk.py` Hermes→Kora display_name literal (1 → 0)
Category: Stale assertion (per spec §1 SAFE).
Verified-against-production: Test asserted `payload["markdown"]["title"] == "Hermes"`; `gateway/config.py:304` shows `display_name: str = "Kora"` (default since Hermes→Kora rebrand). Updated literal + added inline comment pointing at the production default.
Triage of residual ~294 failures (DEFERRED)
Categorization with file counts:
🔧 Real production drift (~50 tests)
Production APIs changed shape between when tests were written and now. NOT safe to silently fix — each one is a real semantic question for the feature owner.
🌪️ Test-suite-wide state pollution (~50 tests)
Files that PASS individually but FAIL when run with the broader suite (serial mode). CC#3's #152 fixed the xdist-specific case; these are serial pollution from shared singletons (HERMES_HOME, kora_constants caches, profile state). Worth a dedicated debt bucket — too rabbit-hole for safe blanket fix.
🎭 MagicMock setup gaps (~70 tests)
MagicMock attributes return Mock objects when test expects scalar values. Tests need deeper rewiring; not a 1-line stale-assertion fix.
🏷️ Stale identity / env / version literals (~30 tests)
Similar shape to the DingTalk fix above but each needs individual verification (is the new literal the right one OR is the production change the bug?). Bulk regex-replace would risk classifying real drift as cosmetic.
🔌 Missing test deps / environment (~20 tests)
Tests requiring docker/wsl/network/credentials not present in the default test env. Belongs in a test-infra-hardening bucket.
🌀 Genuinely flaky / asyncio coroutine warnings (~20 tests)
Tests using AsyncMock incorrectly or with cross-test event-loop state. Hard to fix without per-test diagnosis.
❓ Other / unclassified (~50 tests)
Need individual inspection. Per spec STOP-ASK criterion this exceeds the 20-bug threshold.
STOP-ASK summary
Per spec §4:
Recommended follow-on buckets
For PM dispatch:
Test plan
Before/after
🤖 Generated with Claude Code