fix(discovery): make the by-value fallback reachable, declare the researcher's real tool grant, and echo the input back

[kyle-sexton]

Summary

Three verified HIGH defects in discovery, shipped as one release (0.13.1 → 0.14.0 — new payload fields, a new gate step, and a new frontmatter key are behavior).

1. The by-value fallback was unreachable, and no payload value could reach it

reference/topic-docs.md:34-39 has said since the 2.0.0 contract that "a worker dispatched into its own checkout (worktree or background session) returns findings by value instead, and the parent writes the memory slice." Nothing referenced it as a persistence mode: neither agent, and neither recovery ladder — skills/explore/reference/dispatch.md:78-99 and skills/research/context/dispatch.md:142-172 had rungs for a bad envelope, a live agent, and a refused resume, and none for the worker could not write. Following the rule guaranteed a halt: an empty slice holding only the pre-dispatch baseline is exactly what check-dispatch-artifact.sh:248-251 exits 1 on, and skills/explore/SKILL.md:59 / skills/research/SKILL.md:60 declared any non-zero exit to halt the workflow. Two documents in one plugin, opposite outcomes for the same run, and the correct one unreachable.

No payload value could describe it either. status was complete | truncated; truncated is the turn-budget stop whose ladder consequence is discard the partial slice, and complete requires an artifact: pointer the run had no file to name. Both values misdescribed a run that finished and could not save, and the honest one routed the parent to throw good work away.

Fixed: persistence: written | by-value is now its own payload axis on both agents — truncated keeps meaning the budget stop so the discard rung stays correct, and coverage stays about exploration and the corpus ledger rather than the disk. On the by-value path the agent returns index, sidecars and ledger as verbatim bodies, artifact: names the path the parent must write to (a destination, not a claim a file exists), and both ladders gained the rung ahead of the resume rung: the parent writes the slice, then re-runs the same gate.

The exception is to the halt, not to the gate, and both SKILL.md files say so in those terms. The workflow proceeds only on a subsequent exit 0 — for research, from both the artifact gate and the coverage-ledger gate. persistence: by-value routes the parent and grades nothing; a by-value payload returning findings instead of artifact bodies is a failed dispatch, not a fallback, because a claim the gate is invited to accept on the agent's word is the same laundering the source-tier discipline refuses everywhere else.

This also closes the seam where exit 1 read identically for "never ran" and "ran well, could not persist". The script is right to grade disk state only; the branch belongs one level up, where gate step 1 has already put the payload in the parent's hands.

2. agents/researcher.md described a tool grant it never made

No tools: key and no disallowedTools: key, so it inherited every subagent tool — while its own Tool-honesty section asserted "Edit is absent from your tool list" and "Agent is listed." The paragraph is a verbatim copy from agents/explorer.md, where the allowlist at line 4 makes both sentences true; it was carried into a file whose frontmatter inverts them. Inheritance was observed, not merely derived — the agent's transcript shows ToolSearch and WebFetch calls, neither in the explorer's allowlist. This is an unattended maxTurns: 40 worker whose write boundary is instruction-held, and the false inventory was the calibration input for that boundary.

Fixed: the section states the inherited pool truthfully. No tools: allowlist was added — an allowlist removes every MCP tool ("The subagent can't edit files, write files, or use any MCP tools"), and mandatory discipline #3 requires doc-MCP servers in the tool spread, so the allowlist would break the discipline it was meant to protect. A narrow disallowedTools: denylist is used instead.

Three decisions are now written down rather than accidental:

• NotebookEdit denied — nothing in the contract writes notebooks.
• Edit deliberately kept — research-checklist.md rows go [ ] → [x] per phase, which is Edit-shaped; denying it forces a full-file ledger rewrite at every phase boundary.
• EnterWorktree/ExitWorktree denied, isolation: worktree declined on both agents — these artifacts are graded off disk by the parent, in the parent's checkout, against a slice path resolved before dispatch. Work written into an isolated copy lands where that gate never looks, so the run reads as having produced nothing. Isolation and a disk-graded handoff are incompatible by construction; the plugin chose the handoff. The explorer/researcher asymmetry is now stated as deliberate in both files.

One thing this does not claim. It does not make the write boundary enforceable — you cannot deny "Bash writing a file" without denying Bash, which the research discipline needs. Both agents instead gain an explicit instruction that a refused Write is an answer, not an obstacle, and must not be routed through Bash. That is grounded in the transcript asymmetry it was observed as (three Write calls refused while a Bash-mediated write succeeded to the same directory tree), not in any documented rule about which guard covers which tool.

3. Nothing restated the input, so a corrupted scope/topic passed every gate

Every refusal mechanism was a presence test — token present or MISSING, envelope field present or absent, index on disk or not, ledger rows marked or not, artifact: present or not — so none could fire on an input that arrived present and wrong. Observed 2026-08-10: an argument naming another plugin's ${CLAUDE_PLUGIN_DATA} reached a dispatched agent rewritten to this plugin's own path. The agent was asked a factually wrong question and answered it correctly.

Fixed: both agents echo scope_as_received: / topic_as_received: verbatim (explicitly not paraphrased or normalized), and both acceptance gates compare it against the envelope the parent wrote. A mismatch is a failed dispatch even when the artifact is complete and every mechanical check exits 0; a payload lacking the field is an out-of-date agent definition, not a pass.

The caveat in explore, research and research-deep is observation-shaped, not mechanism-shaped: it states the observation, the two documented facts (skill and agent content is a substitution site for the three ${CLAUDE_*} placeholders "anywhere the placeholder appears"; no escape exists — "A backslash before any other $ is left unchanged"), and the gap (no page states whether argument-supplied text is itself scanned). It carries an unconditional 2027-02-11 expiry. research-deep gets the caveat only — it has no agent and no payload contract of its own.

Cross-ledger adjudications honored

• AD-1 — the researcher tool-honesty defect appears on three ledgers (I1 F2, I9 D-F2, I7 B-F2). Implemented once; all three ledgers' evidence is folded into fix(discovery): agents/researcher.md declares no tools and its Tool-honesty prose describes a grant it never made #2204.
• AD-2 — I1 F1+F3 are one change, and I9's constraint survives it: the value of a third outcome is routing, not acceptance. The payload carries no research fallback the gate then grades; the gate is unchanged and still authoritative.
• AD-3 — I6 RKD-11 says discovery "handles the write block well." I1 F1 governs: that path was graded unreachable by direct inspection and reproduced empirically. Fixed as a HIGH.
• AD-8 — I7 B-F5's evidence (the exit-1 rung dead-ends on a nonexistent index) merged into fix(discovery): the by-value fallback is unreachable and no payload value expresses "complete but unpersisted" #2203 rather than filed separately.
• I1 F4 is rationale_falsified and is not implemented here. Its framing is not reused: the Bash-evasion caution in both agents argues from the transcript asymmetry, never from the worktrees page's isolation check list, which is scoped to a session already isolated in a worktree reaching a main checkout — neither of which the observed run had.

The $ARGUMENTS conflict, adjudicated

The coordinator flagged an unadjudicated tension between I9 D-F6 (the "$ARGUMENTS empties on the preload path" claim, asserted at 5 sites, unsupported by current docs) and I7 B-F1 (this PR's change 3). They are not incompatible — they are about different things, and the appearance of conflict comes from both being described as "$ARGUMENTS handling":

• D-F6 / I1 F8 concern a $ARGUMENTS placeholder the plugin's own body carries, on the preload path.
• B-F1 concerns placeholder-shaped text a caller supplies, on the invocation/inline path and in the dispatch prompt the parent writes.

Both can hold: the skills page treats preload as a distinct path ("Subagents with preloaded skills work differently: the full skill content is injected at startup"). On doc support, B-F1's remediation wins outright — its confirmed halves are the plugins-reference substitution-domain row and the skills-page escape rule, and its unconfirmed half (ordering) does not change the fix, which is correct either way. D-F6's claim is documented nowhere and its nearest analogue (the context: fork walkthrough, showing \$ARGUMENTS arriving unsubstituted) points the other way.

So this PR implements B-F1 and leaves D-F6's five sites untouched — D-F6/F8 are DOC_ONLY and not this lane's rows. To avoid leaving the tension implicit in the tree, each new caveat now states its own boundary: it names the path it covers and says explicitly that it is a different question from the adjacent preload claim and is evidence for neither side of it. Verified mechanically: this diff adds zero new sites restating the empties-on-preload claim (still exactly the 5 pre-existing ones). The echo-back design does not depend on that claim being true or false.

Test plan

Both new tests fail before the change and pass after.

1. The by-value gate pair — scripts/check-dispatch-artifact.test.sh, extended. Asserts the empty by-value slice exits 1 and that the same slice exits 0 once the parent writes it from the payload, with freshness and pointer checks both passing. The pair proves the exception routes through the gate rather than around it. Runs for both artifact families.

$ bash plugins/discovery/scripts/check-dispatch-artifact.test.sh
...
ok   - a slice holding only the dispatch baseline is unusable [EXPLORE.md] (exit 1)
ok   - the by-value end state reports no index at all [EXPLORE.md]
ok   - the same slice is usable once the parent writes it from the payload [EXPLORE.md] (exit 0)
ok   - a parent-written slice passes every check a self-written one does [EXPLORE.md]
ok   - a recovered slice no newer than the baseline is still stale [EXPLORE.md] (exit 1)
...
ok   - a slice holding only the dispatch baseline is unusable [RESEARCH.md] (exit 1)
ok   - the by-value end state reports no index at all [RESEARCH.md]
ok   - the same slice is usable once the parent writes it from the payload [RESEARCH.md] (exit 0)
ok   - a parent-written slice passes every check a self-written one does [RESEARCH.md]
ok   - a recovered slice no newer than the baseline is still stale [RESEARCH.md] (exit 1)
...
all tests passed

2. The agent-contract lock — plugins/discovery/agents/tool-honesty.test.sh, new. Locks the class rather than the instance: prose claiming a tool is absent from (or present in) a tool list must be backed by a tools: key that actually omits (or lists) it; every agent must declare its posture in frontmatter rather than leaving the prose as the only inventory; neither agent may set isolation:; both payload contracts must carry persistence: and the echo-back field. Scoped to this plugin's agents on purpose — a repo-wide sweep would fail this plugin's test on another plugin's drift.

Against HEAD (origin/main) agent definitions, i.e. before the change:

$ bash tool-honesty.test.sh          # run against origin/main's explorer.md + researcher.md
ok   - explorer.md: the claim that `Edit` is absent is backed by the `tools:` allowlist
ok   - explorer.md: the claim that `Agent` is listed is backed by the `tools:` allowlist
ok   - explorer.md: declares its tool posture in frontmatter
ok   - explorer.md: does not set `isolation:`, so its writes land where the gate grades
FAIL - explorer.md: the return payload has no `persistence:` field — a completed run whose write was refused cannot be told apart from one that never ran
FAIL - explorer.md: the return payload has no `scope_as_received:`/`topic_as_received:` field — a corrupted input passes every gate
FAIL - researcher.md: prose says `Edit` is absent from its tool list, but no `tools:` key is declared — with no allowlist the pool is inherited and `Edit` is held
FAIL - researcher.md: prose says `Agent` is listed, but no `tools:` key is declared — it is inherited, not listed
FAIL - researcher.md: declares neither `tools:` nor `disallowedTools:` — the inherited pool is then undeclared, and the prose becomes the only inventory
ok   - researcher.md: does not set `isolation:`, so its writes land where the gate grades
FAIL - researcher.md: the return payload has no `persistence:` field — a completed run whose write was refused cannot be told apart from one that never ran
FAIL - researcher.md: the return payload has no `scope_as_received:`/`topic_as_received:` field — a corrupted input passes every gate

7 test(s) failed
---exit:1---

After the change:

$ bash plugins/discovery/agents/tool-honesty.test.sh
ok   - explorer.md: the claim that `Edit` is absent is backed by the `tools:` allowlist
ok   - explorer.md: the claim that `Agent` is listed is backed by the `tools:` allowlist
ok   - explorer.md: declares its tool posture in frontmatter
ok   - explorer.md: does not set `isolation:`, so its writes land where the gate grades
ok   - explorer.md: the return payload carries a `persistence:` axis
ok   - explorer.md: the return payload echoes back the scope/topic as received
ok   - researcher.md: declares its tool posture in frontmatter
ok   - researcher.md: does not set `isolation:`, so its writes land where the gate grades
ok   - researcher.md: the return payload carries a `persistence:` axis
ok   - researcher.md: the return payload echoes back the scope/topic as received

all tests passed

3. Every test in the plugin

$ for t in $(find plugins/discovery -name '*.test.sh' | sort); do ...; done
=== plugins/discovery/agents/tool-honesty.test.sh
PASS
=== plugins/discovery/scripts/check-coverage-complete.test.sh
PASS
=== plugins/discovery/scripts/check-dispatch-artifact.test.sh
PASS

4. Lint, format, changelog parity, JSON validity

$ shellcheck --rcfile=.shellcheckrc plugins/discovery/agents/tool-honesty.test.sh plugins/discovery/scripts/check-dispatch-artifact.test.sh
shellcheck exit:0
$ shfmt -d plugins/discovery/agents/tool-honesty.test.sh plugins/discovery/scripts/check-dispatch-artifact.test.sh
shfmt exit:0
$ npx markdownlint-cli2 "plugins/discovery/**/*.md"
Linting: 18 files
Summary: 0 issues in 0 files
$ bash scripts/check-changelog-parity.sh --check
Every versioned plugin has a CHANGELOG.md (or a stale-guarded baseline entry), and none documents a version above its manifest.
$ bash scripts/check-changelog-parity.sh --check-order
All 75 changelog(s) read newest-first with no duplicate versions.
$ python -c "import json; [json.load(open(p)) for p in (...)]"
valid json: plugins/discovery/skills/explore/evals/evals.json
valid json: plugins/discovery/skills/research/evals/evals.json
valid json: plugins/discovery/.claude-plugin/plugin.json

Eval cases added on both skills covering the outcomes the new axis must keep apart: a by-value recovery that is written and re-graded rather than discarded; a by-value payload carrying findings instead of artifact bodies (a failed dispatch); an echo-back mismatch that fails a dispatch whose artifact is otherwise perfect; a by-value payload naming a file outside the contract (rejected before anything is written); an explore-side recovery into a slice root already holding an unrelated index; and a research-side recovery of an unbounded corpus, which owes no ledger and must not be graded against one.

Re-run green after every follow-up commit: tool-honesty.test.sh, check-dispatch-artifact.test.sh, check-coverage-complete.test.sh, markdownlint-cli2 over all 18 plugin markdown files, shellcheck --rcfile=.shellcheckrc, shfmt -d, check-shell-portability.sh origin/main, and eval-JSON validity with unique ids.

Security note

Revised after review — my first version of this section was wrong, and the correction is in the diff. I wrote "no trust surface widens." One does. The by-value rung is the only place in this contract where a filename the worker produced becomes a write the parent performs, and the parent holds wider write permission than the sandboxed worker — while the researcher spends its entire run ingesting untrusted third-party pages. A payload it emits is not a trusted source of paths, and the first draft anchored the directory safely while leaving the filenames unchecked.

Fixed in 33e368f: both ladders check filenames before anything reaches disk, accept only the contract's own names as bare filenames (EXPLORE.md / EXPLORE-<section>.md; RESEARCH.md / RESEARCH-<section>.md / research-checklist.md), confirm the resolved write path stays directly inside the destination directory, and treat a directory separator, a .. segment or a leading / as a failed dispatch rather than a name to sanitize. Both agents carry the same rule from their side, with the reason. Eval cases on both skills assert nothing is written — including the well-formed bodies — when one name is out of contract.

Otherwise: the only grant-shaped change is a disallowedTools: denylist, which narrows the researcher's inherited pool. No new hook, no new external read, no new script invocation. And the honest residual, stated in the diff rather than only here: the memory-tier write boundary was already instruction-held and remains so — this PR stops the file from understating what it holds, which is the direction that matters for least privilege.

Review follow-ups (all in this PR)

Four gaps found by review, all in text this PR added, and three of them the same shape the PR exists to close — a new rule contradicting an existing one two files away. All seven review threads are answered and resolved.

1. Collision rule vs the by-value path (33e368f). The explorer's rule sends a slice root already holding an unrelated EXPLORE.md to a sub-slice. The by-value text contradicted it on both sides — the agent was told artifact: is the envelope path unconditionally, the parent to write into the resolved slice path unconditionally — which would have overwritten the index that rule protects, silently and unrecoverably, through the recovery path. All three sites now apply the rule; the parent assigns the sub-slice, as it assigns every other one.
2. Unbounded corpus vs the coverage gate (33e368f). Phase 0 writes no research-checklist.md for an unbounded corpus, but the by-value instructions listed it unconditionally and the ladder re-ran check-coverage-complete.sh "over the ledger the payload supplied" unconditionally. A missing ledger exits 2, which SKILL.md states is a FAIL — so a legitimately complete unbounded-corpus run would have halted on a check that never applied to it. The ledger is now returned and graded only when one was owed, citing the standing rule unchanged.
3. Filename validation (33e368f) — see the security note above.
4. Eval coverage (33e368f). The unbounded-corpus + by-value combination that (2) falls into was uncovered (eval 13 uses a bounded corpus). Added, plus the collision case and a filename-rejection case on both skills.

Plus one nit: disallowedTools is now quoted (789799a), matching every other tools:-bearing agent in the repo. The docs' own example is unquoted and YAML parses both identically, so this is consistency, not correctness.

Related

Closes #2203
Closes #2204
Closes #2205

Inbox items: 20260810-225904-discovery-dispatch-persistence-contract (I1 F1, F2, F3, with F10 closing as a side effect) · 2026-08-10-plugin-quality-audit-four-components 021645Z (I9 D-F1 deferred here, D-F2 merged) · four-components 023241Z (I7 B-F1 implemented, B-F2/B-F5 merged as duplicates).

Evidence sourced from the packets' evidence-1.md, which governs over evidence.md on conflict.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 5m 40s —— View job

---

Review complete

Todo list

• Read repo CLAUDE.md / REVIEW.md guidelines
• Get PR diff (git diff origin/main...HEAD)
• Review agents/explorer.md and agents/researcher.md changes
• Review dispatch reference docs (skills/explore/reference/dispatch.md, skills/research/context/dispatch.md, reference/topic-docs.md)
• Review SKILL.md changes (explore, research, research-deep)
• Review test scripts (tool-honesty.test.sh, check-dispatch-artifact.test.sh)
• Review evals.json changes
• Review CHANGELOG/plugin.json version bump
• Post inline comments for findings
• Post final summary

Scope note

.github/workflows/claude-security-review.yml exists in this repo, so per REVIEW.md's lane split I scoped this review to correctness, design, error handling, tests, and docs — not security. No security-relevant surface changed here anyway (frontmatter change is a disallowedTools: denylist narrowing the researcher's inherited pool, nothing widens).

What I checked

I read every changed file in full (not just the diff hunks) and traced the cross-file contract end to end: the new persistence: written | by-value payload axis and scope_as_received/topic_as_received echo-back fields in agents/explorer.md and agents/researcher.md, the matching gate/ladder changes in both SKILL.md files and both reference/context dispatch.md files, the topic-docs.md cross-reference, and the two test scripts (agents/tool-honesty.test.sh, scripts/check-dispatch-artifact.test.sh).

I traced the recovery-ladder logic by hand: the "by-value" rung is reachable (exit 1 + persistence: by-value in the payload, which check-dispatch-artifact.sh — unmodified by this PR — genuinely produces when the slice holds only the pre-dispatch baseline), the parent-writes-then-regrade step routes back through the same unmodified gate script rather than around it, and the --newer-than freshness check is satisfied by construction since the parent's write happens strictly after its own pre-dispatch touch. I also confirmed check-dispatch-artifact.sh itself is untouched — the new tests exercise real, existing script behavior, not a mocked one.

I hand-verified the new tool-honesty.test.sh logic against both agent files: the claims_about sentence-shape matcher only fires where intended (explorer.md's "is absent from your tool list" / "is listed" claims are backed by its unchanged tools: allowlist; researcher.md's rewritten prose no longer makes either claim, so those two checks correctly produce zero assertions for it, matching the PR's shown "before/after" output shape). I also confirmed no other file in the plugin restates the old payload schema in a way this PR left stale (grep-swept every file mentioning preload_token/artifact:/status: complete).

I was not able to execute bash plugins/discovery/agents/tool-honesty.test.sh, check-dispatch-artifact.test.sh, or the JSON validity/shellcheck/markdownlint commands myself — script execution required approval this sandboxed session doesn't have (only read-only git/grep/file-read style commands ran). So the Test Plan's shown pass/fail output in the PR description is author-claimed and unverified by me; my confidence in the tests is from static reading of the shell logic and the gate script, not from running them.

Findings

One low-severity nit, posted inline: plugins/discovery/agents/researcher.md#L6 — the new disallowedTools: value is unquoted, while every other tools:/disallowedTools: value in the repo is a quoted string (e.g. explorer.md#L4). Not a functional bug — YAML parses it identically either way — just a style inconsistency.

No correctness bugs found. The cross-document contract (agent payload ↔ SKILL.md gate ↔ dispatch.md recovery ladder ↔ topic-docs.md rationale ↔ tests) is internally consistent everywhere I traced it, including the subtler points: status/persistence/coverage are kept as genuinely independent axes rather than overloading one field, the by-value rung is correctly ordered before the resume rung in both ladders (since resuming asks the worker to retry a write it already proved impossible), and the "findings-instead-of-artifact-bodies is a failed dispatch, not a fallback" rule is stated and tested identically on both the explore and research sides.
· Branch

[claude]

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

---

Security Review — PR #2222

Todo list

• Gather context (PR diff, changed files)
• Read all changed files in detail
• Review agent tool-grant / permission changes (explorer.md, researcher.md) for authorization gaps
• Review dispatch/persistence logic (dispatch.md, SKILL.md files) for injection or trust-boundary issues
• Review shell test scripts for injection/unsafe handling
• Review plugin.json / CHANGELOG for permission-widening
• Post final findings

Scope: reviewed only the 14 files changed in this PR (git diff origin/main...HEAD). These are Markdown agent/skill definitions and two test scripts for the discovery plugin — no GitHub Actions workflow files are touched, so no zizmor-covered findings apply here.

---

Findings

IMPORTANT — new by-value recovery path writes agent-supplied filenames with no path-traversal check (confidence: medium)

This PR introduces a new "persistence: by-value" recovery mode: when a dispatched explorer/researcher subagent finishes work but every Write attempt is refused, it returns the full artifact bodies verbatim in its payload, and the parent session (not the sandboxed subagent) is instructed to write them to disk itself:

• agents/researcher.md#L214-L216 — "Append the artifact bodies verbatim ... each in its own fenced block introduced by the filename it belongs in"
• agents/explorer.md#L212-L213 — same pattern
• skills/research/context/dispatch.md#L161-L165 — "Write them into the memory-slice path the parent resolved before dispatch ... under those names"
• skills/explore/reference/dispatch.md#L96-L99 — same pattern

The base directory is anchored safely (it's the parent's own pre-dispatch-resolved slice path, not something the agent controls). But the individual filenames ("RESEARCH.md first, then every sidecar ... then research-checklist.md" / "the index first, then every sidecar") come from the payload the subagent returns, and none of these four documents instructs the parent to validate that a filename is one of the fixed/expected names (or matches the EXPLORE-<section>.md / RESEARCH-<section>.md sidecar pattern) or that the resolved path stays inside the slice directory before writing.

Why this matters here specifically: discovery:researcher carries Bash, WebFetch, WebSearch, and the full MCP pool, and its entire job is to ingest untrusted external content (fetched web pages, gh api/curl output). If adversarial content encountered during a research run manages to influence the agent's return payload (prompt injection is exactly the failure mode issue #2205/echo-back in this same PR is defending against for the scope/topic fields — the same defense wasn't extended to the sidecar filenames), the payload could name a sidecar with a traversal-shaped filename (e.g. containing ../) and have the parent — which typically has broader write privileges than the sandboxed subagent — write attacker-supplied content outside the memory slice (e.g. over a hook script, CLAUDE.md, or another plugin's files) via nothing more than prose instructions with no stated filename sanity check.

This is a genuinely new trust-boundary shift introduced by this PR: previously the only write path was the subagent's own (already instruction-only-bounded) Write tool call; now the contract explicitly routes a subagent-controlled filename to be written by the higher-privileged parent on the strength of return-payload text alone.

Suggested mitigation: add an explicit instruction (mirroring the new scope_as_received/topic_as_received echo-back discipline) that the parent must reject any by-value body whose filename isn't exactly RESEARCH.md/EXPLORE.md/research-checklist.md or doesn't match the sidecar pattern ^(RESEARCH|EXPLORE)-[A-Za-z0-9_-]+\.md$, and must verify the resolved write path stays lexically inside the pre-resolved slice directory before writing — treating any other shape as a failed dispatch, the same way a findings-instead-of-artifact-bodies payload already is.

Fix this →

---

Not flagged (reviewed, no issue found)

• The disallowedTools: NotebookEdit, EnterWorktree, ExitWorktree addition on researcher.md is a genuine narrowing, consistent with the PR's stated intent — no permission widening.
• The residual gap that the researcher's memory-tier write boundary (don't write outside the slice, don't route a refused Write through Bash) remains instruction-held rather than technically enforced is openly disclosed by the PR author in the "Security note" section and isn't newly introduced by this diff — not re-reported here.
• tool-honesty.test.sh and the check-dispatch-artifact.test.sh additions operate only on local repo files with fixed test fixtures; no untrusted input, no eval/command substitution of external data.
• plugin.json change is a version bump only; no manifest permission/config changes.
• No GitHub Actions workflow files are touched by this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 456a82c24d

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

[claude]

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

---

Security Review — PR #2222

Todo list

• Gather context (PR diff since last review, changed files)
• Review what changed since the last security review (commit 456a82c → 3581f81)
• Re-verify the previously reported by-value filename path-traversal finding against current HEAD
• Check new review threads from codex (collision sub-slice, conditional ledger) for security implications
• Post final findings

---

What changed since the last review

The only commit added since the previous security review (456a82c) is 3581f81, which changes only the executable bit on plugins/discovery/agents/tool-honesty.test.sh (git diff 456a82c 3581f81 --stat → 1 file changed, 0 insertions/deletions — mode-only). No file content changed, so this re-review reassesses the same diff (origin/main...HEAD, 14 files, +699/-22) against current HEAD 3581f81093595fe1a6a019b0b51c1428ebf7dac5.

Scope: only the 14 files changed in this PR. No GitHub Actions workflow files are touched, so no permissions:/trigger/zizmor-covered findings apply.

---

Findings

IMPORTANT — new by-value recovery path instructs the parent to write agent-supplied filenames with no path-traversal / allow-list check (confidence: medium)

Confirmed still present, unchanged, at current HEAD. This PR adds a persistence: by-value recovery mode: when a dispatched explorer/researcher subagent finishes but every Write attempt is refused, it returns full artifact bodies verbatim in its return payload, and the parent session (not the sandboxed subagent) is instructed to write them to disk:

• agents/researcher.md#L214-L216 — "Append the artifact bodies verbatim ... each in its own fenced block introduced by the filename it belongs in"
• agents/explorer.md#L212-L213 — same pattern
• skills/research/context/dispatch.md#L161-L165 — "Write them into the memory-slice path the parent resolved before dispatch ... under those names"
• skills/explore/reference/dispatch.md#L96-L99 — same pattern

The base directory is anchored safely (the parent's own pre-dispatch-resolved slice path — not agent-controlled). But the individual filenames (RESEARCH.md / index first, then sidecars, then research-checklist.md) come from the by-value payload the subagent returns, and none of these four documents instructs the parent to validate that a filename matches the fixed/expected set (or the EXPLORE-<section>.md / RESEARCH-<section>.md sidecar pattern), or that the resolved path stays inside the slice directory, before writing.

Why this matters specifically here: discovery:researcher carries Bash, WebFetch, WebSearch, and the full inherited MCP pool, and its job is to ingest untrusted external content (fetched web pages, gh api/curl output). If adversarial content encountered mid-run manages to influence the agent's return payload — prompt injection, exactly the failure mode issue #2205 / the new topic_as_received/scope_as_received echo-back in this same PR is defending against for the scope/topic fields, but that defense wasn't extended to the sidecar filenames — the payload could name a body with a traversal-shaped filename (e.g. containing ../), and have the parent (typically higher write-privilege than the sandboxed subagent) write attacker-influenced content outside the memory slice, on nothing more than prose instructions with no stated filename sanity check.

This is a genuine new trust-boundary shift: previously the only write path was the subagent's own instruction-bounded Write tool call; now the contract explicitly routes a subagent-controlled filename to be written by the higher-privileged parent based on return-payload text alone.

Suggested mitigation: add an explicit instruction (mirroring the scope_as_received/topic_as_received echo-back discipline already in this PR) that the parent must reject any by-value body whose filename isn't exactly one of the fixed names or ^(RESEARCH|EXPLORE)-[A-Za-z0-9_-]+\.md$, and must confirm the resolved write path stays lexically inside the pre-resolved slice directory — treating any other shape as a failed dispatch, the same way a findings-instead-of-artifact-bodies payload already is treated.

---

Not flagged (reviewed, no security issue)

• The two Codex review comments on this PR (skills/explore/SKILL.md:63 on collision sub-slice preservation, skills/research/SKILL.md:66 on the unbounded-corpus ledger requirement) are correctness/data-integrity issues with non-adversarial causes (legitimate scope-naming collisions, legitimate unbounded-topic runs) — not security vulnerabilities, so out of scope for this review.
• disallowedTools: NotebookEdit, EnterWorktree, ExitWorktree on researcher.md narrows the inherited tool pool — no permission widening.
• The residual gap that the researcher's write boundary (don't write outside the slice, don't route a refused Write through Bash) is instruction-held rather than mechanically enforced is openly disclosed in the PR's own "Security note" and isn't newly introduced by this diff.
• tool-honesty.test.sh and the check-dispatch-artifact.test.sh additions operate only on local repo files with fixed test fixtures — no untrusted input, no eval/command substitution of external data.
• plugin.json change is a version bump only. CHANGELOG.md and evals.json changes are documentation/eval-fixture only.
• No GitHub Actions workflow files are touched by this PR.

[github-actions]

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

[claude]

Claude finished @kyle-sexton's task in 5m 7s —— View job

---

Code Review — PR #2222 (correctness / architecture / maintainability lane)

Per this repo's REVIEW.md, security findings are out of scope for this lane (a dedicated claude-security-review workflow exists and already posted its own review above); this review covers correctness, design, error handling, and test coverage only.

Todo list

• Gather context (PR diff, changed files)
• Read repo CLAUDE.md / REVIEW.md for guidelines and lane scoping
• Read all changed files in detail
• Review dispatch/persistence contract logic for correctness (dispatch.md, SKILL.md files, agent payload contracts)
• Review test coverage (tool-honesty.test.sh, check-dispatch-artifact.test.sh, evals.json)
• Cross-check against the two existing Codex review comments (collision sub-slice, unbounded-topic ledger)
• Post inline findings + summary

Findings (posted as inline comments)

I independently traced both issues the automated Codex review flagged on the SKILL.md files down into the rest of the contract, and found the same defects duplicated — meaning a fix scoped only to SKILL.md would leave the plugin internally inconsistent again, which is the exact failure shape ("two documents in one plugin, opposite outcomes") this PR's own defect #1 was written to close.

1. agents/explorer.md:206-212 — the persistence: by-value section says artifact: carries "the index path from your dispatch envelope," unconditionally, but lines 125-132 of the same file require a collision with a pre-existing unrelated EXPLORE.md to be resolved by writing into a sub-slice and naming that path instead. The two rules disagree on what an agent that hit both conditions should report.
2. skills/explore/reference/dispatch.md:96-99 — the parent-side recovery ladder has the matching bug (same wording as the already-flagged SKILL.md:63): it writes the recovered artifact to "the memory-slice path the parent resolved before dispatch," unconditionally, which would silently overwrite the unrelated index the collision rule exists to protect.
3. agents/researcher.md:214-219 — the by-value payload instructions unconditionally include research-checklist.md among the bodies to append, but Phase 0 (SKILL.md:58/:105) writes no ledger file at all for an unbounded-corpus topic.
4. skills/research/context/dispatch.md:161-171 — matching parent-side bug (same defect flagged on SKILL.md:66): unconditionally re-runs check-coverage-complete.sh "over the ledger the payload supplied." I checked the script itself — a missing ledger file exits 2, which SKILL.md:202 confirms is always a FAIL — so a legitimately complete unbounded-corpus run hitting the by-value rung would halt on a check that was never supposed to run.

Both are genuine logic gaps in the new contract text, not stylistic nits — verified by reading the actual script behavior and the conflicting prose myself rather than restating the bot's comments. Details and suggested fixes are in the inline comments.

Test coverage note (not filed inline — no single line to anchor it to)

Neither evals.json addition exercises the unbounded-corpus + persistence: by-value combination that finding #3/#4 above falls into — eval 13 in skills/research/evals/evals.json uses a bounded corpus (3 sidecars + a ledger) for its by-value recovery case. An eval covering "unbounded topic, every write refused" would have caught this gap before merge.

What looks solid

• The tool-honesty.test.sh class-based lock (matching prose shape like `X` is absent from your tool list rather than hardcoding tool names) is a good design choice — it stays useful if the prose is edited later, and it correctly catches both polarities (false "absent" and false "listed" claims).
• The check-dispatch-artifact.test.sh by-value pair is well-constructed: it proves both that the failure state genuinely exits 1 (the exception is needed) and that recovery earns exit 0 through the same gate command including freshness (the exception routes through the gate, not around it) — reusing the existing future_baseline fixture correctly for the added staleness case.
• The persistence/status/coverage axis separation is a sound design fix for the original problem (an unreachable fallback + no payload value able to describe "finished but couldn't save").
• CHANGELOG and version bump (0.13.1 → 0.14.0) are consistent with the behavior change.

[claude]

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

---

Security Review — PR #2222

Todo list

• Gather context (PR diff since last security review, changed files)
• Review what changed in commit 33e368f ("close four gaps") for security implications
• Re-verify the previously reported by-value filename path-traversal finding against current HEAD
• Review the rest of the diff fresh for new security issues
• Post final findings

What changed since the last security review

The previous two security reviews (on 456a82c and 3581f81) flagged one IMPORTANT finding: the new persistence: by-value recovery path had the parent write agent-supplied filenames into the memory-slice directory with no stated allow-list or path-traversal check. Since then, commit 33e368f ("close four gaps review found in the new by-value rung") was pushed, touching agents/explorer.md, agents/researcher.md, both reference/context dispatch.md files, both SKILL.md files, CHANGELOG.md, and both evals.json files. I diffed 456a82c...33e368f directly and re-read every changed section in full.

Previously reported finding: RESOLVED

The path-traversal / filename allow-list gap is fixed on both the agent side and the parent side, symmetrically for discovery:explorer and discovery:researcher:

• Agent side (agents/explorer.md:209-212, agents/researcher.md:224-230) — the agent is now instructed to name only the contract's fixed files (EXPLORE.md/EXPLORE-<section>.md or RESEARCH.md/RESEARCH-<section>.md/research-checklist.md), as bare filenames, explicitly "no directory component, no .., no leading /."
• Parent side (skills/explore/reference/dispatch.md:93-102, skills/research/context/dispatch.md:158-168) — this is the half that actually closes the gap, since the agent is the untrusted party: the recovery ladder now requires the parent to "check every filename before writing anything," accept exactly the fixed names via an explicit regex (^EXPLORE-[A-Za-z0-9_-]+\.md$ / ^RESEARCH-[A-Za-z0-9_-]+\.md$ — the character classes contain no path separators, so no traversal-shaped value can match), reject a directory separator/../leading / as a failed dispatch (not silently sanitized), and confirm the resolved write path stays inside the destination directory. This is stated as step 1, before any write happens.
• Both SKILL.md files carry the same rule in their inline recovery-ladder summary (skills/explore/SKILL.md:63, skills/research/SKILL.md:66), so it isn't only in the reference doc a reader might skip.
• New eval fixtures directly exercise the rejection path: skills/explore/evals/evals.json id 12 and skills/research/evals/evals.json id 17 both supply a by-value payload naming '../../../.claude/settings.local.json' alongside two well-formed bodies, and require nothing be written (including the well-formed bodies) once the check fails.
• The sub-slice destination used for the collision case (agents/explorer.md:125-132, pre-existing/unmodified by this PR) is still parent-chosen, not agent-supplied — the by-value ladder's step 2 explicitly reads the payload's artifact: as "the destination the agent names, never the anchor," so the collision fix doesn't reopen a path the filename check just closed.

I consider this adequately mitigated for a contract expressed as agent instructions (i.e., there's no way to mechanically enforce this in a system with no code path here, only prose) — the fix follows the same shape as the scope_as_received/topic_as_received echo-back defense elsewhere in this PR, is present on both agents and both dispatch ladders, and has eval coverage for the rejection behavior. As before, I'll note the same caveat the PR's own "Security note" already discloses: this remains instruction-held, not technically enforced (no code executes here — it's markdown consumed by an LLM parent), so it's a defense against the failure mode being specified, not one that's unbypassable. That limitation is inherent to the whole contract (also true of the tool-honesty and write-boundary rules already in this PR) and isn't a new gap introduced by this fix.

Rest of the diff (fresh review)

No other file changed between the last review and current HEAD carries security-relevant content:

• CHANGELOG.md — documentation only, describes the same fix.
• Both evals.json files — new eval fixtures (ids 11/12 explore, 16/17 research), no executable code, no untrusted input path.
• tool-honesty.test.sh — this commit only changes its file mode (executable bit); content is unchanged from the prior review.
• reference/topic-docs.md, skills/research-deep/SKILL.md, .claude-plugin/plugin.json — untouched since the prior two security reviews, which found no issues in them.

Conclusion

No outstanding or new security issues. The previously reported IMPORTANT finding (by-value recovery path writing agent-supplied filenames with no traversal check) has been fixed with a filename allow-list enforced on both the agent-instruction side and, more importantly, the parent-instruction side, backed by eval coverage.
· Branch

[claude]

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

---

Security Review — PR #2222 (re-run at HEAD 789799a)

Todo list

• Gather context (PR diff since last security review)
• Diff 3581f81 → 789799a (the two commits since the last security review) for security-relevant content
• Re-verify the previously reported by-value filename path-traversal finding at current HEAD
• Check for any new security-relevant surface in this synchronize
• Post final findings

---

What changed since the last security review

Two commits landed since the prior security review (against 3581f81):

• 33e368f — fixes the by-value filename path-traversal finding this lane reported twice, plus the correctness gaps the correctness-lane review and Codex found (collision sub-slice, conditional coverage ledger).
• 789799a — style-only: quotes the disallowedTools: scalar on researcher.md. No behavior change.

Previously reported finding: now fixed

The prior two runs of this lane flagged that the persistence: by-value recovery path let a dispatched explorer/researcher subagent's return payload name arbitrary filenames that the (higher-privileged) parent would then write into the memory-slice directory, with no stated validation — a path-traversal-shaped trust-boundary gap, worst on the researcher side since that agent's whole job is ingesting untrusted web content.

33e368f closes it on both sides of the contract, not just the agent-authoring side:

• Agent definitions (agents/explorer.md#L217-L220, agents/researcher.md#L224-L230) now instruct the agent to name only the contract-defined filenames, bare, no directory component / .. / leading /.
• Critically, the parent-side ladders also gained an independent, enforced check — not just reliance on the (potentially prompt-injected) agent following its own instructions: skills/explore/reference/dispatch.md#L96-L105 and skills/research/context/dispatch.md#L161-L177 instruct: "Check every filename before writing anything... Accept exactly EXPLORE.md and EXPLORE-<section>.md (^EXPLORE-[A-Za-z0-9_-]+\.md$)" / the equivalent RESEARCH.md/RESEARCH-<section>.md/research-checklist.md allow-list for research, reject any name with a directory separator, .., or leading / as a failed dispatch (not sanitized/normalized), and confirm the resolved write path stays directly inside the destination directory.
• The regex allow-lists ([A-Za-z0-9_-]+) admit no path-separator or traversal characters, so this is a real allow-list, not a denylist that could be bypassed by encoding tricks.
• A new eval was added specifically exercising this: skills/explore/evals/evals.json eval 12 sends a body introduced as '../../../.claude/settings.local.json' and asserts the whole payload is rejected before anything is written (including the two otherwise-well-formed bodies in the same payload) — a good adversarial test of exactly the reported gap, not just the happy path.

I consider this finding resolved. No further action needed on it.

New findings at current HEAD

None. The only other change (789799a) is a non-functional YAML quoting fix with no security relevance. I re-checked the two test scripts (tool-honesty.test.sh, check-dispatch-artifact.test.sh) — unchanged since the last review, still operate only on local fixed fixtures with no untrusted input. CHANGELOG.md/evals.json changes are documentation/fixture-only. No GitHub Actions workflow files are touched by this PR.

Not re-flagged (unchanged from prior reviews, still applies)

• disallowedTools: NotebookEdit, EnterWorktree, ExitWorktree on researcher.md narrows the inherited tool pool — no permission widening.
• The residual gap that the researcher's write boundary (don't write outside the slice, don't route a refused Write through Bash) remains instruction-held rather than mechanically enforced is disclosed in the PR's own "Security note" and isn't newly introduced by this diff.

Branch ·

[claude]

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

---

Code Review — PR #2222 (correctness / architecture / maintainability lane)

Todo list

• Gather context (PR diff, changed files, prior review threads)
• Verify whether commit 33e368f ("close four gaps review found in the new by-value rung") actually fixed the collision-sub-slice and unbounded-ledger findings from the prior review round
• Re-review the four fixed files against the previously-flagged contradictions
• Check for any new correctness/design issues introduced by the fix commits (33e368f, 789799a)
• Post inline findings + summary

Per REVIEW.md, security is out of scope for this lane (a dedicated claude-security-review workflow already covers it and has reviewed this PR separately).

What changed since the last review round

Two commits landed since the prior code-review pass on this PR: 33e368f ("close four gaps review found in the new by-value rung") and 789799a (quote the disallowedTools: value). I traced 33e368f line-by-line against the four issues raised by the earlier reviews (mine, codex's, and the security lane's) rather than re-deriving them from scratch.

All four are correctly fixed, and consistently across every file that needed it:

1. Explorer collision-vs-by-value contradiction (agents/explorer.md#L209-L214, skills/explore/reference/dispatch.md#L96-L104, skills/explore/SKILL.md#L65) — the by-value artifact: path now resolves to the sub-slice when a collision applies, and — importantly — the parent independently re-derives whether a collision exists ("anchor on the memory-slice path the parent resolved before dispatch... if that slice root already holds an unrelated EXPLORE.md... the parent assigns a sub-slice") rather than trusting the agent's artifact: value as the anchor. That matches the "a claim the gate is invited to accept on the agent's word is laundering" principle the rest of this PR is built on.
2. Researcher unbounded-corpus ledger contradiction (agents/researcher.md#L211-L224, skills/research/context/dispatch.md#L160-L172, skills/research/SKILL.md#L66) — research-checklist.md is now appended only "if this run wrote one," and the coverage-ledger gate is re-run "whenever a ledger was owed." Same non-trust pattern as chore: initialize marketplace scaffold #1: the parent determines whether a ledger was owed by reading the recovered index's own unbounded/bounded record, not by trusting the agent's payload — I confirmed this against the pre-existing rule at skills/research/SKILL.md#L58 ("No ledger on disk is correct only when the artifact records the corpus as unbounded"), which this fix cites verbatim rather than inventing a new rule.
3. Filename path-traversal gap (the security-lane finding) — both agent files and both dispatch.md ladders now gate on an explicit allow-list (EXPLORE.md/EXPLORE-<section>.md and RESEARCH.md/RESEARCH-<section>.md/research-checklist.md, each required to be a bare filename with no ..//), checked before anything is written, with a "resolved path stays inside the destination directory" backstop. This is essentially the exact mitigation the security lane suggested.
4. Eval coverage — evals.json on both skills gained cases for the collision-recovery path, the unbounded-corpus-by-value path, and the filename-rejection path, closing the gap the maintainability review noted (the original by-value eval only covered a bounded corpus).

I independently re-read the full text of agents/explorer.md, agents/researcher.md, both dispatch.md files and both SKILL.md files at current HEAD (not just the incremental diff) to make sure the fix didn't just patch the specific lines quoted in review while leaving a stale copy elsewhere — it didn't; I found no third copy of the old unconditional wording anywhere in the plugin.

789799a is a pure formatting fix (quotes disallowedTools: to match every other tools:/disallowedTools: value in the repo) — resolves the earlier style nit, no behavior change.

New findings

None. I did not find any new correctness or design issues introduced by these two commits, and no regressions in the areas the fix touched.

Not independently verified

I do not have shell-execution approval in this sandboxed session (bash ... and python3 -c ... both required approval that wasn't available), so I could not run plugins/discovery/agents/tool-honesty.test.sh, check-dispatch-artifact.test.sh, shellcheck, shfmt, or markdownlint-cli2 myself. My confidence in this fix is from static reading of the prose contracts across all affected files and cross-checking the new conditionals against the pre-existing rules they cite (e.g. SKILL.md:58's unbounded-corpus rule), not from execution. JSON validity of plugin.json I did confirm by inspection (version: "0.14.0", matches the CHANGELOG's ## [0.14.0] heading).
· Branch