kanban: an unattached BoardModel replies "stoull", and two guards written to prevent that never fire

[Yaraslaut]

What happens

BoardModel is keyed by project id, so a freshly registered handler is attached to no board. Every action dispatched in that state answers with the bare string stoull — the what() of the std::invalid_argument that std::stoull("") throws — delivered as though it were a domain error.

send register typeId=BoardModel      -> ok
execute GetBoardState                -> err "stoull"
execute CreateColumn name="Todo"     -> err "stoull"
execute CreateTask ...               -> err "stoull"
execute MoveTaskPosition ...         -> err "stoull"
execute AddComment ...               -> err "stoull"
execute GetEventsSince lastEventId=0 -> err "stoull"
execute GetActivity                  -> err "stoull"
execute GetRules projectId=1         -> err "stoull"

The part that makes it worth an issue

Two of those actions already have a guard with exactly the right message. GetActivity:

// examples/kanban/src/models/board_model.cpp
GetActivityResult BoardModel::execute(const GetActivity& /*action*/) {
    if (!_projectIdStr.has_value()) {
        throw NotFound{"GetActivity: handler was never attached via OpenBoard"};
    }
    requireRole(Role::Viewer);
    ...

and GetRules has the same shape. Neither ever fires. On an unattached handler _projectIdStr is engaged with an empty string, so has_value() is true, control falls through to requireRole, and requireRole reaches std::stoull(*_projectIdStr).

That is worse than having no guard: reading the source suggests the good message exists, so the behaviour looks correct until someone drives it. polls::PollModel gets this right and answers "GetPollState: handler was never attached via OpenPoll", which is the message a client can act on and the one this rung appears to have been copying.

Why it matters to a client

stoull names nothing: not the action, not the model, not the mistake. A client cannot distinguish "I forgot to call OpenBoard" from an internal fault, and cannot present anything useful. It also leaks an implementation detail of the id representation into the wire contract.

Suggested fix

Make the sentinel unambiguous — either leave _projectIdStr disengaged until OpenBoard succeeds, or have requireRole (and the other stoull call sites, board_model.cpp:304, 333, 347, 390, 432, 490, …) check for an empty string and throw the same NotFound{"<Action>: handler was never attached via OpenBoard"} the two existing guards already spell. Whichever way, the two guards that exist today should start firing.

Where it is pinned

scripts/scenario/scenarios/kanban/a-board-must-be-opened-before-it-answers.scenario asserts the current replies per action, including that the two guarded ones do not mention OpenBoard, so a fix has to come through that file.

Found by the out-of-process scenario corpus. The in-process tests attach before acting, so no test had reached this state over a server.

[Yaraslaut]

Triage: rescope — the defect is real, the framing understates it and mis-names the cause

The behaviour is confirmed and worth fixing. Two corrections have to land in the description before anyone builds it, because the fix the issue suggests first cannot be applied as written.

Correction 1 — it is not two dead guards, it is fifteen

$ grep -c "handler was never attached via OpenBoard" examples/kanban/src/models/board_model.cpp
15

GetBoardState (:329), CreateColumn (:343), CreateSwimlane (:386), CreateTask (:428), AddComment (:485), AddAttachment (:532), GetAttachments (:581), RemoveAttachment (:618), CreateRule (:659), GetRules (:705), DeleteRule (:742), ApplyTagMutation (:777), MoveTaskPosition (:845), GetEventsSince (:1126), GetActivity (:1151). Every one carries the same correct message, and every one is dead — a-board-must-be-opened-before-it-answers.scenario asserts stoull for each action it drives, and GetBoardState is in that list with its guard sitting one line above the stoull call.

That the guards pass is itself the proof the optional is engaged: were _projectIdStr disengaged, *_projectIdStr at :304 would be UB rather than a clean std::invalid_argument, and the has_value() checks would have returned NotFound first.

Correction 2 — the empty string is not incidental, and it comes from morph

_projectIdStr has two writers:

• execute(OpenBoard) — _projectIdStr = std::to_string(project.id.Value()) (board_model.cpp:323);
• BoardModel::attachActionLog(log, entityKey) — _projectIdStr = std::move(entityKey) (:223-226).

The second is reached on the ordinary construction path with an empty key. ModelFactory::create (include/morph/core/model.hpp:339-360) attaches the process-wide default log to every new holder and its own doc comment says how: "it is attached to the new holder automatically (with an empty entityKey)" → holder->attachActionLog(std::move(log), {}) (:357). kanban's App installs such a log (app.cpp:57, morph::journal::setActionLog(_actionLog)). Note this is not attachLogIfConfigured, which early-returns on an empty contextKey (remote.hpp:658-661) — that path is innocent.

So the member conflates two independent facts: "which board am I attached to" (OpenBoard) and "what entity key do I stamp on journal entries" (attachActionLog). polls::PollModel gets the right answer because it has no attachActionLog at all — _pollId records only the attach fact (poll_model.cpp:406), which is exactly why its guard fires and this one doesn't.

Why this changes the fix

The issue's first suggestion — "leave _projectIdStr disengaged until OpenBoard succeeds" — is not a local edit. It requires deciding what attachActionLog writes instead (it still needs a key for LogEntry::entityKey, :235/:277), and it would also change the keyed-attach path, where attachLogIfConfigured legitimately sets _projectIdStr to a real project id from a non-empty contextKey before any OpenBoard is dispatched.

The second suggestion is the correct minimal one: treat an empty _projectIdStr as unattached. Either refuse an empty entityKey in attachActionLog (one line, one place) or widen the fifteen guards to !has_value() || empty(). Both leave the keyed path untouched and make all fifteen guards live. Separating the two members is the cleaner long-term shape, but it is a different, larger change and should be argued on its own.

Also verified

All 21 kanban scenarios pass against a live ladder_kanban_server, so the pin file is accurate and a fix has to come through it.

Cross-reference

#369 wants GetRules to compare action.projectId against the attached board — which reads _projectIdStr through stoull. That comparison is unreachable on an unattached handler until this lands, so this issue sequences before #369.

Labels: triage: rescope, bug, area: ladder.