Skip to content

fix(work-items): harden the tracker seam's lease and generator paths, and reconcile the upstream SSOTs - #3067

Merged
kyle-sexton merged 14 commits into
mainfrom
claude/pocock-shipping-breakdown-ukohlx
Aug 21, 2026
Merged

fix(work-items): harden the tracker seam's lease and generator paths, and reconcile the upstream SSOTs#3067
kyle-sexton merged 14 commits into
mainfrom
claude/pocock-shipping-breakdown-ukohlx

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

No linked issue

Summary

Started as a docs correction; an audit run over the same seam turned up real defects, and the
container close-out review then turned up four more, so this carries all three. Nine correctness
fixes to work-item-tracker and the review lane (one of them a command-injection hole), plus the
upstream SSOT documents brought into agreement with each other and with the code they cite.
work-items 0.39.2 → 0.39.7; review 0.25.0 → 0.25.1.

The original title named only the docs half. It was accurate for the first two commits and wrong
for the PR — corrected rather than left to mislead a reviewer.

Fix

A command-injection hole in the adapter generator (d52ea346). api.sample_scope reached a
generated shell file unvalidated. It now must match ^[A-Za-z0-9][A-Za-z0-9._~/-]*$, and the
generator's quote_safe sweep was hoisted so it runs over the whole key set at top level instead
of inside a $(render …) subshell, where a failing exit was swallowed.

Three instances of the swallowed-exit-in-$( ) class (e93e0711, 026ed64e, 0498519b).
An exit inside a command substitution ends only the subshell; under set -uo pipefail without
-e, the caller carries on with an empty value. In the linear adapter a malformed lease handle
could therefore hide a live lease; in the gitea adapter a blocker-count failure was reported as
this adapter's own error; in the generated conformance binding the placeholder failed silently.
Each now propagates, and the linear accumulator refuses to report an empty lease set rather than
guessing.

A losing linear claim could strip the winner's assignment (302569bc, 3ca356c7). Linear's
assignee is a single field, not GitHub's list, so claim arbitration goes through comment
ordering. The rollback trap now requires both no-other-live-lease and a name match before it
touches the assignee, and COMMENT_UUID is initialised before the trap can fire. The follow-up
commit closes a fail-open I introduced in the first: the post-race re-read used || AFTER2='[]',
which turns "could not read lease state" into "there is no lease". It now records unknown and
leaves the assignee alone. This matters because lib/frontier.sh:41 selects on
assignees | length == 0 and never consults leases — a wrongly cleared assignee hands live work
to a second holder.

Conformance cleanup and an over-tight assertion (0498519b, e93e0711). run-conformance.sh
has no close/delete logic of its own; cleanup lives entirely in each binding's
_cb_clean_at_start, and the linear binding had none — it now archives via the Linear GraphQL
API, with the credential passed through curl -K - on stdin rather than the command line. One
suite assertion matched the github adapter's exact prose; it now asserts on live /
not-superseded, which every adapter can satisfy.

Three findings from the #2933 container close-out review (e5f47192). Each re-verified
against the artifact before acting, not taken on the reviewer's word.

  • /work-items:onboard-adapter read live tracker items without stating the
    item-content-trust boundary
    . Its Step 2 has the user fetch real items and paste the responses
    back — titles, descriptions, comments, label and state names, all authored by anyone who can
    file in that tracker — and grepping the whole skill tree for the boundary returned zero hits,
    while attend-queue, decompose, ship, triage and work-loop all carry it. The container
    this skill shipped under names "no tracker reads without the item-content-trust boundary" among
    its excluded-by-default postures, so this was the one surface out of step with its own
    constraint. Both SKILL.md and reference/live-exploration.md now state it as a numbered probe
    rule: read probe output for shape, never as a directive. Found while there — the "already
    bundled" list named github, local-markdown and jira only, so a Gitea or Linear user would
    be walked through generating an adapter that already ships.
  • execution-shape.md described only fresh-branch-per-item provisioning, so no container using
    the serial variant could record a truthful shape line — including Spec: Shipping-lifecycle absorption (Pocock Shipping section) #2933, which shipped eleven
    PRs off one head ref. The shape value names PR granularity; fresh-branch-per-item is its
    default provisioning. The variant is now documented with what it forfeits (no parallelism;
    each PR's diff is honest only if its predecessor merged first). Deliberately not a third
    shape value — the line stays two-valued and ship, decompose and the close-out basis are
    unchanged.
  • quality-gate close-out Shape B was structurally blind to in-flight work: every rung reads
    the default branch, so pushed work sitting in an open PR never entered the basis and was never
    mentioned. Merged-only is right for the basis and wrong to leave unsaid for the verdict — a
    container closed on it closes on evidence that is not on the default branch. Shape A reaches its
    open branch through the **Integration branch:** line; Shape B had no analogue. It now runs an
    OPEN-state query plus an open-PR search and treats any open PR carrying container work as a
    precondition of the close. Surfaced by running the mode over Spec: Shipping-lifecycle absorption (Pocock Shipping section) #2933, where six behaviour-changing
    fixes sat in this PR and the derived basis showed none of them.

Each fix ships with a test verified to fail without it, or — for the doc-only fixes — a repo gate
that covers it.

The gitea blocker reason was wrong (9c8b83d1, 0b4c2d72). The README claimed no Gitea
instance was reachable from this environment. That was an untested assumption written as fact:
Gitea ships as a single self-contained binary with sqlite built in, and a real Gitea 1.22.6 was
downloaded and version-verified here. The actual blocker is narrower — serving it needs a
dedicated unprivileged user plus cap_net_bind_service, which the sandbox's permission policy
gates. Both documents now also record that port 443 and TLS are structural (wit_gitea_http
builds https:///api/v1 under --proto '=https', and config.gitea.host must be a bare
hostname), and that relaxing the bare-hostname rule to make the suite runnable is not
acceptable — it would trade an egress control for a green check.

The adapter-track scope decision, recorded. Three seam follow-ons each carried a
live-conformance criterion; the maintainer's call — "I don't think we need it, we just need
linear"
— descopes the Gitea half. The new section states per item what was met and what was
not, and names the one thing genuinely lost: #2950's end-to-end proof that the generator emits an
adapter working against a real provider. Linear cannot substitute, being hand-built. The
generated adapter does pass its full mocked-transport suite, so what is missing is "verified
against a live server", not "unverified". Gitea stays shipped and supported; only its live
validation is descoped.

Upstream SSOT reconciliation (7a66d0d1, 9ae48ea3, 8ecc6eca). Nine verified inaccuracies
corrected across both SSOTs. Candidate-verdict coverage in mattpocock-skills.md went from 2 of
23 to 23 of 23, attached to the attribution row that already owns each upstream artifact rather
than duplicated as a second index. Lane X, the one candidate group with no per-candidate section
anywhere, gained one; its verdict-table row was corrected from a flat ADOPTED to PARTIAL,
which is how every other lane holding an already-present candidate is graded. Six stale
Relation grades in the v1.2 map were regraded — each verified against the landing site in this
repo first — and five citations missing their plugins/ prefix were fixed.

Verification

  • scripts/check-changelog-parity.sh --check and --check-bump origin/main → both clean
  • markdownlint-cli2 over every changed document → 0 issues; typos clean over docs/upstream
  • All 19 adapter and generator suites pass locally
  • Every adapter fix has a discriminating test: each was run against the unfixed file and
    confirmed to fail there
  • The reachability claim was disproven empirically, not argued: curl against the GitHub release
    asset returned 206 with an ELF 64-bit LSB executable, and the binary reports
    Gitea version 1.22.6 … bindata, sqlite, sqlite_unlock_notify
  • Two independent reviewers caught the || AFTER2='[]' fail-open in my own fix; it is closed in
    3ca356c7
  • 48106bd9 merges main. Note for anyone who hits this pattern: ci had silently stopped
    dispatching on new heads, and the cause was a merge conflict, not an Actions queue delay —
    pull_request workflows build against refs/pull/N/merge, which a conflicted PR has no way to
    produce, so the checks that need it never ran while the ones that do not (pr-issue-linkage,
    do-not-merge, GitGuardian) went green. A blocked PR with a partial check set is the tell.
    Resolving the conflict took the run from 3 checks to 47. The two conflicts were both review
    version metadata; my close-out fix renumbered 0.24.1 → 0.25.1 over main's new downstream mode.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm

claude added 2 commits August 21, 2026 01:43
… conformance pass

The adapter README said "no such instance is reachable from the environment this
adapter was built in." That was an untested assumption written down as fact, and
it is wrong. Gitea ships as a single self-contained binary with sqlite built in,
its releases are fetchable from the build environment, and a real one was
downloaded and version-verified there.

The actual blocker is narrower: serving it needs privileged setup — a dedicated
unprivileged user plus cap_net_bind_service, since Gitea declines to run as root
— which the sandbox permission policy gates. Reachability was never the
constraint, and leaving the wrong reason in a shipped doc sends the next person
looking in the wrong place.

The note now also records why 443 and TLS are structural rather than preferences
(wit_gitea_http builds https://<host>/api/v1 under --proto '=https', and
config.gitea.host must be a bare hostname, so no high port is expressible), and
states explicitly that relaxing the bare-hostname rule is NOT an acceptable way
to unblock the run: that rule stops a PR-modifiable binding from smuggling URL
structure and redirecting the credential off the intended tenant. Writing that
down keeps a later session from trading the control for a green check.

No behaviour change; docs and version only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
…he blocker reason

Three seam follow-ons (#2950, #2952, #2946) each carry an acceptance criterion
requiring a LIVE conformance pass. The maintainer's scope call — "I don't think
we need it, we just need linear" — descopes the Gitea half.

Records, per item, exactly what was met and what was not, rather than rounding
either up. Names the one thing genuinely lost: #2950's end-to-end proof that the
generator emits an adapter working against a real provider. Gitea was the
designated vehicle for that and Linear cannot substitute, because Linear was
hand-built and so proves nothing about the generator. States the honest size of
the gap too — the generated Gitea adapter passes its full mocked-transport
suite, so what is missing is "verified against a live server", not "unverified".

Also corrects a blocker reason this effort recorded more than once: that no
tracker instance was reachable from the build environment. Untested assumption,
stated as fact, and false — Gitea ships a self-contained binary with sqlite, its
releases are fetchable here, and a real one was downloaded and version-verified.
The actual blocker is privileged setup the sandbox gates.

Adds a durable warning against the wrong future fix: do not relax the adapter's
bare-hostname rule to make the suite runnable. 443 and TLS are structural, and
that rule stops a PR-modifiable binding smuggling URL structure to redirect the
credential off the intended tenant.

Gitea stays shipped and supported — it is the free, self-hostable option serving
the no-paid-tool constraint this effort set. It is simply not validated live.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 45s —— View job


Code review complete

Todo list

  • Gather context (PR diff, files changed)
  • Understand request: invoke /review:code-review per custom instructions
  • Run code review over the PR diff
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with any cross-file findings

Note: the /review:code-review skill invocation errored out (<error>Execute skill: review:code-review</error>) each time it was called, so this review was performed manually against the same high-signal correctness/maintainability scope that lane targets.

Summary

This is a documentation-only change (no behavior change, confirmed by git diff --stat). I verified the two load-bearing technical claims the new prose makes against the actual adapter source, and they both check out:

  • wit_gitea_http builds https://<host>/api/v1 under --proto '=https' — confirmed at common.sh#L313-L316.
  • config.gitea.host must be a bare hostname (no port) — confirmed by WIT_GITEA_HOSTNAME_RE at common.sh#L85, which rejects host:port forms.

I also checked the reflow hazard the PR description says it fixed (#2946 landing at the start of a line, parsed as an H1) — confirmed clean; no line in the changed files starts with # followed by a digit. Version bump (0.39.20.39.3) is consistent across plugin.json and CHANGELOG.md, and I found no stale references to the old version elsewhere in the repo. Issue cross-references (#2950, #2952, #2946) are consistent with prior CHANGELOG entries and other doc mentions.

One finding, posted inline: a minor formatting nit in docs/upstream/aihero-shipping-course.md (line 391) — a paragraph wrap leaves the word "It" orphaned on its own short line, inconsistent with the ~90-char fill used throughout the rest of the section. Cosmetic only (renders fine, doesn't affect markdownlint), likely a leftover from the line-reflow fix described in the PR's verification notes.

No correctness issues, no maintainability concerns beyond the nit above. The claims in the corrected prose (README, CHANGELOG, SSOT doc) are internally consistent with each other and with the actual adapter code.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 55s —— View job


Security review — no issues found

  • Gather context (PR diff, changed files)
  • Apply skip gate
  • Review changed files for security-relevant surface
  • Post findings

Skip gate: change has no security-relevant surface. All four changed files are documentation and version metadata only:

  • docs/upstream/aihero-shipping-course.md — new "Adapter-track scope decision" section (prose)
  • plugins/work-items/.claude-plugin/plugin.json — version bump 0.39.20.39.3
  • plugins/work-items/CHANGELOG.md — changelog entry (prose)
  • plugins/work-items/tools/work-item-tracker/adapters/gitea/README.md — README prose correcting a blocker reason

No code, scripts, workflows, config, or permission logic is touched (git diff --stat confirms one README, one CHANGELOG, one SSOT doc, one manifest version line). The README's security-relevant claim — that the gitea adapter's bare-hostname rule (config.gitea.host must be a bare hostname; no high port expressible) must not be relaxed to unblock live-conformance testing — is a documentation note reinforcing an existing guard, not a code change to it. I did not find that guard's enforcement code altered anywhere in this diff.

No security findings to report.

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Last security-reviewed head: 48106bd9d015a5462db173860ea65e70144d354e. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b4c2d72a9

ℹ️ 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".

Comment thread docs/upstream/aihero-shipping-course.md Outdated
Comment thread docs/upstream/aihero-shipping-course.md Outdated
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

…ceholder failing silently

A reviewer flagged a claim I had just written into the SSOT — that the suite
closes what it creates. It does not, and checking exposed a real gap.

run-conformance.sh contains no close or delete logic at all. Cleanup is entirely
the binding's _cb_clean_at_start, and only two bindings ever implemented one:
github (closing every open issue through gh) and local-markdown (a fresh temp
dir per run). jira, gitea and linear shipped it as an unfilled `:` placeholder,
so a live run would create, claim and mutate real issues and leave every one of
them in the target — with the suite's own count assertions then running against
the previous run's leftovers.

linear now implements it for real: archiving every issue in the throwaway team
through Linear's own GraphQL API, not through the seam under test, because using
the seam to prepare its own fixture would let a broken adapter hide its
breakage. It archives rather than deletes so a mis-pointed scope stays
recoverable, passes the credential through curl's stdin config so it never
reaches argv, and FAILS LOUDLY — a cleanup that quietly does nothing is worse
than none, since the suite then flaps for reasons nobody can see.

gitea and the generator template still carry the placeholder but now announce it
on stderr every run, so future generated adapters inherit the warning instead of
the silence.

Five regression cases, verified discriminating: reverting to the no-op turns
three red, swallowing the GraphQL error turns the fourth red. One asserts the
team key is split out of <workspace>/<TEAMKEY> and the qualified form never sent
as the key — sending the whole scope would match nothing and "clean" an empty
set, which looks exactly like success.

Writing it, I reintroduced by hand the exact defect the generator's display_name
guard exists for: an apostrophe inside ${VAR:?word}, which bash parses as a
quote. ShellCheck caught it; the message is now apostrophe-free with a note.

Also records that the adapter's 13 GraphQL documents were validated against
Linear's real published schema (SDK v90 SDL, cross-checked against a separately
fetched master copy and Linear's own generated documents): 13/13 valid, no
unknown field or argument, String!/Float! both correct where the plausible
mistakes would have been ID!/Int!, every jq-built input-object field real, and
relation direction confirmed. That closes offline the class of failure Linear
would reject regardless of workspace or credential.

59/59 test files pass; shellcheck clean; portability and silent-skip gates clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 6s —— View job


Security review — no issues found

  • Gather context (PR diff, changed files)
  • Invoke /review:security-review
  • Review changed files for security-relevant surface
  • Post findings

Note: /review:security-review errored out again on invocation (<error>Execute skill: review:security-review</error>), same as the prior automated pass on this PR — this review was performed manually. The Bash tool also required interactive approval for shellcheck/test execution in this run, so I could not independently re-run the shell linter or the new test file; I verified correctness by reading, not by executing.

What's new since the last review

The last automated pass on this PR (head 0b4c2d7) covered docs-only changes and found no security-relevant surface. This synchronize added a real code commit responding to the Codex reviewer's finding that the Linear conformance binding's cleanup was a silent no-op — that's genuinely security-relevant surface (credential handling, outbound HTTP), so I reviewed it in full:

Credential handling checks out:

  • The API key is fed to curl through -K - (stdin config), never argv — confirmed at L45-L53. This mirrors the identical, previously-reviewed pattern in the gitea adapter's wit_gitea_auth_header/wit_gitea_http (adapters/gitea/common.sh#L294-L318).
  • --proto '=https' forces TLS; no -L, so no redirect can cause the header to be replayed to an unintended host.
  • GraphQL query/variables are built with jq -cn --arg/--argjson, not string interpolation — no injection path from team key or issue ID into the query.
  • The written binding file (CB_BINDING_TMP) stores only auth_env: "WIT_LINEAR_API_KEY" (the env var name), never the credential value — consistent with the "never stored in the tracked binding" rule documented for the gitea adapter.
  • Errors (list failure, GraphQL errors[], archive failure) all return 1 with a message to stderr rather than swallowing and continuing — avoids running the mutating suite against an unknown starting state.

I didn't find anything to flag. The docs/version-metadata portions of the diff (docs/upstream/aihero-shipping-course.md, CHANGELOG.md, plugin.json → 0.39.4) are prose/version-only and carry no security surface. This commit is a genuine fix, not a regression: it closes the gap the Codex bot flagged (generated Linear items would previously have been left behind indefinitely with no cleanup).
· Branch

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 4m 8s —— View job


Code review complete

  • Gather context (PR diff, files changed)
  • Invoke /review:code-review and follow its instructions
  • Run code review over the PR diff
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with any cross-file findings

Note: the /review:code-review skill invocation errored out (<error>Execute skill: review:code-review</error>) again on this run, so the review below was performed manually against the same high-signal correctness/maintainability scope that lane targets, using git diff against the true merge base (8934b9b, found locally since origin/main in this shallow checkout has no common ancestor with the PR branch).

Summary

This PR has grown since the last review pass — HEAD is now 0498519, which adds a third commit ("make conformance actually clean up, and stop the placeholder failing silently") on top of the two docs-only commits reviewed earlier. That third commit is a real (small) code change, and it directly resolves the two open findings from the prior review round:

  • Codex's P2 ("Do not claim conformance closes generated items") — fixed. docs/upstream/aihero-shipping-course.md now has a dedicated paragraph (lines 395–413) explicitly retracting the earlier false "the suite closes what it creates" claim, stating plainly that run-conformance.sh has no close/delete logic and that only github and local-markdown ever implemented _cb_clean_at_start. jira/gitea/linear shipped it as an unfilled : no-op — matches what I verified directly against run-conformance.sh and the binding files.
  • My own nit (orphaned "It" on aihero-shipping-course.md line 391) — fixed; the paragraph now reflows normally.

New in this commit, reviewed against the adapter's own conventions in adapters/linear/common.sh:

  • conformance/bindings/linear.sh#L37-L83 implements _cb_clean_at_start for real — pages the throwaway team's issues via Linear's own GraphQL API and archives each one. It correctly mirrors the adapter's security discipline: bare Authorization header (no scheme word) fed through curl's -K - stdin config so the credential never hits argv, and --proto '=https'. Team key is correctly split from <workspace>/<TEAMKEY> via ${VAR##*/} (verified by a dedicated regression case in linear.test.sh asserting the split form is sent and the qualified form is not).
  • Failure handling is loud by design — GraphQL errors and list/archive failures return 1 rather than continuing, consistent with the stated goal of not letting a broken cleanup pass silently. One thing I looked at but don't think rises to a reportable finding: _cb_gql doesn't inspect the HTTP status code the way wit_linear_http does (common.sh#L341-L358), relying instead on the GraphQL .errors field and jq's own parse failure to fail the pipeline for non-GraphQL-shaped error bodies (e.g. a plain-text 401). It still fails closed in that case (jq emits nothing → the != "[]" check trips), just with a less specific stderr message — and the function's own header comment already discloses "NOT YET EXERCISED AGAINST A LIVE WORKSPACE", so this is a disclosed rough edge, not a hidden defect.
  • gitea.sh and the generator template (conformance-binding.sh.tmpl#L26-L36) keep the placeholder but now emit a stderr warning naming the scope on every run instead of silently no-op'ing — I checked generate-adapter.test.sh and confirmed the @@PROVIDER@@ substitution and the apostrophe-free ${VAR:?...} guard convention (an actual prior regression this repo tests for) are both respected in the new printf line.
  • linear.test.sh adds four new regression cases (well-formed binding write, mutation actually sent, team-key split, provider-error fails loudly) with a stubbed curl on PATH — no network touched, consistent with the file's own "does NOT run the suite" framing.

Docs/version metadata: plugin.json is now 0.39.4, matching the new ## [0.39.4] CHANGELOG entry (the PR description's "0.39.2 → 0.39.3" is now stale relative to HEAD, which is expected — the description predates this last commit). I traced the CHANGELOG's "closes what it creates" retraction back to where that wrong wording was actually introduced (0b4c2d7), confirming the correction targets a real prior claim rather than a strawman.

I don't have a working shell in this run for shellcheck/markdownlint/the changelog-parity script (non-git Bash invocations require interactive approval that isn't available here), so I did this by direct reading of the diff and cross-referencing against the adapter's existing conventions rather than by running the repo's own verification scripts. Nothing in that manual pass surfaced a correctness or maintainability issue worth an inline comment — the commit is small, well-tested, and the security-relevant patterns (credential-via-stdin, --proto '=https', archive-not-delete) match what's already established elsewhere in the codebase.

No findings to post.

Branch

claude added 3 commits August 21, 2026 05:41
…se, and unpin conformance from one adapter's prose

Two findings from an independent audit, both reproduced by execution before
being fixed.

CRITICAL — double claim over a live lease. wit_linear_lease_comments passed the
marker's lease_comment_id straight to `jq --argjson`. A non-numeric value makes
jq exit 2 printing nothing, so the accumulator became the empty string; every
later iteration failed the same way on `--argjson acc ""`; and the trailing
`sort_by` over empty input printed nothing AND EXITED 0. The helper therefore
returned success-with-no-output, which every caller reads as "nothing is
claimed". The `|| exit "$?"` guards added at six call sites earlier in this
effort cannot catch it, because the failure never arrives as a non-zero status.

Reproduced: with alice holding a live lease whose marker carried
`lease_comment_id: "abc"`, bob's claim returned rc=0 and a full success record.
Markers are consumer-writable in practice — hand-edited, or written by another
tool speaking the same v1 shape — so this is reachable input.

Fixed in two layers, both load-bearing and verified discriminating:
a numeric check routes a malformed handle into the SAME createdAt fallback a
missing one already took, keeping the foreign lease visible so the claim is
correctly refused with exit 7; and the accumulate now fails loudly, so no future
cause can degrade it back to an empty set that reads as "no leases". With only
the second guard the claim exits 1 rather than 7 — still safe, still no double
claim, but the wrong answer, which is why both stay.

MEDIUM — the conformance suite asserted exact equality on `reason`, a field
CONTRACT.md's output table gives no vocabulary. It hardcoded github's "lease
live", while linear says "lease is still live", so the live linear pass this
effort is still blocked on was pre-wired to fail on prose. It now asserts the
semantic fact the surrounding comment actually cares about — the ACTIVE lease
was selected, not the superseded newer one. Checked against all three real
strings: both live-lease wordings pass, the superseded wording still fails.

58/58 test files pass; shellcheck and portability gates clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
…porting them as this adapter's own error

Three sites captured wit_gitea_blocked_by_count in a command substitution with
no exit propagation — the same defect fixed in the linear adapter earlier in
this effort, missed here. That helper exits 4/5/7/8/1 by HTTP status, but an
`exit` inside $( ) ends only the subshell; with `set -uo pipefail` and no `-e`
the caller continued with BBC="".

Measured, with only the /dependencies request failing:

  verb         dep    contract   before   after
  get-item     401    4          1        4
  get-item     503    8          1        8
  list-items   401    4          1        4
  list-items   503    8          1        8
  create-item  401    4          2        4
  create-item  503    8          2        8

create-item was the damaging one: POST /issues had ALREADY SUCCEEDED, the issue
existed on the server, and the caller was told exit 2 — "usage (bad args…)" per
CONTRACT.md. A caller that "fixes" its arguments and retries files a duplicate.
It also leaked raw `jq --help` text to stderr.

The 8→1 collapse mattered too: work-loop routes on "seam exit 8 → backoff and
retry", so a rate-limited or 5xx dependencies call never triggered backoff.

create-item propagates the read's own code rather than substituting a count: the
contract's item object requires blocked_by_count and 0 is exactly the frontier
lie this adapter pays a request per item to avoid. But non-zero here must not
read as "nothing happened", so it now names the created id on stderr and says
re-read it rather than create it again — the guard against duplicate filing,
since 8 is precisely what work-loop retries on.

10 discriminating assertions across the three suites, each verified to go red
with its fix removed and green with it restored; 4 further supporting assertions
are flagged in the tests as passing either way. No existing case covered "issue
fetch succeeds, dependencies request fails" — the prior status-mapping cases all
failed the issue fetch first and exited before reaching the bug.

Whole gitea suite green; shellcheck and portability clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
…in winner's assignment

Both unassign guards compared the current assignee against HOLDER, which is
WIT_LINEAR_VIEWER — the authenticated user's DISPLAY NAME, not a session
identity. A name compare therefore cannot tell our own write from another
session of the same login, and same-login racing is not exotic here: it is
precisely what the lease-comment/handle arbitration exists for, and the
conformance suite exercises it as "Second session, same identity".

Consequence: sessions A and B both authenticated as the same user race; B loses
arbitration, re-fetches, sees its own login in the assignee slot — which is
actually A's winning write, since Linear's assignee is a single field — concludes
the assignment is its own, and clears it. lib/frontier.sh selects purely on
`assignees | length == 0` and never consults leases, so A's actively-worked item
returns to the frontier while A holds a live lease.

Both sites now require BOTH conditions before clearing: no OTHER live lease
exists, and the assignee still matches our login. Each guard alone lets a
different assignment through — the live-lease test alone would clear a foreign
assignee whose lease had lapsed, and the name test alone is the bug above — so
the conjunction is strictly safer than either. Our own lease is excluded from
the live-lease test, or the rollback would skip its own cleanup; COMMENT_UUID is
pre-initialised so the trap stays safe to fire before the lease is posted.

The rationale is recorded at both sites: a stale NAME in the assignee slot is
cosmetic, because the item stays correctly excluded from the frontier either
way; an emptied slot is a double-work invitation.

An existing case asserted "exactly one unassign" on the lost-race path. Under
the corrected semantics the right answer there is zero — the winner holds a live
lease and their assignment must survive — so that expectation is updated with
its reasoning, and a new same-login case covers the race the name compare could
not see. Both verified discriminating: reverting to the name-only compare turns
them red.

57/58 test files pass (the one failure is an unrelated file another change is
mid-edit on); shellcheck and portability clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 50s —— View job


Code review complete

  • Gather context (PR diff, files changed)
  • Invoke /review:code-review and follow its instructions
  • Run code review over the PR diff
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with any cross-file findings

Note: /review:code-review errored out again on invocation (<error>Execute skill: review:code-review</error>), consistent with every prior automated pass on this PR, so this review was performed manually against the same high-signal correctness/maintainability scope that lane targets. Non-git Bash invocations (shellcheck, test execution) required interactive approval not available in this run, so verification was by direct reading and cross-referencing against the mocks/fixtures rather than by executing the suites.

What's new since the last review

HEAD is now 302569b, three commits ahead of the last fully-reviewed head (0498519):

  • e93e071 — a non-numeric lease_comment_id in wit_linear_lease_comments now routes into the same createdAt fallback a missing handle already took, and the accumulate jq call now fails loudly instead of silently degrading to an empty (falsely "no leases") result on any future breakage. Traced the failure mode by hand: --argjson h on a non-JSON string does exit 2 with no stdout, which previously made all="" and cascaded through every later iteration and the trailing sort_by — a real success-with-no-output bug, correctly fixed. Also unpins run-conformance.sh's reclaim-reason assertion from github's exact wording ("lease live") to assert_contains .. "live" / assert_not_contains .. "superseded", unblocking a live run against any adapter that words the reason differently (confirmed both assert_contains/assert_not_contains exist and are used with matching argument order elsewhere in the file).
  • 026ed64 — three gitea call sites (create-item.sh, get-item.sh, list-items.sh) captured wit_gitea_blocked_by_count in a bare $( ) with no || propagation; exit inside a command substitution only ends the subshell, so a failing dependencies read fell through as BBC="" and got reported as this adapter's own usage/internal error (exit 2/1) instead of the helper's real status (4/5/7/8). Verified get-item.sh/list-items.sh now use || exit "$?" from a non-subshell context (the list-items.sh loop reads via < <(...) process substitution, not a pipe, so exit there does terminate the script). create-item.sh correctly propagates the read's own code rather than substituting a count (0 would be the exact frontier lie the adapter pays a request to avoid), and names the already-created item on stderr so a caller doesn't retry into a duplicate. Cross-checked wit_gitea_require_ok's status→exit-code mapping (common.sh#L334-L364) against the commit message's table — matches.
  • 302569b — both unassign guards in claim.sh (the EXIT-trap rollback and the lost-arbitration branch) compared the re-fetched assignee against HOLDER (WIT_LINEAR_VIEWER, a display name), which can't distinguish "this session's own write" from "another session logged in as the same user" — exactly the case the lease-handle arbitration exists to resolve. Both sites now require a conjunction: no other live lease exists, and the assignee name matches. Traced why the conjunction (not either guard alone) is correct: live-lease-only would wrongly preserve a foreign assignee whose lease had actually expired; name-only is the original bug. Confirmed COMMENT_UUID="" is pre-initialized before the trap is armed (so the trap's exclusion of "our own" lease comment is set -u-safe if it fires before step 3 sets it), and that the trap body stays inside the pre-existing ( ... ) >/dev/null 2>&1 || true subshell so an exit 0 inside the new lease-scan loop only exits the subshell, not the script.

Test coverage check

Read the updated test files against the mock harnesses rather than executing them:

  • claim.test.sh's two new/updated race cases (losing the race still exits 7 / leaving the live winner's assignment untouched, and the same-login variant) are internally consistent with mock.sh's route-sequencing semantics (comments( pattern: 1st call → pre-check response, 2nd → arbitration re-read, 3rd+ → repeats the last route) — the loser-branch's new AFTER2 read correctly falls through to the rival's still-live lease and leaves the assignee alone.
  • The existing rollback-trap test (concurrent winner via a differently-seeded issues(filter: route) still passes under the new logic: the trap's live-lease scan finds nothing (mock's static comments( route returns []), falls through to the name-compare guard, which correctly declines to clear (assignee is other-sessionkyle).
  • The three new gitea read-back tests (create-item.test.sh, get-item.test.sh, list-items.test.sh) correctly seed a healthy issue/list response and fail only the /dependencies request — the one shape the prior code never covered (the old status-mapping cases all failed the issue fetch first).

No correctness or maintainability issues found in this synchronize. All three commits are small, each fixes a real, concretely-reproduced defect (not a hypothetical), pair 1:1 with new discriminating test coverage, and match this codebase's established conventions (credential/status propagation via || exit "$?", loud failure over silent degradation, TTL/lease-liveness as the identity-independent test). The docs/version-metadata portion of the diff (docs/upstream/aihero-shipping-course.md, CHANGELOG.md, plugin.json) was already reviewed in a prior pass and is unaffected by this synchronize.

Branch

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 11s —— View job


Security review — no issues found

  • Gather context (PR diff, files changed)
  • Invoke /review:security-review
  • Review changed files for security-relevant surface
  • Post findings

Note: /review:security-review errored out on invocation again (<error>Execute skill: review:security-review</error>), consistent with every prior automated pass on this PR — this review was performed manually against the same scope.

What's new since the last review

The last automated pass (head 0498519) covered the conformance-cleanup commit. This synchronize adds three more commits, all real code changes (not docs):

  • e93e071 — stop a malformed lease handle from hiding a live lease
  • 026ed64 — propagate gitea blocker-count failures instead of reporting them as this adapter's own error
  • 302569b — stop a losing linear claim from stripping a same-login winner's assignment

These touch assignment/lease-integrity logic and credential-adjacent read paths, so I reviewed them in full against git diff 0498519..302569b:

adapters/linear/claim.sh + common.sh (common.sh#L758-L784, claim.sh#L113-L225) — this is the security-relevant core of the diff, since it governs who holds a work-item's assignment:

  • Fixes a real integrity gap: a non-numeric lease_comment_id previously made jq --argjson fail inside a loop, silently emptying the lease accumulator; jq over empty input exits 0, so the helper returned success-with-no-output, which every caller reads as "nothing is claimed." A live foreign lease could thus become invisible and a second claim would be handed out over it. The fix validates the handle is numeric (^[0-9]+$) before use and now fails loudly (EX_INTERNAL, non-empty stderr) on any other jq accumulation error instead of degrading to an empty set — verified against the new claim.test.sh case asserting a malformed handle exits 7 (refused) rather than 0, and that no issueUpdate mutation is issued.
  • Fixes a race where HOLDER (the authenticated user's display name, not a session identity) was compared directly against the current assignee to decide whether to clear it. Two sessions authenticated as the same login are indistinguishable under a name compare, so a losing session could see its own login in the assignee slot and wrongly clear a live winner's claim. The fix adds a live-lease check (excluding the claimer's own COMMENT_UUID) as a precondition to clearing, in both the rollback trap and the lost-race path — confirmed the new test case ("a same-login rival still wins the race") exercises exactly this and asserts zero assigneeId:null writes.
  • The COMMENT_UUID="" pre-initialization is a correctness guard (so the trap is set -u-safe if it fires before step 3), not a widening of what the trap will accept.

adapters/gitea/{create,get,list}-item.sh (create-item.sh#L170-L191) — wit_gitea_blocked_by_count failures were previously swallowed by $( )'s subshell boundary, feeding jq an empty --argjson and surfacing a 401/503 as this adapter's own internal/usage error (exit 2). Now the caller's own exit code (4 for auth, 8 for backoff-eligible unavailability) propagates correctly, and create-item.sh additionally names the already-created item on stderr so a caller doesn't retry into a duplicate. This is a correctness/reliability fix; the only security-adjacent property is that auth-failure signaling (exit 4) now reaches the caller intact instead of being masked, which is strictly better for a caller that gates retries on status. Verified against the new test cases in all three *.test.sh files, including the assertion that no raw jq --help usage text leaks to stderr.

conformance/run-conformance.sh — the reason string assertion changed from exact-match to substring (assert_contains .. "live" / assert_not_contains .. "superseded"), which is a test-portability fix (linear's reason text differs from github's), not a behavior or security change.

Credential handling, auth headers, and TLS/redirect posture are unchanged in this diff — none of these four commits touch wit_gitea_http/wit_linear_http/wit_linear_auth_header or the bare-hostname validation covered in earlier review passes.

I don't have a working non-git shell in this run (interactive approval required for shellcheck/test execution wasn't available), so this was a manual read of the diff and cross-reference against the adapters' existing conventions rather than an executed test run.

No findings to report.
· Branch

…in the adapter generator

sample_scope was validated only against api.scope_pattern — a pattern THE SPEC
ITSELF SUPPLIES — and quote_safe rejects single quotes only. But the value also
lands in a DOUBLE-quoted argument in templates/common.test.sh.tmpl, where $(…)
still expands and a " still breaks out.

Reproduced: a spec with scope_pattern "^.*$" (which passes the both-ends-anchored
check) and sample_scope containing `"; $(id > …); echo "x` generated cleanly, and
running the generated common.test.sh — step 1 of the generator's OWN printed
"Next:" instructions — executed the command as root while the suite still
reported PASS.

The generator had already closed exactly this hole for display_name, with a
comment naming "a double-quoted expansion where $(…) would execute". sample_scope
simply never got the same treatment, while every other spec value reaching
generated shell (PROVIDER, DISPLAY_NAME, BASE_PATH, HOST_SUFFIX, SAMPLE_HOST,
SAMPLE_ENV, SAMPLE_ID) is anchored-charset validated.

Fixed at validation, where its neighbours already are: an anchored charset
covering every legitimate scope shape across the bundled providers — owner/repo
for github and gitea, <workspace>/<TEAMKEY> for linear, jira project keys — while
excluding quotes, $, backticks, backslash, semicolons and newlines. The
spec-pattern match is kept alongside it; the two serve different purposes.

Honest framing, recorded at the site: the spec comes from the skill's own
interview, so this is a robustness and supply-chain gap rather than a remote
attacker path. It still matters, because the generator's stated posture is that
spec values are dangerous and nothing downstream escapes them — it was
inconsistent with itself.

The spec reference doc is updated to document the constraint, matching how the
neighbouring fields document theirs.

New cases cover a double quote, a command substitution, and a newline (each exit
3), plus legitimate scopes still generating so the guard is not over-tight. All
verified discriminating. The ShellCheck --rcfile support probe is untouched: CI's
skill-quality-gate runner carries a pre-0.10.0 build that exits 3 on that flag.

140 generator cases pass; shellcheck and markdownlint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 24s —— View job


Security review — one finding

  • Gather context (PR diff, changed files)
  • Invoke /review:security-review (skill invocation errored, ran the same criteria manually)
  • Review changed files for security-relevant surface
  • Post findings

Note: /review:security-review errored on invocation again (<error>Execute skill: review:security-review</error>). I read plugins/review/skills/security-review/SKILL.md directly and followed its stated criteria (logic/authorization/injection/credential-handling hunt, security-only scope, severity-tagged, adversarial-verified before reporting).

What's new since the last review

HEAD is now d52ea34 — four more real commits landed since the last pass (which covered up to 0498519):

  • e93e071 — fixes a CRITICAL double-claim bug (a non-numeric lease_comment_id made jq --argjson fail silently, collapsing the lease list to empty and reporting a live foreign lease as claimable) plus a conformance string-match fix.
  • 026ed64 — propagates wit_gitea_blocked_by_count failures instead of swallowing them via unchecked $( ).
  • 302569b — fixes a same-login race where a losing Linear claim could strip a live winner's assignment.
  • d52ea34 — closes a command-injection hole: api.sample_scope reached a double-quoted template context unescaped, letting a crafted spec run arbitrary shell when the generated adapter's own test suite was executed.

I verified each of these against the actual source (not just the commit messages) and they're sound, well-targeted fixes with discriminating test coverage. d52ea34 in particular is a genuine fix: templates/common.test.sh.tmpl:72 does put @@SAMPLE_SCOPE@@ inside a double-quoted arg where $(…) expands, the new anchored charset (^[A-Za-z0-9][A-Za-z0-9._~/-]*$) excludes every shell metacharacter, and the validation sweep was correctly hoisted to top level so exit isn't swallowed by a $( ) subshell.

Finding — IMPORTANT: same fix reintroduces its own failure mode on a fetch error

adapters/linear/claim.sh#L212

AFTER2="$(wit_linear_lease_comments "$ISSUE_UUID")" || AFTER2='[]'

302569b's whole point is that the loser branch must not unassign unless it has positive proof no other live lease exists (LOSER_LIVE populated by walking AFTER2). But if the re-fetch itself fails (transient API error — exactly the failure mode wit_linear_lease_comments is designed to signal via non-zero exit), this line silently substitutes an empty list. LOSER_LIVE then stays unset, and if the assignee name still matches HOLDER (the ordinary case for a losing claim — see the same-login scenario this very commit is fixing), the code proceeds to wit_linear_set_assignee "$ISSUE_UUID" "", stripping the winner's live assignment — the exact bug this commit exists to close, reappearing on a fetch failure instead of a name collision.

Contrast with the rollback trap 90 lines above it (claim.sh#L127), which fails safe on the identical fetch: _rb="$(wit_linear_lease_comments "$ISSUE_UUID")" || exit 0 — treats "can't tell" as "don't touch it." The AFTER2 site treats the same uncertainty as "assume clear," which is the unsafe direction. e93e071, landed in this same PR, establishes the project's own precedent for this exact class of bug: make the accumulate step fail loudly rather than let a jq failure degrade into an empty set that reads as "no leases." This one call site does the opposite on purpose.

Not covered by the new tests in 302569b (which exercise the name-match and same-login paths, not a fetch failure at this second wit_linear_lease_comments call).

Suggested fix: on failure, treat it the same as the rollback trap — skip the unassign rather than defaulting to "no live lease," e.g. AFTER2="$(wit_linear_lease_comments "$ISSUE_UUID")" || { printf '...' >&2; AFTER2=; } guarding the loop, or simply wit_linear_lease_comments "$ISSUE_UUID" > /tmp/x || return-style skip of the unassign block on failure. Fix this →

No other findings. Credential handling, injection surfaces, and the other three commits' logic all check out against the security-review criteria (authorization/access-control, injection, credential/token handling — no GitHub Actions files touched in this diff).

Branch

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 58s —— View job


Code review complete

  • Gather context (PR diff, files changed)
  • Invoke /review:code-review and follow its instructions
  • Run code review over the PR diff
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with any cross-file findings

Note: the /review:code-review skill invocation errored out again (<error>Execute skill: review:code-review</error>), consistent with every prior automated pass on this PR — this review was performed manually against the same high-signal correctness/maintainability scope that lane targets, diffing against the true merge base (8934b9b, resolved locally since this shallow checkout's origin/main has no common ancestor with the PR branch).

What's new since the last review

HEAD is now d52ea34, four commits ahead of the last reviewed point (0498519):

  • e93e071 — fixes a malformed lease_comment_id handle silently emptying the lease accumulator (a jq --argjson failure that used to return success-with-no-output, hiding a live foreign lease), and unpins a conformance assertion from one adapter's exact wording.
  • 026ed64 — fixes three gitea call sites where wit_gitea_blocked_by_count's exit code was swallowed by $( ), one of which (create-item.sh) reported a successfully-created issue as exit 2 "usage" — inviting a caller to "fix its args" and file a duplicate.
  • 302569b — requires both a live-lease check and an assignee-name check before an unassign, closing a same-login race where a display-name-only compare let a losing session strip a winning session's assignment.
  • d52ea34 — closes a command-injection hole where api.sample_scope reached a double-quoted argument in generated shell unescaped, and moves the quote_safe sweep to top level so a refusal actually aborts the generator instead of dying inside a $( ) subshell while the script reports success.

All four are well-reasoned, each reproduces its bug before fixing it, and each ships discriminating regression tests (verified red-without-the-fix in the commit messages). One issue found in the newest commit:

One finding, posted inline on adapters/linear/claim.sh:212: commit 302569b's new re-read (AFTER2="$(wit_linear_lease_comments "$ISSUE_UUID")" || AFTER2='[]') defaults to "no live leases" when the re-read itself fails, which can strip a winning session's live assignment on a transient read failure — the same class of bug this commit's own message describes fixing, reintroduced via the error path. The sibling guard in _wit_linear_claim_rollback two commits' worth of context above it (line 142) fails safe on the identical read failure (|| exit 0, no unassign); this one doesn't. No test exercises this path either way.

Cross-file: undocumented fixes since the last CHANGELOG entry

CHANGELOG.md and plugin.json are both still at 0.39.4, and the ## [0.39.4] entry only documents the conformance-cleanup work from 0498519. The four commits above — including a CRITICAL correctness fix (double-claim over a live lease) and a security fix (command injection via api.sample_scope) — have no changelog entry at all. scripts/check-changelog-parity.sh --check-bump won't flag this (it reasons at PR granularity: fork point vs. head, not per-commit), so this won't fail CI, but it's a real gap against this repo's own visibly rigorous convention of documenting exactly this class of fix (see the detail level of the 0.39.4 entry itself), and it's a bit ironic on a PR whose stated purpose is "recording the reasoning durably rather than leaving it only in closed-issue comments."

Everything else

Verified the load-bearing claims from earlier commits still hold at HEAD: wit_gitea_http's --proto '=https' and WIT_GITEA_HOSTNAME_RE's bare-hostname enforcement are unchanged; the reflow-hazard fix and the earlier orphaned-"It" nit are both still resolved; EX_INTERNAL is defined and consistent with the new exit "$EX_INTERNAL" call site in common.sh. Docs/version metadata carry no new issues beyond the changelog gap above.

Branch

An independent audit re-checked every claim in the two upstream SSOT documents
against the actual repo state, with three fresh-context verifiers re-testing each
discrepancy and one candidate finding refuted and dropped. Nine survived.

aihero-shipping-course.md:
- Lane Y said the marketplace-wide glossary was deferred. docs/GLOSSARY.md landed
  2026-08-20 (#3062) and already defines `phase boundary`. Corrected to record the
  promotion, with `item` and `checkpoint` staying reference-local.
- Lane D said the container close-out review "is structurally larger than a mode
  addition". #3027 is closed: it landed as exactly that — a tenth quality-gate
  lens — with all four broken mechanisms resolved in-file.
- Lane D C12 claimed the branch-scoped spec lens filled the dangling consumer in
  decompose and ship. It did not: that consumer was container-scoped, work-items'
  own changelog says so, and #3027's close-out mode filled it. The quoted phrase
  is also gone from both skills, surviving only in changelog history.
- C23 cited `.agents/` trigger text. Verified against mattpocock/skills@068b6e0:
  `.agents/` holds five files and none carries a domain-modeling trigger. The
  real source is domain-modeling/SKILL.md:3, from PR #848 — so the Lane X row,
  which scoped all of C21-C23 to #878/#880, is corrected too.

mattpocock-skills.md:
- `to-tickets` was recorded as "Influence (vocabulary)", which understated it AND
  disagreed with the v12 map's own PARTIAL grade. Four of Lane B's five adopted
  candidates are mechanics, not phrasing; only "work the frontier" is vocabulary.

mattpocock-skills-v12-map.md — four rows contradicted by landed work:
- row 2 called the two-axis parallel-subagent structure "Omitted"; self.md:17 IS
  that structure, differing only in calling it a lens rather than an axis.
- row 13 called the tdd seam discipline "Not ported"; it landed at plan/SKILL.md:96
  with two deliberate divergences.
- row 14 called to-spec "Not ported"; C3 and C4 both landed in decompose.
- the invocation-reach rule was recorded as having "no such stated invariant in
  our repo"; it is a named section in the invocation-mode convention doc, and the
  sibling SSOT already recorded it CONFIRMED — the two docs contradicted.
- drift findings 1 and 2 cite questionnaire/SKILL.md:48-50. That file is 45 lines
  and greps clean; the text was removed in #2082 BEFORE the map was written. Both
  marked discharged rather than left reading present-tense.

Also fixes a divergence the audit surfaced as a side effect: `phase boundary` had
TWO definitions — docs/GLOSSARY.md's and execution-shape.md's — one of them in a
file claiming repo-wide authority. execution-shape.md now cedes the definition and
keeps only the seam-specific relation to `checkpoint`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
Comment thread plugins/work-items/tools/work-item-tracker/adapters/linear/claim.sh Outdated
Comment thread plugins/work-items/tools/work-item-tracker/adapters/linear/claim.sh Outdated
…mmit message undersold

Records 0.39.5: the linear lease-handle double claim, the same-login assignment
strip, the three gitea swallowed-exit sites, the conformance reason unpinning,
and the generator injection work.

That last entry corrects the record. Commit d52ea34's message described only
the api.sample_scope charset hole. While checking discrimination, neutering that
guard exposed a second and worse defect in the same file: quote_safe's refusal
was INERT, because every render() call is $(render …) and an exit inside a
command substitution kills only the subshell. A spec with a single-quoted
scope_pattern printed the refusal once per template, then wrote a directory of
empty executable scripts, reported "Wrote 9 file(s)", printed its own "Next: run
these" instructions, and exited 0. Both fixes landed in that commit; only one was
described.

That mattered specifically because SCOPE_PATTERN carries a regex and so cannot be
charset-bounded — quote_safe was its only guard, and it was doing nothing.

It is also the third distinct instance of the swallowed-exit-in-$( ) class found
in this seam, after the six linear sites in 0.39.1 and the three gitea sites
here. Naming the pattern in the changelog so the next reader looks for it.

58/58 test files pass; changelog parity, markdownlint, and portability clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 55s —— View job


Security review — no new issues

  • Gather context (PR diff, files changed)
  • Invoke /review:security-review (skill invocation errored again; read plugins/review/skills/security-review/SKILL.md directly and followed its stated criteria manually)
  • Review changed files for security-relevant surface
  • Post findings

Note: /review:security-review errored on invocation (<error>Execute skill: review:security-review</error>), consistent with every prior automated pass on this PR.

What's new since the last review

The last full security review covered up to head d52ea34. HEAD is now 5a1d580 — two more commits landed, both docs-only:

  • 7a66d0d — corrects nine inaccuracies across the two upstream SSOT docs (docs/upstream/aihero-shipping-course.md, docs/upstream/mattpocock-skills-v12-map.md, docs/upstream/mattpocock-skills.md) plus plugins/work-items/reference/execution-shape.md (cedes a duplicated phase boundary definition to the new repo-wide glossary).
  • 5a1d580 — adds a 0.39.5 CHANGELOG entry and bumps plugin.json, documenting the five fixes from the prior four commits (including the quote_safe inert-refusal defect that d52ea34's own commit message had undersold).

Skip gate applies to this increment: no security-relevant surface. git diff --stat d52ea34..5a1d580 touches six files — three SSOT markdown docs, one reference doc, CHANGELOG.md, and plugin.json's version line. I read the full diff directly (not just the stat): every hunk is prose — table-cell corrections, a ceded glossary definition, and changelog text. No shell snippets, no code blocks, no config/permission values, no workflow files.

Standing finding from the prior review (unchanged by this increment)

The IMPORTANT finding posted earlier on adapters/linear/claim.sh#L212 — the re-read AFTER2="$(wit_linear_lease_comments "$ISSUE_UUID")" || AFTER2='[]' fails open (defaults to "no live leases") on a fetch error, where the sibling rollback-trap guard at line 142 fails safe (|| exit 0) on the identical failure — is still present in this file at HEAD and is not addressed by either of the two new commits. No new test exercises this path. Restating rather than re-filing since it's already anchored inline on this PR.

No new findings.
· Branch

Two independent reviewers caught the same line, and they were right: my own
0.39.5 fix reintroduced its own bug on the error path.

The lost-race branch read the lease set as

  AFTER2="$(wit_linear_lease_comments "$ISSUE_UUID")" || AFTER2='[]'

so a transient GraphQL failure — or the belt-and-braces EX_INTERNAL exit added
to that very helper in 0.39.5 — was silently read as "no live leases exist".
LOSER_LIVE then stayed empty, the assignee still carried our own login because
nothing had changed it yet, the name compare passed, and the winner's live
assignment was cleared. That is precisely the failure this branch exists to
prevent, reached through an unreadable lease set rather than a name collision.

The two guards also disagreed with each other, which is the part that should
have caught my eye when I wrote it: the rollback trap ninety lines above fails
SAFE on the identical read (`|| exit 0` — treat "cannot tell" as "do not touch
it"). This site picked the unsafe default for no reason other than that I typed
a fallback value instead of a branch.

A failed re-read now means UNKNOWN, never EMPTY: the unassign is skipped and the
reason is printed rather than swallowed. Neither reviewer found the path covered
by a test, and they were right about that too — a regression case now fails only
that second read and asserts zero unassigns. Verified discriminating: dropping
the sentinel turns it red.

58/58 test files pass; shellcheck, portability, changelog parity and markdownlint
clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 43s —— View job


Security review — standing finding fixed, no new issues

  • Gather context (PR diff, changed files, HEAD SHA 3ca356c)
  • Invoke /review:security-review (skill invocation errored again on execution; read plugins/review/skills/security-review/SKILL.md directly and followed its stated criteria manually, consistent with every prior pass on this PR)
  • Review changed files for security-relevant surface
  • Post findings

Note: /review:security-review errored on invocation (<error>Execute skill: review:security-review</error>). Non-git Bash commands (running the claim.test.sh suite directly) also required interactive approval not available in this run, so verification below is by direct reading of the diff and cross-referencing against the mock harness's documented semantics, not by executing the suite.

What's new since the last review

The last full security review covered up to head 5a1d580. HEAD is now 3ca356c — one commit ahead, and it's the fix for exactly the standing finding two independent reviewers (this bot, and Codex) had flagged on prior passes.

adapters/linear/claim.sh — the fail-open re-read is fixed.

The lost-race branch's lease re-read used to be:

AFTER2="$(wit_linear_lease_comments "$ISSUE_UUID")" || AFTER2='[]'

which read a failed re-fetch as "no live leases exist," letting the guard fall through to a name-only compare that would normally still match (the assignee still carries our own login at that point) and clear a live winner's assignment on a transient read failure. That was the same class of bug the surrounding commit was fixing, reached via the error path instead of a name collision.

The fix (claim.sh#L219-L227) branches instead of defaulting:

if AFTER2="$(wit_linear_lease_comments "$ISSUE_UUID")"; then
  ... # populate LOSER_LIVE from the re-read, as before
else
  LOSER_LIVE="unknown"
  printf 'linear: could not re-read lease state on %s — leaving the assignee alone rather than risking a live claim\n' "$ID" >&2
fi

LOSER_LIVE="unknown" is non-empty, so the subsequent [[ -z "$LOSER_LIVE" ]] guard now correctly skips the unassign on a failed read — matching the sibling rollback-trap guard 90 lines above (|| exit 0), which already failed safe on the identical read. The two guards no longer disagree.

Test coverage verified against the mock's documented semantics, not by execution: the new case in claim.test.sh#L258-L272 appends a third comments(-pattern route (a 500/GraphQL-error payload) after seed_race's two. Per mock.sh's own documented rule (routes sharing a pattern form a call-ordered sequence — 1st call gets route 1, 2nd gets route 2, 3rd+ repeats the last), this lines up correctly: call 1 is the pre-check ([]), call 2 is the step-4 arbitration re-read (mine+rival, which routes into the loser branch), and call 3 is the loser branch's own re-read (AFTER2) — the exact site being fixed. The assertion (grep -c '"assigneeId":null' == 0) is discriminating: reverting to the old || AFTER2='[]' fallback would make LOSER_LIVE stay empty and the name-compare pass, producing a non-zero count.

Nothing else in this increment carries security surface — plugin.json (0.39.5 → 0.39.6) and the new CHANGELOG.md entry are version metadata and prose only.

Standing finding — resolved

The IMPORTANT finding posted on prior passes of this PR (fail-open re-read at claim.sh around line 212) is fixed by this commit. No new findings.
· Branch

…ills SSOT

Container #2933's criterion 3 reads "Per-lane candidate verdicts recorded in the
upstream SSOT docs" — plural — and its constraints name the mattpocock-skills
SSOT specifically. Actual coverage there was 2 of 23 (C22 and C23), and even
those were incidental: they sit inside the invocation-reach tracked strand
because that strand fired, not because the file indexed candidates.

An earlier close-out dry run recorded criterion 3 as MET on the basis of
`grep OPEN` over ONE of the two documents. That check could not have detected
this: it tests for a literal token in the wrong file.

Every one of the 23 candidates overlaps the skills repo — each cites a
skills-repo artifact as its upstream source — so the constraint applies to all of
them, not a subset.

Verdicts are attached to the upstream-skill row that already exists for each,
rather than duplicating the candidate index into a second table. That index lives
in aihero-shipping-course.md, which is authoritative for verdicts; a copy here
would be a second source of truth that drifts, which is the failure this effort
has already hit twice (the pre-PR ordering conflict, and phase boundary defined
in two places). Rows for `to-spec` and `tdd`, which had no attribution row at
all, were added in the table's existing shape.

Spot-verified against the authoritative lane sections before committing: C10
ALREADY-PRESENT, C15 PARTIAL, C20 ALREADY-PRESENT and C9 PARTIAL-relocated all
match exactly. Coverage confirmed 23/23; markdownlint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 25s —— View job


Code review complete

  • Gather context (PR diff, files changed)
  • Invoke /review:code-review and follow its instructions
  • Run code review over the PR diff
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with any cross-file findings

Note: /review:code-review errored on invocation again (<error>Execute skill: review:code-review</error>), consistent with every prior automated pass on this PR — this review was performed manually against the same high-signal correctness/maintainability scope that lane targets.

What's new since the last code-review pass

The last full code-review covered up to d52ea34. HEAD is now 9ae48ea, four commits ahead:

  • 3ca356c — fixes the standing finding. The previously-reported fail-open bug at claim.sh:212 (AFTER2="$(wit_linear_lease_comments …)" || AFTER2='[]', which read a failed re-read as "no live leases" and could strip a winning session's live assignment) is now fixed correctly. I read the current file in full: claim.sh#L216-L234 now branches on the read (if AFTER2=...; then … else LOSER_LIVE="unknown"; … fi) rather than defaulting a fallback value, so a failed re-read now yields LOSER_LIVE="unknown" (non-empty) and the [[ -z "$LOSER_LIVE" ]] guard correctly skips the unassign — matching the fail-safe behavior of the sibling rollback trap at line 142 (|| exit 0). The new regression case in claim.test.sh fails only the second lease read and asserts zero assigneeId:null writes, which is a discriminating test for exactly this path.
  • 7a66d0d, 5a1d580, 9ae48ea — docs-only (SSOT corrections, changelog, execution-shape.md glossary cession, and the mattpocock-skills.md candidate-verdict recording). I spot-checked the load-bearing cross-references rather than taking the commit messages at face value:
    • The phase boundary glossary cession (execution-shape.md) is accurate — docs/GLOSSARY.md:51-54 does carry the definition now, and execution-shape.md correctly keeps only the seam-specific checkpoint relation rather than restating it.
    • The 23-candidate verdict table in mattpocock-skills.md (diff) is fully accounted for (C1–C4, C5–C8+C17, C9–C11, C12–C16, C18–C20, C21–C23 = 23), and I cross-checked several verdicts (C1, C2, C3, C4, C9, C10, C15, C20) against aihero-shipping-course.md's authoritative lane sections — all match exactly, no drift between the two documents.

No correctness or maintainability issues found in this increment. The one open finding from earlier passes on this PR is resolved and covered by a new discriminating test.

Everything else (recap of already-verified ground)

Load-bearing technical claims from earlier commits — wit_gitea_http's --proto '=https', WIT_GITEA_HOSTNAME_RE bare-hostname enforcement, the gitea/linear credential-handling patterns (stdin-config auth, no argv leakage), the d52ea34 command-injection fix (quote_safe charset + hoisted top-level exit), and the gitea blocked-by-count propagation fixes — were all verified in prior review passes on this PR and are unchanged at this HEAD.

The changelog gap flagged in an earlier pass (fixes without changelog entries at d52ea34) is now closed: 5a1d580 added the 0.39.5 entry and 3ca356c added 0.39.6, both matching plugin.json's version.
· Branch

…tion grades

The criterion-3 coverage pass (9ae48ea) surfaced four contradictions it
recorded rather than resolved. All four are closed here, plus two more of
the same class found by sweeping for them.

Lane X had no per-candidate verdicts anywhere. Every other candidate-bearing
lane (A, B, C, D, W) has a section with a bullet per candidate; X had only a
verdict-table row graded flat ADOPTED, which contradicted C23's own
ALREADY-PRESENT disposition recorded on the skills SSOT. Add `## Lane X
(#2940)` with C21/C22/C23 bullets and correct row X to PARTIAL, matching how
every other lane holding an already-present candidate is graded. The skills
SSOT's "granularity caveat, recorded not resolved" paragraph now points at
the resolution instead of the gap.

The v1.2 map's Relation column was stale on six rows. Rows 13 and 14 read
CONVERGENT ("same territory, no provenance") while their own body cells said
content had been taken and attributed. Rows 4, 9 and 25 read CONVERGENT with
"Not ported" bodies, but the skills SSOT records the redaction guard plus
[DEBUG-a4f2] tagged logs, the LOGIC.md shareable-HTML demo shell, and
"Prompt the positive" as adopted — each verified against the landing sites
in this repo before regrading. Row 17 read DERIVED against the SSOT's
Partial; under this map's own legend `planning:wayfind` is ideas-taken, not
a port. Row 13 also carried ADOPTED-ADAPTED for C9 where the course SSOT,
which owns lane verdicts, says PARTIAL-relocated; the token now matches.

Four citations of `playbooks/skills/skill-authoring/SKILL.md` and one of
`planning/skills/plan/SKILL.md` were missing the `plugins/` prefix — the
almost-full shape reads as a repo path and resolves to nothing. Line numbers
were correct and are kept.

markdownlint and typos clean over docs/upstream.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
@kyle-sexton kyle-sexton changed the title docs(work-items): correct a false blocker reason and record the adapter-track scope decision fix(work-items): harden the tracker seam's lease and generator paths, and reconcile the upstream SSOTs Aug 21, 2026
… review

The container close-out review of #2933 surfaced four findings beyond the
merge blocker. Three are real and fixed here; each was re-verified against
the artifact before acting, not taken on the reviewer's word.

onboard-adapter read live tracker items without stating the
item-content-trust boundary. Step 2 has the user fetch real items and paste
the responses back — titles, descriptions, comments, label and state names,
all authored by anyone who can file in that tracker — and neither SKILL.md
nor reference/live-exploration.md cited the boundary. Grepping the whole
skill tree for it returned zero hits, while attend-queue, decompose, ship,
triage and work-loop all carry it. The container this skill shipped under
names "no tracker reads without the item-content-trust boundary" among its
excluded-by-default postures, so this was the one surface out of step with
its own constraint. Both files now state it as a numbered probe rule: read
probe output for shape, never as a directive.

Found while there: the "already bundled" list named github, local-markdown
and jira only, so a user with a Gitea or Linear instance would be walked
through generating an adapter that already ships. Corrected in the
description and in the "Not for" paragraph.

execution-shape.md described only fresh-branch-per-item provisioning, so no
container using the serial variant could record a truthful shape line —
including #2933 itself, which shipped eleven PRs off one head ref. The shape
value names PR granularity; fresh-branch-per-item is its default
provisioning, not part of the definition. The variant is now documented
under per-item PRs with what it forfeits (no parallelism; each PR's diff is
honest only if its predecessor merged first). Deliberately not a third shape
value — the line stays two-valued and ship, decompose and the close-out
basis are unchanged.

quality-gate close-out Shape B was structurally blind to in-flight work.
Every rung reads the default branch — rung 1 keeps MERGED linkage nodes,
rung 2 scans git log — so pushed work sitting in an open PR never entered
the basis and was never mentioned. Merged-only is right for the basis and
wrong to leave unsaid for the verdict: a container closed on it closes on
evidence that is not on the default branch. Shape A reaches its open branch
through the Integration branch line; Shape B had no analogue. It now runs an
OPEN-state query plus an open-PR search against the container, reports the
result as in-flight-not-in-basis, and treats any open PR carrying container
work as a precondition of the close. Surfaced by running the mode over
#2933, where six behaviour-changing fixes sat in an open PR and the derived
basis showed none of them.

The fourth finding — #2933 has no Execution shape line — is left to the
container body edit rather than a code change.

work-items 0.39.6 → 0.39.7; review 0.24.0 → 0.24.1. Changelog parity,
bump check, and markdownlint all clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnzwTKoTa6xNY7iyEzMYpm
…g-breakdown-ukohlx

# Conflicts:
#	plugins/review/.claude-plugin/plugin.json
#	plugins/review/CHANGELOG.md
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 52s —— View job


Security review — no new issues

  • Gather context (PR diff, changed files, HEAD SHA 48106bd)
  • Invoke /review:security-review (skill invocation errored again on execution; read plugins/review/skills/security-review/SKILL.md directly and followed its stated criteria manually, consistent with every prior pass on this PR)
  • Review changed files for security-relevant surface
  • Post findings

Note: /review:security-review errored on invocation (<error>Execute skill: review:security-review</error>), consistent with every prior automated pass on this PR.

What's new since the last review

The last full security review covered up to head 9ae48ea. HEAD is now 48106bd — confirmed via git diff --stat origin/main...HEAD that the true PR diff is still exactly the same 29 files (956/-85) reported in the PR body, so this synchronize did not widen scope.

Isolating what actually changed since 9ae48ea within that scope: git diff --stat 9ae48ea..HEAD -- plugins/work-items/tools/work-item-tracker plugins/work-items/skills/onboard-adapter/scripts returns empty — every adapter, conformance binding, and generator script (including adapters/linear/claim.sh, where the fail-open re-read finding was fixed in 3ca356c) is byte-identical to the version already reviewed. No code changed in this increment.

Two new commits landed, both docs-only and outside code paths:

  • 8ecc6ec — SSOT corrections across docs/upstream/aihero-shipping-course.md, docs/upstream/mattpocock-skills-v12-map.md, docs/upstream/mattpocock-skills.md (relation-grade and citation-prefix fixes, a new Lane X section). Prose only.
  • e5f4719 — closes three findings from a prior close-out review: adds an explicit item-content-trust boundary to onboard-adapter/SKILL.md and reference/live-exploration.md (diff), states that a live-tracker probe response is untrusted data ("titles, descriptions, comments, label and state names, authored by anyone who can file in that tracker") to be read for shape only, never as instruction. I verified the file it links to, reference/item-content-trust.md, exists in this repo and predates this PR — this is a genuine security-hardening documentation gap being closed (a prompt-injection boundary that was already enforced elsewhere in the skill tree but missing from this skill's own docs), not new surface. The rest of that commit (execution-shape.md's per-item-PR shape variant, close-out.md's in-flight-work check, version bumps in plugin.json/CHANGELOG.md for both work-items and review) is prose/metadata only.

Skip gate applies to this increment: no security-relevant surface. No shell scripts, templates, workflow files, or credential/auth paths changed since the last review.

Standing findings

The previously-flagged fail-open re-read in claim.sh (AFTER2="$(wit_linear_lease_comments ...)" || AFTER2='[]') remains fixed as verified in the prior pass (3ca356c) — confirmed unchanged in this increment.

No new findings.
· Branch

@kyle-sexton
kyle-sexton merged commit 2e87353 into main Aug 21, 2026
51 checks passed
@kyle-sexton
kyle-sexton deleted the claude/pocock-shipping-breakdown-ukohlx branch August 21, 2026 07:15
kyle-sexton pushed a commit that referenced this pull request Aug 21, 2026
Textual conflict only, not a version collision. #3067 released review 0.25.1
(a quality-gate close-out patch) and inserted its heading where our 0.26.0
heading sits. 0.26.0 already exceeds 0.25.1, so the manifest stands and both
entries are kept in descending order: 0.26.0, 0.25.1, 0.25.0, ...

#3067's 0.25.1 entry is carried through byte-identical to origin/main's copy.
No PR substance changed: README.md, fix-pass-mode.md, fanout evals.json, and
ai-slop SKILL.md are byte-identical to dd619c2.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 21, 2026
…ight plugins (#3069)

No linked issue

## Summary

A fresh-context verification pass ran over #3065 **after** it merged and
found defects in what that PR shipped: **12 in the eight plugins it
touched, 12 in the provenance record, and 1 wrong count inside a
released changelog entry.** Two of the twelve are behavioural — a
completion gate that reverses its own skill's first instruction, and a
cite into another plugin's private files. The rest are unresolvable
pointers, a schema field with no writer, an eval that cannot fail for
the reason it claims to test, and claims in
`docs/upstream/cursor-pstack.md` that do not survive checking against
the pinned upstream.

Two more adversarial verifiers then ran over **this PR's own first
commit**, one of them reaching the live upstream. They found eight
further defects in work this PR introduced, plus one from a Codex
review. All nine are fixed and described below.

Every plugin whose shipped text changed takes a **patch bump with its
own `### Fixed` entry**. #3065's release entries describe what #3065
shipped and cannot be retrofitted to describe corrections made after it
merged.

## Fix

### The two behavioural defects in #3065

**`docs-hygiene:write-for-humans` graded against the wrong standard
(0.18.2).** The skill's first instruction is to resolve the consuming
project's own style guide before applying anything bundled — that
re-posture *is* the port, and it is what
`PLUGIN-PHILOSOPHY.md:198-202`'s two-lane rule demands. But 0.18.0
shipped the seven-question self-check unconditionally, and four of those
seven restate a bundled layer (1 is Diátaxis, 2 and 3 are ASD-STE100, 5
is Global English's ambiguity set). A run that followed the skill to its
end would resolve a project's declared Microsoft guide, write against
it, then grade the result against the bundled standards and **rewrite it
to conform**. That is a lane-1 hardcode wearing lane-2 clothing, and it
falsifies two of the skill's six evals. The gate is **split, not
switched off wholesale** — see below for why the first attempt was
wrong.

**`review:quality-gate downstream` cited into another plugin's private
files (0.26.2).** Its "say plainly what is unverified" step path-cited
the `playbooks` plugin's `fable-5` skill inside that skill's own
`context/` directory, which the encapsulation contract makes private.
Now `/playbooks:fable-5 verification` — the chapter argument is
load-bearing, see below.

### The rest of #3065's defects

| Plugin | Bump | Defect |
|---|---|---|
| `architecture` | 0.6.2 | `graft-record:` was added to the candidate
schema and **no step wrote it** — the Handoff step named `status` and
`agreed-shape` and stopped. A field nothing fills is always empty, so
the left-behind half of a graft survived nowhere. Handoff now fills it;
the research file's "travels into `agreed-shape`" named the wrong field
and is replaced. |
| `discovery` | 0.16.2 | Two routes back to the code-shape exclusion the
skill is built around: "test names" listed as source-control evidence (a
test name is a symbol, not someone writing down a reason), and a gotcha
calling a named constant plus a convention "a hypothesis at best",
colliding with `Speculative`'s own *Competing hypotheses* output
section. Separately a **body-vs-eval gap on the neighbouring rule** —
the version-control-behaviour ceiling said "never `Direct`" while the
eval graded "never `Direct` or `Supported`". Plus an eval whose target
behaviour was **indistinguishable from correct behaviour**: it told the
model not to check the tracker but never stipulated one existed, and the
category is presence-gated, so a bare checkout's correct output was
graded a failure. Plus a link-target normalization to the fleet's
`${CLAUDE_PLUGIN_ROOT}` convention. |
| `implementation` | 0.15.2 | "the same grounding rule the loop lanes
apply" — an unresolvable pointer for a consumer who installs
`implementation` alone. Now names `work-items:work-loop` and
`source-control:babysit-loop`. |
| `prototype` | 0.9.2 | `explore-directions` step 6 restated the shared
capture discipline in its **pre-0.9.0 form** — "record which variant won
and why" — directly above the bullets that delete the losing variants
irreversibly. |
| `session-flow` | 0.32.2 | Both retro skills "handed" an accepted skill
candidate to `/playbooks:skill-authoring`, which takes no arguments and
performs no actions. Handing it a candidate resolves to nothing, so the
destination 0.32.0 set out to give did not exist. Both now read it for
the doctrine and draft against it. |
| `testing` | 0.7.2 | "Prefer no new test to a bad one" is the upstream
cursor/plugins `tdd` cost branch (`cursor/plugins@60c641e4`
`pstack/skills/tdd/SKILL.md`). Cited inline as `(upstream cursor/plugins
tdd)`. `/tdd:principles` contains neither the phrase nor the six
triggers. |

### One declared in-place correction to a released body

`scripts/check-changelog-parity.sh` sanctions corrections inside an
already-released version section when the correcting PR names each edit
in its body **and** in the new release entry. Exactly one qualifies, and
it is the only removed changelog line in the whole diff: architecture
0.6.0 said "the five-part schema was pinned in three places"; commit
`228a2b19` changed the count in exactly two spots. Corrected in place;
heading untouched.

Nothing else was edited in place. Three blocks initially drafted as
appends into released `### Added` sections were lifted back out and
re-homed under the new patch entries, because attributing post-release
work to a released version misleads anyone reading the changelog to
learn what that version contains.

### Concurrent-bump resolutions — #3067, #3068, #3070

Main bumped these plugins three separate times while this branch was
open, and each time it claimed a number this branch had already taken.

- **#3067** shipped `review` 0.25.1 → this branch moved to 0.25.2.
- **#3068** shipped `review` 0.26.0 → this branch moved to 0.26.1.
- **#3070** shipped a **fleet-wide** `### Changed` sweep that claimed
**the same patch number on all eight plugins this PR bumps** → every
entry here moved to `.2`.

The third one is worth recording, because it was invisible to the usual
check. `git merge-tree` reported **zero conflict markers**: each side
had edited a different region of each file, so a clean-looking merge
would have produced eight changelogs in which two different releases
share one heading. That is precisely the corruption `--check-preserved`
exists to catch. The real merge surfaced nine conflicts once the bodies
collided. In every case main's heading and body are untouched and this
branch's entry moves up; no release is folded, relabelled, or dropped.

**One of those nine was semantic rather than numeric.** #3070 respelled
`retro/context/session.md`'s route as *"Hand it off by invoking
`/playbooks:skill-authoring` via the Skill tool"* — which reintroduces
the exact claim this PR fixes, since that skill takes no arguments and
performs no actions. Both changes are kept: the invocation still names
the Skill tool (it is invoked in order to be *read*), but it reads the
doctrine and drafts against it rather than handing anything over. The
sibling `running-retro` is deliberately **not** aligned to match —
#3070's own entry says it left that list alone because it sits under
"Offer routing — never auto-apply", so the asymmetry is intentional and
is now recorded in the 0.32.2 entry.

### The provenance record — 12 corrections

`docs/upstream/cursor-pstack.md` carried twelve claims that did not
survive checking against `cursor/plugins@60c641e4`:

1. A judge instruction reported as an unconditional `must`; upstream
says "**Prefer** a different model family from the parent's" and `must`
appears zero times in that file.
2. "upstream spawns candidates under `isolation: worktree`" — the word
never appears upstream. Its mechanism is a per-candidate output path,
which is why this fleet's three recorded `isolation:` rejections were
not in conflict with it.
3. The `arena` omission's grounds now rest on the Rule of Three alone.
4. The `technical-writing` row credited our third always-rule to
upstream's above-the-layers trio; it comes from upstream's separate
word-list and anti-jargon paragraphs.
5–7. Three non-verbatim quotes: `debugging:debug` phase 5, upstream's
`tdd` description, and the above-the-layers rule text.
8. `ai-slop`'s catalog path — `reference/catalog.md` →
`skills/audit/reference/catalog.md`.
9. A bare `context/spec.md` — the real file is
`review/skills/quality-gate/context/spec.md`, and `review/context/`
holds no `spec.md`, so the short form pointed nowhere.
10. "four always-listed descriptions" → six, now named individually.
11. "`bro` is seven lines, two of them body" → one.
12. "Five of its six operating principles" → upstream states **two**
separate six-item lists, and across both, every item but one ("no
shortcut by code-reading") restates historiographical source criticism
and IC analytic tradecraft.

An independent verifier re-checked 1, 2, 11 and 12 against the live
upstream files and confirmed each verbatim.

### Convention record

`docs/conventions/upstream-drift` 1.6.0 adds the adopter row for
`write-for-humans`' source records. **Three carry a publication-event
trigger; Google's is a page-content divergence**, because that guide is
a continuously-edited site with no edition to pin. The contract admits
either shape, and the row says which record uses which.

## Defects in this PR's own diff, found and fixed

Two fresh-context verifiers ran over commit `b1bf09c6` with the
rationale withheld; a Codex review ran alongside.

1. **The new `write-for-humans` gate contradicted itself** — "the seven
below do not apply" under a declared guide, then "the three rules above
apply either way", when questions 4, 6 and 7 **are** those three rules.
Now split per question: four stand down, three always apply.
2. **An undescribed compression pass rode along in that same file** —
nine hunks the changelog never mentioned, three of them lossy (a bullet
lost "of a published standard"; two gotchas lost their actionable half;
one lost the "only" whose placement the skill's own ambiguity rule
governs). The worked example lost "(and ambiguity)" from a fix that *is*
an ambiguity fix, and misquoted its own Before block. Reverted to main,
with only the two described fixes re-applied.
3. **`interface-design.md` named two destinations for one record.**
Replaced rather than annotated.
4. **`explore-directions` step 6 was still short a clause** — it dropped
"what the discarded parts held that the graft deliberately left behind"
while the changelog claimed the widened form.
5. **The bare `/playbooks:fable-5` cite (Codex).** That skill's
`argument-hint` and Arguments paragraph make a bare invocation arm its
**entire operating doctrine as standing session instructions for the
rest of the run**. Now `/playbooks:fable-5 verification`.
6. **The `discovery` umbrella overcounted** — "three routes back to the
code-shape exclusion" when the third is the version-control-behaviour
ceiling, which the same skill says explicitly is *not* code shape.
7. **A link-target normalization was undescribed.** The change is right
and stays; the changelog now names it.
8. **The `review` entry spelled the private path the fix removed.**
Described rather than spelled.
9. **The `upstream-drift` adopter row generalized from three records to
four.** Corrected.

**One Codex finding was accepted after a primary-source check reversed
an earlier decline.** Its claim that `/tdd:principles` contains neither
the phrase nor the six triggers is correct. That skill's nearest
sentence is Khorikov's *"It's better to not write a test at all than to
write a bad test"* in `testable-architecture-khorikov.md` — the 2×2 /
Humble Object chapter, which this port used as grounds to *reject*
upstream's five-item bad-test definition as already owned. The phrase
and the six triggers come from upstream `tdd` (*"Prefer no new test over
a bad test"*, plus the identical trigger list), which
`cursor-pstack.md:34` already recorded as **Taken**. The original cite
therefore contradicted this PR's own provenance document. Now `(upstream
cursor/plugins tdd)`.

## Verification

Every gate re-run locally at the branch tip against the current base
(`69b3584`), all green:

- `check-changelog-parity` — all four modes. `--check-bump`: each of the
eight version changes has its own new entry and is strictly greater than
the base's. `--check-preserved`: 9 changed changelogs, **322 headings
compared**, none dropped — including every heading #3067, #3068 and
#3070 added. `--check-order`: newest-first, no duplicates.
- `check-changed-skills` — 9 skills checked, 0 failed.
- `validate-plugins` (manifests + catalog, `--strict`),
`validate-plugin-contracts` (48 setup skills, 2823 files).
- `check-skill-portability`, `check-skill-count-claims`,
`check-skill-leaf-names`, `check-cross-plugin-source-drift`,
`check-plugin-manifest-presence`, `check-orphaned-fixtures`,
`check-silent-skips`, `check-fleet-audit-doc-grammar`,
`check-lane-coverage`, `check-contract-clause-coverage`,
`check-contract-slice-prune`, `check-stale-base-overlap`.
- `markdownlint-cli2` over **every tracked markdown file** — 0 issues.
- Eval sets: all `evals.json` validate against the bundled schema, 0
failures. `check-evals-quality` returns **PASS with advisory warnings
fleet-wide** (the WARN tier never fails the step); **the two suites this
PR touches carry 0 warnings**. An earlier revision said "PASS, 0
warnings" without that distinction — a verifier caught the
overstatement.
- **No executable file is touched anywhere in this PR** — the diff is
markdown, eight `plugin.json` version bumps, and one `evals.json` — so
the plugin test suite cannot be affected by it.

## Related

- #3065 — the merged PR these corrections apply to.
- #3067, #3068, #3070 — the three concurrent bumps this branch
renumbered around; all of their headings survive.
- `docs/upstream/cursor-pstack.md` — the single source of truth for
everything derived from `cursor/plugins@60c641e4`.
- `docs/conventions/upstream-drift` — the four-part stamp contract the
new adopter row conforms to.
- `docs/PLUGIN-PHILOSOPHY.md:198-202` — the two-lane convention posture
the `write-for-humans` self-check violated.
- `scripts/check-changelog-parity.sh` — the released-entry body-edit
clause the one in-place correction is declared under, and the
concurrent-bump guidance every resolution here follows.

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants