Skip to content

fix(agents): start returns skills_injection/skills_result — skill-delivery failures reach REST and MCP callers (#2991) - #3008

Merged
vybe merged 5 commits into
devfrom
fix/2991-start-skills-result
Sep 25, 2026
Merged

vybe merged 5 commits into
devfrom
fix/2991-start-skills-result

Conversation

@dolho

@dolho dolho commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #2991

Problem

start_agent_internal computed the skill-delivery result, but POST /api/agents/{name}/start rebuilds its response from a whitelist that left it out. So a REST or MCP caller could not tell a clean start from one where an assigned skill failed to land or hit a name conflict.

Fix

  • Endpoint: skills_injection and skills_result are returned beside the credentials pair they mirror.
    • skills_result is a projection built by public_skills_result, with names, statuses and codes only (the ent#334 rule).
    • A per-skill error that carries an exception string, transport URL or path is reduced to a code: skill_too_large, not_in_library, name_conflict, or else error. The only warnings passed through are missing_env:/missing_binary: names; the rest are dropped.
    • A no-op start answers skipped with its reason (no_skills, container_already_running). A missing result reads unknown; the field is never absent (AC4).
  • Conflict (AC3): bug: assigning a library skill whose name matches an agent-authored skill silently overwrites the agent's copy — refuse and show the name conflict in the Skills tab #2914 deliberately keeps success for a conflict, because the agent's own copy runs. inject_assigned_skills now lists it in conflicts, so the case is distinguishable in one response. The status vocabulary is unchanged, as agreed.
  • MCP start_agent (AC2): the first line is unchanged. When delivery wasn't clean, it adds Skills delivery: <status> and one line per skill that didn't land, e.g. - backlog: conflict (name_conflict). A clean or no-op start adds nothing, and an unknown result is never presented as clean.
  • Same gap, closed in the blast radius:
    • POST /api/system-agent/restart returns the same pair.
    • The fleet-restart entry gains skills_conflicts.
    • The projection is shape-defensive. It runs inside the start's try, so a raise would turn a start that happened into a 500 the caller retries.

Tests

  • tests/unit/test_2991_start_skills_result.py drives the real start_agent_endpoint with the lifecycle stubbed, across clean, conflict, partial with leaky error text, no-op, already-running, missing and malformed results. It also covers inject_assigned_skills naming conflicts.

  • The system-agent restart and fleet-restart assertions were added to their existing tests.

  • src/mcp-server/src/tools/agents.start.test.ts drives the real tool execute().

  • Mutation checks (fix reverted, then restored):

    Reverted part Tests that failed
    Whitelist 6
    Raw result passed through 5
    Lifecycle drops conflicts 1
    MCP returns only the message 2
  • Results: 115 backend tests pass across the start, skip-inject, fleet-restart and system-agent suites. The MCP suite passes (277), and tsc is clean.

Review

The independent review found no blockers. I fixed its lows:

  • a shape guard in the projection;
  • the fleet skills_conflicts entry;
  • unassigned_meanwhile is quiet in the MCP output;
  • a misplaced JSDoc.

Noted, out of scope:

  • Credentials half: credentials_result in the same response still passes raw error text through. It's the same projection concern, and I'd suggest a follow-up issue.
  • Reconcile detail: reconcile refused/failed detail is reduced to status plus count.

🤖 Generated with Claude Code

dolho and others added 2 commits September 24, 2026 17:39
…ivery failures reach REST and MCP callers (#2991)

`start_agent_internal` computed the skill-delivery result, and
`POST /api/agents/{name}/start` rebuilt its response from a whitelist that
left it out, so a caller could not tell a clean start from one whose assigned
skill failed or hit a name conflict.

- The endpoint returns `skills_injection` + `skills_result` beside the
  credentials pair. `skills_result` is a projection (`public_skills_result`):
  names, statuses and codes only — a per-skill error that carries an
  exception string, URL or path is reduced to a code (ent#334). A no-op start
  answers `skipped` + reason; a missing result reads `unknown`, never absent.
- A conflict keeps `success` (#2914), so `inject_assigned_skills` now names
  it in `conflicts` — distinguishable from a clean start in one response.
- MCP `start_agent` keeps its first line and, when delivery was not clean,
  adds the status and one line per skill that did not land.
- Same gap closed on `POST /api/system-agent/restart`; the fleet restart entry
  gains `skills_conflicts`. The projection is shape-defensive: it runs inside
  the start's try, so a raise would turn a start that happened into a 500.

Mutations: whitelist reverted (6 red), raw result passed through (5 red),
lifecycle drops conflicts (1 red), MCP returns only the message (2 red).

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

@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/20260925-1608 (#3027, full suite green).

@vybe
vybe merged commit efa6b77 into dev Sep 25, 2026
25 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.

3 participants