Skip to content

fix(system-agent): trinity-system adopts a rebuilt base image at the cold boundary (#1816) - #1867

Merged
vybe merged 15 commits into
devfrom
AndriiPasternak31/issue-1816
Jul 29, 2026
Merged

fix(system-agent): trinity-system adopts a rebuilt base image at the cold boundary (#1816)#1867
vybe merged 15 commits into
devfrom
AndriiPasternak31/issue-1816

Conversation

@AndriiPasternak31

@AndriiPasternak31 AndriiPasternak31 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #1816

system_agent_service.ensure_deployed() returned action: none the instant the trinity-system
container reported runningno drift predicate was evaluated at all. Combined with
restart_policy: unless-stopped and a canonical upgrade path (build-base-image.shstart.sh) that
never touches agent containers, that made the platform orchestrator the most-stale agent in every
fleet
, indefinitely and silently.

This closes the system-agent half that #1809 explicitly left out of scope: #1809 fixed image
adoption for the per-agent start path (start_agent_internal, cold-start-gated ninth predicate) and
named the trinity-system bootstrap as not-covered. Last of the three v0.8.5 upgrade holes (#1814
platform images, #1809 agent images, this).

The part that wasn't obvious

recreate_container_with_updated_config resolves the image from the old container's own
Config.Image reference — the tag trinity-agent-base:latest, not the pinned id. So every
config-drift recreate is also an image adoption
, and AC2 ("a running trinity-system is never
image-recreated mid-operation") cannot be satisfied by gating the image predicate alone.

Two of the eight config predicates were permanently false for a freshly created system agent:

Predicate Why
check_agent_auth_token_env_matches (#1159) _create_system_agent's env dict never wrote TRINITY_AGENT_AUTH_TOKEN — the only writers were crud.py and the two lifecycle recreates
check_full_capabilities_match the container runs cap_add=FULL_CAPABILITIES but carried no trinity.full-capabilities label, and a missing label reads as false against a fleet default of true

So the first POST /api/agents/trinity-system/start after any fresh provision replaced a running
orchestrator and swapped its image mid-operation. Convergent — a recreate writes both values, so the
second start is clean — which is why it survived this long.

What ships

1. Convergence (_create_system_agent) — writes derive_agent_token(SYSTEM_AGENT_NAME) and pins
trinity.full-capabilities: 'true', so both predicates go quiet.

2. Cold-boundary adoption (ensure_deployed):

Boundary Behaviour
running read-only — reports base_image_state (current/stale/unknown), WARNs naming the remedy, raises an operator alarm on stale only. No recreate call exists in this branch (source-pinned between named anchors).
stopped delegates to start_agent_internal instead of a bare container_start — the cold boundary where the #1809 image gate fires. This is where adoption happens.
absent unchanged

3. AC2 as a structural gatestart_agent_internal skips the whole needs_recreation block
for a running trinity-system and returns recreate_deferred: "system_agent_running". Deliberately
independent of predicate count: a tenth predicate added next quarter cannot reopen the hole.

4. Two rebuild fences (review + CSO pass, ADOPT-006/007):

Also fixed along the way: unless-stopped silently vanished on every agent recreate — the old
container's HostConfig was extracted and never read. trinity-system is created with it, so one
recreate downgraded the orchestrator to "stays down after a crash or host reboot".

Deliberate decisions worth reviewing

  • check_full_capabilities_match is system-aware. Pinning the label alone would, on any install
    with agent_full_capabilities=false, produce a mismatch that can never converge — a recreate on
    every start, forever. The predicate (checker) and the recreate's new full_capabilities override
    (writer) both route through one helpers.is_system_agent_name(). That helper is deliberately a
    name test rather than db.is_system_agent: it must be unfailable (a DB error that flips the
    answer would either recreate the orchestrator or leak full capabilities) and it must not widen the
    exemption to any is_system-flagged row.
  • check_base_image_matches split into a 3-state core + an unchanged boolean wrapper. The alarm
    cannot be built on a boolean whose True means both "the image is current" and "the check could not
    run" — alarming on that recreates the bug: a rebuilt agent base image is never picked up — no image-drift predicate, so the v0.8.5 "stop/start-with-recreate" upgrade step is a no-op #1809 symptom one layer up. unknown never alarms.
    bug: a rebuilt agent base image is never picked up — no image-drift predicate, so the v0.8.5 "stop/start-with-recreate" upgrade step is a no-op #1809's consumer is byte-identical (state != "drift").
  • No TRINITY_BACKEND_URL for the system agent. It gates the agent-side heartbeat loop, and
    authorize_heartbeat accepts only scope == "agent" keys — the system agent's is scope == "system".
    This PR makes the recreate a routine path for it, so arming it would newly create a permanent 5s
    403 loop (~17k backend log lines/day). Whether the orchestrator should be visible to fleet health
    is a real question, and a separate one.
  • /reinitialize deliberately does not adopt — already an explicit stop, zero incremental AC
    coverage, and its four-site handle rebinding was the highest-risk edit considered. Pinned by a test.

Consequences operators must know

  1. Adoption destroys the container writable layer. Everything installed outside /home/developer
    apt packages, system pip, /usr/local, /etc — is gone. trinity-system is the one agent that runs
    FULL_CAPABILITIES explicitly for package installation, so this is the most consequential side
    effect of the feature. (/var/lib/trinity/oauth-token is wiped too; harmless — trinity-system uses
    ANTHROPIC_API_KEY.)
  2. Adoption re-reads ANTHROPIC_API_KEY from get_anthropic_api_key() and rewrites trinity.cpu /
    trinity.memory from db.get_resource_limits — creation reads them from template.yaml, so the two
    can legitimately disagree.
  3. On the canonical upgrade path, adoption is operator-triggered (alarm → Restart), not automatic.
    The system agent is running after every canonical upgrade, and that is the branch that must not act.
  4. An admin who changes a running system agent's config and clicks Start now gets no recreate. A
    deliberate divergence from regular-agent semantics; the response names the remedy
    (recreate_deferred).

Verification

Manual, on a live stack — /verify-local --keep Stage 5 (agent-exercise), the blocking check

Mocks cannot model tag-vs-id pinning, which is this bug class — #1809 shipped unit-only and left
exactly this hole. So the adoption was driven against a real trinity-system container on the
Stage-5 stack, rebuilding the real base image mid-flight: 27/27 assertions, 0 failures.

Step Result
creation converges (on a REAL container) TRINITY_AGENT_AUTH_TOKEN baked · trinity.full-capabilities=true · restart_policy=unless-stopped
rebuild moves :latest d9784ee75bfc → d64c89b684b7; the running container stays pinned to the old id
AC2 — running + drift, restart the backend container id unchanged; still on the old image; WARNING + alarm logged
AC1 — the alarm base-image-stale-* operator-queue row raised; content carries no image ids/digests
/api/system-agent/status base_image_state: "stale", enum-only
D1.2POST /agents/trinity-system/start while running id unchanged; response carries recreate_deferred
AC1 — stop it, restart the backend container REPLACED 12722a8dce22 → 46b86ff9e7e3, adopted d64c89b684b7
what survived the replacement restart policy · cap_add (9) · cap_drop ALL · ssh port 2227 · TRINITY_MCP_API_KEY · /template + /home/developer mounts · the workspace volume · the capabilities label · the auth token
no 403 heartbeat loop no TRINITY_BACKEND_URL on the replacement
convergence a second boot does not recreate again; /statuscurrent

Backend log from the AC2 step, verbatim:

Base image drift for trinity-system: sha256:d9784ee75bfc… -> sha256:d64c89b684b7… (reference 'trinity-agent-base:latest')
System agent is running a stale base image. It will NOT be recreated while running (#1816 AC2).
  Adopt it with POST /api/system-agent/restart, or stop it and let the next backend boot pick it up.
System agent: none - System agent already running, but on a STALE base image — restart it … to adopt

Automated

  • /verify-local --keep: PASS — preflight · agent-precheck · build+import-smoke ·
    agent-build+import-smoke · boot+health · Stage 5 agent-exercise · integration, all green.
  • Unit: 5154 passed, 1 failed (full tests/unit, alphabetical) — the 1 is the pre-existing
    failure below. 69 tests are new.
  • Re-run CI-style (cd tests && pytest unit/ -m "not slow" -p randomly --randomly-seed=…) on two
    local seeds: 12345 → 5154 passed / 1 failed (the pre-existing one); 777 → 5145 passed /
    1 failed / 9 errors. The 9 are all test_subscription_auto_switch_pingpong.py and are not mine
    — proven by re-running the same seed with only the test-order fix below reverted (identical item
    set ⇒ identical shuffle, so this is a controlled comparison rather than a cross-tree seed match,
    which is not comparable): byte-identical result, 9 errors + 1 failure either way.
  • tests/lint_sys_modules.py: 203 violations in 60 files, baseline allows 240 — no new violations.
    Pyflakes on every touched file: no new findings.
  • Pre-existing failure, proven not mine (identical on pristine origin/dev):
    test_1069_voip_call_path_param — the verify-venv fastapi get_flat_dependant drift.

One real defect the validation pass caught, and fixed

test_1816_system_agent_adoption.py passed alone and in the full alphabetical run, but 21 of its 67
tests failed
when collected alongside tests/unit/test_start_agent_skip_inject.py. Five sibling unit
files replace services.agent_service / .helpers with Mock objects in sys.modules at
collection time (the grandfathered #762 class), and this file resolves the real modules lazily,
inside fixtures
— i.e. after the contamination. The leak is silent rather than loud:
is_system_agent_name becomes "no agent is the system agent", so the AC2 gate never fires and the
recreate it exists to suppress happens
, while every assertion still looks meaningful. Fixed by
importing the real modules at collection time (this file sorts first) and pinning them per-test via
monkeypatch.setitem — self-restoring, so the siblings' own harnesses are untouched. Reproduction now
green: 21 failed → 0; the original 5-file set 21 failed → 108 passed.

Worth noting for the reviewer: this would not reliably have been caught downstream. It is invisible
to an alphabetical full-suite run, and at seed 777 the adoption suite happened to land in a lucky order
and passed even unfixed — so the 3-seed CI matrix catches it only probabilistically.

Known, documented, not fixed here

ensure_deployed runs once per worker lifespan and the staleness-alarm cooldown is per-process, so a
stale boot files one queue item per worker (--workers 2 ⇒ 2 — visible in the run above).
Deliberate: a cross-worker cursor puts Redis or a DB read on the boot path for an advisory alarm, and
the un-guessable timestamped id that stops an agent pre-creating and silencing it (#1632) is inherently
undedupable by on_conflict_do_nothing. Said plainly in both the code and the flow doc.

Not in scope

Deferred Why
#1817 per-agent start lock Separate issue, separate PR. The boot double-provision race is pre-existing and not widened here — ADOPT-006 fences its worst consequence.
System-scoped MCP key hygiene get_agent_container returns None on any Docker error, so deactivate-before-run would turn a transient socket hiccup into an outage of the privileged orchestrator.
System-agent heartbeat authorize_heartbeat rejects scope='system'; a design question, not this bug.
Rewiring _create_system_agent onto crud.py Follow-up refactor.

Docs

requirements/infrastructure.md §8.5b (ADOPT-001…007, the new home for #1809's semantics too),
architecture.md (system_agent_service entry, the #1560 lifecycle-clearing paragraph, Invariant #8
"Role ≠ human"), feature-flows/internal-system-agent.md (new "Base-image adoption" section),
feature-flows/agent-lifecycle.md, feature-flows.md index row, plus learnings.md.


Reviewers: @webmixgamer (wrote #1809's predicate — please sign off on the 3-state extraction and the
system-aware check_full_capabilities_match), @dolho, @vybe.

AndriiPasternak31 and others added 14 commits July 28, 2026 19:15
…1816)

Rule #1 — documentation before implementation.

- architecture.md: #1560 lifecycle-clearing wording ("never evaluated" →
  "never acted on"), the 3-state split, and the structural AC2 gate; the
  system_agent_service catalog entry.
- feature-flows/internal-system-agent.md: startup diagram gains the drift
  branch, plus a Base-image adoption section carrying the convergence
  invariant, the three boundaries, the AC2 gate and the consequences
  (writable-layer loss, no TRINITY_BACKEND_URL, operator-triggered on the
  canonical upgrade path).
- feature-flows/agent-lifecycle.md: 3-state core + boolean wrapper,
  system-aware capabilities predicate + recreate override, restart-policy
  carry-forward.
- requirements/infrastructure.md: new 8.5b ADOPT-001..005.
- feature-flows.md: Recent Updates row.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dicate (#1816)

T1 — `check_base_image_matches` split into a 3-state `check_base_image_state`
core (`match` | `drift` | `unknown`) plus an unchanged boolean wrapper
(`state != "drift"`). Every WARNING is kept verbatim and the recreate path
still consumes only the boolean, so #1809's behaviour is byte-identical:
`unknown` and `match` both fail open to True. The 3-state exists because the
staleness alarm this issue adds cannot be built on a boolean whose True means
both "the image is current" and "the check could not run" — alarming on that
would recreate the #1809 symptom one layer up.

T2 — `check_full_capabilities_match` is system-aware. `trinity-system` runs
FULL_CAPABILITIES by contract (package installation), not by the fleet
default this predicate compares against, so pinning its
`trinity.full-capabilities` label alone would, on any install with
`agent_full_capabilities=false`, produce a mismatch that can never
converge — a recreate on every start, forever.

Both route through one shared `is_system_agent_name()` so the checker, the
recreate override and the AC2 gate can never disagree. Deliberately a NAME
test rather than `db.is_system_agent`: it must be unfailable (a DB error that
flips this answer would either recreate the orchestrator or leak full
capabilities) and it must not widen the exemption to any `is_system`-flagged
row.

Verified: tests/unit/test_1809_image_drift_recreate.py (19) plus
test_start_agent_skip_inject, test_subscription_auto_switch_no_cred_import,
test_inject_assigned_credentials, test_agent_readiness_probe,
test_1560_breaker_cleared_on_lifecycle, test_1811_recovery_container_parity,
test_base_image_allowlist, test_1484_create_agent_characterization — 102 passed.

Refs #1816

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

T3 + T-A. `_create_system_agent` left two of the eight config predicates
PERMANENTLY false:

  * `check_agent_auth_token_env_matches` (#1159) — the env dict never wrote
    TRINITY_AGENT_AUTH_TOKEN; its only writers were crud.py and the two
    lifecycle recreates.
  * `check_full_capabilities_match` — the container runs with
    cap_add=FULL_CAPABILITIES but carried no `trinity.full-capabilities`
    label, and a missing label reads as 'false' against a fleet default of
    true.

That is not cosmetic. `recreate_container_with_updated_config` resolves the
image from the container's own Config.Image *tag*, so every config recreate
is also an image adoption — a permanently-false predicate means the first
`POST /api/agents/trinity-system/start` after any fresh provision replaces a
RUNNING orchestrator and swaps its image mid-operation, which is precisely
what AC2 forbids. (Convergent: a recreate writes both values, so only the
first start was affected — which is why this survived so long.)

The token derive is fail-closed on an unset AGENT_AUTH_SECRET, accepted
deliberately: the install now fails to CREATE the system agent rather than
creating one the backend can never talk to. ensure_deployed catches →
`create_failed`, and main.py's lifespan catch keeps boot alive.

Deliberately NOT added: TRINITY_BACKEND_URL. It gates the agent-side
heartbeat loop, and authorize_heartbeat accepts only scope='agent' keys —
the system agent's is scope='system', so arming it is a permanent 5s 403
loop. Pinned by an AST guard (key-level, not substring — the code documents
the omission and a text search would fire on its own rationale).

tests/unit/test_1816_system_agent_convergence.py drives `_create_system_agent`
for real and builds the fixture from the `environment`/`labels` kwargs it
actually passes to containers_run — a hand-built fake carrying both values
would assert only that a correct container is correct. Red before this commit
on ['agent_auth_token'] + both pins; 11 passed after.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ate (#1816)

T4 — three lifecycle fixes the adoption path depends on:

* **Restart policy is carried onto the replacement.** `old_host_config` was
  extracted at the top of `recreate_container_with_updated_config` and then
  never read, so `unless-stopped` silently vanished from EVERY recreated
  agent. `trinity-system` is created with it, so one recreate downgraded the
  platform orchestrator to "stays down after a crash or host reboot".
  `_provision_folders_and_run_agent_container` takes a keyword-only
  `restart_policy` and forwards it only when it names a policy, so every
  pre-#1816 caller is byte-identical. Read null-safely — the key can exist
  with a null value and `.get` on None would abort the recreate after the old
  container is already gone.
* **`full_capabilities` override.** `None` (every existing caller) resolves to
  the fleet default for a regular agent and unconditionally True for
  trinity-system, via the same `is_system_agent_name` the predicate exempts
  on — writer and checker cannot disagree.
* **No `TRINITY_BACKEND_URL` for the system agent.** It gates the agent-side
  heartbeat loop and `authorize_heartbeat` accepts only scope='agent' keys;
  the system agent's is scope='system'. #1816 makes this recreate a routine
  path for it, so arming it would newly create a permanent 5s 403 loop.

T5 (AC2) — `start_agent_internal` gains the structural gate: a RUNNING
trinity-system is never recreated, and the caller is told
`recreate_deferred="system_agent_running"` rather than left to infer it. The
gate covers the WHOLE `needs_recreation` block, not just the image predicate:
the recreate resolves the image from a tag, so any predicate that fires is
also an image adoption — gating one would leave AC2 open through the other
eight. Surfaced through `routers/agents.py`'s whitelisted response dict and
its audit details (a field added to the internal dict alone dies at the
router — #1809's own learning).

Two harness stubs updated: a bare `Mock()` auto-creates `is_system_agent_name`
returning a truthy Mock, which reads as "every agent is the system agent" and
silently suppressed every recreate in test_start_agent_skip_inject.

Verified per-file (all green): test_start_agent_skip_inject 9,
test_1809_image_drift_recreate 19, test_1816_system_agent_convergence 11,
test_agent_readiness_probe 5, test_inject_assigned_credentials 8,
test_subscription_auto_switch_no_cred_import 1, plus the 12 other
helpers-stubbing files (91 tests). A cross-file sys.modules ordering flake in
TestCheckBaseImageMatches (9 tests) reproduces IDENTICALLY on pristine
origin/dev @64845458 — pre-existing, not a regression.

Refs #1816

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

The core of the issue. `ensure_deployed` returned `action: none` the instant
the container reported `running`, without evaluating a single drift predicate.
Combined with `restart_policy: unless-stopped` and a canonical upgrade path
(build-base-image.sh → start.sh) that never touches agent containers, that
made the platform orchestrator the most-stale agent in every fleet —
indefinitely, and silently.

T6 — three boundaries, honestly separated:

  * RUNNING → READ-ONLY. Reports `base_image_state`
    (`current`|`stale`|`unknown` — an enum only, never image ids, mirroring
    the /health clone_status contract #1439), WARNs naming the remedy, and
    never recreates. Source-pinned by slicing the branch between two named
    anchors, so a refactor that merely relocates a recreate call cannot pass.
  * STOPPED → delegates to `start_agent_internal` instead of a bare
    `container_start`. This is the cold boundary where the #1809 image gate
    fires, and it inherits #1560 clear-before-recreate ordering, the
    409/NotFound race hardening, the post-recreate handle re-lookup and every
    future predicate — rather than forking a second lifecycle for one agent,
    which is the bug class that produced this issue.
  * no container → unchanged.

Every early return sets both `action` and `message`: main.py's lifespan
indexes them directly, so an omission would raise KeyError inside the boot
log line.

T7 — AC1 on the canonical upgrade path. Since the system agent is RUNNING
after every canonical upgrade, the read-only branch is the one that fires,
so detection without notification would tell no one. An edge-triggered
operator-queue alarm follows the sync_failing/git_bloat idiom (reserved id
prefix registered in #1632's anti-spoof guard, priority high, 6h cooldown,
emit-failure-safe) and is raised on `stale` ONLY — a fail-open probe must
never manufacture an alert, which is exactly why the 3-state split exists.

R1 pre-flight: a recreate REMOVES the old container before running the
replacement, so a run failure leaves the platform with no orchestrator. When
the agent network is missing or the ssh port is bound, the adoption is
declined and a plain start runs instead (costing one stale boot — the
pre-#1816 status quo — rather than the orchestrator). Fail-open on an
unreadable probe; a start failure raises a critical alarm.

T8 — /restart delegates (an explicit stop makes it a cold start, so it is the
operator's remedy for the alarm) and re-fetches the container for its
response, because a recreate replaces the handle. /status gains the 3-state
`base_image_state`, reported only while running. /reinitialize deliberately
unchanged, pinned by a test.

T10 — 49 behavioural cases + the source pins, and the test_1560 create-path
pin repaired: `str.index` is first-occurrence, so it stayed green while
silently ceasing to pin `_create_system_agent` the moment anything above it
grew a `clear_agent_breakers(SYSTEM_AGENT_NAME)` call. Proven by simulation —
the old assertion PASSES with the create-path clear removed, the repaired one
fails with "the clear must live INSIDE _create_system_agent".

Verified: test_1816_system_agent_adoption 49, test_1816_system_agent_convergence
11, test_1560_breaker_cleared_on_lifecycle 16, test_1632_operator_queue_caps 49
— 118 passed. tests/lint_sys_modules.py: no new violations.

Refs #1816

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

The docs written ahead of implementation used `is_system_agent()`, which
collides conceptually with the existing DB-backed `db.is_system_agent`. Names
the real helper and states why it is a name test rather than the DB one.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#1816)

/update-tests + /sync-feature-flows tail steps.

- .claude/agents/test-runner.md: both new unit files catalogued under
  Operations & Observability, a dated Recent Test Additions block (including
  the test_1560 pin repair and the two harness-stub fixes), and the totals.
- feature-flows/async-docker-operations.md: the new `network_get` wrapper.
- feature-flows/operating-room.md: `base-image-stale-` registered in the
  reserved-prefix enumeration (all 3 sites) and named in the platform-create
  exemption list.

agent-lifecycle.md and internal-system-agent.md were already synced in the
docs-first commit; feature-flows.md index is 430 lines.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…plicate gate (#1816)

Self-review polish, no behaviour change:

- `_BASE_IMAGE_STATE_LABELS` is consumed by `routers/system_agent.py`, so a
  leading underscore was wrong — renamed `BASE_IMAGE_STATE_LABELS`.
- `get_system_agent_status` had two consecutive `if status == "running":`
  blocks; folded the health fetch into the first. It keeps its own
  try/except, so `base_image_state` is still set when the agent is
  unreachable.

Verified: test_1816_system_agent_adoption + test_1816_system_agent_convergence
— 60 passed.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ht (#1816)

The stopped branch already holds the handle; re-fetching it inside
`_preflight_ok_for_delegated_start` was a wasted Docker round-trip on the boot
path and a needless TOCTOU window. Still null-safe throughout, so a partially
populated handle degrades to "no port to check" rather than raising.

Verified: 60 passed (adoption + convergence).

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Caught by verify-local, not by any local run: `_create_system_agent` resolves
its template through a CWD-RELATIVE fallback (`./config/agent-templates`, used
whenever `/agent-configs/templates` is absent, i.e. off-container), and
verify-local runs pytest from `tests/`. From there creation died on a missing
template BEFORE reaching the token derive, so
`test_creation_without_agent_auth_secret_fails_closed_without_blocking_boot`
was asserting the wrong failure — green from the repo root, red from `tests/`.

Both suites now pin the CWD to the repo root with the reason stated, so they
hold wherever pytest is invoked from. Verified from BOTH cwds: 60 passed each.

A test bug, not a code bug — the behaviour under test is unchanged.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ensure_deployed` runs once per worker lifespan and the cooldown cursor is
per-process, so a stale boot files one operator-queue item per worker
(`--workers 2` ⇒ 2). Deliberate rather than overlooked — a cross-worker cursor
puts Redis or a DB read on the boot path for an advisory alarm, and the
un-guessable timestamped id (what stops an agent pre-creating and silencing it,
per #1632) is inherently undedupable by `on_conflict_do_nothing`. Said plainly
in both the code and the flow doc rather than left for a reviewer to discover.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#1816)

Review + CSO follow-ups on the base-image adoption work.

- recreate_missing_container refuses trinity-system (409). start_agent_internal
  falls through to it when the container lookup returns None, and #1816 newly
  reaches that from the boot path and /restart — ensure_deployed runs in every
  uvicorn worker with no leader lock, so a concurrent recreate can null the
  lookup mid-flight. That path reconstructs a REGULAR agent: it deactivates the
  system-scoped MCP key and mints an agent-scoped one (plaintext unrecoverable,
  so the orchestrator irreversibly loses its permission bypass), drops
  trinity.is-system, the /template bind and unless-stopped, and arms the
  scope-403 TRINITY_BACKEND_URL. ensure_deployed's create branch rebuilds it
  correctly on the next boot; the race itself is #1817.

- /restart and /reinitialize are human-only. assert_admin rejects connector
  principals but not agent ones, and get_current_user hands an agent-scoped key
  its owner's role — so on a default admin-owned install any non-ephemeral
  agent's TRINITY_MCP_API_KEY passed it. Tolerable while /restart was a
  stop+start; not once it replaces the container. trinity-ops-agent#232
  precedent; no-op for JWT / user-scoped / system-scoped callers.

- Sanitize the start-failure alarm's interpolated exception string. It lands in
  operator_queue.question — durable, operator-visible state — from a path that
  builds env dicts holding OAuth tokens and PATs. The staleness alarm was built
  to carry no identifiers; this one wasn't.

- Bound that alarm across processes via a bucketed id. A per-process cursor
  cannot bound a failure whose symptom is a fresh process, and retention never
  deletes a pending row. A bucket (not a fixed id) so Clear All → cancelled
  can't wedge it shut through on_conflict_do_nothing; guessability is fine
  because the prefix is in _RESERVED_ID_PREFIXES.

- Cooldown cursor uses time.monotonic() — datetime.utcnow() is deprecated and a
  wall-clock step could skip or extend the gate.

- State the real boot cost in the comment (~20-90s of blocked lifespan, not one
  timeout) and name the multi-worker exposure.

9 tests; 5151 unit tests green.

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

The last commit added two fences but only reached requirements + the feature
flow. architecture.md is the current-design doc and carries the invariant, so
both belong here too.

- system_agent_service entry: recreate_missing_container refuses trinity-system
  (409, ADOPT-006). Worth stating where the service is described, because the
  reader's natural assumption is that the generic recovery rebuild is a valid
  way to bring the orchestrator back — it is not, and the downgrade it causes
  (system-scoped MCP key deactivated for an agent-scoped one) is irreversible.

- Invariant #8 gains "Role ≠ human": assert_admin/require_admin answer what
  role, never is-this-a-human, because get_current_user resolves an
  agent-scoped MCP key to its owner carrying the owner's role. Generalized from
  the two known instances (#1644 retention ack, #1816 /restart + /reinitialize)
  into the rule that produced them — an endpoint whose blast radius is
  operator-scale needs reject_agent_principal in ADDITION to the role gate, and
  the trigger to revisit an existing gate is a change in what the endpoint
  DOES. Escalating a handler's destructiveness silently re-prices every
  principal that could already reach it.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…leaks (#1816)

The adoption suite passed alone and in the full alphabetical run, but 21 of its
67 tests failed when collected alongside tests/unit/test_start_agent_skip_inject.py
— i.e. it was green by luck of import order, and CI randomizes.

Five sibling unit files replace services.agent_service / .helpers with Mock
objects in sys.modules at COLLECTION time (the grandfathered #762 class). This
file resolves the real modules lazily, inside fixtures — after the
contamination. The docstring already noted the file is not a contaminator; not
being one is not the same as being immune to one.

The failure mode is silent rather than loud, which is the reason to fix it
rather than order the files: a leaked helpers Mock makes is_system_agent_name
return False for everything, so the AC2 gate never fires and the recreate it
exists to suppress happens — while every assertion still looks meaningful.
check_base_image_state degrades the same way (a Mock can never return
"unknown", so the never-alarm-on-unknown property becomes untestable).

Fix: import the real modules at collection time (this file sorts before all
five) and pin them per-test with monkeypatch.setitem, which self-restores — so
the siblings' own harnesses, which hold direct module references rather than
sys.modules lookups, are untouched. No new lint_sys_modules violations
(monkeypatch.setitem is the sanctioned form).

Reproduction now green: adoption + skip_inject 21 failed -> 0; the original
5-file set 21 failed -> 108 passed.

Refs #1816

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@AndriiPasternak31
AndriiPasternak31 marked this pull request as ready for review July 28, 2026 23:30
@github-actions

Copy link
Copy Markdown

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

@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 via /validate-pr. Fixes #1816 with a closing keyword. The AC2 boundary is honored (running branch is read-only; adoption only at the cold boundary / explicit restart), the staleness alarm follows the #1632 reserved-prefix + #1644 no-load-bearing-queue-item lessons, is_system_agent_name is correctly a deterministic name test that never widens capabilities, and the restart endpoint gets reject_agent_principal per the ops-agent#232 pattern. Docs complete (architecture + 4 flows + requirements + learnings), named #1816 regression suites present, full pytest matrix + prod-image-smoke green.

…s.md 07-28 entries)

Both docs-only both-sides-added conflicts: keep the #1816 system_agent_service
catalog line alongside the #1656 cornelius line, and all three 2026-07-28
learnings entries (#1816 x2 + #1782).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vybe
vybe merged commit 6a3e6b9 into dev Jul 29, 2026
21 of 22 checks passed
vybe pushed a commit that referenced this pull request Jul 29, 2026
… 07-28 entries)

Both docs-only both-sides-added conflicts vs the just-merged #1816 (#1867):
keep the #1804 and #1816 changelog rows, and all four 2026-07-28 learnings
entries.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants