diff --git a/docs/adr/0017-ship-the-product-code-lane-as-its-own-skill.md b/docs/adr/0017-ship-the-product-code-lane-as-its-own-skill.md new file mode 100644 index 0000000000..9147de558d --- /dev/null +++ b/docs/adr/0017-ship-the-product-code-lane-as-its-own-skill.md @@ -0,0 +1,82 @@ +# Ship the product-code lane as its own skill, not an argument of `overengineering:audit` + +- Status: accepted +- Date: 2026-08-23 + +## Context + +The `overengineering` plugin V1 (#2961) shipped one lane: the enforcement surface. Its scrutiny +method was deliberately written lane-reusable at the plugin level +(`plugins/overengineering/context/scrutiny-method.md`), whose "Lane binding" section states that +§§1-12 are lane-independent and that a lane supplies four things: the item inventory, the layer +vocabulary and discovery probes, the evidence sources mapped onto the §2 tiers, and the lane's +protected-class defaults. + +A second lane covering code-level overengineering in product code (speculative abstraction, unearned +indirection, premature generality) was judged valuable during the `overengineering-detection-skill` +interview and deferred to #2897, which left three questions open: the lane's walker and evidence +sources, its boundary against existing owners, and whether it ships as its own skill or as an +argument-selected lane of `overengineering:audit`. (#2897 phrased that as "a third skill", written +when the plugin had two; `delta` has since taken that slot, so the lane in question is the fourth +skill and the third lane.) + +The first two are answered in `plugins/overengineering/context/product-code-lane.md`. This ADR +records the third. + +## Decision + +**The product-code lane ships as its own skill.** `overengineering:audit` stays bound to the +enforcement surface, and its `argument-hint` layer vocabulary is not extended to cover product code. + +This is also now the plugin's established pattern rather than a new one. The `delta` lane (#2898, +plugin `0.2.0`) shipped while this decision was being written, and it shipped the same way: a third +skill composing `audit` rather than an argument selecting a mode inside it. Two lanes, two skills, +and this makes three. + +Four reasons, in the order they carry weight: + +- **Skill descriptions are the routing surface, and they are budgeted.** Claude Code drops skill + descriptions from the listing least-invoked-first, so a description that dilutes its trigger + vocabulary makes the skill harder to match, which makes it less invoked, which drops it sooner. + `overengineering:audit`'s description is already dense with enforcement-surface vocabulary, and + this lane's triggers ("is this abstraction earning its keep", "do we need this interface") share + no keywords with it. Fusing them degrades matching for both lanes. This fleet ships + `claude-ops:audit-skill-visibility` because this failure mode is real here. +- **The protected classes do not map.** §7's enforcement classes are about guards and their bypass. + The product-code lane's classes are about changing code that runs: published API surface under a + compatibility commitment, serialization and wire formats, concurrency primitives, error-containment + boundaries, and testability seams. A shared skill would carry two disjoint protected-class sets and + have to select between them by argument, which is the shape of two skills. +- **Retirement means something different, and costs differently.** Retiring an enforcement mechanism + removes a check. Retiring an abstraction changes code that runs, so §11's rollback ladder carries + behavior risk that the enforcement lane's does not, and `realign`'s enforcement-shaped ladder + (disable, narrow, warn-only, remove) is not the product-code ladder (inline, collapse, narrow, + delete). +- **§10's YAGNI boundary is load-bearing here rather than a corner case.** Fowler's YAGNI is about + product code. The out-of-scope list has to be restated in code-level terms, because "delete the + abstraction" and "delete the safety net" can look alike in a diff. + +### The shared machinery is extracted, not forked + +The one real argument for fusion is that both lanes share walk orchestration. That is solved by +extraction, which is what this plugin already did once for the method itself: + +When the lane is implemented, the lane-independent parts of +`plugins/overengineering/skills/audit/context/surface-walk.md` (the per-layer loop, the aggregating +container rule under "Granularity", incremental artifact writes, and the closing step) move to the +plugin root alongside `scrutiny-method.md`, and both lanes bind them. The enforcement layers 1-10 +stay with `audit`. Neither lane restates the shared parts, per the same no-second-statement rule +`scrutiny-method.md` already enforces. + +Until that extraction lands, `product-code-lane.md` points at the enforcement lane's copy rather than +duplicating it, so there is one statement of each rule at every point in the transition. + +## Consequences + +- `overengineering:audit` keeps its current scope and description. No change to it is required by + this decision. +- `realign` will need a product-code rollback ladder before the new skill can execute findings, and + that is the natural next slice after the skill's audit half. +- The boundary against `/simplify`, `code-tidying`, and `architecture:improve` is documented in + `product-code-lane.md` §6 as three operational handoffs rather than a declaration, so the new + skill's description can point at it rather than restating it. diff --git a/plugins/overengineering/.claude-plugin/plugin.json b/plugins/overengineering/.claude-plugin/plugin.json index 031e947911..5e35499b46 100644 --- a/plugins/overengineering/.claude-plugin/plugin.json +++ b/plugins/overengineering/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "overengineering", - "version": "0.2.1", + "version": "0.2.2", "description": "Evidence-earned-keep audit of an existing enforcement surface — agent hooks and standing instructions, repository and version-control hooks, CI lanes and gate scripts, branch protections, forge apps, declared external integrations — treating every incumbent mechanism as a retirement candidate until empirical evidence earns its keep, arguing every verdict in cost of carry, capping retirement-direction verdicts on security-class artifacts at FLAG-FOR-HUMAN, and realigning to the simplest adequate solution behind an explicit per-item human gate. The audit is read-only and emits a diffable findings artifact; realignment is a separate, explicitly invoked skill; and a third read-only lane re-runs the audit on whatever cadence the consumer wires and reports only what moved since the last run, above a configurable noise budget.", "author": { "name": "Melodic Software", diff --git a/plugins/overengineering/CHANGELOG.md b/plugins/overengineering/CHANGELOG.md index 799e7883e0..81e3185d7a 100644 --- a/plugins/overengineering/CHANGELOG.md +++ b/plugins/overengineering/CHANGELOG.md @@ -3,6 +3,29 @@ All notable changes to the `overengineering` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.2.2] + +### Added + +- **Product-code lane specification (#2897).** `context/product-code-lane.md` supplies the four + things `scrutiny-method.md` asks a lane for, for code-level overengineering in product code: + the item inventory (the abstraction, never the file), an eight-layer vocabulary with discovery + probes (`single-implementation`, `extension-points`, `configuration`, `generality`, `layering`, + `speculative-api`, `dead-branches`, `premature-async`), the evidence sources mapped onto the §2 + tiers, and protected-class defaults extending §7 (published API surface, serialization and wire + formats, concurrency primitives, error-containment boundaries, testability seams). It names the + lane's signature tier-2 probe, whether the second implementation ever arrived, which is what makes + speculative generality checkable as a falsified prediction rather than a matter of taste, and + documents the boundary against `/simplify`, `code-tidying`, and `architecture:improve` as three + operational handoffs. The document is a specification ahead of its skill; no skill or behavior + changes in this release. + +### Changed + +- **`scrutiny-method.md` points at the second lane.** Its "Lane binding" section previously + forward-referenced "a future product-code lane" with nowhere to go; it now links the specification + and the ADR recording the lane's shipping shape. + ## [0.2.1] ### Fixed diff --git a/plugins/overengineering/context/product-code-lane.md b/plugins/overengineering/context/product-code-lane.md new file mode 100644 index 0000000000..e3528a1238 --- /dev/null +++ b/plugins/overengineering/context/product-code-lane.md @@ -0,0 +1,241 @@ +# Product-code lane — the lane binding + +The second lane of this plugin's scrutiny method, covering code-level overengineering in product +code: speculative abstraction, unearned indirection, premature generality. + +**The method is not restated here.** [`scrutiny-method.md`](scrutiny-method.md) §§1-12 are +lane-independent and apply verbatim. This document supplies only the four things its "Lane binding" +section asks a lane for: the item inventory, the layer vocabulary and discovery probes, the evidence +sources mapped onto the §2 tiers, and the lane's protected-class defaults extending §7. Every bare +`§N` below is a section of that document. + +Status: **specification**. The lane's shipping shape is recorded in +[ADR 0017](../../../docs/adr/0017-ship-the-product-code-lane-as-its-own-skill.md); this document is +what that skill binds when it lands. + +## Why this lane needs its own binding at all + +The enforcement lane audits mechanisms that *govern* work. This lane audits the code that *does* the +work, and the difference changes what evidence exists, what a retirement costs, and what may not be +touched: + +- Retiring an enforcement mechanism removes a check. Retiring an abstraction **changes code that + runs**, so §11's rollback ladder carries behavior risk the enforcement lane does not. +- Enforcement mechanisms are sparse and individually named. Product code is dense, so §1's carry + cost is paid per *reader*, and the item inventory below has to aggregate or it will produce a wall + of findings rather than a spine. +- Fowler's YAGNI is *about* product code, which makes §10's boundary load-bearing here rather than a + corner case. Restated for this lane below. + +## 1. Item inventory — what counts as one auditable artifact + +**The item is the abstraction, never the file.** One item is a construct plus everything that exists +to serve it: the declaration, its implementations, its registration or wiring, and its call sites. An +interface with three implementations and eleven call sites is **one** item with a members list, not +fifteen findings. + +This is the enforcement lane's aggregating-container rule +([`../skills/audit/context/surface-walk.md`](../skills/audit/context/surface-walk.md), +"Granularity") applied to code. Two consequences worth stating because they are easy to get wrong: + +- A file touched by several unrelated items is not itself an item. Attribute each finding to its + abstraction and let the file appear in several members lists. +- A single-member container is still a container. An interface with exactly one implementation is the + canonical finding of this lane, and reporting it as "one interface" rather than "an interface and + its sole implementation" hides the cost being argued about. + +## 2. Layer vocabulary and discovery probes + +Eight layers, each a distinct *shape* of unearned generality. They are ordered by how cheaply the +evidence usually settles them, so a scoped pass can stop early. + +The probes below are deliberately written as starting points, not as a portable command set. This +plugin's consumers span languages with incompatible tooling, and a grep that is precise in one +ecosystem is noise in another. Resolve each probe against the consumer's actual language and +toolchain, and record which probe form was used, per §2's rule that a verdict names the source it +consulted. + +### Layer 1 - `single-implementation` (unearned indirection) + +Interfaces, abstract bases, protocols, and traits with exactly one concrete implementation; wrapper +types whose every method delegates without adding behavior. + +**Discovery probes.** Enumerate declared abstract types, then count concrete implementors of each. +The count is the finding; the history is the verdict. + +**Layer notes.** A single implementation is a *question*, never a verdict on its own. Test doubles, +declared external implementors, and dependency-inversion boundaries that exist to break a compile-time +cycle are all legitimate single-implementation cases. §4's intent reconstruction is what separates +them. + +### Layer 2 - `extension-points` + +Registries, strategy tables, factory maps, plugin loaders, and hook dispatchers with zero or one +registered member. + +**Discovery probes.** Find the registration surface, then enumerate registrations. An extension point +whose only registration is its own default is the shape this layer exists to surface. + +### Layer 3 - `configuration` + +Options, settings, feature flags, and tunables whose non-default value is never set anywhere in the +tree, in deployment configuration, or in recorded runtime. + +**Discovery probes.** Enumerate declared options, then search for each being *set* rather than *read*. +An option only ever read is a constant with extra steps. + +**Layer notes.** Overlaps the enforcement lane where the option gates a mechanism. Attribute to +whichever lane owns the thing being configured, and cross-reference rather than reporting twice. + +### Layer 4 - `generality` (premature parameterization) + +Type parameters, generic containers, and function parameters instantiated at exactly one type or +called with exactly one value at every call site. + +**Discovery probes.** For each parameter, collect the set of distinct arguments across all call sites. +A set of size one is the finding. + +### Layer 5 - `layering` (pass-through) + +Layers that only forward: mappers between shapes that are structurally identical, repository or +service wrappers that add no behavior over what they wrap, DTOs identical to the entity they mirror. + +**Discovery probes.** For each layer boundary, sample the transformations and classify each as +identity or not. A boundary that is all-identity is the finding. + +**Layer notes.** The strongest counter-evidence here is a *declared* intent to vary the two sides +independently. §4 asks whether that variation ever arrived; tier 2 answers it. + +### Layer 6 - `speculative-api` + +Exported or public surface with no in-repo caller and no declared external consumer. + +**Discovery probes.** Diff the exported set against the called set. Then check the published-surface +question before drawing any conclusion, because this layer is where the protected classes below bite +hardest: an unused export on a published package is a semver commitment, not dead code. + +### Layer 7 - `dead-branches` + +Permanently-settled feature flags, unreachable conditionals, and compatibility shims for versions, +platforms, or dependencies no longer supported. + +**Discovery probes.** For flags, the tier-1 evaluation record if one exists, else the set of values +the flag is ever assigned. For shims, the declared minimum supported version against the version the +shim targets. + +### Layer 8 - `premature-async` (unearned machinery) + +Concurrency, caching, pooling, batching, and retry machinery introduced without a measured bottleneck +behind it. + +**Discovery probes.** Locate the machinery, then look for the measurement that motivated it: a +benchmark, a profile, a load test, an incident. This layer is where §2's "silence is UNPROVEN, never +KEEP" does the most work, and also where an ablation (§8) is most often unsafe, since removing a +concurrency control can corrupt rather than merely slow. + +## 3. Evidence sources, mapped onto the §2 tiers + +| Tier | This lane's sources | Lane-specific caveat | +|---|---|---| +| 1 | Coverage records, profiler and APM traces, feature-flag evaluation records, production logs | Coverage is this lane's workhorse tier 1, and its caveat is sharp: coverage records what the **test suite** executed, which is evidence about the suite unless production telemetry corroborates it | +| 2 | The commit that introduced the construct and its linked issue; implementation count over time; churn on the abstraction versus on what it wraps; revert history | The lane's signature probe lives here, see below | +| 3 | Incidents the construct was introduced to prevent, and incidents it caused | Absence stays ambiguous by construction (§7) | +| 4 | Author or maintainer attestation: the consumer that was planned, the variation that was expected | Recorded as attestation with date and speaker, never promoted to a measurement (§2) | +| 5 | Docstrings claiming extensibility, ADRs, design docs, TODO and FIXME notes | Claims to verify, nothing more; a docstring promising extensibility is the hypothesis, not the finding | + +### The signature probe: did the second implementation ever arrive? + +Speculative generality is a **prediction** that variation would arrive. Tier 2 can check the +prediction directly and cheaply, which is what makes this lane tractable at all: + +Date the abstraction's introduction, then count its implementations, registrations, or distinct +instantiations at that date and at every point since. A construct built for variation that has stood +at one member for years has a falsified prediction behind it, and that is a tier-2 measurement rather +than a reviewer's taste. A construct that reached two members in a month has an earned keep, on the +same evidence. + +State the elapsed time in the finding. "One implementation" is a count; "one implementation across +four years and 200 commits to the file it wraps" is an argument. §9's analogical thresholds govern +how far such a number transfers. + +**A shallow clone makes this probe unavailable, not silent** (§2). The preflight below establishes +which it is before any layer runs. + +## 4. Protected-class defaults, extending §7 + +§7's enforcement classes continue to apply where product code implements them, an authorization check +written as a plain function is protected by §7 without needing a second listing. This lane adds +classes whose hazard is specific to changing code that runs: + +- **Published API surface with external consumers.** Anything under a semver or compatibility + commitment. Unused-in-repo is not unused, and this lane's layer 6 will surface these routinely. +- **Serialization, persistence, and wire formats.** Any construct whose shape is written by one + version and read by another, including database schemas, message payloads, cached representations, + and on-disk state. Collapsing an "identical" mapper couples the stored shape to the in-memory one. +- **Concurrency and locking primitives.** Removing machinery whose absence corrupts rather than slows. + Overlaps layer 8 deliberately: that layer finds them, this class caps the recommendation. +- **Error-handling, retry, and timeout boundaries.** A pass-through-looking wrapper is often the only + place a failure mode is contained. +- **Seams the test suite depends on structurally.** §10's corollary applied to code: an interface + whose sole non-test implementor is production code, with a test double as its second implementor, + is a testability seam. Retiring it weakens the practice whose output this lane reads as tier-1 + evidence. + +The intentionally-dormant class (§7) carries directly: a compatibility shim with a declared removal +date, and a feature flag mid-rollout, are dormant by design and are not findings. + +## 5. Preflight, and what differs from the enforcement lane + +The enforcement lane's preflight ([`../skills/audit/context/surface-walk.md`](../skills/audit/context/surface-walk.md), +"Preflight") applies as written: repository presence, shallow-clone detection, history depth, +telemetry sink, incident corpus, custody. Three additions are specific to this lane: + +| Probe | What it establishes | +|---|---| +| Language and toolchain inventory | Which discovery probes above are even expressible; a layer with no usable probe in this consumer is reported unavailable, never silently skipped | +| Coverage availability and freshness | Whether tier 1 exists at all, and as of when. A stale coverage artifact is a tier-5 claim about tier-1 data, not tier-1 data | +| Published-surface declaration | Whether this tree publishes a package, library, or API. Settles layer 6's protected-class question before layer 6 runs, rather than per finding | + +Generated, vendored, and machine-owned code is **out of inventory**, not audited and reported. It has +an upstream owner, so §12's delegation applies before any verdict is formed. + +## 6. Boundary against existing owners + +This lane integrates with three neighbours rather than duplicating them. The distinguishing axis is +**retrospective and evidence-gated** versus **prospective and judgment-gated**: this lane is the only +one that asks whether a construct should exist *at all* given what the record says it was built for, +and every verdict it issues cites an evidence tier or is UNPROVEN (§2). + +| Owner | Unit of work | Trigger | Question it answers | +|---|---|---|---| +| `/simplify` | the current diff | before merge | Can the code just written be cleaner? | +| `code-tidying:tidy` | a file or region | on demand, mechanical | Can this be restructured safely without changing behavior? | +| `architecture:improve` | a module | forward-looking | What is the better interface for this? | +| **this lane** | an abstraction | retrospective, evidence-first | Has this construct's generality earned its carry cost? | + +Three handoffs make the boundary operational rather than declarative: + +- **A finding whose answer is "keep, but reshape" is not this lane's.** This lane's remediation + vocabulary is §11's ladder, retire, collapse, inline, narrow. When the evidence supports keeping the + seam but the shape is wrong, say so in the finding and hand off to `architecture:improve`, which + owns redesign and its Design-It-Twice pass. +- **A finding that is safe, mechanical, and behavior-preserving is `code-tidying`'s**, even when this + lane surfaced it. This lane argues about existence; tidying executes structure-preserving changes. +- **This lane never runs on a diff.** A construct introduced in the diff under review has no tier-2 + history yet, so this lane has nothing to weigh it with. Pre-merge scrutiny of new abstraction is + `/simplify`'s and review's; this lane needs the record that only time produces. + +## 7. The §10 boundary, restated for product code + +§10's in-scope/out-of-scope split is where this lane is most likely to be misread, because "delete +the abstraction" and "delete the safety net" can look alike in a diff: + +- **In scope.** Generality carried on anticipated need: indirection with one implementor, extension + points with no extensions, parameters with one argument, layers that only forward, machinery with + no measurement behind it. +- **Out of scope.** The practices that make change safe, in their code-level form: tests and the + seams that make code testable, type declarations and the checker that reads them, error handling, + and the abstractions that exist to contain a failure mode rather than to anticipate a feature. + +A finding that reads "remove this interface so the tests can no longer substitute it" is outside this +method, and the correct response is to say so rather than to argue it on carry cost. diff --git a/plugins/overengineering/context/scrutiny-method.md b/plugins/overengineering/context/scrutiny-method.md index 7280d51be1..9f326fabee 100644 --- a/plugins/overengineering/context/scrutiny-method.md +++ b/plugins/overengineering/context/scrutiny-method.md @@ -20,8 +20,12 @@ Sections 1–12 are lane-independent. A lane supplies four things and inherits e V1 ships one lane: the enforcement surface (agent hooks and standing instructions, repository and version-control hooks, CI lanes and gate scripts, branch protections, forge apps and automations, -declared external integrations). A future product-code lane supplies its own four and reuses §§1–12 -verbatim. +declared external integrations). + +The second lane, product code, supplies its own four in +[`product-code-lane.md`](product-code-lane.md) and reuses §§1–12 verbatim. That document is a +specification ahead of its skill; the shipping shape is +[ADR 0017](../../../docs/adr/0017-ship-the-product-code-lane-as-its-own-skill.md). ## 1. The economic frame: carry cost, never build cost diff --git a/plugins/work-items/.claude-plugin/plugin.json b/plugins/work-items/.claude-plugin/plugin.json index 5158075f1b..1f4b6f30ae 100644 --- a/plugins/work-items/.claude-plugin/plugin.json +++ b/plugins/work-items/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "work-items", - "version": "0.39.14", + "version": "0.39.15", "description": "Manages development work items through a provider-neutral tracker seam that ships with the plugin (bundled dispatcher plus github, local-markdown, jira, gitea, and linear adapters; seam plugin-dir canonical, adapters consumer-local-first): dashboard, taxonomy-labeled creation, a race-safe assignee-plus-lease claim protocol, recurring-schedule checks, TODO scanning, stale-lease auditing, plan decomposition into vertical-slice items, a macro-journey router over spec containers (rollup, per-container execution shape, next-step routing), raw-intake triage (issues and unsolicited PRs through raw, verified, briefed, autonomous-eligible states), plus the two work-items loop lanes of the loop-lane convention: a self-paced autonomous work-loop drain (work-class admission gate, adaptive item cap, PR-only) and an attended attend-queue escalation lane. The re-runnable setup skill binds the provider (.work-item-tracker.json), seeds the recurring-schedule seam (.github/recurring-schedule.json), and remaps canonical role labels.", "author": { "name": "Melodic Software", diff --git a/plugins/work-items/CHANGELOG.md b/plugins/work-items/CHANGELOG.md index 87cf23689a..820836f265 100644 --- a/plugins/work-items/CHANGELOG.md +++ b/plugins/work-items/CHANGELOG.md @@ -3,10 +3,51 @@ All notable changes to the `work-items` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. -## [0.39.14] +## [0.39.15] ### Fixed +- **Lease marker parses with trailing content appended to the comment.** `wit_lease_json` + matched only a body ENDING in `-->`, so any comment carrying the lease plus trailing text + (a bot wrapper's attribution footer, a signature, a CI note) parsed as "not a lease". The + failure was silent and unsafe rather than merely lossy: `claim`'s arbitration found no + incumbent lease and granted over a live holder, and `renew-lease` refused to renew a lease + it had just written. The match is now anchored on the FIRST `-->` after the marker, which + is also strictly more correct, since an HTML comment cannot contain `-->`. +- **GitHub adapter claim/reclaim no longer depend on GraphQL.** `claim` and `reclaim` resolved + assignees through `gh issue edit --add-assignee` / `gh issue view --json assignees`, both of + which route through GitHub's GraphQL API. Sandboxed sessions (Claude Code on the web and + remote execution) serve only a pinned set of GraphQL operations and refuse the rest with + HTTP 403, which made the entire lease protocol unrunnable there. Both verbs now use the REST + `…/issues//assignees` endpoints via shared `wit_read_assignees` / `wit_add_assignee` / + `wit_remove_assignee` / `wit_try_remove_assignee` helpers. The identity routing is unchanged: + the helpers take the same `read` (bare `gh`, session identity) / `write` (bot wrapper) writer + argument the adapter already used, so the claim carve-out that assigns the session user rather + than the bot still holds; `@me` is resolved to the login explicitly because REST takes a + literal login. +- **`claim` verifies its own assignment landed.** REST `POST …/assignees` returns 201 and + silently drops a login that cannot be assigned, where `gh issue edit --add-assignee` failed + loudly. Without an explicit check the port would have introduced a new race: `claim` reporting + a held lease while `list-frontier` still saw the item unassigned, putting two workers on one + item. A dropped assignment now exits `4` (auth) before any lease comment is posted. + +### Changed + +- **Claim-protocol test coverage.** `claim.test.sh` exercised only `--help` and usage errors, so + the protocol itself (assign, sole-assignee check, lease post, arbitration) passed vacuously. + `lease-coordination.test.sh` now drives it against the stubbed `gh`: the happy path, the + foreign-assignee conflict with its rollback, and the silently-dropped-assignment guard. Its + `gh` stub matches the REST assignee shapes. +- **`lease-coordination.test.sh` no longer enables errexit by accident.** The renew-lease case + wrapped its call in `set +e` and then "restored" with `set -e 2>/dev/null || true`, which + ENABLES errexit rather than restoring the file's declared `set -uo pipefail` mode. Every later + case expecting a non-zero exit aborted the suite at that line instead of asserting on it, which + is why the claim cases could not be added until it was found. Both sites now use `|| rc=$?`. + +## [0.39.14] + +### Changed + - **Docs:** the generated options block's headless route no longer implies `--config` applies only at install time, and now carries the CLI version its claim was verified against ([#3111](https://github.com/melodic-software/claude-code-plugins/issues/3111)). The block also diff --git a/plugins/work-items/tools/work-item-tracker/adapters/github/README.md b/plugins/work-items/tools/work-item-tracker/adapters/github/README.md index d9e00f6406..715f9d7730 100644 --- a/plugins/work-items/tools/work-item-tracker/adapters/github/README.md +++ b/plugins/work-items/tools/work-item-tracker/adapters/github/README.md @@ -157,9 +157,28 @@ is `gh issue create` only): gh issue edit --add-label "" --remove-label "" ``` -**Carve-out — claim assignment stays on bare `gh`:** `--add-assignee "@me"` MUST resolve to the -session identity (not a bot), so it runs on bare `gh`. Coordination claims go through the seam -`claim` verb, which owns this. +**Carve-out — claim assignment stays on the session identity:** the assignee MUST be the session +user (not a bot), so it runs on bare `gh`. Coordination claims go through the seam `claim` verb, +which owns this. + +**The lease protocol's own assignee ops are REST, not `gh issue`.** `claim` and `reclaim` do not +use `gh issue edit --add-assignee` / `gh issue view --json assignees`: those route through +GitHub's GraphQL API, and sandboxed sessions (Claude Code on the web and remote execution) serve +only a pinned set of GraphQL operations, refusing the rest with HTTP 403 — which made the lease +protocol unrunnable there. They use `gh api` against `…/issues//assignees` instead, through the +`wit_read_assignees` / `wit_add_assignee` / `wit_remove_assignee` / `wit_try_remove_assignee` +helpers in `common.sh`. Those helpers take the same `read`/`write` writer argument as +`wit_run_gh`, so the carve-out above is preserved: `claim` passes `read` (bare `gh`) and resolves +`@me` to the login explicitly, because REST takes a literal login rather than the `@me` alias. + +Note the two APIs differ on an unassignable user: `gh issue edit --add-assignee` failed loudly, +while REST `POST …/assignees` returns 201 and silently drops the login. `claim` therefore +re-reads the assignees and fails `4` (auth) when its own assignment did not land, rather than +reporting a held lease on an item the frontier still sees as unassigned. + +Verbs beyond the lease protocol (`get-item`, `list-items`, `list-sub-items`, `list-frontier`, +`add-sub-item`, `link-blocks`) still resolve GraphQL-only fields (`issueType`, `blockedBy`, +`parent`, `subIssues`) or use `gh issue list`, so they remain unavailable under that restriction. ## Comment on item / edit a comment diff --git a/plugins/work-items/tools/work-item-tracker/adapters/github/claim.sh b/plugins/work-items/tools/work-item-tracker/adapters/github/claim.sh index efce72e477..275d408709 100755 --- a/plugins/work-items/tools/work-item-tracker/adapters/github/claim.sh +++ b/plugins/work-items/tools/work-item-tracker/adapters/github/claim.sh @@ -41,8 +41,10 @@ owner="$WIT_ID_OWNER" repo="$WIT_ID_REPO" number="$WIT_ID_NUMBER" wit_run_gh read api user --jq .login login="$WIT_GH_OUT" -# 1. Assign the session identity. -wit_run_gh read issue edit "$number" -R "$owner/$repo" --add-assignee "@me" +# 1. Assign the session identity. `read` routes to bare gh, so the assignee is +# the session user, not the bot (README "Edit labels / assignees" carve-out); +# `@me` is resolved to $login explicitly because REST takes a literal login. +wit_add_assignee read "$owner" "$repo" "$number" "$login" # Guard the partial-claim window: any failure between this successful # assignment and a successful lease-comment write (step 3) would otherwise @@ -52,16 +54,27 @@ wit_run_gh read issue edit "$number" -R "$owner/$repo" --add-assignee "@me" # inside a function does not run the caller's ERR trap (only its EXIT trap) — # verified empirically — so this must be an EXIT trap, not ERR. _wit_claim_rollback() { - gh issue edit "$number" -R "$owner/$repo" --remove-assignee "@me" >/dev/null 2>&1 || true + wit_try_remove_assignee "$owner" "$repo" "$number" "$login" } trap _wit_claim_rollback EXIT # 2. Sole-assignee check — a different login present means an established claim. -wit_run_gh read issue view "$number" -R "$owner/$repo" --json assignees --jq '[.assignees[].login]' +wit_read_assignees "$owner" "$repo" "$number" assignees="$WIT_GH_OUT" +# Confirm our own assignment actually landed. REST POST /assignees returns 201 +# and silently drops a login that cannot be assigned (no push access), where +# `gh issue edit --add-assignee` used to fail loudly. Without this check a +# silently-dropped assignment would report a held claim while list-frontier +# still saw the item unassigned — two workers on one item, the exact race the +# lease exists to prevent. +if ! jq -e --arg me "$login" 'any(.[]; . == $me)' <<<"$assignees" >/dev/null; then + printf 'claim: assignment of %s did not take effect (insufficient permission to self-assign on %s/%s)\n' \ + "$login" "$owner" "$repo" >&2 + exit "$EX_AUTH" +fi other="$(jq -r --arg me "$login" '[.[] | select(. != $me)] | first // empty' <<<"$assignees")" if [[ -n "$other" ]]; then - gh issue edit "$number" -R "$owner/$repo" --remove-assignee "@me" >/dev/null 2>&1 || true + wit_try_remove_assignee "$owner" "$repo" "$number" "$login" printf 'claim: item already claimed by %s\n' "$other" >&2 exit "$EX_CONFLICT" fi @@ -114,7 +127,7 @@ if [[ -n "$winner_id" && "$winner_id" != "$our_comment_id" ]]; then # would also catch this on exit; the explicit call keeps the disarm/removal # decision made in this block, not left implicit to the trap). if [[ "$winner_holder" != "$login" ]]; then - gh issue edit "$number" -R "$owner/$repo" --remove-assignee "@me" >/dev/null 2>&1 || true + wit_try_remove_assignee "$owner" "$repo" "$number" "$login" else trap - EXIT # same-login race — @me legitimately stays assigned fi diff --git a/plugins/work-items/tools/work-item-tracker/adapters/github/common.sh b/plugins/work-items/tools/work-item-tracker/adapters/github/common.sh index 7359073ef3..1f90b5d549 100644 --- a/plugins/work-items/tools/work-item-tracker/adapters/github/common.sh +++ b/plugins/work-items/tools/work-item-tracker/adapters/github/common.sh @@ -155,6 +155,46 @@ wit_resolve_repo() { printf '%s\n' "$WIT_GH_OUT" } +# --- Assignee ops (REST) ----------------------------------------------------- +# These deliberately use `gh api` (REST) rather than `gh issue view/edit`. The +# `gh issue` subcommands resolve assignees through GraphQL, and sandboxed +# sessions (Claude Code on the web / remote execution) serve only a pinned set +# of GraphQL operations, refusing the rest with HTTP 403 — which made the whole +# lease protocol unrunnable there. REST `…/issues//assignees` is served. +# The argument keeps the identity routing intact: `read` = bare gh = +# the session identity the claim carve-out requires (README "Edit labels / +# assignees"); `write` = the bot wrapper. + +# wit_read_assignees — WIT_GH_OUT = JSON array of logins. +wit_read_assignees() { + wit_run_gh read api "repos/$1/$2/issues/$3" --jq '[.assignees[].login]' +} + +# wit_add_assignee +# NOTE: unlike `gh issue edit --add-assignee`, REST POST silently IGNORES a user +# who cannot be assigned (no push access) and still returns 201. Callers that +# depend on the assignment having landed MUST verify with wit_read_assignees. +wit_add_assignee() { + local writer="$1" + wit_run_gh "$writer" api --method POST "repos/$2/$3/issues/$4/assignees" \ + -f "assignees[]=$5" --jq '[.assignees[].login]' +} + +# wit_remove_assignee +wit_remove_assignee() { + local writer="$1" + wit_run_gh "$writer" api --method DELETE "repos/$2/$3/issues/$4/assignees" \ + -f "assignees[]=$5" --jq '[.assignees[].login]' +} + +# wit_try_remove_assignee — best-effort rollback +# on the session identity; never fails the caller (mirrors the bare-gh `|| true` +# rollback calls this replaces). +wit_try_remove_assignee() { + gh api --method DELETE "repos/$1/$2/issues/$3/assignees" \ + -f "assignees[]=$4" >/dev/null 2>&1 || true +} + # wit_issue_url wit_issue_url() { printf 'https://github.com/%s/%s/issues/%s\n' "$1" "$2" "$3" diff --git a/plugins/work-items/tools/work-item-tracker/adapters/github/lease-coordination.test.sh b/plugins/work-items/tools/work-item-tracker/adapters/github/lease-coordination.test.sh index 8697d16c42..b1e3e97de3 100755 --- a/plugins/work-items/tools/work-item-tracker/adapters/github/lease-coordination.test.sh +++ b/plugins/work-items/tools/work-item-tracker/adapters/github/lease-coordination.test.sh @@ -16,6 +16,7 @@ source "$SCRIPT_DIR/../../tests/lib.sh" RENEW="$SCRIPT_DIR/renew-lease.sh" RECLAIM="$SCRIPT_DIR/reclaim.sh" +CLAIM="$SCRIPT_DIR/claim.sh" ID="github:acme/widgets#1" # --- gh stub: a bash function (always wins over the external gh, and is inherited @@ -25,38 +26,57 @@ ID="github:acme/widgets#1" # by --jq (`length` = activity count) vs (lease list), and successive lease-list # reads pick lease-comments- when present so a revalidation read can differ. --- gh() { - local d="${GH_STUB_DIR:?}" args="$*" a prev n + local d="${GH_STUB_DIR:?}" args="$*" a n case "$args" in - *"--method PATCH"*) - for a in "$@"; do - case "$a" in repos/*/issues/comments/*) printf 'PATCH %s\n' "${a##*/comments/}" >>"$d/calls.log" ;; *) : ;; esac - done - printf '1\n' + *"--method PATCH"*) + for a in "$@"; do + case "$a" in repos/*/issues/comments/*) printf 'PATCH %s\n' "${a##*/comments/}" >>"$d/calls.log" ;; *) : ;; esac + done + printf '1\n' + ;; + # Assignee ops are REST (`gh api …/issues//assignees`), not `gh issue edit` + # — the gh subcommands route through GraphQL, which sandboxed sessions refuse. + # The login rides in an `assignees[]=` -f field, so it is read off that + # field rather than off a positional flag argument. Both must be matched + # BEFORE the generic --paginate/-f branches below. + *"--method DELETE"*"/assignees"*) + for a in "$@"; do + case "$a" in "assignees[]="*) printf 'REMOVE_ASSIGNEE %s\n' "${a#assignees[]=}" >>"$d/calls.log" ;; *) : ;; esac + done + cat "$d/assignees" 2>/dev/null || printf '[]\n' + ;; + *"--method POST"*"/assignees"*) + for a in "$@"; do + case "$a" in "assignees[]="*) printf 'ADD_ASSIGNEE %s\n' "${a#assignees[]=}" >>"$d/calls.log" ;; *) : ;; esac + done + cat "$d/assignees" 2>/dev/null || printf '[]\n' + ;; + *".assignees[].login"*) cat "$d/assignees" ;; + # The timeline read also carries --paginate, so it must be matched by its path + # BEFORE the generic --paginate (/comments) branch below — order is load-bearing. + *"/timeline"*) cat "$d/pr-activity" 2>/dev/null || printf '0\n' ;; + *"--paginate"*) + case "$args" in + *length*) cat "$d/comment-activity" 2>/dev/null || printf '0\n' ;; + *) + n=$(($(cat "$d/lease-comments.count" 2>/dev/null || printf '0') + 1)) + printf '%s\n' "$n" >"$d/lease-comments.count" + if [[ -f "$d/lease-comments-$n" ]]; then cat "$d/lease-comments-$n"; else cat "$d/lease-comments"; fi ;; - *"--remove-assignee"*) - prev="" - for a in "$@"; do - [[ "$prev" == "--remove-assignee" ]] && printf 'REMOVE_ASSIGNEE %s\n' "$a" >>"$d/calls.log" - prev="$a" - done - ;; - *"--json assignees"*) cat "$d/assignees" ;; - # The timeline read also carries --paginate, so it must be matched by its path - # BEFORE the generic --paginate (/comments) branch below — order is load-bearing. - *"/timeline"*) cat "$d/pr-activity" 2>/dev/null || printf '0\n' ;; - *"--paginate"*) - case "$args" in - *length*) cat "$d/comment-activity" 2>/dev/null || printf '0\n' ;; - *) - n=$(( $(cat "$d/lease-comments.count" 2>/dev/null || printf '0') + 1 )) - printf '%s\n' "$n" >"$d/lease-comments.count" - if [[ -f "$d/lease-comments-$n" ]]; then cat "$d/lease-comments-$n"; else cat "$d/lease-comments"; fi - ;; - esac - ;; - *"issues/comments/"*) cat "$d/comment" ;; - *"-f body="*) printf 'POST_COMMENT\n' >>"$d/calls.log"; printf '1\n' ;; - *) printf 'gh-stub: unhandled: %s\n' "$args" >&2; return 90 ;; + esac + ;; + *"issues/comments/"*) cat "$d/comment" ;; + # claim resolves the session identity before assigning; $d/login lets a scenario + # choose whose claim it is. + *"api user"*) cat "$d/login" 2>/dev/null || printf 'alice\n' ;; + *"-f body="*) + printf 'POST_COMMENT\n' >>"$d/calls.log" + printf '1\n' + ;; + *) + printf 'gh-stub: unhandled: %s\n' "$args" >&2 + return 90 + ;; esac } export -f gh @@ -71,7 +91,11 @@ marker() { lease_array() { jq -cn --argjson id "$1" --arg body "$2" '[{id:$id, node_id:"MDEx", body:$body, created_at:"2020-01-01T00:00:00Z"}]'; } -new_scenario() { GH_STUB_DIR="$(mktemp -d)"; export GH_STUB_DIR; : >"$GH_STUB_DIR/calls.log"; } +new_scenario() { + GH_STUB_DIR="$(mktemp -d)" + export GH_STUB_DIR + : >"$GH_STUB_DIR/calls.log" +} calls() { cat "$GH_STUB_DIR/calls.log"; } cleanup_scenario() { rm -rf "$GH_STUB_DIR"; } @@ -87,10 +111,13 @@ new_scenario EXP_MARKER="$(marker alice "$PAST" 24)" lease_array 123 "$EXP_MARKER" >"$GH_STUB_DIR/lease-comments" jq -cn --arg b "$EXP_MARKER" '{body:$b, issue:"1"}' >"$GH_STUB_DIR/comment" -set +e -bash "$RENEW" "$ID" --lease-comment-id 123 >/dev/null 2>&1 -rc=$? -set -e 2>/dev/null || true +# `|| rc=$?` rather than a `set +e` / `set -e` pair: this file runs under +# `set -uo pipefail` with errexit deliberately OFF, and the pair used to restore +# it with `set -e 2>/dev/null || true`, which ENABLES errexit rather than +# restoring the prior state. Every later case that expected a non-zero exit then +# aborted the suite at that line instead of asserting on it. +rc=0 +bash "$RENEW" "$ID" --lease-comment-id 123 >/dev/null 2>&1 || rc=$? assert_eq "renew-lease returns conflict (7) for an expired active lease" "7" "$rc" if grep -q '^PATCH' "$GH_STUB_DIR/calls.log"; then fail "renew-lease does NOT revive the expired lease (no PATCH)" "no PATCH logged" "PATCH logged" @@ -105,7 +132,8 @@ new_scenario LIVE_MARKER="$(marker alice "$FUTURE" 24)" lease_array 123 "$LIVE_MARKER" >"$GH_STUB_DIR/lease-comments" jq -cn --arg b "$LIVE_MARKER" '{body:$b, issue:"1"}' >"$GH_STUB_DIR/comment" -out="$(bash "$RENEW" "$ID" --lease-comment-id 123 2>/dev/null)"; rc=$? +out="$(bash "$RENEW" "$ID" --lease-comment-id 123 2>/dev/null)" +rc=$? assert_eq "renew-lease succeeds (0) for a live active lease" "0" "$rc" assert_eq "renew-lease emits the renewed lease record" "$ID" "$(jq -r '.id' <<<"$out")" assert_contains "renew-lease PATCHes the live lease comment" "$(calls)" "PATCH 123" @@ -121,7 +149,8 @@ new_scenario EXP_MARKER="$(marker alice "$PAST" 24)" lease_array 123 "$EXP_MARKER" >"$GH_STUB_DIR/lease-comments" jq -cn '["alice","bob"]' >"$GH_STUB_DIR/assignees" -out="$(bash "$RECLAIM" "$ID" 2>/dev/null)"; rc=$? +out="$(bash "$RECLAIM" "$ID" 2>/dev/null)" +rc=$? assert_eq "reclaim succeeds (0)" "0" "$rc" assert_eq "reclaim reports reclaimed:true" "true" "$(jq -r '.reclaimed' <<<"$out")" assert_contains "reclaim removes the expired lease holder (alice)" "$(calls)" "REMOVE_ASSIGNEE alice" @@ -137,7 +166,8 @@ CONCURRENT_MARKER="$(marker carol "$FUTURE" 24)" lease_array 123 "$EXP_MARKER" >"$GH_STUB_DIR/lease-comments-1" lease_array 200 "$CONCURRENT_MARKER" >"$GH_STUB_DIR/lease-comments-2" jq -cn '["alice","carol"]' >"$GH_STUB_DIR/assignees" -out="$(bash "$RECLAIM" "$ID" 2>/dev/null)"; rc=$? +out="$(bash "$RECLAIM" "$ID" 2>/dev/null)" +rc=$? assert_eq "reclaim succeeds (0) on a concurrent-claim revalidation" "0" "$rc" assert_eq "reclaim reports reclaimed:false when the lease changed under it" "false" "$(jq -r '.reclaimed' <<<"$out")" assert_not_contains "reclaim removes NO assignee when revalidation fails" "$(calls)" "REMOVE_ASSIGNEE" @@ -154,11 +184,57 @@ RENEWED_MARKER="$(marker alice "$FUTURE" 24)" lease_array 123 "$EXP_MARKER" >"$GH_STUB_DIR/lease-comments-1" lease_array 123 "$RENEWED_MARKER" >"$GH_STUB_DIR/lease-comments-2" jq -cn '["alice"]' >"$GH_STUB_DIR/assignees" -out="$(bash "$RECLAIM" "$ID" 2>/dev/null)"; rc=$? +out="$(bash "$RECLAIM" "$ID" 2>/dev/null)" +rc=$? assert_eq "reclaim succeeds (0) when the lease was renewed in place during the window" "0" "$rc" assert_eq "reclaim reports reclaimed:false when the lease was renewed in place" "false" "$(jq -r '.reclaimed' <<<"$out")" assert_not_contains "reclaim removes NO assignee when the lease was renewed in place" "$(calls)" "REMOVE_ASSIGNEE" assert_not_contains "reclaim does not supersede when the lease was renewed in place" "$(calls)" "PATCH" cleanup_scenario +# =========================================================================== +# Finding 3 — claim's assignment path. `claim.test.sh` covers only --help and +# usage errors, so the protocol itself (assign → sole-assignee check → lease → +# arbitration) had no coverage; these drive it against the stub. +# =========================================================================== + +# [happy path] nobody else assigned, our lease is the only live one → claim holds. +new_scenario +printf 'alice\n' >"$GH_STUB_DIR/login" +jq -cn '["alice"]' >"$GH_STUB_DIR/assignees" +lease_array 1 "$(marker alice "$FUTURE" 24)" >"$GH_STUB_DIR/lease-comments" +out="$(bash "$CLAIM" "$ID" --ttl-hours 24 2>/dev/null)" +rc=$? +assert_eq "claim succeeds (0) on an unclaimed item" "0" "$rc" +assert_eq "claim assigns the session identity" "alice" "$(jq -r '.holder' <<<"$out")" +assert_eq "claim returns the lease comment handle" "1" "$(jq -r '.lease_comment_id' <<<"$out")" +assert_contains "claim assigns via REST" "$(calls)" "ADD_ASSIGNEE alice" +cleanup_scenario + +# [foreign assignee] our assignment lands, but bob already holds the item → +# conflict (7), and our own @me assignment is rolled back. +new_scenario +printf 'alice\n' >"$GH_STUB_DIR/login" +jq -cn '["alice","bob"]' >"$GH_STUB_DIR/assignees" +lease_array 1 "$(marker alice "$FUTURE" 24)" >"$GH_STUB_DIR/lease-comments" +rc=0 +bash "$CLAIM" "$ID" --ttl-hours 24 >/dev/null 2>&1 || rc=$? +assert_eq "claim returns conflict (7) when another login is assigned" "7" "$rc" +assert_contains "claim rolls its own assignment back on conflict" "$(calls)" "REMOVE_ASSIGNEE alice" +cleanup_scenario + +# [silently dropped assignment] REST POST /assignees returns 201 but drops a login +# that cannot be assigned. Without an explicit landed-check claim would report a +# held lease while list-frontier still saw the item unassigned — two workers on one +# item. Must fail auth (4) instead, and never post a lease comment. +new_scenario +printf 'alice\n' >"$GH_STUB_DIR/login" +jq -cn '[]' >"$GH_STUB_DIR/assignees" +lease_array 1 "$(marker alice "$FUTURE" 24)" >"$GH_STUB_DIR/lease-comments" +rc=0 +bash "$CLAIM" "$ID" --ttl-hours 24 >/dev/null 2>&1 || rc=$? +assert_eq "claim fails auth (4) when the assignment is silently dropped" "4" "$rc" +assert_not_contains "claim posts no lease when the assignment never landed" "$(calls)" "POST_COMMENT" +cleanup_scenario + [[ $FAILED -eq 0 ]] || exit 1 diff --git a/plugins/work-items/tools/work-item-tracker/adapters/github/reclaim.sh b/plugins/work-items/tools/work-item-tracker/adapters/github/reclaim.sh index 472f24be41..7bbb27da2d 100755 --- a/plugins/work-items/tools/work-item-tracker/adapters/github/reclaim.sh +++ b/plugins/work-items/tools/work-item-tracker/adapters/github/reclaim.sh @@ -80,9 +80,9 @@ fi # unassigned while that lease stays live: two workers on one item. Guard on a # fresh assignee read so a holder already unassigned (idempotent re-run) is a no-op. holder="$(jq -r '.holder' <<<"$lease_json")" -wit_run_gh read issue view "$number" -R "$owner/$repo" --json assignees --jq '[.assignees[].login]' +wit_read_assignees "$owner" "$repo" "$number" if jq -e --arg h "$holder" 'any(.[]; . == $h)' <<<"$WIT_GH_OUT" >/dev/null; then - wit_run_gh write issue edit "$number" -R "$owner/$repo" --remove-assignee "$holder" + wit_remove_assignee write "$owner" "$repo" "$number" "$holder" fi superseded="$(jq -c --arg ts "$now" '. + {superseded_at: $ts}' <<<"$lease_json")" diff --git a/plugins/work-items/tools/work-item-tracker/lib/lease.sh b/plugins/work-items/tools/work-item-tracker/lib/lease.sh index 3ffbcc0867..68554cab75 100755 --- a/plugins/work-items/tools/work-item-tracker/lib/lease.sh +++ b/plugins/work-items/tools/work-item-tracker/lib/lease.sh @@ -9,14 +9,23 @@ readonly WIT_LEASE_MARKER='` after the marker, not on the body ENDING there: +# a comment carrying the lease may have trailing content appended by whatever +# posts it — a bot wrapper's attribution footer, a signature, a CI note. An +# end-anchored match treated every such comment as "not a lease", which is +# silent and dangerous rather than merely lossy: claim's arbitration would find +# no incumbent lease and grant, and renew-lease would refuse to renew a lease it +# had just written. Taking the first ` -->` is also strictly more correct — an +# HTML comment cannot contain `-->`, so the first occurrence always closes it. wit_lease_json() { local body="$1" case "$body" in - "$WIT_LEASE_MARKER"*" -->") - body="${body#"$WIT_LEASE_MARKER"}" - printf '%s\n' "${body% -->}" - ;; - *) printf '' ;; + "$WIT_LEASE_MARKER"*" -->"*) + body="${body#"$WIT_LEASE_MARKER"}" + printf '%s\n' "${body%% -->*}" + ;; + *) printf '' ;; esac } diff --git a/plugins/work-items/tools/work-item-tracker/lib/lease.test.sh b/plugins/work-items/tools/work-item-tracker/lib/lease.test.sh index 3da6b40aa0..9046eb40cd 100755 --- a/plugins/work-items/tools/work-item-tracker/lib/lease.test.sh +++ b/plugins/work-items/tools/work-item-tracker/lib/lease.test.sh @@ -16,6 +16,16 @@ assert_eq "wit_iso_to_epoch known ts" "1783828800" "$(wit_iso_to_epoch '2026-07- # Lease-marker JSON extraction. assert_eq "lease json extracted" '{"holder":"me"}' "$(wit_lease_json '')" assert_eq "non-marker → empty" "" "$(wit_lease_json 'just a comment')" +# Trailing content after the marker must still parse: whatever posts the comment +# may append to it (a bot wrapper's attribution footer, a signature, a CI note). +# Treating those as "not a lease" makes claim grant over a live incumbent. +assert_eq "trailing footer after the marker still parses" '{"holder":"me"}' \ + "$(wit_lease_json ' + +--- +_Generated by [Claude Code](https://claude.ai/code)_')" +assert_eq "marker with no close → empty" "" \ + "$(wit_lease_json '