test(feature-unification-scope): add tests + package.json with type deps#1833
test(feature-unification-scope): add tests + package.json with type deps#1833ozgb wants to merge 2 commits into
Conversation
Assisted-by: Claude:claude-4.8-opus Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
Merge closely-related assertions into single test() blocks and drop the reverseClosure unit tests, whose coverage was already fully subsumed by the computeScope end-to-end tests. No behaviour coverage is lost: 38 tests become 19 with the same set of distinct branches exercised. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
![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: 6409fd7873
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "dependencies": { | ||
| "smol-toml": "^1.6.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^22.18.0" |
There was a problem hiding this comment.
With the npm manifest moved from /scripts into scripts/feature-unification-scope, the existing .github/dependabot.yml entries (/ and /scripts) no longer cover these dependencies. GitHub’s Dependabot docs say directory is a single manifest directory and directories is needed for multiple locations, so smol-toml/@types/node version updates will stop for this CI tool unless you add /scripts/feature-unification-scope or switch to a directories entry.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,302 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
The root AGENTS.md requires the LICENSE_HEADER.txt header on all new source files, but this newly added TypeScript source starts without the SPDX/Apache header (after the shebang for an executable); scope.test.ts has the same omission. Please add the standard header so the new source files comply with the repository policy.
Useful? React with 👍 / 👎.
m2ux
left a comment
m2ux
left a comment
There was a problem hiding this comment.
PR Review Summary
PR: #1833 — test(feature-unification-scope): add tests + package.json with type deps
Reviewers: Code Review Agent · Test Suite Review Agent · Validation Agent · Strategic Review Agent
Reports: Prior Feedback Triage · Code Review · Test Suite Review · Validation · Strategic Review
Branch / Commit reviewed: ozgb-feature-unification-script-tests @ 6409fd78 (base giles-fast-feature-unification)
Date: 2026-07-07
Executive Summary
PR #1833 relocates the feature-unification scoper into its own directory, adds a colocated npm manifest for LSP/type support, and introduces a 19-test suite exercising every computeScope() decision-tree branch. Algorithm parity with the baseline monolithic script is confirmed — the refactor is testability-only, with layout-aware IGNORE and CI wiring updated consistently. Unit tests pass locally (19/19 under Node v22.23.1) and Feature Unification Check passes in CI.
Four other CI jobs fail on head; attribution confirms none are introduced by this PR's scoper/test changes (two pre-existing on the base branch, one infra flake, one process gap). Remaining items are P2 policy hygiene (Dependabot directory, license headers), optional test-layer leanness, and PR checklist housekeeping.
Overall Rating: Comment Only (no critical blockers; algorithm and test coverage sound)
Prior Feedback Triage
| # | Prior Comment | Author | Disposition | Reasoning |
|---|---|---|---|---|
| 1 | Four pipeline jobs failed @ 6409fd78 |
datadog-official | Confirmed (non-blocker) | Valid CI signal; Feature Unification Check passes; audit npm and genesis rebuild fail identically on base branch; local-env failure is GHCR timeout; check-changes is missing change file |
| 2 | Automated Codex review posted, no blocking summary | chatgpt-codex-connector | Refuted | Informational review shell only — no actionable blocker in review body |
| 3 | P2: Add /scripts/feature-unification-scope to Dependabot after manifest move |
chatgpt-codex-connector | Confirmed (P2) | New npm manifest location not covered by existing Dependabot directory entries |
| 4 | P2: Missing LICENSE_HEADER.txt SPDX header on new TypeScript sources |
chatgpt-codex-connector | Confirmed (P2) | Valid repo-policy concern per AGENTS.md — applies to both new .ts files |
Finding Details
No formal human reviews or CHANGES_REQUESTED state. Rating cap unset — no unaddressed blocker-class human comments.
PF-3. Dependabot directory (Confirmed — P2)
Manifest relocated from /scripts/package.json to scripts/feature-unification-scope/package.json; .github/dependabot.yml still lists /scripts only.
PF-4. License headers (Confirmed — P2)
scope.ts and scope.test.ts start with inline comments, not the standard SPDX header block.
Code Review Findings
| # | Finding | Severity |
|---|---|---|
| 1 | LockPkg, MetaPkg, Crate exported but unused outside module |
Medium |
| 2 | Dependabot still lists /scripts only — new manifest uncovered |
Low |
| 3 | SPDX license headers absent on new TypeScript sources | Low |
| 4 | Vacuous EXCLUDED regex loop after empty-scope assert |
Low |
Finding Details
CR-1. Exported interfaces wider than needed (Medium)
LockPkg, MetaPkg, and Crate are exported but no test or external caller imports them; only Meta, ScopeInput, and runtime constants are consumed outside the module.
Suggestion: Drop export from the three internal-only interfaces (zero runtime impact).
CR-2. Dependabot directory not updated (Low — P2 policy)
.github/dependabot.yml#L26-L31 lists /scripts only; root manifest removed and new manifest lives under /scripts/feature-unification-scope/.
Suggestion: Add a Dependabot directory entry in this PR or a follow-up.
CR-3. SPDX license headers absent (Low — P2 policy)
New TypeScript files lack the LICENSE_HEADER.txt SPDX block per repo AGENTS.md. Baseline monolithic scoper also had none — this is a new-file policy gap, not a regression.
Suggestion: Add headers to scope.ts and scope.test.ts.
CR-4. Vacuous assertion in excluded-crate test (Low)
scope.test.ts#L194 loops EXCLUDED against out after asserting out === "" — the regex checks can never fail.
Suggestion: Remove the loop.
Verified correct: Algorithm parity (byte-for-byte equivalent decision tree), lockAffected() refactor preserves production behaviour, IGNORE self-exclusion for relocated directory, main() ESM guard at scope.ts#L302, Earthfile npm ci --omit=dev, workflow path filters, dev/runtime dep split.
Test Review Findings
| # | Gap | Severity |
|---|---|---|
| 1 | main() CLI integration untested (argv + cargo metadata subprocess) |
Low |
| 2 | Helper describe blocks partially duplicate computeScope integration coverage (~94 lines) |
Low |
Finding Details
TR-1. main() CLI integration gap (Low)
The pure-function suite is thorough (19 tests, all decision-tree branches covered — IA-4 validated 38→19 trim preserved coverage). main() argv handling and cargo metadata subprocess paths remain untested. Pre-existing class of gap; not introduced by trim.
Suggestion: Optional follow-up integration test.
TR-2. Redundant helper describe blocks (Low)
owningCrate, reverseReachMembers, lockChanged, parseLock, and lockAffected re-characterize behaviour already asserted via computeScope output strings. Estimated ~94 lines removable without losing branch coverage.
Suggestion: Collapse to order-independence and dep-suffix cases not observable via scope output.
Verified: 19/19 tests pass under Node ≥22.18; Feature Unification Check CI pass confirms integration wiring.
Documentation Review
| # | Gap | Severity |
|---|---|---|
| 1 | Missing season change file — check-changes CI fails |
Low |
| 2 | PR submission checklist largely unchecked (DCO, self-review, testing evidence) | Low |
| 3 | README algorithm summary duplicates scope.ts header comment |
Low |
Finding Details
DR-1. Missing change file (Low — process)
check-changes fails with "No changes found for PR 1833". Not a code defect but blocks CI green and is PR-specific.
Suggestion: Add a season change fragment under changes/ or apply a skip label with justification.
DR-2. Submission checklist incomplete (Low)
DCO sign-off, self-review, AGENTS.md compliance, and testing evidence remain unchecked despite substantive new tests. "Ready" is checked under TODO while submission checklist is incomplete.
Suggestion: Complete checklist items; cite npm test output under Node ≥22.18.
DR-3. README duplication (Low)
README.md#L8-L10 restates algorithm details already in the scope.ts header comment.
Suggestion: Trim to layout + dev/run commands; link to scope.ts header as canonical algorithm doc.
Validation Findings
| # | Check | Severity |
|---|---|---|
| 1 | audit job npm — local-environment/ high-severity audit |
Low |
| 2 | Check Genesis Rebuild — missing Compact compiler artifacts | Low |
| 3 | Local Environment Tests — GHCR wallet-indexer pull timeout | Low |
| 4 | check-changes — no season change file for PR 1833 | Low |
Finding Details
VF-1. audit job npm (Low — pre-existing)
Fails on local-environment/package.json audit — unrelated to scoper manifest. Same failure on base branch giles-fast-feature-unification (2026-06-16).
VF-2. Check Genesis Rebuild (Low — pre-existing)
No Compact versions found. Run fetch-compactc first. — genesis rebuild toolchain gap. Same error on base branch.
VF-3. Local Environment Tests (Low — infra)
GHCR pull timeout on wallet-indexer:062ebfd8fc24 — self-hosted runner flake; unrelated to scoper changes.
VF-4. check-changes (Low — process)
PR-specific missing change file. See DR-1.
Verified passing: Feature Unification Check (job/85373622344), Run tests, Lint workflows, Formatting and Linting. Local npm test: 19/19 under Node v22.23.1.
Action Items
Must Address (Blocking):
- None.
Should Address (Recommended):
- Add season change file or skip label with justification (DR-1, VF-4)
- Complete PR submission checklist — DCO, self-review, testing evidence (DR-2)
- Add Dependabot directory entry for
/scripts/feature-unification-scope(CR-2, PF-3) - Add SPDX license headers to new TypeScript files (CR-3, PF-4)
Could Address (Suggested):
- Un-export internal-only interfaces
LockPkg,MetaPkg,Crate(CR-1) - Remove vacuous EXCLUDED loop in excluded-crate test (CR-4)
- Shrink redundant helper describe blocks (~94 lines) (TR-2)
- Trim README algorithm duplication (DR-3)
Nice to Have (Optional):
- Add
main()CLI integration test (TR-1)
Review conducted via work-package workflow (review mode). Artifacts under .engineering/artifacts/planning/2026-07-07-review-midnight-node-pr-1833-feature-unification-script-tests/.
| // current workspace, i.e. inside the check container). Prints the package | ||
| // selection args for `cargo hack check --no-dev-deps`: | ||
| // | ||
| // * "-p a -p b ..." only these crates and their reverse-dependency |
There was a problem hiding this comment.
Is "reverse-dependency" a term for dependent crates?
There was a problem hiding this comment.
I propose:
"only these crates and crates (transitively) dependent on them need re-checking"
With all my love to math, lets leave closures to topologicans.
3 participants
Overview
Adds tests and a package.json with correct dependencies so typescript LSPs work when editing the scope script.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links