chore(release): pre-release P0/P1 cleanup#1393
Merged
Conversation
P0 fixes: - delete src/analysis.ts (179 lines, 0 importers across src/clis/extension) - remove dead OPENCLI_DIAGNOSTIC negative test assertion - rename OPENCLI_BROWSER_TIMEOUT to OPENCLI_BROWSER_IDLE_TIMEOUT — the env controls workspace lease idle release, not command runtime; old name was misleading and undocumented (no fallback needed) - add 'fill' to validate.ts KNOWN_STEP_NAMES so adapters using PR #1222's fill pipeline step do not trip "unknown step name" warnings during validate P1 fixes: - BrowserConnect daemon-not-running hint: replace stale "make sure port is available" with actionable "run opencli doctor / opencli daemon restart" - TimeoutError hint: lead with --timeout flag, demote env var to secondary
@pr-monitor flagged the prior "add 'fill' to KNOWN_STEP_NAMES" fix as treating only the symptom — two parallel hand-maintained lists will keep drifting whenever a new pipeline step is registered. Address the root cause: pipeline/registry.ts now exports `getRegisteredStepNames()` and validate.ts builds KNOWN_STEP_NAMES from that. Adding a step via `registerStep()` automatically allowlists it.
@pr-monitor follow-up: lock the validate ↔ pipeline registry linkage at
the test layer so future drift is caught immediately.
Changes:
- recompute KNOWN_STEP_NAMES per-call (was const at module load) so
steps registered after validate.ts import (plugins, dynamic registration)
are honoured
- add src/validate.test.ts with 3 cases:
1. every step name from getRegisteredStepNames() exists
2. an adapter using every currently registered step does not warn
3. a step registered at runtime is automatically allowlisted by
validate without any source change to validate.ts
Same double-list drift pattern as validate.ts KNOWN_STEP_NAMES (audit follow-up flagged in this PR's evolution thread). The fill step was registered in pipeline/registry.ts (PR #1222) but never added to the browser-only allowlist in capabilityRouting.ts. Concrete impact: - shouldUseBrowserSession() didn't recognize a `[{ fill: ... }]` pipeline as needing a browser, so PUBLIC adapters using fill could end up without a page and crash inside stepFill at `page!.fillText(...)` - pipeline/executor.ts's per-step retry policy (BROWSER_ONLY_STEPS gets 2 retries on transient errors, others get 0) skipped fill — losing retry coverage on a DOM-touching step Fix: - add 'fill' to BROWSER_ONLY_STEPS - add a documenting comment explaining BROWSER_ONLY_STEPS is the browser-touching subset of registered steps (not the full set) - export _validateBrowserOnlyStepsAgainstRegistry() so the test layer catches the inverse drift (browser-only step that no longer exists) - 3 new tests in capabilityRouting.test.ts: * pipeline with fill routes to browser session * BROWSER_ONLY_STEPS subset of registered step names * fill is in both lists This addresses @pr-monitor follow-up #3 (audit similar double-list patterns) for the obvious in-scope candidate. Other candidates outside this PR's scope: build-manifest serialization vs registry shape, error code unions vs lint baselines.
…gression test Self-review nit: `strategy: 'public' as never` worked but bypassed the typed CliOptions union. Use `Strategy.PUBLIC` so the test exercises the real public API.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pre-release code review found 4 P0 + 2 P1 issues. This PR fixes all of them, plus a structural follow-up to one of the P0 fixes after reviewer feedback.
P0 — must-fix
src/analysis.tsdeleted (179 lines, 0 importers across src/, clis/, extension/src/)OPENCLI_DIAGNOSTICtest residue removed (env has 0 production usages)OPENCLI_BROWSER_TIMEOUT→OPENCLI_BROWSER_IDLE_TIMEOUT(old name was misleading and undocumented; safe breaking rename)validate.tsKNOWN_STEP_NAMESwas missingfill(silent friction for adapters using PR feat(browser): add verified fill command #1222'sfillstep) — see "P0 fix: preserve zhihu hot question ids beyond safe integer #4 evolution" belowP1 — should-fix
formatBrowserConnectErrordaemon-not-running hint refresh (port-conflict guidance →opencli doctor/daemon restart)TimeoutErrorhint refresh (--timeoutflag promoted, env demoted to secondary)P0 #4 evolution — single-source-of-truth + plugin-aware invariant lock
Three commits, three layers of fix:
f1ff513c'fill'to the hardcoded allowlist setf1433d59getRegisteredStepNames()exported frompipeline/registry.ts— eliminates the parallel hand-maintained listf871743aconstat module load, missed plugin-registered steps) + add 3 regression tests, including one that registers a step at runtime and verifies validate auto-allowlists itThe third commit is the meaningful one: it locks the invariant at the test layer, so any future drift between the pipeline registry and the validator is caught immediately.
Verification
npx tsc --noEmitcleannpx vitest run src/cli.test.ts src/validate.test.ts src/commanderAdapter.test.ts src/execution.test.ts src/registry.test.ts src/errors.test.ts→ 194/194 pass (3 new in validate.test.ts)npm run buildregenerates manifest cleanlyOPENCLI_BROWSER_TIMEOUT0 residual,OPENCLI_DIAGNOSTIC0 residual,analysis.tsimport 0 residualOut of scope
Test plan
OPENCLI_BROWSER_IDLE_TIMEOUT=120 opencli browser open https://example.comhonors the new env nameopencli validatedoes not warn for adapters usingfillpipeline step