Skip to content

fix(skill-gate): close the #3208 validation gaps before the gate map lands (#3274) - #3345

Merged
vybe merged 7 commits into
devfrom
feature/3274-gate-hardening
Oct 7, 2026
Merged

vybe merged 7 commits into
devfrom
feature/3274-gate-hardening

Conversation

@webmixgamer

@webmixgamer webmixgamer commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The gated-skill check (#3208, abilityai/trinity-enterprise#751) is on dev and inert until the gate map (abilityai/trinity-enterprise#753) holds a row. The #3208 merge-train validation listed seven gaps that a row makes reachable. This closes them, plus #3233 and the UI's handling of the 202.

Merge order: this lands before, or in the same window as, abilityai/trinity-enterprise#753, never after it.

  1. Context scan. The executor's system prompt carries free text the requester sets beside the request: a schedule's name, an MCP key's name and the requester's email. The gate now scans each one, both as given and as the prompt renders it, since the renderer collapses --- and cuts at 80 characters with …. The text is scanned apart from the request, never joined to it. It appears on the approver's card only when it is what matched, and it is never replayed. A parity test classifies every ExecutionContext string field as scanned or platform-controlled.
  2. Telegram quoted reply. A group reply is scanned with the quote line it carries ([Replying to …: "…"]), matching the Workspace twin.
  3. Telegram group history is not scanned (decided). It is now a stated limit in the flow and the requirements, along with sender labels and the group title. The limit names the runtimes without the in-container hook (Codex, Gemini).
  4. Three wiring lines are pinned. Each test runs the code that contains the line:
    • get_current_user with a real loopback token checks is_event_loopback;
    • the observer test slices main._start_maintenance_services out with ast and runs it;
    • OperatorQueueSyncService._poll_cycle checks sweep().
  5. Approved replay. An approved /task or fan-out run is sent the caller's message as the message and its system prompt as the system prompt (frozen_replay), never their concatenation. A record frozen before this change replays its request text, as before.
  6. Inline connector tier. inlineConnectorChat reads the gate result before its own 403 and non-2xx handling, so a refusal keeps its named code.
  7. Matcher.
    • -/x, _/x_ and 1/x match.
    • A trailing run of ., _ or - ends a name.
    • Every Unicode Default_Ignorable code point is matched two ways, and the results are unioned. In the first, the character is removed, except that a run right before a slash reads as a space. In the second, each one is replaced by a space.
  8. No approver email to the requester.
    • A request made by a platform session or key reads the approver's display name.
    • Everyone else reads "the agent's approver": the Workspace, the inline connector, operator resumes, an approver with no usable name, and a failed lookup.
    • An agent requester still learns only the outcome.
  9. The UI says "waiting for approval", not an error.
    • Chat tab and /m: the server's message appears as a platform line. Held lines stay out of the next turn's history.
    • Public link: polling stops at the held turn's skipped row, and the status route returns its notice.
    • Tasks tab: a held row stays, marked pending_approval.
    • Playbooks and Dashboard: an info toast on the agent page. The toast gained a status-info arm, which also fixes the existing info toast rendering red.
    • Refusals show their named message.
  10. bug: a system-scoped caller of a gated skill is promised the outcome but never receives it #3233. outcome_delivery (agent_task / inbox / none) is the one rule _notify follows, and it rides on the 202. When nothing will be delivered, the message says so: "Nothing will be sent back when it is decided: if it is approved, it runs as a new execution on ; if not, nothing runs." The MCP layer promises delivery only for agent_task or inbox. The delegation contract defers to the message; the line was trimmed to fit its 1,730-character budget.

Rider: connector.test.ts now pins run_playbook's exact message.

Deferred (from the same validation, marked "no urgency"):

  • the write-only columns, which need a migration;
  • the architecture-catalog rows, which abilityai/trinity-enterprise#753 edits for its own table anyway;
  • building the requester after read_gates, a perf nicety that would put a union type in security code.

All three are tracked in #3344.

Found during /cso --diff and fixed here: an invisible character before the slash plus one inside the name evaded both normalisations (run­/pay‍-invoice). The report is docs/security-reports/cso-diff-2026-10-07-3274-gate-hardening.md, and the learnings fragment is 2026-10-07-a-matcher-must-read-every-form-the-reader-sees.md.

Review round (b5ad0e862). A full /review with two independent reviewers found and fixed the following.

  • The approval card could hide part of what the approved run sends. The card sanitised the joined message and system prompt, while the run sent each part sanitised separately. A credential pattern spanning the line between them hid the system prompt on the card only. The card is now built from the exact replay parts, and the caller's system prompt is labelled "With these instructions as its system prompt:".
  • The matcher had a quadratic case. A long run of invisible characters took quadratic time (50k chars: ~10 s); it is now linear.
  • A narrower _AFTER missed /x...y. That input is caught again, and a property test against the fix: backend Dockerfile missing COPY for canary/ package (a4eec13 breaks startup) #751 matcher holds that the matcher only ever widens.
  • /m re-sent a refused gated request. It now stays out of the next message's history.
  • Refusals and contrast:
    • the /m notice now has its own gray-400 colour;
    • Playbooks shows a refusal in the persistent error toast;
    • the public status route returns gate (held / refused), so a refusal shows as an error;
    • the new system line and the toast are announced to screen readers.

No DB migration: the replay fields live in the existing dispatch JSON column. No docker/ change.

Changes

  • Backend: services/skill_gate_service.py, services/skill_gate_errors.py, utils/skill_invocation.py, services/task_execution_service.py (backstop + gate_replay), services/chat_execution_service.py (/task), services/dispatch_admission_service.py (/chat), services/fan_out_service.py, adapters/message_router.py, routers/public.py, services/platform_prompt_service.py.
  • MCP: src/mcp-server/src/client.ts, delegation_contract.ts.
  • Frontend: utils/skillGate.js, chat/ChatSystemLine.vue, ChatBubble.vue, ChatPanel.vue, PublicChat.vue, TasksPanel.vue, PlaybooksPanel.vue, DashboardPanel.vue, MobileAdmin.vue, AgentDetail.vue.
  • Docs: feature-flows/skill-gate.md, requirements/security.md §26.13, feature-flows/mcp-connector.md, feature-flows/mcp-orchestration.md.

Test Plan

  • New unit tests pass:
    cd tests && pytest unit/test_3274_*.py unit/test_ent751_skill_invocation.py unit/test_ent751_gate_http_mapping.py -v
    These cover context, replay, notice, wiring, the channel quote, the public skipped status, and the matcher rows.
  • Existing gate suites pass: test_ent751_*, test_ent752_*, test_ent568_delegation_contract, test_ent600_*, test_1028_lifespan_phases. That run was 707 passed.
    • In a 7,532-test related run, one test failed: test_ent549_file_audience::test_whatsapp_media…. It is order-dependent and fails identically on the base under the same fixed order.
  • MCP: the full node --import tsx --test suite passes (811/0), and tsc --noEmit is clean.
  • Frontend (mounted): skillGateSenders.spec.js, mobileAdminSkillGate.spec.js and skillGate.spec.js pass, plus the agentDetailGateNotice.spec.js source pin. The raw-colour, loading-gate and source-text ratchets pass.
  • Mutation. Each new call site was reverted from a copy and its test went red:
    • 21 backend sites, including the display-name branch of the approver notice (test_a_platform_requester_reads_the_deciders_display_name) and the before-slash rule, plus 7 more in the review round;
    • 15 UI sender branches;
    • 2 MCP lines.
  • /verify-local (--skip-agent; nothing under docker/base-image/ changed):
  • Manual eyeball on localhost:
    1. Chat tab ✅
    2. Tasks tab ✅
    3. Playbooks ✅
    4. Public link ✅
    5. Approval notice ✅ (the fallback label; the display-name branch is unit-tested)
  • Check 4 (Dashboard → Update Dashboard) is covered by the mounted skillGateSenders.spec.js.
  • Check 5 (/m chat) is covered by the mounted mobileAdminSkillGate.spec.js. Since fix(frontend): /m says the agent list is admin-only instead of "Couldn't load agents" (#3041) #3071 the /m agent list is admin-only, so the eyeball needs an admin who is not the approver.
  • The frontend unit job may go red on portalBackgroundAskInboxOnly.spec.js. That is not caused by this branch. The spec's "ended" fixture dated 09-30 aged out of its 7-day window on 10-07, and open PR fix(workspace): a turn shows its own reply on a shared thread (#3166) #3332 fixes it.

Eyeball recipe (needs a temporary gate; never commit the shim)

  1. On the checkout that serves localhost, switch to this branch. Snapshot src/backend/services/skill_gate_service.py with cp, then replace the body of list_skill_gates with:
    import json  # EYEBALL SHIM — never commit
    try:
        with open("/data/eyeball-skill-gates.json") as f:
            data = json.load(f)
    except FileNotFoundError:
        return {}
    return {k: SkillGate(**v) for k, v in data.get(agent_name, {}).items()}
  2. Give the agent a temporary, harmless skill of its own:
    docker exec -u developer agent-<agent> sh -c 'mkdir -p ~/.claude/skills/eyeball-gate && printf -- "---\nname: eyeball-gate\ndescription: eyeball only\n---\nReply with the word OK.\n" > ~/.claude/skills/eyeball-gate/SKILL.md'
  3. Write the gate file. The backend's /data is a Docker volume, so write it through the container:
    docker exec -i trinity-backend sh -c 'cat > /data/eyeball-skill-gates.json' <<'JSON'
    {"<agent>": {"eyeball-gate": {"approver": "primary"}}}
    JSON
  4. Ask as a shared account that is not the approver; the approver self-approves and never sees a 202.
    • Chat tab: send /eyeball-gate hi → a centred gray "Not run … waiting for a decision" line, and the message is kept. A follow-up message runs, and only one card is raised.
    • Tasks tab: run the same → the row stays pending_approval with the notice.
    • Playbooks: press Run → a blue info toast, and no jump to Tasks.
    • Public link, in a private window: the gray line ends "Nothing will be sent back when it is decided: …".
    • /m: needs an admin who is not the approver.
    • Approve one card as the approver → the asker's Inbox notice says "…approved by <display name | the agent's approver>…", with no email.
  5. Cleanup:
    • cp the snapshot back and confirm git diff --quiet on the file;
    • docker exec trinity-backend rm /data/eyeball-skill-gates.json;
    • remove ~/.claude/skills/eyeball-gate from the agent;
    • cancel the leftover cards.

Fixes #3274
Fixes #3233

🤖 Generated with Claude Code

…lands (#3274)

The gated-skill check (#3208) is inert until the gate map supplies a row.
The #3208 merge-train validation listed seven gaps that a row makes
reachable; this closes them, plus #3233 and the UI's handling of the 202.
It lands before or together with the gate map, never after it.

- Context scan: a schedule's name, an MCP key's name and the requester's
  email reach the executor's system prompt beside the request. The gate
  now scans them (raw and as the prompt renders them), apart from the
  request, shows them on the card when they alone matched, and never
  replays them. A parity test classifies every ExecutionContext string
  field.
- Telegram: a group reply is scanned with the quote line it carries.
  Group history, sender labels and the group title stay unscanned
  (decided; stated limit, naming the runtimes without the hook).
- Wiring: the event-loopback flag, the ending-observer registration and
  the sweep call are each pinned by a test that runs the line.
- Replay: an approved /task or fan-out run is sent the caller's message
  and system prompt apart, never their concatenation.
- MCP: the inline connector tier reads the gate result like the other
  tiers; a pending answer promises delivery only when the backend makes
  one (outcome_delivery), and the delegation contract defers to the
  message (#3233).
- Matcher: -/x, _/x_ and 1/x match; every Default_Ignorable character
  is matched removed (a run before a slash reads as a space) and spaced.
- Notices: the requester never sees the approver's email; a platform
  request reads the display name, everyone else "the agent's approver".
  A requester nobody notifies is told so and where the outcome shows.
- UI: the Chat tab, /m, the public link, the Tasks tab, Playbooks and
  Dashboard show the server's message instead of an error and poll
  nothing; held chat lines stay out of the next turn's history.

Mutation: each call site reverted from a copy turns its test red — 21
backend sites (test_3274_*, test_ent751_skill_invocation), 15 UI
sender branches (skillGateSenders / mobileAdminSkillGate), 2 MCP lines
(chat-gate.test.ts).

Fixes #3274
Fixes #3233

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@webmixgamer webmixgamer added the ui PR touches the frontend UI — triggers Playwright e2e tests label Oct 7, 2026
Comment thread src/backend/utils/skill_invocation.py Fixed
Comment thread src/backend/utils/skill_invocation.py Fixed
@webmixgamer

Copy link
Copy Markdown
Contributor Author

CI note: frontend-build is red on one spec only, tests/unit/portalBackgroundAskInboxOnly.spec.js › "Action lists it while it waits; All keeps it once it ended" (1 failed | 4953 passed). Its "ended" fixture is dated 2026-09-30 and aged out of a 7-day window on 2026-10-07. It is not caused by this branch: the spec touches no file this PR changes, and it fails the same way on the base. Open PR #3332 updates that spec. The frontend-e2e / changes cancel is the duplicate run triggered by opening with the ui label.

…eQL py/overly-large-range)

CodeQL reads a character range beyond the Basic Multilingual Plane as
`�-�`, so the three such ranges sharing one class in
`_INVISIBLE` (U+1BCA0, U+1D173, U+E0000 blocks) were flagged as
overlapping (alerts 385, 386). Each now sits in a class of its own,
joined by alternation. Same characters either way: 0 of 1,111,998
non-surrogate code points match differently (4,174 invisible before and
after), and the before-slash rule's output is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…inear matcher, honest refusals (#3274)

From the full /review of PR #3345 (two independent reviewers):

- Card vs run (critical): the card sanitised the JOIN of the message and
  the caller's system prompt, while the approved run sent each sanitised
  alone. A credential pattern spanning the line between them redacted
  the system prompt from the card only, so Approve ran an instruction
  the approver never saw. The card is now built from the exact replay
  parts, with the system prompt labelled ("With these instructions as
  its system prompt:").
- Matcher: the before-slash rule was quadratic on a long run of
  invisible characters (50k took ~10 s on the event loop, reachable
  from a public-link message); a run is now matched only from its
  start. `_AFTER` regressed `/x...y` and `/x._y` against #751; the dot
  rule is #751's again, and a property test holds that every input the
  #751 matcher caught is still caught.
- /m: a refused gated request is kept out of the next message's history
  too; the notice gets its own gray-400 colour (theme token) and
  role="status".
- Playbooks: a gate refusal goes to the agent page's error toast, which
  stays until dismissed. The agent page toast carries role status/alert.
- Public link: the status route returns `gate` (held | refused); a held
  turn shows the grey notice, a refusal the red error box.
- Tests: the Workspace decider-label test now reaches its own arm;
  refusal assertions reject the JSON-rendered detail; the public-link
  spec fails on an assertion, not a timeout.

Mutation: each fix reverted from a copy turns its test red (7/7).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vybe

vybe commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

merge-train (mechanical): merged origin/dev into this branch (0d21853) so build re-runs against the date-rot fix in #3348 — the earlier build failure (portalBackgroundAskInboxOnly.spec.js) was a fixture dated 2026-09-30 aging out of the 7-day window, reproduced on plain dev, not this PR. No code changed. Validation: READY; merges ahead of #3347 per both PRs' bodies.

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train/20261007-1609 (#3349)

@vybe
vybe merged commit 547deb2 into dev Oct 7, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants