chore: address nits from PR #261 review#267
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efb74bc7e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if after.endswith("/") or Path(after).suffix == "": | ||
| return after.rstrip("/") + "/" |
There was a problem hiding this comment.
_scope_path_to_skill_pattern now treats any skills/... path with no suffix as a directory and appends /, which misclassifies valid file paths like .claude/skills/code/README as code/README/. In --check mode this creates false out-of-scope failures because _is_allowed will only match descendants, not the file itself. The regression appears whenever a TODO scope lists an extensionless file path, so the audit can block otherwise-correct skill-sync.lock updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Follow-up sweep actioned this PR review comment.
Disposition: fixed
Branch: `chore/pr-review-followups-since-2026-05-05`
Summary:
Disposition: fixed
Evidence: Current _project/scripts/skill_sync_lock_audit.py still had the flagged behavior. I changed it to derive directory prefixes from lock metadata, and added regression coverage in test_skill_sync_lock_audit.py for .claude/skills/code/README as an exact extensionless file.
Verification: uv run -- python -m pytest tests/unit/scripts/test_skill_sync_lock_audit.py -q; uv run -- ruff check _project/scripts/skill_sync_lock_audit.py tests/unit/scripts/test_skill_sync_lock_audit.py
Future sweeps skip comments that already have this marker reply.
joeharris76
force-pushed
the
chore/pr261-review-nits-2026-05-07
branch
from
efb74bc to
36babef
Compare
…https://img.shields.io/badge… Disposition: fixed Source: #250 (comment) Path: benchbox/core/tuning/coverage.py
- tests/uat/matrix.py: drop line-range from `local_stress_test.sh` citation so the comment doesn't drift as that script evolves. - results-explorer/src/components/Tabs.tsx: trim `tabRefs.current` to the current items length so stale refs from prior renders don't linger when items shrinks.
Develop renamed static_regressions to static_matrix_drift in #265 and added an equivalent test for the new-rows-missing case. Drop the stale duplicate test that referenced the old name.
joeharris76
force-pushed
the
chore/pr261-review-nits-2026-05-07
branch
from
36babef to
6aa1e2c
Compare
* fix(pr-followup): PR #245 comment 3201562602 — **<sub><sub> Path: tests/uat/_cli.py * fix(pr-followup): PR #245 comment 3201562610 — **<sub><sub> Path: tests/uat/phases/preflight.py * fix(pr-followup): PR #246 comment 3202118174 — **<sub><sub> Path: _project/audits/screenshots/results-explorer-hierarchy-usability-data-audit-20260507/screenshot-manifest.json * fix(pr-followup): PR #255 comment 3202201883 — **<sub><sub> Path: tests/uat/phases/explorer_smoke.py * fix(pr-followup): PR #256 comment 3202212814 — **<sub><sub> Path: results-explorer/src/pages/BenchmarkIndex.tsx * fix(pr-followup): PR #266 comment 3204995419 — **<sub><sub> Path: results-explorer/src/lib/facetDisplay.ts * fix(pr-followup): PR #267 comment 3205001733 — **<sub><sub> Path: _project/scripts/skill_sync_lock_audit.py --------- Co-authored-by: Joe Harris <57046+joeharris76@users.noreply.github.com>
1 participant
Summary
Two nit fixes surfaced while reviewing #261; both touch code added by that PR, so this branch stacks on
chore/pr-review-followups-2026-05-07.tests/uat/matrix.py— drop the:142-161line-range from theget_platform_portcomment citation. Line numbers drift aslocal_stress_test.shevolves; the function name alone is a stable reference.results-explorer/src/components/Tabs.tsx— settabRefs.current.length = items.lengthso stale entries from prior renders don't linger when the items array shrinks. Read-only impact today (refs are accessed by index), but keeps invariants tight.Stacking note
Until #261 merges, this PR will show all of #261's commits plus one cleanup commit on top. Once #261 lands on
develop, GitHub auto-rebases this PR to the single cleanup commit. Suggested merge order: #261 → this PR.Test plan
npm test -- --run primitives.test— 21/21 pass (Tabs.tsx ref-length-trim is safe)uv run -- python -m pytest tests/uat/test_preflight.py -n 0 -q— 6/6 passuv run -- ruff check tests/uat/matrix.py— cleannpx tsc --noEmit(results-explorer/) — clean