fix(engine): correct Cartfile.resolved regex and cover it with parity checks - #4638
Merged
Merged
Conversation
… checks The gittensory-engine package's diffFilePriority copy matched `cartfile\.lock` (not a real Carthage filename) instead of `cartfile\.resolved`, so the engine's predicted-gate collision check never deprioritized a touched Cartfile.resolved the way the host copies (review-diff.ts, review-grounding.ts) already did. Also generalizes check-engine-parity.ts's gate-decision named-pair escape hatch into a reusable NAMED_TWIN_PAIRS list and adds pairs for safe-url.ts (nested under content-lane/ on the host, so the top-level directory scan never sees it) and the diffFilePriority/ sharesMeaningfulFile functions (duplicated by function rather than by file, so no filename match exists either). Each pair is checked for marker presence in engine-parity:drift-check, including a marker on the exact cartfile\.resolved literal so this regression is caught going forward. Deferred: making listDir recursive so nested duplicates are discovered automatically rather than needing an explicit named pair. The safe-url.ts case wouldn't even be solved by recursion alone (the host and engine copies sit at different relative depths), so this is left as a follow-up rather than risking a broader directory-scan behavior change in this fix.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gittensory-ui | 110895a | Commit Preview URL Branch Preview URL |
Jul 10 2026, 08:42 AM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4638 +/- ##
=======================================
Coverage 94.11% 94.11%
=======================================
Files 432 432
Lines 38370 38370
Branches 13989 13989
=======================================
Hits 36113 36113
Misses 1600 1600
Partials 657 657
🚀 New features to boost your workflow:
|
This was referenced Jul 10, 2026
JSONbored
added a commit
that referenced
this pull request
Jul 10, 2026
discoverEngineParityPairs only listed the immediate children of each
src/{review,settings,signals} area directory, so a hand-duplicated pair
nested one directory deeper on both sides was invisible to the scan by
construction -- even though the existing shim/stub filters would have
handled it correctly once discovered. #4605's own Finding 1 (the
Cartfile.resolved regex typo) was found by manual audit rather than this
tripwire precisely because that class of duplicate lives in files the
directory-only scan can't see.
collectTsFilesRecursive walks the tree via the same pluggable listDir(root,
relativePath) shape the function already accepted, treating a `.ts`-suffixed
entry as a leaf and any other entry as a directory only if listDir on it
returns at least one entry -- reusing defaultListDir's existing
resolve-to-[] convention for non-directories rather than requiring a stat
call. Pairing is now by identical relative sub-path on both sides, not bare
filename, so a depth MISMATCH (e.g. safe-url.ts nested under
content-lane/ on the host but flat on the engine) still correctly falls
through to its NAMED_TWIN_PAIRS entry rather than silently going
undiscovered OR getting falsely paired with an unrelated same-named file
elsewhere in the tree.
Completes #4605's third acceptance criterion (PR #4638 shipped the other
two: the regex fix itself and named-pair coverage for the specific
already-known nested/renamed duplicates).
Part of #4605
21 tasks
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
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
packages/gittensory-engine/src/review/diff-file-priority.ts'sdiffFilePrioritymatchedcartfile\.lock— not a real Carthage filename — instead ofcartfile\.resolved. The two host copies (src/review/review-diff.ts,src/review/review-grounding.ts) already matched the correct filename, so the engine package's predicted-gate collision check silently disagreed with the host for Carthage/iOS repos.scripts/check-engine-parity.ts's directory scan only pairs identical top-level filenames withinsrc/{review,settings,signals}, so it can't see a duplicate that's nested one directory deeper or duplicated by function rather than by file. Generalized the existingGATE_DECISION_TWIN_PAIRescape hatch (fix(engine): require a gittensory-engine version bump on gate-decision logic changes and extend the parity scan to advisory.ts/gate-advisory.ts #4518) into a reusableNAMED_TWIN_PAIRSlist and added named pairs for:safe-url.ts(host copy lives undersrc/review/content-lane/, engine copy is flat underpackages/gittensory-engine/src/review/— different depths, so the scan never lists both under the same relative path).diffFilePriority(host:review-diff.ts; engine:diff-file-priority.ts— different filenames, so no scan match exists at all). Its marker set includes the literalcartfile\.resolvedregex fragment so this exact regression is caught if it ever recurs.sharesMeaningfulFile(host:src/signals/engine.ts; engine:packages/gittensory-engine/src/signals/predicted-gate-engine.ts), which both gate collision-detection ondiffFilePriority's threshold.engine-parity:drift-checknow reports 21 checked pairs (17 directory-discovered + 4 named), up from 18 (17 + 1) before this PR.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Closes #4605
Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateRan the full
npm run test:cigate locally (this touches shared parity-checking infrastructure used by CI itself, so it warranted the fuller local pass rather than just the touched-file scope) — all green, includingtest:engine-parity,engine-parity:drift-check, and the engine package's ownnpm run test --workspace @jsonbored/gittensory-engine. Added a direct regression test inpackages/gittensory-engine/test/diff-file-priority.test.tsthat reproduces the original bug (assertsdiffFilePriority("Cartfile.resolved")andsharesMeaningfulFile's Cartfile.resolved collision case) — verified this test fails without the regex fix and passes with it. Also added anamed twin-pair coverage (#4605)test block totest/unit/check-engine-parity-script.test.tscovering the new pairs, including a synthetic case proving the missing-marker detection actually fires. Neitherscripts/**norpackages/gittensory-engine/src/**is in Codecov'spatchcoverage scope, so this PR owes nocodecov/patchobligation, but both new test suites are exercised bynpm run test:ciregardless.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. (N/A — no visible UI changes.)UI Evidence
N/A — no UI/frontend/docs changes in this PR.
Notes
discoverEngineParityPairs'slistDirrecursive. Named-pair coverage for the two known-missing pairs is must-have and included; full recursion is left as a follow-up so it can be verified against the whole directory tree without risking an unintended behavior change in this focused fix. Note also that recursion alone would not have discoveredsafe-url.ts's pairing anyway, since the host and engine copies sit at different relative depths (content-lane/safe-url.tsvssafe-url.ts) — a name-only recursive match wouldn't line them up without also relaxing the match key, which is a larger design decision better suited to its own PR.src/review/review-diff.tsandsrc/review/review-grounding.tsboth define byte-identical copies ofdiffFilePriorityon the host side. That's an intra-host duplication, not a host/engine drift, so it's outsidecheck-engine-parity.ts's purpose (which only tripwires host vs. published-engine-package drift) — flagging it here rather than folding an unrelated dedup into this fix.