fix: zero-disk credential injection via PRAISONAI_AUTH_CONTENT - #3775
fix: zero-disk credential injection via PRAISONAI_AUTH_CONTENT#3775praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
fixes #3774) Load the whole credential store from a single env var (JSON) in memory when PRAISONAI_AUTH_CONTENT is set; skip all disk reads and make writes in-memory no-ops so nothing is persisted. Surface the in-memory source in auth list/status. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe credential store now accepts a complete JSON credential set from ChangesCredential injection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PRAISONAI_AUTH_CONTENT
participant CredentialStore
participant AuthStatus
PRAISONAI_AUTH_CONTENT->>CredentialStore: Provide JSON credentials
CredentialStore->>CredentialStore: Validate and retain credentials in memory
AuthStatus->>CredentialStore: Read credentials and source mode
CredentialStore-->>AuthStatus: Return credentials with env (in-memory) source
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
src/praisonai-code/tests/unit/test_credential_env_blob.py (2)
56-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover OAuth refresh in zero-disk mode.
This test uses an unexpired token. It does not verify that a refresh updates only
self._memory_store. Add an expired token, mockrequests.post, and assert that the refreshed access token is available throughget_credentialwhilediskstill does not exist.🤖 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/praisonai-code/tests/unit/test_credential_env_blob.py` around lines 56 - 73, Update test_env_blob_oauth_token_available_in_memory to use an expired OAuth token, mock requests.post with a successful refresh response, and trigger the refresh through the CredentialStore. Assert get_credential returns the refreshed access token and confirm the credentials file at disk still does not exist.
23-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the no-read guarantee directly.
This test verifies precedence, but it can pass if an implementation reads the disk file before returning the environment value. Patch
store._effective_read_pathto fail after construction, then callget_credential. This makes any disk-read path fail the test.🤖 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/praisonai-code/tests/unit/test_credential_env_blob.py` around lines 23 - 37, Update test_env_blob_loads_in_memory_without_reading_disk to patch store._effective_read_path after CredentialStore construction so any subsequent disk access fails, then perform the existing get_credential and token assertions. Preserve the precedence and in-memory checks while directly enforcing that the environment blob is used without reading the disk file.
🤖 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/praisonai-code/praisonai_code/cli/configuration/credentials.py`:
- Around line 94-108: Update the environment-variable handling in the
credentials initializer to distinguish an unset AUTH_CONTENT_ENV from a
set-but-empty value, parsing and rejecting the latter with ValueError before any
disk-backed store access. Add a regression test covering an empty
AUTH_CONTENT_ENV value and asserting ValueError.
---
Nitpick comments:
In `@src/praisonai-code/tests/unit/test_credential_env_blob.py`:
- Around line 56-73: Update test_env_blob_oauth_token_available_in_memory to use
an expired OAuth token, mock requests.post with a successful refresh response,
and trigger the refresh through the CredentialStore. Assert get_credential
returns the refreshed access token and confirm the credentials file at disk
still does not exist.
- Around line 23-37: Update test_env_blob_loads_in_memory_without_reading_disk
to patch store._effective_read_path after CredentialStore construction so any
subsequent disk access fails, then perform the existing get_credential and token
assertions. Preserve the precedence and in-memory checks while directly
enforcing that the environment blob is used without reading the disk file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50107777-cdde-4986-bc1b-c5cf0ec48a2c
📒 Files selected for processing (3)
src/praisonai-code/praisonai_code/cli/commands/auth.pysrc/praisonai-code/praisonai_code/cli/configuration/credentials.pysrc/praisonai-code/tests/unit/test_credential_env_blob.py
| self._memory_store: Optional[Dict[str, Dict[str, Any]]] = None | ||
| env_blob = os.environ.get(AUTH_CONTENT_ENV) | ||
| if env_blob: | ||
| try: | ||
| parsed = json.loads(env_blob) | ||
| except json.JSONDecodeError as exc: | ||
| raise ValueError( | ||
| f"{AUTH_CONTENT_ENV} is set but is not valid JSON: {exc}" | ||
| ) from exc | ||
| if not isinstance(parsed, dict): | ||
| raise ValueError( | ||
| f"{AUTH_CONTENT_ENV} must be a JSON object mapping " | ||
| "provider -> credential" | ||
| ) | ||
| self._memory_store = parsed |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Treat an empty environment value as configured input.
Line 96 treats PRAISONAI_AUTH_CONTENT="" as absent. The store then reads and writes the disk-backed credential file instead of rejecting the invalid JSON value. This violates the zero-disk contract when the variable is set.
Proposed fix
- if env_blob:
+ if env_blob is not None:Add a regression test for an empty value that expects ValueError.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._memory_store: Optional[Dict[str, Dict[str, Any]]] = None | |
| env_blob = os.environ.get(AUTH_CONTENT_ENV) | |
| if env_blob: | |
| try: | |
| parsed = json.loads(env_blob) | |
| except json.JSONDecodeError as exc: | |
| raise ValueError( | |
| f"{AUTH_CONTENT_ENV} is set but is not valid JSON: {exc}" | |
| ) from exc | |
| if not isinstance(parsed, dict): | |
| raise ValueError( | |
| f"{AUTH_CONTENT_ENV} must be a JSON object mapping " | |
| "provider -> credential" | |
| ) | |
| self._memory_store = parsed | |
| self._memory_store: Optional[Dict[str, Dict[str, Any]]] = None | |
| env_blob = os.environ.get(AUTH_CONTENT_ENV) | |
| if env_blob is not None: | |
| try: | |
| parsed = json.loads(env_blob) | |
| except json.JSONDecodeError as exc: | |
| raise ValueError( | |
| f"{AUTH_CONTENT_ENV} is set but is not valid JSON: {exc}" | |
| ) from exc | |
| if not isinstance(parsed, dict): | |
| raise ValueError( | |
| f"{AUTH_CONTENT_ENV} must be a JSON object mapping " | |
| "provider -> credential" | |
| ) | |
| self._memory_store = parsed |
🤖 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/praisonai-code/praisonai_code/cli/configuration/credentials.py` around
lines 94 - 108, Update the environment-variable handling in the credentials
initializer to distinguish an unset AUTH_CONTENT_ENV from a set-but-empty value,
parsing and rejecting the latter with ValueError before any disk-backed store
access. Add a regression test covering an empty AUTH_CONTENT_ENV value and
asserting ValueError.
Greptile SummaryThe PR adds an environment-backed, zero-disk credential store and identifies that source in authentication CLI output.
Confidence Score: 4/5The PR is not yet safe to merge because refreshed OAuth state remains isolated to one CredentialStore instance, allowing later instances to retry with stale rotated credentials. Each store reparses the original environment blob, while successful OAuth refreshes update only the current instance; a later store can therefore reload an invalidated refresh token and fail authentication. Files Needing Attention: src/praisonai-code/praisonai_code/cli/configuration/credentials.py
|
| Filename | Overview |
|---|---|
| src/praisonai-code/praisonai_code/cli/configuration/credentials.py | Adds validated environment-blob loading and zero-disk writes; the previously reported cross-instance OAuth refresh-state issue remains outstanding. |
| src/praisonai-code/praisonai_code/cli/commands/auth.py | Labels credentials originating from the environment-backed store without exposing their values. |
| src/praisonai-code/tests/unit/test_credential_env_blob.py | Covers environment precedence, zero-disk updates, payload validation, OAuth access, and disk fallback. |
| src/praisonai/tests/unit/cli/test_interactive_surfaces_parity.py | Updates the test double to accept the newly expected read-only argument. |
Reviews (2): Last reviewed commit: "fix: reject empty/malformed PRAISONAI_AU..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
@claude CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3775 (zero-disk credential injection)
Phase 1 — Architecture review (per AGENTS.md)
Not scope creep — narrow, minimal, and it strengthens security. Phase 2 — Fixes applied
Skipped (with reason):
Phase 3 — Verdict: ✅ ApproveAll blocking issues resolved and CI root cause fixed. Tests: Files changed in |
…t stub - credentials: treat a set-but-empty PRAISONAI_AUTH_CONTENT as a misconfiguration (ValueError) instead of silently falling back to disk, preserving the zero-disk contract; validate that each provider entry is a JSON object so malformed entries fail fast with a clear error. - tests: add regression coverage for empty value and non-object provider entry. - fix pre-existing CI failure in test_async_tui_expands_at_mentions: the _fake_execute stub predated the read_only kwarg on _execute_prompt, so the real call raised a swallowed TypeError and left the prompt uncaptured. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
|
Claude finished @MervinPraison's task —— View job CI Failure Fix — PR #3775
Verdict: Pre-existing test bug — already fixed on this branchThe failing test The fix was already committed in Local verification (HEAD
|
| Test | Result |
|---|---|
test_async_tui_expands_at_mentions (CI failure) |
✅ 1 passed |
test_credential_env_blob.py (PR changes) |
✅ 8 passed |
Note: A separate run showed
test_single_command_registryfailing locally, but that's a local artifact — my editable install resolvedpraisonai_codefrom a stalesite-packagescopy rather than the source tree (.../site-packages/praisonai_code/...). It is unrelated to this PR and not part of the reported CI failure.
Files changed / pushed
None — the CI root cause (test_interactive_surfaces_parity.py:58) was already fixed in 52f333620 on this branch. Nothing further to push. The reported job ran against 2f735488 (before that fix landed); a re-run on current HEAD should pass.
• Branch: claude/issue-3774-20260807-0850
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3774
Summary
Adds a zero-disk credential path for ephemeral/CI runs. When the
PRAISONAI_AUTH_CONTENTenvironment variable is set,CredentialStoreloadsits JSON value as the entire credential store in memory and never reads or
writes
~/.praisonai/credentials.json. This lets a single CI/container secretinject all provider auth — including OAuth/refresh tokens — with nothing
persisted to disk.
Changes
cli/configuration/credentials.py:PRAISONAI_AUTH_CONTENTon init into an in-memory store (invalidJSON / non-object payloads raise a clear
ValueError)._read_credentialsreturns the in-memory blob;_write_credentialsupdates it in memory only and skips all disk I/O (writes become no-ops).
is_in_memoryproperty. Env-blob content takes precedence over disk.cli/commands/auth.py:auth list/auth statusreport the source asenv (in-memory)so the zero-disk mode is not silently invisible.Notes on scope
Minimal, wrapper/CLI-tier change. No new Agent params, no new dependencies, no
new exports beyond a module-level env-var name constant. Per-provider env vars
continue to work unchanged.
Tests
tests/unit/test_credential_env_blob.py(6 tests): env-blob precedence overdisk, writes never touch disk, OAuth token available in-memory, invalid
JSON / non-object rejected, and no-env-blob still uses disk. All pass; the
existing
test_credential_onboarding.pysuite (12) still passes.Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests