Skip to content

feat(server): add control clone-and-bind endpoint#229

Open
abhinav-galileo wants to merge 2 commits into
mainfrom
feature/control-clone-and-bind
Open

feat(server): add control clone-and-bind endpoint#229
abhinav-galileo wants to merge 2 commits into
mainfrom
feature/control-clone-and-bind

Conversation

@abhinav-galileo
Copy link
Copy Markdown
Collaborator

@abhinav-galileo abhinav-galileo commented May 19, 2026

Summary

  • Add controls.cloned_from_control_id plus cloned=true|false filtering on GET /api/v1/controls.
  • Add POST /api/v1/controls/{control_id}/clone-and-bind to clone an active control and create a target binding in one transaction.
  • Add optional attachment expansion on GET /api/v1/controls for direct agents, policies, and target bindings, including each control's action.
  • Apply attachment_target_type / attachment_target_id filters before list pagination, so callers can list controls attached to a specific target.
  • Add PATCH /api/v1/control-bindings/by-key so target-scoped callers can enable or disable an existing binding without knowing the binding ID.
  • Distinguish unexpected authorization-upstream 4xx responses as AUTH_UPSTREAM_REJECTED instead of reporting them as local auth misconfiguration.
  • Update Python and generated TypeScript SDK surfaces with server and SDK tests.

Usage

Clone a control and bind the clone to a target:

POST /api/v1/controls/{control_id}/clone-and-bind
{
  "name": "optional-clone-name",
  "target_binding": {
    "target_type": "environment",
    "target_id": "prod",
    "enabled": true
  }
}
{
  "id": 42,
  "name": "optional-clone-name",
  "cloned_from_control_id": 7,
  "binding_id": 99
}

List root controls or cloned controls:

GET /api/v1/controls?cloned=false
GET /api/v1/controls?cloned=true

List controls with page-scoped attachment details:

GET /api/v1/controls?include_attachments=true

List controls attached to a specific target:

GET /api/v1/controls?include_attachments=true&attachment_target_type=environment&attachment_target_id=prod

When target filters are provided, the control list is filtered before pagination, and each returned control's attachments.targets is filtered to the same target.

Each expanded control includes the normal control details, action, and optional attachments:

{
  "id": 42,
  "name": "example-control",
  "description": "Example control",
  "action": {
    "decision": "deny",
    "steering_context": null
  },
  "attachments": {
    "agents": [{ "agent_name": "example-agent" }],
    "policies": [{ "policy_id": 42 }],
    "targets": [
      {
        "binding_id": 123,
        "target_type": "environment",
        "target_id": "prod",
        "enabled": true
      }
    ]
  }
}

Enable or disable an existing target binding by natural key:

PATCH /api/v1/control-bindings/by-key
{
  "target_type": "environment",
  "target_id": "prod",
  "control_id": 42,
  "enabled": false
}
{
  "success": true,
  "enabled": false
}

This PATCH route updates only existing bindings. It returns CONTROL_BINDING_NOT_FOUND instead of creating a missing binding.

For full target-binding pagination/filtering, callers can continue using GET /api/v1/control-bindings.

Validation

  • make models-test
  • make server-test
  • make sdk-test
  • uv run --package agent-control-models ruff check --config pyproject.toml models/src
  • uv run --package agent-control-server ruff check --config pyproject.toml server/src
  • uv run --package agent-control ruff check --config pyproject.toml sdks/python/src
  • uv run --package agent-control-models mypy --config-file pyproject.toml models/src
  • uv run --package agent-control-server mypy --config-file pyproject.toml server/src
  • uv run --package agent-control mypy --config-file pyproject.toml sdks/python/src
  • make sdk-ts-typecheck
  • make sdk-ts-name-check
  • make sdk-ts-generate-check

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 94.73684% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ver/src/agent_control_server/endpoints/controls.py 88.88% 13 Missing ⚠️
...rver/src/agent_control_server/services/controls.py 98.36% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@abhinav-galileo abhinav-galileo force-pushed the feature/control-clone-and-bind branch 4 times, most recently from a9266d5 to 4f47fd7 Compare May 20, 2026 12:14
@abhinav-galileo abhinav-galileo force-pushed the feature/control-clone-and-bind branch 4 times, most recently from 749be08 to 7dcc23f Compare May 20, 2026 18:19
@abhinav-galileo abhinav-galileo marked this pull request as ready for review May 20, 2026 18:21
@abhinav-galileo abhinav-galileo force-pushed the feature/control-clone-and-bind branch from 7dcc23f to 831e8ce Compare May 20, 2026 19:08
stage: str | None = Query(None, description="Filter by stage ('pre' or 'post')"),
execution: str | None = Query(None, description="Filter by execution ('server' or 'sdk')"),
tag: str | None = Query(None, description="Filter by tag"),
include_attachments: bool = Query(
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.

Silent attachment-filter degradation when CONTROL_BINDINGS_READ auth fails

async def _optional_attachment_target_principal(request: Request) -> Principal | None:
    ...
    try:
        return await get_authorizer(...).authorize(...)
    except (AuthenticationError, ForbiddenError, NotFoundError):
        return None

and downstream:

filter_by_attachment = target_principal is not None and (
    attachment_target_type is not None or attachment_target_id is not None
)

A caller with CONTROLS_READ but no CONTROL_BINDINGS_READ who sets include_attachments=true&attachment_target_type=session&attachment_target_id=ses-1 will:

  • Pass _validate_attachment_filters (because include_attachments=true)
  • Get target_principal = None (auth swallowed)
  • See filter_by_attachment = False
  • Receive all controls in the namespace, with targets=[] on each

The caller's intent ("show me controls bound to this session") is silently inverted into "show me every control." That's a UX trap, and worse, it's the same response shape they'd get if no controls were actually bound to that target — so they can't distinguish "no matches" from "I don't have permission to see matches."

Recommended fix: When the secondary auth fails and the caller supplied attachment_target_*, raise the original ForbiddenError (or a derived one mentioning that target-binding read is required for target-filtered listings). The fallback-to-None-Principal pattern should only apply when no target filter was requested — i.e., when target principal is just being used to gate the targets block.

if include_targets:
target_query = (
select(
ControlBinding.control_id,
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.

For a control with thousands of target bindings (a common pattern when target_type="session" or "log_stream"), GET /controls?include_attachments=true returns every binding row inline. With pagination on the outer list (20-100 controls per page), the worst-case response can be megabytes of binding rows per page, with no cap.

Two issues here, ranked:

Response size DoS vector — a single request can fan out into very large responses.
Memory pressure on the server — every binding row materializes into ControlTargetAttachment then TargetAttachmentRef.

Recommended fix: Add a per-control cap (e.g., 10-20 targets returned inline) and surface a targets_truncated: bool or targets_total: int on ControlAttachments so the caller knows to drill into /control-bindings?control_id=... for the full set. This is the same pattern already used elsewhere in the codebase for usage limits.

Even if the typical caller asks for attachment_target_id filters that narrow to one or zero rows per control, the unfiltered path (include_attachments=true without target filters) has no protection.

@abhinav-galileo abhinav-galileo force-pushed the feature/control-clone-and-bind branch from b1f5e2b to 58d9e14 Compare May 22, 2026 17:15
def upgrade() -> None:
op.add_column(
"controls",
sa.Column("cloned_from_control_id", sa.Integer(), nullable=True),
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.

Why not just keep this in the data column of control?

we could add index into the JSONB column so its faster

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

cloned_from_control_id is system lineage metadata, not part of user-authored control config, so keeping it separate from data feels cleaner. As a column, it also lets us enforce same-namespace referential integrity with a composite FK and filter/index it directly.

@lan17
Copy link
Copy Markdown
Contributor

lan17 commented May 22, 2026

Add POST /api/v1/controls/{control_id}/clone-and-bind to clone an active control and create a target binding in one transaction.

Jw, why does it need to be inside transaction? We could do this via several API calls without making new ones right?

@abhinav-galileo
Copy link
Copy Markdown
Collaborator Author

abhinav-galileo commented May 22, 2026

Add POST /api/v1/controls/{control_id}/clone-and-bind to clone an active control and create a target binding in one transaction.

Jw, why does it need to be inside transaction? We could do this via several API calls without making new ones right?

Yes, this could be done with individual APIs, but that path would need a small refactor to preserve clone lineage consistently.

This endpoint is mainly a transactional convenience API for the UI: clone + bind is one user action, and keeping it server-side avoids partial state and client-side cleanup logic if the bind step fails.

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