feat(connector): add proxy subcommand#277
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughThis PR introduces the Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/application/commands/connector/proxy.ts`:
- Around line 176-186: The telemetry call in context.telemetry.recordProperties
is sending raw user input via input.serviceName which violates the "no raw user
input" rule; update the properties to avoid sending free-form values by removing
or replacing the service property: change the recorded field (currently
referenced as input.serviceName) to a safe alternative such as a boolean flag
like has_service_name (e.g., service_present: input.serviceName !== undefined)
or a bucketed/normalized enum value, and ensure the change is applied where
context.telemetry.recordProperties is invoked (referencing the recordProperties
call and the input.serviceName symbol).
In `@src/application/commands/connector/shared.test.ts`:
- Around line 299-372: The test only covers the success path for
runConnectorProxy; add negative-path tests in
src/application/commands/connector/shared.test.ts that assert the connector
proxy error mapping branches (the insufficient-credit mapping and the
message/code variants implemented around runConnectorProxy lines ~718-763).
Specifically, add tests that mock the fetcher to return: (1) a response
body/error indicating insufficient credit and assert the mapped error
type/code/message from runConnectorProxy, and (2) responses with alternative
error shapes (message-only and code+message) and assert the function normalizes
them into the expected error object/throw behavior; reference the
runConnectorProxy function and its proxy error mapping logic when adding these
assertions. Ensure each test exercises and asserts the exact mapped output
(error code/message/structure) produced by runConnectorProxy.
🪄 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: CHILL
Plan: Pro
Run ID: b9ee279f-068a-4f9f-aeac-ff3ec51b13c8
📒 Files selected for processing (12)
contrib/skills/shared/oo/references/connector-execution.mddocs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/connector/identity.test.tssrc/application/commands/connector/identity.tssrc/application/commands/connector/index.cli.test.tssrc/application/commands/connector/index.tssrc/application/commands/connector/proxy.tssrc/application/commands/connector/shared.test.tssrc/application/commands/connector/shared.tssrc/application/commands/telemetry-decisions.test.tssrc/i18n/catalog.ts
💤 Files with no reviewable changes (1)
- src/application/commands/connector/identity.test.ts
BlackHole1
reviewed
Jun 10, 2026
Member
There was a problem hiding this comment.
Did a full pass over this PR. Overall the proxy subcommand is in good shape: the telemetry decision and manifest are in sync, and the error mapping, i18n, and bilingual docs are fairly complete; I ran lint / ts-check / knip / test on the branch and everything passes.
Left 11 inline comments. Overview:
major (2)
- An invalid
--methodvalue produces a badly misleading error ("Invalid format: . Use json."), reproduced locally - The handler's error branches, the
--datapath, and the default text output have no test coverage at all
question (3)
- Removing the organization query parameter is a wire-contract change (#274 deliberately introduced the dual-write two days ago); needs server-side compatibility confirmation
connectorProxyResponseSchema's strict requirement ondata.headerscan discard an entire successful response- The proxy failure path records no
http_status/error_codetelemetry dimensions, unlike run
minor (6): invalidPayload errors lack the field path, usage errors are gated behind the login check, text output lacks the ?? "null" fallback, the telemetry test lacks regression assertions, --help reuses run-specific wording, and three gaps in the docs
Every item was reproduced/verified on a local checkout of the branch.
Member
|
Missing Zod validation for 'method', needs to be added, needs to support case-insensitivity. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/application/commands/connector/shared.test.ts (1)
411-598: ⚡ Quick winExtract repeated proxy test setup into a local factory at file end.
The repeated
runConnectorProxyinput and fetch-context wiring across these tests makes updates error-prone. Please centralize the shared setup into one local test factory and override per-case fields.♻️ Suggested refactor
+function createProxyRunInput(overrides: Partial<Parameters<typeof runConnectorProxy>[0]> = {}) { + return { + apiKey: "secret-1", + endpoint: "oomol.com", + proxyRequest: { + endpoint: "/search", + method: "GET" as const, + }, + serviceName: "tavily", + ...overrides, + }; +} + +function createProxyContext(fetcher: Fetcher) { + return createRequestContext({ fetcher }); +} + test("runConnectorProxy surfaces message and errorCode on failed proxy responses", async () => { - const error = await expectCliUserError(runConnectorProxy( - { - apiKey: "secret-1", - endpoint: "oomol.com", - proxyRequest: { - endpoint: "/search", - method: "GET", - }, - serviceName: "tavily", - }, - createRequestContext({ - fetcher: async () => new Response(JSON.stringify({ + const error = await expectCliUserError(runConnectorProxy( + createProxyRunInput(), + createProxyContext(async () => new Response(JSON.stringify({ errorCode: "invalid_input", message: "bad query", success: false, }), { status: 400, - }), - }), + })), ));As per coding guidelines, "In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file."
🤖 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/shared.test.ts` around lines 411 - 598, Extract the repeated test setup for runConnectorProxy into a single local factory function (e.g., makeProxyTestInputs or buildProxyTestCtx) placed at the bottom of the file; this factory should return the common input object (apiKey, endpoint, proxyRequest.endpoint/method, serviceName) and a createRequestContext wrapper that accepts a custom fetcher for each test to override the response or thrown error (used in tests calling runConnectorProxy, createRequestContext, expectCliUserError, and createFailedToOpenSocketError); update each test to call the factory and only supply the differing fields (fetcher behavior and any per-test input like errorCode or status) so shared wiring is centralized and easy to maintain.Source: Coding guidelines
🤖 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/shared.test.ts`:
- Around line 411-598: Extract the repeated test setup for runConnectorProxy
into a single local factory function (e.g., makeProxyTestInputs or
buildProxyTestCtx) placed at the bottom of the file; this factory should return
the common input object (apiKey, endpoint, proxyRequest.endpoint/method,
serviceName) and a createRequestContext wrapper that accepts a custom fetcher
for each test to override the response or thrown error (used in tests calling
runConnectorProxy, createRequestContext, expectCliUserError, and
createFailedToOpenSocketError); update each test to call the factory and only
supply the differing fields (fetcher behavior and any per-test input like
errorCode or status) so shared wiring is centralized and easy to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf8117a8-a0e8-42b8-a7bc-11e540697771
📒 Files selected for processing (10)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/connector/index.cli.test.tssrc/application/commands/connector/proxy.tssrc/application/commands/connector/run.tssrc/application/commands/connector/shared.test.tssrc/application/commands/connector/shared.tssrc/application/commands/connector/telemetry.tssrc/application/commands/telemetry-decisions.test.tssrc/i18n/catalog.ts
✅ Files skipped from review due to trivial changes (3)
- src/application/commands/connector/telemetry.ts
- docs/commands.md
- docs/commands.zh-CN.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/application/commands/telemetry-decisions.test.ts
- src/application/commands/connector/index.cli.test.ts
- src/application/commands/connector/proxy.ts
Contributor
Author
|
Handled the latest follow-up in 1172a09.
Validated locally with |
Contributor
Author
|
Updated per latest direction in f2d0ff5: removed the proxy CLI app selector surface for this PR.
Validation: |
Contributor
Author
|
Manual proxy smoke test after removing app selectors:
Both calls used only the default connected app selection; no CLI app selector parameters were involved. |
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
2 participants
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.