feat(api): add scope-guarded profile write endpoints

[sujoydc]

What

The write half of the profile HTTP surface for #510, plus the service changes it
needs and a fix for the P3 finding on #575.

POST   /agents/profiles                 create from a supplied document
PUT    /agents/profiles/{name}          replace an existing local profile
DELETE /agents/profiles/{name}          remove a local profile
GET    /agents/profiles/{name}/source   unresolved authoring read

No UI. That is the next and final PR on this issue.

This closes four of the five contracts @haofeif asked for on #510 (points 1, 2,
3 and 5) and the P3 comment from #575. Point 4, schema completeness, landed in
#575 with a disclosed remainder repeated under Known gaps below.

Review round 1

@haofeif and @fanhongy independently reviewed the first revision and converged on
the same two blocking defects. All four findings are fixed in follow-up commits on
this branch. I reproduced each one before fixing it rather than taking it on
trust, and two turned out worse than reported.

Finding	Fix	Detail
P1 GET /{name}/source had no scope gate	require_any_scope(READ, WRITE, ADMIN) + structural and enforcement tests	Scope guards
P1 delete_profile did not take the write lock	new locked_atomic_delete; deletion moved under the same lock	What changed
P2 DELETE scope contradicted #510	now WRITE, ADMIN	Scope guards
P3 replace_profile missing from __all__	added	—

Two things I want to surface rather than bury, because both are mine and neither
was caught by the tests I wrote:

The replace_profile docstring asserted that enforcing existence inside the lock
closes the update-versus-delete window. It described the hazard accurately and
then left delete_profile unlocked, so the hazard simply moved to the other side
of the pair. A docstring promising a guarantee the code does not deliver is worse
than an undocumented gap.

The concurrency test was named
test_replace_profile_lets_exactly_one_concurrent_deleter_or_writer_win, but its
body deleted the file before the barrier and then raced two writers. It never
overlapped a delete with a write, so the name claimed coverage that did not
exist. @fanhongy caught exactly this.

The scope reversal is discussed in full under Scope guards. Short version: I
justified admin-only on a six-of-seven precedent, but scopes are a flat set, so
admin-only 403s a cao:write client rather than merely adding admin access, and
#510 already published write-or-admin as the contract.

Why

#575 gave the profile surface a shared validator and a read-only validate route.
Nothing can yet create, edit or delete a profile over HTTP, so the Web UI still
cannot manage profiles at all, which is what #510 is for.

Doing that safely needs more than three route handlers. Three of the four write
contracts are properties of the service layer, not the HTTP layer, and getting
them wrong is silent rather than loud.

What changed

replace_profile in services/profile_store.py, the update-only
counterpart to write_profile.

This is the substantive one. write_profile(..., overwrite=True) is an upsert,
which is the wrong primitive for a PUT. A request naming a built-in or
provider-managed profile would not fail: it would create a new local-store file
that shadows the original, silently changing which profile wins on load. That is
exactly the condition duplicated_in was added to surface in #523, so an upsert
would manufacture the thing we warn about.

Only one function is added. write_profile(..., overwrite=False) is already
create-with-409, and #543 documented it as the one supported way to ask for
create-without-clobber, so a create_profile alias would be churn.

must_exist on locked_atomic_write, enforced inside the same critical
section as overwrite:

if not overwrite and target.exists():
    raise FileExistsError(...)
if must_exist and not target.exists():
    raise FileNotFoundError(...)

Same reasoning as the overwrite race we fixed in #543. A caller testing for the
file beforehand would leave a window where a concurrent delete turns an intended
update back into a create. overwrite=False with must_exist=True is
contradictory and raises ValueError rather than always surfacing as
FileExistsError, which would mislead the caller into thinking the file was in
the way.

locked_atomic_delete, also in atomic_file.py. Added during review, and it
is the other half of the guarantee above rather than a nicety. must_exist is
only meaningful if deletion takes the same lock. delete_profile was doing an
unlocked exists() then unlink(), so this interleaving was reachable:

1. replace_profile takes the lock and passes its must_exist check
2. delete_profile unlinks the file and reports success
3. replace_profile publishes, recreating what was just deleted

Both callers were told they succeeded and the deleted profile was back on disk
holding the replacement text. Reproduced deterministically with a barrier, and
the reproduction now shows the deleter blocked on the lock instead. unlink is
already atomic, so the helper adds only the lock, not atomicity. It is safe
against the flock-is-per-inode hazard because the lock file is not the target:
_lock_path_for keys a file under LOCK_DIR by a hash of the resolved path, and
those are never unlinked, so removing a target leaves the lock inode intact.

The docstring I shipped on replace_profile claimed the enclosing guarantee
while delete_profile was still the unlocked side of it, and the concurrency
test was named for a deleter it never exercised. Both are corrected.

Built-in protection falls out of the service boundary, which is what point 3
asked for. profile_store resolves only inside LOCAL_AGENT_STORE_DIR, so a
built-in's name is simply not there, and must_exist therefore rejects PUT
against a built-in under the lock. No handler-level check, no separate list of
protected names.

A shared _validate_profile_for_write helper in api/main.py, used by both
POST and PUT so the two cannot drift apart. It runs the #575 validator on the
exact document being persisted, rejects error-severity findings with 400, and
returns warnings for the response. It also enforces the name rule (below).

Two details in it are deliberate rather than incidental.

It parses the frontmatter once. The obvious implementation calls
validate_profile_text(content), but that parses internally and the helper needs
the metadata anyway for the name check, so the document would be parsed twice.
validate_profile_text's docstring exists specifically to stop callers
duplicating that parse, so the helper parses once and calls
validate_frontmatter(metadata) instead.

Every 400 from a write route uses one detail shape:

{"message": "...", "errors": [{"severity": "error", "message": "...", "path": "engine"}]}

errors is empty for failures that are not attributable to a field, such as
unparseable YAML, but the key is always present so a client can iterate it
unconditionally. Without this, one endpoint returned a dict for a schema failure
and a bare string for a name mismatch or a parse failure, forcing a client to
switch on type(detail). The Web UI is that client, so this would have become
its problem.

This covers the 400s, which are the ones carrying per-field findings. The service
error mappings (404 for a missing target, 409 for a conflict) keep FastAPI's
conventional bare-string detail, since the status code already tells a client
what happened and there are no findings to attach.

Three write routes and one authoring read. Scope guards mirror the existing
conventions rather than inventing one; see below.

The #575 P3 fix, folded in because point 5 makes it load-bearing: once POST
and PUT run the validator before persisting, a crash there 500s a write path
rather than a pure read.

On the name-identity rule (point 2)

A profile has two identities: the storage key (its filename stem) and the
frontmatter name. parse_agent_profile_text treats the stem only as a fallback
when frontmatter omits name:

if "name" not in meta:
    meta["name"] = profile_name

So name: foo in bar.md loads as foo while being addressed as bar, and
nothing reconciles them. Both write routes now require the two to agree, with a
400 on mismatch.

POST takes name explicitly in the body rather than parsing it out of
content, so the 409 target is unambiguous even when the document is malformed.
PUT treats the path parameter as authoritative.

Rename is deliberately not implemented. Your point 2 offers "require them to
match for create/update, or define an explicit rename operation"; this takes
the first branch. Rename is delete-plus-create with its own failure semantics
(partial failure, mid-rename collision, whether references follow) and deserves
its own design rather than riding along here. #510's title covers search, create,
edit, delete and validate, not rename.

On the unresolved authoring read (point 1)

Agreed, and the problem is worse than losing placeholders.
load_agent_profile calls resolve_env_vars(raw_text) on the raw text before
parsing, so substitution reaches the Markdown body as well as the frontmatter,
and the substitution source is the managed CAO .env file. An edit round-trip
through PUT would therefore write resolved secret values into a plaintext
profile in the local store. safe_substitute leaves unset variables intact,
which makes the damage selective and silent: only the variables a user actually
configured get baked in.

GET /agents/profiles/{name}/source returns the document verbatim.

Note on the implementation: it calls the existing
_read_agent_profile_source. That function is named private but already has
importers in three modules (cli/commands/profile.py, install_service.py), so
it is a de facto public API with a private name. I deliberately did not rename
it here: it is already HTTP-reachable through GET /agents/profiles/{name}, so a
second caller adds no new exposure, and renaming would mean four call-site edits
in files this PR otherwise does not touch. Worth doing as its own small change.

Scope guards

All three write routes mirror POST /agents/profiles/install:

_scopes: List[str] = Depends(require_any_scope(SCOPE_WRITE, SCOPE_ADMIN)),

DELETE included. It was SCOPE_ADMIN alone in the first revision and was
changed during review. Two reasons, and the first is the decisive one.

Scopes here are a flat set, not a hierarchy. require_any_scope tests
membership and get_current_scopes returns the token's claims verbatim with no
expansion, so cao:admin does not imply cao:write and cao:write does not
imply cao:read. Admin-only on DELETE therefore does not mean "admins can
also delete", it means a client holding exactly cao:write is 403'd, so the
profile-management credential this PR exists to serve could create and edit a
profile but never remove it. That contradicts the contract published in #510,
which specifies cao:write or cao:admin for all three.

Second, the precedent I originally leaned on splits differently than I claimed:

Route	Guard	Removes
DELETE /sessions/{session_name}	SCOPE_ADMIN	a running session
DELETE /terminals/{terminal_id}	SCOPE_ADMIN	a live terminal
DELETE /workflows/{name}	SCOPE_ADMIN	an orchestration definition
DELETE /flows/{name}	SCOPE_ADMIN	an orchestration definition
DELETE /memory/{key}, DELETE /memory	SCOPE_ADMIN	accumulated agent memory
DELETE /memory/relationships/{id}	SCOPE_WRITE, SCOPE_ADMIN	one graph edge

Six of seven do use admin alone, but every one of those removes running or
generated state. The lone write-or-admin exception is the only content
resource in the list. A profile is an authored document: removing one stops no
in-flight work, destroys nothing that cannot be re-authored, and is already
gated behind ConfirmModal in the UI. It belongs with the relationship delete,
not with the session teardown.

Six enforcement tests assert the guards are real rather than merely declared: a
cao:read token is 403'd on create and on delete, while cao:write is admitted
on all three and cao:admin on delete.

These are real mutations, so there are no _EXEMPT entries, unlike #575's
validate route. The _EXEMPT set in test/api/test_scope_coverage.py is
unchanged.

The read route is gated too

GET /agents/profiles/{name}/source shipped with no scope dependency in the
first revision. That was a real hole and it is now
require_any_scope(SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN), the identical shape
all ten already-guarded GET routes use.

Worth stating why the repo-wide picture did not excuse it. 29 of 39 GET routes
carry no gate, so my first instinct was that this matched convention. But the
ten that are gated are the sensitive reads (/memory/export,
/memory/relationships, /outcomes, /workflows/runs/{run_id}/result), and
this repo already settled the exact question during the #505 review: the new read
routes got the gate while their pre-existing ungated siblings were deliberately
left alone, because tightening a shipped route risks breaking an existing
unauthenticated reader. I have followed that split, so the five pre-existing
profile reads are untouched.

test_scope_coverage.py did not catch this: _MUTATING_METHODS is
{POST, PUT, PATCH, DELETE}, so a new ungated GET is invisible to it. The new
guard therefore comes with a structural test asserting the dependency exists on
the route object, mirroring _NEW_505_READ_ROUTES. A status-code test would
have been worthless here, and the #505 test says so in its own docstring: auth is
default-off, and require_any_scope hands back the full scope set when it is
off, so "the route returns 200" passes whether or not the dependency exists at
all. That is precisely how this shipped ungated.

Route ordering

GET /agents/profiles/{name}/source is declared after
GET /agents/profiles/{name}, which is safe because the extra path segment
cannot be captured by a single {name} parameter. There is a test asserting the
two return different shapes rather than one serving the other, so a future
refactor that collapses them fails loudly.

The #575 P3 fix

Three malformed-but-parseable YAML shapes raised TypeError, which is not caught
by the handler's except ValueError and therefore surfaced as HTTP 500 from a
route whose entire job is reporting what is wrong with a document:

allowedTools: [[Read]]      unhashable type: 'list'       (element not type-guarded)
role: [developer]           unhashable type: 'list'       (no type guard at all)
mcpServers: {1: {}, x: {}}  '<' not supported str vs int  (raw path sort key)

The two advisory checks test set membership, which hashes the value; the schema
error sort used raw path components, which cannot be ordered across types. Fixes:
stringify the sort key, and type-guard both advisory checks per value so the
schema owns the type error. All three now return 200 with valid: false and the
schema error attached, which is the correct answer since the schema already
rejects them.

Behaviour changes, disclosed

1. GET /agents/profiles/{name}'s docstring now points at the source route.
No behaviour change, but the resolved-versus-source distinction was previously
undocumented and is easy to get wrong.

2. Both write routes bound content at 256 KB via Pydantic max_length,
matching #575's validate route. Generous for a profile, and it avoids an
unbounded parse. Happy to drop the cap.

3. Malformed values that previously crashed now produce different text. They
were already rejected, as unknown keys or unhashable values; now the message
names the actual problem with a path a form can render against.

Deliberate choices worth flagging

Three things a reviewer is likely to question. Each is a decision, not an
oversight, so here is the reasoning up front.

PUT an invalid document to a missing profile returns 400, not 404.
Validation precedes persistence, so the malformed body wins over the absent
target:

invalid doc + missing target -> 400
valid doc   + missing target -> 404

Defensible either way. I kept validation first because reordering means an
existence pre-check outside the write lock purely for error ordering, and that
reintroduces a TOCTOU-shaped code path a future reader could mistake for the real
guard. The authoritative existence check has to stay inside the lock.

InvalidProfileNameError on POST is probably unreachable. The schema's
name pattern rejects unsafe names before write_profile sees them, and a
request-name / frontmatter-name divergence trips the mismatch check first. Kept as
defence in depth: the schema pattern and profile_store._PROFILE_NAME_RE are
independent guards that could drift apart. Happy to drop the handler if you would
rather not carry an unreachable branch.

The read and write paths validate names with different strictness.
agent_profiles._validate_agent_name rejects only /, \ and .., while
profile_store._PROFILE_NAME_RE enforces [A-Za-z0-9_-]{1,64}. So bad@name
returns 400 on DELETE and 404 on /source. This asymmetry is pre-existing
between those two modules and already applies to GET /agents/profiles/{name};
the new source route only makes it visible on one more route. Tightening it would
change the existing read route's behaviour, so it does not belong here. Traversal
is blocked on both paths, so this is a consistency wart rather than a security
gap.

Known gaps, disclosed

Point 4 is still only half closed. #575 fixed field presence by adding
container and provider_init_timeout. Field shape is unfinished:
toolsSettings, codexConfig and hooks remain bare {"type": "object"} with
no properties, so a form generated from /agents/profiles/schema cannot render
them as inputs. The UI PR plans validated JSON editors for the object-valued
fields for exactly this reason.

The Markdown body has no explicit round-trip contract. The body is the system
prompt. These routes persist it verbatim and the source route returns it
verbatim, which is the behaviour an editor needs, but the schema still describes
frontmatter only.

No rename, as described above.

Explicitly out of scope

• UI. Next PR.
• Renaming _read_agent_profile_source. Its own change.
• Declaring shapes for the three untyped object fields. Its own change,
  since it touches the schema rather than the write path.

Testing

Full suite: 6,389 passed, 39 skipped, 111 deselected, 1 xfailed.

My local environment also has 62 failures, confined to test/api/test_agui_*,
test/services/agui/, test/telemetry/test_otel_init.py,
test/test_no_ffi_guard.py, and an intermittent test/services/test_fifo_reader.py.
Same count and per-file distribution measured on main with this branch's changes
absent, and none of those modules import profile code. Flagging rather than
omitting, since I can't confirm how they behave in CI.

Counts from pytest --collect-only against both this branch and a clean
origin/main worktree, not inferred:

File	main	branch	new
test/api/test_api_profile_surface.py	32	58	+26
test/utils/test_atomic_file.py	29	39	+10
test/api/test_scope_coverage.py	11	20	+9
test/services/test_profile_store.py	25	32	+7
test/services/test_profile_validator.py	25	31	+6

58 net new tests. An earlier revision of this description said 53, which was
wrong twice over: its own table summed to 48, and the total had been carried
across an edit rather than recomputed. The figures above are re-measured on both
trees.

The ones worth calling out:

• PUT and DELETE against code_supervisor, a real shipped built-in, assert
  404 and that no local file was created. This is the point-3 regression
  guard, at both the service and HTTP layers.
• DELETE overlapped with an in-flight PUT, pausing inside the publish so the
  window is deterministic rather than scheduler-dependent. The deleter must still
  be blocked on the lock while the replace holds it, which is the assertion that
  fails on the unlocked implementation.
• A separate two-thread barrier proves must_exist holds under contention: both
  concurrent updaters of an absent target are refused and neither creates it.
  Named for what it does now; it previously claimed deleter coverage it did not
  have.
• locked_atomic_delete is asserted to contend on the same lock as the writers,
  by holding that lock externally and requiring the delete to time out. A delete
  that keyed a different lock, or took none, would return immediately.
• A structural test asserts the source route carries a scope dependency on the
  route object, plus enforcement tests that a scopeless token is 403'd and a
  cao:read token is admitted.
• An invalid PUT leaves the existing file byte-identical, so validation
  genuinely precedes persistence rather than running alongside it.
• A warning-only profile is written and returns its warnings, which is the
  block/allow contract the UI depends on.
• The source route returns ${MY_TOKEN} unresolved, the point-1 guard.
• Four different rejection kinds are asserted to return the same detail
  shape, so the unified error contract cannot regress silently.

black --check src/ test/ and isort --check-only clean across 536 files.
mypy on both changed service files: no issues.

Exercised against a running server

Every endpoint test monkeypatches the store, so the routes were also driven over
real HTTP against cao-server with CAO_HOME_DIR pointed at a throwaway
directory. Six checks, all as expected.

Happy path:

POST /agents/profiles                -> 201  {"name": "probe", "warnings": []}
GET  /agents/profiles/probe/source   -> 200  content returned verbatim
PUT  /agents/profiles/probe          -> 200
DELETE /agents/profiles/probe        -> 204

The source response is the point-1 evidence. The stored document contained
Token: ${MY_TOKEN} and came back with the placeholder intact:

{"name": "probe", "content": "---\nname: probe\ndescription: Local check.\n---\n\nToken: ${MY_TOKEN}\n"}

GET /agents/profiles/probe would have returned that substituted from the managed
environment file, and writing it back would have persisted the resolved value.

Built-in protection, the point-3 guard:

PUT    /agents/profiles/code_supervisor -> 404, nothing created
DELETE /agents/profiles/code_supervisor -> 404

code_supervisor is a profile that ships with the package. A 200 there would mean
a local file had been created that shadows it on load.

Unified rejection shape, two different causes on the same route:

// name mismatch
{"detail": {"message": "Frontmatter name 'other' does not match the profile name 'x'. ...",
            "errors": []}}

// schema error
{"detail": {"message": "Profile failed validation and was not written.",
            "errors": [{"severity": "error", "message": "'v3' is not one of ['v2', 'kas']",
                        "path": "engine"}]}}

Same keys either way, and the field-level failure carries path: "engine" so a
form can render the error against the right input.

What comes next

The UI: a Profiles panel and nav tab consuming this surface, with inline
validation through POST /agents/profiles/validate, a from-scratch form driven by
GET /agents/profiles/schema, and clone-to-customise for built-ins.

Ref #510. Builds on #523 (read surface), #543 (profile_store,
locked_atomic_write) and #575 (validator service, validate and schema routes).

[haofeif]

I found two blocking defects: the new full-profile source read does not enforce OAuth scopes, and deletion does not use the lock required by the new update-only guarantee.

Correction: I previously described the read issue as normal agent behavior exposing secrets. That was too broad; the supported finding is the missing authorization check on the new HTTP source route.

The admin-only DELETE policy is internally consistent, but #510 still says DELETE accepts cao:write or cao:admin; please update the issue if admin-only is retained.

[haofeif]

[P2] Apply a scope check to the new source read

This is an API authorization issue, not agent runtime behavior. The new /source route returns the complete stored profile document but has no require_any_scope dependency, so a request without a token still returns 200 when OAuth is enabled. Protect this authoring endpoint with write/admin, or at minimum the normal read scope. My earlier claim about normal agent behavior exposing API_TOKEN was too broad and has been removed.

[haofeif]

[P2] Use the same target lock when deleting

The must_exist check only protects this update if deletion also takes the same lock. delete_profile() still does an unlocked exists() followed by unlink(). I paused a replace after this check, let DELETE remove the file successfully, and then let the replace continue; os.replace() recreated the profile and both operations reported success. The new concurrency test deletes the file before starting two writers, so it does not exercise this race. Put the existence check and unlink under the same per-target lock as create and replace.

[fanhongy]

Summary

The write validation and focused tests are generally thorough, but the new surface has two blocking defects: raw profile source remains readable without authentication when OAuth is enabled, and DELETE does not participate in the lock on which PUT's update-only guarantee depends.

Findings

P1 - Require authentication on the raw source endpoint

src/cli_agent_orchestrator/api/main.py:2160

GET /agents/profiles/{name}/source has no require_any_scope(...) dependency. Authentication in this service is route-dependency based, so enabling OAuth does not protect this endpoint. I reproduced this with OAuth enabled, no Authorization header, and a raw malformed profile containing a confidential marker: the endpoint returned 200 and the marker. This is broader than the parsed profile path because _read_agent_profile_source returns exact content from local, provider, and extra stores even when parsing would fail.

Add a read gate such as Depends(require_any_scope(SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN)), plus tests that a missing token is rejected and a read-scoped token is admitted.

P1 - Serialize DELETE with the lock used by update-only PUT

src/cli_agent_orchestrator/services/profile_store.py:182

replace_profile checks must_exist while holding the target lock, but delete_profile performs its exists()/unlink() at lines 210-212 without that lock. A DELETE can therefore unlink after PUT's existence check and before PUT's os.replace; PUT recreates the file and both requests report success. A deterministic barrier reproduction produced delete=success, put=success, and a surviving file containing the replacement. The added concurrency test deletes before starting its writers, so it does not exercise a concurrent deleter.

Make deletion acquire the same target lock for its existence check and unlink, preferably through a public atomic-delete helper, and add a barrier test that overlaps DELETE with PUT.

P2 - Match DELETE authorization to the issue contract

src/cli_agent_orchestrator/api/main.py:2132

Issue #510 specifies that profile deletion accepts cao:write or cao:admin, but this route requires admin only; the new test at test/api/test_scope_coverage.py:225 explicitly locks in a 403 for a write token. A client provisioned with the documented profile-management write scope can create and edit profiles but cannot complete the required delete workflow.

Use require_any_scope(SCOPE_WRITE, SCOPE_ADMIN), or formally revise the issue contract and affected client expectations before retaining the stricter policy.

P3 - Export the new public store operation

src/cli_agent_orchestrator/services/profile_store.py:143

replace_profile is a new public service function, but the module's __all__ list at lines 36-43 still exports every peer operation except this one. Explicit imports happen to work, while wildcard/public-surface consumers omit the update operation.

Add "replace_profile" to __all__.

Validation

• Re-ran all five changed test modules: 170 passed, 3 dependency deprecation warnings.
• Reproduced the PUT/DELETE race deterministically; both operations succeeded and the deleted profile was recreated.
• Reproduced the auth bypass with OAuth enabled and no bearer token; raw content returned with HTTP 200.
• git diff --check passed, and the checkout remained clean at the reviewed SHA.
• Acquisition metadata reports all 24 GitHub checks successful.

[sujoydc]

@haofeif @fanhongy please re-review this PR.

[fanhongy]

Summary

There are no changes after the prior reviewed head; the checkout remains at 167d46ce619d65d65f2018fd752858bc60f911f3. The latest commit correctly gates the source route and serializes deletes with writes. I found one P2 in the aggregate write path and one P3 test-scope nit.

Findings

P2: Write validation can persist profiles that the runtime cannot load

src/cli_agent_orchestrator/api/main.py:1998

_validate_profile_for_write treats an empty JSON-Schema finding list as sufficient and the POST/PUT handlers then persist the document. However, the request contains YAML, whose mappings may have non-string keys, while JSON Schema assumes object keys are strings. For example, both of these documents produce no validator errors:

mcpServers:
  1:
    command: echo

toolAliases:
  1: Read

I reproduced both through POST /agents/profiles: each returned 201 and created the file, but parse_agent_profile_text then raised Pydantic ValidationError at mcpServers.1.[key] or toolAliases.1.[key]. The profile therefore appears to save successfully but cannot be read or launched, contradicting the route's guarantee that invalid profiles never reach disk. The schema limitation predates this PR, but making it the sole gate before the new persistence operation introduces the broken save path.

Validate the parsed metadata with the same AgentProfile model/load semantics before writing, or explicitly reject non-string YAML mapping keys for model fields that require string keys. Add an endpoint regression asserting these requests return 400 and leave no file.

P3: Rejection-shape test claims broader coverage than it provides

test/api/test_api_profile_surface.py:757

TestWriteRejectionShape says it covers “Every 400 from a write route,” but its loop only sends POST requests and only exercises validation-related failures. DELETE's unsafe-name path at src/cli_agent_orchestrator/api/main.py:2161 intentionally returns a bare-string detail, so the stated suite-wide contract is false even though the tested POST behavior is correct. Rename and document this as the POST/PUT validation-error shape, or parameterize the relevant POST and PUT cases and explicitly exclude service/DELETE errors.

Validation

• Confirmed the clean detached checkout and head SHA; there are zero commits after the prior reviewed SHA.
• Read the acquisition metadata, full diff, commit analysis, and context report, then inspected the changed implementation, callers, schema/model, and tests.
• uv run --frozen python -m pytest -q test/api/test_api_profile_surface.py test/api/test_scope_coverage.py test/services/test_profile_store.py test/services/test_profile_validator.py test/utils/test_atomic_file.py: 180 passed, 3 dependency deprecation warnings.
• Custom HTTP reproduction: both non-string-key profiles returned 201, persisted, and failed parse_agent_profile_text with Pydantic key validation errors.
• git diff --check 135e7ff865226955ad67059181b4ea5939c9ae6e..HEAD: passed.
• Acquisition also reports all 24 GitHub checks successful. Current origin/main still produces a merge conflict in api/main.py; the eventual conflict resolution will need separate validation.

[sujoydc]

Thanks, both confirmed and fixed. I reproduced each before changing anything.

P2, write validation can persist profiles the runtime cannot load

Confirmed exactly as described. Both shapes produce zero validator findings,
persist, then fail to load:

mcpServers non-string key    validator errors: 0   would persist: True
                             later load: ValidationError  mcpServers.1.[key]  Input should be a valid string
toolAliases non-string key   validator errors: 0   would persist: True
                             later load: ValidationError  toolAliases.1.[key]
control: valid profile       validator errors: 0   would persist: True   later load: OK

The mismatch is format-level: the body is YAML, which allows any scalar as a
mapping key, and the gate is JSON Schema, where object keys are strings by
definition. Your framing of ownership is right too. The schema limitation
predates this PR; what this PR did was make that schema the sole gate in front of
a new persistence operation, which turned a latent gap into a broken save path.

validate_frontmatter now walks the parsed document and reports any non-string
mapping key as an error. Two choices worth surfacing:

It lives in the validator, not only on the write path. Otherwise
cao profile validate and POST /agents/profiles/validate would call such a
document valid while the write routes reject it, and the UI in PR C validates
before saving, so it would show a contradiction. That does mean this PR now
touches the #575 service, which I'd rather state than have you find.

It checks the key type generally rather than enumerating the object-valued
fields. YAML auto-types more than integers: an unquoted 2026-01-01: key becomes
a datetime.date and fails identically. That case is in the tests.

On your first suggestion, validating the parsed metadata through AgentProfile:
I looked at it and did not take it, because it catches strictly more but collides
with the unresolved-text decision from point 1. The write path persists
unresolved content, so model-validating unresolved would reject
provider_init_timeout: ${TIMEOUT} that the runtime accepts after
resolve_env_vars. Model-validating resolved content instead makes acceptance
depend on the server's environment, so the same document validates on one machine
and not another. Neither is obviously right, so it is a disclosed gap in the
description rather than a decision made here. Say the word if you'd rather it
land in this PR.

Endpoint regressions added as you asked: 400 with nothing on disk for POST, and
400 with the original surviving byte-for-byte for PUT, across all three key
shapes. Plus one asserting the rejected document genuinely fails
parse_agent_profile_text, so the rule reads as a reason rather than an
arbitrary restriction.

P3, rejection-shape test

Correct, and worse than one route. Enumerating every 400 across the four routes
at the reviewed head 167d46c found four bare-string sites, not just DELETE's:

POST   /agents/profiles              InvalidProfileNameError
PUT    /agents/profiles/{name}       InvalidProfileNameError
DELETE /agents/profiles/{name}       InvalidProfileNameError
GET    /agents/profiles/{name}/source   ValueError

Only the validation path produced the dict. So the docstring was false in four
places, and the PR description carried the same claim. DELETE was the reachable
one for the reason you gave, that it has no body to validate first.

I took the other branch of your suggestion and made the claim true rather than
narrowing it. The shape moved to a module-level _profile_write_rejection and all
four use it, since an unsafe name is a rejected input in the same class as a
schema violation, and returning a dict for one and a string for the other is
exactly the type-switching the unification was meant to remove. PR C is the
client that would have paid for that. TestWriteRejectionShape is now
parameterized across POST, PUT and DELETE, including the service-raised
name error. 404 and 409 stay bare strings and that is now documented rather than
implied.

Worth naming the pattern rather than only the instance: this is the third
assertion in this PR that promised more than the code delivered, after the
replace_profile docstring and the concurrency test named for a deleter it never
exercised. Same failure mode each time, prose stating a contract with nothing
executable holding it to account. The parameterization is the actual fix.

Merge, since you flagged it needs separate validation

Merged origin/main at c64c9fa, which brings #526, #545 and #539. One conflict,
the typing import in api/main.py: this branch added Sequence and #526 added
AsyncIterator. Resolved to the union of both, then reformatted by black.
docs/api.md and test/api/test_scope_coverage.py auto-merged.

Verified on the merged tree:

• the five changed test modules plus test/cli/test_profile_cmd.py: 216 passed
• test/api/test_scope_coverage.py: 33 passed
• full suite: 6,691 passed
• black and isort clean across 548 files
• mypy on the changed validator: the two errors it reports are byte-identical at
  the prior head, so pre-existing

One aside in case you hit it too: test_scope_coverage.py had ten failures
locally against 0903561, all in the new workflow-run and terminal-output tests.
They are fixed upstream by c64c9fa and no longer reproduce on pristine main.

71 net new tests, rebaselined on c64c9fa since #526 added its own tests to
test_scope_coverage.py. The description has a Review round 2 section with the
detail.

[sujoydc]

Please review again - @haofeif and @fanhongy

[fanhongy]

LGTM

[haofeif]

Correction after consistency re-check: the earlier source-route authorization and PUT/DELETE race findings are fixed. Two P2 issues remain: the write gate rejects URL-based MCP entries that CAO explicitly supports, and the new recursive YAML-key check can do exponentially growing work on the unauthenticated validation endpoint. I have reclassified the typed-placeholder case as a non-blocking, pre-existing validator limitation rather than a new P2 in this PR.

[haofeif]

[P2] Accept the URL-based MCP servers CAO already supports

This new hard gate rejects a profile containing a standard remote MCP entry such as docs: {type: http, url: https://mcp.example.invalid/mcp}. parse_agent_profile_text accepts it, and resolve_mcp_server_config explicitly preserves commandless URL/transport entries, but this schema check reports 'command' is a required property, so both POST and PUT return 400. This prevents users from creating or editing profiles for a supported MCP form. Please allow URL/SSE entries as well as command-based entries before making this schema a blocking write check.

[haofeif]

Correction — non-blocking follow-up

The behavior above is reproducible, but I should not have classified it as a new P2 in this PR. The shared validator already rejects placeholders in typed schema fields, this limitation was explicitly disclosed before this re-review, and typed placeholders are not a documented profile form. Placeholder-aware validation is worth defining separately, but it should not block this write-endpoint PR.

[haofeif]

[P2] Bound traversal of repeated YAML aliases

The depth limit does not limit the total work here because yaml.safe_load represents every alias as another reference to the same Python object, and this recursion walks that object again for every reference. A 529-byte doubling alias chain at depth 20 took about 1.5 seconds in this function, with each extra level roughly doubling the time. Since the unauthenticated async /agents/profiles/validate route calls this synchronous traversal directly, small requests can stall the server for an unbounded time. Please track already-visited container identities, reject repeated/cyclic aliases, or enforce a global node budget rather than relying only on depth.

[sujoydc]

P2a: URL-based MCP entries

@haofeif , You were right, and the codebase documented the contradiction itself. resolve_mcp_server_config's docstring says commandless {"type": "http", "url": ...} entries pass through untouched, while the schema had "required": ["command"] unconditionally, and this PR is what turned that into a blocking write gate.

Your exact example now validates and survives the round trip:

mcpServers: {docs: {type: http, url: https://mcp.example.invalid/mcp}}

  validator errors: 0
  parse_agent_profile_text: OK
  resolve_mcp_server_config -> {'type': 'http', 'url': 'https://mcp.example.invalid/mcp'}

mcpServers entries now require command or url via anyOf, with url declared as a property. Declaring it matters beyond documentation: the inner object doesn't set additionalProperties: false, so a url entry would have passed even undeclared, but then GET /agents/profiles/schema wouldn't describe the field and a wrong type wouldn't be a finding.

http url                -> accept
sse url                 -> accept
command                 -> accept
command + url together  -> accept
neither                 -> REJECT at mcpServers.docs
url: 7                  -> REJECT at mcpServers.docs.url

anyOf rather than oneOf deliberately, so an entry carrying both keys is still accepted. Nothing asked for exclusivity.

Two limits worth naming rather than leaving you to find:

url is the only spelling the branch accepts, and it appears exactly once in the repo as an MCP field name, in that docstring. An entry naming its endpoint as httpUrl or similar satisfies neither branch and is rejected. Still strictly better than before, which rejected every remote form, but it's a bet on one source. If the providers accept other spellings, say so and I'll widen it.

The rejection message for an entry with neither key is jsonschema's generic is not valid under any of the given schemas. It names the exact entry and path but not the missing key. if/then would produce 'url' is a required property, which reads better and is misleading, since command would also satisfy the rule. I kept the honest-but-vague message rather than adding message-rewriting to the validator. If you'd rather have the better text, expanding error.context for combinator failures is a small general change and I'll do it here.

P2b: bounded traversal, and the vector behind it

Your mechanism was exactly right and I reproduced it independently before changing anything: 640 bytes at 20 levels, ~1s, doubling per level, against ~0s for the jsonschema step beside it. Close enough to your 529 bytes / 1.5s to be the same thing. The depth cap bounded the wrong dimension. Depth was never the problem, revisiting shared objects was.

Of your three suggested remedies I took the first, tracking visited container identities, and added a global budget. I deliberately did not reject repeated or cyclic aliases. Anchor reuse is ordinary YAML and rejecting it would break documents that are fine:

toolsSettings:
  common: &c {timeout: 30}
  fs: *c
  web: *c          -> clean

toolsSettings: &s {self: *s}   (self-referential)  -> clean

Both are pinned by tests so a later "fix" can't satisfy the bound by refusing anchors.

Then I strawmanned that fix and found the larger half. jsonschema builds every error message eagerly, interpolating repr of the offending instance. So a document whose expansion is exponential in its byte count produces an error message that is too. Plant one type error whose instance is the amplified node:

anchor levels	body bytes	longest single message
20	651	25 MB
22	713	101 MB

2x per level, so ~26 levels reaches gigabytes, and that string gets serialised into the response. Allocation is the ceiling rather than CPU, and nothing done inside my traversal avoids it. On the 22-level document the identity-memoized walk costs 0.00003s while iter_errors is what explodes.

This one is pre-existing, not introduced by this PR. Verified against current origin/main at e1f6440 with the checkout's own source on PYTHONPATH so the tree under test is the one being imported:

imported from: <worktree>/src/cli_agent_orchestrator/services/profile_validator.py
has round-4 gate: False
levels=20 bytes=651 time=0.351s longest_msg=25,165,836
levels=22 bytes=713 time=1.422s longest_msg=100,663,308

The route was already scope-exempt and already async there, so this shipped with #575.

I fixed it here anyway. It's the same route and the same class of bug as the finding it sits behind, and telling you "the traversal is bounded" while a larger vector remained on that endpoint would have been half an answer. If you'd rather it went to its own issue against main so this PR stays scoped to the write endpoints, I'll split it out, no argument.

The final shape is one ceiling on expanded size, ahead of both the walk and the schema step, memoized on id() so the count stays linear in distinct objects and capped so an enormous document costs no more to reject than a borderline one. validate_frontmatter returns as soon as it reports, because continuing would pay exactly the cost the ceiling exists to avoid. After:

levels= 20  ->  1 finding, 171-char message
levels= 22  ->  1 finding, 171-char message
levels=200  ->  1 finding, 171-char message

Milliseconds, and flat as levels grow.

Two corrections to my own first attempt at this

Both were in the commit that fixed your finding, so they'd have landed under your name if you hadn't looked twice.

The budget decremented per edge traversed while being named _MAX_WALK_VALUES and reporting holds more than 20000 values. An 84 KB document holding four values, one of them aliased 21,000 times, was rejected for holding twenty thousand of them. Inside the 256 KB content cap, so reachable. Expressing the ceiling as expanded size fixed this at the root rather than by rewording: that document does expand to ~21,004 values, which is what the message now says.

The comment called both ceilings "~1000x" the largest bundled profile. Against its 23 expanded values and depth of 3 they are ~870x and ~21x. Wrong by roughly 50x for depth. Stated separately now.

I also documented something I'd left implicit: the two halves of validate_frontmatter report shared values differently. A shared value that's schema-invalid yields one finding per referencing path, since jsonschema doesn't memoize, while a shared non-string key yields exactly one. Both are correct, but a client rendering findings against a document shouldn't assume one convention.

Tests for all of this are deterministic rather than timed. The bomb is now rejected rather than traversed quickly, so the assertions are on the error message and on response size, under 2 KB for a body that previously returned 25 MB. If the memoization regresses, a test fails instead of hanging to the job timeout, which is what the earlier timing-only assertions would have done.

P2c

Noted, and thanks for revisiting it. No action taken. The Known gaps entry on placeholder-aware validation stays as it was, and I agree it wants its own definition rather than riding along here.

One interaction with #606

#606 gated GET /agents/profiles and GET /agents/profiles/{name}, which are the pre-existing ungated siblings this PR deliberately left alone when it gated /source, on the #505 precedent that tightening a shipped route risks breaking an existing unauthenticated reader. That asymmetry is gone now, so the rationale recorded on the route and in docs/api.md said something that stopped being true at the merge. Both rewritten.

I also added GET /agents/profiles/{name}/source to #606's _GATED_ROUTES and its sample requests, so it's covered by that file's auth-enabled enforcement tests instead of only by this PR's structural one. It belongs there on #606's own definition, arguably more than the parsed route does, since it returns stored bytes verbatim from the local, provider, extra and built-in stores including documents that fail to parse. That list is hand-maintained by design, so a route added later doesn't join it automatically. Flagging it rather than editing another PR's test file quietly.

Verification

Focused, 7 modules	347 passed
Full suite, merged tree	6,939 passed
black / isort	clean, 554 files
Net new tests	98, measured per file with --collect-only against e1f6440

origin/main is merged in as of e1f6440, so #608, #622 and #596 are included. #596 edits the same profile schema this PR does, in a different region, and merged clean; I checked that both sides' additions are present, that the #575 schema/model parity test still passes, and that #596's own profile validates clean through the ceiling added here.

The PR description is rewritten with a Review round 4 section carrying the measurements above.

[haofeif]

Re-reviewed current head 8bda8ec06ed987a855d768d340bae43ac4bf5f1f from scratch against current main. The source-read scope gate, DELETE/PUT serialization, and URL/SSE MCP round-trip fixes now hold. Two blocking gaps remain in the new structural guard, detailed inline: scalar aliases can still amplify a small unauthenticated validation request into gigabytes of schema output, and cyclic YAML is accepted and persisted even though the provider JSON path cannot serialize it.

[haofeif]

[P1] Bound rendered scalar bytes, not only value occurrences

Every scalar returns 1 here, but the downstream cost this guard is meant to bound is repr(instance), which includes the scalar bytes again for every alias. On this head, a 22,136-character profile containing one 2,048-character &s scalar and 5,000 aliases to it in a schema-invalid toolsSettings list passes _structural_bound_finding; unauthenticated POST /agents/profiles/validate returns a 10,260,109-byte response. More importantly, a 250,088-character request (below the 262,144 cap) with a 190,000-character scalar and 15,000 aliases is also accepted at roughly 15,006 counted values, while the one jsonschema instance representation has a 2.85 GB lower bound. Because this route is scope-exempt and runs synchronously inside an async handler, one request can still exhaust process memory and block every request. Please include scalar byte length in the expansion budget, or otherwise bound/sanitize schema error rendering before it is serialized.

[haofeif]

[P3] Reject cyclic graphs instead of counting a back-edge as finite

The provisional memo entry makes a self-reference contribute 1, and the new regression test consequently treats this document as valid:

name: cyc
description: cyclic
toolsSettings: &c {self: *c}

POST /agents/profiles returns 201 and persists it, but the normal Kiro materialization path passes profile.toolsSettings into KiroAgentConfig and model_dump_json() at install_service.py:379, which raises PydanticSerializationError: Circular reference detected (id repeated). The write gate therefore still succeeds for a profile the runtime cannot materialize. A cycle also has no finite fully-expanded size. Please track in-progress identities separately from completed memo entries and return a validation error on a back-edge, while retaining memoization for ordinary shared acyclic subtrees.

[haofeif]

Severity correction after impact/likelihood triage: the scalar-alias amplification is P1 because one unauthenticated, under-limit request can force multi-gigabyte allocation and take down the server. The cyclic-YAML finding is P3 / non-blocking because it requires deliberately self-referential input and affects only that profile. The Changes Requested verdict remains because the P1 is independently blocking.