cmoriarty/trog
1
0
Fork 0
Code Issues 11 Pull requests Projects 1 Releases Packages Wiki Activity Actions

Clean up project to professional software engineering standards #26

New issue
Closed
opened 2026-07-26 11:44:13 -04:00 by cmoriarty · 6 comments
cmoriarty commented 2026-07-26 11:44:13 -04:00
Owner
Copy link

We have been doing a good job of breaking large tasks into smaller ones, and building/testing incrementally as we go. I just haven't been scrutinizing the latest code changes as well as I should have. This ticket is to do a complete code review with the mission for this to be a first-class open source project.

  1. First begin with an audit of the code to create a prioritized list updates. Both a line-by-line granular look (to find irrelevant/dead code and comments, syntax problems, etc), but also a high-level design audit. One goal is for this to be a self-hostable project for other people, although right now it is very specific to my Trogdor server. There are many things that should be considered to make that a reality. Another goal is for me to learn and leverage modern AI tools as much as possible, so if there are any strategies and processes that the TROG project should adopt (graphify, etc), that should be part of the audit.

  2. Then create a plan to address the most critical and high priority things, and allow me to specify which medium/low tasks to take on.

  3. Implement the planned tasks. Possibly create spin off child tickets for large jobs, or keep all work contained in this ticket.

We have been doing a good job of breaking large tasks into smaller ones, and building/testing incrementally as we go. I just haven't been scrutinizing the latest code changes as well as I should have. This ticket is to do a complete code review with the mission for this to be a first-class open source project. 1. First begin with an audit of the code to create a prioritized list updates. Both a line-by-line granular look (to find irrelevant/dead code and comments, syntax problems, etc), but also a high-level design audit. One goal is for this to be a self-hostable project for other people, although right now it is very specific to my Trogdor server. There are many things that should be considered to make that a reality. Another goal is for me to learn and leverage modern AI tools as much as possible, so if there are any strategies and processes that the TROG project should adopt (graphify, etc), that should be part of the audit. 2. Then create a plan to address the most critical and high priority things, and allow me to specify which medium/low tasks to take on. 3. Implement the planned tasks. Possibly create spin off child tickets for large jobs, or keep all work contained in this ticket.
cmoriarty commented 2026-07-26 11:56:16 -04:00
Author
Owner
Copy link

Code audit (#26) — full-repo review, 2026-07-26

Scope: every source file in server/, cli/, studio/src/, audio-server/, playtest/, scripts/, brain/, both compose files, all Dockerfiles, CI workflows, and the test suite (~12k lines). Two lenses per the ticket: line-by-line (dead code, stale comments, bugs) and design-level (self-hostability, process/AI tooling).

Overall: the core is in better shape than the ticket feared. The orchestrator/drain/seat machinery is coherent, comments overwhelmingly explain why (many encode live-caught lessons), the studio is clean idiomatic React, and the test suite is real (118 tests, contracts not mocks-of-mocks). The problems cluster in three places: the repo lies about its portability, conductor-era references that no longer exist, and one genuinely broken thing nobody noticed.

P0 — Critical

1. CI has been red on main for the last 5 commits

tests/test_production.py imports server/graphs/production.py, which does from langchain_deepseek import ChatDeepSeek at module level. langchain-deepseek is pinned in server/requirements.txt but missing from requirements-dev.txt — every CI run since the ChatDeepSeek switch fails at collection with ModuleNotFoundError. Reproduced in a clean venv; the local .venv masked it because the package was installed there manually at some point (pip show confirms nothing requires it).

Fix: add langchain-deepseek==1.1.0 to requirements-dev.
Process fix (the more important half): red CI sat unnoticed while make deploy kept shipping. Options: (a) make redeploy queries the latest Actions run for HEAD and refuses on red (same spirit as its dirty-tree guard), (b) a Forgejo Actions failure notification. Recommend (a) — it's ~15 lines in the existing guard chain.

2. No LICENSE file

README says "TBD — an OSI-approved license before anything ships." For "first-class open source project" this is item zero: without a license file the repo is legally all-rights-reserved regardless of intent. Everything trog builds on is MIT/Apache-2.0, so either works. Needs your call: MIT (maximally simple) or Apache-2.0 (patent grant, matches LangGraph/Aegra).

P1 — High (self-hostability + real bugs)

3. Personal-site values baked in as code defaults

.env.example claims "Nothing site-specific is hardcoded in the repo." Currently false:

  • server/trog_lib/config.py:7 — FORGEJO_URL defaults to https://forgejo.underthere.xyz
  • server/trog_lib/seats.py:29-32 — COMFY_IMAGE/AUDIO_IMAGE default to forgejo.underthere.xyz/cmoriarty/...
  • FILES_URL defaults to http://trogdor:3923 in orchestrator.py, drain.py, playtest.py, cli/main.py
  • cli/main.py:45 — TROG_URL defaults to http://trogdor:8200; TROG_AEGRA_URL to trogdor:2026
  • studio/vite.config.ts — dev proxy targets default to trogdor
  • docker-compose.yml — FORGEJO_URL default repeated 4×; FORGEJO_USER defaults to cmoriarty

Fix: one pass. Server-side settings fail fast when required and unset (the FORGEJO_TOKEN pattern already does this); laptop-side (CLI) defaults become localhost; compose defaults become :?set in stack env or neutral examples. Route everything through config.py so there's exactly one place to look.

4. Two parallel Forgejo clients

trog_lib/forgejo.py (httpx, token auth, FORGEJO_URL/FORGEJO_TOKEN) and comfy_item.py's forgejo_commit/forgejo_fetch (urllib, basic auth, TROG_FORGEJO_URL/TROG_FORGEJO_AUTH) — the second is the vendored conductor contract, which is why compose has to synthesize TROG_FORGEJO_AUTH: user:token beside the token it already passes. The retry/stale-HEAD hardening in forgejo_commit is valuable and should survive the merge; the duplicate env contract should not. Also fold forgejo.commit_file/commit_binary (near-identical bodies) into one.

5. Mock seat has no /vocal route

audio_item.route_for("vocal") → /vocal, but mock_seat.py handles only /music and /sfx — a SEAT_MODE=mock or trog test asset --mock run with a vocal item 404s. Untested because CYCLE_SPEC has no vocal entry, so the plumbing tier never exercises the vocal path. Fix: add the route + a vocal entry (with Expected:) to the cycle spec.

6. Orchestrator shutdown can skip brain restore

lifespan does task.cancel() without awaiting the task, so the worker's CancelledError cleanup (cancel drain, release seats, restore brain) races process exit. Boot-time reconcile() covers the next start, but a plain docker stop of a busy orchestrator can leave the brain down until then. Related: restore_brain() clears _brain_taken even when the container start fails, so no later release path retries. Two small fixes: await the cancelled task with suppress; only clear the flag on successful start.

7. GPU topology is Trogdor's, hardcoded

SEAT_GPUS = {"image": {0,1,2,3}, "animate": {0,2,3}, "audio": {1}}, the seat definitions, BRAIN_CPUSET default, and the brain roster all encode 4× RTX 5000. A stranger with one 24GB card can't run a drain without editing source. Full generalization (hardware profiles) is a real project → propose a child ticket; the P1-sized slice now is: lift SEAT_GPUS/seat defs into env-overridable config and write docs/self-hosting.md stating the actual hardware assumptions (GPU count/VRAM, /mnt/models layout, Forgejo+Portainer, what to edit for other rigs).

P2 — Medium (pick which to take)

a. Conductor-era truth pass (docs/comments only, zero behavior). Stale references to retired machinery: assetq.py docstring (module CLI described as "the conductor talks to it", "exec'd inside the aegra container"), comfy_item.py/audio_item.py ("the conductor's contract", "audio.sh owns ports"), critics.py CLI docstring, assets.py:244 referencing deleted scripts/asset-profiles/, comfyui/workflows/README.md (documents workflows that now live in server/trog_lib/workflows/, references the deleted profile driver), audio-server/server.py header ("GPU3 / NUMA node 1", "the gateway's detect_speech tool" — no gateway exists), mock_seat.py docstring (tests/fixtures/mock-clip.mp4 → actually trog_lib/fixtures/), phasewalk.py curl examples using trogdor, README test-tier table (quick suite now includes the playtest tier the code runs).

b. .env.example refresh. Stale: BRAIN_MODEL_DIR/BRAIN_GGUF/BRAIN_CTX/BRAIN_KV/BRAIN_THREADS (dead since llama-swap — roster lives in brain/llama-swap.yaml). Missing: BRAIN_MODEL, FILES_URL, FORGEJO_USER, ORCH_SEAT_MODE, WEBSEARCH_MCP_URL, PLAYTEST_MAX_SECONDS, and the CLI-side TROG_URL/TROG_FILES_URL/TROG_AEGRA_URL.

c. Dead __main__ CLIs decision. assetq._cli, critics._cli, comfy_item.main, audio_item.main were the conductor's exec contract; nothing invokes them now (grep-verified). Keep review/playtest CLIs (useful by hand), delete or explicitly bless the rest.

d. CI doesn't cover the studio. Add a job: npm ci && npm run typecheck && npm run build (fast, no GPU). Today a tsc break ships silently.

e. Compose consistency. analyzer pins trogdor-audio:latest while every other image uses ${TAG:-latest} — a tagged deploy still floats the analyzer.

f. Constant dedupe. SFX suffix exists twice with drifted wording (orchestrator.SFX_SUFFIX vs assets.SFX_SUFFIX); default brain name exists 3× (compose env, python defaults, studio FeedPane DEFAULT_BRAIN); KIND_TO_PROFILE (orchestrator) overlaps _PROFILE_BY_KIND (assets).

g. assets_phase ignores the brain selector. Every phase/gate node resolves _active_llm(config); assets_phase takes no config and always uses the module default — a run pinned to a different roster brain silently writes its asset spec with the default one.

h. AI-tooling adoption.

  • Commit CLAUDE.md. One exists (written this session) but .gitignore currently ignores it — for a project built by agents, the agent onboarding doc belongs in the repo (also makes it available to CI/cloud agents). Optionally add AGENTS.md as a pointer for non-Claude tools.
  • graphify: run it over the repo and commit graphify-out/ (or regenerate on demand) so architecture queries hit the knowledge graph instead of cold greps; re-run per milestone.
  • Automated review: the strongbad-runner could run an agent review workflow on PRs; cheaper first step is keeping /code-review in the local loop before push.

P3 — Low (grab-bag; cheap, low risk)

  • submit_oneoff ref race: count(*)+1 in a transaction without a lock — concurrent submits can mint duplicate daily refs (cosmetic; multiplayer-studio relevant later).
  • New Postgres connection per call in assetq/ledger; the 1s SSE polls make it connection churn. Fine single-host; a tiny pool is the eventual fix.
  • playtest/server.py:293 Image.getdata deprecation (removal: Pillow 14) — two call sites.
  • cli _watch_poll loops forever if the orchestrator dies permanently mid-watch.
  • orchestrator.JobRequest.kind description omits vocal (the 422 hint too).
  • audio-server _resolve_asr_provider(provider, mode) — mode param unused.
  • /tts, /vad, /transcribe are unused by trog (vendored surface); document as extras or prune from the image later.
  • pixel_post._cleanup_orphans is a per-pixel Python loop — fine at 96px, would hurt on large sheets; vectorize only if ever needed.

Proposed plan

  1. Now, this ticket: P0 (deps fix + CI guard + LICENSE decision from you) and P1 items 3–6, plus the P1-sized slice of 7 (config lift + docs/self-hosting.md).
  2. Child ticket: hardware-profile generalization (arbitrary GPU counts/single-GPU mode) — real design work, shouldn't block this cleanup.
  3. Your picks: which of P2 a–h and P3 to fold in.

Everything lands as small reviewable commits per item, trog test green between each.

# Code audit (#26) — full-repo review, 2026-07-26 Scope: every source file in `server/`, `cli/`, `studio/src/`, `audio-server/`, `playtest/`, `scripts/`, `brain/`, both compose files, all Dockerfiles, CI workflows, and the test suite (~12k lines). Two lenses per the ticket: line-by-line (dead code, stale comments, bugs) and design-level (self-hostability, process/AI tooling). Overall: the core is in better shape than the ticket feared. The orchestrator/drain/seat machinery is coherent, comments overwhelmingly explain *why* (many encode live-caught lessons), the studio is clean idiomatic React, and the test suite is real (118 tests, contracts not mocks-of-mocks). The problems cluster in three places: **the repo lies about its portability**, **conductor-era references that no longer exist**, and **one genuinely broken thing nobody noticed**. --- ## P0 — Critical ### 1. CI has been red on main for the last 5 commits `tests/test_production.py` imports `server/graphs/production.py`, which does `from langchain_deepseek import ChatDeepSeek` at module level. `langchain-deepseek` is pinned in `server/requirements.txt` but **missing from `requirements-dev.txt`** — every CI run since the ChatDeepSeek switch fails at collection with `ModuleNotFoundError`. Reproduced in a clean venv; the local `.venv` masked it because the package was installed there manually at some point (`pip show` confirms nothing requires it). Fix: add `langchain-deepseek==1.1.0` to requirements-dev. Process fix (the more important half): red CI sat unnoticed while `make deploy` kept shipping. Options: (a) `make redeploy` queries the latest Actions run for HEAD and refuses on red (same spirit as its dirty-tree guard), (b) a Forgejo Actions failure notification. Recommend (a) — it's ~15 lines in the existing guard chain. ### 2. No LICENSE file README says "TBD — an OSI-approved license before anything ships." For "first-class open source project" this is item zero: without a license file the repo is legally all-rights-reserved regardless of intent. Everything trog builds on is MIT/Apache-2.0, so either works. Needs your call: MIT (maximally simple) or Apache-2.0 (patent grant, matches LangGraph/Aegra). --- ## P1 — High (self-hostability + real bugs) ### 3. Personal-site values baked in as code defaults `.env.example` claims "Nothing site-specific is hardcoded in the repo." Currently false: - `server/trog_lib/config.py:7` — `FORGEJO_URL` defaults to `https://forgejo.underthere.xyz` - `server/trog_lib/seats.py:29-32` — `COMFY_IMAGE`/`AUDIO_IMAGE` default to `forgejo.underthere.xyz/cmoriarty/...` - `FILES_URL` defaults to `http://trogdor:3923` in orchestrator.py, drain.py, playtest.py, cli/main.py - `cli/main.py:45` — `TROG_URL` defaults to `http://trogdor:8200`; `TROG_AEGRA_URL` to `trogdor:2026` - `studio/vite.config.ts` — dev proxy targets default to `trogdor` - `docker-compose.yml` — `FORGEJO_URL` default repeated 4×; `FORGEJO_USER` defaults to `cmoriarty` Fix: one pass. Server-side settings fail fast when required and unset (the `FORGEJO_TOKEN` pattern already does this); laptop-side (CLI) defaults become `localhost`; compose defaults become `:?set in stack env` or neutral examples. Route everything through `config.py` so there's exactly one place to look. ### 4. Two parallel Forgejo clients `trog_lib/forgejo.py` (httpx, token auth, `FORGEJO_URL`/`FORGEJO_TOKEN`) and `comfy_item.py`'s `forgejo_commit`/`forgejo_fetch` (urllib, basic auth, `TROG_FORGEJO_URL`/`TROG_FORGEJO_AUTH`) — the second is the vendored conductor contract, which is why compose has to synthesize `TROG_FORGEJO_AUTH: user:token` beside the token it already passes. The retry/stale-HEAD hardening in `forgejo_commit` is valuable and should survive the merge; the duplicate env contract should not. Also fold `forgejo.commit_file`/`commit_binary` (near-identical bodies) into one. ### 5. Mock seat has no `/vocal` route `audio_item.route_for("vocal")` → `/vocal`, but `mock_seat.py` handles only `/music` and `/sfx` — a `SEAT_MODE=mock` or `trog test asset --mock` run with a vocal item 404s. Untested because `CYCLE_SPEC` has no vocal entry, so the plumbing tier never exercises the vocal path. Fix: add the route + a vocal entry (with `Expected:`) to the cycle spec. ### 6. Orchestrator shutdown can skip brain restore `lifespan` does `task.cancel()` without awaiting the task, so the worker's `CancelledError` cleanup (cancel drain, release seats, **restore brain**) races process exit. Boot-time `reconcile()` covers the next start, but a plain `docker stop` of a busy orchestrator can leave the brain down until then. Related: `restore_brain()` clears `_brain_taken` even when the container start **fails**, so no later release path retries. Two small fixes: `await` the cancelled task with suppress; only clear the flag on successful start. ### 7. GPU topology is Trogdor's, hardcoded `SEAT_GPUS = {"image": {0,1,2,3}, "animate": {0,2,3}, "audio": {1}}`, the seat definitions, `BRAIN_CPUSET` default, and the brain roster all encode 4× RTX 5000. A stranger with one 24GB card can't run a drain without editing source. Full generalization (hardware profiles) is a real project → propose a child ticket; the P1-sized slice now is: lift `SEAT_GPUS`/seat defs into env-overridable config and write `docs/self-hosting.md` stating the actual hardware assumptions (GPU count/VRAM, `/mnt/models` layout, Forgejo+Portainer, what to edit for other rigs). --- ## P2 — Medium (pick which to take) **a. Conductor-era truth pass (docs/comments only, zero behavior).** Stale references to retired machinery: `assetq.py` docstring (module CLI described as "the conductor talks to it", "exec'd inside the aegra container"), `comfy_item.py`/`audio_item.py` ("the conductor's contract", "audio.sh owns ports"), `critics.py` CLI docstring, `assets.py:244` referencing deleted `scripts/asset-profiles/`, `comfyui/workflows/README.md` (documents workflows that now live in `server/trog_lib/workflows/`, references the deleted profile driver), `audio-server/server.py` header ("GPU3 / NUMA node 1", "the gateway's detect_speech tool" — no gateway exists), `mock_seat.py` docstring (`tests/fixtures/mock-clip.mp4` → actually `trog_lib/fixtures/`), `phasewalk.py` curl examples using `trogdor`, README test-tier table (quick suite now includes the playtest tier the code runs). **b. `.env.example` refresh.** Stale: `BRAIN_MODEL_DIR`/`BRAIN_GGUF`/`BRAIN_CTX`/`BRAIN_KV`/`BRAIN_THREADS` (dead since llama-swap — roster lives in `brain/llama-swap.yaml`). Missing: `BRAIN_MODEL`, `FILES_URL`, `FORGEJO_USER`, `ORCH_SEAT_MODE`, `WEBSEARCH_MCP_URL`, `PLAYTEST_MAX_SECONDS`, and the CLI-side `TROG_URL`/`TROG_FILES_URL`/`TROG_AEGRA_URL`. **c. Dead `__main__` CLIs decision.** `assetq._cli`, `critics._cli`, `comfy_item.main`, `audio_item.main` were the conductor's exec contract; nothing invokes them now (grep-verified). Keep `review`/`playtest` CLIs (useful by hand), delete or explicitly bless the rest. **d. CI doesn't cover the studio.** Add a job: `npm ci && npm run typecheck && npm run build` (fast, no GPU). Today a `tsc` break ships silently. **e. Compose consistency.** `analyzer` pins `trogdor-audio:latest` while every other image uses `${TAG:-latest}` — a tagged deploy still floats the analyzer. **f. Constant dedupe.** SFX suffix exists twice with drifted wording (`orchestrator.SFX_SUFFIX` vs `assets.SFX_SUFFIX`); default brain name exists 3× (compose env, python defaults, `studio FeedPane DEFAULT_BRAIN`); `KIND_TO_PROFILE` (orchestrator) overlaps `_PROFILE_BY_KIND` (assets). **g. `assets_phase` ignores the brain selector.** Every phase/gate node resolves `_active_llm(config)`; `assets_phase` takes no config and always uses the module default — a run pinned to a different roster brain silently writes its asset spec with the default one. **h. AI-tooling adoption.** - **Commit CLAUDE.md.** One exists (written this session) but `.gitignore` currently ignores it — for a project built by agents, the agent onboarding doc belongs in the repo (also makes it available to CI/cloud agents). Optionally add `AGENTS.md` as a pointer for non-Claude tools. - **graphify**: run it over the repo and commit `graphify-out/` (or regenerate on demand) so architecture queries hit the knowledge graph instead of cold greps; re-run per milestone. - **Automated review**: the strongbad-runner could run an agent review workflow on PRs; cheaper first step is keeping `/code-review` in the local loop before push. ## P3 — Low (grab-bag; cheap, low risk) - `submit_oneoff` ref race: `count(*)+1` in a transaction without a lock — concurrent submits can mint duplicate daily refs (cosmetic; multiplayer-studio relevant later). - New Postgres connection per call in `assetq`/`ledger`; the 1s SSE polls make it connection churn. Fine single-host; a tiny pool is the eventual fix. - `playtest/server.py:293` `Image.getdata` deprecation (removal: Pillow 14) — two call sites. - `cli _watch_poll` loops forever if the orchestrator dies permanently mid-watch. - `orchestrator.JobRequest.kind` description omits `vocal` (the 422 hint too). - `audio-server _resolve_asr_provider(provider, mode)` — `mode` param unused. - `/tts`, `/vad`, `/transcribe` are unused by trog (vendored surface); document as extras or prune from the image later. - `pixel_post._cleanup_orphans` is a per-pixel Python loop — fine at 96px, would hurt on large sheets; vectorize only if ever needed. --- ## Proposed plan 1. **Now, this ticket:** P0 (deps fix + CI guard + LICENSE decision from you) and P1 items 3–6, plus the P1-sized slice of 7 (config lift + `docs/self-hosting.md`). 2. **Child ticket:** hardware-profile generalization (arbitrary GPU counts/single-GPU mode) — real design work, shouldn't block this cleanup. 3. **Your picks:** which of P2 a–h and P3 to fold in. Everything lands as small reviewable commits per item, `trog test` green between each.
cmoriarty referenced this issue from a commit 2026-07-26 12:36:39 -04:00
chore: Apache-2.0 license
cmoriarty commented 2026-07-26 13:07:58 -04:00
Author
Owner
Copy link

#26 implementation — done, CI green

Thirteen commits on main (f4b59b7..cdf6940). CI is green on HEAD — both jobs (pytest + the new studio job) — for the first time in 8+ runs.

What landed

P0

  • fix(ci): langchain-deepseek added to requirements-dev — CI had failed at collection since the ChatDeepSeek switch; local venvs masked it.
  • chore: Apache-2.0 LICENSE (README updated).
  • feat: make redeploy refuses when CI is red for HEAD (scripts/check-ci.sh, third guard beside dirty-tree/unpushed; FORCE=1 overrides). Tested against the then-red HEAD — refused correctly.

P1

  • Personal-site defaults stripped: FORGEJO_URL fail-fast, seat images required via env, every trogdor:* fallback now localhost (code, CLI, vite proxies, docs). .env.example's "nothing hardcoded" claim is true now.
  • One Forgejo contract: drivers ride FORGEJO_URL+token via trog_lib.config; TROG_FORGEJO_URL/TROG_FORGEJO_AUTH/FORGEJO_USER deleted from compose; commit_file/commit_binary deduped.
  • Mock seat answers /vocal; cycle spec gains an anthem vocal sentinel (Expected: fail) so the mock tier exercises the route.
  • Orchestrator shutdown awaits the worker (brain restore actually runs); restore_brain retries instead of stranding the brain on a failed start.
  • GPU layout env-driven: SEAT_GPUS_IMAGE/_ANIMATE/_AUDIO; the parallelism matrix derives from the same values; compose passes them through. docs/self-hosting.md states the portability contract. Full hardware profiles → #27 (filed).

P2 (all four bundles)

  • Conductor-era truth pass: assetq/critics/comfy_item/audio_item docstrings fixed, dead argparse mains deleted, workflows README moved beside the JSONs, audio-server header de-lored, README quick-suite row corrected.
  • .env.example rewritten (dead pre-llama-swap brain vars out, real knobs in).
  • CI studio job (npm ci + tsc + vite build) — green on first run; analyzer image rides ${TAG}.
  • Dedupe + small fixes: one SFX_SUFFIX; studio brain selector defaults to the resident model from the live roster instead of a hardcoded name (browser-verified against the running stack — /v1/models is alphabetical, gemma would have outranked the preloaded qwen); assets_phase honors configurable.model.
  • AI tooling: CLAUDE.md committed (un-gitignored) + AGENTS.md pointer; graphify knowledge graph committed (graphify-out/: 1050 nodes / 1767 edges / 58 labeled communities — graphify query "<question>" answers architecture questions from the graph). Graph health notes: 292 dangling-endpoint edges (AST refs to externals — normal), 27 multi-relation pairs collapsed by the undirected build.

P3 (trivial picks)

  • Pillow getdata() deprecation gone from the playtest runner; JobRequest.kind mentions vocal; unused ASR mode param dropped.

CI post-mortem (two more Linux-only defects found while turning it green)

  • tests/conftest.py was missing — trog_lib imports rode on collection-order side effects (green on macOS, ImportError on the runner).
  • pixeloe drags GUI opencv-python in beside the headless pin; its cv2 needs libGL.so.1, absent in the bare job image — the pytest job now installs libgl1. The server image never sees this (ffmpeg supplies libGL). Verified in a bare python:3.14 container before pushing.

Deploy state

  • Portainer stack 92 env updated via API: FORGEJO_URL + COMFY_IMAGE added (settings-only, no redeploy fired; all containers stayed healthy). Compose fails fast without them since this ticket.
  • Laptop ~/.zshrc exports set (TROG_URL/TROG_FILES_URL/TROG_AEGRA_URL → trogdor); trog status verified green.
  • Next make deploy ships the new server/studio images through the normal guarded path.

Deferred (documented, not done)

  • #27: hardware profiles (per-VRAM workflow selection, roster sizing).
  • P3 leftovers from the audit: one-off ref race, per-call DB connections, CLI poll-loop timeout, pruning unused audio endpoints.
# #26 implementation — done, CI green Thirteen commits on main (`f4b59b7..cdf6940`). **CI is green on HEAD — both jobs (pytest + the new studio job) — for the first time in 8+ runs.** ## What landed **P0** - `fix(ci)`: `langchain-deepseek` added to requirements-dev — CI had failed at collection since the ChatDeepSeek switch; local venvs masked it. - `chore`: **Apache-2.0 LICENSE** (README updated). - `feat`: `make redeploy` refuses when CI is red for HEAD (`scripts/check-ci.sh`, third guard beside dirty-tree/unpushed; `FORCE=1` overrides). Tested against the then-red HEAD — refused correctly. **P1** - Personal-site defaults stripped: `FORGEJO_URL` fail-fast, seat images required via env, every `trogdor:*` fallback now localhost (code, CLI, vite proxies, docs). `.env.example`'s "nothing hardcoded" claim is true now. - One Forgejo contract: drivers ride `FORGEJO_URL`+token via `trog_lib.config`; `TROG_FORGEJO_URL`/`TROG_FORGEJO_AUTH`/`FORGEJO_USER` deleted from compose; `commit_file`/`commit_binary` deduped. - Mock seat answers `/vocal`; cycle spec gains an `anthem` vocal sentinel (Expected: fail) so the mock tier exercises the route. - Orchestrator shutdown awaits the worker (brain restore actually runs); `restore_brain` retries instead of stranding the brain on a failed start. - GPU layout env-driven: `SEAT_GPUS_IMAGE/_ANIMATE/_AUDIO`; the parallelism matrix derives from the same values; compose passes them through. **docs/self-hosting.md** states the portability contract. Full hardware profiles → **#27** (filed). **P2 (all four bundles)** - Conductor-era truth pass: assetq/critics/comfy_item/audio_item docstrings fixed, dead argparse mains deleted, workflows README moved beside the JSONs, audio-server header de-lored, README quick-suite row corrected. - `.env.example` rewritten (dead pre-llama-swap brain vars out, real knobs in). - CI studio job (npm ci + tsc + vite build) — green on first run; analyzer image rides `${TAG}`. - Dedupe + small fixes: one `SFX_SUFFIX`; studio brain selector defaults to the **resident** model from the live roster instead of a hardcoded name (browser-verified against the running stack — `/v1/models` is alphabetical, gemma would have outranked the preloaded qwen); `assets_phase` honors `configurable.model`. - AI tooling: CLAUDE.md committed (un-gitignored) + AGENTS.md pointer; **graphify knowledge graph** committed (`graphify-out/`: 1050 nodes / 1767 edges / 58 labeled communities — `graphify query "<question>"` answers architecture questions from the graph). Graph health notes: 292 dangling-endpoint edges (AST refs to externals — normal), 27 multi-relation pairs collapsed by the undirected build. **P3 (trivial picks)** - Pillow `getdata()` deprecation gone from the playtest runner; `JobRequest.kind` mentions vocal; unused ASR `mode` param dropped. **CI post-mortem (two more Linux-only defects found while turning it green)** - `tests/conftest.py` was missing — `trog_lib` imports rode on collection-order side effects (green on macOS, ImportError on the runner). - pixeloe drags GUI `opencv-python` in beside the headless pin; its cv2 needs `libGL.so.1`, absent in the bare job image — the pytest job now installs `libgl1`. The server image never sees this (ffmpeg supplies libGL). Verified in a bare `python:3.14` container before pushing. ## Deploy state - Portainer stack 92 env updated via API: `FORGEJO_URL` + `COMFY_IMAGE` added (settings-only, no redeploy fired; all containers stayed healthy). Compose fails fast without them since this ticket. - Laptop `~/.zshrc` exports set (`TROG_URL`/`TROG_FILES_URL`/`TROG_AEGRA_URL` → trogdor); `trog status` verified green. - Next `make deploy` ships the new server/studio images through the normal guarded path. ## Deferred (documented, not done) - #27: hardware profiles (per-VRAM workflow selection, roster sizing). - P3 leftovers from the audit: one-off ref race, per-call DB connections, CLI poll-loop timeout, pruning unused audio endpoints.
cmoriarty commented 2026-07-26 13:52:32 -04:00
Author
Owner
Copy link

README review + update plan (#26, first-class OSS lens)

Reviewed against the two canonical references — Art of README (cognitive funneling: broadest value first, depth later; "complete when someone can use it without reading the code") and the standard-readme spec (required: title, short description, ToC over 100 lines, install, usage, contributing, license-last) — plus the patterns of large OSS projects.

What's already excellent (keep, don't dilute)

  • The one-liner tagline is genuinely good ("A self-hosted collaborative AI game development studio").
  • The mermaid diagrams (idea→studio→game, architecture, the interrupt sequence, the phase pipeline) — most projects wish they had these.
  • The anatomy table, the testing-tier table, the principles list.
  • The voice. Distinctive, honest ("an autonomous studio should confess its compromises"). First-class doesn't mean corporate.

Gaps (ranked)

  1. Inverted funnel. After the tagline, the very first section is "Lessons learned from Agentic Game Development Studio" — insider history referencing a private predecessor repo, before a stranger learns what trog does or how to try it. The funnel says: value → try it → how it works → history.
  2. No quick start. "Deploying from scratch" is an operator runbook (homelab Forgejo + Portainer + 4 GPUs) — there is no minimum path for a stranger. We're sitting on the answer: SEAT_MODE=mock runs the entire pipeline with zero GPUs — compose up, open the studio, feed a line to phasewalk, run trog test asset --mock. That's a 10-minute GPU-less demo nobody can currently discover.
  3. No table of contents (403 lines; spec requires one over 100).
  4. No Contributing section and no CONTRIBUTING.md — spec-required. Also nothing that says project status (pre-1.0, v1 milestone in flight, API unstable).
  5. Requirements/caveats not upfront. GPU hardware (NVIDIA, 16GB+/seat), self-hosted Forgejo, Linux host — a reader learns this at line ~280. Art of README: caveats stated early.
  6. A visual product with no product visuals. Diagrams ≠ screenshots. The studio feed mid-run, the map with a lit node, an asset review page — one or two images (plus alt text; never load-bearing).
  7. "100% open source" claim needs an honesty footnote on model weights. The code is Apache-2.0, but the default image seat is FLUX.2-dev (non-commercial weights license); other weights vary. First-class projects carry a weights-license table. This is the single biggest "trust" item for outside adopters.
  8. Runbook + reference content bloats the funnel. "Deploying from scratch" (now overlapping docs/self-hosting.md), the full CLI guide, and testing details belong in docs/ with README keeping a summary + link.
  9. Badges: only two, pointing at the personal Forgejo (fine while it's the canonical host — note if a GitHub mirror ever appears, badges/links need a canonical-host decision). Add a license badge.
  10. License section: right place (last), should carry SPDX (Apache-2.0) + copyright holder per spec.

Proposed target structure

1. Title + tagline + badges (pytest · deps · license)
2. Hero: 2-sentence what/why + the idea→game diagram + 1 studio screenshot
3. ⚠ Status & requirements box (pre-1.0; hardware floor; Forgejo needed;
   code Apache-2.0 / weights vary — link to weights table)
4. Table of contents
5. Quick start
   a. Kick the tires, no GPUs: SEAT_MODE=mock compose up → studio → phasewalk
      → trog test asset --mock (the whole pipeline on stubs)
   b. The real rig → docs/self-hosting.md
6. Usage: trog CLI one-liners + 3-line studio tour (link deeper)
7. How it works: architecture diagram + anatomy table + goals condensed,
   each goal marked ✅ working / 🚧 in progress (honest status per feature)
8. The production process (Lemarchand section — keep, lightly tightened)
9. Testing: the tier table + one paragraph (details → docs/testing.md)
10. Roadmap: v1/v2/v3 milestones link
11. Background: lessons-learned moved HERE (trimmed, funnel-correct)
12. Contributing: → new CONTRIBUTING.md (dev setup, make targets, test
    tiers, lint-only-ruff rule, agent workflow / CLAUDE.md pointer)
13. Acknowledgements: Lemarchand's book, Aegra, LangGraph, model authors
14. License: Apache-2.0 (SPDX + holder) + the model-weights license table
    (or link to docs/weights-licenses.md)

Work items (in order)

# item size
1 Restructure README to the skeleton above (content mostly exists; this is moving + trimming, not rewriting) M
2 Write the GPU-less quick start and verify it by actually running it (mock compose profile may need a tiny compose override to be one command) M
3 Move "Deploying from scratch" into docs/self-hosting.md (merge, de-dupe) S
4 New CONTRIBUTING.md + status statement; decide on code of conduct S
5 Model-weights license table (FLUX.2-dev, Z-Image, Wan, LoRAs, ACE-Step, stable-audio-open, HeartMuLa, GGUF bases) — audit each license honestly M
6 Screenshots: studio feed mid-run + review page; commit under docs/media/ S
7 ToC, license badge, SPDX line, link pass (no broken links; predecessor-repo links get context or go) S
8 docs/testing.md if the README testing section slims below the table S

Item 5 is the one with teeth — if FLUX.2-dev's license genuinely restricts commercial output, the README must say what that means for games people generate, and the principles line softens to "code 100% Apache-2.0; default weights have their own licenses, swappable per seat."

Sources: Art of README, standard-readme spec, README best-practice guides.

# README review + update plan (#26, first-class OSS lens) Reviewed against the two canonical references — Art of README (cognitive funneling: broadest value first, depth later; "complete when someone can use it without reading the code") and the standard-readme spec (required: title, short description, ToC over 100 lines, install, usage, contributing, license-last) — plus the patterns of large OSS projects. ## What's already excellent (keep, don't dilute) - The one-liner tagline is genuinely good ("*A self-hosted collaborative AI game development studio*"). - The mermaid diagrams (idea→studio→game, architecture, the interrupt sequence, the phase pipeline) — most projects wish they had these. - The anatomy table, the testing-tier table, the principles list. - The voice. Distinctive, honest ("an autonomous studio should confess its compromises"). First-class doesn't mean corporate. ## Gaps (ranked) 1. **Inverted funnel.** After the tagline, the very first section is "Lessons learned from Agentic Game Development Studio" — insider history referencing a private predecessor repo, before a stranger learns what trog does or how to try it. The funnel says: value → try it → how it works → history. 2. **No quick start.** "Deploying from scratch" is an operator runbook (homelab Forgejo + Portainer + 4 GPUs) — there is no minimum path for a stranger. We're sitting on the answer: **`SEAT_MODE=mock` runs the entire pipeline with zero GPUs** — compose up, open the studio, feed a line to phasewalk, run `trog test asset --mock`. That's a 10-minute GPU-less demo nobody can currently discover. 3. **No table of contents** (403 lines; spec requires one over 100). 4. **No Contributing section** and no CONTRIBUTING.md — spec-required. Also nothing that says project status (pre-1.0, v1 milestone in flight, API unstable). 5. **Requirements/caveats not upfront.** GPU hardware (NVIDIA, 16GB+/seat), self-hosted Forgejo, Linux host — a reader learns this at line ~280. Art of README: caveats stated early. 6. **A visual product with no product visuals.** Diagrams ≠ screenshots. The studio feed mid-run, the map with a lit node, an asset review page — one or two images (plus alt text; never load-bearing). 7. **"100% open source" claim needs an honesty footnote on model weights.** The *code* is Apache-2.0, but the default image seat is FLUX.2-dev (non-commercial weights license); other weights vary. First-class projects carry a weights-license table. This is the single biggest "trust" item for outside adopters. 8. **Runbook + reference content bloats the funnel.** "Deploying from scratch" (now overlapping docs/self-hosting.md), the full CLI guide, and testing details belong in docs/ with README keeping a summary + link. 9. Badges: only two, pointing at the personal Forgejo (fine while it's the canonical host — note if a GitHub mirror ever appears, badges/links need a canonical-host decision). Add a license badge. 10. License section: right place (last), should carry SPDX (`Apache-2.0`) + copyright holder per spec. ## Proposed target structure ``` 1. Title + tagline + badges (pytest · deps · license) 2. Hero: 2-sentence what/why + the idea→game diagram + 1 studio screenshot 3. ⚠ Status & requirements box (pre-1.0; hardware floor; Forgejo needed; code Apache-2.0 / weights vary — link to weights table) 4. Table of contents 5. Quick start a. Kick the tires, no GPUs: SEAT_MODE=mock compose up → studio → phasewalk → trog test asset --mock (the whole pipeline on stubs) b. The real rig → docs/self-hosting.md 6. Usage: trog CLI one-liners + 3-line studio tour (link deeper) 7. How it works: architecture diagram + anatomy table + goals condensed, each goal marked ✅ working / 🚧 in progress (honest status per feature) 8. The production process (Lemarchand section — keep, lightly tightened) 9. Testing: the tier table + one paragraph (details → docs/testing.md) 10. Roadmap: v1/v2/v3 milestones link 11. Background: lessons-learned moved HERE (trimmed, funnel-correct) 12. Contributing: → new CONTRIBUTING.md (dev setup, make targets, test tiers, lint-only-ruff rule, agent workflow / CLAUDE.md pointer) 13. Acknowledgements: Lemarchand's book, Aegra, LangGraph, model authors 14. License: Apache-2.0 (SPDX + holder) + the model-weights license table (or link to docs/weights-licenses.md) ``` ## Work items (in order) | # | item | size | |---|---|---| | 1 | Restructure README to the skeleton above (content mostly exists; this is moving + trimming, not rewriting) | M | | 2 | Write the GPU-less quick start and **verify it by actually running it** (mock compose profile may need a tiny compose override to be one command) | M | | 3 | Move "Deploying from scratch" into docs/self-hosting.md (merge, de-dupe) | S | | 4 | New CONTRIBUTING.md + status statement; decide on code of conduct | S | | 5 | Model-weights license table (FLUX.2-dev, Z-Image, Wan, LoRAs, ACE-Step, stable-audio-open, HeartMuLa, GGUF bases) — audit each license honestly | M | | 6 | Screenshots: studio feed mid-run + review page; commit under docs/media/ | S | | 7 | ToC, license badge, SPDX line, link pass (no broken links; predecessor-repo links get context or go) | S | | 8 | docs/testing.md if the README testing section slims below the table | S | Item 5 is the one with teeth — if FLUX.2-dev's license genuinely restricts commercial output, the README must say what that means for games people generate, and the principles line softens to "code 100% Apache-2.0; default weights have their own licenses, swappable per seat." Sources: [Art of README](https://github.com/hackergrrl/art-of-readme), [standard-readme spec](https://github.com/RichardLitt/standard-readme/blob/master/spec.md), [README best-practice guides](https://www.gitdevtool.com/blog/readme-best-practice).
cmoriarty commented 2026-07-26 13:55:18 -04:00
Author
Owner
Copy link

Post-deploy verification — all green

Deploy of cdf6940 confirmed: all 9 containers healthy on current images, stack env live in-container.

check result
unit + lint + studio CI ✅ green on HEAD (both jobs)
playtest fixture ✅ good passes / broken sentinel fails
critics fixture ✅ 6 pass / orange fails; verdict cache survived deploy
mock asset cycle ✅ PLUMBING OK, 8/8 declared verdicts — new anthem vocal sentinel exercised the mock /vocal route, failed as declared, bounded-regen behaved (1 requeue, then advanced)
brain-swap stress ✅ 2 loops x 4 roster brains = 8 real generations in one thread, zero failures, resident state correct every step (14–77s per swap+gen)
seat bracket (real sfx one-off) ✅ delivered in 36s; brain stopped for the seat, seat kept warm 180s, brain auto-restored after TTL, seat released
studio proxies (:2027 /api /orch /brain) ✅ 200
studio UI e2e (new) ✅ 7/7 twice (once via trog test ui), including a live send→stream→reply mid-drain

New in fa952e4: studio/e2e Playwright suite (user-level, real browser, real stack; trog test ui --headed to watch) — includes the brain-selector-defaults-to-resident regression test.

README review + update plan posted above; awaiting go-ahead on implementation.

# Post-deploy verification — all green Deploy of `cdf6940` confirmed: all 9 containers healthy on current images, stack env live in-container. | check | result | |---|---| | unit + lint + studio CI | ✅ green on HEAD (both jobs) | | playtest fixture | ✅ good passes / broken sentinel fails | | critics fixture | ✅ 6 pass / orange fails; verdict cache survived deploy | | mock asset cycle | ✅ PLUMBING OK, **8/8 declared verdicts** — new anthem vocal sentinel exercised the mock /vocal route, failed as declared, bounded-regen behaved (1 requeue, then advanced) | | **brain-swap stress** | ✅ 2 loops x 4 roster brains = 8 real generations in one thread, zero failures, resident state correct every step (14–77s per swap+gen) | | seat bracket (real sfx one-off) | ✅ delivered in 36s; brain stopped for the seat, seat kept warm 180s, **brain auto-restored after TTL, seat released** | | studio proxies (:2027 /api /orch /brain) | ✅ 200 | | **studio UI e2e (new)** | ✅ 7/7 twice (once via `trog test ui`), including a live send→stream→reply mid-drain | New in fa952e4: `studio/e2e` Playwright suite (user-level, real browser, real stack; `trog test ui --headed` to watch) — includes the brain-selector-defaults-to-resident regression test. README review + update plan posted above; awaiting go-ahead on implementation.
cmoriarty commented 2026-07-26 14:43:57 -04:00
Author
Owner
Copy link

README overhaul landed (bffb255)

All 8 plan items done, plus one bug the plan flushed out.

  • Structure: funnel-correct — hero + status box + ToC + quick start + usage up top; lessons-learned moved to Background; operator runbook merged into docs/self-hosting.md; CONTRIBUTING.md added; SPDX license footer; license badge.
  • Quick start is real, not aspirational: verified by standing up the exact 4-container subset (postgres/redis/aegra/studio, dummy Forgejo env, no GPUs) on an isolated scratch project — phasewalk walked to ship, studio e2e smoke+map passed 4/4 against it.
  • Bug found doing that: studio nginx resolved upstreams at startup — a missing orchestrator/brain (or any one service mid-restart) crash-looped the whole studio. Now resolves at request time via Docker DNS; absent services degrade to 502s the UI already tolerates. This also makes the deployed studio survive single-service restarts.
  • Models section per direction: framework is model-agnostic (brain = any OpenAI-compatible endpoint incl. cloud via BRAIN_URL/BRAIN_API_KEY; image = workflow JSON + weights; audio = env-pointed checkpoints); benchmark winners listed as recommendations with per-model weight licenses — FLUX.2-dev called out honestly (weights non-commercial, outputs commercially usable); principles line now says code Apache-2.0, weights are swappable guests.
  • Screenshots: captured from the live stack by studio/e2e/readme-shots.mjs (reproducible — kicks a real phasewalk run, shoots studio + fixture review page).

Note for next deploy: the nginx fix is in the studio image — ships with the next make deploy.

# README overhaul landed (bffb255) All 8 plan items done, plus one bug the plan flushed out. - **Structure**: funnel-correct — hero + status box + ToC + quick start + usage up top; lessons-learned moved to Background; operator runbook merged into docs/self-hosting.md; CONTRIBUTING.md added; SPDX license footer; license badge. - **Quick start is real, not aspirational**: verified by standing up the exact 4-container subset (postgres/redis/aegra/studio, dummy Forgejo env, no GPUs) on an isolated scratch project — phasewalk walked to `ship`, studio e2e smoke+map passed 4/4 against it. - **Bug found doing that**: studio nginx resolved upstreams at startup — a missing orchestrator/brain (or any one service mid-restart) crash-looped the whole studio. Now resolves at request time via Docker DNS; absent services degrade to 502s the UI already tolerates. This also makes the deployed studio survive single-service restarts. - **Models section** per direction: framework is model-agnostic (brain = any OpenAI-compatible endpoint incl. cloud via BRAIN_URL/BRAIN_API_KEY; image = workflow JSON + weights; audio = env-pointed checkpoints); benchmark winners listed as recommendations with per-model weight licenses — FLUX.2-dev called out honestly (weights non-commercial, **outputs commercially usable**); principles line now says code Apache-2.0, weights are swappable guests. - **Screenshots**: captured from the live stack by `studio/e2e/readme-shots.mjs` (reproducible — kicks a real phasewalk run, shoots studio + fixture review page). Note for next deploy: the nginx fix is in the studio image — ships with the next `make deploy`.
cmoriarty commented 2026-07-27 11:07:49 -04:00
Author
Owner
Copy link

Closing — audit executed end to end, follow-on work landed or ticketed

Everything this ticket asked for shipped and was verified live: the prioritized audit (comments above), the approved P0/P1/P2 implementation (Apache-2.0, self-hostability pass, one Forgejo contract, mock /vocal, shutdown fixes, docs truth pass, CI studio job, dedupe, CLAUDE.md/graphify adoption), the README overhaul with a verified GPU-less quick start, and the post-deploy verification + stress campaign.

The ticket also seeded work that outgrew it, all landed separately:

  • #27 hardware profiles (closed — trogdor profile byte-identical, single-GPU proven on real hardware)
  • trog update --dependencies guided bump pipeline — used in anger to bump the llama.cpp engine (kept; drift report fully green)
  • CI overhaul: cpu-torch index + uv + venv/toolchain caching on a host-networked runner (pytest job from 3+ min of installs to a ~125s warm job, floor still dropping), deploy guard that gates on real test jobs only

Remaining low-priority hygiene items are parked in #29; studio pane polish in #28. Nothing else outstanding.

# Closing — audit executed end to end, follow-on work landed or ticketed Everything this ticket asked for shipped and was verified live: the prioritized audit (comments above), the approved P0/P1/P2 implementation (Apache-2.0, self-hostability pass, one Forgejo contract, mock /vocal, shutdown fixes, docs truth pass, CI studio job, dedupe, CLAUDE.md/graphify adoption), the README overhaul with a verified GPU-less quick start, and the post-deploy verification + stress campaign. The ticket also seeded work that outgrew it, all landed separately: - **#27** hardware profiles (closed — trogdor profile byte-identical, single-GPU proven on real hardware) - `trog update --dependencies` guided bump pipeline — used in anger to bump the llama.cpp engine (kept; drift report fully green) - CI overhaul: cpu-torch index + uv + venv/toolchain caching on a host-networked runner (pytest job from 3+ min of installs to a ~125s warm job, floor still dropping), deploy guard that gates on real test jobs only Remaining low-priority hygiene items are parked in **#29**; studio pane polish in **#28**. Nothing else outstanding.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
feat/11-walking-skeleton
No results found.
Labels
Clear labels   
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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
cmoriarty/trog#26
No description provided.