feat(automl,autorag): add archive support for pipeline runs#7375
feat(automl,autorag): add archive support for pipeline runs#7375chrjones-rh wants to merge 3 commits intoopendatahub-io:mainfrom
Conversation
Add BFF endpoint and UI support for archiving pipeline runs that are in a terminal state (SUCCEEDED, FAILED, CANCELED). Archived runs are hidden from the default list view via the existing storage_state filter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add ArchiveRunModal with type-to-confirm pattern, link to Pipelines archived runs view, and comprehensive test coverage including unit tests, contract tests, OpenAPI specs, and API documentation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[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 |
📝 WalkthroughWalkthroughThis pull request adds an archive operation for pipeline runs to both AutoML and AutoRAG: a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security and Code Quality Observations
Address these actionable items (tests, logging to secure store, input normalization, sentinel protections) before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
chrjones-rh
requested review from
MatthewAThompson,
jefho-rh and
nickmazzi
and removed request for
NickGagan and
srtanish1992
|
UI screenshots: 
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/automl/frontend/src/app/hooks/mutations.ts (1)
33-38:⚠️ Potential issue | 🟠 MajorAdd CSRF protection to the archive POST.
Severity: major, CWE-352. Exploit: with cookie/session auth, an attacker can submit a cross-site POST to the archive URL and hide a terminal run because this request carries no server-validated CSRF proof. Send a CSRF token from the host/shared request layer and reject missing/invalid tokens in the BFF.
Security remediation pattern
async function postPipelineRunAction(url: string, action: string): Promise<void> { + const csrfToken = getCsrfTokenFromHost(); + if (!csrfToken) { + throw new Error('Missing CSRF token'); + } + const response = await fetch(url, { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + credentials: 'same-origin', + headers: { + 'Content-Type': 'application/json', + 'X-CSRF-Token': csrfToken, + }, body: JSON.stringify({}), });As per coding guidelines,
CSRF token validation for state-changing operations.Also applies to: 78-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/frontend/src/app/hooks/mutations.ts` around lines 33 - 38, The POST to archive in postPipelineRunAction currently sends no CSRF proof; update postPipelineRunAction (and the similar function around lines 78-86) to include a server-provided CSRF token in the request (e.g., add 'X-CSRF-Token' header or include the token in the JSON body) by reading the token from the shared host/request layer or app state where the BFF exposes it, and ensure the BFF is configured to validate that token and reject requests missing/with invalid tokens; locate postPipelineRunAction and the other POST handler and add the header/token wiring to every state-changing fetch call.packages/autorag/frontend/src/app/hooks/mutations.ts (1)
33-38:⚠️ Potential issue | 🟠 MajorAdd CSRF protection to the archive POST.
Severity: major, CWE-352. Exploit: with cookie/session auth, an attacker can submit a cross-site POST to the archive URL and hide a terminal run because this request carries no server-validated CSRF proof. Send a CSRF token from the host/shared request layer and reject missing/invalid tokens in the BFF.
Security remediation pattern
async function postPipelineRunAction(url: string, action: string): Promise<void> { + const csrfToken = getCsrfTokenFromHost(); + if (!csrfToken) { + throw new Error('Missing CSRF token'); + } + const response = await fetch(url, { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + credentials: 'same-origin', + headers: { + 'Content-Type': 'application/json', + 'X-CSRF-Token': csrfToken, + }, body: JSON.stringify({}), });As per coding guidelines,
CSRF token validation for state-changing operations.Also applies to: 78-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/frontend/src/app/hooks/mutations.ts` around lines 33 - 38, postPipelineRunAction is issuing a state-changing POST with no CSRF proof; modify it to include the app's CSRF token from the shared/host request layer (e.g., call the shared getCsrfToken()/request helper or read the token provided by the host) and send that token in a CSRF header such as "X-CSRF-Token" (or the app's configured header) on the fetch request, and update the BFF to validate/reject missing or invalid tokens; apply the same change to the other POST helper used around lines 78-86 so all archive/state-changing POSTs carry the CSRF token.packages/autorag/docs/pipeline-runs-api.md (1)
16-30:⚠️ Potential issue | 🟡 MinorUpdate the surrounding docs to include Archive.
The new archive section adds a sixth endpoint, but the overview still lists five endpoints and later sections omit Archive from pipeline discovery and frontend integration behavior.
Documentation alignment
POST /api/v1/pipeline-runs/{runId}/terminate POST /api/v1/pipeline-runs/{runId}/retry +POST /api/v1/pipeline-runs/{runId}/archive-The API provides five endpoints:
+The API provides six endpoints:
@@
- Retry Run: Re-initiate a failed or canceled pipeline run from the point of failure
+- Archive Run: Archive a completed, failed, or canceled pipeline run
@@
-3. The List endpoint returns 200 with an empty runs list if no AutoRAG pipeline is found; other endpoints (Create, Get, Terminate, Retry) return a 500 error
+3. The List endpoint returns 200 with an empty runs list if no AutoRAG pipeline is found; other endpoints (Create, Get, Terminate, Retry, Archive) return a 500 error
@@
- Terminate runs that are currently in progress
- Retry failed or canceled runs
+10. Archive completed, failed, or canceled runs</details> Also applies to: 674-681, 842-855 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/autorag/docs/pipeline-runs-api.mdaround lines 16 - 30, The docs
still describe five endpoints and omit the new "Archive Run" endpoint; update
the overview to say "six endpoints", add the "- Archive Run: Archive a
completed, failed, or canceled pipeline run" bullet next to the existing "-
Retry Run" entry, and update any enumerated behavior lists and error
descriptions that mention endpoints to include "Archive" (e.g., the sentence
that lists which endpoints return 500 on error should add "Archive"). Also
propagate the Archive addition into the pipeline discovery and frontend
integration behavior sections (where the docs enumerate actions like
Terminate/Retry) so they include "Archive" alongside Terminate/Retry and add the
corresponding numbered behavior (Archive completed, failed, or canceled runs).</details> </blockquote></details> <details> <summary>packages/automl/docs/pipeline-runs-api.md (1)</summary><blockquote> `17-30`: _⚠️ Potential issue_ | _🟡 Minor_ **Endpoint list and count are stale after adding archive.** The endpoint block still lists only the five pre-existing endpoints and the prose says "The API provides five endpoints," but this PR adds a sixth (`POST /api/v1/pipeline-runs/{runId}/archive`). Update both the fenced block and the bullet list so the top-of-doc index matches the new Archive section added at line 672. <details> <summary>Proposed fix</summary> ```diff POST /api/v1/pipeline-runs/{runId}/terminate POST /api/v1/pipeline-runs/{runId}/retry +POST /api/v1/pipeline-runs/{runId}/archive-The API provides five endpoints:
+The API provides six endpoints:
- List Runs: Query multiple pipeline runs with optional filtering and pagination
- Get Run: Retrieve a single pipeline run by its ID with full task details
- Create Run: Submit a new AutoML pipeline run with training parameters
- Terminate Run: Stop an active pipeline run that is currently in progress
- Retry Run: Re-initiate a failed or canceled pipeline run from the point of failure
+- Archive Run: Archive a completed, failed, or canceled pipeline run</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/automl/docs/pipeline-runs-api.mdaround lines 17 - 30, Update the
top endpoint list in pipeline-runs-api.md to include the new archive endpoint
and adjust the summary count: add the line "POST
/api/v1/pipeline-runs/{runId}/archive" to the fenced HTTP block, change the
prose "The API provides five endpoints:" to "The API provides six endpoints:",
and add a bullet "- Archive Run: Archive a completed, failed, or canceled
pipeline run" to the bullet list so the header matches the new Archive section.</details> </blockquote></details> </blockquote></details>🧹 Nitpick comments (2)
packages/autorag/bff/internal/integrations/pipelineserver/client.go (1)
382-427: Nit: consider deduplicating the triplet TerminateRun/RetryRun/ArchiveRun.These three methods are now byte-for-byte identical except for the URL verb suffix (
:terminate/:retry/:archive) and the error prefix string. A single helper likepostRunAction(ctx, runID, "archive")would collapse ~45 lines × 3 into one implementation and remove the ongoing maintenance tax of keeping the three copies in lockstep (and also across the autorag/automl packages). Not blocking — ship it, but worth a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/bff/internal/integrations/pipelineserver/client.go` around lines 382 - 427, Extract the duplicated POST logic in ArchiveRun/TerminateRun/RetryRun into a new helper on RealPipelineServerClient, e.g. postRunAction(ctx context.Context, runID, verb string) error, which builds the URL with fmt.Sprintf("%s/apis/v2beta1/runs/%s:%s", c.baseURL, url.PathEscape(runID), verb), creates a POST with body "{}", sets Content-Type and Authorization using c.authToken, executes via c.httpClient.Do, reads up to maxPipelineErrorBodySize on non-200 responses and returns an &HTTPError{StatusCode, Message} with the same "failed to <verb> run <runID>: <body>( (truncated))" message formatting, drains the body on success, and returns nil; then replace ArchiveRun, TerminateRun, and RetryRun bodies to simply validate runID and call postRunAction(ctx, runID, "archive"/"terminate"/"retry") respectively, reusing HTTPError, maxPipelineErrorBodySize, c.httpClient and c.authToken.packages/autorag/bff/internal/api/pipeline_runs_handler.go (1)
203-217: UpdatemapMutationErrorcomment to include archive.Minor: the godoc on line 203-205 still says "terminate/retry" but
ArchivePipelineRunHandlernow also calls through it. Trivial doc drift, worth a one-line fix.-// mapMutationError maps the error from a pipeline run mutation (terminate/retry) +// mapMutationError maps the error from a pipeline run mutation (terminate/retry/archive) // to the appropriate HTTP response: 404 for not-found, 400 for bad-request // from the Pipeline Server, and 500 for everything else.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/bff/internal/api/pipeline_runs_handler.go` around lines 203 - 217, Update the godoc for the mapMutationError function to mention "archive" in addition to "terminate/retry" so it accurately reflects callers like ArchivePipelineRunHandler; edit the comment above func (app *App) mapMutationError to read something like "maps the error from a pipeline run mutation (terminate/retry/archive) to the appropriate HTTP response..." to keep the documentation in sync with the function's usage.🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@packages/automl/contract-tests/__tests__/testAutomlContract.test.ts`: - Around line 1248-1288: The tests currently only assert HTTP status codes for calls made with apiClient.post to '/api/v1/pipeline-runs/{runId}/archive', so they can pass even if the OpenAPI contract is missing or wrong; update each test (the calls using apiClient.post and the result/result.response/result.error checks) to also validate the response against the OpenAPI spec entry for POST /api/v1/pipeline-runs/{runId}/archive (api/openapi/automl.yaml) using the project's response validator (e.g., the apiClient contract validation helper or OpenAPI validator) and assert the contract validation succeeds for the 200, 400 and 404 scenarios respectively. Ensure the validator is invoked after receiving result.response (for success) or result.error (for failures) so the test will fail if the API response does not conform to the OpenAPI declaration. In `@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx`: - Around line 84-87: handleConfirmArchive currently always closes the modal because handleArchive swallows errors; change the flow so the modal only closes on success: update handleArchive to either rethrow the caught error or return a boolean success flag, then modify handleConfirmArchive to await handleArchive and only call setIsArchiveModalOpen(false) when handleArchive indicates success (no exception or returns true). Reference functions: handleArchive, handleConfirmArchive, and the state setter setIsArchiveModalOpen; ensure errors still notify the user when failing. In `@packages/automl/frontend/src/app/components/run-results/ArchiveRunModal.tsx`: - Line 49: The Link in ArchiveRunModal.tsx builds a path with an un-encoded namespace (the <Link ... to={`/develop-train/pipelines/runs/${namespace}/runs/archived`}>), which can allow path-injection; fix by URL-encoding the namespace before interpolation (e.g., use encodeURIComponent(namespace) or an encodedNamespace const) so the Link path uses the encoded value. - Around line 34-35: The confirm gate is bypassable when runName is undefined because confirmMessage falls back to ''—update the logic in ArchiveRunModal (variables confirmMessage, confirmInputValue, isDisabled) to treat an empty confirmMessage as always disabling the Archive action: require confirmMessage to be non-empty (e.g., check runName or confirmMessage.trim() !== '') and only enable when confirmInputValue.trim() exactly equals that non-empty confirmMessage and isArchiving is false; change the isDisabled expression accordingly so the Archive button cannot be enabled if confirmMessage/runName is empty. - Around line 33-40: The confirmInputValue state is only cleared in handleClose, so stale typed text can persist across modal reopen or when runName changes; add effects to reset it: add React.useEffect hooks watching isOpen and runName — one effect that calls setConfirmInputValue('') whenever isOpen becomes false, and another effect that calls setConfirmInputValue('') whenever runName changes (or combine them into a single useEffect([isOpen, runName]) that clears the input when !isOpen or on runName change) to ensure confirmInputValue is cleared appropriately outside of handleClose. - Line 2: ArchiveRunModal currently imports Link from react-router-dom and hardcodes a host route; instead make it router-agnostic by removing the Link usage and adding an optional prop onNavigateToArchived?: () => void to ArchiveRunModalProps, call onNavigateToArchived when the user triggers navigation (e.g., the modal button/link), and update callers to perform routing (host can use its Link or history). Also stop creating a local BrowserRouter in bootstrap.tsx (remove or defer to host) and audit AppRoutes.tsx and other components in automl/autorag to replace direct Link/router usage with navigation callbacks so the federated module relies on the host router singleton. In `@packages/autorag/api/openapi/autorag.yaml`: - Around line 784-796: The OpenAPI spec for archivePipelineRun is missing the 403 response even though the BFF handler chain (RequireAccessToService) can return 403; update the responses block for archivePipelineRun (and similarly for terminate and retry endpoints) to include a "403" response that references the common forbidden response (e.g., $ref: "#/components/responses/Forbidden" or the project's equivalent) so the OpenAPI definitions match the handler behavior exposed by RequireAccessToService. In `@packages/autorag/contract-tests/__tests__/testAutoRagContract.test.ts`: - Around line 1320-1360: Tests in the "Archive Pipeline Run" block only assert HTTP status codes and can pass even if the /api/v1/pipeline-runs/{runId}/archive operation is missing or misdeclared in api/openapi/autorag.yaml; update the tests in packages/autorag/contract-tests/__tests__/testAutoRagContract.test.ts (the "Archive Pipeline Run" describe block) to validate the responses against the OpenAPI contract by asserting the operation exists and the response bodies match the schema defined for the path /api/v1/pipeline-runs/{runId}/archive in api/openapi/autorag.yaml (add assertions for the 200 success response schema and for the 400 and 404 error schemas); use the project’s OpenAPI validation helper (e.g., apiClient.validateResponse or equivalent) to perform schema validation and fail the test if the operation or response schema does not match the spec. In `@packages/autorag/frontend/src/app/components/AutoragRunsTable/AutoragRunsTableRow.tsx`: - Around line 71-74: The archive modal is being closed unconditionally because handleConfirmArchive calls handleArchive() which swallows errors; update useAutoragRunActions.handleArchive to either rethrow errors or return an explicit success boolean (e.g., true on success, false on failure), then change handleConfirmArchive to await the returned value and only call setIsArchiveModalOpen(false) when the operation succeeded (or after catching no error when you choose to rethrow). Ensure you reference useAutoragRunActions.handleArchive and the component's handleConfirmArchive and call setIsArchiveModalOpen(false) only on success, preserving existing error logging/handling in the hook. In `@packages/autorag/frontend/src/app/components/run-results/__tests__/ArchiveRunModal.spec.tsx`: - Around line 87-96: The test titled "should call onClose and clear input when Cancel is clicked" only asserts onClose was called but not that the input was cleared; update the test in ArchiveRunModal.spec.tsx to also verify the input value was reset after Cancel by either re-rendering/opening the modal or inspecting the input's value post-click (targeting the test id 'confirm-archive-input'), ensuring handleClose's call to setConfirmInputValue('') is observed; alternatively, if you prefer not to add the assertion, rename the test to remove the "clear input" promise so the title matches what's actually asserted. In `@packages/autorag/frontend/src/app/components/run-results/ArchiveRunModal.tsx`: - Around line 33-40: The confirmInputValue is only cleared in handleClose so it can persist across reopens; add a React effect in ArchiveRunModal that resets confirmInputValue (via setConfirmInputValue('')) whenever isOpen transitions (and/or when runName changes) so the input is cleared on open or when the run identity changes; reference confirmInputValue, setConfirmInputValue, isOpen, runName and handleClose so you add a React.useEffect(() => { ... }, [isOpen, runName]) that clears the input on relevant transitions. - Around line 1-14: The component hardcodes the archived runs route string in ArchiveRunModal; add a centralized route export (e.g., export const autoragArchivedRunsPathname = '/develop-train/pipelines/runs/:namespace/runs/archived' or a variant matching existing route naming) to routes.ts and update ArchiveRunModal (and the corresponding test) to import and use that constant instead of the literal `/develop-train/pipelines/runs/${namespace}/runs/archived`; ensure the format (parameter vs template) matches how other routes like autoragExperimentsPathname are defined so both component and test reference the single exported symbol. In `@packages/autorag/frontend/src/app/hooks/useAutoragRunActions.ts`: - Around line 82-104: The success toast in handleArchive currently asserts the run "has been archived and will no longer appear," which overstates the asynchronous server behavior; update the notification.success call (in handleArchive, which uses archiveMutation, queryClient.invalidateQueries and onActionComplete) to use hedged wording consistent with retry/stop (e.g. "Archive requested — it may take a few moments for the change to appear in the runs list") so the message reflects that the archive is processed asynchronously and visibility depends on refresh/invalidation. --- Outside diff comments: In `@packages/automl/docs/pipeline-runs-api.md`: - Around line 17-30: Update the top endpoint list in pipeline-runs-api.md to include the new archive endpoint and adjust the summary count: add the line "POST /api/v1/pipeline-runs/{runId}/archive" to the fenced HTTP block, change the prose "The API provides five endpoints:" to "The API provides six endpoints:", and add a bullet "- **Archive Run**: Archive a completed, failed, or canceled pipeline run" to the bullet list so the header matches the new Archive section. In `@packages/automl/frontend/src/app/hooks/mutations.ts`: - Around line 33-38: The POST to archive in postPipelineRunAction currently sends no CSRF proof; update postPipelineRunAction (and the similar function around lines 78-86) to include a server-provided CSRF token in the request (e.g., add 'X-CSRF-Token' header or include the token in the JSON body) by reading the token from the shared host/request layer or app state where the BFF exposes it, and ensure the BFF is configured to validate that token and reject requests missing/with invalid tokens; locate postPipelineRunAction and the other POST handler and add the header/token wiring to every state-changing fetch call. In `@packages/autorag/docs/pipeline-runs-api.md`: - Around line 16-30: The docs still describe five endpoints and omit the new "Archive Run" endpoint; update the overview to say "six endpoints", add the "- **Archive Run**: Archive a completed, failed, or canceled pipeline run" bullet next to the existing "- **Retry Run**" entry, and update any enumerated behavior lists and error descriptions that mention endpoints to include "Archive" (e.g., the sentence that lists which endpoints return 500 on error should add "Archive"). Also propagate the Archive addition into the pipeline discovery and frontend integration behavior sections (where the docs enumerate actions like Terminate/Retry) so they include "Archive" alongside Terminate/Retry and add the corresponding numbered behavior (Archive completed, failed, or canceled runs). In `@packages/autorag/frontend/src/app/hooks/mutations.ts`: - Around line 33-38: postPipelineRunAction is issuing a state-changing POST with no CSRF proof; modify it to include the app's CSRF token from the shared/host request layer (e.g., call the shared getCsrfToken()/request helper or read the token provided by the host) and send that token in a CSRF header such as "X-CSRF-Token" (or the app's configured header) on the fetch request, and update the BFF to validate/reject missing or invalid tokens; apply the same change to the other POST helper used around lines 78-86 so all archive/state-changing POSTs carry the CSRF token. --- Nitpick comments: In `@packages/autorag/bff/internal/api/pipeline_runs_handler.go`: - Around line 203-217: Update the godoc for the mapMutationError function to mention "archive" in addition to "terminate/retry" so it accurately reflects callers like ArchivePipelineRunHandler; edit the comment above func (app *App) mapMutationError to read something like "maps the error from a pipeline run mutation (terminate/retry/archive) to the appropriate HTTP response..." to keep the documentation in sync with the function's usage. In `@packages/autorag/bff/internal/integrations/pipelineserver/client.go`: - Around line 382-427: Extract the duplicated POST logic in ArchiveRun/TerminateRun/RetryRun into a new helper on RealPipelineServerClient, e.g. postRunAction(ctx context.Context, runID, verb string) error, which builds the URL with fmt.Sprintf("%s/apis/v2beta1/runs/%s:%s", c.baseURL, url.PathEscape(runID), verb), creates a POST with body "{}", sets Content-Type and Authorization using c.authToken, executes via c.httpClient.Do, reads up to maxPipelineErrorBodySize on non-200 responses and returns an &HTTPError{StatusCode, Message} with the same "failed to <verb> run <runID>: <body>( (truncated))" message formatting, drains the body on success, and returns nil; then replace ArchiveRun, TerminateRun, and RetryRun bodies to simply validate runID and call postRunAction(ctx, runID, "archive"/"terminate"/"retry") respectively, reusing HTTPError, maxPipelineErrorBodySize, c.httpClient and c.authToken.🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID:
f271297d-5747-41ad-8ed1-662638b99189📒 Files selected for processing (34)
packages/automl/api/openapi/automl.yamlpackages/automl/bff/internal/api/app.gopackages/automl/bff/internal/api/pipeline_runs_handler.gopackages/automl/bff/internal/integrations/pipelineserver/client.gopackages/automl/bff/internal/integrations/pipelineserver/psmocks/client_mock.gopackages/automl/bff/internal/repositories/pipeline_runs.gopackages/automl/contract-tests/__tests__/testAutomlContract.test.tspackages/automl/docs/pipeline-runs-api.mdpackages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsxpackages/automl/frontend/src/app/components/AutomlRunsTable/__tests__/AutomlRunsTableRow.spec.tsxpackages/automl/frontend/src/app/components/run-results/ArchiveRunModal.tsxpackages/automl/frontend/src/app/components/run-results/__tests__/ArchiveRunModal.spec.tsxpackages/automl/frontend/src/app/hooks/mutations.tspackages/automl/frontend/src/app/hooks/useAutomlRunActions.tspackages/automl/frontend/src/app/pages/__tests__/AutomlResultsPage.spec.tsxpackages/automl/frontend/src/app/utilities/__tests__/utils.spec.tspackages/automl/frontend/src/app/utilities/utils.tspackages/autorag/api/openapi/autorag.yamlpackages/autorag/bff/internal/api/app.gopackages/autorag/bff/internal/api/pipeline_runs_handler.gopackages/autorag/bff/internal/integrations/pipelineserver/client.gopackages/autorag/bff/internal/integrations/pipelineserver/psmocks/client_mock.gopackages/autorag/bff/internal/repositories/pipeline_runs.gopackages/autorag/contract-tests/__tests__/testAutoRagContract.test.tspackages/autorag/docs/pipeline-runs-api.mdpackages/autorag/frontend/src/app/components/AutoragRunsTable/AutoragRunsTableRow.tsxpackages/autorag/frontend/src/app/components/AutoragRunsTable/__tests__/AutoragRunsTableRow.spec.tsxpackages/autorag/frontend/src/app/components/run-results/ArchiveRunModal.tsxpackages/autorag/frontend/src/app/components/run-results/__tests__/ArchiveRunModal.spec.tsxpackages/autorag/frontend/src/app/hooks/mutations.tspackages/autorag/frontend/src/app/hooks/useAutoragRunActions.tspackages/autorag/frontend/src/app/pages/__tests__/AutoragResultsPage.spec.tsxpackages/autorag/frontend/src/app/utilities/__tests__/utils.spec.tspackages/autorag/frontend/src/app/utilities/utils.ts
- Contract tests validate error responses against OpenAPI schema - handleArchive rethrows so modal stays open on failure - Guard against empty runName in confirmation gate - Add 403 Forbidden to archive endpoint OpenAPI specs - Centralize archived runs route in routes.ts - Align archive toast wording with retry/stop pattern - Fix test title accuracy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx (1)
111-120: Dead branch in separator condition.
runTerminatable(RUNNING/PENDING/PAUSED) andrunArchivable(SUCCEEDED/FAILED/CANCELED) are mutually exclusive perisRunTerminatable/isRunArchivable, so therunTerminatable ||portion of the guard can never contribute when entering this block. OnlyrunRetryablemeaningfully gates the separator. Consider simplifying toif (runRetryable)to avoid implying a state combination that cannot occur.♻️ Proposed simplification
if (runArchivable) { - if (runTerminatable || runRetryable) { + if (runRetryable) { items.push({ isSeparator: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx` around lines 111 - 120, The separator condition is using runTerminatable || runRetryable even though runTerminatable and runArchivable are mutually exclusive per isRunTerminatable/isRunArchivable, so simplify the guard: inside the runArchivable branch (where runArchivable is true) replace the nested if (runTerminatable || runRetryable) with if (runRetryable) so the separator only depends on runRetryable; update the block that pushes the separator (referencing runArchivable, runRetryable) accordingly and remove the dead runTerminatable check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsx`:
- Around line 111-120: The separator condition is using runTerminatable ||
runRetryable even though runTerminatable and runArchivable are mutually
exclusive per isRunTerminatable/isRunArchivable, so simplify the guard: inside
the runArchivable branch (where runArchivable is true) replace the nested if
(runTerminatable || runRetryable) with if (runRetryable) so the separator only
depends on runRetryable; update the block that pushes the separator (referencing
runArchivable, runRetryable) accordingly and remove the dead runTerminatable
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1d016b72-a060-4ead-81b1-215c9fc41d68
📒 Files selected for processing (14)
packages/automl/api/openapi/automl.yamlpackages/automl/contract-tests/__tests__/testAutomlContract.test.tspackages/automl/frontend/src/app/components/AutomlRunsTable/AutomlRunsTableRow.tsxpackages/automl/frontend/src/app/components/run-results/ArchiveRunModal.tsxpackages/automl/frontend/src/app/components/run-results/__tests__/ArchiveRunModal.spec.tsxpackages/automl/frontend/src/app/hooks/useAutomlRunActions.tspackages/automl/frontend/src/app/utilities/routes.tspackages/autorag/api/openapi/autorag.yamlpackages/autorag/contract-tests/__tests__/testAutoRagContract.test.tspackages/autorag/frontend/src/app/components/AutoragRunsTable/AutoragRunsTableRow.tsxpackages/autorag/frontend/src/app/components/run-results/ArchiveRunModal.tsxpackages/autorag/frontend/src/app/components/run-results/__tests__/ArchiveRunModal.spec.tsxpackages/autorag/frontend/src/app/hooks/useAutoragRunActions.tspackages/autorag/frontend/src/app/utilities/routes.ts
✅ Files skipped from review due to trivial changes (2)
- packages/autorag/frontend/src/app/utilities/routes.ts
- packages/automl/frontend/src/app/utilities/routes.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/autorag/contract-tests/tests/testAutoRagContract.test.ts
- packages/automl/api/openapi/automl.yaml
- packages/automl/frontend/src/app/components/run-results/ArchiveRunModal.tsx
- packages/autorag/frontend/src/app/hooks/useAutoragRunActions.ts
|
@nickmazzi — While implementing the archive flow, we identified two pre-existing issues in the stop/retry handlers (from #7321) that we fixed for archive but did not change for stop/retry to keep this PR scoped:
Neither is urgent — just flagging for awareness so they can be addressed in a follow-up. |
Hey @chrjones-rh ! Just wanted to give you a quick heads up — I think you might have meant to tag @nickmazzi instead of me 😊 I'm not part of this project, so just making sure the right person sees your comment! Cheers! |
Thank you for the correction, and apologies for the mis-tagging. |
|
Converting to draft until we have a firm decision between direct deletion or archiving on the AutoX side. |
2 participants
https://issues.redhat.com/browse/RHOAIENG-50380
https://issues.redhat.com/browse/RHOAIENG-48791
Description
Add support for archiving completed pipeline runs in AutoML and AutoRAG.
BFF changes:
POST /api/v1/pipeline-runs/:runId/archiveendpoint in both packages:archiveAPI to changestorage_statefromAVAILABLEtoARCHIVEDUI changes:
ArchiveRunModalwith type-to-confirm pattern (user must type run name to confirm)Tests & docs:
ArchiveRunModalunit tests (11 tests per package)isRunArchivableutility testsAutomlRunsTableRowandAutoragRunsTableRowtestspipeline-runs-api.mddocs updatedHow Has This Been Tested?
go test ./...in both packages)Test Impact
Unit tests, contract tests, and OpenAPI specs added for the new archive endpoint and ArchiveRunModal component. Existing tests updated to account for the new kebab action on terminal-state runs.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit