fix(bookdrop): recursive fsnotify watcher for nested drops (bookshelf-v3ks)

zombor commented

2026-07-03 19:41:24 +00:00

Owner

Summary

Bug confirmed: Watch() in internal/bookdrop/watcher.go:94 called watcher.Add(dir) for the root only — fsnotify is non-recursive so files dropped into any subdirectory fired no real-time events (only the periodic WalkDir scan recovered them).
At startup, walk the root and Add() every existing subdirectory to the watcher.
At runtime, detect Create events for directories; call Add() on the new dir and enqueue files already present (race-window coverage).
Failed Add() on any subdir logs Warn and continues — non-fatal per spec.
Extracted handleEvent() and addSubdirs() helpers to keep every function under 30 lines and CC < 10.

Files changed

internal/bookdrop/watcher.go — bug fix
internal/bookdrop/export_test.go — AddSubdirs + updated WatchLoop signature
internal/bookdrop/watcher_test.go — 5 new specs + updated existing WatchLoop calls

Test plan

AddSubdirs test: failing add logs warn, does not panic/crash
WatchLoop test: Create event for a directory calls addWatch + enqueues existing files
WatchLoop test: failing addWatch on new dir does not stop the loop
Watch (recursive) test: detects epub in pre-existing subdir (startup walk)
Watch (recursive) test: detects epub in newly-created subdir (runtime add)
All 655 bookdrop specs green
make coverage gate green

Closes bead bookshelf-v3ks on merge.

## Summary - **Bug confirmed**: `Watch()` in `internal/bookdrop/watcher.go:94` called `watcher.Add(dir)` for the root only — fsnotify is non-recursive so files dropped into any subdirectory fired no real-time events (only the periodic WalkDir scan recovered them). - At startup, walk the root and `Add()` every existing subdirectory to the watcher. - At runtime, detect `Create` events for directories; call `Add()` on the new dir and enqueue files already present (race-window coverage). - Failed `Add()` on any subdir logs `Warn` and continues — non-fatal per spec. - Extracted `handleEvent()` and `addSubdirs()` helpers to keep every function under 30 lines and CC < 10. ## Files changed - `internal/bookdrop/watcher.go` — bug fix - `internal/bookdrop/export_test.go` — `AddSubdirs` + updated `WatchLoop` signature - `internal/bookdrop/watcher_test.go` — 5 new specs + updated existing `WatchLoop` calls ## Test plan - [x] `AddSubdirs` test: failing add logs warn, does not panic/crash - [x] `WatchLoop` test: Create event for a directory calls addWatch + enqueues existing files - [x] `WatchLoop` test: failing addWatch on new dir does not stop the loop - [x] `Watch (recursive)` test: detects epub in pre-existing subdir (startup walk) - [x] `Watch (recursive)` test: detects epub in newly-created subdir (runtime add) - [x] All 655 bookdrop specs green - [x] `make coverage` gate green Closes bead bookshelf-v3ks on merge.

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

wf-detail-older-execution.png

56 KiB

recompute-match-score-kebab-open.png

109 KiB

zombor added 1 commit

2026-07-03 19:41:24 +00:00

fix(bookdrop): make fsnotify watcher recursive for nested drops (bookshelf-v3ks)

/ JS Unit Tests (pull_request) Successful in 46s

Details

/ E2E API (pull_request) Successful in 2m5s

Details

/ Lint (pull_request) Successful in 2m57s

Details

/ E2E Browser (pull_request) Successful in 3m2s

Details

/ Integration (pull_request) Successful in 3m20s

Details

/ Test (pull_request) Successful in 4m25s

Details

c0a37ddf7d

fsnotify.Add() is non-recursive by design. Watch() only added the
root bookdrop dir, so files dropped into subdirectories fired no
real-time events (only the periodic WalkDir scan caught them).

Fix:
- At watcher startup, walk the root and Add() every existing subdir.
- In watchLoop, detect Create events on directories; call Add() on the
  new dir and enqueue any files already present inside it (race window).
- handleEvent() extracted so watchLoop stays within the 30-line limit.
- Failed Add() on a subdir logs Warn and continues (non-fatal).

Tests:
- AddSubdirs export + test for failing-add warn path.
- WatchLoop test: Create event on dir calls addWatch + enqueues files.
- WatchLoop test: failing addWatch does not stop the loop.
- Watch (recursive): detects file in pre-existing subdir.
- Watch (recursive): detects file in newly-created subdir.
- Updated all existing WatchLoop test calls with new addWatch param.

All 655 bookdrop specs pass; coverage gate green.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented

2026-07-03 19:42:38 +00:00

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) ![recompute-match-score-kebab-open](/attachments/e2064806-12cc-4972-9f84-4c45c09d12ca)

forgejo-actions commented

2026-07-03 19:43:20 +00:00

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. ![wf-detail-older-execution](/attachments/78117839-a177-4cb1-b0a7-3c4e0705c3fe)

zombor commented

2026-07-03 19:50:51 +00:00

Author

Owner

Security Review — bd-bookshelf-v3ks (recursive bookdrop watcher)

[MAJOR] internal/bookdrop/watcher.go:228 — Symlink escape: os.Stat follows symlinks in handleEvent, permitting ingest-scope escape
os.Stat(name) follows symlinks. If any actor with write access to the bookdrop root creates a symlink-to-directory (e.g. ln -s /mnt/private-books /bookdrop/escape), fsnotify fires a Create event for /bookdrop/escape; os.Stat follows the link and returns IsDir()=true, causing handleNewDir to (a) register the symlink target with inotify via watcher.Add and (b) walk and enqueue its contents. Book-format files in the target directory are ingested into the pipeline as if they were under the bookdrop root. The existing WalkDir function (line 35) explicitly guards against this with d.Type()&os.ModeSymlink != 0; the new runtime path has no equivalent guard. Under DISK_TYPE=NETWORK (NFS/SMB), a remote actor with share-write access can exploit this without local shell access. Fix: replace os.Stat(name) with os.Lstat(name) at line 228 so a symlink is never mistaken for a plain directory.

[MAJOR] internal/bookdrop/watcher.go:111,129 — Unbounded inotify watch-descriptor consumption, no cap or operator signal
addSubdirs (line 111) and handleNewDir (line 129) register every subdirectory recursively with no upper bound. Linux's per-user inotify watch limit defaults to 8,192 (fs.inotify.max_user_watches). A deep or wide directory tree — whether legitimate (large author/series/volume hierarchy) or adversarially planted (via NFS/SMB write access when DISK_TYPE=NETWORK) — can exhaust all descriptors system-wide, silently starving other inotify consumers (editors, container runtimes, other services). Failures from watcher.Add() are only logged at Warn individually; there is no aggregate count, no Prometheus counter, and no escalation to Error — the operator has no visibility that watch coverage is partially degraded. Fix: impose a configurable cap (e.g. 1,000 watched dirs) and log at Error + emit a bookdrop_watch_dirs_total Prometheus counter when the cap is reached; at minimum, emit a counter on every failed Add() call so an operator can alert on descriptor exhaustion.

[MINOR] internal/bookdrop/watcher.go:130 — handleNewDir WalkDir enqueues symlinked files without a symlink check
The WalkDir callback inside handleNewDir (line 130) skips directory entries (d.IsDir()) but does not skip symlink entries (d.Type()&os.ModeSymlink). A file symlink with a book-format extension inside a new subdirectory (e.g. novel.epub -> /home/user/private.epub) is enqueued and follows the full ingest pipeline. The existing production WalkDir (line 35) explicitly skips symlinks. Fix: add if d.Type()&os.ModeSymlink != 0 { return nil } in the handleNewDir WalkDir callback, mirroring the policy in WalkDir.

Architecture boundary: No workflow-engine imports introduced. Clean.
DISK_TYPE guard: The guard lives in RejectProposal (destructive file ops). The watcher itself is unguarded on both main and this branch — no regression introduced here.
Newly-created dir ingest path: Files in new subdirs go through the same isBookFile → onEvent → IngestFile pipeline; no existing validation is bypassed.

REVIEW VERDICT: 0 blocker, 2 major, 1 minor

## Security Review — bd-bookshelf-v3ks (recursive bookdrop watcher) [MAJOR] internal/bookdrop/watcher.go:228 — Symlink escape: `os.Stat` follows symlinks in `handleEvent`, permitting ingest-scope escape `os.Stat(name)` follows symlinks. If any actor with write access to the bookdrop root creates a symlink-to-directory (e.g. `ln -s /mnt/private-books /bookdrop/escape`), fsnotify fires a Create event for `/bookdrop/escape`; `os.Stat` follows the link and returns `IsDir()=true`, causing `handleNewDir` to (a) register the symlink target with inotify via `watcher.Add` and (b) walk and enqueue its contents. Book-format files in the target directory are ingested into the pipeline as if they were under the bookdrop root. The existing `WalkDir` function (line 35) explicitly guards against this with `d.Type()&os.ModeSymlink != 0`; the new runtime path has no equivalent guard. Under DISK_TYPE=NETWORK (NFS/SMB), a remote actor with share-write access can exploit this without local shell access. Fix: replace `os.Stat(name)` with `os.Lstat(name)` at line 228 so a symlink is never mistaken for a plain directory. [MAJOR] internal/bookdrop/watcher.go:111,129 — Unbounded inotify watch-descriptor consumption, no cap or operator signal `addSubdirs` (line 111) and `handleNewDir` (line 129) register every subdirectory recursively with no upper bound. Linux's per-user inotify watch limit defaults to 8,192 (`fs.inotify.max_user_watches`). A deep or wide directory tree — whether legitimate (large author/series/volume hierarchy) or adversarially planted (via NFS/SMB write access when DISK_TYPE=NETWORK) — can exhaust all descriptors system-wide, silently starving other inotify consumers (editors, container runtimes, other services). Failures from `watcher.Add()` are only logged at Warn individually; there is no aggregate count, no Prometheus counter, and no escalation to Error — the operator has no visibility that watch coverage is partially degraded. Fix: impose a configurable cap (e.g. 1,000 watched dirs) and log at Error + emit a `bookdrop_watch_dirs_total` Prometheus counter when the cap is reached; at minimum, emit a counter on every failed `Add()` call so an operator can alert on descriptor exhaustion. [MINOR] internal/bookdrop/watcher.go:130 — `handleNewDir` WalkDir enqueues symlinked files without a symlink check The WalkDir callback inside `handleNewDir` (line 130) skips directory entries (`d.IsDir()`) but does not skip symlink entries (`d.Type()&os.ModeSymlink`). A file symlink with a book-format extension inside a new subdirectory (e.g. `novel.epub -> /home/user/private.epub`) is enqueued and follows the full ingest pipeline. The existing production `WalkDir` (line 35) explicitly skips symlinks. Fix: add `if d.Type()&os.ModeSymlink != 0 { return nil }` in the `handleNewDir` WalkDir callback, mirroring the policy in `WalkDir`. --- **Architecture boundary:** No workflow-engine imports introduced. Clean. **DISK_TYPE guard:** The guard lives in `RejectProposal` (destructive file ops). The watcher itself is unguarded on both main and this branch — no regression introduced here. **Newly-created dir ingest path:** Files in new subdirs go through the same `isBookFile` → `onEvent` → `IngestFile` pipeline; no existing validation is bypassed. REVIEW VERDICT: 0 blocker, 2 major, 1 minor

zombor commented

2026-07-03 19:52:53 +00:00

Author

Owner

CODE REVIEW: NOT APPROVED

Phase 0: DEMO Verification

No explicit DEMO block with a re-runnable command. The PR test-plan has a checked checkbox claiming a temporary probe was used. I ran independent probes on the worktree to verify gate behavior directly.

Probe results (all run against .worktrees/bd-bookshelf-scev):

funlen gate (>40 statements): CONFIRMED WORKING — 66-statement NewBigHandler appended to internal/books/handler.go fires correctly (per-function exclusion works; new function names are gated)
gocyclo gate (CC >30): CONFIRMED WORKING at CC=32, but CONFIRMED SILENT at CC=20 (see [MAJOR] #1 below)
nestif: new files gated correctly; NEW deeply-nested functions in grandfathered files are INVISIBLE to nestif (see [MAJOR] #2 below)

Findings

[MAJOR] .golangci.yml line ~24 — gocyclo threshold=30 allows genuinely complex new functions to ship silently

CLAUDE.md specifies CC<10. Gate at CC>30 leaves the entire CC 11-30 range completely ungated. Probe confirmed: a function with CC=20 (twice the style guide) produces 0 issues. The bead description itself suggested 10-15 as a pragmatic range; 30 is twice that suggestion. A new god-function with CC=25 will pass lint without warning. This is the permanent quality bar being set. At CC>30 the gate only catches the most catastrophic outliers.

Fix: Tighten to 15 (the bead's own upper bound). If existing code violates CC 15-30, grandfather those per-function as done for the other linters. At 15, CC 11-14 still slips through but the range is narrow and defensible. At 30, the gate is cosmetic for the CC 11-29 range that actually needs governance.

[MAJOR] .golangci.yml (nestif per-file section) — 25 whole-file exclusions permanently blind nestif to NEW code in god-files

The 25 per-file nestif exclusions (path: internal/books/handler\.go + linters: [nestif]) exempt EVERY nestif violation in those files — past AND future. Probe confirmed: a new function with 4 levels of nesting (nestif complexity=10) appended to internal/books/handler.go produces 0 issues. The config comment says "New files with deep nesting are still gated" — true for new FILES, but new FUNCTIONS added to existing grandfathered files (the exact god-files that need governance most) are permanently invisible to nestif.

The technical constraint is real: nestif embeds raw condition text in its message (not the function name), making per-function text patterns brittle. But the consequence is a gate that does not gate the worst offenders.

Fix options (in order of preference):

Add a prominent // LINT-EXEMPT: nestif whole-file comment inside each grandfathered source file so developers adding new deeply-nested code to that file see the warning at the point of edit.
Investigate whether golangci-lint's exclude-rules supports matching nestif violations by surrounding function context in newer versions.
Accept the per-file approach but document it explicitly in the config as a "known blind spot for new code in these files, not just grandfathered violations."

[MINOR] .golangci.yml comment/setting mismatch — comment says "nesting complexity >= 4" but min-complexity: 5

The inline comment reads # gate: nesting complexity >= 4 but the actual setting is nestif: min-complexity: 5. The gate fires at complexity >=5, not >=4. Fix: change comment to >= 5.

[MINOR] funlen=60 is 2x the CLAUDE.md style guide (func<30 lines)

Functions up to 60 lines pass silently. The PR is explicit about pragmatism here, and 60 is at least a real gate. Noting it because this is the permanent bar — CLAUDE.md's 30-line aspiration is effectively unenforced by CI. Consider tightening to 45 at the next opportunity.

REVIEW VERDICT: 0 blocker, 2 major, 2 minor

## CODE REVIEW: NOT APPROVED ### Phase 0: DEMO Verification No explicit DEMO block with a re-runnable command. The PR test-plan has a checked checkbox claiming a temporary probe was used. I ran independent probes on the worktree to verify gate behavior directly. **Probe results (all run against `.worktrees/bd-bookshelf-scev`):** - funlen gate (>40 statements): CONFIRMED WORKING — 66-statement `NewBigHandler` appended to `internal/books/handler.go` fires correctly (per-function exclusion works; new function names are gated) - gocyclo gate (CC >30): CONFIRMED WORKING at CC=32, but CONFIRMED SILENT at CC=20 (see [MAJOR] #1 below) - nestif: new files gated correctly; NEW deeply-nested functions in grandfathered files are INVISIBLE to nestif (see [MAJOR] #2 below) --- ### Findings [MAJOR] .golangci.yml line ~24 — gocyclo threshold=30 allows genuinely complex new functions to ship silently CLAUDE.md specifies CC<10. Gate at CC>30 leaves the entire CC 11-30 range completely ungated. Probe confirmed: a function with CC=20 (twice the style guide) produces `0 issues`. The bead description itself suggested 10-15 as a pragmatic range; 30 is twice that suggestion. A new god-function with CC=25 will pass lint without warning. This is the permanent quality bar being set. At CC>30 the gate only catches the most catastrophic outliers. Fix: Tighten to 15 (the bead's own upper bound). If existing code violates CC 15-30, grandfather those per-function as done for the other linters. At 15, CC 11-14 still slips through but the range is narrow and defensible. At 30, the gate is cosmetic for the CC 11-29 range that actually needs governance. [MAJOR] .golangci.yml (nestif per-file section) — 25 whole-file exclusions permanently blind nestif to NEW code in god-files The 25 per-file nestif exclusions (`path: internal/books/handler\.go + linters: [nestif]`) exempt EVERY nestif violation in those files — past AND future. Probe confirmed: a new function with 4 levels of nesting (nestif complexity=10) appended to `internal/books/handler.go` produces `0 issues`. The config comment says "New files with deep nesting are still gated" — true for new FILES, but new FUNCTIONS added to existing grandfathered files (the exact god-files that need governance most) are permanently invisible to nestif. The technical constraint is real: nestif embeds raw condition text in its message (not the function name), making per-function text patterns brittle. But the consequence is a gate that does not gate the worst offenders. Fix options (in order of preference): 1. Add a prominent `// LINT-EXEMPT: nestif whole-file` comment inside each grandfathered source file so developers adding new deeply-nested code to that file see the warning at the point of edit. 2. Investigate whether golangci-lint's `exclude-rules` supports matching nestif violations by surrounding function context in newer versions. 3. Accept the per-file approach but document it explicitly in the config as a "known blind spot for new code in these files, not just grandfathered violations." [MINOR] .golangci.yml comment/setting mismatch — comment says "nesting complexity >= 4" but min-complexity: 5 The inline comment reads `# gate: nesting complexity >= 4` but the actual setting is `nestif: min-complexity: 5`. The gate fires at complexity >=5, not >=4. Fix: change comment to `>= 5`. [MINOR] funlen=60 is 2x the CLAUDE.md style guide (func<30 lines) Functions up to 60 lines pass silently. The PR is explicit about pragmatism here, and 60 is at least a real gate. Noting it because this is the permanent bar — CLAUDE.md's 30-line aspiration is effectively unenforced by CI. Consider tightening to 45 at the next opportunity. --- REVIEW VERDICT: 0 blocker, 2 major, 2 minor

zombor added 1 commit

2026-07-03 20:20:00 +00:00

fix(bookdrop): close two coverage gaps from review-round-2

/ E2E API (pull_request) Successful in 3m5s

Details

/ JS Unit Tests (pull_request) Successful in 1m27s

Details

/ Integration (pull_request) Successful in 4m9s

Details

/ Lint (pull_request) Successful in 4m13s

Details

/ E2E Browser (pull_request) Successful in 4m21s

Details

/ Test (pull_request) Failing after 5m9s

Details

8dd51d40c7

- metrics_test.go: add RecordWatchDirDropped It block so the
  m.watchDirsDropped.Inc() statement is exercised (metrics.go:85-87)
- watcher_test.go: add walkErr permission-error Context; creates a
  mode-000 subdir inside a newly-created parent so WalkDir's second
  callback call (with walkErr != nil) fires and the return-nil branch
  is covered (watcher.go:169-171)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented

2026-07-03 20:22:53 +00:00

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) ![recompute-match-score-kebab-open](/attachments/15bff985-a6ef-45cb-91da-95e14da302a5)

forgejo-actions commented

2026-07-03 20:23:04 +00:00

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. ![wf-detail-older-execution](/attachments/3c13452e-3f22-408d-8bdd-4e0085482b92)

zombor added 1 commit

2026-07-03 20:29:35 +00:00

fix(bookdrop): inject walk func for CI-root-proof walkErr coverage

/ JS Unit Tests (pull_request) Successful in 1m27s

Details

/ E2E API (pull_request) Successful in 2m12s

Details

/ Lint (pull_request) Successful in 3m14s

Details

/ Integration (pull_request) Successful in 3m36s

Details

/ Test (pull_request) Successful in 4m15s

Details

/ E2E Browser (pull_request) Successful in 4m10s

Details

a64a2a7fd7

Replace the chmod-000 approach (ineffective when CI runner is root in Docker)
with an injectable handleNewDirWalkFunc package var — same pattern as
newFsnotifyWatcher. SetHandleNewDirWalkError in export_test.go installs a
synthetic-error callback that exercises the walkErr != nil branch directly,
regardless of filesystem permission semantics.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented

2026-07-03 20:32:07 +00:00

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) ![recompute-match-score-kebab-open](/attachments/e71f8b8f-2149-4095-bd05-df76e1392a5a)

forgejo-actions commented

2026-07-03 20:32:25 +00:00

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. ![wf-detail-older-execution](/attachments/095e48ea-3f29-428c-b408-ce2be6e3acd8)

zombor commented

2026-07-03 20:39:33 +00:00

Author

Owner

Code Review — focused re-review of fix commits (inotify cap + config/metrics wiring)

Phase 0 — DEMO verification

No traditional shell-command DEMO block exists in this PR. The implementation is verified by the test suite. CI reports success on SHA a64a2a7fd7e6dcc3d8bbc6d5c65c212c452417ea. Proceeding on CI-green as the behavioral ground truth per project conventions.

Phase 1 — cappedAdd correctness

(a) Off-by-one: watchedDirs starts at 1 (root added directly at line 108). cappedAdd guard is *count >= maxDirs. With maxDirs=1, first subdir attempt: 1 >= 1 → true → skip. Exactly MaxWatchedDirs dirs are registered. Correct.

(b) Count only on success: *count++ executes only after add(dir) returns nil (lines 136–140). Count is never incremented on a failed Add. Correct.

(c) Concurrency: addSubdirs runs synchronously before watchLoop starts (lines 115–119). Inside watchLoop, only the main select goroutine calls add (via handleEvent → handleNewDir). The flush closure spawned by time.AfterFunc touches only pendingPaths (mutex-protected) and onEvent — it never calls add. No concurrent mutation of count. Correct.

(d) MaxWatchedDirs<=0 → no cap: Guard is maxDirs > 0 && *count >= maxDirs. When maxDirs == 0 the first condition is false and the body is never entered. Correct.

Phase 2 — OnDirDropped

Fires exactly once per dropped dir (one call per cappedAdd invocation that hits the cap). nil-safe: if onDrop != nil { onDrop() } at line 131. Logs at logger.Error (not Warn). All correct per original finding.

Phase 3 — Config wiring

Default 1000 set in config.Defaults() (config.go:317). Flag registered with that default. app.Run() passes cfg.BookdropMaxWatchedDirs directly to WatchOptions.MaxWatchedDirs (app.go:951). No nil/zero-value trap in the production path — Defaults() always sets 1000, and 0 is documented as "no cap" (a safe fallback if someone bypasses defaults).

Phase 4 — Symlink completeness

grep os.Stat internal/bookdrop/watcher.go returns only line 288 (os.Lstat). No remaining os.Stat on event paths. handleNewDir uses d.Type()&os.ModeSymlink != 0 in the WalkDir callback. addSubdirs relies on filepath.WalkDir not following symlinks, so symlinked dirs never satisfy d.IsDir(). Correct.

Phase 5 — Tests

Cap drops Nth+1 + counter: MaxWatchedDirs=1, subdir pre-exists, asserts dropped > 0. Valid.
Deep tree: parent + nested both asserted in added. Valid.
Symlinked-dir not registered: asserts added does not contain symlinkPath. Valid.
Symlinked file not enqueued: asserts symlinkFile not in received. Valid.
walkErr branch: injectable handleNewDirWalkFunc covers the walkErr != nil path without filesystem permission tricks. Valid.

Phase 6 — No new golangci.yml exclusions: Confirmed — diff of .golangci.yml is empty.

Findings

[MINOR] internal/appwire/appwire.go:226 — BookdropMaxWatchedDirs field in appwire.Deps is stored but never consumed
The field is populated in app.New() at app.go:424 (BookdropMaxWatchedDirs: cfg.BookdropMaxWatchedDirs) but no code in the codebase reads deps.BookdropMaxWatchedDirs. app.Run() reads the value directly from its cfg *config.Config parameter (app.go:951). The stored Deps field is dead code. No correctness impact — the correct value flows through cfg — but the field adds noise to the Deps struct. Fix: remove the field from appwire.Deps and the assignment in app.New(), since Run() already sources it from cfg directly.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review — focused re-review of fix commits (inotify cap + config/metrics wiring) **Phase 0 — DEMO verification** No traditional shell-command DEMO block exists in this PR. The implementation is verified by the test suite. CI reports `success` on SHA `a64a2a7fd7e6dcc3d8bbc6d5c65c212c452417ea`. Proceeding on CI-green as the behavioral ground truth per project conventions. --- **Phase 1 — cappedAdd correctness** (a) **Off-by-one:** `watchedDirs` starts at 1 (root added directly at line 108). cappedAdd guard is `*count >= maxDirs`. With `maxDirs=1`, first subdir attempt: `1 >= 1` → true → skip. Exactly MaxWatchedDirs dirs are registered. Correct. (b) **Count only on success:** `*count++` executes only after `add(dir)` returns nil (lines 136–140). Count is never incremented on a failed Add. Correct. (c) **Concurrency:** `addSubdirs` runs synchronously before `watchLoop` starts (lines 115–119). Inside `watchLoop`, only the main select goroutine calls `add` (via `handleEvent → handleNewDir`). The `flush` closure spawned by `time.AfterFunc` touches only `pendingPaths` (mutex-protected) and `onEvent` — it never calls `add`. No concurrent mutation of `count`. Correct. (d) **MaxWatchedDirs<=0 → no cap:** Guard is `maxDirs > 0 && *count >= maxDirs`. When `maxDirs == 0` the first condition is false and the body is never entered. Correct. **Phase 2 — OnDirDropped** Fires exactly once per dropped dir (one call per cappedAdd invocation that hits the cap). nil-safe: `if onDrop != nil { onDrop() }` at line 131. Logs at `logger.Error` (not Warn). All correct per original finding. **Phase 3 — Config wiring** Default 1000 set in `config.Defaults()` (config.go:317). Flag registered with that default. `app.Run()` passes `cfg.BookdropMaxWatchedDirs` directly to `WatchOptions.MaxWatchedDirs` (app.go:951). No nil/zero-value trap in the production path — `Defaults()` always sets 1000, and 0 is documented as "no cap" (a safe fallback if someone bypasses defaults). **Phase 4 — Symlink completeness** `grep os.Stat internal/bookdrop/watcher.go` returns only line 288 (`os.Lstat`). No remaining `os.Stat` on event paths. `handleNewDir` uses `d.Type()&os.ModeSymlink != 0` in the WalkDir callback. `addSubdirs` relies on `filepath.WalkDir` not following symlinks, so symlinked dirs never satisfy `d.IsDir()`. Correct. **Phase 5 — Tests** - Cap drops Nth+1 + counter: MaxWatchedDirs=1, subdir pre-exists, asserts `dropped > 0`. Valid. - Deep tree: parent + nested both asserted in `added`. Valid. - Symlinked-dir not registered: asserts `added` does not contain symlinkPath. Valid. - Symlinked file not enqueued: asserts symlinkFile not in received. Valid. - walkErr branch: injectable `handleNewDirWalkFunc` covers the `walkErr != nil` path without filesystem permission tricks. Valid. **Phase 6 — No new golangci.yml exclusions:** Confirmed — diff of `.golangci.yml` is empty. --- ### Findings [MINOR] internal/appwire/appwire.go:226 — `BookdropMaxWatchedDirs` field in `appwire.Deps` is stored but never consumed The field is populated in `app.New()` at app.go:424 (`BookdropMaxWatchedDirs: cfg.BookdropMaxWatchedDirs`) but no code in the codebase reads `deps.BookdropMaxWatchedDirs`. `app.Run()` reads the value directly from its `cfg *config.Config` parameter (app.go:951). The stored Deps field is dead code. No correctness impact — the correct value flows through `cfg` — but the field adds noise to the Deps struct. Fix: remove the field from `appwire.Deps` and the assignment in `app.New()`, since `Run()` already sources it from `cfg` directly. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor added 1 commit

2026-07-03 20:43:33 +00:00

refactor: remove dead BookdropMaxWatchedDirs field from appwire.Deps

/ JS Unit Tests (pull_request) Successful in 35s

Details

/ E2E API (pull_request) Successful in 2m35s

Details

/ Lint (pull_request) Successful in 3m31s

Details

/ Integration (pull_request) Successful in 3m34s

Details

/ E2E Browser (pull_request) Successful in 4m19s

Details

/ Test (pull_request) Successful in 4m26s

Details

371d119883

The BookdropMaxWatchedDirs field was added to appwire.Deps and assigned
in app.go but never read from Deps. The value flows directly from cfg to
the bookdrop watcher (Run method, line ~951), so the Deps field assignment
is dead code.

Deletions:
- internal/appwire/appwire.go: remove field definition (line 226)
- internal/app/app.go: remove assignment (line 424)

The cfg-sourced value remains intact: config.go default (1000) → cfg →
watcher opts at Run(). No behavior change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

forgejo-actions commented

2026-07-03 20:46:10 +00:00

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. ![wf-detail-older-execution](/attachments/97cb256c-8566-428d-b3b4-abbe7fcdd094)

forgejo-actions commented

2026-07-03 20:46:41 +00:00

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) ![recompute-match-score-kebab-open](/attachments/d713867e-cfa9-4a60-83be-fab2ce219db8)

/ JS Unit Tests (pull_request) Successful in 35s

Details

/ E2E API (pull_request) Successful in 2m35s

Required

Details

/ Lint (pull_request) Successful in 3m31s

Required

Details

/ Integration (pull_request) Successful in 3m34s

Required

Details

/ E2E Browser (pull_request) Successful in 4m19s

Required

Details

/ Test (pull_request) Successful in 4m26s

Required

Details

This pull request is blocked because it's outdated.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin bd-bookshelf-v3ks:bd-bookshelf-v3ks

git switch bd-bookshelf-v3ks

Rows
Columns

fix(bookdrop): recursive fsnotify watcher for nested drops (bookshelf-v3ks) #913

Summary

Files changed

Test plan

Security Review — bd-bookshelf-v3ks (recursive bookdrop watcher)

CODE REVIEW: NOT APPROVED

Phase 0: DEMO Verification

Findings

Code Review — focused re-review of fix commits (inotify cap + config/metrics wiring)

Findings

Checkout