From ba25e8f1764dad4cea5fbfb01fe314131ab68780 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Mon, 9 Nov 2026 18:18:30 +0100 Subject: [PATCH] Wait for the board before a merged Start The board's note cache lags ~10 s after a merge, so Start found "no startable item" (T1863); it now waits up to 30 s for the merged section's card. A Start POST slower than its timeout that did take effect was reported "board unreachable" (T1864); it now re-reads the board and reports ok when the card left Start. Todo: 64d39ab5 Co-Authored-By: Claude Opus 5.5 --- lib/data/repo_note_split.dart | 14 ++++- lib/mcp/board_client.dart | 101 ++++++++++++++++++++++++++------ lib/mcp/task_split.dart | 6 +- test/mcp/board_client_test.dart | 69 +++++++++++++++++++++- test/mcp/mcp_new_note_test.dart | 65 ++++++++++++++++---- 5 files changed, 219 insertions(+), 36 deletions(-) diff --git a/lib/data/repo_note_split.dart b/lib/data/repo_note_split.dart index 57e00bb..f45b13e 100644 --- a/lib/data/repo_note_split.dart +++ b/lib/data/repo_note_split.dart @@ -26,16 +26,24 @@ typedef SplitAdd = ({Note note, SplitKind kind, String? why}); final _heading = RegExp(r'^##\s+(\[x\]\s+)?(.*)$'); -/// The open `## title` headings of [text], outside code fences. -Iterable openSections(String text) sync* { +/// Every `## title` heading of [text] outside code fences, open or done. +Iterable _sections(String text) sync* { var fenced = false; for (final line in text.split('\n')) { if (line.trimLeft().startsWith('```')) fenced = !fenced; final m = fenced ? null : _heading.firstMatch(line); - if (m != null && m.group(1) == null) yield m.group(2)!.trim(); + if (m != null) yield m; } } +/// The open `## title` headings of [text], outside code fences. +Iterable openSections(String text) => _sections( + text, +).where((m) => m.group(1) == null).map((m) => m.group(2)!.trim()); + +/// How many board items [text] splits into: one per section, done or not. +int sectionCount(String text) => _sections(text).length; + /// The separate open note of [repo] with an open section titled [title]. Note? duplicateTarget(Iterable open, String repo, String title) { final hits = diff --git a/lib/mcp/board_client.dart b/lib/mcp/board_client.dart index 0b952ee..bed0930 100644 --- a/lib/mcp/board_client.dart +++ b/lib/mcp/board_client.dart @@ -5,6 +5,7 @@ /// a board that is down must not lose the task that was just saved. library; +import 'dart:async'; import 'dart:convert'; import 'package:http/http.dart' as http; @@ -19,17 +20,34 @@ final _csrf = RegExp(r'>> _cards(String noteId) async { final res = await _http.get(base.resolve('/api/board')).timeout(_timeout); @@ -54,30 +72,77 @@ class BoardClient { } } - /// Clicks Start on the note's last startable item titled [title]. - Future start(String noteId, String title) async { + /// Clicks Start on the note's last startable item titled [title]. With + /// [sections], first waits until the board shows the note's last section + /// (`-`), since a merge just added it. + Future start( + String noteId, + String title, { + int? sections, + }) async { try { - final card = (await _cards(noteId)).reversed - .where((c) => c['title'] == title && _startable(c)) - .firstOrNull; + final last = sections == null ? null : _item(noteId, sections - 1); + bool seen(List> cs) => + last == null || cs.any((c) => c['id'] == last); + final cards = await _until(noteId, seen); + // Never an older same-titled item in place of the merged one. + final card = seen(cards) + ? cards.reversed + .where((c) => c['title'] == title && _startable(c)) + .firstOrNull + : null; if (card == null) return _no('no startable item'); final page = await _http.get(base).timeout(_timeout); final token = _csrf.firstMatch(page.body)?.group(1); if (token == null) return _no('no CSRF token on the board page'); final id = card['id']! as String; - final res = await _http - .post( - base.resolve('/api/task/$id/start'), - headers: {'Origin': base.origin, 'X-Todoimpl-Token': token}, - ) - .timeout(_timeout); - if (res.statusCode == 200) return (ok: true, reason: null, item: id); - return _no('board refused start: ${res.statusCode} ${_message(res)}'); + return await _post(noteId, id, token); } on Object catch (e) { return _no('board unreachable: $e'); } } + Future _post(String noteId, String id, String token) async { + final http.Response res; + try { + res = await _http + .post( + base.resolve('/api/task/$id/start'), + headers: {'Origin': base.origin, 'X-Todoimpl-Token': token}, + ) + .timeout(startTimeout); + } on TimeoutException catch (e) { + // The board may still be starting it: a card that left Start took it. + final cards = await _until( + noteId, + (cs) => cs.any((c) => c['id'] == id && !_startable(c)), + ); + if (cards.any((c) => c['id'] == id && !_startable(c))) { + return (ok: true, reason: null, item: id); + } + return _no('board unreachable: $e'); + } + if (res.statusCode == 200) return (ok: true, reason: null, item: id); + return _no('board refused start: ${res.statusCode} ${_message(res)}'); + } + + /// The note's cards once [done] holds, or the last read after [wait]. + Future>> _until( + String noteId, + bool Function(List>) done, + ) async { + final deadline = DateTime.now().add(wait); + var cards = await _cards(noteId); + while (!done(cards) && DateTime.now().isBefore(deadline)) { + await Future.delayed(poll); + cards = await _cards(noteId); + } + return cards; + } + + static String _item(String noteId, int index) => + '${noteId.substring(0, 8)}-$index'; + static bool _startable(Map card) => (card['actions'] as List) .cast>() .any((a) => a['id'] == 'start' && a['enabled'] == true); diff --git a/lib/mcp/task_split.dart b/lib/mcp/task_split.dart index 9824780..8b88b55 100644 --- a/lib/mcp/task_split.dart +++ b/lib/mcp/task_split.dart @@ -41,7 +41,11 @@ Future addSplit( if (block != null) return '$reply\nstart: skipped ($block)'; final start = board == null ? (ok: false, reason: 'no board configured', item: null) - : await board.start(added.note.id, title.trim()); + : await board.start( + added.note.id, + title.trim(), + sections: sectionCount(added.note.text), + ); if (start.ok) return '$reply\nstart: ok ${start.item}'; return '$reply\nstart: {ok: false, reason: "${start.reason}"}; ' 'the merge is saved, click Start on the board by hand'; diff --git a/test/mcp/board_client_test.dart b/test/mcp/board_client_test.dart index 1c57c23..ebd0407 100644 --- a/test/mcp/board_client_test.dart +++ b/test/mcp/board_client_test.dart @@ -32,13 +32,26 @@ class _Board { String startBody = '{"ok":false,"error":{"message":"nope"}}'; String page = ''; - BoardClient client() => BoardClient( + /// Board reads so far; [onRead] may change [cards] between reads. + int reads = 0; + void Function(int read)? onRead; + Duration postDelay = Duration.zero; + + BoardClient client({ + Duration wait = Duration.zero, + Duration startTimeout = const Duration(seconds: 5), + }) => BoardClient( + wait: wait, + poll: const Duration(milliseconds: 5), + startTimeout: startTimeout, client: MockClient((req) async { if (req.method == 'POST') { posts.add(req); + await Future.delayed(postDelay); return http.Response(startBody, startStatus); } if (req.url.path == '/api/board') { + onRead?.call(++reads); return http.Response.bytes( utf8.encode(jsonEncode({'tasks': cards})), boardStatus, @@ -124,6 +137,60 @@ void main() { ); }); + test('T1863: waits for the board to see a freshly merged section', () async { + final b = _Board([_card('cccccccc-0', 'Młody')]); + b.onRead = (n) { + if (n == 3) b.cards.add(_card('cccccccc-1', 'Młody')); + }; + final r = await b + .client(wait: const Duration(seconds: 2)) + .start(_id, 'Młody', sections: 2); + expect(r, (ok: true, reason: null, item: 'cccccccc-1')); + }); + + test('T1863: a board that never shows the section gives up', () async { + final b = _Board([_card('cccccccc-0', 'Młody')]); + final r = await b + .client(wait: const Duration(milliseconds: 50)) + .start(_id, 'Młody', sections: 2); + expect(r.ok, isFalse); + expect(r.reason, 'no startable item'); + expect(b.posts, isEmpty); + }); + + test('T1864: a slow Start that took effect is ok, not unreachable', () async { + final b = _Board([_card('cccccccc-0', 'Młody')]) + ..postDelay = const Duration(milliseconds: 200); + b.onRead = (n) { + // After the POST was sent the card has left backlog (no Start action). + if (b.posts.isNotEmpty) { + b.cards[0] = _card( + 'cccccccc-0', + 'Młody', + column: 'running', + start: false, + ); + } + }; + final r = await b + .client(startTimeout: const Duration(milliseconds: 50)) + .start(_id, 'Młody'); + expect(r, (ok: true, reason: null, item: 'cccccccc-0')); + }); + + test( + 'T1864: a slow Start that did not take effect is still a reason', + () async { + final b = _Board([_card('cccccccc-0', 'Młody')]) + ..postDelay = const Duration(milliseconds: 200); + final r = await b + .client(startTimeout: const Duration(milliseconds: 50)) + .start(_id, 'Młody'); + expect(r.ok, isFalse); + expect(r.reason, startsWith('board unreachable')); + }, + ); + test('the default client targets the loopback board', () { expect(BoardClient().base.toString(), 'http://127.0.0.1:8786'); }); diff --git a/test/mcp/mcp_new_note_test.dart b/test/mcp/mcp_new_note_test.dart index d2a1d1c..2c2cd93 100644 --- a/test/mcp/mcp_new_note_test.dart +++ b/test/mcp/mcp_new_note_test.dart @@ -6,6 +6,7 @@ import 'package:http/http.dart' as http; import 'package:http/testing.dart'; import 'package:mcp_dart/mcp_dart.dart'; import 'package:todo/mcp/board_client.dart'; +import 'package:todo/mcp/task_service.dart'; import 'mcp_harness.dart'; @@ -15,9 +16,14 @@ void main() { late McpHarness h; late McpClient c; var cards = >[]; + // Cards the board shows only from its next read on (its note cache lags). + var late_ = >[]; + var lag = 0; // board reads before [late_] shows var up = true; final posts = []; final board = BoardClient( + wait: const Duration(seconds: 2), + poll: const Duration(milliseconds: 5), client: MockClient((req) async { if (!up) throw http.ClientException('connection refused'); if (req.method == 'POST') { @@ -25,17 +31,26 @@ void main() { return http.Response('{"ok":true}', 200); } if (req.url.path == '/api/board') { + if (lag-- == 0) { + cards = [...cards, ...late_]; + late_ = []; + } + final shown = [...cards]; return http.Response.bytes( - utf8.encode(jsonEncode({'tasks': cards})), + utf8.encode(jsonEncode({'tasks': shown})), 200, ); } return http.Response('', 200); }), ); - List> cardsOf(String id, String column) => [ + List> cardsOf( + String id, + String column, [ + int index = 0, + ]) => [ { - 'id': '${id.substring(0, 8)}-0', + 'id': '${id.substring(0, 8)}-$index', 'title': 'Młody', 'column': column, 'actions': [ @@ -46,6 +61,8 @@ void main() { setUp(() async { cards = []; + late_ = []; + lag = 0; up = true; posts.clear(); h = McpHarness(board: board); @@ -78,16 +95,22 @@ void main() { expect(posts, isEmpty); }); - test('C3/C6: a duplicate merges and Start is POSTed', () async { - final x = idIn(await add({'done_check': 'true'})); - cards = cardsOf(x, 'backlog'); - final r = await add({'done_check': 'true'}); - expect(r, startsWith('merged into $x')); - expect(r, contains('start: ok ${x.substring(0, 8)}-0')); - expect(posts, ['/api/task/${x.substring(0, 8)}-0/start']); - final listed = await h.call(c, 'list_tasks', {'repo': _repo}); - expect(listed.trim().split('\n'), hasLength(1)); - }); + test( + 'C3/C6/T1863: a duplicate merges, Start waits for the new item', + () async { + final x = idIn(await add({'done_check': 'true'})); + cards = cardsOf(x, 'backlog'); + // The merged section shows up one read later than the merge itself. + late_ = cardsOf(x, 'backlog', 1); + lag = 2; // past the guard's running read and the first Start read + final r = await add({'done_check': 'true'}); + expect(r, startsWith('merged into $x')); + expect(r, contains('start: ok ${x.substring(0, 8)}-1')); + expect(posts, ['/api/task/${x.substring(0, 8)}-1/start']); + final listed = await h.call(c, 'list_tasks', {'repo': _repo}); + expect(listed.trim().split('\n'), hasLength(1)); + }, + ); test('C6: done_check none or low priority merges without Start', () async { final x = idIn(await add({})); @@ -120,6 +143,22 @@ void main() { expect(r, contains('click Start on the board by hand')); }); + test('a non-boolean new_note is refused by the service too', () { + // The MCP schema rejects it first; the service guards its own callers. + expect( + TaskService( + h.session, + ).add({'repo': _repo, 'title': 'Młody', 'new_note': 'yes'}), + throwsA( + isA().having( + (e) => e.message, + 'message', + 'new_note: must be a boolean', + ), + ), + ); + }); + test('the add_task schema offers new_note', () async { final tools = (await c.listTools()).tools; final add = tools.singleWhere((t) => t.name == 'add_task');