Skip to content

feat: projection release v2 — build/release separation#92

Merged
marklubin merged 20 commits intomainfrom
mark/projection-release-v2
Mar 9, 2026
Merged

feat: projection release v2 — build/release separation#92
marklubin merged 20 commits intomainfrom
mark/projection-release-v2

Conversation

@marklubin
Copy link
Copy Markdown
Owner

@marklubin marklubin commented Mar 8, 2026

Closes #50
Closes #56

Summary

Implements the projection release v2 RFC — a complete architectural overhaul that removes build/ as a platform concept and makes .synix/ the single source of truth with an explicit release lifecycle.

What changed

  • Build produces immutable snapshots only — synix build writes to .synix/objects/ and .synix/refs/, never creates a build/ directory
  • Explicit release lifecycle — synix release HEAD --to name materializes projections via adapter contracts
  • Single write path — ArtifactStore dual-write removed, ObjectStore is the only write path
  • Provenance via artifact graph — ProvenanceTracker removed, replaced by parent_labels traversal
  • Adapter contract — pluggable ProjectionAdapter base with synix_search and flat_file built-in adapters
  • SnapshotView — ref-resolved read API for all CLI queries
  • Release-aware search — synix search query --release local queries materialized release targets

Build reliability fixes (latest commit)

  • LLM client timeouts — Per-request HTTP timeout (default 300s) via LLMConfig.timeout, wired through httpx.Timeout to both Anthropic and OpenAI SDK clients. 10-minute hard ceiling on future.result() prevents worker hangs.
  • Checkpoint caching — Per-layer checkpoint manifests written to .synix/checkpoints/ during builds. SnapshotArtifactCache loads checkpoint artifacts as fallback, so interrupted builds recover cached artifacts on restart.
  • Content-addressed dedup tests — Incremental rebuild tests verify artifact_ids and content are byte-identical across builds.
  • Release-time embeddings — SynixSearchAdapter generates embeddings at release time (fail-closed when embedding_config is declared).

Test plan

  • uv run release passes (ruff + pytest + verify-demos)
  • All 5 demo cases pass golden comparison
  • Checkpoint recovery: interrupted build recovery, corrupted checkpoint handling, cleanup after commit
  • Timeout: httpx.Timeout passed to SDK clients, config wiring, future.result ceiling
  • Content-addressed dedup: artifact_ids and content identical across no-change rebuilds

marklubin added 11 commits March 7, 2026 11:25
…ma v2, projection recording

Phase 1: Add SnapshotView — ref-resolved read API over immutable snapshots
with list_artifacts, get_artifact, get_content, get_manifest, get_provenance,
and resolve_prefix methods.

Phase 2: Bump SCHEMA_VERSION to 2 with structured projection entries in
manifests. Validates adapter, input_artifacts, config, config_fingerprint,
and precomputed_oid fields. Backward compat for reading v1 manifests.

Phase 3: BuildTransaction.record_projection() records structured projection
declarations during build. Runner calls _record_snapshot_projections() to
populate manifest projections from pipeline projection layers.
…hotArtifactCache

Remove ArtifactStore dual-write (Phase 4) and ProvenanceTracker (Phase 5).
All consumers now read/write through SnapshotArtifactCache backed by
.synix/objects/. Compatibility files (manifest.json, provenance.json,
layer dirs) are still written for transitional CLI consumers.

Key changes:
- Runner uses BuildTransaction as sole write path; no ArtifactStore
- SnapshotArtifactCache.update_from_build() merges freshly built
  artifacts for validators/fixers to see current-build data
- Validators, fixers, CLI commands all consume SnapshotArtifactCache
- ProvenanceTracker removed; provenance via parent_labels in artifacts
- search/indexer.py decoupled from ProvenanceTracker
- 49 files changed across engine, CLI, tests, and golden files
…e 6+7)

ReleaseClosure resolves a snapshot into a fully-resolved artifact bundle
with content and provenance chains walked. This is the universal input
every adapter receives at release time.

Two built-in adapters ship:
- synix_search: self-contained search.db with FTS5, provenance_chains,
  citation_edges, and release_metadata tables
- flat_file: atomic markdown context document from artifact content

Adapter registry uses lazy import resolution to respect the
build/ -> search/ import boundary.
Release orchestration: resolve ref -> build closure -> dispatch adapters
-> write receipt -> advance release ref -> append history.

CLI commands:
- synix release <ref> --to <name>: materialize projections to a named target
- synix revert <ref> --to <name>: release an older snapshot (same engine)
- synix releases list: show all releases with receipt info
- synix releases show <name>: display receipt details

Each release writes receipt.json, history entry, and advances
refs/releases/<name>. Pending transaction (.pending.json) preserved
on failure for diagnosis.
- list/show/lineage commands read from SnapshotView via --ref/--synix-dir
- search supports --release (query materialized release) and --ref (scratch)
- search auto-detects single release when neither flag given
- new refs list/show commands for snapshot ref inspection
- ReleaseProvenanceProvider reads provenance_chains from released search.db
…e 12)

Remove _write_compatibility_files() from runner.py, eliminating the dual-write
of manifest.json, provenance.json, and layer directories to build/. Rewrite
verify.py to use SnapshotArtifactCache instead of reading from build/ files.
Migrate all E2E tests from ArtifactStore/ProvenanceTracker to snapshot-based
reads. Fix content-addressed tampering in fingerprint invalidation tests to
properly rebuild the snapshot chain when modifying artifact objects.
…napshotArtifactCache

- build_commands: _save_plan_artifact() uses ObjectStore directly
- search_commands: _build_customer_filter() uses SnapshotArtifactCache
- test_cli, test_plan, test_retriever, test_search_cli_extensions,
  test_status, assertions: all migrated to snapshot-based reads

Only 3 test files retain ArtifactStore for testing writable store paths
(LLM trace persistence in validators/fixers).
All 4 LLM-backed demo cases now include an explicit `synix release HEAD --to local`
step between build and search, teaching the build→release→query workflow.

- demo_commands.py: normalize release output paths for golden stability
- case.py (all 4): add release step, renumber note steps
- Golden files regenerated for all demos
… truth (Phase 12)

Runner no longer materializes projections at build time; only search
surfaces are built (to .synix/work/surfaces/). Projections (search.db,
context.md) are now exclusively a release-time concern via `synix release`.

- Remove ProvenanceTracker (source + compat shim + tests)
- Remove ~380 lines of dead projection materialization code from runner
- Clean command targets .synix/releases/ and .synix/work/ instead of build/
- Logs write to .synix/logs/ instead of build/logs/
- Plan no longer reads .projection_cache.json (projections show as "declared")
- Fix adapter name: SearchIndex uses "synix_search" (matching adapter registry)
- Status command checks .synix/ existence instead of build/
- All e2e/integration tests add release step for projection assertions
- Regenerate all demo golden files
- README: quickstart shows build → release → search flow, updated CLI reference
- CLAUDE.md: updated module structure, core concepts, CLI commands
- entity-model.md: .synix/ directory layout replaces build/ layout
- pipeline-api.md: projections materialize via release, not build
- cli-ux.md: added release/revert/releases/refs command specs
- build-phases.md: marked as historical, links to v2 RFC
- DESIGN.md: updated architecture section for build/release separation
- BACKLOG.md: marked implemented items
- llms.txt: updated architecture, commands, quickstart
- init_commands.py: output shows release step
- Template READMEs: added release step, updated paths
- test_build_release_revert: build → release → rebuild → revert lifecycle
- test_search_release: search against release targets, layer filters, error cases
- test_diff_refs: diff between run refs, same-ref no-changes case
- test_clean_releases: clean preserves snapshots, specific release cleanup
- test_no_build_dir: verify build/ is never created, only .synix/

24 new E2E tests covering the full projection release v2 workflow.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Threat assessment — High risk: this PR is a sweeping storage/CLI lifecycle rewrite (build→release separation) with multiple correctness and migration hazards and clear regressions in demos/tests.

One-way doors

  1. CLI lifecycle change: synix build no longer materializes outputs; synix release becomes required
    Hard to reverse because tutorials, user scripts, and mental model will ossify around two-phase operation and named releases.
    Safe to merge if: you ship a compatibility mode/flag (build --release <name>), deprecation timeline, and migration docs + tests proving old workflows still function or fail loudly with actionable guidance.

  2. On-disk state model: removing build/ as a platform concept
    Hard to reverse because users will delete/ignore build_dir assumptions and .synix/ becomes canonical; any tooling around build artifacts breaks.
    Safe to merge if: there’s an explicit migration tool/path for existing projects and strong versioning/compat checks (schema_version + upgrade steps) plus rollback story.

  3. Manifest schema v2 projections as “structured declarations” (src/synix/build/object_store.py, snapshots.py)
    Once external adapters/users start depending on this shape, it’s sticky.
    Safe to merge if: schema versioning is enforced end-to-end (read/write), and you document a compatibility policy for v0.x.

  4. Release receipts as API surface (src/synix/build/release_engine.py, CLI releases show --json)
    Receipts will become automation inputs.
    Safe to merge if: receipt schema is explicitly versioned and tested; fields like resolved_ref mentioned in docs are actually present (currently not).

Findings

  1. src/synix/cli/build_commands.py::_save_plan_artifact overwrites refs/heads/main with a “plan-save” snapshot
    Failure mode: running synix plan can clobber the current HEAD build history, destroying the real main head. That’s catastrophic.
    Severity: [critical]

  2. Scratch realization leaks disk; no cleanup path (src/synix/cli/search_commands.py::_scratch_realize)
    It creates a temp dir under .synix/work/ and never deletes it. Over time this will accumulate large SQLite DBs.
    Severity: [warning]

  3. Release adapter writes incorrect metadata (src/synix/search/adapter.py)
    release_metadata.released_at is set to closure.created_at (snapshot creation time), not actual release time. This breaks auditability and receipt consistency.
    Severity: [warning]

  4. No verify step executed for adapters (src/synix/build/release_engine.py::execute_release)
    Contract defines verify(), but engine never calls it; a corrupted/partial search.db can be “successful” and ref advanced.
    Severity: [critical]

  5. No release locking despite RFC claiming locks (src/synix/build/release_engine.py)
    Concurrency: two releases to same target can interleave writes to .pending.json, receipt, and search.db. Ref advancement is not protected.
    Severity: [critical]

  6. SnapshotArtifactCache.update_from_build does not update provenance parents (src/synix/build/snapshot_view.py)
    Runner now uses cache for parent resolution; overlaying new artifacts without updating _parent_labels_map means validators/fixers may compute stale/empty lineage post-build.
    Severity: [warning]

  7. Transform parent inference regression (src/synix/build/runner.py)
    Previously provenance tracker was authoritative; now _get_parent_labels fallback becomes [inp.label ...] and may be wrong for N:1 transforms unless input_ids is carefully set. You also removed _get_parent_labels usage in batch runner path. Provenance quality likely regresses silently.
    Severity: [warning]

  8. ReleaseClosure provenance chain includes self and is BFS “baggy” (src/synix/build/release.py::_walk_provenance)
    BFS order is not a “chain”; it’s a traversal list. Search UI now shows extra siblings/leaves (goldens reflect this). If downstream expects a path, it’s broken.
    Severity: [minor]

  9. Demo goldens for validate/fix now show “Build directory not found” (multiple templates/**/golden/*.json|stdout.txt)
    This looks like you accepted a regression rather than updating demo harness to the .synix/ model. That’s not an acceptable test update; it hides real failures.
    Severity: [critical]

Missing

  • Tests for release engine: lock contention, adapter failure semantics, verify enforcement, ref advancement rules.
  • Migration coverage: existing build/ projects, SearchIndex compatibility, and behavior of synix search when no releases exist.
  • Cleanup/GC story: .synix/objects grows forever; scratch realizations leak; no pruning command.
  • Updated website content: it still advertises SearchIndex and build/ artifacts in places; landing text appears garbled (“Synix â�� …”).

Verdict

Block — the head-clobbering bug in synix plan, missing release locking/verification, and demo/test regressions make this unsafe to merge even for pre-1.0.

Review parameters
  • Model: gpt-5.2
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 29,718 total — truncated)
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-03-08T20:03:36Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Summary

This PR implements the "Projection Release v2" architecture: a complete separation of build from release. synix build now produces only immutable snapshots in .synix/; a new synix release command materializes projections (search.db, context.md) into named release targets via an adapter contract. The legacy ArtifactStore, ProvenanceTracker, build/ directory, and .projection_cache.json are removed, replaced by ObjectStore, SnapshotView, SnapshotArtifactCache, and ReleaseClosure.

Alignment

This is a high-fidelity execution of the DESIGN.md vision. Content-addressed immutable artifacts, DAG-based provenance via parent_labels, and the separation of build from materialization all strengthen the core build-system-for-memory model. The adapter contract (plan/apply/verify) directly enables the "architecture is a runtime concern" hypothesis — you can release the same snapshot to local files, Postgres, or Qdrant without rebuilding. Manifest schema v2 with structured projection declarations makes projections diffable and inspectable, advancing the "audit determinism" bet. The ReleaseClosure resolving all content and provenance before handing off to adapters is clean and keeps adapters decoupled from the object store.

Observations

  1. [concern] Validate/fix golden files now show "Build directory not found: build" — templates 02, 03, and 04 golden files for validate and fix commands are replaced with error messages. This means the demo flow is broken for those templates. The validate command still looks for build/ via its default --build-dir argument and can't find the .synix/ store. This is a regression in the demo experience.

  2. [concern] SnapshotArtifactCache.__init__ loads all artifacts into memory eagerly (snapshot_view.py lines ~200-240). For 10,000+ artifacts this is an O(n) startup cost with unbounded memory. Every load_artifact, list, and get_parents call thereafter is O(1), but the constructor reads and deserializes every object blob. Consider lazy loading or a streaming interface.

  3. [concern] BFS provenance uses queue.pop(0) in _walk_provenance, SnapshotView.get_provenance, and SnapshotArtifactCache.get_chain. This is O(n²) for large chains. Use collections.deque for O(1) popleft.

  4. [concern] apply_fix with SnapshotArtifactCache is a no-op — the store is read-only, so save_artifact is skipped via getattr. The comment says "fix will be persisted on the next build run" but there's no mechanism to carry the fix forward. A user who runs synix fix will see the fix accepted but it silently vanishes.

  5. [question] _save_plan_artifact creates a full snapshot/manifest/ref chain just to save a plan artifact. This overwrites refs/heads/main, which could clobber a real build snapshot if someone runs synix plan --save after synix build. Is this intentional?

  6. [question] Release locking described in RFC but not implemented. The RFC specifies .synix/releases/<name>/.lock for concurrency control, but release_engine.py has no locking. Concurrent releases to the same target could corrupt state.

  7. [positive] Adapter contract is clean and extensible. ProjectionAdapter with plan/apply/verify + lazy registry is exactly the right abstraction. External developers can register adapters without touching platform code.

  8. [positive] SynixSearchAdapter builds a self-contained search.db with provenance_chains and release_metadata tables. Released databases are fully portable — no .synix/ needed at the consumer.

  9. [nit] Stashing closures in plan.details["_closure"] across plan/apply is a code smell. The ReleasePlan dataclass should carry typed references rather than hiding them in a generic dict.

  10. [positive] Comprehensive RFC document (projection-release-v2-rfc.md) with clear acceptance criteria, implementation order, and test strategy. The design is thorough and well-reasoned.

  11. [concern] No new tests in the diff. The RFC describes extensive unit, integration, and E2E test plans, but this PR ships zero test files. All behavioral changes — release engine, adapter contract, SnapshotArtifactCache, SnapshotView — are untested in this diff.

Verdict

This is the right architectural move — it fully commits to the immutable-snapshot model — but it ships without tests, breaks the validate/fix demo flow for multiple templates, and has scaling concerns in the eager cache loader. It needs a follow-up for tests and the template golden-file fixes before it's shippable.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 29,718 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-08T20:03:45Z

… normalization

- batch_runner: commit snapshots after all layers complete so synix list/show
  works after batch-build (snapshot_txn record + commit)
- validate/fix commands: check .synix/ OR legacy build/ existence instead of
  requiring build/ directory
- demo normalization: add tempfile name and traceback line number normalization
  to _normalize_output() to avoid false golden diffs
- dev.py: clean .synix/ alongside build/ before demo runs
- test_demo_3_team_report: clean .synix/ to avoid stale state from other tests
- Regenerated all golden files for all 5 demo cases
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Threat assessment — High risk: this PR introduces a new on-disk format + a new build/release lifecycle and the diff shows multiple regressions that will break core demo workflows (validate/fix) and likely corrupt/balloon provenance behavior.

One-way doors

  1. Removing build/ as a platform concept + introducing synix release

    • Hard to reverse: Users will script CI/CD around build → release → search --release; adapters/receipts become the new mental model and documented workflow.
    • Safe only if: all CLI commands (search/validate/fix/verify/status) work end-to-end without needing legacy build/, and there’s a clean migration story for existing users/projects.
  2. Manifest schema v2 (ObjectStore SCHEMA_VERSION=2 + structured projections)

    • Hard to reverse: persisted objects and manifests are long-lived; downgrades/mixed versions become ugly fast.
    • Safe only if: there is explicit compatibility behavior (read v1 + write v2, or a migration tool), and tests prove ref resolution across schema versions.
  3. Release receipts + history directory semantics

    • Hard to reverse: receipt JSON becomes an API surface (humans + tooling); you already added --json.
    • Safe only if: receipt schema is versioned and tested; release locking/atomicity semantics are correct (see findings).
  4. Provenance model change (provenance.json removed, provenance baked into objects via parent_labels)

    • Hard to reverse: lineage correctness is core to Synix’s value prop.
    • Safe only if: parent label computation is correct for all transforms (map/reduce/fold/custom), and there are tests that detect provenance explosions/incorrect edges.

Findings

  1. src/synix/search/adapter.py: wrong released_at value

    • Writes ("released_at", closure.created_at) which is snapshot creation time, not release time.
    • Failure mode: receipts/metadata lie; downstream tooling that expects release time breaks.
    • Severity: [warning]
  2. src/synix/search/adapter.py: no FTS/SQLite pragmas; unbounded transaction size

    • Bulk inserting artifacts without batching or executemany, no WAL/synchronous tuning.
    • Failure mode: huge release times / memory spikes on 10k+ artifacts; possible DB corruption risk on crash.
    • Severity: [warning]
  3. src/synix/build/release_engine.py: no locking despite RFC claiming per-release locks

    • No .lock acquisition; concurrent synix release can race and interleave writes + ref updates.
    • Failure mode: torn releases, receipt doesn’t match files, ref points to snapshot not actually materialized.
    • Severity: [critical]
  4. src/synix/cli/search_commands.py: scratch realization leaks work dirs

    • _scratch_realize() creates temp dir under .synix/work/ and never deletes it.
    • Failure mode: disk bloat over time; “ephemeral” isn’t ephemeral.
    • Severity: [warning]
  5. src/synix/build/snapshot_view.py: SnapshotArtifactCache created_at parsing is wrong

    • Reads metadata["created_at"] but artifact object schema in docs doesn’t show that field in metadata; previous system used Artifact.created_at.
    • Failure mode: nondeterministic “Last Built” and status ordering; potential ValueError on invalid ISO strings.
    • Severity: [minor]
  6. src/synix/build/runner.py: projection declarations capture all current artifacts, not declared closure

    • _record_snapshot_projections() uses proj.sources and then records all artifact labels in those layers. That’s not “structured declaration”, that’s a baked closure.
    • Failure mode: manifest size grows with artifact count; diff noise; projection input ordering instability; breaks “declaration vs state” separation.
    • Severity: [warning]
  7. Validators/Fixers still write into legacy build/ paths

    • Goldens show FileNotFoundError ... build/tmp*.tmp and build/violations.jsonl.
    • Concrete code smell: ViolationQueue.load(build_path) and validator state persistence are still rooted in build_path (not .synix/work/ or .synix/).
    • Failure mode: synix validate and synix fix fail in the new world (your goldens literally show it).
    • Severity: [critical]
  8. src/synix/build/fixers.py: “apply_fix” silently no-ops under snapshot cache

    • You explicitly say SnapshotArtifactCache is read-only; apply_fix() becomes a placebo.
    • Failure mode: user “accepts” fixes, nothing persists; massive trust hit.
    • Severity: [critical]
  9. src/synix/build/plan.py: projection planning semantics now “declared” without telling user what will change

    • Previously plan explained cached vs rebuild for projections; now it punts to release.
    • Failure mode: planning no longer predicts user-facing changes/cost (search index rebuild time).
    • Severity: [minor]

Missing

  • Release locking + concurrency tests (per-release .lock behavior is in RFC, absent in code).
  • End-to-end tests proving build → release → search/validate/fix works without legacy build/.
  • Migration story/tests for old projects that already have build/ + old .synix/ v1 objects.
  • A real deletion/cleanup policy for .synix/work/ scratch realizations.
  • Fix/validate state storage relocation (violations queue, logs, traces) into .synix/ with documented lifecycle.

Verdict

Block. You’re changing the platform contract (build/release + storage) while core commands (validate/fix) are broken and release is unsafe under concurrency; fix those and add end-to-end + locking/cleanup tests before merging.

Review parameters
  • Model: gpt-5.2
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 30,334 total — truncated)
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-03-08T20:38:53Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Summary

This PR implements the build/release separation described in a new RFC (projection-release-v2-rfc.md). It removes the mutable build/ directory as source of truth, deletes ArtifactStore and ProvenanceTracker, replaces them with ObjectStore/SnapshotView/SnapshotArtifactCache, introduces a synix release command with an adapter contract (plan/apply/verify), and adds synix_search and flat_file adapters. Projections are no longer materialized at build time — they become structured manifest declarations materialized explicitly via synix release.

Alignment

This is strongly aligned with the core vision. DESIGN.md §1.5 explicitly called out that "future release targets should be materialized from immutable snapshot state." The PR delivers exactly that. Content-addressed immutable artifacts (DESIGN.md §3.3), complete provenance chains embedded in objects (§3.9 audit determinism), and the DAG-based dependency model are all preserved and strengthened. The adapter contract (ReleaseClosure as universal input) cleanly implements the "adapters are not special" principle from the RFC, which directly supports Hypothesis 3 (architecture is a runtime concern) — you can now release the same snapshot to different targets.

Observations

  1. [concern] Validate/fix golden files show crashes. The tv-returns and team-report golden files now expect FileNotFoundError: build/violations.jsonl and Build directory not found: build. The ViolationQueue still writes to build/ (see validators.py save_state/_append_log), but build/ no longer exists. This is a real regression — validate and fix are broken for the new architecture.

  2. [concern] SnapshotArtifactCache.__init__ loads all artifacts into memory eagerly. For 10,000+ conversations with 4 layers, this could mean 40K+ artifact objects deserialized at startup. The BFS provenance walk in ReleaseClosure.from_snapshot also loads everything. This is O(n) memory with no pagination — fine for now but should be flagged.

  3. [concern] _walk_provenance and get_chain use queue.pop(0) (list as deque). This is O(n²) for large chains. Use collections.deque instead. Appears in release.py, snapshot_view.py (twice), and SnapshotArtifactCache.get_chain.

  4. [concern] FixContext.store and ValidationContext.store are typed as object. Duck typing without a Protocol definition makes the contract invisible. A Protocol class would make the required methods (load_artifact, list_artifacts, get_parents, get_chain, iter_entries) explicit and IDE-discoverable.

  5. [question] The _save_plan_artifact function in build_commands.py creates a full snapshot+manifest just to persist a build plan. This advances refs/heads/main to point at a plan-only snapshot, which would break a subsequent real build's parent chain. Is this intentional?

  6. [question] release_engine.py has no file-level locking despite the RFC specifying per-release locks (.synix/releases/<name>/.lock). Concurrent releases to the same target could corrupt state.

  7. [positive] The ReleaseClosure dataclass is frozen and cleanly separates resolution from materialization. Adapters receive fully resolved content and provenance — no adapter touches the object store directly. This is a well-designed seam.

  8. [positive] Manifest schema version bumped to 2 with thorough validation of the new projections structure in object_store.py. Backward compatibility path is clear.

  9. [positive] Atomic writes via tempfile + os.replace in both adapters. The shadow-copy pattern in SynixSearchAdapter is correct.

  10. [nit] ReleaseProvenanceProvider in search_commands.py and _ProvenanceShim in snapshot_view.py are duplicated shim classes with identical structure. Extract to a shared module.

  11. [nit] pipeline.build_dir is still referenced throughout but build_dir no longer exists as a concept. The synix_dir_for_build_dir adapter keeps it working, but the naming is now confusing.

Verdict

This is a well-motivated architectural step that completes the immutable-snapshot vision, but it ships with broken validate/fix flows (golden files confirm crashes) and missing release-level locking — both should be addressed before merge.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 30,334 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-08T20:39:01Z

- _save_plan_artifact: write to refs/plans/latest instead of refs/heads/main
  to avoid clobbering build HEAD (critical finding from both reviewers)
- release_engine: call adapter.verify() after apply(), abort on failure
- _scratch_realize: register atexit cleanup for ephemeral work dirs
- search adapter: use actual release time for released_at metadata
- release.py: use collections.deque for O(1) BFS popleft
- Update test_plan_save_flag to verify via refs/plans/latest
- Update test_search_apply_creates_release_metadata for dynamic timestamp
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Threat assessment — High risk: this PR is an architectural rewrite that changes persistence, CLI workflow, and provenance, and the diff shows multiple user-facing regressions and broken demo goldens.

One-way doors

  1. Build/Release split + removal of build/ directory

    • Why hard to reverse: it changes the mental model (“build produces nothing usable”), breaks existing scripts, and forces a two-step workflow (build then release) into docs/templates.
    • Safe only if: there’s a clear compatibility story (auto-release flag or transitional build/ materialization), and CI proves old workflows either still work or fail with crisp migration errors.
  2. Manifest schema v2 with structured projections

    • Why hard to reverse: snapshots in .synix/objects/ become permanent history; readers/adapters will couple to shape.
    • Safe only if: schema versioning + migration tooling is implemented/tested, and ObjectStore validation matches all writers.
  3. Release receipts + refs (refs/releases/<name>)

    • Why hard to reverse: you’re shipping a git-like ref model and receipt JSON that users will automate against.
    • Safe only if: receipt schema is explicitly versioned and documented as stable/unstable, with tests ensuring backward compatibility.
  4. Provenance source-of-truth switch (parent_labels embedded; no provenance.json)

    • Why hard to reverse: all lineage traversal now depends on artifact object fields and correctness of parent label recording.
    • Safe only if: provenance correctness is validated in tests across all transform patterns (map/reduce/fold/custom).

Findings

  1. templates/* goldens now showing failures for validate/fix (templates/02-tv-returns/golden/validate_*.{json,stdout,stderr}.txt, templates/03-team-report/...)

    • Failure mode: the new architecture still has validators/fixers writing to build/violations.jsonl / using build/ temp paths. Your golden updates literally encode “Build directory not found: build”.
    • Severity: [critical]
  2. Validator state persistence still tied to build_path (src/synix/build/validators.py via ViolationQueue.load(build_path) and internal save_state/_append_log paths)

    • Failure mode: FileNotFoundError when build/ doesn’t exist; breaks synix validate / synix fix.
    • Severity: [critical]
  3. Release engine lacks locking despite RFC promises (src/synix/build/release_engine.py)

    • Failure mode: concurrent synix release can interleave writes to .synix/releases/<name>/search.db, receipt.json, .pending.json, and ref updates → corrupted releases.
    • Severity: [critical]
  4. SnapshotArtifactCache is not actually read-only (and is inconsistent) (src/synix/build/snapshot_view.py)

    • Failure mode: update_from_build() overlays artifacts without updating _parent_labels_map. Then _snapshot_parent_labels() falls back to cache.get_parents() which may be stale/empty, producing wrong lineage in the committed snapshot.
    • Severity: [critical]
  5. ReleaseProvenanceProvider.get_record() invents parents from full chain (src/synix/cli/search_commands.py)

    • Failure mode: parent_labels becomes “all ancestors” not “direct parents”, changing tree shape and inflating downstream traversal; display/provenance semantics drift.
    • Severity: [warning]
  6. SynixSearchAdapter stores layer_level as string in FTS (src/synix/search/adapter.py)

    • Failure mode: schema inconsistency (expects numeric elsewhere); sorting/filtering may break subtly.
    • Severity: [warning]
  7. Memory/perf risk: closure construction loads all artifact objects+content (src/synix/build/release.py)

    • Failure mode: release becomes O(total artifacts) memory spike; will fall over at 10k+ artifacts, especially with large contents.
    • Severity: [warning]
  8. Plan artifact writer creates its own snapshot with schema_version = SCHEMA_VERSION (src/synix/cli/build_commands.py::_save_plan_artifact)

    • Failure mode: snapshot.schema_version now incorrectly uses object store schema version (2) rather than snapshot schema (still 1 in RFC/code elsewhere). This will break readers expecting snapshot schema 1 fields.
    • Severity: [critical]

Missing

  • Tests for the new invariants (release doesn’t create build/, validate/fix work without build/, concurrent release serialization, provenance correctness).
  • A migration/compat layer for validator/fixer state into .synix/work/ (or .synix/violations/) instead of build/.
  • Explicit decision on default release selection (synix search behavior when 0/1/N releases exist) with stable UX.
  • CI proof that templates/demos succeed end-to-end; right now goldens accept failure.

Verdict

Block. Core CLI flows (validate, fix) are broken under the new “no build dir” contract, provenance caching is inconsistent, and release lacks concurrency safety—this isn’t mergeable until those are fixed and covered by tests.

Review parameters
  • Model: gpt-5.2
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 30,381 total — truncated)
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-03-08T20:53:47Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Summary — This PR implements the "projection release v2" architectural change: synix build now produces only immutable snapshots in .synix/; a new synix release command materializes projections (search.db, context.md) into named release targets via an adapter contract. ArtifactStore, ProvenanceTracker, and the build/ directory are removed. A comprehensive 800-line RFC accompanies the code.

Alignment — Strongly aligned. This advances the DESIGN.md §1.5 vision of separating mutable build surfaces from immutable canonical history. Content-addressed objects, immutable snapshots, and provenance embedded in artifact objects (not a side-car JSON file) all strengthen the core principles. The adapter contract (plan/apply/verify) is a clean extension point consistent with DESIGN.md's "architecture is a runtime concern" hypothesis — you could ship to Postgres or Qdrant without platform changes. The ReleaseClosure as a fully-resolved, portable artifact bundle is an elegant reification of the build-system-for-memory model.

Observations

  1. [concern] Validate/fix commands are broken. Golden files for templates 02, 03, 04 now show FileNotFoundError: 'build/violations.jsonl' and Build directory not found: build. ViolationQueue still writes to build/ which no longer exists. This is a shipped regression in experimental but user-visible features.

  2. [concern] SnapshotArtifactCache.__init__ loads all artifacts + content blobs into memory. For 10,000+ conversations, this is an unbounded memory issue. Every SnapshotArtifactCache construction (runner, planner, every CLI command) deserializes the entire object store. Consider lazy loading.

  3. [concern] No tests in this diff. The RFC specifies detailed unit, integration, E2E, and failure test plans. None appear to be included. For a change that rewrites the runner, planner, all CLI commands, and removes two core modules, this is a significant gap.

  4. [concern] apply_fix is effectively a no-op. It calls getattr(store, "save_artifact", None) which returns None for SnapshotArtifactCache. The comment says fixes persist "on the next build run" but no mechanism carries the rewritten content forward.

  5. [question] Provenance tree expansion in search results. Golden files show provenance trees now include transitive ancestors (e.g., monthly-2025-06 now shows t-claude-* as direct children alongside ep-*). Is this the intended behavior of ReleaseProvenanceProvider reading pre-baked chains, or a flattening bug?

  6. [nit] Duck-typed store: object on ValidationContext and FixContext loses all static analysis value. A Protocol class would preserve the duck-typing flexibility while giving IDE/mypy support.

  7. [positive] RFC quality is excellent. The filesystem layout, adapter contract, release transaction semantics, diff semantics, and scratch realization design are thorough and well-reasoned. Open questions are honest.

  8. [positive] Atomic writes throughout. FlatFileAdapter uses tempfile + os.replace; SynixSearchAdapter uses shadow-file swap. Correct for concurrent access.

  9. [positive] _record_snapshot_projections cleanly maps pipeline projection types to structured manifest declarations with proper fingerprinting. This makes manifests diffable as the RFC promises.

  10. [concern] build_dir parameter is still threaded through most CLI commands as the default "./build" even though build/ no longer exists. This is confusing — it's only used to locate .synix/ via synix_dir_for_build_dir. The indirection should be documented or the parameter renamed.

Verdict — The architecture is right and well-documented, but this PR ships broken validate/fix workflows, has no tests, and introduces an O(n) memory issue. It should land the RFC and adapter infrastructure, but the template golden-file regressions indicate incomplete migration that needs resolution before merge.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 30,381 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-08T20:54:12Z

Strip validators and fixers from demo templates 02-04 and the init
scaffold. The feature is not fully baked with the new release
architecture and is marked experimental in docs. All validate/fix
golden files removed, step counts renumbered, goldens regenerated.

The validator/fixer code and its dedicated tests remain intact —
only the demo showcase is removed.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Threat assessment — High risk: this PR rips out the mutable build/ world, introduces a new manifest schema, a new release lifecycle, and rewires provenance/search/validation paths with lots of edge cases and little test evidence in the diff.

One-way doors

  1. Build/Release separation + removal of build/ as a concept (CLI + mental model)

    • Why hard to reverse: Users will script synix build + synix release + --release; templates/docs now encode it. Reintroducing build/ later would be a second migration.
    • Safe if: there’s a documented migration story for existing users, explicit compatibility mode duration, and CI coverage proving old pipelines still work.
  2. Manifest schema v2 projections declaration shape (manifest.projections with adapter/input_artifacts/config/config_fingerprint)

    • Why hard to reverse: Stored in .synix/objects/; older snapshots persist. Any schema churn needs readers, migrators, and allow_older_schema policy.
    • Safe if: schema versioning + forward/backward compatibility is actually implemented and tested (read old manifests, write new, diff).
  3. Release receipts as the “proof of what is live where” (.synix/releases/<name>/receipt.json + history)

    • Why hard to reverse: Receipts become audit log and user tooling depends on it; changing fields breaks automation.
    • Safe if: receipt schema is formally versioned and --json outputs are guaranteed stable.
  4. Search now targets releases (default selection + --ref scratch realizations)

    • Why hard to reverse: Changes default UX and support burden; scripts now need --release.
    • Safe if: robust defaulting rules, good errors, and release selection is deterministic across projects.

Findings

  1. src/synix/build/object_store.pySCHEMA_VERSION = 2 used globally

    • Risk: You changed the global schema version, but snapshots/objects created elsewhere may still be v1; readers/writers may silently diverge. Also snapshot objects appear to use schema_version but the validator logic is unclear for mixed versions.
    • Severity: [critical]
  2. src/synix/build/runner.py — projections recorded from proj.sources

    • Risk: SynixSearch is defined as surface=..., not sources=[...]. This code does source_layer_names = [s.name for s in proj.sources]. If SynixSearch.sources is empty/undefined, your manifest projection input_artifacts becomes empty, producing empty release search.db.
    • Severity: [critical]
  3. src/synix/build/release_engine.py — no locking despite RFC promising per-release locks

    • Risk: Concurrent synix release to same target can interleave writes: receipt/history/db swap/.pending cleanup. Corrupt or mismatched receipt vs artifacts is plausible.
    • Severity: [critical]
  4. src/synix/search/adapter.py — provenance chains limited to indexed artifacts only

    • Risk: Adapter only inserts provenance_chains for artifacts in the projection’s input_artifacts. But provenance chain includes ancestors that are not indexed. Your ReleaseProvenanceProvider.get_record() fakes parent_labels as “everything except self”, destroying graph structure and making lineage trees wrong.
    • Severity: [warning]
  5. src/synix/build/release.py::_walk_provenance — BFS includes self and loses edge structure

    • Risk: A “chain” is not a chain; BFS order is not a stable DAG path. Consumers will treat it as ordered lineage and get nonsense (and non-determinism if parent order varies).
    • Severity: [warning]
  6. src/synix/build/snapshot_view.py — O(n) object loads in prefix resolution + provenance

    • Risk: resolve_prefix() loads each artifact object to check artifact_id prefix. get_provenance() does repeated object loads per node. At 10k+ artifacts this will be slow and IO-heavy.
    • Severity: [warning]
  7. src/synix/build/fixers.py + validators.py — “duck-typed store” that is sometimes read-only

    • Risk: apply_fix() explicitly becomes a no-op for SnapshotArtifactCache; validators’ _store_llm_trace becomes no-op. UX will claim fixes/traces happened but they won’t persist. That’s a behavioral footgun.
    • Severity: [warning]
  8. src/synix/cli/search_commands.py — scratch realizations leak temp dirs on crashes

    • Risk: cleanup via atexit doesn’t run on SIGKILL or hard crashes; .synix/work/scratch_search_* can accumulate unbounded.
    • Severity: [minor]

Missing

  • Tests for release concurrency, idempotence, and failure atomicity (no ref advance on adapter failure; no partial receipts).
  • Tests proving SynixSearch uses surface sources correctly and produces non-empty release outputs.
  • A formal, versioned receipt JSON schema and compatibility policy.
  • Migration doc: what happens to existing users with build/, .projection_cache.json, and synix search without release.

Verdict

Block. The direction is coherent, but there’s a likely correctness bug in projection input resolution (SynixSearch vs sources) plus missing locking/atomicity guarantees that can corrupt release state.

Review parameters
  • Model: gpt-5.2
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 30,400 total — truncated)
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-03-08T21:26:50Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Summary — This PR implements the build/release separation described in a new RFC (projection-release-v2-rfc.md). synix build now writes only to .synix/ (immutable snapshots); synix release materializes projections to named targets via an adapter contract. The legacy ArtifactStore, ProvenanceTracker, build/ directory, and .projection_cache.json are removed. New modules: ObjectStore schema v2, SnapshotView, SnapshotArtifactCache, ReleaseClosure, ReleaseEngine, ProjectionAdapter contract, SynixSearchAdapter, FlatFileAdapter, plus CLI commands for release, revert, releases, and refs.

Alignment — Strongly aligned. DESIGN.md §1.5 explicitly called out that "future release targets should be materialized from immutable snapshot state." This PR delivers that. Content-addressed artifacts remain immutable. The DAG provenance model is preserved — parent_labels embedded in artifact objects replaces the separate provenance.json, which is cleaner. The adapter contract (plan/apply/verify) is a natural extension point consistent with the "Python-first" principle. The manifest schema v2 with structured projection declarations makes projections diffable, advancing the "architecture is a runtime concern" hypothesis. The removal of build/ as a platform concept eliminates the dual-write split-brain problem documented in the RFC's baseline section.

Observations

  1. [positive] ReleaseClosure.from_snapshot (release.py) walks provenance once and hands a fully resolved bundle to adapters. This is the right abstraction — adapters never touch the object store directly.

  2. [positive] The SynixSearchAdapter (search/adapter.py) builds a self-contained search.db with provenance_chains and release_metadata tables baked in. Released artifacts are portable without .synix/, matching the RFC's design principle.

  3. [concern] SnapshotArtifactCache.__init__ eagerly loads every artifact's content blob into memory. With 10,000+ conversations this will be problematic — each artifact's full text is decoded and stored. Consider lazy loading content (load metadata eagerly, content on demand).

  4. [concern] _walk_provenance in release.py and get_provenance in snapshot_view.py both do BFS over parent_labels but include the starting label itself in the chain. The BFS in ReleaseClosure also visits transitively through all ancestor objects. For deep DAGs with shared parents this could produce very large chains. No depth limit is applied.

  5. [question] FixContext.store and ValidationContext.store are now typed as object with duck-typing expectations documented in docstrings. Would a Protocol class be clearer for the contract? The getattr(store, "save_artifact", None) pattern in apply_fix is fragile.

  6. [concern] The _save_plan_artifact function in build_commands.py writes a full manifest+snapshot just for the plan artifact under refs/plans/latest. This creates a valid snapshot with only one artifact, which could confuse SnapshotArtifactCache if it accidentally reads refs/plans/latest instead of HEAD.

  7. [nit] ReleaseProvenanceProvider and _ProvenanceShim are defined in both search_commands.py and snapshot_view.py. The duplicate _ProvenanceShim could be consolidated.

  8. [question] The execute_release function doesn't acquire the per-release lock described in the RFC (".synix/releases/<name>/.lock"). Is this deferred to a follow-up?

  9. [positive] synix clean now correctly distinguishes between release state (deletable) and snapshot history (preserved), matching the RFC's design.

  10. [concern] Demo templates (02-tv-returns, 04-sales-deal-room) had their validators and fixers stripped from pipeline.py. The validator/fixer infrastructure still exists in the codebase but the demos no longer exercise it. This reduces e2e coverage of the validate→fix lifecycle.

  11. [positive] Golden files are updated consistently, and the normalization regex in demo_commands.py handles release-specific output patterns well.

  12. [nit] The search command's --release / --ref / legacy fallback resolution logic in search_commands.py is ~80 lines of branching. A helper function or table-driven resolution would improve readability.

Verdict — This is a well-executed architectural migration that resolves the documented split-brain problem and delivers the build/release separation promised in the design. The eager content loading in SnapshotArtifactCache and the missing release lock are the most important items to address before this scales, but as an incremental step this is sound.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 30,400 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-08T21:27:02Z

…eipt schema

Tests:
- Release failure atomicity: adapter apply/verify failure → no receipt,
  no ref advance, .pending.json preserved; no partial receipts when
  second adapter fails; idempotent re-release
- SynixSearch adapter: surface sources filter correctly (only declared
  artifacts indexed), non-empty release output validation, empty
  input_artifacts edge case

Docs:
- docs/migration-v2.md: migration guide for existing users (build/ →
  .synix/, search requires release, what to delete)
- docs/receipt-schema.md: formal versioned receipt JSON schema with
  compatibility policy

Fix:
- demo_commands.py: normalize Rich line-wrapping of release → path
  across narrow terminals (fixes CI demo-01 failure)
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Threat assessment — High risk: this PR introduces a new persistence model + new CLI workflow + new on-disk schemas, with multiple correctness and migration gaps that will strand users and corrupt provenance/search behavior.

One-way doors

  1. Manifest/object schema bump to v2 (src/synix/build/object_store.py: SCHEMA_VERSION = 2)
    Hard to reverse because existing .synix/objects/* will be written with v2 manifests (projections now structured). Any downgrade path becomes “wipe your store”.
    Safe only if: explicit compatibility policy + reader supports v1+v2; test fixtures proving opening old stores; and a documented migration story (not just “may evolve”).

  2. CLI workflow change: build no longer materializes outputs; release required (README.md, cli/*, DESIGN.md)
    Hard to reverse because templates/docs/scripts will bake in synix release HEAD --to local and synix search --release.
    Safe only if: synix build --release <name> convenience (or default release name) exists, and synix search UX is unambiguous (see findings).

  3. Release receipt format + history directory (src/synix/build/release_engine.py, docs/receipt-schema.md)
    Hard to reverse because receipts become audit artifacts and tooling will parse them.
    Safe only if: schema is validated and versioned in code (not just docs), and receipt writing is atomic + consistent across failure modes.

  4. Removing build/ as a platform concept
    Hard to reverse because any external integrations relying on build/search.db / build/context.md break.
    Safe only if: the “compatibility window” is real in code (not just docs) and CI covers both paths until deprecation.

Findings

  1. src/synix/build/runner.py: _projection_config_fingerprint uses json/hashlib but runner doesn’t import them
    Failure mode: runtime NameError during build when recording projections → build fails.
    Severity: [critical]

  2. src/synix/cli/search_commands.py: scratch realization leaks disk; cleanup only at process exit
    Uses atexit.register(shutil.rmtree, work_dir, True); if the process is killed or crashes, .synix/work/scratch_search_* accumulates unbounded.
    Severity: [warning]

  3. src/synix/build/release.py: provenance chain includes the artifact itself
    _walk_provenance starts queue with label and appends current; chain becomes [self, parents...]. UI output now duplicates leaves / changes lineage semantics (seen in goldens). This is a silent contract change.
    Severity: [warning]

  4. src/synix/build/release.py: unbounded closure resolution (loads all artifact content into RAM)
    ReleaseClosure.from_snapshot loads every artifact JSON + decodes all content blobs. On 10k+ conversations this will be slow and memory-heavy; release/search scratch will OOM.
    Severity: [warning]

  5. src/synix/build/release_engine.py: no locking despite RFC claiming per-release locks
    Concurrent synix release --to local can interleave shadow db swaps and receipts/history writes, producing mismatched receipt vs files.
    Severity: [critical]

  6. src/synix/search/adapter.py: FTS5 schema uses layer_level as TEXT
    SearchIndex queries expect numeric ordering/filtering (CAST(layer_level AS INTEGER) appears elsewhere). Storing as string invites subtle ordering bugs (“10” < “2”).
    Severity: [warning]

  7. src/synix/build/snapshot_view.py: provenance cache is stale after build overlay
    SnapshotArtifactCache.update_from_build() overlays artifacts/manifest entries but does not update _parent_labels_map. Validators/fixers/status can read new artifacts with old parents → wrong lineage + wrong “stale” detection.
    Severity: [critical]

  8. src/synix/build/fixers.py: “apply_fix” becomes a no-op for snapshot stores
    Docstring admits SnapshotArtifactCache is read-only; fixes “persist next build”. That’s not a fix command; it’s a suggestion. Also the CLI still offers accept/apply.
    Severity: [warning]

  9. src/synix/cli/*: synix-dir resolution is inconsistent and sometimes silently returns “No artifacts found”
    E.g. artifact_commands.list swallows ValueError from SnapshotView.open and prints dim output. This makes debugging broken stores painful.
    Severity: [minor]

  10. src/synix/cli/build_commands.py: _save_plan_artifact writes a new snapshot with schema_version = SCHEMA_VERSION (2) for snapshot object
    Snapshot schema version in RFC is 1; mixing schema versions across object types is sloppy and will break validators/older readers.
    Severity: [warning]

Missing

  • Tests for: release concurrency/locking; snapshot v1→v2 compatibility; release failure atomicity (no ref advance, no receipt, no partial outputs); large-store release memory behavior; SnapshotArtifactCache.update_from_build() provenance correctness.
  • Code-level JSON schema validation for receipts (currently only docs).
  • A real migration tool: synix migrate / auto-detect old build/ and import to .synix if needed.
  • Clear default behavior for synix search when no releases exist (current error message says “or specify --release”, but auto-detect might still fall back to legacy build db inconsistently).

Verdict

Block — the architecture direction is fine, but there are multiple critical correctness issues (missing imports, no release locking, stale provenance after build) and missing tests that make this unsafe to ship.

Review parameters
  • Model: gpt-5.2
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 31,016 total — truncated)
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-03-08T21:39:01Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Summary

This PR implements the "Projection Release v2" architecture: a complete separation of build and release. synix build now writes only to .synix/ (content-addressed objects, snapshots, refs). A new synix release command materializes projections (search.db, context.md) into named release targets via an adapter contract. The build/ directory, ArtifactStore, ProvenanceTracker, and .projection_cache.json are removed.

Alignment

This is a strong architectural advancement. It deepens the build-system analogy from DESIGN.md — builds produce immutable artifacts, releases are explicit deployment acts (like bazel build vs. deploying the output). Content-addressed storage, immutable snapshots, and provenance embedded in artifact objects all strengthen the core principles. The adapter contract (plan/apply/verify) for projections directly enables the "architecture evolution" hypothesis: swap a Postgres adapter for the local file adapter without changing the build. The ReleaseClosure design — fully resolving artifacts before handing them to adapters — keeps adapters decoupled from the object store, which is clean. Manifest schema v2 with structured projection declarations makes projections diffable, advancing the "cache keys capture all inputs" principle.

Observations

  1. [positive] ReleaseClosure.from_snapshot walks provenance and resolves all content before adapter dispatch. Adapters never touch the object store. This is the right separation.

  2. [positive] Receipt history with append-only history/ directory and schema-versioned receipts provides audit determinism for releases — consistent with DESIGN.md's audit-over-reproducibility stance.

  3. [concern] SnapshotArtifactCache.__init__ loads all artifacts into memory at construction. For 10,000+ conversations with 4 layers, this could be ~40,000 artifact objects loaded eagerly. The ReleaseClosure.from_snapshot method does the same. Both need lazy loading or pagination for scale.

  4. [concern] SnapshotArtifactCache.update_from_build mutates a nominally read-only cache in-place. The comment explains why, but this creates a hybrid mutable/immutable object that could confuse contributors. Consider a wrapper type or explicit naming like MutableBuildCache.

  5. [concern] The apply_fix function in fixers.py uses getattr(store, "save_artifact", None) duck-typing with a comment "fix will be persisted on the next build run." This means fixes are silently no-ops with SnapshotArtifactCache. A user running synix fix will see "applied" but nothing persists — this needs at minimum a warning in the CLI output.

  6. [question] _save_plan_artifact in build_commands.py writes to refs/plans/latest to avoid clobbering refs/heads/main. Good instinct, but it creates a full snapshot+manifest for a single ephemeral artifact. Is this the intended long-term pattern, or should plan artifacts go elsewhere?

  7. [concern] No tests are included in this diff. The RFC lists comprehensive test plans (test_manifest_v2.py, test_release_closure.py, etc.) but none ship here. For a change that removes ArtifactStore, ProvenanceTracker, rewrites every CLI command's read path, and introduces a new release engine, the absence of tests is a significant gap.

  8. [nit] _ProvenanceShim is defined identically in both snapshot_view.py and search_commands.py. Extract to a shared location.

  9. [positive] The demo golden files are updated consistently, and the normalization logic in demo_commands.py handles the new release output patterns. This shows the E2E flow was exercised manually even without formal tests.

  10. [question] The search command's source resolution priority (--ref > --release > auto-detect > legacy build/) is complex. The fallback chain spans ~50 lines. Is there a risk users hit the legacy path accidentally when they intended release-based search?

  11. [concern] Templates 02 and 04 had their validators and fixers removed from pipeline.py entirely (not just migrated). This breaks the validate/fix demo story that was a selling point. Was this intentional simplification for this PR, or an oversight?

Verdict

This is a well-conceived architectural step that completes the build/release separation described in DESIGN.md — but it should not merge without tests covering at least the new ReleaseClosure, adapter contract, SnapshotArtifactCache, and the release engine's transaction semantics.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 31,016 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-08T21:39:13Z

Detailed status, blocking CI issue with fix instructions,
commit history, review findings, deferred items, and pickup
instructions for continuing on another machine.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Warning

AI Review failed — openai | RateLimitError

The openai blind review could not be completed. This does not affect the PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Summary — This PR completes the build/release separation: synix build now produces only immutable snapshots in .synix/; a new synix release command materializes projections (search.db, context.md) to named release targets via an adapter contract. The build/ directory, ArtifactStore, ProvenanceTracker, and .projection_cache.json are removed entirely.

Alignment — Strongly aligned. This is the natural completion of what DESIGN.md §1.5 describes: "build history should survive synix clean, and future release targets should be materialized from immutable snapshot state." The content-addressed ObjectStore as single write path reinforces artifact immutability. The ReleaseClosure — which walks parent_labels transitively and hands adapters a fully-resolved bundle — preserves complete provenance chains (DESIGN.md §3.9). The adapter contract (plan/apply/verify) is cleanly extensible for the future Postgres/Qdrant adapters mentioned in the RFC. Schema version bump to 2 with structured projection declarations makes manifests diffable, advancing the "architecture is a runtime concern" hypothesis.

Observations

  1. [positive] SnapshotView.get_provenance and SnapshotArtifactCache.get_chain still use queue.pop(0) — O(n²) for long chains. The handoff doc says this was fixed with deque in release.py (_walk_provenance), but the fix wasn't applied to the cache/view classes. (snapshot_view.py lines ~128, ~285)

  2. [concern] SnapshotArtifactCache.update_from_build does not refresh _parent_labels_map — acknowledged in the handoff doc as a known limitation. Validators calling get_parents() on newly-built artifacts will get stale (empty) results. Acceptable for experimental validators, but should be tracked.

  3. [concern] apply_fix with SnapshotArtifactCache silently no-ops on save — the getattr(store, "save_artifact", None) guard means fixes vanish without user feedback. The code should at minimum log a warning.

  4. [positive] The adapter contract is clean and unprivileged — SynixSearchAdapter and FlatFileAdapter follow the same interface. The lazy registry via _BUILTIN_ADAPTERS avoids circular imports elegantly.

  5. [positive] Atomic writes throughout: FlatFileAdapter uses tempfile + os.replace, SynixSearchAdapter uses a shadow DB with os.replace, release receipts use atomic_write. This is correct for crash safety.

  6. [question] _save_plan_artifact creates a full snapshot (manifest + snapshot object) and writes to refs/plans/latest. Is this the right ref namespace? Plan artifacts aren't builds — polluting the object store with plan snapshots that never get cleaned up could accumulate.

  7. [positive] The handoff document is exceptionally thorough — 15 commits mapped, all review findings addressed with commit hashes, blocking CI issue documented with exact fix, pickup instructions provided.

  8. [concern] The search command's source resolution has a complex priority chain (--ref → --release → auto-detect release → fallback to legacy build/). The fallback to legacy resolve_search_output may confuse users during migration if both .synix/releases/ and build/search.db exist.

  9. [positive] Validators and fixers were correctly removed from demo templates since they're experimental and the store is now read-only. This avoids confusing demo failures.

  10. [nit] Provenance tree output in goldens changed from └── to ├── patterns because parent_labels now includes transitive ancestors in the displayed tree. The provenance is richer but visually noisier.

  11. [positive] Receipt schema is versioned (schema_version: 1), has a formal spec document, and from_dict uses .get() with defaults for forward compatibility.

Verdict — This is a well-executed architectural milestone that eliminates the split-brain dual-write problem, makes .synix/ the single source of truth, and introduces a clean adapter contract — all strongly aligned with the build-system-for-memory vision.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 31,169 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-08T21:45:55Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Warning

AI Review failed — openai | RateLimitError

The openai blind review could not be completed. This does not affect the PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Summary

This PR implements the "Projection Release v2" RFC: it removes the build/ directory as a platform concept, makes .synix/ the single source of truth for all state, and introduces an explicit synix release lifecycle with adapter contracts, receipts, and named release targets. It is a large architectural migration (~4,000 lines added, ~3,000 removed) touching the runner, planner, CLI, search, validators, fixers, batch runner, demos, and all documentation.

Alignment

Strongly aligned. This advances several DESIGN.md principles directly:

  • Artifacts immutable and content-addressed: The dual-write to build/ is eliminated; ObjectStore is now the single write path. Content addressing is the only storage model.
  • Provenance chains complete: Provenance moves from a separate provenance.json file into artifact objects via parent_labels, walked transitively. No information loss — provenance is now structurally inseparable from artifacts.
  • Cache keys capture all inputs: SnapshotArtifactCache replaces ArtifactStore with the same fingerprint-based rebuild logic reading from immutable snapshots.
  • Build system analogy: The build/release split mirrors compilation vs. deployment — synix build produces artifacts, synix release materializes them. This is a cleaner expression of the DESIGN.md vision than the previous implicit materialization during build.

The adapter contract (plan/apply/verify) directly enables the extension model DESIGN.md gestures toward with future Postgres/Qdrant adapters.

Observations

  1. [positive] SnapshotView.get_provenance and SnapshotArtifactCache.get_chain both use queue.pop(0) on a plain list — O(n²) for deep chains. The handoff doc mentions fixing this with deque in release.py but these two instances remain. (snapshot_view.py lines ~130, ~280)

  2. [concern] SnapshotArtifactCache.__init__ eagerly loads every artifact's content into memory. For 10,000+ artifacts this could be significant. The old ArtifactStore loaded lazily from disk. Consider lazy content loading.

  3. [concern] apply_fix with SnapshotArtifactCache is explicitly a no-op (the handoff doc acknowledges this). Fixes silently vanish. The code uses getattr duck-typing rather than failing loudly — a user running synix fix would see "applied" but nothing persists. Should at minimum warn.

  4. [positive] The ReleaseClosure.from_snapshot pattern is clean — fully resolves artifacts + provenance before handing to adapters. Adapters never touch the object store directly, which is exactly what the RFC promises.

  5. [question] _save_plan_artifact in build_commands.py writes a full snapshot+manifest just to persist a plan artifact under refs/plans/latest. Is a snapshot the right granularity for a throwaway plan? This creates objects that will never be garbage-collected.

  6. [positive] Atomic writes throughout: FlatFileAdapter uses tempfile + os.replace; SynixSearchAdapter uses shadow DB + os.replace; receipts use atomic_write. Crash safety is well handled.

  7. [nit] ReleaseProvenanceProvider in search_commands.py and _ProvenanceShim in snapshot_view.py are duplicated shim classes with identical structure. Extract to a shared location.

  8. [concern] Validators/fixers are removed from all demo templates (02, 03, 04) without migration notes in the templates themselves. Users following existing READMEs that mention synix validate will find it broken for those pipelines.

  9. [positive] The manifest schema version bump to 2 with structured projection validation in object_store.py is thorough — every required field is type-checked.

  10. [question] synix clean removes .synix/releases/ and .synix/work/ but the command still defaults --build-dir=./build. The legacy fallback path in clean that shutil.rmtrees build/ could surprise users who have unrelated build/ directories.

Verdict

This is a well-executed architectural migration that eliminates the split-brain dual-write problem and cleanly separates build from release — a strong incremental step that aligns with every stated design principle.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 31,170 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-08T22:14:48Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Threat assessment — High risk: this PR is a foundational architecture + UX pivot (build/release separation, on-disk format/schema bump, CLI contract changes) with multiple correctness and operability gaps.

One-way doors

  1. Removal of build/ as a platform concept + mandatory synix release step

    • Why hard to reverse: It rewrites the primary workflow (build no longer yields user-facing artifacts) and invalidates existing scripts, docs, and muscle memory. Reintroducing build/ later would reintroduce dual-write and inconsistency problems you just removed.
    • Safe if: you ship a compatibility mode/flag (even temporary) or an explicit migration/version gate that prevents silent behavior changes; plus clear error messages when users run synix search without a release.
  2. Manifest/ObjectStore schema bump to SCHEMA_VERSION = 2

    • Why hard to reverse: Anything persisted under .synix/objects/ becomes long-lived. Once users have schema v2 objects, your readers must support them forever or provide a migration tool.
    • Safe if: there is a documented compatibility policy and code-level read support for older objects (v1) with tests. Right now I only see validation changes, not a migration/upgrade story beyond docs.
  3. Public adapter contract (plan/apply/verify) + receipt schema

    • Why hard to reverse: External adapters will depend on these method names and receipt fields. You’re effectively defining a plugin API pre-1.0.
    • Safe if: you freeze it behind “experimental” namespacing or require an adapter API version field and enforce it in registry/loader + docs.
  4. Refs and release naming semantics (refs/releases/<name>, synix release HEAD --to local)

    • Why hard to reverse: Names leak into automation and human workflows; ref layout becomes part of the user mental model.
    • Safe if: you specify ref namespace stability (or mark experimental) and add subcommands for ref migration/rename.

Findings

  1. src/synix/build/runner.py: _projection_config_fingerprint uses json/hashlib but they’re not imported

    • Failure mode: runtime NameError during synix build when recording projections → build crashes.
    • Severity: [critical]
  2. src/synix/build/object_store.py: SCHEMA_VERSION globally bumped to 2

    • Failure mode: snapshot objects now written with schema 2, but other code may assume snapshot schema 1 (and your validator only special-cases manifest schema≥2). This is a classic partial-migration hazard.
    • Severity: [critical]
  3. src/synix/build/release_engine.py: no locking despite docs claiming .lock

    • Failure mode: concurrent synix release can interleave adapter writes; receipts/refs can reflect inconsistent on-disk state.
    • Severity: [warning]
  4. src/synix/build/release_engine.py: partial materialization not rolled back

  5. src/synix/build/snapshot_view.py: get_provenance() uses queue.pop(0)

    • Failure mode: O(n²) provenance walks on large graphs. You fixed this elsewhere with deque; not here.
    • Severity: [warning]
  6. src/synix/build/snapshot_view.py: SnapshotArtifactCache uses datetime.fromisoformat on created_at

    • Failure mode: stored timestamps may be Z-suffixed or not ISO-compatible; cache init breaks and silently falls back to empty cache (broad exception). That’s a reliability footgun.
    • Severity: [warning]
  7. src/synix/cli/search_commands.py: scratch realization cleanup via atexit only

    • Failure mode: on abnormal termination, work dirs accumulate; also atexit doesn’t help long-running processes. You need try/finally around the query path.
    • Severity: [warning]
  8. src/synix/build/fixers.py + validators.py: “fix will persist on next build”

    • Failure mode: user accepts a fix and nothing actually changes; this is effectively lying UX. If fixers are still exposed in CLI, they must either be disabled under snapshot store or write a new snapshot.
    • Severity: [warning]

Missing

  • A hard CI gate fix is referenced in docs/handoff-projection-release-v2.md but the PR still includes a golden normalization churn story; I don’t see a clear “CI now green” guarantee in the diff itself.
  • Compatibility tests for reading existing .synix v1 objects and manifests after schema bump.
  • Concurrency/locking tests for release targets (even a unit test asserting lock file behavior).
  • A single authoritative CLI doc update for synix search default behavior (release auto-detect vs error) consistent across README/llms/templates.

Verdict

Block — too many foundational one-way doors plus at least one obvious runtime crash (runner.py missing imports) and unresolved schema/compatibility risks. Fix the criticals, add compatibility + locking strategy, then re-review.

Review parameters
  • Model: gpt-5.2
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 31,192 total — truncated)
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-03-08T22:19:07Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 8, 2026

Note

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

Summary — This PR implements the "Projection Release v2" RFC: it separates build from release absolutely, removes the build/ directory as a platform concept, makes .synix/ the single source of truth, and introduces an explicit release lifecycle with adapter contracts, receipts, and named release targets. It's a large architectural migration touching the runner, planner, CLI, search, validators, fixers, batch runner, templates, and documentation.

Alignment — Strongly aligned. DESIGN.md §1.5 already anticipated this split ("future release targets should be materialized from immutable snapshot state"). Content-addressed immutable artifacts, full provenance via parent_labels embedded in objects, and the DAG structure are all preserved. The ReleaseClosure design — where the platform walks provenance and hands a resolved bundle to adapters — keeps adapters from touching the object store directly, which is clean separation. The adapter contract (plan/apply/verify) mirrors the build system ethos: declared intent → execution → verification. The precomputed_oid escape hatch for caching is appropriately marked as cache, not source of truth. Manifest schema v2 with structured projection declarations makes projections diffable, directly enabling the "architecture migration" thesis from §1.3.

Observations

  1. [concern] SnapshotView.get_provenance() and SnapshotArtifactCache.get_chain() use queue.pop(0) — O(n²) BFS. The handoff doc says this was fixed with collections.deque in commit 1250f59, but snapshot_view.py lines 130 and 297 still use list.pop(0). Only release.py uses deque. For 10k+ artifacts this matters.

  2. [concern] SnapshotArtifactCache.__init__ loads every artifact object and content blob into memory eagerly. With large builds (10k+ conversations × 4 layers), this could consume significant RAM. A lazy-loading approach would be safer.

  3. [concern] update_from_build() on SnapshotArtifactCache doesn't refresh _parent_labels_map — acknowledged in the handoff doc as a known limitation for validators. Since validators are experimental, acceptable for now but should be tracked.

  4. [question] The _save_plan_artifact function in build_commands.py creates a full snapshot+manifest+ref just to persist a plan artifact. It writes to refs/plans/latest which is good (avoids clobbering HEAD), but the plan snapshot has pipeline_fingerprint: "sha256:plan-save" — a hardcoded string. Is this intentional? It means plan snapshots can't participate in normal cache logic.

  5. [concern] apply_fix with SnapshotArtifactCache silently no-ops via getattr(store, "save_artifact", None). The fix vanishes with no user feedback. The handoff doc acknowledges this; it should at minimum log a warning.

  6. [positive] The adapter contract is clean: plan/apply/verify with ReleaseClosure as the universal input. synix_search and flat_file follow the same contract. The lazy adapter registry via importlib avoids circular imports elegantly.

  7. [positive] Atomic writes throughout — SynixSearchAdapter uses shadow file + os.replace; FlatFileAdapter uses tempfile + os.replace. Receipt writes use atomic_write. Release transaction semantics (.pending.json preserved on failure) are sound.

  8. [positive] The migration guide (docs/migration-v2.md) is clear and actionable. The RFC (docs/projection-release-v2-rfc.md) is thorough with acceptance criteria, test strategy, and open questions documented.

  9. [nit] ReleaseProvenanceProvider and _ProvenanceShim are duplicated in search_commands.py and snapshot_view.py. Could share a single definition.

  10. [positive] Demo cases updated consistently — validators/fixers removed from templates (marked experimental), release step added to all flows, golden files regenerated. The 5-demo gate is a strong regression check.

Verdict — This is a well-executed architectural migration that delivers on the immutable build history promise from DESIGN.md §1.5 and eliminates the split-brain dual-write problem. The BFS performance and eager loading are the main scale concerns to address before 10k+ artifact workloads, but for the current pre-1.0 single-user CLI, this is a strong incremental step.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 31,192 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-08T22:19:25Z

… tests

- Add per-request HTTP timeout (default 300s) via LLMConfig.timeout,
  wired through httpx.Timeout to both Anthropic and OpenAI SDK clients.
- Add 10-minute hard ceiling on ThreadPoolExecutor future.result() to
  prevent worker hangs from stalling the entire build.
- Write per-layer checkpoint manifests to .synix/checkpoints/ during
  builds so interrupted builds can recover cached artifacts on restart.
- SnapshotArtifactCache loads checkpoint artifacts as fallback when no
  committed snapshot covers them.
- Add content-addressed dedup assertions to incremental rebuild tests
  (verify artifact_ids and content are byte-identical across builds).
- Generate embeddings at release time in SynixSearchAdapter (fail-closed).
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 9, 2026

Note

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

Threat assessment — High risk: this PR introduces a new on-disk state model and CLI workflow (“build produces snapshots, release materializes outputs”), and it’s not fully hardened (locking, provenance correctness, validator/fixer integration), so regressions will be painful.

One-way doors

  1. Removal of build/ as a platform concept + .synix/ as sole source of truth
    Why hard to reverse: Users/scripts will reorganize around .synix/refs, .synix/releases, receipts, and the new expectation that synix build produces no user-facing outputs. Reintroducing build/ later would create dual sources again and break the new mental model.
    Safe to merge if: there’s a clearly documented compatibility story (including “how to migrate and how to recover if .synix is corrupted”), and basic multi-process safety is implemented (locks) so the new canonical store doesn’t get corrupted.

  2. New public CLI lifecycle: synix release, synix revert, synix releases, synix refs
    Why hard to reverse: CLI commands/flags become external contracts. Names like release, revert, receipts, and --to will be depended on immediately.
    Safe to merge if: help text + docs define semantics precisely (idempotency, failure behavior, what is and isn’t atomic), and the commands behave consistently across platforms.

  3. Manifest schema v2 projections (ObjectStore.SCHEMA_VERSION = 2, structured manifest.projections)
    Why hard to reverse: persisted object schema; older snapshots must remain readable. You now validate projections for schema>=2 but don’t show migration tooling.
    Safe to merge if: there is a tested forward/backward compatibility guarantee (read schema v1 and v2), and explicit schema-version bump rules.

  4. Receipt format and history semantics (.synix/releases/<name>/receipt.json + history/)
    Why hard to reverse: external systems will parse receipts.
    Safe to merge if: receipts are documented as versioned public API (not “internal”), and there are tests enforcing schema stability + upgrade path.

Findings

  1. src/synix/build/snapshot_view.py: O(n²) provenance traversal uses queue.pop(0)
    Risk: SnapshotView.get_provenance() and SnapshotArtifactCache.get_chain() do BFS with list pop(0). On large graphs (10k+ artifacts), lineage/search provenance becomes quadratic.
    Severity: [critical]

  2. src/synix/build/release.py: provenance chain includes the artifact itself and is BFS-ordered
    Risk: Chains are not a “path”; BFS order is not stable/meaningful as “lineage”. Also includes label as first element; downstream display code seems to treat it as ancestry. This already shows up in goldens where roots are duplicated/unexpected.
    Severity: [warning]

  3. src/synix/build/release_engine.py: no locking despite .synix/.lock being documented
    Risk: concurrent synix release (or release + clean) can corrupt release dirs/receipts/refs. You even document per-release locks in RFC; not implemented.
    Severity: [critical]

  4. src/synix/search/adapter.py: embedding generation likely wrong/misaligned
    Pattern: texts = [art.content for art in artifacts.values()] then provider.embed_batch(texts) with no label association.
    Risk: embeddings can’t be joined back to artifact labels unless EmbeddingProvider implicitly reads the DB; from diff, it looks like it writes under target_path, but mapping is unclear. High chance of silently broken semantic/hybrid search in releases.
    Severity: [critical]

  5. src/synix/build/runner.py: snapshot parent labels fallback logic is inconsistent
    Pattern: when building new artifacts, you set parent_labels = _get_parent_labels(...); if not parent_labels: parent_labels=[inp.label...]. Elsewhere _snapshot_parent_labels() also consults cache.
    Risk: parent_labels may differ between cached vs freshly built artifacts, making provenance non-deterministic across runs. That undermines the “full provenance” claim.
    Severity: [warning]

  6. src/synix/cli/search_commands.py: scratch realization cleanup is atexit only
    Risk: crash/kill -9 leaves .synix/work/scratch_search_* garbage forever. You partially addressed earlier “disk leak” but this isn’t robust.
    Severity: [minor]

  7. src/synix/build/fixers.py + validators.py: “duck-typed store” but behavior is now misleading
    Pattern: apply_fix says “persisted on next build run” but nothing records the fix intent; it’s effectively dropped. Validators’ _store_llm_trace is a no-op on SnapshotArtifactCache.
    Risk: synix fix becomes a trap (user thinks they fixed something; they didn’t).
    Severity: [warning]

  8. Docs vs website mismatch (synix.dev still says runtime/memory store, mentions build/, SearchIndex/FlatFile outputs, etc.)
    Risk: Public messaging contradicts new workflow; support burden + confusion.
    Severity: [warning]

Missing

  • Release locking tests/implementation (per-release lock file + contention behavior).
  • A clear definition of provenance chain ordering (path vs set vs BFS) and tests validating stable lineage output.
  • Semantic/hybrid search on released targets: an integration test that releases with embeddings enabled and then successfully runs synix search --mode semantic|hybrid.
  • Migration/back-compat tests: reading schema_version=1 objects after SCHEMA_VERSION=2 bump.
  • Website content update to match “build/release separation” (this PR updates README/docs but not the landing page text).

Verdict

Block — the architecture shift is fine in principle, but the missing locking, quadratic provenance walks, and likely-broken embeddings in released search DB are reliability landmines that will be expensive to unwind once users adopt the new workflow.

Review parameters
  • Model: gpt-5.2
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 32,006 total — truncated)
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-03-09T05:36:59Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 9, 2026

Note

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

Summary — This PR implements the "Projection Release v2" RFC: it removes the mutable build/ directory as a platform concept, makes .synix/ the single source of truth for all state, and introduces an explicit synix release lifecycle with adapter contracts, receipts, and release refs. Build now produces only immutable snapshots; projections are materialized on-demand via synix release HEAD --to <name>.

Alignment — This is deeply aligned with the project vision. DESIGN.md §1.5 explicitly called out the split between mutable build surface and immutable canonical history as intentional but incomplete, noting "projection/release state is still materialized into the local build surface until the explicit release/adapter layer lands." This PR lands that layer. Content-addressed objects, immutable snapshots, and provenance embedded in artifact objects (rather than a separate provenance.json) strengthen the core invariants. The adapter contract (plan/apply/verify) is clean and extensible — exactly what DESIGN.md's "architecture is a runtime concern" hypothesis requires. The ReleaseClosure pattern ensures adapters never touch the object store directly, preserving the content-addressed boundary.

Observations

  1. [positive] SnapshotArtifactCache with checkpoint recovery from interrupted builds is well-designed — it loads from the committed snapshot then overlays checkpoint data, giving crash-resilient cache semantics without complicating the happy path.

  2. [positive] The _walk_provenance BFS in release.py uses collections.deque with popleft() (O(1)), but SnapshotView.get_provenance and SnapshotArtifactCache.get_chain still use queue.pop(0) (O(n²)). The handoff doc notes this was fixed, but only release.py got the fix.

  3. [concern] SnapshotArtifactCache.get_chain and SnapshotView.get_provenance use list.pop(0) — O(n²) for deep provenance chains. At 10,000+ artifacts with deep DAGs this could be noticeable. The handoff doc says this was fixed with deque, but only _walk_provenance in release.py uses it.

  4. [concern] SnapshotArtifactCache.__init__ loads all artifact content into memory eagerly. For 10,000+ artifacts with large content, this could cause significant memory pressure. Consider lazy content loading.

  5. [concern] The apply_fix function in fixers.py now silently no-ops when the store is read-only (SnapshotArtifactCache). The docstring says "The fix will be persisted on the next build run" but there's no mechanism to ensure that — the fix content is simply lost. This is acknowledged as a known deferred item but could confuse users.

  6. [question] _save_plan_artifact in build_commands.py writes to refs/plans/latest — creating a full manifest+snapshot just for a plan artifact feels heavyweight. Is this needed, or could it write a simpler object?

  7. [positive] The adapter contract (ProjectionAdapter ABC with plan/apply/verify) is clean, well-documented, and follows the RFC precisely. The lazy adapter registry via importlib avoids circular imports elegantly.

  8. [positive] Atomic writes throughout — FlatFileAdapter uses tempfile + os.replace, SynixSearchAdapter uses a shadow database with os.replace. Consistent pattern.

  9. [nit] _ProvenanceShim is defined in both snapshot_view.py and search_commands.py with identical structure. Extract to a shared location.

  10. [positive] The migration guide (docs/migration-v2.md) is clear and complete, with a step-by-step flow and command comparison table. Good for a breaking change.

  11. [concern] The handoff doc mentions demo-01 CI failure due to Rich terminal width mismatch with a specific regex fix, but the diff already includes that fix in _normalize_output. Verify the golden files match CI output.

  12. [positive] Validators and fixers are removed from demo templates, correctly acknowledging they're experimental and incompatible with the read-only snapshot cache. Clean separation.

Verdict — This is a well-executed, architecturally significant change that resolves the documented split-brain between build/ and .synix/, completing a design that was explicitly flagged as incomplete. The memory-eager loading pattern is the main scalability concern worth addressing before 1.0.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 32,006 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-09T05:37:05Z

…uilds

SnapshotArtifactCache now scans .synix/objects/ for artifact objects not
covered by any committed snapshot or checkpoint. This recovers cache hits
from builds that were interrupted before commit_build_snapshot ran — the
exact scenario that caused the kimi build to recompute all 1111 already-
built episodes from scratch.

Tests verify: without the scan, 11 LLM calls are made; with it, 0.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 9, 2026

Note

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

Threat assessment — High risk: it’s a deep architectural rewrite (build/release split + new on-disk semantics) touching persistence, CLI UX, and provenance with some sharp edges still visible.

One-way doors

  1. Build/Release split becomes the user mental model + CLI contract (synix build + synix release)
    Why hard to reverse: once users script CI/CD around release receipts, named targets, and --release search, collapsing back to “build produces outputs” breaks workflows/docs/tutorials.
    Safe to merge when: you commit to this split as the v1 direction, and you have a stable compatibility story (deprecation timeline, explicit error messages when users run synix search without releases, migration docs already added but needs enforcement in code paths).

  2. .synix/ directory layout + ref namespace (refs/heads, refs/runs, refs/releases)
    Why hard to reverse: people will back up/copy .synix, write tooling around ref names, and rely on receipts. You’re effectively inventing a mini-git object model.
    Safe to merge when: schema/version policy is enforced (manifest schema v2, object schema v2) + forward/back compat tests exist; and concurrency/locking story is real (docs mention locks; implementation is inconsistent).

  3. Receipt JSON schema v1 (docs/receipt-schema.md + release_engine.py)
    Why hard to reverse: receipts will be parsed by automation. Any field semantic change becomes migration baggage.
    Safe to merge when: receipt schema is validated at write time and --json outputs are documented as versioned contracts (right now only docs claim this).

  4. Released search.db schema (new provenance_chains, release_metadata, citation_edges)
    Why hard to reverse: external consumers may treat the DB as portable runtime artifact.
    Safe to merge when: you clearly mark DB schema as internal OR you add schema_version table and compatibility guarantees.

Findings

  1. src/synix/build/snapshot_view.py: O(n²) provenance walks still present (queue.pop(0) in SnapshotView.get_provenance, SnapshotArtifactCache.get_chain)
    Risk/failure: large graphs (10k+ artifacts) will crawl; this is exactly the scale the tool claims to support.
    Severity: [warning]

  2. src/synix/build/snapshot_view.py: “recover orphaned artifacts by scanning object store” (_recover_orphaned_artifacts)
    Risk/failure: unbounded full scan of .synix/objects/ on startup; could be minutes+ and will get worse forever. Also risks surfacing artifacts not reachable from any snapshot (violates “snapshot is truth”).
    Severity: [critical]

  3. src/synix/build/runner.py: projection declarations computed from proj.sources even for SynixSearch(surface=...) (_record_snapshot_projections)
    Risk/failure: likely wrong closure. SynixSearch should derive inputs from the surface or explicit declaration, not “sources” (which may be empty or not match what the surface indexes). This can silently produce incomplete releases/search.
    Severity: [critical]

  4. src/synix/build/release_engine.py: no release locking
    Risk/failure: concurrent synix release can interleave writes to .pending.json, receipts, and output files; ref advancement is not protected. Docs/RFC mention locks but code doesn’t.
    Severity: [critical]

  5. src/synix/build/release_engine.py: partial materialization left behind (adapter Implement Synix v0.1 - declarative pipeline for AI conversation exports #1 can succeed, adapter Implement Synix v0.1: build system for agent memory #2 fails; pending preserved)
    Risk/failure: release dir contains mixed state without an atomic “current” pointer; users will read half-updated outputs. You explicitly call this “deferred” in handoff; that’s not acceptable if you’re making release the only way to get outputs.
    Severity: [warning]

  6. src/synix/search/adapter.py: embedding generation likely incorrect API usage (provider.embed_batch(texts) with only texts)
    Risk/failure: embeddings need stable IDs/labels and persistence format; this looks like it can’t map vectors back to artifacts. Also release verify doesn’t check embeddings at all.
    Severity: [warning]

  7. src/synix/build/object_store.py: schema bump to SCHEMA_VERSION=2 without clear migration path in code
    Risk/failure: older stores may become unreadable or mis-validated. You say “allow_older_schema” exists but I don’t see end-to-end migration/upgrade tooling here.
    Severity: [warning]

  8. src/synix/cli/search_commands.py: scratch realization cleanup is atexit only
    Risk/failure: crashes/kill -9 leaves .synix/work/scratch_search_* garbage. You already had disk leak concerns; this isn’t robust.
    Severity: [minor]

Missing

  • Concurrency tests for synix release (two releases in parallel; release vs clean; release vs build). The RFC explicitly mentions locks; code doesn’t.
  • A hard failure UX for synix search when no release exists (right now it sometimes falls back to legacy build/, sometimes auto-detects, sometimes “No search index found”).
  • Performance/regression tests for provenance walking and SnapshotArtifactCache init time on large object stores.
  • Validation that SynixSearch(surface=...) inputs are correct (unit test that release index contains exactly the surface closure).

Verdict

Block. The direction is plausible, but the orphan-scan behavior + missing release locking + likely-wrong projection input resolution are too risky to ship as the new core persistence/runtime model.

Review parameters
  • Model: gpt-5.2
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 32,157 total — truncated)
  • Prompt: .github/prompts/openai_review.md
  • Timestamp: 2026-03-09T05:49:27Z

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 9, 2026

Note

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

Summary

This PR implements the "Projection Release v2" RFC: it separates build from release absolutely, removes the build/ directory as a platform concept, makes .synix/ the single source of truth via content-addressed objects, and introduces an explicit synix release command with an adapter contract (plan/apply/verify), release receipts, and ref management. It also removes ArtifactStore, ProvenanceTracker, and .projection_cache.json, replacing them with SnapshotArtifactCache, SnapshotView, and ObjectStore.

Alignment

This is arguably the most architecturally significant change possible for the project. It directly advances DESIGN.md's core principles: artifacts become truly immutable and content-addressed (§3.3), provenance chains are embedded in artifact objects rather than a mutable sidecar file (§3.9 audit determinism), and the separation of build from release creates the clean foundation for the "release targets over canonical surface state" vision (§1.5). The adapter contract (plan/apply/verify) maps cleanly to the "adapters are not special" principle and opens the path for external database adapters without privileging synix_search. The manifest schema v2 with structured projection declarations makes diffing possible — a prerequisite for the branching story (§4.5).

Observations

  1. [positive] SnapshotArtifactCache._recover_orphaned_artifacts does a full scan of .synix/objects/ — this is a smart recovery mechanism for interrupted builds, but with 10,000+ conversations producing many objects, it could be slow. The method only runs when labels are missing, which mitigates this, but there's no upper bound on the scan. (snapshot_view.py)

  2. [concern] SnapshotView.get_provenance and SnapshotArtifactCache.get_chain both use queue.pop(0) — O(n²) list pops. The release.py _walk_provenance correctly uses deque.popleft(), but these two were missed. (snapshot_view.py, lines ~128, ~430)

  3. [concern] SnapshotArtifactCache.update_from_build updates _artifacts_by_label and _manifest_entries but does not update _parent_labels_map. The handoff doc acknowledges this ("stale provenance possible in validators"), but validators actively call store.get_parents(), so this could produce silent wrong answers in the same build run.

  4. [positive] The adapter contract is clean and minimal — plan/apply/verify with ReleaseClosure as the universal input. FlatFileAdapter does atomic write via tempfile + os.replace. SynixSearchAdapter builds a shadow DB then swaps. Both follow the pattern correctly.

  5. [question] _save_plan_artifact in build_commands.py writes to refs/plans/latest — good fix from the review finding about clobbering HEAD. But it creates a full snapshot/manifest just for a plan artifact. Is this the intended long-term pattern, or a stop-gap?

  6. [positive] The write_layer_checkpoint / clear_checkpoints mechanism provides build interruption recovery without complicating the snapshot model. Checkpoints are cleared after successful commit.

  7. [concern] apply_fix with SnapshotArtifactCache silently no-ops via getattr(store, "save_artifact", None). The handoff doc notes this, but the user-facing synix fix command will appear to succeed while doing nothing persistent. This should at minimum warn.

  8. [nit] The search provenance tree output changed shape (golden files show flat sibling lists instead of nested trees). This appears to be because ReleaseProvenanceProvider reconstructs parent_labels from the full chain rather than just direct parents, causing the tree to expand. Verify this matches user expectations.

  9. [positive] Demo templates had validators/fixers removed, which is the right call given those are experimental and this PR focuses on the build/release boundary. The handoff doc is thorough.

  10. [concern] No tests are included in the diff for test_release_flow.py, test_release_engine.py, etc., though the handoff doc claims they exist. If they're in commits not shown, fine — but the diff doesn't demonstrate coverage of the release atomicity path (adapter failure mid-release).

Verdict

This is a strong architectural step that resolves the split-brain problem between build/ and .synix/, directly advances the immutable-artifact and provenance principles from DESIGN.md, and establishes a clean extension model for adapters — well worth merging after fixing the O(n²) BFS pops and the silent no-op in apply_fix.

Review parameters
  • Model: claude-opus-4-6
  • Context: README.md, DESIGN.md, synix.dev, PR diff
  • Diff size: 10,000 lines (of 32,157 total — truncated)
  • Prompt: .github/prompts/claude_review.md
  • Timestamp: 2026-03-09T05:49:31Z

@marklubin marklubin merged commit 62b40a6 into main Mar 9, 2026
12 checks passed
@marklubin marklubin deleted the mark/projection-release-v2 branch March 9, 2026 06:11
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.

Provenance lineage summarization options Content-addressed artifact cache: reuse across pipelines and build dirs

1 participant