Fix: Add missing Docker labels to system agent container - #1
Merged
Conversation
The system agent was missing required Docker labels (trinity.ssh-port, trinity.cpu, trinity.memory, trinity.created), causing port allocation conflicts when creating new agents. Without trinity.ssh-port label: - get_agent_status_from_container() returns port=0 - get_next_available_port() filters out port 0 - System agent's port (2290) appears available - New agents fail with 'port already allocated' error This fix adds all required labels to match the standard agent creation pattern in routers/agents.py, ensuring proper port tracking and conflict prevention.
vybe
added a commit
that referenced
this pull request
Dec 27, 2025
Bug #1: Terminal session lost when switching tabs - Changed v-if to v-show for terminal tab content in AgentDetail.vue - Keeps terminal component mounted, preserving WebSocket connection Bug #2: MCP deploy_local_agent only copied CLAUDE.md - Updated startup.sh to copy ALL template files instead of hardcoded list - Now includes template.yaml and custom directories (src/, lib/, etc.) - Added .trinity-initialized marker to prevent re-copying on restart 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
6 tasks
This was referenced Apr 20, 2026
Closed
4 tasks
dolho
added a commit
that referenced
this pull request
Apr 23, 2026
Move the pre-check exec logic out of routers/internal.py into a dedicated services/pre_check_service.py. Aligns with Invariant #1 (Router → Service → DB): the router becomes a 5-line passthrough, all business logic (path constant, two exec calls, stdout capping, contract dict) lives in the service. Same shape as services/slot_service.py, services/task_execution_service.py, services/monitoring_service.py — testable in isolation, discoverable via grep, won't trip /validate-architecture later. Behavior unchanged. 13/13 scheduler unit tests still pass; live endpoint hit returns same contract dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dolho
added a commit
that referenced
this pull request
Apr 24, 2026
Move the pre-check exec logic out of routers/internal.py into a dedicated services/pre_check_service.py. Aligns with Invariant #1 (Router → Service → DB): the router becomes a 5-line passthrough, all business logic (path constant, two exec calls, stdout capping, contract dict) lives in the service. Same shape as services/slot_service.py, services/task_execution_service.py, services/monitoring_service.py — testable in isolation, discoverable via grep, won't trip /validate-architecture later. Behavior unchanged. 13/13 scheduler unit tests still pass; live endpoint hit returns same contract dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
4 tasks
vybe
pushed a commit
that referenced
this pull request
Apr 27, 2026
… (#501) Replaces raw `DELETE FROM agent_git_config` SQL in routers/git.py with the existing `db.delete_git_config()` method (already used at line 435 of the same file for the init-failure rollback path). Restores Architectural Invariant #1 (Three-Layer Backend) for this router. No behavior change — identical SQL, identical parameter binding. Closes #451 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dolho
added a commit
that referenced
this pull request
Apr 27, 2026
Move the pre-check exec logic out of routers/internal.py into a dedicated services/pre_check_service.py. Aligns with Invariant #1 (Router → Service → DB): the router becomes a 5-line passthrough, all business logic (path constant, two exec calls, stdout capping, contract dict) lives in the service. Same shape as services/slot_service.py, services/task_execution_service.py, services/monitoring_service.py — testable in isolation, discoverable via grep, won't trip /validate-architecture later. Behavior unchanged. 13/13 scheduler unit tests still pass; live endpoint hit returns same contract dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Apr 28, 2026
* feat(scheduler): agent-owned pre-check hook (#454) New optional contract: agents implement POST /api/pre-check in their container; scheduler calls it before firing a cron-triggered chat. Endpoint absent or any error → fire as usual (fail-open). fire=false records a skipped execution. fire=true with a message overrides the schedule.message for that invocation. - docker/base-image/agent_server/routers/pre_check.py: new router that dynamically loads /home/developer/.trinity/pre-check.py (template- supplied) and calls its check() function - agent-server main.py: mount pre_check_router - scheduler/agent_client.py: pre_check() method with fail-open semantics on 404/5xx/timeout/malformed-response - scheduler/service.py: _run_pre_check + pre-check branch in _execute_schedule_with_lock (cron only; manual triggers bypass) - tests/scheduler_tests/test_pre_check.py: 12 tests covering client- and service-level behavior; 161/161 scheduler suite passes Zero schema change — reuses existing ExecutionStatus.SKIPPED and create_skipped_execution. Closes the "wake agent on every cron tick" cost gap noted in docs/planning/PR_REVIEWER_AGENT.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(#454): scheduler pre-check feature flow + arch + requirements - feature-flows/scheduler-pre-check.md: new flow doc with contract, fail-open semantics, error table, testing summary - architecture.md: add /api/pre-check to agent-server endpoint list and pre-check note to Scheduler Service row - requirements.md: SCHED-COND-001 entry under §10 (Scheduling & Execution) - feature-flows.md: index row - docs/planning/PR_REVIEWER_AGENT.md: design doc from which this feature was extracted — committed for traceability Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review: address PR #455 review feedback - pre_check.py: asyncio.get_event_loop() → get_running_loop() (deprecated in 3.10+) - pre_check.py: oversized message override no longer dropped silently — response now carries message_truncated="override dropped: N bytes exceeds 32000 cap" so scheduler/operator can see what happened; log escalated to ERROR with size+limit details - pre_check.py: module-level docstring expanded to note the security scope of check() (full Python interpreter access, same sandbox as chat tools — operators should review .trinity/pre-check.py like any executable template file) and the intentional no-cache behavior - tests/unit/test_pre_check_router.py: 15 new router/unit tests covering oversized-message drop path and non-dict return → 500 (both previously only exercised by inspection). Uses importlib to load pre_check.py directly, avoiding python-multipart requirement from sibling routers - feature-flows/scheduler-pre-check.md: document truncation behavior, security scope expectation, and updated test summary (12 scheduler + 15 router = 176 total passing) Lock-scope concern noted in review is not an issue: the skip path returns from _execute_schedule_with_lock, and the outer _execute_schedule holds the lock in a try/finally that covers the return. No leak. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(#454): docker exec instead of agent-server HTTP endpoint Review feedback on #455 flagged that the HTTP-endpoint design introduced a new system edge (scheduler → agent-server direct) and a novel code- loading pattern (importlib in a router). Both broke with Trinity's established convention that all "run something in an agent container" flows go through `services/docker_service.execute_command_in_container` — the same primitive used by: - services/git_service.py (persistent-state allowlist, #384 S3) - services/ssh_service.py (key provisioning) - services/agent_service/terminal.py (web SSH) - routers/system_agent.py (admin exec) - adapters/message_router.py (Slack file ingest) - routers/voice.py, monitoring_service.py This commit swaps the design accordingly. Changes: - Delete docker/base-image/agent_server/routers/pre_check.py and its router registration. No new HTTP surface on agent-server. - Delete tests/unit/test_pre_check_router.py (router is gone). - Add src/backend/routers/internal.py → POST /api/internal/agents/{name}/pre-check. Runs the template-shipped `.trinity/pre-check.py` via execute_command_in_container. Two-step: `test -f` for existence, then `python3 .../pre-check.py`. Returns {hook_present, exit_code, stdout, stderr}. Gated by existing X-Internal-Secret header (C-003). - Rewrite src/scheduler/service.py::_run_pre_check to call the backend endpoint (scheduler no longer opens a direct edge to agent-server). Translates backend response to the same {fire, message, reason} shape so _execute_schedule_with_lock is unchanged. - Delete AgentClient.pre_check from src/scheduler/agent_client.py. - Rewrite tests/scheduler_tests/test_pre_check.py to mock the backend HTTP (httpx.AsyncClient) instead of the agent HTTP client. 13 tests covering translation: hook absent → None, non-zero exit → None, empty stdout → skip, non-empty stdout → fire with override, 404, 5xx, connection error, malformed JSON. - Update docs/memory/architecture.md §Agent Containers and §Background Services, docs/memory/requirements.md SCHED-COND-001, and docs/memory/feature-flows/scheduler-pre-check.md to reflect the new topology. feature-flows.md index updated. Template side (dolho/pr-reviewer-agent commit 4330d2a): .trinity/ pre-check.py rewritten as a standalone shebanged script that prints the chat prompt to stdout or exits 0 empty. Benefits: - Preserves "scheduler → backend → agent" topology (Invariant #11). - Uses Trinity's dominant pattern for agent-container command execution. - No importlib, no module caching, no pydantic contract — exit code + stdout is unambiguous. - Smaller code footprint: internal endpoint is ~50 lines, scheduler translation is ~50 lines, template script is ~50 lines. Test coverage: 162/162 scheduler suite passing (up from 161 — new case for malformed backend JSON). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(#454): make pre-check hook language-agnostic Drop the `python3` invocation from the backend's pre-check exec and rename the convention path from `~/.trinity/pre-check.py` to `~/.trinity/pre-check`. Trinity now execs the file directly — the interpreter is selected by the file's shebang line. Templates can ship Python, bash, node, or a compiled binary; Trinity stops caring about language. `test -f` (not `-x`) is kept for the existence check so a present-but-non-executable file surfaces as an exec failure (exit 126) in the operator log instead of silently falling through to the backward-compat "no hook" path. Docs (architecture / feature-flow / requirements / index) updated to reflect the new contract. Scheduler-side translation tests are unaffected — they assert the JSON contract, not the exec command string. 13/13 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(#454): extract pre_check_service from internal router Move the pre-check exec logic out of routers/internal.py into a dedicated services/pre_check_service.py. Aligns with Invariant #1 (Router → Service → DB): the router becomes a 5-line passthrough, all business logic (path constant, two exec calls, stdout capping, contract dict) lives in the service. Same shape as services/slot_service.py, services/task_execution_service.py, services/monitoring_service.py — testable in isolation, discoverable via grep, won't trip /validate-architecture later. Behavior unchanged. 13/13 scheduler unit tests still pass; live endpoint hit returns same contract dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 29, 2026
vybe
added a commit
that referenced
this pull request
Apr 30, 2026
* feat(webhooks): agent schedule webhook triggers (WEBHOOK-001, #291)
Add public webhook URLs so external systems (CI/CD, CRMs, monitoring) can
trigger agent schedule executions via a simple HTTP POST with no Trinity
account required — authenticated by a 256-bit opaque token embedded in the URL.
Changes:
- New public router POST /api/webhooks/{token}: rate-limited (10/60s per
token), audit-logged, 202 Accepted, delegates to existing scheduler trigger
- JWT-auth CRUD: POST/GET/DELETE /api/agents/{name}/schedules/{id}/webhook
- DB migration: webhook_token (TEXT UNIQUE), webhook_enabled (INTEGER DEFAULT 0)
on agent_schedules; partial unique index for O(1) token lookup
- Scheduler updated to accept triggered_by param in JSON body so executions
record triggered_by="webhook" correctly
- Webhook context field framed as data to reduce prompt injection surface
- 12 integration tests in tests/test_webhook_triggers.py
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(security): patch 4 Dependabot alerts — happy-dom + vite (#486)
Bumps two dev-only dependencies to patched versions. Production is
unaffected (happy-dom is test-only; vite only runs in local dev).
- src/frontend: vite ^6.0.6 → ^6.4.2 (closes Dependabot #55, CVE-2026-39363)
- tests/git-sync: happy-dom ^15.11.7 → ^20.9.0 (closes #83/#84/#85:
VM context escape RCE, ESM code exec, fetch cookie leakage)
Verified: frontend `vite build` clean, all 10 git-sync vitest tests pass
under happy-dom 20.9.0. No new critical/high alerts introduced.
Closes #485
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(webhooks): add WEBHOOK-001 to requirements and architecture (fixes #484 review)
Add missing documentation for the webhook trigger feature:
- requirements.md: WEBHOOK-001 entry with description, key features, DB changes,
API endpoints, security model, and feature flow link
- architecture.md: webhooks.py listed in Routers table; Schedules table expanded
from 9 to 12 endpoints with the 3 webhook management endpoints; new Webhook
Triggers section documenting the public POST /api/webhooks/{token} endpoint;
webhook_token and webhook_enabled columns added to agent_schedules schema block
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(#488): add status-in-dev label + PR-merge automation (#489)
Close the gap between "PR merged to dev" and "released to main".
- New GH Action `issue-status-on-merge.yml`: on PR merge to dev,
parse Fixes/Closes/Resolves #N from PR body+title, add
`status-in-dev`, remove `status-in-progress`.
- `/release` skill: read `gh issue list --label status-in-dev` as
the authoritative shipping list for release notes; include
`Closes #N` in the release PR body so issues auto-close on merge
to main.
- `DEVELOPMENT_WORKFLOW.md`: SDLC is now Todo → In Progress →
In Dev → Done, each stage mapped 1:1 to commit-graph location.
Fixes #488
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(channels): file upload Phase 2 — workspace delivery hardening (#487) (#494)
Phase 2 of #354 polishes the shared channel-agnostic file delivery path
in `message_router._handle_file_uploads`. Phase 1 (#355) added Telegram
extraction/download/validation; the actual workspace write path was
introduced for Slack inbound (#222). This change hardens the shared path
for both channels:
- New `_sanitize_filename` helper: NFKC unicode normalize → basename →
safe-chars regex → empty/dotfile fallback to `file_{id}` → 200-char
truncation preserving extension → collision dedup with `-1`, `-2`, …
- Spec injection format: `[File uploaded by {uploader}]: {name} ({size})
saved to {path}`. Uploader is the verified email when present
(Issue #311), else `adapter.get_source_identifier(message)`.
- All-writes-failed handling: when every workspace write attempt fails,
the router replies on the channel with an explicit error and skips
agent execution (#487 AC6). Validation rejections (size/MIME/download
errors) still surface in the description block as before.
- Audit log entries gain an `uploader` field.
Per-session upload directory (`/home/developer/uploads/{session_id}/`)
preserved — keeps user uploads isolated and ephemeral, matches the
existing #222 model.
Tests: +17 unit tests across `TestFilenameSanitization` (12),
`TestFileDeliveryFormat` (2), `TestFileDeliveryFailures` (3). 28/28
passing in `tests/unit/test_file_upload.py`.
Docs: `telegram-integration.md` Phase 2 section + revision row;
`slack-file-sharing.md` flow / router / errors / security sections
updated for the shared change; `feature-flows.md` index row.
Closes #487
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(webhooks): import Request in schedules router (#495)
The WEBHOOK-001 commits (c630931 / 8fdf736) added `request: Request`
parameters to `generate_webhook` and `get_webhook_status` without
importing `Request` from fastapi. Backend module import fails with
NameError on startup, blocking all dev deploys.
Integration tests in tests/test_webhook_triggers.py exercise these
endpoints but never caught the bug because the backend never starts —
test setup fails before any test runs.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(backlog): repair drain spawn — lazy-import target after #95 (#496) (#500)
services/backlog_service.py:240 lazy-imported _execute_task_background
from routers.chat, but #95 deleted that function. Every backlog drain
attempt failed with ImportError; the exception was swallowed at
backlog_service.py:218-228, so BACKLOG-001 (#260) was silently dead.
Live observation: 23 drain failures / 24h on a fan-out workload, only
surface signal was the per-execution `error` column.
Why it shipped silently: the unit happy-path test patched
sys.modules["routers.chat"] with a SimpleNamespace stub of whatever
attribute name it expected, masking the production breakage.
Changes:
- Lazy-import _run_async_task_with_persistence (the post-#95
replacement) and adjust the call shape (drop release_slot, drop
orphaned task_activity_id; the unified executor handles both).
- Capture self-task fields (is_self_task, self_task_activity_id,
inject_result) at enqueue time and rehydrate on drain so
SELF-EXEC-001 (#264) survives backlog overflow.
- Emit a stable log token `backlog_drain_spawn_failed` so log-based
detection (Vector / dashboards) can catch import drift or similar
spawn-time regressions at fleet scale rather than per-row.
- AST-based regression guard in tests/unit/test_backlog.py:
TestLazyImportTarget parses routers/chat.py and asserts the import
target exists; paired test asserts the lazy-import string matches
the validated allow-list. Catches both directions of drift without
booting the backend.
- Update happy-path test to use the new symbol and kwarg surface;
add self-task enqueue+drain round-trip tests.
Closes #496
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(announce): add Twitter/X support via API v2 + OAuth 1.0a
Bumps announce skill to v1.6. Adds a Python helper (scripts/post_twitter.py)
that reads tweet text from stdin and posts via Twitter API v2 using OAuth 1.0a
User Context — same exit-0/1 + structured-JSON contract as the existing
Discord/Slack/Telegram send paths so the sequential-only and no-blind-retry
rules apply uniformly. Credentials live in .env (gitignored) under
ANNOUNCE_TWITTER_* keys.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(chat): sync /task long-polls on backlog at capacity (#498) (#515)
Sync parallel `/task` calls (parallel=true, async=false) at capacity used
to fail terminally with HTTP 429 — they never touched the BACKLOG-001
backlog because the spill block was nested under `if request.async_mode:`.
Observed in production: ~40% terminal-failure rate from one MCP fan-out
caller (214 capacity rejections / 24h, 0 enqueues from 541 dispatches).
Sync calls now spill to the same backlog the async path uses and long-poll
on the open HTTP connection until the queued execution reaches a terminal
status, then return the result inline. True 429 only when the backlog is
also full. Total connection hold capped at 2 × effective_timeout.
Implementation:
- New `services/sync_waiter.py` owns the in-process registry and the
`signal_sync_waiter` / `wait_for_sync_terminal` primitives. Wait combines
an asyncio.Future (set by the drain finally block) with a 5s DB-poll
fallback that covers terminal flips routed outside the drain
(corrupt-metadata, expire_stale, cleanup recovery).
- `routers/chat.py` sync branch now mirrors the async branch:
pre-acquires the slot, on at-capacity calls `backlog.enqueue()` then
`wait_for_sync_terminal()`, returns the inline result on wake.
- `_run_async_task_with_persistence` wraps its body in try/finally and
signals any registered sync waiter with the rich TaskExecutionResult
plus chat_session_id. No-op when no waiter is registered (the common
async fire-and-forget path).
Tests (`tests/unit/test_chat_sync_backlog.py`, 13 new):
- Signal / wait / poll-fallback / timeout / cleanup / concurrent waiters
- Regression test pins TERMINAL_TASK_STATUSES to the enum so a new
TaskExecutionStatus value forces a deliberate update (caught a missing
SKIPPED entry pre-merge)
Trade-off (Policy B): worst-case connection hold doubles to
2 × effective_timeout when the request is queued. Honest envelope —
the caller chose to wait. Documented in the architecture diagram of
`persistent-task-backlog.md`.
Companion issue #505 covers the orchestration-education gap (MCP tool
description + platform prompt) so agents pick the right tool for the
job rather than relying on the platform absorbing every misuse.
Closes #498
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(groom): document SDLC stages and add status-label/board reconciliation
Adds SDLC context (Todo → In Progress → In Dev → Done) so grooming respects
in-flight work, and a Step 1b that reconciles status-* labels with board
columns (labels are authoritative).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(agent): classify signal-killed claude exits as 504, not fake auth failure (#517)
External signal terminations of the claude subprocess (timeout SIGKILL,
OOM-kill, parent SIGTERM, operator cancel) used to fall through to the
auth-fallback heuristics and surface as a misleading "Subscription token
may be expired" 503. Same shape as #361 (max-turns), different exit path.
Adds _classify_signal_exit() consulted before the auth heuristics: matches
Python-native signal exits (return_code < 0) and shell-encoded forms
(130/137/143 for SIGINT/SIGKILL/SIGTERM) and raises HTTP 504 with a clear
"killed by SIGKILL/SIGTERM/SIGINT — likely timeout, OOM, or operator
cancel" message. Tightens the zero-token heuristic with return_code > 0
so signal exits cannot reach it.
The bug became routinely reproducible after #61 (PR #326) added
backend-driven terminate_execution_on_agent() — every timeout now
produces a signal-killed claude subprocess on the agent side, which the
old heuristic block misclassified. Also de-risks PR #508 (auth-class
auto-switch): without this fix, every timeout would trigger an
unnecessary subscription rotation.
Backend's task_execution_service.py only flags AUTH on 503; 504 falls
through to the generic FAILED path. No backend changes required.
Closes #516
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(sprint): align skill with DEVELOPMENT_WORKFLOW.md (#519)
Four divergences between the /sprint playbook and the SDLC documented in
docs/DEVELOPMENT_WORKFLOW.md:
- Step 3 used `gh issue edit --add-label status-in-progress` directly,
bypassing .github/workflows/claim.yml and skipping self-assignment.
Now posts `/claim` as an issue comment, which is the workflow's single
source of truth for the In Progress transition.
- Step 8 invoked pytest directly via `cd tests && source .venv/bin/activate
&& python -m pytest …`. Now defers to `/test-runner [feature]`, with a
documented fallback for brand-new files outside the runner's catalog.
- Step 10 commit + PR body used `closes #N`. Workflow §1 specifies
`Fixes #N`; both auto-close on GitHub but the doc is the contract.
- Step 11 final report didn't mention the post-merge automation. Now
warns that issue-status-on-merge.yml owns the
status-in-progress → status-in-dev transition, so operators don't
manually edit labels post-merge.
Non-breaking: argument signature, automation level (gated), state
dependencies, and pipeline overview unchanged. Net +19/-11.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(agent): classify clean-exit empty-result as 502, not silent success (#520) (#521)
* fix(agent): classify clean-exit empty-result as 502, not silent success (#520)
Sibling of #516/#517 on the return_code == 0 path. When the claude
subprocess exits 0 but the final {"type":"result"} JSON line is dropped
before the reader thread captures it (typical cause: a child subprocess
inherited stdout, kept the pipe open past claude exit, the reader thread
leaked, the pgroup unwind closed the pipe), metadata.cost_usd and
metadata.duration_ms stay None. The success path used to return HTTP 200
anyway — agent-server logged "completed successfully" while backend
silently reaped the execution as an orphan minutes later, masking the
real failure with a misleading "completed on agent but recovered by
watchdog" message.
Adds _classify_empty_result(metadata, raw_message_count) consulted after
the return_code != 0 block (#516 + auth heuristics) and before response
building. When both cost_usd and duration_ms are None, raises HTTP 502
with diagnostic context (tools, turns, raw_messages, cause hint).
Backend's task_execution_service.py:542 only flags AUTH on 503, so 502
falls through to the generic FAILED path with the helpful detail
preserved — no backend changes needed.
The two-field check is conservative: single-field nullability could be a
Claude format quirk; both-None is a strong signal that the terminal
result message never arrived. Test coverage pins the scope so a future
edit can't silently broaden it.
Changes:
- docker/base-image/agent_server/services/claude_code.py — new
_classify_empty_result() helper next to _classify_signal_exit; call
site between the return_code != 0 block and response building.
- tests/unit/test_empty_result_classification.py — 9 new tests, all
pass. Covers both-None → 502, populated metadata → None,
single-field-only → None (Claude format quirk tolerance), zero-cost
and zero-duration → None (is None vs falsy), missing metadata → None.
- docs/memory/feature-flows/parallel-headless-execution.md — changelog
entry under Recent Updates.
- docs/memory/feature-flows/task-execution-service.md — row in
error-translation table + new Empty-Result Pre-Check paragraph.
Requires base-image rebuild after merge:
./scripts/deploy/build-base-image.sh
Closes #520
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(feature-flows): index entry for agent error classification (#516, #520)
Combined Recent Updates entry covering the matching pair of agent-side
error-classification fixes that shipped this week — _classify_signal_exit
(#516, PR #517) and _classify_empty_result (#520, PR #521). Both touch
docker/base-image/agent_server/services/claude_code.py and share the
"agent surfaces the right HTTP status so backend records FAILED with a
useful detail" theme.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(agent): off-load synchronous terminate cleanup off the event loop (#523)
The async terminate_execution endpoint and the outer asyncio.TimeoutError
handlers in execute_claude_code and execute_headless_task were calling
registry.terminate() / _terminate_process_group() / _safe_close_pipes()
synchronously. Those helpers do up to 7s of process.wait() (SIGINT grace
+ SIGKILL grace), which blocks the asyncio event loop for the entire
window. While blocked, agent-server cannot serve /health, the backend
circuit breaker opens, and UI fan-out hangs for 5+ minutes per page.
This is the actual user-visible mechanism behind the #523 "agent-server
wedge" symptom, not the FD-inheritance / leaked-reader-thread theory in
the original report (see issue comment for the corrected diagnosis —
the FD_CLOEXEC fix as written would not have helped because dup2 strips
CLOEXEC during the child's stdout setup, and the existing post-#407
killpg + safe_close path actually works in the vast majority of cases).
Wrap the three call sites in loop.run_in_executor(None, ...) so the
blocking process.wait() runs on a thread-pool worker. Pipe-inheritance
fragility remains a slow-burn cleanup item to be filed separately.
- routers/chat.py: terminate_execution dispatches registry.terminate to
the default executor
- services/claude_code.py: outer-timeout cleanup in both async paths
off-loads _terminate_process_group + _safe_close_pipes
- tests/unit/test_terminate_async_executor.py: regression test asserts
registry.terminate runs on a non-event-loop thread and the event-loop
yield stays sub-50ms while terminate is in flight
Fixes #523
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(planning): add Tier 2.6 hardening + actor-model destination roadmap
Records the architectural critique from 2026-04-26 review:
- Tier 2.6 (Sprint D′): #524 state machine contract, #525 idempotency
keys, #526 dispatch circuit breaker. Closes the three contract-level
gaps that survive even after Sprint D's plumbing consolidation.
- Future considerations: 7 unranked recommendations (durable
ProcessRegistry, retry-in-funnel, synchronous terminate ack, dual
streams, fairness, EventBus backpressure, lifecycle contract doc).
- Target architecture section: names the actor model as the destination
(mailbox + journal + processor), maps existing components to the
concepts they already implement, defines a 4-phase gated transition
roadmap, and gates Phase 2 (agent-to-agent experiment) on a one-page
message-envelope + journal-format postcard.
Issues: #524, #525, #526 created and added to project board (Epic
#411 Orchestration Invariants, Theme Reliability).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(planning): mark #291 (WEBHOOK-001) shipped, Sprint C now 5/5
#291 closed 2026-04-24, shipped via PR #484 (token-in-URL trigger
through TaskExecutionService) with follow-up fix PR #493. The plan
doc still listed it as the next item to pick up; align it with the
ground truth and re-aim "what to do next" at #428 (after #306 soak)
plus Tier 2.6 hardening (#524/#525/#526) in parallel.
* refactor(capacity): consolidate three queue/slot primitives into CapacityManager (#428) (#527)
* refactor(capacity): consolidate ExecutionQueue + SlotService + BacklogService into CapacityManager (#428)
Single public facade for agent execution capacity. Composes SlotService
(Redis ZSET counter) and BacklogService (SQL persistent overflow) as
private internals; owns the in-memory overflow store (Redis LIST, depth 3,
lifted from the deleted ExecutionQueue).
Why:
- 7 caller sites now go through one API instead of orchestrating three.
- Each new trigger type (retry, webhook, self-exec, fan-out) gets one path
for capacity, not a choice between three primitives.
- Unblocks #429 (CLEANUP-COLLAPSE) and the actor-model destination by
reducing the surface a single capacity store has to expose.
API:
capacity.acquire(agent, exec_id, max_concurrent, *,
overflow_policy='reject'|'queue_in_memory'|'queue_persistent',
overflow_payload=PersistentTaskPayload(...))
capacity.release(agent, exec_id) # idempotent
capacity.release_if_matches(agent, eid) # TOCTOU-safe (watchdog)
capacity.get_status(agent, max_concurrent)
capacity.reclaim_stale(agent_timeouts) # called by cleanup_service
capacity.force_release(agent) # emergency
capacity.cancel_all_overflow(agent, reason) # agent deletion
capacity.run_maintenance(max_age_hours) # 60s tick from main.py
Wire format unchanged: same Redis keys (agent:slots:*, agent:queue:*),
same SQL columns (schedule_executions.queued_at, backlog_metadata).
In-flight executions unaffected; clean revert path.
Deviations from issue spec (user-approved):
- No feature flag — single runtime path. dev-soak + clean revert is the
rollback mechanism, simpler than a per-agent DB column + flag check at
every call site.
- ExecutionQueue deleted in this PR rather than separate cleanup PR.
SlotService and BacklogService kept as private internals (well-factored,
one job each).
Soak deviation: shipped after 5 days of #306 soak rather than the planned
14 days. Mitigated by additive-style refactor (no wire-format change).
Files:
- NEW services/capacity_manager.py (~480 LOC)
- DELETE services/execution_queue.py (~360 LOC)
- 7 caller migrations: routers/chat.py (4 sites), routers/agents.py (2),
routers/agent_config.py (1), services/cleanup_service.py (4),
services/task_execution_service.py (1), services/agent_service/queue.py (3),
main.py (1, callback wiring is now internal).
- NEW tests/unit/test_capacity_manager.py — 21 tests covering acquire/release
for all three overflow policies, drain wiring, status, force_release,
reclaim_stale, cancel_all_overflow.
- UPDATE tests/test_watchdog_unit.py — 11 mock decorator pairs collapsed to
single get_capacity_manager mock.
Tests: 21 new + 35 watchdog + 33 backlog = 89 green for affected surface.
Fixes #428
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(feature-flows): add capacity-management.md, deprecate predecessor flows (#428)
- NEW capacity-management.md — public surface, overflow policies, end-to-end
/chat and /task flows, storage map, maintenance & recovery, what-replaced-what.
- DEPRECATE notes on the three predecessor flows with redirects:
- execution-queue.md (ExecutionQueue deleted)
- parallel-capacity.md (SlotService internalized)
- persistent-task-backlog.md (BacklogService internalized)
- "Now uses CapacityManager" notes on four downstream flows:
- task-execution-service.md, parallel-headless-execution.md,
cleanup-service.md, execution-termination.md
- Index: Recent Updates row + Core Agent Features row for capacity-management.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(requirements): note BACKLOG-001 is now internal to CapacityManager (#428)
Section 10.8 (Persistent Task Backlog) — replace direct SlotService callback
reference with the unified CapacityManager facade. Status bumped with the
2026-04-26 internalization date and #428 cross-ref.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(agent): drain pipe before close to preserve final result line (#531) (#532)
* fix(agent): drain pipe before close to preserve final result line (#531)
drain_reader_threads previously called safe_close_pipes() immediately
after terminate_process_group(), discarding the kernel pipe buffer before
the reader thread could drain it. On long agentic tasks the final
{"type":"result"} JSON line (cost, duration, answer) was in that buffer
at the moment of close, causing the reader to raise ValueError and
metadata.cost_usd / duration_ms to remain None — triggering the HTTP 502
"Execution completed without a result message" classification from #521.
Fix: reorder so grandchildren are killed first, then the reader is given
post_kill_grace=30s to drain naturally (grandchildren dead → kernel
delivers EOF once the buffer is consumed → reader returns '' and exits
cleanly). safe_close_pipes() is now a true last resort — only called when
the reader is still alive after 30s, which indicates a genuine wedge, not
unfinished backlog drain.
Also extends _classify_empty_result to derive num_turns from raw_messages
when metadata.num_turns is None (result line lost), so the 502 detail
reports an honest turn count instead of always showing 0.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* docs(tests): update test catalog for #531 drain_reader_threads fix
- Add test_subprocess_pgroup.py and test_empty_result_classification.py
to Test Categories (Operations & Observability, unit section)
- Add 2026-04-27 Recent Test Additions entry with description of the
pipe-drain ordering regression tests and raw_messages fallback tests
- Update unit test count: 165 → 170; total: 2,257 → 2,262
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* docs(feature-flows): document drain_reader_threads pipe-ordering fix (#531)
Update parallel-headless-execution.md with the root cause fix for
the "Execution completed without a result message" HTTP 502: the old
drain_reader_threads sequence closed the pipe before the reader could
drain the kernel buffer (including the final result JSON line). New
sequence: kill grandchildren → natural drain (post_kill_grace=30s) →
force-close only as last resort.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(tests): update orphaned-recovery mocks to CapacityManager (#533) (#534)
Replace stale services.slot_service sys-mock with services.capacity_manager
so all four recovery scenario tests pass after the #428 consolidation.
Assertions updated from release_slot → release to match the new API.
Fixes #533
Co-authored-by: Claude <noreply@anthropic.com>
* docs(skills): align validate-pr with DEVELOPMENT_WORKFLOW.md
Add quick triage block, base branch check, PR size warning, type-docs
label, Base Branch/PR Size rows in report table, and review pipeline
matrix linking /review and /cso --diff with their complementary roles.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(validate-architecture): stale-citation filter + dedupe guard (#511) (#513)
* fix(validate-architecture): add stale-citation filter + issue dedupe guard (#511)
The /validate-architecture skill produced false-positive issue #479 by:
1. citing file paths the report's snapshot saw, but `main` no longer has
(process engine deleted in #430 the same day);
2. running `gh issue create` with no check for existing open issues with
the same finding fingerprint.
Two targeted edits to .claude/skills/validate-architecture/SKILL.md:
- New Step 2c "Filter Stale Citations" — `git ls-files --error-unmatch`
every cited path before report. Drop ghosts. Downgrade FAIL → PASS
when an invariant has zero remaining real citations.
- Modified Step 4 — fingerprint = sorted invariant numbers; query
open `automated,priority-p1` issues with `--search "in:body
validate-architecture fingerprint=<fp>"`; comment on existing
issue instead of creating duplicate. Issue body now stamps the
current commit SHA for evidence binding.
Closes #511.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(validate-architecture): clarify dedupe branching, distinct fingerprint marker, quote paths (#511)
Follow-up to review feedback on PR #513. Three small skill-prose
hardenings:
- I2 (LLM-driven flow control): the dedupe branch previously relied on
`if [ -n "$EXISTING" ]; then ...; exit 0; fi` followed by a separate
create block. `exit 0` halts a bash subshell, not an LLM walking the
markdown — a future runner could execute both blocks. Replace with
explicit "Path A — COMMENT, then STOP" / "Path B — CREATE" prose
branching and an explicit DO-NOT note.
- I4 (fingerprint collision): replace free-text body search
`validate-architecture fingerprint=$FP` with HTML-comment marker
`<!-- validate-architecture::fingerprint=$FP -->` plus a quoted-phrase
search. Self-evidently programmatic; won't collide with prose.
- I3 (path quoting): the Step 2c example now uses `"$path"` and a note
about shell metachars, so implementers don't strip the quotes.
- Add concurrency caveat documenting that the dedupe is best-effort,
not atomic (no GitHub primitive provides this).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(config): clean stale Auth0 / AUDIT_URL, document SMTP/SendGrid/FRONTEND_URL (#481) (#509)
- Remove dead Auth0 env vars + build args from docker-compose.prod.yml
and docker/frontend/Dockerfile.prod (Auth0 removed 2026-01-01). The
build-arg fallbacks were also leaking a real Auth0 domain + client ID
into a public repo.
- Drop AUDIT_URL from .env.example (audit-logger service no longer exists;
no Python references it).
- Add FRONTEND_URL to .env.example (required in prod for OAuth post-auth
redirects in slack_service.py / public_links.py and SSH host
auto-detection in ssh_service.py).
- Document SMTP_HOST/PORT/USER/PASSWORD and SENDGRID_API_KEY in
.env.example so the advertised EMAIL_PROVIDER=smtp/sendgrid modes are
actually configurable from the template.
Closes #481
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): require auth on /api/docs endpoints (#452) (#507)
Add Depends(get_current_user) to the three handlers in
src/backend/routers/docs.py so the file no longer violates
Architectural Invariant #8.
Note: this router is not currently registered in main.py, so
the endpoints are not reachable on the running API. The fix is
applied to the file as written so the invariant validator stops
flagging it and so the file is correct if it is ever remounted.
Closes #452
* fix(chat): wrap long unbroken strings in chat bubbles (#457) (#502)
Long URLs, tokens, base64 blobs, and other unbroken strings in agent
chat responses were overflowing their 85% bubble and forcing horizontal
scroll on the entire Chat tab.
Root cause: ChatBubble.vue capped the bubble width but never told
inner content how to handle unbreakable strings. Inline <code> and
the user-text <p> had no overflow-wrap; <pre> defaulted to white-space:
pre with no overflow-x: auto override.
Fix (CSS-only, all 3 render branches — user / self-task / assistant):
- min-w-0 on outer wrapper, overflow-hidden on inner bubble
- break-words on user text and prose container
- prose-pre:overflow-x-auto + prose-pre:max-w-full so code blocks
scroll inside the bubble instead of expanding it
- prose-code:break-words for long inline tokens
- prose-a:break-words for long URLs in markdown links
Verified visually: before/after static test page shows BEFORE leaks
content well past the bubble border; AFTER wraps cleanly with no
regression on normal markdown (headings, lists, links, short code).
* fix(schedules): add missing Request import for webhook endpoints (#493)
Regression from c630931 (WEBHOOK-001 / #291): `routers/schedules.py`
uses `Request` as a type annotation on the `generate_webhook` and
`trigger_webhook` handlers but never imports it, so the module fails
to load and the backend won't start with a NameError.
Minimal fix: add `Request` to the existing `from fastapi import …`
line (line 11). No behavioral change — the annotation was already
intended.
Surfaced while dev-testing FILES-001 (PR #491). uvicorn reload pulled
in the dev branch state and blew up.
Co-authored-by: Pavlo <pash@pashs-MacBook-Pro.local>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(git): route orphan cleanup through db.delete_git_config (#451) (#501)
Replaces raw `DELETE FROM agent_git_config` SQL in routers/git.py with the
existing `db.delete_git_config()` method (already used at line 435 of the same
file for the init-failure rollback path). Restores Architectural Invariant #1
(Three-Layer Backend) for this router. No behavior change — identical SQL,
identical parameter binding.
Closes #451
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(subscription): auto-switch on first failure + auth-class triggers (#441) (#508)
Drop the 2-consecutive-429 gate in `subscription_auto_switch` so a single
subscription failure now triggers a switch — the 2h skip-list on
alternative selection (already pinned by #444 / #476 regression tests) is
sufficient as the lone thrash guard. Broaden the trigger surface to also
fire on auth-class failures (401/403/credit balance/expired OAuth token,
etc.), classified via a centralized `AUTH_INDICATORS` list, so a broken
subscription auto-recovers instead of failing every execution until
manual intervention. Flip the `auto_switch_subscriptions` default to
"true" — operators can still opt out, but the safe behavior is now the
default. Backward-compat shim `handle_rate_limit_error` preserved for
existing 429 callers.
- services/subscription_auto_switch.py: new `handle_subscription_failure`
with `failure_kind` dispatch ("rate_limit" | "auth"); new
`is_auth_failure` classifier; default flipped; notification + log
wording adapts per kind; old shim retained.
- services/task_execution_service.py: 503 / auth-classified errors now
also call the switch path alongside 429.
- routers/chat.py (sync): same broadening on the interactive chat
surface; auth path returns 503+retry hint mirroring the 429 UX.
- routers/subscriptions.py: GET `/auto-switch` default also flipped to
"true" so the UI toggle and runtime gate read the same value.
- scheduler/service.py: dedupe two inline `auth_indicators` copies into
a single module-level constant; cross-reference the canonical list in
backend (cross-container import not viable).
- tests/unit/test_subscription_auto_switch_pingpong.py: new
TestIsAuthFailure + TestSingleEventThreshold classes (8 new tests, all
pingpong + #476 aging tests still green).
- tests/test_subscription_auto_switch.py: flip default-off → default-on.
- docs: SUB-003 feature flow + requirements doc reflect the new
threshold, broadened scope, and on-by-default behavior.
Closes #441
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(feature-flows): clean up #95 drift missed by #500 (#496) (#503)
* fix(backlog): repair drain spawn after #95 rename (#496)
`services/backlog_service.py:_spawn_drain` was lazy-importing
`_execute_task_background` from `routers.chat`, but #95 (PR #316) deleted
that function and replaced it with `_run_async_task_with_persistence`.
Every backlog drain raised `ImportError`, was caught at line 218-228, and
silently marked queued executions FAILED — leaving BACKLOG-001 (#260)
non-functional whenever an agent hit capacity.
Rewire the lazy import to the new helper and adjust the call shape:
- drop `task_activity_id` (not in new signature; chat router already
passes None at enqueue)
- drop `release_slot=True` (the wrapper passes `slot_already_held=True`
to TaskExecutionService, which manages release in its finally block)
- derive `is_self_task` from x_source_agent vs agent_name
- pass `self_task_activity_id=None` (queued items don't carry one;
separate gap, not in scope here)
Add `tests/test_backlog_drain_unit.py` with five regression checks:
two AST-based contract tests that pin the function name and signature
in `routers/chat.py` (would have caught the original break), and three
runtime spy tests covering the kwarg shape `_spawn_drain` forwards. The
existing `tests/unit/test_backlog.py::test_drain_happy_path_spawns_background`
is updated to match the new contract.
Sync the BACKLOG-001 and TaskExecutionService feature-flow docs to
reference the renamed helper.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(feature-flows): sync index + parallel-capacity for #496
- Add #496 entry to feature-flows.md Recent Updates.
- Fix two more stale `release_slot=True` references in
parallel-capacity.md left over from #95 — the param never existed
on `_run_async_task_with_persistence` (slot release happens inside
TaskExecutionService via slot_already_held=True).
Other stale `release_slot=True` references in
authenticated-chat-tab.md and parallel-headless-execution.md are
deeper drift (separate flows, not touched by #496) — leave for a
follow-up doc-cleanup pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(catalog): register test_backlog_drain_unit.py (#496)
Adds the new BACKLOG-001 regression test file to tests/registry.json
so it shows up in the catalog alongside test_event_bus.py and
unit/test_backlog.py.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: drop redundant test_backlog_drain_unit.py
PR #500 (which superseded the original #496 fix scope) shipped
equivalent contract coverage with a more robust setup:
- `TestLazyImportTarget` (AST guard for the lazy-import target)
- `test_drain_threads_self_task_fields` (round-trip via real
BacklogService against sqlite)
The local file used sys.modules stubs which were strictly weaker.
Keeping it would only add maintenance burden for duplicate coverage,
so drop the file and its registry entry. Net effect on PR #503 is
that it becomes a small, focused docs-cleanup PR (parallel-capacity.md
and task-execution-service.md drift from #95, plus the missing
Recent Updates entry for #496).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(deploy): align scripts, configs, and docs with production operating patterns (#504)
* docs(generate-user-docs): add hub-and-spokes deployment structure and ops-pattern import
Restructure skill to produce guides/deploying/ as a hub plus six spokes
(local-development, single-server, public-access, upgrading,
backup-and-restore, monitoring) instead of one flat deploy guide.
Add an "operational guide" template (When to Run → Pre-flight →
Procedure → Verify → Rollback) for procedural docs that don't fit the
feature-shaped dual-audience template, plus verbatim-reuse snippets for
the load-bearing rules: never down/up, rebuild platform services only,
six-probe verification, resource-thresholds table, alpine cp backup.
Add Step 2h to draw operational patterns from the private ops runbook
under ../trinity-ops/, with explicit safe/forbidden import lists and a
sshpass→localhost rewrite rule.
Strengthen Step 2e to cross-check .env.example keys against each
compose's environment block — docs must not promise behavior the
chosen compose can't deliver.
Add public-safety greps in Step 7 (sshpass, trinity-ops, tailnet, real
IPs, instance-dir refs) so leaked private detail blocks completion.
Tracks issue #504.
* fix(deploy): align scripts, configs, and docs with production operating patterns (#504)
Fixes the first-run blocker (agent creation fails silently without base
image) and removes references to the removed audit-logger service that
caused verify-platform.sh and validate.sh to always fail.
Scripts:
- start.sh: detect missing base image and auto-build on first run; use
`docker compose stop` in help text (not `down`, which destroys agents)
- verify-platform.sh: full rewrite — remove trinity-audit-logger and port
8001 audit checks; fix frontend from port 3000 → 80; check scheduler
health at :8001; add MCP/Vector probes; fix login hint
- validate.sh: remove non-existent `deployment/` dir, `QUICK_START.md`,
and `src/audit-logger/audit_logger.py` from required paths; fix port 3000
Config:
- docker-compose.yml: wire 5 missing env vars into backend (PUBLIC_CHAT_URL,
FRONTEND_URL, EXTRA_CORS_ORIGINS, SLACK_SIGNING_SECRET, SSH_HOST)
- .env.example: remove stale AUDIT_URL; annotate prod-only / overlay-only
vars (SLACK_SIGNING_SECRET, PUBLIC_CHAT_URL, FRONTEND_URL, SSH_HOST,
TRINITY_GIT_BASE_URL) so users know scope before setting them
Docs:
- deploying-trinity.md: add explicit build-base-image.sh step; fix
/trinity:connect to use MCP API key flow (not username/password); add
Upgrading, Health Verification, Resource Thresholds, and Common Recovery
Patterns sections from ops runbook; use `docker compose` (v2 syntax)
- setup.md: remove false claim that start.sh builds the base image; correct
admin account creation (env var driven, not wizard); clarify wizard path
(used only when ADMIN_PASSWORD is unset)
New file:
- quickstart.sh: interactive one-command setup (checks Docker, generates
secrets, sets ADMIN_PASSWORD, builds base image, starts services, verifies)
Skill:
- generate-user-docs: add deployment config reading rules (read scripts
literally; cross-check env vars vs compose; never claim "auto" unless code
proves it); add operational guide template (pre-flight/steps/verify/rollback);
resolve conflict preserving the hub+spokes guide structure from branch
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(generate-user-docs): remove private repo name from SKILL.md
Replace explicit `trinity-ops` repo references with generic path aliases
(`../ops-runbook/`) so the private repo name is not embedded in this
public repository.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(reliability): CAS guards on execution status writes + state machine doc (#524) (#541)
Closes the FAILED→SUCCESS and SUCCESS→FAILED races that were patched by
#378 re-verify logic without eliminating the root cause.
Changes:
- update_execution_status: SUCCESS writes are unconditional (agent wins);
non-success terminal writes blocked when row already terminal
- mark_stale_executions_failed / mark_no_session_executions_failed: inner
UPDATE gains AND status='running' to close the SELECT→UPDATE TOCTOU window
- _recover_execution: routes through mark_execution_failed_by_watchdog
(already CAS-guarded) instead of bare update_execution_status
- TaskExecutionStatus: state machine, transitions, and authorized writers
documented in docstring; PENDING_RETRY added to enum
- Remove now-dead _STALE_SLOT_ERROR_PATTERN constant
Full projector architecture (ExecutionStateProjector, agent event emission,
projected_status shadow column) deferred — agents have no Redis access and
the restart-recovery design needs more thought before those land.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(public-chat): build context before storing user message to prevent duplication (#539) (#540)
* fix(public-chat): build context before storing user message to prevent duplication (#539)
In the public chat endpoint, the user message was persisted to the database
before build_public_chat_context read from it, causing the current message
to appear twice in every agent prompt — once in "Previous conversation:"
and once in "Current message:". Reordering the calls so context is built
first (from prior history only) then the user message is stored eliminates
the duplicate on every turn.
Adds unit tests that document both the old broken order (two occurrences)
and the corrected order (one occurrence), guarding against regression.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* docs(feature-flows): update public-agent-links with #539 context ordering fix
- Correct PUB-005 data flow: build_public_chat_context before add_public_chat_message
- Update backend implementation step ordering to match fixed code
- Add revision history entry for the bug fix
- Add #539 entry to feature-flows.md index
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(agent-runtime): guard content_block isinstance in process_stream_line (#542) (#543)
Prevents AttributeError crash when Claude Code stream-json emits a
string element inside a message content array. Guards both
process_stream_line (real-time path) and parse_stream_json_output
(batch path) using the same isinstance(block, dict) pattern already
used by the error_content loop.
Fixes #542
Co-authored-by: Claude <noreply@anthropic.com>
* docs(#411): Phase 1 canary harness design + catalog Phase 1 subset additions (#544)
* docs(#411): Phase 1 canary harness design + catalog Phase 1 subset additions
- New design doc at docs/planning/CANARY_HARNESS_PHASE_1.md scoping the
AC-required infrastructure (snapshot collector, canary_violations table,
canary agent template, fleet, alerts) for the three required invariants
(S-01, E-02, L-03).
- Catalog Phase 1 subset expanded 10 → 12: adds S-03 (slot TTL ≥ exec
timeout, catches #226) and E-05 (dispatched rows have session, catches
#106), since both bugs are cited in the catalog motivation but had no
Phase 1 detector.
* docs(#411): scope fleet to strict minimum for AC's 3 invariants
* docs(#411): expand design doc to cover full Phase 1 (12 invariants, snapshot format)
* feat(settings): add Remove buttons for stored API keys + Slack (#459) (#483)
Settings page lets admins save/test Anthropic API Key, GitHub PAT, and
Slack OAuth credentials, but exposed no UI to clear them once stored.
Only workaround was calling DELETE endpoints directly or editing the DB.
Adds Remove buttons next to Save in each row, conditionally rendered
when the value lives in settings DB (source === 'settings'). Env-var
fallbacks stay uneditable from UI. Confirm dialog before deletion
(reuses ConfirmDialog component + pattern from ApiKeys.vue).
Backend DELETE endpoints already existed — no backend work:
- DELETE /api/settings/api-keys/anthropic
- DELETE /api/settings/api-keys/github
- DELETE /api/settings/slack
Audit of other Settings sections: Trinity Prompt has clearPrompt,
Skills Library blanks via deleteSetting, MCP URL has resetMcpUrl,
GitHub Templates/Email Whitelist have inline remove. Agent Quotas are
config values, not secrets.
Closes #459.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(migrations): swallow duplicate-column race on cold start (#456) (#537)
* fix(migrations): swallow duplicate-column race on cold start (#456)
`_migrate_sync_health` (#389) used a check-then-act PRAGMA → ALTER
pattern that is not atomic across uvicorn workers. On cold start with
`--workers 2`, both workers passed the PRAGMA before either committed
the ALTER, and the loser crashed its child process with
`sqlite3.OperationalError: duplicate column name: auto_sync_enabled`.
Fix:
- Add `_safe_add_column` helper that swallows the duplicate-column
OperationalError (treats it as success — another worker won the race).
Future migrations should route ALTER TABLE ADD COLUMN through it.
- Refactor `_migrate_sync_health` to use the helper for both column
additions and switch the bare `CREATE TABLE` to `CREATE TABLE IF NOT
EXISTS` (atomic in SQLite).
Tests:
- `test_safe_add_column_swallows_duplicate_column_race` — drives the
exact production race via a PRAGMA-lying cursor proxy.
- `test_safe_add_column_propagates_other_errors` — non-duplicate errors
still raise.
- `test_safe_add_column_returns_true_when_added` — happy path.
- `test_migrate_sync_health_idempotent_under_race` — `_migrate_sync_health`
is now safe to re-run on already-migrated schemas, including under
the simulated race.
The other ~50 ALTER ADD COLUMN sites are untouched: they're already
applied on production DBs (run_all_migrations short-circuits via the
schema_migrations tracking table). The race only bites new migrations
on first cold-start; the helper is available for them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(migrations): route all ALTER ADD COLUMN through _safe_add_column (#456)
Mechanical sweep of every check-then-act `PRAGMA table_info` →
`ALTER TABLE ADD COLUMN` site through the `_safe_add_column` helper, so
new migrations on a fresh cold-start with `--workers N` are race-safe by
default — not just `_migrate_sync_health` (the originally reported case).
22 migrations refactored. Bare `try/except Exception` swallows in
`_migrate_chat_messages_source_column` and
`_migrate_agent_ownership_voice_prompt` are also replaced with the
helper, which catches only the duplicate-column error instead of every
exception class.
Verification:
- Schema dump (init_schema + run_all_migrations on fresh DB) is
byte-identical before and after the sweep — every column type,
default, FK, and index preserved.
- run_all_migrations is idempotent across runs and across fresh
connections (2nd/3rd runs print no add/create lines).
- 6-worker concurrent stress test (`threading.Barrier`-coordinated)
completes without any worker crashing; final schema is intact.
- tests/unit/test_migrations_concurrent.py +
tests/unit/test_migrations.py + tests/unit/test_guardrails.py:
72 pass, 0 fail.
`tests/unit/test_guardrails.py::test_migration_is_idempotent` updated
to also exec the `_safe_add_column` helper into its isolated
namespace, since the migration now delegates to it.
The two remaining bare `CREATE TABLE` calls in the file
(`_migrate_agent_sharing_table`, `_migrate_agent_skills_table`) are
one-time DROP+CREATE data-recreation migrations that already shipped
on every existing install; they are out of scope for this sweep.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(git): UI Push no longer commits runtime state (#462)
Expands the platform .gitignore deny-list to cover all runtime files
(.env, .mcp.json, .credentials.enc, instance dirs, content/, Claude
Code state, temp files). Adds idempotent migration that updates
existing agents on next Push and calls `git rm --cached` for files
that are now tracked but newly ignored.
Closes #462
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(frontend): semantic status color tokens (#67) (#553)
Introduce 5 semantic status tokens (`status-success/warning/danger/info/urgent`)
in tailwind.config.js as direct aliases of the green/yellow/red/blue/orange
palettes, then migrate 9 frontend files (4 components, 2 panel-local helpers,
1 composable, 1 utility) from raw color classes to the new tokens. Visual
output is byte-equivalent — tokens compile to identical RGB values.
Add CI safety net: `npm run check:tokens` script verifies token-palette
equivalence and catches typo'd token references in source. Wired into a new
frontend-build.yml workflow that runs `npm ci → check:tokens → build` on PRs
touching `src/frontend/**`.
Drive-by fix: rename postcss.config.js → postcss.config.mjs to fix Node
ESM/CJS interop for local `npm run build` (production Docker build was
masking the issue).
70+ raw-color files remain for follow-up sweep (per autoplan phasing).
Fixes #67
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(files): FILES-001 outbound file sharing — MVP + Phase 1 hardening (#491)
* feat(files): FILES-001 outbound file sharing MVP (Steps 1-6)
Implements outbound file sharing per docs/drafts/amazing-file-outbound.md:
- Schema + migration (agent_shared_files table with FK cascade)
- Per-agent opt-in toggle + Docker publish volume (agent-{name}-public)
- Internal share endpoint with path/MIME/size/quota validation
- Public download endpoint (/api/files/{id}?sig=...) with token auth
- share_file MCP tool (agent-scoped)
- SharingPanel UI: toggle, list, revoke, copy URL
Live-verified on Slack: agent→share_file→URL→download end-to-end.
Unit tests: 33 passed (migration, mixin, mount-match).
Known limitations + production readiness plan:
docs/drafts/amazing-file-outbound-production-readiness.md
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(files): FILES-001 — requirements, architecture, feature-flow doc
- requirements.md §13.10 new entry marking FILES-001 Implemented (2026-04-24)
- architecture.md: add files.ts MCP module, agent_shared_files_service,
routers/files.py, the 5 new API endpoints + dedicated section, and the
agent_shared_files table schema + operational notes
- feature-flows.md: Recent Updates entry + Documented Flows index
- feature-flows/file-sharing-outbound.md: new full vertical-slice doc
(UI → store → router → service → DB → download) matching the template
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* security(files): cap filename length at 255 chars (C2)
ShareFileRequest.filename and ShareFileMcpRequest.filename get
Field(max_length=255, min_length=1). display_name same cap.
Prevents 10KB+ filename edge cases from agent or attacker.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* security(files): disk-space pre-check before write (C3)
New check_disk_space() helper using shutil.disk_usage('/data').
Refuses writes when /data has less than size_bytes + 500MB free
(HTTP 507 Insufficient Storage). Called before persisting.
Protects shared /data mount — SQLite DB, Vector logs, and log
archives live there too; letting an agent fill the disk causes
platform-wide outage, not just a failed share.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(cleanup): purge expired and old-revoked shared files (C4 / Step 7)
Adds delete_expired_and_revoked(revoke_grace_hours=24) to the DB ops
class (returns stored_filename list for disk unlink) + facade forward
+ wired into cleanup_service.py's 5-min tick.
Per cycle:
- SELECT rows where expires_at < now OR revoked_at < now - 24h
- DELETE them from DB
- unlink each /data/agent-files/{stored_filename}
- bumps CleanupReport.shared_files_purged
The 24h grace on revoked rows keeps them queryable for incident
diagnosis right after revocation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* security(files): dedicated rate-limit bucket for downloads (C5)
/api/files/{id} now uses _check_file_download_rate_limit which keys
redis by file_downloads:{ip} instead of sharing the public_link_lookups
bucket used by /api/public/chat and friends. Limits unchanged (60/min
per IP).
Prevents heavy download traffic from starving the rate-limit quota for
public chat or other /api/public/* endpoints on the same IP.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(files): HEAD handler mirroring GET validation (C6)
Link previewers (Slackbot, Twitterbot, Discordbot, facebookexternalhit)
HEAD-probe URLs before GET. Our endpoint was 405-ing those.
Extracted _validate_download_request() helper from GET; new HEAD
handler reuses it and returns Response(200) with the same headers
(Content-Disposition, nosniff, no-store, Content-Length) but no body,
no download counter bump, no audit row. Follows RFC 7231 §4.3.2.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* security(files): tighten list endpoint to owner+admin only (C7)
GET /api/agents/{name}/shared-files previously used can_user_access_agent
(owner/admin/shared). But the list response includes full download URLs
with signed tokens — so anyone able to see the list can reuse every
share. That's the same capability as share_file + revoke, both of which
already require can_user_share_agent.
Change to can_user_share_agent (owner + admin), 403 otherwise.
DELETE was already owner-only; no change needed there.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(prompt): agent nudge for share_file MCP tool (C8)
Add a new 'Sharing Files with Users' section to the system-wide
PLATFORM_INSTRUCTIONS between Collaboration and Operator Communication.
Tells every agent:
- write files to /home/developer/public/
- call share_file MCP tool with the relative filename
- return the URL as-is
This means new agents discover the capability without the user
needing to name the tool explicitly. Applies immediately to every
agent via compose_system_prompt() — no image rebuild needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(pr-491): address validation findings — PII redaction + scope drift
PR #491 /validate-pr flagged two issues:
1. CRITICAL: pavshulin@gmail.com in two draft docs' Owner fields
(amazing-file-outbound.md, amazing-file-outbound-production-readiness.md).
Public repo + CLAUDE.md forbids real user emails.
→ Replaced with @pavshulin (GitHub handle).
2. WARNING: .claude/settings.json committed as new file — personal
Claude Code permission allowlist unrelated to FILES-001 scope.
→ Merged 8 allowlist entries into .claude/settings.local.json
(gitignored per .gitignore:67). Removed .claude/settings.json.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: fix test-ordering contamination from FILES-001 mixin fixture
Two adjustments surfaced by running the full unit suite:
1. test_file_sharing_mixin.py registered `sys.modules['db']` as a plain
module (no `__path__`), which poisoned `from db.X import Y` lookups in
sibling tests (e.g. test_fleet_sync_audit did `from db.schedules ...`
and hit `'db' is not a package`). Now we give our stub a `__path__`
pointing at the real db directory, and restore `sys.modules['db']` on
fixture teardown so no leakage remains.
2. test_start_agent_skip_inject.py didn't mock the new
`check_public_folder_mount_matches` import added by FILES-001 in
services/agent_service/lifecycle.py. The Mock container lacked
iterable `attrs["Mounts"]`, blowing up with TypeError. Stubbed the
whole `file_sharing` submodule and bound the check on `_mod`
per-test to return True by default.
Full unit suite now matches dev baseline: 17 pre-existing failures,
701 passing (+33 over dev, all from FILES-001).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Pavlo <pash@pashs-MacBook-Pro.local>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(channels): deliver images as vision content blocks via stream-json (#562) (#566)
Replaces the broken base64 data-URI-in-text approach (where Claude Code
received images as opaque markdown strings) with proper vision content
blocks fed via --input-format stream-json stdin. Images sent through
Telegram (and other channel adapters) are now visible to the agent.
- message_router: _handle_file_uploads returns 4-tuple (added image_data);
image MIME files collected as {media_type, data} dicts instead of embedded
- task_execution_service: execute_task() accepts images param, forwards in payload
- agent_server models: ParallelTaskRequest.images field added
- agent_server chat router: passes images to runtime.execute_headless()
- claude_code: adds --input-format stream-json and builds JSON content-block
stdin payload when images present; stdout/stderr threads start before stdin
write to prevent pipe deadlock; write moved into executor (not event loop)
- runtime_adapter ABC + GeminiRuntime: images param added to prevent TypeError
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(frontend): add state, brand, accent token families (#555) (#561)
Extends the design-system token system from #67 with three additional families
for colors that don't fit the status taxonomy:
state-* agent operating modes (autonomous, locked)
brand-* third-party product identity (claude, gemini)
accent-* decorative highlights named after the literal color so future
accents (accent-green, etc.) join cleanly
New tokens (all alias full Tailwind palettes, identical visual output):
state-autonomous → amber (AutonomyToggle AUTO mode)
state-locked → rose (ReadOnlyToggle ON mode)
brand-claude → orange (RuntimeBadge for Claude Code)
brand-gemini → blue (RuntimeBadge for Gemini CLI)
accent-purple → purple (DashboardPanel widget badges)
Also extends scripts/check-design-tokens.mjs to validate the new families
via a KNOWN_FAMILIES map; the reference scanner now flags typos within any
of the four families (status/state/brand/accent), not just status-*.
Migrates the 4 components blocked by #67's status-only scope:
- RuntimeBadge.vue → brand-claude, brand-gemini
- AutonomyToggle.vue → state-autonomous
- ReadOnlyToggle.vue → state-locked
- DashboardPanel.vue → accent-purple slot in getStatusColors
Fixes #555
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(scheduler): agent-owned pre-check hook (#454) (#455)
* feat(scheduler): agent-owned pre-check hook (#454)
New optional contract: agents implement POST /api/pre-check in their
container; scheduler calls it before firing a cron-triggered chat.
Endpoint absent or any error → fire as usual (fail-open). fire=false
records a skipped execution. fire=true with a message overrides the
schedule.message for that invocation.
- docker/base-image/agent_server/routers/pre_check.py: new router that
dynamically loads /home/developer/.trinity/pre-check.py (template-
supplied) and calls its check() function
- agent-server main.py: mount pre_check_router
- scheduler/agent_client.py: pre_check() method with fail-open semantics
on 404/5xx/timeout/malformed-response
- scheduler/service.py: _run_pre_check + pre-check branch in
_execute_schedule_with_lock (cron only; manual triggers bypass)
- tests/scheduler_tests/test_pre_check.py: 12 tests covering client-
and service-level behavior; 161/161 scheduler suite passes
Zero schema change — reuses existing ExecutionStatus.SKIPPED and
create_skipped_execution. Closes the "wake agent on every cron tick"
cost gap noted in docs/planning/PR_REVIEWER_AGENT.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(#454): scheduler pre-check feature flow + arch + requirements
- feature-flows/scheduler-pre-check.md: new flow doc with contract,
fail-open semantics, error table, testing summary
- architecture.md: add /api/pre-check to agent-server endpoint list
and pre-check note to Scheduler Service row
- requirements.md: SCHED-COND-001 entry under §10 (Scheduling & Execution)
- feature-flows.md: index row
- docs/planning/PR_REVIEWER_AGENT.md: design doc from which this
feature was extracted — committed for traceability
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* review: address PR #455 review feedback
- pre_check.py: asyncio.get_event_loop() → get_running_loop() (deprecated in 3.10+)
- pre_check.py: oversized message override no longer dropped silently —
response now carries message_truncated="override dropped: N bytes exceeds
32000 cap" so scheduler/operator can see what happened; log escalated to
ERROR with size+limit details
- pre_check.py: module-level docstring expanded to note the security scope
of check() (full Python interpreter access, same sandbox as chat tools —
operators should review .trinity/pre-check.py like any executable template
file) and the intentional no-cache behavior
- tests/unit/test_pre_check_router.py: 15 new router/unit tests covering
oversized-message drop path and non-dict return → 500 (both previously
only exercised by inspection). Uses importlib to load pre_check.py
directly, avoiding python-multipart requirement from sibling routers
- feature-flows/scheduler-pre-check.md: document truncation behavior,
security scope expectation, and updated test summary (12 scheduler +
15 router = 176 total passing)
Lock-scope concern noted in review is not an issue: the skip path returns
from _execute_schedule_with_lock, and the outer _execute_schedule holds
the lock in a try/finally that covers the return. No leak.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(#454): docker exec instead of agent-server HTTP endpoint
Review feedback on #455 flagged that the HTTP-endpoint design introduced
a new system edge (scheduler → agent-server direct) and a novel code-
loading pattern (importlib in a router). Both broke with Trinity's
established convention that all "run something in an agent container"
flows go through `services/docker_service.execute_command_in_container`
— the same primitive used by:
- services/git_service.py (persistent-state allowlist, #384 S3)
- services/ssh_service.py (key provisioning)
- services/agent_service/terminal.py (web SSH)
- routers/system_agent.py (admin exec)
- adapters/message_router.py (Slack file ingest)
- routers/voice.py, monitoring_service.py
This commit swaps the design accordingly.
Changes:
- Delete docker/base-image/agent_server/routers/pre_check.py and its
router registration. No new HTTP surface on agent-server.
- Delete tests/unit/test_pre_check_router.py (router is gone).
- Add src/backend/routers/internal.py →
POST /api/internal/agents/{name}/pre-check. Runs the template-shipped
`.trinity/pre-check.py` via execute_command_in_container. Two-step:
`test -f` for existence, then `python3 .../pre-check.py`. Returns
{hook_present, exit_code, stdout, stderr}. Gated by existing
X-Internal-Secret header (C-003).
- Rewrite src/scheduler/service.py::_run_pre_check to call the backend
endpoint (scheduler no longer opens a direct edge to agent-server).
Translates …
5 tasks
5 tasks
11 tasks
vybe
pushed a commit
that referenced
this pull request
May 4, 2026
* fix(security): split Docker compose into platform and agent networks (#589) Redis at 172.28.0.0/16 was reachable from any agent container. AISEC scan 3aad5469 demonstrated end-to-end exfiltration / cross-user task injection from a legitimately deployed agent. Network segmentation is the strongest control — agents now physically cannot route to Redis. Topology: - trinity-platform (172.29.0.0/16, NEW) — Redis, scheduler, vector - trinity-agent (172.28.0.0/16, name preserved) — frontend, agents - Backend / mcp-server / otel-collector / cloudflared straddle both Agent-creation sites in services/agent_service/* and system_agent_service.py need zero changes because the agent-network external name is preserved. Dev: bind Redis host port to 127.0.0.1:6379 (was 0.0.0.0). Tests connect from the dev machine; LAN cannot. Auth lands in the next commit. Refs #589 — acceptance criterion #3 (network segment separation). * fix(security): mandatory Redis auth, ACL users, auth-aware healthcheck (#589) Both compose files now enforce two passwords (REDIS_PASSWORD admin / REDIS_BACKEND_PASSWORD runtime) with the fail-on-missing :? form. docker compose refuses to render without them. Per-user ACL via inline --user flags. Additive (start from zero, allow only what the runtime needs) — never +@ALL -X, which lets newly added dangerous commands through. backend + scheduler get standard data families plus scripting/transactions/pubsub minus -@dangerous, which covers FLUSHALL, CONFIG, SHUTDOWN, MIGRATE, REPLICAOF, MONITOR. Verified at runtime against redis:7-alpine: PING/SET/GET work for the backend user, FLUSHALL and CONFIG GET return NOPERM, unauth requests return NOAUTH. REDIS_URL on backend + scheduler now embeds the backend ACL user. mcp-server: REDIS_URL and depends_on:redis dropped in prod compose (zero Redis imports in src/mcp-server/). Healthcheck pings as the backend ACL user so a typo'd ACL keeps redis unhealthy and gates dependent services. depends_on:redis switches to service_healthy so backend/scheduler don't race the ACL load. Refs #589 — acceptance criteria #1, #2, #5. * fix(scheduler-test-rig): mirror Redis auth posture (#589) Without this, scheduler container fails fast on startup against the rig because src/scheduler/config.py requires creds in REDIS_URL after #589. No ACL or network split here — this is a 2-service standalone debugging rig, not the production posture. * fix(security): fail-fast on REDIS_URL missing credentials (#589) Backend (src/backend/config.py) and scheduler (src/scheduler/config.py) now raise RuntimeError at import time if REDIS_URL is unset or lacks credentials. Removed the splicing fallback in backend config that papered over an unauth REDIS_URL by joining REDIS_PASSWORD into the URL — single source of truth (compose) eliminates silent drift. Tests that import backend modules need a creds-bearing REDIS_URL in their environment; tests/conftest.py will set a dummy one in the test commit. Refs #589 — acceptance criterion #5. * fix(webhooks): use REDIS_URL for rate-limit client (#589) Webhooks rate-limit was the one Redis client that bypassed REDIS_URL — it used redis.Redis(host="redis", port=6379) and would silently fail-open under requirepass. Switching to redis.from_url(REDIS_URL) picks up the credentialed URL like every other client. Also: distinguish auth/ACL errors (logged at ERROR with exception class) from transient errors (WARN). Fail-open behavior preserved so a Redis blip doesn't 500 legitimate webhooks, but a misconfigured deploy now surfaces in alerts instead of via a webhook abuse incident. Drops the now-unused REDIS_HOST/REDIS_PORT env reads. * feat(deploy): auto-generate Redis passwords on fresh installs (#589) start.sh ensure_redis_passwords matches the existing CREDENTIAL_ENCRYPTION_KEY pattern, with one safety guard: - Fresh install (no redis-data volume) → generate both passwords with openssl rand -hex 24 and append to .env. One-command boot keeps working. - Existing volume + missing password → refuse with a loud error pointing at docs/migrations/REDIS_AUTH.md. Re-keying a populated Redis would lock the backend out of its own data; ops needs to follow the explicit upgrade path. Idempotent — second run is a no-op when both passwords are already set. * docs(security): add Redis auth migration guide + architecture notes (#589) - docs/migrations/REDIS_AUTH.md: operator upgrade guide. Covers fresh installs (auto-generated by start.sh), live upgrades (down --remove-orphans + docker network rm + add passwords), production, and verification commands. - docs/memory/architecture.md: new "Network Topology (Issue #589)" section above Container Security. Documents the two-network split, service membership table, the "agents NEVER on platform network" rule, and the three Redis ACL users + their access patterns. * test(security): network isolation, ACL, fail-fast, webhook rate-limit (#589) tests/conftest.py: top-level autouse env stub for backend imports. Backend config now raises at import-time if REDIS_URL lacks credentials; without this, every test that transitively imports backend modules breaks. Real Redis tests under tests/security/ override via their own conftest from .env. Adds the `integration` marker. tests/unit/test_config_fail_fast.py (new): backend refuses to import without creds-bearing REDIS_URL. 3 cases — missing env, unauth URL, URL with creds. tests/security/test_redis_network_isolation.py (new): 5 integration tests covering acceptance criteria #1-#3: - agent-network container has no route to redis (BLOCKED) - unauth client gets NOAUTH on platform network - backend ACL user can PING with creds - backend ACL user FLUSHALL → NOPERM (no admin) - backend ACL user CONFIG GET → NOPERM (no requirepass leak) tests/security/conftest.py (new): session-scoped fixture loads real .env values for the integration tests; skips the suite if missing. tests/integration/test_webhook_rate_limit.py (new): regression for the from_url switch in webhooks.py. Self-contained — creates agent + schedule + webhook token inline, hits 11×, expects 429 on the 11th. Catches the silent fail-open if Redis auth ever regresses. tests/run-integration.sh (new): pytest -m integration runner. Excluded from run-smoke.sh per the smoke runner's ~30s no-Docker contract. * docs(security): detach agents before network rm (#589) Trinity-managed agent containers are created via the Docker SDK outside compose, so they store the agent network's UUID, not its name. After `docker network rm trinity-agent-network` (step 3 of the upgrade procedure), any later `docker start <agent>` fails: Error response from daemon: failed to set up container networking: network <old-uuid> not found Compose-managed services don't hit this — they're recreated with fresh network refs on `up`. Agent containers aren't, so they keep the stale UUID until disconnected. Add an explicit detach loop as step 2, before the network removal. Verified against a populated install with one running and four stopped agents: all five reattach cleanly to the new network on next start. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(security): CSO OBS-1/2/3 follow-ups — webhook rate-limit + healthcheck hardening (#589) Resolves three observations from the CSO audit (docs/security-reports/cso-2026-05-04-589-diff.md): OBS-1 — webhook rate-limit fail-open + connection-per-request DoS amplifier: * Added in-process secondary rate limiter (3x primary, per-worker) in src/backend/routers/webhooks.py. Bounds blast radius during a Redis outage without breaking the documented fail-open philosophy. * Cached the Redis client at module level under threading.Lock with double-checked init. _check_webhook_rate_limit resets the cache on inner exceptions so stale connections rebuild cleanly. Without caching, a flood would open a fresh TCP per request and exhaust Redis maxclients — turning the rate limiter into the DoS amplifier. OBS-2 — tightened _TOKEN_RE from {20,60} to {43} matching secrets.token_urlsafe(32) (verified against db/schedules.py:524). OBS-3 — switched all three compose healthchecks from `redis-cli -a $$PASS` to `REDISCLI_AUTH="$$PASS" redis-cli` so the password no longer appears in /proc/<pid>/cmdline. Additional #589 hardening (caught while resolving OBS-1): * src/backend/config.py + src/scheduler/config.py: tightened the REDIS_URL credential check from `"@" in url` substring to urlparse validation. Catches redis://@redis:6379, redis://user@redis:6379, etc. * src/scheduler/main.py: redact password from REDIS_URL before logging (was leaking via Vector log aggregator). Tests: * tests/unit/test_webhook_rate_limit_inprocess.py — 7 new tests covering cap, window expiry, token isolation, runtime-error fallback, regex shape, cache hit, cache reset. * tests/unit/test_config_fail_fast.py — 4 new parametrized cases for malformed-credential URL rejection. * 15/15 unit tests pass. * Live Redis healthcheck verified — trinity-redis reports healthy with the new REDISCLI_AUTH form; `redis-cli ping` returns PONG. Also adds .gstack/ to .gitignore so future skill artifacts stay local. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Jul 29, 2026
…cold boundary (#1816) (#1867) * docs(system-agent): base-image adoption semantics for trinity-system (#1816) Rule #1 — documentation before implementation. - architecture.md: #1560 lifecycle-clearing wording ("never evaluated" → "never acted on"), the 3-state split, and the structural AC2 gate; the system_agent_service catalog entry. - feature-flows/internal-system-agent.md: startup diagram gains the drift branch, plus a Base-image adoption section carrying the convergence invariant, the three boundaries, the AC2 gate and the consequences (writable-layer loss, no TRINITY_BACKEND_URL, operator-triggered on the canonical upgrade path). - feature-flows/agent-lifecycle.md: 3-state core + boolean wrapper, system-aware capabilities predicate + recreate override, restart-policy carry-forward. - requirements/infrastructure.md: new 8.5b ADOPT-001..005. - feature-flows.md: Recent Updates row. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(agents): 3-state base-image check + system-aware capabilities predicate (#1816) T1 — `check_base_image_matches` split into a 3-state `check_base_image_state` core (`match` | `drift` | `unknown`) plus an unchanged boolean wrapper (`state != "drift"`). Every WARNING is kept verbatim and the recreate path still consumes only the boolean, so #1809's behaviour is byte-identical: `unknown` and `match` both fail open to True. The 3-state exists because the staleness alarm this issue adds cannot be built on a boolean whose True means both "the image is current" and "the check could not run" — alarming on that would recreate the #1809 symptom one layer up. T2 — `check_full_capabilities_match` is system-aware. `trinity-system` runs FULL_CAPABILITIES by contract (package installation), not by the fleet default this predicate compares against, so pinning its `trinity.full-capabilities` label alone would, on any install with `agent_full_capabilities=false`, produce a mismatch that can never converge — a recreate on every start, forever. Both route through one shared `is_system_agent_name()` so the checker, the recreate override and the AC2 gate can never disagree. Deliberately a NAME test rather than `db.is_system_agent`: it must be unfailable (a DB error that flips this answer would either recreate the orchestrator or leak full capabilities) and it must not widen the exemption to any `is_system`-flagged row. Verified: tests/unit/test_1809_image_drift_recreate.py (19) plus test_start_agent_skip_inject, test_subscription_auto_switch_no_cred_import, test_inject_assigned_credentials, test_agent_readiness_probe, test_1560_breaker_cleared_on_lifecycle, test_1811_recovery_container_parity, test_base_image_allowlist, test_1484_create_agent_characterization — 102 passed. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(system-agent): converge creation on the recreate path's contract (#1816) T3 + T-A. `_create_system_agent` left two of the eight config predicates PERMANENTLY false: * `check_agent_auth_token_env_matches` (#1159) — the env dict never wrote TRINITY_AGENT_AUTH_TOKEN; its only writers were crud.py and the two lifecycle recreates. * `check_full_capabilities_match` — the container runs with cap_add=FULL_CAPABILITIES but carried no `trinity.full-capabilities` label, and a missing label reads as 'false' against a fleet default of true. That is not cosmetic. `recreate_container_with_updated_config` resolves the image from the container's own Config.Image *tag*, so every config recreate is also an image adoption — a permanently-false predicate means the first `POST /api/agents/trinity-system/start` after any fresh provision replaces a RUNNING orchestrator and swaps its image mid-operation, which is precisely what AC2 forbids. (Convergent: a recreate writes both values, so only the first start was affected — which is why this survived so long.) The token derive is fail-closed on an unset AGENT_AUTH_SECRET, accepted deliberately: the install now fails to CREATE the system agent rather than creating one the backend can never talk to. ensure_deployed catches → `create_failed`, and main.py's lifespan catch keeps boot alive. Deliberately NOT added: TRINITY_BACKEND_URL. It gates the agent-side heartbeat loop, and authorize_heartbeat accepts only scope='agent' keys — the system agent's is scope='system', so arming it is a permanent 5s 403 loop. Pinned by an AST guard (key-level, not substring — the code documents the omission and a text search would fire on its own rationale). tests/unit/test_1816_system_agent_convergence.py drives `_create_system_agent` for real and builds the fixture from the `environment`/`labels` kwargs it actually passes to containers_run — a hand-built fake carrying both values would assert only that a correct container is correct. Red before this commit on ['agent_auth_token'] + both pins; 11 passed after. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(agents): restart-policy carry-forward, capability override, AC2 gate (#1816) T4 — three lifecycle fixes the adoption path depends on: * **Restart policy is carried onto the replacement.** `old_host_config` was extracted at the top of `recreate_container_with_updated_config` and then never read, so `unless-stopped` silently vanished from EVERY recreated agent. `trinity-system` is created with it, so one recreate downgraded the platform orchestrator to "stays down after a crash or host reboot". `_provision_folders_and_run_agent_container` takes a keyword-only `restart_policy` and forwards it only when it names a policy, so every pre-#1816 caller is byte-identical. Read null-safely — the key can exist with a null value and `.get` on None would abort the recreate after the old container is already gone. * **`full_capabilities` override.** `None` (every existing caller) resolves to the fleet default for a regular agent and unconditionally True for trinity-system, via the same `is_system_agent_name` the predicate exempts on — writer and checker cannot disagree. * **No `TRINITY_BACKEND_URL` for the system agent.** It gates the agent-side heartbeat loop and `authorize_heartbeat` accepts only scope='agent' keys; the system agent's is scope='system'. #1816 makes this recreate a routine path for it, so arming it would newly create a permanent 5s 403 loop. T5 (AC2) — `start_agent_internal` gains the structural gate: a RUNNING trinity-system is never recreated, and the caller is told `recreate_deferred="system_agent_running"` rather than left to infer it. The gate covers the WHOLE `needs_recreation` block, not just the image predicate: the recreate resolves the image from a tag, so any predicate that fires is also an image adoption — gating one would leave AC2 open through the other eight. Surfaced through `routers/agents.py`'s whitelisted response dict and its audit details (a field added to the internal dict alone dies at the router — #1809's own learning). Two harness stubs updated: a bare `Mock()` auto-creates `is_system_agent_name` returning a truthy Mock, which reads as "every agent is the system agent" and silently suppressed every recreate in test_start_agent_skip_inject. Verified per-file (all green): test_start_agent_skip_inject 9, test_1809_image_drift_recreate 19, test_1816_system_agent_convergence 11, test_agent_readiness_probe 5, test_inject_assigned_credentials 8, test_subscription_auto_switch_no_cred_import 1, plus the 12 other helpers-stubbing files (91 tests). A cross-file sys.modules ordering flake in TestCheckBaseImageMatches (9 tests) reproduces IDENTICALLY on pristine origin/dev @64845458 — pre-existing, not a regression. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(system-agent): adopt a rebuilt base image at the cold boundary (#1816) The core of the issue. `ensure_deployed` returned `action: none` the instant the container reported `running`, without evaluating a single drift predicate. Combined with `restart_policy: unless-stopped` and a canonical upgrade path (build-base-image.sh → start.sh) that never touches agent containers, that made the platform orchestrator the most-stale agent in every fleet — indefinitely, and silently. T6 — three boundaries, honestly separated: * RUNNING → READ-ONLY. Reports `base_image_state` (`current`|`stale`|`unknown` — an enum only, never image ids, mirroring the /health clone_status contract #1439), WARNs naming the remedy, and never recreates. Source-pinned by slicing the branch between two named anchors, so a refactor that merely relocates a recreate call cannot pass. * STOPPED → delegates to `start_agent_internal` instead of a bare `container_start`. This is the cold boundary where the #1809 image gate fires, and it inherits #1560 clear-before-recreate ordering, the 409/NotFound race hardening, the post-recreate handle re-lookup and every future predicate — rather than forking a second lifecycle for one agent, which is the bug class that produced this issue. * no container → unchanged. Every early return sets both `action` and `message`: main.py's lifespan indexes them directly, so an omission would raise KeyError inside the boot log line. T7 — AC1 on the canonical upgrade path. Since the system agent is RUNNING after every canonical upgrade, the read-only branch is the one that fires, so detection without notification would tell no one. An edge-triggered operator-queue alarm follows the sync_failing/git_bloat idiom (reserved id prefix registered in #1632's anti-spoof guard, priority high, 6h cooldown, emit-failure-safe) and is raised on `stale` ONLY — a fail-open probe must never manufacture an alert, which is exactly why the 3-state split exists. R1 pre-flight: a recreate REMOVES the old container before running the replacement, so a run failure leaves the platform with no orchestrator. When the agent network is missing or the ssh port is bound, the adoption is declined and a plain start runs instead (costing one stale boot — the pre-#1816 status quo — rather than the orchestrator). Fail-open on an unreadable probe; a start failure raises a critical alarm. T8 — /restart delegates (an explicit stop makes it a cold start, so it is the operator's remedy for the alarm) and re-fetches the container for its response, because a recreate replaces the handle. /status gains the 3-state `base_image_state`, reported only while running. /reinitialize deliberately unchanged, pinned by a test. T10 — 49 behavioural cases + the source pins, and the test_1560 create-path pin repaired: `str.index` is first-occurrence, so it stayed green while silently ceasing to pin `_create_system_agent` the moment anything above it grew a `clear_agent_breakers(SYSTEM_AGENT_NAME)` call. Proven by simulation — the old assertion PASSES with the create-path clear removed, the repaired one fails with "the clear must live INSIDE _create_system_agent". Verified: test_1816_system_agent_adoption 49, test_1816_system_agent_convergence 11, test_1560_breaker_cleared_on_lifecycle 16, test_1632_operator_queue_caps 49 — 118 passed. tests/lint_sys_modules.py: no new violations. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(agents): name the shared helper correctly (is_system_agent_name) (#1816) The docs written ahead of implementation used `is_system_agent()`, which collides conceptually with the existing DB-backed `db.is_system_agent`. Names the real helper and states why it is a name test rather than the DB one. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tests): sync the test catalog and the two remaining feature flows (#1816) /update-tests + /sync-feature-flows tail steps. - .claude/agents/test-runner.md: both new unit files catalogued under Operations & Observability, a dated Recent Test Additions block (including the test_1560 pin repair and the two harness-stub fixes), and the totals. - feature-flows/async-docker-operations.md: the new `network_get` wrapper. - feature-flows/operating-room.md: `base-image-stale-` registered in the reserved-prefix enumeration (all 3 sites) and named in the platform-create exemption list. agent-lifecycle.md and internal-system-agent.md were already synced in the docs-first commit; feature-flows.md index is 430 lines. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(system-agent): make the state-label map public, merge the duplicate gate (#1816) Self-review polish, no behaviour change: - `_BASE_IMAGE_STATE_LABELS` is consumed by `routers/system_agent.py`, so a leading underscore was wrong — renamed `BASE_IMAGE_STATE_LABELS`. - `get_system_agent_status` had two consecutive `if status == "running":` blocks; folded the health fetch into the first. It keeps its own try/except, so `base_image_state` is still set when the agent is unreachable. Verified: test_1816_system_agent_adoption + test_1816_system_agent_convergence — 60 passed. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(system-agent): pass the container into the adoption pre-flight (#1816) The stopped branch already holds the handle; re-fetching it inside `_preflight_ok_for_delegated_start` was a wasted Docker round-trip on the boot path and a needless TOCTOU window. Still null-safe throughout, so a partially populated handle degrades to "no port to check" rather than raising. Verified: 60 passed (adoption + convergence). Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(system-agent): make the #1816 suites CWD-independent (#1816) Caught by verify-local, not by any local run: `_create_system_agent` resolves its template through a CWD-RELATIVE fallback (`./config/agent-templates`, used whenever `/agent-configs/templates` is absent, i.e. off-container), and verify-local runs pytest from `tests/`. From there creation died on a missing template BEFORE reaching the token derive, so `test_creation_without_agent_auth_secret_fails_closed_without_blocking_boot` was asserting the wrong failure — green from the repo root, red from `tests/`. Both suites now pin the CWD to the repo root with the reason stated, so they hold wherever pytest is invoked from. Verified from BOTH cwds: 60 passed each. A test bug, not a code bug — the behaviour under test is unchanged. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(system-agent): state the staleness alarm's per-worker reach (#1816) `ensure_deployed` runs once per worker lifespan and the cooldown cursor is per-process, so a stale boot files one operator-queue item per worker (`--workers 2` ⇒ 2). Deliberate rather than overlooked — a cross-worker cursor puts Redis or a DB read on the boot path for an advisory alarm, and the un-guessable timestamped id (what stops an agent pre-creating and silencing it, per #1632) is inherently undedupable by `on_conflict_do_nothing`. Said plainly in both the code and the flow doc rather than left for a reviewer to discover. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(system-agent): fence the delegated start, gate the operator remedy (#1816) Review + CSO follow-ups on the base-image adoption work. - recreate_missing_container refuses trinity-system (409). start_agent_internal falls through to it when the container lookup returns None, and #1816 newly reaches that from the boot path and /restart — ensure_deployed runs in every uvicorn worker with no leader lock, so a concurrent recreate can null the lookup mid-flight. That path reconstructs a REGULAR agent: it deactivates the system-scoped MCP key and mints an agent-scoped one (plaintext unrecoverable, so the orchestrator irreversibly loses its permission bypass), drops trinity.is-system, the /template bind and unless-stopped, and arms the scope-403 TRINITY_BACKEND_URL. ensure_deployed's create branch rebuilds it correctly on the next boot; the race itself is #1817. - /restart and /reinitialize are human-only. assert_admin rejects connector principals but not agent ones, and get_current_user hands an agent-scoped key its owner's role — so on a default admin-owned install any non-ephemeral agent's TRINITY_MCP_API_KEY passed it. Tolerable while /restart was a stop+start; not once it replaces the container. trinity-ops-agent#232 precedent; no-op for JWT / user-scoped / system-scoped callers. - Sanitize the start-failure alarm's interpolated exception string. It lands in operator_queue.question — durable, operator-visible state — from a path that builds env dicts holding OAuth tokens and PATs. The staleness alarm was built to carry no identifiers; this one wasn't. - Bound that alarm across processes via a bucketed id. A per-process cursor cannot bound a failure whose symptom is a fresh process, and retention never deletes a pending row. A bucket (not a fixed id) so Clear All → cancelled can't wedge it shut through on_conflict_do_nothing; guessability is fine because the prefix is in _RESERVED_ID_PREFIXES. - Cooldown cursor uses time.monotonic() — datetime.utcnow() is deprecated and a wall-clock step could skip or extend the gate. - State the real boot cost in the comment (~20-90s of blocked lifespan, not one timeout) and name the multi-worker exposure. 9 tests; 5151 unit tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(system-agent): sync architecture.md with the review-pass fences (#1816) The last commit added two fences but only reached requirements + the feature flow. architecture.md is the current-design doc and carries the invariant, so both belong here too. - system_agent_service entry: recreate_missing_container refuses trinity-system (409, ADOPT-006). Worth stating where the service is described, because the reader's natural assumption is that the generic recovery rebuild is a valid way to bring the orchestrator back — it is not, and the downgrade it causes (system-scoped MCP key deactivated for an agent-scoped one) is irreversible. - Invariant #8 gains "Role ≠ human": assert_admin/require_admin answer what role, never is-this-a-human, because get_current_user resolves an agent-scoped MCP key to its owner carrying the owner's role. Generalized from the two known instances (#1644 retention ack, #1816 /restart + /reinitialize) into the rule that produced them — an endpoint whose blast radius is operator-scale needs reject_agent_principal in ADDITION to the role gate, and the trigger to revisit an existing gate is a change in what the endpoint DOES. Escalating a handler's destructiveness silently re-prices every principal that could already reach it. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(system-agent): pin the real modules against sibling sys.modules leaks (#1816) The adoption suite passed alone and in the full alphabetical run, but 21 of its 67 tests failed when collected alongside tests/unit/test_start_agent_skip_inject.py — i.e. it was green by luck of import order, and CI randomizes. Five sibling unit files replace services.agent_service / .helpers with Mock objects in sys.modules at COLLECTION time (the grandfathered #762 class). This file resolves the real modules lazily, inside fixtures — after the contamination. The docstring already noted the file is not a contaminator; not being one is not the same as being immune to one. The failure mode is silent rather than loud, which is the reason to fix it rather than order the files: a leaked helpers Mock makes is_system_agent_name return False for everything, so the AC2 gate never fires and the recreate it exists to suppress happens — while every assertion still looks meaningful. check_base_image_state degrades the same way (a Mock can never return "unknown", so the never-alarm-on-unknown property becomes untestable). Fix: import the real modules at collection time (this file sorts before all five) and pin them per-test with monkeypatch.setitem, which self-restores — so the siblings' own harnesses, which hold direct module references rather than sys.modules lookups, are untouched. No new lint_sys_modules violations (monkeypatch.setitem is the sanctioned form). Reproduction now green: adoption + skip_inject 21 failed -> 0; the original 5-file set 21 failed -> 108 passed. Refs #1816 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Eugene Vyborov <eugene@beingluminous.com>
vybe
pushed a commit
that referenced
this pull request
Jul 29, 2026
…1804) (#1863) * docs(requirements): CAS-won terminal owns the paired activity close (#1804) Trinity Rule #1 — requirements before implementation. scheduling.md §10.15: the terminal-write contract now includes closing the paired agent_activities dispatch row. Records the one-owner helper, the activity CAS lattice (mirrors the execution predicate), the widened lookup, the bulk/per-row split by cardinality, and the terminal-write-anchored parity guard. infrastructure.md §12.9 (CLEANUP-001): the 120-minute activity sweep is demoted to a backstop for the unclaimed, and moves after the stale-slot reaper so it can no longer beat a legitimate closer within one cycle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(activities): make the activity close a lattice CAS with a tri-state outcome (#1804) T1+T2. The db-layer spine for the #1804 contract. - ActivityCloseOutcome (UPDATED / ALREADY_CLOSED / NOT_FOUND). One bool cannot answer both "did this row exist" (routers/internal.py 404s) and "did anything change" (activity_service broadcasts); once idempotent no-op closes are a designed outcome the two answers diverge routinely. - complete_activity becomes an atomic CAS on _close_predicate, which MIRRORS the execution-row predicate in db/schedules/executions.py: incoming COMPLETED -> activity_state != 'cancelled' (an authoritative close may upgrade a provisional FAILED — the #1083 late-SUCCESS-after-lease-expiry path); incoming CANCELLED/FAILED -> activity_state = 'started' (nothing overwrites an authoritative close, so a double close cannot clobber completed_at / duration_ms / error). The lattice diagram is inline over the predicate. - get_open_activity_id_for_execution takes include_failed so the LOOKUP agrees with the write — a lookup narrower than the CAS makes the widened predicate inert. Ordering prefers still-'started' rows. - close_open_activities_for_executions: set-wise bulk close for the watchdog sweeps, one transaction, no per-row WS, chunked at _SQLITE_MAX_IN_VARS. No schema change, no migration (Rule #9 not triggered). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(activities): add close_execution_activity, the single owner of the close contract (#1804) T3. A leaf helper on activity_service (imports only models/database/db.activities — no services.* edge, so no cycle) that every CAS-won terminal writer calls. - Reuses models.activity_state_for_terminal (#1332) — no second mapping. - Lattice-aware lookup: an authoritative terminal (SUCCESS/CANCELLED) searches started|failed so it can upgrade a provisional FAILED; a provisional terminal searches started only. Callers pass a terminal status and stay ignorant of it. - Delegates to complete_activity, so the agent_activity WebSocket broadcast and subscriber notify survive (what a db-layer close would have lost). - Broadcasts only on ActivityCloseOutcome.UPDATED; ALREADY_CLOSED is a designed no-op and must not emit an event claiming the activity just closed. - complete_activity's bool now means "exists and closed" — only NOT_FOUND is False, so routers/internal.py keeps its 404 semantics and an idempotent re-close does not start 404ing the scheduler. - spawn_close_execution_activity: sync fire-and-forget wrapper for the synchronous pull sink, mirroring spawn_task_terminal_event (strong task ref, fail-open with no running loop). Fail-open throughout: the close runs after a committed terminal write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(execution): close the paired activity at every CAS-won terminal writer (#1804) T4-T8. Wires the eight terminal writers onto the single owner. task_execution_service: - _write_terminal_and_gate gains the missing CAS-LOSS branch (the SUCCESS applier has had one since #1332): re-read the row and close the activity in the terminal that actually stands. `activity_status` leaves the signature — it is derived from `status`, so the two can no longer drift. - the asyncio.CancelledError shutdown handler now closes too. #767 closed the execution record here so the sweep would not inflate ITS duration but left the activity for the 120-minute backstop to inflate instead; worse, the row is `failed`, so startup recovery (which scans `running`) skips it forever. routers/internal: the second shutdown writer, same shape (no activity_id in scope — the helper looks it up). cleanup_service: - watchdog and startup _recover_execution close on the CAS-won branch. The startup one was DISCARDING its CAS bool (returned True unconditionally absent an exception); it is now captured, gates the close, and is returned. - _close_bulk_swept_activities: a SIBLING of _emit_bulk_terminal_events, never folded into it — that method short-circuits when nobody subscribes, which would skip the close on every install with no event subscribers. Driven by the collect_failed rows #1714 already collects; one transaction, no per-row WS. - _sweep_stale_activities moves AFTER _sweep_stale_slots: it ran one line before the reaper that legitimately closes activities, so within a single cycle the 120-minute fabricator could beat a real closer. - _close_reaped_activity / _close_stale_slot_activity delegate to the helper. - CleanupReport.activities_closed_on_recovery (not summed into `total` — it is an observability counter over work already counted). pull_coordination_service: closes on the CAS-won branch via the sync spawn wrapper. lease_reaper_service: requeued_execution_ids, closed CANCELLED — a superseded attempt is not a failure, and the re-queue preserves execution_id so the next delivery opens a second activity against the same row. chat_execution_service: terminate folds onto the helper (it was the fifth hand-rolled copy of the idiom, and a copy is invisible to the parity guard). Behaviour unchanged — R4. db/schedules/cleanup: mark_no_session_executions_failed returns the CAS-WON count, not the candidate count (its sibling already did). report. no_session_executions was over-reporting; deliberate, visible value change. Tests updated where a mock encoded the old call surface; one behavioural assertion inverted on purpose (test_terminal_write_cas_gate: a lost CAS now closes the activity in the persisted state instead of leaving it open). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(1804): R1-R5 regressions, the two-table chain test, and the parity guard T9+T10. test_1804_recovery_closes_activity.py — db layer against the real schema (db_harness) + service + every wired writer: R1 a FAILED activity upgrades to COMPLETED, through the lookup (the pairing that keeps the fix from being inert) R3 an already-closed close never touches completed_at/duration_ms/error R2 _write_terminal_and_gate's CAS-loss closes with the PERSISTED state R5 both backend-shutdown CancelledError handlers close their activity plus: broadcast on UPDATED only, watchdog/startup gating on the CAS bool, the bulk close running with zero event subscribers (the #1714 gate scopes the event, never the close), the pull sink on applied-but-not-replayed/ conflict, and the lease reaper's requeued_execution_ids. test_1804_terminal_activity_chain.py — the two CAS predicates live in two different tables; mocks encode what the author believed, so these run the real statements and assert the tables agree. Includes the late-SUCCESS upgrade end to end and the cancel-is-never-overwritten mirror image. test_1804_terminal_activity_parity.py — anchored on terminal WRITES, not on completion-event emission. The emit set is a strict subset (both shutdown writers emit nothing), which is exactly how they hid from review. Explicit allowlist, each entry justified by lifecycle position, plus a self-test that the guard actually fires and a staleness check on the allowlist. R4 (terminate byte-identical) lives in test_1332, which the previous commit updated to the new call surface. tests/lint_sys_modules.py: clean, no new violations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(architecture): record the terminal-activity close contract (#1804) New canonical block "Terminal-Activity Close Contract (#1804)" — one home for the feature, pointers from the places that used to imply the old model: - Fire-and-Forget Dispatch (#1083): the close is a property of winning the CAS, not of holding the activity_id local; apply_result is one writer among eight. - Task Completion Events (#1578): the emit set is NOT the close set — both shutdown handlers write a terminal and emit nothing, which is why the parity guard is anchored on terminal writes. - Background Services / Cleanup Service: the recovery closes, activities_closed_on_recovery, and the backstop moving last in the cycle. - services catalog: activity_service owns the closer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(feature-flows): the terminal-activity close contract (#1804) /sync-feature-flows. activity-stream.md — new "The Close Contract (#1804)" section: the owner (close_execution_activity + the sync spawn wrapper), the state lattice with the side-by-side execution/activity diagram and why the lookup must agree with the CAS, the split by cardinality, the full writer table, and the demotion of the 120-minute backstop. task-execution-service.md — the _write_terminal_and_gate lost-CAS branch (and the dropped activity_status param), the backend-shutdown close, and a row in the Activity Tracking table pointing at the contract. dashboard-timeline-view.md — why the amber bar used to lie, and that no frontend change was needed: ReplayTimeline was reading a lying database. feature-flows.md — Recent Updates row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1804): make the close outcome identity-stable and never downgrade a recovery Two real defects the full-suite run surfaced (both green in isolation, red at 5,100 tests — the pollution class the dossier warned about). 1. ActivityCloseOutcome moves db/activities.py -> models.py. Callers compare it by `is`, and db_harness evicts `db.activities` from sys.modules per test. Any test that imported services.activity_service BEFORE an eviction held one enum class while a later `from db.activities import ...` got a different one, so every identity check silently went False. models is the leaf everything imports and nothing re-imports, so the object stays the same one — and it now sits beside ActivityState/TaskExecutionStatus, where a shared db↔service contract type belongs. Re-exported through db.activities. 2. Both _recover_execution closes get their own try/except. The terminal write and the capacity release have already succeeded at that point; letting a close failure propagate flipped a recovered row into the `errors` bucket and would make the watchdog retry a row it had already recovered. Fail-open is the rule for everything after a committed terminal — this was the one place I left it implicit. Full unit suite: 5136 passed, 14 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1804): the #1083 result callback must look up failed activities too Self-review catch. The callback endpoint IS the late-SUCCESS path — the one the lattice was widened for. A lease reaper that beat the callback has already FAILED the row and closed the activity FAILED; the execution CAS deliberately lets a genuine late SUCCESS correct the row ("a FAILED row falls through so a late SUCCESS can still overwrite a reaper LEASE_EXPIRED"). With a started-only lookup the callback passed activity_id=None, apply_result closed nothing, and the pair settled at execution=success, activity=failed — permanently. That is #1804 inverted, on the exact path the upgrade exists for. Harmless for a FAILED callback: the close CAS refuses to overwrite an already-closed activity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(activity-stream): note the callback's include_failed lookup (#1804) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: confine the git-reset loader's sys.modules stubs to their own test `_load_git_service` installs Mocks under the real names `database`, `services.docker_service` and `services.activity_service`. It deliberately leaves them in `sys.modules` after the loader returns (the reset path imports `activity_service` lazily at call time) — but nothing put them back afterwards either, so the Mocks stayed installed for the rest of the session. That is a silent, ordering-dependent landmine for any later test that lazily imports one of those names. #1804 makes `cleanup_service._close_stale_slot_activity` delegate to `activity_service.close_execution_activity`; with this file running first, `test_1083_lease_reaper.py::test_swallows_errors` awaited the leaked Mock and died with `TypeError: object Mock can't be used in 'await' expression` — a real red under CI's pytest-randomly seeds, in a test unrelated to git reset. Adds the lint-recognised `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` autouse pair, also restoring `services.git_service*` (the loader force-reimports it against the Mocks, so leaving that behind poisons later importers too). `tests/lint_sys_modules.py` drops this file from 9 violations to 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(1804): close the reflexive COMPLETED edge and make CAS-loss labels honest Three review findings on the terminal-activity close contract. 1. The close lattice inherited the upstream's permissive edge (the bug it exists to prevent). `_close_predicate` was specified as an authority ORDERING (`started < failed < {completed, cancelled}`) but implemented as an EXCLUSION copied literally from the execution CAS: `!= 'cancelled'`. Those two disagree exactly on the reflexive case — `!= 'cancelled'` matches `'completed'`, so a second COMPLETED close was permitted and re-dated `completed_at`/`duration_ms`/`error`. Executed against the real path, a 15-minute activity (`duration_ms=900000`) re-closed COMPLETED became ~9h: the exact fabricated-duration symptom #1804 exists to eliminate, arriving through #1804's own fix. It is reachable from `_write_terminal_and_gate`'s lost-CAS branch, which passes an explicit `activity_id` — so the narrower `started|failed` lookup cannot shield it. The literal copy was faithful: `update_execution_status`' SUCCESS predicate really is `!= CANCELLED` and really does admit `success -> success`. The execution row survives that edge only because an upstream guard (the #1083 callback's authoritative-terminal replay short-circuit) stops the duplicate before it reaches the CAS — a protection the activity's new call sites do not have. The predicate now states the ordering literally, `IN ('started','failed')`, and the branch's five lattice tests gain the `X -> X` case they all skipped because it looks like a no-op. 2. A CAS loss to a *successful* row stamped "superseded by ..." onto a clean success, and rendered the enum repr while doing it. `error` is now None on an authoritative SUCCESS close (a non-NULL `error` on a `completed` activity reads as a problem in every activity-derived view — the rule the shipped terminate path already follows), and the label uses `.value`, since `f"{TaskExecutionStatus.FAILED}"` renders as `TaskExecutionStatus.FAILED` on 3.11+ — the `str, Enum` footgun #1578 already paid for. 3. `_recover_execution` returning the terminal CAS bool (it must, to gate the close) changed what a False MEANS to `recover_orphaned_executions`, which counted every False as `errors`. A lost CAS is RELIABILITY-005's guarded writer working as designed — a real completion landed during restart — so folding it into `errors` makes a healthy restart race read as a failing one, and would have made this fix look like it introduced failures. False is now partitioned: genuine exceptions self-count into `stats["errors"]`, the remainder is reported as `cas_lost`. Docs updated in lockstep (architecture.md, requirements/scheduling.md, feature-flows/activity-stream.md) to state the COMPLETED arm as deliberately tighter than the execution CAS rather than a literal mirror, plus a learnings.md entry on inheriting a permissive edge when mirroring a predicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Eugene Vyborov <eugene@beingluminous.com>
AndriiPasternak31
added a commit
that referenced
this pull request
Jul 29, 2026
…talog
One template whose `credentials:` — or `credentials.mcp_servers` — was a list,
a string, or **null** raised an uncaught AttributeError out of
`_build_local_template` -> `get_local_templates()` -> `GET /api/templates`:
HTTP 500 with ZERO templates listed. One bad template hid every good one.
The `github:` builder was worse: no `isinstance` guard at all on untrusted repo
metadata (`_fetch_template_yaml` returns `safe_load(...) or {}`, and a top-level
list is truthy). Deeply nested YAML escaped the parse handler entirely as
`RecursionError` — a `RuntimeError`, not a `yaml.YAMLError`.
Worse than the crash because it was silent: `env_file: "OPENAI_API_KEY"` — an
ordinary typo, the list dash forgotten — was iterated character by character
into the generated `.env`, emitting fifteen single-letter variables, never
writing the real credential, with no error, no warning and no crash. The agent
booted and its MCP server failed at first use with nothing pointing at the
cause.
And `credentials.config_files[].path` was joined onto the staging directory and
opened for write with no normalization, so an absolute path or a `..` escape
was an arbitrary-file-write primitive — reachable by any authenticated user,
since `deploy_local_agent_logic` accepts an uploaded template archive.
Four tolerant readers in `template_service.py` are now the only way into the
block, with two deliberately opposite contracts:
* Read paths never raise. The catalog degrades the derived field to empty,
attaches `credential_errors` to the entry, logs one WARNING naming the
template id — and the template STILL LISTS. `get_local_templates` also fences
each per-template build, so a future unguarded field cannot regress the
property the readers buy.
* The write path fails loud. `generate_credential_files` validates first and
raises the HTTP-free `CredentialDeclarationError`, which its only caller maps
1:1 to 400 `INVALID_CREDENTIAL_DECLARATION` (Invariant #1: services hold no
HTTP concerns). It is called before the docker try/except, so the 400 is not
flattened to a 500.
`config_files[].path` is rejected at the parse boundary AND re-checked at the
write sink (`crud._safe_cred_file_path` -> 400 `INVALID_CREDENTIAL_FILE_PATH`),
using the same resolve + `is_relative_to` CodeQL barrier as
`_safe_local_template_path`.
Absent / null / `{}` all stay a valid zero-credential contract — the ent#124
starter trio ships exactly that, and a commented-out block must not acquire a
spurious warning.
`credentials.env_file` stays a names-only list. The enriched per-variable
declaration lands under its own top-level key (PR-B) precisely so an older
Trinity reading a newer template is structurally untouched.
Tests: `test_ent128a_catalog_resilience.py` — 33 pass here, 31 fail on
`origin/dev`, the two headline ones with the exact bug signatures
(`AttributeError: 'list' object has no attribute 'get'` and the literal
`O=\nP=\nE=\nN=` output). Includes a parity test so a malformed BUNDLED template
fails CI instead of a user's fresh install.
No DB change, so the dual-track migration rule (#9) does not apply.
PR-A of trinity-enterprise#128 (AC #5). PR-B — the enriched declaration schema —
re-gates after this merges.
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Jul 30, 2026
…talog (ent#128 PR-A) (#1835) * fix(templates): stop a malformed `credentials:` block emptying the catalog One template whose `credentials:` — or `credentials.mcp_servers` — was a list, a string, or **null** raised an uncaught AttributeError out of `_build_local_template` -> `get_local_templates()` -> `GET /api/templates`: HTTP 500 with ZERO templates listed. One bad template hid every good one. The `github:` builder was worse: no `isinstance` guard at all on untrusted repo metadata (`_fetch_template_yaml` returns `safe_load(...) or {}`, and a top-level list is truthy). Deeply nested YAML escaped the parse handler entirely as `RecursionError` — a `RuntimeError`, not a `yaml.YAMLError`. Worse than the crash because it was silent: `env_file: "OPENAI_API_KEY"` — an ordinary typo, the list dash forgotten — was iterated character by character into the generated `.env`, emitting fifteen single-letter variables, never writing the real credential, with no error, no warning and no crash. The agent booted and its MCP server failed at first use with nothing pointing at the cause. And `credentials.config_files[].path` was joined onto the staging directory and opened for write with no normalization, so an absolute path or a `..` escape was an arbitrary-file-write primitive — reachable by any authenticated user, since `deploy_local_agent_logic` accepts an uploaded template archive. Four tolerant readers in `template_service.py` are now the only way into the block, with two deliberately opposite contracts: * Read paths never raise. The catalog degrades the derived field to empty, attaches `credential_errors` to the entry, logs one WARNING naming the template id — and the template STILL LISTS. `get_local_templates` also fences each per-template build, so a future unguarded field cannot regress the property the readers buy. * The write path fails loud. `generate_credential_files` validates first and raises the HTTP-free `CredentialDeclarationError`, which its only caller maps 1:1 to 400 `INVALID_CREDENTIAL_DECLARATION` (Invariant #1: services hold no HTTP concerns). It is called before the docker try/except, so the 400 is not flattened to a 500. `config_files[].path` is rejected at the parse boundary AND re-checked at the write sink (`crud._safe_cred_file_path` -> 400 `INVALID_CREDENTIAL_FILE_PATH`), using the same resolve + `is_relative_to` CodeQL barrier as `_safe_local_template_path`. Absent / null / `{}` all stay a valid zero-credential contract — the ent#124 starter trio ships exactly that, and a commented-out block must not acquire a spurious warning. `credentials.env_file` stays a names-only list. The enriched per-variable declaration lands under its own top-level key (PR-B) precisely so an older Trinity reading a newer template is structurally untouched. Tests: `test_ent128a_catalog_resilience.py` — 33 pass here, 31 fail on `origin/dev`, the two headline ones with the exact bug signatures (`AttributeError: 'list' object has no attribute 'get'` and the literal `O=\nP=\nE=\nN=` output). Includes a parity test so a malformed BUNDLED template fails CI instead of a user's fresh install. No DB change, so the dual-track migration rule (#9) does not apply. PR-A of trinity-enterprise#128 (AC #5). PR-B — the enriched declaration schema — re-gates after this merges. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(templates): make the credential-file path guard a two-step barrier CodeQL flagged `_safe_cred_file_path` with two HIGH `py/path-injection` alerts, on the join itself and on the containment check. It was right to: the helper had only step 2 of the pattern (resolve + `is_relative_to`), and CodeQL does not treat that alone as a barrier — the sibling `_safe_local_template_path` clears the same query because it allowlists the RAW string first. Add that step 1 here too: reject empty, absolute, `..`-bearing, and anything outside `[A-Za-z0-9._/-]` before the value ever reaches the join, then keep the resolve + containment as step 2. `_CRED_FILE_PATH_RE` permits `/` (unlike `_LOCAL_TEMPLATE_NAME_RE`) because this is a relative *path*, not a single slug. Not defensive padding for a scanner — the two-step shape is what makes the guard legible to a reader as well, and it is the established pattern in this file. Test extends to the allowlist rejections (empty, leading dash, space, semicolon) alongside the traversal cases. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
dolho
added a commit
that referenced
this pull request
Jul 30, 2026
…ent#180)
An exposed agent's card advertises every `template.yaml capabilities[]` tag, and
the well-known discovery route is unauthenticated by design — so today that full
capability list is world-readable for any exposed agent. This lets an operator
choose what the outside is told.
**A disclosure control, and the code says so.** The card's `skills[]` is
advertisement: inbound `message/send` dispatches free-form text via
`execute_task(triggered_by="a2a")`, with no per-skill routing. Filtering changes
what an orchestrator SEES, never what it may ASK for. That's stated in the
requirement, the seam docstring and the filter itself, because a filter
operators mistake for an invocation gate is a control that looks like security
and isn't. A real boundary (constraining a2a-triggered runs via
allowed_tools/guardrails) is separate work with its own threat model.
Extends the existing ent#157 seam rather than adding a module — same shape as
the inbound allow-list, second provider:
provider.exposed_skills(agent_name) -> Optional[List[str]]
- No provider (OSS) → identity function; the card is byte-identical to before,
by construction. The enterprise module owns the config, storage and UI.
- `None` = no opinion = advertise all: the unconfigured default, so exposure
(already opt-in, default OFF) keeps every existing card unchanged on upgrade.
- `[]` ≠ `None`: an explicit "advertise nothing".
- Stale ids are inert — the selection only subtracts; `template.yaml` stays the
source of truth for what exists.
- Fail-open on provider error (advertise all + WARNING): consistent with the
seam's availability bias and the advertise-all default. Honest only *because*
this isn't a security boundary — failing closed would silently empty a card
and break discovery invisibly.
Both card surfaces (public well-known + authenticated per-agent) go through one
router helper, so they can't disagree and a future third surface gets the filter
by default rather than by remembering. `generate_a2a_card` stays pure — the
provider lookup lives in the helper.
Writing the tests found a real gap: a provider returning a str (a defect) would
iterate into single characters, match no id, and silently empty the card —
fail-CLOSED, the opposite of the contract, and invisible. Malformed returns now
take the same fail-open path as a raised error.
Requirements §32.4 written before the code (CLAUDE.md rule #1); public docs
describe the generic seam only, per the standing enterprise-docs rule.
Related to trinity-enterprise#180
vybe
pushed a commit
that referenced
this pull request
Jul 31, 2026
….8 amend + new §9.9 (ent#260) Requirements-first (Rule #1): records the third dashboard mode (Timeline / Grid / List), the 28-item parity disposition, chassis filter/create migration, the /agents → /?view=list one-shot non-persisting redirect, NavBar consolidation, and the zero-backend-change / N+1-deletion performance shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Jul 31, 2026
…cator (ent#264) New §15.1h: reaction ack + elapsed-time placeholder + channel-agnostic start/progress/resolve seam, group gating (mention/reply OR all-mode; observe stays silent), degradation ladder, default-ON per-binding toggle, GET /telegram access hardening. §15.1c/§15.1e touch-ups. Requirements-first per Rules of Engagement #1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Jul 31, 2026
…ote updated (ent#261) Trinity Rule #1: requirements before implementation. §9.10 specifies the / hotkey filter across Timeline/Grid/List — store-seam predicate, pre-query node invariant, pill honesty, Esc layering, chassis query-empty overlay, kbd hint, list-mode composition, and the deliberate timeline owner-filter behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
Jul 31, 2026
…metadata Closes ent#128 AC #1-2. A template can now describe each credential an operator must supply — title, description, required, secret, format, setup_url, default — and `template_service` surfaces the normalized result as `credential_requirements` on every catalog entry. **Enrichment lives in a NEW sibling top-level key; `credentials:` is FROZEN as names-only, forever.** An already-deployed older Trinity reads `env_file` through `credential_env_file_names` and then does `agent_credentials.get(var_name, "")` — hand it a list of mappings and that is `TypeError: unhashable type: 'dict'` at the moment it writes the agent's `.env`. A sibling key is structurally invisible to that binary, so there is no floor version and enrichment distributes immediately. **Base-set-plus-overlay, so the two keys cannot drift.** One record per variable `credentials:` declares, decorated by `credential_setup:` entries joined BY NAME. An entry naming nothing is a named three-line error (problem, cause, FIX) and is dropped; valid siblings survive. `credential_setup:` can only ever decorate — the sibling-key shape's usual failure mode is closed by construction, not by discipline. Stated honestly: for an EXTERNAL template that error is neither impossible nor visible in the UI — `credential_errors` has zero frontend and zero MCP consumers, so the only human channel is the backend log. It is LOGGED. `required` is a tri-state. Enriched-and-omitted means `True` (an author who described a variable meant it); a legacy bare `- FOO` is `"unknown"`, never `True` — it carries no authorial intent, and reading it as required makes a guided checklist cry wolf. `"unknown"` doubles as the enriched/un-enriched discriminator, which is why no `enriched: false` flag is needed. `secret` defaults `True` (fail-safe). Path-free by construction, so trinity#570's `template.yaml` → `trinity.yaml` rename cannot reach it. **The normalizer never raises, and that is load-bearing.** `_build_template` runs in bare list comprehensions in `get_all_templates()`, OUTSIDE PR-A's per-template fence (which covers `_build_local_template` only) — a raise there is HTTP 500 with an EMPTY CATALOG, i.e. PR-A's exact bug reopened by the change that surfaces the new metadata. And no bomb is needed: `title: 123` or a bare `title:` was enough. So the builders ALSO wrap the call and degrade to `[]` plus a named error, rather than fencing the comprehension — that keeps the named error the resilience contract promises. The property does not rest on one function's discipline. (Which earned its keep immediately: the wrapper caught a real NameError during development instead of emptying the catalog.) Trust boundary — `title`/`description`/`setup_url`/`name`/`source` are author-controlled strings from arbitrary GitHub repos flowing into an operator-facing "paste your API key" checklist: * **Type-guard before touching.** Never `str()` a container from untrusted YAML: `str()` EXPANDS a shared alias during the walk (443 B → 52 MB in 1.5 s, x10 per level), and both the sanitizer and the record cap act after that cost is paid. * **Cap the INPUT**, entries AND errors AND the base set. Capping records while leaving `errors` uncapped built a 35 MB response out of the cap meant to prevent it; and `default` had no type row, so the 100-record cap acted as a x100 multiplier on it. * **`source` is sanitized** — it carries the raw MCP server name, the exact string `_sanitize_for_warning`'s own docstring names as the threat, and it was not on the list. * **Per-field length caps.** Reusing the 80-char terminal-warning default truncated a realistic 159-char description and made a real 90-char vendor console URL unusable. * **`setup_url` above scheme-only**: https (case-insensitive — `HTTPS://` is a legitimate author), a parseable host, NO userinfo (`https://google.com@evil.tld` renders as one host and resolves to another — the display/resolve split IS the attack), ≤2048, printable. Validate THEN sanitize, and never through a truncator. Residual documented, not claimed closed: `isprintable()` rejects RTL/ANSI but an IDN homograph survives, so a consumer must render the parsed hostname beside the link. * **Never mutates its input** — `_metadata_cache` holds the parsed dict for 600 s and YAML aliases genuinely share nodes, so one in-place normalize would rewrite both aliased fields and persist for ten minutes. Asserted against a deep-copy snapshot, including a real `&anchor`/`*alias` document. `credential_shape_errors` also gains the per-server and per-ELEMENT rows for `mcp_servers`, mirroring what `env_file` already had. The element row is the one that matters — an `env_vars` entry smuggled in as a mapping was the single most dangerous shape in the block and was unnamed. Note this makes the write path (`generate_credential_files` → 400) reject a template that previously created an agent with a garbage declaration: correct per PR-A's fail-loud write contract, and release-noted. `generate_credential_files` is deliberately UNTOUCHED — it still reads `env_file` names-only, which is what makes the forward-compatibility argument true rather than asserted. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndriiPasternak31
pushed a commit
that referenced
this pull request
Aug 2, 2026
…ial paths from name: (#1900) (#1935) * fix(templates): contain local: template id resolution on the read path (#1900) `GET /api/templates/{template_id:path}` handed `local:<name>` straight to `get_local_template`, which joined `<name>` onto the templates root with no validation. The `:path` converter permits `/`, so `local:../<x>`, `local:/<abs>/<x>` and a root-escaping symlink each read `<escaped-dir>/template.yaml` and echoed its contents in an authenticated 200. Reachable by any authenticated principal of any role — including an agent-scoped MCP key, so a prompt-injected agent qualifies. In a container the reachable set includes `/data/deployed-templates/<victim>`, where every user's uploaded template archive lands: a cross-tenant read. Two corrections to the issue's framing, both verified here: * it is NOT arbitrary file read — the filename is fixed (`template.yaml`), it must parse as a YAML mapping, and only a fixed key set is echoed. But those keys' VALUES are arbitrary YAML subtrees, not just strings. * `local:..` alone is not an existence oracle: a directory with no `template.yaml` returns the same 404 as an unknown id. Fix: `contained_template_dir(name, root)` — the two-step barrier the CREATE path has had since #950 (`crud._safe_local_template_path`), brought to the read path. A name allowlist runs BEFORE any path math (this is also what CodeQL recognises as a `py/path-injection` barrier; resolve-only was flagged high-severity twice on this codebase), then `resolve()` on BOTH sides plus `is_relative_to`. `str.startswith` is not equivalent — it passes the sibling escape `<root>-evil`. An escaping id returns `None`, so the router's 404 stays byte-identical to an unknown template: no error code, no path, no root name. A distinct error would be a NEW enumeration oracle, which is what #1759's single-sentence 404 exists to close. Rejections log at DEBUG, sanitized — the endpoint has no rate limit, so a per-rejection WARNING would be an authenticated log-flood primitive. The helper is public: the remote-template-registry work (trinity-enterprise#14) edits this same resolver family in this same module and should import it rather than copy it. Tests: every rejection test PLANTS a real `template.yaml` at the escaped location, because unpatched code returns `None` for any id whose target simply does not exist — a rejection test with nothing planted is green before and after and proves nothing. Each is labelled REPRO (verified red pre-fix) or HYGIENE (cannot be made red; contract only). The router guard lives in `tests/unit/` because no gating CI job collects `tests/test_templates.py`. `test_1900_containment_survives_a_symlinked_root` is the landmine guard for resolving both sides: a half-resolved variant passes all 130 pre-existing tests and only that test catches it. Refs #1900 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(templates): stage .mcp.json from the validated template dir, not template name: (#1900) The second traversal sink, found by this issue's own AC #4 audit ("audit the by-name create path for the same join"). It is NOT the create path's `local:` id resolution — that has been contained since #950/#1759 via `crud._safe_local_template_path`, applied at both seams #1759 named. It is the create path's CREDENTIAL STAGING, which threw that validated path away and re-derived a directory from scratch: template_name = template_data.get("name", "") # untrusted mcp_template_path = templates_dir / template_name / ".mcp.json" `name:` comes from an uploaded template.yaml, so any `creator` reaches it via `deploy_local_agent`. `name: ../../data/deployed-templates/<victim>` read another tenant's `.mcp.json` — a credential-bearing file type under Invariant #12 — into the attacker's OWN agent, where they read it at leisure. A victim who hardcoded a token rather than a `${VAR}` placeholder leaks it. Assessed on its own axes, NOT inherited from the read sink: different trigger (`creator` role + an upload + an agent create, vs a bare authenticated GET) and a higher impact ceiling (credential values, not template metadata). Also P2, for different reasons. The derivation was also simply wrong. `name:` is not a directory name — 5 shipped templates declare a display string there ("Test Echo Agent"), so `local:test-echo` resolved to `<curated>/Test Echo Agent/.mcp.json`, which does not exist. That kills the "validate the name" framing: the value should not resolve paths at all. Fix is root-cause, not another guard: `_stage_config_files` already calls `_safe_local_template_path` itself for the `/template` bind decision, so the validated directory is available in the same function. Extract the two-root ladder as `_resolve_local_template_dir` and pass its result as `template_base_path`. The untrusted join is gone from the live path, and the #1759 "seams must agree" property becomes structural across all THREE seams (resolver, bind decision, credential stager) instead of two. Deliberately NOT threaded through `_TemplateResolution` or the return tuple: `_resolve_local_template` returns a 2-tuple that three existing tests depend on, two as monkeypatched `lambda config: ({}, None)` doubles — widening it breaks the test doubles, not just the callers. Its signature, its return arity, `_safe_local_template_path`, `_LOCAL_TEMPLATE_ROOTS`, and everything inside the CodeQL-sensitive `if template_yaml.exists():` block are untouched (#1793 had to revert exactly that reshape). The residual `template_base_path is None` arm is kept and made fail-closed: it is a public function with a `template_base_path=None` default, so a future caller can still reach it. It now contains through the same barrier as the id, which also absorbs a non-string `name:` — `Path(root) / 123` raised TypeError, i.e. an uncaught HTTP 500 during agent creation (the ent#128 bug class, one seam over). One disclosed behaviour change: a deploy-local template that BOTH declares `credentials.mcp_servers` AND ships a `.mcp.json` now gets `${VAR}` substitution, where the old curated-root lookup always missed. Verified no collision with `deploy._prepopulate_workspace_from_template`, which writes the archive's raw copy into the workspace volume: `startup.sh` copies `/generated-creds/.mcp.json` unconditionally (gated only on the directory existing) and AFTER the template-copy block (gated on `.trinity-initialized`), so the substituted file deterministically wins — which is the intended behaviour, the raw copy still carrying unsubstituted placeholders. Not one of the 26 shipped curated templates contains a `.mcp.json`, so the curated rows are provably unchanged. The crud seam tests are mandatory, not decorative: every service-level test calls `generate_credential_files` directly, so an "extracted but never wired" mistake leaves all of them green while deploy-local resolution silently regresses into the fallback arm. Refs #1900 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: record the #1900 containment contracts for template id + credential staging Rule #1 (requirements before implementation) — `core-agent.md`: * §4.1 gains the **read-path resolution contract** as a sibling to the existing create-time contract: `GET /api/templates/{id}` resolves `local:<name>` through the same two-step barrier the create path has had since #950, and a failing name returns a 404 byte-identical to an unknown template (the #1759 non-disclosure rule). Records the deliberate, known asymmetry that `get_local_templates()` still enumerates by `iterdir()` and so could LIST a root-escaping symlink that detail and create both refuse — the listing is the outlier, and planting one needs local filesystem write access, not a request. * §4.3 records that the `credentials.mcp_servers` template lookup now resolves from the validated path rather than the template's own untrusted `name:` field, including the one disclosed behaviour change (deploy-local templates now get `${VAR}` substitution) and why the substituted file wins over the archive's raw copy. `architecture.md` gets one clause on the `templates.py` router catalog entry (the catalog rule caps entries at 2 lines, and no Cross-Cutting Subsystems block is warranted). A bug fix would normally be commit-message-only under the tiered-docs rule; the exception is that this ships a public, importable containment primitive in the exact module and resolver family the remote template registry (trinity-enterprise#14) will edit, and one catalog clause is the cheapest way that author finds it instead of copying the flaw. Not updated, deliberately: no feature-flow doc (no new vertical slice), no user docs (no user-visible change for honest callers), no schema/migration (no DB change, so the dual-track SQLite/Alembic rule does not apply). Refs #1900 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(feature-flows): sync template-processing + local-agent-deploy for #1900 `/sync-feature-flows` was NOT a no-op here — three concrete staleness points in `template-processing.md`, which owns the `local:` resolution surface: * The inlined two-root ladder is now `_resolve_local_template_dir`; the code block showed the pre-extraction form. * "**Two** seams read `_LOCAL_TEMPLATE_ROOTS` and must stay in agreement" was the #1759 claim and is now wrong in the direction that matters: there was always a third seam (the credential-file stager) which did NOT agree — it re-derived the directory from the template's untrusted `name:`. Corrected to three, with the extraction as the structural guarantee. * `generate_credential_files` was cited by stale line range (`:228-299`) and documented none of where the `.mcp.json` template is actually located. Replaced the fragile line-range citation with a symbol reference and added the provenance, the residual fail-closed arm, and the disclosed deploy-local substitution delta. Also documents the read-path containment (`get_local_template` → `contained_template_dir`) beside the existing #1513 catalog-curation note, including the deliberate list-vs-detail asymmetry for a planted symlink. `local-agent-deploy.md` gets one line at the credential-merge step: a deploy-local template's `.mcp.json` now resolves from the deploy-local directory, so a hostile `name:` cannot read another tenant's file and `${VAR}` substitution finally applies to that template. `credential-injection.md` was checked and NOT touched — its `.mcp.json` references are unrelated (credential inject/export/import), and the template lookup lives in template-processing. One dated row added to the feature-flows index (no new flow document — this is a fix to documented behaviour, not a new vertical slice). Tests: adds the end-to-end `test_get_template_rejects_path_traversal` beside the existing 404 test. Stated plainly in the test's own docstring that this file is root-level and collected by NO gating CI job — it runs under `/verify-local` only, so it must not be read as CI coverage. The CI-gated guard is `tests/unit/test_1900_template_id_traversal.py`. The multi-level case is percent-encoded because httpx collapses a literal `../../` client-side. Refs #1900 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(docs,templates): state the #1900 credential-staging delta honestly (review) /review + /cso on the #1900 branch. One MEDIUM finding, verified by execution rather than by reading, plus two comment-accuracy defects. MEDIUM — the disclosed behaviour delta was described as a gain and is a loss. Three docs claimed a deploy-local template "now genuinely receives `${VAR}` substitution". It does not. `generate_credential_files` has exactly one production caller (`crud._stage_config_files`, verified by grep across src/), and it passes `agent_credentials={}` — CRED-002 injects real values AFTER creation, not at staging. So `agent_credentials.get(var_name, "")` rewrites every placeholder to the empty string. Measured against the shipped code: archive .mcp.json : {"env": {"TOKEN": "${MY_TOKEN}", "FIXED": "literal"}, "args": ["-y", "${MY_TOKEN}"]} staged .mcp.json : {"env": {"TOKEN": "", "FIXED": "literal"}, "args": ["-y", ""]} and the staged file WINS over the archive's raw copy, for exactly the `startup.sh` ordering reason the branch already documents. Net effect for a deploy-local template shipping a `.mcp.json`: its placeholders are destroyed, and nothing is substituted in. Non-placeholder content survives verbatim. This is not a reason to revert the root-cause threading — the `.env` arm of the same function has always blanked an un-supplied `credentials.env_file` variable, so blank-at-staging is the platform's model, and `.mcp.json.template` (compatibility check S-009) remains the durable record of required variables, pre-populated untouched. It IS a reason to stop advertising it as an improvement: prose stronger than the code is itself a defect (learnings 2026-07-28), and this one would have shipped into the requirements file. Corrected in requirements §4.3, template-processing.md and local-agent-deploy.md, and pinned by a new test so the flattering restatement cannot come back: `test_1900_staging_with_an_empty_credential_map_blanks_placeholders` asserts the measured output (`""`, `["-y", ""]`, `${MY_TOKEN}` absent) and that hardcoded entries are preserved. LOW — `_LOCAL_TEMPLATE_ROOTS`' own comment still said "Read at TWO seams", which is the pre-#1900 claim and stale in the direction that matters: this PR exists because the third seam did not agree. Corrected to three, naming the extraction as what makes the agreement structural. LOW — "crud -> template_service is forbidden" (helper docstring + parity-test docstring) contradicts crud.py:32, which imports this module today. The ban is on what may be GATED on it (the #1484 MagicMock harness), not on the import edge; a reader who checks the citation and finds it false is one step from "helpfully" importing the regex. Narrowed to say so. Verification performed for this review, beyond the above: * every REPRO test re-run against reverted sources — 19 of 30 red pre-fix; the 11 that are green pre-fix are all correctly labelled HYGIENE / anti-over-block / landmine-guard, so no test is decorative; * `test_1900_containment_survives_a_symlinked_root` re-proved as the sole guard for resolving both sides (a candidate-only-resolve variant fails it and nothing else); * independent attack harness, 55 hostile inputs through `contained_template_dir` and `get_local_template`: zero escapes, zero raises, zero disclosures, and every legitimate name still accepted. The only non-None hostile input is the documented `"sage\n"` `$`-parity edge, which is CONTAINED; * symlink loops, a 200 KB name and a regex-backtracking probe: no exception, no superlinear time (the endpoint is unauthenticated-adjacent and rate-limitless); * `startup.sh` re-verified independently: `/generated-creds/.mcp.json` is copied at :376-383, gated only on the directory existing, AFTER the `.trinity-initialized`-gated template blocks that end at :367. Refs #1900 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tests): state honestly that test_templates.py has no automated runner The docstring claimed the #1900 traversal assertions "run under /verify-local". They do not. Every automated stage collects a subdirectory -- CI runs `pytest unit/`, /verify-local runs `pytest unit/` then `pytest integration/` -- and this file is root-level, so NEITHER collects it. The assertions have no automated runner at all. Corrected to say so, with the manual invocation that does exercise them (needs a booted backend), a pointer to the CI-gated guard that actually protects the fix, and a note that giving this file a runner is a follow-up. Docstring only -- no assertion changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Eugene Vyborov <eugene@beingluminous.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
AndriiPasternak31
pushed a commit
that referenced
this pull request
Aug 2, 2026
…ange (#1931) Rule #1: land the requirements/flow delta before the code. requirements/core-agent.md - §4.1: two bullets — catalog intent is DECLARED, not defaulted (every bundled directory must set `hidden:`; the runtime default is deliberately unchanged, because flipping it turns a forgotten key into a silent absence); and demo fleets ship hidden but stay deployable via a bundled manifest, with the system-name/short-name coupling to dd-lead's hardcoded roster spelled out. - §4.2.1: the shipped GitHub default list is empty and why; None-vs-[] now differ only in the `source` badge; `github:owner/repo` create is untouched. - §4.5: the GitHub-zero empty state — marketplace-first, then the owner/repo CTA, then the role-branched curation hint; plus the precedence rule that makes it mutually exclusive with the page-level empty state. feature-flows - library-page.md: the GitHub-zero placeholder, the 4-row precedence truth table, and a #1931 revision-history row. - template-processing.md: the sort no longer "orders starters ahead of the rest" — after this change there is no rest; component is Library.vue. - mcp-orchestration.md: drop the agent-ruby example that will never again appear in list_templates. No architecture.md change: it documents the mechanism (the `hidden:` filter, the None-vs-[] fallback, the section render), and no mechanism changes here. Adding a catalog-contents paragraph would give requirements/core-agent.md §4 a second home, against that file's own editorial rule #1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AndriiPasternak31
pushed a commit
that referenced
this pull request
Aug 2, 2026
…epo list as starting points (#1934) * docs(templates): requirements + flow delta for the catalog-honesty change (#1931) Rule #1: land the requirements/flow delta before the code. requirements/core-agent.md - §4.1: two bullets — catalog intent is DECLARED, not defaulted (every bundled directory must set `hidden:`; the runtime default is deliberately unchanged, because flipping it turns a forgotten key into a silent absence); and demo fleets ship hidden but stay deployable via a bundled manifest, with the system-name/short-name coupling to dd-lead's hardcoded roster spelled out. - §4.2.1: the shipped GitHub default list is empty and why; None-vs-[] now differ only in the `source` badge; `github:owner/repo` create is untouched. - §4.5: the GitHub-zero empty state — marketplace-first, then the owner/repo CTA, then the role-branched curation hint; plus the precedence rule that makes it mutually exclusive with the page-level empty state. feature-flows - library-page.md: the GitHub-zero placeholder, the 4-row precedence truth table, and a #1931 revision-history row. - template-processing.md: the sort no longer "orders starters ahead of the rest" — after this change there is no rest; component is Library.vue. - mcp-orchestration.md: drop the agent-ruby example that will never again appear in list_templates. No architecture.md change: it documents the mechanism (the `hidden:` filter, the None-vs-[] fallback, the section render), and no mechanism changes here. Adding a catalog-contents paragraph would give requirements/core-agent.md §4 a second home, against that file's own editorial rule #1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(templates): hide the dd-* demo fleet, empty the stale GitHub default list (#1931) A fresh install's Library showed 14 local templates — 11 of them the VC due-diligence demo fleet — plus 6 GitHub repos last pushed Dec-2025/Jan-2026 that no install had ever overridden. The visible catalog is now the 3 starters we actually stand behind. get_local_templates() on the real catalog: 14 -> 3 (sage, scout, scribe) GitHub entries on a default install: 6 -> 0 bundled dirs omitting `hidden:`: 14 -> 0 bundled dirs total: 25 -> 25 (nothing deleted) Catalog - 11 x `hidden: true` on dd-*/template.yaml, placed and commented to match the 11 directories that already declare it. Not deleted, not moved: it is a demo we still run, `local:dd-lead` stays creatable by id, and the resolver never reads `hidden` (verified: get_local_template("local:dd-lead") still resolves). - 3 x `hidden: false` on sage/scout/scribe — the AC asks a new directory to DECLARE its catalog intent, so the declaration has to be mandatory. - DEFAULT_GITHUB_TEMPLATE_REPOS = []. Every consumer already tolerates it (five existing unit tests stub exactly this), and the one path that could have regressed does not: recreating an agent made from `github:abilityai/agent-ruby` routes through get_github_template, whose `if repo in DEFAULT_...` branch and "Dynamic" fallback are byte-identical two-line bodies. Zero behavioural delta. Emptying a BROWSE list deletes no data and stops no agent, so the #1638 "mutable code default read at action time" lesson does not bite here. Side-effect, intended: GET /api/templates now makes zero outbound GitHub calls on a cold metadata cache, where it previously blocked on up to six. Still deployable as a set - config/manifests/vc-due-diligence.yaml, PROMOTED from docs/demos/vc-due-diligence/system-manifest.yaml — not authored. The system name and short names are load-bearing: deployed names are f"{name}-{short}" and dd-lead/CLAUDE.md hardcodes its roster as `vc-due-diligence-dd-*`, so a tidier `vc-demo` + `founder` would deploy 11 healthy containers whose Deal Lead reaches nobody. Added the nine dd-lead -> specialist permissions (the manual post-deploy step the old copy told you to run by hand); dropped `prompt:` (overwrites trinity_prompt), `auto_start:`/`folders:` (not read by parse_manifest) and per-agent `resources:` (each dd-* template.yaml overwrites it at creation). dd-lead is listed FIRST so a creator hitting the default 10-agent quota loses a specialist, not the orchestrator. - The old copy gets a SUPERSEDED banner and the demo README points at the new location; keeping it (rather than moving) preserves any existing link, and the banner is what stops the two drifting. Tests - test_1931_catalog_intent.py (new, dependency-free): every bundled directory declares `hidden:`, it is a real bool, and the visible set is pinned to sage/scout/scribe. Own file so `import yaml` stays out of test_local_templates_listing.py's import block, and because _build_local_template's `bool(data.get("hidden", False))` destroys exactly the present-vs-absent information this asserts on. The RUNTIME default stays visible-by-default on purpose — flipping it turns a forgotten key into a silent absence, which is the worse failure. - test_1931_manifest_roster.py (new): validate_manifest over the glob, plus the assertion that would have caught the naming trap. It anchors on the SHORT NAME (`<prefix>-<short>` in a deployed template's CLAUDE.md must equal that manifest's resolved name), not on the manifest's own name — the latter goes vacuously green on precisely the rename it exists to catch. Verified to fire on both a system rename and a short-name rename, and to find zero false positives across all four bundled manifests. - test_local_templates_listing.py: `dd-` joins the visible-prefix ban; test_real_catalog_surfaces_starters_ahead_of_suite renamed and reworked — its `if dd_positions:` clause could now only go vacuous, so it asserts the priority: 20 mechanism instead, with the ordering clause generalised and labelled inert. - test_ent124_default_system_seed.py: the two on-disk checks widened from default-system.yaml to a glob over config/manifests/*.yaml, parametrised so a failure names the manifest. They are properties of any bundled manifest. Docs - config/agent-templates/README.md: the dd table moves out of "Starter templates" into its own "Demo fleet" subsection under "Not starting points", both demo manifests are named, and the authoring rule becomes the intent-declaration contract. - mcp-orchestration.md + mcp-server create_agent description (Invariant #13): drop the `github:abilityai/agent-ruby` example that will never again appear in list_templates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(library): teach the next action when there are no GitHub templates (#1931) With DEFAULT_GITHUB_TEMPLATE_REPOS emptied, a default install has zero GitHub templates — and the section was wrapped in `v-if="githubTemplates.length > 0"`, so it silently VANISHED. requirements §4.5 promises "per-kind empty states teach the next action"; that promise was being honoured only by accident, because the count had never been zero before. Library.vue - New `noTemplatesAtAll` computed over the WHOLE /api/templates response (both sources). The section now renders on `!noTemplatesAtAll` — not `githubTemplates.length > 0 || !noTemplatesAtAll`, whose first disjunct is provably dead since githubTemplates is a filter over templates. The page-level "No templates configured" block is UNCHANGED and keeps sole ownership of the wholly-empty case; the 4-row truth table is inlined at the computed, and exactly one empty state renders in every row. - Placeholder card, marketplace-first (operator decision): the abilityai/abilities marketplace + create-agent wizards lead, because they exist today and a fresh install's Settings panel does not. Then the secondary "already have a repository?" action, then the curation hint. - The CTA is `useTemplate({ id: 'github-custom' })`, CreateAgentModal's own sentinel for the free-form owner/repo option — deliberately NOT `useTemplate(null)`, which is byte-identically the Blank Agent button two sections up and would land the user on the wrong option under a different label. The sentinel arrives via the existing `initial-template` prop and is explicitly exempted from that component's unknown-template reset, so no CreateAgentModal change is needed. - Only the curation hint branches on role (`useRole()`), mirroring LibrarySkillsSection.vue on this same page — the templates half must not ship the opposite convention to the skills half. The ACTION is offered to both roles so a non-admin is never left holding only an admin-only path. - Tag-along: the page-level hint said "Configure GitHub templates in config.py", which is not an operator surface and is now empty by design. Settings.vue — the destination must not dead-end The Library's own hint sends an admin to Settings → GitHub Templates, where a fresh install said "…or reset to defaults" next to a Reset button that is :disabled in exactly that state and would now reset to the same empty list. This change would otherwise satisfy the AC on the Library and newly violate it one click away. Two strings: drop the impossible action from the empty row, and stop badging an empty set as "Using defaults". Verified: `npm run build` clean. No e2e spec asserts any changed string — smoke.spec.js matches the headings 'Library' / 'Agent Templates' with exact: true (the new card's heading is "No GitHub templates configured"), and settings-tabs.spec.js only route-mocks /api/settings/github-templates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(templates): prove the empty GitHub default list is a zero-delta change (#1931) /update-tests + /sync-feature-flows tail pass. The riskiest claim in this change was verified only by READING: that emptying DEFAULT_GITHUB_TEMPLATE_REPOS cannot break an existing agent created from `github:abilityai/agent-ruby`, because get_github_template's `if repo in DEFAULT_...` branch and its "Dynamic" fallback are byte-identical two-line bodies. That is the sibling-path rule — a guard proven on one codepath has to be proven on every path reaching the same behaviour — so it is now proven by RUNNING: test_1931_empty_github_defaults.py - an unconfigured github: id still resolves with the list empty - the emptied and configured paths return the IDENTICAL template dict - get_all_templates() with [] makes ZERO outbound metadata fetches (the intended side-effect: no ThreadPoolExecutor, no HTTP, no PAT read) - counter-test: a CONFIGURED repo is still fetched, so the assertion above cannot go vacuously green if fetching breaks entirely - the shipped constant is [] Verified to fail when the list is refilled. `tests/lint_sys_modules.py` green (monkeypatch.setitem, no bare sys.modules assignment); order-independent (_metadata_cache cleared per load, since it is module-global and survives). Flow docs — two surfaces the plan had not named: - platform-settings.md (TMPL-001): the section said admins configure repos "replacing the hardcoded config.py list" and that no-DB-config falls back to "[...from config.py...]". Both now misleading. Records the empty default, that None and [] produce the same catalog and differ only in the badge, that Reset-to-Defaults reverts to empty (and is already :disabled there), that create capability is untouched, and the zero-outbound-calls side-effect. - system-manifest.md: a new bundled manifest is DATA, NOT A TRIGGER (BUNDLED_MANIFEST_PATH is hard-coded to default-system.yaml; nothing globs the directory) — the basis on which an 11-agent / ~40 GB manifest was safe to add. Plus the ent124 glob widening and the new roster guard. - feature-flows.md: one Recent Updates row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(templates): scan every prompt surface for a hardcoded roster (#1931) Review findings on this branch's own new tests. 1. The roster guard walked `CLAUDE.md` only. A literal collaborator name is just as load-bearing — and breaks just as silently — in a slash command, and the sibling ent#239 check already treats `.claude/commands/` as a first-class shipped surface. Restricting the walk left four real literals unguarded in the bundled corpus: sage/.claude/commands/request-research.md -> acme-scout demo-analyst/.claude/commands/briefing.md -> research-network-researcher demo-analyst/.claude/commands/request-research.md -> research-network-researcher Widened to CLAUDE.md + .claude/commands/*.md + .claude/skills/**/*.md via a deduped, sorted `_prompt_files()` helper. Token pattern unchanged. Corpus: 13 -> 28 matching tokens across 4 manifests, zero offenders. Mutation-proved twice: renaming the manifest `name:` goes red (as before), and breaking a name inside a .claude/commands file now goes red too — the coverage that did not exist before. 2. `test_shipped_default_is_empty` did a bare `sys.path.insert(0, ...)` in the test body: a permanent, un-undone global side-effect (one duplicate entry per session) that `tests/unit/conftest.py` already makes unnecessary. Dropped; the test still passes standalone. 3. Dropped an unused `import yaml` inside the roster test, and corrected a comment claiming `_metadata_cache` "survives across loads" — each `exec_module` builds a fresh module, so the `.clear()` is defensive only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test): correct the unguarded-literal count to three (#1931) The `test_manifest_resolved_names_satisfy_hardcoded_rosters` docstring said restricting the walk to `CLAUDE.md` left "four" real literals unguarded. It is three. Verified twice: grep -rlE "acme-scout|research-network-researcher" config/agent-templates/ returns 6 files, of which exactly 3 are not `CLAUDE.md` — `demo-analyst/.claude/commands/{briefing,request-research}.md` and `sage/.claude/commands/request-research.md` — and an occurrence count confirms one match per file. The parenthetical that follows already enumerated three; only the count word was wrong. Docstring-only. No assertion, scan surface, or behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(secret-scan): allowlist the sanctioned YOUR_TOKEN placeholder (#1931) gitleaks' default `curl-auth-header` rule flags the `-H "Authorization: Bearer YOUR_TOKEN"` line in the DEPLOY header comment of config/manifests/vc-due-diligence.yaml (entropy 3.121928). It is a documentation placeholder, not a credential, and is verbatim-identical to the pre-existing line in config/manifests/research-network.yaml:10 — that one never tripped the scanner only because the workflow scans the PR commit range, not the whole tree. So this PR followed the house convention rather than introducing a new practice; the manifest comment is left untouched. Add YOUR_TOKEN to the existing `regexes` allowlist beside the other CLAUDE.md-sanctioned placeholders (`your-api-key`, `your-domain.com`). Deliberately NOT a `paths` entry: per the note already in this file, a path allowlist is a PRE-SCAN file skip and would stop config/manifests/ being scanned for real secrets, whereas a targeted regex suppresses only this placeholder. Verified locally with the CI-pinned gitleaks 8.30.1 over the same commit range: 1 leak before, "no leaks found" after; a planted ghp_ PAT under config/manifests/ is still detected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Eugene Vyborov <eugene@beingluminous.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
AndriiPasternak31
pushed a commit
that referenced
this pull request
Aug 2, 2026
…ed templates (#1908) (#1936) * docs(requirements): bundled-template .gitignore hygiene contract (#1908) Rule #1 — requirements before implementation. Trinity grades every agent against docs/agent-validation-spec.md but shipped no gate on the templates it ships itself: all 14 visible bundled templates failed the same four HARD security checks at birth (S-001 .env, S-002 .mcp.json, S-004 .claude/projects/, S-005 .trinity/). Adds core-agent.md §4.1.1 stating the contract, the canonical-list provenance, the G-001 trap (no wholesale .claude/ exclusion), the second-order effect on this repo's own view of config/agent-templates/, the .trinity/* escape hatch for a future template shipping committed hooks, and two honest gaps: hiding a template does not stop it birthing findings (the resolver never reads `hidden`), and T-004/T-005 remain for the three starters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(templates): ship the canonical .gitignore in all 14 visible bundled templates (#1908) An agent created from `sage`, `scout` or `scribe` was born with four HARD compatibility findings and no user changes: the templates shipped no .gitignore at all. The 11 visible `dd-*` templates shipped one, but its entire content was `outputs/` + `*.log` — covering none of `.env`, `.mcp.json`, `.claude/projects/` or `.trinity/`. So 14 of 14 visible templates failed the same four checks, and hiding a template does not fix it: the resolver never reads `hidden`, so a hidden template stays creatable by id and births the same findings. `_GITIGNORE_PATTERNS` was already merged into every agent's .gitignore, but only at git init / first Push — too late for the first compatibility report and never at all for a `local:` agent that never syncs. Shipping the same list in the template moves protection from first sync to first boot. It is byte-identical to what the sync merge and the #668 auto-fix would write, so birth-state == post-auto-fix state == post-sync state. Content is derived mechanically from the fenced gitignore block in docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md (itself parity-tested against the constant), never hand-typed. Each `dd-*` keeps its own `outputs/` rule in a trailing `# Template-specific` section, so this augments those files rather than clobbering their intent. No bundled template gains a bare `.claude/` line — that is HARD check G-001, and it would trade four findings for one. Measured with the real STATIC_CHECKS registry over a collector-shaped snapshot: the 11 `dd-*` go 4 HARD -> 0; the 3 starters go 6 HARD -> 2, the residual being T-004/T-005 (`resources.cpu`/`resources.memory` absent from template.yaml). Those are deliberately not fixed here — a template-level `resources` block overrides the admin's fleet-wide default (RES-001), so pin-vs-inherit is a product decision. AC#2 ("0 hard findings") is therefore met for 11 of 14 templates and not for the 3 starters. tests/unit/test_1908_bundled_template_gitignore.py is both the guard and the regenerator (`--regenerate`), so a new canonical entry is a one-command change however many templates are guarded. It evaluates the real `static_checks.run_static` rather than re-implementing the rules, asserts `status == "pass"` positively plus set-equality with the requested ids (a renamed id returns "skipped", so "nothing failed" would empty itself), pins the one legitimate precondition skip (D-003, no dashboard.yaml), fails if a new *visible* template is added outside GUARDED_TEMPLATES, and applies G-001 to every bundled directory including hidden ones. Verified non-decorative by mutation: appending `.claude/` to sage fails 5 tests and deleting its .gitignore fails 6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(templates): close the doc->template .gitignore seam in the #1908 guard (#1908) The regenerator builds all 14 bundled `.gitignore` files by copying the ```gitignore``` fence out of `docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md` verbatim, but nothing asserted that direction. `test_doc_and_constant_in_sync` checks only `_GITIGNORE_PATTERNS ⊆ doc`, and the two guards added in the previous commit are one-way too (`canonical ⊆ shipped`, then byte-identity *with the regenerator*). So any line added to that doc fence shipped, unreviewed and untested, into every bundled template — and thence into every new agent's own repository, where the sync-time `git rm --cached` sweep would untrack whatever it newly matched. Proven open, not assumed: injecting `skills/` into the fence and re-running the regenerator left `test_guarded_gitignore_mirrors_canonical_patterns` and `test_guarded_gitignore_matches_the_regenerator` both GREEN. - `ALLOWED_NON_CANONICAL` pins the delta that exists today (`!.env.example`, `!.mcp.json.template` — the guide's fence is a superset of the constant by exactly these two negations) with the reason each is admissible. - `test_guarded_gitignore_ships_no_unreviewed_pattern` closes the direction: a shipped line must be canonical, pinned, or that template's own `TEMPLATE_EXTRAS`. It REDs on the `skills/` injection above. - `test_allowed_non_canonical_is_not_stale` retires the pin if the guide drops an entry, so it can never become a standing licence. Also corrects three claims that did not hold: - `_EXPECTED_SKIPS`'s "no bundled template ships a dashboard.yaml" — `trinity-system` does; it is simply hidden and unguarded. - the module docstring's implication that the guide block is a faithful mirror of `_GITIGNORE_PATTERNS` (the parity test is one-way). - requirements §4.1.1's "byte-identical to what the sync merge and the #668 auto-fix would write" — both are append-if-missing, so the accurate and stronger statement is that they are *no-ops* on this file. Guard 78 -> 93 passed; full `tests/unit -m "not slow"` 6086 passed / 18 skipped (baseline 6071, delta = the 15 new cases). Regenerator still idempotent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Eugene Vyborov <eugene@beingluminous.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
Aug 2, 2026
…metadata Closes ent#128 AC #1-2. A template can now describe each credential an operator must supply — title, description, required, secret, format, setup_url, default — and `template_service` surfaces the normalized result as `credential_requirements` on every catalog entry. **Enrichment lives in a NEW sibling top-level key; `credentials:` is FROZEN as names-only, forever.** An already-deployed older Trinity reads `env_file` through `credential_env_file_names` and then does `agent_credentials.get(var_name, "")` — hand it a list of mappings and that is `TypeError: unhashable type: 'dict'` at the moment it writes the agent's `.env`. A sibling key is structurally invisible to that binary, so there is no floor version and enrichment distributes immediately. **Base-set-plus-overlay, so the two keys cannot drift.** One record per variable `credentials:` declares, decorated by `credential_setup:` entries joined BY NAME. An entry naming nothing is a named three-line error (problem, cause, FIX) and is dropped; valid siblings survive. `credential_setup:` can only ever decorate — the sibling-key shape's usual failure mode is closed by construction, not by discipline. Stated honestly: for an EXTERNAL template that error is neither impossible nor visible in the UI — `credential_errors` has zero frontend and zero MCP consumers, so the only human channel is the backend log. It is LOGGED. `required` is a tri-state. Enriched-and-omitted means `True` (an author who described a variable meant it); a legacy bare `- FOO` is `"unknown"`, never `True` — it carries no authorial intent, and reading it as required makes a guided checklist cry wolf. `"unknown"` doubles as the enriched/un-enriched discriminator, which is why no `enriched: false` flag is needed. `secret` defaults `True` (fail-safe). Path-free by construction, so trinity#570's `template.yaml` → `trinity.yaml` rename cannot reach it. **The normalizer never raises, and that is load-bearing.** `_build_template` runs in bare list comprehensions in `get_all_templates()`, OUTSIDE PR-A's per-template fence (which covers `_build_local_template` only) — a raise there is HTTP 500 with an EMPTY CATALOG, i.e. PR-A's exact bug reopened by the change that surfaces the new metadata. And no bomb is needed: `title: 123` or a bare `title:` was enough. So the builders ALSO wrap the call and degrade to `[]` plus a named error, rather than fencing the comprehension — that keeps the named error the resilience contract promises. The property does not rest on one function's discipline. (Which earned its keep immediately: the wrapper caught a real NameError during development instead of emptying the catalog.) Trust boundary — `title`/`description`/`setup_url`/`name`/`source` are author-controlled strings from arbitrary GitHub repos flowing into an operator-facing "paste your API key" checklist: * **Type-guard before touching.** Never `str()` a container from untrusted YAML: `str()` EXPANDS a shared alias during the walk (443 B → 52 MB in 1.5 s, x10 per level), and both the sanitizer and the record cap act after that cost is paid. * **Cap the INPUT**, entries AND errors AND the base set. Capping records while leaving `errors` uncapped built a 35 MB response out of the cap meant to prevent it; and `default` had no type row, so the 100-record cap acted as a x100 multiplier on it. * **`source` is sanitized** — it carries the raw MCP server name, the exact string `_sanitize_for_warning`'s own docstring names as the threat, and it was not on the list. * **Per-field length caps.** Reusing the 80-char terminal-warning default truncated a realistic 159-char description and made a real 90-char vendor console URL unusable. * **`setup_url` above scheme-only**: https (case-insensitive — `HTTPS://` is a legitimate author), a parseable host, NO userinfo (`https://google.com@evil.tld` renders as one host and resolves to another — the display/resolve split IS the attack), ≤2048, printable. Validate THEN sanitize, and never through a truncator. Residual documented, not claimed closed: `isprintable()` rejects RTL/ANSI but an IDN homograph survives, so a consumer must render the parsed hostname beside the link. * **Never mutates its input** — `_metadata_cache` holds the parsed dict for 600 s and YAML aliases genuinely share nodes, so one in-place normalize would rewrite both aliased fields and persist for ten minutes. Asserted against a deep-copy snapshot, including a real `&anchor`/`*alias` document. `credential_shape_errors` also gains the per-server and per-ELEMENT rows for `mcp_servers`, mirroring what `env_file` already had. The element row is the one that matters — an `env_vars` entry smuggled in as a mapping was the single most dangerous shape in the block and was unnamed. Note this makes the write path (`generate_credential_files` → 400) reject a template that previously created an agent with a garbage declaration: correct per PR-A's fail-loud write contract, and release-noted. `generate_credential_files` is deliberately UNTOUCHED — it still reads `env_file` names-only, which is what makes the forward-compatibility argument true rather than asserted. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dolho
added a commit
that referenced
this pull request
Aug 3, 2026
…ent#180)
An exposed agent's card advertises every `template.yaml capabilities[]` tag, and
the well-known discovery route is unauthenticated by design — so today that full
capability list is world-readable for any exposed agent. This lets an operator
choose what the outside is told.
**A disclosure control, and the code says so.** The card's `skills[]` is
advertisement: inbound `message/send` dispatches free-form text via
`execute_task(triggered_by="a2a")`, with no per-skill routing. Filtering changes
what an orchestrator SEES, never what it may ASK for. That's stated in the
requirement, the seam docstring and the filter itself, because a filter
operators mistake for an invocation gate is a control that looks like security
and isn't. A real boundary (constraining a2a-triggered runs via
allowed_tools/guardrails) is separate work with its own threat model.
Extends the existing ent#157 seam rather than adding a module — same shape as
the inbound allow-list, second provider:
provider.exposed_skills(agent_name) -> Optional[List[str]]
- No provider (OSS) → identity function; the card is byte-identical to before,
by construction. The enterprise module owns the config, storage and UI.
- `None` = no opinion = advertise all: the unconfigured default, so exposure
(already opt-in, default OFF) keeps every existing card unchanged on upgrade.
- `[]` ≠ `None`: an explicit "advertise nothing".
- Stale ids are inert — the selection only subtracts; `template.yaml` stays the
source of truth for what exists.
- Fail-open on provider error (advertise all + WARNING): consistent with the
seam's availability bias and the advertise-all default. Honest only *because*
this isn't a security boundary — failing closed would silently empty a card
and break discovery invisibly.
Both card surfaces (public well-known + authenticated per-agent) go through one
router helper, so they can't disagree and a future third surface gets the filter
by default rather than by remembering. `generate_a2a_card` stays pure — the
provider lookup lives in the helper.
Writing the tests found a real gap: a provider returning a str (a defect) would
iterate into single characters, match no id, and silently empty the card —
fail-CLOSED, the opposite of the contract, and invisible. Malformed returns now
take the same fail-open path as a raised error.
Requirements §32.4 written before the code (CLAUDE.md rule #1); public docs
describe the generic seam only, per the standing enterprise-docs rule.
Related to trinity-enterprise#180
vybe
pushed a commit
that referenced
this pull request
Aug 3, 2026
) Trinity Rule #1 — the requirement lands before the code. Documents the three-part shape (container config-truth probe, start-time drift predicate, deliberate rotation), the ordering/concurrency contract (fail-closed lock, captured-id deletion, DELETE-not-deactivate, DB-only path for stopped agents), the allowlist auth rule, the `stale` health state, and what stays exploitable after the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Aug 3, 2026
…ership retrofit (ent#109) (#1947) * fix(lifecycle): one owner for git env across both rebuild paths (ent#109) `recreate_container_with_updated_config` seeds env from the OLD container and re-derived only `GITHUB_PAT`, replaying whatever `GITHUB_REPO` / `GIT_SYNC_*` each container happened to be carrying. The git-env derivation lived only in `_apply_persisted_auth_env` (`recreate_missing_container`). That split is a pre-existing fleet-wide bug, not a cosmetic one: the recreate has exactly one production caller, `start_agent_internal`, which fires on nine config-drift predicates AND on base-image drift at cold start — so a base-image rebuild arms the replay for every agent at once. `_apply_git_env_from_db` is now the single writer. Three load-bearing details: * **The PAT gate is a parameter, never inherited.** The two paths gate differently on purpose. `per_agent_only` (config-drift recreate) preserves #211 verbatim — resolve the effective PAT only when the container already carries one or a per-agent PAT row exists — so a global-only platform PAT is never injected into a previously-tokenless container. A verbatim lift would have swapped that for the 2-tier per-agent -> GLOBAL resolver used by `effective` (the rebuild-from-nothing path, which has no old container to inherit a token from): `configure_push_remote` then clears the push blackhole and a tokenless agent can push a private KB to the shared public upstream. learnings.md ent#162 names this class exactly. * **Set-or-clear**, since the recreate writes into a carried-forward dict. A deleted `agent_git_config` row pops the whole owned set; a `source_mode` flip clears the mode/branch pair. `GITHUB_PAT` alone stays set-only while a repo is bound — clearing it would revoke a live agent`s push on an unrelated recreate. * **`GIT_SYNC_AUTO` = DB flag OR baked env**, plus a convergence backfill. crud.py`s two writers genuinely disagree (`and not config.ephemeral` sits inside a swallowing try/except on the DB side only; the column defaults to 0), so deriving from `auto_sync_enabled` alone would silently stop auto-push for that slice of the fleet. The backfill writes the column the moment the disagreement is observed, so the OR retires itself. Making the #389 toggle authoritative is a separate follow-up. ent#123 is preserved: the gate is the REPO, not the PAT, so a tokenless agent rebuilt after container loss still clones (#843/#1439 silent-empty class). One deliberate divergence from a verbatim lift, asserted by test: a container with a baked `GITHUB_PAT` and NO git binding previously had that token refreshed from the global platform PAT on every recreate; it is now popped. The per-agent PAT is a column ON `agent_git_config`, so "no row" means no per-agent credential and no repo to push to by construction. Tests: tests/unit/test_ent109_git_env_seam.py — each of the four behaviours proved to have teeth by mutation (un-gate the PAT, flip the call site to `effective`, derive GIT_SYNC_AUTO DB-only, drop the clear sweep, drop the source-mode clear, diverge the GIT_SYNC_AUTO literal, unguard the backfill: all seven go red). Plus a static call-site guard, so flipping either gate fails CI even though no behavioural test of the helper alone would catch it. Refs Abilityai/trinity-enterprise#109 (PR 1 of 3) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(architecture): _apply_git_env_from_db owns git env on both rebuild paths (ent#109) Adds the missing agent_service/lifecycle.py catalog entry and records the per-call-site PAT gate, the set-or-clear contract, and the GIT_SYNC_AUTO OR-derivation. Amends the ent#123 clause to point at the new shared seam instead of _apply_persisted_auth_env. Refs Abilityai/trinity-enterprise#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(registry): register test_ent109_git_env_seam.py Refs Abilityai/trinity-enterprise#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(feature-flows): sync the git env-derivation seam (ent#109) github-sync.md: retitle the rebuild-recovery section to "Container-rebuild env — lifecycle.py::_apply_git_env_from_db" and document the per-call-site PAT gate, the set-or-clear contract, the GIT_SYNC_AUTO OR-derivation, and the two vars deliberately NOT owned. git-sync-health.md: GIT_SYNC_AUTO is re-derived on every rebuild as auto_sync_enabled OR the baked env (the two creation writers disagree), with a self-retiring backfill; kill-switch row and file table corrected. agent-lifecycle.md: Revision History row. feature-flows.md: hand-added Recent Updates row (the skill drops it past ~400 lines). Note: that table is at 56 rows against its stated ~20 cap (#1360) — pre-existing drift, deliberately not trimmed here. Refs Abilityai/trinity-enterprise#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#735): re-anchor the lifecycle PAT call-site guard onto _apply_git_env_from_db ent#109 moved GITHUB_PAT derivation out of the inline block in recreate_container_with_updated_config (which the guard anchored on via the comment "Update GITHUB_PAT") into the shared _apply_git_env_from_db. The guard intent is unchanged and still enforced: that block resolves the effective per-agent PAT, never the platform-only get_github_pat(). Also fixes a silent-degradation flaw in the guard itself. str.find returns -1 on a miss, and src[-1:-1+300] slices to an EMPTY string — so a moved anchor made the guard assert "get_github_pat_for_agent in \x27\x27", failing with no hint about why. The anchor is now asserted first with a message naming the fix (re-point it, do not delete it), and the block is sliced to the next top-level def rather than a fixed byte window. Both failure modes proved red by mutation: swapping the helper to get_github_pat() and renaming the anchored function. Refs Abilityai/trinity-enterprise#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(lifecycle): correct-never-introduce git env; drop the auto-sync backfill (ent#109) Two defects found reviewing the ent#109 PR 1 env seam. 1. The config-drift recreate blackholed push for agents bound post-creation. The repo half of the block is repo-gated (ent#123) while the PAT half keeps #211's narrower per-agent gate, and those two disagree for one real row shape: an agent bound via POST /{agent}/git/initialize on the GLOBAL platform PAT. That path writes an agent_git_config row and pushes, but never recreates the container, never bakes git env, never persists a per-agent PAT row, and never writes the token into the workspace .env — so its only credential is the one embedded in .git/config's origin URL, and startup.sh's #1264 fallback does not cover it. Handing startup.sh GIT_SYNC_ENABLED=true with no GITHUB_PAT is exactly what it reads as "deliberately tokenless": the restart branch rewrites origin to the credential-less CLONE_URL, destroying that token, and configure_push_remote blackholes the push remote — silently, and fleet-wide on the same base-image drift this helper exists to fix. `per_agent_only` now writes the block only when the old container already carried GITHUB_REPO or a PAT resolves. It still corrects a stale repo, a flipped source_mode and a deleted row — every case the fix is about; a tokenless ent#123 agent carries GITHUB_REPO from creation, so the flagship is unaffected. `effective` is exempt: with no old container, NOT introducing the block is the #843/#1439 silently-empty-agent bug. 2. The GIT_SYNC_AUTO backfill erased an owner's explicit disable. PUT /{agent}/git/auto-sync writes the row and nothing else while the agent gates on container env, and creation sets both true for the ordinary non-source-mode PAT agent — so "baked true / DB 0" is also exactly what an owner's disable looks like. The backfill re-enabled it on the next recreate and erased the only record of the intent, so the toggle could never stick. It was a privilege boundary too: PUT .../auto-sync is OwnedAgentByName while POST .../start, which triggers the recreate, is AuthorizedAgentByName — so a shared non-owner, or an agent-scoped key resolving to its owner with the owner's role (trinity-ops-agent#232), flipped an owner-only flag arming a 15-minute background commit-and-push loop. The OR-derivation stays (crud.py's two creation writers genuinely disagree, and DB-only derivation would silently stop auto-push for that slice). The write-back is gone; the disagreement is logged. Making the #389 toggle authoritative remains the tracked follow-up that retires the OR honestly. Tests 17 -> 22: a TestIntroduceGuard class (unbaked container untouched, carried repo still corrected, resolvable PAT still introduces, effective exempt, clear sweep unaffected) and the derive-only assertion. Both fixes proved to have teeth by mutation — removing the guard and restoring the backfill each go red on exactly one test. Two learnings.md entries. Refs trinity-enterprise#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(ent#109): pin the git-env WRITER SET with an AST guard, not a source grep The previous call-site guard sliced `lifecycle.py` by function header and counted a single-line literal in each half. Two blind spots: 1. It pinned only the two KNOWN sites. ent#109's bug WAS that git env had two writers and one of them was wrong; a THIRD writer added later on any container-seeded path re-opens exactly that hole, and the grep version stayed green through a planted `pat_gate="effective"` writer (verified by mutation). 2. `lifecycle.py` names the helper in two comments, so a substring count read prose as call sites — the same first blind spot the #1871 guard hit. The AST walk maps `{enclosing function: pat_gate literal}` and asserts the set equals exactly `{recreate_container_with_updated_config: per_agent_only, _apply_persisted_auth_env: effective}`. It also fails loud on a non-literal or omitted `pat_gate` and on a duplicate call in one function — each of which would make the guard silently vacuous, which is worse than the leak it guards. Also drops the stale "convergence backfill" wording from the module docstring and the registry entry (d8da9d08 removed the backfill; the description still described it) and re-states the idempotence test as "the DB row is never mutated". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: describe the ent#109 guard as a writer-set gate, not a call-site check Follow-on to the AST guard: `architecture.md` and the `agent-lifecycle.md` change log both said the static guard "fails CI if either call site flips", which understates what it now enforces. It pins the whole writer SET, so a third writer on any container-seeded path fails CI too — the property that matters, since ent#109's bug was two writers with one of them wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(requirements): add github §11.12 post-creation repo binding (ent#109) Trinity Rule #1 — requirements before implementation. §11.12 specifies the "bind to your own repo" retrofit: FR-1 the explicit supported-row table keyed on source_mode (the column the partial unique index actually keys on) with named structural refusals for everything else, FR-2 source_mode preserved at 1 so no branch reservation is needed, FR-3 the destination-scoped fail-closed lock + CAS + compensating restore (never delete_git_config on a pre-existing row — that is destruction, not rollback), FR-4 the PAT persisted last, FR-5 the mandatory recreate because startup.sh rewrites origin unconditionally from baked env, FR-6 owner-only AND human-only with explicit PAT disclosure, FR-7 the no_write_credentials surfaces. Also amends §11.11 FR-5: the tokenless push refusal no longer teaches the create-a-new-agent-and-import workaround. Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(fork-to-own): extract the shared destination primitive (ent#109 §4.5) AC #4 asks the post-creation rebind to reuse ent#93's machinery rather than build a parallel path. The seam is NOT the destination triage lifted whole — that is not expressible, because the create path's reuse branch IS the template-tip SHA comparison, interleaved with the triage in one if/elif/else. So the seam is one level lower: inspect_or_create_destination_repo() reports created | empty | branches and never decides. Reuse/refuse POLICY stays in each caller, because the two callers genuinely disagree — the create path compares against a template tip; the rebind has no template, its content source is the agent's workspace volume, so any existing branch is a refusal. validate_destination_pat() is a SIBLING, not folded in: the create path validates the PAT before resolving the template tip, so 'bad PAT + unreachable template' reports FORK_PAT_INVALID. Folding it into the inspect primitive (which runs after the tip resolves) would silently reorder that into a template error. Behaviour preservation is asserted, not claimed: the 40 pre-existing test_fork_to_own.py tests pass unchanged, and both new guards were shown to have teeth — making the primitive refuse instead of report turns the create path's SHA-match reuse red, and swapping the validate/resolve order turns the ordering guard red. Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(git): bind an agent to a GitHub repo you own (ent#109) POST /api/agents/{name}/git/bind-to-own-repo — create a user-owned repo from a LIVE agent's current workspace, rebind origin in place, persist the per-agent PAT, and re-bake the container env so the rebind survives a restart. Plus a GET .../status companion so a client that eats a proxy timeout can resolve the outcome from state rather than from a remembered request. Shape, per requirements §11.12: - Orchestration in services/agent_service/repo_binding.py, NOT the router (Invariant #1). It raises BindError and never HTTPException; the router is a thin mapper owning only the two locks, the idempotency claim, and the audit. - Classification partitions on source_mode — the column the partial unique index actually keys on — and refuses every other shape BY NAME rather than mis-routing it. Credential state is an orthogonal column and is not used. - Concurrency: a DESTINATION-scoped lock is the one that serializes the real collision (two different agents, one destination repo); the agent-scoped lock only guards double-submit. Both FAIL CLOSED with 503 + Retry-After — agent_data's fail-open is calibrated for a tar round-trip, not for two repo creates and two concurrent recreates of one container. - The CAS in db.rebind_git_config is the whole commit point, its predicate named in the docstring. The loser path restores the captured previous values; it never calls delete_git_config, which on a pre-existing row is destruction (the next recreate would drop GITHUB_REPO — #843/#1439). - The PAT is persisted LAST and strictly before the recreate: earlier makes the agent look already-writable on a retry, later bakes a repo-bound container with no token that startup.sh then blackholes. - Post-rewire, origin is read back and confirmed — a set-url that exits 0 without taking effect is exactly the silent mismatch AC #5 forbids. - Owner-only AND human-only (reject_agent_principal): an agent-scoped key resolves to its owner carrying the owner's role, so a role gate alone is satisfied by any agent's injected key on a default admin-owned install. Decision #17 (check_github_repo_env_matches) is deliberately CUT: the only drift-proof way to build it is to call _apply_git_env_from_db, which turns PR 1's AST writer-set guard red, and idempotent retry already supplies the convergence it was meant to buy. BIND_RECREATE_FAILED states the retry path instead of a convergence promise, and warns against a plain restart. Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(git): cover the repo-binding commit point, ordering and secret hygiene (ent#109) 31 tests over the properties that would otherwise only be true by inspection: - Classification: the supported shape succeeds; every other shape is refused BY NAME. Two cases asserted rather than argued — an already-writable agent is an ORDINARY rebind (the refusal that used to sit there is what made the documented retry unreachable), and trinity-system is refused through the no-git-config path so it never reaches the recreate that bypasses #1816's running-system gate. - Commit point: a moved row yields 409 with nothing partial, and the post-commit loser is RESTORED to its captured previous values. Asserts delete_git_config is never called — on a pre-existing row that is destruction, and the row is asserted to still exist afterwards. - Ordering: rebind -> pat -> recreate, proven by recorded call order. A push failure persists no PAT; a PAT-persist failure blocks the recreate; and fail-at-push -> retry -> success is an explicit regression test for the contradiction that a 409-on-retry used to produce. - The CAS statement runs against a REAL SQLite engine, not a double — the predicate is the whole safety argument, so a stub cannot verify it. Includes two racers reading the same expected value: exactly one wins. - Secret hygiene: the PAT is absent from the outcome, the audit dict and every error path, and a stale baked token in git output is redacted too. That last group found a real defect, now fixed: repo_binding composed its failure messages from foreign text (git output, a docker exception) and relied on the producer having scrubbed. git_service scrubs what it reads from a container, but the docker and GitHub exception paths arrive through libraries that never saw the token. Added _scrub() as a belt at the boundary where the PAT is actually in scope. Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(git): retire the no_write_credentials create-a-new-agent workaround (ent#109) ent#230's sharpest AC, which ent#109 omitted: the no_write_credentials surfaces must point at the retrofit once it exists. Both change together (Invariant #13): - git_service.NO_WRITE_CREDENTIALS_MESSAGE (consumed by sync_to_github and reset_to_main_preserve_state, mapped 409 in routers/git.py) - the MCP 409 hint in src/mcp-server/src/tools/git.ts Neither now teaches 'create a new agent with fork-to-own and import your data' — an instruction that discards the agent's identity, its 180-day name reservation and its history. ent#123's carve-out is preserved: this branch still suppresses the chat_with_agent remedy, because a chat turn cannot conjure credentials. The third surface — startup.sh's push-remote blackhole sentinel — is deliberately unchanged and now asserted as such: it is a git remote URL (one shell-safe token) that already names a remedy, and editing it would force a base-image rebuild for cosmetics. The parity guard was teeth-checked in both directions: reverting the MCP hint turns it red, AND breaking the source anchor turns it red with a named error rather than silently asserting against an empty slice — the way a source-grep guard usually dies. Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(ui): 'Bind to your own repo' panel on the Git tab (ent#109) A new BindRepoPanel.vue mounted in GitPanel.vue rather than more markup inside it, for two reasons: GitPanel is already 639 lines, and #1430's raw-color ratchet is PER FILE — appending a form there would raise counts that may only shrink. GitPanel's numbers are unchanged at 24 nongray / 146 gray; the new panel is at ZERO raw non-gray with 51 semantic tokens (its 46 grays are the contract's own surface/ink vocabulary — there are no Base* primitives in the repo yet to absorb them). Design-system contract (read first, per CLAUDE.md rule #10): semantic tokens only (action-primary / status-success / status-warning / status-danger), both themes first-class, gray-750 for dark chrome, and no dark:text-gray-500 — the dark ink floor. Behaviour worth noting: - The store method uses raw axios with an explicit 300s timeout, following the surrounding idiom. It deliberately does NOT use api.js, whose instance-wide 30s timeout is far below this call's worst case; aborting the client mid-bind strands the user past the commit point with no response, which is the exact situation the status endpoint exists to rescue rather than manufacture. - The PAT is read out of the reactive ref BEFORE the await and cleared immediately, so it never lingers regardless of how the request ends. - A client timeout is reported as PARTIAL, never as a clean failure — the request may well have landed. - Post-commit failures render as 'Partly applied — action needed' in warning colour rather than as an error, because the binding genuinely IS saved and telling the user it failed would send them looking in the wrong place. - The restart warning states what happens, what is preserved, and how long. Both SFCs verified against the real @vue/compiler-sfc (parse + script + template); npm run check:tokens passes. Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: architecture + feature flow for post-creation repo binding (ent#109) - architecture.md: two endpoint rows, a Post-Creation Repo Binding subsystem block, the two Redis lock keyspaces, repo_binding.py + git_service's new primitives in the service catalog, the shared destination seam on the fork_to_own entry, and the ent#123 paragraph tail now that its no_write_credentials refusal points at the retrofit. PR 1's _apply_git_env_from_db prose is already present on this branch and was NOT re-added. - New feature-flows/agent-repo-binding.md: the end-to-end trace, the five decisions that carry the design (source_mode partition, destination lock, CAS + restore-not-delete, PAT-last, mandatory recreate), the error registry with which codes are partial, the ent#93 sharing seam, security, and known limits — including why Decision #17's drift predicate was cut. - feature-flows.md: Recent Updates row added BY HAND (/sync-feature-flows drops it past ~400 lines) plus the category-table entry. - Cross-linked the three affected flows: github-sync.md and mcp-git-tools.md had the retired workaround quoted verbatim in their prose, and github-repo-initialization.md now names its post-creation sibling and the boundary between them. Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(git): cover the bind ENDPOINT surface (ent#109) /update-tests review found the router layer uncovered — its own rule says a new or changed endpoint needs a caller that exercises the path params and auth dependency (#1069's 422-every-call class). 26 tests over the five things no service-level test can see: - reject_agent_principal really called, and wired in the handler rather than merely imported (an agent-scoped key resolves to its owner CARRYING the owner's role, so an owner/role gate alone is satisfied by any agent's injected key on a default admin-owned install) - route path-param matches the handler parameter, for both routes - locks FAIL CLOSED on a Redis outage, on a raising SETNX, and on contention; the destination key is case-folded; locks release on success AND failure - idempotency key is verb-folded; absent header derives nothing; in-flight 409; completed replay returns the snapshot with X-Idempotent-Replay - audit on EVERY exit path incl. lock contention and the unexpected 500 Also fixes a regression this work introduced: test_ent123_tokenless_clone.py asserted the literal retired wording of NO_WRITE_CREDENTIALS_MESSAGE, and I had not re-run that suite after changing the shared constant. Re-anchored on the CONSTANT plus the invariant ent#123 actually cares about (named message, still actionable) — stronger than before, and it cannot drift again; the exact copy is owned by test_ent109_no_write_credentials_message.py. Both new guards mutation-verified: deleting reject_agent_principal and making the lock fail open each turn two tests red. Full unit suite: 6350 passed, 14 skipped, 0 failed. Identical under random and fixed order (no sys.modules pollution across the new modules). Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(flows): /sync-feature-flows pass over the ent#109 binding surface Verification of the hand-written docs against the code found two gaps: - template-processing.md described the fork-to-own copy pipeline's steps 1-2 as living inline in fork_to_own.py. They are now the SHARED half (validate_destination_pat + inspect_or_create_destination_repo), so a reader tracing the code would have found the triage in a different function than documented. Updated to name the seam and why it sits one level below the triage, with the reuse/refuse policy explicitly still owned by that caller. Behaviour there is unchanged. - The new flow's error registry was missing three codes that ARE reachable on the bind path: FORK_DESTINATION_UNREACHABLE (shared primitive), BIND_DESTINATION_UNREACHABLE (fail-closed guard-read failure) and BIND_UNEXPECTED_ERROR (router catch-all). Verified by diffing the codes in the source against the codes in the doc; the seven still absent are create-path-only and correctly omitted. Checked and deliberately NOT changed: git-sync-health.md and dark-mode-theme.md reference the touched files but document nothing this PR alters. The Recent Updates table is 66 rows against its own stated ~20 cap — pre-existing drift (65 before this PR); trimming 46 of other people's entries is unrelated churn on a feature PR. Refs ent#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(git): converge the documented bind retry; stop a PAT leaking on rejection (ent#109) Fixes from /review (C1, I1, I3, I6, I2) and /cso --diff (S1) on PR 2. ── /review C1: every post-commit failure promises an idempotent retry, and all four are refused ────────────────────────────────────────────────────────── The CAS is the commit point, so after it the row names the destination while the container's origin still names the old repo — and both pre-flight gates read that skew as a refusal: push/rewire fail -> row moved, origin did not -> 409 BIND_STATE_UNCLASSIFIED PAT/recreate fail -> destination holds our own pushed history -> 409 BIND_DESTINATION_EXISTS The §0.4 class the plan wrote a section to eliminate for the PAT ordering, re-entering through the classification guard. The vestige was in the signature: `_classify(agent_name, destination_repo)` never used `destination_repo` — the carve-out had been designed and not written. A row already naming the requested destination is now a resumption: * origin may lag — it never selects what is pushed (step 4 pushes refs/heads/<branch> from the workspace by explicit URL, writes origin after), and it cannot be tightened anyway: a committed CAS has overwritten the old repo name, so "still the old repo" and "something else" are indistinguishable, and treating the ambiguity as fatal strands the agent. * existing branches are accepted — bounded by git, not trust: the push carries no --force and no `+` refspec, so unrelated history is rejected non-fast-forward and an unrelated branch is untouched. * previous_repo=None on a resume leaves `upstream` alone instead of repointing it at the destination itself, erasing the provenance the rebind preserves. A mismatch against any OTHER repo stays BIND_STATE_UNCLASSIFIED. The regression test written for exactly this was green because its double returned `origin_repo=fake_db.config.github_repo` — the container's observed state WAS the row, so they could never disagree — and a hand-set `dest_state = "empty"` stepped around the other gate. The fixture now tracks the container independently and mirrors the real side effects. ── /cso S1: a GitHub PAT reaches the response body and the platform log ────── A PAT is sent as `Authorization: Bearer <pat>`, and h11 rejects an illegal header value by ECHOING it (verified: `LocalProtocolError: Illegal header value b'Bearer ghp_...\r'`). The validator only checked non-emptiness and returned the value UNSTRIPPED, so a token carrying a trailing \r or \n — what a paste from a terminal or clipboard routinely produces — surfaced raw in a 500 body and, via logger.exception, in the Vector-captured platform log. Trigger is far more often an ordinary paste than an attacker. * `models._validate_pat_secret` strips whitespace and rejects anything outside printable ASCII, on BOTH BindAgentRepoRequest and ForkToOwnRequest (ent#93's create path feeds the same GitHubService constructor). * That alone would only RELOCATE the leak: Pydantic v2 records the rejected value in errors()["input"] and FastAPI returns exc.errors() verbatim — proven against a real TestClient. `error_handlers.validation_error_without_input` strips `input` from every 422 entry. Dropped for all fields, not for names that look sensitive: a name allowlist is the new-producer-missing-from-the- consumer's-list class, and the caller already has the value they sent. * The router catch-all and the PAT-persist log line now scrub, and the dual-scrub itself collapses from two copies into one home in `utils/credential_sanitizer` (fork_to_own re-exports for its callers). ── Also ───────────────────────────────────────────────────────────────────── * The bind is `recreate_container_with_updated_config`'s SECOND production call site and skipped the `clear_agent_breakers` that `start_agent_internal` runs immediately before its own call — both breakers are agent-name-keyed with no TTL, so the replacement container inherited its predecessor's verdict (#1560). Cleared before the recreate, not after. Two stale "one production caller" claims corrected. * Audit rows on the two idempotency-replay exits, so "exactly once per exit path" (#905) is literally true. * Client timeout resolves against the status endpoint instead of telling the user to reload the tab. * Five test modules registered in tests/registry.json. Each of the six behaviour fixes was mutation-checked (revert -> red -> restore), including the breaker clear in both directions (absent, and after the recreate). Verified: 6332 backtest unit tests pass; the original C1 probe — written before the fix and unchanged — now reports both post-commit shapes converging; frontend `vite build` and the design-token check pass; GitPanel's raw-color counts are unchanged from baseline and BindRepoPanel is at raw_nongray 0. Refs Abilityai/trinity-enterprise#109 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(git): assert the bind routes are wired to the enumeration-safe deps (ent#109) Plan §7 lists "uniform 404 for unknown *and* inaccessible agent (Invariant #8)" as a PR 2 case, and it was the one bullet with no test behind it. The 404 BEHAVIOUR is not re-tested here — `test_186_enumeration_uniformity.py` already proves parametrically that both helpers evaluate existence and access before branching, so nonexistent and inaccessible come back byte-identical. Re-asserting that would only re-test the shared dependency. What no dependency-level test can see is whether *this* endpoint routes through it. So the assertion is the identity of the callable actually bound to `agent_name` on each route — `get_owned_agent_by_name` on the mutating verb, `get_authorized_agent_by_name` on the read-only status verb — mirroring the existing `reject_agent_principal(current_user)` getsource guard: an annotation that merely looks right in a diff, or a hand-rolled lookup with a 404-then-403 split, is how the enumeration oracle gets reintroduced. The two scopes are not interchangeable, so both are pinned: swapping them would either lock a shared reader out of a surface the Git tab already shows them, or let one rebind an agent they do not own. Not vacuous: the two dependencies are distinct objects, so binding the wrong one fails the assertion. Route introspection goes through `route.dependant`, not `get_flat_dependant` — that symbol drifts in the verify venv. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com>
vybe
pushed a commit
that referenced
this pull request
Aug 3, 2026
…tup:` + two HARD-gate fixes (ent#128 PR-B) (#1899) * fix(agents): stop a malformed `credentials:` also costing runtime + shared_folders `_resolve_local_template` read `creds.get("mcp_servers", {}).keys()` straight through the block. A null / list / string `credentials:` raises AttributeError there, and that read sits FIRST in a run of `config` mutations wrapped in one broad `except Exception` — so the failure skipped every mutation after it. A single malformed key therefore silently cost the agent its `runtime:` (wrong harness) and its `shared_folders:` config too, with only a WARNING to show for it. Reads through PR-A's tolerant `credential_mcp_server_names()` instead, so the credential parse degrades on its own and the unrelated settings survive. The five malformed shapes are pinned as parametrized regressions; all five fail on the pre-fix code. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(agent-server): tolerant `credentials:` read on GET /api/template/info Same uncaught reach-through PR-A fixed on the backend, still live on the agent image: `.get("credentials", {}).get("mcp_servers", {}).keys()` raises AttributeError on a null / list / string block at EITHER level, and the endpoint's own `try/except` wraps only the YAML load — so the crash escaped as a 500 on the Info tab and the brain-orb route guard. `template.yaml` here is read from the agent's own workspace, which the agent itself can rewrite, so this is reachable without an operator touching anything. The agent server ships in its own image and structurally cannot import `src/backend`, so the reader is DUPLICATED, not imported. The two in-repo precedents for that (`credential_paths.py`, `model_context.py`) are vendored byte-identically WITH a parity test; a 6-line reader does not earn a whole vendored module, but it does earn the same guard — before this commit NO parity test covered `agent_server/routers/info.py`, so the copies could diverge freely. Added in the `test_1713_scheduler_utils_parity.py` shape: one shared 17-row table of malformed shapes driven through BOTH implementations, asserting agreement on OUTPUT (the copies are textually divergent by design, so a source diff cannot verify them). Also routes the endpoint through the existing `get_template_path()` helper — `/api/metrics` already does — instead of a second copy of the path literal, so the regression is testable without patching `Path`. This is the change that makes `/verify-local` mandatory WITHOUT `--skip-agent`. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(compatibility): a credential detector must not read narrower than it audits K-001 (HARD) compared `.mcp.json.template`'s `${VAR}` references against an UPPERCASE-ONLY view of `.env.example`. Trinity's substitution engines impose no charset at all — the agent-side writer is a `str.replace` and the `.env` writer slices `env_val[2:-1]` — so `${my_var}` IS substituted at runtime, and a template that correctly documents `my_var=` was HARD-failed for a gap that does not exist. `services/credential_charset.py` is the one place that decision now lives, named for its ROLE (`CREDENTIAL_DETECTOR_CHARSET` — "the widest charset a detector must accept so it is never narrower than the engine it audits"), not for a reach it does not have. Four detectors adopt it; the docstring carries an explicit NON-MEMBERS list with a reason per entry, because the previous framing ("the charset every Trinity surface agrees on") is false and reads as an instruction to the next engineer who greps `[A-Z][A-Z0-9_]*`: * `mcp_validator._ENV_VAR_REF_RE` is a FAIL-CLOSED gate (`.mcp.json` inject → 400, `.credentials.enc` import, deploy-local), deliberately paired with the WIDEST finder (`[^}]*`). Widening it admits input that is currently rejected. * `skill_packaging.ENV_KEY_RE` is an adjacent domain with its own length cap. * `static_checks._ASSIGN_RE` carries the quantifier shape behind an already-FIXED py/polynomial-redos alert, on an agent-supplied-text path. * `c_d006` is a different vocabulary that merely looks similar. The constant lives in a pure-stdlib leaf module, NOT in `services/compatibility/`: that package's `__init__` imports `database`, and `static_checks` imports `template_service`, so a `template_service` → compatibility edge is a hard cycle (reproduced: "cannot import name '_is_platform_injected' from partially initialized module"). Behaviour changes, both named: * K-001 (HARD) `fail → pass` for a documented lowercase variable — the fix. * K-003 (SOFT) `pass → fail` for a lowercase-only, comment-free `.env.example`. `_env_example_vars` is K-003's precondition for DEMANDING comments, so growing it makes the verdict worse. The verdict is correct — that file genuinely has no comments — but it is a `pass → fail` and is release-noted, not smuggled. * S-010 (SOFT) does NOT flip: its `generic` blocklist is uppercase-exact, so no newly-visible lowercase name can join it. Asserted, not assumed — it is safe by coincidence of casing. The two `template_service` extractors are included because both feed the LIVE `collect_mcp_credential_warnings` → `deploy.py` path; leaving them out would half-fix the very inconsistency this closes while a four-way agreement test passed. Direction there is FEWER spurious warnings. `test_deploy_local_validation.py` (the 8-assertion suite on that path) stays green. Also hardens two latent crashes in the same file: a null / non-mapping `template.yaml` document reaching `extract_credentials_from_template_yaml`, and `extract_agent_credentials` reaching through `credentials:` at three levels. New `credential_mcp_env_vars()` reader returns non-empty strings only, so an `env_vars` element smuggled in as a mapping can never reach a consumer — that element is exactly what turns a set comprehension into `TypeError: unhashable type: 'dict'`. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(compatibility): K-002 compared ${VAR}s against section names, and could go dark Two defects in the same HARD gate, one of them a way for the gate to stop protecting entirely. **1. It read the structure, not the declaration.** `listed` was `set(creds.keys())` — `{"mcp_servers", "env_file"}` — so the documented structured form `credentials.mcp_servers.stripe.env_vars: [STRIPE_API_KEY]` satisfied nothing and HARD-failed a correctly declared template, while `${env_file}` and `${mcp_servers}` PASSED. The admitted set was "whichever section names this template happens to use", so the blind spot was template-dependent — the worst kind, because it cannot be found by reading the check. `declared_credential_names()` (the union of `mcp_servers.*.env_vars` and `env_file`, over PR-A's tolerant readers) is now unioned in, and the three known STRUCTURE keys are subtracted. A flat `credentials: {STRIPE_API_KEY: '...'}` mapping is still admitted — that legacy shape is legitimate and keeps passing. The section subtraction is a deliberate `pass → fail` for a genuinely broken template. Shipped named, tested and release-noted, NOT smuggled under a monotonicity claim: the blanket "strictly monotone, fail→pass only" claim is false and a reviewer would find the counter-examples. **2. It could go dark.** `run_static` caught `Exception` → `skipped`, and `_counts` counted only `status == "fail"`, so a raise inside a HARD check DROPPED `hard_count` and could flip `overall_status` from `issues` to `compatible` on an agent with a genuinely undeclared credential. `c_k002` delegates to `c_t015`, so ONE raise took both HARD gates dark together, and the result is indistinguishable from a clean pass in the counts. The trigger is four lines of untrusted YAML: credentials: mcp_servers: s: env_vars: - {STRIPE_SECRET_KEY: "please"} `template.yaml` here is read from a live agent workspace, whose git repo the agent itself owns — a self-attestation bypass on the surface whose job is to police it. The same `TypeError: unhashable type: 'dict'` is the failure mode that argued against enriching `credentials.env_file` in the first place, so reintroducing it at the new call site would have been the plan diagnosing a bug and then shipping it. Three layers, deliberately: * `c_t015` wraps ONLY the new term and degrades to the narrower set — which makes `missing` LARGER, i.e. errs toward failing — never to `skipped`. * `run_static` returns FAIL for a check that raises. A check that could not evaluate is not a check that passed; one bad check still never breaks the report. * `_counts` also counts `skipped` + `skip_reason == "check_error"` as a finding, at the sink (#1525), so the property survives a future path reintroducing the skip. A benign precondition skip (`no_template`, `ai_not_run`) still counts as nothing — that distinction is why the skip path exists. `declared_credential_names` guarantees `str` elements structurally, and the call site filters `isinstance(name, str)` anyway: the gate must not depend on the reader's contract holding. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(templates): MCP-server precedence, and a credentials badge that counts Three catalog defects PR-A deferred, all in the two builders. **Defect D — precedence was backwards.** `_build_local_template` read `credential_mcp_server_names(credentials_block) or data.get("mcp_servers", [])`, so a `credentials:` block silently OUTRANKED the template's own `mcp_servers:` declaration. `agent_server/routers/info.py` has always read them in the other order, so the catalog and the agent's own Info tab disagreed for any template declaring both. Operands flipped; the `credentials:` path stays as the fallback. **W14 — the GitHub builder had no fallback at all**, so a GitHub template declaring only `credentials.mcp_servers` showed an empty list in the catalog while its Info tab listed them. That was the third of three surfaces; all three now agree. **Defect C / W6 — the badge.** Both builders read a flat top-level `required_credentials:` key that ZERO templates declare — 25 bundled and all 7 configured GitHub repos — so `Templates.vue` rendered 0 for everything. Now derived from the declared base set, with `platform_injected` vars EXCLUDED. That exclusion is the badge's semantic, and it is not cosmetic: measured on the real shipped catalog, a naive derivation is correct on 1 of 7 repos and wrong in both directions — the ent#124 first-run agent would read 5 where the operator supplies 2, while three shipped repos stay at 0. The chip is read as "how much work is this to set up", so counting `GEMINI_API_KEY` / `GITHUB_PAT` / `TRINITY_*` inflates it with rows nobody can fill. A consumer that wants every declared variable wants `declared_credential_names`, not this. Derived unconditionally rather than "explicit key wins, else derive": that override branch is unreachable (no template declares the key), so keeping it would be one dead code path guarding a live one. No frontend change: `Templates.vue:103,107,171,175` read only `.length`, so the shape it already expects is preserved and a variable name never reaches the DOM. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(templates): `credential_setup:` — per-variable credential setup metadata Closes ent#128 AC #1-2. A template can now describe each credential an operator must supply — title, description, required, secret, format, setup_url, default — and `template_service` surfaces the normalized result as `credential_requirements` on every catalog entry. **Enrichment lives in a NEW sibling top-level key; `credentials:` is FROZEN as names-only, forever.** An already-deployed older Trinity reads `env_file` through `credential_env_file_names` and then does `agent_credentials.get(var_name, "")` — hand it a list of mappings and that is `TypeError: unhashable type: 'dict'` at the moment it writes the agent's `.env`. A sibling key is structurally invisible to that binary, so there is no floor version and enrichment distributes immediately. **Base-set-plus-overlay, so the two keys cannot drift.** One record per variable `credentials:` declares, decorated by `credential_setup:` entries joined BY NAME. An entry naming nothing is a named three-line error (problem, cause, FIX) and is dropped; valid siblings survive. `credential_setup:` can only ever decorate — the sibling-key shape's usual failure mode is closed by construction, not by discipline. Stated honestly: for an EXTERNAL template that error is neither impossible nor visible in the UI — `credential_errors` has zero frontend and zero MCP consumers, so the only human channel is the backend log. It is LOGGED. `required` is a tri-state. Enriched-and-omitted means `True` (an author who described a variable meant it); a legacy bare `- FOO` is `"unknown"`, never `True` — it carries no authorial intent, and reading it as required makes a guided checklist cry wolf. `"unknown"` doubles as the enriched/un-enriched discriminator, which is why no `enriched: false` flag is needed. `secret` defaults `True` (fail-safe). Path-free by construction, so trinity#570's `template.yaml` → `trinity.yaml` rename cannot reach it. **The normalizer never raises, and that is load-bearing.** `_build_template` runs in bare list comprehensions in `get_all_templates()`, OUTSIDE PR-A's per-template fence (which covers `_build_local_template` only) — a raise there is HTTP 500 with an EMPTY CATALOG, i.e. PR-A's exact bug reopened by the change that surfaces the new metadata. And no bomb is needed: `title: 123` or a bare `title:` was enough. So the builders ALSO wrap the call and degrade to `[]` plus a named error, rather than fencing the comprehension — that keeps the named error the resilience contract promises. The property does not rest on one function's discipline. (Which earned its keep immediately: the wrapper caught a real NameError during development instead of emptying the catalog.) Trust boundary — `title`/`description`/`setup_url`/`name`/`source` are author-controlled strings from arbitrary GitHub repos flowing into an operator-facing "paste your API key" checklist: * **Type-guard before touching.** Never `str()` a container from untrusted YAML: `str()` EXPANDS a shared alias during the walk (443 B → 52 MB in 1.5 s, x10 per level), and both the sanitizer and the record cap act after that cost is paid. * **Cap the INPUT**, entries AND errors AND the base set. Capping records while leaving `errors` uncapped built a 35 MB response out of the cap meant to prevent it; and `default` had no type row, so the 100-record cap acted as a x100 multiplier on it. * **`source` is sanitized** — it carries the raw MCP server name, the exact string `_sanitize_for_warning`'s own docstring names as the threat, and it was not on the list. * **Per-field length caps.** Reusing the 80-char terminal-warning default truncated a realistic 159-char description and made a real 90-char vendor console URL unusable. * **`setup_url` above scheme-only**: https (case-insensitive — `HTTPS://` is a legitimate author), a parseable host, NO userinfo (`https://google.com@evil.tld` renders as one host and resolves to another — the display/resolve split IS the attack), ≤2048, printable. Validate THEN sanitize, and never through a truncator. Residual documented, not claimed closed: `isprintable()` rejects RTL/ANSI but an IDN homograph survives, so a consumer must render the parsed hostname beside the link. * **Never mutates its input** — `_metadata_cache` holds the parsed dict for 600 s and YAML aliases genuinely share nodes, so one in-place normalize would rewrite both aliased fields and persist for ten minutes. Asserted against a deep-copy snapshot, including a real `&anchor`/`*alias` document. `credential_shape_errors` also gains the per-server and per-ELEMENT rows for `mcp_servers`, mirroring what `env_file` already had. The element row is the one that matters — an `env_vars` entry smuggled in as a mapping was the single most dangerous shape in the block and was unnamed. Note this makes the write path (`generate_credential_files` → 400) reject a template that previously created an agent with a garbage declaration: correct per PR-A's fail-loud write contract, and release-noted. `generate_credential_files` is deliberately UNTOUCHED — it still reads `env_file` names-only, which is what makes the forward-compatibility argument true rather than asserted. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(schemas): trinity-agent-credentials.schema.json — the declaration contract Closes ent#128 AC #3's machine-readable half. Follows the established `docs/schemas/` convention (`agent-pipeline.schema.json`): Draft 2020-12, date-stamped `$id` so a future revision keeps answering for templates written against this one, and self-described as the authoritative documentation contract while the backend reader stays deliberately tolerant. **Rooted at `template.yaml`, not at `credentials:`.** The two keys are ONE contract joined by a mandatory cross-reference, and validating either alone cannot check it. **`additionalProperties: true` at the root and on `credentials`** — template.yaml carries many keys this schema deliberately says nothing about, and a template predating the schema must stay VALID. Accepted asymmetry, and it is asserted as a test rather than left as a surprise: a made-up top-level key IS valid here. **`config_files` is enumerated and `deprecated: true`, not omitted.** The earlier posture was "don't delete, don't advertise", which made the authoritative contract answer VALID to `path: "/etc/cron.d/pwn"`. Undocumented is not a control against an author who knows the key — only against the reviewer who doesn't. So it is documented as deprecated, with a containment `pattern` that rejects absolute and `..` paths and a description saying plainly that it writes files into the agent's credential directory. Still reversible, still invalidates nobody. (Whether to DELETE the key is a public behaviour change and stays @vybe's call.) Carries the A2 consumer requirements in `$comment`, because the schema is the artifact a downstream implementer reads: * a record with `required: "unknown"` carries no authorial intent and MUST NOT be presented as a required field — without this a naive UI renders a seeded agent as five mandatory rows, three of them platform variables nobody can fill; * `platform_injected: true` MUST NOT be asked of an operator; * `secret: true` (the default) MUST be masked; * `setup_url` MUST be rendered with its parsed hostname shown, because the IDN homograph residual is real and documented rather than claimed closed; * there is intentionally NO reverse cross-reference requirement — a declared variable with no `credential_setup:` entry is normal. Also states the author cost honestly in the authoring note: declaring in `credentials:` is a separate edit from referencing `${VAR}` in `.mcp.json.template`, K-002 checks the two agree, and that is deliberate because `.mcp.json.template` must not become a second declaration authority. Plus the two brace forms Trinity's readers cannot see (`${my-key}`, `${VAR:-default}`). Tests pin the schema against the implementation — field caps, the format vocabulary, the allowed-key set, the record cap — so the reviewed text and the enforced text cannot drift. The 13 document cases run under `importorskip` (`jsonschema` is not a declared Trinity dependency); the security-relevant pattern assertions are unconditional. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(templates): the Trinity-installable credential contract + reference examples Closes ent#128 AC #3-4. **Reference examples (AC #4).** The substrate the original plan targeted is gone — `3317247e` deleted `config/agent-templates/cornelius/` in favour of seeding from the public upstream repo — so AC #4 lands on what the bundle actually has: * `scout` / `sage` / `scribe` (the ent#124 seeded trio) declare an explicit `credentials: {}` with the zero-credential contract written out. Absent and empty mean the same thing to Trinity, but *absent* is ambiguous to a HUMAN — it could equally mean the author forgot. `{}` says "considered, and there are none", so the catalog's 0-credential badge is trustworthy. * `test-codex` carries the enriched reference: its one real variable gets a title, description, `required`, `secret`, `format` and `setup_url`. Deliberately NO `GEMINI_API_KEY` in any example: it is platform-injected, so an example asking for it would violate the very rule the guide documents — and it makes a K-002 fixture pass VACUOUSLY, which is how a test proves nothing while looking green. A test asserts no bundled example asks for a platform-injected var. Framed honestly rather than oversold: with one enriched declaration and one names-only one in the bundle, the parity test ("every bundled template normalizes with zero errors") is thin today. Its value is as a RATCHET for ent#137's curated fleet. **The guide (AC #3).** New `## Declaring Credentials` section, TOC renumbered 5→21. Covers the field table, the decorate-don't-declare rule with the actual error text, why `credentials:` stays names-only, the zero-credential contract, degrade-don't-demand, the platform-injected list, fork-to-own composition (ent#109), and the two brace forms Trinity's readers cannot see (`${my-key}` silently dropped, `${VAR:-default}` mis-substituted to an empty string). It also states the AUTHOR COST plainly instead of claiming the design is free: declaring a variable is a separate edit from referencing it in `.mcp.json.template`, and three of Trinity's own six default GitHub templates declare zero credentials while referencing 2-6 and documenting 7-12. Those are K-002-red today and stay red until someone does the edit. Kept that way on purpose — if `.mcp.json.template` counted as a declaration it would become a second authority on what an agent needs, which is the drift this design exists to prevent. The practical order is stated: seed `credentials:` first, enrich second. **Memory docs.** `requirements/credentials.md` §3.5's ✅ was false in both halves and is corrected in place with the correction recorded: the extractor it credited has no production caller, and nothing showed configured-vs-missing status because the badge read a key no template defines. `template-processing.md` and `templates-page.md` get the "two shapes, two owners" table that reconciles the objects-vs-strings contradiction (catalog `required_credentials` = names, `credential_requirements` = objects, extractor `required_credentials` = a different function with the same key name), plus the corrected regex. Compatibility checklist gains six credential rows and a starts-with-nothing-configured row. **No DB change → Rule #9 (dual-track migration) does not apply.** Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(templates): close the ent#128 coverage gaps the gate surfaced A transition diff over a corpus with zero coverage of the diff is not evidence, so the changed statements were measured against PR-B's real base (`c07afab7` = origin/dev + PR-A) rather than assumed. The gate found the new paths that no test reached and this closes them: * §4's new `mcp_servers` shape-error rows — per-server AND per-element, six parametrized cases plus the sanitized-server-name case. The element row is the dangerous one and it had no test. * The write-path consequence, asserted explicitly: `generate_credential_files` now raises on `env_vars: [{K: v}]`, where before it created the agent silently. * `_setup_url_error`'s `urlsplit` ValueError branch (malformed IPv6 literal). * The dedup early-return in the base-record builder — a variable declared under two servers AND `env_file` yields one record with a stable `source`. * A non-string mapping key in a descriptor (`{1: "x"}`), which must not reach the "did you mean" helper. * The caller-less `extract_agent_credentials` across eight malformed shapes. It has no production caller, which makes hardening cheap rather than unnecessary — the next caller would have inherited the crashes. Now exercised instead of merely present. Result: 227 changed statements, 225 executed. The two remaining are a defensive `except OSError` around a `Path.resolve()`, and the three gate files (`static_checks.py`, `compatibility/__init__.py`, `credential_charset.py`) are at 100% of changed statements. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(feature-flows): sync the compatibility flow + index for ent#128 `/sync-feature-flows`. `template-processing.md` and `templates-page.md` were already updated with the declaration standard; this adds the flow the code change actually lands hardest on and which nothing had touched: `agent-compatibility-validation.md`. Both credential HARD gates changed, and the flow doc described neither the defect nor the new semantics: * "a detector must never read narrower than the mechanism it audits" — the shared root cause of K-001 and K-002/T-015, with the NON-MEMBERS list spelled out so the next reader does not "align all the regexes" and widen `mcp_validator._ENV_VAR_REF_RE`, which is a fail-closed GATE and not a detector; * "a HARD gate must not be able to go dark" — the `run_static` →`skipped` + `_counts`-counts-only-`fail` interaction that let 4 lines of untrusted YAML drop `hard_count` 1→0, and the three fail-closed layers that replace it; * the complete verdict-transition set, because the blanket "strictly monotone" claim is false and a reader will find K-003's `pass→fail`. The claim that survives is "no agent gains a HARD failure". Testing section records why the bundled templates cannot prove any of this — 0 `.mcp.json.template` and 0 `.env.example` files, so every changed check short-circuits before reaching changed code and a green diff there is green-because-vacuous — and points at the 49-fixture synthetic corpus instead. Plus the Recent Updates row in `feature-flows.md` (the step this skill's own docs warn gets skipped). Observation, deliberately NOT fixed here: the Recent Updates table carries 57 rows against its own documented "newest ~20" cap (#1360), so the index is 434 lines vs the 400-line guideline. Trimming it means deleting 37 other engineers' entries, which is not this PR's call. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(templates): use an unambiguous placeholder credential value `sk-live-xxx` is stripe-shaped and gitleaks' default ruleset covers `sk-`. The value is arbitrary in this test — it only has to round-trip byte-identically through the `.env` writer — so there is no reason to hand CI a secret-shaped string to reason about. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(agents): teach the crud harnesses the tolerant credentials accessor Real regression I introduced in `886aab5b` and initially mis-attributed as pre-existing. Recording both the fix and how the mis-attribution happened, because the second part is the reusable lesson. **The bug.** `test_1484_create_agent_characterization.py` and `test_1759_local_template_not_found.py` MagicMock the whole `services.template_service` module and stub each function crud actually calls with a faithful return value (`generate_credential_files` → `{}`, `get_github_template` → `None`). `_resolve_local_template` now calls a THIRD one — `credential_mcp_server_names` — and it was unstubbed, so it returned a truthy Mock that passed `if mcp_servers:`, landed in `config.mcp_servers`, and blew up later inside a `yaml.dump` as `ValueError: dictionary update sequence element #0 has length 1; 2 is required`. 18 tests, entirely a harness gap: in production the real function returns a list. Stubbed with a faithful 3-line mirror rather than a fixed `[]`, so a fixture that DOES declare `credentials:` cannot be silently masked by the stub. **One test needed a real update, not a stub.** `test_malformed_field_still_creates_and_names_the_template` used `credentials: "a string"` as its trigger for the broad-except degrade path. That is exactly what `886aab5b` fixes — `credentials:` is no longer a trigger BY DESIGN, because it raised FIRST in that run of mutations and so cost the agent its `runtime:` and `shared_folders:` config as collateral. Swapped the trigger to `shared_folders: not-a-mapping`, which still raises, so the degrade path and the two identifiers in its warning stay under test. The docstring records why and points at the new coverage. **How I mis-attributed it.** I compared with `git stash push -- src/backend`, which reverts only the WORKING TREE — commits 1 and 2 were already committed, so my "baseline" still contained the cause and the failures looked identical on both sides. The `-k`-filtered selection also happened to include only 1 of the 13 `test_1484` failures, which made the set look small and stable. Only a worktree at `c07afab7` (PR-A's tip, PR-B absent) showed the truth: 2 failures there vs 20 on the branch. **A baseline has to be a worktree at the base commit, not a stash.** Now identical to `origin/dev` and to `c07afab7`: 2 failures, both genuinely pre-existing (`test_agent_analytics::test_day_stacks_present_in_by_type`, `test_1069_voip_call_path_param` — the documented `get_flat_dependant` venv drift). Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(templates): the tolerant credential reader must not blow up or raise Two holes in the "never raises, never amplifies" property PR-B rests on, both found by asking which OTHER producers reach the surface the new cap protects. 1. `credential_shape_errors` was uncapped. The cap shipped on the NEW function (`normalize_credential_requirements`), but the same PR added a per-ELEMENT loop to this PRE-EXISTING one, and it feeds the same two surfaces: the catalog's `credential_errors`, and the `"; ".join(errors)` that becomes `CredentialDeclarationError`'s agent-creation 400 body. A cap is a property of the producer, not of the PR that invented the concept. YAML anchors make input size a useless proxy for output size, so the bound has to stop the WALK, not slice the result. Measured on a 6,738-byte `template.yaml` (one 200-element anchor aliased across 200 servers): 40,000 errors / 3.64 MB joined (540x) before, 101 errors / 8,973 bytes after. `origin/dev` returns 0 on the same input, so the amplification is this branch's own — reachable since ent#123 by any creator-role user pointing at an arbitrary public repo. 2. `source_trust not in _SOURCE_TRUST_LEVELS` is frozenset membership, so an UNHASHABLE value raised `TypeError` *on the guard line* — before the degrade-to-`github` branch that guard exists to reach. Unreachable from parsed YAML today (every call site passes a literal), but this is the one function whose docstring makes "NEVER RAISES" load-bearing: a raise here is an empty catalog and a dark HARD gate. The property should be literally true, not true-by-call-site-audit. Both regression tests were confirmed to FAIL with their fix reverted and pass with it restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * build(tests): cap fastapi to prod's 0.115.x line The unit suite was validating against a FastAPI ~25 minor versions ahead of the one production ships. `docker/backend/Dockerfile` pins `fastapi==0.115.6` exactly; `tests/requirements-test.txt` carried an unbounded floor that resolved 0.140.13. The comment at :43 already claimed these "match the floors set in docker/backend/Dockerfile" — that file uses exact pins, so the claim was untrue. Surfaced as `test_1069_voip_call_path_param` failing with `ImportError: cannot import name 'get_flat_dependant'`. That test is only the messenger: it is the one test coupled to a private FastAPI symbol (`src/backend` imports none, and the other test touching `fastapi.routing` uses the public `APIRoute`). The obvious ceiling does not work: `0.140.13 < 0.141` is true, so `<0.141` still admits the breaking version. Bisected against the real wheels — present in 0.140.6, gone in 0.140.7 — a private API dropped in a PATCH release, so no minor-level bound is trustworthy. Tracking prod's line is the durable fix. Why now rather than "separate follow-up": CI is green only on a warm pip cache. backend-unit-test.yml keys `cache-dependency-path` on this file, and 0.140.13 allows py3.11, so the next edit to this file for ANY reason busts the key, re-resolves, and breaks CI for everyone. Capping is the safe way to bust that cache — the change that invalidates the key is the one that makes re-resolution correct. Follows this file's own precedent (`bcrypt>=4.2.0,<5`, added when bcrypt 5.0.0 removed the `__about__` shim passlib reads): floor + ceiling + a comment saying why, rather than an exact pin that would break the file's `>=` convention. Verified by execution, not argument: - full tests/unit at 0.115.14: 5861 passed, 16 skipped, 2 xfailed, 0 failed (at 0.140.13 the same command is 1 failed, 5860 passed) - the edited file installs clean in a fresh venv and resolves 0.115.14 - an existing verify venv self-heals: pip downgrades 0.140.13 -> 0.115.14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(templates): take the trust label from the caller, not a tainted path `_build_local_template` derived `is_bundled` itself: is_bundled = template_dir.resolve().parent == _local_templates_dir().resolve() `template_dir` on the by-id path is `_local_templates_dir() / name` where `name` comes from a user-supplied `local:<name>` template id, so this called `.resolve()` on attacker-influenced input. CodeQL flagged it as `py/path-injection` (alert 260, high) — a new tainted-path sink introduced by ent#128 purely to pick a log level (`source_trust` selects `logger.warning` vs `logger.info` and nothing else). `is_bundled` is now a required keyword arg supplied by whoever knows the provenance: - `get_local_templates()` iterates the curated root, so its children are bundled by construction -> `is_bundled=True`. - `get_local_template()` decides from the id STRING (plain single segment, no separator, not a dot-segment) rather than a path operation on it. Behaviour, measured against the old predicate across 9 ids: 7 identical, 2 divergent — `'../agent-templates/sage'` and `'a\b'` go True -> False. Both moves are old=True -> new=False, i.e. strictly more conservative: the new check never grants the `bundled` label where the old one withheld it, only the reverse. An id that traverses to arrive inside the curated root is not curated, so the new answer is also the more correct one; the blast radius either way is one log level. This is deliberately NOT a traversal guard — and as of the 2026-08-02 rebase it no longer needs to be. An earlier version of this message said the traversal was "being routed as its own issue rather than fixed"; that issue, #1900, has since been fixed on `dev` by #1935, which this branch is now rebased onto. `get_local_template` therefore routes `name` through `contained_template_dir()` — a name allowlist plus resolve + `is_relative_to` — BEFORE the label check runs. (The traversal was real while it lasted: `local:..` escaped the templates dir, reachable by any authenticated user via `GET /api/templates/{id:path}`.) That makes the `is_plain_segment` check redundant today — provably True wherever it is reached, since the barrier above rejects every non-plain name first. It is kept as defence in depth: it decides a trust LABEL, and `contained_template_dir` is a shared primitive the remote-template-registry work (trinity-enterprise#14) is expected to edit. A label that silently became `bundled` if that barrier were ever widened is the exact failure this keyword argument exists to prevent. It re-adds no tainted-path sink — it reads the id string, never the filesystem. Two test call sites updated for the new signature. Verified on the rebased branch: full tests/unit 6410 passed / 16 skipped / 0 failed; no `template_dir.resolve()` remains in the module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Aug 3, 2026
) Trinity Rule #1 — the requirement lands before the code. Documents the three-part shape (container config-truth probe, start-time drift predicate, deliberate rotation), the ordering/concurrency contract (fail-closed lock, captured-id deletion, DELETE-not-deactivate, DB-only path for stopped agents), the allowlist auth rule, the `stale` health state, and what stays exploitable after the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Aug 3, 2026
…t creation (trinity-enterprise#89) (#1946) * docs(requirements): template-declared schedules at creation (§10.16, ent#89) Requirements-first per Rule of Engagement #1 — written and committed before any implementation. Covers the declared `schedules:` contract, the total-function reader and its tolerance matrix, the normalized carrier that feeds BOTH resolver branches (the `github:` half needs the creation-resolved PAT + parsed ref, not the catalog's global-PAT cached fetch), the honored-`enabled` decision and its `set_autonomy_status` caveat, idempotency in all three places creation / intra-block / manifest deploy, and the T-018 + A-002 + c_p006 compatibility half. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(templates): tolerant `schedules:` reader for template.yaml (ent#89) New leaf `services/template_schedules.py`: `schedule_shape_errors` + `normalize_declared_schedules` over one private `_parse`, mirroring the sibling `credential_shape_errors` / `credential_mcp_server_names` convention (ent#128). The contract is TOTALITY — template.yaml is untrusted and `yaml.safe_load()` can yield a scalar, list or mapping at any level, so a raise here would empty the template catalog (#1835 class), enter the creation rollback fence, or fail-open the T-018 check. Every shape degrades to a safe value plus a named error. Cron and timezone are validated with the SAME parser the dedicated scheduler registers with (#1472) because `_calculate_next_run_at` swallows a bad cron and `set_schedule_enabled` never re-validates — an unvalidated entry would become a zombie schedule that exists, shows no next run, and can never fire. Errors name the index, the key and a YAML type — never the `name`/`message` VALUE, which is unbounded and lands in a persisted, UI-rendered blob. The cron/timezone strings are the one echo, bounded and printable-filtered by a local twin of `_sanitize_for_warning` (importing it would close a cycle). Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(templates): surface `schedules:` in both builders, fence both GitHub list paths (ent#89) Three changes in one file: 1. `_build_template` (GitHub) and `_build_local_template` (local) both surface `schedules` (normalized) + `schedule_errors`. BOTH, explicitly — the pre-existing asymmetry (`persistent_state` is surfaced only by the GitHub builder) means parity cannot be assumed, and AC #2 covers both sources. 2. R3 — both GitHub catalog list paths were BARE list comprehensions. ent#128 PR-A fenced `_build_local_template` only, so adding an untrusted-input reader to `_build_template` would have put a new raise-capable call on an unfenced path: one malformed repo would 500 the whole GitHub half of GET /api/templates. That is the #1835 bug this feature is modelled on, re-opened by the feature itself. Both are now fenced per-template via `_safe_build_github_template`. 3. `fetch_template_metadata_for_create` — the creation path must NOT read the catalog cache. That cache uses the GLOBAL platform PAT (creation resolves per-agent -> per-user -> global, ent#162), sends no `?ref=` (so an `@branch` create would read the default branch), and is a 10-minute per-process TTL. The new fetch takes the resolved PAT + parsed ref, and is loud on every failure — a silently empty declaration on the `github:` path is the exact class this feature exists to close. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(agents): materialize a template's declared schedules at creation (ent#89) `_TemplateResolution.declared_schedules` is a normalized carrier populated by BOTH resolver branches, so AC #2's "GitHub and local" is real rather than nominal. It is deliberately not folded into `template_data`: that field is raw template YAML on the `local:` path and `{}` on the `github:` path — which has never populated it, so #383's `persistent_state` and #1169's `data_paths` are effectively `local:`-only — and `_stage_config_files` gates credential-file generation on `if template_data:`, so merging the two shapes would change credential generation for every GitHub agent. `reconcile_declared_schedules` is shaped as a reconcile primitive (takes `agent_name`, not `AgentConfig`) so a future "re-apply template" can reuse it. No recreate hook is added — an eager re-materialize would resurrect schedules an operator deliberately deleted. Two failure modes handled explicitly: - `db.create_schedule` RETURNS None on three paths (unknown user, no access, the #1445 is_agent_live gate) and never raises, so a try/except alone would catch nothing and a length-derived counter would report schedules that were never written. The return value is checked and counted as failed. - The whole step is non-fatal and the try/except wraps the entire call including `list_agent_schedules` — this function sits inside the destructive rollback fence, so an escaping raise would roll back a successful creation over a schedule. `enabled` is passed explicitly (ScheduleCreate defaults it to True, which would invert AC #3), and ghosts are skipped at the caller per ent#69 fleet hygiene. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(systems): don't duplicate a schedule the template already materialized (ent#89) `deploy_manifest` creates each agent and THEN calls `create_schedules`, so post-ent#89 it is the second schedule producer for the same agent: the first call materializes the template's declared block, the second adds the manifest's. With no UNIQUE(agent_name, name) index, a manifest declaring `daily-briefing` on a template that also declares it produced two rows. This is a regression ent#89 itself creates, so the guard ships with it rather than as a follow-up. Fails open on a read error — dropping a manifest's schedules would be worse than the duplicate this prevents. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(compat): T-018 schedules well-formedness + three in-radius corrections (ent#89) T-018 (SOFT, STATIC) reports the `schedules:` block's STRUCTURE only, sharing one reader with the materializer and the catalog surface so the report cannot drift from what creation actually does. It deliberately does not report cron syntax — see correction 2. It fails CLOSED, against the grain of every other check: `run_static` turns a raise into `skipped` and the report counts only `fail`, so a raising SOFT check drops soft_count 1->0 and flips overall_status issues->compatible exactly when its finding was the only failure — the entire population T-018 exists to serve — and `_report_from_persisted` then replays that clean bill of health from checks_json on every stopped-agent read. Detail carries the exception TYPE only; `str(e)` can embed untrusted template content into a persisted, UI-rendered blob. Three corrections, all in blast radius: 1. `c_p006` was missing the `isinstance(..., list)` guard its four sibling readers of this field all carry. `schedules: 5` raised TypeError -> swallowed -> a HARD check silently vanished from hard_count. A live instance of the exact class T-018 guards against, which is what makes the fail-closed design evidence-backed rather than theoretical. 2. `_valid_cron` (A-002) was a per-field `^[\d*/,\-]+$` regex, wrong in BOTH directions: it rejected `0 9 * * MON` and accepted `99 99 * * *`. It now delegates to `validate_cron_expression` — the same parser the scheduler registers with (#1472) and the same one the ent#89 reader gates on. One cron authority, agreeing with the executor. 3. `run_static`'s swallow now logs. It previously left no trace anywhere, for all ~100 checks; this is the instrument for deciding later whether to flip it to `fail` platform-wide. Catalog 100 -> 101. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(templates): stop echoing the schedule name in the duplicate error (ent#89) The error list is persisted into agent_compatibility_results.checks_json, rendered in the UI, and returned in the catalog response, so the discipline is index + key + type name only. The entry index already identifies the offender; the name added disclosure without adding actionability. Cron and timezone stay the only echoed values — bounded, printable-filtered, and what makes those particular errors fixable. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: cover template-declared schedules end to end (ent#89) Three new files plus compatibility extensions, 143 tests. test_ent89_template_schedules.py — the reader's tolerance matrix (40 rows), error-string discipline (no name/message/description ever echoed; cron sanitized and bounded), the catalog surface in BOTH builders, the GitHub list-path fence, and the create-path fetch. Plus a Hypothesis property over recursive JSON-ish values asserting totality — a 40-row matrix cannot be a totality proof for `yaml.safe_load` output, and totality is this module's entire contract. test_ent89_schedule_materialization.py — REAL DB rows via db_harness, not `mock.assert_called`. Both recorded lessons apply here: a mock-`db` suite is blind to a facade gap, and a parameter only one branch consumes is a severed wire a mock will happily confirm — which is precisely the failure history of the `github:` half of AC #2. So both resolver branches are driven for real, and the github fetch's PAT and ref are asserted; without that the §0 regression test would be self-attestation. test_ent89_manifest_no_duplicate.py — R5, in creation order: template first, manifest second, one row, and the template's row is not overwritten. test_compatibility_checks.py — T-018 pass/fail/absent, its fail-closed branch (and that `detail` never carries `str(e)`), a `build_report`-level test pinning the DIRECTION (a raising reader must still yield `overall_status == "issues"`), the `_report_from_persisted` recompute, A-002 in both directions and agreeing with the materializer, c_p006 on `schedules: 5`, the run_static log, and the whole static catalog run against 7 hostile templates asserting no check lands at `check_error` — a T-018-only assertion would never have caught c_p006. The two fence tests pin `get_github_templates` by sys.modules KEY rather than by module object: `get_all_templates` imports it lazily inside the function, so an attribute patch on a separately-imported reference passed in isolation and failed under the full suite, where an earlier file swaps that object. Full unit suite: 6405 passed. The one red, test_1069_voip_call_path_param, is a pre-existing venv FastAPI drift (`get_flat_dependant` no longer exported) in a file this branch does not touch. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(architecture): template-declared schedules + the compat fail-open class (ent#89) - template_service bullet: the `schedules:` reader beside the `credentials:` ones, the newly fenced GitHub catalog list paths, and why the creation path needs its own metadata fetch rather than the global-PAT default-branch cache. - New `template_schedules.py` leaf bullet with its totality contract. - crud bullet: the `declared_schedules` carrier, why it is not `template_data`, and the non-fatal reconcile step. - Compatibility block: T-018, why it is the one check that fails closed, and the two live instances of that class it fixes (c_p006, _valid_cron/A-002). - template.yaml file-tree note now names its four declarative blocks. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(flows): template-declared schedules + the compat fail-open class (ent#89) - template-processing.md: the `schedules:` reader beside the `credentials:` ones, the three deliberate differences (normalized surface, both GitHub list paths fenced, creation does not read the catalog's copy), and the error-string discipline. - scheduling.md: new flow 1c — the fourth schedule producer, why the carrier is not `template_data`, why non-fatality is the invariant, the falsy-return check, and idempotency in all three places. - agent-compatibility-validation.md: T-018, why it is the one check that fails closed, the A-002 cron-authority consolidation, and the c_p006 live instance. - feature-flows.md: Recent Updates row (required even when the flow docs already existed). - learnings.md: the durable half — a per-item swallow plus a count that ignores skips makes a validator's own bug invisible, and persistence replays it. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: GitHub contents-API contract + live catalog surface for ent#89 Two gaps the /update-tests coverage checklist surfaced: - Every other create-path-fetch test stubs `_fetch_template_yaml_result`, i.e. BELOW the HTTP layer — so a wrong param name (`?ref=` is what pins the revision) or a dropped Authorization header would leave them all green while the feature silently read the default branch, or read nothing for a private repo. That is precisely the R2 failure this fetch exists to prevent, so it is now asserted at the wire, including the ent#123 tokenless case sending no Authorization header at all. - The unit suite proves both BUILDERS emit `schedules`/`schedule_errors`; only a live call proves the router serves them, that entries are the normalized shape, and that a template reporting errors is still LISTED — the ent#128/#1835 property, which now also covers the two newly-fenced GitHub list paths. Unit run: 208 passed across the four ent#89-touched unit files. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(flows): sync system-manifest.md with the ent#89 schedule dedupe /sync-feature-flows caught what the plan had deliberately trimmed: the `create_schedules()` section documents a function whose behaviour this change alters. Its docstring block gave no hint that the function is now the SECOND schedule producer for the same agent. Adds the name-match skip, why it exists (no UNIQUE(agent_name, name) index and adding one is a dual-track schema change that would fail on installs already holding duplicates), that the skip does not overwrite the template's row including its `enabled` value, and that an unreadable existing set fails open. Index row now lists the flow too. Noted, not acted on: `feature-flows.md` Recent Updates carries 65 dated rows against the ~20 cap #1360 set — pre-existing drift, out of scope here. Refs trinity-enterprise#89 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(templates): sanitize the create-path fetch's failure reason (ent#89) `fetch_template_metadata_for_create`'s WARNING interpolated `reason` raw while its two neighbours on the same call are `_sanitize_for_warning`-wrapped. The value embeds `str(e)`, and an httpx error message carries the request URL — i.e. the caller-supplied `owner/repo`. A repo with no `/` skips `_GITHUB_REPO_PATH_RE` upstream, so control bytes do reach that line. Bounded at 200 rather than the 80 default: this WARNING exists to be diagnosable, and an 80-char truncation defeats its purpose. Raised as a code-consistency item by the /review pass; the CSO diff audit discarded it as a finding under hard exclusion #9 (log spoofing), so this is hygiene inside one call, not a security fix. Tests pin both halves (control-char stripping and the length bound) and both fail without the change. Also renames a stale `_template_schedule_errors()` reference in the requirements doc and cites #1945 for the autonomy-clobber follow-up the doc previously promised without an issue number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(security): CSO diff audit for ent#89 template-declared schedules PASS with 1 MEDIUM (LLM cost amplification: declared schedules meeting the pre-existing `set_autonomy_status` clobber). No CRITICAL, no HIGH. The finding is an amplification of a pre-existing mechanism — there has never been a per-agent schedule cap — so it is filed as #1945 rather than changing this PR. Matches the convention of the 98 reports already tracked here; the file's Trend section is only meaningful alongside its siblings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The system agent was missing required Docker labels (trinity.ssh-port, trinity.cpu, trinity.memory, trinity.created), causing port allocation conflicts when creating new agents.
Without trinity.ssh-port label:
This fix adds all required labels to match the standard agent creation pattern in routers/agents.py, ensuring proper port tracking and conflict prevention.