Skip to content

Commit d21a2a8

Browse files
refactor(coord): tighten path-claims/dispatcher + bench harness + CI integration (#143)
## Summary Post-merge cleanup on top of #142. Three commits, all on top of the squash-merged #142 content: 1. **`6f861a1` — refactor.** Tighten `path-claims.js` (cache segment-splits, single-regex `//` normalise, extract `DEFAULT_TTL_S`); split `dispatcher.dispatchLocalCoord` into explicit `pathClaimsBefore`/`pathClaimsAfter` hooks; DRY the shell helpers (`_coord_claim_quiet` shared between `coord-claim` and `coord-worktree`). Adds a `PR Workflow` section to `.claude/CLAUDE.md` documenting the squash-merge + follow-up-commit hazard that produced #142's ghost-conflict. 2. **`9bb7df5` — bench.** `mcp-bridge/tests/path_claims_bench.js` + `just bench-bridge` recipe. Reference numbers on dev host: 240k ops/s at 10 active claims, 3.9k ops/s at 1000 claims, `pathsOverlap` ~170 ns/op. 3. **`f1e646a` — CI.** New `bench-bridge` job in `e2e.yml`: runs the bench, uploads artifact, posts a **sticky** PR comment (marker-tag find-or-update) so bench deltas show up inline across pushes. Per-job `pull-requests: write` override keeps workflow-level perms at `read-all`. Comment failure is non-fatal (`continue-on-error`) — token hiccups never gate the bench. ## Behavioural changes None. All 26 bridge tests stay green. Backend (Idris2/Zig) untouched. ## Test plan - [x] `node --test mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/path_claims_test.js` → 26/26 pass locally - [x] `node mcp-bridge/tests/path_claims_bench.js` → completes in ~3s, stable output - [x] `python3 -c "import yaml; yaml.safe_load(open('.github/workflows/e2e.yml'))"` → valid YAML - [ ] First PR push triggers `bench-bridge` job; sticky comment appears with numbers - [ ] Second push to same PR updates the sticky comment in place (no spam) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _Generated by [Claude Code](https://claude.ai/code/session_018MBrAtPrwfgn2WG4BAerZW)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0a61760 commit d21a2a8

7 files changed

Lines changed: 327 additions & 80 deletions

File tree

.claude/CLAUDE.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,14 @@ Adding to this list requires explicit user approval and an unblock condition. Au
2727
### Documentation Format
2828

2929
- All docs `.adoc` (AsciiDoc) except GitHub-required files (SECURITY.md, CONTRIBUTING.md, CODE_OF_CONDUCT.md, CHANGELOG.md).
30+
31+
---
32+
33+
## PR Workflow
34+
35+
This repo squash-merges PRs. Two consequences worth knowing before pushing follow-ups:
36+
37+
- **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`.
38+
- **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 <branch> && git push origin --delete <branch>`.
39+
40+
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.

.github/workflows/e2e.yml

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,3 +139,70 @@ jobs:
139139
name: benchmark-results
140140
path: ffi/zig/zig-out/bench*
141141
retention-days: 30
142+
143+
# ─── Bench Bridge: mcp-bridge JS perf (path-claims) ────────────────
144+
# Separate job from `benchmarks` above because the bridge has no Zig
145+
# toolchain dependency — keeps logs untangled and lets the JS bench
146+
# run in parallel on its own runner. Posts a sticky PR comment so
147+
# deltas across pushes are visible inline.
148+
bench-bridge:
149+
name: Bench — mcp-bridge (path-claims)
150+
runs-on: ubuntu-latest
151+
timeout-minutes: 5
152+
permissions:
153+
contents: read
154+
# Per-job override: only this step needs to write PR comments.
155+
# Workflow-level permissions stay read-all.
156+
pull-requests: write
157+
158+
steps:
159+
- name: Checkout
160+
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
161+
162+
- name: Setup Node
163+
uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
164+
with:
165+
node-version: '22'
166+
167+
- name: Run bridge bench
168+
run: node mcp-bridge/tests/path_claims_bench.js | tee bench-bridge.txt
169+
170+
- name: Upload bridge bench artifact
171+
if: always()
172+
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4
173+
with:
174+
name: bench-bridge-results
175+
path: bench-bridge.txt
176+
retention-days: 30
177+
178+
- name: Sticky PR comment with bench numbers
179+
if: github.event_name == 'pull_request'
180+
# Advisory — a comment failure must never gate the bench job.
181+
# Same reasoning as the hypatia-scan PR-comment step.
182+
continue-on-error: true
183+
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v7
184+
with:
185+
script: |
186+
const fs = require('fs');
187+
const MARKER = '<!-- bench-bridge:path-claims -->';
188+
const output = fs.readFileSync('bench-bridge.txt', 'utf8');
189+
const body = `${MARKER}\n## 🏁 path-claims bench\n\nCommit \`${context.sha.slice(0, 7)}\`\n\n<details open><summary>Numbers</summary>\n\n\`\`\`\n${output}\n\`\`\`\n</details>\n\n*Host-dependent — compare deltas across commits, not absolute values.*`;
190+
191+
const { owner, repo } = context.repo;
192+
const issue_number = context.issue.number;
193+
194+
const existing = await github.paginate(
195+
github.rest.issues.listComments,
196+
{ owner, repo, issue_number, per_page: 100 },
197+
);
198+
const prior = existing.find((c) => c.body && c.body.includes(MARKER));
199+
200+
if (prior) {
201+
await github.rest.issues.updateComment({
202+
owner, repo, comment_id: prior.id, body,
203+
});
204+
} else {
205+
await github.rest.issues.createComment({
206+
owner, repo, issue_number, body,
207+
});
208+
}

Justfile

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,10 @@ bench:
354354
@echo "Running benchmarks..."
355355
cd ffi/zig && zig build bench
356356

357+
# Run the mcp-bridge JS benches (path-claims overlap scan, leaf primitives)
358+
bench-bridge:
359+
@node mcp-bridge/tests/path_claims_bench.js
360+
357361
# Run end-to-end integration tests
358362
integration:
359363
@echo "Running integration tests..."

coord-tui/shell/coord-hooks.sh

Lines changed: 45 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,27 @@ _coord_post() {
3636
-d "$payload" 2>/dev/null
3737
}
3838

39+
# Quiet claim — echoes the raw backend response, sets the return code
40+
# (0 on success, 1 otherwise). Shared by coord-claim and coord-worktree
41+
# so both interpret "granted" the same way.
42+
_coord_claim_quiet() {
43+
local task="$1"
44+
local tok; tok="$(_coord_token)"
45+
[ -z "$tok" ] && return 2
46+
local resp
47+
resp=$(_coord_post coord_claim_task \
48+
"{\"token\":\"$tok\",\"task\":\"$task\"}" 2>/dev/null)
49+
printf '%s' "$resp"
50+
printf '%s' "$resp" | python3 -c "
51+
import sys, json
52+
try:
53+
d = json.load(sys.stdin)
54+
sys.exit(0 if d.get('success') else 1)
55+
except Exception:
56+
sys.exit(1)
57+
" 2>/dev/null
58+
}
59+
3960
_coord_auto_register() {
4061
local kind="$1"
4162
# Registers silently, writes ~/.cache/coord-tui/peer.env, sets window title.
@@ -98,25 +119,27 @@ else:
98119
# Claim a task: coord-claim hypatia/my-task
99120
coord-claim() {
100121
local task="${1:?Usage: coord-claim <task-name>}"
101-
local tok; tok="$(_coord_token)"
102-
if [ -z "$tok" ]; then
103-
echo "Not registered — run: coord-tui --id --kind claude" >&2; return 1
122+
local resp rc
123+
resp="$(_coord_claim_quiet "$task")"; rc=$?
124+
if [ $rc -eq 2 ]; then
125+
echo "Not registered — run: coord-tui --id --kind claude" >&2
126+
return 1
104127
fi
105-
local result
106-
result=$(_coord_post coord_claim_task \
107-
"{\"token\":\"$tok\",\"task\":\"$task\"}" 2>/dev/null)
108-
echo "$result" | python3 -c "
109-
import sys, json
128+
if [ -z "$resp" ]; then
129+
echo " ✗ Failed (adapter not running?)"
130+
return 1
131+
fi
132+
printf '%s' "$resp" | TASK="$task" python3 -c "
133+
import sys, os, json
110134
d = json.load(sys.stdin)
135+
task = os.environ.get('TASK', '')
111136
if d.get('success'):
112137
msg = d.get('message','')
113-
if msg == 'granted':
114-
print(f' ✓ Claimed: $task')
115-
else:
116-
print(f' ✗ {msg}')
138+
print(f' ✓ Claimed: {task}' if msg == 'granted' else f' ✗ {msg}')
117139
else:
118140
print(f' ✗ {d.get(\"error\",\"unknown error\")}')
119-
" 2>/dev/null || echo " ✗ Failed (adapter not running?)"
141+
" 2>/dev/null
142+
return $rc
120143
}
121144

122145
# Claim a task AND provision an isolated git worktree for it.
@@ -149,28 +172,15 @@ coord-worktree() {
149172
local branch="agent/${peer}/${safe}"
150173

151174
# Claim first — if the backend says no, don't touch the working tree.
152-
local tok; tok="$(_coord_token)"
153-
if [ -n "$tok" ]; then
154-
local claim
155-
claim=$(_coord_post coord_claim_task \
156-
"{\"token\":\"$tok\",\"task\":\"$task\"}" 2>/dev/null)
157-
local ok
158-
ok=$(echo "$claim" | python3 -c "
159-
import sys, json
160-
try:
161-
d = json.load(sys.stdin)
162-
print('yes' if d.get('success') else 'no')
163-
except Exception:
164-
print('no')
165-
" 2>/dev/null)
166-
if [ "$ok" != "yes" ]; then
167-
echo " ✗ Claim refused — not provisioning worktree." >&2
168-
echo "$claim" >&2
169-
return 1
170-
fi
171-
else
172-
echo " ! No coord token — provisioning worktree without claim." >&2
173-
fi
175+
local resp rc
176+
resp="$(_coord_claim_quiet "$task")"; rc=$?
177+
case $rc in
178+
0) ;; # granted
179+
2) echo " ! No coord token — provisioning worktree without claim." >&2 ;;
180+
*) echo " ✗ Claim refused — not provisioning worktree." >&2
181+
[ -n "$resp" ] && echo "$resp" >&2
182+
return 1 ;;
183+
esac
174184

175185
mkdir -p "$(dirname "$wt_dir")"
176186
if [ -d "$wt_dir" ]; then

mcp-bridge/lib/dispatcher.js

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -220,16 +220,7 @@ async function dispatchLocalCoord(toolName, args) {
220220
}
221221
}
222222

223-
// Advisory path-claims are bridge-layer only — strip from the
224-
// payload forwarded to the verified Zig backend so its schema stays
225-
// unchanged. Backend stays the source of truth for ownership.
226-
let declaredPaths;
227-
let forwarded = args || {};
228-
if (toolName === "coord_claim_task" && args && Array.isArray(args.paths)) {
229-
declaredPaths = args.paths;
230-
const { paths, ...rest } = args;
231-
forwarded = rest;
232-
}
223+
const { forwarded, ctx } = pathClaimsBefore(toolName, args);
233224

234225
try {
235226
const res = await fetch(`${LOCAL_COORD_URL}/tools/${toolName}`, {
@@ -243,7 +234,7 @@ async function dispatchLocalCoord(toolName, args) {
243234
} catch {
244235
return { success: false, error: "local-coord-mcp backend returned non-JSON" };
245236
}
246-
return annotatePathClaims(toolName, args, data, declaredPaths);
237+
return pathClaimsAfter(toolName, args, data, ctx);
247238
} catch (e) {
248239
return {
249240
success: false,
@@ -253,27 +244,38 @@ async function dispatchLocalCoord(toolName, args) {
253244
}
254245
}
255246

256-
function annotatePathClaims(toolName, args, data, declaredPaths) {
247+
// Path-claims live entirely at the bridge layer. The verified backend
248+
// has no `paths` field on coord_claim_task, so we strip it before
249+
// forwarding and stash it for the post-response annotate step.
250+
function pathClaimsBefore(toolName, args) {
251+
const a = args || {};
252+
if (toolName === "coord_claim_task" && Array.isArray(a.paths)) {
253+
const { paths, ...rest } = a;
254+
return { forwarded: rest, ctx: { declaredPaths: paths } };
255+
}
256+
return { forwarded: a, ctx: {} };
257+
}
258+
259+
function pathClaimsAfter(toolName, args, data, ctx) {
257260
if (!data || typeof data !== "object") return data;
258261
const task = args?.task;
259262
switch (toolName) {
260263
case "coord_claim_task": {
261-
if (!declaredPaths || !task || data.success === false) return data;
262-
const holder = data.holder || "(unknown)";
263-
const ttl_s = typeof data.ttl_s === "number" ? data.ttl_s : undefined;
264+
if (!ctx.declaredPaths || !task || data.success === false) return data;
264265
const { paths, overlaps } = pathClaims.register({
265-
task, holder, paths: declaredPaths, ttl_s,
266+
task,
267+
holder: data.holder || "(unknown)",
268+
paths: ctx.declaredPaths,
269+
ttl_s: typeof data.ttl_s === "number" ? data.ttl_s : undefined,
266270
});
267271
return { ...data, declared_paths: paths, path_overlap: overlaps };
268272
}
269-
case "coord_progress": {
273+
case "coord_progress":
270274
if (task) pathClaims.refresh(task, data.ttl_s);
271275
return data;
272-
}
273-
case "coord_report_outcome": {
276+
case "coord_report_outcome":
274277
if (task) pathClaims.release(task);
275278
return data;
276-
}
277279
default:
278280
return data;
279281
}

0 commit comments

Comments
 (0)