Skip to content

test(#586) + test-suite recovery + doc backfill + CSO 2026-05-17#875

Merged
vybe merged 23 commits into
devfrom
AndriiPasternak31/issue-586-plan
May 23, 2026
Merged

test(#586) + test-suite recovery + doc backfill + CSO 2026-05-17#875
vybe merged 23 commits into
devfrom
AndriiPasternak31/issue-586-plan

Conversation

@AndriiPasternak31
Copy link
Copy Markdown
Contributor

Summary

Test plan

  • pytest tests/unit/test_subprocess_pgroup.py::TestDrainReaderThreads::test_setsid_escapee_drained_via_orphan_killer_preserves_result_line passes on Linux CI (skipped on macOS — uses /proc).
  • Full unit suite green on CI.
  • No lint regressions from the sys_modules linter baseline change.
  • Manual: agent with a git push stop hook executes without the Reader thread(s) still busyforce-closing pipesExecution completed without a result message failure pattern.

Refs #586

🤖 Generated with Claude Code

AndriiPasternak31 and others added 19 commits May 14, 2026 08:58
test_voice_tools.py and test_fleet_status_resilience.py install
incomplete stubs of services.agent_client into sys.modules at
module-collection scope. The autouse restore in tests/conftest.py
only restored keys whose baseline was non-None, and only ran for
the non-unit tier (the unit tier sets norecursedirs = .. in
tests/unit/pytest.ini and bypasses the parent conftest entirely).

The polluted stubs missed CircuitState, so transitive importers
(adapters → task_execution_service:32 → from services.agent_client
import CircuitState) raised ImportError, taking down 19 tests in
test_file_upload.py and 1 in test_session_persistence_flag.py.

Two changes:

1. tests/conftest.py: add services.agent_client to
   _SYS_MODULES_INVARIANT_KEYS and pre-import it before the baseline
   is captured, so the baseline is a real module object that gets
   restored between tests (covers the non-unit tier).

2. tests/unit/conftest.py: mirror the baseline+autouse-restore
   mechanism for services and services.agent_client. The unit tier
   has its own rootdir; without this, the parent conftest's defenses
   never run for the unit suite.

Unit tier: 20 failed → 4 failed (the 4 remaining are unrelated SQL
schema and KeyError failures in test_extract_photo_largest_size and
test_voice_auth, out of scope for this task).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rror

Follow-up to #762 — code-review feedback. except Exception: pass
would mask non-import errors (e.g. side-effect runtime exceptions
at module load) and silently degrade the autouse-restore defense.
ImportError is the only expected failure mode for the preload.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… probe tests

Seven tests in TestCircuitBreakerFastFail and
TestCancelledErrorInExecuteTask failed with "object MagicMock can't
be used in 'await' expression" — but only when test_validation.py
ran first in the same pytest session. The TypeError fires at
`await svc.execute_task(...)`, not on a single attribute mock.

Root cause: test_validation.py does
`sys.modules["services.task_execution_service"] = MagicMock()` at
module-collection time. The conftest baseline-restore (#762) cannot
undo it because the baseline value is `None` — the real
task_execution_service module isn't loadable from conftest preload
(it imports `database`, which mkdirs `/data` and fails outside
Docker). The autouse restore explicitly preserves None-baseline
entries to avoid clobbering deliberate stubs.

When test_cb_probe later does
`from services.task_execution_service import TaskExecutionService`
it gets `MagicMock.TaskExecutionService`, instantiating returns a
MagicMock, `svc.execute_task(...)` returns a MagicMock, and
`await MagicMock` raises TypeError.

Fix: in each affected class's autouse `_patch_env` fixture, pop
`services.task_execution_service` from sys.modules before the test
body's import — forcing a fresh load of the real module. Also
replace the module-level `setdefault` of `utils.credential_sanitizer`
with an unconditional install (test_validation.py installs a
partial stub that lacks `sanitize_execution_log`, so setdefault was
a no-op and the fresh task_execution_service import failed at
`from utils.credential_sanitizer import sanitize_execution_log`).

Verified: full file passes (10/10) standalone and after
test_validation.py pollution.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…port

Outcome (a) from the Task 5 plan: when an admin calls
POST /api/agents/{name}/credentials/import on an agent whose container
status is "running" but whose internal FastAPI server hasn't bound to
port 8000 yet, `import_to_agent()` raises `httpx.ConnectError`
("All connection attempts failed"), which is not a `ValueError`. The
existing handler had only `except ValueError → 400` and a bare
`except Exception → 500`, so the transient race surfaced as a 500.

`test_import_credentials_no_enc_file_fails` accepts 400 (file missing)
or 503 (agent not ready) but never 500 — so the regression failed CI.

Fix mirrors the pattern already used by `inject_credentials` (same file,
line 250) and `routers/agent_files.py:82`: catch `httpx.RequestError`
(parent of ConnectError / TimeoutException / ReadError) and map to 503
with a warning log. Applied symmetrically to `export_credentials` since
it has the same shape and would 500 on the same transient condition.

`CredentialsFileNotFoundError(ValueError)` is unaffected — when the
agent server *is* reachable but no `.credentials.enc` exists,
`import_to_agent()` still raises that ValueError subclass and the
existing 400 mapping still fires.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rom .env

Closes the env-friction set surfaced by the May 2026 test-recovery audit:
  - tests/setup-env.sh sourced by run-*.sh exports the four vars
    pytest needs from project .env.
  - tests/conftest.py picks them up the same way for direct pytest
    invocation (alias ADMIN_PASSWORD -> TRINITY_TEST_PASSWORD).
  - tests/requirements-test.txt installs src/cli editable so
    test_cli_admin_login.py and test_cli_profiles.py can collect.
  - tests/README.md documents the env matrix, tiers, the rate-limit +
    setup-completed recovery commands, and the Conductor-worktree
    backend-mount caveat.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The setdefault chain in tests/conftest.py was order-dependent: line 30
unconditionally setdefault'd "test" as a placeholder for backend-config
import safety, which made the subsequent setdefault from .env a no-op.
tests/security/ ACL tests then ran with the wrong password and failed
with NOAUTH unless the caller manually exported REDIS_BACKEND_PASSWORD
before pytest.

Switch to explicit override: when the .env value is present AND the
current env-var is either unset or the "test" placeholder, replace it.
An explicit caller export (any value other than "test") still wins.

Follow-up to dbdb808.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two changes:

1. lint_sys_modules.py:iter_test_files — skip directories that contain
   third-party / generated code (.venv, venv, __pycache__,
   .pytest_cache, node_modules). The previous rglob walked into
   tests/.venv/lib/python3.11/site-packages and reported 30+ pseudo-
   violations in dependency code that vary per machine / dep version.
   The baseline already excluded these (zero .venv entries) so the
   committed baseline was machine-relative without anyone noticing
   until a dep upgrade exposed the drift.

2. tests/lint_sys_modules_baseline.txt — regenerate. Two real changes:
   - test_cb_probe_execution_close.py: 5 → 7 (+2 sys.modules.pop calls
     added in commit f586532 to evict cross-file stub pollution).
   - tests/unit/test_cleanup_unreachable_orphan.py removed (cleaned up
     during the dev rebase).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary of the 7 fixes landed in this branch (CircuitState sys.modules,
MagicMock-await eviction, credentials 503 mapping, env friction docs +
helpers, lint baseline regen + linter .venv exclusion) plus an
inventory of out-of-scope failures and Conductor-worktree caveats that
need follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds test_setsid_escapee_drained_via_orphan_killer_preserves_result_line
to pin the full production path: parent emits result, forks a setsid()'d
grandchild that holds stdout open, parent exits. Asserts that
drain_reader_threads invokes _kill_orphan_pipe_writers and the buffered
"RESULT_LINE" survives — i.e. the drain doesn't fall through to the
force-close fallback that discards the kernel pipe buffer.

Sibling of the #531 buffered-data test, distinct because the grandchild
calls os.setsid() — the case that escapes terminate_process_group(pgid)
and is the real-world signature in production (git push → ssh).

Also adds a KNOWN_ISSUES.md entry describing the bug class, the platform
fix (#620), and operator-side defense-in-depth for stop hooks that spawn
network processes.

Refs #586

Co-Authored-By: Claude <noreply@anthropic.com>
- agent-lifecycle.md / container-capabilities.md: document Phase 3c
  capability drop (#602 / PR #830, 2026-05-13) — SYS_PTRACE, MKNOD,
  NET_RAW, FSETID removed from FULL_CAPABILITIES (now 9 caps, was 13);
  constants extracted to capabilities.py so test_capability_set.py can
  import them stdlib-only.
- credential-injection.md: document 503 mapping for import/export
  endpoints (commit 35d4e78, 2026-05-17) — httpx.RequestError now
  surfaces 503 instead of 500, mirroring inject and agent-files.
- session-tab.md: update lock semantics for #779 — cold turns now
  serialised on session_lock:cold:{session_id} (previously short-
  circuited, letting two concurrent first POSTs race on
  update_cached_claude_session_id and orphan one JSONL inside the agent).
- feature-flows.md: index entries for the above.

Co-Authored-By: Claude <noreply@anthropic.com>
- cso-2026-05-17.{md,json}: daily full-phase audit (Phases 0-14).
  Verdict CLEAR at 8/10 confidence gate; one persistent MEDIUM
  (Dockerfile USER hardening — tracked since 2026-04-05). No new
  CRITICAL or HIGH findings.
- cso-diff-2026-05-13.md: diff-mode report covering the working-tree
  changes for the #586 regression test and KNOWN_ISSUES.md entry. No
  vulns or secrets — docs + test-only, no production code path.

Co-Authored-By: Claude <noreply@anthropic.com>
Picks up the methodology toolkit head that adds DEVELOPMENT_WORKFLOW.md.

Co-Authored-By: Claude <noreply@anthropic.com>
…e-586-plan

# Conflicts:
#	docs/memory/feature-flows.md
#	docs/memory/feature-flows/session-tab.md
pip resolves relative paths in requirements files against the current
working directory, not the requirements file. `-e ../src/cli` (added in
dbdb808) only worked when pip was invoked from `tests/`; CI runs from
the repo root and got `../src/cli` -> outside workspace -> 6 pytest jobs
+ schema-parity + regression-diff all red on PR #875.

Switch to `-e ./src/cli` and update tests/README.md to instruct invoking
from repo root.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… reload

test_config_accepts_url_with_credentials calls _reload_config() which
pops sys.modules["config"] and re-imports. The reloaded module reads
SECRET_KEY from os.environ at reload time, picking up
"test-secret-key-for-unit-tests" from test_voice_auth.py's
os.environ.setdefault — but the *original* config (loaded at conftest
pre-import time, before that env was set) had a random SECRET_KEY from
secrets.token_hex(32). The test then leaves the new module in
sys.modules permanently.

Downstream, routers/voice.py's runtime `from config import SECRET_KEY,
ALGORITHM` (called from voice_websocket) reads the new SECRET_KEY,
while test_voice_auth.py's JWTs were signed with the original key
captured at its module-collection time. JWT decode raises JWTError,
voice_websocket closes 4001 instead of the expected 4003/accept, and
the 3 ownership-gate tests fail — but only under pytest-randomly seeds
that schedule test_config_fail_fast before test_voice_auth (notably
seed 12345 after PR #875 added test_subprocess_pgroup.py and shifted
the random ordering).

Snapshot/restore sys.modules["config"] via an autouse fixture so each
test's reload is scoped to itself. The 3 voice_auth tests
(test_owner_passes_auth_gate, test_other_user_rejected_4003,
test_admin_bypasses_ownership) now pass on seed 12345.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_fail_fast

Renames _restore_config_module to the lint-recognized
_STUBBED_MODULE_NAMES + _restore_sys_modules pair so the helper-exception
in tests/lint_sys_modules.py fires (precedent:
tests/unit/test_telegram_webhook_backfill.py).

The previous attempt (0966243) used a bespoke fixture name + bare
sys.modules.pop, which is exactly what the #762 hygiene linter bans
outside conftest.py — the lint job went red on push. Same behavior,
correct pattern, exempted by the linter.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lot_per_slot_ttl.py

#871 landed on dev (commit 98574f3) with test_slot_per_slot_ttl.py
containing 6 bare sys.modules.{pop,setdefault,assign} calls but no
matching baseline entry. Dev's own post-merge lint job is now red on
that commit (Issue #802 caught it as intended). Any PR that merges
with current dev inherits the failure.

This PR is unrelated to the slot file but blocked by the inherited
red. Bump the baseline to match what dev actually ships, unblocking
CI here. The proper fix — refactor test_slot_per_slot_ttl.py to use
the _STUBBED_MODULE_NAMES + _restore_sys_modules helper pattern, or
scope its sys.modules mutations via monkeypatch — should land as a
follow-up on #871's author or whoever owns the slot file (file is
otherwise untouched here).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n test_slot_per_slot_ttl.py"

This reverts commit 459b535.
@vybe
Copy link
Copy Markdown
Contributor

vybe commented May 19, 2026

PR Validation Report

PR: #875test(#586) + test-suite recovery + doc backfill + CSO 2026-05-17
Branch: AndriiPasternak31/issue-586-plandev
Files Changed: 26 (+964/-33)
Issue: Refs #586 (P1 bug — #620 fixed it; this PR pins it with a regression test)

Summary

Category Status Notes
Commit Messages 19 commits, conventional prefixes with detailed bodies
Base Branch targets dev
PR Size ⚠️ 26 files — large, but mostly tests + docs
GitHub Issues ⚠️ Refs #586 (correct — #620 was the fix). Close manually on merge.
Requirements No new feature
Architecture No API/schema changes (503-mapping mirrors existing pattern)
Feature Flows 4 flows updated (agent-lifecycle, container-capabilities, credential-injection, session-tab) + KNOWN_ISSUES.md
Security Check No keys/tokens/emails/IPs/.env/secrets. .env references are env-var loaders, not values
Code Quality ⚠️ Mixed scope — see Warnings
CSO Audit cso-2026-05-17.md CLEAR at 8/10; cso-diff-2026-05-13.md clean

CI Status

Check Result
pytest (base + head × seeds 12345/67890/99999) ✅ 6/6 pass
regression diff, schema-parity
lint (sys.modules pollution check) FAIL

Issues Found

Critical (Block Merge)

Warnings (Review Required)

Suggestions

  • Squash-merge to collapse 19 commits (including baseline-bump-then-revert) into clean concerns.
  • File a follow-up issue for test_slot_per_slot_ttl.py refactor — upstream cause of the lint red.

Recommendation

REQUEST CHANGES — solely on the red lint check.

Substance is strong: real regression test that pins #620's fix for a P1 production bug, disciplined test-suite recovery (7 fixes documented in TEST_RECOVERY_2026-05-17.md), complete doc backfill, attached CSO audit, security clean. 6 random-seed pytest runs green.

The blocker is environmental — the sys.modules linter is red because of test_slot_per_slot_ttl.py from #871, not because of this PR's changes.

Two paths forward:

  1. Land the test_slot_per_slot_ttl.py refactor on dev first (clean ownership), then rebase here.
  2. Re-apply the baseline bump from 4595b535 with framing like "absorbing fix(cleanup): per-slot TTL prevents false-kill of long-running executions (#869) #871 dev-side red, refactor tracked in follow-up issue #XXX" and a follow-up issue filed.

🤖 Generated with Claude Code

Copy link
Copy Markdown
Contributor

@vybe vybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Substance is approved — real #586 regression test, narrow credentials 503 fix mirroring existing patterns, test-recovery commits each with clear root-cause analysis, doc backfill aligned with the touched code, clean CSO audit, 6-seed pytest matrix + regression-diff + schema-parity all green.

Blocker before merge:

  • Rebase on current dev and resolve the baseline conflict. PR #884 just merged and regenerated tests/lint_sys_modules_baseline.txt (adds test_slot_per_slot_ttl.py: 6, removes test_cleanup_unreachable_orphan.py: 3). This branch's 45419160 reverted its own equivalent baseline bump in anticipation of #884 landing, so HEAD now diverges. After rebase, accept dev's version of the baseline file and the lint job should go green on the next CI run.

Once CI is green I'll re-approve immediately.

…e-586-plan

Resolves PR #875 reviewer request: rebase on dev to pick up PR #884's
baseline regeneration (adds test_slot_per_slot_ttl.py: 6 via #881's
_STUBBED_MODULE_NAMES refactor; removes test_cleanup_unreachable_orphan.py: 3).

After merge, sys.modules lint is green:
- test_slot_per_slot_ttl.py: now exempt via sanctioned _STUBBED_MODULE_NAMES + _restore_sys_modules pattern (PR #881 on dev)
- test_cb_probe_execution_close.py: baseline 7 matches actual (this PR's stub eviction)
- test_config_fail_fast.py: actual 0, baseline 1 (reduced — sanctioned)

Conflict in docs/memory/feature-flows.md resolved by keeping both
this PR's #35d4e78 row and dev's #887/#888 rows.
@AndriiPasternak31
Copy link
Copy Markdown
Contributor Author

Rebased — merged latest dev (through 009c7fc8 / #884).

Conflict resolution:

  • tests/lint_sys_modules_baseline.txt — auto-merged cleanly; dev's +6 test_slot_per_slot_ttl.py and -3 test_cleanup_unreachable_orphan.py are in; this branch's 7 test_cb_probe_execution_close.py (matches the stub-eviction in f586532d) preserved.
  • docs/memory/feature-flows.md — kept both dev's #887/#888 rows and this PR's #35d4e78 row.

Local lint result:

Violations retired or reduced (👍 below baseline — consider regenerating):
  tests/unit/test_config_fail_fast.py: 1 → 0 (-1)
  tests/unit/test_slot_per_slot_ttl.py: 6 → 0 (-6)
OK: 230 violation(s) in 65 file(s); baseline allows 237 — no new violations.

test_slot_per_slot_ttl.py is now exempt via the sanctioned _STUBBED_MODULE_NAMES + _restore_sys_modules pattern that #881 landed on dev (actual count 0, baseline 6 — a "reduced" notice, not a fail). Should be green on the next CI run.

…e-586-plan

Two new commits on dev since the previous merge (c9008e4):
- b892069: feat(observability): emit [METRIC] drain_outcome on slow-path orphan-killer (#586) (#837)
- c6bc456: feat(soft-delete): agent_ownership soft-delete + retention purge (#834 Phase 1a) (#838)

Conflicts resolved:
- tests/unit/test_subprocess_pgroup.py: kept BOTH this PR's
  test_setsid_escapee_drained_via_orphan_killer_preserves_result_line
  (#586 regression) AND dev's #837 metric tests
  (test_emits_metric_on_natural_drain, test_emits_metric_on_force_close_path).
  Tests cover orthogonal aspects of the slow-path drain: setsid escape
  + result-line preservation vs. drain_outcome metric emission.
- docs/memory/feature-flows.md: kept both this PR's #35d4e78 row and
  dev's #586/#837 row.

Lint still green; sys.modules baseline already current after the
previous dev-merge (c9008e4).
@AndriiPasternak31
Copy link
Copy Markdown
Contributor Author

CI fully green on 51bb5943 (latest dev re-merge after b8920696 / c6bc456a landed):

  • lint (sys.modules pollution check) ✅
  • pytest base × 3 seeds + head × 3 seeds ✅
  • schema-parity, regression diff ✅
  • CodeQL (python + js/ts) ✅

Second merge resolved two conflicts:

Ready for re-review.

…e-586-plan

# Conflicts:
#	tests/test_cb_probe_execution_close.py
@vybe
Copy link
Copy Markdown
Contributor

vybe commented May 22, 2026

Merged current dev into the branch (55a481cc) to clear the conflict. One real conflict in tests/test_cb_probe_execution_close.py — kept your _install_sanitizer_stub() helper (overrides instead of setdefault, which is the correctness fix your comment block explains) and added sanitize_dict = lambda x: x to its body so #797's salvage-path import in services.task_execution_service still resolves.

The other 18 touched files merged cleanly. CI re-running.

The branch's submodule bump (3873d2c) was a parent of dev's current
pointer (9650477), so merging would silently revert the
"fix(announce): prevent duplicate Slack sends by switching to Python
urllib" change in .claude.

Resetting the submodule pointer to dev's current state effectively
drops the bump from this PR. The DEVELOPMENT_WORKFLOW.md doc added
by 3873d2c is still present (9650477 includes it as an ancestor).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vybe
Copy link
Copy Markdown
Contributor

vybe commented May 23, 2026

Pushed one fix-up commit on your behalf — branch is now MERGEABLE.

What I changed:

  • Reverted the .claude submodule pointer (86d31b4e). Your branch bumped it from dev's current 9650477 to 3873d2c, which is the parent commit — merging would have silently reverted the "fix(announce): prevent duplicate Slack sends" change in .claude. Reset to match dev.

Validation pass summary (all green except the submodule, which I just fixed):

Will re-approve and squash-merge once CI re-runs on 86d31b4e.

Copy link
Copy Markdown
Contributor

@vybe vybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved after submodule fix. CI green (lint, 6 pytest seeds, schema-parity, regression-diff, CodeQL). Pushed 86d31b4e myself to reset the .claude submodule pointer to dev's current — your bump was to the parent commit which would have silently reverted the announce fix.

@vybe vybe merged commit dbf5e64 into dev May 23, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants