fix: align layering claims, remove app-log ineffective dynamic import, refresh architecture records (#1222)

[devin-ai-integration]

Summary

Aligns architecture enforcement and records with the repository’s actual scope and shipped state.

• Classifies every production zone as ranked or intentionally unranked. R1–R4 remain global; ranked back-edge rejection applies only when both endpoints are in TARGET_DAG_RANK.
• Routes request-scoped app-log provider composition through app-log-request-scope.ts, removing the specific ineffective-dynamic-import warning without claiming app-log is lazy: app-log.ts remains eager through observability and teardown.
• Refreshes ADR 0009 and ADR 0012. ADR 0012 preserves its historical audit context, records steps 1–6 and agent-supervised repair step 8 as shipped (#1228), and keeps benchmark step 7 deferred.
• Refreshes stale experiment references and clarifies the ranked-spine diagram as ordering, not a claim that every displayed import exists.

Validation: format, typecheck, lint, layering, Fallow, build, unit/coverage, provider integration, and git diff --check pass. Local check:affected --run completes all runnable deterministic gates but cannot start SkillGym because this environment lacks the claude executable.

Link to Devin session: https://app.devin.ai/sessions/b1c3093bf2b248ca82a1a5df89152d02
Requested by: @thymikee

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.7 MB	1.7 MB	-150 B
JS gzip	538.5 kB	538.5 kB	-46 B
npm tarball	648.2 kB	648.2 kB	-56 B
npm unpacked	2.3 MB	2.3 MB	-150 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	28.3 ms	29.9 ms	+1.6 ms
CLI --help	60.2 ms	60.5 ms	+0.3 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/internal/daemon.js	-224 B	-116 B
dist/src/session.js	0 B	-12 B
dist/src/apps.js	0 B	+2 B
dist/src/selector-runtime.js	0 B	-1 B
dist/src/snapshot2.js	0 B	+1 B

[devin-ai-integration]

P2 (doc-accuracy nit, non-blocking). This compressed spine chain misplaces cli. Reading ◄ as "imported by" (kernel = sink, per the header), the trailing { cli, client, daemon-server } ◄ daemon-client asserts daemon-client imports every member of that group — including cli. But per TARGET_DAG_RANK, cli is rank 6 (the spine top) and daemon-client is rank 5, so daemon-client → cli is exactly a rank 5 → 6 back-edge the gate rejects. cli is the highest zone; nothing in the spine imports it.

check.ts avoids this by keeping client ◄ daemon/client on its own line and never chaining cli above daemon-client. Only this single CONTEXT.md chain introduces the inversion. Suggest moving cli to the far-right (top), e.g. … ◄ { client, daemon-server } ◄ daemon-client ◄ cli, or dropping cli from the compressed chain. model.ts ranks are authoritative and correct — this is purely the prose sketch, so it's a nit, but it's in the one doc whose stated purpose is describing the enforced order.

[devin-ai-integration]

Fixed in 5809c2b. The compressed CONTEXT chain now reads kernel ◄ platforms ◄ core ◄ commands ◄ { client, daemon-server } ◄ daemon-client ◄ cli, so cli (rank 6) sits at the far-right top and nothing in the spine imports it — no more implied daemon-client → cli back-edge. For coherence I also collapsed the check.ts header sketch onto that same single ordered line and added an explicit `A ◄ B` = B imports A legend so the direction is unambiguous. model.ts ranks were already authoritative/correct; this was prose-only.

Also in this commit (per the review's two coherence cleanups): ADR 0009 no longer contradicts itself on deferred scope — the closing line now says "within this consolidation the public Platform collapse is the last deferred step (net-new visionOS spatial-input QA is separate follow-up, outside the consolidation)"; and the app-log-request-scope.ts comment is trimmed to a 2-line module rationale, with the detailed bundler history kept in the PR description.

Re-ran on Node 22.18: format:check, check:layering (799 files, guard green), build (0 INEFFECTIVE_DYNAMIC_IMPORT), and check:affected --base origin/main --run all pass; only skillgym fails locally (spawn claude ENOENT, environmental — passes in CI).

[devin-ai-integration]

Adversarial review — no P1 blockers; 1 P2 doc nit (inline on CONTEXT.md)

Attacked each claim in the PR against code, a clean build on both branches, and PR/issue history. Everything holds.

Layering

• Every production zone is deliberately classified — no path-mapping blind spot. RANKED_ZONES (12) ∪ UNRANKED_ZONES (12) = 24 zones, which is exactly the set collectZones(listSourceFiles()) resolves the current tree into (23 top folders, with daemon split into daemon-server/daemon-client by targetDagZone). No unclassified zone, no stale-named zone.
• Global R1–R4 vs ranked-only R5 wording matches the implementation. checkLayeringRules (R1–R3) and findValueImportCycles (R4) run over all resolved edges; backEdgePair returns null unless both endpoints are in TARGET_DAG_RANK, so R5 is genuinely ranked-spine-only. The check.ts header, report() string, and CONTEXT.md all state this scope split correctly.
• A source zone cannot escape classification without failing tests. Verified empirically: adding a tracked src/zzznewzone/foo.ts makes every production zone is deliberately classified… fail with actual [ 'zzznewzone' ]. (Guard uses git ls-files, i.e. tracked files — identical to the gate's own discovery, so consistent, not a gap.)
• Unranked zones are not described as unconstrained. Docs say they still obey R1–R4; R3 (platforms-seam) and R4 (global cycles) genuinely constrain them (R1/R2 are vacuous on them by construction but they are not exempted). Accurate.

app-log INEFFECTIVE_DYNAMIC_IMPORT — removed, not hidden

Clean pnpm build (Node 24) on each branch:

	INEFFECTIVE warnings	internal/daemon.js
main (ae2106792)	1 (app-log.ts dyn-imported by request-platform-providers, statically by session-observability/session-teardown)	356.16 kB
PR branch	0	355.93 kB

• The shim is an effective split: dist/src/app-log-request-scope.js = 0.07 kB, dynamically imported and statically imported by nobody. app-log.ts now has no dynamic importer, so the warning is structurally gone.
• Cold-start / chunk graph preserved. daemon.js 356.16 → 355.93 kB (matches the −224 B size report). withAppLogProvider stays eager in daemon.js (re-export), so provider/request-scope behavior is byte-identical.
• Justification for the shim over a static import reproduced. Patching request-platform-providers to statically import withAppLogProvider and rebuilding: daemon.js 355.93 → 441.75 kB (~86 kB Apple-runner stack pulled eager) and 2 warnings — confirming the "naive static import is a cold-start regression" claim.
• No misleading boundary / no cycle. Shim → app-log.ts only; app-log.ts does not import the shim; R4 (global cycle rejection) passes. The comment honestly states app-log stays eager.

Architecture records (verified against current main)

• ADR 0009 shipped: src/platforms/apple/os/tvos/ exists; APPLE_OS_CAPABILITIES in src/platforms/apple/capabilities.ts (parity test apple-os-capability-table-parity.test.ts passes, 4/4); watchOS sentinel present (all-false, reserved in AppleOS, never produced by discovery). Deferred is honest: PUBLIC_PLATFORMS = ['ios','macos',…] — the public ios/macos→apple collapse has not shipped. #972 is closed.
• ADR 0012 shipped: steps 1–6 map to #1193/#1197/#1196/#1209/#1211, all merged into main (confirmed via git log). #1221 and #1183 are closed. #1209's selector-miss/identity-mismatch/identity-unverifiable kinds + target guard exist in src/replay/ and src/daemon/handlers/session-replay-target-*. Step 7 deferred.

Tests run locally (Node 24)

• pnpm check:layering — PASS (8 model tests + guard over 799 files).
• Focused: request-platform-providers (6), providers-plugin-routing-parity (3), app-log (14), apple-os-capability-table-parity (4), test:output-economy (26) — all PASS.
• pnpm build — PASS, 0 warnings.
• CI: 19/19 green.

Verdict: no blockers. Only the inline CONTEXT.md:100 P2 doc-ordering nit is worth addressing (optional).

[thymikee]

Re-review at a58fb5a: the prior ADR 0012 honesty finding is not fixed. The status and migration table now say steps 1 through 6 shipped, but the Context still states in present tense that update healing is active, that no target-binding verification exists, and that update rewrites ad files. Current code and the same ADR migration table contradict those claims.

Mark the Context as historical or update those present-tense descriptions before restoring ready-for-human. Runtime layering and app-log changes otherwise reviewed cleanly. No fixer dispatched.

[devin-ai-integration]

Fixed at a2a698451: the section is now explicitly Context (historical baseline), points readers to the migration table as authoritative, and converts the contradictory --update/target-binding claims to acceptance-time wording. git diff --check and formatting pass.

[thymikee]

Coordinator re-review at a2a698451 against current main: one documentation blocker appeared after #1228 merged.

docs/adr/0012-interactive-replay.md says migration step 8 (agent-supervised re-record repair) is Accepted; implementation pending and that only steps 1-6 shipped. Current main now contains #1228, which implemented step 8. Git merges this branch cleanly, so merging as-is would silently publish a stale implementation record rather than producing a conflict.

Please rebase onto current main and update ADR 0012 so step 8 is recorded as shipped in #1228; keep step 7 deferred if that remains accurate. Re-run the relevant documentation/format and affected gates on the rebased head.

One non-blocking wording correction should land with that update: app-log-request-scope.ts isolates the dynamic request-scope composition boundary and silences the specific Rolldown warning, but app-log.ts itself remains eager through session-observability.ts and session-teardown.ts. Reword the comment so it does not imply that app-log became genuinely lazy or improved cold-start/bundle size.

#1214 overlaps the app-log portion but is an alternative implementation, not a prerequisite: it makes all provider wrappers static, while this PR retains the existing dynamic provider boundaries and changes only app-log through app-log-request-scope.ts. Do not merge both. Assuming the rebased #1227 remains clean, #1227 should supersede #1214 and #1214 should be closed rather than merged.

ready-for-human remains withheld pending the updated head and final re-review.

[devin-ai-integration]

Addressed coordinator re-review 4956207866 in rebased 03acf2947: ADR 0012 records steps 1–6 and 8 shipped (#1228) with step 7 deferred, and the app-log wording now states the implementation remains eager through observability/teardown. Format, typecheck, lint, layering, build, unit/coverage, and provider-integration gates pass; local affected execution only cannot start SkillGym because this environment lacks the claude executable.

[thymikee]

Coordinator re-review at 03acf2947: prior ADR 0012 and app-log wording findings are fixed. The rebase preserves the reviewed layering/runtime changes, all 22 checks pass, and the PR is cleanly mergeable. The app-log boundary intentionally removes the bundler warning without claiming app-log became lazy. No actionable blockers remain; ready-for-human restored by coordinator.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-13 10:22 UTC