Architecture Review Report — 2026-09-01

Mode: /architecture-review (full) · Verdict: CONCERNS → all findings applied same session; ADR-0013 + ADR-0014 now ACCEPTED
Engine: Unity 6.3 LTS (6000.3) · URP 2D (Render Graph) · Shader Graph 17.x · NGO 2.x · Input System 1.20 · Addressables 2.x · UI Toolkit
Loaded: 10 ADRs (8 Accepted at review time; ADR-0013 + ADR-0014 reviewed as Proposed and Accepted at the end of this session) · tr-registry.yaml (111 IDs) · traceability-index.md · architecture.md · engine reference (VERSION, breaking-changes, deprecated-apis, current-best-practices, modules) · art bible §4.3/§7/§8 · 7 APPROVED UX specs · docs/registry/architecture.yaml
Scope: incremental over architecture-review-2026-08-28.md. Two new ADRs reviewed in full; the prior review's blocking items re-verified against source, not against the session log.
Method: structured extraction of both new ADRs; an independent cross-ADR conflict pass; two independent engine audits whose verdicts on ADR-0013's load-bearing claim conflicted and were adjudicated by the reviewer against Unity's primary documentation (see Engine Compatibility).

Prior Review's Blocking Items — Verified Closed

Each item was checked against the file, not the changelog.

Pre-gate checklist is now fully green (was 4/4 ❌ at the last review): tests/unit/, tests/integration/, .github/workflows/tests.yml, design/accessibility-requirements.md, design/ux/interaction-patterns.md all present.

Traceability Summary

Coverage as assessed at review time, when both new ADRs were still Proposed — credit is granted on Acceptance, not authorship. Both were Accepted later in this session; the final numbers are 73 / 26 / 12 (see Resolution below and traceability-index.md).

Foundation layer: 0 gaps (unchanged).

No new TR-IDs were surfaced by this review. Both ADRs address requirements already registered on 2026-08-28; tr-registry.yaml is therefore unmodified — all 111 IDs keep their numbers, text and revised dates.

Projected coverage on Acceptance of both ADRs (actual outcome: 73 / 26 / 12 — TR-tide-008 was already ✅ and merely shed its remainder, and TR-ui-002 landed ⚠️ not ✅)

• 7 gaps closed: TR-tint-001, TR-tint-002, TR-tint-003, TR-bb-009 (ADR-0013); TR-ui-001, TR-ui-002, TR-ui-003 (ADR-0014).
• 3 partials resolved: TR-cam-001 (→ ADR-0013), TR-ui-004 and TR-combat-013 (→ ADR-0014).
• TR-tide-008's "driver → ADR-0013" remainder closes.
• TR-ui-002 closes on two of three limbs by the ADR's own declaration — the world-space ping needs an ADR-0007 Ping binding that does not exist. Verified: zero occurrences of a Ping action in ADR-0007.

Both ADRs match their architecture.md Required-ADR charters (rows 13 and 14) exactly — no scope drift in either direction.

Cross-ADR Conflicts

F1 — ADR-0014 vs ADR-0005 · World-space UI atlas · CONCERN — ✅ RESOLVED THIS SESSION

• ADR-0014 §9 claimed: world-space UI sprites and TMP text "are sorted with the DriftLayer sprites and share the sprite atlas, so they batch with the 2D content rather than adding a draw call each"; restated in Performance Implications as "No per-element draw call."
• ADR-0005 claims (Accepted): "the UI atlas is separate and non-addressable" (§2); each DriftLayer SpriteAtlas is packed inside the per-template Land_<id> Addressables group and "is referenced by exactly one addressable (this prefab), which is what makes the setup free of the duplicate-texture pitfall"; UI has its own import gate (sources ≤ 128²) distinct from land atlases (BC3, ≤ 2048²) and the home atlas (BC7, ≤ 4096²). art-bible.md §8.A.5 independently specifies the same three atlas classes.
• Impact: implemented literally, UI sprites would live inside a bundle that unloads at LandTornDown — flyouts and ally indicators lose their sprites at teardown — and ADR-0005's single-consumer invariant breaks. Implemented correctly (UI in its own atlas), the zero-draw-call claim is simply false. Either way the budget is safe (< 100 with a total near 40); the defect was a wrong justification and a latent lifetime hazard, not a blown budget.
• Note: ADR-0013 §8 hit this exact trap and handled it correctly, naming the home atlas explicitly and writing "Batching with 'the atlas' is only true of the right one." ADR-0014 — authored in the same session — made the generic mistake its sibling warned against.
• Resolution applied (user-approved, 2026-09-01; ADR-0014 is Proposed, so no Accepted document was amended): §9 and Performance Implications now name the separate non-addressable UI atlas and register the honest ≈ +2 draw calls, fixed (UI atlas + one shared TMP font-atlas material). New V7 measures it as a marginal delta that must not scale with element count — a per-element cost means the UI atlas is fragmented or TMP is not sharing one font-atlas material. Recorded in a new Amendment History section. The ms budgets are untouched: ui-worldspace 0.2 traces to hud.md's own performance AC and never depended on the atlas claim.

F2 — ADR-0013 vs ADR-0005/ADR-0003 · LandTornDown signature · NOTE

• ADR-0013 §5 subscribes to LandTornDown(socket) and calls TintFloats(0) using a bare int.
• ADR-0005 declares event Action<SocketId> LandTornDown; ADR-0003 (TD-ADR C9) makes SocketId the representation "everywhere" — while ADR-0001's ITideClock.TintFloats(int socket) genuinely takes int.
• Impact: the driver needs a .Value unwrap. Compile-time catch. Worth one sentence so a reader does not "fix" the wrong side and break ITideClock's Accepted signature.

Checked — no conflict

ADR Dependency Order

Foundation (Accepted):
  1. ADR-0001 Tide Scheduler pure assembly
  2. ADR-0002 Host commit model
  3. ADR-0003 Assembly + purity boundaries
  4. ADR-0006 Game data loading + validation
  5. ADR-0004 Save format + reconcile pipeline
  6. ADR-0005 Land lifecycle on Addressables
  7. ADR-0007 Input action map
Core (Accepted):
  8. ADR-0015 Combat model
Presentation (Proposed — this review):
  9. ADR-0013 Tide-tint render pipeline   (← 0001, 0003, 0005)
 10. ADR-0014 UI stack                    (← 0003, 0004, 0006, 0007)
Not yet written:
  ADR-0008 Netcode wire (playtest-gated) → 0009, 0010, 0011, 0012

No cycles. No Proposed ADR depends on another Proposed ADR — both sit cleanly on the Accepted foundation, so either can be Accepted independently of the other.

GDD Revision Flags (Architecture → Design Feedback)

All three are art-bible.md edits already scheduled inside ADR-0013's "Pending on Acceptance" block. They are listed here because the review independently confirmed each against the art bible's own text.

No systems-index status changes are required — rows 13 and 14 remain Designed, and neither ADR invalidates a GDD design decision.

Engine Compatibility

Audit Results

Engine: Unity 6.3 LTS (6000.3) · ADRs with an Engine Compatibility section: 10 / 10 · Version agreement: 10 / 10 Deprecated API references: none in binding context. Every hit in the two new ADRs (SetupRenderPasses, AddRenderPasses, AccessibilityNode.selected) is in explicit "deprecated — do not use" or "renamed to" framing. ADR-0014 correctly targets AccessibilityNode.invoked, the 6.3 replacement, not the deprecated .selected. Stale version references: none.

⚠️ E14 — ADR-0013 §1 names the wrong Shader Graph control · BLOCKING (pre-Acceptance)

This finding required adjudication. Two independent engine audits returned opposite verdicts on the load-bearing mechanism of ADR-0013. The reviewer resolved it against Unity's primary documentation rather than accepting either.

What ADR-0013 §1 says: the two globals must be blackboard properties with "Show in Inspector" unchecked, on the reasoning that such a property "is hidden from the Material Inspector (Shader Graph emits [HideInInspector]) and, never being set per-material, falls through the precedence chain to the global."

What the primary sources establish:

• Shader.SetGlobalColor (6000.3): "Global properties are used if a shader needs them but the material does not have them defined (for example, if the shader does not expose them in Properties block)."
• Shader Graph 17.0 Property Types: "Show In Inspector" — "If you disable this option, it includes an [HideInInspector] attribute to the material property." It remains a material property, merely hidden.
• The same page documents a separate control, "Scope", with values Global / Per Material / Hybrid Per Instance — Global: "Makes the property editable at a global level, through a C# script only."

Therefore: visibility and declaration scope are two different controls. [HideInInspector] keeps the property in the shader's Properties block, so the material does have it defined, so — by the precedence rule ADR-0013 itself quotes correctly — the material value wins and Shader.SetGlobal* is a silent no-op. This is precisely the failure mode the ADR was written to prevent; §1 currently prescribes it.

The correct mechanism is Scope = Global on the blackboard property. (One audit named this control "Override Property Declaration → Shader Declaration", which is older/source-level terminology; in the pinned Shader Graph 17.x manual the control is "Scope". The substance is identical.)

Knock-on corrections required with the fix:

• The §1 editor test asserts the wrong condition. As written it asserts Shader.GetPropertyFlags returns HideInInspector — which a broken (Per Material + hidden) shader also satisfies, so the gate would pass on the bug it exists to catch. It must instead assert the property is absent from the material property block entirely (FindPropertyIndex returns −1 for a Global-scope property). That inverts the assertion into a genuine gate. Note also that FindPropertyIndex/GetPropertyFlags are instance methods on a Shader object, not statics.
• §1's terminology mitigation conflates two things. It anticipates a future rename by naming "the behaviour (un-exposed / not material-serialized)" as the rule — but those are exactly the two properties this finding separates. After the fix the rule should name only "declared Global scope / not material-serialized."
• V2 gains real weight. Changing a property's scope alters the UnityPerMaterial CBuffer layout, which is SRP-batcher-relevant. Global is the batching-safe direction (the value leaves the per-material buffer), but V2 must now be run specifically against the Global-scope shader, not treated as a formality.

Why this is not a FAIL-level finding: everything around §1 is right — the precedence rule is quoted correctly, the failure mode is real and correctly rated, the rule-plus-build-failing-test response is the right shape, and V1 is scoped to catch exactly this during an hour-long spike scheduled as migration step 1. The error is narrow: it names the wrong checkbox. It is BLOCKING only because §1 is a binding authoring rule scheduled to be copied into art-bible.md §8.B.3 on Acceptance and cited as the basis of a forbidden registry stance — so an uncorrected §1 propagates a wrong instruction to artists and to the next global anyone adds.

Engine Specialist Findings

ADR-0014 is clean on all of its engine claims (two wording tightenings, no errors). ADR-0013 is clean on seven of eight, with the one exception being its load-bearing mechanism.

Process item (carried, E10): modules/rendering.md still contains a pre-Render-Graph AddRenderPasses(ScriptableRenderer, ref RenderingData) signature and 6000.0 URLs; modules/ui.md is likewise undated for 6.3. Both need the DEPRECATED — DO NOT COPY banner already applied to networking.md and input.md. ADR-0013 §6 flagged rendering.md independently.

Architecture Document Coverage

• Both new ADRs map to existing architecture.md Required-ADR rows (13, 14) with no orphaned architecture and no uncovered system.
• architecture.md row 13's named verification — "verify Shader Graph 2D global-property read in 6.3" — was executed by this review and changed the design (E14). This is the HIGH-risk item the architecture document demanded be closed before implementation; it is now closed with a correction pending.
• Carried housekeeping (pre-existing, not caused by either ADR): systems-index.md still has two rows numbered 15 (Tide-Tint Polish; Base-Defense Events) — flagged 2026-08-28, still open. ADR-0015 registers no per-frame ms budget. docs/consistency-failures.md does not exist (not created — the skill appends only to an existing file).

Verdict: CONCERNS

Both ADRs are well-constructed: dependencies rest entirely on Accepted decisions, the HIGH-risk rendering surface is avoided rather than navigated, and the "Pending on Acceptance" discipline is correct — every amendment target named by both ADRs was verified to exist and to read as claimed, and ADR-0014's "Blocked On" table is honest (confirmed: no ReadHeader/createdUtc/lastPlayedUtc/CreateWorld in ADR-0004, no Ping action in ADR-0007, no ADR-0010).

Off PASS for one engine error in a load-bearing rule (E14/E15) and two wording tightenings (E16, E17), plus carried housekeeping. Not FAIL: no Foundation gap, no cross-ADR contradiction survives (F1 was fixed this session), and E14's fix is a checkbox and an inverted test assertion — it disturbs nothing else in the decision.

✅ Resolution — all findings applied 2026-09-01 (same session)

Every blocking and concern-level finding below was applied and both ADRs were flipped to Accepted. Coverage moved 64 / 28 / 19 → 73 / 26 / 12.

Amendment sets applied on Acceptance (previously held in both ADRs' "Pending on Acceptance" blocks): art-bible.md ×3 (§4.3 N > 1 caveat, §8.B.3 Global-scope precondition, §8.A.4 telegraph citation) · adr-0006 (ui.json in §1 file list, Configs.Ui, stage 9 with ranges as warnings) · adr-0007 §3 (title-screen map state) · accessibility-requirements.md (OQ 1 resolved, new Known Intentional Limitation for world-space text, UI-scaling row annotated) · architecture.md (header, rows 13/14 → Accepted, QQ-06 closed) · UX spec OQ closures (settings 4/5/6/10, hud 7, title-screen 10) · technical-preferences.md ADR log + 4 new forbidden patterns.

Registry stances registered (docs/registry/architecture.yaml, YAML validated): 4 interfaces (tide_tint_driver, settings_service, panel_manager, reason_selector), 2 api decisions (tide_tint_globals, ui_framework_topology), 5 forbidden patterns (mpb_on_driftlayer_sprite, exposed_shader_global_read, non_render_graph_pass, baked_tide_state_variant, ui_event_system_present), 2 budgets (ui-panels 0.0, ui-worldspace 0.2 — total 1.5 ms of 16.6).

Blocking issues — required before ADR-0013 flips to Accepted (all applied — see above)

• §1 mechanism — replace "Show in Inspector unchecked" with Scope = Global on both blackboard properties. Reword the terminology mitigation to name declared-Global / not-material-serialized, not un-exposed.
• §1 editor test — invert the assertion to Shader.FindPropertyIndex(...) == -1 (property absent from the material property block); note the methods are instance, not static.
• V2 scope — state that it runs against the Global-scope shader specifically, because changing scope alters the UnityPerMaterial layout.
• §4 (E16) — restate the SpriteRenderer UV2 premise as "very likely unavailable without a custom mesh renderer," so the Tier-1 spike starts from the right question.
• (Recommended, F2) — state the SocketId → .Value unwrap in §5.

Required before ADR-0014 flips to Accepted

• §10 (E17) — scope the "map name is not asserted" claim to "because this project explicitly reassigns the UI Actions pointer," rather than as a general engine fact.
• F1 is already applied.

Required ADRs (unchanged priority)

• ADR-0008 Netcode wire — playtest-gated; resolves 9 ⚠️ + 2 ❌ + TR-tint-004.
• ADR-0010 Inventory replication — 5 items incl. TR-inv-007; also unblocks ADR-0014's who-took-what tags.
• ADR-0009 Player transform authority; 4. ADR-0011 Structure registry; 5. ADR-0012 Enemy simulation.

Pre-gate checklist (checked 2026-09-01)

• ✅ tests/unit/, tests/integration/
• ✅ .github/workflows/tests.yml
• ✅ design/accessibility-requirements.md
• ✅ design/ux/interaction-patterns.md

All pre-gate artifacts are present. /gate-check pre-production is reachable once the two ADRs are Accepted.

History