feat(work-items): add sub-hour lease TTL via ttl_minutes - #2463
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 2m 28s —— View job Security Review
Reviewed the diff ( IMPORTANT — dropped The refactor that introduces # before (origin/main)
[[ -n "$renewed" && "$ttl" =~ ^[0-9]+$ ]] || return 1
renewed_epoch="$(wit_iso_to_epoch "$renewed")" || return 1
# after (this PR)
ttl_seconds="$(wit_lease_ttl_seconds "$lease")" || return 1
((ttl_seconds > 0)) || return 1
renewed_epoch="$(wit_iso_to_epoch "$renewed")" || return 1
This matters because lease comments on the wit_run_gh read api --paginate "repos/$owner/$repo/issues/$number/comments?per_page=100" \
--jq '[.[] | select(.body | startswith("` (no `renewed_at`, no `superseded_at`). Under the new code this forged lease is picked up by `wit_select_active_lease` and evaluated as **live**, which:
- blocks legitimate `claim` attempts ([`claim.sh#L97`](https://github.com/melodic-software/claude-code-plugins/blob/9fdcafbac5a5919d83a4452809aeba4ef96a47ef/plugins/work-items/tools/work-item-tracker/adapters/github/claim.sh#L97)), and
- blocks `reclaim` of the item ([`reclaim.sh#L39`](https://github.com/melodic-software/claude-code-plugins/blob/9fdcafbac5a5919d83a4452809aeba4ef96a47ef/plugins/work-items/tools/work-item-tracker/adapters/github/reclaim.sh#L39), [`reclaim.sh#L73`](https://github.com/melodic-software/claude-code-plugins/blob/9fdcafbac5a5919d83a4452809aeba4ef96a47ef/plugins/work-items/tools/work-item-tracker/adapters/github/reclaim.sh#L73)),
effectively letting a single crafted comment lock a work item indefinitely — a denial-of-service on the race-safe claim protocol that this file's own docstring says it guards. The comment-authorship trust boundary predates this PR, but the missing-`renewed_at` → "treated as live" behavior is newly introduced here; the added test (`FRESH_15M`) doesn't cover a missing/empty `renewed_at`, so it went uncaught.
[Fix this →](https://claude.ai/code?q=In%20plugins%2Fwork-items%2Ftools%2Fwork-item-tracker%2Flib%2Flease.sh%2C%20the%20wit_lease_is_live%20function%20%28around%20line%2069%29%20no%20longer%20checks%20that%20%24renewed%20is%20non-empty%20before%20calling%20wit_iso_to_epoch%2C%20unlike%20the%20pre-PR%20version%20which%20had%20%60%5B%5B%20-n%20%22%24renewed%22%20%26%26%20...%20%5D%5D%20%7C%7C%20return%201%60.%20Because%20GNU%20%60date%20-u%20-d%20%22%22%60%20silently%20resolves%20to%20midnight%20today%20instead%20of%20erroring%2C%20a%20lease%20JSON%20missing%20renewed_at%20is%20now%20treated%20as%20live%20instead%20of%20invalid.%20Restore%20the%20explicit%20%5B%5B%20-n%20%22%24renewed%22%20%5D%5D%20%7C%7C%20return%201%20guard%20in%20wit_lease_is_live%20before%20calling%20wit_iso_to_epoch%2C%20and%20add%20a%20lease.test.sh%20case%20for%20a%20lease%20missing%20renewed_at%20to%20prevent%20regression.&repo=melodic-software/claude-code-plugins)
---
No other issues found. `jq --arg`/`jq -cn --arg` is used consistently for all JSON construction in `claim.sh` (both adapters), so there's no injection surface from `ttl_minutes`/`ttl` values into the lease JSON. Input validation (`[[ "$ttl_minutes" =~ ^[0-9]+$ ]]`) is applied consistently in both adapters and in `binding.sh` before the value is trusted. No changes to workflow permissions, trigger types, or secret handling in this PR (only shell/docs/manifest files changed) — supply-chain and workflow-trigger patterns are out of scope for this lane per zizmor's coverage. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fdcafbac5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
39081d7 to
92a164c
Compare
92a164c to
081adac
Compare
|
Claude finished @kyle-sexton's task in 3m 13s —— View job Security Review
Reviewed IMPORTANT — missing The refactor that extracts # origin/main
[[ -n "$renewed" && "$ttl" =~ ^[0-9]+$ ]] || return 1
renewed_epoch="$(wit_iso_to_epoch "$renewed")" || return 1
# this PR
ttl_seconds="$(wit_lease_ttl_seconds "$lease")" || return 1
((ttl_seconds > 0)) || return 1
renewed_epoch="$(wit_iso_to_epoch "$renewed")" || return 1
For the wit_run_gh read api --paginate ".../comments?per_page=100" \
--jq '[.[] | select(.body | startswith("` (no `renewed_at`, no `superseded_at`). `wit_select_active_lease` picks this up and `wit_lease_is_live` now evaluates it as live, which:
- blocks legitimate `claim` attempts ([`claim.sh#L97`](https://github.com/melodic-software/claude-code-plugins/blob/081adac325ab05535d4f8627cad9d4085550c342/plugins/work-items/tools/work-item-tracker/adapters/github/claim.sh#L97)), and
- blocks `reclaim` of the item (same lease-liveness check is used there).
This lets a single crafted comment lock a work item indefinitely — a denial-of-service on the race-safe claim protocol this file's docstring exists to guard. The comment-authorship trust boundary itself predates this PR, but the "missing `renewed_at` → treated as live" behavior is newly introduced here (the pre-PR code correctly rejected it via `[[ -n "$renewed" ]]`). The added `FRESH_15M` test case doesn't cover a missing/empty `renewed_at`, so this regression isn't caught by `lease.test.sh`.
[Fix this →](https://claude.ai/code?q=In%20plugins%2Fwork-items%2Ftools%2Fwork-item-tracker%2Flib%2Flease.sh%2C%20the%20wit_lease_is_live%20function%20%28around%20line%2069%29%20no%20longer%20checks%20that%20%24renewed%20is%20non-empty%20before%20calling%20wit_iso_to_epoch%2C%20unlike%20the%20pre-PR%20version%20which%20had%20%60%5B%5B%20-n%20%22%24renewed%22%20%26%26%20...%20%5D%5D%20%7C%7C%20return%201%60.%20Because%20GNU%20%60date%20-u%20-d%20%22%22%60%20silently%20resolves%20to%20midnight%20today%20instead%20of%20erroring%2C%20a%20lease%20JSON%20missing%20renewed_at%20is%20now%20treated%20as%20live%20instead%20of%20invalid.%20Restore%20the%20explicit%20%5B%5B%20-n%20%22%24renewed%22%20%5D%5D%20%7C%7C%20return%201%20guard%20in%20wit_lease_is_live%20before%20calling%20wit_iso_to_epoch%2C%20and%20add%20a%20lease.test.sh%20case%20for%20a%20lease%20missing%20renewed_at%20to%20prevent%20regression.&repo=melodic-software/claude-code-plugins)
---
No other security issues found. `jq --arg`/`jq -cn --arg` is used consistently for all JSON construction in `claim.sh` (both adapters), so there's no injection surface from `ttl_minutes`/`ttl` values into lease JSON. `[[ "$ttl_minutes" =~ ^[0-9]+$ ]]` input validation is applied consistently in both adapters and in `binding.sh` before the value is trusted. The `binding.sh` provider-name regex guard (`^[a-zA-Z0-9_-]+$`, preventing path traversal into `adapters/<provider>`) is unchanged by this PR. No changes to workflow permissions, trigger types, or secret handling — only shell/docs/manifest files changed in this PR — so supply-chain and workflow-trigger patterns are out of scope for this lane per zizmor's coverage.
*(Out of scope for this security lane, noted only for awareness: `plugins/work-items/CHANGELOG.md` in this PR's HEAD contains unresolved `<<<<<<<`/`=======`/`>>>>>>>` merge-conflict markers — a correctness/documentation issue, not a security one, so not detailed further here. Two functional bugs in `local-markdown/claim.sh` and `github/claim.sh` were also flagged by Codex's review on this PR — an extra `}` breaking the result-emission jq filter, and `ttl_minutes` being dropped from emitted/renewed lease records — both are correctness gaps, not authorization bypasses, since the pre-write lease check still enforces exclusivity either way.)* |
|
Claude finished @kyle-sexton's task in 5m 5s —— View job Code Review
Reviewed for correctness, error handling, test coverage, and maintainability per this repo's Verified independently (not relying on the PR's own test-plan claims): reproduced the Inline findings
Cross-file, not anchorable to a changed line
No other correctness/design issues found in the diff — the |
081adac to
1858fec
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 1m 16s —— View job Security Review
Reviewed IMPORTANT — missing/empty The refactor that extracts # origin/main
[[ -n "$renewed" && "$ttl" =~ ^[0-9]+$ ]] || return 1
renewed_epoch="$(wit_iso_to_epoch "$renewed")" || return 1
# this PR
ttl_seconds="$(wit_lease_ttl_seconds "$lease")" || return 1
((ttl_seconds > 0)) || return 1
renewed_epoch="$(wit_iso_to_epoch "$renewed")" || return 1
For the
A single crafted comment can lock a work item indefinitely — a denial-of-service on the race-safe claim protocol this file's own docstring exists to guard. The comment-authorship trust boundary predates this PR, but the "missing No other security issues found. (Out of scope for this lane, noted only for awareness: |
eb19585 to
2bbe1c8
Compare
Fixes #1034 Adds optional ttl_minutes to lease records, --ttl-minutes on claim, and config.lease_ttl_minutes in the binding. Expiry uses hours*3600 + minutes*60. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
2bbe1c8 to
3792762
Compare
|
Claude finished @cursor[bot]'s task in 1m 45s —— View job Security Review
Skip gate: no new security-relevant surface at this HEAD. The commits since the last completed security review of this PR's feature commit (
Unresolved from prior reviews (unchanged, still present at this HEAD — for awareness, not re-reported as a new finding): No new security findings at this head. All other prior findings on this PR (CHANGELOG merge-conflict markers, the local-markdown |
|
Last security-reviewed head: |
|
Claude finished @cursor[bot]'s task in 3m 32s —— View job Code Review
Reviewed Inline findings (posted on the diff)
Cross-file, not anchorable to a changed line
No other correctness/design issues found. |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 1m 48s —— View job Security Review
Skip gate: no new security-relevant surface at this HEAD. Last completed security review on this PR covered head
Unresolved from prior reviews (unchanged, still present at this HEAD — for awareness, not re-reported as a new finding): No new security findings at this head. Other outstanding items on this PR (the |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 1m 44s —— View job Security Review
Skip gate: no new security-relevant surface at this HEAD. Last completed security review on this PR covered head
All four are exactly the correctness/style items previously flagged by the code-review lane (jq syntax error, unenforced 0–59 bound, missing Unresolved from prior reviews (unchanged, still present at this HEAD — for awareness, not re-reported as a new finding):
No new security findings at this head. All other items previously flagged on this PR remain correctness/style, out of scope for this lane per |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 1m 2s —— View job Security Review
Skip gate: no new security-relevant surface at this HEAD. Last completed security review on this PR covered head Both changes replace an en-dash ( Unresolved from prior reviews (unchanged, still present at this HEAD — for awareness, not re-reported as a new finding):
No new security findings at this head. All other previously flagged items on this PR (CHANGELOG merge-conflict markers, |
|
Claude finished @cursor[bot]'s task in 49s —— View job Code Review
Skip gate: PR is merged (not open). This PR ( For reference, this PR received extensive prior review from both the code-review and security lanes across its lifecycle, including an unresolved security finding (missing |
2 participants
Fixes #1034
Adds optional
ttl_minutesto lease records,--ttl-minutesonclaim, andconfig.lease_ttl_minutesin the binding.Test plan
lease.test.sh(includes fresh 15m case)Related