feat(tailnet): kirocrew tailnet up — actually publish the dashboard - #2109
Conversation
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound feature, but the PR body and Watch
Suggestions
[DESIGN-REVIEWED] 01244a5 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/cli_commands.py:1876 -- False positive or not applicable? A repository writer can comment: |
e4ea192 to
2e4caa3
Compare
|
Reviewed SHA All five items were legitimate. Two of them were mine getting a property wrong, not BLOCKING An undetermined state refuses too, deliberately. This code has never seen a BLOCKING That helper already carries the non-str guard ( FINDING FINDING Tests: +10 (38 in Gates on |
2e4caa3 to
5383df9
Compare
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThis is a careful, well-reasoned PR. Let me verify the key correctness concern — the port-resolution flow in No reachable defect survives falsification on the changed lines. No findings. [OPUS-REVIEWED] 01244a5 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
5383df9 to
b8d2f7a
Compare
|
Reviewed SHA BLOCKING
BLOCKING
End-to-end coverage (new)
That covers a class the mocked suites structurally cannot:
The argv contract is now verified against upstream source, not assumed: Still not verified: Tailscale's own runtime behaviour and the real shape of Gates on |
b8d2f7a to
ddd1044
Compare
|
Reviewed SHA BLOCKING The finding is right, and reading upstream turned up a better fix than verifying Withdrawal now passes Ownership is narrowed to match what withdrawal actually removes: BLOCKING The race is real: Rather than change what every
One correction, and a pre-existing issue found while testing itA first attempt at these tests asserted that the file gains no default-filled Gates on |
ddd1044 to
cb816d8
Compare
The Opus lane had a 2.7% blocking rate against GPT's 18% across 73 PRs, and 63 of 73 posted comments contained literally "No findings." A controlled experiment on this repo found the cause is not the model and not the architecture -- it is three clauses in the prompt, each independently sufficient to silence a defect the same model reports 3/3 times without them: the closed-list reading of the residual defect classes, the certainty threshold, and "drop the finding if the fix touches untouched code". Splitting the call while keeping those clauses in the first half did NOT help: the discovery pass produced zero candidates, so the filter had nothing to keep. Precision enforcement has to sit downstream of discovery, which is also what Anthropic's own code-review plugin does (parallel discovery agents, then a per-candidate validation agent with a confidence floor). What changes: * Stage 1 (discovery) generates candidates with generous recall and no precision gates. Its output is never posted and gates nothing. * Stage 2 (validation) is an independent call that re-derives input / call path / observable outcome for every candidate from code it opens itself, keeps only those it scores >= 80, and only then applies the closed blocking list. It keeps `id: review`, so the existing transcript capture, comment upsert and fail-closed gate are unchanged. * The prompts move out of the YAML into .github/review-prompts/ and are shared by the same-repo and fork lanes, which previously carried near-duplicate copies. They are materialised from the BASE commit, so a PR can neither weaken the rules that govern it nor rewrite the prompt that reviews it; a missing prompt fails the job rather than degrading into an unspecified review that could look clean. * Candidates cross the stage boundary as a workspace FILE, never string interpolation -- model output must not reach YAML or a shell argument, and a file has no arg-length ceiling. * The fix-scope rule changes behaviour instead of being deleted: a finding whose only remedy lies outside the changed lines is reported as advisory rather than dropped, because the author cannot land the remedy here but the signal is still real. A regression the diff itself introduces still blocks, since reverting the hunk is an in-diff fix. Job name, required check, tool surface and the marker contract are unchanged. The fork lane keeps its no-shell posture (its diff is pre-fetched from GitHub's compare endpoint). Measured locally on the same corpus, running these exact prompt files: * 8 negative-control runs (PRs where BOTH production lanes reviewed the same SHA and both emitted zero findings): 33 candidates in, 0 BLOCKING out, 7 advisory out. The validator removed 79%. No run would have turned the required check red on a PR two reviewers passed. * Positive controls (a GPT blocking the old Opus lane missed): recovered on #2109 (blocking, same guard GPT named, 2/2) and reported on #2169 (advisory, 4/4, where the old lane said nothing). #2152's GPT finding was generated by discovery but dropped by the validator. Three tests that asserted on prompt text follow it to its new home rather than being deleted, and TestOpusTwoStageArchitecture locks the split in place -- including a cross-check that the marker the gate greps equals the marker the validation prompt is told to emit, because a typo either side fails every PR closed and silently. Known and deliberately not addressed here: the two stages are sequential, so end-to-end latency roughly doubles under the same 90-minute runaway backstop. That number can only be measured on a real CI run. Stacked on the prompt-file bootstrap PR. Because the extraction reads the base commit, the prompts have to exist on main before this can run -- and for the same reason, a future PR that EDITS a prompt is still reviewed by the OLD one. The workflow header documents that.
|
Rebased onto current main ( What main changed under this PR, and what it required here. Phase 3
The E2E fixture therefore patches Round-6 fixes now in the tree (previously unpushed)Undetermined serve state no longer permits destructive replacement. The malformed-config window in this command is closed. Two tests changed rather than being preserved, because the contract changed: the one Gates on |
276d11d to
314c65d
Compare
|
Reviewed SHA BLOCKING The finding is correct: the caller-side fingerprint re-check leaves a residual The lock had to go inside
This closes #2147 for Tests
Regression surfaceBecause this changes a function 29 call sites share, the config-adjacent suites were |
314c65d to
d892b30
Compare
|
Reviewed SHA BLOCKING — Both reviewers are right, and this one was self-inflicted in the sharpest possible The wait is now taken only off the loop: BLOCKING — Correct, and it closes a hole in my own earlier reasoning. I had guarded only All three actions now validate the raw file first. Between rewriting the operator's FINDING (Opus) — Windows shard 1 — MY TEST was wrong, not the product — FIXED. Gates on |
d892b30 to
43edbb3
Compare
|
Reviewed SHA BLOCKING — This one lands squarely on my own argument from two rounds ago. I made the lock The lock now yields three states instead of a boolean, because "did not acquire"
Refusing on BLOCKING — Correct, and the consequence is worse than the 502 I had been reasoning about. So this command resolves evidence before intent: Three tests: the marker outranks the configured URL, an explicit env port outranks the Tests changed rather than preservedFour assertions in Regression surface10681 passed, isort, flake8, mypy (863 files), docs-lint. This widens the failure Windows shard 4 is not this branch. |
|
Reviewed SHA BLOCKING — Correct, and it names the gap the marker-first change left: when discovery cannot
BLOCKING —
Brand Name Gate — FIXED. One prose Windows shard 1 — one regression of mine, one bad test design
The two event-loop tests failed on Windows for a better reason: they simulated One test was added while rewriting them, because the split it verifies was otherwise Gates on |
|
Reviewed SHA Why the scope narrowed insteadBoth findings this round were about the config-locking primitive I added, not the
The deciding evidence arrived this round: the That is two review findings in genuine tension, resolvable only by a judgement about So What this PR still guaranteesEverything tailnet-specific is unchanged and still covered:
Also rebased onto Gates on |
|
Reviewed SHA 1. Unverified port fallback could expose an unrelated service — FIXEDCorrect, and the sharper of the three.
The test that previously asserted the fallback published now asserts it refuses: 2. Status could overwrite malformed configuration — FIXEDCorrect: the guard only covered The guarded set is now derived from 3. Concurrent config updates silently lost — FIXED, and this time in the right placeYour suggested fix was "reject an intervening config change", and that is now done —
The fingerprint is SHA-256 of the bytes ( Against a non-participating writer this detects and refuses rather than clobbering. VerificationGates on On the full suite this host is noisy, so I compared failure sets against a pristine New tests: |
|
Reviewed SHA FIXED —
|
| round | what was tried | outcome |
|---|---|---|
| 1 | caller-side fingerprint check | flagged: check→rename window |
| 2 | lock + CAS inside write_config_atomically |
flagged: blocking sleep on the gateway event loop |
| 3 | tri-state lock | flagged: CAS proceeding unlocked |
| 4 | reverted the primitive; plain write | flagged: lost update returns |
| 5 (now) | lock + digest re-check local to this command | flagged again: window between re-check and rename |
The current code takes a sidecar lock, hashes the file's bytes before the read, and
re-checks that digest immediately before the write. Against another kirocrew tailnet
that is exclusion. Against the dashboard, which does not take this lock, a residual
window between the re-check and the rename remains, and no caller-side construct can
remove it — only a lock that every writer takes, which is the ~29-site repo-wide change
tracked in #2147 and which was already demonstrated (round 2/3) to break the gateway when
forced into this PR.
The three ways out are a product call, not a code call:
- Accept it — identical semantics to the other 29 config writers, including
config set. Needs a writer's/ai-review override. - Drop the automatic write (the requested fix) —
uppublishes, then prints the
kirocrew config set dashboard.tailscale.enabled truecommand. Closes this finding
permanently, and returns the flow to the two steps this PR set out to collapse into one. - Route the write through the running gateway so one process is the only writer.
upalready requires a live gateway since the port fix, so the precondition holds —
but it is a real redesign and only serialises if the gateway's own handler does.
Asked the requester to choose. Nothing is being smuggled through in the meantime: the
behaviour on this SHA is option 1's, and it is stated here rather than implied.
Gates on 21f64846, all by exit code: isort 0, flake8 0, mypy 0 (863 files), docs-lint 0,
tailnet suites 144 passed / 3 skipped. config/loader.py still byte-identical to main.
E2E (stub ACP backend, offline) passed on 1a3a9d9a, which closes out the earlier
E2E red — it was the ConfigChangedError(OSError) change in the shared loader, and
reverting that fixed it.
|
Reviewed SHA What changed
Three reasons this is the right shape rather than a capitulation:
One correction to the record, found by my own testI wrote a test asserting the config file is byte-identical after each action. It So the honest claim is narrower than "nothing is written", and that is what the test now It also means removing my write did not take the command's total write count to zero — Tests
Docs updated: the guide shows the one-time Gates on |
KiroCrew never ran `tailscale serve` anywhere. The config switch made the gateway
*trust* the tailnet origin; putting the dashboard on the tailnet was a command the
operator had to know and type. Two independent changes are required and either one
alone is a dead end — publish without trust and every request is refused by the
Origin check with a bare 403, trust without publish and there is nothing listening
— so the documented flow was three commands, one of them undocumented in the UI.
Adds `kirocrew tailnet {up,down,status}`.
`up` publishes first and records the config only once publishing succeeded. The
reverse order would leave a host claiming tailnet access is on with nothing serving
it, and the operator's next clue would be a 403 from another device. It prints the
URL to open, and says a restart is needed *unconditionally* — including when the
switch was already on, because the origin is resolved once at gateway startup.
`status` reports the three things that are independently required: whether the
setting is on, whether a MagicDNS name resolves right now, and whether serve is
actually pointing at this dashboard. Any one of them being wrong looks identical
from the user's chair, so a single on/off line would not be diagnostic. The live
name read is correct here and would be wrong in `GET /api/tailnet/status`: this
command reports what the machine can do next, the endpoint reports what the running
server already trusts.
`down` stops publishing and deliberately leaves the config alone — the trusted
origin is unreachable without serve, so clearing it would be an unrequested second
change that also demanded a restart to undo a withdrawal that took effect at once.
New module rather than an addition to `dashboard/tailnet.py`
`tailnet.py`'s documented contract is the opposite of what a write path needs: it
swallows every failure so the gateway boots on a host that has never heard of
Tailscale. Here a failure is the point of the call — `tailscale serve` refuses for
reasons the operator can act on and cannot guess, most often because changing serve
config needs root or an `--operator` grant. `tailnet_serve.py` therefore returns a
`ServeResult(ok, code, detail)` and **always passes the daemon's own stderr
through verbatim**: `code` is a best-effort classification for the UI to branch on,
`detail` is what Tailscale actually said. Upstream owns that wording, so a wrong
classification must still leave the operator with the real reason.
Published-state detection does not read the JSON schema. This host has no
Tailscale, so the shape of `tailscale serve status --json` is unverified here.
Rather than guess key paths, `serve_state` searches the parsed document for a proxy
target naming our own port, and an unrecognisable document reports `unknown` rather
than `not published` — reporting "not published" for a published node is the
checked-but-never-ran defect in a new costume.
Governance
`publish` is a fourth chokepoint on `capabilities.tailnet_origin`, checked before
the spawn: a pinned fleet forbids putting this host's dashboard on a tailnet, and
refusing after publishing would be theatre.
`unpublish` is deliberately NOT gated, and the asymmetry is load-bearing.
`is_governance_pinned_off` returns true both for a real policy deny and for a
ceiling it could not evaluate, so gating withdrawal would mean a transient
policy-read failure leaves a dashboard published on a tailnet with no supported way
to take it down — a fail-closed control failing open in effect. Same direction that
lets a config write of `false` through while `true` is refused.
Spawn hardening is shared with the read path by import, not copied: the binary
comes from `_cli_path`'s vetted absolute allowlist and never from `PATH`, and the
child gets `sandbox.scrub_env()`. Registered in `BENIGN_SPAWNS` with the reason it
is not routed through `sandboxed_spawn_argv` — the call's whole purpose is to mutate
the local daemon's serve config through its unix socket, which is precisely the
ambient authority a sandbox removes.
Config write
`tailnet up` records the setting through its own small writer
(`_record_tailnet_enabled`) rather than sharing `config set`'s path, and
`config set` is left byte-identical. An earlier revision did extract a shared
writer; it was reverted because `config set` reads the file TWICE on the way to a
write -- validate, then `KiroCrewConfig.load()` to build the full serialisation it
persists -- and another writer truncating between those reads makes the second one
observe defaults, which are then written over everything the user has. Sharing that
path meant inheriting the race or changing what every `config set` invocation writes
to disk.
The private writer is one read, one mutation, one atomic write:
`read_config_for_update` (`{}` for an absent file, raises `ConfigReadError` for a
present-but-unreadable one), the value set on that same dict, `write_config_atomically`.
A non-object `dashboard` / `dashboard.tailscale` section is refused rather than
coerced -- replacing it with `{}` would discard the operator's data and still report
success.
`up` then verifies the EFFECTIVE value rather than the write, because
`config.local.json` takes precedence: a host whose overlay disables this has just
had a successful write that changes nothing on restart, and printing "= true" there
is the false promise this feature exists to remove.
Docs
The guide's Tailscale section leads with `kirocrew tailnet up`, keeps the manual
three-command form as the fallback, and states the root/`--operator` requirement.
The governance spec's chokepoint table goes from three rows to four and records the
withdrawal asymmetry.
Not verified against a real Tailscale daemon — no Tailscale on this host. What that
leaves unverified is narrow and stated in the code: the exact argv is asserted, the
failure classification is best-effort with the real stderr always passed through,
and published-state detection is schema-agnostic by design.
Gates: 43 tailnet tests + 587 in the surrounding suites, isort, flake8, mypy (826
files), docs-lint, brand-lint. `test_beacon.py::TestInstallId` fails on this host
before and after this change (fails when run alone on the base commit too).
|
Reviewed SHA FIXED — the overlay bypassed the shape guardCorrect, and a real hole in my own fix from two rounds ago. I validated Both files are now validated before anything calls Tests: every action refuses on a malformed overlay for both REBUTTED — "persist the enabled setting after publishing succeeds"This is the write path you blocked on three consecutive SHAs:
Reinstating it would reopen a blocker that took five rounds to close, so I am not making On the premise itself — "contrary to the stated one-command flow" — that flow is no Two things worth keeping in view, since the trade is deliberate rather than free:
Gates on |
KiroCrew never ran
tailscale serveanywhere. The config switch made the gatewaytrust the tailnet origin; putting the dashboard on the tailnet was a command the
operator had to know and type. Two independent changes are required and either one
alone is a dead end — publish without trust and every request is refused by the
Origin check with a bare 403, trust without publish and there is nothing listening
— so the documented flow was three commands, one of them undocumented in the UI.
Adds
kirocrew tailnet {up,down,status}.uppublishes first and records the config only once publishing succeeded. Thereverse order would leave a host claiming tailnet access is on with nothing serving
it, and the operator's next clue would be a 403 from another device. It prints the
URL to open, and says a restart is needed unconditionally — including when the
switch was already on, because the origin is resolved once at gateway startup.
statusreports the three things that are independently required: whether thesetting is on, whether a MagicDNS name resolves right now, and whether serve is
actually pointing at this dashboard. Any one of them being wrong looks identical
from the user's chair, so a single on/off line would not be diagnostic. The live
name read is correct here and would be wrong in
GET /api/tailnet/status: thiscommand reports what the machine can do next, the endpoint reports what the running
server already trusts.
downstops publishing and deliberately leaves the config alone — the trustedorigin is unreachable without serve, so clearing it would be an unrequested second
change that also demanded a restart to undo a withdrawal that took effect at once.
New module rather than an addition to
dashboard/tailnet.pytailnet.py's documented contract is the opposite of what a write path needs: itswallows every failure so the gateway boots on a host that has never heard of
Tailscale. Here a failure is the point of the call —
tailscale serverefuses forreasons the operator can act on and cannot guess, most often because changing serve
config needs root or an
--operatorgrant.tailnet_serve.pytherefore returns aServeResult(ok, code, detail)and always passes the daemon's own stderrthrough verbatim:
codeis a best-effort classification for the UI to branch on,detailis what Tailscale actually said. Upstream owns that wording, so a wrongclassification must still leave the operator with the real reason.
Published-state detection does not read the JSON schema. This host has no
Tailscale, so the shape of
tailscale serve status --jsonis unverified here.Rather than guess key paths,
serve_statesearches the parsed document for a proxytarget naming our own port, and an unrecognisable document reports
unknownratherthan
not published— reporting "not published" for a published node is thechecked-but-never-ran defect in a new costume.
Governance
publishis a fourth chokepoint oncapabilities.tailnet_origin, checked beforethe spawn: a pinned fleet forbids putting this host's dashboard on a tailnet, and
refusing after publishing would be theatre.
unpublishis deliberately NOT gated, and the asymmetry is load-bearing.is_governance_pinned_offreturns true both for a real policy deny and for aceiling it could not evaluate, so gating withdrawal would mean a transient
policy-read failure leaves a dashboard published on a tailnet with no supported way
to take it down — a fail-closed control failing open in effect. Same direction that
lets a config write of
falsethrough whiletrueis refused.Spawn hardening is shared with the read path by import, not copied: the binary
comes from
_cli_path's vetted absolute allowlist and never fromPATH, and thechild gets
sandbox.scrub_env(). Registered inBENIGN_SPAWNSwith the reason itis not routed through
sandboxed_spawn_argv— the call's whole purpose is to mutatethe local daemon's serve config through its unix socket, which is precisely the
ambient authority a sandbox removes.
Refactor
cli_config.set_base_config_keyis extracted fromconfig setsotailnet upgets the same write semantics (unknown-key detection, overlay subtraction) rather
than a second, subtly different writer. Governance gating stays at the call sites,
where the wording and exit path differ per command.
Docs
The guide's Tailscale section leads with
kirocrew tailnet up, keeps the manualthree-command form as the fallback, and states the root/
--operatorrequirement.The governance spec's chokepoint table goes from three rows to four and records the
withdrawal asymmetry.
End-to-end verification
test/test_tailnet_e2e.pyexercises the real process boundary — a realexecutable named
tailscaleon disk, found by the production_cli_path, spawnedby the production
subprocess.rununder the productionscrub_env(), answeringover real stdout with real exit codes. It persists serve config, so
publish -> status -> withdrawruns as a real state machine, and it records everyargv it received. That covers what mocks structurally cannot: the argv a real daemon
would get, that
scrub_env()leaves a usable environment (a scrubber thatstripped too much passes every mocked test and fails on every real host), stderr and
exit codes propagating through the CLI's own exit path, and the foreign-443 refusal
end to end.
The argv contract is verified against upstream source rather than assumed:
cmd/tailscale/cli/serve_v2.goconfirms--bg(:239),--httpsas aUintVarso--https=443parses (:241),serve status --json(:257/:262), andoffas atrailing positional with the target optional (:361).
Still unverified: Tailscale's own runtime behaviour and the real shape of
status --json. A real daemon is unobtainable on this host —pkgs.tailscale.comand
proxy.golang.orgare both outside the sandbox's egress allowlist, andTailscale ships no binaries on GitHub releases.
TestAgainstARealDaemoncloses thatgap and skips unless a real tailscale is installed, so it is inert in CI and is
the whole verification on a host that has one.
Gates: 43 tailnet tests + 587 in the surrounding suites, isort, flake8, mypy (826
files), docs-lint, brand-lint.
test_beacon.py::TestInstallIdfails on this hostbefore and after this change (fails when run alone on the base commit too).
Stacked on #2102 (the status card). Review that one first; this PR's diff against it is the serve-control half.