Skip to content

feat: ship secure web workbench - #18

Merged
joungminsung merged 2 commits into
mainfrom
agent/fix-review-findings
Jul 9, 2026
Merged

joungminsung merged 2 commits into
mainfrom
agent/fix-review-findings

Conversation

@joungminsung

@joungminsung joungminsung commented Jul 9, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • ship the simplified multilingual web workbench and local Docker demo
  • align supported model, parser, and connector plugins across CLI, server, docs, and tests
  • address all findings from the final review: health checks, proxy trust, archive limits, token refresh, stale search results, collection write scopes, and release metadata
  • update GitHub Actions to supported Node 24-based action runtimes

Review fixes

  • add an unauthenticated minimal /healthz liveness endpoint and update Docker probes
  • trust forwarded IP/protocol headers only when the direct peer matches configured proxy IPs or CIDRs
  • bound ZIP entry count, per-entry size, total output, and compression ratio
  • refresh Google Drive service-account tokens before expiry and retry once after a 401
  • exclude stale chunks whenever their source document is no longer indexed
  • require document:write for collection mutation routes
  • add a Changeset covering all affected public packages

Validation

  • local npm run build — 26/26 tasks passed
  • local npm run typecheck — 29/29 tasks passed
  • local npm run lint — 29/29 tasks passed
  • local npm run test — 51/51 tasks passed
    • core: 50 files, 424 tests
    • server: 13 files, 63 tests
  • both Docker Compose configurations validated
  • npx changeset status passed
  • git diff --check passed
  • GitHub Actions CI passed on Node.js 20 and 22 for head SHA 20ff97a

@joungminsung
joungminsung marked this pull request as ready for review July 9, 2026 23:49
@joungminsung
joungminsung merged commit 6358786 into main Jul 9, 2026
2 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20ff97afbf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/web/src/App.tsx
}

setAuthState('authenticated')
setAuthState('unavailable')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Route auth failures to the login page

In team mode, an unauthenticated /api/v1/health response is { error: 'API key required...' }, and request() throws that body text rather than HTTP 401, so the regex above does not match. With this new fallback, first-time team-mode users are sent to the “server unavailable” screen instead of LoginPage, leaving no way to enter an API key from the web UI.

Useful? React with 👍 / 👎.

Comment thread docker-compose.demo.yml
- "${OPENDOCUMENTS_PORT:-3333}:3000"
volumes:
- opendocuments-demo-data:/data
- ./docker/demo/opendocuments.config.ts:/app/opendocuments.config.ts:ro

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Commit the demo config before mounting it

The documented demo command mounts ./docker/demo/opendocuments.config.ts, but that file is not present in docker/demo or elsewhere in the repo (rg opendocuments.config only finds this mount/docs). A fresh docker compose -f docker-compose.demo.yml up --build will therefore fail the bind mount or create a directory at /app/opendocuments.config.ts, after which config loading fails, making the new demo compose file unusable as checked in.

Useful? React with 👍 / 👎.

Comment on lines +104 to 105
if (!options.force && !store.hasContentChanged(existing.id, contentHash)) {
return { documentId: existing.id, chunks: existing.chunk_count ?? 0, status: 'skipped' }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid skipping errored forced reindexes

When --reindex now calls ingest(..., { force: true }) on unchanged content and that reindex fails before updateContentHash, the old content_hash is left in place while the row is marked error. A later normal indexing run of the same file hits this skip path because the hash still matches, so the errored document is never restored to indexed and remains filtered out of search results.

Useful? React with 👍 / 👎.

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.

1 participant