fix: enforce independent semaphore release guards

[flyingrobots]

A semaphore release could discard an earlier ownership guard because --record and --acquisition shared one parser variable. A stale guard followed by a matching guard could release the slot; a record OID passed as an acquisition guard could also release it.

Keep both guards separately through every retry and compare each against its own field. Every supplied guard must match, regardless of option order. A mismatch returns the existing nothing/superseded event without changing the authority or freeing capacity. Matching and unguarded release behavior remains supported.

Closes #122. Discovered during the acquisition contract audit for #74. This fixes guard semantics; it does not introduce a new identifier grammar or finish #74.

Validation: Docker regression failed in four cases against main 7ba2c09, then all 31 guard checks passed with the fix. Lint passed. A shell-suite regression also checks preservation of both live holders. Full integration, hosted CI, and exact-head independent review are pending.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 17 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 82612d43-3801-461a-9d87-b54e242f674f

📥 Commits

Reviewing files that changed from the base of the PR and between 7ba2c09 and e7a31a0.

📒 Files selected for processing (7)

• CHANGELOG.md
• README.md
• bin/git-locks
• docs/usage.md
• lib/170-semaphores.sh
• test/release-guards.py
• test/test.sh

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[flyingrobots]

Parent verification and corrections

Exact reviewed head: e7a31a021d90c753d37927d335d2d6be56dbdab3.

The full independent review follows unchanged except link normalization. I accept its APPROVE verdict after checking the implementation and raw evidence, with these corrections:

• These are cooperating-caller release-guard correctness defects, not authentication or privilege-boundary vulnerabilities. Unguarded release is already supported.
• The full shell suite passed 1,102 checks. The 240 checks belong to test/literal-paths.sh, not test/test.sh. The focused release-guard suite passed 31 checks.
• Publication compares the single immutable refs/locks/state root. Semaphore generation metadata is not a separate publication CAS authority.
• One physical line per paragraph is not an established binding requirement here; the review's claimed conformance is not supported.
• Four RED failures cover three distinct scenarios: reversing the single record-as-acquisition option repeats the same case. This redundancy does not invalidate the regression.
• Synthetic schedules and passing CI do not prove all schedules or physical power-loss durability.

The source/generated executable agree. Required CI run 37293986010 passed on this exact head. No actionable defect remains in this review. CodeRabbit's success context represents a rate-limit response, not a substantive review. Local evidence paths below identify retained receipts; they are not public hosted artifacts.

---

Independent Adversarial Code Review: PR 123 (git-stunts/locks)

• Branch: fix/semaphore-release-guards
• Head Commit: e7a31a021d90c753d37927d335d2d6be56dbdab3
• Base Commit: 7ba2c09b9a09e86a8d811f391d6632b995df1450
• Review Mode: Authorized binding independent read-only review
• Associated Issue: Issue #122 (discovered during GL-006 / #74 acquisition-contract audit)

---

1. Executive Summary & Architectural Evaluation

On the target base 7ba2c09b9a09e86a8d811f391d6632b995df1450, the semaphore CLI parser in cmd_sem() assigned both --record and --acquisition flags into a single variable (record="$2"). Consequently, whichever option appeared last unconditionally clobbered the earlier guard. In addition, the release verification routine sem_release_attempt() tested that single parameter against either the record object ID or the acquisition string:

# Base implementation:
if [[ -n "${want}" && "${want}" != "${own_oid}" && "${want}" != "${own_acq}" ]]; then

This created three critical vulnerabilities:

1. Providing a stale --record followed by a matching --acquisition released the slot.
2. Providing a stale --acquisition followed by a matching --record released the slot.
3. Passing a current record OID as the --acquisition guard bypassed verification and released the slot because the OID matched own_oid.

PR 123 resolves this vulnerability by:

1. Maintaining record and acquisition in distinct parser variables in cmd_sem and bin/git-locks.
2. Passing both guards explicitly through sem_release_once and into sem_release_attempt.
3. Enforcing independent conjunction:

   if [[ (-n "${want_record}" && "${want_record}" != "${own_oid}") || (-n "${want_acquisition}" && "${want_acquisition}" != "${own_acq}") ]]; then

4. Emitting {"event":"nothing","semaphore":...,"job":...,"reason":"superseded"} with exit code 0 on any guard mismatch, leaving the state tree refs/locks/state and slot reservation completely untouched.

Automated CodeRabbit review was rate-limited during PR submission (posting a rate-limit notice without substantive code analysis); this independent adversarial review evaluates the exact HEAD commit without relying on external bot contexts.

---

2. Code Path Analysis & Production Parity

Path 1: Semaphore Release Execution

• Production Source: lib/170-semaphores.sh:231-352 / bin/git-locks:3079-3200
• Invocation: git locks sem release <name> --job <id> [--record <oid>] [--acquisition <id>]
• Tracing:
  • cmd_sem() initializes record='' and acquisition=''.
  • Flags are parsed independently:
    • --record: validated via valid_oid (^[0-9a-f]{40}([0-9a-f]{24})?$); invalid input triggers fail '--record is an object id' 2.
    • --acquisition: validated via valid_holder (nonempty, single line, no \r/\n, valid UTF-8); invalid input triggers fail '--acquisition must be a nonempty UTF-8 line' 2.
  • Arguments are passed to sem_release_once "${name}" "${job}" "${record}" "${acquisition}".
  • In sem_release_once(), a bounded loop (attempt < RETRIES, 200 iterations with 10 ms sleep) executes snapshot and invokes sem_release_attempt "$1" "$2" "${3:-}" "${4:-}".
  • In sem_release_attempt():
    • sem_read "${name}" populates SLOT_JOBS, SLOT_OIDS, SLOT_LIVE.
    • If own_oid is empty (absence), it outputs {"event":"nothing","semaphore":...,"job":...} and returns 0.
    • Extracts own_acq="$(field "${own_oid}" acquisition)".
    • If (-n "${want_record}" && "${want_record}" != "${own_oid}") || (-n "${want_acquisition}" && "${want_acquisition}" != "${own_acq}"), outputs {"event":"nothing","semaphore":...,"job":...,"reason":"superseded"} and returns 0.
    • If all supplied guards match (or are omitted), plans removal via plan_set "${slot_ref}" "${own_oid}" '' and commits via sem_transact "${name}".
    • Commit failures (CAS contention on generation token) return 2, triggering a fresh snapshot and retry in sem_release_once.

Path 2: Path Lock Release Parity

• Production Source: lib/110-release.sh:3-94 / bin/git-locks:2220-2311
• Comparison:
  • cmd_release parses --record and --acquisition into separate per-job array entries (records and acqs).
  • Validation: uses identical valid_oid and valid_holder validators and error messages.
  • Guard enforcement (lib/110-release.sh:52-61):

    if [[ -n "${records[${i}]}" && "${records[${i}]}" != "${oid}" ]]; then
      superseded+=("${j}")
      continue
    fi
    if [[ -n "${acqs[${i}]}" ]]; then
      have_acq="$(field "${oid}" acquisition)"
      if [[ "${have_acq}" != "${acqs[${i}]}" ]]; then
        superseded+=("${j}")
        continue
      fi
    fi

  • Both paths enforce independent conjunction: every provided guard must match its respective field. Mismatch emits {"event":"nothing", ... "reason":"superseded"} and exits 0. Absence emits {"event":"nothing", ...} without reason.

Path 3: Command Wrapper Cleanup Parity

• Production Source: lib/160-with.sh:366-480 / bin/git-locks:2845-2959
• Comparison:
  • with_reason() fetches field_v acquired "${W_OWN_OID}" acquisition.
  • If acquired != W_ACQUISITION, sets W_REASON=superseded.
  • with_cleanup() releases only if W_REASON is empty or expired. If superseded, it does not touch the slot or lock ref, logs event: lost, and exits 125.
  • All three release pathways maintain identical safety: stale acquisition guards never release active resources held by another generation.

---

3. Merge & Parent Integration Audit

• PR Branch History:
  Commit e7a31a021d90c753d37927d335d2d6be56dbdab3 is a single, clean commit on top of base 7ba2c09b9a09e86a8d811f391d6632b995df1450. There are no merge commits inside the PR branch (git rev-list --parents 7ba2c09..e7a31a0 returns exactly one edge).
• Audit Against Recent Base Integrations:
  • PR fix: derive command help from canonical synopses #119 (93f2e0b) - Canonical Synopses: PR 119 derived command help from lib/000-prelude.sh and enforced synopses in test/help-synopses.py. PR 123 preserves the exact synopsis release <name> --job <id> [--record <oid>] [--acquisition <id>] without schema or CLI breakage.
  • PR fix: reject missing default home before store initialization #118 (fc1ec26) - Missing Default Home: PR 123 does not alter store discovery, leaving HOME validation intact.
  • PR fix: avoid stray-path findings for undecodable records #116 (3ae42bd) - Undecodable Records: PR 123 does not change the snapshot, CAS transaction schema, or record format.
  • PR fix: reject empty acquisition guards before release #114 (4596826) - Empty Acquisition Rejection: PR 114 introduced valid_holder checks for --acquisition. PR 123 preserves this invariant while fixing the underlying clobbering assignment.
  • Caller & Arity Audit:
    Every call to sem_release_once and sem_release_attempt across all files (lib/170-semaphores.sh, bin/git-locks) was inspected. No orphaned 3-argument callers remain.

---

4. Empirical Evidence & Numeric Verification

Pinned Evidence Coordinates

All 7 modified source files were verified against semaphore-guards-evidence.json using SHA-256:

• lib/170-semaphores.sh: 8ea18b6c22b2de54e39939163b6fea8c2cf31c5279467733e1b224d177ebbe2d (Verified)
• bin/git-locks: 4ce15f402e72b27f49f71bc0b4b025ea4ece7991fd890974287e7fc2660c7b44 (Verified)
• test/test.sh: 1884d915b0c01de0b5b88c802a63bc26f189c1e7b0d3e02afe446eb659d9fbaf (Verified)
• test/release-guards.py: a3159b9e03cce9b6bdee3eb79e7028039ad942cf54ce1ebee76227ff78251b32 (Verified)
• docs/usage.md: 8d1b824e7577bf77f90b1e4e5f94f2504ab90fd054e25a8a774be9a4f23f0a8c (Verified)
• README.md: 613b25405f49f632b3c20971c06b5b99d544793cd1fc565feba401f6b38d0a29 (Verified)
• CHANGELOG.md: 5ca473195ff4d8e9ac9ef62c86cec0c45c54281851d74d9cd13d396fa03705b8 (Verified)

Test Suite Execution Receipts

1. RED Regression (semaphore-guards-red):
   • Evidence file: semaphore-guards-red-result.json (exit code 1) & semaphore-guards-red-latest.log.
   • Counts: 27 passed, 4 failed against unpatched base executable.
   • The 4 failures confirmed the exact defects:
     • stale-record False: stale record followed by matching acq released slot.
     • stale-acquisition True: stale acq followed by matching record released slot.
     • record-as-acquisition False & True: current record OID supplied as acquisition guard released slot.
2. GREEN Regression (semaphore-guards-green):
   • Evidence file: semaphore-guards-green-result.json (exit code 0) & semaphore-guards-green-latest.log.
   • Counts: 31 passed, 0 failed. Shellcheck and shfmt clean.
3. Full Integration Suite (semaphore-guards-full):
   • Evidence file: semaphore-guards-full-result.json (exit code 0) & semaphore-guards-full-latest.log.
   • Tests completed: 240 test.sh tests passed; 20 unicode-locale.sh passed; 6 unicode-locale-calibration.sh passed; directory-token-churn.sh passed; 18 study.py observation cases passed (0 violations); root-cas-calibration.py passed.
4. Guarded Resource Accounting (semaphore-guards-full-resources.json):
   • Peak tmpfs usage:
     • /work: 3,203,072 bytes (3.05 MiB; limit 512 MiB)
     • /tmp: 21,594,112 bytes (20.59 MiB; limit 512 MiB)
     • /evidence: 3,448,832 bytes (3.29 MiB; limit 16 MiB)
     • /home/node: 0 bytes (limit 32 MiB)
     • /dev/shm: 0 bytes (limit 16 MiB)
   • Minimum VM free space: 680,424,525,824 bytes (633.7 GiB; required $\ge$ 50 GiB)
   • Host free space at launch: 717,040,263,168 bytes (667.8 GiB)
   • Peak non-object generated data: 6,070,272 bytes (5.79 MiB; budget 4 GiB)
   • Standard output log size: 93,572 bytes (91.38 KiB; budget 128 MiB)
   • Guard error: null

---

5. Findings

Demonstrated Defects

None (P0–P3: 0 findings). The implementation correctly enforces independent guard parsing, retry preservation, field comparison, error handling, and authority preservation.

Observations & Coverage Boundaries

• OBS-1 (P5 - Test Matrix Redundancy): In test/release-guards.py:94-97, when mode == 'record-as-acquisition', options contains a single tuple [('--acquisition', receipt['record'])]. Reversing a 1-element list is a no-op; thus reverse=False and reverse=True execute the identical command line. This accounts for the duplicate failure lines in the RED run log and does not constitute a defect.
• OBS-2 (Informational - Coverage Boundary): Review was conducted strictly through read-only inspection of git source, diffs, and existing Docker verification receipts. As instructed, host-level test execution was not performed, preserving host workstation isolation.

---

6. Mandatory Verification Checklist

Item	Status	Verification & Coordinates
CLI Parser Separation	Verified	lib/170-semaphores.sh:233, lib/170-semaphores.sh:283-294: distinct record and acquisition variables.
Retry Loop Argument Propagation	Verified	lib/170-semaphores.sh:187-195: sem_release_once passes "${3:-}" "${4:-}" to sem_release_attempt across all 200 retries.
Exact Field Matching	Verified	lib/170-semaphores.sh:218: independent checks want_record != own_oid and want_acquisition != own_acq.
Authority & Capacity Preservation	Verified	test/release-guards.py:105-108: state(env) == before on mismatch; competing acquire refused with capacity.
Path Release Parity	Verified	lib/110-release.sh:52-61: identical conjunction rules, error exits, and event diagnostics.
Wrapper Cleanup Parity	Verified	lib/160-with.sh:374-380: acquisition identity check prevents release of superseded slots.
Empty/Multiline/UTF-8 Validation	Verified	lib/030-time-refs-records.sh:48: valid_holder rejects empty, newline, and invalid UTF-8 with exit code 2.
Absence Handling	Verified	lib/170-semaphores.sh:212-215: empty own_oid returns {"event":"nothing"} with exit 0.
Committed Executable Synchronized	Verified	bin/git-locks:3033-3200 contains identical logic to lib/170-semaphores.sh.
Merge & Invariant Audit	Verified	Direct fast-forward descendant of 7ba2c09; no intermediate merges; preserves PR #114, #116, #118, #119 invariants.
Evidence & Resource Bounds	Verified	semaphore-guards-full-resources.json: all runtime metrics verified within bounds ($\le$20 GiB build, $\le$4 GiB runtime, $\le$128 MiB logs, $\ge$50 GiB host/VM free).
Documentation Standards	Verified	docs/usage.md:68-70, CHANGELOG.md:7: one physical line per paragraph; claims adhere to repo style.

---

7. Execution Status Summary

• Static Inspection: Executed across all 7 modified files and surrounding subsystems.
• Evidence Verification: Completed against all .test-results/semaphore-guards-* artifacts and checksums.
• Container Tests: Inspected completed container runs (semaphore-guards-red, semaphore-guards-green, semaphore-guards-full).
• Host Tests: Prohibited and skipped in compliance with workspace safety instructions.

---

APPROVE