Skip to content

protocol cleanup - #9147

Merged
jamadeo merged 12 commits into
mainfrom
protocol-cleanup
May 22, 2026
Merged

protocol cleanup#9147
jamadeo merged 12 commits into
mainfrom
protocol-cleanup

Conversation

@jamadeo

@jamadeo jamadeo commented May 11, 2026

Copy link
Copy Markdown
Member

Clean up the ACP+ protocol a bit. Add a version tag to the custom methods and improve some types.

@jamadeo
jamadeo marked this pull request as ready for review May 18, 2026 13:19

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

Copy link
Copy Markdown

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: 57bb146304

ℹ️ 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 thread ui/sdk/src/generated/client.gen.ts Outdated
Comment thread ui/sdk/src/generated/client.gen.ts Outdated

/// Target scope for creating or importing sources.
#[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema)]
#[serde(tag = "scope", rename_all = "camelCase")]

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.

nice!

@jamadeo
jamadeo force-pushed the protocol-cleanup branch from 8a82a74 to b5224dd Compare May 20, 2026 15:13

@alexhancock alexhancock left a comment

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.

It looks good. I do think we should have a planned timeline on which we expect to progress things to stable, but that can come at another time.

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

Copy link
Copy Markdown

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: 312fe12476

ℹ️ 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".

DefaultsReadRequest,
DefaultsReadResponse,
DefaultsSaveRequest,
AddConfigExtensionRequest_unstable,

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 Import existing unstable type aliases in generated client

client.gen.ts now imports snake-case symbols like AddConfigExtensionRequest_unstable, but types.gen.ts exports these as AddConfigExtensionRequestUnstable (PascalCase). That leaves most type imports unresolved, so the SDK package cannot type-check/build after regeneration.

Useful? React with 👍 / 👎.

Comment thread ui/goose2/src/shared/api/dictation.ts Outdated
): Promise<void> {
const client = await getClient();
await client.goose.GooseDictationModelSelect({ provider, modelId });
await client.goose.GooseDictationModelsSelect({ provider, modelId });

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 Call the generated unstable dictation select method

The regenerated SDK exposes GooseUnstableDictationModelsSelect, but this call uses GooseDictationModelsSelect, which is not present on client.goose. This causes a compile-time/property-missing failure (or runtime missing-function error) when saving dictation model selection.

Useful? React with 👍 / 👎.

@jamadeo

jamadeo commented May 20, 2026

Copy link
Copy Markdown
Member Author

It looks good. I do think we should have a planned timeline on which we expect to progress things to stable, but that can come at another time.

Yeah, 100% agree. If anything I think this encourages us to stabilize -- it makes the api uglier! But right now we have a consumer of ACP+ that isn't part of this repo, so we want to make sure these are able to break compatibility.

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

Copy link
Copy Markdown

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: 19292064db

ℹ️ 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 thread ui/sdk/generate-schema.ts Outdated
function methodToCamelCase(method: string): string {
return method
.split(/[/_]/)
.filter((part) => part !== "v1")

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 Drop unstable segment from generated SDK method names

Keeping the unstable path segment in methodToCamelCase renames every generated method to GooseUnstable* (for example GooseUnstableSourcesCreate), but the rest of the tree still calls GooseSources*/GooseProviders* names (e.g. ui/goose2/src/features/skills/api/skills.ts and ui/sdk/scripts/check-binary-compat.mjs). That creates immediate missing-method compile/runtime failures after regeneration, so this change breaks the SDK consumers unless you either strip unstable during name generation or add compatibility aliases.

Useful? React with 👍 / 👎.

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

Copy link
Copy Markdown

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: d00fa2fa71

ℹ️ 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 thread ui/goose2/src/features/extensions/api/extensions.ts Outdated
Comment thread ui/goose2/src/features/chat/ui/McpAppView.tsx Outdated
@jamadeo
jamadeo added this pull request to the merge queue May 20, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks May 20, 2026

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

Copy link
Copy Markdown

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: 63890ed81d

ℹ️ 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 thread ui/text/src/extensions.tsx Outdated
const [configResp, sessionResp] = await Promise.all([
client.goose.GooseConfigExtensions({}),
client.goose.GooseSessionExtensions({sessionId}),
client.goose.GooseConfigExtensionsList({}),

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 Call the generated unstable extension methods

Fresh evidence in this commit: the regenerated SDK client (ui/sdk/src/generated/client.gen.ts) exposes configExtensionsList_unstable / sessionExtensionsList_unstable (and sessionExtensionsAdd/Remove_unstable), not Goose-prefixed methods like GooseConfigExtensionsList. Calling client.goose.GooseConfigExtensionsList here will fail with a missing-function error (and fails type-checking in typed consumers), so the TUI extensions screen cannot reliably load or manage session extensions.

Useful? React with 👍 / 👎.

@jamadeo
jamadeo added this pull request to the merge queue May 22, 2026
@jamadeo
jamadeo removed this pull request from the merge queue due to a manual request May 22, 2026
@jamadeo
jamadeo added this pull request to the merge queue May 22, 2026
Merged via the queue into main with commit 0a95821 May 22, 2026
22 checks passed
@jamadeo
jamadeo deleted the protocol-cleanup branch May 22, 2026 21:11
lifeizhou-ap added a commit that referenced this pull request May 25, 2026
* main: (48 commits)
  docs: stats update (#9410)
  Build summon instructions per turn (#9329)
  Fix desktop chat search session limiting (#9366)
  fix(agents): serialize per-session agent creation to stop duplicate MCP init (#9357)
  chore(deps): bump image from 0.24.9 to 0.25.10 (#9383)
  chore(deps): bump agent-client-protocol from 0.11.1 to 0.12.1 (#9381)
  chore(deps): bump ctor from 0.2.9 to 1.0.6 (#9380)
  chore(deps): bump strum from 0.27.2 to 0.28.0 (#9384)
  chore(deps): bump lru from 0.16.3 to 0.18.0 (#9382)
  chore(deps): bump shlex from 1.3.0 to 2.0.1 (#9379)
  chore(deps): bump sigstore-verify from 0.6.6 to 0.8.0 (#9378)
  chore(deps): bump clap_mangen from 0.2.33 to 0.3.0 (#9377)
  chore(deps): bump the cargo-minor-and-patch group with 12 updates (#9376)
  chore(deps): bump qs and express in /documentation (#9375)
  chore(deps): bump docker/build-push-action from 6.18.0 to 7.2.0 (#9374)
  chore(deps): bump step-security/harden-runner from 2.19.1 to 2.19.4 (#9373)
  chore(deps): bump EmbarkStudios/cargo-deny-action from 2.0.17 to 2.0.19 (#9372)
  chore(deps): bump actions/setup-python from 5 to 6 (#9371)
  chore(deps): bump actions-rust-lang/setup-rust-toolchain from 1.16.0 to 1.16.1 (#9370)
  protocol cleanup (#9147)
  ...
shafqatevo pushed a commit to shafqatevo/goose that referenced this pull request Aug 7, 2026
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.

3 participants