Skip to content

feat(assignments): go live — un-fence get_agent_assignments, fail-closed consent, gitlink bump (abilityai/trinity-enterprise#500) - #3034

Merged
AndriiPasternak31 merged 10 commits into
devfrom
AndriiPasternak31/ent500-pr3
Oct 2, 2026
Merged

AndriiPasternak31 merged 10 commits into
devfrom
AndriiPasternak31/ent500-pr3

Conversation

@AndriiPasternak31

@AndriiPasternak31 AndriiPasternak31 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Ready. trinity-enterprise#669 is merged (be1160f3a) and the src/backend/enterprise gitlink now points at it (0c0a99de9), so the un-fence and the module go live together. A one-line follow-up, trinity-enterprise#750 (consent join folds email case, dolho's #669 note), will need one more gitlink bump once it merges; it is not required for this PR.

Why this PR exists

PR1 (#2596) shipped the OSS half of role assignments. PR2a (trinity-enterprise#669) ships the
private module behind it. This PR is the go-live gate, not #669's merge. It does five things:

  1. fixes dolho's [I1] from the feat(assignments): the OSS seam and the execution-context fields — who fills which role #2596 approval, which he asked for "before the gitlink bump
    that makes this live"
  2. reverses the 0.9.5 fence (fix(mcp): fence get_agent_assignments — built, not registered, for the 0.9.5 cut (F1) #2863) so get_agent_assignments is registered again
  3. takes [I2] and [I3] from the same review, since this PR already touches assignments.ts
  4. closes [R1], the same fail-open as [I1] one layer earlier: the provider seam coerced consent
    with bool(), so a provider answering the string "false" rendered "proactive contact permitted"
  5. bumps the enterprise gitlink to MCP get_fleet_health returns 500 while all per-agent get_agent_health calls return 200 #669's merge commit

Merge order (satisfied): the un-fence ships with the gitlink bump

Commit 2 advertises get_agent_assignments to user/agent/system keys again. On any build
without #669, the route does not exist, so the tool always degrades to "not available" (a
uniform 404). That is harmless but wrong, and it is the reason the tool was fenced for 0.9.5.
The un-fence and the bump go live together, in this PR, in that order.

Commits

# Commit What
1 fix(assignments): unresolved proactive consent fails closed [I1]
2 feat(mcp): register get_agent_assignments again — reverse the 0.9.5 fence (#2863) un-fence
3 fix(mcp): get_agent_assignments never echoes the raw error text [I3]
4 fix(mcp): get_agent_assignments enforces agent self-scope in the tool [I2]
5 fix(assignments): a non-bool consent is unresolved, never coerced [R1]
6 chore(enterprise): bump the private module to trinity-enterprise#669 gitlink → be1160f3a

[I1] Consent fails closed (required)

platform_prompt_service.py::_render_assignment appended the consent qualifier only
if ctx.proactive_consent is not None. _validated skips absent keys, so a provider answering
{primary_user_display, role_id} and nothing else is a valid answer, and it rendered a bare
name
. The function's own docstring forbids exactly that inference: a name with no qualifier
reads as permission to reach out.

Now only an explicit True renders "proactive contact permitted". False and None both render
the restrictive clause.

The second consumer (compose_system_prompt, the proactive_consent= merge, ~:1015):
left unchanged, on purpose.
It keeps a caller-supplied value, otherwise it takes the
provider's, and an unresolved None passes through as None. The renderer now closes that
case, so the fail-open lived only in the renderer. The route dolho flagged, where the name and
the consent bit come from different places, only exists when a caller pre-fills consent and
nothing else. No src/ caller pre-fills any assignment field (grepped), and a caller that
explicitly passes True has made the grant itself.

Tests (tests/unit/test_ent500_execution_context_fields.py) use a non-ambient stub: the display
name and role are supplied and consent is omitted, through both build_execution_context and
the real compose_system_prompt → provider path. Both were red before the fix (the line
rendered … (role: finance-controller) with no clause) and green after.
test_unknown_consent_renders_no_qualifier_either_way pinned the fail-open behaviour and was
replaced. test_display_name_is_still_bounded now measures the name rather than the whole line,
because the line always carries the clause now.

Un-fence (#2863 reversed)

Restores the createAssignmentTools import and its toolGroups entry in server.ts, the
access.ts policy row, the presence pins in tool-visibility.test.ts, and #2863's doc edits
(docs/memory/architecture/mcp-server.md, docs/memory/feature-flows/role-assignments.md,
docs/user-docs/integrations/mcp-server.md). The text is restored verbatim from gh pr diff 2863.
server.ts and the user-doc row had drifted since #2863, so those two were re-applied by hand to
the same text. The test flip went first: with the pins flipped and the registration still fenced,
the suite was 503/504, red. After restoring the registration it was 504/504.

The user-doc header goes back to #2863's pre-image (130 tools / 125 operator). I did not
recount tools that later PRs added (search_executions, metrics, teardown), so that header may be
stale for reasons unrelated to this PR.

[I3] No raw error text reaches the agent

degrade() returned detail: error.message next to the curated message. On the
unexpected-status branch, that is a raw backend body or an internal URL handed to a model. The
field is dropped on every branch, not just capped. The curated per-status sentence is the
whole answer: {enabled: false, agent_name, message}. Tests throw an error that carries a fake
internal URL through 502, 404, 403 and a transport error, and assert that no detail key appears
and the text is never echoed (4 red → green).

[I2] Self-scope enforced in the tool

Of the two options (enforce it, or reword the description), I chose enforce it. The
description promises "an agent may only read its own roster", and the only thing that kept that
promise was a route in a private repo. a2a_call.ts's checkSelf is the direct precedent,
deliberately redundant with its backend. McpAuthContext.agentName was already available.

  • an agent key naming another agent → {success:false, not_authorized:true} before any
    backend call
    , audited as a denial through accessDenied (bug(mcp): a denied chat_with_agent / fan_out call is audited as a successful tool call #2807)
  • an agent key with no agent name → refused (fails closed, matching access.ts's
    "a key row with no agent name is denied")
  • user / system keys → passed to the backend, which scopes them by owner
  • the access.ts row moves from baselined (ENT629) to in-tool

The refusal does no lookup, so it is identical for existing and non-existing names, and it adds
no enumeration signal.

[R1] A non-bool consent is unresolved, never coerced

Found in the pre-push review of this PR. assignment_provider._validated did
out["proactive_consent"] = bool(value), and bool("false") is True. A provider that answered
the string "false" (or "0", "no") therefore rendered "proactive contact permitted":
the same permission-fails-open class as [I1], one layer before the renderer.

Now only a real bool is kept. Anything else is dropped with a WARNING, like every other
malformed field, so consent is unresolved and [I1]'s path renders the restrictive clause.
test_proactive_consent_is_coerced_to_bool pinned the coercion (1 → True) and is replaced by
test_a_real_bool_consent_is_kept (True/False) and test_a_non_bool_consent_is_unresolved
("false", "False", "0", "no", "true", 1, 0). The end-to-end test
test_a_provider_answering_string_false_renders_the_restrictive_clause drives the real
compose_system_prompt → provider path. All 8 new cases were red against bool() (the line
read … (role: finance-controller) — proactive contact permitted) and green after. Reverting the
fix again on the final tip turns the same 8 red. The feature flow's seam section, failure table
and testing notes say this now.

Test evidence (local)

Rebased onto origin/dev @ ebc2bfb4f on 2026-09-27 (24 new dev commits, no conflicts).

Check Result
src/mcp-server npx tsc --noEmit clean (exit 0)
src/mcp-server npm test dev baseline 550/550 → 559/559 (+9: 4 [I3], 5 [I2]). 10 repeat runs: 0 failures
tests/unit/test_ent500_*.py (5 files), pytest-randomly seeds 12345 / 67890 / 99999 81 passed on each (72 before R1; −1 replaced, +9 new)
test_platform_prompt_{unit,runtime}.py 48 passed
Every unit file that touches the seam or compose_system_prompt (13 files), seed 12345 435 passed, 1 skipped
Each commit on its own (ent500 + platform_prompt pytest, npm test, tsc) green at all 5 commits
Fix reverted on the final tip, tests re-run [I1] → 3 fail (its 2 + R1's end-to-end test, which rides the [I1] path); [I3] → 4 fail; [I2] → 2 fail; [R1] → 8 fail
  • The earlier access-wiring.test.ts "bug(mcp): a denied chat_with_agent / fan_out call is audited as a successful tool call #2807: a refused loop start is audited as a refusal" flake
    did not reproduce in 10 runs. dev has since landed bug(tests): mcp-server access-wiring "#2807 refused loop is audited as a refusal" reads the previous test's late fire-and-forget audit row — flaky on dev alone #2952 (each loop-tool call identity-checks
    its own audit row), which targets that test. Not claimed as fixed, just not seen.
  • The enterprise-docs-guard grep, run exactly as CI runs it over docs/, CLAUDE.md and the
    seam files (assignment_provider.py is one): 0 hits (positive control: it matches a
    made-up private table name).
  • /validate-pr checklist done by hand before push (the skill reads the PR from GitHub). Lane B
    (no auth/credential/CI/image/submodule path in the diff yet; the gitlink commit will move it to
    Lane C), no schema. Secret, email, IP, env-var and host-path greps are clean. The 10 deleted
    test lines are the two tests that pinned the fail-open behaviour ([I1]'s
    test_unknown_consent_renders_no_qualifier_either_way and [R1]'s
    test_proactive_consent_is_coerced_to_bool), each replaced by a stricter one. Feature flow
    role-assignments.md is updated and the index has a Recent Updates row. No requirements change,
    since no new capability: a bug fix plus restoring a documented tool.

Before merge (checklist)

  • trinity-enterprise#669 merged (be1160f3a)
  • gitlink bumped to MCP get_fleet_health returns 500 while all per-agent get_agent_health calls return 200 #669's merge SHA (0c0a99de9)
  • python3 scripts/ci/check_alembic_heads.py src/backend/enterprise/backend/migrations/versions
    reports one head (19 revisions, 1 head: 0018_agent_assignments)
  • git ls-tree origin/AndriiPasternak31/ent500-pr3 src/backend/enterprise shows the new SHA
    (ignore=all hides a gitlink change from status and diff)
  • /validate-pr green on the real PR (author run + dolho's approving run) (the hand-run checklist above is not a substitute)
  • review by dolho — APPROVED 2026-10-02 (wrote I1/I2/I3; R1 is the same class as I1). No self-merge (SOC 2).

Refs abilityai/trinity-enterprise#500 · #2596 · #2863 · abilityai/trinity-enterprise#669

🤖 Generated with Claude Code

…enterprise#500 [I1])

_render_assignment appended the consent qualifier only when
proactive_consent was not None, so a provider answering
{primary_user_display, role_id} and nothing else -- a valid answer,
since _validated skips absent keys -- rendered a bare name. That is the
inference the function's own docstring forbids: a name with no
qualifier reads as permission to reach out.

Only an explicit True now renders "permitted"; False and None both
render the restrictive clause. The compose-time merge in
compose_system_prompt is left as is: it preserves None, which the
renderer now closes, and no src/ caller pre-fills the fields.

dolho's go-live condition from the #2596 review.
…fence (#2863) (trinity-enterprise#500)

#2863 withheld the tool from toolGroups for the 0.9.5 cut because the
assignments layer had no backend. PR2a (trinity-enterprise#669) ships
that layer, so the registration, the access.ts policy row and the
presence pins in tool-visibility.test.ts return, along with the doc
text #2863 replaced (restored verbatim from its diff).

Must not merge before the enterprise gitlink bump: on a build without
#669 the tool is advertised but always answers "not available".
…ity-enterprise#500 [I3])

degrade() returned `detail: error.message` beside the curated message.
On the unexpected-status branch that is a raw backend body or an
internal URL, handed unfiltered to a model. Drop the field on every
branch; the curated per-status sentence is the whole answer.

dolho's [I3] from the #2596 review.
… (trinity-enterprise#500 [I2])

The tool description promises "an agent may only read its own roster",
but only the backend route -- in the private module -- enforced it. An
agent key naming another agent, or carrying no agent name, is now
refused with not_authorized before any backend call and audited as a
denial via accessDenied. Deliberately redundant with the backend, like
a2a_call.ts checkSelf. user and system keys still pass to the backend.

The access.ts row moves from baselined to in-tool. The I3 doc note for
the degraded shape lands here too, in the same flow section.

dolho's [I2] from the #2596 review.
…inity-enterprise#500 [R1])

assignment_provider._validated coerced proactive_consent with bool(value),
and bool("false") is True -- so a provider answering the string "false"
(or "0", "no") rendered "proactive contact permitted". The same
permission-fails-open class as [I1], one layer earlier.

Only a real bool is kept now. Anything else is dropped with a WARNING
like any malformed field, so consent is unresolved and the renderer's
[I1] path states the restrictive clause.

test_proactive_consent_is_coerced_to_bool pinned the coercion and is
replaced. New tests were red against bool() (8 failed) and green after;
the end-to-end one drives the real compose_system_prompt path with a
provider answering "false".

Found in the PR3 pre-push review.
@github-actions

github-actions Bot commented Sep 29, 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.

@github-actions

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.

…00-pr3

# Conflicts:
#	docs/user-docs/integrations/mcp-server.md
…00-pr3

# Conflicts:
#	docs/memory/feature-flows.md
…00-pr3

Conflicts in src/mcp-server/src/access.ts and src/mcp-server/src/server.ts:
dev added the ent#661 projects tools (createProjectTools + their policy
rows) next to the 0.9.5 assignments fence comments. Keep dev's projects
import, registration and rows; replace the fence comments with this PR's
un-fenced createAssignmentTools import/registration and the in-tool
get_agent_assignments row.
…bilityai/trinity-enterprise#500)

Points src/backend/enterprise at be1160f3a, the squash-merge of the
assignments record and prompt (#669) on trinity-enterprise main. Adds
migration 0018_agent_assignments; one enterprise Alembic head.
@AndriiPasternak31
AndriiPasternak31 marked this pull request as ready for review October 2, 2026 09:23

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

PR Validation Report (/validate-pr)

Lane C (src/backend/enterprise gitlink), no schema change in the OSS tree.

Category Status Notes
Base branch ✅ dev
Size ✅ 14 files (+286/−66)
Commits ✅ conventional, each maps to one review item (I1/I2/I3/R1, un-fence, bump)
Issue link / auto-promotion ⚠️ Refs abilityai/trinity-enterprise#500, no closing keyword. That's fine for a multi-PR epic, but whoever closes it has to bump status by hand
Gitlink bump ✅ 29ec9e7..be1160f is exactly one commit, the trinity-enterprise#669 merge (merged 2026-10-02). Nothing else rides along
Enterprise alembic heads ✅ check_alembic_heads.py at be1160f: 19 revisions, 1 head (0018_agent_assignments ← 0017_credential_vault)
Feature flows / arch docs ✅ role-assignments.md, the feature-flows index and architecture/mcp-server.md updated. The enterprise-docs-guard job passed
Security greps (§4.1–4.10) ✅ no keys, emails, IPs, mode flips or host paths. backend.internal is a test fixture
Merge gate, Tier 1 ✅ required contexts, pytest (head, seed 12345), regression diff and gitleaks are all green
Merge gate, Lane C Tier 2 on PR ✅ journey-smoke and prod-image-smoke green. e2e was skipped, and no src/frontend/ file changed
dev before merge ✅ dev-ci: success, no open dev-ci red issue

Execution coverage (§5.4)

changed symbol executed by live consumer verdict
_render_assignment consent (I1) test_a_provider_that_omits_consent_renders_the_restrictive_clause through the real compose_system_prompt compose_system_prompt → prompt block ✅
_validated non-bool consent (R1) test_a_non_bool_consent_is_unresolved (7 params), plus the end-to-end string-"false" test resolve_assignment → build_execution_context ✅
degrade() drops detail (I3) 4 cases call the real execute() with a leaking error MCP tool response ✅
self-scope check (I2) own / other / nameless agent keys, plus user and system keys, with backend calls counted get_agent_assignments.execute ✅
server.ts registration tool-visibility.test.ts, a source-regex guard (pins the registration set) plus the scope-visibility checks createServer toolGroups ✅ (guard)

Fix mutation: the author reverted each fix on the final tip and named the tests that went red (I1→3, I3→4, I2→2, R1→8). The tests check the rendered line verbatim and assert the exact backend call count, so they would catch a reverted fix.

Review notes

  • [I1] is True instead of is not None is the right shape. None, False and anything a future caller passes now fail closed in the renderer. Leaving the compose_system_prompt merge alone is fine, because the renderer is now the single place that decides.
  • [R1] is a good catch. bool("false") is True would have reopened I1 one layer earlier. Dropping the field with a WARNING matches how every other malformed field is handled.
  • [I2] The tool refuses before any lookup, so existing and missing names get the same answer and there is no enumeration signal. An agent key with no name fails closed. The denial is recorded through accessDenied, so it is audited. Matches the checkSelf precedent.
  • [I3] Dropping detail on every branch is better than capping it.

Non-blocking

  • The PR body still opens with "DRAFT. Do not merge yet … pending #669", and the "Before merge" checklist is unticked. Both are out of date now that #669 is merged and be1160f is pinned. Please update the body before merge so nobody who reads it holds the merge.
  • Two older github-actions comments report a merge conflict against dev. GitHub now shows the PR as MERGEABLE, so they look stale.
  • The tool-count header in the user docs may still be stale from later PRs. The author already flagged this, and it's not this PR's problem.
  • No separate /review pass was posted. This report covers process and execution coverage. My own read of the auth path (I2) found no issue.

Recommendation: APPROVE. All four items from the #2596 review (I1, I2, I3, R1) are fixed with tests that run the real code paths. The gitlink is exactly #669, the enterprise migrations have one head, and CI is green in every tier this lane requires.

🤖 Generated with Claude Code

@AndriiPasternak31
AndriiPasternak31 merged commit 1a2b3cf into dev Oct 2, 2026
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants