feat: Architecture 2.0

[brent-hoover]

No description provided.

[claude]

Claude finished @brent-hoover's task in 7m 55s —— View job

---

PR Review: feat: Architecture 2.0 (Pass 10)

• Gather context and identify new commits since Pass 9
• Review new changes for correctness, async safety, type hints, test coverage
• Check status of previously flagged unresolved issues
• Summarize findings

---

What changed since Pass 9

Ten commits (merged as PR #229) implementing Epic 2 MVP PR B2 — wiring store WRITE persistence through TicketStore — and a series of security/correctness fixes discovered via review:

Commit	Summary
e0dd8a3	feat(substrate): Epic 2 MVP PR B2 — wire store WRITE persistence through TicketStore
abc0a2f	fix(substrate): close store-write key tampering + cross-process dup-create race
b302887	fix(substrate): reload before create-vs-update decision; complete store cache invalidation
0c81431	fix(substrate): reload store before create-vs-update regardless of ownership
556f889	fix(substrate): reject jig-N-shaped URI ids on store writes
fa42f2d	fix(store): make JsonlStore.load() hold the write lock (safe live refresh)
9929ec8	fix(store): make store-write create-vs-update decision atomic under the lock
b993f8d	fix(store): atomic load() replay (no live-state damage on error) + tighten test
7affc5d	docs(substrate): correct write() docstring step (3)
4e90932	Merge pull request #229

Files changed: jig/substrate/store_authority.py, jig/store/tickets.py, jig/store/core.py, jig/uri/cache.py, tests/substrate/test_store_authority.py, tests/substrate/test_store_authority_write_gate.py, tests/test_store_core.py, tests/test_ticket_store.py, tests/test_uri_cache.py.

---

Overall Assessment

A well-executed, security-aware write path. The architecture is correct: write_addressed() is the right
primitive — atomic create-vs-update under the cross-process flock against fresh disk state. The multi-commit fix
sequence is disciplined: each round addresses one identified security or correctness concern with a targeted test.
No correctness bugs found. Two minor observations below.

---

jig/store/tickets.py — write_addressed()

✅ Atomic create-vs-update decision is correct

tickets.py:233–247

async with self._key_lock():
    await self._collection.load()  # fresh disk state, held until we write
    existing = await self._collection.get(ticket_id)
    if existing is None:
        ticket = Ticket.model_validate({**fields, "_id": ticket_id})
        await self._create_locked(ticket)
        result, created = ticket, True
    else:
        self._check_transition_gate(existing, fields)
        result, created = await self._write_update(ticket_id, fields), False

The flock is acquired once, the collection reloads under it, and the create/update decision is made against
current disk state — all before the flock releases. A concurrent creator can never turn an update into a
spurious duplicate-id error or a partial-body create failure. The callbacks fire correctly after the lock
releases (_fire_create for new tickets, _fire_status_change inside _write_update via asyncio.ensure_future
— scheduled while the flock is held but executed after it releases).

✅ load() holding the asyncio.Lock prevents in-memory corruption on live refresh

store/core.py:53 — async with self._lock: in load() ensures a reload cannot interleave with a concurrent
insert/update/delete on the same in-memory map. The fix is the right place: root layer, not each caller.

✅ Atomic replay (local map + swap on success) protects live state

store/core.py:54–101 — Replay into a local docs dict, then swap only after full validation. A corrupt row
raises but leaves the live store's current state intact. The test at test_load_leaves_live_state_intact_on_replay_error
pins this correctly — checks both the in-memory doc and the index after a failed reload. Strong.

🟢 _write_update docstring lists only update/approve as callers

jig/store/tickets.py:327

"""Validated write of an update row. NOT gated — callers (``update``,
``approve``) enforce their own transition rules first."""

write_addressed is now also a caller (calling _check_transition_gate before delegating here). The "NOT gated"
claim is still accurate, but a future reader may miss the third caller context (e.g. that write_addressed holds
_key_lock() during the call, which changes the locking semantics). A one-liner mention in the docstring
would complete the picture: "and ``write_addressed`` (which additionally calls this under ``_key_lock()``)." Fix this →

---

jig/store/core.py and jig/uri/store.py

🟢 docs.pop(doc_id, None) in load() replay — unreachable None default (two sites)

jig/store/core.py:91 (new, in the atomically-replayed load()) and jig/uri/store.py:72 (carried from Pass 9):

# In both replay loops, after:
if doc_id not in live_ids:  # (or: if doc_id not in docs:)
    raise ValueError(...)
docs.pop(doc_id, None)  # ← None default unreachable — key is guaranteed present

In both cases the prior guard ensures doc_id is in docs. del docs[doc_id] is more precise. Low priority,
same finding at both sites. Fix this →

---

jig/substrate/store_authority.py

✅ Key tampering closed correctly

_prepare_body (store_authority.py:193–213) rejects a non-empty key field and strips id/_id/key
from the body. A caller can supply key="" (falsy) which is silently stripped — correct since it can't shadow
the server-assigned key. The if doc.get("key"): guard is the right falsy-sentinel here.

✅ jig-N id disjointness enforcement

_RESERVED_KEY_ID = re.compile(r"^jig-\d+$") at line 44 correctly covers the pattern that TicketStore.resolve_ref
checks. The test test_write_rejects_a_jig_key_shaped_id pins this.

✅ Stale-snapshot cases all covered

Three distinct stale scenarios now have dedicated tests:

• test_write_through_a_stale_injected_store_still_updates — injected store loaded before another instance created the ticket
• test_write_to_a_ticket_another_instance_created_updates_it — owned store snapshot predates a cross-instance create
• test_concurrent_writes_to_a_new_id_create_then_update — two concurrent asyncio.gather writes; exactly one creates, the other updates, no failure

The asyncio.gather test correctly exercises the in-process concurrent write path through _key_lock()'s
LOCK_NB poll — the flock serializes them even within the same process since each _key_lock() call opens a
new fd and the OS assigns independent flock contexts.

---

jig/uri/cache.py

✅ exact=True list invalidation is correct

cache.py:182–183 — on TicketStateChanged, both the per-ticket prefix and the bare ("tickets",) list key
are invalidated (the list key with exact=True so sibling ticket entries survive). The comment notes the
branch is currently unreachable via normal resolution but retained for parity. Clean.

---

Carried from prior reviews — still unresolved

🟡 No tombstone or test for (BuildPhase.MERGING, AgentCompleted) (10 passes)

jig/engines/build/decide.py:209–214 — _TRANSITIONS still has no entry for this pair. The decide() docstring
now says "An unknown (status, event) pair is a no-op" — that's helpful — but there's still no tombstone comment
at the gap and no test asserting the fallthrough for this pair. 10 passes. Fix this →

🟡 Misleading comment in concurrent coordinator test (9 passes)

tests/engines/build/test_coordinator.py:114

await asyncio.sleep(0)  # let both reach dispatch before either commits

t1 holds the global asyncio.Lock inside gate.wait(); t2 blocks on the lock — serialized dispatch,
not concurrent. Still says "let both reach dispatch before either commits." Fix this →

🟡 from_dict() stream construction outside the try/except KeyError block (since Pass 7)

jig/runtime/recorded.py:102–104

stream=tuple(
    JigEvent(type=e["type"], data=e["data"]) for e in data.get("stream", [])
),  # ← still outside the try block; malformed stream entry raises KeyError, not ValueError

Fix this →

🟢 Inline imports in test_model_and_sub_models_are_immutable (since Pass 5)

tests/model/test_invariant_logic.py:85–86 — import pytest and from pydantic import ValidationError are still inside the function body (ruff PLC0415). Fix this →

---

What's well done in this pass

• Multi-commit fix sequence discipline: each of the 9 fix commits addresses exactly one identified problem, adds a targeted test, and verifies the full suite. This is the right model for security-critical path work.
• write_addressed as the atomic primitive: moving the reload + create-vs-update decision inside _key_lock() eliminates all the TOCTOU windows that the earlier per-caller-reload approach had. The docstring explains the invariant precisely.
• load() under asyncio.Lock: the fix is at the right layer. Every consumer of JsonlStore.load() gets the protection without caller changes.
• Replay-into-local-map: the pattern is exactly right — validate the whole file, then swap, so a corrupt reload never damages the live instance. The companion test (live state intact + index intact after failed reload) is thorough.
• Cross-process flock + LOCK_NB poll: the polling design is correct (no blocking acquire on the event loop, no thread-hold/cancellation race). The test_concurrent_writes_to_a_new_id_create_then_update test proves the in-process concurrent path.
• Callback timing: _fire_create fires after the lock releases; _fire_status_change is scheduled (via ensure_future) while the lock is held but executes after — both correct.
• writable iterable constructor: frozenset(writable) consumes generators without a separate list() call. Clean.

---

Summary

Epic 2 MVP PR B2 is correct and security-hardened. The write path's invariants are:

1. Schema validation via Ticket.model_validate (no raw dict appends)
2. Operator-only PROPOSED → OPEN gate (no bypass path)
3. jig-N key assigned server-side (tampering rejected at gate)
4. Atomic create-vs-update under cross-process flock (no TOCTOU window)
5. Callback delivery after commit (no announce-before-write race)

• 2 new minor observations: _write_update docstring caller list; docs.pop(..., None) in core.py:91
• 4 still-carried: (MERGING, AgentCompleted) tombstone + test (10 passes); misleading concurrent test comment (9 passes); from_dict() stream error inconsistency; inline imports in immutability test

Ready to merge.

[claude]

Claude finished @brent-hoover's task in 4m 43s —— View job

---

PR Review: feat: Architecture 2.0 (Pass 11)

• Gather context and identify new commits since Pass 10
• Review new changes (Epic 2 MVP PR C — typed bus migration)
• Check status of previously flagged unresolved issues
• Summarize findings

---

What changed since Pass 10

Three commits (merged as PR #230) implementing Epic 2 MVP PR C — migrating the ticket lifecycle bus to typed events:

Commit	Summary
05ca5d5	feat(substrate): Epic 2 MVP PR C — migrate ticket lifecycle bus to typed events
d9bea19	fix: tolerant typed-event decode + fully-atomic load() index rebuild
e09349a	test(substrate): parametrize internal round-trip + assert decode preserves dispatch topic

Files changed: jig/substrate/events.py (+75), jig/ticket_events.py (+/-43), jig/ticket_mcp.py (+/-30),
jig/orchestrator.py (+/-47), jig/store/core.py (+21/-5),
tests/substrate/test_events.py (+112), tests/test_store_core.py (+22).

---

Overall Assessment

Clean, well-scoped execution. The from_message / decode_event inverse pair is correct, the tolerant-decode design (returns None on any malformed input, never raises) is exactly right for a dispatch loop, and the orchestrator retains a raw-kind fallback so partial/legacy messages still schedule rather than being silently dropped. The atomic index rebuild fix closes a correctness gap in JsonlStore.load(). No correctness bugs found.

---

jig/substrate/events.py

✅ decode_event tolerant-decode design is correct

try:
    return decoder.from_message(msg)
except (KeyError, ValidationError):
    return None

KeyError catches missing required payload fields (e.g. p["ticket_id"] on a partial message). ValidationError catches type mismatches from pydantic. The pre-dispatch isinstance(kind, str) guard ensures a non-string or missing kind returns None without entering from_message(). A stray message can never kill the dispatch loop.

✅ TicketCreated.from_message() correctly skips the type alias

The comment explains why work_type (not type) is passed: extra="forbid" would reject the type alias if fed back. from_message() reads p["work_type"] (which to_message() always includes), so the round-trip is faithful. If a truly legacy payload has only type and no work_type, KeyError fires and decode_event() returns None — correct tolerant behavior.

✅ _envelope() preserves explicit topics through the round-trip

_TicketLifecycleEvent's model validator only overrides an empty-string topic. Since _envelope(msg)["topic"] passes the wire topic (e.g. "orchestrator") explicitly, the validator doesn't override it. The test_decode_preserves_an_explicit_dispatch_topic test pins this.

🟢 No test for msg.payload is None

kind = (msg.payload or {}).get("kind")

None payload → {} → kind=None → isinstance(None, str) → False → decoder=None → returns None. Correct by construction, but test_decode_returns_none_when_payload_has_no_kind uses an empty-dict payload — a None-payload test would complete coverage.
Fix this →

---

jig/orchestrator.py

✅ Typed-first dispatch with raw-kind fallback is correct

event = decode_event(msg)
kind = event.kind if event else payload.get("kind")
if isinstance(event, TicketCreated):
    ...
elif isinstance(event, TicketUpdated):
    if event.status == TicketStatus.OPEN.value:
        ...
elif kind == "ticket_created":  # fallback for undecodable messages
    ...

The fallback paths preserve the pre-typed tolerant behavior exactly — ticket_created still schedules if ticket_id is present; ticket_updated still reschedules if the status matches. Undecodable messages that the new path can't handle still work.

✅ _update_ticket_status migration preserves semantics

Old: sender="orchestrator", to="broadcast", topic=f"tickets.{ticket_id}".
New: TicketUpdated(ticket_id=ticket_id, status=status.value).to_message().

TypedEvent defaults sender="orchestrator" and recipient="broadcast". _TicketLifecycleEvent's validator sets topic to ticket_topic(ticket_id) when empty. The wire message is byte-identical. ✓

🟢 _reschedule_reset_ticket extraction is clean

Moving the pop + reschedule into a helper eliminates the duplicated logic between the typed and fallback branches. The docstring is precise.

---

jig/store/core.py

✅ Atomic index rebuild is correct

indexes: dict[str, dict[Any, set[str]]] = {field: {} for field in self._index_fields}
for doc in docs.values():
    self._index_insert(doc, indexes)
self._docs = docs
self._indexes = indexes

Building indexes into a local dict before swapping means an unhashable indexed value (e.g. a list where an int is expected) raises without touching the live store's _docs or _indexes. Prior to this fix, _docs was swapped first, leaving the live store inconsistent if index rebuild failed. The test_load_leaves_live_state_intact_on_index_rebuild_error test pins both halves: get("b") still returns None (doc not swapped in) and find_by("n", 1) still returns [{a}] (old index intact).

✅ _index_insert optional indexes parameter is clean

The None-default path uses self._indexes (normal inserts/updates); the provided-dict path uses the caller's local map (replay). No behavioral change to callers that don't pass the argument.

---

Test coverage

8 new cases in test_events.py cover: full TicketCreated round-trip, explicit-topic dispatch copy, TicketUpdated round-trip (parametrized internal True/False), untyped kind, no-kind payload, partial typed payload, and base from_message raises. Strong coverage for the decode_event contract.

---

Carried from prior reviews — still unresolved

🟡 No tombstone comment or test for (BuildPhase.MERGING, AgentCompleted) (11 passes)

jig/engines/build/decide.py:209–214

_TRANSITIONS has no entry for (BuildPhase.MERGING, AgentCompleted). A late or duplicate AgentCompleted while a ticket is MERGING falls through decide()'s fallthrough path at line 241–242 and silently no-ops. The decide() docstring says "An unknown (status, event) pair is a no-op" — helpful — but there is still no tombstone at the specific gap and no test asserting the fallthrough behavior for this pair.
Fix this →

🟡 Misleading comment in concurrent coordinator test (10 passes)

tests/engines/build/test_coordinator.py:114

await asyncio.sleep(0)  # let both reach dispatch before either commits

t1 holds the global asyncio.Lock inside gate.wait(); t2 blocks on the lock. The test exercises serialized dispatch, not concurrent dispatch. The comment still says "let both reach dispatch before either commits."
Fix this →

🟡 from_dict() stream construction outside the try/except KeyError block (since Pass 7)

jig/runtime/recorded.py:102–104

stream=tuple(
    JigEvent(type=e["type"], data=e["data"]) for e in data.get("stream", [])
),  # ← outside the try block; malformed stream entry raises KeyError, not ValueError

Fix this →

🟢 Inline imports in test_model_and_sub_models_are_immutable (since Pass 5)

tests/model/test_invariant_logic.py:85–86

import pytest and from pydantic import ValidationError are still inside the function body (ruff PLC0415).
Fix this →

---

What's well done in this pass

• Tolerant decode: decode_event() never raises — KeyError + ValidationError both return None. Dispatch loops are safe against any malformed message.
• _envelope() static helper: clean extraction of the four routing fields, shared by all concrete from_message() implementations.
• Fallback path in orchestrator: the elif kind == "ticket_created" / elif kind == "ticket_updated" branches are clearly commented and preserve pre-typed behavior for undecodable messages. The comment "can't kill the dispatch loop" is exactly the right framing.
• TicketUpdated._payload() manual build: correct choice to bypass super()._payload() — it avoids including internal with the wrong key name; _internal: True is only added when set.
• Parametrized internal test: @pytest.mark.parametrize("internal", [False, True]) explicitly proves both payload shapes round-trip correctly.
• Atomic index rebuild: staging into a local indexes dict before the swap closes the last live-state corruption window in JsonlStore.load(). The test proves the doc AND the index both survive a failed reload.

---

Summary

Epic 2 MVP PR C lands cleanly. The typed event decode path is correct and well-tested, the orchestrator service loop is safe against partial/legacy messages, and the store index rebuild is now fully atomic.

• 1 new minor finding: no test for msg.payload = None in decode_event (correct by construction, but the test gap is minimal)
• 4 still-carried: (MERGING, AgentCompleted) tombstone + test (11 passes); misleading concurrent test comment (10 passes); from_dict() stream error inconsistency (since Pass 7); inline imports in immutability test (since Pass 5)

Ready to merge.

[claude]

Claude finished @brent-hoover's task in 6m 1s —— View job

---

PR Review: feat: Architecture 2.0 (Pass 12)

• Gather context and identify new commits since Pass 11
• Review new changes for correctness, async safety, type hints, test coverage
• Check status of previously flagged unresolved issues
• Summarize findings

---

What changed since Pass 11

Six commits (merged as PR #232) implementing Epic 2 MVP PR D Wave 1 — StoreAuthority as the orchestrator's store composition root — plus two supporting commits:

Commit	Summary
d5c550a	docs(architecture): ADR-0001 — store access routing model
3ecd70d	feat(substrate): Epic 2 PR D wave 1 — StoreAuthority as orchestrator store composition root
1305498	fix(orchestrator): harden service-loop raw fallback against malformed ids
59f03bc	docs(architecture): move ADR-0001 to docs/reference/ per repo convention
c4b56ed	fix(substrate): make StoreAuthority.load() idempotent + cover injected ticket store
bc7d2f2	Merge pull request #232

Files changed: jig/substrate/store_authority.py (+98), jig/orchestrator.py (+96/-45),
tests/substrate/test_store_authority.py (+89), tests/test_service_loop_dispatch.py (new, +94),
docs/reference/adr-0001-runtime-store-access-routing.md (new, +165), architecture/plan.md (+7).

---

Overall Assessment

Clean wave. The composition-root pattern is mechanically correct: StoreAuthority.load() owns construction +
parallel loading of all six domain stores, the orchestrator's attribute aliases point at the same instances, and the
idempotency guard (if self._threads is not None: return) prevents vended-store orphaning on double-load. The
service-loop hardening closes a real crash vector. ADR-0001 is a well-reasoned architectural decision document. One
new minor finding; three carried items from prior passes (one reduced in severity).

---

jig/substrate/store_authority.py

✅ load() idempotency guard is correct

if self._threads is not None: return uses _threads as the sentinel because it is always None
before load() and always non-None after — unlike _ticket_store, which can be non-None before load() when
tickets are injected. A second call returns early without orphaning any already-vended store. ✓

✅ Injected ticket store flows correctly through load()

When tickets is injected in __init__, _ticket_store is already set. load() sees if self._ticket_store is None: is False and skips re-creating it — the injected instance (with its pre-wired callbacks) is preserved. The
test_load_reuses_an_injected_ticket_store test and the shared-instance invariant test together prove both sides of
this contract.

🟢 sa.tickets accessible without load() when tickets is injected — undocumented and untested

_require(_ticket_store, "tickets") returns the injected store before load() is called. This is intentional
(the test seam relies on it) but subtly inconsistent with the "load() required" contract that every other port
enforces. test_typed_port_access_before_load_raises only tests sa.threads — a matching test for sa.tickets
(no injection, before load()) would complete the matrix. The current behavior for injected tickets-before-load
is implicitly load-free by design, but it's not documented. A one-line docstring note on the tickets property
would prevent a future contributor from treating this as a bug.
Fix this →

---

jig/orchestrator.py

✅ startup() composition-root wiring is correct

StoreAuthority(self._project_path) is constructed and store.load() called in parallel with self.bus.load() and
self.analytics.load(). After the gather, the attribute aliases (self.tickets = store.tickets, etc.) are set from
the vended ports. Callbacks (set_status_change_callback, wire_create_publisher) are wired on the alias — which
IS store.tickets (same object) — so the shared-instance + callback invariant holds end-to-end. ✓

Note: startup() still does store_dir.mkdir(parents=True, exist_ok=True) even though load() also does it.
This is intentional — self.bus = MessageBus(store_dir / "messages.jsonl") is constructed from store_dir before
load() is called, so the mkdir is needed there. The double-mkdir with exist_ok=True is harmless.

✅ _handle_service_message extraction is correct

Moving the dispatch body to a named method makes it unit-testable without running the full event loop. The
tests/test_service_loop_dispatch.py suite proves all six dispatch paths. The isinstance(ticket_id, str) and ticket_id guard correctly rejects unhashable ids before they reach the scheduler's _running_tickets dict key. ✓

🟢 No test for raw (undecodable) ticket_updated with a valid string id

test_raw_partial_created_still_schedules proves the raw ticket_created fallback. The symmetric case — a
ticket_updated message that decode_event() can't decode (missing required fields) but whose raw ticket_id is a
valid string and status is "open" — has no dedicated test. The code path is exercised only by the
unhashable-id test, which proves the rejection path, not the happy-path scheduling. A one-line test would complete
the fallback symmetry.
Fix this →

---

docs/reference/adr-0001-runtime-store-access-routing.md

✅ ADR is well-structured and the decision rationale is sound

The "URIs for everything" alternative is correctly rejected at the right layer — runtime code needs typed models and
rich domain operations, not serialized dict dumps. The "URI delegates to typed" invariant (rule 3) closes the drift
gap that a plain provider model would leave open. The "runtime-capability split" (authoring-authority vs.
can_create_ticket) is correctly deferred while fixing the concept now. Migration approach is illustrative-not-
prescriptive, which is the right posture given the in-flight work.

✅ ADR location is correct

Moved from architecture/decision-store-access-routing.md to docs/reference/adr-0001-runtime-store-access-routing.md per repo convention (docs/reference/ for long-lived reference material and ADRs). The plan.md reference
(../docs/reference/adr-0001...) resolves correctly from architecture/. ✓

---

What's well done in this pass

• Parallel store loading via asyncio.gather(*to_load) in load(): correct — the six domain stores
  have no inter-store load dependency, so loading them in parallel is safe.
• Teardown completeness: self.store = None is added alongside the existing per-store None resets, so the
  composition root is correctly released.
• _require as a generic static method: TypeVar-based return type preserves the concrete type for each
  accessor property, avoiding Any erasure at call sites. Correctly uses @staticmethod (no self needed).
• test_uri_write_fires_the_vended_ports_create_callback: proves the deepest shared-instance invariant — a
  URI-door write fires a callback wired on the typed port. This is the key correctness property ADR-0001 depends on.
• test_typed_port_and_uri_door_share_one_ticket_store: bi-directional proof: create via typed port → read via
  URI door; write via URI door → read via typed port. Exactly the right integration test for the composition-root
  design.
• Service-loop hardening: isinstance(ticket_id, str) and ticket_id is the minimum correct guard — str rejects
  lists/dicts that would crash _running_tickets[ticket_id]; the truthy check rejects empty strings. Both applied
  symmetrically to ticket_created and ticket_updated raw paths.

---

Carried from prior reviews — still unresolved

🟡 No tombstone comment or test for (BuildPhase.MERGING, AgentCompleted) (12 passes)

jig/engines/build/decide.py:209–214 — _TRANSITIONS still has no entry for this pair. A late or duplicate
AgentCompleted while a ticket is MERGING silently no-ops via decide()'s fallthrough. The decide() docstring
says "An unknown (status, event) pair is a no-op" — but there is still no tombstone at the specific gap and no
test asserting the fallthrough for this pair. Now the longest-running open item at 12 passes.
Fix this →

🟡 Misleading comment in concurrent coordinator test (11 passes)

tests/engines/build/test_coordinator.py:114

await asyncio.sleep(0)  # let both reach dispatch before either commits

t1 holds the global asyncio.Lock inside gate.wait(); t2 blocks on the lock — serialized dispatch, not
concurrent. Still says "let both reach dispatch before either commits."
Fix this →

🟡 from_dict() stream construction outside the try/except KeyError block (since Pass 7)

jig/runtime/recorded.py:102–104

stream=tuple(
    JigEvent(type=e["type"], data=e["data"]) for e in data.get("stream", [])
),  # ← outside the try block; malformed stream entry raises KeyError, not ValueError

Fix this →

🟢 Inline imports in test_model_and_sub_models_are_immutable (since Pass 5)

tests/model/test_invariant_logic.py:85–86 — import pytest and from pydantic import ValidationError are still inside the function body (ruff PLC0415).
Fix this →

---

Summary

Epic 2 PR D Wave 1 lands correctly. The StoreAuthority composition-root pattern is mechanically sound, the
shared-instance invariant is proven by dedicated integration tests, and the service-loop hardening closes a real
crash path.

• 2 new minor findings: sa.tickets before load() is undocumented for the injected case; raw
  ticket_updated fallback has no happy-path test (only the rejection path is covered)
• 4 still-carried: (MERGING, AgentCompleted) tombstone + test (12 passes, longest-running); misleading
  concurrent test comment (11 passes); from_dict() stream error inconsistency (since Pass 7); inline imports
  in immutability test (since Pass 5)

Ready to merge. The four carried items are low-risk individually but should land before the next wave — they
will be progressively harder to find as the diff grows.

[claude]

Claude finished @brent-hoover's task in 4m 32s —— View job

---

PR Review: feat: Architecture 2.0 (Pass 13)

• Gather context and understand what's new since Pass 12
• Identify new commits since Pass 12
• Review new changes for correctness, async safety, type hints, test coverage
• Check status of previously flagged unresolved issues
• Summarize findings

---

What changed since Pass 12

Four commits (merged as PR #233) implementing Epic 2 MVP PR D Wave 2 — URI store reads routing through the typed TicketStore (ADR-0001 rule 3):

Commit	Summary
f6cbf49	feat(substrate): Epic 2 PR D — URI store reads project over the typed store (ADR-0001 rule 3)
2743c31	fix(uri): reject store before the cache in resolve_project_uri
aaf232c	refactor(uri): frozenset wired collections + reject jig-N ids on store reads
db07692	Merge pull request #233

Files changed: jig/substrate/store_authority.py (+53), jig/uri/resolver.py (+14/-9),
jig/uri/store.py (-63 net), tests/substrate/test_store_uri_read.py (new, +115),
tests/substrate/test_store_authority.py (+/-39), tests/test_uri.py (+21/-16),
tests/uri/test_store_resolver.py (deleted).

---

Overall Assessment

Clean wave. The architectural shift — eliminating the parallel JSONL replay in the URI read path and routing store reads through the typed TicketStore — is the right call (ADR-0001 rule 3). The resolve_project_uri now rejects store upfront, preventing any path by which a cached store entry could be served from the sync dispatcher. The new test file for the typed read path has 10 cases, and the jig-N rejection on reads is correctly symmetric with the write path. One new concern about async initialization, everything else is correct.

---

jig/substrate/store_authority.py

🟡 _tickets() initialization is not protected against concurrent async callers — and _read_store now calls it too

jig/substrate/store_authority.py:284–294

async def _tickets(self) -> "TicketStore":
    if self._ticket_store is None:
        store = TicketStore(self._root / ".jig" / "store" / "tickets.jsonl")
        await store.load()
        self._ticket_store = store
    return self._ticket_store

The check-and-set is not atomic across the await store.load() suspension point. Two concurrent callers (e.g. asyncio.gather(sa.write(...), sa.read(...)) on a fresh StoreAuthority that hasn't been load()-ed) can both enter the if branch, both construct a TicketStore, both call load(), and the second to complete overwrites the first. The caller that ran first holds an orphaned store instance not equal to self._ticket_store.

In production this is safe because:

1. StoreAuthority.load() is called at startup, which sets _ticket_store before any reads
2. write_addressed reloads under the cross-process flock on every write, so data correctness holds regardless

But the shared_instance invariant (same TicketStore instance backing both the typed port and the URI door) would be violated in this edge case. A one-line asyncio.Lock guard or an asyncio.Event-style "initialized once" pattern would close it. Alternatively, a docstring note on _tickets() that concurrent lazy-init callers are safe but may double-load would document the known trade-off.

Fix this →

✅ _read_store reload-per-read design is correct

await tickets.load() on every _read_store() call ensures the URI read reflects current persisted state (another process may have written since the in-memory snapshot was taken). The cost is O(JSONL file) per read, matching the prior disk-replay cost. The comment explains this accurately. The acknowledged "double-load on first call" (from _tickets() lazy-init + _read_store() reload) is a perf concern for later. ✓

✅ jig-N rejection on reads is symmetric with writes

_read_store checks _RESERVED_KEY_ID.match(parsed.path[1]) and raises ProjectUriError — exactly mirroring _write_store's rejection. test_jig_n_shaped_id_is_rejected_on_read pins this. ✓

---

jig/uri/resolver.py

✅ Store rejection before cache is the correct ordering

if parsed.authority == "store":
    raise ProjectUriError(
        "store reads are resolved by StoreAuthority.read (async, through the "
        "typed stores), not the sync project-URI dispatcher"
    )

The guard fires before cache.get(parsed), so a manually-populated or retained cache entry for project://store/... can never be served by the sync dispatcher. test_dispatcher_rejects_store_before_consulting_the_cache proves this with a pre-populated cache that must not be hit. ✓

✅ cacheable guard removed cleanly

With store rejected upfront, the cacheable = parsed.authority != "store" flag is dead. Both the cache-get and cache-put now apply unconditionally to whatever reaches those lines (only spec/arch/design/plan). The code is simpler and correct. ✓

---

jig/uri/store.py

✅ _WIRED_FILES → _WIRED_COLLECTIONS refactor is correct

The original dict[str, str] was only ever used for key membership (collection not in _WIRED_FILES); the value side was dead after resolve_store_uri was deleted. frozenset is the right type. ✓

✅ serialize_ticket is the right canonical serialization

ticket.model_dump(mode="json", by_alias=True) matches TypedCollection's persistence serialization, so the URI door and the typed store return identical shapes. Comment explains this. ✓

✅ _replay_jsonl and resolve_store_uri deletion is correct

The only caller was resolve_project_uri's store branch, which is now deleted. The new tests/substrate/test_store_uri_read.py covers the full read path through StoreAuthority.read. tests/uri/test_store_resolver.py is correctly deleted. ✓

---

Test coverage

tests/substrate/test_store_uri_read.py — 10 cases: single ticket, missing ticket (null data), ticket list, empty store, unwired collection, deep sub-path, fragment, revision pin, jig-N id rejection, and corrupt op-log surfaced via canonical load. Strong.

test_read_surfaces_a_corrupt_oplog_via_the_canonical_load proves the ADR-0001 property that there is one canonical replay path. ✓

test_dispatcher_rejects_store_before_consulting_the_cache is an important security-boundary test — it would have caught the earlier bug (found and fixed in 2743c31) where a stale cache hit could bypass the store routing. ✓

---

Carried from prior reviews — still unresolved

🟡 No tombstone comment or test for (BuildPhase.MERGING, AgentCompleted) (13 passes)

jig/engines/build/decide.py:209–214

_TRANSITIONS still has no entry for (BuildPhase.MERGING, AgentCompleted). A late or duplicate AgentCompleted while a ticket is MERGING falls through decide()'s fallthrough path at line 241–242 and silently no-ops. The decide() docstring says "An unknown (status, event) pair is a no-op" — helpful — but there is still no tombstone at the specific gap and no test asserting the fallthrough for this pair. 13 passes.

Fix this →

🟡 Misleading comment in concurrent coordinator test (12 passes)

tests/engines/build/test_coordinator.py:114

await asyncio.sleep(0)  # let both reach dispatch before either commits

t1 holds the global asyncio.Lock inside gate.wait(); t2 blocks on the lock. The test exercises serialized dispatch, not concurrent dispatch. Still says "let both reach dispatch before either commits."

Fix this →

🟡 from_dict() stream construction outside the try/except KeyError block (since Pass 7)

jig/runtime/recorded.py:102–104

stream=tuple(
    JigEvent(type=e["type"], data=e["data"]) for e in data.get("stream", [])
),  # ← outside the try block; malformed stream entry raises KeyError, not ValueError

Fix this →

🟢 Inline imports in test_model_and_sub_models_are_immutable (since Pass 5)

tests/model/test_invariant_logic.py:85–86 — import pytest and from pydantic import ValidationError are still inside the function body (ruff PLC0415).

Fix this →

🟢 Pass 12 items not yet addressed

• sa.tickets before load() is undocumented for the injected case — test_typed_port_access_before_load_raises only tests sa.threads; no test covers sa.tickets without injection before load(). Fix this →
• Raw ticket_updated fallback missing happy-path test — test_service_loop_dispatch.py tests the raw ticket_created fallback but not the symmetric ticket_updated raw path. Fix this →

---

What's well done in this pass

• ADR-0001 rule 3 enforced mechanically: the _replay_jsonl deletion removes the only path by which the URI door and typed store could disagree on schema/state. There is now one canonical JSONL reader per store type.
• test_read_surfaces_a_corrupt_oplog_via_the_canonical_load: the exactly right test for the "one canonical replay" claim — proves a corrupt op-log surfaces as an error through StoreAuthority.read, not a silently mangled dict.
• test_dispatcher_rejects_store_before_consulting_the_cache: catches the specific bug 2743c31 fixed. Good regression anchor.
• resolve_project_uri docstring updated: the new "declared artifacts only" framing is accurate and tells the reader where to go for store reads.
• _WIRED_COLLECTIONS as frozenset: the right type for a membership-only set. Cleaner than a dict where the value side is ignored.
• Test file migration: tests/uri/test_store_resolver.py deleted (the sync-replay path no longer exists), tests/substrate/test_store_uri_read.py added (the typed-store path). The test location correctly follows the code boundary.

---

Summary

Epic 2 PR D Wave 2 (URI read delegation through typed stores) is clean and correct. The elimination of _replay_jsonl enforces ADR-0001's one-canonical-load rule mechanically. The resolve_project_uri cache-bypass fix closes a correctness gap that was one unguarded cache put away from serving stale store data.

• 1 new finding: _tickets() concurrent init race — safe in production (always call load() first), but the shared-instance invariant is not guaranteed for concurrent lazy-init callers. Worth a docstring note.
• 5 carried from prior passes: (MERGING, AgentCompleted) tombstone + test (13 passes); misleading concurrent test comment (12 passes); from_dict() stream error inconsistency (since Pass 7); inline imports in immutability test (since Pass 5); two Pass-12 nits still unaddressed.

Ready to merge.

[claude]

Claude finished @brent-hoover's task in 4m 35s —— View job

---

PR Review: feat: Architecture 2.0 (Pass 14)

• Gather context and identify new commits since Pass 13
• Review new changes for correctness, async safety, type hints, test coverage
• Check status of previously flagged unresolved issues
• Summarize findings

---

What changed since Pass 13

Two commits (merged as PR #235) implementing Epic 3 MVP PR 1 — routing orchestrator spawns through the RunAgent seam — plus a doc-only fix:

Commit	Summary
5d1f012	feat(runtime): Epic 3 MVP PR 1 — route orchestrator spawns through the RunAgent seam
b0d23b1	docs(orchestrator): fix dangling 'Bones' fragment + stale Track-E provisioning semantics

Files changed: jig/orchestrator.py (+20/-13), jig/runtime/real.py (+3/-1), tests/runtime/test_orchestrator_seam.py (new, +39), and 34 monkeypatch sites across 14 test files updated.

---

Overall Assessment

Clean execution of the evaluability pivot. The self._run_agent injection point lands correctly, the default RealRunAgent is a transparent wrapper (emitter is captured at construction, attribute access preserves monkeypatching), and the 34-site test migration is behavior-preserving. The warnings tolerance fix is the right defensive measure for non-standard RunAgent implementations. One stale-doc finding below, everything else is correct.

---

jig/orchestrator.py

✅ run_agent parameter doesn't shadow a module-level name

The old from jig.agent import run_agent is removed. The new run_agent: "RunAgent | None" = None parameter name is safe — no shadowing hazard at the module or class level. ✓

✅ RealRunAgent emitter capture is correct

RealRunAgent(emitter=emitter) is constructed in __init__ with the same emitter passed to the orchestrator. RealRunAgent.__call__ calls agent_mod.run_agent(ctx, self._emitter) — positional argument binds to the emitter parameter of jig.agent.run_agent(ctx, emitter=None). Contract-correct. ✓

✅ Monkeypatching "jig.agent.run_agent" reaches RealRunAgent

RealRunAgent.__call__ uses agent_mod.run_agent (attribute access on the imported module object), so monkeypatch.setattr("jig.agent.run_agent", ...) patches the attribute that RealRunAgent reads. The comment in real.py explains this explicitly. The 34-site migration is mechanically correct. ✓

✅ result_status = "failed" default is safe on exception

result_status is initialized to "failed" before the try block in _run_agent_with_analytics. If self._run_agent(ctx) raises, the finally block still fires with a meaningful default (no NameError, no misleading "success" cleanup). ✓

✅ AgentRunResult is a field-superset of RunAgentResult

All callers of _run_agent_with_analytics that use the result access only .status. AgentRunResult.status: str satisfies all call sites. The _run_review_federation method still returns RunAgentResult from jig.agent directly (not through the seam), which is fine — it's a different code path. ✓

🟢 _phase5p_helpers.py:41 docstring is now stale

# tests/_phase5p_helpers.py:41
# The caller is still responsible for ``monkeypatch.setattr(
# orch_module, "run_agent", ...)`` and for calling
# ``orch.startup()`` / ``orch.shutdown()``.

run_agent is no longer on the orchestrator module — all callsites have correctly migrated to monkeypatch.setattr("jig.agent.run_agent", ...). The helper's own docstring still points callers at the old pattern. A reader setting up a new phase5p test would follow the wrong instructions. One-line fix: update orch_module, "run_agent" to "jig.agent.run_agent".

Fix this →

---

jig/runtime/real.py

✅ warnings tolerance is correctly implemented

warnings=list(getattr(result, "warnings", None) or []),

getattr(..., None) handles a result without a warnings attribute at all. or [] handles None (explicit absence). For standard RunAgentResult, warnings is always a list[str] with a default factory, so this is a no-op in production. The comment references SF-I5 correctly. ✓

---

tests/runtime/test_orchestrator_seam.py

✅ Injection contract is correctly pinned

Three tests cover: default is RealRunAgent, injected agent is used, and the seam returns the injected result. The ctx = object() trick works because FixtureRunAgent is context-agnostic by design. The module docstring explains why the full integration path is covered by the existing suite rather than here. Acceptable and honest. ✓

---

Carried from prior reviews — still unresolved

🟡 No tombstone comment or test for (BuildPhase.MERGING, AgentCompleted) (14 passes)

jig/engines/build/decide.py:207–214

_TRANSITIONS still has no entry for (BuildPhase.MERGING, AgentCompleted). The decide() docstring says "An unknown (status, event) pair is a no-op" — but no tombstone at the specific gap and no test for this pair. The longest-running open item.

Fix this →

🟡 Misleading comment in concurrent coordinator test (13 passes)

tests/engines/build/test_coordinator.py:115

await asyncio.sleep(0)  # let both reach dispatch before either commits

t1 holds the global asyncio.Lock inside gate.wait(); t2 blocks on the lock — serialized dispatch, not concurrent.

Fix this →

🟡 from_dict() stream construction outside the try/except KeyError block (since Pass 7)

jig/runtime/recorded.py:102–104 — JigEvent(type=e["type"], data=e["data"]) is outside the try block; a malformed stream entry raises KeyError, not ValueError, inconsistent with the missing status/final_text error path.

Fix this →

🟢 Inline imports in test_model_and_sub_models_are_immutable (since Pass 5)

tests/model/test_invariant_logic.py:84–85 — import pytest and from pydantic import ValidationError are still inside the function body (ruff PLC0415).

Fix this →

🟡 _tickets() concurrent async init race (since Pass 13)

jig/substrate/store_authority.py:284 — Two concurrent async callers before load() can both enter the if self._ticket_store is None: branch across the await store.load() suspension point. Safe in production (always call load() first), but the shared-instance invariant isn't guaranteed for concurrent lazy-init callers. Warrants a docstring note.

🟢 sa.tickets before load() undocumented for the injected case (since Pass 12)

jig/substrate/store_authority.py — test_typed_port_access_before_load_raises only tests sa.threads; no test covers sa.tickets (without injection) before load().

🟢 Raw ticket_updated fallback missing happy-path test (since Pass 12)

tests/test_service_loop_dispatch.py — The raw ticket_created fallback has a test but the symmetric raw ticket_updated with valid string id and status="open" does not.

---

What's well done in this pass

• Seam migration is minimal and safe: only __init__ grows a new parameter, only _run_agent_with_analytics changes its call site. The rest of the orchestrator is untouched.
• 34-site test migration is behavior-preserving: all tests now patch "jig.agent.run_agent", which RealRunAgent reads via attribute access — the tests exercise the seam without knowing it.
• Doc fix is accurate: the Track E correction ("provisioning failures mark the ticket failed (SF-1) rather than running against default services") matches the code at lines 621–637. The prior docstring was wrong.
• Module docstring in real.py: correctly notes this module imports the full agent stack and must not be re-exported from jig.runtime. The Bones/MVP/Final trajectory is documented.

---

Summary

Epic 3 MVP PR 1 is correct. The RunAgent seam is wired, the injection contract is pinned, and the test migration is clean.

• 1 new minor finding: stale _phase5p_helpers.py:41 docstring still points at the old orch_module + run_agent pattern
• 7 still-carried: (MERGING, AgentCompleted) tombstone + test (14 passes); misleading concurrent test comment (13 passes); from_dict() stream error inconsistency (since Pass 7); inline imports in immutability test (since Pass 5); _tickets() concurrent init race note (since Pass 13); two Pass-12 nits

Ready to merge.

[claude]

Claude finished @brent-hoover's task in 4m 10s —— View job

---

PR Review: feat: Architecture 2.0 (Pass 15)

• Gather context and identify new commits since Pass 14
• Review new changes for correctness, async safety, type hints, test coverage
• Check status of previously flagged unresolved issues
• Summarize findings

---

What changed since Pass 14

Three commits (merged as PR #236) implementing Epic 3 MVP PR 2 — the runtime owning per-agent MCP server assembly:

Commit	Summary
20cfc48	feat(runtime): Epic 3 MVP PR 2 — runtime owns per-agent MCP server assembly
02f14ae	fix(runtime): harden resolve_external_mcps + precise dict types
67e0771	Merge pull request #236

Files changed: jig/runtime/mcp.py (+178, new), jig/agent.py (-143 net), tests/runtime/test_agent_mcp_assembly.py (+164, new), tests/test_agent_streaming.py (+4/-0 net), tests/test_review_invocation_shaping.py (+8/-8 net).

---

Overall Assessment

Clean boundary extraction. build_agent_mcp_servers, resolve_external_mcps, and effective_ticket_base_ref move out of agent.py into jig/runtime/mcp.py, making the "runtime owns the MCP lifecycle" boundary real in the dependency graph. The call site in run_agent reduces from ~50 lines to one. The hardening fix correctly adds isinstance guards for malformed configs at both the user-level and plugin-level paths. Two minor observations; carried items unchanged.

---

jig/runtime/mcp.py

✅ Malformed config guards are correct and symmetric

User-level path:

root = json.loads(user_mcp.read_text())
servers = root.get("mcpServers", {}) if isinstance(root, dict) else {}
if not isinstance(servers, dict):
    servers = {}

Plugin-level path:

servers = json.loads(mcp_json.read_text())
if not isinstance(servers, dict):
    _logger.warning("Ignoring %s: root is not an object", mcp_json)
    break

Both guard against non-dict roots after JSON parse. The break on the plugin-level malformed case skips remaining versions of that plugin — same behavior as before the fix, since break was always outside the try block. Not a regression.

✅ _patch_home monkeypatch is more robust than patching Path.home directly

class _Path(type(home)):
    @classmethod
    def home(cls) -> Path:
        return home

monkeypatch.setattr(mcp_mod, "Path", _Path)

Inheriting from type(home) (the concrete platform Path subtype) means all other Path operations on _Path instances work correctly. Patching the module-level Path attribute rather than stdlib.Path.home avoids affecting other code that calls Path.home() outside the module under test. Exactly right.

✅ Isolation fitness tests are the right design

Two subprocess tests check that importing jig.runtime.mcp doesn't drag in jig.orchestrator / jig.ws_server / jig.container / jig.daemon or the TUI package. The pattern matches the existing test_the_path_is_headless_no_legacy_stack in the edge layer — consistent and correct.

✅ can_waive threading is correctly preserved

build_agent_mcp_servers(ctx, can_waive=cap.can_waive) in run_agent passes can_waive through to create_agent_mcp_server inside the new assembly function. The test_run_agent_threads_can_waive_from_materialization_into_mcp_factory spy test in test_agent_streaming.py now patches "jig.runtime.mcp.create_agent_mcp_server" — the correct target after the extraction. ✓

🟢 Plugin-path .mcp.json adds all bundled server names, not just the allowed one

jig/runtime/mcp.py:103–105

for server_name, config in servers.items():
    result[server_name] = config  # adds ALL keys from the plugin's .mcp.json
remaining.discard(name)

The user-level path (~/.claude/.mcp.json) adds only the explicitly named server (result[name] = servers[name]). The plugin-level path adds every key in the plugin's .mcp.json, regardless of what was in allowed_mcps. A plugin named weather whose .mcp.json declares {"weather": {...}, "weather-history": {...}} would inject both servers — only weather was allowed.

This is unchanged from the pre-extraction _resolve_external_mcps in agent.py, so it's not a regression introduced here. The format asymmetry between user and plugin files (user-level nests under "mcpServers"; plugin files use flat {server_name: config}) is also pre-existing. Worth a docstring note on the plugin-path format to help future plugin authors understand the contract.
Fix this →

🟢 No test for plugin cache path resolution

tests/runtime/test_agent_mcp_assembly.py

test_build_merges_external_mcps tests the user-level ~/.claude/.mcp.json path. The plugin cache path (~/.claude/plugins/cache/*/*/.mcp.json) is not directly tested in the new file. The _patch_home helper is already extracted and usable, so adding a test for the plugin resolution path would be straightforward. Low priority — the logic is a port from agent.py — but the hardened isinstance guard introduced in 02f14ae on that path specifically has no regression test.
Fix this →

---

Test migration is correct

• tests/test_agent_streaming.py:552 — monkeypatch.setattr("jig.runtime.mcp.create_agent_mcp_server", _spy) correctly targets the factory call site after extraction. ✓
• tests/test_review_invocation_shaping.py — from jig.runtime.mcp import effective_ticket_base_ref replaces the private from jig.agent import _effective_ticket_base_ref. The function is now public API with an identical signature. ✓

---

Carried from prior reviews — still unresolved

🟡 Stale docstring in _phase5p_helpers.py (since Pass 14)

tests/_phase5p_helpers.py:40–41

The caller is still responsible for ``monkeypatch.setattr(
orch_module, "run_agent", ...)`` and for calling

run_agent is no longer on orch_module — all 34 call sites now patch "jig.agent.run_agent". A contributor following this helper's instructions would use the wrong target.
Fix this →

🟡 No tombstone comment or test for (BuildPhase.MERGING, AgentCompleted) (15 passes)

jig/engines/build/decide.py:209–214 — _TRANSITIONS still has no entry for this pair. A late or duplicate AgentCompleted while a ticket is MERGING silently falls through decide()'s fallthrough path at line 241–242 and returns the current state unchanged. The decide() docstring says "An unknown (status, event) pair is a no-op" — but no tombstone at the specific gap and no test asserting the fallthrough for this pair.
Fix this →

🟡 Misleading comment in concurrent coordinator test (14 passes)

tests/engines/build/test_coordinator.py:114

await asyncio.sleep(0)  # let both reach dispatch before either commits

t1 holds the global asyncio.Lock inside gate.wait(); t2 blocks on the lock — serialized dispatch, not concurrent. The comment still says "let both reach dispatch before either commits."
Fix this →

🟡 from_dict() stream construction outside the try/except KeyError block (since Pass 7)

jig/runtime/recorded.py:102–104

stream=tuple(
    JigEvent(type=e["type"], data=e["data"]) for e in data.get("stream", [])
),  # ← outside the try block; malformed stream entry raises KeyError, not ValueError

Fix this →

🟢 Inline imports in test_model_and_sub_models_are_immutable (since Pass 5)

tests/model/test_invariant_logic.py:85–86 — import pytest and from pydantic import ValidationError are still inside the function body (ruff PLC0415).
Fix this →

---

What's well done in this pass

• Boundary is now real in the dependency graph: agent.py drops from ~50 lines of MCP assembly to one call. The jig.runtime.mcp module docstring explains the evaluability rationale precisely — "the piece a headless, daemon-free pipeline needs to control or substitute."
• analytics_emitter cast is documented: cast("EventEmitter | None", ctx.analytics_emitter) avoids arg-type suppression while honestly narrowing the loose object | None type. The inline comment explains why the type is loose on the context.
• frozenset(ctx.phase.questions_to) guard: correctly handles None phase (standalone spawns, tests) with the if ctx.phase else frozenset() pattern — same as the original code in agent.py.
• __all__ on mcp.py: lists the three public functions. Consistent with other runtime module exports.
• jig/runtime/__init__.py is not burdened: mcp.py imports jig.mcp_server (heavyweight), so it is correctly NOT re-exported from the package root, preserving the cheap import for runtime's many consumers. Matches the same rationale as jig.runtime.real.

---

Summary

Epic 3 MVP PR 2 is correct. The MCP assembly boundary extraction is clean, the isolation fitness tests are in place, and the malformed-config hardening fix is correctly applied.

• 2 new minor observations: plugin .mcp.json format asymmetry undocumented; plugin cache path has no regression test for the isinstance guard added in 02f14ae
• 5 still-carried: stale _phase5p_helpers.py docstring (Pass 14); (MERGING, AgentCompleted) tombstone + test (15 passes, longest-running); misleading concurrent test comment (14 passes); from_dict() stream error inconsistency (since Pass 7); inline imports in immutability test (since Pass 5)

Ready to merge.