feat: add docker support#848
Merged
Merged
Conversation
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
xkonjin
reviewed
Apr 15, 2026
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
General
Solid addition — Dockerizing the web UI is a clear win for self-hosting, and the multi-platform GHCR workflow is well thought out.
Security
- COOP/COEP headers: Good catch on
Cross-Origin-Opener-Policy: same-originandCross-Origin-Embedder-Policy: require-corpfor WASM SharedArrayBuffer. Those are easy to miss when moving off Vite. - Path traversal in
docker-server.mjs: TheresolvePathguard (candidate.startsWith(root)) is correct, but consider also rejecting null bytes in the path to defensively harden the check. - Cache-Control: The
immutablepolicy on/assets/is appropriate for Vite-hashed builds.
Bugs / Robustness
- Missing error handler on
createReadStream: Indocker-server.mjs,createReadStream(filePath).pipe(res)can throw if the file is truncated or unreadable mid-flight. Pipe errors are swallowed by default. Add:const stream = createReadStream(filePath); stream.on('error', (err) => { res.destroy(); }); stream.pipe(res);
existsSyncafterstatrace condition: Youstatthe path, then later callexistsSync(filePath). Between those two calls the file could be removed. Prefer reusing thestatresult or handlingENOENTon the read stream directly.
Testing
- No automated tests for
docker-server.mjs. Given it's a security-facing boundary, consider a lightweight integration test that hits the server with:- A valid asset
- An SPA fallback route
- A path traversal attempt (
../../../etc/passwd) - A request for a missing file
Minor
- Node version drift: Dockerfile uses
node:20-bookworm-slimbut the repo may have Node 22+ elsewhere. Worth pinning to the same major version used in CI to avoid subtle runtime differences.
Overall: LGTM with the stream error handler as a recommended follow-up.
Defensively harden the path traversal guard by returning null early when the URL contains a null byte, before normalization runs. Made-with: Cursor
|
Someone is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
added 3 commits
April 16, 2026 08:40
Attach an error listener before piping so mid-flight read errors (truncated file, permission change) cleanly destroy the response instead of being silently swallowed. Made-with: Cursor
Eliminates the TOCTOU race between the initial stat call and the subsequent existsSync check. Reuses the async stat pattern already in place and removes the now-unused existsSync import. Made-with: Cursor
Decode the URL before the null-byte check so percent-encoded null bytes (%00) are also rejected with 400 instead of falling through to the SPA fallback. Adds 5 node:test integration tests covering valid assets, SPA fallback, path traversal, null bytes, and 404. Made-with: Cursor
Contributor
CI Report❌ Some checks failed Pipeline Status
Test Results
✅ All 6509 tests passed 97 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
Made-with: Cursor
Collaborator
|
@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review. You are reviewing a PR in the GitNexus monorepo:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
|
Contributor
|
Claude finished @magyargergo's task in 5m 12s —— View job Production-Readiness Review — PR #848
|
| Check | Result |
|---|---|
| No secrets / tokens committed | ✓ .env.example contains only placeholders |
| No blind find-and-replace renames | ✓ Additive-only change |
| No HIGH/CRITICAL impact on shared symbols | ✓ New files only; vite.config.ts alias is additive |
| PR title follows conventional-commit format | ✓ feat: add docker support |
| PolyForm Noncommercial license untouched | ✓ |
| GUARDRAILS.md escalation triggers | None triggered |
What I could not fully verify
- Whether
docker-server.test.mjsruns in CI — the Vitest config files forgitnexus/andgitnexus-web/were not in the diff; I cannot confirm whether either picks up root-level.mjsfiles. - Whether the Prettier fix in
1019ee2fully resolves the format failure — CI has not re-run on HEAD. - Node version in CI setup-node step —
ci.ymlwas not modified in this PR and I didn't read the full setup-node configuration. - Whether
ghcr.io/abhigyanpatwari/gitnexusis the canonical upstream image location — depends on whether the fork and upstream share the same org name.
Final verdict
Blocking items (priority order):
- Trigger a new CI run to verify the Prettier fix in
1019ee2clears thequality / formatgate. No code change needed if the fix is correct — just re-run CI. - Confirm
docker-server.test.mjsruns in CI — if it doesn't, either move to Vitest or add an explicit CI step. Given that this file covers the path-traversal guard, silent test exclusion is not acceptable for a security boundary.
The resolvePath separator bug and the docker-compose.yaml namespace issue are worth addressing before landing but are not merge blockers given the Docker image's filesystem containment and the fact that IMAGE_NAME is overridable.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Collaborator
|
@BRAINIFII Can you please look at these findings? 🙏 |
Contributor
Author
Sure, i'll check it out |
added 2 commits
April 16, 2026 14:13
…mage namespace - Add `node --test docker-server.test.mjs` step to ci-tests.yml so the path-traversal guard tests run in every CI pass instead of being silently skipped. - Fix resolvePath containment check: `startsWith(root)` would allow sibling directories like `/app/dist-evil/`; now guards with `root + sep` or exact match. - Update docker-compose.yaml default image from `abhigyanpatwari` namespace to `brainifii` to match what docker.yml publishes to GHCR.
- Modify Dockerfile and Dockerfile.test to include options for apt-get to bypass validity checks during updates. - Set ownership of the /app directory to the 'node' user in the runtime stage for improved security and proper permission handling.
magyargergo
requested changes
Apr 16, 2026
- Update Dockerfile to use Alpine-based Node.js images for both builder and runtime stages, reducing image size and improving performance. - Replace apt-get commands with apk for package installation in the runtime stage.
magyargergo
requested changes
Apr 16, 2026
- Change base image from node:20-alpine to node:22-alpine
Collaborator
|
@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review. You are reviewing a PR in the GitNexus monorepo:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
|
Contributor
|
Claude finished @magyargergo's task in 2m 2s —— View job Production-Readiness Review — PR #848
|
| Check | Result |
|---|---|
| No secrets / tokens committed | ✓ .env.example uses real image references (not credentials); no API keys or tokens |
| No blind find-and-replace renames | ✓ Additive-only |
| No HIGH/CRITICAL impact on shared symbols | ✓ New files only; vite.config.ts alias is additive |
| PR title follows conventional-commit format | ✓ feat: add docker support |
| PolyForm Noncommercial license untouched | ✓ |
| Concurrency convention per CONTRIBUTING.md | ✓ cancel-in-progress: ${{ github.ref == 'refs/heads/main' }} is event-aware and correct |
What I could not fully verify
- Whether CI currently passes on HEAD — the latest commit fixes Prettier; CI has not re-run during this review. Based on the fix commit message and the clean implementation, Prettier should now pass.
- Whether
docker.ymlhas been manually dispatched and successfully built on this branch's current HEAD — the PR body claims a successful manual dispatch, but Node version mismatch was introduced after that test.
Final verdict
Blocking items (priority order):
- Fix runtime stage Node version (
Dockerfile:21): changenode:20-alpine→node:22-alpineto match the builder stage and fulfill the explicitly requested change. Fix this →
All other items from the previous review cycle have been resolved:
- ✅ Prettier format issue: addressed by style fix commit
- ✅ Test discoverability:
ci-tests.ymlnow explicitly runsnode --test docker-server.test.mjs - ✅ Path traversal separator bug: fixed (
root + sep) - ✅ Missing stream error handler: fixed (
stream.on('error', () => res.destroy()))
Collaborator
|
@BRAINIFII can you please resolve the node version conflicts between stages in docker? |
Contributor
Author
Oops missed it, on it |
Collaborator
|
Amazing work! Once the CI is green I'll merge your changes! Thank you for your contribution! 🚀 |
magyargergo
approved these changes
Apr 18, 2026
Collaborator
|
Ok, the e2e tests failures are not releated to your changes! I'll look into them. |
magyargergo
added a commit
that referenced
this pull request
Apr 18, 2026
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, #877 #878 #890) - AST-aware embedding chunking (#889) - jQuery / axios HTTP consumer detection (#887) - SemanticModel wired as first-class resolution input, SM-20 (#885) - ImportSemantics split into per-strategy hooks (#886) - Python dotted-import fix (#899); worker warnings non-terminal (#900 / #261); global-install ENOTEMPTY fixes (#843 #846); embeddings staleness fix (#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
magyargergo
added a commit
that referenced
this pull request
Apr 18, 2026
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, #877 #878 #890) - AST-aware embedding chunking (#889) - jQuery / axios HTTP consumer detection (#887) - SemanticModel wired as first-class resolution input, SM-20 (#885) - ImportSemantics split into per-strategy hooks (#886) - Python dotted-import fix (#899); worker warnings non-terminal (#900 / #261); global-install ENOTEMPTY fixes (#843 #846); embeddings staleness fix (#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
github714801013
pushed a commit
to github714801013/GitNexus
that referenced
this pull request
Apr 28, 2026
* feat: add docker support * feat: move docker files to root * feat: add docker build and push workflow * fix: pin docker action SHAs to verified commits Made-with: Cursor * fix: remove redundant --platform=$TARGETPLATFORM from runtime stage Made-with: Cursor * fix: upgrade docker actions to Node.js 24-compatible versions Made-with: Cursor * docs: updated readme * fix: update docker references * fix(docker-server): reject null bytes in resolvePath Defensively harden the path traversal guard by returning null early when the URL contains a null byte, before normalization runs. Made-with: Cursor * fix(docker-server): handle createReadStream errors Attach an error listener before piping so mid-flight read errors (truncated file, permission change) cleanly destroy the response instead of being silently swallowed. Made-with: Cursor * fix(docker-server): replace existsSync with async stat Eliminates the TOCTOU race between the initial stat call and the subsequent existsSync check. Reuses the async stat pattern already in place and removes the now-unused existsSync import. Made-with: Cursor * test(docker-server): add integration tests; fix %00 null-byte bypass Decode the URL before the null-byte check so percent-encoded null bytes (%00) are also rejected with 400 instead of falling through to the SPA fallback. Adds 5 node:test integration tests covering valid assets, SPA fallback, path traversal, null bytes, and 404. Made-with: Cursor * style: fix prettier formatting in docker-server files Made-with: Cursor * fix(docker): wire tests into CI, fix resolvePath separator, correct image namespace - Add `node --test docker-server.test.mjs` step to ci-tests.yml so the path-traversal guard tests run in every CI pass instead of being silently skipped. - Fix resolvePath containment check: `startsWith(root)` would allow sibling directories like `/app/dist-evil/`; now guards with `root + sep` or exact match. - Update docker-compose.yaml default image from `abhigyanpatwari` namespace to `brainifii` to match what docker.yml publishes to GHCR. * fix(docker): update apt-get commands and set user permissions - Modify Dockerfile and Dockerfile.test to include options for apt-get to bypass validity checks during updates. - Set ownership of the /app directory to the 'node' user in the runtime stage for improved security and proper permission handling. * fix(docker): switch to Alpine base images for smaller footprint - Update Dockerfile to use Alpine-based Node.js images for both builder and runtime stages, reducing image size and improving performance. - Replace apt-get commands with apk for package installation in the runtime stage. * fix(docker): update Node.js version in Dockerfile - Change base image from node:20-alpine to node:22-alpine * fix(docker): update Node.js version in Dockerfile to 22-alpine for runtime --------- Co-authored-by: kritik.b <kritik.b@media.net>
github714801013
pushed a commit
to github714801013/GitNexus
that referenced
this pull request
Apr 28, 2026
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (abhigyanpatwari#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, abhigyanpatwari#877 abhigyanpatwari#878 abhigyanpatwari#890) - AST-aware embedding chunking (abhigyanpatwari#889) - jQuery / axios HTTP consumer detection (abhigyanpatwari#887) - SemanticModel wired as first-class resolution input, SM-20 (abhigyanpatwari#885) - ImportSemantics split into per-strategy hooks (abhigyanpatwari#886) - Python dotted-import fix (abhigyanpatwari#899); worker warnings non-terminal (abhigyanpatwari#900 / abhigyanpatwari#261); global-install ENOTEMPTY fixes (abhigyanpatwari#843 abhigyanpatwari#846); embeddings staleness fix (abhigyanpatwari#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
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
3 participants
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.
Summary
Closes #485
Adds Docker support for the GitNexus web UI: a multi-platform
Dockerfile, adocker-compose.yaml, a lightweight production static file server, and a GitHub Actions workflow that automatically builds and pushes to GHCR on every version tag andmainpush.Motivation / context
Users asked for a way to self-host the GitNexus web UI without running the Vite dev server. Docker provides a portable, reproducible deployment that anyone can run with a single command.
Areas touched
gitnexus/(CLI / core / MCP server)gitnexus-web/(Vite / React UI).github/(workflows, actions)eval/or other toolingAGENTS.md,CLAUDE.md,.cursor/,llms.txt, etc.)Scope & constraints
In scope
linux/amd64,linux/arm64) Docker image for the web UIdocker-server.mjs— minimal Node.js static file server with SPA fallback,Cache-Control, and required COOP/COEP headers for WASMSharedArrayBufferdocker-compose.yamlwith healthcheck, configurable image/container name/port via env vars.env.exampleand.dockerignoredocker.ymlGitHub Actions workflow — builds and pushes to GHCR onv*tags andmain, with GHA layer cachingExplicitly out of scope / not done here
gitnexus serve)Implementation notes
node:20-bookworm-slimwithcurlinstalled for healthcheck supportCross-Origin-Opener-Policy: same-origin,Cross-Origin-Embedder-Policy: require-corp) are set on every response — required for WASMSharedArrayBufferwhich Vite dev server adds automatically but a plain static server does notghcr.io/<owner>/gitnexusand authenticated with the built-inGITHUB_TOKEN— no extra secrets requiredTesting & verification
cd gitnexus && npm testcd gitnexus && npm run test:integration(if core/indexing/MCP paths changed)cd gitnexus && npx tsc --noEmitcd gitnexus-web && npm test(if web changed)cd gitnexus-web && npx tsc -b --noEmit(if web changed)1m 51s,linux/amd64+linux/arm64)docker run --rm -p 4173:4173 ghcr.io/<fork>/gitnexus:<sha>— web UI loads correctlyRisk & rollout
docker pullto pick up the new COOP/COEP headers if they were using an older buildChecklist
AGENTS.md/ overlays changed: headers, scope block, and changelog updated per project conventions