-
Notifications
You must be signed in to change notification settings - Fork 0
plan: mask-membrane-valhalla-integration-v1 (the layered consolidation) #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+593
−0
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f3e0ceb
plan: mask-membrane-valhalla-integration-v1 (the layered consolidation)
claude 1ed338d
plan: apply the three Codex P2 corrections
claude 0ffd677
plan: apply the five CodeRabbit findings
claude 30986b4
plan: W0 scope honesty + the standing gate-observability question
claude e52bc3a
plan: check-then-read atomicity is a mandatory W1.1 decision output
claude 586d081
plan: measurement runs under the chosen atomicity arm + knowledge: Gi…
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # GitHub access paths — three credentials, one rate-limit identity trap | ||
|
|
||
| > READ BY: any session doing PR babysitting, review-thread resolution, | ||
| > or hitting a GitHub 403/RATE_LIMIT in this environment. | ||
| > Measured 2026-08-28 (PR #47/#48 review cycle), every row exercised | ||
| > live — none of this is inferred. | ||
|
|
||
| ## The core fact most sessions get wrong | ||
|
|
||
| There are THREE ways out of this container to GitHub, and they carry | ||
| only TWO identities: | ||
|
|
||
| | path | credential | identity | | ||
| |---|---|---| | ||
| | MCP `mcp__github__*` tools | the GitHub App session credential | **user ID 200276742** | | ||
| | direct `api.github.com` with `GH_TOKEN` (proxy-bypassed: `curl --noproxy '*'` / Python `ProxyHandler({})`) | the env token (`ghp_…`, strip the wrapping quotes) | **the SAME user ID 200276742** | | ||
| | through the session proxy (default env, NO Authorization header you author) | the proxy attaches its OWN credential | **a different identity** | | ||
|
|
||
| Consequence: when the account hits a rate limit, **switching from MCP to | ||
| pygithub/curl-with-GH_TOKEN changes nothing** — same identity, same | ||
| limit. The tesseract-rs "a 403 is usually the proxy" lesson is about | ||
| *authorization* 403s; it does NOT transfer to *rate* limits. The only | ||
| path with a different budget is the proxy's own credential. | ||
|
|
||
| ## What each path can actually do (measured) | ||
|
|
||
| | operation | MCP | direct GH_TOKEN | session proxy | | ||
| |---|---|---|---| | ||
| | REST reads (PRs, comments) | ✅ (until limited) | ✅ (until limited) | ✅ `200` | | ||
| | REST writes (review-comment **replies**: `POST /pulls/{n}/comments/{id}/replies`) | ✅ | ✅ | ✅ **works — posted live** | | ||
| | GraphQL, arbitrary queries | ✅ | ✅ | ❌ "only the pinned set of PR-review operations is served" | | ||
| | GraphQL `resolveReviewThread` mutation | ✅ (via `resolve_review_thread`) | ✅ | ❌ rejected — NOT in the pinned set | | ||
| | git fetch/push | n/a | ✅ (one-shot token URL rules apply) | ✅ (plain `git push origin`; proxy carries auth) | | ||
|
|
||
| So **review-thread resolution is the one operation with no fallback**: | ||
| it is GraphQL-only, the proxy's pinned set excludes the mutation, and | ||
| both non-proxy paths share the limited identity. When the account is | ||
| limited, the only correct move is to wait for the reset and re-arm a | ||
| check-in — do not burn turns cycling transports. | ||
|
|
||
| ## Recognizing a SECONDARY (abuse) limit | ||
|
|
||
| Signature, observed live: `GET /rate_limit` reports **5000/5000 | ||
| remaining** while GraphQL simultaneously returns | ||
| `RATE_LIMIT: API rate limit already exceeded for user ID …`. The quota | ||
| endpoint reflects only the primary budget; secondary limits are | ||
| invisible there. The `reset` timestamp the quota endpoint names is | ||
| still the best available estimate of the cool-off (observed: the limit | ||
| held until roughly that time). Do not retry in a loop — secondary | ||
| limits extend under hammering. | ||
|
|
||
| ## Proxy-path mechanics that save a session ten minutes | ||
|
|
||
| - Plain `curl https://api.github.com/...` with the default env goes | ||
| THROUGH the proxy and gets the proxy credential automatically — do | ||
| not add an `Authorization` header; authoring one replaces the good | ||
| credential with the limited one. | ||
| - The proxy's GraphQL rejection message suggests `gh api` — `gh` is not | ||
| installed here; plain REST via curl/urllib is the equivalent. | ||
| - Replies posted via the proxy land as `claude[bot]`. CodeRabbit | ||
| auto-skips engaging with other bots' replies ("Skipped: comment is | ||
| from another GitHub bot") — expected, not a failure. | ||
| - Token hygiene rules (never in URLs, never printed, strip the wrapping | ||
| quotes inline) are unchanged and live in the sibling repos' | ||
| CLAUDE.md files (MedCare-rs "GH_TOKEN" section is the canonical | ||
| statement); this doc adds the identity/limit map, not new hygiene. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| # epoch-recheck-phase0-v1 — the W1.1 council SPEC (Phase 0, 5+3 protocol) | ||
|
|
||
| **Status: SPEC READY (2026-08-28) — Phase 1 (the 5 savants) not yet cast.** | ||
| This is the Phase-0 specification for resolving `ISS-LGJ-EPOCH-UNCHECKED` | ||
| (`.claude/board/ISSUES.md`), referenced by | ||
| `mask-membrane-valhalla-integration-v1.md` W1.1. It was drafted in-session | ||
| on 2026-08-28 under the `/5plus3` protocol and is committed here so the | ||
| reference is durable and the question sets are auditable — a spec that | ||
| exists only in a session transcript is not a spec | ||
| (the falsifiability rule's "a doc-comment claim is not a behaviour", | ||
| applied to plans). | ||
|
|
||
| ## Scope finding (measured before this spec was written) | ||
|
|
||
| The epoch-recheck machinery already exists on BOTH sides, more than the | ||
| issue implied: `registry.rs` stamps a *global* monotonic `epoch: u64` | ||
| (`next_epoch()`) on every resource at creation — separate from the | ||
| per-slot `generation` the handle registry checks — so two different | ||
| resources, even in the same slot, are always distinguishable | ||
| (`epochs_are_never_reused`, `registry.rs`). `Engine.java` already has | ||
| `Engine.epoch(long handle)`, which calls `lgj_resource_info` fresh and | ||
| returns the live epoch. The gap is narrower than "wire epoch checking | ||
| from scratch": **no call site in `RowStore.java`/`Mask.java` ever calls | ||
| `Engine.epoch(handle)` again and compares it against the `epoch` cached | ||
| in `LaneWindow` before trusting a cached `lanes[]`/`words` segment.** | ||
|
|
||
| ## 1. FROZEN DECISIONS | ||
|
|
||
| - D1. `ISS-LGJ-EPOCH-UNCHECKED` — the problem statement; not re-litigated. | ||
| - D2. `CLAUDE.md`'s corrected "Pointer value is not provenance" bullet | ||
| (PR #46) — scopes the generation-registry guarantee to handle-mediated | ||
| ops and explicitly excludes the cached-descriptor path; this spec's job | ||
| is to close that exclusion or formally ratify it. | ||
| - D3. The zero-copy law: temporary scratch allowed, a second canonical | ||
| copy is not — a fix must not introduce a copy to solve staleness. | ||
| - D4. `LifetimeTest`/`RowStoreLifetimeTest`'s existing falsifiers are | ||
| frozen baselines — no regression. | ||
|
|
||
| ## 2. INPUT INVENTORY | ||
|
|
||
| - `native/lgj-abi/src/registry.rs`: `epoch: u64` field (~:139-141), | ||
| `next_epoch()` (:233), stamped at each creation site | ||
| (:352/:365/:417/:456), tests `lane_epoch_matches_resource_epoch` + | ||
| `epochs_are_never_reused` (both green). | ||
| - `native/lgj-abi/src/exports.rs`: `epoch: entry.epoch` copied into | ||
| `LgjLaneDesc`/`LgjMaskDesc` on describe (:304, :393). | ||
| - `java/.../internal/ffm/Engine.java`: `rowCount`/`epoch(long handle)` | ||
| (:77-90, the live re-fetch primitive), `lane(...)` construction caching | ||
| `epoch` into `LaneWindow` once (:362-390), `LaneWindow` record (:405, | ||
| `epoch` field with no consumer). | ||
| - `java/.../internal/ffm/Layouts.java`: `LANE_EPOCH` (:209), | ||
| `INFO_EPOCH` (:228) — wired for reads, neither consumed. | ||
| - `java/.../RowStore.java`: `lane(int laneId)` lazy resolve + cache; the | ||
| per-row accessors reading through it; `checkedRow(long)`. | ||
| - `java/.../Mask.java`: the cached `words` lane, same shape. | ||
| - `java/.../RowStoreLifetimeTest.java`, `LifetimeTest.java` — falsifiers | ||
| to extend, not replace. | ||
|
|
||
| ## 3. THE PROPOSED RESOLUTION | ||
|
|
||
| Two candidate resolutions; the spec commits to attempting (a) first, (b) | ||
| only if (a) is measured to be wrong: | ||
|
|
||
| **(a) Wire the check.** On each access through a cached | ||
| `LaneWindow`/`words` segment, compare the cached `epoch` against a | ||
| freshly-fetched live epoch (`Engine.epoch(handle)` exists; a lighter | ||
| epoch-only export is the fallback IF measurement shows the full 32-byte | ||
| `lgj_resource_info` read is too costly per access). Mismatch → the same | ||
| `ClosedResourceException`-shaped failure the `closed` boolean already | ||
| produces — a strictly-additive second guard, not a new exception type. | ||
| **If the epoch-only export is minted, it is a full ABI citizen** (a | ||
| CodeRabbit review addition, PR #47): its own ABI minor bump, manifest + | ||
| `abi.md` entry, `requireMinor(N)` gating at the Java call site, and an | ||
| old-library rejection leg in `OldAbiCompatTest` — otherwise the fallback | ||
| recreates the missing-symbol failure class W1 exists to remove. | ||
|
|
||
| **Atomicity constraint (a second CodeRabbit review addition, PR #47 — | ||
| the check-then-read TOCTOU):** the epoch fetch and the cached | ||
| `LaneWindow`/`words` read are TWO steps; a `close(handle)` between them | ||
| (or same-slot reuse) defeats the check. Two facts frame the resolution | ||
| honestly: (1) the CURRENT `closed`-boolean guard carries the IDENTICAL | ||
| race — the epoch re-check neither introduces nor widens it; (2) within | ||
| one thread the window is unreachable (user code cannot call `close` | ||
| inside an accessor), so the residual window is exclusively | ||
| cross-thread lifecycle-vs-access. The council must therefore decide the | ||
| CONCURRENCY CONTRACT the check ships under, as an explicit output, one | ||
| of: (i) serialize epoch-fetch + cached read against close (a | ||
| read/close lock or a native lifetime lease covering both steps) — then | ||
| an INTERLEAVING falsifier for both cached paths is mandatory; or (ii) | ||
| scope the guarantee in writing to stale-cache detection BETWEEN | ||
| top-level calls, with the facade's lifecycle-vs-access thread contract | ||
| stated in `RowStore`/`Mask` javadoc and the doctrine wording matching | ||
| that scope (never "unconditional"). Silently shipping (ii)'s semantics | ||
| under (i)'s wording is the overclaim class the PR #46 council already | ||
| corrected once — not again. | ||
|
|
||
| **(b) Prove permanent unreachability, formally, and downgrade the | ||
| doctrine wording** — only if (a)'s cost is measured unacceptable AND a | ||
| falsifier-backed proof (a test that tries to construct the bad sequence | ||
| and fails, not prose) shows the facade's lifecycle discipline makes | ||
| slot-reuse-while-cached structurally impossible. **Outcome (b) is a | ||
| DOWNGRADE, never "resolved"** (CodeRabbit, PR #47): the doctrine's | ||
| scope-note stays live, `ISS-LGJ-EPOCH-UNCHECKED` closes as | ||
| DOWNGRADED-DOCUMENTED (a distinct status), and the unconditional-wording | ||
| restoration in `CLAUDE.md` never happens under (b). | ||
|
|
||
| ## 4. NON-GOALS | ||
|
|
||
| - Not touching the native `epoch`/`generation` machinery (already | ||
| correct and tested — the Scope finding above). | ||
| - Not redesigning `LaneWindow`/`RowStore` caching wholesale — the fix is | ||
| a recheck at the access boundary. | ||
| - Not resolving the doctrine-text PRs — separate, already handled. | ||
|
|
||
| ## 5. PRE-REGISTERED GATES | ||
|
|
||
| - A disable-run: with the recheck removed, a test constructing | ||
| same-slot-resource-reuse-while-cached must go RED (the check is | ||
| load-bearing, not decorative) — this test is itself a deliverable. | ||
| - With the check active: same test GREEN; every existing | ||
| `LifetimeTest`/`RowStoreLifetimeTest` falsifier stays green. | ||
| - **The overhead measurement targets the cached-descriptor accessors | ||
| THEMSELVES** (`classidAt`/`payloadLow64At`/`payloadHi32At` through | ||
| `lane()`, `Mask`'s cached-`words` reads) — a Codex review correction | ||
| (PR #47): the banked hop/columnar benches run entirely through native | ||
| operations, never touch this path, and are structurally blind to | ||
| per-access overhead. Protocol: the 65,536-row fixture, ns/accessor-call | ||
| over ≥1M calls, median of 5 runs, before/after, banked. | ||
| - Threshold (the reproducible units CodeRabbit asked for): if the | ||
| re-check cannot be brought under 2× the accessor's measured baseline | ||
| cost — including via the epoch-only export — resolution (b)'s track | ||
| opens; below 2×, (a) ships without further debate. | ||
| - **The measurement runs under the CHOSEN atomicity arm (§3), not the | ||
| bare epoch compare.** Arm (i) puts a lock/lease on every access — | ||
| a categorically different cost than a compare — so measuring the | ||
| bare compare and then shipping (i) would invalidate this gate: the | ||
| gate would no longer observe the cost it names, the exact | ||
| gate-observability failure the integration plan's standing question | ||
| exists to catch. Order is therefore: atomicity decision FIRST, then | ||
| the measurement, of that arm. | ||
| - `cargo test -p lgj-abi` + full Java suite green; clippy `-D warnings`. | ||
| - Board: `ISS-LGJ-EPOCH-UNCHECKED` flips OPEN → RESOLVED under (a), or | ||
| OPEN → DOWNGRADED-DOCUMENTED under (b), same commit as the code. | ||
|
|
||
| ## 6. PER-SAVANT QUESTION SETS (Phase 1, when cast) | ||
|
|
||
| | # | lens | card | questions | | ||
| |---|---|---|---| | ||
| | 1 | handle safety | `handle-lifecycle-auditor` | Is `Engine.epoch(handle)` itself generation-checked (does its `lgj_resource_info` call go through `resolve`)? Which arm of §3's atomicity constraint — serialize/lease (i) or scoped-contract (ii) — is right, and why? (This is a mandatory decision output, not just a question — see §3; the TOCTOU is pre-existing, shared with the `closed` boolean, and cross-thread-only.) | | ||
| | 2 | ABI membrane | `abi-membrane-warden` | Does an epoch-only export need a minor bump (yes per the resolution text — verify nothing cheaper suffices)? Does it fit the bulk/lifecycle taxonomy (`abi.md` §6)? | | ||
| | 3 | zero-copy law | `zero-copy-warden` | Is the recheck O(1) per access with zero row-proportional allocation? Does caching the comparison RESULT risk becoming a second staleness authority? | | ||
| | 4 | Java surface | `java-surface-warden` | Does the fix stay entirely inside `internal.ffm`/`RowStore`/`Mask` — no new public API, no freshness method leaking implementation physics? | | ||
| | 5 | measurement | `measurement-skeptic` | Is the disable-run's red genuinely caused by the removed check? Is the per-accessor measurement isolated and comparable (the protocol in §5), not an isolated-stage percentage? | | ||
|
|
||
| Output contract: ≤10 findings each, verdict vocab | ||
| `CONFIRMS / VIOLATES / GAP / PRIOR-ART-AT / RISK`, file:line evidence, | ||
| ≤2 sentences. Phase 2 consolidates before any reviewer exists; Phase 3 | ||
| reviewers see draft v2 only. | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.