One-step desktop setup: python oc-desktop.py self-bootstraps everything#2137
One-step desktop setup: python oc-desktop.py self-bootstraps everything#2137JSv4 wants to merge 39 commits into
Conversation
…Redis process launcher
Collapse the local.yml compose topology toward a single-user desktop app that
runs as plain processes with no Docker and no Redis:
- config/settings/desktop.py: no-Redis profile (LocMem cache, in-process
channels, filesystem Celery broker + SQLAlchemy DB result backend, local
per-user storage, Auth0 off) that serves the built SPA from Daphne and
selects in-process pipeline components.
- opencontractserver/desktop/{paths,launcher}.py + oc-desktop.py: one-command
supervisor - embedded PostgreSQL+pgvector (pgserver) or external DATABASE_URL,
migrate, first-run bootstrap, Daphne + Celery worker/beat, clean shutdown.
- opencontractserver/pipeline/parsers/warp_ingest_parser.py: in-process,
rule-based PDF parser (Warp-Ingest) replacing the Docling microservice. Lazy
optional import; emits PAWLS tokens + structural annotations + OC_PARENT_CHILD
hierarchy directly as an OpenContractDocExport.
- desktop_bootstrap management command: local superuser, PipelineSettings
seeding, nltk corpora.
- config/spa.py + gated config/urls.py catch-all: serve the SPA from Daphne.
- requirements/desktop.txt, docs/deployment/desktop_packaging.md, changelog.
Known Phase-0 limitation (documented): notification WebSocket delivery needs a
loop-safe transport under the in-process channel layer; agent chat is
unaffected (streams inline). See the docs for the phased plan.
Address CI + review feedback on the Phase 0 desktop packaging PR: Security (CodeQL, 2 high alerts): - Stop persisting the SECRET_KEY and the local login password to plaintext files. Secrets are now env-sourced only: the launcher generates one shared DJANGO_SECRET_KEY and exports it to every child process; without it an ephemeral in-process key is used (with a warning). The local user password comes only from OC_DESKTOP_PASSWORD; when unset the user is created with an unusable password and the operator sets one. Typing (mypy): - Replace the _shutdown()-in-lambda signal handlers with a named function. - Type the filesystem-broker transport options dict override. - Test fixes: setattr for injected module attrs, assert-narrow Optional returns before indexing, type: ignore for the sys.modules None sentinel. Coverage (codecov patch): - Add opencontractserver/tests/test_desktop_packaging.py covering the per-OS path helpers and the SPA catch-all traversal guard. - Omit the desktop launcher (a process-supervision entry point, like manage.py/wsgi.py) from coverage. Review nits: - Drop the unused REACT_APP_WS_ROOT_URL key from env-config generation. - Remove the unused WHITENOISE_ROOT/INDEX_FILE settings (the SPA catch-all serves assets + index.html). - Fix the misleading CELERY_BEAT_SCHEDULER comment (beat runs as a subprocess; APScheduler is a Phase-1 follow-up). - Surface migrate_pipeline_settings failures loudly instead of swallowing. - Note the _free_port() TOCTOU window.
- Fix /media/ 404s in the desktop build: set SERVE_MEDIA_WITHOUT_DEBUG=True so Django serves local media (uploaded PDFs, thumbnails) with DEBUG=False and no nginx/S3 in front. Without it every FieldFile.url would 404. - Correct the config/spa.py docstring: spa_fallback serves the SPA's assets + index.html itself; WhiteNoise only serves Django's STATIC_ROOT, not the dist/. - Drop the redundant set_unusable_password() in desktop_bootstrap (Django's create_superuser(password=None) already stores an unusable password). - Tighten the SPA catch-all regex to whole-segment exclusions so a future route like /mcp-guide still falls through to index.html. - Log a warning when collectstatic fails instead of silently ignoring it. - Doc note: an external DATABASE_URL must already have the pgvector extension (the embedded pgserver path creates it automatically).
The command no longer persists a generated password to disk; correct the module docstring accordingly and consolidate a doubled comment block in _seed_pipeline_settings left over from an earlier edit.
…ettings cache Addresses two real bugs found in review: - SECRET_KEY was regenerated every launch, so PipelineSettings' encrypted secrets (e.g. OPENAI_API_KEY) — derived from SECRET_KEY via Fernet/PBKDF2 — became permanently undecryptable on the next restart, silently breaking the Tier-1 'set your API key once' flow. The launcher now resolves a STABLE key from the OS keyring (Keychain/Credential Locker/Secret Service), with an ephemeral fallback + warning when no backend is available. No plaintext file (keeps the CodeQL secret-storage fix intact). Adds 'keyring' to desktop reqs. - PipelineSettings.get_instance() cached the singleton in process-local LocMemCache; with Daphne/worker/beat as separate processes, a settings change in Daphne never invalidated the worker's copy (stale parser/embedder/key for up to the TTL). Set PIPELINE_SETTINGS_CACHE_TTL_SECONDS=0 on desktop so reads are always fresh (cheap for a single user). Review nits: drop dead CELERY_TASK_EAGER_PROPAGATES; cap early-stage desktop deps (warp-ingest/pgserver/sqlalchemy) with upper bounds; cross-reference the deliberate CELERY_BROKER_TRANSPORT_OPTIONS reassignment in base.py; correct the config/urls.py SERVE_MEDIA_WITHOUT_DEBUG comment; add desktop_bootstrap unit tests (password set/unset/idempotent, pipeline seeding + failure path).
Return (command, stdout, stderr) StringIO streams from the helper rather than assigning self.out/self.err in a helper method, which keeps the test mypy-clean under django-stubs (no instance attribute defined outside __init__).
Fixes from review: - requirements/desktop.txt was missing a Postgres driver (base.txt has none; it lives in local/production.txt), so 'manage.py migrate' would fail on a clean desktop install. Add psycopg2-binary==2.9.12. - config.settings.desktop reassigned USE_AUTH0/STORAGE_BACKEND AFTER 'from .base import *', but base.py branches on both at its own import time (requiring AUTH0_*/GS_BUCKET_NAME with no defaults). A stray USE_AUTH0=true / STORAGE_BACKEND=GCP inherited from the process env would crash the import first. Force both to desktop-safe values via os.environ BEFORE the base import (hard set, since setdefault would leave an explicit hostile value in place). Drop the now-redundant later reassignments. Add a subprocess test that imports the profile under a hostile env. Cleanups: - Extract the Django->SQLAlchemy result-backend URL mapping to opencontractserver/desktop/db.py (pure, unit-tested: postgres scheme, query string preserved, non-postgres passthrough). - WarpIngestParser now marks ConnectionError/TimeoutError transient (retryable, e.g. a first-run OCR model download) instead of permanent; add a test. - launcher: single teardown path (atexit only; signal handlers just sys.exit) and read DATABASE_URL from the threaded env for consistency. - Note in the SPA catch-all that mcp/sse/ws exclusions are defensive-only (those routes are handled at the ASGI/protocol layer, never reaching http urlpatterns).
- WarpIngestParser: requests/httpx network errors do not subclass the builtin ConnectionError/TimeoutError, so classify transient by matching the exception class name across its MRO (no optional-lib import) in addition to the builtins — a flaky first-run OCR model fetch is now retryable, not permanent. Add a test for the httpx-style (non-builtin) case. - launcher: warn on the console when the server does not answer within 60s before opening the browser, instead of discarding _wait_for_http's result. - desktop settings: drop "0.0.0.0" from ALLOWED_HOSTS (a bind address, never an incoming Host header; Daphne binds 127.0.0.1).
…a dir Both from the review's non-blocking observations: - WarpIngestParser streams the stored PDF to the temp file via shutil.copyfileobj instead of read()-into-memory, so a very large PDF no longer briefly doubles memory usage. - launcher scopes the OS-keyring SECRET_KEY entry by a hash of OC_DESKTOP_DATA_DIR so two desktop instances under different data dirs (e.g. a test profile beside a real one) each keep their own persisted key instead of sharing one.
Review follow-ups (none blocking): - launcher: redirect each child's (Daphne/worker/beat) stdout+stderr to a per-child file under logs_dir() so crashes leave a durable traceback — needed once the Phase-2 Tauri shell runs with no attached console. logs_dir() was created but unused before. Close the handles on shutdown. - launcher: reserve the free port immediately before Daphne binds it (after the slow worker/beat spawn) to keep the release-then-rebind TOCTOU window near-instant; update the _free_port docstring accordingly. - Fix WhiteNoise/SPA doc drift left over from removing WHITENOISE_ROOT: the spa_fallback view serves the SPA assets itself (WhiteNoise covers only STATIC_ROOT). Corrected in config/spa.py, config/urls.py and the deployment doc's compose-replacement table. - Move the placeholder desktop DATABASE_URL into a named constant (db.DEFAULT_DESKTOP_DATABASE_URL) per the no-magic-values convention. - Docs: note OC_DESKTOP_PASSWORD is a superuser password and is not strength-validated on the desktop profile.
_spawn opened the per-child log file before subprocess.Popen; if Popen raised (e.g. a bad executable) the fd leaked. Spawn inside try/except, close the handle on failure and re-raise, and only track the handle/child after a successful spawn. Flagged by github-code-quality static analysis.
…heck fix Reliability fixes from review: - launcher._stable_secret_key runs the OS-keyring call in a daemon thread with a 10s timeout: a locked GNOME Keyring/KWallet can block on an interactive unlock prompt instead of raising, which would hang the launcher. On timeout or error, fall back to an ephemeral key with a clear warning. - First-run bootstrap is now retry-safe: desktop_bootstrap exits non-zero (CommandError) if pipeline seeding failed, and the launcher writes the .bootstrapped marker ONLY on a clean exit — so a transient first-run failure is retried on the next launch (every step is idempotent) instead of permanently disabling Tier-1 embeddings/chat. Add a handle()-raises test. - Health-check /api/health/ instead of / before opening the browser: when the SPA isn't built, / redirects to the absent :3000 dev server and every poll would error, falsely reporting the server down and burning the full 60s. - _resolve_spa_dir reads OC_DESKTOP_FRONTEND_DIR from the threaded env for consistency with the rest of the launcher. - Docs: note Windows Popen.terminate() is a hard kill (no graceful drain); reconcile_stuck_documents recovers stranded docs — graceful Windows teardown is a Phase-1 item.
Groundwork for reducing desktop setup to 'python oc-desktop.py': - desktop/bootstrap.py: stdlib-only Python-version gate (3.10-3.12; pgserver ships no 3.13 wheels), private venv under app-data, automatic requirements install with content-fingerprint refresh, re-exec in venv. - desktop/spa_dist.py: acquire the built SPA without a Node toolchain - repo dist -> cached download -> GitHub release asset (version-matched tag, then latest) -> yarn fallback; zip-slip-guarded extraction. - docker-build-release.yml: build-frontend-dist job attaches opencontracts-frontend-dist.zip to every release. - tests: BootstrapTests + SpaDistTests in test_desktop_packaging.py. Entry-point/launcher wiring lands in the follow-up commit (kept separate while a clean-room usability test of the current flow completes).
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| os.environ["USE_AUTH0"] = "False" | ||
| os.environ["STORAGE_BACKEND"] = "LOCAL" | ||
|
|
||
| from .base import * # noqa: E402,F401,F403 |
…n password prompt (part 2) Clean-room usability test findings (agent role-playing a non-technical user) and fixes: - BLOCKER: every fresh desktop install died at migration annotations/0074 because the embedded pgserver Postgres bundles no pg_trgm. The migration now probes pg_available_extensions via RunPython and skips the extension + GIN index when absent (runtime queries are plain icontains and work unindexed); real Postgres deployments get the identical extension + index. Reverse now drops only the index, removing the documented DROP EXTENSION hazard. - oc-desktop.py routes through desktop/bootstrap.py: python-version gate, private venv under app-data, automatic dependency install, re-exec. One command, no pip/venv/Node knowledge needed. - desktop_bootstrap: password now comes from OC_DESKTOP_PASSWORD or an interactive first-run prompt (min 8 chars, confirmed); a password-less account left over from a headless run self-heals on the next launch. - launcher: SPA auto-acquisition via spa_dist (release bundle download, yarn fallback); stable default port 8406 with ephemeral fallback; startup banner with URL, the previously-undocumented 'desktop' username, and Ctrl+C stop instructions. - Discoverability: README 'Desktop (no Docker)' Quick Start section, docs/quick_start.md cross-link, desktop doc rewritten around the one-command flow and added to the mkdocs nav. - Tests: 39 pass against a real embedded Postgres (full migration run), incl. new TrigramMigrationGuardTests and prompt/self-heal coverage. - Drop unused 'import sys' in spa_dist (review bot).
…e download An explicit CA override (corporate proxy, custom bundle) must win; certifi stays as the fallback for interpreters with no usable system roots.
…p-simplify-plgwgj # Conflicts: # opencontractserver/pipeline/parsers/warp_ingest_parser.py
These paths (venv creation, pip install, re-exec, release download, yarn build) are exercised end-to-end by the live desktop launch, not unit tests — same rationale as the existing launcher.py coverage omit.
…oke test without nltk corpora
ReviewWent through the full diff (settings profile, launcher, bootstrap, SPA acquisition, migration change, and the new parser/tests). This is a large but well-scoped PR, and it's clear a lot of iteration already went into hardening it (secret handling, zip-slip guard, path-traversal guard, hostile-env import test, retry-safe bootstrap). Nice test coverage overall ( CorrectnessCached SPA bundle is never version-checked, so a backend upgrade can silently keep serving a stale frontend.
def ensure_spa(repo_root: Path, version: str) -> Path | None:
repo_dist = repo_root / "frontend" / "dist"
if (repo_dist / "index.html").is_file():
return repo_dist
cached = _dist_dir_within(paths.subdir("spa"))
if cached:
return cached # <-- `version` is never consulted here
return download_spa(version) or build_spa_with_yarn(repo_root)
Given the backend/frontend GraphQL contract can change between releases, this could produce confusing runtime errors (missing fields, broken mutations) after what looks like a normal upgrade. Suggest stamping the extracted dist with the version it was built for (e.g. a small Minor / non-blocking
What looks solid
|
ReviewReviewed the full diff ( Strengths
Minor / non-blocking observations
Nothing above is blocking. The overall design (stdlib-only bootstrap before deps exist, private venv, defensive zip extraction, keyring-backed secret persistence, whole-segment route exclusion, idempotent/self-healing first-run bootstrap) is solid and the test suite backs up the claims made in the changelog fragments. |
Review: One-step desktop setup (
|
- spa_dist: stamp downloaded bundles with the checkout version and refresh on mismatch, so a backend upgrade can't silently keep serving a stale frontend; stale cache is still used (with a warning) when the refresh fails offline. Tests for match/refresh/offline-fallback. - bootstrap: venv-creation failure now gets its own actionable message (Debian/Ubuntu python3-venv hint) instead of pip-flavored guidance; comment documenting the deliberate absence of pip --upgrade. - warp_ingest_local_parser: warn when apply_ocr and disable_ocr are both set; cross-reference comment explaining why transient-error detection differs from the REST sibling. - docs: note not to double-launch during first-run install (no cross-process lock yet, Phase-1 item). - test_warp_ingest_parser: smoke test also requires punkt/punkt_tab corpora before running.
ReviewReviewed the full diff ( Correctness / risk items
Minor / style
Test coverageTest coverage for the pure/testable surfaces ( SummaryNo blocking issues found. The main things I'd want resolved or at least explicitly acknowledged before merge are (1) confirming the migration-rewrite is intentional and safe for any environment that already ran 0074, and (6) confirming the release workflow trigger actually populates |
Review: One-step desktop setup (
|
- docker-build-release.yml publishes opencontracts-frontend-dist.zip.sha256 alongside the bundle; download_spa verifies it before extracting and refuses a mismatched asset (older releases without the sibling fall back to TLS-only trust). Rate-limit hint added to the not-found message. - bootstrap: third Ctrl+C force-kills a hung child (escape hatch). - paths.subdir: created app-data dirs are user-private (0o700 on POSIX) - the tree holds the full local database and uploaded documents. - launcher._write_env_config: document the deliberate env-config.js rewrite side effect on repo-local dists. - desktop_bootstrap: correct the MIN_PASSWORD_LENGTH comment (validators are bypassed by create_superuser, not skipped by the profile). - migration 0074: docstring invariant note - state claims the trigram index exists even where the DB skipped it; future index-aware operations on raw_text must stay tolerant of its absence.
|
Consolidated response to the review rounds above — adopted items are in commits Adopted
Acknowledged, no change
Generated by Claude Code |
- nltk corpora download now prints a progress line instead of running silently (read as a hang). - keyring-fallback warning rewritten in plain language (what breaks and why), with the env-var fix labeled as the advanced option. - no-UI warning distinguishes offline from a pre-release checkout with no published bundle and gives the yarn recipe for the latter.
…wn comment - bootstrap: prompt/error strings precomputed outside the password flow so no print() sits on password-tainted data (CodeQL py/clear-text-logging-sensitive-data on line 80 was a taint-flow FP - the strings never contained the password - but the hoist makes that provable). - spa_dist: 500 MB safety cap on the bundle download (defense-in-depth against a compromised/oversized release asset). - launcher: explanatory comment in the deliberate empty except during teardown drain (quality bot).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
py/clear-text-logging-sensitive-data treats any /password/i identifier as sensitive data; MIN_PASSWORD_LENGTH interpolated into the printed prompt strings tripped it twice (alerts 86/87) despite no secret in the output. The rename makes the non-flow provable to the heuristic; comment documents the constraint for future editors.
Review: One-step desktop setup (
|
Review: One-step desktop setup (
|
- launcher.main: os.environ.pop(OC_DESKTOP_PASSWORD) after bootstrap - env.pop() only cleaned the dict handed to children, while the supervising process (set via bootstrap's early prompt on os.environ) kept the plaintext password in /proc/<pid>/environ for the whole session, contradicting the comment's guarantee. - _stable_secret_key docstring: document the accepted timed-out-thread persist race. - Tests for launcher's pure helpers (_free_port stable-default/fallback, _keyring_username determinism + data-dir scoping) despite the coverage omit. - Rename test_warp_ingest_parser.py -> test_warp_ingest_local_parser.py: it tests WarpIngestLocalParser; the REST client's tests live in test_doc_parser_warp_ingest.py (reviewer-flagged ambiguity). (psycopg2-binary==2.9.12 pin verified by three real installs in the live usability runs.)
Review: One-step desktop setup (
|
…rashing An empty/mangled .sha256 response (truncated connection, proxy interstitial) hit .split()[0] -> IndexError, which escaped download_spa's catch tuple and crashed the whole launcher on first run. Raise ValueError instead (already on the graceful-degrade path: refuse the bundle, fall back to cache/yarn/API-only). Test added for the malformed-response case.
Review: One-step desktop setup (
|
…ever tightens Reviewer-caught ordering bug: on a first run, 'python -m venv' is the first thing to create the app-data ROOT (with umask defaults, typically 0755), and every later mkdir(mode=0o700, exist_ok=True) silently no-ops on existing dirs - so the documented user-private invariant failed at the root and venv levels (sensitive leaves were still 0700). parents=True has the same gap for intermediate levels. - paths.ensure_private_dir: mkdir -p + explicit idempotent chmod 0o700 on the root and every level below it (suppressed on Windows); subdir(create=True) routes through it. - bootstrap pre-creates the venv dir via the helper BEFORE python -m venv. - launcher _ensure_dirs/pgdata and spa_dist staging use the helper. - Regression tests assert st_mode on pre-existing permissive trees and intermediate parents (the exact eyeball-proof gap). - Drive-by: placeholder-less f-string in corpuses/0028 print.
Review: One-step desktop setup (#2137)This is a big, well-iterated PR — the commit history shows ~15+ rounds of CodeQL/mypy/reviewer-feedback already addressed (zip-slip, symlink zip members, secret persistence, permission races, keyring hangs, etc.). I focused on what might still be wrong after all that polish, reviewing 1.
|
Three reviewer-confirmed gaps closed: - bootstrap: O_CREAT|O_EXCL install lock around venv creation + pip install (double-launched first runs interleaved writes into the same venv); stale locks reclaimed via PID liveness (POSIX) or age; state re-checked under the lock. - launcher: _wait_for_http short-circuits when a tracked child has died, and main() prints a logs-pointing ERROR instead of a success banner over a crashed Daphne; health/shutdown timeouts promoted to named constants. - spa_dist: the swap moves the old cache ASIDE and restores it if the rename fails (AV/indexer file locks), instead of rmtree-before-rename; ensure_spa re-resolves the cache after a failed refresh rather than trusting the pre-download Path; 4KB cap on the checksum sidecar read. Hardening nits: mode=0o700 on ensure_private_dir's mkdir (narrows the pre-chmod window), None-return guard in WarpIngestLocalParser, SPA fallback tests now assert the actual index.html body (not just 200 + content type).
Review: Single-user desktop packaging (Phase 0)This is a big PR (30 squashed commits, ~3.7k additions across 26 files) but it's unusually careful work for its risk surface (subprocess orchestration, an embedded DB, and edits to two already-shipped migrations). I checked out the merged tree against the pre-PR base ( Things done well
Worth a look
Nothing above blocks merging in my view — 1–2 are the ones I'd actually want a "yes, known, tracked for Phase 1" acknowledgment on before shipping to non-technical end users, since a stuck loopback port or pgdata lock is exactly the kind of cliff the PR's own usability testing was designed to catch. |
… trade-off doc - LauncherShutdownTests: _spawn a real child, assert _shutdown stops it and leaves the durable per-child log (the riskiest launcher logic now has a regression net despite the coverage omit). - StableSecretKeyFallbackTests: pin the fallback contract (hung keyring -> ephemeral key within timeout; broken backend -> ephemeral; healthy -> persisted key) so a refactor can't silently make the accepted race worse. - deps_ready docstring documents the intentional importability-over- version-pinning trade-off (ambient interpreters bypass the venv).
|
Re: the latest review — implemented 2 and 3, acknowledging 1, 4, 5 (commit
Generated by Claude Code |
Review of PR #2137 (single-user desktop packaging)This is a big, well-scoped change (settings profile, launcher, in-process PDF parser, migration fixes, docs/CI). I reviewed it in parallel across four areas: settings/launcher/security, migrations + bootstrap command, the new PDF parser, and docs/CI/tests. Overall the security-sensitive parts (subprocess handling, secret-key storage, SPA path traversal, URL catch-all scoping) are handled carefully — no injection, traversal, or hardcoded-secret issues found. The findings below are the residual gaps worth a look before merge. Correctness / robustness
Test coverage
Minor / low severity
What looks solid
Nice work on the usability-driven approach (baseline vs. fixed clean-room tests) — that's a good way to validate this kind of change. The parser hardening (#1/#2) and the CI dependency gap (#5) are the two I'd prioritize before merge; the rest are minor polish. |
- WarpIngestLocalParser: max_file_size_mb guard (default 200, matching the REST variant - parsing is in-process, so the cap bounds worker memory/CPU exposure; parse TIME stays bounded by Celery task limits, an in-process alarm would be POSIX-only); apply_ocr+disable_ocr now raises a classified non-transient error like the REST sibling instead of forwarding contradictory flags; export shape validated with isinstance(dict), not just None. - launcher: one-generation log rotation at spawn (10 MB cap) so daphne/worker/beat logs can't grow unbounded across restarts; friendly disk-full/read-only message around _ensure_dirs. - bootstrap: same friendly OSError message around the locked first-run install (read-only home, full disk, restricted app-data). - paths: Windows docstring no longer overclaims (chmod is a no-op there; protection rests on default per-user %LOCALAPPDATA% ACLs). - tests: DEFAULT_PORT-busy runner skips instead of false-failing.
Review: One-step desktop setup (python oc-desktop.py)Reviewed the full diff (28 files, +3854/-37) plus the actual source in this checkout (which already reflects the PR's merge state). Overall this is an unusually well-engineered PR: extensive inline rationale, careful failure-mode handling (stale locks, TOCTOU port binding, staged/atomic SPA swaps, zip-slip + symlink guards, checksum verification, keyring timeout handling), and thorough test coverage (test_desktop_packaging.py, test_warp_ingest_local_parser.py). Below are the things worth a second look. Correctness / security
launcher.py::main() (lines 483-484) is careful to scrub the var from both env and os.environ before spawning the long-lived Daphne/worker/beat children, with an explicit comment that a long-lived process's environment block (readable on Linux via its procfs entry) must not hold the password. That same reasoning applies equally to the outer bootstrap.py process (the literal python3 oc-desktop.py the user ran), which also lives until Ctrl+C - but its own os.environ["OC_DESKTOP_PASSWORD"] is never popped. So on a fresh install (the primary target scenario for this PR), the freshly-chosen login password sits in the outer wrapper process's environment (readable by the same OS user or root via the same procfs mechanism the launcher already worries about) for the whole session. Practical severity is limited (same-uid/root already has equivalent access to the app-data dir, venv, DB, etc.), but it's an inconsistency in a design that otherwise treats this exact leak vector as worth defending against. Fix: pop OC_DESKTOP_PASSWORD from os.environ in bootstrap.main() immediately after copying it into the child's env in _reexec_in_venv, or wherever the parent no longer needs it. Minor / informational
Strengths worth calling out
Test coverage Good breadth: pure path helpers, zip-slip/symlink guards, checksum verification (including malformed-response handling), trigram migration skip/create paths against a real Postgres, password prompt/self-heal/no-reprompt flows, keyring timeout/fallback behavior, and shutdown/child-termination. I didn't find a test exercising OC_DESKTOP_PASSWORD scrubbing across the bootstrap->launcher process boundary, consistent with finding #1 above going unnoticed. Nice work overall - this is a substantial usability lift validated by the described clean-room testing, and the code reads like it already went through several rounds of self-review. |
The launcher scrubbed OC_DESKTOP_PASSWORD from itself and its children, but the OUTER wrapper (the literal 'python3 oc-desktop.py' the user ran) set it via the early prompt and then lived until Ctrl+C in _reexec_in_venv's wait loop with the password still in its environment block - the exact procfs leak vector the launcher already defends against. Pop it right after copying the child env, so the password crosses each process boundary exactly once. Cross-boundary regression test added (the coverage gap the reviewer used to spot this). Also reword the MAX_BUNDLE_BYTES comment (500MB is ~33x the real bundle, not 'an order of magnitude').
ReviewReviewed the full diff (28 files, +3885/-37). This is an impressively thorough piece of packaging work — extensive prose documentation of why each decision was made, careful handling of edge cases (stale locks, partial installs, offline fallbacks), and 887 lines of new tests. Comments below are mostly minor/confirmatory; I didn't find any blocking correctness bugs. OverviewAdds a one-command desktop launcher ( Strengths
Issues / suggestions
Test coverageGood breadth for stdlib-only/pure logic. One gap: SecurityNo blocking issues found. Zip extraction, subprocess invocation, secret handling, and settings isolation (forcing |
- safe_extract_zip: cap the declared uncompressed total at MAX_BUNDLE_BYTES (same ceiling as the download), closing the one defense-in-depth gap reviewers found in the extraction pipeline; test included. - docs/test_scripts/desktop_one_step_setup.md: the clean-room manual procedure (first launch, login verification, teardown, warm relaunch) per the repo's manual-test-script convention, for future regression checks of the orchestration code that unit tests can't cover.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
codecov/patch failed at 54% because setup.cfg's coverage omit makes launcher.py carry NO line data in coverage.xml - and Codecov counts no-data diff lines as MISSES (232 of the 305 missing lines). The effective mechanism is .codecov.yml's ignore list (existing precedent: data_extract_tasks.py); added launcher.py there with the rationale, validated instead by docs/test_scripts/desktop_one_step_setup.md. The remaining gap was real and is now tested (desktop modules 83%->98%): - install-lock acquire/release/contention/stale-PID-reclaim/agefallback - venv helpers (_venv_is_current fingerprint logic, marker locations) - prompt mismatch-retry path - parser guards (size cap, OCR-flag conflict, non-dict export) - spa_dist branches (certifi context, symlink member rejection, no-index, nothing-available) - spa_fallback missing-index 404 - full call_command invocation of desktop_bootstrap (argument parsing + nltk seeding with mocked download) - config/urls.py desktop wiring block marked no-cover (import-time only, inert under test settings; the registered view is fully tested)
Review: One-step desktop setup (python3 oc-desktop.py)This is a large, well-scoped PR (30 files, ~4.2k additions) that replaces the multi-step desktop install with a single self-bootstrapping entry point. Overall engineering quality is high: the zip-slip guard, SHA-256 verification-before-extraction, TLS/certifi handling, port fallback, and the migration's pg_trgm availability probe are all correctly implemented. I reviewed the diff in four parts (bootstrap/password handling, spa_dist/launcher, migrations/CI/parser rename, tests/docs) and verified the findings below directly against the checked-out source. Security
Correctness
Migration safety (0074 / 0028 in-place edits) Verified this is safe as described: Django tracks applied migrations by (app, name) not content, there is no makemigrations --check / frozen-migration CI gate in this repo, and the SeparateDatabaseAndState operations are unchanged so makemigrations will not flag drift either. The pg_trgm availability probe correctly gates on connection.vendor first, queries the always-readable pg_available_extensions catalog, and forward/reverse are symmetric (DROP INDEX CONCURRENTLY IF EXISTS can't crash if the index was never created). 0028's edit is print-only (alarm to warning, gated on Corpus.objects.exists()), no functional change. Both are well covered by test_desktop_packaging.py. One forward-looking note already flagged by the migration's own docstring: state always claims the index exists even on desktop where it doesn't, so any future migration touching this index via state-derived SQL must stay IF EXISTS-tolerant. Parser rename / CI workflow warp_ingest_local_parser.py is consistent with its REST sibling (same mutual-exclusion error, same max_file_size_mb guard) and has solid test coverage. The new build-frontend-dist CI job is correctly ordered (build -> zip -> sha256 -> upload under bash -eo pipefail, so a failed build can't publish a stale/corrupt bundle). Test coverage / docs
Not an issue (verified, flagging only because it looked odd at first) opencontractserver/corpuses/migrations/0028_fix_grant_permission_to_corpus_creators.py initially looked like scope creep but is also an in-place edit of an already-landed migration (print statements only, gated to avoid alarming desktop first-run testers) -- legitimately in scope. Nice work overall -- the security-sensitive paths (zip-slip, checksum-before-extract, TLS handling, password floor/confirm/EOF-safety, private-dir permissioning) are handled carefully and are genuinely tested. The two security findings above (transient password-in-env during install, and the zip-bomb cap not holding on the TLS-only-trust path) are the ones I'd prioritize before merge; the rest are correctness/UX polish. |
… guard Two reviewer-verified security gaps and four correctness items: - Password window: the venv/pip install subprocesses (multi-minute) and the launcher's migrate subprocess inherited OC_DESKTOP_PASSWORD via the environment; both now receive explicitly scrubbed env copies - only desktop_bootstrap ever sees the password. - Zip-bomb guard actually holds now: the previous check summed the archive's DECLARED file_size (attacker-controlled central-directory metadata, not enforced by zipfile's decompression loop). Extraction now streams each member through archive.open() with a running byte budget against MAX_BUNDLE_BYTES - the real last line of defense on the TLS-only-trust path (releases without a .sha256 sidecar). - Re-prompt invariant fixed: a retried first run (marker unwritten because a LATER bootstrap step failed) re-prompted for a password the account already had and silently discarded the answer. desktop_ bootstrap now records a secret-free .password-set marker whenever the account has a usable password; the early prompt gates on it. - Interrupted-swap self-heal: a hard kill between the two swap renames left spa/ absent with good copies stranded at spa.new/spa.old; ensure_spa now promotes them (verified staging preferred). - Install-lock age ceiling on POSIX (PID reuse after a crash no longer wedges the lock); _venv_is_current treats unreadable requirements files as stale instead of raising a raw traceback; _fetch_json read capped like every other network path. - Docs: stale 'no cross-process lock yet' paragraph replaced (the lock exists and is tested); changelog fragment updated to the actual LOGIN_MIN_LENGTH constant name. Tests for all of it (desktop modules at 98%, suite green); mypy clean.
|
Review: Single-user desktop packaging (Phase 0) This is a large PR (about 4.4k lines) that adds a one-command desktop launcher (python oc-desktop.py). I read through the diff plus the checked-out working tree for the security-sensitive modules (opencontractserver/desktop/*, config/settings/desktop.py, config/spa.py, the two edited migrations, desktop_bootstrap.py, warp_ingest_local_parser.py) and the roughly 1200-line test suite. Overall impression: Code quality is high and clearly the product of multiple adversarial review passes already (the changelog fragments reference a clean-room usability test and a second round of adversarial PR reviews, and the code comments back that up). Highlights:
Minor notes (nothing blocking):
Security summary: No exploitable issues found in the security-sensitive surfaces (zip extraction, subprocess env handling, secret storage, file permissions, path traversal in config/spa.py via safe_join, the GitHub release fetch which is capped/timeout-bounded and github.com-only). Item 1 above is the only actual finding and it is low severity given the trust boundary. Test coverage: comprehensive for the new pure-logic modules. The one deliberate coverage gap (opencontractserver/desktop/launcher.py, excluded via .codecov.yml and setup.cfg) is reasonable since it is pure process-orchestration/IO, and it is backed by the manual end-to-end procedure in docs/test_scripts/desktop_one_step_setup.md, which follows the CLAUDE.md manual-test-script convention. Overall this is a thoughtfully hardened piece of infrastructure with its trade-offs called out inline rather than hidden. |
Defense-in-depth against the Actions script-injection pattern; low risk on a release-published trigger but free to avoid (reviewer nit).
3 participants
Goal
Make the single-user desktop packaging usable by a minimally technical end user: from a source download to the app in a browser in as close to one step as possible, cross-platform (Windows/macOS/Linux).
Validated by two clean-room usability tests with an agent role-playing a non-technical user: one against the baseline flow (to find the real cliffs), one against the new flow (to confirm they're gone). Results are posted as a PR comment.
What the baseline test found
The old flow (
yarn build+pip install+OC_DESKTOP_PASSWORD=... python oc-desktop.py) failed a non-technical user four ways:pgserverPostgres bundles no contrib extensions, soannotations/0074's unconditionalTrigramExtension()raisedNotSupportedError: extension "pg_trgm" is not available. Escaping it required compiling the extension from PostgreSQL source.yarn build) fails as written (yarn installnever mentioned) and assumes a Node toolchain.desktopdocumented nowhere (README'sadminfails).What changes
One command now does everything:
python3 oc-desktop.py(Windows:py oc-desktop.py).opencontractserver/desktop/bootstrap.py(new, stdlib-only): Python version gate 3.10–3.12 (pgserverhas no cp313 wheels) with a plain-English error; asks for the login password up front (the one interactive question, then setup runs unattended); creates a private venv under the per-user app-data dir; installsrequirements/desktop.txt(auto-reinstalls when the requirement files change, via content fingerprint) under a cross-process install lock; re-execs the launcher inside the venv. Loop-guarded, with actionable failure messages (missing python3-venv, disk full/read-only, no network).opencontractserver/desktop/spa_dist.py(new): removes the Node requirement. SPA resolution: repofrontend/dist→ version-stamped cached download in app-data →opencontracts-frontend-dist.zipfrom the GitHub release (version-matched tag, then latest; SHA-256-verified, size-capped, staged-then-swapped so a failed refresh never destroys a working cache) →yarn install && yarn buildfallback for developers. Zip-slip and symlink-member guarded extraction; honorsSSL_CERT_FILE, falls back to certifi..github/workflows/docker-build-release.yml: newbuild-frontend-distjob attaches the built SPA + its.sha256to every release (bundle is environment-agnostic — the launcher rewritesenv-config.jsat startup).annotations/0074migration: probespg_available_extensionsviaRunPython; skips extension + GIN index whenpg_trgmis absent (the queries it accelerates are plainicontains— correct unindexed, fine at single-user scale). Real Postgres deployments get the identical objects. Reverse now drops only the index, removing the documentedDROP EXTENSIONreversal hazard. Note on the in-place edit (considered deliberately): Django tracks applied migrations by(app, name), never content, so no already-migrated database re-runs it; 0074 only landed on main days ago via Authority packs: baseline-origin collision guard + multi-YAML taxonomy merge (#2057) #2133, and the new body is behavior-identical whereverpg_trgmexists — a superseding migration would not have fixed fresh desktop installs, which crash inside 0074 itself.desktop_bootstrap: password fromOC_DESKTOP_PASSWORDor the interactive first-run prompt (min-length floor enforced on both paths, confirmed twice, EOF/Ctrl+C-safe); a password-less account left over from a headless run self-heals on the next launch; never re-prompts once a usable password exists. The password is scrubbed from every long-lived process's environment (wrapper, launcher, children) after the single hand-off.desktopusername, and Ctrl+C stop instructions; crash-aware health check (no success banner over a dead Daphne — points at the per-child logs, which rotate at 10 MB); clean-teardown farewell stating where data lives; user-private (0o700) app-data tree enforced even for pre-existing dirs.docs/quick_start.mdcross-link;docs/deployment/desktop_packaging.mdrewritten around the one-command flow; page added to the mkdocs nav.Merge with main
origin/mainmerged in. The one conflict was add/add onwarp_ingest_parser.py: main's version is the REST client to the Warp-Ingest microservice; this branch's is the in-process library parser the desktop build needs. Resolution keeps main's REST client at the original path and moves the in-process variant towarp_ingest_local_parser.py/WarpIngestLocalParser("Warp-Ingest Parser (Local)",WARP_INGEST_LOCAL_*env vars, samemax_file_size_mbguard and mutual-exclusion error convention as the REST sibling) so both components coexist unambiguously.Tests
opencontractserver/tests/test_desktop_packaging.py(64 tests): path resolution per-OS, private-dir permission enforcement (incl. pre-existing permissive trees), SPA resolution order/zip-slip/symlink/checksum edge cases, trigram migration guard against a real Postgres, password prompt/self-heal/no-reprompt/floor/cross-process-scrub flows, launcher teardown and keyring-fallback behavior.🤖 Generated with Claude Code
https://claude.ai/code/session_01E4ahQjPm1McibG65spkxLR