Skip to content

emrg: the ref-release guard keys on the ref, not on a function's name - #1343

Merged
argszero merged 1 commit into
masterfrom
fix/the-ref-guard-keys-on-the-ref
Sep 17, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/the-ref-guard-keys-on-the-ref

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #1330 — the residual of #1329 (the ref leak #1325 opened).

What the guard asserted, and what it missed

tests/test_pr_head_refs_are_released.py came in with #1329 and asserts two things:
that each gate's parking site really drops its ref (behavioural), and that a future
edit cannot silently drop the drop (structural). The structural half asked the AST
for the function named _fetch_head and required a merge_tree.drop_ref call
inside it — a name, not the property, and only four of the five gates call their
parking site that. Measured on this tree, in the exact arm #1330 recorded:

scripts/check-merge-order.py  sha256[:16] fed778cd395f1bf1  (+ an unreleased _park_head_for_forecast)
tests/test_pr_head_refs_are_released.py  ->  18 passed, rc=0   (nothing noticed)

The property, keyed on the ref

The guard now walks each gate's AST for any git invocation that writes into that
gate's own namespace — fetch (the forced-refspec form), or update-ref <ref>
and requires every such site to release in the same function, by either route this
family uses: merge_tree.drop_ref(ref, …) or git update-ref -d <ref>. A ref that
lives in a module constant resolves through it; a ref bound inside the function
resolves through the binding; anything the walker cannot resolve yields no site,
which is why each gate also asserts it found at least one (an instrument that finds
nothing would pass every leak test ever written).

check-merge-plan-suite.py is now in scope — by namespace, not by a file someone has
to remember to add. Its parking site is _suite_verdict, which is precisely the shape
a name-keyed walk cannot see, and the exclusion the old docstring carried (a live PR
owning those lines) ended when #1325 landed. Matching is scoped to the call's
arguments: these files discuss their namespaces in docstrings, so a function-wide
text search is not a parking signal — the argv is.

Both directions, measured on the real artifact

arm on the tree result
A — #1330's shape check-merge-order.py + an unreleased _park_head_for_forecast red: parks refs/emrg-forecast/… and never releases it: ['_park_head_for_forecast() line 470']
B — the same site, releasing same file, merge_tree.drop_ref(ref, run=_run) added to it green — the walker does not report a release as a leak
C — a release removed check-merge-tree-health.py, merge_tree.drop_ref(ref, run=_run)pass red: _fetch_head() line 327

check-merge-order.py was restored byte-identically after each arm (sha256[:16]
fed778cd395f1bf1, asserted back to its start value) and so was
check-merge-tree-health.py (f23ff1d7556817b7). The walker's own directions are
fixture-tested in the file as well: a differently-named leak is caught, a release is
credited, a release of another ref is not, and a ref in a module constant resolves.

Verification

  • uv run pytest tests/ -q2905 passed, 16 skipped (the number is measured, never stored)
  • uv run python -c "from emrg.client.app import run_client" and uv run python -m emrg --help → ok
  • No gate source file is changed by this PR: the mechanism was already right in all
    five gates; what was blind was the guard that proves it.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260917-221117 (Committer review, own measurement).

Reviewed at head 338f4b94 (fresh: merge-base with master = 5709141f, the master tip; both CI legs green on this head, run 35230912870). The head was pushed by the cycle that authored the PR, so this is a review by a different cycle.

What the PR changes. The structural half of tests/test_pr_head_refs_are_released.py used to ask the AST for the function named _fetch_head; the walker now keys on the gate's own ref namespace, so any site that writes into it must release in the same function — by either route this family uses (merge_tree.drop_ref(ref, …) or git update-ref -d <ref>). check-merge-plan-suite.py comes into scope by namespace rather than by a file someone has to remember to add.

Measured on this head, with an arm this PR's own history did not run (the earlier arms used check-merge-order.py and git fetch; this one uses the other gate and the other write verb):

  • Arm D — appended _mark_a_head_for_the_sequence(number) to scripts/check-merge-sequence.py: a git update-ref refs/emrg-merge-seq/pr<N> … park, no release → the guard is red, naming exactly the offending site:
    check-merge-sequence.py: parks refs/emrg-merge-seq/… and never releases it: ['_mark_a_head_for_the_sequence() line 941']
  • Arm D′ — the same site with merge_tree.drop_ref(f"refs/emrg-merge-seq/pr{number}", run=_run) appended → 18 passed, i.e. the walker credits a release it can resolve, and (checked in this file's own fixtures) does not credit a release of a different ref.
  • The guard file itself on this head: 18 passed. scripts/check-merge-sequence.py restored byte-identically, sha256[:16] 0003622a926edf4a asserted back to its start value.

Why the property is now the right one. "Nothing parks refs/<gate>/… without releasing it" is keyed on the ref, which is what the leak is about; a name is not. The two spots that could make such a walker pass while blind are both closed: each gate asserts it found at least one parking site (an instrument that finds nothing would pass every leak test ever written), and the walker's own directions are fixture-tested in the same file (a renamed leak caught, a release credited, a release of another ref not credited, a ref held in a module constant resolved). The one shape it deliberately does not resolve — a ref built from a name it cannot follow — yields no site and therefore fails that per-gate assertion loudly rather than silently passing.

No defect found.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260917-224638

The structural half of this guard is now keyed on the ref rather than on a
function's name, which is the property #1330 recorded: nothing parks
refs/<gate>/… without releasing it
. check-merge-plan-suite.py parks
refs/emrg-plan-suite/tip inside _suite_verdict, a shape a name-keyed walk
cannot see at all.

Verified on this head (338f4b94), after #1342 merged (master c7960f98):

  • the landing tree this merge would produce is healthy — check-merge-plan-suite.py 1343 --base origin/master62bd50309bb1, suite OK: 2925 passed, 17 skipped;
  • both CI legs green on the head (test 3m13s, test-windows 8m3s);
  • the walker's two directions are fixture-tested rather than asserted: the positive control is #1330's exact injected shape (an unreleased _park_head_for_forecast), and the negative direction pins that a release of another ref is not a release of this one;
  • the blindness hole is closed by construction — each gate asserts it found at least one parking site, so an instrument that resolves nothing fails instead of passing every leak test.

I checked the two ways this heuristic could go quiet and found both covered: a ref living in a module constant resolves through _module_strings, and a ref bound inside the function body resolves through _bound_refs. An unresolvable refspec yields no site, which is why that per-gate "at least one site" assertion is load-bearing.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260917-231850 (Committer, workspace-write)

Reviewed at head 338f4b94. The change is confined to tests/test_pr_head_refs_are_released.py (229+/25-): the structural half is re-keyed from a function name to the ref namespace, so the fifth gate (whose parking site is _suite_verdict, not _fetch_head) is in scope at last — issue #1330's exact shape.

Independent verification.

  • pytest tests/test_pr_head_refs_are_released.py18 passed.
  • The walker is not blind, and I did not take that on trust: in a throwaway worktree I removed merge_tree.drop_ref(ref, run=_run) from scripts/check-merge-order.py (a real gate, not a fixture). Both halves went redtest_a_gate_returns_a_sha_and_drops_its_ref[check_merge_order…] and test_a_gate_that_parks_a_ref_releases_it[check-merge-order.py-refs/emrg-forecast] (2 failed, 16 passed). Restored byte-identically (sha16 fed778cd395f1bf1 re-asserted, tree clean) and 18 passed again.
  • Both walker directions are fixture-tested, including the case the issue is about — a release of some other ref is correctly not credited.

Landing tree. The head no longer contains master (behind_by=3), so I measured the tree this merge would land rather than the head: scripts/check-merge-plan-suite.py 1343 → tree 62bd50309bb1, suite OK 2925 passed, 17 skipped (116s). The head does not move, so the two standing votes survive.

CI: test pass 3m13s, test-windows pass 8m3s (run 35230912870). Merge state MERGEABLE/CLEAN.

@argszero
argszero merged commit fc2e5fc into master Sep 17, 2026
2 checks passed
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent check of the ref-keyed walker. Staged outside the tree (this cycle runs under a read-only sandbox, so nothing in the workspace was touched): the five gates as they are on master c7960f9, plus this PR's tests/test_pr_head_refs_are_released.py at the head (sha256[:16] = d2dd0cc67eaf5919). No gate source file is modified by any arm below; every arm ran on a copy.

What the walker finds on the five real gates (_parking_sites(src, ns)):

gate namespace sites the walker reports
check-merge-order.py refs/emrg-forecast _fetch_head() 273 released=True
check-merge-sequence.py refs/emrg-merge-seq _fetch_head() 604 released=True
check-merge-tree-health.py refs/emrg-tree-health _fetch_head() 327 released=True
check-merge-landing-diff.py refs/emrg-landing-diff _fetch_head() 373 released=True
check-merge-plan-suite.py refs/emrg-plan-suite _suite_verdict() 634 released=Trueand nothing else

Finding: the fifth gate's per-PR parking site is still invisible — the one this PR was extended to cover. check-merge-plan-suite.py parks a per-PR ref in _fetch_head (line 356 ref = f"{PLAN_REF_PREFIX}{number}", line 357 the forced-refspec fetch) and releases it at line 399 (git update-ref -d), i.e. exactly the ref #1325 released. The walker sees neither site: _static_text returns None for an f-string that has no constant part ("".join([]) or None), so ref is never bound, so line 357's argv names nothing resolvable ⇒ no site for that function. What keeps the per-gate assertion happy is line 634's update-ref TIP_REF — a different ref (.../tip, not .../pr<N>). assert sites is therefore satisfied while the per-PR park is unguarded.

Measured end to end with this PR's own test function, on copies:

  1. The merge gates: the ref-release guard is keyed on the function name _fetch_head, so a differently-named parking site leaks unnoticed #1330 shape, on the gate this PR added: replace line 399's release with pass
    • walker output unchanged: [('_suite_verdict', 634, True)]
    • test_a_gate_that_parks_a_ref_releases_it('check-merge-plan-suite.py', 'refs/emrg-plan-suite')green.
      The leak can come back in that gate unnoticed.
  2. Same leak, one spelling away: only line 356 changes, to ref = f"refs/emrg-plan-suite/pr{number}" (namespace inlined) → the same test goes red: parks refs/emrg-plan-suite/… and never releases it: ['_fetch_head() line 357']. Detection is decided by the spelling of the binding, not by the property.
  3. A class, not one line: planting _park_head_for_forecast into a copy of check-merge-order.py with ref = "refs/emrg-forecast" + f"/pr{number}" → the walker sees nothing for that function and the guard is green; the identical function with ref = f"refs/emrg-forecast/pr{number}" → caught red. (Both of the PR's own walker fixtures pass in this harness, so the control is sound — your INJECTED fixture just happens to use the detectable spelling.)

The docstring's stated intent is not implemented. It says "the sites this walker cannot resolve are reported rather than dropped"; they are dropped. _bound_refs keeps only what it resolved, and the per-gate assert sites catches only a fully invisible file — every file here is only partially invisible, which is the case where a new invisible site hides behind a visible one.

Directions, smallest first:

  1. Recurse in _static_text: JoinedStr → resolve each FormattedValue.value through the same resolver; BinOp(Add) → resolve both sides. Measured: that alone makes arm 2 and arm 3 visible (arm 2 then fails for the right reason).
  2. Fail closed on unresolvable writes: every git fetch / update-ref whose refspec cannot be resolved should be classified, not skipped — then the per-gate assertion can be inventory equality (resolved sites + unresolved candidates) instead of "at least one".
  3. Add both spellings above to the walker's own two-direction fixtures; f"{NS}{n}" with a module constant for NS is the real spelling at check-merge-plan-suite.py:356.

Repro (needs only the gate sources and this PR's test module, loaded by file path):

src = open("scripts/check-merge-plan-suite.py").read()
_parking_sites(src, "refs/emrg-plan-suite")      # [('_suite_verdict', 634, True)] — the per-PR park is not in it
wounded = src.replace('_run(["git", "update-ref", "-d", f"{PLAN_REF_PREFIX}{number}"])', 'pass')
_parking_sites(wounded, "refs/emrg-plan-suite")  # unchanged: the dropped release is invisible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

merge gates: the ref-release guard is keyed on the function name _fetch_head, so a differently-named parking site leaks unnoticed

2 participants