Repository navigation
fix(harness): #59 と #61 のレビュー指摘の残りを直す (formatVersion の報告、ハーネスのメニュー、Entry lane の denial ほか) - #64
Merged
Merged
Conversation
…of the app delegate HarnessAppDelegate was untested by design, so the Effects menu assembly (with its #available branch) and the Router's start and stop were the 13 lines codecov/patch flagged on #61. HarnessMenu holds them: it owns the AppRouter, builds the main menu (Quit, then an Effects item hosting EffectMenu in an NSHostingMenu where the system has one) and forwards start() and stop() to the Router. The hosted-menu builder is injectable, so DevHarnessTests also cover a system without NSHostingMenu. HarnessMenuTests run it over a Router whose Presenter talks to a FakeEffectInteractor, built through an override, so only HarnessLiveWiringTests resolves the stateful catalog values live; that test now builds HarnessMenu.live(), the call the delegate makes. The delegate keeps the calls that need a running application: the window, the activation and setting the menu on NSApplication.
…ive Metal Nothing in the repository backs it: ExamplePackagesTests pins the package's shape through the DataSource, and whether the kernel compiles is checked on a device in the Lane 1 run of #52. State only that.
…and which is only accepted The architecture paragraph and the isMissing comment said fileNoSuchFile was a shape the in-memory and live gateways produce. Both raise fileReadNoSuchFile (InMemoryFileSystemGatewayTests, FileSystemGatewayImplTests); fileNoSuchFile is accepted defensively and only the scripted failRead test passes it.
…the real file system The File system row of the Physical Abstraction Table and testing.md said the real file system appears only in Gateway smoke suites. ExamplePackagesTests reads the repository's Examples/ from disk; say so, as the single read-only exception.
… name
A manifest that is valid JSON but whose formatVersion is 1.5 or 99999999999999999999 was reported as 'not valid JSON': the decoder refuses a number it cannot read as an Int with no coding path. The probe now reads formatVersion as an Int and, failing that, as a Double, so the key is known: a fraction is 'not an integer', a whole number beyond Int is 'out of range'. Rows added for both, a negative one, a null formatVersion and a null name (the valueNotFound arm had no row). The dataCorrupted arm that named a key ('... is not valid') was reachable only through that Int-range failure and is gone.
…older in memory defaultGraphChoosesTheBundledEffect created its InMemoryFileSystemGateway inline and never read it back, so 'no test touches the user's folder' was a comment. The fakes are now kept per test (Fakes.live), and the test asserts the folder was made once and listed through the in-memory file system and that no error was logged. Dropping only the fileSystemGateway override turns it red.
…s that sway together band is float(gid.y) / bandHeight and is not floored, so every row has its own position on the noise lattice: the sway is a smooth wave with a new random swing every 48 rows, not rows that move together. The kernel is unchanged; its comments, the manifest description, the README's copy of it and the Lane 1 table's wording follow what it does.
…sorts them ExamplePackagesTests sorted the folder names including .lens, while EffectPackageDataSourceImpl sorts the ids without it. The two differ as soon as one name is a prefix of another followed by a character below '.': with Ripple.lens and Ripple-b.lens the names put Ripple-b first and the ids put Ripple first. With a scratch Ripple-b.lens copied into Examples/ the old expectation was red and the new one green; the scratch copy is removed.
…the Router starts The quiet-before-start check existed only for an injected factory. The default factory is what the host and the harness run, so the test over it now reads the fake Interactor's starts, stops and the Presenter's rows before start(). A default factory that started its Presenter was green before (the later start() is a no-op on a started Presenter) and is red now.
…sing on macOS 14 NSMenuItem.subtitle is API_AVAILABLE(macos(14.4)), and the header (the same text in the macOS 26.4 and 26.5 SDKs) says that on macOS 14 an item with an attributed title does not show it, while macOS 15 and later do. The package's floor is 14.0, so the host's MenuBarExtra rows may lack the author or the reason line there. Written as a known limit of the Menu bar decision (D8) with the alternative not taken, and aligned in the Lane 1 note and EffectMenu's comment. The header does not say whether SwiftUI sets an attributed title, so the claim is 'may'.
The Router alone starts the Presenter that owns the effects menu's Interactor, but the Entry lane may resolve Interactors, so nothing stopped App or DevHarness from resolving effectInteractor and starting the catalog a second time. check-dependency-keypaths.sh gets a DENIALS table beside EXCEPTIONS: <module glob> <key> <reason>, checked before the lane's kinds. A working denial is never matched by a resolution, so staleness is static: an entry fails when its key is no longer a declared port, no module matches its glob, or the lane's kinds already forbid the key. A scratch resolution in App (attribute form) and in DevHarness (Dependency<T>(...) initializer form) fails with the reason and exit 1, a clean tree passes, and each of the three staleness cases fails on a temporary copy. Documented in architecture-boundaries.md (Per-key denials), the Menu bar and Interactor/Presenter decisions, AGENTS.md and the module checklist.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Entry dependency boundary scripts/check-dependency-keypaths.sh, .claude/rules/architecture-boundaries.md, .claude/rules/module-checklist.md, AGENTS.md, Sources/AppRouter/AppRouter.swift, docs/ARCHITECTURE.md, .claude/rules/dev-verification.md, Sources/Views/EffectMenu.swift, Package.swift, Sources/DevHarness/main.swift |
The key-path checker denies effectInteractor access from App and DevHarness before lane and exception checks. It rejects stale denials. Architecture and module guidance documents the restriction and the Router’s Presenter lifecycle. |
HarnessMenu lifecycle and wiring Sources/DevHarness/HarnessMenu.swift, Sources/DevHarness/HarnessAppDelegate.swift |
HarnessMenu builds the main menu and optional Effects submenu, and forwards start and stop calls to AppRouter. HarnessAppDelegate installs and starts the menu beside the session, then stops the menu before stopping the session. |
Menu and Router tests Tests/AppRouterTests/AppRouterTests.swift, Tests/DevHarnessTests/*, Package.swift |
Tests cover menu construction, optional and hosted Effects menus, Router lifecycle, quiet Presenter state before startup, and live harness wiring through HarnessMenu. |
Package validation and test fixtures
| Layer / File(s) | Summary |
|---|---|
Manifest version classification Sources/EffectPackageDataSource/EffectManifestDecoding.swift, Tests/EffectPackageDataSourceTests/EffectManifestDecodingTests.swift, README.md, Sources/EffectPackageDataSource/EffectPackageDataSourceImpl.swift, docs/ARCHITECTURE.md |
The decoder distinguishes non-integer or out-of-range numeric formatVersion values from unsupported integer versions. Tests cover fractional and out-of-range values, as well as null fields. The README and architecture documentation describe the validation rules. |
Example package listing and filesystem fixture Tests/EffectPackageDataSourceTests/ExamplePackagesTests.swift, .claude/rules/testing.md, docs/ARCHITECTURE.md |
The example-package test sorts IDs without the .lens suffix and copies repository examples into an in-memory filesystem. Testing and architecture guidance document this read-only filesystem access. |
Default effects graph assertions Tests/HostDependencyInjectionTests/HostEffectRegistrationsTests.swift |
The tests use a Fakes value to retain in-memory gateways and recording logs. The default-graph test checks effects-directory operations and confirms that the error log is empty. |
Ripple description
| Layer / File(s) | Summary |
|---|---|
Ripple wave description Examples/Ripple.lens/effect.metal, Examples/Ripple.lens/manifest.json, .claude/rules/dev-verification.md, README.md |
Comments and descriptions now characterize Ripple as a smooth sideways wave that varies by row and drifts with time. No executable shader code changed. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant HarnessAppDelegate
participant HarnessSession
participant HarnessMenu
participant AppRouter
participant MenuBarPresenter
HarnessAppDelegate->>HarnessSession: start
HarnessAppDelegate->>HarnessMenu: start
HarnessMenu->>AppRouter: start
AppRouter->>MenuBarPresenter: start
HarnessAppDelegate->>HarnessMenu: stop
HarnessMenu->>AppRouter: stop
AppRouter->>MenuBarPresenter: stop
HarnessAppDelegate->>HarnessSession: stop
Possibly related issues
- GeneralD/mirage issue 52: The PR updates live-wiring tests and related documentation, but it does not perform the remaining Lane 1 device verification.
- GeneralD/mirage issue 42: The PR changes manifest validation and the construction and tests of the Effects menu, which overlap with the issue’s package-loading and menu objectives.
Merge Risk: 
90fd8
-
No material new merge risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title is written in Japanese and names a real, specific part of the changeset: fixing remaining review findings from #59and#61. It lists the areas touched: formatVersion reporting, harness menu,…Linked Issues check Issue #63is the only direct link. Its coding requirements, grouped by objective:
1. [#59] Docs (ARCHITECTURE.md .lens paragraph, fileNoSuchFile description, File system row, testing.md exception): t…Out of Scope Changes check Each changed area traces to an issue #63objective or supports one:
- Sources/EffectPackageDataSource/EffectManifestDecoding.swift and EffectPackageDataSourceImpl.swift (isMissing doc):#59formatVer…Full details: Title check
-
Explanation
The title is written in Japanese and names a real, specific part of the changeset: fixing remaining review findings from
#59and#61. It lists the areas touched: formatVersion reporting, harness menu, and the Entry lane denial. The raw summary confirms each of these changes. The title covers several areas rather than one main change, and it is long at 90 characters, but it is not vague or off-topic.
Full details: Linked Issues check
-
Explanation
Issue
#63is the only direct link. Its coding requirements, grouped by objective:- [
#59] Docs (ARCHITECTURE.md .lens paragraph, fileNoSuchFile description, File system row, testing.md exception): the PR description and the AI change summaries cover each item. The docs were assessed from these summaries, not from full text. - [
#59] formatVersion (EffectManifestDecoding.swift at head 90fd8ed): verified.FormatVersiontriesInt, thenDouble. A non-integer number yields "formatVersion" is not an integer; an out-of-range integer yields "formatVersion" is out of range. The dead dataCorrupted key-path arm is removed. Null formatVersion and name fall through to valueNotFound with a key path. The 6 test rows are listed in the PR and the summary of EffectManifestDecodingTests. - [
#59] HostEffectRegistrationsTests: the Fakes scope records folder creation, listing, and an empty error log. Per the summary, the override removal turns 4 assertions red. - [
#59] Ripple comments, manifest description, and README: updated per the summary. Kernel code unchanged. - [
#59] ExamplePackagesTests (verified at head): packageIDs() strips .lens and sorts the ids, matching the requirement. - [
#61] Harness menu extraction: verified. HarnessAppDelegate uses HarnessMenu.live(), sets mainMenu, and calls menu.start()/stop() around the session. HarnessMenu.swift and HarnessMenuTests.swift are present, with injectable menu builder for the nil path. - [
#61] NSMenuItem.subtitle limit in D8, dev-verification.md, and EffectMenu comment: updated per the summary. - [
#61] Entry-lane .effectInteractor denial via check-dependency-keypaths.sh DENIALS for App and DevHarness: per the summary and the PR description; the script change is in the change set. - [
#61] AppRouterTests: pre-start quiet assertions added per the summary.
Unverified: the acceptance criterion that codecov/patch is green is a CI result outside the repository. The PR reports local gate results (make format, lint, check, build, build-dev-harness, test, test-gpu, all PASS). The PR also states that codecov ignore rules are unchanged. No coding requirement is left unmet on the evidence available.
- [
Full details: Out of Scope Changes check
-
Explanation
Each changed area traces to an issue
#63objective or supports one:- Sources/EffectPackageDataSource/EffectManifestDecoding.swift and EffectPackageDataSourceImpl.swift (isMissing doc):
#59formatVersion and fileNoSuchFile items. - Sources/DevHarness/HarnessMenu.swift (new), HarnessAppDelegate.swift, main.swift comment:
#61codecov item (menu and Router start/stop moved to a testable type). The injectable menu builder is needed to test the nil (pre-macOS 14.4) path, which CI cannot run. - Tests/DevHarnessTests (HarnessMenuTests, HarnessLiveWiringTests), Package.swift (DevHarnessTests gains Presenters and Views): test support for the
#61items. - Tests/AppRouterTests:
#61pre-start quiet assertion. - Tests/EffectPackageDataSourceTests (EffectManifestDecodingTests, ExamplePackagesTests):
#59items. - Tests/HostDependencyInjectionTests/HostEffectRegistrationsTests.swift:
#59in-memory folder item. - Examples/Ripple.lens (effect.metal comments, manifest.json description) and README Ripple example:
#59Ripple item. Kernel logic is unchanged per the summary. - scripts/check-dependency-keypaths.sh:
#61per-key denial. The added stale-denial checks (undeclared key, no matching module, kind already disallows key) support the denial rule. They do not add unrelated behavior. - .claude/rules/*, AGENTS.md, docs/ARCHITECTURE.md, Package.swift comments, EffectMenu.swift and AppRouter.swift comments: documentation the issue requires (D8, Entry lane, testing exception, Ripple text).
Not in the change set: the repeated unreadable-package error log and version.txt, both excluded by the issue. No change lacks a connection to an objective.
- Sources/EffectPackageDataSource/EffectManifestDecoding.swift and EffectPackageDataSourceImpl.swift (isMissing doc):
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 26 functions across 15 files. (9 skipped: 9 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Owner
Author
|
マージ |
GeneralD
added a commit
that referenced
this pull request
Oct 29, 2026
fix: #64 のマージ後レビューの指摘を直し、0.21.1 に bump する (formatVersion の README・テスト行、HarnessMenu.live() の owner guard ほか)
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
issue #63 の
。#59 (#45) と #61 (#50) がマージされた
に残っていたレビュー指摘 11 件を、main (0.21.0) で今も成り立つかを 1 件ずつ確かめてから直した。11 件すべてが main でも成り立っていたので、見送った項目は無い (#62 が
した 
した)。1 項目 1 commit。
HostEffectRegistrationsTestsとHarnessLiveWiringTestsも、下の各項目で現物を読んでアプリの
が変わるのは 
/ コメント / ハーネスの
/
formatVersionが整数でない manifest のメッセージだけ。それ以外は docs /make checkのスクリプト。version.txtには触れていない。#59 由来
.lens段落の裏付けの無い記述を削った (1c8d797): 「Examples/Ripple.lensは live の Metal で一度コンパイルした」を消し、固定されている事実だけにした。ExamplePackagesTestsが形式を固定し、コンパイルは 実物結線・loopback 統合・Lane 1 実機確認・docs と grep の閉じ #52 の Lane 1 で確かめるfileNoSuchFileの記述を正した (5f8580f): fake (InMemoryFileSystemGateway) も live gateway (FileSystemGatewayImplTests) も出すのはfileReadNoSuchFile。fileNoSuchFileはfailReadのmissingIsToldByCode) だけが通す。isMissingのコメントも81c181c):ExamplePackagesTestsはリポジトリのExamples/を実ディスクから読む。Physical Abstraction Table の File system 行と.claude/rules/testing.mdに、「real FS は Gateway smoke だけ」の唯一の読み取り専用の例外として書いた (他にディスクを読むAppDirectoriesProviderImplTestsと Gateway smoke だけで、どちらも Gateway の smoke)formatVersionが整数でない manifest を名指しで2ce0da9): 1.5 や 99999999999999999999 は、valid な JSON なのに「not valid JSON」とIntで読めない数を coding path 無しのdataCorruptedで返すため)。probe がInt、だめならDoubleで読み、小数は"formatVersion" is not an integer、Intを超える整数は"formatVersion" is out of rangeになる。EffectManifestDecodingTestsに 1.5 / 1.5 + 他のformatVersion: null/"name": nullの 6 行を足した (旧 decoder に対してvalueNotFoundの arm に行が無かったためのdataCorruptedの「<key>is not valid」の arm を消したHostEffectRegistrationsTestsが実際に in-memory のフォルダを使ったことを assert した (70078e4): MVP-B の終端: 実物の結線・loopback 統合・Lane 1 の計測表・docs と grep の閉じ (#52) #62 のdefaultGraphChoosesTheBundledEffectはInMemoryFileSystemGatewayをその場で作って捨てていて、「ユーザーのフォルダに触れない」はコメントだけだった。fake をFakesに束ねて残し、フォルダが 1 回だけ作られ、列挙され、error log が空であることを読み戻す。fileSystemGatewayの override だけを外すと赤になるExamples/Ripple.lensの記述をbb14860):bandは floor していないので、揺れは帯ごとに揃うのではなく、48 行ごとに新しいランダムな振れを持つ滑らかな波。effect.metalの冒頭と 2 か所のコメント、manifest.jsonのdescription、README の例 (manifest とExamplePackagesTestsの期待する並び順を DataSource と4dd51d8): フォルダ名 (.lensつき) を sort していたのを、.lensを除いた id を sort するようにした。Ripple.lensとRipple-b.lensでは名前だとRipple-bが先 (-が.より小さい)、id だとRippleが先で、DataSource はExamples/Ripple-b.lensを置いて旧版が赤・新版が緑を確かめてから消した#61 由来
5621e7a):HarnessAppDelegateから Effects メニューの組み立て (#available(macOS 14.4, *)の分岐を含む) と router の start / stop をHarnessMenuに移した。HarnessMenuTests(7 tests) が Quit、Effects 項目、NSHostingMenuが無いNSHostingMenu<EffectMenu>であること、メニューを読んでも何も start されないこと、start()/stop()が Router を通って Interactor に届くことを固定する。delegate に残るのは実行中の application を要る呼び出し (window、activate、NSApplicationへのメニューのセット) だけ。codecov の ignore には何も足していないNSMenuItem.subtitleのd5d4c7a): SDK のNSMenuItem.h(macOS 26.4 と 26.5 の SDK でsubtitleがAPI_AVAILABLE(macos(14.4))で、macOS 14 では attributed title の項目に出ず macOS 15 以降で出ると書いている。MenuBarExtraの行の 2 行目が出ない場合がある。SwiftUI がこの行に attributed title を付けるかは header に書かれておらず、macOS 14 では動かしていないので、書いたのは「may」まで。Lane 1 の記録欄とEffectMenuのコメントも合わせた\.effectInteractorを禁じた (90fd8ed):scripts/check-dependency-keypaths.shにDENIALS(<module glob> <key> <reason>) を足し、AppとDevHarnessが\.effectInteractorを解決すると、.claude/rules/architecture-boundaries.md(Per-key denials)、docs/ARCHITECTURE.mdの D8 / D9、AGENTS.md、module checklist に書いたAppRouterTestsの既定の factory の060b408):start()のstart()は start 済みの Presenter では何もしない) で、今は赤check-live-owner.sh方式ではなくcheck-dependency-keypaths.shの per-key denial にした: Entry lane の kind (Interactor) はcameraInteractor/harnessInteractorを許すので、kind のcheck-live-owner.sh流の grep は、コメントや文字列も拾ってしまう (実際、この\.effectInteractorと書いただけで落ちた) し、lane の表と別の場所に@Dependency/Dependency(...)/Dependency<T>(...)のどの綴りでもHarnessMenuは Router を init の引数に取り、既定はlive():AppRouter(makeMenuBarPresenter:)をwithDependencies { $0.effectInteractor = fake }の中で作る。\.effectInteractorの key path はHarnessLiveWiringTests以外に書かず (check-live-owner.shが通る)、static letの live 値を解決するのも従来どおりHarnessLiveWiringTestsだけ。そのHarnessMenu.live()を呼ぶので、delegate の組み立てが#availableの else 側は実行できない。builder をNSHostingMenuを作れないreturn nilの経路で確かめる。既定の builder がNSHostingMenu<EffectMenu>を返すことは別のformatVersionはIntを先に読み、だめならDouble:Doubleだけで読むと 2^53 を超える整数が丸まって1.0/1e0は従来どおり 1 として読まれる。1e400のようにDoubleでも持てない数は、引き続き「not valid JSON」(decoder がそれ以上のことを言わない)最終 head
なし →
90fd8edで、ローカルの gate を順に全部緑 (make formatのmake lint→make check→make build→make build-dev-harness→make test→make test-gpu):HarnessMenuの Effects 項目を足す行を外すとHarnessMenuTestsの 2 本が赤。start()を空にするとHarnessMenuTestsの start / stop の 2 本とHarnessLiveWiringTestsの 1 本が赤 (メニューが Router を起こさないので、catalog が走らない)。stop()を空にしてもstopの1.5が 2 行、巨大整数が 2 行)HostEffectRegistrationsTestsからfileSystemGatewayの override だけを外すと、createdDirectories/isDirectory/listings/ error log の 4 つの assert が赤AppRouterの既定の factory にpresenter.start()を足すとAppRouterTestsの既定の factory のSources/App(@Dependency(\.effectInteractor)) とSources/DevHarness(Dependency<any EffectInteractor>(\.effectInteractor)) に置くとcheck-dependency-keypathsが exit 1 で落ち、cameraInteractorは通る。消すと緑に戻るExamples/Ripple-b.lensを置くと、旧ExamplePackagesTestsが赤、新版が緑。消したsettle/tickUntil/Collector.waitForCountのみ。固定のTask.sleepも壁時計も使っていないdocs
docs/ARCHITECTURE.md:.lens段落 (Ripple のコンパイル、no-such-file の 2 つの code、formatVersionのHostEffectRegistrationsTestsの assert)、Entry 行と Coverage (HarnessMenu)、Menu bar の段落 (D8: 起動役、.claude/rules/architecture-boundaries.md(Per-key denials)、module-checklist.md、testing.md(読み取り専用の例外)、dev-verification.md(Ripple の文言、2 行目の記録欄)AGENTS.md: key-path のREADME.md:formatVersionのRippleのdescriptionPackage.swift:DevHarnessのコメント、DevHarnessTestsの依存にPresentersとViewsAppRouter、EffectMenu、EffectPackageDataSourceImpl.isMissing、HarnessAppDelegate、main.swift入れていないもの
version.txtのCloses #63
Summary by CodeRabbit