fix: route FME default auth through client#323
Conversation
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
|
|
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
There was a problem hiding this comment.
Findings:
- Important:
src/client/harness-client.ts(HarnessClient.applyDefaultAuth()),tests/client/harness-client.test.ts(request — headersFME cases),tests/utils/session-headers.test.ts(resolves FME auth from the session key...),tests/registry/feature-flags.test.ts(FME request routing) — moving FME auth selection out ofRegistryand into the client is the right direction, but the PR drops the old registry-level multi-user failure-path assertion without replacing it with a real session-config-to-client regression. The new coverage proves single-user success/placeholder cases and helper-level session merging, but it never exercisesHARNESS_MCP_MODE="multi-user"throughmergeConfigWithSessionHeaders()into an FMEclient.request(). That leaves the main behavior this refactor is meant to protect — session-scoped FME auth plus the fail-loud remediation path — unpinned by tests.
Open questions:
- none
Overall:
- Not acceptable as-is; the runtime change looks sound, but it should land with a focused multi-user FME auth regression that covers the full session-config -> client request path.
Sent by Cursor Automation: Sunil On Demand Architecture Review
There was a problem hiding this comment.
Findings:
- Important:
src/client/harness-client.tsnow owns FME fallback auth selection, but the PR does not replace the old multi-user FME failure-path coverage with a test that drives a session-scoped config into the moved client logic. The current suite proves single-user success/placeholder cases and helper-level session merging, yet it never exercisesHARNESS_MCP_MODE="multi-user"throughmergeConfigWithSessionHeaders()into an FMEclient.request(). That leaves the main behavior this refactor is meant to protect — session-scoped FME auth plus the fail-loud multi-user remediation path — unpinned.
Open questions:
- none
Overall:
- Not acceptable as-is; the runtime change looks sound, but it should land with a focused multi-user FME auth regression that covers the full session-config -> client request path.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| expect(headers["Harness-Account"]).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("uses bearer auth from a non-placeholder API key for FME requests", async () => { |
There was a problem hiding this comment.
These added cases cover the single-user fallback and placeholder paths, but they still do not exercise the main contract this refactor is protecting in hosted mode: mergeConfigWithSessionHeaders() producing a multi-user session config that then flows into HarnessClient.applyDefaultAuth() for an FME request.
Please add one focused regression that constructs a multi-user session config from headers, builds a HarnessClient from it, and then proves either:
- the session PAT is normalized to
Authorization: Bearer ...for an FME call, or - the multi-user remediation error is raised when the session credential is missing/placeholder.
Without that end-to-end test, the session-scoped FME path that motivated moving auth out of Registry is still not pinned.


Description
Moves FME/Split fallback auth selection into
HarnessClientso caller-providedAuthorizationis preserved,x-api-keyplaceholders are stripped before Split/FME calls, and per-session/non-placeholder API keys are normalized to Bearer auth. Registry dispatch now only marks FME requests with product/base URL routing.Type of Change
Checklist
pnpm test)pnpm typecheck)Additional verification:
pnpm build,pnpm docs:generate,pnpm docs:check,git diff --check, and focused FME Vitest coverage.