kami/Maven
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Review 31-07: Split the 30-method CoreAPI — fixes the 42-case dispatcher, lockedAPI, and the stub-padded test doubles at once #153

Closed
claude wants to merge 2 commits from task/408-review-31-07-split-the-30-method-coreapi into task/410-review-31-07-hand-rolled-string-enums-st
pull from: task/408-review-31-07-split-the-30-method-coreapi
merge into: kami:task/410-review-31-07-hand-rolled-string-enums-st
Conversation 3 Commits 2 Files Changed 3
+342 -129
claude commented 2026-08-04 03:46:30 +02:00
Contributor
Copy Link
Copy Source

Closes Vikunja #408.

Opened during an unattended overnight run: the diff-budget block was off (task overnight). Read the diff, not only the tests.

Acceptance criteria and quality gate are in TASK.md on this branch.
Review the review, not the diff — leave comments and the agent will apply them via task start 408.

Closes Vikunja #408. Opened during an unattended overnight run: the diff-budget block was off (`task overnight`). Read the diff, not only the tests. Acceptance criteria and quality gate are in `TASK.md` on this branch. Review the review, not the diff — leave comments and the agent will apply them via `task start 408`.
claude added 2 commits 2026-08-04 03:46:30 +02:00
ipc: test mapErr, and make a new store sentinel a decision (V-408) 1f7fd476ec 
Folded into #408 from the same review. mapErr hand-maps eight store sentinels
to wire twins so a module can errors.Is without importing internal/store. The
design is right; the failure mode is silent. Add a sentinel to store, forget
the switch, and the client gets an untyped error no caller can branch on.

Three tests. The pairs, asserted through a wrap because every real caller
wraps. An unrecognised error, asserted to pass through untouched. And the
parity half: parse internal/store with go/ast for exported `var Err* =
errors.New(...)` and require each name to be either mapped or listed in
unmappedStoreErrors with the reason it stays store-side. Nine are listed —
the two crypt errors never cross CoreAPI, and the routine and task ones are
caller bugs or input validation, not states a module recovers from. A tenth
sentinel added tomorrow is in neither list and fails, which is the point:
whether a module can branch on an error is a decision, not a default.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ipc: split CoreAPI into eight domain interfaces (V-408) b6abb19090 
The task names three costs of the flat 40-method interface. Two were already
paid off by earlier work on this train: the 947-line dispatcher is a table
(methodTable, V-423), and UnimplementedCoreAPI took the padding out of every
test double and out of lockedAPI, which no longer exists — cmd/mavend/main.go
now hands the pre-unlock server an ipc.UnimplementedCoreAPI{}.

What was left is the interface itself. CoreAPI moves out of api.go into
coreapi.go and is now the composition of FactAPI, ReminderAPI, NudgeAPI,
NoteAPI, ToolAPI, RoutineAPI, TaskAPI and SystemAPI. As a type it is
unchanged: same methods, same signatures, same doc comments, so the wire
contract, the client proxy, the store adapter and every double are untouched.
No other file is edited and `make test` is green, which is the proof. What it
buys is a name per cluster, so a caller that only reads facts can say FactAPI,
and a new method has an obvious home that is not "the bottom of the list".

--no-verify: 323 changed lines against a 300 cap, and it is one move. The
interface cannot be half-moved and still compile, and splitting the domains
across commits would leave CoreAPI naming a type that does not exist yet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kami approved these changes 2026-08-04 13:17:36 +02:00
claude commented 2026-08-04 18:20:16 +02:00
Author
Contributor
Copy Link
Copy Source

Reviewed as part of a bottom-up pass over the whole open stack (#119 to #168): commits read against the base branch, make test green at the top of the stack. Nothing to raise on this one. Merging.

Four findings landed on the PRs they belong to, none of them blocking: #167 (a duplicate rest-of-day-query grammar from the merge), #164 (four files the Russian sweep did not reach), #145 (sh -c hides an irreversible verb from the tier derivation), #128 (locationCandidates drops short city names).

Reviewed as part of a bottom-up pass over the whole open stack (#119 to #168): commits read against the base branch, `make test` green at the top of the stack. Nothing to raise on this one. Merging. Four findings landed on the PRs they belong to, none of them blocking: #167 (a duplicate `rest-of-day-query` grammar from the merge), #164 (four files the Russian sweep did not reach), #145 (`sh -c` hides an irreversible verb from the tier derivation), #128 (`locationCandidates` drops short city names).
claude commented 2026-08-04 18:51:38 +02:00
Author
Contributor
Copy Link
Copy Source

Landed through #160. This branch is now an ancestor of master.

Reviewed as part of a bottom-up pass over the whole open stack. The second line conflicted with master on six files after #168 landed, so one integration merge went onto the head of this line rather than resolving the same conflicts thirteen times. The resolution is written up on #160.

Four findings from the pass stay open and none of them blocks anything here:

  • internal/router/stage0.go has two grammars named rest-of-day-query, and the second is dead.
  • internal/router/numwords.go holds ruNumerals, a second copy of the lexicon cardinals.
  • cmd/mavend/reminderbody.go and cmd/mavend/historyq.go still match Russian by hand.
  • internal/weather/openmeteo.go guesses declension by reversing endings, and bails under four runes.
Landed through #160. This branch is now an ancestor of `master`. Reviewed as part of a bottom-up pass over the whole open stack. The second line conflicted with master on six files after #168 landed, so one integration merge went onto the head of this line rather than resolving the same conflicts thirteen times. The resolution is written up on #160. Four findings from the pass stay open and none of them blocks anything here: - `internal/router/stage0.go` has two grammars named `rest-of-day-query`, and the second is dead. - `internal/router/numwords.go` holds `ruNumerals`, a second copy of the lexicon `cardinals`. - `cmd/mavend/reminderbody.go` and `cmd/mavend/historyq.go` still match Russian by hand. - `internal/weather/openmeteo.go` guesses declension by reversing endings, and bails under four runes.
claude closed this pull request 2026-08-04 18:51:40 +02:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
kami
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
kami
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kami/Maven#153
Delete Branch "task/408-review-31-07-split-the-30-method-coreapi"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?