feat(snapshots): record immutable run snapshots

[marklubin]

Summary

• add the first immutable snapshotting slice behind a new .synix object/ref store
• record snapshot and manifest objects after each successful build while keeping the existing build/ layout intact
• expose snapshot metadata in build output and add \
• add unit, integration, and e2e coverage for object store, refs, snapshot commits, and CLI history

Type of Change

• New feature
• Docs
• Tests

Validation

• Ran tests locally
• Added/updated tests where needed
• Updated docs (if applicable)

Related Issues

• Refs First-class build snapshots + artifact diff #34

Notes

• This is intentionally additive: mutable build outputs remain the active read/write path for now.
• Release/revert semantics are deferred to the next slice.
• The branch already includes the snapshot/release RFC commit and the first implementation commit.

[github-actions]

Note

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

Threat assessment — High risk: this PR silently introduces a new persistent storage model (.synix snapshots/refs) and begins wiring it into the build path, but the implementation is incomplete, leaky, and makes several hard-to-reverse choices.

One-way doors

1. On-disk .synix/objects + ref namespace shape (refs/heads/main, refs/runs/<run_id>, HEAD format)
   Hard to reverse because users/scripts will key off these paths and semantics; future migrations will be painful.
   Safe only if: you commit to a versioned storage format (schema/version header), document compatibility guarantees, and provide migration tooling or an explicit “format may change” gate with opt-in flag.

2. OID computation + JSON canonicalization policy (ensure_ascii=True, sort_keys, separators)
   Hard to reverse because OIDs become the identity of stored objects; any change breaks dedup and history lookup.
   Safe only if: you specify the canonicalization in docs as a stable contract (including encoding rules) and add tests pinning exact bytes-to-oid behavior.

3. Run ref naming coupled to run_id timestamp string
   Hard to reverse because ref names become part of history and UX (synix runs list).
   Safe only if: you guarantee uniqueness/collision behavior across processes and define whether run_id is a ref-safe identifier vs a display string.

Findings

1. src/synix/build/snapshots.py: snapshotting uses ArtifactStore(build_path) and ProvenanceTracker(build_path) reading mutable build/
   Failure mode: snapshot is not a first-class immutable build output; it’s a scrape of whatever happens to be in build/ at commit time. Any partial build, stale files, or concurrent build can poison snapshots.
   Severity: [critical]

2. commit_build_snapshot(...) advances refs unconditionally, even if build/validation failed upstream
   In runner.run(), snapshot commit happens after validators but without checking overall success. If transforms failed but left partial outputs, you still write a snapshot and move HEAD.
   Severity: [critical]

3. synix_dir_for_build_dir(build_dir): .synix is computed as build_dir.parent/.synix
   This bakes in a project layout assumption and breaks if build_dir is not ./build directly under project root (custom paths, nested builds, multiple pipelines). Also makes two pipelines in same parent share a store (corruption).
   Severity: [critical]

4. ObjectStore.put_json(): writes JSON via atomic_write(path, encoded.decode("utf-8")) but put_bytes() uses bespoke _atomic_write_bytes
   Inconsistent atomicity semantics and likely inconsistent fsync behavior (depends on atomic_write implementation). Also pointless encode/decode churn.
   Severity: [warning]

5. ObjectStore.get_json(): read_text() default encoding
   Platform-default encoding risk; your writer uses UTF-8. You should read with encoding="utf-8".
   Severity: [minor]

6. commit_build_snapshot: artifact content stored as JSON “blob” with base64 of UTF-8 string
   This inflates storage, loses original bytes if artifacts are not UTF-8 text (or contain invalid sequences), and double-wraps JSON inside JSON. Also contradicts the RFC “blob object is raw bytes”; here blob is JSON that base64-embeds bytes.
   Severity: [warning]

7. Projection capture is incorrect and overbroad: input_oids set to all artifact oids for every projection
   Every projection claims to depend on everything, destroying diffability and incremental release planning. You already have projection.sources; use those labels to select the correct closure.
   Severity: [critical]

8. FlatFile projection snapshot reads from projection.output_path (may be outside build dir)
   You’re pulling arbitrary filesystem paths into the snapshot store. That’s a security footgun and a reproducibility bug (path may be user home, shared, etc.).
   Severity: [warning]

9. No object/schema validation (ObjectStore accepts any dict; RefStore accepts any ref string)
   Corruption will be silent and permanent. You need type/version fields validated and ref-name normalization (no .., no absolute paths, etc.).
   Severity: [warning]

10. Concurrency/race: refs updated without locking (RefStore.write_ref)
    Two builds can interleave and clobber HEAD / run refs. Atomic write of file contents doesn’t solve multi-writer sequencing.
    Severity: [warning]

11. CLI/API mismatch: README documents synix build, diff adds snapshot printing to build_commands.build, but e2e calls main ["run", ...]
    Either “run” is an alias or tests are drifting from docs. This is user-facing confusion.
    Severity: [minor]

Missing

• A feature flag / opt-in for snapshotting; right now it’s always-on and writes new persistent state.
• Failure-mode tests explicitly listed in the RFC (failed build doesn’t advance ref, etc.). None added.
• Any docs updates (README/DESIGN/Entity model) reflecting .synix, refs, runs, snapshot semantics.
• Tests for projection dependency closure correctness and for multiple pipelines/build dirs not sharing .synix.
• Ref name sanitization + schema versioning.

Verdict

Block — you’re introducing a durable storage/identity layer and moving HEAD based on mutable build scraping without correctness, isolation, or failure semantics; this will create irreversible corrupted history and a de facto on-disk API you can’t safely change later.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 1,708 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T07:20:49Z

[github-actions]

Note

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

Summary

This PR introduces a git-like immutable snapshot model for Synix builds: a content-addressed object store under .synix/objects, symbolic refs with HEAD, manifests as closures over artifacts and projections, and snapshot objects linking builds into a chain. Every successful build now commits a snapshot, advances refs, and is inspectable via a new synix runs list command. The PR includes an RFC document, implementation of the core primitives, integration into the build runner, and tests at unit/integration/e2e levels.

Alignment

This is strongly aligned with DESIGN.md's core principles. Artifacts become immutable and content-addressed in the object store (§3.1, §3.3). Manifests capture the full closure — consistent with "cache keys capture all inputs." The parent-snapshot chain preserves the provenance DAG across builds (§3.9 audit determinism). The ref model lays groundwork for branch builds (§4.5), which DESIGN.md identifies as the "sharpest differentiator." Separating build from release respects the hypothesis that architecture is a runtime concern — you can build experimental variations without committing to a deployment.

Observations

1. [concern] object_store.py uses two different atomic write paths: _atomic_write_bytes for put_bytes and the imported atomic_write (string-based) for put_json. The put_bytes path does manual fd/os.write; put_json decodes back to string to use the other helper. This inconsistency risks subtle encoding bugs. Consolidate on one atomic write primitive that accepts bytes.

2. [concern] snapshots.py:108 stores the entire search.db SQLite binary as a base64-encoded JSON blob object. For a database that could be tens of megabytes at 10k+ conversations, this will blow up memory and storage. The RFC acknowledges full-rebuild release for v1, but the base64 encoding inside a JSON wrapper roughly doubles the size. Consider storing projection state as raw bytes via put_bytes instead.

3. [concern] snapshots.py:113 sets input_oids for projections to all artifact OIDs regardless of which layers the projection actually sources from. This breaks the provenance contract — a SearchIndex sourcing [episodes, monthly, core] would incorrectly claim dependency on every artifact. Should filter by projection.sources.

4. [question] runner.py calls commit_build_snapshot unconditionally after the build, including after failed/partial builds (the call is outside the if pipeline.validators: block but appears to run regardless of build errors). The RFC explicitly states "failed build does not advance the build ref." Is there error-guarding I'm not seeing upstream?

5. [question] logging.py changes run_id format to microsecond precision (%f). This fixes the collision issue but changes the format of an identifier that may appear in existing run logs, batch-build state, or mesh state. Is this a breaking change for any persisted state?

6. [positive] The RFC is exceptionally thorough — it defines terminology, object schemas, lifecycle phases, adapter contracts, and a complete test strategy before implementation. This is exactly how infrastructure primitives should be introduced in a pre-1.0 project.

7. [positive] Test coverage is solid: unit tests for ObjectStore and RefStore, integration tests covering successive builds and source changes, and an e2e test exercising the full CLI flow with snapshot verification. The parent-snapshot linkage assertion in test_successive_builds_preserve_old_run_ref is particularly good.

8. [nit] snapshots.py accesses store._manifest (private attribute) to iterate artifact labels. This couples the snapshot layer to ArtifactStore internals. Add a public iteration method.

9. [nit] refs.py:read_ref has no cycle detection for symbolic ref chains. A ref: ref: ... loop would cause infinite recursion. Low probability but easy to guard with a depth limit.

10. [nit] The RFC document is 867 lines. Consider splitting the design rationale from the implementation specification — future contributors will want the spec without re-reading the full motivation.

Verdict

This is a well-designed foundational change that directly enables the branching, rollback, and architecture-migration capabilities that define Synix's differentiation — the projection input_oid accuracy issue (#3) and the unconditional snapshot commit (#4) should be fixed before merge.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 1,708 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T07:20:55Z

[github-actions]

Note

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

Threat assessment — High risk: it quietly introduces a new persistence model (.synix snapshots/refs) that will become user-visible and hard to unwind, but the implementation is partial, leaky, and not failure-safe.

One-way doors

1. .synix on-disk object store + refs layout (objects/, refs/, HEAD)
   Hard to reverse because users/scripts/other tooling will start to rely on the directory structure and ref semantics.
   Safe only if: schema/versioning is defined, migration/compat story is explicit, and corruption/partial-write behavior is nailed down.

2. Run ref naming = refs/runs/{run_id} and run_id format change to microseconds
   Hard to reverse because run IDs appear in receipts/logs and are now part of ref paths; changing later breaks automation.
   Safe only if: you commit to a stable run-id format (or introduce an abstraction layer/aliasing) and document it.

3. Snapshot object schema baked into content-addressed JSON (type, manifest_oid, parent_snapshot_oids, etc.)
   Hard to reverse because once written, old objects must remain readable forever or you need a migration layer.
   Safe only if: object schema versioning exists (e.g., schema_version), plus strict validation and forward-compat plan.

Findings

1. src/synix/build/snapshots.py: snapshot commit runs even after failed builds
   runner.run() unconditionally calls commit_build_snapshot(...) after validators, with no guard on build success. If transforms/projections failed or produced partial outputs, you still advance HEAD and record an “immutable” snapshot of garbage.
   Severity: [critical]

2. src/synix/build/snapshots.py: projection capture is wrong / overbroad input set
   For projections, input_oids is [artifact_oids[label] for label in artifact_oids] i.e., all artifacts, ignoring the projection’s configured sources. That breaks provenance/diff semantics and will explode object sizes as pipelines grow.
   Severity: [critical]

3. src/synix/build/snapshots.py: FlatFile projection reads from projection.output_path (outside build dir)
   You’re snapshotting arbitrary filesystem paths (could be outside repo, could be huge, could be secrets) and treating it as build output. This violates isolation expectations and makes snapshots non-reproducible and unsafe.
   Severity: [critical]

4. src/synix/build/refs.py: synix_dir_for_build_dir derives .synix from build_dir.parent
   This assumes build_dir is always ./build under project root. If users set build_dir elsewhere (docs show it’s configurable), you’ll write .synix next to that directory, not the project. That’s surprising and will create multiple stores.
   Severity: [warning]

5. src/synix/build/object_store.py: inconsistent atomic write + dead code
   You define _atomic_write_bytes but put_json uses atomic_write (string-based) and put_bytes uses _atomic_write_bytes. Mixed implementations increase risk of partial/corrupt writes, plus unnecessary complexity.
   Severity: [warning]

6. src/synix/build/object_store.py: “blob” JSON wrapper base64 inflates storage and is awkward
   You store bytes as JSON with base64 inside, then hash the JSON. That’s space-inefficient and makes later tooling painful. A blob should be raw bytes addressed by hash; JSON should reference it.
   Severity: [warning]

7. src/synix/build/snapshots.py: uses private store._manifest
   Tight coupling to ArtifactStore internals. Any refactor breaks snapshots. Also you’re iterating labels from a mutable manifest file—if that file is inconsistent with on-disk artifacts, snapshot will silently skip.
   Severity: [warning]

8. src/synix/build/refs.py: iter_refs reads raw values, may include symbolic refs but returns un-resolved
   Docstring says “direct oid values” but you don’t filter/resolve ref:. This will confuse callers and UIs.
   Severity: [minor]

Missing

• Failure-mode tests: build failure must not advance refs; partial projection build must not be snapshotted; object store corruption handling.
• Schema/versioning: no schema_version, no validation on read/write of objects (type checks, required fields).
• Docs updates: README/CLI docs still talk in terms of mutable build/ outputs; no mention of .synix, refs, runs list.
• Concurrency story: no locking around .synix writes; concurrent builds can race (refs, objects, HEAD).
• Boundaries: no size limits/streaming for reading search.db and flat files into memory before base64 encoding.

Verdict

Block. The idea is plausible, but the current implementation can record invalid snapshots, captures the wrong projection inputs, and can snapshot arbitrary files outside the build—those are correctness and security footguns that will be extremely costly once shipped.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 1,708 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T07:22:52Z

[github-actions]

Note

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

Summary

This PR introduces an RFC document for immutable snapshots, refs, and projection release, along with a working first implementation of the object store, ref management, snapshot creation on build, a synix runs list CLI command, and supporting tests at all three levels (unit, integration, e2e).

Alignment

Strong fit with the project vision. DESIGN.md explicitly models artifacts as immutable and content-addressed (§3.3 materialization keys, §3.9 audit determinism). This PR materializes that into a git-like object store where every build produces an immutable snapshot — a natural extension of "artifacts are build outputs with full provenance." The manifest-as-closure concept maps cleanly to the DESIGN.md notion that cache keys capture all inputs. The RFC's separation of build and release echoes the DESIGN.md separation of "define" from "execute" (§4.4.1 sync() vs run()). The branching model in the RFC (refs as named pointers to snapshots) provides the substrate for the branch experiments described in §4.5 without implementing full branching UX yet — appropriate scoping for pre-1.0.

Observations

1. [concern] object_store.py has two write paths: put_bytes uses _atomic_write_bytes (raw os.write), while put_json uses the imported atomic_write from synix.core.errors (which takes a string). The inconsistency is confusing and could mask bugs — put_json round-trips through encode/decode unnecessarily. Pick one atomic write strategy.

2. [concern] snapshots.py:commit_build_snapshot stores the entire search.db binary as a base64-encoded JSON blob object. For large search indexes this will be very expensive — a 50MB SQLite file becomes ~67MB of base64 inside a JSON string, all held in memory. This is acknowledged as "full rebuild" for v1, but it's an O(n) memory bomb that should at minimum have a size guard or a comment flagging the known limitation.

3. [concern] commit_build_snapshot accesses store._manifest (private attribute). This couples the snapshot layer to ArtifactStore internals. A public iteration method would be safer.

4. [question] The _pipeline_fingerprint function in snapshots.py captures layer.config and layer.depends_on, but doesn't include pipeline.llm_config version/hash or prompt source hashes — the components that DESIGN.md §3.3 says must be part of the cache key. Is this intentional given that the fingerprint here is for manifest-level identity rather than per-artifact caching?

5. [concern] refs.py:read_ref follows symbolic refs recursively with no cycle detection or depth limit. A corrupted ref file pointing to itself would infinite-loop. Add a max-depth guard.

6. [positive] The run ID collision fix in logging.py (microsecond precision) is a good proactive fix that prevents ref collisions on fast successive builds — exactly the kind of thing snapshot semantics would expose.

7. [positive] Test coverage is thorough: unit tests for object store canonicalization and ref resolution, integration tests for successive builds with parent linkage, and a full e2e test through the CLI runner. The e2e test validates both build output and runs list output, covering the complete user-facing path.

8. [nit] The RFC document at 867 lines is substantial. It might benefit from being split — the RFC proper vs. the test strategy appendix — to make review easier, but this is a style preference.

9. [question] The RFC mentions synix release and synix revert commands but the implementation doesn't include them. The RFC scope section is clear about this, but the CLI currently has no way to use the snapshots beyond listing them. Is there a follow-up issue tracked?

10. [positive] The snapshot is written after validation completes but unconditionally (even if validation fails). This matches the "audit determinism" principle — record what happened, even if it's bad. However, the RFC's test strategy says "failed build does not advance the build ref," which isn't yet implemented.

Verdict

This is a well-scoped foundational PR that introduces the right abstraction (immutable snapshots with content-addressed storage) at the right layer, with good test coverage and a clear RFC roadmap — the recursive ref resolution and binary-blob-as-JSON patterns need attention before this scales.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 1,708 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T07:23:02Z

[github-actions]

Note

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

Threat assessment — Medium-high risk: this PR quietly introduces a durable storage + identity model (.synix object store + refs) that will be extremely painful to change once users have real data, but the implementation is still leaky/incomplete.

One-way doors

1. .synix on-disk layout + ref namespace (HEAD, refs/..., objects/aa/<oid>)

   • Why hard to reverse: Users will accumulate history; tooling/scripts will assume paths and semantics. Migrating object stores is non-trivial.
   • Safe to merge when: You commit to backward-compat migration tooling/versioning (schema migration plan + compatibility reader) and document the stability level explicitly.

2. Object schema contracts + SCHEMA_VERSION=1 with strict required fields

   • Why hard to reverse: Stored objects become long-lived. Tight validation without a migration story will brick old repos.
   • Safe to merge when: You add forward/backward compatibility rules (reader tolerates unknown fields; schema bump + migrator design) and tests for reading older versions.

3. Run ref format refs/runs/{run_id} and run_id timestamp shape

   • Why hard to reverse: Anything consuming run history will depend on ref naming and sorting.
   • Safe to merge when: You guarantee uniqueness + ordering semantics and define whether run_id is sortable / stable across machines.

4. Pipeline fingerprint definition in snapshots._pipeline_fingerprint()

   • Why hard to reverse: This becomes the basis for diffing/understanding snapshot identity. Current fingerprint includes absolute dirs and config dict ordering assumptions.
   • Safe to merge when: Fingerprint is canonicalized and scoped to “meaningful build inputs” (exclude machine-local paths unless intentionally part of identity).

Findings

1. src/synix/build/object_store.py — “blob object” is not actually a blob

   • Stores raw bytes at objects/<sha256(bytes)>, then creates a JSON “blob object” that points to content_oid, returning the JSON object oid. This double-indirection is undocumented and wastes objects; worse, naming is misleading.
   • Severity: [warning]

2. src/synix/build/snapshots.py:_store_blob_bytes — stores artifact content by reading full text into memory

   • artifact.content.encode("utf-8") will scale poorly for large artifacts (core memories, indices). You already implemented streaming for files but not for strings.
   • Severity: [warning]

3. src/synix/build/snapshots.py:commit_build_snapshot — snapshotting depends on mutable build outputs

   • You’re “committing” by re-reading build/ outputs (search.db, flat files, artifact store). This is not immutable-by-construction; it’s “copy whatever is currently there”, susceptible to partial builds, concurrent builds, or external edits.
   • Severity: [critical]

4. src/synix/build/refs.py — ref writes are “atomic” per-file but not transactionally consistent

   • You write refs/runs/<run_id> and then refs/heads/main. Crash between writes yields a run ref without HEAD advance (or vice versa). Same problem for future release refs.
   • Severity: [warning]

5. src/synix/build/refs.py:synix_dir_for_build_dir — conflicting precedence vs load_pipeline

   • load_pipeline forces pipeline.synix_dir = <pipeline.py dir>/.synix when unset, but synix_dir_for_build_dir prefers build/.synix unless legacy exists. This is a recipe for multiple stores per project depending on entrypoint.
   • Severity: [critical]

6. src/synix/build/snapshots.py:_pipeline_fingerprint — includes build_dir/source_dir

   • Makes snapshots non-portable and breaks “same pipeline definition on another machine” comparisons. Also includes raw llm_config dict which may contain secrets.
   • Severity: [warning]

7. src/synix/core/logging.py — run_id includes microseconds but still not collision-proof

   • Two processes can still collide (same clock resolution, container time quirks). Also run ref path now contains long strings; no validation in refs.py for arbitrary run_id (it validates ref name, but run_id is embedded).
   • Severity: [minor]

8. src/synix/build/object_store.py:_validate_object_payload — strict required fields but no type validation

   • You validate presence, not types/shape. Corrupt objects will pass if keys exist. Later code will blow up unpredictably.
   • Severity: [warning]

Missing

• Docs updates: README/CLI reference now has synix runs list but README doesn’t mention it; also no mention of .synix store behavior.
• Concurrency/locking: no file lock around snapshot commit; race conditions if two builds write same .synix.
• Transaction semantics: no “build succeeded => snapshot committed” atomic boundary. Need either lock + temp namespace + finalize, or ref updates in a single commit record.
• Migration/compat story: what happens to existing build/manifest.json, provenance.json, search.db users? No reader compatibility layer beyond a weak legacy .synix path check.
• Tests for partial failure: crash between writing objects and moving refs; corrupt HEAD contents; missing parent snapshot object.

Verdict

Block — you’re introducing a storage/ref substrate (a one-way door) but the current .synix location semantics and non-transactional snapshot commit make data loss / split-brain stores likely; fix the .synix resolution rule + add locking/transactionality before this can ship.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,068 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T07:39:09Z

[github-actions]

Note

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

Summary

This PR introduces the foundation of an immutable snapshot model for Synix: a content-addressed object store (.synix/objects), git-like refs with symbolic HEAD, manifest/snapshot objects written after each successful build, and a synix runs list CLI command. It also includes a detailed RFC document specifying the full vision (release, revert, projection adapters) beyond what this first implementation covers.

Alignment

Strong fit. DESIGN.md establishes that artifacts are content-addressed (§3.3 materialization keys, SHA-256 fingerprints), provenance chains must be complete (§3.9 audit determinism), and the system is modeled on build-system lineage (Bazel, dbt, lakeFS). This PR makes immutability real at the storage layer rather than just the cache layer. The RFC's separation of build from release echoes DESIGN.md's separation of plan() from run(). The snapshot-as-commit model directly enables the branching vision in §4.5 and the "architecture migration without data loss" pitch from §1.3.

Observations

1. [positive] object_store.py — Atomic writes with fsync, canonical JSON serialization for stable OIDs, schema version validation with per-type required fields. This is careful foundational work.

2. [positive] refs.py — Cycle detection with MAX_REF_DEPTH, regex validation on ref names and OIDs, symbolic HEAD resolution. The ref model is minimal but correct for this stage.

3. [concern] snapshots.py:commit_build_snapshot — The _atomic_write_bytes helper in object_store.py correctly uses os.close(fd) only once, but there's a subtle issue: if os.fsync(fd) raises, the exception handler calls os.close(fd) which is correct. However if os.write(fd, data) does a partial write (possible for large data), there's no write loop. For the JSON payloads this is fine, but put_bytes is a general interface.

4. [concern] snapshots.py reads the entire artifact content into memory (artifact.content.encode("utf-8")) for every artifact in the manifest. With 10,000+ conversations this could spike memory. The put_file streaming path exists but isn't used for artifact content.

5. [question] refs.py:synix_dir_for_build_dir — The fallback logic checks nested then legacy then defaults to nested. But pipeline.py:load_pipeline unconditionally sets synix_dir to filepath.parent / ".synix" (sibling to pipeline.py, not inside build). These two defaults could diverge if the pipeline file isn't in the project root. Is that intentional?

6. [concern] runner.py — The snapshot commit condition is if result.validation is None or result.validation.passed. But result.validation is only set when validate=True is passed to run(). For non-validating builds, validation is None → snapshot always commits. That's correct, but the conditional reads ambiguously — a comment would help.

7. [positive] test_validation_failure_does_not_advance_snapshot_refs — Directly tests the "failed build must not advance refs" invariant from the RFC's must-have failure tests. Good discipline.

8. [nit] logging.py run_id format changed to microsecond precision (%f). Good fix for collision avoidance, but this changes the run ref namespace format (refs/runs/...). Existing builds with old-format run refs won't match the new pattern — worth a migration note.

9. [question] The RFC is 867 lines and covers release, revert, adapters, notebooks, and streaming — but only ~30% is implemented here. Should the RFC live in docs/ permanently, or will it be superseded by actual docs? Risk of RFC diverging from implementation over time.

10. [positive] Test coverage is thorough: unit tests for object store canonicalization and ref cycles, integration tests for successive builds/parent linkage/source changes/validation failures/path escapes, and an e2e test exercising the full CLI flow with runs list. The RFC's test plan maps well to what's delivered.

11. [nit] artifacts.py:iter_entries returns dict[str, dict] but the docstring says "shallow copy." Since the inner dicts aren't copied, mutations to returned entries would affect the store. Consider deepcopy or documenting the contract.

Verdict

This is a well-structured first cut of a foundational capability that directly advances the project's core vision of immutable, content-addressed build artifacts with full provenance — a good incremental step.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,068 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T07:39:23Z

[github-actions]

Note

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

Threat assessment — Medium-high risk: this introduces a new on-disk persistence model (.synix snapshots/refs) that will be very hard to change later, and the implementation has several correctness and portability holes.

One-way doors

1. .synix on-disk object/ref format + SCHEMA_VERSION=1

   • Why hard to reverse: Once users have history in .synix/objects and HEAD/refs/*, any schema/layout change becomes a migration/compatibility promise. Your README already claims “compatibility path”.
   • Safe to merge when: you have (a) explicit version negotiation/upgrade plan, (b) corruption detection + repair story, (c) documented guarantees (what fields are stable, what may change), and (d) roundtrip tests across at least one “future schema version” fixture (even if synthetic).

2. Default placement rule: sibling <build parent>/.synix chosen implicitly

   • Why hard to reverse: Users will discover .synix “magically” appearing next to their build dir and will script around it; changing it later breaks tooling and expectations.
   • Safe to merge when: there’s a single authoritative rule documented in one place (CLI + docs), and CLI exposes --synix-dir consistently across commands that read/write snapshots.

3. CLI surface: synix runs list + “immutable build snapshots” language

   • Why hard to reverse: This becomes user-facing and will be relied upon for audit/history. If semantics change (what counts as a run, retention, ordering), you break workflows.
   • Safe to merge when: command supports targeting pipeline explicitly, filtering, stable sort order, and docs describe retention/GC (even “none yet”) and what “immutable” means under concurrent builds.

Findings

1. src/synix/build/object_store.py — _REQUIRED_FIELDS vs payloads mismatch

   • Failure mode: _REQUIRED_FIELDS["artifact"] omits fields you actually write (prompt_id, model_config, created_at, parent_labels), but that’s fine (extra fields allowed). The real issue: _REQUIRED_FIELDS["blob"] requires content_oid and size_bytes, but your RFC blob schema suggests encoding/bytes. This is already divergent and will confuse future compatibility/migrations.
   • Severity: [warning]

2. src/synix/build/object_store.py — non-atomic put_text placement

   • Pattern: temp file created in self.objects_dir then os.replace(tmp, path) where path is in objects/<prefix>/....
   • Failure mode: os.replace across directories can fail on some platforms/filesystems; also you don’t ensure the target directory exists until after hashing. You do create it, but the cross-dir rename is still risky.
   • Severity: [critical]

3. src/synix/build/snapshots.py — provenance/closure not actually “closure”

   • Pattern: projection input_oids is built from projection.sources (layers) via layer_artifact_oids[entry["layer"]].
   • Failure mode: This assumes entry["layer"] names match Layer.name and that “layer” is the correct attribution. If artifact store’s “layer” is a build level or class grouping, you’ll silently produce wrong projection closures. Also: manifest “closure” ignores build-time files like provenance.json, .projection_cache.json, etc. Yet README/RFC frame this as reproducibility substrate.
   • Severity: [warning]

4. src/synix/build/snapshots.py — storing artifact content as “blob object” wrapping “content_oid”

   • Pattern: _store_blob_text returns an OID of a JSON blob object, not the raw bytes oid; then you store that as content_oid in artifact object.
   • Failure mode: field naming is misleading: content_oid now points to a blob object, not content bytes. This will bite you when you add streaming/remote stores or want direct content addressing/dedup semantics.
   • Severity: [warning]

5. src/synix/build/snapshots.py — lock is POSIX-only and effectively a no-op on Windows

   • Pattern: fcntl optional; otherwise no locking at all.
   • Failure mode: concurrent builds/releases on Windows (or environments without fcntl) will corrupt refs/objects (torn writes, races). Even on POSIX, locking .lock doesn’t guarantee atomic multi-ref updates.
   • Severity: [critical]

6. src/synix/build/refs.py — synix_dir_for_build_dir legacy/nested precedence is underspecified

   • Pattern: prefers existing <build parent>/.synix, else <build>/.synix, else <build parent>/.synix.
   • Failure mode: If both exist, you silently pick sibling. If user previously used nested store, you might “fork” history unexpectedly. Also naming “legacy” is backwards (sibling is new per README).
   • Severity: [warning]

7. src/synix/build/runner.py — snapshot commit depends on mutable build/ outputs

   • Failure mode: You’ve introduced “immutable snapshot history” but you still build into mutable build/ and then copy into .synix. Any partial/corrupt build outputs (crash after writing search.db but before artifacts manifest update) can be snapshotted unless you have an integrity check.
   • Severity: [warning]

8. CLI/doc mismatch: README says uvx synix runs list; tests call main ["run", ...]

   • Failure mode: unclear whether the command is build or run in current UX; you’re adding more surface without reconciling naming.
   • Severity: [minor]

Missing

• GC/retention strategy: .synix/objects will grow unbounded; no pruning, no “keep last N”, no reachability walk from refs.
• Ref update atomicity: you update run ref then head ref; there’s no transaction/rollback if second write fails.
• Windows-safe locking: either a cross-platform lock or explicit “unsupported” behavior.
• Docs updates beyond README: entity model / cache semantics docs should mention .synix now being canonical for history.
• Boundary validation: ensure run_id is safe as a path component (refs/runs/{run_id}) and length-bounded.

Verdict

Ship with fixes — the model is directionally right, but the current object store write-path and concurrency story are not safe enough to land as the foundation for “immutable snapshots.”

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,298 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T07:51:17Z

[github-actions]

Note

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

Summary

This PR introduces a git-like immutable snapshot model for Synix builds: a content-addressed object store (.synix/objects/), symbolic refs and HEAD, manifest/snapshot objects written after each successful build, and a synix runs list CLI command. It also includes an RFC document describing the full long-term vision (release, revert, projection adapters).

Alignment

Strong fit. DESIGN.md §3.3 defines materialization keys as content-addressed cache keys, §3.9 commits to audit determinism, and the overall vision treats artifacts as immutable build outputs with full provenance. This PR extends that to the build-level: every successful build becomes an immutable snapshot with a content-addressed manifest, which directly supports the "architecture evolution without data loss" thesis. The separation of .synix/ from build/ means synix clean preserves history — consistent with the "experiment safely" promise. The RFC's projection/release separation respects DESIGN.md's distinction between artifacts and queryable surfaces (§6.1).

Observations

1. [positive] object_store.py — Schema versioning (SCHEMA_VERSION = 1) with validation on both write and read is forward-looking for a pre-1.0 project. Canonical JSON serialization ensures stable oids across key ordering.

2. [positive] refs.py — Cycle detection with MAX_REF_DEPTH and seen tracking prevents infinite loops. Good defensive coding.

3. [concern] snapshots.py:_snapshot_lock — On Windows (fcntl unavailable), the lock degenerates to no-op. The comment doesn't acknowledge this. Concurrent builds on Windows could corrupt refs. At minimum, document the limitation or use msvcrt.locking as a fallback.

4. [concern] snapshots.py:commit_build_snapshot — The two write_ref calls (run ref then HEAD) are not atomic. A crash between them leaves HEAD pointing to the old snapshot while the run ref exists. This contradicts the RFC's own "failed build does not advance the build ref" goal. Consider writing both under a transaction or at least documenting the window.

5. [question] object_store.py:put_text — The incremental encoder approach avoids doubling memory for large texts, which is good. But the temp file is created in self.objects_dir root rather than in the target shard directory. On cross-device setups, os.replace could fail since source and destination must be on the same filesystem. put_bytes and put_file correctly use path.parent for the temp file.

6. [concern] runner.py — Snapshot commit happens after projections have already been materialized (search.db written, flat files generated). If commit_build_snapshot fails, the mutable build dir has advanced but no snapshot exists. The RFC says build and release should be separate phases — this conflation may cause issues.

7. [nit] logging.py — Changing run_id format from %Y%m%dT%H%M%SZ to %Y%m%dT%H%M%S%fZ (microseconds) is sensible but is a breaking change for any external tooling parsing run IDs. The README note about .synix format changes doesn't cover this.

8. [positive] Test coverage is thorough: unit tests for object store and refs, integration tests covering successive builds, source changes, validation failure blocking snapshots, and FlatFile outside build dir. The e2e test exercises the full CLI path including runs list.

9. [question] artifacts.py:iter_entries returns dict(self._manifest) — a shallow copy. If entries contain nested mutable dicts (they do — each entry is a dict), callers could mutate shared state. snapshots.py only reads, so this is safe today but fragile.

10. [nit] runs_commands.py checks build_path.exists() but the .synix dir may exist independently (the whole point of the separation). Should check for the synix dir instead or both.

11. [positive] The RFC is unusually well-structured for a design doc — it separates build vs. release, defines adapter contracts, and explicitly scopes what's out of the first implementation. The test strategy section in the RFC maps directly to delivered tests.

Verdict

This is a well-executed foundational step that correctly separates immutable history from mutable build state, with strong test coverage and clear alignment to the project's core principles — the Windows locking gap and non-atomic ref writes should be addressed before merge.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,298 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T07:51:27Z

[github-actions]

Note

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

Threat assessment — Medium-high risk: introduces a new persistence model (.synix snapshots/refs) that will quickly become load-bearing, but the implementation is only partially aligned with the RFC and has correctness gaps.

One-way doors

1. .synix on-disk object/ref format + schema_version=1
   Hard to reverse because users will accumulate history under .synix and tooling/automation will depend on ref names and object schemas. “May evolve” isn’t enough.
   Safe to merge only if: you define explicit forward-compat guarantees (migration tooling or dual-read), document stability boundaries (which object types/fields are stable vs experimental), and add versioned readers (accept older schema versions).

2. Ref namespace + run ref naming (refs/runs/{run_id} and refs/heads/main)
   Once people start scripting synix runs list / parsing run refs, you’re stuck.
   Safe only if: run_id format and ref layout are declared stable (or clearly marked experimental with a CLI output that’s parse-stable, e.g., --json).

3. Making build/ “not source of truth” while keeping commands defaulting to it
   Mental model lock-in + confusing split brain (mutable build dir vs immutable snapshots).
   Safe only if: commands that read artifacts/search can target a ref/snapshot consistently (at least internally), and clean/deletion semantics are documented and tested.

Findings

1. src/synix/build/object_store.py — _REQUIRED_FIELDS mismatch with actual payloads
   blob requires content_oid/size_bytes, but RFC blob schema had encoding/bytes; fine. The real problem: artifact requires input_ids, but RFC uses input_artifact_oids; plus record_artifact() writes extra fields (created_at, parent_labels, prompt_id, model_config) not required/typed. This is going to drift immediately and breaks validation usefulness. [warning]

2. src/synix/build/snapshots.py — projection snapshot reads from mutable build/ outputs
   record_projection(SearchIndex) snapshots build_dir/search.db. That violates your new claim: .synix is canonical and snapshot commit “does not need to scrape build/”. You are scraping build outputs for projection state. This makes reproducibility depend on mutable directory contents and timing. [critical]

3. src/synix/build/snapshots.py — FlatFile projection constrained to build_dir
   You now hard-error if FlatFile writes outside build_dir. That’s an API break (previously likely allowed). Also inconsistent with the README claim that build/ is just a compatibility surface; now you force it for snapshotting. [warning]

4. src/synix/build/snapshots.py — pipeline fingerprint includes layer.config and proj.config unsafely
   You JSON-dump configs blindly. If configs include non-JSON types (callables, Path, datetimes), fingerprinting will crash during commit. This is plausible given “Python-first”. [critical]

5. src/synix/build/runner.py — snapshot transaction records cached artifacts repeatedly, risk of wrong “canonical”
   You call _record_snapshot_artifact() both on built and cached paths; OK. But you store artifact content into .synix every time (even cached) and do not assert that cached artifact content matches object-store content / artifact_id. If cache corruption happens, snapshot makes it permanent. [warning]

6. src/synix/build/refs.py / snapshots.py — locking is not portable and not process-safe on Windows
   _snapshot_lock uses fcntl when available; otherwise it’s a no-op. That means concurrent builds on Windows (or environments without fcntl) can corrupt refs. [critical]

7. src/synix/build/object_store.py — put_text still creates large transient encoded chunks
   The comment claims “without creating a second full-sized bytes copy”, but it still creates chunk bytes; fine. The bigger issue: you hash twice (once for digest, then re-encode to write). That’s O(2n) CPU and can be expensive for big artifacts. [minor]

8. src/synix/cli/runs_commands.py — no machine-readable output
   If “runs list” is meant for inspection only, fine. If it’s a platform primitive, you need --json. Otherwise users will parse tables and you’ve created an accidental API. [warning]

Missing

• Docs updates beyond README: docs/entity-model.md, CLI UX docs, and anything describing clean semantics and .synix lifecycle. You added a huge RFC but didn’t integrate it into user docs.
• Read-path support for refs/snapshots: list/show/search still appear to operate on mutable build outputs; no --ref plumbing yet.
• Failure-mode tests: crash mid-commit; partial object writes; release receipts not implemented but referenced; Windows locking behavior; corrupted ref files.
• Compatibility/migration plan: handling existing build/.synix vs sibling .synix beyond best-effort resolution; what happens if both exist?

Verdict

Block — you’re shipping a one-way persistence model while projections still depend on mutable build/ state, fingerprinting can crash on real Python configs, and concurrency safety is non-portable. Fix those or explicitly narrow scope (e.g., snapshot artifacts only, projections excluded) before merge.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,576 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:15:58Z

[github-actions]

Note

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

Summary

This PR introduces an immutable, git-like snapshot model for Synix builds. Every successful build now writes content-addressed objects (blobs, artifacts, projections, manifests, snapshots) under .synix/, advances a HEAD ref, and records an immutable run ref. The mutable build/ directory is retained as a compatibility surface. Includes an RFC document, object store, ref store, snapshot transaction logic, a synix runs list CLI command, and tests at all three levels.

Alignment

Strong fit. DESIGN.md §3.3 defines materialization keys as content-addressed cache keys, and §3.9 commits to audit determinism — this PR extends both by making entire build states content-addressed and immutable. The snapshot/manifest/ref model is the natural substrate for the branching and migration story (DESIGN.md §4.5, §1.3) — you can't do branch.diff() or branch.promote() cleanly against a mutable build/ directory. The RFC's separation of build vs. release aligns with the design's distinction between Steps (processing) and Artifacts (queryable targets). The projection-as-build-target concept directly maps to DESIGN.md §6.1's two artifact types.

Observations

1. [positive] The BuildTransaction pattern avoids scraping mutable build/ state at commit time. The test test_commit_uses_transaction_state_not_mutable_build_files explicitly verifies this — good defensive design.

2. [positive] Optimistic concurrency check in commit_build_snapshot (HEAD must not advance during build) fails closed. The test for this exists. Correct for single-machine safety.

3. [concern] _snapshot_lock falls back to no-op locking when fcntl is unavailable (Windows). The optimistic HEAD check partially compensates, but two concurrent builds on Windows could race between the HEAD check and the ref writes. Worth a comment or a Windows-compatible lock.

4. [concern] put_text in object_store.py iterates the string twice (once for hashing, once for writing). The outer try/except BaseException: raise on lines ~248-249 is a no-op — it catches and re-raises unconditionally. Dead code that adds confusion.

5. [question] _record_snapshot_artifact is called for both built and cached artifacts, but the runner calls it in ~7 separate locations with identical boilerplate. Could a single wrapper at the _save_artifact boundary reduce duplication and the risk of missing a call site?

6. [concern] run_id uses %Y%m%dT%H%M%S%fZ (microsecond precision), which becomes a ref path segment. If a ref name ever contains characters outside _REF_RE ([A-Za-z0-9._-]), it will fail validation. The %f digits are safe, but the format should be documented as a contract since refs/runs paths are user-visible.

7. [nit] iter_entries on ArtifactStore returns dict[str, dict] but the docstring says "shallow copy." This is only used indirectly — confirm it's needed in this PR or defer.

8. [positive] Tests are thorough: unit tests for object store canonicalization and ref cycles, integration tests for concurrent HEAD advancement, validation-failure gating, successive builds preserving old refs, source changes producing new manifests, and an e2e test exercising the full CLI flow including runs list. The RFC's "must-have failure tests" checklist is substantially covered.

9. [question] The RFC mentions schema versioning (SCHEMA_VERSION = 1) and the README notes a compatibility path, but get_json currently hard-rejects any version ≠ 1. Is there a plan for a version negotiation path before objects accumulate in the wild?

10. [concern] _projection_input_oids collects oids from layer_artifact_oids by iterating source_layers, but ordering depends on dict insertion order and list append order during the build. If build order changes across runs, the projection object's input_oids list changes, producing a different oid for semantically identical projections. Consider sorting.

Verdict

This is a well-structured foundational change that directly enables the branching, diffing, and rollback capabilities central to Synix's thesis — a good incremental step with solid test coverage and a clear migration path.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,576 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:16:13Z

[github-actions]

Note

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

Threat assessment — High-risk: you’re introducing a new canonical storage model (.synix snapshots + refs) that will become foundational, but the implementation is only half-integrated and has correctness/scale gaps.

One-way doors

1. .synix on-disk object + ref format (SCHEMA_VERSION=1)
   Hard to reverse because once users have history, you’re stuck supporting migrations or breaking their build history. “May evolve” in README isn’t a migration strategy.
   Safe to merge if: you commit to explicit migration tooling/version negotiation now (even minimal), and write compatibility tests for reading older schema versions (even if “upgrade only”).

2. Git-like refs/HEAD semantics as user-visible UX (runs list, refs/runs/<run_id>)
   Names/paths become contract. Changing them breaks scripts and mental model.
   Safe to merge if: you clearly document ref namespace stability guarantees and add --ref plumbing to existing list/show/search soon (otherwise “canonical store” is unusable except via one command).

3. Run ID format change to microseconds (%Y%m%dT%H%M%S%fZ)
   This leaks into ref names (refs/runs/<run_id>). Any later change is breaking.
   Safe to merge if: you lock the run_id format or decouple run refs from run_id (e.g., snapshot oid or monotonic counter).

Findings

1. src/synix/build/object_store.py: object schema validation is inconsistent with what you write
   artifact required fields omit prompt_id, model_config, created_at, parent_labels but snapshots.py writes them; meanwhile required includes input_ids but RFC calls it input_artifact_oids/input_oids. Also “blob” schema requires content_oid + size_bytes but blob objects store another oid pointing to raw bytes—fine, but naming is confusing.
   Failure mode: schema drift + future migrations become guesswork.
   Severity: [warning]

2. src/synix/build/snapshots.py: snapshot lock + ref atomicity is not actually atomic across platforms/filesystems
   You lock .synix/.lock, then write refs/runs/... then refs/heads/main. If crash between them, you leave dangling run refs or a head not advanced. Also atomic_write implementation unknown; if it’s write+replace without fsync(dir), you can lose refs on power loss.
   Severity: [critical]

3. ObjectStore.put_bytes: race can corrupt on concurrent writers
   Pattern: if path.exists(): return; _atomic_write_bytes(path, data). Two processes can both not see the file and then race writing. os.replace will make last-writer win, but contents identical so OK; however temp file naming in same dir is fine. Bigger issue: no verification that existing file matches oid (corruption undetected).
   Severity: [warning]

4. BuildTransaction.record_artifact: stores full artifact content as a blob object per artifact, every run
   There’s no dedupe at artifact-object level unless all metadata identical; content blob is deduped, but artifact JSON includes created_at, parent_labels, metadata (with layer fields) etc. You will generate a new artifact object each run even when cached. That explodes .synix size over time on large corpora.
   Severity: [critical]

5. runner.py: snapshot records for cached artifacts call provenance.get_parents(cached.label)
   If provenance tracking is mutable/run-scoped, parents for cached artifacts might be missing or wrong (depends on provenance implementation). You’re encoding provenance into immutable artifact objects (parent_labels) without ensuring it’s complete for cached reuse.
   Severity: [critical]

6. Design mismatch: README claims .synix is “source of truth”, but core commands still operate on build/
   Snapshot manifest has projections: {} always; search/show/list still presumably read mutable build artifacts, not snapshots. That’s a conceptual split-brain: “canonical” but not used.
   Severity: [warning]

7. refs.synix_dir_for_build_dir: legacy/nested naming is confusing and likely wrong
   Variable legacy = build_path.parent / ".synix" but doc says nested build/.synix is compatibility; name is inverted. More importantly: if both exist, you silently prefer sibling store; users can end up with two stores and confusion.
   Severity: [minor]

Missing

• Scale tests / retention strategy for .synix growth (GC, compaction, pruning, or at least “no-GC yet but bounded by X” guidance).
• Crash-consistency tests (kill between writing run ref and head ref; partial writes; corrupted object file).
• Docs update beyond README snippet: entity model + cache semantics should mention snapshots/refs and what is/ չէ isn’t canonical yet.
• Integration into show/list/search with --ref/--snapshot to make snapshots actually usable.

Verdict

Block — this introduces a one-way-door storage substrate but currently has durability and unbounded-growth risks, and it encodes potentially-incomplete provenance into immutable history. Fix crash consistency + artifact object churn + provenance correctness before merging.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,536 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:20:51Z

[github-actions]

Note

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

Summary

This PR introduces an immutable snapshot layer (.synix/) alongside the existing mutable build/ directory. It adds a content-addressed object store, git-like refs (HEAD, run refs), manifest/snapshot objects, a BuildTransaction that accumulates artifact state during builds, and a synix runs list CLI command. An RFC document lays out the full vision; the code implements the first slice (artifacts only, no projection snapshots yet).

Alignment

Strong fit. DESIGN.md's core invariants — artifacts are immutable and content-addressed, provenance chains are complete, cache keys capture all inputs — are directly reinforced. The snapshot model makes immutability a storage-level guarantee rather than just a logical one. The RFC's separation of build vs. release echoes DESIGN.md's sync() vs. run() pattern and respects "audit determinism over reproducibility" by creating an inspectable history of every build state. The _pipeline_fingerprint function captures layers, configs, and dependencies — consistent with the materialization key philosophy. Keeping Python-first pipeline definitions unchanged respects the CDK-over-config bet.

Observations

1. [positive] object_store.py — Atomic writes, canonical JSON serialization, schema versioning, and content-addressed OIDs are all textbook correct for an immutable store. The _validate_object_payload with per-type required fields and type checks is thorough.

2. [positive] snapshots.py — commit_build_snapshot checks both HEAD target and HEAD OID before advancing, failing closed on concurrent modification. This is the right safety model for a pre-1.0 system without WAL-level concurrency.

3. [concern] snapshots.py:_snapshot_lock — The Windows msvcrt path writes a byte then locks byte 0, but if the file is newly created and empty, the read(1) check works — however, if two processes race on creation, both could write b"0" before either locks. The lock acquisition itself should be safe since msvcrt.locking is blocking, but the preamble is fragile. Consider simplifying to always seek(0) and lock without the read/write dance.

4. [concern] runner.py — _record_snapshot_artifact is called in 7 places with identical argument patterns. This is a maintenance hazard. If a new artifact-producing path is added and the call is forgotten, the snapshot silently omits artifacts. Consider hooking this into store.save_artifact or the provenance tracker instead of sprinkling it through the runner.

5. [question] refs.py:synix_dir_for_build_dir — The fallback logic checks legacy then nested then defaults to legacy. But legacy is build_dir.parent / ".synix" — if the project root differs from build_dir.parent, this silently places .synix in the wrong directory. The load_pipeline integration resolves relative paths, but CLI commands like runs list --build-dir ./build bypass pipeline loading entirely. Is this reliable for non-default layouts?

6. [concern] object_store.py:put_text — The text is iterated twice (once for hashing, once for writing). For very large artifact content this doubles CPU work. The outer try/except BaseException: raise at the bottom is a no-op and should be removed.

7. [positive] Test coverage is strong: unit tests for object store, refs (including cycle detection), integration tests for concurrent HEAD advancement, validation failure blocking snapshot advancement, successive builds, source changes, and an e2e CLI flow test. The validation-failure-no-snapshot test is exactly the right edge case.

8. [nit] run_id format changed to microsecond precision (%Y%m%dT%H%M%S%fZ). This is used as a ref path component (refs/runs/{run_id}). The _REF_RE regex allows digits and hyphens but the %f microseconds add 6 extra digits — this works, but the resulting ref names are unwieldy. Consider a shorter unique suffix.

9. [nit] artifacts.py:iter_entries returns dict[str, dict] — the name suggests iteration but the return type is a full dict copy. get_entries() or entries() would be clearer.

10. [question] The RFC mentions LENS as a motivating use case ("required by LENS") but LENS appears nowhere in existing documentation. This creates an unexplained forward reference for reviewers.

Verdict

This is a well-structured foundational PR that correctly implements the immutable-snapshot substrate the project needs, with appropriate scoping (artifacts-only first slice), good failure-mode coverage, and clean separation from the existing mutable build path — a strong incremental step.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,536 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:21:02Z

[github-actions]

Note

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

Threat assessment — High risk: this introduces a new canonical persistence model (.synix snapshots/refs) that will become foundational, but it lands partially (artifacts-only) and with several correctness and compatibility gaps.

One-way doors

1. .synix object store + schema_version=1 format

   • Why hard to reverse: Once users have histories in .synix/objects and tooling/scripts depend on it, changing object schemas, keying, or layout becomes a migration problem.
   • Safe to merge if: there’s an explicit migration/compat story documented (even “no migration until v1”), plus a compatibility reader for older schema versions, and tests proving mixed schema dirs don’t brick builds.

2. Git-like refs semantics (HEAD, refs/heads/main, refs/runs/<run_id>)

   • Why hard to reverse: Ref naming and resolution rules become user mental model + automation surface (CI, “rollback”, etc).
   • Safe to merge if: ref naming is locked down now (run_id format, allowed chars), and you add a stable CLI/API to resolve refs (not just “list and scrape”).

3. Run IDs as UTC timestamp + microseconds, used as ref names

   • Why hard to reverse: refs/runs/<run_id> bakes the ID format into filesystem paths and user tooling.
   • Safe to merge if: you guarantee uniqueness (include random suffix) and define the format as stable or explicitly “may change” with a machine-readable mapping (e.g., store run objects indexed by oid, not by formatted id).

4. Meaning of synix clean

   • Why hard to reverse: Users will trust clean semantics. You’re changing it: “doesn’t delete history”.
   • Safe to merge if: CLI help/docs updated and clean --all (or similar) exists to remove .synix with explicit confirmation.

Findings

1. src/synix/build/snapshots.py: record_artifact() stores blob incorrectly
   Pattern: content_oid = _store_blob_text(...); artifact_payload["content_oid"]=content_oid
   Failure: _store_blob_text returns the oid of the blob JSON object, not the raw bytes oid. Field name says content_oid but actually points to a “blob object”. The RFC itself is inconsistent here too. This will confuse every reader and breaks future tooling expecting content_oid to be bytes oid. [critical]

2. src/synix/build/object_store.py: _atomic_write_bytes() leaks tmp file on success
   Pattern: os.replace(tmp, str(path)) but never closes/unlinks tmp path; also uses os.close(fd) before replace but if replace succeeds, tmp no longer exists; fine—however the real issue is missing finally for close on success in all branches and potential double-close on exception paths. This is fragile. [warning]

3. src/synix/build/runner.py: snapshot records on cached artifacts may overwrite built artifacts order-dependently
   Pattern: _record_snapshot_artifact() called for cached and built paths; transaction removes prior oid from layer_artifact_oids only if previous oid is in list.
   Risk: inconsistent “canonical” artifact per label depending on build order/caching behavior. You’re asserting label uniqueness but not enforcing it globally across layers. [warning]

4. src/synix/build/snapshots.py: _pipeline_fingerprint() includes layer.config which may include prompt functions/objects
   Pattern normalizes callables to module.qualname only.
   Risk: prompt changes won’t change fingerprint unless config contains actual prompt text elsewhere; also module.qualname is not stable across refactors. You are building a “canonical history model” on a weak fingerprint. [warning]

5. src/synix/build/refs.py: synix_dir_for_build_dir() precedence is odd and undocumented in CLI
   It prefers sibling .synix if it exists, else nested build/.synix if it exists, else sibling. That means moving a project directory can silently fork history depending on which dir happens to exist. [warning]

6. **src/synix/cli/runs_commands.py: --json output is wrong API use** Pattern: console.print_json(json.dumps(runs))(string) instead of passing object. Output likely double-encoded JSON. Tests probably pass accidentally becausejson.loads(runs_json.output)` might be consuming extra formatting/newlines unpredictably. [minor]

7. Design doc mismatch: DESIGN.md emphasizes materialization keys + DB-backed records; PR introduces git-like snapshots without reconciling with that model. You’re creating two sources of truth (DB vs .synix) with no stated relationship. [warning]

Missing

• Documentation update beyond README: CLI UX docs, entity-model doc, cache semantics doc should explain .synix as source of truth.
• A clean --all / prune command and tests.
• Tests for corruption/partial writes in .synix (power loss mid-write), and for Windows locking behavior.
• Explicit API for “show/search/list at ref” (README implies .synix is source of truth, but commands still operate on build/).
• A clear definition of what becomes canonical: artifacts only vs projections later—right now “snapshot” is not a full build state, so naming is misleading.

Verdict

Block — the snapshot store is a one-way door, and right now the object identity fields are internally inconsistent (blob/content_oid), fingerprints are unreliable, and the system introduces a second canonical store without a coherent integration story. Fix the critical schema/oid issue and tighten semantics/tests/docs before merging.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,614 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:23:48Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots with a git-like ref/object model (.synix/ directory, content-addressed object store, HEAD, run refs). Every successful build now commits an immutable snapshot; the mutable build/ directory is retained as a compatibility surface. A new synix runs list CLI command exposes snapshot history. An accompanying RFC document lays out the full vision including projections, release, and revert as future work.

Alignment

Strong fit. DESIGN.md's core invariant is that artifacts are immutable and content-addressed, with complete provenance chains. This PR moves the source of truth from a mutable build/ directory to content-addressed objects keyed by SHA-256 — a direct strengthening of those guarantees. The snapshot/manifest/ref model mirrors the materialization key design (cache keys capture all inputs) and makes audit determinism concrete: you can now point at an exact snapshot OID and trace what was built. The RFC explicitly separates build from release, which aligns with "architecture is a runtime concern" — you can build multiple variations and release selectively. The Python-first principle is preserved; no config files introduced.

Observations

1. [positive] object_store.py — Canonical JSON serialization (sort_keys=True, deterministic separators) ensures stable OIDs for equivalent payloads. Validated by test_put_json_is_canonical. This is exactly right for content-addressed storage.

2. [positive] snapshots.py commit_build_snapshot — The lock-then-check-HEAD-hasn't-moved pattern is a sound optimistic concurrency control. The test test_commit_rejects_when_head_advances_during_build covers this.

3. [concern] snapshots.py:_snapshot_lock — The msvcrt locking branch writes a byte then locks byte 0. If the file is empty and another process races the same check, both could write before either locks. The Unix fcntl path is fine. Consider using msvcrt.locking on a pre-allocated byte or documenting this as Unix-primary.

4. [concern] object_store.py:put_text — Text is encoded twice: once for hashing, once for writing. For large artifacts this doubles CPU work. The two encoder instances could theoretically produce different output if the encoder has state side-effects (unlikely for UTF-8, but the generic codecs.getincrementalencoder path allows arbitrary encodings). Consider hashing-then-writing in one pass, or restricting to UTF-8.

5. [question] snapshots.py:record_artifact — The artifact_id integrity check (artifact_id != content_hash) only fires when artifact.content is truthy. Source artifacts with empty content would silently skip validation. Is that intentional?

6. [concern] runner.py — _record_snapshot_artifact is called at seven distinct call sites via copy-pasted blocks. This is fragile; a new artifact-emitting code path could easily miss the call. Consider a single _save_and_record helper that wraps both store.save_artifact and the snapshot recording.

7. [nit] logging.py — Run ID changed from %Y%m%dT%H%M%SZ to %Y%m%dT%H%M%S%fZ (microseconds). The %f format always emits 6 digits on CPython but the RFC mentions formats like 20260306T082007123456Z. The ref path refs/runs/20260306T082007123456Z is valid per _REF_RE, but the README note says the format is experimental — good.

8. [positive] Test coverage is thorough: unit tests for object store, refs (including cycle detection), integration tests covering concurrent HEAD advancement, artifact ID integrity, validation-blocks-snapshot, fingerprint stability ignoring secrets/paths, and an e2e test exercising two builds then runs list with both table and JSON output.

9. [question] refs.py:synix_dir_for_build_dir — Falls back to a legacy sibling path, then a nested path inside build, then defaults to legacy. If neither exists yet, a fresh project always gets the sibling. But synix clean (documented as only removing the mutable surface) won't touch .synix/. Is there a synix clean --all or equivalent planned?

10. [nit] snapshots.py:_pipeline_fingerprint includes layer.config which may contain Path objects or callables. _normalize_fingerprint_value handles these, but the callable fingerprint uses __qualname__ — renaming a function changes the fingerprint silently. This matches DESIGN.md's "source hash" approach for prompts, so it's consistent but worth documenting.

Verdict

This is a well-structured, well-tested foundational change that strengthens Synix's core immutability and provenance guarantees; the main actionable concern is the duplicated snapshot recording call sites in runner.py which should be consolidated before the pattern proliferates further.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,614 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:24:06Z

[github-actions]

Note

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

Threat assessment — High risk: this PR quietly introduces a new on-disk “source of truth” and ref model that will be hard to change once users have real history in .synix/.

One-way doors

1. Canonical on-disk store .synix/ + object schema v1
   Hard to reverse because users will accumulate build history and tooling will start depending on object types/fields and directory layout.
   Safe-to-merge requires: explicit migration/versioning policy implemented (not just documented), plus backward-compat read support for at least one prior schema, and a clear “experimental: may break” gating (feature flag or --enable-snapshots).

2. Refs/HEAD semantics and naming (refs/heads/main, refs/runs/<run_id>)
   Hard to reverse because scripts/ops will start treating these as stable APIs.
   Safe-to-merge requires: commit to ref namespace rules (allowed chars, max depth) and publish a stability promise; otherwise keep it behind experimental CLI namespace (synix exp runs list).

3. Run ID format change to microsecond timestamp (core/logging.py)
   Hard to reverse because it becomes part of ref names and filesystem paths.
   Safe-to-merge requires: collision guarantees + documented parsing rules + decide whether run IDs are sortable, unique across machines, etc.

4. Pipeline API surface change: Pipeline.synix_dir (core/models.py, build/pipeline.py)
   Hard to reverse because it becomes part of Python pipeline definitions.
   Safe-to-merge requires: decide if synix_dir is truly public/stable; if not, keep it internal and infer from project root.

Findings

1. src/synix/build/refs.py:synix_dir_for_build_dir chooses store based on existence
   Risk: running same project with different build_dir values can “discover” different .synix roots depending on what exists. This is a footgun for multi-pipeline/multi-build-dir setups and can silently split history. [critical]

2. src/synix/build/snapshots.py:BuildTransaction.record_artifact uses if artifact.content:
   Empty-string artifacts skip artifact_id integrity normalization but still store a blob and artifact object with potentially artifact_id=None (yet _REQUIRED_FIELDS requires artifact_id). This will throw or corrupt semantics depending on upstream. Should treat empty string as valid content and hash it. [critical]

3. Object store writes are not crash-safe at directory-creation boundaries (object_store.put_bytes/put_json)
   You atomic-write the file, but two writers can race: both see not exists, both write temp, last os.replace wins. Content is same so OK, but you can still hit exceptions around tmp handling on Windows. Also no fsync of parent directory after replace (POSIX durability hole). [warning]

4. RefStore.write_ref is non-atomic at the ref-set level
   You added a journal to recover, but commit applies two ref writes sequentially. If process dies after writing one ref and before journal cleanup, recovery replays (fine). If process dies before writing any ref but after writing journal, recovery replays (fine). The real gap: partial writes to the ref file if atomic_write isn’t truly atomic cross-platform. I can’t verify atomic_write implementation; this is a reliability dependency. [warning]

5. runner.py concurrency block indentation looks wrong
   In the diff, if use_concurrent: block is indented such that _execute_transform_concurrent(...) call may be over-indented (would be a SyntaxError or logic bug). If it’s real, this is a hard blocker; if it’s just diff formatting, ignore—but you need a test that exercises concurrent path. [critical]

6. Snapshot integrity leaks mutable artifact.metadata (record_artifact)
   You mutate artifact.metadata in-place to add layer_name/layer_level. That metadata is also written to the legacy build/ surface and provenance. You’re now coupling snapshot schema to whatever random metadata is present at runtime. Expect nondeterministic fingerprints and bloat. [warning]

7. ObjectStore.get_json validates schema_version==1 only
   You’ve guaranteed future compatibility in README notes, but code currently hard-fails on any schema bump. That’s fine internally, but contradicts “compatibility path” language unless you implement multi-version readers. [warning]

8. CLI option name --json (runs_commands.py)
   This is a UX one-way door and clashes with user expectation across commands (some may already use --json). If you standardize, do it consistently across CLI. [minor]

Missing

• Documentation updates beyond README: CLI UX docs and entity model docs should mention .synix, clean semantics, and refs.
• Tests that cover concurrent builds against the same .synix store (lock correctness, “HEAD advanced during build” path).
• Explicit boundaries: what commands read from snapshots vs legacy build/? Right now it’s “source of truth” language in README but most commands likely still read build/.
• A migration story for existing users with build/.synix nested stores (you mention compatibility, but behavior is “exists-based” guessing).

Verdict

Block. This introduces a new canonical persistence model and ref semantics, but still has correctness holes (empty content artifact hashing) and path-resolution footguns; tighten invariants and clarify/store selection before shipping.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,844 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:30:53Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots, a content-addressed object store (.synix/objects), git-like refs (HEAD, refs/runs/*), and a synix runs list CLI command. Every successful build now commits an immutable snapshot with a manifest closure over artifact oids, while the mutable build/ directory is retained as a compatibility surface. An RFC document lays out the full vision including projections, release, and revert — only the artifact snapshot slice ships here.

Alignment

Strong fit. DESIGN.md states artifacts are content-addressed (content_fingerprint = SHA-256), layers form a DAG, and provenance chains must be complete. This PR makes those properties durable: artifacts become immutable objects keyed by content hash, manifests capture the full closure, and snapshots form a linked chain via parent_snapshot_oids. The separation of build from release directly supports Hypothesis 3 (architecture is a runtime concern) — you can build, inspect, and only then decide what to materialize. The audit determinism principle (Appendix C) is advanced: snapshots make "explain what produced what" structurally guaranteed rather than best-effort from mutable files.

Observations

1. [positive] object_store.py — Canonical JSON serialization (sort_keys=True, separators) ensures identical payloads produce identical oids. This is the right foundation for content-addressed storage.

2. [positive] test_snapshots.py — Excellent failure-mode coverage: HEAD-advance rejection, artifact-id/content mismatch, ref journal recovery, validation-failure-blocks-snapshot. These directly match the RFC's "Must-Have Failure Tests" section.

3. [concern] runner.py:285-289 — Indentation of _execute_transform_concurrent call is mangled (extra indentation). This looks like an accidental whitespace change that will cause a syntax/style issue or confuse future readers, even if Python accepts it.

4. [concern] snapshots.py _snapshot_lock — The msvcrt locking branch writes a byte and locks byte 0, but fcntl.flock locks the whole file. On Windows, if the file is empty, msvcrt.locking(..., 1) locks 1 byte starting at position 0 — but the read(1) == b"" / write(b"0") dance is fragile if two processes race to that initialization. Low risk for now (single-writer builds), but worth a comment.

5. [concern] runner.py — The _on_batch_complete and _on_cached callback signatures changed from (list[Artifact]) to (list[Artifact], list[Artifact]). Any external code or custom transform calling these callbacks with the old arity would break silently. The PR doesn't document this as a breaking change. Since it's pre-1.0 this is acceptable, but the _execute_transform_concurrent function signature should be checked for all callers.

6. [question] snapshots.py:_pipeline_fingerprint — The fingerprint includes pipeline.llm_config but not source_dir or build_dir. This is tested and intentional (test at line ~190 of test_snapshots.py). However, it also doesn't include projection output_path, which means two pipelines writing to different FlatFile targets get the same fingerprint. Is that intended?

7. [question] refs.py:synix_dir_for_build_dir — The fallback resolution checks legacy then nested then defaults to legacy. If someone has both .synix (sibling) and build/.synix (nested), the sibling wins silently. This seems correct but could surprise users migrating between layouts.

8. [nit] logging.py — Run ID format changed from %Y%m%dT%H%M%SZ to %Y%m%dT%H%M%S%fZ for microsecond precision. Good fix for collision avoidance, but %f gives 6 digits — the README example shows 20260306T082007123456Z which is consistent. The ref path refs/runs/20260306T082007123456Z is a valid ref name per _REF_RE. Looks correct.

9. [positive] The RFC document is thorough and clearly separates what ships now from what's deferred. The "Notebook / Mutable Overlay" non-goal is a smart scope cut.

10. [concern] put_text in object_store.py encodes text twice — once for hashing, once for writing. For large artifacts this doubles CPU cost. The comment-free implementation makes this non-obvious. A brief comment explaining why (avoid holding full encoded bytes in memory) would help.

11. [nit] _get_parent_labels now returns [] when multiple labels share the same artifact_id hash. This silently drops lineage for duplicate-content artifacts. The fallback in _snapshot_parent_labels catches this, but the empty-list sentinel is subtle.

Verdict

This is a well-structured incremental step that moves Synix from mutable build state toward the immutable, content-addressed model the design document demands — the indentation issue in runner.py should be fixed before merge, but the architecture is sound.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,844 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:31:09Z

[github-actions]

Note

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

Threat assessment — High risk: this PR quietly introduces a new persistence substrate (.synix snapshots/refs) and run-id semantics that will be very hard to unwind once users have data on disk.

One-way doors

1. On-disk .synix object store + schema_version=1

   • Why hard to reverse: Users will accumulate history under .synix/objects and refs; any future format change becomes a migration/compat burden. You’re effectively committing to “git-like storage” as the core substrate.
   • Safe-to-merge if: You provide an explicit compatibility/migration policy (even if minimal), and you pin “v0.15.x format unstable” in docs and in code via a hard “format version” gate and a synix gc/synix snapshots migrate plan (even if stubbed).

2. Ref namespace + HEAD semantics (refs/heads/main, refs/runs/<run_id>)

   • Why hard to reverse: Users/tools will script around ref names and meanings. Changing default branch name or run-ref format breaks automation.
   • Safe-to-merge if: You commit to ref naming stability (or clearly mark experimental in CLI help and docs), and you add a --ref selector to read-only commands soon (otherwise users will treat mutable build/ as truth anyway).

3. run_id format change to microsecond timestamp

   • Why hard to reverse: Run refs and log paths now depend on a specific timestamp encoding; external systems may parse it.
   • Safe-to-merge if: Document run_id as opaque everywhere (CLI/API) and stop presenting it as time-sortable “by contract”.

Findings

1. src/synix/build/object_store.py: _validate_object_payload enforces schema_version == SCHEMA_VERSION

   • Failure mode: You cannot load older objects once SCHEMA_VERSION increments. This is an instant hard-break of existing stores; no forward-compat path.
   • Severity: [critical]

2. src/synix/build/snapshots.py: record_artifact() recomputes artifact.artifact_id from UTF-8 content

   • Failure mode: You’re redefining artifact identity in the snapshot lane, potentially diverging from existing artifact_id semantics (normalization rules, whitespace handling, binary payloads, JSON artifacts, etc.). Also silently overwrites artifact.artifact_id.
   • Severity: [critical]

3. src/synix/build/runner.py: snapshotting captures cached artifacts as canonical without guaranteeing content availability

   • Failure mode: For cached artifacts, you sometimes snapshot artifact.content from in-memory objects. If cache loading ever becomes lazy/partial, snapshots become incomplete/corrupt. There’s no assertion that content is present for every snapshotted artifact.
   • Severity: [warning]

4. src/synix/build/refs.py: synix_dir_for_build_dir() “legacy” vs “nested” resolution is confusing and can fork history

   • Failure mode: If both <parent>/.synix and <build>/.synix exist at different times, you can split runs across stores. The precedence rule (“legacy if exists else nested”) is non-obvious and stateful.
   • Severity: [warning]

5. src/synix/build/snapshots.py: Windows locking is not robust

   • Failure mode: The msvcrt.locking usage is brittle; file-locking semantics differ and you’re locking 1 byte with ad-hoc initialization. Also no stale-lock handling. Multi-process build reliability is questionable.
   • Severity: [warning]

6. src/synix/build/snapshots.py: ref update journaling is half-transactional

   • Failure mode: You write a journal, then update refs one-by-one. If you crash mid-way, recovery replays updates, but there’s no checksum/validation of journal content, no atomic “all-or-nothing” marker, and no handling for partial ref file corruption.
   • Severity: [warning]

7. src/synix/cli/runs_commands.py: --json flag name collides with common expectations

   • Failure mode: Click option --json is fine, but you also encourage “Prefer runs list --json over scraping”; meanwhile other commands likely don’t have JSON parity. This becomes an inconsistent CLI contract users will depend on.
   • Severity: [minor]

Missing

• Compatibility story in code: there’s no reader that can tolerate unknown schema versions or “best-effort” load with explicit error messaging + upgrade instruction.
• Tests for store selection edge cases: both nested and sibling .synix present; ensure you don’t silently read one and write the other.
• Tests for concurrent writers: two builds in parallel should not corrupt refs/objects; current tests only simulate HEAD advance, not actual parallelism.
• Docs updates beyond README: entity model / cache semantics still talk like build/ is canonical; this PR changes the mental model.
• A synix clean test: README claims it won’t delete snapshots; no test asserts .synix survives.

Verdict

Block. The snapshot substrate is a foundational one-way door, but schema/versioning and artifact identity are not safely defined yet; you’re locking in format and semantics without a credible compatibility and correctness story.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,830 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:32:43Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots — a git-like object store (.synix/objects), ref system (HEAD, run refs), manifest/snapshot objects, and a synix runs list CLI command. It also includes a detailed RFC (docs/snapshots-release-rfc.md) laying out the full vision for build/release separation, with this PR implementing the first mergeable slice. Projections remain in the mutable build/ surface for now.

Alignment

Strong fit. DESIGN.md's core invariants — artifacts are immutable and content-addressed, provenance chains are complete, cache keys capture all inputs — are directly served by this change. The content-addressed object store with SHA-256 oids mirrors the materialization key design. The snapshot-as-commit model enables the branching and architecture-migration story from Hypotheses 2 and 3. Separating build from release aligns with "audit determinism over reproducibility" — you can always inspect exactly what a build produced. The RFC explicitly defers branching UX to later work, which is consistent with the phased roadmap.

Observations

1. [concern] runner.py lines 285-289 — indentation of _execute_transform_concurrent is mangled (extra leading whitespace). This will either fail to parse or silently change scope. Likely a rebase artifact.

2. [concern] _snapshot_parent_labels falls back to [inp.label for inp in inputs] when both _get_parent_labels and provenance.get_parents return empty. For a Source layer with no real parents, this produces self-referential lineage (source artifacts listing themselves as parents). The fallback should probably return [].

3. [concern] _get_parent_labels now returns [] on ambiguous hash-to-label collisions (multiple labels share the same artifact_id). This silently drops lineage. A warning or explicit sentinel would help debugging. The old code didn't have this path — it's a behavioral change with no test covering the ambiguous case.

4. [question] put_text iterates the string twice (once for hashing, once for writing). For very large artifacts this doubles CPU work. Was streaming with tee-style write considered? Not blocking, but worth noting for the 10k-conversation scale concern.

5. [question] The _on_batch_complete and _on_cached callback signatures changed from (list[Artifact]) to (list[Artifact], list[Artifact]). Any external code or custom transforms calling these will break silently. Are these internal-only?

6. [positive] The ref-update journal with crash recovery (recover_pending_ref_updates) is a thoughtful detail — it prevents partial ref updates from leaving the store in an inconsistent state. The corresponding test (test_runs_list_recovers_pending_ref_updates) covers this well.

7. [positive] commit_build_snapshot checks that HEAD hasn't advanced during the build, failing closed on concurrent writes. Exactly the right safety for a content-addressed store.

8. [positive] Test coverage is strong: unit tests for object store, refs, and cycle detection; integration tests for concurrent HEAD, content-hash integrity, validation-blocks-snapshot, successive builds; e2e test for the full CLI flow with --json output verification.

9. [nit] run_id format changed from %Y%m%dT%H%M%SZ to %Y%m%dT%H%M%S%fZ (microseconds). The README documents run refs as refs/runs/20260306T082007123456Z. This is fine but the %f suffix looks like it could be confused with fractional seconds in ISO 8601 — consider an underscore separator.

10. [nit] _sanitize_llm_config strips keys containing "key", "token", "secret", "password" — this would also strip legitimate config like max_tokens. The substring match is too aggressive; max_tokens contains "token".

11. [concern] Item 10 is actually a correctness bug: _sanitize_llm_config will strip max_tokens from the pipeline fingerprint, meaning changing max_tokens won't change the fingerprint and won't trigger a rebuild. This contradicts "cache keys capture all inputs."

Verdict

This is a well-structured foundational change that directly enables the project's core differentiators, but the max_tokens fingerprint bug (#10/11) and the indentation issue (#1) need fixing before merge.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,830 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:32:57Z

[github-actions]

Note

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

Threat assessment — High risk: it quietly introduces a second canonical persistence model (.synix snapshots + refs) that will become hard to unwind once users have history on disk.

One-way doors

1. On-disk snapshot store layout + schemas (.synix/objects, HEAD, refs/*)

   • Why hard to reverse: Users will accumulate build history; changing object schema/layout requires migrations and compatibility readers forever.
   • Safe to merge if: You commit to a formal versioning/migration policy (reader/writer compatibility matrix), add an upgrade tool (even if minimal), and freeze ref naming semantics before advertising in README.

2. Run ref format is now user-visible (refs/runs/<run_id>)

   • Why hard to reverse: People/automation will copy-paste refs and/or parse them.
   • Safe to merge if: Stop presenting the raw ref as the primary handle in CLI output (show a stable “run id” separate from internal ref path), and document ref stability guarantees.

3. Pipeline(..., synix_dir=...) public API

   • Why hard to reverse: Becomes part of the Python API surface area and tutorials.
   • Safe to merge if: You define precedence rules and lifecycle (project-local vs global), and add validation that prevents foot-guns (e.g., synix_dir inside build_dir when clean exists).

Findings

1. src/synix/build/object_store.py: object schema mismatch vs RFC and internal usage
   Failure mode: _REQUIRED_FIELDS["blob"] expects {"type","schema_version","content_oid","size_bytes"}, but put_bytes/put_text/put_file store raw bytes at objects/<oid> with no JSON blob object at all. The RFC also says blob is just { "oid": ... } and not stored as JSON. This is inconsistent design and will rot quickly. [warning]

2. src/synix/build/snapshots.py: content hashing not normalized / inconsistent with stated semantics
   record_artifact() hashes artifact.content.encode('utf-8') without any normalization (README/DESIGN talk about normalized content / strip trailing whitespace). Also other places hash with default .encode() (no explicit utf-8). You’re baking in subtle cache/snapshot identity drift. [warning]

3. src/synix/build/snapshots.py: projections explicitly excluded from snapshot closure but README implies “canonical snapshots”
   README says “canonical immutable artifact snapshots”; RFC says manifest is closure over artifacts and projections, but implementation sets manifest["projections"] == {} always. Users will assume rollback/release is possible; it’s not. This is a semantic trap. [warning]

4. src/synix/build/runner.py: snapshot commit gating on validators changes meaning of “successful build”
   Now a build that produces artifacts but fails validation creates no snapshot history. That’s an operational one-way door: you lose forensic history for failed builds (often what you need most). Consider writing a snapshot marked “failed/invalid” and not advancing HEAD instead. [warning]

5. src/synix/core/logging.py: run_id-based refs are not collision-safe across processes
   Microseconds reduce collisions but don’t eliminate them on fast concurrent processes or clock anomalies. If two runs produce same run_id, refs/runs/<run_id> collides and history is corrupted/overwritten. Need randomness (uuid/ulid) or collision detection. [critical]

6. src/synix/build/refs.py: synix_dir_for_build_dir() discovery rules are brittle and can fork history
   It prefers <build parent>/.synix but will “honor nested build/.synix if it exists”. That means merely creating a nested directory changes where history is read from, and you can end up with confusing split-brain. You partially guard with “both exist” error, but single-existence still silently redirects. [warning]

7. src/synix/build/snapshots.py: ref update journaling is not transactional
   Journal is written, then refs written, then journal deleted. If crash after some refs written, journal replay may reapply (ok) but there’s no validation of journal schema/type/version beyond presence of updates. Also recover_pending_ref_updates will raise and block everything on one corrupt journal file. Needs “best-effort with quarantine” or strict atomicity. [minor]

8. Docs/Website mismatch remains
   Website content is garbled encoding (“Synix â��”) and now README mentions .synix semantics but website doesn’t; user confusion. [minor]

Missing

• A clear compatibility/migration story for existing users with build/.synix vs sibling .synix (tooling + doc).
• Tests for multi-process concurrency (two builds in parallel) and run ref collision behavior.
• Tests that synix clean does not remove .synix (README claims behavior; no coverage shown).
• CLI/docs updates for run vs build terminology: tests call synix run but README shows synix build. That mismatch is already a usability bug.

Verdict

Ship with fixes — the core direction is plausible, but you must fix run id collisions and clarify/align the snapshot/projection semantics before this becomes a de-facto storage API.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,874 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:35:15Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots — a git-like object store (.synix/objects), ref management (HEAD, refs/runs/*), manifest/snapshot objects, and a synix runs list CLI command. It also includes an RFC document (docs/snapshots-release-rfc.md) laying out the full vision for release/revert semantics. The implementation lands only the first slice: artifact snapshots and refs, with projections deferred.

Alignment

Strong fit. DESIGN.md defines artifacts as content-addressed, provenance chains as complete, and cache keys capturing all inputs. This PR makes those properties structurally durable — artifacts become immutable objects, builds become snapshots with parent chains, and HEAD gives a stable pointer. The RFC's separation of build vs. release mirrors DESIGN.md's distinction between artifacts (build data) and projections (queryable surfaces). The "audit determinism over reproducibility" principle is directly served: snapshots make the audit trail durable even when synix clean removes the mutable surface. Python-first is preserved — no config files introduced.

Observations

1. [positive] object_store.py — Canonical JSON serialization (sort_keys=True, deterministic separators) ensures identical payloads produce identical oids. This is the right content-addressing discipline. Validation is thorough with per-type required fields and type checks.

2. [positive] snapshots.py — The commit_build_snapshot function implements optimistic concurrency (check HEAD hasn't moved) with a write-ahead ref journal for crash recovery. This is a well-considered approach for concurrent builds. The lock uses platform-appropriate primitives (fcntl/msvcrt).

3. [concern] runner.py — The _save_artifact closure signature changed to add parent_inputs, and _on_batch_complete/_on_cached callbacks now take unit_inputs. These are internal but the concurrent execution path in _execute_transform_concurrent (line ~555) reconstructs unit_inputs from units_to_run[idx] — but idx iterates over results indices, not units_to_run indices. If units_to_run is a subset of all units (some were cached), idx won't correspond to units_to_run[idx]. This looks like a potential index mismatch bug.

4. [concern] models.py line 37 — Removing the and self.content guard from Artifact.__post_init__ means constructing Artifact(content="", ...) now always hashes. Good — the test covers this. But Artifact(content=None) would crash on .encode(). The dataclass default is "", so this only fails if someone passes content=None explicitly. Fragile but probably fine for now.

5. [question] snapshots.py _pipeline_fingerprint — The fingerprint includes pipeline.llm_config (with secrets stripped) but not source_dir or build_dir. The test confirms this is intentional. But it does include layer config dicts, which may contain local paths in practice. Is _normalize_fingerprint_value handling Path → str sufficient, or should paths be excluded like secrets?

6. [concern] object_store.py put_text — Computes the hash by encoding text in 64KB chunks via incremental encoder, then re-encodes the entire text on a second pass for writing. For very large artifacts this doubles the work. Acceptable at current scale, but worth a comment or TODO.

7. [positive] Test coverage is excellent. Unit tests for object store, refs (including cycle detection, ambiguous store resolution). Integration tests cover concurrent HEAD advancement, artifact-id/content mismatch, empty content, journal recovery, validation failure blocking snapshots, and source-change manifest divergence. E2E test exercises full CLI flow including --json output.

8. [nit] refs.py _REF_RE allows dots and hyphens but the RFC shows timestamp-based refs like 2026-03-06T12-30-11Z. The run_id format from logging.py is %Y%m%dT%H%M%S%fZ (no hyphens), so the actual refs will match. But the regex would also accept refs with characters that could cause issues on case-insensitive filesystems.

9. [positive] The _get_parent_labels fix (hash→list mapping) correctly handles the case where multiple inputs share the same content hash, returning [] to avoid ambiguous lineage rather than silently picking one. This is a provenance correctness improvement.

10. [nit] README says uvx synix runs list but the command is registered as runs group → list subcommand. Consistent, but the notes about run ref format being experimental are good defensive documentation.

Verdict

This is a well-designed, well-tested foundational change that makes Synix's immutability and provenance guarantees structurally real rather than implied — a strong incremental step. The potential index mismatch in the concurrent execution callback (observation 3) should be verified before merge.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,874 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:35:35Z

[github-actions]

Note

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

Threat assessment — High risk: it introduces a new on-disk canonical store (.synix) + ref semantics that will be extremely hard to unwind once users have data.

One-way doors

1. Canonicalization of .synix as “source of truth”

   • Hard to reverse because users will accumulate history in .synix/objects and refs; any later redesign becomes a migration problem (and “clean” semantics become sacred).
   • Safe to merge only if: you commit to a stable compatibility/migration policy now (e.g., schema versioning rules, upgrade tooling, and an explicit “delete everything” command that includes snapshots).

2. Public CLI surface: synix runs list [--json]

   • Hard to reverse because automation will parse it; even with the README warning, people will depend on JSON keys and ref naming.
   • Safe to merge only if: JSON schema is documented and versioned (even a schema_version field in output) and ref naming is not described as “timestamp-based experimental” while simultaneously being emitted as the primary identifier.

3. Ref naming and run_id format as externally visible IDs

   • run_id now includes microseconds and becomes refs/runs/<run_id>. That is now a durable external identifier. Changing later breaks scripts and stored references.
   • Safe to merge only if: you define a stable run-id format policy (and collision rules) and/or add an internal opaque ID separate from presentation.

Findings

1. src/synix/build/artifacts.py / src/synix/core/models.py: hashing empty content

   • Pattern: changed if not artifact.artifact_id and artifact.content to if not artifact.artifact_id, but still does artifact.content.encode() unconditionally.
   • Failure mode: artifact.content is None now crashes with AttributeError. You tightened the precondition without adding validation.
   • Severity: [critical]

2. src/synix/build/snapshots.py: snapshotting assumes UTF-8 text artifacts

   • Pattern: hashlib.sha256(artifact.content.encode('utf-8')) and put_text(artifact.content) with no type/None checks.
   • Failure mode: any non-text artifact (or content None) explodes; also you’re baking “all artifacts are UTF-8 text” into the snapshot store even though projections/artifacts might become structured/binary later.
   • Severity: [warning]

3. src/synix/build/snapshots.py: manifest “closure” is not actually a closure

   • Pattern: manifest stores artifacts={label: oid} only for labels recorded during this run; projections empty by design.
   • Failure mode: if a run is incremental and reuses artifacts but never touches some labels (e.g., not traversed due to configuration), are they included? You rely on _record_snapshot_artifact being called for cached/built paths, but any missed path yields an incomplete manifest that cannot reproduce HEAD.
   • Severity: [critical]

4. src/synix/build/runner.py: validators gate ref advancement but not object creation

   • Pattern: snapshot transaction records artifacts during build; commit skipped on validation failure.
   • Failure mode: orphaned objects accumulate forever on failed validations (content blobs + artifact objects) with no GC, no compaction, no “prune”. This will become a disk leak at scale.
   • Severity: [warning]

5. src/synix/build/refs.py: synix_dir_for_build_dir precedence is confusing and mismatched with docs

   • Pattern: chooses sibling .synix by default, but will silently use nested build/.synix if present.
   • Failure mode: users can fork history by accidentally creating the nested dir; you only error if both exist. That’s a reliability footgun.
   • Severity: [warning]

6. src/synix/build/object_store.py: schema validation is inconsistent

   • Pattern: _REQUIRED_FIELDS["blob"] expects JSON blob objects, but put_bytes/put_text/put_file store raw bytes directly with no JSON wrapper. Also _OPTIONAL_FIELD_TYPES["snapshot"]["run_id"] duplicates required run_id.
   • Failure mode: dead schema paths + confusing contracts; future maintainers will assume “blob” objects exist.
   • Severity: [minor]

7. CLI/documentation mismatch

   • README: says uvx synix runs list but tests invoke synix run not synix build. There’s drift between CLI verbs in docs/design.
   • Failure mode: user confusion + broken docs; pre-1.0 is when you should be converging, not widening.
   • Severity: [minor]

Missing

• A clear deletion/retention story: synix clean explicitly does not remove .synix, but there is no synix snapshots prune / synix reset --hard equivalent.
• Tests for non-UTF8 / binary / None artifact content, and for artifact content type enforcement at boundaries.
• Any tests around concurrent builds in different processes beyond HEAD-advance detection (e.g., partial object writes, interleaving two commits).
• Documentation updates outside README: entity-model/cache semantics likely now need .synix mentioned, and “build directory is canonical” statements must be purged.

Verdict

Ship with fixes — the snapshot/ref substrate is plausible, but the current implementation has boundary-condition crashes (content None), a potentially incomplete manifest contract, and no pruning story; merging as-is creates a hard-to-reverse storage/API commitment with obvious operational debt.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,923 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:37:43Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots, a content-addressed object store (.synix/objects), git-like refs (HEAD, refs/runs/*, refs/heads/main), and a synix runs list CLI command. It's the first mergeable slice of a larger RFC that separates build from release, making every successful build produce an immutable snapshot while preserving the mutable build/ directory as a compatibility surface.

Alignment

Strong fit. DESIGN.md's core principles — artifacts are immutable and content-addressed, provenance chains are complete, cache keys capture all inputs — are exactly what this PR formalizes at the storage layer. The content-addressed object store with SHA-256 oids mirrors the materialization key design (§3.3). The snapshot/manifest model creates a proper closure over build state, which is prerequisite infrastructure for the branching and architecture-migration stories (§1.3, §4.5). Separating build from release aligns with "audit determinism over reproducibility" — you can now point at any snapshot and explain exactly what was built. The RFC explicitly defers projections from the snapshot closure, which is honest scoping.

Observations

1. [positive] object_store.py — Canonical JSON serialization (sort_keys=True, separators) ensures equivalent payloads produce identical oids. This is the right choice for content-addressed storage and directly supports the DESIGN.md fingerprinting contract.

2. [positive] snapshots.py — The commit_build_snapshot function performs optimistic concurrency control (checking HEAD hasn't advanced), uses a ref update journal for crash recovery, and acquires a file lock. This is serious infrastructure for a pre-1.0 project and prevents the class of bugs where concurrent builds silently corrupt state.

3. [positive] Test coverage is thorough: unit tests for object store, refs, and ref cycles; integration tests for concurrent HEAD advancement, artifact-id/content mismatch, empty content, journal recovery, pipeline fingerprinting, validation-failure-blocks-snapshot; and a full e2e CLI flow test. The failure-mode tests (items 2-4 in integration) are particularly good.

4. [concern] object_store.py:put_text — The text is encoded twice: once for hashing, once for writing. For large artifacts this doubles CPU work. The chunked incremental encoder is careful about memory, but the two-pass approach means the digest and the written bytes could theoretically diverge if the encoder has internal state that isn't perfectly reset. Consider hashing during the write pass instead.

5. [concern] runner.py — The _save_artifact closure now takes parent_inputs as an optional kwarg, but the _on_batch_complete and _on_cached callback signatures changed from (list[Artifact]) to (list[Artifact], list[Artifact]). Any external code or transform that calls these callbacks with the old arity will break silently. The _execute_transform_concurrent function is updated, but grep the codebase for other callers.

6. [concern] snapshots.py:_snapshot_lock — The msvcrt locking branch writes a byte if the file is empty, then locks byte 0. This is fragile — if two processes race on the initial write, the lock region may not be consistent. On Windows this could allow concurrent snapshot commits.

7. [question] refs.py:synix_dir_for_build_dir — The default placement is build_dir.parent / ".synix". If a user sets build_dir = "./output/build", the .synix dir lands in ./output/, not the project root. Is this intentional? It could surprise users who expect .synix next to pipeline.py.

8. [question] models.py — The Artifact.__post_init__ change removes the and self.content guard, meaning construction with content="" now computes a hash eagerly. This is tested and intentional per the empty-content test, but it means Artifact(label="x", artifact_type="t", content="") is now semantically valid everywhere. Are there codepaths that use empty content as a sentinel?

9. [nit] logging.py — Run ID changed from %Y%m%dT%H%M%SZ to %Y%m%dT%H%M%S%fZ (microsecond precision). The %f format gives 6 digits, making run refs like refs/runs/20260306T082007123456Z. The RFC examples show refs/runs/2026-03-06T12-30-11Z with dashes — the actual format doesn't match the RFC's examples.

10. [nit] snapshots.py:_pipeline_fingerprint — Includes pipeline.llm_config but not pipeline.source_dir. This is correct per the test (machine-local paths excluded), but pipeline.concurrency is also excluded. Worth a comment explaining the boundary.

Verdict

This is a well-designed, well-tested foundational change that correctly advances Synix toward its stated vision of immutable, content-addressed build artifacts with full provenance — the _on_batch_complete signature change needs careful verification against all callers, but otherwise this is a strong incremental step.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 2,923 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:37:51Z

[github-actions]

Note

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

Threat assessment — Medium-to-high risk: this PR introduces a new persistence substrate (.synix snapshots/refs) that will become foundational, but it lands partially (artifacts only) and the seams with existing build/ behavior are not fully pinned down.

One-way doors

1. On-disk .synix object/ref format (SCHEMA_VERSION=1)

   • Why hard to reverse: Once users have .synix/objects and refs, you own migrations/compat forever. The README already documents it, and runs list --json encourages tooling integration.
   • Safe to merge when: you have an explicit migration policy + loader behavior for older schemas (you partially do), plus a forward-compat story for ref layout and run_id naming, and you commit to never reusing incompatible state silently (README says this, but code needs enforcement beyond schema_version checks).

2. Ref namespace and semantics (HEAD, refs/heads/main, refs/runs/<run_id>)

   • Why hard to reverse: Users will script around ref paths; changing them breaks automation. Also the “HEAD is symbolic” model becomes mental-model bedrock.
   • Safe to merge when: ref naming is nailed (including run refs), and there’s a documented stability tier (“experimental” is not enough if it ships in CLI help/README).

3. Pipeline.synix_dir public model field

   • Why hard to reverse: This is now part of the Python API surface; downstream code will set it.
   • Safe to merge when: you document precedence rules and ensure consistent behavior across CLI vs library usage.

Findings

1. src/synix/build/object_store.py: blob schema vs implementation mismatch

   • Pattern: _REQUIRED_FIELDS["blob"] expects {"type","schema_version","content_oid","size_bytes"} but put_bytes/put_text/put_file store raw bytes only, not JSON “blob objects”.
   • Failure mode: You’ve defined two incompatible “blob” concepts (raw-bytes addressed by oid vs JSON blob object). Future contributors will write “blob objects” that your store can’t create/read consistently.
   • Severity: [warning]

2. src/synix/build/snapshots.py: snapshot artifact map keyed by label

   • Pattern: manifest["artifacts"] is {label: oid}.
   • Failure mode: labels are not guaranteed stable/unique across pipeline evolution (renames, collisions, multi-layer same label). This makes manifests fragile and undermines “immutable logical snapshot” claims. You already have artifact_id as content identity; label is presentation.
   • Severity: [critical]

3. src/synix/build/runner.py: concurrency + snapshot recording thread-safety

   • Pattern: _execute_transform_concurrent calls on_complete from the main thread, but artifact creation happens in worker threads; you’re mutating snapshot_txn from callbacks that appear serialized but depend on implementation details.
   • Failure mode: subtle race if callbacks ever move into worker context or if future refactors call _save_artifact concurrently. No locking in BuildTransaction.record_artifact.
   • Severity: [warning]

4. src/synix/core/models.py: Artifact now requires content: str

   • Pattern: hard TypeError in Artifact.__post_init__.
   • Failure mode: breaks any existing transforms/projections that emit non-text (JSON/dicts) or binary-ish content. Design docs explicitly mention “text/data”; README mentions --raw JSON. This is an API behavior break.
   • Severity: [critical]

5. src/synix/build/pipeline.py: implicit .synix placement tied to build_dir

   • Pattern: pipeline.synix_dir inferred from build_dir if unset.
   • Failure mode: moving build_dir changes snapshot store location (or triggers “ambiguous” error) causing “forked history” surprises. This is especially nasty in CI or templates.
   • Severity: [warning]

6. src/synix/build/refs.py: ref validation blocks common names

   • Pattern: _REF_RE disallows slashes? It allows / segments, but only [A-Za-z0-9._-]. No @, no :. Fine, but you should be explicit since README suggests timestamp refs with T/Z (ok) but RFC examples include 2026-03-06T... (dash/colon issues).
   • Failure mode: documented ref formats may be invalid depending on exact string. (RFC shows 2026-03-06T12-30-11Z vs code uses run_id YYYYMMDDT...%fZ.)
   • Severity: [warning]

Missing

• Docs alignment: DESIGN.md still frames v0.1 as SQLite/materialization keys; no mention of .synix snapshots/refs or “build is not source of truth”. This is a design-level shift and should be reflected.
• CLI/API consistency docs: how synix list/show/search relate to HEAD vs build/ now. Right now snapshots exist, but reads still appear to be from mutable surfaces.
• Migration tests: handling existing projects with only build/ state; behavior when .synix exists but is corrupt/partial.
• Negative tests: interrupted commit mid-ref update (you added journal recovery) but no test for partial object writes / power loss around _atomic_write_bytes.

Verdict

Block — the snapshot substrate is a one-way door, but the current shape (label-keyed manifest + forced text-only artifacts + blob schema confusion) is too likely to ossify into an unfixable format/API mistake. Fix those before this becomes “the way Synix works.”

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,024 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:44:45Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots to Synix: a content-addressed object store (.synix/objects/), git-like refs and HEAD, manifest/snapshot objects, and a build transaction that captures artifact state during the run. It also includes an RFC document laying out the full vision for snapshot/release/revert semantics, of which this PR implements the first slice.

Alignment

Strong fit. DESIGN.md specifies artifacts are content-addressed, provenance chains must be complete, and cache keys capture all inputs. This PR makes those properties durable and immutable rather than relying on mutable build/ files. The manifest-as-closure concept directly implements the "materialization keys" vision. The separation of build from release aligns with the "architecture evolution" thesis — you can now diff and revert without replay. The RFC's projection lifecycle model (builder vs. adapter) correctly defers complexity while leaving clean extension points.

Observations

1. [positive] commit_build_snapshot validates HEAD hasn't advanced via optimistic concurrency check, with a file lock and write-ahead journal for crash recovery. This is a serious implementation for a pre-1.0 project and correctly prevents silent history corruption.

2. [positive] assert_complete() on BuildTransaction is a fail-closed design — if any artifact was missed during the run, the snapshot is rejected. This prevents partial snapshots from becoming canonical.

3. [positive] Test coverage is thorough: unit tests for object store, refs, and ref cycles; integration tests for concurrent HEAD advancement, content hash mismatches, empty content, closure validation, pending journal recovery, pipeline fingerprint stability, validation-failure blocking; and an e2e test for the full CLI flow including synix clean preserving history.

4. [concern] _snapshot_parent_labels falls back to provenance.get_parents() then to [inp.label for inp in inputs]. The third fallback assigns all layer inputs as parents for any artifact, which could create false provenance edges in N:1 transforms where only a subset of inputs are actual parents. This is a correctness risk for lineage queries.

5. [concern] _get_parent_labels now returns [] when multiple inputs share the same artifact_id (the len(labels) > 1 branch). This silently drops provenance for legitimate content-duplicate artifacts. The caller _snapshot_parent_labels then falls through to the broad fallback. Consider logging a warning or using a different disambiguation strategy.

6. [question] The on_batch_complete and on_cached callback signatures changed from (artifacts) to (artifacts, unit_inputs). Are there any external callers or custom transforms that supply these callbacks? If so, this is a breaking change to the extension model.

7. [concern] Artifact.__post_init__ now raises TypeError if content is not a string. This is a behavioral change that could break any code path that constructs Artifacts with None content (e.g., lazy-loading patterns or placeholder artifacts). The old code silently allowed it. The save_artifact guard if not artifact.artifact_id and artifact.content was also changed to drop the and artifact.content check — consistent with the new invariant but worth noting as a coupled change.

8. [nit] _normalize_fingerprint_value handles Path, callables, datetime-like objects, sets, and more — good defensive coding. However, set iteration order is non-deterministic in Python, so fingerprints containing sets won't be stable across runs.

9. [nit] put_text chunks at 64KB through an incremental encoder and writes to a temp file in objects_dir root rather than the target prefix directory. For very large text artifacts, temp files land in a different directory than final placement, which is fine but differs from put_bytes/put_file which use path.parent.

10. [positive] The RFC document is well-structured, clearly separates first-slice scope from future work, and explicitly calls out what is not in scope. The test strategy section in the RFC maps directly to the tests actually written in this PR.

11. [question] Run IDs changed from second to microsecond precision (%Y%m%dT%H%M%S%fZ). This fixes collisions but makes ref paths like refs/runs/20260306T082007123456Z — is this the intended long-term format, or will it be normalized before v1.0?

Verdict

This is a well-executed first slice of a critical architectural feature — it makes Synix's immutability guarantees real rather than aspirational, with serious attention to crash safety, concurrency, and test coverage. The provenance fallback logic deserves a closer look before merge.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,024 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:44:56Z

[github-actions]

Note

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

Threat assessment — Medium-high risk: you’re introducing a new canonical persistence model (.synix snapshots/refs) that will rapidly become load-bearing, but the current slice is only partially integrated and has a couple of correctness and operability traps.

One-way doors

1. .synix on-disk object/refs format (schema_version=1)

   • Why hard to reverse: Once users accumulate history, any change requires migration tooling or compatibility readers forever.
   • Safe to merge if: you commit to (a) formal spec + migration policy, (b) upgrade tests across schema versions, and (c) a “no silent fork” story for ambiguous stores across upgrades (you started this).

2. Run ref naming + JSON output schema

   • Why hard to reverse: refs/runs/<run_id> and {schema_version:1,runs:[...]} will get scripted against immediately.
   • Safe to merge if: you document stable ordering, field guarantees, and deprecation policy; also ensure run_id format is explicitly part of the contract (or make it clearly opaque everywhere).

3. Pipeline API adds Pipeline.synix_dir

   • Why hard to reverse: becomes part of the public configuration surface and will appear in templates/tutorials.
   • Safe to merge if: you define precedence rules and defaults in docs (including multi-pipeline repos), and test relative/absolute path behavior.

Findings

1. src/synix/build/artifacts.py:save_artifact removed and artifact.content guard

   • Risk/failure mode: now computes artifact.content.encode() unconditionally when artifact_id missing; if any code path creates an Artifact with content=None (previously tolerated), this becomes an AttributeError. You also changed Artifact.__post_init__ to forbid non-str content, but that’s a broader behavioral change and may break callers.
   • Severity: [warning]

2. src/synix/core/models.py:Artifact.__post_init__ enforces content is str

   • Risk/failure mode: This is an API-breaking constraint. Docs/design talk about “text/data”; code now rejects JSON/dicts/binary. That will block projections or structured artifacts later, and will surprise existing users/tests.
   • Severity: [warning]

3. src/synix/build/snapshots.py:_pipeline_fingerprint includes layer.config and projection config via repr() fallback

   • Risk/failure mode: fingerprint instability across Python versions / repr differences / insertion ordering in nested structures you didn’t normalize (you normalize dict keys, but repr(obj) is a footgun). Also includes callables only by module+qualname (not source hash), so prompt changes in code may not change pipeline_fingerprint even though behavior changed.
   • Severity: [warning]

4. src/synix/build/snapshots.py:commit_build_snapshot advances refs only when validators pass

   • Risk/failure mode: You silently drop historical “failed build attempts” from immutable history. That may be intended, but it conflicts with “immutable build snapshots” language and makes debugging/repro of failures harder.
   • Severity: [minor]

5. src/synix/build/refs.py:RefStore.write_ref is not atomic w.r.t concurrent writers

   • Risk/failure mode: atomic_write makes the file write atomic, but there’s no compare-and-swap. Two builds can race and last writer wins; your commit path mitigates by checking HEAD didn’t move, but direct write_ref is still used elsewhere and is an attractive future footgun.
   • Severity: [minor]

6. src/synix/build/object_store.py:get_bytes/get_json no explicit corruption handling

   • Risk/failure mode: a truncated object file yields JSON decode errors without context; no checksum verification on read (you trust path=hash). At minimum, error messages should include oid/path to debug disk corruption.
   • Severity: [minor]

7. CLI/doc mismatch: README says uvx synix runs list but CLI command wired under main includes runs group; build command prints Snapshot only when not demo-mode

   • Risk/failure mode: Users follow README, don’t see snapshot info in demo-mode / or when validation fails, and think feature is broken.
   • Severity: [minor]

Missing

• Docs updates beyond README: entity model / CLI UX docs should mention .synix, clean semantics, and that projections aren’t snapshotted yet (critical caveat).
• Explicit migration/upgrade story: what happens if .synix exists with future schema_version; you validate “newer schema” on read but provide no CLI to report/upgrade.
• Tests for multi-process concurrency: you added a lock, but no test that two concurrent builds don’t corrupt refs/object store or deadlock.
• Ref resolution security: no guardrails against refs pointing outside .synix via crafted paths (regex helps) but no explicit test for path traversal attempts.
• Performance/scale test: list_runs loads every snapshot JSON; at 10k runs this becomes slow.

Verdict

Ship with fixes. The direction is correct, but you’re locking in a persistence/identity model while still leaking instability (pipeline fingerprinting) and making a likely-too-strict “artifact content must be str” API bet without aligning docs/design.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,085 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:47:10Z

[github-actions]

Note

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

Summary

This PR introduces an immutable snapshot model for Synix builds: a .synix/ content-addressed object store, git-like refs and HEAD, manifest/snapshot objects, a synix runs list CLI command, and an RFC documenting the full future design. Builds now record canonical immutable state alongside the existing mutable build/ directory.

Alignment

Strong fit. DESIGN.md specifies artifacts as content-addressed (content_fingerprint = SHA-256), provenance chains as complete, and cache keys capturing all inputs. This PR makes those properties durable across builds rather than just within one. The immutable snapshot model directly enables the "architecture migration without data loss" vision — you can now revert to a known-good build state. The RFC's separation of build vs. release echoes DESIGN.md's distinction between records (build data) and artifacts (queryable surfaces). The synix_dir sitting outside build/ means synix clean preserves history, consistent with "evolve without data loss."

Observations

1. [positive] object_store.py — atomic writes, canonical JSON serialization, schema validation with versioning, and allow_older_schema for forward compatibility. This is the right foundation for a pre-1.0 format.

2. [positive] snapshots.py assert_complete() — fail-closed verification that the transaction captured every artifact the build produced. This prevents silent snapshot corruption.

3. [positive] Concurrent-writer detection in commit_build_snapshot — checks both HEAD target ref and HEAD oid haven't changed. The ref-update journal provides crash recovery. Well-thought-out for a first implementation.

4. [concern] _snapshot_parent_labels in runner.py has a three-tier fallback (derive from inputs → stored provenance → all inputs). The final fallback [inp.label for inp in inputs] could produce incorrect lineage for transforms where only a subset of inputs are actual parents. This is the same fallback _get_parent_labels already uses, but now it's persisted immutably — incorrect lineage becomes permanent.

5. [concern] _get_parent_labels now returns [] when multiple inputs share the same artifact_id (hash collision on content). This silently drops lineage rather than recording the ambiguity. For content-addressed builds this could happen with duplicate source conversations. The empty list then triggers the _snapshot_parent_labels fallback, potentially recording wrong parents.

6. [question] runner.py — _on_batch_complete and _on_cached callback signatures changed from (artifacts) to (artifacts, unit_inputs). Are all external callers of _execute_transform_concurrent updated? The concurrent executor passes these correctly, but if batch-build code also calls these paths, it could break.

7. [concern] Artifact.__post_init__ now raises TypeError on non-string content. This is correct for snapshots but is a breaking change — any existing custom transform returning None content will crash. The artifacts.py change removing the and artifact.content guard is consistent but could surface errors in user pipelines that previously worked silently.

8. [nit] _sanitize_llm_config redacts keys ending with _token — this would strip max_tokens, a non-secret field. The suffix check lower.endswith(("_api_key", "_secret", "_password", "_token")) is overly broad. Test test_pipeline_fingerprint_keeps_non_secret_llm_fields doesn't catch this because it uses max_tokens as a top-level key that doesn't match the suffix patterns — but auth_token vs completion_token would be ambiguous.

9. [positive] Test coverage is excellent — unit tests for object store, refs, and logging; integration tests for snapshot commit, concurrent writers, content mismatch, empty content, closure verification, journal recovery, fingerprinting; e2e tests for CLI flow and clean-preserves-history. Failure modes are explicitly tested.

10. [nit] snapshots.py _snapshot_lock — the Windows msvcrt path writes a byte then locks byte 0. This works but the pattern is fragile if the file already has content from a prior incomplete lock cycle.

11. [question] The RFC references LENS multiple times ("required by LENS") but LENS isn't defined anywhere in the provided documentation. This creates an opaque dependency for reviewers.

Verdict

This is a well-executed foundational PR that correctly separates immutable build history from the mutable working surface, with strong test coverage and a clear incremental migration path — a good step toward the project's architecture-evolution goals.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,085 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:47:15Z

[github-actions]

Note

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

Threat assessment — High risk: this PR introduces a new on-disk canonical store + ref semantics that users will depend on, but it’s only partially integrated and has several correctness/operational gaps.

One-way doors

1. .synix object store + schema_version=1 format

   • Why hard to reverse: Once users accumulate history, any incompatibility or relocation requires migration tooling and backward readers.
   • Safe to merge when: you commit to a compatibility policy (reader supports N prior schema versions), publish a formal spec (not just RFC), and add explicit migration/upgrade command(s) or auto-migrator with tests.

2. Git-like refs (HEAD, refs/heads/main, refs/runs/<run_id>)

   • Why hard to reverse: Users will script against ref layout and meanings; ref resolution rules become de facto API.
   • Safe to merge when: you lock down ref naming rules, document lifecycle (when refs advance, failure behavior), and provide stable machine-readable output everywhere (not just runs list).

3. Changing “source of truth” away from build/

   • Why hard to reverse: Tooling, docs, templates, and user mental model shift; “clean doesn’t delete history” becomes relied upon.
   • Safe to merge when: all read paths (list/show/search/lineage) can target snapshots/refs consistently, or you clearly gate this behind an opt-in flag/env until the rest of CLI is snapshot-aware.

Findings

1. src/synix/build/snapshots.py::_pipeline_fingerprint includes callables by module.qualname

   • Risk: violates the project’s own cache semantics (“transform source code affects rebuild”). Renaming a function or moving modules changes fingerprint without logic change; worse, changing function body without renaming won’t change fingerprint. This makes snapshot identity misleading and breaks reproducibility/diff intent.
   • Severity: [critical]

2. src/synix/build/runner.py snapshotting records artifacts but manifests omit projections (projections={})

   • Risk: You’re creating “immutable build snapshots” that are not actually build closures (search index / flat files aren’t captured). Users will treat snapshots as rollback points; they aren’t. README text tries to caveat this, but the CLI prints “Snapshot:” without warning.
   • Severity: [critical]

3. src/synix/build/refs.py::synix_dir_for_build_dir ambiguous legacy/nested logic

   • Risk: Default write path is <build parent>/.synix, but read-side “also honor nested build/.synix”. This is a footgun: a stray nested directory changes where history is read from. You do fail when both exist, but the “either one exists” branch silently switches.
   • Severity: [warning]

4. src/synix/build/object_store.py object validation is shallow/incomplete

   • Risk: Required fields only checked for presence + basic type; no validation of inner structures (e.g., manifest artifacts entries), no validation that oids referenced exist, no max-size guards. Corrupt objects become load-bearing forever.
   • Severity: [warning]

5. src/synix/build/snapshots.py::recover_pending_ref_updates is non-atomic and unsafe under concurrency

   • Risk: Two processes can both “recover” and both apply journals; no lock is held here. You do lock in commit_build_snapshot, but list_runs() calls recovery without lock. This can race and/or partially apply.
   • Severity: [warning]

6. src/synix/core/logging.py run_id format changed

   • Risk: anything parsing the old %Y%m%dT%H%M%SZ run IDs breaks. Also refs/runs/<run_id> now includes random suffix; ordering is “roughly sortable” but not strictly lexicographic by time if clocks skew.
   • Severity: [warning]

7. src/synix/build/artifacts.py now forces artifact.content to be str

   • Risk: This is an API tightening. If any existing transforms produce structured content (dict) and relied on JSON serialization later, they now hard-fail. Might be correct, but it’s a breaking behavioral change that isn’t called out.
   • Severity: [warning]

Missing

• A clear, enforced compatibility mode / feature flag. Right now snapshots are “canonical” but most CLI still operates on mutable build/. That split-brain needs gating or full integration.
• Tests for concurrent builds writing to the same .synix (lock coverage, ref journal correctness, list_runs race).
• Docs updates beyond README: docs/entity-model.md, CLI docs, cache semantics docs should reflect new canonical storage and identity model (README claims this is new in v0.15.x; where’s the definitive spec?).
• A plan/command for garbage collection: object store is append-only; no pruning strategy means unbounded disk growth.

Verdict

Block — this is a platform-level one-way door shipped half-wired: snapshot identity is not trustworthy (fingerprint), snapshots aren’t real closures (projections omitted), and ref recovery/concurrency is shaky. Fix those before merging.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,175 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:52:12Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots: a content-addressed object store (.synix/objects/), git-like refs and HEAD, manifest/snapshot objects, and a BuildTransaction that captures artifact state during pipeline runs. It also adds synix runs list CLI commands and an RFC document describing the full vision including future release/revert semantics.

Alignment

Strong fit. DESIGN.md defines artifacts as content-addressed with materialization keys, provenance as complete chains, and the build-system analogy as foundational. This PR operationalizes "artifacts are immutable and content-addressed" at the storage layer — blobs get SHA-256 oids, snapshots form a parent-linked chain, and manifests capture the full artifact closure. The separation of build from release aligns with DESIGN.md's distinction between steps (processing) and artifacts (queryable targets). The synix clean behavior — removing mutable build/ but preserving .synix/ — directly supports "architecture evolution without data loss."

Observations

1. [positive] BuildTransaction.assert_complete() fails closed on missing artifacts. This is exactly the right contract for a system that values provenance completeness.

2. [positive] _snapshot_lock handles both fcntl and msvcrt for cross-platform file locking, and the ref-update journal provides crash recovery. The commit protocol (journal → write refs → delete journal, with recover_pending_ref_updates on read paths) is well-thought-out.

3. [concern] _snapshot_parent_labels returns [] when input_ids exist but can't be resolved unambiguously, while _provenance_parent_labels falls back to all inputs. This divergence means snapshot provenance can be strictly less informative than legacy provenance for aggregate transforms. The RFC doesn't discuss this tradeoff — it could silently degrade lineage quality for the canonical store.

4. [concern] _get_parent_labels now returns [] when any input_id maps to multiple labels (hash collision across inputs). This is a behavioral change to the existing provenance system that could break lineage for pipelines with content-duplicate artifacts. The change is untested for that specific scenario.

5. [question] on_complete and on_cached callback signatures changed from (list[Artifact]) to (list[Artifact], list[Artifact]). Is every call site updated? The concurrent runner test covers on_complete, but are there external/custom transforms that might pass callbacks with the old signature?

6. [positive] Pipeline fingerprint explicitly strips secrets (api_key, _token, etc.) and ignores local paths (source_dir, build_dir). Tests verify both inclusion and exclusion semantics.

7. [concern] put_text writes to a temp file in self.objects_dir (the root), then moves to the fan-out path. For large text, this could fail if the objects dir is on a different filesystem than the temp dir. put_bytes and put_file correctly use path.parent as the temp dir. Inconsistent.

8. [nit] _normalize_fingerprint_value handles set by sorting JSON-serialized elements — works but is O(n log n) with a JSON dump per element. Fine at current scale but worth a comment.

9. [positive] Test coverage is thorough: unit tests for object store, refs, and run ID generation; integration tests for concurrent-head rejection, content-hash mismatch, empty content, journal recovery, closure verification, validation-blocks-snapshot, and successive builds; e2e test covers full CLI workflow including clean preserving history.

10. [question] The RFC document is 881 lines but the implementation is explicitly "slice 1" — should the RFC live in docs/ permanently, or is it a tracking document that should reference the issue?

11. [nit] run_id format in refs (refs/runs/20260306T082007123456Z-1f2e3d4c) embeds microseconds + random hex. The ref regex _REF_RE allows dots and hyphens, which accommodates this, but the README note says "opaque, time-prefixed ids" — the format is actually quite structured and users will depend on it.

Verdict

This is a well-executed foundational change that brings Synix's storage model in line with its build-system identity; the temp-file directory inconsistency in put_text and the silent provenance degradation for ambiguous input mappings should be addressed before merge.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,175 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:52:19Z

[github-actions]

Note

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

Threat assessment — High risk: this PR introduces a new canonical on-disk state model (.synix snapshots/refs) that will become user data and is difficult to change safely later.

One-way doors

1. .synix on-disk object/refs layout and schemas
   Hard to reverse because users will accumulate histories that future versions must read; any bug becomes permanent debt.
   Safe to merge only if: you commit to migration tooling/version negotiation now (at least “read old, write new” rules), and you document durability guarantees (what can change pre-1.0, what cannot).

2. Ref naming semantics (HEAD, refs/heads/main, refs/runs/<run_id>)
   Users/scripts will build around these names; changing later breaks automation.
   Safe only if: you freeze ref naming patterns (or clearly mark them experimental in CLI help, not just README), and you provide stable JSON output as the primary interface.

3. Run ID format becomes externally visible (YYYYMMDDT...Z-<hex>)
   It’s printed and used in ref paths; changing format breaks parsing and file layouts.
   Safe only if: you treat run_id as opaque everywhere (no ordering assumptions), and you don’t encode it into storage paths without an indirection (or you guarantee format stability).

4. Behavioral contract change: synix clean no longer deletes “source of truth”
   This will surprise existing users and will be depended on once documented.
   Safe only if: the CLI makes this unmissable (prompt text, docs, maybe --purge-history), and clean’s behavior is consistent across versions.

Findings

1. src/synix/build/artifacts.py: artifact_id computation now ignores falsy content
   Failure mode: previously content="" would skip hashing; now it hashes. That changes cache/provenance identity for empty artifacts and could create “new” artifacts unexpectedly.
   Severity: [warning]

2. src/synix/build/snapshots.py / object_store.py: object store uses plain SHA256 hex as oid without algorithm prefix
   Failure mode: you’ve created two ID schemes (artifact_id is sha256:<hex>, oid is <hex>). This will leak everywhere and complicate future multi-hash support/migrations.
   Severity: [warning]

3. src/synix/build/snapshots.py: pipeline fingerprint includes layer.config and proj.config via _normalize_fingerprint_value(repr(value)) fallback
   Failure mode: non-deterministic repr() (addresses, ordering, object-specific repr) will spuriously change pipeline_fingerprint between runs, defeating “immutable history” usefulness and making diffs noisy.
   Severity: [critical]

4. src/synix/build/snapshots.py: snapshots capture artifacts only; projections omitted but still built into mutable build/
   Failure mode: “immutable build snapshot” is not actually reproducible for user-facing outputs (search index, flat file). Users will assume rollback works, but it won’t once projections diverge.
   Severity: [warning]

5. src/synix/build/refs.py + snapshots.py: locking is local-file based and per .synix dir, but build still writes build/ concurrently without coordination
   Failure mode: two concurrent builds can race writing mutable build/ outputs while only one can commit refs; you’ll get inconsistent build/ surface that doesn’t match HEAD.
   Severity: [critical]

6. src/synix/build/snapshots.py: recover_pending_ref_updates replays any JSON under ref_journal/ without schema/type validation beyond updates being a dict
   Failure mode: corrupted/malicious journal can rewrite arbitrary refs; also partial writes could brick state.
   Severity: [warning]

7. src/synix/cli/runs_commands.py: --json output uses console.print_json(json.dumps(...))
   Failure mode: double-encoding; print_json expects an object or JSON string depending on Rich version—this is brittle. Also schema guarantees are only in README, not command help/output.
   Severity: [minor]

Missing

• A clear migration story for existing projects with build/.synix vs sibling .synix beyond “error if both exist”. Needs a command (synix snapshots migrate) or documented manual steps.
• Tests for concurrent builds producing consistent build/ outputs and ref advancement rules (you test ref advancement failure, not surface consistency).
• Documentation updates beyond README/DESIGN: CLI help text for clean, and a dedicated doc page for snapshot semantics (what’s durable, what’s experimental).
• A garbage-collection or retention policy for .synix/objects (unbounded growth is guaranteed).

Verdict

Block. The snapshot/ref model is a major one-way door, but the fingerprint determinism and concurrent build surface consistency problems make the current implementation unsafe to ship as “canonical history.”

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,201 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:53:50Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots: a content-addressed object store (.synix/objects/), git-like refs and HEAD, manifest/snapshot objects, a build transaction that accumulates artifact state during pipeline runs, and a synix runs list CLI command. It also includes the full RFC document motivating the design. The mutable build/ directory is retained as a compatibility surface while .synix/ becomes the canonical history.

Alignment

Strong fit. DESIGN.md §3.3 establishes content-addressed fingerprinting and materialization keys as core. This PR extends that principle to build outputs themselves — artifacts are now immutable, content-addressed objects with schema-versioned metadata. The snapshot/manifest/ref model directly supports the "architecture evolution" thesis (Hypothesis 3): you can now trace, compare, and eventually revert between different build states without losing history. The separation of build and release aligns with DESIGN.md's distinction between Records (build data) and Artifacts (queryable surfaces). synix clean preserving snapshot history is exactly right — it mirrors make clean not deleting your git history.

Observations

1. [positive] BuildTransaction.assert_complete() is a fail-closed integrity check that prevents partial snapshots. This is the right call for a system where provenance completeness must be 100% (DESIGN.md §9.5).

2. [positive] The _snapshot_lock with journal-based crash recovery (recover_pending_ref_updates) handles the interrupted-commit case correctly. The test test_runs_list_recovers_pending_ref_updates exercises this path explicitly.

3. [positive] Pipeline fingerprinting strips API keys via _sanitize_llm_config while preserving model/temperature/max_tokens. Well-tested with both positive and negative fingerprint assertions.

4. [concern] _snapshot_parent_labels returns [] when input_ids exist but can't resolve unambiguously, while _provenance_parent_labels falls back to all inputs. This divergence is intentional per the docstrings, but the empty-list case means snapshot artifacts can have no parent linkage at all — weakening provenance completeness for the canonical store. Worth a follow-up to surface these as warnings.

5. [concern] put_text writes to a temp file in self.objects_dir root before knowing the oid, then moves to the fan-out path. If two concurrent processes call put_text simultaneously, temp files accumulate in the objects root. The temp suffix .tmp avoids collision via mkstemp, but orphaned temps from crashes are never cleaned.

6. [question] The _on_batch_complete and _on_cached callback signatures changed from (artifacts) to (artifacts, unit_inputs). Is this a breaking change for any external code that supplies these callbacks, or are they purely internal? The concurrent runner test covers this, but the contract change is worth noting.

7. [concern] commit_build_snapshot does an optimistic concurrency check comparing parent_snapshot_oid, but the lock is process-local (fcntl.flock). If two synix build processes target the same .synix/ from different working directories, the check is correct but the error message ("rerun against the latest snapshot") doesn't explain recovery. Consider documenting this.

8. [nit] _FIELD_TYPES for release_receipt is missing — validation only checks required fields exist but not their types. Inconsistent with other object types.

9. [positive] Test coverage is thorough: unit tests for object store, refs, and run ID generation; integration tests for transaction integrity, concurrent HEAD advancement, content mismatch rejection, empty content, closure completeness, validation failure blocking snapshots; and a full e2e test covering build→build→clean→verify-history-survives.

10. [nit] run_id format in refs (refs/runs/20260306T082007123456Z-1f2e3d4c) embeds microseconds + random hex. The RFC examples use a different format (refs/runs/2026-03-06T12-30-11Z). Minor, but the README note about the format being experimental is appropriate.

Verdict

This is a well-executed first slice of a foundational capability — it advances the core vision of immutable, content-addressed build history without breaking existing workflows, and the test coverage matches the criticality of the change.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,201 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:54:01Z

[github-actions]

Note

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

Threat assessment — High risk: this PR introduces a new on-disk canonical store (.synix/) + refs/HEAD semantics that users and future tooling will immediately depend on, but the lifecycle, migration, and concurrency edges aren’t fully nailed down.

One-way doors

1. .synix/ as canonical history store (layout + object schemas)
   Hard to reverse because it becomes persisted user data and de facto API for tools (“immutable snapshots”, “refs”, “objects”, schema_version).
   Safe to merge only if: explicit migration story exists (including detection/repair), schema evolution rules are documented and enforced, and there’s a compatibility contract for older stores.

2. Git-like refs + HEAD semantics (refs/heads/main, refs/runs/<run_id>)
   Hard to reverse because CLI and automation will bake in ref names and meaning.
   Safe to merge only if: ref namespace is finalized (or clearly namespaced “experimental”), and commands that read artifacts/search can target refs consistently (today they still read build/, creating a split-brain mental model).

3. Run ID format becomes user-visible and persisted in ref paths (YYYY...Z-<hex>)
   Hard to reverse because it becomes part of filenames, scripts, and expectations about sortability.
   Safe to merge only if: stated stability guarantees exist (or it’s kept entirely internal behind an abstraction), and collisions/ordering semantics are explicitly defined.

Findings

1. src/synix/build/snapshots.py — snapshot object store mixes “artifact_id” and “oid” identity without a stable contract
   Failure mode: object references and provenance can’t be reasoned about long-term; future refactors will break assumptions (artifact_id is sha256:text; oid is sha256:canonical-json-bytes; content_oid is sha256:encoded-bytes). This is already a partial divergence from the RFC’s intent (“don’t key object store by artifact_id”), yet artifact objects embed artifact_id anyway.
   Severity: [warning]

2. commit_build_snapshot() — manifest captures all artifacts by label, no scoping by “final build closure” vs intermediate/cached ambiguity
   Failure mode: label collisions across layers or rebuilds could silently redefine what a label means historically. Your transaction keys by artifact.label only, not (layer, label); if labels are not globally unique, history corrupts. The code assumes global uniqueness but doesn’t validate it.
   Severity: [critical]

3. BuildTransaction.record_artifact() — mutates artifact.artifact_id during snapshotting
   Failure mode: snapshotting is no longer observational; it changes in-memory objects that later code might use for caching/provenance decisions. That’s spooky action at a distance.
   Severity: [warning]

4. src/synix/build/runner.py — parent label derivation is inconsistent and can intentionally drop parents to [] (_snapshot_parent_labels)
   Failure mode: snapshots can lose provenance edges (“artifact.input_ids present but ambiguous => []”), making lineage incomplete precisely when ambiguity exists. That’s not “fail closed”; it’s “store less data”. If ambiguity is possible, you need a deterministic tie-break or store multiple parents.
   Severity: [warning]

5. src/synix/build/refs.py — synix_dir_for_build_dir doc vs behavior mismatch
   You call the sibling store legacy and nested store nested, but the doc says sibling is “persistent” and nested is “compat”. Naming aside, behavior is: default returns sibling path even if it doesn’t exist. That’s fine, but “legacy” is misleading and will cause future migration bugs.
   Severity: [minor]

6. ObjectStore.put_text() writes temp files into objects_dir root, then moves into a prefix subdir
   Failure mode: cross-device rename issues if objects_dir is on a different filesystem than objects_dir/prefix (unlikely but possible with symlinks/mounts). Safer to create temp in the target directory. put_bytes does that; put_text does not.
   Severity: [warning]

7. ObjectStore.get_json() allows older schemas but no forward-compatible handling beyond a version check
   Failure mode: you’ll accept older schema versions but don’t normalize them; callers will assume fields exist. This will break as soon as you bump schemas.
   Severity: [warning]

8. CLI surface: synix runs list --json declares a versioned JSON shape, but the run objects inside are not versioned/stable
   Failure mode: consumers rely on fields that will churn; you’ve only versioned the wrapper, not the items.
   Severity: [minor]

Missing

• Documentation updates beyond README blurb: docs/entity-model.md, docs/cache-semantics.md, and CLI UX docs should reflect that build/ is no longer source of truth.
• Validation of label uniqueness across pipeline (or change manifest keying to include layer).
• Tests for Windows + concurrent writers behavior (lock + ref journals + atomic_write semantics).
• Migration/repair command (synix doctor or similar) for ambiguous .synix placement and journal recovery.
• Any integration path for “show/search/list/lineage by ref”; currently you’ve created a canonical history that most commands ignore.

Verdict

Block — this is foundational storage + identity + history semantics with at least one critical correctness hole (label uniqueness/keying) and several durability/compatibility gaps; fix those before shipping a new canonical store.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,236 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:55:35Z

[github-actions]

Note

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

Summary

This PR introduces immutable build snapshots — a content-addressed object store (.synix/objects), git-like refs and HEAD, manifest/snapshot objects, and a build transaction that captures artifact state during pipeline runs. It also adds an RFC document laying out the full vision for snapshot/release/revert semantics, though only the artifact-snapshot slice ships here.

Alignment

Strong fit. DESIGN.md's core principles — artifacts are immutable and content-addressed, provenance chains are complete, cache keys capture all inputs — are directly advanced. The snapshot model makes immutability a first-class storage property rather than just a logical convention. The build/ vs .synix/ split maps cleanly to the "audit determinism" bet: you can now explain what every build produced, survive synix clean, and diff between runs. The RFC's projection/release separation respects DESIGN.md's distinction between artifacts (build data) and projections (queryable surfaces). The explicit deferral of projection snapshotting is honest scoping.

Observations

1. [positive] object_store.py — atomic writes, canonical JSON serialization, schema validation with version gating, and fan-out directory structure are all solid. The put_text streaming encoder avoids double-buffering.

2. [positive] snapshots.py — BuildTransaction.assert_complete() is a fail-closed integrity check that prevents partial snapshots. The ref-update journal with crash recovery (recover_pending_ref_updates) is a thoughtful durability mechanism.

3. [positive] Test coverage is thorough: unit tests for object store, refs, and run ID generation; integration tests for concurrent HEAD advancement, artifact-id/content mismatches, empty content, closure completeness, pending journal recovery, fingerprint stability, and validation-gated snapshots; e2e tests for the full CLI flow including clean preserving history.

4. [concern] _snapshot_lock uses fcntl/msvcrt file locking, but the msvcrt branch writes a dummy byte and locks byte 0. If two processes open the lock file concurrently on Windows before either writes, both could read empty and both write — there's a narrow TOCTOU window. Low risk for now but worth a comment.

5. [concern] runner.py — the _save_artifact closure now takes a parent_inputs kwarg, but the closure already captures _inputs as a default. The interaction between effective_inputs = parent_inputs if parent_inputs is not None else _inputs and the outer inputs variable across loop iterations could surprise future contributors. The closure captures _inputs at definition time via default arg, but parent_inputs is passed at call time — this is correct but fragile.

6. [question] _on_batch_complete and _on_cached callback signatures changed from (list[Artifact]) to (list[Artifact], list[Artifact]). Are all external callers of _execute_transform_concurrent updated? The concurrent runner test is updated, but any out-of-tree transform that passes custom callbacks would break silently.

7. [concern] put_file reads the source file twice — once for hashing, once for copying. For large source files this doubles I/O. A single-pass hash-while-copying approach would be better at scale.

8. [nit] snapshots.py imports atomic_write from synix.core.errors — an odd module name for a utility function. Not introduced by this PR but worth noting.

9. [positive] Pipeline fingerprinting explicitly strips API keys and secrets while preserving model config — tested with dedicated assertions. The callable source-hashing via inspect.getsource aligns with DESIGN.md's prompt identity model.

10. [question] The _get_parent_labels change returns [] when multiple inputs share the same artifact_id. This is a correctness fix for ambiguous provenance, but it means snapshot parent_labels silently drops to empty for deduped content. Is this the right default, or should it warn?

11. [positive] The RFC is well-structured and explicitly scopes what ships now vs. later, with clear non-goals. The test strategy section in the RFC maps 1:1 to actually-written tests.

Verdict

This is a well-executed first slice of a foundational capability — it advances immutability and provenance from logical conventions to durable storage primitives, with thorough tests and careful scoping.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,236 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:55:47Z

[github-actions]

Note

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

Threat assessment — High risk: this PR quietly introduces a new canonical persistence layer (.synix snapshots/refs) and starts writing it on every build, which is a foundational storage/API one-way door.

One-way doors

1. .synix/ as canonical history store + ref namespace (HEAD, refs/*)

   • Why hard to reverse: Once users have history in .synix, any change to layout, ref semantics, or resolution rules risks orphaning history or forking it (nested vs sibling .synix already acknowledged).
   • Safe to merge only if: you commit to a migration story (explicit, tested), document precedence rules, and have corruption/rollback handling. Also decide whether .synix is project local vs build-dir derived vs configured only—right now it’s all three.

2. New CLI surface: synix runs list --json with schema_version contract

   • Why hard to reverse: scripts will depend on JSON keys and the meaning of “runs”.
   • Safe to merge only if: JSON schema is documented in a stable place and has forward/backward compatibility guarantees (at minimum: additive-only + version bump).

3. Run ref naming = refs/runs/<run_id> with time-prefixed random IDs

   • Why hard to reverse: IDs leak into support/debugging and user workflows; changing format later breaks tooling and expectations.
   • Safe to merge only if: you either (a) treat as internal and do not print it prominently, or (b) commit to the format. README says “experimental” but the CLI prints it as primary output.

Findings

1. src/synix/build/object_store.py: schema validation is incomplete for some types

   • Failure mode: _REQUIRED_FIELDS includes release_receipt but _FIELD_TYPES doesn’t; so type validation is partial/inconsistent. Also no validation that content_oid, manifest_oid, etc are valid oids everywhere (only manifest.artifacts[*].oid checked).
   • Severity: [warning]

2. src/synix/build/snapshots.py: snapshot commit advances refs/heads/main even if pipeline name differs

   • Failure mode: .synix store is derived from build_dir sibling. Two different pipelines sharing the same build/ parent will share the same .synix and clobber each other’s HEAD / refs/heads/main. The manifest contains pipeline_name but refs are global within the store.
   • Severity: [critical]

3. src/synix/build/runner.py: snapshotting happens even when build is “cached” and provenance parents can be empty

   • Failure mode: _snapshot_parent_labels returns [] when artifact.input_ids exists but can’t be mapped uniquely. That creates canonical snapshot artifacts with missing lineage (worse than “best effort” provenance), violating “full provenance” claims.
   • Severity: [critical]

4. src/synix/build/runner.py: parent label derivation depends on content-hash uniqueness

   • Failure mode: _get_parent_labels now returns empty if two inputs share the same artifact_id (identical content). That’s common for duplicated boilerplate/empty strings. You’ll drop parents instead of recording multiple parents, again breaking lineage.
   • Severity: [critical]

5. src/synix/build/snapshots.py: pipeline fingerprint includes layer.config but not prompt identity unless embedded

   • Failure mode: _pipeline_fingerprint uses layer.config and class name, but prompt/template identity may live outside config in existing layer types. You risk snapshots claiming identical pipeline_fingerprint when prompts changed.
   • Severity: [warning]

6. src/synix/cli/runs_commands.py: --json output uses console.print_json(json.dumps(...))

   • Failure mode: print_json already JSON-encodes/pretty-prints; passing a string risks double-encoding or formatting quirks depending on rich version. Also click option --json shadows Python json module name (minor).
   • Severity: [minor]

7. Docs vs implementation mismatch: “content-addressed objects, manifests, snapshots, refs”

   • Failure mode: implementation snapshots artifacts only, projections ignored, but README/DESIGN addition risks users assuming search index/projections are reproducible/rollbackable via snapshots. That’s not true.
   • Severity: [warning]

Missing

• A hard guarantee/tests that multiple pipelines cannot share the same .synix unless explicitly configured (or namespaced refs by pipeline).
• Documentation for .synix discovery precedence (sibling vs nested) and explicit migration guidance.
• Tests that synix clean does not delete .synix under all layouts (nested .synix under build/ will get wiped if clean deletes build/ recursively).
• A command to resolve/show artifacts by --ref/snapshot (you introduced refs but no way to consume them besides listing).

Verdict

Block — the ref/store scoping and lineage loss behaviors are fundamental correctness issues; shipping them will create corrupted “canonical history” that you can’t reliably fix later without migrations and user pain.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,275 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T08:57:27Z

[github-actions]

Note

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

Summary

This PR introduces immutable snapshot infrastructure for Synix builds: a content-addressed object store (.synix/objects), git-like refs and HEAD, manifest/snapshot objects, a build transaction that captures artifact state during pipeline execution, a synix runs list CLI command, and an RFC document describing the full vision. The mutable build/ directory is retained as a compatibility surface while .synix/ becomes the canonical build history.

Alignment

Strong fit. DESIGN.md explicitly states artifacts are content-addressed, provenance chains must be complete, and cache keys capture all inputs. This PR operationalizes those principles at the storage layer — objects are immutable and keyed by SHA-256, manifests are closures over artifacts, and snapshots form a linked chain. The split between immutable history and mutable build surface directly supports the "architecture evolution" thesis: you can synix clean and rebuild without losing provenance. The RFC's ref model also lays groundwork for the branching story (DESIGN.md §4.5) without prematurely implementing it. The audit determinism over reproducibility bet is respected — snapshots capture what was built, not a guarantee it's reproducible.

Observations

1. [positive] BuildTransaction.assert_complete() is a fail-closed integrity check that prevents partial snapshots. This is exactly the right contract for a build system.

2. [positive] The ref update journal in snapshots.py with recover_pending_ref_updates handles crash recovery for interrupted commits — serious infrastructure thinking for a pre-1.0 project.

3. [positive] Test coverage is thorough: unit tests for object store, refs, and logging; integration tests for concurrent HEAD advancement, artifact ID mismatches, empty content, closure validation, ref recovery, pipeline fingerprinting, validation failures; e2e tests for the full CLI workflow including clean preserving history.

4. [concern] _snapshot_lock in snapshots.py uses fcntl/msvcrt file locking but the msvcrt path writes a dummy byte and locks just 1 byte. If the file is empty and two processes race, the read(1) == b"" check followed by write(b"0") is not atomic — both could see empty and write. The lock should still work (both would lock byte 0), but the logic is fragile. Consider always writing the sentinel before locking.

5. [concern] _get_parent_labels in runner.py now returns [] when multiple inputs share the same artifact_id. This silently drops provenance for content-duplicate artifacts (e.g., identical summaries from different conversations). The fallback to [inp.label for inp in inputs] is also lost. The snapshot path (_snapshot_parent_labels) then falls through to [] as well. This could create orphan artifacts in the provenance DAG.

6. [question] put_text writes to a tempfile in self.objects_dir (the root), but the final destination is in a 2-char prefix subdirectory. If there are many concurrent put_text calls, tempfiles accumulate at the root level. put_bytes and put_file correctly use path.parent as the tempdir. Is this intentional for the streaming encoder case, or a bug?

7. [concern] _on_batch_complete and _on_cached callback signatures changed from (artifacts) to (artifacts, unit_inputs). Any external code calling _execute_transform_concurrent with callbacks will break. The test in test_concurrent_runner.py covers this, but the function is not prefixed with _ in all call sites — verify no external consumers exist.

8. [nit] The run_id format uses microsecond-precision timestamps plus 4 random bytes (secrets.token_hex(4)). The regex in the test validates \d{6} for the microsecond portion — this is correct but the _generate_run_id format should be documented since refs embed it in their path.

9. [positive] Pipeline fingerprinting correctly strips API keys and secrets via _sanitize_llm_config, with tests verifying both exclusion and inclusion behavior. The callable source-hashing is a nice touch for detecting prompt function changes.

10. [question] iter_entries() on ArtifactStore returns a shallow copy of the manifest dict — but the inner dicts are still shared references. If a caller mutates an entry, it corrupts the store's state. Should this be a deep copy, or is shallow sufficient given current usage?

11. [positive] Validation failures correctly prevent snapshot advancement (test_validation_failure_does_not_advance_snapshot_refs), maintaining the invariant that only clean builds become canonical.

Verdict

This is a well-designed, well-tested incremental step that establishes the immutable storage substrate the project needs — it advances the core vision without breaking existing behavior.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,275 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T08:57:37Z

[github-actions]

Note

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

Threat assessment — Medium-high risk: it introduces a new on-disk canonical history model (.synix objects/refs) and new CLI output contracts, with concurrency/atomicity edge cases that can brick or fork history if they go wrong.

One-way doors

1. .synix object store + schema_version=1 contract
   Hard to reverse because users will accumulate state under .synix/objects and refs; changing schemas later requires migration tooling and backwards readers.
   Safe to merge only if: you commit to a real migration/compat story (reader supports older versions, writer bumps versions intentionally), and you document the stability level + cleanup strategy.

2. Ref namespace + run ref format (refs/runs/<run_id>) and HEAD semantics
   Hard to reverse because tooling/scripts and mental model will anchor on git-like refs and specific ref paths.
   Safe to merge only if: you’re confident you want git semantics long-term, and you have a compatibility alias plan if ref layout changes.

3. CLI JSON output shape for runs list --json
   You explicitly recommend consumers depend on it. That’s an API.
   Safe to merge only if: you add a documented JSON schema (fields + meaning), and commit to version bump rules.

4. Default store placement (<build parent>/.synix)
   This affects multi-project layouts and cleanup expectations. Reversing later strands history.
   Safe to merge only if: docs cover multi-project collisions and you add guardrails (pipeline name scoping or per-project synix_dir).

Findings

1. src/synix/build/refs.py::synix_dir_for_build_dir store collision risk
   Failure mode: two different pipelines using build dirs under the same parent will share .synix, interleaving refs/histories. Nothing namespaces by pipeline.
   Severity: [critical]

2. src/synix/build/snapshots.py::_snapshot_lock weak cross-platform locking
   On Windows, msvcrt.locking(..., 1) locks only 1 byte, and the file is opened append+binary with ad-hoc sentinel write. This is easy to get wrong with multiple processes.
   Severity: [warning]

3. src/synix/build/snapshots.py::recover_pending_ref_updates can apply garbage journals
   It trusts any *.json in ref_journal/ with minimal validation (no type check, no schema_version bounds, no oid validation). A partial/corrupt file can crash listing or rewrite refs to nonsense.
   Severity: [critical]

4. src/synix/build/runner.py callback “arity introspection” is a brittle compat hack
   _invoke_callback uses inspect.signature and guesses based on positional params. This will behave weirdly with keyword-only args, partials, functools.wraps, C-callables. Also silently calls callback() when it sees 0 positional params, dropping data.
   Severity: [warning]

5. src/synix/build/snapshots.py::BuildTransaction.record_artifact mutates artifact.artifact_id
   Side effect: you’re mutating shared Artifact objects during snapshot capture; if artifacts are reused elsewhere (e.g., projections), you’ve now tightened coupling.
   Severity: [minor]

6. Artifact parent semantics split is inconsistent and may regress lineage (_snapshot_parent_labels vs _provenance_parent_labels)
   For snapshots you may return [] when artifact.input_ids exists but mapping is ambiguous. That creates a canonical snapshot with “no parents” even though provenance surface shows parents. That violates “full provenance” claims for the canonical store.
   Severity: [warning]

7. ObjectStore.get_bytes/get_json don’t validate oid format on read
   A caller typo becomes filesystem path traversal-like behavior within .synix/objects/<prefix>/...? You split by first 2 chars; malformed oid gives weird paths and confusing errors. Validate _OID_RE at API boundary.
   Severity: [minor]

Missing

• Tests for multi-pipeline collision: two pipelines with different names but same build_dir.parent should not share refs/history unless explicitly configured.
• Validation on ref journals: schema_version/type/oid/ref-name validation + corrupt journal handling.
• Docs update on synix clean and project layout beyond README blurb: what happens in monorepos, how to set pipeline.synix_dir.
• A stability policy for .synix schema evolution (writer bump rules, reader support window).
• A ref-scoping strategy (per-pipeline refs root, or include pipeline name in snapshot objects and enforce on commit).

Verdict

Block — the snapshot/refs substrate is directionally fine, but unscoped .synix storage and unvalidated ref journal recovery are footguns that will corrupt or cross-contaminate history in real projects. Fix those before shipping this as a new “source of truth.”

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,375 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-03-06T16:36:47Z

[github-actions]

Note

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

Summary

This PR introduces the first slice of an immutable snapshot model: a content-addressed object store (.synix/objects/), git-like refs with symbolic HEAD, build transactions that capture artifact state during pipeline runs, and a synix runs list CLI command. It also includes a detailed RFC document laying out the full vision for snapshots, releases, and projection adapters.

Alignment

Strong fit. DESIGN.md establishes that artifacts are content-addressed and provenance chains must be complete. This PR makes those properties structurally enforced rather than incidental — artifacts get verified content hashes on snapshot entry, and the manifest is a closure over exactly the artifacts produced. The split between immutable .synix/ and mutable build/ directly supports the "architecture evolution" thesis: synix clean removes the working surface without destroying history, enabling the experiment-and-iterate loop. The RFC's separation of build from release aligns with "audit determinism over reproducibility" — you can always inspect what was built at any ref. The BuildTransaction.assert_complete() check is a good fail-closed design that prevents partial snapshots.

Observations

1. [positive] object_store.py — Atomic writes, canonical JSON serialization, thorough field-level validation per object type, and the fan-out directory layout (aa/bbccdd...) are all solid. The put_text streaming encoder avoids double-buffering large content.

2. [positive] snapshots.py — The ref-update journal with crash recovery (recover_pending_ref_updates) is a thoughtful durability mechanism. The commit function's optimistic concurrency check (HEAD hasn't moved) is the right call for pre-1.0.

3. [concern] runner.py — The _invoke_callback introspection shim (lines 525–550) uses inspect.signature to dispatch callbacks with 1 or 2 arguments. This is fragile — *args callbacks, decorated functions, and C extensions can all confuse signature inspection. A cleaner approach would be a versioned callback protocol or just always pass both arguments and update all callers.

4. [concern] _snapshot_lock in snapshots.py uses fcntl.flock / msvcrt.locking but the msvcrt path writes a dummy byte and locks byte 0. If the file is empty on first open with a+b, read(1) returns b"", then it writes b"0" — but a second concurrent process opening the same file could also see it empty before the first writes. This is a narrow race on Windows. Acceptable for pre-1.0 but worth a # TODO comment.

5. [question] _get_parent_labels now returns [] when multiple inputs share the same artifact_id (line 493). This silently drops provenance for content-duplicate artifacts. Is this intentional? The docstring on _snapshot_parent_labels says "should not invent broad parent sets," but losing provenance for legitimate duplicate content seems like a different concern.

6. [concern] _pipeline_fingerprint includes pipeline.layers and pipeline.projections configs, which can contain arbitrary objects. The _normalize_fingerprint_value fallback for unknown types returns {"object_type": "..."} with no content — two structurally different configs of the same type would fingerprint identically. The callable handling is good, but non-callable custom objects are a silent collision risk.

7. [positive] Test coverage is thorough: unit tests for object store, refs, and run-id generation; integration tests for concurrent HEAD advancement, content-hash integrity, closure completeness, journal recovery, fingerprint stability; e2e tests for the full build→list→clean→list flow. The validation-failure-blocks-snapshot test is especially important.

8. [nit] The RFC at 885 lines is large for a single file. Consider splitting the "first slice" specification from the "future vision" sections once the first slice ships, to avoid confusion about what's implemented vs. planned.

9. [positive] The synix runs list --json output is schema-versioned from day one (schema_version: 1), matching the project's stated intent to preserve compatibility paths.

10. [nit] run_id format uses microsecond-precision timestamps plus 4 random bytes. The regex in the test (\d{8}T\d{6}\d{6}Z-[0-9a-f]{8}) is correct but the ref path refs/runs/20260306T082007123456Z-1f2e3d4c will be long and opaque in CLI output. The README note about preferring --json over scraping is appropriate.

Verdict

This is a well-designed incremental step that makes Synix's "artifacts are immutable and content-addressed" principle structurally real, with strong test coverage and a clear scope boundary between what ships now and what's deferred.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 3,375 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-03-06T16:37:01Z