fix(review): register selected advisor model - #10838
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe advisor configuration now registers the selected model instead of always using the default. Specialist model defaults and tests now use the ChangesAdvisor model selection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
Heads-up: this looks like it broke
It now resolves instead: Cause is the the registry contains So the "unknown model must fail closed" boundary is gone, not just the test: any typo in a model id is now silently registered and dialed instead of refused. That is worth restoring on its own merits, independent of the test. Reproduced identically on #10845, #10759, and #10760 — three unrelated diffs (state migration, MCP restart, CLI exec), same assertion, same line, so it is not any of those PRs. Flagging rather than patching since this is your change and the intended contract for a caller-selected id is your call. Happy to take the fix if that helps. |
ericksoa
added a commit
that referenced
this pull request
Sep 2, 2026
<!-- markdownlint-disable MD041 --> ## Outcome Restores the OpenShell-managed Langfuse Cloud credential path for Hermes by binding the exact public and secret keys to the Langfuse ingestion endpoint. Before this change, Hermes accepted the resolver placeholders but OpenShell 0.0.106 could withhold or reject them because NemoClaw supplied no endpoint-bearing Langfuse profile. ## Reason NemoClaw migrated from OpenShell 0.0.101 to 0.0.106, which changed static credentials from destination-independent placeholder rewriting to provider-identity and endpoint-bound resolution. PR #7447 retained the Hermes validator compatibility patch, but it did not migrate the Langfuse provider boundary, so the previously working manual provider path regressed while raw sandbox keys continued to work by bypassing OpenShell. ### Related issues - Refs #10840 - Part of #7446 ## Changes - Add the checked-in `langfuse-hermes-v1` profile with exact `LANGFUSE_PUBLIC_KEY` and `LANGFUSE_SECRET_KEY` declarations. - Bind the profile to `cloud.langfuse.com:443`, `/api/public/**`, and the managed Hermes Python runtime so the credential cannot resolve at another destination or from another sandbox binary. - Include the profile in the Hermes Portable build context. - Prove `credentials add` imports the checked-in profile before provider creation, registers both exact keys, and does not return either value. - Require an absolute HTTPS Langfuse base URL before Hermes constructs an authenticated client; reject HTTP, userinfo, query strings, fragments, and malformed ports. - Document the Langfuse Cloud setup, the exact-host self-hosted profile path, stale `~/.hermes/.env` placeholder cleanup, and diagnostics that distinguish Hermes validation from OpenShell endpoint denial. - Align the current-main advisor security proof with #10838: selected model IDs now register successfully in memory, and the test continues to require credential removal before any tool turn. Production advisor code is unchanged. The endpoint-bearing profile is required because OpenShell 0.0.106 will not release an unbound static credential. A validator-only change is insufficient: it lets Hermes construct the client but provides no authority for OpenShell to rewrite the outbound Basic-auth header. ## Verification - `npx vitest run --project cli src/lib/actions/credentials-provider-adapter.test.ts src/lib/onboard/experimental/hermes-portable-build-context.test.ts` - 76 passed. - `npx vitest run --project e2e-support test/e2e/support/hermes-langfuse-credential-patch.test.ts` - 3 passed. - `npx vitest run --project integration test/automation/pull-requests/pr-review-advisor-security-boundaries.test.ts` - 6 passed after aligning the stale current-main expectation. - `npx vitest run --project cli src/commands/global-oclif-command-adapters.test.ts src/commands/sandbox/oclif-command-adapters.test.ts` - 30 passed after the changed-test run hit unrelated timeouts. - `npm run typecheck:cli` - passed. - `npm run checks:repository` - passed. - `npm run source-shape:check` - passed with zero source-shape cases. - `npm run docs` - completed with zero errors and two Fern warnings. - The patcher applied cleanly to the pinned Hermes `v2026.7.20` Langfuse plugin source, which then accepted HTTPS, rejected HTTP, and accepted the versioned OpenShell placeholder. - `npx vitest run --project integration test/agents/hermes/hermes-final-image-layout.test.ts test/agents/hermes/hermes-image-build-probes.test.ts` - 54 passed. - `npx prek run --all-files` - passed, including formatting, lint, schema validation, repository checks, secret scanning, Markdown lint, source-shape budget, and growth guardrails. - `npm test` - attempted twice after installing both root and plugin workspaces. The monolithic multi-project run was non-green from unrelated fixed-timeout failures across MCP locks, dashboard ports, rebuild, uninstall, package-contract, and installer suites; no changed Langfuse lane failed. The second attempt ran without the competing external Vitest process but retained the broad timeout pattern. This draft does not claim the broad local gate. - Diff and hook secret scans found no secrets, API keys, or credentials. ## Review notes - Security review is required because this adds a credential-bearing provider endpoint. - Live Langfuse authentication and trace-ingestion evidence through OpenShell 0.0.106 is still required before this leaves draft. - The Hermes client rejects any non-absolute-HTTPS base URL before credential-bearing initialization. - The checked-in profile covers Langfuse Cloud. Self-hosted deployments must import a separate reviewed profile with their exact HTTPS host; this PR does not authorize a wildcard or operator-supplied host in the checked-in profile. - AI-assisted implementation: Codex Desktop. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Langfuse support for Hermes, including credential configuration and portable build integration. - Added guidance for configuring Langfuse Cloud and self-hosted deployments with exact HTTPS origins and ports. - **Bug Fixes** - Improved validation for Langfuse URLs, rejecting insecure, malformed, credential-bearing, query-based, and fragment-based addresses. - Invalid Langfuse configuration now prevents initialization and provides a warning. - **Tests** - Expanded coverage for credential handling, URL validation, initialization failures, and configuration compatibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
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
3 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.
Outcome
Both PR Review Advisor model lanes use their complete upstream model IDs and configure successfully in the embedded advisor registry.
Reason
OpenShell forwards the configured model ID verbatim and requires
openai/openai/gpt-5.6-terra. The embedded Pi registry previously registered only the Azure default, so the complete OpenAI ID failed local lookup. Shortening that ID passed local lookup only after changing the matrix, but OpenShell then rejected it with HTTP 403 because the key was not authorized foropenai/gpt-5.6-terra.Changes
openai/openai/gpt-5.6-terramatrix identifier.Verification
key_model_access_deniedforopenai/gpt-5.6-terra; Azure specialists succeeded.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Improvements
Bug Fixes