Skip to content

refactor(qa): reuse canonical process-counter parsers - #140527

Merged
steipete merged 1 commit into
mainfrom
steipete/cleanup-qa-process-counters
Sep 7, 2026
Merged

refactor(qa): reuse canonical process-counter parsers#140527
steipete merged 1 commit into
mainfrom
steipete/cleanup-qa-process-counters

Conversation

@steipete

@steipete steipete commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related: #135868

What Problem This Solves

QA Lab carries private process-counter parsers that duplicate helpers already provided by its existing Plugin SDK import. This is maintainer-requested cleanup for the #135868 split campaign and extracts no feature content from that PR.

Why This Change Was Made

Use the canonical positive/nonnegative integer parsers for Windows process IDs and the canonical finite-number sign check for CPU/RSS counters. Only the private absence checks change from null to undefined; the public readers still return number or null.

Before this change, the complete owner matches stable v2026.9.2. The deleted helpers have only local callers, and the existing SDK barrel already exports their replacements. No compatibility, process-ownership, SDK, configuration, or storage contract is removed.

User Impact

No intended behavior change. QA metrics retain strict decimal safe-integer process IDs, fractional and large finite counters, accepted exponent-form metrics, zero values, rounding, and descendant aggregation. Subprocess commands, trusted PowerShell selection, timeouts, and POSIX parsing remain unchanged.

Evidence

  • All 39 unchanged tests passed before and after with node scripts/run-vitest.mjs: Windows process snapshots, POSIX CPU/RSS parsing, and canonical number coercion.
  • Independent Codex local and committed-branch reviews found no actionable P0–P2 issues.
  • Temporary public-boundary comparison passed 2,526 CPU/RSS comparisons across 1,263 synthetic snapshot/root cases. Complete old and actual new owners use real SDK parsers and Windows path resolution; only child-local spawnSync is stubbed. Values, null results, propagated errors, and exact command/args/options match for Windows/Linux/macOS inputs. Mutants with permissive PID parsing and falsy-zero loss are detected. This is synthetic parser-boundary proof, not native OS integration.
  • Full node scripts/check-changed.mjs plan passed in 329.66 seconds on clean Linux Blacksmith Testbox (run 34067942680, lease tbx_01m1wj0d2r51fzz2rkta0gs3gp, stopped). Extension and extension-test types, lint, Doctor contract checks, zero dead exports and zero runtime cycles passed. All nine recorded inputs remained unchanged through the main rebase; no lockfile change.

Production +14/−33, net −19; one private plugin file, 270→251 lines. Tests, tooling, docs, and baselines unchanged. No build, packaging, dynamic import, new SDK path, or published API change.

ClawSweeper completed its review of the same patch with no correctness or security findings and no before-merge/rank-up requests. Initial exact-head CI 34068374981 passed (35 successful jobs, 16 expected skips). The final landing rebase only picked up unrelated agent-turn test fixtures; all nine proof inputs/lock stayed unchanged, all 39 tests passed again, and fresh branch review was clean. Final-head CI is required before merge.

Final exact-head CI 34068805999 is green on a320362cb8563a7cc875de376e69f94e2c480d3d: 35 successful jobs, 16 expected skips; canonical watcher GREEN. No test retry or source repair was needed.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@steipete
steipete marked this pull request as ready for review September 6, 2026 23:58
@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 7, 2026
@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 8:13 PM ET / September 7, 2026, 00:13 UTC.

ClawSweeper review

What this changes

QA Lab replaces private Windows process-counter parsers with existing Plugin SDK helpers while preserving CPU and memory measurements.

Merge readiness

✅ Ready for maintainer review

Keep open as useful, maintainer-requested cleanup that remains absent from current main. The reviewed patch preserves behavior, with no actionable correctness or security findings.

Priority: P3
Reviewed head: a320362cb8563a7cc875de376e69f94e2c480d3d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, behavior-preserving consolidation with useful supplemental validation and no actionable findings.
Proof confidence 🌊 off-meta tidepool Not applicable: The maintainer-authored cleanup is exempt from external-contributor runtime proof and changes no authority boundary; reported synthetic comparisons exercise the changed metric readers but do not establish native OS integration.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The maintainer-authored cleanup is exempt from external-contributor runtime proof and changes no authority boundary; reported synthetic comparisons exercise the changed metric readers but do not establish native OS integration.
Evidence reviewed 9 items Pinned introduced change: The verified base-to-head diff replaces three private parsers and updates their absence checks. Public null results, rounding, aggregation, subprocess arguments, and timeouts remain unchanged.
Equivalent canonical parser contracts: The integer helpers delegate to the same strict safe-integer parser and apply equivalent sign checks. The finite-number guard preserves zero, negative zero, fractions, and large finite values after strict numeric parsing; invalid values become undefined and are translated back to the existing public null result.
Existing SDK boundary: The existing number-runtime barrel already exports all replacement helpers from the repository-owned normalization package. No SDK API, package export, dependency, or Codex runtime contract changes.
Findings None None.
Security None None.

How this fits together

QA Lab samples the Gateway process and its descendants during testing. Its process-tree reader converts operating-system counters into aggregate CPU time and memory usage.

flowchart LR
  A[Gateway process ID] --> B[Process snapshot reader]
  C[Windows process counters] --> B
  D[POSIX process counters] --> B
  B --> E[Validate IDs and measurements]
  E --> F[Sum process descendants]
  F --> G[QA CPU and memory metrics]
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Source -19. Total -19 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 14 33 -19
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 14 33 -19

Review metrics

None.

Technical review

Best possible solution:

Use the existing SDK parsers as the single numeric-validation contract while retaining QA Lab's public metric semantics.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR removes duplicate parsing logic without claiming a user-visible defect; source comparison supports equivalent behavior.

Is this the best way to solve the issue?

Yes. Reusing already-exported SDK helpers removes duplication without expanding the API or changing accepted inputs and public results.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 4a03450435fe.

Labels

Label justifications:

  • P3: This is a bounded internal cleanup with equivalent metric behavior and no new user-facing surface.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-authored cleanup is exempt from external-contributor runtime proof and changes no authority boundary; reported synthetic comparisons exercise the changed metric readers but do not establish native OS integration.

Evidence

What I checked:

  • Pinned introduced change: The verified base-to-head diff replaces three private parsers and updates their absence checks. Public null results, rounding, aggregation, subprocess arguments, and timeouts remain unchanged. (extensions/qa-lab/src/process-tree-cpu.ts:65, a320362cb856)
  • Equivalent canonical parser contracts: The integer helpers delegate to the same strict safe-integer parser and apply equivalent sign checks. The finite-number guard preserves zero, negative zero, fractions, and large finite values after strict numeric parsing; invalid values become undefined and are translated back to the existing public null result. (packages/normalization-core/src/number-coercion.ts:284, a320362cb856)
  • Existing SDK boundary: The existing number-runtime barrel already exports all replacement helpers from the repository-owned normalization package. No SDK API, package export, dependency, or Codex runtime contract changes. (src/plugin-sdk/number-runtime.ts:6, a320362cb856)
  • Current-main necessity: Fetched main still contains the private positive-integer, nonnegative-integer, and nonnegative-number helpers. The requested consolidation is therefore still outstanding. (extensions/qa-lab/src/process-tree-cpu.ts:16, 4a03450435fe)
  • Existing behavioral coverage: Inspected unchanged Windows and POSIX tests covering public CPU/RSS readers, descendant aggregation, malformed counters, strict IDs, command selection, and bounded probes. Shared parser tests cover zero, fractions, exponent-form counters, and unsafe integers. No tests were executed during this read-only review. (extensions/qa-lab/src/process-tree-cpu.windows.test.ts:23, a320362cb856)
  • Reported validation and review continuity: The supplied body reports 39 unchanged tests passing and 2,526 synthetic public-reader comparisons using actual parsers with stubbed spawnSync. This is supplemental parser-boundary evidence, not native integration proof. The previous completed review retained no findings or rank-up requests; its earlier head was unavailable locally for comparison. (a320362cb856)

Likely related people:

  • Vincent Koc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-07T00:05:36.854Z sha 3a77491 :: needs maintainer review before merge. :: none

@steipete
steipete force-pushed the steipete/cleanup-qa-process-counters branch from 3a77491 to a320362 Compare September 7, 2026 00:07
@steipete steipete self-assigned this Sep 7, 2026
@steipete
steipete merged commit 8049794 into main Sep 7, 2026
112 checks passed
@steipete
steipete deleted the steipete/cleanup-qa-process-counters branch September 7, 2026 00:16
@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Sep 7, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 7, 2026
@steipete
refactor(qa): reuse canonical process-counter parsers (openclaw#140527)
b3514c2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

extensions: qa-lab maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete