Repository navigation
Document release binary usage in README - #6
Merged
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Merged
linletian
added a commit
that referenced
this pull request
Oct 15, 2026
…develop (#119) * diag(terminal): [tty-metrics] instrumentation for tty WS subscriber overflow (issue #114) Data collection only, no behaviour change. Server: metered *ws.Conn wrapper (internal/app/tty_metrics.go) timing every TTY socket write, logging handshake replay stats, per-overflow drain-rate fields, per-connection write-block summary, and >=1s slow-write lines (op=ping proves heartbeat starvation). Client (index.html): per-connection ready/close lines with rx bytes, #94-bracketed replay volume, tab visibility and xterm.js parse backlog, visibilitychange logging, and window.__ttyMetrics() snapshot. Respects node-test slicing rules (no new free identifiers in sliced blocks, document behind typeof guard) and keeps the pinned overflow log line and reconnect source line verbatim. Full test suite passes; verified end-to-end with a live yes-flood smoke run (overflow fired 56ms after subscribe; server binBytes matched client rx to the byte). Runbook: docs/plans/fix-issue114-tty-overflow/DATA-COLLECTION.md * diag(terminal): gate [tty-metrics] instrumentation to beta builds via -tags ttymetrics (issue #114) Build-channel gating so beta releases keep the instrumentation long-term while stable ships none of it: - internal/app/tty_metrics.go splits into shared types plus tty_metrics_on.go (//go:build ttymetrics: real timing/logging) and tty_metrics_off.go (//go:build !ttymetrics: passthrough stubs that inline away); ttyMetricsBuild is the channel switch. - release-beta.yml builds both binaries with -tags ttymetrics; release.yml is untouched (no tag). go-ci.yml tests and builds both variants so neither bit-rots. - withServerRevision hands out the mw_tty_metrics=1 cookie only in beta builds; the client half (console lines, visibility logging, xterm.js write-callback timing, __ttyMetrics dump) arms only when the cookie is present, so stable pages stay fully silent. Review follow-ups (REVIEW-TEMP-fix-issue114-tty-metrics.md, F1-F5, all confirmed valid and fixed): - F1: stable builds now actively EXPIRE a stale mw_tty_metrics cookie planted by a beta daemon earlier in the same browser session, instead of leaving the client half armed with no server counterpart. - F2: the two EndOffset probes (handshake entry, overflow branch) and the handshake metrics snapshot are const-gated behind ttyMetricsBuild, so stable builds compile the manager reads away entirely; the off-file comment no longer overclaims zero cost. - F3: cookie checks use a boundary-bracketed match ("; mw_tty_metrics=1;" in "; "+cookie+";") so suffix-named lookalikes cannot false-arm the gate (module helper and connectTTY's typeof-guarded local, per the node slicing rules). - F4: the client close line now carries the xterm.js parse backlog in both scopes: session-cumulative (writeCalls/writeWait*) and per-connection deltas (connWriteCalls/connWriteWaitMs) against a baseline snapshotted at connect. - F5: the instrumentation gains automated coverage: tty_metrics_test.go (tagged: handshake/conn-closed/overflow lines with field payloads, cookie planted only when absent) and tty_metrics_cookie_off_test.go (untagged: stale cookie expired, never planted). Verified: both build variants compile; full untagged suite green (21 packages); tagged app/ws tests green; internal/ui node slice tests pass; gofmt clean; beta/stable runtime cookie behaviour smoke-checked. * fix(instance/reasonix): kill orphaned serve via in-memory pid table when the pid file is gone (issue #113, #111 P1) Root cause: Driver.Stop read the pid solely from <DataDir>/reasonix/<id>/pid. When that file was wiped before Stop ran — Restart migrates onto a fresh id and Cleanup removes the old dir; tests RemoveAll their data dir the same way — Stop reported "never started" and the serve (a fake-serve.py fixture in tests, ~16MB, serving until explicitly killed) was orphaned forever: every go test round leaked 2. On the dev box this accumulated to hundreds of orphans (~2GB) and was a measured amplifier of the issue #111 OOM incident. Fix (production): the Driver keeps an in-memory pid table (pidsMu/pids, id → pid) as a second, independent pid source. - Start registers at cmd.Start (SysProcAttr.Setpgid makes the exec pid the group leader, so Stop's kill(-pid) reaches the whole group during the readiness window), overwrites with the pid file's authoritative pid after waitReady, and the readiness-failure path untracks. - Start's idempotent re-attach branch adopts a survivor pid (this Driver never spawned it); kind.go Reattach deliberately stays Health-only. - Stop falls back to the table on all three ways the pid file yields nothing (missing / unreadable / unparseable-or-non-positive); "never started" now means neither source has a positive pid. The kill sequence itself is unchanged. - Cleanup still neither kills nor untracks (a Stop arriving after the dir is gone needs the entry — that is the whole point), and now logs a WARNING when it runs while a live pid is still tracked: the Stop-then-Cleanup order becomes a tripwire for future callers. Deliberate scope (arbitration, recorded in the pids field comment): production does NOT scan the process table to kill historical orphans by cmdline pattern (risks foreign processes), and a startup sweep of <DataDir>/reasonix/*/pid to adopt cross-restart orphans is recorded as a known, not-implemented follow-up — #113's scope is the test leak. Fix (test hygiene, symmetric backstops so a forgotten Stop can no longer leak): newFakeServeDriver sweeps everything the driver still tracks (trackedInstanceIDs); framework newReasonixTestManagerOn and app newProxyTestEnv gained t.Cleanup backstops through the production StopAllKind; TestRestartReasonixCleansOldDir stops the FRESH id Restart migrated onto (its old-id Stop never covered it). Dual-review record: opencode (step-5-preview-free, independent, no sub-agents) and mcode both APPROVE the final diff. Round-1 findings R1-R5 were fixed and re-reviewed; round-2 findings F1-F6 (all comment/CHANGELOG accuracy, arbitrated valid) are corrected in this diff — verified mechanical: the non-comment changed lines in driver.go are exactly the reviewed code set above. opencode's proposal of a liveness pre-check before signaling the fallback pid was REJECTED with evidence: a dead pid already exits via the first processAlive poll (~100ms; the Start reaper goroutine prevents zombies), and the pid-reuse hazard it targeted is pre-existing and isomorphic to the pid-file path Stop has always had. Pinned by TestStopFallsBackToTrackedPid (state dir wiped before Stop), TestStopDuringPendingStartKillsTrackedPid (Stop mid-Start, no pid file yet), TestFailedStartDropsTrackedPid (readiness failure leaves no dirty entry), TestCleanupWarnsWhenStopWasSkipped (warning fires on the wrong order, silent on the right one, Cleanup provably kills nothing). Verified: go build ./..., go vet ./internal/..., gofmt clean; go test ./internal/... -count=1 green (plus -race on reasonix/framework/app in review); fake-serve orphan delta 0. * fix(app,instance/reasonix): P3 hygiene — scope-tracker reset wiring, authFails TTL sweep, serve.log rotation (issue #111) Three long-resident-daemon hygiene leaks from the issue #111 review, each bounded but permanent: - S1: ScopeTracker.Reset (opencode_web/dsh_web) documented itself as "used on instance stop" but had zero production callers — per-instance proxy state accumulated forever. Server.dropScopeState(id) now clears both trackers on the success path of all five in-process teardown doors (HTTP stop/restart/delete, MCP instance_stop/instance_delete; restart clears the OLD id only — Restart migrates ids). Refused teardowns keep state (the instance still lives, the warning must stay). Known residuals, recorded in the CHANGELOG and code comments: instances that die on their own (runLifecycle marks terminal inside framework, no app callback) and the cross-process CLI door (`myworktree instance stop` builds its own Manager in a separate process, cli.go:315/368 — daemon trackers are structurally unreachable); both are bounded by historical store records and reclaimed by any later server-side door. Follow-up recorded: delegate CLI teardown to the daemon HTTP API. - S2: authFails (20/min/IP sliding window) only reset an entry when that IP returned — one-shot scanners left permanent entries. A threshold-triggered sweep now evicts closed windows from allowAuthAttempt (no goroutine: 1m cadence, 5s when busy, soft cap 1024 triggers early, never truncates live entries). Rate-limit semantics are byte-identical: the eviction predicate is the same now.After(WindowEnd) the read path uses, so an evicted IP and an expired one get the same answer (Count 1). - S3: reasonix serve.log was O_APPEND forever. Start now rotates an over-cap (10MB) serve.log to serve.log.1 (single backup, atomic os.Rename overwrite) before OpenFile; rotation while running stays out of scope (python holds the fd). Size check uses os.Lstat so the object judged is the object rename moves (a symlinked serve.log is left alone instead of silently breaking rotation). Dual-review record: opencode (step-5-preview-free, independent) and mcode. Round 1: opencode APPROVE (3 nit), mcode REQUEST_CHANGES (1 major: the unwired cross-process CLI door falsified the CHANGELOG's "every teardown door" claim; 1 minor same; 2 nit). Fixes F1-F6 (CHANGELOG precision, Lstat, locked test calls, dead-assertion removal, comment accuracy) — re-review: opencode APPROVE (no findings), mcode APPROVE (1 nit: Lstat lacked a pinning test, added). Mutation-checked by both reviewers and the arbiter: every wiring point, predicate and threshold is pinned red when removed/flipped. Pinned by scope_reset_test.go (5 tests incl. refused-teardown and restart-old-id-only), auth_fail_ttl_test.go (9 tests incl. boundary predicate, cadence, concurrent -race), serve_log_rotate_test.go (9 tests incl. at-cap boundary, .1 overwrite, symlink skip). Verified: go build/vet/gofmt clean; go test ./internal/... -count=1 green (21 packages); -race on app/reasonix/opencode_web/dsh_web; fake-serve orphan delta 0. * feat(monitor): instance process-tree visibility + system-level pre-OOM alerts (issue #111 P2) The blind spot the incident named (issue #111 §4/#6): collector sampled the daemon PID and instance MAIN PIDs only, the modal footer confessed "Child processes spawned by instances are not counted", and 230 leaked fake-serve.py children (~2GB) hid in exactly that gap. This implements the READ-ONLY observability design from issue comment #2 / roadmap comment #4 P2. ZERO-INTERVENTION is the spine: every new line only READS (gopsutil + /proc/pressure); nothing kills, reaps, signals, cgroup-caps, or restarts. Every "advice" is a USER action pointing at docs/PLAYBOOK-memory-alerts.md. Detection only; disposal is the user's. - Tree aggregation (internal/monitor/tree.go): BFS below each running instance's main PID over a SINGLE process enumeration per collect (injectable listProcs seam, ppid index — the first cut's per-node Children() was a full /proc scan per node). Bounded by depth 8 + 512 descendants/instance + visited dedup (cycle guard AND pid-reuse dedup). children = top-10 by RSS; "and M more" = child_count - len(children). children_truncated means WALK-CAP ONLY (ChildCount is then a floor, not a census); the top-N display cut is NOT truncation. tree_unavailable marks an unreadable tree (unreadable != measured zero). - Alert evaluation (internal/monitor/alerts.go): EvaluateAlerts is pure. PSI memory some avg10 >=30 amber / >=50 red (red mirrors oomd's own kill line, same source, so the warning is always first); available RAM <15%; one instance subtree >40% of RAM; one instance descendants >50; daemon >1GiB. One entry per signal at the highest severity; reds first. TotalBytes==0 skips ratios but NOT child-count (absolute count = the leak fingerprint); PSI whole-token `some` match. - Layered polling (internal/monitor/sampler.go): modal's 2s full poll carries fresh alerts + per-instance subtree + system; a background AlertSampler goroutine samples PSI + available RAM + daemon RSS every ~10s (never a tree walk). Stop joins the loop via done (no-leak), dead state sticky in both stop orders, /proc read outside the cache lock. GET /api/monitor/alerts serves the cache with sampled_at_ms so cold (0 = no-data) and warm stay distinct; a blind refresh (no signal read) stays cold. - API additive only (internal/monitor/stats.go, docs/API.md): new InstanceStat subtree fields + Stats.alerts/system + GET /api/monitor/alerts; no existing field re-defined. - UI (internal/ui/static/index.html): status-bar alert badge (amber/red) + count while Transport text/wiring unchanged; modal gains the alerts strip + per-instance expandable subtree (top-N + "and M more"); the old disclaimer is replaced by measured subtree totals + the honest visibility boundary. Light poll stands down for the badge while the modal is open; badge gated on sampled_at_ms>0. Pinned by internal/monitor/{tree,alerts,sampler}_test.go, internal/app/ monitor_alerts_test.go and internal/ui/testdata/monitor_alerts.test.mjs (18 cases). Playbook: docs/PLAYBOOK-memory-alerts.md with explicit <a id> deep-link anchors (every advice anchor resolves; pinned by TestPlaybookAdviceAnchorsAreReachable). Dual-review record: - Round 1 (opencode sub-agent independent review + external mcode) both REQUEST_CHANGES. MAJOR F1 children_truncated floor/top-N conflation (unpinned: deleting the top-N assignment stayed green); F2 dead playbook anchors + CHANGELOG naming a TestPlaybookAdviceAnchorsAreReachable that did not exist; F3 sampler join "pinned" overclaim (removing <-a.done stays green); F4 PublishSystem coherence tautological. Plus MINOR/NIT: badge flicker, unused MainRSS, API tree_unavailable/children omitempty, sampled_at_ms claim, API example strings, Start-wiring test, unconditional buildProcIndex, dead CSS class, transport color, sortAlerts unknown severity. The arbiter ruled every finding with evidence (mutation testing) and a coding agent fixed them. - Round 2 both reviewers APPROVE; mutation gate F1/F2/F3/F4/F9/F13 all RED (no vacuous pin). Residuals MINOR/NIT (CHANGELOG anchor wording, blind-but-warm cache honesty, API stats example self-consistency, nil-index guard, children omitempty doc) fixed and confirmed by the arbiter's own mutation tests (disabling the tick blind-guard and the nil-index guard each turn their pin RED). Verified: go build/vet/gofmt clean; go test ./internal/... -count=1 green; -race on internal/monitor + internal/app green; node --test monitor_alerts 18/18; fake-serve orphan delta 0 across the run; no leaked external processes. * refactor(monitor,app,ui): address PR #119 review — PSI stale-clear, formatBytes half-up, churn/truncation honesty, pins + doc fixes (issue #111) Follow-up to the #111 Resource Monitor feature, driven by the two PR #119 reviews (minimax Code / Mavis, opencode). No behavior regressions; all changes arbitrated with evidence. #114 files deliberately untouched (shared SHAs with fix/fix-issue114). Honesty / correctness: - F-R1m2 sampler: a FAILED PSI read now CLEARS HasPSI/PSIsSomeAvg10 (tick and PublishSystem). It was only ever set true, so one unreadable /proc/pressure/memory left the last reading standing and a red psi-memory alert could stick until the next successful read or restart — stale value presented as current. Mem-only refresh now reports PSI unknown. Pinned by TestAlertSamplerFailedPSIReadClearsStaleRed. - F-M1 alerts.formatBytes now rounds HALF-UP (math.Round then FormatFloat) to match the UI's toFixed; strconv is ties-to-even, so 1.25 MiB printed "1.2 MB" (Go) vs "1.3 MB" (UI) and 3.125 GiB "3.12 GB" vs "3.13 GB" in the same modal. Comment corrected to the half-up truth. Pinned by TestFormatBytesTiesRoundHalfUpToMatchUI. - F-m3 collector: an instance whose tree is tree_unavailable is SKIPPED when building the alert instance set — an unreadable tree is UNKNOWN, not a small one, so its main-PID-only RSS must not fire a subtree>40% alert with an unmeasured Current. Pinned by TestCollectSkipsUnavailableTreeRowsForAlerts. - F-m4 tree: the depth-cap probe now sets Truncated only when a descendant is actually unvisited (a back-edge already in visited is not dropped data), honoring the G4 "only when data was actually dropped" promise. Pinned by TestWalkSubtreeDepthCapBackEdgeIsNotTruncated. - F-R1n1 UI: closeMonitorModal now calls fetchMonitorAlerts() after close, so the status-bar badge converges from the modal's full set to the cache-only set immediately instead of lingering up to 10s. 19th mjs case pins it. - F-n5 API.md + modal footnote now actually carry the churn-floor caveat the CHANGELOG already claimed was documented (mid-walk churn can drop a visible branch silently, so child_count/subtree may be a floor even when children_truncated=false). Test-gap closures (behavior was already correct; the pin was missing/vacuous): - F-M3 sampler: PublishSystem blind-refresh guard pinned (TestAlertSamplerPublishSystemBlindStaysCold). - F-M4 app: restart "clears old id only" is now non-vacuous — a pre-seeded canary id must survive and the tracker must lose exactly one entry (TestHandleInstanceRestartClearsOldIdOnly). - F-M5 app: Stop-failure keeps-scope subtest. framework.Manager.Stop swallows kind errors and always returns nil (manager.go:389), so the handler's Stop-error branch has no live failure today; a minimal documented `managerStop` seam (monitor seam pattern) makes the defensive ordering testable. Pins the "wipe sits after the Manager call" ordering. - F-m5 alerts: PSI boundary table pins exact severity (49.99 amber / 50.0 red / 30.0 amber), so a >=→> mutation goes red. - F-n2 alerts: NewAlertSampler clamps interval<=0 to defaultAlertInterval (time.NewTicker(0) would panic); TestNewAlertSamplerNonPositiveInterval. - F-daemon-twice: PublishSystem takes the already-sampled daemon RSS instead of re-reading it on every full poll (one read per PID). - Nits: alerts comment example "≥ 30.0%"; API.md children bullet indent; churn-floor footnote; moved TestWalkSubtreeNilIndexIsUnavailable into tree_test.go; authFailsSweepBusy comment corrected to the true bound (the 60 s eviction window governs the map size; the 5 s busy cadence only shortens the post-flood drain tail and sweeps 12× while over the soft cap). Deferred to #114 (out of #111 scope, keeps shared SHAs): m1 tty_metrics logOverflow liveFrames. M2 gone-vs-unreadable split: recorded as accepted — procInfoOf reports ok=false only when the process is gone by enumeration; field-read failures return ok=true, and under the single-user trust model an instance's own subtrees are readable, so the honesty gap is closed by the new churn-floor documentation rather than a Gone/Unreadable split that would surface normal churn. CHANGELOG mjs count updated 18→19. Verified: build/vet/gofmt clean; go test ./internal/... -count=1 green; -race on monitor+app green; node mjs 19/19; fake-serve orphan delta 0. Each new pin verified RED on the reverting mutation. * refactor(monitor,app,ui): address PR #119 round-2 — partial-tree lower-bound alert, dialog close-event, dropScopeHook seam (issue #111) Follow-up to #111 Resource Monitor, from the round-2 PR #119 reviews (minimax Mavis, opencode — both re-confirmed the earlier fixes with independent mutation checks). - F-A2m1 (REWORK, adopts Mavis over the round-2 skip): a tree_unavailable row's subtree_rss is a MEASURED main-PID value (memRSS is sampled independently of the enumeration), so true subtree RSS = memRSS + descendants >= memRSS. When memRSS > 40% of total RAM the threshold crossing is PROVABLE — the row must alert, not be skipped. collector now FEEDS the row (no skip) with a Partial flag; EvaluateAlerts renders the subtree-rss Current of a Partial row as a `≥` LOWER BOUND ("≥ X% (≥ Y; main PID measured, children unenumerated — actual is higher)"). The blind spot stays lit; a floor is never dressed up as a census. Deletes the round-2 skip test; pins TestEvaluateAlertsPartialTree- RendersLowerBound + TestCollectUnavailableTreeFiresLowerBoundSubtreeAlert. docs/API.md corrected: on an unenumerated tree child_count/children are UNKNOWN but subtree_rss_bytes is a measured lower bound. - F-N1 (Mavis nit + opencode N1): a native <dialog> ESC bypasses closeMonitorModal(), so the 2s full poll never stopped and the badge never converged. Now the cleanup lives on ONE `#modal-monitor` 'close' event listener (button, ESC, and any future close all fire it): handleMonitorDialogClose clears monitorTimer and calls fetchMonitorAlerts(); closeMonitorModal just dlg.close(); fetchMonitorStats gains a dialog.open guard as a kill-switch for a stray tick. Badge converges immediately on ANY close; the 2s /proc poll cannot run past close. New mjs cases pin it; count 19→22. - F-N2 (opencode): the "restart clears the OLD id only" pin could not see a handler that also wiped the fresh id (it had no recorded state). dropScopeState call sites now route through a `scopeDropHook` seam (managerStop pattern, default = production verbatim); the restart test wraps it to RECORD the wiped-id SET and asserts exactly {req.ID}. - F-N3 + CHANGELOG: records the F-R1m2 (failed PSI read clears the signal/alert) and F-R1n1+F-N1 (any close stops the 2s poll and converges the badge) behavior changes, plus the A2m1 ≥ lower-bound behavior and the new monitor pins in the P2 "Pinned by" enumeration. Deferred to #114 (shared SHAs): m1 tty_metrics logOverflow liveFrames. M2 gone/unreadable stays recorded-as-accepted (churn-floor documentation covers it; a split would surface normal churn). Verified: build/vet/gofmt clean; go test ./internal/... -count=1 green; -race monitor+app green; node mjs 22/22; fake-serve orphan delta 0. Each pin verified RED on the reverting mutation (F-A2m1 partial-≥ branch, F-N2 item.ID drop, F-N1 close listener). tty_metrics untouched. #114 out of scope. * refactor(monitor,app,ui): address PR #119 round-3 — uniform lower-bound口径 + in-flight close race (issue #111) Follow-up to #111 Resource Monitor, from the round-3 PR #119 reviews (minimax Mavis, opencode — both independently re-confirmed the A2m1 arbiter reversal; opencode explicitly corrected its own round-2 acceptance). #114 untouched. - F-N6 (arbiter ACCEPT — the other half of A2m1's "floor != census"): a children_truncated row's subtree_rss is ALSO a lower bound (capped) and its child_count is a floor (512), yet only tree_unavailable rows got the ≥ treatment. InstanceTree.Partial is split into two explicit states — Unreadable (tree could not be enumerated; subtree = measured main PID) and Capped (walk hit a depth/512 cap). subtree-rss renders ≥ for (Unreadable || Capped) with a reason-specific note ("children unenumerated" vs "walk capped, descendants not fully counted"), and a Capped row's child-count renders "≥ N". A readable, uncapped row stays byte-identical (golden-dump verified). docs/API.md now states truncated rows are lower bounds too. - F-N5 + F-A3m1 (advice & shipped copy must speak the new口径): an UNREADABLE row firing subtree-rss no longer advises "inspect children in the table below" (that row renders "Tree unavailable" — a dead end on the worst rows); it now names the walk ("main PID crossed the line and the /proc walk failed — check permissions / hidepid"). The two remaining shipped strings (subtree-empty message + modal footnote) that still said "RSS unknown / numbers unknown" — a same-screen contradiction with the new ≥ alert — now say "child count unknown, subtree RSS a measured lower bound (main PID only)". The churn-floor sentence is preserved. The playbook's #subtree-rss gains a 读数下限 row so the advice anchor and the CHANGELOG claim stay true. - F-A3nit + F-N4 (openode reproduced): fetchMonitorStats' dialog.open guard sat BEFORE the await, so a stats request in flight when the dialog closed still repainted the FULL alert set on the badge (F-N1's symptom, back for one ~10s tick). Now re-checks dialog.open AFTER the await, before renderMonitorStats; the pre-await guard's comment is corrected to its real scope (a not-yet-started tick). New mjs case pins "an in-flight response after close does not repaint" (22 -> 23). Verified: build/vet/gofmt; go test ./internal/... -count=1 green; -race monitor+app green; node mjs 23/23; fake-serve orphan delta 0. Pins re-confirmed RED on mutation: post-await guard removed -> in-flight mjs case fails; capped ≥ dropped -> "...must say 'actual is higher'" fails. Exact-row byte-identity proven. tty_metrics untouched; #114 out of scope. --------- Co-authored-by: linletian <linletian@users.noreply.github.com>
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
Validation