Skip to content

Pass 4d — Mesh + presentation/misc lane duplication audit #388

Closed
2 of 2 issues completed
Closed
Pass 4d — Mesh + presentation/misc lane duplication audit#388
2 of 2 issues completed
Labels
phase:4dPass 4d — Mesh + presentation/misc lane duplication audit (#388)priority:P3Low / somedayrefactorPart of the #377 codebase/docs duplication-audit efforttype:epicLarge effort tracked via sub-issues

Description

@gsdali
gsdali
opened on Jul 26, 2026

Part of the segmented duplication audit in #377. Passes 2a (#382), 2b (#383) and 3 (#384) are the prerequisites — check they are closed before starting.

This lane: meshing plus the Metal-visualization presentation and misc utilities.

Step 1 — Re-derive the scope. The list below is already stale.

The previous version said ~2,173 LOC. Measured 2026-08-19 it is 2,345; MeasurementHelpers.swift alone went 225 → 375.

wc -l Sources/OCCTSwift/{Mesh,MeasurementHelpers,KDTree,PresentationMesh,ZLayerSettings,ClipPlane,Camera,DisplayDrawer,PixMap,FontManager,Date,OCCTSerialQueue}.swift

This lane has the only confirmed detector hits of the four Pass 4 lanes. Measured 2026-08-19, detect-duplicate-logic.py --swift reports six candidate pairs repo-wide and two are yours:

pair score shared shingles
PresentationMesh.swift:45 / :120 — both shadedMesh 0.93 172
PresentationMesh.swift:85 / :158 — both edgeMesh 0.90 111

Two same-named overloads in one 185-line file, each near-duplicating its sibling. Start there. It is the highest-value known lead in any Pass 4 lane, and the other three lanes return nothing from this detector at all.

Files

This exact format is machine-read. .claude/workflows/duplication-audit.js parses this section and expects one backtick-wrapped repo-relative path per line with a LOC count in parentheses. Keep the shape when you update the numbers, or the workflow aborts with "Could not resolve a file scope".

  • Sources/OCCTSwift/Mesh.swift (723)
  • Sources/OCCTSwift/MeasurementHelpers.swift (375)
  • Sources/OCCTSwift/PresentationMesh.swift (185)
  • Sources/OCCTSwift/ZLayerSettings.swift (184)
  • Sources/OCCTSwift/ClipPlane.swift (166)
  • Sources/OCCTSwift/Camera.swift (159)
  • Sources/OCCTSwift/KDTree.swift (110)
  • Sources/OCCTSwift/PixMap.swift (108)
  • Sources/OCCTSwift/Date.swift (106)
  • Sources/OCCTSwift/DisplayDrawer.swift (102)
  • Sources/OCCTSwift/OCCTSerialQueue.swift (64)
  • Sources/OCCTSwift/FontManager.swift (63)

The bridge side is Sources/OCCTBridge/src/OCCTBridge_Mesh.mm and OCCTBridge_Visualization.mm.

This method section is a deliberate copy. The canonical version lives in #377. It is
duplicated into every pass issue on purpose: a linked document does not get read, and this
programme has already lost work to instructions nobody followed. If you change the method, change
#377 and the sibling passes too. This is the one place in this repo where duplication is correct.

If you are picking this up cold, start here

Written for someone with no prior context. Work through it in order.

What a "pass" is. #377 splits one repo-wide duplication sweep into thirteen per-layer passes. Each audits a fixed set of files for duplicated logic, files one sub-issue per confirmed finding, fixes them, and closes.

What "duplication" means here. Not just identical text. Four angles: a helper reimplemented under a second name, copy-pasted maths that has drifted, doc comments that no longer describe the code below them, and two parallel types that should be one. In this repo these are frequently live bugs. Pass 2a found Shape.fixed(...) silently discarding three parameters and two tolerance defaults drifted apart for the same question; Pass 3 found six call sites that had lost a null guard their siblings kept.

Behaviour fixes are in scope. If a duplication has a real defect on one side, fix it here. See #377's "Scope: behavior fixes are in scope".

Step 0 — Read the policies before you touch anything

Working rules live in okf/policies/. Read these five now:

policy why it binds you here
measure-dont-assume.md This repo's most-repeated failure is a census built by grep that was wrong. Do not trust any count below, including the file list.
search-before-building.md The point of the pass. Before adding a helper, find the one that exists.
issue-tracking.md Every issue needs type:* and priority:*. Pass 3 filed three issues with zero labels because the author wrote **Type:** chore in body prose instead of applying the label.
changelog-on-merge.md Your PR carries its CHANGELOG entry in the body, never in the diff.
prove-the-test-fails.md Every new test is run once with its subject broken, and you report that it failed.

Read at the step that binds them: semver-at-release.md (never edit docs/SEMVER.md in a PR — missed twice, #829 and #943, both shipping an incomplete break table), code-style.md, docs-current.md, writing-style.md, code-structure.md.

okf/index.md lists the rest. CLAUDE.md at the repo root has a long "Known OCCT Bugs" section — read the entries for this lane.

Step 2 — Branch

git fetch origin
git checkout -b refactor/388-pass4d origin/main

From origin/main, not local main (may be stale), not refactor/377-segmented-audit (dead since 2026-07-29).

One branch, not two. Pass 2a used a single branch merged straight to main. Pass 3 grew a second -integration branch with a PR between them, an extra layer nothing requires. If you do use one, issue-tracking.md says a merge into it is not Done on the board — cards stay at Code-Review until it reaches main.

Step 3 — Run the duplication detector

python3 Scripts/repro/784-duplication-rescan/detect-duplicate-logic.py --self-test   # always first
python3 Scripts/repro/784-duplication-rescan/detect-duplicate-logic.py --swift
python3 Scripts/repro/784-duplication-rescan/detect-duplicate-logic.py --bridge

--self-test every time. Three gate scripts here were confidently wrong because the detector had gone blind while still reporting "all clear" (#618, #624/#630, #626).

A clean detector run finishes Step 3, not the audit. It compares function bodies and is blind to angles 3 and 4 entirely. Pass 3's declared scope returned zero hits and the pass still produced five findings.

Step 4 — Run the audit workflow

.claude/workflows/duplication-audit.js, invoked with this issue number (388) as its argument. It reads the ## Files section, bin-packs it, and runs the four angles with an adversarial verify stage. Not the generic code-review workflow — that reads a diff and this pass has none.

It produces false positives. Nobody has measured this workflow's rate; the sibling over-coverage detector built in #928 measured 41% false over a 40-row hand-adjudicated sample. Expect the same order. Adjudicate every candidate against the real code.

Step 5 — Triage into sub-issues

One sub-issue per confirmed finding. Pass 2a produced 26, Pass 2b 13, Pass 3 five over a much smaller lane. .claude/workflows/duplication-triage.js drafts bodies from {parentIssue: 388, findings: [...]}; it deliberately does not create them, so each filing stays auditable.

A draft body must carry four sections. Pass 3's first three issues shipped with two and had to be backfilled:

  1. Duplication site — every file:line with the symbol at each.
  2. Divergence — the concrete behavioural or doc difference already observed, or an explicit statement that the copies still agree.
  3. Tests — coverage found at each location by file and suite name, or an explicit "no coverage found" per location.
  4. OCCT functions — the specific OCCT C++ class and method each side invokes, not just the C bridge function name.

Creating and labelling

gh issue create \
  --title "<specific, standalone title>" \
  --body-file draft.md \
  --label "type:chore,priority:P3,refactor,phase:4d"

Link each as a sub-issue — note -F, not -f; the field is numeric and -f fails with a confusing 422:

ID=$(gh api repos/SecondMouseAU/OCCTSwift/issues/<new-number> --jq '.id')
gh api -X POST repos/SecondMouseAU/OCCTSwift/issues/388/sub_issues -F sub_issue_id=$ID

Board: OCCTSwift Refactor (#377). Move cards at the moment the workflow visits them, not in a later sweep.

Step 6 — Fix

One PR per sub-issue, or one per tight cluster touching the same file.

  • Closes #<n> — repeat the keyword per issue. Closes #1, #2 closes only the first, and several issues here stayed open after their fix merged because the title said fix(#N) with no keyword.
  • ## CHANGELOG entry in the body.
  • ## SemVer impact in the body. Breaking? Say so there and do not touch docs/SEMVER.md.
  • Tests, each proven to fail against the unfixed code, and say so.

Gates before pushing — gate-scripts is the one required check on main:

python3 Scripts/check-bridge-index.py
python3 Scripts/check-null-handle-guards.py
python3 Scripts/check-docs-defaults.py
python3 Scripts/check-docs-existence.py
python3 Scripts/derive-bridge-header-split.py --verify
python3 Scripts/count-operations.py
python3 Scripts/census-unmeasured-values.py
python3 Scripts/census-doc-occt-attribution.py
python3 Scripts/check-changelog-transcription.py
python3 Scripts/check-style-manifest.py --base origin/main

gate-scripts is not the whole story. code-style is a separate required-in-practice job, and an
empty manifest makes it stricter, not weaker. See "Code style" below before you push anything.

Code style: the manifest being empty does not mean CI passes itself

New code has been failing the code-style CI job even with both manifests empty. That is not a
contradiction, it is what an empty manifest actually does: CI's swift-format lint --strict step
runs over "every file under Sources/OCCTSwift minus the manifest"
(.github/workflows/code-style.yml), and an empty manifest means that set is now the whole tree,
not just the files this PR touches. A latent violation anywhere in Sources/OCCTSwift can turn this
pass's PR red even if the diff never goes near it.

The pre-commit hook does not catch this either, by design: it runs the bridge clang-format check but
deliberately not swift-format or swiftlint (see the hook's own header comment, Scripts/git-hooks/pre-commit),
so a Swift-side style violation is currently push-and-find-out no matter how clean the manifest is.
Run CI's own commands yourself before pushing, so the first read of them isn't a red check:

swift-format format -i --configuration .swift-format <files you changed>
swift-format lint --strict --configuration .swift-format --recursive Sources/OCCTSwift
swiftlint lint --strict --config .swiftlint.yml

format -i fixes the mechanical stuff (indentation, spacing, blank lines) in place. lint --strict
is what actually gates, and reports things -i will not fix for you: fileScopedDeclarationPrivacy
(a file-scope declaration must be explicitly private) and orderedImports are the two that most
often surprise a first-time contributor here. Run lint over the whole tree as shown, not just your
changed files, since that's what CI does.

For the bridge side (only relevant if this pass touches a Sources/OCCTBridge file):

Scripts/format-bridge.sh          # rewrites in place
Scripts/format-bridge.sh --check  # what CI actually runs

Doc comments: stay to one sentence plus only the Parameter/Returns/Throws tags that add
something the summary doesn't; design rationale and examples belong in docs/, not in the comment.
Scripts/comment-ratio-check.py reports (never fails) a file whose comment lines outnumber its code
lines, run it as a signal. A verbose doc comment is a findable style regression here, not a matter of
taste.

Step 7 — Close the pass

Close when every sub-issue is closed and you have answered the scope question in writing. Post a closing comment recording which files you claimed and which you handed on, anything found and deliberately not fixed with the reason, and any candidate that proved not real so the next pass does not re-investigate it. Pass 2a's closing comment is the model.

Traps this repo has actually hit

Where you put an extracted helper is a correctness decision, not a style one. Decide by reach: count every site with that logic, across every file, before choosing. A static helper in a .mm is confined to that translation unit, so a copy in another .mm can never converge on it and drifts independently.

This has shipped a defect twice in three days. occtComputeBoundingBox was file-static in OCCTBridge_Topology.mm, which is why OCCTShapeGetBounds in OCCTBridge_Properties.mm was the one bounds entry point with no IsVoid() guard (fixed 2026-08-17, #943, by moving it to OCCTBridge_Internal.h as inline). Then Pass 3's own #949 made occtDocumentInit file-static in OCCTBridge_Document.mm while twelve copies sat in OCCTBridge_IO.mm, six already missing a null guard — that is #957.

Before extracting: grep -rn '<the distinctive call>' Sources/OCCTBridge/src/*.mm. More than one file → the helper goes in OCCTBridge_Internal.h as inline. Sites that are not identical are where the bug usually is; record the difference rather than flattening it.

The style manifest is a one-way ratchet. A file you touch must be brought fully clean and removed from Scripts/style-manifest-{swift,bridge}.txt in that same PR, and check-style-manifest.py refuses to let anything back on. Removing a file without actually making it clean just moves the failure to the linter, which is how main sat red for five merges (#942). OCCTBridge_Modeling.mm is the most format-sensitive file in the repo — check the manifest before touching it.

A green test suite is not coverage. Some tests are gated on OCCTSWIFT_LOCAL=1 and silently skip in CI, and the suite reports the same total either way — only the per-test line distinguishes them. If you rely on a test to prove something, confirm from the log that it started.

swift-format diagnostics strip inline code spans. A doc comment reading Split `self` by `tool`. is quoted back as "Split by .". The doubled spaces are an artefact of the message, not damage to the file (#942).

Building against the released kernel gives false results. Change anything under Sources/OCCTBridge/ and build from source. In a worktree, Libraries/ is its own tree — symlink it from the main checkout and set OCCTSWIFT_LOCAL=1.

Refetch before transcribing anything. Two branches adding a CHANGELOG entry both insert at the top of the same section; a stale base gives a conflict whose resolution silently drops the other side's work.

What this lane already knows

Ordering

Depends on: Pass 2a (#382), 2b (#383), 3 (#384). Parallel with: 4a (#385), 4b (#386), 4c (#387). Companion: #814 is this lane's refman-coverage audit and runs after this pass.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    phase:4dPass 4d — Mesh + presentation/misc lane duplication audit (#388)priority:P3Low / somedayrefactorPart of the #377 codebase/docs duplication-audit efforttype:epicLarge effort tracked via sub-issues

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions