fix(progress): add per-page progress callback during batch hook execution

[zeroedin]

Summary

• Adds BatchProgressFunc type and RunBatchWithProgress method to HookRegistry
• BatchCallHook uses atomic.Int64 counter to report completion across concurrent worker goroutines
• RunBatchWithTimeout delegates to RunBatchWithProgress(... nil) for backward compatibility
• Transforms stage in build.go now reports real-time per-page progress during hook execution instead of jumping to 100% after all results return
• Registry closures thread progress callback from BatchProgressFunc → func(int) for BatchCallHook

Closes #686

Test plan

• All 190 plugin tests pass (including 4 new batch progress spec tests)
• go build ./... compiles cleanly
• cmd tests pass

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds real-time progress reporting during the Transforms (onPageRendered) batch hook stage, which previously sat idle for many seconds on large sites and then jumped straight to 100%. A new BatchProgressFunc callback is threaded from HookRegistry.RunBatchWithProgress through the registry batch closures down into NodeRuntime.BatchCallHook, which fires the callback (using an atomic.Int64) after each per-page bridge.Send completes across the worker pool.

Changes:

• Add BatchProgressFunc and extend BatchHookFunc to take an onProgress arg; introduce RunBatchWithProgress with RunBatchWithTimeout delegating for backward compat.
• Update NodeRuntime.BatchCallHook signature and increment a shared atomic counter per completed page, invoking the callback from worker goroutines.
• Wire a progress closure from the Transforms stage in build.go and drop the now-unused per-result reportUpdate timing on the result-application loop.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
internal/plugin/hooks.go	New BatchProgressFunc type, updated BatchHookFunc signature, and RunBatchWithProgress implementation.
internal/plugin/node.go	BatchCallHook accepts progress callback and reports per-page completion via atomic.Int64.
internal/plugin/registry.go	Type assertion and three batchFn closures updated to thread the progress callback into BatchCallHook.
internal/pipeline/build.go	Transforms stage uses RunBatchWithProgress with a closure that calls reportUpdate; removes the post-batch per-page reportUpdate timing.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[zeroedin]

Fixed in c3d7da8. Added sync.Mutex around the reportUpdate call in the progress callback. Concurrent worker goroutines now serialize through the lock before writing to the reporter.

[zeroedin]

Fixed in c3d7da8. Dropped pages[completed-1].RelPath — the atomic counter is a running total across concurrent workers, not an index into the pages slice. Progress callback now passes empty string for the file path; the count alone is accurate.

[zeroedin]

The no-op closure is required by the spec — test "nil progress callback does not panic" (hooks_test.go:522) calls onProgress(i+1, len(ps)) without nil-checking and expects RunBatchWithProgress to supply a no-op. However, I removed the pointless wrapper indirection per comment #4 — onProgress is now passed directly when non-nil, and only the nil case gets a no-op.

[zeroedin]

Fixed in c3d7da8. Removed the wrapper — onProgress is now assigned directly to itemProgress when non-nil.

[zeroedin]

Acknowledged. The three closures handle distinct registration paths (RegisterBatchWithOptions with scope, RegisterBatchWithPriority with priority, and the default-priority fallback). Extracting a helper would couple them and make each path harder to read independently. Leaving as-is since the pattern is pre-existing and each closure is 4 lines.

[zeroedin]

Code Review — PR #689

Verdict: Ready with fixes — 9 reviewers, 6 findings (2 P1, 4 P2)

P1 — High

#	File	Issue	Reviewers	Conf
1	build.go:675	pages[completed-1] displays wrong page. The atomic counter in node.go is a running total (1, 2, 3...), not the index of the page that just finished. With concurrent workers, completed=2 could mean page 500 finished second, but the display shows pages[1].RelPath. The progress count is correct; the page name is wrong for every update	correctness, adversarial	100
2	build.go:673	Data race: worker goroutines in node.go call onProgress concurrently → callback chain reaches reportUpdate → TTYProgress.Update writes p.usedBar and calls fmt.Fprintf without synchronization. go test -race would flag this. Add a mutex to the callback or make TTYProgress thread-safe	adversarial	90

P2 — Moderate

#	File	Issue	Reviewers	Conf
3	hooks.go:382	Multiple hooks for same event reset progress counter — each BatchCallHook creates a fresh atomic.Int64, so progress bar regresses from 100% back to 0% between hooks	correctness, adversarial	100
4	hooks.go:395	No-op closure func(int,int){} allocated per batch hook call when onProgress is nil. Pass nil through instead — callees already nil-check	correctness, maintainability, performance	100
5	hooks.go:451	Timeout path in per-item fallback skips onProgress call — timed-out items never advance the progress count, so it never reaches total	correctness, adversarial	100
6	node.go:424	Partial failure: errOnce captures first error but other goroutines keep running and calling onProgress. Caller receives progress updates after the error is returned	adversarial	75

Residual risks

• Three identical registry closure blocks (lines 297, 310, 330) — pre-existing pattern, each handles a different registration path
• API contract changes (BatchHookFunc, BatchCallHook, RunBatchWithProgress) are all internal-only
• progressFn closure captures pages by reference — safe now but fragile if pages is re-sliced before callbacks fire
• Post-error progress callbacks are benign since the error path discards results

Testing gaps

• No test validates page identity correctness under concurrent execution (Layout resolution tests: contradictory assertions for same input #1)
• No test exercises timeout + progress interaction in per-item fallback (feat(template): implement Go html/template engine #5)
• No test covers multi-hook-per-event progress behavior (feat: implement phases 1-3 (config, content, data, cascade, permalink, collection, template, output, assets) #3)

Coverage

• 1 finding suppressed at conf 50 (naming)
• 2 testing findings demoted (scope expansion on architect-owned coverage)
• 1 maintainability finding demoted (pre-existing registry pattern)
• Learnings: architecture context found in plans (issues feat: auto-scaling worker pool with configurable parallelism for subprocess plugins #491, fix: add progress reporting for layout rendering, post-render hooks, and output writing #493, spec: update progress reporter test for multi-stage Update calls (#493) #506, feat: concurrent file copies within static/asset copy stage #511, fix(progress): add per-page progress callback during batch hook execution #686)
• Agent-native: no gaps from this PR

---

_{Run ID: 20260515-123031-6a75bae1 · 9 reviewers · model: claude-opus-4-6}

[zeroedin]

Review response (c3d7da8)

P1 #1: Wrong page name — fixed

Dropped pages[completed-1].RelPath. The atomic counter is a running total across concurrent workers, not an index into the pages slice. Progress callback now passes empty string for file path; the count is accurate.

P1 #2: Data race — fixed

Added sync.Mutex around reportUpdate in the progress callback. Concurrent worker goroutines serialize through the lock before writing to the reporter.

P2 #3: Multi-hook progress reset — acknowledged

Each BatchCallHook creates a fresh atomic.Int64, so progress would regress between hooks. In practice, OnPageRendered is registered once per plugin (single batchFn per event). If multi-hook-per-event becomes real, the counter should move to RunBatchWithProgress level. Not fixing now — no test coverage and no current path triggers it.

P2 #4: No-op closure — partially fixed

Removed the pointless wrapper indirection — onProgress is now passed directly when non-nil. The no-op branch for nil must stay: spec test "nil progress callback does not panic" (hooks_test.go:522) calls onProgress() without nil-checking and explicitly requires RunBatchWithProgress to supply a no-op.

P2 #5: Timeout path skips onProgress — fixed

Added onProgress(j+1, len(current)) to the timeout case in the per-item fallback path.

P2 #6: Partial failure continues calling onProgress — acknowledged

Worker goroutines continue calling onProgress after errOnce captures the first error. This is benign — the error path discards results, and extra progress updates don't affect correctness. Stopping workers early would require a cancellation channel threaded through the goroutines, which adds complexity for a case that produces no user-visible bug.

Copilot: Registry closure duplication — declined

Pre-existing pattern across 3 distinct registration paths. Each closure is 4 lines. Extracting a helper would couple them without meaningful benefit.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[zeroedin]

Fixed in 2e8984a. Reordered to sort, strings, sync, time.

[zeroedin]

Fixed in 2e8984a. Added a high-water mark tracked under the mutex — out-of-order updates from concurrent workers are now discarded. The progress bar only moves forward.

[zeroedin]

Fixed in 2e8984a. Updated BatchProgressFunc docstring to clarify it fires once per dispatched item (success or timeout), not just on successful completion.

[zeroedin]

Re-Review — PR #689 (batch progress reporting)

Verdict: Approve | 10 reviewers | 0 actionable findings

4 of 6 first-review findings addressed. 2 explicitly declined with reasoning.

---

Prior feedback status

#	Finding	Status
1	Wrong page identity (pages[completed-1].RelPath)	ADDRESSED — empty string
2	Data race on reportUpdate	ADDRESSED — sync.Mutex
3	Multi-hook progress counter reset	NOT ADDRESSED — declined (no multi-hook path in practice)
4	No-op closure allocation	ADDRESSED — direct pass-through
5	Timeout path skips onProgress	ADDRESSED — callback added to timeout case
6	Partial failure continues onProgress	NOT ADDRESSED — declined (benign, stopping workers needs cancellation channel)

---

New finding — advisory

Leaked goroutine after batch timeout (adversarial P1 + correctness convergent → advisory residual)

After batch timeout fires in hooks.go:417, the batchFn goroutine continues running BatchCallHook because it has no context parameter — ctx is received by the batchFn closure in registry.go but never forwarded. The leaked goroutine fires onProgress → reportUpdate while the main goroutine has already called reportEndStage and potentially started a new stage. The mutex protects the happy path but does not protect against cross-stage reporter state mutations (stageName, total, usedBar).

Demoted to advisory: fix requires threading context through BatchCallHook for cooperative cancellation — architectural change beyond this PR's scope. Batch timeout is an uncommon trigger path.

---

Residual risks (advisory)

• Multi-hook progress reset (feat: implement phases 1-3 (config, content, data, cascade, permalink, collection, template, output, assets) #3): each hook creates fresh atomic.Int64 / resets j counter. Progress goes 1→N then 1→N per hook. Currently no multi-hook path for same event in production.
• Partial failure + progress (feat(static): implement static file copy and passthrough #6): after errOnce captures first error, other goroutines keep calling onProgress. Error path discards results; extra progress updates are cosmetic.
• Out-of-order progress values: concurrent goroutines may deliver completed counts non-monotonically. Mutex serializes but does not reorder. Visual effect negligible at callback firing speed.
• ctx not forwarded to BatchCallHook: pre-existing — BatchCallHook never accepted context. bridge.Send blocks with no deadline. Not introduced by this PR.
• Batch timeout skips final progress: batch path timeout handler does not call onProgress(total, total). Progress bar freezes at partial count. Subsumed by leaked goroutine advisory.

Demoted testing gaps

Testing reviewer assigned P0/P1/P2 to missing coverage for error paths, concurrent callbacks, and mutex-protected reporter. All demoted: miscalibrated severity (P0 ≠ missing tests for correct code) and scope expansion on architect-owned coverage decisions.

Reviewer breakdown

Reviewer	Findings	Notes
correctness (Opus)	0 actionable	4 findings: multi-hook reset P2, out-of-order P3, batch timeout P3, VerboseProgress P3 conf 50 (suppressed)
adversarial (Opus)	0 actionable	5 findings all demoted to advisory: leaked goroutine P1, out-of-order P3, ctx not forwarded P2 (pre-existing), multi-hook reset P2, partial failure P2
previous-comments	0 actionable	Confirmed 4/6 addressed, 2 not addressed (acknowledged/declined)
testing	0 actionable	3 findings demoted: P0/P1/P2 miscalibrated severity + scope expansion
maintainability	0 actionable	P0 registry duplication demoted (miscalibrated), P1 no-op closure (addressed), P1 conf 50 mutex coupling (below gate)
performance	0 actionable	P3 conf 50 mutex contention at scale — below confidence gate
api-contract	0 actionable	BatchHookFunc signature change is internal-only, backward compat maintained via RunBatchWithTimeout delegation
project-standards	0 findings
agent-native	0 findings
learnings	No docs/solutions/

---

Run: 20260515-124730-819926b2 · First review

[zeroedin]

Review response — round 3 (2e8984a)

Reviewer approved with 0 actionable findings. Addressing 3 new Copilot comments:

Non-monotonic progress — fixed

Added a high-water mark tracked under the mutex. Concurrent workers may deliver completed counts out of order (worker A gets 5, worker B gets 6, B acquires lock first), but updates where completed <= highWater are now discarded. The progress bar only moves forward.

Import ordering — fixed

Reordered sync import to alphabetical position: sort, strings, sync, time.

Timeout progress docstring — fixed

Updated BatchProgressFunc docstring to clarify it fires once per dispatched item (success or timeout), not just on successful completion.