fix(profiles): resolve inherits by declared name, emit filament_vendor, log degrade paths - #3
Merged
Conversation
…r, log degrades
Three related silent failures in the bundled-profile inheritance walk.
1. Slash-named parents broke the walk. Some bundled profiles declare a
`name` containing a literal `/` while the file on disk substitutes
something else — and the substitute is not consistent:
"Bambu Support For PA/PET @base" -> "Bambu Support For PA PET @base.json"
"Bambu Support For PLA/PETG @base" -> "Bambu Support For PLA-PETG @base.json"
profile-resolver derived the parent's path from the `inherits` string,
so the `/` acted as a directory separator, the read ENOENTed, and the
whole remaining ancestor chain was dropped without a word. Because the
walk is shared with the slice path and /resolved-process, a user preset
inheriting such a base sliced under-specified, silently.
Parents are now located by each bundled file's *declared* `name` via a
directory index rather than by re-deriving a path — the sanitization
scheme is undocumented and demonstrably differs per case, so guessing at
it is the wrong approach. The direct path is still tried first and the
index is built lazily only on a miss, memoised per category directory:
resolveProfile runs once per preset in a ~2500-file listing loop and the
basename matches for the overwhelming majority, so the hot path stays a
single readFile and cold-listing latency is unchanged.
This also closes a path traversal: a `..` (or absolute path) in an
`inherits` value previously reached outside the profiles directory by
the same mechanism. The direct lookup now refuses anything that is not a
plain basename, and the index can only ever name files enumerated from
inside the category directory.
2. `filament_vendor` sits on the family base ("Bambu ABS @base" ->
["Bambu Lab"]), one hop up the chain the walk already traverses, and was
never read. It now gets the same treatment as colour, reusing firstScalar
so an empty string or non-string yields null rather than a fabricated
value. It has only one spelling — no second key was invented.
3. Both degrade paths in GET /profiles/bundled logged nothing, so a
systematic resolution failure would present as the endpoint quietly
answering all-null again. Both now warn, and the listing emits a
resolved-count line — which also covers the dangling-parent case, whose
degrade is silent by design and never throws.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Three follow-ups found while reviewing #2, bundled into one PR because sidecar changes cost an image rebuild and rebuilding twice for three small changes is waste. Bambuddy ticket AFKFelix#64.
1. Slash-named parents silently dropped the whole inherits chain
Some bundled profiles carry a literal
/in theirname, while the file on disk sanitizes it.resolveProfilederived the parent's path from theinheritsstring, so the/was read as a directory separator, the read ENOENTed, and the resolver silently deletedinheritsand stopped — discarding every remaining ancestor without a word.This was never just a listing bug:
profile-resolver.tsis shared with the slice path and/resolved-process, so a user preset inheriting a slash-named base lost its entire ancestor chain and sliced under-specified, silently. Pre-existing; not introduced by #2.What the upstream trees actually contain
Grepped from read-only clones of
SoftFever/OrcaSlicer@v2.3.2andbambulab/BambuStudio@v02.07.01.57— not from an image:inheritscontains/inheritscontaining..BambuStudio is the two known Bambu families. OrcaSlicer is not — it has five more, all QIDI, which is what settled the design question:
Four different mappings, and the QIDI ones are not sanitizations at all —
@Q2-Seriesbecomes@Q2and a space appears before the@. No sanitizingpath.joincan work, because the filename is not a pure function of the declared name. So this indexes the directory by each file's declaredname, as the ticket preferred.Within each
BBL/<category>directory — the resolver's actual index scope — there are zero declared-name collisions in either slicer (Orca 1071 names / 1072 files, BambuStudio 1723 / 1727), so the index is unambiguous.Design: lazy-on-miss
The direct
path.joinread is tried first and the index is built only when it misses, memoised per category directory. The hot path stays exactly onereadFileper hop, so the ~2500-file listing loop pays no directory enumeration when nothing is slash-named. Two unit tests pin that contract (readdircalled once across five slash resolutions; never when the direct path hits).Path traversal closed structurally
The direct lookup now refuses anything that is not a plain basename (
/,\,\0,.,.., absolute), and the index can only ever name filesreaddirproduced from inside the category directory. There is no longer a path by which aninheritsvalue reaches outside the profiles tree. No shipped profile exercises this (0 occurrences of..in either tree) — it is hardening, not an active exploit.2.
filament_vendorOne hop up the chain the walk already traverses —
["Bambu Lab"]onBambu ABS @basein both slicers. Given the same treatment ascolourOf, reusingfirstScalarso an empty string or non-string yieldsnullrather than a fabricated value. Note it has only one spelling; unlike colour there is nodefault_filament_vendorin either tree.3. Degrade paths now log
catch { fields = leaf; }and the outercatch { continue; }both logged nothing, which is why bambuddy#47 and AFKFelix#51 each took a ticket to localize. Both nowconsole.warn.Added beyond the ticket: a
console.inforesolved-count line once per cache fill. The commonest silent failure — a danglinginherits— by design neither throws nor warns, so only the counts make a tier-wide regression to all-nullvisible.Measured against the real upstream trees
The endpoint run against the actual
resources/profiles/BBLtrees at the pinned versions (still no image — these are the upstream repos):Slash-named presets: 22 (Orca) and 34 (BambuStudio), 0 typed before, all typed after.
Bambu Support For PA/PET @BBL A1now resolvesPA/Bambu Lab.This also independently confirms #2's headline numbers (920/974 and 1584/1618) and its cold-listing figures (300/471 ms), which had been measured but not reproduced outside the images.
Cold-listing cost, stated honestly: +109 ms Orca, +169 ms BambuStudio. That is the one directory enumeration the index build performs, paid once per cache fill (1 h TTL) and only because slash-named presets exist at all. The per-preset hot path is unchanged.
Correcting the record from #2
#2's description, and its comment on bambuddy#51, state that the
Support For PA/PETandPLA/PETGfamilies'@baseparent is "genuinely absent from the shipped tree" and their material "not knowable from the bundled JSON". That is false. The parents ship in both slicers under sanitized filenames; the walk simply could not reach them:Identical in both slicers.
The 32 still untyped on OrcaSlicer are a different bug
They are
eSUN PLA+ @base,Overture PLA @base,PolyLite PETG @baseand friends — parents that live inBBL/filament/subdirectories, whichreadBundledDirnever reads because itsreaddiris non-recursive (9 subdirectories, 615 files on Orca; 3 and 150 on BambuStudio). Deliberately out of scope here and filed separately. BambuStudio reaches 100% because its untyped set was entirely the slash families.Tests
+20 tests (8 unit, 12 e2e).
tests/e2e/bundled-filament-metadata.spec.tsfrom #2 keeps passing untouched.bambuddy/profile-resolverThe 22 failures are pre-existing and environmental — identical set on both branches, verified by diffing failing test names. They are the slice/schema e2e specs, which need
BUNDLED_PROFILES_PATH/ORCASLICER_PATHand the slicer binary that only exists inside the image. Zero regressions.tsc --noEmitclean.eslintreports 3 errors, all pre-existing and in files this PR does not touch (health.service.ts:48,ecosystem.config.cjs:1,bundle.service.spec.ts:138); zero new.Mutation-checked
Each fix was made inert and the corresponding tests confirmed to fail — run independently of the implementation:
path.join..traversal test (expected 'yes' to be undefined— the traversal really did leak), the index-caching test, the count linefilament_vendorforced tonullemits filament_vendor resolved from an ancestorconsole.warn+console.infosilencedOne honest non-result: the
absolute inheritstest passes under the reverted lookup too —path.join(dir, "/abs/path")happens to produce a nonexistent nested path, so the old code failed to exfiltrate by luck rather than design. It is a regression guard, not a mutation-proven one. The..test is the one that actually catches traversal.Not verified here
Nothing in this PR has been run inside a rebuilt image — no Docker or registry command was run in this session, deliberately. Still requiring the rebuild session: behaviour against the images' own trees (which may differ from upstream at these tags), real cold-listing latency in-container, and bambuddy#51's end-to-end criterion that a stock install auto-picks an ABS profile for an ABS plate.
Bambuddy-side,
filament_vendoris already read atbackend/app/api/routes/slicer_presets.py:447, so it should flow through with no Bambuddy change — worth confirming, and it likely obsoletes bambuddy#62 (theBambu->Bambu Labalias), which should not be closed until checked against a rebuilt image.