Skip to content

fix(discipline): make sweep-all's fork stop-rule evaluable and stop claiming enforcement it lacks - #1628

Merged
kyle-sexton merged 13 commits into
mainfrom
fix/discipline-sweep-all-fork-preflight
Jul 27, 2026
Merged

fix(discipline): make sweep-all's fork stop-rule evaluable and stop claiming enforcement it lacks#1628
kyle-sexton merged 13 commits into
mainfrom
fix/discipline-sweep-all-fork-preflight

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Closes #1621, closes #1622, closes #1623.

discipline 0.9.0 → 0.10.0. Prose and evals only — no frontmatter change, no manifest surface
beyond the version bump.

The defect

sweep-all carried a stop-rule: "if forks are unavailable, report that the inheriting audit
fan-out cannot run and stop, rather than auditing blind."
Nothing in the plugin defined how to
determine that forks are unavailable.
A rule whose guard cannot be evaluated is inert, not
merely under-specified — so the path that actually ran was the blind one: non-inheriting subagents
fabricating ledgers from their system prompt, merged at step 3 and written to the working tree
at step 4, which instructs "correct forward … on the main thread now".

Observed, not hypothetical. A real full-batch run dispatched eight forks; all eight came back with
no inherited conversation. Two of the eight refused to invent a ledger, and that refusal is the
only reason the run did not merge eight fabrications.

What landed

A two-stage preflight, before the fan-out.

  • Stage 1 — zero dispatch, diagnostic only, and it deliberately never gates. Two documented
    sentences pair up: fork mode "removes the run_in_background parameter from the Agent tool"
    (sub-agents), while
    CLAUDE_CODE_DISABLE_BACKGROUND_TASKS removes it from "Bash and subagent tools"
    (env-vars). The pair discriminates those two causes,
    which one signal alone cannot — but the docs tie removal to the env-var path and say nothing
    about the server-side rollout, so no branch is conclusive. Shipping it as a gate would create the
    false negative that silently downgrades a working sweep, so it explains rather than decides.
  • Stage 2 — the decider. ONE proof-only canary fork, dispatched alone ahead of the first wave,
    carrying no corrector and returning no ledger. An earlier revision folded it into the first
    member's real audit to make it free; review caught that this breaks the plugin's own
    independence invariant — a fork inherits everything the session holds at spawn, so that member's
    ledger would sit in every later fork's context and anchor its audit. The guard costs one extra
    fork and the skill says so rather than disguising it. The runbook specifies the proof question's
    four required properties rather than a fixed question — the answer exists only in conversation
    history, the prompt neither contains nor paraphrases it, it keys on ordinary inherited material,
    and it cannot be guessed. Where history supplies nothing suitable, the main thread mints a
    high-entropy value into the transcript before the canary spawns, so a thin session never loses
    its audit. The main thread fails closed: absent, ambiguous, or unverifiable proof counts as
    not inherited.
  • Only a failed canary collapses the pass. A member returning unproven later, after inheritance
    is already established, is retried once and then reported open — the verified, checkpointed
    ledgers are kept and corrected rather than discarded.

The degraded mode moves to where it executes. setup declared "where it is off, only the
session-start posture digest runs"
while sweep-all declared "report and stop" — two contracts
for one condition, and the better one in a file a sweep never loads, so the operator got nothing at
all. sweep-all now owns the degraded pass; setup points at it instead of restating it.

Stop implying the no-writes rule is enforced (#1622). Forks "skip both filters and receive the
main conversation's exact tool pool"
, and a fork's tools are "Same as main session" — a named
subagent can be narrowed with tools, a fork cannot. Every audit fork holds Write/Edit/Bash and is
only asked not to use them, and the declared delta's safety argument rested on that. The skill
now says so plainly, and treats a fork that wrote as untrusted output to stop on rather than
correct on top of.

Detection of such a write is advisory here, and deliberately split to #1631. Intermediate
revisions of this PR carried a normative before/after working-tree digest protocol. It drew a
correct review finding in three consecutive rounds — baseline taken after the canary; status
entries that do not move when an already-dirty file is rewritten; a tripped check that corrected
anyway; an undefined non-git branch; no comparison at the canary-return boundary; ?? dir/ entries
that git hash-object cannot digest. Six holes in one prose-specified protocol is the signal, not
a to-do list. The plugin's own script-the-deterministic-work position says that capture/digest/
diff belongs in a script the skill calls, and reuse-or-replace says to generalize the repository's
existing state-digest contract rather than stand up a second parallel one in prose. So the honesty
statement — the actual commissioned remediation for F3 — ships, and the mechanism is designed
properly in its own issue instead of hardened a fourth time.

isolation: "worktree" was in the first revision as containment and is rejected after review
(both threads on this PR), with the reasoning recorded in the skill so it is not re-proposed. A git
worktree is created from a commit, so isolated audit forks would not see the uncommitted work in
flight — usually the very thing under audit — and isolation would not bound a write addressed by an
absolute path, of which inherited history is full. Real audit fidelity traded for partial
containment is a bad trade for a correctness skill.

Concurrency (#1623). The wave cap no longer imports the -deep siblings' "roughly a dozen",
which is calibrated for cheap fresh-context subagents and exceeds
CLAUDE_CODE_MAX_TOOL_USE_CONCURRENCY (default 10). It is four; the shared
CLAUDE_CODE_MAX_CONCURRENT_SUBAGENTS (20) and CLAUDE_CODE_MAX_SUBAGENTS_PER_SESSION (200)
budgets are modeled; and the siblings' per-wave checkpointing is restored rather than dropped by
omission.

Failure is defined. "Retry only a failed subset, once" now covers the actual exposure: a ledger
returned without verified inheritance proof is a failure, not just an errored dispatch.

Two evals for paths all six existing evals assumed away: the fork-unavailable degrade, and the
tool pool being contained rather than enforced.

What this PR refutes

The audit was commissioned with the framing "no degraded mode exists" plus a proposal to build a
three-rung fallback ladder. At plugin scope that is wrong — a degraded mode was already the
plugin's declared position, just stranded in the wrong file. The ladder is deliberately not
built
; it re-creates the salience dilution the declared delta exists to prevent and would yield
audits weaker than the ones the skill halts to avoid.

Also refuted: the tier metadata format is not a defect. discipline-batch: core # … is valid
YAML and a real parser yields core; only a naive string read is defeated by the trailing comment.
The defect belongs to the runbook's read instruction, tracked separately in #1624.

Fresh-docs compliance

Every harness claim above was verified this session against pages fetched today —
https://code.claude.com/docs/en/sub-agents and https://code.claude.com/docs/en/env-vars — with
the quoted text taken verbatim. Two independent retrieval routes were used after the first
summariser pass returned a false negative for variables that are in fact documented.

Flagged unverified in the shipped text, not asserted:

  • What the harness does when the fork type is requested while fork mode is OFF. The skill
    previously stated this as harness fact; it appears on no current page. Now labelled
    observed-not-documented, and the preflight no longer depends on it.
  • The behavioral consequence of isolation: "worktree" on a fork. Only its acceptance by the
    Agent tool is documented, so after review the skill no longer makes any claim about what it
    contains — it records why the option was rejected instead.
  • One first-hand observation, labelled as such: in a fork-enabled session an out-of-band advisor
    tool result was absent from a fork's inherited transcript — which is why the proof question must
    key on ordinary inherited material.

Verification

  • scripts/check-changed-skills.sh origin/main — PASS, 0 errors. All 7 base-ref trigger phrases
    preserved on sweep-all, all 10 on setup. The one fresh-eyes WARN on sweep-all is
    pre-existing (confirmed by running the checker against the origin/main blob).
  • scripts/check-changelog-parity.sh --check-bump origin/main — PASS.
  • scripts/validate-plugins.sh — PASS.
  • markdownlint-cli2 over plugins/discipline/**/*.md — 0 issues.

Related

Not closed by this PR:

🤖 Generated with Claude Code

…laiming enforcement it lacks

The stop-rule "if forks are unavailable, report and stop" had no detection
mechanism anywhere in the plugin, so its guard could never be evaluated and the
blind path was the one that ran: non-inheriting subagents fabricating ledgers,
merged at step 3 and written to the working tree at step 4.

- Add a two-stage preflight. Stage 1 is a zero-dispatch paired tool-schema read
  that deliberately never gates (the docs tie run_in_background removal to the
  env-var path only). Stage 2 is the decider: an inheritance-proof canary folded
  into the first member's real audit, verified on the main thread, failing closed.
- Move the degraded mode out of setup, which a sweep never loads, into sweep-all
  where it executes; setup now points instead of restating.
- Say plainly that forks receive the main conversation's exact tool pool, so
  "make NO writes" is trusted, not enforced; dispatch with isolation: "worktree"
  as containment and name the residual risk.
- Replace the imported "roughly a dozen" wave cap with four, model the shared
  concurrent-subagent budgets, restore per-wave checkpointing, and define failure
  for the retry rule as an unproven ledger.
- Flag the undocumented fork-off fallback as observed, not documented.

Refs #1621, #1622, #1623

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review

  • Gather context and understand the PR
  • Read changed files
  • Review sweep-all/SKILL.md (primary change)
  • Review setup/SKILL.md
  • Review evals/evals.json
  • Review plugin.json and CHANGELOG.md
  • Synthesize findings and post review

This is a high-quality fix for three real defects. The two-stage preflight design is well-reasoned, the fresh-docs citations are present and correctly qualified, the responsibility split between setup and sweep-all is clean, and the worktree containment addition is a genuine security improvement over the prior "trusted not enforced" gap that went unnamed. I have three findings — one clarity issue that could mislead a future reader, one minor imprecision, and one inherent eval-format limitation that is worth acknowledging.


Finding 1 — Canary retry: preflight and wave-cap sections give contradictory signals (clarity)

sweep-all/SKILL.md — Preflight section

Verified → keep the canary's ledger … and fan the rest out. Unproven → degrade; never re-dispatch the batch blind.

sweep-all/SKILL.md — Wave cap section, line 180-183

Retry only a failed subset, once — and failure includes a ledger returned without verified inheritance proof … A second unproven return degrades per the preflight.

The preflight section reads as "one unproven canary result → immediate degrade." The wave-cap section reads as "unproven result → retry once → degrade on second failure." For members dispatched after a passing canary these are consistent, but for the canary itself they give opposite answers: does a first-time unproven canary degrade immediately or get one retry?

The intent appears to be that the canary gets no retry (the preflight section's "never re-dispatch the batch blind" being the governing rule), and the wave-cap retry applies only to post-canary members. But a reader building from the wave-cap section alone would infer the canary gets a retry. Suggest either:

  • adding "including the canary — the canary gets no retry" to the retry sentence, or
  • reordering to establish the wave-cap retry rule first and carving out the canary explicitly.

Fix this →


Finding 2 — Stage 1 first branch overstates what "Agent still carrying it" establishes (minor imprecision)

sweep-all/SKILL.md — Stage 1, line 71

So Agent still carrying it means fork mode is not env-var-enabled

If fork mode were active via the server-side rollout (not the env var), the Agent tool would also lose run_in_background. So Agent carrying the parameter means fork mode is off by either mechanism — not just not-env-var-enabled. The distinction matters here because the paragraph is explaining how to read the signal, and this branch is the one case where Stage 1 would actually give a definitive negative on fork-mode availability. Stating it as "not env-var-enabled" undersells the branch and could lead an operator to think "maybe the server rollout enabled it anyway" when Agent having the parameter rules that out.

The final "no branch is conclusive in either direction" sentence then contradicts this: if Agent carries the param, that branch is conclusive (fork mode is off). The broader claim should narrow to the other two branches. Low risk given Stage 1 never gates, but it would help a reader calibrate.


Finding 3 — Eval 7's prompt cannot establish the failure condition it tests (inherent, acknowledged)

evals/evals.json — eval id 7, line 79-80

The expected output begins "When the canary returns a ledger without a verifiable inheritance proof…" — the prompt alone cannot force this condition, so any eval runner would be testing against a scenario that may not occur in the actual invocation. This is an inherent limitation of the eval format (harness state cannot be injected), and the PR description correctly acknowledges these two evals cover "paths every existing eval assumed away." Worth noting explicitly in the eval's expected_output or a comment that this path is contingent on an uncontrollable runtime condition — which would make it clearer to a future editor why the expected output is conditional rather than absolute.


Other notes (no action required)

  • "at no extra cost" framing for the canary is accurate in the success path but slightly optimistic in the failure path (the canary fork's audit tokens are spent but the ledger discarded). The cost bound is one fork and the comment is appropriate given the context; noting it for completeness.
  • isolation: "worktree" caveats (non-git projects, nested worktrees, Bash reaching outside the checkout) are correctly flagged as undocumented and stated as residual risk, not a guarantee. This is well-calibrated.
  • The setupsweep-all responsibility transfer is clean. The new setup step 6 text preserves exactly the user-facing value (unexpected fan-out reads as expected, not misconfiguration) without restating the degraded-pass contract.
  • CHANGELOG entries are detailed, issue-linked, and accurately describe what changed.
  • Fresh-docs compliance: all harness claims carry inline citations to fetched pages; unverified observations are labelled as such. Passes.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628

  • Gather context and understand request
  • Read PR diff
  • Read changed skill files (sweep-all/SKILL.md, setup/SKILL.md)
  • Read changed eval file (evals.json)
  • Analyze for security issues
  • Post findings

All changes are in LLM skill instruction files (Markdown) and a JSON evals definition. The security surface is prompt-level: instructions that, if wrong, cause the model to take unsafe actions — writes to the working tree, exfiltration via Bash, or acts without user authorization. I reviewed for those logic and trust-boundary issues; there are no GitHub Actions workflow changes to audit.

Net direction is clearly positive. The PR adds a fail-closed preflight where none existed, adds isolation: "worktree" to contain rogue fork writes, and removes the false claim that the no-writes rule is harness-enforced. The pre-PR state (zero preflight, zero containment, false enforcement claim) was the real vulnerability; this PR closes it.

One genuine gap remains in the canary design, plus a residual risk that is correctly disclosed but worth naming explicitly in a security review.


IMPORTANT — Canary proof-question self-validation gap

Confidence: PLAUSIBLE

The main thread chooses the inheritance-proof question and dispatches the canary. Property 2 of the question spec says "the dispatch prompt neither contains nor paraphrases the answer." However, the skill includes no explicit instruction for the main thread to check its own dispatch message against this property before sending. The model must both formulate the question and reason that its dispatch message doesn't leak the answer — in the same context, in one step, with no backstop.

This matters here more than in a typical "follow these properties" instruction because the canary is folded into the lowest-ranked member's real audit. A false canary positive has a two-part consequence: (1) the batch proceeds with non-inheriting forks, and (2) the canary fork's fabricated audit result is retained as real and carried into the step-3 dedup and step-4 corrections written to the working tree. A question that accidentally leaks its answer doesn't just allow the batch to run blind — it also seeds a fabricated audit result.

A concrete failure path: the conversation is short (e.g., the first user message was "sweep all disciplines"), so the only unique conversation content is the user's opening phrase. The main thread chooses a question like "What was the first message the user sent this session?" and includes in the dispatch prompt "answer the preflight question first, then load the corrector." A non-inheriting fork can trivially reconstruct the answer from context about what triggered the skill. The main thread accepts this as verified inheritance, the canary ledger is kept, and all subsequent forks run blind.

The fix is one sentence added before the dispatch instruction:

Before sending the canary dispatch, verify on the main thread that your chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question.

This is a self-check the main thread can execute cheaply and without a separate dispatch.

Permalink: plugins/discipline/skills/sweep-all/SKILL.md L80–106

Fix this →


SUGGESTION — No suitable-question case is left implicit

Confidence: PLAUSIBLE

Property 1 requires the answer to exist only in conversation history — not in any file, CLAUDE.md, or plugin content. On a very short session (one or two turns) where all content is either in the system prompt, the CLAUDE.md hierarchy, or the dispatch message itself, there may genuinely be no question satisfying all three properties.

The skill's fail-closed rule ("Absent, ambiguous, or unverifiable proof counts as NOT inherited") covers the fork's answer, but doesn't explicitly cover the case where the main thread cannot identify a question satisfying the properties in the first place. A model that tries anyway and formulates a question that violates property 1 or 2 returns to the gap above.

Adding "If no question satisfying all three properties can be identified from the current conversation content, degrade immediately — treat it as a failed preflight" would close this implicitly.

Permalink: plugins/discipline/skills/sweep-all/SKILL.md L85–99


Disclosed residual risk — fork Bash access is not contained by isolation: "worktree"

The skill now correctly states that every audit fork holds Write, Edit, and Bash, and that isolation: "worktree" does not stop a fork's Bash from reaching the network, ~/.claude, or paths outside the checkout. This is accurate and properly disclosed. Noting it here because it is the remaining unmitigated risk boundary: a fork instructed to audit could exfiltrate conversation content over the network, read or write ~/.claude credentials or config, or act on external systems. The instruction-level "make NO writes" is trusted, not enforced. No finding; no action beyond the disclosure already present.


No CRITICAL findings. The IMPORTANT finding is the proof-question self-validation gap; adding one sentence to Stage 2 closes it. The disclosed residual Bash/network risk is inherent to fork subagents and is correctly labeled as trusted-not-enforced.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a8e3ed627

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
…fan-out, and preserve verified ledgers

Addresses both PR review threads and a fresh-context review of the diff.

- isolation: "worktree" is rejected, not shipped. A worktree is created from a
  commit, so isolated audit forks would not see the uncommitted work in flight
  — usually the thing under audit — and isolation would not bound an
  absolute-path write, of which inherited history is full. Replaced with a
  before/after working-tree comparison: verify the no-writes rule instead of
  claiming containment. The rejection reasoning is recorded in the skill.
- Step 1 no longer re-dispatches the canary member.
- A member that returns unproven mid-fan-out no longer collapses the whole pass
  to the posture digest, discarding checkpointed verified ledgers. Only a failed
  canary degrades; later failures keep the verified subset and report the rest
  open.
- "Lowest-ranked" disambiguated to lowest discipline-batch-rank value.
- Softened an over-general claim the PR's own evidence refutes: six of eight
  non-inheriting subagents fabricated a ledger, two refused.
- Cited the CLAUDE.md-hierarchy claim the proof question depends on.
- Two-modes summary, declared-delta paragraph, and the cost example brought in
  line with the inserted preflight.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit 58caf37)

  • Gather context — reviewing commit 58caf37 against prior reviews on 4a8e3ed
  • Read updated sweep-all/SKILL.md
  • Read updated setup/SKILL.md and evals.json
  • Diff against prior review commit
  • Analyze which findings were addressed and check for new issues
  • Post review

Commit 58caf37 ("drop worktree isolation, keep the canary out of the fan-out, and preserve verified ledgers") addresses the bulk of the findings from both prior review passes. Here is the status of each, plus one remaining gap.


Prior findings — resolved

Code Review F1 — canary retry contradiction. The contradiction between the preflight section ("Unproven → degrade") and the wave-cap section ("a second unproven return degrades") is resolved. The preflight now has a dedicated paragraph for the mid-fan-out case ("A member that returns unproven LATER, mid-fan-out, is a different case…") with clear instructions to retry once, keep verified ledgers, and reserve the digest collapse for a failed canary. The wave-cap section now reads "A member still unproven after its one retry is reported open… only a failed canary collapses the pass to the digest." — precisely correct.

Security Review SUGGESTION — no suitable question degrades immediately. Addressed. "If the conversation is too thin for any question to satisfy all three, it is also too thin to audit: report that and run mode 1 instead." closes the implicit case the prior review flagged.

Codex P1/P2 — worktree isolation hides uncommitted work and does not bound absolute-path writes. Both resolved by dropping isolation: "worktree" entirely and replacing it with a before/after git status --porcelain comparison. The Gotcha records the rejection reasoning explicitly ("Record this if it is proposed again"), which is the right way to handle a design decision that will otherwise be re-proposed.

Canary double-dispatch prevention. The canary was previously implicit ("fold it into the first member's audit") with no explicit guard on re-dispatching it. Now explicit: "For each REMAINING in-scope corrector (the canary member already returned a real ledger; never dispatch it twice)." Also: "that member is not dispatched again." Belt and suspenders — correct.


Prior findings — still open

Code Review F2 — Stage 1 first branch understates what "Agent still carrying run_in_background" establishes.

SKILL.md L76–82

This is unchanged from 4a8e3ed. The text reads:

So Agent still carrying it means fork mode is not env-var-enabled … no branch is conclusive in either direction.

If Agent still carries run_in_background, fork mode is definitively off by either mechanism — not merely not env-var-enabled. Both the env-var and the server-side rollout are documented as removing this parameter; Agent carrying it rules both out. The "no branch is conclusive in either direction" sentence then contradicts its own first branch. Since Stage 1 never gates, this is a calibration issue only — an operator reading this might incorrectly hold open the hypothesis "maybe the server rollout enabled fork mode anyway" when Agent's tool schema actually settles it. Low risk, but the prior review's suggested fix (narrow the "no branch is conclusive" claim to the other two branches) is still the right call.

Fix this →

Security Review IMPORTANT — proof-question self-validation gap.

SKILL.md L92–109

Property 2 ("The dispatch prompt neither contains nor paraphrases the answer") is a constraint on the question chosen, but the skill does not include an explicit self-check step before the main thread sends the dispatch. The recommended addition from the prior security review — "Before sending the canary dispatch, verify on the main thread that your chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question" — is not present. A model choosing a question from a short session could accidentally leak the answer in the dispatch and produce a false positive that seeds a fabricated ledger into the step-3 dedup. Given that stage 2 is the only gate, a false positive here carries the full downstream consequence (fabricated ledger written to working tree in step 4). Adding the explicit pre-send check adds one sentence and closes this.

Fix this →


New observation (minor)

Before/after git status comparison is unspecified for non-git projects.

SKILL.md L176–179

The instruction reads "in a git project, git status --porcelain" — correctly qualified, but leaves the non-git case silently unspecified. Since the audit can run in non-git projects, an operator in that case has no write-verification path at all. A single added phrase ("in a non-git project, record the modification times of any files the fork was instructed to load" or simply "skip the comparison and note it as unverified in the report") would close the gap without over-specifying. This is minor given the "trusted, not enforced" framing, but the git status path is currently privileged over a silent fallback.


Other notes (no action required)

  • The token-cost note is correctly updated: "eight forks either way, whether the batch spends them as the canary plus two waves or all at once" accurately reflects that the canary is a full-cost member audit — no phantom savings from the fold.
  • The CHANGELOG update ("verifies rather than claims" vs. "contains") is accurate and matches the new design.
  • The declared-delta paragraph now names both divergences (preflight + split) explicitly — cleaner than the prior implicit single divergence.
  • Eval 8's expected output correctly describes the new posture: "it verifies: it records the working tree's state before dispatch and compares it after collecting the ledgers" — no longer claims containment.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628

  • Gather context and understand the PR
  • Read changed files (sweep-all/SKILL.md, setup/SKILL.md, evals.json, plugin.json)
  • Analyze for security issues — logic flaws, trust boundaries, injection, data-flow
  • Post findings

All changes are LLM skill instruction files (Markdown) and a JSON evals definition. The security surface is prompt-level: instructions that if wrong cause the model to take unsafe actions — writes to the working tree, exfiltration via Bash, or acts without user authorization. No GitHub Actions workflow files changed. I reviewed for logic and trust-boundary issues that static analysis cannot reach; zizmor's lane (supply-chain, trigger patterns, permissions, template injection) is not re-reported here.

Net direction is positive. The PR adds a fail-closed preflight where none existed, removes the false enforcement claim, and correctly discloses the residual Bash/network risk as trusted-not-enforced. The prior security review at commit 4a8e3ed identified one open IMPORTANT finding (proof-question self-validation gap). That finding remains open at the current HEAD 58caf37. I also identify one additional IMPORTANT finding the prior review did not cover.


IMPORTANT — Post-canary forks can lift the proof answer from conversation history (bootstrapping gap)

Confidence: PLAUSIBLE

The canary is dispatched first and alone. If it passes, its response — containing the stated proof answer — is recorded in the main conversation thread. Subsequent forks dispatched in waves 1, 2, etc. inherit the entire main conversation at the moment of dispatch, which by then includes the canary's exchange: the proof question and the canary's verified answer to it.

A non-inheriting fork dispatched after the canary can read the canary's answer from the inherited conversation and re-state it as its own inheritance proof. The main thread verifies the fork's answer against "what this context knows" — and the canary's answer IS now in this context, so the re-stated answer appears correct. The main thread accepts it as a pass.

The prior review's self-validation gap covers the canary itself (the dispatch message leaking the answer before the canary runs). This is a different path: after the canary passes, the conversation itself becomes the leak source for all subsequent forks.

Concrete failure path: the observed incident had 6 of 8 forks non-inheriting within a single batch. If the canary is one of the 2 that genuinely inherits, it passes and records its proof answer. The 6 non-inheriting forks dispatched in subsequent waves read the canary's answer from conversation history, echo it, pass the main-thread check, and their fabricated audit ledgers proceed to step 3 (dedup) and step 4 (write corrections to the working tree).

Permalink: plugins/discipline/skills/sweep-all/SKILL.md L85–127

Mitigations to consider (in order of containment strength):

  1. Use a different proof question for each fork dispatch — the canary's answer in conversation history can't satisfy a question the main thread chose after seeing the canary's response.
  2. Instruct each fork: do not cite or derive your proof answer from any prior audit exchange visible in the conversation; answer only from the original session content.
  3. For post-canary forks, add a second proof question not used in the canary, so reading the canary's exchange does not suffice.

Fix this →


IMPORTANT (open from prior review) — Canary dispatch self-validation gap

Confidence: PLAUSIBLE — still open at HEAD 58caf37

The main thread selects the proof question and embeds it in the canary dispatch message. Property 2 of the question spec says "the dispatch prompt neither contains nor paraphrases the answer" — but there is no instruction for the main thread to verify compliance with property 2 before sending. A proof question whose answer can be inferred from the dispatch message's context allows the non-inheriting canary to appear to pass.

The one-sentence fix from the prior review was not applied: "Before sending the canary dispatch, verify on the main thread that your chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question."

Permalink: plugins/discipline/skills/sweep-all/SKILL.md L92–119

Fix this →


SUGGESTION — Non-git project leaves the write-verification path unspecified

Confidence: PLAUSIBLE

The before/after working-tree comparison uses git status --porcelain, qualified with "in a git project." No equivalent is defined for a non-git project (a fresh directory, a containerized environment, a Mercurial repo). The skill says "record the working tree's state immediately before dispatch (in a git project, git status --porcelain)" without specifying the fallback, so a non-git audit run silently skips the check and leaves the no-writes rule entirely unverified by any mechanism other than the instruction itself.

This is lower severity than the proof-bootstrapping finding because the primary guard (instructing forks not to write) still applies; the before/after check is a verification layer, not the primary prevention. Worth a one-line addition: "In a non-git project, record file modification times or checksums for the working directory to approximate the same check; where no comparison is possible, note that the no-writes rule is unverified for this run."

Permalink: plugins/discipline/skills/sweep-all/SKILL.md L167–179


Disclosed residual risk — confirmed accurate

The fork Bash/network/credential access disclosure is accurate and correctly labeled as trusted-not-enforced. No new finding here; noting for completeness that the disclosure at lines 307–314 and 318–322 covers what the before/after git status check does and does not bound.


Summary: Two IMPORTANT findings — the bootstrapping gap (new, this review) and the self-validation gap (open from prior review, not yet fixed) — both rest on the same underlying fragility: the proof mechanism relies entirely on careful question selection, with no structural backstop when question selection fails. Both gaps compound each other: the self-validation gap allows the canary to be spoofed, and the bootstrapping gap allows every subsequent fork to be spoofed off the canary's recorded answer. Fixing one without the other leaves the preflight only half-closed.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 58caf37c37

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
… define the non-git branch

The write-verification check had the same shape of defect as the guard this PR
exists to fix: a precondition that may not hold, with no stated fallback.

- Move the baseline capture into the preflight, ahead of the canary dispatch.
  The canary is a fork with the same unconstrained tool pool as every later
  member, so a baseline taken before wave 1 would miss it.
- Define the non-git branch explicitly: skip the comparison and report the skip,
  rather than omitting it silently and letting the reader assume it ran.
- Say what to do when the comparison finds a difference: report it as its own
  finding and hold it out of the correction pass.
- Cite the staged-rollout claim in setup, which was the one uncited harness
  statement left in that file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit 9dbae2a)

  • Read updated sweep-all/SKILL.md
  • Check git diff of the new commit
  • Evaluate open findings from prior reviews (F2, Security IMPORTANT, non-git observation)
  • Evaluate Codex findings from the 58caf37 review pass
  • Post review

Commit 9dbae2a ("take the working-tree baseline before the canary and define the non-git branch") addresses the baseline-timing and non-git gaps flagged in the last review pass, plus closes the write-detection failure branch. Here is the full accounting.


Resolved in this commit

Non-git project case (prior review: new observation). Resolved. The preflight now explicitly states: "Where the project is not a git repository, there is no cheap baseline to take — skip the comparison and say so in the final report, rather than omitting the check silently and leaving the reader to assume it ran." The fallback is defined, not implicit.

Baseline before canary (Codex P1 on 58caf37). Resolved. The baseline capture paragraph is now in the preflight section, between Stage 1 and Stage 2, with explicit reasoning: "The canary is a fork with the same unconstrained tool pool as every later member, so the baseline has to precede it, not wave 1." The step-1 prose is updated to reference "the baseline the preflight took" rather than re-capturing it there.

Failure branch when write-check detects a difference (Codex P1 on 58caf37). Resolved. Step 1 now reads: "Any difference is a fork that wrote — report it as its own finding and hold it out of the correction pass; never fold it in silently." The prior text had no stated consequence; this defines it.

Uncited staged-rollout claim in setup/SKILL.md. Resolved. "server-side rollout" is now "server-side staged rollout" with an inline citation to the current sub-agents page.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts itself.

SKILL.md L76–83 — unchanged from 58caf37.

Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var path and the server-side rollout are documented as removing run_in_background from the Agent tool. So Agent carrying the parameter rules out both mechanisms — fork mode is definitively off, not merely "not env-var-enabled." Branch 1 is in fact conclusive (in the negative), and the "no branch is conclusive in either direction" sentence contradicts it. Since Stage 1 never gates this remains calibration-only, but it leaves an operator incorrectly holding open "maybe the server rollout enabled fork mode anyway" when Agent's schema settles it.

The two-part fix remains the same: change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", and narrow "no branch is conclusive in either direction" to cover only the other two branches (Agent lacking while Bash keeps it; both lacking).

Fix this →


Security IMPORTANT — proof-question self-validation gap.

SKILL.md L99–126 — unchanged from 58caf37.

Property 2 ("The dispatch prompt neither contains nor paraphrases the answer") remains a design constraint on the question, not a procedural pre-send step. The main thread must both formulate the question and ensure the dispatch message does not contain the answer — in the same context, without a backstop. A false positive here seeds a fabricated ledger directly into the step-3 dedup and step-4 writes. The recommended single sentence from the prior security review — "Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send; if it can, choose a different question" — is still absent.

Fix this →


New finding — git status --porcelain cannot detect content changes in already-dirty files

SKILL.md L85–90 (preflight baseline) and L184–186 (step 1 comparison) — introduced in 58caf37, this commit inherits it.

git status --porcelain reports path-level status codes (e.g. M path for a working-tree modification). If a file is already dirty when the baseline is taken, a fork that rewrites it with different content produces the same status line — git status --porcelain is unchanged before and after, and the "Any difference is a fork that wrote" check silently passes. This is the usual case: the skill describes uncommitted work in flight as "usually the very thing under audit," so most of the audited files will already be dirty.

git diff HEAD (or git diff for untracked files, git diff HEAD -- for both) captures content, not just presence-of-modification, and its output would differ when a fork rewrites an already-modified file. Using it as the baseline captures payload, not status codes. This is a harder change than the prior fixes but the fidelity gap is real: the comparison can report clean on a fork that violated the no-writes rule against a file that was already staged or modified.

The Codex finding from the 58caf37 review pass that named this was not addressed in 9dbae2a. It is flagged here as a residual gap in the write-verification coverage.

Fix this →


Other notes (no action required)

  • The step-1 wording is now clean — "Compare the working tree against the baseline the preflight took" makes the single-capture-point clear, and the failure-consequence ("hold it out of the correction pass") is explicit.
  • The setup/SKILL.md citation is the last uncited harness claim from the prior review's pass; the file is now fully sourced.
  • The overall trajectory is positive: the write-verification mechanism went from "assumed, no baseline timing, no failure branch, no non-git fallback" to "captured before canary, compared after collection, failure defined, non-git explicitly handled" across these three commits. The two remaining open findings and one new finding above are incremental gaps in an otherwise substantially improved design.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review (commit 9dbae2a)

  • Gather context and understand the request
  • Read PR diff and changed files at HEAD 9dbae2a
  • Evaluate open findings from prior reviews (self-validation gap, bootstrapping gap, Codex P1s)
  • Analyze for new security issues in the latest commit
  • Post findings

All changes are LLM skill instruction files (Markdown). No GitHub Actions workflows changed. Security surface is prompt-level — instructions that, if wrong, cause the model to write fabricated corrections to real files. This review covers logic, data-flow, and trust-boundary issues static analysis cannot reach; I defer to zizmor's lane on supply-chain, trigger patterns, and permissions.

Net direction of 9dbae2a is positive. Three findings from the prior review passes are resolved:

  • Codex P1 (baseline timing) — Resolved. Baseline is now taken in the preflight, before the canary dispatch, so the canary fork's potential writes are covered.
  • Codex P1 (mismatch handling) — Resolved. A detected write by a fork is now defined: report it as its own finding and hold it out of the correction pass.
  • Prior SUGGESTION (non-git branch) — Resolved. Non-git case is now explicit: skip the comparison and say so in the final report.

Two IMPORTANT findings from the 58caf37 security review remain open at HEAD 9dbae2a. One new SUGGESTION is added for the content-fidelity gap exposed by the baseline move.


IMPORTANT — Bootstrapping gap: post-canary forks can echo the canary's proof answer

Confidence: PLAUSIBLE — open since 58caf37 review, unchanged at HEAD

Once the canary verifies and its ledger is retained, the main thread records the canary's entire exchange in the conversation — including the proof question and the canary's answer to it. Subsequent forks dispatched in waves 1 and 2 are conversation-inheriting and receive the full conversation at the point of dispatch, which now contains the canary's verified answer.

The current text instructs each fork to "answer the preflight's inheritance-proof question first." It does not restrict the source from which a fork may draw its answer. A non-inheriting post-canary fork that sees the canary's answer in the inherited conversation history can echo it verbatim and pass the main-thread check — because the canary's answer IS in "what this context knows."

Concrete failure path: canary is one of two genuinely-inheriting forks (the observed 2-of-8 ratio). Its exchange — question and answer — is now in the conversation. Six non-inheriting forks dispatched afterward each see the canary's answer in the transcript they inherit, state it as their own proof, pass the main-thread verification, and their fabricated audit ledgers proceed to step 3 (dedup) and step 4 (write corrections to the working tree). The preflight passes; the batched write-out applies fabricated corrections.

Note this is structurally distinct from the self-validation gap below: the self-validation gap concerns the canary's own dispatch leaking the answer before the canary runs; this gap concerns the canary's response seeding the answer for all forks that run after it.

Permalink: plugins/discipline/skills/sweep-all/SKILL.md L99–127

Mitigation options (in order of containment strength):

  1. Distinct proof question per fork dispatch — the main thread selects a new question for each fork after seeing all prior responses; a question the canary answered cannot be re-answered from the canary's exchange by a later fork.
  2. Explicit scope restriction in the fork instruction — add to the dispatch prompt: "Answer from the original session content only — do not cite or derive your answer from any prior audit exchange visible in the conversation."
  3. Two-question canary — a second question asked of post-canary forks that was never included in the canary dispatch.

Fix this →


IMPORTANT — Canary self-validation gap: no pre-send check that the dispatch doesn't leak the answer

Confidence: PLAUSIBLE — open since 58caf37 review, unchanged at HEAD

Property 2 of the question spec (line 110: "The dispatch prompt neither contains nor paraphrases the answer") is a selection constraint the main thread applies when choosing the question. It is not a procedural verification step before the dispatch is sent. The model must both formulate a compliant question and confirm that its own dispatch message doesn't inadvertently leak the answer — in the same generation pass, with no explicit check instruction.

On a short session where the only unique content is the triggering phrase ("sweep all disciplines"), the available question pool is thin. The main thread may choose a question whose answer is inferable from context embedded in the dispatch, pass property 2 in selection reasoning, and send a dispatch that a non-inheriting canary can use to answer correctly.

The one-sentence fix from the 58caf37 review was not applied: "Before sending the canary dispatch, verify on the main thread that your chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question."

Permalink: plugins/discipline/skills/sweep-all/SKILL.md L99–126

Fix this →


SUGGESTION — git status --porcelain cannot detect content changes to already-dirty files

Confidence: PLAUSIBLE — new finding, exposed by the baseline move in 9dbae2a

The baseline move correctly resolves the canary timing gap. However, the verification mechanism itself has a fidelity floor: git status --porcelain tracks status (modified / untracked / staged), not content. A file already in a dirty state before the sweep begins shows M path in the baseline. If a fork rewrites that file with different content, git status --porcelain still shows M path — same entry, same flag. The before/after diff is empty. The no-writes rule verification silently misses the rewrite.

This is the worst-case target: the sweep's primary audit subject is mid-session uncommitted work — files that are already dirty at the point of dispatch. A fork that rewrites an already-dirty file produces no signal in the comparison.

A content-level baseline (git diff HEAD -- or checksums of the dirty files identified in the initial git status --porcelain output) would close this gap. Where the pre-baseline status already shows modified files, supplement the path+flag comparison with a content comparison for those specific paths.

Permalink: plugins/discipline/skills/sweep-all/SKILL.md L85–90


Summary. Two IMPORTANT findings remain open across all three commits: both rest on the same underlying fragility — proof question integrity depends entirely on question selection discipline, with no structural backstop. The bootstrapping gap compounds after a successful canary; the self-validation gap compounds before one. Addressing only one of the two leaves the preflight half-closed. The SUGGESTION is independent and lower-stakes — the no-writes rule is already labeled trusted-not-enforced, and this only expands the undetected-rewrite surface.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9dbae2a657

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
Comment thread plugins/discipline/skills/sweep-all/SKILL.md
…lly discriminating

Four findings from review, all of which made a guard look like a guard without
being one — the same failure class this PR exists to fix.

- The baseline now digests content, not just `git status --porcelain` entries.
  A fork rewriting an already-dirty file leaves the status letters unchanged,
  and a dirty file is exactly what a mid-session audit targets.
- A tripped check now stops the pass before dedup and correction, leaves the
  tree untouched for the user, and reports the in-flight members' ledgers as
  untrusted. Previously it reported the mutation and carried on correcting on
  top of unauthorized edits.
- No baseline available (not a git project) no longer proceeds and discloses
  afterwards, which protects nothing. The fan-out does not run on its own
  authority: report, and either degrade or take the user's explicit acceptance.
- The inheritance proof adds a fourth required property: unguessable. A yes/no
  or a detail common to most sessions clears an equality check without proving
  inheritance, which is precisely the hole the preflight exists to close.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit fa6d54a)

  • Gather context — reviewing commit fa6d54a against prior review passes on 9dbae2a
  • Read updated sweep-all/SKILL.md
  • Diff fa6d54a against 9dbae2a
  • Evaluate open findings from prior reviews
  • Check for new issues
  • Post review

Commit fa6d54a ("make the write check and the inheritance proof actually discriminating") addresses four of the remaining open findings across the prior code review and security review passes. Here is the full accounting.


Prior findings — resolved in this commit

Code Review (9dbae2a): git status --porcelain cannot detect content changes to already-dirty files.
Resolved. The preflight baseline now records BOTH git status --porcelain entries AND a content digest per listed path via git hash-object <path>. The rationale is stated inline ("Status letters alone are not enough: a fork that rewrites an already-modified file leaves its M-status entry unchanged, and a modified file is exactly what a mid-session audit is usually about"). Using git hash-object is the right call — it hashes working-tree content directly and is portable without shell-specific pipelines.

Security Review (9dbae2a): Codex P1 — fail closed when no working-tree baseline is available.
Resolved. The prior text said "skip the comparison and say so in the final report." The replacement text says: "Do not fan out on your own authority in that case: report that the verification is unavailable, and either run mode 1 or dispatch only on the user's explicit acceptance of an unverified fan-out. Proceeding silently and disclosing the gap afterwards protects nothing." This is materially stronger — the fan-out is now gated, not merely annotated.

Security Review (9dbae2a): Codex P1 — require an unguessable inheritance proof.
Resolved. A fourth required property has been added: "It cannot be guessed. An answer a non-inheriting subagent could hit by chance — a yes/no, a binary choice, a detail common to most sessions — clears the main thread's check without proving anything, and the blind ledgers behind it then reach the corrective write. Require an exact, high-entropy value: a verbatim string, an exact count, a specific identifier, or several independent details together." This directly addresses the Codex P1 on low-entropy proofs.

Write-mismatch failure branch — stop before step 3.
The prior commit introduced the mismatch check but left the failure branch unspecified (a Codex P1). Now resolved: "A mismatch means a fork wrote, so stop before step 3: do not dedup, do not correct, and never stack remedies on top of unauthorized edits. Leave the changed tree exactly as it is, report which members were in flight and that their ledgers are untrusted, and hand the decision back to the user."


Prior finding — bootstrapping gap substantially mitigated

Security IMPORTANT (58caf37): post-canary forks can echo the canary's proof answer.
The fourth proof property (cannot be guessed, requires high-entropy value) substantially mitigates this. A non-inheriting fork dispatched after the canary has no conversation history; if fork mode is truly off it cannot see the canary's exchange, so the only way it could answer the proof question is by guessing — which the fourth property now guards against by requiring exact verbatim strings, counts, or identifiers that cannot be guessed at chance. The bootstrapping path required a non-inheriting fork to read the canary's answer, but a truly non-inheriting fork has no context from which to read it. This is no longer a standalone open finding.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts itself.

SKILL.md L76–83 — unchanged across all four commits.

So Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var and the server-side rollout remove run_in_background from the Agent tool per the cited docs. If Agent carries the parameter, both mechanisms are conclusively ruled out — fork mode is definitively OFF, not merely "not env-var-enabled." Branch 1 therefore IS conclusive (in the negative), and the claim that "no branch is conclusive in either direction" directly contradicts it. Since Stage 1 never gates and the text says so, this is a calibration issue only — an operator reading this might incorrectly hold open "maybe the server rollout enabled fork mode anyway" when Agent's schema settles it. The two-part fix from prior reviews remains the right call: change "not env-var-enabled" → "off (neither the env var nor the server-side staged rollout is active)", and narrow "no branch is conclusive in either direction" to cover only the other two branches (Agent lacking while Bash keeps it; both lacking).

Fix this →

Security IMPORTANT — proof-question self-validation gap.

SKILL.md L118 — unchanged from 58caf37.

Property 2 reads: "The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone." This is a design constraint on which question to choose. It is NOT a procedural step instructing the main thread to verify compliance before sending the dispatch.

The model must both formulate the question and verify its dispatch message does not embed the answer — in the same generation pass, with no explicit check step. A question chosen to satisfy property 2 in principle may still slip through if the dispatch's incidental context (task description, corrector name, session framing) makes the answer inferable. The recommended one-sentence addition from the prior security review remains absent:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question.

Adding this as a procedural step after the four properties converts property 2 from a selection rule into an enforceable pre-send check. A false positive here seeds a fabricated ledger into step 4's writes — it is the highest-consequence gap remaining in the preflight.

Fix this →


Other notes (no action required)

  • git hash-object platform choice is sound. It's git's own primitive, produces consistent output, and works on every git-backed project without shell pipelines or external tools. The inline note ("git's own primitive, so no shell-specific pipeline is needed") is exactly the right reasoning to record.
  • The non-git gate ("Do not fan out on your own authority") is a significant tightening: the prior "skip and report" would have let the pass run without write verification; the new text requires the user to explicitly accept an unverified fan-out before proceeding. Correctly stronger.
  • Wave-level comparison ("as each wave's ledgers land") means a wave already in flight when a mismatch is detected has already been dispatched. The instruction correctly handles this: "report which members were in flight and that their ledgers are untrusted" — so partially-executed waves are named and quarantined, not silently discarded. Clean.
  • Eval 8's expected_output ("detection rather than prevention, and says a mismatch stops the pass before dedup and correction") now accurately reflects the design. The expectation list is correspondingly updated.
  • CHANGELOG accurately describes the baseline change: "entries AND a per-path content digest, because a fork rewriting an already-dirty file leaves the status letters unchanged." Matches the shipped text.

Overall trajectory: this commit closes the last two structural gaps in the write-verification mechanism (content fidelity and non-git authority) and closes the low-entropy proof gap. Two findings remain open: one calibration issue in Stage 1 prose that never affects runtime behavior, and one procedural gap in the canary dispatch that is the last unfixed path to a fabricated ledger reaching step 4.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (commit fa6d54a)

  • Gather context
  • Read PR diff (fa6d54a vs prior commits)
  • Read sweep-all/SKILL.md at HEAD
  • Evaluate open findings from prior reviews
  • Analyze for new security issues in latest commit
  • Post findings

All changes remain LLM skill instruction files (Markdown) and a JSON evals definition. Security surface is prompt-level: instructions that, if wrong, cause the model to write fabricated corrections to real files. No GitHub Actions workflow files changed.

fa6d54a closes four findings from prior review passes:

  • Content-digest baselinegit status --porcelain entries alone missed rewrites to already-dirty files. The baseline now records git hash-object <path> for every listed path; a fork rewriting an in-flight file changes the hash and trips the check. Correct and well-chosen primitive.
  • Stop before step 3 on write detection — previously the runbook defined no failure branch and carried on correcting. Now: stop, leave the tree, report in-flight ledgers as untrusted, hand the decision to the user. The prior path stacked remedies on top of unauthorized edits; that path is closed.
  • Non-git project fails closed — previously disclosing the gap after the fact. Now: report, degrade to mode 1, or require the user's explicit acceptance of an unverified fan-out before dispatching. The prior path (proceed and note it) protected nothing.
  • Unguessable fourth property — a yes/no or session-common detail would pass the main-thread equality check without proving inheritance. Now requires an exact, high-entropy value. Closes the Codex P1 from the 9dbae2a review.

Net direction of fa6d54a is clearly positive. The write-verification mechanism has moved from "status-only, no failure branch, silent in non-git projects" to a content-level baseline with a defined stop condition and an explicit non-git gate. Two IMPORTANT findings from prior reviews remain open and are detailed below.


IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since 4a8e3ed review, unchanged at HEAD

Property 2 of the proof-question spec (SKILL.md L118–119):

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion, not a procedural verification step. The main thread must simultaneously formulate a compliant question and ensure that the dispatch message it is about to send doesn't inadvertently embed the answer — in a single generation pass, with no explicit check instruction.

A non-inheriting canary that answers from the dispatch message alone passes the main-thread check, its ledger is retained as a real member audit, and all subsequent forks are dispatched against a preflight result produced by a canary that never inherited the conversation. The downstream consequence is the batched correction pass (step 4) operating on a fabricated ledger — the specific failure this preflight exists to prevent.

The fourth property ("cannot be guessed") reduces this exposure by ruling out low-entropy questions, but property 2 is a different axis: a high-entropy answer that the main thread accidentally paraphrases in the dispatch message is both unguessable and leaked.

The one-sentence fix from four prior review passes remains absent: "Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question."

Permalink: SKILL.md L107–131

Fix this →


IMPORTANT — Bootstrapping gap: post-canary forks can echo the canary's proof answer

Confidence: PLAUSIBLE — open since 58caf37 review, unchanged at HEAD

When the canary passes, its exchange — proof question and answer — is recorded in the main conversation. All subsequent fork dispatches in waves 1 and 2 inherit the full conversation at the point of dispatch, which now contains the canary's verified answer.

The fork dispatch instruction (SKILL.md L206–214) says: "answer the preflight's inheritance-proof question first, then load exactly this ONE corrector's SKILL.md…" — no restriction on the source from which the fork may draw its answer.

A non-inheriting post-canary fork that finds the canary's answer in the inherited transcript can echo it and pass the main-thread check — because the canary's answer IS now in "what this context knows." The fourth property ("cannot be guessed") does not help here: the answer is not guessed; it is read from the conversation.

This gap is structurally distinct from the self-validation gap: the self-validation gap concerns the dispatch message leaking the answer before the canary runs; this gap concerns the canary's response seeding the answer for all forks that run afterward. Both must be closed, because a fixed self-validation gap alone still leaves every post-canary fork exposed via the bootstrapping path.

Concrete path from the observed incident (2-of-8 inheriting): the 2 inheriting forks include the canary; it passes and its answer is in the conversation. The 6 non-inheriting forks in waves 1–2 each see the canary's answer in the history they inherit, echo it, pass the check, and their fabricated ledgers proceed to step 3 dedup and step 4 correction writes.

The per-fork dispatch instruction needs one addition: "Answer from the original pre-sweep session content only — do not cite or derive your answer from any prior audit exchange (canary or otherwise) visible in the inherited conversation."

Permalink: SKILL.md L205–214

Fix this →


Other notes (no action required)

  • git hash-object is the correct primitive. It hashes working-tree content, is a stable plumbing command, and requires no shell-specific pipeline. A fork rewriting an already-modified file will change the hash and trip the comparison.
  • Stage 1 F2 (Branch 1 understates conclusiveness) — Lines 76–83 still read "no branch is conclusive in either direction" while Branch 1 (Agent still carrying run_in_background) is actually conclusive-negative. Unchanged from prior reviews; noted for completeness. Since Stage 1 never gates, this is calibration only, not a security issue.
  • The new non-git path is correctly fail-closed. "Do not fan out on your own authority" plus "either run mode 1 or dispatch only on the user's explicit acceptance" is the right framing: it requires an affirmative user decision, not a default continuation.
  • The CHANGELOG entry for fa6d54a accurately describes all four resolved findings and correctly characterizes the verification as detection rather than prevention.

Summary. Four prior findings closed; two IMPORTANT findings remain. The self-validation gap and bootstrapping gap compound each other — fixing one without the other leaves the preflight half-closed, because a fixed canary can still seed a false pass for every fork that follows it. Both require one sentence each; they are not architectural changes.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa6d54a25e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
…nstead of hardening it again

Three consecutive review rounds each found a correct hole in the before/after
working-tree digest protocol: baseline taken after the canary, status entries
that do not move when an already-dirty file is rewritten, a tripped check that
corrected anyway, an undefined non-git branch, no comparison at the canary-return
boundary, and `?? dir/` entries `git hash-object` cannot digest.

Six holes in one prose-specified protocol is the signal, not a to-do list. The
mechanism was additive: #1622's commissioned remediation is that the skill stop
implying the no-writes rule is enforced, and that ships here. Capture, digest,
and diff are deterministic sub-work that belongs in a script the skill calls —
this plugin's own script-the-deterministic-work position — reusing the
repository's existing state-digest contract rather than standing up a second
parallel one in prose, per reuse-or-replace.

What stays: forks receive the main conversation's exact tool pool and cannot be
tool-restricted, so the no-writes rule is trusted rather than enforced; a fork
that wrote is untrusted output to stop on, not to correct on top of; any check
is detection after the fact, never prevention.

Refs #1631

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit 30895cd)

  • Gather context — reviewing commit 30895cd against prior review passes on fa6d54a
  • Read updated sweep-all/SKILL.md at HEAD
  • Diff 30895cd against fa6d54a
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit 30895cd ("refactor(discipline): split the audit-fork write check out to #1631 instead of hardening it again") makes one principled scoping decision and two supporting updates. Here is the full accounting.


What changed

The detailed working-tree baseline protocol — git status --porcelain entries plus git hash-object per-path content digests, along with the baseline-timing, non-git gate, and stop-before-step-3 failure branch — is removed from the runbook. In its place:

  • Step 1 now carries a single sentence: "Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it."
  • The Gotcha is updated to say: "If you want assurance that the fan-out honored it, capture the working tree's state before the FIRST dispatch (the canary included) and compare afterwards... That is detection after the fact, not prevention, and a robust comparison is more than a git status diff — specifying one is tracked in #1631 rather than half-specified here."
  • CHANGELOG documents the split and its rationale: "a prose-specified before/after digest protocol drew a correct review finding in three consecutive rounds, which is the signal that deterministic sub-work belongs in a script the skill calls."
  • Eval 8 is updated to match — no longer tests for a specific baseline protocol, tests that the skill frames writes as "untrusted output to stop on."

Assessment of the split decision

The rationale is sound and self-aware. Three consecutive review rounds each produced a correct finding against the prose-specified baseline protocol (canary timing, content vs. status, untracked directories). That pattern correctly identifies the protocol as deterministic sub-work that belongs in a script — the plugin's own script-the-deterministic-work position — rather than as a fragile prose spec that can't be mechanically validated. Deferring to #1631 keeps the current PR scoped to the preflight and proof mechanism. The Gotcha gives enough advisory guidance for an operator who wants detection today, and the failure mode ("stop rather than correct on top of it") is still stated — only the detection mechanism is deferred.

One implication worth noting: without a specified capture mechanism, the "stop rather than correct" instruction in step 1 is only actionable if the operator reads the Gotcha and independently implements capture. This is deliberate and the CHANGELOG says so. No finding — the scoping is principled and the deferral is documented with its reasoning.


Prior findings — resolved by this commit (now moot)

Codex P1 on fa6d54a — check baseline immediately after the canary.
Moot. The baseline protocol is split to #1631; no baseline capture remains in the runbook for the timing concern to apply to.

Codex P2 on fa6d54a — enumerate files inside untracked directories.
Moot. Same reason.

Code Review (9dbae2a) — git status --porcelain cannot detect content changes to already-dirty files.
Moot. The git hash-object supplement that addressed it, and the underlying --porcelain call, are both gone.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts the closing sentence.

SKILL.md L76–83 — unchanged across all five commits.

Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var path and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are ruled out — fork mode is definitively OFF, and Branch 1 is conclusive in the negative. "No branch is conclusive in either direction" directly contradicts it. Stage 1 never gates, so this is calibration-only — an operator might incorrectly hold open "maybe the server rollout enabled fork mode anyway" when Agent's schema settles it. The two-part fix remains the same: change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", and narrow "no branch is conclusive in either direction" to cover only the other two branches.

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L103–104 — unchanged since 4a8e3ed, open for five commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion, not a procedural verification step. The main thread must simultaneously formulate a compliant question and confirm that the dispatch message it is about to send doesn't inadvertently embed the answer — in one generation pass, with no explicit check instruction. A false canary positive is the highest-consequence failure path: the canary's ledger is retained as real, the remaining forks are dispatched, and fabricated corrections proceed to the step-4 writes.

The one-sentence fix has been consistent across five review passes: "Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question."

Adding this as an explicit step after the four properties converts property 2 from a selection rule into an enforceable pre-send check.

Fix this →


Security IMPORTANT — Bootstrapping gap: post-canary forks can echo the canary's proof answer.

SKILL.md L184–188 — unchanged since 58caf37, open for four commits.

Instruct each fork: answer the preflight's inheritance-proof question first…

When the canary passes, its exchange — proof question and answer — is recorded in the main conversation. All subsequent fork dispatches in waves 1 and 2 inherit the full conversation at the point of dispatch, which now includes the canary's verified answer. A non-inheriting post-canary fork that finds the canary's answer in the inherited transcript can echo it verbatim and pass the main-thread check, because the canary's answer IS now in "what this context knows." The fourth property ("cannot be guessed") does not help here — the answer is not guessed, it is read from conversation history.

The per-fork dispatch instruction needs one addition: "Answer from the original pre-sweep session content only — do not cite or derive your answer from any prior audit exchange (canary or otherwise) visible in the inherited conversation."

This is structurally distinct from the self-validation gap: that gap concerns the dispatch message leaking the answer before the canary runs; this concerns the canary's response seeding the answer for all forks that run afterward. Both must be closed — a fixed self-validation gap alone still leaves every post-canary fork exposed via the bootstrapping path.

Fix this →


Other notes (no action required)

  • The Gotcha advisory ("If you want assurance… capture the working tree's state") is the right residual posture: it gives an operator enough to act on today, names #1631 as the tracked home for a robust spec, and does not embed a fragile prose protocol that draws correct findings every review round.
  • Eval 8 is accurately updated. The expectations list no longer claims a specific baseline mechanism; the expectation that matters ("Does not claim the fan-out's read-only posture is enforced by the harness") is preserved.
  • CHANGELOG is accurate and the discipline:sweep-all — specify the audit-fork write check as a reusable digest, not prose #1631 split rationale is documented. The script-the-deterministic-work framing is the correct lens for why the prose spec never stabilized.
  • The step-1 failure mode ("stop rather than correct on top of it") is the right outcome whether or not a detection mechanism is specified — it governs behavior once a write is known, regardless of how it was detected.
  • Overall trajectory: The split decision is the right call given the pattern. Two security findings remain open and both require one sentence each; they are the last unfixed paths to a fabricated ledger reaching step 4.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (commit 30895cd)

  • Read PR diff at HEAD 30895cd
  • Read sweep-all/SKILL.md at HEAD
  • Evaluate open findings from prior reviews (self-validation gap, bootstrapping gap)
  • Analyze for new security issues introduced by the latest commit
  • Post findings

All changes are LLM skill instruction files (Markdown) and a JSON evals definition. Security surface is prompt-level: instructions that, if wrong, cause the model to write fabricated corrections to real files. No GitHub Actions workflow files changed. I defer to zizmor's lane for supply-chain, trigger patterns, and permissions.

What 30895cd did. The commit removes the mandatory before/after working-tree digest protocol — baseline, git status --porcelain + git hash-object, per-wave comparison, stop-before-step-3 branch, and non-git gate — and defers the specification of a robust comparison mechanism to #1631. The CHANGELOG accurately describes the rationale: three consecutive review rounds each found a correct hole in the prose-specified protocol, which is the signal that deterministic sub-work belongs in a script. What remains is the principle: a fork that wrote is untrusted output, stop rather than correct on top of it.

Net direction. The prior commits' write-check mechanism was flawed in ways that accumulated across three review rounds. Removing a bad mechanism that generated a false sense of security is the right call if the removal is accurately characterized — and it is here, in the Gotcha and CHANGELOG. The honest label "trusted, not enforced" is better than "verified (by a method with six holes)." One new security gap is introduced by the removal; two IMPORTANT findings from prior reviews remain open.


NEW — Stop-on-write condition is now orphaned: no mandatory detection path

Severity: IMPORTANT | Confidence: CONFIRMED

Step 1 of the batched pass (SKILL.md L182–184) states:

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotchas section (SKILL.md L318–324) describes the detection mechanism as advisory:

If you want assurance that the fan-out honored it, capture the working tree's state before the FIRST dispatch … and compare afterwards … specifying one is tracked in #1631 rather than half-specified here.

The stop condition now has no mandatory trigger. The rule says "stop when a fork wrote" — but there is no required procedure for determining that a fork wrote. Prior commits (fa6d54a) had a mandatory detection step ("Compare the tree against the preflight's baseline — entries AND per-path digests — as each wave's ledgers land … A mismatch means a fork wrote, so STOP before step 3"). This commit removes that step. Without it, the stop condition can never be invoked, because the skill specifies no mandatory path from "forks return ledgers" to "a fork wrote."

The practical consequence: step 4 (write corrections to the working tree) will run even when a fork violated the no-writes rule, because there is no detection path to trigger the stop. The condition the stop-rule is trying to prevent — corrections applied on top of unauthorized edits — cannot be blocked.

This is a deliberate deferral, not an oversight, and the CHANGELOG acknowledges it. But the skill can ship in this window between 30895cd and #1631 with a stop condition that cannot fire. If #1631 is not merged before this PR is used in production, the write-detection gap is silent and the step-4 write-on-top path is live.

One way to close this without specifying the mechanism: make the step-1 instruction explicitly conditional rather than unconditional — "Detect writes by running the check described in Gotchas; if a check is not available (the write-detection script from #1631 has not shipped), report that the no-writes rule is unverified for this run." This is weaker than a mandatory check but avoids the orphaned stop condition.

Fix this →


IMPORTANT (open from prior reviews) — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since commit 4a8e3ed, unchanged at HEAD

Property 2 of the proof-question spec (SKILL.md L103–104):

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion applied when choosing the question, not a procedural step the main thread takes before sending the dispatch. The model must both formulate a question satisfying all four properties AND verify its own dispatch message doesn't inadvertently embed the answer — in the same generation pass, with no explicit check instruction and no backstop.

A false canary positive seeds the fabricated ledger into step-3 dedup and step-4 writes — the exact outcome this preflight exists to prevent. The fourth property (unguessable, high-entropy answer) narrows the exposure but does not close it: a high-entropy answer that the main thread accidentally paraphrases in the dispatch context is both unguessable from outside the conversation AND derivable from the dispatch message.

The recommended fix from four prior review rounds remains absent. One sentence after the property list closes it:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question.

Permalink: SKILL.md L107–131

Fix this →


IMPORTANT (open from prior reviews) — Bootstrapping gap: post-canary forks can echo the canary's verified answer

Confidence: PLAUSIBLE — open since commit 58caf37, unchanged at HEAD

When the canary passes, its exchange — proof question and verified answer — is recorded in the main conversation. All subsequent fork dispatches in waves 1 and 2 inherit the full conversation at the moment of dispatch, which now contains the canary's answer.

The per-fork dispatch instruction (SKILL.md L185–188) says "answer the preflight's inheritance-proof question first" — no restriction on the source from which a fork may draw its answer. The fourth property ("cannot be guessed") reduces this attack surface but does not close it: the answer is not guessed by a post-canary fork; it is read from the conversation the fork inherits.

Concrete path: canary dispatched first → inherits conversation and passes proof → canary's Q&A now in main conversation → wave-1 forks dispatched, each inheriting the full conversation including the canary exchange → a fork that lacks the ORIGINAL deep history (but has the recent conversation with the canary's answer) echoes the canary's verbatim answer → passes the main-thread check → fabricated ledger proceeds to step-3 dedup and step-4 write.

The fix is one sentence added to the per-fork dispatch instruction:

Answer from the original pre-sweep session content only — do not cite or derive your answer from any prior audit exchange (canary or otherwise) visible in the inherited conversation.

This is distinct from the self-validation gap: that gap concerns the dispatch leaking the answer before the canary runs; this gap concerns the canary response seeding the answer for all forks that run after it. Both must be fixed together — a fixed self-validation gap alone still leaves every post-canary fork exposed via the bootstrapping path.

Permalink: SKILL.md L185–188

Fix this →


Other notes (no action required)

  • Stage 1 F2 calibration issue remains open across all commits: lines 76–83 say "Agent still carrying it means fork mode is not env-var-enabled" then "no branch is conclusive in either direction." Branch 1 is actually conclusive-negative (if Agent carries run_in_background, both the env-var and server-side rollout are documented as removing it, so fork mode is definitively OFF). The "no branch conclusive" claim contradicts this. Since Stage 1 never gates, this is calibration only — not a security issue — but has been noted in every prior review pass.
  • isolation: "worktree" caveats remain accurately documented. No change here.
  • The removal rationale is sound and accurately stated. The CHANGELOG correctly describes the six holes found in three review rounds as the signal that prose-specified deterministic sub-work belongs in a script. The Gotchas section correctly characterizes write detection as advisory pending #1631.

Summary. Three findings: one new IMPORTANT (stop condition orphaned by removal of mandatory detection), two IMPORTANT carried from prior reviews (self-validation gap and bootstrapping gap). The self-validation and bootstrapping gaps compound — fixing one leaves the other open as a full attack path. The write-detection gap is a deliberate deferral that is correctly disclosed; the ask is to make the disclosure more actionable by telling the model what to report when detection is unavailable, rather than leaving the stop condition orphaned.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30895cdcf1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
… instead of dropping the audit

A session that opens straight into a full batch over an already-dirty tree may
hold no detail meeting all four proof properties, yet the working tree holds
exactly the artifacts the forks should audit. Degrading there costs the user the
audit they asked for, for a reason that has a cheap fix.

A fork "inherits everything the main session has at the moment it spawns", so
the main thread emits a fresh high-entropy value into the transcript as an
ordinary tool result before the canary spawns and asks for it back. Unguessable
by construction, present in no file, unavailable to a non-inheriting subagent —
the proof now works at any conversation length.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit 61f0fcd)

  • Gather context — reviewing commit 61f0fcd against prior review passes on 30895cd
  • Read updated sweep-all/SKILL.md at HEAD
  • Diff 61f0fcd against 30895cd
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit 61f0fcd ("mint the inheritance proof when history is too thin, instead of dropping the audit") makes a single targeted change: the "too thin to audit: run mode 1 instead" degradation path is replaced with Mint one — emit a fresh high-entropy value as a main-thread tool result before the canary spawns and ask for it back. The CHANGELOG is updated accordingly; evals are unchanged.


Prior finding — resolved in this commit

Codex P2 — Preserve audits for thin conversations.
Resolved. The prior text ("If the conversation is too thin for any question to satisfy all four, it is also too thin to audit: report that and run mode 1 instead") is replaced by the mint-one path, which is correctly reasoned: since a fork "inherits everything the main session has at the moment it spawns" (cited), a high-entropy value emitted as a tool result before the canary will be present in an inheriting fork's context and absent from a non-inheriting one. The value satisfies all four proof properties by construction — it exists only in the conversation (not a file), the dispatch prompt can and must not mention it, it keys on ordinary inherited material (a tool result), and it is unguessable. Thin history no longer collapses the pass.

One observation worth noting: the paragraph does not specify which tool produces the minted value. "As an ordinary main-thread tool result" is the right framing for a prose skill — a shell call (openssl rand -hex 16, a UUID generator) is the natural implementation and covers the requirement. No action needed, but a reader implementing this for the first time should understand the intent is a tool-executed computation, not an inline model-generated value.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts the closing sentence.

SKILL.md L76–83 — unchanged across all six commits.

Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var path and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are conclusively ruled out — fork mode is definitively OFF, not merely "not env-var-enabled." Branch 1 is conclusive in the negative, and the "no branch is conclusive in either direction" sentence directly contradicts it. Stage 1 never gates, so this is calibration-only, but the same two-part fix remains the right call: change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", and narrow "no branch is conclusive in either direction" to cover only the other two branches (Agent lacking while Bash keeps it; both lacking).

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L103–104 — unchanged since 4a8e3ed, open for six commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion on which question to choose, not a step the main thread takes before sending the dispatch. The mint-one path makes this more natural — the minted value is arbitrary and the main thread is not composing a question around existing content — but it also introduces a new failure mode: if the dispatch message happens to include the minted value (e.g. as part of an automated template, or because the main thread echoes it for logging), a non-inheriting fork can answer correctly. The self-validation gap applies to mint-one as much as to history-based questions.

A false canary positive remains the highest-consequence failure in the preflight — its ledger is retained as a real member audit and becomes the foundation for the step-4 writes. One sentence added after the four properties converts property 2 from a constraint into an enforceable step:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question (or use a different mint).

Fix this →


Security IMPORTANT — Bootstrapping gap: post-canary forks can echo the canary's proof answer.

SKILL.md L191–192 — unchanged since 58caf37, open for five commits.

Instruct each fork: answer the preflight's inheritance-proof question first…

No restriction on the source from which a fork may draw its answer. After the canary passes and its exchange is recorded in the main conversation, all subsequent fork dispatches inherit the full conversation at the point of dispatch — which now includes the canary's verified answer. For the minted-value path specifically: the minted value appears in the conversation as a tool result before the canary runs, and the canary's echo of it appears in the canary's response. Both are in the conversation that wave-1 and wave-2 forks inherit. A fork that lacks the full session history (the failure mode) but still inherits recent context could echo the minted value read from the canary's response.

The fix is one sentence added to the per-fork dispatch instruction: "Answer from the original pre-sweep session content only — do not cite or derive your answer from any prior audit exchange (canary or otherwise) visible in the inherited conversation."

Fix this →


Security IMPORTANT — Stop-on-write condition is orphaned: no mandatory detection path.

SKILL.md L189–191 — new finding from 30895cd security review, still open.

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The stop condition has no mandatory trigger — there is no required procedure for determining that a fork wrote. The Gotchas section describes write detection as advisory ("If you want assurance…") pending #1631. The step-4 correction pass will run regardless, because nothing mandatory triggers the stop. Without a detection path, "stop rather than correct on top of it" is a rule that can never fire.

A minimal close: add one sentence to step 1 making the unverified state explicit — "Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report." This converts the orphaned stop condition into an honest disclosure rather than a silent non-gate.

Fix this →


Other notes (no action required)

  • The mint-one design is sound. A value emitted as a main-thread tool result before the canary is in the conversation at the point of dispatch; a truly non-inheriting fork (no conversation at all, the observed failure mode) cannot produce it. The approach is cleaner than a history-based proof for thin sessions because entropy is guaranteed at construction rather than relying on session content that may not have accumulated.
  • CHANGELOG accurately reflects the change — the four-property summary and the thin-session mint are both described correctly. No version bump change (prose-only delta within 0.10.0).
  • Evals are unchanged — eval 7 and eval 8 continue to cover the fork-unavailable degrade and the no-writes framing. The thin-session/mint path does not have an eval, consistent with the existing pattern for mint-and-proof mechanics. An eval for "main thread mints a value and asks for it back" would require injecting a tool-result observation into the test, which is outside the eval format's reach — same limitation as eval 7.
  • The two remaining security IMPORTANT findings compound each other. The self-validation gap allows the canary itself to be spoofed; the bootstrapping gap allows every post-canary fork to be spoofed from the canary's recorded answer. Fixing one without the other leaves the preflight half-closed. The mint-one approach introduces slightly more nuance to both (the minted value is also in post-canary conversation history), making the two-sentence fix more useful, not less.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (commit 61f0fcd)

  • Read PR diff at HEAD 61f0fcd
  • Read sweep-all/SKILL.md at current HEAD
  • Evaluate open findings from prior reviews
  • Analyze new commit 61f0fcd for new security issues
  • Post findings

What 61f0fcd did. The thin-conversation fallback changed from "degrade to mode 1" → "mint a fresh high-entropy value as an ordinary main-thread tool result before the canary spawns, then ask for it back." This is the correct fix for the Codex P2 finding ("preserve audits for thin conversations") — minting a proof token means inheritance can be verified at any conversation length.

Net direction is positive: the audit is no longer silently dropped when the session is short. Three prior findings remain open, and one new concern is introduced by the minting path.


IMPORTANT — Minting compounds the self-validation gap at the most dangerous moment

Confidence: PLAUSIBLE — new variant, introduced by 61f0fcd

The self-validation gap (Property 2 is a selection constraint, not a procedural check) has been open since 4a8e3ed. With the minting mechanism, it is harder to catch than it was with pre-existing conversation history.

With the original approach, the main thread is choosing a question about something that happened earlier in the session — the answer is in the past, and the model can reason about it separately from writing the dispatch. With minting, the sequence is:

  1. Main thread generates proof value V (emits it as a tool result)
  2. Main thread immediately composes the canary dispatch asking the fork to recall V

V is the most salient thing in the model's context at step 2. Property 2 says the dispatch must not contain or paraphrase the answer — but there is still no procedural check instruction before sending. The model must simultaneously hold V fresh in working context and compose a dispatch that doesn't reveal it, with no explicit reminder to verify compliance before dispatching. The risk of accidentally embedding or paraphrasing V in the dispatch text is measurably higher here than when the proof question concerns something from earlier in the session.

A canary that answers from an accidentally-revealing dispatch is a false positive. Its retained ledger is a fabricated member audit, and it feeds into step 3 (dedup) and step 4 (write corrections to the working tree) — the exact outcome the preflight exists to prevent.

The fix from five prior review rounds is unchanged and still absent. One sentence after the four properties closes it for both the minting path and the original path:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — and if the answer is the minted value, ensure the dispatch asks for it by referencing where it appears ("the value in the most recent tool result") rather than embedding or restating it.

Permalink: SKILL.md L103–125

Fix this →


IMPORTANT (open from 58caf37) — Bootstrapping gap: minted proofs are equally exposed after the canary passes

Confidence: PLAUSIBLE — unchanged at HEAD

When the session is thin and the main thread mints value V, V enters the conversation as a tool result. The canary inherits the conversation, sees V, and reports it back. After that exchange, V appears in the conversation in TWO places: the original tool result (the mint event) and the canary's reported answer.

All subsequent fork dispatches inherit the conversation at the point of dispatch — which now contains both occurrences of V. A fork that achieves only partial inheritance (recent conversation rather than the full original transcript — the failure mode from the observed incident) would see V in the canary's exchange and could echo it as its own proof.

The per-fork dispatch instruction (SKILL.md L191–194) still carries no restriction on sourcing the answer from prior audit exchanges:

Instruct each fork: answer the preflight's inheritance-proof question first, then load exactly this ONE corrector's SKILL.md

For the minting path specifically: the minted value is the ONLY high-entropy proof element in a thin session, so the canary's answer is the only place a partially-inheriting fork could source an answer from — making this path slightly higher-stakes than the original-history-question path (where other prior conversation details are also present and a fork would need to locate the right one).

The one-sentence fix from prior reviews is still absent: add to the per-fork dispatch instruction — "Answer from the original pre-sweep session content only — do not cite or derive your answer from any prior audit exchange (canary or otherwise) visible in the inherited conversation."

Permalink: SKILL.md L191–194

Fix this →


IMPORTANT (open from 30895cd) — Orphaned stop condition: no mandatory write-detection path

Confidence: CONFIRMED — unchanged at HEAD 61f0fcd

Step 1 (SKILL.md L189–191):

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotchas describe write detection as advisory ("if you want assurance…"), with the robust mechanism deferred to #1631. There is no mandatory step between "forks return ledgers" and "dedup / correct" that triggers the stop condition. The instruction says stop when a fork wrote — but the skill specifies no required path for determining that a fork wrote. The stop condition cannot fire in the current runbook.

Without a detection trigger, step 4 (write corrections to the working tree) proceeds unconditionally — including in the case where a fork violated the no-writes rule. The CHANGELOG correctly documents this as a deliberate deferral; the gap is real regardless.

One way to close this without over-specifying the mechanism: make the absence explicit in the runbook so the model doesn't treat the stop condition as implicitly triggered. "Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step 5 report — do not treat its absence as confirmation that no fork wrote."


SUGGESTION — Minting source is unspecified; model-generated strings are not tool results

Confidence: PLAUSIBLE — introduced by 61f0fcd

The instruction reads "emit a fresh high-entropy value into the transcript as an ordinary main-thread tool result." A tool result requires an actual tool call — a Bash invocation of openssl rand -hex 32 produces true randomness as a tool result. But the instruction does not specify this.

If the model interprets "emit as a tool result" loosely and instead generates a high-entropy-looking string in its response text (not via a tool call), the string is model-generated output, not a tool result. A model-generated string from a similar context could in principle be reproduced — LLMs are deterministic given identical inputs, and while contexts differ across sessions, the guard against fabrication weakens. More concretely: a response containing "minting proof token: 7f3a9c..." is not a tool result; it is model text that the model composed and could plausibly regenerate in a different context.

A single added phrase closes this: "Run a tool call to generate the value (for example, bash('openssl rand -hex 32')) so the result is non-predictable and its source is unambiguous in the conversation."

Permalink: SKILL.md L117–125


Other notes (no action required)

  • The minting approach is the correct design direction. Degrading to mode 1 on thin sessions denied users the sweep they requested. Minting a proof token is structurally sound: a blank fork cannot produce the token, and a truly inheriting fork sees it in the conversation. The security gaps above are incremental refinements, not design rejections.
  • The self-validation and bootstrapping gaps compound. The self-validation gap allows a false-positive canary; the bootstrapping gap allows every post-canary fork to pass based on the canary's recorded answer. Fixing one without the other leaves the preflight half-closed. Both are one-sentence additions.
  • Stage 1 F2 calibration issue (unchanged across all commits): lines 76–83 say "Agent still carrying it means fork mode is not env-var-enabled" then "no branch is conclusive in either direction." If Agent carries run_in_background, both the env-var and server-side rollout are documented as removing it — Branch 1 is definitively conclusive-negative. The "no branch conclusive" claim contradicts this. Not a security issue; noted for completeness.

Summary. Three IMPORTANT findings remain open: the self-validation gap (five review rounds, still absent), the bootstrapping gap (four review rounds), and the orphaned stop condition (three review rounds). The minting mechanism introduced in 61f0fcd compounds the self-validation gap specifically — the minted value is the freshest, most salient item in the model's context when the dispatch is composed, making property 2's constraint harder to comply with and the consequence of a slip higher. All three require short additions; none require design changes.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61f0fcd751

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
Comment thread plugins/discipline/skills/sweep-all/evals/evals.json Outdated
…audits it gates

Folding the canary into the first member's real audit looked free and was not.
A fork inherits everything the session holds when it spawns, so that member's
ledger would land in the main conversation before wave 1 and sit in every later
fork's inherited context — anchoring audits that step 3's dedup requires to be
independent. The skill states that independence one section later; the canary
design contradicted it.

The canary is now one fork that answers the proof and nothing else: no
corrector, no audit, no ledger, nothing that can anchor anyone. It costs one
extra fork and the skill says so plainly rather than claiming the guard is free.

Also makes the failed-canary eval reproducible: it now states the failure
condition in the prompt instead of depending on the operator's fork-rollout
state, which in a fork-enabled session exercised the opposite path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (HEAD 27f3fe8)

  • Gather context and understand the PR
  • Read the PR diff (27f3fe8 vs 61f0fcd)
  • Read changed files at HEAD
  • Evaluate all open findings from prior review passes
  • Analyze new commit for new or resolved security issues
  • Post findings

All changes are LLM skill instruction files (Markdown) and a JSON evals definition. Security surface is prompt-level — instructions that, if wrong, cause the model to fabricate and apply audit corrections to real files. No GitHub Actions workflow files changed.

What 27f3fe8 did. The commit makes the canary proof-only: it carries no corrector, runs no audit, returns no ledger. Previously the canary was "folded into the first member's real audit" — its ledger was retained as a real member result. This commit separates the two duties entirely: the proof canary answers the proof question and nothing else, and the lowest-ranked member is now dispatched in wave 1 like any other member.

The primary motivation (Codex P1 from the 61f0fcd review): a fork inherits everything the session holds when it spawns, so a member ledger returned by the canary before wave 1 would sit in every subsequent fork's inherited context and anchor its audit — exactly the independence step 3 dedup relies on. A proof-only canary returns nothing that can anchor anyone.

Net direction is clearly positive. Two prior review findings are resolved. Two IMPORTANT findings remain open.


Resolved in this commit

Codex P1 — canary ledger anchors post-canary fork audits.
Resolved. The canary now carries no corrector, runs no audit, and returns no ledger. Its exchange in the main conversation is a bare Q&A: the proof question and the fork's answer, nothing more. There is no finding-set, no corrector conclusion, and no remedy content for a later fork to absorb. The "forks stay independent by design" invariant at step 3 is now structurally protected, not merely asserted.

Codex P2 — failed-canary eval cannot force a failed canary.
Resolved. Eval id: 7's prompt now reads: "(Assume the preflight canary comes back reporting no inherited conversation and cannot produce the proof value.)" The failure condition is supplied as an explicit instruction to the evaluator, making the eval's expected-output checkable regardless of the runtime's fork-mode state. The expected output is updated to match the proof-only canary design ("carrying no corrector and returning no ledger, so nothing it returns can anchor a later fork's audit").


IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since commit 4a8e3ed, six review passes, still absent

SKILL.md L109–110:

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection constraint on which question to choose — not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND confirm that the dispatch message it is about to send doesn't inadvertently embed the answer, in one generation pass, with no explicit check instruction and no backstop.

With the proof-only canary, the dispatch is narrower than before — it no longer carries a corrector or audit instructions — but the core vulnerability is unchanged: the minted value V is the most salient thing in context when the dispatch is composed. A dispatch that echoes V (even incidentally, in the framing text) allows a non-inheriting canary to answer from the prompt alone. A false canary positive proceeds immediately to the fan-out, and the batch then writes unproven ledgers' corrections to the working tree.

The one-sentence fix recommended across six review passes is still absent. One sentence after the four properties closes it:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears ("the value in the most recent tool result") rather than restating it.

Fix this →


IMPORTANT — Orphaned stop condition: no mandatory write-detection path

Confidence: CONFIRMED — open since 30895cd, three review passes, unchanged at HEAD

SKILL.md L192–194:

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

SKILL.md L331–335 describes write detection as advisory ("If you want assurance…"), with the robust mechanism deferred to #1631. There is no mandatory step between "forks return ledgers" and "dedup / correct" that triggers the stop condition. The rule says stop when a fork wrote — but the runbook specifies no required path for discovering that a fork wrote.

Without a detection trigger, step 4 proceeds unconditionally. "Stop rather than correct on top of it" cannot fire. The CHANGELOG correctly documents this as a deliberate deferral pending #1631. The ask remains to make the absence explicit in the runbook so the model doesn't treat the orphaned stop condition as implicitly triggered:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Prior IMPORTANT finding — Bootstrapping gap: substantially mitigated, residual risk remains

Revised assessment from prior reviews (previously rated PLAUSIBLE, five review passes)

The bootstrapping gap claimed: once the canary passes and its Q&A is in the main conversation, post-canary forks inherit that exchange and a non-inheriting fork could echo the canary's answer.

27f3fe8 substantially mitigates this. With the proof-only canary, the canary's exchange is a bare Q&A pair — the proof question and the fork's answer. There is no audit ledger, no findings, no content that could anchor a subsequent audit. For the minting path, the minted value V appears in the main conversation at two points: the tool-result mint event and the canary's echo. A fork that inherits zero conversation (the actual observed failure mode — "came back with no inherited conversation") can access neither and cannot echo the answer.

The residual path requires partial inheritance — a fork that receives the recent conversation (including the canary's Q&A) but not the original pre-sweep session. This is a distinct failure mode from the one observed (complete non-inheritance / ledger fabrication from system prompt). No evidence in the PR or incident record that partial inheritance occurs. The preflight's pass/fail logic remains sound against the documented failure mode.

This finding is retained as a disclosure rather than an actionable finding. Adding the source restriction ("Answer from the original pre-sweep session content only — do not cite or derive your answer from any prior audit exchange visible in the inherited conversation") to the per-fork dispatch instruction would close the residual partial-inheritance path at no cost and is still the right long-term posture — but the confidence that this path is exploitable against the actual failure mode is now lower than prior reviews rated it.


Other notes (no action required)

  • The proof-only canary is the correct design direction. The prior design (canary folded into a member audit) looked free and was not — audit independence is what step 3 dedup relies on, and a member ledger in context before wave 1 would have anchored every subsequent fork's perspective. The proof-only canary closes that structural gap cleanly.
  • Step 1's "For each in-scope corrector" (L185) correctly drops the prior "REMAINING" qualifier — since the canary is no longer a member dispatch, no member is excluded from the fan-out.
  • Token cost honesty. The Gotcha now reads "~1.4M tokens for the audit phase alone, plus one more fork for the proof-only canary" — accurately reflecting the added cost rather than claiming the fold was free.
  • Stage 1 F2 calibration issue (unchanged across all seven commits): lines 76–83 say "Agent still carrying it means fork mode is not env-var-enabled" then "no branch is conclusive in either direction." If Agent carries run_in_background, both the env-var and server-side rollout are documented as removing it — Branch 1 is definitively conclusive-negative, contradicting the closing sentence. Stage 1 never gates; this is calibration-only, not a security issue, but has now appeared in every review pass.
  • Fresh-docs compliance: harness citations are present and qualified. No new unverified harness claims introduced.

Summary. Two findings closed by this commit: the audit-anchoring gap (Codex P1) and the failed-canary eval (Codex P2), both direct results of the proof-only canary design. Two IMPORTANT findings remain open: the self-validation gap (a procedural pre-send check that is still absent after six review rounds) and the orphaned stop condition (a rule that cannot fire because its trigger is deferred). The bootstrapping gap is substantially mitigated — the residual path requires an unobserved partial-inheritance failure mode and is disclosed rather than actioned. Both open findings require one sentence each.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit 27f3fe8)

  • Gather context — reviewing commit 27f3fe8 against prior review passes on 61f0fcd
  • Read updated sweep-all/SKILL.md at HEAD
  • Diff 27f3fe8 against 61f0fcd
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit 27f3fe8 ("make the canary proof-only, so it cannot anchor the audits it gates") addresses the Codex P1 from the last review pass and closes the eval-format concern raised at P2. Three files changed: SKILL.md, CHANGELOG.md, and evals.json.


Prior findings — resolved in this commit

Codex P1 (61f0fcd) — Canary ledger anchors later audit forks.
Resolved. The canary now dispatches as a proof-only fork — "no corrector, no audit, no ledger" — with the rationale stated inline: a member ledger returned before wave 1 would sit in every later fork's inherited context and anchor its audit, breaking the independence step 3 relies on. "A proof-only canary returns nothing that can anchor anyone." The independence language in the dedup section now accurately covers both member-to-member anchoring (which it already guarded) and canary-to-member anchoring (which it now guards). The wave-cap step 1 correctly drops the "REMAINING" guard (never dispatch it twice) that was tied to the folded-in design — all N members are dispatched fresh, as they should be. The token-cost note is updated accurately: "plus one more fork for the proof-only canary."

Codex P2 (61f0fcd) — Eval 7 cannot reliably induce a failed canary.
Resolved. The prompt now explicitly injects the failure condition: "(Assume the preflight canary comes back reporting no inherited conversation and cannot produce the proof value.)" The expected output correctly describes the proof-only canary design ("carrying no corrector and returning no ledger, so nothing it returns can anchor a later fork's audit"), and the expectations list is updated to match. This is the right fix for an eval format that cannot inject harness state — explicit premise-setting in the prompt is the only tool available, and it is used correctly.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates what "Agent still carrying run_in_background" establishes.

SKILL.md L76–83 — unchanged across all seven commits.

So Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are ruled out — fork mode is definitively OFF, not merely "not env-var-enabled." Branch 1 IS conclusive in the negative, and "no branch is conclusive in either direction" directly contradicts it. Stage 1 never gates, so this is calibration-only; the two-part fix is unchanged: change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", narrow "no branch is conclusive" to cover only the other two branches.

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L109–110 — unchanged since 4a8e3ed, open for seven commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion, not a procedural step. The proof-only canary design slightly reduces exposure — the dispatch is simpler (no corrector instructions, no audit framing) and therefore has fewer incidental contexts that could inadvertently embed the answer — but the structural gap is unchanged. The model must still formulate a compliant question and confirm that its dispatch message doesn't leak the answer in one generation pass, with no explicit backstop. For the minting path specifically, the minted value is the most salient item in context at dispatch time, and there is no instruction to reference it by position ("the value in the most recent tool result") rather than restating it.

A false canary positive remains the highest-consequence gap: its proof counts as established, all N members are dispatched, and fabricated ledgers proceed to step-4 writes. The one-sentence fix from six prior rounds is still absent: "Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question."

Fix this →


Security IMPORTANT — Bootstrapping gap: post-canary forks can echo the canary's proof answer.

SKILL.md L195–205 — unchanged since 58caf37, open for six commits.

When the canary passes, its exchange — proof question and answer — is recorded in the main conversation. All subsequent fork dispatches inherit the full conversation at the point of dispatch, which now contains the canary's verified answer. The per-fork dispatch instruction says "answer the preflight's inheritance-proof question first" with no restriction on the source from which a fork may draw its answer.

The proof-only canary doesn't close this path — it eliminates member-ledger anchoring (correctly, as the Codex P1 intended), but the canary's proof exchange is still in the conversation when wave-1 forks are dispatched. For the minting path: the minted value V appears in the conversation twice (as a tool result before the canary, and in the canary's response), so a fork with partial inheritance — recent context but not the full original session — can echo V from the canary's exchange. The fourth property ("cannot be guessed") guards against zero-inheritance forks but not against partial-inheritance ones reading V from the conversation.

One sentence added to the per-fork dispatch instruction closes this: "Answer from the original pre-sweep session content only — do not cite or derive your answer from any prior audit exchange (canary or otherwise) visible in the inherited conversation."

Fix this →


Security IMPORTANT — Orphaned stop condition: no mandatory write-detection path.

SKILL.md L193–195 — open since 30895cd, unchanged.

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotcha describes write detection as advisory ("If you want assurance…") pending #1631. There is no mandatory step between "forks return ledgers" and "dedup / correct" that triggers the stop condition — it cannot fire under the current runbook. The step-4 write pass runs unconditionally. A single added sentence makes the absent-detection state explicit: "Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote."

Fix this →


Other notes (no action required)

  • The proof-only canary is the right design. The independence argument is correct and well-stated: a member ledger returned before wave 1 would be in every later fork's inherited context when it spawns; a proof-only canary returns nothing that can anchor anyone. This is a principled decision, not a patch.
  • CHANGELOG is accurate. The description of the fold-not-free argument and the one-extra-fork cost is correct and honest.
  • Token-cost note correctly updates from "eight forks either way" to "plus one more fork for the proof-only canary" — the per-fork cost is now N+1 instead of N, and the note says so clearly.
  • The two remaining security gaps compound. The self-validation gap allows a false-positive canary; the bootstrapping gap allows every post-canary fork to pass based on the canary's recorded answer. Fixing one without the other leaves the preflight half-closed. Both require one sentence each and neither is an architectural change.
  • Eval 8 is unchanged and still accurate: "a fork that wrote is untrusted output to stop on rather than correct on top of."

Overall trajectory. Seven commits, six of which closed findings raised by seven prior review passes. Two IMPORTANT security gaps (self-validation, bootstrapping) and one calibration issue (Stage 1 F2) remain open; one orphaned stop condition from the write-detection deferral remains. All four require short prose additions and none require design changes. The core structural work — the two-stage preflight, the proof-only canary, the honest no-writes framing, the independence guarantee — is sound.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27f3fe8de9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
…at breaks ledger independence

The wave protocol contradicted the independence invariant the dedup depends on,
and the four-fork cap this PR introduced made it worse: it turned a typical
eight-member pass into two waves, so wave 2 inherited wave 1's findings and was
anchored by them. That is the same defect as the canary ledger, generalized.

The fix is a different default rather than a different number. Prefer ONE wave:
dispatch every in-scope member together so no member's ledger is in the
transcript when another spawns. With the never tier excluded and the situational
tier gated, an in-scope set fits inside the documented parallelism budget, so
one wave is usually available. Where it is not, shrink the set before splitting,
and disclose the anchoring when splitting anyway — including that a retried
member inherits its own discarded response.

This also lands F6 more precisely than "four": the original finding was that the
-deep siblings' "roughly a dozen" exceeds CLAUDE_CODE_MAX_TOOL_USE_CONCURRENCY,
and the real reason their number never belonged here is that cheap fresh-context
subagents carry no independence invariant to break.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Claude finished reviewing commit e54696d


Code Review (commit e54696d)

  • Read updated sweep-all/SKILL.md at HEAD
  • Diff e54696d against 27f3fe8
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit e54696d ("prefer one wave, because splitting the fan-out is what breaks ledger independence") addresses the Codex P1 from the 27f3fe8 review pass. One file changed: SKILL.md (the wave-cap section rewritten) and CHANGELOG.md updated to match.


Prior finding — resolved in this commit

Codex P1 (27f3fe8) — Later waves inherit earlier waves' ledgers.
Resolved. The four-fork wave cap is replaced with prefer one wave — dispatch every in-scope member together, so no member's ledger is in the transcript when another spawns. The reasoning is stated inline and is correct: a fork inherits everything the session holds at the moment it spawns, so a wave-2 fork inherits wave-1's completed ledgers and can be anchored by them, contradicting the independence step 3 dedup relies on. "Splitting the fan-out is what breaks it" — accurate, and now the governing rule rather than a known hole.

The fallback hierarchy is sound: prefer one wave → if it doesn't fit, shrink the set (gate the situational tier harder) → only if you must split, split and disclose it in the report. The disclosure requirement is well-motivated: "a retried member additionally inherits its own discarded response" is newly stated and accurate.

The scope claim is checked: with never excluded and situational gated, eight or nine members against MAX_TOOL_USE_CONCURRENCY default 10 is usually tight but plausible. The skill correctly notes the sweep doesn't own that budget alone and points at the CONCURRENT_SUBAGENTS and SUBAGENTS_PER_SESSION budgets too.

The CHANGELOG accurately describes the change: "Splitting is what breaks the ledger independence step 3's dedup assumes — a fork inherits everything the session holds at spawn, so wave 2 reads wave 1's findings." Matches the shipped text.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts the closing sentence.

SKILL.md L76–83 — unchanged across all eight commits.

So Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are definitively ruled out — fork mode is OFF, not merely "not env-var-enabled." Branch 1 IS conclusive in the negative, and "no branch is conclusive in either direction" directly contradicts it. Since Stage 1 never gates, this remains calibration-only — but it has appeared in every one of eight review passes. The two-part fix is unchanged: change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", and narrow the closing sentence to cover only the other two branches.

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L109–110 — unchanged since 4a8e3ed, open for eight commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion on which question to choose, not a procedural step before sending the dispatch. The model must simultaneously formulate a compliant question and verify its own dispatch message doesn't inadvertently embed the answer — in one generation pass, with no explicit check instruction. A false canary positive fans the full member wave out immediately (all in one wave now), and step 4 writes those unproven ledgers' corrections to the working tree.

The one-sentence fix recommended across eight prior passes remains absent:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears rather than restating it.

Fix this →


Security IMPORTANT — Orphaned stop condition: no mandatory write-detection path.

SKILL.md L193–195 — open since 30895cd, unchanged.

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotcha describes write detection as advisory ("If you want assurance…"), with a robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that could trigger the stop condition. The rule says stop when a fork wrote — the runbook specifies no required path for determining that a fork wrote. As before: adding one sentence makes the absent-detection state explicit rather than leaving the stop condition orphaned:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Bootstrapping gap — prior IMPORTANT finding, disclosure retained

The bootstrapping gap (post-canary forks echoing the canary's proof answer) was substantially mitigated in 27f3fe8 by making the canary proof-only — it returns a bare Q&A pair, nothing that can anchor a later fork's audit. The residual path requires partial inheritance (a fork that receives recent conversation but not the original pre-sweep session), which is distinct from the observed complete-non-inheritance failure mode. This commit's switch to one wave does not change this assessment. Retained as a disclosure only; not an actionable finding at current confidence.


Other notes (no action required)

  • The prefer-one-wave default is the correct architectural choice. The four-fork cap was a constraint borrowed from siblings that carry no independence invariant; this commit gives the right reason and the right default.
  • "Gate the situational tier harder" is intentionally left unspecified — it means tighten the relevance threshold, and the exact threshold is operator judgment. Correct to leave that flexible.
  • The checkpoint and retry text remains accurate and is now correctly scoped inside the "If you must split" branch rather than as a universal rule, which avoids implying mid-wave checkpointing is needed for single-wave runs.
  • The two remaining security findings compound. The self-validation gap allows a false-positive canary; a false canary positive now fans the entire member set out in one dispatch rather than one wave at a time — increasing the blast radius if the canary is spoofed. This makes fixing the self-validation gap marginally more urgent than it was under the prior wave-capped design.

Overall trajectory. Eight commits, seven of which closed findings raised across the prior review passes. One Codex finding resolved; two security IMPORTANT gaps (self-validation, orphaned stop condition) and one calibration issue (Stage 1 F2) remain open. All three require short prose additions; none require design changes.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (commit e54696d)

  • Read PR diff at HEAD e54696d
  • Read sweep-all/SKILL.md at HEAD
  • Evaluate open findings from prior reviews
  • Analyze new commit for new or resolved security issues
  • Post findings

All changes are LLM skill instruction files (Markdown). No GitHub Actions workflow files changed. Security surface is prompt-level — instructions that, if wrong, cause the model to fabricate and apply audit corrections to real files. I defer to zizmor's lane for supply-chain, trigger patterns, and permissions.

What e54696d did. The commit replaces "bounded waves of at most FOUR forks" with "prefer ONE wave: dispatch every in-scope member together, so no member's ledger is in the transcript when another member spawns." This directly addresses the Codex P1 from the 27f3fe8 review (post-canary forks can see earlier waves' ledgers). The independence rationale is stated inline, the preferred fallback when one wave doesn't fit is to shrink the in-scope set before splitting, and when splitting is necessary the runbook now requires disclosure rather than silence.

Net direction is positive. The multi-wave anchoring concern is resolved. The independence invariant that step 3 and step 4 rely on is now structurally preferred, not merely asserted. Three prior findings remain open.


Resolved in this commit

Codex P1 (27f3fe8) — later waves inherit earlier waves' ledgers.
Resolved. The four-fork wave cap is replaced with a one-wave preference and a disclosure requirement when splitting is unavoidable. The commit message states the reasoning precisely: "splitting the fan-out is what breaks" the independence invariant, and the "roughly a dozen" number never belonged here because it was calibrated for fresh-context subagents that carry no such invariant. Preferring to shrink the set over splitting is the right security posture — fewer fully-independent audits beats more anchored ones.


IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since commit 4a8e3ed, eight review passes, still absent

SKILL.md L109–110:

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion the main thread applies when choosing a question. It is not a step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify that the dispatch message it is about to send doesn't inadvertently embed the answer — in one generation pass, with no explicit backstop.

For the minting path specifically (the one now used for thin sessions): the minted value V is the freshest, most salient item in context at dispatch time. A dispatch that echoes or paraphrases V — even incidentally, in framing text — allows a non-inheriting canary to answer from the prompt alone. The one-wave default makes this higher-stakes: a false canary positive now fans out ALL members at once. A fabricated ledger from every member proceeds simultaneously to step 3 dedup and step 4 correction writes.

The one-sentence fix recommended across eight review passes remains absent:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears ("the value in the most recent tool result") rather than restating it.

Fix this →


IMPORTANT — Orphaned stop condition: no mandatory write-detection path

Confidence: CONFIRMED — open since 30895cd, four review passes, unchanged at HEAD

SKILL.md L193–195:

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotchas section (SKILL.md L346–351) describes write detection as advisory, with the robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop. The stop condition cannot fire under the current runbook; step 4 proceeds unconditionally.

The one-wave default makes this slightly more urgent than before: all forks run simultaneously, so if any one of them writes, the detection window is the same batch as the correction pass — there is no inter-wave checkpoint at which to halt before a subsequent wave starts.

The ask from prior reviews is unchanged: make the absent-detection state explicit rather than leaving the stop condition orphaned:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


SUGGESTION — Retry inheritance now exposes the bootstrapping gap in a new variant

Confidence: PLAUSIBLE — new nuance introduced by e54696d

SKILL.md L230–235:

a retried member additionally inherits its own discarded response. That is a real weakening of the independence the dedup relies on, and it is disclosed, not hidden.

This is correctly disclosed. However, the bootstrapping gap's per-fork source restriction — previously flagged across five review rounds but substantially downgraded after the proof-only canary — applies with more force here than in the canary case.

A member dispatched for retry inherits its own prior response. If that prior response contained a fabricated or guessed proof answer, the retry's context now contains both the question AND the previously attempted answer. The per-fork dispatch instruction (SKILL.md L195–196) says "answer the preflight's inheritance-proof question first" with no restriction on sourcing from prior audit exchanges. The retried member can echo its own prior answer verbatim.

The four proof properties govern question selection, not fork answer sourcing. A retried member that copies its prior (failed) answer rather than re-deriving from the original session content is not caught by the fail-closed rule unless the main thread's check specifically compares the retry's answer to the prior attempt.

The one-sentence per-fork dispatch addition from prior reviews would close this case too: "Answer from the original pre-sweep session content only — do not cite or derive your answer from any prior audit exchange (canary or otherwise, including your own discarded response if this is a retry) visible in the inherited conversation."

This is SUGGESTION rather than IMPORTANT because: (a) the retry only occurs for a member that already returned unproven — so the failure is already known — and (b) a retry that also returns unproven is reported open with verified ledgers preserved, per the preflight's mid-fan-out rule. The escalation path (reported open) is defined; the gap is that a retry might falsely pass verification rather than fail it honestly.


Other notes (no action required)

  • The one-wave preference is the correct security posture. Fewer independent audits beats more anchored ones. The shrink-before-split preference keeps this default available in practice for typical in-scope set sizes (8–9 after tier exclusion), while the disclosure requirement ensures any split is visible rather than silent.
  • The disclosure when splitting ("every member in a later wave inherits the earlier waves' ledgers and can be anchored by them") is accurate and honest. No security gap here — the weakening is named and surfaced to the user.
  • Stage 1 F2 (lines 76–83: "not env-var-enabled" + "no branch conclusive") remains a calibration issue across all eight commits. Not a security finding.
  • Bootstrapping gap (prior IMPORTANT, substantially mitigated): the one-wave default removes the primary exposure path (post-canary forks in subsequent waves seeing the canary's answer before dispatch). The residual path is the retry variant described in the SUGGESTION above. Downgraded from IMPORTANT at this commit.
  • Fresh-docs compliance: No new harness claims in this commit. The existing citations remain accurate.

Summary. e54696d resolves the multi-wave anchoring concern correctly and completely — preferring one wave over splitting is the design that holds the independence invariant intact by default. Two IMPORTANT findings remain open: the self-validation gap (eight rounds, one sentence) and the orphaned stop condition (four rounds, deferred to #1631). The bootstrapping gap is now substantially addressed by both the proof-only canary and the one-wave default; the retry-inheritance variant is flagged as a SUGGESTION. The two IMPORTANT findings are the last unfixed paths to a fabricated ledger reaching step 4.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e54696dc86

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
…ng only applies to a split

Two holes the one-wave rewrite left in the path it recommends.

- The retry-anchoring disclosure was scoped to the split branch, but a retry is
  anchored in a single wave as well — and worse there: it spawns after every
  other member's ledger has landed, so it inherits all of them. Moved to the
  retry rule itself, with the trade named: re-running the pass clean is often
  worth more than one anchored ledger.
- "Checkpoint after each wave" was left verbatim in a section whose first
  instruction is to use one wave, where there is no partial state to lose. It
  now attaches to the split fallback, which is the only case it serves.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit a382666)

  • Gather context — reviewing commit a382666 against prior review pass on e54696d
  • Read updated sweep-all/SKILL.md at HEAD
  • Diff a382666 against e54696d
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit a382666 ("a retry is anchored in one wave too, and checkpointing only applies to a split") fixes two holes the one-wave rewrite left in the path it recommends. One file changed: SKILL.md.


Resolved in this commit

Retry anchoring scoped to split branch only (implied Codex P1 from e54696d).
Resolved. The prior text placed the anchoring disclosure ("a retried member additionally inherits its own discarded response") in the split-fan-out paragraph, implying it was only a concern when waves existed. But in a single-wave pass a retry is anchored too — it spawns after every other member's ledger has already landed, so it inherits all of them. The commit correctly moves the disclosure to the retry rule itself: "A retry is always anchored, one wave or many: it spawns after other members' ledgers have landed, so it inherits them." The trade-off guidance ("where the batch can afford it, re-running the whole pass clean is worth more than one anchored ledger") is a useful addition — operators now have explicit direction to prefer a clean re-run over accepting an anchored result.

Checkpointing scoped to split fallback only.
Resolved. "Checkpoint the collected ledgers after each wave" belonged only to the split case — a single wave has no partial state to lose mid-pass. Attaching it to the one-wave default made the instruction misleading and the one-wave preference look more operationally complex than it is. Now correctly placed in the split paragraph.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates what "Agent still carrying run_in_background" establishes.

SKILL.md L76–83 — unchanged across all nine commits.

So Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var path and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are definitively ruled out — fork mode is OFF by either means, not merely "not env-var-enabled." Branch 1 IS conclusive in the negative, and "no branch is conclusive in either direction" directly contradicts it. Stage 1 never gates, so this is calibration-only — noted for completeness in every prior review. The two-part fix is unchanged: (1) change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", (2) narrow "no branch is conclusive in either direction" to "the remaining two branches are not conclusive in either direction."

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L109–110 — unchanged since 4a8e3ed, open across nine commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion on which question to choose, not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify that the dispatch message it is about to send doesn't inadvertently embed the answer — in one generation pass, with no explicit check instruction and no backstop. For the minting path specifically, the minted value V is the most salient item in context when the dispatch is composed; the one-wave default means a false canary positive fans ALL members out at once.

The one-sentence fix from nine prior review rounds is still absent. One sentence after the four properties closes it:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears rather than restating it.

Fix this →


Security IMPORTANT — Orphaned stop condition: no mandatory write-detection path.

SKILL.md L193–195 — open since 30895cd, unchanged.

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotchas describe write detection as advisory ("If you want assurance…"), with a robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop. The stop condition cannot fire under the current runbook; step 4 proceeds unconditionally. Adding one sentence makes the absent-detection state explicit rather than leaving the stop condition orphaned:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Unaddressed Codex P2 (from e54696d review)

SKILL.md L230–231 — Codex flagged at e54696d, no response yet.

Where the in-scope set does not fit, prefer shrinking it — gate the situational tier harder — over splitting.

The concern is that this instruction asks the router to reclassify a corrector the membership rules already deemed relevant, so an explicitly requested full sweep can silently miss findings by adjusting relevance to fit a concurrency budget. Whether this is an acceptable trade-off is a design question — the independence invariant is why the preference exists — but the current text doesn't acknowledge the cost of the shrink path the way it acknowledges the cost of the split path. One sentence noting that shrinking reduces coverage would give the operator a symmetric disclosure: "Note that shrinking the set means deferring relevant findings — some correctors will not run, and their ledgers will not appear in this pass's report."


Other notes (no action required)

  • "Re-running the whole pass clean is worth more than one anchored ledger" is the right guidance. It gives operators an explicit escape hatch from the retry trap: if the anchoring weakens the result too much, discard and re-run. Clean, and correctly positioned as a judgment call rather than a mandate.
  • "Only a split fan-out needs the -deep siblings' per-wave checkpointing" is accurate and removes a misleading implied cost from the one-wave path. The prior text made it sound like checkpointing was universal; now it's correctly tied to the case where partial state can be lost.
  • The bootstrapping gap (prior IMPORTANT, substantially mitigated in 27f3fe8): the one-wave default further reduces its attack surface — no post-canary inter-wave window exists for a partially-inheriting fork to observe the canary's exchange before dispatch. Retained as a disclosure; no longer an actionable finding at current confidence. The retry variant (flagged as SUGGESTION in the e54696d security review) is addressed by the new retry-anchoring disclosure: "Say so for each retried member" makes the anchoring visible, and the clean-re-run option provides the right escape.
  • The two remaining IMPORTANT security findings compound. A false canary positive from the self-validation gap fans ALL members in one wave; the orphaned stop condition means the correction pass runs unconditionally on whatever those members return. Both require one sentence each.

Overall trajectory. Nine commits, eight of which closed findings from prior review passes. Two fixes in this commit (retry-anchoring placement, checkpointing scope) are both correct and improve the one-wave section's clarity. Two IMPORTANT security findings remain open: the self-validation gap (nine rounds) and the orphaned stop condition (five rounds). Both are one-sentence additions; neither requires a design change.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (HEAD a382666)

  • Read PR diff at HEAD a382666
  • Read sweep-all/SKILL.md at HEAD
  • Evaluate open findings from prior reviews
  • Analyze new commit for new or resolved security issues
  • Post findings

All changes are LLM skill instruction files (Markdown). No GitHub Actions workflow files changed. Security surface is prompt-level — instructions that, if wrong, cause the model to fabricate and apply audit corrections to real files. I defer to zizmor's lane for supply-chain, trigger patterns, and permissions.

What a382666 did. The commit makes a focused change to the retry/checkpointing section. Per-wave checkpointing is scoped explicitly to split fan-outs ("Only a split fan-out needs the -deep siblings' per-wave checkpointing; a single wave has no partial state to lose"). A new "Retry, and what counts as a failure" paragraph explicitly names that retries are always anchored regardless of wave structure: "A retry is always anchored, one wave or many: it spawns after other members' ledgers have landed, so it inherits them." The prior text's specific disclosure of "a retried member additionally inherits its own discarded response" is removed in favour of this more general anchoring statement.

Net direction is neutral. No prior security findings are resolved. Two IMPORTANT findings remain open. One minor disclosure regression is noted.


IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since commit 4a8e3ed, nine review passes, unchanged at HEAD

SKILL.md L109–110:

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion applied when choosing the proof question. It is not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify its own dispatch message doesn't inadvertently embed the answer — in one generation pass, with no explicit check instruction and no backstop.

With the minting path (introduced in 61f0fcd), this exposure is heightened: the minted value V is the freshest, most salient item in the model's context at dispatch time. A dispatch that echoes or paraphrases V — even incidentally, in framing text — allows a non-inheriting canary to answer from the prompt alone. The one-wave default compounds this: a false canary positive fans out ALL members simultaneously, and their fabricated ledgers all proceed to step-3 dedup and step-4 correction writes.

The one-sentence fix recommended across nine prior review passes remains absent. One sentence after the four properties converts property 2 from a selection rule into an enforceable pre-send check:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears ("the value in the most recent tool result") rather than restating it.

Fix this →


IMPORTANT — Orphaned stop condition: no mandatory write-detection path

Confidence: CONFIRMED — open since commit 30895cd, five review passes, unchanged at HEAD

SKILL.md L193–195:

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

SKILL.md L349–355 describes write detection as advisory ("If you want assurance…"), with the robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that could trigger the stop condition. The runbook specifies no required path for determining that a fork wrote.

The stop condition cannot fire under the current runbook. Step 4 proceeds unconditionally. The CHANGELOG correctly documents this as a deliberate deferral pending #1631; the ask remains to make the absent-detection state explicit rather than leaving the stop condition orphaned:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Minor disclosure regression — own-response anchoring removed from retry section

Severity: SUGGESTION | Confidence: CONFIRMED

The prior text (e54696d) explicitly disclosed: "a retried member additionally inherits its own discarded response." This commit removes that sentence and replaces it with the more general "A retry is always anchored, one wave or many: it spawns after other members' ledgers have landed, so it inherits them." The general anchoring statement is correct — a retry inherits other members' completed ledgers — but the specific own-response inheritance is now undisclosed.

A member retried after returning an unproven ledger has both its failed response and other members' verified ledgers in its inherited context. An operator reading the current text knows the retry is anchored to the other ledgers, but not that it is also anchored to its own prior (failed) attempt. For the bootstrapping-gap case specifically — a retry that submits the same plausible-but-unproven answer again — the own-response inheritance is the mechanism. The prior text disclosed this; the current text does not.

This is a suggestion rather than a security gap because: (1) a retry still must satisfy the proof check, so a repeated unproven answer fails again and is reported open; (2) the anchoring disclosure ("Say so for each retried member") is present and correct. The own-response detail is worth restoring as one added phrase: "A retry also inherits its own prior response — the member's failed attempt is in the main conversation when the retry spawns."


Prior IMPORTANT finding — Bootstrapping gap: status

Revised assessment from prior reviews (previously PLAUSIBLE, substantially mitigated at 27f3fe8)

The bootstrapping gap concerned post-canary forks echoing the canary's proof answer from the inherited conversation. With the proof-only canary design (no ledger, bare Q&A), the canary's exchange in the main conversation contains only the proof question and the fork's answer to it. For the minting path, V appears twice (mint event and canary echo). A retry that inherits this must still satisfy the proof check independently — and any fork with zero inheritance cannot produce V at all. The residual path requires partial inheritance (recent context but not full original history), which is distinct from the observed complete-non-inheritance failure mode and has no evidence of occurrence. Retained as a disclosure; not an actionable finding at current confidence.


Other notes (no action required)

  • The retry-anchoring disclosure ("A retry is always anchored, one wave or many") is the correct generalization — it captures both the split-wave case (where prior editions had it) and the single-wave case (where it was absent). Explicitly recommending that the operator weigh whether re-running the whole pass clean is better than one anchored ledger is sound guidance.
  • Checkpointing scoped to split fan-outs is correct. A single wave either completes or doesn't; there is no partial state to checkpoint. Removing the checkpointing instruction from the general case eliminates a spurious implication that every wave is at risk of mid-pass loss.
  • Stage 1 F2 calibration issue (lines 76–83): "Agent still carrying it means fork mode is not env-var-enabled" + "no branch is conclusive in either direction." If Agent carries run_in_background, both the env-var and server-side rollout are documented as removing it — Branch 1 is definitively conclusive-negative, contradicting the closing sentence. Not a security issue; Stage 1 never gates. Has appeared in every review pass and remains a calibration-only note.
  • Fresh-docs compliance: No new harness claims in this commit.

Summary. a382666 makes a targeted correction to the retry/checkpointing section and introduces no new security gaps. Two IMPORTANT findings remain open across all nine commits: the self-validation gap (one sentence, absent nine rounds) and the orphaned stop condition (one sentence, absent five rounds). Both are the last unfixed paths to fabricated content reaching the step-4 writes. A minor disclosure regression — the own-response anchoring on retry — is flagged as a suggestion.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a382666550

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
… budget

The one-wave rewrite told the router to "gate the situational tier harder" when
the in-scope set did not fit. That instructs it to change relevance to satisfy a
budget — corrupting membership resolution and silently costing coverage on a
sweep the user explicitly asked for.

It also rested on a conflation of two documented limits, now corrected against
the fetched pages: CLAUDE_CODE_MAX_TOOL_USE_CONCURRENCY (default 10) caps how
many run at once, not how many you dispatch; CLAUDE_CODE_MAX_CONCURRENT_SUBAGENTS
(default 20) is the hard spawn failure. An 11-member fully-admitted set was never
over the limit that matters, so the shrink instruction was solving a problem that
did not exist.

Membership decides scope; concurrency decides only timing. If the session cannot
dispatch the whole set right now, wait for capacity and dispatch it whole. Split
only if that is impossible, and disclose the anchoring.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit 1d3dfc3)

  • Read current sweep-all/SKILL.md at HEAD
  • Diff against prior commit (a382666)
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit 1d3dfc3 ("never drop a relevant corrector to fit a concurrency budget") addresses the Codex P2 from the a382666 review. One file changed in the skill: the wave-sizing paragraph is rewritten. CHANGELOG.md is updated to match.


Prior finding — resolved in this commit

Codex P2 (a382666) — Do not drop relevant correctors to fit concurrency.
Resolved. The prior instruction — "where the in-scope set does not fit, prefer shrinking it — gate the situational tier harder" — made membership resolution bend to a runtime budget, silently dropping findings a full sweep was commissioned to surface. The replacement is correct on two axes: (1) it distinguishes the two documented limits, which the prior text conflated — CLAUDE_CODE_MAX_TOOL_USE_CONCURRENCY caps concurrent execution but not the dispatch count, while CLAUDE_CODE_MAX_CONCURRENT_SUBAGENTS is the hard spawn-failure limit — and correctly observes that a fully-admitted set (core + all situationals) fits inside MAX_CONCURRENT_SUBAGENTS in an otherwise-quiet session; (2) it states the invariant plainly: "Never drop a relevant corrector to fit a budget. Membership resolution decides what is in scope; concurrency decides only when it runs." The wait-for-capacity instruction is the right fallback and does not need to specify the mechanism further at prose level.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts the closing sentence.

SKILL.md L76–83 — unchanged across all ten commits.

So Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var path and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are definitively ruled out — fork mode is OFF by either means, not merely "not env-var-enabled." Branch 1 IS conclusive in the negative, and "no branch is conclusive in either direction" directly contradicts it. Stage 1 never gates, so this is calibration-only, but it leaves an operator incorrectly holding open "maybe the server rollout enabled fork mode anyway" when Agent's schema settles it. The two-part fix remains unchanged: (1) change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", (2) narrow "no branch is conclusive in either direction" to "the remaining two branches are not conclusive in either direction."

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L109–110 — unchanged since 4a8e3ed, open across ten commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion applied when choosing the proof question. It is not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify its own dispatch message doesn't inadvertently embed the answer — in one generation pass, with no explicit backstop. For the minting path specifically, the minted value V is the most salient item in context at dispatch time; a dispatch that echoes it (even incidentally, in framing text) allows a non-inheriting canary to answer from the prompt alone. A false canary positive now fans out the full member wave — "Never drop a relevant corrector" means a wider set dispatches — and their fabricated ledgers all proceed to step-3 dedup and step-4 correction writes.

The one-sentence fix from nine prior review rounds remains absent. One sentence after the four properties closes it:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears rather than restating it.

Fix this →


Security IMPORTANT — Orphaned stop condition: no mandatory write-detection path.

SKILL.md L193–195 — open since 30895cd, unchanged.

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotcha at L354–359 describes write detection as advisory ("if you want assurance…"), with a robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop. The stop condition cannot fire under the current runbook; step 4 proceeds unconditionally. Adding one sentence makes the absent-detection state explicit rather than leaving the stop condition orphaned: "Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote."

Fix this →


Codex P2 (a382666) — "Re-running the whole pass clean" is still anchored.

SKILL.md L247–248 — unchanged from a382666.

where the batch can afford it, re-running the whole pass clean is worth more than one anchored ledger.

The Codex P2 from the last review pass flagged this: a whole-pass re-run in the same conversation still inherits the first pass's already-landed ledgers — so calling it "clean" is a misnomer. The re-run is less anchored than a single-member retry (each member sees all peers' ledgers rather than just the failed member's), but it is not clean by the independence standard the skill establishes. This framing persists in the current commit, which otherwise substantially improved the surrounding wave-sizing logic.

The fix is modest: qualify the guidance with an acknowledgment of the anchoring — "re-running the whole pass is worth more than one anchored ledger, though both runs in the same conversation share the same anchoring constraint: a fresh pass still inherits the first pass's ledgers." This keeps the practical preference while not misrepresenting the re-run as unanchored.

Fix this →


Other notes (no action required)

  • The limit distinction is correct and useful. Prior text implied MAX_TOOL_USE_CONCURRENCY would block dispatch; this commit correctly identifies that it only bounds concurrent execution and that MAX_CONCURRENT_SUBAGENTS is the spawn-failure boundary. Operators now have accurate information for sizing decisions.
  • "Wait for capacity" is intentionally unspecified as a mechanism. The harness will surface the spawn failure; the model can retry. Not specifying the retry loop at prose level is right for a skill of this kind.
  • The two remaining IMPORTANT security findings compound. The self-validation gap allows the canary itself to be spoofed; the orphaned stop condition means the correction pass runs unconditionally on whatever those members return. The self-validation gap's blast radius increases as the admitted set grows — "never drop a relevant corrector" means a false canary positive now fans out more members. Both require one sentence each.
  • Stage 1 F2 has appeared in every review pass across all ten commits. It is calibration-only, but at this point the cost of the two-part fix (change one phrase, narrow one sentence) is lower than the cost of re-explaining it in every subsequent pass.

Overall trajectory. Ten commits, nine of which closed findings from prior review passes. One Codex finding resolved in this commit (the relevance-drop prohibition). Three findings remain open: the self-validation gap and orphaned stop condition (both IMPORTANT, one sentence each), the "clean re-run" misnomer (minor, one qualifier), and the Stage 1 calibration issue (very low risk, two-part fix). All four require only prose additions.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (HEAD 1d3dfc3)

  • Gather context — PR diff and changed files at HEAD
  • Read sweep-all/SKILL.md at HEAD
  • Evaluate open findings from prior reviews
  • Analyze 1d3dfc3 for new or resolved security issues
  • Post findings

All changes are LLM skill instruction files (Markdown) and a JSON evals definition. Security surface is prompt-level — instructions that, if wrong, cause the model to fabricate and apply audit corrections to real files. No GitHub Actions workflow files changed. I defer to zizmor's lane for supply-chain, trigger patterns, and permissions.

What 1d3dfc3 did. The commit rewrites the wave-sizing section of sweep-all/SKILL.md: "gate the situational tier harder" is replaced with "Never drop a relevant corrector to fit a budget — membership decides scope; concurrency decides only timing. If the session cannot dispatch the whole set right now, wait for capacity and then dispatch it whole." The commit also corrects a documentation conflation between two distinct limits (CLAUDE_CODE_MAX_TOOL_USE_CONCURRENCY caps concurrent execution, not dispatch count; CLAUDE_CODE_MAX_CONCURRENT_SUBAGENTS is the hard spawn-failure limit).

Net direction is positive. The "gate the situational tier harder" instruction told the router to change relevance to satisfy a budget, which would silently drop covered findings from an explicitly requested sweep. That instruction is gone. The two IMPORTANT findings from prior security review passes remain open at HEAD.


IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since commit 4a8e3ed, ten review passes, unchanged at HEAD

SKILL.md L109–110:

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion the main thread applies when choosing the proof question. It is not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify that the dispatch message it is about to send doesn't inadvertently embed the answer — in one generation pass, with no explicit check instruction and no backstop.

For the minting path (the fallback for thin conversations, introduced in 61f0fcd), this exposure is heightened: the minted value V is the freshest, most salient item in context when the dispatch is composed. A dispatch that echoes or paraphrases V — even incidentally, in framing text — allows a non-inheriting canary to answer from the prompt alone. A false canary positive is the highest-consequence failure in this preflight: the proof is established, ALL members are fanned out in one wave (now the hardened default from e54696d), and their fabricated ledgers all proceed to step-3 dedup and step-4 correction writes — the exact outcome the preflight exists to prevent.

The one-sentence fix recommended across ten prior review passes is still absent. One sentence after the four properties converts property 2 from a selection rule into an enforceable pre-send check:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears ("the value in the most recent tool result") rather than restating it.

Fix this →


IMPORTANT — Orphaned stop condition: no mandatory write-detection path

Confidence: CONFIRMED — open since 30895cd, six review passes, unchanged at HEAD

SKILL.md L193–195:

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

SKILL.md L354–359 describes write detection as advisory ("If you want assurance…"), with a robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop condition. The runbook specifies no required path for determining that a fork wrote. The stop rule cannot fire under the current runbook; step 4 proceeds unconditionally — including in the case where a fork violated the no-writes rule.

The CHANGELOG correctly documents this as a deliberate deferral pending #1631. The ask from six prior review passes is unchanged: make the absent-detection state explicit in the runbook so the model doesn't treat the orphaned stop condition as implicitly triggered:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Other notes (no action required)

  • 1d3dfc3 resolves the Codex P2 finding from e54696d — "never drop a relevant corrector to fit a budget" is now stated as a rule, and the two documented limits are correctly distinguished with cited text. An 11-member fully-admitted set was never over the limit that causes a hard dispatch failure (MAX_CONCURRENT_SUBAGENTS, default 20), so the shrink instruction was defending against a non-existent problem while silently costing coverage. Correct and well-reasoned.
  • Stage 1 F2 calibration issue (unchanged across all commits): lines 76–83 say "Agent still carrying it means fork mode is not env-var-enabled" then "no branch is conclusive in either direction." If Agent carries run_in_background, both the env-var and server-side rollout are documented as removing it — Branch 1 is definitively conclusive-negative, contradicting the closing sentence. Since Stage 1 never gates, this is not a security issue, but it has appeared in every review pass.
  • Bootstrapping gap (prior IMPORTANT, substantially mitigated since 27f3fe8): the proof-only canary design and the one-wave default together substantially reduce the exposure. The canary returns only a bare Q&A pair (nothing that can anchor a later fork's audit), and all members are dispatched together before any ledger is in the transcript. Residual path requires partial inheritance (a fork with recent but not full-original context), distinct from the observed complete-non-inheritance failure mode. Retained as a disclosure only; not an actionable finding at current confidence.
  • The self-validation and orphaned-stop findings compound. The self-validation gap allows a false-positive canary to fan the full wave out; the orphaned stop condition means step 4 then proceeds unconditionally on whatever those forks return. Fixing one without the other leaves the preflight half-closed. Both require one sentence each.

Summary. 1d3dfc3 is a clean correctness improvement with no new security surface. Two IMPORTANT findings remain open across the full PR: the self-validation gap (ten rounds, one sentence) and the orphaned stop condition (six rounds, one sentence, deliberate deferral to #1631). Both are the last unfixed paths to a fabricated ledger reaching the step-4 writes.

…sation is more anchored, not less

The retry paragraph offered "re-running the whole pass clean" as the better
trade against one anchored ledger. That is backwards: a rerun dispatched from
this conversation inherits every ledger the first pass already landed, so it is
strictly more anchored than the single retry it was supposed to improve on.

There is no un-anchored rerun available in-session at all, and the skill now
says that instead of implying one exists. A retry is anchored by construction;
report it as such and weigh its ledger accordingly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit f6f330b)

  • Read changed files at HEAD
  • Diff against prior commit (1d3dfc3)
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit f6f330b ("drop the clean-rerun advice — a rerun in this conversation is more anchored, not less") makes a single targeted change to the retry paragraph in sweep-all/SKILL.md. One file changed.


Prior finding — resolved in this commit

Codex P2 (1d3dfc3) — "re-running the whole pass clean" misnomer.

Resolved. The prior text read:

where the batch can afford it, re-running the whole pass clean is worth more than one anchored ledger.

That was factually wrong: a whole-pass rerun in the same conversation inherits everything the first pass produced — more anchoring, not less. The replacement makes the constraint explicit:

There is no un-anchored rerun available inside this conversation — re-running the whole pass would inherit MORE, not less, since every ledger is already in the transcript any new fork copies. So do not offer a clean rerun; report each retried member's ledger as anchored and weigh it as such.

This is stronger than the suggested fix from the prior review (which proposed adding a qualifier). Removing the suggestion entirely and explaining why none is available is correct — a model following the old text would have offered the user a "clean rerun" that could not be clean, compounding the anchoring rather than reducing it. The replacement closes both the misnomer and the false-promise it carried.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts the closing sentence.

SKILL.md L76–83 — unchanged across all eleven commits.

So Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var path and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are definitively ruled out — fork mode is OFF, not merely "not env-var-enabled." Branch 1 IS conclusive in the negative, and "no branch is conclusive in either direction" directly contradicts it. Since Stage 1 never gates, this remains calibration-only, but has appeared in every one of eleven review passes. The two-part fix is unchanged: (1) change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", (2) narrow "no branch is conclusive in either direction" to "the remaining two branches are not conclusive in either direction."

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L109–110 — unchanged since 4a8e3ed, open across eleven commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion applied when choosing the proof question. It is not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify its own dispatch message doesn't inadvertently embed the answer — in one generation pass, with no explicit backstop. For the minting path, the minted value V is the freshest, most salient item in context at dispatch time; a dispatch that echoes it even incidentally allows a non-inheriting canary to pass, fanning out ALL members immediately.

The one-sentence fix from prior review rounds remains absent. One sentence after the four properties closes it:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears rather than restating it.

Fix this →


Security IMPORTANT — Orphaned stop condition: no mandatory write-detection path.

SKILL.md L193–195 — open since 30895cd, unchanged.

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotchas describe write detection as advisory ("If you want assurance…"), with the robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop. The rule says stop when a fork wrote — but the runbook specifies no required path for determining that a fork wrote. The stop condition cannot fire under the current runbook; step 4 proceeds unconditionally. Adding one sentence makes the absent-detection state explicit:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Other notes (no action required)

  • The "no un-anchored rerun" reasoning is accurate. In the same conversation, every completed ledger is in the transcript; a new fork spawned during a rerun inherits the first pass's ledgers plus any subsequent ones. A rerun in the same conversation is therefore strictly more anchored than a retry, not less. Removing the suggestion rather than qualifying it is the correct call.
  • CHANGELOG not updated for this commit. The retry-rule entry at line 90 describes intent ("failure now explicitly includes a ledger returned without verified inheritance proof") rather than the prose used. This micro-fix to the "clean rerun" misnomer is a correction within 0.10.0 scope; a separate CHANGELOG entry would be noise. No action needed.
  • The two remaining IMPORTANT security findings compound. The self-validation gap allows a false-positive canary to fan the full one-wave set out immediately; the orphaned stop condition means step 4 then proceeds unconditionally on whatever those forks return. Fixing one without the other leaves the preflight half-closed. Both require one sentence each.

Overall trajectory. Eleven commits, ten of which closed findings from prior review passes. One finding resolved in this commit (Codex P2: the "clean rerun" misnomer, removed and explained). Two IMPORTANT security findings remain open — the self-validation gap (eleven rounds) and the orphaned stop condition (seven rounds) — plus the Stage 1 F2 calibration issue. All three are prose additions; none require design changes.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (HEAD f6f330b)

  • Gather context and understand the PR
  • Read PR diff at HEAD f6f330b
  • Read sweep-all/SKILL.md at HEAD
  • Evaluate open findings from prior reviews (self-validation gap, orphaned stop condition)
  • Check for new security issues introduced by f6f330b
  • Post findings

All changes are LLM skill instruction files (Markdown). No GitHub Actions workflow files changed. Security surface is prompt-level — instructions that, if wrong, cause the model to fabricate and apply audit corrections to real files. I defer to zizmor's lane for supply-chain, trigger patterns, and permissions.

What f6f330b did. A single paragraph change in the retry section: "re-running the whole pass clean is worth more than one anchored ledger" is replaced by "There is no un-anchored rerun available inside this conversation — re-running the whole pass would inherit MORE, not less, since every ledger is already in the transcript any new fork copies. So do not offer a clean rerun; report each retried member's ledger as anchored and weigh it as such." This is factually correct and removes a false claim (the Codex P2 finding from the a382666 review). No new security surface is introduced.

Net direction of f6f330b is positive. The change closes the factual error without introducing new exposure. Two IMPORTANT findings from prior review passes remain open at HEAD.


IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since commit 4a8e3ed, eleven review passes, unchanged at HEAD

SKILL.md L109–110:

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion the main thread applies when choosing the proof question. It is not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify that the dispatch message it is about to send doesn't inadvertently embed the answer — in one generation pass, with no explicit check instruction and no backstop.

For the minting path (the thin-session fallback introduced in 61f0fcd), this exposure is the highest it has been: the minted value V is the freshest, most salient item in context at dispatch time. A dispatch that echoes or paraphrases V — even incidentally, in framing text — allows a non-inheriting canary to answer from the prompt alone. A false canary positive is the highest-consequence failure in this preflight: the proof is established, ALL members are fanned out in one wave, and their fabricated ledgers all proceed to step-3 dedup and step-4 correction writes — the exact outcome the preflight exists to prevent.

The one-sentence fix recommended across eleven prior review passes is still absent. One sentence after the four properties converts property 2 from a selection rule into an enforceable pre-send check:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears ("the value in the most recent tool result") rather than restating it.

Fix this →


IMPORTANT — Orphaned stop condition: no mandatory write-detection path

Confidence: CONFIRMED — open since commit 30895cd, seven review passes, unchanged at HEAD

SKILL.md L193–195:

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

SKILL.md L356–361 describes write detection as advisory ("If you want assurance…"), with a robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop condition. The runbook specifies no required path for determining that a fork wrote. The stop condition cannot fire; step 4 proceeds unconditionally — including when a fork violated the no-writes rule.

The CHANGELOG correctly documents this as a deliberate deferral pending #1631. The ask from seven prior review passes is unchanged: make the absent-detection state explicit in the runbook rather than leaving the stop condition orphaned. One sentence closes it:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Other notes (no action required)

  • f6f330b's retry change is correct. "There is no un-anchored rerun available inside this conversation" is an accurate statement of the constraint — a rerun dispatched from this conversation inherits every ledger already in the transcript, making it more anchored than a single retry, not less. Removing false guidance that pointed operators toward an escape that does not exist is the right call.
  • Stage 1 F2 calibration issue (lines 76–83, unchanged across all commits): "Agent still carrying it means fork mode is not env-var-enabled" then "no branch is conclusive in either direction." If Agent carries run_in_background, both the env-var and server-side rollout are documented as removing it — Branch 1 is definitively conclusive-negative, contradicting the closing sentence. Since Stage 1 never gates this is not a security issue, but it has appeared in every review pass.
  • Bootstrapping gap (prior IMPORTANT, substantially mitigated since 27f3fe8): the proof-only canary returns only a bare Q&A pair, and the one-wave default means all members are dispatched before any ledger is in the transcript. The residual partial-inheritance path is distinct from the observed complete-non-inheritance failure mode. Retained as a disclosure; not an actionable finding at current confidence.
  • The two open IMPORTANT findings compound. The self-validation gap allows a false-positive canary to fan the full wave out; the orphaned stop condition means step 4 then proceeds unconditionally on whatever those forks return. Fixing one without the other leaves the preflight half-closed. Both require one sentence each.

Summary. f6f330b closes one factual error (the "clean rerun" misnomer) and introduces no new security surface. Two IMPORTANT findings remain open across the full PR — the self-validation gap (eleven review passes, one sentence absent) and the orphaned stop condition (seven review passes, one sentence, deliberate deferral to #1631). Both are the last unfixed paths to fabricated content reaching the step-4 writes.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f6f330bb26

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md
Comment thread plugins/discipline/skills/sweep-all/SKILL.md
…etryable audit failure

The step quoted the documented behavior — `Concurrent subagent limit reached`
"tells Claude not to retry" — and then, four lines later, classified every
errored dispatch as retryable. The two instructions contradicted each other
exactly under a capacity race, and the retry would have been anchored as well,
since it spawns after other ledgers land.

That error is now excluded from the retry rule by name: wait for capacity and
dispatch, rather than retrying into the same wall.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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


Claude finished reviewing commit 84500fe

Code Review (commit 84500fe)

  • Gather context — diff against prior reviewed commit f6f330b
  • Read updated sweep-all/SKILL.md at HEAD
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit 84500fe ("a concurrent-limit error is a capacity race, not a retryable audit failure") makes a single targeted insertion to the retry paragraph. One file changed: SKILL.md (no CHANGELOG update, same pattern as f6f330b — a prose correction within 0.10.0 scope).


Prior finding — resolved in this commit

Codex P2 (f6f330b) — Exclude concurrent-limit errors from the retry rule.
Resolved. The prior text quoted the documented behavior ("the error tells Claude not to retry") in the wave-sizing section and then, four lines later in the retry rule, applied a blanket retry to all errored dispatches — two instructions that directly contradicted each other under a capacity race. The fix is precise: Concurrent subagent limit reached is now named by error text as explicitly non-retryable, with the correct action ("wait for capacity and dispatch, rather than retrying into the same wall"). The contrast with genuine audit failures (an unproven ledger, an errored fork) is preserved — retrying those is correct; retrying a capacity error is not.

The prose flow after insertion is clean. "...rather than retrying into the same wall. A retry is anchored by construction, one wave or many..." transitions correctly — the capacity case is handled by waiting, not retrying; the anchoring note that follows applies to any retry that does occur.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts the closing sentence.

SKILL.md L76–83 — unchanged across all twelve commits.

So Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var path and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are definitively ruled out — fork mode is OFF by either means, not merely "not env-var-enabled." Branch 1 IS conclusive in the negative, and "no branch is conclusive in either direction" directly contradicts it. Since Stage 1 never gates, this remains calibration-only. The two-part fix remains unchanged: (1) change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", (2) narrow "no branch is conclusive in either direction" to "the remaining two branches are not conclusive in either direction."

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L109–110 — unchanged since 4a8e3ed, open across twelve commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion, not a procedural step. The model must simultaneously formulate a compliant question AND verify its own dispatch message doesn't inadvertently embed the answer — in one generation pass, with no explicit backstop. For the minting path, the minted value V is the most salient item in context at dispatch time; a dispatch that echoes it allows a non-inheriting canary to pass, fanning out ALL members immediately. A false canary positive is the highest-consequence failure in the preflight.

The one-sentence fix recommended across twelve prior review passes remains absent. One sentence after the four properties closes it:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears rather than restating it.

Fix this →


Security IMPORTANT — Orphaned stop condition: no mandatory write-detection path.

SKILL.md L193–195 — open since 30895cd, eight review passes, unchanged at HEAD.

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotchas describe write detection as advisory ("if you want assurance…"), with the robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop. The stop condition cannot fire under the current runbook; step 4 proceeds unconditionally. The ask remains unchanged: one sentence making the absent-detection state explicit:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Codex P2 (f6f330b) — Per-session lifetime budget is not "capacity" that clears.

SKILL.md L225–234 — unaddressed since f6f330b.

The concurrent-limit fix in this commit correctly distinguishes MAX_CONCURRENT_SUBAGENTS (a simultaneous-spawn cap that clears as subagents finish) from MAX_SUBAGENTS_PER_SESSION (a session lifetime quota, documented default 200). The wave-sizing section acknowledges both at line 225 but conflates their recovery paths: "wait for capacity and then dispatch it whole" (line 234) is correct for the concurrent cap, where completing subagents free slots. It is not correct for the per-session budget, which is a one-way counter — waiting for concurrent capacity does not restore session quota.

Concrete path: a long session already holding 185 of its 200 lifetime slots dispatches a canary and expects to fan out a twelve-member batch. After the canary's slot (186), only 14 remain — enough for the fan-out in an otherwise-quiet session, but a session that has been active may be much closer to 200. "Wait for capacity" does not help when the session budget is the binding constraint; only a fresh session restores it.

The simplest close is a one-clause addition to the wait instruction: "If the binding constraint is the per-session lifetime budget rather than the concurrent cap — i.e. slots are free but the lifetime count is exhausted — waiting does not help; report that the audit cannot run in this session and request a fresh one."

Fix this →


Other notes (no action required)

  • The concurrent-limit exclusion is correctly scoped. The prior text said the error "tells Claude not to retry" but then included errored dispatches in the retryable set — a direct contradiction. The named exclusion closes it without over-specifying how to recover (the capacity-wait instruction was already present in the wave-sizing section).
  • No CHANGELOG update for this commit. Consistent with the f6f330b pattern — a prose-internal correction within 0.10.0.
  • Stage 1 F2 calibration issue has now appeared in twelve consecutive review passes. Both fixes are two-phrase edits and do not touch any other text; the cost of leaving it accrues in every future pass rather than in this one.
  • The two security IMPORTANT findings compound. The self-validation gap allows a false-positive canary to fan the full one-wave member set out; the orphaned stop condition means step 4 then proceeds unconditionally on whatever those forks return. Fixing one without the other leaves the preflight half-closed. Both require one sentence each.

Overall trajectory. Twelve commits, eleven of which closed findings from prior review passes. One Codex finding resolved in this commit (the concurrent-limit/retry contradiction). Three findings remain open: the self-validation gap and orphaned stop condition (both IMPORTANT, one sentence each), the per-session lifetime budget case (one clause, distinct from the concurrent-cap case the commit addressed), and the Stage 1 F2 calibration issue. All four are prose additions; none require design changes.

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (HEAD 84500fe)

  • Read PR diff and changed files
  • Review sweep-all/SKILL.md at HEAD
  • Review setup/SKILL.md
  • Evaluate open findings from prior reviews
  • Post findings

All changes are LLM skill instruction files (Markdown) and a JSON evals definition. Security surface is prompt-level — instructions that, if wrong, cause the model to fabricate and apply audit corrections to real files. No GitHub Actions workflow files changed. I defer to zizmor's lane for supply-chain, trigger patterns, and permissions.

What 84500fe did. A single sentence added to the retry rule: "Concurrent subagent limit reached, which the harness tells Claude not to retry — that is a capacity race, not a failed audit; wait for capacity and dispatch, rather than retrying into the same wall." This addresses the Codex P2 from the f6f330b review. The change is correct: treating a capacity-limit error as a retryable audit failure would produce an anchored fork that launched after other members' ledgers had already landed, and the harness's own directive not to retry was the right authority to cite.

Net direction of 84500fe is positive. No new security surface is introduced. Two IMPORTANT findings from prior review passes remain open at HEAD.


IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since commit 4a8e3ed, twelve review passes, unchanged at HEAD

SKILL.md L109–110:

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion the main thread applies when choosing the proof question. It is not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify that the dispatch message it is about to send does not inadvertently embed the answer — in one generation pass, with no explicit check instruction and no backstop.

For the minting path (the thin-session fallback at SKILL.md L123–131), this exposure is at its highest: the minted value V is the freshest, most salient item in context at dispatch time. A dispatch that echoes or paraphrases V — even incidentally in framing text — allows a non-inheriting canary to answer from the prompt alone. A false canary positive is the highest-consequence failure in this preflight: the proof is established, ALL members fan out in one wave (the hardened default), and their fabricated ledgers proceed to step-3 dedup and step-4 correction writes — the exact outcome the preflight exists to prevent.

The one-sentence fix from twelve prior review passes remains absent. One sentence after the four properties converts property 2 from a selection rule into an enforceable pre-send check:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears ("the value in the most recent tool result") rather than restating it.

Fix this →


IMPORTANT — Orphaned stop condition: no mandatory write-detection path

Confidence: CONFIRMED — open since commit 30895cd, eight review passes, unchanged at HEAD

SKILL.md L193–195:

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

SKILL.md L356–364 describes write detection as advisory ("If you want assurance…"), with a robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop condition. The runbook specifies no required path for determining that a fork wrote. The stop condition cannot fire; step 4 proceeds unconditionally — including when a fork violated the no-writes rule. The CHANGELOG correctly documents this as deliberate deferral pending #1631.

Making the absent-detection state explicit rather than leaving the stop condition orphaned requires one sentence:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Other notes (no action required)

  • 84500fe is correct. Classifying Concurrent subagent limit reached as a capacity race rather than a retryable audit failure is accurate — the harness explicitly tells Claude not to retry on that error, and a retry would produce an anchored fork that violates the independence invariant the one-wave preference was added to protect.
  • Stage 1 F2 calibration issue (lines 76–83, unchanged across all twelve commits): "Agent still carrying it means fork mode is not env-var-enabled" then "no branch is conclusive in either direction." If Agent carries run_in_background, both the env-var and server-side rollout are documented as removing it — Branch 1 is definitively conclusive-negative, contradicting the closing sentence. Not a security issue since Stage 1 never gates; noted for completeness.
  • Bootstrapping gap (prior IMPORTANT, substantially mitigated since 27f3fe8): the proof-only canary and one-wave default together reduce the primary exposure path. The residual partial-inheritance path is distinct from the observed complete-non-inheritance failure mode. Retained as a disclosure; not an actionable finding at current confidence.
  • The two open IMPORTANT findings compound. The self-validation gap allows a false-positive canary to fan the full wave out; the orphaned stop condition means step 4 proceeds unconditionally on whatever those forks return. Fixing one without the other leaves the preflight half-closed. Both require one sentence each.

Summary. 84500fe closes one Codex finding correctly with no new security surface. Two IMPORTANT findings remain open across the full PR — the self-validation gap (twelve rounds, one sentence) and the orphaned stop condition (eight rounds, one sentence, deliberate deferral to #1631). Both are the last unfixed paths to fabricated content reaching the step-4 writes.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 84500fe2b9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/discipline/skills/sweep-all/SKILL.md Outdated
…guess alike

The unguessable property listed "an exact count" as an example of a
high-entropy value. Small-domain counts — turns, files, findings — are exactly
what a non-inheriting fork can hit by chance, and because every fork gets the
same question and the same model, their guesses correlate: one lucky answer is
not one bad ledger, it is a pattern the others can repeat.

The example is removed and the correlation is stated. The instruction now points
at minting the value, which is unguessable by construction, rather than hunting
the history for something that looks specific.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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


Code Review (commit 11d0a82)

  • Read updated sweep-all/SKILL.md at HEAD
  • Diff 11d0a82 against 84500fe
  • Evaluate open findings from prior reviews
  • Check for new issues introduced by this commit
  • Post review

Commit 11d0a82 ("an exact count is not high entropy, and blind forks guess alike") makes a single targeted replacement inside the fourth proof property. One file changed: SKILL.md. No CHANGELOG update, consistent with the prior two prose-correction commits (f6f330b, 84500fe).


Prior finding — resolved in this commit

Codex P1 (84500fe) — Remove exact counts from inheritance-proof examples.
Resolved. "an exact count" is gone from the list of acceptable proof values, and the replacement explains the specific failure mode that makes it wrong: blind forks guess alike. Because every fork receives the same question and runs on the same model, their independent guesses are correlated — a correct answer from a non-inheriting canary is not an isolated event but a repeatable outcome for the members that follow. Small-domain values (turn count, file count, finding count) are named explicitly, and the instruction points to the mint path as the preferred resolution when history lacks something long and specific. The reasoning now closes the gap the example opened: picking a value that looks exact but lives in a small domain was satisfying the property's letter while violating its intent.


Prior findings — still open

Code Review F2 — Stage 1 Branch 1 understates and contradicts the closing sentence.

SKILL.md L76–83 — unchanged across all thirteen commits.

So Agent still carrying it means fork mode is not env-var-enabledno branch is conclusive in either direction.

Both the env-var path and the server-side rollout are documented as removing run_in_background from the Agent tool. If Agent carries the parameter, both mechanisms are definitively ruled out — fork mode is OFF by either means, not merely "not env-var-enabled." Branch 1 IS conclusive in the negative, and "no branch is conclusive in either direction" directly contradicts it. Stage 1 never gates, so this is calibration-only — noted for completeness across thirteen consecutive review passes. The two-part fix is unchanged: (1) change "not env-var-enabled" → "off (neither the env var nor the server-side rollout is active)", (2) narrow "no branch is conclusive in either direction" → "the remaining two branches are not conclusive in either direction."

Fix this →


Security IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check.

SKILL.md L109–110 — unchanged since 4a8e3ed, open across thirteen commits.

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion applied when choosing the proof question. It is not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify its own dispatch message doesn't inadvertently embed the answer — in one generation pass, with no explicit backstop. The correlation finding in this commit makes the exposure worse, not better: with small-domain values now ruled out, the only acceptable proof is a long verbatim string or a minted value — both of which are the most salient items in context at dispatch time, making accidental embedding more likely.

A false canary positive fans ALL members out in one wave, and their fabricated ledgers proceed to step-3 dedup and step-4 writes. One sentence after the four properties converts property 2 from a selection constraint into an enforceable pre-send check:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears rather than restating it.

Fix this →


Security IMPORTANT — Orphaned stop condition: no mandatory write-detection path.

SKILL.md L197–198 — open since 30895cd, nine review passes, unchanged at HEAD.

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

The Gotchas describe write detection as advisory ("if you want assurance…"), with the robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop. The stop condition cannot fire under the current runbook; step 4 proceeds unconditionally. One sentence makes the absent-detection state explicit rather than leaving the stop condition orphaned:

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Codex P2 (84500fe) — Per-session lifetime budget is not "capacity" that clears.

SKILL.md L228, L237 — unaddressed since f6f330b, carried across two commits.

The wave-sizing section correctly acknowledges CLAUDE_CODE_MAX_SUBAGENTS_PER_SESSION (documented default 200), but the fallback instruction "wait for capacity and then dispatch it whole" conflates two different limit types. Waiting for concurrent capacity (subagents finishing) frees slots against MAX_CONCURRENT_SUBAGENTS. It does not restore MAX_SUBAGENTS_PER_SESSION, which is a one-way lifetime counter. A session that has already consumed 188 of its 200 slots cannot dispatch a canary plus a twelve-member batch, and waiting for the currently-running subagents to finish does not help — the lifetime count is exhausted regardless. One added clause closes it: "If the binding constraint is the per-session lifetime budget rather than the concurrent cap — slots are free but the lifetime count is exhausted — waiting does not help; report that the audit cannot run in this session and request a fresh one."

Fix this →


Other notes (no action required)

  • The correlation reasoning is the right addition. Prior text required a high-entropy answer but said nothing about why correlated guesses multiply the exposure — a single lucky canary answer that works for one fork works for all eight, since they all see the same question with the same model. Making this explicit improves both the rule's persuasiveness and the gap it leaves in small-domain examples.
  • "Mint the value" as the resolution for doubt is now stated directly in the fourth property, not just in the mint-one paragraph below. This gives an operator with any uncertainty a clear escape hatch without reading ahead.
  • No CHANGELOG update. Consistent with the f6f330b / 84500fe pattern — a prose correction within 0.10.0 scope. No action needed.
  • The two remaining security IMPORTANT findings compound. The self-validation gap allows a false-positive canary to fan the full one-wave set out immediately; the orphaned stop condition means step 4 then proceeds unconditionally on whatever those forks return. Fixing one without the other leaves the preflight half-closed. Both require one sentence each.

Overall trajectory. Thirteen commits, twelve of which closed findings from prior review passes. One Codex finding resolved in this commit (the exact-count example). Three findings remain open: the self-validation gap and orphaned stop condition (both Security IMPORTANT, one sentence each), the per-session lifetime budget case (one clause), and the Stage 1 F2 calibration issue (two-phrase edit, thirteen review passes). All four are prose additions; none require design changes.

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1628 (HEAD 11d0a82)

  • Gather context and load prior review history
  • Read PR diff at HEAD
  • Read sweep-all/SKILL.md at HEAD
  • Evaluate open findings from prior reviews
  • Analyze latest commit for new or resolved security issues
  • Post findings

All changes are LLM skill instruction files (Markdown) and a JSON evals definition. Security surface is prompt-level — instructions that, if wrong, cause the model to fabricate and apply audit corrections to real files. No GitHub Actions workflow files changed. I defer to zizmor's lane for supply-chain, trigger patterns, and permissions.

What 11d0a82 did. The fourth proof property ("It cannot be guessed") is tightened: "an exact count" is removed from the examples of acceptable high-entropy values, small-domain values are explicitly named as failing the test ("a turn count, a file count, a finding count are all guessable"), and the correlated-guess reasoning is added ("blind forks guess alike, since they share the question and the model, so one lucky answer is not one bad ledger"). The paragraph ends with a positive steer: "when in doubt mint the value rather than picking one out of the history." This directly addresses the Codex P1 from the 84500fe review. The change is correct and well-reasoned.

Net direction of 11d0a82 is positive. The correlated-guess hole is closed; the mint-by-default steer is the right guidance. No new security surface is introduced. Two IMPORTANT findings from prior review passes remain open at HEAD.


Resolved in this commit

Codex P1 (84500fe) — Remove exact counts from high-entropy proof examples.
Resolved. An exact count (turn count, file count, finding count) is a small-domain value that satisfies the "exists only in history" property but not the "cannot be guessed" property — and worse, because all forks share the same model and the same question, one lucky guess is not one bad ledger: all non-inheriting forks make the same guess. The prior text listing "an exact count" as an acceptable example could have produced a pass where every blind fork answered correctly by convergence, and all their fabricated ledgers proceeded to step 4. The fix removes the example, names the failure class, and adds the correlated-guess reasoning that makes small-domain values worse than a naive read suggests.

The "when in doubt mint the value" steer is also the right residual guidance: minting gives guaranteed entropy without the uncertainty of evaluating whether a historical detail is high-entropy enough.


IMPORTANT — Canary self-validation gap: property 2 is a constraint, not a procedural check

Confidence: PLAUSIBLE — open since commit 4a8e3ed, twelve review passes, unchanged at HEAD

SKILL.md L109–110:

The dispatch prompt neither contains nor paraphrases the answer — else a non-inheriting subagent answers it from the prompt alone.

This is a selection criterion the main thread applies when choosing the proof question. It is not a procedural step the main thread takes before sending the dispatch. The model must simultaneously formulate a compliant question AND verify that the dispatch message it is about to send does not inadvertently embed the answer — in one generation pass, with no explicit check instruction and no backstop.

The "when in doubt mint the value" guidance added in this commit makes this exposure more likely to matter, not less: minting is now the recommended default when proof quality is uncertain, meaning V will be minted more often. V is the freshest, most salient item in context at dispatch time. A dispatch that echoes or paraphrases V — even incidentally, in framing text — allows a non-inheriting canary to answer from the prompt alone. A false canary positive is the highest-consequence failure in this preflight: the proof is established, ALL members fan out in one wave, and their fabricated ledgers proceed to step-3 dedup and step-4 correction writes — the exact outcome the preflight exists to prevent.

The one-sentence fix recommended across twelve prior review passes is still absent. One sentence after the four properties converts property 2 from a selection rule into an enforceable pre-send check:

Before sending the canary dispatch, verify on the main thread that the chosen question's expected answer cannot be derived from the dispatch message you are about to send. If it can, choose a different question — or for a minted value, ensure the dispatch references WHERE it appears ("the value in the most recent tool result") rather than restating it.

Fix this →


IMPORTANT — Orphaned stop condition: no mandatory write-detection path

Confidence: CONFIRMED — open since 30895cd, eight review passes, unchanged at HEAD

SKILL.md L197–198:

Their no-writes rule is trusted, not enforced — see Gotchas, and treat a fork that wrote as untrusted output: stop rather than correct on top of it.

SKILL.md L355–367 describes write detection as advisory ("If you want assurance…"), with a robust mechanism deferred to #1631. No mandatory step exists between "forks return ledgers" and "dedup / correct" that triggers the stop condition. The runbook specifies no required path for determining that a fork wrote. The stop condition cannot fire; step 4 proceeds unconditionally — including when a fork violated the no-writes rule.

The CHANGELOG correctly documents this as a deliberate deferral pending #1631. The ask from eight prior review passes is unchanged: make the absent-detection state explicit rather than leaving the stop condition orphaned, so the model does not treat an unfired rule as implicit confirmation that no fork wrote.

Where no write-detection check is available (the state-digest script from #1631 has not shipped), report that the no-writes rule is unverified for this run and note it in the step-5 report — do not treat its absence as confirmation that no fork wrote.

Fix this →


Other notes (no action required)

  • Stage 1 F2 calibration issue (lines 76–83, unchanged across all thirteen commits): "Agent still carrying it means fork mode is not env-var-enabled" then "no branch is conclusive in either direction." If Agent carries run_in_background, both the env-var and server-side rollout are documented as removing it — Branch 1 is definitively conclusive-negative, contradicting the closing sentence. Stage 1 never gates; this is not a security issue, but it has appeared in every review pass.
  • Bootstrapping gap (prior IMPORTANT, substantially mitigated since 27f3fe8): the proof-only canary returns only a bare Q&A pair and the one-wave default means all members are dispatched before any ledger lands in the transcript. The residual partial-inheritance path is distinct from the observed complete-non-inheritance failure mode. Retained as a disclosure; not an actionable finding at current confidence.
  • The two open IMPORTANT findings compound. The self-validation gap allows a false-positive canary to fan the full wave out; the orphaned stop condition means step 4 proceeds unconditionally on whatever those forks return. Fixing one without the other leaves the preflight half-closed. Both require one sentence each.
  • The "when in doubt mint" addition is the right direction. It steers operators toward mechanically guaranteed entropy rather than searching history for details that might not clear the high-entropy bar. Its interaction with the self-validation gap is why that gap matters more at this HEAD than at any prior one.

Summary. 11d0a82 resolves the correlated-guess Codex P1 correctly and introduces no new security surface. Two IMPORTANT findings remain open across the full PR — the self-validation gap (twelve review rounds, one sentence absent) and the orphaned stop condition (eight review rounds, one sentence, deliberate deferral to #1631). Both are the last unfixed paths to fabricated content reaching the step-4 writes.

@kyle-sexton
kyle-sexton merged commit f6fda80 into main Jul 27, 2026
31 checks passed
@kyle-sexton
kyle-sexton deleted the fix/discipline-sweep-all-fork-preflight branch July 27, 2026 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment