Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new specification document was added that defines the architecture for skills and workflows in Claude Code/ACP, including discovery mechanisms, installation models, session layering, marketplace integration, authentication, and local usage patterns via a load-workflow skill. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/skills-and-workflows.md`:
- Around line 235-236: The spec and frontend disagree on the workflow metadata
route (spec uses "/content/workflow-metadata" while frontend calls
"/projects/{project}/agentic-sessions/{session}/workflow/metadata"); pick one
canonical route (e.g., the project-scoped
"/projects/{project}/agentic-sessions/{session}/workflow/metadata") and update
the spec and server implementation to match, then add backward-compatible
handling in the server (accept the legacy "/content/workflow-metadata" and
respond with a 301/307 redirect or return the same payload under the new route)
and update any relevant references (e.g., the frontend service that calls
workflow/metadata and DATA_MODEL_COMPARISON documentation) to ensure a single
source of truth and prevent drift.
- Around line 166-173: Update the spec for /workspace/sources/{name} to define a
deterministic, safe naming scheme: specify a canonical source ID generated from
host+owner+repo+ref+path (e.g., host|owner|repo|branch|subpath) with a stable
hash suffix for uniqueness, normalize to lowercase, strip/replace unsafe
characters (disallow path separators like '/' or '..'), enforce a max length,
and on collision append a short unique hash; state that this canonical ID is
used as the directory name under /workspace/sources/{name} and when constructing
add_dirs for the Claude Agent SDK so imports are deterministic and safe.
- Around line 148-151: The spec requires storing workspace-level installed
sources in the ProjectSettings CR but the CRD lacks a matching field; update the
ProjectSettings CRD (components/manifests/base/crds/projectsettings-crd.yaml) to
include a top-level `sources` (or `installedSources`) schema (array of objects
with fields like id/name/version/registry/installedAt and appropriate types),
mark it optional for backward compatibility, and add a migration note in the
spec describing how to handle existing ProjectSettings (tolerate absent field,
populate from runtime registry on upgrade, and provide a one-time migration to
backfill values). Also update any references in the spec text to reference the
exact schema shape and backward-compat expectations so the feature is
implementable.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 077df6b6-e5e2-4f58-8cd0-04a80496c833
📒 Files selected for processing (1)
specs/skills-and-workflows.md
specs/skills-and-workflows.md
Outdated
Comment on lines
+148
to
+151
| Source references installed at the workspace level are stored in the ProjectSettings CR. These represent the workspace's **registry** — what's available to sessions. | ||
|
|
||
| The registry is NOT auto-injected into every session. At session creation, users select which sources to load from the registry. The workflow they choose also pulls in its own dependencies from the `sources` array in `ambient.json`. | ||
|
|
Contributor
There was a problem hiding this comment.
ProjectSettings storage is specified without a matching CRD contract.
This section makes workspace-level source registry a required persisted field, but the current ProjectSettings CRD schema (see components/manifests/base/crds/projectsettings-crd.yaml:1-95) does not define any sources/installed-source field. Please add the required schema shape and migration/backward-compat expectations in this spec, or this is not implementable as written.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/skills-and-workflows.md` around lines 148 - 151, The spec requires
storing workspace-level installed sources in the ProjectSettings CR but the CRD
lacks a matching field; update the ProjectSettings CRD
(components/manifests/base/crds/projectsettings-crd.yaml) to include a top-level
`sources` (or `installedSources`) schema (array of objects with fields like
id/name/version/registry/installedAt and appropriate types), mark it optional
for backward compatibility, and add a migration note in the spec describing how
to handle existing ProjectSettings (tolerate absent field, populate from runtime
registry on upgrade, and provide a one-time migration to backfill values). Also
update any references in the spec text to reference the exact schema shape and
backward-compat expectations so the feature is implementable.
specs/skills-and-workflows.md
Outdated
Comment on lines
+166
to
+173
| Each source gets its own directory at `/workspace/sources/{name}/`, added to the Claude Agent SDK as a separate `--add-dir`. Claude Code discovers `.claude/skills/`, `.claude/commands/`, `.claude/agents/` inside each add_dir automatically via its standard discovery mechanism. | ||
|
|
||
| This mirrors how repos work (`/workspace/repos/{name}/`). Benefits: | ||
|
|
||
| - **Clean separation** — each source is isolated, easy to see what came from where | ||
| - **No conflicts** — two sources with a skill named `review` coexist in separate add_dirs | ||
| - **Simple removal** — removing a source is `rm -rf /workspace/sources/{name}/` | ||
| - **Standard mechanism** — `add_dirs` is exactly what Claude Code designed for external skill loading |
Contributor
There was a problem hiding this comment.
/workspace/sources/{name} needs a deterministic and safe naming spec.
Using {name} as the clone directory key is underspecified for custom imports and can cause collisions (different repos/branches resolving to same name) or unsafe path handling if not normalized. Please specify canonical source ID generation (e.g., host+repo+path hash) and path-sanitization rules.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🧰 Tools
🪛 LanguageTool
[style] ~173-~173: Consider an alternative for the overused word “exactly”.
Context: ... Standard mechanism — add_dirs is exactly what Claude Code designed for external ...
(EXACTLY_PRECISELY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/skills-and-workflows.md` around lines 166 - 173, Update the spec for
/workspace/sources/{name} to define a deterministic, safe naming scheme: specify
a canonical source ID generated from host+owner+repo+ref+path (e.g.,
host|owner|repo|branch|subpath) with a stable hash suffix for uniqueness,
normalize to lowercase, strip/replace unsafe characters (disallow path
separators like '/' or '..'), enforce a max length, and on collision append a
short unique hash; state that this canonical ID is used as the directory name
under /workspace/sources/{name} and when constructing add_dirs for the Claude
Agent SDK so imports are deterministic and safe.
specs/skills-and-workflows.md
Outdated
Comment on lines
+235
to
+236
| The runner's `/content/workflow-metadata` endpoint returns all discovered skills, commands, and agents across all loaded sources and built-in Claude Code skills. The frontend uses this to populate the Skills toolbar button and `/` autocomplete in the chat input. | ||
|
|
Contributor
There was a problem hiding this comment.
Workflow metadata endpoint conflicts with existing API contract.
The spec names /content/workflow-metadata, but frontend code currently calls /projects/{project}/agentic-sessions/{session}/workflow/metadata (components/frontend/src/services/api/workflows.ts:1-61), and components/ambient-api-server/DATA_MODEL_COMPARISON.md:1-50 marks workflow metadata as missing. Please define one canonical route (and compatibility/transition behavior) to prevent frontend/backend drift.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/skills-and-workflows.md` around lines 235 - 236, The spec and frontend
disagree on the workflow metadata route (spec uses "/content/workflow-metadata"
while frontend calls
"/projects/{project}/agentic-sessions/{session}/workflow/metadata"); pick one
canonical route (e.g., the project-scoped
"/projects/{project}/agentic-sessions/{session}/workflow/metadata") and update
the spec and server implementation to match, then add backward-compatible
handling in the server (accept the legacy "/content/workflow-metadata" and
respond with a 301/307 redirect or return the same payload under the new route)
and update any relevant references (e.g., the frontend service that calls
workflow/metadata and DATA_MODEL_COMPARISON documentation) to ensure a single
source of truth and prevent drift.
Gkrumbach07
force-pushed
the
spec/skills-and-workflows
branch
from
0dbdde8 to
9b1d8b4
Compare
Define the architecture for how skills, workflows, and agents work in ACP: - Workflow = prompt + list of skill source references (not embedded copies) - Skills are the atomic reusable unit, discovered by Claude Code from add_dirs - Manifest format (workflow.yaml) is the core contract — consumed by ACP runner, local load-workflow skill, or Claude Code plugin - Three pillars: discovery (marketplace), installation (workspace library with per-session selection), usage (layered loading into add_dirs) - Local portability: same manifest works with manual --add-dir, a meta-skill, or a Claude Code SessionStart hook plugin
…ient.json
Resolved all reviewer comments:
- Drop systemPrompt/startupPrompt from ambient.json, prompt lives in CLAUDE.md only
- Remove workflow.yaml concept, sources go in ambient.json
- Remove manifest-as-contract/multiple-consumers framing
- Sources support both structured {url, branch, path} and single URL string
- Scanner expects three source types: Claude Code plugins, marketplace
catalogs, and standalone .claude repos
- Keep rubric as-is (optional, existing workflows use it)
- Auth for private sources uses existing workspace credential system
- Multi-agent orchestration noted as out of scope
- Local usage simplified to manual --add-dir or /load-workflow skill
Major changes: - Everything is a source reference, no type distinction - Marketplace behind feature flag - Workflow builder removed (not MVP), replaced with visualization - Skills go where Claude expects (not file-uploads), exact path TBD - Versioning: branch (auto-update) + optional sha/tag (pinning) - Selection: registry = available, session picks, 'always add' TBD - Removed from scope: plugin export, RHAI alignment, security/supply chain - All examples in JSON (not YAML), ambient.json only - Local usage simplified to manual --add-dir or /load-workflow skill
Each source gets /workspace/sources/{name}/ as its own add_dir.
Claude Code discovers .claude/skills/ inside each automatically.
Clean separation, no conflicts, simple removal, standard mechanism.
Mirrors how repos work (/workspace/repos/{name}/).
Removed skill storage path from open questions (resolved).
Plugins (plugin.json, marketplace.json) are read by the marketplace for
browsing and display. At runtime, everything is a source reference —
cloned to /workspace/sources/{name}/ and added to add_dirs. No plugin
hooks, MCP, or LSP at session runtime.
Defines the architecture for how skills, workflows, and agents work in ACP. **Core insight**: A workflow is just a prompt + a list of skill source references. Skills are the atomic reusable unit. The manifest format is the contract — ACP and local tooling are just consumers. - **Workflows don't contain skills** — they reference them by Git URL + path - **Skills are always fetched from canonical sources** at load time, not embedded copies - **Workspace library != auto-inject** — installing to workspace makes items available, not active. Users select what loads per session. - **Same manifest works everywhere** — ACP runner, local `--add-dir`, a `/load-workflow` meta-skill, or a Claude Code plugin - **Layered loading** — workflow dependencies + user-selected extras + live imports, all fed into `add_dirs` 1. **Core Concepts** — Skill, Workflow, Agent, Manifest Format 2. **Discovery** — Marketplace, scanning, catalog format 3. **Installation & Configuration** — Workspace level, session level, workflow builder, selection model 4. **Usage in Sessions** — Loading order, runtime management, metadata 5. **Local Usage** — Manual, load-workflow skill, Claude Code plugin 6. **Manifest Format** — Proposed YAML schema 7. **Open Questions** — Format, versioning, plugin compat, RHAI alignment, security Looking for feedback on: - The manifest format (extend ambient.json vs new workflow.yaml?) - The selection model (workspace library vs auto-inject) - The Agent concept (persona + workflows + skills) - RHAI alignment (RHAIRFE-1370) --------- Co-authored-by: Ambient Code Bot <bot@ambient-code.local>
Gkrumbach07
force-pushed
the
spec/skills-and-workflows
branch
from
9b1d8b4 to
f00769c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.