Feat/phase2 tests ci#2
Conversation
- Modular monolith architecture with clean domain boundaries - Go 1.23 with Gin, pgx/v5, redis/v9, RabbitMQ, JWT auth - PostgreSQL schema: users, jobs, media_items with proper indexes - Worker pool pattern with bounded channels, context cancellation - Redis for progress tracking, rate limiting, SSE pub/sub - RabbitMQ for async job processing - SSE endpoint for real-time progress updates - OpenAPI/Swagger specification - Docker Compose for local development - Database migrations with golang-migrate
|
Warning Review limit reached
Next review available in: 9 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 Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a Go-based Streamforge API and worker system with PostgreSQL persistence, Redis progress tracking, RabbitMQ messaging, JWT authentication, job and media-item endpoints, Docker deployment, CI automation, database migrations, and operational documentation. ChangesStreamforge platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant JobsService
participant PostgreSQL
participant RabbitMQ
participant WorkerPool
participant Redis
Client->>API: Create job request
API->>JobsService: CreateJob(user_id, source_url)
JobsService->>PostgreSQL: Insert job
JobsService->>Redis: Initialize progress
API->>RabbitMQ: Publish PROCESS message
RabbitMQ->>WorkerPool: Consume job message
WorkerPool->>JobsService: Update status and media progress
JobsService->>PostgreSQL: Persist job and item updates
JobsService->>Redis: Increment progress
JobsService-->>API: Publish progress event
API-->>Client: Stream SSE event
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (26)
migrations/000001_init.up.sql-1-73 (1)
1-73: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winResolve conflicting and duplicate migration files. The
000001_init.up.sqlmigration contains the consolidated schema for all tables, but the standalone migration files for users, jobs, and media items were not removed. Keeping both will cause migration tools to fail due to conflicting version prefixes (e.g., two000001_files) and duplicate table definitions.
migrations/000001_init.up.sql#L1-L73: Keep this consolidated initialization migration. Ensure its schema matches the struct models (e.g.,size_bytesvssize, and ENUM values) before finalizing.migrations/000001_create_users_table.up.sql#L1-L15: Delete this redundant file.migrations/000002_create_jobs_table.up.sql#L1-L22: Delete this redundant file.migrations/000003_create_media_items_table.up.sql#L1-L22: Delete this redundant 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 `@migrations/000001_init.up.sql` around lines 1 - 73, Keep the consolidated schema in migrations/000001_init.up.sql (lines 1-73), first aligning its columns and ENUM values with the struct models, including size_bytes versus size. Delete the redundant migrations/000001_create_users_table.up.sql (lines 1-15), migrations/000002_create_jobs_table.up.sql (lines 1-22), and migrations/000003_create_media_items_table.up.sql (lines 1-22) so only the consolidated initialization migration defines these tables.internal/database/postgres.go-20-22 (1)
20-22: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent connection failures by URL-encoding credentials.
Similar to the configuration layer, constructing the connection URL via simple string concatenation will cause connection failures if the database username or password contains special characters. Use the
net/urlpackage to safely build the DSN.🐛 Proposed fix
First, import
net/urlat the top of the file, then update theDSNmethod:func (c Config) DSN() string { - return "postgres://" + c.User + ":" + c.Password + "@" + c.Host + ":" + c.Port + "/" + c.Name + u := url.URL{ + Scheme: "postgres", + User: url.UserPassword(c.User, c.Password), + Host: c.Host + ":" + c.Port, + Path: "/" + c.Name, + } + return u.String() }🤖 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 `@internal/database/postgres.go` around lines 20 - 22, Update Config.DSN to construct the PostgreSQL connection string using net/url so User and Password are URL-encoded before insertion, while preserving the existing host, port, and database name values.internal/config/config.go-61-63 (1)
61-63: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent connection failures by URL-encoding credentials.
Constructing the RabbitMQ URI via simple string concatenation will cause connection failures if the username or password contains special characters (like
@or/). Use thenet/urlpackage to safely construct the URI.🐛 Proposed fix
Ensure
net/urlis imported, then update theURImethod:func (r RabbitMQConfig) URI() string { - return "amqp://" + r.User + ":" + r.Password + "@" + r.Host + ":" + r.Port + "/" + u := url.URL{ + Scheme: "amqp", + User: url.UserPassword(r.User, r.Password), + Host: r.Host + ":" + r.Port, + Path: "/", + } + return u.String() }🤖 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 `@internal/config/config.go` around lines 61 - 63, Update RabbitMQConfig.URI to construct the AMQP URI with net/url rather than concatenating credential strings. URL-encode User and Password while preserving the existing Host, Port, and path components so credentials containing reserved characters produce a valid connection URI.internal/config/config.go-38-40 (1)
38-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent connection failures by URL-encoding credentials.
Constructing the connection URL via simple string concatenation will cause connection failures if the database username or password contains special characters like
@,:, or/. Use thenet/urlpackage to safely build the DSN.🐛 Proposed fix
First, import the
net/urlpackage at the top of the file:import ( + "net/url" "os"Then update the
DSNmethod:func (d DatabaseConfig) DSN() string { - return "postgres://" + d.User + ":" + d.Password + "@" + d.Host + ":" + d.Port + "/" + d.Name + u := url.URL{ + Scheme: "postgres", + User: url.UserPassword(d.User, d.Password), + Host: d.Host + ":" + d.Port, + Path: "/" + d.Name, + } + return u.String() }🤖 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 `@internal/config/config.go` around lines 38 - 40, Update DatabaseConfig.DSN to construct the PostgreSQL connection string with net/url, ensuring User and Password are URL-encoded so special characters remain valid credentials. Preserve the existing host, port, and database name components.internal/api/middleware/middleware.go-76-85 (1)
76-85: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRecovery middleware exposes panic details in both packages. Return a generic response and log the recovered value server-side.
internal/api/middleware/middleware.go#L76-L85: remove the client-facing panic message and return after writing the response.internal/middleware/middleware.go#L75-L84: apply the same generic recovery behavior.🤖 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 `@internal/api/middleware/middleware.go` around lines 76 - 85, The Recovery function in internal/api/middleware/middleware.go must stop exposing recovered panic details: log the recovered value server-side, return only the generic internal-server-error response, and return immediately after writing it. Apply the same behavior in internal/middleware/middleware.go, updating its corresponding recovery middleware as well.internal/jobs/service.go-230-250 (1)
230-250: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCreate media items and update the total atomically.
SetJobTotalItemserrors are ignored and run before the item inserts. Any later insert failure leaves a partial item set with a total describing the full input. Move these writes into one repository transaction and propagate every error.🤖 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 `@internal/jobs/service.go` around lines 230 - 250, Update CreateMediaItems to execute SetJobTotalItems and all CreateMediaItem calls within one repository transaction, propagating errors from each operation. Move the total update into the transaction alongside the inserts so any failure rolls back the entire operation and avoids partial item sets.internal/jobs/service.go-159-167 (1)
159-167: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not report successful cancellation after dependent updates fail.
Redis status deletion and media-item cancellation errors are discarded. The job can be
CANCELLEDwhile its items remain active and the cache reports another state. Check each result and define transactional/compensating behavior.🤖 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 `@internal/jobs/service.go` around lines 159 - 167, Update the cancellation flow around UpdateJobStatus to check and propagate errors from redis.SetJobStatus, redis.DeleteProgress, and repo.UpdateMediaItemsStatus instead of discarding them. Define and apply the appropriate transactional or compensating behavior so the method does not return success when any dependent update fails and cannot leave inconsistent job, item, or cache state.internal/jobs/service.go-267-310 (1)
267-310: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThe in-process SSE channel lifecycle can race and panic. Channel registration, removal, closure, and response writes lack a single owner.
internal/jobs/service.go#L267-L310: snapshot subscribers under lock and formalize channel ownership.internal/api/routes.go#L274-L296: do not close the registered channel or write to Gin's response from a second goroutine.🤖 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 `@internal/jobs/service.go` around lines 267 - 310, The SSE subscriber lifecycle has competing owners, allowing channel sends, removal, closure, and response writes to race or panic. In internal/jobs/service.go lines 267-310, update Subscribe and PublishEvent so subscriber registration, removal, snapshotting, and channel ownership follow one synchronized lifecycle without sending to a closed channel; in internal/api/routes.go lines 274-296, stop closing the registered channel and stop writing to Gin’s response from a separate goroutine, leaving response writes to the request-handling goroutine that owns them.internal/api/routes.go-127-138 (1)
127-138: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid persisting an orphan job when queue submission fails.
CreateJobcommits the job beforeSubmit. If submission fails, the API returns 500 while the job remainsCREATED; client retries can create duplicates that will never run. Use a transactional outbox, or compensate by marking the persisted job failed/cancelled before returning.🤖 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 `@internal/api/routes.go` around lines 127 - 138, Update the job-creation flow around CreateJob and pool.Submit so a failed queue submission cannot leave the persisted job in CREATED state. Use the existing transactional outbox mechanism if available; otherwise, compensate by marking resp.ID failed or cancelled before returning the 500 response, while preserving the current success path.internal/jobs/service.go-253-262 (1)
253-262: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftJob events are not routed across the API/worker process boundary. Item events lose their job ID, Redis publication is skipped without local subscribers, and the SSE route never consumes Redis.
internal/jobs/service.go#L253-L262: provide the media item's actual job ID.internal/jobs/service.go#L287-L315: publish to Redis regardless of local subscriber count.internal/api/routes.go#L245-L289: subscribe the SSE connection to the job's Redis event channel.🤖 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 `@internal/jobs/service.go` around lines 253 - 262, Route item events across processes by resolving and passing the media item’s actual job ID in UpdateMediaItemStatus at internal/jobs/service.go:253-262; update PublishEvent at internal/jobs/service.go:287-315 to publish to Redis regardless of local subscriber count; update the SSE handler at internal/api/routes.go:245-289 to subscribe each connection to the job’s Redis event channel.docs/openapi.yaml-196-201 (1)
196-201: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMatch the cancellation response to the implemented payload.
The route returns
{"message":"job cancelled"}, notJobResponse. Document that object or change the handler to return the updated job.Possible contract fix
'200': description: Job cancelled content: application/json: schema: - $ref: '`#/components/schemas/JobResponse`' + type: object + required: [message] + properties: + message: + type: string🤖 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 `@docs/openapi.yaml` around lines 196 - 201, Update the OpenAPI 200 response schema for the job-cancellation route to match the implemented payload: document an object containing the cancellation message instead of referencing JobResponse. Keep the existing cancellation response description and content type unchanged.internal/redis/redis.go-130-141 (1)
130-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSet the rate-limit expiry atomically.
INCRfollowed byEXPIREcan leave a permanent counter if the second call fails or is skipped; use a Lua script or a Redis transaction so the TTL is always applied with the increment.🤖 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 `@internal/redis/redis.go` around lines 130 - 141, Update Client.RateLimitCheck to perform the counter increment and expiry assignment atomically, using a Lua script or Redis transaction so every successful increment receives the requested window TTL even if the counter is newly created or the expiry operation would otherwise fail; preserve the existing limit comparison and error propagation.internal/redis/redis.go-70-109 (1)
70-109: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the progress update atomic.
GEThappens outside the transaction and nothing is watched, so concurrent callers can overwrite each other and lose increments. UseWATCHwith retries or move the read-modify-write into a Lua script.🤖 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 `@internal/redis/redis.go` around lines 70 - 109, Update Client.IncrementProgress so the read-modify-write operation is atomic: watch the progress key and perform the GET, increment, and SET within a retryable transaction, or replace the flow with an equivalent Lua script. Preserve the existing missing-key, serialization, expiration, and TxFailedErr handling while ensuring concurrent callers cannot lose increments.internal/queue/queue.go-156-164 (1)
156-164: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep deliveries unacked until worker processing succeeds.
Line 158 ACKs after only buffering the task, so a worker crash loses the persistent RabbitMQ message. The default branch also immediately requeues when the buffer is full, potentially creating a redelivery loop. Pass a delivery/envelope to the worker and ACK/NACK upon processing completion; block for backpressure instead of using
default.🤖 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 `@internal/queue/queue.go` around lines 156 - 164, Update the queue dispatch flow around the delivery select and worker processing so workers receive the full delivery/envelope rather than only the message, and acknowledge it only after processing succeeds; NACK with requeue on processing failure or context cancellation. Remove the default branch and block on sending to the worker channel to provide backpressure without premature requeue loops.internal/queue/queue.go-140-169 (1)
140-169: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExpose the consumer lifetime to its caller.
Consumereturnsnilimmediately and hides delivery-channel closure inside a detached goroutine. The worker process therefore cannot detect or recover from a broker/channel failure. Run this loop synchronously—the caller already invokesConsumein a goroutine—and return whenmsgscloses.🤖 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 `@internal/queue/queue.go` around lines 140 - 169, The Consume method currently detaches its delivery loop and returns immediately, preventing callers from observing broker or channel termination. Remove the inner goroutine around the select loop in Consume, run the loop synchronously, and return when msgs closes or ctx.Done() fires; preserve the existing acknowledgment and rejection behavior..github/workflows/ci.yml-18-19 (1)
18-19: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable checkout credential persistence in every job.
Subsequent test, build, and third-party action steps do not require the checkout token to remain in Git configuration.
Proposed configuration
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: falseAlso applies to: 75-76, 113-114, 145-146
🤖 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 @.github/workflows/ci.yml around lines 18 - 19, Update every actions/checkout@v4 step in the workflow, including the jobs at the referenced locations, to disable persisted checkout credentials by configuring persist-credentials as false. Apply the same setting consistently to all checkout actions without changing unrelated job steps.Source: Linters/SAST tools
cmd/worker/main.go-53-59 (1)
53-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not continue running after the queue consumer fails.
If
Consumecannot start, the error is only logged and Line 59 still reports a started worker that will never process jobs. Supervise the consumer error alongside the signal channel, then cancel and enter shutdown or retry with backoff.🤖 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 `@cmd/worker/main.go` around lines 53 - 59, Update the queue consumer supervision around Consume and the worker startup flow so a Consume failure is propagated instead of only logged; coordinate the consumer error with the signal channel, cancel the worker context, and enter the existing shutdown or retry-with-backoff path. Do not report “Worker started” after startup has failed, and preserve normal signal-driven shutdown behavior.internal/worker/pool.go-148-151 (1)
148-151: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not close the externally writable task channel.
JobChanexposestaskQueueto the queue consumer, so closing it can race with a send and panic. It also makes the secondStopcall fromcmd/worker/main.godeterministically panic. Cancellation already stops the workers; removingclosemakes shutdown idempotent.Proposed fix
func (p *Pool) Stop() { p.cancel() - close(p.taskQueue) p.wg.Wait() }🤖 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 `@internal/worker/pool.go` around lines 148 - 151, Update Pool.Stop to remove the close(p.taskQueue) call; JobChan exposes taskQueue for external writes, so cancellation via p.cancel followed by p.wg.Wait should be the only shutdown behavior and must remain safe for repeated Stop calls.internal/worker/pool.go-87-95 (1)
87-95: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail the job when media-item initialization fails.
Errors from
SetJobTotalItems,CreateMediaItems, and the secondGetMediaItemscall are discarded. If creation fails, the loop is skipped and Lines 117-119 still mark the empty job completed.Proposed handling
- p.jobService.SetJobTotalItems(ctx, jobID, 10) + if err := p.jobService.SetJobTotalItems(ctx, jobID, 10); err != nil { + p.jobService.UpdateJobStatus(ctx, jobID, "FAILED", err.Error()) + return + } ... - p.jobService.CreateMediaItems(ctx, jobID, []jobs.MediaItemInput{ + if err := p.jobService.CreateMediaItems(ctx, jobID, []jobs.MediaItemInput{ {Title: fmt.Sprintf("Media Item %d", i), SourceURL: fmt.Sprintf("https://example.com/media/%d.mp4", i)}, - }) + }); err != nil { + p.jobService.UpdateJobStatus(ctx, jobID, "FAILED", err.Error()) + return + } } - items, _, _ = p.jobService.GetMediaItems(ctx, "", jobID, "", 100, 0) + items, _, err = p.jobService.GetMediaItems(ctx, "", jobID, "", 100, 0) + if err != nil { + p.jobService.UpdateJobStatus(ctx, jobID, "FAILED", err.Error()) + return + }🤖 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 `@internal/worker/pool.go` around lines 87 - 95, Handle errors from SetJobTotalItems, each CreateMediaItems call, and the second GetMediaItems invocation within the len(items) == 0 initialization path. On any failure, fail or return the job through the existing worker error-handling flow so an empty or partially initialized job cannot be marked completed; only continue to completion after successful initialization and reload..github/workflows/ci.yml-9-12 (1)
9-12: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict the workflow token to read-only repository access.
No
permissionsblock is defined, so repository defaults apply across every job. These jobs only require source checkout.Proposed fix
env: GO_VERSION: '1.23' DATABASE_URL: postgres://streamforge:streamforge@localhost:5432/streamforge?sslmode=disable +permissions: + contents: read + jobs:🤖 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 @.github/workflows/ci.yml around lines 9 - 12, Add a top-level permissions block in the CI workflow granting repository contents read-only access, so all jobs—including source checkout—use the least-privileged token scope. Keep the existing environment variables and job behavior unchanged.Source: Linters/SAST tools
internal/worker/pool.go-97-114 (1)
97-114: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the duplicate Redis progress increment and event.
Lines 110-112 increment and publish directly, then
UpdateJobProgressincrements Redis and publishes another progress event internally. Each item therefore advances Redis twice and emits duplicate events. Letjobs.Service.UpdateJobProgressown this operation.Proposed fix
- for _, item := range items { + for completed, item := range items { ... - updated, _ := p.redis.IncrementProgress(ctx, jobID) - if updated != nil { - p.redis.PublishJobEvent(ctx, jobID, "progress", updated) - p.jobService.UpdateJobProgress(ctx, jobID, updated.Completed) + if err := p.jobService.UpdateJobProgress(ctx, jobID, completed+1); err != nil { + p.jobService.UpdateJobStatus(ctx, jobID, "FAILED", err.Error()) + return }🤖 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 `@internal/worker/pool.go` around lines 97 - 114, Remove the direct IncrementProgress and PublishJobEvent calls from the item-processing loop, leaving jobs.Service.UpdateJobProgress as the sole owner of Redis progress updates and progress event publication. Preserve the existing completed-count value passed to UpdateJobProgress in the worker flow..github/workflows/ci.yml-87-92 (1)
87-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the host-side Redis and RabbitMQ checks.
ubuntu-latestdoesn’t includeredis-cliorrabbitmq-diagnostics, and the service health checks already gate the job until those containers are ready.🤖 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 @.github/workflows/ci.yml around lines 87 - 92, Update the “Wait for services” step in the CI workflow to remove the host-side redis-cli and rabbitmq-diagnostics commands, leaving only the necessary PostgreSQL check and existing wait behavior because service health checks already verify Redis and RabbitMQ readiness..golangci.yml-1-1 (1)
1-1: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMigrate
.golangci.ymlto golangci-lint v2 and pin the workflow version
.golangci.ymlis still v1-style, and.github/workflows/ci.ymlusesversion: latest; that can break lint runs as soon as the action picks up v2. Migrate the config and pin CI to the same v2 release.🤖 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 @.golangci.yml at line 1, Migrate the .golangci.yml configuration from v1 syntax to the v2 schema, preserving the existing lint behavior and settings. Update the version value in the CI workflow to pin the exact golangci-lint v2 release used by the migrated configuration, replacing latest.cmd/api/main.go-53-65 (1)
53-65: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftRemove worker pool execution and queue consumption from the API entrypoint.
The
cmd/api/main.gofile initializes aworkerPooland starts consuming jobs from RabbitMQ. Because there is a dedicatedcmd/workerservice designed for this exact purpose, having the API server also act as a consumer defeats the separation of concerns. The API instances will compete for messages onQUEUE_NAME: media.processing.queue. If these jobs involve heavy media processing, it will starve the API nodes of CPU resources and cause severe latency spikes for HTTP requests.Please remove the worker pool initialization and queue consumption routines from the API process. If the
workerPoolparameter insetupRouter(Line 67) andapi.RegisterRoutesis not genuinely needed for HTTP handlers, it should be removed from their signatures as well.🤖 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 `@cmd/api/main.go` around lines 53 - 65, Remove worker pool creation, lifecycle management, and queue consumption from the API entrypoint around worker.NewPool, workerPool.Start, workerPool.Stop, and queueSvc. Then remove workerPool from setupRouter and api.RegisterRoutes signatures and call sites if no HTTP handler uses it, preserving only dependencies required by the API server.Dockerfile-14-25 (1)
14-25: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRun containers as a non-root user. Both Dockerfiles run their final Alpine stages as the default
rootuser, violating container security best practices (flagged by Trivy DS-0002).
Dockerfile#L14-L25: Create a non-root user/group, switch to it usingUSER, and update theCOPYinstructions to assign ownership. For example:RUN apk add --no-cache ca-certificates tzdata && \ addgroup -S appgroup && adduser -S appuser -G appgroup WORKDIR /app COPY --chown=appuser:appgroup --from=builder /streamforge . COPY --chown=appuser:appgroup --from=builder /app/docs ./docs USER appuserDockerfile.worker#L14-L22: Apply the same user creation and switch. Update theCOPYinstruction to:COPY --chown=appuser:appgroup --from=builder /streamforge-worker .🤖 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 `@Dockerfile` around lines 14 - 25, Run both final Alpine stages as a non-root user. In Dockerfile lines 14-25, create appuser/appgroup, add --chown=appuser:appgroup to both COPY instructions, and switch with USER appuser; apply the same user creation and USER appuser switch in Dockerfile.worker lines 14-22, adding ownership to its streamforge-worker COPY.Source: Linters/SAST tools
docker-compose.yml-60-64 (1)
60-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
REDIS_HOST/REDIS_PORTandRABBITMQ_HOST/RABBITMQ_PORThere.
internal/config/config.goreads those names, soREDIS_ADDR/RABBITMQ_URLare ignored and the containers fall back tolocalhost, breaking Redis/RabbitMQ connectivity in compose.🤖 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 `@docker-compose.yml` around lines 60 - 64, Update the compose environment configuration to use the REDIS_HOST and REDIS_PORT variables consumed by the config loader instead of REDIS_ADDR, and replace RABBITMQ_URL with RABBITMQ_HOST and RABBITMQ_PORT. Set the values to the service hostnames and ports so the containers connect to Redis and RabbitMQ rather than localhost.
🟡 Minor comments (2)
internal/auth/service.go-131-134 (1)
131-134: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winValidate only HS256 tokens
ValidateTokenshould reject tokens signed with anything other thanHS256, sincegenerateTokenalways issues that algorithm. Pinning the parser keeps the auth contract explicit and avoids accepting unexpected HMAC variants.Proposed fix
-token, err := jwt.ParseWithClaims(tokenString, &Claims{}, func(token *jwt.Token) (interface{}, error) { - return s.jwtKey, nil -}) +token, err := jwt.ParseWithClaims( + tokenString, + &Claims{}, + func(token *jwt.Token) (interface{}, error) { + return s.jwtKey, nil + }, + jwt.WithValidMethods([]string{jwt.SigningMethodHS256.Alg()}), +)🤖 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 `@internal/auth/service.go` around lines 131 - 134, Update ValidateToken’s jwt.ParseWithClaims key-function to verify that token.Method is the HS256 signing method before returning s.jwtKey; return an error for any other algorithm, preserving the existing validation flow for HS256 tokens.README.md-75-77 (1)
75-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the database password in the migration command.
The local development instructions instruct the user to run migrations with the
streamforgepassword, but thedocker-compose.ymlfile configures the local Postgres instance withstreamforge_dev. This mismatch will cause the migration command to fail with an authentication error.🩹 Proposed fix
# Run migrations -go run github.com/golang-migrate/migrate/v4/cmd/migrate@latest -path migrations -database "postgres://streamforge:streamforge@localhost:5432/streamforge?sslmode=disable" up +go run github.com/golang-migrate/migrate/v4/cmd/migrate@latest -path migrations -database "postgres://streamforge:streamforge_dev@localhost:5432/streamforge?sslmode=disable" up🤖 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 `@README.md` around lines 75 - 77, Update the migration command in the README to use the local Postgres password configured by docker-compose.yml, replacing the `streamforge` password with `streamforge_dev` while preserving the rest of the connection string.
🧹 Nitpick comments (10)
migrations/000001_init.down.sql (3)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
DROP INDEXstatements.Indices are automatically dropped when the table they belong to is dropped. You can safely remove the
DROP INDEXstatements.♻️ Proposed refactor
-- Migration: 000001_drop_users_table.down.sql -DROP INDEX IF EXISTS idx_users_email; DROP TABLE IF EXISTS users;🤖 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 `@migrations/000001_init.down.sql` around lines 21 - 22, Remove the redundant DROP INDEX statement for idx_users_email from the migration rollback, leaving the DROP TABLE IF EXISTS users operation unchanged.
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
DROP INDEXstatements.Indices are automatically dropped when the table they belong to is dropped. You can safely remove the
DROP INDEXstatements.♻️ Proposed refactor
-- Migration: 000003_drop_media_items_table.down.sql -DROP INDEX IF EXISTS idx_media_items_status; -DROP INDEX IF EXISTS idx_media_items_job_id; DROP TABLE IF EXISTS media_items;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrations/000001_init.down.sql` around lines 8 - 10, Remove the redundant DROP INDEX statements from the migration rollback, keeping the DROP TABLE IF EXISTS media_items statement in the down migration.
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
DROP INDEXstatements.Indices are automatically dropped when the table they belong to is dropped. You can safely remove the
DROP INDEXstatements.♻️ Proposed refactor
-- Migration: 000002_drop_jobs_table.down.sql -DROP INDEX IF EXISTS idx_jobs_created_at; -DROP INDEX IF EXISTS idx_jobs_status; -DROP INDEX IF EXISTS idx_jobs_user_id; DROP TABLE IF EXISTS jobs;🤖 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 `@migrations/000001_init.down.sql` around lines 14 - 17, Remove the redundant DROP INDEX statements for idx_jobs_created_at, idx_jobs_status, and idx_jobs_user_id from the migration rollback, leaving DROP TABLE IF EXISTS jobs as the sole cleanup operation.internal/api/middleware/middleware.go (1)
70-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not silently discard every access log.
This middleware performs Gin's logging work but always emits an empty record, removing request-level observability. Use the standard logger or output a structured record.
🤖 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 `@internal/api/middleware/middleware.go` around lines 70 - 74, Update the Logger function’s Gin formatter so it returns a meaningful request log record instead of an empty string. Reuse Gin’s standard logger output or format the available LogFormatterParams into a structured record, preserving request-level observability.internal/auth/service_test.go (1)
27-81: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for unsupported signing algorithms.
After restricting validation to HS256, generate an HS384/HS512 token with the same secret and assert
ErrInvalidToken; otherwise the security contract can regress unnoticed.🤖 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 `@internal/auth/service_test.go` around lines 27 - 81, Extend TestService_ValidateToken_Invalid or add a focused regression test using the same secret to create an HS384 or HS512 token, then validate it through the service and assert ErrInvalidToken with nil claims. Keep the existing HS256 generation and validation coverage unchanged, and exercise the algorithm restriction in ValidateToken.internal/middleware/middleware.go (1)
69-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImplement logging or remove this misleading middleware.
Logger()currently provides no request logging. Callers can believe logging is enabled while receiving no access records.🤖 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 `@internal/middleware/middleware.go` around lines 69 - 73, Update the Logger middleware function to emit request access logs after c.Next(), including the relevant request and response details, or remove Logger and its callers if logging is not intended. Ensure the resulting middleware does not advertise enabled logging without producing records.internal/jobs/service_test.go (1)
14-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftCover the service's state-transition and event paths.
Add tests for partial media-item failures, cancellation dependency failures, empty/local subscriber sets, correct job-channel routing, and concurrent subscribe/unsubscribe/publish. The current mapping tests cannot detect the production issues in these paths.
🤖 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 `@internal/jobs/service_test.go` around lines 14 - 101, Extend the service tests beyond NewService and the mapping helpers to cover state transitions and event handling: partial media-item failures, cancellation dependency errors, empty and local subscriber sets, correct per-job channel routing, and concurrent subscribe/unsubscribe/publish operations. Exercise the service’s existing failure, cancellation, subscription, and publishing methods using appropriate mocks or test doubles, and assert both returned errors and resulting state/events.README.md (2)
131-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Add the
textlanguage identifier to resolve markdown lint warnings.🎨 Proposed fix
-``` +```text🤖 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 `@README.md` at line 131, Update the fenced code block in README.md by adding the text language identifier to its opening fence, preserving the block’s existing contents and closing fence.Source: Linters/SAST tools
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Add a language identifier (e.g.,
textormermaid) to the fenced code block to improve syntax highlighting and resolve markdown lint warnings.🎨 Proposed fix
-``` +```text🤖 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 `@README.md` at line 7, Specify a language identifier on the fenced code block in the README, using the appropriate value such as text or mermaid, while preserving the block’s existing content.Source: Linters/SAST tools
Makefile (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare all targets in
.PHONYand add a defaultalltarget.The
clean,generate, andtidytargets are missing from.PHONY, which could cause failures if files with those names are created. Additionally, it is a standard convention to provide analltarget.🛠️ Proposed fix
-.PHONY: help build test lint docker-up docker-down migrate-up migrate-down fmt vet +.PHONY: all help build test lint docker-up docker-down migrate-up migrate-down fmt vet generate tidy clean + +all: build🤖 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 `@Makefile` around lines 1 - 2, Update the Makefile’s .PHONY declaration to include clean, generate, and tidy, and add all as the default target. Ensure all depends on the appropriate primary build targets while preserving the existing target behavior.Source: Linters/SAST tools
🤖 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 `@go.mod`:
- Line 9: Update the github.com/jackc/pgx/v5 dependency in go.mod from v5.5.5 to
v5.9.2 or a later patched release, and refresh the corresponding go.sum entries
if required.
- Line 13: Update the golang.org/x/crypto dependency declaration in go.mod from
v0.29.0 to v0.52.0 or a newer secure release, and refresh the module checksums
as needed while preserving dependency consistency.
In `@internal/database/repository.go`:
- Line 112: Cast every listed string parameter to UUID in the SQL queries within
internal/database/repository.go: update the predicates at lines 112, 130, 164,
181, 189, 197, 242, 260, 294, 311, 319, and 327 by appending the PostgreSQL
::uuid cast to their respective placeholders, while preserving the existing
repository methods and parameter ordering.
- Around line 79-89: Update the Job.ErrorMessage field from string to *string so
nullable error_message database values can be scanned safely, preserving the
existing field and struct behavior otherwise.
- Around line 202-213: Update the MediaItem struct so the nullable database
columns Title, SizeBytes, and ErrorMessage use pointer types instead of
primitive string or int64 types, allowing NULL values to be scanned safely while
leaving non-nullable fields unchanged.
---
Major comments:
In @.github/workflows/ci.yml:
- Around line 18-19: Update every actions/checkout@v4 step in the workflow,
including the jobs at the referenced locations, to disable persisted checkout
credentials by configuring persist-credentials as false. Apply the same setting
consistently to all checkout actions without changing unrelated job steps.
- Around line 9-12: Add a top-level permissions block in the CI workflow
granting repository contents read-only access, so all jobs—including source
checkout—use the least-privileged token scope. Keep the existing environment
variables and job behavior unchanged.
- Around line 87-92: Update the “Wait for services” step in the CI workflow to
remove the host-side redis-cli and rabbitmq-diagnostics commands, leaving only
the necessary PostgreSQL check and existing wait behavior because service health
checks already verify Redis and RabbitMQ readiness.
In @.golangci.yml:
- Line 1: Migrate the .golangci.yml configuration from v1 syntax to the v2
schema, preserving the existing lint behavior and settings. Update the version
value in the CI workflow to pin the exact golangci-lint v2 release used by the
migrated configuration, replacing latest.
In `@cmd/api/main.go`:
- Around line 53-65: Remove worker pool creation, lifecycle management, and
queue consumption from the API entrypoint around worker.NewPool,
workerPool.Start, workerPool.Stop, and queueSvc. Then remove workerPool from
setupRouter and api.RegisterRoutes signatures and call sites if no HTTP handler
uses it, preserving only dependencies required by the API server.
In `@cmd/worker/main.go`:
- Around line 53-59: Update the queue consumer supervision around Consume and
the worker startup flow so a Consume failure is propagated instead of only
logged; coordinate the consumer error with the signal channel, cancel the worker
context, and enter the existing shutdown or retry-with-backoff path. Do not
report “Worker started” after startup has failed, and preserve normal
signal-driven shutdown behavior.
In `@docker-compose.yml`:
- Around line 60-64: Update the compose environment configuration to use the
REDIS_HOST and REDIS_PORT variables consumed by the config loader instead of
REDIS_ADDR, and replace RABBITMQ_URL with RABBITMQ_HOST and RABBITMQ_PORT. Set
the values to the service hostnames and ports so the containers connect to Redis
and RabbitMQ rather than localhost.
In `@Dockerfile`:
- Around line 14-25: Run both final Alpine stages as a non-root user. In
Dockerfile lines 14-25, create appuser/appgroup, add --chown=appuser:appgroup to
both COPY instructions, and switch with USER appuser; apply the same user
creation and USER appuser switch in Dockerfile.worker lines 14-22, adding
ownership to its streamforge-worker COPY.
In `@docs/openapi.yaml`:
- Around line 196-201: Update the OpenAPI 200 response schema for the
job-cancellation route to match the implemented payload: document an object
containing the cancellation message instead of referencing JobResponse. Keep the
existing cancellation response description and content type unchanged.
In `@internal/api/middleware/middleware.go`:
- Around line 76-85: The Recovery function in
internal/api/middleware/middleware.go must stop exposing recovered panic
details: log the recovered value server-side, return only the generic
internal-server-error response, and return immediately after writing it. Apply
the same behavior in internal/middleware/middleware.go, updating its
corresponding recovery middleware as well.
In `@internal/api/routes.go`:
- Around line 127-138: Update the job-creation flow around CreateJob and
pool.Submit so a failed queue submission cannot leave the persisted job in
CREATED state. Use the existing transactional outbox mechanism if available;
otherwise, compensate by marking resp.ID failed or cancelled before returning
the 500 response, while preserving the current success path.
In `@internal/config/config.go`:
- Around line 61-63: Update RabbitMQConfig.URI to construct the AMQP URI with
net/url rather than concatenating credential strings. URL-encode User and
Password while preserving the existing Host, Port, and path components so
credentials containing reserved characters produce a valid connection URI.
- Around line 38-40: Update DatabaseConfig.DSN to construct the PostgreSQL
connection string with net/url, ensuring User and Password are URL-encoded so
special characters remain valid credentials. Preserve the existing host, port,
and database name components.
In `@internal/database/postgres.go`:
- Around line 20-22: Update Config.DSN to construct the PostgreSQL connection
string using net/url so User and Password are URL-encoded before insertion,
while preserving the existing host, port, and database name values.
In `@internal/jobs/service.go`:
- Around line 230-250: Update CreateMediaItems to execute SetJobTotalItems and
all CreateMediaItem calls within one repository transaction, propagating errors
from each operation. Move the total update into the transaction alongside the
inserts so any failure rolls back the entire operation and avoids partial item
sets.
- Around line 159-167: Update the cancellation flow around UpdateJobStatus to
check and propagate errors from redis.SetJobStatus, redis.DeleteProgress, and
repo.UpdateMediaItemsStatus instead of discarding them. Define and apply the
appropriate transactional or compensating behavior so the method does not return
success when any dependent update fails and cannot leave inconsistent job, item,
or cache state.
- Around line 267-310: The SSE subscriber lifecycle has competing owners,
allowing channel sends, removal, closure, and response writes to race or panic.
In internal/jobs/service.go lines 267-310, update Subscribe and PublishEvent so
subscriber registration, removal, snapshotting, and channel ownership follow one
synchronized lifecycle without sending to a closed channel; in
internal/api/routes.go lines 274-296, stop closing the registered channel and
stop writing to Gin’s response from a separate goroutine, leaving response
writes to the request-handling goroutine that owns them.
- Around line 253-262: Route item events across processes by resolving and
passing the media item’s actual job ID in UpdateMediaItemStatus at
internal/jobs/service.go:253-262; update PublishEvent at
internal/jobs/service.go:287-315 to publish to Redis regardless of local
subscriber count; update the SSE handler at internal/api/routes.go:245-289 to
subscribe each connection to the job’s Redis event channel.
In `@internal/queue/queue.go`:
- Around line 156-164: Update the queue dispatch flow around the delivery select
and worker processing so workers receive the full delivery/envelope rather than
only the message, and acknowledge it only after processing succeeds; NACK with
requeue on processing failure or context cancellation. Remove the default branch
and block on sending to the worker channel to provide backpressure without
premature requeue loops.
- Around line 140-169: The Consume method currently detaches its delivery loop
and returns immediately, preventing callers from observing broker or channel
termination. Remove the inner goroutine around the select loop in Consume, run
the loop synchronously, and return when msgs closes or ctx.Done() fires;
preserve the existing acknowledgment and rejection behavior.
In `@internal/redis/redis.go`:
- Around line 130-141: Update Client.RateLimitCheck to perform the counter
increment and expiry assignment atomically, using a Lua script or Redis
transaction so every successful increment receives the requested window TTL even
if the counter is newly created or the expiry operation would otherwise fail;
preserve the existing limit comparison and error propagation.
- Around line 70-109: Update Client.IncrementProgress so the read-modify-write
operation is atomic: watch the progress key and perform the GET, increment, and
SET within a retryable transaction, or replace the flow with an equivalent Lua
script. Preserve the existing missing-key, serialization, expiration, and
TxFailedErr handling while ensuring concurrent callers cannot lose increments.
In `@internal/worker/pool.go`:
- Around line 148-151: Update Pool.Stop to remove the close(p.taskQueue) call;
JobChan exposes taskQueue for external writes, so cancellation via p.cancel
followed by p.wg.Wait should be the only shutdown behavior and must remain safe
for repeated Stop calls.
- Around line 87-95: Handle errors from SetJobTotalItems, each CreateMediaItems
call, and the second GetMediaItems invocation within the len(items) == 0
initialization path. On any failure, fail or return the job through the existing
worker error-handling flow so an empty or partially initialized job cannot be
marked completed; only continue to completion after successful initialization
and reload.
- Around line 97-114: Remove the direct IncrementProgress and PublishJobEvent
calls from the item-processing loop, leaving jobs.Service.UpdateJobProgress as
the sole owner of Redis progress updates and progress event publication.
Preserve the existing completed-count value passed to UpdateJobProgress in the
worker flow.
In `@migrations/000001_init.up.sql`:
- Around line 1-73: Keep the consolidated schema in
migrations/000001_init.up.sql (lines 1-73), first aligning its columns and ENUM
values with the struct models, including size_bytes versus size. Delete the
redundant migrations/000001_create_users_table.up.sql (lines 1-15),
migrations/000002_create_jobs_table.up.sql (lines 1-22), and
migrations/000003_create_media_items_table.up.sql (lines 1-22) so only the
consolidated initialization migration defines these tables.
---
Minor comments:
In `@internal/auth/service.go`:
- Around line 131-134: Update ValidateToken’s jwt.ParseWithClaims key-function
to verify that token.Method is the HS256 signing method before returning
s.jwtKey; return an error for any other algorithm, preserving the existing
validation flow for HS256 tokens.
In `@README.md`:
- Around line 75-77: Update the migration command in the README to use the local
Postgres password configured by docker-compose.yml, replacing the `streamforge`
password with `streamforge_dev` while preserving the rest of the connection
string.
---
Nitpick comments:
In `@internal/api/middleware/middleware.go`:
- Around line 70-74: Update the Logger function’s Gin formatter so it returns a
meaningful request log record instead of an empty string. Reuse Gin’s standard
logger output or format the available LogFormatterParams into a structured
record, preserving request-level observability.
In `@internal/auth/service_test.go`:
- Around line 27-81: Extend TestService_ValidateToken_Invalid or add a focused
regression test using the same secret to create an HS384 or HS512 token, then
validate it through the service and assert ErrInvalidToken with nil claims. Keep
the existing HS256 generation and validation coverage unchanged, and exercise
the algorithm restriction in ValidateToken.
In `@internal/jobs/service_test.go`:
- Around line 14-101: Extend the service tests beyond NewService and the mapping
helpers to cover state transitions and event handling: partial media-item
failures, cancellation dependency errors, empty and local subscriber sets,
correct per-job channel routing, and concurrent subscribe/unsubscribe/publish
operations. Exercise the service’s existing failure, cancellation, subscription,
and publishing methods using appropriate mocks or test doubles, and assert both
returned errors and resulting state/events.
In `@internal/middleware/middleware.go`:
- Around line 69-73: Update the Logger middleware function to emit request
access logs after c.Next(), including the relevant request and response details,
or remove Logger and its callers if logging is not intended. Ensure the
resulting middleware does not advertise enabled logging without producing
records.
In `@Makefile`:
- Around line 1-2: Update the Makefile’s .PHONY declaration to include clean,
generate, and tidy, and add all as the default target. Ensure all depends on the
appropriate primary build targets while preserving the existing target behavior.
In `@migrations/000001_init.down.sql`:
- Around line 21-22: Remove the redundant DROP INDEX statement for
idx_users_email from the migration rollback, leaving the DROP TABLE IF EXISTS
users operation unchanged.
- Around line 8-10: Remove the redundant DROP INDEX statements from the
migration rollback, keeping the DROP TABLE IF EXISTS media_items statement in
the down migration.
- Around line 14-17: Remove the redundant DROP INDEX statements for
idx_jobs_created_at, idx_jobs_status, and idx_jobs_user_id from the migration
rollback, leaving DROP TABLE IF EXISTS jobs as the sole cleanup operation.
In `@README.md`:
- Line 131: Update the fenced code block in README.md by adding the text
language identifier to its opening fence, preserving the block’s existing
contents and closing fence.
- Line 7: Specify a language identifier on the fenced code block in the README,
using the appropriate value such as text or mermaid, while preserving the
block’s existing content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45ae74b0-c4b1-4540-b0ea-1c1fa3f92de6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
.gitattributes.github/workflows/ci.yml.golangci.ymlDockerfileDockerfile.workerMakefileREADME.mdcmd/api/main.gocmd/worker/main.godocker-compose.ymldocs/openapi.yamlgo.modinternal/api/middleware/middleware.gointernal/api/routes.gointernal/auth/service.gointernal/auth/service_test.gointernal/config/config.gointernal/config/config_test.gointernal/database/postgres.gointernal/database/repository.gointernal/jobs/service.gointernal/jobs/service_test.gointernal/middleware/middleware.gointernal/queue/queue.gointernal/queue/queue_test.gointernal/redis/redis.gointernal/worker/pool.gointernal/worker/pool_test.gomigrations/000001_create_users_table.up.sqlmigrations/000001_init.down.sqlmigrations/000001_init.up.sqlmigrations/000002_create_jobs_table.up.sqlmigrations/000003_create_media_items_table.up.sql
| github.com/gin-gonic/gin v1.10.0 | ||
| github.com/golang-jwt/jwt/v5 v5.3.1 | ||
| github.com/google/uuid v1.6.0 | ||
| github.com/jackc/pgx/v5 v5.5.5 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the latest secure version of pgx/v5
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/jackc/pgx/v5") {
nodes {
advisory {
summary
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: amirmalekian/streamforge
Length of output: 1057
Upgrade github.com/jackc/pgx/v5 to 5.9.2 or later. Version 5.5.5 is still covered by known SQL-injection and memory-safety advisories, so it needs a patched release.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[CRITICAL] 9-9: github.com/jackc/pgx/v5 5.5.5: CVE-2026-33815 in github.com/jackc/pgx
(GO-2026-4771)
[CRITICAL] 9-9: github.com/jackc/pgx/v5 5.5.5: CVE-2026-33816 in github.com/jackc/pgx
(GO-2026-4772)
[CRITICAL] 9-9: github.com/jackc/pgx/v5 5.5.5: SQL Injection via placeholder confusion with dollar quoted string literals in github.com/jackc/pgx
(GO-2026-5004)
[CRITICAL] 9-9: github.com/jackc/pgx/v5 5.5.5: Memory-safety vulnerability in github.com/jackc/pgx/v5.
[CRITICAL] 9-9: github.com/jackc/pgx/v5 5.5.5: pgx: SQL Injection via placeholder confusion with dollar quoted string literals
[CRITICAL] 9-9: github.com/jackc/pgx/v5 5.5.5: pgx contains memory-safety vulnerability
🤖 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 `@go.mod` at line 9, Update the github.com/jackc/pgx/v5 dependency in go.mod
from v5.5.5 to v5.9.2 or a later patched release, and refresh the corresponding
go.sum entries if required.
Source: Linters/SAST tools
| github.com/rabbitmq/amqp091-go v1.10.0 | ||
| github.com/redis/go-redis/v9 v9.5.1 | ||
| github.com/stretchr/testify v1.9.0 | ||
| golang.org/x/crypto v0.29.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the latest secure version of golang.org/x/crypto
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/crypto") {
nodes {
advisory {
summary
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: amirmalekian/streamforge
Length of output: 1134
Upgrade golang.org/x/crypto to v0.52.0 or newer. v0.29.0 falls within the vulnerable range for multiple advisories, including auth bypass and panic/DoS issues.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Misuse of connection.serverAuthenticate may cause authorization bypass in golang.org/x/crypto
(GO-2024-3321)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Potential denial of service in golang.org/x/crypto
(GO-2025-3487)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Potential denial of service in golang.org/x/crypto/ssh/agent
(GO-2025-4116)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Unbounded memory consumption in golang.org/x/crypto/ssh
(GO-2025-4134)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Malformed constraint may cause denial of service in golang.org/x/crypto/ssh/agent
(GO-2025-4135)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking key constraints not enforced in golang.org/x/crypto/ssh/agent
(GO-2026-5005)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking agent constraints dropped when forwarding keys in golang.org/x/crypto/ssh/agent
(GO-2026-5006)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking byte arithmetic causes underflow and panic in golang.org/x/crypto/ssh
(GO-2026-5013)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking bypass of certificate restrictions in golang.org/x/crypto/ssh
(GO-2026-5014)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking server panic during CheckHostKey/Authenticate in golang.org/x/crypto/ssh
(GO-2026-5015)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking memory leak when rejecting channels can lead to DoS in golang.org/x/crypto/ssh
(GO-2026-5016)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking client can cause server deadlock on unexpected responses in golang.org/x/crypto/ssh
(GO-2026-5017)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking pathological RSA/DSA parameters may cause DoS in golang.org/x/crypto/ssh
(GO-2026-5018)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking bypass of FIDO/U2F security keys physical interaction in golang.org/x/crypto/ssh
(GO-2026-5019)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking infinite loop on large channel writes in golang.org/x/crypto/ssh
(GO-2026-5020)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking auth bypass via unenforced @revoked status in golang.org/x/crypto/ssh/knownhosts
(GO-2026-5021)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking VerifiedPublicKeyCallback permissions skip enforcement in golang.org/x/crypto/ssh
(GO-2026-5023)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Invoking pathological inputs can lead to client panic in golang.org/x/crypto/ssh/agent
(GO-2026-5033)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: The golang.org/x/crypto/openpgp package is unmaintained, unsafe by design, and has known security issues
(GO-2026-5932)
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto vulnerable to invoking bypass of certificate restrictions
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto vulnerable to auth bypass via unenforced @revoked status
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto is vulnerable to invoking server panic during CheckHostKey/Authenticate flow
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto: FIDO/U2F security key physical presence check can be bypassed
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto: Invoking pathological inputs can lead to client panic
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto doesn't drop invoking agent constraints when forwarding keys
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto/ssh/agent vulnerable to panic if message is malformed due to out of bounds read
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto Vulnerable to Denial of Service (DoS) via Slow or Incomplete Key Exchange
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto/ssh allows an attacker to cause unbounded memory consumption
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto doesn't enforce invoking key constraints
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto: Invoking byte arithmetic causes underflow and panic
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto: Invoking memory leak when rejecting channels can lead to DoS
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto vulnerable to infinite loop on large channel writes
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: Misuse of ServerConfig.PublicKeyCallback may cause authorization bypass in golang.org/x/crypto
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto: Invoking client can cause server deadlock on unexpected responses
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto: Invoking pathological RSA/DSA parameters may cause DoS
[CRITICAL] 13-13: golang.org/x/crypto 0.29.0: golang.org/x/crypto: Invoking VerifiedPublicKeyCallback permissions skip enforcement
🤖 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 `@go.mod` at line 13, Update the golang.org/x/crypto dependency declaration in
go.mod from v0.29.0 to v0.52.0 or a newer secure release, and refresh the module
checksums as needed while preserving dependency consistency.
Source: Linters/SAST tools
| type Job struct { | ||
| ID uuid.UUID | ||
| UserID uuid.UUID | ||
| SourceURL string | ||
| Status string | ||
| TotalItems int | ||
| CompletedItems int | ||
| ErrorMessage string | ||
| CreatedAt string | ||
| UpdatedAt string | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Use a pointer for nullable database columns.
The error_message column in the jobs table is nullable. When pgx attempts to scan a NULL value into a primitive string variable, it will return an error (e.g., can't scan NULL into *string), causing job retrieval and creation to fail instantly for healthy jobs. Change ErrorMessage to *string to safely handle NULL values.
🐛 Proposed fix
type Job struct {
ID uuid.UUID
UserID uuid.UUID
SourceURL string
Status string
TotalItems int
CompletedItems int
- ErrorMessage string
+ ErrorMessage *string
CreatedAt string
UpdatedAt string
}📝 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.
| type Job struct { | |
| ID uuid.UUID | |
| UserID uuid.UUID | |
| SourceURL string | |
| Status string | |
| TotalItems int | |
| CompletedItems int | |
| ErrorMessage string | |
| CreatedAt string | |
| UpdatedAt string | |
| } | |
| type Job struct { | |
| ID uuid.UUID | |
| UserID uuid.UUID | |
| SourceURL string | |
| Status string | |
| TotalItems int | |
| CompletedItems int | |
| ErrorMessage *string | |
| CreatedAt string | |
| UpdatedAt string | |
| } |
🤖 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 `@internal/database/repository.go` around lines 79 - 89, Update the
Job.ErrorMessage field from string to *string so nullable error_message database
values can be scanned safely, preserving the existing field and struct behavior
otherwise.
| job := &Job{} | ||
| err := r.pool.QueryRow(ctx, ` | ||
| SELECT id, user_id, source_url, status, total_items, completed_items, error_message, created_at, updated_at | ||
| FROM jobs WHERE id = $1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Cast string parameters to UUID in queries. By default, pgx sends Go string arguments as text type. Since PostgreSQL strictly enforces types and does not implicitly cast text to uuid in extended query protocol comparisons, these queries will fail with operator does not exist: uuid = text. Add the ::uuid cast to all parameters where a string is checked against a UUID column.
internal/database/repository.go#L112-L112: UpdateWHERE id = $1toWHERE id = $1::uuid.internal/database/repository.go#L130-L130: UpdateWHERE user_id = $1toWHERE user_id = $1::uuid.internal/database/repository.go#L164-L164: UpdateWHERE user_id = $1toWHERE user_id = $1::uuid.internal/database/repository.go#L181-L181: UpdateWHERE id = $3toWHERE id = $3::uuid.internal/database/repository.go#L189-L189: UpdateWHERE id = $2toWHERE id = $2::uuid.internal/database/repository.go#L197-L197: UpdateWHERE id = $2toWHERE id = $2::uuid.internal/database/repository.go#L242-L242: UpdateWHERE id = $1toWHERE id = $1::uuid.internal/database/repository.go#L260-L260: UpdateWHERE job_id = $1toWHERE job_id = $1::uuid.internal/database/repository.go#L294-L294: UpdateWHERE job_id = $1toWHERE job_id = $1::uuid.internal/database/repository.go#L311-L311: UpdateWHERE id = $4toWHERE id = $4::uuid.internal/database/repository.go#L319-L319: UpdateWHERE id = $2toWHERE id = $2::uuid.internal/database/repository.go#L327-L327: UpdateWHERE job_id = $2toWHERE job_id = $2::uuid.
📍 Affects 1 file
internal/database/repository.go#L112-L112(this comment)internal/database/repository.go#L130-L130internal/database/repository.go#L164-L164internal/database/repository.go#L181-L181internal/database/repository.go#L189-L189internal/database/repository.go#L197-L197internal/database/repository.go#L242-L242internal/database/repository.go#L260-L260internal/database/repository.go#L294-L294internal/database/repository.go#L311-L311internal/database/repository.go#L319-L319internal/database/repository.go#L327-L327
🤖 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 `@internal/database/repository.go` at line 112, Cast every listed string
parameter to UUID in the SQL queries within internal/database/repository.go:
update the predicates at lines 112, 130, 164, 181, 189, 197, 242, 260, 294, 311,
319, and 327 by appending the PostgreSQL ::uuid cast to their respective
placeholders, while preserving the existing repository methods and parameter
ordering.
| type MediaItem struct { | ||
| ID uuid.UUID | ||
| JobID uuid.UUID | ||
| Title string | ||
| SourceURL string | ||
| Status string | ||
| Progress int | ||
| SizeBytes int64 | ||
| ErrorMessage string | ||
| CreatedAt string | ||
| UpdatedAt string | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Use pointers for nullable database columns.
The title, size_bytes, and error_message columns in the media_items table are nullable. Scanning NULL into primitive string or int64 variables will cause pgx to return an error and fail the retrieval operation. Change these fields to pointers to safely handle NULL values.
🐛 Proposed fix
type MediaItem struct {
ID uuid.UUID
JobID uuid.UUID
- Title string
+ Title *string
SourceURL string
Status string
Progress int
- SizeBytes int64
- ErrorMessage string
+ SizeBytes *int64
+ ErrorMessage *string
CreatedAt string
UpdatedAt string
}📝 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.
| type MediaItem struct { | |
| ID uuid.UUID | |
| JobID uuid.UUID | |
| Title string | |
| SourceURL string | |
| Status string | |
| Progress int | |
| SizeBytes int64 | |
| ErrorMessage string | |
| CreatedAt string | |
| UpdatedAt string | |
| } | |
| type MediaItem struct { | |
| ID uuid.UUID | |
| JobID uuid.UUID | |
| Title *string | |
| SourceURL string | |
| Status string | |
| Progress int | |
| SizeBytes *int64 | |
| ErrorMessage *string | |
| CreatedAt string | |
| UpdatedAt string | |
| } |
🤖 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 `@internal/database/repository.go` around lines 202 - 213, Update the MediaItem
struct so the nullable database columns Title, SizeBytes, and ErrorMessage use
pointer types instead of primitive string or int64 types, allowing NULL values
to be scanned safely while leaving non-nullable fields unchanged.
- lint: golangci-lint with Go 1.23 - test: unit tests with postgres, redis, rabbitmq services - build: compile API and worker binaries - docker: multi-arch builds on main branch push
- Use go-version-file: go.mod for automatic Go version detection - Pin golangci-lint to v1.63.4 (compatible with Go 1.23) - Remove deprecated Node.js version warnings - Fix redis service health check options - Simplify to single workflow with lint, test, build jobs
- Remove apt-get install redis-tools step - Rely on service container health checks for redis readiness - Use go-version-file: go.mod for automatic Go version detection - Keep lint/test/build stages only
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
24-27: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPin the
golangci-lintversion.The PR summary mentions "pinned golangci-lint compatibility," but the action uses
version: latest. Usinglatestis highly discouraged because a new release will eventually introduce new linters and break the build unexpectedly. Pin this to a specific version (e.g.,v1.59.1).🛠️ Proposed fix
- name: Run golangci-lint uses: golangci/golangci-lint-action@v6 with: - version: latest + version: v1.59.1 args: --timeout=5m🤖 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 @.github/workflows/ci.yml around lines 24 - 27, Update the golangci/golangci-lint-action configuration to replace version: latest with a specific pinned golangci-lint release, such as v1.59.1, while preserving the existing action and timeout settings.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
5-7: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRestrict default workflow permissions.
By default, GitHub Actions workflows run with broad permissions. It is a security best practice to adhere to the principle of least privilege by explicitly setting top-level permissions.
Please consider adding a permissions block at the top of the file (before the
on:orjobs:keys):permissions: contents: read🤖 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 @.github/workflows/ci.yml around lines 5 - 7, Add a top-level permissions block in the workflow configuration before the on or jobs keys, granting only contents: read by default. Preserve the existing pull_request branch configuration and other workflow behavior unchanged.Source: Linters/SAST tools
83-88: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the redundant wait step.
GitHub Actions natively pauses job execution until all defined service container health checks pass. Since the
postgresservice already has a comprehensivepg_isreadyhealth check configured (lines 41-45), this manualsleepand validation step is redundant and only slows down the pipeline.♻️ Proposed fix to remove redundant step
- - name: Wait for services - run: | - sleep 10 - pg_isready -h localhost -p 5432 -U streamforge -d streamforge - # Redis and RabbitMQ readiness verified by their health checks -🤖 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 @.github/workflows/ci.yml around lines 83 - 88, Remove the entire “Wait for services” step from the workflow, including the sleep and pg_isready commands; rely on the existing postgres service health check and the Redis/RabbitMQ health checks to gate job execution.
🤖 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 @.github/workflows/ci.yml:
- Around line 94-95: Update the “Run tests” CI step to pass the integration
build tag to the Go test command, ensuring integration tests are included while
preserving the existing race detection and coverage options.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 24-27: Update the golangci/golangci-lint-action configuration to
replace version: latest with a specific pinned golangci-lint release, such as
v1.59.1, while preserving the existing action and timeout settings.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 5-7: Add a top-level permissions block in the workflow
configuration before the on or jobs keys, granting only contents: read by
default. Preserve the existing pull_request branch configuration and other
workflow behavior unchanged.
- Around line 83-88: Remove the entire “Wait for services” step from the
workflow, including the sleep and pg_isready commands; rely on the existing
postgres service health check and the Redis/RabbitMQ health checks to gate job
execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| - name: Run tests | ||
| run: go test -v -race -coverprofile=coverage.out -covermode=atomic ./... |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run integration tests.
The CI test step does not run integration tests, despite .golangci.yml actively linting the integration build tag. Without -tags integration, any integration tests will be silently skipped during execution.
🛠️ Proposed fix to include integration tests
- name: Run tests
- run: go test -v -race -coverprofile=coverage.out -covermode=atomic ./...
+ run: go test -tags integration -v -race -coverprofile=coverage.out -covermode=atomic ./...📝 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.
| - name: Run tests | |
| run: go test -v -race -coverprofile=coverage.out -covermode=atomic ./... | |
| - name: Run tests | |
| run: go test -tags integration -v -race -coverprofile=coverage.out -covermode=atomic ./... |
🤖 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 @.github/workflows/ci.yml around lines 94 - 95, Update the “Run tests” CI
step to pass the integration build tag to the Go test command, ensuring
integration tests are included while preserving the existing race detection and
coverage options.
1 participant
Summary
This PR adds the testing and continuous integration foundation for StreamForge.
What's included
Add unit tests for core packages:
Add GitHub Actions CI workflow
Enable automated linting, testing, and build validation
Add Makefile for common development tasks
Configure golangci-lint
Update Go module dependencies for testing
Validation
The following commands complete successfully:
Notes
This PR focuses only on improving code quality and developer tooling. No production features or API behavior have been changed.
Closes Phase 2 of the StreamForge roadmap.
Summary by CodeRabbit