[PR #2976] [MERGED] fix(session): reject an unknown configured provider before unbound routing #5532

New issue

Closed

opened 2026-10-31 01:07:22 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:07:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2976
Author: @555aaditya
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/2592-unknown-configured-provider

📝 Commits (1)

11f78e8 fix(session): reject an unknown configured provider before unbound routing

📊 Changes

2 files changed (+23 additions, -1 deletions)

View changed files

📝 src/application/binary/AnalysisProviderRegistry.test.ts (+22 -0)
📝 src/application/binary/AnalysisProviderRegistry.ts (+1 -1)

📄 Description

Summary

Fixes #2592.

When every registered deep provider rejects a target (for example an archive target, where no deep provider applies), AnalysisProviderRegistry.select() returned an intentionally unbound success for any concrete environment selector. That included an identifier no provider is registered under. So a misconfigured REA_ANALYSIS_PROVIDER=missing-provider opened the target unbound. The first deep operation then failed with target_unsupported, blaming the target instead of the unknown configuration. Passing the same identifier explicitly already failed correctly with unknown_provider.

After this change, the unbound shortcut applies only to a registered environment preference. An unknown configured identifier goes on to concrete selection and fails with unknown_provider, the same as an explicit request. Registered preferences and auto still leave artifact-only targets unbound, so artifact-only routing is unchanged.

Problem and expected behavior

Smallest trigger: a registry built with one registered candidate alpha and the default selector missing, then select(archiveTarget) with no explicit request, where alpha reports targetSupport.status === "unsupported" for the target.

Call	Before	After
`select(target)` with configured `missing`	`ok`, `binding: null`; a later `unboundOperationError` reports `target_unsupported`	`err`, `selection_reason: "unknown_provider"`, `requested_provider_id: "missing"`
`select(target, "missing")` (explicit)	`unknown_provider`	`unknown_provider` (unchanged)
`select(target)` with configured `alpha` (registered)	`ok`, unbound	`ok`, unbound (unchanged)
`select(target)` with `auto`	`ok`, unbound	`ok`, unbound (unchanged)

Violated invariant: AGENTS.md says unresolved identity must stay unknown rather than be treated as absent or aligned. The shortcut decided "intentionally unbound" before the identity of the configured provider was resolved; that check only ran later, in #selectConcrete.

Change and scope

The owning check is the unbound shortcut in AnalysisProviderRegistry.select(), so the fix goes there. The guard selector !== "auto" becomes this.#byId.has(selector). auto is reserved and can never be a registered id (analysisProviderIdSchema refuses it), so the new condition is strictly narrower than the old one. Every selector it excludes is an unknown concrete id, and those now go through the existing #selectConcrete path, which already produces unknown_provider with the full candidate list.

Not included: rejecting the unknown identifier when configuration is parsed (src/contracts/providerSelection.ts). The configuration boundary doesn't know which providers are registered. The registry does, and it already owns the unknown_provider diagnosis for explicit requests, so both paths now share it.

Contribution scope: an ordinary fix within an existing boundary, so a maintainer scope decision is not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: src/application/binary/AnalysisProviderRegistry.ts, in select() (application layer).
Contribution scope fit and linked maintainer decision: bug fix; not applicable.
CLI and MCP/tool-catalog contract: no schema or catalog change. open_binary (and the CLI paths that open a target) now report unknown_provider instead of succeeding unbound, but only when REA_ANALYSIS_PROVIDER names an unregistered provider and no registered provider supports the target.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details: target selection is stricter only for unknown configured identifiers. No provider is started or probed on the new failure path; #selectConcrete returns before evaluating any candidate.
Generated metadata, package, or installation impact: none.

Expected effects:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

(No target interaction: this is provider selection logic over already-parsed target metadata and candidate status.)

Evidence and regression coverage

Tests added or updated: src/application/binary/AnalysisProviderRegistry.test.ts, test rejects an unknown environment preference even when every candidate rejects the target. It checks that the configured unknown selector and the explicit unknown selector both fail with unknown_provider, requested_provider_id: "missing" and candidate_ids: ["alpha"]. The existing test keeps artifact-only targets unbound under auto or environment preference still covers the registered-preference and auto controls from the issue's restoration check.
Base reproduction: with only the source change reverted, the new test fails (1 failed | 6 passed), because the configured unknown selector returns an unbound success. With the change, all 7 tests in the file pass.
User-visible CLI/MCP output: the existing unknown_provider selection error, now also returned for the configured-default path.
Remaining proof gaps: I didn't run a live open_binary session with a real misconfigured REA_ANALYSIS_PROVIDER. Production routing forwards this registry result unchanged, and the provider-selection MCP boundary test passes.

Validation performed

All run on macOS (arm64), Node 26.5.0, npm 11.17.0, with locked dependencies, rebased on current origin/main:

npx vitest run src/application/binary/AnalysisProviderRegistry.test.ts: with the source fix reverted, 1 failed (the new test) and 6 passed.
npx vitest run src/application/binary/: 6 files, 45 passed.
npx vitest run tests/boundary/mcp/providerSelectionMcp.test.ts src/application/binary/BinarySessionStatus.test.ts: passed.
npm run check:fast (typecheck + lint + module boundaries): passed, also via the pre-push hook.
npx oxfmt --check and npx oxlint on both changed files: clean.

Compatibility, safety, and release

Breaking changes or migration steps: none for valid configuration. A setup with REA_ANALYSIS_PROVIDER set to an unregistered id, which previously opened artifact-only targets "successfully", now gets the unknown_provider diagnostic and should correct the variable or remove it.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (selection logic, exercised with registry candidates).
Package or release metadata impact: none.
Security, privacy, process, or containment review: no process, filesystem, or network interaction.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2976 **Author:** [@555aaditya](https://github.com/555aaditya) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/2592-unknown-configured-provider` --- ### 📝 Commits (1) - [`11f78e8`](https://github.com/morluto/rea/commit/11f78e80f0213dd1aecadda83398e75ca986ce8e) fix(session): reject an unknown configured provider before unbound routing ### 📊 Changes **2 files changed** (+23 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/application/binary/AnalysisProviderRegistry.test.ts` (+22 -0) 📝 `src/application/binary/AnalysisProviderRegistry.ts` (+1 -1) </details> ### 📄 Description ## Summary Fixes #2592. When every registered deep provider rejects a target (for example an archive target, where no deep provider applies), `AnalysisProviderRegistry.select()` returned an **intentionally unbound** success for any concrete environment selector. That included an identifier no provider is registered under. So a misconfigured `REA_ANALYSIS_PROVIDER=missing-provider` opened the target unbound. The first deep operation then failed with `target_unsupported`, blaming the target instead of the unknown configuration. Passing the same identifier explicitly already failed correctly with `unknown_provider`. After this change, the unbound shortcut applies only to a **registered** environment preference. An unknown configured identifier goes on to concrete selection and fails with `unknown_provider`, the same as an explicit request. Registered preferences and `auto` still leave artifact-only targets unbound, so artifact-only routing is unchanged. ## Problem and expected behavior **Smallest trigger:** a registry built with one registered candidate `alpha` and the default selector `missing`, then `select(archiveTarget)` with no explicit request, where `alpha` reports `targetSupport.status === "unsupported"` for the target. | Call | Before | After | |---|---|---| | `select(target)` with configured `missing` | `ok`, `binding: null`; a later `unboundOperationError` reports `target_unsupported` | `err`, `selection_reason: "unknown_provider"`, `requested_provider_id: "missing"` | | `select(target, "missing")` (explicit) | `unknown_provider` | `unknown_provider` (unchanged) | | `select(target)` with configured `alpha` (registered) | `ok`, unbound | `ok`, unbound (unchanged) | | `select(target)` with `auto` | `ok`, unbound | `ok`, unbound (unchanged) | **Violated invariant:** AGENTS.md says unresolved identity must stay unknown rather than be treated as absent or aligned. The shortcut decided "intentionally unbound" before the identity of the configured provider was resolved; that check only ran later, in `#selectConcrete`. ## Change and scope The owning check is the unbound shortcut in `AnalysisProviderRegistry.select()`, so the fix goes there. The guard `selector !== "auto"` becomes `this.#byId.has(selector)`. `auto` is reserved and can never be a registered id (`analysisProviderIdSchema` refuses it), so the new condition is strictly narrower than the old one. Every selector it excludes is an unknown concrete id, and those now go through the existing `#selectConcrete` path, which already produces `unknown_provider` with the full candidate list. Not included: rejecting the unknown identifier when configuration is parsed (`src/contracts/providerSelection.ts`). The configuration boundary doesn't know which providers are registered. The registry does, and it already owns the `unknown_provider` diagnosis for explicit requests, so both paths now share it. Contribution scope: an ordinary fix within an existing boundary, so a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/application/binary/AnalysisProviderRegistry.ts`, in `select()` (application layer). - Contribution scope fit and linked maintainer decision: bug fix; not applicable. - CLI and MCP/tool-catalog contract: no schema or catalog change. `open_binary` (and the CLI paths that open a target) now report `unknown_provider` instead of succeeding unbound, but only when `REA_ANALYSIS_PROVIDER` names an unregistered provider and no registered provider supports the target. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: target selection is stricter only for unknown configured identifiers. No provider is started or probed on the new failure path; `#selectConcrete` returns before evaluating any candidate. - Generated metadata, package, or installation impact: none. Expected effects: - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. (No target interaction: this is provider selection logic over already-parsed target metadata and candidate status.) ## Evidence and regression coverage - **Tests added or updated:** `src/application/binary/AnalysisProviderRegistry.test.ts`, test `rejects an unknown environment preference even when every candidate rejects the target`. It checks that the configured unknown selector and the explicit unknown selector both fail with `unknown_provider`, `requested_provider_id: "missing"` and `candidate_ids: ["alpha"]`. The existing test `keeps artifact-only targets unbound under auto or environment preference` still covers the registered-preference and `auto` controls from the issue's restoration check. - **Base reproduction:** with only the source change reverted, the new test fails (`1 failed | 6 passed`), because the configured unknown selector returns an unbound success. With the change, all 7 tests in the file pass. - **User-visible CLI/MCP output:** the existing `unknown_provider` selection error, now also returned for the configured-default path. - **Remaining proof gaps:** I didn't run a live `open_binary` session with a real misconfigured `REA_ANALYSIS_PROVIDER`. Production routing forwards this registry result unchanged, and the provider-selection MCP boundary test passes. ## Validation performed All run on macOS (arm64), Node 26.5.0, npm 11.17.0, with locked dependencies, rebased on current `origin/main`: - `npx vitest run src/application/binary/AnalysisProviderRegistry.test.ts`: with the source fix reverted, 1 failed (the new test) and 6 passed. - `npx vitest run src/application/binary/`: 6 files, 45 passed. - `npx vitest run tests/boundary/mcp/providerSelectionMcp.test.ts src/application/binary/BinarySessionStatus.test.ts`: passed. - `npm run check:fast` (typecheck + lint + module boundaries): passed, also via the pre-push hook. - `npx oxfmt --check` and `npx oxlint` on both changed files: clean. ## Compatibility, safety, and release - Breaking changes or migration steps: none for valid configuration. A setup with `REA_ANALYSIS_PROVIDER` set to an unregistered id, which previously opened artifact-only targets "successfully", now gets the `unknown_provider` diagnostic and should correct the variable or remove it. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (selection logic, exercised with registry candidates). - Package or release metadata impact: none. - Security, privacy, process, or containment review: no process, filesystem, or network interaction. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:07:22 +00:00