fix(#528): apply the three accepted Gemini review findings

1. An unprefixed reply no longer resolves an arbitrary pending command.
   #532 constrained prefixed replies only; the unprefixed fallback still
   used firstWhere, which with two or more in flight is map-iteration
   order, so a caller could receive another command's output. The
   fallback cannot just be deleted: firmware only echoes the prefix when
   the command exceeds four characters including it
   (simple_repeater/MyMesh.cpp, strlen(command) > 4 && command[2] == '|'),
   so a very short command legitimately answers without one. It now
   resolves only when exactly one command is pending, and surfaces the
   ambiguity otherwise.

2. A late reply no longer overwrites unsaved edits. The settings screen
   applied a late `get` payload unconditionally, so a reply arriving after
   its timeout could revert a field the user had typed into while waiting.
   It now applies only when nothing is dirty, and says which happened.

3. _expiredCommands is pruned on timeout as well as on receive. It was
   pruned only in handleResponse, so a run of commands that all time out
   with no traffic coming back accumulated records until disposal.

The reviewer described 3 as unbounded growth. It is bounded at 256 by the
prefix token space, and stale records could never be mis-attributed
because handleResponse prunes before matching, so the severity was
overstated. Fixed anyway; it is nearly free.

A fourth finding, that the new l10n strings are untranslated in the other
17 locales, is rejected. That is this project's established pipeline:
strings are authored in app_en.arb, gen-l10n emits English fallbacks, and
untranslated.json tracks the gap, which it now does for these keys. Every
string in the app arrived this way, so it is not a defect this branch
introduces.

The ambiguity test was verified to fail against the pre-fix code.
pull/561/head
Strycher 2 months ago
parent 89cc2b4e4d
commit cb281ccb59

@ -1586,6 +1586,7 @@
}
},
"repeater_cliUnmatchedResponse": "Unrequested response from repeater",
"repeater_lateResponseDiscarded": "A late response arrived but was not applied, because you have unsaved changes.",
"repeater_lateResponseReceived": "A late response arrived after the command timed out. See the CLI screen.",
"repeater_cliQuickGetName": "Get Name",
"repeater_cliQuickGetRadio": "Get Radio",

@ -5144,6 +5144,12 @@ abstract class AppLocalizations {
/// **'Unrequested response from repeater'**
String get repeater_cliUnmatchedResponse;
/// No description provided for @repeater_lateResponseDiscarded.
///
/// In en, this message translates to:
/// **'A late response arrived but was not applied, because you have unsaved changes.'**
String get repeater_lateResponseDiscarded;
/// No description provided for @repeater_lateResponseReceived.
///
/// In en, this message translates to:

@ -2919,6 +2919,10 @@ class AppLocalizationsBg extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2922,6 +2922,10 @@ class AppLocalizationsDe extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2863,6 +2863,10 @@ class AppLocalizationsEn extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2913,6 +2913,10 @@ class AppLocalizationsEs extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2935,6 +2935,10 @@ class AppLocalizationsFr extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2926,6 +2926,10 @@ class AppLocalizationsHu extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2919,6 +2919,10 @@ class AppLocalizationsIt extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2799,6 +2799,10 @@ class AppLocalizationsJa extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2797,6 +2797,10 @@ class AppLocalizationsKo extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2899,6 +2899,10 @@ class AppLocalizationsNl extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2927,6 +2927,10 @@ class AppLocalizationsPl extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2911,6 +2911,10 @@ class AppLocalizationsPt extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2917,6 +2917,10 @@ class AppLocalizationsRu extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2898,6 +2898,10 @@ class AppLocalizationsSk extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2897,6 +2897,10 @@ class AppLocalizationsSl extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2883,6 +2883,10 @@ class AppLocalizationsSv extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2915,6 +2915,10 @@ class AppLocalizationsUk extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -2751,6 +2751,10 @@ class AppLocalizationsZh extends AppLocalizations {
String get repeater_cliUnmatchedResponse =>
'Unrequested response from repeater';
@override
String get repeater_lateResponseDiscarded =>
'A late response arrived but was not applied, because you have unsaved changes.';
@override
String get repeater_lateResponseReceived =>
'A late response arrived after the command timed out. See the CLI screen.';

@ -194,12 +194,20 @@ class _RepeaterSettingsScreenState extends State<RepeaterSettingsScreen> {
/// (#528).
void _handleUnmatchedResponse(UnmatchedRepeaterResponse unmatched) {
if (!mounted) return;
if (unmatched.command != null) {
_handleGetResponse(unmatched.command!, unmatched.response);
}
// A late reply is stale by definition. Applying it over edits the user
// made while waiting would silently discard their input, so it only lands
// when nothing is unsaved (Gemini review).
final applied =
!_hasChanges &&
unmatched.command != null &&
_handleGetResponse(unmatched.command!, unmatched.response);
showDismissibleSnackBar(
context,
content: Text(context.l10n.repeater_lateResponseReceived),
content: Text(
applied
? context.l10n.repeater_lateResponseReceived
: context.l10n.repeater_lateResponseDiscarded,
),
);
}

@ -166,6 +166,11 @@ class RepeaterCommandService {
() {
final completer = _pendingCommands[commandId];
if (completer != null && !completer.isCompleted) {
// Prune here as well as on receive. A run of commands that all
// time out with no traffic coming back would otherwise accumulate
// records until disposal, since handleResponse is the only other
// place that prunes and it never runs (Gemini review).
_pruneExpiredCommands();
// Remember what this prefix asked so a reply arriving after the
// window can still reach the user with its question attached.
_expiredCommands[prefix] = _ExpiredCommand(
@ -211,13 +216,7 @@ class RepeaterCommandService {
// to matching by repeater.
final matchedId = prefix != null ? _pendingByPrefix[prefix] : null;
final commandId =
matchedId ??
(prefix != null
? ''
: _pendingCommands.keys.firstWhere(
(id) => id.startsWith(repeaterKey),
orElse: () => '',
));
matchedId ?? (prefix != null ? '' : _solePendingFor(repeaterKey));
if (commandId.isNotEmpty) {
final completer = _pendingCommands[commandId];
@ -267,6 +266,28 @@ class RepeaterCommandService {
);
}
/// The one command awaiting a reply from [repeaterKey], or `''` when that is
/// ambiguous.
///
/// An unprefixed reply carries no correlation token, so position is all that
/// is left to match on. Firmware only echoes the prefix when the command is
/// longer than four characters including it (`simple_repeater/MyMesh.cpp`
/// `strlen(command) > 4 && command[2] == '|'`), so a very short command can
/// legitimately answer without one and the fallback has to stay.
///
/// It is only sound with exactly one candidate though. With two or more,
/// "first" is map-iteration order, and choosing one would hand a caller
/// another command's output. Ambiguity is surfaced instead of guessed.
String _solePendingFor(String repeaterKey) {
String? only;
for (final id in _pendingCommands.keys) {
if (!id.startsWith(repeaterKey)) continue;
if (only != null) return '';
only = id;
}
return only ?? '';
}
Completer<String> _registerPending(String commandId, String prefix) {
final completer = Completer<String>();
_pendingCommands[commandId] = completer;

@ -221,4 +221,57 @@ void main() {
expect(surfaced, isEmpty);
});
});
group('unprefixed reply ambiguity (Gemini review)', () {
// #532 constrained PREFIXED replies. An unprefixed one still fell back to
// "first pending command for this repeater", which with two or more in
// flight is map-iteration order, so a caller could receive another
// command's output.
//
// The fallback cannot simply be deleted: firmware only echoes the prefix
// when the command exceeds four characters including it
// (simple_repeater/MyMesh.cpp, `strlen(command) > 4 && command[2] == '|'`),
// so a very short command legitimately replies without one.
test(
'with exactly one pending, an unprefixed reply still resolves it',
() async {
final only = service.registerPendingForTest(
repeater.publicKeyHex,
'E1|',
);
service.handleResponse(repeater, 'bare payload');
expect(await only, 'bare payload');
expect(surfaced, isEmpty);
},
);
test('with two pending, an unprefixed reply resolves NEITHER', () async {
final first = service.registerPendingForTest(
repeater.publicKeyHex,
'E1|',
);
final second = service.registerPendingForTest(
repeater.publicKeyHex,
'E2|',
);
var firstDone = false, secondDone = false;
unawaited(first.then((_) => firstDone = true));
unawaited(second.then((_) => secondDone = true));
service.handleResponse(repeater, 'ambiguous payload');
await Future<void>.delayed(Duration.zero);
expect(firstDone, isFalse);
expect(secondDone, isFalse);
expect(
surfaced.single.response,
'ambiguous payload',
reason: 'ambiguity must be surfaced, never guessed at',
);
expect(surfaced.single.command, isNull);
});
});
}

Loading…
Cancel
Save

Powered by TurnKey Linux.