fix(search): expose markdown frontmatter on chunks#314
Conversation
|
Thanks for working on this! Exposing the markdown frontmatter on chunks is a great addition for search filtering. |
|
Updated per feedback. I added the include_frontmatter_in_metadata flag, set the default config value to false, and adjusted tests to cover both the backward-compatible default and the opt-in metadata behavior. Verification:
|
1e6d822 to
193a4ae
Compare
|
Rebased onto current Resolution notes:
Local verification before pushing:
|
|
Pushed a follow-up commit New: Why: the previous PR exposed all non-empty frontmatter, which is fine for short, well-curated frontmatter but starts leaking implementation metadata ( Behavior matrix (now covered by
Local verification:
No change to |
5ef7feb to
5aec72c
Compare
|
Heads-up: the branch head on the fork had been reverted to Local verification on the restored chain:
If anyone wonders where the earlier revert came from: it lined up with a local branch named |
5aec72c to
8d1a49e
Compare
|
Heads-up on the previous push: rebased onto Conflict resolution: kept #325's Local verification:
|
ployts
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-through, @RerankerGuo — this looks good to merge. 🎉
The backward-compat concerns are fully addressed:
- include_frontmatter_in_metadata defaults to false, so existing users see no behavior change.
- include_frontmatter_keys_in_metadata (default []) gives a clean opt-in path to expose only the specific frontmatter keys needed for search filters, avoiding the implementation-metadata leakage from the original approach.
- Rebased onto current origin/main (82971ac), conflicts on reme/config/default.yaml resolved while keeping #325's json/jsonl chunker entries, and tests cover both the default and opt-in paths (targeted pytest + pre-commit passing).
Approving — will merge. Thanks again for exposing frontmatter for search filtering, this is a nice addition. 🙏
|
Thanks @ployts for the review and the merge! 🙏 Quick recap of the collaboration loop on this one for future reference:
The allow-list landed as a follow-up precisely because your first-round comment pointed at the same surface area; happy to keep using that pattern (default-OFF + explicit opt-in) on future config-facing PRs. Will leave #332 (docs follow-up) and #333 (jinli_lme.yaml lme_one_question job) sitting for now — they're complementary to #326 and shouldn't need changes from your side unless the LongMemEval direction tightens further. |
Summary
Refs #302
Verification
2 passed in 1.02s
check yaml...........................................(no files to check)Skipped
check xml............................................(no files to check)Skipped
check toml...........................................(no files to check)Skipped
check json...........................................(no files to check)Skipped
detect private key.......................................................Passed
trim trailing whitespace.................................................Passed
Add trailing commas......................................................Passed
black....................................................................Passed
flake8...................................................................Passed
pylint...................................................................Passed
Check package with Pyroma................................................Passed