feat(server): one-shot primary nav smoke (closes #167) - #585
Conversation
divshekhar
left a comment
There was a problem hiding this comment.
Thanks for this — the naming discipline here is genuinely good. Calling the result
renderedWithoutConsoleErrors rather than "the route works" is exactly the honesty this repo asks
for, and making the skip reasons explicit rather than folding them into a pass is the harder and
correct choice.
Three things before this can go in, in order of how much they cost you:
1. CI is red on five checks — verify, package-quality, macos, windows, gate.
verify red usually means format:check or lint, and verify is a dependency of every expensive
gate behind it, so nothing downstream has actually run against this branch yet. pnpm format writes
the formatting fixes; please get pnpm format:check && pnpm lint && pnpm typecheck && pnpm test:unit
green locally before pushing again, since each push burns a full CI run.
2. No e2e spec. This adds a new action to the tool surface, and the unit gate cannot see
cross-package drift on the tool surface — that is specifically what pnpm test:e2e exists for. A new
verb with no spec in the battery is a verb that can silently stop working without anything going red.
3. Timing. This is +515 lines and a new directory, and the current release is scoped tightly to
stability: things that break, crash, or stop a verification completing. A new capability is off that
theme, not because it is unwanted but because reviewing a new subsystem mid-stability-release is how
both get worse.
So: get CI green, add the e2e spec, and this is a strong candidate for the next feature release
rather than this one. Requesting changes to keep it out of the merge queue for now, not to send it
away.
4f4c5a6 to
edc5fed
Compare
|
@divshekhar thanks — addressed all three:
|
Closes reticlehq#167. Walks nav landmark links and returns a consolidated table with honest renderedWithoutConsoleErrors naming instead of over-claiming route health. Signed-off-by: Kushal S <skushal.mys@gmail.com>
exactOptionalPropertyTypes rejected optional fields assigned from possibly-undefined values, and the unit stubs were incomplete ReticleEvents. An e2e spec now walks next-smoke's real nav so the new verify action cannot silently rot. Signed-off-by: Kushal S <skushal.mys@gmail.com>
edc5fed to
68c1f8e
Compare
|
Rebased onto |
|
Rebased this locally onto current main to see whether the red CI was staleness. It is not — there is one real failure, and it is small: The repo has a guard requiring every numeric tool input to carry a bound, and the new Everything else was clean after the rebase — format, lint and typecheck all pass, and the rest of the server suite is green (6,527 of 6,528). So it is one schema change plus a rebase, not a rework. Worth saying since the diff is +581: I have not reviewed the design of the nav-smoke feature itself yet, only established that the CI red is one fixable thing. I will read it properly once it is green. |
|
Nudge rather than new review — the three items in my earlier review still stand, and item 1 is the one blocking everything else: #167 is still open and nobody else has taken it. The naming discipline in this PR is the reason I want it in rather than rewritten. |
Summary
reticle_verify { action: "nav_smoke" }to walk primary nav links (default scope:navlandmark)href, clicks each internal link (bounded, default 25), returns one consolidated tablerenderedWithoutConsoleErrorsnaming per issue [agent] One-shot nav smoke: walk the primary routes and return one pass/fail table #167 (not "the route works")skippedreasonsTest plan
pnpm format:checkand@reticlehq/serverlint greenCloses #167