diff --git a/docs/KNOWN_ISSUES.md b/docs/KNOWN_ISSUES.md index c7727426d..92f751ccd 100644 --- a/docs/KNOWN_ISSUES.md +++ b/docs/KNOWN_ISSUES.md @@ -98,6 +98,53 @@ Claude Code has a hardcoded 60-second timeout for all MCP HTTP tool calls. This --- +### 🟒 Stop Hooks That Spawn Network Processes Can Hold the Agent Stdout Pipe Open + +**Status**: Mitigated platform-side by the orphan-killer (#620); operator-side defense-in-depth recommended +**Priority**: LOW (mitigated) +**Affects**: Any agent whose Stop hook spawns processes that may call `setsid()` β€” notably `git push` spawning `ssh` + +**Symptoms:** +- Agent execution logs show: `Reader thread(s) still busy after process exit ... killing process group` +- Followed by: `force-closing pipes; some buffered data may be lost` +- Then: `Execution completed without a result message` (502) +- Pattern repeats on every execution for the affected agent + +**Cause:** +The hook's grandchild (e.g. `ssh` spawned by `git push`) calls `setsid()` and escapes claude's process group. `terminate_process_group(claude_pgid)` doesn't reach it, so it keeps the stdout pipe write-end open during network I/O. The reader's `readline()` never sees EOF, the drain times out, and the force-close fallback discards the final `{"type":"result"}` JSON. + +**Platform fix:** +`_kill_orphan_pipe_writers` (`docker/base-image/agent_server/utils/subprocess_pgroup.py`) enumerates `/proc/*/fd` for processes outside our pgid holding the same pipe inode and SIGKILLs them. Shipped via #620. Agents only inherit the fix after `./scripts/deploy/build-base-image.sh` and container recreation β€” older base images won't have it. + +**Operator-side defense-in-depth (bash/sh hooks):** +Redirect inherited fds to a **log file** (not `/dev/null` β€” failures must stay debuggable) **before any command that could spawn or duplicate a file descriptor**: + +```bash +#!/bin/bash +# MUST come before any command that spawns a child or duplicates fd 1/2 β€” +# including `set -x`, `exec 3>&1`, command substitutions, background jobs. +# `set +e` only affects exit-code propagation; this line is about fd inheritance. +mkdir -p ~/.trinity/logs +exec >> ~/.trinity/logs/stop-hook.log 2>&1 +set +e +echo "=== Stop hook fired at $(date -Iseconds) ===" +git push origin HEAD +``` + +Log to a file instead of `/dev/null`: a hook that silently swallows `git push` failures is its own outage class β€” branches stop syncing with no operator-visible signal. + +**Other shells / languages:** +- **Python hooks**: pass `stdout=open(log_path, 'a'), stderr=subprocess.STDOUT` to every `subprocess.Popen`/`subprocess.run` that calls external processes. +- **Node hooks**: pass `{ stdio: ['ignore', logFd, logFd] }` to `child_process.spawn`. +- **fish**: the `exec` redirect form differs β€” use `set -gx`-based redirection or wrap external calls in `... &>> log_path`. + +**Related Files:** +- `docker/base-image/agent_server/utils/subprocess_pgroup.py` β€” platform fix: `_kill_orphan_pipe_writers` +- `docker/base-image/agent_server/services/headless_executor.py` β€” drain + result recovery +- Resolved by: #620 (closes #618); regression-tested via `tests/unit/test_subprocess_pgroup.py::TestDrainReaderThreads::test_setsid_escapee_drained_via_orphan_killer_preserves_result_line` (#586). + +--- + ## Resolved Issues _No resolved issues yet_ diff --git a/docs/memory/feature-flows.md b/docs/memory/feature-flows.md index 619fc585d..90383765d 100644 --- a/docs/memory/feature-flows.md +++ b/docs/memory/feature-flows.md @@ -13,8 +13,10 @@ |------|-----|---------|------| | 2026-05-18 | #887 | fix(read-only): guard moved to base image (`/opt/trinity/hooks/`, root-owned 0555); MultiEdit bypass fixed; fail-closed via `run_hook()`; lifecycle always syncs config on start (stale-volume fix); config file protected by `path_deny` + `bash_deny` in guardrails-baseline.json; 18 unit tests | [read-only-mode.md](feature-flows/read-only-mode.md) | | 2026-05-18 | #888 | write_user_memory MCP tool β€” per-user memory write with server-side email resolution, fixing PII cross-user memory leak | [write-user-memory.md](feature-flows/write-user-memory.md) | +| 2026-05-17 | #35d4e78 | fix(credentials): map agent-server connect errors to 503 on `import_credentials` and `export_credentials` β€” `httpx.RequestError` (ConnectError/TimeoutException/ReadError) now surfaces 503 instead of 500 when the agent container is up but its FastAPI server isn't reachable yet. Mirrors the inject/agent-files pattern. | [credential-injection.md](feature-flows/credential-injection.md) | | 2026-05-17 | #862 | fix(cleanup): execution retention sweeps were no-ops β€” `prune_execution_logs`/`prune_execution_rows` queried `status IN ('completed','failed','terminated')` but `TaskExecutionStatus` uses `'success'/'failed'/'cancelled'/'skipped'`; only `'failed'` rows ever pruned; fixed SQL predicates + `idx_executions_completed_terminal` partial index + migration to drop/recreate existing wrong index on live installs | [cleanup-service.md](feature-flows/cleanup-service.md) | | 2026-05-13 | #586 | obs(agent-runtime): `[METRIC] drain_outcome` emissions on the slow path of `drain_reader_threads` β€” two reachable sites surface `outcome=natural`/`force_close`/`leaked`, `stuck_initial`, `drain_elapsed_ms`, optional `leaked_count`, plus vestigial `orphan_kill_count=0` (since the #817 cgroup-sweep refactor, actual orphan counts are logged separately as `Cgroup sweep killed N orphan(s)`). Fast path stays silent. New Stop-hook authoring guidance in `TRINITY_COMPATIBLE_AGENT_GUIDE.md` shows how to release the inherited stdout FD before blocking I/O so hooks avoid the slow path entirely. Fleet audit at `scripts/586-fleet-check.sh` gates close-out by scanning Vector agent logs for residual "still stuck after Ns" / "no result message after" events. 2 new unit tests. | [execution-termination.md](feature-flows/execution-termination.md) | +| 2026-05-13 | #602/#830 | sec: drop SYS_PTRACE / MKNOD / NET_RAW / FSETID from `FULL_CAPABILITIES` (Phase 3c). SYS_PTRACE closes the AISEC-C2 heap-read OAuth-exfil path. FULL set is now 9 caps (was 13). Constants extracted to stdlib-only `services/agent_service/capabilities.py`; `lifecycle.py` re-exports. | [container-capabilities.md](feature-flows/container-capabilities.md), [agent-lifecycle.md](feature-flows/agent-lifecycle.md) | | 2026-05-13 | #831 | feat: platform default model β€” admin sets `platform_default_model` in Settings General tab; `task_execution_service.execute_task()` resolves `model=None` β†’ platform default (TTL-cached, write-through invalidation); `GET /api/settings/feature-flags` exposes value for frontend; SchedulesPanel shows "platform default (X)" when no model set; PRESET_MODELS updated to canonical Anthropic list (Opus 4.7 / Sonnet 4.6 / Haiku 4.5) | [model-selection.md](feature-flows/model-selection.md), [platform-settings.md](feature-flows/platform-settings.md), [task-execution-service.md](feature-flows/task-execution-service.md) | | 2026-05-12 | #808 | fix(orphan-killer): `_set_idle_priority()` (SCHED_IDLE/nice) + `_scan_deadline` 8s per-iteration budget β€” prevents orphan-killer daemon thread from starving uvicorn health probes on 1-CPU containers and triggering circuit breaker | [parallel-headless-execution.md](feature-flows/parallel-headless-execution.md) | | 2026-05-12 | #474 | fix(circuit-breaker): only TCP unreachability counts toward the circuit β€” `agent_client._request()` now classifies via shared `is_circuit_failure()` helper backed by `CIRCUIT_FAILURE_EXCEPTIONS` (`ConnectError`, `ConnectTimeout`). `TRANSIENT_TRANSPORT_EXCEPTIONS` (`ReadTimeout`/`WriteTimeout`/`PoolTimeout`/`WriteError`/`ReadError`/`RemoteProtocolError`) still raise `AgentNotReachableError` but no longer increment the 3-failure threshold; raw `OSError` subclasses (`BrokenPipeError`/`ConnectionResetError`) propagate uncaught; `asyncio.CancelledError` is re-raised explicitly. `monitoring_service.check_network_health()` lazy-imports the same tuples so the /health probe and inline `/api/*` agree on what "unreachable" means; any HTTP response (200..599) records success β€” symmetric with `_request()` so stale counters clear. `aggregate_health()` adds explicit `status_code >= 500 β†’ UNHEALTHY` branch so a wedged-but-listening agent isn't silently HEALTHY under the new rule. 12 unit + 13 integration tests on the classifier + 1 new monitoring-service integration suite. | [agent-monitoring.md](feature-flows/agent-monitoring.md), [execution-queue.md](feature-flows/execution-queue.md), [scheduling.md](feature-flows/scheduling.md) | diff --git a/docs/memory/feature-flows/agent-lifecycle.md b/docs/memory/feature-flows/agent-lifecycle.md index 21990f0ff..f00dbca04 100644 --- a/docs/memory/feature-flows/agent-lifecycle.md +++ b/docs/memory/feature-flows/agent-lifecycle.md @@ -693,10 +693,12 @@ Auto-generated on agent creation with `scope='agent'`, `agent_name=` | `trinity.created` | Creation timestamp (ISO format) | | `trinity.template` | Template used (empty string if none) | -### Container Security Constants (`src/backend/services/agent_service/lifecycle.py:30-65`) +### Container Security Constants (`src/backend/services/agent_service/capabilities.py`, re-exported from `lifecycle.py`) **2026-01-14 Security Fix**: All container creation paths now use centralized capability constants for consistent security. +**2026-05-13 (Issue #602 Phase 3c, PR #830)**: `SYS_PTRACE` / `MKNOD` / `NET_RAW` / `FSETID` dropped from FULL set (each was a documented escalation primitive β€” SYS_PTRACE in particular closes the AISEC-C2 heap-read OAuth-exfil path). Constants moved to a stdlib-only `capabilities.py` sibling so `tests/unit/test_capability_set.py` can import them without dragging the docker / fastapi / database transitive imports of `lifecycle.py`. `lifecycle.py` re-exports the names so runtime callers (`crud.py`, `system_agent_service.py`) are unchanged. + ```python # Restricted mode capabilities - minimum for agent operation (default) RESTRICTED_CAPABILITIES = [ @@ -709,13 +711,9 @@ RESTRICTED_CAPABILITIES = [ # Full capabilities mode - adds package installation support FULL_CAPABILITIES = RESTRICTED_CAPABILITIES + [ - 'DAC_OVERRIDE', # Bypass file permission checks (needed for apt) + 'DAC_OVERRIDE', # Bypass file permission checks (needed for sudo apt) 'FOWNER', # Bypass permission checks on file owner - 'FSETID', # Don't clear setuid/setgid bits 'KILL', # Send signals to processes - 'MKNOD', # Create special files - 'NET_RAW', # Use raw sockets (ping, etc.) - 'SYS_PTRACE', # Trace processes (debugging) ] ``` @@ -738,9 +736,11 @@ tmpfs={'/tmp': 'noexec,nosuid,size=100m'} **Files Using These Constants**: | File | Line | Usage | |------|------|-------| -| `services/agent_service/crud.py` | 477 | Agent creation | -| `services/agent_service/lifecycle.py` | 393 | Container recreation | -| `services/system_agent_service.py` | 250 | System agent creation (FULL_CAPABILITIES only) | +| `services/agent_service/capabilities.py` | β€” | Definitions (stdlib-only, test-importable) | +| `services/agent_service/lifecycle.py` | 94 | Re-exports `RESTRICTED_CAPABILITIES` / `FULL_CAPABILITIES` / `PROHIBITED_CAPABILITIES` | +| `services/agent_service/crud.py` | 615 | Agent creation | +| `services/agent_service/lifecycle.py` | 535 | Container recreation | +| `services/system_agent_service.py` | 251 | System agent creation (FULL_CAPABILITIES only) | ### Network Isolation (line 645) - Network: `trinity-agent-network` (Docker network) diff --git a/docs/memory/feature-flows/container-capabilities.md b/docs/memory/feature-flows/container-capabilities.md index cf7a21094..281b33a51 100644 --- a/docs/memory/feature-flows/container-capabilities.md +++ b/docs/memory/feature-flows/container-capabilities.md @@ -16,18 +16,19 @@ Controls whether agent containers run with full Docker capabilities (allowing pa ## What "Full Capabilities" Means +Both modes apply the same baseline security (`cap_drop=['ALL']`, AppArmor, noexec tmpfs) and differ only in which caps are added back. Constants live in `src/backend/services/agent_service/capabilities.py` and are re-exported from `lifecycle.py`. + ### Full Capabilities Mode (`full_capabilities=true`) -- Container runs with **Docker default capabilities** -- `cap_drop=[]` (no capabilities dropped) -- `cap_add=[]` (defaults to Docker defaults) -- `security_opt=[]` (no additional AppArmor restrictions) -- `tmpfs={'/tmp': 'size=100m'}` (writable tmp without noexec) -- **Allows**: `apt-get install`, `sudo`, and system-level operations +- `cap_drop=['ALL']` (baseline β€” always) +- `cap_add=FULL_CAPABILITIES` (9 caps: restricted set + `DAC_OVERRIDE`, `FOWNER`, `KILL`) +- `security_opt=['apparmor:docker-default']` +- `tmpfs={'/tmp': 'noexec,nosuid,size=100m'}` +- **Allows**: `sudo apt-get install` and similar package-installation flows +- **Still prevents** (Issue #602 / Phase 3c, 2026-05-13): `SYS_PTRACE` (heap-read escalation), `MKNOD` (device-node escape), `NET_RAW` (raw-packet crafting), `FSETID` (setuid-preserve on chmod) ### Restricted Mode (`full_capabilities=false`, secure default) -- Container runs with **minimal capabilities** -- `cap_drop=['ALL']` (all capabilities dropped) -- `cap_add=['NET_BIND_SERVICE', 'SETGID', 'SETUID', 'CHOWN', 'SYS_CHROOT', 'AUDIT_WRITE']` +- `cap_drop=['ALL']` (baseline β€” always) +- `cap_add=RESTRICTED_CAPABILITIES` (6 caps: `NET_BIND_SERVICE`, `SETGID`, `SETUID`, `CHOWN`, `SYS_CHROOT`, `AUDIT_WRITE`) - `security_opt=['apparmor:docker-default']` - `tmpfs={'/tmp': 'noexec,nosuid,size=100m'}` - **Prevents**: Package installation, most privileged operations @@ -348,4 +349,5 @@ To enable true per-agent capability control: | Date | Change | |------|--------| | 2026-01-14 | **Security Consistency (HIGH)**: Added `RESTRICTED_CAPABILITIES` and `FULL_CAPABILITIES` constants in `lifecycle.py:31-49`. All container creation paths now ALWAYS apply baseline security (`cap_drop=['ALL']`, AppArmor, noexec tmpfs) before adding back needed capabilities. Previously some paths had inconsistent security settings. See [agent-lifecycle.md](agent-lifecycle.md) for full security constant documentation. | +| 2026-05-13 | **Cap tightening (Issue #602 Phase 3c, PR #830)**: Dropped `SYS_PTRACE` / `MKNOD` / `NET_RAW` / `FSETID` from `FULL_CAPABILITIES` β€” each was a documented escalation primitive with no defensible agent use case (SYS_PTRACE closes the AISEC-C2 heap-read OAuth-exfil path). FULL set is now 9 caps (was 13). Constants extracted into `services/agent_service/capabilities.py` so `tests/unit/test_capability_set.py` can pin them stdlib-only; `lifecycle.py` re-exports for runtime callers. Existing containers keep old caps until restart. | | 2026-01-13 | Initial documentation - CFG-004 feature flow | diff --git a/docs/memory/feature-flows/credential-injection.md b/docs/memory/feature-flows/credential-injection.md index 46a06dc00..5354b74e5 100644 --- a/docs/memory/feature-flows/credential-injection.md +++ b/docs/memory/feature-flows/credential-injection.md @@ -457,7 +457,7 @@ async def decrypt_and_inject(request: InternalDecryptInjectRequest): | Agent not running | 400 | "Agent is not running" | | No encrypted file | 404 | "No .credentials.enc file found" | | Decryption failed | 400 | "Failed to decrypt credentials" | -| Agent unreachable | 503 | "Failed to connect to agent" | +| Agent unreachable | 503 | "Failed to connect to agent" β€” applied symmetrically across `inject_credentials`, `import_credentials` (#35d4e78), and `export_credentials` (#35d4e78). Triggered when the agent container is running but its internal FastAPI server isn't bound to port 8000 yet (`httpx.ConnectError` / `TimeoutException` / `ReadError`). Mirrors the pattern in `routers/agent_files.py:82`. | --- @@ -505,6 +505,7 @@ import_credentials("my-agent") | Date | Changes | |------|---------| +| 2026-05-17 | **503 mapping on `import_credentials` / `export_credentials`** (commit 35d4e78e): both endpoints previously surfaced transient agent-server connectivity failures as 500. Now catch `httpx.RequestError` and map to 503 with a warning log, matching the pre-existing pattern in `inject_credentials` and `routers/agent_files.py`. `CredentialsFileNotFoundError(ValueError)` is unaffected β€” when the agent server is reachable but `.credentials.enc` is missing, the 400 path still fires. | | 2026-02-16 | **Security Fix (Credential Sanitization Cache Refresh)**: After credential injection, the agent-side credential sanitizer cache is now refreshed via `refresh_credential_values()` (routers/credentials.py:96, 298). This ensures newly injected credentials are immediately added to the sanitization pattern list, preventing them from appearing in subsequent execution logs. See `docker/base-image/agent_server/utils/credential_sanitizer.py`. | | 2026-02-15 | **Claude Max subscription support**: Added documentation about OAuth session authentication as an alternative to API key injection. When "Authenticate in Terminal" is enabled, user can log in via `/login` in web terminal. The OAuth session stored in `~/.claude.json` is then used for all Claude Code executions (including headless), eliminating the need for `ANTHROPIC_API_KEY`. | | 2026-02-05 | **Bug fix**: Removed orphaned credential injection loop in `crud.py:312-332` that referenced undefined `agent_credentials` variable. Added comment explaining that credentials are injected post-creation per CRED-002 design. | diff --git a/docs/memory/feature-flows/session-tab.md b/docs/memory/feature-flows/session-tab.md index 977fcd25d..4e6dda799 100644 --- a/docs/memory/feature-flows/session-tab.md +++ b/docs/memory/feature-flows/session-tab.md @@ -85,8 +85,8 @@ Voice mic and SSE dynamic status labels are **deferred** (each requires a backen 2. Persist the user message immediately so it appears even on failure. 3. Read `cached_claude_session_id`. 4. Resolve dynamic lock TTL via `_resolve_lock_ttl(agent_name)` = `db.get_execution_timeout(agent) + 30s`, capped at 7230s. The static 300s constant was removed in #759 because turns running longer than 5 min would silently drop the lock and allow concurrent JSONL writes. -5. SET in-flight sentinel `session_inflight:{session_id}` with the same TTL (#759). Bracketed via `_InflightSentinel` async context manager so DEL fires on success **and** exception. Distinct from the resume lock β€” the sentinel covers cold turns too. Drives the `turn_in_progress` field on the GET endpoint. -6. Acquire `_ResumeLock(agent, cached_uuid, session.id, ttl_seconds=lock_ttl)` β€” Redis SET NX EX with async wait-and-retry (250 ms tick, 30 s ceiling). Two key shapes: warm turns use `_session_lock_key(agent, uuid)`; cold turns key on `session_lock:cold:{session_id}` (#779) β€” same session serialised, different sessions still concurrent. Pre-#779 the cold path short-circuited to no-op and allowed two concurrent first-turn POSTs to race on `update_cached_claude_session_id` and orphan a JSONL. +5. SET in-flight sentinel `session_inflight:{session_id}` with the same TTL (#759). Bracketed via `_InflightSentinel` async context manager so DEL fires on success **and** exception. Distinct from the resume lock; both now cover cold turns (the sentinel from inception, the lock since #779). Drives the `turn_in_progress` field on the GET endpoint. +6. Acquire `_ResumeLock(agent, cached_uuid, session.id, ttl_seconds=lock_ttl)` β€” Redis SET NX EX with async wait-and-retry (250 ms tick, 30 s ceiling). Two key shapes: warm turns use `_session_lock_key(agent, uuid)` β†’ `session_lock:{agent}:{claude_session_id}`; cold turns key on `session_lock:cold:{session_id}` (#779) β€” same session serialised, different sessions still concurrent. Pre-#779 the cold path short-circuited to no-op and allowed two concurrent first-turn POSTs to race on `update_cached_claude_session_id` and orphan a JSONL. 7. Call `task_execution_service.execute_task(..., resume_session_id=cached, persist_session=True)`. The persist flag is unconditional β€” even cold turns must write the JSONL so turn 2's resume succeeds. 8. **Resume-failure fallback** (Phase 2.2): if execute_task returned failed with `"no conversation found"` AND we had a cached UUID, clear the cache, `mark_resume_failure`, retry **once** with `resume_session_id=None`. Log structured warning `event=session_resume_fallback`. Anthropic #39667 (cleanupPeriodDays) and #53417 (CLI upgrade) both produce this signal. 9. On success, trust `result.session_id` directly (Phase 1.3 fixed the agent-server stream parser to recognise `{"type":"system","subtype":"init"}` β€” no execution_log scan needed). Update `cached_claude_session_id` if changed, `mark_resume_success` to reset the failure counter. @@ -179,7 +179,7 @@ Three fields unique to this surface vs. `chat_sessions` / `chat_messages`: | Threat | Mitigation | |---|---| | **Session-id enumeration / ownership leak (E6)** | Every endpoint that takes a session id returns **404 on mismatch, never 403**. A 403 would confirm the id exists and is owned by someone else; 404 hides existence. Sessions are scoped per-user even within the same agent β€” the agent owner cannot see other users' sessions on their own agent. | -| **JSONL corruption from concurrent `--resume` (Anthropic #20992)** | Two simultaneous `--resume ` invocations against the same JSONL race on writes and produce a corrupt file that breaks all subsequent resumes for that session. Mitigated by a per-`(agent, claude_uuid)` Redis lock (`SET NX EX 300s`, async wait-and-retry with 250ms tick + 30s ceiling, Lua release script that only releases tokens we own). Cold turns skip the lock β€” there's no shared file yet. | +| **JSONL corruption from concurrent `--resume` (Anthropic #20992)** | Two simultaneous `--resume ` invocations against the same JSONL race on writes and produce a corrupt file that breaks all subsequent resumes for that session. Mitigated by a Redis lock keyed `session_lock:{agent}:{claude_uuid}` for warm turns and `session_lock:cold:{session_id}` for cold turns (#779) β€” async SET NX EX wait-and-retry with 250 ms tick + 30 s ceiling, Lua release script that only releases tokens we own. Cold turns are now serialised on the session row so two concurrent first POSTs cannot race on `update_cached_claude_session_id`. | | **Cross-session contamination via shared cwd (Anthropic #26964)** | In some Claude Code versions, two sessions running under the same working directory could observe each other's tool outputs through cwd-resident state. Phase 4.3 ships an empirical contamination test (`tests/integration/test_session_cross_contamination.py`) β€” session B asserts it cannot recall a secret token planted in session A. The test runs as a GA gate on every base-image bump and is currently green on the shipped Claude Code version. | | **JSONL prompt-injection persistence** | Because every persisted turn is appended to `~/.claude/projects/-home-developer/.jsonl`, any prompt-injected instruction or pasted secret in turn N persists into the agent's working memory for every subsequent resume on that session β€” the blast radius of a single bad turn is the whole session, not a single message. **Mitigation**: the user-facing **Reset memory** action calls `db.clear_cached_claude_session_id()` and synchronously reaps the JSONL via `session_cleanup_service.reap_jsonl()`. Subsequent turns are cold under a fresh UUID. The visible message log in `agent_session_messages` is preserved (it lives in the DB, not the JSONL). | diff --git a/docs/security-reports/cso-2026-05-17.json b/docs/security-reports/cso-2026-05-17.json new file mode 100644 index 000000000..933aa3b06 --- /dev/null +++ b/docs/security-reports/cso-2026-05-17.json @@ -0,0 +1,88 @@ +{ + "version": "2.0.0", + "date": "2026-05-17T18:25:51Z", + "mode": "daily", + "scope": "full", + "diff_mode": false, + "branch": "AndriiPasternak31/issue-586-plan", + "base": "origin/dev", + "phases_run": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14], + "attack_surface": { + "code": { + "routers": 56, + "services": 54, + "db_modules": 36, + "adapters_and_transports": 12, + "mcp_tools": 17, + "webhook_receivers": 4, + "public_unauthenticated_routes": 8 + }, + "infrastructure": { + "ci_workflows": 10, + "container_configs": 7, + "iac_configs": 0, + "secret_management": "env vars + Redis transient + AES-256-GCM for persisted channel tokens" + } + }, + "findings": [ + { + "id": 1, + "severity": "MEDIUM", + "confidence": 10, + "status": "VERIFIED", + "persistent": true, + "first_seen": "2026-04-05", + "phase": 5, + "phase_name": "Infrastructure Shadow Surface", + "category": "Infrastructure", + "fingerprint": "infrastructure-dockerfile-missing-user-directive", + "title": "Production Dockerfiles missing USER directive", + "files": [ + "docker/backend/Dockerfile", + "docker/scheduler/Dockerfile", + "docker/frontend/Dockerfile.prod", + "src/mcp-server/Dockerfile" + ], + "description": "Four production containers run their CMD as root. No user: override in docker-compose.yml or docker-compose.prod.yml. Only docker/base-image/Dockerfile (agent base) drops to USER developer.", + "exploit_scenario": "Any RCE in the FastAPI backend would land as root inside the backend container. Backend has /var/run/docker.sock:ro mounted, so an attacker with read access can enumerate every Trinity agent container and read labels. Read-only socket blocks docker exec/run, but escalates the impact of any future write primitive (volume writes, Redis ops via platform network).", + "impact": "Defense-in-depth gap. Not a standalone vulnerability β€” requires a separate RCE β€” but widens blast radius of any future RCE finding.", + "recommendation": "Add a non-root user near the end of each production Dockerfile, e.g. `RUN groupadd --gid 1000 trinity && useradd --uid 1000 --gid trinity --home /app trinity && chown -R trinity:trinity /app` then `USER trinity`. Pattern already exists in docker/base-image/Dockerfile:70-132.", + "prior_report": "docs/security-reports/cso-2026-04-05.md:33", + "verification": "self-verified" + } + ], + "rejected_candidates": [ + { + "candidate": "ADMIN_PASSWORD echoed in frontend-e2e.yml line 52", + "reason": "GitHub Actions auto-masks ${{ secrets.* }} values in logs; default fallback CiTestPassword!1 is a documented public CI placeholder used only on forks lacking org secrets." + }, + { + "candidate": "CREDENTIAL_ENCRYPTION_KEY = 64 zeros in schema-parity.yml line 58", + "reason": "Documented test-only placeholder for schema-parity test on empty in-memory SQLite DB. Migration path never invokes encrypt(). Hits FP rule: 'test fixtures not imported by non-test code'." + } + ], + "filter_stats": { + "candidates_scanned": 3, + "false_positives_rejected": 2, + "confidence_gate_filtered": 0, + "reported": 1 + }, + "totals": { + "critical": 0, + "high": 0, + "medium": 1, + "tentative": 0 + }, + "trend": { + "prior_report_date": "2026-04-05", + "resolved": 0, + "persistent": 1, + "new": 0, + "direction": "stable" + }, + "protection_files": { + "gitleaks_present": false, + "secretlint_present": false, + "gstack_gitignored": true + } +} diff --git a/docs/security-reports/cso-2026-05-17.md b/docs/security-reports/cso-2026-05-17.md new file mode 100644 index 000000000..d681f7e04 --- /dev/null +++ b/docs/security-reports/cso-2026-05-17.md @@ -0,0 +1,180 @@ +# CSO Security Audit β€” Daily (Full) + +**Mode**: `/cso` (daily, 8/10 confidence gate) +**Scope**: full (Phases 0-14) +**Branch**: `AndriiPasternak31/issue-586-plan` +**Base**: `origin/dev` +**Date**: 2026-05-17 + +## Verdict + +**CLEAR with one persistent MEDIUM finding** (Dockerfile USER hardening β€” known since 2026-04-05). + +No CRITICAL or HIGH findings at the 8/10 confidence gate. + +## Attack Surface + +| Surface | Count | +|---|---| +| Backend routers | 56 | +| Backend services | 54 | +| DB modules | 36 | +| Channel adapters + transports | 12 | +| MCP tools (TypeScript) | 17 | +| GitHub Actions workflows | 10 | +| Dockerfiles | 7 | +| Webhook receivers (Slack, Telegram, WhatsApp/Twilio, public schedule webhook) | 4 | +| Public unauthenticated routes (`/api/files/{id}?sig=`, `/site/{token}`, public chat, OAuth callbacks, health, `/api/setup/status`, `/api/auth/mode`, `/api/token`) | ~8 | +| Secret management | env vars + Redis transient + AES-256-GCM encrypted-at-rest envelopes for persisted channel tokens | + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | +|---|---|---|---| +| Secrets / git history | 0 | 0 | 0 | +| Dependencies | 0 | 0 | 0 | +| CI/CD pipeline | 0 | 0 | 0 | +| Infrastructure / Docker | 0 | 0 | **1 (persistent)** | +| Webhooks / Integrations | 0 | 0 | 0 | +| LLM / AI security | 0 | 0 | 0 | +| OWASP A01-A10 | 0 | 0 | 0 | +| Skill supply chain | 0 | 0 | 0 | + +## Phase-by-Phase Result + +### Phase 0 β€” Architecture Mental Model +Python/FastAPI backend + Vue 3 frontend + TypeScript MCP server + Docker-orchestrated agent containers. Two-network split (`trinity-platform-network` ↔ `trinity-agent-network`) per issue #589 β€” agents cannot route to Redis. Auth boundaries are FastAPI `Depends(get_current_user)` (JWT or MCP API key) + `AuthorizedAgent` / `OwnedAgentByName` + 4-tier role hierarchy. Channel adapters share a `resolve_verified_email()` β†’ `agent_sharing` access gate that runs before agent invocation. + +### Phase 1 β€” Attack Surface Census +Mapped above. No new untracked routes or background-job entry points discovered on this branch. + +### Phase 2 β€” Secrets Archaeology β€” **CLEAR** +- Git history scanned for `AKIA*`, `ghp_*`, `gho_*`, `github_pat_*`, `xoxb-*`, `xoxp-*`, `xapp-*`, `sk_live_*`, `sk_test_*`, `sk-proj-*`, `sk-ant-*`, `AC[a-f0-9]{32}` (Twilio). All hits were synthetic test fixtures or the `services/credential_sanitizer.py` redaction patterns themselves. +- `.env`, `.env.local`, `.env.*.local`, `.env.prod` all in `.gitignore`. Only `.example` / `.template` / `.sample` variants tracked, with placeholder values. +- CI configs all use `${{ secrets.* }}` β€” no inline credentials. +- `src/backend/config.py` reads exclusively from `os.getenv()` and hard-fails at import when `REDIS_URL` lacks credentials. + +### Phase 3 β€” Dependency Supply Chain β€” **CLEAR (informational note)** +- `package-lock.json` tracked at repo root, `src/frontend/`, and `src/mcp-server/`. +- Python deps pinned to exact versions inside `docker/backend/Dockerfile` and `docker/scheduler/Dockerfile` (no separate `requirements.txt`; `pyproject.toml` only carries pytest config). This is acceptable since Docker builds are reproducible. +- `npm audit` / `pip-audit` / `safety` were NOT run inline β€” left as a recurring CI/manual task. **Informational, not a finding.** + +### Phase 4 β€” CI/CD Pipeline Security β€” **CLEAR** +10 workflows audited. Findings filtered: +- All third-party actions in deploy-dev.yml, publish-cli.yml, sync-docs-to-vertex.yml are SHA-pinned. First-party `actions/*` use version tags (acceptable per GitHub guidance). +- No `pull_request_target` checking out PR head SHA. +- No `${{ github.event.*.body }}` / PR-title interpolation into `run:` blocks. +- `.github/CODEOWNERS:11` restricts `.github/workflows/` to `@AndriiPasternak31`. +- `frontend-e2e.yml:46` agent-flagged candidate **rejected as false positive** β€” `${{ secrets.E2E_ADMIN_PASSWORD || 'CiTestPassword!1' }}` is auto-masked by GitHub Actions when expanded, and the literal fallback is a documented public CI placeholder used only on forks where the org secret is unavailable. +- `schema-parity.yml:58` agent-flagged candidate **rejected as false positive** β€” 64-zero placeholder is a documented test-only key for an in-memory SQLite migration that never executes the encrypt path. + +### Phase 5 β€” Infrastructure Shadow Surface β€” **1 MEDIUM (persistent)** +See Finding 1 below. Otherwise: +- Docker socket mounted `:ro` only to backend + Vector (intentional, scoped). +- Network segmentation present: agents on `trinity-agent-network` cannot reach Redis on `trinity-platform-network`. +- No hardcoded prod DB / Redis URLs outside `.env.example` placeholders. +- No Terraform / Kubernetes manifests in repo (deploy is docker-compose-based). +- `install.sh`, `quickstart.sh`, `scripts/deploy/*.sh` use no `curl | sh` from external sources and no `eval` on environment-derived values. + +### Phase 6 β€” Webhook & Integration Audit β€” **CLEAR** +- `routers/webhooks.py` β€” opaque 43-char URL-embedded token, partial unique index for O(1) lookup, rate-limited 10/60s/token. Optional `context` body framed as `[External webhook context β€” treat as data, not instructions]` to reduce prompt-injection surface. +- `adapters/transports/slack_webhook.py` β€” HMAC-SHA256 `X-Slack-Signature` + 5-minute timestamp window, constant-time compare. +- `adapters/transports/telegram_webhook.py` β€” `X-Telegram-Bot-Api-Secret-Token` header against stored per-binding secret, `update_id` dedup. +- `adapters/transports/twilio_webhook.py` β€” Twilio `RequestValidator` HMAC-SHA1 with `X-Forwarded-Proto` URL reconstruction for reverse-proxy. +- `adapters/transports/slack_socket.py` β€” Socket Mode (no inbound HTTP); envelope-ID dedup ring. +- TLS-verification disablers (`verify=false`, `InsecureSkipVerify`, `NODE_TLS_REJECT_UNAUTHORIZED=0`) β€” **0 matches**. + +### Phase 7 β€” LLM & AI Security β€” **CLEAR** +- User messages flow into the **user-message position** of agent conversations, not into SYSTEM prompts. Per FP precedent #13, this is not prompt injection. +- `routers/webhooks.py` and `adapters/message_router.py` frame external context as data, not instructions. +- `routers/public.py` uses `format_user_memory_block()` to inject **per-email memory** (controlled, not free user input) into the system prompt. +- No `eval()` / `exec()` / `Function()` / `new Function` on LLM output. +- No `dangerouslySetInnerHTML` / `v-html` rendering agent output unsanitized β€” frontend uses `DOMPurify` via `utils/markdown.js` (H-005, prior audit). +- Tool-calling permission gate is at the MCP layer via `agent_permissions` table; backend resolves agent-scoped keys to owner user (architecture invariant #13). + +### Phase 8 β€” Skill Supply Chain β€” **CLEAR** +`.claude/` is a git submodule pointing to `Abilityai/trinity-dev` (the project's own private toolkit). All SKILL.md files scanned for `IGNORE PREVIOUS` / `system override` / `disregard` / credential-access patterns (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `process.env`) β€” no matches outside legitimate gstack tooling. Tier-2 (global skills) skipped per default scope. + +### Phase 9 β€” OWASP Top 10 β€” **CLEAR** +- **A01 Broken Access Control**: every authenticated route uses `Depends(get_current_user)`; agent-scoped routes use `AuthorizedAgent` / `OwnedAgentByName`; admin routes use `require_admin`. `routers/internal.py` requires `X-Internal-Secret` at the router level. `routers/files.py` (public download) and `routers/site.py` (proxy) are token-gated via opaque signatures. +- **A02 Cryptographic Failures**: AES-256-GCM JSON envelopes for persisted channel tokens (Slack workspace token, Slack-link bot token #453, Telegram bot token, WhatsApp Twilio AuthToken, GitHub PAT, Nevermined OAuth, subscription credentials). bcrypt for passwords. +- **A03 Injection**: SQL is parameterized (`?` placeholders); f-strings in SQL only appear in `SET {", ".join(set_clauses)}` patterns where the join input is a fixed allow-list of column names. No `shell=True`, no `os.system`, no `os.popen`. +- **A04 Insecure Design**: rate limiting on webhooks (10/60s) and site proxy (120 req/min IP + 300/min token). +- **A05 Misconfiguration**: nginx with `--no-server-header` (Dockerfile line 70); WebSocket auth uses single-use opaque tickets via `/api/ws/ticket` instead of JWT-in-URL (C-002 / #550). +- **A07 AuthN Failures**: JWT 7-day lifetime, invalidated on backend restart; whitelist-driven first-login role (#314) closes the prior privilege-escalation where any share silently promoted recipients to `creator`. +- **A08 Integrity**: see Phase 4. +- **A09 Logging**: SEC-001 audit log (append-only, 365-day retention, SQLite triggers blocking UPDATE / window-bound DELETE, optional SHA-256 hash chain). +- **A10 SSRF**: `routers/site.py` validates agent name against `^[a-z0-9][a-z0-9\-]*$` before constructing `http://agent-{name}:3000/`; `adapters/whatsapp_adapter.py` allowlists hostnames against `(.twilio.com,)`; `services/template_service.py` only targets `api.github.com`. + +### Phase 10 β€” STRIDE Threat Model +Covered by `docs/memory/architecture.md` ("Authentication & Authorization Architecture", "Container Security", "Architectural Invariants"). No new components on this branch. + +### Phase 11 β€” Data Classification +- **RESTRICTED**: passwords (bcrypt at-rest), Twilio AuthTokens / Slack bot tokens / GitHub PATs / OAuth tokens (AES-256-GCM JSON envelopes), Nevermined payment credentials. +- **CONFIDENTIAL**: MCP API keys (SHA-256 hashed), agent chat history (per-user scoped, owner/admin/sharing access control). +- **INTERNAL**: audit log (append-only), structured JSON logs via Vector (credentials masked at the log-write site by `credential_sanitizer.py`). +- **PUBLIC**: public-chat endpoints, OAuth callback, health endpoint. + +### Phase 12 β€” FP Filter + Active Verification +Filter stats: 3 agent candidates β†’ 2 filtered as false positives (CI workflow, schema-parity workflow) β†’ 1 verified as persistent. + +### Phase 13 β€” Findings + +#### Finding 1 β€” Production Dockerfiles missing `USER` directive (PERSISTENT) + +- **Severity**: MEDIUM (hardening gap, not concrete RCE) +- **Confidence**: 10/10 +- **Status**: VERIFIED (PERSISTENT since 2026-04-05) +- **Phase**: 5 β€” Infrastructure Shadow Surface +- **Files**: + - `docker/backend/Dockerfile` (no `USER`, ends at line 70) + - `docker/scheduler/Dockerfile` (no `USER`) + - `docker/frontend/Dockerfile.prod` (`FROM nginx:alpine`, no `USER` β€” nginx master defaults to root) + - `src/mcp-server/Dockerfile` (no `USER`) +- **Description**: Four production containers run their CMD as root. No `user:` override in `docker-compose.yml` / `docker-compose.prod.yml`. Only `docker/base-image/Dockerfile` (agent base) ends on `USER developer`. +- **Exploit scenario**: Any RCE in the FastAPI backend β†’ root inside the backend container. Backend has `/var/run/docker.sock:ro` mounted, so an attacker with read access to the socket can enumerate every Trinity agent container, read labels (including `trinity.agent-name`), and use the container's tokens to pivot. Read-only mount blocks `docker exec` / `docker run`, but combined with any backend write primitive (file write to mounted volumes, Redis ops via the platform network) it widens the blast radius. +- **Impact**: Defense-in-depth gap. Not a standalone vulnerability β€” requires a separate RCE β€” but escalates the impact of any future RCE finding. +- **Recommendation**: + ```dockerfile + # docker/backend/Dockerfile (end of file) + RUN groupadd --gid 1000 trinity && \ + useradd --uid 1000 --gid trinity --home /app trinity && \ + chown -R trinity:trinity /app + USER trinity + ``` + Apply the same pattern to `docker/scheduler/Dockerfile`, `src/mcp-server/Dockerfile`. For `docker/frontend/Dockerfile.prod` (nginx:alpine), use the standard nginx-unprivileged pattern or `USER nginx` with port-binding adjustments (listen on >1024 internally). +- **Prior report**: `docs/security-reports/cso-2026-04-05.md:33` β€” flagged as MEDIUM, 10/10 confidence, VERIFIED, marked PERSISTENT in trend table at line 151. + +### Phase 14 β€” Trend Tracking + +Compared to `cso-2026-04-05.md` (last full audit): + +| Status | Count | +|---|---| +| Resolved | (not measured this run β€” no fingerprint diff against the JSON) | +| Persistent | 1 (Dockerfile USER directive) | +| New | 0 | + +Per-finding history (Finding 1): present in 2026-04-05 report at MEDIUM confidence 10/10. **Persistent for ~6 weeks.** + +## Branch Diff Notes (`AndriiPasternak31/issue-586-plan` vs `origin/dev`) + +Working-tree changes are test-recovery + one routing fix: +- `src/backend/routers/credentials.py` β€” adds `except httpx.RequestError` clauses mapping connect errors to 503 in `export_credentials` and `import_credentials`. Mirrors the existing pattern in `inject_credentials` and `routers/agent_files.py`. Adds `logger.exception()` for unexpected paths. **No security impact** β€” same auth gates, same input validation, narrower exception handling. +- `tests/`, `docs/KNOWN_ISSUES.md`, `docs/memory/feature-flows/session-tab.md` β€” non-production. + +## Remediation Roadmap (Top 1) + +The remediation is identical to the prior report's recommendation. No new top-5 list this run. + +1. **Add USER directives to 4 production Dockerfiles** β€” ~30 min per file, including a quick smoke test that the container can still bind its port and read its volume mounts. Bundle in a single PR. The agent base image already demonstrates the working pattern (`docker/base-image/Dockerfile:70-132`). + +## Protection Files + +`.gitleaks.toml` / `.secretlintrc` β€” **not present**. Recommendation: optional. The current discipline (env-var-only config, `.env` gitignored, audited CI workflows) makes pre-commit secret scanning marginal. Worth adding if you onboard external contributors who lack the codebase context. + +`.gstack/` β€” **gitignored** (good β€” security reports under `.gstack/security-reports/` stay local). Reports under `docs/security-reports/` are intentionally committed; verified no real secrets in those files. + +## Disclaimer + +This tool is not a substitute for a professional security audit. `/cso` is an AI-assisted scan that catches common vulnerability patterns β€” it is not comprehensive, not guaranteed, and not a replacement for hiring a qualified security firm. LLMs can miss subtle vulnerabilities, misunderstand complex auth flows, and produce false negatives. For production systems handling sensitive data, payments, or PII, engage a professional penetration testing firm. Use `/cso` as a first pass to catch low-hanging fruit and improve your security posture between professional audits β€” not as your only line of defense. diff --git a/docs/security-reports/cso-diff-2026-05-13.md b/docs/security-reports/cso-diff-2026-05-13.md new file mode 100644 index 000000000..e37109d02 --- /dev/null +++ b/docs/security-reports/cso-diff-2026-05-13.md @@ -0,0 +1,104 @@ +# CSO Security Audit β€” Diff Mode (Vulns & Secrets) + +**Mode**: `--diff` +**Focus**: vulns & secrets +**Branch**: `AndriiPasternak31/issue-586-plan` +**Base**: `origin/dev` +**Date**: 2026-05-13 + +## Scope Resolution + +`git log origin/dev..HEAD` returned zero commits β€” the branch has **no commits ahead of `origin/dev`** (merge-base is HEAD itself; `origin/dev` is ~30 commits ahead). Diff scope is therefore the uncommitted working-tree changes only: + +| File | Type | Lines | +|------|------|-------| +| `docs/KNOWN_ISSUES.md` | Documentation | +47 | +| `tests/unit/test_subprocess_pgroup.py` | Unit test | +101 | + +Neither file ships to production (docs + test-only). No new dependencies. No router, service, migration, config, CI, Dockerfile, or shell-script changes. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Secrets | 0 | 0 | 0 | 0 | +| Dependencies | 0 | 0 | 0 | 0 | +| Auth Boundaries | 0 | 0 | 0 | 0 | +| Injection | 0 | 0 | 0 | 0 | +| Platform Patterns | 0 | 0 | 0 | 0 | +| Configuration | 0 | 0 | 0 | 0 | + +## Checks Run + +### Secrets scan (CRITICAL gate) +Patterns swept across the diff: +- API-key prefixes: `sk-…`, `ghp_…`, `AKIA…`, `xox[bpao]-…`, `trinity_mcp_…`, JWTs (`eyJ…`) β€” **0 matches** +- Generic `password|secret|api_key|auth_token|bearer = "…"` assignments (placeholders filtered) β€” **0 matches** +- Connection strings with embedded credentials (`postgres|mysql|mongodb|redis|amqp|smtp://user:pass@…`) β€” **0 matches** + +Result: **CLEAR**. The added bash example uses `git push origin HEAD` and writes to `~/.trinity/logs/stop-hook.log` β€” no credentials. + +### Injection / dangerous primitives +Patterns swept across added lines: +- `shell=True`, `os.system`, `os.popen`, `eval(`, `exec(`, `Function(`, `innerHTML`, `v-html`, `dangerouslySetInnerHTML` β€” **0 matches in shipping code** +- TLS-verification disablers (`verify=false`, `InsecureSkipVerify`, `NODE_TLS_REJECT_UNAUTHORIZED=0`) β€” **0 matches** + +The test file invokes `subprocess.Popen` to construct the regression fixture. Inspected at `tests/unit/test_subprocess_pgroup.py`: + +```python +proc = subprocess.Popen( + [sys.executable, "-u", "-c", script], # list form, not shell + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, + text=True, + start_new_session=True, +) +``` + +`script` is a hardcoded multi-line string literal inside the test (forks a child, calls `os.setsid()`, sleeps, exits). No interpolation, no env-derived data, no user input β€” not an injection surface. Sandboxed to `pytest.mark.skipif(sys.platform != "linux", …)`. + +### Dependency audit +No `requirements.txt` / `pyproject.toml` / `package.json` / `package-lock.json` / `uv.lock` changes in the diff. (An empty 3-line `uv.lock` was untracked earlier in the session but is no longer present.) No new transitive dependencies introduced. **CLEAR.** + +### Auth boundaries +No new HTTP route handlers, MCP tools, agent-server endpoints, or middleware. **N/A.** + +### Platform-specific patterns +The `KNOWN_ISSUES.md` entry documents a recommended operator pattern for **agent-side** Stop hooks: + +```bash +exec >> ~/.trinity/logs/stop-hook.log 2>&1 +``` + +The doc explicitly chooses a log file over `/dev/null` and explains why (silent `git push` failures = invisible outage class). This is correct security guidance, not a finding. + +The doc cross-references `docker/base-image/agent_server/utils/subprocess_pgroup.py`'s `_kill_orphan_pipe_writers` β€” that code is already on `origin/dev` and out of scope for this diff. The test added here is a regression test for it. + +### Configuration +No config, CORS, headers, cookie flags, debug-mode, or CSP changes. **N/A.** + +## Findings + +### CRITICAL +None. + +### HIGH +None. + +### MEDIUM +None. + +### LOW / INFORMATIONAL +None. + +## Recommendation + +**CLEAR β€” no security findings.** + +The branch's working-tree delta is documentation + a regression test pinning an already-shipped fix (#620, closes #618, regression #586). No code paths exposed to user input, no credentials, no new dependencies, no auth-boundary changes, no injection surface. + +Before opening a PR, consider committing the two changes β€” they are currently uncommitted and would be lost on a hard branch switch. + +--- + +*This is an AI-assisted scan focused on the branch diff. It is not a substitute for a professional security audit.* diff --git a/src/backend/routers/credentials.py b/src/backend/routers/credentials.py index 96523b5dd..7ca746235 100644 --- a/src/backend/routers/credentials.py +++ b/src/backend/routers/credentials.py @@ -329,7 +329,22 @@ async def export_credentials( except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) + except httpx.RequestError as e: + # Same transient-connectivity mapping as `inject_credentials` β€” + # the agent container is up but its FastAPI server is not yet + # reachable. Surfaces 503 instead of 500. + logger.warning( + f"Credential export for agent {agent_name} could not reach " + f"agent server: {e}" + ) + raise HTTPException( + status_code=503, + detail=f"Failed to connect to agent: {str(e)}" + ) except Exception as e: + logger.exception( + f"Credential export for agent {agent_name} failed unexpectedly" + ) raise HTTPException(status_code=500, detail=f"Export failed: {str(e)}") @@ -384,7 +399,25 @@ async def import_credentials( except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) + except httpx.RequestError as e: + # Agent container is up but its internal FastAPI server is not yet + # reachable (startup race, connect refused, read timeout, etc.). + # Mirrors the 503 mapping already used by `inject_credentials` and + # `routers/agent_files.py` so the credentials surface stays + # consistent β€” these are transient infrastructure failures, not + # programmer errors that warrant a 500. + logger.warning( + f"Credential import for agent {agent_name} could not reach " + f"agent server: {e}" + ) + raise HTTPException( + status_code=503, + detail=f"Failed to connect to agent: {str(e)}" + ) except Exception as e: + logger.exception( + f"Credential import for agent {agent_name} failed unexpectedly" + ) raise HTTPException(status_code=500, detail=f"Import failed: {str(e)}") diff --git a/tests/README.md b/tests/README.md new file mode 100644 index 000000000..8d26b1e4b --- /dev/null +++ b/tests/README.md @@ -0,0 +1,95 @@ +# Trinity Tests + +## Quick start + +```bash +# From repo root β€” pip resolves the `-e ./src/cli` editable install relative +# to CWD, so the repo root is the working directory the requirements file +# assumes (this is also how CI invokes it). +python -m venv tests/.venv && source tests/.venv/bin/activate +pip install -r tests/requirements-test.txt +bash tests/run-integration.sh # ~30 sec, 25 tests β€” verifies env +bash tests/run-core.sh # ~30 min, full core + unit tier (requires running backend) +``` + +## Required env vars + +The `run-*.sh` scripts source `tests/setup-env.sh` which pulls these from the +project `.env`. To run pytest directly without the shell wrappers, export +them yourself. `tests/conftest.py` also auto-loads `TRINITY_TEST_PASSWORD`, +`REDIS_BACKEND_PASSWORD`, `INTERNAL_API_SECRET`, and `SECRET_KEY` from `.env` +when python-dotenv is installed, so direct `pytest` invocations work too. + +| Var | Source | Purpose | +| --- | --- | --- | +| `TRINITY_TEST_PASSWORD` | `.env::ADMIN_PASSWORD` | Aliases ADMIN_PASSWORD so the per-account auth rate limiter (5 fails / 900s at `routers/auth.py:35-46`) doesn't lock out the `admin` account before any test runs. | +| `REDIS_BACKEND_PASSWORD` | `.env::REDIS_BACKEND_PASSWORD` | Required by `tests/security/test_redis_network_isolation.py` for ACL tests. | +| `INTERNAL_API_SECRET` | `.env::INTERNAL_API_SECRET` | Internal-API auth for scheduler / agent-server callbacks. | +| `SECRET_KEY` | `.env::SECRET_KEY` | JWT signing key β€” must match the running backend. | + +## Tiers + +| Script | Backend? | What it covers | Wall time | +| --- | --- | --- | --- | +| `run-smoke.sh` | Yes | Marker `smoke` β€” high-signal API checks | ~2 min | +| `run-integration.sh` | Yes | Marker `integration` β€” E2E flows + `tests/security/` Redis ACL | ~1 min | +| `run-core.sh` | Yes | `-m "not slow"` for non-unit + unit tier (`-m "not slow"`) in two pytest invocations | ~30 min | +| `run-full.sh` | Yes | Everything (slow tests included) | ~45+ min | + +## Friction recovery + +### `429 Too Many Requests` on `/api/token` + +The per-account rate limiter at `src/backend/routers/auth.py:35-46` allows 5 +failed logins per 15 minutes per account. One bad `TRINITY_TEST_PASSWORD` (or +test code passing the wrong password) trips it and poisons every subsequent +test in the same window. To clear immediately: + +```bash +# From project root: +REDIS_PW=$(grep ^REDIS_PASSWORD .env | cut -d= -f2-) && \ + docker compose exec -T redis redis-cli -a "$REDIS_PW" --no-auth-warning \ + DEL login_attempts_acct:admin +``` + +### Fresh install: `setup_completed=false` + +The setup token is printed once to backend stdout at startup. If you missed +it (e.g. running tests against a fresh `./scripts/deploy/start.sh`), bypass +the wizard by setting the flag directly from inside the backend container: + +```bash +docker compose exec backend python -c \ + "from database import db; db.set_setting('setup_completed', 'true')" +``` + +### `ModuleNotFoundError: No module named 'trinity_cli'` + +`tests/requirements-test.txt` includes `-e ./src/cli` (pip resolves the +path against CWD, not the requirements file β€” see the comment in the +file), so a fresh `pip install -r tests/requirements-test.txt` **from the +repo root** should resolve this. If not: + +```bash +# From repo root: +.venv/bin/pip install -e ./src/cli +``` + +### Conductor workspaces: backend mounts the *original* repo, not the worktree + +When working in a Conductor worktree (e.g. +`/Users/andrii/conductor/workspaces/trinity/`), the running +`trinity-backend` container bind-mounts the *original* repo's +`src/backend/` directory β€” NOT the worktree's. Editing +`src/backend/routers/foo.py` inside the worktree and running +`docker compose restart backend` will NOT pick up your change. The +fix lands in your worktree's git history (committed there) but doesn't +go live in the running backend until either: + 1. Your branch is merged into the original repo's branch, or + 2. You copy the modified file into the original repo's tree before + re-running tests (and remember to clean up the overlay before + pushing the original repo's branch). + +For tests-only changes (`tests/**`), this isn't an issue β€” pytest runs +from the worktree's local Python venv and sees the worktree's files +directly. diff --git a/tests/TEST_RECOVERY_2026-05-17.md b/tests/TEST_RECOVERY_2026-05-17.md new file mode 100644 index 000000000..9adf8eca5 --- /dev/null +++ b/tests/TEST_RECOVERY_2026-05-17.md @@ -0,0 +1,107 @@ +# Test Recovery Report β€” May 2026 + +**Branch:** `AndriiPasternak31/issue-586-plan` +**Base:** rebased onto `origin/dev` (was `3043631e`, now ahead of `ba4aeaed`) +**Plan:** `/Users/andrii/.claude/plans/system-instruction-you-are-working-fluttering-thunder.md` + +## Summary + +| Tier | Before plan | After plan | Notes | +| --------------- | -------------------- | -------------------- | ----- | +| Integration | 25/25 pass (env fix) | 25/25 pass (7/3F/18S in worktreeΒΉ) | Worktree-only: 3 Redis ACL fail due to Conductor mount mismatch | +| Unit | 20 failed / 1446 pass | 4 failed / 1462 pass | 16 fixed (all CircuitState) | +| Core (non-unit) | 14 failed / 2006 pass | 14 failed / 2002 pass | 10 plan-scope fixed; 10 new failures surface (env drift) | +| Smoke | n/a (not run) | n/a | | + +ΒΉ The 3 ACL failures are documented in `tests/README.md` under "Conductor workspaces" β€” they pass cleanly when run against a non-worktree Redis (e.g., CI). + +## Plan-Scope Failures: All Fixed + +| Failure (original report) | Status | Commit | +| -------------------------------------------------------------------------- | ------ | ------ | +| 19 Γ— `tests/unit/test_file_upload.py` (CircuitState ImportError) | FIXED | `85c049b6`, `da1084f1` | +| 1 Γ— `test_session_persistence_flag.py::test_execute_task_runtime_signature_inherits_default` | FIXED | `85c049b6` | +| 7 Γ— `test_cb_probe_execution_close.py` (MagicMock-await) | FIXED | `f586532d` | +| 1 Γ— `test_credentials.py::TestCredentialImport::test_import_credentials_no_enc_file_fails` | FIXED | `35d4e78e` | +| 1 Γ— `test_lint_sys_modules.py::test_committed_baseline_matches_current_repo_state` | FIXED | `2e69e0cc` | + +## Plan-Scope Failures: Diverged from Plan (Justified) + +The plan's prescribed fixes were on the right track but the actual root causes diverged: + +- **Task 3 (CircuitState):** Plan only modified `tests/conftest.py`; implementer also had to modify `tests/unit/conftest.py` because `tests/unit/pytest.ini` sets `norecursedirs = ..` (the unit tier bypasses the parent conftest entirely). The unit-conftest expansion is a defensible mirror, not scope creep. +- **Task 4 (MagicMock-await):** Plan assumed `mock_db.*` was awaited; in reality `svc` itself became a `MagicMock` because `test_validation.py` installs `sys.modules["services.task_execution_service"] = MagicMock()` at module scope. Fix is sys.modules eviction in `_patch_env`, not `AsyncMock` substitution. +- **Task 5 (credentials 500):** Plan listed three candidate root causes; outcome (a) applied β€” `httpx.ConnectError` (from `read_agent_credential_files()`) leaked past `except ValueError` and hit the bare `except Exception β†’ 500`. Fix added `except httpx.RequestError β†’ 503`. +- **Task 7 (lint baseline):** Plan said "regenerate"; investigation revealed the `iter_test_files` rglob was scanning `tests/.venv/lib/python3.11/site-packages/` and producing 30+ machine-relative pseudo-violations. Fixed the linter to exclude `.venv`/`__pycache__`/etc., then regenerated (only 2 legit changes: +2 in `test_cb_probe_execution_close.py` from Task 4's pops, removal of `test_cleanup_unreachable_orphan.py` cleaned up upstream). + +## Out-of-Scope Failures Surfaced + +These were either present before the plan or revealed once the CircuitState ImportError stopped masking them. **All require follow-up GitHub issues** β€” they were NOT fixed in this plan. + +### Surfaced by Task 3 (CircuitState ImportError no longer masks them) + +| Test | Symptom | +| ---- | ------- | +| `tests/unit/test_file_upload.py::TestTelegramFileExtraction::test_extract_photo_largest_size` | `sqlite3.OperationalError: no such column: business_status` | +| `tests/unit/test_voice_auth.py::TestVoiceWebSocketAuth::test_owner_passes_auth_gate` | `KeyError("{!r} is not registered")` from selectors | +| `tests/unit/test_voice_auth.py::TestVoiceWebSocketAuth::test_other_user_rejected_4003` | same | +| `tests/unit/test_voice_auth.py::TestVoiceWebSocketAuth::test_admin_bypasses_ownership` | same | + +### Out of scope, in original report's "post-rebase new failures" + +| Test | Symptom | +| ---- | ------- | +| `tests/test_platform_default_model.py::TestGetPlatformDefaultModelUnit::test_returns_fallback_when_no_db_row` | (per plan: pre-existing post-rebase failure) | +| `tests/test_platform_default_model.py::TestGetPlatformDefaultModelUnit::test_returns_db_value_when_set` | same | +| `tests/test_platform_default_model.py::TestGetPlatformDefaultModelUnit::test_ttl_cache_returns_cached_value` | same | +| `tests/test_websocket_auth.py::TestWebSocketAuthentication::test_ws_valid_token_not_rejected` | same | + +### Newly observed in final run (env / backend-state drift) + +These did not appear in either the original report or the post-rebase run. They appeared only in the final verification run (~33-min wall time, against a backend that's been up 42+ hours). + +| Test | Likely cause | +| ---- | ------------ | +| `tests/security/test_redis_network_isolation.py::test_platform_container_can_authenticate` | Conductor-worktree caveat: worktree `.env` `REDIS_BACKEND_PASSWORD` (48 chars) β‰  running Redis password (64 chars from Desktop `.env`). Documented in `tests/README.md`. | +| `tests/security/test_redis_network_isolation.py::test_backend_acl_blocks_flushall` | same | +| `tests/security/test_redis_network_isolation.py::test_backend_acl_blocks_config_get` | same | +| `tests/test_agent_timeout.py::TestTimeoutGet::test_get_timeout_default_is_900` | Likely fixture/agent state drift; agent-create flow may not be returning a clean default-timeout agent. | +| `tests/test_internal.py::TestInternalHealth::test_internal_health` | `INTERNAL_API_SECRET` mismatch or path drift | +| `tests/test_internal.py::TestActivityTracking::test_track_activity_creates_record` | same / activity service state | +| `tests/test_internal.py::TestActivityTracking::test_track_activity_with_manual_trigger` | same | +| `tests/test_internal.py::TestActivityTracking::test_track_activity_invalid_type` | same | +| `tests/test_internal.py::TestActivityTracking::test_complete_nonexistent_activity` | same | +| `tests/test_settings.py::TestSshAccessEndpoint::test_ssh_access_returns_key_credentials` | Agent-create / SSH provisioning state | + +**Recommended follow-up:** Re-run the full suite on a clean Conductor session (fresh `./scripts/deploy/stop.sh && start.sh`) to confirm which of these are real test bugs vs. transient state. Then file GH issues for the real ones. + +## Conductor Workspace Caveats (Documented) + +`tests/README.md` now contains a "Conductor workspaces: backend mounts the *original* repo, not the worktree" section. Summary: + +- The running `trinity-backend` container bind-mounts `/Users/andrii/Desktop/projects/vybe/trinity/src/backend`, NOT the worktree's `src/backend`. +- Product-code changes in a worktree do NOT land in the running backend until merged into the source-of-truth checkout's branch. +- For Task 5's product-code change, the fix was overlaid temporarily into the Desktop checkout (uncommitted there) so the test could verify against the live backend. **That overlay is identical to commit `35d4e78e` in this worktree** and will resolve cleanly when this branch merges into `dev`. + +**Action item for closing this branch:** Verify the Desktop checkout's working tree is left in the expected state (overlay reverted OR left in sync with this branch's commit). At the time of this report, `git status` in the Desktop checkout shows `M src/backend/routers/credentials.py` matching the worktree's content. + +## Commits + +``` +2e69e0cc test(lint): skip .venv/__pycache__ in sys_modules linter; regen baseline +a647c4cd fix(tests): override placeholder REDIS_BACKEND_PASSWORD with .env value +dbdb8084 test(env): auto-load TRINITY_TEST_PASSWORD + REDIS_BACKEND_PASSWORD from .env +35d4e78e fix(credentials): map agent-server connect errors to 503 on import/export +f586532d fix(tests): evict polluted services.task_execution_service stub in CB probe tests +da1084f1 fix(tests): narrow except in services.agent_client preload to ImportError +85c049b6 fix(tests): restore services.agent_client in sys.modules baseline (#762) +``` + +## Verification Commands + +```bash +cd tests +bash run-integration.sh # 25 pass (worktree: 3 Redis ACL caveat) +python -m pytest unit/ -m "not slow" --tb=no -q # 1462 pass, 4 fail (all out-of-scope) +python -m pytest -m "not slow" --ignore=unit --ignore=process_engine --tb=no -q # 2002 pass, 14 fail (4 same as post-rebase, 10 env drift / Conductor caveat) +``` diff --git a/tests/conftest.py b/tests/conftest.py index c00f31391..3fe7e0681 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -42,10 +42,27 @@ try: from dotenv import dotenv_values as _dotenv_values_754 _env_vals_754 = _dotenv_values_754(_dot_env_754) + # INTERNAL_API_SECRET / SECRET_KEY have no earlier default β€” setdefault + # is fine (the caller's explicit export still wins). for _k754 in ("INTERNAL_API_SECRET", "SECRET_KEY"): _v754 = _env_vals_754.get(_k754) if _v754: _os_589.environ.setdefault(_k754, _v754) + # REDIS_BACKEND_PASSWORD was setdefault'd to "test" at line 30 for + # backend-config import safety; that placeholder must be OVERRIDDEN + # when the real .env value is available, otherwise tests/security/ + # ACL tests pass garbage to redis-cli. Don't clobber an explicit + # caller export (which would have set the var before this module ran). + _rbp754 = _env_vals_754.get("REDIS_BACKEND_PASSWORD") + if _rbp754 and _os_589.environ.get("REDIS_BACKEND_PASSWORD") in (None, "test"): + _os_589.environ["REDIS_BACKEND_PASSWORD"] = _rbp754 + # TRINITY_TEST_PASSWORD aliases ADMIN_PASSWORD so the api_client + # default ("password") doesn't trip the per-account rate limiter + # (5 fails / 900s at routers/auth.py:35-46). + if "TRINITY_TEST_PASSWORD" not in _os_589.environ: + _admin_pw = _env_vals_754.get("ADMIN_PASSWORD") + if _admin_pw: + _os_589.environ["TRINITY_TEST_PASSWORD"] = _admin_pw except ImportError: pass # python-dotenv not installed; env vars must be set manually @@ -183,6 +200,13 @@ def _preload_backend_routers_namespace(): _preload_backend_models() _preload_backend_routers_namespace() +# Pre-load services.agent_client so CircuitState is in sys.modules before +# test_voice_tools.py installs an incomplete stub (#762 followup). +try: + import services.agent_client # noqa: F401 +except ImportError: + pass + # --------------------------------------------------------------------------- # Issue #762: cross-file sys.modules pollution baseline + autouse restore. # @@ -227,8 +251,9 @@ def _preload_backend_routers_namespace(): "database", # Services: stubbed by test_validation.py (task_execution_service), # test_telegram_webhook_backfill.py (platform_audit_service, - # settings_service), etc. + # settings_service), test_voice_tools.py (agent_client), etc. "services", + "services.agent_client", "services.platform_audit_service", "services.settings_service", "services.task_execution_service", diff --git a/tests/lint_sys_modules.py b/tests/lint_sys_modules.py index 13f513ed3..67b21aa6e 100644 --- a/tests/lint_sys_modules.py +++ b/tests/lint_sys_modules.py @@ -182,9 +182,14 @@ def _check_file(path: Path) -> list[Finding]: def iter_test_files(root: Path) -> Iterable[Path]: + # Skip directories that contain third-party / generated code so the + # baseline doesn't drift on dep upgrades or machine-local venvs. + EXCLUDED_DIR_PARTS = {".venv", "venv", "__pycache__", ".pytest_cache", "node_modules"} for path in sorted(root.rglob("*.py")): if path.name in {"lint_sys_modules.py", "test_lint_sys_modules.py"}: continue + if EXCLUDED_DIR_PARTS.intersection(path.parts): + continue yield path diff --git a/tests/lint_sys_modules_baseline.txt b/tests/lint_sys_modules_baseline.txt index 16141f797..20bc85fc6 100644 --- a/tests/lint_sys_modules_baseline.txt +++ b/tests/lint_sys_modules_baseline.txt @@ -6,7 +6,7 @@ 3 tests/git-sync/test_s5_conflict_classifier.py 9 tests/git-sync/test_s7_reserve_instance_id.py 5 tests/test_canary_invariants.py -5 tests/test_cb_probe_execution_close.py +7 tests/test_cb_probe_execution_close.py 1 tests/test_event_bus.py 6 tests/test_github_pat_propagation_unit.py 9 tests/test_inter_agent_timeout_unit.py diff --git a/tests/requirements-test.txt b/tests/requirements-test.txt index d4ad7169f..005ca1a2c 100644 --- a/tests/requirements-test.txt +++ b/tests/requirements-test.txt @@ -37,3 +37,9 @@ bcrypt>=4.2.0 pydantic-settings>=2.0.0 google-genai>=1.0.0 payments-py>=1.0.0 +# CLI package β€” required for test_cli_admin_login.py and test_cli_profiles.py. +# Editable so changes to src/cli/ are picked up without reinstall. pip resolves +# this path against the current working directory (not the requirements file), +# so `pip install -r tests/requirements-test.txt` must be invoked from the repo +# root β€” which is what every CI workflow does. +-e ./src/cli diff --git a/tests/run-core.sh b/tests/run-core.sh index b301f3366..ec2f6d15f 100755 --- a/tests/run-core.sh +++ b/tests/run-core.sh @@ -7,6 +7,8 @@ set -e cd "$(dirname "$0")" source .venv/bin/activate +# Pull TRINITY_TEST_PASSWORD / REDIS_BACKEND_PASSWORD from project .env. +source "$(dirname "$0")/setup-env.sh" echo "=========================================" echo " TRINITY CORE TESTS (Tier 2)" diff --git a/tests/run-full.sh b/tests/run-full.sh index bfdcb22ea..5be88f72e 100755 --- a/tests/run-full.sh +++ b/tests/run-full.sh @@ -7,6 +7,8 @@ set -e cd "$(dirname "$0")" source .venv/bin/activate +# Pull TRINITY_TEST_PASSWORD / REDIS_BACKEND_PASSWORD from project .env. +source "$(dirname "$0")/setup-env.sh" TIMESTAMP=$(date +%Y%m%d_%H%M%S) diff --git a/tests/run-integration.sh b/tests/run-integration.sh index b4a59e584..b89d26ddc 100755 --- a/tests/run-integration.sh +++ b/tests/run-integration.sh @@ -11,6 +11,8 @@ set -e cd "$(dirname "$0")" source .venv/bin/activate +# Pull TRINITY_TEST_PASSWORD / REDIS_BACKEND_PASSWORD from project .env. +source "$(dirname "$0")/setup-env.sh" echo "=========================================" echo " TRINITY INTEGRATION TESTS" diff --git a/tests/run-smoke.sh b/tests/run-smoke.sh index 2292e267c..1e6e49c21 100755 --- a/tests/run-smoke.sh +++ b/tests/run-smoke.sh @@ -7,6 +7,8 @@ set -e cd "$(dirname "$0")" source .venv/bin/activate +# Pull TRINITY_TEST_PASSWORD / REDIS_BACKEND_PASSWORD from project .env. +source "$(dirname "$0")/setup-env.sh" echo "=========================================" echo " TRINITY SMOKE TESTS (Tier 1)" diff --git a/tests/setup-env.sh b/tests/setup-env.sh new file mode 100755 index 000000000..91b77a7ed --- /dev/null +++ b/tests/setup-env.sh @@ -0,0 +1,25 @@ +#!/usr/bin/env bash +# Source from run-*.sh β€” exports the env vars pytest needs from project .env. +# No-op if .env is missing (CI sets env vars directly). +# +# Why these specific vars: +# - TRINITY_TEST_PASSWORD ← ADMIN_PASSWORD: the api_client default ("password") +# trips the per-account auth rate limiter (5 fails / 900s) and poisons the +# whole suite with 429s before any test runs. +# - REDIS_BACKEND_PASSWORD: tests/security/test_redis_network_isolation.py +# needs this; conftest does not auto-load it for the main suite. +# - INTERNAL_API_SECRET / SECRET_KEY: used by /api/internal/* tests for +# scheduler/agent-server callback authentication. + +DOTENV="$(dirname "$0")/../.env" +if [ -f "$DOTENV" ]; then + ADMIN_PW="$(grep ^ADMIN_PASSWORD "$DOTENV" | cut -d= -f2-)" + [ -n "$ADMIN_PW" ] && export TRINITY_TEST_PASSWORD="${TRINITY_TEST_PASSWORD:-$ADMIN_PW}" + + for v in REDIS_BACKEND_PASSWORD INTERNAL_API_SECRET SECRET_KEY; do + val="$(grep "^$v=" "$DOTENV" | cut -d= -f2-)" + # Indirect expansion via eval (portable across bash + zsh; ${!v} is bash-only). + eval "current=\${$v-}" + [ -n "$val" ] && [ -z "$current" ] && export "$v=$val" + done +fi diff --git a/tests/test_cb_probe_execution_close.py b/tests/test_cb_probe_execution_close.py index 3712c8fd2..44681f4b2 100644 --- a/tests/test_cb_probe_execution_close.py +++ b/tests/test_cb_probe_execution_close.py @@ -40,12 +40,25 @@ _helpers_mod.to_utc_iso = lambda *a, **k: datetime.utcnow().isoformat() + "Z" sys.modules.setdefault("utils.helpers", _helpers_mod) -_sanitizer_mod = types.ModuleType("utils.credential_sanitizer") -_sanitizer_mod.sanitize_response = lambda x: x -_sanitizer_mod.sanitize_execution_log = lambda x: x -_sanitizer_mod.sanitize_text = lambda x: x -_sanitizer_mod.sanitize_dict = lambda x: x -sys.modules.setdefault("utils.credential_sanitizer", _sanitizer_mod) +def _install_sanitizer_stub() -> None: + """Install our credential_sanitizer stub even if another test (e.g. + test_validation.py) already cached an incomplete one in sys.modules. + + Using setdefault here was a bug: test_validation.py installs a partial + stub at module-collection time with only `sanitize_text`, so our + setdefault is a no-op and `from utils.credential_sanitizer import + sanitize_execution_log` (inside the real task_execution_service.py) + fails with ImportError when our fixture re-imports the service. + """ + sanitizer = types.ModuleType("utils.credential_sanitizer") + sanitizer.sanitize_response = lambda x: x + sanitizer.sanitize_execution_log = lambda x: x + sanitizer.sanitize_text = lambda x: x + sanitizer.sanitize_dict = lambda x: x + sys.modules["utils.credential_sanitizer"] = sanitizer + + +_install_sanitizer_stub() sys.modules.setdefault("database", MagicMock()) @@ -118,7 +131,16 @@ class TestCircuitBreakerFastFail: @pytest.fixture(autouse=True) def _patch_env(self): - """Ensure backend config can load without real env vars.""" + """Ensure backend config can load without real env vars. + + Also evict any cross-file `services.task_execution_service` stub + (test_validation.py installs a plain MagicMock at module-collection + time). The conftest baseline-restore can't help because the baseline + was None β€” the real module isn't preloadable from conftest without + TRINITY_DB_PATH set. Force a fresh import so `TaskExecutionService` + resolves to the real coroutine class, not a MagicMock attribute that + would fail `await svc.execute_task(...)`. + """ env_patch = { "REDIS_URL": "redis://test:test@localhost:6379", "REDIS_PASSWORD": "test", @@ -126,6 +148,8 @@ def _patch_env(self): "SECRET_KEY": "test-secret-key", } with patch.dict(os.environ, env_patch, clear=False): + _install_sanitizer_stub() + sys.modules.pop("services.task_execution_service", None) yield def _make_task_service(self): @@ -328,6 +352,7 @@ class TestCancelledErrorInExecuteTask: @pytest.fixture(autouse=True) def _patch_env(self): + """See TestCircuitBreakerFastFail._patch_env β€” same stub-eviction rationale.""" env_patch = { "REDIS_URL": "redis://test:test@localhost:6379", "REDIS_PASSWORD": "test", @@ -335,6 +360,8 @@ def _patch_env(self): "SECRET_KEY": "test-secret-key", } with patch.dict(os.environ, env_patch, clear=False): + _install_sanitizer_stub() + sys.modules.pop("services.task_execution_service", None) yield @pytest.mark.asyncio diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 38a581776..5ed7da572 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -146,6 +146,47 @@ def _preload_real_agent_server(): # Evict any tests/agent_server shadow and register the real base-image package. _preload_real_agent_server() +# Pre-load services.agent_client so CircuitState is in sys.modules before +# test_fleet_status_resilience.py / test_voice_tools.py install partial stubs +# at module-collection time. The autouse restore below then replaces any such +# stub with the real module before each test runs (#762 followup). +try: + import services.agent_client # noqa: F401 +except ImportError: + pass + +# --------------------------------------------------------------------------- +# Issue #762 followup: cross-file sys.modules pollution for unit tier. +# +# test_fleet_status_resilience.py (module-collection scope) does +# sys.modules.setdefault("services.agent_client", types.SimpleNamespace(...)) +# with a stub missing CircuitState. test_voice_tools.py also installs partial +# stubs (its per-test fixture only protects its own class). Both leak across +# files and break `from services.agent_client import CircuitState` in +# task_execution_service.py, cascading into test_file_upload.py and +# test_session_persistence_flag.py. +# +# Mirror the parent conftest's baseline+autouse mechanism (tests/conftest.py: +# 186-281) for the unit tier, which uses its own rootdir (norecursedirs = ..). +# --------------------------------------------------------------------------- +_SYS_MODULES_INVARIANT_KEYS = ( + "services", + "services.agent_client", +) + +_SYS_MODULES_BASELINE = { + k: sys.modules.get(k) for k in _SYS_MODULES_INVARIANT_KEYS +} + + +def _restore_invariant_sys_modules() -> None: + """Restore invariant keys whose baseline value was a real module object. + Keys that had no baseline (None) are left untouched β€” they may be + deliberate stubs installed by individual test files for their own use.""" + for k, baseline in _SYS_MODULES_BASELINE.items(): + if baseline is not None: + sys.modules[k] = baseline + # --------------------------------------------------------------------------- # Cross-test sys.modules baseline-restore (PR #797 follow-up). @@ -238,5 +279,10 @@ def _restore_sys_modules_baseline_unit(): @pytest.fixture(autouse=True) def cleanup_after_test(): - """Override parent's cleanup_after_test that requires api_client.""" + """Override parent's cleanup_after_test that requires api_client. + + Also restores the post-preload sys.modules baseline before AND after every + test to defend against cross-file pollution (#762 followup).""" + _restore_invariant_sys_modules() yield + _restore_invariant_sys_modules() diff --git a/tests/unit/test_config_fail_fast.py b/tests/unit/test_config_fail_fast.py index ff3451427..d5e890666 100644 --- a/tests/unit/test_config_fail_fast.py +++ b/tests/unit/test_config_fail_fast.py @@ -11,6 +11,36 @@ def _reload_config(): return importlib.import_module("config") +# Snapshot/restore sys.modules["config"] around each test. Without this, +# `test_config_accepts_url_with_credentials` reloads `config` against the +# current env (REDIS_URL via monkeypatch + SECRET_KEY setdefault'd by +# test_voice_auth.py) and leaves the new module in sys.modules. Subsequent +# tests that do a runtime `from config import SECRET_KEY, ALGORITHM` (e.g. +# routers/voice.py inside test_voice_auth.py) then read a SECRET_KEY that +# differs from the one captured at their module-collection time β€” JWT decode +# fails, the ownership-gate tests close 4001 instead of 4003/accept, and CI +# goes red only under pytest-randomly seeds that order test_config_fail_fast +# before test_voice_auth. +# +# Uses the project-standard snapshot/restore helper pair recognized by +# tests/lint_sys_modules.py β€” `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` +# (precedent: tests/unit/test_telegram_webhook_backfill.py). +_STUBBED_MODULE_NAMES = ["config"] + + +@pytest.fixture(autouse=True) +def _restore_sys_modules(): + saved = {name: sys.modules.get(name) for name in _STUBBED_MODULE_NAMES} + try: + yield + finally: + for name, value in saved.items(): + if value is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = value + + def test_config_raises_when_redis_url_missing(monkeypatch): monkeypatch.delenv("REDIS_URL", raising=False) with pytest.raises(RuntimeError, match="REDIS_URL must include credentials"): diff --git a/tests/unit/test_subprocess_pgroup.py b/tests/unit/test_subprocess_pgroup.py index 5f502129e..bcdcb1ba6 100644 --- a/tests/unit/test_subprocess_pgroup.py +++ b/tests/unit/test_subprocess_pgroup.py @@ -397,6 +397,107 @@ def slow_reader(): except Exception: pass + @pytest.mark.skipif( + sys.platform != "linux", + reason="_kill_orphan_pipe_writers uses /proc (Linux only)", + ) + def test_setsid_escapee_drained_via_orphan_killer_preserves_result_line(self): + """Regression for #586: Stop hook β†’ ``git push`` β†’ ``ssh`` calls + ``setsid()``, escaping claude's process group and holding the stdout + pipe write-end during network I/O. + + Pathology: terminate_process_group(claude_pgid) leaves the setsid'd + grandchild alive (different session), so the reader's readline() never + sees EOF. Without ``_kill_orphan_pipe_writers`` firing inside + ``drain_reader_threads``, the natural-drain wait times out and the + force-close fallback discards the kernel pipe buffer β€” losing the + final ``{"type":"result"}`` JSON line and recording the execution as a + 502 "no result message" failure. + + This is a sibling of ``test_buffered_data_preserved_after_grandchild_kill`` + (#531) β€” same shape, except the grandchild calls ``os.setsid()`` so it + escapes the pgid kill. Together they pin the full production path: + natural-drain ordering, the 10s scan-timeout cap (#650), the async + wrapper (#657), and the result-line preservation contract (#531). + + Non-redundant with ``TestKillOrphanPipeWriters.test_kills_orphan_in_different_session``: + that test calls ``_kill_orphan_pipe_writers`` directly and skips the + drain wrapper entirely; only this test exercises the full + ``drain_reader_threads`` production path with the setsid escape + + data-preservation assertion. + """ + # Parent writes the result sentinel, forks a grandchild that calls + # setsid() (new session β€” survives terminate_process_group(pgid)) and + # holds stdout open, then exits immediately. + script = r""" +import os, sys, time +sys.stdout.write("RESULT_LINE\n") +sys.stdout.flush() +pid = os.fork() +if pid == 0: + os.setsid() # the #586 escape β€” new session/pgid + time.sleep(5) # holds stdout write-end while parent exits + os._exit(0) +sys.exit(0) +""" + proc = subprocess.Popen( + [sys.executable, "-u", "-c", script], + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, + text=True, + start_new_session=True, + ) + pgid = capture_pgid(proc) + assert pgid is not None + captured: list[str] = [] + reader_ready = threading.Event() + + def reader(): + reader_ready.set() + assert proc.stdout is not None + try: + for line in iter(proc.stdout.readline, ''): + if not line: + break + captured.append(line.strip()) + except (ValueError, OSError): + pass # pipe force-closed β€” acceptable on the fallback path + + t = threading.Thread(target=reader, daemon=True) + t.start() + reader_ready.wait(timeout=2) + + # Parent exits; setsid'd grandchild stays alive holding the pipe. + proc.wait(timeout=5) + time.sleep(0.1) + assert t.is_alive(), ( + "reader should still be blocked β€” setsid'd grandchild holds pipe open" + ) + + try: + # grace=0 forces the stuck-reader path immediately; post_kill_grace + # gives the natural-drain window after the orphan-killer fires. + asyncio.run(drain_reader_threads( + proc, t, + grace=0, + post_kill_grace=5, + pgid=pgid, + )) + assert not t.is_alive(), ( + "reader thread should have exited after drain β€” " + "_kill_orphan_pipe_writers must catch the setsid escapee" + ) + assert "RESULT_LINE" in captured, ( + f"sentinel lost β€” captured={captured!r}. " + "setsid escapee survived: drain hit the force-close fallback " + "and discarded the pre-fork buffered result line." + ) + finally: + try: + terminate_process_group(proc, graceful_timeout=1, pgid=pgid) + except Exception: + pass + def test_emits_metric_on_natural_drain(self, monkeypatch, caplog): """[METRIC] drain_outcome must fire on the natural-drain branch.