Skip to content

feat(canvas): share a canvas at a link, and download it as a PDF (ent#554) - #2623

Merged
vybe merged 28 commits into
devfrom
feature/554-canvas-share-export
Sep 14, 2026
Merged

vybe merged 28 commits into
devfrom
feature/554-canvas-share-export

Conversation

@dolho

@dolho dolho commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Fixes abilityai/trinity-enterprise#554

Implements abilityai/trinity-enterprise#554 — a canvas can now leave the Workspace: a share link, and a PDF.

Stacked on #2619 (ent#553). Base is feature/553-canvas-lifecycle, so this diff is only the share/export work. Review #2619 first, or read this one's three commits and ignore the first two.

The share link

Two scopes, and the default is the narrow one.

scope reach enforcement
authorized (default) the people who could already see the canvas a DEEP link — the view requires a signed-in principal and re-checks can_user_access_agent. The link points at a canvas; it never grants access to one.
public anyone holding the URL explicit, separate, audited; protected only by 256 bits of token entropy

Failing narrow is enforced in five independent places — the column default, the Pydantic default, normalize_scope's fallback for an unrecognised value, the order of SHARE_SCOPES, and the radio the dialog preselects — because a link that reaches further than the sharer understood is the one failure this feature must not have. Creating a public link audits under its own action so "who made this readable by anyone with the URL" is answerable without reading payloads; the token never enters the audit row (it is the capability — the G-04 rule).

Why a separate table, and not agent_public_links

That table has a type column that looks purpose-built for this. Reusing it would have been a real vulnerability: nothing in its 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, unexploited only because nothing creates those rows today (db_models.py: "currently only 'chat' is supported"). A separate table makes the isolation structural rather than dependent on every consumer remembering to check. Pinned by a test that fails if that read path ever starts filtering on type, at which point the decision is worth revisiting.

Live, and it says so

Operator ruling: the link renders the canvas as it is now, with 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 stamped, never deleted), because a revoked link has to be able to say it was revoked — which it cannot once the row is gone. The status vocabulary splits along disclosure: revoked and expired are returned only for a token that matched a row, since whoever holds such a link was already told the canvas exists; 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.

The PDF: print-first

The path the issue recommended, and it held up. 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 "works identically 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 whatever theme the viewer is in, and when window.print is unavailable the control says why and the share link still works.

Boundary with ent#425

ent#425 (hosted deliverable pages) is still open, so by this issue's own rule the narrower share link ships here and #425 adopts it later. Decided by fact, not preference.

Verification

Exercised against a real database, not stubs — the full resolution matrix:

scope + viewer result
public + anonymous ok
authorized + anonymous sign_in_required
authorized + owner ok
authorized + stranger not_authorized
unknown token not_found
revoked / expired revoked / expired
unparseable expiry expired (fails closed)
canvas deleted after sharing not_found

Plus: views counted only on a successful render, cross-agent revoke refused, a second revoke preserving the first revocation time, and 256-bit unique tokens.

23 backend tests and 20 vitest cases for the pure rules (canvasShare.js — vitest runs environment: 'node' with no mount harness, so a rule inside an SFC is one no test can reach).

Docs: flow doc gains sharing + export; user doc gains a section; set_canvas's MCP description now tells agents a canvas may be shared, so titles are written for someone to read.

Related to abilityai/trinity-enterprise#554

🤖 Generated with Claude Code

https://claude.ai/code/session_01CdxGmvuKuqaWJZ6pKUgaUJ

dolho and others added 5 commits September 8, 2026 15:21
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
…ment 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
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
…he 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
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
@dolho
dolho requested a review from vybe September 9, 2026 08:14
dolho and others added 5 commits September 9, 2026 11:15
…eachable

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
…ecycle

# Conflicts:
#	src/backend/client_portal/service.py
#	src/backend/models.py
#	src/backend/routers/canvas.py
#	src/backend/services/canvas_service.py
…t#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
… 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
@dolho

dolho commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Re-review. The share design is careful — the separate table over agent_public_links is the right call and the argument for it (nothing in that read path filters on type, so a canvas row there is a working public-chat token) is the strongest thing in this PR. parse_iso_timestamp returns tz-aware UTC on both arms, so _is_expired has no naive/aware trap, and print-first for the PDF holds up.

One blocker, and it is not visible from this branch.

The Alembic graph forks the moment #2619 lands. This PR is stacked on feature/553-canvas-lifecycle, but its base is behind that branch by 4 commits — including the one that renumbered the pinned migration 0058_agent_canvases_pinned → 0059_agent_canvases_pinned (down 0058_portal_file_dismissals). This branch still carries the old 0058_ file and chains off it:

0059_agent_canvas_shares.down_revision = "0058_agent_canvases_pinned"   # gone after #2619

Merging the current #2619 in reproduces it — git merge resolves the rename by dropping the 0058_ file, so this PR's parent becomes dangling and the repo's own required guard fails:

$ python3 scripts/ci/check_alembic_heads.py src/backend/migrations/versions
alembic-heads: FAIL — resolves to 2 heads across 61 revision(s); exactly 1 is required.
  • 0059_agent_canvas_shares
  • 0059_agent_canvases_pinned
The heads share no common ancestor in this directory.

alembic upgrade head is singular and resolves its target before applying anything, so that graph applies zero revisions — every revision since the fork stops arriving, not just this one. On PostgreSQL that is a boot failure, and agent_canvas_shares never gets created.

CI here is green only because it is measured against the stale base. That is the part worth taking away: a stacked PR's migration graph is valid only against its base's current head, and neither branch's own checks can see the fork — #2619's tree has one head, this tree has one head, and the union has two. It is also the same class the #2619 review already caught once ("the Alembic fork is gone"); it moved one PR up the stack rather than being resolved.

Fix is small: merge #2619, drop the duplicated 0058_agent_canvases_pinned.py, and repoint 0059_agent_canvas_shares.down_revision at 0059_agent_canvases_pinned. Renumber it to 0060_ while you are there — two files sharing the 0059 prefix is legal (ids are strings) but the numeric prefix is the graph's only human ordering cue.

#2628 is stacked above this and inherits the same fork (0060_execution_open_canvas → 0059_agent_canvas_shares), plus a merge conflict against the current #2619. Whatever order you fix these in, the three have to be re-based as a stack.

Two smaller things, neither blocking:

  • resolve() calls db.record_canvas_share_view() on the OK path, so an unauthenticated GET on a public link is a DB write. check_public_link_rate_limit bounds it at 60/min/IP, which is the same budget the other token routes share, so this is noted rather than objected to — but it is worth stating in the docstring that the view counter is a write on a read-shaped path, since that is exactly the shape ent#545 was filed about.
  • SIGN_IN_REQUIRED (401) is returned to an anonymous caller for a live authorized-scope token, while an unknown token gets 404. That is a token-validity oracle for someone with no credential at all — narrower than it sounds, since it needs the 256-bit token in hand, and consistent with the docstring's own rule ("whoever holds such a link was already told the canvas exists"). Recording it because that rule is stated for revoked/expired, where the holder has used the link, and this arm extends it to a holder who never has.

dolho and others added 12 commits September 10, 2026 12:44
… (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
Resolve src/frontend/raw-color-baseline.json: keep dev's #2662 notes, totals
patched to the merged tree's real values (per-file entries unchanged on both
sides; scanner + ratchet test verified).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015owqMKD5QDjzZrUTF2Joht
…d 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
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
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
…atch (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
…t#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
…to feature/554-canvas-share-export

# Conflicts:
#	src/frontend/raw-color-baseline.json
@dolho

dolho commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Re-based on the current #2619 (def38cf7d: the 2026-09-11 fix plus dev merged through it). One conflict, in raw-color-baseline.json's named_increases block — both sides' notes kept (_ent553_note for 25 → 46, ent554 for 46 → 62), CanvasPanel.vue stays at 62. check_alembic_heads.py on this tree: 61 revisions, 1 head (0060_agent_canvas_shares ← 0059_agent_canvases_pinned). vitest 2803 passed (128 files); canvas backend suites 135 passed; ratchet unchanged.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RS8FBA7oEtAAv6GamZKong

dolho and others added 2 commits September 14, 2026 10:07
…ecycle

Conflict: src/frontend/raw-color-baseline.json (the `totals` block only).

The two sides moved different counters for unrelated reasons, so the
resolution takes both rather than choosing a side:

- dev (#2718) fixed a scanner false positive — a `#` followed by hex
  digits in rendered copy (issue references like `(#526)`) was read as a
  colour — dropping hardcoded_colors 456 -> 447 across 8 files. Nothing
  on this branch touches those files.
- this branch adds the canvas delete/pin/search chrome, which raises
  semantic_tokens 4020 -> 4030.

Merged totals are therefore hardcoded_colors 447 (dev's) and
semantic_tokens 4030 (this branch's); raw_nongray, raw_gray and
files_with_violations were identical on both sides and merged cleanly.

`totals` is informational — the ratchet compares PER-FILE counts — but it
is kept honest anyway. Verified with the gate itself:
`npx vitest run tests/unit/rawColorRatchet.spec.js` → 14 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ
…hare-export

Carries the dev merge resolved in ent#553 down the stack.

Conflict: src/frontend/raw-color-baseline.json, in two places, both from
the same cause — this branch re-froze the baseline before dev's #2718
landed, so the two sides edited neighbouring lines for unrelated reasons.

- `refrozen` notes: union, not a choice. dev's `_2718_note` (the scanner
  false-positive fix) and this branch's `ent554` note (CanvasPanel
  raw_gray 46 -> 62 for the Share / Download PDF chrome) both survive,
  with the shared `_2616_note` between them.
- `totals`: raw_gray 8760 from this branch — 8744 already counted
  CanvasPanel at 46, and ent#554 adds the +16 that the per-file entry
  (which merged cleanly at 62) records. hardcoded_colors 447 from dev's
  scanner fix. raw_nongray, semantic_tokens and files_with_violations
  merged cleanly.

Verified with the gate itself:
`npx vitest run tests/unit/rawColorRatchet.spec.js` → 14 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ
@dolho

dolho commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Review: /review + /validate-pr

Reviewed against the merge-base with feature/553-canvas-lifecycle (46f4928e1), so this is only the share/export work. Recommendation: REQUEST CHANGES — one critical finding, confirmed by running the code rather than reading it.

❌ [C1] An agent-scoped key can mint a public share link for its own canvas (Confidence: 9/10)

src/backend/routers/canvas.py — create_canvas_share:

"""Mint a share link for one canvas (ent#554).

Owner-or-admin and human-only via `_gate_human_removal`: a share is a
GRANT, not a use, and the agent that writes a canvas does not decide who
outside the platform may read it.
"""
_gate_human_removal(current_user, name)

_gate_human_removal is not human-only. Its own docstring (added by #2619, one screen above) says so:

* an **agent-scoped key** may only touch its OWN canvas — unchanged, the
  #918 self-gate. `clear_canvas` is an agent tidying up after itself.
...
if current_user.agent_name:
    _require_self(current_user, name)
    return

So the gate admits an agent key acting on its own agent, and CanvasShareCreate accepts scope="public". An agent that is prompt-injected can POST /api/agents/<self>/canvas/<id>/share {"scope":"public"} with the TRINITY_MCP_API_KEY already in its container, and publish whatever it chooses to write into that canvas at an unauthenticated URL.

Verified, not inferred — four probes against the real gate, all passing:

probe result
create_canvas_share source contains _gate_human_removal(, not _gate_pin( ✅
its docstring contains the string human-only ✅
_gate_human_removal(User(agent_name="agent-a"), "agent-a") raises nothing ✅
_gate_pin(User(agent_name="agent-a"), "agent-a") raises HTTPException(403) ✅
CanvasShareCreate(scope="public") is accepted ✅

This repo's own test already documents the behaviour — test_ent553_canvas_lifecycle.py::test_an_agent_key_may_not_pin_its_own_canvas ends with the line canvas_router._gate_human_removal(_user(agent_name="agent-a"), "agent-a"), asserting exactly this pass-through as correct for delete.

Why this is the more consequential instance of a class #2619 already fixed. _gate_pin's docstring makes the argument verbatim: "'no tool exposes it' is a property of the client, and this route is reachable with the agent's own key." That reasoning was applied to pin, which reorders a list. It was not applied to share, which publishes content to the open internet. Three things compound it:

  1. The share is LIVE, not a snapshot (the service docstring says so, and test_a_shared_canvas_shows_current_content_not_a_copy pins it). So one minted link is a persistent, self-updating exfiltration channel, not a one-time disclosure.
  2. The agent controls the content completely — it is the only writer of canvas blocks, so anything it can read it can copy into a canvas and publish.
  3. audience is never consulted on the share path. ent#438 makes audience fail-closed so "a canvas reaches a Workspace client only because the agent said so"; a public share serves an operator-audience canvas to anyone with the URL. For a human owner that is a defensible override. For the agent itself it removes the last gate.

list_canvas_shares uses the same gate and returns the token in its payload (its own docstring: "this read hands over the capability itself and cannot be wider than the write"), so an agent key can also enumerate live share URLs for its own agent.

Fix: point create_canvas_share, list_canvas_shares and revoke_canvas_share at _gate_pin — it is already exactly the right predicate — or factor it out under a name that reads for both uses (_gate_human_only). Worth a test mirroring test_an_agent_key_may_not_pin_its_own_canvas, since the current suite has 23 tests and none covers the agent-key case.

/review — everything else

Checked and clean, with the line that proves it:

  • Token entropy — secrets.token_urlsafe(32), 256 bits, matching the agent_shared_files.download_token shape. A public link's only protection, and it is the right one.
  • Failing narrow — normalize_scope is an allowlist defaulting to authorized (bug: subscription reports "rate-limited" for the provider's own warning tier — allowed_warning fails the _headroom_indicates_limited catch-all #2396's rule), and the PR body's claim of five independent narrow defaults holds up.
  • The agent_public_links analysis in the PR body is correct and is the best thing in this diff. Reusing that table would have made a canvas token a working public-chat token, because get_public_link_by_token / is_link_valid / _validate_public_link all resolve a token without filtering on type. A separate table makes the isolation structural. Good call, and good that it is pinned by a test.
  • Enumeration — revoked / expired are returned only for a token that matched a row; everything else collapses to one not_found. The reasoning in the service docstring is right: a holder of a dead link has already been told the canvas exists.
  • get_optional_user — delegates to get_current_user instead of re-implementing it, and the docstring warns against reuse on a route that would otherwise require auth. Correct construction for the one caller.
  • XSS on an unauthenticated same-origin page — agent-authored html blocks go through sanitizeCanvasHtml (DOMPurify) at CanvasBlock.vue:132, and diagrams through sanitizeSvg. Pre-existing from ent#438/fix(migrations): swallow duplicate-column race on cold start (#456) #537, not introduced here — but note that ent#554 is what first puts that output on a page a stranger can load, so the sanitizer is now load-bearing for more than it was.
  • Expiry fails closed — an unparseable expires_at reads as expired.
  • Rename/purge — agent_canvas_shares registered in AGENT_REFS, pinned by test_share_rows_follow_their_agent_through_rename_and_purge.

Informational

[I1] audience is not consulted when minting a share. (Confidence: 7/10) Independent of C1, an owner can publicly share an operator-audience canvas. Probably intended — the owner can read it anyway, and a share is a human override of the agent's declaration — but ent#438 documents audience as the fail-closed gate, and nothing in this diff records that a public share deliberately outranks it. One sentence in agent-canvas.md would settle it.

[I2] A view is a DB write on an unauthenticated route. (Confidence: 6/10) record_canvas_share_view fires per successful resolve. Bounded by check_public_link_rate_limit per IP, so this is a note rather than a finding — but it is the first write-on-read the public token surface has.

/validate-pr — Lane C+schema

Lane C on routers/public.py + dependencies.py — genuinely, unlike #2619 where it was only the compose files.

Category Status Notes
Base branch ✅ feature/553-canvas-lifecycle — correct for a stacked PR; retargets to dev when #2619 lands
Closing keyword ⚠️ body says "Implements abilityai/trinity-enterprise#554" — not a closing keyword. It is cross-tracker, so the automation would not promote it anyway, but rephrasing to Fixes abilityai/trinity-enterprise#554 keeps the convention and makes the manual status-in-dev bump obvious
Dual-track migration ✅ SQLite agent_canvas_shares + Alembic 0060_agent_canvas_shares
Alembic single head ✅ 61 revisions, 1 head (0060), chained off 0059 — no fork
Security greps ✅ clean
Docs ✅ feature-flows/agent-canvas.md (+68), user-docs/agents/agent-canvas.md (+23)
Merge gate ✅ all checks green
Test adequacy ⚠️ 23 tests, strong on scope/expiry/revocation/enumeration — but no agent-principal test on any of the three share routes, which is how C1 got through
ui label ⚠️ adds SharedCanvas.vue, CanvasDocument.vue and +146 lines of CanvasPanel.vue; no label, so frontend-e2e did not exercise the new public page

🤖 Generated with Claude Code · https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ

dolho and others added 3 commits September 14, 2026 11:01
…nt#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
…on (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
…ong route (ent#554 review)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ
@dolho

dolho commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Re-review — [C1] fixed

Pushed to this branch: 1005bd2d9 (the fix), f5a39e1a4 (the # mcp: header), 6d77d988f (learnings). Recommendation moves REQUEST CHANGES → APPROVE.

What changed

The predicate was never wrong — _gate_pin has had the right rule since ent#553. What shipped wrong was three routes reaching for the neighbouring gate, whose name describes a verb (_gate_human_removal) rather than a rule. So the fix renames the rule instead of patching call sites:

_gate_human_only(user, name, *, agent_detail)     # owner-or-admin, no agent principal at all
├── _gate_pin    → "A pin is a human's ordering; an agent may not pin a canvas"
└── _gate_share  → "A share link decides who outside the platform may read this canvas…"

agent_detail is per-caller because an agent reads the refusal to decide what to do next, and "may not pin" is unhelpful on a share.

Final gate map — the three share routes moved, the delete routes deliberately did not:

route gate agent key
create_canvas_share _gate_share ❌
list_canvas_shares _gate_share ❌ — it returns the token, which is the capability
revoke_canvas_share _gate_share ❌ — no legitimate way to create a link means none to turn off a person's
pin_canvas _gate_pin ❌ unchanged
clear_canvas _gate_human_removal ✅ unchanged
bulk_delete_canvases _gate_human_removal ✅ unchanged
write_canvas / patch_canvas _gate_write ✅ unchanged

Verification — the original probes, inverted

The four probes that confirmed the finding now assert the opposite, and pass:

probe before after
create_canvas_share source contains _gate_human_removal( ✅ present ❌ absent, _gate_share( instead
_gate_share(User(agent_name="agent-a"), "agent-a") (gate did not exist) raises HTTPException(403)
the docstring's human-only claim matches the code contradicted matches
scope="public" reachable by an agent yes refused at the gate

Six regression tests added, and four of them fail against the parent commit — verified by reverting routers/canvas.py, running, and restoring:

FAILED test_an_agent_key_may_not_mint_a_share_link
FAILED test_all_three_share_routes_use_the_human_only_gate
FAILED test_listing_shares_is_gated_because_it_returns_the_token
FAILED test_the_human_only_gate_names_its_rule_not_a_verb

The other two pass both ways by design — they are the over-correction guards, and they earned themselves immediately. My first attempt used one str.replace on a body shape that occurs twice in this file, and silently swept clear_canvas into the human-only gate. That would have broken a real MCP tool for every agent — a security fix breeding the next bug, the /review §4.14 class — so test_deleting_a_canvas_is_deliberately_still_agent_callable now asserts the boundary in the other direction. (test_ent438_agent_canvas.py:294 would have caught it too.)

Re-review of the fix itself

Checked for the adjacent-sibling class, since that is how the original defect arrived:

  • Every remaining _gate_human_removal call site is a delete verb (bulk_delete_canvases, clear_canvas) — nothing grant-shaped is left on the permissive gate.
  • No share route exists on the portal prefix, so the Workspace side needs no equivalent change; _require_canvas_manager there is already platform-only + owner.
  • No frontend change needed — the share control is gated on canManage, which derives from can_manage_canvases (a human owner/admin check). The fix narrows only the REST surface, which is the point: agents have no UI, so nothing that could previously reach the control has lost it.

Tests

suite result
-k canvas across tests/unit 291 passed, 2 skipped
test_1310_auth_wiring + test_1310_auth_consolidation + test_293_admin_gate_rejects_agent_keys + test_186_enumeration_uniformity + test_models_centralized 116 passed
CI on this branch 11/11 pass, 0 fail

Docs

  • user doc — "Only the agent's owner (or an admin) can delete, pin or share", plus a bullet stating that an agent can mark a canvas for its roster but cannot create, list or revoke a share link.
  • flow doc — records the decision, the blast radius, why list_canvas_shares is gated identically, and why the delete routes stay permissive.
  • # mcp: header — the three share routes are now listed as deliberately not exposed. Worth more than a comment here: the ent#553 entry already stated the rule the share routes then failed to follow, so a reader consulting this header for a fourth route's gate would have found the reasoning but not the precedent.
  • docs/memory/learnings.md — the class is now confirmed twice in two PRs, so it is written down: a gate named after a verb gets reached for by a route that shares the verb but not the rule. It also captures the coverage half — all 23 existing tests would have passed against the vulnerable code, because none constructed a User(agent_name=…).

Still open from the first pass (unchanged, non-blocking)

  • [I1] audience is not consulted when minting a share. Now recorded in the flow doc as part of the blast-radius paragraph, but the human-sharer case is still undecided-in-writing. One sentence would settle it.
  • [I2] a view is a DB write on an unauthenticated route — bounded by the per-IP limit; still a note.
  • ⚠️ no closing keyword — Implements abilityai/trinity-enterprise#554. Rephrase to Fixes … for the convention; cross-tracker, so status-in-dev needs a manual bump either way.
  • ⚠️ no ui label — frontend-e2e still has not exercised the new public share page.

🤖 Generated with Claude Code · https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ

#2619 (ent#553) squash-landed on dev, so the stacked base is gone. Conflicts
were the parent-squash echo in nine canvas files (branch already carried
#2619's final tree, 46f4928) plus two docs files dev had moved on:
learnings.md keeps both the 2026-09-13 and the ent#554 entries;
user-docs/agent-canvas.md takes the #2746 sync and re-adds the share/PDF
section and the share bullets. Tree vs dev is exactly the ent#554 delta
(27 files, +2178/-20).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LAFj2RWjsQWPgnq1vNT9Xm
@vybe
vybe changed the base branch from feature/553-canvas-lifecycle to dev September 14, 2026 10:14
@vybe vybe added the ui PR touches the frontend UI — triggers Playwright e2e tests label Sep 14, 2026

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Validated (/validate-pr, Lane C+schema): auth boundary reviewed (get_optional_user delegates to get_current_user; share routes human-only via _gate_share), dual-track migration present, single Alembic head, no secrets/emails/IPs. Merged dev in (parent-squash echo resolved; tree vs dev is exactly the ent#554 delta), retargeted to dev; full pytest matrix, CodeQL, e2e green.

@vybe
vybe merged commit bf7ab51 into dev Sep 14, 2026
32 of 33 checks passed
vybe added a commit that referenced this pull request Sep 14, 2026
#2619 (ent#553) and #2623 (ent#554) both squash-landed on dev, so the stacked
base is gone. Conflicts were the parent-squash echo in the seven files this
branch touches on top of them (branch already carried both parents' final
trees) plus two docs files this branch never edits, taken from dev. The
enterprise submodule pointer is repinned to dev's commit — the branch carried
an older one from before the ent#190 bump. Tree vs dev is exactly the ent#555
delta (22 files, +685/-25).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LAFj2RWjsQWPgnq1vNT9Xm
vybe pushed a commit that referenced this pull request Sep 14, 2026
…ss verdict (#2734) (#2779)

* docs(canvas): the canvas header states two facts and derives no verdict (#2734)

Trinity Rule #1 — the requirements delta lands before the code.

FR-5 stops describing a derived staleness mark and describes two
unconditional facts instead: when the canvas was written, and when the
agent last finished a run. The age-threshold rejection is kept verbatim
and extended one step — the verdict it replaced could not know what a
canvas is for either, and it fired on the writing run's own output,
because a run completes after it writes and the stamp that would exclude
it (`updated_by_execution_id`) is optional and absent on most live
canvases. That is why "Updated just now" and "may be out of date" were
rendered together.

Also records the two properties a reader of the code would otherwise have
to rediscover: the second fact is OMITTED, never narrated, because the
field is null both for "never ran" and for a failed read and the payload
cannot tell them apart; and `stale` stays computed and unrendered, kept
so the derivation is recoverable rather than because it is endorsed.

The feature flow section is retitled and rewritten with a Change Log row,
the observability bullet follows it, and the user doc carries the new
user-facing sentence.

Refs #2734

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv

* fix(canvas): the canvas payload carries when the agent last ran (#2734)

`decorate` already reads `last_completed_execution_at` once per agent to
derive the staleness verdict. It now also carries that instant on the
payload as `agent_last_run_at`, so the header can state the fact instead
of compressing it into a conclusion.

Three properties are load-bearing:

* The field is DECLARED on `CanvasSummary`, not merely set on the dict.
  Every canvas read route declares `response_model=`, and FastAPI filters
  a dict through it, dropping an undeclared key silently — while the
  voice panel route (no `response_model`) and the portal payload (plain
  dicts) would have kept it. Omitting the declaration would have shipped
  the fact on two surfaces out of three and looked like a frontend bug.
  A `Field(description=...)` rather than a comment, so `model_fields` can
  be asserted and the text reaches OpenAPI and the MCP tool schema.

* The read is normalised beside the read, not in `db/canvas.py`.
  `MAX(completed_at)` is a raw boundary #1474 never covered; a naive
  stored row used to fail quiet in a lexicographic compare, and rendered
  it would be parsed as LOCAL time by the browser. `is_stale` keeps
  receiving the RAW value — it is retired in place and its comparison is
  not this change's to alter.

* Absence is omission, never narration. The field is null both when the
  agent has never finished a run and when the read failed, and the
  payload cannot tell those apart — so it asserts neither. The failure is
  logged, so an operator sees what a reader cannot.

`empty_canvas` declares the key too: it bypasses `decorate`, and two
constructors of one response shape drift unless something pins them.

Tests: 8 new, all shown failing on base (`KeyError: 'agent_last_run_at'`,
`8 failed, 54 passed`) before the source change. The round trip starts
from `decorate`'s own output, so a misspelled dict key cannot pass by
being copied into the test.

Refs #2734

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv

* fix(canvas): the canvas header renders two facts instead of a staleness verdict (#2734)

The header said "Updated just now" and "may be out of date" at the same
time. The two were not in conflict by accident: the verdict was derived
from the same timestamp it contradicted, and it fired on the writing run's
own output, because a run completes after it writes and the stamp that
would exclude it (`updated_by_execution_id`) is optional and absent on
most live canvases.

So the verdict is gone and two neutral facts take its place:

    Updated 2h ago · agent last ran 40m ago

Both are unconditional, both come from `freshness()`, and both are
measured against the SAME injected clock — so whatever is wrong with that
instant is wrong for both by the same amount in the same direction, and
their relationship, the only thing a reader is judging, cannot invert. The
warning pill, its title and the note paragraph are deleted; the header now
carries no tooltip and nothing that appears and disappears as a derived
value flips.

Four details that are decisions, not incidentals:

* The second fact is OMITTED when absent, never narrated. The field is
  null both for "never ran" and for a failed server read, and the payload
  cannot tell them apart — so the gate is `Date.parse`, not truthiness,
  because this module's `relativeTime` answers "at an unknown time" for a
  bad value and that is a narrated non-fact.

* `basis-full` puts the line on its own row. The sibling h3 is
  `flex: 1 1 0%`, so it contributes basis 0 to line-breaking: inline, the
  span never wraps and the TITLE truncates instead — from ~35 characters
  to ~16 at 400px, on every canvas. It also pre-resolves the collision
  with the header buttons arriving in #2623.

* A 60s tick drives `now`. Agent Detail does not poll, so a `computed`
  with no time dependency would keep saying "agent last ran just now" for
  hours — a liveness claim, not a provenance one. The tick refreshes the
  string, not the payload, so it can only make the agent look less
  recently active than it is.

* `canvasChanged` compares the run time too. It is a property of the
  world, not of the loaded object, so comparing `updated_at` alone
  discarded every poll carrying only a fresher run time — on the one
  surface that polls every ~3s.

`stale` is still computed and still ships; nothing renders it. The MCP
read descriptions now say so, since they were the only text that ever
explained the field to an agent.

Tests: 8 new, all shown failing on base (`8 failed | 139 passed`) before
the source change. Baseline: `CanvasPanel.vue` raw_gray 25 → 21, the four
gray classes on the deleted note — the one entry edited in place, so no
unrelated drift is absorbed with it.

Closes #2734

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv

* merge-train: strip agent_last_run_at from the share-link payload (#2779) — mechanical, per the merge-train note on the PR

The body says the share link was NOT widened; the wire payload was. The
share payload now drops the field, pinned by a test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KtVjZEsxjyz2E4x5XdQg99

---------

Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: sim <sim@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants