Skip to content

Manage skills as sources over ACP#8675

Merged
lifeizhou-ap merged 13 commits intomainfrom
jamadeo/sources-acp
Apr 21, 2026
Merged

Manage skills as sources over ACP#8675
lifeizhou-ap merged 13 commits intomainfrom
jamadeo/sources-acp

Conversation

@jamadeo
Copy link
Copy Markdown
Member

@jamadeo jamadeo commented Apr 20, 2026

Introduces "sources", a generalization over skills/agents/projects. Uses sources CRUD to manage skills, removing the tauri command handlers.

Related Issue: #8686

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 828ef1908a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +45 to +46
const client = await getClient();
const raw = await client.extMethod("_goose/sources/list", { type: "skill" });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve a mockable transport for skills API calls

Routing listSkills through getClient().extMethod(...) here bypasses the Tauri invoke("list_skills") path that our Playwright fixture still mocks in ui/goose2/tests/e2e/fixtures/tauri-mock.ts. In tauri-mocked runs, get_goose_serve_url is unhandled and resolves to null, so ACP initialization fails and SkillsView drops to an empty list, breaking the existing skills E2E flows that assert mocked skills are visible. Please add ACP mocking for these methods (or a test-mode fallback) before switching this call path.

Useful? React with 👍 / 👎.

Comment thread ui/goose2/src-tauri/src/commands/acp.rs Outdated
Comment on lines +7 to +8
if env::var("GOOSE_SERVE_URL").is_ok() {
return Ok(env::var("GOOSE_SERVE_URL").unwrap());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Ignore blank GOOSE_SERVE_URL overrides

This override accepts any present GOOSE_SERVE_URL, including an empty string, and then skips launching the managed GooseServe process. If the variable is set but blank (a common CI/shell misconfiguration), the app returns an invalid websocket URL and ACP-dependent features fail instead of falling back to GooseServeProcess::get. Validate that the env var is non-empty (and preferably URL-shaped) before returning it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 023342e8d9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +315 to +319
"version": 1,
"type": type_slug,
"name": name,
"description": description,
"content": content,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve v1 skill field names in exported JSON

export_source still labels the payload as "version": 1 but now writes the skill body to "content" instead of the v1 "instructions" field, which causes silent data loss when the file is imported by clients that only understand the original v1 schema (they read an empty body). To keep cross-version .skill.json sharing safe, either emit the legacy instructions key (optionally alongside content) or bump the version and make older readers fail explicitly.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

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

I have thoughts, but let's merge!

const PERSONAS = ${personas};
const SKILLS = ${skills};
const PROJECTS = ${projects};
const FAKE_ACP_URL = "ws://127.0.0.1:0/mock-acp";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I feel we might not need e2e tests in the future. Basically we can mock the response in the unit test. and we can use the app e2e test to test the integration with acp server

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a3fb9b3199

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

if !raw.trim_start().starts_with("---") {
return (String::new(), raw.to_string());
}
match parse_frontmatter::<SkillFront>(raw) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Handle --- inside skill frontmatter values

parse_skill_frontmatter delegates to parse_frontmatter, whose implementation in agents/platform_extensions/mod.rs splits the entire file on every ---. If a valid description contains --- (for example description: 'A --- B'), YAML parsing fails and this function falls back to (empty description, raw file); as a result list_sources/export_source can show blank descriptions and export frontmatter as content. The previous skills parser only looked for the closing frontmatter delimiter, so this is a regression for otherwise valid skill files.

Useful? React with 👍 / 👎.

* main:
  handle full node paths in goose2 kill recipe (#8709)
  overhaul provider inventory and agent/model selection (#8652)
  Remove unused import (#8676)
  delete the goose2 migration plan prompt (#8678)
  Add health score badge to README (#8677)
@lifeizhou-ap lifeizhou-ap added this pull request to the merge queue Apr 21, 2026
Merged via the queue into main with commit b1235e7 Apr 21, 2026
25 checks passed
@lifeizhou-ap lifeizhou-ap deleted the jamadeo/sources-acp branch April 21, 2026 03:17
lifeizhou-ap added a commit that referenced this pull request Apr 21, 2026
* main:
  feat(hooks): add Husky git hooks for ui/goose2 (#8577)
  fix: links in chat could not be opened (#8544)
  fix: run setup before dev and dev-debug in goose2 justfile (#8718)
  Manage skills as sources over ACP (#8675)
  handle full node paths in goose2 kill recipe (#8709)
  overhaul provider inventory and agent/model selection (#8652)
  Remove unused import (#8676)
  delete the goose2 migration plan prompt (#8678)
  Add health score badge to README (#8677)
  feat(goose2): voice dictation via direct-ACP pattern (#8609)
  consistently use actions-rust-lang/setup-rust-toolchain (#8671)
  fix(developer): run shell tool under bash/sh regardless of login shell (#8659)
  refactor(providers): extract shared OAuth device-flow helper (#8619)
  Add a goose2 release workflow (#8629)
  chore(deps): bump ncipollo/release-action from 1.20.0 to 1.21.0 (#8664)
  docs: add blog post about Mesh LLM provider option (#8655)
  fix: append /chat/completions for prefixed v1 base URLs (#8521)
  Reset ChatGPT Codex auth during OAuth setup (#8569)
  chore(deps): bump EmbarkStudios/cargo-deny-action from 2.0.15 to 2.0.17 (#8665)
  Add dependabot config for pnpm workspace, cargo, and actions (#8660)
spikewang pushed a commit to spikewang/goose that referenced this pull request Apr 22, 2026
Co-authored-by: Lifei Zhou <lifei@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants