Remove Caliper AWS secret and improve the vault - #82
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPhase-based vault helpers were added and adopted by skeleton and llm_d orchestration commands. Caliper export and replot code stopped handling AWS credential-file paths, and several orchestration configs now reference Vault-backed MLflow secrets. ChangesVault and MLflow secret migration
Sequence Diagram(s)sequenceDiagram
participant Main as main(ctx)
participant VaultInit as vault.phase_vault_init
participant ResolveEntry as create_fournos_resolve_entrypoint
participant VaultList as vault.phase_vault_list_all
Main->>VaultInit: phase_vault_init(ctx.invoked_subcommand)
Main->>ResolveEntry: pass vault.phase_vault_list_all
ResolveEntry->>VaultList: list vault names
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
/test fournos skeleton |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/caliper/orchestration/replot.py (1)
148-155: 🎯 Functional Correctness | 🔴 CriticalFix the call to
run_replot_from_orchestration_configto remove thevault_aws_secretargument.The caller in
projects/core/library/replot.pystill passesvault_aws_secret(line 71), but the function definition inprojects/caliper/orchestration/replot.py(line 148) no longer accepts this parameter. This removal of the argument from the signature and failure to update the call site will result in a runtime error.Call site details
return run_replot_from_orchestration_config( replot_url=replot_url, artifact_directory=artifact_directory, vault_name=vault_name, vault_mlflow_secret=vault_mlflow_secret, vault_aws_secret=vault_aws_secret, # Remove this line keep_replot_dir=keep_replot_dir, postprocess_config=postprocess_config, )🤖 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 `@projects/caliper/orchestration/replot.py` around lines 148 - 155, The call to run_replot_from_orchestration_config in the replot helper still passes vault_aws_secret even though the function signature no longer accepts it. Update the caller in projects/core/library/replot.py to stop passing vault_aws_secret and keep the argument list aligned with run_replot_from_orchestration_config and its parameters vault_name, vault_mlflow_secret, keep_replot_dir, and postprocess_config.
🧹 Nitpick comments (1)
projects/caliper/orchestration/replot.py (1)
100-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove stale AWS credential comments.
These comments still describe AWS credential setup after the AWS credential path plumbing was removed, which makes the new MLflow-secret-only contract ambiguous.
Suggested cleanup
- # Set tracking URI before creating client (AWS credentials need to be set first) + # Set tracking URI after mlflow_connection_env applies MLflow credentials. mlflow.set_tracking_uri(mlflow_uri)- # Get AWS credentials if provided -Also applies to: 183-183
🤖 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 `@projects/caliper/orchestration/replot.py` around lines 100 - 103, The comments around mlflow_connection_env in replot.py are stale and still mention AWS credential setup even though that path was removed. Update the nearby comments in the replot flow and any matching comment near the export/replot setup to describe only the current MLflow-secret-based behavior, using the mlflow_connection_env and mlflow.set_tracking_uri call sites as the anchors.
🤖 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 `@projects/caliper/orchestration/export.py`:
- Around line 96-101: The multi-run dry-run path in export() sets ret to 0 but
never creates status_yaml, so the later status file write/open path can fail
with FileNotFoundError. Update the dry-run branch in the multi-run handling to
either generate the expected status_yaml payload or return/skip before any code
that opens it, using the existing export() flow and status_yaml variable as the
main anchors.
In `@projects/core/library/vault.py`:
- Around line 492-520: phase_vault_init currently only reads phase-scoped keys
via _phase_vault_get_for_phase, so legacy flat vault lists are ignored and
nothing gets initialized. Update phase_vault_init to fall back to the existing
flat vault list when the phase-based lookups return empty, while still
preserving the current all/all-optional and phase-specific behavior. Use the
existing symbols phase_vault_init, _phase_vault_get_for_phase,
phase_vault_list_all, and init to keep compatibility with both legacy and phased
vault configurations.
- Around line 527-535: Handle the missing vaults config before iterating in
resolve-fournos-config: the vault_config value from
config.project.get_config("vaults") can be absent or non-mapping, so update the
vault handling in the vault resolution logic to treat a missing value as empty
and only call .items() on a dict-like new-format config. Keep the existing
list-vs-dict behavior in vault_config processing, but add a safe fallback so a
project without a vaults stanza returns an empty secretRefs list instead of
crashing.
---
Outside diff comments:
In `@projects/caliper/orchestration/replot.py`:
- Around line 148-155: The call to run_replot_from_orchestration_config in the
replot helper still passes vault_aws_secret even though the function signature
no longer accepts it. Update the caller in projects/core/library/replot.py to
stop passing vault_aws_secret and keep the argument list aligned with
run_replot_from_orchestration_config and its parameters vault_name,
vault_mlflow_secret, keep_replot_dir, and postprocess_config.
---
Nitpick comments:
In `@projects/caliper/orchestration/replot.py`:
- Around line 100-103: The comments around mlflow_connection_env in replot.py
are stale and still mention AWS credential setup even though that path was
removed. Update the nearby comments in the replot flow and any matching comment
near the export/replot setup to describe only the current MLflow-secret-based
behavior, using the mlflow_connection_env and mlflow.set_tracking_uri call sites
as the anchors.
🪄 Autofix (Beta)
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
Run ID: 94b71e3f-50b1-495b-829b-32c1e89413e4
📒 Files selected for processing (10)
projects/caliper/orchestration/export.pyprojects/caliper/orchestration/export_config.pyprojects/caliper/orchestration/replot.pyprojects/core/library/vault.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/config.yamlprojects/mcp_gateway/orchestration/config.yamlprojects/rhaiis/orchestration/config.yamlprojects/skeleton/orchestration/ci.pyprojects/skeleton/orchestration/config.yaml
💤 Files with no reviewable changes (4)
- projects/caliper/orchestration/export_config.py
- projects/llm_d/orchestration/config.yaml
- projects/rhaiis/orchestration/config.yaml
- projects/mcp_gateway/orchestration/config.yaml
| if len(run_dirs) > 1: | ||
| if export_cfg.dry_run: | ||
| logger.info( | ||
| "dry-run: would export %d run dirs from %s (skipping)", len(run_dirs), from_path | ||
| ) | ||
| ret = 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Write or return status for multi-run dry runs.
Line 101 sets ret = 0 without producing status_yaml, but Line 134 still opens that file, so multi-run dry-runs will fail with FileNotFoundError after logging success.
Possible localized fix
if export_cfg.dry_run:
logger.info(
"dry-run: would export %d run dirs from %s (skipping)", len(run_dirs), from_path
)
- ret = 0
+ return {
+ "mlflow": {
+ "status": "dry_run",
+ "detail": f"would export {len(run_dirs)} run dirs from {from_path}",
+ }
+ }📝 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.
| if len(run_dirs) > 1: | |
| if export_cfg.dry_run: | |
| logger.info( | |
| "dry-run: would export %d run dirs from %s (skipping)", len(run_dirs), from_path | |
| ) | |
| ret = 0 | |
| if len(run_dirs) > 1: | |
| if export_cfg.dry_run: | |
| logger.info( | |
| "dry-run: would export %d run dirs from %s (skipping)", len(run_dirs), from_path | |
| ) | |
| return { | |
| "mlflow": { | |
| "status": "dry_run", | |
| "detail": f"would export {len(run_dirs)} run dirs from {from_path}", | |
| } | |
| } |
🤖 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 `@projects/caliper/orchestration/export.py` around lines 96 - 101, The
multi-run dry-run path in export() sets ret to 0 but never creates status_yaml,
so the later status file write/open path can fail with FileNotFoundError. Update
the dry-run branch in the multi-run handling to either generate the expected
status_yaml payload or return/skip before any code that opens it, using the
existing export() flow and status_yaml variable as the main anchors.
| def phase_vault_init(phase: str) -> None: | ||
| """Initialize vaults for a specific phase.""" | ||
|
|
||
| # Get global mandatory vaults (always loaded) | ||
| global_mandatory = _phase_vault_get_for_phase("all") | ||
|
|
||
| # Get phase-specific mandatory vaults | ||
| phase_mandatory = _phase_vault_get_for_phase(phase) | ||
|
|
||
| # Combine all mandatory vaults | ||
| mandatory_vaults = global_mandatory + phase_mandatory | ||
|
|
||
| # Get global optional vaults (always loaded optionally) | ||
| global_optional = _phase_vault_get_for_phase("all-optional") | ||
|
|
||
| # Get phase-specific optional vaults | ||
| phase_optional = _phase_vault_get_for_phase(f"{phase}-optional") | ||
|
|
||
| # Combine all optional vaults | ||
| optional_vaults = global_optional + phase_optional | ||
|
|
||
| if not mandatory_vaults and not optional_vaults: | ||
| logger.info(f"No vault to initialize for phase '{phase}'") | ||
| return | ||
|
|
||
| # Initialize both mandatory and optional vaults in a single call | ||
| # Mandatory vaults: strict=True (automation fails if missing/invalid) | ||
| # Optional vaults: strict=False (automation continues with warnings if missing/invalid) | ||
| init(mandatory_vaults=mandatory_vaults, optional_vaults=optional_vaults) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve legacy flat vault lists during phase init.
Line 492 now initializes only vaults.all / phase keys. If vaults is still a flat list, _phase_vault_get_for_phase(...) returns [] and no vaults are initialized, while phase_vault_list_all() still supports that legacy format.
Proposed fix
def phase_vault_init(phase: str) -> None:
"""Initialize vaults for a specific phase."""
+ from projects.core.library import config
+
+ vault_config = config.project.get_config("vaults", [])
+ if isinstance(vault_config, list):
+ if not vault_config:
+ logger.info(f"No vault to initialize for phase '{phase}'")
+ return
+
+ init(vaults=vault_config)
+ return
# Get global mandatory vaults (always loaded)
global_mandatory = _phase_vault_get_for_phase("all")📝 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.
| def phase_vault_init(phase: str) -> None: | |
| """Initialize vaults for a specific phase.""" | |
| # Get global mandatory vaults (always loaded) | |
| global_mandatory = _phase_vault_get_for_phase("all") | |
| # Get phase-specific mandatory vaults | |
| phase_mandatory = _phase_vault_get_for_phase(phase) | |
| # Combine all mandatory vaults | |
| mandatory_vaults = global_mandatory + phase_mandatory | |
| # Get global optional vaults (always loaded optionally) | |
| global_optional = _phase_vault_get_for_phase("all-optional") | |
| # Get phase-specific optional vaults | |
| phase_optional = _phase_vault_get_for_phase(f"{phase}-optional") | |
| # Combine all optional vaults | |
| optional_vaults = global_optional + phase_optional | |
| if not mandatory_vaults and not optional_vaults: | |
| logger.info(f"No vault to initialize for phase '{phase}'") | |
| return | |
| # Initialize both mandatory and optional vaults in a single call | |
| # Mandatory vaults: strict=True (automation fails if missing/invalid) | |
| # Optional vaults: strict=False (automation continues with warnings if missing/invalid) | |
| init(mandatory_vaults=mandatory_vaults, optional_vaults=optional_vaults) | |
| def phase_vault_init(phase: str) -> None: | |
| """Initialize vaults for a specific phase.""" | |
| from projects.core.library import config | |
| vault_config = config.project.get_config("vaults", []) | |
| if isinstance(vault_config, list): | |
| if not vault_config: | |
| logger.info(f"No vault to initialize for phase '{phase}'") | |
| return | |
| init(vaults=vault_config) | |
| return | |
| # Get global mandatory vaults (always loaded) | |
| global_mandatory = _phase_vault_get_for_phase("all") | |
| # Get phase-specific mandatory vaults | |
| phase_mandatory = _phase_vault_get_for_phase(phase) | |
| # Combine all mandatory vaults | |
| mandatory_vaults = global_mandatory + phase_mandatory | |
| # Get global optional vaults (always loaded optionally) | |
| global_optional = _phase_vault_get_for_phase("all-optional") | |
| # Get phase-specific optional vaults | |
| phase_optional = _phase_vault_get_for_phase(f"{phase}-optional") | |
| # Combine all optional vaults | |
| optional_vaults = global_optional + phase_optional | |
| if not mandatory_vaults and not optional_vaults: | |
| logger.info(f"No vault to initialize for phase '{phase}'") | |
| return | |
| # Initialize both mandatory and optional vaults in a single call | |
| # Mandatory vaults: strict=True (automation fails if missing/invalid) | |
| # Optional vaults: strict=False (automation continues with warnings if missing/invalid) | |
| init(mandatory_vaults=mandatory_vaults, optional_vaults=optional_vaults) |
🤖 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 `@projects/core/library/vault.py` around lines 492 - 520, phase_vault_init
currently only reads phase-scoped keys via _phase_vault_get_for_phase, so legacy
flat vault lists are ignored and nothing gets initialized. Update
phase_vault_init to fall back to the existing flat vault list when the
phase-based lookups return empty, while still preserving the current
all/all-optional and phase-specific behavior. Use the existing symbols
phase_vault_init, _phase_vault_get_for_phase, phase_vault_list_all, and init to
keep compatibility with both legacy and phased vault configurations.
| vault_config = config.project.get_config("vaults") | ||
|
|
||
| # Handle both old format (list) and new format (dict with categories) | ||
| if isinstance(vault_config, list): | ||
| return vault_config | ||
|
|
||
| # New format: collect all vaults from all categories | ||
| all_vaults = [] | ||
| for _category, vaults in vault_config.items(): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle missing vaults before calling .items().
Line 527 has no default, then Line 535 assumes a mapping. A project without a vaults stanza can crash resolve-fournos-config instead of producing an empty secretRefs list.
Proposed fix
- vault_config = config.project.get_config("vaults")
+ vault_config = config.project.get_config("vaults", [])
# Handle both old format (list) and new format (dict with categories)
if isinstance(vault_config, list):
return vault_config
+
+ if not vault_config:
+ return []
+
+ if not isinstance(vault_config, dict):
+ raise TypeError("vaults must be a list or mapping of phase/category names to lists")
# New format: collect all vaults from all categories📝 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.
| vault_config = config.project.get_config("vaults") | |
| # Handle both old format (list) and new format (dict with categories) | |
| if isinstance(vault_config, list): | |
| return vault_config | |
| # New format: collect all vaults from all categories | |
| all_vaults = [] | |
| for _category, vaults in vault_config.items(): | |
| vault_config = config.project.get_config("vaults", []) | |
| # Handle both old format (list) and new format (dict with categories) | |
| if isinstance(vault_config, list): | |
| return vault_config | |
| if not vault_config: | |
| return [] | |
| if not isinstance(vault_config, dict): | |
| raise TypeError("vaults must be a list or mapping of phase/category names to lists") | |
| # New format: collect all vaults from all categories | |
| all_vaults = [] | |
| for _category, vaults in vault_config.items(): |
🤖 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 `@projects/core/library/vault.py` around lines 527 - 535, Handle the missing
vaults config before iterating in resolve-fournos-config: the vault_config value
from config.project.get_config("vaults") can be absent or non-mapping, so update
the vault handling in the vault resolution logic to treat a missing value as
empty and only call .items() on a dict-like new-format config. Keep the existing
list-vs-dict behavior in vault_config processing, but add a safe fallback so a
project without a vaults stanza returns an empty secretRefs list instead of
crashing.
|
🟢 Test of 'skeleton test' succeeded after 00 hours 00 minutes 00 seconds 🟢 • Link to the test results. • Generated 2 Caliper report(s):
Test configuration: |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes