feat(dashboard): mesh_bridge slice of #2848 — the signed rail honours the kill switch and an e-stop reports what the peers said (#2977) - #3205
Conversation
…urs the mesh kill switch The dashboard's mesh peer, extracted from draft strands-labs#2848 as a slice of the strands-labs#2977 decomposition, with the two mesh-safety findings from that draft's review (7 and 8) fixed on arrival rather than carried: - signed_estop no longer answers three different fleets with one payload: lockout_engaged keeps its value (it is true, and the resume control is gated on it) and the peer half rides alongside it as responses_received and peers_not_stopped, graded by mesh.core._peers_that_did_not_stop rather than by a second copy of that rule on the safety path. - _safety_mesh, the bridge's second Mesh construction site, asks mesh_disabled_by_env() before the constructor, so STRANDS_MESH=false no longer gains a *-safety gateway peer from the first e-stop. The refusal names the switch; a genuinely broken rail still answers "unavailable". Adapted from the draft to this tree's vocabulary: the kill switch is mesh_disabled_by_env() (the draft's mesh_kill_switch_engaged does not exist here), and the joint_silence annotation grading plus the /api/safety/estop route pin stay with the server slice that supplies their inputs. Refs strands-labs#2848, strands-labs#2977. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jmv1uhoHXZxuP3GKhfJuq2
…d by its owner STRANDS_MESH_CAMERA_HZ is the mesh's knob, not the dashboard's. Mesh._resolve_camera_hz reads it through mesh.session.hz_from_env and disables the camera loop when it is unset, non-positive, or a value no loop can pace itself with; the dashboard is the surface that WRITES it, since the settings panel holds camera_hz and settings.apply_mesh_env pushes it into the environment the peers read. mesh_info read it back through a bare float(), so six of nine spellings an operator can type disagreed with what the peer published: -5, nan, inf and 1e999 were echoed to the panel as live rates, nan/inf are not JSON so /api/mesh/config emitted a body no conformant client can parse, and a typo or whitespace raised ValueError out of the handler that would have shown the posture. It now asks hz_from_env and mirrors the publisher's documented fallback. The bridge's own STRANDS_DASHBOARD_* rate and TTL knobs are resolved at import, where a bare float() costs more: a typo raised ValueError while the module body executed, so the bridge did not lose one knob but failed to import from a frame naming float rather than the variable, and a non-finite value was accepted and reached each consumer as one side of a comparison it removes rather than widens - age > ttl is False for every age against both nan and inf, so a departed robot was never aged out of the fleet snapshot. They now route through the shared utils.finite_number_error domain, in the shape simulation/isaac/simulation._env_float and mesh/core._parse_positive_float_env already use, with every rejection reported. Each knob's floor is left to its consumer as before: prune_peers reads a non-positive ttl as "never prune" and EventCoalescer.allow reads a non-positive rate as "no ceiling". Two further guards the suite reports on this module, both reached only after the first is cleared because the test step runs with -x: - The event coalescer compared an elapsed span against a period using time.time(), so an NTP step moved the decision by the size of the step - suppressing a reading that was due or forwarding one the rate had capped. It reads time.monotonic() now, and allow() documents the clock its caller owes it. - Three comments carried U+2014; sources are ASCII. The package-wide env-float sweep is widened to the position this defect took. It stated a rule over the package and derived its population from every function that reads the environment and coerces with float(), so a knob resolved by a module-level statement was not a function that failed the scan - it was invisible to it, which is the same blind spot the sweep's own docstring records for a per-package root. Module-level statements are classified too, keyed module::<module>, and a guard is credited only to the scope that applies it: a walk that descended into an enclosed def read a block as bounded by an isfinite it never called. upstream/main is at zero such statements, so the widening enrols no other module.
|
Pushed a fix for the required check, plus the two further guard failures on this module that the test step could not reach. 1.
|
STRANDS_MESH_CAMERA_HZ |
camera loop publishes at | panel said | panel says now | JSON before |
|---|---|---|---|---|
| (unset) | 0.0 |
0.0 |
0.0 ✅ |
yes |
5 |
5.0 |
5.0 |
5.0 ✅ |
yes |
0 |
0.0 |
0.0 |
0.0 ✅ |
yes |
-5 |
0.0 |
-5.0 ❌ |
0.0 ✅ |
yes |
nan |
0.0 |
nan ❌ |
0.0 ✅ |
NO (NaN/Infinity) |
inf |
0.0 |
inf ❌ |
0.0 ✅ |
NO (NaN/Infinity) |
1e999 |
0.0 |
inf ❌ |
0.0 ✅ |
NO (NaN/Infinity) |
not-a-number |
0.0 |
raises ValueError ❌ |
0.0 ✅ |
endpoint fails |
" " (whitespace) |
0.0 |
raises ValueError ❌ |
0.0 ✅ |
endpoint fails |
6 of 9 spellings disagreed -> 0. Three of them made /api/mesh/config emit NaN/Infinity, which is not JSON; two raised ValueError out of the handler that would have shown the posture. It now asks hz_from_env and mirrors the publisher's documented fallback (unusable or non-positive is 0.0, publishing off - frames are large, so substituting a rate nobody asked for is worse than not publishing).
2. The module's own knobs were coerced at import
PEER_TTL_S and the seven COALESCE_HZ rates. That position has a failure mode a per-call resolver does not:
$ STRANDS_DASHBOARD_PEER_TTL_S=abc python -c 'from strands_robots.dashboard import mesh_bridge'
ValueError: could not convert string to float: 'abc' # the module, not the knob
$ STRANDS_DASHBOARD_PEER_TTL_S=nan -> PEER_TTL_S=nan # 'age > ttl' is False for every age
$ STRANDS_DASHBOARD_PEER_TTL_S=inf -> PEER_TTL_S=inf # so a departed peer is never aged out
A non-finite value is not a widened bound, it is a removed one: nan/inf read as "no ceiling" to prune_peers and to the coalescer alike. These now route through the shared utils.finite_number_error domain in the shape simulation/isaac/simulation._env_float and mesh/core._parse_positive_float_env already use, and every rejection is logged. Each knob's floor is left to its consumer, unchanged: prune_peers reads a non-positive ttl as "never prune", EventCoalescer.allow reads a non-positive rate as "no ceiling".
3. Two guards the -x step never reached (both pre-existing on e6149eb)
| guard | finding |
|---|---|
test_expiry_gates_survive_a_clock_step |
_emit paced the coalescer with time.time(). What the coalescer compares is an elapsed span against 1.0 / hz, so an NTP step moved the decision by the size of the step - suppressing a reading that was due, or forwarding one the rate had capped. Reads time.monotonic() now; allow() documents the clock its caller owes it. |
test_source_strings_no_unicode_dashes |
three comments carried U+2014. |
4. The sweep now covers the position the defect took
test_env_float_knobs_resolve_to_a_finite_value.py states a rule over the whole package and derived its population from every function that reads the environment and coerces with float(). A knob resolved by a module-level statement was not a function that failed the scan - it was invisible to it, the same blind spot the sweep's own docstring records for a per-package root. Module-level statements are classified too (module::<module>), and a guard is credited only to the scope that applies it: a walk that descended into an enclosed def read a block as bounded by an isfinite it never called.
On e6149eb the widened sweep names both positions, where CI could only see one:
adrift: ['dashboard/mesh_bridge.py::<module>', 'dashboard/mesh_bridge.py::mesh_info']
upstream/main is at 0 module-level unguarded statements, so the widening enrols no other module.
Gate
Regression split on e6149eb with the new cells applied: 87 failed / 12 passed -> 99 passed, the 12 being the spellings a bare float() happens to get right plus the usable-rate controls.
| mutation | cells that fire |
|---|---|
| control (no mutation) | 0 |
| mesh_info back to a bare float() | 15 |
| module knobs back to a bare float() | 6 |
| _env_float also refuses non-positive | 2 |
| unusable rate substitutes a rate, not off | 5 |
| the non-positive-is-off rule dropped | 1 |
| the rejection is no longer reported | 1 |
| guard walks into nested scopes again | 2 |
| guard stops classifying module scope | 3 |
collected stable at 114 on every row; the control fires nothing.
| check | result |
|---|---|
ruff check / ruff format --check (strands_robots tests tests_integ) |
clean, 1875 files |
mypy strands_robots tests tests_integ |
Success: no issues found in 1872 source files |
| 222 tree-walking guards | 7 failed / 11139 passed |
tests/test_dashboard_*.py + tests/mesh/ |
5375 passed / 10 skipped / 0 failed |
| changelog fragment graders | 91 passed |
The 7 are this checkout's environment, not the branch: 5 assert rclpy is absent where /opt/ros/jazzy supplies it, and 2 read websockets 16.0 against the >=17.0 floor cosmos3-service declares (Server.shutdown does not exist at 16.0).
Production delta in mesh_bridge.py: 653 -> 678 executable statements (+25 by AST) - two resolvers and the routing; the rest of the diff is the docstrings arguing why. Docs: two entries in docs/dashboard/troubleshooting.md for the observable symptoms (a panel reporting camera_hz 0 after a rate was typed; a STRANDS_DASHBOARD_* knob with no effect).
|
The required check came back red on a test this branch does not touch, and it is a load-dependent flake rather than a regression - I have re-run the failed job.
Nothing in the diff reaches that path: it is |
…till computing `check_merge_blockers.py --pr N` reported an already-merged pull request as `merge-state-unknown`, owed by nobody, with the remedy "Re-read the pull request: mergeability is computed on demand and settles on a later read". No later read settles it. A merged pull request is closed, so `mergeable` stays null permanently -- strands-labs#2586 still read `null`/`unknown` fourteen days after it squashed -- and the advice describes a wait with no terminating condition, in the reassuring direction: "no party owes an action" is literally true while the answer is "you already merged it". `mergeable is None` was ambiguous between "GitHub is still computing" and "there is nothing left to compute". Measured on 2026-09-05 right after strands-labs#3219 and strands-labs#3230 squashed, both merged rows and open strands-labs#3205 were identical in every field the script read, so the genuine transient strands-labs#2585 was written for is live at the same time as the terminal state and must keep its re-read. The field that separates them was already in the payload: `resolve_state` fetched the whole REST pull request object and read five keys from it, and `merged` came back in the same response. - `PullRequestState` gains `merged`, populated from that key. - A terminal `already-merged` outcome, owed by nobody, reported ahead of every rule and short-circuiting them: on a change already on the base, "0 of 1 approvals" is not an unsatisfied rule. Not a finding, so the exit status keeps its meaning. - `primary` and `_next_action` gain a terminal tier ahead of the gating one, so a merged pull request cannot get "the answer is not in yet". - The `merge-state-unknown` docstring no longer implies its "every open pull request" clause covers the merged case. Reading a null as clean is still wrong (strands-labs#1035), and the open recomputing path is unchanged -- pinned by the control row of the new table cell. Closes strands-labs#3231
…till computing (#3233) `check_merge_blockers.py --pr N` reported an already-merged pull request as `merge-state-unknown`, owed by nobody, with the remedy "Re-read the pull request: mergeability is computed on demand and settles on a later read". No later read settles it. A merged pull request is closed, so `mergeable` stays null permanently -- #2586 still read `null`/`unknown` fourteen days after it squashed -- and the advice describes a wait with no terminating condition, in the reassuring direction: "no party owes an action" is literally true while the answer is "you already merged it". `mergeable is None` was ambiguous between "GitHub is still computing" and "there is nothing left to compute". Measured on 2026-09-05 right after #3219 and #3230 squashed, both merged rows and open #3205 were identical in every field the script read, so the genuine transient #2585 was written for is live at the same time as the terminal state and must keep its re-read. The field that separates them was already in the payload: `resolve_state` fetched the whole REST pull request object and read five keys from it, and `merged` came back in the same response. - `PullRequestState` gains `merged`, populated from that key. - A terminal `already-merged` outcome, owed by nobody, reported ahead of every rule and short-circuiting them: on a change already on the base, "0 of 1 approvals" is not an unsatisfied rule. Not a finding, so the exit status keeps its meaning. - `primary` and `_next_action` gain a terminal tier ahead of the gating one, so a merged pull request cannot get "the answer is not in yet". - The `merge-state-unknown` docstring no longer implies its "every open pull request" clause covers the merged case. Reading a null as clean is still wrong (#1035), and the open recomputing path is unchanged -- pinned by the control row of the new table cell. Closes #3231 Co-authored-by: cagataycali <cagataycali@users.noreply.github.com>
… the rule check_merge_blockers.py derives the party who owes the next action from the outcome alone. For every outcome but one that is enough. An absent approval is owed by "any reviewer" only where any reviewer's approval would count, and under require_last_push_approval the pusher's would not -- a second fact the evaluator has already resolved and prints in its own table one row above the party it contradicts. Measured live on strands-labs/robots, 2026-09-06: strands-labs#3205 and strands-labs#3212 both read missing-approval / "any reviewer" with "head pushed by cagataycali" in the same report. That account's approval cannot satisfy the count, so the review round the report invites arrives at pusher-only-approval, which needs somebody else regardless -- the state strands-labs#2907 has been sitting in since it was approved by that same account on 2026-09-04. Blocker carries an explicit party, set where the rule discounts the pusher. The outcome is deliberately unchanged: it also decides gating, finding and exit status, and an unreviewed pull request is still the ordinary state, so it must not begin gating or reporting red merely because the eligible set is narrower. Four existing pins asserted "any reviewer" on a fixture whose branch carries the rule and whose head has a pusher; each is retargeted with its original point intact (strands-labs#2480 still asserts the party is a person rather than NOBODY).
|
Blocker measured 2026-09-06, Worth stating explicitly, because the report does not currently say it: the head was pushed by So the next action is owed by an approving reviewer other than Everything else is clean: green required check, 0 unresolved threads, (#3248 corrects the report so this case names the eligible party itself rather than "any reviewer".) |
cagataycali
left a comment
There was a problem hiding this comment.
The decomposition worked: at +2,058 across 11 files this slice can be read to a conclusion, which the 476-file draft could not. Both findings it claims are correctly resolved, and I could grade them rather than take them on faith. One finding below, of the same class as finding 8 and introduced by the same commit, on the method next door.
[MUST FIX] The resume half of the rail reports a switched-off rail as a fault
signed_estop learned to distinguish "you switched this off" from "this is broken". signed_resume (mesh_bridge.py:1062-1067) did not, and it is the same rail, the same cause and the same operator. Measured on the branch head, iterating the kill spellings from their owner (_mesh_switch.NEGATIVE):
STRANDS_MESH='false'
signed_estop -> {'signed': False, 'error': 'signed safety rail disabled by STRANDS_MESH=false'}
signed_resume -> {'signed': False, 'error': 'safety mesh unavailable'}
STRANDS_MESH='0'
signed_estop -> {'signed': False, 'error': 'signed safety rail disabled by STRANDS_MESH=false'}
signed_resume -> {'signed': False, 'error': 'safety mesh unavailable'}
STRANDS_MESH='no'
signed_estop -> {'signed': False, 'error': 'signed safety rail disabled by STRANDS_MESH=false'}
signed_resume -> {'signed': False, 'error': 'safety mesh unavailable'}
What makes this a defect rather than a missing nicety is that this PR establishes the vocabulary and its own suite pins it. tests/test_dashboard_signed_rail_honours_the_mesh_kill_switch.py:120 asserts, for a rail that really is broken:
assert out == {"signed": False, "error": "safety mesh unavailable"}So "safety mesh unavailable" is now the graded meaning of a fault, and line 1066 emits exactly that string in the one state the same test file spends 30 lines proving is not a fault. The two sites are 38 lines apart and "safety mesh unavailable" occurs twice in the tree — once guarded (1028), once not (1066).
The cost is the one the comment at 1053-1057 names for the sibling: docs/dashboard/troubleshooting.md:179-184 ("A resume after e-stop is refused") documents exactly two causes, a missing override_code and a per-peer STRANDS_MESH_OVERRIDE_CODE mismatch. An operator who set the switch, POSTs the documented /api/safety/resume and reads "unavailable" is sent to those two faults and to a code that is fine, rather than to the switch they set. signed_resume is public on this module and that route is documented on main, so the state is reachable without the server slice.
Why nothing caught it: the AST guard is structurally blind here, because it keys on Mesh construction and this method constructs nothing.
| method | constructs Mesh | asks mesh_disabled_by_env |
graded by the guard |
|---|---|---|---|
_safety_mesh |
yes | yes | yes |
signed_estop |
no | yes | no |
signed_resume |
no | no | no |
And behaviourally, grep -rn signed_resume tests/ is empty on this branch: no cell touches the method at all.
The shape I would suggest, since the wording argument in this module is consistently "one owner": the ternary at 1023-1029 already is the rule, so lift it to one small helper that both methods call, rather than a second copy of the string in a second place. Plus a cell for signed_resume under the switch. Optional but cheap while you are there: widen the guard from "every site that constructs a Mesh asks the predicate" to "every rail-unavailable answer asks it", which is the invariant that actually failed.
What is right, and graded
Finding 7. The hardcoded lockout_engaged: True is a true statement, not a convenience: Mesh.emergency_stop sets self._estop_lockout.set() at core.py:3695 unconditionally, three lines before broadcast at 3698, so the issuer's latch is engaged on every path including the one where nobody answers. Keeping it True also keeps the resume control reachable, which "let it say False" would have broken. The peer half is delegated rather than re-derived: peers_not_stopped calls mesh.core._peers_that_did_not_stop, and responses_received is len(responses), the same expression core puts in its own envelope at 3720 -- so the two numbers really are comparable, as claimed. The equivalence cell at test_dashboard_signed_estop_reports_what_the_peers_said.py:136 asserts the bridge's verdict against that function's for the same replies, which is the assertion that fails if a second copy of the grading ever appears.
Finding 8. The predicate is asked before construction, not after, which is the distinction that matters since constructing is what joins the fleet -- and RecordingMesh records construction separately from start() precisely so a guard placed one line too late cannot pass. Parametrizing over NEGATIVE instead of retyping the spellings is right for the reason that module's note gives.
Gate
| check | result |
|---|---|
| the 4 slice/sweep test files | 148 passed |
ruff check (module + 2 new suites) |
clean |
ruff format --check (module) |
already formatted |
lockout_engaged: True vs core.py:3695 |
verified unconditional, pre-broadcast |
_peers_that_did_not_stop single owner |
verified, bridge is the 2nd caller |
CI rollup on 7fb449a7 |
SUCCESS |
Scope of this pass: the two claimed findings, their pins, the kill-switch guard and the safety rail's two exported methods. Not a line-by-line pass over all 1,268 lines of the extracted module -- the extraction fidelity against #2848 is not something reading this diff can establish, and I did not try.
One process note for the record, no action: the head commit was pushed by an account other than the PR author, so require_last_push_approval will want an approval from someone who is not that pusher once the finding is addressed.
… the rule (#3248) check_merge_blockers.py derives the party who owes the next action from the outcome alone. For every outcome but one that is enough. An absent approval is owed by "any reviewer" only where any reviewer's approval would count, and under require_last_push_approval the pusher's would not -- a second fact the evaluator has already resolved and prints in its own table one row above the party it contradicts. Measured live on strands-labs/robots, 2026-09-06: #3205 and #3212 both read missing-approval / "any reviewer" with "head pushed by cagataycali" in the same report. That account's approval cannot satisfy the count, so the review round the report invites arrives at pusher-only-approval, which needs somebody else regardless -- the state #2907 has been sitting in since it was approved by that same account on 2026-09-04. Blocker carries an explicit party, set where the rule discounts the pusher. The outcome is deliberately unchanged: it also decides gating, finding and exit status, and an unreviewed pull request is still the ordinary state, so it must not begin gating or reporting red merely because the eligible set is narrower. Four existing pins asserted "any reviewer" on a fixture whose branch carries the rule and whose head has a pusher; each is retargeted with its original point intact (#2480 still asserts the party is a person rather than NOBODY). Co-authored-by: strands-robots <strands-robots@users.noreply.github.com>
…witch signed_estop distinguishes "you switched this off" from "this is broken"; signed_resume is the same rail and the same operator and did not. With STRANDS_MESH set to any kill spelling it answered "safety mesh unavailable", which is the wording this rail's own suite pins as meaning a fault, and docs/dashboard/troubleshooting.md documents only override_code causes for a refused resume - so an operator who set the switch was sent to a code that is fine rather than to the switch they set. Both verbs now answer through one owner, _rail_unavailable(), so the two wordings have a single spelling. The rail's guard is widened with it: keying on Mesh(...) construction grades "did we open a session" and is structurally blind to a verb that constructs nothing, which is how the resume half drifted. Every answer that reports the rail unavailable is now graded for asking the kill switch, and signed_resume gains the behavioural cells it had none of.
Iteration
|
| corner | tests | production | result |
|---|---|---|---|
| A | pre | pre | 23 passed |
| B | new | pre | 10 failed, 27 passed |
| C | new | new | 37 passed |
| D | pre | new | 23 passed |
23 + 14 = 37, and the 27 passing in B are the 23 existing cells plus 4 controls that hold on both trees. D is the one that matters for regression risk: no pre-existing cell moved.
The 10 failures in B are the 9 kill spellings plus the widened guard, which names the method:
these answer that the rail is unavailable without asking
mesh_disabled_by_env(): ['signed_resume']
Break table (5 mutations, collected stable at 37)
| mutation | fires | cells |
|---|---|---|
| b0 control | 0 | - |
| b1 resume re-inlines the bare string (the defect) | 10 | resume kill rows + guard |
| b2 the owner stops asking the predicate | 19 | both verbs' kill rows + guard |
| b3 over-reach: every failure called the switch | 3 | both broken-rail controls + guard |
| b4 the estop half regresses to an ungated copy | 10 | estop rows + guard |
| b5 a third ungated rail answer, no cell of its own | 1 | guard alone |
b3 is why the broken-rail control exists - it fires if "point at the switch" is bought by describing every failure as the switch. b5 is why the widened guard earns its lines: it is the only cell that can see a future third answer, and the construction-site guard cannot, because such a verb constructs nothing.
Gate
| check | result |
|---|---|
ruff check strands_robots tests tests_integ |
All checks passed |
ruff format --check (same scope) |
1875 files already formatted |
mypy strands_robots tests tests_integ |
Success: no issues found in 1872 source files |
| the 4 slice/sweep suites | 148 -> 162 passed (+14 = the new cells) |
tests/mesh/ + all tests/test_dashboard*.py |
5389 passed, 10 skipped |
| docs + changelog + markdown-link graders | 336 passed |
scripts/check_whole_tree_graders.py |
4331 passed; 7 pre-existing host failures |
The 7 are environmental and unrelated: 5 assert rclpy is absent where this host supplies it from /opt/ros/jazzy, and 2 read installed websockets 16.0 against the >=17.0 floor declared for cosmos3-service. None reference mesh_bridge or the kill-switch suite.
Cost
+3 executable statements in production (678 -> 681 by AST); the remainder of +26/-15 is the owner's docstring. Docs +7. Tests +115/-5.
docs/dashboard/troubleshooting.md now names the switch under "A resume after e-stop is refused", which was the operator cost identified in the review - the sheet listed only the two override_code causes, neither of which is the switch.
Pushed as a fast-forward, so the original commits are preserved as parents. The last-push identity is unchanged by this iteration, so the approval note in the review still applies.
|
CI is green on State: I am deliberately not clearing that myself: I raised the finding and wrote the fix, so approving it would make the review circular. Handing it to a second pair of eyes. |
cagataycali
left a comment
There was a problem hiding this comment.
Approving. This clears the CHANGES_REQUESTED from the previous round, whose single MUST FIX finding is resolved at cf90f8da and graded below rather than taken on faith.
Measured on the branch tip (check_checkout_is_pr_head.py --pr 3205 reports checkout, branch tip and recorded headRefOid all cf90f8da, so nothing here is derived from a stale tree).
The finding is resolved, and in the stronger of the two shapes offered
The suggestion was to lift the ternary into one owner rather than add a second copy of the wording. That is what landed:
| claim | measured |
|---|---|
"safety mesh unavailable" occurrences in strands_robots/ |
1, at mesh_bridge.py:1033, inside _rail_unavailable |
| rail verbs answering through it | 2, at :1041 (signed_estop) and :1077 (signed_resume) |
signed_resume cells in tests/ |
5, where grep -rn signed_resume tests/ was empty last round |
The over-reach case is the one worth stating: pointing at the switch was not bought by calling every failure the switch. _rail_unavailable still emits the fault wording when the switch is clear and the rail genuinely will not start, and test_a_broken_rail_still_reports_unavailable_on_the_resume_path pins that direction on the path that was wrong.
The widened guard fails on pre-fix code
This is the part that makes it a pin rather than a restatement. Running test_every_rail_unavailable_answer_asks_the_kill_switch's own predicate against both revisions of the module:
PRE-FIX (cf90f8da^) emitters={'signed_estop': True, 'signed_resume': False}
guard: FAILS -> ['signed_resume']
POST-FIX (cf90f8da) emitters={'_rail_unavailable': True}
guard: passes
So the pre-fix population contains the offender and the guard names it. The construction-site guard is correctly kept beside it rather than replaced: it keys on Mesh(...), which is the right key for "did we open a session" and is structurally blind to a method that constructs nothing, and the two are not each other's substitute.
docs/dashboard/troubleshooting.md gains the switch as a third cause under "A resume after e-stop is refused", which was the operator cost named last round: the sheet listed only the two override_code causes, so an operator who set the switch was sent to a code that was fine.
Gate
| check | result |
|---|---|
call-test-lint / Test and Lint on cf90f8da |
SUCCESS |
| required set (the one required context) | satisfied |
| review threads | 1, resolved |
check_thread_is_answered.py --pr 3205 |
nothing-owed |
check_merge_base_overlap.py --all-open |
#3205 absent from the base-moved rows; behind_by is not a concern here |
| round-2 commit shape | 4 files, +166/-20, one concern |
Scope: the finding I raised, its pin, the widened guard's population, and the two exported rail verbs. Not a line-by-line pass over the extracted module -- extraction fidelity against #2848 is not something reading this diff can establish, and I did not try. Earlier rounds cover the rest.
What this approval does not do, stated so the state is not misread
cf90f8da was pushed by cagataycali, which is also this approval's account, so under require_last_push_approval this approval does not count and the pull request stays BLOCKED. check_last_push_approval.py --pr 3205 reads awaiting-first-review. It needs one approval from an account that is not cagataycali; no further review from this one supplies it, and the remedy is a reviewer rather than a push.
That state is the recurring one AGENTS.md records for #1035 and #1722 and is currently true of all three open pull requests here, each authored by a contributor with a head pushed by this account. It is the already-tracked class rather than anything new about this branch, so no action on this pull request -- flagging it only because BLOCKED here is indistinguishable from reviewer latency in every field a status sweep reads, and the two need opposite actions.
One optional observation, explicitly not worth a round and not worth a push on this branch: the new guard walks ast.FunctionDef only, and the module already carries one AsyncFunctionDef. Counting async definitions changes the verdict on neither revision today ({'_rail_unavailable': True} either way), so this is a latent blind spot for a future async rail verb, not a defect. Best folded into whichever later slice touches this file.
… account that made it (#3268) * fix(scripts): a standing request for changes is a blocker owed by the account that made it `check_merge_blockers.py` modelled only the approval side of the review decision, so a pull request sitting at `CHANGES_REQUESTED` was reported as `missing-approval` owed by "a reviewer other than the pusher". That party cannot clear it. With required reviews in force a standing request for changes holds the merge until its own author approves or dismisses it, so another account's approval satisfies `required_approving_review_count` and the pull request stays `BLOCKED` -- the #1905 presentation reached from the review-decision side rather than the last-push side. Measured on #3205: `CHANGES_REQUESTED` stood 15h44m with its one review thread resolved, `call-test-lint` SUCCESS, and `check_thread_is_answered.py` reading `nothing-owed`. 12h51m of that was after the fix had landed. Thread resolution and review decision are separate objects, and the requester's own follow-up reply is a `COMMENTED` review, which expresses no position and supersedes nothing -- so every sweep the repository has read clean while the decision went on blocking. The new `changes-requested` outcome is reported ahead of the approval rules it is not answerable by, names the holding accounts in its detail rather than only the role, and is scoped to a ruleset that actually requires reviews. It is deliberately not a finding: it is owed by a reviewer, and a finding that fires whenever a review is in progress fires on the ordinary state. Standing is resolved by the sibling check rather than re-derived, so the rule that a `COMMENTED` review retracts nothing has one owner across both questions. Pinned by 13 cells that fail on pre-fix code, 7 of them over the sweep's verdict and 6 over the shared resolver. * docs(changelog): fragment for the changes-requested merge blocker --------- Co-authored-by: cagataycali <cagataycali@users.noreply.github.com>
|
This is blocked by Every required check is green and the one review thread is resolved. The gate is that I pushed the head commit
This presents as Two ways out, either is fine:
No behaviour change requested. Separately, because
It also merges cleanly, with one auto-merge in |
|
Correction, state has moved since this note: MERGEABLE at @yinsong1986 a second approving account is the only gate left, and auto-merge is armed, so this lands on your approval. |
`docs/dashboard/troubleshooting.md` no longer exists. All six `docs/dashboard/` pages were removed in strands-labs#3472: they opened with a `python -m strands_robots dashboard` command `__main__` does not dispatch, and sat outside the MkDocs nav with no inbound link from any page. This branch added 27 lines to that page, which is the modify/delete conflict against main. The deletion is taken rather than the page restored. strands-labs#3472 named the home for what only lived there - the environment-variable reference - and the knobs this branch introduces (`STRANDS_DASHBOARD_PEER_TTL_S` and the seven `_*_HZ` coalescer ceilings) go there instead, so `test_env_vars_the_package_reads_are_documented` has a page to find them on. Two docstrings cited the page by path; both now state the fact directly, since a reader cannot open the page to check it. `mesh_bridge`'s public members gain the summary lines `ruff`'s pydocstyle rules require on `strands_robots/` - enforced on main and not when this branch diverged, so 13 D102/D103 were waiting on the merge.
There was a problem hiding this comment.
Conflict resolved. docs/dashboard/troubleshooting.md no longer exists - #3472 removed all six docs/dashboard/ pages because they opened with a python -m strands_robots dashboard command __main__ does not dispatch. The deletion is taken rather than the page restored, and the two docstrings that cited it by path now state the fact directly. CONFLICTING -> MERGEABLE.
Merging main locally surfaced two blockers the conflict was hiding, both measured on that merge:
| gate | before | after |
|---|---|---|
test_env_vars_the_package_reads_are_documented |
1 failed - 8 variables "appear in no page under README.md or docs/" | 22 passed |
ruff check strands_robots tests tests_integ |
13 D102/D103 in mesh_bridge.py |
clean, 2018 files |
The lint half is pushed: pydocstyle is enforced on strands_robots/ on main and was not when this branch diverged 352 commits ago, so every public member added here now carries the summary line its neighbours have, written from each body.
The documentation half needs the merged tree - docs/reference/configuration.md postdates this branch point, so it cannot be edited from here (and a PUT .../update-branch is refused: the merge carries main's workflow files). After Update branch, these two rows beside STRANDS_MESH_SUBSCRIBE_ALLOW are what turns that gate green:
| `STRANDS_DASHBOARD_PEER_TTL_S` | How long a peer may stay quiet before `dashboard.mesh_bridge.prune_peers` drops it from the fleet snapshot entirely (it greys out as `stale` after 15s regardless, and a peer with a live managed process is never dropped). Non-positive means never prune. A value no age comparison can be built from -- unparsable, or non-finite like `inf`/`nan` -- falls back to the default with a warning naming the variable, because both read as "no bound" and would keep a departed peer on the fleet view forever | `300` |
| `STRANDS_DASHBOARD_PRESENCE_HZ`, `_CAMERA_META_HZ`, `_POSE_HZ`, `_IMU_HZ`, `_ODOM_HZ`, `_LIDAR_HZ`, `_HEALTH_HZ` | Per-event-type ceiling on how often the dashboard mesh bridge forwards an UNCHANGED repeat of that topic; a changed reading is never delayed, so a moving robot still streams. Non-positive means no ceiling. Same fallback as `_PEER_TTL_S` -- an unusable value keeps the default and says so | per type: `1.0`/`2.0`/`1.0`/`1.0`/`1.0`/`1.0`/`0.5` |With those rows in place the merge is green end to end: 184 passed across the four dashboard/env suites, 932 across the whole-tree grader roster, 2363 across the docs/dashboard/changelog slice, mkdocs build --strict clean, mypy at the examples/isaac_gs baseline, overlap check "No overlap".
One reviewer note on the sweep this PR extends: reinstating a module-level bare float() for PEER_TTL_S fails it by name (dashboard/mesh_bridge.py::<module>), so the new module::<module> population has teeth on real source and not only on planted trees.
The one red check, and the combination it asks for
Detect an untested overlap with the base branch fails on one path: tests/test_env_float_knobs_resolve_to_a_finite_value.py, changed on both sides since 92e5e14a. Its remedy is Update branch, which is also what the two documentation rows above need, so one action clears both.
The check exists to prompt someone to compile the combination first. Compiled: the two sides auto-merge with no conflict and compose rather than collide - main emptied NOT_A_RESOLVER_DOMAIN (the policies/vera exemption went with #3389) while this branch adds the module::<module> population, and the merged file is 15 passed. So the update is a base advance, not a rebase to negotiate.
tests/test_env_vars_the_package_reads_are_documented.py, on main since strands-labs#3547's neighbour landed, refuses an environment variable the package reads that no page under README.md or docs/ names. mesh_bridge.py reads STRANDS_DASHBOARD_PEER_TTL_S and the seven per-type coalescing rates, none of which had a row. Each row states what the knob selects, its default, and the domain the resolver applies (finite number; a non-positive rate is read by the consumer as no ceiling).
…ed renderer tests/test_refusal_messages_never_raise.py grades every guard in the package for a caller value interpolated straight into its refusal text, because rendering a value can raise (strands-labs#1873) and a refusal that raises while it is being built never reaches the caller. The shape a peer publishes in its camera_meta is such a value. refusal_repr defers to repr wherever repr succeeds, so the message is unchanged for every shape that could already be rendered.
cagataycali
left a comment
There was a problem hiding this comment.
Approving at d1be3a3a. The previous approval was dismissed by the push that answered the round-3 note, so this re-reads what that push added rather than restating earlier rounds.
Three commits since f9da7fea, measured on the branch tip (git fetch Vivek0712/robots feat/dashboard-mesh-bridge -> d1be3a3a, equal to headRefOid, so nothing here is derived from a stale tree):
| commit | what it is | checked |
|---|---|---|
5074b5d7 |
base merge of main |
the two blockers the merge exposed last round are the next two rows; nothing else in the PR's own diff moved |
2f836914 |
docs/reference/configuration.md +8 rows |
the eight STRANDS_DASHBOARD_* knobs the package reads, one row each, defaults matching the resolver (300, 1.0/2.0/1.0/1.0/1.0/1.0/0.5) |
d1be3a3a |
mesh_bridge.py 2 lines |
{shape!r} -> refusal_repr(shape) in _raw_to_jpeg, so a peer-published shape whose repr raises cannot take the refusal down with it; refusal_repr defers to repr where it succeeds, so every message that already rendered is byte-identical |
That last one is a whole-tree grader (tests/test_refusal_messages_never_raise.py) catching the branch on the merged tree, which is the class the round-3 note said the merge would surface. Correct fix, minimal shape.
Gate at d1be3a3a
| check | result |
|---|---|
call-test-lint / Test and Lint |
SUCCESS |
Detect an untested overlap with the base branch |
SUCCESS (was FAILURE at f9da7fea; the base merge cleared it) |
Report the last-push-approval state |
SUCCESS |
| review threads | 1, resolved |
check_thread_is_answered.py --pr 3205 |
nothing-owed |
| auto-merge | armed, SQUASH |
What this approval does not do
d1be3a3a is attributed to this account as pusher, so under require_last_push_approval this approval does not count and the pull request stays BLOCKED (check_last_push_approval.py reads it as pusher-only-approval once this lands). It needs one approval from a second account; no further review from this one supplies it. Same class as #3427, #3496, #3515 and #3577 in the current queue, all armed for auto-merge on that approval. No action is asked of the author.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
This PR lands strands_robots/dashboard/mesh_bridge.py as the first slice of the #2848 decomposition per #2977: the dashboard's mesh peer (fleet snapshot with TTL pruning, per-type event coalescing, responder-scoped RPC mirroring Mesh.send, and the signed safety rail), with the two mesh-safety findings from the draft's review resolved on arrival. I verified the slice against the head tree rather than taking the description on faith: every cross-module symbol the bridge imports exists at d1be3a3a (mesh_disabled_by_env, _peers_that_did_not_stop, cmd_bytes_cap, hz_from_env/CAMERA_HZ, finite_number_error/refusal_repr, _mesh_switch.NEGATIVE, settings.apply_mesh_env, security.validate_command, the Mesh.alive property); _rail_unavailable() gives the switched-off-vs-broken wording a single owner shared by both rail verbs, matching the resolution of the round-1 resume finding; send_cmd's NUL rejection also covers the BROADCAST_RESPONDER sentinel (which contains a NUL byte), so the response-hijack surface Mesh.send guards is closed in the clone as well; _resolve_mesh_camera_hz mirrors Mesh._resolve_camera_hz exactly and the parity test uses the publisher's own method as the oracle; and the lock nesting (_peers_lock -> _coalesce_lock in snapshot()) has no reverse-order path, so no inversion. All required checks are green on the head commit.
What's good
- The e-stop grading is imported from
mesh.core._peers_that_did_not_stoprather than copied, with a test pinning that the bridge's verdict equals mesh.core's for the same replies. - Both AST guards (every
Mesh(...)construction site must askmesh_disabled_by_env(), and every emitter of the fault wording must too) close the structural blind spot that let the resume half drift in the first round. - The env-float sweep extension to module-scope statements comes with planted-file tests for the scope-crediting edge cases (a guard inside an enclosed
defdoes not vouch for its host). - The eight new
STRANDS_DASHBOARD_*knobs are documented indocs/reference/configuration.mdin the same PR, per the AGENTS.md env-var rule.
What
The first decomposition slice of draft #2848, per #2977's own seam table:
strands_robots/dashboard/mesh_bridge.py— the dashboard's mesh peer (fleet snapshot, event coalescing, responder-scoped RPC, signed safety rail) — extracted whole from the draft and landed with the two must-fix findings that belong to this slice resolved. The module is @logesh4v's work from #2848 (including their own fixes for both findings on the PR branch, commits 59fc905 / 0173d5d); this PR is extraction + adaptation to currentmain, filed so the slice can be reviewed to a conclusion at reviewable size (+1,539 across 6 files vs the draft's 476).Finding 7 — an e-stop that reached nobody must not report like one that reached everyone
lockout_engageddeliberately staysTrue—Mesh.emergency_stopsets the local lockout unconditionally before broadcasting, so "let it say False" would be a second false statement and would hide the resume control the UI gates on. Instead the payload now carries the peer half alongside:responses_received(replies, the #1680 meaning, so numbers stay comparable) andpeers_not_stopped, graded by importingmesh.core._peers_that_did_not_stop— the single owner of that rule — so only affirmative failure reports flag a peer. A test pins that the bridge's verdict equals mesh.core's for the same replies, and that all-acked / nobody-answered / answered-but-refused fleets render three distinct payloads.Finding 8 —
STRANDS_MESH=falsemust keep the safety rail off the fleet_safety_mesh()now asksmesh_disabled_by_env()— main's sole kill-switch predicate, resolving through_mesh_switch's single-owner vocabulary, no env re-parse — before constructingMesh, since constructing is what joins the fleet. The refusal names the switch ("signed safety rail disabled by STRANDS_MESH=false") while a genuinely broken rail still reads "unavailable". An AST guard test fails any futureMeshconstruction site in this file that skips the predicate; the kill-switch test parametrizes over_mesh_switch.NEGATIVEdirectly.Adaptations from the draft
mesh_kill_switch_engagedpredicate does not exist on main — all three call sites route throughmesh_disabled_by_envwith comments citing that predicate's "every path that can open one answers this" doctrine and fix(mesh): honour the STRANDS_MESH kill switch at the robot-less gateway too #2515.joint_silence.merge→ plain overlay with a documented why (that module and its only feeder land with the server slice).__init__.pyrequire_optionalsgate, matching fragment 3051's documented decision.Out of scope (next slices, per #2977's own decomposition)
The frontend half of finding 7 (EstopSheet.tsx / estopReach.ts) and the
/api/safety/estoproute pin (needdashboard.server); finding 4 (degraded_reportin mesh/core.py); findings 1–3, 5–6.Tests
34/34 slice tests; 156/156 including main's kill-switch, vocabulary-single-owner, session-acquire, e-stop-grading and safety-state suites; 128/128 repo guard tests (changelog fragments, no-host-paths, docstring xrefs).
ruff check,ruff format --check,mypyclean on touched files. Threechangelog.d/fragments;CHANGELOG.mduntouched.Closes nothing on its own — advances #2977 (slice 1 of the plan), addresses findings 7+8 of #2848.
Round changelog
Round 5 (head
d1be3a3a) — the required check was red on twomaingraders that landed after this branch's merge base, not on anything the review asked for.5074b5d7mergesorigin/main(359 commits behind;git merge-treereported zero conflicts,git show --ccis 0 lines, so the PR's own diff is byte-identical). This is what theDetect an untested overlap with the base branchcheck was asking for.2f836914documents the eightSTRANDS_DASHBOARD_*knobsmesh_bridge.pyreads indocs/reference/configuration.md, whichtests/test_env_vars_the_package_reads_are_documented.py(now onmain) refuses to leave unnamed. That was the one failing test incall-test-lintatf9da7fea(1 failed, 36829 passed).d1be3a3aroutes_raw_to_jpeg's refusedshapethroughrefusal_repr, the shared renderertests/test_refusal_messages_never_raise.pyrequires of every guard in the package. Message text is unchanged for any shapereprcould already render.Verification: the five slice/grader suites pass (274 tests), and the whole-tree roster was run on this composition and on bare
mainin the same environment; the only delta was the twotest_refusal_messages_never_raisecases thatd1be3a3aclears. The remaining failures are identical on both and are environment-bound (nomujoco,lerobot,torchon this host).🤖 Generated with Claude Code
https://claude.ai/code/session_01Jmv1uhoHXZxuP3GKhfJuq2