spec: skills and workflows — discovery, installation, and usage#1117
spec: skills and workflows — discovery, installation, and usage#1117mergify[bot] merged 2 commits intomainfrom
Conversation
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
WalkthroughAdds a new "Skills & Workflows" specification describing discoverable Skills, Workflows as prompts plus source manifests, discovery/scanning rules, installation/session semantics, session loading, CLI/agent integration, and local usage patterns including Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Backend
participant Repo/Marketplace
participant AgentSDK
User->>CLI: request load/install workflow (--add-dir / select source)
CLI->>Backend: send source refs / add-dir info
Backend->>Repo/Marketplace: resolve/clone/scan sources
Repo/Marketplace-->>Backend: return discovered SKILL.md, CLAUDE.md, ambient.json
Backend->>Backend: resolve ambient.json.sources (load-time), merge session selections
Backend->>AgentSDK: provide workspace dirs (+ --add-dir, env var) and selected workflow
AgentSDK-->>User: workflow loaded and ready in session
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🤖 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 269-311: Update the manifest documentation to clarify current vs
proposed file paths and formats: explicitly state the Current state as the
existing `.ambient/n` JSON file and its schema, and the Proposed state as
`workflow.yaml` (or renamed `ambient.json`) using the camelCase fields shown
(`systemPrompt`, `startupPrompt`, `rubric`, `sources`); modify the text that
currently references incorrect filenames to mention `.ambient/n` or clearly
label the example as a proposed new format/migration plan so readers can find
the real implementation in ambient_runner/app.py and
ambient_runner/bridges/claude/tools.py.
- Line 62: Update the fenced code blocks in the markdown to include language
specifiers so syntax highlighting and tooling work: add `text` or `plaintext` to
the Agent formula code block (the fenced block labeled "Agent formula" around
the currently untagged block), add `text` to the fenced block showing the file
path, and add `bash` to the usage example fenced block (the usage example block
near the bottom); locate these by the "Agent formula" label and the nearby
file-path and usage-example sections and add the appropriate language
identifiers after the opening triple backticks.
- Line 23: The spec references the manifest path ".ambient/ambient.json" but the
implementation (ambient_runner/endpoints/content.py where config_path =
Path(workflow_dir) / ".ambient" / "n" and ambient_runner/app.py with the same
config_path) uses ".ambient/n"; fix the mismatch by either updating all spec
occurrences to ".ambient/n" to match the code, or updating the two config_path
uses to point to ".ambient/ambient.json" (and adjust any loader logic
accordingly); also state in the spec whether "workflow.yaml" is an alternative
supported format or a replacement so tooling knows which filename(s) to expect.
- Line 154: Choose and document a single design decision to resolve the conflict
between exporting workflows as a Claude Code plugin (`plugin.json`) and the
stated reference model ("the manifest holds references" / "skills are never
copied into the workflow"): either (A) remove plugin export support and
explicitly state workflows remain reference-only, (B) add a documented
copy/bundle mode for `plugin.json` that explicitly breaks the reference model
and describe its trade-offs, or (C) define a new plugin format that preserves
reference-based skill loading; then update the mention of `plugin.json`, the
manifest rule ("skills are never copied into the workflow"), and the Open
Questions section so the spec consistently reflects the chosen option and its
implications.
- Line 323: Update the spec wording that currently asks about extending
`ambient.json` to instead reference the actual implemented filename
`.ambient/n`, or explicitly state this is a proposal to rename/migrate from
`ambient.json` to `.ambient/n`; modify the sentence that lists `name`,
`description`, `systemPrompt`, `startupPrompt`, `rubric` and the suggested
additions `sources` and `prompt` so it refers to `.ambient/n` (or notes the
rename), ensuring consistency between the document and the implementation.
- Around line 331-332: Elevate the brief "Security" note into a dedicated
section titled "Security" that expands into three subsections: "Threat model"
(enumerate risks like malicious skill sources, MITM, compromised registries,
supply-chain attacks when cloning/executing Git-based skills and installing at
workspace/session levels), "Proposed mitigations" (specify commit SHA pinning,
signature/manifest verification, secure transport, sandboxing/containers,
dependency vulnerability scanning, least-privilege installs, and runtime
syscall/network restrictions), and "Enterprise requirements" (private
registries, allow-lists/block-lists, audit logs, provenance/signature
enforcement, and attestation of approved skill manifests); update any references
to the earlier one-line "Security" note so they point to the new section and
ensure the spec mentions where SHA pinning and signature verification are
enforced in the skill installation workflow (e.g., skill install/clone and
manifest validation steps).
- Around line 113-116: The spec text mismatches the code: update the scanning
section in specs/skills-and-workflows.md so it matches the actual implementation
by removing the unsupported `.claude/skills/*/SKILL.md` scan entry (or
alternatively implement skills scanning in the code if you intend to support
it), and correct the manifest path reference from `.ambient/ambient.json` to
`.ambient/n` so the documented workflow check matches the real manifest lookup;
adjust the listed examples (`.claude/commands/*.md`, `commands/*.md`,
`agents/*.md`, `CLAUDE.md`) only if the implementation differs.
🪄 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: 1953663e-1657-470a-bd80-b25d413c2177
📒 Files selected for processing (1)
specs/skills-and-workflows.md
specs/skills-and-workflows.md
Outdated
| - `.claude/skills/*/SKILL.md`, `.claude/commands/*.md`, `.claude/agents/*.md` | ||
| - `skills/*/SKILL.md`, `commands/*.md`, `agents/*.md` (registry layout like ai-helpers) | ||
| 4. Checks for `.ambient/ambient.json` (indicates this is a workflow) | ||
| 5. Checks for `CLAUDE.md` (indicates project instructions) |
There was a problem hiding this comment.
Scanning description doesn't match implementation.
This section describes scanning for .claude/skills/*/SKILL.md, but as noted earlier, the implementation doesn't support skills directory scanning. Additionally, line 115 references .ambient/ambient.json which should be .ambient/n based on the actual implementation.
Cross-references:
- Skills discovery gap: see comment on lines 13-17
- Manifest file path issue: see comment on line 23
🤖 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 113 - 116, The spec text
mismatches the code: update the scanning section in
specs/skills-and-workflows.md so it matches the actual implementation by
removing the unsupported `.claude/skills/*/SKILL.md` scan entry (or
alternatively implement skills scanning in the code if you intend to support
it), and correct the manifest path reference from `.ambient/ambient.json` to
`.ambient/n` so the documented workflow check matches the real manifest lookup;
adjust the listed examples (`.claude/commands/*.md`, `commands/*.md`,
`agents/*.md`, `CLAUDE.md`) only if the implementation differs.
specs/skills-and-workflows.md
Outdated
| - Save as a workflow manifest that can be: | ||
| - Stored in the workspace | ||
| - Exported as a Git repo | ||
| - Exported as a Claude Code plugin (`plugin.json`) |
There was a problem hiding this comment.
Design tension: plugin export conflicts with reference model.
Line 154 proposes exporting workflows as Claude Code plugins (plugin.json), but this conflicts with the core design principle stated on line 156: "The key constraint: skills are never copied into the workflow. The manifest holds references."
The Open Questions section (line 327-328) acknowledges this: "plugins cache/copy files which breaks the dynamic reference model."
This design tension needs resolution before implementation:
- Option A: Don't support plugin export, keep workflows purely reference-based
- Option B: Support plugin export with explicit copy/bundle mode (acknowledge it breaks the reference model)
- Option C: Propose a new plugin format that supports reference-based skill loading
Recommendation: Make an explicit design decision and update both sections consistently. The current spec presents plugin export as a feature while simultaneously questioning whether it's compatible with the architecture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/skills-and-workflows.md` at line 154, Choose and document a single
design decision to resolve the conflict between exporting workflows as a Claude
Code plugin (`plugin.json`) and the stated reference model ("the manifest holds
references" / "skills are never copied into the workflow"): either (A) remove
plugin export support and explicitly state workflows remain reference-only, (B)
add a documented copy/bundle mode for `plugin.json` that explicitly breaks the
reference model and describe its trade-offs, or (C) define a new plugin format
that preserves reference-based skill loading; then update the mention of
`plugin.json`, the manifest rule ("skills are never copied into the workflow"),
and the Open Questions section so the spec consistently reflects the chosen
option and its implications.
specs/skills-and-workflows.md
Outdated
|
|
||
| ## Open Questions | ||
|
|
||
| 1. **Manifest format**: YAML vs JSON? Should we extend `ambient.json` or create a new `workflow.yaml`? The existing `ambient.json` has `name`, `description`, `systemPrompt`, `startupPrompt`, `rubric` — we'd add `sources` and `prompt`. |
There was a problem hiding this comment.
Open question references incorrect filename.
The question asks "Should we extend ambient.json" but the implementation uses .ambient/n. While the structural fields mentioned (name, description, systemPrompt, startupPrompt, rubric) are accurate, the filename is not.
This suggests the spec may have been written without full awareness of the implementation details.
Recommendation: Update to reference the actual filename .ambient/n or clarify that this question is proposing a rename/migration from .ambient/n to ambient.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/skills-and-workflows.md` at line 323, Update the spec wording that
currently asks about extending `ambient.json` to instead reference the actual
implemented filename `.ambient/n`, or explicitly state this is a proposal to
rename/migrate from `ambient.json` to `.ambient/n`; modify the sentence that
lists `name`, `description`, `systemPrompt`, `startupPrompt`, `rubric` and the
suggested additions `sources` and `prompt` so it refers to `.ambient/n` (or
notes the rename), ensuring consistency between the document and the
implementation.
specs/skills-and-workflows.md
Outdated
| 5. **Security**: How do we verify skill sources haven't been tampered with? Git commit SHAs provide content-addressable verification. Enterprise customers may need signed manifests. | ||
|
|
There was a problem hiding this comment.
Security considerations merit deeper treatment.
The spec correctly identifies security as a concern (verifying skill sources, tamper detection, signed manifests), but this is currently just an open question. Given that this architecture involves:
- Cloning and executing arbitrary code from Git URLs
- Installing skills at workspace and session levels
- Potential for supply-chain attacks via compromised skill repos
Security should be elevated to a dedicated section with:
- Threat model (malicious skill sources, MITM attacks, compromised registries)
- Proposed mitigations (commit SHA pinning, signature verification, sandboxing)
- Enterprise requirements (private registries, allow-lists, audit logs)
Would you like me to draft an expanded security section covering threat modeling and mitigation strategies for code-loading architectures?
🤖 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 331 - 332, Elevate the brief
"Security" note into a dedicated section titled "Security" that expands into
three subsections: "Threat model" (enumerate risks like malicious skill sources,
MITM, compromised registries, supply-chain attacks when cloning/executing
Git-based skills and installing at workspace/session levels), "Proposed
mitigations" (specify commit SHA pinning, signature/manifest verification,
secure transport, sandboxing/containers, dependency vulnerability scanning,
least-privilege installs, and runtime syscall/network restrictions), and
"Enterprise requirements" (private registries, allow-lists/block-lists, audit
logs, provenance/signature enforcement, and attestation of approved skill
manifests); update any references to the earlier one-line "Security" note so
they point to the new section and ensure the spec mentions where SHA pinning and
signature verification are enforced in the skill installation workflow (e.g.,
skill install/clone and manifest validation steps).
jwm4
left a comment
jwm4
left a comment
There was a problem hiding this comment.
In general, this direction seems good. I had a bunch of comments on details, but broadly speaking, I like where this is going.
|
|
||
| ### Agent (future) | ||
|
|
||
| A persona — prose defining what an agent is responsible for. "Backend Agent", "Security Agent", "PM Agent". An Agent uses workflows and standalone skills to accomplish its goals. An Agent is a session template with a personality: |
There was a problem hiding this comment.
Agents could also be sources presumably, right?
There was a problem hiding this comment.
i think so - because I might want to leverage a hosted agent. in that case i might need auth support, too. the easiest example is the internal productpages MCP which requires auth and contains key ground truth for any agent.
There was a problem hiding this comment.
Yes — sources is generic. Any Git URL with discoverable Claude Code content (skills, agents, commands, plugins) works.
There was a problem hiding this comment.
Agreed — sources is generic, includes hosted agents. Auth handled via existing workspace credential system.
specs/skills-and-workflows.md
Outdated
| A persona — prose defining what an agent is responsible for. "Backend Agent", "Security Agent", "PM Agent". An Agent uses workflows and standalone skills to accomplish its goals. An Agent is a session template with a personality: | ||
|
|
||
| ``` | ||
| Agent = Persona (prompt/directive) + Workflows (skill bundles) + Standalone skills |
There was a problem hiding this comment.
This formula does make sense, but I think there's also a lot of design around workflows that include multiple agents, e.g., a workflow for creating a report that includes a research agent, followed by a writer agent, followed by an editor agent, followed by a distributor agent. I think it's going to be hard for us to have a concept of an agent here that fits well with all the different things that people want to do with multiple agents.
There was a problem hiding this comment.
ACP is grounded in workflows, I would want this implementation to reflect that.
There was a problem hiding this comment.
Noted — added that multi-agent orchestration is out of scope for this spec. The Agent concept here is single-persona.
There was a problem hiding this comment.
Spec centers workflows as the primary concept. Skills are atoms, workflows are the user-facing thing.
specs/skills-and-workflows.md
Outdated
|
|
||
| ### Alignment with Claude Code | ||
|
|
||
| Adopting Claude Code's `marketplace.json` format where possible makes ACP skills portable. Users could add the same marketplace from local Claude Code via `/plugin marketplace add`. The catalog format should normalize to the same shape regardless of source type. |
There was a problem hiding this comment.
That sounds reasonable to me. I'd really prefer to align with a non-vendor-specific equivalent, but I don't think there is one.
There was a problem hiding this comment.
We can make adapters as the ecosystem develops, or just rewrite them all. I am working under the assumption that there are no sacred cows at this point. However the concept of workflows, that you can automate them and that they offer maximum power to knowledge-workers, that has to be delivered.
There was a problem hiding this comment.
Updated — we follow Claude Code's plugin format and the Agent Skills (agentskills.io) open standard as the closest cross-tool spec.
There was a problem hiding this comment.
Agreed — we follow Claude Code plugin format for now, can adapt as ecosystem develops.
specs/skills-and-workflows.md
Outdated
| path: code-review | ||
|
|
||
| # Optional: quality evaluation criteria | ||
| rubric: |
There was a problem hiding this comment.
I guess I'm increasingly inclined to drop this. Having it in our schema encourages people to use it and then they wind up making things that are less portable because they depend on it.
There was a problem hiding this comment.
Wouldn't you rather see engineers figuring out + adopting evals? Maybe there is some other way we could encourage that.
There was a problem hiding this comment.
Yes, I definitely want an automated review/repair loop, and I do think we should encourage it. I just think the way the industry is going for how to do that is via SKILL.md files.
There was a problem hiding this comment.
Keeping rubric as-is for now — existing workflows (bugfix, triage) use it. It's optional and doesn't hurt portability if unused.
There was a problem hiding this comment.
Rubric kept as-is. Agree evals matter — the format may evolve but the capability stays.
There was a problem hiding this comment.
Agreed — rubric kept but the format may evolve. The goal (automated review/repair) is right, the mechanism can improve.
specs/skills-and-workflows.md
Outdated
|
|
||
| ## Open Questions | ||
|
|
||
| 1. **Manifest format**: YAML vs JSON? Should we extend `ambient.json` or create a new `workflow.yaml`? The existing `ambient.json` has `name`, `description`, `systemPrompt`, `startupPrompt`, `rubric` — we'd add `sources` and `prompt`. |
There was a problem hiding this comment.
we'd add
sourcesandprompt
the idea presented earlier in the document is that prompt would replace a bunch of existing capabilities (systemPrompt, startupPrompt, CLAUDE.md). I think that makes more sense than adding prompt in addition to the things that are already there.
There was a problem hiding this comment.
Applied — prompt consolidated to just CLAUDE.md. systemPrompt and startupPrompt dropped from ambient.json.
specs/skills-and-workflows.md
Outdated
| - Prompt text (inline or file reference) | ||
| - Skill sources (Git URL + branch + path) | ||
| - Optional: system prompt, startup prompt, rubric | ||
| - Optional: environment variables, MCP server configs |
There was a problem hiding this comment.
just want to call out explicitly for MCP - must support auth and tls
There was a problem hiding this comment.
Applied — noted that MCP sources requiring auth and TLS are handled through workspace integration configuration.
specs/skills-and-workflows.md
Outdated
| This format is consumed by: | ||
| - ACP runner (production) | ||
| - `/load-workflow` skill (local) | ||
| - Claude Code plugin hook (automated local) |
There was a problem hiding this comment.
Is there a marketplace that the ACP project provides? (not the top level one that does the inheritance), for ACP itself - like this /load-workflow?
There was a problem hiding this comment.
Yes — ACP provides its own marketplace via ConfigMap sources. Behind a feature flag.
specs/skills-and-workflows.md
Outdated
|
|
||
| 3. **Plugin format**: Should workflow export produce a Claude Code plugin (`plugin.json`)? Pros: portable, namespaced, versioned. Cons: plugins cache/copy files which breaks the dynamic reference model. | ||
|
|
||
| 4. **RHAI alignment**: How does this map to RHAIRFE-1370 (Skills Registry)? Our manifest format and marketplace could inform the product's in-cluster registry design. |
There was a problem hiding this comment.
By default, we should be using the exact components/feature or aiming to converge. If there are decisions that lead down different paths, we have to discuss.
There was a problem hiding this comment.
Let me explain - the product may not prescribe an agent framework, even though the ACP project may need one. In that case, we pick our solution and ensure interop with RHOAI.
There was a problem hiding this comment.
Applied — we follow Claude Code plugin format as the standard. Agent Skills (agentskills.io) open standard.
There was a problem hiding this comment.
Acknowledged — we aim to converge with product direction.
specs/skills-and-workflows.md
Outdated
|
|
||
| 4. **RHAI alignment**: How does this map to RHAIRFE-1370 (Skills Registry)? Our manifest format and marketplace could inform the product's in-cluster registry design. | ||
|
|
||
| 5. **Security**: How do we verify skill sources haven't been tampered with? Git commit SHAs provide content-addressable verification. Enterprise customers may need signed manifests. |
There was a problem hiding this comment.
I don't see these things any differently than traditional source code, so we should anticipate (unsure on timeline) all the expected signing and distribution security.
There was a problem hiding this comment.
Removed from spec scope. Acknowledged as future concern.
specs/skills-and-workflows.md
Outdated
|
|
||
| 5. **Security**: How do we verify skill sources haven't been tampered with? Git commit SHAs provide content-addressable verification. Enterprise customers may need signed manifests. | ||
|
|
||
| 6. **Workspace defaults**: Should some workspace-level items be "always-on" (loaded in every session regardless of workflow)? Example: company-wide coding standards skill. Or should this be handled via org-level Claude Code managed settings? |
There was a problem hiding this comment.
Ensure this top level enforcement feature exists in this feature. Leave it there, inert/feature-flagged off, until enforcement is requested.
There was a problem hiding this comment.
Removed from spec scope. Marketplace itself is behind feature flag.
specs/skills-and-workflows.md
Outdated
|
|
||
| 2. **Skill versioning**: Sources reference branches today. Should we support tags or SHAs for pinning? What happens when a skill source updates — do sessions get the latest on next start? | ||
|
|
||
| 3. **Plugin format**: Should workflow export produce a Claude Code plugin (`plugin.json`)? Pros: portable, namespaced, versioned. Cons: plugins cache/copy files which breaks the dynamic reference model. |
There was a problem hiding this comment.
I'm on the fence with dynamic reference model. It seems fine for casual usage. I bet people will ask to pin.
There was a problem hiding this comment.
Applied — sources support optional sha/tag for pinning. Branch default gives auto-update. User's choice.
specs/skills-and-workflows.md
Outdated
|
|
||
| 1. **Manifest format**: YAML vs JSON? Should we extend `ambient.json` or create a new `workflow.yaml`? The existing `ambient.json` has `name`, `description`, `systemPrompt`, `startupPrompt`, `rubric` — we'd add `sources` and `prompt`. | ||
|
|
||
| 2. **Skill versioning**: Sources reference branches today. Should we support tags or SHAs for pinning? What happens when a skill source updates — do sessions get the latest on next start? |
There was a problem hiding this comment.
How about we provide the user with choice here - they would opt-in to "Auto-update Harness" at the project and session level.
There was a problem hiding this comment.
Covered — branch default = auto-update. SHA pinning = manual update. User chooses per source.
specs/skills-and-workflows.md
Outdated
|
|
||
| 1. **Manifest format**: YAML vs JSON? Should we extend `ambient.json` or create a new `workflow.yaml`? The existing `ambient.json` has `name`, `description`, `systemPrompt`, `startupPrompt`, `rubric` — we'd add `sources` and `prompt`. | ||
|
|
||
| 2. **Skill versioning**: Sources reference branches today. Should we support tags or SHAs for pinning? What happens when a skill source updates — do sessions get the latest on next start? |
There was a problem hiding this comment.
Applied — versioning with branch (auto-update) + optional sha/tag (pinning).
specs/skills-and-workflows.md
Outdated
|
|
||
| 2. **Skill versioning**: Sources reference branches today. Should we support tags or SHAs for pinning? What happens when a skill source updates — do sessions get the latest on next start? | ||
|
|
||
| 3. **Plugin format**: Should workflow export produce a Claude Code plugin (`plugin.json`)? Pros: portable, namespaced, versioned. Cons: plugins cache/copy files which breaks the dynamic reference model. |
There was a problem hiding this comment.
no, it can, but no.
There was a problem hiding this comment.
Applied — removed plugin format export from spec.
specs/skills-and-workflows.md
Outdated
|
|
||
| 3. **Plugin format**: Should workflow export produce a Claude Code plugin (`plugin.json`)? Pros: portable, namespaced, versioned. Cons: plugins cache/copy files which breaks the dynamic reference model. | ||
|
|
||
| 4. **RHAI alignment**: How does this map to RHAIRFE-1370 (Skills Registry)? Our manifest format and marketplace could inform the product's in-cluster registry design. |
There was a problem hiding this comment.
Removed from spec.
specs/skills-and-workflows.md
Outdated
|
|
||
| 4. **RHAI alignment**: How does this map to RHAIRFE-1370 (Skills Registry)? Our manifest format and marketplace could inform the product's in-cluster registry design. | ||
|
|
||
| 5. **Security**: How do we verify skill sources haven't been tampered with? Git commit SHAs provide content-addressable verification. Enterprise customers may need signed manifests. |
There was a problem hiding this comment.
Removed from spec.
specs/skills-and-workflows.md
Outdated
|
|
||
| 5. **Security**: How do we verify skill sources haven't been tampered with? Git commit SHAs provide content-addressable verification. Enterprise customers may need signed manifests. | ||
|
|
||
| 6. **Workspace defaults**: Should some workspace-level items be "always-on" (loaded in every session regardless of workflow)? Example: company-wide coding standards skill. Or should this be handled via org-level Claude Code managed settings? |
There was a problem hiding this comment.
we need to chat about always on
There was a problem hiding this comment.
Kept as open question — needs further discussion.
…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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
specs/skills-and-workflows.md (2)
261-268:⚠️ Potential issue | 🟠 MajorSecurity is underspecified for a remote-code-loading design.
Given this architecture clones and activates external sources at runtime, leaving integrity controls as an open question is risky. Add a normative Security section with minimum requirements (pinning strategy, provenance/signature verification, trust policy/allowlist, and enforcement points in install/load flow).
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 261 - 268, Add a normative "Security" section to the skills-and-workflows spec that mandates integrity controls for remote code loading: define a pinning strategy (support branch/tag/SHA pinning and specify default behavior for session updates), require provenance/signature verification (signed manifests or commit SHA checks) during the install/load flow, describe a trust policy/allowlist and where enforcement happens (e.g., in the installer/activator code paths that clone/activate sources), and document failure modes and audit/logging expectations; reference existing symbols like "sources" format, "skill source", and "install/load flow" so implementers know exactly where to enforce these checks.
154-165:⚠️ Potential issue | 🟠 MajorResolve the plugin-export contradiction with the reference-only model.
This still presents two incompatible rules: exporting as Claude Code plugin (Line 162/Line 263) vs “skills are never copied” (Line 164). Please choose one explicit mode (reference-only, or explicit bundle/copy mode) and make both sections consistent.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 263-264
🤖 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 154 - 165, Summary: The spec contradicts itself by saying skills are reference-only (ambient.json sources) while also allowing "export as a Claude Code plugin" which implies copying/bundling. Fix: pick one mode and make both sections consistent—either (A) reference-only: clarify that workflows export only ambient.json + CLAUDE.md and the "sources" array must reference remote repos and that the runner resolves/clones at runtime (remove/disable the "export as a Claude Code plugin" option), or (B) bundle mode: introduce an explicit exportMode/bundle flag, describe that export as Git repo or Claude Code plugin will create a bundled copy of referenced skills into the repo and update ambient.json to include bundled manifest paths; ensure the loader/runner knows to use ambient.json.sources for reference-mode and the bundle manifest for bundle-mode. Update all mentions of "sources", "ambient.json", "CLAUDE.md", and the export list to reflect the chosen mode and add a short note about the runner resolution behavior for that mode.
🤖 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 66-69: Tighten the "single URL string" parsing by defining a
canonical, host-agnostic grammar and normalization rules: specify that a URL
string must be parsed into the fields url, branch, and path (matching the
structured object keys), support common host path patterns (e.g.,
/tree/:branch/:path, /blob/:branch/:path, and plain repo root), treat branch as
optional with a defined fallback (e.g., default branch detection or "main"
fallback), percent-decode path components, strip leading/trailing slashes,
ignore query/fragments, and normalize host/owner/repo case consistently; update
the spec text around the "Structured object" and "Single URL string" examples to
enumerate these rules and list how loaders should resolve branch/path
deterministically so different loaders produce identical url/branch/path
outputs.
---
Duplicate comments:
In `@specs/skills-and-workflows.md`:
- Around line 261-268: Add a normative "Security" section to the
skills-and-workflows spec that mandates integrity controls for remote code
loading: define a pinning strategy (support branch/tag/SHA pinning and specify
default behavior for session updates), require provenance/signature verification
(signed manifests or commit SHA checks) during the install/load flow, describe a
trust policy/allowlist and where enforcement happens (e.g., in the
installer/activator code paths that clone/activate sources), and document
failure modes and audit/logging expectations; reference existing symbols like
"sources" format, "skill source", and "install/load flow" so implementers know
exactly where to enforce these checks.
- Around line 154-165: Summary: The spec contradicts itself by saying skills are
reference-only (ambient.json sources) while also allowing "export as a Claude
Code plugin" which implies copying/bundling. Fix: pick one mode and make both
sections consistent—either (A) reference-only: clarify that workflows export
only ambient.json + CLAUDE.md and the "sources" array must reference remote
repos and that the runner resolves/clones at runtime (remove/disable the "export
as a Claude Code plugin" option), or (B) bundle mode: introduce an explicit
exportMode/bundle flag, describe that export as Git repo or Claude Code plugin
will create a bundled copy of referenced skills into the repo and update
ambient.json to include bundled manifest paths; ensure the loader/runner knows
to use ambient.json.sources for reference-mode and the bundle manifest for
bundle-mode. Update all mentions of "sources", "ambient.json", "CLAUDE.md", and
the export list to reflect the chosen mode and add a short note about the runner
resolution behavior for that mode.
🪄 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: 3a20fcbb-760a-4e2c-bcbe-3667ac8d0d65
📒 Files selected for processing (1)
specs/skills-and-workflows.md
| Sources support two formats: | ||
| - **Structured object**: `{"url": "...", "branch": "...", "path": "..."}` — works with any Git host, branch is explicit | ||
| - **Single URL string**: `"https://github.com/org/repo/tree/main/path"` — auto-parsed, convenient for sharing | ||
|
|
There was a problem hiding this comment.
Source URL string format needs a strict, host-agnostic parsing contract.
"single URL string" is convenient, but the current example is GitHub tree/...-shaped and “auto-parsed” is underspecified. Without a canonical grammar and normalization rules, different loaders can resolve different branch/path values from the same string.
Proposed spec tightening
-Sources support two formats:
-- **Structured object**: `{"url": "...", "branch": "...", "path": "..."}` — works with any Git host, branch is explicit
-- **Single URL string**: `"https://github.com/org/repo/tree/main/path"` — auto-parsed, convenient for sharing
+Sources support two formats:
+- **Structured object**: `{"url": "...", "branch": "...", "path": "..."}` — canonical internal representation.
+- **Single URL string**: convenience input that MUST be normalized to the structured object before use.
+
+Normalization rules (required):
+1. If string format is unsupported/ambiguous for a host, reject with validation error.
+2. Extract and persist explicit `url` (repo), `branch` (or default branch), and `path`.
+3. Store and transmit only the normalized structured form across ACP components.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 66 - 69, Tighten the "single URL
string" parsing by defining a canonical, host-agnostic grammar and normalization
rules: specify that a URL string must be parsed into the fields url, branch, and
path (matching the structured object keys), support common host path patterns
(e.g., /tree/:branch/:path, /blob/:branch/:path, and plain repo root), treat
branch as optional with a defined fallback (e.g., default branch detection or
"main" fallback), percent-decode path components, strip leading/trailing
slashes, ignore query/fragments, and normalize host/owner/repo case
consistently; update the spec text around the "Structured object" and "Single
URL string" examples to enumerate these rules and list how loaders should
resolve branch/path deterministically so different loaders produce identical
url/branch/path outputs.
jwm4
left a comment
jwm4
left a comment
There was a problem hiding this comment.
My main concerns are mostly addressed now.
Merge Queue Status
This pull request spent 15 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
|
bot should not have merged. continue convo here #1120 I havbe addressed all comments here already |
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>
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>
3 participants
Summary
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.
Key Decisions
--add-dir, a/load-workflowmeta-skill, or a Claude Code pluginadd_dirsSections
For Review
Looking for feedback on: