Skip to content

process: the dashboard draft #2848 is 476 files in one diff, so its 19 open findings cannot be reviewed to a conclusion - decompose before the next round #2977

Description

@cagataycali

What

#2848 (feat(dashboard): operator web dashboard for the robot fleet) is a draft carrying +80,304 / -3,499 across 476 files in 21 commits, currently CONFLICTING / DIRTY, with 19 of 20 review threads unresolved.

This issue is not about any one of those findings. It is about the shape: the review it has already received is thorough and correct, and it still cannot converge, because every fix has to be landed into a diff too large to re-review and too broad to keep conflict-free. Filing it so the decision survives outside a draft's comment thread.

Why another review round is the wrong next move

The open findings are not variations on one concern. They are eight independent numbered must-fixes plus three stale-documentation copies, spanning six subsystems:

# subsystem concern
1 pyproject.toml four dashboard deps declared only in a hatch dev env, so the wheel ships strands_robots/dashboard/ with unsatisfiable top-level imports; no dashboard extra among the 32 declared
2 dashboard/server.py, dashboard/auth.py both enrollment endpoints are in PUBLIC_PATHS, so the loopback rule never applies; with --host defaulting to 0.0.0.0, first-come wins ownership of the fleet on a fresh install
3 tests/test_dashboard_calibration.py a host-shaped path literal fails test_no_host_paths.py, which is one live cause of the red required check
4 mesh/core.py degraded_report overwrites the snapshot["degraded"] block written from _degraded_probes(), and reads a since key no record in the tree carries, so the shipped for_seconds is always 0.0
5 mesh/security.py a per-joint input envelope with no call sites - already tracked independently as #2935
6 dashboard/server.py, dashboard/device_manager.py two py/path-injection highs with no containment check; existence is validated, containment is not
7 dashboard/mesh_bridge.py lockout_engaged is a literal True, never derived from responses, so an e-stop that reached nobody reports identically to one that reached every peer
8 dashboard/mesh_bridge.py the safety-gateway Mesh is constructed without the mesh_kill_switch_engaged() check start() performs, so STRANDS_MESH=false still puts a *-safety peer on the live fleet

Two of these keep the one required check (call-test-lint / Test and Lint) red on their own (#3, and the [dashboard] extra named by an install hint that [project.optional-dependencies] does not declare). So the draft cannot go green in its present shape, which means every future reviewer pays the same 476-file read to reach the same conclusions.

All of the above is confined to code this branch introduces. Measured on main at c9da51a, input_envelope_for_units, NORM_MODE_UNITS, INPUT_ENVELOPES_BY_UNIT, degraded_report and _read_state_hw_seen each have zero occurrences - so none of it is an extractable pre-existing defect, and none of it is fixable anywhere other than in this work.

Proposed decomposition

The subsystem column above is already the seam. Each row below is independently reviewable, independently testable, and independently mergeable, and the ordering is by what unblocks the rest rather than by size:

  1. Packaging. Declare the dashboard extra, fold it into all, route the imports through require_optional() so the refusal names the extra, and align the three documented install commands. Clears finding 1 and one of the two red-check causes.
  2. The two mesh defects, against main and without any dashboard code. Findings 4, 7 and 8 are mesh behaviour, and 4 and 8 are pinnable with regression tests that fail on pre-fix code: a degraded probe that has been failing must report for_seconds > 0, and the kill switch must be honoured at the second construction site per the existing one-predicate rule. This is the highest-value slice and the one least coupled to the dashboard.
  3. Auth and path containment. Findings 2 and 6 together, because seizing enrollment is what turns an authenticated arbitrary-directory read into a remote one - they share a threat model and should be reviewed as one change.
  4. The dashboard surface itself, once 1-3 have landed, with the stale documentation (three copies of a banner asserting that /api/record does not exist, when record_api.py mounts it) corrected in the same change that makes it true.

Step 2 is worth pulling out first regardless of what happens to the rest: those are live mesh-safety defects whose fixes do not depend on the dashboard existing, and today they are trapped behind a 476-file diff.

Not proposed

Closing the draft. There are 21 commits of real work here and the review on it is high quality; the finding is that the container is wrong, not the contents. Leaving it as a draft reference while the slices land against main keeps both.

Refs #2848, #2935.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions