Conversation
Zig bitcode is now processed alongside Rust bitcode during the Bazel release build, enabling interprocedural optimization across the FFI boundary. Pipeline: zig -femit-llvm-bc → clang -flto=thin wrap → llvm-ar → rustc -Clinker-plugin-lto → ld64.lld ThinLTO merge. Key changes: - patches/rules_zig_lto.patch: two-step bitcode build for zig_static_library (adapted from forge) - patches/rules_zig_cdeps.patch: filter CC flag leaks into Zig - tools/clang-lto-wrapper.sh: strip -plugin-opt args that ld64.lld rejects from rustc -Clinker-plugin-lto - MODULE.bazel: add apple_support, scope toolchains_llvm_bootstrapped to Linux (macOS blocked by Apple SDK 403) - .bazelrc: CC toolchain config, -fllvm, use_cc_common_link, macOS ld64.lld linking, -Clinker-plugin-lto for Rust Requires: brew install llvm (provides clang, llvm-ar, ld64.lld). Wrapper installed at /opt/homebrew/opt/llvm/bin/clang-lto-wrapper.
Under cross-language ThinLTO, the Zig fault-injection check (__marky_test_force_create_fail__) is dead-code-eliminated. The test now asserts this: engine creates successfully under LTO, confirming cross-language optimization is active. Skips in debug builds where fault injection is still live.
Bazel replaces Cargo as the primary build/test system for CI, release, and local install. Cargo is kept as a lightweight canary. Key changes: - .bazelrc: split release config into common (build:release) + macOS (build:macos-lto) using extra_rustc_flag (singular, accumulates) - scripts/install.sh: builds with Bazel LTO, installs to ~/.local/bin - ci.yml: Bazel build & test primary, lint stays Cargo, Cargo canary job added. No more setup-zig or Zig cache workarounds. - release.yml: Bazel for macOS/Linux native builds (with LTO), Cargo for Windows + Linux cross-compile - CLAUDE.md: Quick Reference updated to show Bazel-first workflow
Moves -femit-llvm-bc from zig/BUILD.bazel zigopts (unconditional) to .bazelrc build:macos-lto (conditional). The LTO patch hardcodes Homebrew LLVM paths that don't exist on Linux CI runners.
Fixes glibc bootstrap on Ubuntu 24.04 CI runners.
rustc injects -lgcc_s into the link line for x86_64-unknown-linux-gnu, but the hermetic LLVM sysroot (--sysroot=/dev/null) uses compiler-rt and has no libgcc_s. toolchains_llvm_bootstrapped provides a built-in stub via experimental_stub_libgcc_s=True. Matches the e2e/rules_rust config from toolchains_llvm_bootstrapped.
- Resolve audit.jsonl via bn merge-driver (union merge) - Accept docs/MEMORY.md deletion (migrated to auto-memory) - Convert dev's cargo criterion bench to Bazel: new bench_index_construction rust_binary target with bench-internals feature-gated library variant - CI benchmark step now runs criterion via bazel run
Convert CONFIGS from string to array to eliminate SC2086 word-splitting warnings. Quote /markymark in subshell to handle paths with spaces.
…Bazel build The Bazel build previously produced a binary without optional features (semantic-search, local-embeddings). These are now enabled by default across all crate BUILD.bazel files so the install script produces a fully-featured binary. - Add crate_features for semantic-search and local-embeddings to markymark-cli, markymark-core, markymark-mcp, markymark-index - Add fastembed via crate.spec() in MODULE.bazel (optional dep not resolved by crate_universe from_cargo) - Pass HOME to sandbox via --action_env so ort-sys can download ONNX Runtime binaries - Remove incorrect license section from plugin README
select-binary.sh and select-binary.ps1 always used the plugin-local bin/markymark, ignoring a system-installed version on PATH. Add a command -v / Get-Command check at the top of main() so an existing install (e.g. ~/.local/bin/markymark) takes precedence. Precedence is now: system PATH > plugin bin/ > GitHub Release download. Test harness hardened with CLEAN_PATH to strip the real markymark from PATH, keeping bundled-binary tests isolated from the host environment.
Contributor
|
Required keyword not found in PR title or description. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR promotes Bazel to the primary build system and adds cross-language ThinLTO between Rust and Zig (with macOS-specific linker/tooling adjustments), while updating CI/release workflows and the Claude plugin packaging/selection logic.
Changes:
- Add macOS ThinLTO support via a clang linker wrapper, Bazel config split (
release+macos-lto), and patchedrules_zigto produce linkable LTO archives. - Migrate CI and parts of release builds to Bazel; keep Cargo as a compatibility canary and for select targets.
- Update plugin behavior to prefer a system-installed
markymark, expand LSP filetype mappings, and refactor plugin metadata.
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/clang-lto-wrapper.sh | Adds clang wrapper to strip unsupported linker plugin flags on macOS. |
| scripts/install.sh | Adds Bazel-based installer targeting ~/.local/bin. |
| patches/rules_zig_lto.patch | Patches rules_zig to support a two-step bitcode→object→archive flow for macOS LTO. |
| patches/rules_zig_cdeps.patch | Filters leaked CC link flags in Zig cdeps handling. |
| patches/BUILD.bazel | Exports patch files for Bazel module patching. |
| markymark-plugin/tests/test_select_binary.sh | Expands plugin tests; adds PATH isolation and system-binary preference coverage. |
| markymark-plugin/scripts/select-binary.sh | Prefers system markymark on PATH before bundled/downloaded binary. |
| markymark-plugin/scripts/select-binary.ps1 | Same preference behavior for Windows PowerShell. |
| markymark-plugin/README.md | Removes embedded license section. |
| markymark-plugin/.lsp.json | Expands extension mappings and renames top-level server key. |
| markymark-plugin/.claude-plugin/plugin.json | Removes inline server definitions; keeps metadata/keywords only. |
| markymark-mcp/src/engine/tests/mod.rs | Replaces a fallback test with an LTO “canary” test. |
| markymark-mcp/BUILD.bazel | Enables semantic-search feature for Bazel-built MCP crate. |
| markymark-index/BUILD.bazel | Refactors deps, adds feature-gated bench variant, and adds a Bazel criterion bench target. |
| markymark-core/src/structured.rs | Recognizes .mdx as Markdown. |
| markymark-core/BUILD.bazel | Enables additional crate features and adds deps for semantic-search/local-embeddings. |
| markymark-cli/BUILD.bazel | Enables semantic-search and local-embeddings features for the Bazel CLI binary. |
| MODULE.bazel | Adds apple_support, updates LLVM toolchain version/registration, applies rules_zig patches, and pins fastembed spec. |
| Cargo.lock | Dependency updates. |
| CLAUDE.md | Updates docs to reflect Bazel-first workflows and build configs. |
| .gitignore | Ignores static archives and chunkhound outputs. |
| .github/workflows/release.yml | Moves macOS + Linux x86_64 builds to Bazel; adjusts packaging steps accordingly. |
| .github/workflows/ci.yml | Adds Bazel build/test as primary CI job; keeps Cargo lint/canary and benchmarks adapted to Bazel. |
| .bones/tasks/marky-qqb.md | Adds task record documenting Bazel/LTO migration work. |
| .bones/tasks/marky-o8e.md | Adds dependency linkage to the new task. |
| .bones/audit.jsonl | Logs task creation/dependency events. |
| .bazelrc | Splits configs and adds macOS-specific linker/LTO settings plus release ThinLTO flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+58
to
+61
| build:macos-lto --@rules_zig//zig/settings:zigopt=-femit-llvm-bc | ||
| build:macos-lto --@rules_rust//rust/settings:extra_rustc_flag=-Clinker=/opt/homebrew/opt/llvm/bin/clang-lto-wrapper | ||
| build:macos-lto --@rules_rust//rust/settings:extra_rustc_flag=-Clink-arg=-fuse-ld=/opt/homebrew/bin/ld64.lld | ||
| build:macos-lto --@rules_rust//rust/settings:extra_rustc_flag=-Clink-arg=-flto=thin |
There was a problem hiding this comment.
The macOS LTO config hardcodes /opt/homebrew/.../clang-lto-wrapper and /opt/homebrew/bin/ld64.lld, which will fail on Intel macOS (usually /usr/local/...) and any non-Homebrew LLVM install. Make these tool paths configurable (e.g., via --action_env/repo rule) or derive them from brew --prefix llvm during setup.
Comment on lines
+26
to
+30
| + # ld64.lld recognizes. Raw .bc causes "Invalid record" at link time. | ||
| + _llvm_prefix = "/opt/homebrew/opt/llvm/bin/" | ||
| + bc_obj = ctx.actions.declare_file(ctx.label.name + ".lto.o") | ||
| + ctx.actions.run_shell( | ||
| + outputs = [bc_obj], |
There was a problem hiding this comment.
This patch hardcodes the Homebrew LLVM toolchain prefix (/opt/homebrew/opt/llvm/bin/). That will not work on Intel macOS (typically /usr/local/opt/llvm/bin/) and also makes the build non-hermetic (Bazel can’t track these host tools as action inputs). Consider wiring clang/llvm-ar in via toolchain-provided tools (or an explicit repository rule) and passing them to the action as declared tools/inputs.
Comment on lines
121
to
125
| @@ -81,27 +125,26 @@ jobs: | |||
| fi | |||
There was a problem hiding this comment.
The Cargo release builds (Windows + Linux aarch64) run without enabling the markymark-cli features that Bazel enables (semantic-search, local-embeddings). This will produce different binaries across targets. Either pass the needed --features ... to the Cargo build here, or avoid enabling those features unconditionally in the Bazel targets so releases stay consistent.
Comment on lines
+9
to
+16
| # - macOS only: brew install llvm | ||
|
|
||
| INSTALL_DIR="${INSTALL_DIR:-$HOME/.local/bin}" | ||
| mkdir -p "$INSTALL_DIR" | ||
|
|
||
| CONFIGS=(--config=release) | ||
| case "$(uname -s)" in | ||
| Darwin) CONFIGS+=(--config=macos-lto) ;; |
There was a problem hiding this comment.
On macOS this script adds --config=macos-lto, which (per .bazelrc) expects clang-lto-wrapper to already exist at a fixed Homebrew path. install.sh only lists brew install llvm and does not install/copy the wrapper, so a fresh machine will likely fail the build. Consider installing the wrapper automatically (deriving the LLVM prefix dynamically) or documenting the manual step here.
Suggested change
| # - macOS only: brew install llvm | |
| INSTALL_DIR="${INSTALL_DIR:-$HOME/.local/bin}" | |
| mkdir -p "$INSTALL_DIR" | |
| CONFIGS=(--config=release) | |
| case "$(uname -s)" in | |
| Darwin) CONFIGS+=(--config=macos-lto) ;; | |
| # - macOS only: Homebrew LLVM (e.g.,: brew install llvm) | |
| INSTALL_DIR="${INSTALL_DIR:-$HOME/.local/bin}" | |
| mkdir -p "$INSTALL_DIR" | |
| CONFIGS=(--config=release) | |
| case "$(uname -s)" in | |
| Darwin) | |
| CONFIGS+=(--config=macos-lto) | |
| # Ensure clang-lto-wrapper exists in the Homebrew LLVM prefix expected by .bazelrc. | |
| if command -v brew >/dev/null 2>&1; then | |
| LLVM_PREFIX="$(brew --prefix llvm 2>/dev/null || true)" | |
| if [[ -n "${LLVM_PREFIX:-}" ]]; then | |
| WRAPPER="${LLVM_PREFIX}/bin/clang-lto-wrapper" | |
| CLANG="${LLVM_PREFIX}/bin/clang" | |
| if [[ ! -x "${WRAPPER}" && -x "${CLANG}" ]]; then | |
| echo "Creating clang-lto-wrapper symlink at ${WRAPPER}" | |
| ln -sf "${CLANG}" "${WRAPPER}" | |
| fi | |
| fi | |
| fi | |
| ;; |
Comment on lines
+20
to
+23
| # Use system LLD (Homebrew LLVM) for CC linking — enables cross-language LTO. | ||
| # Apple ld doesn't support cross-language LTO; system ld64.lld does. | ||
| common:macos --linkopt=-fuse-ld=/opt/homebrew/bin/ld64.lld | ||
| common:macos --linkopt=-flto=thin |
There was a problem hiding this comment.
common:macos hardcodes /opt/homebrew/bin/ld64.lld and enables -flto=thin for all macOS builds. This makes even debug builds depend on Homebrew LLVM and breaks on Intel macOS where the prefix is typically /usr/local. Consider moving these --linkopt entries under build:macos-lto (or making the path configurable) so plain bazel build works without Homebrew LLVM and supports both macOS architectures.
Comment on lines
+877
to
+881
| async fn lto_eliminates_fault_injection() { | ||
| // In debug builds the fault injection is live — skip. | ||
| if cfg!(debug_assertions) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is described as an “LTO canary”, but the skip condition only checks cfg!(debug_assertions). In -c opt builds without LTO, debug_assertions is false so the test will run and likely fail, which can make local opt-mode runs brittle. Consider gating on an explicit build-time signal/env var indicating LTO is enabled so it only runs when LTO is expected.
| *) args+=("$arg") ;; | ||
| esac | ||
| done | ||
| exec /opt/homebrew/opt/llvm/bin/clang "${args[@]}" |
There was a problem hiding this comment.
The wrapper hardcodes the Homebrew LLVM clang path (/opt/homebrew/opt/llvm/bin/clang). This will break on Intel macOS runners (Homebrew prefix is typically /usr/local) and on developer machines with a different LLVM install path. Prefer resolving the LLVM prefix dynamically (e.g., via brew --prefix llvm), or pass the tool path in via an env var/action_env and use that here.
Suggested change
| exec /opt/homebrew/opt/llvm/bin/clang "${args[@]}" | |
| LLVM_CLANG_BIN="${CLANG_LTO_WRAPPER_CLANG:-}" | |
| if [ -z "$LLVM_CLANG_BIN" ] && command -v brew >/dev/null 2>&1; then | |
| if LLVM_PREFIX="$(brew --prefix llvm 2>/dev/null)"; then | |
| LLVM_CLANG_BIN="${LLVM_PREFIX}/bin/clang" | |
| fi | |
| fi | |
| : "${LLVM_CLANG_BIN:=/opt/homebrew/opt/llvm/bin/clang}" | |
| exec "$LLVM_CLANG_BIN" "${args[@]}" |
Comment on lines
+36
to
+38
| - **zig_build_kwargs | ||
| + command = _llvm_prefix + "clang -c -x ir " + bc.path + " -o " + bc_obj.path + " -flto=thin -target arm64-apple-macos", | ||
| + mnemonic = "WrapBitcode", |
There was a problem hiding this comment.
The clang invocation here hardcodes -target arm64-apple-macos. That will produce the wrong object format when building x86_64 macOS targets (including the macos-13 runner in the release matrix) and likely breaks linking. Use the actual target triple from the Zig/Bazel toolchain context instead of a fixed arm64 target.
Comment on lines
+2
to
6
| "markymark": { | ||
| "command": "${CLAUDE_PLUGIN_ROOT}/scripts/select-binary.sh", | ||
| "args": ["--lsp"], | ||
| "extensionToLanguage": { | ||
| ".md": "markdown", |
There was a problem hiding this comment.
The top-level key changed from markdown to markymark, but markymark-plugin/tests/test_select_binary.sh still reads .lsp.json['markdown']['command'] (and any external tooling may also expect the markdown key). This mismatch will cause tests to fail and may break plugin consumers. Either keep the original key, or update the tests/consumers to use the new key consistently.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 17-38: Add a PR-time macOS smoke job to the workflow that runs a
Bazel build with the macOS LTO config to validate the new Homebrew
LLVM/wrapper/rules_zig path: create a new job (e.g., name it
"macos-macos-lto-smoke") that uses runs-on: macos-latest, checks out the repo,
sets up Bazelisk (like the existing bazel-build-and-test job), and runs bazel
build --config=release --config=macos-lto //markymark-cli:markymark; ensure the
job mirrors the cache/setup steps from the existing bazel-build-and-test job so
it runs in PRs and catches regressions before the tag-only release workflow.
In @.github/workflows/release.yml:
- Around line 26-29: Replace the deprecated runner label by updating the macOS
runner for the Intel target: in the job entry that specifies target:
x86_64-apple-darwin (and currently uses os: macos-13), change os: macos-13 to
os: macos-15-intel so the Intel release job can schedule properly; keep the
existing bazel and bazel_config keys (bazel: true and bazel_config:
"--config=release --config=macos-lto") unchanged.
In `@markymark-mcp/src/engine/tests/mod.rs`:
- Around line 872-881: The test lto_eliminates_fault_injection incorrectly
claims to verify LTO dead-code elimination; the real cause is compile-time
cfg!(debug_assertions) behavior in should_force_engine_create_fail_for_tests
(engine/mod.rs), so either rename the test to accurately reflect what it
verifies (e.g., release_mode_disables_fault_injection) and update its
doc-comment and test name, or replace it with a true LTO verification harness
(e.g., emit and inspect binary symbols or use a separate cross-module
unreachable-path that LTO can remove); locate and change the async test function
lto_eliminates_fault_injection in tests/mod.rs and ensure any references or
documentation are updated to use the new name if you choose renaming.
In `@markymark-plugin/.lsp.json`:
- Line 2: The repo rename changed the .lsp.json key from "markdown" to
"markymark" but a consumer still reads .lsp.json['markdown']['command']; update
that consumer (the test that references .lsp.json['markdown']['command']) to
read .lsp.json['markymark']['command'] (or add a fallback that checks both keys)
so the check matches the new key name "markymark".
- Around line 9-18: The .lsp.json file exposes many structured-file extensions
(".json", ".yaml", ".toml", ".env", ".ini", etc.) but markymark-lsp currently
only indexes Markdown in markymark-lsp/src/state/mod.rs (see the Markdown-only
indexing at ~lines 320-330), so remove or revert the structured-file mappings
and limit .lsp.json to just Markdown/MDX extensions (e.g., .md, .mdx, .markdown)
so the LSP advertises only formats the server actually indexes; alternatively,
if you prefer to keep the mappings, implement structured-document indexing in
the state module (functions in mod.rs that perform indexing) before adding those
extensions to .lsp.json.
In `@markymark-plugin/scripts/select-binary.ps1`:
- Around line 17-22: The launcher currently prefers the system markymark
(Get-Command -> $systemBinary) which shadows the bundled plugin binary; change
the order so the packaged binary is invoked first: detect and invoke the bundled
executable (e.g., the plugin's shipped markymark in the script's directory such
as using $PSScriptRoot or the bundled path) with `@args` and preserve exit code
(exit $LASTEXITCODE), and only if the packaged binary is missing fall back to
using Get-Command/$systemBinary as the secondary option.
In `@markymark-plugin/scripts/select-binary.sh`:
- Around line 77-82: The launcher currently prefers any markymark on PATH by
using command -v and exec; change it so the packaged binary remains the default:
only use the system-installed binary when an explicit opt-in is set (e.g. an env
var like USE_SYSTEM_MARKYMARK=1 or MARKYMARK_PREFER_SYSTEM) or when you verify
compatibility (run the system binary with --version and compare to the bundled
version) before exec'ing it; update the block that assigns system_binary and the
conditional around exec "${system_binary}" "$@" (and related logic that
determines the path to the bundled binary) to implement the opt-in flag or
version check.
In `@markymark-plugin/tests/test_select_binary.sh`:
- Around line 729-732: Replace the hard-coded clean_path value with the shared
CLEAN_PATH test helper: instead of setting local clean_path="/usr/bin:/bin" and
using PATH="${clean_path}" when invoking "${TEST_DIR}/scripts/select-binary.sh",
use PATH="${CLEAN_PATH}" so the test reuses the sanitized PATH helper; update
the local variable or call site in the test function to reference CLEAN_PATH and
remove the hard-coded path string.
In `@patches/rules_zig_cdeps.patch`:
- Around line 6-8: The list comprehension that replaces
args.add_all(link.user_link_flags) filters out "-pthread", "-Wl,*" and "-l*"
flags which can silently drop needed linker options; add a concise inline
comment above or next to the args.add_all(...) call explaining why these
specific flags are excluded (e.g., Zig's linker handling and Bazel/C++ toolchain
incompatibilities), referencing linking_context.linker_inputs,
link.user_link_flags and args.add_all so future maintainers understand the
intent and know where to look if linking behavior needs adjustment.
In `@patches/rules_zig_lto.patch`:
- Around line 27-37: The command currently hardcodes "-target arm64-apple-macos"
when compiling IR to object (in the block creating bc_obj and calling
ctx.actions.run_shell), which causes wrong-arch objects for non-ARM macOS
builds; change the run_shell command to derive the clang target from
zigtargetinfo.triple (e.g., build the command string using _llvm_prefix + "clang
... -target " + zigtargetinfo.triple + " ...") instead of the hardcoded value so
the clang target matches the Zig target; update the call that constructs command
(the ctx.actions.run_shell invocation that references bc.path and bc_obj.path)
to use zigtargetinfo.triple for the -target argument.
In `@scripts/install.sh`:
- Around line 14-17: The case statement that appends platform-specific configs
only handles Darwin (CONFIGS and the Darwin) CONFIGS+=(--config=macos-lto)
branch; add a Linux branch to append a Linux-specific config (e.g.,
CONFIGS+=(--config=linux-lto)) or include a commented placeholder for future
Linux LTO support so the script can opt into a linux-lto config when/if added in
.bazelrc.
In `@tools/clang-lto-wrapper.sh`:
- Line 12: The wrapper currently execs a hardcoded
/opt/homebrew/opt/llvm/bin/clang which only works on Apple Silicon; change the
exec to resolve Homebrew's LLVM prefix dynamically (e.g. use the output of brew
--prefix llvm to build the clang path) or detect architecture and pick
/opt/homebrew/... vs /usr/local/opt/... before calling exec so the final exec
uses the resolved path with "${args[@]}".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32e8cdda-791e-4f80-8ae1-20975835a5e3
⛔ Files ignored due to path filters (4)
.bones/audit.jsonlis excluded by!.bones/**.bones/tasks/marky-o8e.mdis excluded by!.bones/**.bones/tasks/marky-qqb.mdis excluded by!.bones/**Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.bazelrc.github/workflows/ci.yml.github/workflows/release.yml.gitignoreCLAUDE.mdMODULE.bazelmarkymark-cli/BUILD.bazelmarkymark-core/BUILD.bazelmarkymark-core/src/structured.rsmarkymark-index/BUILD.bazelmarkymark-mcp/BUILD.bazelmarkymark-mcp/src/engine/tests/mod.rsmarkymark-plugin/.claude-plugin/plugin.jsonmarkymark-plugin/.lsp.jsonmarkymark-plugin/README.mdmarkymark-plugin/scripts/select-binary.ps1markymark-plugin/scripts/select-binary.shmarkymark-plugin/tests/test_select_binary.shpatches/BUILD.bazelpatches/rules_zig_cdeps.patchpatches/rules_zig_lto.patchscripts/install.shtools/clang-lto-wrapper.sh
💤 Files with no reviewable changes (1)
- markymark-plugin/README.md
Comment on lines
+17
to
+38
| bazel-build-and-test: | ||
| name: Bazel Build & Test | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Bazelisk | ||
| uses: bazelbuild/setup-bazelisk@v3 | ||
|
|
||
| - name: Cache Bazel | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.cache/bazel | ||
| key: ${{ runner.os }}-bazel-${{ hashFiles('MODULE.bazel', 'Cargo.lock', '.bazelrc') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-bazel- | ||
|
|
||
| - name: Build all targets | ||
| run: bazel build //... | ||
|
|
||
| - name: Run all tests | ||
| run: bazel test //... |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a PR-time macOS --config=macos-lto smoke job.
This workflow only exercises Bazel on Ubuntu, so none of the new Homebrew LLVM/wrapper/rules_zig LTO path is validated before the tag-only release workflow. A single macOS bazel build --config=release --config=macos-lto //markymark-cli:markymark job would catch these regressions much earlier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 17 - 38, Add a PR-time macOS smoke job
to the workflow that runs a Bazel build with the macOS LTO config to validate
the new Homebrew LLVM/wrapper/rules_zig path: create a new job (e.g., name it
"macos-macos-lto-smoke") that uses runs-on: macos-latest, checks out the repo,
sets up Bazelisk (like the existing bazel-build-and-test job), and runs bazel
build --config=release --config=macos-lto //markymark-cli:markymark; ensure the
job mirrors the cache/setup steps from the existing bazel-build-and-test job so
it runs in PRs and catches regressions before the tag-only release workflow.
Comment on lines
+26
to
+29
| - target: x86_64-apple-darwin | ||
| os: macos-latest | ||
| os: macos-13 | ||
| bazel: true | ||
| bazel_config: "--config=release --config=macos-lto" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '22,30p' .github/workflows/release.yml
echo
curl -fsSL https://docs.github.com/en/actions/reference/runners/github-hosted-runners \
| rg -n 'macos-(13|15-intel|15|latest)'Repository: sethyanow/markymark
Length of output: 50375
Update os: macos-13 to a valid Intel macOS label.
The macos-13 runner label is no longer available; the Intel release job cannot schedule with this configuration. Use macos-15-intel instead.
Suggested update
- target: x86_64-apple-darwin
- os: macos-13
+ os: macos-15-intel
bazel: true
bazel_config: "--config=release --config=macos-lto"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - target: x86_64-apple-darwin | |
| os: macos-latest | |
| os: macos-13 | |
| bazel: true | |
| bazel_config: "--config=release --config=macos-lto" | |
| - target: x86_64-apple-darwin | |
| os: macos-15-intel | |
| bazel: true | |
| bazel_config: "--config=release --config=macos-lto" |
🧰 Tools
🪛 actionlint (1.7.11)
[error] 27-27: label "macos-13" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2025-vs2026", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26-large", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 26 - 29, Replace the deprecated
runner label by updating the macOS runner for the Intel target: in the job entry
that specifies target: x86_64-apple-darwin (and currently uses os: macos-13),
change os: macos-13 to os: macos-15-intel so the Intel release job can schedule
properly; keep the existing bazel and bazel_config keys (bazel: true and
bazel_config: "--config=release --config=macos-lto") unchanged.
Comment on lines
+872
to
+881
| /// LTO canary: verifies cross-language ThinLTO eliminates the test-only fault | ||
| /// injection in the Zig engine. Under LTO, the magic-filename check is optimized | ||
| /// away, so the engine creates successfully. Without LTO (debug builds), the | ||
| /// fault injection fires and this test is skipped. | ||
| #[tokio::test] | ||
| async fn engine_fallback_scan_when_no_stale_state() { | ||
| let dir = make_temp_realm_dir("create-fail"); | ||
| // Magic filename triggers forced create failure — no engine created. | ||
| async fn lto_eliminates_fault_injection() { | ||
| // In debug builds the fault injection is live — skip. | ||
| if cfg!(debug_assertions) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Test premise is incorrect — this does not verify LTO dead-code elimination.
The test claims LTO eliminates the fault injection, but the actual mechanism is different. Per markymark-mcp/src/engine/mod.rs:277-279, should_force_engine_create_fail_for_tests uses cfg!(debug_assertions) which is evaluated at compile time, not runtime. In release builds, cfg!(debug_assertions) evaluates to false at compile time, so the function always returns false regardless of LTO.
The test passes because release mode disables debug_assertions, not because LTO performs any dead-code elimination. To actually test LTO dead-code elimination, you would need:
- A code path that is reachable at compile time but provably unreachable via cross-module analysis
- A way to verify the code was eliminated (e.g., checking binary symbols or execution traces)
Consider renaming this test to release_mode_disables_fault_injection to accurately reflect what it verifies, or implement actual LTO verification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@markymark-mcp/src/engine/tests/mod.rs` around lines 872 - 881, The test
lto_eliminates_fault_injection incorrectly claims to verify LTO dead-code
elimination; the real cause is compile-time cfg!(debug_assertions) behavior in
should_force_engine_create_fail_for_tests (engine/mod.rs), so either rename the
test to accurately reflect what it verifies (e.g.,
release_mode_disables_fault_injection) and update its doc-comment and test name,
or replace it with a true LTO verification harness (e.g., emit and inspect
binary symbols or use a separate cross-module unreachable-path that LTO can
remove); locate and change the async test function
lto_eliminates_fault_injection in tests/mod.rs and ensure any references or
documentation are updated to use the new name if you choose renaming.
| @@ -1,10 +1,21 @@ | |||
| { | |||
| "markdown": { | |||
| "markymark": { | |||
There was a problem hiding this comment.
Update the remaining markdown consumer before renaming this key.
markymark-plugin/tests/test_select_binary.sh still reads .lsp.json['markdown']['command'] on Line 434, so this rename makes that check fail until the consumer is migrated too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@markymark-plugin/.lsp.json` at line 2, The repo rename changed the .lsp.json
key from "markdown" to "markymark" but a consumer still reads
.lsp.json['markdown']['command']; update that consumer (the test that references
.lsp.json['markdown']['command']) to read .lsp.json['markymark']['command'] (or
add a fallback that checks both keys) so the check matches the new key name
"markymark".
Comment on lines
+9
to
+18
| ".json": "json", | ||
| ".jsonc": "jsonc", | ||
| ".json5": "json5", | ||
| ".jsonl": "jsonl", | ||
| ".yaml": "yaml", | ||
| ".yml": "yaml", | ||
| ".toml": "toml", | ||
| ".env": "dotenv", | ||
| ".ini": "ini", | ||
| ".cfg": "ini" |
There was a problem hiding this comment.
Don’t route structured files to the LSP yet.
markymark-lsp/src/state/mod.rs still only indexes Markdown on Lines 320-330. With these mappings, JSON/YAML/TOML/.env/INI files will be sent to markymark and then ignored, so the plugin advertises support it does not actually provide. Keep .lsp.json limited to Markdown/MDX for now, or land structured-document indexing in the LSP first.
Minimal safe rollback
"extensionToLanguage": {
".md": "markdown",
".markdown": "markdown",
- ".mdx": "markdown",
- ".json": "json",
- ".jsonc": "jsonc",
- ".json5": "json5",
- ".jsonl": "jsonl",
- ".yaml": "yaml",
- ".yml": "yaml",
- ".toml": "toml",
- ".env": "dotenv",
- ".ini": "ini",
- ".cfg": "ini"
+ ".mdx": "markdown"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ".json": "json", | |
| ".jsonc": "jsonc", | |
| ".json5": "json5", | |
| ".jsonl": "jsonl", | |
| ".yaml": "yaml", | |
| ".yml": "yaml", | |
| ".toml": "toml", | |
| ".env": "dotenv", | |
| ".ini": "ini", | |
| ".cfg": "ini" | |
| ".md": "markdown", | |
| ".markdown": "markdown", | |
| ".mdx": "markdown" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@markymark-plugin/.lsp.json` around lines 9 - 18, The .lsp.json file exposes
many structured-file extensions (".json", ".yaml", ".toml", ".env", ".ini",
etc.) but markymark-lsp currently only indexes Markdown in
markymark-lsp/src/state/mod.rs (see the Markdown-only indexing at ~lines
320-330), so remove or revert the structured-file mappings and limit .lsp.json
to just Markdown/MDX extensions (e.g., .md, .mdx, .markdown) so the LSP
advertises only formats the server actually indexes; alternatively, if you
prefer to keep the mappings, implement structured-document indexing in the state
module (functions in mod.rs that perform indexing) before adding those
extensions to .lsp.json.
Comment on lines
+729
to
+732
| # Use a PATH that definitely has no markymark | ||
| local clean_path="/usr/bin:/bin" | ||
| local output | ||
| output="$(PATH="${clean_path}" "${TEST_DIR}/scripts/select-binary.sh" 2>&1)" || true |
There was a problem hiding this comment.
Reuse CLEAN_PATH here too.
Line 730 hard-codes /usr/bin:/bin, which can still contain a real markymark on machines where it is package-installed. That makes this test host-dependent and defeats the sanitized PATH helper you added above.
Proposed fix
- local clean_path="/usr/bin:/bin"
+ local clean_path="${CLEAN_PATH}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Use a PATH that definitely has no markymark | |
| local clean_path="/usr/bin:/bin" | |
| local output | |
| output="$(PATH="${clean_path}" "${TEST_DIR}/scripts/select-binary.sh" 2>&1)" || true | |
| # Use a PATH that definitely has no markymark | |
| local clean_path="${CLEAN_PATH}" | |
| local output | |
| output="$(PATH="${clean_path}" "${TEST_DIR}/scripts/select-binary.sh" 2>&1)" || true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@markymark-plugin/tests/test_select_binary.sh` around lines 729 - 732, Replace
the hard-coded clean_path value with the shared CLEAN_PATH test helper: instead
of setting local clean_path="/usr/bin:/bin" and using PATH="${clean_path}" when
invoking "${TEST_DIR}/scripts/select-binary.sh", use PATH="${CLEAN_PATH}" so the
test reuses the sanitized PATH helper; update the local variable or call site in
the test function to reference CLEAN_PATH and remove the hard-coded path string.
Comment on lines
+6
to
+8
| for link in linking_context.linker_inputs.to_list(): | ||
| - args.add_all(link.user_link_flags) | ||
| + args.add_all([f for f in link.user_link_flags if f != "-pthread" and not f.startswith("-Wl,") and not f.startswith("-l")]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Filter logic is sound but may benefit from documentation.
The inline list comprehension correctly filters out flags that Zig's build system doesn't handle (-pthread, -Wl,*, -l*). However, this broad filtering could silently drop flags that might be needed in the future.
Consider adding a comment in the patch or a nearby doc explaining why these specific flags are filtered, so future maintainers understand the intent when debugging linking issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patches/rules_zig_cdeps.patch` around lines 6 - 8, The list comprehension
that replaces args.add_all(link.user_link_flags) filters out "-pthread", "-Wl,*"
and "-l*" flags which can silently drop needed linker options; add a concise
inline comment above or next to the args.add_all(...) call explaining why these
specific flags are excluded (e.g., Zig's linker handling and Bazel/C++ toolchain
incompatibilities), referencing linking_context.linker_inputs,
link.user_link_flags and args.add_all so future maintainers understand the
intent and know where to look if linking behavior needs adjustment.
Comment on lines
+27
to
+37
| + _llvm_prefix = "/opt/homebrew/opt/llvm/bin/" | ||
| + bc_obj = ctx.actions.declare_file(ctx.label.name + ".lto.o") | ||
| + ctx.actions.run_shell( | ||
| + outputs = [bc_obj], | ||
| inputs = [bc], | ||
| - executable = zigtoolchaininfo.zig_exe_path, | ||
| - arguments = ["build-lib", global_args, lib_args], | ||
| - mnemonic = "ZigBuildLib", | ||
| - progress_message = "zig build-lib %{label}", | ||
| - **zig_build_kwargs | ||
| + command = _llvm_prefix + "clang -c -x ir " + bc.path + " -o " + bc_obj.path + " -flto=thin -target arm64-apple-macos", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Hardcoded Mach-O target in rules_zig patch:"
rg -n 'arm64-apple-macos' patches/rules_zig_lto.patch
echo
echo "Release matrix macOS targets:"
sed -n '22,30p' .github/workflows/release.ymlRepository: sethyanow/markymark
Length of output: 771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full context of patch around lines 37 and 90 ==="
sed -n '20,45p' patches/rules_zig_lto.patch
echo "..."
sed -n '80,100p' patches/rules_zig_lto.patch
echo
echo "=== Check if zigtargetinfo is already used in the file ==="
rg -n 'zigtargetinfo' patches/rules_zig_lto.patch | head -20Repository: sethyanow/markymark
Length of output: 2889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for zigtargetinfo definition or documentation ==="
rg -n 'class.*zigtargetinfo|def.*zigtargetinfo|zigtargetinfo.*=' --type starlark --type python
echo
echo "=== Look for usage patterns of zigtargetinfo.triple properties ==="
rg -n 'zigtargetinfo\.triple\.' --type starlark --type python -A 1
echo
echo "=== Check if there's a zig_target or similar mapping structure ==="
rg -n 'apple-macos|aarch64.*darwin|x86_64.*darwin' --type starlark --type python | head -20Repository: sethyanow/markymark
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find .bzl files that mention zigtargetinfo ==="
rg -n 'zigtargetinfo' --glob '*.bzl' | head -30
echo
echo "=== Look for how zigtargetinfo.triple properties are accessed ==="
rg -n 'zigtargetinfo\.triple\.' --glob '*.bzl' -A 1 | head -30
echo
echo "=== Search in the original file being patched for clues ==="
git show HEAD:$(rg --glob '*.bzl' -l 'zigtargetinfo' | head -1) 2>/dev/null | head -100 || echo "Could not retrieve original file"Repository: sethyanow/markymark
Length of output: 279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find all .bzl files in repo ==="
fd -e bzl
echo
echo "=== Search for zigtargetinfo in .bzl files ==="
rg 'zigtargetinfo' -g '*.bzl'
echo
echo "=== Look at the patch file more carefully to understand the context ==="
head -100 patches/rules_zig_lto.patchRepository: sethyanow/markymark
Length of output: 312
🏁 Script executed:
#!/bin/bash
set -euo pipefixo pipefail
echo "=== Repository structure ==="
ls -la
echo
echo "=== Search for zigtargetinfo in ALL files ==="
rg 'zigtargetinfo'
echo
echo "=== Full patch file to understand context ==="
wc -l patches/rules_zig_lto.patch
head -150 patches/rules_zig_lto.patchRepository: sethyanow/markymark
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Repository structure ==="
ls -la
echo
echo "=== Search for zigtargetinfo in ALL files ==="
rg 'zigtargetinfo'
echo
echo "=== Full patch file to understand context ==="
wc -l patches/rules_zig_lto.patch
head -150 patches/rules_zig_lto.patchRepository: sethyanow/markymark
Length of output: 9448
Don't force every macOS ThinLTO archive to arm64-apple-macos.
Lines 37 and 90 compile the emitted IR with -target arm64-apple-macos, but the release workflow builds both aarch64-apple-darwin and x86_64-apple-darwin with --config=macos-lto. The Intel lane will archive ARM64 objects and fail at link time. Derive the clang target from zigtargetinfo.triple instead of hardcoding it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patches/rules_zig_lto.patch` around lines 27 - 37, The command currently
hardcodes "-target arm64-apple-macos" when compiling IR to object (in the block
creating bc_obj and calling ctx.actions.run_shell), which causes wrong-arch
objects for non-ARM macOS builds; change the run_shell command to derive the
clang target from zigtargetinfo.triple (e.g., build the command string using
_llvm_prefix + "clang ... -target " + zigtargetinfo.triple + " ...") instead of
the hardcoded value so the clang target matches the Zig target; update the call
that constructs command (the ctx.actions.run_shell invocation that references
bc.path and bc_obj.path) to use zigtargetinfo.triple for the -target argument.
Comment on lines
+14
to
+17
| CONFIGS=(--config=release) | ||
| case "$(uname -s)" in | ||
| Darwin) CONFIGS+=(--config=macos-lto) ;; | ||
| esac |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding Linux-specific config if needed.
The script applies --config=macos-lto on Darwin, but there's no Linux-specific config. Per .bazelrc, the release config applies to all platforms, and macos-lto is macOS-specific. If Linux LTO support is added in the future, this case statement can be extended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install.sh` around lines 14 - 17, The case statement that appends
platform-specific configs only handles Darwin (CONFIGS and the Darwin)
CONFIGS+=(--config=macos-lto) branch; add a Linux branch to append a
Linux-specific config (e.g., CONFIGS+=(--config=linux-lto)) or include a
commented placeholder for future Linux LTO support so the script can opt into a
linux-lto config when/if added in .bazelrc.
| *) args+=("$arg") ;; | ||
| esac | ||
| done | ||
| exec /opt/homebrew/opt/llvm/bin/clang "${args[@]}" |
There was a problem hiding this comment.
Hardcoded path only supports Apple Silicon Macs.
The path /opt/homebrew/opt/llvm/bin/clang is specific to Apple Silicon (ARM) Macs. On Intel Macs, Homebrew installs to /usr/local/opt/llvm/bin/clang. If Intel Mac support is needed, consider detecting the architecture or using $(brew --prefix llvm)/bin/clang.
🔧 Proposed fix to support both architectures
-exec /opt/homebrew/opt/llvm/bin/clang "${args[@]}"
+LLVM_BIN="${LLVM_BIN:-$(brew --prefix llvm 2>/dev/null)/bin}"
+exec "${LLVM_BIN}/clang" "${args[@]}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| exec /opt/homebrew/opt/llvm/bin/clang "${args[@]}" | |
| LLVM_BIN="${LLVM_BIN:-$(brew --prefix llvm 2>/dev/null)/bin}" | |
| exec "${LLVM_BIN}/clang" "${args[@]}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/clang-lto-wrapper.sh` at line 12, The wrapper currently execs a
hardcoded /opt/homebrew/opt/llvm/bin/clang which only works on Apple Silicon;
change the exec to resolve Homebrew's LLVM prefix dynamically (e.g. use the
output of brew --prefix llvm to build the clang path) or detect architecture and
pick /opt/homebrew/... vs /usr/local/opt/... before calling exec so the final
exec uses the resolved path with "${args[@]}".
- Drop x86_64-apple-darwin from release matrix (Intel Mac discontinued) - Fix test_select_binary.sh: read ['markymark'] key (was ['markdown']) - Fix test_select_binary.sh: use CLEAN_PATH instead of hard-coded path - Add --features semantic-search,local-embeddings to Cargo release builds - install.sh: auto-copy clang-lto-wrapper on macOS if missing - LTO canary: gate on MARKYMARK_LTO_ENABLED env var, not debug_assertions - .bazelrc: set MARKYMARK_LTO_ENABLED=1 in test:release config - bones: create marky-hgz blocking marky-qqb for triage fixes
- brew install lld alongside llvm (LLVM 22 split lld into separate formula) - apt-get install libssl-dev for Linux builds (semantic-search features pull in openssl-sys)
Bazel hermetic sysroot and cross Docker container both ignore system-installed libssl-dev. Pass OPENSSL_DIR explicitly.
Bazel's hermetic sysroot (--sysroot=/dev/null) blocks system OpenSSL
headers. cross Docker containers don't inherit host env vars. Adding
openssl vendored feature compiles OpenSSL from bundled source inside
both sandboxes — no system dependency needed.
- Add openssl = { version = "0.10", features = ["vendored"] } to workspace deps
- Add matching crate.spec in MODULE.bazel for crate_universe
- Remove OPENSSL_DIR workarounds from release workflow
- Keep libssl-dev + perl install for vendored build prerequisites
Windows uses SChannel not OpenSSL. The unconditional openssl dep broke the Windows build (perl paths incompatible with MSYS2). Move to [target.'cfg(unix)'.dependencies] so it only applies on Linux and macOS.
- Add version.bzl as single source for Bazel targets; wire VERSION into
markymark-cli rust_binary so --version reports correctly (was 0.0.0)
- Move internal crate deps to [workspace.dependencies]; member crates now
use { workspace = true } (eliminates 15 per-crate version pins)
- Align markymark-vscode/package.json to workspace version
- Rewrite RELEASING.md to cover all five version sites
- unicode-segmentation 1.13.1 (yanked) → 1.13.2 - rand 0.9.2 (RUSTSEC-2026-0097, unsound with custom logger) → 0.9.4 Remaining cargo-audit warnings (number_prefix 0.4.0, paste 1.0.15) are unmaintained but latest versions of their respective crates, gated on upstream fastembed → indicatif/tokenizers updates.
…-trailing-newline markdown parse_block_tree_only internally appends \n when source lacks a trailing newline (required by tree-sitter-md's block grammar) and returns only the Tree. from_engine_result_with_source retained the un-normalized source and passed it to is_logseq_heading, which sliced via Node::utf8_text against tree positions referencing the longer normalized buffer — panic. Caught by catch_unwind in extract_content_blocks, so the process kept running but the affected file was silently dropped from the index. Reproducer: an 11-byte file containing "- # Heading" with no EOL. Fix: extract markymark_parser::normalize_block_source(&str) -> Cow<str> and use it at all four sites that previously duplicated the logic (parse_block_tree_only, parse_tree_only, parse_with_old_tree, and the new caller in from_engine_result_with_source). Normalizing at entry in from_engine_result_with_source means DocumentOwner.source_text, the tree walked by collect_blocks, and utf8_text slices all agree on a single byte sequence. Regression tests in markymark-index/tests/parse_robustness.rs, wired as //markymark-index:parse_robustness_test. 8 cases covering Logseq headings, nested lists, CRLF, BOM, trailing paragraphs — all green. 72-case torture harness (adversarial inputs across md/json/yaml/toml/jsonl/ini /env × BOM × CRLF × no-trailing-newline) passes 0/72 panics. Live bazel-bin/markymark-cli/markymark --mcp <repro> exits 0, empty stderr, no panic output. Incidental: resolved two pre-existing clippy warnings surfaced by pre-commit — clippy::question_mark in parser/extract/frontmatter.rs (?-rewrite) and clippy::collapsible_match in index/document/helpers.rs (guard on match arm). Both behaviour-preserving. Follow-ups tracked under epic marky-p88 (including wiring remaining integration tests into Bazel — they currently only run in cargo CI).
…-ups
Created during the 2026-04-20 panic-diagnosis session. Linear dep chain
so each item runs in its own focused session via /executing-plans.
marky-p88 [EPIC] Parse robustness refinement — tree-sitter normalization & panic safety
├── marky-prs (CLOSED) Fix parse_block_tree_only normalization leak
├── marky-lpz Wire integration tests into Bazel across all crates
├── marky-v6c Add ignore-filter to collect_documents (workspace scan hygiene)
├── marky-4g3 Surface warnings when extract_content_blocks catch_unwind fires
├── marky-gnk Bounds-checked utf8_text helper + audit all call sites
└── marky-vew Normalize source in structured parsers (json/yaml/toml/jsonl)
marky-prs retained in open-and-closed state for review traceability —
the fix already landed in c9f6860; the skeleton captures root cause,
evidence chain, files changed, and verification steps.
Next session: bn ready → claim marky-lpz → /executing-plans.
…ky-lpz)
49 new rust_test targets added; bazel test //... now runs 55 tests
(up from 8). Mirrors cargo's integration test coverage.
- core: basic_types, core_engine, miri_arena (3)
- parser: frontmatter, structured_{jsonc,json5}, ast_self_cell
(+ compile_data for include_str), typed_frontmatter,
tree_sitter_integration (6)
- index: connection_graph, realm_index, typed_frontmatter,
semantic_index, resolution, document_index, document_self_cell
(+ compile_data) (7)
- lsp: new markymark-lsp-testing variant (crate_features =
["test-helpers"]) + 15 integration tests
- mcp: 10 tests (5 with shared mod common, 1 with full
runtime_engine_tests/ submodule), crate_features = ["semantic-search"]
on tool_handler_tests to align cfg gates with library
- cli: 8 tests. cli_args, smoke_lsp, smoke_mcp, lsp_methods,
mcp_methods, triage_consistency via MARKYMARK_BIN /
MARKYMARK_CORPUS_DIR / MARKYMARK_TRIAGE_DOC env fallbacks
(cargo path preserved). alignment, bench_lsp tagged manual
(marksman sandbox handshake / benchmark).
Sentinel-file pattern used where \$(rootpath) needs a single-file
label (tests/corpus/basic.md). corpus_dir() helpers detect file
vs dir and strip filename accordingly.
Root BUILD.bazel exports docs/research/marksman-alignment-triage.md
for cross-package consumption by triage_consistency.
state_tests.rs:619,644 exercise test hooks at state/mod.rs:130,134 which are `cfg!(debug_assertions)`-gated. Under bazel -c opt the hooks never fire; update succeeds normally; index is replaced instead of falling back. Gate the tests to match. bazel test -c opt //markymark-lsp:state_test: 30/30 pass (2 cfg'd out) bazel test //markymark-lsp:state_test: 32/32 pass Surfaced when user ran bazel test with -c opt after marky-lpz wired state_test into Bazel; cargo default + bazel fastbuild both use debug so the divergence was previously invisible.
Fill gaps in the collect_documents ignore-filter task: - Implementation section with stepwise TDD order and file paths - Anti-Patterns section (SRE auto-reject gate) - Failure catalog: filter_entry semantics, non-UTF-8 paths, global-gitignore test contamination, symlink-cycle weak assertion, Bazel crate repin - Requirements→Criteria bijection repaired (symlink safety, sort determinism, opt-in baseline each now have a matching checkbox) - Converted "< 5s" numeric target to "measurable speedup recorded" per CLAUDE.md rule against invented numeric targets - Verified skeleton claims against codebase: function location, call sites, no competing scanners in LSP
Rewrite markymark-mcp's collect_documents to use ignore::WalkBuilder, preserving the stack-walker's behavior (follow symlinks, walk hidden files) while adding: - Baseline hard-ignore of .git/, target/, node_modules/, __pycache__/, .venv/, venv/, and any directory starting with bazel- - Respect for .gitignore, .ignore, and a new .markymarkignore custom file, applied regardless of whether the workspace has .git/ (via require_git(false)). Key toolchain behaviors surfaced during GREEN + integration runs: - ignore::WalkBuilder defaults to hidden(true) — had to set hidden(false) to keep indexing .env files (a first-class DocumentKind::DotEnv). - Defaults to follow_links(false) — had to set follow_links(true) so Bazel runfiles (symlinked test fixtures) are still walked. Built-in cycle detection keeps termination guaranteed. - Defaults to require_git(true) — had to set require_git(false) so a bare .gitignore in a markymark workspace is honored. Tests (markymark-mcp/src/engine/tests/mod.rs): - 5 regression tests for the SRE-specified scenarios (hard-ignore baseline, .gitignore respect, baseline without any ignore file, sort determinism, symlink-cycle termination). - 8 adversarial tests (empty dir, non-existent root, root-is-a-file, unicode filenames, second-run determinism, 64-level deep nesting, broken symlink, regular file named "target" — addresses the SRE filter_entry adversarial finding). - Ignored timing probe documenting 628 docs in 29.4ms on this worktree vs the prior session's > 60s hang. bazel test //... green (55 tests pass). Full details + failure catalog in the task skeleton.
- v6c closed: collect_documents ignore-filter landed (commit 14735dc) - marky-n1h opened (P0): markymark-mcp/src/engine/tests/mod.rs is 1413 lines. Pure code-motion refactor to split tests into per-topic submodules following the existing concurrency/curation/enrich pattern.
Capture the refactoring-diagnosis + refactoring-design output in the task skeleton so the next session has the full context: - Pre-refactor evidence (submodule sizes, inlined-test groups, shared fixtures, 7 smells + 3 test smells with file:line evidence) - Design decisions (pattern mapping, 4 invariants to preserve) - 10-step execution plan with per-step baseline-compare command - 5 open-at-execution decisions called out explicitly - 2 user-decision questions (parametrization + tautology deletion) - Anti-patterns extended (no env_mutation in workspace_scan, one commit per extraction) - Structural success criteria (no line count) Priority stays P2 — test file reorg is tech debt, not a blocker.
RUSTSEC-2025-0119 (number_prefix unmaintained) cleared by indicatif 0.17 -> 0.18 (now uses unit-prefix). hf-hub 0.4.3 -> 0.5.0, fastembed 5.13.0 -> 5.13.3, tokio 1.50.0 -> 1.52.1. Remaining: RUSTSEC-2024-0436 (paste unmaintained) still pulled via tokenizers 0.22.2 -> fastembed. Shows as warning only, not error; cargo audit exits 0. Upstream tokenizers issue. Also includes stale bones audit + p88 log entries from the v6c closeout session that didn't commit cleanly earlier.
Constraints were behind Cargo.lock for no reason: tokio: 1.42 -> 1.52 (lock: 1.52.1) fastembed: 5.11 -> 5.13 (lock: 5.13.3) insta: 1.42 -> 1.47 (lock: 1.47.2, dev-dep) libc: 0.2.182 -> 0.2 (precise pin was cosmetic; drops to minor range) No behavior change -- Cargo.lock already had these versions. This just makes the Cargo.toml intent match reality. Not bumped (needs separate task + API review): rmcp 0.15 -> 1.5 (major API migration) md5 0.7 -> 0.8 (need to audit our call sites)
rmcp 1.x marks model types as non_exhaustive, forcing construction
through the new builder-style constructors. Migration is mechanical:
ServerInfo { instructions, capabilities, ..default() }
-> ServerInfo::new(capabilities).with_instructions(instr)
PromptArgument { name, description, required, .. }
-> PromptArgument::new(name)
.with_description(desc).with_required(req)
GetPromptResult { description, messages }
-> GetPromptResult::new(messages).with_description(desc)
ReadResourceResult { contents } -> ReadResourceResult::new(contents)
All 357 default-feature tests pass. The trait surface (ServerHandler,
#[tool_router]/#[tool_handler]/#[tool] macros, RequestContext, ErrorData,
ServiceExt) is identical between 0.15 and 1.5 per the rmcp-sdk wiki.
md5 0.7 -> 0.8 is API-compatible (md5::compute still returns Digest);
only call site is the brza_kernels bench.
Also bumps doc: docs/rust_crates/rmcp.md Cargo.toml example.
Known pre-existing issue unrelated to this bump: tests under
--features semantic-search fail with RuntimeEngine field mismatches
(tracked in marky-c91).
Every caller passed a string that was discarded (prefix `_` on the param name). 55 call sites across 6 submodules. Pure parameter removal; signature: fn make_temp_realm_dir(_suffix: &str) -> tempfile::TempDir -> fn make_temp_realm_dir() -> tempfile::TempDir Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 3.
Move 5 HashEmbeddingProvider tests from engine/tests/mod.rs into engine/tests/hash_embedding.rs. Per-test #[cfg(feature = "semantic-search")] becomes a single module-level gate on the `mod hash_embedding` declaration in mod.rs -- matches the existing concurrency/preview_profiling pattern. Tests moved: fnv1a32_is_stable_and_deterministic hash_embedding_output_is_normalized_and_correct_dims hash_embedding_is_deterministic hash_embedding_rejects_empty_text hash_embedding_rejects_zero_dims Private `fnv1a32` from engine/mod.rs is reachable via `use super::*;` (descendant-private visibility). Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged under --no-default-features; hash_embedding tests compile out as before). marky-n1h step 4.
Move collect_documents_* suite and v6c_speedup_probe from mod.rs into engine/tests/workspace_scan.rs. Module header carries the v6c-specific hazard: do NOT env_mutation inside these tests (cargo parallel test, process-global env race). Tests moved (all now at engine::tests::workspace_scan::*): collect_documents_includes_json_alongside_markdown collect_documents_markdown_unchanged collect_documents_hard_ignore_baseline collect_documents_respects_gitignore collect_documents_baseline_without_ignore_file collect_documents_output_is_sorted collect_documents_terminates_on_symlink_cycle collect_documents_empty_dir collect_documents_nonexistent_root_does_not_panic collect_documents_root_is_a_file collect_documents_unicode_filenames collect_documents_is_deterministic_across_runs collect_documents_deep_nesting collect_documents_broken_symlink_is_skipped collect_documents_file_named_target_is_not_filtered v6c_speedup_probe (still #[ignore]d) Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 5.
Move get_outline_uses_named_realm + 9 outline_* / outline_tree_* tests from mod.rs into engine/tests/outline.rs. Tests moved (now at engine::tests::outline::*): get_outline_uses_named_realm outline_flat_format_backward_compat outline_tree_format_nested_hierarchy outline_tree_format_skipped_levels outline_tree_format_no_headings outline_tree_root_node_no_heading outline_include_text_false_omits_field outline_include_text_true_inlines_content outline_structured_doc_tree_fallback_to_flat outline_unicode_headings Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 6.
Move batch_indexed_* + engine_* + lto_eliminates_fault_injection from mod.rs into engine/tests/engine_indexing.rs. Open-at-execution decision: lto_eliminates_fault_injection HOME = engine_indexing.rs (not standalone lto.rs). Reason: the assertion is about engine-indexing behaviour under LTO, and its natural neighbour is engine_fallback_stale_on_update_failure (the non-LTO fault-injection companion). Single-test lto.rs would fragment the pair. Tests moved (now at engine::tests::engine_indexing::*): batch_indexed_docs_have_code_spans batch_indexed_docs_preserve_frontmatter engine_index_creates_persistent_engines engine_fallback_stale_on_update_failure lto_eliminates_fault_injection engine_cleanup_on_root_removal engine_frontmatter_preserved Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 7.
Move find_references_* tests from mod.rs into engine/tests/find_references.rs. Tests moved (now at engine::tests::find_references::*): find_references_uses_named_realm find_references_structured_doc_key_returns_empty_locations find_references_structured_doc_off_key_returns_error Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 8 (1/5).
Move rename_* tests from mod.rs into engine/tests/rename.rs. Tests moved (now at engine::tests::rename::*): rename_uses_named_realm rename_structured_doc_returns_not_supported_error Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 8 (2/5).
Move from_text_equivalence_* tests into engine/tests/from_text_equivalence.rs. These are sync #[test] (not #[tokio::test]) equivalence tests between DocumentIndex::from_text and fallback_scan_with_frontmatter. Tests moved (now at engine::tests::from_text_equivalence::*): from_text_equivalence_with_fallback_scan_mixed_doc from_text_equivalence_frontmatter_only_doc Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 8 (3/5).
Move export_index_uses_named_realm from mod.rs into engine/tests/export_index.rs. Open-at-execution decision: standalone file rather than absorbed into export_docs_index.rs. ExportIndex and ExportDocsIndex are different CoreOperation variants (DocumentExport vs DocsIndexExport result shapes). Grouping by operation, not by name similarity. Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 8 (4/5).
Move search_symbols_uses_named_realm from mod.rs into engine/tests/search_symbols.rs. Open-at-execution decision: standalone file rather than absorbed into engine::search::tests. The MCP-level test exercises realm routing through full RuntimeEngine dispatch; unit-level search ranking tests live in engine::search::tests (inside src/engine/search.rs). Different concerns. Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). After this commit, mod.rs contains only shared fixtures and module declarations -- no inlined tests remain. marky-n1h step 8 (5/5).
Promote make_engine_with_root(dir) from 3 submodule-local copies into the shared tests/mod.rs. Distinct from make_engine_with_custom_realm (uses default realm, no CreateRealm step). Before: same 10-line async fn duplicated in curation.rs:7, export_docs_index.rs:7, recommend.rs:7. After: one copy in tests/mod.rs; submodules pick it up via super::*. extract_curation_report / extract_entries / extract_recommendations stay local to their submodules (domain-specific CoreOperationResult variant matchers). Helpers-block comment annotated accordingly. Tests: 135 passed / 0 failed / 1 ignored (baseline unchanged). marky-n1h step 9.
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
rules_zig, Homebrew LLVM, andclang-lto-wrapperlibgcc_sstub +toolchains_llvm_bootstrapped0.5.9Changes
Cross-language ThinLTO
rules_zigpatches: cdeps flag filtering + two-step bitcode build forzig_static_library.bazelrcsplit:build:release(all platforms) +build:macos-lto(clang-lto-wrapper + ld64.lld)tools/clang-lto-wrapper.shstrips-plugin-optargs ld64.lld rejectsBazel primary build system
MODULE.bazel:apple_supportfor macOS,toolchains_llvm_bootstrapped0.5.9 (Linux-only), fastembed explicit specsemantic-search/local-embeddingsfeatures, bench-internals variant, criterion target.bazelrc: platform-specific config, CC toolchain flags, sandbox mounts, ONNXHOMEpassthroughCI fixes
libgcc_sstub for hermetic LLVM + rules_rust (experimental_stub_libgcc_s=True)toolchains_llvm_bootstrapped0.3.1 → 0.5.9Plugin
select-binary.sh/.ps1: prefer system-installed markymark over bundled.lsp.json: expanded file type mappings (JSON, YAML, TOML, .env, INI)plugin.json: removed inline server defs (now in.lsp.json/.mcp.json)Other
structured.rs:.mdxrecognized as Markdownscripts/install.sh: bash array for CONFIGS, quoted pathsQuality Gates
cargo fmt --check: passedcargo clippy --all-targets -D warnings: passedcargo test --workspace: passed (all suites, 0 failures)Test plan
bazel build //markymark-cli:markymarkbazel build --config=release --config=macos-lto //markymark-cli:markymarkbazel test --config=release //markymark-mcp:markymark-mcp_testselect-binary.shuses it.mdxfiles are recognized by LSPSummary by CodeRabbit
New Features
Documentation