Skip to content

fix: #64 のマージ後レビューの指摘を直し、0.21.1 に bump する (formatVersion の README・テスト行、HarnessMenu.live() の owner guard ほか) - #67

Merged
GeneralD merged 11 commits into
mainfrom
fix/pr64-post-merge-review
Oct 29, 2026

Conversation

@GeneralD

Copy link
Copy Markdown
Owner

agent type follow-up

Follow-up of #64

概要

PR #64 はレビューラウンドが走る前にマージされ、version.txt の bump も入っていませんでした (main は 0.21.0 のまま)。マージ後に独立した 3 人のレビュアーから出た 17 件の指摘を origin/main (9153a42) のコードに照らして一件ずつ検証し、妥当なものを直し、0.21.0 → 0.21.1 の patch bump を足します。重複する指摘 (README の formatVersion、HarnessMenu.live() の盲点、macOS 14 の second line、manifest のテスト行) は 1 つの修正にまとめました。17 件のうち採用しなかったのは 1 件です。Sources/ の変更は doc comment だけで、挙動は変わりません。

変更点

  • README の formatVersion の説明を直しました。Int に収まらない整数は unsupported ではなく invalid (out of range) として報告されます (e95b66c)。指摘 1 / 6 / 16。
  • formatVersion の端の値を行として固定しました (6424cde)。指摘 2 / 12。
    • 2^53+1、Int.max、Int.min は unsupported、Int.max+1 は out of range、1.0 と 1e0 は 1 として読まれる、1e400 と name の lone surrogate escape は key を名指しできない "not valid JSON"。
    • FormatVersion を Double だけで読む形に一時的に変えると、2^53+1 と Int.max の 2 行が赤になりました (Int-then-Double の設計を固定できています)。
    • .dataCorrupted の arm は、lone surrogate も 1e400 も coding path が空になることをテスト行で確認したため、削除したままにしました。コメントも、行が示している内容だけに直しました。
  • macOS 14 では menu row の second line (作者、壊れた package の理由) が描画されないことがある、という既知の制限を、README・ARCHITECTURE.md の .lens の段落・dev-verification.md の Lane 1 の Effects menu 行に反映しました (ca1a8a7)。指摘 4 / 5。
  • scripts/check-dependency-keypaths.sh の DENIALS の staleness check が、glob の文字列そのものから lane を決めていたため、Dev* のような wildcard が None lane の dead weight と誤判定されていました。glob を展開した各 module の lane で判定するようにしました (e67661e)。指摘 9。Dev* の行は通り、一致する module が無い行・port でない key の行・lane が既に禁じている key の行はそれぞれ失敗することを、script の一時コピーに行を足して確認しました。D8 の「key, module or reason が無くなると失敗する」という書き方も、script が実際に見ている 3 条件 (architecture-boundaries.md と同じ) に揃えました。指摘 7。
  • HarnessMenu.swift のコメントが「entry lane」全体を denial の対象と書いていたのを、実際の対象の App と DevHarness に直しました (aaab619)。指摘 8。
  • scripts/check-live-owner.sh に HarnessMenu.live() の呼び出しを owner 以外のテストで禁止する grep を足しました (fa29f57)。HarnessMenu.live() は live の AppRouter を作り、その Presenter が effectInteractor を解決して process 全体の singleton の scope を固めるためです。owner 以外のテストの doc comment に HarnessMenu.live() と書くと script が失敗することを確認し、grep はコメントも読むため HarnessMenuTests の doc comment の綴りを言い換えました。ARCHITECTURE.md と AGENTS.md にも反映しています。指摘 3 / 15。
  • HarnessLiveWiringTests の使われなくなった import AppRouter を削除しました (bee0110)。指摘 10。
  • FileSystemGateway の doc comment に、存在しない path は CocoaError の .fileReadNoSuchFile を投げ、EffectPackageDataSource がそれで missing と unreadable を分けることを書きました (da1f235)。指摘 11。
  • ARCHITECTURE.md の「ディスクに触れる suite はすべて UUID の一時ディレクトリで動く」を、2 つの file gateway の smoke suite に限定し、ExamplePackagesTests が read-only で Examples/ を読む例外を明記しました (32872c9)。指摘 14。
  • HostEffectRegistrationsTests が file events と key-value store を読み戻していなかった点を直しました (836507e)。指摘 13。
    • default graph のテストで、effects folder への directory watch が FakeFileWatchGateway に張られたこと、developer log に "could not watch" が無いこと、何も書き込まれていないことを確認します。
    • 新しいテストで、in-memory store に package:glitch を seed し、in-memory の effects folder にその package を置いて、復元された selection と runtime に渡された source を確認します。
    • fileWatchGateway の override を外すと default graph のテストが、keyValueStoreGateway の override を外すと復元のテストが、それぞれ赤になりました。
    • Finder は override しているだけでこの suite は openFolder を呼ばないため、header と ARCHITECTURE.md の記述を、実際に読み戻しているもの (folder、file events、defaults) に狭めました。
  • version.txt を 0.21.0 から 0.21.1 に上げました (18bf00a)。

採用しなかった指摘

  • 指摘 17 (DENIALS の hit path と staleness failure に自動の negative test が無い): 指摘自体が「script の self-test を足すなら」という条件付きで、他の check-* script にも negative test は無く、既存の運用 (一時コピーで行を足して手で確認する) と同じです。今回も 9 の修正で同じ手順を踏みました。self-test の導入は別の話なので、この PR には含めません。

テスト

head 18bf00a のローカルゲート (make format の差分なし、make lint、make check、make build、make build-dev-harness、make test、make test-gpu):

format: PASS (exit 0)
lint: PASS (exit 0)
check: PASS (exit 0)
build: PASS (exit 0)
build-dev-harness: PASS (exit 0)
test: PASS (exit 0)  Test run with 959 tests in 130 suites passed ... with 10 known issues.
test-gpu: PASS (exit 0)  Test run with 27 tests in 6 suites passed
GATE ALL GREEN

known issues の 10 件は main にもある既存の件数です。

…pted answer

Rows for 2^53+1, Int.max, Int.min (unsupported), Int.max+1 (out of range), 1.0 and 1e0 (read as 1), 1e400 and a lone surrogate escape in name (not valid JSON, no key). A Double-only FormatVersion turns the first two red. The comments now say only what the rows show: the decoder gives .dataCorrupted an empty coding path, so the arm that named a key for a non-empty path had nothing to catch.
… 14 may not draw

The README, the .lens paragraph of ARCHITECTURE.md and the Lane 1 Effects-menu row still stated the second line unconditionally, while the Menu bar decision (D8) and the step 8 note already call it a known limit on 14.x.
lane_of read the glob's own text, so a wildcard such as Dev* fell into the None lane and failed as dead weight. The staleness check now expands the glob and passes when at least one matched module's lane resolves the key's kind. The descriptions of the three staleness reasons (D8 in ARCHITECTURE.md, Per-key denials in architecture-boundaries.md) now say what the script checks.
HarnessMenu.live() builds the live AppRouter, whose Presenter resolves effectInteractor, so a second test calling it would freeze the process-wide singleton in its own scope and pass the key-path grep. check-live-owner.sh now fails on the call outside the owner suite (verified with a temporary Tests file); the one other spelling, a doc comment in HarnessMenuTests, is reworded because the grep reads comments. ARCHITECTURE.md and AGENTS.md state the new reach.
…e gateways' smoke suites

The paragraph said every suite that touches the disk runs in a UUID-named temporary directory, while ExamplePackagesTests reads the repository's own Examples/ with no temporary directory (the File system row and testing.md already say so).
…ly the folder

The default-graph test asserted only the in-memory file system and the error log, so dropping the fileWatchGateway or keyValueStoreGateway override still passed. It now asserts the directory watch reached the fake file events with no 'could not watch' line, and a second test seeds the in-memory store with a package choice and reads the restored selection back. Dropping either override turns the suite red; Finder is only overridden, and the header and ARCHITECTURE.md now say so.
Copilot AI lite review requested due to automatic review settings October 29, 2026 19:36
@GeneralD GeneralD self-assigned this Oct 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@coderabbitai

coderabbitai Bot commented Oct 29, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 51 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 523fec5e-9bf2-4ece-969c-2ab8d5492917
Commits

Reviewing files that changed from the base of the PR and between 9153a42 and 18bf00a.

Files selected for processing: 15
  • .claude/rules/architecture-boundaries.md
  • .claude/rules/dev-verification.md
  • AGENTS.md
  • README.md
  • Sources/DevHarness/HarnessMenu.swift
  • Sources/Domain/Gateway/FileSystemGateway.swift
  • Sources/EffectPackageDataSource/EffectManifestDecoding.swift
  • Tests/DevHarnessTests/HarnessLiveWiringTests.swift
  • Tests/DevHarnessTests/HarnessMenuTests.swift
  • Tests/EffectPackageDataSourceTests/EffectManifestDecodingTests.swift
  • Tests/HostDependencyInjectionTests/HostEffectRegistrationsTests.swift
  • docs/ARCHITECTURE.md
  • scripts/check-dependency-keypaths.sh
  • scripts/check-live-owner.sh
  • version.txt
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@GeneralD
GeneralD merged commit 83c3e4e into main Oct 29, 2026
8 checks passed
@GeneralD
GeneralD deleted the fix/pr64-post-merge-review branch October 29, 2026 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants