feat(templates): the credential declaration standard — credential_setup: + two HARD-gate fixes (ent#128 PR-B) - #1899
Open
AndriiPasternak31 wants to merge 15 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Open
4 tasks
Contributor
Author
|
Filed the pre-existing traversal this PR's CodeQL alert surfaced (but did not introduce): #1900. CodeQL alert 260 is resolved in |
|
Resolve by running |
…hared_folders
`_resolve_local_template` read `creds.get("mcp_servers", {}).keys()` straight
through the block. A null / list / string `credentials:` raises AttributeError
there, and that read sits FIRST in a run of `config` mutations wrapped in one
broad `except Exception` — so the failure skipped every mutation after it. A
single malformed key therefore silently cost the agent its `runtime:` (wrong
harness) and its `shared_folders:` config too, with only a WARNING to show for
it.
Reads through PR-A's tolerant `credential_mcp_server_names()` instead, so the
credential parse degrades on its own and the unrelated settings survive.
The five malformed shapes are pinned as parametrized regressions; all five fail
on the pre-fix code.
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…info
Same uncaught reach-through PR-A fixed on the backend, still live on the agent
image: `.get("credentials", {}).get("mcp_servers", {}).keys()` raises
AttributeError on a null / list / string block at EITHER level, and the
endpoint's own `try/except` wraps only the YAML load — so the crash escaped as a
500 on the Info tab and the brain-orb route guard. `template.yaml` here is read
from the agent's own workspace, which the agent itself can rewrite, so this is
reachable without an operator touching anything.
The agent server ships in its own image and structurally cannot import
`src/backend`, so the reader is DUPLICATED, not imported. The two in-repo
precedents for that (`credential_paths.py`, `model_context.py`) are vendored
byte-identically WITH a parity test; a 6-line reader does not earn a whole
vendored module, but it does earn the same guard — before this commit NO parity
test covered `agent_server/routers/info.py`, so the copies could diverge freely.
Added in the `test_1713_scheduler_utils_parity.py` shape: one shared 17-row table
of malformed shapes driven through BOTH implementations, asserting agreement on
OUTPUT (the copies are textually divergent by design, so a source diff cannot
verify them).
Also routes the endpoint through the existing `get_template_path()` helper —
`/api/metrics` already does — instead of a second copy of the path literal, so
the regression is testable without patching `Path`.
This is the change that makes `/verify-local` mandatory WITHOUT `--skip-agent`.
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… it audits
K-001 (HARD) compared `.mcp.json.template`'s `${VAR}` references against an
UPPERCASE-ONLY view of `.env.example`. Trinity's substitution engines impose no
charset at all — the agent-side writer is a `str.replace` and the `.env` writer
slices `env_val[2:-1]` — so `${my_var}` IS substituted at runtime, and a template
that correctly documents `my_var=` was HARD-failed for a gap that does not exist.
`services/credential_charset.py` is the one place that decision now lives, named
for its ROLE (`CREDENTIAL_DETECTOR_CHARSET` — "the widest charset a detector must
accept so it is never narrower than the engine it audits"), not for a reach it
does not have. Four detectors adopt it; the docstring carries an explicit
NON-MEMBERS list with a reason per entry, because the previous framing ("the
charset every Trinity surface agrees on") is false and reads as an instruction to
the next engineer who greps `[A-Z][A-Z0-9_]*`:
* `mcp_validator._ENV_VAR_REF_RE` is a FAIL-CLOSED gate (`.mcp.json` inject →
400, `.credentials.enc` import, deploy-local), deliberately paired with the
WIDEST finder (`[^}]*`). Widening it admits input that is currently rejected.
* `skill_packaging.ENV_KEY_RE` is an adjacent domain with its own length cap.
* `static_checks._ASSIGN_RE` carries the quantifier shape behind an
already-FIXED py/polynomial-redos alert, on an agent-supplied-text path.
* `c_d006` is a different vocabulary that merely looks similar.
The constant lives in a pure-stdlib leaf module, NOT in `services/compatibility/`:
that package's `__init__` imports `database`, and `static_checks` imports
`template_service`, so a `template_service` → compatibility edge is a hard cycle
(reproduced: "cannot import name '_is_platform_injected' from partially
initialized module").
Behaviour changes, both named:
* K-001 (HARD) `fail → pass` for a documented lowercase variable — the fix.
* K-003 (SOFT) `pass → fail` for a lowercase-only, comment-free `.env.example`.
`_env_example_vars` is K-003's precondition for DEMANDING comments, so growing
it makes the verdict worse. The verdict is correct — that file genuinely has
no comments — but it is a `pass → fail` and is release-noted, not smuggled.
* S-010 (SOFT) does NOT flip: its `generic` blocklist is uppercase-exact, so no
newly-visible lowercase name can join it. Asserted, not assumed — it is safe
by coincidence of casing.
The two `template_service` extractors are included because both feed the LIVE
`collect_mcp_credential_warnings` → `deploy.py` path; leaving them out would
half-fix the very inconsistency this closes while a four-way agreement test
passed. Direction there is FEWER spurious warnings. `test_deploy_local_validation.py`
(the 8-assertion suite on that path) stays green.
Also hardens two latent crashes in the same file: a null / non-mapping
`template.yaml` document reaching `extract_credentials_from_template_yaml`, and
`extract_agent_credentials` reaching through `credentials:` at three levels. New
`credential_mcp_env_vars()` reader returns non-empty strings only, so an
`env_vars` element smuggled in as a mapping can never reach a consumer — that
element is exactly what turns a set comprehension into
`TypeError: unhashable type: 'dict'`.
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… could go dark
Two defects in the same HARD gate, one of them a way for the gate to stop
protecting entirely.
**1. It read the structure, not the declaration.** `listed` was
`set(creds.keys())` — `{"mcp_servers", "env_file"}` — so the documented structured
form `credentials.mcp_servers.stripe.env_vars: [STRIPE_API_KEY]` satisfied nothing
and HARD-failed a correctly declared template, while `${env_file}` and
`${mcp_servers}` PASSED. The admitted set was "whichever section names this
template happens to use", so the blind spot was template-dependent — the worst
kind, because it cannot be found by reading the check.
`declared_credential_names()` (the union of `mcp_servers.*.env_vars` and
`env_file`, over PR-A's tolerant readers) is now unioned in, and the three known
STRUCTURE keys are subtracted. A flat `credentials: {STRIPE_API_KEY: '...'}`
mapping is still admitted — that legacy shape is legitimate and keeps passing.
The section subtraction is a deliberate `pass → fail` for a genuinely broken
template. Shipped named, tested and release-noted, NOT smuggled under a
monotonicity claim: the blanket "strictly monotone, fail→pass only" claim is false
and a reviewer would find the counter-examples.
**2. It could go dark.** `run_static` caught `Exception` → `skipped`, and `_counts`
counted only `status == "fail"`, so a raise inside a HARD check DROPPED
`hard_count` and could flip `overall_status` from `issues` to `compatible` on an
agent with a genuinely undeclared credential. `c_k002` delegates to `c_t015`, so
ONE raise took both HARD gates dark together, and the result is indistinguishable
from a clean pass in the counts. The trigger is four lines of untrusted YAML:
credentials:
mcp_servers:
s:
env_vars:
- {STRIPE_SECRET_KEY: "please"}
`template.yaml` here is read from a live agent workspace, whose git repo the agent
itself owns — a self-attestation bypass on the surface whose job is to police it.
The same `TypeError: unhashable type: 'dict'` is the failure mode that argued
against enriching `credentials.env_file` in the first place, so reintroducing it
at the new call site would have been the plan diagnosing a bug and then shipping
it.
Three layers, deliberately:
* `c_t015` wraps ONLY the new term and degrades to the narrower set — which
makes `missing` LARGER, i.e. errs toward failing — never to `skipped`.
* `run_static` returns FAIL for a check that raises. A check that could not
evaluate is not a check that passed; one bad check still never breaks the
report.
* `_counts` also counts `skipped` + `skip_reason == "check_error"` as a finding,
at the sink (#1525), so the property survives a future path reintroducing the
skip. A benign precondition skip (`no_template`, `ai_not_run`) still counts as
nothing — that distinction is why the skip path exists.
`declared_credential_names` guarantees `str` elements structurally, and the call
site filters `isinstance(name, str)` anyway: the gate must not depend on the
reader's contract holding.
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ounts
Three catalog defects PR-A deferred, all in the two builders.
**Defect D — precedence was backwards.** `_build_local_template` read
`credential_mcp_server_names(credentials_block) or data.get("mcp_servers", [])`, so
a `credentials:` block silently OUTRANKED the template's own `mcp_servers:`
declaration. `agent_server/routers/info.py` has always read them in the other
order, so the catalog and the agent's own Info tab disagreed for any template
declaring both. Operands flipped; the `credentials:` path stays as the fallback.
**W14 — the GitHub builder had no fallback at all**, so a GitHub template declaring
only `credentials.mcp_servers` showed an empty list in the catalog while its Info
tab listed them. That was the third of three surfaces; all three now agree.
**Defect C / W6 — the badge.** Both builders read a flat top-level
`required_credentials:` key that ZERO templates declare — 25 bundled and all 7
configured GitHub repos — so `Templates.vue` rendered 0 for everything. Now derived
from the declared base set, with `platform_injected` vars EXCLUDED.
That exclusion is the badge's semantic, and it is not cosmetic: measured on the
real shipped catalog, a naive derivation is correct on 1 of 7 repos and wrong in
both directions — the ent#124 first-run agent would read 5 where the operator
supplies 2, while three shipped repos stay at 0. The chip is read as "how much work
is this to set up", so counting `GEMINI_API_KEY` / `GITHUB_PAT` / `TRINITY_*`
inflates it with rows nobody can fill. A consumer that wants every declared
variable wants `declared_credential_names`, not this.
Derived unconditionally rather than "explicit key wins, else derive": that override
branch is unreachable (no template declares the key), so keeping it would be one
dead code path guarding a live one.
No frontend change: `Templates.vue:103,107,171,175` read only `.length`, so the
shape it already expects is preserved and a variable name never reaches the DOM.
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…metadata Closes ent#128 AC #1-2. A template can now describe each credential an operator must supply — title, description, required, secret, format, setup_url, default — and `template_service` surfaces the normalized result as `credential_requirements` on every catalog entry. **Enrichment lives in a NEW sibling top-level key; `credentials:` is FROZEN as names-only, forever.** An already-deployed older Trinity reads `env_file` through `credential_env_file_names` and then does `agent_credentials.get(var_name, "")` — hand it a list of mappings and that is `TypeError: unhashable type: 'dict'` at the moment it writes the agent's `.env`. A sibling key is structurally invisible to that binary, so there is no floor version and enrichment distributes immediately. **Base-set-plus-overlay, so the two keys cannot drift.** One record per variable `credentials:` declares, decorated by `credential_setup:` entries joined BY NAME. An entry naming nothing is a named three-line error (problem, cause, FIX) and is dropped; valid siblings survive. `credential_setup:` can only ever decorate — the sibling-key shape's usual failure mode is closed by construction, not by discipline. Stated honestly: for an EXTERNAL template that error is neither impossible nor visible in the UI — `credential_errors` has zero frontend and zero MCP consumers, so the only human channel is the backend log. It is LOGGED. `required` is a tri-state. Enriched-and-omitted means `True` (an author who described a variable meant it); a legacy bare `- FOO` is `"unknown"`, never `True` — it carries no authorial intent, and reading it as required makes a guided checklist cry wolf. `"unknown"` doubles as the enriched/un-enriched discriminator, which is why no `enriched: false` flag is needed. `secret` defaults `True` (fail-safe). Path-free by construction, so trinity#570's `template.yaml` → `trinity.yaml` rename cannot reach it. **The normalizer never raises, and that is load-bearing.** `_build_template` runs in bare list comprehensions in `get_all_templates()`, OUTSIDE PR-A's per-template fence (which covers `_build_local_template` only) — a raise there is HTTP 500 with an EMPTY CATALOG, i.e. PR-A's exact bug reopened by the change that surfaces the new metadata. And no bomb is needed: `title: 123` or a bare `title:` was enough. So the builders ALSO wrap the call and degrade to `[]` plus a named error, rather than fencing the comprehension — that keeps the named error the resilience contract promises. The property does not rest on one function's discipline. (Which earned its keep immediately: the wrapper caught a real NameError during development instead of emptying the catalog.) Trust boundary — `title`/`description`/`setup_url`/`name`/`source` are author-controlled strings from arbitrary GitHub repos flowing into an operator-facing "paste your API key" checklist: * **Type-guard before touching.** Never `str()` a container from untrusted YAML: `str()` EXPANDS a shared alias during the walk (443 B → 52 MB in 1.5 s, x10 per level), and both the sanitizer and the record cap act after that cost is paid. * **Cap the INPUT**, entries AND errors AND the base set. Capping records while leaving `errors` uncapped built a 35 MB response out of the cap meant to prevent it; and `default` had no type row, so the 100-record cap acted as a x100 multiplier on it. * **`source` is sanitized** — it carries the raw MCP server name, the exact string `_sanitize_for_warning`'s own docstring names as the threat, and it was not on the list. * **Per-field length caps.** Reusing the 80-char terminal-warning default truncated a realistic 159-char description and made a real 90-char vendor console URL unusable. * **`setup_url` above scheme-only**: https (case-insensitive — `HTTPS://` is a legitimate author), a parseable host, NO userinfo (`https://google.com@evil.tld` renders as one host and resolves to another — the display/resolve split IS the attack), ≤2048, printable. Validate THEN sanitize, and never through a truncator. Residual documented, not claimed closed: `isprintable()` rejects RTL/ANSI but an IDN homograph survives, so a consumer must render the parsed hostname beside the link. * **Never mutates its input** — `_metadata_cache` holds the parsed dict for 600 s and YAML aliases genuinely share nodes, so one in-place normalize would rewrite both aliased fields and persist for ten minutes. Asserted against a deep-copy snapshot, including a real `&anchor`/`*alias` document. `credential_shape_errors` also gains the per-server and per-ELEMENT rows for `mcp_servers`, mirroring what `env_file` already had. The element row is the one that matters — an `env_vars` entry smuggled in as a mapping was the single most dangerous shape in the block and was unnamed. Note this makes the write path (`generate_credential_files` → 400) reject a template that previously created an agent with a garbage declaration: correct per PR-A's fail-loud write contract, and release-noted. `generate_credential_files` is deliberately UNTOUCHED — it still reads `env_file` names-only, which is what makes the forward-compatibility argument true rather than asserted. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n contract Closes ent#128 AC #3's machine-readable half. Follows the established `docs/schemas/` convention (`agent-pipeline.schema.json`): Draft 2020-12, date-stamped `$id` so a future revision keeps answering for templates written against this one, and self-described as the authoritative documentation contract while the backend reader stays deliberately tolerant. **Rooted at `template.yaml`, not at `credentials:`.** The two keys are ONE contract joined by a mandatory cross-reference, and validating either alone cannot check it. **`additionalProperties: true` at the root and on `credentials`** — template.yaml carries many keys this schema deliberately says nothing about, and a template predating the schema must stay VALID. Accepted asymmetry, and it is asserted as a test rather than left as a surprise: a made-up top-level key IS valid here. **`config_files` is enumerated and `deprecated: true`, not omitted.** The earlier posture was "don't delete, don't advertise", which made the authoritative contract answer VALID to `path: "/etc/cron.d/pwn"`. Undocumented is not a control against an author who knows the key — only against the reviewer who doesn't. So it is documented as deprecated, with a containment `pattern` that rejects absolute and `..` paths and a description saying plainly that it writes files into the agent's credential directory. Still reversible, still invalidates nobody. (Whether to DELETE the key is a public behaviour change and stays @vybe's call.) Carries the A2 consumer requirements in `$comment`, because the schema is the artifact a downstream implementer reads: * a record with `required: "unknown"` carries no authorial intent and MUST NOT be presented as a required field — without this a naive UI renders a seeded agent as five mandatory rows, three of them platform variables nobody can fill; * `platform_injected: true` MUST NOT be asked of an operator; * `secret: true` (the default) MUST be masked; * `setup_url` MUST be rendered with its parsed hostname shown, because the IDN homograph residual is real and documented rather than claimed closed; * there is intentionally NO reverse cross-reference requirement — a declared variable with no `credential_setup:` entry is normal. Also states the author cost honestly in the authoring note: declaring in `credentials:` is a separate edit from referencing `${VAR}` in `.mcp.json.template`, K-002 checks the two agree, and that is deliberate because `.mcp.json.template` must not become a second declaration authority. Plus the two brace forms Trinity's readers cannot see (`${my-key}`, `${VAR:-default}`). Tests pin the schema against the implementation — field caps, the format vocabulary, the allowed-key set, the record cap — so the reviewed text and the enforced text cannot drift. The 13 document cases run under `importorskip` (`jsonschema` is not a declared Trinity dependency); the security-relevant pattern assertions are unconditional. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nce examples Closes ent#128 AC #3-4. **Reference examples (AC #4).** The substrate the original plan targeted is gone — `3317247e` deleted `config/agent-templates/cornelius/` in favour of seeding from the public upstream repo — so AC #4 lands on what the bundle actually has: * `scout` / `sage` / `scribe` (the ent#124 seeded trio) declare an explicit `credentials: {}` with the zero-credential contract written out. Absent and empty mean the same thing to Trinity, but *absent* is ambiguous to a HUMAN — it could equally mean the author forgot. `{}` says "considered, and there are none", so the catalog's 0-credential badge is trustworthy. * `test-codex` carries the enriched reference: its one real variable gets a title, description, `required`, `secret`, `format` and `setup_url`. Deliberately NO `GEMINI_API_KEY` in any example: it is platform-injected, so an example asking for it would violate the very rule the guide documents — and it makes a K-002 fixture pass VACUOUSLY, which is how a test proves nothing while looking green. A test asserts no bundled example asks for a platform-injected var. Framed honestly rather than oversold: with one enriched declaration and one names-only one in the bundle, the parity test ("every bundled template normalizes with zero errors") is thin today. Its value is as a RATCHET for ent#137's curated fleet. **The guide (AC #3).** New `## Declaring Credentials` section, TOC renumbered 5→21. Covers the field table, the decorate-don't-declare rule with the actual error text, why `credentials:` stays names-only, the zero-credential contract, degrade-don't-demand, the platform-injected list, fork-to-own composition (ent#109), and the two brace forms Trinity's readers cannot see (`${my-key}` silently dropped, `${VAR:-default}` mis-substituted to an empty string). It also states the AUTHOR COST plainly instead of claiming the design is free: declaring a variable is a separate edit from referencing it in `.mcp.json.template`, and three of Trinity's own six default GitHub templates declare zero credentials while referencing 2-6 and documenting 7-12. Those are K-002-red today and stay red until someone does the edit. Kept that way on purpose — if `.mcp.json.template` counted as a declaration it would become a second authority on what an agent needs, which is the drift this design exists to prevent. The practical order is stated: seed `credentials:` first, enrich second. **Memory docs.** `requirements/credentials.md` §3.5's ✅ was false in both halves and is corrected in place with the correction recorded: the extractor it credited has no production caller, and nothing showed configured-vs-missing status because the badge read a key no template defines. `template-processing.md` and `templates-page.md` get the "two shapes, two owners" table that reconciles the objects-vs-strings contradiction (catalog `required_credentials` = names, `credential_requirements` = objects, extractor `required_credentials` = a different function with the same key name), plus the corrected regex. Compatibility checklist gains six credential rows and a starts-with-nothing-configured row. **No DB change → Rule #9 (dual-track migration) does not apply.** Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A transition diff over a corpus with zero coverage of the diff is not evidence, so
the changed statements were measured against PR-B's real base (`c07afab7` =
origin/dev + PR-A) rather than assumed. The gate found the new paths that no test
reached and this closes them:
* §4's new `mcp_servers` shape-error rows — per-server AND per-element, six
parametrized cases plus the sanitized-server-name case. The element row is the
dangerous one and it had no test.
* The write-path consequence, asserted explicitly: `generate_credential_files`
now raises on `env_vars: [{K: v}]`, where before it created the agent silently.
* `_setup_url_error`'s `urlsplit` ValueError branch (malformed IPv6 literal).
* The dedup early-return in the base-record builder — a variable declared under
two servers AND `env_file` yields one record with a stable `source`.
* A non-string mapping key in a descriptor (`{1: "x"}`), which must not reach the
"did you mean" helper.
* The caller-less `extract_agent_credentials` across eight malformed shapes. It
has no production caller, which makes hardening cheap rather than unnecessary —
the next caller would have inherited the crashes. Now exercised instead of
merely present.
Result: 227 changed statements, 225 executed. The two remaining are a defensive
`except OSError` around a `Path.resolve()`, and the three gate files
(`static_checks.py`, `compatibility/__init__.py`, `credential_charset.py`) are at
100% of changed statements.
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`/sync-feature-flows`. `template-processing.md` and `templates-page.md` were already
updated with the declaration standard; this adds the flow the code change actually
lands hardest on and which nothing had touched: `agent-compatibility-validation.md`.
Both credential HARD gates changed, and the flow doc described neither the defect
nor the new semantics:
* "a detector must never read narrower than the mechanism it audits" — the shared
root cause of K-001 and K-002/T-015, with the NON-MEMBERS list spelled out so
the next reader does not "align all the regexes" and widen
`mcp_validator._ENV_VAR_REF_RE`, which is a fail-closed GATE and not a detector;
* "a HARD gate must not be able to go dark" — the `run_static` →`skipped` +
`_counts`-counts-only-`fail` interaction that let 4 lines of untrusted YAML drop
`hard_count` 1→0, and the three fail-closed layers that replace it;
* the complete verdict-transition set, because the blanket "strictly monotone"
claim is false and a reader will find K-003's `pass→fail`. The claim that
survives is "no agent gains a HARD failure".
Testing section records why the bundled templates cannot prove any of this — 0
`.mcp.json.template` and 0 `.env.example` files, so every changed check
short-circuits before reaching changed code and a green diff there is
green-because-vacuous — and points at the 49-fixture synthetic corpus instead.
Plus the Recent Updates row in `feature-flows.md` (the step this skill's own docs
warn gets skipped).
Observation, deliberately NOT fixed here: the Recent Updates table carries 57 rows
against its own documented "newest ~20" cap (#1360), so the index is 434 lines vs
the 400-line guideline. Trimming it means deleting 37 other engineers' entries,
which is not this PR's call.
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`sk-live-xxx` is stripe-shaped and gitleaks' default ruleset covers `sk-`. The value is arbitrary in this test — it only has to round-trip byte-identically through the `.env` writer — so there is no reason to hand CI a secret-shaped string to reason about. Refs Abilityai/trinity-enterprise#128 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Real regression I introduced in `886aab5b` and initially mis-attributed as
pre-existing. Recording both the fix and how the mis-attribution happened, because
the second part is the reusable lesson.
**The bug.** `test_1484_create_agent_characterization.py` and
`test_1759_local_template_not_found.py` MagicMock the whole
`services.template_service` module and stub each function crud actually calls with a
faithful return value (`generate_credential_files` → `{}`, `get_github_template` →
`None`). `_resolve_local_template` now calls a THIRD one —
`credential_mcp_server_names` — and it was unstubbed, so it returned a truthy Mock
that passed `if mcp_servers:`, landed in `config.mcp_servers`, and blew up later
inside a `yaml.dump` as `ValueError: dictionary update sequence element #0 has
length 1; 2 is required`. 18 tests, entirely a harness gap: in production the real
function returns a list.
Stubbed with a faithful 3-line mirror rather than a fixed `[]`, so a fixture that
DOES declare `credentials:` cannot be silently masked by the stub.
**One test needed a real update, not a stub.**
`test_malformed_field_still_creates_and_names_the_template` used
`credentials: "a string"` as its trigger for the broad-except degrade path. That is
exactly what `886aab5b` fixes — `credentials:` is no longer a trigger BY DESIGN,
because it raised FIRST in that run of mutations and so cost the agent its
`runtime:` and `shared_folders:` config as collateral. Swapped the trigger to
`shared_folders: not-a-mapping`, which still raises, so the degrade path and the
two identifiers in its warning stay under test. The docstring records why and points
at the new coverage.
**How I mis-attributed it.** I compared with `git stash push -- src/backend`, which
reverts only the WORKING TREE — commits 1 and 2 were already committed, so my
"baseline" still contained the cause and the failures looked identical on both
sides. The `-k`-filtered selection also happened to include only 1 of the 13
`test_1484` failures, which made the set look small and stable. Only a worktree at
`c07afab7` (PR-A's tip, PR-B absent) showed the truth: 2 failures there vs 20 on the
branch. **A baseline has to be a worktree at the base commit, not a stash.**
Now identical to `origin/dev` and to `c07afab7`: 2 failures, both genuinely
pre-existing (`test_agent_analytics::test_day_stacks_present_in_by_type`,
`test_1069_voip_call_path_param` — the documented `get_flat_dependant` venv drift).
Refs Abilityai/trinity-enterprise#128
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two holes in the "never raises, never amplifies" property PR-B rests on, both found by asking which OTHER producers reach the surface the new cap protects. 1. `credential_shape_errors` was uncapped. The cap shipped on the NEW function (`normalize_credential_requirements`), but the same PR added a per-ELEMENT loop to this PRE-EXISTING one, and it feeds the same two surfaces: the catalog's `credential_errors`, and the `"; ".join(errors)` that becomes `CredentialDeclarationError`'s agent-creation 400 body. A cap is a property of the producer, not of the PR that invented the concept. YAML anchors make input size a useless proxy for output size, so the bound has to stop the WALK, not slice the result. Measured on a 6,738-byte `template.yaml` (one 200-element anchor aliased across 200 servers): 40,000 errors / 3.64 MB joined (540x) before, 101 errors / 8,973 bytes after. `origin/dev` returns 0 on the same input, so the amplification is this branch's own — reachable since ent#123 by any creator-role user pointing at an arbitrary public repo. 2. `source_trust not in _SOURCE_TRUST_LEVELS` is frozenset membership, so an UNHASHABLE value raised `TypeError` *on the guard line* — before the degrade-to-`github` branch that guard exists to reach. Unreachable from parsed YAML today (every call site passes a literal), but this is the one function whose docstring makes "NEVER RAISES" load-bearing: a raise here is an empty catalog and a dark HARD gate. The property should be literally true, not true-by-call-site-audit. Both regression tests were confirmed to FAIL with their fix reverted and pass with it restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The unit suite was validating against a FastAPI ~25 minor versions ahead of
the one production ships. `docker/backend/Dockerfile` pins `fastapi==0.115.6`
exactly; `tests/requirements-test.txt` carried an unbounded floor that resolved
0.140.13. The comment at :43 already claimed these "match the floors set in
docker/backend/Dockerfile" — that file uses exact pins, so the claim was untrue.
Surfaced as `test_1069_voip_call_path_param` failing with `ImportError: cannot
import name 'get_flat_dependant'`. That test is only the messenger: it is the
one test coupled to a private FastAPI symbol (`src/backend` imports none, and
the other test touching `fastapi.routing` uses the public `APIRoute`).
The obvious ceiling does not work: `0.140.13 < 0.141` is true, so `<0.141`
still admits the breaking version. Bisected against the real wheels — present
in 0.140.6, gone in 0.140.7 — a private API dropped in a PATCH release, so no
minor-level bound is trustworthy. Tracking prod's line is the durable fix.
Why now rather than "separate follow-up": CI is green only on a warm pip cache.
backend-unit-test.yml keys `cache-dependency-path` on this file, and 0.140.13
allows py3.11, so the next edit to this file for ANY reason busts the key,
re-resolves, and breaks CI for everyone. Capping is the safe way to bust that
cache — the change that invalidates the key is the one that makes re-resolution
correct.
Follows this file's own precedent (`bcrypt>=4.2.0,<5`, added when bcrypt 5.0.0
removed the `__about__` shim passlib reads): floor + ceiling + a comment saying
why, rather than an exact pin that would break the file's `>=` convention.
Verified by execution, not argument:
- full tests/unit at 0.115.14: 5861 passed, 16 skipped, 2 xfailed, 0 failed
(at 0.140.13 the same command is 1 failed, 5860 passed)
- the edited file installs clean in a fresh venv and resolves 0.115.14
- an existing verify venv self-heals: pip downgrades 0.140.13 -> 0.115.14
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ted path
`_build_local_template` derived `is_bundled` itself:
is_bundled = template_dir.resolve().parent == _local_templates_dir().resolve()
`template_dir` on the by-id path is `_local_templates_dir() / name` where `name`
comes from a user-supplied `local:<name>` template id, so this called `.resolve()`
on attacker-influenced input. CodeQL flagged it as `py/path-injection` (alert 260,
high) — a new tainted-path sink introduced by ent#128 purely to pick a log level
(`source_trust` selects `logger.warning` vs `logger.info` and nothing else).
`is_bundled` is now a required keyword arg supplied by whoever knows the
provenance:
- `get_local_templates()` iterates the curated root, so its children are
bundled by construction -> `is_bundled=True`.
- `get_local_template()` decides from the id STRING (plain single segment, no
separator, not a dot-segment) rather than a path operation on it.
Behaviour, measured against the old predicate across 9 ids: 7 identical, 2
divergent — `'../agent-templates/sage'` and `'a\b'` go True -> False. Both moves
are old=True -> new=False, i.e. strictly more conservative: the new check never
grants the `bundled` label where the old one withheld it, only the reverse. An id
that traverses to arrive inside the curated root is not curated, so the new
answer is also the more correct one; the blast radius either way is one log
level.
This is deliberately NOT a traversal guard. `name` still reaches the filesystem
exactly as it did before ent#128 — `get_local_template` is byte-identical to dev
and untouched here. That pre-existing traversal is real (`local:..` escapes the
templates dir, reachable by any authenticated user via
`GET /api/templates/{id:path}`) and is being routed as its own issue rather than
fixed inside a credential-declaration PR or silently dismissed.
Two test call sites updated for the new signature.
Verified: full tests/unit 5905 passed / 0 failed; no `template_dir.resolve()`
remains in the module.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndriiPasternak31
force-pushed
the
feature/ent128b-credential-declaration-standard
branch
from
July 31, 2026 14:10
5c0f712 to
e46db13
Compare
AndriiPasternak31
marked this pull request as ready for review
July 31, 2026 14:48
Contributor
Author
|
Un-drafted per @vybe's call on ent#128. Rebased onto
Two timings looked too good, so I checked rather than trusting the banner:
On the CI blip: Worth flagging separately: the suite is now 6199 tests, a normal shard is ~10m, and the cap is 25m — a 2.5× slow runner lands exactly on the ceiling. Not this PR's doing, but that margin is thinning. Release-note requirement: agents only pick up the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ent#128 PR-B — the credential declaration standard
Closes AC #1–4 of abilityai/trinity-enterprise#128. AC #5 already shipped in #1835 (PR-A).
Cross-repo
Fixesdoes not fire — the issue is referenced, not closed. Close it manually at the release cut, and setstatus-in-devby hand (the same-repo automation won't promote it).Baseline: rebased onto
origin/dev@553382c8(2026-07-31), which contains PR-A (8d59b6c8, #1835) — so this diff isolates PR-B cleanly.Rebase resolved two doc conflicts by conceding dev: ent#263 renamed
feature-flows/templates-page.md→library-page.mdand rewrote it (477 → 168 lines), dropping therequired_credentialsblocks my annotations targeted — the three-way shape table survives intemplate-processing.md:183-185, so nothing was lost. Index +learnings.mdentries merged in date order.Size: 25 files, +3390/−61 — but that splits as 922 lines of production code, 1993 of tests, 418 of docs. The single largest file in the diff is a test file.
✅ Decisions — all resolved by @vybe
Ruling on ent#128. No vetoes; nothing outstanding.
required(unknownfor legacy bare names) — that's what ent#137 propagatesconfig_filesdeprecatedin the schema. Don't delete heretrinity-enterprise— documents a working amplification attack against live installs; nothing public until the platform-wide caps ship${VAR:-default}type-bug, P1 — breaks the seeded first-run agentThe path-traversal CodeQL surfaced (pre-existing, not introduced here) is #1900.
Behaviour changes to look for (rather than trip over)
Two HARD gates change verdicts — these were live false-positives
c_t015compared${VAR}s against the credentials block's section names, never the variable names. A template whose${VAR}happened to match a section name passed vacuously; a correctly-declared variable could fail._env_example_vars' uppercase-only filter could not see a documented lowercase variable, so it reported it as undocumented.Neither needed a spec change:
docs/agent-validation-spec.mdalready describes the intended behaviour ("is listed intemplate.yamlcredentials schema"). The code was wrong, not the spec.I want to be explicit because an earlier version of this write-up claimed "only fail→pass", and that is measured false. Across 54 fixtures × 66 static checks (
origin/dev@39f29c64vs this branch) there are 60 transitions in three classes:fail → passskipped(check_error) → failrun_staticfailing closed. All 27 on one fixture (60k-deep-nesting), whosehard_countgoes 7 → 15, i.e. UPpass → failThe claim I am defending is not "only fail→pass". It is narrower and checkable:
run_staticon either tree.skipped/check_error).hard_countdropped traces line-by-line to an intendedfail→pass— none to afail→skipped. That is the property that matters: no check went quiet to buy a green number.Correction to my own brief: the 6
pass→failsplit is 2 HARD + 4 SOFT, not "4 HARD, 2 SOFT". Verified againstspec.py(K-002: hard,K-001: hard,T-015: soft,K-003: soft) and cross-checked against the per-fixture hard/soft deltas:var-is-section-env_fileandvar-is-section-mcp_servers— K-002 (HARD) + T-015 (SOFT) each,hard 8→9,soft 12→13. These are the section-name bug in the opposite direction: a${VAR}that collides with a section name used to pass vacuously and now correctly fails. Same root cause as the 27fail→pass.env_example-generic-lowercase,env_example-lowercase-only-no-comments— K-003 (SOFT),soft →+1. K-001 now sees lowercase vars, so K-003 ("comments explain each variable") finally has them to evaluate and correctly reports the undocumented ones.Evidence + rerunnable harness in
.plan/ent128b-transition-evidence/(git-excluded — it does not travel with the branch, which is why the numbers are pasted here).Smaller behaviour changes
required_credentials:key that no template defines. Release-note it.mcp_servers:andcredentials.mcp_serversnow report the former. This matches whatinfo.pyalready did..env.examplevars are now extracted. No production caller —extract_agent_credentialsis dead code today (V4).docker/base-image/**changed (info.py), so this needs a base-image rebuild to reach agents.Two commits landed after the plan was written
4af7973→4b84e9f— the tolerant credential reader must not blow up or raiseTwo holes in the "never raises, never amplifies" property:
(a)
credential_shape_errorswas uncapped while its siblingnormalize_credential_requirementsgot the cap — and both feed the same two surfaces (the catalog'scredential_errors, and the"; ".join(errors)that becomes the agent-creation 400 body).Measured on a 6,738-byte template with one 200-element anchor aliased across 200 servers (re-verified on this rebased branch, not copied forward):
origin/dev@39f29c64origin/devreturns 0, so the amplification is this branch's own — introduced and closed within the PR. The cap bounds the walk, not just the output; a cap applied after the loops would still have built and joined every string.(b)
source_trust not in _SOURCE_TRUST_LEVELSis a frozenset membership test, so an unhashable value raisedTypeErroron the guard line itself. Verified as shipped:list,dict,set,str,Noneall pass through and fall back togithubwith a warning.Both regression tests were confirmed to fail with the fix reverted.
db242ce→c9b74b5— cap fastapi to prod's 0.115.x lineOut of scope for a credential PR, flagged rather than let ride in silently. @vybe ruled: "One PR is fine, don't split" — so it stays.
The case for keeping it: prod pins
fastapi==0.115.6exactly, while the test env floated to0.140.13— ~25 minors ahead, i.e. the unit suite was validating against a different FastAPI than the one that runs. CI is green only on a warm pip cache keyed ontests/requirements-test.txt, so the next edit to that file for any reason busts the key, re-resolves, and breaks CI for everyone. Capping is the safe way to bust that cache — the change that invalidates the key is the one that makes re-resolution correct.Note
<0.141does not work (0.140.13 < 0.141is true). The symbol died in0.140.7— a PATCH release — so no minor-level bound is trustworthy.CodeQL — resolved, and what it turned up
The first push tripped 1 new HIGH
py/path-injection(alert 260). Disposition, since it is worth understanding rather than skimming:The flagged line was mine, but it performed no I/O — it was a pure
==comparison used to pick a trust label (source_trustselectslogger.warningvslogger.infoand nothing else). It still called.resolve()on a path built from a user-supplied template id, which is a genuine tainted-path sink, added to decide a log level.Fixed in
5c0f712fby deleting the sink, not by suppressing the alert.is_bundledis now a required kwarg from whoever knows the provenance: the catalog scan iterates the curated root (bundled by construction), and the by-id lookup decides from the id string. Measured against the old predicate across 9 ids: 7 identical, 2 divergent ('../agent-templates/sage','a\b'), bothTrue → False— strictly more conservative, never over-grantingbundled.What it turned up is the part worth your attention. Tracing the taint to its source found a pre-existing traversal in
get_local_template(byte-identical ondev, not touched by ent#128):_local_templates_dir() / namewith no containment check, reachable by any authenticated user viaGET /api/templates/{id:path}. Verified thatlocal:..escapes the templates root.That is not fixed here — filed as #1900 rather than folded into a credential-declaration PR or dismissed. Nothing was dismissed.
Verification
Run on the rebased branch, at the capped FastAPI:
tests/unit/test_ent128{a,b1,b2}*.py→ 203 passed (b1 = 39, b2 = 131, a = 33)tests/unit→ 5905 passed, 16 skipped, 1 xfailed, 0 failed (6m09s).env, no credential files. Two grep hits are intentional test fixtures:https://google.com@evil.tld(thesetup_urluserinfo-phishing validator's own test input) andSTRIPE_API_KEY: 'from the vault'(the deliberate unambiguous placeholder).credential_charset.pylives underservices/, whichdocker/backend/Dockerfilecopies wholesale — no COPY-list gap (bug: backend image missing redis_breaker_util.py — Dockerfile COPY not added for new top-level module (#986) #1033 class). No newos.getenv()→ no compose/.env.examplegap (feat(voice): VoIP telephony — agents place/receive phone calls over Gemini Live #1056 class).Not run:
/verify-local.docker/base-image/**is touched, so a base-image rebuild + live agent exercise is the right gate before release.Honest framing
credential_setup:key rather than by extendingcredentials:— because an old Trinity iterating an enrichedenv_fileraisesTypeError: unhashable type: 'dict'. Same outcome, better compatibility. (Posted as an issue comment so the AC isn't read as unmet at close.)credentials:is frozen, names-only, forever.credential_setup:decorates it and can never declare a variablecredentials:does not — base-set-plus-overlay, joined by name, so the pair cannot drift.origin/devand pass here — the proof they encode real defects rather than being written to fit the implementation. Measured, not asserted: copyingtest_ent128b1_compat_gates.pyonto a pristineorigin/devworktree gives 21 failed / 18 passed; on this branch all 39 pass. The failures are exactly the named defects (test_k002_no_longer_passes_a_reference_to_a_section_name[env_file|mcp_servers|config_files],test_a_raising_check_is_a_failure_not_a_skip,test_check_error_cannot_erase_a_hard_finding, …).test-codex); the other three bundled templates declarecredentials: {}. So the parity test guards ent#137's curated fleet going forward — it does not prove breadth today. Not overselling it.credential_errors+ the compatibility check). Trinity does not gate a third-party repo's CI.normalize_credential_requirementsnever raises and never mutates —_build_templateruns unfenced insideget_all_templates(), so a raise there empties the whole catalog. That is the property (a) above exists to keep literally true rather than true-by-call-site-audit.🤖 Generated with Claude Code