Repository navigation
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProvider identifiers are normalized to lowercase during storage and profile loading. Credential and profile lookups accept provider names without case sensitivity. Regression tests cover token retrieval, UI detection parity, provider filtering, and persisted active-profile migration. ChangesCredential provider normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
A rabbit stores provider names low, Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/security/credentials/core.rs (1)
152-173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSupport bare provider slugs during profile selection.
A credential stored as
provider:DeepSeekbecomesprovider:deepseek. A lookup fordeepseekcannot match its active key, default profile ID, or stored provider value. The assertion at Line 1419 fails. Bare-slug callers also cannot read this credential.Use one shared provider-equivalence helper that treats
provider:<slug>and<slug>as the same provider. Apply it to active-profile and fallback-profile matching.Proposed fix
+fn provider_matches(left: &str, right: &str) -> bool { + let bare = |value: &str| value.strip_prefix("provider:").unwrap_or(value); + bare(left).eq_ignore_ascii_case(bare(right)) +} + - .find(|(k, _)| k.eq_ignore_ascii_case(provider)) + .find(|(k, _)| provider_matches(k, provider)) .map(|(_, v)| v) @@ - .provider - .eq_ignore_ascii_case(provider) + .provider + .as_str() + .pipe(|stored_provider| provider_matches(stored_provider, provider)) .then(|| id.clone())Use an ordinary local binding instead of
.pipe(...)if that helper is not already available.🤖 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/openhuman/security/credentials/core.rs` around lines 152 - 173, Update the profile-selection logic around the active-profile lookup and fallback search to use one shared provider-equivalence helper that treats provider:<slug> and the bare <slug> as equivalent, matching case-insensitively. Apply this helper to active profile keys, default profile IDs, and stored profile.provider values so bare-slug callers resolve existing credentials; use a local binding instead of .pipe(...) if needed.
🤖 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/openhuman/security/credentials/core.rs`:
- Around line 45-46: In src/openhuman/security/credentials/core.rs:45-46, add
grep-friendly diagnostics for provider normalization and the credential storage
result, logging only the normalized provider and sanitized outcome; in
core.rs:152-173, log which selection branch was used—exact active key, fallback
active key, default profile, provider fallback, or no match—without profile
names or metadata; in src/openhuman/security/credentials/ops.rs:964-964, add
entry, normalized-provider, and sanitized failure/completion diagnostics,
ensuring tokens, metadata values, and full PII are never logged.
In `@src/openhuman/security/credentials/ops.rs`:
- Line 964: Update list_provider_credentials to compare provider_filter with
stored provider values using eq_ignore_ascii_case instead of exact equality,
while preserving existing filtering behavior for other fields. Add a regression
test that stores a mixed-case provider and successfully retrieves it using a
differently cased provider filter.
In `@src/openhuman/security/credentials/profiles.rs`:
- Around line 854-862: Update the active-profile migration loop in the
profile-loading method around new_active to detect case-insensitive key
collisions before insertion instead of silently overwriting entries. Prefer an
existing lowercase key, record and report conflicts where legacy keys differ
only by case, and add a regression test covering two case variants that
reference different profile IDs while preserving the selected lowercase profile.
- Around line 854-867: The persisted active-profile key normalization branch
lacks a migration diagnostic. Update the key-migration flow around key_migrated
and new_active to emit a verbose, grep-friendly [auth] log when normalization
occurs, including the count of migrated keys, while excluding provider keys and
profile IDs.
---
Outside diff comments:
In `@src/openhuman/security/credentials/core.rs`:
- Around line 152-173: Update the profile-selection logic around the
active-profile lookup and fallback search to use one shared provider-equivalence
helper that treats provider:<slug> and the bare <slug> as equivalent, matching
case-insensitively. Apply this helper to active profile keys, default profile
IDs, and stored profile.provider values so bare-slug callers resolve existing
credentials; use a local binding instead of .pipe(...) if needed.
🪄 Autofix
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 Plus
Run ID: f68665c0-a4bd-40fb-989b-d0b23ffdee95
📒 Files selected for processing (5)
src/openhuman/security/credentials/core.rssrc/openhuman/security/credentials/ops.rssrc/openhuman/security/credentials/ops_tests.rssrc/openhuman/security/credentials/profiles.rssrc/openhuman/security/credentials/profiles_tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e48d512fc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e48d512 to
1ceee59
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/openhuman/security/credentials/profiles.rs`:
- Around line 854-880: The active-profile migration count currently uses
persisted entry count minus conflicts rather than the number of keys whose
casing changed. In the migration loop around new_active and migration_conflicts,
add a counter incremented only when lower != *k, then use that counter in the
normalized-key debug message while preserving collision handling and existing
diagnostics.
🪄 Autofix
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 Plus
Run ID: 3f660c77-d2be-4449-8a8b-6cb972b1b765
📒 Files selected for processing (5)
src/openhuman/security/credentials/core.rssrc/openhuman/security/credentials/ops.rssrc/openhuman/security/credentials/ops_tests.rssrc/openhuman/security/credentials/profiles.rssrc/openhuman/security/credentials/profiles_tests.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/openhuman/security/credentials/ops_tests.rs
- src/openhuman/security/credentials/core.rs
- src/openhuman/security/credentials/ops.rs
|
hi @Al629176 please review this pr and tell me if anything needs to be changed |
1ceee59 to
47a3d1f
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
hi @senamakel @Al629176 please review this pr |
|
hello @sanil-23 @senamakel @senamakel-droid @Al629176 please review this pr |
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.1117 · 71,019 in / 5,065 out · 3,584 cached (5%) · minimax/minimax-m3, moonshotai/kimi-k3
critique: $0.0412 · 13,009 in / 1,236 out · 0 cached (0%) · moonshotai/kimi-k3, minimax/minimax-m3
security: $0.0548 · 16,112 in / 430 out · 0 cached (0%) · moonshotai/kimi-k3
tests: $0.0019 · 6,585 in / 157 out · 896 cached (14%) · minimax/minimax-m3
commits: $0.0038 · 10,595 in / 866 out · 1,792 cached (17%) · minimax/minimax-m3
description: $0.0036 · 7,867 in / 1,166 out · 768 cached (10%) · minimax/minimax-m3
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/openhuman/security/credentials/core.rs`:
- Around line 188-204: Update the bare-provider fallback in the
profile-selection function to resolve provider:{slug} through the existing
active-profile, default-profile, and provider-scan precedence rather than
directly scanning data.profiles. Preserve the fallback log and return behavior,
and add a regression test with two namespaced profiles where
active_profiles["provider:deepseek"] selects the non-first profile.
🪄 Autofix
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 Plus
Run ID: efeb5d22-a0ce-454a-8a4c-63c9bdf58abd
📒 Files selected for processing (1)
src/openhuman/security/credentials/core.rs
6c1b731 to
47a3d1f
Compare
There was a problem hiding this comment.
Requesting changes: 4 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0972 · 63,597 in / 6,973 out · 5,504 cached (9%) · moonshotai/kimi-k3, minimax/minimax-m3
critique: $0.0236 · 12,123 in / 225 out · 1,152 cached (10%) · moonshotai/kimi-k3, minimax/minimax-m3
security: $0.0566 · 17,023 in / 781 out · 2,304 cached (14%) · moonshotai/kimi-k3
tests: $0.0057 · 11,422 in / 2,084 out · 768 cached (7%) · minimax/minimax-m3
commits: $0.0048 · 10,276 in / 1,509 out · 512 cached (5%) · minimax/minimax-m3
description: $0.0065 · 12,753 in / 2,374 out · 768 cached (6%) · minimax/minimax-m3
|
@senamakel please guide me through this |
|
@senamakel @Al629176 please review this pull request |
|
Maintainer review pass — findings only, nothing pushed to your branch. There is a lot of review text on this PR, some of it wrong, so I went through each open item and checked it against the code. The bug is real and still unfixed on Which open comments to act onReal — please fix:
Answered already — reply and resolve, don't rework:
Wrong — say so and move on:
The rebase is the real workYou are ~3 weeks behind across a large file-splitting refactor, which is why all five files conflict. The functions did not change — they moved. Map for the rebase onto
Note CIBoth failures are stale infra from 2026-08-09, not your code — One request while you rebase: the diff adds a fair amount of |
|
To use Codex here, create a Codex account and connect to github. |
|
thank you for this valuable feedback @M3gA-Mind will definitely work on it |
The `scrub_json_credentials` helper function was relocated from `middleware_part_02` to `middleware_part_06` so it sits closer to the only call site that uses it, improving code locality and reducing unnecessary cross-module distance. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
This test previously used a synchronous global event bus initialisation that is no longer available. The change updates it to call the new async bus initialisation, which matches the current API and ensures the test can run correctly. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
…ontext functions The `scrub_json_credentials` function and `generated_context` method on `ToolPolicyMiddleware` were dead code that had no callers within the codebase. Removing them eliminates unnecessary compilation overhead and clarifies that credential scrubbing is handled elsewhere in the middleware pipeline. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d13c3d20e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… normalization When loading profiles, the code now gracefully handles keychain secret migration failures by retaining the original profile ID and persisted representation instead of proceeding with a broken migration. This ensures that old keychain entries remain reachable on the next load. Additionally, active profile references are now updated to match the final resolved profile ID after migration. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…sing Add a test asserting that profile migration lowercases the provider segment while leaving the profile name's casing intact, so mixed-case profile names are not silently rewritten during normalization. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Wrap the active_profiles assertion across multiple lines to satisfy rustfmt line width limits. No behaviour change. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…gration When a profile's provider casing differs from its normalized form, the migration logic was overwriting the provider field with its own value instead of keeping the original casing. This change stores the original provider before normalization and restores it when the provider casing has not changed, ensuring that profiles retain their original provider string when no casing migration is needed. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6e6969e61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The provider filter passed to list_provider_credentials is now normalized before matching so mixed-case input resolves to the same profiles as the canonical form. Profile migration also collects stale keychain entries and deletes them only after the persisted state is written, and migration targets are keyed by the original id so collisions are tracked correctly. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Strip explanatory comments from the profile migration and cleanup logic in the auth profiles store. The code is unchanged; the removed notes merely restated what the surrounding statements already make clear. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Removed two stray blank lines inside the auth profiles store method to keep the block formatting consistent. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Profile identifiers are now lowercased only in their provider segment rather than across the whole string, so profile names that are case-sensitive keep their original casing while provider lookups stay canonical. Migration targets are also registered under the normalized form so existing references resolve correctly. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…ration The active profile migration loop tracked the number of case-variant collisions in a counter variable and logged each individual conflict, but the final summary warning was the only externally visible effect. Since the migration logic already handles conflicts by preferring the canonical lowercase key, the per-conflict debug logs and the summary counter added noise without providing actionable information. Removing them simplifies the code and eliminates unnecessary logging. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Extracted the workspace directory resolution logic from `resolve_workspace_dir_from_process_state` into a new `resolve_workspace_dir` function that accepts an optional registered workspace parameter. This allows tests to call the resolution logic directly with controlled inputs instead of relying on the global process state, making the production resolution rule more directly testable. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test assertion in `production_resolution_still_honours_the_workspace_env_var` was unnecessarily split across multiple lines, making it harder to read. This change collapses the assertion into a single line for improved clarity without altering the test's behaviour. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
The migration introduced in tinyhumansai#5432 now rewrites every profile to its canonical `<provider>:<profile_name>` id, so the raw `legacy-empty` key is no longer kept verbatim. The test assertion is updated to verify that the profile is stored under `legacy:empty` instead, and to check the new canonical id and provider fields. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Summary
provider:DeepSeek) while read paths used case-sensitive exact matching, causing a silent save/read mismatch.store_provider_tokenandstore_provider_credentialsnow normalize provider slugs to lowercase at write time vianormalize_provider()/.to_ascii_lowercase().select_profile_idand bearer-token resolution gain a case-insensitive fallback (eq_ignore_ascii_case) for profiles written before this fix.AuthProfilesStore::load()auto-migrates legacy on-diskactive_profileskeys to lowercase on first load and rewrites the file; subsequent reads are no-ops. When case variants collide, the canonical lowercase entry wins and the dropped variant is logged.ControllerSchemanamespace strings are untouched.Problem
After saving a DeepSeek API key via Settings, the UI confirmed success but the dialog immediately showed the key as missing and all API calls failed. The problem persisted across restarts.
Root cause: the write path stored the profile under
provider:DeepSeek(mixed case from the frontend slug), while read paths looked upprovider:deepseek(lowercase). The keys never matched, soget_provider_bearer_tokenand thehas_tokenUI check both returnedfalsefor a key that was physically on disk.Solution
Normalize at write time (primary fix):
core.rs—store_provider_token:let provider = normalize_provider(provider)?before buildingAuthProfile.ops.rs—store_provider_credentials:let provider = provider.trim().to_ascii_lowercase()before any storage call.Case-insensitive read fallback (defence-in-depth):
select_profile_idand active-profile resolution useeq_ignore_ascii_caseso stale mixed-case profiles still resolve.On-load migration (existing data):
profiles.rs—AuthProfilesStore::load()detects anyactive_profileskey wherekey != key.to_lowercase(), rebuilds the map, and rewrites the file. Runs once; free on all subsequent loads.Submission Checklist
store_and_retrieve_provider_token_case_insensitive— mixed-case write resolves correctly on readlegacy_mixed_case_active_profile_key_migrated_on_load— stale on-disk fixture normalised and rewritten on firstload()ui_has_api_key_check_and_backend_get_profile_agree_for_same_credential—has_token(UI) andget_provider_bearer_token(backend) can never silently disagreesecurity/credentials/are covered by the three tests aboveCloses #NNN— see RelatedImpact
~/.openhuman/; no mobile/web/CLI impact.active_profiles(typically ≤ 5 entries).Related
ProviderSlugnewtype that enforces lowercase at construction time to make future regressions a compile error.AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/deepseek-provider-slug-normalizatione48d512fcValidation Run
pnpm --filter openhuman-app format:check— Blocked (no frontend files changed; CI will verify)pnpm typecheck— Blocked (node_modules not installed locally; no frontend files changed)cargo test -p openhuman credentials— passedcargo fmt --all --check— clean (exit 0)Validation Blocked
command:pnpm --filter openhuman-app format:checkerror:node_modules missingimpact:None — no frontend files modified in this PRBehavior Changes
Parity Contract
eq_ignore_ascii_casefallback on read paths ensures profiles written before this fix continue to resolve without data loss.ui_has_api_key_check_and_backend_get_profile_agree_for_same_credentialenforces strict equality between UI surface and backend resolution path.Duplicate / Superseded PR Handling
4dc3e9b) — pre-rebase, wrong file pathsgit push --force-with-leaseSummary by CodeRabbit
Bug Fixes
Tests