feat(connector): accept multiple <service>.<action> ids in schema command#290
Merged
Conversation
…mand Previously `oo connector schema` took exactly one bare service name plus a required `--action` option, so inspecting more than one action contract (e.g. an async submit/result pair, or a read step feeding a write step) meant one command per action. `connectorSchemaCommand` now accepts one or more `<service>.<action>` qualified ids directly as variadic positional arguments. A single id keeps the historical JSON object shape; two or more widen the output to a JSON array in request order. The legacy `--action` form is kept for backwards compatibility but is now restricted to exactly one bare, undotted service name, and mixing it with the qualified form is rejected before any metadata request is issued. Telemetry for `connector.schema` gains `action_count_bucket` and `qualified` alongside the existing `refresh` property, still without recording service or action identity. Bundled `oo`/`oo-create-skill` skill references and `docs/commands*.md` are updated to the `<service>.<action>` syntax and document the batch form for lifecycle-style workflows. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis PR changes the Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as connectorSchemaCommand
participant Parser as ActionId Parser
participant Loader as loadConnectorActionSchema
participant Telemetry
User->>CLI: oo connector schema <actionId...>
CLI->>Parser: parse qualified or legacy action ids
Parser-->>CLI: resolved targets or CliUserError
CLI->>Telemetry: record action_count_bucket, qualified, refresh
loop for each target
CLI->>Loader: fetch schema for service/action
Loader-->>CLI: schema result
end
CLI-->>User: JSON object (1 target) or JSON array (multiple)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/application/commands/connector/schema.ts (1)
84-99: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLoad multiple action schemas concurrently.
Line 87 serializes independent schema loads, so batched
connector schema a.b c.d ...latency grows with the sum of all requests.Promise.all()preserves the input order expected by the output array.As per coding guidelines, "Use
Promise.all()for independent async operations instead of sequentialawait." <coding_guidelines>Proposed refactor
- const account = await requireCurrentAccount(context); - const outputs: ConnectorActionSchemaOutput[] = []; - - for (const target of targets) { - const actionSchema = await loadConnectorActionSchema( - { - account, - actionName: target.actionName, - refresh: input.refresh, - serviceName: target.serviceName, - }, - context, - ); - - outputs.push(createConnectorActionSchemaOutput(actionSchema)); - } + const account = await requireCurrentAccount(context); + const outputs: ConnectorActionSchemaOutput[] = await Promise.all( + targets.map(async target => { + const actionSchema = await loadConnectorActionSchema( + { + account, + actionName: target.actionName, + refresh: input.refresh, + serviceName: target.serviceName, + }, + context, + ); + + return createConnectorActionSchemaOutput(actionSchema); + }), + );🤖 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 `@src/application/commands/connector/schema.ts` around lines 84 - 99, The loop in the connector schema command is awaiting each load sequentially, so independent schema fetches are serialized instead of running in parallel. Update the logic around requireCurrentAccount, loadConnectorActionSchema, and createConnectorActionSchemaOutput to start all schema loads together with Promise.all, then build the outputs array from the resolved results in the same input order.Source: Coding guidelines
src/application/commands/connector/index.cli.test.ts (1)
3694-3701: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winStrengthen telemetry assertions to guarantee absence of raw identity fields.
toMatchObjectonly verifies the listed keys; it won't fail ifservice,action, or a rawactionIdalso leaked intotelemetryPayload.properties. Given this PR's explicit goal of "avoiding recording service or action identity," a stricter assertion (e.g., asserting the exact property set, orexpect(telemetryPayload.properties).not.toHaveProperty("service")) would materially strengthen the regression guard.As per path instructions, "When adding command-specific telemetry properties, add or update tests that assert the expected safe property shape and absence of raw/private values."
♻️ Example stronger assertion
expect(telemetryPayload).toMatchObject({ properties: { action_count_bucket: "1-5", command_full: "connector.schema", qualified: true, refresh: false, }, }); + expect(telemetryPayload.properties).not.toHaveProperty("service"); + expect(telemetryPayload.properties).not.toHaveProperty("action");Also applies to: 3804-3811, 3950-3957
🤖 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 `@src/application/commands/connector/index.cli.test.ts` around lines 3694 - 3701, Strengthen the telemetry tests around connector command payloads so they verify the safe property shape, not just a subset of keys. In the affected assertions in connector CLI tests, replace or supplement the current `expect(telemetryPayload).toMatchObject(...)` checks with assertions on `telemetryPayload.properties` that guarantee no raw identity fields leak, especially `service`, `action`, and any raw `actionId`. Use the existing `telemetryPayload`/`properties` assertions in the connector command test cases to confirm both the expected fields and the absence of private values.Source: Path instructions
🤖 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.
Nitpick comments:
In `@src/application/commands/connector/index.cli.test.ts`:
- Around line 3694-3701: Strengthen the telemetry tests around connector command
payloads so they verify the safe property shape, not just a subset of keys. In
the affected assertions in connector CLI tests, replace or supplement the
current `expect(telemetryPayload).toMatchObject(...)` checks with assertions on
`telemetryPayload.properties` that guarantee no raw identity fields leak,
especially `service`, `action`, and any raw `actionId`. Use the existing
`telemetryPayload`/`properties` assertions in the connector command test cases
to confirm both the expected fields and the absence of private values.
In `@src/application/commands/connector/schema.ts`:
- Around line 84-99: The loop in the connector schema command is awaiting each
load sequentially, so independent schema fetches are serialized instead of
running in parallel. Update the logic around requireCurrentAccount,
loadConnectorActionSchema, and createConnectorActionSchemaOutput to start all
schema loads together with Promise.all, then build the outputs array from the
resolved results in the same input order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3b0154c-037c-4aa3-8e59-43c4ecb0b29f
⛔ Files ignored due to path filters (1)
src/application/commands/connector/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
contrib/skills/shared/oo-create-skill/SKILL.mdcontrib/skills/shared/oo/references/connector-execution.mdcontrib/skills/shared/oo/references/search-and-selection.mddocs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/connector/index.cli.test.tssrc/application/commands/connector/schema.tssrc/application/commands/telemetry-decisions.test.tssrc/i18n/catalog.ts
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.
oo connector schemaused to take exactly one bare service name plus a required--actionoption, so inspecting more than one action contract — an async submit/result pair, or a read step feeding a write step — meant running the command once per action. This changes the command to accept one or more<service>.<action>qualified ids directly as positional arguments: a single id keeps the existing JSON object output, and two or more widen it to a JSON array in request order.The old
--actionform is kept for backwards compatibility but is now restricted to exactly one bare, undotted service name; mixing it with the qualified form is rejected before any metadata request goes out. Telemetry forconnector.schemagainsaction_count_bucketandqualifiedalongside the existingrefreshproperty, without recording service or action identity.Bundled
oo/oo-create-skillskill references anddocs/commands*.mdare updated to the<service>.<action>syntax, including guidance to batch lifecycle-style workflows (e.g. submit + result) into one schema call instead of two.