Skip to content

RFC: Workspace abstraction — first-class unit of configuration#124

Merged
marklubin merged 12 commits intomainfrom
mark/workspace-abstraction
Apr 6, 2026
Merged

RFC: Workspace abstraction — first-class unit of configuration#124
marklubin merged 12 commits intomainfrom
mark/workspace-abstraction

Conversation

@marklubin
Copy link
Copy Markdown
Owner

Summary

This RFC proposes a Workspace class as the first-class unit of configuration in Synix, replacing the scattered state across Project, ServerConfig, Pipeline, and the _state dict.

Problem: There's no single entity for "a synix project with its config, pipeline, buckets, build state, and runtime services." This causes 60 lines of manual wiring in serve.py, the viewer can't start without a release, CLI commands bypass Project entirely, and nothing enforces that these pieces form a coherent namespace.

Proposal: Workspace wraps Project via composition and absorbs workspace-scoped config (buckets, auto_build, vllm, pipeline_path) while leaving server-specific settings (ports, hosts) separate.

  • Computed lifecycle states: FRESH → CONFIGURED → BUILT → RELEASED → SERVING
  • WorkspaceRuntime replaces the global _state dict with typed runtime services
  • Config split: [workspace] + [buckets] + [auto_build] + [vllm] are workspace-scoped; [server] is process-scoped
  • Viewer binds to Workspace (can start fresh, discovers releases lazily)
  • Full backward compatibility: open_project() unchanged, old TOML format parsed

See docs/workspace-rfc.md for full design, API, migration strategy, and phased implementation plan.

Test plan

  • Review RFC design doc
  • Implementation follows in separate PR after design approval

Proposes a Workspace class that unifies the currently scattered
project configuration: Project (sdk), ServerConfig (server/config),
Pipeline (core/models), and the _state dict (mcp_tools).

One Workspace = one synix namespace with identity, pipeline, buckets,
.synix state, releases, and runtime services. Wraps Project via
composition (backward compatible). Replaces the _state dict with
typed WorkspaceRuntime. Viewer binds to Workspace instead of Release.

See docs/workspace-rfc.md for full design, API, migration strategy,
and implementation plan.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 5, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Moderate risk: this is “just an RFC” in the diff, but it proposes a user-facing config and runtime abstraction that cuts across CLI, SDK, server, and viewer, so bad decisions here will be expensive to unwind.

One-way doors

  1. Workspace as the top-level abstraction
    Hard to reverse because it becomes the mental model across SDK, CLI, server, docs, and tests. README/design currently center Project/Pipeline; adding a second first-class noun invites long-term duplication or deprecation pain. Safe only if you can clearly define boundaries: when users should use Project vs Workspace, and guarantee one does not become a leaky wrapper around the other.

  2. synix.toml + config shape split ([workspace], [server], [buckets], [auto_build], [vllm])
    Config formats ossify fast. Users, deploy scripts, examples, and plugin code will depend on exact section names and discovery order. Safe only if you commit to a migration/compat story, schema validation, and documented precedence rules when both synix.toml and synix-server.toml exist.

  3. project_dir removal from config
    This changes deployment semantics and discovery assumptions. Hard to reverse once container/deploy tooling is updated. Safe only if discovery is unambiguous in all supported execution contexts and there’s a tested fallback for nonstandard working directories.

  4. Global runtime singleton replacement (_state_workspace)
    Still a singleton, just with a nicer type. If external integrations or tests start assuming one active workspace per process, multi-workspace serving becomes harder later. Safe only if you explicitly accept “one workspace per process” as a constraint or redesign now to pass workspace through request/app context.

Findings

  1. docs/workspace-rfc.md — abstraction drift from design docs
    The design docs are Python-first and build/release centered around Project/Pipeline; this RFC introduces Workspace as the true unit of composition and pulls in server/runtime concerns. That couples offline build semantics to always-on services, which conflicts with the README’s “Synix runs offline… decoupled” positioning. Severity: [warning]

  2. docs/workspace-rfc.mdWorkspaceState is underspecified and likely misleading
    States like BUILT = “at least one snapshot ref” and RELEASED = “at least one release receipt” are derived from disk artifacts, not current usability. A workspace with stale pipeline/config but an old release still appears “released.” This invites UI/CLI bugs and false confidence. Severity: [warning]

  3. docs/workspace-rfc.md — config parser backward compatibility is hand-waved
    “Parser understands both old format and new format” is exactly where subtle regressions happen: precedence, error messages, mixed-format files, and discovery when both files exist are not defined. That is a system boundary and needs deterministic rules, not aspiration. Severity: [warning]

  4. docs/workspace-rfc.md — imports config dataclasses from server/config.py
    The new supposedly core abstraction depends on server-layer types (BucketConfig, BuildQueueConfig, VLLMConfig). That is backwards coupling. Workspace is being positioned as the first-class unit, but it is parasitic on server internals. You are baking server concerns into the core namespace. Severity: [warning]

  5. docs/workspace-rfc.md — runtime model still relies on mutable in-process state
    Replacing _state dict with WorkspaceRuntime does not solve concurrency or lifecycle issues. asyncio.Lock in process memory is not protection if multiple processes access the same .synix/ workspace, which is a plausible deployment mode for server + CLI or multiple server workers. Severity: [critical]

  6. docs/workspace-rfc.md — viewer/release semantics contradict the stated problem
    The RFC says viewer “can’t start without a release,” then proposes workspace binding so it can discover releases better. That’s not solving the product constraint, just moving lookup logic. If viewer remains release-dependent, say so; if not, define what it renders for unreleased builds. Severity: [minor]

  7. docs/workspace-rfc.mdWorkspace API surface is too broad for a first pass
    Identity, config, buckets, pipeline loading, build/release delegation, runtime activation, direct project access. That is a god object. Given pre-1.0 churn, this is how accidental public API gets frozen before you understand the seams. Severity: [warning]

Missing

  • A precise compatibility matrix: old config only, new config only, both present, explicit --config, cwd mismatch.
  • Validation rules for TOML schema and bucket path safety.
  • Concurrency model for multiple processes touching one workspace.
  • Documentation updates reconciling Project vs Workspace in README/SDK docs.
  • Tests for failure modes: missing pipeline file, invalid TOML, runtime activated without pipeline, stale release refs, mixed old/new config keys.

VerdictBlock: the proposal is directionally plausible, but it cements a new top-level abstraction and config format without resolving ownership boundaries, compatibility rules, or multi-process correctness.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 234 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-05T21:50:26Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 5, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR adds an RFC document (docs/workspace-rfc.md) proposing a new Workspace abstraction that unifies five currently-disconnected configuration surfaces (Project, ServerConfig, Pipeline, _state dict, CLI options) into a single first-class entity. No code is changed — this is a design document with a phased implementation plan.

Alignment

Good fit. DESIGN.md defines Pipeline as the core container but never addresses the runtime wiring problem this RFC identifies. The Workspace sits above Pipeline and Project without replacing either — it's a composition wrapper, not a new domain primitive. This respects the build/release separation (§1.5): Workspace delegates build/release to Project and keeps runtime concerns (queue, vllm, lock) in a separate WorkspaceRuntime that only activates during serve. The RFC's "state derived from disk, never persisted" principle is consistent with the content-addressed, immutable-artifact philosophy — no new mutable manifest files.

The synix.toml config file introduces a TOML surface, but for infrastructure wiring (ports, bucket paths), not pipeline definition. This is consistent with the Python-first decision (§4.1, Appendix C): pipeline logic stays in Python, operational config goes in TOML. Reasonable boundary.

Observations

  1. [positive] The phased implementation plan is well-structured — Phase 1 is purely additive with a gate (uv run release passes), meaning each phase can be reviewed independently. This is disciplined incremental delivery.

  2. [positive] WorkspaceState as a computed enum derived from disk state (not persisted) avoids introducing mutable state files. Consistent with the "artifacts are immutable, truth lives in .synix/" principle.

  3. [concern] The RFC proposes WorkspaceConfig importing BucketConfig, BuildQueueConfig, VLLMConfig from server/config.py. This creates a dependency from the new core-level workspace.py into the server/ package. If Workspace is meant to be the foundational abstraction, the shared config types should live at the workspace level, with server importing from workspace — not the reverse.

  4. [concern] No discussion of how open_workspace() interacts with open_project() when both are exported from __init__.py. Users will encounter both entry points. Which should the README/quickstart recommend? The RFC should specify the guidance or deprecation path.

  5. [question] The backward-compat adapter serve_from_config(config: ServerConfig) in Phase 2 — is this intended to be permanent or transitional? If transitional, what's the removal timeline? If permanent, it's a second code path to maintain indefinitely.

  6. [question] The TOML discovery order (synix.tomlsynix-server.toml) handles backward compat, but what happens when both files exist? The RFC doesn't specify precedence or conflict behavior.

  7. [nit] The WorkspaceRuntime dataclass bundles asyncio.Lock — this couples the abstraction to async. If anyone calls build() synchronously through the Workspace while runtime is active, is the lock meaningful? Worth a sentence on the threading/async contract.

  8. [concern] The test plan lists unit tests for workspace state, TOML parsing, and delegation, but no integration test verifying the full open_workspace() → build → release → search path. Phase 4 verification relies on a container smoke test, but there's no in-repo e2e test covering the new entry point. Given that Workspace becomes the primary wiring surface, an e2e test should be explicit in the plan.

  9. [nit] bucket_dir(name) -> Path appears in the API but the relationship between buckets and pipeline.source_dir isn't clarified. Are buckets a server-mode-only concept, or do they supersede source_dir for all builds?

Verdict

A well-motivated RFC that addresses real configuration sprawl without disturbing core abstractions — but it needs to resolve the dependency direction between workspace and server config types, and add an e2e test to the implementation plan before merging.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 234 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-05T21:50:33Z

- Config types (BucketConfig, BuildQueueConfig, VLLMConfig) move to
  workspace.py; server/config.py re-exports for backward compat
- Explicit config file precedence: synix.toml wins, warning if both exist
- Document open_project() vs open_workspace() guidance
- Add scope/constraints section: single-process, not a god object,
  buckets vs source_dir distinction
- WorkspaceState describes capability not recency
- Add e2e test (test_workspace_e2e.py) to implementation plan
- Add decisions log table resolving all review concerns
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 5, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR adds an RFC document (docs/workspace-rfc.md) proposing a Workspace abstraction that unifies scattered configuration state — Project, ServerConfig, Pipeline, the _state dict, and CLI options — into a single composable entity. No code is changed; this is a design document with a phased implementation plan.

Alignment

Strong fit. DESIGN.md establishes Pipeline as the build-time concept and Project as the SDK entry point, but says nothing about operational concerns (serving, buckets, runtime services). The RFC fills that gap without disturbing core invariants: artifacts stay immutable and content-addressed, Project retains all build/release/provenance logic, and Workspace is explicitly a composition layer that delegates rather than absorbs. The config-file-as-TOML decision is consistent with the Python-first principle — TOML describes deployment topology, not pipeline architecture. The distinction "pipeline is code, workspace is config" respects the CDK-vs-CloudFormation line drawn in DESIGN.md §4.1.

Observations

  1. [positive] The explicit constraint "Workspace is not a god object" with the delegation model (_project, _config, _runtime) is well-reasoned. The RFC preempts the most obvious architectural concern and documents it in the decisions log.

  2. [positive] Backward compatibility is handled carefully — re-exports from server/config.py, old format detection, precedence rules when both config files exist, transitional serve_from_config(). This is appropriate for a pre-1.0 project with existing users.

  3. [concern] WorkspaceState is an enum with five values computed from disk state, but the RFC never specifies how it's used besides description. If nothing gates on state (e.g., build() raising if state is FRESH and no pipeline is loaded), it's dead weight. If things do gate on it, those invariants should be specified here.

  4. [concern] The activate_runtime() method mutates Workspace in place, making it stateful. Combined with the module-level _workspace: Workspace | None in mcp_tools.py (Phase 3), this reintroduces the same global-mutable-singleton pattern that _state had. The RFC should acknowledge this is a lateral move for now, not an improvement to the concurrency model.

  5. [question] The RFC says buckets "may overlap" with Pipeline.source_dir but are "distinct concerns." What happens when synix build runs while the server's auto-builder is writing to a bucket that maps to source_dir? The single-process constraint is stated, but CLI-while-server-running is explicitly called out as using "SQLite WAL and filesystem refs." Bucket writes aren't covered by WAL — this needs a note about atomicity of source ingestion.

  6. [nit] The phased plan lists tests/unit/test_workspace_config.py in the verification section and files-changed table, but Phase 1 only mentions tests/unit/test_workspace.py. The second test file appears from nowhere.

  7. [question] open_workspace() vs open_project() as parallel public APIs creates a "which do I use?" decision for every consumer. The RFC documents guidance, but the README quickstart and SDK docs currently only show open_project(). Who updates those, and when?

  8. [positive] The test plan is thorough — unit tests for config parsing/precedence/discovery, e2e for full lifecycle, container smoke test, and full regression suite. This is above-average for an RFC.

  9. [nit] load_server_bindings() is a free function that parses "just the [server] section." This feels like it should be a method on WorkspaceConfig or part of the TOML parser, not a standalone factory — it introduces an asymmetry in the parsing API.

Verdict

This is a well-scoped RFC that addresses real configuration sprawl without compromising core build-system invariants — a good incremental step, pending clarification on state gating semantics and the mutable-singleton tradeoff.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 287 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-05T22:12:46Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 5, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium risk: this is “just an RFC,” but it proposes a user-facing config and API split that creates several one-way doors without showing the code that proves the migration is safe.

One-way doors

  1. Workspace as a new top-level public abstraction
    Hard to reverse because README/API/docs will teach users open_workspace() vs open_project(), and server/viewer/CLI will start depending on it. Safe only if the boundary is stable, minimal, and actually reflected in code/tests across SDK, CLI, server, and viewer.

  2. Config format split: synix.toml + [workspace]/[server] schema
    Hard to reverse because users will commit these files, deployment docs will ossify, and precedence rules become compatibility obligations. Safe only if old/new config migration is implemented, tested on real fixtures, and documented publicly.

  3. Config discovery precedence
    “First match wins,” especially synix.toml silently shadowing synix-server.toml, is behavioral API. Safe only if warnings are surfaced reliably in CLI/server logs and ambiguous setups are tested.

  4. Lifecycle state model (FRESH/CONFIGURED/BUILT/RELEASED/SERVING)
    Hard to reverse once UI/CLI/viewer expose it. Safe only if states are not used for correctness decisions and stale/invalid/corrupt states are handled explicitly.

Findings

  1. docs/workspace-rfc.md — introducing public API and config shape in an RFC-only PR
    This changes the project’s conceptual model (Workspace, synix.toml, open_workspace) but there are no corresponding updates to README, website copy, CLI docs, or DESIGN alignment. You are effectively pre-announcing an API contract without showing implementation. Severity: [warning]

  2. docs/workspace-rfc.mdWorkspaceState computed from disk state is underspecified for failure/corruption modes
    “HEAD ref points to a valid snapshot” and “at least one release receipt exists” sound clean, but there’s no mention of partial releases, corrupt receipts, missing projection outputs, or incompatible on-disk formats. The viewer/server will make assumptions from these states and misbehave in edge cases. Severity: [warning]

  3. docs/workspace-rfc.md — config precedence silently ignores synix-server.toml when both exist
    That is a migration footgun. In a deploy environment, an old file can remain present and operators think it is active when it is not. A logged warning is not enough if startup is non-interactive. This should probably be a hard error during the transition period, not silent precedence. Severity: [warning]

  4. docs/workspace-rfc.md — old/new format detection via presence of [workspace] is brittle
    “No mixed-format support” is nice on paper, but TOML files drift. A partial migration or typo creates ambiguous behavior. There is no validation spec for unknown keys, duplicate semantics, or conflicting project_dir/root inference. Severity: [warning]

  5. docs/workspace-rfc.md — single-process model conflicts with stated operational use
    README explicitly positions CLI, serve, MCP, direct SQLite access, and auto-build flows. This RFC says one workspace is active per process and punts cross-process coordination to “same as today.” That’s not a design resolution; it’s an admission that runtime coherence is still ad hoc. Severity: [warning]

  6. docs/workspace-rfc.mdWorkspace claims to be a thin wrapper but absorbs config ownership, runtime services, state model, discovery, and factories
    That is already a coordination object with policy. Calling it “not a god object” does not make it so. This increases coupling between SDK/server/viewer/CLI around one central abstraction before the code proves it can stay narrow. Severity: [warning]

  7. docs/workspace-rfc.md — divergence from DESIGN’s Python-first stance
    DESIGN is explicit that Python-first is core and TOML is export/interop, not primary interface. This RFC makes TOML discovery central for operations and introduces behavior based on config file presence. Maybe justified, but it is a design shift and should be called out as such. Severity: [minor]

  8. docs/workspace-rfc.md — state “SERVING” is process-local while other states are disk-derived
    Mixing ephemeral runtime state with durable capability state under one enum is confusing and likely to leak into UI/CLI semantics. A second process cannot observe SERVING reliably. Severity: [minor]

Missing

  • Actual code diff. This PR describes implementation files and tests, but none are present.
  • README/docs/website updates for Workspace, synix.toml, and config migration.
  • Explicit validation rules for malformed or mixed TOML.
  • Migration plan for existing deploys beyond “warning is logged.”
  • Concurrency and locking tests for server + CLI operating on the same workspace.
  • On-disk compatibility analysis: how Workspace interacts with existing .synix/ layouts and releases.

VerdictBlock: this proposes multiple compatibility-affecting abstractions and config behaviors, but without implementation, migration proof, or public doc updates it is not safe to merge even as design direction.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 287 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-05T22:12:46Z

- Replace config precedence chain with single well-known location:
  synix.toml at workspace root. No fallback, no discovery logic.
  Old format handled by one-time migration CLI.
- Add Phase 5: viewer shows workspace as first-class UI entity —
  name + state badge in header, config summary in Pipeline tab,
  workspace identity in /api/status response.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium risk: this is “just an RFC” in the diff, but it proposes user-facing config and API shapes that would be painful to unwind once implemented.

One-way doors

  1. Workspace as a public top-level abstraction
    Hard to reverse because open_workspace(), Workspace.state, Workspace.config, and delegation methods become SDK surface area and mental model. Safe only if you are sure Project vs Workspace is a durable split and not another temporary layering mistake.

  2. synix.toml as the canonical config file
    Hard to reverse because docs, deploy scripts, templates, and automation will bake in file name/location semantics. Safe only if you commit to a single-root config model and have a tested migration story from synix-server.toml.

  3. Lifecycle enum semantics (FRESH/CONFIGURED/BUILT/RELEASED/SERVING)
    Hard to change once UI/API/CLI expose them. The meanings are already fuzzy (“SERVING” is process-local while others are disk-derived). Safe only if these states are actually stable, mutually comprehensible, and useful across CLI, viewer, and server.

  4. Server/runtime tied to single active workspace per process
    Hard to undo because global singleton patterns in mcp_tools.py and viewer/server plumbing will spread. Safe only if you explicitly want to preclude multi-workspace hosting and have validated no existing code assumes reentrancy/test isolation.

Findings

  1. docs/workspace-rfc.md — introduces Workspace alongside Project without resolving overlapping authority
    Failure mode: two public entry points with partially overlapping capabilities guarantees drift. README already teaches open_project() for runtime access; now “operations” use open_workspace(). That split is conceptual, not enforceable, and users will pick the wrong one. Severity: [warning]

  2. docs/workspace-rfc.md — config ownership contradicts Python-first design
    DESIGN.md is explicit that Python is the primary interface and config-first is a non-goal. This RFC makes synix.toml required for serve/viewer and centralizes buckets/auto_build/vllm there. That’s a real shift in product direction, not a refactor. Severity: [warning]

  3. docs/workspace-rfc.mdWorkspaceState mixes persisted capability with ephemeral runtime
    SERVING is process-local, computed “on access,” and not persisted, while BUILT/RELEASED are disk-derived. That means the same workspace can report different states depending on which process inspects it. UI/API built on this will be misleading. Severity: [warning]

  4. docs/workspace-rfc.md — “single-process model” hand-waves cross-process coordination
    The RFC claims CLI/server concurrency stays “same as today” via SQLite WAL and filesystem refs, but runtime also includes queue, prompt store, build lock, vLLM manager. SQLite WAL does nothing for those. This is exactly where races and split-brain behavior hide. Severity: [critical]

  5. docs/workspace-rfc.md — no validation rules for config boundary
    synix.toml adds buckets, ports, paths, model names, allowed_hosts, etc., but the RFC says nothing about path normalization, root containment, conflicting bucket names, invalid glob patterns, port collisions, or missing pipeline files. You are adding a new boundary without specifying validation. Severity: [warning]

  6. docs/workspace-rfc.md — migration story is underspecified and internally inconsistent
    It says “no fallback chain” and also “TOML parser handling both old and new formats” plus “fallback to old path if synix-server.toml detected.” That is a fallback chain. You haven’t actually reduced complexity; you’ve renamed it. Severity: [warning]

  7. docs/workspace-rfc.md — viewer semantics are still release-centric while claiming workspace-centric identity
    Earlier context says viewer can’t start without a release; later phases say workspace awareness fixes this, but no behavior is defined for CONFIGURED/BUILT without release. Does viewer show pipeline metadata only? Empty state? Error? This is exactly the sort of “simple refactor” that breaks UX assumptions. Severity: [minor]

  8. docs/workspace-rfc.md — deploy file rename is user-facing but docs are not updated
    README and website currently teach project structure and commands but not synix.toml. If this lands, the public onboarding story changes. No documentation update is included. Severity: [warning]

Missing

  • Actual code diff. This is an RFC posing as a PR; there is nothing to verify against behavior.
  • Migration tests covering old/new config coexistence, malformed config, and repeated migration.
  • Explicit validation/error contracts for open_workspace(), load_pipeline(), and bucket_dir().
  • Concurrency tests for server + CLI access to the same workspace.
  • Documentation updates to README/website/integration docs if synix.toml becomes canonical.
  • Clarification of whether Workspace is internal scaffolding or supported SDK API.

VerdictBlock: this proposes multiple one-way public abstractions and config semantics, but it is still internally inconsistent and under-specified on migration, validation, and concurrency.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 308 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:00:41Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR adds an RFC document (docs/workspace-rfc.md) proposing a Workspace abstraction that unifies scattered configuration — Project, ServerConfig, Pipeline, the _state dict, and CLI options — into a single compositional entity. No code is changed; this is a design document with a phased implementation plan.

Alignment

Strong fit. DESIGN.md's build/release separation is preserved: Workspace delegates build/release to Project rather than absorbing that logic. The RFC explicitly respects the Python-first principle (config is TOML for operational concerns like ports and buckets, not for pipeline definition). The immutability and content-addressing of artifacts are untouched. The distinction between open_project() (SDK/scripts) and open_workspace() (operations) maps cleanly to DESIGN.md's separation between pipeline authors and deployment concerns — pipeline.py stays pure, operational context wraps around it.

Observations

  1. [positive] The "not a god object" constraint is stated upfront and enforced by design — Workspace holds no build logic, no transform execution. This is the right call for a composition layer.

  2. [positive] The phased implementation plan is unusually well-structured for an RFC. Phase 1 is purely additive with a gate (uv run release passes), which makes incremental review feasible.

  3. [concern] WorkspaceState is computed from disk state but the RFC doesn't specify the exact derivation rules. What makes something CONFIGURED vs FRESH? If a pipeline file exists but fails to parse, which state? These edge cases will matter for the viewer's state badge (Phase 5) and should be pinned down before implementation.

  4. [concern] The RFC introduces synix.toml as the single config file but doesn't address synix init — does the scaffold command create synix.toml? The README's quick start (uvx synix init my-project) currently produces no such file. If Workspace requires it for serve/viewer but not for build, the two-tier experience needs documenting in the RFC itself.

  5. [question] activate_runtime() sets WorkspaceRuntime on the workspace instance. In Phase 3, mcp_tools.py uses a module-level _workspace. What happens if activate_runtime() is called twice (e.g., server restart without process restart)? The RFC says single-process model, but the lock and queue reinitialization semantics aren't specified.

  6. [question] The dependency direction is workspace.pyserver/config.py, with ServerConfig composing WorkspaceConfig + ServerBindings. But serve_from_config() is described as transitional. Once removed, does ServerConfig itself disappear? The RFC should state the end state explicitly.

  7. [nit] The files changed table lists tests/unit/test_workspace_config.py (~100 lines) but the Phase 1 description only mentions test_workspace.py. The config-specific test file appears without introduction.

  8. [nit] bucket_dir(name) -> Path is listed in the API but bucket semantics ("buckets write to source_dir") are described as potentially overlapping with Pipeline.source_dir. A sentence on conflict resolution (what if both point to different dirs?) would help.

  9. [positive] The decisions log at the bottom captures real review feedback with concrete resolutions. This is good institutional practice for a solo-maintainer project.

  10. [positive] Test plan covers unit, e2e, regression, container smoke test, and backward compat — comprehensive for an RFC.

Verdict

This is a well-scoped RFC that addresses real configuration sprawl without violating core architectural principles; it's a good incremental step, pending clarification of state derivation rules and the synix init interaction.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 308 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:00:45Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium risk: this is “just an RFC,” but it proposes new user-facing config and API names that are easy to ossify and it cuts across CLI, server, viewer, and config loading.

One-way doors

  1. Workspace as a public top-level abstraction
    Hard to reverse because open_workspace(), Workspace, and synix.toml become the user’s mental model and likely land in docs/examples/plugins. Safe only if you’re confident this is the stable unit of composition across SDK, CLI, server, and viewer—not just a cleanup for current server wiring.

  2. Config file rename to synix.toml
    Hard to reverse because file names become tooling conventions, templates, container images, and docs defaults. Safe only if migration from synix-server.toml is implemented, tested, and documented, with clear precedence rules during transition.

  3. Single-process / one active workspace per process model
    Hard to reverse because internals and server APIs will start assuming global singleton state. Safe only if you explicitly prove this won’t block multi-tenant serving, parallel test isolation, or embedding the server in larger processes.

Findings

  1. docs/workspace-rfc.md — public API drift from project’s documented surface
    README/DESIGN present Project/open_project and a build/release model centered on projects and pipelines. This RFC introduces Workspace as “the first-class unit” without reconciling whether project or workspace is the primary model. That is conceptual churn at the API boundary. Severity: [warning]

  2. docs/workspace-rfc.md — config semantics contradict “Python-first, not config-first”
    DESIGN is explicit that Python is the primary interface and config should not become the control plane. This RFC makes synix.toml required for serve/viewer and loads buckets, auto_build, vllm, server bindings, and pipeline path from TOML. That’s not a small detail; it’s a second declaration system. Severity: [warning]

  3. docs/workspace-rfc.mdWorkspaceState is underspecified and likely misleading
    States are “computed, never persisted,” but CONFIGURED = pipeline loaded or buckets defined mixes unrelated concerns, and BUILT = HEAD ref points to valid snapshot / RELEASED = at least one release receipt exists ignore branch/release targeting. Viewer/CLI code will depend on these states and users will read meaning into them. Severity: [warning]

  4. docs/workspace-rfc.md — singleton runtime still leaks global mutable state
    Replacing _state dict with _workspace: Workspace | None is not an architectural improvement; it’s the same process-global coupling with a nicer name. This preserves test interference and makes concurrent serving/embed scenarios impossible. Severity: [warning]

  5. docs/workspace-rfc.md — file layout assumptions are too rigid
    The RFC hardcodes pipeline.py, sources/, .synix/, synix.toml at root and even bucket directories under sources/. README currently emphasizes editable Python pipelines and decoupled build/release, not a mandatory workspace skeleton. This will create edge cases for existing projects with nonstandard layouts. Severity: [minor]

  6. docs/workspace-rfc.md — migration story is hand-waved for a cross-cutting change
    “One-time migration: synix migrate-config” is proposed, but no compatibility behavior is specified when both files exist, when CLI --config is passed, or how old deploy/container flows behave. This is exactly where users get stranded. Severity: [warning]

  7. docs/workspace-rfc.md — status/staleness logic is not credible
    “Staleness is a separate concern (detectable by comparing HEAD ref timestamp to source mtimes)” is a design smell. The rest of Synix is content-addressed/materialization-key driven; using mtimes here is a regression to weaker semantics and will misreport under restores, copies, generated sources, or bucket ingestion. Severity: [warning]

  8. docs/workspace-rfc.md — direct coupling of workspace to runtime services
    WorkspaceRuntime bakes in queue, prompt store, vLLM manager, and build lock. That means the “thin composition layer” is already server-specific. This leaks implementation details into the core abstraction and will be painful if runtime backends change. Severity: [warning]

Missing

  • No actual diff implementing this—only an RFC. That means no proof the design survives contact with real imports, circular dependencies, or current tests.
  • No migration plan for existing docs/templates/website/CLI help, despite proposing synix.toml, open_workspace(), and changed serve behavior.
  • No explicit compatibility matrix for old projects without synix.toml.
  • No concurrency/failure-mode analysis for queue.db, prompts.db, refs, and release operations when CLI and server touch the same workspace.
  • No explanation of how this interacts with the documented build/release separation and content-addressed provenance model.

VerdictBlock: this is a broad architectural proposal with public API and config one-way doors, and it currently conflicts with the project’s own Python-first story while keeping the same singleton-state fragility underneath.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 392 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:03:51Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR adds an RFC document (docs/workspace-rfc.md) proposing a Workspace abstraction that unifies scattered configuration — Project, ServerConfig, Pipeline, the _state dict, and CLI options — into a single composition layer. No code is changed; this is a design document with a phased implementation plan.

Alignment

Good fit with the vision. DESIGN.md establishes Pipeline as the build-time abstraction but says nothing about how operational concerns (server, buckets, viewer, ingestion queue) bind together. The RFC fills that gap without disturbing the build-system core. Key principles are respected:

  • Composition, not inheritanceWorkspace wraps Project, keeping build semantics untouched. Artifacts remain content-addressed, the DAG is unaffected.
  • Python-firstopen_workspace() / open_project() split preserves the scriptable SDK path (DESIGN.md §4.1) while adding an operational layer.
  • Provenance/immutability — no changes to artifact storage or materialization keys.
  • "Architecture is a runtime concern"WorkspaceState as computed-not-persisted and the config/pipeline separation support iterating on the pipeline without touching operational config.

The introduction of synix.toml is the one tension point: DESIGN.md explicitly chose Python over TOML for pipeline definition. The RFC is careful to limit TOML to operational config (buckets, ports, vLLM), not pipeline definition, which stays in pipeline.py. This is a reasonable boundary.

Observations

  1. [positive] The "when to use which" guidance (Project for scripts/tests, Workspace for operations) is well-drawn and should reduce confusion from having two entry points.

  2. [positive] The phased implementation plan is genuinely additive — Phase 1 changes nothing existing, each subsequent phase has a clear blast radius. Good for a pre-1.0 project under rapid iteration.

  3. [concern] The WorkspaceState enum conflates orthogonal capabilities. A workspace can be BUILT but also SERVING — these aren't mutually exclusive stages. The doc says states describe "capability," but the enum is linear. If SERVING implies RELEASED implies BUILT, say so explicitly. If not, this should be a flags/set model.

  4. [concern] _workspace: Workspace | None = None as a module-level global (Phase 3, mcp_tools.py) is the same pattern as the _state dict it replaces — a typed version of a global singleton. The RFC acknowledges "single-process model" but doesn't address what happens if two CLI commands race (e.g., synix build while synix serve holds _workspace). Current SQLite WAL is mentioned but the lock semantics around WorkspaceRuntime.build_lock spanning processes aren't specified.

  5. [question] serve_from_config() is marked "transitional — kept for one release cycle." For a pre-1.0 project, is a deprecation cycle necessary? Consider just cutting it.

  6. [question] The RFC mentions synix migrate-config for old synix-server.toml files but doesn't spec the migration behavior (what if both files exist? what if the old file has fields the new format doesn't support?). The TOML parsing tests list "both files present" as a case — good — but the RFC text should specify the expected behavior.

  7. [nit] The bucket_dir(name) -> Path API resolves bucket paths but the RFC doesn't specify behavior when a bucket name doesn't exist in config. Raise? Return None? Matters for the MCP tools migration.

  8. [positive] Test plan is thorough — unit tests for state computation, TOML parsing, and delegation; e2e for the full lifecycle; plus container smoke tests. This is better coverage planning than most RFCs.

  9. [nit] The disk layout shows queue.db and prompts.db inside .synix/. These are runtime/server artifacts living alongside content-addressed build objects. Worth a brief note on why this is acceptable (or whether they should live elsewhere).

Verdict

A well-scoped RFC that addresses real operational friction without touching the build-system core — good incremental step, pending clarification on state enum semantics and cross-process locking.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 392 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:04:01Z

- Phase 6: every template gets synix.toml, goldens regenerated
- Phase 7: two e2e tests — full workspace lifecycle + server integration
- Phase 8: deploy to /srv/synix/agent-memory/, rename config
- synix init generates complete workspace scaffold (synix.toml included)
- Updated files table and verification checklist
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium-high risk: this is an architectural RFC that introduces a new top-level abstraction and config format, but the PR as shown is docs-only, so the risk is mostly in the one-way decisions it is trying to bless without implementation proof.

One-way doors

  1. Workspace as a public top-level abstraction
    Hard to reverse because it changes the user mental model, SDK entrypoints (open_project vs open_workspace), and likely future docs/examples. Safe only if there is a clear, enforced boundary and evidence it does not become a second overlapping API surface.

  2. synix.toml as the canonical config file at workspace root
    Hard to reverse because templates, deploy layouts, CLI behavior, and user repos will all standardize on it. Safe only if migration from synix-server.toml is implemented, tested, and documented, including mixed-version behavior.

  3. synix init creating .synix/ eagerly
    Hard to reverse because it fixes disk layout and bootstrap semantics in every new project. Safe only if the project guarantees .synix/ can be safely initialized before any build and this doesn’t conflict with content-addressed state/versioning assumptions.

  4. Single-process, one-active-workspace-per-process model
    Hard to reverse because server/runtime internals will bake in globals and process-local state. Safe only if there is an explicit compatibility story for concurrent CLI/server access and a plan to remove globals later.

Findings

  1. docs/workspace-rfc.md — introducing WorkspaceState lifecycle enum
    This is leaky and underspecified. “CONFIGURED” means “pipeline loaded or buckets defined”; “BUILT” means HEAD exists; “RELEASED” means any release receipt exists. These are not orthogonal and will drift from reality fast. You are creating a user-visible state machine before defining the invariant source of truth. Severity: [warning]

  2. docs/workspace-rfc.md — explicit split between Project and Workspace
    The doc claims “Project stays unchanged” while also routing server, viewer, CLI, templates, and init through Workspace. That is de facto a second primary abstraction. Expect duplication, partial feature parity, and bugs where CLI/SDK disagree on discovery/loading. This conflicts with the README’s simpler “open_project/build/release/search” story. Severity: [warning]

  3. docs/workspace-rfc.mdsynix.toml required for serve/viewer, but bare mode still exists
    This creates two operational modes with different discovery semantics. “No fallback chain” sounds simple, but now every command must decide whether it is workspace-aware, bare-project-compatible, or both. Hidden complexity is pushed into command behavior, not removed. Severity: [warning]

  4. docs/workspace-rfc.md_workspace: Workspace | None = None replacing _state
    This is still global mutable process state, just renamed and typed. It does not solve the underlying coupling/race issue the RFC complains about. The design doc emphasizes build/release separation and immutable snapshots; process-global runtime state cuts across that. Severity: [warning]

  5. docs/workspace-rfc.md — eager runtime composition (queue.db, prompts.db, vLLM, locks) under .synix/
    You are further mixing build state, release state, ingestion queue state, and runtime service state under one filesystem namespace. That increases cross-module coupling and raises corruption/locking risk if CLI and server touch the same workspace concurrently. The RFC handwaves “same as today” instead of defining guarantees. Severity: [critical]

  6. docs/workspace-rfc.md — template migration and golden regeneration
    Regenerating all goldens because of workspace output is a red flag. If adding a thin composition layer changes demo outputs, either the abstraction is not thin or test fixtures are too coupled to incidental formatting. Severity: [minor]

  7. docs/workspace-rfc.md — status/staleness based on timestamps
    The RFC suggests stale detection by comparing HEAD ref timestamp to source mtimes. That is a direct mismatch with the project’s stated content-addressed/materialization-key principles. mtimes are not a safe provenance signal. Severity: [warning]

Missing

  • Actual code diff. This is an RFC proposing invasive behavior changes without implementation to review.
  • Migration details for existing users: exact old/new TOML schema, failure behavior, rollback path.
  • Concurrency/locking tests for server + CLI touching .synix/, queue.db, and releases simultaneously.
  • Documentation updates to README/website/CLI docs, since synix init, serve behavior, and config expectations are user-facing.
  • Boundary validation rules: invalid TOML, missing pipeline path, bucket paths outside workspace root, malformed server section.
  • Explanation of how this fits with the design doc’s Python-first stance; this materially increases TOML’s importance.

VerdictBlock: too many architectural decisions are being committed in docs without implementation proof, concurrency guarantees, or reconciliation with the existing Project/Python-first model.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 444 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:06:49Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR adds an RFC document (docs/workspace-rfc.md) proposing a Workspace abstraction that unifies scattered configuration (Project, ServerConfig, Pipeline, _state dict, CLI options) into a single composable entry point. No code is changed — this is a design document with an 8-phase implementation plan.

Alignment

Good fit. DESIGN.md's build/release separation is preserved — Workspace delegates build/release to Project rather than absorbing that logic. The composition-not-inheritance approach ("Not a subclass — Project stays unchanged") respects the existing layering. Content-addressed artifacts, immutable snapshots, and the DAG model are untouched. The synix.toml config file is scoped to operational concerns (buckets, vllm, ports), not pipeline definition — consistent with the Python-first design decision (Appendix C) that pipeline logic belongs in code, not config.

Observations

  1. [positive] The constraint section is unusually disciplined: "not a god object," single-process model, no build logic in Workspace. This preempts the most obvious architectural risk.

  2. [positive] Dual entry points (open_project vs open_workspace) with clear guidance preserves backward compatibility for SDK consumers while giving operators what they need. This matches the DESIGN.md separation between pipeline authors and deployment.

  3. [concern] WorkspaceState is computed from disk state on every access. With .synix/ containing potentially thousands of objects, the RELEASED check (scanning for release receipts) could be slow. The RFC doesn't specify how state is derived — is it checking for file existence, querying SQLite, or walking directories? Worth specifying the exact checks to avoid an O(n) surprise.

  4. [question] Phase 1 lists tests/e2e/test_workspace_e2e.py at ~100 lines, but Phase 7 also introduces tests/e2e/test_workspace_e2e.py at ~150 lines with more detailed scenarios. Are these the same file? The duplication suggests the phasing was revised without reconciling. The "Files changed" table lists 150 lines, so Phase 1's reference may be stale.

  5. [concern] The serve_from_config() adapter is marked "transitional — kept for one release cycle, then removed," but for a pre-1.0 project with no stability guarantees, a deprecation cycle adds complexity. Consider just making the breaking change in one step.

  6. [question] Buckets are defined as "a workspace concept, not a pipeline concept," but Pipeline.source_dir is how the pipeline reads sources. The RFC acknowledges "these may overlap" but doesn't specify who owns the mapping. If a bucket writes to sources/sessions/ and source_dir is ./sources/, does the pipeline auto-discover subdirectories? This seam needs a concrete contract.

  7. [nit] The RFC uses both "~250 lines" (Phase 1 description) and "~250 lines" (files changed table) for workspace.py, which is consistent, but the total new test code (~500 lines across 4 files) is substantial for a "thin composition layer." Not necessarily wrong, but worth noting that testing the delegation and state machine may be more complex than the abstraction itself.

  8. [positive] The decisions log at the bottom captures review feedback with explicit resolutions — good practice for an RFC that will be referenced during implementation.

  9. [nit] Phase 8 (deploy config rename) is a production deployment change bundled with a library abstraction RFC. These have different risk profiles and review audiences. Consider splitting.

Verdict

A well-scoped RFC that addresses real configuration sprawl without compromising the core build-system model — good incremental step, ready to guide implementation once the bucket/source_dir contract (observation 6) and state computation cost (observation 3) are clarified.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 444 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:06:57Z

ViewerState now accepts release=None and discovers releases via
before_request hook. Viewer starts immediately in "no release" mode
and serves a 503 on data endpoints until the first build completes.

- ViewerState.has_release property + try_discover_release() method
- _require_release() guard on data endpoints (layers, artifacts,
  search, lineage) — returns 503 with clear message
- serve.py: run_viewer passes release=None when no release exists,
  vLLM startup is non-blocking (asyncio.create_task)
- viewer/__init__.py: serve() and create_app() accept release=None
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Moderate risk: small code diff, but it quietly changes a user-facing server/viewer contract and introduces polling-style release discovery without clear concurrency or UX guarantees.

One-way doors

  1. Viewer can now start without a release

    • Hard to reverse because this becomes an operational expectation for synix serve and viewer startup behavior. Scripts, docs, and users will depend on “serve first, build later.”
    • Safe to merge only if the empty-state contract is explicitly documented, tested end-to-end, and stable across API/UI routes.
  2. /api/status semantics changed

    • Previously effectively implied a loaded release; now loaded can be false and release can be null. Frontends and automation may already assume both are always present.
    • Safe to merge only if all consumers are audited and updated, and compatibility expectations are documented.

Findings

  1. src/synix/viewer/server.pybefore_request re-opens project and scans releases on every request

    • @app.before_request calls state.try_discover_release(), which imports synix, re-opens the project from disk, and lists releases until one exists. That is hidden I/O on every request in the empty state. It’s inefficient, noisy under load, and creates timing-dependent behavior if releases appear mid-request burst.
    • Severity: [warning]
  2. src/synix/viewer/server.py — no synchronization around mutating state.release / state.project

    • try_discover_release() mutates shared viewer state from request handlers with no lock. Flask dev server is often single-threaded, but that is not a safe assumption for deployments or tests. Race is plausible if concurrent requests hit before first release appears.
    • Severity: [warning]
  3. src/synix/viewer/server.py — API contract regression on empty state

    • All data routes now return 503 until a release exists. That’s a behavior change, not just a feature. If the frontend previously loaded /api/layers or /api/search unconditionally, you’ve moved failure from startup time to runtime. That may be acceptable, but it is a breaking change in API semantics.
    • Severity: [warning]
  4. src/synix/server/serve.py + src/synix/viewer/__init__.py — release selection policy is hardcoded twice

    • Both paths assume "local" is preferred, otherwise first release. That is magic policy baked into server/viewer code rather than configuration. If release naming changes, or multiple releases exist, behavior is arbitrary and user-visible.
    • Severity: [minor]
  5. src/synix/viewer/server.py — caches are not invalidated when release is discovered

    • _layer_cache, _children_index, and search cache are initialized before release exists, then a release can be attached later. Today that happens only once from None -> release, so it may appear fine, but the design is fragile if release switching is ever added. Hidden statefulness for little gain.
    • Severity: [minor]
  6. docs/workspace-rfc.md included in PR without implementation

    • The RFC describes a broad architecture shift (Workspace, config migration, template changes, CLI changes), but the actual diff only tweaks viewer startup. That creates misleading review context and suggests the PR is being justified by future design rather than current code.
    • Severity: [minor]
  7. No validation of project.project_root access in release discovery path

    • open_project(str(self.project.project_root)) assumes project_root is valid and stable. If the project was opened from a deleted/moved path or with partial initialization, release discovery degrades into repeated exceptions on every request.
    • Severity: [minor]

Missing

  • Tests for the new empty-state behavior:
    • viewer starts with no releases
    • /api/status returns loaded=false, release=null
    • data endpoints return 503 before release exists
    • release becomes discoverable after creation without restarting viewer
  • Documentation updates for viewer/server behavior. README and site still present release-first workflows; nothing describes “discovery mode” or changed API semantics.
  • Any explicit frontend/UI handling shown for 503 or “waiting for first build” state. The backend claims that mode exists; the diff does not prove the UI actually supports it.
  • Throttling/backoff for discovery attempts. Polling the filesystem/project on every request is crude.

VerdictShip with fixes: the idea is reasonable, but the API behavior change and unsynchronized per-request discovery need tests and at least basic hardening before merge.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 682 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:11:43Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR does two things: (1) introduces an RFC document proposing a Workspace abstraction that unifies scattered configuration (Project, ServerConfig, Pipeline, _state dict, CLI options) into a single composable entity, and (2) ships the immediate prerequisite — making the viewer start without a release and discover one lazily via a before_request hook.

Alignment

The Workspace RFC is well-aligned with the build-system mental model. DESIGN.md's build/release separation (§1.5) is preserved — Workspace delegates build/release to Project rather than absorbing that logic. The RFC explicitly calls out that Workspace holds no build logic, no transform execution, no search indexing, which respects the immutable-artifacts-and-DAG architecture. The WorkspaceState enum (FRESH → CONFIGURED → BUILT → RELEASED → SERVING) maps cleanly to the pipeline lifecycle in §4.4. Config-as-TOML for operational concerns while keeping pipeline-as-Python honors the Python-first decision (§4.1, Appendix C). Buckets as a workspace concept distinct from pipeline sources is a sound separation that keeps the pipeline's source fingerprinting clean.

Observations

  1. [positive] viewer/server.py: The _require_release() pattern is clean — consistent 503 with a human-readable message across all data endpoints, while /api/status degrades gracefully with loaded: false. Good incremental UX.

  2. [concern] viewer/server.py try_discover_release(): Calls _synix.open_project() on every failed discovery attempt (i.e., every request while no release exists). This re-reads .synix/ state from disk. Under load (or a monitoring tool polling /api/status), this could be expensive. Consider a cooldown — e.g., only re-check every N seconds.

  3. [concern] try_discover_release() hardcodes "local" as the preferred release name. This duplicates the _RELEASE_NAME constant already used in serve.py. If that constant changes, these will diverge silently. Should be a shared constant or passed in at construction.

  4. [question] try_discover_release() does import synix as _synix inside the method body rather than using self.project directly to re-open. The comment says "Re-open to pick up refs created since server start" — is Project not able to see new refs after initial open? If it uses filesystem reads (which it appears to), a re-open shouldn't be necessary. If there's caching, this is worth documenting.

  5. [nit] viewer/__init__.py: The release parameter changed from required to optional (Release | None = None), but there's no guard ensuring at least one of release or project is provided. create_app(None, project=None) would produce a viewer that can never discover anything — a silent misconfiguration.

  6. [positive] The RFC document is thorough — phased implementation, clear file-change inventory, explicit decisions log, verification steps. The "when to use which" guidance (Project for SDK/scripts, Workspace for operations) is the right call for avoiding a god object.

  7. [question] The RFC lists tests/e2e/test_workspace_e2e.py in both Phase 1 and Phase 7 with different line counts (100 vs 150). Is Phase 7 an expansion of Phase 1's test, or are these duplicates?

  8. [concern] The smoke test file change (deploy/smoke-test/sources/documents/smoke-test.md) looks like an incidental timestamp update baked into a prior test run. This shouldn't be committed with the PR — it'll cause unnecessary cache invalidation on that source artifact and muddies the diff.

  9. [positive] The serve.py change correctly inverts the viewer startup logic: instead of refusing to start without a release (return early), it starts unconditionally and passes release=None. This directly enables the Workspace RFC's Phase 4/5 goals without waiting for the full abstraction.

  10. [nit] _require_release() returns either None or a tuple. The call pattern err = _require_release(); if err: return err works but is non-obvious — a decorator or Flask abort(503) would be more idiomatic.

Verdict

Good incremental step: the code change is a focused, correct prerequisite for the Workspace abstraction, and the RFC is well-structured and architecturally sound. Address the discovery polling cost (#2) and the hardcoded release name (#3) before merge.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 682 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:11:59Z

New module src/synix/workspace.py:
- Workspace class wrapping Project via composition
- WorkspaceConfig, BucketConfig, BuildQueueConfig, VLLMConfig
- WorkspaceState enum (FRESH/CONFIGURED/BUILT/RELEASED/SERVING)
- WorkspaceRuntime dataclass (replaces _state dict pattern)
- ServerBindings dataclass (port/host config)
- open_workspace() and init_workspace() factories
- synix.toml parser

31 unit tests covering identity, state transitions, delegation,
bucket resolution, runtime activation, TOML parsing, and factories.
1414 total tests pass with no regressions.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Moderate risk: the viewer change is small, but Workspace introduces a new public root abstraction and config format with several hard-to-reverse edges and some design/doc mismatches.

One-way doors

  1. Workspace as public top-level API (src/synix/__init__.py) — Exporting Workspace, WorkspaceConfig, WorkspaceState, init_workspace, and open_workspace makes this part of the supported mental model immediately. Reversing later means breaking imports, docs, templates, and examples. Safe only if the team is confident this abstraction survives contact with CLI, server, viewer, and SDK without another rename/split in the next few releases.
  2. synix.toml as canonical workspace config — File name, location, and schema ([workspace], [buckets], [auto_build], [vllm], [server]) will get baked into templates, deploy setups, and operator docs. Renaming/restructuring later is migration work. Safe only if migration semantics from synix-server.toml are implemented, documented, and tested end-to-end.
  3. Lifecycle state enum semanticsFRESH/CONFIGURED/BUILT/RELEASED/SERVING will leak into UI, API responses, tests, and likely user automation. If the state model is incomplete or misleading, changing it later is painful. Safe only if states are proven accurate under real edge cases and don’t conflate “loaded pipeline”, “has config”, and “has valid build”.

Findings

  1. src/synix/workspace.py / state + _has_snapshots() checks refs/heads/main
    This appears inconsistent with the project’s documented build/release model, which talks about HEAD refs and immutable snapshots, not “main”. A built workspace could be misclassified as FRESH or CONFIGURED if builds don’t populate refs/heads/main. That breaks status/UI/server behavior. Severity: [critical]

  2. src/synix/workspace.py / load_pipeline() eagerly resolves root / pipeline_path without existence validation
    Missing or bad pipeline_path becomes a downstream failure from Project.load_pipeline, with unclear boundary validation. This is exactly the kind of config-edge failure that should be caught at the workspace layer. Severity: [warning]

  3. src/synix/workspace.py / WorkspaceState.CONFIGURED depends on project.pipeline is not None or buckets
    This does not match the RFC text or user expectation. A workspace with synix.toml and pipeline_path but no loaded pipeline is treated as FRESH unless buckets exist. So “configured” depends on execution order, not disk state. That’s a leaky abstraction and likely wrong for status commands/UI. Severity: [warning]

  4. src/synix/workspace.py / WorkspaceRuntime and Workspace use Any heavily
    This punts all interface guarantees on the new central abstraction. If this is meant to replace the _state glue bag, typing it as Any preserves the same hidden coupling, just wrapped in a class. Severity: [minor]

  5. src/synix/__init__.py exports internal-ish config types
    WorkspaceConfig and WorkspaceState are now top-level public API, but there’s no evidence they’re stable or even fully integrated elsewhere. You’re publishing implementation detail before proving it. Severity: [warning]

  6. src/synix/viewer/server.py / before_request re-opens the project on every request until a release appears
    This is cheap at small scale, but it’s still repeated filesystem/TOML/open logic on the request path. Under a polling frontend, “waiting for first build” becomes a tight loop of disk activity. At minimum this wants throttling or memoized backoff. Severity: [minor]

  7. src/synix/viewer/server.py / 503 behavior change for all data endpoints
    This is a user-visible API change. Previously viewer startup failed early with no release; now it serves and endpoints return 503 until discovery succeeds. I don’t see docs or frontend contract updates in the diff. Severity: [warning]

  8. tests/unit/test_workspace.py / test writes to /tmp/test-abs-bucket-ws
    Non-isolated filesystem usage in unit tests is sloppy and can collide across runs or environments. Use tmp_path. Severity: [minor]

  9. tests/unit/test_vllm_manager.py changes startup timeout default from 120 to 300 with no corresponding docs/update rationale
    This is behavioral drift in operational startup characteristics. Longer startup impacts UX and deployment timeouts. The RFC mentions it only implicitly through new config defaults; no migration note or deploy doc change is shown. Severity: [minor]

Missing

  • No implementation or tests for old synix-server.toml migration despite the RFC claiming a one-time migration path.
  • No tests covering the documented release model vs Workspace.state detection (HEAD, invalid receipt, corrupt refs, no refs/heads/main).
  • No tests for viewer request behavior before/after first release, especially repeated polling and transition from 503 to ready.
  • No documentation updates to README/CLI docs for open_workspace, init_workspace, synix.toml, or changed viewer startup semantics.
  • No validation tests for malformed TOML types (allowed_hosts non-list, invalid bucket entries, bad window, etc.).

VerdictShip with fixes: the viewer change is fine, but the new Workspace abstraction has at least one likely incorrect state-detection bug and is being published as API before the migration, validation, and documentation are in place.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,418 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:16:31Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR introduces the Workspace abstraction — a composition layer above Project that unifies configuration (synix.toml), bucket definitions, lifecycle state, and runtime services into a single entity. It also makes the viewer resilient to starting without a release, discovering one lazily. The RFC document and workspace.py implementation are the core additions.

Alignment

Strong fit. DESIGN.md envisions Project as a build-time concept, but the operational surface (serve, viewer, buckets, vllm) had no unifying entity — config was scattered across _state dicts and ServerConfig. Workspace fills this gap without violating core principles: artifacts remain content-addressed in .synix/, the DAG is untouched, provenance chains are unaffected, and Project retains all build logic. The explicit constraint "Workspace is not a god object" directly echoes DESIGN.md's separation of concerns. The open_project() / open_workspace() split maps cleanly to the DESIGN.md distinction between pipeline authors (Python-first SDK) and operators (deployment/serving).

Observations

  1. [positive] workspace.py — The config types (BucketConfig, VLLMConfig, etc.) are canonical here with server/config.py re-exporting. Correct dependency direction; eliminates the backwards coupling the RFC identifies.

  2. [positive] server.py viewer startup — Changing from "no releases → bail out" to "start in discovery mode" is a real UX improvement. The before_request hook with try_discover_release() is a clean pattern for lazy binding.

  3. [concern] server.py:try_discover_release — Re-opens the entire project on every request when no release exists (_synix.open_project(str(self.project.project_root))). Under load or with many concurrent requests, this could be expensive. Consider a cooldown or timestamp check to avoid redundant re-discovery on every request.

  4. [concern] workspace.py:_has_snapshots — Imports RefStore inside the method and catches all exceptions with a bare except Exception. This silently swallows real errors (corrupt refs, permission issues) and would make debugging state misclassification difficult. At minimum, log the exception.

  5. [concern] workspace.py:WorkspaceRuntime — All fields typed as Any. The RFC names concrete types (DocumentQueue, PromptStore, asyncio.Lock). Even if you avoid the import to prevent circular deps, a TYPE_CHECKING guard with proper types would preserve safety and IDE support.

  6. [question] VLLMConfig.startup_timeout changed from 120 to 300 (visible in the test_vllm_manager.py update). This is a behavioral change to an existing default — was this intentional or a side effect of moving the canonical definition? If intentional, it should be documented.

  7. [nit] _load_config returns WorkspaceConfig | None, but Workspace.__init__ falls back to WorkspaceConfig() on None. The factory open_workspace passes this None through. This means a bare workspace has a config with auto_build.enabled=True — probably not intended for a project with no synix.toml.

  8. [positive] Test coverage is thorough: identity, all five lifecycle states, bucket resolution (relative, absolute, missing), TOML parsing (full, minimal, defaults), factories (bare, with toml, explicit path, missing raises). The mock_llm fixture for build/release state tests shows integration-level confidence.

  9. [question] The RFC describes 8 implementation phases, but the diff only delivers Phase 1 (workspace.py + viewer changes). The RFC is committed as docs/workspace-rfc.md. Is this intentional — land the RFC and Phase 1 together, with subsequent phases as follow-up PRs? If so, the phasing should be noted in the PR description.

  10. [nit] test_bucket_dir_absolute creates a project at /tmp/test-abs-bucket-ws without using tmp_path, leaving test debris on failure.

Verdict

This is a well-scoped foundational PR — it introduces the right abstraction at the right layer, with solid tests and no disruption to the build-system core. The lazy viewer discovery is a tangible UX win. Address the per-request re-discovery cost and the silent exception swallowing before merge.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,418 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:16:41Z

6 tests covering:
- init_workspace creates complete scaffold (synix.toml, .synix/, sources/)
- Full lifecycle: FRESH → CONFIGURED → BUILT → RELEASED with search
- Reopen workspace preserves state across process restart
- Custom synix.toml with buckets and workspace name
- Bare workspace without synix.toml
- Build + release produces accessible artifacts

2116 total tests pass.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium-high risk: this looks like an additive “workspace” abstraction, but it quietly changes public API, startup behavior, config semantics, and viewer lifecycle with weak compatibility proof.

One-way doors

  1. Workspace exported from synix.__init__ — This makes Workspace, WorkspaceConfig, WorkspaceState, open_workspace, and init_workspace part of the top-level public API. Hard to reverse because users will import these names directly. Safe only if you’re prepared to support them through 1.0 or clearly mark them experimental in docs and API policy.
  2. synix.toml as canonical config filename — Naming and layout become user mental model and template contract. Reversing later means migration tooling and broken automation. Safe only if CLI/docs/templates/deploy path are updated atomically and backward compatibility for synix-server.toml is actually implemented, not just promised in an RFC.
  3. Viewer supports release=None startup — This changes server/viewer operational semantics from “must have release” to “eventually consistent discovery.” Hard to back out once operators rely on it. Safe only if race conditions, refresh semantics, and API contract changes are tested and documented.

Findings

  1. src/synix/workspace.py, _has_snapshots() reads refs/heads/main
    Design doc and README are HEAD/snapshot oriented (release HEAD --to local), but state detection hardcodes a branch ref path. If builds only advance HEAD or non-main refs, Workspace.state lies. That infects UI/CLI decisions. Severity: [warning]

  2. src/synix/workspace.py, open_workspace() returns Workspace(project, config) where config may be None, but config property is typed WorkspaceConfig
    Internally you normalize to WorkspaceConfig() in __init__, but _load_config() returns None and tests assert “config is not None.” The type/API story is sloppy and will confuse callers about bare mode vs configured mode. Severity: [minor]

  3. src/synix/workspace.py, init_workspace() claims to create pipeline.py and source directories but only guarantees synix.toml and sources/
    The docstring and RFC overstate behavior. You delegate to sdk_init() and assume it scaffolds the rest. If that changes, this abstraction breaks silently. Severity: [warning]

  4. src/synix/workspace.py, _parse_toml() does unchecked coercion from TOML to ints/floats/lists
    No schema validation at the config boundary. Bad user config yields generic ValueError/TypeError with poor context. This is exactly where validation should exist. Severity: [warning]

  5. src/synix/workspace.py, load_server_bindings() defaults silently when config file missing
    Operational misconfiguration becomes “bind to defaults and hope.” For serve, silently accepting the wrong config path is dangerous. Severity: [warning]

  6. src/synix/viewer/server.py, @app.before_request reopens project on every request until release exists
    That’s hidden polling via request path. It adds repeated filesystem work and bakes request-driven discovery into correctness. Under load or with many 503s, you get needless churn. At minimum cache/throttle attempts. Severity: [warning]

  7. src/synix/viewer/server.py, API contract changed to 503 for most endpoints pre-release
    This is user-visible behavior change. No docs/update noted in README/website/integration docs, and no compatibility handling for clients expecting empty data vs errors. Severity: [warning]

  8. src/synix/server/serve.py, viewer now starts without release but still opens project directly, not via Workspace
    The PR’s stated architecture is “everything binds to a Workspace,” but the actual code change is a tactical viewer hack, not the abstraction migration. Design doc says one thing, code does another. Severity: [warning]

  9. tests/unit/test_workspace.py, uses real /tmp/... path in test_bucket_dir_absolute
    Non-isolated test, possible collision on shared runners, and bad hygiene. Severity: [minor]

  10. tests/unit/test_vllm_manager.py, startup timeout default changed from 120 to 300
    That is an operational behavior change with user-facing latency implications. No changelog/docs/update, no rationale in code, only a test tweak. Severity: [warning]

Missing

  • No tests for viewer pre-release API behavior (/api/status, /api/layers, /api/search) and post-release transition.
  • No tests for release discovery race conditions while build/release happens concurrently.
  • No docs updates in README/website/CLI docs for Workspace, synix.toml, or changed viewer semantics.
  • No implementation of old-config migration despite RFC claiming one-time migration path.
  • No tests proving Workspace.state matches actual build/release refs across existing project layouts.

VerdictBlock: the viewer changes are shipping ahead of the workspace design, and the new abstraction introduces public API/config one-way doors without enough compatibility, validation, or documentation.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,589 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:21:26Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR introduces the Workspace abstraction — a composition layer over Project that unifies configuration (buckets, vLLM, auto-build), lifecycle state, and runtime services into a single entity backed by synix.toml. It also makes the viewer resilient to starting without a release, discovering one lazily. The diff includes the RFC document, the workspace.py implementation, viewer changes, and comprehensive unit + e2e tests.

Alignment

Strong fit. DESIGN.md establishes that Synix is Python-first and build/release separation is absolute — Workspace respects both. It wraps Project via composition without touching build logic, artifact immutability, or the content-addressed object store. The RFC explicitly calls out that "Workspace is not a god object" and that buckets (ingestion) are distinct from Pipeline.source_dir (build-time reads) — this preserves the clean build-system mental model. The WorkspaceState enum (FRESH → CONFIGURED → BUILT → RELEASED → SERVING) maps naturally to the build lifecycle in DESIGN.md §4.4. Config types moving from server/config.py to workspace.py corrects the dependency direction — workspace is foundational, server is a consumer — which aligns with the layered architecture.

Observations

  1. [positive] workspace.py — Clean separation. ~380 lines, purely compositional. No build logic, no search indexing, no transform execution. The delegation methods are thin pass-throughs. This is the right scope.

  2. [positive] Viewer lazy discovery (server.py:try_discover_release) — Eliminates a hard startup dependency. The before_request hook with _require_release() returning 503 is a clean pattern. API endpoints degrade gracefully.

  3. [concern] server.py:try_discover_release calls _synix.open_project() on every failed discovery attempt (every request when no release exists). This re-reads disk state per request. Under load before a first build, this could be expensive. Consider a cooldown timer (e.g., check at most once per 5 seconds).

  4. [concern] workspace.py:_has_snapshots imports RefStore inside the method and instantiates it on every state property access. Since state is called frequently (tests, viewer status API), this import + construction on every call is wasteful. Consider caching or at least moving the import to module level.

  5. [question] WorkspaceRuntime uses Any for all service types (queue, prompt_store, build_lock, vllm_manager). The RFC says this replaces the _state dict — but the typing is equally opaque. Is there a reason not to use Protocol types or at minimum the concrete types with TYPE_CHECKING imports?

  6. [nit] _parse_toml manually casts vLLM int/float fields (int(val), float(val)), but TOML already parses integers and floats natively. The casts are defensive but unnecessary for well-formed TOML — they'd only help if someone wrote port = "8100" (a string), which is a TOML schema error. Consider validating instead of silently coercing.

  7. [positive] Test coverage is thorough. test_workspace.py (311 lines) covers identity, state transitions, delegation, buckets (relative, absolute, missing), runtime activation, TOML parsing (full, minimal, defaults, server bindings). test_workspace_e2e.py (165 lines) covers the complete lifecycle including reopening from disk and bare mode. Both happy paths and edge cases.

  8. [nit] test_bucket_dir_absolute writes to /tmp/test-abs-bucket-ws — a real filesystem path outside tmp_path. This could collide across parallel test runs and leaves state on disk.

  9. [question] The RFC document (Phase 2-8) describes migrating serve.py, mcp_tools.py, api.py, CLI commands, viewer identity, and templates — but the diff only implements Phase 1 plus the viewer changes. Is the intent to land the RFC + Phase 1 together, with subsequent PRs for Phases 2-8? The serve.py change in the diff is partial (viewer resilience only, not the full serve() refactor).

  10. [positive] init_workspace won't overwrite an existing synix.toml — tested explicitly in test_init_workspace_preserves_existing_toml. Good for idempotency.

  11. [nit] The smoke test source file timestamp change (smoke-test.md) looks like an accidental inclusion.

Verdict

This is a well-scoped, well-tested foundational abstraction that consolidates real configuration sprawl without disrupting the build-system core — a good incremental step.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,589 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:21:32Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium-high risk: this PR introduces a new top-level abstraction and exports it publicly, but only a slice of the migration landed, so the surface is becoming user-visible before the design is actually coherent.

One-way doors

  1. Public Workspace API in src/synix/__init__.py
    Hard to reverse because Workspace, WorkspaceConfig, WorkspaceState, init_workspace, and open_workspace are now importable from synix and will get copied into user code immediately. Safe to merge only if the team is committed to this shape for at least one compatibility window and has audited naming, lifecycle semantics, and delegation methods.

  2. synix.toml as canonical workspace config in src/synix/workspace.py + RFC
    Hard to reverse because templates, docs, deploy layout, and operator mental model will standardize on this filename and structure. Safe only if config precedence/migration behavior is actually implemented end-to-end, not just described in RFC prose.

  3. Viewer behavior change: start without a release
    Hard to reverse once operators/scripts depend on synix serve bringing up a viewer before first build. Safe only if all viewer/API consumers are updated to handle loaded=false / 503 semantics and this behavior is documented.

Findings

  1. src/synix/workspace.py + src/synix/__init__.py: premature public API export
    You exported a substantial new abstraction before the migration is real. The RFC says server/config, CLI, templates, viewer, _state replacement, and deploy config all move to workspace, but the actual diff only adds the core class and some viewer support. This creates an API users can adopt while internals still disagree on the system boundary. Severity: [warning]

  2. src/synix/workspace.py: Workspace.state uses refs/heads/main directly
    This leaks a branch/storage convention into a supposedly foundational abstraction. DESIGN emphasizes refs/snapshots/releases, not “main” as a universal truth. If branch names or ref layout change, state detection lies. Severity: [warning]

  3. src/synix/workspace.py: _has_releases() scans .synix/releases/*/receipt.json
    Another abstraction leak. You are baking on-disk release layout into lifecycle logic instead of delegating to Project.releases() or a release store. That is exactly the kind of format coupling that becomes painful when on-disk compatibility evolves, which README explicitly warns may happen. Severity: [warning]

  4. src/synix/workspace.py: init_workspace() writes files with no validation or atomicity
    It unconditionally scaffolds synix.toml and sources/ after sdk_init, with no rollback if writes fail and no validation that pipeline.py exists or is loadable. Partial workspace creation is plausible on permission/disk errors. Severity: [minor]

  5. `src/synix/viewer/server.py: before_request release discovery re-opens project on every request until success
    This is hidden complexity and unnecessary filesystem churn. Under a polling UI, you’ve created repeated project re-open / ref scan behavior on the hot path. Works at toy scale, gets noisy fast, and has racey semantics if release materialization is in progress. Severity: [warning]

  6. src/synix/viewer/server.py: API contract changed to 503 for most endpoints when no release exists, but no docs updated
    This is user-facing behavior. README/website position synix serve as “knowledge server with ... viewer,” but there is no mention that viewer APIs may return empty status + 503 until first release. Severity: [warning]

  7. src/synix/workspace.py: WorkspaceConfig defaults create contradictory “bare mode” semantics
    open_workspace() without config returns Workspace(project, None), which then exposes a non-None default WorkspaceConfig(). Tests even assert ws.config is not None in bare mode. That muddies the documented distinction between “no config file exists” and “default config exists.” Expect bugs around detecting configured vs implicit defaults. Severity: [warning]

  8. tests/unit/test_workspace.py: test_bucket_dir_absolute uses /tmp/test-abs-bucket-ws
    Non-isolated real filesystem path in unit test. This can fail under parallel runs or polluted CI hosts. Basic test hygiene issue. Severity: [minor]

  9. tests/unit/test_vllm_manager.py: default timeout changed from 120 to 300 with no code diff shown or docs rationale
    This is a behavioral change with operational impact. If startup latency assumptions changed, I’d expect corresponding implementation diff and documentation. Right now it looks like the test was updated to bless a new default without review context. Severity: [warning]

Missing

  • Actual migration of server/config.py, CLI serve commands, _state replacement, and template/config changes described in the RFC.
  • Tests for viewer endpoints in “no release yet” mode, especially /api/status, /api/layers, and transition after first release appears.
  • Concurrency/race tests for release discovery while a build/release is being written.
  • Documentation updates in README/website for the changed viewer startup behavior and the new Workspace API.
  • Validation tests for malformed synix.toml types and unknown fields.

VerdictBlock: the direction may be reasonable, but this lands a one-way public API and changed runtime behavior while the actual migration is incomplete and the abstraction still leaks storage details.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,586 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:23:02Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR introduces a Workspace abstraction that unifies scattered configuration (Project, ServerConfig, Pipeline, _state dict, CLI options) into a single composable entity. It adds workspace.py with config types, state machine, and factories; modifies the viewer to start without a release and discover one lazily; and includes comprehensive unit + e2e tests. An RFC document describes the full 8-phase rollout — this diff implements Phase 1 plus the viewer changes from Phases 4-5.

Alignment

Strong fit. DESIGN.md §1.5 defines the build/release separation and .synix/ as single source of truth — Workspace respects this by wrapping Project via composition rather than absorbing build logic. The RFC explicitly states "Workspace is not a god object" and keeps build/search/transform in existing modules. The WorkspaceState enum (FRESH→CONFIGURED→BUILT→RELEASED→SERVING) maps cleanly to the build-system lifecycle. Config types moving from server/ to workspace.py corrects the dependency direction — workspace is foundational, server is a consumer — consistent with the layered architecture principle.

The Python-first bet (DESIGN.md §4.1) is preserved: synix.toml handles operational concerns (buckets, ports, vLLM) while pipeline definition stays in Python code. No drift toward config-as-architecture.

Observations

  1. [positive] workspace.py — Clean composition pattern. Workspace delegates to Project for all build/release operations and holds no transform logic. The ~250 lines are well-scoped.

  2. [positive] Viewer lazy discovery (server.py:try_discover_release) — The before_request hook that discovers releases and the _require_release() guard returning 503 are clean patterns. This unblocks the "start viewer before first build" UX.

  3. [concern] server.py:try_discover_release calls _synix.open_project() on every request when no release exists. This re-reads disk state per request. Under load (or with a slow filesystem), this could be expensive. Consider a cooldown timer or checking at most once per N seconds.

  4. [concern] workspace.py:_load_config returns WorkspaceConfig | None, but Workspace.__init__ falls back to WorkspaceConfig() when config is None. In open_workspace, the None propagates correctly, but the type signature config: WorkspaceConfig | None = None on __init__ combined with self._config = config or WorkspaceConfig() means self._config is never None — yet config property exposes it with no annotation update. Minor but could confuse consumers.

  5. [question] WorkspaceRuntime fields are all typed as Any. The RFC mentions these replace the _state dict, but the lack of type constraints means the contract is implicit. Is this intentional to avoid circular imports, or should Protocol types be defined?

  6. [nit] test_bucket_dir_absolute writes to /tmp/test-abs-bucket-ws — a hard-coded path outside tmp_path. This could collide in parallel test runs or leave artifacts. Use tmp_path instead.

  7. [positive] Test coverage is thorough — 311 lines of unit tests covering identity, state transitions, delegation, buckets, runtime, TOML parsing, and factory functions. E2e tests cover the full lifecycle including bare-workspace and config-with-buckets scenarios. Edge cases (missing TOML, explicit config path, preserving existing TOML) are covered.

  8. [concern] _has_snapshots imports RefStore inside the method and catches all exceptions. A corrupted .synix/ directory would silently report FRESH state instead of surfacing the error. Consider narrowing the exception type.

  9. [nit] VLLMConfig.startup_timeout default changed from 120→300 (visible in the vllm_manager test update). This is a behavioral change to an existing config — not documented in the PR description or RFC decisions log.

  10. [positive] The RFC document is exceptionally well-structured — phased implementation, clear file-change inventory, verification steps, and a decisions log addressing review feedback. This is a model for incremental rollout planning.

  11. [question] The smoke test timestamp change (smoke-test.md) — is this an accidental commit, or does it verify that content-addressed artifacts detect source changes?

Verdict

This is a well-designed foundational abstraction that consolidates real configuration sprawl without absorbing responsibilities it shouldn't own — a good incremental step that unblocks the server/viewer UX and sets up clean phases for the remaining migration.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,586 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:23:07Z

…st isolation

- _has_snapshots() now follows HEAD → ref → OID instead of hardcoding
  refs/heads/main (supports any branch name)
- Viewer try_discover_release() has 5-second cooldown to avoid
  per-request filesystem churn before first build
- Fix test_bucket_dir_absolute to use tmp_path instead of /tmp/
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium-high risk: this PR adds a new exported top-level abstraction and changes viewer/server startup behavior, but only a slice of the RFC is actually implemented, so the surface area is bigger than the proof.

One-way doors

  1. Public Workspace API in src/synix/__init__.py
    Hard to reverse because Workspace, WorkspaceConfig, WorkspaceState, init_workspace, and open_workspace are now top-level imports users will code against. Renaming or changing semantics later becomes a compatibility problem. Safe only if the team is willing to support this API through 1.0 or explicitly mark it experimental in docs and module naming.

  2. synix.toml as canonical workspace config shape
    Hard to reverse because templates, init scaffolding, deploy paths, and operator workflows will depend on this filename and section layout. Safe only if config migration strategy and precedence rules are implemented, not just described in the RFC.

  3. Viewer “start without a release” behavior
    This changes operational expectations and API semantics (/api/* now returns 503 until a release exists). Safe only if the frontend/UI and docs are updated to handle this state explicitly.

Findings

  1. src/synix/__init__.py exporting Workspace* types
    You are freezing a public API before finishing the migration. The RFC mentions many follow-on changes, but the code only ships workspace.py, tests, and partial viewer behavior. This is a premature one-way door. Severity: [warning]

  2. src/synix/workspace.py Workspace.state uses self._config.buckets to decide CONFIGURED
    open_workspace() can return config=None, but __init__ replaces that with default WorkspaceConfig(). That means “no config file exists” is indistinguishable from “config file exists with defaults.” You’ve collapsed an important state boundary the RFC claims to model. Severity: [warning]

  3. src/synix/workspace.py _has_snapshots() reads .synix/HEAD directly and treats len(head_content) >= 40 as a direct OID
    This is weak artifact/ref validation and bakes in undocumented storage assumptions. If ref format changes, Workspace.state silently lies. This leaks storage internals into a high-level abstraction that’s supposed to be “thin composition.” Severity: [warning]

  4. src/synix/workspace.py _parse_toml() does unchecked coercion of user config
    int(auto_build_raw.get("window", 30)), int(val), float(val) will throw raw ValueError on bad TOML values, with no boundary validation or actionable message. This is exactly where config validation should exist. Severity: [warning]

  5. src/synix/viewer/server.py ViewerState._last_discovery_attempt: float = 0.0 declared as a class attribute
    That cooldown state is shared across instances until shadowed. In tests or multiple app instances in one process, discovery timing can bleed between viewers. Make it an instance field in __init__. Severity: [minor]

  6. src/synix/viewer/server.py before_request re-opens the project from disk every discovery attempt
    This is hidden complexity and unnecessary churn. You’ve introduced periodic filesystem reopening on every request path until release exists. It’s throttled, but still an odd execution model for Flask request handling. Severity: [minor]

  7. src/synix/server/serve.py changed viewer startup semantics without corresponding tests
    The code now starts the viewer with release=None, but there are no tests for API 503 behavior, discovery after release creation, or UI compatibility. This is a behavioral change with no regression protection. Severity: [warning]

  8. tests/unit/test_vllm_manager.py updates startup_timeout expectation from 120 to 300
    The actual production code change for this default is not in the diff. If vllm_manager still defaults to 120, this test is just wrong; if it now gets config from workspace.py, that coupling is implicit and undocumented. Either way this is brittle. Severity: [warning]

  9. Docs mismatch: README/website still center open_project() and make no mention of Workspace
    If Workspace is now first-class and exported, public docs are behind. If it isn’t first-class yet, don’t export it. Right now you get the downside of both. Severity: [warning]

Missing

  • Tests for viewer endpoints in “no release yet” mode and transition to available release.
  • Tests for malformed synix.toml values and user-friendly errors.
  • Any implementation of the server/config migration or CLI migration described in docs/workspace-rfc.md.
  • Documentation updates in README/website/integration docs for Workspace and synix.toml.
  • Concurrency story tests: viewer discovery while release is being materialized.

VerdictShip with fixes if you keep Workspace internal; otherwise block because exporting it now hardens an incomplete design with docs/tests not ready.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,606 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:24:19Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR introduces a Workspace abstraction — a composition layer over Project that unifies configuration (buckets, vLLM, auto-build), lifecycle state, and runtime services into a single entity backed by synix.toml. It also makes the viewer resilient to starting without a release, discovering one lazily after the first build completes. The diff includes the new workspace.py module, viewer changes, an RFC doc, and thorough unit + e2e tests.

Alignment

Strong fit. DESIGN.md §1.5 establishes build/release separation and .synix/ as the single source of truth — Workspace respects this by delegating all build/release operations to Project rather than reimplementing them. The RFC explicitly states "Workspace is not a god object" and "does not contain build logic," which preserves the content-addressed artifact model and provenance chain. Config types (buckets, vLLM) are operational concerns, cleanly separated from the pipeline DAG — consistent with the Python-first, "architecture is a runtime concern" hypothesis. The WorkspaceState enum being computed from disk state (never persisted) avoids introducing mutable metadata that could drift from the immutable object store.

Observations

  1. [positive] workspace.py — Config type ownership is correct. Moving BucketConfig/VLLMConfig to workspace and having server/config.py re-export avoids the backwards coupling the RFC identifies. Dependency direction is clean.

  2. [positive] Viewer discovery mechanism (try_discover_release) with 5-second cooldown is a pragmatic solution. The before_request hook + _require_release() returning 503 is the right HTTP semantic for "not ready yet."

  3. [concern] _has_snapshots() in workspace.py:222-235 directly parses HEAD file format and imports RefStore. This duplicates knowledge of the ref storage format. If Project or RefStore ever changes HEAD semantics, this breaks silently. Should this delegate to Project (e.g., project.has_snapshots() or project.head_ref())?

  4. [concern] WorkspaceRuntime uses Any types for queue, prompt_store, build_lock, and vllm_manager. This avoids circular imports but sacrifices all type safety at the boundary where runtime services are composed. A Protocol or TYPE_CHECKING import would preserve the contract without the coupling.

  5. [question] _load_config returns WorkspaceConfig | None, but Workspace.__init__ falls back to WorkspaceConfig() when None. In open_workspace, the None flows through — meaning bare workspaces get an empty config with auto_build.enabled=True as default. Is it intentional that a bare workspace (no synix.toml) defaults to auto-build enabled?

  6. [nit] viewer/server.py_last_discovery_attempt is a class variable initialized to 0.0 on ViewerState, then mutated on instances. Works in practice since only one ViewerState exists, but it's a subtle pattern. Should be initialized in __init__.

  7. [positive] Test coverage is excellent. Unit tests cover identity, state transitions, bucket resolution (relative, absolute, missing), TOML parsing (full, minimal, defaults, server bindings), and both factory paths. E2e tests cover the full lifecycle including reopen-from-disk and bare mode. The mock_llm fixture is used correctly to avoid real LLM calls.

  8. [question] The RFC doc (Phase 3) describes replacing _state dict in mcp_tools.py, but the diff doesn't include that change. Is this PR intentionally Phase 1 only? If so, the RFC probably shouldn't ship in the same PR — it describes future work as if committed.

  9. [nit] VLLMConfig.startup_timeout default changed from 120 to 300 (visible in the vllm_manager test update). This is a behavioral change buried in a config refactor — worth a changelog note.

  10. [positive] The smoke-test timestamp change confirms the deploy pipeline was exercised end-to-end.

Verdict

This is a well-structured incremental step that introduces a necessary coordination layer without violating the core build-system invariants — the _has_snapshots coupling and untyped runtime fields should be addressed before later phases build on this foundation.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,606 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:24:32Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium-high risk: this PR introduces a new public abstraction and changes viewer/server startup behavior with thin test coverage around the real failure modes.

One-way doors

  1. Public Workspace API export in src/synix/__init__.py
    Hard to reverse because Workspace, WorkspaceConfig, WorkspaceState, init_workspace, and open_workspace become top-level SDK surface users will import directly. Renaming/removing later is a breaking change. Safe only if the team is committed to Workspace as a core concept, with semantics that won’t churn in pre-1.0 every week.

  2. synix.toml as canonical workspace config in src/synix/workspace.py / docs/workspace-rfc.md
    File name, location, and schema become user mental model and deployment contract. Reversing means migration tooling and docs churn. Safe only if CLI/templates/server are actually migrated consistently and the old config path has a tested compatibility story.

  3. Viewer supports release-less startup in src/synix/viewer/__init__.py and src/synix/viewer/server.py
    This changes operational semantics: callers can now rely on booting the viewer before any release exists. Backing this out later would break workflows and deployment assumptions. Safe only if all viewer endpoints and UI are intentionally designed for this state, not just patched to return 503.

Findings

  1. src/synix/workspace.py + tests/unit/test_workspace.py: state computation depends on in-memory pipeline, not disk config
    Workspace.state returns CONFIGURED only if self._project.pipeline is not None or self._config.buckets. A workspace with synix.toml and pipeline.py but no loaded pipeline stays FRESH, contradicting the RFC’s “pipeline loaded or buckets defined” and making status dependent on call order. This is a leaky abstraction: state is not “computed from disk state on access.” Severity: [warning]

  2. src/synix/workspace.py:init_workspace() claims full scaffold but does not create pipeline.py or bucket dirs
    RFC and tests talk about complete workspace scaffolding; implementation only calls sdk_init, writes synix.toml, and creates sources/. Whether pipeline.py exists is delegated implicitly to sdk_init, but not verified here. This is exactly the kind of hidden coupling the abstraction was meant to remove. Severity: [warning]

  3. src/synix/viewer/server.py: class variable misuse for _last_discovery_attempt
    _last_discovery_attempt: float = 0.0 is defined at class scope, not initialized on the instance. In Python that becomes shared mutable state across all ViewerState instances until one instance assigns over it. That creates cross-instance throttling and test bleed. Severity: [warning]

  4. src/synix/viewer/server.py:try_discover_release() hardcodes "local"
    _RELEASE_NAME is used in server/serve.py, but discovery later hardcodes "local" or first release. That’s inconsistent and will surprise operators relying on another canonical release name. This is user-facing behavior drift without docs. Severity: [warning]

  5. src/synix/viewer/server.py: per-request project reopen with no concurrency story
    before_request may reopen the project every 5 seconds under concurrent traffic. There’s no locking around self.release / self.project mutation. Flask can run threaded; this is a racey read/write of shared state. Probably harmless in dev, not obviously safe in server mode. Severity: [minor]

  6. src/synix/workspace.py:_has_releases() scans directory shape directly
    The new abstraction bypasses Project and infers release existence from .synix/releases/*/receipt.json. That couples Workspace to on-disk layout the design says should be delegated to Project. If release storage changes, Workspace.state silently breaks. Severity: [warning]

  7. tests/e2e/test_workspace_e2e.py does not test viewer/server integration for the new no-release startup behavior
    The risky behavior change is in viewer startup and lazy discovery, but tests only exercise build/release lifecycle. No endpoint-level test for 503-before-release then success-after-release. Severity: [warning]

  8. docs/workspace-rfc.md added, but no user-facing docs updated
    README/website still present open_project() and do not explain Workspace, synix.toml, or viewer “discovery mode.” If this is intentional public surface, docs are stale on day one. Severity: [minor]

Missing

  • Tests for viewer API behavior before first release and after discovery.
  • Tests for try_discover_release() with non-local release names.
  • Tests for Workspace.state when pipeline.py exists on disk but has not been loaded.
  • Any implementation of the claimed migration/compat story for old config files; RFC mentions it, diff does not.
  • User-facing documentation updates if synix init is now expected to scaffold synix.toml.

VerdictShip with fixes: the core idea is reasonable, but the current implementation leaks disk-layout details, has inconsistent state semantics, and under-tests the actual behavior changes.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,630 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:26:10Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR introduces a Workspace abstraction — a composition layer over Project that unifies configuration (buckets, vLLM, auto-build), identity, lifecycle state, and runtime services into a single entity backed by synix.toml. It also makes the viewer resilient to starting without a release, discovering one lazily after the first build completes. The PR includes the implementation code, an RFC document, and comprehensive unit + e2e tests.

Alignment

Strong fit. DESIGN.md §1.5 explicitly separates build from release; Workspace respects this by delegating build/release to Project rather than reimplementing. The RFC's constraint — "Workspace is not a god object" — directly preserves the build-system layering. Config types moving from server/config.py to workspace.py corrects the dependency direction (foundational abstraction owns the types, consumer imports them). The synix.toml approach stays Python-first per §4.1 — config describes operational concerns (buckets, ports), not pipeline logic. Pipeline definition remains code.

The viewer's lazy release discovery advances Hypothesis 3 (architecture is a runtime concern) — you can start serving before you've figured out your pipeline, and the system adapts when a build lands.

Observations

  1. [positive] _has_snapshots() correctly follows HEAD → ref → OID indirection rather than just checking file existence, which respects the content-addressed object model.

  2. [concern] _has_snapshots() imports RefStore inside the method body. If RefStore changes its interface or the HEAD format evolves, this is a hidden coupling. Consider injecting or delegating to Project which already knows how to resolve refs.

  3. [concern] try_discover_release() calls open_project() on every successful discovery, creating a fresh Project instance and replacing self.project. This means any state held by the original Project (loaded pipeline, cached data) is silently discarded. If the viewer ever holds a reference to the old project, it's stale.

  4. [question] WorkspaceRuntime types are all Any. The RFC says this replaces the _state dict — but the dict at least had implicit contracts. Would lightweight Protocol types or string annotations avoid circular imports while preserving discoverability?

  5. [nit] _load_config returns WorkspaceConfig | None, but Workspace.__init__ does config or WorkspaceConfig(). This means open_workspace with no toml silently creates a default config, but config property always returns a WorkspaceConfig — never None. The type signature on _load_config is misleading; the None is consumed immediately.

  6. [positive] The 5-second cooldown in try_discover_release() prevents filesystem thrashing on high-request-rate viewers. Good defensive design.

  7. [concern] _require_release() returns a tuple (response, 503) or None. The pattern err = _require_release(); if err: return err works but is fragile — a future contributor might forget the guard on a new endpoint. A decorator (@requires_release) would be safer and more self-documenting.

  8. [positive] Test coverage is thorough: unit tests cover all state transitions, TOML parsing (full, minimal, defaults, missing), bucket resolution (relative, absolute, not-found), factory edge cases. E2e tests cover the complete init→build→release→search lifecycle plus bare mode.

  9. [question] The e2e tests reference a mock_llm fixture that isn't defined in the diff. If this is a shared conftest fixture, fine — but the source-only pipeline (just Source("notes")) shouldn't need LLM mocking. Is mock_llm actually exercised in these tests, or is it a leftover?

  10. [nit] The VLLMConfig.startup_timeout default changed from 120 to 300 (visible in the test_vllm_manager.py change). This is a behavioral change to an operational default that's not mentioned in any changelog or commit message.

  11. [positive] The RFC document is exceptionally well-structured — phased implementation, clear file-change table, explicit decisions log addressing review feedback. This is a model for pre-implementation planning in a solo-maintainer project.

  12. [concern] _parse_toml does manual type coercion (int(val), float(val)) for vLLM fields. TOML already has native integer/float types — this coercion is only needed if someone writes port = "8100" (a string). Either trust TOML's type system or validate and raise on type mismatch instead of silently coercing.

Verdict

This is a well-designed incremental step that consolidates scattered configuration into a coherent abstraction without disrupting the build-system core — exactly the kind of infrastructure work a pre-1.0 project needs before scaling its surface area.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,630 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:26:26Z

- Move _last_discovery_attempt from class attribute to __init__ (was
  shared across all ViewerState instances, causing cross-instance
  throttling)
- WorkspaceState.CONFIGURED now also checks if pipeline.py exists on
  disk, not just if pipeline is loaded in memory — state is truly
  derived from disk
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Red Team Review — OpenAI GPT-5.4 | Adversarial review (docs + diff only)

Threat assessment — Medium-high risk: this PR introduces a new exported abstraction and changes viewer/server startup semantics, but only partially wires the system around it.

One-way doors

  1. Workspace as public top-level API (src/synix/__init__.py)
    Hard to reverse because users will import Workspace, open_workspace, WorkspaceConfig, and WorkspaceState directly. Renaming or changing semantics later becomes a compatibility burden. Safe only if the team is ready to support this API shape through 1.0 or keep it clearly experimental and undocumented.

  2. synix.toml as canonical workspace config format (src/synix/workspace.py, docs/workspace-rfc.md)
    Config file names and section layout become sticky fast. The RFC claims migration from synix-server.toml, but this PR does not implement that migration path. Safe only if backward-compat loading, migration tooling, and template/init changes land together.

  3. Viewer “starts without a release” behavior (src/synix/viewer/__init__.py, src/synix/viewer/server.py, src/synix/server/serve.py)
    This changes operational expectations and API behavior from “viewer requires a released build” to “viewer may return 503 until discovery succeeds.” That becomes part of deploy scripts and UI assumptions. Safe only if all viewer clients, health checks, and docs are updated for the new lifecycle.

Findings

  1. src/synix/__init__.py exports unstable design-RFC types
    You exported WorkspaceConfig and WorkspaceState, not just open_workspace. That freezes internal config/state modeling prematurely. The RFC is still aspirational; the implementation is not integrated across CLI/server/templates. Severity: [warning]

  2. src/synix/workspace.py: open_workspace() returns Workspace(project, None) while tests assume non-None config
    In bare mode _load_config() returns None, but TestFactories.test_open_workspace_bare asserts ws.config is not None. Either the test is wrong or the API contract is inconsistent. More importantly, callers now have to reason about nullable config despite config property returning WorkspaceConfig. Severity: [warning]

  3. src/synix/workspace.py: _has_snapshots() hardcodes HEAD/ref internals
    This leaks storage implementation into a high-level abstraction and couples WorkspaceState to exact ref layout. Any future ref format change breaks state detection. This violates the “thin composition layer” claim in the RFC. Severity: [warning]

  4. src/synix/viewer/server.py: before_request` polling + reopen-project every 5 seconds
    This is hidden complexity and a scalability footgun. Every request on an unloaded viewer hits discovery logic, including filesystem reads and project reopen attempts. At low traffic fine; under repeated probes/health checks this becomes noisy and fragile. Severity: [minor]

  5. **src/synix/viewer/server.py: API contract changed to 503 for most endpoints when no release exists** That is a behavioral change for /api/layers, /api/artifacts, /api/search`, etc. No docs updated, no compatibility note, no tests covering client behavior. Anything expecting the old viewer contract can break. Severity: [warning]

  6. `src/synix/server/serve.py: viewer now starts in “discovery mode” with no release, but no health/readiness distinction
    Operationally, process starts “successfully” even when unusable for queries. If deployment treats process-up as ready, this masks build/release failures. Severity: [warning]

  7. docs/workspace-rfc.md massively overstates implemented scope
    The diff adds an RFC describing migration of serve.py, _state, templates, CLI, deploy config, tests, etc. The actual code only adds workspace.py, exports it, tweaks viewer startup, and changes one default timeout. Design doc and code diverge badly. Severity: [warning]

  8. tests/e2e/test_workspace_e2e.py does not test the changed viewer/server behavior at all
    The risky behavior here is lazy release discovery and 503 semantics. The e2e only tests workspace build/release delegation. The actual regression surface is untested. Severity: [warning]

  9. tests/unit/test_vllm_manager.py updates timeout expectation to 300 with no corresponding code shown
    Either the real config default changed elsewhere and is omitted from the diff context, or the test is being moved ahead of implementation. In either case, this PR mixes unrelated behavior. Severity: [minor]

Missing

  • Tests for viewer started with release=None: /api/status, /api/search, and discovery after a release appears.
  • Documentation updates in README/website/integration docs for new viewer startup semantics and synix.toml/workspace API.
  • Any CLI/template/init changes promised by the RFC.
  • Migration/backward-compat handling for synix-server.toml.
  • Validation of TOML schema and error messages for malformed config.

VerdictBlock: this is an incomplete architectural slice that exports a new public abstraction, changes viewer behavior, and ships an RFC pretending the rest already exists.

Review parameters
  • Model: gpt-5.4
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,633 lines
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-04-06T03:30:00Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Note

Architectural Review — Claude Opus | Blind review (docs + diff only)

Summary

This PR introduces a Workspace abstraction that unifies scattered configuration (Project, ServerConfig, Pipeline, _state dict, CLI options) into a single composable entity. It adds workspace.py with config types, lifecycle states, and factory functions; makes the viewer start without a release (lazy discovery); and includes comprehensive unit and e2e tests. An RFC document lays out the full 8-phase migration plan, but only phases 1 and part of phase 4 ship in this diff.

Alignment

Strong fit. DESIGN.md §1.5 establishes build/release separation with .synix/ as the single source of truth, and Workspace respects this — it wraps Project via composition, delegates all build/release operations, and adds no build logic. The RFC explicitly states "Workspace is not a god object" and "does not contain build logic," which preserves the content-addressed artifact model. Config types moving from server/config.py to workspace.py corrects a dependency inversion (foundational concept depending on server module). The WorkspaceState enum (FRESH→CONFIGURED→BUILT→RELEASED→SERVING) maps cleanly to the build-system lifecycle without introducing mutable state that could compromise immutability guarantees.

Observations

  1. [positive] workspace.py:_has_snapshots() correctly follows HEAD → ref indirection via RefStore, respecting the existing ref-based snapshot model rather than inventing new state tracking.

  2. [positive] Test coverage is thorough — 312-line unit test covers identity, state transitions, delegation, bucket resolution (relative/absolute/missing), runtime activation, TOML parsing (full/minimal/defaults), and factory edge cases. E2e test covers the complete init→build→release→search flow.

  3. [concern] viewer/server.py:try_discover_release() re-opens the entire project on every discovery attempt (_synix.open_project(str(self.project.project_root))). With the 5-second cooldown this is fine at low scale, but the comment says "avoid per-request filesystem churn" while the implementation does significant I/O per attempt. Consider caching the re-opened project or just re-reading refs.

  4. [concern] workspace.py:_load_config() returns WorkspaceConfig | None, but Workspace.__init__ does self._config = config or WorkspaceConfig(). This means open_workspace with no TOML silently creates a default config. The config property then always returns a WorkspaceConfig, so callers can't distinguish "no config file" from "empty config file." The test_open_workspace_bare test asserts ws.config is not None — correct per implementation, but the semantics are subtle.

  5. [question] WorkspaceRuntime uses Any for all service types (queue, prompt_store, build_lock, vllm_manager). This avoids circular imports but sacrifices type safety at the boundary where runtime bugs are most likely. Is there a plan to introduce a protocol/ABC for these?

  6. [nit] _parse_toml manually casts vLLM int/float fields (int(val), float(val)) even though TOML natively types integers and floats. This is only needed if values arrive as strings, which shouldn't happen with well-formed TOML. Defensive but misleading.

  7. [concern] init_workspace calls sdk_init then writes synix.toml, but doesn't create bucket directories defined in config. The scaffold creates sources/ but if a user writes buckets pointing to sources/sessions/, those won't exist. The e2e test manually creates docs_dir with mkdir(parents=True).

  8. [positive] The viewer's _require_release() returning 503 with a clear message is the right HTTP semantic for "not ready yet" — clients can retry.

  9. [nit] The RFC document (docs/workspace-rfc.md) is 438 lines but only phases 1 and partial phase 4 are implemented. The RFC references future phases (mcp_tools replacement, template migration, deploy rename) that aren't in this diff — consider marking which phases this PR covers.

  10. [positive] The VLLMConfig.startup_timeout default changed from 120→300, with the corresponding test updated. Clean cross-cutting fix.

Verdict

This is a well-structured incremental step — it introduces the Workspace concept additively without breaking existing paths, has strong test coverage, and correctly preserves the build-system invariants. The lazy viewer discovery is a meaningful UX improvement. Ship it with a note to address the config None vs default ambiguity.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 1,633 lines
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-04-06T03:30:07Z

@marklubin marklubin merged commit 65194b7 into main Apr 6, 2026
13 checks passed
@marklubin marklubin deleted the mark/workspace-abstraction branch April 6, 2026 03:37
@marklubin marklubin mentioned this pull request Apr 6, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant