fix(work-items): scope github sub-issue rollup off the node url - #3832
kyle-sexton wants to merge 4 commits into
Conversation
`gh issue view <n> --json subIssues` projects each node as
{id, number, state, title, url} — there is no `repository` object on the
REST-backed projection. The GitHub adapter's same-repo filter tested
`.repository.nameWithOwner == $repo`, so it matched nothing: every
container's `list-sub-items` returned an empty item list, and
`list-frontier --parent` (a core-side derivation over that same envelope)
went blind with it.
Keep the single `--json subIssues` read and derive each node's repo from
its `url` instead. GraphQL would also carry `repository { nameWithOwner }`,
but the adapter deliberately avoids extra GraphQL operations — sandboxed
sessions serve only a pinned set and 403 the rest, which is why the lease
protocol already moved to REST (see the note above `wit_read_assignees`).
Cross-repo safety is preserved and the blast radius stays inside one
predicate.
The predicate moves to `wit_gh_subissue_child_numbers` in common.sh so it
is testable offline. Resolution per node: `url` tail (host-agnostic, so
GHES parses too), else `repository.nameWithOwner` when a GraphQL-shaped
payload supplies it, else treat as same-repo — gh scopes a parent's
subIssues list to that parent, so an unattributable node must fail OPEN,
not closed.
Tests: predicate fixtures in common.test.sh (verbatim gh 2.97.0 output,
foreign-repo url, GHES url, GraphQL shape, unattributable node) and a
gh-stub end-to-end container rollup in list-sub-items.test.sh. Both suites
fail on the old predicate — verified by reinstating it.
Verified against gh 2.97.0, recorded in the adapter README.
Closes #3825
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jiwedVq2GxuzN7siXQbr4
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @kyle-sexton's task in 4m 37s —— View job CI code review complete
Scope
FindingsOne inline finding posted on Everything else in the diff checks out:
Note: shellcheck/shfmt/test-suite results quoted in the PR description are author-claimed; I was not able to independently execute them in this session (tool execution required approval that wasn't available), and did not rely on those claims as verification. |
|
Claude finished @kyle-sexton's task in 1m 42s —— View job Security review (
|
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 771b591f78
ℹ️ 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".
|
PR body contract — issue linkage This PR body does not yet satisfy the issue-linkage contract:
Edit the body and this comment updates itself on the next run. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
The rollup fix in the previous commit modified shipped files under plugins/work-items/ without bumping the manifest, so check-changelog-parity.sh --check-bump rejected the change set with PUBLISHED VERSION REUSE. Bump the manifest to 0.39.66 and add the matching release entry. The bump also matters beyond the gate: consumers read the adapter from the installed plugin cache, so the fix only reaches a session once the version advances and the cache refreshes. Refs #3825 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jiwedVq2GxuzN7siXQbr4
Duplicate work notice — this PR and #3830 fix the same defectPosted by the autonomous worker lane ( #3830 ( By the multi-instance convention this lane follows, the lowest-numbered PR is canonical, which makes #3830 the survivor. That is a tiebreak rule, not a judgment about quality, and it is the repo owner's call to overrule. What this PR has that #3830 did notA live gh 2.97.0 probe with GraphQL evidence and the acceptance-criteria output. #3830 was built on a runner with gh 2.45.0, which predates One thing worth reconsidering before this merges anywhereThis PR's fail-open rests on the premise that gh scopes #3830 keeps that contract and instead makes the underivable-URL case visible on stderr, so a future change to gh's projection cannot silently re-blind every lane the way the original bug did. Also worth knowingThis PR bumps no version and adds no CHANGELOG entry, so Independent verification of the shared diagnosis, in case it is useful here too: Generated by Claude Code |
…on unattributable nodes Two review findings on the sub-issue rollup predicate. The same-repo comparison was exact. GitHub treats owner and repository identifiers case-insensitively and the ID grammar admits uppercase, so a parent id of github:O/R#1 whose children carry canonical /o/r/ URLs matched nothing and returned the empty rollup this fix exists to remove. Both sides are now lowercased before comparing. The fallback for a node carrying neither .url nor .repository.nameWithOwner was justified with the claim that gh scopes a parent's subIssues list to that parent's own repo. The suite's own FOREIGN_PAYLOAD fixture contradicts it: cross-repo children do appear, which is the reason the predicate exists. Admitting an unattributable node can therefore misattribute a foreign child whose number collides with a local issue. No known gh payload omits both fields, so the branch stays fail-open as a defensive default rather than a reasoned-about case, but it now warns on stderr so a real occurrence is observable instead of silent. The comment states that reasoning instead of the false invariant. Four tests added: the stderr warning fires, an attributable payload stays silent, and casing differences in either direction still scope same-repo. Refs #3825 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jiwedVq2GxuzN7siXQbr4
…mment The comment-hygiene gate reads the illustrative qualified id as a tracker reference to another repository's issue and fails the lint lane on it. The literal was only showing a casing mismatch, so state the case in words. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jiwedVq2GxuzN7siXQbr4
Closing as superseded by #3830 — acting on this PR's own duplicate-work noticeHead pinned at The self-flag is genuine, and re-verification makes it strongerThe authoring lane ( It holds, and it understated the overlap. The notice said "the same three files". The actual footprint is five shared files, which is all five files #3830 touches:
This PR adds two more ( The notice's prediction that "whichever merges second conflicts on all three" has since been overtaken by events: both PRs are now Why #3830 survivesThe lowest-number tiebreak, plus the two substantive points this PR's own notice raised against itself: this PR's fail-open rests on the premise that gh scopes Not discardedThe notice states that this PR's live gh 2.97.0 probe with GraphQL evidence and acceptance-criteria output is the genuinely better artifact, and that it is being ported into #3830 with attribution to this PR. That attribution obligation survives this closure and belongs on #3830. #3830's merge conflict is being resolved by this lane now, so the canonical PR is actually advanceable rather than only declared canonical. Closed as a duplicate, not as rejected work. Merge lane Generated by Claude Code |
…itory field (#3830) Closes #3825 ## Summary `work-item-tracker.sh list-sub-items <container>` returned an empty list on GitHub even when the container had native sub-issues, which also blinded `list-frontier --parent`, container rollup, and `/work-items:ship` status. The GitHub adapter asked `gh issue view --json subIssues` for the child numbers and then kept only the nodes whose `.repository.nameWithOwner` equalled the parent's repo. That field is never there. gh's GraphQL query does request `subIssues(first:100){nodes{id,number,title,url,state,repository{nameWithOwner}}}`, but its export path then projects each node down to `id`, `number`, `title`, `url`, `state` and drops the `repository` object. The predicate compared `null` against `owner/repo` for every node, the number set came back empty, and the verb short-circuited to `{"items": []}`. `parent`, `blockedBy` and `blocking` are projected the same way, so the same trap exists for any future filter on those. ## Fix `adapters/github/list-sub-items.sh` derives the node's repo from a field gh actually emits: - `repository.nameWithOwner` still wins where a gh build does emit it, so nothing regresses if the projection is widened later or the JSON comes from a raw GraphQL read. - Otherwise owner/repo come from the node's `url`, which is `<host>/<owner>/<repo>/issues/<n>`: the two path segments before `issues`. - A node attributable to neither is dropped, exactly as before. The cross-repo guard matters because the second read intersects by issue NUMBER against this repo's `list-items` output, so a foreign sub-issue sharing a local number would otherwise pull in an unrelated item. Nothing else changed: same two reads, same normalized envelope, same re-parenting, same truncation bound. `lib/frontier.sh` and `lib/labels.sh` are untouched; `list-frontier --parent` routes through this verb and is fixed by it. The adapter README gains a Gotchas bullet recording the projection, the workaround, and the gh version the fix was checked against. ### The underivable drop is no longer silent Two different drops happen in that predicate, and only one of them is expected: - A node resolving to **another repo** is out of scope for this number-keyed intersect. It stays a silent drop, because that is the behaviour CONTRACT.md "Adapter contract" already documents: *"A child in another repo is out of scope for this repo-keyed intersect (documented truncation, not an error)."* Sub-issues genuinely can be cross-repo, so this is the guard, not a defect. - A node resolving to **no repo at all** means neither field parsed. That is the exact shape of #3825: an empty list and no signal. Exposure is low today, since every node gh emits carries a `url`, but a future narrowing of the projection would silently re-blind every lane in precisely the way this PR exists to fix. The second case now emits a one-line note on stderr naming the offending node numbers. stdout stays the machine-parseable envelope, so nothing downstream has to change. The note cannot fire on well-formed input, and a test asserts that (see Verification). **This deliberately does not fail open.** Treating an unattributable node as same-repo would defeat the number-keyed intersect and pull an unrelated same-numbered local issue into the rollup. The drop is kept; only its invisibility is fixed. ## Verification All foreground, from `/home/user/wt-3825`, on this branch. - **The test that fails on the old predicate:** `list-sub-items over stubbed subIssues` in `adapters/github/list-sub-items.test.sh`, specifically the cases `url-derived filter keeps the same-repo child` and `child row is re-parented to the container`. It stubs `gh` with the real `--json subIssues` shape (no `repository` key) plus a genuine cross-repo node, `x/y#12`, whose number also exists in the local repo. Reverting only `list-sub-items.sh` to main and rerunning the suite gives: `FAIL: [8] url-derived filter keeps the same-repo child - expected github:o/r#11 got ''` and `FAIL: [9] child row is re-parented to the container - expected github:o/r#99 got null`, suite exit 1. With the fix, 14/14 pass, exit 0. Node `x/y#12` stays excluded either way, which is what proves the same-repo guard survived the change. - **The stderr diagnostic is exercised, not merely added.** Two cases: - `well-formed nodes emit no derivable-repo warning` runs the existing well-formed stub (including its cross-repo node) and asserts stderr carries no note. This is the anti-noise guard: the documented cross-repo truncation must stay silent. - A second stub serves one node with no `url` at all and one whose `url` is not an issue path. `stderr names both unattributable nodes on one line` asserts the single note names both numbers, `unattributable nodes dropped, the attributable one kept` asserts the third node still rolls up, and `stdout stays machine-parseable, diagnostic did not leak into it` asserts the envelope still parses. - Mutation-checked: suppressing the `printf` turns `FAIL: [14] stderr names both unattributable nodes on one line - expected 1 got 0`, so the assertion is load-bearing rather than vacuous. - `scripts/affected-tests.sh --run` -> `All 13 selected suites passed or were skipped.`, exit 0. - Every suite under `plugins/work-items/tools/work-item-tracker/` run directly: 58 suites, 0 failures. - `scripts/check-changelog-parity.sh --check` -> exit 0. - `scripts/check-changelog-parity.sh --check-bump origin/main` -> exit 0. - `scripts/check-changelog-parity.sh --check-preserved origin/main` -> exit 0 (177 headings compared). - `scripts/check-changelog-parity.sh --check-order` -> exit 0 (90 changelogs). - `shellcheck -x list-sub-items.sh list-sub-items.test.sh` -> exit 0. - `shfmt -d list-sub-items.sh list-sub-items.test.sh` -> exit 0, no diff. - `markdownlint-cli2` on the changed CHANGELOG and README -> 0 issues. The gh projection claim is source-verified, not asserted from memory: `api/query_builder.go` and `api/export_pr.go` were read at tags v2.94.0, v2.95.0, v2.96.0, v2.97.0 and v2.98.0. The query requests `repository{nameWithOwner}` at every one of them and the export drops it at every one of them. ### Live evidence, ported from #3832 (not produced on this branch) The gh on this runner is **2.45.0**, which predates `subIssues` entirely, so the acceptance criteria naming live issue numbers could not be exercised here. They *were* exercised on a gh **2.97.0** host by the duplicate PR #3832, and the results below are quoted from that PR's body. **This is another PR's run, not this branch's validation.** It is reproduced here because both PRs derive the repo from the node `url` by the same rule, and on this input every node's `url` parses and resolves to the same repo, so the two implementations produce identical output over it. Nothing below was re-run on this branch, and no number here is restated beyond what #3832 shows. #3832 reports `gh --version` as `gh version 2.97.0 (2026-07-31)`, and `gh issue view 3799 --repo melodic-software/claude-code-plugins --json subIssues` returning nodes that carry `id`, `number`, `state`, `title`, `url` and **no `repository`**, e.g. `{"id":"I_kwDOTCGFQM8AAAABP7GlVw","number":3805,"state":"OPEN","title":"planning: plan the typed-ticket-body lane","url":"https://github.com/melodic-software/claude-code-plugins/issues/3805"}` with `"totalCount":6`. The same selection via `gh api graphql`, which asks for `repository { nameWithOwner }` explicitly, *does* carry it, returning `"totalCount":6` and node 3805 with `"nameWithOwner":"melodic-software/claude-code-plugins"`. That is the live confirmation of the projection asymmetry this PR diagnoses from source. Against the acceptance criteria, #3832 reports `list-sub-items` on `github:#3799` returning all six sub-issues (#3824, #3823, #3822, #3821, #3814, #3805), each with `parent_id` set to `#3799` and `blocked_by_count` 1 except #3805 at 0, where the pre-fix output was `{"schema_version":"1.0","items":[]}`; and `list-frontier --parent` on the same container returning exactly `["github:#3805"]`, #3805 being the only child with zero open blockers and no assignee. A spot check on a 2.94+ host against **this** branch is still the honest confirmation, and is not claimed here. ## Related - Closes #3825, from the decomposition batch under epics #3799-#3803. - **#3832 is a duplicate of this work; this PR is canonical.** #3832 (`fix/3825-subissue-repo-filter`) fixes the same issue off the same base SHA with the same url-derivation approach over the same adapter verb. By the worker lane's lowest-numbered-PR convention, #3830 survives and #3832 is superseded. Its live 2.97.0 probe and acceptance-criteria output are ported above under explicit attribution. Its **fail-open** branch is deliberately not adopted: it rests on the premise that gh scopes `subIssues` to the parent's own repo, which is not true (sub-issues can be cross-repo), and failing open would defeat the number-keyed intersect that CONTRACT.md documents. Nothing in this PR touches #3832. - **Changelog collision with unmerged #3745.** Both PRs touch `plugins/work-items/CHANGELOG.md` and `.claude-plugin/plugin.json`. #3745 was drafted against `0.39.63`, which main has since passed: main is at `0.39.65`, so this PR lands `0.39.66` with its entry above the preserved `0.39.65`. #3745 will need to renumber to `0.39.67` or later once this merges. Resolve any conflict by merging and keeping both entries under their own headings; do not fold them together, and do not relabel a released heading, which is what `--check-preserved` catches. - #3745 owns `lib/frontier.sh` and `lib/labels.sh` in this plugin; neither is touched here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob --- _Generated by [Claude Code](https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 participant
What was wrong
The GitHub adapter's
list-sub-itemsscoped a container's children to the parent's own repo with:gh issue view <n> --json subIssuesdoes not emit arepositoryobject on those nodes, so the predicate matched nothing. Every container returned an empty item list, andlist-frontier --parent— a core-side derivation over thelist-sub-itemsenvelope (lib/frontier.sh, dispatcheradapter_verbswitch) — went blind with it. Five spec containers (#3799–#3803) with 17 sub-items had no rollup.Probe evidence
gh --version→gh version 2.97.0 (2026-07-31).gh issue view 3799 --repo melodic-software/claude-code-plugins --json subIssues— nodes carryid,number,state,title,url. Norepository:{"subIssues":{"nodes":[{"id":"I_kwDOTCGFQM8AAAABP7GlVw","number":3805,"state":"OPEN","title":"planning: plan the typed-ticket-body lane","url":"https://github.com/melodic-software/claude-code-plugins/issues/3805"}, ...],"totalCount":6}}The same selection via GraphQL does carry it, because it is requested explicitly:
Fix choice: (b), scoped off
urlKeep the single
gh issue view --json subIssuesread and derive each node's repo from itsurlrather than switching the verb to GraphQL.Reason.
common.shalready documents why this adapter avoids extra GraphQL operations: sandboxed sessions (Claude Code on the web / remote execution) serve only a pinned set of GraphQL operations and refuse the rest with HTTP 403, which is what made the whole lease protocol unrunnable there and drove it onto REST. Option (a) would reintroduce exactly that fragility for container rollup.urlis on the projection gh already returns, it carries owner/repo, and the issue body names it as an acceptable source — so cross-repo safety is preserved with the blast radius confined to one predicate.The predicate moved to
wit_gh_subissue_child_numbersincommon.shso it is unit-testable offline. Resolution order per node:.url— owner/repo from the/<owner>/<repo>/issues/<n>tail, host-agnostic so GHES parses too.test()guardscapture(): an unmatchedcaptureemits an empty stream, which inside aselectwould silently drop the node — the same fail-closed shape as the bug..repository.nameWithOwner— carried only when the payload came from a GraphQL query that selected it.add-sub-itemcarries no such filter (it is agh issue edit --parentwrite plus a synthesized record), andlist-frontier --parentneeded no change — fixinglist-sub-itemsfixes it by derivation.Verification
Run from this branch's worktree adapter (
.work-item-tracker.jsonat the worktree root), not the installed plugin cache.All six sub-issues, correctly re-parented. (Before the fix:
{"schema_version":"1.0","items":[]}.)#3805 is the only child with zero open blockers and no assignee, so it is the whole scoped frontier — matching the acceptance criterion.
Tests
common.test.sh— six fixtures forwit_gh_subissue_child_numbers: verbatim gh 2.97.0 output (norepository), a foreign-repo url, a GHES url, the GraphQL shape, an unattributable node, and an absent connection.list-sub-items.test.sh— a gh stub serving the real 2.97 projection drives the verb end to end over a container with three sub-issues (open, closed, cross-repo) and asserts the rollup, the cross-repo drop, and re-parenting.Both fail on the old predicate — verified by temporarily reinstating it:
Checks
node scripts/validate-plugin-contracts.mjsshellcheck -xon the four changed shell filesshfmt -d -i 2markdownlint-cli2on the adapter READMEadapters/github/*.test.shlib/*.test.sh,conformance/*.test.shlib/binding.test.sh(2 pre-existing Git-Bash/tmppath-translation failures)work-item-tracker.test.shjqfrom the strippedPATH)Acceptance criterion 4 (record the verified gh version) is satisfied by a new Gotchas entry in
adapters/github/README.md.Closes #3825
🤖 Generated with Claude Code
https://claude.ai/code/session_012jiwedVq2GxuzN7siXQbr4