fix(discovery): drop the CLAUDE_CODE_FORK_SUBAGENT gate from forked explore - #1273
Conversation
…xplore Closes #1267 `plugins/discovery` conditioned skill-level `context: fork` on `CLAUDE_CODE_FORK_SUBAGENT=1` in four places. That variable does not gate `context: fork`. Per the sub-agents reference (fetched 2026-07-24) the variable gates the Agent tool's `fork` subagent type and the `/subtask` command. The skills reference documents `context: fork` with no environment gate at all. Its actual effect also runs the opposite direction from the claim: setting it forces every subagent to the background and nullifies the frontmatter `background` field, so unset is the normal working state. The documented fallback was unreachable. `explore-deep/SKILL.md` declares `context: fork` in its own frontmatter, so the body executes inside the fork and cannot detect fork-unavailability. The eval case asserting that branch is deleted rather than adjusted. `explore-deep` also claimed it inherits the parent's full toolset. That is conversation-fork behavior; the skills reference states a forked skill's subagent is a regular agent type, so the background-subagent exemption for conversation forks does not cover it. The narrower background built-in tool set is now enumerated in the skill, with `background: false` named as the escape hatch. No functional change today — every tool the workflow uses is inside that set. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 4s —— View job PR Review — fix(discovery): drop the CLAUDE_CODE_FORK_SUBAGENT gate from forked explore
SummaryThis is a clean, well-reasoned correction. The core argument is sound and each change is justified. I have one substantive concern about the new tool-set enumeration, a minor nit on the escape-hatch wording, and a process note inherited from the PR description. Findings1. Tool-set enumeration is correct in premise but brittle in form — consider a lighter referenceThe new paragraph enumerates every tool in the background built-in set by name, with a point-in-time citation (
— captures the constraint without needing maintenance every time the platform adds a tool. Flagging as a recommendation rather than a blocker; the current wording is not wrong. 2.
|
|
Claude finished @kyle-sexton's task in 1m 22s —— View job Security Review — PR #1273
No security issues found. All six changed files are documentation, configuration metadata, or skill prompt text. The changes are documentation corrections — removing an incorrect environment-variable gate, correcting a false toolset claim, and deleting an unreachable eval case. Here is the file-by-file breakdown:
Toolset enumeration note (not a new issue): The updated GitHub Actions, dependencies, permissions: The PR does not touch |
![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: 58a3e127da
ℹ️ 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".
…e reference Review findings on #1273, both on the one sentence the prior commit added. The narrower background-subagent tool set and the `background` frontmatter field are BOTH gated at Claude Code >=2.1.218. The added text stated the behavior unconditionally and prescribed `background: false` as the remedy — a field that does not exist below that version. Below it a forked skill always ran in the foreground, which is exempt from the background tool filter, so the sentence this PR deleted was the correct statement on those versions. The text now carries the version floor and states the pre-2.1.218 behavior. The same sentence hand-copied Anthropic's 18-name background allowlist verbatim. That list is upstream-owned and already changed once; a `fetched` stamp acknowledges the drift risk without solving it, and nothing re-flags the line when the list next changes. Replaced with a pointer to the reference anchor that owns it — pointer-not-copy, per this fleet's own rule. Also dates the new CHANGELOG entry. Nine of the ten prior entries are dated; only 0.8.2 immediately above is not, and the prior commit propagated that outlier. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 5m 0s —— View job PR Review — fix(discovery): drop the CLAUDE_CODE_FORK_SUBAGENT gate from forked explore
SummaryThis is a clean, well-reasoned correction. Both commits hold up under documentation verification. The second commit ( Addressing the Codex P2 concernThe Codex reviewer stated that This is incorrect. Per the skills reference fetched 2026-07-25:
And from the skills reference body:
The Codex reviewer conflated two distinct mechanisms: a conversation fork (via Findings1. Toolset claim is now correctly indirect — addresses prior review recommendationThe second commit replaced the inline tool enumeration with a pointer to the sub-agents reference, which is exactly what the prior review recommended. The current text defers to the canonical source and avoids the maintenance trap of a stale inline list. No concerns. 2.
|
|
Claude finished @kyle-sexton's task in 1m 32s —— View job Security Review — PR #1273
No security issues found. All six changed files are skill prompt text, JSON eval fixtures, changelog/readme documentation, and a version-bump manifest. The changes are documentation corrections — removing an incorrect environment-variable gate, updating a toolset description, and deleting an unreachable eval case. None introduces a new code-execution path, injection surface, credential-handling change, or permission-widening. File-by-file breakdown
Toolset paragraph (
|
#1273 landed 0.8.3 for discovery while this branch held the same number, so this entry renumbers to 0.8.4 and stacks above it. Also carries criterion 9's strengthening into the eval that checks it: the expectation still read only the "topmost existing rung" off the output, so a response could nominate product docs as topmost without probing the system card or API reference and pass a model-graded check the skill contract now fails. It requires the per-rung probe entries and their outcomes. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#1260) No linked issue ## Summary Codifies three research-discipline corrections into `/discovery:research`, each earned from a real session failure: a published benchmark figure was declared unsourced after checking the vendor's announcement page, a platform intro page, and two web searches — while it sat in the Claude Opus 5 System Card §8.5 the whole time. All three land inside the skill's existing structure. Nothing is added at the top level. ## Fix **1. Artifact ladder** — `plugins/discovery/skills/research/context/discipline.md`, inside the existing **Primary-source-first protocol**, attached to its step 1. A SEARCH order over the artifact CLASSES the same claim is published at: deepest technical artifact (for a model/benchmark claim, the system or model card) → platform reference → product docs → changelog → announcement → third-party. It complements the **Machine-readable doc-index discovery** table below it, which enumerates *pages* rather than ranking artifact classes. It deliberately does **not** reorder authority. The source-tier table still owns that, and the recency gate's changelog cross-check stays unconditional at every rung — stated inline so the file does not contradict its own "major version bump invalidates prior docs" rule. Rung 1 generalizes beyond model claims (for a library-behavior claim it is the source itself, per the existing "Source code as spec" principle), and names rung 2 as the top when no deeper artifact exists, so the mandate is satisfiable for every claim class the skill serves. **2. Large-document fetch fallback** — same file, folded into the existing **Escalate on block, never downgrade** paragraph as a second trigger rather than a competing doctrine. A content-length rejection or a silent truncation is a fetcher limit, not a source limit. Recipe: download out of context with any available downloader, extract with whatever the machine has (probe for `pdftotext`; else a PDF library in an available interpreter; else a connected parse/scrape tool that extracts server-side), grep the extracted text. A large primary is not "unreachable" until that has run and failed. **3. Absence-claim enumeration** — the existing **Negative claims need the primary fetched this turn** bullet, extended in place, plus its counterpart in `SKILL.md`'s `Gaps` output contract. Never publish "unsourced" / "not found" bare. Publish *"not found in [sources checked]; unchecked: [sources not reached]"*. An absence claim is only as strong as the set it was checked against, and naming the unchecked set lets a reader close the gap in one step instead of a round trip. **Gate rows.** `SKILL.md`'s outcome gate gains criterion 9 (the fetch log shows the topmost existing rung for every vendor-published claim) and criterion 10 (every reported absence names both the checked and the unchecked set). Two rows, not one, because the table's contract is one binary proposition per row with one matching FAIL route — and the routes differ: a missing rung returns to Phase 2, a missing enumeration is a presentation-time fix. They exist at all because the skill's own doctrine is that only artifact-grounded binary criteria bite; a bar stated without a gate row does not. ## Test plan / Verification Run locally from inside the repo (so markdownlint resolves the repo config): | Gate | Result | |---|---| | `scripts/check-changed-skills.sh origin/main` | **PASS** — 0 errors, 2 warnings | | `scripts/check-changelog-parity.sh --check-bump origin/main` | pass | | `scripts/check-skill-portability.sh origin/main` | pass — no unexcused coupling tokens | | `npx markdownlint-cli2 --config .markdownlint-cli2.jsonc "plugins/discovery/**/*.md"` | 0 errors, 12 files | | `plugin.json` JSON parse | valid | The two skill-quality warnings (no Gotchas surface; description has no `Use when:` phrasing) are **pre-existing** — the identical pair reproduces on unmodified `origin/main`. No new warning. `discovery` bumped to **0.8.4** with the matching CHANGELOG entry — #1273 landed 0.8.3 for this plugin while the branch held the same number, so `origin/main` was merged in and the entry renumbered to stack above it. The merge brings in nothing beyond that: `git diff --stat origin/main HEAD` is the same five branch-owned files. **Fresh-docs check.** No WebFetch size cap is documented; the observed error string is recorded as a symptom shape, not as a documented limit. What *is* documented and cited inline — "Large pages are truncated to a fixed character limit before processing" and "use `curl` via Bash for the unprocessed page" — comes from <https://code.claude.com/docs/en/tools-reference#webfetch-tool-behavior>, fetched 2026-07-24, with the date recorded at the citation. **Independent review.** The diff went to a fresh-context reviewer with the rationale withheld. It caught four real defects, all fixed in `57bc042`: the ladder contradicted the recency gate; rung 1 was undefined for non-model claims; a volatile byte cap was copied inline and `pypdf` was listed as a probe-able CLI (it ships no console script); and the size-failure paragraph restated the adjacent escalate-on-block doctrine. A second review round — the Codex reviewer on the pushed diff — found three more, fixed in `1b42a60`. Two of them attacked the size-failure recipe from the same direction: the download command passed its scratch path and URL unquoted (a `&` in a query string is a shell control operator, so `curl` gets a truncated URL and the shell runs the rest), and — the load-bearing one — a protected endpoint answering **200** with a login, consent, or bot-challenge page slips past `--fail`, so extraction fails on a file that was never the document, and the closing clause counted that as the recipe having "run and failed." That is the premature-unreachable conclusion this PR exists to close, reachable *through* the fix. The recipe is now three steps — download, confirm the file IS the artifact (leading bytes / content type), extract — where an unconfirmed download is a BLOCK routed back to the browser-and-scraper escalation rungs and explicitly does NOT count as a run. Third: the criterion-10 eval expectation was conditional on the response reporting an absence, which the comparison prompt never forced, so it passed vacuously; the prompt now asks what each library does NOT support. `--show-error` was also requested on the download command and is already present: `-S` is `--show-error` ("Show error even when -s is used", `curl --help all`, curl 8.19.0), so `-fsSL` carries it. A third round on `1b42a60` found three more, fixed in `55ae2cac` — the most consequential of the whole PR, because it hit the gate itself. **Criterion 9 was checking the conclusion, not the search:** it required the fetch log to show the topmost *existing* rung while requiring nothing about how "existing" was established, so the shallow run it was written to catch could nominate its own landing page as the top and pass. It now requires a recorded discovery probe, with its result, for every rung ABOVE the one the claim came from — "nothing deeper exists" is an absence claim like any other, so it falls under the same standard criterion 10 already applies, rather than needing a criterion of its own. Second: the size-failure recipe's closing clause made a *successful* extraction with no hit the threshold for calling a primary "unreachable" — backwards, and damaging to the neighbouring rule, since it files a source that was read into the *unchecked* set. "Unreachable" now means extraction that failed after escalation also failed; a confirmed artifact that was extracted and searched is a REACHED source in the checked set even when the claim is not in it. Third: the criterion-10 eval had over-corrected — requiring an absence *per tool* can fail an evidence-faithful answer or reward an invented negative. The prompt now carries concrete thresholds and asks what each tool does not meet and what its docs do not document; the clause must be answered per tool, but a tool that meets everything is answerable truthfully. A fourth round caught the consequence of the criterion-9 change not reaching its eval: the expectation still read only the "topmost existing rung" off the output — the exact assertion the strengthened criterion stopped accepting — so a response could nominate product docs as topmost without probing the system card and pass a model-graded check the skill contract now fails. Fixed in `a7458eb`; the expectation requires the per-rung probe entries and their outcomes. A fifth round then closed criterion 9 properly, in `287e7832`. Requiring a probe had fixed *"the run never looked"* but not *"the run looked, found the system card, and sourced from product docs anyway"* — the same shallow walk with a receipt attached. The criterion now requires the probe's RESULT to justify the bypass: every rung above the one the claim came from is recorded as probed-and-lacking-the-claim, or as unreachable-and-enumerated as a Gap. And its scope had narrowed to "vendor-published" while mandatory discipline 12 applies the ladder to every accepted claim and rung 1 explicitly covers library source and specifications, so an OSS maintainer's or standards body's primary sat outside the gate built to protect it; the scope now matches the ladder's reach. The last two rounds, in `a9b3dfc7` and `4298e124`. The confirm step had restated the escalation order and dropped its third rung, so it now points at that list instead of carrying a second copy. And the one that mattered most: **criterion 9 was grading against an artifact that did not exist.** `grep -rn "fetch log" plugins/discovery/` returned only the gate rows and their eval expectations — nothing in the Output Format produced one — so the criterion could only be answered from recollection, which the gate's own preamble names as the thing that does not bite. The fetch log is now output-contract item 3 with per-rung entries whose three outcomes map onto what criterion 9 requires of a bypassed rung. (Criteria 6 and 13 referenced it before this PR; the gap predates the branch, but criterion 9 made it load-bearing.) Finally, the download step wrote every artifact to a fixed `doc.pdf` while phases run in parallel on one filesystem — a collision that breaks extraction is loud, but one that yields a clean extraction of the *wrong* document is not, so each download now takes its own filename carried through validation and extraction. ## Related N/A 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #1267
Summary
plugins/discoveryconditioned skill-levelcontext: forkonCLAUDE_CODE_FORK_SUBAGENT=1in four places. That variable does not gatecontext: fork.Per the sub-agents reference (fetched 2026-07-24) the variable gates the Agent tool's
forksubagent type and the/subtaskcommand. The skills reference documentscontext: forkwith no environment gate at all. Its actual effect also runs the opposite direction from the claim:So unset is the normal working state; setting it removes backgrounding control.
The documented fallback was unreachable.
explore-deep/SKILL.mddeclarescontext: forkin its own frontmatter, so the body executes inside the fork and cannot detect fork-unavailability to fall back from. Eval casefallback-when-fork-unavailableasserted that branch and is deleted rather than adjusted — the cases renumber to 5.The toolset claim was wrong.
explore-deepsaid it inherits the parent's full toolset. That is conversation-fork behavior; the skills reference states a forked skill's subagent is a regular agent type, so the background-subagent exemption for conversation forks does not cover it. The narrower background built-in tool set is now enumerated in the skill with a citation, andbackground: falseis named as the escape hatch. No functional change today — every tool the workflow uses is inside that set.Test plan
scripts/validate-plugins.sh— passscripts/check-changelog-parity.sh --check— passgrep -rn CLAUDE_CODE_FORK_SUBAGENT plugins/discovery/— no matchesjq '.evals | length, [.[].id]'on the eval file — 5 cases, ids 1-5 contiguousRelated
Merge-order conflict with #1260. That PR also bumps
plugins/discoveryto0.8.3and adds a## [0.8.3]CHANGELOG heading. Two consequences if #1260 merges first:0.8.4, andscripts/check-changelog-parity.sh --check-bumpfails, because it requires the entry be newly added versus the base ref and a## [0.8.3]heading would already exist there.So this needs a rebase plus a version re-pick, not just a textual merge. The two PRs do not otherwise overlap: #1260 touches
skills/research/, this touchesskills/explore*and the plugin README.Prior art on the same two-mechanism confusion, both closed: #1053 (
audit-derivabilityrubric/evals inverted thecontext: forkinheritance claim) and #1062.