Skip to content

fix(effect-package): 壊れたパッケージのログは変化したときだけ出す (#71) - #72

Merged
GeneralD merged 15 commits into
mainfrom
fix/broken-package-log-once
Nov 3, 2026
Merged

GeneralD merged 15 commits into
mainfrom
fix/broken-package-log-once

Conversation

@GeneralD

@GeneralD GeneralD commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

agent type scope closes

概要

壊れたパッケージ(manifest.json / effect.metal が欠けている、読めない、など)のログが、カタログが一覧を作り直すたびに同じ行で出ていました。ログは 変化したときだけ 出すようにしました(#71)。止まっていた担当の commit 5 件を元 SHA のまま種にし、事前レビューの指摘 14 件を実コードに当てて 1 件ずつ検証しました。

変更点

  • c955c19 fix(effect-package): log a broken package from the listing only, not from every single read(止まった作業から回収)。DataSource は壊れたパッケージについてログを出さない。
  • 0f78243 fix(effect-package): log a broken package on a change, from the catalog, not on every listing(止まった作業から回収)。EffectPackageRepositoryImpl が直前の一覧と比べ、変化した分だけ記録する。
  • 2ba2d6a test(effect-package): pin that the name trim takes non-ASCII whitespace(止まった作業から回収)
  • f48ea8c chore(lint): scan Examples/ for the product name in make check(止まった作業から回収)
  • edaae14 docs(package): say that EffectPackageDataSource resolves FileSystemGateway now(止まった作業から回収)
  • 166e26f docs(lint): name Examples/ in the product-name scan that AGENTS.md describes
  • 0640272 test(preview): expect the resized drawable at the view's backing size
  • a3b29a1 docs(effect-package): point the live-value claim at the D10 paragraph that states it
  • 4ea96ae docs(effect-package): count the listings the one-line test makes
  • 077dd79 docs(effect-package): weigh the UseCase option for the broken-package memory
  • 5ead02c docs(effect-package): name the folder errors the broken-package log does not dedupe
  • 86aa8c5 test(effect-package): pin the broken-package log's memory across packages, streams and empty listings
  • ccbff89 docs(effect-package): say that an empty listing forgets the broken packages it had
  • ad37a90 test(effect-package): log a broken package's error once through ten changes on the real catalog
  • dfba54a chore(release): 0.24.1

設計判断

  • どの層が前回の一覧を覚えるか: EffectPackageRepositoryImpl。各 catalog() ストリームの task の中に、直前の一覧の壊れたパッケージ(id → problem)を値として持たせました。記憶の寿命がストリームの寿命と一致するので、リセットが要らず、メニューを開くたびに 1 回ずつ記録されます。
  • 却下した案:
    • DataSource に記憶: live 値は process 全体で 1 つの static let で、全 suite と consumer が共有します。あるカタログの一覧が別のカタログのログを黙らせるため不可。
    • UseCase に記憶: フレーム経路の drop 重複排除(FramePipelineUseCaseImpl の lastDropReasons)と同じ形にできますが、EffectCatalogUseCaseImpl も process 全体の live 値なので、記憶が run を越えて残ります。run ごとのリセットという第二のライフサイクルが要るため採りませんでした。
    • Repository インスタンスに記憶: 共有のされ方が DataSource と同じなので不可。
  • 比較 は problem 全体の == です。種類が同じでも理由(message)が変われば記録し、同じものだけを抑えます。
  • 空の一覧(フォルダが空、または読めないフォルダ)は Repository から区別できません(DataSource が同じ空の inventory を返すため)。その一覧では壊れたパッケージを忘れ、再び現れたときに再度記録します。Domain / Entity に flag を足す案は port の変更になるため採らず、仕様として ARCHITECTURE と Repository のヘッダに明記し、テストで固定しました。

レビュー対応

事前レビューの指摘 14 件を実コードに当てて検証しました。

  • 修正
    • 1: 空の一覧で忘れることを仕様として明記・テストで固定(86aa8c5、ccbff89)
    • 2: AGENTS.md の走査範囲に Examples/ を追加(166e26f)
    • 3・9: 「the paragraph above」を節の名前で指す形に直した(a3b29a1)
    • 4: テストの listing 数を「ten changes(eleven listings)」と書いた(4ea96ae)
    • 5: 壊れたパッケージ 2 件の一覧を 1 件だけ変えるテストを追加(86aa8c5)
    • 6: UseCase の選択肢を重みづけて書いた(077dd79)
    • 8: フォルダのエラーが重複して出る件を「取りこぼしていない」範囲に明記(5ead02c)
    • 10: test-gpu の失敗を直した(0640272)。再現時、resize 後の drawable は points の 2 倍(36 × 64)で、layer の contentsScale は 1 のままでした。期待値は view の convertToBacking で出す形に変え、ヘッダの「off-screen の scale は 1」も直しました。厳密比較は残しています。
    • 11: 2 つのストリームが同じ壊れたパッケージを各々記録するテストを追加(86aa8c5)
    • 12: 実配線(LoopbackRig の real catalog)で 10 回の変更に対しエラーが 1 行になる統合テストを追加(ad37a90)
    • 13: unsupportedFormatVersion の遷移をテストに追加(86aa8c5)
  • 却下
  • 検証のみ(コミットなし)
    • 14: Examples/ の走査は下記の一時ファイルで確認しました。

テスト

  • ゲート(gate.py、tag issue71-land、HEAD dfba54a):
    • format: PASS(差分なし)
    • lint: PASS
    • check: PASS
    • build: PASS
    • build-dev-harness: PASS
    • test: PASS(Test run with 1062 tests in 139 suites passed ... with 12 known issues。12 件は main に元からある既知の issue)
    • test-gpu: PASS(Test run with 27 tests in 6 suites passed)
  • 変異確認(一時的に変えて戻した):
    • 「変化したときだけ記録」を外す → 新規の「二つの壊れたパッケージ」テストと統合テストを含む 4 件が赤くなる。
    • 記憶を Repository の task の外(共有変数)へ出す → 「各ストリーム」テストだけが赤くなる。
  • Examples/ の走査: Examples/Planted.lens/manifest.json に製品名を書いて scripts/check-product-name.sh を実行 → Examples/Planted.lens/manifest.json:1: product name appears outside the three allowed files で失敗(1 violation)。削除後は check-product-name: OK。

入れていないもの

Closes #71

Summary by CodeRabbit

  • Bug Fixes
    • Broken packages are reported when their status changes, with repeated reports suppressed until the problem changes or the package recovers. Packages that disappear from the catalog are no longer tracked.
    • Reading a broken package directly no longer adds duplicate log entries.
    • Manifest names surrounded by additional whitespace characters are now trimmed when decoded.

…from every single read

report() sat inside read(), which both packages() and package(_:) call, so an unreadable package was logged at error level on every re-listing and again each time the catalog session asked for its source. The listing is the read that shows a package, so it keeps the line; package(_:) reads the same files again for one id and now logs nothing. A new test pins both halves: the listing's lines are unchanged (logs) and a single read adds none.
…og, not on every listing

A listing logged every broken package again on each re-listing, so one unreadable package wrote the same error line after every file event. The catalog now remembers the broken packages of its last listing (in its own stream task) and logs only the transitions: became broken, a problem that changed in kind or reason, and readable again. The DataSource logs nothing for a broken package, so package(_:) cannot repeat a line either. The decision and the alternative are recorded in docs/ARCHITECTURE.md (Key Design Decisions).
The round-trip alphabet and the padding held only ASCII whitespace, so narrowing the decoder's trim from .whitespacesAndNewlines to an ASCII set stayed green. The alphabet and the padding now include U+00A0 and U+3000, and the decoding table has a row for a name wrapped in both. With the trim narrowed to ASCII the suite fails (33 issues).
Examples/ is a content class of its own (the sample .lens packages: a manifest and a kernel) that no scan reached, so the invariant that the name lives in three files rested on a grep someone had to remember to run. check-product-name.sh now looks at the names under Examples/ and at every file in it, the way it already does for Tools/, and the Rename-safe naming paragraph states the scope without the by-hand sentence. A file named for the product and a file mentioning it, both planted under Examples/, each fail the check.
…teway now

The comment on FileSystemGateway still read as a future condition (nothing above resolves it until EffectPackageDataSource) although this PR makes it true; the neighbouring EffectPackageDataSource comment was already fixed.
Copilot AI lite review requested due to automatic review settings November 3, 2026 11:42
@GeneralD GeneralD self-assigned this Nov 3, 2026
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The data source no longer logs broken-package reads. Each catalog stream logs broken-package transitions, including recovery, and suppresses repeated unchanged problems. The pull request also updates manifest whitespace tests, product-name scan coverage, a GPU resize expectation, and the package version.

Changes: Broken-package logging

Layer / File(s) Summary
Remove logging from package reads
Package.swift, Sources/EffectPackageDataSource/EffectPackageDataSourceImpl.swift, Tests/EffectPackageDataSourceTests/EffectPackageDataSourceTests.swift
The data source no longer reports broken-package problems during reads. Tests verify that listing and individual package reads produce no broken-package log entries.
Track broken-package transitions per catalog stream
Sources/EffectPackageRepository/EffectPackageRepositoryImpl.swift, Tests/EffectPackageRepositoryTests/EffectPackageRepositoryTests.swift, Tests/FramePathIntegrationTests/EffectCatalogIntegrationTests.swift, docs/ARCHITECTURE.md
The repository compares each listing with the previous broken-package map. It logs changed problems and recovery, suppresses unchanged problems, and forgets packages that disappear. Tests cover transitions, independent streams, and repeated file events.

Manifest whitespace handling

Layer / File(s) Summary
Cover non-ASCII whitespace in manifest names
Tests/EffectPackageDataSourceTests/EffectManifestDecodingTests.swift, Tests/EffectPackageDataSourceTests/EffectManifestRoundTripTests.swift
Manifest decoding and round-trip tests include no-break and ideographic spaces around names.

Product-name scan coverage

Layer / File(s) Summary
Include Examples in product-name checks
scripts/check-product-name.sh, AGENTS.md, docs/ARCHITECTURE.md
The product-name script scans Examples/ paths and file contents. The guidance and architecture documentation describe the expanded scan.

GPU resize test

Layer / File(s) Summary
Check resized drawable backing size
Tests/GPUSmokeTests/PreviewSmokeTests.swift
The resize test compares drawable size with the view bounds converted to backing coordinates instead of fixed point dimensions.

Release version

Layer / File(s) Summary
Update version value
version.txt
The version changes from 0.24.0 to 0.24.1.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Low

Sequence Diagram(s)

sequenceDiagram
  participant EffectPackageRepositoryImpl
  participant EffectPackageDataSourceImpl
  participant EffectLog
  EffectPackageRepositoryImpl->>EffectPackageDataSourceImpl: read package listings
  EffectPackageDataSourceImpl-->>EffectPackageRepositoryImpl: return ready and broken listings
  EffectPackageRepositoryImpl->>EffectLog: log changed problems and recovery
  EffectPackageRepositoryImpl-->>EffectPackageRepositoryImpl: retain current broken-package map
Loading

Possibly related issues

Possibly related PRs

  • GeneralD/mirage#59: This PR changes the data-source broken-package logging behavior introduced by PR #59 and moves transition reporting to the repository.
  • GeneralD/mirage#62: The integration test extends the session helper and Neon package fixtures introduced by PR #62.
Merge Risk: Minimal · up to dfba5

Broken-package logs now follow catalog transitions. No merge-blocking risk was identified.

Pre-merge checks: Passed 2 · Failed 2 · Inconclusive 1

Failed Failed checks: 2 warnings · 1 inconclusive

Check name Status Explanation Resolution
Out of Scope Changes check Warning The effect-package implementation, tests, and architecture documentation support issue #71. The version.txt change from 0.24.0 to 0.24.1 is a release change and has no demonstrated connection to… Remove the unrelated version.txt release change from this pull request, or link a release objective that requires it.
Docstring Coverage Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check Inconclusive For issue #71, the code removes broken-package reporting from EffectPackageDataSourceImpl.package(_:). Repository tests verify that repeated source reads add no logs. EffectPackageRepositoryImpl t… Provide the Codecov/patch check result for this pull request.
Passed Passed checks: 2 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title is in Japanese and reads 'fix(effect-package): log broken packages only when they change (#71)'. It matches the main change: the repository now logs a broken package only when it becomes bro…
Full details: Title check

Explanation

The title is in Japanese and reads 'fix(effect-package): log broken packages only when they change (#71)'. It matches the main change: the repository now logs a broken package only when it becomes broken, its problem changes, or it becomes readable again. The title does not name the repository-level tracking, but it covers the primary behavior change. The title is written in Japanese, which is acceptable if the team uses Japanese for commit titles.

Full details: Out of Scope Changes check

Explanation

The effect-package implementation, tests, and architecture documentation support issue #71. The version.txt change from 0.24.0 to 0.24.1 is a release change and has no demonstrated connection to the linked issue's logging behavior or coding requirements.

Resolution

Remove the unrelated version.txt release change from this pull request, or link a release objective that requires it.

Full details: Docstring Coverage

Explanation

Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 10 files. (3 skipped: 3 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Full details: Linked Issues check

Explanation

For issue #71, the code removes broken-package reporting from EffectPackageDataSourceImpl.package(_:). Repository tests verify that repeated source reads add no logs. EffectPackageRepositoryImpl tracks broken package problems per catalog stream, logs new or changed problems once, logs recovery, and forgets absent packages. docs/ARCHITECTURE.md documents this design. The reported make check, make lint, make test, and make test-gpu results support the automated-test requirements. The available evidence does not establish the required Codecov/patch status.

Resolution

Provide the Codecov/patch check result for this pull request.

Finishing Touches
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.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
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: 7112757f-dacd-4ed6-8bcb-3a7ae693e8db
Commits

Reviewing files that changed from the base of the PR and between 24bee02 and dfba54a.

Files selected for processing: 13
  • AGENTS.md
  • Package.swift
  • Sources/EffectPackageDataSource/EffectPackageDataSourceImpl.swift
  • Sources/EffectPackageRepository/EffectPackageRepositoryImpl.swift
  • Tests/EffectPackageDataSourceTests/EffectManifestDecodingTests.swift
  • Tests/EffectPackageDataSourceTests/EffectManifestRoundTripTests.swift
  • Tests/EffectPackageDataSourceTests/EffectPackageDataSourceTests.swift
  • Tests/EffectPackageRepositoryTests/EffectPackageRepositoryTests.swift
  • Tests/FramePathIntegrationTests/EffectCatalogIntegrationTests.swift
  • Tests/GPUSmokeTests/PreviewSmokeTests.swift
  • docs/ARCHITECTURE.md
  • scripts/check-product-name.sh
  • version.txt

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

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.

🟢 Approval recommended

No active review issues are reported for the changes.

No open issues 🎉

What changed in this PR

This pull request fixes duplicate logging for broken .lens effect packages. Broken-package diagnostics are now tracked per catalog stream and emitted only when the package’s problem state changes, while direct package reads remain silent.

Changes:

  • Move broken-package transition logging from the data source to the repository.
  • Add coverage for deduplication, recovery, changed failure reasons, stream isolation, and repeated reads.
  • Update manifest whitespace tests, GPU preview expectations, product-name checks, documentation, dependency comments, and version 0.24.1.
File Description
version.txt Bumps the release version to 0.24.1.
Tests/​GPUSmokeTests/​PreviewSmokeTests.swift Uses the view’s backing size for resized drawable assertions.
Tests/​FramePathIntegrationTests/​EffectCatalogIntegrationTests.swift Adds integration coverage for deduplicated broken-package logs.
Tests/​EffectPackageRepositoryTests/​EffectPackageRepositoryTests.swift Tests log transitions, recovery, stream isolation, repeated listings, and related behavior.
Tests/​EffectPackageDataSourceTests/​EffectPackageDataSourceTests.swift Verifies listings and direct package reads do not log broken packages.
Tests/​EffectPackageDataSourceTests/​EffectManifestRoundTripTests.swift Extends manifest round-trip coverage for non-ASCII whitespace.
Tests/​EffectPackageDataSourceTests/​EffectManifestDecodingTests.swift Adds decoding coverage for trimming non-ASCII whitespace.
Sources/​EffectPackageRepository/​EffectPackageRepositoryImpl.swift Tracks broken-package state per catalog stream and logs only changes.
Sources/​EffectPackageDataSource/​EffectPackageDataSourceImpl.swift Removes broken-package logging from individual package reads.
scripts/​check-product-name.sh Extends product-name validation to Examples/.
Package.swift Clarifies the file-system dependency comment.
docs/​ARCHITECTURE.md Documents broken-package logging ownership and transition semantics.
AGENTS.md Documents that product-name checks include Examples/.

Review effort: Lite

@codecov

codecov Bot commented Nov 3, 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 97b8a40 into main Nov 3, 2026
9 checks passed
@GeneralD
GeneralD deleted the fix/broken-package-log-once branch November 3, 2026 11:52
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.

壊れた .lens パッケージの error ログが再列挙と package(_:) のたびに繰り返される

2 participants