Skip to content

fix(deploy): auto re-own data volumes to UID 1000 (#958 / #874) - #969

Merged
dolho merged 1 commit into
devfrom
fix/958-deploy-non-root-volume-fixup
May 28, 2026
Merged

fix(deploy): auto re-own data volumes to UID 1000 (#958 / #874)#969
dolho merged 1 commit into
devfrom
fix/958-deploy-non-root-volume-fixup

Conversation

@dolho

@dolho dolho commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Dev deploys have been failing since 2026-05-27 with container ***-backend is unhealthy. Root cause confirmed via docker logs on the dev VM:

sqlite3.OperationalError: attempt to write a readonly database
  File "/app/db/migrations.py", line 2096, in _migrate_null_legacy_schedule_timeouts
    cursor.execute("UPDATE agent_schedules SET timeout_seconds = NULL ...")

After issue #874 the backend and scheduler run as UID 1000 inside the container, but the dev VM's /data bind mount + agent-configs named volume were still owned by root from the prior root-container era. Earlier deploys survived because no migration actually wrote to existing rows — #922's null_legacy_schedule_timeouts was the first UPDATE migration after #874, so it's the one that surfaces the permission gap. SQLite then fails the whole startup, /health returns 503 forever, and the workflow's up -d times out at the unhealthy gate.

docs/migrations/NON_ROOT_CONTAINERS_2026-05.md documents the manual re-own recipe for operators. Nobody ran it on the dev VM.

Fix

Pre-flight in deploy-dev.yml that auto-runs the canonical recipe. Idempotent:

  • Bind mount: stat -c %u "${TRINITY_DATA_PATH:-./trinity-data}"chown -R 1000:1000 only if not already 1000.
  • Named volume ${PROJECT}_agent-configs: probe ownership with an ephemeral alpine container, chown via a second ephemeral container (--user 0) only if not 1000.

Both branches no-op silently on healthy VMs, so it's safe on every deploy.

Why CI rather than a one-shot manual fix

Test plan

  • Merge → deploy-dev workflow runs → pre-flight detects the root-owned data path, re-owns, build proceeds, backend reaches /health healthy.
  • Subsequent deploys: pre-flight is silent (no Re-owning ... log line), unchanged behavior.
  • If the dev VM's TRINITY_DATA_PATH is something other than the compose default, the bind-mount check is a no-op (directory doesn't exist) — script proceeds without error.

Out of scope (follow-up)

When up -d times out for any other reason, the workflow currently produces no logs because set -e aborts before the "Error check" step. Worth a separate small PR to wrap the up + health checks with a trailing docker logs capture so the next incident is one-click triaged. Not bundling here.

🤖 Generated with Claude Code

After #874 the backend and scheduler run as UID 1000 inside the
container, but the dev VM's `/data` bind mount and `agent-configs`
named volume were still owned by root from the prior root-container
era. Earlier deploys survived because no migration *wrote* to existing
rows — the first one that does (#922's `null_legacy_schedule_timeouts`
UPDATE) hits `sqlite3.OperationalError: attempt to write a readonly
database`, /health returns 503 forever, and `up -d` times out with
"container ***-backend is unhealthy".

Confirmed root cause via `docker logs trinity-backend` on the dev VM:
the migration framework crashes mid-run on the SQLite write before
incrementing `schema_migrations`.

Fix: add an idempotent pre-flight to the deploy workflow that checks
ownership and re-owns to UID 1000 only when needed:

- `${TRINITY_DATA_PATH:-./trinity-data}` bind mount: `stat -c %u` then
  `chown -R 1000:1000` if not 1000.
- `${PROJECT}_agent-configs` named volume: same check via ephemeral
  alpine container, chown via second ephemeral container.

Both checks no-op silently on healthy VMs, so this is safe to run on
every deploy. Canonical recipe matches
`docs/migrations/NON_ROOT_CONTAINERS_2026-05.md`.

Related to #958

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dolho
dolho requested a review from AndriiPasternak31 as a code owner May 28, 2026 13:07
@dolho

dolho commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

/review report — fine to merge

Critical findings: 0
Informational findings: 2 (bounded, non-blocking)

Pass 1 (critical / blockers)

  • SQL & data safety — no DB writes
  • Race / concurrency — chown to 1000 when the running container also runs as 1000 is a no-op; prior failed deploys left backend/scheduler stopped, so no in-flight writes to contend with
  • Auth boundary — no new endpoints
  • Credential exposure — log lines emit $DATA_UID (integer) and $TRINITY_DATA_PATH (relative path); no secrets

Pass 2 (informational)

[I1] Custom TRINITY_DATA_PATH from .env would be silently missed. Script reads TRINITY_DATA_PATH="${TRINITY_DATA_PATH:-./trinity-data}" from the SSH shell. SSH session doesn't source .env, so if the dev VM's .env sets a non-default path (e.g. /var/lib/trinity), the shell defaults to ./trinity-data, the [ -d ... ] guard fails, the chown silently skips, and the same migration crash recurs on the actual mounted path. Not a problem for the current dev VM (uses the default), but cheap insurance worth adding: set -a; . .env 2>/dev/null || true; set +a before reading the var, OR parse the path from docker-compose config.

[I2] alpine image is unpinned. docker run --rm … alpine … pulls latest from Docker Hub. The repo's container-security.yml workflow pins SHAs elsewhere; consistency would suggest pinning here too. Low priority — ephemeral container, chown only, bounded blast radius. Pin to alpine:3.20 or a digest if you want to close it.

Scope check

CLEAN. Single file, single concern, scope matches PR title and body exactly.

Other categories — clean

Conditional side effects (guards fail safe — chown fires on unknown UIDs, no-op on 1000) · magic numbers (1000 matches the Dockerfile USER directive; varying would be worse) · dead code · error handling (set -e at line 30 catches chown failures; stat … || echo "0" defaults to "definitely chown" on probe failure — safe default) · frontend (none) · performance (one-shot chown + two ephemeral containers per deploy, negligible after first run) · enum completeness (no enums)

🤖 Generated by /review (Claude Opus 4.7)

@dolho
dolho merged commit d5d8b32 into dev May 28, 2026
13 checks passed
vybe pushed a commit that referenced this pull request May 28, 2026
* fix(deploy-local): write templates to host-mapped path (#950)

`deploy_local_agent` was silently creating empty agents. The backend
probed `/agent-configs/templates` for writability, hit the intentional
`:ro` mount, fell back to `./config/agent-templates/<name>` — a path
that resolves to `/app/config/...` INSIDE the backend container, with
no host mapping. The new agent's bind mount (computed from
`HOST_TEMPLATES_PATH`) then pointed at a host path that did not exist,
so `/template` came up empty and the workspace was bare.

Fix: write deployed-local templates to `/data/deployed-templates/`,
which is already host-bound via `TRINITY_DATA_PATH`, writable, and
owned by UID 1000 (after #874 / #969). The curated catalog at
`/agent-configs/templates` stays read-only — operators' source of truth
is preserved.

- `services/agent_service/deploy.py`: drop the writability probe and
  silent fallback; always write to `DEPLOYED_TEMPLATES_DIR_IN_BACKEND`;
  on `OSError`, fail with HTTP 500 + `code=DEPLOYED_TEMPLATES_DIR_UNWRITABLE`
  pointing the operator at the non-root-volumes migration doc.
- `services/agent_service/crud.py`: extend `local:` template resolution
  to look in both `/agent-configs/templates` (curated) and
  `/data/deployed-templates` (deployed). Curated wins on name collision.
  Bind into the new agent uses `HOST_TEMPLATES_PATH` or
  `HOST_DEPLOYED_TEMPLATES_PATH` accordingly.
- `docker-compose.yml` + `docker-compose.prod.yml`: add
  `HOST_DEPLOYED_TEMPLATES_PATH` env var (defaults to
  `${TRINITY_DATA_PATH:-./trinity-data}/deployed-templates`) so the
  backend knows the host path for the per-agent bind.
- `tests/test_deploy_local.py`: poll `/api/agents/{name}/files` after
  deploy and assert `template.yaml` + `CLAUDE.md` are visible in the
  agent's workspace. This catches the silent-empty-agent class.
- `docs/memory/feature-flows/local-agent-deploy.md`: replace the stale
  probe-and-fallback description with the new single-path behavior.

Related to #950

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

* fix(deploy-local): refactor to put_archive — works on dev + prod (#950)

Manual end-to-end test on the local dev compose surfaced a second
bug class: the bind-mount transport for `local:` templates assumed
the backend's `/data` and the new agent's host bind would resolve
to the same host path. That's true in prod compose (host bind for
`/data`) but false in dev compose (named volume for `/data`), so
any host-path math in crud.py was right on prod and wrong on dev.
Net: prior version of this PR fixed prod but would still ship empty
agents on dev.

Refactored: drop the bind-mount transport for deploy-local entirely.
Instead, pre-populate `agent-{version}-workspace` directly via
`put_archive` into an ephemeral `alpine:3.20` container that mounts
the workspace volume. The agent's `startup.sh` sees the
`.trinity-initialized` marker we drop in the same tar and skips its
`/template` -> `/home/developer` copy.

Same uniform path on dev and prod — neither environment depends on
host-path mapping for the deploy-local case. Curated templates
(`/agent-configs/templates/...`) are unchanged; they keep the
existing bind-mount transport because the curated catalog IS bound
from the same host path into both backend and agent.

Verified live on local docker-compose.yml stack:
- POST /api/agents/deploy-local returns 200
- /home/developer is `developer:developer` (UID 1000) so the agent
  can write to its own home
- template.yaml + CLAUDE.md + nested .claude/skills/test/SKILL.md
  all visible via /api/agents/{name}/files and downloadable
- .trinity-initialized marker present
- Agent startup creates .cache/, content/, .trinity/ without
  permission errors (was failing before chown of volume root)

Diff vs prior commit on this branch:
- deploy.py: + `_prepopulate_workspace_from_template` helper. Creates
  workspace volume, builds a tar with extracted template + marker
  entry (all uid/gid 1000), runs an ephemeral alpine container that
  receives the tar via put_archive and chowns the volume root to
  1000:1000. Image is pre-pulled if missing.
- crud.py: drop the deployed-templates branch in template_volume
  setup. /data/deployed-templates is still consulted by the
  template.yaml resolution above for redeploys, but the bind into
  the new agent container is no longer needed for deploy-local.
- docker-compose.yml + prod.yml: drop HOST_DEPLOYED_TEMPLATES_PATH
  env var (no longer read by any code path).
- tests/test_deploy_local.py: explicitly start the agent after
  deploy before polling /files (deploy returns the agent stopped).
- feature-flows/local-agent-deploy.md: document the put_archive
  approach and why we avoid the bind-mount transport.

Related to #950

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

* fix(deploy): guard local template name against path traversal (#950)

CodeQL py/path-injection on PR #971 flagged two paths in crud.py
where `config.template[6:]` (user-supplied after `local:` strip) is
joined directly onto `/agent-configs/templates` and
`/data/deployed-templates`. A `local:../../...` payload would resolve
outside the templates dir, letting a `creator`-role user probe for
the presence of `*/template.yaml` files on the backend FS and — if
one happened to exist — have its contents pulled into `template_data`
(type / resources / tools / runtime fields), surfaced via the agent
config and `GET /api/agents/{name}`.

The pre-existing code had the same issue; the alerts surfaced because
the #950 diff touched those lines. Fixing now while the surface is
fresh.

Add `_validate_local_template_name` that requires
`[a-zA-Z0-9][a-zA-Z0-9_.-]*` with no `..` substring, called from
both the template.yaml-read block (line 282) and the template-volume-
bind block (line 379). Rejection returns HTTP 400 with structured
`code=INVALID_LOCAL_TEMPLATE_NAME`.

Verified live:
- `POST /api/agents` with `template=local:../../etc/passwd` → 400
- `POST /api/agents` with a valid name → proceeds as before

Related to #950

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

* fix(deploy): use resolve()+is_relative_to barrier for path-injection (#950)

Prior commit's regex-only validator passed the security test but
CodeQL didn't recognise it as a barrier — `py/path-injection` alerts
174 and 175 stayed open on PR #971 after re-scan because the static
analyser doesn't follow the regex barrier through the helper-function
indirection. Switch to the canonical pattern CodeQL does recognise:

  candidate = (root / name).resolve()
  if not candidate.is_relative_to(root): raise

Defense is now layered:
  1. Regex allowlist (fail fast for obviously hostile input)
  2. Resolve + is_relative_to (the CodeQL-recognised barrier)

Helper renamed `_validate_local_template_name` -> `_safe_local_template_path`
to reflect that it now returns the resolved path. Both crud.py callsites
updated to use the resolved path directly. The `template_volume` bind
in the second callsite uses `curated_path.name` instead of the raw
input — `.name` is the last path component of an already-validated +
resolved path, which CodeQL treats as fresh data.

Behavior preserved (verified live on local stack):
- POST /api/agents with template=local:../../etc/passwd -> 400 INVALID_LOCAL_TEMPLATE_NAME
- POST /api/agents with a valid name -> proceeds normally

Related to #950

---------

Co-authored-by: Claude Opus 4.7 (1M context) <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.

1 participant