neureka/arcane-test

Fork 0

forked from arcane/arcane

Update docker/Dockerfile #1

Open

neureka wants to merge 2 commits from test into main

neureka commented

2026-06-12 05:58:17 +00:00

Owner

Codex Bot Review

Status: Request changes
Risk: High
Reviewed: 65a22acada360a9c4b718d85792b60ca9d3f087d...a871d9b2552cdc5147c5a30247d74bad3f02ca47
Trigger: @mention
Updated: 2026-06-13T00:16:22.7717142+00:00

Summary

The PR removes the manual next-image build workflow and changes the manager image default backend port. The review focused on Docker packaging behavior and release workflow impact. The Dockerfile port change appears to break the documented/container default for the manager service.

Walkthrough

Area	What changed	Review notes
`.github/workflows/build-next-images.yml`	Removed the manual workflow for building and signing next images.	No specific defect found from the diff alone, but this removes an existing release/publishing path.
`docker/Dockerfile`	Changed the runtime `PORT` environment variable from `3552` to `35522`.	High risk because the packaged manager container is expected to listen on port `3552`.

Findings

CR-001 — HIGH — Manager image now listens on the wrong default port

Category: compatibility
Location: docker/Dockerfile:86

Issue:
The manager image default PORT was changed from 3552 to 35522. Arcane’s documented backend/container port is 3552, and existing Docker deployments commonly map host/container port 3552. With this change, the application will bind to 35522 inside the container while users and compose examples continue to target 3552.

Why it matters:
This can make newly built manager images unreachable after upgrade, because -p 3552:3552 or equivalent compose mappings route traffic to a container port where the server no longer listens. That is a production compatibility break for the primary Docker artifact.

Suggested fix:
Restore ENV PORT=3552 unless the port migration is intentional. If it is intentional, update all Docker exposure, compose examples, docs, health checks, and migration notes in the same change so existing users have a clear upgrade path.

LLM fix prompt

You are fixing Codex Bot review finding CR-001.

Repository: current review checkout
PR: current pull request
Severity: HIGH
Category: compatibility
Location: docker/Dockerfile:86

Issue:
The manager Docker image default PORT was changed from 3552 to 35522, which breaks existing deployments and documented container port mappings that target 3552.

Why it matters:
New images can become unreachable when users map 3552:3552 because the server listens on 35522 inside the container instead of 3552.

Required change:
Restore the Dockerfile runtime PORT to 3552, or if the port change is intentional, update all related Docker exposure, compose examples, docs, health checks, and migration guidance in the same PR.

Constraints:
- Fix only this finding.
- Preserve existing style, architecture, and public API behavior unless the finding requires an API change.
- Do not perform unrelated refactors.
- Do not hide errors; handle them explicitly.
- Add or update tests only for the behavior needed to prove this fix.
- Do not update generated files, snapshots, lockfiles, or migrations unless directly required.

Acceptance criteria:
- The manager image listens on the documented/default port 3552, or every deployment-facing reference is consistently migrated to the new port.
- Validation confirms a built manager container is reachable through the documented port mapping.

Suggested change

diff --git a/docker/Dockerfile b/docker/Dockerfile
index af37dae1c..5c18f04e6 100644
--- a/docker/Dockerfile
+++ b/docker/Dockerfile
@@ -82,7 +82,7 @@ ARG VERSION
 ARG REVISION
 
 ENV GIN_MODE=release
-ENV PORT=35522
+ENV PORT=3552
 ENV ENVIRONMENT=production
 ENV NVIDIA_VISIBLE_DEVICES=all \
     NVIDIA_DRIVER_CAPABILITIES=compute,utility \

Confidence Score: 2/5

The PR needs rework before merge because the Dockerfile change affects the primary runtime artifact and is likely to break existing container deployments. The workflow deletion may be intentional, but the port change is not safely coordinated with the rest of the deployment surface.

Suggested validation

Build the manager image and verify it is reachable through the documented 3552:3552 container port mapping.

Not reviewed

Target repository tests/builds were not run by Codex Bot.
Runtime validation and release workflow execution were not performed in this read-only review.

_{Reviews (10): Last reviewed commit: Delete .github/workflows/build-next-images.yml}

_{Generated by Codex Bot. This block is replaced on each review run.}

## Codex Bot Review **Status:** Request changes **Risk:** High **Reviewed:** `65a22acada360a9c4b718d85792b60ca9d3f087d...a871d9b2552cdc5147c5a30247d74bad3f02ca47` **Trigger:** `@mention` **Updated:** `2026-06-13T00:16:22.7717142+00:00` ### Summary The PR removes the manual next-image build workflow and changes the manager image default backend port. The review focused on Docker packaging behavior and release workflow impact. The Dockerfile port change appears to break the documented/container default for the manager service. ### Walkthrough | Area | What changed | Review notes | | --- | --- | --- | | `.github/workflows/build-next-images.yml` | Removed the manual workflow for building and signing next images. | No specific defect found from the diff alone, but this removes an existing release/publishing path. | | `docker/Dockerfile` | Changed the runtime `PORT` environment variable from `3552` to `35522`. | High risk because the packaged manager container is expected to listen on port `3552`. | ### Findings #### CR-001 — HIGH — Manager image now listens on the wrong default port **Category:** compatibility **Location:** `docker/Dockerfile:86` **Issue:** The manager image default `PORT` was changed from `3552` to `35522`. Arcane’s documented backend/container port is `3552`, and existing Docker deployments commonly map host/container port `3552`. With this change, the application will bind to `35522` inside the container while users and compose examples continue to target `3552`. **Why it matters:** This can make newly built manager images unreachable after upgrade, because `-p 3552:3552` or equivalent compose mappings route traffic to a container port where the server no longer listens. That is a production compatibility break for the primary Docker artifact. **Suggested fix:** Restore `ENV PORT=3552` unless the port migration is intentional. If it is intentional, update all Docker exposure, compose examples, docs, health checks, and migration notes in the same change so existing users have a clear upgrade path. <details> <summary>LLM fix prompt</summary> ```text You are fixing Codex Bot review finding CR-001. Repository: current review checkout PR: current pull request Severity: HIGH Category: compatibility Location: docker/Dockerfile:86 Issue: The manager Docker image default PORT was changed from 3552 to 35522, which breaks existing deployments and documented container port mappings that target 3552. Why it matters: New images can become unreachable when users map 3552:3552 because the server listens on 35522 inside the container instead of 3552. Required change: Restore the Dockerfile runtime PORT to 3552, or if the port change is intentional, update all related Docker exposure, compose examples, docs, health checks, and migration guidance in the same PR. Constraints: - Fix only this finding. - Preserve existing style, architecture, and public API behavior unless the finding requires an API change. - Do not perform unrelated refactors. - Do not hide errors; handle them explicitly. - Add or update tests only for the behavior needed to prove this fix. - Do not update generated files, snapshots, lockfiles, or migrations unless directly required. Acceptance criteria: - The manager image listens on the documented/default port 3552, or every deployment-facing reference is consistently migrated to the new port. - Validation confirms a built manager container is reachable through the documented port mapping. ``` </details> <details> <summary>Suggested change</summary> ```diff diff --git a/docker/Dockerfile b/docker/Dockerfile index af37dae1c..5c18f04e6 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -82,7 +82,7 @@ ARG VERSION ARG REVISION ENV GIN_MODE=release -ENV PORT=35522 +ENV PORT=3552 ENV ENVIRONMENT=production ENV NVIDIA_VISIBLE_DEVICES=all \ NVIDIA_DRIVER_CAPABILITIES=compute,utility \ ``` </details> ### Confidence Score: 2/5 The PR needs rework before merge because the Dockerfile change affects the primary runtime artifact and is likely to break existing container deployments. The workflow deletion may be intentional, but the port change is not safely coordinated with the rest of the deployment surface. ### Suggested validation - Build the manager image and verify it is reachable through the documented `3552:3552` container port mapping. ### Not reviewed - Target repository tests/builds were not run by Codex Bot. - Runtime validation and release workflow execution were not performed in this read-only review. Reviews (10): Last reviewed commit: [Delete .github/workflows/build-next-images.yml](https://code.neureka.dev/neureka/arcane-test/commit/a871d9b2552cdc5147c5a30247d74bad3f02ca47) Generated by Codex Bot. This block is replaced on each review run.

✅ 1

neureka added 1 commit

2026-06-12 05:58:17 +00:00

Update docker/Dockerfile 6a2f9c517a

neureka added 1 commit

2026-06-13 00:16:04 +00:00

Delete .github/workflows/build-next-images.yml a871d9b255

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin test:test

git switch test

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff test

git switch test

git rebase main

git switch main

git merge --ff-only test

git switch test

git rebase main

git switch main

git merge --no-ff test

git switch main

git merge --squash test

git switch main

git merge --ff-only test

git switch main

git merge test

git push origin main

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

neureka/arcane-test!1

No description provided.

Rows
Columns