refactor(webui): Rename S3Manager to StreamFilesS3Manager; namespace stream-output AWS env vars.#2195
Open
junhaoliao wants to merge 5 commits intomainfrom
Open
refactor(webui): Rename S3Manager to StreamFilesS3Manager; namespace stream-output AWS env vars.#2195junhaoliao wants to merge 5 commits intomainfrom
junhaoliao wants to merge 5 commits intomainfrom
Conversation
…stream-output AWS env vars.
Contributor
WalkthroughAdds CLP-namespaced S3 credential environment variables, updates deployment and compose templates to use them, modifies S3Manager to accept optional AWS credentials and renames the Fastify decoration to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as StreamFilesRoute
participant Fastify
participant S3Mgr as StreamFilesS3Manager
participant S3
Client->>Route: Request stream metadata
Route->>Fastify: access config & check decorator
Fastify->>S3Mgr: call getPreSignedUrl(path)
S3Mgr->>Fastify: read CLP_STREAM_OUTPUT_AWS_* from config (optional)
S3Mgr->>S3: generate presigned URL (uses credentials if provided)
S3-->>S3Mgr: presigned URL
S3Mgr-->>Route: presigned URL
Route-->>Client: return metadata with URL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/webui/server/src/plugins/app/S3Manager/index.ts`:
- Around line 85-92: The S3 client config silently omits credentials when only
one of accessKeyId/secretAccessKey is provided, causing unintended SDK
credential fallback; inside the S3Manager initialization (where s3ClientConfig
and variables accessKeyId, secretAccessKey, profile are used) add a validation
that if exactly one of accessKeyId or secretAccessKey is set you immediately
throw or return an error (fail fast) with a clear message about the partial
credential configuration, otherwise continue to set credentials when both are
present and keep the existing profile/region handling; reference s3ClientConfig,
accessKeyId, secretAccessKey, and profile when adding the guard so the check is
colocated with the current config assembly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f7ae897-3b39-4319-8e8b-6c8cc64413c5
📒 Files selected for processing (6)
components/webui/server/.envcomponents/webui/server/src/plugins/app/S3Manager/index.tscomponents/webui/server/src/plugins/external/env.tscomponents/webui/server/src/routes/api/stream-files/index.tstools/deployment/package-helm/templates/webui-deployment.yamltools/deployment/package/docker-compose-all.yaml
Comment on lines
+85
to
+92
| const s3ClientConfig: S3ClientConfig = { | ||
| ...((accessKeyId && secretAccessKey) ? | ||
| {credentials: {accessKeyId, secretAccessKey}} : | ||
| {}), | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| ...((null !== profile) && {profile}), | ||
| region, | ||
| }; |
Contributor
There was a problem hiding this comment.
Fail fast on partial credential config to avoid unintended identity fallback.
At Line 86, when only one of the two CLP_STREAM_OUTPUT_* values is set, credentials are silently omitted and SDK default credential resolution is used. That can generate pre-signed URLs with the wrong IAM identity and mask misconfiguration.
Proposed fix
const {
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID: accessKeyId,
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY: secretAccessKey,
} = fastify.config;
+ const hasAccessKeyId = "" !== accessKeyId;
+ const hasSecretAccessKey = "" !== secretAccessKey;
+ if (hasAccessKeyId !== hasSecretAccessKey) {
+ throw new Error(
+ "Both CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and " +
+ "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY must be set together."
+ );
+ }
+
const s3ClientConfig: S3ClientConfig = {
- ...((accessKeyId && secretAccessKey) ?
+ ...((hasAccessKeyId && hasSecretAccessKey) ?
{credentials: {accessKeyId, secretAccessKey}} :
{}),
// eslint-disable-next-line `@typescript-eslint/no-unnecessary-condition`
...((null !== profile) && {profile}),
region,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/webui/server/src/plugins/app/S3Manager/index.ts` around lines 85 -
92, The S3 client config silently omits credentials when only one of
accessKeyId/secretAccessKey is provided, causing unintended SDK credential
fallback; inside the S3Manager initialization (where s3ClientConfig and
variables accessKeyId, secretAccessKey, profile are used) add a validation that
if exactly one of accessKeyId or secretAccessKey is set you immediately throw or
return an error (fail fast) with a clear message about the partial credential
configuration, otherwise continue to set credentials when both are present and
keep the existing profile/region handling; reference s3ClientConfig,
accessKeyId, secretAccessKey, and profile when adding the guard so the check is
colocated with the current config assembly.
junhaoliao
commented
Apr 10, 2026
- Revert S3Manager class to original constructor (region, profile) with eager S3Client init; add optional credentials parameter. - Revert class docstring (not stream-files-specific). - controller.py already uses CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID (no changes needed).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/webui/server/src/plugins/app/S3Manager/index.ts (1)
84-97:⚠️ Potential issue | 🟠 MajorFail fast on partial stream-output credential config.
If only one of
CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID/CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEYis set, this silently falls back to the SDK default credential chain and can sign URLs with the wrong identity. Reject the partial config before deciding whichS3Managerconstructor path to take.Proposed fix
const { CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID: accessKeyId, CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY: secretAccessKey, } = fastify.config; + + const hasAccessKeyId = "" !== accessKeyId; + const hasSecretAccessKey = "" !== secretAccessKey; + if (hasAccessKeyId !== hasSecretAccessKey) { + throw new Error( + "Both CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and " + + "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY must be set together." + ); + } fastify.log.info( {region, profile}, "Initializing StreamFilesS3Manager" ); fastify.decorate( "StreamFilesS3Manager", - (accessKeyId && secretAccessKey) ? + (hasAccessKeyId && hasSecretAccessKey) ? new S3Manager(region, profile, {accessKeyId, secretAccessKey}) : new S3Manager(region, profile) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/webui/server/src/plugins/app/S3Manager/index.ts` around lines 84 - 97, Detect and reject partial stream-output AWS credentials before constructing the S3Manager: check CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY (fastify.config) and if exactly one is set, throw or fail fast with a clear error (do not fall back to SDK defaults); only call new S3Manager(region, profile, {accessKeyId, secretAccessKey}) when both are present, otherwise call new S3Manager(region, profile) when neither is set. Update the initialization of the decorated "StreamFilesS3Manager" to perform this validation and error out on partial config to avoid accidental credential mixing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/webui/server/src/plugins/app/S3Manager/index.ts`:
- Around line 84-97: Detect and reject partial stream-output AWS credentials
before constructing the S3Manager: check CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and
CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY (fastify.config) and if exactly one is
set, throw or fail fast with a clear error (do not fall back to SDK defaults);
only call new S3Manager(region, profile, {accessKeyId, secretAccessKey}) when
both are present, otherwise call new S3Manager(region, profile) when neither is
set. Update the initialization of the decorated "StreamFilesS3Manager" to
perform this validation and error out on partial config to avoid accidental
credential mixing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8af4f02e-f66f-4d9c-9473-6e73d37431f1
📒 Files selected for processing (1)
components/webui/server/src/plugins/app/S3Manager/index.ts
junhaoliao
commented
Apr 10, 2026
junhaoliao
commented
Apr 10, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/webui/server/src/plugins/app/S3Manager/index.ts (1)
90-92:⚠️ Potential issue | 🟠 MajorFail fast on partial stream-output credentials (still unresolved).
Line 90 still treats partial config as “no credentials”, which can silently fall back to SDK default identity and generate URLs with the wrong IAM principal.
Proposed fix
- const credentials = (accessKeyId && secretAccessKey) ? - {accessKeyId, secretAccessKey} : - null; + const hasAccessKeyId = "" !== accessKeyId; + const hasSecretAccessKey = "" !== secretAccessKey; + if (hasAccessKeyId !== hasSecretAccessKey) { + throw new Error( + "Both CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID and " + + "CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEY must be set together." + ); + } + const credentials = (hasAccessKeyId && hasSecretAccessKey) ? + {accessKeyId, secretAccessKey} : + null;#!/bin/bash set -euo pipefail file="components/webui/server/src/plugins/app/S3Manager/index.ts" # Show the credential assembly block under review. nl -ba "$file" | sed -n '80,98p' # Verify whether a paired-credentials guard already exists. rg -n 'must be set together|hasAccessKeyId|hasSecretAccessKey' "$file" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/webui/server/src/plugins/app/S3Manager/index.ts` around lines 90 - 92, The current credential assembly sets credentials to null when only one of accessKeyId or secretAccessKey is provided, which allows silent fallback to SDK defaults; update the logic around the credentials variable so it validates paired credentials: if both accessKeyId and secretAccessKey are present use {accessKeyId, secretAccessKey}, if neither are present keep null, but if exactly one is present throw an explicit error (or return a rejected Promise) with a clear message indicating both accessKeyId and secretAccessKey must be set together; locate the const credentials assignment and add the guard using accessKeyId and secretAccessKey to enforce the fail-fast behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/webui/server/src/plugins/app/S3Manager/index.ts`:
- Around line 90-92: The current credential assembly sets credentials to null
when only one of accessKeyId or secretAccessKey is provided, which allows silent
fallback to SDK defaults; update the logic around the credentials variable so it
validates paired credentials: if both accessKeyId and secretAccessKey are
present use {accessKeyId, secretAccessKey}, if neither are present keep null,
but if exactly one is present throw an explicit error (or return a rejected
Promise) with a clear message indicating both accessKeyId and secretAccessKey
must be set together; locate the const credentials assignment and add the guard
using accessKeyId and secretAccessKey to enforce the fail-fast behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d620cb9-45d8-48b1-bee8-af1914bd8e6d
📒 Files selected for processing (2)
components/webui/server/src/plugins/app/S3Manager/index.tstools/deployment/package-helm/Chart.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extracted from the WIP prototype in #2169.
Renames the existing
S3ManagerFastify decorator toStreamFilesS3Managerto clarify its role(serving pre-signed URLs for stream files) and prepare for a second
LogsInputS3Managerdecoratorin a follow-up PR. Also namespaces the AWS credential env vars from the generic
AWS_ACCESS_KEY_ID/
AWS_SECRET_ACCESS_KEYtoCLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID/CLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEYso they don't collide with futureCLP_LOGS_INPUT_AWS_*vars.
Key changes:
credentials: Nullable<AwsCredentialIdentity>parameter to theconstructor so explicit credentials can be passed (required because the AWS SDK default credential
chain no longer finds credentials under the renamed env var names). Settings values are now cast
to
Nullable<string>, removing the need for eslint-disable comments.FastifyInstance.S3Manager→FastifyInstance.StreamFilesS3Manager(interface + decorator + all references in
stream-filesroute).CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_IDandCLP_STREAM_OUTPUT_AWS_SECRET_ACCESS_KEYwith empty defaults.AWS_ACCESS_KEY_ID→CLP_STREAM_OUTPUT_AWS_ACCESS_KEY_ID(same for secret key) inside the webui service.
conditional block.
0.2.1-dev.7.Checklist
breaking change.
Validation performed
Scenario 1: TypeScript build
Task: Verify the renamed decorator and updated imports compile without errors.
Command:
Output:
Scenario 2: ESLint
Task: Verify no new lint violations are introduced.
Command:
Output:
Scenario 3: Verify decorator rename is complete
Task: Confirm no stale references to the old decorator name remain.
Command:
grep -rn '"S3Manager"' components/webui/server/src/Output:
Explanation: All references to the old
S3Managerdecorator have been updated toStreamFilesS3Manager.Summary by CodeRabbit
Release Notes
New Features
Chores