Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: minor
---

### Added

- work-item-tracker: the github and local-markdown `claim` take an optional `--phase <kind>` (`triage`, `discovery` or `implementation`) and write it into the lease as `phase` (#11871). A lease without the field is an implementation claim. `renew-lease`, and the github `reclaim`, keep it, so a triage or discovery claim can be told apart from an implementation claim and timed per phase. Arbitration and liveness ignore the field. Any other value is refused with exit 2 before anything is written.
10 changes: 8 additions & 2 deletions plugins/work-items/tools/work-item-tracker/CONTRACT.md
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ user-global layer.
```text
work-item-tracker.sh create-item --title <t> [--body <b> | --body-file <path>] [--labels a,b] [--type <name>] [--parent <id>] [--blocked-by <id>[,<id>]] [--repo <o>/<r>]
work-item-tracker.sh get-item <id>
work-item-tracker.sh claim <id> [--ttl-hours <n>] [--ttl-minutes <n>] [--session-id <s>] [--holder <h>] [--lease-only]
work-item-tracker.sh claim <id> [--ttl-hours <n>] [--ttl-minutes <n>] [--session-id <s>] [--holder <h>] [--lease-only] [--phase <kind>]
work-item-tracker.sh renew-lease <id> --lease-comment-id <n>
work-item-tracker.sh release <id> --lease-comment-id <n> [--holder <h>]
work-item-tracker.sh reclaim <id>
Expand Down Expand Up @@ -549,7 +549,7 @@ Arbitration is the same in both modes (step 4), so a local claim made while a la
backs off, and a lane run that finds a live local lease backs off without starting its model.

```text
<!-- work-item-lease v1 {"schema_version":"1.0","holder":"<gh login>","acquired_at":"<ISO>","renewed_at":"<ISO>","ttl_hours":24,"session_id":"<opt>"} -->
<!-- work-item-lease v1 {"schema_version":"1.0","holder":"<gh login>","acquired_at":"<ISO>","renewed_at":"<ISO>","ttl_hours":24,"session_id":"<opt>","phase":"<opt>"} -->
```

- Created at claim; **edited in place** at renew (`renewed_at` bump); superseded at
Expand All @@ -569,6 +569,12 @@ backs off, and a lane run that finds a live local lease backs off without starti
TTL-based handoff. Recovery from an expired lease is a fresh claim/reclaim, not a renew.
- `session_id` is diagnostic metadata only, optional and collision-prone; a
missing/duplicate `session_id` still counts as a competing lease.
- `phase` names the work the lease is for: `triage`, `discovery` or `implementation`. The github
and local-markdown `claim` write it from `--phase <kind>` and refuse any other value (exit 2)
before any write. A lease with no `phase` field is an implementation claim. `renew-lease`, and
the github `reclaim`, keep the field, since they edit the lease JSON in place. It is metadata only:
Comment thread
kyle-sexton marked this conversation as resolved.
Resolved
arbitration and liveness ignore it, so a triage lease blocks an implementation claim like any
other live lease. The claim and renew result objects do not carry it; read it from the lease.
- The **lease handle** (`lease_comment_id`, emitted by `claim`/`renew-lease`) is
provider-specific: the GitHub adapter uses the lease comment's own id (external,
not stored in the JSON); the local-markdown adapter has no external ids, so it
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#!/usr/bin/env bash
# claim <id> [--ttl-hours <n>] [--session-id <s>] [--holder <h>] [--lease-only] —
# claim <id> [--ttl-hours <n>] [--session-id <s>] [--holder <h>] [--lease-only] [--phase <kind>] —
# CONTRACT.md "Lease protocol". Assignment is bare gh (@me = session identity); the
# lease comment goes through the bot. Race arbitration is by lease-comment identity,
# not session_id. --holder names the lease holder instead of `gh api user`, which an
Expand All @@ -8,13 +8,14 @@ set -uo pipefail
# shellcheck source=common.sh
source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/common.sh"

usage='usage: claim <id> [--ttl-hours <n>] [--ttl-minutes <n>] [--session-id <s>] [--holder <h>] [--lease-only]
usage='usage: claim <id> [--ttl-hours <n>] [--ttl-minutes <n>] [--session-id <s>] [--holder <h>] [--lease-only] [--phase <kind>]
--ttl-minutes (0-59) adds to --ttl-hours; for a sub-hour lease pass --ttl-hours 0
--holder names the lease holder instead of the authenticated login
--lease-only posts the lease comment and assigns no one'
--lease-only posts the lease comment and assigns no one
--phase (triage, discovery or implementation) records the work the lease is for; absent means implementation'
wit_help_if_requested "$usage" "$@"

ttl="${WIT_LEASE_TTL_HOURS:-}" ttl_minutes="${WIT_LEASE_TTL_MINUTES:-0}" session_id="" holder="" lease_only=false
ttl="${WIT_LEASE_TTL_HOURS:-}" ttl_minutes="${WIT_LEASE_TTL_MINUTES:-0}" session_id="" holder="" lease_only=false phase=""
id="${1:-}"
[[ -n "$id" ]] || wit_usage_error "$usage"
shift
Expand Down Expand Up @@ -44,10 +45,16 @@ while [[ $# -gt 0 ]]; do
lease_only=true
shift
;;
--phase)
[[ $# -ge 2 && -n "$2" ]] || wit_usage_error "--phase needs a value"
phase="$2"
shift 2
;;
*) wit_usage_error "unknown argument: $1" ;;
esac
done
wit_require_github_id "$id" || wit_usage_error "malformed or non-github id: $id"
[[ -z "$phase" ]] || wit_lease_phase_valid "$phase" || wit_usage_error "--phase must be triage, discovery or implementation"
[[ "$ttl" =~ ^[0-9]+$ ]] || wit_usage_error "--ttl-hours must be a non-negative integer (binding config.lease_ttl_hours supplies the default)"
if ! [[ "$ttl_minutes" =~ ^[0-9]{1,2}$ ]] || ((10#$ttl_minutes > 59)); then
wit_usage_error "--ttl-minutes must be 0-59 (binding config.lease_ttl_minutes defaults to 0)"
Expand Down Expand Up @@ -106,11 +113,12 @@ fi
# 3. Post our lease comment.
now="$(date -u +%Y-%m-%dT%H:%M:%SZ)"
lease="$(jq -cn --arg sv "$WIT_SCHEMA_VERSION" --arg holder "$login" --arg now "$now" \
--arg ttl "$ttl" --arg ttl_minutes "$ttl_minutes" --arg sid "$session_id" \
--arg ttl "$ttl" --arg ttl_minutes "$ttl_minutes" --arg sid "$session_id" --arg phase "$phase" \
'{schema_version: $sv, holder: $holder, acquired_at: $now, renewed_at: $now,
ttl_hours: ($ttl | tonumber)}
+ (if ($ttl_minutes | tonumber) > 0 then {ttl_minutes: ($ttl_minutes | tonumber)} else {} end)
+ (if $sid != "" then {session_id: $sid} else {} end)')"
+ (if $sid != "" then {session_id: $sid} else {} end)
+ (if $phase != "" then {phase: $phase} else {} end)')"
wit_run_gh write api "repos/$owner/$repo/issues/$number/comments" \
-f body="${WIT_LEASE_MARKER}${lease} -->" --jq '.id'
our_comment_id="$WIT_GH_OUT"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
# the caller's active one;
# (5) a lane's lease-only claim and release under a --holder override: a local
# claim backs off while the lane lease is live and holds once it is
# released, and a lane claim backs off on a live local lease.
# released, and a lane claim backs off on a live local lease;
# (6) claim --phase writes the lease's phase and renew-lease keeps it.
# shellcheck disable=SC2154 # FAILED/CASE_NUM initialized by the sourced lib
set -uo pipefail

Expand Down Expand Up @@ -521,4 +522,44 @@ ttl_case "a 169 h claim is refused" 2 0 --ttl-hours 169
ttl_case "a 168 h 1 m claim is refused" 2 0 --ttl-hours 168 --ttl-minutes 1
ttl_case "a 168 h claim is accepted" 0 1 --ttl-hours 168

# [phase] claim --phase writes the field into the lease comment and renew-lease keeps
# it; no --phase writes no field (an implementation claim); an unknown phase is
# refused before any write.
stored_lease() { jq -r --argjson id "$1" '.[] | select(.id == $id) | .body | ltrimstr("<!-- work-item-lease v1 ") | rtrimstr(" -->")' "$GH_STUB_DIR/lease-comments"; }
new_scenario
: >"$GH_STUB_DIR/stateful"
printf 'alice\n' >"$GH_STUB_DIR/login"
printf '[]\n' >"$GH_STUB_DIR/assignees"
printf '[]\n' >"$GH_STUB_DIR/lease-comments"
out="$(bash "$CLAIM" "$ID" --ttl-hours 24 --phase triage 2>/dev/null)"
assert_eq "claim --phase triage succeeds (0)" "0" "$?"
PHASE_CID="$(jq -r '.lease_comment_id' <<<"$out")"
assert_eq "claim --phase triage writes phase into the lease comment" "triage" "$(stored_lease "$PHASE_CID" | jq -r '.phase')"
sleep 1
bash "$RENEW" "$ID" --lease-comment-id "$PHASE_CID" >/dev/null 2>&1
assert_eq "renew-lease on the triage lease succeeds (0)" "0" "$?"
assert_eq "renew-lease keeps the phase" "triage" "$(stored_lease "$PHASE_CID" | jq -r '.phase')"
assert_eq "renew-lease bumped renewed_at past acquired_at" "true" \
"$(stored_lease "$PHASE_CID" | jq -r '.renewed_at > .acquired_at')"
cleanup_scenario

new_scenario
: >"$GH_STUB_DIR/stateful"
printf 'alice\n' >"$GH_STUB_DIR/login"
printf '[]\n' >"$GH_STUB_DIR/assignees"
printf '[]\n' >"$GH_STUB_DIR/lease-comments"
out="$(bash "$CLAIM" "$ID" --ttl-hours 24 2>/dev/null)"
assert_eq "a claim with no --phase writes no phase field" "false" \
"$(stored_lease "$(jq -r '.lease_comment_id' <<<"$out")" | jq -r 'has("phase")')"
cleanup_scenario

new_scenario
: >"$GH_STUB_DIR/stateful"
printf '[]\n' >"$GH_STUB_DIR/lease-comments"
rc=0
bash "$CLAIM" "$ID" --ttl-hours 24 --lease-only --holder "$LANE" --phase review >/dev/null 2>&1 || rc=$?
assert_eq "claim refuses an unknown phase (2)" "2" "$rc"
assert_not_contains "a refused phase posts no lease" "$(calls)" "POST_COMMENT"
cleanup_scenario

[[ $FAILED -eq 0 ]] || exit 1
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@ set -uo pipefail
# shellcheck source=common.sh
source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/common.sh"

usage='usage: claim <id> [--ttl-hours <n>] [--ttl-minutes <n>] [--session-id <s>]
--ttl-minutes (0-59) adds to --ttl-hours; for a sub-hour lease pass --ttl-hours 0'
usage='usage: claim <id> [--ttl-hours <n>] [--ttl-minutes <n>] [--session-id <s>] [--phase <kind>]
--ttl-minutes (0-59) adds to --ttl-hours; for a sub-hour lease pass --ttl-hours 0
--phase (triage, discovery or implementation) records the work the lease is for; absent means implementation'
wit_help_if_requested "$usage" "$@"

id="${1:-}"
[[ -n "$id" ]] || wit_usage_error "$usage"
shift
ttl="${WIT_LEASE_TTL_HOURS:-}" ttl_minutes="${WIT_LEASE_TTL_MINUTES:-0}" session_id=""
ttl="${WIT_LEASE_TTL_HOURS:-}" ttl_minutes="${WIT_LEASE_TTL_MINUTES:-0}" session_id="" phase=""
while [[ $# -gt 0 ]]; do
case "$1" in
--ttl-hours)
Expand All @@ -33,10 +34,16 @@ while [[ $# -gt 0 ]]; do
session_id="$2"
shift 2
;;
--phase)
[[ $# -ge 2 && -n "$2" ]] || wit_usage_error "--phase needs a value"
phase="$2"
shift 2
;;
*) wit_usage_error "unknown argument: $1" ;;
esac
done
wit_require_local_id "$id" || wit_usage_error "malformed or non-local-markdown id: $id"
[[ -z "$phase" ]] || wit_lease_phase_valid "$phase" || wit_usage_error "--phase must be triage, discovery or implementation"
[[ "$ttl" =~ ^[0-9]+$ ]] || wit_usage_error "--ttl-hours must be a non-negative integer (binding config.lease_ttl_hours supplies the default)"
if ! [[ "$ttl_minutes" =~ ^[0-9]{1,2}$ ]] || ((10#$ttl_minutes > 59)); then
wit_usage_error "--ttl-minutes must be 0-59 (binding config.lease_ttl_minutes defaults to 0)"
Expand Down Expand Up @@ -66,10 +73,12 @@ lease_id="$(wit_next_lease_id)"

lease="$(jq -cn --arg sv "$WIT_SCHEMA_VERSION" --arg holder "$holder" --arg now "$now" \
--arg ttl "$ttl" --arg ttl_minutes "$ttl_minutes" --arg sid "$session_id" --arg cid "$lease_id" \
--arg phase "$phase" \
'{schema_version: $sv, holder: $holder, acquired_at: $now, renewed_at: $now,
ttl_hours: ($ttl | tonumber), lease_comment_id: ($cid | tonumber)}
+ (if ($ttl_minutes | tonumber) > 0 then {ttl_minutes: ($ttl_minutes | tonumber)} else {} end)
+ (if $sid != "" then {session_id: $sid} else {} end)')"
+ (if $sid != "" then {session_id: $sid} else {} end)
+ (if $phase != "" then {phase: $phase} else {} end)')"
# The lease marker and the assignee are one claim and must land together: a live
# lease with an empty assignees would look available and let a later claim race the
# live lease. A failed assignee write attempts to roll the marker back (wit_claim_write
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,19 @@ ITEM3_FILE="$(jq -r '.url' <<<"$ITEM3" | sed 's#^file://##')"
BEFORE="$(cat "$ITEM3_FILE")"
WIT_STORAGE_DIR="$STORE" assert_usage_error "$S" "$(jq -r '.id' <<<"$ITEM3")" --ttl-hours 169
assert_eq "a refused over-cap claim leaves the item file unchanged" "$BEFORE" "$(cat "$ITEM3_FILE")"

# --phase writes the field into the stored lease; no --phase stores none (an
# implementation claim); an unknown phase is refused before the item file is written.
ITEM4="$(WIT_STORAGE_DIR="$STORE" bash "$ADAPTER/create-item.sh" --title "triage claim")"
ITEM4_FILE="$(jq -r '.url' <<<"$ITEM4" | sed 's#^file://##')"
WIT_STORAGE_DIR="$STORE" bash "$S" "$(jq -r '.id' <<<"$ITEM4")" --ttl-hours 24 --phase triage >/dev/null
assert_contains "claim --phase triage stores the phase in the lease" \
"$(grep -F 'work-item-lease' "$ITEM4_FILE")" '"phase":"triage"'
assert_not_contains "a claim with no --phase stores no phase field" "$STORED" '"phase"'
BEFORE="$(cat "$ITEM3_FILE")"
WIT_STORAGE_DIR="$STORE" assert_usage_error "$S" "$(jq -r '.id' <<<"$ITEM3")" --ttl-hours 24 --phase review
WIT_STORAGE_DIR="$STORE" assert_usage_error "$S" "$(jq -r '.id' <<<"$ITEM3")" --ttl-hours 24 --phase ""
assert_eq "a refused phase leaves the item file unchanged" "$BEFORE" "$(cat "$ITEM3_FILE")"
rm -rf "$STORE"

[[ $FAILED -eq 0 ]] || exit 1
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,20 @@ assert_eq "the failed renewal left the stored lease untouched" \
"$LEASE_BEFORE" "$(wit_active_lease_json "$LIVE_FILE")"
rm -rf "$SHIM"

# A claim's phase survives renewal: renew-lease edits the stored lease in place.
PHASE_JSON="$(bash "$TRACKER" create-item --title "triage renew keeps phase")"
PHASE_ID="$(jq -r '.id' <<<"$PHASE_JSON")"
PHASE_FILE="$(jq -r '.url' <<<"$PHASE_JSON" | sed 's#^file://##')"
PHASE_CID="$(bash "$TRACKER" claim "$PHASE_ID" --ttl-hours 24 --phase triage | jq -r '.lease_comment_id')"
assert_eq "claim --phase triage stores the phase" "triage" "$(jq -r '.phase' <<<"$(wit_active_lease_json "$PHASE_FILE")")"
sleep 1
PHASE_OUT="$(bash "$TRACKER" renew-lease "$PHASE_ID" --lease-comment-id "$PHASE_CID")"
assert_eq "renew-lease on the triage lease succeeds (0)" "0" "$?"
PHASE_LEASE="$(wit_active_lease_json "$PHASE_FILE")"
assert_eq "renew-lease keeps the phase" "triage" "$(jq -r '.phase' <<<"$PHASE_LEASE")"
assert_eq "the kept-phase lease carries the renewed_at it reported" \
"$(jq -r '.renewed_at' <<<"$PHASE_OUT")" "$(jq -r '.renewed_at' <<<"$PHASE_LEASE")"

# Remove the store the items actually landed in as well as the one this suite
# asked for; on a host that translates the path they are not the same directory.
# The guard is not optional: LIVE_FILE comes from the created item's reported
Expand Down
9 changes: 9 additions & 0 deletions plugins/work-items/tools/work-item-tracker/lib/lease.sh
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,15 @@ wit_lease_ttl_within_cap() {
((10#$1 * 60 + 10#$2 <= WIT_LEASE_MAX_TTL_HOURS * 60))
}

# wit_lease_phase_valid <kind> — 0 when <kind> is a lease phase claim accepts
# (CONTRACT.md "Lease protocol"). A lease with no phase field is an implementation claim.
wit_lease_phase_valid() {
case "$1" in
triage | discovery | implementation) return 0 ;;
*) return 1 ;;
esac
}

# wit_lease_is_live <lease-json> <now-epoch> — 0 when no superseded_at and not expired.
# A lease renewed more than 300 s after now, or with a TTL over WIT_LEASE_MAX_TTL_HOURS, is not live.
wit_lease_is_live() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,10 @@ Verbs:
[--type <name>] [--parent <id>] [--blocked-by <id>[,<id>]] [--repo <owner>/<repo>]
get-item <id>
claim <id> [--ttl-hours <n>] [--ttl-minutes <n>] [--session-id <s>] [--holder <h>] [--lease-only]
[--phase triage|discovery|implementation]
--ttl-minutes (0-59) ADDS to --ttl-hours; for a sub-hour lease pass
--ttl-hours 0 --ttl-minutes <n>
--phase is github and local-markdown only; absent means implementation
renew-lease <id> --lease-comment-id <n>
release <id> --lease-comment-id <n> [--holder <h>]
reclaim <id>
Expand Down
Loading