Skip to content

fix(files): credential files are owner-tier to read; file reads never follow a link; runtime config files join the write deny lists - #3304

Open
AndriiPasternak31 wants to merge 24 commits into
devfrom
AndriiPasternak31/ent819-823
Open

AndriiPasternak31 wants to merge 24 commits into
devfrom
AndriiPasternak31/ent819-823

Conversation

@AndriiPasternak31

@AndriiPasternak31 AndriiPasternak31 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Description

File download and preview now apply an owner tier to credential files, file reads never follow a link, and the Claude Code, Gemini and Codex runtime config files join every write list.

Credential files are owner-tier to read. GET /api/agents/{name}/files/download and /files/preview refuse a credential path unless the caller is a person who passes the owner tier (the agent's owner or an admin). Credential paths: .env, .env.*, .mcp.json, .mcp.json.template, .credentials.enc, .ssh/*, .aws/*, .gcp/*, .claude/settings*.json, .git/config, the runtime config files below, their /proc and /dev spellings, .trinity/git-credential, .trinity/backup/*, and the secret file classes (*.pem, *.key, *.p12, *.pfx, .kube/config, .config/gcloud/*). The check runs on the normalised path, and that same path is what the agent server receives. A refusal is a 403 with {code: "owner_tier_path", message, path}; refused reads, and allowed reads by anyone other than the owner's own signed-in session, write an AUTHORIZATION audit row. Agent, system, connector and ops keys get person_required on these paths; on an agent running the new image they still read the other files they could read before (for example .trinity/pipelines/*).

Teammates on a shared agent. They can still chat with the agent, and the agent keeps using its credentials. They cannot open or download credential files. Managing credentials (set, inject, import, export) was already owner-only. To see or change a credential, they ask the agent's owner or an admin; the 403 message and the user docs say so. A grant that lets an owner delegate credential management follows separately.

File reads never follow a link. The agent server opens a download or preview path one component at a time without following any link, and serves the file it opened. A path that is a link, or passes through one, is refused with 403 resolved_path_mismatch for every caller, the owner and the platform's own reads included; the backend records the refusal. A platform read that meets such a link logs a warning naming the file. Templates should ship real files at the paths Trinity reads (CLAUDE.md, template.yaml, dashboard images): a linked CLAUDE.md stops receiving the refreshed Platform Skills section, and a linked template.yaml is unreadable to the platform.

Agents not yet on the new base image. The backend checks once per container and image whether the agent server opens reads without following links. Until an agent is recreated on the new image, download and preview by anyone other than its owner or an admin (agent keys included) answer 403 agent_restart_required: "This agent needs a restart to apply an update. Ask the agent's owner or an admin to stop and start it in Trinity." Each such refusal is logged with a running count.

Runtime config files join the write lists. ~/.claude.json, ~/.claude/.credentials.json, ~/.gemini/settings.json and ~/.tmp/codex/* (Codex's login and MCP config) are refused for PUT, mkdir and DELETE at the backend, in the file guardrail hook's path_deny (plus an image-build smoke row for ~/.claude.json), and at the agent server (by name, and on the resolved target for all four). The Files tab can no longer delete .tmp, since it holds the Codex runtime's login and MCP config. Platform writers are unaffected: a grep finds no writer of these files through the backend Files routes or the agent server's PUT /api/files (subscriptions use CLAUDE_CODE_OAUTH_TOKEN; plugins_reinstall.py changes ~/.claude.json through the claude CLI; snapshot restore writes through restore_from_tar, which no list consults; Gemini's MCP entry and Codex's login are written straight to disk by the platform).

UI. The Files and Credentials panels show the server's refusal message. The Credentials panel reads a file before opening its editor, so a refused read shows the message instead of an empty editor.

Rollout. The backend half applies to every agent on deploy. The agent-side halves (no-link reads, the hook baseline, the agent-server lists) reach an agent when it is recreated on the new base image: stop and start each agent through Trinity.

Proposed release-note line: "Credential files in an agent are owner-tier to read; file reads no longer follow links; Claude Code, Gemini and Codex runtime config files join the protected-file lists."

Release step (required): after deploying, stop and start every agent through Trinity so each one is recreated on the new base image. Until an agent is recreated, every read of its files by anyone below its owner or an admin answers 403 agent_restart_required. That covers people the agent is shared with and agent keys, including the MCP tools get_agent_pipeline_state and list_agent_pipelines that orchestrator agents use to read sibling pipelines.

Hold for the linked private issue before merging.

Related Issue

Refs abilityai/trinity-enterprise#819
Fixes abilityai/trinity-enterprise#823

Journey Impact

Journey Impact: none: the owner's Files and Credentials tabs are unchanged except that links are not opened; a non-owner gets a 403 with an explanation on credential paths

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change) — links are no longer opened by download/preview; .tmp can no longer be deleted from the Files tab
  • Documentation update

Testing

New and extended tests (Python 3.14 locally; CI runs 3.13):

File Tests Red on dev before this branch
tests/unit/test_ent819_files_read_tier.py (new) 205 163 of the first 195 (the 10 route-level ones were added after that run)
tests/unit/test_ent819_agent_server_read_boundary.py (new; loads the shipped agent-server router by path) 27 all (no read seam on dev); teeth shown by mutation below
tests/unit/test_ent819_no_platform_links_in_home.py (new; premise pin: Trinity creates no links in an agent home) 13 green by design; teeth by planting a line in a temp copy
tests/unit/test_ent823_agent_server_protected_paths.py (new) 23 21
tests/unit/test_files_protected_paths.py 237 collection error on dev (new constant); 64 per test with that import guarded
tests/unit/test_ent792_file_guardrail_hook.py 33 8
tests/unit/test_persistent_state_reader.py (list snapshot) 7 1
src/frontend/tests/unit/fileReadRefusal.spec.js (new) 11 7
  • Frontend: npm run test:unit 281 files / 4874 passed; npm run build passes (raw-colour and loading-gate ratchets green).
  • /verify-local with the agent stage: backend build + import, agent base-image build + import (the build executed guard002-smoke.py, including the new ~/.claude.json row), boot, and a real agent reaching /health all pass; Claude Code CLI in the image is 2.1.281. Unit and integration reds were attributed by failing node id against a clean dev worktree with the same interpreter and order: none appears only on this branch (integration: test_circuit_breaker.py::TestDormantState::test_dormant_transition_emits_operator_queue_alert fails identically on dev).
  • Live checks on a local stack with a real share: a non-admin user the agent is shared with gets 403 owner_tier_path on .env and .mcp.json (download and preview) and 200 on .trinity/pipelines/x.yaml; the owner gets 200 on .env; refusals write file_read_refused rows. As owner, a file that is a link gets 403 resolved_path_mismatch on download and preview, with an audit row. Inside a running agent on the new image, the agent server refuses PUT ~/.claude.json and DELETE ~/.claude (an ordinary write with the same header succeeds), and the backend refuses deleting .tmp. An agent on the previous image answers a shared user's read with agent_restart_required and the exact message, and logs refusals=1; the same user reads from the agent on the new image.

Mutation: reverted from a scratch copy, restored byte-identical, red-in-K/N:

  • download without the owner-tier call: 67 of 535 backend tests red (1 of 61 through the real route);
  • forwarding the raw path instead of the checked one: 6 of 10 route-level tests (test_the_real_routes_send_the_agent_the_path_that_was_checked);
  • an audit failure that raises: 4 of 10 route-level tests (test_the_real_routes_refuse_a_shared_user_when_the_audit_store_fails, …_serve_an_admin_when_the_audit_store_fails);
  • the older-image check without the PERSON gate: 2 of 10 route-level tests (test_the_real_routes_refuse_the_owners_agent_key_on_an_unverified_image), 5 of 535 overall;
  • agent server without O_NOFOLLOW: 6 of 61 (test_a_link_anywhere_in_the_path_is_refused, test_the_handlers_refuse_a_link); without the leading-slash collapse: 1 of 61; preview reopening by name: the swap test (test_the_descriptor_not_the_name_is_what_is_read); DELETE without the ancestor check: 3 of 61;
  • the probe caching a Docker error: 3 (test_the_real_exec_helper_without_docker_is_not_cached and two cases of test_an_inconclusive_probe_fails_safe_and_is_not_cached); reads past the opened size: 2 (test_the_handlers_serve_the_size_they_opened).

Checklist

  • My code follows the project's style guidelines
  • I have updated the documentation (if applicable)
  • I have not committed any sensitive data (API keys, credentials, etc.)
  • I have added appropriate logging for new functionality

Screenshots (if applicable)

None attached. The Credentials and Files panels show the server's refusal message; the panel look is pending review.

…low a link; runtime config files join the write lists

Requirements (content-files §13.1, GUARD-002 §28) and the file-routes
bullet in the architecture area file, ahead of the code.

Refs Abilityai/trinity-enterprise#819
Refs Abilityai/trinity-enterprise#823
…review

One credential constant feeds the write list and a new owner-tier read
set: the credential paths plus .git/config, the /proc and /dev spellings,
the Trinity-managed copies (.trinity/git-credential, .trinity/backup/*) and
the secret file classes (.kube/config, .config/gcloud/*, *.key, *.pem,
*.p12, *.pfx). Download and preview refuse an owner-tier path unless the
caller is a person who passes the owner tier (owner or admin); the PERSON
gate runs first. A NUL byte is refused with 400 before the agent is
called, and the agent is sent the normalised path that was checked.

The 403 body says what a teammate can still do and whom to ask.

Refs Abilityai/trinity-enterprise#819
Every refused owner-tier read writes one file_read_refused row, and every
allowed one by anyone other than the owner's own signed-in session writes
file_read_allowed (AUTHORIZATION, routed endpoint, normalised path, rule).
A person is filed as actor_user; an agent key as its agent; other keys by
scope and key, never as the owner. The audit is best-effort: a failure
never changes the answer.

Refs Abilityai/trinity-enterprise#819
~/.claude.json, ~/.claude/.credentials.json, ~/.gemini/settings.json and
~/.tmp/codex/* (the Codex runtime's login and MCP config) are refused on
write, mkdir and delete at the backend and are owner-tier to read.
Deleting .tmp or .tmp/codex through the Files routes is refused, since
they hold the Codex login.

Refs Abilityai/trinity-enterprise#823
path_deny gains ~/.claude.json, ~/.claude/.credentials.json,
~/.gemini/settings.json and ~/.tmp/codex/*, and the image-build smoke
gains a ~/.claude.json row, so the build proves the baseline.

Refs Abilityai/trinity-enterprise#823
… its file routes

PROTECTED_PATHS and EDIT_PROTECTED_PATHS gain .claude.json and
.credentials.json. PUT, mkdir and DELETE also refuse any resolved target
inside the runtime config paths (Claude Code's two login files, Gemini's
settings.json, Codex's .tmp/codex), so a link elsewhere in the home that
points at one is refused too; DELETE refuses a directory above one. The
home is a module constant so the handlers run over a temporary home in
tests. Snapshot refreshed.

Refs Abilityai/trinity-enterprise#823
The agent server opens a download or preview path one component at a
time with O_NOFOLLOW and serves the descriptor it opened, so the checked
file is the file read. A path that is a link, or passes through one, is
refused with 403 resolved_path_mismatch for every caller, the owner and
the platform's own reads included. The backend turns that refusal into a
file_read_refused audit row and a structured 403; a platform read that
meets one logs a warning naming the file. The fence is resolved-path
containment, not a string prefix.

A source scan pins that Trinity itself creates no links in an agent home.

Refs Abilityai/trinity-enterprise#819
…reads

The backend probes each agent container once, per container and image,
for the agent server's no-link read (ent#708-style: grep its source for
the read function). Until an agent is stopped and started on the current
image, download and preview by anyone below the owner tier are refused
with 403 agent_restart_required; the owner and admins are unaffected and
never trigger the probe. A missing target fails safe and is remembered for
that image; an exec that raises or times out fails safe and is probed
again on the next read. Each refusal is logged with a running count.

Refs Abilityai/trinity-enterprise#819
The download and preview store calls parse a text or blob error body back
into {detail, code, path}, so a structured refusal reads as its message
instead of "Request failed with status code 403". The Credentials panel
reads first and opens the editor only on content or a 404; any other
failure keeps it closed and shows the message in an InlineError beside
the file list. The Files panel's Download shows the message too.

Refs Abilityai/trinity-enterprise#819
…ads, no-link reads and runtime config files

file-browser (owner-tier set, link refusal, older-image refusal, audit,
error bodies and what the UI shows), agent-guardrails (path_deny + 4),
agent-sharing and credential-injection, the index; user docs say what a
teammate on a shared agent can and cannot do and whom to ask, that the
Files tab does not open links, and that the runtime login files and .tmp
are not editable there.

Refs Abilityai/trinity-enterprise#819
Refs Abilityai/trinity-enterprise#823
The Work card reads pipeline files over its own httpx client, so the
platform-read warning in agent_client.read_file never reached it. A 403 body
is now read under the same byte budget and passed to that warning.

Refs Abilityai/trinity-enterprise#819
The 403 agent_restart_required message now reads: "This agent needs a
restart to apply an update. Ask the agent's owner or an admin to stop and
start it in Trinity." It is shown on agents not yet on the current image.

Refs Abilityai/trinity-enterprise#819
The older-image check on download and preview now exempts the same caller
the credential tier serves: a person who passes the owner gate (the
agent's owner, or an admin). Agent keys, the owner's own included, are
refused with 403 agent_restart_required like any caller below the owner
tier until the agent restarts on the current image; on a verified image
they read as before.

Refs Abilityai/trinity-enterprise#819
The read-policy probe caches a verdict only when grep answered: exit 0
(verified), or exit 1 or 2 with no output (token or file absent; grep -qs
prints nothing). The exec helper reports a Docker fault as exit 1 with
text in its output, so that, a timeout or any other code now reads as
"not verified" for this read, is logged, and is probed again next time.

Refs Abilityai/trinity-enterprise#819
A guard reads the probe's path and token from the backend module, maps the
path through the base image's COPY of ./agent_server to the repo file, and
asserts that file defines the token as a module-level function (matched on
the def with comments stripped). Renaming or moving the read function now
fails here instead of on every agent after the next image build.

Refs Abilityai/trinity-enterprise#819
The objective join reads template.yaml and objectives over the agent door
directly, so the platform-read warning in agent_client.read_file never
reached it. A 403 from that read now goes through the same warning, which
names the file when the agent server refused it as a link. The read still
reports the file as unreadable.

Refs Abilityai/trinity-enterprise#819
Download reads at most the file's size when it was opened, and the preview
stream stops at the Content-Length it declares. A file that grows after
the open (an agent log, say) is served at its opened size, so the body
never runs past the declared length.

Refs Abilityai/trinity-enterprise#819
…s nothing

Quick Inject reads the current .env before merging. A coded 403 on that
read now has a mounted case: the server's message is shown and nothing is
injected; an uncoded failure keeps the generic message.

Refs Abilityai/trinity-enterprise#819
A Python file that fails to tokenize, or is not UTF-8, was skipped by the
scan for link creation in an agent's home. It is now scanned as raw text,
comments included, so it can only add a finding, never hide one.

Refs Abilityai/trinity-enterprise#819
… tier

The requirements, architecture and feature-flow lines on the file routes
now state the rule: reading credential paths and the secret file classes
is owner-tier, the credential, alias and Trinity-copy patterns are on the
write deny list, and the per-verb tier follows separately.

Refs Abilityai/trinity-enterprise#819
…onfig test files

Registry entries for the four new test files and the two extended ones.

Refs Abilityai/trinity-enterprise#819
Refs Abilityai/trinity-enterprise#823
…nk read

The file-browser flow describes the PERSON gate before the owner tier, the
older-image check (only a person who is the owner or an admin skips it;
only grep's quiet answer is cached), the descriptor-based agent-server
reads that stop at the opened size, the three platform readers that name a
linked file, and the current line references. The sharing and credential
flows match the panel and the restart message.

Refs Abilityai/trinity-enterprise#819
… teammate gets a restart

The architecture bullet and requirements now say a conclusive older-image
probe is cached per container and image, and that only a person who is the
owner or an admin skips the check (agent keys included in the refusal).
Requirements state reads stop at the size the file had when opened.
The Files user doc quotes the restart message and lists every owner-tier
path; the sharing doc says whom to ask for a restart and that credential
management is for the owner or an admin.

Refs Abilityai/trinity-enterprise#819
Refs Abilityai/trinity-enterprise#823
…it failure and probe agent keys

Route-level tests over routers.agent_files.router for download and preview:
the agent is sent the normalized path, a failing audit store changes no
answer (403 for a shared user, 200 for an admin), and the owner's
agent-scoped key gets agent_restart_required on an unverified image.

Refs Abilityai/trinity-enterprise#819
@AndriiPasternak31 AndriiPasternak31 added the ui PR touches the frontend UI — triggers Playwright e2e tests label Oct 7, 2026
@AndriiPasternak31
AndriiPasternak31 marked this pull request as ready for review October 7, 2026 00:48
@dolho

dolho commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

/review Report

Branch: AndriiPasternak31/ent819-823 → dev (merge-base a72271d9c)
Files Changed: 31 (+2537/-206)
Scope: CLEAN
Plan Completion (ent#823 acceptance criteria, plus the ent#819 read-tier ask): 3 done / 0 partial / 0 not done / 0 changed / 1 unverifiable

  • DONE: both runtime-config paths (plus Gemini and Codex equivalents) refused on PUT / mkdir / DELETE at the backend (services/agent_service/files.py _RUNTIME_CONFIG_PATTERNS folded into _FILE_WRITE_DENY_PATTERNS), in the hook baseline (guardrails-baseline.json path_deny + guard002-smoke.py row) and at the agent server (PROTECTED_PATHS / EDIT_PROTECTED_PATHS by name, _touches_runtime_config on the resolved path).
  • DONE: tests drive the shipped modules (the agent-server router is loaded by path, the backend routes are driven end to end).
  • DONE (ent#819): download and preview gate credential paths at the owner tier via _enforce_owner_tier_read, with audit rows, and the agent server reads through _open_for_read (per-component O_NOFOLLOW, serves the opened descriptor).
  • UNVERIFIABLE from the diff: "subscription credential injection and platform .claude.json writes keep working". I grepped src/backend and docker/base-image and found no platform writer of these files that goes through the backend file routes or the agent server's PUT /api/files (Gemini's settings.json is written directly by agent_server/services/trinity_mcp.py, not through the route), which matches the PR's claim.

Execution coverage (Step 2.5)

changed symbol / test file executed by live consumer verdict
_enforce_owner_tier_read, _is_owner_tier_read_path, _audit_read test_ent819_files_read_tier.py (service and real-route tests) download_agent_file_logic / preview_agent_file_logic ← routers/agent_files.py:222,233 ✅ executed
_refuse_below_owner_on_unverified_image, _agent_reads_without_links test_ent819_files_read_tier.py (probe and route cases) same two service functions ✅ executed
agent server _open_for_read, download_file, preview_file test_ent819_agent_server_read_boundary.py (router loaded by path over a temporary home) the agent server's /api/files/download and /api/files/preview ✅ executed
agent server _touches_runtime_config, list additions test_ent823_agent_server_protected_paths.py update_file / create_folder / delete_file ✅ executed
_warn_if_link_refusal (agent client, pipeline reader, objective join) test_ent819_files_read_tier.py client.py:619, pipeline_state.py, objective_join_service.py:756 ✅ executed
guardrails-baseline.json path_deny test_ent792_file_guardrail_hook.py (runs the hook), the image-build smoke the hook inside the agent image ✅ executed
test_ent819_no_platform_links_in_home.py — CI itself (scans the codebase for link-creating calls) 🛡 guard
fileReadRefusal.spec.js mounted with @vue/test-utils under jsdom stores/agents.js, CredentialsPanel.vue, FilesPanel.vue ✅ executed (not run locally: the local node_modules lacks jsdom; CI is green)

Fix mutation (done locally on a scratch edit, then restored; working tree clean afterwards):

  • replacing both await _enforce_owner_tier_read(...) calls with pass: 112 red across test_ent819_files_read_tier.py + test_files_protected_paths.py
  • dropping O_NOFOLLOW from the agent server's walk: 6 red in test_ent819_agent_server_read_boundary.py
  • disabling the _touches_runtime_config check in update_file: 4 red in test_ent823_agent_server_protected_paths.py

The 545 tests in the seven touched Python test files pass locally. All CI checks are green.

Critical Findings (block merge)

None.

Informational Findings (review required)

[I1] Test/Security gap: hard links are not covered by the no-link read (Confidence: 7/10)
File: docker/base-image/agent_server/routers/files.py (_open_for_read)
Issue: O_NOFOLLOW refuses symbolic links only. A hard link (ln .env notes.txt, created by the agent process, which owns both files) is a regular file. It passes S_ISREG and is served under its non-credential name, so the backend's lexical owner-tier check doesn't apply to it. The practical impact is limited, because a teammate who can get the agent to make that link can also just ask it to print the file in chat, and the PR already accepts that boundary. Still, the PR describes link refusal as a rule for every caller.
Suggestion: Either refuse st.st_nlink > 1 after the final fstat with the same resolved_path_mismatch code (cheap, and consistent with the stated rule), or state in the file-browser feature flow that only symbolic links are covered.

[I2] Older-image probe trusts a file the agent can write (Confidence: 6/10)
File: src/backend/services/agent_service/files.py (_agent_reads_without_links); docker/base-image/Dockerfile:350-354 (/app is developer:developer)
Issue: The probe greps /app/agent_server/routers/files.py for _open_for_read. On an older image the agent process can write that token into the file, so the probe reports the image as verified and the verdict is cached for the container's lifetime. Below-owner reads would then go to a server that still follows links. The impact is the same limited one as I1, since it needs a cooperating agent. Worth knowing that the check measures what the file says, not what the running process does.
Suggestion: No change needed for this PR. If the check outlives the rollout, a version endpoint served by the running process, or a root-owned marker, would be a stronger signal.

[I3] Probe cache is unbounded (Confidence: 7/10)
File: src/backend/services/agent_service/files.py (_READ_POLICY_PROBE_CACHE)
Issue: The cache is keyed by (container id, image id) and never evicted, so every recreate adds an entry for the life of the worker. The growth is small (one bool per container generation), but it has no ceiling.
Suggestion: Bound it, for example with a small LRU or by dropping entries for container ids that no longer exist, or note that the growth is accepted.

[I4] Rollout: agent keys lose file reads on every non-recreated agent (Confidence: 8/10)
File: src/backend/services/agent_service/files.py (_refuse_below_owner_on_unverified_image)
Issue: This is intended and disclosed. Only a person at the owner tier skips the image check, so agent-scoped and system keys get agent_restart_required on every path (including .trinity/pipelines/*, read by the MCP get_agent_pipeline_state / list_agent_pipelines tools) until the target agent is recreated. Orchestrator agents reading sibling pipelines will fail across the fleet between deploy and restart.
Suggestion: Keep the proposed release-note line and make "stop and start every agent" an explicit step in the release checklist.

[I5] Process: merge hold (Confidence: 9/10)
The PR body asks to hold for the linked private issue before merging. This approval covers the code only and does not lift that hold.

Clean Categories

  • SQL & data safety: no SQL or schema changes in the diff.
  • Race conditions: the agent server serves the descriptor it opened (no check-then-open-by-name), and download and preview cap reads at the opened size. The probe cache is per-process and stores only conclusive grep answers.
  • Auth boundary: the owner tier runs after the existing can_user_access_agent gate. assert_person comes first, so agent, connector, system and ops keys are below the tier (PERSON_SCOPES = {None, "user"}). The forwarded path is the normalized one that was checked. A leading // is collapsed on both sides (_normalize_user_path, _open_for_read), and /proc/* and /dev/* aliases are owner-tier.
  • Credential exposure: refusal bodies and logs carry the path and the code only, never file contents. Audit failures are swallowed with a warning and don't turn into a 500 or an allowed read.
  • Enterprise disclosure in public docs: no paid-module or private-schema content in the docs diff, and the enterprise-docs-guard check passes.
  • Documentation: architecture security.md, feature flows, requirements and user docs are updated alongside the code.

Summary

  • Critical: 0 (none found)
  • Informational: 5 (review recommended; I1 is the one worth a small follow-up)
  • Scope: clean

🤖 Generated with Claude Code

@dolho dolho 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.

Approved — /review found no blocking findings; see review comment above.

@vybe

vybe commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

merge-train (2026-10-07): not on this train, because of the hold in the body ("Hold for the linked private issue before merging"). The ent#819 question about preserving proxy access logs is still unanswered. Validation otherwise found no CRITICAL. Warnings for whoever lifts the hold: every non-owner file read returns 403 agent_restart_required until agents are recreated on the new image (the release note needs a stop/start step), and the body should say Fixes abilityai/trinity-enterprise#823 (fully resolved) and keep Refs for ent#819 (partial). Rides the next train once the hold is lifted.

@github-actions

github-actions Bot commented Oct 7, 2026

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 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

merge-train: not taken this batch — the body's "Hold for the linked private issue before merging" still stands (the log-preservation question on the private issue is unanswered). Validation otherwise found no CRITICALs.

When the hold lifts:

  • it needs a dev merge (conflicts in docs/memory/feature-flows.md, docs/user-docs/agents/agent-runtimes.md, tests/registry.json — routine);
  • the body should carry Fixes abilityai/trinity-enterprise#823 + Refs abilityai/trinity-enterprise#819 (cross-tracker: set status-in-dev by hand after merge);
  • the release notes need an explicit stop/start step: until an agent runs the new image, every non-owner read (agent keys included → get_agent_pipeline_state / list_agent_pipelines) gets 403 agent_restart_required.

@vybe vybe added the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

⚠️ Live-instance suite skipped — merge conflict against dev.

Resolve by merging dev locally and pushing the result; the next nightly re-tests.

vybe added a commit that referenced this pull request Oct 9, 2026
…orable secret value is a named 422 (#3325) (#3417)

Fixes #3325 — independent fix, one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. Base is `dev`. **Draft until the operator merges.**

## What
- `src/backend/services/credential_encryption.py::decrypt`: every malformed envelope shape raises `ValueError`, as the function promises — JSON that is not an object (`[]`, `null`), and a non-string nonce or ciphertext. Importing such a `.credentials.enc` now returns **400** instead of a 500 with a stack trace in the log. The envelope format and every successful decrypt are unchanged.
- `src/backend/services/secret_settings.py::encrypt_secret_setting`: the key is checked first, in its own `try`, and only that check can raise `MissingEncryptionKeyError`. A value that cannot be encoded (for example a lone surrogate) raises the new `SecretSettingValueError(ValueError)`. Its message names the setting, never the value.
- `src/backend/error_handlers.py` + `src/backend/main.py`: one app-level handler maps `SecretSettingValueError` to **422**. `src/backend/routers/settings/credentials.py`: three pass-through lines in the routes whose catch-all would have turned it into a 500 (Anthropic, GitHub PAT, `PUT /slack`).
- Tests: the three strict-xfail markers in `test_ec_credential_crypto_edges.py` are removed; new `tests/unit/test_3325_credential_error_types.py` (registered in `tests/registry.json`) asserts no value in the message or the exception chain, a missing key reported before a bad value, `PUT /api/settings/api-keys/anthropic` → 422, and that `main.py` registers the handler.

## Rulings carried (orchestrator, on the operator's behalf — plan file)
- TD-1: one app-level handler plus the three pass-through lines — as recommended.
- TD-2: a `.credentials.enc` that decrypts correctly but whose contents are not an object is deferred — as recommended; forging one needs the platform key.
- TD-3: 422, matching the existing 422 for refused cleartext writes — as recommended.

## Review + security
`/review` reading pass (claude-fable-5-1, report-only): **MERGEABLE**, no critical findings. The secret cannot leave through the new error: it is raised after the `except` block, so `__context__` is cleared, not merely hidden; the handler returns only the message; nothing logs it; audit rows are written only after a successful write. The only residual carrier is traceback frame locals, which nothing in the backend renders. The handler is keyed on the subclass, so no other `ValueError` is swallowed or relabelled. `/cso --diff`: no findings.

Evidence sweep (claude-opus-5-5): **GREEN**. 23 unit files import the touched modules; the 16 most direct ran shuffled (seed 12345), one file per process, all passed. All 7 tests in the new file ran, 0 skipped. With both service files taken from `dev`, exactly the three formerly-xfail tests fail (8 cases). Seven less direct files are left to CI and named in the review file. `git merge-tree` against `dev` f6bedcd is clean.

## Tests
Targeted counts above. Not run here: the full unit island (CI).

## Before merge
- Startup auto-import (`lifecycle.py`) on a malformed envelope now lands in its `ValueError` retry arm: it retries without a warning per attempt and ends in the same "failed" result, with the existing error log after the last attempt.
- Enterprise callers of `decrypt` get `ValueError` where they got `AttributeError` / `TypeError` on a malformed envelope. Two catch broadly, three propagate as before; no change needed there.
- The new test file has a module-level `importorskip` for `sqlalchemy` and `cryptography`: it skips silently where those are missing. They are present on the engineer and in CI.
- Ten open PRs share `tests/registry.json` or `src/backend/main.py` with this branch (#3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2713, #2709). A test-merge of this head against each adds no conflict beyond what that PR already has with `dev`.

## Handoffs (not in this diff)
- TD-2 above: the decrypted-but-not-an-object case can still 500 on import.
- `PUT /api/settings/slack` saves the client ID before a later secret write fails (partial update, pre-existing).
- The GitHub PAT, Resend and Gemini routes are covered by the app-level handler but not exercised by the new route test.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
vybe added a commit that referenced this pull request Oct 9, 2026
…L is refused, not stored (#3323) (#3420)

Fixes #3323 — independent fix, one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. Base is `dev`. **Draft until the operator merges.**

## What
- `src/backend/utils/url_validation.py::reject_embedded_credentials`: strips its input and treats a leading `//` as already having a host, the same rule `strip_url_credentials` uses. `//<token>@github.com/o/r` is now refused like the `https://` form, including with leading whitespace or mixed case. All three write paths that store a skills-library URL go through this one function (`routers/skills.py` create and update, legacy adoption in `services/skill_service.py`), so the token is no longer saved in plaintext to `skill_sources.url` or the audit details.
- If `urlparse` refuses an input that starts with `//`, the function falls back to the module's existing userinfo regex instead of raising, so this fix adds no new 500.
- Tests: the D3 strict-xfail marker is removed; four more refused shapes and a four-case "must not raise / must not over-reach" test are added.

## Rulings carried (orchestrator, on the operator's behalf — plan file)
- TD-1: the fallback applies only to inputs starting with `//`; the bare `ValueError` for every other malformed input is #3322's and is unchanged (D4 stays a strict xfail) — as recommended.
- TD-2: keep the username/password check; `//@host` carries no secret and is not refused — as recommended.
- TD-3, TD-4: deferred, see Handoffs — as recommended.

## Review + security
`/review` reading pass (claude-fable-5-1, report-only): **MERGEABLE**, zero critical, five informational. The pre-fix and post-fix modules were executed side by side over forty inputs: nothing that was refused before now passes, credential-free inputs are unchanged, the fallback regex is anchored and linear. `/cso --diff`: no findings.

Fixed after review:
- `546f329a` — the `tests/registry.json` descriptions no longer call #3323 a strict xfail.
- `a75a5e04` — the reject stub in `test_ent183_skill_packages.py` mirrors the fixed `//` rule. It is deliberately stricter than production on a malformed bracket (`//[oops/x` raises).

Evidence sweep after the fix (claude-opus-5-5): **GREEN**. 28 unit files import the module; the 14 most direct ran shuffled (seed 12345), one file per process. Three did not come back clean, none because of this branch:
- `test_ent236_skills_lifecycle.py::TestNonRepoDirectoryRecovers::test_real_repo_still_pulls` and `test_skill_service_user_agent.py` (collection error) fail the same way on `dev` f6bedcd in the engineer's environment.
- `test_ent237_skill_sources.py` hit the 240 s per-file limit at about 72 % with no failure printed — left to CI.

`git merge-tree` against `dev` f6bedcd is clean.

## Tests
895 passed across the 11 files that completed cleanly. Not run here: the 14 less direct importer files, `test_ent237_skill_sources.py` to completion, and the full unit island (CI).

## Before merge
- Ten open PRs share `tests/registry.json` with this branch (#3417, #3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2709). A test-merge of this head against each adds no conflict beyond what that PR already has with `dev`.
- One stale docstring remains at `tests/unit/test_ec_url_validation_properties.py:36` (still calls D3 a strict xfail).

## Handoffs (not in this diff)
- TD-3 (P3): shapes that still pass both reject and strip — `/\t/tok@`, `/\n/tok@`, `///tok@`, `https:/tok@` (`urlparse` removes tab and newline after the check). Fixing it needs the same change in `strip_url_credentials` and cases in both test families.
- TD-4: a property test that reject and strip always agree over the shared `test_2052` corpus; it needs exceptions for empty userinfo.
- #3322 (bare `ValueError` / `UnicodeError` → 500) is the sibling issue in the same file and is untouched.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
vybe added a commit that referenced this pull request Oct 9, 2026
…t the database (#3385) (#3421)

Fixes #3385 — one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. **Stacked on #3414** (the fix for #3314): base is `feature/3314-queue-fingerprint-falsy`, because both issues edit `_clamp_ingested_item` and the fingerprint helpers. Merge #3414 first, then retarget this to `dev`. **Draft until the operator merges.**

## What
- `src/backend/services/operator_queue_service.py`: new `OPERATOR_QUEUE_TYPE_MAX = 64` and `_bounded_type`. An agent-written `type` longer than 64 characters is shortened with the existing `…[truncated]` marker at ingest (`_clamp_ingested_item`); the ask is never refused or lost. `_comparable_type` applies the same bound, so a shortened row does not read as a rewrite (#2915) and a row stored before this fix at full length still compares equal.
- `src/backend/db/operator_queue.py::_insert_values`: `_DB_BELT_TYPE_MAX_BYTES = 1024`; above it the insert raises `ValueError`, like the id / title / question / context belts. All three create paths go through it.
- Docs: five lines in `architecture/api-endpoints.md`, `feature-flows/operating-room.md` and `requirements/security.md` that list the clamp and belt fields now name `type`.
- Tests in `tests/unit/test_1632_operator_queue_caps.py`: an oversized type is shortened with the marker; every platform-emitted type and every type `ask_operator` accepts is unchanged; the belt rejects an oversized type and passes 64 four-byte characters; `test_3385_clamped_type_is_not_a_rewrite` (clamped row vs raw entry, legacy full-length row, a real rewrite inside the first 52 characters).

## Rulings carried (orchestrator, on the operator's behalf — plan file)
- T1: a fixed constant, no environment variable — as recommended.
- T2: 64 characters — as recommended. The longest type the platform emits is `workspace_problem_report`, 24.
- T3: the DB belt raises, like its siblings — as recommended.
- Orchestrator: stacked on #3314 instead of branching from `dev`, so the two fixes do not hand the operator a conflict in the same two functions.

## Review + security
`/review` reading pass (claude-fable-5-1, report-only, against the stacked base): **MERGEABLE**, zero critical. Census of every literal `type` the platform and the enterprise submodule pass to the insert paths: longest is 24 characters, none is altered. Every truncated value ends in the marker, which no set member can match, so truncation cannot move a value into or out of a set that decides control flow (`_BUDGETED_ALERT_TYPES` and neighbours). The new `ValueError` is reachable only from the file sync loop, which quarantines it under its existing handler. `/cso --diff`: no findings; the error text carries the constant, never the value.

Fixed after review: `eab91c10` — I1, the five doc lines. Noted, not changed: I2, a non-string `type` over 1 KiB now raises the named `ValueError` where the base failed at the bind — same quarantine outcome.

Evidence sweep after the fix (claude-opus-5-5): **GREEN**. 13 operator-queue files ran shuffled (seed 12345), one file per process: 836 passed, 7 xfailed. With both production files taken from the #3314 branch, the two `TestClampType` cases, the belt test and the clamped-row fingerprint case go red. `git merge-tree` against `dev` f6bedcd is clean; the base branch has not moved (`923b1edc`).

## Tests
Counts above. Not run here: `test_ent815_queue_walk`, `test_ent815_broad_list_agent_scope`, `test_ent751_gate_entries` (2–4 minutes each; they ran green on the #3314 base in that PR's sweep) and the full unit island.

## Before merge
- **This PR's checks are weaker than a dev-based PR's:** a PR whose base is a feature branch runs a reduced CI set and `backend-unit-test` does not run. The first full run happens when it retargets to `dev` after #3414 merges — read that run before merging.
- Accepted trade-off: an agent that rewrites an oversized type only after character 52 is not detected as a change. Titles and questions already behave this way.
- Thirteen open PRs share a file with this branch (mostly `tests/registry.json` and `docs/memory/architecture/api-endpoints.md`; #3256 also `db/operator_queue.py` and the three docs). A test-merge of this head against each of #3420, #3417, #3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2713 and #2709 adds no conflict beyond what that PR already has with `dev`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants