Skip to content

feat: enforce unified security mode across Web, CLI, and Wiki with test hardening#539

Closed
Pan311 wants to merge 3 commits into
abhigyanpatwari:mainfrom
Pan311:main
Closed

feat: enforce unified security mode across Web, CLI, and Wiki with test hardening#539
Pan311 wants to merge 3 commits into
abhigyanpatwari:mainfrom
Pan311:main

Conversation

@Pan311

@Pan311 Pan311 commented Mar 27, 2026

Copy link
Copy Markdown
  • add centralized security-mode configuration for runtime decisions

  • align proxy, LLM settings, clone flow, and API/serve/wiki paths under the same security policy

  • reduce misconfiguration risk by making security behavior consistent across entry points

Summary

This PR enforces a unified security-mode policy across Web and CLI/server/wiki paths, and adds test hardening for local-only provider behavior and integration DB isolation. Together, this reduces config drift and improves test reliability for security-sensitive paths.

Motivation / context

Runtime security behavior previously varied by entry point, increasing misconfiguration risk. This change makes policy handling consistent across key flows and aligns tests with the intended local-only constraints and shared DB lifecycle behavior.

Areas touched

  • gitnexus/ (CLI / core / MCP server)
  • gitnexus-web/ (Vite / React UI)
  • .github/ (workflows, actions)
  • eval/ or other tooling
  • Docs / agent config only (AGENTS.md, CLAUDE.md, .cursor/, llms.txt, etc.)

Scope & constraints

In scope

  • Centralize and enforce security-mode behavior.
  • Align proxy/LLM/clone/API/serve/wiki logic to one security policy.
  • Fix web unit test expectation for local-only OpenAI config.
  • Isolate integration test DB paths to avoid shared-file lock collisions.

Explicitly out of scope / not done here

  • No auth/provider redesign.
  • No CI workflow changes.
  • No eval harness updates.
  • No feature/UI expansion outside security-policy alignment.

Implementation notes

  • Web test now uses a loopback OpenAI base URL under local-only mode.
  • withTestLbugDB now copies the shared DB template into per-suite temp paths, reducing cross-suite lock contention.
  • Commit validated: 5b5f742.

Testing & verification

  • cd gitnexus && npm test
    Result: tests passed (111 files passed, 3995 tests passed, 2 skipped), but Vitest still reports Errors 1 with unhandled worker exit.
  • cd gitnexus && npm run test:integration (if core/indexing/MCP paths changed)
    Result: integration files passed (45 files passed, 1931 tests passed), but same Vitest unhandled worker-exit error remains.
  • cd gitnexus && npx tsc --noEmit
  • cd gitnexus-web && npm test (if web changed)
    Result: passed (11/11 files, 198/198 tests).
  • cd gitnexus-web && npx tsc -b --noEmit (if web changed)
  • Manual / Playwright E2E (not run in this pass)

Risk & rollout

Medium risk due to cross-path security behavior alignment, with low code churn in test hardening changes.
No intended API-breaking change.
Known residual issue: Vitest pool intermittently reports an unhandled worker-exit error despite passing assertions; worth follow-up in test infra.

Checklist

  • PR body meets repo minimum length (workflow may label short descriptions)
  • If AGENTS.md / overlays changed: headers, scope block, and changelog updated per project conventions
  • No secrets, tokens, or machine-specific paths committed

@vercel

vercel Bot commented Mar 27, 2026

Copy link
Copy Markdown

@phatphamhong-tech is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

@magyargergo

Copy link
Copy Markdown
Collaborator

⚠️ Upcoming Prettier formatting — rebase instructions

PR #563 adds Prettier as the code formatter for the repo. When it merges, the bulk format commit will touch ~350 files (style-only: whitespace, quotes, trailing commas). Your branch will likely conflict.

After #563 merges, rebase your branch:

git fetch origin
git checkout <your-branch>
git rebase origin/main

# Conflicts will be formatting-only — accept your version:
git checkout --theirs .
git add .
git rebase --continue

# Then re-format your branch to match the new style:
npx prettier --write .
git add -A
git commit -m "style: apply prettier formatting"
git push --force-with-lease

New setup step: Run npm install at the repo root (not just in gitnexus/) to get prettier + activate the pre-commit hook. The hook auto-formats staged files on every commit going forward.

@Pan311 Pan311 force-pushed the main branch 2 times, most recently from 23b14a7 to 37899f9 Compare March 29, 2026 15:22
@magyargergo

Copy link
Copy Markdown
Collaborator

@claude is it ready to be merged?

@github-actions

github-actions Bot commented Mar 29, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 1m 42s —— View job


PR Merge Readiness Assessment

  • Gather context and understand the request
  • Review changed files
  • Assess test results and CI status
  • Check for blocking issues
  • Provide final verdict

Verdict: Not quite ready — two items need attention before merging

The implementation is solid and well-thought-out, but there are two blockers and several minor issues worth addressing.


Blockers

1. Pending Prettier formatting PR (#563)

The comment from @magyargergo warns that PR #563 adds Prettier as the repo formatter and will touch ~350 files. This branch will conflict when that merges. The rebase instructions are already in the comments above — this branch should be rebased and reformatted after #563 lands before this PR is merged.

2. Breaking behavior change: isLocalOnlyMode() defaults to true when env var is absent

The isLocalOnlyEnabled helper in all three CLI/server locations (and the web security-mode.ts) uses this logic:

value === undefined || value === '' || (value !== '0' && value !== 'false')

This means when GITNEXUS_LOCAL_ONLY is not set at all, isLocalOnlyMode() returns true. Any user who was previously running the wiki command, serve, or web UI against an external LLM/provider without this env var set will find their provider suddenly blocked.

This is a significant behavioral change and should be explicitly confirmed as intentional. If it is intentional, it needs a changelog/migration note. If it's not intentional (i.e., the default should be false for unset), the logic needs to be value !== undefined && value !== '' && value !== '0' && value !== 'false'.

Fix this →


Non-blocking issues

3. Code duplication of isLocalOnlyEnabled

The same helper is copy-pasted in three CLI/server files:

  • gitnexus/src/server/api.ts:26-28
  • gitnexus/src/cli/serve.ts:3-5
  • gitnexus/src/core/wiki/llm-client.ts:32-34

The web side nicely centralizes this in security-mode.ts. The CLI side should do the same — a shared src/config/security-mode.ts (or similar) would avoid drift if the logic ever needs to change.

Fix this →

4. Double local-only enforcement in wiki.ts

The local-only enforcement block appears at lines 223-235 and again at lines 447-459 in gitnexus/src/cli/wiki.ts. The second check after interactive setup is intentional (to catch user selections that violate policy), but the duplication increases maintenance surface.

5. Residual Vitest worker-exit error

Noted in the PR body — Errors 1 with unhandled worker exit persists despite passing assertions. This is pre-existing and not introduced here, but worth a follow-up issue.

6. Vercel deployment authorization pending

The Vercel bot requires a team member to authorize the deployment. This is a deployment concern, not a code concern, but should be handled before the PR is considered fully validated.


What's done well

  • The proxy (api/proxy.ts) has solid security: CORS allowlist, HTTPS-only enforcement, host allowlist, stripped www-authenticate header, and no forwarding of auth headers to non-core GitHub hosts.
  • withTestLbugDB isolation fix (copying shared DB template per suite) is a clean solution to the cross-worker file lock issue.
  • The web test fix (baseUrl: 'http://127.0.0.1:11434/v1') correctly aligns the test with local-only mode semantics.
  • Security logic is now consistent across Web, CLI, and Wiki — the core goal of this PR.

TL;DR: Rebase after #563 merges, confirm/fix the default-on local-only behavior, then this is ready.

@magyargergo

Copy link
Copy Markdown
Collaborator

Please submit a new PR if this is still relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants