test: cover path-utils resolve/join/slug branches - #56
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed September 8, 2026, 7:00 AM ET / 11:00 UTC. ClawSweeper reviewWhat this changesAdds nine tests for path resolution, required-path errors, separator normalization, path joining, and artifact-name formatting. Merge readiness⛔ Blocked before merge - 3 items remain The focused coverage remains useful and is not present on current main. No actionable patch defect was found; the previously identified test-only proof exception remains unresolved. Priority: P3 Review scores
Verification
How this fits togetherPlugin Inspector checks OpenClaw plugin compatibility and produces reports and execution plans. Its shared path helpers turn configured paths and entrypoint names into consistent report locations and artifact identifiers. flowchart LR
A[Configured paths] --> C[Shared path helpers]
B[Entrypoint names] --> C
C --> D[Report locations]
C --> E[Execution plan artifacts]
F[Nine regression tests] --> C
Decision needed
Why: The external-PR proof rule exempts docs-only changes but not test-only changes; the reviewer cannot grant that exception. Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Retain the focused contract tests and resolve their proof gate through an explicit authorized test-only exception. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds coverage for existing behavior rather than reporting a runtime defect; the contributor supplies passing tests and mutation controls. Is this the best way to solve the issue? Yes: direct assertions against the existing helpers are a narrow, maintainable way to protect their contracts, without another implementation or dependency. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 92db8c57e1d5. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (91 earlier review cycles; latest 8 shown)
|
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
What Problem This Solves
src/path-utils.jsexports six shared path helpers. Existing workspace-plan and profile-diff consumer tests already execute the module, but there is no dedicatedtest/path-utils.test.json main. These tests add direct branch coverage for labeled required-path errors, relative/absolute paths, separator normalization, falsy path segments, repeated slashes, and artifact slug trimming.Why This Change Was Made
Coverage-only: one new file,
test/path-utils.test.js(+64/-0), with nine tests against the real exported helpers and no stubs or production changes. The assertions protect existing path and artifact contracts without adding configuration, dependencies, report fields, or runtime behavior.Original contribution by @KrasimirKralev. The contributor's branch and exact head
8e79692ee54c6023c99633c72a380020f22db926remain unchanged.User Impact
No user-visible or runtime change. Maintainers gain focused regression checks for existing path-helper behavior. No changelog, release, downstream pin update, or external runtime integration is needed for this tests-only diff.
Evidence
Fresh Maintainer Verification
84ede904fd6e766a9fc4de002f39af87d90c1916.Secret source: Noneand read-only Contents/Metadata token permissions. No fork code was executed locally.src/path-utils.jsis byte-identical at the PR base, contributor head, and current main. The merge preview adds only the original 64 test lines.git diff --checkpassed.The original August 3 CI run could no longer be rerun because it was over a month old. Closing and reopening this same PR triggered the fresh run without editing the contributor branch.
Original Contributor Evidence
The contributor reported Linux/Node22.22 validation at PR base
392cc7c8d6419965bb351584aaa47a50abd2ebce: nine focused tests passed, and the then-current full suite increased from 234 to 243 passing tests. These are historical counts, not the fresh current-main totals above.The contributor also reported mutation controls, restored byte-identically afterward:
path-utils.jsposixJoinfalsy filteringposixJoinslash collapsingThe mutation controls are contributor-reported historical evidence, not local maintainer execution.