diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 5a5a518e..d4c10d8b 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -27,3 +27,14 @@ Adding to this list requires explicit user approval and an unblock condition. Au ### Documentation Format - All docs `.adoc` (AsciiDoc) except GitHub-required files (SECURITY.md, CONTRIBUTING.md, CODE_OF_CONDUCT.md, CHANGELOG.md). + +--- + +## PR Workflow + +This repo squash-merges PRs. Two consequences worth knowing before pushing follow-ups: + +- **Don't pile follow-up commits onto a branch whose PR is in review.** When the PR is squash-merged, `main` gets a new commit with a new SHA. Any commits you pushed after the PR was opened are still on the feature branch, on top of a base that no longer matches `main`. GitHub will then mark the PR as `mergeable_state: "blocked"` and any rebase will produce ghost-conflicts — the conflicting hunks are the *same content*, but git can't tell because the SHAs differ. If you have follow-up work, open it as a new PR off the current `main`. +- **After a squash-merge, delete the feature branch.** It contains pre-squash commits with stale SHAs; reusing it for new work re-creates the ghost-conflict problem. `git checkout main && git pull && git branch -D && git push origin --delete `. + +Diagnostic: if a PR shows `blocked` and `git diff origin/main HEAD` is empty, the PR's content is already on main via squash-merge — close the PR rather than trying to merge it. diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 59e3d389..9ec27c09 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -139,3 +139,70 @@ jobs: name: benchmark-results path: ffi/zig/zig-out/bench* retention-days: 30 + + # ─── Bench Bridge: mcp-bridge JS perf (path-claims) ──────────────── + # Separate job from `benchmarks` above because the bridge has no Zig + # toolchain dependency — keeps logs untangled and lets the JS bench + # run in parallel on its own runner. Posts a sticky PR comment so + # deltas across pushes are visible inline. + bench-bridge: + name: Bench — mcp-bridge (path-claims) + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + # Per-job override: only this step needs to write PR comments. + # Workflow-level permissions stay read-all. + pull-requests: write + + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Setup Node + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 + with: + node-version: '22' + + - name: Run bridge bench + run: node mcp-bridge/tests/path_claims_bench.js | tee bench-bridge.txt + + - name: Upload bridge bench artifact + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: bench-bridge-results + path: bench-bridge.txt + retention-days: 30 + + - name: Sticky PR comment with bench numbers + if: github.event_name == 'pull_request' + # Advisory — a comment failure must never gate the bench job. + # Same reasoning as the hypatia-scan PR-comment step. + continue-on-error: true + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v7 + with: + script: | + const fs = require('fs'); + const MARKER = ''; + const output = fs.readFileSync('bench-bridge.txt', 'utf8'); + const body = `${MARKER}\n## 🏁 path-claims bench\n\nCommit \`${context.sha.slice(0, 7)}\`\n\n
Numbers\n\n\`\`\`\n${output}\n\`\`\`\n
\n\n*Host-dependent — compare deltas across commits, not absolute values.*`; + + const { owner, repo } = context.repo; + const issue_number = context.issue.number; + + const existing = await github.paginate( + github.rest.issues.listComments, + { owner, repo, issue_number, per_page: 100 }, + ); + const prior = existing.find((c) => c.body && c.body.includes(MARKER)); + + if (prior) { + await github.rest.issues.updateComment({ + owner, repo, comment_id: prior.id, body, + }); + } else { + await github.rest.issues.createComment({ + owner, repo, issue_number, body, + }); + } diff --git a/Justfile b/Justfile index e308ec0e..5ea928d4 100644 --- a/Justfile +++ b/Justfile @@ -354,6 +354,10 @@ bench: @echo "Running benchmarks..." cd ffi/zig && zig build bench +# Run the mcp-bridge JS benches (path-claims overlap scan, leaf primitives) +bench-bridge: + @node mcp-bridge/tests/path_claims_bench.js + # Run end-to-end integration tests integration: @echo "Running integration tests..." diff --git a/coord-tui/shell/coord-hooks.sh b/coord-tui/shell/coord-hooks.sh index a1a70383..bb277d7a 100644 --- a/coord-tui/shell/coord-hooks.sh +++ b/coord-tui/shell/coord-hooks.sh @@ -36,6 +36,27 @@ _coord_post() { -d "$payload" 2>/dev/null } +# Quiet claim — echoes the raw backend response, sets the return code +# (0 on success, 1 otherwise). Shared by coord-claim and coord-worktree +# so both interpret "granted" the same way. +_coord_claim_quiet() { + local task="$1" + local tok; tok="$(_coord_token)" + [ -z "$tok" ] && return 2 + local resp + resp=$(_coord_post coord_claim_task \ + "{\"token\":\"$tok\",\"task\":\"$task\"}" 2>/dev/null) + printf '%s' "$resp" + printf '%s' "$resp" | python3 -c " +import sys, json +try: + d = json.load(sys.stdin) + sys.exit(0 if d.get('success') else 1) +except Exception: + sys.exit(1) +" 2>/dev/null +} + _coord_auto_register() { local kind="$1" # Registers silently, writes ~/.cache/coord-tui/peer.env, sets window title. @@ -98,25 +119,27 @@ else: # Claim a task: coord-claim hypatia/my-task coord-claim() { local task="${1:?Usage: coord-claim }" - local tok; tok="$(_coord_token)" - if [ -z "$tok" ]; then - echo "Not registered — run: coord-tui --id --kind claude" >&2; return 1 + local resp rc + resp="$(_coord_claim_quiet "$task")"; rc=$? + if [ $rc -eq 2 ]; then + echo "Not registered — run: coord-tui --id --kind claude" >&2 + return 1 fi - local result - result=$(_coord_post coord_claim_task \ - "{\"token\":\"$tok\",\"task\":\"$task\"}" 2>/dev/null) - echo "$result" | python3 -c " -import sys, json + if [ -z "$resp" ]; then + echo " ✗ Failed (adapter not running?)" + return 1 + fi + printf '%s' "$resp" | TASK="$task" python3 -c " +import sys, os, json d = json.load(sys.stdin) +task = os.environ.get('TASK', '') if d.get('success'): msg = d.get('message','') - if msg == 'granted': - print(f' ✓ Claimed: $task') - else: - print(f' ✗ {msg}') + print(f' ✓ Claimed: {task}' if msg == 'granted' else f' ✗ {msg}') else: print(f' ✗ {d.get(\"error\",\"unknown error\")}') -" 2>/dev/null || echo " ✗ Failed (adapter not running?)" +" 2>/dev/null + return $rc } # Claim a task AND provision an isolated git worktree for it. @@ -149,28 +172,15 @@ coord-worktree() { local branch="agent/${peer}/${safe}" # Claim first — if the backend says no, don't touch the working tree. - local tok; tok="$(_coord_token)" - if [ -n "$tok" ]; then - local claim - claim=$(_coord_post coord_claim_task \ - "{\"token\":\"$tok\",\"task\":\"$task\"}" 2>/dev/null) - local ok - ok=$(echo "$claim" | python3 -c " -import sys, json -try: - d = json.load(sys.stdin) - print('yes' if d.get('success') else 'no') -except Exception: - print('no') -" 2>/dev/null) - if [ "$ok" != "yes" ]; then - echo " ✗ Claim refused — not provisioning worktree." >&2 - echo "$claim" >&2 - return 1 - fi - else - echo " ! No coord token — provisioning worktree without claim." >&2 - fi + local resp rc + resp="$(_coord_claim_quiet "$task")"; rc=$? + case $rc in + 0) ;; # granted + 2) echo " ! No coord token — provisioning worktree without claim." >&2 ;; + *) echo " ✗ Claim refused — not provisioning worktree." >&2 + [ -n "$resp" ] && echo "$resp" >&2 + return 1 ;; + esac mkdir -p "$(dirname "$wt_dir")" if [ -d "$wt_dir" ]; then diff --git a/mcp-bridge/lib/dispatcher.js b/mcp-bridge/lib/dispatcher.js index 741158a8..c9ad5a85 100644 --- a/mcp-bridge/lib/dispatcher.js +++ b/mcp-bridge/lib/dispatcher.js @@ -220,16 +220,7 @@ async function dispatchLocalCoord(toolName, args) { } } - // Advisory path-claims are bridge-layer only — strip from the - // payload forwarded to the verified Zig backend so its schema stays - // unchanged. Backend stays the source of truth for ownership. - let declaredPaths; - let forwarded = args || {}; - if (toolName === "coord_claim_task" && args && Array.isArray(args.paths)) { - declaredPaths = args.paths; - const { paths, ...rest } = args; - forwarded = rest; - } + const { forwarded, ctx } = pathClaimsBefore(toolName, args); try { const res = await fetch(`${LOCAL_COORD_URL}/tools/${toolName}`, { @@ -243,7 +234,7 @@ async function dispatchLocalCoord(toolName, args) { } catch { return { success: false, error: "local-coord-mcp backend returned non-JSON" }; } - return annotatePathClaims(toolName, args, data, declaredPaths); + return pathClaimsAfter(toolName, args, data, ctx); } catch (e) { return { success: false, @@ -253,27 +244,38 @@ async function dispatchLocalCoord(toolName, args) { } } -function annotatePathClaims(toolName, args, data, declaredPaths) { +// Path-claims live entirely at the bridge layer. The verified backend +// has no `paths` field on coord_claim_task, so we strip it before +// forwarding and stash it for the post-response annotate step. +function pathClaimsBefore(toolName, args) { + const a = args || {}; + if (toolName === "coord_claim_task" && Array.isArray(a.paths)) { + const { paths, ...rest } = a; + return { forwarded: rest, ctx: { declaredPaths: paths } }; + } + return { forwarded: a, ctx: {} }; +} + +function pathClaimsAfter(toolName, args, data, ctx) { if (!data || typeof data !== "object") return data; const task = args?.task; switch (toolName) { case "coord_claim_task": { - if (!declaredPaths || !task || data.success === false) return data; - const holder = data.holder || "(unknown)"; - const ttl_s = typeof data.ttl_s === "number" ? data.ttl_s : undefined; + if (!ctx.declaredPaths || !task || data.success === false) return data; const { paths, overlaps } = pathClaims.register({ - task, holder, paths: declaredPaths, ttl_s, + task, + holder: data.holder || "(unknown)", + paths: ctx.declaredPaths, + ttl_s: typeof data.ttl_s === "number" ? data.ttl_s : undefined, }); return { ...data, declared_paths: paths, path_overlap: overlaps }; } - case "coord_progress": { + case "coord_progress": if (task) pathClaims.refresh(task, data.ttl_s); return data; - } - case "coord_report_outcome": { + case "coord_report_outcome": if (task) pathClaims.release(task); return data; - } default: return data; } diff --git a/mcp-bridge/lib/path-claims.js b/mcp-bridge/lib/path-claims.js index a7a5769d..20bf803c 100644 --- a/mcp-bridge/lib/path-claims.js +++ b/mcp-bridge/lib/path-claims.js @@ -4,45 +4,62 @@ // Advisory path-claims for local-coord-mcp. // // The Zig/Idris backend enforces a task-id mutex; this module layers an -// in-bridge advisory map of `task -> {holder, paths, expires_at}` so +// in-bridge advisory map of `task -> {holder, segs, expires_at}` so // `coord_claim_task` can return `path_overlap` hints when two active // claims declare overlapping working-tree paths. Advisory by design: // the backend is the source of truth for ownership; this module never // rejects a claim, it only annotates the response. -const _claims = new Map(); // task -> { holder, paths, expires_at_ms } +const DEFAULT_TTL_S = 300; +const _claims = new Map(); // task -> { holder, paths, segs, expires_at_ms } function normalize(p) { if (typeof p !== "string") return null; let s = p.trim(); if (!s) return null; - s = s.replace(/\\/g, "/"); - while (s.includes("//")) s = s.replace(/\/\//g, "/"); - if (s.endsWith("/") && s.length > 1) s = s.slice(0, -1); + s = s.replace(/\\/g, "/").replace(/\/+/g, "/"); if (s.startsWith("./")) s = s.slice(2); - return s; + if (s.length > 1 && s.endsWith("/")) s = s.slice(0, -1); + return s || null; } -function segments(p) { +function toSegments(p) { return p.split("/").filter((s) => s !== "" && s !== "."); } -// Two paths overlap when one is a segment-prefix of the other (or equal). -// `src/a` overlaps `src/a/b` and `src/a`, but NOT `src/abc`. -export function pathsOverlap(a, b) { - const A = segments(a); - const B = segments(b); +function segmentPrefixMatch(A, B) { const n = Math.min(A.length, B.length); for (let i = 0; i < n; i++) if (A[i] !== B[i]) return false; return true; } +// Two paths overlap when one is a segment-prefix of the other (or equal). +// `src/a` overlaps `src/a/b` and `src/a`, but NOT `src/abc`. +// Kept as a stable public helper for tests and external use. +export function pathsOverlap(a, b) { + return segmentPrefixMatch(toSegments(a), toSegments(b)); +} + function sweep(nowMs = Date.now()) { for (const [task, entry] of _claims) { if (entry.expires_at_ms && entry.expires_at_ms <= nowMs) _claims.delete(task); } } +function ttlMs(ttl_s) { + return (typeof ttl_s === "number" && ttl_s > 0 ? ttl_s : DEFAULT_TTL_S) * 1000; +} + +function normalizePaths(paths) { + if (!Array.isArray(paths)) return []; + const out = []; + for (const p of paths) { + const n = normalize(p); + if (n) out.push(n); + } + return out; +} + /** * Register an advisory path-claim and return overlap warnings with * other *active* claims (excluding the same task by the same holder). @@ -56,32 +73,37 @@ function sweep(nowMs = Date.now()) { */ export function register({ task, holder, paths, ttl_s }) { sweep(); - const norm = Array.isArray(paths) - ? paths.map(normalize).filter(Boolean) - : []; + const norm = normalizePaths(paths); + const newSegs = norm.map(toSegments); + const overlaps = []; for (const [otherTask, other] of _claims) { if (otherTask === task && other.holder === holder) continue; - const hits = []; - for (const a of norm) { - for (const b of other.paths) { - if (pathsOverlap(a, b)) hits.push(a); + const hits = new Set(); + for (let i = 0; i < newSegs.length; i++) { + const a = newSegs[i]; + for (const b of other.segs) { + if (segmentPrefixMatch(a, b)) { + hits.add(norm[i]); + break; + } } } - if (hits.length) { + if (hits.size) { overlaps.push({ task: otherTask, holder: other.holder, paths: other.paths.slice(), - with: Array.from(new Set(hits)), + with: Array.from(hits), }); } } - const ttl = typeof ttl_s === "number" && ttl_s > 0 ? ttl_s : 300; + _claims.set(task, { holder, paths: norm, - expires_at_ms: Date.now() + ttl * 1000, + segs: newSegs, + expires_at_ms: Date.now() + ttlMs(ttl_s), }); return { paths: norm, overlaps }; } @@ -90,8 +112,7 @@ export function register({ task, holder, paths, ttl_s }) { export function refresh(task, ttl_s) { const entry = _claims.get(task); if (!entry) return false; - const ttl = typeof ttl_s === "number" && ttl_s > 0 ? ttl_s : 300; - entry.expires_at_ms = Date.now() + ttl * 1000; + entry.expires_at_ms = Date.now() + ttlMs(ttl_s); return true; } diff --git a/mcp-bridge/tests/path_claims_bench.js b/mcp-bridge/tests/path_claims_bench.js new file mode 100644 index 00000000..e6e69348 --- /dev/null +++ b/mcp-bridge/tests/path_claims_bench.js @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2026 Jonathan D.A. Jewell (hyperpolymath) +// +// Bench harness for mcp-bridge/lib/path-claims.js. +// +// Run: node mcp-bridge/tests/path_claims_bench.js +// Or: just bench-bridge +// +// Path-claims sit on every coord_claim_task call, so the overlap scan +// is on the hot path for multi-agent coordination. Real workloads are +// small (~10-50 active claims), but stress scenarios sanity-check the +// O(n) register scan + the cached segment-split optimisation from the +// post-#142 refactor. Output is stable so commits can be diffed. + +import { + register, + refresh, + release, + list, + pathsOverlap, + _reset, +} from "../lib/path-claims.js"; + +const NS_PER_MS = 1_000_000n; + +function bench(name, iters, fn) { + // Warmup: ~10% of iters or 1k, whichever is smaller. + const warm = Math.min(iters / 10 | 0, 1000); + for (let i = 0; i < warm; i++) fn(i); + const t0 = process.hrtime.bigint(); + for (let i = 0; i < iters; i++) fn(i); + const t1 = process.hrtime.bigint(); + const totalNs = t1 - t0; + const nsPerOp = Number(totalNs) / iters; + const opsPerSec = iters / (Number(totalNs) / 1e9); + return { name, iters, totalMs: Number(totalNs / NS_PER_MS), nsPerOp, opsPerSec }; +} + +function fmtRow({ name, iters, totalMs, nsPerOp, opsPerSec }) { + const ns = nsPerOp < 1000 + ? `${nsPerOp.toFixed(1)} ns/op` + : nsPerOp < 1_000_000 + ? `${(nsPerOp / 1000).toFixed(2)} µs/op` + : `${(nsPerOp / 1_000_000).toFixed(2)} ms/op`; + const ops = opsPerSec > 1e6 + ? `${(opsPerSec / 1e6).toFixed(2)}M ops/s` + : opsPerSec > 1e3 + ? `${(opsPerSec / 1e3).toFixed(1)}k ops/s` + : `${opsPerSec.toFixed(0)} ops/s`; + return ` ${name.padEnd(50)} ${String(iters).padStart(8)} iters ${String(totalMs).padStart(5)} ms ${ns.padStart(14)} ${ops.padStart(14)}`; +} + +function seedClaims(n, pathsPerClaim) { + _reset(); + for (let i = 0; i < n; i++) { + const paths = []; + for (let j = 0; j < pathsPerClaim; j++) { + paths.push(`pkg-${i}/mod-${j}/file-${i}-${j}.js`); + } + register({ task: `seed-${i}`, holder: `peer-${i % 5}`, paths, ttl_s: 3600 }); + } +} + +const SCENARIOS = [ + // Realistic: 10 active claims, declaring 3 paths each. Typical multi- + // agent workstation: 3-5 peers, 1-3 claims apiece. + { name: "register: 10 active claims, 3 new paths", seed: 10, declared: 3, iters: 50_000 }, + // Heavier: 100 claims. Sustained multi-day session. + { name: "register: 100 active claims, 3 new paths", seed: 100, declared: 3, iters: 20_000 }, + // Stress: 1000 claims. Beyond design point — measures graceful scaling. + { name: "register: 1000 active claims, 3 new paths", seed: 1000, declared: 3, iters: 5_000 }, + // Wide declared paths (rare but possible — a sweeping refactor). + { name: "register: 100 active claims, 20 new paths", seed: 100, declared: 20, iters: 5_000 }, +]; + +console.log("path-claims bench (node " + process.version + ")\n"); +console.log(" " + "scenario".padEnd(50) + " iters ms ns/op ops/s"); +console.log(" " + "-".repeat(110)); + +for (const s of SCENARIOS) { + seedClaims(s.seed, 3); + // Pre-build the declared-paths array once so we measure register(), + // not array construction. + const declared = []; + for (let j = 0; j < s.declared; j++) { + declared.push(`pkg-new/feature/file-${j}.js`); + } + // Reuse one task slot — each iter is an update against the same + // seeded population, so active-claim count stays at s.seed and the + // overlap-scan cost is what we actually want to measure. + const result = bench(s.name, s.iters, () => { + register({ + task: "hot-task", + holder: "peer-hot", + paths: declared, + ttl_s: 3600, + }); + }); + console.log(fmtRow(result)); +} + +// Leaf primitive — drives the inner loop in register(). Use long-ish +// paths to exercise the segment comparator under realistic depth. +const aLong = "cartridges/local-coord-mcp/abi/LocalCoord/Identity.idr"; +const bLong = "cartridges/local-coord-mcp/abi/LocalCoord/Auth.idr"; +const aShort = "src/foo"; +const bShort = "src/foo/bar/baz.js"; + +console.log(); +console.log(fmtRow(bench("pathsOverlap: deep diverge at segment 4", 1_000_000, () => { + pathsOverlap(aLong, bLong); +}))); +console.log(fmtRow(bench("pathsOverlap: short prefix match", 1_000_000, () => { + pathsOverlap(aShort, bShort); +}))); + +// refresh/release/list — bookkeeping that fires on coord_progress and +// coord_report_outcome. Measure to catch accidental regressions. +seedClaims(100, 3); +console.log(); +let r = 0; +console.log(fmtRow(bench("refresh (existing claim)", 100_000, () => { + refresh(`seed-${r++ % 100}`, 300); +}))); +seedClaims(100, 3); +let q = 0; +console.log(fmtRow(bench("list (100 active claims)", 50_000, () => { + list(); + q++; +}))); + +console.log("\n (Bench numbers depend on host; use deltas across commits, not absolute values.)");