fix: address 5 LENS integration issues

[marklubin]

Closes #54

Summary

Addresses 5 issues from the LENS Datagen integration report:

1. Manifest format collision — ArtifactStore._load_manifest() validates JSON structure, skips invalid/foreign entries, warns instead of crashing
2. OpenAI API compat — Config-driven passthrough: LLMConfig.max_completion_tokens and nullable temperature let pipelines control exactly what params hit the API
3. Build failure tracebacks — _print_error() helper: default shows TypeError: msg, -v adds at file.py:line, -vv shows full traceback
4. --plain flag — PlainBuildProgress with timestamped line output for CI/piped environments (partially closes Non-interactive automation mode for CI/scripted runs #54)
5. --version flag — @click.version_option(package_name="synix") on the CLI group

Also includes demo golden comparison improvements: output_masks for per-step regex exclusions, cleanup for run artifacts, improved normalization.

Test plan

• uv run release passes (874 tests, 3 demos)
• synix --version prints version string
• synix build --plain produces pipe-safe output
• Build with -vv and broken pipeline shows full traceback
• Foreign manifest.json doesn't crash the build
• max_completion_tokens in llm_config passes through to OpenAI API

[github-actions]

Note

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

Summary

This PR introduces a citation system (synix:// URIs), a citation validator and fixer, a new "sales deal room" template demonstrating citation-backed competitive intelligence, plus several infrastructure improvements: --plain CLI output for CI, pipeline-relative imports, manifest loading hardening, OpenAI reasoning model support, and --version.

Alignment

Strong alignment. The citation system extends provenance from the build graph into artifact content — claims inside synthesized artifacts now link back to source labels via synix:// URIs, stored as citation_edges in the search index. This is a natural extension of DESIGN.md's "full lineage" principle and "audit determinism" decision: you can now trace not just which inputs produced an artifact, but which specific source grounded each claim within it. The --plain flag directly addresses known limitation #54 (non-interactive automation mode). Pipeline-relative imports address #55. The template demonstrates architecture evolution (staged intel → cascade rebuild) exactly as the vision prescribes.

Observations

1. [positive] core/citations.py is clean, well-scoped, and the synix:// scheme is a good primitive. Separate from the build graph, composable with any text format.

2. [concern] _URI_RE = re.compile(r"synix://([\w-]+)") — labels containing dots (e.g., ep-conv-123.v2) or underscores already work, but labels with dots won't match since \w doesn't include .. The golden outputs show labels like t-text-acme_analytics which work, but any label with a dot would silently fail to parse. The README shows labels like monthly-2024-03 — the hyphen is covered, but worth verifying the label character set is intentionally restricted.

3. [concern] load_pipeline in pipeline.py adds the pipeline directory to sys.path then removes it in finally. But sys.path.remove() only removes the first occurrence — if another thread or import added the same path concurrently, this is fragile. Acceptable for now but worth a comment.

4. [positive] Manifest loading hardening in _load_manifest is thorough — handles corrupt JSON, non-dict top-level, missing required keys, and reserved _ prefixed keys. Good defensive coding.

5. [concern] CitationValidator.validate has a bare except Exception: continue that silently swallows errors per-artifact. This means a misconfigured LLM client or prompt file could produce zero violations with no indication of failure. At minimum, log a warning.

6. [question] The CitationEnrichmentFixer searches for evidence using ctx.search_index.query(claim) — does the search index exist at fix time? The fixer runs after build, so the index should be populated, but this dependency isn't documented in the fixer's docstring.

7. [nit] _REASONING_MODEL_PREFIXES = ("o1", "o3", "o4") — this will also match models like o1-preview, o3-mini etc. which is likely correct, but also matches hypothetical models like o100. Prefix matching is inherently fragile for model detection.

8. [positive] PlainBuildProgress implements the same callback interface as BuildProgress, enabling clean substitution. Good duck-typing contract.

9. [positive] The golden test files for template 02 were updated from error stubs (Error loading pipeline: No module named 'transforms') to actual expected output — the pipeline-relative import fix unblocks the entire template 02 test suite.

10. [concern] The cassette file (llm.yaml) is 2,500+ lines of recorded LLM responses. This is necessary for deterministic demos but will be painful to maintain if prompts change. No concern for correctness, but worth noting the maintenance cost.

11. [nit] MANIFEST_FILENAME constant is good, but it's only used in artifacts.py, verify.py, and info_commands.py — not in all manifest-touching code paths. A grep for "manifest.json" string literals elsewhere would be prudent.

Verdict

This is a well-structured incremental step that extends Synix's provenance model into artifact content, fixes real usability issues (#54, #55), and ships a compelling new template — all coherent with the project vision.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 10,000 lines (of 22,823 total — truncated)
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-02-15T19:10:47Z

[github-actions]

Note

Red Team Review — OpenAI o3 | Adversarial review (docs + diff only)

Threat assessment — Medium-high. Adds new public surface area (citation URI scheme, search-db schema, CLI flags) with scant versioning or migration story; several brittle heuristics and silent-failure paths.

One-way doors

1. Citation URI scheme (synix://…) and core/citations.py
   – Once users embed these URIs in prompts/docs they are forever in notebooks and artifacts. Re-naming or altering syntax later would break provenance links. Must be frozen only after syntax review, version tag and round-trip tests.
2. Search-db schema change – new citation_edges table added in search/indexer.py::create
   – Existing build directories will be recreated, but any external BI tools pointing at search.db will choke on missing table versioning. Need schema version header + migration guard.
3. New validator/fixer names (citation, citation_enrichment) become part of Pipeline API. Removing/renaming later will break user pipelines. Should land only with documented stability guarantee.
4. New CLI flag --plain and hidden -v behaviour change. Scripts/CI will start relying on them; reversing is breaking. Needs docs + changelog.
5. Loader adds pipeline directory to sys.path. Code that starts importing relative modules will depend on this side-effect; removing later will be breaking. Needs explicit contract or an opt-in flag.

Findings

1. src/synix/build/llm_client.py – heuristic _is_reasoning_model / _needs_max_completion_tokens
   Risk: future model names will slip through and cause wrong param, API 400. [warning]
2. build/indexer.py::insert – no size guard on citation_edges; for large corpora runs un-indexed INSERT OR IGNORE per citation, O(n²) on rebuild. [warning]
3. citation_edges writes without surrounding transaction batching – WAL disabled default, will degrade on many small inserts. [warning]
4. core/citations.py uses regex [\w-]+; forbids dots/slashes that user labels may already contain (README shows monthly-2024-03). Breaks existing labels with dots or Unicode. [critical]
5. cli/build_commands.py – duplicates run logic for plain vs rich; copy-paste diverging, but error-handling differs (one path wraps in Live, one not). Subtle behavioural skew. [minor]
6. build/pipeline.py::load_pipeline path injection isn’t thread-safe (global sys.path mutate) – concurrent builds in same process will collide. [warning]
7. Manifest loader silently swallows invalid entries and logs warning only – may mask corruption. Should fail hard. [minor]
8. Validator parses LLM JSON via regex across ``` blocks — brittle; any non-JSON response results in silent pass (returns empty list). False negatives. [minor]
9. No upgrade path for existing search indexes; create() unconditionally drops tables – data loss on rebuild if user added custom FTS virtual columns. [critical]

Missing

• No unit tests for citation parsing, validator, fixer, or new DB schema.
• No migration note or version bump for search.db change.
• Docs/README not updated with --plain flag or citation validator.
• No concurrency tests for pipeline sys.path hack.
• Search index write path lacks foreign-key check; verify fails to check citation_edges.

Verdict — Block. API & storage one-way doors added without versioning, plus critical label-parsing bug and data-loss on index recreation. Require schema-versioning plan, regex fix, and explicit documentation before merge.

Review parameters

• Model: o3
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 10,000 lines (of 22,823 total — truncated)
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-02-15T19:10:52Z

[github-actions]

Note

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

Threat assessment — Medium risk: mostly “quality of life” changes, but it quietly alters persisted build behavior (manifest parsing) and introduces new CLI surface area.

One-way doors

1. synix build --plain / synix run --plain CLI flag

   • Why hard to reverse: Once documented/used in CI scripts, removing/renaming breaks automation. Output format also becomes a quasi-contract.
   • Safe to merge when: You commit to keeping --plain stable (or add --output (rich|plain|json) now). Add docs + an explicit “format not stable pre-1.0” warning if you won’t support it.

2. manifest.json tolerance + reserved “_” keys semantics

   • Why hard to reverse: You’re defining that manifest.json is both (a) the canonical Synix manifest and (b) may contain foreign keys/metadata that Synix will ignore. Users/tools may start relying on _version/_generator keys or on being able to co-locate other manifests.
   • Safe to merge when: You explicitly specify the manifest schema and reserved key behavior in docs, and you decide whether Synix will ever write reserved metadata keys itself.

Findings

1. src/synix/build/artifacts.py — silently discarding manifest entries

   • Pattern: _load_manifest() drops any entry missing {"path","layer"} with only a warning, then continues.
   • Failure mode: corrupted/partially-written manifest (or schema drift) becomes a “successful” empty/partial store; downstream commands might treat this as “no artifacts” and rebuild/lose traceability without a hard error. That’s the wrong default for integrity-sensitive build outputs.
   • Severity: [warning]

2. src/synix/build/artifacts.py — no versioning/migration story

   • Pattern: reserved _... keys are skipped but there’s no _schema_version check and no upgrade path.
   • Failure mode: future manifest format changes can’t be detected cleanly; you’ll keep adding permissive parsing and ambiguity instead of an explicit migration boundary.
   • Severity: [warning]

3. src/synix/cli/build_commands.py — _print_error traceback location is misleading

   • Pattern: uses tb[-1] (deepest frame) as the location.
   • Failure mode: for import/load errors, the last frame is often in Click/runpy internals, not the pipeline file. This will misdirect debugging.
   • Severity: [minor]

4. src/synix/cli/progress.py — PlainBuildProgress timestamps are non-deterministic

   • Pattern: datetime.now() printed for every line.
   • Failure mode: breaks golden testing (you avoided it by not asserting exact output) and makes CI diffs noisy. Also complicates log parsing.
   • Severity: [minor] (but annoying)

5. src/synix/build/llm_client.py + src/synix/core/config.py — OpenAI param selection is under-specified

   • Pattern: if max_completion_tokens set, omit max_tokens; temperature omitted when None.
   • Failure mode: OpenAI API surface is fragmented (chat vs responses, reasoning models, etc.). This code assumes chat.completions.create accepts max_completion_tokens universally. If not, this becomes runtime-only breakage depending on SDK version/model.
   • Severity: [warning] (compat risk)

6. Docs/design mismatch: CLI “run” vs “build”

   • README/website emphasize synix build; DESIGN.md emphasizes synix run. The PR adds --plain to both, but there’s still conceptual drift in public surface naming.
   • Failure mode: user confusion + duplicated compatibility burden.
   • Severity: [minor]

Missing

• Documentation update for --plain, --version, and the new manifest collision behavior. README has a “non-interactive automation mode” limitation; this PR partially resolves it but doesn’t update that section.
• Explicit manifest schema/versioning (even just _schema_version: 1) and a decision: when do we warn vs fail hard?
• Test asserting --plain output stability properties (e.g., no Rich control codes and deterministic timestamps or optional --no-timestamps).
• OpenAI SDK compatibility guardrails: pinned minimum openai library version or a defensive fallback when max_completion_tokens is rejected.

Verdict

Ship with fixes — the intent is fine, but you’re baking in “silent recovery” semantics for a core build artifact and adding a CI flag without documenting/stabilizing its contract.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 1,166 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-02-15T20:23:51Z

[github-actions]

Note

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

Summary — This PR bundles four changes: (1) defensive manifest parsing so foreign manifest.json files don't crash builds, (2) --plain flag for CI-friendly output, (3) OpenAI reasoning-model support via max_completion_tokens / nullable temperature, and (4) golden-file normalization improvements for the demo test harness. It also adds --version to the CLI and improves error message formatting.

Alignment — All changes fit the build-system-for-memory model. The manifest hardening protects artifact store integrity without relaxing immutability — invalid entries are skipped, not silently accepted. The --plain flag directly addresses known limitation #54 (non-interactive automation mode). OpenAI param passthrough respects DESIGN.md's Python-first principle: the pipeline author controls model params explicitly rather than Synix guessing from model names, which is a clean design choice. The MANIFEST_FILENAME constant prevents magic-string drift across the codebase, consistent with the build system's emphasis on artifact integrity.

Observations

1. [positive] _load_manifest validation is thorough — handles corrupt JSON, non-dict top-level, missing required keys, and reserved _-prefixed keys. Good defensive boundary for a file that could collide with npm/other tools. Tests cover all five failure modes.

2. [positive] OpenAI param passthrough avoids model-name sniffing heuristics. The config explicitly declares intent (max_completion_tokens, temperature=None), which is the right contract for a pipeline API. Six unit tests cover the full matrix.

3. [concern] _print_error at verbose=1 uses rsplit('/', 1)[-1] for the filename, which will break on Windows paths. Should use Path(tb[-1].filename).name or os.path.basename.

4. [concern] The build command duplicates the run_pipeline call block — once for use_plain, once for the Live path. The only difference is the progress object and the Live context manager. This could be a single block with a conditional context manager (e.g., contextlib.nullcontext), reducing the duplicated error-handling code from ~20 lines to ~5.

5. [question] _REQUIRED_ENTRY_KEYS = {"path", "layer"} — is "layer" always present in synix-produced manifest entries? If older builds used a different key (e.g., "layer_name"), this would silently drop all existing entries on upgrade. Worth a comment or migration note.

6. [nit] _apply_masks compiles regexes on every call. For the demo test harness this is fine, but the function doesn't cache compiled patterns. Acceptable at current scale.

7. [positive] Golden files are updated consistently across all three templates. The normalization improvements (<BUILD_COUNTS>, <BUILD_TOTAL>, dropping LLM stats lines) make goldens more stable across fresh vs. incremental runs — good investment for CI reliability.

8. [question] The PlainBuildProgress class imports datetime inside _ts(). This is called per-event. Minor, but unusual — is this avoiding a circular import or just an oversight?

9. [positive] The e2e test test_foreign_manifest_then_build exercises the full path: write foreign manifest → run build via CLI → verify synix artifacts appear. This is exactly the right level of test for this bug class.

10. [nit] test_build_error_shows_exception_type asserts only "Error" in result.output — this is very loose. Could match any output containing "Error". Asserting "ValueError" would be more precise.

11. [concern] LLMConfig.temperature changed from float to float | None with default 0.3. This is a public-facing config change. Existing pipelines passing temperature=None explicitly would previously have been a type error; now it silently omits the parameter. The behavioral change is correct but worth a changelog note since it changes what gets sent to the API.

Verdict — A solid incremental PR that hardens the artifact store, improves CI ergonomics, and adds real model compatibility — all without drifting from the project's core abstractions. The duplicated control flow in build and the Windows path issue should be addressed, but neither blocks merge.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 1,166 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-02-15T20:24:02Z

[github-actions]

Note

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

Threat assessment — Medium risk: mostly additive/defensive changes, but it quietly cements artifact manifest behavior and introduces a “plain” progress mode that can drift out of sync with the runner.

One-way doors

1. Manifest filename + schema tolerance (manifest.json + skipping invalid entries)
   Why hard to reverse: The manifest file and its semantics are effectively part of the on-disk artifact format users may script against. Once you start “accepting” foreign/invalid entries (and reserving _ keys), users will rely on that leniency and on being able to stash metadata in the same file. Tightening later becomes a breaking change.
   Safe to merge if: You explicitly specify the manifest contract in docs (filename, reserved keys, required keys, forward/back compat policy) and add versioning inside the manifest (e.g., "_synix": {"version": 1}), not just “skip weird stuff”.

2. New CLI surface: synix build --plain / synix run --plain
   Why hard to reverse: CLI flags get copy-pasted into CI and wrapper scripts. Removing/renaming later is painful.
   Safe to merge if: You commit to stable behavior (no ANSI, line-oriented, flush semantics) and test it under non-TTY + piping. Ideally also add --log-format jsonl|text rather than a one-off flag.

3. Public config field: LLMConfig.max_completion_tokens
   Why hard to reverse: This becomes part of the Python pipeline API and any serialized pipeline config.
   Safe to merge if: You clarify precedence rules vs max_tokens and ensure parity across providers (or document provider-specific fields cleanly).

Findings

1. src/synix/build/artifacts.py — still uses manifest.json (MANIFEST_FILENAME) despite collision report
   Risk/failure mode: The “fix” is only to skip invalid entries, not to avoid clobbering/being clobbered. Two tools writing manifest.json in the same dir will still overwrite each other on save. Your own report calls this critical; this PR doesn’t actually solve it.
   Severity: [critical]

2. ArtifactStore._load_manifest() silently drops entries without surfacing to user
   Risk/failure mode: If Synix’s own manifest gets corrupted/partially written, you’ll “start empty” and treat everything as cache-miss, potentially triggering expensive rebuilds without a hard error. Warnings via logger are easy to miss in CLI UX.
   Severity: [warning]

3. src/synix/build/llm_client.py — OpenAI token param selection is config-driven but incomplete
   Risk/failure mode: You set max_completion_tokens from config, but still pass max_tokens sourced from a local max_tokens variable otherwise. Where does that value come from—config or call site? If call site passes a default that diverges from LLMConfig.max_tokens, you’ve now got two sources of truth. Also: you didn’t address other OpenAI-breaking params (reasoning models often reject temperature, may require different endpoints).
   Severity: [warning]

4. src/synix/cli/build_commands.py::_print_error chooses last traceback frame
   Risk/failure mode: tb[-1] is often in Click/Rich internals or re-raise wrappers, not the actionable frame. You’ll point users to the wrong file:line. Also exc.__traceback__ can be None.
   Severity: [minor]

5. src/synix/cli/progress.py::PlainBuildProgress may not match BuildProgress interface over time
   Risk/failure mode: No static/interface enforcement. If runner adds a callback, plain mode breaks at runtime only. Also it doesn’t flush explicitly; in some CI buffering scenarios you still won’t get “live” logs.
   Severity: [warning]

6. tests/e2e/test_manifest_collision.py — weak assertion for “no ANSI escapes”
   Risk/failure mode: The test claims no ANSI but doesn’t check for \x1b[ sequences. It may pass while still emitting control codes.
   Severity: [minor]

7. Docs mismatch: README/Website say build dir contains manifest.json + SQLite, etc.; PR introduces reserved _ keys and “skip invalid entries” behavior without documentation
   Risk/failure mode: Users will keep treating manifest.json as theirs to write/merge; you’ll get more collisions, and now they’ll be silent.
   Severity: [warning]

Missing

• Actual collision prevention: namespacing the manifest filename (e.g. .synix/manifest.json or .synix_manifest.json) or writing to a tool-owned subdir. Skipping invalid entries is not a real fix if the other tool overwrites the file.
• Manifest versioning and explicit schema in docs (and in verify checks).
• A real ANSI/TTY test for --plain (assert "\x1b[" not in output) and a non-TTY simulation.
• Changelog/README update for --plain, --version, and max_completion_tokens.

Verdict

Ship with fixes — the error verbosity and OpenAI param passthrough are fine, but the manifest “collision” is still a footgun because you didn’t claim ownership of the filename/path; that’s the one-way door you shouldn’t half-solve.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 1,343 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-02-15T22:39:54Z

[github-actions]

Note

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

Summary

This PR addresses five issues discovered during a real integration (LENS datagen pipeline), documented in a detailed integration report. The fixes are: (1) graceful handling of foreign manifest.json files in the build directory, (2) config-driven OpenAI API parameter passthrough (max_completion_tokens, nullable temperature), (3) verbosity-dependent error reporting with tracebacks, (4) --plain flag for CI-friendly build output, and (5) --version CLI flag. Golden test files and normalization logic are updated accordingly.

Alignment

Strong fit. The manifest collision fix directly protects artifact immutability and content-addressed caching — a corrupted manifest was silently breaking the cache layer. The integration report itself embodies the DESIGN.md hypothesis that "memory architecture is a runtime concern" — someone iterated on a real pipeline and hit real edges. The --plain flag addresses known limitation #54 (non-interactive automation mode). The LLM client change respects "Python-first" by letting pipeline authors control API params via LLMConfig fields rather than baking in model-name heuristics — this is the right call per the design's emphasis on user-declared configuration.

Observations

1. [positive] _load_manifest validation in artifacts.py is thorough: handles non-JSON, non-dict top-level, string values, missing required keys, and _-prefixed metadata keys. This is exactly the defensive coding the manifest collision demanded.

2. [positive] The LLM client avoids model-name sniffing (no _needs_new_token_param heuristic from the report's suggestion). Instead, the pipeline author sets max_completion_tokens explicitly. This is cleaner and consistent with "pipelines are Python, you control the config."

3. [positive] Test coverage is excellent. Unit tests for all manifest corruption variants (TestManifestValidation), OpenAI param passthrough (TestOpenAIParamPassthrough), CLI flags, and an e2e test for the foreign manifest scenario. Edge cases (non-JSON, array top-level, mixed entries, _-prefixed keys) are all covered.

4. [concern] _REQUIRED_ENTRY_KEYS = {"path", "layer"} doesn't include artifact_id, which the README shows as part of the manifest schema. If a manifest entry has path and layer but no artifact_id, it'll pass validation but may break downstream code that assumes artifact_id exists.

5. [concern] The _print_error function uses rsplit('/', 1)[-1] for filename extraction, which will fail on Windows paths using backslashes. Minor for now but worth noting since the project is Python-first and cross-platform.

6. [nit] PlainBuildProgress imports datetime inside _ts() on every call. Move to module-level.

7. [nit] The MANIFEST_FILENAME constant is good, but verify.py still reads manifest JSON directly rather than going through ArtifactStore._load_manifest, so the validation logic isn't shared — a foreign manifest could still cause verify to report confusing errors.

8. [positive] The integration report (LENS_INTEGRATION_REPORT.md) is an unusually valuable artifact — it's a real-world validation of the build system on a non-trivial pipeline (5 layers, 75 LLM calls, concurrent execution). The "what worked well" section validates core design bets: DAG resolution, content-addressed caching, provenance tracking, fingerprint-based invalidation.

9. [question] The output_masks and cleanup_files additions to demo commands — are there corresponding demo case YAML files in this PR that use them? The diff only shows the code, not the case definitions. If no case uses them yet, these are dead code paths.

10. [positive] Golden file updates are consistent with the normalization changes. The shift from <N> <STATUS> to <BUILD_COUNTS> and dropping LLM stats lines makes goldens more stable across environments.

Verdict

This is a solid, well-tested PR that fixes real issues found during real usage, advances CI usability (a documented limitation), and does so without architectural drift — a good incremental step.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 1,343 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-02-15T22:40:00Z

[github-actions]

Note

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

Threat assessment — Medium risk: mostly additive fixes, but it subtly changes caching/manifest behavior and OpenAI request semantics in ways that can brick existing builds if assumptions differ.

One-way doors

1. Manifest entry “validation + skipping” semantics (src/synix/build/artifacts.py)
   Hard to reverse because users will now rely on “foreign/garbled manifest won’t fail, it will silently ignore entries.” If later you tighten validation and error out, you’ll break workflows.
   Safe to merge if: you define an explicit policy: either (a) strict by default with a clear error + “--repair/--ignore-foreign-manifest”, or (b) always tolerant but loudly warns and writes a _synix_manifest_version so tooling can detect “this is a synix manifest”.

2. New public CLI flags: build/run --plain and global --version (src/synix/cli/*.py)
   Once documented, you carry them forever. Changing names/output format becomes user-visible breakage in CI scripts.
   Safe to merge if: you commit to stable output guarantees for --plain (at least line structure + no ANSI), and add docs/help text that it’s intended for non-TTY/CI.

3. LLMConfig API shape: temperature: float | None and max_completion_tokens (src/synix/core/config.py)
   Config schema becomes a de-facto public API. Reversing field names breaks pipelines.
   Safe to merge if: you document precedence rules (max_completion_tokens overrides max_tokens), and add validation to prevent contradictory configs across providers.

Findings

1. src/synix/build/artifacts.py: _load_manifest() silently drops invalid entries
   Failure mode: you can mask real corruption of Synix’s own manifest (partial write, schema regression, user edits) and then proceed with an empty cache → expensive rebuilds without a hard failure. Warnings are easy to miss.
   Severity: [warning]

2. src/synix/build/artifacts.py: MANIFEST_FILENAME remains "manifest.json" despite the integration report recommending namespacing
   Failure mode: you’re treating the symptom (skip invalid) but not the cause (filename collision). Collisions remain likely in “build/” where users dump other outputs.
   Severity: [warning]

3. src/synix/build/llm_client.py: OpenAI token parameter now config-driven, but no provider-level validation
   Failure mode: a user can set max_completion_tokens for non-OpenAI providers or set both oddly; behavior becomes unclear and may be ignored silently. Also: using max_tokens fallback may still be wrong for some OpenAI models; you’ve punted model detection to users without guardrails.
   Severity: [warning]

4. src/synix/cli/build_commands.py: _print_error() uses tb[-1] (innermost frame) which may be in Click internals, not Synix
   Failure mode: “TypeError at core.py:line” might point at framework plumbing, not the originating Synix frame, reducing usefulness. You likely want the last frame within synix/, else fallback.
   Severity: [minor]

5. src/synix/cli/progress.py: PlainBuildProgress emits Rich markup-like brackets but not guaranteed “no ANSI”
   Failure mode: you claim “no TUI safe for CI/pipes” but still depend on Rich Console.print(). If console is configured with color/markup, you can still emit control sequences. Tests only check absence of ANSI escapes indirectly (and not robustly).
   Severity: [minor]

6. tests/unit/test_cli.py: test_build_error_shows_exception_type asserts only `"Error" in output
   Failure mode: this doesn’t verify the actual new contract (exception type included). It will pass on many unrelated outputs and won’t catch regressions.
   Severity: [minor]

Missing

• Docs updates: README/CLI reference doesn’t mention --plain, --version, or verbosity behavior changes. This is user-facing.
• Manifest collision strategy decision: either rename to .synix-manifest.json (preferred) or explicitly codify “manifest.json is reserved for Synix”.
• Stricter tests for --plain: assert no ANSI via regex (\x1b\[[0-9;]*[A-Za-z]) and stable line formats.
• Config validation tests: ensure max_completion_tokens is only used/supported for OpenAI provider (or explicitly allowed but ignored with warning).

Verdict

Ship with fixes — the direction is right, but you’re papering over a manifest namespace collision without resolving it, and the new “tolerant manifest” behavior risks silent cache breakage and surprise rebuild cost.

Review parameters

• Model: gpt-5.2
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 985 lines
• Prompt: .github/prompts/openai_review.md
• Timestamp: 2026-02-15T23:54:33Z

[github-actions]

Note

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

Summary — This PR addresses five issues discovered during a real integration (LENS datagen pipeline), documented in a detailed integration report. The code changes fix: (1) manifest format collisions when the build directory contains a foreign manifest.json, (2) OpenAI API compatibility for newer models requiring max_completion_tokens, (3) swallowed tracebacks in build failures, (4) non-TTY-friendly Rich output via a --plain flag, and (5) a missing --version CLI flag.

Alignment — Strong. Every fix strengthens the "workbench" thesis from DESIGN.md. The manifest collision fix protects artifact immutability and content-addressed caching when build directories are shared with other tools — a realistic scenario for pipeline authors. The --plain flag directly addresses issue #54 (non-interactive automation mode) and the DESIGN.md emphasis on pipelines being composable with other tooling. The LLM client change keeps the Python-first API working across model families without baking in model-name heuristics — the pipeline author controls config, consistent with "architecture is a runtime concern." The integration report itself is a strong signal: someone used Synix on a real pipeline and fed findings back systematically.

Observations

1. [positive] The manifest validation in _load_manifest is defensive without being lossy — valid entries survive, invalid ones are skipped with warnings, and _-prefixed keys are reserved for future metadata. This is the right design for a build artifact store that might share a directory.

2. [positive] The OpenAI param fix avoids model-name sniffing (no prefix matching on "o1", "gpt-5"). Instead, the pipeline author sets max_completion_tokens in config and the client passes it through. This is cleaner than the integration report's own suggestion and consistent with Python-first control.

3. [positive] Test coverage is thorough: 6 unit tests for manifest validation edge cases, 4 unit tests for OpenAI param passthrough, an e2e test for the foreign manifest scenario, CLI tests for --version, --plain visibility, and error output format.

4. [concern] PlainBuildProgress duplicates the callback interface of BuildProgress without a shared protocol or ABC. If the runner adds a new callback method, PlainBuildProgress will silently lack it. A Protocol class or @runtime_checkable would catch drift at test time.

5. [concern] The --plain branch in build_commands.py duplicates the entire run_pipeline call and exception handling block. Both the if use_plain: and else: branches are nearly identical except for progress instantiation and the Live context manager. Extract the common logic to reduce surface area for divergence.

6. [nit] use_plain = plain on line ~130 is an unnecessary alias — plain is already a clear name from the Click parameter.

7. [question] The _print_error helper imports traceback inside the function body. This is a cold-path function so performance isn't a concern, but the pattern is inconsistent with the module's other imports. Intentional to avoid import cost on the happy path?

8. [nit] The integration report references synix version 0.10.0 but the --version flag reads from package metadata. If the report is committed as documentation, the version string will go stale. Consider noting it's a point-in-time snapshot.

9. [positive] The MANIFEST_FILENAME constant is extracted and used consistently across artifacts.py, verify.py, and info_commands.py — good single-source-of-truth refactor that makes a future rename to .synix_manifest.json (as the report suggests) a one-line change.

10. [question] The e2e test test_foreign_manifest_then_build asserts that transcript entries appear in the manifest after build, but doesn't verify the foreign keys were dropped. Adding an assertion that "name" or "dependencies" are absent would strengthen the collision test.

11. [positive] LLMConfig.temperature changing to float | None is the right move — reasoning models (o1, o3) reject temperature entirely, and None semantics are tested explicitly.

Verdict — This is a solid, well-tested PR that fixes real issues found during actual pipeline usage, with changes that are architecturally consistent and well-scoped. The duplication in the build command's plain/rich branches should be cleaned up, and the progress interface should get a shared protocol, but neither blocks merge.

Review parameters

• Model: claude-opus-4-6
• Context: README.md, DESIGN.md, synix.dev, PR diff
• Diff size: 985 lines
• Prompt: .github/prompts/claude_review.md
• Timestamp: 2026-02-15T23:54:44Z