[kans]

Ledger handlers receive the *ledgerPage they write into as a parameter. The four context.Context keys that carried ledger state (ledgerInvocationKey, ledgerCommitKey, ledgerAttemptsKey, ledgerWorkerKey), the code that dispatched on them, and the ledgerInvocation type are deleted. -841 / +530, all in pkg/sync.

Before: nextPageOrFinishAction staged if ctx carried an invocation, else called a mutex-guarded parallelActionTransitioner field that syncParallel installed for its lifetime, else committed. transitionActionState failed at runtime if ctx lacked a commit key. Handlers reached invocation.page.writer.PutX.

After: handler type is func(ctx, page *ledgerPage, action *Action) error. ledgerPage owns everything scoped to one page: the writer, the row, stage, staged children, after-commit hooks, connector observations, the restart request, and the worker's *ledgerAttempts. Handlers call page.PutX(...) (six forwarders; the lifecycle half of PageWriter stays private to the page and scheduler), page.stage(...), page.recordDuration(start), page.markTypeScopedPlanned(), page.addObservations(...). invokeActionPage takes worker, attempts, and the queue transition func as parameters; syncParallel passes them instead of mutating the syncer. transitionActionState takes ledgerTransitionCommit as a parameter.

Deleted: nextPageOrFinishAction, ledgerInvocation, setParallelActionTransitioner / parallelTransitionMu, the four key types, and the two pin tests from #1203 that guarded the ctx-key shape (TestNextPageCallersAreLedgerHandlers, TestTransitionActionStateRejectsContextWithoutCommitKey).

Behaviour: none intended. Stage/commit semantics, retry, and queue logic are untouched. Nothing persisted changes (no pkg/dotc1z, pb, or proto files in the diff; ledger row fields are written at the same points); a sync started on the previous version resumes on this one. The rate-limit wait observer closes over the worker's *ledgerAttempts instead of reading it from ctx; ObserveWait reads only the innermost observer, so retry-wait stats are recorded once as before.

Tests: parameter swaps except TestLedgerWaitObservationsStayWithWorker, which now allocates the coordinator accumulator and passes it in; assertions unchanged.

Acceptance: rg 'ctx\.Value\(' pkg/sync --glob '!*_test.go' is empty; handlers never name writer: rg '\.writer\.' pkg/sync hits only ledger_page.go, the scheduler, the restart path, and the skipLedgerSync seed. Build, vet, lint clean; go test ./pkg/sync/... -count=1 and make chaos-check ok.

[github-actions]

Superseded — see the current review report for commit 860b4380dab7

General PR Review: sync: ledger handlers take the page invocation as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: full
View review run

Review Summary

This is an internal refactor in pkg/sync. Ledger handlers now receive *ledgerInvocation as a parameter instead of reading it from ctx. invokeActionPage now takes the worker index, the *ledgerAttempts, and the queue transition func explicitly. transitionActionState takes ledgerTransitionCommit as a parameter. The global parallelActionTransitioner and its mutex are gone, and every ctx key except the wait-observer key is removed. I scanned the full PR diff for security and correctness and checked that behavior matches the old code:

• Init still runs with worker 0 and fresh attempts.
• Serial steps still share the coordinator attempts.
• Workers in syncParallel still get their own index and attempts, and their commits still go through queue.transition. The coordinator blocks inside syncParallel, so no non-worker commit could have used the old global transitioner.
• Retry-wait stats are still recorded once. Each withLedgerAttempts layer installs a new observer, but ratelimit.ObserveWait (pkg/ratelimit/wait_observer.go:49-53) calls only the innermost one, and production always installs the outer observer first (syncer.go:841).

I applied the repo-local criteria. Triage:

• Silence: no. A mis-routed commit is now a type error, not a runtime fallback.
• Durability: indirect. Ledger commit semantics are untouched.
• Uncontrolled dimensions: no. Shared mutable syncer state was removed.
• Consumer distance: same package.

Verdict: LOW. The renamed Sync* methods hang off the unexported syncer type, so no exported API, proto, serialized-state, default, or dependency surface changes. No go.mod changes.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

Superseded — see the current review report for commit ff05f899cf19

General PR Review: sync: ledger handlers take the page invocation as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: full
View review run

Review Summary

This is an internal refactor in pkg/sync. Ledger handlers now receive *ledgerInvocation as a parameter instead of reading it from ctx. invokeActionPage now takes the worker index, the *ledgerAttempts, and the queue transition func explicitly. transitionActionState takes ledgerTransitionCommit as a parameter. The global parallelActionTransitioner and its mutex are gone, and every ctx key except the wait-observer key is removed. I scanned the full PR diff for security and correctness and checked that behavior matches the old code:

• Init still runs with worker 0 and fresh attempts.
• Serial steps still share the coordinator attempts.
• Workers in syncParallel still get their own index and attempts, and their commits still go through queue.transition. The coordinator blocks inside syncParallel, so no non-worker commit could have used the old global transitioner.
• Retry-wait stats are still recorded once. Each withLedgerAttempts layer installs a new observer, but ratelimit.ObserveWait (pkg/ratelimit/wait_observer.go:49-53) calls only the innermost one, and production always installs the outer observer first (syncer.go:841).

I applied the repo-local criteria. Triage:

• Silence: no. A mis-routed commit is now a type error, not a runtime fallback.
• Durability: indirect. Ledger commit semantics are untouched.
• Uncontrolled dimensions: no. Shared mutable syncer state was removed.
• Consumer distance: same package.

Verdict: LOW. The renamed Sync* methods hang off the unexported syncer type, so no exported API, proto, serialized-state, default, or dependency surface changes. No go.mod changes.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: 860b4380dab7

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit ff05f899cf19

General PR Review: sync: ledger handlers take the page invocation as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: incremental since 860b438
View review run

Review Summary

The two new commits change what the handlers receive. Instead of a *ledgerInvocation pulled from ctx, every ledger handler now gets *ledgerPage as its parameter: func(ctx, *ledgerPage, *Action) error. stage, action, attempts, worker, children, afterCommit, connectorObservations and restart move from ledgerInvocation onto ledgerPage. The page also gets recordDuration, markTypeScopedPlanned and addObservations helpers. invokeActionPage now takes the worker, the attempts and the queue transition explicitly, and transitionActionState takes ledgerTransitionCommit as a parameter. The global parallelActionTransitioner and the four ctx keys are deleted.

The PR body still describes the earlier shape (inv *ledgerInvocation, inv.stage(...)). The code actually uses *ledgerPage / page.stage(...). This doesn't affect behavior, but reviewers should read the code rather than the body.

I scanned the full PR diff (all 40 files, all in pkg/sync) for security and correctness. Points I checked:

• opened cannot be nil on success. runPageWithCommit always calls the handler before it can return success (ledger_page.go:159-166).
• Worker and attempts are the same as before. Coordinator steps use worker 0 with coordinatorAttempts. Init uses worker 0 with fresh attempts. Workers use index i with their own attempts.
• Commits still go through queue.transition only inside syncParallel.
• Retry-wait stats are still recorded once. ratelimit.ObserveWait calls only the innermost observer (pkg/ratelimit/wait_observer.go:49-53).
• Every remaining helper is only called with a non-nil page. This covers getLedgerResourceFromConnector, recordLedgerConnectorResponse and ledgerCollection.

Repo criteria triage:

• Silence: no. Stage/commit semantics are unchanged.
• Durability: no. No change to the format of the ledger, c1z or tokens.
• Uncontrolled dimensions: low. Attempts still belong to one goroutine and are now passed explicitly.
• Consumer distance: none. All the changed symbols are unexported.

Verdict: LOW. The rest of the criteria don't apply: no exported API, proto, serialized state, default or dependency changes.

Not covered: I didn't run tests; this review is read-only. Two items rely on the author's description and on the compiler rather than on me checking them: the deleted pin tests (next_page_callers_meta_test.go) and the new parameter-swapped test signatures.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

Superseded — see the current review report for commit c942edd6da3e

General PR Review: sync: ledger handlers take the page invocation as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: incremental since 860b438
View review run

Review Summary

The two new commits change what the handlers receive. Instead of a *ledgerInvocation pulled from ctx, every ledger handler now gets *ledgerPage as its parameter: func(ctx, *ledgerPage, *Action) error. stage, action, attempts, worker, children, afterCommit, connectorObservations and restart move from ledgerInvocation onto ledgerPage. The page also gets recordDuration, markTypeScopedPlanned and addObservations helpers. invokeActionPage now takes the worker, the attempts and the queue transition explicitly, and transitionActionState takes ledgerTransitionCommit as a parameter. The global parallelActionTransitioner and the four ctx keys are deleted.

The PR body still describes the earlier shape (inv *ledgerInvocation, inv.stage(...)). The code actually uses *ledgerPage / page.stage(...). This doesn't affect behavior, but reviewers should read the code rather than the body.

I scanned the full PR diff (all 40 files, all in pkg/sync) for security and correctness. Points I checked:

• opened cannot be nil on success. runPageWithCommit always calls the handler before it can return success (ledger_page.go:159-166).
• Worker and attempts are the same as before. Coordinator steps use worker 0 with coordinatorAttempts. Init uses worker 0 with fresh attempts. Workers use index i with their own attempts.
• Commits still go through queue.transition only inside syncParallel.
• Retry-wait stats are still recorded once. ratelimit.ObserveWait calls only the innermost observer (pkg/ratelimit/wait_observer.go:49-53).
• Every remaining helper is only called with a non-nil page. This covers getLedgerResourceFromConnector, recordLedgerConnectorResponse and ledgerCollection.

Repo criteria triage:

• Silence: no. Stage/commit semantics are unchanged.
• Durability: no. No change to the format of the ledger, c1z or tokens.
• Uncontrolled dimensions: low. Attempts still belong to one goroutine and are now passed explicitly.
• Consumer distance: none. All the changed symbols are unexported.

Verdict: LOW. The rest of the criteria don't apply: no exported API, proto, serialized state, default or dependency changes.

Not covered: I didn't run tests; this review is read-only. Two items rely on the author's description and on the compiler rather than on me checking them: the deleted pin tests (next_page_callers_meta_test.go) and the new parameter-swapped test signatures.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: ff05f899cf19

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit c942edd6da3e

General PR Review: sync: ledger handlers take the page invocation as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: incremental since ff05f899
View review run

Review Summary

The new commit c942edd6 deletes the intermediate ledgerInvocation struct. Inside the runPageWithCommit callback, invokeActionPage now writes action, attempts and worker straight onto the fresh *ledgerPage (pkg/sync/ledger_scheduler.go:67-71). It also renames the ledgerPage method receivers to p.

I scanned the full PR diff (40 files, all in pkg/sync) for security and correctness:

• Handlers. They now take (ctx, *ledgerPage, *Action) and stage through page.stage.
• Commits. The pending ledger commit is now a parameter of transitionActionState. The queue transition closure replaces the mutex-guarded parallelActionTransitioner field.
• Wait observers. One is installed per worker or coordinator ledgerAttempts. It is shadowed correctly because ratelimit.ObserveWait calls only the innermost observer (pkg/ratelimit/wait_observer.go:49-53).
• Page state. ledgerPage is allocated fresh on every call (pkg/sync/ledger_page.go:159), so per-page children, afterCommit and restart cannot leak between pages, just as with the old per-call invocation.
• Worker indices and attempts. Coordinator steps use worker 0 with coordinatorAttempts, init uses worker 0 with fresh attempts, and syncParallel workers use index i with their own attempts. This matches the old context values.

Note: the PR body still says handlers take inv *ledgerInvocation and call inv.stage(...). After this commit they take *ledgerPage and call page.stage(...). The code and the head commit message agree; only the description is out of date.

Criteria applied (risk triage per docs/BUG_CATCHING.md §2):

• Silence — no. This is a mechanical refactor whose old runtime check ("requires a ledger commit") is now enforced by the compiler.
• Durability — no. No c1z, ledger-row, token or proto format changes.
• Uncontrolled dimensions — no new ones. ledgerAttempts still has one owner per goroutine, and the pin test TestLedgerWaitObservationsStayWithWorker is kept.
• Consumer distance — same package only. The changed Sync* methods are on the unexported syncer, so there is no exported API, wire or serialized-state surface.
• Verdict: LOW. No escalation needed. No dependency or proto changes, and nothing touches defaults.

There were no prior inline findings to audit. I found no coverage gaps.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

Superseded — see the current review report for commit 1ac1b9ae1cc2

General PR Review: sync: ledger handlers take the page invocation as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: incremental since ff05f899
View review run

Review Summary

The new commit c942edd6 deletes the intermediate ledgerInvocation struct. Inside the runPageWithCommit callback, invokeActionPage now writes action, attempts and worker straight onto the fresh *ledgerPage (pkg/sync/ledger_scheduler.go:67-71). It also renames the ledgerPage method receivers to p.

I scanned the full PR diff (40 files, all in pkg/sync) for security and correctness:

• Handlers. They now take (ctx, *ledgerPage, *Action) and stage through page.stage.
• Commits. The pending ledger commit is now a parameter of transitionActionState. The queue transition closure replaces the mutex-guarded parallelActionTransitioner field.
• Wait observers. One is installed per worker or coordinator ledgerAttempts. It is shadowed correctly because ratelimit.ObserveWait calls only the innermost observer (pkg/ratelimit/wait_observer.go:49-53).
• Page state. ledgerPage is allocated fresh on every call (pkg/sync/ledger_page.go:159), so per-page children, afterCommit and restart cannot leak between pages, just as with the old per-call invocation.
• Worker indices and attempts. Coordinator steps use worker 0 with coordinatorAttempts, init uses worker 0 with fresh attempts, and syncParallel workers use index i with their own attempts. This matches the old context values.

Note: the PR body still says handlers take inv *ledgerInvocation and call inv.stage(...). After this commit they take *ledgerPage and call page.stage(...). The code and the head commit message agree; only the description is out of date.

Criteria applied (risk triage per docs/BUG_CATCHING.md §2):

• Silence — no. This is a mechanical refactor whose old runtime check ("requires a ledger commit") is now enforced by the compiler.
• Durability — no. No c1z, ledger-row, token or proto format changes.
• Uncontrolled dimensions — no new ones. ledgerAttempts still has one owner per goroutine, and the pin test TestLedgerWaitObservationsStayWithWorker is kept.
• Consumer distance — same package only. The changed Sync* methods are on the unexported syncer, so there is no exported API, wire or serialized-state surface.
• Verdict: LOW. No escalation needed. No dependency or proto changes, and nothing touches defaults.

There were no prior inline findings to audit. I found no coverage gaps.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: c942edd6da3e

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 1ac1b9ae1cc2

General PR Review: sync: ledger handlers take the page as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: incremental since c942edd
View review run

Review Summary

New commit. Commit 1ac1b9a adds thin forwarding methods on *ledgerPage: PutResourceTypes, PutResources, PutEntitlements, PutGrants, PutAsset and GetResource (pkg/sync/ledger_page.go:71-93). Handlers and tests now call page.X instead of page.writer.X. Each method forwards straight to the same PageWriter call, so behaviour is unchanged. The only direct page.writer. uses left are in page/scheduler plumbing (SetPendingWork, SetRestart, SetCounterBucket, Commit), not in data handlers.

Full-diff pass. I scanned the whole PR diff for security and correctness, including invokeActionPage, syncParallel/syncOneAction, transitionActionState and withLedgerAttempts. The coordinator path passes transition=nil and goes straight to transitionActionState. That matches the old behaviour, because parallelActionTransitioner was only installed while syncParallel ran. On the success path, opened is always non-nil because runPageWithCommit only succeeds after it has invoked the handler.

Criteria triage. I applied the repo criteria triage to the whole PR:

• Silence: no change to persisted output.
• Durability: no proto, pb or dotc1z changes, and row fields are written at the same points.
• Uncontrolled dimensions: ledgerAttempts still has one owner per goroutine, and the wait observer is innermost-only.
• Consumer distance: same package only, since all changed Sync* methods are on the unexported syncer.

Verdict: LOW. There is no exported API, wire or serialized-state surface, and no change to go.mod.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

Superseded — see the current review report for commit 75c4b7879c92

General PR Review: sync: ledger handlers take the page as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: incremental since c942edd
View review run

Review Summary

New commit. Commit 1ac1b9a adds thin forwarding methods on *ledgerPage: PutResourceTypes, PutResources, PutEntitlements, PutGrants, PutAsset and GetResource (pkg/sync/ledger_page.go:71-93). Handlers and tests now call page.X instead of page.writer.X. Each method forwards straight to the same PageWriter call, so behaviour is unchanged. The only direct page.writer. uses left are in page/scheduler plumbing (SetPendingWork, SetRestart, SetCounterBucket, Commit), not in data handlers.

Full-diff pass. I scanned the whole PR diff for security and correctness, including invokeActionPage, syncParallel/syncOneAction, transitionActionState and withLedgerAttempts. The coordinator path passes transition=nil and goes straight to transitionActionState. That matches the old behaviour, because parallelActionTransitioner was only installed while syncParallel ran. On the success path, opened is always non-nil because runPageWithCommit only succeeds after it has invoked the handler.

Criteria triage. I applied the repo criteria triage to the whole PR:

• Silence: no change to persisted output.
• Durability: no proto, pb or dotc1z changes, and row fields are written at the same points.
• Uncontrolled dimensions: ledgerAttempts still has one owner per goroutine, and the wait observer is innermost-only.
• Consumer distance: same package only, since all changed Sync* methods are on the unexported syncer.

Verdict: LOW. There is no exported API, wire or serialized-state surface, and no change to go.mod.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: 1ac1b9ae1cc2

[github-actions]

No blocking issues found — see the full review report

[ggreer]

It might be nice to have the actions in this error.

[kans]

Done in the next commit: ledger page for <op> <id> transitioned <op> <id>.

[github-actions]

Superseded — see the current review report for commit 75c4b7879c92

General PR Review: sync: ledger handlers take the page as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: incremental since 1ac1b9ae
View review run

Review Summary

What the new commit changes: one line. ledgerPage.stage (pkg/sync/ledger_page.go:97) now names the op and ID of both actions when a page transitions the wrong action. Before, it returned a fixed string. ActionOp has a String() method, so %s prints the op name.

Full-PR pass: I scanned the whole diff for security and correctness and read every non-test hunk. The refactor matches the PR description:

• Handlers now get the *ledgerPage as a parameter, and the four context keys are gone.
• invokeActionPage now receives the worker, attempts and transition function as parameters. Every caller passes the same values the old context lookups produced: the Init page gets worker 0, its own attempts and a direct transitionActionState; serial steps get the coordinator's attempts; syncParallel workers get their own attempts and the queue transition.
• Pages always start with a fresh row (ledger_page.go:186), so markTypeScopedPlanned is equivalent to the old unconditional assignment.
• ratelimit.ObserveWait calls only the innermost observer (pkg/ratelimit/wait_observer.go:49-53). Retry waits are still recorded once, into the right ledgerAttempts.

Repo-local criteria (applied): I scored the risk triage as LOW:

• Silence: no. The removed context-key paths are now compile-time parameters.
• Durability: no. No proto, pb or dotc1z changes, and row fields are written at the same points.
• Uncontrolled dimensions: no. Each goroutine still owns its ledgerAttempts.
• Consumer distance: none outside the package. Every changed Sync* method is on the unexported syncer.

No exported API, wire format, serialized state, default or dependency surface changes, so no escalation instrument is needed. There were no prior findings to audit.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

General PR Review: sync: ledger handlers take the page as a parameter

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 0a61df5c4bdf.
Review mode: incremental since 1ac1b9ae
View review run

Review Summary

What the new commit changes: one line. ledgerPage.stage (pkg/sync/ledger_page.go:97) now names the op and ID of both actions when a page transitions the wrong action. Before, it returned a fixed string. ActionOp has a String() method, so %s prints the op name.

Full-PR pass: I scanned the whole diff for security and correctness and read every non-test hunk. The refactor matches the PR description:

• Handlers now get the *ledgerPage as a parameter, and the four context keys are gone.
• invokeActionPage now receives the worker, attempts and transition function as parameters. Every caller passes the same values the old context lookups produced: the Init page gets worker 0, its own attempts and a direct transitionActionState; serial steps get the coordinator's attempts; syncParallel workers get their own attempts and the queue transition.
• Pages always start with a fresh row (ledger_page.go:186), so markTypeScopedPlanned is equivalent to the old unconditional assignment.
• ratelimit.ObserveWait calls only the innermost observer (pkg/ratelimit/wait_observer.go:49-53). Retry waits are still recorded once, into the right ledgerAttempts.

Repo-local criteria (applied): I scored the risk triage as LOW:

• Silence: no. The removed context-key paths are now compile-time parameters.
• Durability: no. No proto, pb or dotc1z changes, and row fields are written at the same points.
• Uncontrolled dimensions: no. Each goroutine still owns its ledgerAttempts.
• Consumer distance: none outside the package. Every changed Sync* method is on the unexported syncer.

No exported API, wire format, serialized state, default or dependency surface changes, so no escalation instrument is needed. There were no prior findings to audit.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: 75c4b7879c92

[github-actions]

No blocking issues found — see the full review report