[caliper] Extend the notification and include artifacts censoring - #145
[caliper] Extend the notification and include artifacts censoring#145kpouget wants to merge 17 commits into
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
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds artifact censoring rules, vault-secret discovery, CLI and orchestration integration, export controls, enhanced notifications, MLflow configuration wiring, and richer CI status metadata. ChangesCaliper artifact censoring
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant OrchestrationExport
participant Censoring
participant MLflow
participant Notifications
CLI->>OrchestrationExport: invoke export with censoring and file-export flags
OrchestrationExport->>Censoring: scan and sanitize artifact files
Censoring-->>OrchestrationExport: return censoring status and report data
OrchestrationExport->>MLflow: export processed artifacts
MLflow-->>OrchestrationExport: return export status
OrchestrationExport->>Notifications: provide status and artifact details
Notifications-->>CLI: write notification files and return outcome
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/core/library/export.py (1)
432-454: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
logger, and don't emit placeholder link targets. Line 436 uses the rootlogginglogger instead of the modulelogger. More importantly,_create_mlflow_urlnow returnsf"BASE_URL_MISSING/{step_dir_name}"(line 229) and this code falls back to"NO_URL", so the notification renders markdown links pointing at non-URLs. Prefer emitting plain text for the step title when no MLflow URL is 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 `@projects/core/library/export.py` around lines 432 - 454, The _process_step_status function uses the root logging API and creates placeholder link targets when _create_mlflow_url cannot produce a valid URL. Replace the warning call with the module-level logger, and when mlflow_log_url is unavailable, emit the step title as plain text rather than constructing a markdown link with "NO_URL" or another placeholder target.projects/caliper/orchestration/export.py (1)
237-308: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winTwo control-flow problems in the new fall-through to
status.yaml.
- Lines 298-299 raise
CensoringOccurredExceptionbeforestatus.yamlis read, so the caller never receives the real export status. Inprojects/core/library/export.pythe handler substitutes{"backends": {}}, which drops the MLflow run/experiment URLs from the notification — exactly the case where censoring info matters most. Prefer reading/returning the status first and signalling censoring via the returnedcensoring_occurredfield (the caller already checksstatus.get("censoring_occurred")at line 1124), or attach the status to the exception.- The dry-run paths now fall through to
open(status_yaml)at line 301: the multi-run dry-run branch (lines 238-241) only logs and skips, andrun_artifacts_export(dry_run=True)is unlikely to write the file either, so this raisesFileNotFoundError(andstatusmay beNonefor multi-run before line 305). Return early fordry_run.🤖 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 237 - 308, Fix the export control flow around the single-run and multi-run branches: return the real status from status_yaml before signaling censoring, preserving the censoring_occurred field for callers instead of raising CensoringOccurredException first. Add an early dry_run return before the final status file read, covering both the multi-run skip path and run_artifacts_export(dry_run=True), and ensure no missing or None status reaches the final status handling.
🧹 Nitpick comments (17)
projects/caliper/engine/file_export/censoring_rules.py (2)
85-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pattern.matchonly works because every filename pattern is.*-prefixed.Any future entry added to
SENSITIVE_FILE_PATTERNSwithout a leading.*will silently never match a full path.pattern.searchmakes the intent explicit and removes the hidden coupling.♻️ Proposed change
- return any(pattern.match(filename) for pattern in COMPILED_FILE_PATTERNS) + return any(pattern.search(filename) for pattern in COMPILED_FILE_PATTERNS)(with the leading
.*then removable from the pattern list)🤖 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/engine/file_export/censoring_rules.py` at line 85, Update the filename matching in the censoring-rule function to use pattern.search instead of pattern.match, and remove the now-unnecessary leading .* prefixes from SENSITIVE_FILE_PATTERNS while preserving matching against any path segment.
28-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant case variant under
re.IGNORECASE.
COMPILED_KEYWORD_PATTERNScompiles every pattern withre.IGNORECASE(Line 50), so the lowercasebearerentry is dead weight and only inflates the scan loop.♻️ Proposed cleanup
# Bearer tokens r"Bearer\s+[A-Za-z0-9+/=]+", - r"bearer\s+[A-Za-z0-9+/=]+",🤖 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/engine/file_export/censoring_rules.py` around lines 28 - 29, Remove the redundant lowercase bearer pattern from COMPILED_KEYWORD_PATTERNS’ source pattern list, retaining the case-insensitive Bearer pattern because compilation already uses re.IGNORECASE.projects/caliper/cli/commands.py (2)
1040-1046: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant existence checks; let click enforce the directory contract.
The option already declares
exists=True, so Lines 1075-1077 are unreachable. Addingfile_okay=Falsealso makes Lines 1079-1081 unnecessary.♻️ Proposed cleanup
- type=click.Path(path_type=Path, exists=True), + type=click.Path(path_type=Path, exists=True, file_okay=False, dir_okay=True),Also applies to: 1075-1081
🤖 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/cli/commands.py` around lines 1040 - 1046, Update the from_path click option to enforce the directory contract directly by adding file_okay=False alongside exists=True, then remove the redundant existence and directory validation branches in the related command logic around from_path.
1115-1144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport format diverges from the orchestration report, and the full body is echoed.
projects/caliper/orchestration/censoring.pywrites YAML (censoring_report.yaml) andprojects/core/library/export_notifications.pyLine 455 keys its links on that exact name; this command emits JSON with a different schema (censored_detailsvscensored_by_reason). Also, Line 1141 dumps the whole report to stdout, which is noisy for large artifact trees — the path alone is enough.🤖 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/cli/commands.py` around lines 1115 - 1144, Update the report-writing block in the command to emit YAML to the canonical filename censoring_report.yaml, matching the schema produced by orchestration/censoring.py, including censored_by_reason instead of censored_details. Replace the full report body echoed after writing with a concise message containing only the report path, while preserving dry-run behavior and error handling.projects/caliper/engine/file_export/censoring.py (1)
99-107: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
read_bytes()[:1024]loads the entire file to inspect 1 KB.For extension-less artifacts of arbitrary size (logs, dumps, archives) this is a needless full read on the hot path. Read only the sample.
♻️ Proposed fix
- sample = file_path.read_bytes()[:1024] + with file_path.open("rb") as f: + sample = f.read(1024)🤖 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/engine/file_export/censoring.py` around lines 99 - 107, Update the extension-less file inspection logic around the sample calculation to read only the first 1024 bytes from the file stream instead of calling file_path.read_bytes() and loading the entire file. Preserve the existing empty-file handling and printable_ratio classification behavior.projects/core/library/vault.py (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
is_sensibleis a pass-through over a public field.The property adds no encapsulation since
sensibleis already public, and consumers now have two names for one concept. Also considersensitive— "sensible" reads as a false friend in English and this name is now baked into the vault YAML schema, so renaming later is a breaking change.♻️ Optional simplification
- `@property` - def is_sensible(self) -> bool: - """Whether this content should be considered sensitive for censoring purposes""" - return self.sensible -with
content_def.is_sensible→content_def.sensibleinprojects/caliper/orchestration/censoring.pyLine 71.🤖 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 42 - 45, Remove the redundant is_sensible property from the content definition class and use the existing sensible field directly in consumers such as the censoring logic. Rename the public schema field to sensitive instead of sensible, updating its YAML serialization/deserialization and all references consistently.projects/caliper/orchestration/censoring.py (2)
24-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
VAULT_CONTENT_TYPESis never used.
discover_vault_secretsiterates all vault content and filters only onis_sensible, so this list is dead code that implies a name-based filter that does not exist. Remove it, or wire it in if name-based selection was intended.🤖 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/censoring.py` around lines 24 - 38, Remove the unused VAULT_CONTENT_TYPES constant, since discover_vault_secrets currently filters vault content only through is_sensible and does not perform name-based selection. Do not add name-based filtering unless the surrounding implementation explicitly requires that behavior.
178-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
apply_multi_run_censoringduplicatesapply_orchestration_censoringverbatim.The two bodies differ only in log message prefixes. Any fix (including the exclusion gap above) has to be applied twice, which is exactly how the two paths will drift.
♻️ Proposed consolidation
+def _apply_censoring( + from_path: Path, + export_cfg: CaliperOrchestrationExportConfig, + disable_censoring: bool = False, + label: str = "", +) -> bool: + ... # single implementation, `label` used in log prefixes + + +def apply_multi_run_censoring(from_path, export_cfg, disable_censoring=False) -> bool: + return _apply_censoring(from_path, export_cfg, disable_censoring, label="multi-run ")🤖 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/censoring.py` around lines 178 - 244, Consolidate apply_multi_run_censoring with apply_orchestration_censoring by extracting their shared censoring workflow into one helper or making one function delegate to the other, parameterizing only the differing log prefix or context. Preserve each function’s public behavior, including disable-censoring handling, reporting, notifications, return values, and exclusion processing, so future fixes apply through a single implementation.projects/caliper/orchestration/export.py (4)
371-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the redundant first scan.
all_artifact_pathsat line 371 is overwritten at line 377 before any use; censoring runs in-place between them, so only the second scan matters.♻️ Suggested fix
- all_artifact_paths = [p for p in from_path.rglob("*") if p.is_file()] - # Apply censoring for multi-run export if enabled censoring_occurred = apply_multi_run_censoring(from_path, export_cfg, disable_censoring) # Update artifact paths - use original paths since we modified in-place all_artifact_paths = [p for p in from_path.rglob("*") if p.is_file()]🤖 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 371 - 378, Remove the initial all_artifact_paths scan before apply_multi_run_censoring in the export flow. Keep the post-censoring scan because it is the only assignment used after in-place modifications.
472-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
raise ... from Nonediscards the original traceback. Usefrom eto keep the cause chain for debugging.♻️ Suggested fix
- raise ExportFailedException(f"Failed to write status YAML: {e}") from None + raise ExportFailedException(f"Failed to write status YAML: {e}") from e🤖 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 472 - 493, Update the OSError handler in the status YAML export block to raise ExportFailedException from the caught exception e instead of suppressing its cause with from None. Preserve the existing error message and handling in the surrounding status_yaml flow.
420-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock branch inside
trymasks nothing, but hardcoded localhost URLs leak into real notifications. When--disable-file-exportis used in CI,run_url/experiment_urlpoint athttp://localhost:5000/...and are rendered as clickable "MLflow Run" links in the notification. Consider marking them clearly (e.g.DISABLED_EXPORTsentinel) so readers aren't given dead links.🤖 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 420 - 466, Update the disable_file_export mock metadata in the multi-run export flow so run_url and experiment_url use an unmistakable DISABLED_EXPORT sentinel instead of localhost URLs. Keep the mock success result and other metadata unchanged, ensuring notifications cannot render the values as misleading clickable MLflow links.
169-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn annotation is stale. The function now returns the parsed
statusdict (line 308), not anint.♻️ Suggested fix
-) -> int: +) -> dict[str, Any]:🤖 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 169 - 171, Update the return annotation of the affected function to reflect that it returns the parsed status dictionary rather than an int. Locate the function by its parameters disable_censoring and disable_file_export, and keep its existing return behavior unchanged.projects/core/library/export.py (4)
406-410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn annotation contradicts the body. The function returns
list[str](return []/return notifications_from_files) but is annotated-> None.♻️ Suggested fix
-def _process_notification_files(step_dir: Path) -> None: +def _process_notification_files(step_dir: Path) -> list[str]:🤖 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/export.py` around lines 406 - 410, Update the return annotation of _process_notification_files to list[str] so it matches the empty-list and notifications_from_files return values; leave the existing processing behavior unchanged.
890-892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
artifact_diris accepted but unused. The directory is communicated to the orchestration layer viaconfig.project.set_config("caliper.export.from", ...)in the entrypoint, so this parameter is dead. Either use it (e.g. set the config here) or drop it.🤖 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/export.py` around lines 890 - 892, Remove the unused artifact_dir parameter from run_caliper_orchestration_export, and update all call sites to stop passing it; retain the existing configuration flow through config.project.set_config("caliper.export.from", ...) in the entrypoint.
612-618: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSignature says
-> str, body returns(str, bool).send_notificationunpacks two values, so fix the annotation.♻️ Suggested fix
-) -> str: +) -> tuple[str, bool]:Also applies to: 685-685
🤖 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/export.py` around lines 612 - 618, Update the return annotation of _build_enhanced_notification to describe the two-value tuple it actually returns, matching the unpacking performed by send_notification. Apply the same correction to the related annotation at the additional location.
688-701: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the shadowing local imports.
Pathandyamlare already imported at module scope; re-importing inside these helpers adds noise.Also applies to: 758-767
🤖 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/export.py` around lines 688 - 701, Remove the redundant local imports of Path and yaml from _extract_test_labels_info and the additionally referenced helper around the indicated second location, relying on the existing module-level imports while leaving the helper behavior unchanged.projects/core/library/export_notifications.py (1)
102-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRebinding the
artifact_dirparameter toenv.ARTIFACT_DIRis surprising. Line 107 overwrites the caller-supplied directory just to build the Slack context; use a distinct local name to avoid confusion for future readers.🤖 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/export_notifications.py` around lines 102 - 115, Rename the local value derived from env.ARTIFACT_DIR in the notification export flow to a distinct context-specific name, then pass that renamed variable to NotificationContext while preserving the caller-supplied artifact_dir parameter unchanged.
🤖 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/cli/commands.py`:
- Around line 1098-1099: Update the `censored_files` and `clean_files`
comprehensions to distinguish sanitized results from files that were only
censored: include a result in `censored_files` only when `censored` is true and
`sanitized` is false, and include sanitized results in `clean_files` so both
in-place and `--output` modes retain them.
- Around line 1092-1095: Propagate the command’s dry_run state into
apply_censoring_to_artifacts and the underlying censoring flow, adding a dry_run
parameter where needed; when enabled, skip the write_text operation while
preserving censoring analysis and results. Update the call site near the
existing apply_censoring_to_artifacts invocation so dry-run mode does not modify
files.
In `@projects/caliper/engine/file_export/censoring.py`:
- Around line 166-168: Update the exception path in the file sanitization flow
to fail closed: when the operation around the censoring/write-back logic raises,
return a CensoringResult with censored=True so the file is excluded from export.
Preserve the existing warning and failure message details.
- Around line 134-147: Update the keyword-handling logic around
COMPILED_KEYWORD_PATTERNS so each matched span is redacted in place rather than
replacing the entire content with “Content censored by caliper”. Preserve the
existing reason reporting using KEYWORD_PATTERNS, mark sanitized when any match
is found, and retain all non-matching content.
In `@projects/caliper/orchestration/censoring.py`:
- Around line 140-151: Ensure both orchestration export paths remove or
quarantine every result marked censored and not sanitized before exporting, or
pass the processed_paths allow-list to the exporter instead of exporting the
entire from_path tree. Apply this handling in the single-run flow around the
clean_files/sanitized_files/excluded_files classification at
projects/caliper/orchestration/censoring.py lines 140-151 and in the multi-run
flow at lines 209-220; both sites require the same change.
In `@projects/core/library/ci.py`:
- Line 249: Remove the logger.info call that logs the caller-provided message in
the notification flow; retain the existing created-path log and do not log the
full payload or its sensitive contents.
- Around line 172-184: Sanitize the reason assigned in the command execution
flow before it reaches persistence or logging. In the result-handling branch,
accept only an explicit allowlist of safe reason values; for exceptions handled
by handle_ci_exception, set reason to the generic command_failed value instead
of str(e). Ensure the downstream artifact and log handling uses only this
sanitized reason.
In `@projects/core/library/config.py`:
- Around line 215-219: Update the configuration-processing flow around the
deferred file write so messages for keys successfully applied before a later
exception are still appended to presets_applied.txt. Flush accumulated
file_messages in a finally path, or keep one append handle open during
processing, while preserving the existing message format and partial-application
behavior.
- Around line 167-173: Update the preset/override message construction and
persistence flow around dest_txt, file_messages, and the related logging to
never interpolate raw value or resolved actual_value; record only the key or an
explicit safe redaction. Avoid resolving secret references solely for message
formatting, including the handled_secretly=False path, while preserving
non-sensitive override reporting and writing only redacted content under
ARTIFACT_DIR.
- Around line 193-213: Update the config override handling around set_config and
the new-key branch so each applied override appends its sanitized metadata
message to file_messages regardless of log. Restrict log to guarding logger.info
calls only, preserving the existing message content and save behavior; ensure
ignored overrides remain unchanged unless they are also considered applied.
In `@projects/core/library/export_notifications.py`:
- Around line 354-359: Update the notification URL construction around
mlflow_log_url and step_title to reuse _create_mlflow_file_url_for_step instead
of concatenating paths onto mlflow_run_url, preserving query parameters and
fragments. Ensure step_status always receives a step heading: keep the linked
title when mlflow_run_url exists and append an equivalent plain-text title when
it does not.
- Around line 182-256: The notification-building logic is duplicated between
this module and projects/core/library/export.py; choose one canonical
implementation and remove the duplicate definitions, updating callers to import
and reuse the canonical symbols including _build_enhanced_notification,
_extract_artifact_links, _process_notification_files, _process_step_details,
_create_mlflow_file_url_for_step, and CI_METADATA_DIRNAME. Ensure
CI_METADATA_DIRNAME is imported from projects.core.ci_entrypoint.prepare_ci and
preserve the existing public behavior through the shared implementation.
- Around line 49-67: Update the dry_run flow in the notification export function
so the notification file-writing block executes before returning
notification_success. Preserve the existing dry-run logging and ensure the later
dry_run-specific file log remains reachable; remove the early return before the
file write.
- Around line 517-537: Update get_file_link to return a valid fallback or omit
the link when mlflow_run_url is absent, preventing None from being interpolated
into notifications. Update _create_mlflow_file_url_for_step to accept the string
paths supplied by notification callers by normalizing file_path to a Path before
using name or is_file, while preserving URL generation for both Path and str
inputs.
In `@projects/core/library/export.py`:
- Around line 705-750: Update _process_step_details and the __test_labels__.yaml
processing so MLflow links retain the step-directory prefix when artifact_dir is
the step directory. Pass the step name explicitly or compute the path relative
to the artifact root, and use that prefixed path in
_create_mlflow_file_url_for_step; ensure the displayed directory name is the
step name rather than "root" for files directly inside a step directory.
- Around line 1001-1029: Normalize the resolved artifact_dir to a pathlib.Path
before it is stored in config or passed to notification generation. Apply this
after the precedence resolution and before the dry-run/logging and set_config
flow, while preserving the existing FOURNOS_CI parent-directory handling and
error return for missing values.
In `@vaults/psap-forge-mlflow-export.yaml`:
- Around line 20-22: Update the `description` for the `mlflow username` vault
entry to explicitly state why it is intentionally marked `sensible: false` and
exempt from CI log censoring, replacing the conflicting claim that it is stored
for censoring.
---
Outside diff comments:
In `@projects/caliper/orchestration/export.py`:
- Around line 237-308: Fix the export control flow around the single-run and
multi-run branches: return the real status from status_yaml before signaling
censoring, preserving the censoring_occurred field for callers instead of
raising CensoringOccurredException first. Add an early dry_run return before the
final status file read, covering both the multi-run skip path and
run_artifacts_export(dry_run=True), and ensure no missing or None status reaches
the final status handling.
In `@projects/core/library/export.py`:
- Around line 432-454: The _process_step_status function uses the root logging
API and creates placeholder link targets when _create_mlflow_url cannot produce
a valid URL. Replace the warning call with the module-level logger, and when
mlflow_log_url is unavailable, emit the step title as plain text rather than
constructing a markdown link with "NO_URL" or another placeholder target.
---
Nitpick comments:
In `@projects/caliper/cli/commands.py`:
- Around line 1040-1046: Update the from_path click option to enforce the
directory contract directly by adding file_okay=False alongside exists=True,
then remove the redundant existence and directory validation branches in the
related command logic around from_path.
- Around line 1115-1144: Update the report-writing block in the command to emit
YAML to the canonical filename censoring_report.yaml, matching the schema
produced by orchestration/censoring.py, including censored_by_reason instead of
censored_details. Replace the full report body echoed after writing with a
concise message containing only the report path, while preserving dry-run
behavior and error handling.
In `@projects/caliper/engine/file_export/censoring_rules.py`:
- Line 85: Update the filename matching in the censoring-rule function to use
pattern.search instead of pattern.match, and remove the now-unnecessary leading
.* prefixes from SENSITIVE_FILE_PATTERNS while preserving matching against any
path segment.
- Around line 28-29: Remove the redundant lowercase bearer pattern from
COMPILED_KEYWORD_PATTERNS’ source pattern list, retaining the case-insensitive
Bearer pattern because compilation already uses re.IGNORECASE.
In `@projects/caliper/engine/file_export/censoring.py`:
- Around line 99-107: Update the extension-less file inspection logic around the
sample calculation to read only the first 1024 bytes from the file stream
instead of calling file_path.read_bytes() and loading the entire file. Preserve
the existing empty-file handling and printable_ratio classification behavior.
In `@projects/caliper/orchestration/censoring.py`:
- Around line 24-38: Remove the unused VAULT_CONTENT_TYPES constant, since
discover_vault_secrets currently filters vault content only through is_sensible
and does not perform name-based selection. Do not add name-based filtering
unless the surrounding implementation explicitly requires that behavior.
- Around line 178-244: Consolidate apply_multi_run_censoring with
apply_orchestration_censoring by extracting their shared censoring workflow into
one helper or making one function delegate to the other, parameterizing only the
differing log prefix or context. Preserve each function’s public behavior,
including disable-censoring handling, reporting, notifications, return values,
and exclusion processing, so future fixes apply through a single implementation.
In `@projects/caliper/orchestration/export.py`:
- Around line 371-378: Remove the initial all_artifact_paths scan before
apply_multi_run_censoring in the export flow. Keep the post-censoring scan
because it is the only assignment used after in-place modifications.
- Around line 472-493: Update the OSError handler in the status YAML export
block to raise ExportFailedException from the caught exception e instead of
suppressing its cause with from None. Preserve the existing error message and
handling in the surrounding status_yaml flow.
- Around line 420-466: Update the disable_file_export mock metadata in the
multi-run export flow so run_url and experiment_url use an unmistakable
DISABLED_EXPORT sentinel instead of localhost URLs. Keep the mock success result
and other metadata unchanged, ensuring notifications cannot render the values as
misleading clickable MLflow links.
- Around line 169-171: Update the return annotation of the affected function to
reflect that it returns the parsed status dictionary rather than an int. Locate
the function by its parameters disable_censoring and disable_file_export, and
keep its existing return behavior unchanged.
In `@projects/core/library/export_notifications.py`:
- Around line 102-115: Rename the local value derived from env.ARTIFACT_DIR in
the notification export flow to a distinct context-specific name, then pass that
renamed variable to NotificationContext while preserving the caller-supplied
artifact_dir parameter unchanged.
In `@projects/core/library/export.py`:
- Around line 406-410: Update the return annotation of
_process_notification_files to list[str] so it matches the empty-list and
notifications_from_files return values; leave the existing processing behavior
unchanged.
- Around line 890-892: Remove the unused artifact_dir parameter from
run_caliper_orchestration_export, and update all call sites to stop passing it;
retain the existing configuration flow through
config.project.set_config("caliper.export.from", ...) in the entrypoint.
- Around line 612-618: Update the return annotation of
_build_enhanced_notification to describe the two-value tuple it actually
returns, matching the unpacking performed by send_notification. Apply the same
correction to the related annotation at the additional location.
- Around line 688-701: Remove the redundant local imports of Path and yaml from
_extract_test_labels_info and the additionally referenced helper around the
indicated second location, relying on the existing module-level imports while
leaving the helper behavior unchanged.
In `@projects/core/library/vault.py`:
- Around line 42-45: Remove the redundant is_sensible property from the content
definition class and use the existing sensible field directly in consumers such
as the censoring logic. Rename the public schema field to sensitive instead of
sensible, updating its YAML serialization/deserialization and all references
consistently.
🪄 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 Plus
Run ID: 54eefc44-f3cf-4e10-aa24-7d1ce6abacd2
📒 Files selected for processing (14)
projects/caliper/cli/commands.pyprojects/caliper/cli/main.pyprojects/caliper/engine/file_export/censoring.pyprojects/caliper/engine/file_export/censoring_rules.pyprojects/caliper/orchestration/censoring.pyprojects/caliper/orchestration/export.pyprojects/caliper/orchestration/notification.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/library/ci.pyprojects/core/library/config.pyprojects/core/library/export.pyprojects/core/library/export_notifications.pyprojects/core/library/vault.pyvaults/psap-forge-mlflow-export.yaml
| censored_files = [r.file_path for r in censoring_results if r.censored] | ||
| clean_files = [r.file_path for r in censoring_results if not r.censored] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Sanitized files are misclassified, causing data loss in both output modes.
CensoringResult.censored is True for sanitized files too (censoring.py Line 162), so:
censored_filesincludes them → the in-place branch at Lines 1175-1176unlink()s files that were already safely sanitized.clean_filesexcludes them →--outputmode silently omits them from the exported copy.
Only filename-excluded results (censored and not sanitized) should be removed/omitted.
🐛 Proposed fix
- # Separate censored and clean files
- censored_files = [r.file_path for r in censoring_results if r.censored]
- clean_files = [r.file_path for r in censoring_results if not r.censored]
+ # Files excluded outright (sensitive filename) vs files safe to export
+ censored_files = [r.file_path for r in censoring_results if r.censored and not r.sanitized]
+ clean_files = [r.file_path for r in censoring_results if not r.censored or r.sanitized]📝 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.
| censored_files = [r.file_path for r in censoring_results if r.censored] | |
| clean_files = [r.file_path for r in censoring_results if not r.censored] | |
| # Files excluded outright (sensitive filename) vs files safe to export | |
| censored_files = [r.file_path for r in censoring_results if r.censored and not r.sanitized] | |
| clean_files = [r.file_path for r in censoring_results if not r.censored or r.sanitized] |
🤖 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/cli/commands.py` around lines 1098 - 1099, Update the
`censored_files` and `clean_files` comprehensions to distinguish sanitized
results from files that were only censored: include a result in `censored_files`
only when `censored` is true and `sanitized` is false, and include sanitized
results in `clean_files` so both in-place and `--output` modes retain them.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
projects/core/library/ci.py (2)
249-249: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon't log the full notification payload.
messageis caller-supplied content; Line 248 already records the created path. Drop this line or log a redacted summary.As per coding guidelines, "Never print, log, or include secret values in error messages."
🤖 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/ci.py` at line 249, Remove the logger.info call that logs the caller-supplied message payload, preserving the existing path logging on the preceding line; if logging is required, replace it with a non-sensitive redacted summary that excludes secret values.Source: Coding guidelines
172-184: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
reasonfrom exception text is persisted toexit_status.yamland logged.Line 184 captures arbitrary exception text (which can embed credentials or URLs with userinfo) and Lines 193-204 write it under the CI metadata dir and log it. Persist only allowlisted reason values and use a generic
command_failedfor exceptions.As per coding guidelines, "Never persist secrets or sensitive data ... to
ARTIFACT_DIR, logs, or files."🤖 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/ci.py` around lines 172 - 184, Update the exception path in the command execution wrapper to stop assigning exception text to reason; use the generic allowlisted reason "command_failed" instead. Preserve handle_ci_exception(e) and exit_code = 1, and ensure only allowlisted reason values reach the existing exit_status.yaml persistence and logging flow.Source: Coding guidelines
projects/caliper/cli/commands.py (1)
1098-1099: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winSanitized files are still classified as censored.
apply_censoring_to_artifactssetscensored=Truefor sanitized files too, so the in-place branch (Lines 1175-1176) deletes files that were safely sanitized and--outputmode omits them. Onlycensored and not sanitized(filename-excluded) results should be treated as censored.🐛 Proposed fix
- censored_files = [r.file_path for r in censoring_results if r.censored] - clean_files = [r.file_path for r in censoring_results if not r.censored] + censored_files = [r.file_path for r in censoring_results if r.censored and not r.sanitized] + clean_files = [r.file_path for r in censoring_results if not r.censored or r.sanitized]🤖 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/cli/commands.py` around lines 1098 - 1099, Update the comprehensions building censored_files and clean_files to distinguish sanitized results from filename-excluded results: classify a result as censored only when r.censored is true and r.sanitized is false, while retaining sanitized files in the clean_files collection so in-place and --output modes preserve them.
🧹 Nitpick comments (7)
projects/core/library/export_notifications.py (1)
107-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon't rebind the
artifact_dirparameter. The Slack provider getsenv.ARTIFACT_DIRwhile_build_enhanced_notificationused the caller-suppliedartifact_dir, so the two channels can describe different directories. Use a distinct local name and prefer the passed value.- artifact_dir = Path(env.ARTIFACT_DIR) if env.ARTIFACT_DIR else None + context_artifact_dir = artifact_dir or ( + Path(env.ARTIFACT_DIR) if env.ARTIFACT_DIR else None + ) context = NotificationContext( ... - artifact_dir=artifact_dir, + artifact_dir=context_artifact_dir, )🤖 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/export_notifications.py` around lines 107 - 114, Update the notification-building flow around _build_enhanced_notification to avoid rebinding the artifact_dir parameter: store env.ARTIFACT_DIR in a distinct local variable, then set NotificationContext.artifact_dir to the caller-supplied artifact_dir when present, falling back to the environment-derived value only when needed.projects/caliper/orchestration/export.py (1)
371-377: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDead first
rglobscan.Line 371's list is discarded and recomputed at Line 377; drop it and collect paths only after censoring.
♻️ Proposed cleanup
- all_artifact_paths = [p for p in from_path.rglob("*") if p.is_file()] - # Apply censoring for multi-run export if enabled censoring_occurred = apply_multi_run_censoring(from_path, export_cfg, disable_censoring) # Update artifact paths - use original paths since we modified in-place all_artifact_paths = [p for p in from_path.rglob("*") if p.is_file()]🤖 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 371 - 377, Remove the initial unused all_artifact_paths rglob scan before apply_multi_run_censoring. Keep the single artifact-path collection after censoring so paths reflect the in-place modifications.projects/caliper/cli/commands.py (1)
1139-1142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid echoing the whole report back.
report_path.read_text()duplicates the entire report to stdout on every run; the path log is enough, and verbose mode already lists censored files.♻️ Proposed tweak
- click.echo( - f"\nCensoring report written to {report_path}\n{report_path.read_text()}" - ) + click.echo(f"\nCensoring report written to {report_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/cli/commands.py` around lines 1139 - 1142, Remove the report_path.read_text() interpolation from the click.echo call in the report-writing flow, leaving only a concise message containing report_path. Keep the existing report generation and verbose censored-file output unchanged.projects/caliper/engine/file_export/censoring_rules.py (1)
53-72: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRemove the duplicate case-insensitive bearer pattern.
KEYWORD_PATTERNSis compiled withre.IGNORECASE, sor"Bearer\s+[A-Za-z0-9+/=]+"already matches lowercase bearer tokens; keep only one case-normalized bearer pattern.🤖 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/engine/file_export/censoring_rules.py` around lines 53 - 72, Remove the duplicate case-insensitive bearer-token regex from KEYWORD_PATTERNS, retaining a single bearer pattern that works with the existing re.IGNORECASE compilation. Leave the other sensitive-file patterns and compilation logic unchanged.projects/caliper/orchestration/censoring.py (2)
293-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
export_blocked=Truepath is now unreachable.Both call sites pass
export_blocked=False, and given the engine's current behavior (see companion comment inprojects/caliper/engine/file_export/censoring.py),excluded_filesis always empty, so this branch and thecensoring_blockednotification can never fire. Root cause and full context noted in the consolidated comment.Also applies to: 317-334
🤖 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/censoring.py` around lines 293 - 296, Remove the unused export_blocked parameter and unreachable censoring_blocked notification branch from _create_censoring_notification, then update both call sites to use the simplified signature. Preserve the remaining censoring notification behavior for actual censoring results.
108-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate implementations of the single-run and multi-run censoring flows.
apply_orchestration_censoringandapply_multi_run_censoringare copy-pasted except for log wording. Extracting a shared_apply_censoring(from_path, export_cfg, disable_censoring, label)helper would cut the duplication and avoid the two copies drifting (e.g. a future bugfix landing in only one of them).♻️ Sketch
-def apply_orchestration_censoring(from_path, export_cfg, disable_censoring=False) -> bool: - ... -def apply_multi_run_censoring(from_path, export_cfg, disable_censoring=False) -> bool: - ... +def _apply_censoring(from_path, export_cfg, disable_censoring, label="") -> bool: + if disable_censoring: + ... + vault_secrets, secret_mapping = discover_vault_secrets(verbose=export_cfg.verbose) + all_artifact_paths = [p for p in from_path.rglob("*") if p.is_file()] + ... + +def apply_orchestration_censoring(from_path, export_cfg, disable_censoring=False) -> bool: + return _apply_censoring(from_path, export_cfg, disable_censoring, label="") + +def apply_multi_run_censoring(from_path, export_cfg, disable_censoring=False) -> bool: + return _apply_censoring(from_path, export_cfg, disable_censoring, label="multi-run ")Also applies to: 178-244
🤖 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/censoring.py` around lines 108 - 176, Extract the shared censoring workflow from apply_orchestration_censoring and apply_multi_run_censoring into a helper named _apply_censoring accepting from_path, export_cfg, disable_censoring, and a label for flow-specific log wording. Move the common discovery, processing, result classification, reporting, notification, and return-value logic into that helper, then reduce both public functions to delegate to it with their appropriate labels while preserving existing behavior.projects/caliper/engine/file_export/censoring.py (1)
115-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Excluded" file semantics are now unreachable — everything gets sanitized-in-place instead.
Every branch that sets
censored=Truehere also setssanitized=True(this filename branch, and the keyword/vault branch below). No code path ever returnscensored=True, sanitized=False. That makesCensoringResult.__str__'sEXCLUDEDcase, the module docstring's "Excluding files with sensitive filename patterns" (line 251), and downstream orchestration logic inprojects/caliper/orchestration/censoring.pythat branches onexcluded_files/export_blockedall dead/unreachable. Either update the docs/messaging to reflect "always sanitize in place," or reintroduce a genuine exclusion path if that's still the intended UX for unsanitizable sensitive files.🤖 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/engine/file_export/censoring.py` around lines 115 - 131, Restore a genuine exclusion path in _sanitize_file_content for sensitive files that cannot be safely sanitized, returning censored=True with sanitized=False so CensoringResult.__str__ and orchestration excluded_files/export_blocked handling remain meaningful. Preserve in-place sanitization for files that can be safely rewritten, and update the module documentation only if the intended behavior is instead always-sanitize.
🤖 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/engine/file_export/censoring.py`:
- Around line 144-181: Run vault-secret scanning independently of the
keyword-pattern branch in the censoring flow. Keep keyword matches redacted and
their reasons recorded, then always iterate over self.vault_secrets to replace
detected values, set sanitized, and append the vault identifier reason; remove
the conditional that skips this scan when keyword_detected is true.
- Around line 144-172: Update the keyword-redaction logic around
COMPILED_KEYWORD_PATTERNS to collect all matches in a single scan, track matched
pattern indices, and merge overlapping or adjacent spans before redacting them
in reverse order. Remove the redundant keyword_detected pass and rename the
unused pattern_idx in the reasons loop to an underscore. Preserve recording
reasons for every pattern that matched and set sanitized when merged matches are
redacted.
In `@projects/caliper/orchestration/export.py`:
- Around line 420-436: Update the disable_file_export branch that builds ml_meta
for FileExportBackendResult so it does not populate fake MLflow experiment_url,
run_url, or tracking_uri values. Omit those URL keys or mark them disabled,
while preserving the mock run_id and successful result behavior for
notifications.
- Around line 301-308: Update run_from_orchestration_config to return dict[str,
Any] and guard the status.yaml read for multi-run dry-run execution, returning
an appropriate status without opening the file when no export writes it;
preserve the existing status loading and single-run censoring behavior for
normal executions.
In `@projects/core/library/export_notifications.py`:
- Around line 259-275: Update _get_execution_engine_config to use an explicit
allowlist of approved non-sensitive cluster configuration keys instead of
iterating over every cluster_config entry. Include only allowlisted display
values in the notification, ignore all other keys—including future
credential-like fields—and preserve the existing formatting and None behavior
when no approved values are present.
- Around line 8-13: Add the missing re import alongside the existing
standard-library imports so _process_notification_files can call re.sub without
raising NameError or failing lint.
---
Duplicate comments:
In `@projects/caliper/cli/commands.py`:
- Around line 1098-1099: Update the comprehensions building censored_files and
clean_files to distinguish sanitized results from filename-excluded results:
classify a result as censored only when r.censored is true and r.sanitized is
false, while retaining sanitized files in the clean_files collection so in-place
and --output modes preserve them.
In `@projects/core/library/ci.py`:
- Line 249: Remove the logger.info call that logs the caller-supplied message
payload, preserving the existing path logging on the preceding line; if logging
is required, replace it with a non-sensitive redacted summary that excludes
secret values.
- Around line 172-184: Update the exception path in the command execution
wrapper to stop assigning exception text to reason; use the generic allowlisted
reason "command_failed" instead. Preserve handle_ci_exception(e) and exit_code =
1, and ensure only allowlisted reason values reach the existing exit_status.yaml
persistence and logging flow.
---
Nitpick comments:
In `@projects/caliper/cli/commands.py`:
- Around line 1139-1142: Remove the report_path.read_text() interpolation from
the click.echo call in the report-writing flow, leaving only a concise message
containing report_path. Keep the existing report generation and verbose
censored-file output unchanged.
In `@projects/caliper/engine/file_export/censoring_rules.py`:
- Around line 53-72: Remove the duplicate case-insensitive bearer-token regex
from KEYWORD_PATTERNS, retaining a single bearer pattern that works with the
existing re.IGNORECASE compilation. Leave the other sensitive-file patterns and
compilation logic unchanged.
In `@projects/caliper/engine/file_export/censoring.py`:
- Around line 115-131: Restore a genuine exclusion path in
_sanitize_file_content for sensitive files that cannot be safely sanitized,
returning censored=True with sanitized=False so CensoringResult.__str__ and
orchestration excluded_files/export_blocked handling remain meaningful. Preserve
in-place sanitization for files that can be safely rewritten, and update the
module documentation only if the intended behavior is instead always-sanitize.
In `@projects/caliper/orchestration/censoring.py`:
- Around line 293-296: Remove the unused export_blocked parameter and
unreachable censoring_blocked notification branch from
_create_censoring_notification, then update both call sites to use the
simplified signature. Preserve the remaining censoring notification behavior for
actual censoring results.
- Around line 108-176: Extract the shared censoring workflow from
apply_orchestration_censoring and apply_multi_run_censoring into a helper named
_apply_censoring accepting from_path, export_cfg, disable_censoring, and a label
for flow-specific log wording. Move the common discovery, processing, result
classification, reporting, notification, and return-value logic into that
helper, then reduce both public functions to delegate to it with their
appropriate labels while preserving existing behavior.
In `@projects/caliper/orchestration/export.py`:
- Around line 371-377: Remove the initial unused all_artifact_paths rglob scan
before apply_multi_run_censoring. Keep the single artifact-path collection after
censoring so paths reflect the in-place modifications.
In `@projects/core/library/export_notifications.py`:
- Around line 107-114: Update the notification-building flow around
_build_enhanced_notification to avoid rebinding the artifact_dir parameter:
store env.ARTIFACT_DIR in a distinct local variable, then set
NotificationContext.artifact_dir to the caller-supplied artifact_dir when
present, falling back to the environment-derived value only when needed.
🪄 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 Plus
Run ID: 83754375-4267-400d-ac40-38a25484f8ae
📒 Files selected for processing (12)
projects/caliper/cli/commands.pyprojects/caliper/cli/main.pyprojects/caliper/engine/file_export/censoring.pyprojects/caliper/engine/file_export/censoring_rules.pyprojects/caliper/orchestration/censoring.pyprojects/caliper/orchestration/export.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/library/ci.pyprojects/core/library/export.pyprojects/core/library/export_notifications.pyprojects/core/library/vault.pyvaults/psap-forge-mlflow-export.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- projects/core/ci_entrypoint/prepare_ci.py
- vaults/psap-forge-mlflow-export.yaml
- projects/core/library/vault.py
- projects/caliper/cli/main.py
- projects/core/library/export.py
|
/test fournos skeleton |
❌ Execution of
|
🟢 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (7)
projects/core/library/ci.py (2)
249-249: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the raw notification-payload log.
messageis caller-provided notification content and may contain sensitive data. Keep the created-path log, but do not log the payload.🤖 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/ci.py` at line 249, Remove the logger.info call that interpolates the caller-provided message in the notification flow, while preserving the existing log indicating the notification was created.Source: Coding guidelines
172-204: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not persist or log arbitrary exit reasons.
reasoncan be caller-controlled orstr(e), which may contain credentials. It is written underARTIFACT_DIRand logged. Allowlist safe reason codes; usecommand_failedfor exceptions and avoid logging raw reasons.🤖 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/ci.py` around lines 172 - 204, Restrict exit reasons in the command execution and status-persistence flow to safe allowlisted codes: assign “command_failed” in the exception handler instead of str(e), and only persist or log recognized safe reason values. Update the logic around handle_ci_exception, exit_status_data, and the reason-based logger calls so arbitrary caller-controlled or exception text is never written or logged.Source: Coding guidelines
projects/caliper/engine/file_export/censoring.py (1)
144-172: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOverlapping matches from different patterns still corrupt the redacted output, and the scan runs twice.
Spans are collected against the original
content(Lines 156-159) then applied by slicing in reversestartorder; overlapping spans from two patterns (e.g.token\s*[:=]\s*\S+andaccess[_-]?token\s*[:=]\s*\S+) leave staleendoffsets that garble text or leak part of the match. Merge spans before redacting, and drop the redundant detection pass at Lines 145-151 (which also triggers Ruff B007 on the unusedpattern_idxat Line 165).🐛 Single-pass merge-then-redact
- keyword_detected = False - matched_patterns = set() - for i, pattern in enumerate(COMPILED_KEYWORD_PATTERNS): - matches = list(pattern.finditer(content)) - if matches: - keyword_detected = True - matched_patterns.add(i) - - if keyword_detected: - # Replace all matched spans with redacted text, preserving other content - # Process patterns in reverse order by position to maintain string indices - all_matches = [] - for i, pattern in enumerate(COMPILED_KEYWORD_PATTERNS): - for match in pattern.finditer(content): - all_matches.append((match.start(), match.end(), i)) - - # Sort by start position in reverse order - all_matches.sort(reverse=True) - - # Replace each match with [REDACTED] - for start, end, pattern_idx in all_matches: - content = content[:start] + "[REDACTED]" + content[end:] - - # Add reasons for all matched patterns + spans = [] + matched_patterns = set() + for i, pattern in enumerate(COMPILED_KEYWORD_PATTERNS): + for match in pattern.finditer(content): + spans.append((match.start(), match.end())) + matched_patterns.add(i) + + if spans: + spans.sort() + merged = [] + for start, end in spans: + if merged and start <= merged[-1][1]: + merged[-1] = (merged[-1][0], max(merged[-1][1], end)) + else: + merged.append((start, end)) + + for start, end in reversed(merged): + content = content[:start] + "[REDACTED]" + content[end:] + for pattern_idx in sorted(matched_patterns): reasons.append(f"contains keyword pattern: {KEYWORD_PATTERNS[pattern_idx]}") sanitized = True🤖 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/engine/file_export/censoring.py` around lines 144 - 172, Update the keyword-processing block around COMPILED_KEYWORD_PATTERNS to collect matches and matched pattern indices in one pass, then merge overlapping or adjacent spans before redacting. Apply each merged span against the original content in reverse order, preserve reasons for every matched pattern, and remove the redundant detection loop and unused pattern_idx handling.Source: Pipeline failures
projects/caliper/cli/commands.py (1)
1097-1099: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winSanitized files are still classified as
censored, so they get deleted or dropped.
_sanitize_file_contentreturnscensored=True, sanitized=Truefor files it repaired in place (projects/caliper/engine/file_export/censoring.pyLines 129-131, 198). Consequently the in-place branchunlink()s them (Line 1176) even though they are now safe, and--outputmode omits them from the copy. Onlycensored and not sanitizedresults should be excluded — or simply use thefiltered_pathsallow-list the engine already returns (currently unused).🐛 Proposed fix
- # Separate censored and clean files - censored_files = [r.file_path for r in censoring_results if r.censored] - clean_files = [r.file_path for r in censoring_results if not r.censored] + # Files excluded outright vs files safe to export (clean or sanitized in place) + censored_files = [r.file_path for r in censoring_results if r.censored and not r.sanitized] + clean_files = [r.file_path for r in censoring_results if not r.censored or r.sanitized]🤖 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/cli/commands.py` around lines 1097 - 1099, Update the file classification around censoring_results so repaired files with censored=True and sanitized=True remain eligible for output. Exclude only results that are censored and not sanitized, or reuse the engine’s filtered_paths allow-list consistently in both the in-place unlink flow and the --output copy flow.projects/caliper/orchestration/export.py (2)
419-435: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFake MLflow URLs leak into notifications. With
--disable-file-export,http://DRY_RUN_MLFLOW_FAKE_URL/...values are written intostatus.yamland rendered as clickable artifact links. Omit the URL keys (or mark them disabled) so consumers don't surface bogus links.🤖 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 419 - 435, The mock results created in the disable_file_export branch must not expose fake MLflow links. Update the result metadata in the disable_file_export handling to omit the experiment_url, run_url, and tracking_uri keys (or mark them disabled) while preserving the mock run identifier and successful status.
300-307: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMulti-run dry-run still falls through to reading a non-existent
status.yaml. Whenlen(run_dirs) > 1andexport_cfg.dry_runis set, nothing writesstatus_yaml, yet Line 300 opens it unconditionally →FileNotFoundError.🐛 Suggested guard
+ if not status_yaml.exists(): + logger.info("No status YAML produced (dry-run); returning empty status") + return {} + with open(status_yaml) as f: - status = yaml.safe_load(f.read()) + status = yaml.safe_load(f.read()) or {}🤖 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 300 - 307, Guard the status_yaml read in the export flow so multi-run dry runs do not open a file that was never created. Use the existing run_dirs length and export_cfg.dry_run conditions to return an appropriate status before the unconditional open, while preserving status loading and censoring information for normal and single-run executions.projects/core/library/export_notifications.py (1)
320-331: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBroken artifact URL and missing heading without MLflow.
mlflow_run_urlalready ends in/artifacts(possibly with?workspace=...and a#fragment), so appending/artifacts/{step}/run.logyields e.g..../artifacts?workspace=x/artifacts/<step>/run.log. Reuse_create_mlflow_url(Line 832) instead. Also, whenmlflow_run_urlisNoneno heading is emitted while the step body lines still are, producing orphaned content.🐛 Suggested fix
- mlflow_log_url = ( - f"{mlflow_run_url}/artifacts/{step_name}/run.log" if mlflow_run_url else "#" - ) - if mlflow_run_url: - step_title = f"#### {exit_status_emoji} [{step_name}]({mlflow_log_url}){log_summary}" - step_status.append(step_title) + mlflow_log_url = _create_mlflow_url(mlflow_run_url, step_name) if mlflow_run_url else None + if mlflow_log_url: + step_status.append( + f"#### {exit_status_emoji} [{step_name}]({mlflow_log_url}){log_summary}" + ) + else: + step_status.append(f"#### {exit_status_emoji} {step_name}{log_summary}")🤖 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/export_notifications.py` around lines 320 - 331, Update the notification step formatting around mlflow_log_url and step_title: build the log link with the existing _create_mlflow_url helper so artifact URLs preserve query parameters and fragments, and always append a step heading even when mlflow_run_url is None. Keep the existing step body processing via _process_notification_files and _process_step_details unchanged.
🧹 Nitpick comments (8)
projects/caliper/orchestration/censoring.py (1)
23-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
VAULT_CONTENT_TYPESis dead code.
discover_vault_secretsnow gates oncontent_def.is_sensibleand never consults this list; leaving it invites the wrong assumption that content-name filtering is still applied.🤖 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/censoring.py` around lines 23 - 38, Remove the unused VAULT_CONTENT_TYPES constant from the censoring module, and ensure discover_vault_secrets continues to rely solely on content_def.is_sensible for discovery without introducing another content-name filter.projects/caliper/engine/file_export/censoring.py (1)
175-180: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueVault-secret substring scan is O(files × secrets × size) with repeated
.strip().Pre-normalize the secret set once in
__init__(and drop empty entries there) so this hot loop does a singlein/replaceper secret. Also consider thatsecret_mappingkeys are already stripped upstream indiscover_vault_secrets.🤖 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/engine/file_export/censoring.py` around lines 175 - 180, Pre-normalize non-empty, stripped vault secrets once in __init__ and retain the normalized collection for censoring. Update the loop in the secret-scanning method to iterate over those normalized values without repeated strip calls, while using the already-stripped keys in secret_mapping for vault identifiers.projects/caliper/cli/commands.py (1)
1011-1016: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
--mlflow-insecure-tlscan only be turned on, never off.
mlflow_insecure_tls or config.get("insecure_tls", False)means a config file withinsecure_tls: truewins even when the flag is absent, contradicting the "CLI args override config file values" comment. Same shape as the otherfinal_*lines, but here the insecure default is the risky direction.🤖 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/cli/commands.py` around lines 1011 - 1016, Update the final_insecure_tls resolution in the CLI configuration flow so an explicitly provided --mlflow-insecure-tls value overrides config.get("insecure_tls", False), including an explicit false value, while retaining the config value only when the CLI option is absent. Keep the existing final_* resolution behavior unchanged.projects/caliper/engine/file_export/censoring_rules.py (1)
88-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
find_sensitive_content_in_texthelper.No code consumes it, and
censoring.pyalready implements keyword detection directly, so keeping it only adds an unused public API surface.🤖 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/engine/file_export/censoring_rules.py` around lines 88 - 104, Remove the unused find_sensitive_content_in_text helper and its associated implementation, leaving keyword detection in censoring.py as the sole mechanism.projects/core/library/export.py (2)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImporting underscore-private helpers across modules.
_check_job_shutdown_statusand_create_mlflow_file_url_for_stepare consumed outside their defining module; promote them to public names inexport_notificationsto make the boundary explicit.🤖 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/export.py` around lines 26 - 30, Rename the cross-module helpers _check_job_shutdown_status and _create_mlflow_file_url_for_step to public names in export_notifications, then update their definitions and all imports and call sites, including export.py, to use the new names while leaving send_notification unchanged.
183-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
artifact_diris required but unused. The body relies entirely oncaliper.export.frombeing set by the caller. Either set the config from this parameter here (making the function self-contained) or drop the parameter.♻️ Suggested option
def run_caliper_orchestration_export( *, artifact_dir: Path, disable_censoring: bool = False, disable_file_export: bool = False ): + config.project.set_config("caliper.export.from", str(artifact_dir), print=False)🤖 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/export.py` around lines 183 - 185, Update run_caliper_orchestration_export so artifact_dir is not an unused required parameter: either apply it to the caliper.export.from configuration within the function, making the export self-contained, or remove the parameter and update its callers; preserve the existing export behavior.projects/caliper/orchestration/export.py (2)
372-377: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate full-tree
rglob. Line 370 already walked the whole artifact tree and its result is discarded here; only the post-censoring listing is used. Drop the earlier computation (or move it below censoring) to avoid the redundant recursive scan.♻️ Suggested cleanup
- all_artifact_paths = [p for p in from_path.rglob("*") if p.is_file()] - # Apply censoring for multi-run export if enabled censoring_occurred = apply_orchestration_censoring(from_path, export_cfg, disable_censoring) - # Update artifact paths - use original paths since we modified in-place + # Collect artifact paths after in-place censoring all_artifact_paths = [p for p in from_path.rglob("*") if p.is_file()]🤖 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 372 - 377, Remove the earlier full-tree artifact path computation in the export flow and retain a single recursive listing after apply_orchestration_censoring. Ensure all_artifact_paths is populated from the post-censoring tree so the updated in-place artifacts are used without a redundant scan.
471-486: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead-modify-rewrite of
status.yamlis fragile.yaml.safe_loadcan returnNone(or raiseyaml.YAMLError) — neither is covered by theexcept OSError, sostatus_data["censoring_occurred"] = ...would raiseTypeErrorand escape as an unclassified failure. Also the rewrite drops thesort_keys=False/ block-style formatting used bywrite_artifacts_status_yaml.♻️ Suggested hardening
try: write_artifacts_status_yaml(status_yaml, results) # Add censoring information to the status file with open(status_yaml) as f: - status_data = yaml.safe_load(f) + status_data = yaml.safe_load(f) or {} status_data["censoring_occurred"] = censoring_occurred with open(status_yaml, "w") as f: - yaml.dump(status_data, f, indent=4) + yaml.safe_dump(status_data, f, default_flow_style=False, sort_keys=False) if not disable_file_export: click.echo(f"Wrote status YAML to {status_yaml}") - except OSError as e: + except (OSError, yaml.YAMLError) as e:🤖 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 471 - 486, Harden the status YAML update in the status_yaml export flow around write_artifacts_status_yaml: handle empty or non-mapping safe_load results and yaml.YAMLError alongside OSError so failures become ExportFailedException with the existing error reporting. Preserve the formatting established by write_artifacts_status_yaml by rewriting with sort_keys=False and block-style output, or reuse that writer while retaining the censoring_occurred field.
🤖 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/engine/file_export/censoring_rules.py`:
- Around line 53-69: The SENSITIVE_FILE_PATTERNS substring rules for “secret”,
“credential”, and “password” currently match ancestor directory names and can
overwrite unrelated files. Update matches_sensitive_filename to apply these
filename-based rules to Path(filename).name, while preserving full-path matching
only for path-dependent .ssh patterns and existing exact-extension/key-name
rules.
In `@projects/caliper/engine/file_export/censoring.py`:
- Around line 100-110: Update the file-content sampling logic in the shown try
block to open file_path and read at most the first 1024 bytes, rather than
calling read_bytes() on the entire file. Preserve the existing empty-file
handling, printable_ratio calculation, and return threshold while ensuring
multi-GB files are never fully loaded into memory.
- Around line 187-195: Import the stat module used by the PermissionError
fallback in the file export logic, so the chmod expression in the read-only
handling block can resolve stat.S_IWUSR and retry writing successfully.
In `@projects/caliper/orchestration/censoring.py`:
- Around line 102-105: Update discover_vault_secrets so failures during expected
vault discovery are not converted into empty results: propagate the exception,
or return an explicit discovery_failed result that orchestration_apply_censoring
treats as fatal before exporting files. Preserve normal empty-secret behavior
only when vault discovery was not expected or completed successfully.
- Around line 108-112: Align the censoring function name between the definition
in orchestration_apply_censoring and the importer/call in
run_from_orchestration_config; rename the exported function or update the import
and invocation so both files consistently use apply_orchestration_censoring with
the existing arguments and return behavior.
In `@projects/caliper/orchestration/export.py`:
- Around line 166-170: Update the return annotation of
run_from_orchestration_config to match its actual parsed status-mapping return
value, using the appropriate dictionary type rather than int. Leave the
function’s runtime behavior and downstream export handling unchanged.
- Around line 271-282: Update the disable_file_export mock status written by the
export flow to match the schema produced by write_artifacts_status_yaml: wrap
export metadata under caliper_artifacts_export, include its version, and
represent the mlflow backend with the expected status/detail and run information
fields. Preserve the existing disabled-export success semantics and
censoring/duration values so _extract_artifact_links and _update_final_artifacts
can consume the mock status.
In `@projects/core/library/config.py`:
- Around line 194-214: The configuration override reporting in
projects/core/library/config.py lines 194-214 must stop interpolating raw values
into logger output and file_messages; report the key with the established safe
redaction and remove the actual_value lookup used only for formatting. Apply the
same key-or-preset-name plus safe-redaction reporting to
projects/core/library/config.py lines 240-250, ensuring both affected paths
protect secrets in logs and ARTIFACT_DIR/presets_applied.txt.
- Around line 242-250: Update the preset-processing flow around set_config() so
each message is appended to preset_messages only after its corresponding
configuration save succeeds. Move the batched presets_applied.txt write into a
finally block that always flushes accumulated successful messages, including
when a later set_config() call raises.
In `@projects/core/library/export_notifications.py`:
- Around line 436-445: Update the notification setup around get_file_link so
format_postprocess_status_notification receives None when mlflow_run_url is
unset, rather than a callback that returns None. Only create and pass the
file-link closure when mlflow_run_url is available, while preserving the
existing _create_mlflow_file_url_for_step behavior.
- Around line 250-266: Update _get_execution_engine_config to use an explicit
allowlist of approved non-sensitive cluster configuration keys, and only include
values whose keys are on that allowlist. Stop iterating and publishing every
entry from cluster_config, while preserving the existing formatting and None
behavior when no approved values are available.
- Around line 8-18: Add a typing-only import for StepStatus under TYPE_CHECKING
in export_notifications.py, ensuring the annotation references at the affected
functions resolve for Ruff without introducing a runtime dependency. Update the
typing import alongside the existing imports and preserve the function-body
imports.
- Around line 940-952: Remove the unused _process_step_status function,
including its call to _process_step_details and the invalid
test_labels_info/postprocess_info mapping access. Do not alter other
step-processing paths.
- Around line 107-114: Stop rebinding the caller-resolved artifact_dir in the
NotificationContext construction; use a separate local for env.ARTIFACT_DIR if
that value is needed, while keeping artifact_dir unchanged so Slack reports the
same directory used to build the GitHub body.
In `@projects/core/library/export.py`:
- Around line 368-377: The censoring handler in run_from_orchestration_config
discards the previously written export status and its artifact links. Preserve
or reload the existing status payload when handling CensoringOccurredException,
then merge success_with_censoring, censoring_occurred, and the exception message
without removing fields such as caliper_artifacts_export, run_url, or
experiment_url.
---
Duplicate comments:
In `@projects/caliper/cli/commands.py`:
- Around line 1097-1099: Update the file classification around censoring_results
so repaired files with censored=True and sanitized=True remain eligible for
output. Exclude only results that are censored and not sanitized, or reuse the
engine’s filtered_paths allow-list consistently in both the in-place unlink flow
and the --output copy flow.
In `@projects/caliper/engine/file_export/censoring.py`:
- Around line 144-172: Update the keyword-processing block around
COMPILED_KEYWORD_PATTERNS to collect matches and matched pattern indices in one
pass, then merge overlapping or adjacent spans before redacting. Apply each
merged span against the original content in reverse order, preserve reasons for
every matched pattern, and remove the redundant detection loop and unused
pattern_idx handling.
In `@projects/caliper/orchestration/export.py`:
- Around line 419-435: The mock results created in the disable_file_export
branch must not expose fake MLflow links. Update the result metadata in the
disable_file_export handling to omit the experiment_url, run_url, and
tracking_uri keys (or mark them disabled) while preserving the mock run
identifier and successful status.
- Around line 300-307: Guard the status_yaml read in the export flow so
multi-run dry runs do not open a file that was never created. Use the existing
run_dirs length and export_cfg.dry_run conditions to return an appropriate
status before the unconditional open, while preserving status loading and
censoring information for normal and single-run executions.
In `@projects/core/library/ci.py`:
- Line 249: Remove the logger.info call that interpolates the caller-provided
message in the notification flow, while preserving the existing log indicating
the notification was created.
- Around line 172-204: Restrict exit reasons in the command execution and
status-persistence flow to safe allowlisted codes: assign “command_failed” in
the exception handler instead of str(e), and only persist or log recognized safe
reason values. Update the logic around handle_ci_exception, exit_status_data,
and the reason-based logger calls so arbitrary caller-controlled or exception
text is never written or logged.
In `@projects/core/library/export_notifications.py`:
- Around line 320-331: Update the notification step formatting around
mlflow_log_url and step_title: build the log link with the existing
_create_mlflow_url helper so artifact URLs preserve query parameters and
fragments, and always append a step heading even when mlflow_run_url is None.
Keep the existing step body processing via _process_notification_files and
_process_step_details unchanged.
---
Nitpick comments:
In `@projects/caliper/cli/commands.py`:
- Around line 1011-1016: Update the final_insecure_tls resolution in the CLI
configuration flow so an explicitly provided --mlflow-insecure-tls value
overrides config.get("insecure_tls", False), including an explicit false value,
while retaining the config value only when the CLI option is absent. Keep the
existing final_* resolution behavior unchanged.
In `@projects/caliper/engine/file_export/censoring_rules.py`:
- Around line 88-104: Remove the unused find_sensitive_content_in_text helper
and its associated implementation, leaving keyword detection in censoring.py as
the sole mechanism.
In `@projects/caliper/engine/file_export/censoring.py`:
- Around line 175-180: Pre-normalize non-empty, stripped vault secrets once in
__init__ and retain the normalized collection for censoring. Update the loop in
the secret-scanning method to iterate over those normalized values without
repeated strip calls, while using the already-stripped keys in secret_mapping
for vault identifiers.
In `@projects/caliper/orchestration/censoring.py`:
- Around line 23-38: Remove the unused VAULT_CONTENT_TYPES constant from the
censoring module, and ensure discover_vault_secrets continues to rely solely on
content_def.is_sensible for discovery without introducing another content-name
filter.
In `@projects/caliper/orchestration/export.py`:
- Around line 372-377: Remove the earlier full-tree artifact path computation in
the export flow and retain a single recursive listing after
apply_orchestration_censoring. Ensure all_artifact_paths is populated from the
post-censoring tree so the updated in-place artifacts are used without a
redundant scan.
- Around line 471-486: Harden the status YAML update in the status_yaml export
flow around write_artifacts_status_yaml: handle empty or non-mapping safe_load
results and yaml.YAMLError alongside OSError so failures become
ExportFailedException with the existing error reporting. Preserve the formatting
established by write_artifacts_status_yaml by rewriting with sort_keys=False and
block-style output, or reuse that writer while retaining the censoring_occurred
field.
In `@projects/core/library/export.py`:
- Around line 26-30: Rename the cross-module helpers _check_job_shutdown_status
and _create_mlflow_file_url_for_step to public names in export_notifications,
then update their definitions and all imports and call sites, including
export.py, to use the new names while leaving send_notification unchanged.
- Around line 183-185: Update run_caliper_orchestration_export so artifact_dir
is not an unused required parameter: either apply it to the caliper.export.from
configuration within the function, making the export self-contained, or remove
the parameter and update its callers; preserve the existing export behavior.
🪄 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 Plus
Run ID: 967c4184-a676-4a7a-a9d2-6f0acefd97e2
📒 Files selected for processing (14)
projects/caliper/cli/commands.pyprojects/caliper/cli/main.pyprojects/caliper/engine/file_export/censoring.pyprojects/caliper/engine/file_export/censoring_rules.pyprojects/caliper/orchestration/censoring.pyprojects/caliper/orchestration/export.pyprojects/caliper/orchestration/notification.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/library/ci.pyprojects/core/library/config.pyprojects/core/library/export.pyprojects/core/library/export_notifications.pyprojects/core/library/vault.pyvaults/psap-forge-mlflow-export.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/core/ci_entrypoint/prepare_ci.py
| SENSITIVE_FILE_PATTERNS = [ | ||
| r".*\.pem$", # PEM certificate files | ||
| r".*\.key$", # Private key files | ||
| r".*\.p12$", # PKCS#12 certificate files | ||
| r".*\.pfx$", # PKCS#12 certificate files (Windows) | ||
| r".*secret.*", # Any file with "secret" in the name | ||
| r".*credential.*", # Any file with "credential" in the name | ||
| r".*password.*", # Any file with "password" in the name | ||
| r".*\.ssh/.*", # SSH directory contents | ||
| r".*/\.ssh/.*", # SSH directory contents (with path) | ||
| r".*id_rsa.*", # SSH private keys | ||
| r".*id_dsa.*", # DSA private keys | ||
| r".*id_ecdsa.*", # ECDSA private keys | ||
| r".*id_ed25519.*", # Ed25519 private keys | ||
| r".*\.env$", # Environment files | ||
| r".*\.env\..*", # Environment files with suffixes | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Path-wide substring patterns will wipe every file under a directory whose name contains secret/credential/password.
matches_sensitive_filename is called with str(file_path) (projects/caliper/engine/file_export/censoring.py Line 119), and the whole matched file is then overwritten in place with a one-line placeholder. Patterns such as .*secret.*, .*credential.* and .*password.* therefore match on any ancestor directory component — e.g. artifacts/mlflow-secret-config/run.log or a step dir named create_password_secret — and irrecoverably destroy unrelated artifacts. Consider matching these substring rules against Path(filename).name only, or keeping the full-path form solely for the .ssh/ rules.
🤖 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/engine/file_export/censoring_rules.py` around lines 53 - 69,
The SENSITIVE_FILE_PATTERNS substring rules for “secret”, “credential”, and
“password” currently match ancestor directory names and can overwrite unrelated
files. Update matches_sensitive_filename to apply these filename-based rules to
Path(filename).name, while preserving full-path matching only for path-dependent
.ssh patterns and existing exact-extension/key-name rules.
| try: | ||
| # Read first 1KB and check if it's mostly printable | ||
| sample = file_path.read_bytes()[:1024] | ||
| if not sample: | ||
| return True # Empty file is text | ||
|
|
||
| # Check if most bytes are printable ASCII or common UTF-8 | ||
| printable_ratio = sum(1 for b in sample if 32 <= b <= 126 or b in (9, 10, 13)) / len( | ||
| sample | ||
| ) | ||
| return printable_ratio > 0.7 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
read_bytes()[:1024] loads the entire file into memory.
Every extension-less artifact (including multi-GB tarballs or core dumps in a CI artifact tree) is fully read just to sample 1 KB. Open and read a bounded chunk instead.
♻️ Proposed fix
- sample = file_path.read_bytes()[:1024]
+ with file_path.open("rb") as fh:
+ sample = fh.read(1024)📝 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.
| try: | |
| # Read first 1KB and check if it's mostly printable | |
| sample = file_path.read_bytes()[:1024] | |
| if not sample: | |
| return True # Empty file is text | |
| # Check if most bytes are printable ASCII or common UTF-8 | |
| printable_ratio = sum(1 for b in sample if 32 <= b <= 126 or b in (9, 10, 13)) / len( | |
| sample | |
| ) | |
| return printable_ratio > 0.7 | |
| try: | |
| # Read first 1KB and check if it's mostly printable | |
| with file_path.open("rb") as fh: | |
| sample = fh.read(1024) | |
| if not sample: | |
| return True # Empty file is text | |
| # Check if most bytes are printable ASCII or common UTF-8 | |
| printable_ratio = sum(1 for b in sample if 32 <= b <= 126 or b in (9, 10, 13)) / len( | |
| sample | |
| ) | |
| return printable_ratio > 0.7 |
🤖 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/engine/file_export/censoring.py` around lines 100 - 110,
Update the file-content sampling logic in the shown try block to open file_path
and read at most the first 1024 bytes, rather than calling read_bytes() on the
entire file. Preserve the existing empty-file handling, printable_ratio
calculation, and return threshold while ensuring multi-GB files are never fully
loaded into memory.
| except Exception as e: | ||
| logger.warning(f"Failed to discover vault secrets: {e}") | ||
|
|
||
| return vault_secrets, secret_mapping |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Vault discovery fails open: censoring proceeds with zero secrets.
Any failure inside the block (vault manager not initialized, secret dir missing) is swallowed as a warning and discover_vault_secrets returns empty sets, so files containing real vault values are exported unredacted while the report claims everything is clean. For a censoring gate, propagate the failure (or return a discovery_failed flag that orchestration_apply_censoring treats as fatal) when vaults were expected to be 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 `@projects/caliper/orchestration/censoring.py` around lines 102 - 105, Update
discover_vault_secrets so failures during expected vault discovery are not
converted into empty results: propagate the exception, or return an explicit
discovery_failed result that orchestration_apply_censoring treats as fatal
before exporting files. Preserve normal empty-secret behavior only when vault
discovery was not expected or completed successfully.
| artifact_dir = Path(env.ARTIFACT_DIR) if env.ARTIFACT_DIR else None | ||
| context = NotificationContext( | ||
| status=status, | ||
| finish_reason=str(finish_reason), | ||
| project_name=project or "unknown", | ||
| pr_number=os.environ.get("PULL_NUMBER"), | ||
| job_type=os.environ.get("JOB_TYPE"), | ||
| artifact_dir=artifact_dir, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Parameter rebinding changes which directory Slack reports. artifact_dir (the caller-resolved root used to build the GitHub body) is overwritten with env.ARTIFACT_DIR here, so the Slack context sees a different directory. Use a separate local if that is intentional.
🔧 Suggested fix
- artifact_dir = Path(env.ARTIFACT_DIR) if env.ARTIFACT_DIR else None
+ slack_artifact_dir = artifact_dir or (
+ Path(env.ARTIFACT_DIR) if env.ARTIFACT_DIR else None
+ )
context = NotificationContext(
@@
- artifact_dir=artifact_dir,
+ artifact_dir=slack_artifact_dir,
)📝 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.
| artifact_dir = Path(env.ARTIFACT_DIR) if env.ARTIFACT_DIR else None | |
| context = NotificationContext( | |
| status=status, | |
| finish_reason=str(finish_reason), | |
| project_name=project or "unknown", | |
| pr_number=os.environ.get("PULL_NUMBER"), | |
| job_type=os.environ.get("JOB_TYPE"), | |
| artifact_dir=artifact_dir, | |
| slack_artifact_dir = artifact_dir or ( | |
| Path(env.ARTIFACT_DIR) if env.ARTIFACT_DIR else None | |
| ) | |
| context = NotificationContext( | |
| status=status, | |
| finish_reason=str(finish_reason), | |
| project_name=project or "unknown", | |
| pr_number=os.environ.get("PULL_NUMBER"), | |
| job_type=os.environ.get("JOB_TYPE"), | |
| artifact_dir=slack_artifact_dir, |
🤖 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/export_notifications.py` around lines 107 - 114, Stop
rebinding the caller-resolved artifact_dir in the NotificationContext
construction; use a separate local for env.ARTIFACT_DIR if that value is needed,
while keeping artifact_dir unchanged so Slack reports the same directory used to
build the GitHub body.
| def _get_execution_engine_config() -> str | None: | ||
| """Get execution engine configuration for notification.""" | ||
| try: | ||
| cluster_config = config.project.get_config("cluster", None, warn=False) | ||
| if not cluster_config: | ||
| return None | ||
|
|
||
| config_parts = [] | ||
| for key, value in cluster_config.items(): | ||
| if value: | ||
| config_parts.append(f"`{key}`: {value}") | ||
|
|
||
| if config_parts: | ||
| return "* " + " \n* ".join(config_parts) | ||
| except Exception: | ||
| pass | ||
| return None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Blanket dump of the whole cluster config section into a publicly-readable notification. Any credential-like key added to that section later would leak without a code change. An explicit display-key allowlist is the safer boundary.
As per coding guidelines: "Treat everything under ARTIFACT_DIR as publicly accessible; write only non-sensitive operational data".
🤖 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/export_notifications.py` around lines 250 - 266, Update
_get_execution_engine_config to use an explicit allowlist of approved
non-sensitive cluster configuration keys, and only include values whose keys are
on that allowlist. Stop iterating and publishing every entry from
cluster_config, while preserving the existing formatting and None behavior when
no approved values are available.
Source: Coding guidelines
| except CensoringOccurredException as e: | ||
| logger.info(f"Export completed with censoring: {e}") | ||
| # Create success status with censoring flag for notification | ||
| status = { | ||
| "success": True, | ||
| "final_status": "success_with_censoring", | ||
| "censoring_occurred": True, | ||
| "message": str(e), | ||
| "backends": {}, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Censoring path throws away the real export status, so notifications lose all artifact links. run_from_orchestration_config writes status.yaml (with MLflow run_url/experiment_url) before raising CensoringOccurredException, but this handler replaces it with a bare dict lacking caliper_artifacts_export. _extract_artifact_links then emits "No direct links available" for every censored run — arguably the case where links matter most. Consider attaching the status payload to the exception (or reloading status.yaml) and merging the censoring flag in.
🤖 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/export.py` around lines 368 - 377, The censoring
handler in run_from_orchestration_config discards the previously written export
status and its artifact links. Preserve or reload the existing status payload
when handling CensoringOccurredException, then merge success_with_censoring,
censoring_occurred, and the exception message without removing fields such as
caliper_artifacts_export, run_url, or experiment_url.
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
|
🔴 Submission of
|
|
@kpouget: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary by CodeRabbit
artifacts censorCLI command.