feat(spec): add AgenticApplication to data model spec#1648
feat(spec): add AgenticApplication to data model spec#1648markturansky wants to merge 2 commits into
Conversation
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR documents a new ChangesAgenticApplication GitOps Specification
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/internal/design/ambient-model.spec.md`:
- Around line 268-269: The doc currently contradicts itself about the scope of
apply -k vs AgenticApplication; update the wording so it’s explicit and
consistent: either change the AgenticApplication sentence to say it syncs only
"project-scoped fleet definitions (a subset of resource kinds handled by acpctl
apply -k)" or change the apply -k description (the block describing
Cluster/Ambient support) to clarify those kinds are supported by acpctl but
intentionally excluded from AgenticApplication; update both the
AgenticApplication paragraph (mentioning AgenticApplication by name) and the
apply -k paragraph (the block describing Cluster and Ambient support) so they
state the chosen boundary unambiguously.
- Around line 324-325: Several operation examples use unprefixed paths (e.g.,
"POST /agentic_applications/{id}/sync" and "POST
/projects/{id}/agents/{agent_id}/ignite") while the API reference uses
fully-qualified paths; update all examples to the canonical fully-qualified
prefix "/api/ambient/v1" so examples match the API reference. Locate and replace
the unprefixed examples in the spec (including the occurrences referenced by the
reviewer) with their normalized forms like "POST
/api/ambient/v1/agentic_applications/{id}/sync" and "POST
/api/ambient/v1/projects/{id}/agents/{agent_id}/ignite" and ensure other example
paths in the same document follow this same fully-qualified pattern.
- Around line 417-423: The spec mixes two canonical session APIs (/ignite vs
/start) causing drift; pick one canonical endpoint and update all occurrences
and related sections to match the OpenAPI source-of-truth. Replace every
reference to `/ignite` or `ignite` (e.g., "created via `POST
/projects/{project_id}/agents/{agent_id}/ignite`" and lines referencing ignite
at 417 and 722–724) with the chosen canonical verb (`/start` or `/ignite`) and
likewise update the implementation matrix entries (the Sessions start/stop
endpoints at ~1080–1082), the API reference, the CLI mapping, and any examples
mentioning Session.prompt vs Agent.prompt so entity semantics remain consistent
with the openapi/openapi.yaml. Ensure the Session creation wording (Sessions are
not directly creatable) matches the chosen contract and that generated SDK/type
names in the repo reflect the selected endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 235e1956-d598-488d-92de-26f389a0803f
📒 Files selected for processing (1)
docs/internal/design/ambient-model.spec.md
| An AgenticApplication syncs **project-scoped fleet definitions** — the same resource kinds that `acpctl apply -k` handles: | ||
|
|
There was a problem hiding this comment.
apply -k scope is internally contradictory
Line 268 says AgenticApplication syncs “the same resource kinds that acpctl apply -k handles” and then scopes that to project resources, but Line 560-566 documents apply supporting Cluster and Ambient too. This contradiction changes GitOps boundary semantics and should be resolved explicitly (either narrow apply -k in this section or restate that AgenticApplication intentionally uses a subset of apply-supported kinds).
Also applies to: 560-566
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/internal/design/ambient-model.spec.md` around lines 268 - 269, The doc
currently contradicts itself about the scope of apply -k vs AgenticApplication;
update the wording so it’s explicit and consistent: either change the
AgenticApplication sentence to say it syncs only "project-scoped fleet
definitions (a subset of resource kinds handled by acpctl apply -k)" or change
the apply -k description (the block describing Cluster/Ambient support) to
clarify those kinds are supported by acpctl but intentionally excluded from
AgenticApplication; update both the AgenticApplication paragraph (mentioning
AgenticApplication by name) and the apply -k paragraph (the block describing
Cluster and Ambient support) so they state the chosen boundary unambiguously.
| For automated sync (`auto_sync=true`), this lifecycle runs on a configurable polling interval (default: 3 minutes). For manual sync, it runs on `POST /agentic_applications/{id}/sync`. | ||
|
|
There was a problem hiding this comment.
Endpoint naming/prefix usage is inconsistent in examples
Several operation examples use unprefixed paths (POST /agentic_applications/{id}/sync, POST /projects/{id}/agents/{agent_id}/ignite) while the API reference uses /api/ambient/v1/.... Given the PR’s endpoint naming conformance objective, this should be normalized to one style (prefer fully-qualified API paths in spec examples).
Also applies to: 735-736, 801-802
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/internal/design/ambient-model.spec.md` around lines 324 - 325, Several
operation examples use unprefixed paths (e.g., "POST
/agentic_applications/{id}/sync" and "POST
/projects/{id}/agents/{agent_id}/ignite") while the API reference uses
fully-qualified paths; update all examples to the canonical fully-qualified
prefix "/api/ambient/v1" so examples match the API reference. Locate and replace
the unprefixed examples in the spec (including the occurrences referenced by the
reviewer) with their normalized forms like "POST
/api/ambient/v1/agentic_applications/{id}/sync" and "POST
/api/ambient/v1/projects/{id}/agents/{agent_id}/ignite" and ensure other example
paths in the same document follow this same fully-qualified pattern.
| Sessions are **not directly creatable**. They are run artifacts created exclusively via `POST /projects/{project_id}/agents/{agent_id}/ignite`. | ||
|
|
||
| `Session.prompt` scopes the task for this specific run — separate from `Agent.prompt` which defines who the agent is. | ||
|
|
||
| ``` | ||
| Project.prompt → "This workspace builds the Ambient platform API server in Go." | ||
| Agent.prompt → "You are a backend engineer specializing in Go APIs..." |
There was a problem hiding this comment.
Unify ignite vs start/stop contract terminology and endpoints
This spec currently defines sessions as created via /ignite (Line 417, Line 722), while the implementation matrix still lists Sessions start/stop endpoints (Line 1081), and generated API artifacts in-repo describe /start. Please pick one canonical contract and align all sections (entity semantics, API reference, CLI mapping, and coverage matrix) to avoid client/server drift.
Based on learnings: "The OpenAPI specification in ../ambient-api-server/openapi/openapi.yaml is the source of truth for SDK types and client behavior".
Also applies to: 722-724, 1080-1082
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/internal/design/ambient-model.spec.md` around lines 417 - 423, The spec
mixes two canonical session APIs (/ignite vs /start) causing drift; pick one
canonical endpoint and update all occurrences and related sections to match the
OpenAPI source-of-truth. Replace every reference to `/ignite` or `ignite` (e.g.,
"created via `POST /projects/{project_id}/agents/{agent_id}/ignite`" and lines
referencing ignite at 417 and 722–724) with the chosen canonical verb (`/start`
or `/ignite`) and likewise update the implementation matrix entries (the
Sessions start/stop endpoints at ~1080–1082), the API reference, the CLI
mapping, and any examples mentioning Session.prompt vs Agent.prompt so entity
semantics remain consistent with the openapi/openapi.yaml. Ensure the Session
creation wording (Sessions are not directly creatable) matches the chosen
contract and that generated SDK/type names in the repo reflect the selected
endpoint.
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Spec review — AgenticApplication data model
Solid foundational spec. The Argo CD analogy is well-constructed, the ERD is clean, and the "what gets synced / what does not" distinction between fleet definitions and infrastructure inventory is well-argued. Several gaps need closing before implementation starts.
[SECURITY] RoleBinding sync bypasses escalation rules
The What Gets Synced table includes:
RoleBinding| Created if not present; idempotent by user+role+scope key
The rbac-enforcement spec (merged) establishes strict escalation rules: a principal can only grant roles at levels strictly below their own. Those rules bind human API callers — but the AgenticApplication sync controller is acting as a service principal against the destination Ambient's REST API. If the controller has a powerful service credential, it can create RoleBindings that no human caller would be permitted to create.
Concretely: whoever controls the source git repo controls the RBAC of the destination project. A malicious or compromised repo commit could grant project:owner or gitops:admin to an arbitrary user.
Options to address:
- Restrict sync scope — only allow the sync engine to create RoleBindings where the role is at or below
agent:observerin the hierarchy; any binding above that requires human approval (manual sync + approval gate). - Bind the sync engine by escalation rules — state explicitly that the sync engine must satisfy the same escalation rules as the service credential it uses. This means the service credential level sets the ceiling on what RoleBindings the sync can create.
- Exclude RoleBindings from auto-sync — mark RoleBindings as "manual review required" even when
auto_sync=true.
At minimum, add a design decision that names this risk explicitly.
[SECURITY] Remote Ambient authentication is not implementable as specified
authentication via forwarded token or service credential
AgenticApplication has no field for a credential. When destination_ambient_id is set, the sync engine needs a token for the remote Ambient's REST API. The existing security spec defines a Credential type (with encrypted storage and credential:owner RBAC). The correct field is something like credential_id FK → Credential.ID — the stored credential provides the auth context for the remote Ambient SDK client.
"Forwarded token" is also dangerous: if the sync runs as an async background controller (as implied by auto_sync polling), there is no request context to forward a token from. The controller must use a stored credential.
Add a credential_id field to AgenticApplication and a design decision explaining how the credential is resolved (same kubeconfig_enc-style write-only pattern as Cluster?).
[IMPORTANT] gitops:admin / gitops:viewer not integrated with escalation rules
Two new built-in roles are introduced but the escalation rule chain is not updated. The rbac-enforcement spec defines who can grant project:owner, cluster:admin, etc. Missing:
- Can
platform:admingrantgitops:admin? (Almost certainly yes — but unstated.) - Can
project:ownergrantgitops:admin? (Probably no — but unstated.) - At what scope can
gitops:adminbe granted? Global only? Or project-scoped? - Is
gitops:vieweruser-grantable or platform-internal?
Per the escalation spec pattern, platform-scoped roles (cluster:admin, and apparently gitops:admin) should only be grantable by platform:admin. Add this to the rbac-enforcement spec or add a normative statement here that gitops:admin / gitops:viewer are grantable only by platform:admin.
[IMPORTANT] Shared kustomize engine / Cluster + Ambient kind handling is underspecified
The spec states:
The sync engine reuses the same kustomize rendering logic as
acpctl apply -k. Extracted to a shared package.
But acpctl apply supports Cluster and Ambient as apply kinds, while the AgenticApplication sync explicitly excludes them ("not synced"). If a kustomize overlay contains a Cluster or Ambient manifest:
- Does the sync engine skip it silently?
- Does it warn in
operation_message? - Does it error and set
operation_phase: Failed?
Implementors need a clear answer. The shared engine model implies the same code path, which makes the divergence a runtime choice. Recommend: explicitly state that Cluster/Ambient documents in a kustomize tree are silently skipped by the sync engine (not applied), with a per-resource status entry of {"kind": "Ambient", "name": "...", "status": "Skipped", "message": "infrastructure inventory — not synced by AgenticApplication"}.
[IMPORTANT] auto_prune=true on a Project is destructive; spec doesn't warn
The prune behavior for Agents is expected. But if a git overlay removes the Project manifest itself and auto_prune=true, the sync would delete the project — cascading through all agents, their sessions, inbox messages, and session message history. This is irrecoverable.
Add an explicit design decision or warning box:
Pruning a Project is permanently destructive. All Agents, Sessions, Inbox messages, and SessionMessages in the project are deleted. The spec recommends setting
auto_prune=falseat the Project level, or implementing a "tombstone" pattern (mark project for deletion, require manual confirmation) before implementing project prune.
At minimum, note this in the auto_prune field description in the Field Reference.
[NOTABLE] project:owner cannot manage GitOps sync for their own project
The permission matrix shows project:owner has — for AgenticApplications. This means a team that fully owns a project must escalate to a gitops:admin to create a GitOps binding for their own project — even though they already have full control over the project contents.
In Argo CD, the equivalent is that a namespace owner can create an Application targeting their own namespace (if allowed by the AppProject policy). The current design requires a platform-level role for what is effectively a project-level operation.
This may be intentional (AgenticApplications have cross-environment reach that exceeds project scope — the Ambient destination could be in another environment). If intentional, add it as an explicit design decision: "gitops:admin is platform-scoped because AgenticApplications can target any Ambient, including production environments; project:owner scope would not be sufficient to authorize cross-environment sync."
If not intentional, consider allowing project:owner to create AgenticApplications where destination_project == their owned project AND destination_ambient_id == null (local only), with gitops:admin required for remote Ambient targets.
[NOTABLE] PUT vs PATCH inconsistency for Cluster and Ambient
Cluster and Ambient update endpoints are listed as PUT:
PUT /clusters/{id}
PUT /ambients/{id}
All other mutable resources use PATCH (partial update). PUT implies full replacement — callers must send the complete resource, not a diff. This is inconsistent with the pattern established for Projects, Agents, and AgenticApplications (all PATCH). Change to PATCH /clusters/{id} and PATCH /ambients/{id}.
[MINOR] Inbox dedup key uses from_name which is mutable
Inbox (seed messages) | Idempotent delivery — only new messages (by
from_name+bodydedup) are posted
from_name is "denormalized sender display name" — it can change if an agent is renamed. A seed message previously delivered under from_name="lead" would be re-delivered after an agent rename, breaking idempotency. Dedup by from_agent_id + body (where from_agent_id is the immutable FK) or by a content hash of the body alone.
[MINOR] Health status "expected state" is underspecified
Healthy= all agents exist and have expected state
"Expected state" needs definition. Candidates:
- Agent exists with exact prompt from git = Healthy
- Agent exists regardless of current prompt = Healthy (drift detected separately via
self_heal) - Agent exists and has an active session = Healthy
The third interpretation conflicts with the model ("only one session per agent; sessions are run artifacts"). Recommend the first or second. Define what triggers Degraded per resource kind.
What's correct
- ERD relationships: AgenticApplication → Ambient (nullable FK), AgenticApplication → Project (syncs_to) are correct.
- "What does not get synced" is well-argued and clear.
- Sync lifecycle (Refresh → Render → Diff → Sync → Status) is the right sequence.
- Destination resolution (null → local Ambient) is clean.
- Multi-environment promotion via multiple AgenticApplications is idiomatic and matches Argo CD's App-per-env pattern.
- All AgenticApplication endpoints follow existing naming conventions (
/api/ambient/v1/agentic_applications). gitops:adminnot having RBAC column permissions in the matrix is correct (the role manages apps, not bindings).- Design decisions section is comprehensive; the existing decisions are well-reasoned.
- Implementation coverage matrix rows for AgenticApplication correctly marked 🔲 planned throughout.
markturansky
force-pushed
the
feat/agentic-application-spec
branch
from
061e043 to
9f032dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
specs/api/ambient-model.spec.md (1)
311-320: ⚡ Quick winClarify Credential sync scope in "What Gets Synced" table.
Line 317 states "Credential | Created if not present; idempotent by name" but Credentials are global resources while AgenticApplication targets a single project. The binding mechanism (via RoleBinding sync in line 318) should be explicitly noted in the Credential row to avoid confusion.
📝 Suggested clarification
-| `Credential` | Created if not present; idempotent by name | +| `Credential` | Created globally if not present; idempotent by name. Access granted to destination project via RoleBinding sync. |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/api/ambient-model.spec.md` around lines 311 - 320, Update the "What Gets Synced" table entry for Credential to clarify scope: state that Credentials are global resources created if not present but that AgenticApplication only references or binds them into the target project via the RoleBinding sync (see RoleBinding row) — i.e., note "Credentials are global; AgenticApplication will create/ensure the global Credential by name and then bind it into the destination project via RoleBinding." Mention AgenticApplication, Credential, and RoleBinding to make the relationship explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/api/ambient-model.spec.md`:
- Around line 369-376: The spec currently references "authentication via
forwarded token or service credential" for
AgenticApplication.destination_ambient_url but the AgenticApplication entity
lacks any field to represent remote credentials; update the AgenticApplication
schema by adding either a nullable remote_credential_id (FK to Credential)
and/or a remote_token_secret_ref (k8s Secret reference) with clear semantics,
and/or explicitly document that "forwarded token" means the sync controller will
use its own service account token (include how token forwarding is initiated).
Ensure you update the AgenticApplication definition and accompanying prose to
state which field is used for which authentication mode and how the sync engine
chooses between forwarded token vs stored credential.
- Around line 886-895: The endpoint paths in the API spec use underscores
("agentic_applications") but should use hyphens ("agentic-applications"); update
every occurrence of the route path strings (e.g., GET/POST/PATCH/DELETE
/api/ambient/v1/agentic_applications and the subpaths /{id}/sync, /{id}/refresh,
/{id}/status) to use "agentic-applications" instead, and propagate this rename
to the route registration handlers, OpenAPI/spec file, tests, docs, and any CLI
or client references that expect "agentic_applications" so they match the new
hyphenated paths; ensure route handler names or identifiers (e.g.,
createAgenticApplication/listAgenticApplications or any router.register(...)
entries that reference "agentic_applications") are updated accordingly and run
tests to confirm no remaining references to the underscored path.
---
Nitpick comments:
In `@specs/api/ambient-model.spec.md`:
- Around line 311-320: Update the "What Gets Synced" table entry for Credential
to clarify scope: state that Credentials are global resources created if not
present but that AgenticApplication only references or binds them into the
target project via the RoleBinding sync (see RoleBinding row) — i.e., note
"Credentials are global; AgenticApplication will create/ensure the global
Credential by name and then bind it into the destination project via
RoleBinding." Mention AgenticApplication, Credential, and RoleBinding to make
the relationship explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 25f9117a-a67a-4b85-8265-800d3dc8c9dd
📒 Files selected for processing (1)
specs/api/ambient-model.spec.md
| ``` | ||
| AgenticApplication.destination_ambient_url set? | ||
| |── null ──> local Ambient (this API server's own service layer) | ||
| |── set ──> remote Ambient (SDK client pointed at the URL) | ||
| ──> authentication via forwarded token or service credential | ||
| ``` | ||
|
|
||
| When targeting a remote Ambient, the sync engine acts as an API client to the remote Ambient's REST API. This is different from how Sessions use kubeconfig for direct K8s provisioning — the AgenticApplication works entirely at the Ambient API layer. |
There was a problem hiding this comment.
Specify authentication mechanism for remote Ambient instances.
Line 373 mentions "authentication via forwarded token or service credential" but the entity schema (lines 231-257) has no field to store remote credentials. Without specifying how authentication works, implementers cannot build remote sync.
Options to consider:
- Add
remote_credential_idFK to reference an existing Credential - Add
remote_token_secret_reffield with k8s secret reference - Document that forwarded token means the sync controller uses its own service account token
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/api/ambient-model.spec.md` around lines 369 - 376, The spec currently
references "authentication via forwarded token or service credential" for
AgenticApplication.destination_ambient_url but the AgenticApplication entity
lacks any field to represent remote credentials; update the AgenticApplication
schema by adding either a nullable remote_credential_id (FK to Credential)
and/or a remote_token_secret_ref (k8s Secret reference) with clear semantics,
and/or explicitly document that "forwarded token" means the sync controller will
use its own service account token (include how token forwarding is initiated).
Ensure you update the AgenticApplication definition and accompanying prose to
state which field is used for which authentication mode and how the sync engine
chooses between forwarded token vs stored credential.
| GET /api/ambient/v1/agentic_applications list all agentic applications | ||
| POST /api/ambient/v1/agentic_applications create agentic application | ||
| GET /api/ambient/v1/agentic_applications/{id} read agentic application (includes status) | ||
| PATCH /api/ambient/v1/agentic_applications/{id} update agentic application | ||
| DELETE /api/ambient/v1/agentic_applications/{id} delete agentic application | ||
|
|
||
| POST /api/ambient/v1/agentic_applications/{id}/sync trigger sync (apply target state to live state) | ||
| POST /api/ambient/v1/agentic_applications/{id}/refresh refresh (fetch git, diff against live, update sync_status) | ||
| GET /api/ambient/v1/agentic_applications/{id}/status read sync/health status and per-resource detail | ||
| ``` |
There was a problem hiding this comment.
Use hyphens in API endpoint paths, not underscores.
The new AgenticApplication endpoints use agentic_applications (underscore), but existing multi-word resources use hyphens (scheduled-sessions). REST APIs conventionally favor hyphens in URLs. The CLI already uses agentic-application correctly.
🔧 Proposed fix
-GET /api/ambient/v1/agentic_applications list all agentic applications
-POST /api/ambient/v1/agentic_applications create agentic application
-GET /api/ambient/v1/agentic_applications/{id} read agentic application (includes status)
-PATCH /api/ambient/v1/agentic_applications/{id} update agentic application
-DELETE /api/ambient/v1/agentic_applications/{id} delete agentic application
+GET /api/ambient/v1/agentic-applications list all agentic applications
+POST /api/ambient/v1/agentic-applications create agentic application
+GET /api/ambient/v1/agentic-applications/{id} read agentic application (includes status)
+PATCH /api/ambient/v1/agentic-applications/{id} update agentic application
+DELETE /api/ambient/v1/agentic-applications/{id} delete agentic application
-POST /api/ambient/v1/agentic_applications/{id}/sync trigger sync (apply target state to live state)
-POST /api/ambient/v1/agentic_applications/{id}/refresh refresh (fetch git, diff against live, update sync_status)
-GET /api/ambient/v1/agentic_applications/{id}/status read sync/health status and per-resource detail
+POST /api/ambient/v1/agentic-applications/{id}/sync trigger sync (apply target state to live state)
+POST /api/ambient/v1/agentic-applications/{id}/refresh refresh (fetch git, diff against live, update sync_status)
+GET /api/ambient/v1/agentic-applications/{id}/status read sync/health status and per-resource detail📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GET /api/ambient/v1/agentic_applications list all agentic applications | |
| POST /api/ambient/v1/agentic_applications create agentic application | |
| GET /api/ambient/v1/agentic_applications/{id} read agentic application (includes status) | |
| PATCH /api/ambient/v1/agentic_applications/{id} update agentic application | |
| DELETE /api/ambient/v1/agentic_applications/{id} delete agentic application | |
| POST /api/ambient/v1/agentic_applications/{id}/sync trigger sync (apply target state to live state) | |
| POST /api/ambient/v1/agentic_applications/{id}/refresh refresh (fetch git, diff against live, update sync_status) | |
| GET /api/ambient/v1/agentic_applications/{id}/status read sync/health status and per-resource detail | |
| ``` | |
| GET /api/ambient/v1/agentic-applications list all agentic applications | |
| POST /api/ambient/v1/agentic-applications create agentic application | |
| GET /api/ambient/v1/agentic-applications/{id} read agentic application (includes status) | |
| PATCH /api/ambient/v1/agentic-applications/{id} update agentic application | |
| DELETE /api/ambient/v1/agentic-applications/{id} delete agentic application | |
| POST /api/ambient/v1/agentic-applications/{id}/sync trigger sync (apply target state to live state) | |
| POST /api/ambient/v1/agentic-applications/{id}/refresh refresh (fetch git, diff against live, update sync_status) | |
| GET /api/ambient/v1/agentic-applications/{id}/status read sync/health status and per-resource detail |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/api/ambient-model.spec.md` around lines 886 - 895, The endpoint paths
in the API spec use underscores ("agentic_applications") but should use hyphens
("agentic-applications"); update every occurrence of the route path strings
(e.g., GET/POST/PATCH/DELETE /api/ambient/v1/agentic_applications and the
subpaths /{id}/sync, /{id}/refresh, /{id}/status) to use "agentic-applications"
instead, and propagate this rename to the route registration handlers,
OpenAPI/spec file, tests, docs, and any CLI or client references that expect
"agentic_applications" so they match the new hyphenated paths; ensure route
handler names or identifiers (e.g.,
createAgenticApplication/listAgenticApplications or any router.register(...)
entries that reference "agentic_applications") are updated accordingly and run
tests to confirm no remaining references to the underscored path.
Review: feat(spec): add AgenticApplication to data model specTL;DR: Pure spec PR. The core approach — modeling AgenticApplication as "Argo CD for Ambient" — is the right abstraction. The design decisions are largely sound. A handful of gaps worth addressing before merge. What's Good
Concerns1. Credential sync is a security gap (high priority) The spec lists 2. Private git repo auth is unspecified
3.
4. Health vs. sync-correctness naming
5. The permission matrix says 6. No concurrency/locking spec If 7.
Minor
VerdictRight approach, draft-quality gaps to close. Approve the direction; request addressing Credential token handling (security), private git repo auth, and |
…odel spec Adds the AgenticApplication Kind — the Ambient equivalent of an Argo CD Application — to the platform data model spec. Defines source/destination binding, sync lifecycle, auto-sync/prune/self-heal policies, multi-environment promotion pattern, API endpoints, CLI commands, RBAC roles, and coverage matrix. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- SECURITY: Add credential_id FK for remote Ambient auth (no forwarded tokens) - SECURITY: Sync engine bound by credential escalation rules for RoleBindings - IMPORTANT: Project prune requires manual confirmation (auto_prune never deletes Projects) - IMPORTANT: Unsupported kinds (Cluster, Ambient) silently skipped with Skipped status - IMPORTANT: gitops:admin/viewer grantable only by platform:admin - NOTABLE: project:owner can create local-only AgenticApplications for own projects - MINOR: Inbox dedup uses immutable from_agent_id, not mutable from_name - MINOR: Health status fully defined with per-resource field-level drift detection 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
feat/agentic-application-spec
branch
from
2c00ab8 to
5a5fda8
Compare
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Amber Review — PR #1648: AgenticApplication data model spec
Spec-only review. Living-doc convention is correct throughout — all implementation items are marked 🔲 planned. Focused on internal consistency, completeness, and design gaps.
Issues requiring fix before merge
F1 — operation_phase value inconsistency: "idle" vs empty string
The ERD entity defines the field as:
string operation_phase "Succeeded | Failed | Running | idle"
The Field Reference table defines it as:
State of the last sync operation: Succeeded, Failed, Running, or empty if never synced
"idle" (ERD) and "" (field ref) are different values. Pick one and make both sections consistent. Argo CD uses empty string for the not-yet-synced case — recommend aligning with that.
F2 — ERD relationship implies FK that doesn't exist
The ERD includes:
AgenticApplication }o--o| Project : "syncs_to"
But destination_project is a name string (the field reference confirms: "Target project name"), not a destination_project_id FK. The ERD relationship implies a join column that doesn't exist. Either:
- Drop the
syncs_toERD relationship and replace with a comment noting it's by-name, not by FK, or - Add a
destination_project_idFK column (nullable) to the model
If it stays by-name: project renames will silently break sync — worth documenting explicitly in the field reference for destination_project.
F3 — No mechanism for private git repo authentication
The field reference for source_repo_url says "HTTPS or SSH." But credential_id is documented as auth for the destination Ambient API, not the source git repository. There's no field for git authentication. This means:
- Public HTTPS repos: work
- Private HTTPS repos: no credential mechanism defined
- SSH repos: no key mechanism defined
Argo CD solves this with a separate Repository object. Options for this spec:
- Add
source_credential_idFK → Credential (a Credential whose token is used as the git PAT / Basic auth for HTTPS, or holds an SSH private key for SSH) - Restrict v1 to public HTTPS repos and mark SSH + private repo auth as
🔲 planned
As written, advertising SSH support in the field reference is inaccurate since there's no auth mechanism for it.
Non-blocking observations
N1 — Agent auto-prune cascade to Sessions is undocumented
The auto_prune / Project prune safety section documents cascade behavior for Project deletion:
"All Agents, Sessions, Inbox messages, and SessionMessages in the project are cascade-deleted"
But Agent-level auto-prune cascade is not described. If an Agent is deleted via auto_prune, what happens to its active sessions, inbox messages, and session messages? Worth one sentence in the auto_prune field reference.
N2 — Polling interval is not a configurable field
The sync lifecycle section states "default: 3 minutes" for auto-sync polling. This implies a global constant, but it's not defined as a field on AgenticApplication. If the interval should be per-application (useful for high-frequency or low-priority fleets), it should be a field. If it's global config, note that explicitly. Currently, the spec implies it's configurable (the word "configurable") but provides no configuration surface.
N3 — Concurrent sync semantics undefined
If auto_sync=true fires while a manual POST /sync is in operation_phase: Running, the spec doesn't define what happens. Does the auto-sync skip? Queue? Fail? Worth a single sentence in the Sync Lifecycle section.
N4 — health_status conflates sync correctness with runtime health
The health_status definition: "Healthy = all synced resources exist in the destination and match the target state." This is really a sync-correctness check, not a runtime health check. An Agent could match git exactly but be in a failure state (all its sessions failing, no successful runs). Recommend documenting this explicitly: "For v1, health reflects sync correctness (resource exists and fields match git), not runtime agent health."
N5 — project:owner self-service authorization logic not specified
The permission matrix and design decision both say project:owner can create local AgenticApplications for their own project. But the authorization enforcement isn't specified: what checks does the handler perform to validate destination_ambient_url == null AND destination_project is owned by the requester? The runtime enforcement spec should define this gate, similar to how rbac-enforcement.spec.md defines the B1–B5 requirements. Noting this as a gap for the implementation spec, not a blocker here.
What's strong
Project prune safety is correct. The explicit block on auto-pruning a Project, and requiring prune: true, prune_project: true as dual opt-in flags on POST /sync, is the right design. Cascade destruction of a project requires intentional confirmation at every layer.
RoleBinding escalation ceiling is well-specified. The sync engine being bound by the credential's effective role level prevents a compromised git repo from escalating RBAC. This is the right invariant and it's clearly stated in both the sync table and the design decisions.
Remote Ambient auth via no-cache Credential is correct. Resolving the token fresh per sync cycle via GET /credentials/{id}/token and not caching it is the right pattern for async polling controllers. The design decision explaining why (no request context for async controllers) is the right level of documentation.
Argo CD concept mapping is clear. The table is accurate and useful for readers coming from an Argo CD background.
Living-doc convention is correct. All API, CLI, and coverage matrix rows are marked 🔲 planned. The > **Status:** Proposed is not used here but isn't needed since the entity is being added to the model spec (not claiming implementation status).
Fix F1 and F2 before merging (small). F3 needs a design decision — either add the field or scope-limit the claim. N1–N5 are recommended for this PR or a follow-up.
— Amber
2 participants
Summary
docs/internal/design/ambient-model.spec.md— a GitOps continuous sync resource (Argo CD for Ambient) that reconciles agent fleet definitions from a git repository to a target Ambient instance/sync,/refresh,/statussub-resources)acpctl get/create/sync/refresh agentic-application)gitops:adminandgitops:viewerRBAC roles with permission matrix columnDesign Highlights
< /dev/null | Argo CD | Ambient |
|---|---|
| Application | AgenticApplication |
| Cluster + Namespace | Ambient instance + Project |
| Helm / Kustomize / Jsonnet | Kustomize only (built-in engine from
acpctl apply -k) || K8s API targeting | Ambient REST API targeting (SDK client, not kubeconfig) |
| App of Apps | Multiple AgenticApplications per sector (s0→s5 promotion) |
Test plan
/api/ambient/v1/agentic_applications)🤖 Generated with Claude Code
Summary by CodeRabbit